diff --git a/internal/background/process_posix.go b/internal/background/process_posix.go index af38a922d..63620f1e6 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 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 49945e5db..f88fd4d93 100644 --- a/internal/background/terminate.go +++ b/internal/background/terminate.go @@ -22,6 +22,22 @@ func TerminateProcess(pid int) error { return terminateProcess(pid) } +// 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") + } + _, 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..3d3df0803 --- /dev/null +++ b/internal/background/terminate_posix_test.go @@ -0,0 +1,181 @@ +//go:build !windows + +package background + +import ( + "bufio" + "io" + "os" + "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) + } + finalized := false + var childPID int + t.Cleanup(func() { + if finalized { + return + } + 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) + + 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) + } + finalized = true +} + +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") +} + +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.go b/internal/daemon/launcher.go index ab2f8bc3b..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,12 +84,17 @@ 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 } - // 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 +174,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..c77638f30 --- /dev/null +++ b/internal/daemon/launcher_posix_test.go @@ -0,0 +1,279 @@ +//go:build !windows + +package daemon + +import ( + "context" + "errors" + "os" + "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) + } + finalized := false + // Phase 1: Arm generic handle cleanup immediately upon launch. + t.Cleanup(func() { + if finalized { + return + } + _ = 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 finalized { + return + } + if childPID > 0 { + _ = syscall.Kill(childPID, syscall.SIGKILL) + } + }) + + 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() { + if finalized { + return + } + _ = 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") + } + + // 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) + } + 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) { + // CommandContext Cancel must use the same launch-time group identity as + // 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 $!; exit 0"}, + Env: []string{}, + }) + if err != nil { + t.Fatalf("NewExecLauncher: %v", err) + } + h, err := launcher(ctx, WorkerSpec{}) + 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() + }) + + // 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) + } + }) + + 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() { + if finalized { + return + } + _ = 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") + } + + // 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) + } + if w.cmd.ProcessState == nil { + 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 { + t.Helper() + type res struct { + pid int + err error + } + ch := make(chan res, 1) + go func() { + line, ok, err := h.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 + } +} + +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") +} + +// 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") +}