Skip to content

fs: copy directory trees for fs.cp() on the thread pool - #65488

Open
codebytere wants to merge 2 commits into
nodejs:mainfrom
codebytere:perf/fs-cp-dir-threadpool
Open

fs: copy directory trees for fs.cp() on the thread pool#65488
codebytere wants to merge 2 commits into
nodejs:mainfrom
codebytere:perf/fs-cp-dir-threadpool

Conversation

@codebytere

@codebytere codebytere commented Aug 22, 2026

Copy link
Copy Markdown
Member

Runs the directory walk of fs.cp() / fsPromises.cp() as one thread pool request using the C++ implementation fs.cpSync() already has, instead of a JavaScript walk with several awaited round trips per entry; the first commit fixes that implementation giving created directories default permissions instead of the source directory's mode.

before after
benchmark/fs/bench-cp.js (new, fsPromises.cp() of 500 files), 30 runs +340 % ±14 %
fsPromises.cp() of a 2 100-file tree ~215 ms ~28 ms
main-thread time for that copy ~110 ms < 1 ms
cpSync of a 0700 directory (umask 022) comes out 0755 0700

The JavaScript walk does opendir batches, two stat()s, the copyFile() and a chmod() per entry, each awaited in sequence, with the bookkeeping on the main thread. fs.cpSync() without a filter has done the whole walk in C++ since #58461, but that walk creates directories with default permissions where the JavaScript walk (and cpSync before the port) gives them the source directory's mode; the first commit fixes that (the mode is applied once the directory's contents are copied, so read-only source directories still copy), with a test that fails on main.

