Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fs: fix rmSync error code #57103

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fs: fix rmSync error code #57103

wants to merge 1 commit into from

Conversation

koplas
Copy link

@koplas koplas commented Feb 17, 2025

Return the correct error code, when a directory_not_empty error occurred.

Fixes: #57095

@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 Feb 17, 2025
Copy link
Member

@jazelly jazelly left a comment

Choose a reason for hiding this comment

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

LGTM. Can you please add a test

@koplas
Copy link
Author

koplas commented Feb 17, 2025

It doesn't seem very easy to test. This error only occurs for me if some files were added to the folder while RmSync was called. Reproducing this can result in flaky tests, as the error condition is timing-dependent.
I will take a look at how to reproduce this error reliably.

Update: There is already a test for this that is now failing. I will update the test.

Copy link

codecov bot commented Feb 17, 2025

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 89.06%. Comparing base (1d8593e) to head (2bff9c0).
Report is 34 commits behind head on main.

Files with missing lines Patch % Lines
src/node_file.cc 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #57103      +/-   ##
==========================================
- Coverage   89.09%   89.06%   -0.04%     
==========================================
  Files         665      665              
  Lines      193249   193306      +57     
  Branches    37231    37261      +30     
==========================================
- Hits       172175   172159      -16     
- Misses      13802    13826      +24     
- Partials     7272     7321      +49     
Files with missing lines Coverage Δ
src/node_file.cc 76.85% <0.00%> (-0.04%) ⬇️

... and 34 files with indirect coverage changes

@koplas
Copy link
Author

koplas commented Feb 18, 2025

The behavior of std::filesystem::remove_all seems to differ between operating systems. There is already a unique case for Windows. It may be necessary to introduce this for Linux and macOS.

The libcxx implementation ignores errors when deleting files that can not be deleted:
https://github.com/llvm/llvm-project/blob/251377c47d296000158347d6a1cba2e6b30132a3/libcxx/src/filesystem/operations.cpp#L886-L951
While the libstdc++ returns an error if a file cannot be deleted:
https://github.com/gcc-mirror/gcc/blob/b22f191b7c594b33fb4b4a07769dbf0ca45bc9e9/libstdc%2B%2B-v3/src/c%2B%2B17/fs_ops.cc#L1363-L1409

I will make the required changes and check how the test runs under Windows. But first, I will ask the libcxx folks if this is expected behavior or if it also needs fixing.

Return the correct error code, when a directory_not_empty error
occurred.

Fixes: nodejs#57095
@jazelly jazelly added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 19, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 19, 2025
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

@jazelly jazelly added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ci PRs that need a full CI run. labels Feb 21, 2025
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++. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fs.rmSync returns wrong error code for not empty directory
3 participants