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

[cairn-intern]

Summary

Fixes #862.

terminateOwnedProcess on POSIX always returned leaderAlreadyExited=false, unlike Windows which probes GetExitCodeProcess before taskkill. TerminateCommand only discards a termination error after a successful reap when that flag is set, so a zombie group leader with ps unresolvable (PATH="") reported did not exit after SIGKILL even though cmd.Wait collected exit status 0. kill(0) succeeds against a zombie; without ps, signalTargetRunning conservatively treats that as still running.

POSIX now probes whether the leader is already waitable-as-exited before signalling, matching the Windows intent:

• Linux: waitid(P_PID, WEXITED|WNOHANG|WNOWAIT) plus /proc/<pid>/stat state Z
• Darwin: sysctl kern.proc.pid P_stat == SZOMB
• other POSIX: leave the flag false (today's conservative behaviour)

Group/tree termination still runs, so live descendants are signalled (TestTerminateCommandKillsChildAfterLeaderExits). The flag is about the leader only.

Regression: TestTerminateCommandZombieLeaderWithoutPS — zombie leader, empty PATH, TerminateCommand must return nil.

Notes

Opened by cairn-intern. Follow-up from review of #774. Issue #862 is not issue-approved; intern broadening per Vasanth/euxaristia.

gofmt applied. go test was not run locally (no full checkout). Worth go test ./internal/background/ -count=1 on Linux CI, including -run TestTerminateCommandZombieLeaderWithoutPS.

Summary by CodeRabbit

• Bug Fixes
  • Improved command termination handling for processes whose leaders exit before cleanup completes.
  • Prevented false termination timeout errors when reaping zombie process leaders.
  • Preserved genuine process-group termination failures when child processes remain active.
  • Improved consistency across supported operating systems.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Team

Run ID: 3f0f736c-f383-4380-9636-1f2aec20e0ce

📥 Commits

Reviewing files that changed from the base of the PR and between 1b5db17 and 12dfeac.

📒 Files selected for processing (7)

• internal/background/process_posix.go
• internal/background/process_posix_test.go
• internal/background/process_waitable_darwin.go
• internal/background/process_waitable_linux.go
• internal/background/process_waitable_other.go
• internal/background/process_windows.go
• internal/background/terminate.go

Included review availability: Your plan provides up to 4 included reviews per hour; 1 remains after this review.

---

Walkthrough

TerminateCommand now detects waitable-exited leaders without relying on ps, verifies the original termination target after reap, preserves genuine group failures, and retains platform-specific fallback behavior.

Changes

Process termination accuracy

Layer / File(s)	Summary
Platform-specific exit detection internal/background/process_waitable_linux.go, internal/background/process_waitable_darwin.go, internal/background/process_waitable_other.go	Linux checks waitid and /proc; Darwin checks process state through sysctl; other supported platforms return a conservative result.
Termination target verification internal/background/process_posix.go, internal/background/process_windows.go, internal/background/terminate.go	Termination records whether the leader already exited and suppresses errors only when the original signal target is also gone. Windows keeps its existing dead-root behavior.
POSIX termination regression tests internal/background/process_posix_test.go	Tests cover zombie leaders when ps is unavailable and group failures with an exited leader and live child.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Merge Risk: ⚪ Minimal · up to 12dfe

The change narrows POSIX termination handling to recognize already-exited zombie leaders while preserving descendant cleanup; no actionable merge-blocking risk remains beyond normal checks and review.

Sequence Diagram(s)

sequenceDiagram
  participant TerminateCommand
  participant terminateOwnedProcess
  participant leaderWaitableExited
  participant terminationTargetGoneAfterReap
  TerminateCommand->>terminateOwnedProcess: terminate command
  terminateOwnedProcess->>leaderWaitableExited: inspect leader state
  terminateOwnedProcess-->>TerminateCommand: return termination result and exit flag
  TerminateCommand->>terminationTargetGoneAfterReap: check original signal target
  terminationTargetGoneAfterReap-->>TerminateCommand: return target status

Loading

Suggested reviewers: gnanam1990, pierrunoyt

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 7 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: treating an already-exited POSIX zombie leader as exited during termination.
Linked Issues check	✅ Passed	The implementation addresses issue #862 by detecting exited leaders without relying on ps, preserving descendant termination, and preventing error suppression unless the original signal target is also…
Out of Scope Changes check	✅ Passed	All changes are directly related to issue #862. The platform-specific helpers, termination logic, test seam, and regression tests support zombie-leader detection and safe error handling. No unrelated …

Full details: Linked Issues check

Explanation

The implementation addresses issue #862 by detecting exited leaders without relying on ps, preserving descendant termination, and preventing error suppression unless the original signal target is also gone. The regression tests cover both the unavailable-ps zombie case and the live-descendant failure case.

Full details: Out of Scope Changes check

Explanation

All changes are directly related to issue #862. The platform-specific helpers, termination logic, test seam, and regression tests support zombie-leader detection and safe error handling. No unrelated changes are identified.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[Vasanthdev2004]

Approving. I went looking for the usual ways this shape goes wrong and did not find one.

The probe does not steal the exit status. WNOWAIT leaves the child waitable, so the cmd.Wait that follows still collects it. That was my first concern and it is the right flag. WEXITED alone also means a stopped or continued child is not reported as exited, and the CLD_EXITED/KILLED/DUMPED check makes that explicit rather than relying on it.

A zombie leader with live descendants is still handled. This was the failure mode I most expected: the flag is computed before signalling, but TerminateProcessGroup / terminateProcess still runs either way, so the group is signalled regardless. The flag only decides whether a termination error is discarded, and terminate.go:58 gates that on the pre-termination observation rather than on the reap succeeding. The comment there says why, and it is the right reason: a successful reap says nothing about the tree.

The PID cannot be recycled between probe and kill. The leader is unreaped at probe time, so the zombie holds its PID until Wait.

Build tags are exhaustive. linux, darwin, and !windows && !linux && !darwin. It cross-compiles clean for linux, darwin, windows, freebsd and openbsd, so the other fallback is genuinely reachable rather than an orphan file, and returning false there preserves the old conservative behaviour.

The other fallback and the pid <= 1 guard both fail in the safe direction, and on linux a waitid that cannot answer falls through to /proc rather than guessing.

The regression is a good one: it waits for the leader to actually become waitable-exited instead of sleeping and hoping, sets PATH="" to reproduce #862 exactly, and asserts both that TerminateCommand returns nil and that the process was reaped. It also skips loudly on other platforms rather than passing vacuously. Mutating the package timing vars is safe here since nothing in this package uses t.Parallel(), and it matches what the neighbouring tests already do.

What I could not run. process_posix_test.go is //go:build !windows, so none of it executed on my machine and I could not falsify the new test locally. It ran on the ubuntu job (ok github.com/Gitlawb/zero/internal/background), and macOS covers the sysctl path, but I am not going to imply I exercised either. The darwin implementation I read rather than ran; SysctlKinfoProc with SZOMB is the reasonable choice given x/sys/unix has no waitid wrapper there.

Worth knowing: this PR had never run CI at all. Its checks were sitting at action_required behind the fork gate, so the green you may have seen was CodeRabbit alone. I released it and the full suite passed. go test -race -count=2 ./internal/background is also clean here, for whatever the Windows subset is worth.

[jatmn]

I found issues that need to be addressed before this is ready.

Merge readiness

• [P1] Rebase onto the current main before merge
  HEAD
  This head is based on 27b319ca, while live main is 1b5db176, two commits ahead. The intervening OAuth and TUI work does not overlap this diff and a three-way merge is currently clean, but that is not a substitute for a current review base: repository policy requires a fresh base so that CI and review apply to the actual merge result. Rebase onto main, preserve the upstream changes, and have the resolved head re-reviewed.

Findings

• [P2] Do not turn a failed group termination into a successful cleanup
  internal/background/terminate.go:58
  The root cause is that leaderAlreadyExited is evidence about one process, while terminateErr is evidence about the whole PID/tree target. A zombie leader can be reaped successfully even when a descendant is still alive. The new condition discards every terminateErr after that leader reap, so in the conditional but real case where a descendant cannot be signalled or remains alive after TERM/KILL, TerminateProcessGroup reports the failure but this method returns nil and tells its caller cleanup succeeded. The base branch returned the error.

  The fix needs to preserve the #862 outcome—do not report the false timeout for a zombie-only group when ps is unavailable—without treating a reaped leader as proof that the group is empty. Keep the pre-Wait group signal and the safe PID-only fallback. Suppress an error only after independently establishing that no live group member remains; otherwise return the original termination error. Add a regression that covers an already-exited leader plus a descendant that cannot be terminated, and demonstrate that it fails if the unconditional error suppression is restored. The current tests cover a zombie-only group and a normally signallable descendant, but not this failure boundary.

Overall guidance

This PR is close in scope, but the review friction comes from mixing two different process-lifecycle facts: leader exit/reap and tree termination. Before changing TerminateCommand, write the decision table for those facts separately:

• zombie-only leader + unavailable ps → successful reap and no spurious timeout;
• zombie leader + live, signallable descendant → signal the group and verify the descendant is gone;
• zombie leader + descendant that cannot be stopped → preserve the termination error;
• live leader that exits after TERM → retain the existing conservative behavior unless that broader case is deliberately designed and tested.

Then make the implementation and tests prove the same table on each supported platform. Keeping the approval and fresh-base gates settled before implementation will also keep future review rounds focused on one immutable, policy-approved diff rather than process and branch-state follow-ups.

[jatmn]

@Vasanthdev2004 lgtm