Skip to content

fix(process-host): let a slow Windows taskkill finish the tree instead of abandoning it - #28573

Closed
nwparker wants to merge 1 commit into
mainfrom
nwparker/fix-win-taskkill-tree-cap
Closed

nwparker wants to merge 1 commit into
mainfrom
nwparker/fix-win-taskkill-tree-cap

Conversation

@nwparker

@nwparker nwparker commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 2 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​36 $\color{#cf222e}{\Huge{\mathbf{−}}}$​2 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​34
Prod 1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​6 $\color{#cf222e}{\Huge{\mathbf{−}}}$​1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​5

ELI5

When Orca stops a command on Windows, it asks Windows' taskkill tool to end the command and everything it started. On a busy machine taskkill can take several seconds. Orca only gave it 2 seconds, then gave up, killed just the top process, and left its children running. This lets taskkill finish.

Draft. Rated MEDIUM by review, so it is not being merged automatically. See "Open design question" below.

What Changed

  • src/packages/process-host/src/process-tree-termination.ts: the Windows taskkill /pid X /t /f call gets its own 15 s bound (WINDOWS_TASKKILL_TIMEOUT_MS), up from the shared 2 s SUBPROCESS_TIMEOUT_MS. POSIX probing keeps 2 s.
  • Two unit tests:
    • a taskkill that finishes at 7 s is awaited and reports the tree as verified (this fails against the old 2 s cap);
    • a wedged taskkill is still killed at 15 s, and the root is still killed.
  • src/main/ipc/preflight-runnable-local-cli.test.ts, the "hung batch launcher" test:
    • Its timeout is now 30 s: 5 s probe deadline, plus taskkill, plus the next probe.
    • Its fixture child no longer exits on its own after 30 s, so "the child is dead" can only pass if Orca actually killed it.

Why

Symptom: package (windows) failed 8 times in about 1,000 pr.yml runs (about 20 h) on "Windows preflight batch shims › recovers after a hung batch launcher and terminates its Node child". Every failure was a 15 s timeout; passing runs take 5.3 s. This was the single most frequent Windows CI flake.

Root cause, measured on real GitHub runners (diagnostic workflow dispatch, runProcess({ terminationBarrier: true }) on a .CMD that starts a hanging node child, runner loaded with 2× CPU-core burners):

runner taskkill cap barrier settles after node grandchild
windows-2022 2 s (today) timeout + ~12.5 s alive 8/8
windows-2022 15 s timeout + ~18 s killed 8/8
windows-11-arm 2 s or 15 s timeout + ~0.5 s killed 8/8

taskkill alone took 4.6–7.7 s under that load on windows-2022, against about 50 ms idle.

With the 2 s cap, the sequence is: taskkill is killed before finishing, the root is killed by handle, and the barrier reports "unverified". The barrier then waits its 10 s unverified grace and settles anyway, while the grandchild is still running. So on a loaded Windows machine every barrier caller pays an extra ~12 s, and the descendants it was meant to kill survive. The CI test caught exactly that: the 5 s probe plus the ~12.5 s unverified path is longer than its 15 s cap.

Open design question (why this is a draft)

The adversarial review rated this MEDIUM:

  • 15 s is below the observed 18 s. It is a mitigation, not a proven completion bound.
  • Worst case for a boolean barrier caller becomes timeout + 25 s (15 s taskkill plus the 10 s unverified grace), up from timeout + ~12 s.
    • Some callers await cleanup in sequence. The OpenCode launch probe runs two probes, so it can add up to about 30 s under load.
    • The waits are async, so they don't block the main thread. runProcessSync is unaffected.
  • The better design separates "how long the caller waits" from "how long cleanup runs":
    • let one tracked taskkill run to completion after the caller stops waiting, and deduplicate repeated termination requests;
    • report unfinished cleanup as unverifiable, and keep git admission held until cleanup is confirmed;
    • longer term, owned Job Objects remove the tree walk entirely.

Decision needed: take this bound now as a mitigation, or do the decoupled design.

Linked Issue

N/A (maintainer PR; CI reliability with a product defect behind it).

Visual Proof

N/A: process termination behavior, no UI.

Testing

  • vitest run src/packages/process-host/src/ passes locally (124 tests). The new 7 s taskkill test fails with the old 2 s cap.
  • Real-runner measurements above (windows-2022, windows-11-arm), from a dispatch-only diagnostic branch that is not part of this PR.
  • Automated tests added/updated

AI Disclosure

Review

Agent skill upstream boundary

  • Not applicable, or this change copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

  • SSH: no wire change. Only the executing host's Windows termination policy changes; POSIX hosts are unchanged.
  • WSL: WSL git uses its own guest-group barrier and does not go through taskkill.
  • Daemon: the daemon/PTY and git-runner termination paths have separate taskkill code that this PR does not touch.

Checklist

  • This PR is small and focused
  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

…d of abandoning it

Under CPU load taskkill /t took 4.6-18 s on windows-2022; the 2 s cap killed it,
fell back to the root, and left the grandchild alive in 8/8 runs. Raise the cap
to 15 s, and keep the preflight fixture alive so it can't pass by exiting.

Co-Authored-By: Claude <noreply@anthropic.com>
@nwparker
nwparker force-pushed the nwparker/fix-win-taskkill-tree-cap branch from 06676c9 to 7cecbaf Compare October 14, 2026 05:39
@nwparker

Copy link
Copy Markdown
Contributor Author

Closing: #28577 already landed the same mitigation (a separate Windows taskkill cap, now 5 s; the batch-shim test at 30 s with a longer-lived child).

Keeping the measurements here for whoever revisits it. These are real GitHub runners with 2× CPU-core burners, running runProcess({ terminationBarrier: true }) on a .CMD that starts a hanging node child:

runner taskkill cap barrier settles after node grandchild
windows-2022 2 s timeout + ~12.5 s alive 8/8
windows-2022 15 s timeout + ~18 s killed 8/8
windows-11-arm 2 s / 15 s timeout + ~0.5 s killed 8/8

Bare taskkill /t /f took 4.6–7.7 s under that load (about 50 ms idle), so 5 s still sits inside the observed tail. On a heavily loaded machine the barrier can still abandon the walk and leave descendants running.

The more durable fix, from the review of this draft:

  • separate how long the caller waits from how long cleanup runs: let one tracked taskkill finish after the caller stops waiting, and deduplicate termination requests;
  • keep git admission held until cleanup is confirmed;
  • longer term, owned Job Objects.

@nwparker nwparker closed this Oct 14, 2026
@nwparker
nwparker deleted the nwparker/fix-win-taskkill-tree-cap branch October 14, 2026 06:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant