Skip to content

fs: pass symlink type in cp when filter is provided - #62654

Open
shulaoda wants to merge 1 commit into
nodejs:mainfrom
shulaoda:04-10-fs_pass_symlink_type_in_cp_when_filter_is_provided
Open

fs: pass symlink type in cp when filter is provided#62654
shulaoda wants to merge 1 commit into
nodejs:mainfrom
shulaoda:04-10-fs_pass_symlink_type_in_cp_when_filter_is_provided

Conversation

@shulaoda

@shulaoda shulaoda commented Apr 9, 2026

Copy link
Copy Markdown

When fs.cp/fs.cpSync is called with both verbatimSymlinks: true and a filter function, directory symlinks are incorrectly created as file symlinks on Windows.

Root cause

fs.cp takes two code paths:

  • Without filter: the C++ fast path (cpSyncCopyDir) uses std::filesystem::copy_symlink() which preserves the symlink type automatically.
  • With filter: the JS fallback path calls symlinkSync(resolvedSrc, dest) without a type parameter. On Windows, symlinkSync tries to auto-detect the type by stat-ing the resolved target at the destination. During a recursive copy, the target directory may not yet exist at the destination (e.g. linked/ is copied before packages/ in alphabetical order), so stat fails and type defaults to 'file' creating a file symlink instead of a directory symlink.

Fix

Detect the symlink type from the source (which always exists) using internalModuleStat(src) and pass it explicitly to symlinkSync/symlink. The onLink function already computed srcIsDir for subdirectory validation but only after the early-return paths this change moves it before those returns and threads the derived symlinkType through to all symlinkSync/symlink call sites.

Both the sync (cp-sync.js) and async (cp.js) implementations are fixed.

Test

Added two test files that verify directory symlinks are preserved when copying with verbatimSymlinks: true and a filter function:

  • test-fs-cp-sync-verbatim-dir-symlinks-with-filter.mjs
  • test-fs-cp-async-verbatim-dir-symlinks-with-filter.mjs

Fixes: #62653

@shulaoda
shulaoda marked this pull request as ready for review April 9, 2026 16:25
@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Apr 9, 2026
@shulaoda
shulaoda force-pushed the 04-10-fs_pass_symlink_type_in_cp_when_filter_is_provided branch 2 times, most recently from 979dcbe to 386c58e Compare May 22, 2026 09:09

@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 request-ci Add this label to start a Jenkins CI on a PR. commit-queue Add this label to land a pull request using GitHub Actions. labels May 22, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 22, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@codecov

codecov Bot commented May 22, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 57.14286% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.33%. Comparing base (e6a8d06) to head (793de42).
⚠️ Report is 711 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/fs/cp/cp-sync.js 28.57% 5 Missing ⚠️
lib/internal/fs/cp/cp.js 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #62654      +/-   ##
==========================================
- Coverage   92.01%   90.33%   -1.69%     
==========================================
  Files         379      760     +381     
  Lines      166972   248526   +81554     
  Branches    25554    46903   +21349     
==========================================
+ Hits       153639   224496   +70857     
- Misses      13041    15452    +2411     
- Partials      292     8578    +8286     
Files with missing lines Coverage Δ
lib/internal/fs/cp/cp.js 88.86% <85.71%> (+5.06%) ⬆️
lib/internal/fs/cp/cp-sync.js 63.26% <28.57%> (+4.65%) ⬆️

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

@shulaoda
shulaoda force-pushed the 04-10-fs_pass_symlink_type_in_cp_when_filter_is_provided branch from 386c58e to 73d40d1 Compare May 22, 2026 13:49
@shulaoda

Copy link
Copy Markdown
Author

@mcollina Sorry, I had to amend the commit to get lint-commit-message CI to pass. Mind re-approving? Thanks! 🙏

