Send signals to process group for complete cleanup

Use kill(-pid, sig) instead of kill(pid, sig) in Process.Stop()
and terminateStaleRunner() so the entire runner process group is
terminated. The runner already starts with Setsid: true (PGID == PID),
so this is safe and strictly better when the runner has children.

Guard against unsafe PIDs (0 and 1) to prevent catastrophic
kill(0)/kill(-1) calls. Use errors.Is for ESRCH checks.

Closes #4

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Author: JAORMX

propolis.go

@@ -285,16 +285,21 @@ func terminateStaleRunner(cfg *config) {
 		slog.Debug("could not load state for stale runner check", "error", err)
 		return
 	}
-	if st.PID <= 0 {
+	if st.PID <= 1 {
 		return
 	}
 	if !cfg.processAlive(st.PID) {
 		slog.Debug("stale runner already dead", "pid", st.PID)
 		return
 	}
 
-	slog.Warn("terminating stale runner process", "pid", st.PID)
-	if err := cfg.killProcess(st.PID, syscall.SIGTERM); err != nil {
+	// Use negative PID to signal the entire process group (PGID == PID
+	// because the runner starts with Setsid: true). This ensures any
+	// children spawned by the runner are also terminated.
+	target := -st.PID
+
+	slog.Warn("terminating stale runner process group", "pid", st.PID)
+	if err := cfg.killProcess(target, syscall.SIGTERM); err != nil {
 		slog.Warn("failed to send SIGTERM to stale runner", "pid", st.PID, "error", err)
 		return
 	}
@@ -311,7 +316,7 @@ func terminateStaleRunner(cfg *config) {
 
 	// Force kill.
 	slog.Warn("stale runner did not exit after SIGTERM, sending SIGKILL", "pid", st.PID)
-	if err := cfg.killProcess(st.PID, syscall.SIGKILL); err != nil {
+	if err := cfg.killProcess(target, syscall.SIGKILL); err != nil {
 		slog.Warn("failed to send SIGKILL to stale runner", "pid", st.PID, "error", err)
 	}
 }

propolis_test.go

@@ -791,7 +791,7 @@ func TestTerminateStaleRunner_AliveProcess_GracefulExit(t *testing.T) {
 	aliveCount := 0
 
 	cfg.killProcess = func(pid int, sig syscall.Signal) error {
-		assert.Equal(t, 99999, pid)
+		assert.Equal(t, -99999, pid, "should signal the process group (negative PID)")
 		mu.Lock()
 		signals = append(signals, sig)
 		mu.Unlock()
@@ -834,7 +834,7 @@ func TestTerminateStaleRunner_AliveProcess_RequiresKill(t *testing.T) {
 	var signals []syscall.Signal
 
 	cfg.killProcess = func(pid int, sig syscall.Signal) error {
-		assert.Equal(t, 99999, pid)
+		assert.Equal(t, -99999, pid, "should signal the process group (negative PID)")
 		mu.Lock()
 		signals = append(signals, sig)
 		mu.Unlock()
@@ -852,6 +852,75 @@ func TestTerminateStaleRunner_AliveProcess_RequiresKill(t *testing.T) {
 	assert.Equal(t, syscall.SIGKILL, signals[1])
 }
 
+func TestTerminateStaleRunner_SendsToProcessGroup(t *testing.T) {
+	t.Parallel()
+
+	dataDir := t.TempDir()
+
+	mgr := state.NewManager(dataDir)
+	ls, err := mgr.LoadAndLock(context.Background())
+	require.NoError(t, err)
+	ls.State.Active = true
+	ls.State.PID = 55555
+	require.NoError(t, ls.Save())
+	ls.Release()
+
+	cfg := defaultConfig()
+	cfg.dataDir = dataDir
+
+	var mu sync.Mutex
+	var receivedPIDs []int
+	aliveCount := 0
+
+	cfg.killProcess = func(pid int, _ syscall.Signal) error {
+		mu.Lock()
+		receivedPIDs = append(receivedPIDs, pid)
+		mu.Unlock()
+		return nil
+	}
+	cfg.processAlive = func(_ int) bool {
+		mu.Lock()
+		defer mu.Unlock()
+		aliveCount++
+		return aliveCount <= 1
+	}
+
+	terminateStaleRunner(cfg)
+
+	mu.Lock()
+	defer mu.Unlock()
+	require.Len(t, receivedPIDs, 1)
+	assert.Equal(t, -55555, receivedPIDs[0], "killProcess should receive negative PID for process group")
+}
+
+func TestTerminateStaleRunner_PID1_Skipped(t *testing.T) {
+	t.Parallel()
+
+	dataDir := t.TempDir()
+
+	// Write state with PID=1 (init). This must never produce kill(-1, sig).
+	mgr := state.NewManager(dataDir)
+	ls, err := mgr.LoadAndLock(context.Background())
+	require.NoError(t, err)
+	ls.State.Active = true
+	ls.State.PID = 1
+	require.NoError(t, ls.Save())
+	ls.Release()
+
+	cfg := defaultConfig()
+	cfg.dataDir = dataDir
+
+	var killCalled bool
+	cfg.killProcess = func(_ int, _ syscall.Signal) error {
+		killCalled = true
+		return nil
+	}
+	cfg.processAlive = func(_ int) bool { return true }
+
+	terminateStaleRunner(cfg)
+	assert.False(t, killCalled, "should not attempt to kill PID 1")
+}
+
 func TestTerminateStaleRunner_ZeroPID(t *testing.T) {
 	t.Parallel()
 

runner/spawn.go

@@ -6,6 +6,7 @@ package runner
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"log/slog"
 	"os"
@@ -68,6 +69,17 @@ type Process struct {
 // PID returns the process ID (implements ProcessHandle).
 func (p *Process) PID() int { return p.pid }
 
+// killTarget returns the negative PID for process-group-wide signals.
+// Because the runner starts with Setsid: true, its PGID == PID,
+// so kill(-pid, sig) targets the entire process group.
+// Panics if pid <= 1 to prevent kill(0) (own group) or kill(-1) (all processes).
+func (p *Process) killTarget() int {
+	if p.pid <= 1 {
+		panic(fmt.Sprintf("killTarget called with unsafe pid %d", p.pid))
+	}
+	return -p.pid
+}
+
 // Spawn starts the propolis-runner binary as a detached subprocess.
 // Deprecated: Use SpawnProcess or DefaultSpawner instead.
 func Spawn(ctx context.Context, cfg Config) (*Process, error) {
@@ -164,7 +176,9 @@ func (p *Process) Stop(ctx context.Context) error {
 	}
 
 	// Send SIGTERM for graceful shutdown.
-	if err := p.deps.kill(p.pid, syscall.SIGTERM); err != nil {
+	// Use killTarget() (negative PID) to signal the entire process group,
+	// ensuring any children spawned by the runner are also terminated.
+	if err := p.deps.kill(p.killTarget(), syscall.SIGTERM); err != nil {
 		// If the process is already gone, that is fine.
 		if !isNoSuchProcess(err) {
 			return fmt.Errorf("send SIGTERM to pid %d: %w", p.pid, err)
@@ -181,15 +195,15 @@ func (p *Process) Stop(ctx context.Context) error {
 		select {
 		case <-ctx.Done():
 			// Context canceled — force kill.
-			_ = p.deps.kill(p.pid, syscall.SIGKILL)
+			_ = p.deps.kill(p.killTarget(), syscall.SIGKILL)
 			return ctx.Err()
 		case <-ticker.C:
 			if !p.IsAlive() {
 				return nil
 			}
 			if time.Now().After(deadline) {
 				// Timeout — force kill.
-				if err := p.deps.kill(p.pid, syscall.SIGKILL); err != nil && !isNoSuchProcess(err) {
+				if err := p.deps.kill(p.killTarget(), syscall.SIGKILL); err != nil && !isNoSuchProcess(err) {
 					return fmt.Errorf("send SIGKILL to pid %d: %w", p.pid, err)
 				}
 				return nil
@@ -260,5 +274,5 @@ func libPathEnvVar() string {
 
 // isNoSuchProcess returns true if the error indicates the process does not exist.
 func isNoSuchProcess(err error) bool {
-	return err == syscall.ESRCH
+	return errors.Is(err, syscall.ESRCH)
 }

runner/spawn_test.go

@@ -309,6 +309,88 @@ func TestIsNoSuchProcess(t *testing.T) {
 	assert.False(t, isNoSuchProcess(os.ErrNotExist))
 }
 
+// --- Process group cleanup tests ---
+
+func TestProcess_Stop_SendsToProcessGroup(t *testing.T) {
+	t.Parallel()
+
+	terminated := false
+
+	var receivedPID int
+	p := &Process{
+		pid: 12345,
+		deps: processDeps{
+			findProcess: func(_ int) (*os.Process, error) {
+				if terminated {
+					return nil, os.ErrNotExist
+				}
+				return os.FindProcess(os.Getpid())
+			},
+			kill: func(pid int, sig syscall.Signal) error {
+				receivedPID = pid
+				if sig == syscall.SIGTERM {
+					terminated = true
+				}
+				return nil
+			},
+		},
+	}
+
+	err := p.Stop(context.Background())
+	require.NoError(t, err)
+	assert.Equal(t, -12345, receivedPID, "kill should receive negative PID for process group")
+}
+
+func TestProcess_Stop_KillSendsToProcessGroup(t *testing.T) {
+	t.Parallel()
+
+	// Process stays alive forever: SIGTERM succeeds but process never exits.
+	// A short-lived context should cause SIGKILL, also to the process group.
+	var killPIDs []int
+	p := &Process{
+		pid: 12345,
+		deps: processDeps{
+			findProcess: func(_ int) (*os.Process, error) {
+				return os.FindProcess(os.Getpid())
+			},
+			kill: func(pid int, _ syscall.Signal) error {
+				killPIDs = append(killPIDs, pid)
+				return nil
+			},
+		},
+	}
+
+	ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond)
+	defer cancel()
+
+	_ = p.Stop(ctx)
+	// Both SIGTERM and SIGKILL should target the process group (negative PID).
+	for i, pid := range killPIDs {
+		assert.Equal(t, -12345, pid, "kill call %d should use negative PID", i)
+	}
+}
+
+func TestProcess_killTarget(t *testing.T) {
+	t.Parallel()
+
+	p := &Process{pid: 42}
+	assert.Equal(t, -42, p.killTarget())
+}
+
+func TestProcess_killTarget_PanicsOnPID0(t *testing.T) {
+	t.Parallel()
+
+	p := &Process{pid: 0}
+	assert.Panics(t, func() { p.killTarget() }, "killTarget with PID 0 should panic")
+}
+
+func TestProcess_killTarget_PanicsOnPID1(t *testing.T) {
+	t.Parallel()
+
+	p := &Process{pid: 1}
+	assert.Panics(t, func() { p.killTarget() }, "killTarget with PID 1 should panic")
+}
+
 // --- Interface compliance ---
 
 func TestDefaultSpawner_ImplementsInterface(t *testing.T) {

vm.go

@@ -127,6 +127,8 @@ func (vm *VM) Remove(ctx context.Context) error {
 
 // RunnerPID returns the runner process ID, or 0 if the ID cannot be parsed
 // as a PID (e.g. non-process-based backends).
+// Because the runner starts with Setsid: true, its PGID equals its PID.
+// Callers needing process-group signals can use syscall.Kill(-pid, sig).
 func (vm *VM) RunnerPID() int {
 	pid, err := pidFromID(vm.handle.ID())
 	if err != nil {