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.51%. Comparing base (4f24aff) to head (f7000b5).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #61040      +/-   ##
==========================================
- Coverage   88.53%   88.51%   -0.03%     
==========================================
  Files         703      703              
  Lines      208546   208546              
  Branches    40217    40222       +5     
==========================================
- Hits       184634   184590      -44     
- Misses      15926    15968      +42     
- Partials     7986     7988       +2     
Files with missing lines Coverage Δ
src/node_file.cc 75.51% <100.00%> (-0.04%) ⬇️

... and 30 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
@Han5991 Han5991 force-pushed the fix/fs-rmsync-broken-symlink-61020 branch from 00ad33d to f7000b5 Compare December 14, 2025 00:20
@Renegade334 Renegade334 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. labels Dec 14, 2025
@Renegade334 Renegade334 requested a review from anonrig December 14, 2025 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. request-ci Add this label to start a Jenkins CI on a PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fs.rmSync() cannot remove broken symbolic link

5 participants