Skip to content

Conversation

mag123c
Copy link

@mag123c mag123c commented Jul 8, 2025

This PR fixes an issue where fs.readdirSync('M:\\') and related calls would fail with an ENOENT error on Windows when using subst drive roots.

Previously, Node.js added a trailing backslash (\) even when the input path was already a drive root (e.g., M:\ or \\?\M:\), resulting in invalid paths like M:\\\. This patch introduces a check to detect standard and namespaced drive root formats and avoid appending an extra slash in those cases.

The fix is Windows-specific and scoped only to the logic within fs.readdir.

Changes

  • Updated src/node_file.cc to detect and skip appending \ for drive root paths
  • Added regression test: test/parallel/test-fs-readdir-windows-subst.js
  • Covers subst-like paths with and without trailing slashes
  • Verifies no ENOENT error is thrown for valid drive roots

Context

Fixes: #58970

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Jul 8, 2025
Copy link

codecov bot commented Jul 8, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.05%. Comparing base (4bfcad1) to head (c97462b).
⚠️ Report is 732 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #58989      +/-   ##
==========================================
- Coverage   90.14%   90.05%   -0.09%     
==========================================
  Files         630      645      +15     
  Lines      186784   189130    +2346     
  Branches    36653    37089     +436     
==========================================
+ Hits       168377   170327    +1950     
- Misses      11205    11511     +306     
- Partials     7202     7292      +90     
Files with missing lines Coverage Δ
src/node_file.cc 75.90% <100.00%> (-1.08%) ⬇️

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

@mag123c mag123c force-pushed the fix-windows-subst-drive-issue branch 2 times, most recently from 1474ed1 to fcaee03 Compare July 9, 2025 00:31
@mag123c
Copy link
Author

mag123c commented Jul 10, 2025

I've force-pushed a fix (e.g. lint issue resolved).

When using fs.readdir() or fs.readdirSync() on subst drive root paths
such as M:\, Node.js was incorrectly adding a second backslash,
resulting in M:\\, which caused ENOENT errors on Windows.

This patch adds a safeguard to avoid appending a backslash if the path
already represents a drive root, including both standard (e.g. C:\)
and namespaced (e.g. \\?\C:\) formats.

Fixes: nodejs#58970
@mag123c mag123c force-pushed the fix-windows-subst-drive-issue branch from fcaee03 to c97462b Compare July 15, 2025 11:41
@mag123c
Copy link
Author

mag123c commented Jul 15, 2025

I apologize - the format-cpp check failed again. I've pushed another fix to address the formatting issue.
However, the test-macOS job is failing with a timeout in test-fs-promises-watch-iterator.js:

This appears to be an unrelated test, as:

  • My changes are Windows-specific and wrapped in #ifdef _WIN32

@StefanStojanovic StefanStojanovic added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 22, 2025
@github-actions github-actions bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Jul 22, 2025
Copy link
Contributor

Failed to start CI
   ⚠  No approving reviews found
   ✘  Refusing to run CI on potentially unsafe PR
https://github.com/nodejs/node/actions/runs/16441290676

Copy link
Contributor

@StefanStojanovic StefanStojanovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving to enable starting the CI. Still not approved to land AFAIC.

@StefanStojanovic StefanStojanovic added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. labels Jul 22, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 22, 2025
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@StefanStojanovic StefanStojanovic added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 21, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 21, 2025
@nodejs-github-bot
Copy link
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. 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.

ENOENT error when reading root of subst drive on Windows
3 participants