install: remove files deleted from a folder dependency on reinstall with the isolated linker - #37138
install: remove files deleted from a folder dependency on reinstall with the isolated linker#37138robobun wants to merge 4 commits into
Conversation
…ed linker relinks its store entry The isolated linker rebuilds a folder (file:) dependency's store entry on every install by walking the source folder and hardlinking/copying each entry. The walk only visits names present in the source, so the rebuild merged over the previous contents and a file deleted from the dependency's folder survived every subsequent install. The hoisted linker removes the destination before reinstalling, so the two linkers diverged. Delete the store entry's package directory before the walk. Lifecycle scripts re-run on every install for folder entries under both linkers, so artifacts they write into the package directory are recreated, matching hoisted behavior.
WalkthroughThe installer now removes existing store entries before rebuilding folder and root packages. A regression test verifies that reinstalling a folder dependency removes deleted files, updates modified files, and preserves dependency links. ChangesIsolated install cleanup
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…se builds too A scheduling invariant in isolated_install.rs is what keeps the root project entry out of LinkPackage; if that ever broke, the debug_assert would compile out and the delete would remove the project directory. Assert on the delete's precondition directly (the path extended past the top-level dir) so release builds fail loudly instead.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
My earlier concern (debug_assert → release assert) was addressed in 38d7270. I didn't find further issues, but this adds a delete_tree call to the package manager's install path, so a maintainer familiar with the isolated linker should sign off.
Checked: delete_tree returns Ok on ENOENT so first-install is unaffected; the delete runs only after the source folder opens, so a missing source doesn't wipe the previous build; append_store_path for Root with a valid dep_id and for Folder both extend into node_modules/.bun/..., so the release assert covers the only unextended case. The hardlink→copyfile fallback loop runs after the single delete, so a mid-walk EXDEV retry doesn't re-delete.
Extended reasoning...
Overview
The PR adds a delete_tree of the store entry's package directory before the isolated linker's Folder | Root rebuild walk (src/install/isolated_install/Installer.rs:905-919), so files removed from a file: dependency no longer survive reinstalls. A regression test in test/cli/install/isolated-install.test.ts covers a deleted file, a deleted nested directory, a modified file, and that the entry's dependency symlinks (no-deps) still resolve after rebuild.
Security risks
The new call recursively deletes a directory computed from init_top_level_dir() + append_store_path. The degenerate case where append_store_path appends nothing (Root entry with invalid_dependency_id) would target the project root itself. That path is unreachable via task scheduling today (isolated_install.rs sets the root entry's step to SymlinkDependencies), and the release-mode assert!(prev_build.len() > top_level_len) now guards it at the delete site. I traced append_store_path (Installer.rs:2702-2742): for Root with a valid dep_id and for every non-root tag it appends node_modules/.bun/<entry>/node_modules/<name>, so the assert holds on every reachable path. No injection or auth surface is touched.
Level of scrutiny
High. This is a destructive filesystem operation inside bun install, on a path derived from lockfile/store state. The blast radius of a wrong path is user data loss. The change is small and mirrors the hoisted linker's uninstall_before_install, but destructive changes to the package manager warrant a maintainer's eyes even when the diff looks correct.
Other factors
delete_tree_open_initial_subpath(src/sys/dir.rs:256) returnsOk(None)onENOENT, so the first install (no prior store entry) is not broken.- The delete is placed after
open_dir_for_iterationon the source succeeds, so a missing/renamed source folder fails the task without destroying the previous build — matches the PR description's claim. - The delete sits before the
'backend: loop, so the EXDEV/EACCES →Copyfilefallback does not re-delete a partially-hardlinked destination. - The test follows existing patterns in the file (
readlinkSync+joinfor symlink targets,registry.createTestDir,runBunInstall), and the sibling test "can install folder dependencies on root package" already exercises theRoot-with-valid-dep_idarm. - The comment-cop bot re-flagged the trimmed comments; the author pushed back with a reasonable justification and the threads are resolved. That's a style call for a human, not a correctness issue.
…t-as-dep and missing-source relinks Replace the length assert with the explicit condition it encoded, matching the tag guard on_task_fail uses for its cleanup delete: only Folder entries and Root entries that are a dependency on the root package get the delete; the plain root entry's store path is the project dir itself. Extend the root-as-dep test with a reinstall after deleting a project file, so the Root half of the arm runs against an existing build in CI, and pin the open-before-delete ordering: a folder dependency whose source folder disappeared fails the install without destroying the previous build.
|
Self-review on the last push surfaced three gaps, addressed in 2f50fa4:
Full suite: 64 pass. |
There was a problem hiding this comment.
The earlier concern about delete_tree reaching the project root is now addressed — the explicit Folder || dep_id != invalid guard fails closed and matches exactly when append_store_path produces a store subpath. I didn't find anything else wrong, but this adds a recursive delete to the install path, so it's worth a maintainer's eyes before merging.
What was reviewed:
- Traced
append_store_pathfor bothFolderandRoottags — the guard covers every case where it would return the top-level dir. delete_treetreats ENOENT as success, so first install (no prior build) is unaffected.- Delete is placed after the source-folder open and before the
'backendloop, so a missing source preserves the old build and the hardlink→copyfile fallback doesn't re-delete.
Extended reasoning...
Overview
Adds a delete_tree of the isolated-linker store entry's package directory before the hardlink/copy walk in the Folder | Root arm of Step::LinkPackage, so files removed from a file: dependency are pruned on reinstall instead of surviving forever. ~20 lines of production code in src/install/isolated_install/Installer.rs plus three new/extended tests in test/cli/install/isolated-install.test.ts.
Security risks
The destructive operation is a recursive delete. The path is derived from lockfile resolution data via append_store_path, which for Folder and Root-with-valid-dep_id always yields <top_level>/node_modules/.bun/<entry>/node_modules/<name>. The one degenerate case (Root with invalid_dependency_id, whose store path is the project dir itself) is now excluded by an explicit condition rather than the earlier debug_assert! I flagged — so it fails closed in release builds. Fd::cwd().delete_tree(abs_path) is the same pattern already used at six other sites in this file. No injection, auth, or network surface.
Level of scrutiny
High — this is a recursive filesystem delete inside the package manager, where a wrong path is unrecoverable data loss. The change itself is small and the guard is provably correct against the current append_store_path implementation, but the invariant it depends on (task scheduling never routes the root-project entry through LinkPackage) lives in a different file, and adding a new destructive call to that arm is the kind of thing a maintainer familiar with the isolated linker's scheduling should confirm.
Other factors
My earlier review comment was addressed across three follow-up commits, ending with the explicit tag-based guard (better than the release assert! I'd suggested — it skips rather than panics). Test coverage is thorough: deleted file, deleted nested dir, modified file, dependency symlinks preserved, root-as-dep ("self": "file:.") reinstall with the project's own files untouched, and the open-before-delete ordering pinned by a missing-source test. The one remaining unresolved inline comment is the automated comment-cop linter re-firing on the explanatory comment; the author already trimmed twice and the current comment is genuinely load-bearing (it explains why the delete must exist), so I don't consider it blocking.
|
Updated 8:34 AM PT - Aug 7th, 2026
✅ @robobun, your commit 2f50fa46716610324830f560b1f3d86a9e10fe3a passed in 🧪 To try this PR locally: bunx bun-pr 37138That installs a local version of the PR into your bun-37138 --bun |
|
does this mean we delete the entire folder before each install if it's in the store? i'm not sure we can do that |
|
Yes, with tight scope: on every install, for folder ( What made me judge the delete acceptable:
If the full delete is still not acceptable (for example the transient window where the whole directory is empty mid-install, which hoisted also has), the alternative is an incremental prune: after the link walk, walk the destination and remove only names that no longer exist in the source. That keeps unchanged files in place and closes the window, at the cost of a second directory walk and more code. Happy to rework it that way if you prefer. |
2 participants
Repro
With
linker = "hoisted"the second install correctly removesnode_modules/local/foo.js.Cause
The isolated linker relinks folder (
file:) dependencies on every install to keep them up to date. TheLinkPackagestep'sFolder | Rootarm (src/install/isolated_install/Installer.rs) rebuilds the store entry by walking the source folder withHardlinker/FileCopier, which only visits names present in the source: existing destination files are overwritten, but files that exist only in the destination are never touched. So the rebuild merges over the previous build, and a file deleted from the dependency's folder survives innode_modules/.bun/<name>@file+<path>/node_modules/<name>forever.The hoisted linker runs
uninstall_before_install(delete, then copy) for the same case, so the two linkers diverged.Fix
Delete the store entry's package directory before the walk, once per relink, after the source folder has been opened (a missing source still fails without destroying the previous build). Only the package directory inside the entry is removed; the entry's
node_modulessiblings (dependency symlinks) are left for the later steps, which re-run on every install for folder entries.This matches hoisted semantics for lifecycle scripts too: both linkers re-run a folder dependency's scripts on every install, and hoisted already wipes the destination first, so artifacts scripts write into the package directory are recreated the same way under both linkers.
Root resolutions reach this arm only as a dependency on the root package (
"x": "file:."), whereappend_store_pathyields the store entry's package dir. The delete is additionally scoped by resolution tag, the same wayon_task_failguards its cleanup delete: a Root entry that is not a dependency on the root package (whose store path is the project dir itself) never reaches it, in release builds too.Same defect class as #37137 (npm/git/tarball arm of the same step); the two changes are in different arms and independent.
Verification
Tests in
test/cli/install/isolated-install.test.tscover a deleted file, a deleted nested directory, a modified file, and that the entry's dependency symlinks still resolve after the rebuild; the root-as-dependency case ("self": "file:.") reinstalling over an existing build with a file deleted from the project (and the project's own files untouched); and that a folder dependency whose source folder disappeared fails the install while the previous build and thenode_modulessymlink survive. It fails on bun 1.4.0-canary.1 (extra.jsstill exists after reinstall) and passes with this change. All 63 tests in the file pass, as doesbun-install-hardlink-fallback.test.ts(exercises the copyfile fallback in the same arm).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