feat(microvm): virtio-fs mount-ns isolation + volume-quota verification (RIG-2497) - #912
Merged
trunk-io[bot] merged 3 commits intoSep 8, 2026
Conversation
|
😎 Merged successfully - details. |
|
Compass engineering docs preview: https://compass-runner-rig-2497-micr.compass-eng-docs.pages.dev Deployed from |
rigel-mintaka
added a commit
that referenced
this pull request
Sep 6, 2026
…, test non-vacuity (RIG-2497) Addresses the round-1 review of #912 (2 highs + 6 mediums + 4 lows fixed; the third high — `--uid-map` vs `--translate-uid` capability surface — is a design fork filed as RIG-3330, with its record-faithful hardening landed here). ## Fail-open fixes (the two acted highs) - **QuotaRequired was dead code.** `MicroVMConfig.QuotaRequired` was never wired to any operator knob, so the D7 quota gate could never fire in production — it was settable only from tests. Added `--microvm-quota-required` (+ `$COMPASS_MICROVM_QUOTA_REQUIRED`, via a new `boolOrEnv` that *refuses* an unparseable value rather than silently reading false) and threaded it into the config literal. `selectEngine` split out a testable `backendConfig()` seam; new `backend_flags_test.go` pins flag/env/precedence end to end. - **verifyQuota probed the wrong filesystem.** It targeted `RunRoot` (the short `/tmp` socket dir), not the session-volume filesystem (the durable D9 volume, arriving as `Mount.HostPath`). Added `MicroVMConfig.VolumeRoot` (+ `--microvm-volume-root`) and pointed the check at it. When `QuotaRequired` is set and `VolumeRoot` is unknown, startup now **fails closed** with a named error instead of reporting a verdict about a filesystem it never probed. ## id-map hardening (RIG-3330 interim, no mechanism swap) Kept the record's named `--uid-map`/`--gid-map` mechanism; added `--modcaps=-mknod` (the one capability with no legitimate use on a workspace share) and an explicit security-posture comment stating the namespace-scoped capability set the ns-uid-0 mapping confers (bounded to the volume subtree by the mount-ns pivot_root). The narrower `--translate-uid` alternative is tracked as fork RIG-3330. ## Correctness + test-adequacy fixes - **subuid base was hardcoded 100000** — a silent EINVAL boot failure on any host whose `/etc/subuid` range starts elsewhere. Now parsed from the invoking user's `/etc/subuid` entry (behind a pure seam), with a named error when absent, and added as a startup preflight axis so it fails legibly at startup, not at first boot's opaque socket-wait. New `launch_idmap_test.go` pins the exact argv (injecting a non-100000 base so a regression is caught). - **XFS inode-only quota false-negative** — the mount-root `f_files` is a dynamic estimate (`fakeinos` shrinks as the fs fills); added a tolerance margin + documented the dynamism so a genuinely-quota'd volume is not read as unbounded. - **mountRoot false-positive** — a stat-blocked ancestor was swallowed and could report a bogus active quota (a sibling project's bound); now an inconclusive probe that fails closed under `QuotaRequired`. - **Cross-session test vacuity** — the `ls` row asserted content that `ls` never prints, and the sweep row depended on `grep`, which the guest image does not ship, so it exited 127 having never searched. Rewrote each row's success condition to match what its command emits (a bash+awk sweep printing `path:line`, needle via env, self-`/proc` matches excluded), added the non-zero-exit assertion to every confined row, and added a control test proving the sweep finds a planted canary. - **Hardcoded exec uid** `"1000"` → `agentuid.AgentUID` in the isolation, egress, and vsock-gateway microVM suites (a routine const change would otherwise silently make every escape attempt run as an unmapped uid and pass green). - **Parity leg** now hard-fails on malformed `stat` output instead of silently skipping the host→guest direction, and its error text names the flags the code actually uses. Lows: corrected the non-Linux stub's rationale, documented `UsedRatio`'s projected-vs-whole-fs dual meaning (and log it only when a quota is active), flattened the virtiofsd argv construction, and de-duplicated the quota rationale headers to a single source. ## Verification All gates green on a real-KVM box: build+vet (default + `-tags microvm` + darwin `!linux` stub), hermetic `-race` (runtime + microvm + cmd), the KVM isolation suite (real boots — the stricter cross-session sweep genuinely walks the tree and failed twice on real self-match artifacts during development, proving it no longer passes vacuously), golangci-lint (tagged + untagged) 0 issues, nilaway clean (default + microvm). Spec-impact: none. Refs RIG-2497 Co-authored-by: Matt Wilkinson <matt@rigel.build>
rigel-mintaka
force-pushed
the
compass-runner/rig-2497-microvm-v6-isolation-quota
branch
from
September 6, 2026 02:25
bc65d7d to
a2aa9ec
Compare
rigel-mintaka
added a commit
that referenced
this pull request
Sep 6, 2026
…, test non-vacuity (RIG-2497) Addresses the round-1 review of #912 (2 highs + 6 mediums + 4 lows fixed; the third high — `--uid-map` vs `--translate-uid` capability surface — is a design fork filed as RIG-3330, with its record-faithful hardening landed here). ## Fail-open fixes (the two acted highs) - **QuotaRequired was dead code.** `MicroVMConfig.QuotaRequired` was never wired to any operator knob, so the D7 quota gate could never fire in production — it was settable only from tests. Added `--microvm-quota-required` (+ `$COMPASS_MICROVM_QUOTA_REQUIRED`, via a new `boolOrEnv` that *refuses* an unparseable value rather than silently reading false) and threaded it into the config literal. `selectEngine` split out a testable `backendConfig()` seam; new `backend_flags_test.go` pins flag/env/precedence end to end. - **verifyQuota probed the wrong filesystem.** It targeted `RunRoot` (the short `/tmp` socket dir), not the session-volume filesystem (the durable D9 volume, arriving as `Mount.HostPath`). Added `MicroVMConfig.VolumeRoot` (+ `--microvm-volume-root`) and pointed the check at it. When `QuotaRequired` is set and `VolumeRoot` is unknown, startup now **fails closed** with a named error instead of reporting a verdict about a filesystem it never probed. ## id-map hardening (RIG-3330 interim, no mechanism swap) Kept the record's named `--uid-map`/`--gid-map` mechanism; added `--modcaps=-mknod` (the one capability with no legitimate use on a workspace share) and an explicit security-posture comment stating the namespace-scoped capability set the ns-uid-0 mapping confers (bounded to the volume subtree by the mount-ns pivot_root). The narrower `--translate-uid` alternative is tracked as fork RIG-3330. ## Correctness + test-adequacy fixes - **subuid base was hardcoded 100000** — a silent EINVAL boot failure on any host whose `/etc/subuid` range starts elsewhere. Now parsed from the invoking user's `/etc/subuid` entry (behind a pure seam), with a named error when absent, and added as a startup preflight axis so it fails legibly at startup, not at first boot's opaque socket-wait. New `launch_idmap_test.go` pins the exact argv (injecting a non-100000 base so a regression is caught). - **XFS inode-only quota false-negative** — the mount-root `f_files` is a dynamic estimate (`fakeinos` shrinks as the fs fills); added a tolerance margin + documented the dynamism so a genuinely-quota'd volume is not read as unbounded. - **mountRoot false-positive** — a stat-blocked ancestor was swallowed and could report a bogus active quota (a sibling project's bound); now an inconclusive probe that fails closed under `QuotaRequired`. - **Cross-session test vacuity** — the `ls` row asserted content that `ls` never prints, and the sweep row depended on `grep`, which the guest image does not ship, so it exited 127 having never searched. Rewrote each row's success condition to match what its command emits (a bash+awk sweep printing `path:line`, needle via env, self-`/proc` matches excluded), added the non-zero-exit assertion to every confined row, and added a control test proving the sweep finds a planted canary. - **Hardcoded exec uid** `"1000"` → `agentuid.AgentUID` in the isolation, egress, and vsock-gateway microVM suites (a routine const change would otherwise silently make every escape attempt run as an unmapped uid and pass green). - **Parity leg** now hard-fails on malformed `stat` output instead of silently skipping the host→guest direction, and its error text names the flags the code actually uses. Lows: corrected the non-Linux stub's rationale, documented `UsedRatio`'s projected-vs-whole-fs dual meaning (and log it only when a quota is active), flattened the virtiofsd argv construction, and de-duplicated the quota rationale headers to a single source. ## Verification All gates green on a real-KVM box: build+vet (default + `-tags microvm` + darwin `!linux` stub), hermetic `-race` (runtime + microvm + cmd), the KVM isolation suite (real boots — the stricter cross-session sweep genuinely walks the tree and failed twice on real self-match artifacts during development, proving it no longer passes vacuously), golangci-lint (tagged + untagged) 0 issues, nilaway clean (default + microvm). Spec-impact: none. Refs RIG-2497 Co-authored-by: Matt Wilkinson <matt@rigel.build>
rigel-mintaka
added a commit
that referenced
this pull request
Sep 6, 2026
…ness, mount-point quota degeneracy (RIG-2497) Round 2 of the #912 review returned 2 highs / 4 mediums / 5 lows, all folded here. The two highs are defects the round-1 fix code itself introduced or left latent, and both fail in the direction that matters: silently, on a host that differs from this one. ## Fail-silent fixes (the highs) - **`/etc/subgid` is a separate allocation.** Round 1 correctly stopped hardcoding the subordinate base, but read only `/etc/subuid` and fed that one value into *both* `--uid-map` and `--gid-map` — while `newgidmap` validates the gid range against `/etc/subgid`, an independently-editable file (`usermod --add-subuids` and `--add-subgids` are separate flags; the code's own remediation text conceded they can diverge). A host whose two ranges differ would die in virtiofsd's gid map at first boot with no useful diagnostic. Both files are now read independently through a path-injected core, the two arms carry their own bases, and `VerifySubordinateIDRange` resolves both so a missing subgid range fails at *startup* naming the file and the fix. The false "allocated in lockstep" claim is gone. This box cannot observe the bug (both files read `mattw:100000:65536`), so the regression test injects divergent bases as fixtures rather than reading the host. - **Readiness ignored liveness.** `waitForSockets` polled only for socket-path existence, and virtiofsd binds its socket *before* the id-map step that can fail — so any mapping failure left the socket on disk, readiness returned nil, and cloud-hypervisor was started against a dead virtiofsd, surfacing as an inscrutable vhost-user error instead of virtiofsd's own message. Readiness is now liveness-aware and its error carries the daemon's name and log tail. Doing this properly required fixing the supervision model underneath it: only the VMM had a reaper, virtiofsd and passt were `Wait`ed lazily during teardown, and liveness was tracked by an atomic set by whichever path happened to `Wait` first — so a poll-loop check would have raced a second `Wait`. There is now exactly one reaper per child, installed at spawn, with `waitErr` published before the `exited` channel closes as the happens-before edge. `Running()` checks exit *first*, since a zombie still answers `Signal(0)`. ## Correctness - **Quota verification refused startup on a correctly-provisioned host.** `mountRoot` never checked whether the path it was handed *is* the mount point — `mountRoot("/tmp") == "/tmp"` — so when the volume root is the mount point of a dedicated quota'd filesystem (the natural production layout), the probe compared a filesystem against itself and `Active()` was false by construction. That case is now a distinct inconclusive verdict naming the fix (point `--microvm-volume-root` at a subdirectory), not a false negative. The former "positive control" would have passed an identity-returning implementation; it now asserts the walk actually climbs and converges from two depths. - The empty-path error still named the retired `--microvm-runroot` knob, with its own test pinning the stale text — a defect regression-locked by its test. Both corrected. ## Test cost and honesty - The cross-session sweep forked one `awk` per file across the whole guest filesystem: ~72s against a hard 120s exec cap, i.e. a flake waiting for a loaded box. Batched (one `awk` per 200 files) and scoped to the trees that discriminate, it now runs in milliseconds. Batching made the existing control partly vacuous — it never fills a batch — so a cross-batch test covers the mid-loop flush and the accumulator, with needles planted in both the first and final batch. Every confined row is now timed against the cap so creep is caught rather than rediscovered. - The quota-fill leg wrote the entire project limit plus 64MiB, which against a realistic quota would blow the exec cap and report a timeout instead of the `EDQUOT` verdict it exists to prove. It now fills only remaining headroom plus margin, and refuses with a named reason when the volume is too large to be a purpose-sized test quota. Lows: pinned the `--modcaps` literal in a shared constant (virtiofsd silently accepts misspelled capability names, so the argv assertion is the only guard), reworded the provisioning hint so `100000` reads as an example rather than canonical, and documented `Active()`'s deliberate byte/inode asymmetry. `boolOrEnv`'s precedence is unchanged and now documented as a contract — an explicit `--microvm-quota-required=false` does not override an env true, which fails toward leaving the multi-tenant gate on — with a test pinning that direction. ## Verification Gates green on a real-KVM box: build + vet (default, `-tags microvm`, darwin `!linux` stub), hermetic `-race`, the KVM suite under `COMPASS_REQUIRE_MICROVM=1` (real boots), golangci-lint 0 issues both tag sets, nilaway clean both. The load-bearing tests are mutation-verified: reverting the liveness check to path-existence-only reproduces the finding's exact failure, and an identity-returning `mountRoot` fails the strengthened control while passing the old one. Spec-impact: none. Refs RIG-2497 Co-authored-by: Matt Wilkinson <matt@rigel.build>
rigel-mintaka
force-pushed
the
compass-runner/rig-2497-microvm-v6-isolation-quota
branch
from
September 6, 2026 04:08
a2aa9ec to
3b798cc
Compare
rigel-mintaka
added a commit
that referenced
this pull request
Sep 6, 2026
…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>
rigel-mintaka
added a commit
that referenced
this pull request
Sep 6, 2026
…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>
…on (RIG-2497)
V6 of the frozen microVM Runner backend design (record § Plan > V6): per-session virtio-fs isolation proven under real KVM, plus the D7 verify-never-assign volume-quota preflight. Executes against the frozen record (`docs/designs/infra/runtime/compass-elastic-session-runtime/microvm-runner.md`, D7 + Approach (d)).
## What lands
- **Quota verification (D7: verify, never assign).** `VolumeQuota{Bytes,Inodes}` (the expected bound), `verifyVolumeQuota(path, want, probe) → (QuotaReading, error)`, and `MicroVMConfig.QuotaRequired` (appended, additive). The Runner never *assigns* quota (that needs `CAP_SYS_ADMIN` it lacks); it only *verifies* an operator-provisioned project quota is active — a rootless, read-only check wired into `verifyMicroVMSupport` as step 5. When `QuotaRequired` is set (multi-tenant profile) an absent quota fails startup naming the volume + fix; unset (Dogfood single-tenant) it logs utilization and proceeds.
- **The rootless quota read is `statfs(2)`-derived, not `quotactl`.** `quotactl(Q_XGETQUOTA, PRJQUOTA)` EPERMs rootless (the kernel gates a non-self quota id on `CAP_SYS_ADMIN`); `FS_IOC_FSGETXATTR` yields only the project-id label, not whether enforcement is live. On XFS and ext4 the kernel rewrites a project-quota'd directory's `statfs` totals to the project's limit+usage, so path-totals < mount-root-totals *is* the kernel reporting an enforced quota — answered unprivileged, with the utilization in the same call.
- **The isolation-proving KVM suite.** Real guests booted under KVM: traversal-confined (dot-dot, symlink-to-host, absolute-path, deep-dot-dot — all blocked), cross-session-unreachable (two live guests; A cannot reach B's volume by any vector), and host-ownership parity (guest-authored files land host-side `uid:gid` matching podman's `--userns=keep-id` target). The real ENOSPC/EDQUOT-in-guest leg is honestly gated (`requireQuotaFS`): it skips where no quota'd filesystem + root exist (a `COMPASS_REQUIRE_QUOTA_FS=1` hard-fail switch for the CI/managed profile), never fake-passing. The verification *logic* is proven by the hermetic unit tests, which run everywhere.
## Load-bearing fix: guest writes to /workspace were broken
The parity test surfaced a real defect masked because no prior test wrote to the volume: under `--sandbox=namespace` alone, rootless virtiofsd could not become namespace-root (`Couldn't set the process uid as root: -1`), so it could not chown a newly created inode to the requesting guest id — **every** guest create on the share failed `EINVAL`. The share was effectively read-only to the guest.
The fix is the record's named mechanism (§(d): "virtiofsd does its own uid/gid translation via that userns"): `--uid-map`/`--gid-map` map a subordinate id to namespace-root (so the daemon can chown) and the agent id to the invoking host `(uid, gid)`. gid maps to the host *gid*, not the uid — the guest agent runs `uid==gid`, but a host user's gid differs (e.g. `1000:100`), and collapsing gid onto uid is exactly the parity break the test detects. Rootless throughout (a `/etc/subuid` subordinate id, no `CAP_SYS_ADMIN`); `--uid-map` over `--translate-uid` also keeps POSIX ACLs available on the share.
`BootConfig.AgentUID` carries the guest uid (zero disables the mapping, preserving the V2a spike harness); `Create` threads `spec.UID` through.
## Verification
All gates green on a real-KVM box (Intel, nested virt): build+vet (default + `-tags microvm`, darwin cross-build of the non-linux stub), hermetic `-race` suite, the KVM isolation suite (real boots, `COMPASS_REQUIRE_MICROVM=1`), full tagged regression (boot/contract/egress/Q-budget unregressed), golangci-lint (tagged + untagged) 0 issues, nilaway clean (default + microvm).
No OTel metric registration (V7 owns the metric set — V6 only exposes `QuotaReading.UsedRatio()`), no teardown/reap (V7), no benchmark (V8). `go/internal/vfs` untouched. `ContainerRuntime` interface unchanged.
Spec-impact: none. Refs RIG-2497
Co-authored-by: Matt Wilkinson <matt@rigel.build>
…, test non-vacuity (RIG-2497) Addresses the round-1 review of #912 (2 highs + 6 mediums + 4 lows fixed; the third high — `--uid-map` vs `--translate-uid` capability surface — is a design fork filed as RIG-3330, with its record-faithful hardening landed here). ## Fail-open fixes (the two acted highs) - **QuotaRequired was dead code.** `MicroVMConfig.QuotaRequired` was never wired to any operator knob, so the D7 quota gate could never fire in production — it was settable only from tests. Added `--microvm-quota-required` (+ `$COMPASS_MICROVM_QUOTA_REQUIRED`, via a new `boolOrEnv` that *refuses* an unparseable value rather than silently reading false) and threaded it into the config literal. `selectEngine` split out a testable `backendConfig()` seam; new `backend_flags_test.go` pins flag/env/precedence end to end. - **verifyQuota probed the wrong filesystem.** It targeted `RunRoot` (the short `/tmp` socket dir), not the session-volume filesystem (the durable D9 volume, arriving as `Mount.HostPath`). Added `MicroVMConfig.VolumeRoot` (+ `--microvm-volume-root`) and pointed the check at it. When `QuotaRequired` is set and `VolumeRoot` is unknown, startup now **fails closed** with a named error instead of reporting a verdict about a filesystem it never probed. ## id-map hardening (RIG-3330 interim, no mechanism swap) Kept the record's named `--uid-map`/`--gid-map` mechanism; added `--modcaps=-mknod` (the one capability with no legitimate use on a workspace share) and an explicit security-posture comment stating the namespace-scoped capability set the ns-uid-0 mapping confers (bounded to the volume subtree by the mount-ns pivot_root). The narrower `--translate-uid` alternative is tracked as fork RIG-3330. ## Correctness + test-adequacy fixes - **subuid base was hardcoded 100000** — a silent EINVAL boot failure on any host whose `/etc/subuid` range starts elsewhere. Now parsed from the invoking user's `/etc/subuid` entry (behind a pure seam), with a named error when absent, and added as a startup preflight axis so it fails legibly at startup, not at first boot's opaque socket-wait. New `launch_idmap_test.go` pins the exact argv (injecting a non-100000 base so a regression is caught). - **XFS inode-only quota false-negative** — the mount-root `f_files` is a dynamic estimate (`fakeinos` shrinks as the fs fills); added a tolerance margin + documented the dynamism so a genuinely-quota'd volume is not read as unbounded. - **mountRoot false-positive** — a stat-blocked ancestor was swallowed and could report a bogus active quota (a sibling project's bound); now an inconclusive probe that fails closed under `QuotaRequired`. - **Cross-session test vacuity** — the `ls` row asserted content that `ls` never prints, and the sweep row depended on `grep`, which the guest image does not ship, so it exited 127 having never searched. Rewrote each row's success condition to match what its command emits (a bash+awk sweep printing `path:line`, needle via env, self-`/proc` matches excluded), added the non-zero-exit assertion to every confined row, and added a control test proving the sweep finds a planted canary. - **Hardcoded exec uid** `"1000"` → `agentuid.AgentUID` in the isolation, egress, and vsock-gateway microVM suites (a routine const change would otherwise silently make every escape attempt run as an unmapped uid and pass green). - **Parity leg** now hard-fails on malformed `stat` output instead of silently skipping the host→guest direction, and its error text names the flags the code actually uses. Lows: corrected the non-Linux stub's rationale, documented `UsedRatio`'s projected-vs-whole-fs dual meaning (and log it only when a quota is active), flattened the virtiofsd argv construction, and de-duplicated the quota rationale headers to a single source. ## Verification All gates green on a real-KVM box: build+vet (default + `-tags microvm` + darwin `!linux` stub), hermetic `-race` (runtime + microvm + cmd), the KVM isolation suite (real boots — the stricter cross-session sweep genuinely walks the tree and failed twice on real self-match artifacts during development, proving it no longer passes vacuously), golangci-lint (tagged + untagged) 0 issues, nilaway clean (default + microvm). Spec-impact: none. Refs RIG-2497 Co-authored-by: Matt Wilkinson <matt@rigel.build>
…ness, mount-point quota degeneracy (RIG-2497) Round 2 of the #912 review returned 2 highs / 4 mediums / 5 lows, all folded here. The two highs are defects the round-1 fix code itself introduced or left latent, and both fail in the direction that matters: silently, on a host that differs from this one. ## Fail-silent fixes (the highs) - **`/etc/subgid` is a separate allocation.** Round 1 correctly stopped hardcoding the subordinate base, but read only `/etc/subuid` and fed that one value into *both* `--uid-map` and `--gid-map` — while `newgidmap` validates the gid range against `/etc/subgid`, an independently-editable file (`usermod --add-subuids` and `--add-subgids` are separate flags; the code's own remediation text conceded they can diverge). A host whose two ranges differ would die in virtiofsd's gid map at first boot with no useful diagnostic. Both files are now read independently through a path-injected core, the two arms carry their own bases, and `VerifySubordinateIDRange` resolves both so a missing subgid range fails at *startup* naming the file and the fix. The false "allocated in lockstep" claim is gone. This box cannot observe the bug (both files read `mattw:100000:65536`), so the regression test injects divergent bases as fixtures rather than reading the host. - **Readiness ignored liveness.** `waitForSockets` polled only for socket-path existence, and virtiofsd binds its socket *before* the id-map step that can fail — so any mapping failure left the socket on disk, readiness returned nil, and cloud-hypervisor was started against a dead virtiofsd, surfacing as an inscrutable vhost-user error instead of virtiofsd's own message. Readiness is now liveness-aware and its error carries the daemon's name and log tail. Doing this properly required fixing the supervision model underneath it: only the VMM had a reaper, virtiofsd and passt were `Wait`ed lazily during teardown, and liveness was tracked by an atomic set by whichever path happened to `Wait` first — so a poll-loop check would have raced a second `Wait`. There is now exactly one reaper per child, installed at spawn, with `waitErr` published before the `exited` channel closes as the happens-before edge. `Running()` checks exit *first*, since a zombie still answers `Signal(0)`. ## Correctness - **Quota verification refused startup on a correctly-provisioned host.** `mountRoot` never checked whether the path it was handed *is* the mount point — `mountRoot("/tmp") == "/tmp"` — so when the volume root is the mount point of a dedicated quota'd filesystem (the natural production layout), the probe compared a filesystem against itself and `Active()` was false by construction. That case is now a distinct inconclusive verdict naming the fix (point `--microvm-volume-root` at a subdirectory), not a false negative. The former "positive control" would have passed an identity-returning implementation; it now asserts the walk actually climbs and converges from two depths. - The empty-path error still named the retired `--microvm-runroot` knob, with its own test pinning the stale text — a defect regression-locked by its test. Both corrected. ## Test cost and honesty - The cross-session sweep forked one `awk` per file across the whole guest filesystem: ~72s against a hard 120s exec cap, i.e. a flake waiting for a loaded box. Batched (one `awk` per 200 files) and scoped to the trees that discriminate, it now runs in milliseconds. Batching made the existing control partly vacuous — it never fills a batch — so a cross-batch test covers the mid-loop flush and the accumulator, with needles planted in both the first and final batch. Every confined row is now timed against the cap so creep is caught rather than rediscovered. - The quota-fill leg wrote the entire project limit plus 64MiB, which against a realistic quota would blow the exec cap and report a timeout instead of the `EDQUOT` verdict it exists to prove. It now fills only remaining headroom plus margin, and refuses with a named reason when the volume is too large to be a purpose-sized test quota. Lows: pinned the `--modcaps` literal in a shared constant (virtiofsd silently accepts misspelled capability names, so the argv assertion is the only guard), reworded the provisioning hint so `100000` reads as an example rather than canonical, and documented `Active()`'s deliberate byte/inode asymmetry. `boolOrEnv`'s precedence is unchanged and now documented as a contract — an explicit `--microvm-quota-required=false` does not override an env true, which fails toward leaving the multi-tenant gate on — with a test pinning that direction. ## Verification Gates green on a real-KVM box: build + vet (default, `-tags microvm`, darwin `!linux` stub), hermetic `-race`, the KVM suite under `COMPASS_REQUIRE_MICROVM=1` (real boots), golangci-lint 0 issues both tag sets, nilaway clean both. The load-bearing tests are mutation-verified: reverting the liveness check to path-existence-only reproduces the finding's exact failure, and an identity-returning `mountRoot` fails the strengthened control while passing the old one. Spec-impact: none. Refs RIG-2497 Co-authored-by: Matt Wilkinson <matt@rigel.build>
rigel-mintaka
added a commit
that referenced
this pull request
Sep 6, 2026
…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>
rigel-mintaka
added a commit
that referenced
this pull request
Sep 6, 2026
…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>
rigel-mintaka
added a commit
that referenced
this pull request
Sep 6, 2026
…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>
rigel-mintaka
force-pushed
the
compass-runner/rig-2497-microvm-v6-isolation-quota
branch
from
September 6, 2026 17:01
3b798cc to
97f9537
Compare
mattwilkinsonn
approved these changes
Sep 7, 2026
trunk-io Bot
pushed a commit
that referenced
this pull request
Sep 7, 2026
…ility (RIG-2498) (#931) * docs(runtime): detail microVM V7 — teardown, crash recovery, observability (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> * docs(runtime): fold V7 design review — teardown reachability, VM generations, 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> * docs(runtime): fold V7 review round 2 — per-life death scoping, one lock 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> --------- Co-authored-by: Matt Wilkinson <matt@rigel.build>
This was referenced Sep 7, 2026
mattwilkinsonn
approved these changes
Sep 8, 2026
trunk-io
Bot
deleted the
compass-runner/rig-2497-microvm-v6-isolation-quota
branch
September 8, 2026 04:41
|
This pull request was merged into |
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
2 participants
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.
This PR is part of a stack containing 2 PRs:
mainV6 of the frozen microVM Runner backend design (record § Plan > V6): per-session virtio-fs isolation proven under real KVM, plus the D7 verify-never-assign volume-quota preflight. Executes against the frozen record (
docs/designs/infra/runtime/compass-elastic-session-runtime/microvm-runner.md, D7 + Approach (d)).What lands
VolumeQuota{Bytes,Inodes}(the expected bound),verifyVolumeQuota(path, want, probe) → (QuotaReading, error), andMicroVMConfig.QuotaRequired(appended, additive). The Runner never assigns quota (that needsCAP_SYS_ADMINit lacks); it only verifies an operator-provisioned project quota is active — a rootless, read-only check wired intoverifyMicroVMSupportas step 5. WhenQuotaRequiredis set (multi-tenant profile) an absent quota fails startup naming the volume + fix; unset (Dogfood single-tenant) it logs utilization and proceeds.statfs(2)-derived, notquotactl.quotactl(Q_XGETQUOTA, PRJQUOTA)EPERMs rootless (the kernel gates a non-self quota id onCAP_SYS_ADMIN);FS_IOC_FSGETXATTRyields only the project-id label, not whether enforcement is live. On XFS and ext4 the kernel rewrites a project-quota'd directory'sstatfstotals to the project's limit+usage, so path-totals < mount-root-totals is the kernel reporting an enforced quota — answered unprivileged, with the utilization in the same call.uid:gidmatching podman's--userns=keep-idtarget). The real ENOSPC/EDQUOT-in-guest leg is honestly gated (requireQuotaFS): it skips where no quota'd filesystem + root exist (aCOMPASS_REQUIRE_QUOTA_FS=1hard-fail switch for the CI/managed profile), never fake-passing. The verification logic is proven by the hermetic unit tests, which run everywhere.Load-bearing fix: guest writes to /workspace were broken
The parity test surfaced a real defect masked because no prior test wrote to the volume: under
--sandbox=namespacealone, rootless virtiofsd could not become namespace-root (Couldn't set the process uid as root: -1), so it could not chown a newly created inode to the requesting guest id — every guest create on the share failedEINVAL. The share was effectively read-only to the guest.The fix is the record's named mechanism (§(d): "virtiofsd does its own uid/gid translation via that userns"):
--uid-map/--gid-mapmap a subordinate id to namespace-root (so the daemon can chown) and the agent id to the invoking host(uid, gid). gid maps to the host gid, not the uid — the guest agent runsuid==gid, but a host user's gid differs (e.g.1000:100), and collapsing gid onto uid is exactly the parity break the test detects. Rootless throughout (a/etc/subuidsubordinate id, noCAP_SYS_ADMIN);--uid-mapover--translate-uidalso keeps POSIX ACLs available on the share.BootConfig.AgentUIDcarries the guest uid (zero disables the mapping, preserving the V2a spike harness);Createthreadsspec.UIDthrough.Verification
All gates green on a real-KVM box (Intel, nested virt): build+vet (default +
-tags microvm, darwin cross-build of the non-linux stub), hermetic-racesuite, the KVM isolation suite (real boots,COMPASS_REQUIRE_MICROVM=1), full tagged regression (boot/contract/egress/Q-budget unregressed), golangci-lint (tagged + untagged) 0 issues, nilaway clean (default + microvm).No OTel metric registration (V7 owns the metric set — V6 only exposes
QuotaReading.UsedRatio()), no teardown/reap (V7), no benchmark (V8).go/internal/vfsuntouched.ContainerRuntimeinterface unchanged.Spec-impact: none. Refs RIG-2497
Co-authored-by: Matt Wilkinson matt@rigel.build