diff --git a/internal/background/process_posix.go b/internal/background/process_posix.go index af38a922d..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" @@ -45,6 +47,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 +64,20 @@ 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 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) } diff --git a/internal/background/process_posix_test.go b/internal/background/process_posix_test.go index b2080134c..f13e268dc 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,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 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/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 49945e5db..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,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,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 { @@ -52,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 }