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.
|
Warning Review limit reached
Next review available in: 59 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 Plus Run ID: 📒 Files selected for processing (8)
WalkthroughThe detached daemon startup path 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
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] Preserve the configured process-group target when the leader has already exited
internal/background/terminate.go:41
TerminateCommandnow hands only the leader PID toTerminateProcessTree, which re-discovers the group withGetpgid. On macOS, that lookup returnsESRCHafter an unreaped group leader exits even if its child is still in the configured group. The fallback consequently signals the dead positive PID, treatsESRCHas success, reaps only the leader, and leaves the descendant running. This is not hypothetical: the required macOS smoke job fails inTestTerminateCommandKillsChildAfterLeaderExits, and its runner subsequently cleans up the orphanedsleepprocess. Retain the launch-timeSetpgidfact (and target-cmd.Process.Pidwhen applicable) before attempting the reap. -
[P3] Do not inspect a PID-only target as if it were a process group
internal/execution/process_unix.go:108
processSignalTargetdeliberately returns a positive PID for a process that is not its own group leader, butsignalTargetRunningpasses that value toprocessGroupHasRunningMember. Since the target's actual PGID differs from its PID, an unreaped zombie produces no matching group row and is conservatively considered live. The termination loop then waits both grace periods, signals an already-dead process, and returns a spurious cleanup failure. Branch on the sign oftarget: inspect group members only for a negative group target and inspect the individual process state for the positive-PID fallback. -
[P3] Treat an already-exited Windows child as successfully cleaned up after it is reaped
internal/background/terminate.go:43
If the daemon exits before the readiness deadline,taskkill /Tfails because its PID no longer exists and the fallbackProcess.Killalso fails.waitForTerminatedCommandWithinthen successfully reaps the exited command, butTerminateCommandstill returns the earlier termination error, sostartAndAwaitDaemonProcessreportscleanup failedeven though cleanup succeeded. The new Windows test discardsTerminateCommand's error, which hides this path. Classify the already-gone result as success after a successful reap and assert the returned error in the test.
…cation Addresses jatmn's three Gitlawb#774 review findings: - TerminateCommand rediscovered the process group via execution.TerminateProcessTree's Getpgid(pid) lookup. On Darwin, that lookup can return ESRCH once an unreaped group leader has already exited, even though live descendants remain in the group it configured at launch — silently falling back to signalling only the (already dead) leader PID and leaving descendants running (the required macOS smoke job's failure). Add execution.TerminateProcessGroup, which signals the negative PID directly using the launch-time Setpgid invariant instead of rediscovering it, and route TerminateCommand through it. - signalTargetRunning's zombie check matched the individual-PID fallback target (a process that is not its own group leader) against the process table by PGID. That target's actual PGID differs from its own PID by definition, so no row ever matched, unknown resulted every time, and a zombie individual-PID target was always conservatively reported as still running. Branch on the sign of the target: group members by PGID, individual PIDs by PID. - TerminateCommand returned the termination error even when the subsequent reap succeeded — a successful cmd.Wait is authoritative proof the leader is gone and collected, regardless of how the kill attempt itself went (e.g. taskkill /T racing an already-exited PID on Windows). Classify a successful reap as success. All three regression tests are verified to fail against the pre-fix code (confirmed by temporarily reverting each fix and rerunning). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Pushed [P1] Preserve the configured process-group target when the leader has already exited (
Fixed by adding [P3] Do not inspect a PID-only target as if it were a process group (
Fixed by branching on the sign of Added [P3] Treat an already-exited Windows child as successfully cleaned up after it is reaped ( If the daemon exits before the readiness deadline, Fixed by treating a successful reap as authoritative: All three regression tests verified to have teeth: temporarily reverted each fix in isolation, confirmed the corresponding test fails against the pre-fix code, then restored and confirmed all pass. Validation:
Same limit as before: the group-leader fix targets a Darwin-specific @jatmn ready for another look. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 70-71: Validate pid in TerminateProcessGroup before negating it,
rejecting values less than or equal to 1 consistently with processSignalTarget.
Return the established invalid-PID error and avoid calling terminateTarget for
rejected inputs; preserve the existing termination flow for valid process-group
leader PIDs.
🪄 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: 75363abf-3f4f-4d97-83bd-4f3af30cece7
📒 Files selected for processing (6)
internal/background/process_posix.gointernal/background/process_windows.gointernal/background/process_windows_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/process_windows_test.go
- internal/background/terminate.go
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] Reject invalid process-group PIDs before negating them
internal/execution/process_unix.go:70
UnlikeTerminateProcessTree, this new exported entry point bypassesprocessSignalTargetand immediately negates its argument. A zero PID therefore callskill(0, SIGTERM)/SIGKILLagainst the caller's process group, while PID 1 becomeskill(-1, ...)and can signal every permitted process. Rejectpid <= 1with the established invalid-PID error before constructing the negative group target. -
[P3] Document or enforce
TerminateCommand's process-group precondition
internal/background/terminate.go:37
The current daemon caller establishes the required group correctly, but this new exported API promises to terminate any started command while the POSIX path unconditionally callsTerminateProcessGroup(-cmd.Process.Pid). For a normalexec.Command("sleep", "300")started withoutConfigureChildProcessGroup, that negative PID is not a group ID, soSIGTERMreturnsESRCH, the termination helper reports success, and the subsequent reap times out whilesleepremains alive. Either make the required pre-start configuration an enforced/private invariant, or fall back to the safe PID/tree termination path for unconfigured commands; add coverage for the unconfigured case. -
[P3] Preserve the delayed PID-file synchronization test
internal/config/command_test.go:167
This merge/rebase resolution removes the polling helper andTestAssertProcessTerminatedWaitsForDelayedPIDFilethat currentmainadded in #802. The background-command fixtures write their PID asynchronously, so the restored one-shotos.ReadFilecan fail whenever cleanup finishes before the child records the PID—the exact Windows startup race that #802 fixed. Keep the base branch's bounded wait and its regression test; this daemon-lifecycle change should not revert that unrelated test stabilization.
…n-startup-cleanup
Amp-Thread-ID: https://ampcode.com/threads/T-019fa018-abb8-73de-9489-51e9e6514c6b Co-authored-by: Pierre Bruno <pierrebruno@hotmail.ch>
|
Final follow-up on current head
The PR is mergeable. PierrunoYT requests a fresh review to supersede the previous |
…n-startup-cleanup Amp-Thread-ID: https://ampcode.com/threads/T-019fa55a-55b2-740c-a267-ede95939ea93 # Conflicts: # internal/background/process_posix_test.go
|
Merged current upstream The only merge conflict was in the POSIX forked-child lifecycle test. I resolved it semantically so the test keeps both guarantees:
I also re-read the current reviews and audited the merged result. The still-valid lifecycle requests remain addressed:
Validation with Go 1.26.5 on Linux:
Skipped locally: native macOS and Windows execution (this is a Linux orb). The required platform CI jobs are queued on the new head and will execute those tests natively. The remaining |
|
CI follow-up for The PR remains |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Thanks for this. The problem is real and the ordering argument in the TerminateCommand comment (signal the tree before Wait releases the PID) is exactly right, so I want this to land. Three things to sort first, and one of them is a comment that says something the code does not do.
The Windows doc comment is not true
terminateOwnedProcess says:
KillProcessTree always operates on the whole process tree via taskkill /T regardless of how the PID was obtained
KillProcessTree is taskkill /T, and if that returns any error it falls back to os.FindProcess(pid) then process.Kill(), which is the leader alone. So "always operates on the whole process tree" is not right on the fallback path.
More importantly, /T itself does not reach a tree whose root has already exited. I checked rather than assumed:
leader pid: 4264
before kill, PING.EXE present=true
taskkill /T on exited leader err: exit status 128
after kill, PING.EXE present=true
A leader that exits on its own leaving a descendant is precisely the shape the POSIX side has a dedicated test for (TestTerminateCommandKillsChildAfterLeaderExits), and there is no Windows counterpart. The Windows test covers a leaf child with no descendants.
A reaped leader is evidence about the leader, not the group
// A successful reap is authoritative: cmd.Wait returned, so the leader is
// confirmed gone and collected.
return nilThat is authoritative about the leader, and TerminateCommand's own doc says it "stops the whole group". Put the two together with the probe above and you get the bad case: taskkill /T fails, the fallback cannot help either, cmd.Wait reaps the leader fine, and the function returns nil while a descendant is still running. The caller is told cleanup succeeded.
The swallow is right for the case it was added for, a kill racing a process that already exited. The issue is that it is not scoped to that case, so it also hides a genuine group-termination failure.
The abandoned Wait goroutine
go func() { waitDone <- cmd.Wait() }()
select {
case waitErr := <-waitDone:
return classifyWaitError(waitErr)
case <-time.After(timeout):
return fmt.Errorf("process %d did not reap after termination", cmd.Process.Pid)
}On the timeout branch the goroutine is still sitting in cmd.Wait(), and it will later write cmd.ProcessState. The channel is buffered so nothing leaks on the send, but the caller gets its error back while still holding a cmd that a live goroutine is going to mutate, with no happens-before edge. Anything that subsequently reads cmd.ProcessState is racing.
That post-condition is not documented either. If the intent is "after a non-nil return, never touch cmd again", please say so on the exported function, since TerminateCommand is the thing callers see.
Scope, to be fair to you
I do not think this is currently exploitable in production. zero daemon start launches zero.exe daemon start --foreground, which does not fork and exit, so during the startup window there should be no descendant to orphan. I am treating the Windows gap as a latent hole in a newly exported cross-platform primitive whose comment overstates its guarantee, rather than as a live bug. Correct me if there is a caller I have missed.
What I would like
- Fix the comment so it describes the fallback and the dead-root case, or make the code match the comment.
- Scope the
terminateErrswallow to the already-exited case instead of every successful reap, so a real group-termination failure still reaches the caller. - Document (or remove) the abandoned-goroutine post-condition on the reap timeout.
- A Windows test for the leader-exits-with-descendant case, to match the POSIX one.
One more thing I would like your read on rather than asserting, since I have not run it myself: I am told TestTerminateCommandKillsChildAfterLeaderExits still passes on Linux with the process_posix.go group-retention change removed, leaving macOS as the only platform that actually discriminates. If that is right the test is weaker than it looks on two of three platforms. Worth checking before this merges.
Amp-Thread-ID: https://ampcode.com/threads/T-019fa7a0-1223-701d-9529-48ba5d7cf8c8 Co-authored-by: Pierre Bruno <pierrebruno@hotmail.ch>
|
PierrunoYT addressed the Windows termination review in
Validation: affected Linux packages pass with |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Re-reviewed e22f193a. All three are addressed and the way you addressed the third one is better than what I asked for. Approving.
I ran the new tests on real Windows rather than reading them:
--- PASS: TestTerminateCommandReturnsTreeFailureAfterRootFallbackReaps (0.02s)
--- PASS: TestTerminateCommandReapsExitedLeader (0.73s)
--- PASS: TestTerminateCommandDeadLeaderCannotIdentifyDescendantTree (1.48s)
And mutation checked the one that matters. Reverting the !leaderAlreadyExited scoping back to the unconditional swallow fails with TerminateCommand error = <nil>, want tree failure, so the fix is load-bearing rather than decorative.
The comment now says taskkill can discover descendants only while the root PID exists, and that a Windows tree has no addressable identity once its root is gone. That is accurate and it is the thing I could not verify from the old wording.
The swallow is now conditioned on the platform having demonstrated, before termination was attempted, that the leader had already exited, via GetExitCodeProcess on a pinned handle. That is a stronger basis than the reap I was objecting to, because the reap only ever proved something about the leader.
The goroutine post-condition is documented on the exported function, including why the Wait is left running rather than abandoned. That answers the question I actually had, which was not "fix this" but "say what a caller may do afterwards".
TestTerminateCommandDeadLeaderCannotIdentifyDescendantTree asserting that the descendant SURVIVES is the right call and better than what I suggested. Pinning the limitation is more useful than a test that would have to pretend the platform can do something it cannot, and it means a future change cannot quietly claim otherwise.
Pinning the handle across the taskkill call to stop PID recycling was not something I raised. Good addition.
Two notes, neither blocking and neither needing a response:
stillActiveExitCode = 259 is the documented value, and it is also a legitimate exit code a process can return. A child that genuinely exits with 259 reads as still-running here, so leaderAlreadyExited stays false and a termination error would be reported rather than swallowed. That is the safe direction, so I would leave it, but it is worth a word in the comment if you touch the file again.
I never verified my earlier remark that the POSIX regression may be insensitive on Linux. Treat it as unconfirmed rather than as something you still owe me.
Summary
Validation
go test ./internal/cli -count=1go vet ./internal/cligit diff --checkCloses #770
Summary by CodeRabbit
Bug Fixes
Tests