From 77e439a98b47176fb3f17a812ea3f04c3aff44ed Mon Sep 17 00:00:00 2001 From: cairn-intern Date: Thu, 27 Aug 2026 19:11:33 +0000 Subject: [PATCH 1/6] fix(daemon): kill launcher workers via launch-time process group execWorker.Kill and CommandContext Cancel still called TerminateProcess(pid), which rediscovers the group with Getpgid. On Darwin that lookup returns ESRCH for an unreaped group leader and leaves descendants running. Route both sites through TerminateOwnedProcess so they use the ConfigureChildProcessGroup identity instead. TerminateCommand is the wrong helper: it Wait()s, and the pool still owns the reap. Fixes #861 --- internal/background/process_posix.go | 5 +- internal/background/terminate.go | 15 +++ internal/background/terminate_posix_test.go | 90 +++++++++++++ internal/daemon/launcher.go | 15 ++- internal/daemon/launcher_posix_test.go | 139 ++++++++++++++++++++ 5 files changed, 256 insertions(+), 8 deletions(-) create mode 100644 internal/background/terminate_posix_test.go create mode 100644 internal/daemon/launcher_posix_test.go diff --git a/internal/background/process_posix.go b/internal/background/process_posix.go index af38a922d..1ebb405c5 100644 --- a/internal/background/process_posix.go +++ b/internal/background/process_posix.go @@ -49,8 +49,9 @@ func terminateProcess(pid int) error { // 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. +// because both TerminateOwnedProcess and TerminateCommand require +// ConfigureChildProcessGroup's Setpgid convention; worth covering explicitly +// if Setsid-configured commands start using this path too. 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) diff --git a/internal/background/terminate.go b/internal/background/terminate.go index 49945e5db..27c6414e2 100644 --- a/internal/background/terminate.go +++ b/internal/background/terminate.go @@ -22,6 +22,21 @@ func TerminateProcess(pid int) error { return terminateProcess(pid) } +// TerminateOwnedProcess stops a started command using the launch-time process +// group identity established by ConfigureChildProcessGroup, without reaping. +// Unlike TerminateProcess, this does not rediscover the group via Getpgid, so +// Darwin ESRCH on an unreaped group leader cannot leave descendants running +// (see execution.TerminateProcessGroup). Unlike TerminateCommand, this does +// not Wait: callers such as execWorker.Kill and CommandContext Cancel still +// own the subsequent reap. +func TerminateOwnedProcess(cmd *exec.Cmd) error { + if cmd == nil || cmd.Process == nil { + return errors.New("terminate owned process: process was never started") + } + _, err := terminateOwnedProcess(cmd) + return err +} + // 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 diff --git a/internal/background/terminate_posix_test.go b/internal/background/terminate_posix_test.go new file mode 100644 index 000000000..aea0189fb --- /dev/null +++ b/internal/background/terminate_posix_test.go @@ -0,0 +1,90 @@ +//go:build !windows + +package background + +import ( + "bufio" + "os/exec" + "strconv" + "strings" + "syscall" + "testing" + "time" +) + +func TestTerminateOwnedProcessDoesNotReap(t *testing.T) { + cmd := exec.Command("sleep", "30") + ConfigureChildProcessGroup(cmd) + if err := cmd.Start(); err != nil { + t.Fatalf("start: %v", err) + } + t.Cleanup(func() { + if cmd.ProcessState == nil { + _ = cmd.Process.Kill() + _ = cmd.Wait() + } + }) + + if err := TerminateOwnedProcess(cmd); err != nil { + t.Fatalf("TerminateOwnedProcess: %v", err) + } + if cmd.ProcessState != nil { + t.Fatal("TerminateOwnedProcess must not Wait; the caller still owns the reap") + } + if err := cmd.Wait(); err == nil { + t.Fatal("expected the terminated command's Wait to report a signal") + } +} + +func TestTerminateOwnedProcessKillsChildAfterLeaderExits(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 and is left + // unreaped. TerminateOwnedProcess must signal the launch-time process + // group rather than rediscovering it via Getpgid (Darwin ESRCH, #861). + 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() + } + }) + + if err := TerminateOwnedProcess(cmd); err != nil { + t.Fatalf("TerminateOwnedProcess: %v", err) + } + if cmd.ProcessState != nil { + t.Fatal("TerminateOwnedProcess must not reap the leader") + } + if err := cmd.Wait(); err != nil && terminatingSignal(err) == 0 { + // Leader may have exited 0 before the group signal, or have been + // signalled; either is a successful reap. + t.Fatalf("Wait after TerminateOwnedProcess: %v", err) + } + + deadline := time.Now().Add(2 * time.Second) + for !processStopped(childPID) { + if time.Now().After(deadline) { + t.Fatalf("forked child %d survived TerminateOwnedProcess — group kill failed", childPID) + } + time.Sleep(20 * time.Millisecond) + } +} diff --git a/internal/daemon/launcher.go b/internal/daemon/launcher.go index ab2f8bc3b..1071ca701 100644 --- a/internal/daemon/launcher.go +++ b/internal/daemon/launcher.go @@ -72,9 +72,12 @@ func (w *execWorker) Kill() error { if w.cmd.Process == nil { return nil } - // background.TerminateProcess is the cross-platform terminate (kills the - // process group on POSIX, taskkill /T on Windows). - return background.TerminateProcess(w.cmd.Process.Pid) + // TerminateOwnedProcess uses the launch-time group identity from + // ConfigureChildProcessGroup rather than TerminateProcess's Getpgid + // rediscovery. On Darwin, Getpgid of an unreaped group leader can return + // ESRCH and leave descendants running (#861, #774). TerminateCommand is + // the wrong helper here: it Wait()s, and the pool still owns the reap. + return background.TerminateOwnedProcess(w.cmd) } // readerLines adapts a bufio.Reader to the Lines interface. Unlike a capped @@ -154,13 +157,13 @@ func NewExecLauncher(cfg ExecLauncherConfig) (Launcher, error) { background.ConfigureChildProcessGroup(cmd) // CommandContext's default cancel sends os.Process.Kill to the LEADER only, // orphaning the process group we just configured (a stuck worker's children - // would survive ctx cancellation). Terminate the whole group instead — the - // same cross-platform group terminate Kill() uses (D11). + // would survive ctx cancellation). Terminate the whole group instead via + // the launch-time identity — the same primitive Kill() uses (D11, #861). cmd.Cancel = func() error { if cmd.Process == nil { return nil } - return background.TerminateProcess(cmd.Process.Pid) + return background.TerminateOwnedProcess(cmd) } stdout, err := cmd.StdoutPipe() diff --git a/internal/daemon/launcher_posix_test.go b/internal/daemon/launcher_posix_test.go new file mode 100644 index 000000000..c3db962ba --- /dev/null +++ b/internal/daemon/launcher_posix_test.go @@ -0,0 +1,139 @@ +//go:build !windows + +package daemon + +import ( + "context" + "errors" + "os/exec" + "strconv" + "strings" + "syscall" + "testing" + "time" +) + +func TestExecWorkerKillTerminatesProcessGroup(t *testing.T) { + // The leader forks a child then exits and is left unreaped. Kill must + // signal the launch-time process group (ConfigureChildProcessGroup) rather + // than TerminateProcess's Getpgid rediscovery, so the child dies even + // while Darwin Getpgid would return ESRCH on the zombie leader (#861). + launcher, err := NewExecLauncher(ExecLauncherConfig{ + Executable: "/bin/sh", + BaseArgs: []string{"-c", "sleep 300 & echo $!; exit 0"}, + Env: []string{}, + }) + if err != nil { + t.Fatalf("NewExecLauncher: %v", err) + } + h, err := launcher(context.Background(), WorkerSpec{}) + if err != nil { + t.Fatalf("launch: %v", err) + } + w, ok := h.(*execWorker) + if !ok { + t.Fatalf("launcher returned %T, want *execWorker", h) + } + if w.cmd.SysProcAttr == nil || !w.cmd.SysProcAttr.Setpgid || w.cmd.SysProcAttr.Pgid != 0 { + t.Fatal("worker was not configured as its own process-group leader") + } + + childPID := readWorkerPIDLine(t, w) + t.Cleanup(func() { + _ = syscall.Kill(childPID, syscall.SIGKILL) + if w.cmd.ProcessState == nil { + _, _ = w.Wait() + } + }) + + if err := w.Kill(); err != nil { + t.Fatalf("Kill: %v", err) + } + if w.cmd.ProcessState != nil { + t.Fatal("Kill must not reap; Wait still owns the child") + } + if _, err := w.Wait(); err != nil { + t.Fatalf("Wait after Kill: %v", err) + } + assertProcessStopped(t, childPID) +} + +func TestExecLauncherCancelTerminatesProcessGroup(t *testing.T) { + // CommandContext Cancel must use the same launch-time group identity as + // Kill, not TerminateProcess(pid). The leader stays alive (wait) so the + // context-cancel goroutine is the path under test. + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + launcher, err := NewExecLauncher(ExecLauncherConfig{ + Executable: "/bin/sh", + BaseArgs: []string{"-c", "sleep 300 & echo $!; wait"}, + Env: []string{}, + }) + if err != nil { + t.Fatalf("NewExecLauncher: %v", err) + } + h, err := launcher(ctx, WorkerSpec{}) + if err != nil { + t.Fatalf("launch: %v", err) + } + w, ok := h.(*execWorker) + if !ok { + t.Fatalf("launcher returned %T, want *execWorker", h) + } + if w.cmd.SysProcAttr == nil || !w.cmd.SysProcAttr.Setpgid || w.cmd.SysProcAttr.Pgid != 0 { + t.Fatal("worker was not configured as its own process-group leader") + } + + childPID := readWorkerPIDLine(t, w) + t.Cleanup(func() { + _ = syscall.Kill(childPID, syscall.SIGKILL) + if w.cmd.ProcessState == nil { + _, _ = w.Wait() + } + }) + + cancel() + if _, err := w.Wait(); err != nil { + t.Fatalf("Wait after cancel: %v", err) + } + assertProcessStopped(t, childPID) +} + +func readWorkerPIDLine(t *testing.T, w *execWorker) int { + t.Helper() + line, ok, err := w.Stdout().Next() + if err != nil { + t.Fatalf("read worker stdout: %v", err) + } + if !ok { + t.Fatal("worker stdout ended before printing the forked child pid") + } + pid, err := strconv.Atoi(strings.TrimSpace(line)) + if err != nil { + t.Fatalf("parse forked child pid %q: %v", line, err) + } + return pid +} + +func assertProcessStopped(t *testing.T, pid int) { + t.Helper() + deadline := time.Now().Add(2 * time.Second) + for !launcherProcessStopped(pid) { + if time.Now().After(deadline) { + t.Fatalf("forked child %d survived process-group termination", pid) + } + time.Sleep(20 * time.Millisecond) + } +} + +func launcherProcessStopped(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 058fabda28c88f64a462a4b15e5a8deba93d5f30 Mon Sep 17 00:00:00 2001 From: cairn-intern Date: Fri, 28 Aug 2026 02:17:30 +0000 Subject: [PATCH 2/6] test: wait for unreaped zombie leader before group terminate Wait until cmd.Process.Pid is a zombie via /proc//stat (or ps) without Wait/reaping so the Darwin Getpgid ESRCH path is actually exercised. Cover TerminateOwnedProcess(nil) and an unstarted *exec.Cmd failure paths. --- internal/background/terminate_posix_test.go | 56 +++++++++++++++++++++ internal/daemon/launcher_posix_test.go | 33 ++++++++++++ 2 files changed, 89 insertions(+) diff --git a/internal/background/terminate_posix_test.go b/internal/background/terminate_posix_test.go index aea0189fb..adbe240bd 100644 --- a/internal/background/terminate_posix_test.go +++ b/internal/background/terminate_posix_test.go @@ -4,6 +4,7 @@ package background import ( "bufio" + "os" "os/exec" "strconv" "strings" @@ -68,6 +69,10 @@ func TestTerminateOwnedProcessKillsChildAfterLeaderExits(t *testing.T) { } }) + // echo $! races ahead of exit 0. Wait until the leader is an unreaped + // zombie without calling Wait, so this exercises Darwin Getpgid ESRCH. + waitUntilUnreapedZombie(t, cmd.Process.Pid) + if err := TerminateOwnedProcess(cmd); err != nil { t.Fatalf("TerminateOwnedProcess: %v", err) } @@ -88,3 +93,54 @@ func TestTerminateOwnedProcessKillsChildAfterLeaderExits(t *testing.T) { time.Sleep(20 * time.Millisecond) } } + +func TestTerminateOwnedProcessNil(t *testing.T) { + defer func() { + if r := recover(); r != nil { + t.Fatalf("TerminateOwnedProcess(nil) panicked: %v", r) + } + }() + if err := TerminateOwnedProcess(nil); err == nil { + t.Fatal("TerminateOwnedProcess(nil) must return an error") + } +} + +func TestTerminateOwnedProcessUnstarted(t *testing.T) { + defer func() { + if r := recover(); r != nil { + t.Fatalf("TerminateOwnedProcess(unstarted) panicked: %v", r) + } + }() + cmd := exec.Command("true") + if err := TerminateOwnedProcess(cmd); err == nil { + t.Fatal("TerminateOwnedProcess on an unstarted command must return an error") + } +} + +// waitUntilUnreapedZombie polls until pid is a zombie without calling Wait. +func waitUntilUnreapedZombie(t *testing.T, pid int) { + t.Helper() + deadline := time.Now().Add(2 * time.Second) + for !isUnreapedZombie(pid) { + if time.Now().After(deadline) { + t.Fatalf("pid %d did not become an unreaped zombie before terminate", pid) + } + time.Sleep(20 * time.Millisecond) + } +} + +func isUnreapedZombie(pid int) bool { + if data, err := os.ReadFile("/proc/" + strconv.Itoa(pid) + "/stat"); err == nil { + s := string(data) + i := strings.LastIndexByte(s, ')') + if i < 0 || i+2 >= len(s) { + return false + } + return s[i+2] == 'Z' + } + state, err := exec.Command("ps", "-o", "stat=", "-p", strconv.Itoa(pid)).Output() + if err != nil { + return false + } + return strings.HasPrefix(strings.TrimSpace(string(state)), "Z") +} diff --git a/internal/daemon/launcher_posix_test.go b/internal/daemon/launcher_posix_test.go index c3db962ba..f3898cc1a 100644 --- a/internal/daemon/launcher_posix_test.go +++ b/internal/daemon/launcher_posix_test.go @@ -5,6 +5,7 @@ package daemon import ( "context" "errors" + "os" "os/exec" "strconv" "strings" @@ -46,6 +47,10 @@ func TestExecWorkerKillTerminatesProcessGroup(t *testing.T) { } }) + // echo $! races ahead of exit 0. Wait until the leader is an unreaped + // zombie without calling Wait, so Kill exercises Darwin Getpgid ESRCH. + waitUntilUnreapedZombie(t, w.cmd.Process.Pid) + if err := w.Kill(); err != nil { t.Fatalf("Kill: %v", err) } @@ -137,3 +142,31 @@ func launcherProcessStopped(pid int) bool { } return strings.HasPrefix(strings.TrimSpace(string(state)), "Z") } + +// waitUntilUnreapedZombie polls until pid is a zombie without calling Wait. +func waitUntilUnreapedZombie(t *testing.T, pid int) { + t.Helper() + deadline := time.Now().Add(2 * time.Second) + for !isUnreapedZombie(pid) { + if time.Now().After(deadline) { + t.Fatalf("pid %d did not become an unreaped zombie before Kill", pid) + } + time.Sleep(20 * time.Millisecond) + } +} + +func isUnreapedZombie(pid int) bool { + if data, err := os.ReadFile("/proc/" + strconv.Itoa(pid) + "/stat"); err == nil { + s := string(data) + i := strings.LastIndexByte(s, ')') + if i < 0 || i+2 >= len(s) { + return false + } + return s[i+2] == 'Z' + } + state, err := exec.Command("ps", "-o", "stat=", "-p", strconv.Itoa(pid)).Output() + if err != nil { + return false + } + return strings.HasPrefix(strings.TrimSpace(string(state)), "Z") +} From ce270fb4c489b00c963d00c5857bafd443f68942 Mon Sep 17 00:00:00 2001 From: cairn-intern Date: Tue, 1 Sep 2026 02:18:43 -0400 Subject: [PATCH 3/6] test(daemon): cover cancel after leader exit --- internal/background/process_posix.go | 6 +++--- internal/background/terminate.go | 15 ++++++++------- internal/daemon/launcher_posix_test.go | 13 +++++++++---- 3 files changed, 20 insertions(+), 14 deletions(-) diff --git a/internal/background/process_posix.go b/internal/background/process_posix.go index 1ebb405c5..63620f1e6 100644 --- a/internal/background/process_posix.go +++ b/internal/background/process_posix.go @@ -49,9 +49,9 @@ func terminateProcess(pid int) error { // 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 both TerminateOwnedProcess and TerminateCommand require -// ConfigureChildProcessGroup's Setpgid convention; worth covering explicitly -// if Setsid-configured commands start using this path too. +// because current group-owning callers use ConfigureChildProcessGroup's Setpgid +// convention; worth covering explicitly if Setsid-configured commands start +// using this path too. 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) diff --git a/internal/background/terminate.go b/internal/background/terminate.go index 27c6414e2..f88fd4d93 100644 --- a/internal/background/terminate.go +++ b/internal/background/terminate.go @@ -22,13 +22,14 @@ func TerminateProcess(pid int) error { return terminateProcess(pid) } -// TerminateOwnedProcess stops a started command using the launch-time process -// group identity established by ConfigureChildProcessGroup, without reaping. -// Unlike TerminateProcess, this does not rediscover the group via Getpgid, so -// Darwin ESRCH on an unreaped group leader cannot leave descendants running -// (see execution.TerminateProcessGroup). Unlike TerminateCommand, this does -// not Wait: callers such as execWorker.Kill and CommandContext Cancel still -// own the subsequent reap. +// TerminateOwnedProcess stops a started command without reaping it. On POSIX, +// commands prepared by ConfigureChildProcessGroup are stopped through their +// launch-time process-group identity, so Darwin ESRCH on an unreaped leader +// cannot leave descendants running (see execution.TerminateProcessGroup). +// Commands without that configuration use the platform's safe PID/process-tree +// fallback; Windows always uses its rooted process-tree implementation. Unlike +// TerminateCommand, this does not Wait: callers such as execWorker.Kill and +// CommandContext Cancel still own the subsequent reap. func TerminateOwnedProcess(cmd *exec.Cmd) error { if cmd == nil || cmd.Process == nil { return errors.New("terminate owned process: process was never started") diff --git a/internal/daemon/launcher_posix_test.go b/internal/daemon/launcher_posix_test.go index f3898cc1a..4e69ac30f 100644 --- a/internal/daemon/launcher_posix_test.go +++ b/internal/daemon/launcher_posix_test.go @@ -65,14 +65,15 @@ func TestExecWorkerKillTerminatesProcessGroup(t *testing.T) { func TestExecLauncherCancelTerminatesProcessGroup(t *testing.T) { // CommandContext Cancel must use the same launch-time group identity as - // Kill, not TerminateProcess(pid). The leader stays alive (wait) so the - // context-cancel goroutine is the path under test. + // Kill, not TerminateProcess(pid). The leader exits without being reaped, + // while its descendant keeps the stdout pipe open. That makes cancellation + // exercise the Darwin Getpgid -> ESRCH failure shape fixed by this change. ctx, cancel := context.WithCancel(context.Background()) defer cancel() launcher, err := NewExecLauncher(ExecLauncherConfig{ Executable: "/bin/sh", - BaseArgs: []string{"-c", "sleep 300 & echo $!; wait"}, + BaseArgs: []string{"-c", "sleep 300 & echo $!; exit 0"}, Env: []string{}, }) if err != nil { @@ -98,10 +99,14 @@ func TestExecLauncherCancelTerminatesProcessGroup(t *testing.T) { } }) + waitUntilUnreapedZombie(t, w.cmd.Process.Pid) cancel() - if _, err := w.Wait(); err != nil { + if _, err := w.Wait(); err != nil && !errors.Is(err, context.Canceled) { t.Fatalf("Wait after cancel: %v", err) } + if w.cmd.ProcessState == nil { + t.Fatal("Wait after cancel did not reap the exited worker leader") + } assertProcessStopped(t, childPID) } From d9ddcd863eb081956f32d0848c252b81269e3918 Mon Sep 17 00:00:00 2001 From: cairn-intern Date: Tue, 1 Sep 2026 16:10:28 -0400 Subject: [PATCH 4/6] test(daemon): arm immediate fixture cleanup and bounded pid reads --- internal/background/terminate_posix_test.go | 52 ++++++++++--- internal/daemon/launcher_posix_test.go | 81 ++++++++++++++------- 2 files changed, 97 insertions(+), 36 deletions(-) diff --git a/internal/background/terminate_posix_test.go b/internal/background/terminate_posix_test.go index adbe240bd..72c3755d9 100644 --- a/internal/background/terminate_posix_test.go +++ b/internal/background/terminate_posix_test.go @@ -4,6 +4,7 @@ package background import ( "bufio" + "io" "os" "os/exec" "strconv" @@ -54,21 +55,22 @@ func TestTerminateOwnedProcessKillsChildAfterLeaderExits(t *testing.T) { 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) - } + var childPID int t.Cleanup(func() { - _ = syscall.Kill(childPID, syscall.SIGKILL) - if cmd.ProcessState == nil { - _ = cmd.Wait() + if childPID > 0 { + _ = syscall.Kill(childPID, syscall.SIGKILL) + } + if cmd.Process != nil { + _ = syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) + _ = cmd.Process.Kill() + if cmd.ProcessState == nil { + _ = cmd.Wait() + } } }) + childPID = readPIDWithTimeout(t, stdout, 3*time.Second) + // echo $! races ahead of exit 0. Wait until the leader is an unreaped // zombie without calling Wait, so this exercises Darwin Getpgid ESRCH. waitUntilUnreapedZombie(t, cmd.Process.Pid) @@ -144,3 +146,31 @@ func isUnreapedZombie(pid int) bool { } return strings.HasPrefix(strings.TrimSpace(string(state)), "Z") } + +func readPIDWithTimeout(t *testing.T, r io.Reader, timeout time.Duration) int { + t.Helper() + type res struct { + pid int + err error + } + ch := make(chan res, 1) + go func() { + line, err := bufio.NewReader(r).ReadString('\n') + if err != nil { + ch <- res{err: err} + return + } + pid, err := strconv.Atoi(strings.TrimSpace(line)) + ch <- res{pid: pid, err: err} + }() + select { + case out := <-ch: + if out.err != nil { + t.Fatalf("read child PID: %v", out.err) + } + return out.pid + case <-time.After(timeout): + t.Fatalf("timed out waiting for child PID after %v", timeout) + return 0 + } +} diff --git a/internal/daemon/launcher_posix_test.go b/internal/daemon/launcher_posix_test.go index 4e69ac30f..d44952192 100644 --- a/internal/daemon/launcher_posix_test.go +++ b/internal/daemon/launcher_posix_test.go @@ -35,17 +35,24 @@ func TestExecWorkerKillTerminatesProcessGroup(t *testing.T) { if !ok { t.Fatalf("launcher returned %T, want *execWorker", h) } + var childPID int + t.Cleanup(func() { + if childPID > 0 { + _ = syscall.Kill(childPID, syscall.SIGKILL) + } + if w.cmd != nil && w.cmd.Process != nil { + _ = syscall.Kill(-w.cmd.Process.Pid, syscall.SIGKILL) + _ = w.cmd.Process.Kill() + if w.cmd.ProcessState == nil { + _, _ = w.Wait() + } + } + }) if w.cmd.SysProcAttr == nil || !w.cmd.SysProcAttr.Setpgid || w.cmd.SysProcAttr.Pgid != 0 { t.Fatal("worker was not configured as its own process-group leader") } - childPID := readWorkerPIDLine(t, w) - t.Cleanup(func() { - _ = syscall.Kill(childPID, syscall.SIGKILL) - if w.cmd.ProcessState == nil { - _, _ = w.Wait() - } - }) + childPID = readWorkerPIDLine(t, w) // echo $! races ahead of exit 0. Wait until the leader is an unreaped // zombie without calling Wait, so Kill exercises Darwin Getpgid ESRCH. @@ -87,17 +94,24 @@ func TestExecLauncherCancelTerminatesProcessGroup(t *testing.T) { if !ok { t.Fatalf("launcher returned %T, want *execWorker", h) } + var childPID int + t.Cleanup(func() { + if childPID > 0 { + _ = syscall.Kill(childPID, syscall.SIGKILL) + } + if w.cmd != nil && w.cmd.Process != nil { + _ = syscall.Kill(-w.cmd.Process.Pid, syscall.SIGKILL) + _ = w.cmd.Process.Kill() + if w.cmd.ProcessState == nil { + _, _ = w.Wait() + } + } + }) if w.cmd.SysProcAttr == nil || !w.cmd.SysProcAttr.Setpgid || w.cmd.SysProcAttr.Pgid != 0 { t.Fatal("worker was not configured as its own process-group leader") } - childPID := readWorkerPIDLine(t, w) - t.Cleanup(func() { - _ = syscall.Kill(childPID, syscall.SIGKILL) - if w.cmd.ProcessState == nil { - _, _ = w.Wait() - } - }) + childPID = readWorkerPIDLine(t, w) waitUntilUnreapedZombie(t, w.cmd.Process.Pid) cancel() @@ -112,18 +126,35 @@ func TestExecLauncherCancelTerminatesProcessGroup(t *testing.T) { func readWorkerPIDLine(t *testing.T, w *execWorker) int { t.Helper() - line, ok, err := w.Stdout().Next() - if err != nil { - t.Fatalf("read worker stdout: %v", err) - } - if !ok { - t.Fatal("worker stdout ended before printing the forked child pid") - } - pid, err := strconv.Atoi(strings.TrimSpace(line)) - if err != nil { - t.Fatalf("parse forked child pid %q: %v", line, err) + type res struct { + pid int + err error + } + ch := make(chan res, 1) + go func() { + line, ok, err := w.Stdout().Next() + if err != nil { + ch <- res{err: err} + return + } + if !ok { + ch <- res{err: errors.New("worker stdout ended before printing the forked child pid")} + return + } + pid, err := strconv.Atoi(strings.TrimSpace(line)) + ch <- res{pid: pid, err: err} + }() + + select { + case out := <-ch: + if out.err != nil { + t.Fatalf("read worker stdout: %v", out.err) + } + return out.pid + case <-time.After(3 * time.Second): + t.Fatal("timed out waiting for worker child pid") + return 0 } - return pid } func assertProcessStopped(t *testing.T, pid int) { From c89e5710fe1694b0ca21099cb3d1b6bb94151074 Mon Sep 17 00:00:00 2001 From: cairn-intern Date: Tue, 1 Sep 2026 18:16:30 -0400 Subject: [PATCH 5/6] test(daemon): arm generic and descendant cleanups before launcher assertions --- internal/daemon/launcher_posix_test.go | 87 ++++++++++++++++---------- 1 file changed, 55 insertions(+), 32 deletions(-) diff --git a/internal/daemon/launcher_posix_test.go b/internal/daemon/launcher_posix_test.go index d44952192..1ec2712fa 100644 --- a/internal/daemon/launcher_posix_test.go +++ b/internal/daemon/launcher_posix_test.go @@ -31,39 +31,49 @@ func TestExecWorkerKillTerminatesProcessGroup(t *testing.T) { if err != nil { t.Fatalf("launch: %v", err) } - w, ok := h.(*execWorker) - if !ok { - t.Fatalf("launcher returned %T, want *execWorker", h) - } - var childPID int + // Phase 1: Arm generic handle cleanup immediately upon launch. + t.Cleanup(func() { + _ = h.Kill() + _, _ = h.Wait() + }) + + // Phase 2: Obtain descendant PID and arm direct descendant fallback before leader exits. + childPID := readWorkerPIDLine(t, h) t.Cleanup(func() { if childPID > 0 { _ = syscall.Kill(childPID, syscall.SIGKILL) } - if w.cmd != nil && w.cmd.Process != nil { - _ = syscall.Kill(-w.cmd.Process.Pid, syscall.SIGKILL) - _ = w.cmd.Process.Kill() - if w.cmd.ProcessState == nil { - _, _ = w.Wait() - } - } }) + + w, ok := h.(*execWorker) + if !ok { + t.Fatalf("launcher returned %T, want *execWorker", h) + } + if w.cmd != nil && w.cmd.Process != nil { + leaderPID := w.cmd.Process.Pid + t.Cleanup(func() { + _ = syscall.Kill(-leaderPID, syscall.SIGKILL) + _ = syscall.Kill(leaderPID, syscall.SIGKILL) + }) + } + + // Phase 3: Setup validation — only now assert process group configuration. if w.cmd.SysProcAttr == nil || !w.cmd.SysProcAttr.Setpgid || w.cmd.SysProcAttr.Pgid != 0 { t.Fatal("worker was not configured as its own process-group leader") } - childPID = readWorkerPIDLine(t, w) - - // echo $! races ahead of exit 0. Wait until the leader is an unreaped - // zombie without calling Wait, so Kill exercises Darwin Getpgid ESRCH. + // Phase 4: Wait until leader is unreaped zombie. waitUntilUnreapedZombie(t, w.cmd.Process.Pid) + // Phase 5: Production action. if err := w.Kill(); err != nil { t.Fatalf("Kill: %v", err) } if w.cmd.ProcessState != nil { t.Fatal("Kill must not reap; Wait still owns the child") } + + // Phase 6: Finalization. if _, err := w.Wait(); err != nil { t.Fatalf("Wait after Kill: %v", err) } @@ -90,31 +100,44 @@ func TestExecLauncherCancelTerminatesProcessGroup(t *testing.T) { if err != nil { t.Fatalf("launch: %v", err) } - w, ok := h.(*execWorker) - if !ok { - t.Fatalf("launcher returned %T, want *execWorker", h) - } - var childPID int + // Phase 1: Arm generic handle cleanup immediately upon launch. + t.Cleanup(func() { + _ = h.Kill() + _, _ = h.Wait() + }) + + // Phase 2: Obtain descendant PID and arm direct descendant fallback. + childPID := readWorkerPIDLine(t, h) t.Cleanup(func() { if childPID > 0 { _ = syscall.Kill(childPID, syscall.SIGKILL) } - if w.cmd != nil && w.cmd.Process != nil { - _ = syscall.Kill(-w.cmd.Process.Pid, syscall.SIGKILL) - _ = w.cmd.Process.Kill() - if w.cmd.ProcessState == nil { - _, _ = w.Wait() - } - } }) + + w, ok := h.(*execWorker) + if !ok { + t.Fatalf("launcher returned %T, want *execWorker", h) + } + if w.cmd != nil && w.cmd.Process != nil { + leaderPID := w.cmd.Process.Pid + t.Cleanup(func() { + _ = syscall.Kill(-leaderPID, syscall.SIGKILL) + _ = syscall.Kill(leaderPID, syscall.SIGKILL) + }) + } + + // Phase 3: Setup validation. if w.cmd.SysProcAttr == nil || !w.cmd.SysProcAttr.Setpgid || w.cmd.SysProcAttr.Pgid != 0 { t.Fatal("worker was not configured as its own process-group leader") } - childPID = readWorkerPIDLine(t, w) - + // Phase 4: Wait until leader is unreaped zombie. waitUntilUnreapedZombie(t, w.cmd.Process.Pid) + + // Phase 5: Production action. cancel() + + // Phase 6: Finalization. if _, err := w.Wait(); err != nil && !errors.Is(err, context.Canceled) { t.Fatalf("Wait after cancel: %v", err) } @@ -124,7 +147,7 @@ func TestExecLauncherCancelTerminatesProcessGroup(t *testing.T) { assertProcessStopped(t, childPID) } -func readWorkerPIDLine(t *testing.T, w *execWorker) int { +func readWorkerPIDLine(t *testing.T, h WorkerHandle) int { t.Helper() type res struct { pid int @@ -132,7 +155,7 @@ func readWorkerPIDLine(t *testing.T, w *execWorker) int { } ch := make(chan res, 1) go func() { - line, ok, err := w.Stdout().Next() + line, ok, err := h.Stdout().Next() if err != nil { ch <- res{err: err} return From 19da35e024c27ac0c45de663027f62ef6be486af Mon Sep 17 00:00:00 2001 From: cairn-intern Date: Wed, 2 Sep 2026 04:47:47 -0400 Subject: [PATCH 6/6] fix(daemon): synchronize worker wait and kill to prevent signaling reaped PIDs --- internal/background/terminate_posix_test.go | 5 +++ internal/daemon/launcher.go | 19 +++++++- internal/daemon/launcher_posix_test.go | 48 +++++++++++++++++++++ 3 files changed, 71 insertions(+), 1 deletion(-) diff --git a/internal/background/terminate_posix_test.go b/internal/background/terminate_posix_test.go index 72c3755d9..3d3df0803 100644 --- a/internal/background/terminate_posix_test.go +++ b/internal/background/terminate_posix_test.go @@ -55,8 +55,12 @@ func TestTerminateOwnedProcessKillsChildAfterLeaderExits(t *testing.T) { if err := cmd.Start(); err != nil { t.Fatalf("start: %v", err) } + finalized := false var childPID int t.Cleanup(func() { + if finalized { + return + } if childPID > 0 { _ = syscall.Kill(childPID, syscall.SIGKILL) } @@ -94,6 +98,7 @@ func TestTerminateOwnedProcessKillsChildAfterLeaderExits(t *testing.T) { } time.Sleep(20 * time.Millisecond) } + finalized = true } func TestTerminateOwnedProcessNil(t *testing.T) { diff --git a/internal/daemon/launcher.go b/internal/daemon/launcher.go index 1071ca701..10e8ae407 100644 --- a/internal/daemon/launcher.go +++ b/internal/daemon/launcher.go @@ -9,6 +9,7 @@ import ( "os" "os/exec" "strings" + "sync" "github.com/Gitlawb/zero/internal/background" "github.com/Gitlawb/zero/internal/sandbox" @@ -47,6 +48,8 @@ func scrubWorkerEnv(env []string) []string { // execWorker is a WorkerHandle backed by a `zero exec` child process speaking // stream-json on stdout. type execWorker struct { + mu sync.Mutex + reaped bool cmd *exec.Cmd stdout io.ReadCloser lines Lines @@ -57,7 +60,19 @@ func (w *execWorker) Stdout() Lines { return w.lines } func (w *execWorker) Pid() int { return w.pid } func (w *execWorker) Wait() (int, error) { + w.mu.Lock() + if w.reaped { + w.mu.Unlock() + return 0, nil + } + w.mu.Unlock() + err := w.cmd.Wait() + + w.mu.Lock() + w.reaped = true + w.mu.Unlock() + if err == nil { return 0, nil } @@ -69,7 +84,9 @@ func (w *execWorker) Wait() (int, error) { } func (w *execWorker) Kill() error { - if w.cmd.Process == nil { + w.mu.Lock() + defer w.mu.Unlock() + if w.reaped || w.cmd.Process == nil { return nil } // TerminateOwnedProcess uses the launch-time group identity from diff --git a/internal/daemon/launcher_posix_test.go b/internal/daemon/launcher_posix_test.go index 1ec2712fa..c77638f30 100644 --- a/internal/daemon/launcher_posix_test.go +++ b/internal/daemon/launcher_posix_test.go @@ -31,8 +31,12 @@ func TestExecWorkerKillTerminatesProcessGroup(t *testing.T) { if err != nil { t.Fatalf("launch: %v", err) } + finalized := false // Phase 1: Arm generic handle cleanup immediately upon launch. t.Cleanup(func() { + if finalized { + return + } _ = h.Kill() _, _ = h.Wait() }) @@ -40,6 +44,9 @@ func TestExecWorkerKillTerminatesProcessGroup(t *testing.T) { // Phase 2: Obtain descendant PID and arm direct descendant fallback before leader exits. childPID := readWorkerPIDLine(t, h) t.Cleanup(func() { + if finalized { + return + } if childPID > 0 { _ = syscall.Kill(childPID, syscall.SIGKILL) } @@ -52,6 +59,9 @@ func TestExecWorkerKillTerminatesProcessGroup(t *testing.T) { if w.cmd != nil && w.cmd.Process != nil { leaderPID := w.cmd.Process.Pid t.Cleanup(func() { + if finalized { + return + } _ = syscall.Kill(-leaderPID, syscall.SIGKILL) _ = syscall.Kill(leaderPID, syscall.SIGKILL) }) @@ -78,6 +88,33 @@ func TestExecWorkerKillTerminatesProcessGroup(t *testing.T) { t.Fatalf("Wait after Kill: %v", err) } assertProcessStopped(t, childPID) + finalized = true +} + +func TestExecWorkerKillAfterWaitIsNoop(t *testing.T) { + launcher, err := NewExecLauncher(ExecLauncherConfig{ + Executable: "/bin/sh", + BaseArgs: []string{"-c", "echo hello; exit 0"}, + Env: []string{}, + }) + if err != nil { + t.Fatalf("NewExecLauncher: %v", err) + } + h, err := launcher(context.Background(), WorkerSpec{}) + if err != nil { + t.Fatalf("launch: %v", err) + } + w, ok := h.(*execWorker) + if !ok { + t.Fatalf("launcher returned %T, want *execWorker", h) + } + if _, err := w.Wait(); err != nil { + t.Fatalf("Wait: %v", err) + } + // Calling Kill after Wait must be an idempotent no-op and never signal + if err := w.Kill(); err != nil { + t.Fatalf("Kill after Wait returned error: %v", err) + } } func TestExecLauncherCancelTerminatesProcessGroup(t *testing.T) { @@ -100,8 +137,12 @@ func TestExecLauncherCancelTerminatesProcessGroup(t *testing.T) { if err != nil { t.Fatalf("launch: %v", err) } + finalized := false // Phase 1: Arm generic handle cleanup immediately upon launch. t.Cleanup(func() { + if finalized { + return + } _ = h.Kill() _, _ = h.Wait() }) @@ -109,6 +150,9 @@ func TestExecLauncherCancelTerminatesProcessGroup(t *testing.T) { // Phase 2: Obtain descendant PID and arm direct descendant fallback. childPID := readWorkerPIDLine(t, h) t.Cleanup(func() { + if finalized { + return + } if childPID > 0 { _ = syscall.Kill(childPID, syscall.SIGKILL) } @@ -121,6 +165,9 @@ func TestExecLauncherCancelTerminatesProcessGroup(t *testing.T) { if w.cmd != nil && w.cmd.Process != nil { leaderPID := w.cmd.Process.Pid t.Cleanup(func() { + if finalized { + return + } _ = syscall.Kill(-leaderPID, syscall.SIGKILL) _ = syscall.Kill(leaderPID, syscall.SIGKILL) }) @@ -145,6 +192,7 @@ func TestExecLauncherCancelTerminatesProcessGroup(t *testing.T) { t.Fatal("Wait after cancel did not reap the exited worker leader") } assertProcessStopped(t, childPID) + finalized = true } func readWorkerPIDLine(t *testing.T, h WorkerHandle) int {