fs: copy directory trees for fs.cp() on the thread pool - #65488
fs: copy directory trees for fs.cp() on the thread pool#65488codebytere wants to merge 2 commits into
Conversation
Sorry, something went wrong.
|
Review requested:
|
Sorry, something went wrong.
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>
Codecov Report❌ Patch coverage is 81.41593% with 63 lines in your changes missing coverage. Please review.
@@ Coverage Diff @@
## main #65488 +/- ##
==========================================
+ Coverage 90.12% 90.16% +0.04%
==========================================
Files 752 751 -1
Lines 252315 253662 +1347
Branches 47444 47797 +353
==========================================
+ Hits 227395 228725 +1330
+ Misses 16217 16192 -25
- Partials 8703 8745 +42
... and 84 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Sorry, something went wrong.
mcollina
left a comment
There was a problem hiding this comment.
lgtm
Sorry, something went wrong.
Sorry, something went wrong.
| // entry, links inside the tree must be dereferenced, or the permission model | ||
| // has to check each path. Copying into an existing tree keeps the per-entry | ||
| // walk below and its rules for what may already be there. | ||
| if (!opts.filter && !opts.dereference && !permission.isEnabled()) { |
There was a problem hiding this comment.
Should this also require opts.mode === 0? CpDirJob does not receive opts.mode, so COPYFILE_FICLONE_FORCE is silently ignored and becomes a normal copy.
import { promises as fs, constants } from 'node:fs';
import { join } from 'node:path';
import { tmpdir } from 'node:os';
const root = await fs.mkdtemp(join(tmpdir(), 'cp-'));
const src = join(root, 'src');
await fs.mkdir(src);
await fs.writeFile(join(src, 'file'), 'x');
for (const [name, filter] of [['default'], ['filter', () => true]]) {
try {
await fs.cp(src, join(root, name), {
recursive: true,
mode: constants.COPYFILE_FICLONE_FORCE,
filter,
});
console.log(name, 'success');
} catch (err) {
console.log(name, err.code, err.syscall);
}
}It got rejected for both on v24.7
default ENOSYS copyfile filter ENOSYS copyfile
but on this branch:
default success filter ENOSYS copyfile
Sorry, something went wrong.
There was a problem hiding this comment.
@jakecastelli yes, thanks - done in 7e01f9b: mode !== 0 keeps the JS walk so the flags reach copyFile(), and test-fs-cp-async-with-mode-flags now asserts the outcome is the same with and without a filter. (cpSync's C++ path drops mode the same way today; that one belongs with #58869.)
Sorry, something went wrong.
| } else if (dir_entry.is_regular_file(error)) { | ||
| std::filesystem::copy_file( | ||
| dir_entry.path(), dest_file_path, file_copy_opts, error); |
There was a problem hiding this comment.
This can still write through a destination symlink that appears after the fresh destination directory is created.
Reproduction:
import fs from 'node:fs';
import os from 'node:os';
import path from 'node:path';
const root = fs.mkdtempSync(path.join(os.tmpdir(), 'cp-race-'));
const src = path.join(root, 'src');
const dest = path.join(root, 'dest');
const outside = path.join(root, 'outside');
fs.mkdirSync(src);
for (let i = 0; i < 10_000; i++)
fs.writeFileSync(path.join(src, String(i).padStart(5, '0')), 'source');
const victim = fs.readdirSync(src).at(-1);
fs.writeFileSync(outside, 'outside');
let injected = false;
let done = false;
function race() {
if (!injected && fs.existsSync(dest)) {
injected = true;
fs.symlinkSync(outside, path.join(dest, victim));
}
if (!done) setImmediate(race);
}
race();
await fs.promises.cp(src, dest, { recursive: true });
done = true;
console.log({
injected,
destIsSymlink:
fs.lstatSync(path.join(dest, victim)).isSymbolicLink(),
outside: fs.readFileSync(outside, 'utf8'),
});Node.js v24.7.0 replaces the injected link and leaves the outside file unchanged:
{ injected: true, destIsSymlink: false, outside: 'outside' }
This branch follows the injected link and overwrites its target:
{ injected: true, destIsSymlink: true, outside: 'source' }
Sorry, something went wrong.
There was a problem hiding this comment.
@jakecastelli thanks - 45406cc closes this for files the same way as for directories: on this path every file is created with an exclusive uv_fs_copyfile() (UV_FS_COPYFILE_EXCL plus the mode flags, which also lets mode take this path again) instead of std::filesystem::copy_file, so a link that appears inside the new tree mid-copy gives EEXIST and is neither followed nor replaced; your repro now ends with rejected EEXIST copyfile and outside untouched. that is stricter than the JS walk, which unlinks and re-copies, but the rule for the whole job is now one line: it never opens or follows anything it didn't create.
Sorry, something went wrong.
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. The same tree now takes ~30 ms with under 1 ms on the main thread. For that job the walk follows the JavaScript walk's rules rather than cpSync's: it creates every directory with mkdir() and every file with an exclusive uv_fs_copyfile() (honouring the copyFile() mode flags) and fails with EEXIST if anything has appeared in their place since the JavaScript check, so it never opens or follows something it did not create; sockets, FIFOs and unknown entries are reported back to JavaScript, which rejects them with the same SystemErrors as before; relative link targets are made absolute lexically as path.resolve() does. cpSync keeps merging into existing directories, skipping special files and canonicalizing link targets. 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' or 'copyfile'/'mkdir' as the syscall. Signed-off-by: Shelley Vohr <shelley.vohr@gmail.com>
Reviewers
At least 1 approving review is required to merge this pull request.
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.
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, no mode flags for copyFile(), permission model off); the request creates every destination directory itself with mkdir() and fails with EEXIST if anything has appeared in its place since the JavaScript check, so it never writes through a late symbolic link; 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), and relative link targets are made absolute lexically as path.resolve() does (cpSync canonicalizes 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) test-fs-cp-unreadable-directory.mjs (aborts on main for cpSync) test-fs-cp-async-destination-appears-late.mjs and test-fs-cp-async-symlink-targets.mjs; 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.