diff --git a/internal/background/process_posix.go b/internal/background/process_posix.go index 62a30deac..b6c763e9a 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 group directly when its launch +// configuration proves it was made a group leader. This avoids fragile Getpgid +// rediscovery after an owned leader exits. Ordinary commands fall back to the +// safe PID/tree path rather than assuming their PID is also a process-group ID. +func terminateOwnedProcess(cmd *exec.Cmd) (bool, error) { + if cmd.SysProcAttr != nil && cmd.SysProcAttr.Setpgid && cmd.SysProcAttr.Pgid == 0 { + return false, execution.TerminateProcessGroup(cmd.Process.Pid, terminationGracePeriod, terminationPollInterval) + } + return false, terminateProcess(cmd.Process.Pid) +} diff --git a/internal/background/process_posix_test.go b/internal/background/process_posix_test.go index ad1932b3b..b2080134c 100644 --- a/internal/background/process_posix_test.go +++ b/internal/background/process_posix_test.go @@ -147,11 +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) - // Only ESRCH proves the child is gone; any other error (e.g. EPERM) would - // wrongly pass the test, so treat it as still-present and keep polling. - for !errors.Is(syscall.Kill(childPID, syscall.Signal(0)), syscall.ESRCH) { + // processStopped treats only ESRCH or an observed zombie state as stopped; + // other errors (for example EPERM) remain live and keep polling. + for !processStopped(childPID) { if time.Now().After(deadline) { _ = syscall.Kill(childPID, syscall.SIGKILL) t.Fatalf("forked child %d survived terminateProcess — group kill failed", childPID) @@ -159,3 +160,73 @@ func TestTerminateProcessKillsForkedChildren(t *testing.T) { time.Sleep(20 * time.Millisecond) } } + +func TestTerminateCommandStopsUnconfiguredCommand(t *testing.T) { + cmd := exec.Command("sleep", "30") + if err := cmd.Start(); err != nil { + t.Fatalf("start: %v", err) + } + t.Cleanup(func() { + if cmd.ProcessState == nil { + _ = cmd.Process.Kill() + } + }) + + if err := TerminateCommand(cmd); err != nil { + t.Fatalf("TerminateCommand: %v", err) + } + if cmd.ProcessState == nil { + t.Fatal("unconfigured command was not reaped") + } +} + +func TestTerminateCommandKillsChildAfterLeaderExits(t *testing.T) { + grace, poll := terminationGracePeriod, terminationPollInterval + terminationGracePeriod, terminationPollInterval = 2*time.Second, 20*time.Millisecond + t.Cleanup(func() { terminationGracePeriod, terminationPollInterval = grace, poll }) + + // The leader exits immediately after launching the child. TerminateCommand + // must capture and signal the process group before Wait reaps the leader and + // makes its group identity unavailable. + cmd := exec.Command("sh", "-c", "sleep 300 & echo $!; exit 0") + ConfigureChildProcessGroup(cmd) + stdout, err := cmd.StdoutPipe() + if err != nil { + t.Fatalf("stdout pipe: %v", err) + } + if err := cmd.Start(); err != nil { + t.Fatalf("start: %v", err) + } + line, err := bufio.NewReader(stdout).ReadString('\n') + if err != nil { + t.Fatalf("read forked child pid: %v", err) + } + childPID, err := strconv.Atoi(strings.TrimSpace(line)) + if err != nil { + t.Fatalf("parse forked child pid %q: %v", line, err) + } + + if err := TerminateCommand(cmd); err != nil { + t.Fatalf("TerminateCommand: %v", err) + } + + deadline := time.Now().Add(2 * time.Second) + for !processStopped(childPID) { + if time.Now().After(deadline) { + _ = syscall.Kill(childPID, syscall.SIGKILL) + t.Fatalf("forked child %d survived TerminateCommand", childPID) + } + time.Sleep(20 * time.Millisecond) + } +} + +func processStopped(pid int) bool { + if errors.Is(syscall.Kill(pid, syscall.Signal(0)), syscall.ESRCH) { + return true + } + state, err := exec.Command("ps", "-o", "stat=", "-p", strconv.Itoa(pid)).Output() + if err != nil { + return errors.Is(syscall.Kill(pid, syscall.Signal(0)), syscall.ESRCH) + } + return strings.HasPrefix(strings.TrimSpace(string(state)), "Z") +} diff --git a/internal/background/process_windows.go b/internal/background/process_windows.go index c248bc853..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). @@ -16,3 +25,28 @@ func ConfigureChildProcessGroup(cmd *exec.Cmd) { execution.ConfigureProcessGroup func terminateProcess(pid int) error { return execution.TerminateProcessTree(pid, 0, 0) } + +// terminateOwnedProcess asks taskkill /T to terminate the tree rooted at cmd, +// with KillProcessTree's direct Process.Kill fallback if taskkill fails. taskkill +// can discover descendants only while the root PID still exists; unlike a POSIX +// process group, a Windows tree has no independently addressable identity after +// its root exits. +func terminateOwnedProcess(cmd *exec.Cmd) (bool, error) { + var ( + alreadyExited bool + terminateErr error + ) + err := cmd.Process.WithHandle(func(handle uintptr) { + var exitCode uint32 + if windows.GetExitCodeProcess(windows.Handle(handle), &exitCode) == nil { + alreadyExited = exitCode != stillActiveExitCode + } + // Keep the exact process identity pinned while the PID-based taskkill + // operation runs, preventing the PID from being recycled underneath it. + terminateErr = terminateProcessForTest(cmd.Process.Pid) + }) + if err != nil { + return false, fmt.Errorf("pin process %d for termination: %w", cmd.Process.Pid, err) + } + return alreadyExited, terminateErr +} diff --git a/internal/background/process_windows_test.go b/internal/background/process_windows_test.go new file mode 100644 index 000000000..15ee0eb93 --- /dev/null +++ b/internal/background/process_windows_test.go @@ -0,0 +1,106 @@ +//go:build windows + +package background + +import ( + "bufio" + "errors" + "os/exec" + "strconv" + "strings" + "testing" + "time" +) + +func TestTerminateCommandReturnsTreeFailureAfterRootFallbackReaps(t *testing.T) { + cmd := exec.Command("powershell.exe", "-NoProfile", "-NonInteractive", "-Command", "Start-Sleep -Seconds 300") + if err := cmd.Start(); err != nil { + t.Fatalf("start: %v", err) + } + treeErr := errors.New("taskkill /T failed") + originalTerminateProcess := terminateProcessForTest + terminateProcessForTest = func(pid int) error { + if err := cmd.Process.Kill(); err != nil { + return errors.Join(treeErr, err) + } + return treeErr + } + t.Cleanup(func() { terminateProcessForTest = originalTerminateProcess }) + + err := TerminateCommand(cmd) + if !errors.Is(err, treeErr) { + t.Fatalf("TerminateCommand error = %v, want tree failure", err) + } + if cmd.ProcessState == nil { + t.Fatal("root process was not reaped") + } +} + +func TestTerminateCommandReapsExitedLeader(t *testing.T) { + cmd := exec.Command("cmd.exe", "/c", "exit", "0") + if err := cmd.Start(); err != nil { + t.Fatalf("start: %v", err) + } + // Let the child exit without calling Wait so cleanup exercises the Windows + // taskkill/TerminateProcess race while it still owns the process handle. + time.Sleep(500 * time.Millisecond) + + // jatmn's #774 finding: taskkill /T racing an already-exited PID (and the + // Process.Kill fallback) both fail here, but the reap below still succeeds — + // the process-handle exit-code check proves the leader was already gone, so + // TerminateCommand must not surface the expected kill-attempt error. + if err := TerminateCommand(cmd); err != nil { + t.Fatalf("TerminateCommand: %v, want nil (leader was already gone and got reaped)", err) + } + if cmd.ProcessState == nil { + t.Fatal("exited process was not reaped") + } +} + +func TestTerminateCommandDeadLeaderCannotIdentifyDescendantTree(t *testing.T) { + cmd := exec.Command("powershell.exe", "-NoProfile", "-NonInteractive", "-Command", + `$p = Start-Process powershell.exe -ArgumentList '-NoProfile','-Command','Start-Sleep -Seconds 300' -PassThru; $p.Id`) + stdout, err := cmd.StdoutPipe() + if err != nil { + t.Fatalf("stdout pipe: %v", err) + } + if err := cmd.Start(); err != nil { + t.Fatalf("start: %v", err) + } + line, err := bufio.NewReader(stdout).ReadString('\n') + if err != nil { + t.Fatalf("read descendant pid: %v", err) + } + descendantPID, err := strconv.Atoi(strings.TrimSpace(line)) + if err != nil { + t.Fatalf("parse descendant pid %q: %v", line, err) + } + t.Cleanup(func() { + _ = exec.Command("taskkill.exe", "/F", "/PID", strconv.Itoa(descendantPID)).Run() + }) + + // Wait until the leader is observably exited but deliberately leave it + // unreaped, matching the POSIX regression's ordering. + deadline := time.Now().Add(5 * time.Second) + for processExistsOnWindows(cmd.Process.Pid) { + if time.Now().After(deadline) { + t.Fatal("leader did not exit") + } + time.Sleep(20 * time.Millisecond) + } + if err := TerminateCommand(cmd); err != nil { + t.Fatalf("TerminateCommand: %v", err) + } + + // Windows has no persistent group ID analogous to POSIX: once the leader is + // gone, taskkill /T cannot rediscover this descendant. Assert that platform + // distinction rather than claiming TerminateCommand can provide it here. + if !processExistsOnWindows(descendantPID) { + t.Fatal("descendant unexpectedly stopped after its root exited") + } +} + +func processExistsOnWindows(pid int) bool { + return exec.Command("powershell.exe", "-NoProfile", "-NonInteractive", "-Command", + "Get-Process -Id "+strconv.Itoa(pid)+" -ErrorAction Stop | Out-Null").Run() == nil +} diff --git a/internal/background/terminate.go b/internal/background/terminate.go index bc9ce7dfc..49945e5db 100644 --- a/internal/background/terminate.go +++ b/internal/background/terminate.go @@ -1,5 +1,14 @@ package background +import ( + "errors" + "fmt" + "os/exec" + "time" +) + +const commandReapTimeout = 3 * time.Second + // TerminateProcess stops a background process by PID — on Windows its process // tree; on POSIX its whole process group when the PID leads its own group (the // invariant ConfigureChildProcessGroup establishes for processes started through @@ -12,3 +21,69 @@ package background func TerminateProcess(pid int) error { return terminateProcess(pid) } + +// TerminateCommand stops a started command and reaps its leader. The caller must +// have exclusive ownership of cmd: it must not have previously called Wait or +// Process.Release, and no goroutine may call either concurrently. On POSIX it +// stops the whole group when the command was configured as its leader; ordinary +// commands safely fall back to PID/tree discovery. On Windows, success for a +// leader that was already dead confirms only that the leader was reaped; +// descendants may survive because Windows cannot rediscover a tree from a dead +// root. `zero daemon start` needs this operation when readiness times out: it +// launched the child, so it must both stop the tree and collect the leader. +// +// The order matters: the tree is signalled first, because Wait releases the +// leader's PID and a later group lookup could then resolve to nothing (or, worse, +// to a recycled PID). Termination stays in the shared execution lifecycle +// primitives rather than introducing a second platform-specific killer. +// +// If the bounded reap times out, the Wait call already started by this function +// continues in its goroutine. From that point onward the caller must not access +// cmd at all, including ProcessState, because that would race with Wait. +func TerminateCommand(cmd *exec.Cmd) error { + if cmd == nil || cmd.Process == nil { + return errors.New("terminate command: process was never started") + } + leaderAlreadyExited, terminateErr := terminateOwnedProcess(cmd) + reapErr := waitForTerminatedCommandWithin(cmd, commandReapTimeout) + if reapErr != nil { + if terminateErr != nil { + return fmt.Errorf("%v (reap failed: %w)", terminateErr, reapErr) + } + return reapErr + } + if terminateErr != nil && !leaderAlreadyExited { + return terminateErr + } + // Only discard a termination error when the platform demonstrated before + // attempting termination that the leader had already exited. A successful + // reap alone says nothing about whether a live descendant tree was stopped. + return nil +} + +// classifyWaitError treats a non-zero exit status as success: a process being +// terminated is expected to report one (or a signal), so the only interesting +// failure is Wait itself not working. +func classifyWaitError(waitErr error) error { + var exitErr *exec.ExitError + if waitErr != nil && !errors.As(waitErr, &exitErr) { + return fmt.Errorf("reap process: %w", waitErr) + } + return nil +} + +// waitForTerminatedCommandWithin bounds the reap so a child that somehow survives +// termination cannot hang the caller — the daemon-start cleanup path runs while a +// user waits at the CLI. On timeout its Wait goroutine is intentionally left to +// finish: exec.Cmd permits only one Wait call, and abandoning it without a reaper +// would leak the child once it eventually exits. +func waitForTerminatedCommandWithin(cmd *exec.Cmd, timeout time.Duration) error { + waitDone := make(chan error, 1) + go func() { waitDone <- cmd.Wait() }() + select { + case waitErr := <-waitDone: + return classifyWaitError(waitErr) + case <-time.After(timeout): + return fmt.Errorf("process %d did not reap after termination", cmd.Process.Pid) + } +} diff --git a/internal/cli/daemon.go b/internal/cli/daemon.go index 5074312e4..2dfebd0ba 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,56 @@ func runDaemonStartDetached(paths daemon.Paths, stdout io.Writer, stderr io.Writ cmd.Stdout = logFile cmd.Stderr = logFile background.ConfigureChildProcessGroup(cmd) // own process group: outlives this shell - if err := cmd.Start(); err != nil { + if err := startAndAwaitDaemonProcess(cmd, func() bool { return daemonReachable(paths) }, 5*time.Second, 25*time.Millisecond); err != nil { + if errors.Is(err, errDaemonStartTimeout) { + return writeAppError(stderr, err.Error()+"; see "+logPath, exitCrash) + } return writeAppError(stderr, err.Error(), exitCrash) } - _ = cmd.Process.Release() + fmt.Fprintf(stdout, "zero daemon started (socket %s)\n", paths.Socket) + return exitSuccess +} + +var errDaemonStartTimeout = errors.New("daemon did not come up within timeout") - deadline := time.Now().Add(5 * time.Second) - for time.Now().Before(deadline) { - if daemonReachable(paths) { - fmt.Fprintf(stdout, "zero daemon started (socket %s)\n", paths.Socket) - return exitSuccess +func startAndAwaitDaemonProcess(cmd *exec.Cmd, reachable func() bool, timeout time.Duration, pollInterval time.Duration) error { + if err := cmd.Start(); err != nil { + return err + } + if waitForDaemonReadiness(reachable, timeout, pollInterval) { + if err := cmd.Process.Release(); err != nil { + cleanupErr := terminateAndReapDaemonProcess(cmd) + if cleanupErr != nil { + return fmt.Errorf("release daemon process: %w (cleanup failed: %v)", err, cleanupErr) + } + return fmt.Errorf("release daemon process: %w", err) } - time.Sleep(25 * time.Millisecond) + return nil + } + if err := terminateAndReapDaemonProcess(cmd); err != nil { + return fmt.Errorf("%w; cleanup failed: %v", errDaemonStartTimeout, err) } - return writeAppError(stderr, "daemon did not come up within timeout; see "+logPath, exitCrash) + return errDaemonStartTimeout +} + +func waitForDaemonReadiness(reachable func() bool, timeout time.Duration, pollInterval time.Duration) bool { + deadline := time.Now().Add(timeout) + for { + if reachable() { + return true + } + remaining := time.Until(deadline) + if remaining <= 0 { + break + } + time.Sleep(min(pollInterval, remaining)) + } + // Avoid killing a daemon that became reachable at the timeout boundary. + return reachable() +} + +func terminateAndReapDaemonProcess(cmd *exec.Cmd) error { + return background.TerminateCommand(cmd) } func runDaemonStop(args []string, stdout io.Writer, stderr io.Writer) int { diff --git a/internal/cli/daemon_test.go b/internal/cli/daemon_test.go index 5418537cc..3e316b8c1 100644 --- a/internal/cli/daemon_test.go +++ b/internal/cli/daemon_test.go @@ -2,8 +2,15 @@ package cli import ( "bytes" + "errors" + "os" + "os/exec" + "path/filepath" "strings" "testing" + "time" + + "github.com/Gitlawb/zero/internal/background" ) // isolateDaemonPaths points DefaultPaths at a temp dir so the test never touches @@ -121,3 +128,84 @@ func TestDaemonSubcommandsRejectExtraArgs(t *testing.T) { } } } + +func TestWaitForDaemonReadinessChecksTimeoutBoundary(t *testing.T) { + checks := 0 + ready := waitForDaemonReadiness(func() bool { + checks++ + return checks == 2 + }, 0, time.Millisecond) + if !ready { + t.Fatal("daemon that became reachable at the timeout boundary was not detected") + } + if checks != 2 { + t.Fatalf("reachability checks = %d, want initial and final checks", checks) + } +} + +func TestTerminateAndReapDaemonProcess(t *testing.T) { + cmd := daemonDetachedChildCommand("hang") + if err := cmd.Start(); err != nil { + t.Fatalf("start helper process: %v", err) + } + + if err := terminateAndReapDaemonProcess(cmd); err != nil { + t.Fatalf("terminate and reap helper process: %v", err) + } + if cmd.ProcessState == nil { + t.Fatalf("helper process was not reaped: state=%v", cmd.ProcessState) + } + if cmd.ProcessState.Success() { + t.Fatalf("helper process unexpectedly exited successfully: %v", cmd.ProcessState) + } +} + +func TestStartAndAwaitDaemonProcessTimeoutTerminatesAndReaps(t *testing.T) { + cmd := daemonDetachedChildCommand("hang") + if err := startAndAwaitDaemonProcess(cmd, func() bool { return false }, 0, time.Millisecond); !errors.Is(err, errDaemonStartTimeout) { + t.Fatalf("start and await error = %v, want timeout", err) + } + if cmd.ProcessState == nil { + t.Fatal("timed-out helper process was not reaped") + } +} + +func TestStartAndAwaitDaemonProcessReleasesReadyChild(t *testing.T) { + marker := filepath.Join(t.TempDir(), "child-finished") + cmd := daemonDetachedChildCommand("mark", "ZERO_TEST_DAEMON_CHILD_MARKER="+marker) + if err := startAndAwaitDaemonProcess(cmd, func() bool { return true }, time.Second, time.Millisecond); err != nil { + t.Fatalf("start and await ready helper: %v", err) + } + + deadline := time.Now().Add(5 * time.Second) + for time.Now().Before(deadline) { + if _, err := os.Stat(marker); err == nil { + return + } + time.Sleep(10 * time.Millisecond) + } + t.Fatal("released helper process did not remain alive to write its marker") +} + +func daemonDetachedChildCommand(mode string, extraEnv ...string) *exec.Cmd { + cmd := exec.Command(os.Args[0], "-test.run=TestDaemonDetachedChildProcess") + cmd.Env = append(os.Environ(), "ZERO_TEST_DAEMON_DETACHED_CHILD="+mode) + cmd.Env = append(cmd.Env, extraEnv...) + background.ConfigureChildProcessGroup(cmd) + return cmd +} + +func TestDaemonDetachedChildProcess(t *testing.T) { + switch os.Getenv("ZERO_TEST_DAEMON_DETACHED_CHILD") { + case "": + return + case "mark": + if err := os.WriteFile(os.Getenv("ZERO_TEST_DAEMON_CHILD_MARKER"), []byte("ready"), 0o600); err != nil { + os.Exit(2) + } + os.Exit(0) + } + for { + time.Sleep(time.Hour) + } +} diff --git a/internal/execution/process_unix.go b/internal/execution/process_unix.go index 27302afdf..58b2b0156 100644 --- a/internal/execution/process_unix.go +++ b/internal/execution/process_unix.go @@ -6,6 +6,8 @@ import ( "errors" "fmt" "os/exec" + "strconv" + "strings" "syscall" "time" ) @@ -38,12 +40,45 @@ 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 } - alive := func() bool { return syscall.Kill(target, syscall.Signal(0)) == nil } + 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 { + if pid <= 1 { + return fmt.Errorf("refusing to signal invalid pid %d", pid) + } + 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) { return nil @@ -77,11 +112,91 @@ 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 } +// signalTargetRunning reports whether the signal target still has a process that +// is actually RUNNING, as opposed to one waiting to be reaped. +// +// kill(2) with signal 0 succeeds for a zombie: the PID still exists until its +// parent collects it. That matters because the target here is usually a process +// GROUP, and a terminated leader's child is briefly a zombie before init/launchd +// reaps it. Treating that window as "still alive" made termination sit through +// both grace periods, SIGKILL an already-dead group, and then report +// "did not exit after SIGKILL" for a tree that had in fact stopped — turning a +// successful cleanup into a spurious failure (and a flaky one, since it depends on +// reap timing). +// +// Zombie detection goes through ps, which is available on every Unix we target and +// avoids a /proc dependency Darwin does not have. It is only consulted when the +// cheap kill check says something is still there, so the common path costs nothing +// extra. If ps cannot be run or its output cannot be parsed, the conservative +// pre-existing answer (the kill result) stands. +func signalTargetRunning(target int) bool { + if syscall.Kill(target, syscall.Signal(0)) != nil { + return false + } + // 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) + } + if !ok { + return true + } + return running +} + +// 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 + } + found := false + for line := range strings.SplitSeq(string(output), "\n") { + fields := strings.Fields(line) + if len(fields) < 3 { + continue + } + rowPid, errPid := strconv.Atoi(fields[0]) + rowPgid, errPgid := strconv.Atoi(fields[1]) + if errPid != nil || errPgid != nil || !match(rowPid, rowPgid) { + continue + } + found = true + if !strings.HasPrefix(fields[2], "Z") { + return true, true + } + } + return false, found +} + func processSignalTarget(pid int) (int, error) { if pid <= 1 { return 0, fmt.Errorf("refusing to signal invalid pid %d", pid) @@ -91,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 } diff --git a/internal/execution/process_unix_test.go b/internal/execution/process_unix_test.go new file mode 100644 index 000000000..36ba99189 --- /dev/null +++ b/internal/execution/process_unix_test.go @@ -0,0 +1,133 @@ +//go:build !windows + +package execution + +import ( + "os/exec" + "strconv" + "syscall" + "testing" + "time" +) + +// TestTerminateProcessTreeTreatsZombieGroupAsExited is the regression test for +// counting unreaped processes as alive: kill(2) with signal 0 succeeds for a +// zombie, so a group whose only remaining member is waiting to be reaped used to +// sit through both grace periods, take a pointless SIGKILL, and then be reported +// as "did not exit after SIGKILL" — a spurious cleanup failure whose timing +// depended on when the reaper ran. +func TestTerminateProcessTreeTreatsZombieGroupAsExited(t *testing.T) { + cmd := exec.Command("sh", "-c", "exit 0") + ConfigureProcessGroup(cmd) + if err := cmd.Start(); err != nil { + t.Fatalf("start: %v", err) + } + pid := cmd.Process.Pid + // Deliberately do NOT Wait: the child stays a zombie in its own group, which + // is exactly the state a terminated tree passes through. + 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) + } + if syscall.Kill(-pid, syscall.Signal(0)) != nil { + t.Skip("the zombie group is no longer signalable; cannot exercise the case") + } + + if signalTargetRunning(-pid) { + t.Fatal("a group holding only a zombie must not count as running") + } + if err := TerminateProcessTree(pid, 50*time.Millisecond, 10*time.Millisecond); err != nil { + t.Fatalf("TerminateProcessTree on an already-exited tree: %v", err) + } +} + +// TestTerminateProcessTreeStopsRunningGroup is the positive control: a group with +// a live member must still be seen as running and then actually stopped, so the +// zombie tolerance above cannot degrade into ignoring real processes. +func TestTerminateProcessTreeStopsRunningGroup(t *testing.T) { + cmd := exec.Command("sh", "-c", "sleep 30") + ConfigureProcessGroup(cmd) + if err := cmd.Start(); err != nil { + t.Fatalf("start: %v", err) + } + pid := cmd.Process.Pid + defer func() { _ = cmd.Wait() }() + + if !signalTargetRunning(-pid) { + t.Fatal("a group with a live member must count as running") + } + if err := TerminateProcessTree(pid, time.Second, 10*time.Millisecond); err != nil { + t.Fatalf("TerminateProcessTree: %v", err) + } + if running, ok := processGroupHasRunningMember(pid); ok && running { + t.Fatal("the group still has a running member after termination") + } +} + +// 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) { + output, err := exec.Command("ps", "-o", "stat=", "-p", strconv.Itoa(pid)).Output() + if err != nil { + return false, false + } + state := string(output) + for len(state) > 0 && (state[0] == ' ' || state[0] == '\n' || state[0] == '\t') { + state = state[1:] + } + if state == "" { + return false, false + } + return state[0] == 'Z', true +} 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) + } +}