install: fix isolated-linker deadlock installing patched git/github dependencies in workspaces - #37136
install: fix isolated-linker deadlock installing patched git/github dependencies in workspaces#37136robobun wants to merge 5 commits into
Conversation
…er the isolated linker The isolated install loop treated every patched dependency whose preinstall state was not Done as missing from the cache and re-enqueued its tarball download. When the resolve phase had already downloaded that same tarball (a github dependency of a workspace member misses get_package_id during re-resolution because the parsed version's package_name is empty), the install-phase context was pushed onto the completed task's already-drained callback list, nothing was scheduled, and the pending-task wait spun forever. Check for the unpatched cache folder instead, the same way the hoisted installer's package_missing_from_cache does: strip the _patch_hash= suffix for PatchInfo::Patch, treat PatchInfo::Remove like an unpatched package, and only enqueue a download when the folder is genuinely absent. When it is present, apply_package_patch derives the patched folder from it and the store task starts immediately.
|
Warning Review limit reached
Next review available in: 9 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)
WalkthroughIsolated installs now detect unpatched cache entries for patched packages. Cache checks support npm, patched, removed, and non-npm entries. Regression tests cover GitHub tarballs and Git-over-HTTP dependencies across patch transitions. ChangesIsolated install cache handling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/isolated-install.test.ts`:
- Around line 803-809: Extend the isolated-install workspace dependency tests
around the existing `github:` fixture to also exercise the `git:` variant. Add
equivalent patch-add and patch-remove scenarios using a git workspace
dependency, covering both cache-folder and checkout behavior implemented by
ResolutionTag::Git while preserving the existing GitHub coverage.
- Around line 784-790: Update the install subprocess flow around spawn to
consume proc.stdout concurrently with proc.stderr and proc.exited, such as by
awaiting all three operations together. Preserve the piped configuration and
existing assertions while ensuring neither output stream can block the child
process.
🪄 Autofix
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: 01f02a60-b8bf-4edf-b2c2-b1f9e1a72d1b
📒 Files selected for processing (2)
src/install/isolated_install.rstest/cli/install/isolated-install.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…t protocol and cold cache - Remove the set_length(full_len) restore: nothing reads the buffer after the block, and in the Patch case slice_z had already written a NUL at the truncation point, so the restore produced a corrupted view. - Drain stdout in the test's install helper and drop the per-test timeout. - Add a git: variant of the regression test (local repo served over git's dumb HTTP protocol) and a cold-cache leg to the github test.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/isolated-install.test.ts`:
- Around line 972-977: Add a cold-cache reinstall step to the git: test after
the patched-content assertion, clearing .bun-cache and both relevant
node_modules directories while retaining the patched lockfile, then run
install() and assert the patched content remains correct. Mirror the existing
github: test coverage without changing the subsequent original-content
restoration flow.
- Around line 881-887: Update the isolated-install test setup around the git
helper to detect availability with Bun.which("git"), define the executable
prerequisite, and apply test.skipIf(!gitExecutable) to skip when git is
unavailable. Add a brief comment documenting that git is required.
🪄 Autofix
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: d62683d2-357b-4aed-b057-2b5bb15e3988
📒 Files selected for processing (2)
src/install/isolated_install.rstest/cli/install/isolated-install.test.ts
There was a problem hiding this comment.
No bugs found this pass, and all four of my earlier inline nits (dead set_length restore, undrained stdout pipe, per-test timeout, comment length) have been addressed. Deferring to a human because this changes when the isolated installer transitions preinstall state to Done and skips download enqueueing — core bun install state-machine logic — which is worth a maintainer's eyes even though the change mirrors the hoisted path.
What was reviewed:
- Verified the
PatchInfo::Nonepath is behavior-identical to the pre-change code (sameNpmpackage.json check, same_directory check, sameset_preinstall_state). - Compared the new
Patch/Removelogic againstPackageInstall::package_missing_from_cache(src/install/PackageInstall.rs:2305-2380) — samelast_index_of(b"_patch_hash=")truncation, same panic message, samedirectory_exists_aton the unpatched folder. - Confirmed
pkg_cache_dir_subpathis dead after line 2392, so the in-place truncation is safe (nothing downstream reads the patched path).
Extended reasoning...
Overview
Two files: src/install/isolated_install.rs (~50 lines replacing a // TODO: why does this look like it will never work? + unconditional break 'missing_from_cache true with a real cache check for patched packages) and test/cli/install/isolated-install.test.ts (~260 lines: two end-to-end tests for github: and git: dependencies, each running install → add patch → install → cold-cache install → remove patch → install and asserting file contents at each step).
Security risks
None. No untrusted-input parsing, no auth/crypto, no path handling beyond the existing _patch_hash= suffix strip already used verbatim by the hoisted installer.
Level of scrutiny
Medium-high. This is bun install under the default (configVersion: 1) isolated linker, and the change alters when set_preinstall_state(pkg_id, Done) fires and when a tarball/git download is enqueued vs skipped. I traced the PatchInfo::None case through both old and new code and it is byte-for-byte behavior-preserving — only Patch(_) and Remove(_) change (from always-redownload to check-unpatched-cache-first), which is exactly what PackageInstall::package_missing_from_cache already does for the hoisted linker. The panic on missing _patch_hash= suffix is copy-pasted from the hoisted version, and put_patch_hash in PackageManagerDirectories.rs guarantees the suffix is present whenever contents_hash() is Some (which Patch(_) implies). Still, someone who owns the install subsystem's task-queue/preinstall-state invariants should confirm the interaction with apply_package_patch and the cold-cache download path.
Other factors
- All four of my earlier inline findings and both CodeRabbit nits (skipIf on missing git, cold-cache leg for the git test) are resolved in the current diff.
- The last comment-cop flag on the 3-line comment at 2359-2361 was reasonably pushed back on by the author — the comment carries non-local info (why the unpatched folder is checked) plus the PR link, and further shortening loses that.
- No CODEOWNERS entry for
src/install/. - The PR description notes two pre-existing issues in this area (redundant re-resolution download; stale
.bun-tagmarker on re-add) that reproduce on released bun and are explicitly out of scope here.
|
Folded into #38269 together with the other two |
…fix bun patch for non-npm deps and isolated hang (#38269) ### What does this PR do? Install-cache and `bun patch` robustness, 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::checkout` cloned straight into `<cache>/@g@<sha>` and checked out in place; `Repository::download` cloned 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.lock` name falls back to the URL basename), and a half-cloned mirror fails every later `git fetch`. Both now build under a temporary sibling inside the cache dir (`CacheStaging`, same-filesystem so it's the same `renameat_concurrently` ladder 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 contain `package.json`, git checkouts must contain the `.bun-tag` written last, everything else stays a directory probe. Used by `checkout()`'s resolve-time hit, `determine_preinstall_state`, and the hoisted and isolated installers — previously every git hit was a bare directory probe (`.bun-tag` in the cache was written but never read), and `determine_preinstall_state` didn't probe `package.json` for 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 it `O_EXCL|O_NOFOLLOW`, and fails the checkout rather than publish an untaggable folder (a repo shipping a symlink named `.bun-tag` now 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 --commit` on git, github and tarball dependencies** failed with `Could not access '.../@gh@@@@1'` and wrote nothing (#18792, #17945): it loads its own lockfile but computed the cache path against the empty `manager.lockfile`. The loaded lockfile is moved into the manager before the path is computed (`install_with_manager` reloads it afterwards, as it already does for `bun 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 install` in a workspace with a `patchedDependencies` entry 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 --commit` flows; 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 `--commit` cache 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-patch` all pass locally; new tests fail on release bun (half-built `@G@` folder left behind; `--commit` error; isolated hang). Each fix was also driven by hand with the debug binary: `patch --commit` on a `file:` 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 `.tmp` residue. Windows rename/escaping legs are left to CI. Local gotcha: these files need `HOME` pointed at an empty dir if `~/.npmrc` sets `install-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()` uses `delete_tree` for a checked-in `.bun-tag`, so a directory under that name is replaced instead of failing the install (test added).
2 participants
Symptom
bun installhangs forever (no output afterResolved, downloaded and extracted [N], main thread asleep in the event loop) when all of the following hold:patchedDependencieshas an entry for agithub:(orgit:) dependency of a workspace member,configVersion: 1default).No
bun patchcommands are involved; a plainbun installafter adding thepatchedDependenciesentry deadlocks. The same project completes with--linker hoisted. Reproduced on1.4.0-canary.1 (0ffabf64d)for both thegithub:andgit:protocols. This is the hang documented as a known limitation in #37124 (the internal reinstall run bybun patch --commitreaches it the same way).Repro (offline, fake GitHub API):
gh-dep) from a local HTTP server, setGITHUB_API_URL=http://localhost:PORTand a freshBUN_INSTALL_CACHE_DIR.{"name":"ws-root","workspaces":["packages/*"]}; member depends on"gh-dep": "github:testowner/testrepo#aaaaaaa".bun install --linker isolatedsucceeds."patchedDependencies": {"gh-dep@github:testowner/testrepo#aaaaaaa": "patches/gh-dep.patch"}to the root and runbun install --linker isolatedagain: hangs forever. Removing an existing entry hangs the same way.Cause
Adding (or removing) the patch entry triggers a re-resolution. The member's github dependency misses
lockfile.get_package_idduring re-resolution (the re-parsed version'spackage_nameis still empty, so the lookup uses the wrong name hash) and the resolve phase re-downloads the tarball, registering it intask_queueandnetwork_dedupe_mapunderTask::Id::for_tarball(url). When the extract completes in the resolve phase its callback list is drained, but the (now empty)task_queueentry and the dedupe entry stay behind. The extract appends a transient duplicate package and marks that idDone, while the dependency ends up resolved to the original package, whose preinstall state staysUnknownthrough the lockfile clean. (The duplicate itself is in-memory only; the clean prunes it, sobun.lockstays correct. The redundant re-download on re-resolution is an upstream inefficiency tracked separately.)The isolated install loop then hits this in
install_isolated_packages(src/install/isolated_install.rs):Every patched package whose preinstall state is not
Donewas unconditionally treated as missing from the cache, so the installer calledenqueue_tarball_for_downloadfor the same URL the resolve phase already downloaded.task_queue.get_or_putfound the stale entry (found_existing), pushed the store entry's install context onto the already-drained callback list, and returned without scheduling anything. That context is never drained, the entry's pending-task slot never releases, andWait::is_donespins onpending_task_count() > 0forever.git:dependencies deadlock the same way through their own task-id space: the isolated installer enqueues them viaenqueue_git_for_checkout, which parks the entry context on the resolve phase's completed checkout task and returns.The hoisted installer is structurally immune: its
package_missing_from_cachechecks the cache for the unpatched folder (the patched_patch_hash=<hash>folder is always derived locally from it), so it never re-requests a tarball the resolve phase already extracted.Fix
Give the isolated loop the same cache check the hoisted installer has, replacing the
break true:PatchInfo::Patch: strip the_patch_hash=<hash>suffix and check for the unpatched cache folder. If present, mark the packageDoneand fall through to the existing path that runsapply_package_patch(which builds the patched folder from the unpatched one) and starts the store task. Only enqueue a download when the unpatched folder is genuinely absent, in which case no stale resolve-phase task can exist for that URL (a failed resolve-phase download removes itstask_queueentry and marks the dedupe entry failed, which surfaces as an install error rather than a hang).PatchInfo::Remove: its computed subpath is already the unpatched folder (contents_hash()isNone), so it now uses the same existence check as unpatched packages instead of always re-downloading. This also fixes the identical hang when removing a patch entry.This also stops isolated installs from re-downloading patched npm tarballs that are already extracted in the cache.
Verification
Two new tests in
test/cli/install/isolated-install.test.ts, both asserting patched/unpatched file contents across install -> add patch -> install -> remove patch -> install:adding and removing a patch for a github dependency in a workspace completes: fake GitHub API viaBun.servewith a hand-built tarball. Includes a cold-cache leg while the patch is active (cache and node_modules wiped, patch still in the lockfile), which exercises the other half of the fix: the install phase downloads the tarball itself and applies the patch after extraction.adding and removing a patch for a git dependency in a workspace completes: a real local repository served over git's dumb HTTP protocol (git update-server-info+ static file serving), covering thegit:clone/checkout task path.On unfixed bun each variant deadlocks at the add-patch install until the test times out; with the fix both run in under a second.
bun bd test test/cli/install/isolated-install.test.ts: 64 passbun bd test test/cli/install/bun-install-patch.test.ts: 18 passbun bd test test/cli/install/bun-patch.test.ts: 31 passAlso verified manually against the shell repros: add/remove/re-add patch, cold-cache install from a patched lockfile, and warm-cache reinstall all complete on isolated and hoisted, with patched content where expected.
Two related pre-existing issues reproduce on released bun without this change and are tracked separately: the re-resolution re-download of git/github dependencies described above, and an isolated-linker staleness where re-adding a previously removed patch is skipped as "no changes" because a stale
.bun-tag-<hash>marker survives the store entry rebuild.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/install/isolated-install.test.ts