install: reinstall the patched package when a removed patchedDependencies entry is re-added with the isolated linker - #37137
Conversation
…an isolated store entry Project-local store entries are built in place, and the clone/hardlink/ copy walk only visits names present in the cache folder, so files only the previous build produced survived a rebuild. Removing a patchedDependencies entry left the patched build's .bun-tag-<hash> marker (and any files the patch created) in the rebuilt store entry; re-adding the same patch then found the stale marker, reported "no changes", and kept the unpatched content. Delete the previous build output before relinking so a rebuild starts clean, matching how global-store entries already replace stale project-local directories.
WalkthroughThe installer now removes the existing project-local store entry before rebuilding it. A regression test covers patch application, removal, restoration, cleanup, and reapplication. ChangesIsolated install patch rebuild
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/install/isolated_install/Installer.rs`:
- Around line 1219-1226: Change the `prev_build` declaration in the cleanup path
to use `AutoAbsPath` instead of `AutoPath`, while preserving the existing
`init_top_level_dir`, `append_real_store_path`, and deletion behavior. Match the
type used by the nearby `staging` cleanup path.
🪄 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: 1197f268-657b-4a34-ba68-4474490b7d62
📒 Files selected for processing (2)
src/install/isolated_install/Installer.rstest/cli/install/isolated-install.test.ts
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/install/isolated_install/Installer.rs:1209-1227— The delete-before-rebuild sits inside the!uses_global_storeblock reached only via the npm/git/github/tarball fall-through arm; the siblingResolutionTag::Folder | ResolutionTag::Rootarm (Installer.rs:889-1076) does its Hardlinker/FileCopier work inline andcontinues before reaching it, so a file deleted from afile:dependency's source folder still survives in the store entry across reinstalls — the exact merge-over defect the new comment describes, in the parallel arm. Folder deps can't be patched, so the specific stale-.bun-tagskip symptom doesn't apply here, but per the "fix the whole class — parallel switch arms" rule it's worth either extending the delete to that arm or stating the intentional exclusion in the PR.Extended reasoning...
What the finding is
The PR's fix — deleting the previous build tree before relinking a project-local store entry — is placed inside the
if !uses_global_store { ... }block atInstaller.rs:1219-1227. That block is only reached after thematch tag { ... }falls through the catch-alltag =>arm at line 1079, which handles npm/git/github/tarball resolutions.The sibling arm
ResolutionTag::Folder | ResolutionTag::RootatInstaller.rs:889-1076handles its install inline viaHardlinker/FileCopier, then doesstep = self.next_step(current_step); continue;at lines 1075-1076. It never reaches the newdelete_tree.Code path that triggers it
isolated_install.rs:2185-2189— forResolutionTag::Folder,installer.start_task(entry_id)is called unconditionally on every install (comment: "folders are always hardlinked to keep them up-to-date"), with noneeds_installgate.Hardlinker.rs:38-46— the walker iterates the sourcefolder_dirand mkdirs/links each entry into the destination. It never enumerates the destination, so files that exist only in the destination are never touched.FileCopierhas the same shape.
So a Folder-dep rebuild merges over the previous store entry rather than replacing it — the identical root cause the PR's own comment at lines 1210-1212 describes ("the clone/hardlink/copy walk only visits names present in the cache folder, so files only the previous build produced would survive a rebuild"), in the parallel switch arm.
Why existing code doesn't prevent it
The Folder arm has no equivalent delete-before-rebuild, and Hardlinker/FileCopier only add/overwrite — they never remove dest-only entries. The new
delete_treeis placed after the arm has alreadycontinued.Step-by-step repro
package.jsonhas"local": "file:./local-pkg"where./local-pkgcontainsfoo.js.bun install --linker isolated→node_modules/.bun/local@file+local-pkg/node_modules/local/foo.jsexists.- Delete
./local-pkg/foo.js. bun install→ Folder arm re-runs (always does), Hardlinker walks the source (which no longer containsfoo.js), links nothing new, and never visits the dest-onlyfoo.js. It survives in the store entry and thus innode_modules/local/.
The "keep them up-to-date" intent is undermined: it only adds/overwrites, never removes.
Impact and why this is a nit
This is pre-existing behavior the PR does not change. The specific symptom the PR targets — a stale
.bun-tag-<hash>marker causingneeds_installto skip a re-added patch — genuinely cannot occur for Folder deps:patch_infois only computed in the non-Folder arm (line 1080-1081), no marker file is written for Folder deps, and Folder deps bypassneeds_installentirely. So the PR is complete for its stated scope.The remaining Folder-arm impact (deleted source files surviving in the store entry) is a milder defect than the skip-on-re-add bug. But the repo's review rule "Fix the whole class in the same PR — grep for every sibling site sharing the pattern: parallel switch arms … If a site is intentionally excluded, say so in the PR" applies directly: this is a same-class sibling site the PR's own root-cause comment identifies, and the PR description lists "npm, git, github, and tarball" without saying why Folder is excluded.
How to address
Either extend the delete-before-rebuild to the Folder/Root arm (delete the destination tree before the Hardlinker/FileCopier walk), or add a sentence to the PR description noting Folder deps are intentionally out of scope (no patch/marker, so the skip symptom can't occur) so the exclusion is on record.
|
Good catch on the parallel arm. The Folder/Root arm is intentionally excluded from this change:
The dest-only-file staleness for folder deps is real though. I verified a file deleted from a |
Force the hardlink backend so the test fails pre-fix on macOS too (the default clonefile backend already replaced whole trees), cover the patch-contents edit path (A -> B -> A hits the same stale-marker false skip through the needs_install marker probe), assert exactly one .bun-tag-* marker after each patched install and none after unpatched ones, and pin the skip path with a same-state install that must still report "(no changes)".
There was a problem hiding this comment.
No issues found; my earlier note about forcing --backend hardlink was addressed in 7969afc. Deferring to a human because this adds a recursive delete inside the isolated linker's install path — small and looks correct, but worth a maintainer sign-off.
What was reviewed:
- The delete is gated to
!uses_global_storeand only reachable for npm/git/github/tarball tags (Folder/Root/Workspace/Symlink exit earlier), soappend_real_store_pathalways resolves undernode_modules/.bun/<entry>/node_modules/<pkg>— never a workspace source or the shared cache. delete_treetreats ENOENT asOk(()), so fresh installs are unaffected; it runs after the stale-symlink detach so it cannot follow a live link into<cache>/links/.- The deleted path is the same one the subsequent link walk repopulates (
Which::Staging==Which::Finalfor project-local entries), andLinkPackageonly runs whenneeds_installalready said yes — no steady-state overhead.
Extended reasoning...
Overview
The PR adds a delete_tree(prev_build) call inside Step::LinkPackage in src/install/isolated_install/Installer.rs, gated to project-local (non-global-store) entries, so that rebuilding a store entry replaces the previous tree instead of merging over it. This fixes a bug where removing and re-adding a patchedDependencies entry left the unpatched package installed because the previous build's .bun-tag-<hash> marker survived the merge and short-circuited needs_install. A ~130-line regression test in test/cli/install/isolated-install.test.ts drives the full patch lifecycle (add → remove → re-add → repeat → edit A→B → edit B→A) and asserts marker counts, file contents, patch-created-file cleanup, and (no changes) output.
Security risks
The change adds a recursive delete of a directory whose path is derived from lockfile data. I traced the path construction: inside the !uses_global_store branch, only ResolutionTag::{Npm, Git, Github, LocalTarball, RemoteTarball} reach the delete (Folder/Root are handled in the earlier arm and continue; Workspace/Symlink hit the inner _ => continue before line 1128). For those tags append_real_store_path → append_store_path builds <top_level>/node_modules/.bun/<store-path>/node_modules/<pkg_name>, which is always under the project's own node_modules/.bun/. The delete runs after the stale-symlink detach block (lines 1147–1207), so a leftover global-store symlink at node_modules/.bun/<entry> has already been unlinked and cannot redirect the delete into the shared cache. delete_tree_open_initial_subpath returns Ok(None) on ENOENT, so a fresh install with no prior build is a no-op. I don't see a way for this to escape node_modules/.bun/ or touch user source.
Level of scrutiny
Medium-high. The Rust change itself is 15 lines and mechanically simple, and the deleted path is exactly the one the immediately-following clone/hardlink/copy walk repopulates (dest_subpath at line 1256 uses the identical append_real_store_path call, and for non-global entries Which::Staging and Which::Final both fall through to append_store_path). But it's a recursive filesystem delete in bun install's core path, which every user hits, so a maintainer familiar with the isolated linker's store-entry lifecycle should confirm the placement and the decision to leave the Folder/Root arm unchanged.
Other factors
All prior review threads are resolved: CodeRabbit's AutoAbsPath nit was addressed in 59b913a; the comment-cop length complaints were addressed by trimming to five lines; my earlier note that the test was vacuous on macOS (clonefile already replaces on EEXIST) was addressed in 7969afc — the test now forces --backend hardlink and additionally covers the patch-content-edit path with marker-count and (no changes) assertions. The PR description explicitly scopes out the Folder/Root arm with a stated reason (folder deps aren't patched, no marker written, re-linked every install) and notes the related folder-dep staleness is filed separately. CodeRabbit flags a related PR #37136 touching the same area, which is another reason a human should coordinate the merge.
1 participant
What
With
--linker isolated, removing apatchedDependenciesentry and later re-adding the same entry leaves the unpatched package installed. The re-add install reportsDone! Checked N packages (no changes)whilenode_modulesstill has the unpatched content. The hoisted linker handles the same sequence correctly. Applies to npm, git, github, and tarball dependencies.Cause
Project-local store entries (
node_modules/.bun/<pkg>@<ver>/node_modules/<pkg>) are rebuilt in place, and the clonefile/hardlink/copyfile walk only visits names present in the source cache folder. Files that only the previous build produced survive a rebuild.So the rebuild triggered by removing a patch overwrites colliding files with the unpatched versions, but leaves the patched build's
.bun-tag-<contents-hash>marker (and any files the patch created) sitting in the store entry. The install that re-adds the patch decides whether the entry is up to date by checking for exactly that marker file (needs_installinisolated_install.rs), finds the stale one, and skips the entry.Observed store entry after step 2:
.bun-tag-60dea556caf42d1e,index.js(unpatched),package.json.Fix
In the
LinkPackagestep, delete the previous build output before relinking a project-local store entry, so a rebuild replaces the old tree instead of merging over it. This also removes files a patch created once the patch is removed (previously they survived innode_modules).Global-store entries already get replace semantics (builds are staged and renamed into place, and
link_project_to_global_storedeletes a stale project-local directory), so this aligns the default project-local path. The delete runs after the stale-symlink detach, so it never follows a live link into the shared global store, and it is an ENOENT no-op for fresh installs.The
Folder/Rootarm ofLinkPackageis intentionally not changed: folder dependencies cannot be patched (no.bun-tag-*marker is ever written for them, and they bypass theneeds_installmarker check), and they re-link on every install, so giving them delete-before-rebuild semantics would change steady-state behavior (for example lifecycle-script artifacts in the package dir) and is tracked as a separate fix.Test
test/cli/install/isolated-install.test.ts: drivesno-deps@1.0.0through the full patch lifecycle against the local registry: install, add patch A, remove it, re-add it, repeat in place, edit the patch to B, edit back to A. Patch A modifiesindex.jsand createspatched.txt, so the legs cover a file each rebuild must revert and one it must delete; editing the patch contents exercises the second door into the same stale-marker skip (the.bun-tag-<hash>probe inneeds_install). After every install the test asserts the store entry holds exactly one.bun-tag-*marker when patched and none when not, that rebuild installs do not report(no changes), and that a same-state repeat install still does (the marker survives the delete-and-relink round trip). The install forces--backend hardlinkbecause macOS defaults to clonefile, which already replaced whole trees, so the merge bug only reproduces under the walking backends.Fails before the fix (first at
patched.txtsurviving the patch removal), passes after. The fullisolated-install,bun-install-patch, andbun-patchsuites pass with the change.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