install: link packages into a staging directory and rename them into place - #38783
install: link packages into a staging directory and rename them into place#38783robobun wants to merge 3 commits into
Conversation
|
Warning Review limit reached
Next review available in: 5 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 |
|
Updated 6:03 AM PT - Aug 15th, 2026
❌ @robobun, your commit e2740a7 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 38783That installs a local version of the PR into your bun-38783 --bun |
|
Status: reproduced on main with both linkers by killing The first CI run found two cases the first revision got wrong, both fixed in d0a3640 and covered by tests in the same file: aliases that are already 255 bytes long (the staging name is now a hash), and a workspace depended on under two names, whose nested packages the hoisted linker installs twice into the same directory (an occupied final path is now replaced like a reinstall). Both tests that failed in that run ( CI for e2740a7 (rebased onto current main, adapting to #38271's |
|
Found 6 issues this PR may fix:
🤖 Generated with Claude Code |
![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 the automated bug hunt found no issues. Given that it restructures the core install path for both linkers (stage-then-rename, backend-loop control flow, on_task_fail cleanup) plus a Windows-specific rename retry, a human look from someone who owns bun_install would still be worthwhile.
What was checked:
append_real_store_path(.., Which::Final)is behavior-preserving for non-global entries;append_store_path's_arm now delegates to the new helper with the same output.delete_treereturnsOkonENOENT, so making the pre-link cleanup fatal inStep::LinkPackagedoesn't break the fresh-install case.- Folder/Root entries branch off before the staged
'backendloop and keep their ownnext_step, so they neither stage nor hitcommit_local_store_package;on_task_failcorrectly dropsFolderfrom the cleanup match for the same reason. install_from_linkis a separate entry point frominstall()and is not routed through the staging path.
Extended reasoning...
Overview
The PR changes both package linkers to write into a dot-prefixed staging sibling (.<name>.bun-tmp) and renameat onto the final path once linking completes, so an interrupted install never leaves a partial tree that later installs treat as complete. Touches PackageInstall.rs (hoisted linker: new StagingPath, rename_staging_into_place, install() split into a wrapper + install_into() with all backend methods now taking an explicit dest_subpath), isolated_install/Installer.rs (isolated linker: 'backend loop restructured to break to a shared commit_local_store_package tail, new append_store_package_path, on_task_fail cleanup rewritten), a comment in isolated_install.rs, and a new SIGKILL-during-link test.
Security risks
None identified. This is filesystem I/O inside node_modules; no auth, crypto, or untrusted-input parsing is added. The staging name is derived from the package name, which is already used to build the final path.
Level of scrutiny
High. This is the core write path of bun install — it runs for every package on every install, on all platforms, across five backends (clonefile, clonefile-each-dir, hardlink, copyfile, symlink) with fallback transitions between them. The isolated-linker change also restructures control flow (three step = self.next_step(); continue 'step; sites collapsed into break 'backend to a shared tail) and replaces the on_task_fail cleanup. The Windows retry loop is new platform-specific behavior. Any regression here breaks package installation.
Other factors
The PR description is thorough, cites the mechanism precisely, lists which existing test suites were run locally, and confirms cargo check on all three target triples. The new test covers both linkers with a real kill-during-link scenario and asserts both the partial-tree invariant and that a follow-up install repairs it with no leftover staging directory. Spot-checks (delete_tree ENOENT handling, Folder/Root bypass, Which::Final path-builder equivalence, install_from_link being a separate entry point, buf_print_z overflow handling) all held up. No prior human review on the timeline. The scope and criticality — every-install hot path, control-flow refactor, cross-platform rename semantics — put this outside what should land without a maintainer look, even absent identified bugs.
![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. Because it changes the core install flow for both linkers (link-in-place → stage-then-rename) and adds Windows-specific retry behavior on the hot path of every package install, a human look would still be worthwhile.
What was reviewed:
- Traced
install()→install_into()threading ofdest_subpaththrough all five backends (clonefile, clonefile-each-dir, hardlink, symlink, copyfile) and the fallback chain — each now writes to the staging path. - Confirmed the isolated linker's folder/root branch keeps its own
'backendloop withappend_store_pathandnext_step, so it does not stage or reachcommit_local_store_package; the outer staging path is only reached for Npm/Git/Github/LocalTarball/RemoteTarball, matching theon_task_failcleanup match. - Checked
append_real_store_path(.., Final)vs(.., Staging)for those tags both resolve to siblings undernode_modules/.bun/<storepath>/node_modules/, socommit_local_store_package's rename is between siblings, not into an ancestor. delete_treereturnsOkonENOENT, so the new hard-fail on stale-staging cleanup does not break fresh installs; it does upgrade a previously fire-and-forget delete of the final path to a hard error, which is required now that the rename needs it free.
Extended reasoning...
Overview
This PR changes how both the hoisted and isolated package linkers materialise packages into node_modules: instead of linking files directly into the final directory, each backend links into a deterministic .bun-tmp-<hash> sibling and renames it onto the final path once complete. The goal is to make an interrupted bun install leave either nothing or a complete package at the final path, so the existing package.json-based skip check remains correct. Touches PackageInstall.rs (+170/−30, new StagingPath, rename_staging_into_place, install() split into install() + install_into(), all backends now take an explicit dest_subpath), isolated_install/Installer.rs (+90/−60, staging for project-local store entries, commit_local_store_package, on_task_fail cleanup rewritten), a one-line comment in isolated_install.rs, and a new 237-line test file exercising both linkers.
Security risks
None identified. The staging directory is a dot-prefixed sibling inside the same node_modules (or .bun/<store>/node_modules) tree the package was already going to be written to; no new user-controlled path components are introduced (the hash is of the destination subpath). The rename is between siblings under a directory the installer already controls.
Level of scrutiny
High. This is the core file-materialisation step of bun install for both linkers — every package install now goes through an extra delete_tree and renameat. The change is architectural rather than a targeted bugfix, replaces a fire-and-forget cleanup with a hard failure in the isolated linker, and adds a Windows-only sleep-and-retry loop (up to 630ms) that is difficult to validate outside Windows CI. The append_real_store_path semantics are now asymmetric between Which::Final (whole-entry path via append_store_path) and Which::Staging (package-level path); I traced that the only callers reaching this with Npm/Git/Github/LocalTarball/RemoteTarball tags see sibling paths, but the asymmetry is subtle enough that a maintainer familiar with the isolated store layout should confirm it.
Other factors
- The kill-during-install test polls in a tight
Bun.sleep(0)loop and SIGKILLs on the first directory entry; it has a documented reason for not beingtest.concurrent, and the assertion structure (either absent or complete, then a repair install yields the complete tree with no leftover siblings) is sound.expect(stderr).not.toContain("error")is weaker than ideal but paired with an exit-code and full-tree check. - The occupied-destination retry in hoisted
install()callsuninstall_before_installa second time, which was previously conditional onskip_delete; the second call is unconditional, which is correct for the two-alias-workspace case the PR describes and is covered by a test. - CI so far shows one unrelated failure (
test-http-chunk-problem.js) and the new test file passes on both debug+ASAN and release per the PR evidence block. - The comment-cop bot's flags were addressed in e67d0ba and the remaining comments are one-to-three lines each documenting non-obvious constraints (NAME_MAX aliases, workspace double-walk, NTFS occupied vs locked); the author's justification for keeping them is reasonable.
…place Both linkers decide whether a package is already installed by looking at the directory it was installed into: the hoisted linker reads node_modules/<pkg>/package.json and the isolated linker checks that node_modules/.bun/<store>/node_modules/<pkg>/package.json exists. Both also built that directory in place, file by file, so an install killed while linking left a directory that the next install accepted as complete. Each backend now links into a dot-prefixed sibling (.<name>.bun-tmp, next to the final directory) which is renamed onto the final path once every file is in place, so the final path is either absent or complete. The staging name is deterministic; an install removes whatever a previous interrupted one left there before linking, and removes its own on failure. On Windows the rename is retried briefly, since a scanner holding a freshly written file open fails directory renames (the same failure #11250 reports for the cache publish). The isolated installer's cleanup on a failed task unlinked node_modules/<storepath>, a path that does not exist in the store layout; it now removes the staging directory instead.
Two cases from the full suite: An alias can already be 255 bytes long (bun-install-registry's long path test), so a staging name built by adding to it exceeds NAME_MAX. The staging directory is now .bun-tmp-<hash of the package path>, next to the package as before. A workspace that is depended on under a second name is linked into node_modules under both names, and the hoisted linker walks the packages nested inside it once per name (migration/complex-workspace). With a fresh node_modules nothing is moved away before installing, so the second walk's rename found the first walk's directory and failed with ENOTEMPTY. It now moves the occupant aside the way a reinstall does and renames again. The Windows retry helper returns at once when the destination exists, since NTFS reports that with the same errors as a transient lock.
e67d0ba to
e2740a7
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes the core install path for both linkers — every backend now writes to a staging sibling and renames into place, with new failure-cleanup and Windows retry logic — a human look would still be worthwhile before this ships in 1.4.
Checked: StagingPath hashing keeps names under NAME_MAX and stays a sibling of scoped packages; the hoisted install() wrapper cleans up the staging tree on both link failure and rename failure, and the occupied-destination fallback is guarded against .; the isolated 'backend loop's new break tail reaches commit_local_store_package from all three arms while the Folder/Root inline block still continues past it; append_store_package_path(.., Final) is byte-identical to the code it replaced in append_store_path; on_task_fail's dropped Folder arm was previously a no-op unlink on a directory path.
Extended reasoning...
Overview
This PR changes both install linkers to build each package in a dot-prefixed staging sibling (.bun-tmp-<wyhash>) and rename it onto the final path once complete, so an interrupted bun install never leaves a truncated directory that the next install's package.json check accepts as installed. It touches src/install/PackageInstall.rs (new StagingPath, rename_staging_into_place, Step::MovingIntoPlace; install() now wraps a new install_into() and every backend takes an explicit dest_subpath), src/install/isolated_install/Installer.rs (the LinkPackage step now clears both final and staging trees, all three backend arms break 'backend to a shared commit_local_store_package tail; on_task_fail rewritten to delete the staging tree; new append_store_package_path), a one-line comment in isolated_install.rs, and a new 237-line test file exercising interrupted installs, doubly-aliased workspaces, and 255-byte aliases across both linkers.
Security risks
None identified. The change is filesystem-local within node_modules and the isolated store; the staging name is a wyhash of the package path (no user-controlled component beyond the alias, which was already used verbatim as a directory name). No new external inputs are parsed and no trust boundaries are crossed.
Level of scrutiny
High. bun install is a production-critical path exercised by every user, and this PR changes its write strategy across every backend (clonefile, clonefile-each-dir, hardlink, copyfile, symlink) on every platform. The Windows retry loop in rename_staging_into_place (six attempts, 630ms total, gated on !directory_exists_at(dest)) is a heuristic a maintainer should sign off on. The occupied-destination fallback in the hoisted linker (a second uninstall_before_install when the rename fails) and the control-flow reshaping in the isolated step machine are both correct on my reading but subtle enough to deserve a second pair of eyes.
Other factors
The PR description is unusually thorough and the test file demonstrably fails on main and passes with the fix (the gate evidence shows both linkers leaving 37- and 1-entry truncated trees on main). CI is green except for test-http-chunk-problem.js, which the author confirms also fails on main and is unrelated. The comment-cop bot flagged several multi-line comments; the author shortened most and justified the remainder as recording non-obvious constraints (all threads resolved). Nothing outstanding blocks review, but the scope and criticality put this beyond what I'd approve without a human maintainer's sign-off.
2 participants
Problem
bun installkilled while it is linking a package out of the cache leaves a truncated package directory innode_modules, and every laterbun install(including--frozen-lockfile) reportsno changes, exit 0. Reproduced on main with both linkers (fuzz ledger entry 14691; also on 1.3.14, so not a regression, but it ships in 1.4).package.jsonas the "this package is installed" marker: hoistedPackageInstall::verifyreadsnode_modules/<pkg>/package.json(src/install/PackageInstall.rs,verify_package_json_name_and_version), isolated checks thatnode_modules/.bun/<store>/node_modules/<pkg>/package.jsonexists (src/install/isolated_install.rs, theneeds_installblock).init_install_dircreatesnode_modules/<pkg>and then walks the cache folder into it (same for the clonefile backends), isolatedHardlinker/FileCopier/FileClonerwrite straight to the store path.package.jsonis just one of the files in the walk, so a kill after it is linked produces a directory the next install trusts.node_modulesside.Fix
StagingPath(PackageInstall.rs): a package is linked into a dot-prefixed sibling of its final directory,.bun-tmp-<hash of the package path>(node_modules/.bun-tmp-3fa369cacb4f6ef8,node_modules/@types/.bun-tmp-..., and the same insidenode_modules/.bun/<store>/node_modules/), and the directory is renamed onto the final path once every file is in place. The final path is therefore either absent or complete, and the existingpackage.json/.bun-tagchecks become correct without changing them..<name>.bun-tmpbecause an alias can already be 255 bytes long (bun-install-registry's long path test); it is deterministic so the next install of the same package removes a stale one before linking (the backends overlay an existing tree, and a stale one may belong to another version), and a failed link removes its own. The leading dot keeps it out of package resolution andbun pmscans, which already skip dot entries..old-<hex>(the existing rename-aside inuninstall_before_install) is unchanged.install()computes the staging name, clears a stale one, runs the existing backend dispatch (nowinstall_into, with the destination passed explicitly instead of read fromself.destination_dir_subpath) against it, then renames. If the final path turns out to be occupied it is moved aside withuninstall_before_installand the rename repeated: with a freshnode_modulesnothing is moved aside up front, and a workspace that is depended on under a second name is linked intonode_modulesunder both names, so the hoisted linker walks the packages nested in it once per name and installs them twice into the same directory (migration/complex-workspacein CI; previously the second install silently overwrote the first file by file). NewStep::MovingIntoPlacenames the rename in the existingfailed <step> for packageerror.uninstall_before_installstill runs first whenever it did before, so failure behaviour is unchanged.append_real_store_path(.., Which::Staging)now yields the package-level staging directory for project-local entries (global-store entries keep their entry-level staging, which already covered this).Step::LinkPackageclears both the previous tree and a stale staging tree, links, andcommit_local_store_packagerenames; the three backendsbreakto that shared tail instead of advancing the step themselves. No occupied-destination handling is needed here because the final path is deleted in the same step.on_task_failnow removes the staging directory; the block it replaces unlinkednode_modules/<storepath>, a path that does not exist in the store layout, so it never removed anything.rename_staging_into_placeis shared by both linkers. On Windows it retriesEPERM/EACCES/EBUSYwith backoff (630ms total) unless the destination exists, because NTFS refuses to rename a directory while a scanner holds a freshly written file inside it open, and reports an occupied destination with the same errors (bun installfails on Windows: Operation not permitted (NtSetInformationFile()) #11250 is the scanner failure for the cache publish; install: retry cache publish renames on Windows while a scanner holds a file open #35568, if it lands, is the natural replacement for this loop). On POSIX it is a singlerenameat.folder:/root entries in the isolated linker are relinked on every install and never consult the marker, so they still link in place;install_from_linkcreates a single symlink, which is already atomic; the double walk of multiply-aliased workspaces is pre-existing and left alone.test/cli/install/bun-install-staging.test.ts, each case for both linkers:Bun.serveregistry, deletesnode_modules, runsbun installagain and SIGKILLs it the moment anything appears where the package goes, then asserts the final directory is absent or complete and that one morebun installyields the complete tree with nothing else (no leftover staging directory) next to it. Fails on main for both linkers (final directory present with a handful to a few hundred of its 2081 entries; release and debug builds, Linux and Windows); with this change the kill lands inside.bun-tmp-<hash>.ENOTEMPTYfor the hoisted linker on the first revision of this branch).isolated-install,isolated-relink,bun-install-patch,bun-patch,bun-install-hardlink-fallback(hardlink to copyfile fallback inside the staging dir),bun-install-git-deps,bun-prune,bun-workspaces,bun-install-registry's long path test,bun-installandbun-install-lifecycle-scripts(remaining failures in the last two need network access,nodeon PATH, or are the debug-build stack dump on install failure).cargo check -p bun_installfor the linux,aarch64-apple-darwinandx86_64-pc-windows-msvctargets.Background
PackageInstall): for each package in anode_modulestree,verify()decides whether it is installed; if not,install()picks a backend (clonefile on macOS, hardlink elsewhere, copyfile fallback, per-file symlinks forfile:folders) and materialises the cache folder undernode_modules/<alias>.uninstall_before_installrenames any existing directory to.old-<hex>and deletes it on the thread pool; it is skipped whennode_moduleswas just created.node_modulesdirectory per tree. A workspace package linked intonode_modulesunder two names yields two trees whose paths differ but resolve, through the workspace symlinks, to the samepackages/<ws>/node_modulesdirectory.node_modules/.bun/<name>@<version>[+peers]/node_modules/<name>;isolated_install.rsdecides per entry whether itneeds_install, andInstaller::Taskruns the stepsLinkPackage(files),SymlinkDependencies, binaries, scripts. Entries that are skipped still get a relink task for their symlinks, so the package directory is the only thing the marker has to vouch for.<cache>/links/<entry>.tmp-<suffix>and renamed bycommit_global_store_entry;Which::{Final, Staging}is the enum its path builders use, now also covering the project-local case.[review] gate passed · iteration 0 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file