feat(microvm): host-written per-session pidfiles with PID-reuse defense (RIG-2498)

go/internal/runtime/microvm/boot_microvm_test.go

@@ -20,6 +20,7 @@ package microvm
 import (
 	"context"
 	"os"
+	"path/filepath"
 	"strings"
 	"testing"
 	"time"
@@ -117,7 +118,7 @@ func TestNetOnlyBootSmoke(t *testing.T) {
 // 1024 MB), complete the Health handshake with net_provisioned &&
 // workspace_mounted inside the 60 s deadline, record boot latency and
 // per-process PSS, and verify Shutdown leaves no orphan process and removes the
-// unix sockets.
+// unix sockets and the three pidfiles.
 func TestFullBoot(t *testing.T) {
 	env := microvmtest.Require(t)
 	cfg := bootConfig(t, env, 2, 1024)
@@ -143,6 +144,14 @@ func TestFullBoot(t *testing.T) {
 				t.Errorf("socket %s not removed by Shutdown", s)
 			}
 		}
+		// And the three pidfiles: a record must not outlive the session it
+		// names, or the reaper would find a dir full of dead records for a
+		// session that shut down cleanly.
+		for _, p := range vm.pidfiles {
+			if fileExists(p) {
+				t.Errorf("pidfile %s not removed by Shutdown", p)
+			}
+		}
 	})
 
 	// Poll Health until the guest reports both flags true, bounded by the boot
@@ -185,6 +194,70 @@ func TestFullBoot(t *testing.T) {
 	}
 }
 
+// TestFullBootRecordsSettledPidfiles is W1's KVM assertion (§(a)): after a real
+// Launch the runtime dir must hold a SETTLED record for each of the three
+// children — no intent line left behind — and each record must name a process
+// that is actually live with a matching starttime. That is exactly the input
+// the orphan reaper acts on, so the hermetic tier's coverage of the primitives
+// is not enough: this pins that launch wires them to the REAL pids of the REAL
+// children, under the real spawn ordering (virtiofsd, passt, then the VMM).
+func TestFullBootRecordsSettledPidfiles(t *testing.T) {
+	env := microvmtest.Require(t)
+	cfg := bootConfig(t, env, 2, 1024)
+
+	vm, err := Launch(t.Context(), cfg)
+	if err != nil {
+		t.Fatalf("Launch failed: %v", err)
+	}
+
+	dir := filepath.Dir(cfg.Net.VhostUserSocket)
+	// The pid each name is expected to carry, from the child handles launch
+	// built — so a record naming SOME live process (or another child's pid)
+	// cannot pass.
+	wantPIDs := map[string]int{
+		"vmm.pid":       vm.vmm.cmd.Process.Pid,
+		"virtiofsd.pid": vm.virtiofsd.cmd.Process.Pid,
+		"passt.pid":     vm.passt.cmd.Process.Pid,
+	}
+	paths := make([]string, 0, len(wantPIDs))
+	for name, wantPID := range wantPIDs {
+		path := filepath.Join(dir, name)
+		paths = append(paths, path)
+
+		rec, readErr := readPidfile(path)
+		if readErr != nil {
+			t.Errorf("readPidfile(%s): %v", name, readErr)
+			continue
+		}
+		if rec.Intent {
+			t.Errorf("%s still holds an intent record after Launch; the settled write did not land", name)
+			continue
+		}
+		if rec.PID != wantPID {
+			t.Errorf("%s records pid %d, want the child's %d", name, rec.PID, wantPID)
+		}
+		// Liveness via the recorded identity, not a bare signal: this is the
+		// starttime comparison the reaper makes before it kills anything.
+		alive, aliveErr := rec.alive()
+		if aliveErr != nil {
+			t.Errorf("%s alive(): %v", name, aliveErr)
+			continue
+		}
+		if !alive {
+			t.Errorf("%s does not name a live process with a matching starttime (%+v)", name, rec)
+		}
+	}
+
+	if shutErr := vm.Shutdown(context.WithoutCancel(t.Context())); shutErr != nil {
+		t.Fatalf("Shutdown: %v", shutErr)
+	}
+	for _, path := range paths {
+		if fileExists(path) {
+			t.Errorf("pidfile %s survived Shutdown", path)
+		}
+	}
+}
+
 // armRuleset is a minimal but representative in-guest egress arm: it creates the
 // inet table + a conntrack-stateful output rule, forcing a representative slice
 // of the netfilter autoload chain — the NETLINK_NETFILTER socket (nfnetlink),

