fix(slopmachine): bound verification process cleanup

cli/slopmachine/cmd/slopmachine/main.go

@@ -809,7 +809,10 @@ func cmdVerify(st *store.Store, args []string, opts runOptions) int {
 			err = errVerificationCommandCancelled
 		}
 		if err != nil {
-			cancelCode := 130
+			cancelCode := 1
+			if errors.Is(err, errVerificationCommandCancelled) {
+				cancelCode = 130
+			}
 			if unixSignal, ok := received.(syscall.Signal); ok {
 				cancelCode = 128 + int(unixSignal)
 			}
@@ -1788,6 +1791,7 @@ var errVerificationCommandCancelled = errors.New("verification command cancelled
 func runShell(ctx context.Context, command string, jsonOut bool) (int, string, error) {
 	cmd := exec.Command("sh", "-c", command)
 	cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}
+	cmd.WaitDelay = 2 * time.Second
 	stdoutDigest := newOutputDigester()
 	stderrDigest := newOutputDigester()
 	if jsonOut {
@@ -1800,41 +1804,37 @@ func runShell(ctx context.Context, command string, jsonOut bool) (int, string, e
 	if err := cmd.Start(); err != nil {
 		return 1, digestOutputs(stdoutDigest, stderrDigest), nil
 	}
-	waited := make(chan error, 1)
-	go func() { waited <- cmd.Wait() }()
-
-	select {
-	case err := <-waited:
-		return shellExitCode(err), digestOutputs(stdoutDigest, stderrDigest), nil
-	case <-ctx.Done():
-		select {
-		case err := <-waited:
-			return shellExitCode(err), digestOutputs(stdoutDigest, stderrDigest), nil
-		default:
-		}
-	}
-
+	// Keep the exited leader unreaped until cleanup is complete: its PID
+	// reserves the process-group identity while descendants are signalled.
 	pid := cmd.Process.Pid
-	_ = signalShellGroup(pid, syscall.SIGTERM)
-	timer := time.NewTimer(verificationTerminationGrace)
-	ticker := time.NewTicker(10 * time.Millisecond)
-	defer timer.Stop()
-	defer ticker.Stop()
-	waitComplete := false
-	for {
-		if waitComplete && !shellGroupAlive(pid) {
-			return 130, digestOutputs(stdoutDigest, stderrDigest), errVerificationCommandCancelled
-		}
-		select {
-		case <-waited:
-			waitComplete = true
-		case <-timer.C:
-			if shellGroupAlive(pid) {
-				_ = signalShellGroup(pid, syscall.SIGKILL)
-			}
-		case <-ticker.C:
-		}
-	}
+	watchErr := waitForExit(ctx, pid)
+	cancelled := errors.Is(watchErr, context.Canceled) || errors.Is(watchErr, context.DeadlineExceeded)
+	var cleanupErr error
+	if cancelled {
+		cleanupErr = signalShellGroup(pid, syscall.SIGTERM)
+		// The leader remains reserved for the whole grace period, including
+		// when it exits before a descendant finishes its TERM handler.
+		time.Sleep(verificationTerminationGrace)
+	}
+	killErr := signalShellGroup(pid, syscall.SIGKILL)
+	if ignoreCleanupErrorAfterExit(pid, killErr) {
+		killErr = nil
+	}
+	cleanupErr = errors.Join(cleanupErr, killErr)
+	if watchErr != nil {
+		if err := cmd.Process.Kill(); err != nil && !errors.Is(err, os.ErrProcessDone) {
+			cleanupErr = errors.Join(cleanupErr, err)
+		}
+	}
+	waitErr := cmd.Wait()
+	digest := digestOutputs(stdoutDigest, stderrDigest)
+	if cancelled || ctx.Err() != nil {
+		return 130, digest, errors.Join(errVerificationCommandCancelled, cleanupErr)
+	}
+	if watchErr != nil || cleanupErr != nil {
+		return 1, digest, fmt.Errorf("verification process cleanup: %w", errors.Join(watchErr, cleanupErr))
+	}
+	return shellExitCode(waitErr), digest, nil
 }
 
 func shellExitCode(err error) int {
@@ -1856,11 +1856,6 @@ func signalShellGroup(pid int, sig syscall.Signal) error {
 	return err
 }
 
-func shellGroupAlive(pid int) bool {
-	err := syscall.Kill(-pid, 0)
-	return err == nil || errors.Is(err, syscall.EPERM)
-}
-
 func randomID() (string, error) {
 	var b [4]byte
 	if _, err := rand.Read(b[:]); err != nil {

cli/slopmachine/cmd/slopmachine/main_test.go

@@ -13,7 +13,6 @@ import (
 	"path/filepath"
 	"strconv"
 	"strings"
-	"syscall"
 	"testing"
 	"time"
 
@@ -26,29 +25,30 @@ import (
 func TestRunShellCancellationReapsProcessGroup(t *testing.T) {
 	pidFile := filepath.Join(t.TempDir(), "pids")
 	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
 	type result struct {
 		code int
 		err  error
 	}
 	done := make(chan result, 1)
-	command := fmt.Sprintf(`trap '' TERM; sh -c 'trap "" TERM; printf "%%s %%s\n" "$PPID" "$$" > "$1"; while :; do sleep 1; done' child %q & wait`, pidFile)
+	command := fmt.Sprintf(`trap '' TERM; sh -c 'trap "" TERM; sleep 30 & printf "%%s %%s %%s\n" "$PPID" "$$" "$!" > "$1"; wait' child %q & wait`, pidFile)
 	go func() {
 		code, _, err := runShell(ctx, command, true)
 		done <- result{code: code, err: err}
 	}()
 
-	var groupPID, childPID int
+	var groupPID, childPID, grandchildPID int
 	deadline := time.Now().Add(3 * time.Second)
 	for time.Now().Before(deadline) {
 		contents, err := os.ReadFile(pidFile)
 		if err == nil {
-			if n, _ := fmt.Sscanf(string(contents), "%d %d", &groupPID, &childPID); n == 2 {
+			if n, _ := fmt.Sscanf(string(contents), "%d %d %d", &groupPID, &childPID, &grandchildPID); n == 3 {
 				break
 			}
 		}
 		time.Sleep(10 * time.Millisecond)
 	}
-	if groupPID == 0 || childPID == 0 {
+	if groupPID == 0 || childPID == 0 || grandchildPID == 0 {
 		t.Fatal("verification descendants did not start")
 	}
 	cancel()
@@ -61,13 +61,8 @@ func TestRunShellCancellationReapsProcessGroup(t *testing.T) {
 	case <-time.After(3 * time.Second):
 		t.Fatal("runShell did not finish cancellation")
 	}
-	if shellGroupAlive(groupPID) {
-		t.Fatalf("process group %d remains alive", groupPID)
-	}
-	for _, pid := range []int{groupPID, childPID} {
-		if err := syscall.Kill(pid, 0); !errors.Is(err, syscall.ESRCH) {
-			t.Fatalf("process %d remains: %v", pid, err)
-		}
+	for _, pid := range []int{groupPID, childPID, grandchildPID} {
+		waitVerificationProcessTerminated(t, pid)
 	}
 }
 

cli/slopmachine/cmd/slopmachine/process_darwin.go

@@ -0,0 +1,104 @@
+//go:build darwin
+
+package main
+
+import (
+	"context"
+	"errors"
+	"time"
+
+	"golang.org/x/sys/unix"
+)
+
+func waitForExit(ctx context.Context, pid int) error {
+	queue, err := unix.Kqueue()
+	if err != nil {
+		return err
+	}
+	defer unix.Close(queue)
+	change := unix.Kevent_t{
+		Ident:  uint64(pid),
+		Filter: unix.EVFILT_PROC,
+		Flags:  unix.EV_ADD | unix.EV_ENABLE | unix.EV_ONESHOT,
+		Fflags: unix.NOTE_EXIT,
+	}
+	if _, err := unix.Kevent(queue, []unix.Kevent_t{change}, nil, nil); err != nil {
+		if errors.Is(err, unix.ESRCH) {
+			return nil
+		}
+		return err
+	}
+	events := make([]unix.Kevent_t, 1)
+	for {
+		timeout := unix.NsecToTimespec((10 * time.Millisecond).Nanoseconds())
+		count, err := unix.Kevent(queue, nil, events, &timeout)
+		if errors.Is(err, unix.EINTR) {
+			continue
+		}
+		if err != nil {
+			return err
+		}
+		if count != 0 {
+			return nil
+		}
+		select {
+		case <-ctx.Done():
+			return ctx.Err()
+		default:
+		}
+	}
+}
+
+func ignoreCleanupErrorAfterExit(leaderPID int, err error) bool {
+	if !errors.Is(err, unix.EPERM) {
+		return false
+	}
+	for range 10 {
+		safe, pending := cleanupStateAfterExit(leaderPID)
+		if safe {
+			return true
+		}
+		if !pending {
+			return false
+		}
+		time.Sleep(time.Millisecond)
+	}
+	return false
+}
+
+func cleanupStateAfterExit(leaderPID int) (safe, pending bool) {
+	processes, queryErr := unix.SysctlKinfoProcSlice("kern.proc.pgrp", leaderPID)
+	if queryErr != nil {
+		return false, false
+	}
+	if containsOnlyZombieLeader(processes, leaderPID) {
+		return true, false
+	}
+	if len(processes) == 1 && int(processes[0].Proc.P_pid) == leaderPID {
+		return false, true
+	}
+	if len(processes) != 0 {
+		return false, false
+	}
+	leader, queryErr := unix.SysctlKinfoProc("kern.proc.pid", leaderPID)
+	if queryErr != nil {
+		return false, false
+	}
+	if isZombieLeader(leader, leaderPID) {
+		return true, false
+	}
+	return false, int(leader.Proc.P_pid) == leaderPID
+}
+
+func containsOnlyZombieLeader(processes []unix.KinfoProc, leaderPID int) bool {
+	return len(processes) == 1 &&
+		isZombieLeader(&processes[0], leaderPID)
+}
+
+func isZombieLeader(process *unix.KinfoProc, leaderPID int) bool {
+	// Darwin's SZOMB value is 5 in sys/proc.h but is not exported by x/sys.
+	const zombieState = 5
+	return process != nil &&
+		int(process.Proc.P_pid) == leaderPID &&
+		process.Proc.P_stat == zombieState
+}

cli/slopmachine/cmd/slopmachine/process_darwin_test.go

@@ -0,0 +1,33 @@
+package main
+
+import (
+	"os/exec"
+	"syscall"
+	"testing"
+
+	"golang.org/x/sys/unix"
+)
+
+func TestCleanupDoesNotIgnoreLiveLeader(t *testing.T) {
+	command := exec.Command("sleep", "30")
+	command.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}
+	if err := command.Start(); err != nil {
+		t.Fatal(err)
+	}
+	defer func() {
+		_ = command.Process.Kill()
+		_ = command.Wait()
+	}()
+	pid := command.Process.Pid
+	if safe, pending := cleanupStateAfterExit(pid); safe || !pending {
+		t.Fatalf("live leader cleanup state = (%t, %t)", safe, pending)
+	}
+	if ignoreCleanupErrorAfterExit(pid, unix.EPERM) {
+		t.Fatal("permission error ignored for a live process group")
+	}
+}
+
+func verificationProcessZombie(pid int) bool {
+	process, err := unix.SysctlKinfoProc("kern.proc.pid", pid)
+	return err == nil && isZombieLeader(process, pid)
+}

cli/slopmachine/cmd/slopmachine/process_linux.go

@@ -0,0 +1,38 @@
+//go:build linux
+
+package main
+
+import (
+	"context"
+	"errors"
+	"time"
+
+	"golang.org/x/sys/unix"
+)
+
+func waitForExit(ctx context.Context, pid int) error {
+	ticker := time.NewTicker(10 * time.Millisecond)
+	defer ticker.Stop()
+	for {
+		var info unix.Siginfo
+		err := unix.Waitid(unix.P_PID, pid, &info, unix.WEXITED|unix.WNOHANG|unix.WNOWAIT, nil)
+		if errors.Is(err, unix.EINTR) {
+			continue
+		}
+		if err != nil {
+			return err
+		}
+		if info.Signo != 0 {
+			return nil
+		}
+		select {
+		case <-ctx.Done():
+			return ctx.Err()
+		case <-ticker.C:
+		}
+	}
+}
+
+func ignoreCleanupErrorAfterExit(_ int, _ error) bool {
+	return false
+}

cli/slopmachine/cmd/slopmachine/process_linux_test.go

@@ -0,0 +1,21 @@
+package main
+
+import (
+	"fmt"
+	"os"
+	"strings"
+)
+
+func verificationProcessZombie(pid int) bool {
+	data, err := os.ReadFile(fmt.Sprintf("/proc/%d/stat", pid))
+	if err != nil {
+		return false
+	}
+	// The parenthesized command name may itself contain spaces and ')'.
+	end := strings.LastIndexByte(string(data), ')')
+	if end < 0 {
+		return false
+	}
+	fields := strings.Fields(string(data)[end+1:])
+	return len(fields) > 0 && fields[0] == "Z"
+}