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

[lolbinarycat]

fixes #127576

try-job: test-various

[rustbot]

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-]

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

[tgross35]

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]

r? libs-api

[tgross35]

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

r? libs-api

[Amanieu]

🤔 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]

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]

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-]

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.

[lolbinarycat]

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

1. inside the loop, we need to continue on ENOENT, not ignore it
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.

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

[tbu-]

1. 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.

[lolbinarycat]

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-]

Suggested change

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

[tbu-]

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

[jieyouxu]

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?

[jieyouxu]

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...)

[ChrisDenton]

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.

[Noratrieb]

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

[ChrisDenton]

I do not think that's better, lol.

[jieyouxu]

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.

[Amanieu]

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.

[lolbinarycat]

@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.

[Amanieu]

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

[jieyouxu]

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]

@rustbot review

[jieyouxu]

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]

@bors r-

[ChrisDenton]

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.

[lolbinarycat]

@rustbot review

[tgross35]

@bors r=Amanieu

[bors]

📌 Commit 736f773 has been approved by Amanieu

It is now in the queue for this repository.