fs: honor dereference for symlinks nested in cpSync trees by christianaurichzm · Pull Request #65731 · nodejs/node

christianaurichzm · 2026-09-02T05:32:32Z

fs.cpSync(src, dest, { dereference: true, recursive: true }) copies symlinks found inside the tree as symlinks instead of copying their targets.

Reported in #59168 as a regression in 22.17 and still reproducible on main:

$ mkdir src linked && touch linked/foo && ln -s ../linked/foo src/bar
$ node -e "fs.cpSync('./src', './out', { dereference: true, recursive: true })"
$ ls -l out
lrwxrwxrwx bar -> /tmp/.../linked/foo

Cause

Without a filter, cp-sync.js hands the directory tree to the native CopyDirRecursive() walker. Nested symlinks were always handled as symlinks there, even when dereference was enabled.

A symlink passed directly as src is unaffected because it is resolved before the native directory walk begins.

Fix

With dereference enabled, nested symlinks that resolve to directories or regular files now use the corresponding native copy paths.

Unreachable targets are checked through uv_fs_stat() so filesystem errors are reported correctly across platforms.

Existing destinations keep the JavaScript walker's force and errorOnExist behavior.

This version is rebased onto the shared CopyDirRecursive() walker introduced by #65488. The async fs.cp() native path remains unchanged.

Fixes: #59168

codecov · 2026-09-02T07:43:02Z

Codecov Report

❌ Patch coverage is 70.27027% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.17%. Comparing base (fd6682c) to head (d7ae5a4).
⚠️ Report is 32 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_file.cc	70.27%	5 Missing and 6 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #65731      +/-   ##
==========================================
+ Coverage   90.16%   90.17%   +0.01%     
==========================================
  Files         771      771              
  Lines      265445   265509      +64     
  Branches    50455    50482      +27     
==========================================
+ Hits       239329   239425      +96     
+ Misses      17056    17012      -44     
- Partials     9060     9072      +12

Files with missing lines	Coverage Δ
src/node_file.cc	`75.50% <70.27%> (+0.05%)`	⬆️

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

christianaurichzm · 2026-09-05T18:45:19Z

@codebytere, would you mind taking a look when you have a chance? Thanks!

codebytere

LGTM - ran the new test and checked parity with the JS walker locally (dest as dir / file / link under force, force: false, errorOnExist), all matches. couple of non-blocking notes inline.

codebytere · 2026-09-05T19:00:52Z

+        if (is_symlink && dereference) {
+          // Mirror the JavaScript walk: create the destination only when it
+          // does not exist, otherwise recurse into the existing path.
+          std::error_code dest_error;
+          const bool dest_exists =
+              std::filesystem::exists(dest_file_path, dest_error);
+          if (dest_error) {
+            env->ThrowStdErrException(dest_error, "cp", dest_str.c_str());
+            return false;
+          }
+          if (!dest_exists) {
+            std::filesystem::create_directory(dest_file_path, dest_error);
+            if (dest_error) {
+              env->ThrowStdErrException(dest_error, "cp", dest_str.c_str());
+              return false;
+            }
+          }
+        } else {
+          std::filesystem::create_directory(dest_file_path);
+        }


(Non-blocking) std::filesystem::create_directory(dest_file_path, ec) already treats an existing directory (or link to one) as success and reports EEXIST for anything else, so i think this whole block can be that one call plus the if (ec) throw, and the unchecked create_directory() in the else arm could take the same form while we're here - that one still goes through the throwing overload.

I tried simplifying this to std::filesystem::create_directory(dest_file_path, dest_error), but it changes the existing destination-file case from ENOTDIR to EEXIST, while the JS walker returns ENOTDIR. So I kept the current branch to preserve parity. Thanks for the suggestion!

codebytere · 2026-09-05T19:00:52Z

+        if (is_symlink && dereference) {
+          // Only a dereferenced link reaches this branch as a link, so what an
+          // occupied destination means here is settled the way the JavaScript
+          // walk settles it: replaced under force, left untouched otherwise.
+          // Replacing an existing destination unlinks the entry first, which is
+          // what keeps an existing link there from being written through.
+          std::error_code dest_error;
+          const bool dest_exists =
+              std::filesystem::exists(dest_file_path, dest_error);
+          if (dest_error) {
+            env->ThrowStdErrException(dest_error, "cp", dest_str.c_str());
+            return false;
+          }
+
+          if (dest_exists) {
+            if (!force) {
+              if (error_on_exist) {
+                THROW_ERR_FS_CP_EEXIST(
+                    isolate,
+                    "[ERR_FS_CP_EEXIST]: Target already exists: "
+                    "cp returned EEXIST (%s already exists)",
+                    dest_file_path);
+                return false;
+              }
+              continue;
+            }
+
+            std::filesystem::remove(dest_file_path, dest_error);
+            if (dest_error) {
+              env->ThrowStdErrException(dest_error, "cp", dest_str.c_str());
+              return false;
+            }
+          }
+        }


