fix: fs::remove_dir_all: treat internal ENOENT as success #127623

lolbinarycat · 2024-07-11T19:53:20Z

try-job: test-various

rustbot · 2024-07-11T19:53:27Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @cuviper (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

tbu- · 2024-07-12T01:22:51Z

I think there's also the fallback implementation which also needs this treatment.

tgross35 · 2024-07-12T21:22:44Z

This changes behavior of a user-visible interface, and has an ACP open at rust-lang/libs-team#410.

@rustbot label +S-waiting-on-ACP +T-libs-api -T-libs

cuviper · 2024-07-12T21:33:38Z

r? libs-api

tgross35 · 2024-07-12T21:37:52Z

🤔 does libs-api not go through triagebot.toml? Mara removed herself from the libs rotation recently.

r? libs-api

Amanieu · 2024-07-12T21:45:47Z

🤔 does libs-api not go through triagebot.toml? Mara removed herself from the libs rotation recently.

libs-api uses the team member list directly. triagebot.toml is only used for the initial assignment.

tgross35 · 2024-07-12T21:48:55Z

I stand corrected about the ACP, that is for a superset of this change so this PR alone shouldn't be blocked.

@rustbot author

rustbot · 2024-07-13T17:40:24Z

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

tbu- · 2024-07-13T21:09:05Z

library/std/src/sys/pal/unix/fs.rs

+    fn is_enoent(result: &io::Result<()>) -> bool {
+        if let Err(err) = result
+            && matches!(err.raw_os_error(), Some(libc::ENOENT))
+        {
+            true
+        } else {
+            false
+        }
+    }


Suggested change

fn is_enoent(result: &io::Result<()>) -> bool {

if let Err(err) = result

&& matches!(err.raw_os_error(), Some(libc::ENOENT))

{

true

} else {

false

}

}

fn ignore_enoent(result: io::Result<()>) -> io::Result<()> {

if let Err(err) = &result && err.raw_os_error() == Some(libc::ENOENT) {

Ok(())

} else {

result

}

}

With this function, you can simply wrap the relevant function calls in ignore_enoent.

this seems clever, but it doesn't work for a few reasons:

inside the loop, we need to continue on ENOENT, not ignore it

there are lots of function calls, and almost all of them need to ignore ENOENT. if someone adds a new unlinkat call in the future, they will probably also need to wrap it, and if they don't, it will introduce a sneaky regression.

using try blocks doesn't have either of these issues.

inside the loop, we need to continue on ENOENT, not ignore it

As far as I can tell, this actually works with the function. It'd treat removal failed with ENOENT the same as successful deletion and continue into the next loop.

2. there are lots of function calls, and almost all of them need to ignore ENOENT. if someone adds a new unlinkat call in the future, they will probably also need to wrap it, and if they don't, it will introduce a sneaky regression.

I actually feel the opposite way, it's better if we annotate all the places where we treat ENOENT as not an error rather than treating all possible ENOENT in this function as non-errors.

E.g. the recursive call does not need to ignore ENOENT as far as I can tell, but using try blocks will silence any errors that come from that.

E.g. the recursive call does not need to ignore ENOENT as far as I can tell, but using try blocks will silence any errors that come from that.

actually, maybe we should ignore the error in the recursive call, but not ignore any ENOENT errors outside of the for loop, as top-level ENOENT errors should be returned (currently this happens in remove_dir_all_modern).

tbu- · 2024-07-13T21:09:29Z

library/std/src/sys/pal/unix/fs.rs

+            false
+        }
+    }
+
    fn remove_dir_all_recursive(parent_fd: Option<RawFd>, path: &CStr) -> io::Result<()> {
        // try opening as directory
        let fd = match openat_nofollow_dironly(parent_fd, &path) {


Suggested change

let fd = match openat_nofollow_dironly(parent_fd, &path) {

let fd = match ignore_enoent(openat_nofollow_dironly(parent_fd, &path)) {

tbu- · 2024-07-13T21:11:47Z

library/std/src/sys/pal/unix/fs.rs

-                    remove_dir_all_recursive(Some(fd), child_name)?;
-                }
-                Some(false) => {
-                    cvt(unsafe { unlinkat(fd, child_name.as_ptr(), 0) })?;


ignore_enoent(cvt(unsafe { unlinkat(fd, child_name.as_ptr(), 0) }))?;

jieyouxu · 2024-07-14T07:05:39Z

tests/run-make/remove-dir-all-race/rmake.rs

@@ -0,0 +1,37 @@
+use run_make_support::{


This test slightly concerns me, as in is this a non-deterministic test that usually fails or usually passes, but we can't guarantee it to be reproducible?

This will also need to run through various try jobs, including various cross-compilation scenarios.

Is this test meant to be reproducible across each platform? Are macOS/various linux/Windows behavior different with respect to remove_dir_all?

Also note that some of the CI runners are ran as root, which may cause behavioral differences versus running as non-root. (As in, even if it passes in CI does not guarantee it passes locally as non-root...)

I think to test this reliably it'd require a much larger directory structure and with many more threads. Even then it may not be 100% reliable.

if we make the test platform specific we may be able to do some LD_PRELOAD fault injection? not sure if that's better^^

I do not think that's better, lol.

I think @jieyouxu is suggesting that it may be better to have no test than a potentially flaky test? Because flaky tests can be hard to pin down and diagnose.

To be clear, I'm not advocating to have no tests, I'm just uneasy whenever I see a potentially flakey test that may potentially non-deterministically fail (whether in CI or locally). I'm not going to block this PR on this (I'm not a T-libs reviewer anyway) especially since I don't know of a better alternative way to deterministic test this.

I'm also somewhat uncomfortable with this test. I would prefer if the test didn't fail if it couldn't manage to cause a race.

@Amanieu is there another status a run-make test can have other than pass or fail? i would like to alert people if the test breaks.

Not that I know of. Either the test passes or it fails and blocks CI.

I'm also somewhat uncomfortable with this test. I would prefer if the test didn't fail if it couldn't manage to cause a race.

+1 to this. If this test absolutely has to be flaky and non-deterministic and has no other choice, then I would prefer it at least not fail if we couldn't manage to cause a race, because otherwise it can randomly block unrelated PRs on PR CI or full CI. It's still not ideal because then if this regresses, a PR-local try-job might pass, and so might its full CI test run, but then start failing in completely unrelated PRs.

Is there another status a run-make test can have other than pass or fail? i would like to alert people if the test breaks.

One possible idea is to make this a separate test suite called something like run-make-no-blocking which is exercised in a separate non-blocking CI job. This would require some significant testing infra support in terms of bootstrap and compiletest (something like a tool state check job?)... but then the feedback loop can be quite long. And you'd have to gather consensus from T-infra, which I don't think really makes sense for just this one test.

lolbinarycat · 2024-07-15T16:43:48Z

@rustbot review

Rollup of 6 pull requests Successful merges: - rust-lang#127623 (fix: fs::remove_dir_all: treat internal ENOENT as success) - rust-lang#128627 (Special case DUMMY_SP to emit line 0/column 0 locations on DWARF platforms.) - rust-lang#129187 (bootstrap: fix clean's remove_dir_all implementation) - rust-lang#129190 (Added f16 and f128 to tests/ui/consts/const-float-bits-conv.rs) - rust-lang#129231 (improve submodule updates) - rust-lang#129257 (Allow rust staticlib to work with MSVC's /WHOLEARCHIVE) r? `@ghost` `@rustbot` modify labels: rollup

…=Amanieu fix: fs::remove_dir_all: treat internal ENOENT as success fixes rust-lang#127576 try-job: test-various

Rollup of 6 pull requests Successful merges: - rust-lang#127623 (fix: fs::remove_dir_all: treat internal ENOENT as success) - rust-lang#128627 (Special case DUMMY_SP to emit line 0/column 0 locations on DWARF platforms.) - rust-lang#129187 (bootstrap: fix clean's remove_dir_all implementation) - rust-lang#129190 (Added f16 and f128 to tests/ui/consts/const-float-bits-conv.rs) - rust-lang#129231 (improve submodule updates) - rust-lang#129284 (rustdoc: animate the `:target` highlight) r? `@ghost` `@rustbot` modify labels: rollup

jieyouxu · 2024-08-20T18:59:09Z

Failed in #129304 (comment)

command: "C:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\test\\run-make\\remove-dir-all-race\\rmake.exe"
--- stderr -------------------------------
thread '<unnamed>' panicked at C:\a\rust\rust\tests\run-make\remove-dir-all-race\rmake.rs:20:45:
called `Result::unwrap()` on an `Err` value: Os { code: 5, kind: PermissionDenied, message: "Access is denied." }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Not sure if it's due to this PR or if it's due to some compiletest/bootstrap remove_dir_all issues.

jieyouxu · 2024-08-20T18:59:26Z

@bors r-

ChrisDenton · 2024-08-20T21:03:06Z

Not sure if it's due to this PR or if it's due to some compiletest/bootstrap remove_dir_all issues.

This is likely to be a pre-existing issue with the remove_dir_all impl. It was designed to be safe against concurrent access but it's not necessarily fully robust. I can look into but I'd suggest marking the test as ignore windows for now and merging because I suspect the fix will be non-trivial.

tests/run-make/remove-dir-all-race/rmake.rs

fixes rust-lang#127576 windows implementation still needs some work

lolbinarycat · 2024-08-22T18:19:57Z

@rustbot review

tgross35 · 2024-08-22T19:19:42Z

@bors r=Amanieu

bors · 2024-08-22T19:19:44Z

📌 Commit 736f773 has been approved by Amanieu

It is now in the queue for this repository.

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#127623 (fix: fs::remove_dir_all: treat internal ENOENT as success) - rust-lang#128876 (Ship MinGW-w64 runtime DLLs along with `rust-lld.exe` for `-pc-windows-gnu` targets) - rust-lang#129055 (Migrate `x86_64-fortanix-unknown-sgx-lvi` `run-make` test to rmake) - rust-lang#129386 (Use a LocalDefId in ResolvedArg.) - rust-lang#129400 (Update `compiler_builtins` to `0.1.120`) - rust-lang#129414 (Fix extern crates not being hidden with `doc(hidden)`) - rust-lang#129417 (Don't trigger refinement lint if predicates reference errors) - rust-lang#129433 (Fix a missing import in a doc in run-make-support) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#127623 - lolbinarycat:fix_remove_dir_all, r=Amanieu fix: fs::remove_dir_all: treat internal ENOENT as success fixes rust-lang#127576 try-job: test-various

rustbot assigned cuviper Jul 11, 2024

rustbot added A-run-make Area: port run-make Makefiles to rmake.rs O-unix Operating system: Unix-like S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 11, 2024

This comment has been minimized.

Sign in to view

lolbinarycat force-pushed the fix_remove_dir_all branch from a696f34 to 281d6da Compare July 11, 2024 20:00

This comment has been minimized.

Sign in to view

tgross35 mentioned this pull request Jul 12, 2024

Change std::fs::remove_dir_all to be idempotent rust-lang/libs-team#410

Closed

rustbot added S-waiting-on-ACP Status: PR has an ACP and is waiting for the ACP to complete. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 12, 2024

rustbot assigned m-ou-se and unassigned cuviper Jul 12, 2024

rustbot assigned Amanieu and unassigned m-ou-se Jul 12, 2024

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 12, 2024

This comment has been minimized.

Sign in to view

tbu- reviewed Jul 13, 2024

View reviewed changes

jieyouxu reviewed Jul 14, 2024

View reviewed changes

jieyouxu mentioned this pull request Aug 20, 2024

Rollup of 6 pull requests #129300

Closed

jieyouxu added a commit to jieyouxu/rust that referenced this pull request Aug 20, 2024

Rollup merge of rust-lang#127623 - lolbinarycat:fix_remove_dir_all, r…

d38d4e5

…=Amanieu fix: fs::remove_dir_all: treat internal ENOENT as success fixes rust-lang#127576 try-job: test-various

jieyouxu mentioned this pull request Aug 20, 2024

Rollup of 6 pull requests #129304

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 20, 2024

lolbinarycat force-pushed the fix_remove_dir_all branch from 5fc3993 to 5dbfdd6 Compare August 20, 2024 23:58

jieyouxu reviewed Aug 21, 2024

View reviewed changes

tests/run-make/remove-dir-all-race/rmake.rs Show resolved Hide resolved

ChrisDenton mentioned this pull request Aug 22, 2024

bootstrap: fix clean's remove_dir_all implementation #129187

Merged

fix: fs::remove_dir_all: treat ENOENT as success

736f773

fixes rust-lang#127576 windows implementation still needs some work

lolbinarycat force-pushed the fix_remove_dir_all branch from 5dbfdd6 to 736f773 Compare August 22, 2024 18:19

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 22, 2024

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 22, 2024

matthiaskrgr mentioned this pull request Aug 23, 2024

Rollup of 8 pull requests #129443

Merged

bors merged commit 370b326 into rust-lang:master Aug 23, 2024
6 checks passed

rustbot added this to the 1.82.0 milestone Aug 23, 2024

lolbinarycat mentioned this pull request Aug 24, 2024

guarantee that remove_dir will only return ErrorKind::NotFound if the file does not exist rust-lang/libs-team#433

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: fs::remove_dir_all: treat internal ENOENT as success #127623

fix: fs::remove_dir_all: treat internal ENOENT as success #127623

lolbinarycat commented Jul 11, 2024 •

edited by tgross35

Loading

rustbot commented Jul 11, 2024

This comment has been minimized.

This comment has been minimized.

tbu- commented Jul 12, 2024

tgross35 commented Jul 12, 2024

cuviper commented Jul 12, 2024

tgross35 commented Jul 12, 2024

Amanieu commented Jul 12, 2024

tgross35 commented Jul 12, 2024

rustbot commented Jul 13, 2024

This comment has been minimized.

tbu- Jul 13, 2024

lolbinarycat Jul 15, 2024

tbu- Jul 15, 2024

lolbinarycat Jul 15, 2024

tbu- Jul 13, 2024

tbu- Jul 13, 2024

jieyouxu Jul 14, 2024 •

edited

Loading

jieyouxu Jul 14, 2024

ChrisDenton Jul 14, 2024

Noratrieb Jul 14, 2024

ChrisDenton Jul 14, 2024

jieyouxu Jul 15, 2024

Amanieu Jul 20, 2024

lolbinarycat Jul 23, 2024

Amanieu Jul 29, 2024

jieyouxu Aug 16, 2024 •

edited

Loading

lolbinarycat commented Jul 15, 2024

jieyouxu commented Aug 20, 2024

jieyouxu commented Aug 20, 2024

ChrisDenton commented Aug 20, 2024 •

edited

Loading

lolbinarycat commented Aug 22, 2024

tgross35 commented Aug 22, 2024

bors commented Aug 22, 2024

	let fd = match openat_nofollow_dironly(parent_fd, &path) {
	let fd = match ignore_enoent(openat_nofollow_dironly(parent_fd, &path)) {

fix: fs::remove_dir_all: treat internal ENOENT as success #127623

fix: fs::remove_dir_all: treat internal ENOENT as success #127623

Conversation

lolbinarycat commented Jul 11, 2024 • edited by tgross35 Loading

rustbot commented Jul 11, 2024

This comment has been minimized.

This comment has been minimized.

tbu- commented Jul 12, 2024

tgross35 commented Jul 12, 2024

cuviper commented Jul 12, 2024

tgross35 commented Jul 12, 2024

Amanieu commented Jul 12, 2024

tgross35 commented Jul 12, 2024

rustbot commented Jul 13, 2024

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jieyouxu Jul 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jieyouxu Aug 16, 2024 • edited Loading

Choose a reason for hiding this comment

lolbinarycat commented Jul 15, 2024

jieyouxu commented Aug 20, 2024

jieyouxu commented Aug 20, 2024

ChrisDenton commented Aug 20, 2024 • edited Loading

lolbinarycat commented Aug 22, 2024

tgross35 commented Aug 22, 2024

bors commented Aug 22, 2024

lolbinarycat commented Jul 11, 2024 •

edited by tgross35

Loading

jieyouxu Jul 14, 2024 •

edited

Loading

jieyouxu Aug 16, 2024 •

edited

Loading

ChrisDenton commented Aug 20, 2024 •

edited

Loading