node:fs: set directory times after copyDir in cp with preserveTimestamps by robobun · Pull Request #47255 · oven-sh/bun

robobun · 2026-10-28T22:23:02Z

Problem

fs.cpSync, fs.cp and fs.promises.cp with preserveTimestamps: true keep the times of files. Each directory gets the time of the copy. Node v26.12.1, which process.version reports, keeps directory times too (fs: preserve directory timestamps in cp nodejs/node#65540).
Cause: only the file arm of the two walkers sets times (src/js/internal/fs/cp.ts:308, src/js/internal/fs/cp-sync.ts:441).

Fix

copyDir sets the times of a directory after its last entry, in both walkers. Created and existing directories both pass through it, in all three forms.
Verified: test/js/node/fs/cp-preserve-timestamps.test.ts (40 cases, 33 fail on main, all pass on node v26.12.1), a root-only case in cp.test.ts, node's two -preserve-timestamps-dir tests.
Self-reviewed: 10 concerns raised, 8 addressed. Not done: the cpSync error without recursive (separate work) and new pins for the walkers' header links (unrelated).

Background

With the option, cp runs in two JS walkers, a port of node's lib/internal/fs/cp.
Considered node's three call sites: the same result from four statements. Considered a native step: node:fs: copy file times natively and exactly in cp with preserveTimestamps #41868 has one for files, and it always sets a mode.

Downsides

With the option, a copy stops after the entries of an existing directory whose times it cannot set: EPERM (another user's directory, EACCES on macOS) or EROFS. It stops with ENOENT from stat when a source directory goes away after the copy handled its entries. Node v26.12.1 does the same, and main completes the copy.
With the option, an existing destination directory takes the times of its source.
Without the option: +2 bytecodes per directory in cpSync, +4 in cp, no new call, cell or syscall. With it: +1 stat, +1 utimes per directory.

Notes

Where this comes from. No Bun user reported it. An automated comparison with node v26.12.1 found it. The PR makes three of node's v26.12.1 test files pass. It is not a sync of node:fs to v26.12.1.

Repro (Linux x64, same result in the three forms):

fs.mkdirSync("src/a/b", { recursive: true });
fs.writeFileSync("src/a/b/f.txt", "x");
const old = new Date("2001-02-03T04:05:06Z");
for (const p of ["src/a/b/f.txt", "src/a/b", "src/a", "src"]) fs.utimesSync(p, old, old);
fs.cpSync("src", "dest", { recursive: true, preserveTimestamps: true });
// mtime of dest/a/b/f.txt, dest/a/b, dest/a, dest
// bun 1.4.3 and main: 2001, now, now, now
// node v26.12.1:      2001, 2001, 2001, 2001    (node v26.3.0: as bun)

A probe of 39 rows (three forms, with filter, dereference, verbatimSymlinks, force: false, mode, an existing destination tree, a rejected sub-directory, a link to a directory, a read-only directory) differs from node v26.12.1 in 34 rows on main and in 1 row with this PR. That row is the error of cpSync on a directory without recursive, which this PR does not touch.

What node does. Node stamps after copyDir in onDir and in mkDirAndCopy of cp.js, in a finally of copyDir in cp-sync.js (the walk it uses with a filter), and in its C++ walk. copyDir is the one function both directory arms of each Bun walker call, so one statement there covers the same cases. Order per directory (call log): entries, utimes(dir), chmod(dir) for a directory the copy creates. Entries, utimes(dir), no chmod for one that exists. Child before parent, root last.

Behavior changes, for a maintainer to rule on. The first two come from one choice: a directory that existed in the destination before the copy is stamped too, as node v26.12.1 does (onDir in cp.js, lines 306 to 309, the same on node's main today). The third comes from the stamp itself, and a directory that the copy creates has it too.

With the option, an existing destination directory takes the times of its source, also when nothing is copied into it. A copy with force: false into a complete destination wrote nothing on main. It now sets the times of each directory, and a watcher on the destination sees it: 4 change events from fs.watch for a tree of 4 directories (main 0).
With the option, a copy into an existing directory whose times the caller cannot set now throws. These calls succeed on bun 1.4.3 and main. utimes with explicit times needs the owner of the directory (utimensat(2)). XNU answers another user with EACCES where Linux answers EPERM (vnode_authattr).

destination	node v26.12.1	main	this PR
holds a directory `a` of another user (mode 0777), caller is uid 65534	`EPERM` (`EACCES` on macOS, seen in CI), syscall `utime`, path `dest/a`. The entries of `a` are copied, the 8 siblings after it are not, the parent is not stamped	copies all, `a` keeps its time	as node
is a sticky directory of another user, such as `/tmp`	the same error for the destination, after all entries are copied	copies all	as node
holds an append-only directory `a`, caller is root	`EPERM` `utime` `dest/a`, its entry is copied	copies all	as node
is on a read-only filesystem, `force: false`, nothing to write	`EROFS` `utime` `dest/a`	succeeds	as node

Which entries are copied before the stop depends on the order of the directory. A retry stops at the same place. Without the option the copy completes.

With the option, a copy now throws ENOENT when a source directory goes away after the copy handled its entries. These calls complete on bun 1.4.3-canary.1 and with main's walkers. After the entries of a directory, the stamp reads the source directory again with stat, and that stat fails. A filter that moves a directory at its last entry does this on every run. A second process that cleans the source during the copy can do it too.

fs.mkdirSync("src/a", { recursive: true });
fs.writeFileSync("src/a/skipped.txt", "x");
const filter = p => {
  if (!p.endsWith("skipped.txt")) return true;
  fs.renameSync("src/a", "moved");
  return false;
};
fs.cpSync("src", "dest", { recursive: true, preserveTimestamps: true, filter });
// node v26.12.1 and this PR: ENOENT, syscall stat, path src/a
// main's walkers, bun 1.4.3-canary.1 and node v26.3.0: returns, dest/a is there

fs.promises.cp and callback cp do the same. So does a dest/a that exists before the copy. Without the option, or when nothing moves, all five complete the copy. Runs: the three forms 10 times each on node v26.12.1, on this PR and on main's walkers (this branch built with main's cp.ts and cp-sync.ts), one output per runtime. No test pins this case.

The second stat is node's (setDestTimestamps): the copy gets the atime that the source has after the copy read it. Measured on ext4 with relatime, node v26.12.1 and this PR: a source directory with both times in 2001 gives a copy with mtime 2001 and the atime of the copy. A stamp from the stat that the walker already has would not fail here, and it would give atime 2001, which node does not.

The two shapes. Node's tests assert the times of directories that the copy creates. None asserts a directory that existed before the copy, so both shapes pass them.

As pushed: the statement is at the end of copyDir, which both directory arms call. Node's result where node succeeds, node's error where it fails. A second look at the design, with the failures of change 2 counted, kept this shape: the other one reports success and leaves directories unpreserved in calls as plain as mkdir dest followed by cp, or a second copy into the same destination.
Created directories only: move the statement in each walker from copyDir into mkDirAndCopy, between copyDir and setDestMode. Changes 1 and 2 do not happen. Change 3 stays for a directory that the copy creates (measured with the statement moved: ENOENT in the three forms). An existing destination directory keeps its own times, where node gives it the source's. The 12 cases for an existing destination and the 3 for a sticky directory would then move to Bun-only tests.

If the second shape is wanted, say so and I will switch. It is two moved lines and the test move.

Still different from node.

cpSync with a filter that throws part-way: node's filter walk stamps and chmods the directories it unwinds through (the finally). Bun does neither and throws the filter's error, as node's three other walks do. A test pins the error.
Times are whole milliseconds, for directories as for files on main (setDestTimestamps goes through Date). node:fs: copy file times natively and exactly in cp with preserveTimestamps #41868 makes file times exact with a native step that also sets a mode, and it deletes setDestTimestamps. It says directories are separate work. When it is rebased over this PR, it has to keep setDestTimestamps for the two copyDir calls or add a step that sets times only. Until then a copied directory can read up to 1 ms older than its source.
Without the option, Bun on macOS makes a new destination with one clonefile(2) when the copy has no filter and the tree holds only files and directories. A clone keeps every time, so there the directories have the source's times with or without the option. Node gives them the time of the copy. This PR does not change that path.
Not ported here: fs: pass symlink type in cp when filter is provided nodejs/node#62654 (the symlink type that onLink passes, Windows). The error of cpSync on a directory without recursive (node throws from a C++ check with another message and no path).

Cost (release builds of main 11c367163a and the branch, Linux x64):

Bytecodes per directory without the option: +2 in cpSync copyDir (get_by_id, jfalse), +4 in cp / promises.cp copyDir. The tail calls of the sync walker stay (BUN_JSC_dumpGeneratedBytecodes).
node:fs calls per copy of 17 directories and 256 files: 1,383 = 1,383 without the option. With it 1,895 -> 1,929, +2 per directory (call counter over the node:fs exports).
File system syscalls for 5 directories and 16 files: cpSync 222 = 222 without the option, 254 -> 264 with it. promises.cp 227 = 227 and 259 -> 269 (statx +5, utimensat +5, gdb catch syscall).
GC cells per directory: +0 without the option. With it +3 in cpSync, +14 cells and +5 promises in promises.cp (heapStats slope between 100 and 200 directories, BUN_JSC_useGC=0).
Built-in JS text: internal/fs/cp.js +71 bytes, internal/fs/cp-sync.js +65. No host function, no native file in the diff. The walkers load on first use, so start-up does not change.

Other open PRs in these files. #47102 adds an early return in the sync copyDir for an empty directory, and #41112 adds one in the async copyDir. Both merge above the new statement with no conflict, and then skip it. The empty-directory rows of the new tests fail for #47102 in that case.

Tests.

cp-preserve-timestamps.test.ts uses node:test and node:assert, so the same file runs under node. For cpSync, promises.cp and callback cp: a new destination, an existing destination tree (its directories get the source's times, a directory keeps its mode, a file the source does not have keeps its time), each also with filter, dereference and force: false. A directory reached through a link with dereference. A sub-directory that the filter rejects. Parents created above the destination. No stamp without the option. A copy into a sticky directory of another user (runs when the directory above the temporary directory is one). A filter that throws in cpSync. Only mtime is asserted.
Node v26.12.1: 40 pass as a normal user, 37 pass and 3 skip as root. Main: 33 fail. This PR: as node.
cp.test.ts has the root-only case with a directory of another user inside the destination. It also checks that the parent is not stamped.
Node's test-fs-cp-sync-preserve-timestamps-dir.mjs and test-fs-cp-async-preserve-timestamps-dir.mjs (both fail on main) and test-fs-cp-sync-preserve-timestamps-force-false.mjs, unedited.
Also run with this build: all 80 test-fs-cp* node tests and test/js/node/fs/.
Not run here: Windows and macOS. CI runs them. The first macOS run failed two of the new cases for the two macOS facts above (the clone, and EACCES), and the cases now allow for both.

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, test/js/node/fs/cp-preserve-timestamps.test.ts

fs.cpSync, fs.cp and fs.promises.cp gave each copied file the times of its source and left every directory with the time of the copy. Node v26.12.1 gives a directory the times of its source too (nodejs/node#65540). copyDir is the one function that fills a directory, for a directory the copy creates and for one that exists. It now sets the times after the last entry, so both cases and the three forms are covered by one statement in each walker. Add node's tests for this: test-fs-cp-sync-preserve-timestamps-dir.mjs, test-fs-cp-async-preserve-timestamps-dir.mjs and test-fs-cp-sync-preserve-timestamps-force-false.mjs.

…ops at a directory Move the preserveTimestamps cases to cp-preserve-timestamps.test.ts, written with node:test so the same file runs under node. Add a directory reached through a link with dereference, a copy into a sticky directory of another user (EPERM from utimes, entries copied), and a filter that throws in cpSync. The root-only case in cp.test.ts now also checks that the walk stops: the parent of the failing directory is not stamped. Cut the two comments to one line each, with the node v26.12.1 lines.

coderabbitai · 2026-10-28T22:23:11Z

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 15 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration

Source mode: Auto
Repository: oven-sh/bun
Branch: robobun/a8703464/cp-preserve-directory-times
File: .coderabbit.yaml
Commit: 8de7a43
Layers: Repository YAML
Review profile: ASSERTIVE
Plan: Advanced
Run ID: ddc8f62c-6b6c-4c49-a776-56b6ed17565e

Commits

Reviewing files that changed from the base of the PR and between 73b5310 and 8de7a43.

Files selected for processing: 7

src/js/internal/fs/cp-sync.ts
src/js/internal/fs/cp.ts
test/js/node/fs/cp-preserve-timestamps.test.ts
test/js/node/fs/cp.test.ts
test/js/node/test/parallel/test-fs-cp-async-preserve-timestamps-dir.mjs
test/js/node/test/parallel/test-fs-cp-sync-preserve-timestamps-dir.mjs
test/js/node/test/parallel/test-fs-cp-sync-preserve-timestamps-force-false.mjs

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

robobun · 2026-10-28T22:23:14Z

Status

Reproduced on bun 1.4.3 and on main (6c3d1bf956): after fs.cpSync("src", "dest", { recursive: true, preserveTimestamps: true }) the files in dest have the source's mtime and every directory has the time of the copy. fs.cp and fs.promises.cp do the same. Node v26.12.1 keeps the directory times.
The new test file test/js/node/fs/cp-preserve-timestamps.test.ts fails 33 of 40 cases on main. With this PR it passes as on node v26.12.1.
Fix: this PR, node:fs: set directory times after copyDir in cp with preserveTimestamps #47255.
CI: the macOS test jobs pass on x64 and aarch64 at 8de7a43a5a (build 134922), so the two macOS fixes hold. Every Linux and Windows test job that ran the cp tests passed at dbefd71c25 (build 134685), and only macOS expectations in the tests changed after that. The red jobs of build 134922 are 6 test-aot-drawn jobs (tests under test/aot/), which this PR does not touch. 26 Linux jobs of that build were canceled with Failed to create agent.

robobun · 2026-10-28T22:23:17Z

^{Updated 2:48 AM PT - Oct 29th, 2026}

❌ @robobun, your commit 8de7a43 has 7 failures in Build #134922 (All Failures):

test/aot/perf/generated-code-of-idioms-14.test.ts - code 1 on 🐧 13 x64 - test-aot-drawn
test/aot/programs-that-ask-the-shell-3.test.ts - code 1 on 🐧 13 aarch64 - test-aot-drawn
test/aot/perf/generated-code-of-idioms-13.test.ts - code 1 on 🐧 13 x64 - test-aot-drawn
test/aot/perf/generated-code-of-idioms-12.test.ts - code 1 on 🐧 13 aarch64 - test-aot-drawn
test/aot/perf/generated-code-of-idioms-15.test.ts - code 1 on 🐧 13 aarch64 - test-aot-drawn
test/aot/minifying-first-saves-no-code-16.test.ts - code 1 on 🐧 13 x64 - test-aot-drawn
Failed to create agent for 🐧 3.23 x64 - test-bun

CapacityLimitExceeded

Failed to create agent for 🐧 3.23 x64 - test-bun

CapacityLimitExceeded

Failed to create agent for 🐧 3.23 x64 - test-bun

CapacityLimitExceeded

Failed to create agent for 🐧 3.23 x64 - test-bun

CapacityLimitExceeded

Failed to create agent for 🐧 x64-musl - verify-baseline

CapacityLimitExceeded

Failed to create agent for 🐧 3.23 x64 - test-bun

CapacityLimitExceeded

Failed to create agent for 🐧 3.23 x64 - test-bun

CapacityLimitExceeded

Failed to create agent for 🐧 3.23 x64 - test-bun

CapacityLimitExceeded

Failed to create agent for 🐧 3.23 x64 - test-bun

CapacityLimitExceeded

Failed to create agent for 🐧 3.23 x64 - test-bun

CapacityLimitExceeded

Failed to create agent for 🐧 3.23 x64 - test-bun

CapacityLimitExceeded

Failed to create agent for 🐧 25.04 x64 - test-bun

CapacityLimitExceeded

Failed to create agent for 🐧 25.04 x64 - test-bun

CapacityLimitExceeded

Failed to create agent for 🐧 25.04 x64 - test-bun

CapacityLimitExceeded

Failed to create agent for 🐧 13 x64 - test-bun

CapacityLimitExceeded

Failed to create agent for 🐧 13 x64 - test-aot

CapacityLimitExceeded

Failed to create agent for 🐧 25.04 x64 - test-bun

CapacityLimitExceeded

Failed to create agent for 🐧 25.04 x64 - test-bun

CapacityLimitExceeded

Failed to create agent for 🐧 13 x64 - test-bun

CapacityLimitExceeded

Failed to create agent for 🐧 13 x64 - test-bun

CapacityLimitExceeded

Failed to create agent for 🐧 3.23 aarch64 - test-bun

CapacityLimitExceeded

Failed to create agent for 🐧 3.23 aarch64 - test-bun

CapacityLimitExceeded

Failed to create agent for 🐧 3.23 aarch64 - test-bun

CapacityLimitExceeded

Failed to create agent for 🐧 3.23 aarch64 - test-bun

CapacityLimitExceeded

Failed to create agent for 🐧 3.23 aarch64 - test-bun

CapacityLimitExceeded

Failed to create agent for 🐧 aarch64-musl - verify-baseline

CapacityLimitExceeded

🧪 To try this PR locally:

bunx bun-pr 47255

That installs a local version of the PR into your bun-47255 executable, so you can run:

bun-47255 --bun

claude

Beyond the inline findings, I also checked that a copy aborting inside the new utimes step leaving a freshly created directory with mkdir's default mode is not a divergence: node's mkDirAndCopy also runs copyDir (and its stamp) before chmod, so the order matches. Both walkers (cp-sync.ts, cp.ts, which the callback form also uses) receive the same statement, and neither has an ahead-of-time/ twin that would need the change.

Extended reasoning...

The change adds one preserveTimestamps guard to copyDir in each of the two JS cp walkers so directories receive the source's atime/mtime, plus node:test-based and Bun-harness tests; no security-sensitive surface is touched. Two confirmed findings are posted inline (a macOS clonefile fast path that can keep source mtimes and break the new negative test, and the behavioral change to EPERM on pre-existing directories owned by another uid), so a human should weigh them; this note only records what else was examined.

claude · 2026-10-28T22:35:40Z

    if (!skipped) getStats(destStat, srcItem, destItem, opts);
  }
+  // Not in a `finally` as in node's filter walk: a copy that throws keeps its times and mode, as in node's native cpSync. https://github.com/nodejs/node/blob/v26.12.1/lib/internal/fs/cp/cp-sync.js#L188-L197
+  if (opts.preserveTimestamps) setDestTimestamps(src, dest);


🔴 Callers using preserveTimestamps into an existing destination directory they do not own now get EPERM after its entries were copied; the base copies and succeeds. copyDir at src/js/internal/fs/cp-sync.ts:502 (same at src/js/internal/fs/cp.ts:380) stamps every directory, including pre-existing ones, and utimes with explicit times needs the owner. The PR calls this accepted Node parity; with force: false it also rewrites every existing directory's mtime when nothing is copied. Fix: stamp only directories the copy creates (move the call into mkDirAndCopy in both walkers) or make a failed stamp of a pre-existing directory non-fatal, so root-owned world-writable dests keep working. [also at: src/js/internal/fs/cp.ts:380 - Users who run an idempotent cp/cpSync with force: false and preserveTimestamps: true into a destination on a read-only mount now get an EROFS rejection where the base branch returned success as a no-op. copyDir at src/js/internal/fs/cp.ts:380 and src/js/internal/fs/cp-sync.ts:502 call setDestTimestamps on every visited directory, including pre-existing ones that the walk wrote nothing into.]

Why this was flagged

Trigger: fs.cpSync, fs.cp or fs.promises.cp with { recursive: true, preserveTimestamps: true } where dest already exists and is owned by another uid but is writable; this arrives through onDir at src/js/internal/fs/cp-sync.ts:484 / src/js/internal/fs/cp.ts:361, which calls copyDir for an existing directory. copyDir copies the entries, then src/js/internal/fs/cp-sync.ts:502 (async twin src/js/internal/fs/cp.ts:380) calls setDestTimestamps, whose utimesSync at src/js/internal/fs/cp-sync.ts:470 fails with EPERM because utimensat with explicit times requires ownership. The error is thrown after the directory's entries were written, siblings after it are not copied, and the parent is not stamped; on the base branch the same call copies everything and returns normally. A second effect: with force: false into a fully populated destination, nothing is copied but every existing directory's mtime is overwritten with the source's. No guard in the walkers distinguishes a directory the copy created from one that pre-existed.

Verification: src/js/internal/fs/cp-sync.ts:484 onDir with an existing directory falls through to copyDir, which at :502 runs setDestTimestamps; utimensat(2) with explicit times requires the caller to be the file owner, so this throws EPERM after the entries were written. Identical path at src/js/internal/fs/cp.ts:380. On the base neither copyDir had this line, so the same copy completed successfully.

This is the first item under Downsides in the description, and it is a choice a maintainer has to make. The facts, all measured: node v26.12.1 stamps a directory that existed before the copy and throws the same error at the same place (its onDir, unchanged on node's main today). Node's own tests only check directories that the copy creates, so they pass with either shape. The shape you propose (stamp in mkDirAndCopy only) is two moved lines. It never fails where main succeeds, and it leaves an existing destination directory with its own times, where node gives it the source's. I am weighing the shapes a second time (as pushed, created directories only, or stamp existing directories and not fail on them) and will post the outcome in this thread. I leave the thread open until then.

The outcome of the second look: the shape stays as pushed, and the choice is now written out for a maintainer in the description ("Behavior changes, for a maintainer to rule on" and "The two shapes").

What decided it: with the stamp in mkDirAndCopy only, calls as plain as mkdir dest followed by cp, or a second copy into the same destination, report success and leave directories at the time of the copy, where node v26.12.1 gives them the source's times. The price of the shape as pushed is the set of calls in the table there. They succeed on main and now throw as on node: EPERM (EACCES on macOS) for an existing directory of another user, EROFS on a read-only filesystem. A copy with force: false into a complete destination now also sets directory times (4 change events from fs.watch for 4 directories, main 0).

I did not take the third shape (stamp an existing directory and not fail on it). It returns success after a stamp that was refused, and nothing else in the two walkers ignores a failed step that an option asked for.

The switch to your shape is two moved lines and a move of 15 test cases. I leave this thread open for the maintainer who rules on it.

One correction to my two replies above. The shape with the stamp in mkDirAndCopy only can also fail where main succeeds.

After the entries of a directory, the stamp reads the source directory again with stat. If that source directory is gone by then, the copy throws ENOENT (syscall stat). A filter that moves a directory at its last entry does this on every run. A second process that cleans the source during the copy can do it too. A directory that the copy creates is stamped in both shapes, so both shapes have this failure.

Measured with preserveTimestamps: true, the same result in cpSync, promises.cp and callback cp:

the copy creates dest/a dest/a exists

node v26.12.1 ENOENT ENOENT

as pushed ENOENT ENOENT

stamp in mkDirAndCopy only (measured with the statement moved) ENOENT completes

main's walkers, bun 1.4.3-canary.1, node v26.3.0 completes completes

The description has it now: the first item under Downsides, and change 3 in the Notes with the repro. It gives no reason to switch shapes, because a created directory fails the same way in both. The choice between the shapes stays with a maintainer.

Without a filter, Bun on macOS makes a new destination with one clonefile(2), and a clone keeps the times of the source. The case that checks that directories are not stamped without preserveTimestamps now passes a filter for the new destination, so it runs the walker on every platform.

…ectory utimes with explicit times on a directory of another user fails with EPERM on Linux and with EACCES on macOS.

claude

I re-reviewed the two newer commits and found no bugs in this revision; a human should still weigh in, because the production change deliberately picks one of two behaviors (stamping pre-existing destination directories as Node v26.9+ does, versus only the directories the copy creates) and that is a compatibility policy call rather than a correctness question.

What was reviewed:

The two copyDir additions: preserveTimestamps always routes to the JS walkers (src/js/node/fs.ts:1007, src/js/node/fs.promises.ts:177), and in mkDirAndCopy the stamp lands before the final chmod, so a read-only source mode cannot block utimes on a directory the copy owns.
The no-option negative test now uses a filter for the fresh destination and a pre-existing destination for the other copy, so neither can take the darwin clonefile path; the earlier macOS concern is addressed.
The root-only fixture in cp.test.ts: unprivilegedSpawnOptions chowns the tree to nobody first, then to-*/a is chowned back to root at 0777, so the file copy succeeds and only the utime on that directory fails; syscall: "utime" matches node_fs.rs:8648.

Extended reasoning...

The diff adds one guarded setDestTimestamps call to copyDir in both the sync and async cp walkers under src/js/internal/fs/, plus ~450 lines of tests (a node:test-portable file, a root-gated subprocess case in cp.test.ts, and three Node-style parallel tests). It touches no security-sensitive surface; the new failure modes are EPERM/EACCES/EROFS from utimes on directories the caller does not own, which the PR description itself lists as a downside. The two commits since the prior review are test-only (the clonefile fix and the macOS EACCES expectation); the production lines are unchanged, so the earlier inline thread about stamping pre-existing directories remains the open design question for a maintainer. Deferring rather than approving because that behavior choice is intentional, user-visible, and not something an automated check can settle.

Still open from earlier reviews (1):

🔴 src/js/internal/fs/cp-sync.ts:502 — Callers using preserveTimestamps into an existing destination directory they do not own now get EPERM after its entries…

If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.

robobun added 2 commits October 28, 2026 21:02

robobun assigned Jarred-Sumner Oct 28, 2026

github-actions Bot added the claude label Oct 28, 2026

claude Bot reviewed Oct 28, 2026

View reviewed changes

robobun added 2 commits October 28, 2026 22:39

node:fs: expect EACCES on macOS when cp cannot set the times of a dir…

8de7a43

…ectory utimes with explicit times on a directory of another user fails with EPERM on Linux and with EACCES on macOS.

claude Bot reviewed Oct 29, 2026

View reviewed changes

This was referenced Oct 29, 2026

node:fs: read into the BYOB view from index 0 in FileHandle.readableWebStream #47246

Open

node:fs: cp keeps a <symlink>/.. root as given, and does not copy a directory into its own output #47102

Open

Navigation Menu

Repository navigation

node:fs: set directory times after copyDir in cp with preserveTimestamps - #47255

node:fs: set directory times after copyDir in cp with preserveTimestamps#47255
robobun wants to merge 4 commits into
mainfrom
robobun/a8703464/cp-preserve-directory-times

robobun commented Oct 28, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Oct 28, 2026 •

edited

Loading

Review limit reached

Uh oh!

robobun commented Oct 28, 2026 •

edited

Loading

Uh oh!

robobun commented Oct 28, 2026 •

edited

Loading

Uh oh!

claude Bot left a comment

Uh oh!

Uh oh!

claude Bot Oct 28, 2026

Uh oh!

robobun Oct 29, 2026

Uh oh!

robobun Oct 29, 2026

Uh oh!

robobun Oct 29, 2026

Uh oh!

claude Bot left a comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	the copy creates `dest/a`	`dest/a` exists
node v26.12.1	`ENOENT`	`ENOENT`
as pushed	`ENOENT`	`ENOENT`
stamp in `mkDirAndCopy` only (measured with the statement moved)	`ENOENT`	completes
main's walkers, bun 1.4.3-canary.1, node v26.3.0	completes	completes

Conversation

robobun commented Oct 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Fix

Background

Downsides

Uh oh!

coderabbitai Bot commented Oct 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Uh oh!

robobun commented Oct 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robobun commented Oct 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

claude Bot Oct 28, 2026

Choose a reason for hiding this comment

Uh oh!

robobun Oct 29, 2026

Choose a reason for hiding this comment

Uh oh!

robobun Oct 29, 2026

Choose a reason for hiding this comment

Uh oh!

robobun Oct 29, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

robobun commented Oct 28, 2026 •

edited

Loading

coderabbitai Bot commented Oct 28, 2026 •

edited

Loading

robobun commented Oct 28, 2026 •

edited

Loading

robobun commented Oct 28, 2026 •

edited

Loading