Conversation
The native single-file fast path behind fs.cpSync, fs.cp and fs.promises.cp was also taken when the destination already existed as a regular file. It opens the existing file and rewrites it in place, so when the destination is a hard link the new contents show up in every other link to the inode, and a read-only destination fails with EACCES. node unlinks the destination and creates a fresh file, which is also what bun's own directory walker does for nested files. Only take the native copy when the destination does not exist, the same condition the recursive fast path already uses. An existing destination goes through the ported walker (unlink, copyFile, chmod).
|
Warning Review limit reached
Next review available in: 44 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 (3)
Comment |
|
Status: fix pushed, CI running (Buildkite build 94902); the automated review found nothing to change and there are no open review threads. Reproduced on bun 1.4.0 with the script in the PR description: With this branch the new tests in |
There was a problem hiding this comment.
LGTM — narrows the single-file native fast-path gate to !destStat, so an existing dest falls through to the ported node walker's unlink-then-copy.
What was reviewed:
- Traced both callers (
fs.ts:979,fs.promises.ts:188) — whenokis false,checkedis threaded tocpSyncFn/cpFn, which reachesmayCopyFile→unlink(dest)→copyFile→chmod(srcStat.mode), matching node. - Confirmed the change is symmetric across the sync/async twins and only tightens the gate; the recursive-directory branch already used the same
!destStatcondition. - Checked the read-only test's mode assertion: the walker chmods dest to
srcStat.mode, so comparing against the source file's mode is the right invariant.skipIfon root/Windows is correct. - Hard-link tests assert
storecontent, dest content, andnlinktogether — would fail withstore: "new",nlink: 2on the old code.
Extended reasoning...
Overview
Two-line production change: in tryNativeFastPathSync (src/js/internal/fs/cp-sync.ts) and tryNativeFastPath (src/js/internal/fs/cp.ts), the return condition tightens from srcStat.isFile() && (!destStat || destStat.isFile()) to srcStat.isFile() && !destStat. The accompanying comment is updated to explain why an existing regular-file dest must go through the ported walker (node unlinks first; the native binding rewrites the inode in place). Five new tests in test/js/node/fs/cp.test.ts cover the hard-link case (cpSync, promises.cp, callback cp) and the read-only-dest case (cpSync, promises.cp).
Security risks
None. This is a Node.js-compat behavioral fix in the fs.cp dispatch layer. The change only narrows when a native fast path is taken; the fallback is the existing node-ported walker already used for recursive copies, non-default options, and every other non-regular-file case. No new syscalls, no new user-controlled inputs, no auth/crypto/permissions surface.
Level of scrutiny
Low-to-medium. The production diff is a single boolean-gate tightening applied identically to both sync/async twins — a strictly safer change (fast path taken in fewer cases, and the fallback is the reference implementation). I traced the callers in src/js/node/fs.ts and src/js/node/fs.promises.ts to confirm the {ok: false, checked} result is threaded into cpSyncFn/cpFn, which then reaches onFile → mayCopyFile (since destStat is set) → unlink(dest) + copyFile + chmod(srcStat.mode). That is exactly node's algorithm, and it's the same path the recursive walker already uses per-file when merging into an existing tree — so the "dir/dir already worked" observation in the PR description checks out.
Other factors
- The recursive-directory branch in the same functions already gates on
!destStat; this brings the single-file branch to parity. - Tests follow harness conventions:
tempDirwithawait using, exact-value object assertions, callback form wired throughPromise.withResolverswith the error path rejecting. The read-only test correctly skips as root (mode not enforced) and on Windows; its mode assertion compares against the source file's mode, which is whatsetDestModewrites. - The PR description states the whole
cp.test.tsfile and the 77 upstreamtest-fs-cp-*parallel tests pass on the debug build, and the before/after hard-link assertions demonstrate the tests would fail on the old code. - No CODEOWNERS entries cover these paths; no prior reviewer comments to address.
|
Updated 4:05 PM PT - Aug 13th, 2026
❌ @robobun, your commit aca0dff has some failures in 🧪 To try this PR locally: bunx bun-pr 38226That installs a local version of the PR into your bun-38226 --bun |
Problem
fs.cpSync(file, dest),fs.cp(file, dest, cb)andfs.promises.cp(file, dest)onto adestthat already exists as a regular file write the new contents into the existing inode. Ifdestis a hard link (bun install's Linux backend and pnpm both hard-linknode_modulesfiles out of their store), every other link to that inode now has the new contents too. node leaves the other links untouched; so does bun's own recursivecp(dir, dir)for the same file one level down.destwith mode0444, all three forms fail withEACCES: permission denied, open '.../dest'when not running as root. node succeeds.tryNativeFastPathSync(src/js/internal/fs/cp-sync.ts:308) andtryNativeFastPath(src/js/internal/fs/cp.ts:180) hand the copy to the native binding whensrcis a regular file anddestis missing or a regular file. The native copy (copy_single_file_sync,src/runtime/node/node_fs.rs:8551) opensdestwithO_WRONLY | O_CREATand clones /copy_file_ranges into whatever inode is there. node'smayCopyFile(and the ported walker's,cp-sync.ts:375) doesunlink(dest)thencopyFile, creating a new inode.Fix
destdoes not exist, which is the condition the recursive fast path in the same functions already uses. An existingdestfalls through to the ported walker, which unlinks it, copies and chmods exactly as node does. The stats collected by the gate are passed along, so the walker does not re-stat.test/js/node/fs/cp.test.ts: new hard-link tests forcpSync,promises.cpand callbackcp, and read-only-dest tests forcpSyncandpromises.cp(skipped as root, where the mode is not enforced, and on Windows). Before the fix the hard-link tests fail withstore: "new",nlink: 2; as a non-root user the read-only tests fail withEACCES. With the fix the file passes (50 pass, 7 skip on Linux), and all 5 new tests pass as a non-root user.test/js/node/test/parallel/test-fs-cp-*tests pass with the debug build.Background
fs.cpfamily dispatch (src/js/node/fs.tscpSync,src/js/node/fs.promises.tscp): when no option other than the defaultforce: trueis set, the JS side first runs node's validation (checkPaths,checkParentPaths) and then decides between the native binding and the JS port of node'slib/internal/fs/cp/walker. ThetryNativeFastPath*functions are that decision; they only say yes when the native result is indistinguishable from the walker's.copyFileSyncwrites through in both node and bun; onlycphas the unlink-first semantics.Repro
Before (bun 1.4.0), the fuller script covering all forms:
After (this branch), and node v26.3.0 for the single-file forms:
no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/fs/cp.test.ts