@shulaoda
shulaoda requested a review from mcollina May 22, 2026 13:54
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels May 22, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator
Commit Queue failed
- Loading data for nodejs/node/pull/62654
✔  Done loading data for nodejs/node/pull/62654
----------------------------------- PR info ------------------------------------
Title      fs: pass symlink type in cp when filter is provided (#62654)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     shulaoda:04-10-fs_pass_symlink_type_in_cp_when_filter_is_provided -> nodejs:main
Labels     fs, needs-ci
Commits    1
 - fs: pass symlink type in cp when filter is provided
Committers 1
 - shulaoda <165626830+shulaoda@users.noreply.github.com>
PR-URL: https://github.com/nodejs/node/pull/62654
Fixes: https://github.com/nodejs/node/issues/62653
Reviewed-By: Aviv Keller <me@aviv.sh>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Stefan Stojanovic <stefan.stojanovic@janeasystems.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/62654
Fixes: https://github.com/nodejs/node/issues/62653
Reviewed-By: Aviv Keller <me@aviv.sh>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Stefan Stojanovic <stefan.stojanovic@janeasystems.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Thu, 09 Apr 2026 16:25:30 GMT
   ✔  Approvals: 4
   ✔  - Aviv Keller (@avivkeller): https://github.com/nodejs/node/pull/62654#pullrequestreview-4084591716
   ✔  - Juan José Arboleda (@juanarbol): https://github.com/nodejs/node/pull/62654#pullrequestreview-4084831882
   ✔  - Stefan Stojanovic (@StefanStojanovic): https://github.com/nodejs/node/pull/62654#pullrequestreview-4103501656
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/62654#pullrequestreview-4345294374
   ✘  10 GitHub CI job(s) failed:
   ✘    - github-actions: ACTION_REQUIRED
   ✘    - github-actions: ACTION_REQUIRED
   ✘    - github-actions: ACTION_REQUIRED
   ✘    - github-actions: ACTION_REQUIRED
   ✘    - github-actions: ACTION_REQUIRED
   ✘    - github-actions: ACTION_REQUIRED
   ✘    - github-actions: ACTION_REQUIRED
   ✘    - github-actions: ACTION_REQUIRED
   ✘    - github-actions: ACTION_REQUIRED
   ✘    - github-actions: ACTION_REQUIRED
   ℹ  Last Full PR CI on 2026-05-22T13:09:00Z: https://ci.nodejs.org/job/node-test-pull-request/73616/
   ⚠  Commits were pushed after the last Full PR CI run:
   ⚠  - fs: pass symlink type in cp when filter is provided
- Querying data for job/node-test-pull-request/73616/
✔  Build data downloaded
   ✘  Last Jenkins CI still running
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/26292186844

When `fs.cp`/`fs.cpSync` is called with both `verbatimSymlinks: true`
and a `filter` function, directory symlinks were incorrectly created
as file symlinks on Windows.

Without a `filter`, cp takes the C++ fast path (`cpSyncCopyDir`) which
uses `std::filesystem::copy_symlink()` and preserves the symlink type
automatically. With a `filter`, the JS fallback calls `symlinkSync`/
`symlink` without a `type` argument. On Windows, that causes the type
to be auto-detected by stat-ing the resolved target at the destination,
but during a recursive copy the target directory may not exist yet at
the destination (e.g. `linked/` is copied before `packages/` in
alphabetical order). The stat fails and `type` falls back to `'file'`,
producing a file symlink in place of a directory symlink.

Detect the symlink type from the source (which always exists) via
`internalModuleStat(src)` and pass it explicitly to the `symlinkSync`/
`symlink` call sites. `onLink` already computed `srcIsDir` for
subdirectory validation; hoist that computation above the early-return
paths and thread the derived `symlinkType` through `copyLink` as well.
Both the sync (`cp-sync.js`) and async (`cp.js`) implementations are
fixed.

Add two regression tests that copy a tree containing a relative
directory symlink with `verbatimSymlinks: true` and a `filter`
function, then assert the destination link still resolves as a
directory.

Fixes: nodejs#62653
Signed-off-by: shulaoda <165626830+shulaoda@users.noreply.github.com>
@shulaoda
shulaoda force-pushed the 04-10-fs_pass_symlink_type_in_cp_when_filter_is_provided branch from 73d40d1 to 793de42 Compare June 25, 2026 23:19
@aduh95 aduh95 closed this Aug 10, 2026
@aduh95 aduh95 reopened this Aug 10, 2026
@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Aug 10, 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

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.

fs.cp creates file symlink instead of directory symlink on Windows when filter is provided

7 participants