fix(daemon): stop worker process groups by their launch-time identity

Kill and the Cancel hook now go through background.TerminateCommandGroup,
which signals the group the worker was launched into (ConfigureChild-
ProcessGroup's Setpgid) instead of rediscovering it from the PID, so a
worker that has exited, or on Darwin one that is still a zombie, no longer
leaves its children running. It is TerminateCommand without the reap:
the pool waits on its workers itself and exec.Cmd calls Cancel from
inside Wait, so reaping there would be a second Wait.

On Windows it is the same tree kill, with the process handle pinned while
it runs. A command that has already been waited cannot be pinned; that
now reports os.ErrProcessDone, which exec.Cmd's Cancel contract treats
as nothing to cancel, instead of the unexported "already released" error.

Fixes #861

internal/background/process_windows.go

@@ -4,6 +4,7 @@ package background
 
 import (
 	"fmt"
+	"os"
 	"os/exec"
 
 	"github.com/Gitlawb/zero/internal/execution"
@@ -46,7 +47,13 @@ func terminateOwnedProcess(cmd *exec.Cmd) (bool, error) {
 		terminateErr = terminateProcessForTest(cmd.Process.Pid)
 	})
 	if err != nil {
-		return false, fmt.Errorf("pin process %d for termination: %w", cmd.Process.Pid, err)
+		// WithHandle refuses only a process that has already been waited or
+		// released, so it has finished. Say so in the form exec.Cmd understands:
+		// a Cancel error wrapping os.ErrProcessDone is "nothing to cancel", where
+		// the unexported "already released" error it would otherwise carry made
+		// Wait report a spurious cancel failure. A dead root has no tree left to
+		// find either way.
+		return true, fmt.Errorf("pin process %d for termination: %w: %w", cmd.Process.Pid, os.ErrProcessDone, err)
 	}
 	return alreadyExited, terminateErr
 }

internal/background/terminate.go

@@ -73,6 +73,29 @@ func TerminateCommand(cmd *exec.Cmd) error {
 	return nil
 }
 
+// TerminateCommandGroup stops a started command's process group on POSIX, or its
+// process tree on Windows, WITHOUT reaping it.
+//
+// It is TerminateCommand without the Wait, for a caller whose Wait already runs
+// somewhere else: a worker pool waiting on the command in its own goroutine, or
+// the command's own Cancel hook, which exec.Cmd invokes while its Wait is in
+// progress. TerminateCommand would be a second Wait there. Both used to pass
+// cmd.Process.Pid to TerminateProcess, which rediscovers the group at signal time
+// with Getpgid, and once the leader has exited that lookup fails, so only the dead
+// leader was signalled and its descendants kept running. When cmd was configured
+// by ConfigureChildProcessGroup the group now comes from that launch-time
+// configuration instead. On Windows it is the tree kill TerminateProcess performs,
+// with the process handle pinned while it runs so the PID cannot be recycled
+// underneath it; once the command has been waited that pin fails with an error
+// wrapping os.ErrProcessDone.
+func TerminateCommandGroup(cmd *exec.Cmd) error {
+	if cmd == nil || cmd.Process == nil {
+		return nil
+	}
+	_, err := terminateOwnedProcess(cmd)
+	return err
+}
+
 // classifyWaitError treats a non-zero exit status as success: a process being
 // terminated is expected to report one (or a signal), so the only interesting
 // failure is Wait itself not working.

internal/background/terminate_group_test.go

