Repository navigation
install: don't re-apply a patch per peer variant in isolated installs - #33646
Conversation
The isolated linker applied a patched dependency's patch once per store entry. Peer variants of the same package share one patched cache directory (its name is keyed by the patch contents hash, not the peer set), so every later variant rebuilt and replaced the directory that earlier variants' hardlink tasks were already reading from. On Windows the replacement falls back to delete_tree + rename, which fails the in-flight hardlinks with EPERM; on POSIX it silently leaves each store variant hardlinked to a different orphaned generation of the package. Skip the patch application when the content-addressed patched cache directory already exists, matching how the hoisted installer's preinstall state treats an existing patched cache folder as done. Fixes #28147
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughInstaller::apply_package_patch now checks whether the shared patched cache directory already exists and returns early, skipping rebuild/reapply of the patch for subsequent peer variants. A regression test validates single materialization and shared inodes across variants after install and reinstall. ChangesPatch cache reuse fix
Possibly related issues
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
For context: #31245 (open) is a different isolated-installer concurrency bug in the same file — a trusted-dependency lookup race, not the patched-peer cache rebuild fixed here — so the two are complementary, not duplicates. Flagging since neither references the other and a reviewer of one will want to know about the family. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, well-reasoned early-return guard that mirrors the hoisted installer's existing invariant, with a thorough inode-level regression test.
Extended reasoning...
Overview
This PR adds a 7-line early-return guard (plus 3 lines of comment) to Installer::apply_package_patch in src/install/isolated_install/Installer.rs: if the content-addressed patched cache directory already exists, skip rebuilding it. It also adds a ~95-line regression test to test/cli/install/isolated-install.test.ts that constructs 4 peer variants of a patched package and asserts (via inode identity under --backend=hardlink) that all variants share exactly one patched cache materialization, on both cold and warm cache installs.
Security risks
None. This is a filesystem race-avoidance fix in the isolated linker's patch-application path. No auth, crypto, permissions, or untrusted-input parsing is touched.
Level of scrutiny
Medium — the package installer is production-critical, but the change is narrow and additive (an early return before existing logic). Key correctness properties I verified:
- No leak on early return: the
PatchTaskGuardis armed before the new check, so itsDropcorrectly destroys the boxedPatchTaskwhen we return early. - Content-addressing makes the skip safe:
cache_dir_subpathincludes_patch_hash=<contents-hash>(seePackageManagerDirectories.rs:571), so a changed patch produces a different directory name and is never skipped. - "Exists" ⇒ "complete": per the PR description,
patch_task.apply()builds into a temp dir and renames it into place, so a partially-applied patch from a crashed install cannot leave a directory at the final path. - Mirrors an existing invariant: the hoisted installer's
determine_preinstall_state(PackageManagerLifecycle.rs:125) already treats an existing patched cache folder asdone; this brings the isolated linker to parity rather than introducing new semantics. - Covers both call sites: the guard is inside
apply_package_patch, which is called from bothon_package_extracted's per-entry loop (Installer.rs:213) and the warm-cache path (isolated_install.rs:2386).
Other factors
- The bug-hunting system found no issues.
- No CODEOWNERS cover
src/install/ortest/cli/install/. - The test follows existing patterns in the file (Verdaccio registry harness,
readdirSorted, concurrent pipe drain, exit-code-last), usessys::directory_exists_atconsistently with neighboring code, and asserts a strong invariant (single inode across all variants + cache) rather than just "no error". - The PR description documents that the test fails with the
src/hunk reverted (3–4 distinct inode generations) and that the fullisolated-install.test.ts,bun-patch.test.ts, andbun-install-patch.test.tssuites pass.
|
Empirical check of the bot's "Found 1 issue this PR may fix" list, on Linux x64, debug builds of the PR tip ( #33520 — patchedDependencies silently not applied on warm-cache CI installs: CANNOT VERIFY a fix; no evidence this PR fixes it. Do not add What I ran on both builds:
So: this PR verifiably fixes the #28147 behavior its test covers, but nothing I could run connects it to #33520's symptom (unpatched Issues safe to auto-close from the bot's list: (none — the PR body's existing |
|
@robobun can you identify why CI is failing and fix? |
|
Found it. CI's Fixed in 45e7b10 by pinning |
…in the right place CI's runner.node.mjs exports BUN_INSTALL_CACHE_DIR, which takes precedence over bunfig's install.cache, so the patched cache directory was created outside packageDir and readdirSorted(packageDir/.bun-cache) hit ENOENT.
There was a problem hiding this comment.
I didn't find any issues — the guard is small, content-addressed, fail-safe (.unwrap_or(false)), and the PatchTaskGuard frees the task on the early return — but since this is a concurrency-adjacent change in the package manager's install path, it's worth a maintainer's confirmation.
Extended reasoning...
Overview
This PR adds a ~10-line early-return guard to Installer::apply_package_patch in src/install/isolated_install/Installer.rs: if the content-addressed patched cache directory (<name>@<version>..._patch_hash=<hash>) already exists, skip re-applying the patch. The rest of the diff is a ~100-line regression test in test/cli/install/isolated-install.test.ts that constructs 4 peer variants of a patched package and asserts they all hardlink to a single cache materialization (identical inodes) on both cold and warm-cache installs.
Both call sites of apply_package_patch (the per-entry loop in on_package_extracted at Installer.rs:213 and the warm-cache path at isolated_install.rs:2386) are covered by the single guard. The PatchTaskGuard RAII is armed before the new early return, so the boxed PatchTask is freed on that path. .unwrap_or(false) on the directory_exists_at result means any syscall error falls through to the pre-existing apply behavior. The cache directory is content-addressed by patch hash and produced via atomic renameat_concurrently from a temp dir, so existence implies completeness within a single process, and a changed patch yields a different subpath that is never skipped.
Security risks
None identified. No untrusted-input parsing, path construction, or permission handling is changed; the guard reads a directory-existence bit on a path the installer already computed and was about to write to.
Level of scrutiny
Medium-high. The change is small, follows the established hoisted-installer invariant the PR description cites (determine_preinstall_state treats an existing patched cache dir as done), and the bug-hunting pass found nothing. But it sits in the isolated installer's main-thread orchestration of patch application while background hardlink tasks run concurrently — exactly the class of code where subtle assumptions (e.g. --force, a crashed prior install leaving a stale-but-present directory) are worth a human who owns this subsystem confirming.
Other factors
The regression test is well-constructed (hermetic Verdaccio registry, forces --backend=hardlink for cross-platform inode assertions, pins BUN_INSTALL_CACHE_DIR after the CI failure, covers cold + warm cache), and the author verified it fails on main at the inode assertion and passes on the branch. Maintainers have already engaged: Jarred-Sumner empirically confirmed the fix behavior against #28147 without raising concerns about the change itself, and dylan-conway's CI-failure request was addressed in 45e7b10. No outstanding reviewer comments and no CODEOWNERS on this path.
Brings in the 55 commits upstream had that we did not. Only ten touch packages/opencode, core or tui, and of those only three are fixes in our own path: the namespaced session identity headers, the Cloudflare AI Gateway provider timeouts, and the MCP browser launcher. The rest is web, console, stats and docs. Four conflicts, resolved as follows. `.github/actions/setup-bun/action.yml` — took upstream's plain `bun install` and dropped our Windows retry loop. This reverses what I said earlier about preserving it: upstream now pins Bun 1.4.2 on Windows citing oven-sh/bun#33646, the patched peer-variant fix, which is the root cause our retry papered over. Keeping the loop would also keep `--linker hoisted` on Windows, a node_modules layout that differs from theirs for no remaining reason. `packages/tui/package.json` and `bun.lock` — kept `jsonc-parser`, dropped `open`. `packages/tui/src/context/theme.tsx` imports the first; nothing under packages/tui imports the second any more, because upstream moved the browser opener into core, which declares `open` itself. `packages/opencode/test/cli/run/run-process.test.ts` — comment only. Upstream adds the line explaining why that one test is `cliIt.live` rather than `.concurrent`; the body matches the claim, so the comment stays. Not verified locally, and this is the honest limit of this merge: `bun install` cannot complete in this container (403 on the anomalyco/ghostty-web tarball that packages/app depends on), so `[email protected]`, which the merge introduces for core, is not installed. Without it `packages/core` does not typecheck and the opencode suite loads 787 tests instead of 3723. In that state a regression from the merge is indistinguishable from the missing-module cascade, so CI is the verification. What could be checked: `packages/core` runs 1138 pass with only the two pre-existing flock failures, which fail identically before the merge. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_017Gr2KVmmJbKQPntkpbsnP9
What does this PR do?
Fixes an isolated-linker (
--linker=isolated) bug where a patched dependency that resolves into multiple peer-variant store entries had its patch re-applied once per store entry.All peer variants of a package share a single patched cache directory — its name is keyed by
name@version+ the patch contents hash, not the peer set — butInstaller::apply_package_patchran once per entry. Each re-application rebuilds the patched tree in a temp dir and renames it over the shared cache directory while earlier variants' hardlink tasks are already walking that directory:renameat_concurrentlyhas no atomic-exchange fallback, so it falls back todelete_tree(dest)+rename, deleting the tree out from under the in-flightCreateHardLinkWcalls →EPERM: Operation not permitted: failed to link package(the intermittent CI failure in the issue; OpenCode works around it by forcing the hoisted linker).renameat2(RENAME_EXCHANGE)hides the race, but each store variant ends up hardlinked to a different, orphaned generation of the package instead of the single cache materialization.The fix: skip the patch application when the content-addressed patched cache directory already exists. This is the same invariant the hoisted installer already relies on (
determine_preinstall_statetreats an existing patched cache folder asdone), and it covers both isolated call sites (on_package_extracted's per-entry loop and the warm-cache path inisolated_install.rs). A changed patch produces a different contents hash → a different directory name → is never skipped.Fixes #28147
How did you verify your code works?
Added a test in
test/cli/install/isolated-install.test.ts: a workspace with 4 packages, each resolving a differentno-depsversion, all depending on[email protected](peer onno-deps: *) with apatchedDependenciesentry — producing 4 isolated store variants sharing one patched cache dir. It asserts (with--backend=hardlink) that exactly one patched cache directory exists, the patch is applied in every variant, and every variant's files share the cache file's inode, on both a cold-cache install and a warm-cache reinstall.src/hunk reverted: fails — the 4 variants hold 3–4 distinct inode generations, i.e. each later variant rebuilt the cache dir the earlier ones had already linked from.isolated-install.test.ts(56 tests),bun-patch.test.ts, andbun-install-patch.test.tspass.rm -rf node_modules, changing the patch contents (new hash → new cache dir picked up by all variants), and removing the patch.