install: remove the same-filesystem probe from $TMPDIR when the rename into the cache fails - #38859
install: remove the same-filesystem probe from $TMPDIR when the rename into the cache fails#38859robobun wants to merge 2 commits into
Conversation
…e into the cache fails get_temporary_directory_run creates an empty .<hex>-N.hm file in the tempdir and renames it into the cache directory to check that both live on the same filesystem. When the rename fails (EXDEV when $TMPDIR and the cache are on different mounts), it switches to <cache>/.tmp and retries, but only ever unlinks the retry's probe from the cache directory. The first probe stayed behind in $TMPDIR, one per bun install. Unlink the probe from the directory it was created in before moving on.
|
Warning Review limit reached
Next review available in: 3 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — two-line cleanup on the rename-failure path plus a targeted regression test.
What was reviewed:
- Traced
get_temporary_directory_run: onrenameat_zfailure the source stays intempdir, and the newdelete_file_zmirrors the existing success-path unlink of the same probe fromcache_directory. - Test: confirmed
findCrossDeviceDircompares againstos.tmpdir()(where harnesstempDirroots the cache), so the EXDEV precondition holds; asserts<cache>/.tmpexists so the fallback provably ran; hermeticfile:dep, no network;mkdtempSyncon/dev/shmis unavoidable here and is removed infinally. - Checked the
create_file_z-failure branch above — no probe exists there, so no analogous unlink is needed.
Extended reasoning...
Overview
Two-line change to get_temporary_directory_run in src/install/PackageManager/PackageManagerDirectories.rs: when the renameat_z probe from $TMPDIR into the cache directory fails (typically EXDEV), unlink the probe from the directory it was created in before either falling back to <cache>/.tmp or crashing. Previously the only unlink was on the success path against cache_directory, so the first-iteration probe leaked in $TMPDIR. Adds test/cli/install/bun-install-tmpdir-exdev.test.ts, which forces the cross-device layout via /dev/shm (or /tmp when the harness tmpdir is elsewhere), runs an install with a local file: dependency, and asserts both that the fallback ran (<cache>/.tmp exists) and that the $TMPDIR directory is empty afterward.
Security risks
None. The change adds a best-effort unlinkat of a file bun itself just created, using the same fd and name it created with. No new inputs, paths, or trust boundaries.
Level of scrutiny
Low. A failed rename(2) leaves the source in place by contract, so the probe is guaranteed to still be at (tempdir, tmpname) on this branch — the unlink is the obvious symmetric cleanup and uses the identical let _ = <dir>.delete_file_z(tmpname) shape as the existing success-path cleanup ~30 lines below. The unlink runs before tempdir is reassigned to the fallback Dir, so it targets the correct directory. On the second loop iteration (fallback also fails to rename) the extra unlink is harmless — bun crashes immediately after. The other early-fallback branch (probe creation fails) needs no unlink because no file was created.
Other factors
The test is well-constructed against the repo's review rules: it asserts its own precondition (<cache>/.tmp exists proves the EXDEV path actually executed, so the test cannot pass vacuously on a same-device layout), it is hermetic (local file: dependency, no registry), it drains stdout/stderr/exit concurrently, asserts output before exit code, spreads bunEnv, and skipIfs cleanly on platforms without a writable cross-device mount (Windows, typical macOS). The mkdtempSync on /dev/shm deviates from the "use harness tempDir" guidance, but that is unavoidable — the directory must be on a specific other device — and it is removed in a finally registered before the assertions. The new file follows the established test/cli/install/bun-install-<topic>.test.ts pattern alongside a dozen siblings.
There was a problem hiding this comment.
LGTM — the probe cleanup is placed before tempdir is reassigned, so it unlinks from the directory the file was actually created in, and it mirrors the existing cache_directory.delete_file_z(tmpname) on the success path.
What was reviewed
- Traced the
'brkloop: on rename failure the source file is guaranteed to still exist intempdir; the new unlink runs before the.tmpfallback reassignment and also before thetried_dot_tmpcrash path, so both failure exits are covered. - Checked the sibling
create_file_zerror arm — no probe exists there, so no cleanup needed. - Test:
mkdtempSyncon/dev/shmis a justified exception to the harness rule (needs a different device thantempDir), cleaned up infinally; theexistsSync(cache/.tmp)assertion proves the EXDEV fallback actually ran; pipes drained concurrently;skipIfcovers Windows/macOS where no cross-device candidate exists.
Extended reasoning...
Overview
Two-line fix in get_temporary_directory_run (src/install/PackageManager/PackageManagerDirectories.rs): when sys::renameat_z fails while probing whether $TMPDIR and the install cache share a filesystem, unlink the probe from the tempdir it was created in before either falling back to <cache>/.tmp or crashing. Previously only the success path unlinked (from the cache directory), so a cross-device $TMPDIR leaked one empty .hm file per bun install. A new test forces EXDEV via /dev/shm (or /tmp when it differs from the harness tempdir) and asserts the tempdir is empty afterward.
Security risks
None. The change adds a best-effort delete_file_z of a file bun itself just created, keyed by the same tmpname used for creation, in a directory bun already has an open fd to. No new inputs, no path construction from untrusted data.
Level of scrutiny
Low. This is a targeted resource-cleanup fix on an error path, using the same helper (delete_file_z) and same ignore-result pattern (let _ =) as the adjacent success-path cleanup three lines below. POSIX rename(2) guarantees the source is untouched on failure, so the file is always present to unlink; the result is discarded so a lost race is harmless. The unlink is placed before tempdir is reassigned to the .tmp fallback, so it targets the correct directory. On the second-iteration failure (rename fails inside .tmp too), the same line cleans up before Global::crash(), which is strictly better than before.
Other factors
The PR includes fails-without-fix / passes-with-fix evidence on both ASAN debug and release builds. The test follows harness conventions (bunEnv spread, tempDir, concurrent pipe drain, exit-code asserted last, cleanup in finally registered before assertions, no network). It uses mkdtempSync directly on /dev/shm rather than the harness tempDir — necessary here because the whole point is a directory on a different device than the harness tempdir, and it is removed in finally. The existsSync(join(cache, ".tmp")) assertion guards against a false pass by proving the EXDEV fallback branch actually executed. skipIf(!crossDeviceDir) means it only runs where a writable cross-device mount exists (Linux CI); the fix itself is platform-agnostic.
|
CI status: the red check is queue capacity, not a test failure.
Retrying the two expired jobs in Buildkite once the pool catches up should turn the check green; I am not going to push further retriggers since they only add to that queue. |
2 participants
Problem
bun installleaves one empty$TMPDIR/.<hex>-N.hmfile behind when$TMPDIRand the install cache are on different filesystems (tmpfs/tmpwith the cache on disk, or aBUN_INSTALL_CACHE_DIRon another mount). They accumulate until the tmpfs is cleared; a long-running fuzzer left ~76,000 of them in three hours.get_temporary_directory_runinsrc/install/PackageManager/PackageManagerDirectories.rscreates the probe in the tempdir andrenameats it into the cache directory (line 242). When the rename fails it switchestempdirto<cache>/.tmpand retries, and the only unlink in the loop (line 277) removes the retry's probe from the cache directory. The first probe is still in$TMPDIRand nothing removes it.Fix
renameleaves the source in place, so the probe is always still intempdiron that path; the unlink result is ignored like the existing cache-directory unlink.test/cli/install/bun-install-tmpdir-exdev.test.tspointsTMPDIR/BUN_TMPDIRat a directory on a different device (/dev/shm, or/tmpwhen the harness temp dir is elsewhere; skipped when neither differs, which covers Windows and the usual macOS layout), installs afile:dependency with the cache inside the test directory, and asserts that<cache>/.tmpwas created (the fallback actually ran) and that theTMPDIRdirectory is empty afterwards. It fails on the released build with the leaked.hmfile listed and passes with this change.$TMPDIRin the cross-device layout, and the same-filesystem layout still removes its probe and does not create<cache>/.tmp.Background
bun installextracts each package into a temp directory and thenrenames it into the cache, so the tempdir must be on the same filesystem as the cache (renameacross filesystems fails withEXDEV). The probe described above is how bun detects this once per process; on failure it uses<cache>/.tmpas the tempdir instead.FileSystem::tmpnameproduces the.<hex>-<counter>.<ext>names; the probe uses thehmextension.[review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file