@@ -0,0 +1,71 @@
+package background
+
+import (
+	"errors"
+	"os"
+	"os/exec"
+	"runtime"
+	"testing"
+	"time"
+)
+
+// TerminateCommandGroup LEAVES THE WAIT TO ITS CALLER. Its callers already have a
+// Wait running or about to run: the daemon pool waits on its workers, and
+// exec.Cmd calls the Cancel hook from inside Wait. Reaping here, the way
+// TerminateCommand does, would make their Wait fail with "Wait was already
+// called" instead of reporting how the command ended.
+func TestTerminateCommandGroupLeavesTheWaitToTheCaller(t *testing.T) {
+	cmd := exec.Command("sleep", "30")
+	if runtime.GOOS == "windows" {
+		cmd = exec.Command("powershell.exe", "-NoProfile", "-NonInteractive", "-Command", "Start-Sleep -Seconds 30")
+	}
+	ConfigureChildProcessGroup(cmd)
+	if err := cmd.Start(); err != nil {
+		t.Fatalf("start: %v", err)
+	}
+	if err := TerminateCommandGroup(cmd); err != nil {
+		_ = cmd.Process.Kill()
+		_ = cmd.Wait()
+		t.Fatalf("TerminateCommandGroup: %v", err)
+	}
+
+	waited := make(chan error, 1)
+	go func() { waited <- cmd.Wait() }()
+	select {
+	case err := <-waited:
+		var exitErr *exec.ExitError
+		if err != nil && !errors.As(err, &exitErr) {
+			t.Fatalf("the caller's own Wait failed after TerminateCommandGroup: %v", err)
+		}
+	case <-time.After(10 * time.Second):
+		_ = cmd.Process.Kill()
+		t.Fatal("the command was still running 10s after TerminateCommandGroup")
+	}
+}
+
+// A command that was never started, or a nil one, is nothing to stop.
+func TestTerminateCommandGroupWithNoProcessIsANoOp(t *testing.T) {
+	if err := TerminateCommandGroup(nil); err != nil {
+		t.Fatalf("nil command: %v", err)
+	}
+	if err := TerminateCommandGroup(exec.Command("sleep", "1")); err != nil {
+		t.Fatalf("unstarted command: %v", err)
+	}
+}
+
+// After the caller has waited, Windows cannot pin the process any more. The error
+// must still read as "already finished", which is what exec.Cmd's Cancel contract
+// and the pool both treat as nothing wrong.
+func TestTerminateCommandGroupAfterWaitReportsProcessDone(t *testing.T) {
+	if runtime.GOOS != "windows" {
+		t.Skip("POSIX signals the group, which is simply gone after the wait")
+	}
+	cmd := exec.Command("cmd.exe", "/c", "exit", "0")
+	ConfigureChildProcessGroup(cmd)
+	if err := cmd.Run(); err != nil {
+		t.Fatalf("run: %v", err)
+	}
+	if err := TerminateCommandGroup(cmd); err != nil && !errors.Is(err, os.ErrProcessDone) {
+		t.Fatalf("after Wait the error is %v, want nil or one wrapping os.ErrProcessDone", err)
+	}
+}

internal/daemon/launcher.go

@@ -72,9 +72,10 @@ func (w *execWorker) Kill() error {
 	if w.cmd.Process == nil {
 		return nil
 	}
-	// background.TerminateProcess is the cross-platform terminate (kills the
-	// process group on POSIX, taskkill /T on Windows).
-	return background.TerminateProcess(w.cmd.Process.Pid)
+	// The group the worker was launched into, not one rediscovered from its PID:
+	// the pool may call this after the worker has exited and been waited, and its
+	// children can still be running. See background.TerminateCommandGroup.
+	return background.TerminateCommandGroup(w.cmd)
 }
 
 // readerLines adapts a bufio.Reader to the Lines interface. Unlike a capped
