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.
|
Warning Review limit reached
Next review available in: 7 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughDetached daemon startup retains ownership of the child through readiness checks, performs a final timeout check, terminates and reaps failed processes, and releases successful ones. Cross-platform command termination now handles process trees and reaping. ChangesDaemon startup and process lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant startAndAwaitDaemonProcess
participant daemonReachable
participant TerminateCommand
participant daemonProcess
CLI->>startAndAwaitDaemonProcess: start detached daemon
startAndAwaitDaemonProcess->>daemonProcess: start child
startAndAwaitDaemonProcess->>daemonReachable: poll readiness
daemonReachable-->>startAndAwaitDaemonProcess: ready or unavailable
alt Ready
startAndAwaitDaemonProcess->>daemonProcess: release process handle
else Timeout
startAndAwaitDaemonProcess->>daemonReachable: perform final check
startAndAwaitDaemonProcess->>TerminateCommand: terminate and reap child
TerminateCommand->>daemonProcess: stop process tree and wait
end
startAndAwaitDaemonProcess-->>CLI: startup result
Possibly related PRs
🚥 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.
Summary
Validation
go test ./internal/cli -count=1go vet ./internal/cligit diff --checkCloses #770
Summary by CodeRabbit