fix: sandbox2: tolerate socket unlink failure in Listener::drop - #1215
fix: sandbox2: tolerate socket unlink failure in Listener::drop#1215gominimal-aw-bot[bot] wants to merge 1 commit into
Conversation
The Listener::drop destructor unwrapped the io::Result from remove_file, so any unlink error (ENOENT from a racing cleanup, EACCES) panicked the destructor. When Drop runs while the stack is already unwinding from a build panic, the second panic escalates to abort(), taking down minimald and every concurrent session. Log the error and continue instead, so a stale-socket condition is tolerated at shutdown.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesSocket cleanup
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to The change prevents socket cleanup failures from causing a shutdown panic while preserving normal cleanup behavior. No actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Routing-Key: inbox-route/I_kwDOSUhdos8AAAABMrkmdA
The
Dropimpl forListener<C>incrates/sandbox2/src/listener.rscalledstd::fs::remove_file(&self.socket_path).unwrap(), so any unlink error — a racing cleanup that already removed the socket, or a permission error — panicked the destructor. BecauseDropruns during stack unwind from an earlier panic on sandbox teardown after a failed build, that second panic escalates toabort(), terminatingminimaldand every concurrent session. No adversarial input is required: a failed build plus a stale socket is enough. The fix logs the error viatracing::warn!and continues, tolerating the stale-socket condition at shutdown. Prior hardening addedcatch_unwindin the channel handler loop but did not cover this destructor (informed by #152). A broader audit of otherDropimpls for the same panic-on-cleanup pattern is left as follow-up.Verification
cargo fmt --all --check --manifest-path target/Cargo.toml — passed, no drift
cargo clippy --workspace --locked -- -D warnings — passed, no warnings
cargo build --workspace --locked — passed
cargo test --workspace --locked — passed, exit 0 (all unit + doctests green)
Note
Fix
Listenerdrop to warn instead of panic on socket file removal failureIn listener.rs, the
Dropimpl forListenerpreviously calledunwrap()onstd::fs::remove_file, causing a panic if the socket file could not be removed. It now logs a warning viatracing::warnand continues without panicking.Macroscope summarized 31e6132.
Summary by CodeRabbit