fix(daemon): clean up child after startup timeout

internal/background/process_posix.go

@@ -39,3 +39,14 @@ func ConfigureChildProcessGroup(cmd *exec.Cmd) {
 func terminateProcess(pid int) error {
 	return execution.TerminateProcessTree(pid, terminationGracePeriod, terminationPollInterval)
 }
+
+// terminateOwnedProcess terminates cmd's process group directly when its launch
+// configuration proves it was made a group leader. This avoids fragile Getpgid
+// rediscovery after an owned leader exits. Ordinary commands fall back to the
+// safe PID/tree path rather than assuming their PID is also a process-group ID.
+func terminateOwnedProcess(cmd *exec.Cmd) (bool, error) {
+	if cmd.SysProcAttr != nil && cmd.SysProcAttr.Setpgid && cmd.SysProcAttr.Pgid == 0 {
+		return false, execution.TerminateProcessGroup(cmd.Process.Pid, terminationGracePeriod, terminationPollInterval)
+	}
+	return false, terminateProcess(cmd.Process.Pid)
+}

internal/background/process_posix_test.go

@@ -147,15 +147,86 @@ func TestTerminateProcessKillsForkedChildren(t *testing.T) {
 		t.Fatalf("terminateProcess: %v", err)
 	}
 
-	// The forked child must be gone too (poll until reaped by init).
+	// The forked child must no longer be running. An orphaned zombie is already
+	// dead but may remain visible briefly until the platform's init reaps it.
 	deadline := time.Now().Add(2 * time.Second)
-	// Only ESRCH proves the child is gone; any other error (e.g. EPERM) would
-	// wrongly pass the test, so treat it as still-present and keep polling.
-	for !errors.Is(syscall.Kill(childPID, syscall.Signal(0)), syscall.ESRCH) {
+	// processStopped treats only ESRCH or an observed zombie state as stopped;
+	// other errors (for example EPERM) remain live and keep polling.
+	for !processStopped(childPID) {
 		if time.Now().After(deadline) {
 			_ = syscall.Kill(childPID, syscall.SIGKILL)
 			t.Fatalf("forked child %d survived terminateProcess — group kill failed", childPID)
 		}
 		time.Sleep(20 * time.Millisecond)
 	}
 }
+
+func TestTerminateCommandStopsUnconfiguredCommand(t *testing.T) {
+	cmd := exec.Command("sleep", "30")
+	if err := cmd.Start(); err != nil {
+		t.Fatalf("start: %v", err)
+	}
+	t.Cleanup(func() {
+		if cmd.ProcessState == nil {
+			_ = cmd.Process.Kill()
+		}
+	})
+
+	if err := TerminateCommand(cmd); err != nil {
+		t.Fatalf("TerminateCommand: %v", err)
+	}
+	if cmd.ProcessState == nil {
+		t.Fatal("unconfigured command was not reaped")
+	}
+}
+
+func TestTerminateCommandKillsChildAfterLeaderExits(t *testing.T) {
+	grace, poll := terminationGracePeriod, terminationPollInterval
+	terminationGracePeriod, terminationPollInterval = 2*time.Second, 20*time.Millisecond
+	t.Cleanup(func() { terminationGracePeriod, terminationPollInterval = grace, poll })
+
+	// The leader exits immediately after launching the child. TerminateCommand
+	// must capture and signal the process group before Wait reaps the leader and
+	// makes its group identity unavailable.
+	cmd := exec.Command("sh", "-c", "sleep 300 & echo $!; exit 0")
+	ConfigureChildProcessGroup(cmd)
+	stdout, err := cmd.StdoutPipe()
+	if err != nil {
+		t.Fatalf("stdout pipe: %v", err)
+	}
+	if err := cmd.Start(); err != nil {
+		t.Fatalf("start: %v", err)
+	}
+	line, err := bufio.NewReader(stdout).ReadString('\n')
+	if err != nil {
+		t.Fatalf("read forked child pid: %v", err)
+	}
+	childPID, err := strconv.Atoi(strings.TrimSpace(line))
+	if err != nil {
+		t.Fatalf("parse forked child pid %q: %v", line, err)
+	}
+
+	if err := TerminateCommand(cmd); err != nil {
+		t.Fatalf("TerminateCommand: %v", err)
+	}
+
+	deadline := time.Now().Add(2 * time.Second)
+	for !processStopped(childPID) {
+		if time.Now().After(deadline) {
+			_ = syscall.Kill(childPID, syscall.SIGKILL)
+			t.Fatalf("forked child %d survived TerminateCommand", childPID)
+		}
+		time.Sleep(20 * time.Millisecond)
+	}
+}
+
+func processStopped(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")
+}

internal/background/process_windows.go

@@ -3,11 +3,20 @@
 package background
 
 import (
+	"fmt"
 	"os/exec"
 
 	"github.com/Gitlawb/zero/internal/execution"
+	"golang.org/x/sys/windows"
 )
 
+// stillActiveExitCode is the documented GetExitCodeProcess value for a process
+// that has not terminated. x/sys/windows does not expose Windows' STILL_ACTIVE
+// constant.
+const stillActiveExitCode uint32 = 259
+
+var terminateProcessForTest = terminateProcess
+
 // ConfigureChildProcessGroup is a no-op on Windows: process-tree termination is
 // delegated to execution.TerminateProcessTree, so no launch-time process-group
 // setup is required (the POSIX build sets Setpgid here instead).
@@ -16,3 +25,28 @@ func ConfigureChildProcessGroup(cmd *exec.Cmd) { execution.ConfigureProcessGroup
 func terminateProcess(pid int) error {
 	return execution.TerminateProcessTree(pid, 0, 0)
 }
+
+// terminateOwnedProcess asks taskkill /T to terminate the tree rooted at cmd,
+// with KillProcessTree's direct Process.Kill fallback if taskkill fails. taskkill
+// can discover descendants only while the root PID still exists; unlike a POSIX
+// process group, a Windows tree has no independently addressable identity after
+// its root exits.
+func terminateOwnedProcess(cmd *exec.Cmd) (bool, error) {
+	var (
+		alreadyExited bool
+		terminateErr  error
+	)
+	err := cmd.Process.WithHandle(func(handle uintptr) {
+		var exitCode uint32
+		if windows.GetExitCodeProcess(windows.Handle(handle), &exitCode) == nil {
+			alreadyExited = exitCode != stillActiveExitCode
+		}
+		// Keep the exact process identity pinned while the PID-based taskkill
+		// operation runs, preventing the PID from being recycled underneath it.
+		terminateErr = terminateProcessForTest(cmd.Process.Pid)
+	})
+	if err != nil {
+		return false, fmt.Errorf("pin process %d for termination: %w", cmd.Process.Pid, err)
+	}
+	return alreadyExited, terminateErr
+}

internal/background/process_windows_test.go

@@ -0,0 +1,106 @@
+//go:build windows
+
+package background
+
+import (
+	"bufio"
+	"errors"
+	"os/exec"
+	"strconv"
+	"strings"
+	"testing"
+	"time"
+)
+
+func TestTerminateCommandReturnsTreeFailureAfterRootFallbackReaps(t *testing.T) {
+	cmd := exec.Command("powershell.exe", "-NoProfile", "-NonInteractive", "-Command", "Start-Sleep -Seconds 300")
+	if err := cmd.Start(); err != nil {
+		t.Fatalf("start: %v", err)
+	}
+	treeErr := errors.New("taskkill /T failed")
+	originalTerminateProcess := terminateProcessForTest
+	terminateProcessForTest = func(pid int) error {
+		if err := cmd.Process.Kill(); err != nil {
+			return errors.Join(treeErr, err)
+		}
+		return treeErr
+	}
+	t.Cleanup(func() { terminateProcessForTest = originalTerminateProcess })
+
+	err := TerminateCommand(cmd)
+	if !errors.Is(err, treeErr) {
+		t.Fatalf("TerminateCommand error = %v, want tree failure", err)
+	}
+	if cmd.ProcessState == nil {
+		t.Fatal("root process was not reaped")
+	}
+}
+
+func TestTerminateCommandReapsExitedLeader(t *testing.T) {
+	cmd := exec.Command("cmd.exe", "/c", "exit", "0")
+	if err := cmd.Start(); err != nil {
+		t.Fatalf("start: %v", err)
+	}
+	// Let the child exit without calling Wait so cleanup exercises the Windows
+	// taskkill/TerminateProcess race while it still owns the process handle.
+	time.Sleep(500 * time.Millisecond)
+
+	// jatmn's #774 finding: taskkill /T racing an already-exited PID (and the
+	// Process.Kill fallback) both fail here, but the reap below still succeeds —
+	// the process-handle exit-code check proves the leader was already gone, so
+	// TerminateCommand must not surface the expected kill-attempt error.
+	if err := TerminateCommand(cmd); err != nil {
+		t.Fatalf("TerminateCommand: %v, want nil (leader was already gone and got reaped)", err)
+	}
+	if cmd.ProcessState == nil {
+		t.Fatal("exited process was not reaped")
+	}
+}
+
+func TestTerminateCommandDeadLeaderCannotIdentifyDescendantTree(t *testing.T) {
+	cmd := exec.Command("powershell.exe", "-NoProfile", "-NonInteractive", "-Command",
+		`$p = Start-Process powershell.exe -ArgumentList '-NoProfile','-Command','Start-Sleep -Seconds 300' -PassThru; $p.Id`)
+	stdout, err := cmd.StdoutPipe()
+	if err != nil {
+		t.Fatalf("stdout pipe: %v", err)
+	}
+	if err := cmd.Start(); err != nil {
+		t.Fatalf("start: %v", err)
+	}
+	line, err := bufio.NewReader(stdout).ReadString('\n')
+	if err != nil {
+		t.Fatalf("read descendant pid: %v", err)
+	}
+	descendantPID, err := strconv.Atoi(strings.TrimSpace(line))
+	if err != nil {
+		t.Fatalf("parse descendant pid %q: %v", line, err)
+	}
+	t.Cleanup(func() {
+		_ = exec.Command("taskkill.exe", "/F", "/PID", strconv.Itoa(descendantPID)).Run()
+	})
+
+	// Wait until the leader is observably exited but deliberately leave it
+	// unreaped, matching the POSIX regression's ordering.
+	deadline := time.Now().Add(5 * time.Second)
+	for processExistsOnWindows(cmd.Process.Pid) {
+		if time.Now().After(deadline) {
+			t.Fatal("leader did not exit")
+		}
+		time.Sleep(20 * time.Millisecond)
+	}
+	if err := TerminateCommand(cmd); err != nil {
+		t.Fatalf("TerminateCommand: %v", err)
+	}
+
+	// Windows has no persistent group ID analogous to POSIX: once the leader is
+	// gone, taskkill /T cannot rediscover this descendant. Assert that platform
+	// distinction rather than claiming TerminateCommand can provide it here.
+	if !processExistsOnWindows(descendantPID) {
+		t.Fatal("descendant unexpectedly stopped after its root exited")
+	}
+}
+
+func processExistsOnWindows(pid int) bool {
+	return exec.Command("powershell.exe", "-NoProfile", "-NonInteractive", "-Command",
+		"Get-Process -Id "+strconv.Itoa(pid)+" -ErrorAction Stop | Out-Null").Run() == nil
+}

internal/background/terminate.go

@@ -1,5 +1,14 @@
 package background
 
+import (
+	"errors"
+	"fmt"
+	"os/exec"
+	"time"
+)
+
+const commandReapTimeout = 3 * time.Second
+
 // TerminateProcess stops a background process by PID — on Windows its process
 // tree; on POSIX its whole process group when the PID leads its own group (the
 // invariant ConfigureChildProcessGroup establishes for processes started through
@@ -12,3 +21,69 @@ package background
 func TerminateProcess(pid int) error {
 	return terminateProcess(pid)
 }
+
+// TerminateCommand stops a started command and reaps its leader. The caller must
+// have exclusive ownership of cmd: it must not have previously called Wait or
+// Process.Release, and no goroutine may call either concurrently. On POSIX it
+// stops the whole group when the command was configured as its leader; ordinary
+// commands safely fall back to PID/tree discovery. On Windows, success for a
+// leader that was already dead confirms only that the leader was reaped;
+// descendants may survive because Windows cannot rediscover a tree from a dead
+// root. `zero daemon start` needs this operation when readiness times out: it
+// launched the child, so it must both stop the tree and collect the leader.
+//
+// The order matters: the tree is signalled first, because Wait releases the
+// leader's PID and a later group lookup could then resolve to nothing (or, worse,
+// to a recycled PID). Termination stays in the shared execution lifecycle
+// primitives rather than introducing a second platform-specific killer.
+//
+// If the bounded reap times out, the Wait call already started by this function
+// continues in its goroutine. From that point onward the caller must not access
+// cmd at all, including ProcessState, because that would race with Wait.
+func TerminateCommand(cmd *exec.Cmd) error {
+	if cmd == nil || cmd.Process == nil {
+		return errors.New("terminate command: process was never started")
+	}
+	leaderAlreadyExited, terminateErr := terminateOwnedProcess(cmd)
+	reapErr := waitForTerminatedCommandWithin(cmd, commandReapTimeout)
+	if reapErr != nil {
+		if terminateErr != nil {
+			return fmt.Errorf("%v (reap failed: %w)", terminateErr, reapErr)
+		}
+		return reapErr
+	}
+	if terminateErr != nil && !leaderAlreadyExited {
+		return terminateErr
+	}
+	// Only discard a termination error when the platform demonstrated before
+	// attempting termination that the leader had already exited. A successful
+	// reap alone says nothing about whether a live descendant tree was stopped.
+	return nil
+}
+
+// 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.
+func classifyWaitError(waitErr error) error {
+	var exitErr *exec.ExitError
+	if waitErr != nil && !errors.As(waitErr, &exitErr) {
+		return fmt.Errorf("reap process: %w", waitErr)
+	}
+	return nil
+}
+
+// waitForTerminatedCommandWithin bounds the reap so a child that somehow survives
+// termination cannot hang the caller — the daemon-start cleanup path runs while a
+// user waits at the CLI. On timeout its Wait goroutine is intentionally left to
+// finish: exec.Cmd permits only one Wait call, and abandoning it without a reaper
+// would leak the child once it eventually exits.
+func waitForTerminatedCommandWithin(cmd *exec.Cmd, timeout time.Duration) error {
+	waitDone := make(chan error, 1)
+	go func() { waitDone <- cmd.Wait() }()
+	select {
+	case waitErr := <-waitDone:
+		return classifyWaitError(waitErr)
+	case <-time.After(timeout):
+		return fmt.Errorf("process %d did not reap after termination", cmd.Process.Pid)
+	}
+}