Skip to content

fix: sandbox2: tolerate socket unlink failure in Listener::drop - #1215

Open
gominimal-aw-bot[bot] wants to merge 1 commit into
mainfrom
inbox-patch/sandbox2-listener-drop-unwrap-3244e2d35b8d911a
Open

fix: sandbox2: tolerate socket unlink failure in Listener::drop#1215
gominimal-aw-bot[bot] wants to merge 1 commit into
mainfrom
inbox-patch/sandbox2-listener-drop-unwrap-3244e2d35b8d911a

Conversation

@gominimal-aw-bot

@gominimal-aw-bot gominimal-aw-bot Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Routing-Key: inbox-route/I_kwDOSUhdos8AAAABMrkmdA

The Drop impl for Listener<C> in crates/sandbox2/src/listener.rs called std::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. Because Drop runs during stack unwind from an earlier panic on sandbox teardown after a failed build, that second panic escalates to abort(), terminating minimald and every concurrent session. No adversarial input is required: a failed build plus a stale socket is enough. The fix logs the error via tracing::warn! and continues, tolerating the stale-socket condition at shutdown. Prior hardening added catch_unwind in the channel handler loop but did not cover this destructor (informed by #152). A broader audit of other Drop impls 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 Listener drop to warn instead of panic on socket file removal failure

In listener.rs, the Drop impl for Listener previously called unwrap() on std::fs::remove_file, causing a panic if the socket file could not be removed. It now logs a warning via tracing::warn and continues without panicking.

Macroscope summarized 31e6132.

Summary by CodeRabbit

  • Bug Fixes
    • Improved shutdown reliability by preventing cleanup failures from causing a panic.
    • Cleanup issues are now logged as warnings instead of interrupting the application.

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

coderabbitai Bot commented Aug 13, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 195d4bde-7386-4021-a8e0-a1f85a2709f4

📥 Commits

Reviewing files that changed from the base of the PR and between 1a1025e and 31e6132.

📒 Files selected for processing (1)
  • crates/sandbox2/src/listener.rs

📝 Walkthrough

Walkthrough

Listener::drop now logs a warning when socket-file removal fails. The cleanup path no longer panics on this error.

Changes

Socket cleanup

Layer / File(s) Summary
Listener drop cleanup
crates/sandbox2/src/listener.rs
Listener::drop logs a warning when remove_file fails instead of unwrapping the error and panicking.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Merge Risk: ⚪ Minimal · up to 31e61

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: 0chroma

Poem

I hop by the socket at close of day,
A failed cleanup no longer holds sway.
The warning is logged,
The panic is fogged,
And the listener bounds away.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title uses the required Conventional Commit format and clearly describes the socket unlink failure handling change.
Description check ✅ Passed The description explains the failure mode, fix, impact, follow-up scope, and verification results, but it omits the template headings and checklist.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch inbox-patch/sandbox2-listener-drop-unwrap-3244e2d35b8d911a

Comment @coderabbitai help to get the list of available commands.

@norrietaylor
norrietaylor marked this pull request as ready for review August 14, 2026 22:06
@norrietaylor
norrietaylor requested a review from a team as a code owner August 14, 2026 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants