Some fs.cp* tests are constantly failing on Windows

[joyeecheung]

After the split of the monolithic test-fs-cp, it revealed the test cases that are constantly failing on Windows - these are likely regressions that went ignored when the test cases were buried in the monolith marked as flaky. Opening this issue to track the current failures in the CI before skipping them directly on Windows CI.

Another interesting point to consider - these tests are constantly failing in the daily CI run on the main branch and the v22.x-staging branch, but not on v20.x-staging e.g. see https://ci.nodejs.org/job/node-test-binary-windows-js-suites/36519/ - this indicates that the regressions happened in PRs backported to 22 but not 20.

From https://ci.nodejs.org/job/node-test-binary-windows-js-suites/36514/#showFailuresLink (the latest daily master job) - as noted in #59408 (comment) - I could reproduce the first three locally on Windows, though I can't reproduce the parallel/test-fs-cp-sync-unicode-folder-names that is crashing with 3221226505. @dario-piotrowicz is looking into reverting the recent fs.cp* changes to see if that would make the regressions go away.

parallel/test-fs-cp-sync-symlink-points-to-dest-error

---
duration_ms: 236.002
exitcode: 1
severity: flaky
stack: |-
  node:internal/modules/run_main:107
      triggerUncaughtException(
      ^

  AssertionError [ERR_ASSERTION]: Missing expected exception.
      at file:///C:/workspace/node-test-binary-windows-js-suites/node/test/parallel/test-fs-cp-sync-symlink-points-to-dest-error.mjs:17:8
      at ModuleJob.run (node:internal/modules/esm/module_job:371:25)
      at async onImport.tracePromise.__proto__ (node:internal/modules/esm/loader:683:26)
      at async asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:101:5) {
    generatedMessage: false,
    code: 'ERR_ASSERTION',
    actual: undefined,
    expected: { code: 'ERR_FS_CP_EINVAL' },
    operator: 'throws',
    diff: 'simple'
  }

  Node.js v25.0.0-pre
...

parallel/test-fs-cp-async-symlink-points-to-dest

---
duration_ms: 150.002
exitcode: 1
severity: flaky
stack: |-
  file:///c:/workspace/node-test-binary-windows-js-suites/node/test/parallel/test-fs-cp-async-symlink-points-to-dest.mjs:19
    assert.strictEqual(err.code, 'ERR_FS_CP_EINVAL');
                           ^

  TypeError: Cannot read properties of null (reading 'code')
      at file:///c:/workspace/node-test-binary-windows-js-suites/node/test/parallel/test-fs-cp-async-symlink-points-to-dest.mjs:19:26
      at c:\workspace\node-test-binary-windows-js-suites\node\test\common\index.js:472:15
      at node:fs:180:23
      at process.processTicksAndRejections (node:internal/process/task_queues:90:21)

  Node.js v25.0.0-pre
...

parallel/test-fs-cp-sync-error-on-exist

---
duration_ms: 231.019
exitcode: 1
severity: flaky
stack: |-
  node:internal/modules/run_main:107
      triggerUncaughtException(
      ^

  AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
  + actual - expected

    Comparison {
  +   code: ''
  -   code: 'ERR_FS_CP_EEXIST'
    }

      at file:///c:/workspace/node-test-binary-windows-js-suites/node/test/parallel/test-fs-cp-sync-error-on-exist.mjs:14:8
      at ModuleJob.run (node:internal/modules/esm/module_job:371:25)
      at async onImport.tracePromise.__proto__ (node:internal/modules/esm/loader:683:26)
      at async asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:101:5) {
    generatedMessage: true,
    code: 'ERR_ASSERTION',
    actual: Error: , The file exists. '\\?\c:\workspace\node-test-binary-windows-js-suites\node\test\.tmp.341\copy_%1\a\b'
        at copyDir (node:internal/fs/cp/cp-sync:145:22)
        at onDir (node:internal/fs/cp/cp-sync:137:10)
        at getStats (node:internal/fs/cp/cp-sync:68:12)
        at cpSyncFn (node:internal/fs/cp/cp-sync:58:10)
        at cpSync (node:fs:3123:3)
        at assert.throws.code (file:///c:/workspace/node-test-binary-windows-js-suites/node/test/parallel/test-fs-cp-sync-error-on-exist.mjs:15:9)
        at getActual (node:assert:584:5)
        at assert.throws (node:assert:732:24)
        at file:///c:/workspace/node-test-binary-windows-js-suites/node/test/parallel/test-fs-cp-sync-error-on-exist.mjs:14:8
        at ModuleJob.run (node:internal/modules/esm/module_job:371:25) {
      errno: 80,
      code: '',
      path: '\\\\?\\c:\\workspace\\node-test-binary-windows-js-suites\\node\\test\\.tmp.341\\copy_%1\\a\\b',
      syscall: 'cp'
    },
    expected: { code: 'ERR_FS_CP_EEXIST' },
    operator: 'throws',
    diff: 'simple'
  }

  Node.js v25.0.0-pre
...

parallel/test-fs-cp-sync-unicode-folder-names

---
duration_ms: 286.005
exitcode: 3221226505
severity: flaky
stack: ''
...

[dario-piotrowicz]

@joyeecheung thanks a lot for the ping 🙏

Sorry I haven't started trying to look into the revert yet, I can look as soon as I can in the next few days however I wanted to mention that I had some discussions with @anonrig (and @npaun who'd be happy to help with the cpSync issues) and the idea of trying to roll forward and trying to fix the newly surfaced issues in the new implementation was brought up as a potentially simpler and safer (given the amount of time that has passed from when the cpSync issues were introduced) solution compared to trying the revert strategy.

We should start exploring together that strategy this week, and of course we'll also keep in consideration the above failures.

That being said please let me know if trying to roll forward sounds like a strategy that you'd agree with. If not I am also totally happy to at least investigate and see if a simple-enough revert would at least be possible (since the feasibility of a revert is also in question) at the very least to keep all possible options open and compare them.

What do you think? Do you have any specific preference here?

(@anonrig and @npaun please feel free to chip in if you want 🙂)

[dario-piotrowicz]

cc. @jasnell

[jasnell]

I'm fine with rolling forward or reverting really. We just need to be very careful that rolling forward doesn't just replace one set of bugs for another, and I think we need to make sure we have adequate test coverage of the areas in question.

[dario-piotrowicz]

Yes we definitely agreed that regardless on the strategy that we take here, the test coverage of this area of the codebase needs to be expanded as part of the fix, thankfully for cpSync you've already created some known-issues tests (plus the legitimate failures above etc...), so we do already have a nice starting point I think/hope 😄

[joyeecheung]

I am fine either way as long as they get fixed - technically I don't feel very strongly about how or when they should be fixed myself, it's purely out of "it feels icky that there's a regression that went unnoticed and unfixed for months", but I am not affected, just assume that someone is.