install: stage git cache folders and rename on success; hit only tagged checkouts - #38274
install: stage git cache folders and rename on success; hit only tagged checkouts#38274Jarred-Sumner wants to merge 1 commit into
Conversation
…as the cache-hit marker Git checkouts (@g@<sha>) and bare mirrors (<hash>.git) were cloned straight into their final cache name, so an install killed mid-way left a folder that every later install trusted: an empty checkout resolved as an empty package and a half-cloned mirror failed every later fetch. Both are now built under a temporary sibling in the cache dir and renamed into place once complete, the same way tarball extraction already works. Cache-hit probes for unpatched entries go through one helper: npm folders must contain package.json, git checkouts the .bun-tag written last. This replaces the bare directory probes in determine_preinstall_state and the hoisted/isolated installers (and the hoisted installer's in-place mutation of the shared folder-name buffer). Since the tag is now the marker, checkout() replaces anything a repository checked in under that name and fails instead of publishing a folder it could never hit.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 42 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 (6)
Comment |
|
Updated 4:19 PM PT - Aug 13th, 2026
@Jarred-Sumner, your commit 88ab61d is building: |
|
Folded into #38269 so the install-cache/patch changes land as one PR. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nits, I also checked that join_z_buf in the new is_package_in_cache_at produces a relative <folder>/<marker> path (it does not absolutize), so exists_at(cache_dir, ...) sees the same probe the old in-place buffer edit did; and that renameat_concurrently_a is the same primitive extract_tarball.rs uses for its stage-then-rename, so the exchange-then-discard() sequence in publish() matches an existing pattern.
Extended reasoning...
This is a substantive change to package-manager cache correctness (stage-then-rename for git cache folders, new cache-hit marker semantics across four call sites, .bun-tag write changed from best-effort to hard-fail), with Windows rename behavior explicitly deferred to CI by the author. All three findings are nits and none block, but the scope and cross-platform surface put this outside the simple/mechanical bar for auto-approval. The two items above were the non-obvious correctness questions I traced through the helpers and ruled out; noting them so a human pass doesn't re-derive them.
| let _ = dir.delete_file_z(bun_core::zstr!(".bun-tag")); | ||
| let tagged = bun_sys::File::openat( | ||
| dir.fd(), | ||
| bun_core::zstr!(".bun-tag"), | ||
| bun_sys::O::WRONLY | ||
| | bun_sys::O::CREAT | ||
| | bun_sys::O::EXCL | ||
| | if cfg!(windows) { | ||
| 0 | ||
| } else { | ||
| bun_sys::O::NOFOLLOW | ||
| }, | ||
| 0o664, | ||
| ) | ||
| .and_then(|f| f.write_all(resolved)); | ||
| // Windows cannot rename a directory with an open handle inside it. | ||
| dir.close(); | ||
| if let Err(err) = tagged { |
There was a problem hiding this comment.
🟡 delete_file_z is unlinkat(.., 0) (no AT_REMOVEDIR), so a repository that checks in a directory named .bun-tag survives the unlink and then O_CREAT|O_EXCL fails EEXIST — the install hard-fails with writing ".bun-tag" failed: EEXIST, whereas before this PR it installed (with degraded verify). dir.delete_tree(b".bun-tag") (matching .git two lines up) would make the comment "anything the repository checked in under that name is replaced" true; alternatively the error could name the actual condition. Pathological input, so nit.
Extended reasoning...
What the bug is
The comment on line 1017 says ".bun-tag is the cache-hit marker, so anything the repository checked in under that name is replaced", and the PR description says checkout "unlinks whatever a repository checked in as .bun-tag". Both are accurate for a file or symlink, but not for a directory: delete_file_z is unlinkat(fd, path, 0) with no AT_REMOVEDIR (src/sys/dir.rs:369-373), which fails with EISDIR on Linux / EPERM on macOS/BSD when the target is a directory. That failure is swallowed by let _ =, and the directory remains. The subsequent openat(.., O_WRONLY|O_CREAT|O_EXCL, ..) then fails with EEXIST, and because this PR now treats a tag-write failure as fatal, the install aborts with writing ".bun-tag" for "<name>" failed: EEXIST.
Code path
At src/install/repository.rs:1018-1035 in checkout(), after git checkout completes into the staging directory:
let _ = dir.delete_tree(b".git");
let _ = dir.delete_file_z(bun_core::zstr!("node_modules")); // intentionally file-only (bundleDependencies)
// `.bun-tag` is the cache-hit marker, so anything the repository checked in under that name is replaced.
let _ = dir.delete_file_z(bun_core::zstr!(".bun-tag")); // <-- fails EISDIR/EPERM on a directory, ignored
let tagged = bun_sys::File::openat(dir.fd(), zstr!(".bun-tag"),
O::WRONLY | O::CREAT | O::EXCL | O::NOFOLLOW, 0o664) // <-- EEXIST
.and_then(|f| f.write_all(resolved));
...
if let Err(err) = tagged {
staging.discard();
log.add_error_fmt(.., "writing \".bun-tag\" for \"{}\" failed: {}", ..);
return Err(InstallFailed); // <-- install hard-fails
}Why existing code doesn't prevent it
The three variants at that path are file / symlink / directory. The PR handles file (unlinked) and symlink (unlinked; test updated to assert the symlink target is untouched), but delete_file_z cannot remove a directory. There is no fallback — the very next line requires the name to be free.
Step-by-step proof
- A git dependency's tree contains
/.bun-tag/(a directory, possibly empty) checked into the repo. bun installclones the mirror, thencheckout()clones into the staging temp dir and runsgit checkout <sha>; the working tree now contains.bun-tag/.dir.delete_file_z(zstr!(".bun-tag"))→unlinkat(dirfd, ".bun-tag", 0)→ EISDIR (Linux) / EPERM (macOS). Result discarded vialet _ =.openat(dirfd, ".bun-tag", O_WRONLY|O_CREAT|O_EXCL|O_NOFOLLOW, 0o664)→ EEXIST (the directory still occupies the name).taggedisErr(EEXIST)→staging.discard()+ error logged +return Err(InstallFailed).
Before this PR: the same repo also failed to write the tag (old code used O_TRUNC, which fails EISDIR on a directory), but the error was swallowed and the folder was published under @G@<sha> with .bun-tag still a directory. The install succeeded — though verify_git_resolution (which reads .bun-tag as a file) would fail on every subsequent run and reinstall the package from cache. So this PR turns "installs but silently reinstalls every run" into "install hard-fails" for this input.
Impact and severity
Nit. The trigger is extremely pathological — .bun-tag is a Bun-internal marker with no meaning outside Bun's cache; there's no known real-world package that ships a directory by that name at its root. The pre-PR behavior for this input wasn't correct either, and failing loudly with a named error is arguably an improvement over a silent per-run reinstall loop. Per REVIEW.md's "enumerate the input space deliberately" it's worth mentioning (file/symlink/directory are the three variants and only two are handled), but per "no speculative edge-case handling nobody filed an issue for" it shouldn't block merge.
How to fix
One line: use dir.delete_tree(b".bun-tag") instead of dir.delete_file_z(zstr!(".bun-tag")), matching the .git handling two lines above — that removes files, symlinks, and directories alike and makes the comment accurate. If the intent is instead to reject such repositories, the error message should say so rather than surface as a bare EEXIST.
| /// A cache folder is built under a temporary sibling name and renamed onto `folder_name` once complete. | ||
| struct CacheStaging { | ||
| cache_dir: bun_sys::Fd, | ||
| tmp_name_buf: [u8; 64], | ||
| tmp_name_len: usize, | ||
| } |
There was a problem hiding this comment.
🟡 nit: CacheStaging names a temp directory that must be removed on every error path, and the six error returns between CacheStaging::new() and publish() each manually call staging.discard(). All current paths do so correctly, but per CLAUDE.md #8 ("Prefer RAII (Drop) over manual cleanup") a Drop impl that calls discard(), disarmed by a published: bool set in publish(), would collapse those calls and stay correct if a future ? or early return is added. Not blocking — worst-case miss is only an orphaned random-named folder in the cache.
Extended reasoning...
What this is
CacheStaging (src/install/repository.rs:424-429) holds a temporary sibling name inside the cache directory. git clone creates a directory at that name; on success publish() renames it onto the final cache name, and on failure the caller must call discard() to delete_tree the staging dir. There is no impl Drop for CacheStaging, so cleanup is manual at every exit point.
Tracing download() and checkout():
download(): 1 error path afternew()— thegit clone --barefailure — callsstaging.discard()beforereturn Err.checkout(): 5 error paths afternew()—git clonefails,git checkoutfails,open_at(staging.tmp_name())fails, and writing.bun-tagfails — each callsstaging.discard()before returning. (publish()itself also callsdiscard()internally after the rename to sweep any exchanged folder.)
All six current error paths correctly call discard(). There is no leak in the code as written.
Why suggest a change
CLAUDE.md note 8 says "Prefer RAII (Drop) over manual cleanup", and REVIEW.md's memory-safety section says "Arm a Drop/RAII guard before any fallible call; disarm only after ownership provably transfers." The current shape — six hand-written staging.discard(); return Err(...) sequences spread across ~90 lines — is exactly what a Drop impl collapses: any future edit that adds a ? or a new early return between CacheStaging::new() and publish() would silently skip cleanup.
Step-by-step: how a future miss would happen
checkout()callslet staging = CacheStaging::new(cache_dir)?;(line ~961).- A future edit inserts, say,
let x = some_fallible_setup()?;between thegit cloneblock and thegit checkoutblock. some_fallible_setup()returnsErr. The?propagates immediately;stagingis dropped with noDropimpl, sodiscard()never runs.- The
.<hex>.tmpdirectory thatgit clone --no-checkoutcreated remains in the cache directory forever.
The consequence is benign — an orphaned randomly-named folder that is never treated as a cache hit (the whole point of this PR is that only the tagged final name is trusted). It is disk clutter, not incorrect behavior.
Suggested shape
struct CacheStaging {
cache_dir: bun_sys::Fd,
tmp_name_buf: [u8; 64],
tmp_name_len: usize,
published: bool,
}
impl Drop for CacheStaging {
fn drop(&mut self) {
if !self.published {
let _ = bun_sys::Dir::borrow(&self.cache_dir).delete_tree(self.tmp_name());
}
}
}publish() sets self.published = true before the rename (or takes mut self and sets it after a successful rename, then calls discard() explicitly on the exchanged folder as it does today). All six manual staging.discard() calls in the callers can then be deleted.
Addressing the counter-argument
One reviewer notes (correctly) that (a) extract_tarball.rs — the precedent this PR's description cites for the stage-then-rename pattern — does not use a Drop/scopeguard for its temp extraction dir either, so CacheStaging matches (and is actually stricter than) the existing in-tree convention; and (b) the REVIEW.md rule sits under "memory safety", where the stakes are fds/allocations/refcounts, not stray directories. Both points are accurate. This is why the finding is nit, not normal: there is no defect to fix, the precedent doesn't demand RAII here, and a hypothetical future miss produces harmless clutter. It's a robustness/style improvement per CLAUDE.md #8, nothing more.
| } | ||
| const cache = join(ctx.package_dir, "node_modules", ".cache"); | ||
|
|
||
| expect(await install()).toMatchObject({ exitCode: 0 }); |
There was a problem hiding this comment.
🟡 nit: the three success-path checks (expect(await install()).toMatchObject({ exitCode: 0 }), also 5444/5451) assert only exitCode. toMatchObject's failure diff does print the full received object so err is not actually hidden, but destructuring and asserting err first — as the failure path at 5438-5440 already does — would surface git's stderr as the primary diff line rather than buried in the + Received block. Worth it here since the PR description defers Windows rename behavior to CI.
Extended reasoning...
What this is
The new test's install() helper returns { out, err, exitCode }, and the three success-path assertions at lines 5428, 5444, and 5451 use:
expect(await install()).toMatchObject({ exitCode: 0 });CLAUDE.md's testing guidance says: "tests should expect(stdout).toBe(...) BEFORE expect(exitCode).toBe(0). This gives you a more useful error message on test failure." REVIEW.md's subprocess-test rule says to "assert a combined { stdout, stderr, exitCode } object". The failure-path assertion in the same test already follows the destructure-first pattern (lines 5438–5440: expect(failed.err).toContain(...) before expect(failed.exitCode).not.toBe(0)), so the success paths are the odd ones out within this test.
Addressing the refutation: stderr is not actually hidden
One verifier correctly points out that the original framing overstated the problem. Bun's toMatchObject failure path (src/runtime/test_runner/expect/toMatchObject.rs) uses DiffFormatter with received: Some(received_object), which pretty-prints the full received object via JestPrettyFormat and line-diffs it against the expected subset. So when install() returns { out, err: "<git stderr>", exitCode: 1 }, the err string does appear in the + Received block of the failure output — it is not silently dropped.
That refutation is factually correct, and this comment is scoped accordingly: this is a diagnostic-quality/convention nit, not a "the failure is undiagnosable" bug.
Why it's still worth mentioning
The remaining value is presentation, not visibility. With toMatchObject({ exitCode: 0 }), the primary assertion diff is - exitCode: 0 / + exitCode: 1, and git's stderr appears as one of several + lines inside the pretty-printed received object — where a long multi-line err (git subprocess output routinely is) gets rendered as a single quoted string with escaped \\ns and can be truncated by the pretty-printer's max-width. By contrast, the failure-path pattern this test already uses at 5438 puts stderr front-and-center as the first failing expectation.
The PR description explicitly says "Windows rename behavior is left to CI" — i.e. the author expects that if this test is going to fail anywhere, it'll be on a Windows CI runner they can't reproduce locally. That's exactly the situation where having git's stderr be the headline diff (rather than embedded in a received-object dump) shortens the debug loop.
Step-by-step example
- On a Windows CI runner,
renameat_concurrentlyinCacheStaging::publishfails (the case the PR description defers to CI). bun installwriteserror: moving "checkout-fails" to cache dir failed: <errno>to stderr and exits non-zero.- Line 5428 fails. The current output is roughly:
The cause is present but embedded and escaped.
expect(received).toMatchObject(expected) - Expected + Received Object { + "err": "bun install v...\\n[...] error: moving \\"checkout-fails\\" to cache dir failed: EACCES\\n...", - "exitCode": 0, + "exitCode": 1, + "out": "...", } - With
const ok = await install(); expect(ok.err).toContain("Saved lockfile"); expect(ok.exitCode).toBe(0);(or a combinedtoEqualon the whole object), the first failing line is the stderr content itself.
How to fix
For each of 5428/5444/5451, either destructure and assert err before exitCode (matching 5438–5440), or toMatchObject on { err: expect.stringContaining("Saved lockfile"), exitCode: 0 } so the diff highlights the stderr mismatch directly. Not blocking — the test is correct as written and the diagnostic is recoverable from the current output.
2 participants
What does this PR do?
Git dependency cache folders were built in place:
git clone --no-checkoutstraight into<cache>/@G@<sha>, thengit checkoutinside it, then.bun-tag; the per-URL bare mirror<cache>/<hash>.gitlikewise. Abun install(or its git child) killed between those steps left a folder at the trusted name, and every later install took it as a cache hit — an empty@G@folder resolves as an empty package via the "git dependency without package.json" path (exit 0, name inbun.lockfalls back to the URL basename), and a half-cloned mirror makes every latergit fetchfail. Seen in CI when git was OOM-killed. Replaces #37145.Repository::checkoutandRepository::downloadnow build under a temporary sibling inside the cache directory (CacheStaging) and rename it onto the final name only once complete, via the samerenameat_concurrentlyladder tarball extraction uses; a failed clone/checkout removes its staging dir.is_package_in_cache_at(cache_dir, folder, tag): npm folders must containpackage.json, git checkouts must contain the.bun-tagwritten last, everything else stays a directory probe. Used bycheckout()'s resolve-time hit,determine_preinstall_state, and the hoisted and isolated installers — so folders left by older versions are re-cloned rather than installed, and the hoisted installer's unsafe in-place edit of the shared folder-name buffer goes away. (.bun-tagwas previously written but only ever read fromnode_modules/<pkg>.)checkout()unlinks whatever a repository checked in as.bun-tag, creates itO_EXCL|O_NOFOLLOW, and fails the checkout if that can't be written instead of publishing a folder that would never hit. A repo shipping a symlink named.bun-tagtherefore now gets a real tag (its target is still never written; test updated).Not done: an existing bare mirror is not validated structurally — new ones can't be half-built any more, and there's no marker for old ones.
How did you verify your code works?
New test in
bun-install.test.tsover a local dumb-HTTP git server: install →@G@<sha>contains.bun-tag; delete the tree object from the cached mirror sogit checkoutfails → error reported, cache contains only the mirror (release bun leaves the half-built@G@folder, which is the failing assertion there); restore → installs; replace the folder with an empty one → re-cloned and installed. Git-filteredbun-install(37),isolated-install+bun-install-patch(82) pass locally; fullbun-install+bun-install-registryfiles: 427 pass, 0 fail (7 skipped). Also drove the debug binary by hand: no.tmpresidue after install, empty folder at the cache name gets re-cloned. Windows rename behavior is left to CI.