fix(daemon): clean up child after startup timeout#774
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a startup-timeout failure mode in zero daemon start where the CLI could detach from the spawned daemon before readiness is confirmed, allowing a timed-out start attempt to leave a live background daemon behind.
Changes:
- Refactors detached daemon startup to retain ownership of the child process until readiness succeeds, including a final readiness probe at the timeout boundary.
- Adds timeout cleanup logic to terminate and reap the launched child on readiness timeout.
- Adds subprocess-based tests covering timeout cleanup, successful release, and the timeout-boundary readiness probe.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/cli/daemon.go | Moves detached start into a start+await helper, adds readiness polling with a boundary check, and adds termination/reap cleanup on timeout. |
| internal/cli/daemon_test.go | Adds subprocess helpers and tests to validate timeout cleanup, readiness-boundary behavior, and successful release behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func terminateAndReapDaemonProcess(cmd *exec.Cmd) error { | ||
| killErr := cmd.Process.Kill() | ||
| waitErr := cmd.Wait() | ||
| var exitErr *exec.ExitError | ||
| if waitErr == nil || errors.As(waitErr, &exitErr) { |
There was a problem hiding this comment.
Addressed in 27c9bd3. Timeout cleanup now uses a coordinated background.TerminateCommand path instead of leader-only Process.Kill:
- POSIX captures and signals the process group before
Waitcan reap the leader, then reaps concurrently while preserving escalation/liveness checks. - Windows uses
taskkill /T, always reaps the leader, and preserves tree-termination failures if fallback leader termination is needed. - Added POSIX coverage for an exited leader with a surviving child and Windows coverage for the exited-leader reap race.
- Daemon subprocess tests now configure the same child-process-group invariant as production.
Validated on Windows and WSL/Linux, plus go vet ./internal/background ./internal/cli.
WalkthroughThe detached daemon startup path now retains process ownership until readiness succeeds, terminates and reaps timed-out children, and performs zombie-aware process-group liveness checks. Tests cover timeout boundaries, cleanup, release behavior, POSIX process trees, and Windows reaping. ChangesDetached daemon process lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant DaemonProcess
participant TerminateCommand
CLI->>DaemonProcess: start and retain process handle
CLI->>DaemonProcess: poll readiness
DaemonProcess-->>CLI: ready or timeout
CLI->>DaemonProcess: release after readiness
CLI->>TerminateCommand: terminate and reap after timeout
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/cli/daemon_test.go (1)
144-160: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGood coverage of timeout-kill-reap, timeout-termination, and successful release paths.
Solid regression coverage for the behavior described in the PR (deadline-boundary readiness, termination+reaping of a hanging child, release of a ready child). One minor observation: all three tests duplicate the same
exec.Command(os.Args[0], "-test.run=TestDaemonDetachedChildProcess")+ env setup; consider extracting a smallnewHangingDaemonHelper(t *testing.T) *exec.Cmdto cut the repetition, but this is optional.Also applies to: 162-171, 173-192
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/daemon_test.go` around lines 144 - 160, Optionally reduce duplication across the daemon process tests, including TestTerminateAndReapDaemonProcess and the related tests, by extracting the shared exec.Command and ZERO_TEST_DAEMON_DETACHED_CHILD environment setup into a helper such as newHangingDaemonHelper(t *testing.T). Use that helper wherever the detached child process is started, preserving each test’s existing assertions and behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/cli/daemon_test.go`:
- Around line 144-160: Optionally reduce duplication across the daemon process
tests, including TestTerminateAndReapDaemonProcess and the related tests, by
extracting the shared exec.Command and ZERO_TEST_DAEMON_DETACHED_CHILD
environment setup into a helper such as newHangingDaemonHelper(t *testing.T).
Use that helper wherever the detached child process is started, preserving each
test’s existing assertions and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3f62b5b4-f2b4-4e99-a7c5-91b81df6b580
📒 Files selected for processing (2)
internal/cli/daemon.gointernal/cli/daemon_test.go
|
Fixed the macOS smoke failure in f73194f. The process-group cleanup succeeded, but macOS can retain an orphaned child briefly as a zombie; kill(pid, 0) still succeeds for zombies, so the regression test incorrectly classified the dead child as running. The POSIX assertion now accepts either ESRCH or Z process state. Revalidated internal/background on WSL/Linux and internal/background + internal/cli on Windows. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/background/process_posix.go (1)
173-180: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared
cmd.Wait()error classification instead of duplicating it 3x.The same
var exitErr *exec.ExitError; if waitErr != nil && !errors.As(waitErr, &exitErr) { return fmt.Errorf("reap process: %w", waitErr) }block is duplicated verbatim across both platform files. As per coding guidelines, "Prefer one cross-platform function with small conditional checks over duplicated platform-specific helpers when behavior can remain unified" — this logic is platform-independent and belongs in one shared helper (e.g. in terminate.go).
internal/background/process_posix.go#L173-L180: keep as the canonical helper, or replace its body with a call to a new sharedclassifyWaitError(err error) error.internal/background/process_posix.go#L163-L166: replace inline block with a call to the shared helper.internal/background/process_windows.go#L37-L41: replace inline block with a call to the same shared helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/background/process_posix.go` around lines 173 - 180, Extract the platform-independent wait-error classification into one shared helper, such as classifyWaitError, in the shared background package. In internal/background/process_posix.go lines 173-180, retain the existing behavior in that helper or delegate waitForTerminatedCommand to it; replace the inline classification at lines 163-166 and in internal/background/process_windows.go lines 37-41 with calls to the shared helper, preserving the existing handling of *exec.ExitError and wrapped reap-process errors.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/background/process_posix.go`:
- Around line 107-122: Update terminateCommand so the non-process-gone error
branch after syscall.Getpgid also calls waitForTerminatedCommand before
returning, ensuring the child is reaped on every error path while preserving the
existing error handling for processGoneError.
In `@internal/background/process_windows.go`:
- Around line 31-52: Update terminateCommand to reap cmd with a bounded wait,
mirroring the timeout-based approach used by the POSIX terminateCommand: perform
cmd.Wait() asynchronously and select between its result and the existing
termination timeout. Return a timeout error when the process cannot be reaped in
time, while preserving the current taskkill, fallback-kill, and wait-error
handling for completed waits.
---
Nitpick comments:
In `@internal/background/process_posix.go`:
- Around line 173-180: Extract the platform-independent wait-error
classification into one shared helper, such as classifyWaitError, in the shared
background package. In internal/background/process_posix.go lines 173-180,
retain the existing behavior in that helper or delegate waitForTerminatedCommand
to it; replace the inline classification at lines 163-166 and in
internal/background/process_windows.go lines 37-41 with calls to the shared
helper, preserving the existing handling of *exec.ExitError and wrapped
reap-process errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7291e022-8dba-44b4-b1a4-45dce9dd5ef2
📒 Files selected for processing (7)
internal/background/process_posix.gointernal/background/process_posix_test.gointernal/background/process_windows.gointernal/background/process_windows_test.gointernal/background/terminate.gointernal/cli/daemon.gointernal/cli/daemon_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/cli/daemon.go
- internal/cli/daemon_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/background/process_posix_test.go (1)
153-162: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLift the exit condition into the
forloop.The loops use an unidiomatic
for { if condition { break } }pattern. As flagged by static analysis, lifting the check into the loop condition improves readability and clarifies the intent.
internal/background/process_posix_test.go#L153-L162: Lift!processStopped(childPID)into the loop condition and remove the internalbreak.internal/background/process_posix_test.go#L196-L205: Lift!processStopped(childPID)into the loop condition and remove the internalbreak.♻️ Proposed refactor
- for { - if processStopped(childPID) { - break - } + for !processStopped(childPID) { if time.Now().After(deadline) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/background/process_posix_test.go` around lines 153 - 162, Update both polling loops in internal/background/process_posix_test.go at lines 153-162 and 196-205 to use !processStopped(childPID) as the for-loop condition, remove the internal break, and preserve the existing deadline timeout, kill, failure, and sleep behavior.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/background/process_posix_test.go`:
- Around line 153-162: Update both polling loops in
internal/background/process_posix_test.go at lines 153-162 and 196-205 to use
!processStopped(childPID) as the for-loop condition, remove the internal break,
and preserve the existing deadline timeout, kill, failure, and sleep behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 66bb37f6-1461-4153-901a-27df32e5a8e3
📒 Files selected for processing (1)
internal/background/process_posix_test.go
|
Fixed the remaining macOS CI race in 4cb866e. On Darwin, Getpgid can return ESRCH after the configured process-group leader exits even though descendants still remain in that group. TerminateCommand now uses the launch-time Setpgid/Pgid configuration (which guarantees PGID == leader PID) rather than trying to rediscover it after exit, so the descendants are still signalled. Validated internal/background 10x on Linux, internal/background + internal/cli and vet on Windows, and Darwin cross-compilation. |
|
Addressed the remaining CodeRabbit feedback in 3ed4571: shared wait-error classification, bounded Windows reaping, fallback termination plus bounded reaping on non-ESRCH Getpgid failures, and the two staticcheck loop cleanups. Validated background/CLI tests and vet on Windows, background tests 10x plus vet on Linux, and Darwin cross-compilation. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Resolve the current merge conflict before this can be merged
GitHub currently reports this PR asmergeable: falsewith merge statedirty. Please rebase or otherwise resolve the conflict against the current base, then rerun the affected checks so the reviewed diff is mergeable. -
[P2] Do not count zombie descendants as a surviving process group
internal/background/process_posix.go:158
aliveuseskill(-pgid, 0) == nil, but that call succeeds for zombie processes. The newly added test explicitly treats aZprocess state as stopped because an orphaned child can remain a zombie temporarily. When a timed-out daemon's group contains such a child, this helper waits through both grace periods, sends SIGKILL to an already-dead group, and finally returnsprocess group <pid> did not exit after SIGKILL. That makeszero daemon startreport a cleanup failure even though the tree has stopped (and can make the new macOS scenario flaky). Use liveness semantics that distinguish zombies from running descendants, or otherwise accept the already-dead state before declaring cleanup failed.
…n-startup-cleanup main's Gitlawb#781 unified process lifecycle into internal/execution, reducing internal/background's platform files to wrappers over execution.TerminateProcessTree. This branch had grown its own POSIX/Windows killers there, so the conflict resolves to main's wrappers and the branch's substance moves onto the unified path: - background.TerminateCommand keeps the capability the daemon needs (stop the tree AND reap the leader, which TerminateProcess cannot do for a caller that still owns the exec.Cmd) but is now cross-platform: it calls terminateProcess and then bounds cmd.Wait, instead of duplicating a second platform-specific killer. That also settles the earlier review note about duplicating the Wait-error classification per platform. - The zombie-liveness fix moves to internal/execution/process_unix.go, which is where the flawed check now lives. Zombie fix: kill(2) with signal 0 succeeds for a process waiting to be reaped, so a group whose only remaining member was a zombie sat through both grace periods, took a pointless SIGKILL, and was then reported as "did not exit after SIGKILL" — a spurious cleanup failure whose timing depended on when the reaper ran, which is exactly the macOS flake risk. signalTargetRunning now consults process states (via ps, which both Linux and Darwin provide, and only after the cheap kill check says something is still there) and treats an all-zombie group as exited. If ps is unavailable the previous conservative answer stands. Tests: process_unix_test.go covers the zombie group (it must be seen as exited, and TerminateProcessTree must succeed rather than report the SIGKILL error) plus a live-group positive control so the tolerance cannot degrade into ignoring real processes.
|
Merged current [P1] Resolve the merge conflict — done, and it was not a mechanical resolution:
[P2] Do not count zombie descendants as a surviving process group — fixed, in
That removes exactly the failure you described: a timed-out daemon whose group held a zombie no longer waits through both grace periods, SIGKILLs a dead group, and reports Tests —
Validation (Windows host, Go 1.26.5): Being explicit about one limit: the new POSIX test is compile-verified here but cannot execute on this Windows host, so the ubuntu/macOS CI jobs are what will run the zombie scenario — the case whose timing you flagged as flaky. If it misbehaves there I will follow up rather than leave it. @jatmn — ready for another look, especially the decision to move the fix into @coderabbitai review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/execution/process_unix.go (1)
147-162: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNon-
ESRCHGetpgiderrors are silently discarded.When
syscall.Getpgid(pid)fails with something other thanESRCH, the function falls through and returns(pid, nil)without surfacing the error, unlike the explicitly-documentedESRCHbranch. Worth at least a comment noting this is an intentional conservative fallback (signal the individual pid), so future readers don't mistake it for an oversight.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/execution/process_unix.go` around lines 147 - 162, Update processSignalTarget to document the intentional conservative fallback when syscall.Getpgid returns a non-ESRCH error: retain the individual pid target and return successfully, while preserving the existing ESRCH handling and process-group behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/execution/process_unix.go`:
- Around line 104-117: Update signalTargetRunning to distinguish process groups
from individual processes: call processGroupHasRunningMember only for negative
target values, and for positive targets perform a per-process zombie check using
processIsZombie or the equivalent production helper. Preserve the existing
syscall liveness check and ensure positive raw PIDs never use their numeric
value as a process-group ID.
---
Nitpick comments:
In `@internal/execution/process_unix.go`:
- Around line 147-162: Update processSignalTarget to document the intentional
conservative fallback when syscall.Getpgid returns a non-ESRCH error: retain the
individual pid target and return successfully, while preserving the existing
ESRCH handling and process-group behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5448f12d-28c9-4663-b99f-a96fd1f3ee01
📒 Files selected for processing (4)
internal/background/process_posix_test.gointernal/background/terminate.gointernal/execution/process_unix.gointernal/execution/process_unix_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/background/terminate.go
- internal/background/process_posix_test.go
| func signalTargetRunning(target int) bool { | ||
| if syscall.Kill(target, syscall.Signal(0)) != nil { | ||
| return false | ||
| } | ||
| pgid := target | ||
| if pgid < 0 { | ||
| pgid = -pgid | ||
| } | ||
| running, ok := processGroupHasRunningMember(pgid) | ||
| if !ok { | ||
| return true | ||
| } | ||
| return running | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Positive target is treated as a pgid even when it isn't one.
processSignalTarget returns a positive target (the raw pid, not a group id) whenever Getpgid reports the process isn't its own group leader, or when it fails with a non-ESRCH error (that error is silently dropped too). signalTargetRunning then does pgid := target unconditionally and searches ps for members whose actual pgid equals this value — but for a non-leader pid, that's not its real group id, just its own pid number. The ok=false fallback usually masks this, but if some unrelated process group's real pgid happens to numerically match this pid, the zombie-check on that unrelated group's state gets applied to this target's liveness determination, which could make TerminateProcessTree conclude the target is gone (and skip SIGKILL) while it's actually still running.
Consider branching on the sign of target: use processGroupHasRunningMember only when target < 0 (a real group), and use a per-pid zombie check (like processIsZombie in the test file) when target > 0.
Sketch of the fix
func signalTargetRunning(target int) bool {
if syscall.Kill(target, syscall.Signal(0)) != nil {
return false
}
- pgid := target
- if pgid < 0 {
- pgid = -pgid
- }
- running, ok := processGroupHasRunningMember(pgid)
- if !ok {
- return true
- }
- return running
+ if target < 0 {
+ running, ok := processGroupHasRunningMember(-target)
+ if !ok {
+ return true
+ }
+ return running
+ }
+ zombie, ok := processIsZombie(target)
+ if !ok {
+ return true
+ }
+ return !zombie
}Also applies to: 147-162
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/execution/process_unix.go` around lines 104 - 117, Update
signalTargetRunning to distinguish process groups from individual processes:
call processGroupHasRunningMember only for negative target values, and for
positive targets perform a per-process zombie check using processIsZombie or the
equivalent production helper. Preserve the existing syscall liveness check and
ensure positive raw PIDs never use their numeric value as a process-group ID.
Summary
Validation
go test ./internal/cli -count=1go vet ./internal/cligit diff --checkCloses #770
Summary by CodeRabbit
Bug Fixes
Tests