fix(install): resolve absolute paths for Windows CopyFileW in FileCopier#28136
fix(install): resolve absolute paths for Windows CopyFileW in FileCopier#28136
Conversation
On Windows, FileCopier passed relative dest_subpath to CopyFileW and CreateDirectoryExW, which require absolute paths to handle long paths correctly. This caused ENOENT errors for packages with long scoped names like @emotion/use-insertion-effect-with-fallbacks. Resolve the CWD to an absolute path once before the copy loop, then join it with dest_subpath for each entry and add the NT path prefix, matching the pattern used by Hardlinker.zig. Closes #28133 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Updated 2:53 AM PT - Mar 15th, 2026
❌ Your commit 🧪 To try this PR locally: bunx bun-pr 28136That installs a local version of the PR into your bun-28136 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughUpdates FileCopier to compute and use absolute Windows NT-style destination paths for directory creation and file copying, reworking walker and non-walker copy paths to avoid ENOENT on scoped or long-path packages. Adds a regression test exercising the copyfile backend with scoped and nested packages. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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: 1
🤖 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/isolated_install/FileCopier.zig`:
- Around line 101-112: The per-iteration buffer pool get/put calls for
dest_abs_buf and dest_abs_buf2 should be hoisted outside the loop to avoid
repeated allocation overhead: allocate both buffers once before the loop using
bun.w_path_buffer_pool.get(), reuse them inside the loop when calling
bun.path.joinStringBufWZ and bun.strings.addNTPathPrefixIfNeeded with
this.dest_subpath, and then put them back to the pool after the loop (ensure the
defer or explicit put is adjusted so it runs once after the loop completes);
keep the same buffers' lifetimes consistent with their use so no concurrent
iterations overwrite needed data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 80f51a08-263a-4f8a-a49e-87e693364319
📒 Files selected for processing (2)
src/install/isolated_install/FileCopier.zigtest/regression/issue/28133.test.ts
Move dest_abs_buf and dest_abs_buf2 pool get/put outside the while loop to avoid repeated allocation overhead per iteration. The buffers are overwritten each iteration with no overlapping lifetimes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward fix that mirrors the existing Hardlinker.zig pattern for Windows absolute path resolution with NT prefix.
Extended reasoning...
Overview
This PR fixes a Windows-specific ENOENT error in FileCopier.zig during bun install with the copyfile backend. The fix resolves the CWD to an absolute path once before the copy loop, then joins it with dest_subpath for each entry and adds the \\?\ NT path prefix. This is the exact same pattern already used in Hardlinker.zig (lines 49-78). The POSIX code path is unaffected and was correctly left unchanged (it uses fd-relative operations). A regression test is included.
Security risks
No security risks. The change only affects path resolution mechanics on Windows — converting relative paths to absolute paths with NT prefix. No user input handling, authentication, or permissions logic is modified.
Level of scrutiny
Low-to-medium scrutiny is appropriate. While this touches the package installer (a somewhat critical path), the change is mechanical and follows a well-established pattern already present in the same directory (Hardlinker.zig). The buffer pool usage (get/put with defer inside the loop) is identical to how Hardlinker.zig handles it. The code restructuring that separates Windows and POSIX branches into distinct if/else blocks actually makes the code clearer.
Other factors
- The only bug found by the bug hunting system is pre-existing (the silent error swallowing when
dirnamereturns null on Windows) and was not introduced by this PR. - The regression test (
28133.test.ts) exercises the copyfile backend with scoped packages and nested directory structures. - The PR description clearly explains the root cause (MAX_PATH limitation without NT prefix) and links to the upstream issue.
| switch (entry.kind) { | ||
| .directory => { | ||
| if (bun.windows.CreateDirectoryExW(this.src_path.sliceZ(), this.dest_subpath.sliceZ(), null) == 0) { | ||
| if (bun.windows.CreateDirectoryExW(this.src_path.sliceZ(), dest_abs, null) == 0) { | ||
| bun.MakePath.makePath(u16, dest_dir, entry.path) catch {}; | ||
| } | ||
| }, | ||
| .file => { | ||
| bun.copyFile(this.src_path.sliceZ(), this.dest_subpath.sliceZ()).unwrap() catch { | ||
| bun.copyFile(this.src_path.sliceZ(), dest_abs).unwrap() catch { | ||
| if (bun.Dirname.dirname(u16, entry.path)) |entry_dirname| { | ||
| bun.MakePath.makePath(u16, dest_dir, entry_dirname) catch {}; | ||
| switch (bun.copyFile(this.src_path.sliceZ(), this.dest_subpath.sliceZ())) { | ||
| switch (bun.copyFile(this.src_path.sliceZ(), dest_abs)) { | ||
| .result => {}, | ||
| .err => |err| { |
There was a problem hiding this comment.
🟣 Pre-existing issue: On Windows, when bun.copyFile fails for a root-level file (no parent directory component), bun.Dirname.dirname(u16, entry.path) returns null and the catch block silently swallows the error — the file is not copied and no error is reported. The POSIX branch (line 155-164) correctly handles this by placing the retry outside the if (dirname) block and fatally erroring on the second failure. Since this code is being restructured, it would be a good opportunity to align the Windows error handling.
Extended reasoning...
Bug Description
In the Windows branch of FileCopier.copy() (lines 120-131), when bun.copyFile fails, the error recovery logic attempts to create parent directories and retry. However, the retry is gated inside if (bun.Dirname.dirname(u16, entry.path)) |entry_dirname|. If dirname returns null — which happens for files at the root of a package with no directory component in their path — the entire catch block does nothing, and the original copy error is silently discarded.
Code Path
Here is the concrete flow for a root-level file like index.js:
entry.pathisindex.js(no directory separator)bun.copyFile(src, dest_abs)fails (e.g., due to permissions, disk full, or any transient error)- The
catchblock executes bun.Dirname.dirname(u16, "index.js")returnsnullbecause there is no directory component- The
ifbranch is skipped entirely - Execution continues to the next file — no retry, no error propagation
- The file is simply missing from the output with no diagnostic
Comparison with POSIX Branch
The POSIX branch (lines 155-164) handles this correctly. It uses a labeled block pattern:
var dest = dest_dir.createFileZ(entry.path, .{}) catch dest: {
if (bun.Dirname.dirname(...)) |entry_dirname| {
bun.MakePath.makePath(...);
}
break :dest dest_dir.createFileZ(entry.path, .{}) catch |err| {
Output.prettyErrorln(...);
Global.exit(1);
};
};Critically, the break :dest retry is outside the if (dirname) block — so it always retries regardless of whether dirname returned null. And on the second failure, it fatally errors with Global.exit(1). This means the POSIX path never silently drops a file.
Impact
The practical impact is limited but real: if copyFile fails for a root-level file in a package on Windows (e.g., package.json, index.js, LICENSE), the file will be silently missing from the installed package. This could cause confusing runtime errors that are very difficult to diagnose since no installation error is reported.
Suggested Fix
Align the Windows error handling with the POSIX branch. After the if (dirname) block, always retry the copy and propagate the error on failure:
bun.copyFile(this.src_path.sliceZ(), dest_abs).unwrap() catch {
if (bun.Dirname.dirname(u16, entry.path)) |entry_dirname| {
bun.MakePath.makePath(u16, dest_dir, entry_dirname) catch {};
}
// Always retry, even if dirname was null
switch (bun.copyFile(this.src_path.sliceZ(), dest_abs)) {
.result => {},
.err => |err| return .initErr(err),
}
};Pre-existing Note
This bug is pre-existing — the same control flow existed before this PR. The PR only changed the path arguments from relative to absolute. However, since this exact code block was restructured by the PR, it is a missed opportunity to fix the asymmetry.
| if (bun.windows.CreateDirectoryExW(this.src_path.sliceZ(), dest_abs, null) == 0) { | ||
| bun.MakePath.makePath(u16, dest_dir, entry.path) catch {}; | ||
| } | ||
| }, | ||
| .file => { | ||
| bun.copyFile(this.src_path.sliceZ(), this.dest_subpath.sliceZ()).unwrap() catch { | ||
| bun.copyFile(this.src_path.sliceZ(), dest_abs).unwrap() catch { | ||
| if (bun.Dirname.dirname(u16, entry.path)) |entry_dirname| { | ||
| bun.MakePath.makePath(u16, dest_dir, entry_dirname) catch {}; | ||
| switch (bun.copyFile(this.src_path.sliceZ(), this.dest_subpath.sliceZ())) { | ||
| switch (bun.copyFile(this.src_path.sliceZ(), dest_abs)) { |
There was a problem hiding this comment.
🟣 Pre-existing issue: The source path (this.src_path.sliceZ()) passed to CopyFileW (line 122) and CreateDirectoryExW (line 117) does not receive the \\?\ NT path prefix, while the destination does. On Windows, both paths need this prefix to bypass the MAX_PATH (260 char) limit. Since this PR specifically fixes long-path handling, it would be worth applying addNTPathPrefixIfNeeded to the source path as well for completeness.
Extended reasoning...
What the bug is
The PR adds the NT path prefix (\\?\) to the destination path via bun.strings.addNTPathPrefixIfNeeded, but the source path (this.src_path.sliceZ()) is passed directly to both CreateDirectoryExW (line 117) and bun.copyFile/CopyFileW (lines 122, 125) without any NT prefix treatment.
Why it matters
On Windows, CopyFileW resolves paths internally and is subject to the MAX_PATH (260 character) limit unless both the source and destination paths carry the \\?\ NT prefix. The underlying call in copy_file.zig:116 is CopyFileW(in.ptr, out.ptr, 0) with no internal prefix handling. If the source package cache path exceeds 260 characters — for example, C:\Users\longusername\projects\my-very-long-project-name\node_modules\.bun\install\packages\@emotion+use-insertion-effect-with-fallbacks@2.0.0\node_modules\@emotion\use-insertion-effect-with-fallbacks\dist\... — the copy would fail with ENOENT on the source side.
Step-by-step proof
this.src_pathis abun.AbsPath— an absolute Windows path likeC:\Users\...\node_modules\.bun\install\packages\@scope+pkg@1.0.0\node_modules\@scope\pkg\lib\deeply\nested\file.jsthis.src_path.append(entry.path)extends it with the walker entrythis.src_path.sliceZ()returns the raw wide string — no\\?\prefixbun.copyFile(this.src_path.sliceZ(), dest_abs)callsCopyFileW(in.ptr, out.ptr, 0)directly- If
this.src_pathexceeds 260 chars, Windows rejects the source path even thoughdest_abshas the NT prefix
Pre-existing nature
This is not introduced by this PR. The same pattern exists in Hardlinker.zig (line 83) where this.src.sliceZ() is passed to sys.link without the NT prefix. Before this PR, neither path had the prefix — so the PR is strictly an improvement. The original issue (#28133) was specifically about relative destination paths, not source paths. The source path (package cache) is also much less likely to exceed MAX_PATH than the destination (which combines CWD + deep node_modules nesting).
Suggested fix
Apply addNTPathPrefixIfNeeded to the source path as well, using a similar buffer allocation pattern. This would make the long-path fix complete for both sides. The same fix could be applied to Hardlinker.zig line 83 for consistency.
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
This is extremely expensive. Do not do this. Come up with a better approach.
2 participants
Summary
bun installwhen using the copyfile backend (isolated installs)FileCopierwas passing relativedest_subpathtoCopyFileWandCreateDirectoryExW, which require absolute paths to handle long paths correctlydest_subpathfor each entry and add the NT path prefix — matching the pattern already used byHardlinker.zigRoot Cause
On Windows,
CopyFileWresolves relative paths against the process CWD but fails with ENOENT when the resulting path exceedsMAX_PATH(260 chars) without the\\?\NT prefix. Packages with long scoped names like@emotion/use-insertion-effect-with-fallbackseasily exceed this limit when combined with thenode_modules/.bun/install/packages/prefix.The POSIX code path was unaffected because it uses fd-relative operations (
openat,createFileZ, fd-based copy).Test plan
test/regression/issue/28133.test.ts) that tests copyfile backend with isolated install for scoped packagesCloses #28133
🤖 Generated with Claude Code