@@ -154,13 +155,11 @@ func NewExecLauncher(cfg ExecLauncherConfig) (Launcher, error) {
 		background.ConfigureChildProcessGroup(cmd)
 		// CommandContext's default cancel sends os.Process.Kill to the LEADER only,
 		// orphaning the process group we just configured (a stuck worker's children
-		// would survive ctx cancellation). Terminate the whole group instead — the
-		// same cross-platform group terminate Kill() uses (D11).
+		// would survive ctx cancellation). Terminate the whole group instead, the
+		// same launch-time group terminate Kill() uses (D11). Cancel runs while Wait
+		// is in progress, so it must not reap.
 		cmd.Cancel = func() error {
-			if cmd.Process == nil {
-				return nil
-			}
-			return background.TerminateProcess(cmd.Process.Pid)
+			return background.TerminateCommandGroup(cmd)
 		}
 
 		stdout, err := cmd.StdoutPipe()

internal/daemon/launcher_group_unix_test.go

@@ -0,0 +1,90 @@
+//go:build !windows
+
+package daemon
+
+import (
+	"context"
+	"errors"
+	"os/exec"
+	"strconv"
+	"strings"
+	"syscall"
+	"testing"
+	"time"
+)
+
+// A WORKER'S CHILDREN ARE STOPPED THROUGH THE GROUP IT WAS LAUNCHED INTO.
+//
+// Kill and the Cancel hook used to hand the worker's PID to
+// background.TerminateProcess, which looks the group up again with Getpgid when it
+// signals. Once the worker has exited and been waited, that lookup fails, only the
+// dead PID is signalled, and whatever the worker left running survives; on Darwin
+// the lookup already fails while the exited worker is still a zombie. The worker
+// here is a shell that backgrounds a sleep, prints its PID and exits, so after the
+// wait the sleep is the only process left in the worker's group. Reported in #861.
+func TestExecWorkerStopsWhatItLeftRunningAfterItExits(t *testing.T) {
+	for _, stop := range []struct {
+		name string
+		run  func(*execWorker) error
+	}{
+		{"Kill", func(worker *execWorker) error { return worker.Kill() }},
+		{"Cancel hook", func(worker *execWorker) error { return worker.cmd.Cancel() }},
+	} {
+		t.Run(stop.name, func(t *testing.T) {
+			launcher, err := NewExecLauncher(ExecLauncherConfig{
+				Executable: "/bin/sh",
+				BaseArgs:   []string{"-c", "sleep 300 >/dev/null 2>&1 & echo $!"},
+			})
+			if err != nil {
+				t.Fatalf("NewExecLauncher: %v", err)
+			}
+			handle, err := launcher(context.Background(), WorkerSpec{})
+			if err != nil {
+				t.Fatalf("launch: %v", err)
+			}
+			worker, ok := handle.(*execWorker)
+			if !ok {
+				t.Fatalf("SETUP INVALID: the launcher returned %T, not the exec worker under test", handle)
+			}
+			line, ok, err := worker.Stdout().Next()
+			if err != nil || !ok {
+				t.Fatalf("read the child's pid: ok=%v err=%v", ok, err)
+			}
+			childPID, err := strconv.Atoi(strings.TrimSpace(line))
+			if err != nil {
+				t.Fatalf("parse the child's pid %q: %v", line, err)
+			}
+			t.Cleanup(func() { _ = syscall.Kill(childPID, syscall.SIGKILL) })
+
+			if _, err := worker.Wait(); err != nil {
+				t.Fatalf("wait for the worker: %v", err)
+			}
+			if childStopped(childPID) {
+				t.Fatalf("SETUP INVALID: the worker's child %d is not running, so there is nothing left to stop", childPID)
+			}
+
+			// The pool ignores what Kill returns; the outcome is what matters.
+			_ = stop.run(worker)
+
+			deadline := time.Now().Add(5 * time.Second)
+			for !childStopped(childPID) {
+				if time.Now().After(deadline) {
+					t.Fatalf("the worker's child %d survived %s after the worker was reaped", childPID, stop.name)
+				}
+				time.Sleep(20 * time.Millisecond)
+			}
+		})
+	}
+}
+
+// childStopped reports whether pid is gone or only waiting to be reaped.
+func childStopped(pid int) bool {
+	if errors.Is(syscall.Kill(pid, syscall.Signal(0)), syscall.ESRCH) {
+		return true
+	}
+	state, err := exec.Command("ps", "-o", "stat=", "-p", strconv.Itoa(pid)).Output()
+	if err != nil {
+		return errors.Is(syscall.Kill(pid, syscall.Signal(0)), syscall.ESRCH)
+	}
+	return strings.HasPrefix(strings.TrimSpace(string(state)), "Z")
+}