install: stage git cache folders, require completion markers on hit, fix bun patch for non-npm deps and isolated hang - #38269
Conversation
…solated-linker patch hang `bun patch --commit` failed with "Could not access .../@gh@@@@1" for git, github and tarball dependencies: it loaded its own lockfile but computed the cache path against the empty manager lockfile. Move the loaded lockfile into the manager before computing the path. Patch filenames also escape the NTFS-reserved characters these resolutions contain. Under the isolated linker, installing with a patchedDependencies entry for a git/github dependency in a workspace hung forever: the installer treated every patched package as missing and re-enqueued a download the resolve phase had already completed, parking the entry on a drained task. Probe the unpatched cache folder the same way unpatched packages do.
e64fe1f to
1df1053
Compare
|
Updated 10:05 PM PT - Aug 13th, 2026
❌ @Jarred-Sumner, your commit 25855c0 has some failures in 🧪 To try this PR locally: bunx bun-pr 38269That installs a local version of the PR into your bun-38269 --bun |
Jarred-Sumner
changed the title
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 43 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 (10)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| ]) { | ||
| await using proc = Bun.spawn({ cmd: ["git", ...args], cwd: repo, env: gitEnv, stderr: "pipe" }); | ||
| const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]); | ||
| expect(exitCode, `git ${args.join(" ")} failed: ${stderr}`).toBe(0); |
There was a problem hiding this comment.
🟡 The git-setup loop spawns with stderr: "pipe" but omits stdout, so it defaults to "pipe" — yet only proc.stderr.text() and proc.exited are awaited. These git commands (init -q, config, add -A, commit -q, update-server-info) produce essentially no stdout so it won't deadlock in practice, but the sibling git() helper this PR adds in isolated-install.test.ts does drain all three, so the two are inconsistent — either add stdout: "ignore" here or drain proc.stdout.text() alongside stderr.
Extended reasoning...
What the issue is
At test/cli/install/bun-patch.test.ts:1177-1180, the git-setup loop in the new "git dependency" test spawns each git command with:
await using proc = Bun.spawn({ cmd: ["git", ...args], cwd: repo, env: gitEnv, stderr: "pipe" });
const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]);stdout is unspecified, so per Bun.spawn's documented default (packages/bun-types/bun.d.ts, @default "pipe") it is a pipe — but nothing ever reads it. REVIEW.md's "Subprocess tests: drain pipes concurrently" rule requires Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]) so an unread pipe cannot fill the ~64 KB OS buffer and deadlock the child.
Step-by-step
Bun.spawn({ ..., stderr: "pipe" })—stdoutomitted → defaults to"pipe".- The kernel allocates a stdout pipe with a fixed buffer (~64 KB on Linux).
- The loop awaits
Promise.all([proc.stderr.text(), proc.exited])— stdout is never drained. - If a git command wrote > 64 KB to stdout, it would block on
write(1, …)while the test blocks onproc.exited→ deadlock. - In practice the five commands here (
init -q,config core.autocrlf false,add -A,commit -q -m init,update-server-info) all write zero or a few bytes to stdout, so the pipe never fills and the test does not hang.
Why nothing prevents it
There is no stdout: "ignore" and no proc.stdout.text() in the Promise.all. The await using cleanup runs only after the awaited promise settles, so it cannot rescue a deadlock.
Inconsistency within the diff
The same PR adds a git() helper in test/cli/install/isolated-install.test.ts that does follow the convention:
await using proc = spawn({ cmd: [gitExecutable!, ...args], cwd, env: gitEnv, stdout: "pipe", stderr: "pipe" });
const [out, err, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);So the two new git helpers introduced by this PR are inconsistent with each other, and this one diverges from the pattern REVIEW.md and the surrounding file (runBun at the top of the same describe block drains all three) both use.
Impact
None in practice — the specific git invocations produce negligible stdout, so this cannot deadlock as written. It is a harness-convention nit and a within-diff consistency issue, not a functional bug; merging as-is would not break anything.
Fix
Either add stdout: "ignore" to the spawn options, or drain it:
await using proc = Bun.spawn({ cmd: ["git", ...args], cwd: repo, env: gitEnv, stdout: "ignore", stderr: "pipe" });or
const [, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);…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.
| const env = { ...bunEnv, BUN_INSTALL_CACHE_DIR: join(String(dir), ".bun-cache") }; | ||
|
|
||
| const patchKey = await expectPatchFlowWorks(project, env, "node_modules/pkg-to-patch"); |
There was a problem hiding this comment.
🟡 The env passed to expectPatchFlowWorks here is only { ...bunEnv, BUN_INSTALL_CACHE_DIR: ... }, so the git clone/git checkout that bun install runs for this dependency inherits the machine's global/system git config — including core.autocrlf on Windows. The sibling test this PR adds in isolated-install.test.ts spreads gitConfigEnv (GIT_CONFIG_NOSYSTEM/GIT_CONFIG_GLOBAL) into the bun install env for exactly this reason (with a comment naming the hazard); doing the same here — e.g. const env = { ...bunEnv, GIT_CONFIG_NOSYSTEM: "1", GIT_CONFIG_GLOBAL: join(String(dir), "no-gitconfig"), BUN_INSTALL_CACHE_DIR: ... } — would make this test hermetic and consistent with its sibling.
Extended reasoning...
What the issue is
The new "git dependency" test in test/cli/install/bun-patch.test.ts builds a gitEnv with GIT_CONFIG_NOSYSTEM: "1" and GIT_CONFIG_GLOBAL: <nonexistent> and uses it for the fixture-creating git init/commit/update-server-info commands — but the env it then passes to expectPatchFlowWorks (which drives bun install, bun patch, and bun patch --commit) is only { ...bunEnv, BUN_INSTALL_CACHE_DIR: join(String(dir), ".bun-cache") }. bunEnv (test/harness.ts) spreads process.env and does not set GIT_CONFIG_NOSYSTEM or GIT_CONFIG_GLOBAL.
Bun's git-dependency checkout (src/install/repository.rs) spawns git clone -c core.longpaths=true --no-checkout <url> <dir> followed by git -C <dir> checkout --quiet <sha> with the inherited process env. It does not pass -c core.autocrlf=… and does not isolate git from the machine's config. The fixture repo's local git config core.autocrlf false is written to the source repo's .git/config, which does not transfer over the dumb-HTTP clone; the clone consults the machine's system/global config.
Step-by-step on a Windows runner with global core.autocrlf=true
- The fixture repo commits
index.jscontaining LF-only bytes (module.exports = "original";\n) — the fixture's localcore.autocrlf=falseandGIT_CONFIG_NOSYSTEMingitEnvkeep these bytes as-is in the blob. expectPatchFlowWorksrunsbun installwithenv = { ...bunEnv, BUN_INSTALL_CACHE_DIR: ... }. Inside, Bun spawnsgit clone --no-checkoutthengit checkout <sha>with that env.- Because
GIT_CONFIG_NOSYSTEM/GIT_CONFIG_GLOBALare not set, git reads the runner's globalcore.autocrlf=trueand the checkout writesindex.jsto the cache with CRLF line endings. bun patchcopies the cache folder intonode_modules/pkg-to-patch; the test thenBun.writes an LF-onlyindex.jsover it.bun patch --commitdiffs LF against the CRLF cache copy; the reinstall re-checks-out with CRLF and applies the patch on top.- The final assertion is
expect(await Bun.file(...).text()).toBe('module.exports = "patched";\n')— an exact.toBeon a\n-terminated string. With CRLF in the checkout/reapply path, the file on disk (and/or the diff's-line matching) becomes environment-dependent, so the test's outcome depends on the runner's git config rather than the code under test.
Why nothing prevents it
gitEnv is only used for the loop that builds the fixture repo. The env object handed to runBun is a separate literal that does not spread gitEnv's config-isolation keys. Bun's own SharedEnv::get() in repository.rs clones the process env and only injects GIT_ASKPASS/GIT_SSH_COMMAND; it does not force autocrlf off.
Within-diff inconsistency
The sibling test this same PR adds in test/cli/install/isolated-install.test.ts explicitly builds:
const gitConfigEnv = { GIT_CONFIG_NOSYSTEM: "1", GIT_CONFIG_GLOBAL: join(packageDir, "gitconfig") };
...
const env = { ...bunEnv, ...gitConfigEnv, BUN_INSTALL_CACHE_DIR: ... };and comments it as "Isolate git from system/global config (e.g. core.autocrlf on Windows would rewrite the checked-out file contents this test asserts on)". The bun-patch.test.ts version omits the same spread, so the two new tests disagree on how to isolate the same hazard. REVIEW.md: "Tests must be hermetic" and "deduplicate within your own diff".
Impact
Nit. It may well pass on the current CI Windows image (depends on whether that image sets core.autocrlf=true globally), and it does not affect the shipped runtime fix. It is a hermeticity gap and a within-diff inconsistency, not a functional bug.
Fix
Spread the same config-isolation keys into the env passed to expectPatchFlowWorks, matching the isolated-install.test.ts approach:
const env = {
...bunEnv,
GIT_CONFIG_NOSYSTEM: "1",
GIT_CONFIG_GLOBAL: join(String(dir), "no-gitconfig"),
BUN_INSTALL_CACHE_DIR: join(String(dir), ".bun-cache"),
};(or equivalently ...gitEnv in place of ...bunEnv since gitEnv already spreads bunEnv).
Adds a re-add leg to the github and git workspace patch tests and an add/remove/re-add cycle for an npm dependency. The suspected stale .bun-tag-<hash> skip on re-add did not reproduce; these pin the cycle. No-Verification-Needed: test-only change
Jarred-Sumner
changed the title
…ed-in .bun-tag directory The hardlink and copyfile backends overlay files onto an existing store entry, so removing a patch (or changing it) kept every file the previous patched build had added, along with its .bun-tag-<hash> marker; clonefile happened to replace the tree, which is why this only showed on Linux. Delete the previous project-local package tree before rebuilding an entry. Repository::checkout used delete_file_z to clear a checked-in .bun-tag, which fails on a directory and then made the O_EXCL create fail; use delete_tree so any checked-in file, symlink or directory is replaced.
There was a problem hiding this comment.
Re-ran on 25855c0 — the .bun-tag-as-directory case is fixed (delete_tree + the new "replaces a .bun-tag directory" test), and I found no issues in the added isolated-store delete_tree(previous) rebuild path. The two earlier test nits are non-blocking. Given the scope (cache-hit marker semantics across three call sites, CacheStaging publish/discard on every git error path, the manager.lockfile = lockfile ownership move in do_patch_commit, and the store-entry wipe in the isolated installer), a human pass is still worthwhile.
Checked in this run:
checkout()now handles a checked-in.bun-tagfile, symlink, and directory before theO_EXCLcreate; every early-return path callsstaging.discard().is_package_in_cache_atis used consistently by the hoisted installer, isolated installer, anddetermine_preinstall_state; the patched branch still probes the unpatched folder.Installer.rsdelete_treetargets the project-local final store path (not the global cache) and only runs on the rebuild branch.
Extended reasoning...
Overview
Consolidated install-robustness PR touching src/install/{repository.rs, PackageInstall.rs, isolated_install.rs, isolated_install/Installer.rs, PackageManager/{PackageManagerDirectories.rs, PackageManagerLifecycle.rs, patchPackage.rs}} plus three test files. Four independent fixes: (1) git cache folders are built in a staging sibling and renamed atomically via CacheStaging, (2) cache hits now require a completion marker (package.json for npm, .bun-tag for git) via a shared is_package_in_cache_at helper, (3) bun patch --commit for git/github/tarball deps moves the loaded lockfile into manager.lockfile before computing the cache path so resolution strings resolve against the correct buffer, and (4) the isolated linker no longer hangs on patched git/github deps and now wipes the previous project-local store entry before rebuilding so removed-patch files don't linger under hardlink/copyfile.
Follow-up commit
25855c0 addressed my earlier finding: delete_file_z(".bun-tag") → delete_tree(b".bun-tag") so a checked-in .bun-tag/ directory is replaced instead of hard-failing the install on O_EXCL → EEXIST. A test with a nested .bun-tag/nested.txt was added alongside the existing symlink test. The same commit added the Fd::cwd().delete_tree(previous.slice()) call in Installer.rs to fix the Linux hardlink-backend staleness the PR description documents; I confirmed append_real_store_path(..., Which::Final) targets the project-local .bun/ store entry, not the shared global cache, and sits inside the needs-rebuild branch so warm installs skip it.
Security risks
None identified. The .bun-tag write path already went through is_safe_resolved_tag before this PR; the new O_EXCL|O_NOFOLLOW open plus the preceding delete_tree is strictly safer than the old O_TRUNC write (which could have followed a checked-in symlink on Windows where NOFOLLOW is 0 — now the link is removed first). escape_patch_filename gains NTFS-reserved characters, which only widens the escape set. CacheStaging::publish uses renameat_concurrently within the same directory (same filesystem), matching the existing tarball-extract ladder.
Level of scrutiny
High. This is production-critical bun install behavior: changing what counts as a cache hit affects every install, and the manager.lockfile = lockfile move in do_patch_commit changes ownership flow that the surrounding code (and the later install_with_manager reload noted in the PR description) depends on. The isolated-store delete_tree is a new destructive step. All four fixes are well-tested (staging-dir-only-on-success, tag-required-for-hit, patch-commit for github/git/tarball, add/remove/re-add cycles for npm/github/git under isolated), but the interaction surface is large enough that a maintainer should sign off.
Other factors
Two open 🟡 nits from earlier runs remain (stdout not drained in the bun-patch.test.ts git-setup loop; gitConfigEnv not spread into the bun install env for the git patch test). Both are test-hermeticity concerns with no practical failure mode as written and do not block. CI on 826113f is still building per the robobun comment; Windows rename/escaping legs are explicitly deferred to CI per the PR description.
2 participants
What does this PR do?
Install-cache and
bun patchrobustness, consolidated from #37124, #37136 and #37145 (rebased and reshaped; #32749 from the same batch landed separately).Git dependency cache folders are built in a staging dir and renamed on success.
Repository::checkoutcloned straight into<cache>/@G@<sha>and checked out in place;Repository::downloadcloned the bare mirror straight into<cache>/<hash>.git. An install (or its git child — seen OOM-killed in CI) dying between steps left a folder at the trusted name: an empty@G@folder resolves as an empty package through the "git dependency without package.json" path (exit 0,bun.lockname falls back to the URL basename), and a half-cloned mirror fails every latergit fetch. Both now build under a temporary sibling inside the cache dir (CacheStaging, same-filesystem so it's the samerenameat_concurrentlyladder tarball extraction uses) and are renamed into place only when complete; failures remove the staging dir.Cache hits require the entry's completion marker. One helper,
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 — previously every git hit was a bare directory probe (.bun-tagin the cache was written but never read), anddetermine_preinstall_statedidn't probepackage.jsonfor npm either. Folders left by older versions are re-cloned instead of installed. Deletes the hoisted installer's unsafe in-place edit of the shared folder-name buffer and the isolated installer's append/truncate copy. Since the tag is now the marker,checkout()unlinks anything a repo checked in as.bun-tag, creates itO_EXCL|O_NOFOLLOW, and fails the checkout rather than publish an untaggable folder (a repo shipping a symlink named.bun-tagnow gets a real tag; its target is still never written — test updated). Existing bare mirrors are not validated structurally; there's no marker for them.bun patch --commiton git, github and tarball dependencies failed withCould not access '.../@GH@@@@1'and wrote nothing (#18792, #17945): it loads its own lockfile but computed the cache path against the emptymanager.lockfile. The loaded lockfile is moved into the manager before the path is computed (install_with_managerreloads it afterwards, as it already does forbun update; the double parse is left alone). Patch filenames additionally escape NTFS-reserved characters, which only these resolutions contain. Fixes #18792, fixes #17945.Isolated linker hang — a plain
bun installin a workspace with apatchedDependenciesentry for a git/github dependency hung forever: the installer treated every patched package as missing and re-enqueued a download the resolve phase had already completed, parking on a drained task list. It now probes the unpatched folder (computed with no patch hash) like everything else; also fixes removing an entry.Tests: github/git/tarball
patch --commitflows; add → cold cache → remove → re-add of a patch under isolated for github, git and npm (the suspected stale-.bun-tag-<hash>skip on re-add did not reproduce, so these just pin the cycle); git checkout failure leaves only the mirror in the cache and an empty folder at the cache name is re-cloned; a pre-existing test pinned to the bogus--commitcache path now asserts the step that genuinely fails.How did you verify your code works?
bun-install+bun-install-registry(427),isolated-install(65),bun-install-patch,bun-patchall pass locally; new tests fail on release bun (half-built@G@folder left behind;--commiterror; isolated hang). Each fix was also driven by hand with the debug binary:patch --commiton afile:tarball fails on main's binary and succeeds here; a workspace with a patched local git dep installs, requires as patched, survives remove and re-add, and re-clones an emptied cache folder, with no.tmpresidue. Windows rename/escaping legs are left to CI. Local gotcha: these files needHOMEpointed at an empty dir if~/.npmrcsetsinstall-strategy=hoisted.Added after CI (
25855c0fe97): the npm add/remove/re-add test failed on Linux — the hardlink and copyfile backends overlay files onto an existing isolated store entry, so removing a patch kept every file the patched build had added (and its.bun-tag-<hash>); clonefile replaces the tree, which is why macOS passed and why the earlier "did not reproduce" was wrong — this is the staleness #37136 mentioned. The task now deletes the previous project-local package tree before rebuilding an entry (only reached when the entry needs a rebuild, so warm installs don't pay for it); the npm cycle test pins the hardlink backend. Also from review:checkout()usesdelete_treefor a checked-in.bun-tag, so a directory under that name is replaced instead of failing the install (test added).