docs(runtime): detail microVM V7 — teardown, crash recovery, observability (RIG-2498) - #931
Open
rigel-mintaka wants to merge 3 commits into
Open
docs(runtime): detail microVM V7 — teardown, crash recovery, observability (RIG-2498)#931rigel-mintaka wants to merge 3 commits into
rigel-mintaka wants to merge 3 commits into
Conversation
…ility (RIG-2498) Details the V7 milestone under the frozen parent `microvm-runner.md` (its Plan § V7, plus Approach (f) "Teardown and mid-session death" and (g) "Observability + kill switch"). V7 is the largest remaining V-task and the last unwritten design in the microVM spine; every other non-trivial V-task (V2a, V2b, V3, V4, V5) got its own detailing record, and V6 — which skipped one — spent three review rounds on defects a design pass would likely have caught. Docs-only: no file under `go/` is touched, so this is reviewable as pure design and cannot collide with V6 (#912), which is live on `go/internal/runtime/**`. V7 lands after it. ## What the record settles - **(a) Per-session pidfiles.** Today only passt records a pid on disk, and it records it itself — so the parent's "reaps orphaned VMM/virtiofsd/net-backend processes by their per-session runtime dir (pidfiles + process-liveness check)" has nothing to work with. Three host-written pidfiles carry `<pid> <starttime> <bootid>`: a bare pid cannot survive PID reuse, and the reuse case is the kill-an-innocent hazard the reaper must defend against. Written atomically (temp + rename in the same dir), because a torn write during exactly the crash window the reaper exists for would demote a recorded live child to the no-pidfile arm — leaking the process while destroying its only record. `PR_SET_PDEATHSIG` does not rescue that; it is explicitly best-effort. - **(b) `ReapOrphans(ctx) error`.** Kill-and-remove at startup, never adopt — the parent is verbatim on this, and the reasoning holds at source: the exec-gate nonce, the `guestVM` seam handle, and the reaper channels all die with the process, so a found VM's handshake state is not reconstructable. Starttime-verified SIGTERM → grace → SIGKILL, VMM first; `ESRCH` is benign (died between probe and signal), `EPERM` means the pid is no longer ours and is never blindly retried; `ctx` bounds the escalation. - **(c) Mid-session death.** `VM.DeathWatch()` selects over the reaper channels V6 already installed — no second `Wait`, so the one-reaper-per-child invariant is preserved. A per-session monitor runs the one existing teardown path; `*runtime.SessionDeadError` mirrors the working `TimeoutError` precedent. In-flight execs consult the VMM's exit state directly rather than only the recorded cause, closing the window where the monitor has not yet taken the lock. - **(d) Observability.** The parent's `compass_microvm_*` names are Prometheus-style and illustrative; the tree is OpenTelemetry and contains exactly **one** instrument today. The record translates them into concrete dotted names with instrument types, units, and a closed-enum attribute set, inheriting the sole existing precedent's warn-and-disable construction posture. The PSS gauge snapshots the session list under the lock and reads `smaps_rollup` outside it — otherwise every collection would block `Create`/`Start`/`Exec` on `m.mu`, contradicting the design's own claim. Five dependency-ordered W-slices, each with exact Go signatures and a hermetic-vs-KVM test split. ## Red-teamed before review Drafted by the design subagent, then attacked by a design-critic pass whose 8 clear improvements are folded above (boot id, atomic write, `tearingDown` reset, the lock-scope fix, a single-consumer contract on `DeathWatch`, the direct VMM probe, signal-error handling, plan re-ordering + a weighed cgroup/process-group alternative). Four findings were genuine forks and are promoted to load-bearing Open Questions rather than decided silently. The sharpest: **nothing excludes two Runner processes sharing a RunRoot**, and the realistic case is a restart racing a still-draining predecessor — the new Runner's `ReapOrphans` sees live sessions as orphans, and the starttime verification *confirms* the kill rather than preventing it. That is the same kill-an-innocent hazard as (a), one layer up, and the fix changes startup semantics, so it is Matt's call. ## Open Questions Ten, six load-bearing — they block the merge-freeze per the design workflow. Two are places the frozen parent is under-specified or reads differently than V7 implements: "every existing session metric gains a `backend` label" is vacuous when no session metric exists, and the supervised set includes "the guest via the supervisor channel's liveness" while V7's death matrix covers only host children. Both are surfaced for a ruling rather than papered over. Spec-impact: none. Ledger-impact: none — V7 details behavior the parent's frozen decisions already ratified, so it proposes no new ledger row; its four load-bearing Open Questions are rulings on existing frozen sentences, not new decisions. Refs RIG-2498 Co-authored-by: Matt Wilkinson <matt@rigel.build>
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
|
Compass engineering docs preview: https://compass-runner-rig-2498-micr.compass-eng-docs.pages.dev Deployed from Changed pages: |
…rations, a delivered reaper lock (RIG-2498) Folds all 10 gating findings (4 high, 6 medium) from the review of the V7 record, plus the lows worth taking. Docs-only; still one file, no `go/` change. The four highs were design defects rather than wording — this record freezes on merge, so each would have become a contract an executor built against. ## `Stop` must not refuse a dead session The refuse list included `Stop`, and `AgentRuntime.Teardown` is Stop-then-Remove with an early return on Stop's error (`go/internal/runtime/agent.go:216-222`) — the Runner's only teardown path. So a refusing `Stop` meant a dead session could **never** be torn down: `Teardown` returns at stage "stop", `Remove` is never reached, and the session-table entry plus the runtime dir leak permanently. It also contradicted the frozen parent's "`Remove` is idempotent on an already-dead VM" (`microvm-runner.md:248`), this record's own "Idempotent Remove, unchanged", and today's deliberately tolerant `Stop` (`microvm_lifecycle.go:615-617`). Only the exec verbs refuse now; `Stop`-on-dead is a no-op success. The W3 cycle had tested "deliberate Stop/Remove ⇒ monitor exits silently" and "Remove after death ⇒ nil" but never Stop-after-death, so no named test could fail on it. Added. ## A boolean cannot disambiguate two VM lives `tearingDown` was the whole mechanism for telling a deliberate teardown's exit from a crash, and it is per-session while a death is per-VM. `Shutdown` kills the VMM and waits on `<-vm.vmmExited` (`launch.go:361-369`), so VM#1's exit channel is already closed when `Stop` returns — but nothing schedules its `DeathWatch` selector before the caller resumes. A stale monitor can therefore deliver VM#1's death *after* `Start` cleared the flag and stored VM#2, permanently marking a healthy just-booted session dead and refusing every subsequent `Exec`. Replaced with a monotonic `epoch` per VM life, stored on the session under `m.mu`: the monitor captures its epoch at spawn and discards a death unless the session still carries it, so a stale monitor is structurally inert rather than merely unlikely. `tearingDown` now only suppresses the current epoch. ## The reaper's safety property is now delivered, not recommended The record's most serious self-identified hazard — a restarting Runner's `ReapOrphans` killing a still-draining predecessor's live sessions, with the pid/starttime/boot-id check *confirming* the kill — was defended entirely by an Open Question's recommendation. No W-slice or Task produced it, and the W-slices are the implementation contract, so W1-W5 implemented verbatim shipped exactly the reaper OQ-8 exists to prevent. `lockRunRoot` is now W2's first named deliverable (`LOCK_EX|LOCK_NB` on `<RunRoot>/microvm/.runner.lock`, taken before any scan, held for the Runner's life, refuse-not-wait), with a step 0 in the procedure, a Global Constraint that the reaper never scans without it, and a hermetic case asserting a second acquisition is refused. OQ-8 still rules on *which* mechanism; it no longer supplies it. ## Atomicity closed the wrong window The atomic temp+rename closes the torn-write path, but OQ-7 leaned on it to justify removing an unparseable dir — conflating it with the spawn→write window. The VMM starts **last** (`launch.go:168`, `:197`, `:222`), so a Runner dying between `startChild(vm.vmm)` and the pidfile rename leaves parseable pidfiles for dead helpers and no record of a live orphan VMM; the reaper then concludes "all recorded processes confirmed gone" and `RemoveAll`s the only evidence. A pre-spawn `intent` record closes it, so the on-disk set is conservative by construction: over-naming is tolerable, under-naming is unrecoverable. ## Mediums Citation convention (an unprefixed `launch.go:N` resolves against `main`; a V6 claim carries `PR #912`) applied per-cite and swept; W5's startup-order cycle no longer cites a seam that cannot observe it; W2's KVM cycle now SIGKILLs a real child Runner so the kernel's lock-release arm is exercised rather than assumed; W4 names the quota-reading mechanism and its staleness posture instead of a definite article for an artifact that does not exist; §(d) picks one instrument-construction posture and targets `selectEngine`; OQ-6 states plainly that passt leaves the parent's supervised set and is re-graded load-bearing. Verified: markdownlint 0 errors across 199 files; 124 code citations audited in-bounds, 0 out-of-bounds; one file changed, zero `go/` files. Spec-impact: none. Ledger-impact: none — V7 details behavior the parent's frozen decisions already ratified, so it proposes no new ledger row; its load-bearing Open Questions are rulings on existing frozen sentences, not new decisions. Refs RIG-2498 Co-authored-by: Matt Wilkinson <matt@rigel.build>
…ock owner, a meaningful quota gate (RIG-2498) Folds all 6 gating findings (2 high, 4 medium) from round 2, plus the lows worth taking. Round 2 independently verified round 1's 4 highs, 6 mediums and 7 lows as fixed at source rather than against the previous commit message. Docs-only; one file, no `go/` change. Both highs are the same lesson twice: a fix for one finding opened a path the record had not reasoned about. ## A death in the first VM life must not refuse execs in the second `deadCause` was written once and never cleared, while the epoch design this record introduced asserts a session can have two VM lives. So a crash recorded in life #1 refused every `Exec` in life #2 forever — the exact failure the epoch was introduced to prevent, arriving through a different door, and made reachable through the ordinary API by round 1's own fix (`Stop`-on-dead became a no-op success, so the cause deliberately survives a teardown that now succeeds). Fixed with `deadEpoch` beside `deadCause`: readers refuse only when the recorded cause belongs to the current generation, so the read side is scoped exactly as the epoch already scoped the write side. Clearing on `Start` was the alternative and is strictly weaker — `Remove` is specified to observe and report the cause after a death while `Stop`-on-dead returns before any `Start` runs, so no single moment satisfies both "the cause survives for `Remove`" and "the cause is gone by the next `Exec`". The refuse condition is now stated once and referenced, rather than restated at three sites that had already drifted. ## The RunRoot lock has one owner Round 1 promoted the lock from an Open Question's recommendation to a real W2 deliverable, but left its owner contradictory: four places said `ReapOrphans` takes it, W2's interface said the startup unit did. The `ReapOrphans`-owned reading self-refuses on the second invocation this record itself designs for, and cannot satisfy the hold-for-the-Runner's-life requirement, since the lock would release on return. The startup unit owns it: `run()` (`go/cmd/compass-runner/main.go:43`) acquires once ahead of the reap hook and defers the release, and `ReapOrphans` takes a `held RunRootLock` token — a value an executor cannot forget to check, rather than a documented precondition they can. Every mention now names one acquirer; the two sentences that still put acquisition on the reaper are the ones that must (OQ-8's verbatim option text, and the Alternatives entry rejecting that shape). ## A quota ratio is emitted only when it means something The new gauge gated on `QuotaReading.Active()`, which has two independent arms: the inode arm returns true with `LimitBytes == 0`, and `UsedRatio()` short-circuits to zero in exactly that case (`PR #912 microvm_quota.go:117-125`, `:134-153`). So an inode-only projected quota — a legitimate configuration — would have emitted a constant, meaningless zero into a utilization series. The gate is now `LimitBytes > 0`, the condition under which the ratio is defined; `Active()` remains the separate is-there-a-bound question, and the parenthetical states both arms accurately. ## Mediums The monitor loop had a specified arm for every case except channel close, leaving a reachable spin-forever goroutine once `DeathWatch` widened to multi-send — it now ranges over the channel and returns when it closes, with a test cycle for a non-fatal death followed by close. `reapGrace` is unexported in package `microvm` while `ReapOrphans` lives in `runtime`, so the escalation is specified against a reaper-local constant that names its relationship to the original instead of an identifier it cannot reference. And OQ-9's body-assumed answer — per-session PSS on a low-frequency INFO line — is now a named W4 deliverable with a Task, closing the same body-vs-deliverable gap round 1 found in OQ-8; the fold also swept the remaining Open Questions for that class and reports none left. Verified: markdownlint 0 errors across 199 files; 142 code citations audited in-bounds, 0 out-of-bounds, including the OTel `setDelegate` retro-wiring claim that justifies construction-time instrument creation (`go.opentelemetry.io/otel@v1.46.0/internal/global/meter.go:126-147`); one file changed, zero `go/` files. Spec-impact: none. Ledger-impact: none — V7 details behavior the parent's frozen decisions already ratified, so it proposes no new ledger row; its load-bearing Open Questions are rulings on existing frozen sentences, not new decisions. Refs RIG-2498 Co-authored-by: Matt Wilkinson <matt@rigel.build>
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.
Details the V7 milestone under the frozen parent
microvm-runner.md(its Plan § V7, plus Approach (f) "Teardown and mid-session death" and (g) "Observability + kill switch"). V7 is the largest remaining V-task and the last unwritten design in the microVM spine; every other non-trivial V-task (V2a, V2b, V3, V4, V5) got its own detailing record, and V6 — which skipped one — spent three review rounds on defects a design pass would likely have caught.Docs-only: no file under
go/is touched, so this is reviewable as pure design and cannot collide with V6 (#912), which is live ongo/internal/runtime/**. V7 lands after it.What the record settles
<pid> <starttime> <bootid>: a bare pid cannot survive PID reuse, and the reuse case is the kill-an-innocent hazard the reaper must defend against. Written atomically (temp + rename in the same dir), because a torn write during exactly the crash window the reaper exists for would demote a recorded live child to the no-pidfile arm — leaking the process while destroying its only record.PR_SET_PDEATHSIGdoes not rescue that; it is explicitly best-effort.ReapOrphans(ctx) error. Kill-and-remove at startup, never adopt — the parent is verbatim on this, and the reasoning holds at source: the exec-gate nonce, theguestVMseam handle, and the reaper channels all die with the process, so a found VM's handshake state is not reconstructable. Starttime-verified SIGTERM → grace → SIGKILL, VMM first;ESRCHis benign (died between probe and signal),EPERMmeans the pid is no longer ours and is never blindly retried;ctxbounds the escalation.VM.DeathWatch()selects over the reaper channels V6 already installed — no secondWait, so the one-reaper-per-child invariant is preserved. A per-session monitor runs the one existing teardown path;*runtime.SessionDeadErrormirrors the workingTimeoutErrorprecedent. In-flight execs consult the VMM's exit state directly rather than only the recorded cause, closing the window where the monitor has not yet taken the lock.compass_microvm_*names are Prometheus-style and illustrative; the tree is OpenTelemetry and contains exactly one instrument today. The record translates them into concrete dotted names with instrument types, units, and a closed-enum attribute set, inheriting the sole existing precedent's warn-and-disable construction posture. The PSS gauge snapshots the session list under the lock and readssmaps_rollupoutside it — otherwise every collection would blockCreate/Start/Execonm.mu, contradicting the design's own claim.Five dependency-ordered W-slices, each with exact Go signatures and a hermetic-vs-KVM test split.
Red-teamed before review
Drafted by the design subagent, then attacked by a design-critic pass whose 8 clear improvements are folded above (boot id, atomic write,
tearingDownreset, the lock-scope fix, a single-consumer contract onDeathWatch, the direct VMM probe, signal-error handling, plan re-ordering + a weighed cgroup/process-group alternative). Four findings were genuine forks and are promoted to load-bearing Open Questions rather than decided silently.The sharpest: nothing excludes two Runner processes sharing a RunRoot, and the realistic case is a restart racing a still-draining predecessor — the new Runner's
ReapOrphanssees live sessions as orphans, and the starttime verification confirms the kill rather than preventing it. That is the same kill-an-innocent hazard as (a), one layer up, and the fix changes startup semantics, so it is Matt's call.Open Questions
Ten, six load-bearing — they block the merge-freeze per the design workflow. Two are places the frozen parent is under-specified or reads differently than V7 implements: "every existing session metric gains a
backendlabel" is vacuous when no session metric exists, and the supervised set includes "the guest via the supervisor channel's liveness" while V7's death matrix covers only host children. Both are surfaced for a ruling rather than papered over.Spec-impact: none.
Ledger-impact: none — V7 details behavior the parent's frozen decisions already
ratified, so it proposes no new ledger row; its four load-bearing Open Questions
are rulings on existing frozen sentences, not new decisions.
Refs RIG-2498
Co-authored-by: Matt Wilkinson matt@rigel.build