install: don't leak package extraction temp directories into $TMPDIR - #33979
install: don't leak package extraction temp directories into $TMPDIR#33979robobun wants to merge 7 commits into
Conversation
When two bun install processes raced the same cache entry, the loser's rename fell back to RENAME_EXCHANGE, which swapped its temp directory with the winner's cache entry and left the swapped-out tree stranded in the temp dir forever. On long-lived machines this accumulated a full extracted package copy per raced install (reported at ~25 GB over 40 hours of CI). Deleting the swapped-out tree after the exchange is not safe either: the winner may still hold an open fd into that tree while hardlinking it into node_modules, and deleting it under the winner fails its install with ENOENT. Instead, treat the publish race as first writer wins: a new keep_existing_destination option on renameat_concurrently keeps the existing (equivalent) destination and deletes the caller's private temp copy, which no other process can be reading. Also clean up the temp directory on extraction and patch-apply error paths, which previously leaked a partially extracted tree per failed attempt. Fixes #33977
|
Updated 10:27 AM PT - Jul 11th, 2026
❌ @robobun, your commit c0b8c8d has 4 failures in
🧪 To try this PR locally: bunx bun-pr 33979That installs a local version of the PR into your bun-33979 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
WalkthroughChangesThe install pipeline now cleans temporary extraction and patch directories on failure, preserves destinations during concurrent cache publication, initializes rename options with defaults, and adds coverage for concurrent installs and cleanup after extraction or patch failures. Install cleanup and concurrent publication
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/cli/install/bun-install-tempdir-cleanup.test.ts`:
- Around line 122-219: Mark all three independent tests—“concurrent installs
sharing a cache do not leak temp directories,” “a tarball that fails to extract
does not leak its temp directory,” and “a patch that fails to apply does not
leak its temp directory”—as concurrent tests using the repository’s established
test.concurrent pattern. Preserve their isolated temp directories, registries,
and existing assertions.
- Around line 179-184: Reorder the assertions in the install cleanup test so the
readdirSorted(tmpDir) filesystem validation runs before the exitCode assertion.
Keep the stderr assertion where it is, and make expect(exitCode).not.toBe(0) the
final assertion in the test.
- Around line 215-219: Update the test around runInstall to destructure exitCode
alongside stderr, then assert that exitCode is non-zero (or matches the expected
failure code) in addition to checking the patch error message and
temporary-directory cleanup. Keep the existing stderr and readdirSorted
assertions unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a0b4c152-430e-45dc-99c6-1458281ee815
📒 Files selected for processing (5)
src/install/PackageManager/patchPackage.rssrc/install/extract_tarball.rssrc/install/patch_install.rssrc/sys/lib.rstest/cli/install/bun-install-tempdir-cleanup.test.ts
The Windows arm of move_to_cache_directory handled a destination that already exists by renaming it out of the cache and deleting it, then retrying its own move. When the existing entry was published by a concurrent install that is still copying from it, this yanks the tree out from under that process and fails it with ENOENT (#28062). Apply the same first-writer-wins rule as the POSIX path: if the destination directory exists, delete our own temp copy and use the existing entry. This deletes the rename-out-and-delete fallback block. Also address review feedback on the tests: run them concurrently, assert the patch failure exit code, and move exit-code assertions after filesystem checks.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
When the filesystem lacks RENAME_NOREPLACE (EOPNOTSUPP), the errno does not say whether the destination exists, and the fallback deleted it unconditionally before renaming. With keep_existing_destination set, check for the destination first and keep it, deleting the source instead, so the option's contract holds on those filesystems too.
|
Checked both suggested issues: #28062: confirmed related. The Windows arm of move_to_cache_directory handled a publish collision by renaming the existing cache entry out of the cache and deleting it, which is exactly the mechanism that issue's analysis points at (and the same failure I reproduced on Linux when testing a delete-based variant of this fix: the winning install fails with ENOENT while copying from the entry it just published). Commit 120895b applies first-writer-wins on Windows too and removes that fallback, so I added Fixes #28062 to the description. My concurrent-install test did not trigger the ENOENT on an unfixed Windows release build in 6 runs (the race window there is narrow), so the claim rests on the mechanism being removed rather than on a reproduced failure. #11250: only partially addressed. The stranded temp directories after a failed move are now cleaned up by the new error-path guards, but the underlying EPERM from NtSetInformationFile is not fixed here, so I did not add it as auto-close. |
keep_existing_destination assumed an existing cache entry is always an equivalent copy from a concurrent install, but package_missing_from_cache treats an npm entry without package.json as absent and re-extracts to heal it. Keeping such an entry broke that heal (caught by bun-install-registry.test.ts on the Windows lanes). Delete an existing npm entry that is missing package.json before publishing the fresh copy; no process reads from an entry that fails the cache check. Also make the keep_existing_destination fallback existence check directory-aware: Windows exists_at returns true only for files, so the guard never fired for directory destinations there.
Windows exists_at is file-only, so the assertion was vacuous there.
There was a problem hiding this comment.
No blocking issues found after four rounds of feedback (all addressed), but this changes the concurrent cache-publish semantics from swap to first-writer-wins across Linux/macOS/Windows and rewrites the Windows collision path in extract_tarball.rs, so it's worth a human look at the design.
Reviewed: the new keep_existing_destination path in renameat_concurrently_without_fallback (default-off callers keep old RENAME_EXCHANGE behavior; all call sites are in this PR's files). Checked the scopeguard placement in extract/apply disarms only on success and covers every early return. Checked the Windows arm's directory_exists_at_w + delete-source replaces the old rename-out-and-delete without leaving the retry loop unbounded. The narrow TOCTOU on the invalid-entry pre-delete (flagged inline earlier) remains as an accepted followup.
Extended reasoning...
Overview
Five files touched: src/sys/lib.rs gains a keep_existing_destination flag on RenameatConcurrentlyOptions and a new branch in renameat_concurrently_without_fallback that deletes the source on EEXIST/ENOTEMPTY (and, after follow-up commits, on the racy fallback path via a directory-aware existence check). src/install/extract_tarball.rs and src/install/patch_install.rs opt into that flag, add scopeguard-based cleanup of the extraction/patch temp dir on every failure exit, and (on Windows) replace the rename-out-and-delete fallback with first-writer-wins. src/install/PackageManager/patchPackage.rs is purely mechanical ..Default::default() additions required by the new struct field. A new integration test file plus a bun_sys unit test cover the concurrent-race, corrupt-tarball, failed-patch, and heal-invalid-entry cases.
Security risks
None identified. The change operates on Bun's own private temp directories and package cache; no new user-controlled paths are introduced, and the deleted trees are the caller's own freshly-created temp copies. The delete_tree targets are derived from FileSystem::tmpname / cached_*_folder_name_print, not from tarball contents.
Level of scrutiny
High. This is production package-manager code on a hot path (bun install), it changes the concurrency contract of a shared syscall helper, and it has per-platform arms (Linux renameat2 flags, Windows NtSetInformationFile errno handling, an EOPNOTSUPP fallback for NFS/FUSE). The design decision — keep the existing cache entry rather than swap or replace it — trades the old behavior's temp-dir leak for a narrow TOCTOU on the invalid-entry pre-delete (flagged inline as non-blocking; requires a pre-corrupted cache entry plus two installs preempted between adjacent syscalls). That trade-off is reasonable but should be a maintainer's call, not an automated approval.
Other factors
I've reviewed this PR across four prior runs; each raised a specific gap (EOPNOTSUPP fallback bypassing the guard; Windows exists_at being file-only in both the production sad path and the unit test; the pre-delete TOCTOU) and each was fixed or acknowledged in a follow-up commit (f55cf7e, 9943a49, 61c7ea4). All CodeRabbit test-convention nits are resolved. Every call site of renameat_concurrently* lives in files this PR touches, so the signature change has no untouched consumers. The evidence block shows the new tests fail on main and pass on the PR under both ASAN-debug and release builds. Given the cross-platform surface and the semantic shift, deferring to a human reviewer.
There was a problem hiding this comment.
No bugs found, but this changes concurrent cache-publish semantics in renameat_concurrently (first-writer-wins replacing the RENAME_EXCHANGE fallback) and rewrites the Windows collision arm in extract_tarball.rs, so it's worth a human look at the cross-process behavior.
Checked: the new keep_existing_destination path deletes only the caller's private temp copy on EEXIST/ENOTEMPTY and on the sad-path fallback; the bun patch working-dir renames in patchPackage.rs correctly do not opt in; the extract/patch scopeguards are defused only after the rename succeeds; move_file_z and the existing rename callers keep their old semantics via Default::default(). The earlier inline nits (EOPNOTSUPP fallback, Windows file-only exists_at, vacuous unit-test assertion) were all addressed in f55cf7e / 9943a49 / 61c7ea4. One acknowledged narrow TOCTOU remains in the invalid-cache-entry pre-delete as a followup.
Extended reasoning...
Overview
The PR stops bun install from leaking package-extraction temp directories into $TMPDIR by (1) adding a keep_existing_destination option to renameat_concurrently that, on a publish collision, keeps the existing cache entry and deletes the caller's private temp copy instead of swapping/replacing it, (2) opting the tarball-extract and patch-apply paths into that option, (3) wrapping both extraction paths in scopeguards so error exits also clean up the temp dir, and (4) rewriting the Windows collision arm of move_to_cache_directory to the same first-writer-wins policy (removing the old rename-out-and-delete fallback). It also adds a pre-delete of npm cache entries that lack package.json so a corrupt entry isn't preserved forever by the new keep-existing semantics. ~410 lines across src/sys/lib.rs, src/install/extract_tarball.rs, src/install/patch_install.rs, src/install/PackageManager/patchPackage.rs, plus a new 4-test integration file and a Rust unit test.
Security risks
None identified. The change operates on Bun-created temp directories under $BUN_TMPDIR and the install cache; no new user-controlled paths reach delete_tree. The patchPackage.rs hunks are purely mechanical ..Default::default() additions.
Level of scrutiny
High. This is production-critical package-manager code with cross-process concurrency semantics on two very different platforms. The signature of the shared renameat_concurrently_without_fallback helper changed, and the Windows extract arm was substantially rewritten. Three rounds of prior review on this PR each caught a real (if narrow) semantic gap — EOPNOTSUPP falling through to the destructive sad path, Windows exists_at being file-only so the sad-path guard was a no-op there, and the same issue in the unit test — which is a signal that the interactions here are subtle enough to merit human eyes on the final state.
Other factors
- All prior review comments (CodeRabbit and mine) are resolved; the diff reflects the fixes.
- One inline note I left on
extract_tarball.rs(a narrow check-then-act TOCTOU when two installs both encounter a pre-existing corrupt cache entry) was explicitly called out as a non-blocking followup and left as-is; that's a reasonable trade-off but a maintainer should confirm. - Tests are hermetic (local in-process registry, per-test tempDir/cache), use
test.concurrent, and were shown to fail on the unfixed build. The concurrent-install test relies on timing to hit the race and iterates 5× to make it reliable; that's inherently probabilistic but the assertion ("tmp dir is empty afterwards") can't false-fail if the race isn't hit. - The
RENAME_EXCHANGEfallback branch inrenameat_concurrently_without_fallbackis now only reachable whenkeep_existing_destinationis false — a maintainer may want to weigh in on whether any remaining caller actually wants the old swap-and-strand behavior, or whether that branch should be removed too.
|
CI status across the last three runs: every install-related suite is green (bun-install-registry, bun-install-patch, bun-patch, bun-install-streaming-extract, bun-install-tarball-integrity, and the new bun-install-tempdir-cleanup tests). The remaining red lanes are unrelated, pre-existing flaky tests that change from run to run (napi on Windows baseline, worker-message-port and fetch-backpressure, then no-orphans on macOS 26 and net-mongodb-pattern-leak), each already tracked separately. The diff itself is ready for review. |
|
One interaction to be aware of when reviewing this: keeping the existing destination unconditionally is not right for #38702 fixes that leak for tarballs by deciding before extraction whether the folder already existed ( |
|
Another data point for this one, from a report against current main (ada2a67), plain npm packages only: four cold For whoever reviews: this PR, #36229 and #38702 each merge cleanly onto main today, but any two of them conflict with each other in |
1 participant
Fixes #33977
Fixes #28062
Problem
bun installleaks package-extraction temp directories (e.g..5dfdff2c5fefbff7-00000006.tmp,.79dbbadf3f7fb2bb-7.react-native) into $TMPDIR. On long-lived CI containers the reporter measured ~25 GB over 40 hours.Reproduction: run two concurrent
bun installprocesses sharing the sameBUN_INSTALL_CACHE_DIR. Every package whose cache publish they race leaks one fully extracted copy per install, even though both installs exit 0. Error paths (a tarball that fails to extract, a patch that fails to apply) also leak one temp directory per attempt.Cause
Extraction writes the package into a temp dir, then publishes it into the cache via
renameat_concurrently(renameat2withRENAME_NOREPLACE). When a concurrent install already published the entry, that fails withEEXISTand the code fell back toRENAME_EXCHANGE, which swaps the temp dir with the existing cache entry and returns success. The swapped-out tree now sits in $TMPDIR under the temp name and nothing ever deletes it. The patched-dependency apply path and the streaming extractor share the same helper and leaked the same way.Deleting the swapped-out tree after the exchange turns the leak into a correctness bug: the process that won the race can still hold an open directory fd into that tree while hardlinking it into
node_modules, and deleting the entries under it makes its install fail withENOENT: failed copying files from cache to destination(observed while testing that variant).The Windows arm of
move_to_cache_directoryalready had that stronger form of the bug: on a publish collision it renamed the existing cache entry out of the cache and deleted it before retrying its own move, yanking the entry out from under the concurrent install that published it. That is the mechanism behind #28062 (ENOENT: failed opening cache/package/version diron parallel installs sharing a cache on Windows).Fix
Treat the cache publish race as first-writer-wins on every platform.
RenameatConcurrentlyOptionsgainskeep_existing_destination: when the destination already exists, keep it (it is an equivalent tree extracted from the same tarball, and other processes may be mid-read of it) and delete the caller's private temp copy, which no other process can have open. The tarball extract path (buffered and streaming) and the patch apply path opt in; thebun patchworking-directory renames keep the old replace semantics. The Windows arm applies the same rule when the destination directory exists; its rename-out-and-delete fallback block is now dead and deleted.Error paths are covered by scope guards:
ExtractTarball::extractandPatchTask::applynow delete their temp directory when they fail before the rename. This also stops the stranded temp directories reported in #11250, though not the underlyingEPERMthere, so that issue is not closed by this PR.Verification
New tests in
test/cli/install/bun-install-tempdir-cleanup.test.ts(all three fail on the unfixed build on Linux, leaking 40/2/1 temp dirs respectively against a local registry; the two error-path tests also fail on unfixed Windows):Plus a
bun_sysunit test thatkeep_existing_destinationkeeps the destination's contents and removes the source. Existing suitesbun-install-patch,bun-patch,bun-install-streaming-extract, andbun-install-tarball-integritypass. The full test file was also run against a Windows debug build (3/3 clean runs).[review] gate passed · iteration 3 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 0 rejected · iteration 3
evidence per changed file
root cause · written by the author bot
The root cause was that bun install's extract-then-rename flow assumed the rename into the global cache would always consume the temporary extraction directory, but on paths where the destination already existed, the rename failed, or the install errored out early, the temp directory was simply abandoned in $TMPDIR and accumulated indefinitely. The fix adds a keep-existing-destination mode to the concurrent rename helper that deletes the source temp directory when another writer has already populated the cache entry (first-writer-wins instead of swap, including on the Windows collision path…