fix(background): treat POSIX zombie leader as already-exited

internal/background/process_posix.go

@@ -3,7 +3,9 @@
 package background
 
 import (
+	"errors"
 	"os/exec"
+	"syscall"
 	"time"
 
 	"github.com/Gitlawb/zero/internal/execution"
@@ -45,15 +47,37 @@ func terminateProcess(pid int) error {
 // 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.
 //
+// The bool result is whether the leader had already exited before termination
+// was attempted — the POSIX analogue of Windows GetExitCodeProcess returning a
+// value other than STILL_ACTIVE. A waitable zombie is already-exited: Wait can
+// collect it, but kill(0) still succeeds, and when ps is unavailable
+// signalTargetRunning conservatively treats that as "still running".
+// TerminateCommand uses the flag to discard the resulting spurious
+// SIGKILL-timeout once the reap has independently succeeded. The flag is about
+// the leader only; group/tree termination is still invoked so live descendants
+// are signalled.
+//
 // The Setpgid && Pgid == 0 guard only recognizes ConfigureChildProcessGroup's
 // own convention. A command made its own session leader via Setsid (as opposed
 // to Setpgid) is also its own process-group leader in practice, but takes the
 // slower rediscovery path here since Setsid isn't checked. Harmless today
 // because TerminateCommand has exactly one caller in this codebase; worth
 // covering explicitly if Setsid-configured commands start using this path too.
 func terminateOwnedProcess(cmd *exec.Cmd) (bool, error) {
+	alreadyExited := leaderWaitableExited(cmd.Process.Pid)
+	if cmd.SysProcAttr != nil && cmd.SysProcAttr.Setpgid && cmd.SysProcAttr.Pgid == 0 {
+		return alreadyExited, execution.TerminateProcessGroup(cmd.Process.Pid, terminationGracePeriod, terminationPollInterval)
+	}
+	return alreadyExited, terminateProcess(cmd.Process.Pid)
+}
+
+// terminationTargetGoneAfterReap independently checks the exact signal target
+// used for an owned command. In particular, reaping a zombie group leader does
+// not make this return true while any descendant still occupies that group.
+func terminationTargetGoneAfterReap(cmd *exec.Cmd) bool {
+	target := cmd.Process.Pid
 	if cmd.SysProcAttr != nil && cmd.SysProcAttr.Setpgid && cmd.SysProcAttr.Pgid == 0 {
-		return false, execution.TerminateProcessGroup(cmd.Process.Pid, terminationGracePeriod, terminationPollInterval)
+		target = -target
 	}
-	return false, terminateProcess(cmd.Process.Pid)
+	return errors.Is(syscall.Kill(target, syscall.Signal(0)), syscall.ESRCH)
 }

internal/background/process_posix_test.go

@@ -6,6 +6,7 @@ import (
 	"bufio"
 	"errors"
 	"os/exec"
+	"runtime"
 	"strconv"
 	"strings"
 	"syscall"
@@ -220,6 +221,109 @@ func TestTerminateCommandKillsChildAfterLeaderExits(t *testing.T) {
 	}
 }
 
+func TestTerminateCommandZombieLeaderWithoutPS(t *testing.T) {
+	if runtime.GOOS != "linux" && runtime.GOOS != "darwin" {
+		t.Skip("waitable-exited leader probe is implemented on linux and darwin")
+	}
+
+	grace, poll := terminationGracePeriod, terminationPollInterval
+	terminationGracePeriod, terminationPollInterval = 150*time.Millisecond, 10*time.Millisecond
+	t.Cleanup(func() { terminationGracePeriod, terminationPollInterval = grace, poll })
+
+	// Issue #862: a zombie group leader with ps unresolvable used to burn both
+	// grace periods (kill(0) succeeds against a zombie) and return
+	// "did not exit after SIGKILL" even though cmd.Wait reaped exit status 0.
+	// The already-exited probe must answer without ps so TerminateCommand can
+	// discard that spurious timeout after a successful reap.
+	cmd := exec.Command("sh", "-c", "exit 0")
+	ConfigureChildProcessGroup(cmd)
+	if err := cmd.Start(); err != nil {
+		t.Fatalf("start: %v", err)
+	}
+	pid := cmd.Process.Pid
+	t.Cleanup(func() {
+		if cmd.ProcessState == nil {
+			_ = cmd.Process.Kill()
+			_ = cmd.Wait()
+		}
+	})
+
+	deadline := time.Now().Add(5 * time.Second)
+	for !leaderWaitableExited(pid) {
+		if time.Now().After(deadline) {
+			t.Fatalf("leader %d did not become waitable-exited within 5s", pid)
+		}
+		time.Sleep(10 * time.Millisecond)
+	}
+
+	t.Setenv("PATH", "")
+	if err := TerminateCommand(cmd); err != nil {
+		t.Fatalf("TerminateCommand: %v, want nil (zombie leader with ps unavailable must not report SIGKILL timeout)", err)
+	}
+	if cmd.ProcessState == nil {
+		t.Fatal("zombie leader was not reaped")
+	}
+}
+
+func TestTerminateCommandPreservesGroupFailureWithExitedLeaderAndLiveChild(t *testing.T) {
+	if runtime.GOOS != "linux" && runtime.GOOS != "darwin" {
+		t.Skip("waitable-exited leader probe is implemented on linux and darwin")
+	}
+
+	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)
+	}
+	t.Cleanup(func() {
+		_ = syscall.Kill(childPID, syscall.SIGKILL)
+		if cmd.ProcessState == nil {
+			_ = cmd.Wait()
+		}
+	})
+
+	deadline := time.Now().Add(5 * time.Second)
+	for !leaderWaitableExited(cmd.Process.Pid) {
+		if time.Now().After(deadline) {
+			t.Fatalf("leader %d did not become waitable-exited within 5s", cmd.Process.Pid)
+		}
+		time.Sleep(10 * time.Millisecond)
+	}
+
+	treeErr := errors.New("process group could not be terminated")
+	originalTerminateOwnedProcess := terminateOwnedProcessForTest
+	terminateOwnedProcessForTest = func(got *exec.Cmd) (bool, error) {
+		if got != cmd {
+			t.Fatalf("terminate called with %p, want %p", got, cmd)
+		}
+		return true, treeErr
+	}
+	t.Cleanup(func() { terminateOwnedProcessForTest = originalTerminateOwnedProcess })
+
+	err = TerminateCommand(cmd)
+	if !errors.Is(err, treeErr) {
+		t.Fatalf("TerminateCommand error = %v, want live-group termination failure", err)
+	}
+	if cmd.ProcessState == nil {
+		t.Fatal("exited leader was not reaped")
+	}
+	if processStopped(childPID) {
+		t.Fatalf("child %d unexpectedly stopped; failure boundary needs a live group member", childPID)
+	}
+}
+
 func processStopped(pid int) bool {
 	if errors.Is(syscall.Kill(pid, syscall.Signal(0)), syscall.ESRCH) {
 		return true

internal/background/process_waitable_darwin.go

@@ -0,0 +1,28 @@
+//go:build darwin
+
+package background
+
+import "golang.org/x/sys/unix"
+
+// szomb is sys/proc.h SZOMB: the process has exited and is waiting to be reaped.
+const szomb int8 = 5
+
+// leaderWaitableExited reports whether pid has already exited and is waiting
+// to be reaped. This is the POSIX analogue of Windows GetExitCodeProcess
+// returning a value other than STILL_ACTIVE: the leader is a zombie, waitable
+// as exited, without collecting it. A true result does not mean the process
+// group is empty — descendants may still be running.
+//
+// Darwin has no /proc and x/sys/unix does not wrap waitid here, so the probe
+// is kern.proc.pid via sysctl. It does not use ps, which is the #862 failure
+// mode when PATH cannot resolve ps.
+func leaderWaitableExited(pid int) bool {
+	if pid <= 1 {
+		return false
+	}
+	kinfo, err := unix.SysctlKinfoProc("kern.proc.pid", pid)
+	if err != nil {
+		return false
+	}
+	return kinfo.Proc.P_stat == szomb
+}

internal/background/process_waitable_linux.go

@@ -0,0 +1,68 @@
+//go:build linux
+
+package background
+
+import (
+	"bytes"
+	"os"
+	"strconv"
+
+	"golang.org/x/sys/unix"
+)
+
+// POSIX waitid si_code values for an already-exited child (bits/waitflags.h).
+const (
+	cldExited int32 = 1
+	cldKilled int32 = 2
+	cldDumped int32 = 3
+)
+
+// leaderWaitableExited reports whether pid has already exited and is waiting
+// to be reaped. This is the POSIX analogue of Windows GetExitCodeProcess
+// returning a value other than STILL_ACTIVE: the leader is waitable as exited
+// (typically a zombie) without collecting it. A true result does not mean the
+// process group is empty — descendants may still be running.
+//
+// Detection does not use ps. waitid(WNOHANG|WNOWAIT) asks the kernel whether
+// an exit status is already available. /proc/<pid>/stat state Z is the
+// fallback when waitid cannot answer (for example if procfs is the only
+// usable source). Either path is independent of PATH, which is the #862
+// failure mode: empty PATH makes ps unresolvable and kill(0) still succeeds
+// against a zombie.
+func leaderWaitableExited(pid int) bool {
+	if pid <= 1 {
+		return false
+	}
+	if waitidAlreadyExited(pid) {
+		return true
+	}
+	return procIsZombie(pid)
+}
+
+func waitidAlreadyExited(pid int) bool {
+	var info unix.Siginfo
+	err := unix.Waitid(unix.P_PID, pid, &info, unix.WEXITED|unix.WNOHANG|unix.WNOWAIT, nil)
+	if err != nil {
+		return false
+	}
+	switch info.Code {
+	case cldExited, cldKilled, cldDumped:
+		return true
+	}
+	return false
+}
+
+func procIsZombie(pid int) bool {
+	data, err := os.ReadFile("/proc/" + strconv.Itoa(pid) + "/stat")
+	if err != nil {
+		return false
+	}
+	// /proc/<pid>/stat: pid (comm) state ... — comm may contain spaces or
+	// parentheses, so the state is the first field after the last ')'.
+	i := bytes.LastIndexByte(data, ')')
+	if i < 0 || i+1 >= len(data) {
+		return false
+	}
+	rest := bytes.TrimSpace(data[i+1:])
+	return len(rest) > 0 && rest[0] == 'Z'
+}

internal/background/process_waitable_other.go

@@ -0,0 +1,10 @@
+//go:build !windows && !linux && !darwin
+
+package background
+
+// leaderWaitableExited cannot positively identify a waitable-exited leader on
+// this platform without ps. Returning false keeps TerminateCommand's existing
+// conservative behaviour: a termination error is not discarded after reap.
+func leaderWaitableExited(pid int) bool {
+	return false
+}

internal/background/process_windows.go

@@ -50,3 +50,8 @@ func terminateOwnedProcess(cmd *exec.Cmd) (bool, error) {
 	}
 	return alreadyExited, terminateErr
 }
+
+// Windows has no persistent process-group identity to query after a dead root
+// is reaped. Preserve TerminateCommand's existing, documented dead-root
+// behavior; the stronger independent target check is available on POSIX.
+func terminationTargetGoneAfterReap(*exec.Cmd) bool { return true }

internal/background/terminate.go

@@ -9,6 +9,10 @@ import (
 
 const commandReapTimeout = 3 * time.Second
 
+// terminateOwnedProcessForTest is a seam for the failure-boundary tests. Tests
+// replace it only while no other test in this package is running in parallel.
+var terminateOwnedProcessForTest = terminateOwnedProcess
+
 // 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
@@ -26,11 +30,15 @@ func TerminateProcess(pid int) error {
 // 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.
+// commands safely fall back to PID/tree discovery. When POSIX observes a
+// waitable-zombie leader before signalling, a termination error is discarded
+// after reap only if the launch-time signal target is independently gone; the
+// leader observation alone says nothing about descendants. Windows retains its
+// existing behavior of ignoring the expected kill-attempt error when
+// GetExitCodeProcess shows that the root was already dead. Windows cannot
+// rediscover a tree from a dead root, so descendants may survive there. `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,
@@ -44,20 +52,24 @@ 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)
+	leaderAlreadyExited, terminateErr := terminateOwnedProcessForTest(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
+	if terminateErr != nil {
+		if !leaderAlreadyExited || !terminationTargetGoneAfterReap(cmd) {
+			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.
+	// On POSIX, discard a termination error only when the leader was already
+	// exited and the launch-time signal target is independently gone after reap.
+	// A successful leader reap alone says nothing about live descendants.
+	// Windows retains its existing dead-root semantics because it has no
+	// persistent group identity to query after the root exits.
 	return nil
 }