fs: fix rmsync error swallowing

[Linkgoron]

Fix rmsync swallowing errors instead of throwing them. Note that I've also changed a few places that retry errors, to align rmsync to rm.

If anyone has a better idea for a test, I'd be happy to implement it - as changing permissions feels like it might cause errors if the file somehow survives.

fixes: #38683
fixes: #34580

[nodejs-github-bot]

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

[RaisinTen]

I had a feeling the Windows tests would fail. Not quite sure why they are failing though. I had a PR open that has a similar test using the chmod + rm combo in order to fix another rm bug and even that fails: #36815.

[Linkgoron]

I had a feeling the Windows tests would fail. Not quite sure why they are failing though. I had a PR open that has a similar test using the chmod + rm combo in order to fix another rm bug and even that fails: #36815.

Yep, and it failed to remove the file which caused other tests to fail as well

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[aduh95]

Windows CI's still failing.

[Linkgoron]

Windows CI's still failing.

ARM is also consistently failing

[Linkgoron]

@Trott do you think that there's any chance that this fix uncovered some things that didn't really work on Windows? Some calls to rmSync that were errored but didn't throw?

[Trott]

@Trott do you think that there's any chance that this fix uncovered some things that didn't really work on Windows? Some calls to rmSync that were errored but didn't throw?

¯\(ツ)/¯

@cjihrig

[cjihrig]

Sorry, but I'm also ¯\(ツ)/¯ here, especially on Windows.

[Linkgoron]

Looks like rmsync failed for every test that didn't close all of its open fds. I think that I've caught all of them.

[Linkgoron]

Given all of the issues encountered, and assuming they were actually fixed, I think that it makes sense to mark this as semver-major.

[nodejs-github-bot]

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

[jasnell]

Landed in f9447b7

[richardlau]

Is it possible this has caused the parallel/test-fs-writefile-with-fd Windows failures in today's daily master CI and other recent CIs?
https://ci.nodejs.org/job/node-test-binary-windows-js-suites/10075/#showFailuresLink

Can't clean tmpdir: C:\workspace\node-test-binary-windows-js-suites\node\test\.tmp.262
Files blocking: [ 'test.txt' ]

C:\workspace\node-test-binary-windows-js-suites\node\test\common\tmpdir.js:55
    throw e;
    ^

Error: ENOTEMPTY: directory not empty, rmdir '\\?\C:\workspace\node-test-binary-windows-js-suites\node\test\.tmp.262'
    at Object.rmdirSync (node:fs:1158:10)
    at _rmdirSync (node:internal/fs/rimraf:260:21)
    at rimrafSync (node:internal/fs/rimraf:193:7)
    at Object.rmSync (node:fs:1205:10)
    at rmSync (C:\workspace\node-test-binary-windows-js-suites\node\test\common\tmpdir.js:9:6)
    at process.onexit (C:\workspace\node-test-binary-windows-js-suites\node\test\common\tmpdir.js:40:5)
    at process.emit (node:events:406:35) {
  errno: -4051,
  syscall: 'rmdir',
  code: 'ENOTEMPTY',
  path: '\\\\?\\C:\\workspace\\node-test-binary-windows-js-suites\\node\\test\\.tmp.262'
}

[targos]

I also suspect this change.

[Linkgoron]

yes, it's 100% this change - the test opens test.txt but doesn't close the fd which causes errors on Windows when trying to remove the directory. I'm not sure how it passed CI. The test needs to be fixed to close the fds. Have there been other tests that also failed? In theory ARM tests should also exhibit errors, if I missed anything.

[richardlau]

Yeah, arm-fanned failed the same parallel/test-fs-writefile-with-fd test as it runs on an NFS-mounted FS: https://ci.nodejs.org/job/node-test-binary-arm-12+/11011/

Can't clean tmpdir: /home/iojs/tmp/.tmp.261
Files blocking: [ '.nfs00000000013426cf00004f89' ]
Note: ".nfs*" might be files that were open and unlinked but not closed.
See http://nfs.sourceforge.net/#faq_d2 for details.

/home/iojs/build/workspace/node-test-binary-arm/test/common/tmpdir.js:55
    throw e;
    ^

Error: ENOTEMPTY: directory not empty, rmdir '/home/iojs/tmp/.tmp.261'
    at Object.rmdirSync (node:fs:1158:10)
    at _rmdirSync (node:internal/fs/rimraf:260:21)
    at rimrafSync (node:internal/fs/rimraf:193:7)
    at Object.rmSync (node:fs:1205:10)
    at rmSync (/home/iojs/build/workspace/node-test-binary-arm/test/common/tmpdir.js:9:6)
    at process.onexit (/home/iojs/build/workspace/node-test-binary-arm/test/common/tmpdir.js:40:5)
    at process.emit (node:events:406:35) {
  errno: -39,
  syscall: 'rmdir',
  code: 'ENOTEMPTY',
  path: '/home/iojs/tmp/.tmp.261'
}

[Linkgoron]

fix here: #38820