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 for a caller that launched it
+// through this package (ConfigureChildProcessGroup was called before Start), so
+// cmd.Process.Pid is known — from launch time — to be its own process-group
+// leader. It signals the group directly via execution.TerminateProcessGroup
+// rather than terminateProcess's TerminateProcessTree, whose Getpgid
+// rediscovery is fragile once the leader may have already exited (see
+// TerminateProcessGroup's doc comment for why).
+func terminateOwnedProcess(cmd *exec.Cmd) error {
+	return execution.TerminateProcessGroup(cmd.Process.Pid, terminationGracePeriod, terminationPollInterval)
+}

internal/background/process_posix_test.go

@@ -147,18 +147,65 @@ 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)
-	for {
-		// 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.
-		if errors.Is(syscall.Kill(childPID, syscall.Signal(0)), syscall.ESRCH) {
-			break // child no longer exists
-		}
+	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 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

@@ -16,3 +16,11 @@ func ConfigureChildProcessGroup(cmd *exec.Cmd) { execution.ConfigureProcessGroup
 func terminateProcess(pid int) error {
 	return execution.TerminateProcessTree(pid, 0, 0)
 }
+
+// terminateOwnedProcess terminates cmd's process. Windows has no process-group
+// rediscovery concern — KillProcessTree always operates on the whole process
+// tree via taskkill /T regardless of how the PID was obtained — so this is the
+// same as terminateProcess.
+func terminateOwnedProcess(cmd *exec.Cmd) error {
+	return terminateProcess(cmd.Process.Pid)
+}

internal/background/process_windows_test.go

@@ -0,0 +1,30 @@
+//go:build windows
+
+package background
+
+import (
+	"os/exec"
+	"testing"
+	"time"
+)
+
+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 —
+	// that's authoritative proof the leader is gone, so TerminateCommand must
+	// report success rather than surfacing the earlier 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")
+	}
+}

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,60 @@ package background
 func TerminateProcess(pid int) error {
 	return terminateProcess(pid)
 }
+
+// TerminateCommand stops a started command's process tree/group AND reaps the
+// leader, which TerminateProcess alone cannot do: a caller that still owns the
+// exec.Cmd must Wait for it, or the exited leader lingers as a zombie for the
+// lifetime of the parent. `zero daemon start` needs exactly this 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 itself goes through terminateProcess, so this
+// package keeps ONE cross-platform kill path (execution.TerminateProcessTree)
+// rather than a second platform-specific killer beside it.
+func TerminateCommand(cmd *exec.Cmd) error {
+	if cmd == nil || cmd.Process == nil {
+		return errors.New("terminate command: process was never started")
+	}
+	terminateErr := terminateOwnedProcess(cmd)
+	reapErr := waitForTerminatedCommandWithin(cmd, commandReapTimeout)
+	if reapErr != nil {
+		if terminateErr != nil {
+			return fmt.Errorf("%v (reap failed: %w)", terminateErr, reapErr)
+		}
+		return reapErr
+	}
+	// A successful reap is authoritative: cmd.Wait returned, so the leader is
+	// confirmed gone and collected. A terminateErr alongside that (e.g. a kill
+	// attempt racing a process that had already exited on its own, such as
+	// taskkill /T failing on Windows because the PID is already gone) describes
+	// how the attempt went, not whether the goal — the process is gone — was met.
+	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.
+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)
+	}
+}

internal/cli/daemon.go

