Skip to content

Conversation

@Han5991
Copy link
Contributor

@Han5991 Han5991 commented Dec 13, 2025

Summary

Fix fs.rmSync() so it correctly removes broken symbolic links (and symlinks to
directories) instead of becoming a no-op or throwing ERR_FS_EISDIR.

Fixes: #61020

Details

rmSync was using std::filesystem::status(), which follows symlinks. For
broken symlinks this reports not_found, causing rmSync to return early
without unlinking the symlink itself. Switching to std::filesystem::symlink_status()
makes rmSync operate on the symlink entry, matching fs.rm() behavior.

Tests

  • ./node test/parallel/test-fs-rm.js

@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 Dec 13, 2025
@codecov
Copy link

codecov bot commented Dec 13, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.52%. Comparing base (81e05e1) to head (00ad33d).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #61040      +/-   ##
==========================================
- Coverage   88.53%   88.52%   -0.02%     
==========================================
  Files         703      703              
  Lines      208538   208546       +8     
  Branches    40224    40219       -5     
==========================================
- Hits       184629   184606      -23     
- Misses      15912    15958      +46     
+ Partials     7997     7982      -15     
Files with missing lines Coverage Δ
src/node_file.cc 75.47% <100.00%> (-0.08%) ⬇️

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

Copy link
Member

@Renegade334 Renegade334 left a comment

Choose a reason for hiding this comment

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

This looks like the correct approach to me, we use lstat-based checks in rimraf etc.

Could you please squash your commits, so that the final commit message is the fs: one, not the test: one?

@Han5991 Han5991 force-pushed the fix/fs-rmsync-broken-symlink-61020 branch from 6b1b314 to 00ad33d Compare December 13, 2025 15:15
@Han5991 Han5991 requested a review from Renegade334 December 13, 2025 15:15
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.

fs.rmSync() cannot remove broken symbolic link

3 participants