Repository navigation
fix(sandbox2,minimald): fork every container on a thread that never retires - #1748
Conversation
…etires hakoniwa's `Command::spawn` forks on the calling thread and the child arms `PR_SET_PDEATHSIG(SIGKILL)`, which Linux delivers when the forking *thread* exits. Session containers were forked on whichever tokio worker ran the launch, so any path that retires a worker (`block_in_place` on session activation displaces one; blocking-pool threads are reaped after their idle keep-alive) SIGKILLed every session and build forked on it, unrelated ones included. Users saw `Session ended abnormally: container received signal SIGKILL` on a session they had not touched, about 10 s after creating or destroying another one. Add `sandbox2::forker`, one process-wide thread started on first use and never joined, and route both container spawns through it. The caller still blocks for the fork-and-exec handshake, as it did forking inline. Replace the sandbox2 doc that told callers to avoid retirable threads with the guarantee that they no longer need to. Tests fork a PDEATHSIG-armed child from a short-lived thread directly (dies with it, the control) and via the fork thread (survives), and check a panic in the forked closure is re-raised without taking the fork thread down. Refs: #1210
|
Warning Review limit reached
This review includes 3 billable files and costs up to $0.75.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Or wait 49 minutes for your next included review. View limit detailsLimit details: You’ve used all 2 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (3)
Comment |
|
Reviewed: 1 finding, none blocking at Address the comments on the diff. Clobber: none: the only prior work this replaces is the |
| /// Runs `f` on the process-wide fork thread and blocks until it returns. | ||
| /// | ||
| /// Anything that forks a container goes through here rather than forking on | ||
| /// its own thread; see the module docs. The caller blocks for the duration of | ||
| /// `f`, which for a container spawn is the fork-and-exec handshake, the same | ||
| /// time a direct `spawn()` costs. | ||
| /// | ||
| /// A panic in `f` unwinds on the fork thread, is carried back, and is | ||
| /// re-raised here; the fork thread itself survives it. | ||
| pub fn on_fork_thread<T, F>(f: F) -> T | ||
| where | ||
| T: Send + 'static, | ||
| F: FnOnce() -> T + Send + 'static, | ||
| { | ||
| let (tx, rx) = mpsc::sync_channel(1); | ||
| let job: Job = Box::new(move || { | ||
| let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(f)); | ||
| let _ = tx.send(result); | ||
| }); | ||
| // The static sender keeps the receiver loop alive for the life of the | ||
| // process, so neither send nor recv can fail. | ||
| sender() | ||
| .send(job) | ||
| .expect("the sandbox fork thread has exited"); | ||
| match rx.recv().expect("the sandbox fork thread dropped a job") { |
There was a problem hiding this comment.
LOW · correctness · confidence 0.55 · in on_fork_thread
on_fork_thread has no re-entrancy guard, and a re-entrant call deadlocks irrecoverably. The caller blocks on rx.recv() (line 62) waiting for the fork thread to run its job, but if the caller already is the fork thread, the job it just enqueued can never run — the fork thread is stuck in for job in rx waiting for a job that will never be consumed — so both the caller and the fork thread hang forever, taking every later container fork with them. Nothing in the tree calls it re-entrantly today (the only two jobs are command.spawn() / cmd.spawn(), neither of which forks again), so this is latent rather than live. Given this codebase's pattern of documenting exactly these thread-affinity footguns (the deleted lib.rs doc spent three bullets on block_in_place/spawn_blocking), a one-line warning in the module docs — or a cheap current_thread check that panics instead of deadlocking — would close it. Fix is local to forker.rs.
What the reviewer checked
forker.rs:59-62 sender().send(job).expect("the sandbox fork thread has exited");
match rx.recv().expect("the sandbox fork thread dropped a job") {
Hand this to an agent
Act on a code-review finding.
File: crates/sandbox2/src/forker.rs
Lines: 38-62
Category: correctness
Severity: low (reviewer confidence 0.55)
Finding:
`on_fork_thread` has no re-entrancy guard, and a re-entrant call deadlocks irrecoverably. The caller blocks on `rx.recv()` (line 62) waiting for the fork thread to run its job, but if the caller already *is* the fork thread, the job it just enqueued can never run — the fork thread is stuck in `for job in rx` waiting for a job that will never be consumed — so both the caller and the fork thread hang forever, taking every later container fork with them. Nothing in the tree calls it re-entrantly today (the only two jobs are `command.spawn()` / `cmd.spawn()`, neither of which forks again), so this is latent rather than live. Given this codebase's pattern of documenting exactly these thread-affinity footguns (the deleted `lib.rs` doc spent three bullets on `block_in_place`/`spawn_blocking`), a one-line warning in the module docs — or a cheap `current_thread` check that panics instead of deadlocking — would close it. Fix is local to `forker.rs`.
First verify this against the code yourself. It is model output and
may be wrong. If it is wrong, or the tradeoff was deliberate, say so
and change nothing.
👍 useful · 👎 noise — react to tell the reviewer. Replying works too, and says more.
…, not a runtime worker (gominimal#2572) hakoniwa arms `PR_SET_PDEATHSIG(SIGKILL)` in every container it forks, and Linux delivers that signal when the forking *thread* exits. gominimal#1748 moved the session launch and `Sandbox::run_with_cancel` onto `sandbox::forker`'s never-retired thread for exactly that reason, but the task-run path in `exec.rs` kept forking its container on whichever tokio worker ran the exec. Any `block_in_place` on that worker (a later activation's `configure_loadout`) displaces it, tokio retires it after its 10 s keep-alive, and every idle task session forked on it dies: hakoniwa reports the container's death as exit 125, with no client disconnect, which is the signature in every lab run of inbox#710. The spawn goes through `on_fork_thread` like the other two. Nothing else changes: the caller still blocks for the fork-and-exec handshake as it did inline. Refs: gominimal/inbox#710, gominimal#1748 Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Summary
Live sessions were dying with
Session ended abnormally: container received signal SIGKILLwhile the user was doing nothing in them, roughly 10 s after creating or destroying some other session. Diagnosis of twomin bugbundles is on #1210 (08-12 at 0.5.0, 09-26 at 0.6.0-dev.78).Mechanism. hakoniwa's
Command::spawnforks on the calling thread, and the child armsPR_SET_PDEATHSIG(SIGKILL). Linux delivers that signal when the forking thread exits, not the process. Session containers were forked on whichever tokio worker ran the launch, so anything that retires a worker kills every container forked on it, unrelated ones included.crates/sandbox2documented this and told callers to stay off retirable threads; session activation has runscaffold_mfile_if_missingundertokio::task::block_in_placesince #1339, which displaces the worker onto a fresh thread and retires the old one after tokio's 10 s idle keep-alive. That matches the 10.03 s gap in the 09-25 log. The 08-12 case predates #1339, so a second retirement path exists; this fix does not depend on finding it.Fix.
sandbox2::forkerowns one thread, started on first use and never joined, and both container spawn sites (Sandbox::run_with_canceland the session launch inminimald::session_host) fork on it. The caller still blocks for the fork-and-exec handshake, exactly as it did forking inline. The sandbox2 doc that warned callers offblock_in_placeandspawn_blockingnow states the guarantee instead.Not changed: the
block_in_placeinsession.rsstays. It is now harmless to containers, and converting it tospawn_blockingmeans restructuring the session actor's borrows, which is a separate cleanup.Refs: #1210
Testing
crates/sandbox2/src/forker.rstests, run undercross(aarch64 musl, Docker):The first is the control: a
PR_SET_PDEATHSIG-armedsleepforked directly from a short-lived thread is dead after that thread joins. The second forks the same child through the fork thread from a short-lived thread and asserts it is still running. The third checks a panic in the forked closure is re-raised to the caller and the fork thread keeps serving.Also run:
cross clippy -p sandbox2 -p minimald --all-targets -- -D warningsclean;cargo fmt --checkclean.sandbox2::tests::none_plan_refuses_vsock_familyfails undercrosson this host with and without this change (the seccomp socket-family probe exits 127 inside the Docker container); pre-existing.cross test -p minimald(full lib suite, 927 s in Docker): 464 passed, 8 failed. Four of the eight (hooks::…hook_injection…×2,nsenter::…shim…×2) fail identically with main'ssession_host.rs, so they are host-environment failures. The other four (connection::direct_tcpip_served_in_release,net::switch::{allowed_egress_succeeds, egress_drop_logged_rate_limited, relay_rejects_non_lease_source}) pass when run on their own with this change applied; they failed only under the full-suite load in the container.Checklist
BREAKING CHANGE:footer present if this is a breaking change