(Non-blocking) the !force half of this duplicates what file_copy_opts already does (skip_existing / the file_exists -> ERR_FS_CP_EEXIST mapping below), so the part that's new is the remove() under force. That one matters beyond dereferenced links though: a plain file copied over an existing dest symlink is written through the link by copy_file(overwrite_existing) today, where the JS walker unlinks and replaces it. i'd either do the remove for every regular-file copy under force or leave it out here and we fix the write-through separately.

Removing the !force branch changes the destination-directory case: std::filesystem::copy_file() returns EINVAL, while the JS walker leaves the destination directory untouched. The guard is still needed for parity here.

christianaurichzm · 2026-09-06T16:41:38Z

@jasnell, since you had looked at the original regression in #59168, could you take a look at this fix when you get a chance? If it looks good, could you also kick off the Jenkins CI? Thanks!

When no filter is given, cpSync copies directory contents in C++. That loop recreated every symlink it found, consulting dereference only for the subdirectory-of-self guards, so a symlink nested in the tree was copied as a link even with dereference set. Only a symlink passed as src was dereferenced, because that one is resolved by stat() in JavaScript before the C++ copy starts. The directory and regular file branches already follow symlinks, so links that resolve to those types can fall through to them. A link whose target cannot be reached has nothing to copy, and uv_fs_stat() reports the underlying filesystem error. When following a link, preserve the existing force and errorOnExist behavior for occupied destinations. Under force, remove an existing destination first so copying does not write through a destination symlink. Signed-off-by: Christian Aurich <christian.aurichzm@gmail.com>

christianaurichzm · 2026-09-09T23:52:35Z

Rebased onto current main after #65488 and ported the fix to the shared CopyDirRecursive() walker.

The regression test still fails on main and passes here. I also reran all 88 test-fs-cp-* tests plus parallel, sequential, and es-module; formatting and lint checks are clean.

Re-requesting review since the implementation changed during the rebase. If this looks good, could someone also add request-ci for a fresh Jenkins run? Thanks!

jasnell · 2026-09-10T00:00:59Z

@christianaurichzm ... thanks for the ping. generally LGTM but I'm going to take another read through before signing off.

nodejs-github-bot · 2026-09-11T14:38:45Z

CI: https://ci.nodejs.org/job/node-test-pull-request/77363/

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to file-system APIs and the fs module. needs-ci PRs that need a full CI run. labels Sep 2, 2026

christianaurichzm force-pushed the fs-cp-dereference branch from 86f460c to 346ed1c Compare September 2, 2026 05:44

codebytere approved these changes Sep 5, 2026

View reviewed changes

christianaurichzm force-pushed the fs-cp-dereference branch from 346ed1c to d7ae5a4 Compare September 9, 2026 23:50

christianaurichzm requested a review from codebytere September 9, 2026 23:52

jasnell approved these changes Sep 10, 2026

View reviewed changes

panva added author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Sep 11, 2026

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 11, 2026

Navigation Menu

Uh oh!

Uh oh!

fs: honor dereference for symlinks nested in cpSync trees - #65731

fs: honor dereference for symlinks nested in cpSync trees#65731
christianaurichzm wants to merge 1 commit into
nodejs:mainfrom
christianaurichzm:fs-cp-dereference

christianaurichzm commented Sep 2, 2026 •

edited

Loading

Uh oh!

codecov Bot commented Sep 2, 2026 •

edited

Loading

Uh oh!

christianaurichzm commented Sep 5, 2026

Uh oh!

codebytere left a comment

Uh oh!

codebytere Sep 5, 2026

Uh oh!

christianaurichzm Sep 5, 2026

Uh oh!

codebytere Sep 5, 2026 •

edited

Loading

Uh oh!

christianaurichzm Sep 5, 2026

Uh oh!

christianaurichzm commented Sep 6, 2026

Uh oh!

christianaurichzm commented Sep 9, 2026

Uh oh!

jasnell commented Sep 10, 2026

Uh oh!

nodejs-github-bot commented Sep 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Uh oh!

Conversation

christianaurichzm commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Cause

Fix

Uh oh!

codecov Bot commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

christianaurichzm commented Sep 5, 2026

Uh oh!

codebytere left a comment

Choose a reason for hiding this comment

Uh oh!

codebytere Sep 5, 2026

Choose a reason for hiding this comment

Uh oh!

christianaurichzm Sep 5, 2026

Choose a reason for hiding this comment

Uh oh!

codebytere Sep 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christianaurichzm Sep 5, 2026

Choose a reason for hiding this comment

Uh oh!

christianaurichzm commented Sep 6, 2026

Uh oh!

christianaurichzm commented Sep 9, 2026

Uh oh!

jasnell commented Sep 10, 2026

Uh oh!

nodejs-github-bot commented Sep 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

christianaurichzm commented Sep 2, 2026 •

edited

Loading

codecov Bot commented Sep 2, 2026 •

edited

Loading

codebytere Sep 5, 2026 •

edited

Loading