fix(config): kill provider-command process tree via job object on Windows#690
fix(config): kill provider-command process tree via job object on Windows#690PierrunoYT wants to merge 11 commits into
Conversation
…dows taskkill /T walks the process tree by parent PID in user space and can miss descendants, leaving the sleeping child holding the inherited stdout/stderr pipes so cmd.Wait() blocked until natural completion (~10s instead of the 5s timeout). Assign the started command to a job object with JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE and terminate the job on timeout so the kernel kills the whole tree atomically; taskkill remains as a fallback. Also set cmd.WaitDelay so Wait can never hang on pipes held by an escaped orphan, and add a regression assertion that the sleeping child is actually gone after the timeout returns. Fixes Gitlawb#683 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses Windows flakiness/performance in provider-command timeouts by ensuring the entire provider-command process tree is reliably terminated (fixing Issue #683, where TestLoadProviderCommandTimeout could take ~10s instead of ~5s on Windows).
Changes:
- Introduces a cross-platform
commandProcessabstraction and uses a Windows job object (JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE) to kill the process tree atomically on timeout (with taskkill/Process.Kill fallback). - Sets
cmd.WaitDelay = 1sto bound post-exit pipe draining soWait()can’t hang indefinitely. - Extends the timeout regression test to record a sleeper PID and assert the child process is actually terminated on both Windows and POSIX.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/config/process_windows.go | Adds job-object-based process-tree termination for provider commands on Windows (plus fallback termination). |
| internal/config/process_windows_test.go | Adds Windows implementation of processAlive(pid) used by the timeout regression assertion. |
| internal/config/process_posix.go | Refactors POSIX termination to the new commandProcess abstraction (process group kill + direct kill). |
| internal/config/process_posix_test.go | Adds POSIX implementation of processAlive(pid) used by the timeout regression assertion. |
| internal/config/command.go | Hooks in attachCommandProcess + Close() lifecycle and sets cmd.WaitDelay to bound Wait() behavior. |
| internal/config/command_test.go | Updates timeout fixture to persist PID and assert no leftover sleeper process remains. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if _, err := windows.SetInformationJobObject(job, windows.JobObjectExtendedLimitInformation, uintptr(unsafe.Pointer(&info)), uint32(unsafe.Sizeof(info))); err != nil { | ||
| _ = windows.CloseHandle(job) | ||
| return 0, err | ||
| } |
| const stillActive = 259 // STATUS_PENDING | ||
| return code == stillActive |
| sleep := "Start-Sleep -Seconds " + itoa(script.SleepSeconds) | ||
| if script.PidFile != "" { | ||
| sleep = "Set-Content -Path '" + script.PidFile + "' -Value $PID -Encoding Ascii; " + sleep | ||
| } | ||
| lines = append(lines, "powershell -NoProfile -Command \""+sleep+"\"") |
| if script.PidFile != "" { | ||
| lines = append(lines, "echo $$ > '"+script.PidFile+"'") | ||
| } |
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] Terminate the process group when
WaitDelayexpires
internal/config/command.go:71
A command such assleep 600 & exitlets its shell exit immediately while the background child retains the inherited stdout/stderr pipes.cmd.Wait()then returnsexec.ErrWaitDelayafter the new one-second delay, which selects this branch and returns without callingproc.Terminate(). On POSIXCloseis a no-op, so the child continues running; before this change the five-second timer would kill that process group. Treat the wait-delay result as an incomplete command, terminate the tree, and add a background-child regression case. -
[P1] Close the Start-to-job-assignment escape window
internal/config/process_windows.go:31
The job is attached only aftercmd.Start()has allowedcmd.exeto execute. A fast command can create and detach a child beforeAssignProcessToJobObject, leaving that child outside the job. On timeout,Terminate()kills the job/root first and only then runstaskkillagainst the now-dead root, so the fallback cannot traverse to that reparented child. It can continue holding the pipes or executing after the timeout, preserving the race this PR is intended to eliminate. Start suspended and assign before resuming (or use equivalent launch-time containment), and test a forced pre-assignment child. -
[P2] Do not kill descendants after a successful provider command
internal/config/process_windows.go:80
Closeis deferred for every return path, and the job hasJOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE. Therefore a provider command which successfully emits its JSON but launches a detached helper/proxy has that child terminated asLoadProviderCommandreturns. The old behavior only killed the tree on timeout. Restrict the kill-on-close cleanup to timeout/error cleanup, or otherwise preserve successful-command descendants. -
[P3] Quote PID-file paths when generating the timeout fixture
internal/config/command_test.go:159
The new PowerShell and POSIX fixtures interpolatePidFileinside single-quoted shell literals without escaping apostrophes. A temporary directory such asO'Brienproduces an invalid script, so the PID file is never written and this test fails before exercising the timeout behavior. Escape the platform-specific quote character (or avoid shell interpolation) for both generated scripts. -
[P3] Correct the Windows process-status comment
internal/config/process_windows_test.go:19
Exit code259isSTILL_ACTIVE, notSTATUS_PENDING. The value is correct, but the comment misstates the Windows API contract and will mislead future maintenance; use the proper constant name or correct the comment.
Address code review on PR Gitlawb#690: terminate the process tree when cmd.Wait() returns exec.ErrWaitDelay (a background descendant kept inherited pipes open, previously left running); start the Windows process suspended and assign it to the job object before resuming its main thread, closing the window where a fast child could escape the job; drop JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE so a successful command's detached helpers survive Close() instead of being reaped; escape single quotes in generated PID-file paths; and correct a misnamed Windows exit-code constant in a comment. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 6 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 (3)
WalkthroughProvider command execution now bounds I/O draining, attaches platform-specific process handles, and terminates process trees on timeout or wait-delay errors. Cross-platform tests verify prompt termination of timed-out commands and detached children. ChangesProvider command termination
Provider wizard test update
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ProviderCommand
participant ProcessHandle
participant ProcessTree
ProviderCommand->>ProcessHandle: attach command
ProcessHandle->>ProcessTree: register process group or job
ProviderCommand->>ProcessHandle: terminate on timeout or wait-delay
ProcessHandle->>ProcessTree: kill descendants
ProcessTree-->>ProviderCommand: command returns timeout
🚥 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}
gofmt's doc-comment formatter rewrites a trailing '' into a curly closing quote, which CI's gofmt -l check flags as unformatted. Reword to avoid the pattern instead of embedding a raw quote pair.
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] Terminate leaked descendants when the provider shell exits nonzero
internal/config/command.go:71
exec.Cmd.Waitpreserves anExitErrorover its pipe-drainErrWaitDelay. A command such assleep 600 & exit 7therefore returnsexit status 7after the one-second drain deadline, bypasses thisErrWaitDelaybranch, and leaves the background process running: POSIXCloseis a no-op and the Windows job handle is merely released. Track the drain-timeout condition independently and terminate the tree even when the shell's exit status is nonzero; add a nonzero-exit background-child regression case. -
[P1] Do not rely on
taskkillafter failed job assignment and a completed wait
internal/config/process_windows.go:53
If a parent job forbids nested jobs (or either job setup call fails), assignment falls back totaskkill. A provider command can then start a detached child and exit; by the timeWaitDelayreturns,cmd.exehas been reaped and/T /PID <cmd.exe>cannot walk to the reparented child.LoadProviderCommandreports a timeout while the child remains running, recreating the leak this PR is intended to eliminate. Provide launch-time containment for the fallback or otherwise preserve a reliable descendant handle, and test a forced assignment-failure/background-child path. -
[P1] Never pass a reaped provider PID to
taskkill
internal/config/process_windows.go:85
On theErrWaitDelaypath,cmd.Waithas already reaped the root process. Even after a successfully attached job has terminated the original tree, this method unconditionally runstaskkill /PIDusing that stale numeric PID. Windows can reuse PIDs, so a fast-reused PID can make a provider-config timeout forcibly kill an unrelated process tree. Skip the PID fallback once the job path was used, and do not use a post-reap PID as a tree identity. -
[P2] Handle failure to resume the suspended command
internal/config/process_windows.go:64
Every provider command is created withCREATE_SUSPENDED, but failures from the Toolhelp snapshot, thread enumeration,OpenThread, andResumeThreadare ignored. If the primary thread cannot be reopened (Toolhelp acquisition itself is fallible), it stays suspended and an otherwise valid command is misreported as a five-second timeout. Retry or surface the resume failure, ensure the process is cleaned up, and add coverage for that failure path. -
[P3] Correct the PR description's job-object semantics
internal/config/process_windows.go:99
The PR description says the job usesJOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE, but the current implementation explicitly does not set that limit and intentionally leaves successful-command descendants running whenClosereleases the job handle. Update the description so reviewers do not approve a behavior the code deliberately avoids.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
The job-object approach is sound — suspend-start, assign to the job, then resume means descendants can't escape the tree, and the WaitDelay + ErrWaitDelay handling closes the pipe-leak gap that taskkill /T had. Build, vet, gofmt, and the config tests all pass on Windows, including the new background-child test (1.66s, confirming the WaitDelay path fires and the leftover child is actually killed). Two small things worth a glance: resumeMainThread resumes every thread owned by the pid, not just the primary one (harmless, arguably more correct, but the name oversells it); and the PR description claims JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE is set, but the code intentionally doesn't set it — the Close() comment is accurate, so the description should be corrected to avoid confusing future readers. Approving as-is.
There was a problem hiding this comment.
Switching my earlier approve to request changes I reproduced the leak jatmn flagged.
The blocking one: a provider command that exits nonzero while a detached child holds the inherited pipes (e.g. sleep 600 & exit 7) leaks the child. Go only returns exec.ErrWaitDelay on a successful exit a nonzero exit yields the bare *ExitError, so the errors.Is(err, exec.ErrWaitDelay) branch at command.go:72 is skipped, proc.Terminate() never runs, and the child survives (on Windows Close just drops the job handle no KILL_ON_JOB_CLOSE; on POSIX Close is a no-op). I confirmed it on Windows: the sleeper PID stayed alive after a ...exit status 7. The new background-child test uses exit 0, so it only walks the already-working branch. It's cross-platform the POSIX path has the same gating. Smallest fix: call proc.Terminate() whenever err != nil (killing an already-dead parent is harmless), and add an ExitCode: 7 + BackgroundSleepSeconds test variant.
Second one, high: process_windows.go Terminate() runs taskkill /T /F /PID + Process.Kill() unconditionally, even after TerminateJobObject already killed the tree. On the ErrWaitDelay path the root is already reaped by Wait, so that PID is free and Windows can hand it to an unrelated process taskkill /T on a reused PID force-kills the wrong tree. Gate the PID fallback with if p.job == 0 (it's only the real fallback when job creation failed) and skip it once the process is reaped.
The rest are real but fine as follow-ups: the no-job fallback can't reach a reparented descendant (non-regressive pre-PR had no containment at all), resumeMainThread swallows Toolhelp/OpenThread/ResumeThread failures so a stuck-suspended command misreports as a 5s timeout, the new background-child test can flake if PowerShell cold-start loses the race with the 1s WaitDelay kill before bg.pid is written, and the itoa test helper returns "" for negatives. What's solid: CREATE_SUSPENDED + assign-before-resume closes the root escape window, the job kills the whole tree on the success path, the POSIX group-kill is correct, and bounding Wait with WaitDelay is the right idea.
…reuse Terminate the process tree on any error returned from cmd.Wait, not only exec.ErrWaitDelay. Go only returns ErrWaitDelay when the shell itself exited successfully; a nonzero exit (e.g. sleep 600 & exit 7) yields the bare *ExitError instead, so a detached background child that held the inherited pipes open was never terminated on that path. Also stop falling through to the taskkill/PID fallback once a job handle successfully contained the tree: by the time Terminate runs on the ErrWaitDelay path, cmd.Wait has already reaped the root process, so its PID may have been reused by an unrelated process, and taskkill /T /PID against a reused PID would force-kill the wrong tree. Addresses PR Gitlawb#690 review feedback.
resumeMainThread now reports whether it actually resumed at least one thread. If Toolhelp/OpenThread/ResumeThread all fail, the launched process would otherwise sit suspended forever, never exit on its own, and get misreported as a plain 5s provider-command timeout instead of the real resume failure. attachCommandProcess now terminates the process immediately in that case. Also documents the known, non-regressive limitation that the taskkill fallback (used only when job creation/assignment itself fails) can still miss a descendant that has reparented away from the tree it walks by PID. Addresses remaining PR Gitlawb#690 review feedback; PR description corrected separately to drop the outdated JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE claim.
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] Do not invoke
taskkillwith a PID thatWaithas already reaped
internal/config/process_windows.go:115
When creating or assigning the job fails (for example, in a non-nestable parent job),proc.jobstays zero. On the newWaitDelayand nonzero-exit paths,cmd.Wait()has already reapedcmd.exebeforeTerminatereaches this fallback, so its numeric PID can already have been recycled.taskkill /T /F /PIDcan then force-kill an unrelated process tree; this is precisely the stale-PID hazard the job path avoids above. Keep the fallback from using the PID after a completed wait (and use an identity-preserving cleanup path for the no-job case). -
[P2] Synchronize the Windows fixture before starting the one-second wait-delay race
internal/config/command_test.go:235
start /B powershellreturns before the child gets toSet-Content. On a cold or loaded Windows machine, the new job can be terminated by the one-secondWaitDelaybefore that write, after whichassertProcessTerminatedfails to readbg.pid. That makes the regression test flaky and can let it fail without ever proving that a pipe-holding child was started. Arrange for the fixture to publish the PID before the parent exits, or otherwise synchronize PID-file creation before exercising the wait-delay path.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Most of this landed exactly as asked: Terminate() now fires on any nonzero Wait result, the exit-7 background-child test proves the leak is closed, the job path no longer falls through to taskkill after TerminateJobObject, and surfacing resume failures instead of letting a suspended process masquerade as a 5s timeout is a nice touch. CI is green across the board, including the new tests on Windows.
One thing keeps me at request-changes, and it's the second half of my earlier ask — jatmn hit the same spot on this head. When job creation/assignment fails, Terminate() still hands the raw PID to taskkill /T /F on the WaitDelay/nonzero-exit paths (process_windows.go:123-125), where cmd.Wait() has already reaped cmd.exe. A dead root can't be tree-walked, so the call buys nothing there — and if Windows recycled the PID it force-kills an unrelated tree. Before this PR, taskkill only ever ran while the root was still alive, so this path is new exposure. Cheapest fix I see: skip the PID fallback whenever Wait has already returned (the caller knows which branch it's on), or keep the handle you already open in attachCommandProcess when assignment fails and check STILL_ACTIVE before touching the PID. The start /B fixture race jatmn mentioned is worth tightening while you're in there, but I'm not blocking on that one since CI is green.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/config/process_windows.go (1)
127-133: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a timeout to the
taskkillfallback execution.
exec.Command(...).Run()blocks indefinitely until the command completes. Whiletaskkill.exeusually completes quickly, underlying system issues (such as an unresponsive WMI or RPC service on Windows) can cause it to hang, which would indefinitely block the termination sequence on the request thread.Consider using
exec.CommandContextwith a short timeout to ensure the process tree termination gracefully falls back toTerminateProcessiftaskkillstalls.💡 Proposed refactor
- if err := windows.GetExitCodeProcess(p.processHandle, &exitCode); err == nil && exitCode == stillActive { - taskkill := taskkillPath() - _ = exec.Command(taskkill, "/T", "/F", "/PID", strconv.Itoa(p.cmd.Process.Pid)).Run() - } + if err := windows.GetExitCodeProcess(p.processHandle, &exitCode); err == nil && exitCode == stillActive { + taskkill := taskkillPath() + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + _ = exec.CommandContext(ctx, taskkill, "/T", "/F", "/PID", strconv.Itoa(p.cmd.Process.Pid)).Run() + }(Note: Ensure the
contextandtimepackages are imported.)🤖 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/config/process_windows.go` around lines 127 - 133, Update the taskkill fallback in the process termination method to use exec.CommandContext with a short context timeout, importing context and time as needed. Preserve the existing taskkill arguments and ensure the flow continues to windows.TerminateProcess even when taskkill times out or fails.
🤖 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/config/process_windows.go`:
- Around line 127-133: Update the taskkill fallback in the process termination
method to use exec.CommandContext with a short context timeout, importing
context and time as needed. Preserve the existing taskkill arguments and ensure
the flow continues to windows.TerminateProcess even when taskkill times out or
fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 767acdaf-31d1-41e7-a28a-505cf5c7cc40
📒 Files selected for processing (3)
internal/config/command_test.gointernal/config/process_windows.gointernal/config/process_windows_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/config/command_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] Do not signal a recycled POSIX process group after
Waitreaps the shell
internal/config/process_posix.go:26
The new unconditional error path callsTerminateonly aftercmd.Wait()has returned, which means the shell has already been reaped. On every ordinary nonzero provider-command exit, the unconditionalKill(-p.cmd.Process.Pid, SIGKILL)can therefore use a recycled numeric process-group ID; if another process has claimed that PID and formed a group before this call, Zero kills that unrelated group. The same window exists afterErrWaitDelaywhen a child has left the original group, and the new tests only cover children that remain in it. Preserve an identity/liveness guarantee for the group, or avoid the group-directed kill onceWaithas completed.
|
Addressed the remaining process-lifecycle feedback in Process safety
Additional requested fix
Validation
Please re-review the POSIX process-group identity fix. |
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/config/process_posix.go`:
- Line 33: Update the process-group setup around cmd.SysProcAttr to preserve any
attributes already configured on the exec.Cmd: reuse the existing SysProcAttr
when non-nil, initialize it only when absent, and then set Setpgid and Pgid
without replacing the struct.
🪄 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: b23708cd-9bbf-4e5f-aa60-034c2618bf30
📒 Files selected for processing (5)
internal/config/command.gointernal/config/process_posix.gointernal/config/process_posix_test.gointernal/config/process_windows.gointernal/tui/provider_wizard_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/config/process_posix_test.go
- internal/config/command.go
- internal/config/process_windows.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] Restore the credential-step footer assertion
internal/tui/provider_wizard_test.go:194
This test now expectsEnter/→ continue, but the credential step has no right-arrow action and the renderer returnsEnter continue ← back Esc close. The assertion therefore fails deterministically: Smoke (ubuntu-latest), Smoke (macos-latest), and Smoke (windows-latest) all fail on it. Restore the assertion to the current UI contract, or change the production shortcut and its rendering together if that is the intended product change.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Re-verified on the current head, and the good news first: every concern I raised is genuinely fixed. Nonzero-exit termination fires now, not just on ErrWaitDelay; the Windows job path no longer falls through to taskkill after TerminateJobObject; and the reaped-PID-to-taskkill hazard I flagged last round is closed the way we discussed, with the retained process handle plus a GetExitCodeProcess == STILL_ACTIVE gate before taskkill. The process-kill logic itself is ready.
The hold is CI: smoke is red on all three OSes, and it's a real failure this PR introduced, not a flake. TestProviderWizardAdvancesProviderAPIKeyAndModelSteps (provider_wizard_test.go:198) expects the key-step footer to contain "Enter/→ continue", but the wizard actually renders "Enter continue ← back Esc close". The test's expected string doesn't match what the footer prints, so it looks like a footer-string change got half-applied to the test. Fix the expectation (or the footer) so the suite goes green and I'll approve.
The credential step's footer only shows the /→ shortcut once a key is present, whether typed or inherited from the provider's AuthEnvVars. The previous commit changed the assertion to "Enter/→ continue" without accounting for that, so the test only passed by coincidence when ANTHROPIC_API_KEY happened to be set in the environment — failing deterministically on a clean CI runner (as smoke reported on all three OSes). Pin the env var empty and assert the no-key footer text so the test is correct regardless of ambient environment. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Review: PR #690 — fix(config): kill provider-command process tree via job object
Verdict: Approve. The core fix is correct and load-bearing — the retained POSIX group anchor and the Windows job object give Terminate() a stable handle on the whole tree even after cmd.Wait() has reaped the root, which is exactly the leak the PR targets. One minor behavioral regression is worth a conscious accept/decline, but nothing here blocks merge.
What the fix does well
cmd.WaitDelay = time.Second(internal/config/command.go:47) is what makescmd.Wait()unblock ~1s after the shell exits instead of hanging on descendant-held pipes until the 5s timer — verified load-bearing:TestLoadProviderCommandTerminatesBackgroundChild(asleep 10 & exitscenario) asserts return in <4s and that the leaked child PID is killed; that test fails on base (base has no WaitDelay, so it only unblocks at 5s).- The POSIX anchor process (internal/config/process_posix.go:20-43) deliberately does not trust the reaped shell PID to name the group — the comment at line 17-19 spells out why, and
Terminate()SIGKILLs-groupID. - Windows
Terminate()(process_windows.go:111-145) correctly refuses to fall through to ataskkill /T /PIDagainst a possibly-reused PID once the root is reaped, gating the fallback on a liveSTILL_ACTIVEhandle. The CREATE_SUSPENDED-then-assign-to-job sequencing (line 23-25, 45-79) closes the escape window for fast descendants. Good defensive commenting throughout.
[Minor] WaitDelay=1s turns a previously-successful command into a spurious timeout — PR-introduced
internal/config/command.go:47 (surfaces at :79-81 and :17-21)
Concrete scenario — a provider command that prints valid JSON then backgrounds a helper without redirecting its stdio and keeps it alive 1–5s:
echo '{"name":"x","provider":"openai","apiKey":"sk","model":"gpt-4"}'
sleep 2 &
exit 0- Base (no
WaitDelay): the JSON is buffered, the shell exits 0,cmd.Wait()drains the inherited pipes untilsleep 2exits (~2s, under the 5s timer), returns nil, andLoadProviderCommandparses the buffered JSON → success. - PR:
WaitDelayfires 1s after the shell exits, force-closes the pipes,cmd.Wait()returnsexec.ErrWaitDelay; command.go:79-81 maps that toerrProviderCommandTimeout, andLoadProviderCommand(line 17-21) discards the already-captured stdout and returns"provider command timed out after 5s"— a config load that worked before now fails (and the helper is SIGKILLed).
This only bites commands that leave an stdio-inheriting descendant alive for 1–5s; anything that redirects (daemon >/dev/null 2>&1 &) is unaffected, which is why this reads as an intended bound on drain time rather than an oversight. Two things worth noting if you accept it: (1) the message says "timed out after 5s" when the cut actually happens at ~1s, which is misleading; (2) on the ErrWaitDelay path stdout already holds valid JSON — parsing it before returning the timeout would preserve base success behavior for this case. Either fix the message or attempt the parse; otherwise, an explicit acknowledgement that this changes the success contract is enough.
Tests
gofmt -lclean on all 7 changed files;go buildandgo vetoninternal/config/...andinternal/tui/...both exit 0.go test -race ./internal/config/...: ok. All PR tests pass, includingTestLoadProviderCommandTimeout,TestLoadProviderCommandTerminatesBackgroundChild{,OnFailure},TestCommandProcessPreservesSysProcAttr,TestCommandProcessRetainsGroupIdentityAfterProviderWait. Windows-only files are build-tag excluded on darwin.internal/tuipackage reports FAIL, but solely fromTestHandleAddDirCommand(add_dir_test.go:43) — this is a pre-existing environmental sandbox failure: it fails identically on the clean base worktree with the same error (the sandbox already has/private/tmp+ the macOS temp dir in its write roots) and is unrelated to the diff. The one tui test the PR actually touches,TestProviderWizardAdvancesProviderAPIKeyAndModelSteps, passes on both head and base. Not PR-attributable.
Merge is kevin's call per the program gate.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Approving on 467ff55. Our only hold last round was CI (the provider-wizard credential-step footer assertion), and that is green now across macos, ubuntu, and windows.
The delta since then is clean: the footer test is fixed, and process_posix.go now sets Setpgid/Pgid on the existing SysProcAttr rather than replacing the whole struct, so any attributes a caller already set are preserved. The Windows process-kill logic we worked through last round is unchanged, still gated on the retained process handle plus a GetExitCodeProcess == STILL_ACTIVE check before taskkill.
Good to go, thanks for the thorough back-and-forth. LGTM.
Summary
Fixes #683 —
TestLoadProviderCommandTimeoutcould take ~10.2s on Windows because the 5s provider-command timeout didn't reliably kill the process tree.taskkill /Twalks the process tree by parent PID in user space, so it can miss descendants (and depends ontaskkill.exebeing runnable in the environment). When the sleeping PowerShell child survived, it kept the inherited stdout/stderr pipe write ends open, andcmd.Wait()blocked until the child completed naturally.Changes
CREATE_SUSPENDEDand assigned to a job object before its main thread resumes, so a fast command can't spawn and detach a descendant before containment is in place. On timeout,TerminateJobObjectkills every member of the tree atomically. Note: the job does not setJOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE— releasing the handle on the success path intentionally leaves a provider command's detached helpers running, matching pre-job-object behavior; termination only happens explicitly viaTerminate()on timeout/error.taskkill /T /F+Process.Kill()remain as a fallback for the rare case where job creation/assignment fails, and are skipped when a job successfully contained the tree (to avoid acting on a PID thatcmd.Wait()may have already reaped and that Windows could have reused).cmd.WaitDelay = 1s: bounds pipe draining after process exit, soWaitcan't hang on pipe handles held by an escaped orphan.Terminate()now runs for any error fromWait(not justexec.ErrWaitDelay), since a nonzero-exit shell (e.g.sleep 600 & exit 7) also needs its leaked background child cleaned up.LoadProviderCommandreturns (Windows and POSIX), including a nonzero-exit background-child variant and aresumeMainThreadfailure-reporting test.Known limitation
If job creation or assignment itself fails, the
taskkill /Tfallback can still miss a descendant that has reparented away from the tree it walks. This is not a regression versus pre-job-object behavior (taskkill was the only mechanism then too) and is tracked as a follow-up rather than blocking this fix.Verification
go build ./...— passgo vet ./internal/config/...— passgo test ./internal/config/...— pass, including the new nonzero-exit background-child test and theresumeMainThreadfailure-reporting testSummary by CodeRabbit