fs: fix rmSync to handle non-ASCII characters

[Yeaseen]

This is my first pull request here and I read the contribution guide and commit guide.

Update fs.rmSync in src/node_file.cc to properly handle file paths that include non-ASCII characters. This change prevents crashes and errors when attempting to delete files with international or special characters in their names.

Add a test in test/parallel/test-fs-rmSync-special-char.js to ensure that files with non-ASCII characters can be deleted without issues, covering cases that previously led to unexpected behavior or crashes on certain file systems.

Fixes: https://github.com/nodejs/node/issues/56049

For building the node and running the tests, I used:

./configure --ninja
ninja -C out/Release -j 20

make test-only

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 53.73134% with 31 lines in your changes missing coverage. Please review.

Project coverage is 89.16%. Comparing base (50d405a) to head (896e117). Report is 94 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_file.cc	51.56%	18 Missing and 13 partials :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #56117      +/-   ##
==========================================
- Coverage   89.22%   89.16%   -0.06%     
==========================================
  Files         663      665       +2     
  Lines      191974   192604     +630     
  Branches    36926    37055     +129     
==========================================
+ Hits       171286   171742     +456     
- Misses      13561    13664     +103     
- Partials     7127     7198      +71     

Files with missing lines	Coverage Δ
src/api/exceptions.cc	94.18% <100.00%> (ø)
src/node_file.cc	77.16% <51.56%> (-0.07%)	:arrow_down:

... and 48 files with indirect coverage changes

[jazelly]

Can you please update your first commit message to follow the guideline, i.e. each line needs to be below 72 chars. Also there are some linting issues in your js test.

[Yeaseen]

@jazelly I made the updates for the first commit message and linter issues. I think it should be fine now.

[Yeaseen]

@jazelly This time a linter issue: I didn't put a newline at the end of the test file. I am learning new things! Thanks for your understanding.

[Yeaseen]

@jazelly @joyeecheung It's updated based on the review.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[Yeaseen]

@joyeecheung Sorry for late update. It was a long winter vacation and my marriage ceremony. Hence the hiatus. I updated the changes you mentioned.

[joyeecheung]

LGTM, thanks for following up!

[nodejs-github-bot]

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

[joyeecheung]

This doesn't add new implementation of the conversion, it just moves existing conversion macros to somewhere earlier so that it can be used in an earlier function. If you want the conversion macros to use ToU8StringView() instead of via wide strings, I think it would be better if you open a separate PR to update these macros. For the purpose of fixing this bug, which has an increasing amount of reports being filed, I think what's done in this PR is adequate, and there is no need to block a bug fix because the existing helper it uses could've been worked better in a separate PR.

[jasnell]

I agree with @joyeecheung here. The conversion to use ToU8StringView() can be done separately.

[aduh95]

It looks like the CI is failing on Windows:

---
duration_ms: 198.975
exitcode: 1
severity: fail
stack: |-
  node:internal/assert/utils:281
      throw err;
      ^

  AssertionError [ERR_ASSERTION]: Error message should include the path treated as a directory
      at Object.<anonymous> (C:\workspace\node-test-binary-windows-js-suites\node\test\parallel\test-fs-rmSync-special-char.js:41:3)
      at expectedException (node:assert:808:17)
      at expectsError (node:assert:931:3)
      at Function.throws (node:assert:986:3)
      at Object.<anonymous> (C:\workspace\node-test-binary-windows-js-suites\node\test\parallel\test-fs-rmSync-special-char.js:37:8)
      at Module._compile (node:internal/modules/cjs/loader:1738:14)
      at Object..js (node:internal/modules/cjs/loader:1903:10)
      at Module.load (node:internal/modules/cjs/loader:1473:32)
      at Function._load (node:internal/modules/cjs/loader:1285:12)
      at TracingChannel.traceSync (node:diagnostics_channel:322:14) {
    generatedMessage: false,
    code: 'ERR_ASSERTION',
    actual: false,
    expected: true,
    operator: '=='
  }

  Node.js v24.0.0-pre
...

[lemire]

It looks like the CI is failing on Windows:

It seems to fail on the additional test that is being added.

@Yeaseen Did you test it under Windows?

[Yeaseen]

@lemire No. I will be trying this on Windows. I thought it was a simple fix.

[lemire]

@Yeaseen

I thought it was a simple fix.

Famous last words. 😄

[Yeaseen]

@joyeecheung I think I figured out the main problem: it was in src/api/exceptions.cc file. I tested both on Windows 11 and Ubuntu 22.04 There's some conflict, making the last few commits messy. Sorry about that.

[Yeaseen]

@lemire could you please review this and start a CI? I tested this locally, though. Thank you.

[nodejs-github-bot]

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

[Yeaseen]

There's a conflict because some other code already modified what I worked on. I am gonna do this task on a fresh PR