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 3 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>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
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 that timed-out commands and detached child processes exit promptly. ChangesProvider command termination
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 | ✅ 5✅ Passed checks (5 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.
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
cmd.Start(), the process is assigned to a job object withJOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE; on timeout the job is terminated, so the kernel kills the entire tree atomically.taskkill /T /F+Process.Kill()remain as a fallback for the rare case where job assignment fails or a descendant was spawned before assignment.cmd.WaitDelay = 1s: bounds pipe draining after process exit, soWaitcan never hang on pipe handles held by an escaped orphan.LoadProviderCommandreturns (both Windows and POSIX).Verification
On Windows (go1.26.5 windows/amd64):
go test ./internal/config -run '^TestLoadProviderCommandTimeout$' -count=1 -v→ PASS in 5.3s, including the new no-leftover-child assertion.go build ./...andgo test ./internal/configpass.Note: I couldn't reproduce the original 10.2s failure locally (taskkill happens to work here), but the job object removes the user-space tree-walk race entirely and the new assertion would catch any regression.
🤖 Generated with Claude Code
Summary by CodeRabbit