install: remove the node_modules links of a renamed or removed workspace - #38511
install: remove the node_modules links of a renamed or removed workspace#38511robobun wants to merge 5 commits into
Conversation
Both linkers only create or repoint the node_modules/<name> links the current lockfile asks for. When a workspace package is renamed (or removed from the project), its old name leaves the lockfile and the install summary counts it as removed, but the link under the old name stayed behind in the root node_modules and, with the isolated linker, in the node_modules of every workspace that depended on it. After the lockfile is rebuilt and before the linker runs, unlink node_modules/<name> at the root and in every current workspace for each workspace name that was in the previous lockfile and is not in the new one. Only symlinks (junctions on Windows) are removed, and because this happens before linking, a name the new lockfile still places there is linked again.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 7 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 (4)
Comment |
|
Status Reproduced on the 1.4.0 release and on a debug build of main with both linkers: after renaming a workspace package and running Fix in this PR: #38511. Scope: the install that performs the rename or removal (the description's Scope section covers what this deliberately does not do and how it relates to #38333). The new tests are the CI on the final commit (1d58928, build 96349): 177 of 179 jobs passed, including every Linux, ASAN and Windows test shard; the build is marked failed only because the two macOS shards expired in the queue without ever running. The annotations are flaky tests unrelated to install that passed alone or on retry. Ready for a maintainer decision. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes install_with_manager to delete entries from node_modules — a behavior change in the core install path — a human look would still be worthwhile.
What was reviewed:
- The
had_any_diffsgate and placement afterclean_with_logger/ before the linker; confirmed--dry-run,--lockfile-only, and--frozen-lockfilenever reach the new call. - Path construction:
alias_is_safe_install_targetguards the name component, and thereadlinkgate keeps real directories/files untouched (the Windowsrmdir→unlinkfallback matches the pattern inisolated_install/Symlinker.rs). lockfile_before_cleanstill holds the old workspace packages after the diff (onlyworkspace_pathswas overwritten in place, which this code does not read fromprevious).
Extended reasoning...
Overview
Adds remove_stale_workspace_links (~90 lines of Rust) that runs during install_with_manager after Lockfile::clean_with_logger and before either linker. It walks the pre-clean lockfile's packages, and for every ResolutionTag::Workspace whose name hash is absent from the new lockfile's workspace_paths, unlinks node_modules/<name> in the project root and in every current workspace's directory. The call is gated on had_any_diffs, and each removal is gated on readlink succeeding (so only symlinks/junctions are touched). Ten new tests in bun-workspaces.test.ts cover rename/remove for both linkers, scoped names, an alias that still wants the old name, a real directory being left alone, and --dry-run.
Security risks
None identified. The only untrusted-ish component of the removed path is the workspace name, which is filtered through the same alias_is_safe_install_target check the linkers already apply (rejects empty, ./.., absolute, backslash, NUL, and multi-segment non-scoped names). Workspace paths come from the user's own package.json. The readlink gate means a non-symlink at that path is never deleted.
Level of scrutiny
High. install_with_manager is the core of bun install, and this introduces a new place where bun deletes things from node_modules. The gating looks sound (had_any_diffs is only set in the LoadResult::Ok diffing branch, so a fresh install or unchanged lockfile skips it entirely; --dry-run clears INSTALL_PACKAGES so the block is never entered; --lockfile-only returns earlier; --frozen-lockfile with a diff crashes before this point). But the decision of what to clean (workspace links) vs. what to leave (.bin entries, entries stale from a previous install on another machine) is a scoping call a maintainer should confirm.
Other factors
The PR description is unusually thorough and the test matrix is good (both linkers × rename/scoped-rename/remove/alias-reuse, plus the negative cases). I verified the lockfile_before_clean snapshot still contains the old workspace packages entries after the in-place diff (the diff only overwrites workspace_paths/workspace_versions/etc. on the old lockfile, and the new code reads previous.packages, not previous.workspace_paths). The Windows removal (rmdir then unlink on failure) is a slightly broader fallback than Symlinker.rs (which only falls back on EPERM), but since this is best-effort cleanup with errors discarded either way, that difference is harmless.
|
On the one difference noted above: the Windows fallback is deliberately broader than the one in |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Checked both against their diffs; neither covers the case this PR fixes, and together they still do not.
Neither removes the root link after a rename under the hoisted linker (the repro in the description), and neither removes |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Since it changes bun install's filesystem behavior (unlinking entries in every workspace's node_modules based on the lockfile diff) and overlaps in scope with #32974 / #29794, a human look at the approach would still be worthwhile.
Checked: readlink gate keeps real directories/files intact (tested); alias_is_safe_install_target blocks path escape; --dry-run/--lockfile-only/--frozen-lockfile never reach the new call; append(b"") for the root case is a no-op after trim; running before the linker lets a still-wanted alias be recreated (tested for both linkers).
Extended reasoning...
Overview
Adds src/install/PackageManager/remove_stale_workspace_links.rs (~86 lines) and wires it into install_with_manager immediately before the linker runs. When the lockfile diff shows changes, it walks the previous lockfile's packages, and for every ResolutionTag::Workspace package whose name hash is no longer in the new lockfile's workspace_paths, it unlinks node_modules/<name> in the project root and in every current workspace directory. Ten new tests in test/cli/install/bun-workspaces.test.ts cover both linkers, scoped names, removal, alias re-linking, real-directory preservation, and --dry-run.
Security risks
The name that becomes a path component is guarded by the same alias_is_safe_install_target the linkers already use (rejects empty, ./.., absolute, backslash, NUL, and anything with more than one /), so it cannot escape node_modules. Only entries where readlink succeeds are removed, so a real directory or file the user placed there is left alone. The workspace paths iterated come from the current lockfile Bun just built. I don't see a path-traversal or unintended-deletion vector.
Level of scrutiny
High. This is the main bun install code path and it now deletes filesystem entries it previously left in place. The change is defensive (had_any_diffs gate, symlink-only removal, safe-name check, runs before the linker so anything the new lockfile still wants is recreated) and thoroughly tested, but it is a behavioral/design change rather than a mechanical fix. Two open PRs (#32974, #29794) take a different, scan-driven approach to overlapping cleanup; the author explains why neither covers this case, but a maintainer should weigh in on which direction to take.
Other factors
- The comment-cop bot re-fired at 17:13 on the current commit; the comments in the diff are now 1–5 lines each and look fine to me, so I read those as resolved by a02fc5d.
- The one CI failure so far (
test-cluster-shared-leak.jstimeout on Windows aarch64) is unrelated to this change. AutoAbsPathChecked::appendreturnsOk(())on empty input after trim, so passingb""for the root'spackage_diris correct; scoped names keep their single/inside the appended segment.- The Windows
rmdir-then-unlinkfallback matches how the isolated linker'sSymlinkerremoves wrong links, per the author's note.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes what bun install deletes from node_modules and overlaps with two other pending prune PRs (#32974, #29794), a human look would still be worthwhile.
What was reviewed:
- Path safety: names go through
alias_is_safe_install_target(rejects.., empty, NUL, etc.) before use as a path component, and only entries wherereadlinksucceeds are removed — real directories and files are left alone. - Ordering: runs after
clean_with_loggerand before the linker, so anything the new lockfile still places under the old name is re-linked (covered by the alias test); gated onhad_any_diffsinsidedo_.install_packages(), so no-op installs,--dry-run,--lockfile-onlyand--frozen-lockfilenever reach it. - The Windows
rmdir→unlinkfallback matches the existing pattern inisolated_install/Installer.rs;needs_new_lockfilepaths leavehad_any_diffsfalse solockfile_before_cleanis never read in a state where it lacks the old workspace entries.
Extended reasoning...
Overview
Adds src/install/PackageManager/remove_stale_workspace_links.rs (~80 lines) and a call site in install_with_manager gated on had_any_diffs && do_.install_packages(). When a workspace's name leaves the lockfile (rename or removal), unlinks node_modules/<name> in the project root and in every current workspace, for both linkers. Ships 10 new tests in bun-workspaces.test.ts covering rename, scoped rename, removal, alias re-link, directory-left-alone, and --dry-run, each across both linkers where applicable.
Security risks
The only untrusted-ish input is a package name from the previous lockfile. It is filtered through the existing alias_is_safe_install_target (rejects empty, ., .., \\, :, NUL; allows only name or @scope/name) before being appended to <top_level_dir>/[<workspace_path>/]node_modules/, and only paths where readlink succeeds are ever removed. No traversal or over-deletion vector found.
Level of scrutiny
High — this is bun install, and the change deletes filesystem entries. That said, the logic is narrow, best-effort (all syscall errors discarded), only touches symlinks/junctions, and runs immediately before the linker that would re-create anything the new lockfile still wants. The Windows branch mirrors isolated_install/Installer.rs:1171. I traced the had_any_diffs / needs_new_lockfile interaction: when the differ short-circuits to a new lockfile, had_any_diffs stays false and this code never runs, so lockfile_before_clean is always the loaded-from-disk lockfile with the old workspace entries.
Other factors
The PR description is unusually thorough about mechanism and scope. The author already addressed the comment-cop feedback (comments are now one-liners). Two related PRs (#32974, #29794) propose broader node_modules pruning; the author's analysis of why neither covers this case looks correct, but a maintainer should decide whether to land this targeted fix now or fold it into a general prune. Given it deletes from node_modules in a critical path, deferring rather than auto-approving.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it adds a new deletion step to the bun install hot path and overlaps with the design space of #32974 / #29794 / #38333, a maintainer should confirm the approach before it lands.
What was reviewed:
- Traced the call site: gated on
had_any_diffs(only set inside theLoadResult::Okdiffer arm) and behinddo_.install_packages(), so--dry-run/--lockfile-only/--frozen-lockfile/ no-lockfile installs never reach it. - Path safety: names go through
alias_is_safe_install_target(rejects.,.., empty,\/:/NUL, and >2 components), andAutoAbsPathChecked::append(b"")is a no-op so the root path is built correctly. readlinkgate leaves real directories/files alone; verified against the "a directory under the old name" test.
Extended reasoning...
Overview
The PR adds a new 80-line module remove_stale_workspace_links.rs and a 5-line call site in install_with_manager.rs, plus a 176-line test block in bun-workspaces.test.ts (10 tests × 2 linkers). It compares the pre-clean lockfile's workspace-resolved packages against the post-clean lockfile's workspace_paths, and for any name that dropped out, unlinks node_modules/<name> in the project root and in every remaining workspace directory — but only if the entry is a symlink/junction (checked via readlink).
Security risks
The only filesystem write is a targeted unlink/rmdir of a path built from <top_level_dir>/[<workspace_path>/]node_modules/<name>. <name> comes from the previous lockfile and is validated by the same alias_is_safe_install_target the linkers use (no .., no absolute components, at most @scope/name). <workspace_path> comes from the current lockfile's workspace_paths, which is the same source the linkers already use to place links. AutoAbsPathChecked::append returns Err(MaxPathExceeded) on overflow, which is handled by early return. I don't see a path-traversal or accidental-deletion vector; the readlink gate additionally means a real directory can never be removed.
Level of scrutiny
High. install_with_manager is the core of bun install, and this adds a new syscall path that runs on every install where the diff is non-empty. A regression here (e.g., unlinking something the linker won't recreate) would corrupt users' node_modules. The change itself is small and defensive, but the placement and interaction with the two linkers is subtle enough that I'm not comfortable approving it without a maintainer look.
Other factors
- The PR description is unusually thorough and the author has already responded to the comment-cop bot and the duplicate-PR bot with detailed rebuttals.
- Test coverage is good: both linkers × {rename, scoped rename, remove, alias-still-uses-old-name}, plus the negative cases (real directory left alone,
--dry-runskipped). 7 of 10 tests fail on release per the description, satisfyingUSE_SYSTEM_BUN=1validation. - CI on the head commit passed 177/179 (2 macOS shards expired unrun).
- The author explicitly notes this sits in the same design space as #38333's install-time prune and two other open PRs; the description says the module "becomes deletable" if a general prune lands. Whether to merge this narrow fix now or wait for/fold into the general prune is a maintainer call.
- I checked one candidate concern the bug-hunt didn't flag: when
needs_new_lockfileis true,lockfile_before_cleanis not the loaded-from-disk lockfile but the freshly-built one — howeverhad_any_diffsis only ever set inside theLoadResult::Okdiffer block, so that path never reachesremove_stale_workspace_links.
|
No changes needed from the review above. The merge question it raises (narrow fix now vs waiting for a general install-time prune) is the one already laid out in the PR description: this removes dangling links left behind by a workspace rename or removal today, and the module can be deleted if a general prune like #38333 lands later. That call is a maintainer decision. |
2 participants
Problem
package.jsonnameand the dependencies on it) and runbun install: the summary prints2 packages removed, butnode_modules/<old name>is still there, pointing at the same folder asnode_modules/<new name>. Imports of the old name keep working on the machine the rename was made on, so the leftovers are found by CI or the next fresh clone instead of by the person doing the rename.packages/<dependent>/node_modules/<old name>for every workspace that depended on it. Removing a workspace from the project leaves its (now dangling) link behind the same way. Scoped names too. Reproduces on 1.4.0 and main with both linkers (script and output below). Found by a bot while tighteningbun-install.test.ts; there is no user report for this specific case, only the general stale-entry issues (Runningbun installdoes not delete extraneous dependencies #16176 and friends).Step::SymlinkDependenciesinsrc/install/isolated_install/Installer.rs; hoisted:verify+install_from_linkinsrc/install/PackageInstaller.rs), so an entry whose name is no longer in the lockfile is never visited. Theremovedcount comes from the lockfile diff (src/install/lockfile/Package.rs:1336, "It will be cleaned up later"), and nothing cleans it up.2is the two root dependency edges on the old name: the implicit edge every workspace gets on the root package plus the explicitdevDependenciesentry (a workspace the root does not list prints1). The count is left as is.Scope
bun.lockalready has the new name) keeps the stale link. That machine is also the one with nothing to find; entries that went stale on some earlier install are the general extraneous-entry problem (Runningbun installdoes not delete extraneous dependencies #16176), which needs a scan ofnode_modulesagainst the lockfile. install: pnpm parity — dedupe, prune, pm licenses, audit fix, add --filter/--catalog, nested overrides, transitive update, and workspace fixes #38333 adds that scan as the manualbun prune(its tests cover the renamed-workspace link) and says the automatic form should be built on prune's planner later; this PR does not change that, and if an install-time prune lands this module becomes deletable while the tests here still apply as they are.remove_collapsed_copies(manager, &lockfile_before_clean)also compares the previous lockfile with the new one frominstall_with_manager, for dedupe / audit fix / update on the hoisted linker. Plainbun installafter a rename is not covered by it, andgit merge-treeof this branch against install: pnpm parity — dedupe, prune, pm licenses, audit fix, add --filter/--catalog, nested overrides, transitive update, and workspace fixes #38333's head reports no conflicts..binentries are not touched. For a rename the linker repoints the bins when it links the new name (checked with both linkers); for a removed workspace the bin link stays behind, as it does for any removed dependency today.Fix
src/install/PackageManager/remove_stale_workspace_links.rs, called frominstall_with_managerafterLockfile::clean_with_loggerand before the linker runs: every package with aworkspaceresolution in the previous lockfile whose name is not in the new lockfile'sworkspace_pathshasnode_modules/<name>unlinked in the project root and in every workspace of the new lockfile.had_any_diffs). A renamed or removed workspace always changes the root package's dependency list, and without a diff the two workspace sets are identical, so a no-opbun installdoes no extra work.--dry-run,--lockfile-onlyand--frozen-lockfilenever reach it.readlinksucceeds on: symlinks, and on Windows junctions, removed withrmdirthe way the isolated linker'sSymlinkerreplaces a wrong link. A real directory or file under the old name was not put there as a workspace link and is left alone. Names pass the samealias_is_safe_install_targetcheck the linkers apply before they are used as a path component."a": "workspace:a-renamed@*", or an npm package that now has that name), the linker sees it missing and links it again, exactly as on a fresh install. The hoisted linker verifies links throughnode_modules/<alias>/package.json, the isolated linker throughreadlink, so both recreate a missing one.test/cli/install/bun-workspaces.test.ts, newlinks of a renamed or removed workspaceblock: rename (root link, and with isolated the dependent workspace's link; a third install reports no changes), scoped rename and removal of a workspace, each for both linkers; the old name linked again when an alias still uses it, for both linkers; a directory under the old name kept;--dry-runleaving the links alone. 7 of the 10 fail on the current release, all 10 pass with the fix.bun-workspaces,isolated-install,bun-add,bun-update,bun-removeand the workspace tests ofbun-install.test.tsagainst the debug build (all pass), andcargo checkof the crate for the Windows and macOS targets.Background
node_modules. Both linkers create a symlink (on Windows a symlink or junction) named after the dependency alias that points at the workspace folder. The hoisted linker links every workspace into the rootnode_modulesunder its own name, becausePackage::parsegives the root package an implicit dependency on each workspace; the isolated linker only links the workspaces a package explicitly depends on, into that package's ownnode_modules.bun installdiffs the loaded lockfile againstpackage.json(Diff::generate, the source of the add/remove counts in the summary), thenLockfile::clean_with_loggerbuilds a fresh lockfile holding only the packages still reachable from the root.install_with_managerkeeps the old one aslockfile_before_clean; itspackagesstill contain the old workspace entries, which is what this change reads.workspace_pathson the new lockfile maps the name hash of each current workspace to its path.node_modulesonly and keeps symlinks under the hoisted linker; install: prune stale workspace node_modules in hoisted installs #29794 walks workspace directories under the hoisted linker only.Repro (release 1.4.0, same result with
--linker hoisted)mkdir -p ws/packages/{a,b} && cd ws echo '{"name":"root","workspaces":["packages/*"],"devDependencies":{"a":"*"}}' > package.json echo '{"name":"a","version":"0.0.0"}' > packages/a/package.json echo '{"name":"b","version":"0.0.0","dependencies":{"a":"*"}}' > packages/b/package.json bun install echo '{"name":"root","workspaces":["packages/*"],"devDependencies":{"a-renamed":"*"}}' > package.json echo '{"name":"a-renamed","version":"0.0.0"}' > packages/a/package.json echo '{"name":"b","version":"0.0.0","dependencies":{"a-renamed":"*"}}' > packages/b/package.json bun install # 2 packages removed ls node_modules packages/b/node_modulesBefore:
After (
BUN_DEBUG_PackageManager=1on a debug build also logs the two removals):bun.lockis identical in both cases and only containsa-renamed.