From 2f4d837c5f6f2f4430a33124c02669f2b4516da4 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Mon, 20 Jul 2026 15:41:58 +0200 Subject: [PATCH 1/6] 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/6] 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/6] 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/6] 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/6] 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/6] 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) {