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: stop fixing EPERM on Windows #38810

Closed

Conversation

RaisinTen
Copy link
Contributor

@RaisinTen RaisinTen commented May 25, 2021

Originally, when rimraf faced an EPERM on Windows, it would try to
change the file permissions of the entry and attempt to remove it
again. However, rm -rf doesn't change any file permissions, so rimraf
shouldn't either.

This change also updates test/common/tmpdir.js to repeatedly try
changing the file permissions of its contents to 0o777 when an EACCES or
an EPERM is faced while attempting to remove them.

Signed-off-by: Darshan Sen raisinten@gmail.com

@github-actions github-actions bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels May 25, 2021
@aduh95
Copy link
Contributor

aduh95 commented May 25, 2021

What should be the semverness of this change?

@RaisinTen
Copy link
Contributor Author

@aduh95

Since this is not an API change, I don't think it's going to be semver-major.

There is a behavioural change involved, so maybe this is a semver-minor?

However, I would consider this to be just a fix as this change aligns rm with what the docs already say:

modeled on the standard POSIX rm utility

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

LGTM, 69 lines of deleted code, nice!

nit: it reads clearer to me if we let the throw expression inside a if block, feel free to disagree oc

lib/internal/fs/rimraf.js Outdated Show resolved Hide resolved
lib/internal/fs/rimraf.js Outdated Show resolved Hide resolved
@Linkgoron
Copy link
Member

Linkgoron commented May 26, 2021

I think that it makes sense to add a test for this - or at least fix the tests that I added in the previous rmSync fix, so that they don't allow the folder to get deleted when trying to delete it (which I allowed, to get it to pass the tests). It probably makes sense to add a test to rm as well (my test only checks rmSync)

@nodejs-github-bot

This comment has been minimized.

@targos
Copy link
Member

targos commented May 26, 2021

@nodejs/fs

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@targos
Copy link
Member

targos commented May 28, 2021

Could it be that the CI runs of this PR are leaving broken directories on Windows hosts?

See #38811 (comment) / https://ci.nodejs.org/job/node-test-binary-windows-js-suites/10108/RUN_SUBSET=0,nodes=win10-COMPILED_BY-vs2019/console

14:56:28 > git clean -dffx 
14:56:29 warning: failed to remove test/.tmp.243/rm-6: Permission denied

@RaisinTen
Copy link
Contributor Author

@targos Yes I think one of the CI runs here is to blame, how do we fix it?

@targos
Copy link
Member

targos commented May 28, 2021

/cc @nodejs/build-infra ^

@richardlau
Copy link
Member

Could it be that the CI runs of this PR are leaving broken directories on Windows hosts?

See #38811 (comment) / https://ci.nodejs.org/job/node-test-binary-windows-js-suites/10108/RUN_SUBSET=0,nodes=win10-COMPILED_BY-vs2019/console

14:56:28 > git clean -dffx 
14:56:29 warning: failed to remove test/.tmp.243/rm-6: Permission denied

Yes I think one of the CI runs here is to blame, how do we fix it?

FYI Anyone in @nodejs/build can run https://ci.nodejs.org/job/git-clean-windows/ to clean out the git workspaces on the Windows hosts. (I've just done so.)

@nodejs-github-bot

This comment has been minimized.

test/parallel/test-fs-rm.js Outdated Show resolved Hide resolved
@nodejs-github-bot

This comment has been minimized.

@RaisinTen
Copy link
Contributor Author

@nodejs/build Sorry, I think I broke Windows again:

21:25:07 > git clean -dffx 
21:25:07 warning: failed to remove test/.tmp.242/rm-6: Permission denied

Refs: https://ci.nodejs.org/job/node-test-binary-windows-js-suites/10120/RUN_SUBSET=0,nodes=win10-COMPILED_BY-vs2019/console

Why don't we run https://ci.nodejs.org/job/git-clean-windows/ at the end of every CI run on the Windows hosts?

test/parallel/test-fs-rm.js Outdated Show resolved Hide resolved
@aduh95
Copy link
Contributor

aduh95 commented May 29, 2021

It looks like this also broke non-Windows CI machine setups – all builds made after https://ci.nodejs.org/job/node-test-pull-request/38372/ are red, failing on test.parallel/test-fs-rm...

@RaisinTen
Copy link
Contributor Author

RaisinTen commented May 30, 2021

