Make the Windows hardlink install backend link serially - #33113
dylan-conway wants to merge 3 commits into
Conversation
…hread pool Synchronized CreateHardLinkW bursts from the per-package thread-pool fan-out get pended 20-180ms each by the AV minifilter, and the per-package barrier put every straggler on the critical path (~3.7ms per file; 6x slower than --backend copyfile on a 24-core machine with Defender on, scaling almost linearly with pool size). Link creation is kernel-serialized anyway, so the pool bought nothing even without the barrier. Link serially on the install thread like the unix branch: a 12-package / 516-file warm install drops from ~1.9s to ~0.37s (debug build A/B), with byte-identical output trees. Create destination directories from the walker's Directory entries (the walker yields a directory before its contents) so the first file in each directory no longer pays a doomed CreateHardLinkW through the filter, and so empty directories install like the unix and copyfile branches. Delete the now-unused HardLinkWindowsInstallTask, NewTaskQueue, HasWorkPoolTask, and HARDLINK_QUEUE machinery; drop the dead FailedToCopyFile arm (no producer exists); fix the NameTooLong guard off-by-one shared by the hardlink/copyfile/symlink branches (a path exactly filling the buffer tail panicked on the NUL write instead of returning NameTooLong). Error semantics now match unix: the first failing file aborts the package instead of attempting every remaining file and reporting the first error after the barrier.
|
Updated 8:32 PM PT - Jun 29th, 2026
❌ @robobun, your commit 4583418 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 33113That installs a local version of the PR into your bun-33113 --bun |
|
Found 4 issues this PR may fix:
🤖 Generated with Claude Code |
|
Warning Review limit reached
Next review available in: 29 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 (2)
WalkthroughWindows package hardlink installation is rewritten from a thread-pool task-queue model to a synchronous inline walker. A new ChangesWindows Hardlink Install Rewrite + Tests
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: 3
🤖 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/PackageInstall.rs`:
- Around line 1518-1523: The directory-handling branch in
PackageInstall::hardlink_file_windows is swallowing mkdir_w failures by
assigning the result to a discarded variable, which can hide empty-directory
creation errors. Update the is_dir path to check and propagate the sys::mkdir_w
result instead of treating it as best-effort, so the install fails when an empty
directory cannot be created and the package tree is preserved correctly.
In `@test/cli/install/install-backends.test.ts`:
- Around line 14-20: The install-backends fixture only covers file entries, so
the new Directory-walker path is not validated; update the PKG_FILES fixture in
install-backends.test.ts to include an actual empty directory entry and extend
the install assertion to verify that empty directory is materialized. Use the
existing tarball/fixture setup in the install-backends test to locate the
relevant package entries and keep the coverage tied to the backend install
behavior. This should exercise the empty-directory case directly so the
regression is covered by the same test suite.
- Around line 60-86: In the install backend test, assert the result of
install([]) and install(["--force"]) before any filesystem reads or stat calls,
so a failed bun install is reported directly instead of being masked by ENOENT
from Bun.file(...).text() or statSync(). Update the test in
test/cli/install/install-backends.test.ts by checking the first and second
install results immediately after each call, using the existing install() return
value and existing exitCode assertions, before touching pkgDir or any
node_modules paths.
🪄 Autofix (Beta)
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: 05cb5fd9-d8b3-4003-9dd4-bf9742e611a2
📒 Files selected for processing (3)
src/install/PackageInstall.rssrc/install/npm.rstest/cli/install/install-backends.test.ts
| if is_dir { | ||
| // The walker yields a directory before its contents, so each | ||
| // file's first CreateHardLinkW finds its parent (mirrors the | ||
| // unix arm's make_path; failures surface on the file ladder). | ||
| let _ = sys::mkdir_w(bun_core::WStr::from_buf(head1, dest_len)); | ||
| continue; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don't ignore directory-creation failures for empty directory entries.
Line 1522 drops the sys::mkdir_w result. If an empty directory cannot be created, this walker can still return success because there is no later file entry to trip hardlink_file_windows, so the install silently omits part of the package tree. Bubble the error here instead of treating it as best-effort. As per coding guidelines, "Never swallow a failure or signal success on one." Based on PR objectives, this path is supposed to preserve empty directories.
🤖 Prompt for 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.
In `@src/install/PackageInstall.rs` around lines 1518 - 1523, The
directory-handling branch in PackageInstall::hardlink_file_windows is swallowing
mkdir_w failures by assigning the result to a discarded variable, which can hide
empty-directory creation errors. Update the is_dir path to check and propagate
the sys::mkdir_w result instead of treating it as best-effort, so the install
fails when an empty directory cannot be created and the package tree is
preserved correctly.
Source: Coding guidelines
| const PKG_FILES: Record<string, string> = { | ||
| "package/package.json": JSON.stringify({ name: "backend-pkg", version: "1.0.0", main: "index.js" }), | ||
| "package/index.js": `module.exports = require("./lib/a.js") + require("./lib/deep/b.js");\n`, | ||
| "package/lib/a.js": `module.exports = "a".repeat(64);\n`, | ||
| "package/lib/deep/b.js": `module.exports = "b".repeat(64);\n`, | ||
| "package/README.md": `# backend-pkg\n`, | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add a real empty-directory case to this fixture.
This tarball only contains files, so the new walker path for Directory entries is never exercised. The nested lib/deep files cover “first file in a directory”, but the PR’s empty-directory materialization fix can still regress without this suite noticing. Please add an empty directory entry and assert it exists after install. Based on learnings, every behavioral change should ship with automated coverage in the same PR.
🤖 Prompt for 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.
In `@test/cli/install/install-backends.test.ts` around lines 14 - 20, The
install-backends fixture only covers file entries, so the new Directory-walker
path is not validated; update the PKG_FILES fixture in install-backends.test.ts
to include an actual empty directory entry and extend the install assertion to
verify that empty directory is materialized. Use the existing tarball/fixture
setup in the install-backends test to locate the relevant package entries and
keep the coverage tied to the backend install behavior. This should exercise the
empty-directory case directly so the regression is covered by the same test
suite.
Source: Coding guidelines
| const first = await install([]); | ||
| const pkgDir = join(String(projDir), "node_modules", "backend-pkg"); | ||
|
|
||
| // Every file materialized with identical contents. | ||
| for (const [archivePath, contents] of Object.entries(PKG_FILES)) { | ||
| const rel = archivePath.replace("package/", ""); | ||
| expect(await Bun.file(join(pkgDir, rel)).text()).toBe(contents); | ||
| } | ||
|
|
||
| // hardlink must share the inode with the cache copy; copy-based | ||
| // backends must not. | ||
| const nlink = statSync(join(pkgDir, "lib", "deep", "b.js")).nlink; | ||
| if (backend === "hardlink") { | ||
| expect(nlink).toBeGreaterThan(1); | ||
| } else { | ||
| expect(nlink).toBe(1); | ||
| } | ||
|
|
||
| expect(first.exitCode).toBe(0); | ||
|
|
||
| // A forced reinstall must re-materialize deleted files through the same | ||
| // backend. (Deletion, not tampering: with hardlink the node_modules name | ||
| // shares the cache inode, so writes through it would corrupt the cache.) | ||
| rmSync(join(pkgDir, "lib", "a.js")); | ||
| const second = await install(["--force"]); | ||
| expect(await Bun.file(join(pkgDir, "lib", "a.js")).text()).toBe(PKG_FILES["package/lib/a.js"]); | ||
| expect(second.exitCode).toBe(0); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the install result before touching node_modules.
If either install fails here, the later Bun.file(...).text() / statSync() calls can throw ENOENT and hide the real bun install failure. In test/cli/install, guard the install result before any filesystem assertions for both the first install and the --force reinstall.
Suggested change
const first = await install([]);
const pkgDir = join(String(projDir), "node_modules", "backend-pkg");
+ expect({ stdout: first.stdout, exitCode: first.exitCode }).toMatchObject({ exitCode: 0 });
// Every file materialized with identical contents.
for (const [archivePath, contents] of Object.entries(PKG_FILES)) {
const rel = archivePath.replace("package/", "");
expect(await Bun.file(join(pkgDir, rel)).text()).toBe(contents);
@@
- expect(first.exitCode).toBe(0);
-
// A forced reinstall must re-materialize deleted files through the same
// backend. (Deletion, not tampering: with hardlink the node_modules name
// shares the cache inode, so writes through it would corrupt the cache.)
rmSync(join(pkgDir, "lib", "a.js"));
const second = await install(["--force"]);
+ expect({ stdout: second.stdout, exitCode: second.exitCode }).toMatchObject({ exitCode: 0 });
expect(await Bun.file(join(pkgDir, "lib", "a.js")).text()).toBe(PKG_FILES["package/lib/a.js"]);
- expect(second.exitCode).toBe(0);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const first = await install([]); | |
| const pkgDir = join(String(projDir), "node_modules", "backend-pkg"); | |
| // Every file materialized with identical contents. | |
| for (const [archivePath, contents] of Object.entries(PKG_FILES)) { | |
| const rel = archivePath.replace("package/", ""); | |
| expect(await Bun.file(join(pkgDir, rel)).text()).toBe(contents); | |
| } | |
| // hardlink must share the inode with the cache copy; copy-based | |
| // backends must not. | |
| const nlink = statSync(join(pkgDir, "lib", "deep", "b.js")).nlink; | |
| if (backend === "hardlink") { | |
| expect(nlink).toBeGreaterThan(1); | |
| } else { | |
| expect(nlink).toBe(1); | |
| } | |
| expect(first.exitCode).toBe(0); | |
| // A forced reinstall must re-materialize deleted files through the same | |
| // backend. (Deletion, not tampering: with hardlink the node_modules name | |
| // shares the cache inode, so writes through it would corrupt the cache.) | |
| rmSync(join(pkgDir, "lib", "a.js")); | |
| const second = await install(["--force"]); | |
| expect(await Bun.file(join(pkgDir, "lib", "a.js")).text()).toBe(PKG_FILES["package/lib/a.js"]); | |
| expect(second.exitCode).toBe(0); | |
| const first = await install([]); | |
| const pkgDir = join(String(projDir), "node_modules", "backend-pkg"); | |
| expect({ stdout: first.stdout, exitCode: first.exitCode }).toMatchObject({ exitCode: 0 }); | |
| // Every file materialized with identical contents. | |
| for (const [archivePath, contents] of Object.entries(PKG_FILES)) { | |
| const rel = archivePath.replace("package/", ""); | |
| expect(await Bun.file(join(pkgDir, rel)).text()).toBe(contents); | |
| } | |
| // hardlink must share the inode with the cache copy; copy-based | |
| // backends must not. | |
| const nlink = statSync(join(pkgDir, "lib", "deep", "b.js")).nlink; | |
| if (backend === "hardlink") { | |
| expect(nlink).toBeGreaterThan(1); | |
| } else { | |
| expect(nlink).toBe(1); | |
| } | |
| // A forced reinstall must re-materialize deleted files through the same | |
| // backend. (Deletion, not tampering: with hardlink the node_modules name | |
| // shares the cache inode, so writes through it would corrupt the cache.) | |
| rmSync(join(pkgDir, "lib", "a.js")); | |
| const second = await install(["--force"]); | |
| expect({ stdout: second.stdout, exitCode: second.exitCode }).toMatchObject({ exitCode: 0 }); | |
| expect(await Bun.file(join(pkgDir, "lib", "a.js")).text()).toBe(PKG_FILES["package/lib/a.js"]); |
🤖 Prompt for 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.
In `@test/cli/install/install-backends.test.ts` around lines 60 - 86, In the
install backend test, assert the result of install([]) and install(["--force"])
before any filesystem reads or stat calls, so a failed bun install is reported
directly instead of being masked by ENOENT from Bun.file(...).text() or
statSync(). Update the test in test/cli/install/install-backends.test.ts by
checking the first and second install results immediately after each call, using
the existing install() return value and existing exitCode assertions, before
touching pkgDir or any node_modules paths.
Source: Learnings
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this rewrites the default Windows install backend (removing the thread-pool/barrier machinery, changing error-abort semantics, and adding directory-entry handling), and CI showed build-rust failures on the pre-autofix commit — worth a human look before merge.
Extended reasoning...
Overview
This PR rewrites the Windows hardlink install backend in src/install/PackageInstall.rs from a thread-pool fan-out (HardLinkWindowsInstallTask / NewTaskQueue / HARDLINK_QUEUE / WaitGroup) to a serial inline loop on the install thread, mirroring the unix linkat path. It deletes ~200 lines of concurrency infrastructure, extracts a hardlink_file_windows helper, adds directory-entry handling in the walker (so parent dirs are created before their files), fixes a > → >= off-by-one in three NUL-terminator bounds guards (hardlink/copyfile/symlink), removes a dead FailedToCopyFile arm, tightens cfg-gated imports, updates a stale comment in npm.rs, and adds a new cross-backend correctness test.
Security risks
None identified. The change is confined to local filesystem operations (CreateHardLinkW/CopyFileW/mkdir) on paths already derived from the package cache and destination dir. No auth, crypto, network, or untrusted-input parsing is touched.
Level of scrutiny
High. This is the default install backend on Windows — every bun install on Windows runs this code path. The PR changes observable behavior in two ways: (1) error semantics now abort on the first failing file instead of attempting all files and reporting the first error after the barrier, and (2) empty directories are now materialized from walker Directory entries. Both are reasonable and well-argued in the description, but they are behavioral changes to a hot, platform-specific path that I cannot verify locally (Windows-only), and they could affect edge cases like partially-failed installs or packages with unusual directory layouts.
Other factors
- CI: robobun reported build-rust failures across 9 non-Windows targets on commit 93307ca; an autofix.ci commit (29786eb) followed, but the timeline doesn't yet show a green build for it.
- Outstanding comments: coderabbitai left three open comments (best-effort
mkdir_wfor empty dirs, missing empty-directory test coverage, and test exitCode assertion ordering). Thelet _ = mkdir_wis intentional per the inline comment ("failures surface on the file ladder") and matches the unix arm'slet _ = make_path, but the author hasn't responded yet. - Test coverage: The new
install-backends.test.tsis a good correctness check (per-backend tree contents +nlinkverification +--forcereinstall), though it doesn't exercise the Windows-specific code path on non-Windows CI. - The PR description is exceptionally thorough with benchmarks and rationale, and the bug hunter found nothing — but the scope (rewriting a default install backend, removing concurrency primitives, changing error semantics) is well beyond a mechanical change.
|
oh i bet this makes it slower when anti-virus is not in use |
Nothing produces this error, and the arm was a no-op: the unconditional return below it builds the identical InstallResult::fail.
|
I built and tested this on Windows and independently verified both correctness and the performance claim. Summary: the conclusion holds and the change is the right call, but one sentence in the justification is narrower than stated, so I measured the regime it doesn't cover. Correctness
The performance claim without an AV filterEvery number in the description is from a machine with Defender's real-time protection on. The one sentence that generalizes past that is:
I ran the same A/B on a 16-vCPU Windows Server 2019 VM with a local NVMe SSD and no Defender at all (the Methodology: both binaries are debug builds from the same toolchain and worktree, differing only in
(A second pass of the first fixture on a noisier stretch of the VM landed at 808ms vs 1500ms, same direction, wider spread.) So without an AV filter the result is shape-dependent, and it tracks the description's own analysis of where the per-package barrier hurts:
I think the debug-build numbers are, if anything, conservative for the rows where ConclusionThe change still looks right to land:
And the I also pushed 4583418: the description says the dead |
This branch has not been deployed
3 participants
Summary
bun installon Windows (default--backend hardlink) materialized node_modules ~6x slower than--backend copyfile— about 3.7ms per file on a 24-core Windows 11 machine with Defender's real-time protection on — even though a singleCreateHardLinkWcosts the same ~0.5ms as aCopyFileWon the same volume.The cause is not the syscall and not extra per-file work (the hardlink path issues fewer syscalls than copyfile). The Windows-only code fanned each package's per-file links out to the full
cpu_countthread pool and blocked on a per-package barrier. Every barrier resynchronizes the pool, so each package starts with a synchronized burst of ~24 concurrentCreateHardLinkWcalls. Those bursts trigger pathological antivirus filter work (MsMpEng burned 20+ CPU-seconds during a 1.7s install; the same links unbatched cost it ~3s, mostly asynchronous), which pends a few links per package for 20–180ms — and the barrier puts every straggler on the critical path. Install time scales almost linearly with pool size: 425ms at 2 threads, 1914ms at 24, for a 12-package / 516-file warm install. Link creation is kernel-serialized anyway, so the pool bought nothing even without the barrier (steady-state parallel linking measures 0.7ms/op vs 0.55ms/op serial).This change makes the Windows branch link inline and serially on the install thread, exactly like the unix branch's
linkatloop:Directoryentries (the walker yields a directory before its contents), so the first file in each directory no longer pays a guaranteed-failedCreateHardLinkWthrough the AV filter, and empty directories install like the unix and copyfile branches.HardLinkWindowsInstallTask/NewTaskQueue/HasWorkPoolTask/HARDLINK_QUEUEmachinery is deleted, along with the deadFailedToCopyFileerror arm (nothing produces that error).NameTooLongguard and panicked on the NUL terminator write instead of returning an error.Test plan
test/cli/install/install-backends.test.ts: per-backend (hardlink/copyfile, +clonefile on macOS) tree-correctness from a local tarball,nlink > 1proves the hardlink backend actually links (andnlink == 1for copy backends), and a--forcereinstall re-materializes deleted files. Passes on the debug build and on the released bun.test/cli/install/bun-link.test.ts,bun-add.test.ts,bun-install-pathname-trailing-slash.test.tson the patched Windows debug build (the one bun-add failure is a pre-existing environment issue: its fixture shells out to baretar, which fails under GNU tar when Git'susr/binprecedes System32 inPATH; it fails identically without this change).cargo checkacross all 10 target/platform combinations.