test(runner): cover BootCanary teardown-error-join contract (RIG-3219)

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 = <teardown>` 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