Skip to content

fs: implement copyFileSync in C++ - #65376

Open
anonrig wants to merge 1 commit into
nodejs:mainfrom
anonrig:cursor/copyfilesync-cpp-upstream-317c
Open

fs: implement copyFileSync in C++#65376
anonrig wants to merge 1 commit into
nodejs:mainfrom
anonrig:cursor/copyfilesync-cpp-upstream-317c

Conversation

@anonrig

@anonrig anonrig commented Aug 18, 2026

Copy link
Copy Markdown
Member

Move fs.copyFileSync() path validation, file URL conversion, NUL
checks, mode validation, permission-model checks, and the copy into a
dedicated C++ binding.

JavaScript only does VFS dispatch (still before validation). The copy
itself uses uv_fs_copyfile, so COPYFILE_EXCL / FICLONE /
FICLONE_FORCE, mode/timestamp preservation, and UV error shapes
(syscall: 'copyfile', path / dest) stay the same.

This also fixes a crash in the C++ FileURLToPath helper:
ERR_INVALID_FILE_URL_HOST used a format string without %s, which
aborted on file URLs with a hostname. That path is now reachable from
copyFileSync.

Performance

Same-binary comparison of the old JS getValidatedPath +
binding.copyFile path vs binding.copyFileSync (n=10000):

case old C++ change
valid copy 64.7 µs/op 60.6 µs/op ~6% faster
missing src (ENOENT) 4.64 µs/op 4.29 µs/op ~7% faster
invalid type 8.31 µs/op 2.68 µs/op ~68% faster
file URL 61.0 µs/op 60.3 µs/op ~1% faster

benchmark/fs/bench-copyFileSync.js (n=10000) is dominated by
uv_fs_copyfile I/O on valid and by ENOENT on invalid; those
cases stay within a few percent of the old path.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Tests
  • test/parallel/test-fs-copyfile.js (Buffer, Uint8Array, file: URL, non-file URL, encoded slashes, file URL host)
  • test/parallel/test-fs-copyfile-respect-permissions.js
  • test/parallel/test-fs-null-bytes.js
  • test/parallel/test-fs-error-messages.js
  • test/parallel/test-permission-fs-read.js / test-permission-fs-write.js
  • VFS copyFileSync tests

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/url

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Aug 18, 2026
@cursor
cursor Bot force-pushed the cursor/copyfilesync-cpp-upstream-317c branch from e13ed43 to 8170ff5 Compare August 18, 2026 12:47
Move path validation, file URL conversion, NUL checks, mode
validation, permission checks, and the copy into a dedicated
C++ binding so fs.copyFileSync() no longer goes through
getValidatedPath() in JavaScript.

VFS dispatch stays in JS and still runs before validation.
The copy itself uses uv_fs_copyfile, so flags, mode/timestamp
preservation, and UV error shapes stay the same.

Also fix FileURLToPath aborting on file URLs with a hostname
because the ERR_INVALID_FILE_URL_HOST format string lacked %s.

Signed-off-by: Yagiz Nizipli <yagiz@nizipli.com>
@cursor
cursor Bot force-pushed the cursor/copyfilesync-cpp-upstream-317c branch from 8170ff5 to e4dc3a1 Compare August 18, 2026 13:11
@codecov

codecov Bot commented Aug 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 72.56637% with 31 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.12%. Comparing base (cf30b2e) to head (e4dc3a1).
⚠️ Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
src/node_file.cc 72.07% 10 Missing and 21 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #65376      +/-   ##
==========================================
+ Coverage   90.11%   90.12%   +0.01%     
==========================================
  Files         752      752              
  Lines      251861   251968     +107     
  Branches    47365    47396      +31     
==========================================
+ Hits       226955   227082     +127     
+ Misses      16238    16203      -35     
- Partials     8668     8683      +15     
Files with missing lines Coverage Δ
lib/fs.js 98.36% <100.00%> (-0.01%) ⬇️
src/node_url.cc 80.97% <100.00%> (+3.26%) ⬆️
src/node_file.cc 73.96% <72.07%> (-0.27%) ⬇️

... and 32 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.

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++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants