install(bin): support cross-volume Windows global installs - #30154
install(bin): support cross-volume Windows global installs#30154robobun wants to merge 5 commits into
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughWindows bin shims now support absolute/cross-volume targets via a new Cross-Volume Windows Bin Shim Support
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Review rate limit: 0/5 reviews remaining, refill in 6 minutes and 12 seconds. Comment |
|
Updated 11:05 PM PT - May 4th, 2026
❌ @robobun, your commit 4551282 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 30154That installs a local version of the PR into your bun-30154 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/install/windows-shim/BinLinkingShim.zig (1)
33-52:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAvoid publishing v6 metadata before the matching launcher is guaranteed to land.
Line 34 bumps the shim format to
v6, and Lines 56-87 change the footer layout tou32, butsrc/install/bin.zigstill truncates/writes.bunxfirst and then ignoreserror.EBUSYif rewriting the sibling.exefails. That can leave an old v5 launcher next to a v6.bunx, after which direct launches of that global bin will fail until something rewrites the.exe. Please either make the update transactional or keep the new launcher backward-compatible before publishing v6 metadata.Also applies to: 56-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/install/windows-shim/BinLinkingShim.zig` around lines 33 - 52, VersionFlag was incremented to v6 and footer layout changed to u32 while bin-writing logic still writes the .bunx first and swallows error.EBUSY when updating the sibling .exe, which can leave mismatched v5/.bunx v6 pairs; either make the install update transactional or keep v6 metadata out until launchers exist. Fix by updating the writer in src/install/bin.zig: implement atomic replacement (write both .bunx and .exe to temp files, fsync, then rename into place) or change order to create/replace the .exe first and only replace the .bunx if the .exe update succeeds, and ensure error.EBUSY on the second write triggers rollback of the first (delete temp or restore previous file) so no mismatched pair remains; alternatively revert the VersionFlag bump (remove v6 current) or retain backward-compatible footer parsing until the new launcher is published. Ensure references: VersionFlag (v6), footer u32 change, and the bin-writing logic in src/install/bin.zig are updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/install/windows-shim/bun_shim_impl.zig`:
- Around line 590-605: The bounds check uses buf1.len (count of u16s) instead of
byte capacity, causing false rejections when relocating metadata in
flags.is_absolute_target; update the comparison to use byte length (buf1_u8.len)
or equivalently buf1.len * `@sizeOf`(u16)) so the condition becomes if (read_len >
buf1_u8.len - new_offset) before returning mode.fail(.InvalidShimDataSize) —
adjust the check around new_offset/read_len in the relocation block to compare
bytes, not u16 count.
---
Outside diff comments:
In `@src/install/windows-shim/BinLinkingShim.zig`:
- Around line 33-52: VersionFlag was incremented to v6 and footer layout changed
to u32 while bin-writing logic still writes the .bunx first and swallows
error.EBUSY when updating the sibling .exe, which can leave mismatched v5/.bunx
v6 pairs; either make the install update transactional or keep v6 metadata out
until launchers exist. Fix by updating the writer in src/install/bin.zig:
implement atomic replacement (write both .bunx and .exe to temp files, fsync,
then rename into place) or change order to create/replace the .exe first and
only replace the .bunx if the .exe update succeeds, and ensure error.EBUSY on
the second write triggers rollback of the first (delete temp or restore previous
file) so no mismatched pair remains; alternatively revert the VersionFlag bump
(remove v6 current) or retain backward-compatible footer parsing until the new
launcher is published. Ensure references: VersionFlag (v6), footer u32 change,
and the bin-writing logic in src/install/bin.zig are updated accordingly.
🪄 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: ade1b510-0550-4e3b-a194-d305974e3827
📒 Files selected for processing (4)
src/install/bin.zigsrc/install/windows-shim/BinLinkingShim.zigsrc/install/windows-shim/bun_shim_impl.zigtest/regression/issue/30129.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/30129.test.ts`:
- Around line 44-53: The test currently spawns Bun (proc via Bun.spawn with cmd
[bunExe(), "install"]) and collects outputs but discards stderr; update the
assertion after awaiting Promise.all([proc.stdout.text(), proc.stderr.text(),
proc.exited]) to assert exitCode === 0 and that the captured stderr string does
not include the substring "error:" (use the variable bound to proc.stderr.text()
— e.g., stderr) so install regressions fail if Bun emitted its error marker;
apply the same change to the similar occurrence at the other location referenced
(line ~93).
- Around line 73-88: Add a new Windows-only test that directly exercises the
is_absolute_target path by constructing a .bunx shim whose rel_target_from_bin
is an absolute path and verifying the shim decoding logic in bun_shim_impl.zig
yields that absolute path; specifically, on Windows create a .bunx file
containing the UTF-16LE-encoded shim header with an absolute target (e.g.,
"C:\\path\\to\\foo.js"), then read it with the same
TextDecoder("utf-16le")/terminator logic used in the existing test and assert
the decoded slice equals the absolute path string so the is_absolute_target
branch is covered.
🪄 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: cfdc5e45-ac7b-4161-a86f-16fb279651f5
📒 Files selected for processing (3)
src/install/bin.zigsrc/install/windows-shim/bun_shim_impl.zigtest/regression/issue/30129.test.ts
|
CI x64-asan test-bun shard failed on a known pre-existing flake in Confirmed as a pre-existing flake: build #50489 on an unrelated branch ( The rest of the test-bun shards are still in progress (72/287 jobs completed, 213 waiting). I'll let them run — requesting a retry on the known-flaky asan job. |
|
Update: 278/287 test-bun shards now passed. Two flakes remain, both pre-existing on unrelated test files:
Worth flagging: the Windows shard that did run my test passed it cleanly — Neither flake touches files this PR modifies ( |
|
Final CI state for build 50504 (no more retries remaining):
No action this PR can take — both genuine failures are in tests for HTTP/2 fetch + bake dev server HMR, on ASAN and Windows specifically. Would appreciate a maintainer retrying the two flaky test-bun jobs (the retry button for |
Fixes #30129 On Windows, bun install -g panicked at install/bin.zig:738 with 'Internal assertion failure' when BUN_INSTALL_BIN lived on a different physical drive from the global package store. The assertion checked that path.relative from the shim's .bin directory to the target binary starts with '..\\' — Windows cannot produce such a walk between two volumes, so relativeBufZ fell through to returning the absolute target and the assertion fired. The .bunx file had already been opened with O_TRUNC, so a corrupted empty .bunx was left behind. This is the supported split-storage case called out in Microsoft's Dev Drive docs: package caches and repositories on a non-system Dev Drive, launcher/tool shims on PATH on a separate drive. Fix: anchor the stored bin_path to the parent of the .bin directory — matching where bun_shim_impl.exe's walk-back loop actually lands at runtime — instead of the .bin directory itself. That handles two shapes naturally without stripping a '..\\' prefix: - common case: relative path is 'some-pkg\\dist\\cli.js' (no '..') - zero-'..' case: target lives inside .bin, relative is '.bin\\foo.js' For truly cross-volume setups, relativeBufZ returns the absolute target. Encode it directly and set a new is_absolute_target flag in the shim header (VersionFlag bumped to v6, Flags packed struct grew from u16 to u32 to make room). The launcher relocates the metadata to start at buf1_u8[2 * nt_object_prefix.len] when the flag is set, so the subsequent decode path sees an absolute path at the same offset the relative case would have produced — no further branching needed in the command-line construction. Also drop the equivalent non-Windows assertion in createSymlink: symlink(2) is perfectly happy with any relative string, including a bare filename when the target resolves inside abs_dest_dir itself. Verification: test/regression/issue/30129.test.ts installs a local package whose bin resolves into the sibling .bin directory, producing the same zero-'..' shape that fires the assertion on both POSIX (createSymlink) and Windows (createWindowsShim). Without the fix, bun install panics during bin linking and exits non-zero. The Windows branch additionally decodes the .bunx header and asserts bin_path equals '.bin\\foo.js'.
claude[bot] caught a regression: when BUN_INSTALL_BIN is one level
below a drive root (e.g. E:\bin), path.dirname(path.dirname("E:\bin"))
returns "E:" without a trailing \, which isAbsoluteWindows treats
as drive-relative. path.relative then silently resolves against
top_level_dir instead of E:\, encoding the wrong path. The bug's
repro (E:\Packages\bun\bin) hides this because it's 3 levels deep.
Switch the anchor computation so it doesn't depend on two-level
dirname: compute rel_target relative to <bin_dir> (single dirname,
always absolute), then shape based on the result:
- Starts with '..\\': common case — strip the single '..\\' (the
launcher walks back one level past <bin_dir>).
- Absolute: cross-volume — flag as is_absolute_target.
- Otherwise: zero-'..' shape (target inside <bin_dir> itself) —
prepend basename(<bin_dir>) + '\\' so the launcher's
post-walk-back concatenation lands back inside the bin directory.
This handles E:\bin, C:\Users\foo\bin, cross-drive, and the
inside-'.bin' zero-'..' case all through the same code path.
Also address three review nits in the same push:
- createSymlink doc comment: use forward-slash example (this function
is POSIX-only; the previous '..\\..\\package\\bin.js' example was
misleading).
- bun_shim_impl bounds check: clarify the byte-vs-u16 equivalence
with an explicit buf1_u8_len alias, per coderabbit.
- Test: drop the expect(stderr).not.toContain("Internal assertion
failure") line. assertWithLocation compiles out in release, so
the check is tautological in CI release lanes; the exitCode and
lockfile-existence assertions already gate the regression
equivalently (and catch it in debug builds too).
coderabbit suggestion. Distinct from the debug-only 'Internal assertion failure' check I removed earlier — 'error:' is Bun's user-facing error marker that appears in both debug and release lanes. Without the fix, if assertions are compiled out (release), bin-linking can still fail and surface an 'error:' line even though the process doesn't panic.
b83cee5 to
1fe22e8
Compare
… jobs across darwin, windows, alpine, freebsd)
There was a problem hiding this comment.
I didn't find any new issues and all prior feedback is resolved, but this bumps the .bunx shim format (v5→v6, Flags u16→u32) and adds buffer-relocation pointer arithmetic in bun_shim_impl.zig whose is_absolute_target branch can't be exercised in CI — worth a human pass on the launcher changes and format-bump compat before merging.
Extended reasoning...
Overview
Touches four files: src/install/bin.zig (rewrites createWindowsShim's bin-path computation into a three-way shape dispatch and drops the POSIX createSymlink assertion), src/install/windows-shim/BinLinkingShim.zig (format bump v5→v6, Flags packed struct widened u16→u32, new is_absolute_target field), src/install/windows-shim/bun_shim_impl.zig (new metadata-relocation branch with copyForwards over overlapping ranges, re-anchored read_ptr, recomputed validation_length_offset), and a new regression test.
Security risks
None identified. Inputs to the changed code are filesystem paths derived from the install layout, not attacker-controlled network/user data. The shim launcher already validates flags/lengths before acting; the new branch adds a defensive bounds check before the relocation copy.
Level of scrutiny
High. This is a binary-format version bump for the Windows .bunx shim plus hand-tuned pointer arithmetic in a launcher that runs for every globally-installed bin on Windows. The Flags size change means the new launcher reads 4 trailing bytes instead of 2, so any pre-existing v5 .bunx decodes as invalid under the new code (handled by the existing isValid() fallback in non-standalone mode, but the standalone .exe is co-written so should stay paired). The author and reviewers explicitly agree the is_absolute_target relocation path in bun_shim_impl.zig cannot be exercised on current CI without a real multi-drive topology. The file header also directs questions to a specific maintainer.
Other factors
All seven prior inline review threads (mine and CodeRabbit's) are resolved; the drive-root E:\\bin regression I flagged was fixed by switching to single-dirname + shape dispatch. CI is green on the shards that ran the new test (Linux POSIX branch and Windows .bunx-decode branch both passed); remaining failures are documented pre-existing flakes in unrelated HTTP/2 and bake HMR tests. The change is well-reasoned and likely correct, but the combination of format bump + untested launcher branch + intricate buffer-offset logic is beyond what I'm comfortable auto-approving.
|
Update on the retrigger build (51396):
Re: @claude[bot]'s heads-up on the launcher branch being exercisable only via real multi-drive topology — agreed and flagged in a prior thread. The |
|
Closing: this PR's implementation lives entirely in Zig source files that have since been removed from the tree as part of the Rust migration. The change can no longer merge cleanly and the files it edits no longer exist on If the underlying issue is still present, it will need a fresh fix against the Rust implementation. |
1 participant
Fixes #30129
Repro
On Windows, with the global bin directory on a different physical drive than the global package store:
Panics with:
…leaving a truncated empty
.bunxbehind. Microsoft's Dev Drive docs specifically call out this layout — package caches on a Dev Drive, launcher shims on PATH elsewhere.Cause
createWindowsShimcomputes a relative path from the shim's.bindirectory to the target binary and asserts it starts with..\:Windows cannot express a relative path between two absolute paths on different volumes, so
relativeBufZfalls through to returning the absolute target (C:\Users\…\cli.js),hasPrefixComptimeis false, and the assertion fires. The.bunxfile has already been opened withO_TRUNC, so an empty file is left in the bin directory.The same assertion family also fires on POSIX (
createSymlink) and on Windows for the zero-..shape — a package whosebinfield resolves inside the sibling.bindirectory.Fix
Anchor the stored
bin_pathto the parent of the.bindirectory — which is wherebun_shim_impl.exe's walk-back loop actually lands at runtime — instead of to.binitself. Two shapes then Just Work without stripping a..\prefix:some-pkg\\dist\\cli.js(no..)..case:.bin\\foo.js(target lives inside the bin dir)For truly cross-volume setups,
relativeBufZreturns the absolute target. Encode it directly and set a newis_absolute_targetflag in the shim header:VersionFlagbumped tov6Flagspacked struct grew fromu16tou32to make roombuf1_u8[2 * nt_object_prefix.len]when the flag is set, so the subsequent decode path sees an absolute path at the same offset the relative case would have produced — no further branching in the command-line construction.Also drop the equivalent POSIX assertion in
createSymlink:symlink(2)is happy with any relative string.Verification
test/regression/issue/30129.test.tsinstalls a local package whosebinresolves into the sibling.bindirectory, producing the same zero-..shape that fires the assertion on both POSIX (createSymlink) and Windows (createWindowsShim). Runs on Linux and Windows CI:bun installpanics during bin linking and exits non-zero beforenode_modulesartefacts land on disk.foo.json POSIX,.exe+.bunxpair on Windows). The Windows branch additionally decodes the.bunxheader and assertsbin_pathequals.bin\\foo.js— the parent-of-.bin-anchored form — so a regression that stored the wrong relative form would be caught.Gate proof (Linux,
bun bd):Related
anchor-at-parent-of-
.binapproach + absolute-target fallback supersedesboth.