The second commit factors the walk into CopyDirRecursive(), which records an error instead of throwing so it can run on any thread, and runs it as a ThreadPoolWork request when the destination directory does not exist yet and nothing has to run per entry (no filter, no dereference, permission model off); copying into an existing tree keeps the JavaScript walk and every rule it has for what may already be there (#58869 lists where cpSync's walk differs). Sockets, FIFOs and unknown entries found by the request are handed back to JavaScript, which rejects them with the same SystemErrors as before (cpSync keeps skipping them). The walk uses the error_code overloads of std::filesystem throughout, so an unreadable directory inside the tree is reported as EACCES by both cp() and cpSync() where cpSync() currently terminates the process; filesystem errors from inside the walk keep their codes and report cp as the syscall, as cpSync does.

Refs: #58461

Tests: new test-fs-cp-sync-directory-mode.mjs, test-fs-cp-async-special-files-in-tree.mjs (a socket and a FIFO inside the tree: rejected by cp(), skipped by cpSync(), same as on main) and test-fs-cp-unreadable-directory.mjs (aborts on main for cpSync); all test-fs-cp* pass; a differential run over the option matrix (dereference, verbatimSymlinks, preserveTimestamps, force/errorOnExist, fresh and pre-populated destinations, symlinks, a socket and a FIFO in the tree) produces the same trees and outcomes as before.


Disclosure: the code, test, benchmark, measurements and this description were written by Claude Code, directed and reviewed by @codebytere.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/performance

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Aug 22, 2026
@codebytere
codebytere force-pushed the perf/fs-cp-dir-threadpool branch from 3065c10 to 6d25d6a Compare August 22, 2026 15:18
The C++ fast path that fs.cpSync() takes when no filter is given created
the destination directories with default permissions, so a 0700
directory came out of the copy as 0755 (with the default umask). The
JavaScript implementation, which fs.cp(), fs.promises.cp() and
fs.cpSync() with a filter still use, chmod()s every directory it
creates to the mode of its source, and so did cpSync before the port.

Set the source directory's permissions on each directory the copy
creates (the destination root included); directories that already
exist keep theirs, as before.

Signed-off-by: Shelley Vohr <shelley.vohr@gmail.com>
@codebytere
codebytere force-pushed the perf/fs-cp-dir-threadpool branch from 6d25d6a to cce0cf5 Compare August 22, 2026 15:55
@codecov

codecov Bot commented Aug 22, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 74.83660% with 77 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.14%. Comparing base (21f0f27) to head (6a197c9).
⚠️ Report is 59 commits behind head on main.

Files with missing lines Patch % Lines
src/node_file.cc 71.26% 38 Missing and 37 partials ⚠️
lib/internal/fs/cp/cp.js 95.55% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #65488      +/-   ##
==========================================
+ Coverage   90.12%   90.14%   +0.01%     
==========================================
  Files         752      751       -1     
  Lines      252315   252791     +476     
  Branches    47444    47576     +132     
==========================================
+ Hits       227395   227867     +472     
+ Misses      16217    16206      -11     
- Partials     8703     8718      +15     
Files with missing lines Coverage Δ
lib/internal/fs/cp/cp.js 92.80% <95.55%> (+4.45%) ⬆️
src/node_file.cc 74.30% <71.26%> (+0.33%) ⬆️

... and 69 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@codebytere
codebytere force-pushed the perf/fs-cp-dir-threadpool branch 2 times, most recently from 73a8c02 to 0ecff36 Compare August 22, 2026 19:42
@jakecastelli jakecastelli added request-ci Add this label to start a Jenkins CI on a PR. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels Aug 23, 2026
Comment thread src/node_file.cc Outdated
@jakecastelli jakecastelli removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 23, 2026
@codebytere
codebytere force-pushed the perf/fs-cp-dir-threadpool branch from 0ecff36 to 508ece6 Compare August 23, 2026 11:19
Comment thread src/node_file.cc
if (error) {
return CpError::Std(error, ConvertPathToUTF8(dest_file_path));
}
CpError inner = copy_dir_contents(entry_dir_path, dest_file_path);

@jakecastelli jakecastelli Aug 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An existing destination directory symlink can reach this recursive call. The new native path follows it and copies into its target, while the previous JS walk rejects with ERR_FS_CP_DIR_TO_NON_DIR.

small repro:

import fs from 'node:fs/promises';
import { join } from 'node:path';
import os from 'node:os';

const root = await fs.mkdtemp(join(os.tmpdir(), 'cp-'));
const src = join(root, 'src');
await fs.mkdir(join(src, 'dir'), { recursive: true });
await fs.writeFile(join(src, 'dir', 'file'), 'x');

for (const [name, filter] of [['native'], ['js', () => true]]) {
  const dest = join(root, name);
  const target = join(root, `${name}-target`);
  await fs.mkdir(dest);
  await fs.mkdir(target);
  await fs.symlink(target, join(dest, 'dir'), 'dir');
  try {
    await fs.cp(src, dest, { recursive: true, filter });
    console.log(name, 'success',
                await fs.readFile(join(target, 'file'), 'utf8'));
  } catch (err) {
    console.log(name, err.code);
  }
}

on v24.7:

native ERR_FS_CP_DIR_TO_NON_DIR

js ERR_FS_CP_DIR_TO_NON_DIR

on this branch:

native success x

js ERR_FS_CP_DIR_TO_NON_DIR

This is an observable behaviour change: the previous async implementation rejects an existing destination directory symlink, while the native path follows it. ref #58869, is aligning the async API with cpSync() here intentional, or should the previous async behaviour be preserved?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jakecastelli not intentional, thanks - the intent was to take the native walk only where it behaves like the JS one, and an existing destination is exactly where the two differ (your #58869 list). 0c17f68 narrows it to the case where the destination directory doesn't exist yet (the mkDirAndCopy path); copying into an existing tree keeps the JS walk and all of its checks, so both halves of your repro reject with ERR_FS_CP_DIR_TO_NON_DIR again. The fresh-destination copy keeps the speedup (2 100 files: ~27 ms vs ~210 ms into an existing tree).

@codebytere
codebytere force-pushed the perf/fs-cp-dir-threadpool branch from 508ece6 to 0c17f68 Compare August 23, 2026 12:31
Comment thread src/node_file.cc
fs.cp() and fs.promises.cp() walked the tree in JavaScript with several
thread pool round trips per entry (opendir batches, two stat()s, the
copyFile(), a chmod()), all awaited in sequence: a 2 100-file tree took
~215 ms with ~110 ms of that on the main thread, against ~36 ms for
fs.cpSync(), which copies the tree in C++ when no filter is given.

Factor that C++ walk into CopyDirRecursive(), which records the error
instead of throwing so that it can run on any thread, and run it as one
ThreadPoolWork request (CpDirJob) for fs.cp()/fs.promises.cp() when
the destination directory does not exist yet and nothing has to run per
entry (no filter, no dereference, permission model off). Copying into
an existing tree keeps the JavaScript walk and its rules for what may
already be there. Sockets, FIFOs and unknown entries found by the job
are reported back to JavaScript, which rejects them with the same
SystemErrors as before (cpSync keeps skipping them). The same tree now
takes ~28 ms with under 1 ms on the main thread.

The walk now uses the error_code overloads of std::filesystem throughout
(directory iteration included), so an unreadable directory inside the
tree is reported as EACCES by both cp() and cpSync() instead of
terminating the process, which cpSync() has done since the walk moved
to C++. Filesystem errors raised inside the walk keep their codes, with
'cp' as the syscall as cpSync reports them.

Signed-off-by: Shelley Vohr <shelley.vohr@gmail.com>

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 23, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 23, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants