From 2f4d837c5f6f2f4430a33124c02669f2b4516da4 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Mon, 20 Jul 2026 15:41:58 +0200 Subject: [PATCH 1/8] fix(daemon): clean up child after startup timeout --- internal/cli/daemon.go | 64 ++++++++++++++++++++++++---- internal/cli/daemon_test.go | 84 +++++++++++++++++++++++++++++++++++++ 2 files changed, 139 insertions(+), 9 deletions(-) diff --git a/internal/cli/daemon.go b/internal/cli/daemon.go index 5074312e4..5f36559a9 100644 --- a/internal/cli/daemon.go +++ b/internal/cli/daemon.go @@ -1,6 +1,7 @@ package cli import ( + "errors" "fmt" "io" "os" @@ -160,20 +161,65 @@ 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 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 { + killErr := cmd.Process.Kill() + waitErr := cmd.Wait() + var exitErr *exec.ExitError + if waitErr == nil || errors.As(waitErr, &exitErr) { + return nil + } + if killErr != nil && !errors.Is(killErr, os.ErrProcessDone) { + return fmt.Errorf("terminate daemon process: %w", killErr) } - return writeAppError(stderr, "daemon did not come up within timeout; see "+logPath, exitCrash) + return fmt.Errorf("reap daemon process: %w", waitErr) } func runDaemonStop(args []string, stdout io.Writer, stderr io.Writer) int { diff --git a/internal/cli/daemon_test.go b/internal/cli/daemon_test.go index 5418537cc..573f010e6 100644 --- a/internal/cli/daemon_test.go +++ b/internal/cli/daemon_test.go @@ -2,8 +2,13 @@ package cli import ( "bytes" + "errors" + "os" + "os/exec" + "path/filepath" "strings" "testing" + "time" ) // isolateDaemonPaths points DefaultPaths at a temp dir so the test never touches @@ -121,3 +126,82 @@ 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 := exec.Command(os.Args[0], "-test.run=TestDaemonDetachedChildProcess") + cmd.Env = append(os.Environ(), "ZERO_TEST_DAEMON_DETACHED_CHILD=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 := exec.Command(os.Args[0], "-test.run=TestDaemonDetachedChildProcess") + cmd.Env = append(os.Environ(), "ZERO_TEST_DAEMON_DETACHED_CHILD=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 := exec.Command(os.Args[0], "-test.run=TestDaemonDetachedChildProcess") + cmd.Env = append(os.Environ(), + "ZERO_TEST_DAEMON_DETACHED_CHILD=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 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) + } +} From 27c9bd3e6432de73db54b3bdf1a1c10fc0e41f29 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Mon, 20 Jul 2026 16:31:09 +0200 Subject: [PATCH 2/8] fix(daemon): terminate startup process tree --- internal/background/process_posix.go | 76 +++++++++++++++++++++ internal/background/process_posix_test.go | 43 ++++++++++++ internal/background/process_windows.go | 25 +++++++ internal/background/process_windows_test.go | 24 +++++++ internal/background/terminate.go | 9 +++ internal/cli/daemon.go | 11 +-- internal/cli/daemon_test.go | 22 +++--- 7 files changed, 191 insertions(+), 19 deletions(-) create mode 100644 internal/background/process_windows_test.go diff --git a/internal/background/process_posix.go b/internal/background/process_posix.go index e4b100a38..1b6d7b9e3 100644 --- a/internal/background/process_posix.go +++ b/internal/background/process_posix.go @@ -5,6 +5,7 @@ package background import ( "errors" "fmt" + "os" "os/exec" "syscall" "time" @@ -103,6 +104,81 @@ func terminateProcess(pid int) error { return nil } +func terminateCommand(cmd *exec.Cmd) error { + pid := cmd.Process.Pid + if pid <= 1 { + return fmt.Errorf("refusing to terminate invalid pid %d", pid) + } + + target := pid + if pgid, err := syscall.Getpgid(pid); err == nil { + if pgid == pid { + target = -pid + } + } else if processGoneError(err) { + return waitForTerminatedCommand(cmd) + } else { + return err + } + + if err := syscall.Kill(target, syscall.SIGTERM); err != nil { + if processGoneError(err) { + return waitForTerminatedCommand(cmd) + } + killErr := cmd.Process.Kill() + if killErr != nil && !errors.Is(killErr, os.ErrProcessDone) { + return fmt.Errorf("terminate process group: %v (fallback kill failed: %w)", err, killErr) + } + if waitErr := waitForTerminatedCommand(cmd); waitErr != nil { + return waitErr + } + return fmt.Errorf("terminate process group: %w", err) + } + + waitDone := make(chan error, 1) + go func() { + waitDone <- cmd.Wait() + }() + alive := func() bool { return syscall.Kill(target, syscall.Signal(0)) == nil } + deadline := time.Now().Add(terminationGracePeriod) + for time.Now().Before(deadline) && alive() { + time.Sleep(terminationPollInterval) + } + if alive() { + if err := syscall.Kill(target, syscall.SIGKILL); err != nil && !processGoneError(err) { + return fmt.Errorf("force-kill process group: %w", err) + } + deadline = time.Now().Add(terminationGracePeriod) + for time.Now().Before(deadline) && alive() { + time.Sleep(terminationPollInterval) + } + } + + var waitErr error + select { + case waitErr = <-waitDone: + case <-time.After(terminationGracePeriod): + return fmt.Errorf("process %d did not reap after termination", pid) + } + var exitErr *exec.ExitError + if waitErr != nil && !errors.As(waitErr, &exitErr) { + return fmt.Errorf("reap process: %w", waitErr) + } + if alive() { + return fmt.Errorf("process group %d did not exit after SIGKILL", pid) + } + return nil +} + +func waitForTerminatedCommand(cmd *exec.Cmd) error { + waitErr := cmd.Wait() + var exitErr *exec.ExitError + if waitErr != nil && !errors.As(waitErr, &exitErr) { + return fmt.Errorf("reap process: %w", waitErr) + } + return nil +} + // processGoneError reports whether an error means the process group has already // exited (so termination is effectively done). syscall.Kill reports ESRCH when // no process in the target group remains. diff --git a/internal/background/process_posix_test.go b/internal/background/process_posix_test.go index fedd9205c..adf2abfa9 100644 --- a/internal/background/process_posix_test.go +++ b/internal/background/process_posix_test.go @@ -162,3 +162,46 @@ func TestTerminateProcessKillsForkedChildren(t *testing.T) { 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 { + if errors.Is(syscall.Kill(childPID, syscall.Signal(0)), syscall.ESRCH) { + break + } + if time.Now().After(deadline) { + _ = syscall.Kill(childPID, syscall.SIGKILL) + t.Fatalf("forked child %d survived TerminateCommand", childPID) + } + time.Sleep(20 * time.Millisecond) + } +} diff --git a/internal/background/process_windows.go b/internal/background/process_windows.go index b032e2004..8b9ccca4b 100644 --- a/internal/background/process_windows.go +++ b/internal/background/process_windows.go @@ -3,6 +3,8 @@ package background import ( + "errors" + "fmt" "os" "os/exec" "path/filepath" @@ -26,6 +28,29 @@ func terminateProcess(pid int) error { return process.Kill() } +func terminateCommand(cmd *exec.Cmd) error { + taskkillErr := exec.Command(taskkillPath(), "/T", "/F", "/PID", strconv.Itoa(cmd.Process.Pid)).Run() + var killErr error + if taskkillErr != nil { + killErr = cmd.Process.Kill() + } + waitErr := cmd.Wait() + var exitErr *exec.ExitError + if waitErr != nil && !errors.As(waitErr, &exitErr) { + return fmt.Errorf("reap process: %w", waitErr) + } + if taskkillErr == nil { + return nil + } + if killErr != nil && !errors.Is(killErr, os.ErrProcessDone) { + return fmt.Errorf("terminate process tree: %v (fallback kill failed: %w)", taskkillErr, killErr) + } + // ErrProcessDone proves only that the leader is gone. Keep the taskkill + // failure actionable because its former descendants cannot be verified by + // PID after the leader exits. + return fmt.Errorf("terminate process tree: %w", taskkillErr) +} + func taskkillPath() string { systemRoot := os.Getenv("SystemRoot") if systemRoot == "" { diff --git a/internal/background/process_windows_test.go b/internal/background/process_windows_test.go new file mode 100644 index 000000000..a4f527ec6 --- /dev/null +++ b/internal/background/process_windows_test.go @@ -0,0 +1,24 @@ +//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) + + _ = TerminateCommand(cmd) + if cmd.ProcessState == nil { + t.Fatal("exited process was not reaped") + } +} diff --git a/internal/background/terminate.go b/internal/background/terminate.go index bc9ce7dfc..f6cf9cdcc 100644 --- a/internal/background/terminate.go +++ b/internal/background/terminate.go @@ -1,5 +1,7 @@ package background +import "os/exec" + // 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 +14,10 @@ package background func TerminateProcess(pid int) error { return terminateProcess(pid) } + +// TerminateCommand stops a started command's process tree/group and reaps the +// leader. Keeping both operations together lets platform implementations signal +// the tree before Wait can discard the leader identity needed to find it. +func TerminateCommand(cmd *exec.Cmd) error { + return terminateCommand(cmd) +} diff --git a/internal/cli/daemon.go b/internal/cli/daemon.go index 5f36559a9..2dfebd0ba 100644 --- a/internal/cli/daemon.go +++ b/internal/cli/daemon.go @@ -210,16 +210,7 @@ func waitForDaemonReadiness(reachable func() bool, timeout time.Duration, pollIn } func terminateAndReapDaemonProcess(cmd *exec.Cmd) error { - killErr := cmd.Process.Kill() - waitErr := cmd.Wait() - var exitErr *exec.ExitError - if waitErr == nil || errors.As(waitErr, &exitErr) { - return nil - } - if killErr != nil && !errors.Is(killErr, os.ErrProcessDone) { - return fmt.Errorf("terminate daemon process: %w", killErr) - } - return fmt.Errorf("reap daemon process: %w", waitErr) + return background.TerminateCommand(cmd) } func runDaemonStop(args []string, stdout io.Writer, stderr io.Writer) int { diff --git a/internal/cli/daemon_test.go b/internal/cli/daemon_test.go index 573f010e6..3e316b8c1 100644 --- a/internal/cli/daemon_test.go +++ b/internal/cli/daemon_test.go @@ -9,6 +9,8 @@ import ( "strings" "testing" "time" + + "github.com/Gitlawb/zero/internal/background" ) // isolateDaemonPaths points DefaultPaths at a temp dir so the test never touches @@ -142,8 +144,7 @@ func TestWaitForDaemonReadinessChecksTimeoutBoundary(t *testing.T) { } func TestTerminateAndReapDaemonProcess(t *testing.T) { - cmd := exec.Command(os.Args[0], "-test.run=TestDaemonDetachedChildProcess") - cmd.Env = append(os.Environ(), "ZERO_TEST_DAEMON_DETACHED_CHILD=hang") + cmd := daemonDetachedChildCommand("hang") if err := cmd.Start(); err != nil { t.Fatalf("start helper process: %v", err) } @@ -160,8 +161,7 @@ func TestTerminateAndReapDaemonProcess(t *testing.T) { } func TestStartAndAwaitDaemonProcessTimeoutTerminatesAndReaps(t *testing.T) { - cmd := exec.Command(os.Args[0], "-test.run=TestDaemonDetachedChildProcess") - cmd.Env = append(os.Environ(), "ZERO_TEST_DAEMON_DETACHED_CHILD=hang") + 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) } @@ -172,11 +172,7 @@ func TestStartAndAwaitDaemonProcessTimeoutTerminatesAndReaps(t *testing.T) { func TestStartAndAwaitDaemonProcessReleasesReadyChild(t *testing.T) { marker := filepath.Join(t.TempDir(), "child-finished") - cmd := exec.Command(os.Args[0], "-test.run=TestDaemonDetachedChildProcess") - cmd.Env = append(os.Environ(), - "ZERO_TEST_DAEMON_DETACHED_CHILD=mark", - "ZERO_TEST_DAEMON_CHILD_MARKER="+marker, - ) + 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) } @@ -191,6 +187,14 @@ func TestStartAndAwaitDaemonProcessReleasesReadyChild(t *testing.T) { 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 "": From f73194fcc1fb7e5be95fc37a080359b6ac023530 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Mon, 20 Jul 2026 16:39:41 +0200 Subject: [PATCH 3/8] test(background): accept reaped-later zombie children --- internal/background/process_posix_test.go | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/internal/background/process_posix_test.go b/internal/background/process_posix_test.go index adf2abfa9..28afed9bf 100644 --- a/internal/background/process_posix_test.go +++ b/internal/background/process_posix_test.go @@ -147,13 +147,12 @@ 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 + if processStopped(childPID) { + break } if time.Now().After(deadline) { _ = syscall.Kill(childPID, syscall.SIGKILL) @@ -195,7 +194,7 @@ func TestTerminateCommandKillsChildAfterLeaderExits(t *testing.T) { deadline := time.Now().Add(2 * time.Second) for { - if errors.Is(syscall.Kill(childPID, syscall.Signal(0)), syscall.ESRCH) { + if processStopped(childPID) { break } if time.Now().After(deadline) { @@ -205,3 +204,14 @@ func TestTerminateCommandKillsChildAfterLeaderExits(t *testing.T) { 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") +} From 4cb866e042e888e02227605e792462b32450b151 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Mon, 20 Jul 2026 17:10:35 +0200 Subject: [PATCH 4/8] fix(background): retain configured process group target --- internal/background/process_posix.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/internal/background/process_posix.go b/internal/background/process_posix.go index 1b6d7b9e3..ab0e0e3a1 100644 --- a/internal/background/process_posix.go +++ b/internal/background/process_posix.go @@ -111,7 +111,12 @@ func terminateCommand(cmd *exec.Cmd) error { } target := pid - if pgid, err := syscall.Getpgid(pid); err == nil { + // ConfigureChildProcessGroup records that the child leads a group whose ID + // is its PID. Use that launch-time fact directly: on Darwin Getpgid can + // return ESRCH once the leader exits even while descendants remain alive. + if cmd.SysProcAttr != nil && cmd.SysProcAttr.Setpgid && cmd.SysProcAttr.Pgid == 0 { + target = -pid + } else if pgid, err := syscall.Getpgid(pid); err == nil { if pgid == pid { target = -pid } From 3ed4571da6f0d830fbe42a992e70c04d0169f42f Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Mon, 20 Jul 2026 17:23:42 +0200 Subject: [PATCH 5/8] fix(background): address termination review feedback --- internal/background/process_posix.go | 25 ++++++++++++-------- internal/background/process_posix_test.go | 10 ++------ internal/background/process_windows.go | 6 ++--- internal/background/terminate.go | 28 ++++++++++++++++++++++- 4 files changed, 46 insertions(+), 23 deletions(-) diff --git a/internal/background/process_posix.go b/internal/background/process_posix.go index ab0e0e3a1..734848ebf 100644 --- a/internal/background/process_posix.go +++ b/internal/background/process_posix.go @@ -123,7 +123,18 @@ func terminateCommand(cmd *exec.Cmd) error { } else if processGoneError(err) { return waitForTerminatedCommand(cmd) } else { - return err + killErr := cmd.Process.Kill() + waitErr := waitForTerminatedCommandWithin(cmd, terminationGracePeriod) + if killErr != nil && !errors.Is(killErr, os.ErrProcessDone) { + if waitErr != nil { + return fmt.Errorf("get process group: %v (fallback kill failed: %v; %w)", err, killErr, waitErr) + } + return fmt.Errorf("get process group: %v (fallback kill failed: %w)", err, killErr) + } + if waitErr != nil { + return fmt.Errorf("get process group: %v (%w)", err, waitErr) + } + return fmt.Errorf("get process group: %w", err) } if err := syscall.Kill(target, syscall.SIGTERM); err != nil { @@ -165,9 +176,8 @@ func terminateCommand(cmd *exec.Cmd) error { case <-time.After(terminationGracePeriod): return fmt.Errorf("process %d did not reap after termination", pid) } - var exitErr *exec.ExitError - if waitErr != nil && !errors.As(waitErr, &exitErr) { - return fmt.Errorf("reap process: %w", waitErr) + if err := classifyWaitError(waitErr); err != nil { + return err } if alive() { return fmt.Errorf("process group %d did not exit after SIGKILL", pid) @@ -176,12 +186,7 @@ func terminateCommand(cmd *exec.Cmd) error { } func waitForTerminatedCommand(cmd *exec.Cmd) error { - waitErr := cmd.Wait() - var exitErr *exec.ExitError - if waitErr != nil && !errors.As(waitErr, &exitErr) { - return fmt.Errorf("reap process: %w", waitErr) - } - return nil + return classifyWaitError(cmd.Wait()) } // processGoneError reports whether an error means the process group has already diff --git a/internal/background/process_posix_test.go b/internal/background/process_posix_test.go index 28afed9bf..83d6a7046 100644 --- a/internal/background/process_posix_test.go +++ b/internal/background/process_posix_test.go @@ -150,10 +150,7 @@ func TestTerminateProcessKillsForkedChildren(t *testing.T) { // 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 { - if processStopped(childPID) { - break - } + for !processStopped(childPID) { if time.Now().After(deadline) { _ = syscall.Kill(childPID, syscall.SIGKILL) t.Fatalf("forked child %d survived terminateProcess — group kill failed", childPID) @@ -193,10 +190,7 @@ func TestTerminateCommandKillsChildAfterLeaderExits(t *testing.T) { } deadline := time.Now().Add(2 * time.Second) - for { - if processStopped(childPID) { - break - } + for !processStopped(childPID) { if time.Now().After(deadline) { _ = syscall.Kill(childPID, syscall.SIGKILL) t.Fatalf("forked child %d survived TerminateCommand", childPID) diff --git a/internal/background/process_windows.go b/internal/background/process_windows.go index 8b9ccca4b..52a9ebada 100644 --- a/internal/background/process_windows.go +++ b/internal/background/process_windows.go @@ -34,10 +34,8 @@ func terminateCommand(cmd *exec.Cmd) error { if taskkillErr != nil { killErr = cmd.Process.Kill() } - waitErr := cmd.Wait() - var exitErr *exec.ExitError - if waitErr != nil && !errors.As(waitErr, &exitErr) { - return fmt.Errorf("reap process: %w", waitErr) + if err := waitForTerminatedCommandWithin(cmd, commandReapTimeout); err != nil { + return err } if taskkillErr == nil { return nil diff --git a/internal/background/terminate.go b/internal/background/terminate.go index f6cf9cdcc..72f828168 100644 --- a/internal/background/terminate.go +++ b/internal/background/terminate.go @@ -1,6 +1,13 @@ package background -import "os/exec" +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 @@ -21,3 +28,22 @@ func TerminateProcess(pid int) error { func TerminateCommand(cmd *exec.Cmd) error { return terminateCommand(cmd) } + +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 +} + +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) + } +} From b63289bf4b75a2bf8fa805960698a5a2fc68203c Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Sun, 26 Jul 2026 14:37:57 +0200 Subject: [PATCH 6/8] fix(daemon): use launch-time group identity, fix zombie/reap classification MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses jatmn's three #774 review findings: - TerminateCommand rediscovered the process group via execution.TerminateProcessTree's Getpgid(pid) lookup. On Darwin, that lookup can return ESRCH once an unreaped group leader has already exited, even though live descendants remain in the group it configured at launch — silently falling back to signalling only the (already dead) leader PID and leaving descendants running (the required macOS smoke job's failure). Add execution.TerminateProcessGroup, which signals the negative PID directly using the launch-time Setpgid invariant instead of rediscovering it, and route TerminateCommand through it. - signalTargetRunning's zombie check matched the individual-PID fallback target (a process that is not its own group leader) against the process table by PGID. That target's actual PGID differs from its own PID by definition, so no row ever matched, unknown resulted every time, and a zombie individual-PID target was always conservatively reported as still running. Branch on the sign of the target: group members by PGID, individual PIDs by PID. - TerminateCommand returned the termination error even when the subsequent reap succeeded — a successful cmd.Wait is authoritative proof the leader is gone and collected, regardless of how the kill attempt itself went (e.g. taskkill /T racing an already-exited PID on Windows). Classify a successful reap as success. All three regression tests are verified to fail against the pre-fix code (confirmed by temporarily reverting each fix and rerunning). Co-Authored-By: Claude Sonnet 5 --- internal/background/process_posix.go | 11 ++++ internal/background/process_windows.go | 8 +++ internal/background/process_windows_test.go | 8 ++- internal/background/terminate.go | 15 +++-- internal/execution/process_unix.go | 70 ++++++++++++++++++--- internal/execution/process_unix_test.go | 43 +++++++++++++ 6 files changed, 139 insertions(+), 16 deletions(-) diff --git a/internal/background/process_posix.go b/internal/background/process_posix.go index 62a30deac..61f92e8b3 100644 --- a/internal/background/process_posix.go +++ b/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) +} diff --git a/internal/background/process_windows.go b/internal/background/process_windows.go index c248bc853..e9b14ef7a 100644 --- a/internal/background/process_windows.go +++ b/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) +} diff --git a/internal/background/process_windows_test.go b/internal/background/process_windows_test.go index a4f527ec6..1d5dc7ae7 100644 --- a/internal/background/process_windows_test.go +++ b/internal/background/process_windows_test.go @@ -17,7 +17,13 @@ func TestTerminateCommandReapsExitedLeader(t *testing.T) { // taskkill/TerminateProcess race while it still owns the process handle. time.Sleep(500 * time.Millisecond) - _ = TerminateCommand(cmd) + // 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") } diff --git a/internal/background/terminate.go b/internal/background/terminate.go index 3a9e48dd6..76dae3188 100644 --- a/internal/background/terminate.go +++ b/internal/background/terminate.go @@ -38,15 +38,20 @@ func TerminateCommand(cmd *exec.Cmd) error { if cmd == nil || cmd.Process == nil { return errors.New("terminate command: process was never started") } - terminateErr := terminateProcess(cmd.Process.Pid) + terminateErr := terminateOwnedProcess(cmd) reapErr := waitForTerminatedCommandWithin(cmd, commandReapTimeout) - if terminateErr != nil { - if reapErr != nil { + if reapErr != nil { + if terminateErr != nil { return fmt.Errorf("%v (reap failed: %w)", terminateErr, reapErr) } - return terminateErr + return 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 diff --git a/internal/execution/process_unix.go b/internal/execution/process_unix.go index 94312b92e..8464dd028 100644 --- a/internal/execution/process_unix.go +++ b/internal/execution/process_unix.go @@ -40,11 +40,41 @@ func KillProcessTree(pid int) error { // TerminateProcessTree requests graceful termination, then force-kills the // process tree after grace. Callers retain their distinct persistence models; // this function owns only the OS lifecycle primitive. +// +// The signal target is REDISCOVERED here via processSignalTarget(pid), which +// asks the OS for pid's current group. A caller that already knows pid was +// configured as its own group leader at launch (e.g. via ConfigureProcessGroup) +// should use TerminateProcessGroup instead — see its doc comment for why this +// rediscovery is fragile once the leader may have already exited. func TerminateProcessTree(pid int, grace, poll time.Duration) error { target, err := processSignalTarget(pid) if err != nil { return err } + return terminateTarget(pid, target, grace, poll) +} + +// TerminateProcessGroup is like TerminateProcessTree, but for a caller that +// already knows pid is (or was, at launch) its own process-group leader — e.g. +// a command started after ConfigureProcessGroup(cmd), which sets Setpgid so +// cmd.Process.Pid always leads its own group at the moment it is signalled. +// +// TerminateProcessTree instead rediscovers the group via syscall.Getpgid(pid) +// at call time. On Darwin, that lookup can return ESRCH once an unreaped group +// leader has already exited, even though live descendants remain in the group +// it configured — TerminateProcessTree would then silently fall back to +// signalling only the (already dead) leader PID and leave those descendants +// running. Skipping rediscovery in favor of the launch-time invariant avoids +// that: the negative PID is used directly, regardless of whether the leader is +// still alive to be looked up. +func TerminateProcessGroup(pid int, grace, poll time.Duration) error { + return terminateTarget(pid, -pid, grace, poll) +} + +// terminateTarget signals target (an individual PID or, negative, a process +// group) with SIGTERM then, after grace, SIGKILL. reportPid identifies the +// original process in error messages regardless of which form target takes. +func terminateTarget(reportPid, target int, grace, poll time.Duration) error { alive := func() bool { return signalTargetRunning(target) } if err := syscall.Kill(target, syscall.SIGTERM); err != nil { if errors.Is(err, syscall.ESRCH) { @@ -79,7 +109,7 @@ func TerminateProcessTree(pid int, grace, poll time.Duration) error { time.Sleep(poll) } if alive() { - return fmt.Errorf("process %d did not exit after SIGKILL", pid) + return fmt.Errorf("process %d did not exit after SIGKILL", reportPid) } return nil } @@ -105,11 +135,18 @@ func signalTargetRunning(target int) bool { if syscall.Kill(target, syscall.Signal(0)) != nil { return false } - pgid := target - if pgid < 0 { - pgid = -pgid + // A negative target is a process GROUP: check whether any member is + // non-zombie. A positive target is an individual PID that is NOT its own + // group leader (processSignalTarget's fallback) — its actual PGID differs + // from its own PID, so matching rows by PGID here would look for a group + // that doesn't exist and always report "unknown" (conservatively alive). + // Check its own PID's state instead. + var running, ok bool + if target < 0 { + running, ok = processGroupHasRunningMember(-target) + } else { + running, ok = processIsRunning(target) } - running, ok := processGroupHasRunningMember(pgid) if !ok { return true } @@ -119,6 +156,21 @@ func signalTargetRunning(target int) bool { // processGroupHasRunningMember reports whether any member of pgid is in a state // other than zombie. ok is false when the group's states could not be determined. func processGroupHasRunningMember(pgid int) (running bool, ok bool) { + return processTableStateMatches(func(_, rowPgid int) bool { return rowPgid == pgid }) +} + +// processIsRunning reports whether pid itself is in a state other than zombie. +// ok is false when pid's state could not be determined. +func processIsRunning(pid int) (running bool, ok bool) { + return processTableStateMatches(func(rowPid, _ int) bool { return rowPid == pid }) +} + +// processTableStateMatches scans the process table once, via ps, and reports +// whether any row satisfying match is non-zombie, and whether at least one +// matching row was found at all (ok). No rows found means either the process +// (or group) genuinely doesn't exist (a race with exit) or ps's output was +// unparseable/restricted; either way, the caller should not claim knowledge. +func processTableStateMatches(match func(pid, pgid int) bool) (running bool, ok bool) { output, err := exec.Command("ps", "-A", "-o", "pid=,pgid=,stat=").Output() if err != nil { return false, false @@ -129,8 +181,9 @@ func processGroupHasRunningMember(pgid int) (running bool, ok bool) { if len(fields) < 3 { continue } - memberPgid, err := strconv.Atoi(fields[1]) - if err != nil || memberPgid != pgid { + rowPid, errPid := strconv.Atoi(fields[0]) + rowPgid, errPgid := strconv.Atoi(fields[1]) + if errPid != nil || errPgid != nil || !match(rowPid, rowPgid) { continue } found = true @@ -138,9 +191,6 @@ func processGroupHasRunningMember(pgid int) (running bool, ok bool) { return true, true } } - // No rows at all means ps could not see the group (a race with exit, or an - // environment where the listing is restricted); only claim knowledge when at - // least one member was observed. return false, found } diff --git a/internal/execution/process_unix_test.go b/internal/execution/process_unix_test.go index 39e26ca3b..36ba99189 100644 --- a/internal/execution/process_unix_test.go +++ b/internal/execution/process_unix_test.go @@ -72,6 +72,49 @@ func TestTerminateProcessTreeStopsRunningGroup(t *testing.T) { } } +// TestSignalTargetRunningTreatsZombieIndividualPIDAsExited is the regression +// test for jatmn's second #774 finding: signalTargetRunning's zombie check +// matched the individual-PID fallback target (a process that is NOT its own +// group leader — processSignalTarget's positive-PID case) against process-table +// rows by PGID. That target's actual PGID differs from its own PID by +// definition (that's exactly why processSignalTarget fell back to the +// individual PID instead of a negative group target), so no row ever matched, +// "unknown" resulted every time, and signalTargetRunning conservatively (and +// incorrectly) treated a zombie individual-PID target as still running. +func TestSignalTargetRunningTreatsZombieIndividualPIDAsExited(t *testing.T) { + cmd := exec.Command("sh", "-c", "exit 0") + // Deliberately do NOT call ConfigureProcessGroup: the child inherits this + // test process's group, so it is not its own leader and processSignalTarget + // falls back to the individual (positive) PID. + if err := cmd.Start(); err != nil { + t.Fatalf("start: %v", err) + } + pid := cmd.Process.Pid + t.Cleanup(func() { _ = cmd.Wait() }) + + deadline := time.Now().Add(5 * time.Second) + for { + if zombie, ok := processIsZombie(pid); ok && zombie { + break + } + if time.Now().After(deadline) { + t.Skip("child did not reach the zombie state; ps output is unavailable in this environment") + } + time.Sleep(10 * time.Millisecond) + } + + target, err := processSignalTarget(pid) + if err != nil { + t.Fatalf("processSignalTarget: %v", err) + } + if target != pid { + t.Skip("child unexpectedly became its own group leader; cannot exercise the individual-PID fallback") + } + if signalTargetRunning(target) { + t.Fatal("a zombie individual-PID target must not count as running") + } +} + // processIsZombie reports a process's zombie state via ps. ok is false when the // state could not be read. func processIsZombie(pid int) (zombie bool, ok bool) { From dce4f27e0b140805e258087a96bc02d4ab41b01f Mon Sep 17 00:00:00 2001 From: Amp Date: Sun, 26 Jul 2026 20:47:41 +0000 Subject: [PATCH 7/8] fix(daemon): harden command termination inputs Amp-Thread-ID: https://ampcode.com/threads/T-019fa018-abb8-73de-9489-51e9e6514c6b Co-authored-by: Pierre Bruno --- internal/background/process_posix.go | 16 ++++++++-------- internal/background/process_posix_test.go | 19 +++++++++++++++++++ internal/background/terminate.go | 10 +++++----- internal/execution/process_unix.go | 15 +++++++++++---- 4 files changed, 43 insertions(+), 17 deletions(-) diff --git a/internal/background/process_posix.go b/internal/background/process_posix.go index 61f92e8b3..626d01e2f 100644 --- a/internal/background/process_posix.go +++ b/internal/background/process_posix.go @@ -40,13 +40,13 @@ 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). +// 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) error { - return execution.TerminateProcessGroup(cmd.Process.Pid, terminationGracePeriod, terminationPollInterval) + if cmd.SysProcAttr != nil && cmd.SysProcAttr.Setpgid && cmd.SysProcAttr.Pgid == 0 { + return execution.TerminateProcessGroup(cmd.Process.Pid, terminationGracePeriod, terminationPollInterval) + } + return terminateProcess(cmd.Process.Pid) } diff --git a/internal/background/process_posix_test.go b/internal/background/process_posix_test.go index 83d6a7046..5482ef9f0 100644 --- a/internal/background/process_posix_test.go +++ b/internal/background/process_posix_test.go @@ -159,6 +159,25 @@ func TestTerminateProcessKillsForkedChildren(t *testing.T) { } } +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 diff --git a/internal/background/terminate.go b/internal/background/terminate.go index 76dae3188..c9e6b205b 100644 --- a/internal/background/terminate.go +++ b/internal/background/terminate.go @@ -22,8 +22,9 @@ 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 +// TerminateCommand stops a started command and reaps its leader. On POSIX it +// stops the whole group when the command was configured as its leader; ordinary +// commands safely fall back to PID/tree discovery. 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 @@ -31,9 +32,8 @@ func TerminateProcess(pid int) error { // // 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. +// to a recycled PID). Termination stays in the shared execution lifecycle +// primitives rather than introducing a second platform-specific killer. func TerminateCommand(cmd *exec.Cmd) error { if cmd == nil || cmd.Process == nil { return errors.New("terminate command: process was never started") diff --git a/internal/execution/process_unix.go b/internal/execution/process_unix.go index 8464dd028..58b2b0156 100644 --- a/internal/execution/process_unix.go +++ b/internal/execution/process_unix.go @@ -68,6 +68,9 @@ func TerminateProcessTree(pid int, grace, poll time.Duration) error { // that: the negative PID is used directly, regardless of whether the leader is // still alive to be looked up. func TerminateProcessGroup(pid int, grace, poll time.Duration) error { + if pid <= 1 { + return fmt.Errorf("refusing to signal invalid pid %d", pid) + } return terminateTarget(pid, -pid, grace, poll) } @@ -203,10 +206,14 @@ func processSignalTarget(pid int) (int, error) { if pgid == pid { target = -pid } - } else if errors.Is(err, syscall.ESRCH) { - // Preserve the individual target; the signal call below treats ESRCH as - // already gone, which is a successful lifecycle outcome. - return pid, nil + } else { + if errors.Is(err, syscall.ESRCH) { + // Preserve the individual target; the signal call below treats ESRCH as + // already gone, which is a successful lifecycle outcome. + return pid, nil + } + // Conservatively retain the individual PID after other lookup failures; + // guessing and signalling a process group could affect unrelated processes. } return target, nil } From e22f193a0e5dec65ed8d76b090a43618bffa6509 Mon Sep 17 00:00:00 2001 From: Amp Date: Tue, 28 Jul 2026 10:40:37 +0000 Subject: [PATCH 8/8] fix(background): preserve process tree failures Amp-Thread-ID: https://ampcode.com/threads/T-019fa7a0-1223-701d-9529-48ba5d7cf8c8 Co-authored-by: Pierre Bruno --- internal/background/process_posix.go | 6 +- internal/background/process_windows.go | 38 ++++++++-- internal/background/process_windows_test.go | 80 ++++++++++++++++++++- internal/background/terminate.go | 35 +++++---- internal/execution/process_windows.go | 28 +++++++- internal/execution/process_windows_test.go | 29 ++++++++ 6 files changed, 191 insertions(+), 25 deletions(-) create mode 100644 internal/execution/process_windows_test.go diff --git a/internal/background/process_posix.go b/internal/background/process_posix.go index 626d01e2f..b6c763e9a 100644 --- a/internal/background/process_posix.go +++ b/internal/background/process_posix.go @@ -44,9 +44,9 @@ func terminateProcess(pid int) error { // 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) error { +func terminateOwnedProcess(cmd *exec.Cmd) (bool, error) { if cmd.SysProcAttr != nil && cmd.SysProcAttr.Setpgid && cmd.SysProcAttr.Pgid == 0 { - return execution.TerminateProcessGroup(cmd.Process.Pid, terminationGracePeriod, terminationPollInterval) + return false, execution.TerminateProcessGroup(cmd.Process.Pid, terminationGracePeriod, terminationPollInterval) } - return terminateProcess(cmd.Process.Pid) + return false, terminateProcess(cmd.Process.Pid) } diff --git a/internal/background/process_windows.go b/internal/background/process_windows.go index e9b14ef7a..1671f3000 100644 --- a/internal/background/process_windows.go +++ b/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). @@ -17,10 +26,27 @@ 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) +// 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 } diff --git a/internal/background/process_windows_test.go b/internal/background/process_windows_test.go index 1d5dc7ae7..15ee0eb93 100644 --- a/internal/background/process_windows_test.go +++ b/internal/background/process_windows_test.go @@ -3,11 +3,39 @@ 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 { @@ -19,8 +47,8 @@ func TestTerminateCommandReapsExitedLeader(t *testing.T) { // 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. + // 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) } @@ -28,3 +56,51 @@ func TestTerminateCommandReapsExitedLeader(t *testing.T) { 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 +} diff --git a/internal/background/terminate.go b/internal/background/terminate.go index c9e6b205b..49945e5db 100644 --- a/internal/background/terminate.go +++ b/internal/background/terminate.go @@ -22,23 +22,29 @@ func TerminateProcess(pid int) error { return terminateProcess(pid) } -// TerminateCommand stops a started command and reaps its leader. On POSIX it +// 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. 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. +// 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") } - terminateErr := terminateOwnedProcess(cmd) + leaderAlreadyExited, terminateErr := terminateOwnedProcess(cmd) reapErr := waitForTerminatedCommandWithin(cmd, commandReapTimeout) if reapErr != nil { if terminateErr != nil { @@ -46,11 +52,12 @@ func TerminateCommand(cmd *exec.Cmd) error { } 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. + 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 } @@ -67,7 +74,9 @@ func classifyWaitError(waitErr error) error { // 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. +// 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() }() diff --git a/internal/execution/process_windows.go b/internal/execution/process_windows.go index beb919535..fcf51eb4b 100644 --- a/internal/execution/process_windows.go +++ b/internal/execution/process_windows.go @@ -3,6 +3,8 @@ package execution import ( + "errors" + "fmt" "os" "os/exec" "path/filepath" @@ -12,10 +14,34 @@ import ( func ConfigureProcessGroup(cmd *exec.Cmd) {} +// KillProcessTree asks taskkill /T to kill pid and descendants it can still +// discover from that live root. If taskkill fails, it falls back to killing pid +// alone but still returns the tree error because descendants may have survived. +// Neither path can recover descendants after the root has exited. func KillProcessTree(pid int) error { - if err := exec.Command(taskkillPath(), "/T", "/F", "/PID", strconv.Itoa(pid)).Run(); err == nil { + return killProcessTree(pid, runTaskkill, killRootProcess) +} + +func killProcessTree(pid int, killTree, killRoot func(int) error) error { + treeErr := killTree(pid) + if treeErr == nil { return nil } + rootErr := killRoot(pid) + if rootErr != nil { + return errors.Join( + fmt.Errorf("kill process tree rooted at %d: %w", pid, treeErr), + fmt.Errorf("kill root process %d: %w", pid, rootErr), + ) + } + return fmt.Errorf("kill process tree rooted at %d (root process killed directly): %w", pid, treeErr) +} + +func runTaskkill(pid int) error { + return exec.Command(taskkillPath(), "/T", "/F", "/PID", strconv.Itoa(pid)).Run() +} + +func killRootProcess(pid int) error { process, err := os.FindProcess(pid) if err != nil { return err diff --git a/internal/execution/process_windows_test.go b/internal/execution/process_windows_test.go new file mode 100644 index 000000000..5f948d5cf --- /dev/null +++ b/internal/execution/process_windows_test.go @@ -0,0 +1,29 @@ +//go:build windows + +package execution + +import ( + "errors" + "strings" + "testing" +) + +func TestKillProcessTreeRetainsTreeErrorWhenRootFallbackSucceeds(t *testing.T) { + treeErr := errors.New("taskkill failed") + err := killProcessTree(42, func(int) error { return treeErr }, func(int) error { return nil }) + if !errors.Is(err, treeErr) { + t.Fatalf("killProcessTree error = %v, want tree error", err) + } + if !strings.Contains(err.Error(), "root process killed directly") { + t.Fatalf("killProcessTree error = %q, want fallback context", err) + } +} + +func TestKillProcessTreeJoinsTreeAndRootErrors(t *testing.T) { + treeErr := errors.New("taskkill failed") + rootErr := errors.New("root kill failed") + err := killProcessTree(42, func(int) error { return treeErr }, func(int) error { return rootErr }) + if !errors.Is(err, treeErr) || !errors.Is(err, rootErr) { + t.Fatalf("killProcessTree error = %v, want both failures", err) + } +}