@@ -1,6 +1,7 @@
 package cli
 
 import (
+	"errors"
 	"fmt"
 	"io"
 	"os"
@@ -160,20 +161,56 @@ func runDaemonStartDetached(paths daemon.Paths, stdout io.Writer, stderr io.Writ
 	cmd.Stdout = logFile
 	cmd.Stderr = logFile
 	background.ConfigureChildProcessGroup(cmd) // own process group: outlives this shell
-	if err := cmd.Start(); err != nil {
+	if err := startAndAwaitDaemonProcess(cmd, func() bool { return daemonReachable(paths) }, 5*time.Second, 25*time.Millisecond); err != nil {
+		if errors.Is(err, errDaemonStartTimeout) {
+			return writeAppError(stderr, err.Error()+"; see "+logPath, exitCrash)
+		}
 		return writeAppError(stderr, err.Error(), exitCrash)
 	}
-	_ = cmd.Process.Release()
+	fmt.Fprintf(stdout, "zero daemon started (socket %s)\n", paths.Socket)
+	return exitSuccess
+}
+
+var errDaemonStartTimeout = errors.New("daemon did not come up within timeout")
 
-	deadline := time.Now().Add(5 * time.Second)
-	for time.Now().Before(deadline) {
-		if daemonReachable(paths) {
-			fmt.Fprintf(stdout, "zero daemon started (socket %s)\n", paths.Socket)
-			return exitSuccess
+func startAndAwaitDaemonProcess(cmd *exec.Cmd, reachable func() bool, timeout time.Duration, pollInterval time.Duration) error {
+	if err := cmd.Start(); err != nil {
+		return err
+	}
+	if waitForDaemonReadiness(reachable, timeout, pollInterval) {
+		if err := cmd.Process.Release(); err != nil {
+			cleanupErr := terminateAndReapDaemonProcess(cmd)
+			if cleanupErr != nil {
+				return fmt.Errorf("release daemon process: %w (cleanup failed: %v)", err, cleanupErr)
+			}
+			return fmt.Errorf("release daemon process: %w", err)
 		}
-		time.Sleep(25 * time.Millisecond)
+		return nil
+	}
+	if err := terminateAndReapDaemonProcess(cmd); err != nil {
+		return fmt.Errorf("%w; cleanup failed: %v", errDaemonStartTimeout, err)
 	}
-	return writeAppError(stderr, "daemon did not come up within timeout; see "+logPath, exitCrash)
+	return errDaemonStartTimeout
+}
+
+func waitForDaemonReadiness(reachable func() bool, timeout time.Duration, pollInterval time.Duration) bool {
+	deadline := time.Now().Add(timeout)
+	for {
+		if reachable() {
+			return true
+		}
+		remaining := time.Until(deadline)
+		if remaining <= 0 {
+			break
+		}
+		time.Sleep(min(pollInterval, remaining))
+	}
+	// Avoid killing a daemon that became reachable at the timeout boundary.
+	return reachable()
+}
+
+func terminateAndReapDaemonProcess(cmd *exec.Cmd) error {
+	return background.TerminateCommand(cmd)
 }
 
 func runDaemonStop(args []string, stdout io.Writer, stderr io.Writer) int {

internal/cli/daemon_test.go

@@ -2,8 +2,15 @@ package cli
 
 import (
 	"bytes"
+	"errors"
+	"os"
+	"os/exec"
+	"path/filepath"
 	"strings"
 	"testing"
+	"time"
+
+	"github.com/Gitlawb/zero/internal/background"
 )
 
 // isolateDaemonPaths points DefaultPaths at a temp dir so the test never touches
@@ -121,3 +128,84 @@ func TestDaemonSubcommandsRejectExtraArgs(t *testing.T) {
 		}
 	}
 }
+
+func TestWaitForDaemonReadinessChecksTimeoutBoundary(t *testing.T) {
+	checks := 0
+	ready := waitForDaemonReadiness(func() bool {
+		checks++
+		return checks == 2
+	}, 0, time.Millisecond)
+	if !ready {
+		t.Fatal("daemon that became reachable at the timeout boundary was not detected")
+	}
+	if checks != 2 {
+		t.Fatalf("reachability checks = %d, want initial and final checks", checks)
+	}
+}
+
+func TestTerminateAndReapDaemonProcess(t *testing.T) {
+	cmd := daemonDetachedChildCommand("hang")
+	if err := cmd.Start(); err != nil {
+		t.Fatalf("start helper process: %v", err)
+	}
+
+	if err := terminateAndReapDaemonProcess(cmd); err != nil {
+		t.Fatalf("terminate and reap helper process: %v", err)
+	}
+	if cmd.ProcessState == nil {
+		t.Fatalf("helper process was not reaped: state=%v", cmd.ProcessState)
+	}
+	if cmd.ProcessState.Success() {
+		t.Fatalf("helper process unexpectedly exited successfully: %v", cmd.ProcessState)
+	}
+}
+
+func TestStartAndAwaitDaemonProcessTimeoutTerminatesAndReaps(t *testing.T) {
+	cmd := daemonDetachedChildCommand("hang")
+	if err := startAndAwaitDaemonProcess(cmd, func() bool { return false }, 0, time.Millisecond); !errors.Is(err, errDaemonStartTimeout) {
+		t.Fatalf("start and await error = %v, want timeout", err)
+	}
+	if cmd.ProcessState == nil {
+		t.Fatal("timed-out helper process was not reaped")
+	}
+}
+
+func TestStartAndAwaitDaemonProcessReleasesReadyChild(t *testing.T) {
+	marker := filepath.Join(t.TempDir(), "child-finished")
+	cmd := daemonDetachedChildCommand("mark", "ZERO_TEST_DAEMON_CHILD_MARKER="+marker)
+	if err := startAndAwaitDaemonProcess(cmd, func() bool { return true }, time.Second, time.Millisecond); err != nil {
+		t.Fatalf("start and await ready helper: %v", err)
+	}
+
+	deadline := time.Now().Add(5 * time.Second)
+	for time.Now().Before(deadline) {
+		if _, err := os.Stat(marker); err == nil {
+			return
+		}
+		time.Sleep(10 * time.Millisecond)
+	}
+	t.Fatal("released helper process did not remain alive to write its marker")
+}
+
+func daemonDetachedChildCommand(mode string, extraEnv ...string) *exec.Cmd {
+	cmd := exec.Command(os.Args[0], "-test.run=TestDaemonDetachedChildProcess")
+	cmd.Env = append(os.Environ(), "ZERO_TEST_DAEMON_DETACHED_CHILD="+mode)
+	cmd.Env = append(cmd.Env, extraEnv...)
+	background.ConfigureChildProcessGroup(cmd)
+	return cmd
+}
+
+func TestDaemonDetachedChildProcess(t *testing.T) {
+	switch os.Getenv("ZERO_TEST_DAEMON_DETACHED_CHILD") {
+	case "":
+		return
+	case "mark":
+		if err := os.WriteFile(os.Getenv("ZERO_TEST_DAEMON_CHILD_MARKER"), []byte("ready"), 0o600); err != nil {
+			os.Exit(2)
+		}
+		os.Exit(0)
+	}
+	for {
+		time.Sleep(time.Hour)
+	}
+}