From 17a93195d0485231b50114eb5a30e66f9fc00866 Mon Sep 17 00:00:00 2001 From: mintaka Date: Fri, 4 Sep 2026 12:09:56 -0400 Subject: [PATCH 1/3] test(runner): cover BootCanary teardown-error-join contract (RIG-3219) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `BootCanary`'s documented contract is that the always-run Remove teardown's error is joined into its return, never discarded — it gates Runner startup, so a silently-swallowed teardown failure would report a canary that leaked a live VMM+virtiofsd as a clean boot, inverting the fail-closed posture on the exact path the gate protects. The `#847` review loop found that contract had zero regression coverage: swallowing both `errors.Join` blocks in `microvm_preflight.go` left the entire hermetic suite green. The named-return + `errors.Join` idiom is precisely what a "tidy the error path" refactor flattens to a plain `defer m.Remove(...)`, and error-swallow discipline is CI-enforced repo law, so the join needs a test that keeps it correct. `TestBootCanaryTeardownErrorJoined` drives an otherwise-successful canary (boot + echo + nonce all pass) with a guest whose `Shutdown` fails, and asserts `BootCanary` returns a non-nil error carrying the teardown failure while the report is still assembled from what ran before teardown. A `shutdownErr` field on `canaryFakeVM` (threaded through `canaryLaunchRecorder`, mirroring the existing `pssErr` knob) is returned from `Shutdown`, so `Remove`'s own `errors.Join` surfaces it into the named return. Test-only; `microvm_preflight.go` is unchanged (the production code was already correct). Mutation-verified: gating off the teardown join reddens only this test and leaves the other ten green; restored to green. Stacked on `#847` (RIG-3148) because it exercises the `BootCanary` teardown code that PR introduces. Spec-impact: none. Refs RIG-3219 Co-authored-by: Matt Wilkinson --- go/internal/runtime/boot_canary_test.go | 63 +++++++++++++++++++++---- 1 file changed, 55 insertions(+), 8 deletions(-) diff --git a/go/internal/runtime/boot_canary_test.go b/go/internal/runtime/boot_canary_test.go index 8234f704..6971c02d 100644 --- a/go/internal/runtime/boot_canary_test.go +++ b/go/internal/runtime/boot_canary_test.go @@ -38,14 +38,16 @@ import ( // canaryFakeVM is a guestVM handle for the canary path: Health answers ready and // echoes the boot nonce the fake launchFunc decoded from cfg.Cmdline (so // awaitHealthy's identity binding passes), PSS returns a configurable non-empty -// map (so GuestRSSBytes is assertable), and Shutdown is recorded (the teardown +// map (so GuestRSSBytes is assertable), Shutdown is recorded (the teardown +// assertion) and can be forced to fail via shutdownErr (the teardown-error-join // assertion). type canaryFakeVM struct { - nonce []byte - pss map[string]int64 - pssErr error - mu sync.Mutex - shutdown bool + nonce []byte + pss map[string]int64 + pssErr error + shutdownErr error + mu sync.Mutex + shutdown bool } func (f *canaryFakeVM) Health(context.Context) (*compassv1.HealthResponse, error) { @@ -60,7 +62,7 @@ func (f *canaryFakeVM) Shutdown(context.Context) error { f.mu.Lock() f.shutdown = true f.mu.Unlock() - return nil + return f.shutdownErr } func (f *canaryFakeVM) WaitVMMExit(_ time.Duration) bool { return true } @@ -83,6 +85,7 @@ type canaryLaunchRecorder struct { mu sync.Mutex pss map[string]int64 pssErr error + shutdownErr error launchErr error vms []*canaryFakeVM calls int @@ -104,7 +107,7 @@ func (r *canaryLaunchRecorder) launch(ctx context.Context, cfg microvm.BootConfi if err != nil { return nil, err } - vm := &canaryFakeVM{nonce: nonce, pss: r.pss, pssErr: r.pssErr} + vm := &canaryFakeVM{nonce: nonce, pss: r.pss, pssErr: r.pssErr, shutdownErr: r.shutdownErr} r.vms = append(r.vms, vm) return vm, nil } @@ -405,6 +408,50 @@ func TestBootCanaryPartialPSSStillReported(t *testing.T) { assertNoTempLeak(t, before) } +// TestBootCanaryTeardownErrorJoined pins the documented teardown-error-join +// contract (BootCanary doc, record §(e)/(f)): the always-run Remove teardown's +// error is joined into BootCanary's return, never discarded. On an OTHERWISE +// successful canary (boot + echo + nonce all pass), a failing guest Shutdown must +// still surface as a non-nil BootCanary error carrying the teardown failure. The +// existing failure-path tests only assert teardown RAN (wasShutdown); none proves +// a FAILING teardown reaches the caller. Because BootCanary gates Runner startup, +// a silently-swallowed Remove failure would report a canary that leaked a live +// VMM+virtiofsd as a clean boot — the fail-closed posture inverted on the exact +// path the gate protects. The named-return + errors.Join idiom is the kind a +// "tidy the error path" refactor flattens to a plain defer; this test breaks on +// that. (The sibling throwaway-workspace RemoveAll join at BootCanary's other +// defer uses the identical named-return errors.Join mechanism this proves, but +// cannot be reddened deterministically without a production temp-dir seam — +// os.TempDir() is not writable to force RemoveAll to fail regardless of uid — so +// it rides the shared mechanism proof rather than a root-sensitive test.) +func TestBootCanaryTeardownErrorJoined(t *testing.T) { + before := canaryTempDirs(t) + m, rec, _ := seamCanary(t, map[string]int64{"cloud-hypervisor": 100}) + rec.shutdownErr = errors.New("boom: shutdown refused") + + report, err := m.BootCanary(t.Context()) + if err == nil { + t.Fatal("BootCanary = nil, want a non-nil error carrying the teardown failure") + } + if !strings.Contains(err.Error(), "boom: shutdown refused") { + t.Errorf("BootCanary error = %v, want it to carry the teardown shutdown failure", err) + } + // The boot chain itself succeeded, so the report is still assembled from what + // ran before teardown — the error is the teardown's, not the boot's. + if want := int64(100 * 1024); report.GuestRSSBytes != want { + t.Errorf("GuestRSSBytes = %d, want %d (the boot chain succeeded; only teardown failed)", report.GuestRSSBytes, want) + } + // Teardown still RAN despite erroring: the VM's Shutdown was invoked and the + // session table drained (Remove deletes the entry before Shutdown). + if len(rec.vms) != 1 || !rec.vms[0].wasShutdown() { + t.Error("canary VM Shutdown was not invoked during teardown") + } + if n := sessionCount(m); n != 0 { + t.Errorf("session table has %d entries after BootCanary, want 0", n) + } + assertNoTempLeak(t, before) +} + // TestBootCanaryDerivesDeadlineWhenCallerHasNone: with a deadline-less caller // ctx, BootCanary derives the internal canaryDeadline bound and threads it into // Start (so a wedged boot cannot hang Runner startup) — the launch ctx carries a From 12dfa95240d6a8f778c9b93a523400e81c9ce141 Mon Sep 17 00:00:00 2001 From: mintaka Date: Fri, 4 Sep 2026 12:41:48 -0400 Subject: [PATCH 2/3] test(runner): pin BootCanary error joins against overwrite + cover workspace-cleanup leg (RIG-3219) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fold the two floor findings from #860's review, both test-only. - The existing teardown-join test drives an otherwise-successful canary, so `err` is nil at the join and `errors.Join(nil, x)` is observationally identical to a plain overwrite — the exact "tidy the error path" refactor the test claims to catch slips through. Add TestBootCanaryBootAndTeardownErrorsBothJoined: fail BOTH the boot chain (echo exec refused) and teardown (Shutdown refused), assert the returned error carries each. Reddens under join->overwrite, and only it. - The sibling throwaway-workspace os.RemoveAll join had no regression coverage; the doc comment wrongly claimed it needed a production temp-dir seam. It is reachable through the existing launch seam: cfg.FSSharedDir is the minted workspace. Add an onLaunch hook on canaryLaunchRecorder and TestBootCanaryWorkspaceCleanupErrorJoined, which locks a subtree (0555) so os.RemoveAll fails EACCES and asserts BootCanary surfaces it. Root-skipped per go/server/socket_test.go. Correct the doc comment to point at the new test. Both mutation-verified: gate off either join and only the matching new test reddens; production code (microvm_preflight.go) unchanged. Refs RIG-3219 Co-authored-by: Matt Wilkinson --- go/internal/runtime/boot_canary_test.go | 85 +++++++++++++++++++++++-- 1 file changed, 81 insertions(+), 4 deletions(-) diff --git a/go/internal/runtime/boot_canary_test.go b/go/internal/runtime/boot_canary_test.go index 6971c02d..452db610 100644 --- a/go/internal/runtime/boot_canary_test.go +++ b/go/internal/runtime/boot_canary_test.go @@ -87,6 +87,7 @@ type canaryLaunchRecorder struct { pssErr error shutdownErr error launchErr error + onLaunch func(shared string) vms []*canaryFakeVM calls int lastDeadline time.Time @@ -109,6 +110,9 @@ func (r *canaryLaunchRecorder) launch(ctx context.Context, cfg microvm.BootConfi } vm := &canaryFakeVM{nonce: nonce, pss: r.pss, pssErr: r.pssErr, shutdownErr: r.shutdownErr} r.vms = append(r.vms, vm) + if r.onLaunch != nil { + r.onLaunch(cfg.FSSharedDir) + } return vm, nil } @@ -420,10 +424,10 @@ func TestBootCanaryPartialPSSStillReported(t *testing.T) { // path the gate protects. The named-return + errors.Join idiom is the kind a // "tidy the error path" refactor flattens to a plain defer; this test breaks on // that. (The sibling throwaway-workspace RemoveAll join at BootCanary's other -// defer uses the identical named-return errors.Join mechanism this proves, but -// cannot be reddened deterministically without a production temp-dir seam — -// os.TempDir() is not writable to force RemoveAll to fail regardless of uid — so -// it rides the shared mechanism proof rather than a root-sensitive test.) +// defer uses the identical named-return errors.Join mechanism; it has its own +// regression test, TestBootCanaryWorkspaceCleanupErrorJoined, which reddens that +// leg through the existing launch seam by locking a subtree in the minted +// workspace so os.RemoveAll fails EACCES.) func TestBootCanaryTeardownErrorJoined(t *testing.T) { before := canaryTempDirs(t) m, rec, _ := seamCanary(t, map[string]int64{"cloud-hypervisor": 100}) @@ -452,6 +456,79 @@ func TestBootCanaryTeardownErrorJoined(t *testing.T) { assertNoTempLeak(t, before) } +// TestBootCanaryBootAndTeardownErrorsBothJoined pins that the teardown join is a +// genuine errors.Join, not a plain overwrite: when the boot chain ITSELF fails +// (echo exec refused) AND teardown then also fails (Shutdown refused), BootCanary +// must return an error carrying BOTH. The boot diagnostic is what an operator +// debugging a failed canary needs, and a "tidy the error path" refactor that +// flattens the named-return errors.Join to `err = ` would silently +// discard it. The sibling TestBootCanaryTeardownErrorJoined drives an +// otherwise-successful canary, where err is nil at the join so overwrite and join +// are observationally identical; only a both-legs-fail case distinguishes them, +// and this test breaks on the overwrite (record §(e)/(f)). +func TestBootCanaryBootAndTeardownErrorsBothJoined(t *testing.T) { + before := canaryTempDirs(t) + m, rec, client := seamCanary(t, nil) + client.execErr = errors.New("boom: exec refused") // the BOOT failure + rec.shutdownErr = errors.New("boom: shutdown refused") // the TEARDOWN failure + + _, err := m.BootCanary(t.Context()) + if err == nil { + t.Fatal("BootCanary = nil, want both the boot and teardown failures joined") + } + if !strings.Contains(err.Error(), "exec refused") { + t.Errorf("BootCanary error = %v dropped the BOOT failure (teardown overwrote it instead of joining)", err) + } + if !strings.Contains(err.Error(), "shutdown refused") { + t.Errorf("BootCanary error = %v dropped the TEARDOWN failure", err) + } + assertNoTempLeak(t, before) +} + +// TestBootCanaryWorkspaceCleanupErrorJoined pins the SIBLING teardown join — the +// throwaway-workspace os.RemoveAll defer (microvm_preflight.go), which joins its +// error into the same named return. It reaches that leg through the existing +// launch seam: cfg.FSSharedDir IS the canary's minted workspace, so an onLaunch +// hook plants a 0555 subdir holding a file, making os.RemoveAll fail EACCES on the +// inner unlink. On an otherwise-successful canary, BootCanary must surface that +// cleanup failure; a regression dropping the join leaks a live virtio-fs share +// silently, and this test breaks on that. (Skipped as root, which bypasses the +// directory write bit — mirrors go/server/socket_test.go.) +func TestBootCanaryWorkspaceCleanupErrorJoined(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root bypasses the directory write bit, so os.RemoveAll would not fail with EACCES") + } + m, rec, _ := seamCanary(t, map[string]int64{"cloud-hypervisor": 100}) + rec.onLaunch = func(shared string) { + locked := filepath.Join(shared, "locked") + if err := os.Mkdir(locked, 0o755); err != nil { + t.Errorf("planting locked subdir: %v", err) + return + } + if err := os.WriteFile(filepath.Join(locked, "pinned"), []byte("x"), 0o600); err != nil { + t.Errorf("planting pinned file: %v", err) + return + } + if err := os.Chmod(locked, 0o555); err != nil { + t.Errorf("locking subdir: %v", err) + return + } + // Restore write so the deliberately-leaked workspace is cleaned up after. + t.Cleanup(func() { + _ = os.Chmod(locked, 0o755) + _ = os.RemoveAll(shared) + }) + } + + _, err := m.BootCanary(t.Context()) + if err == nil { + t.Fatal("BootCanary = nil, want a non-nil error carrying the workspace-cleanup failure") + } + if !strings.Contains(err.Error(), "removing throwaway workspace") { + t.Errorf("BootCanary error = %v, want it to carry the workspace-cleanup failure", err) + } +} + // TestBootCanaryDerivesDeadlineWhenCallerHasNone: with a deadline-less caller // ctx, BootCanary derives the internal canaryDeadline bound and threads it into // Start (so a wedged boot cannot hang Runner startup) — the launch ctx carries a From 7b3a8aa13ef30d60a5babd957fbfd117e7117fca Mon Sep 17 00:00:00 2001 From: mintaka Date: Fri, 4 Sep 2026 13:12:11 -0400 Subject: [PATCH 3/3] test(runner): cover workspace-cleanup join against overwrite (RIG-3219) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-2 review of #860 found the symmetric twin of round-1's teardown-join gap: the throwaway-workspace RemoveAll join was covered against DROPPING the join but not against FLATTENING it to a plain overwrite. TestBootCanaryWorkspaceCleanupErrorJoined drives an otherwise-successful canary, so `err` is nil when that defer runs and `errors.Join(nil, x)` is observationally identical to `err = x` — the overwrite mutant survived. Since that defer is registered first it runs last (LIFO), so an overwrite there would discard both the boot diagnostic and the teardown-session error the sibling join preserves. - Add TestBootCanaryWorkspaceCleanupAndBootErrorsBothJoined: fail the boot chain (exec refused) AND the workspace RemoveAll (EACCES on a planted 0555 subtree) and assert the returned error carries both substrings. Mutation- verified: flattening the join at microvm_preflight.go:327 to a plain overwrite reddens ONLY this test; the drop-the-join sibling stays green. - Extract the 0555-subtree plant into a shared plantLockedSubtree helper so both workspace tests share one mechanism. - Correct the parity claim in TestBootCanaryTeardownErrorJoined's comment to name both sibling workspace tests. - Document the onLaunch-under-r.mu constraint on canaryLaunchRecorder, and pin the empty session table on the workspace-cleanup path with a rationale for the deliberately-absent temp-leak assertion. Test-only; the two production files are byte-identical to main. Refs RIG-3219 Co-authored-by: Matt Wilkinson --- go/internal/runtime/boot_canary_test.go | 75 +++++++++++++++++++++---- 1 file changed, 64 insertions(+), 11 deletions(-) diff --git a/go/internal/runtime/boot_canary_test.go b/go/internal/runtime/boot_canary_test.go index 452db610..cfe00a81 100644 --- a/go/internal/runtime/boot_canary_test.go +++ b/go/internal/runtime/boot_canary_test.go @@ -82,11 +82,13 @@ var _ guestVM = (*canaryFakeVM)(nil) // the deadline the launch ctx carried (for the ctx-derivation assertions). A // non-nil launchErr makes launch fail (the Start-failure case). type canaryLaunchRecorder struct { - mu sync.Mutex - pss map[string]int64 - pssErr error - shutdownErr error - launchErr error + mu sync.Mutex + pss map[string]int64 + pssErr error + shutdownErr error + launchErr error + // onLaunch fires under r.mu: it must not call back into the recorder + // (snapshot/vms) or it deadlocks on the non-reentrant sync.Mutex. onLaunch func(shared string) vms []*canaryFakeVM calls int @@ -425,9 +427,10 @@ func TestBootCanaryPartialPSSStillReported(t *testing.T) { // "tidy the error path" refactor flattens to a plain defer; this test breaks on // that. (The sibling throwaway-workspace RemoveAll join at BootCanary's other // defer uses the identical named-return errors.Join mechanism; it has its own -// regression test, TestBootCanaryWorkspaceCleanupErrorJoined, which reddens that -// leg through the existing launch seam by locking a subtree in the minted -// workspace so os.RemoveAll fails EACCES.) +// regression tests — TestBootCanaryWorkspaceCleanupErrorJoined pins the drop-the-join +// case and TestBootCanaryWorkspaceCleanupAndBootErrorsBothJoined pins join-not-overwrite +// — both reaching that leg through the existing launch seam by locking a subtree in +// the minted workspace so os.RemoveAll fails EACCES.) func TestBootCanaryTeardownErrorJoined(t *testing.T) { before := canaryTempDirs(t) m, rec, _ := seamCanary(t, map[string]int64{"cloud-hypervisor": 100}) @@ -499,7 +502,31 @@ func TestBootCanaryWorkspaceCleanupErrorJoined(t *testing.T) { t.Skip("root bypasses the directory write bit, so os.RemoveAll would not fail with EACCES") } m, rec, _ := seamCanary(t, map[string]int64{"cloud-hypervisor": 100}) - rec.onLaunch = func(shared string) { + rec.onLaunch = plantLockedSubtree(t) + + _, err := m.BootCanary(t.Context()) + if err == nil { + t.Fatal("BootCanary = nil, want a non-nil error carrying the workspace-cleanup failure") + } + if !strings.Contains(err.Error(), "removing throwaway workspace") { + t.Errorf("BootCanary error = %v, want it to carry the workspace-cleanup failure", err) + } + // No assertNoTempLeak here: this test's mechanism IS an unremovable workspace + // (plantLockedSubtree's t.Cleanup reclaims it instead). The session table must + // still be empty — the teardown ran to completion despite the cleanup error. + if n := sessionCount(m); n != 0 { + t.Errorf("session table = %d after teardown, want 0", n) + } +} + +// plantLockedSubtree returns an onLaunch hook that plants a 0555 subdir holding +// a file inside the canary's minted workspace, so os.RemoveAll fails EACCES on +// the inner unlink — the mechanism both workspace-cleanup tests share. It +// registers a t.Cleanup that restores write and removes the deliberately-leaked +// workspace afterward. +func plantLockedSubtree(t *testing.T) func(shared string) { + t.Helper() + return func(shared string) { locked := filepath.Join(shared, "locked") if err := os.Mkdir(locked, 0o755); err != nil { t.Errorf("planting locked subdir: %v", err) @@ -519,13 +546,39 @@ func TestBootCanaryWorkspaceCleanupErrorJoined(t *testing.T) { _ = os.RemoveAll(shared) }) } +} + +// TestBootCanaryWorkspaceCleanupAndBootErrorsBothJoined pins that the +// throwaway-workspace RemoveAll join is a genuine errors.Join, not a plain +// overwrite — the symmetric partner to TestBootCanaryBootAndTeardownErrorsBothJoined +// for the SIBLING leg. Because the workspace-cleanup defer is registered FIRST it +// runs LAST (LIFO), so it holds the most accumulated error to destroy: an +// overwrite there would discard both the boot diagnostic AND the teardown-session +// error, silently un-doing the sibling join's coverage from the outside. When the +// boot chain fails (echo exec refused) AND the workspace RemoveAll then fails +// (EACCES on the planted 0555 subtree), BootCanary must return an error carrying +// BOTH. TestBootCanaryWorkspaceCleanupErrorJoined drives an otherwise-successful +// canary where err is nil at that join, so overwrite and join are observationally +// identical; only this both-legs-fail case distinguishes them (record §(e)/(f)). +// (Skipped as root, which bypasses the directory write bit — mirrors +// go/server/socket_test.go.) +func TestBootCanaryWorkspaceCleanupAndBootErrorsBothJoined(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root bypasses the directory write bit, so os.RemoveAll would not fail with EACCES") + } + m, rec, client := seamCanary(t, map[string]int64{"cloud-hypervisor": 100}) + client.execErr = errors.New("boom: exec refused") // the BOOT failure + rec.onLaunch = plantLockedSubtree(t) // the CLEANUP failure _, err := m.BootCanary(t.Context()) if err == nil { - t.Fatal("BootCanary = nil, want a non-nil error carrying the workspace-cleanup failure") + t.Fatal("BootCanary = nil, want both the boot and workspace-cleanup failures joined") + } + if !strings.Contains(err.Error(), "exec refused") { + t.Errorf("BootCanary error = %v dropped the BOOT failure (workspace cleanup overwrote it instead of joining)", err) } if !strings.Contains(err.Error(), "removing throwaway workspace") { - t.Errorf("BootCanary error = %v, want it to carry the workspace-cleanup failure", err) + t.Errorf("BootCanary error = %v dropped the workspace-cleanup failure", err) } }