Repository navigation
fix(cli): track Windows background jobs and runner identity natively - #14857
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (8 files)
Fix these issues in Kilo Cloud Previous Review Summaries (4 snapshots, latest commit a5ee128)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit a5ee128)Status: No Issues Found | Recommendation: Merge The incremental commits after the previous review ( Files Reviewed (4 files)
Previous review (commit d285bd8)Status: No Issues Found | Recommendation: Merge The one remaining issue from the previous review is resolved. The outer return in Files Reviewed (1 file)
Previous review (commit b332eb6)Status: 1 Issue Found | Recommendation: Address before merge The prior review's 7 findings are all resolved in this commit: the control file is now removed only after the fallible Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (4 files)
Fix these issues in Kilo Cloud Previous review (commit 37f81b1)Status: 7 Issues Found | Recommendation: Address before merge Overview
The Windows job-object approach is a sound fix for the PID-reuse/orphan problems this PR targets, and the FFI signatures, struct offsets, and PID-reuse guard in Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (7 files)
Reviewed by deepseek-v4.1-flash · Input: 112.9K · Output: 36.7K · Cached: 1.7M Review guidance: REVIEW.md from base branch |
…ssful process walk
| return watch().catch((err: unknown) => { | ||
| if (failure !== undefined && err === failure) throw err | ||
| note(`job object failed, polling the process table instead: ${String(err)}`) | ||
| return windows(input, child, done, start) |
There was a problem hiding this comment.
I think the job-object approach is the right direction. Can we preserve the original leader exit state when an established job falls back to WMI?
A healthy job should not normally fail midway. This is separate from startup fallback when FFI cannot load or a job cannot be created/assigned. However, this catch covers filesystem errors while checking/removing the .stop file as well as job-query or catchable FFI exceptions. A file-operation failure can therefore reach this handoff even if the job is healthy. A stop timeout itself does not switch to WMI, and a native crash cannot be caught this way.
If the leader already exited, windows() attaches a new handler to the settled done promise and sets exited = Date.now(), rather than preserving the original exit time. Its first walk also runs before that callback, with no exit bound. After PID reuse, unrelated children can enter seen and later be passed to taskkill.
I reproduced the incorrect adoption with the actual walk() and process-row fixtures, not by triggering a native failure on Windows. I would carry the original exit state into the fallback and add a regression test for failover after leader exit and PID reuse.
There was a problem hiding this comment.
yeah both are real. They come from the runtime handoff I added so a failing job doesnt leave the command unmanaged. I can carry the leader's real exit time and the last members the job reported into windows(), keep .stop file errors out of the handoff, and add your two cases as regression tests. Or I drop the runtime handoff and keep polling the job when a query fails, since a healthy job shouldnt fail midway, which leaves less code on the path that kills processes. Which do you prefer?
There was a problem hiding this comment.
went with dropping the runtime handoff in cd907bc, its less code on that path. Once the runner is in its job it stays there, a query or kill that throws gets retried with one note and a stop the job cant confirm ends as "did not stop safely". So once the job exists nothing walks the process table anymore. The new live test breaks the job's query and kill mid-run and checks that, and it fails on d285bd8. The startup fallback still walks, and it had the exit-time race WebReflection found on #14796, a5ee128 reads the exit after the query there too. If youd rather keep the handoff I can do that instead, its quite a bit more code though
| const GRACE = 1_000 | ||
|
|
||
| async function windows(input: Input, child: ReturnType<typeof spawn>, done: Promise<number>) { | ||
| async function windows(input: Input, child: ReturnType<typeof spawn>, done: Promise<number>, start: number) { |
There was a problem hiding this comment.
Can we preserve verified descendant identities across job-to-WMI failover instead of starting with an empty seen map?
This is another concern limited to runtime failover, not startup fallback. A healthy job should not normally fail, but the caller catches .stop filesystem errors as well as job-query exceptions, so the transition is not limited to a broken job object.
A detached grandchild can remain in the job after both the leader and its immediate parent have exited. A fresh WMI walk cannot reconnect it to the leader because the intermediate parent is gone. With no retained membership, the fallback can conclude seen is empty and exit the runner while the grandchild keeps running, including when handling a stop.
I reproduced the lost descendant with the actual walk() and process-row fixtures: an empty map misses it, while retained identity information keeps it tracked. I have not forced this runtime failover on Windows. I would carry trusted member identities into the fallback, or keep ownership uncertain rather than report completion if they cannot be recovered. A regression test with an exited intermediate parent and a surviving detached grandchild would cover this.
There was a problem hiding this comment.
yes, the job knows its members until it fails, so the walk can start from the last list it got instead of an empty map. Same change as in the other thread, unless the handoff goes away
There was a problem hiding this comment.
the detached grandchild is that live test now. Leader and intermediate parent exit, query and kill start failing, the runner keeps guarding and a stop comes back unconfirmed, and once the job answers again a stop ends the grandchild
There was a problem hiding this comment.
The startup process-walk fallback still cannot reconnect a grandchild whose parent exits between two walks; that older limitation is unchanged
github-actions
Bot
added
area:cli
mardausdennis
changed the title
…dows-job-runner-stop
Issue
Refs #14793, #12584 and #14437
Context
Windows runners repeatedly launch PowerShell to enumerate descendants, and serve launches it again to verify persistent runner identities. The retained 40s A/B result on a loaded i9-11900K found 0.90 PowerShell CPU cores for one idle job and 2.12 cores total for four jobs. The native path started zero PowerShell processes during those running-job windows; this is an idle measurement, not a guarantee about startup or fallback
This branch contains #14796 at
fde4234371through a merge commit, rather than sharing its base. The birth-checked exact-PID stop is part of the stack, so neither stop path asks taskkill to traverse/tparent linksIf #14796 is squash-merged, this branch gets rebased onto main before it is merged
Implementation
The runner assigns its live leader to a Windows Job Object, counts membership natively, and terminates that job on stop. Detached descendants remain tracked until they exit and are included in stop. Closing the runner does not use
KILL_ON_JOB_CLOSEIf startup assignment fails, the process walk is used. Once assigned, transient job failures keep the runner guarding the job, with a correlated warning, rather than replacing it with an incomplete process walk. A native job that never answers again can keep its runner alive; it cannot safely claim completion. In the startup fallback, failed or malformed walks cannot authorize cached descendant kills. Post-exit confirmation has a 30s window with 5s per-walk caps, so a stop request is checked between walks. Its final warning distinguishes unavailable process information from verified descendants still running. A grandchild whose parent exits between startup-fallback walks can still be missed; that older polling limitation is unchanged
Serve reads runner identity through
NtQueryInformationProcess;STATUS_PROCESS_IS_TERMINATINGmeans wait for exit, not fall back to PowerShell. Windows persistent stop and startup rollback use a separate 23s confirmation window, based on measured OLD runners taking 4.6 to 17.6s and native runners about 0.1s; startup fallback was independently observed up to 8.8sProcess exit and an HTTP stop can finish together. They now share one persistent-artifact cleanup promise, so the manifest, log and control file are each removed once instead of racing into
rmerrorsAn unconfirmed stop request raises an error, the runner exits 1, and the job output records the cause; the existing manager can still label that terminal row
stopped. Its warning names the leader if it survives, alongside any verified descendantsScreenshots / Video
N/A, no UI change
How to Test
Manual/local verification
Run by an agent on my machine, Windows 11
The native terminating and five-second cooperative-stop tests fail before the fix and pass after it. The rollback test verifies that a failed start waits for its runner to end before returning the original identity error. Process-row tests reject stale-parent adoption, forbid
/t, and cover children that appear during stopThe seven Windows background-process files were run serially. The runner, native status, handoff, monitor, shutdown and tool cases pass. One manager cleanup case hit its 23s stop window under the current load, then passed when rerun alone. Opencode typecheck, touched-file lint and formatting pass. The native handoff fixture added by this PR now allows 20s for a condition, above the measured 12s timeout under load and below its 35s self-kill. The manager outcome test fails when its runner exit is forced to 0 and cleans up its own child even when an assertion fails before stop
Reviewer test steps
packages/opencode, runbun run script/test-runner.ts background-processbun test ./test/kilocode/background-process-windows-handoff.test.tson Windows for job-query, stop and control-file failure casesBlocked checks and substitute verification
Windows is the local proof target; Linux/macOS are covered by CI. The loaded-machine suite uses an external test watchdog where a pre-existing deadline is too short. Real PID reuse was not reproduced locally, so the stale-edge tests prove the acceptance and target-selection rules rather than a real-world kill incident
Checklist