install: stop leaking the displaced cache folder when a tarball is re-extracted - #38702
install: stop leaking the displaced cache folder when a tarball is re-extracted#38702robobun wants to merge 4 commits into
Conversation
|
Warning Review limit reached
Next review available in: 1 minute 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 (7)
Comment |
|
Status: reproduced and fixed; CI is green on every lane that exercises the change.
|
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Not a duplicate of those three, though they all touch the same publish step (see "Related open PRs" in the description):
The bug fixed here (one extracted copy left behind per lockfile-less install, single process, no race) is not fixed by any of them as written. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Review bot follow-ups, in f135318 and ab7e269:
No behaviour change in either commit. |
There was a problem hiding this comment.
I re-reviewed after the follow-up commits and didn't find any bugs — the earlier stdout-drain nit is addressed. Given this touches cache-publish semantics that four open PRs (#33979, #36229, #33884, #31868) are also reworking, a maintainer sign-off on the CachePublish design would be worthwhile.
What was reviewed:
cache_publish()computes the same@T@folder name asmove_to_cache_directory(both go throughcached_tarball_folder_name_printonself.url), so the pre-check and the rename target agree.- Cleanup matrix after the rename: for every (Replace/Supersede/KeepExisting × ok/err × POSIX/Windows) combination the temp name ends up deleted or intentionally kept;
Replace+ok is the only case left alone, matching prior npm/GitHub behaviour. RenameatConcurrentlyOptions::default()inrepository.rsand the..Default::default()additions inpatchPackage.rs/move_file_zkeep the old field values, so those callers are unchanged.
Extended reasoning...
Overview
The PR fixes a temp-dir leak where re-extracting a file:/URL tarball over its existing cache folder swaps the old folder out under the staging name and never deletes it. It introduces a three-way CachePublish enum (Replace/Supersede/KeepExisting), computed before extraction from whether the target cache folder already exists, threads it through both the buffered (extract) and streaming (TarballStream::init) paths into move_to_cache_directory, and adds a keep_existing_destination flag to renameat_concurrently. Struct-literal call sites in patchPackage.rs and repository.rs gain ..Default::default() with no behaviour change. Two integration tests and one bun_sys unit test cover the new paths.
Security risks
None identified. The change deletes the process's own staging directory (a randomly-named child of the install temp dir) or a swapped-out cache folder under names Bun itself generated; no user-controlled path is fed to delete_tree. keep_existing_destination only deletes the from path the caller already owned.
Level of scrutiny
Medium-high. The mechanical change is small, but it encodes a design decision about what happens on cache-folder collisions (replace-and-delete vs keep-existing) that differs by resolution kind and is deliberately positioned relative to four other open PRs touching the same publish step. The Supersede branch newly deletes a folder that was previously left alone, and the Windows arm gains a new early-break on KeepExisting. That is package-manager-cache-correctness territory where a maintainer familiar with the concurrent-install history (#9738, #28062) should confirm the direction.
Other factors
- My previous inline comment (stdout not drained in
installFresh) was addressed in f135318; the comment-cop feedback was addressed in ab7e269. - I traced
cache_publish()againstmove_to_cache_directory's folder-name computation for the tarball tags and they agree (same helper, sameself.urlinput, sameNonepatch hash). The thread-local buffer borrow incache_publish()is released beforeextract()re-borrows it later, andTarballStream::initruns on a different thread frompopulate_result, so there is no TL_BUFS aliasing. - The post-rename cleanup
if moved.is_err() || publish != CachePublish::Replace { delete_tree(tmpname) }was checked against every mode/outcome combination on both the POSIX (renameat_concurrentlywith the new option) and Windows (retry loop with the newbreak) arms; each either deletes the intended tree or is a harmless no-op on an already-gone name. move_file_zandrepository::CacheStaging::publishkeepkeep_existing_destination: falseviaDefault, so the new deletion path is not reached by existing callers.- The tests look sound (drain both pipes, assert the combined object, hermetic temp dirs, local server on port 0, streaming path forced by chunked incompressible payload).
Deferring rather than approving because the design overlaps with several in-flight PRs on the same function and a human should confirm this is the shape the maintainers want the others to compose with.
|
Updated 10:05 PM PT - Aug 14th, 2026
❌ @robobun, your commit 0613908 has some failures in 🧪 To try this PR locally: bunx bun-pr 38702That installs a local version of the PR into your bun-38702 --bun |
There was a problem hiding this comment.
I re-reviewed this PR and found no bugs; the earlier stdout-drain note is addressed. Given the open design question the author flagged (whether CachePublish is the shape #33979 / #36229 / #33884 should build on) and that this changes the cache-publish step those PRs also rewrite, a maintainer should sign off on the direction.
What was reviewed:
CachePublishon the POSIX and Windows arms ofmove_to_cache_directory: the post-movedelete_tree(tmpname)was traced for each mode (Supersede after EXCHANGE removes the swapped-out folder; KeepExisting/no-collision and Replace hit an already-gone or intentionally-kept path; the new delete-on-error only removes our own staging).keep_existing_destinationinrenameat_concurrently: the exists-check-then-delete-source ordering, and thatrepository.rs/patchPackage.rscallers keep their prior behaviour under..Default::default().cache_publish()re-usingTL_BUFS.folder_name_bufbeforemove_to_cache_directoryalso does — sequential on the buffered path, different thread on the streaming path, so no conflict.
Extended reasoning...
Overview
The PR fixes a leak where re-extracting a file: or URL tarball over its existing @T@ cache folder leaves the swapped-out copy behind in the install temp dir (one per lockfile-less install). It introduces a CachePublish enum decided before extraction (Supersede when the folder pre-exists, KeepExisting when it does not, Replace unchanged for npm/GitHub), threads it through both the buffered extractor and TarballStream, and adds keep_existing_destination to RenameatConcurrentlyOptions in src/sys/lib.rs. Mechanical ..Default::default() updates in patchPackage.rs/repository.rs keep other callers behaviourally unchanged. Two new integration tests (file: and streamed URL) plus a bun_sys unit test cover the fix.
Security risks
None identified. The new delete_tree targets only the randomly-named staging path inside the install temp dir (or the folder swapped into that name by EXCHANGE); no user-controlled path reaches it. keep_existing_destination only broadens what is kept, and the source it deletes is the caller's own staging.
Level of scrutiny
High. This is core package-manager cache publishing on both POSIX and Windows, with concurrency semantics (racing installs sharing a cache) and file deletion. It also intersects three other open PRs (#33979, #36229, #33884) that rewrite the same step differently, and the author explicitly calls out "Open item for a maintainer: whether CachePublish … is the shape the other open PRs on this publish step should build on." That is a design decision a human should make.
Other factors
The prior automated finding (undrained stdout in installFresh) was fixed in f135318; the comment-cop items were shortened in ab7e269; all inline threads are resolved. CI on build 96684 was green on the lanes that ran, per the status comment. The tests are well-targeted (three lockfile-less installs must leave the temp dir empty and one @T@ folder, then a repacked tarball must win), and the bun_sys unit test covers the new rename option directly. No correctness issue was found in this pass, but the interaction with the sibling PRs and the open design question mean this should not land on automated approval alone.
2 participants
Problem
bun installwithout a lockfile that has afile:or URL tarball dependency already in the cache leaves one fully extracted copy of that tarball behind in the install temp dir (<cache>/.tmp/.<hex>-1.<name>/, or$TMPDIRwhen that is on the cache's filesystem). N installs leave N-1 copies; the install exits 0.package.jsonis what resolves it).move_to_cache_directory(src/install/extract_tarball.rs) publishes the staging dir withrenameat_concurrently:RENAME_NOREPLACEfails because the folder exists, so it falls back toRENAME_EXCHANGE, which swaps the old folder out under the staging name. Nothing deletes it. The step "2b" described in the comment there never had code behind it (the line that was meant to do it in Make duplicate simultaneousbun installwork better #9738 ran before the rename, against a variable that was stillfalse).https://tarballs, through both the buffered and the streaming extractor, since both end inmove_to_cache_directory.Fix
ExtractTarball::cache_publish()decides, before extraction starts, how the result will be published, and the buffered path (extract) and the streaming path (TarballStream::init) pass that decision tomove_to_cache_directoryasCachePublish:Supersede: the folder name was already taken when the task started, so this is a re-extraction of the tarball. The fresh copy is swapped in as before, and the folder it displaced is deleted from the temp dir. This is the case in the report, and it keeps the current behaviour that a repacked tarball wins once the lockfile is removed.KeepExisting: the name was free when the task started. If it is taken by the time the rename happens, a concurrent install extracted the same tarball first; its folder may already be copied from, so it is kept and our staging copy is deleted.renameat_concurrentlygets akeep_existing_destinationoption for this, and the Windows arm ofmove_to_cache_directorydoes the equivalent on its existing-destination errors. Previously this case replaced the other install's folder (POSIX: swap and leak it; Windows: delete it, which is Windows: parallel bun install --no-cache with shared BUN_INSTALL_CACHE_DIR can fail with ENOENT opening cache/package/version dir #28062).Replace: npm and GitHub folders, unchanged. Their swapped-out folder is still left alone because nothing there can tell a race from a re-extraction; the open PRs listed below deal with those.@T@<hash>@@@1), not its contents, so the contents of an existing folder say nothing (Tag::is_tarballdocuments this).Replacemode is the one thing that is not.test/cli/install/bun-install-tarball-integrity.test.ts, two new tests (file:tarball, tarball URL): three installs without a lockfile must leave the temp dir empty and one@T@folder, and a repacked tarball must be the version installed afterwards. On the current build both fail with two.<hex>-1.pkgdirs left in the temp dir. The URL test serves a 256 KiB tarball in chunks without a Content-Length, and--verboseshowsStreamedfor all of its installs, so it covers the streaming extractor's half of the plumbing.bun_sysunit test forkeep_existing_destination(destination kept, source removed; free destination renames normally).bun-install-streaming-extract,bun-add,bun-patch,bun-install-patch,bun-install-git-deps, the tarball/github/cache subset ofbun-install.test.ts(one test in it needs network access and fails identically without this change), and the rest ofbun-install-tarball-integritypass with the debug build.cargo checkforbun_sys/bun_installpasses on Linux andx86_64-pc-windows-msvc; clippy is clean.RenameatConcurrentlyOptionsgained a field, so the struct literals inpatchPackage.rsandrepository.rsnow spell out the defaults; their behaviour is unchanged.Related open PRs
file:/URL tarballs that is theSupersedecase above: a repacked tarball reinstalled without a lockfile would keep serving the old extraction while the new sha512 is written to the lockfile.CachePublishis the information those changes need to keep the existing folder only on a race; this PR does not change npm or GitHub publishing itself.KeepExistingmode. The Windows branch added here is the small version of that and will be superseded by it.Supersedecould additionally be skipped when the integrity matches; without it the folder has to be replaced to pick up a changed tarball.--force) adds the same predicate asTag::is_tarballunder another name, and every re-download it adds is aSupersedepublish, so it would leak one copy per forced install without this change.Background
renameat_concurrently(src/sys/lib.rs): triesrenameat2(RENAME_NOREPLACE), thenRENAME_EXCHANGE(atomically swaps source and destination, leaving the old destination under the source name), thendelete_tree+rename. Since Make duplicate simultaneousbun installwork better #9738 the exchange is what makes two installs sharing a cache not fail each other; the swapped-out folder is left because the other install may still be reading it.@T@folders:file:and URL tarballs are cached under a hash of the path or URL string (cached_tarball_folder_name_print), so the same folder holds whatever was last extracted from that path. npm folders (name@version) and GitHub folders (resolved commit) are keyed by content identity instead, which is why the race-oriented PRs above can treat an existing folder as equivalent for those but not for tarballs.TarballStream.rs): for larger or chunked downloads the tarball is extracted while it downloads; it sharesmove_to_cache_directorywith the buffered path, which is why the publish decision is captured inTarballStream::init(on the main thread, when the download is queued) and inextractfor the buffered path.Reproduction from the report
Before (1.4.0-canary,
b7a043103): 0, 1, 2, 3, 4 staging dirs (in$TMPDIRwhen it shares a filesystem with the cache, otherwisecache/.tmp). After: 0 every time, and the@T@folder holds the latest extraction.The leaked dir holds the previous cache contents, not the new extraction: repack
tdep.tgzbetween two lockfile-less installs and the leaked copy has the old files while the cache folder has the new ones.[review] gate passed · iteration 3 · 7 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 3
evidence per changed file