fix(lifecycle): two signal/restart races (monitor resurrect, kill PID-identity) - #144
Merged
Merged
Conversation
…kill paths From an adversarial concurrency audit. Both are real multi-actor races (the monitor daemon and CLI processes coordinate only via the per-modify flock, which does NOT span an await), with concrete interleavings. 1. (HIGH) Monitor health-restart resurrects a box the user explicitly stopped. The unhealthy branch awaits graceful_stop (up to 10s) OUTSIDE any lock, then unconditionally marks the box dead and re-boots it. If the user runs `a3s-box stop` during that window (setting status=stopped, stopped_by_user), the monitor erases the stop and resurrects the box. Fix: after the await, re-validate the freshly-loaded record (new `health_restart_still_wanted`) and ABORT the restart if it was stopped/stopped_by_user in the meantime. 2. (MED) `kill` fallback host-signal fires with no PID-identity re-check. After a guest-delivery attempt that can block up to ~10s, the `!delivered` fallback calls a bare `send_signal(pid)` with no re-check — the one-shot identity check in require_live_pid is stale by then, so a concurrent force-kill that frees the shim PID (and a kernel PID reuse) could route the signal to an unrelated host process. Fix: re-check is_process_alive_with_identity immediately before the host signal; refuse rather than blind-kill on mismatch. Test: health_restart_still_wanted unit test (neuter-verified — forcing it true makes the test fail on the stopped case). #2 reuses the existing identity check (a defensive re-check, not cleanly unit-testable end-to-end). Full a3s-box-cli lib suite (598) green; fmt + clippy clean.
This was referenced Jun 17, 2026
ZhiXiao-Lin
added a commit
that referenced
this pull request
Jun 17, 2026
… race (#146) HIGH, from the concurrency audit. `boot_from_record` creates a real VM OUTSIDE the state lock (the flock only spans the post-boot record write). A box that is both the monitor's auto-restart target AND a valid user `restart`/`start` target (a dead box) can be booted by both concurrently: each clones the record, boots a VM unlocked, then races to persist. apply_boot_result has no already-running guard, so the second write overwrites the first's pid — the first VM (shim + overlay mount + network endpoint) becomes untracked and is never reaped. ensure_network_connected is idempotent for the dup and the overlay stacks without EBUSY, so the second boot doesn't fail on its own. Fix: a per-box advisory `BootLock` (flock on `locks/<box_id>.boot.lock`) held across the boot AND the record write, via a new `boot::boot_and_record` helper used by both the monitor and `restart`. A loser that acquires the lock and finds the box already running with a live, identity-matched shim returns `AlreadyRunning` and does NOT boot a duplicate — so there is no orphan to tear down (the winner records its pid before releasing the lock, so the loser observes it). The removed-during-boot orphan teardown is centralized into the same helper (both call sites previously needed it; restart lacked it). Validated: full a3s-box-cli lib suite (597) green, fmt + clippy clean on the KVM server. A cross-process boot serialization fix; the interleaving needs a multi-process/real-VM harness to exercise (like the other concurrency fixes), but the flock + re-check-under-lock is self-evidently correct. NOTE: touches monitor.rs poll_once near PR #144's region — rebase if #144 merges first (adjacent, non-overlapping edits). Co-authored-by: Roy Lin <roylin@a3s.box>
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
1 participant
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.
From an adversarial concurrency/TOCTOU audit of the daemonless lifecycle paths. Both are real multi-actor races — the
monitordaemon and CLI processes coordinate only via the per-modifyflock, which does not span anawait.1. (HIGH) Monitor health-restart resurrects a box the user explicitly stopped
The unhealthy branch awaits
graceful_stop(up to 10s) outside any lock, then unconditionally marks the box dead and re-boots it.Interleaving: user runs
a3s-box stop <box>during that 10s window (→status=stopped,stopped_by_user=true); the monitor resumes, overwrites todead, and re-boots. The user's explicit stop is silently erased.Fix: after the await, re-validate the freshly-loaded record (new
health_restart_still_wanted) and abort the restart if it was stopped/stopped_by_userin the meantime.2. (MED)
killfallback host-signal fires with no PID-identity re-checkAfter a guest-delivery attempt that can block up to ~10s, the
!deliveredfallback calls a baresend_signal(pid)— the one-shot identity check inrequire_live_pidis stale by then. A concurrent force-kill that frees the shim PID + a kernel PID reuse could route the signal to an unrelated host process.Fix: re-check
is_process_alive_with_identityimmediately before the host signal; refuse rather than blind-kill on mismatch.Tests
health_restart_still_wantedunit test — neuter-verified (forcing ittruemakes the test FAIL on the stopped case). #2 reuses the existing identity check (a defensive re-check, not cleanly unit-testable end-to-end, like the prior monitor-SIGTERM/durable-write fixes). Fulla3s-box-clilib suite (598) green; fmt + clippy clean on the KVM server.Part of a concurrency audit (4 findings)
Remaining, being fixed separately: concurrent restart+monitor-boot → orphan VM (HIGH, needs per-box boot serialization) and warm-pool replenish-after-drain leak (MED).