From 69c1252c9475df1d23bf9b1dfd2368cafa4091ab Mon Sep 17 00:00:00 2001 From: mintaka Date: Mon, 7 Sep 2026 13:54:26 -0400 Subject: [PATCH 1/2] fix(microvm): stop the teardown fakes leaking their tail (RIG-3480) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 1 on #978. The sleep repair stopped the fakes dying early, but made them leak: /bin/sh forks a bare tail and lives on as its parent, so the pid each fake records with echo $$ is the SHELL, not the process that stays alive. reap sends SIGTERM to that shell, the recorded pid dies, the no-orphan assertion passes — and the tail is reparented to init and survives its full 30 seconds. Measured +10 leaked processes per -count=5 run, every one with PPID 1. Before this PR the fakes died instantly on a missing sleep, so the fixture leaked nothing; the repair introduced the leak and the assertion could not see it. exec on the tail fixes both halves: there is no grandchild to orphan, and the recorded pid now names the process that actually stays alive, which is what the assertion has to probe for "no orphan left sleeping" to mean anything. Measured 0 leaked processes after, and the leak returns when exec is removed. The exec now lives in one place, longLivedFakeBody, rather than being spelled in each stub literal, with a test asserting the tail it builds starts with exec. Dropping the keyword fails that test 5/5 while the launch test carries on passing — which is the point: nothing else in the suite can see this leak, because a reparented orphan is unreachable from the dead pid it came from, and a readiness signal a stub emits necessarily precedes its own exec, so there is no happens-before edge to observe the exec on a live process without polling. The invariant is therefore checked where it is established, in the string every fake is built from. Also tighten the non-vacuity control to match "resolving cloud-hypervisor on PATH" rather than the bare binary name. The exited-before-start check that runs between waitForSockets and the lookup also embeds "cloud-hypervisor" in its message, so the bare-name match admitted precisely the dead-daemon case the control exists to reject. And correct a comment that named the wrong mechanism: reap does not Wait each child. It blocks on the child reaper channel, and startChild s sole reaper owns the single cmd.Wait. The conclusion was right and the stated reason was not, which matters here because the surrounding code warns at length that a second concurrent Wait races. --- .../runtime/microvm/launch_teardown_test.go | 66 +++++++++++++++---- 1 file changed, 55 insertions(+), 11 deletions(-) diff --git a/go/internal/runtime/microvm/launch_teardown_test.go b/go/internal/runtime/microvm/launch_teardown_test.go index 10b434f5a..3ecdf44cb 100644 --- a/go/internal/runtime/microvm/launch_teardown_test.go +++ b/go/internal/runtime/microvm/launch_teardown_test.go @@ -54,14 +54,16 @@ func TestLaunchFailClosedTeardown(t *testing.T) { 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 -`+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 -`+sleepBin+` 30`) + // Both fakes record their pid, touch the socket launch waits on, then stay + // alive as `sleep`. longLivedFakeBody owns the `exec` that makes the + // recorded pid name the surviving process rather than a shell parenting it; + // see its doc for why that is load-bearing rather than style. + writeFake(t, bin, "virtiofsd", longLivedFakeBody(vfsPidFile, + `for a in "$@"; do case "$a" in --socket-path=*) : > "${a#--socket-path=}";; esac; done`, + sleepBin, "30")) + writeFake(t, bin, "passt", longLivedFakeBody(passtPidFile, + `p=""; for a in "$@"; do [ "$p" = --socket ] && : > "$a"; p="$a"; done`, + sleepBin, "30")) // cloud-hypervisor deliberately absent → LookPath fails after aux are up. t.Setenv("PATH", bin) @@ -88,14 +90,21 @@ p=""; for a in "$@"; do [ "$p" = --socket ] && : > "$a"; p="$a"; done // 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") { + // + // Match the lookup's own wording, not the bare binary name: the + // exited-before-start check that runs between waitForSockets and the lookup + // also embeds "cloud-hypervisor" in its message, so a bare-name match would + // admit precisely the dead-daemon case this control exists to reject. + if !strings.Contains(err.Error(), "resolving cloud-hypervisor on PATH") { 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. Shutdown's reap Waits each child, so by - // the time Launch has returned this is a settled fact, not a race to poll. + // cleanup — no orphan left sleeping. Shutdown's reap blocks on each child's + // reaper channel before returning (it does NOT Wait — startChild's sole + // reaper owns the single cmd.Wait), 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) { @@ -104,6 +113,41 @@ p=""; for a in "$@"; do [ "$p" = --socket ] && : > "$a"; p="$a"; done } } +// longLivedFakeBody builds the body of a shell stub that records its pid, runs +// setup, and then STAYS ALIVE as the given command. +// +// The `exec` is the point of this helper and is why the tail is not written by +// hand at the callsites. Without it /bin/sh forks the tail and lives on as its +// parent, so `echo $$` records the SHELL: teardown's SIGTERM then kills the +// recorded pid, a no-orphan assertion keyed on that pid passes, and the tail is +// reparented to init and survives its full lifetime. Measured at +10 leaked +// processes per -count=5 run of TestLaunchFailClosedTeardown before the exec +// was added, 0 after. Keeping the spelling in one place means a future edit +// cannot reintroduce that leak by dropping a keyword from a string literal. +func longLivedFakeBody(pidFile, setup, tailBin string, tailArgs ...string) string { + tail := strings.Join(append([]string{tailBin}, tailArgs...), " ") + return "echo $$ > " + pidFile + "\n" + setup + "\nexec " + tail +} + +// TestLongLivedFakeBodyExecsItsTail pins the property above at the point it is +// decided. Asserting it on a running stub is not possible without a poll: the +// only observable difference is /proc//comm after the exec, and there is +// no happens-before edge to that moment — a readiness signal the stub emits +// necessarily precedes its own exec. So the invariant is checked where it is +// actually established, in the string every fake is built from. +func TestLongLivedFakeBodyExecsItsTail(t *testing.T) { + body := longLivedFakeBody("/tmp/x.pid", ": > /tmp/sock", "/bin/sleep", "30") + lines := strings.Split(body, "\n") + tail := lines[len(lines)-1] + if !strings.HasPrefix(tail, "exec ") { + t.Errorf("stub tail is %q, want an `exec ` prefix — without it the shell forks the tail and "+ + "teardown cannot reach the process that stays alive", tail) + } + if !strings.HasPrefix(body, "echo $$ > /tmp/x.pid\n") { + t.Errorf("stub must record its pid first, got %q", body) + } +} + // TestWaitVMMExitObservesPromptSelfExit pins M2's prompt-exit contract: a VMM // that exits on its own (as the guest does on RB_POWER_OFF) is observed by // WaitVMMExit via the sole reaper WELL UNDER the grace window — it must NOT burn From 61feaed4fea7e3c58111c9983a9db2755bc54ac4 Mon Sep 17 00:00:00 2001 From: mintaka Date: Tue, 15 Sep 2026 23:20:48 -0400 Subject: [PATCH 2/2] fix(microvm): exec the third long-lived stub too (RIG-3480) Review round 2. The previous fold claimed centralizing the exec meant a future edit could not reintroduce the leak. That claim was false: a third long-lived stub, in TestWaitForSocketsSucceedsForALiveDaemon, builds its body by hand and never reached the constructor, so it had no keyword to drop. It does not leak today, but only because `sh -c` implicitly execs its LAST command, which that stub happens to satisfy. Measured: the exact form runs with the sleep already as the direct child (children list empty, no survivor after SIGTERM), while appending one trailing command forks and leaves the sleep reparented to init - the identical round-1 defect, invisible to every test in the package. So the exec now lives in longLivedStayAlive, which all three stubs go through, with longLivedFakeBody delegating to it for the two that also record a pid. Dropping the exec from that single function now fails the guard test 3/3 and covers the hand-written stub as well, where before it could only ever see the two callsites the constructor was written for. That stub also used a bare `sleep` off the ambient PATH while the rest of the file resolves an absolute path with exec.LookPath. It was safe, because that test does not narrow PATH, but two spellings of one hazard in one file is how the next reader picks the wrong one. It now resolves the absolute path like the others. --- .../runtime/microvm/launch_teardown_test.go | 88 +++++++++++++------ 1 file changed, 60 insertions(+), 28 deletions(-) diff --git a/go/internal/runtime/microvm/launch_teardown_test.go b/go/internal/runtime/microvm/launch_teardown_test.go index 3ecdf44cb..21908659c 100644 --- a/go/internal/runtime/microvm/launch_teardown_test.go +++ b/go/internal/runtime/microvm/launch_teardown_test.go @@ -113,38 +113,60 @@ func TestLaunchFailClosedTeardown(t *testing.T) { } } -// longLivedFakeBody builds the body of a shell stub that records its pid, runs -// setup, and then STAYS ALIVE as the given command. +// longLivedStayAlive builds shell text that runs setup and then STAYS ALIVE as +// the given command. It is the single place the `exec` is spelled, for every +// stub in this file that has to outlive the call under test. // -// The `exec` is the point of this helper and is why the tail is not written by -// hand at the callsites. Without it /bin/sh forks the tail and lives on as its -// parent, so `echo $$` records the SHELL: teardown's SIGTERM then kills the -// recorded pid, a no-orphan assertion keyed on that pid passes, and the tail is -// reparented to init and survives its full lifetime. Measured at +10 leaked -// processes per -count=5 run of TestLaunchFailClosedTeardown before the exec -// was added, 0 after. Keeping the spelling in one place means a future edit -// cannot reintroduce that leak by dropping a keyword from a string literal. -func longLivedFakeBody(pidFile, setup, tailBin string, tailArgs ...string) string { +// Without the exec, /bin/sh forks the tail and lives on as its parent. A +// teardown that signals the child it started then reaches only that shell, and +// the tail is reparented to init and survives its full lifetime — while the pid +// a caller recorded, or the pid Go handed it, IS reaped, so a no-orphan +// assertion keyed on it passes. Measured at +10 leaked processes per -count=5 +// run of TestLaunchFailClosedTeardown before the exec was added, 0 after. +// +// `sh -c` does exec its LAST command implicitly, so a stub whose sleep happens +// to be last leaks nothing today. That is an accident of the shell, not a +// property of the test: appending one trailing command restores the leak, and +// nothing in this package can observe it. Hence one helper rather than the +// spelling repeated per callsite. +func longLivedStayAlive(setup, tailBin string, tailArgs ...string) string { tail := strings.Join(append([]string{tailBin}, tailArgs...), " ") - return "echo $$ > " + pidFile + "\n" + setup + "\nexec " + tail + return setup + "\nexec " + tail +} + +// longLivedFakeBody is longLivedStayAlive plus the pid record the fail-closed +// teardown test reads back. +func longLivedFakeBody(pidFile, setup, tailBin string, tailArgs ...string) string { + return "echo $$ > " + pidFile + "\n" + longLivedStayAlive(setup, tailBin, tailArgs...) } -// TestLongLivedFakeBodyExecsItsTail pins the property above at the point it is -// decided. Asserting it on a running stub is not possible without a poll: the -// only observable difference is /proc//comm after the exec, and there is -// no happens-before edge to that moment — a readiness signal the stub emits -// necessarily precedes its own exec. So the invariant is checked where it is -// actually established, in the string every fake is built from. -func TestLongLivedFakeBodyExecsItsTail(t *testing.T) { - body := longLivedFakeBody("/tmp/x.pid", ": > /tmp/sock", "/bin/sleep", "30") - lines := strings.Split(body, "\n") - tail := lines[len(lines)-1] - if !strings.HasPrefix(tail, "exec ") { - t.Errorf("stub tail is %q, want an `exec ` prefix — without it the shell forks the tail and "+ - "teardown cannot reach the process that stays alive", tail) +// TestLongLivedStayAliveExecsItsTail pins the property above at the point it is +// decided, for both constructors. +// +// It cannot be asserted on a running stub without a poll. The only observable +// difference is /proc//comm after the exec, and there is no happens-before +// edge to that moment: a readiness signal a stub emits necessarily precedes its +// own exec. A post-teardown residue scan can see the leak itself, but it +// reports on whichever stubs a test happens to start; this reports on the +// string every long-lived stub in the file is built from, which is the property +// that actually has to hold. Both constructors are covered so neither can drift +// away from the other. +func TestLongLivedStayAliveExecsItsTail(t *testing.T) { + for name, body := range map[string]string{ + "longLivedStayAlive": longLivedStayAlive(": > /tmp/sock", "/bin/sleep", "30"), + "longLivedFakeBody": longLivedFakeBody("/tmp/x.pid", ": > /tmp/sock", "/bin/sleep", "30"), + } { + lines := strings.Split(body, "\n") + tail := lines[len(lines)-1] + if !strings.HasPrefix(tail, "exec ") { + t.Errorf("%s tail is %q, want an `exec ` prefix — without it the shell forks the tail "+ + "and teardown cannot reach the process that stays alive", name, tail) + } } - if !strings.HasPrefix(body, "echo $$ > /tmp/x.pid\n") { - t.Errorf("stub must record its pid first, got %q", body) + // The pid record must come first, since the teardown test reads it back. + if body := longLivedFakeBody("/tmp/x.pid", ": > /tmp/sock", "/bin/sleep", "30"); !strings.HasPrefix( + body, "echo $$ > /tmp/x.pid\n") { + t.Errorf("longLivedFakeBody must record its pid first, got %q", body) } } @@ -257,10 +279,20 @@ func TestWaitForSocketsFailsFastOnADeadDaemon(t *testing.T) { func TestWaitForSocketsSucceedsForALiveDaemon(t *testing.T) { dir := t.TempDir() socket := filepath.Join(dir, "live.sock") + sleepBin, err := exec.LookPath("sleep") + if err != nil { + t.Fatalf("resolving sleep on PATH (the fake needs it to stay alive): %v", err) + } + // Built with longLivedStayAlive for the same reason the launch fakes are: + // the tail must be exec'd, or this stub is a shell parenting the sleep and + // reap's SIGTERM orphans the sleep to init. `sh -c` happens to exec its + // LAST command implicitly, so the bare form leaked nothing — but appending + // one trailing command silently restores the leak, which no test here can + // see. Going through the helper makes it explicit instead of incidental. c := &child{ name: "virtiofsd", logPath: filepath.Join(dir, "virtiofsd.log"), - cmd: exec.CommandContext(t.Context(), "/bin/sh", "-c", ": > "+socket+"; sleep 30"), + cmd: exec.CommandContext(t.Context(), "/bin/sh", "-c", longLivedStayAlive(": > "+socket, sleepBin, "30")), } if err := startChild(c); err != nil { t.Fatalf("startChild(live fake): %v", err)