fix(coding-agent): survive an unresolvable Windows taskkill - #807
fix(coding-agent): survive an unresolvable Windows taskkill#807yeongjunyoo wants to merge 5 commits into
Conversation
`killProcessTree` wrapped `spawn("taskkill", ...)` in a try/catch, but spawn
reports a failed executable lookup asynchronously on the child's `error` event.
With no listener Node re-emitted it as an uncaught exception, so a session whose
PATH had lost `%SystemRoot%\System32` died on shutdown with
`Error: spawn taskkill ENOENT` instead of exiting -- and the tracked child was
never killed.
Resolve the absolute `%SystemRoot%\System32\taskkill.exe`, handle the `error`
event, fall back to `process.kill(pid)` so the target still dies, and `unref()`
the detached killer. This matches the launcher handling already used in
`utils/open-browser.ts`. The Node harness in `packages/agent` carried an
identical copy and gets the same fix.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: efdb1c142e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| killer.once("error", () => { | ||
| killProcessDirectly(pid); |
There was a problem hiding this comment.
When taskkill is unresolvable during an immediate-exit path, such as emergencyTerminalExit() calling killTrackedDetachedChildren() immediately before process.exit(129), this error callback cannot run because spawn errors are delivered on a later event-loop turn and process.exit() terminates synchronously. The listener prevents an uncaught ENOENT when the event loop continues, but in this shutdown path the direct child remains alive; make the kill operation awaitable or perform a synchronous/direct fallback before exiting.
AGENTS.md reference: packages/coding-agent/AGENTS.md:L57-L57
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and confirmed by measurement — fixed in dafbbe4.
The error listener stopped the uncaught exception but did nothing for
emergencyTerminalExit(), which runs killTrackedDetachedChildren() and then
process.exit(129) in the same tick. The tracked child stayed alive there.
Both copies now use spawnSync under a 5s timeout instead of spawn:
spawnSyncreports a failed executable lookup on its returnederrorfield rather than
emitting it, so ENOENT still cannot become an uncaught exception — no listener needed.- The
process.kill(pid)fallback runs before the function returns, so an immediate
process.exit()cannot outrun it. - A
nullstatus (timeout-killed launcher) also takes the fallback; a real taskkill exit code
counts as handled.
Blocking is intentional and bounded: every caller is a teardown path
(killTrackedDetachedChildren, the harness dispose/abort paths), and the timeout caps the wait.
Measured on Windows 11 / Node v24.18.0 with PATH cleared and SystemRoot pointed at a directory
without System32\taskkill.exe, running the real shutdown chain and probing the child
synchronously right after the kill returns:
--- main ---
immediately after the synchronous kill: child alive=true
FAIL uncaughtException: spawn taskkill ENOENT
--- efdb1c1 (async listener, the version you reviewed) ---
immediately after the synchronous kill: child alive=true <- your finding
PASS: no uncaught exception
--- dafbbe4e (spawnSync) ---
immediately after the synchronous kill: child alive=false
PASS: no uncaught exception
Pinned by a new unit test in both packages, issues the fallback kill synchronously, before the caller can exit: it swaps process.kill for a recorder for the duration of the call and asserts
the kill was already issued when the call returned. Against the async version it fails with
expected [] to deeply equal [ [ 4242, undefined ] ].
Review follow-up: `emergencyTerminalExit()` runs `killTrackedDetachedChildren()` and then `process.exit(129)` in the same tick, so a fallback wired to the killer's asynchronous `error` event never ran and the tracked child survived whenever `taskkill` was unresolvable. Switch both copies to `spawnSync` under a 5s timeout. `spawnSync` reports a failed executable lookup on its returned `error` field instead of emitting it, so ENOENT still cannot become an uncaught exception, and the `process.kill(pid)` fallback now runs before the caller returns.
|
@codex review — the implementation changed materially since the last pass. Per your P2 finding, the Windows kill is now synchronous ( |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dafbbe4ebf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -0,0 +1,110 @@ | |||
| import { spawn } from "node:child_process"; | |||
There was a problem hiding this comment.
This issue-regression test was added at the test root, but packages/coding-agent/test/AGENTS.md requires issue regressions to use suite/regressions/<issue>-<slug>.test.ts; relocate and rename it so the coverage remains discoverable and follows the owning suite's convention.
AGENTS.md reference: packages/coding-agent/test/AGENTS.md:L40-L40
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in 7d93d1b. Filed the crash as #812 so the mandated <issue>-<slug> name is accurate, and moved the file to
packages/coding-agent/test/suite/regressions/issue-812-windows-taskkill-enoent.test.ts.
The packages/agent copy stays at test/harness/kill-process-tree-windows.test.ts; that package's tests are not
covered by packages/coding-agent/test/AGENTS.md and it has no suite/regressions tree.
| if (taskkillHandledTree(pid, taskkillPath)) return; | ||
| killProcessDirectly(pid); |
There was a problem hiding this comment.
When taskkill is unavailable and the tracked Windows shell has spawned grandchildren, this fallback terminates only the direct shell before callers such as emergencyTerminalExit() immediately exit, leaving the grandchildren orphaned. The repository's Windows PTY implementation explicitly documents this process.kill/TerminateProcess behavior in packages/pty/src/pipe-fallback.ts:242-246; use a synchronous tree-capable fallback rather than treating the direct-child kill as complete process-tree cleanup.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right that a direct process.kill is not process-tree cleanup — fixed in 7d93d1b, though not by adding a
second tree-capable launcher.
The reported failure was PATH resolution, not a missing taskkill.exe. So the real defect your comment exposes
is that a broken PATH silently downgraded a tree kill to a direct kill. windowsTaskkillCandidates() now returns
every absolute System32 / Sysnative taskkill.exe that actually exists under SystemRoot, SYSTEMROOT,
windir, or SystemDrive, followed by the bare PATH-resolved name, and killWindowsProcessTree tries each until
one launches. existsSync filters the list first, so the normal case still runs exactly one spawnSync.
Measured with PATH cleared and SystemRoot pointed at a directory with no System32\taskkill.exe:
[A: PATH cleared, SystemRoot bogus, SystemDrive intact]
candidates: ["C:\\Windows\\System32\\taskkill.exe","taskkill.exe"]
alive immediately after the synchronous kill: false child exited code=134 <- real taskkill /T
[B: no launcher reachable at all]
candidates: ["taskkill.exe"]
alive immediately after the synchronous kill: false child exited code=1 <- degraded direct kill
Scenario B is the only path that still reaches process.kill, and I did not find an honest way to make it
tree-capable: enumerating Windows descendants requires an external tool (taskkill, wmic, PowerShell), all of
which live in the same System32 that is unreachable by definition in that state, and wmic is gone on current
Windows 11. So it is documented as degraded, citing the pipe-fallback.ts:242-246 note you pointed at, rather
than pretending to be complete cleanup. If you would rather have it hard-fail loudly there instead of best-effort,
say so and I will change it.
Writing this also surfaced a real bug in my own candidate list: an empty SystemDrive produced \Windows\...,
which Windows resolves drive-relative and silently reintroduced a candidate the environment never declared. Fixed,
with coverage.
The P1 is addressed too: the regression moved to
packages/coding-agent/test/suite/regressions/issue-812-windows-taskkill-enoent.test.ts, filed as #812 so the
<issue>-<slug> name is accurate.
There was a problem hiding this comment.
Correction to one number I posted above, before anyone relies on it.
In the scenario-A transcript I quoted alive immediately after the synchronous kill: false. Re-running it after
merging origin/main (097bbef) that probe came back true, with the child exiting 134 a moment later. The probe
is racy on the taskkill path by nature: taskkill /F /T returns once termination has been requested, so the target
can still be reported alive right after the call. It is only decisive on the direct-kill path (scenario B), where we
issue TerminateProcess ourselves.
Nothing about the fix changes — the invariant your comment was about is "the kill is issued before the call returns",
and that is pinned deterministically by issues the fallback kill synchronously, before the caller can exit, which
swaps process.kill for a recorder and asserts it was already called on return. The repro script now says so
instead of implying the immediate probe is a guarantee.
Also merged origin/main into the branch: the only conflict was both sides adding a top entry to
packages/agent/src/changes.md, resolved by keeping both with upstream's newer entry first. npm run check clean,
scoped tests green (coding-agent 9, agent 4).
Review follow-up. The direct `process.kill` fallback maps to TerminateProcess and leaves descendants orphaned, so it must not be reached just because PATH cannot resolve `taskkill` -- which is exactly the reported failure. `windowsTaskkillCandidates()` now returns every absolute `System32`/`Sysnative` `taskkill.exe` that exists under `SystemRoot`, `SYSTEMROOT`, `windir`, or `SystemDrive`, followed by the bare PATH-resolved name. `killWindowsProcessTree` tries each until one launches, so the real tree kill survives any PATH breakage; the direct kill is now reached only when no launcher exists at all and is documented as degraded. Also moves the coding-agent regression into `test/suite/regressions/issue-812-windows-taskkill-enoent.test.ts` as `packages/coding-agent/test/AGENTS.md` requires. fixes code-yeongyu#812
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…enoent # Conflicts: # packages/agent/src/changes.md
1 participant
fixes #812
Problem
On Windows the CLI can die on exit instead of exiting:
Reported against
omo-ai@5.0.0-0.beta.5, which ships@code-yeongyu/senpi@2026.8.11-2.The
spawnargsidentify the call site exactly:killProcessTreeinpackages/coding-agent/src/utils/shell.ts, reached fromkillTrackedDetachedChildren()in the interactive-mode signal handler.
Two independent defects:
try/catchis dead code.child_process.spawn()never throws on a failedexecutable lookup; it reports ENOENT asynchronously on the child's
errorevent. With nolistener, Node re-emits it as an uncaught exception and takes the process down.
taskkillis resolved through PATH. Any session whose PATH lost%SystemRoot%\System32— a POSIX-style PATH inherited from a Git Bash/MSYS launcher, atruncated user PATH, a locked-down service account — cannot resolve it. The tracked child
is then never killed either, because there is no fallback.
#792 fixed the same class of bug in
packages/pty/src/pipe-fallback.ts(taskkill.exeplus anerrorlistener). These two copies were missed:packages/coding-agent/src/utils/shell.ts—killProcessTree(the crash in the report)packages/agent/src/harness/env/nodejs.ts— an identical private copy in the Node harnessReview follow-up (dafbbe4)
Codex flagged that an
error-event fallback still cannot run onemergencyTerminalExit(), which callskillTrackedDetachedChildren()and thenprocess.exit(129)in the same tick. Confirmed and fixed: both copiesnow use
spawnSyncunder a 5s timeout, so theprocess.kill(pid)fallback is issued before the call returns.spawnSyncalso reports a failed lookup on its returnederrorfield instead of emitting it, so ENOENT stillcannot become an uncaught exception.
Measured with PATH cleared and
SystemRootpointed at a directory withoutSystem32\taskkill.exe, probing thechild synchronously right after the kill returns:
Pinned by
issues the fallback kill synchronously, before the caller can exitin both packages.Review round 2 (7d93d1b)
P2, orphaned descendants. The direct kill must not be reached just because PATH cannot resolve
taskkill—which was the reported failure.
windowsTaskkillCandidates()now returns every absoluteSystem32/Sysnativetaskkill.exethat exists underSystemRoot,SYSTEMROOT,windir, orSystemDrive, then the barePATH-resolved name;
killWindowsProcessTreetries each until one launches.existsSyncfilters first, so thenormal case still runs exactly one
spawnSync.Only the second state reaches
process.kill, and it cannot be made tree-capable in-process: enumerating Windowsdescendants needs an external tool that lives in the same unreachable
System32(andwmicis gone on currentWindows 11). It is documented as degraded, citing the same limitation
pipe-fallback.ts:242-246records.Writing that check found a bug in my own candidate list: an empty
SystemDriveproduced\Windows\..., whichWindows resolves drive-relative and silently reintroduced an undeclared candidate. Fixed, with coverage.
P1, test location. Filed the crash as Windows: CLI shutdown crashes with uncaught spawn taskkill ENOENT #812 and moved the regression to
packages/coding-agent/test/suite/regressions/issue-812-windows-taskkill-enoent.test.ts.Scoped runs after the change: coding-agent 31 passed | 4 skipped, agent 4 passed,
npm run checkclean.Fix
Both copies now:
%SystemRoot%\System32\taskkill.exe(SystemRoot/SYSTEMROOT/windir) and keepthe bare
taskkill.exeonly as a last resort,killer.once("error", ...)and fall back toprocess.kill(pid)so shutdown stillterminates the target,
unref()the detached killer so a fire-and-forget kill cannot hold the event loop open during exit.This is the pattern
packages/coding-agent/src/utils/open-browser.tsalready uses(
.on("error", () => {}).unref()), so no new convention is introduced. The two harnesses keepindependent copies, as they already do for
getShellEnvand bash resolution; unifying them wouldmean widening
@earendil-works/pi-agent-core's public surface for a platform detail.resolveWindowsTaskkillPathandkillWindowsProcessTreeare exported so the failure is testablewithout mocking the module registry (the repo has no
mock.moduleusage).Verification
resolveWindowsTaskkillPathand the ENOENT fallback are covered on every platform: the tests pass adeliberately unresolvable launcher name, so
spawnfails the same way on Windows, macOS, and Linux.Regression proof. Removing only the
killer.once("error", ...)handler makes the new test failwith the exact reported payload:
End-to-end repro through the real shutdown chain (
killTrackedDetachedChildren→killProcessTree), with PATH cleared andSystemRootpointed at a directory withoutSystem32\taskkill.exe, on Windows 11 / Node v24.18.0:Pre-existing failures.
packages/agent $ npx vitest --run test/harness/on Windows fails5 files / 15 tests both with and without this change (reverting only
packages/agent/src/harness/env/nodejs.tstomaingives 6 files / 17 tests, the extra one beingthis branch's new test importing helpers that do not exist there). Those are Windows path-separator
and
ignore-package failures inskills.test.ts,sqlite-node.test.ts, andtools.test.ts,unrelated to process termination.
QA evidence is saved under
local-ignore/qa-evidence/20260811-windows-taskkill-enoent/.changes.mdfork-tracker entries and[Unreleased]changelog entries are included for both packages.