From 34d423a0e3aa80c23d116540fc0934ecb7cc057b Mon Sep 17 00:00:00 2001 From: cairn-intern Date: Thu, 27 Aug 2026 19:13:26 +0000 Subject: [PATCH 1/2] fix(background): treat POSIX zombie leader as already-exited terminateOwnedProcess always returned leaderAlreadyExited=false, so TerminateCommand surfaced "did not exit after SIGKILL" when ps was unavailable and the leader was already a waitable zombie. Probe the leader the way Windows uses GetExitCodeProcess, then still signal the group so live descendants are not skipped. --- internal/background/process_posix.go | 15 +++- internal/background/process_posix_test.go | 45 ++++++++++++ .../background/process_waitable_darwin.go | 28 ++++++++ internal/background/process_waitable_linux.go | 68 +++++++++++++++++++ internal/background/process_waitable_other.go | 10 +++ internal/background/terminate.go | 11 +-- 6 files changed, 171 insertions(+), 6 deletions(-) create mode 100644 internal/background/process_waitable_darwin.go create mode 100644 internal/background/process_waitable_linux.go create mode 100644 internal/background/process_waitable_other.go diff --git a/internal/background/process_posix.go b/internal/background/process_posix.go index af38a922d..928bd2e92 100644 --- a/internal/background/process_posix.go +++ b/internal/background/process_posix.go @@ -45,6 +45,16 @@ 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 @@ -52,8 +62,9 @@ func terminateProcess(pid int) error { // 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 false, execution.TerminateProcessGroup(cmd.Process.Pid, terminationGracePeriod, terminationPollInterval) + return alreadyExited, execution.TerminateProcessGroup(cmd.Process.Pid, terminationGracePeriod, terminationPollInterval) } - return false, terminateProcess(cmd.Process.Pid) + return alreadyExited, terminateProcess(cmd.Process.Pid) } diff --git a/internal/background/process_posix_test.go b/internal/background/process_posix_test.go index b2080134c..fdb4d0c69 100644 --- a/internal/background/process_posix_test.go +++ b/internal/background/process_posix_test.go @@ -6,6 +6,7 @@ import ( "bufio" "errors" "os/exec" + "runtime" "strconv" "strings" "syscall" @@ -220,6 +221,50 @@ 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 processStopped(pid int) bool { if errors.Is(syscall.Kill(pid, syscall.Signal(0)), syscall.ESRCH) { return true diff --git a/internal/background/process_waitable_darwin.go b/internal/background/process_waitable_darwin.go new file mode 100644 index 000000000..5e1013dc9 --- /dev/null +++ b/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 +} diff --git a/internal/background/process_waitable_linux.go b/internal/background/process_waitable_linux.go new file mode 100644 index 000000000..790aa15cb --- /dev/null +++ b/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//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//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' +} diff --git a/internal/background/process_waitable_other.go b/internal/background/process_waitable_other.go new file mode 100644 index 000000000..77e5b2991 --- /dev/null +++ b/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 +} diff --git a/internal/background/terminate.go b/internal/background/terminate.go index 49945e5db..a51ea0067 100644 --- a/internal/background/terminate.go +++ b/internal/background/terminate.go @@ -26,10 +26,13 @@ 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 +// commands safely fall back to PID/tree discovery. If the platform observed that +// the leader had already exited before signalling (Windows: GetExitCodeProcess +// not STILL_ACTIVE; POSIX: the leader is a waitable zombie), a termination +// error is discarded after a successful reap — that is a statement about the +// leader, not descendants. POSIX still signals the launch-time process group. +// 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 From 12dfeacfa66da3af77ba718279597e289262ca92 Mon Sep 17 00:00:00 2001 From: cairn-intern Date: Tue, 1 Sep 2026 02:33:06 -0400 Subject: [PATCH 2/2] fix(background): preserve live-group termination failures --- internal/background/process_posix.go | 13 +++++ internal/background/process_posix_test.go | 59 +++++++++++++++++++++++ internal/background/process_windows.go | 5 ++ internal/background/terminate.go | 37 ++++++++------ 4 files changed, 100 insertions(+), 14 deletions(-) diff --git a/internal/background/process_posix.go b/internal/background/process_posix.go index 928bd2e92..40290eea9 100644 --- a/internal/background/process_posix.go +++ b/internal/background/process_posix.go @@ -3,7 +3,9 @@ package background import ( + "errors" "os/exec" + "syscall" "time" "github.com/Gitlawb/zero/internal/execution" @@ -68,3 +70,14 @@ func terminateOwnedProcess(cmd *exec.Cmd) (bool, error) { } 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 { + target = -target + } + return errors.Is(syscall.Kill(target, syscall.Signal(0)), syscall.ESRCH) +} diff --git a/internal/background/process_posix_test.go b/internal/background/process_posix_test.go index fdb4d0c69..f13e268dc 100644 --- a/internal/background/process_posix_test.go +++ b/internal/background/process_posix_test.go @@ -265,6 +265,65 @@ func TestTerminateCommandZombieLeaderWithoutPS(t *testing.T) { } } +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 diff --git a/internal/background/process_windows.go b/internal/background/process_windows.go index 1671f3000..3348414af 100644 --- a/internal/background/process_windows.go +++ b/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 } diff --git a/internal/background/terminate.go b/internal/background/terminate.go index a51ea0067..453b7d903 100644 --- a/internal/background/terminate.go +++ b/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,14 +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. If the platform observed that -// the leader had already exited before signalling (Windows: GetExitCodeProcess -// not STILL_ACTIVE; POSIX: the leader is a waitable zombie), a termination -// error is discarded after a successful reap — that is a statement about the -// leader, not descendants. POSIX still signals the launch-time process group. -// 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. +// 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, @@ -47,7 +52,7 @@ 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 { @@ -55,12 +60,16 @@ func TerminateCommand(cmd *exec.Cmd) error { } 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 }