diff --git a/go/internal/runtime/boot_canary_test.go b/go/internal/runtime/boot_canary_test.go index 8234f704..cfe00a81 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 } @@ -80,10 +82,14 @@ 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 - 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 lastDeadline time.Time @@ -104,8 +110,11 @@ 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) + if r.onLaunch != nil { + r.onLaunch(cfg.FSSharedDir) + } return vm, nil } @@ -405,6 +414,174 @@ 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; it has its own +// 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}) + 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) +} + +// 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 = 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) + 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) + }) + } +} + +// 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 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 dropped 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