fix(compile): sweep the leaked perry_strip scratch directories (#7261) - #7297
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthrough
ChangesStrip scratch cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #7261.
perry compilecreatesperry_strip_<pid>/under the system temp directory at eight sites and never removes it. The_extractsubdirectories are cleaned; the parent — holding every_<lib>_trimmed.lib— is not.Why this is worth a PR rather than a periodic
rmIt leaks one directory per compile, at roughly 64 per two hours of ordinary activity, and it twice took this development machine to zero bytes free today.
The failure mode is badly mis-signalled: a full disk surfaces as unrelated build and test failures in every concurrent process, not as a disk error at the leak site. It killed one background agent outright and stalled another, and the time went into debugging the symptoms.
The fix
All eight sites now share one
strip_tmp_base(), which on first use creates the directory and sweeps other processes' dead-PID directories.Startup sweep rather than an exit hook, deliberately. A
Dropguard oratexithandler does not run onSIGKILL, on a panic-abort, or onprocess::exit— exactly the cases that leave the largest messes. A sweep at first use heals all of them, including leftovers from crashes that happened before this change landed.A live PID's directory is never touched, so concurrent
perryinvocations are safe.The bug my own boundary test caught
The first version probed liveness with
libc::kill(pid as pid_t, 0).u32::MAX as pid_tis-1, andkill(-1, 0)means "every process we may signal" — it succeeds, so a dead directory read as live and was never swept.kill()gives special meaning to every non-positive pid:0is our process group,-1is everything,< -1is a process group.pid_is_livenow rejects anything that cannot fit a positivepid_tbefore probing. This is why the test asserts the boundaries (self,1,u32::MAX,0) rather than just the happy path.Errno is read through
std::io::Error::last_os_error()rather than a platform-specific symbol (__erroron macOS vs__errno_locationon Linux), and anything other thanESRCHerrs toward "live" so an unexpected errno never deletes a directory in use.Verification
cargo test -p perry --bins— noteperryis a binary crate with no lib target, so these run in the bin harness. All pass.remove_dir_allremoved,sweep_removes_dead_pid_dirs_and_keeps_live_onesfails withdead-PID dir must be swept (#7261). The test has teeth.perry_strip_1, init/launchd) proves the sweep discriminates rather than deleting everything it finds.lintgates pass locally, pluscargo fmt --check.Not verified
pid_is_livereturnstrueunconditionally on non-Unix, so Windows keeps the current behaviour (leak) rather than risking a wrong probe. Worth a follow-up if Windows compiles are affected.Summary by CodeRabbit
perry compileleaving behind temporary directories after each invocation, which could eventually exhaust disk space.