fs: pass symlink type in cp when filter is provided

Project: nodejs/node | Language: JavaScript | Concepts: error-handling, testing-strategy | Difficulty: mid
Link: https://github.com/nodejs/node/pull/62654

The problem

When using fs.cp or fs.cpSync with both verbatimSymlinks: true and a filter function on Windows, directory symlinks were incorrectly copied as file symlinks. This meant that a symlink pointing to a directory would be created as a symlink to a file, breaking expected behavior and causing errors when accessing the copied symlink as a directory.

Why it's non-trivial

A naive fix might simply copy the symlink as-is, relying on the destination to infer the symlink type. However, on Windows, the symlink type ('file' or 'dir') must be specified explicitly when creating a symlink. The fallback JS implementation tried to auto-detect the type by checking the destination, but during recursive copies, the destination might not exist yet, leading to incorrect type inference and broken symlinks.

The approach

The fix ensures that the correct symlink type is always passed to symlinkSync/symlink by explicitly determining if the source is a directory or file, and then passing this information to the symlink creation function.

1. Explicitly determine the source type and pass it to symlink creation:

+  const srcIsDir = fsBinding.internalModuleStat(src) === 1;
+  const symlinkType = srcIsDir ? 'dir' : 'file';
   if (!destStat) {
-    return symlinkSync(resolvedSrc, dest);
+    return symlinkSync(resolvedSrc, dest, symlinkType);
   }

This code checks if the source is a directory using fsBinding.internalModuleStat(src) === 1. It then sets symlinkType accordingly and passes it to symlinkSync. This ensures the correct type is used, even if the destination does not exist yet.

2. Pass the symlink type through helper functions:

-  return copyLink(resolvedSrc, dest);
+  return copyLink(resolvedSrc, dest, symlinkType);

The helper function copyLink now takes the symlinkType parameter, ensuring the type information is preserved through the call chain.

3. Update the helper function to use the type:

-function copyLink(resolvedSrc, dest) {
+function copyLink(resolvedSrc, dest, symlinkType) {
   unlinkSync(dest);
-  return symlinkSync(resolvedSrc, dest);
+  return symlinkSync(resolvedSrc, dest, symlinkType);
 }

The copyLink function now always passes the explicit symlink type when creating the link.

4. Mirror these changes in the async implementation:

The async path (cp.js) received the same updates as the sync path, ensuring consistency between both implementations.

5. Add regression tests:

Two new test files were added to verify the fix for both async and sync APIs:

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

Each test:

• Sets up a directory symlink in the source.
• Copies it with verbatimSymlinks: true and a filter.
• Asserts that the copied symlink is still a directory symlink and works as expected.

Example from the test:

assert.ok(statSync(destSymlink).isDirectory(), 'symlink target should be accessible as a directory');
assert.deepStrictEqual(readdirSync(destSymlink), ['index.js']);

This confirms the symlink behaves as a directory after copying.

From the review thread

No notable review discussion on this PR.

Takeaways

• Always explicitly specify required metadata (like symlink type) when the platform requires it, rather than relying on auto-detection that may fail in edge cases.
• When fixing a bug, add regression tests that precisely capture the scenario that was broken to prevent future regressions.
• Ensure consistency between sync and async code paths to avoid subtle discrepancies in behavior.