go/internal/runtime/microvm/launch.go

@@ -108,7 +108,7 @@ func (c *child) hasExited() bool {
 
 // VM is a running (or partially-started, on the Launch error path) guest and
 // its two supporting host daemons. It owns their process handles, the captured
-// serial console + per-daemon logs, and the AF_UNIX sockets/pidfile that must
+// serial console + per-daemon logs, and the AF_UNIX sockets/pidfiles that must
 // be removed on teardown. The zero devices (virtiofsd nil under the net-only
 // smoke) are tolerated by Shutdown and PSS.
 type VM struct {
@@ -127,10 +127,13 @@ type VM struct {
 	vsockSocket string // host end of the hybrid vsock (empty under the net-only smoke)
 	vsockPort   uint32
 
-	// Cleanup targets: the AF_UNIX sockets the daemons/VMM serve and passt's
-	// pidfile. Removed by Shutdown after the processes are reaped.
-	sockets []string
-	pidfile string
+	// Cleanup targets: the AF_UNIX sockets the daemons/VMM serve and the three
+	// host-written pidfiles (§(a)). Removed by Shutdown after the processes are
+	// reaped. A pidfile path is appended by startRecordedChild BEFORE its first
+	// write, so a boot that fails between the intent record and the settled one
+	// still has its record cleaned up by the deferred Shutdown.
+	sockets  []string
+	pidfiles []string
 
 	shutdownOnce sync.Once
 	shutdownErr  error
@@ -212,8 +215,8 @@ func launch(ctx context.Context, cfg BootConfig, opts launchOptions) (_ *VM, err
 			//nolint:gosec // G204: the microVM harness seam — virtiofsdPath is LookPath-resolved and the argv is harness-built from BootConfig, neither user-controlled
 			cmd: exec.CommandContext(ctx, virtiofsdPath, virtiofsdArgs(cfg, subUIDBase, subGIDBase)...),
 		}
-		if startErr := startChild(vm.virtiofsd); startErr != nil {
-			return nil, fmt.Errorf("microvm: starting virtiofsd: %w", startErr)
+		if startErr := vm.startRecordedChild(vm.virtiofsd, dir, "virtiofsd.pid"); startErr != nil {
+			return nil, startErr
 		}
 		vm.sockets = append(vm.sockets, cfg.FSSocket)
 	}
@@ -222,27 +225,31 @@ func launch(ctx context.Context, cfg BootConfig, opts launchOptions) (_ *VM, err
 	if lookErr != nil {
 		return nil, fmt.Errorf("microvm: resolving passt on PATH: %w", lookErr)
 	}
-	vm.pidfile = filepath.Join(dir, "passt.pid")
 	vm.passt = &child{
 		name:    "passt",
 		logPath: filepath.Join(dir, "passt.log"),
 		// -f keeps passt in the foreground so this *exec.Cmd IS the passt
 		// process (default is to daemonize, which would orphan it and make the
 		// Cmd exit immediately). The -a/-g/-n/-D flags fix the host-controlled
 		// address plan passt serves over DHCP (§(c)).
+		//
+		// NO --pid: passt's self-written pidfile is retired (§(a)). Because -f
+		// makes this Cmd the passt process, the host knows passt's pid exactly
+		// as it knows the other two, and a host-written record can carry the
+		// starttime+boot-id reuse defense that a daemon's own bare-pid file
+		// cannot. One writer, one format, three files.
 		//nolint:gosec // G204: the microVM harness seam — passtPath is LookPath-resolved and the argv is harness-built (fixed flags + BootConfig socket), neither user-controlled
 		cmd: exec.CommandContext(ctx, passtPath,
 			"--vhost-user",
 			"--socket", cfg.Net.VhostUserSocket,
-			"--pid", vm.pidfile,
 			"-f",
 			"-a", guestAddr,
 			"-g", guestGW,
 			"-n", guestPrefix,
 			"-D", guestDNS),
 	}
-	if startErr := startChild(vm.passt); startErr != nil {
-		return nil, fmt.Errorf("microvm: starting passt: %w", startErr)
+	if startErr := vm.startRecordedChild(vm.passt, dir, "passt.pid"); startErr != nil {
+		return nil, startErr
 	}
 	vm.sockets = append(vm.sockets, cfg.Net.VhostUserSocket)
 
@@ -282,8 +289,8 @@ func launch(ctx context.Context, cfg BootConfig, opts launchOptions) (_ *VM, err
 		//nolint:gosec // G204: the microVM harness seam — vmmPath is LookPath-resolved and vmmArgs is harness-built from BootConfig, neither user-controlled
 		cmd: exec.CommandContext(ctx, vmmPath, vmmArgs(cfg, vm.consolePath, opts)...),
 	}
-	if startErr := startChild(vm.vmm); startErr != nil {
-		return nil, fmt.Errorf("microvm: starting cloud-hypervisor: %w", startErr)
+	if startErr := vm.startRecordedChild(vm.vmm, dir, "vmm.pid"); startErr != nil {
+		return nil, startErr
 	}
 	// startChild installed the sole reaper that owns cloud-hypervisor's single
 	// cmd.Wait; hoist its channel onto the VM so WaitVMMExit observes a guest
@@ -557,20 +564,37 @@ func (vm *VM) Health(ctx context.Context) (*compassv1.HealthResponse, error) {
 // Shutdown tears the guest and its daemons down: the VMM is killed first (a VM
 // gets no graceful drain), then virtiofsd and passt are reaped (SIGTERM, a
 // bounded wait, then SIGKILL), each Wait'd to avoid zombies, and finally the
-// AF_UNIX sockets and passt's pidfile are removed. It runs at most once (guarded
-// by sync.Once) so it is safe to call explicitly AND from t.Cleanup. The serial
-// console log is deliberately NOT removed — the test reads it after teardown.
+// AF_UNIX sockets and the three pidfiles are removed. It runs at most once
+// (guarded by sync.Once) so it is safe to call explicitly AND from t.Cleanup.
+// The serial console log is deliberately NOT removed — the test reads it after
+// teardown.
 func (vm *VM) Shutdown(ctx context.Context) error {
 	vm.shutdownOnce.Do(func() {
 		var errs []error
 		// VMM first: kill outright, then let the sole reaper's single Wait
 		// complete via vmmExited (Shutdown must not Wait the VMM itself — that
 		// would be a second Wait on the same process).
+		//
+		// The RECEIVE carries its own nil guard, because vmmExited is hoisted
+		// onto the VM only AFTER startRecordedChild returns: a
+		// startRecordedChild that fails once startChild has already spawned
+		// the VMM leaves a live process handle beside a NIL channel, and
+		// launch's deferred Shutdown runs in exactly that state. Receiving on
+		// the nil channel there blocks FOREVER — a hang with no stack and no
+		// diagnostic, replacing the launch error it was cleaning up after. It
+		// is the same nil-channel condition WaitVMMExit guards on.
+		//
+		// The guard is on the receive alone and NOT on the arm: such a process
+		// is started, live, and unrecorded, so skipping the Kill would orphan
+		// precisely the child the deferred Shutdown exists to clean up. Its
+		// own reaper still owns the Wait, so nothing is left unreaped.
 		if vm.vmm != nil && vm.vmm.cmd.Process != nil {
 			if killErr := vm.vmm.cmd.Process.Kill(); killErr != nil && !errors.Is(killErr, os.ErrProcessDone) {
 				errs = append(errs, fmt.Errorf("killing cloud-hypervisor: %w", killErr))
 			}
-			<-vm.vmmExited
+			if vm.vmmExited != nil {
+				<-vm.vmmExited
+			}
 		}
 		// Then the auxiliary daemons: SIGTERM, bounded wait, SIGKILL.
 		for _, c := range []*child{vm.virtiofsd, vm.passt} {
@@ -581,15 +605,15 @@ func (vm *VM) Shutdown(ctx context.Context) error {
 				errs = append(errs, reapErr)
 			}
 		}
-		// Remove the sockets and pidfile now that nothing is serving them.
+		// Remove the sockets and pidfiles now that nothing is serving them.
 		for _, s := range vm.sockets {
 			if rmErr := os.Remove(s); rmErr != nil && !errors.Is(rmErr, os.ErrNotExist) {
 				errs = append(errs, fmt.Errorf("removing socket %s: %w", s, rmErr))
 			}
 		}
-		if vm.pidfile != "" {
-			if rmErr := os.Remove(vm.pidfile); rmErr != nil && !errors.Is(rmErr, os.ErrNotExist) {
-				errs = append(errs, fmt.Errorf("removing pidfile %s: %w", vm.pidfile, rmErr))
+		for _, p := range vm.pidfiles {
+			if rmErr := os.Remove(p); rmErr != nil && !errors.Is(rmErr, os.ErrNotExist) {
+				errs = append(errs, fmt.Errorf("removing pidfile %s: %w", p, rmErr))
 			}
 		}
 		vm.shutdownErr = errors.Join(errs...)
@@ -763,3 +787,56 @@ func (vm *VM) childByName(name string) *child {
 		return nil
 	}
 }
+
+// startRecordedChild records c's on-disk identity around its spawn, per §(a)'s
+// two-step write: the pre-spawn INTENT record, then startChild, then the
+// SETTLED record naming the pid the spawn returned. pidfileName is the file's
+// name within the session runtime dir (vmm.pid/virtiofsd.pid/passt.pid).
+//
+// Either write failing fails the boot: orphan reapability is load-bearing, so a
+// session that cannot be recorded must not run. launch's deferred vm.Shutdown
+// tears down whatever already started and removes the paths registered here.
+func (vm *VM) startRecordedChild(c *child, dir, pidfileName string) error {
+	path := filepath.Join(dir, pidfileName)
+	// Registered before the first write, not after the last: the intent record
+	// exists precisely to survive a crash between the two writes, so Shutdown
+	// must already know to remove it if the spawn in between fails.
+	vm.pidfiles = append(vm.pidfiles, path)
+	if err := writePidIntent(path); err != nil {
+		return fmt.Errorf("microvm: recording %s pid intent: %w", c.name, err)
+	}
+	if err := startChild(c); err != nil {
+		return fmt.Errorf("microvm: starting %s: %w", c.name, err)
+	}
+	if err := writePidfile(path, c.cmd.Process.Pid); err != nil {
+		return pidfileWriteError(c, err)
+	}
+	return nil
+}
+
+// pidfileWriteError shapes a writePidfile failure from startRecordedChild by
+// its actual cause. writePidfile's second step reads /proc/<pid>/stat, and a
+// child that exits fast can be reaped between the spawn and that read — so the
+// read fails for a reason that is NOT a pidfile fault: the child is simply
+// dead. Reporting a /proc path there buries the real cause, which is in the
+// daemon's own log, so a confirmed-dead child gets the SAME error shape launch
+// uses for a daemon that died before the VMM was started (name the daemon,
+// wrap waitResult, carry the log tail).
+//
+// BOTH conditions are required: procReadMeansGone alone could describe a /proc
+// that vanished under a still-live pid (it cannot, but the pairing makes the
+// claim "this child is dead" one the reaper's channel actually proves), and
+// hasExited alone would swallow a genuine write fault on a child that merely
+// happens to have exited. Every other writePidfile failure stays fatal as a
+// pidfile error, because orphan reapability is load-bearing.
+//
+// The on-disk record is deliberately NOT touched on the dead-child branch: the
+// intent record stands and the path stays registered, so Shutdown removes it
+// and no window exists where the dir under-names a child that may have run.
+func pidfileWriteError(c *child, err error) error {
+	if procReadMeansGone(err) && c.hasExited() {
+		return fmt.Errorf("microvm: %s exited before its pidfile could be recorded: %w; log tail:\n%s",
+			c.name, waitResult(c.name, c.waitErr), tailFile(c.logPath))
+	}
+	return fmt.Errorf("microvm: recording %s pidfile: %w", c.name, err)
+}

go/internal/runtime/microvm/launch_teardown_test.go

@@ -43,14 +43,25 @@ func TestLaunchFailClosedTeardown(t *testing.T) {
 	vfsPidFile := filepath.Join(run, "vfs.pid")
 	passtPidFile := filepath.Join(run, "passt.pid")
 
+	// The fakes must OUTLIVE launch, and the only absence this test wants is
+	// cloud-hypervisor's — so `sleep` is resolved on the real PATH now, while it
+	// is still intact, and burned into the stubs as an absolute path. Naming it
+	// bare would make the stubs die on `sleep: command not found` the moment
+	// PATH is narrowed below (there is no `sleep` builtin in /bin/sh), so both
+	// daemons would exit on their own and "teardown reaped them" would be
+	// trivially true — the RIG-3480 defect.
+	sleepBin, err := exec.LookPath("sleep")
+	if err != nil {
+		t.Fatalf("resolving sleep on PATH (the fakes need it to stay alive): %v", err)
+	}
 	// virtiofsd: record pid, touch its --socket-path=, then stay alive.
 	writeFake(t, bin, "virtiofsd", `echo $$ > `+vfsPidFile+`
 for a in "$@"; do case "$a" in --socket-path=*) : > "${a#--socket-path=}";; esac; done
-sleep 30`)
+`+sleepBin+` 30`)
 	// passt: record pid, touch the path following --socket, then stay alive.
 	writeFake(t, bin, "passt", `echo $$ > `+passtPidFile+`
 p=""; for a in "$@"; do [ "$p" = --socket ] && : > "$a"; p="$a"; done
-sleep 30`)
+`+sleepBin+` 30`)
 	// cloud-hypervisor deliberately absent → LookPath fails after aux are up.
 	t.Setenv("PATH", bin)
 
@@ -71,8 +82,20 @@ sleep 30`)
 		t.Errorf("expected a nil VM on the error path, got %v", vm)
 	}
 
+	// The failure must be cloud-hypervisor's LookPath, which is the ONLY absence
+	// this test arranges. Anything earlier — notably a fake that exited before
+	// its socket was served — means the daemons never really came up, so the
+	// no-orphan assertion below would pass vacuously. This control is what
+	// caught RIG-3480: it fired on 39/40 runs of the merge result while the
+	// assertion it guards stayed green.
+	if !strings.Contains(err.Error(), "cloud-hypervisor") {
+		t.Fatalf("launch failed before the cloud-hypervisor lookup (%v); the aux daemons never came up, "+
+			"so the no-orphan assertion below would be vacuous", err)
+	}
+
 	// The daemons launch already started must have been reaped by the deferred
-	// cleanup — no orphan left sleeping.
+	// cleanup — no orphan left sleeping. Shutdown's reap Waits each child, so by
+	// the time Launch has returned this is a settled fact, not a race to poll.
 	for name, pidFile := range map[string]string{"virtiofsd": vfsPidFile, "passt": passtPidFile} {
 		pid := readPidFile(t, pidFile)
 		if pidAlive(pid) {
@@ -209,21 +232,21 @@ func TestWaitForSocketsSucceedsForALiveDaemon(t *testing.T) {
 	}
 }
 
-// readPidFile reads a pid a fake wrote, retrying briefly since the fake writes
-// it asynchronously after exec.
+// readPidFile reads the pid a fake recorded. It does NOT poll: each fake writes
+// its pid BEFORE it touches its socket, and the caller has already confirmed
+// launch got past the socket wait, which orders the write before this read. So a
+// missing or malformed pidfile here is a real defect in that ordering rather
+// than a race to wait out, and it fails immediately instead of behind a
+// wall-clock budget that can only ever expire (RIG-3480).
 func readPidFile(t *testing.T, path string) int {
 	t.Helper()
-	deadline := time.Now().Add(2 * time.Second)
-	for {
-		raw, err := os.ReadFile(path)
-		if err == nil {
-			if pid, convErr := strconv.Atoi(strings.TrimSpace(string(raw))); convErr == nil {
-				return pid
-			}
-		}
-		if time.Now().After(deadline) {
-			t.Fatalf("fake never wrote its pid to %s", path)
-		}
-		time.Sleep(10 * time.Millisecond) //nolint:forbidigo // bounded poll tick; event-gated on the fake writing its pidfile above with a deadline (rule://go-no-sleep-in-test poll-until exemption)
+	raw, err := os.ReadFile(path)
+	if err != nil {
+		t.Fatalf("reading the pid the fake recorded at %s: %v", path, err)
+	}
+	pid, err := strconv.Atoi(strings.TrimSpace(string(raw)))
+	if err != nil {
+		t.Fatalf("pidfile %s holds %q, not a pid: %v", path, string(raw), err)
 	}
+	return pid
 }