It looks like this also broke non-Windows CI machine setups – all builds made after https://ci.nodejs.org/job/node-test-pull-request/38372/ are red, failing on test.parallel/test-fs-rm...

That is right. 😕
Since one failure seems to trigger the rest of the tests in the file to fail, I think I'll move each chmod+rm test to a separate file and manually handle the deletion instead of leaving it to tmpdir.

@Linkgoron
Copy link
Member

Linkgoron commented May 30, 2021

Maybe we could change the tmpdir exit code to try to chmod(0o777) files that it couldn't delete, if it gets EPERM of EACCES and then let it try to delete the folders again.

@RaisinTen
Copy link
Contributor Author

Maybe we could change the tmpdir exit code to try to chmod(0o777) files that it couldn't delete, if it gets EPERM of EACCES and then let it try to delete the folders again.

@Linkgoron Updated tmpdir. PTAL.


if (errCode === 'EACCES' || errCode === 'EPERM') {
const surroundingDir = path.join(errPath, '..');
fs.chmodSync(surroundingDir, 0o777);
Copy link
Member

Choose a reason for hiding this comment

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

Do we need extras checks to make sure we're not changing access to a directory outside (e.g. the parent) of the tmpdir?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since err comes from rmSync(tmpPath);, the path in err will always be something inside tmpPath, so I don't think we need more checks.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@aduh95
Copy link
Contributor

aduh95 commented May 31, 2021

It might be just an unlucky coincidence, build-windows GH Actions is failing consistently in every PRs since 939a1f2.

https://github.com/nodejs/node/actions/workflows/build-windows.yml

@targos
Copy link
Member

targos commented Jun 16, 2021

Not necessarily him, but I would feel much more comfortable with this if it's approved by someone from the tooling team.

@RaisinTen
Copy link
Contributor Author

Not necessarily him, but I would feel much more comfortable with this if it's approved by someone from the tooling team.

👍

@nodejs/tooling Pinging again for review because a week has passed and we didn't get any review yet.

@RaisinTen RaisinTen added the review wanted PRs that need reviews. label Jun 17, 2021
@bcoe
Copy link
Contributor

bcoe commented Jun 18, 2021

@RaisinTen with regards to this change, my concern is that the rimraf logic we pulled in is fairly battle tested ... the fact that changing this behavior is now leaving uncleaned up files in our CI/CD is a canary in the coal mine, won't many people who are using fs.rmdir for similar cleanup tasks in their CI/CD begin also failing to cleanup files on Windows ...

Given that tests are one of the most common places where this helper is likely to be used, are we sure this is a change we want to take?

@isaacs
Copy link
Contributor

isaacs commented Jun 18, 2021

The semantics around removing read-only files differs between Windows and Posix.

rm -rf doesn't do this, because it is not needed on Posix systems. However, in order to replicate the Posix behavior on win32 systems in some situations, this workaround was necessary.

See: isaacs/rimraf#24 (comment)

Removing this will make it impossible for users to remove read-only files on Windows, which is allowed by posix rm -rf.

$ touch foo

$ chmod 0444 foo

$ ls -laF
total 0
drwxr-xr-x    3 isaacs  staff    96 Jun 18 09:00 ./
drwxr-xr-x  144 isaacs  staff  4608 Jun 18 08:50 ../
-r--r--r--    1 isaacs  staff     0 Jun 18 09:00 foo

$ rm -rf foo

$ ls -laF
total 0
drwxr-xr-x    2 isaacs  staff    64 Jun 18 09:00 ./
drwxr-xr-x  144 isaacs  staff  4608 Jun 18 08:50 ../

$ mkdir dir

$ chmod 0555 dir

$ ls -laF dir
total 0
dr-xr-xr-x  2 isaacs  staff  64 Jun 18 09:02 ./
drwxr-xr-x  3 isaacs  staff  96 Jun 18 09:02 ../

$ rm -rf dir

$ ls -laF dir
ls: dir: No such file or directory

The divergence between Posix and this rimraf implementation on windows, is that on Posix systems, while you can delete read-only files and directories, you may not delete the contents of a read-only directory. I would perhaps be +1 on replicating that behavior, however, the expectation of being able to remove directories on Windows (eg, by deleting them in Explorer) is that a read-only folder may be removed if the user has permission to change its read-only status, and its contents may be removed as well. Thus, the decision was made to keep the workaround as it is.

tl;dr - If you remove this EPERM workaround, you will make a lot of windows users unable to delete .git folders, and they will be unhappy about it.

@bcoe
Copy link
Contributor

bcoe commented Jun 18, 2021

@RaisinTen @isaacs perhaps rather than landing this change, we could land a test that demonstrates initializing an empty git repo, and then uses rm to delete it? verify this edge case.

And then add a comment to our implementation which explains @isaacs' reasoning in detail.

@isaacs
Copy link
Contributor

isaacs commented Jun 18, 2021

What problem is this patch aiming to solve?

If there isn't a substantive complaint about the functionality as it is, it seems to me like it's changing code to match the documentation, when we ought to be updating the documentation to match what the code has been doing without complaint for 8 years.

Copy link
Contributor

@boneskull boneskull left a comment

Choose a reason for hiding this comment

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

I'm -1 on this. It's going to break users for reasons of cross-platform consistency where no such consistency exists in the first place. Better to update docs.

@RaisinTen
Copy link
Contributor Author

If we want to chmod() and retry deleting folders on Windows when an EPERM occurs, shouldn't we do it from inside rmdir() instead of rimraf? Why keep rm() and rmdir() inconsistent while removing a read only directory on Windows?

Also, shouldn't we depend on what Libuv does for these kinds of things instead of handling them from inside Node.js? I know that the implementation of a number of functions in src/win/fs.c of Libuv have been done without using the CRT functions to make things consistent between platforms (for example, unlink()). rmdir() in Libuv currently just calls the _wrmdir() CRT function which returns EACCES (incorrectly translated as EPERM) on read only directories. I think we should write the EPERM handling logic inside fs__rmdir() of src/win/fs.c.

Windows users can run chmod() before removing .git folders if they want. If it is a .git-only issue, shouldn't the issue be reported at Git?

The divergence between Posix and this rimraf implementation on windows, is that on Posix systems, while you can delete read-only files and directories, you may not delete the contents of a read-only directory.

Here too, an initial chmod would work. I have also considered adding a recursive option to fs.chmod(), so that it is easier to remove directories that have multiple levels of read only directories inside them.

@bcoe
Copy link
Contributor

bcoe commented Jun 22, 2021

If we want to chmod() and retry deleting folders on Windows when an EPERM occurs, shouldn't we do it from inside rmdir() instead of rimraf? Why keep rm() and rmdir() inconsistent while removing a read only directory on Windows?

On my mind when we worked on this feature, was that once you say rmdir/recursive a user is opting in to a somewhat more aggressive version of rmdir.

Please delete these files for me, and try hard to do so, it's okay I know what I'm doing.

It felt okay to have this, while having rmdir (without recursive) stay more conservative.

Windows users can run chmod() before removing .git

Many folks relying on rmdir recursive, or mdkir recursive are doing so in the context of unit tests, which they'll then run in a test matrix that includes a variety of platforms.

Someone writing a library that generates .git folders during testing doesn't want to add additional chmod behavior in their before logic solely for the benefit of Windows...

It creates cruft in tests, and is confusing (you would need to know about this edge case in folder permissions ahead of time to address the issue).

Shouldn't we depend on what Libuv does for these kinds of things instead of handling them from inside Node.js

I'm very supportive of removing functionality from Node.js if we can get it upstreamed in libuv 👍

I also think it's perfectly okay to have functionality in Node.js that improves platform ergonomics ... sometimes it's harder to fix things in a lower level, which can be relatively easy to fix a layer of abstraction up.

My two cents

If I'm understanding correctly, Node.js' own test suites started leaving .git folders around because we ourselves were relying on rimraf's fixing of EPERM on Windows? This is a strong indicator for me that I would consider this change breaking, and without a strong user case for doing so, I would avoid making this change.

Having said this, I'm very supportive of:

  • removing functionality from Node if behavior was added upstream to libuv.
  • signing off on a PR that explains the current behavior.
  • revisiting this decision if there are real world examples as to why the current behavior of rimraf is a bad idea.

@RaisinTen
Copy link
Contributor Author

I don't think this is worth the breakage anymore.

@RaisinTen RaisinTen closed this Mar 18, 2022
@RaisinTen RaisinTen deleted the fs/stop-fixing-EPERM-on-Windows branch March 18, 2022 06:31
@bcoe
Copy link
Contributor

bcoe commented Mar 20, 2022

@RaisinTen thanks for being understanding, regarding the tradeoffs discussed in this thread 👍

One thought, we should probably still add a test for deleting .git potentially -- if this is was one of the main motivators for the EPERM fix on Windows.

@RaisinTen RaisinTen removed the review wanted PRs that need reviews. label Mar 20, 2022
RaisinTen added a commit to RaisinTen/node that referenced this pull request Mar 20, 2022
Git for Windows creates read-only files inside the .git directory.
fs.rm() was built in a way, to work around any EPERM error that can
happen while deleting the .git directory by turning the directory
into a writable one. This change adds a regression test for that.

Refs: isaacs/rimraf#21
Refs: nodejs#38810 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>
@RaisinTen
Copy link
Contributor Author

@bcoe I've sent a PR to add the tests - #42410. :)

nodejs-github-bot pushed a commit that referenced this pull request Mar 28, 2022
Git for Windows creates read-only files inside the .git directory.
fs.rm() was built in a way, to work around any EPERM error that can
happen while deleting the .git directory by turning the directory
into a writable one. This change adds a regression test for that.

Refs: isaacs/rimraf#21
Refs: #38810 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: #42410
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
juanarbol pushed a commit that referenced this pull request Apr 4, 2022
Git for Windows creates read-only files inside the .git directory.
fs.rm() was built in a way, to work around any EPERM error that can
happen while deleting the .git directory by turning the directory
into a writable one. This change adds a regression test for that.

Refs: isaacs/rimraf#21
Refs: #38810 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: #42410
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
juanarbol pushed a commit to juanarbol/node that referenced this pull request Apr 5, 2022
Git for Windows creates read-only files inside the .git directory.
fs.rm() was built in a way, to work around any EPERM error that can
happen while deleting the .git directory by turning the directory
into a writable one. This change adds a regression test for that.

Refs: isaacs/rimraf#21
Refs: nodejs#38810 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: nodejs#42410
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
juanarbol pushed a commit that referenced this pull request Apr 6, 2022
Git for Windows creates read-only files inside the .git directory.
fs.rm() was built in a way, to work around any EPERM error that can
happen while deleting the .git directory by turning the directory
into a writable one. This change adds a regression test for that.

Refs: isaacs/rimraf#21
Refs: #38810 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: #42410
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
xtx1130 pushed a commit to xtx1130/node that referenced this pull request Apr 25, 2022
Git for Windows creates read-only files inside the .git directory.
fs.rm() was built in a way, to work around any EPERM error that can
happen while deleting the .git directory by turning the directory
into a writable one. This change adds a regression test for that.

Refs: isaacs/rimraf#21
Refs: nodejs#38810 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: nodejs#42410
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
juanarbol pushed a commit that referenced this pull request May 31, 2022
Git for Windows creates read-only files inside the .git directory.
fs.rm() was built in a way, to work around any EPERM error that can
happen while deleting the .git directory by turning the directory
into a writable one. This change adds a regression test for that.

Refs: isaacs/rimraf#21
Refs: #38810 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: #42410
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Jun 27, 2022
Git for Windows creates read-only files inside the .git directory.
fs.rm() was built in a way, to work around any EPERM error that can
happen while deleting the .git directory by turning the directory
into a writable one. This change adds a regression test for that.

Refs: isaacs/rimraf#21
Refs: #38810 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: #42410
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jul 11, 2022
Git for Windows creates read-only files inside the .git directory.
fs.rm() was built in a way, to work around any EPERM error that can
happen while deleting the .git directory by turning the directory
into a writable one. This change adds a regression test for that.

Refs: isaacs/rimraf#21
Refs: #38810 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: #42410
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jul 11, 2022
Git for Windows creates read-only files inside the .git directory.
fs.rm() was built in a way, to work around any EPERM error that can
happen while deleting the .git directory by turning the directory
into a writable one. This change adds a regression test for that.

Refs: isaacs/rimraf#21
Refs: #38810 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: #42410
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jul 31, 2022
Git for Windows creates read-only files inside the .git directory.
fs.rm() was built in a way, to work around any EPERM error that can
happen while deleting the .git directory by turning the directory
into a writable one. This change adds a regression test for that.

Refs: isaacs/rimraf#21
Refs: #38810 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: #42410
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
Git for Windows creates read-only files inside the .git directory.
fs.rm() was built in a way, to work around any EPERM error that can
happen while deleting the .git directory by turning the directory
into a writable one. This change adds a regression test for that.

Refs: isaacs/rimraf#21
Refs: nodejs/node#38810 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: nodejs/node#42410
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. needs-citgm PRs that need a CITGM CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.