fix: use remove_dir_all in cargo clean to resolve race condition

[trevyn]

Fixes #11441

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon.

Please see the contribution instructions for more information.

[weihanglo]

I believe it might introduce a bit more readdir calls underneath the hood, though I am fine with that. It was using remove_dir_all back to the day before this got merged.

[epage]

I believe it might introduce a bit more readdir calls underneath the hood, though I am fine with that. It was using remove_dir_all back to the day before this got merged.

I had vague memories of another PR related to it but it just touched test code (#11333).

The thing that doesn't quite feel right to me is that this feels like a hack: "If new contents are added to the "removed" directory, let's try removing the contents again". How many "agains" are sufficient?. This is inherently a race condition that we can't win theoretically though we might be able to make it work practically. I'm just wondering if its an effort that is worth it.

[weihanglo]

This is inherently a race condition that we can't win theoretically though we might be able to make it work practically.

My original point is back in the day Cargo didn't have a progress bar for cargo clean, it just remove_dir_all regardlessly. Despite that, I completely agree with you.

Just give a data from mine: I use cargo clean a lot everyday on macOS and never seen this issue.

[epage]

My original point is back in the day Cargo didn't have a progress bar for cargo clean, it just remove_dir_all regardlessly. Despite that, I completely agree with you.

It did a remove_dir_all on the root which then does the same walking that the progress bar code does.

This brings up an interesting thought: This change is effectively a remove_dir_all that does another remove_dir_all on failure. If we feel this is needed here, do any of the other cases where we call remove_dir_all need this error recovery?

[trevyn]

Apparently this is also hitting Windows: brson/wasm-opt-rs#116 (comment)

And the rabbit hole goes deep: rust-lang/rust#29497

Would it make sense to accept that removing contents and then calling remove_dir does not reliably remove a directory, and delegate any necessary complexity to std::fs::remove_dir_all?

In which case, paths::remove_dir_all is actually doing the same manual "remove contents then call fs::remove_dir" thing, instead of calling std::fs::remove_dir_all.

[ChrisDenton]

Note that, as far as I know, cargo clean was persistently failing in that case even with retries, where as std::fs::remove_dir_all succeeds first time. But unfortunately I've not had time to investigate the issue further. Also note that the Windows std code has improved a fair bit more since I last commented on rust-lang/rust#29497 (though there are still some more improvements to be made).

But yeah, remove_dir_all is highly specialized for different platforms (especially in light of CVE-2022-21658) whereas I guess the cargo code isn't?

[trevyn]

r? @weihanglo

[weihanglo]

But yeah, remove_dir_all is highly specialized for different platforms (especially in light of CVE-2022-21658) whereas I guess the cargo code isn't?

It is only a naive filesystem walk and remove. The purpose of Cargo doing this is to display the progress bar if needed and show info in verbose mode. There might be ways to improve but I don't see any alternative can remove Walkdir entirely.

---

@trevyn. The question epage raised might need a response to clarify in what situation a remove failure needs a recovery. I don't have a particular preference though.

This brings up an interesting thought: This change is effectively a remove_dir_all that does another remove_dir_all on failure. If we feel this is needed here, do any of the other cases where we call remove_dir_all need this error recovery?

[ChrisDenton]

@weihanglo the cargo way has better progress bar and error messaging which is great. The standard library can't offer that currently (though I would like to expose some of the primitives so crates can build upon them in the future). But I was more responding to @epage concern that this would be basically just a remove_dir_all that calls remove_dir_all on failure. My point was that these methods are in practice different enough to warrant the attempt. Even if the fallback is, sadly, less user friendly whatever its advantages.

[rbtcollins]

FWIW I'm very happy to work with folk to extend the remove_dir_all crate to have appropriate callbacks/what-have-you so that the progress bar can be built as the deletion proceeds without double-handling anything. Note that the cost of building the progress bar is itself most of the time of the deletion, so walking upfront isn't particular beneficial IMO.

[rbtcollins]

Two other thoughts.

The 0.5 and lower versions of the remove_dir_all crate renamed deleted entries into a temp dir (in order to avoid retries on files that don't get removed from the directory atomically). This would have the effect that programs like rust-analyzer doing concurrent compilations into the target that is being cleaned would get an error, and recreate the deleted directory, rather than the remove-dir-all invocation itself failing ... until the unlink of the root directory itself.

If programs work with _at style calls, this approach doesn't help, because they will end up writing to the renamed directory. OTOH anything that is spawning lots of processes - like rust-analyzer - will invariably end up operating on paths, as passing capabilities around is very uncommon so far.

So one possible approach would be to :

• rename the top level paths being deleted to 'path.clean_'
• use the remove-dir-all implementation of choice to delete the newly named old directory
• recover from interrupts by deleting any existing prefix.clean* paths in target/ as well as the current things to clean.

This would cause new compilations to operate in a fresh directory, and should be quite robust. We could put this back in the remove_dir_all crate, but theres enough app-specific logic there that I suspect just doing the rename and interrupt-safety work outside the recursive-delete workhorse would make sense.

[HamishPoole]

The thing that doesn't quite feel right to me is that this feels like a hack: "If new contents are added to the "removed" directory, let's try removing the contents again". How many "agains" are sufficient?. This is inherently a race condition that we can't win theoretically though we might be able to make it work practically. I'm just wondering if its an effort that is worth it.

This brings up an interesting thought: This change is effectively a remove_dir_all that does another remove_dir_all on failure. If we feel this is needed here, do any of the other cases where we call remove_dir_all need this error recovery?

Agree with @epage on this. Ran into this issue, #11441

Given a similar issue is in #11872

Root cause is clearly not MacOS related (still stumped why .DS_Store appeared in my target directory).

std::fs::remove_dir
std::fs::remove_dir_all
and
cargo_util::paths:remove_file

All clearly seem to fail on multiple platforms without logging a proper error.

error: could not remove build directory

Caused by:
  failed to remove directory `/Users/e/Documents/GitHub/turbo/target/wasm32-unknown-unknown`

Caused by:
  Directory not empty (os error 66)

std::remove_dir should panic here IMO. It doesn't. Adding panics to cargo clean is the wrong level of abstraction.

Fixing std::remove_dir_all to panic if it fails to remove a directory is the best solution I think.

As per #11441 (comment), the Finder issue was impossible for me to replicate.

[trevyn]

@weihanglo : To address @epage 's question:

This change is effectively a remove_dir_all that does another remove_dir_all on failure. If we feel this is needed here, do any of the other cases where we call remove_dir_all need this error recovery?

in light of @ChrisDenton 's observation that:

[fs::]remove_dir_all is highly specialized for different platforms

I've added a fallback from paths::remove_dir_all to fs::remove_dir_all on error.

Please let me know if this is suitable, and if you have any other concerns!

[epage]

I've added a fallback from paths::remove_dir_all to fs::remove_dir_all on error.

I've not refreshed myself on the context of this PR but if there is a stronger variant for us to fallback to that will do the right thing for us with maybe some degraded quality of experience, that makes sense to me.

[weihanglo]

Thanks.

As a side note, we still look for a better way around remove_dir_all. The performance of the ad-hoc implementation in Cargo is not pretty good, especially for walking the filesystem.

[weihanglo]

@bors r+

[bors]

📌 Commit ac3ccdd has been approved by weihanglo

It is now in the queue for this repository.

[bors]

⌛ Testing commit ac3ccdd with merge 58b22f0...

[bors]

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 58b22f0 to master...