Repository navigation
Conversation
0xsline
left a comment
There was a problem hiding this comment.
Thanks, this fixes the hang, and registering the exit listener before the read loop is the right call.
One gap before we merge: on Windows the real Claude Code CLI still survives Stop. child.kill() ends cmd.exe, and closing our ends of the pipes lets the turn settle, but the grandchild keeps running. Your own probe shows grandchildAlive= true, and this patch doesn't change that. So after the user presses Stop (or the turn times out), the CLI can keep executing tool calls and editing the project in the background.
Could you terminate the whole process tree on win32? For example, run taskkill /pid <child.pid> /T /F (with windowsHide: true) in onAbort instead of relying on child.kill() there, and keep child.kill() for other platforms. Please also extend the abort case in the probe or verify to assert the grandchild is gone after abort, not only that the turn settles.
Happy to merge once the tree is killed.
\child.kill()\ ends the cmd.exe that \claudeCodeCommand()\ spawns for a .cmd shim, but the CLI itself is that cmd.exe's child and keeps running: it holds the stdout write end open and can keep editing the project after the user pressed Stop. Use \ askkill /pid <pid> /T /F\ on win32 and keep \child.kill()\ everywhere else. Add turn-runner-abort.verify.ts, which runs the real turn against a fake .cmd that starts a long-lived grandchild, aborts the turn, and asserts both that it settles and that the grandchild is gone. It is wired into verify:server-runs and is a no-op off Windows.
|
Fixed in a598ce3.
Against the previous |
What breaks
On Windows, aborting a running Claude Code turn never settles. The Stop button and the turn timeout both leave the turn promise pending: the spinner keeps going and the server-side run never reaches a terminal state. On Windows
npm testalso reaches this verify and never returns from it.Two separate things combine in
server/claude-code/turn-runner.ts:server/claude-code/command.tsspawns a.cmdthroughcmd.exe, so the CLI is a grandchild of the process the runner holds.onAbortonly calledchild.kill(), which terminatescmd.exe; the real CLI keeps running and keeps the stdout write end open, sofor await (const rawLine of rl)waits for an EOF that never arrives.child.once('exit')was registered after that loop, so once the loop could end the exit event had already fired and the followingawait promisewould never settle either.Measured on Windows with Node 24.21.0, running the project's own verify
server/claude-code/claude-code-agent.verify.ts(the filenpm run verify:server-runsinvokes), againstmainat 72ddb33:No output at all in 60 s. (The wrapper is 20 lines: spawn this file under the local
node, kill the tree at the deadline, print the exit code and the elapsed time.)An instrumented probe of the abort case alone shows where it stops:
grandchildAliveisprocess.kill(pid, 0)on the pid the fake CLI wrote to disk, i.e. the processchild.kill()was supposed to remove.The change
One file,
server/claude-code/turn-runner.ts, +21 / -5.onAbortnow closes the readline interface and destroyschild.stdout/child.stderrbeforechild.kill(). Closing this side of the pipe is what ends thefor await, because no EOF is coming from the grandchild.child.exitCode ?? (await exitPromise), so an exit that already happened is not missed.finallydestroys both pipes too. Without that a surviving grandchild leaves a readable handle in the event loop and the process never exits: on my first attempt the file printedokand then hung past 60 s.Same verify, same machine, patched:
Five runs: 505, 507, 509, 511 and 518 ms, all
okwith exit 0. The abort case on its own now settles in 192 ms (probe, patched) instead of never.No assertion in the verify file was touched. The abort case still requires the emitted events to be exactly
[{ type: 'session', sessionId: 'fake-session-hang' }], and the successful-turn, oversized-system-prompt and failing-turn cases all still pass.What I deliberately did not change
child.kill()reachescmd.exeonly; I did not addtaskkill /Tor a job object. The verify file already acknowledges this by skipping its "the CLI process is gone" assertion on win32. Terminating the tree is a different decision about what abort guarantees, so this change only makes the turn settle. Worth a follow-up if you want the process itself gone.command.ts, no change to any other backend, and no change to the event translation.What remains unproven
tsc -bandoxlintwere not run: this machine has no checkout withnode_modules. The change only usesPromise.withResolvers,child.exitCodeandReadable#destroy, which are already available to this file.node server/claude-code/claude-code-agent.verify.ts), not throughtsx, becausetsxis not installed here. The assertions that ran are the file's own, unmodified.server/agent-runs/claude-code-turn.verify.tsneeds the app's dependency tree and could not run here.npm testwas not run, for the same reason. What is proven is the behaviour of the single file it invokes.server/claude-code/**—ci.ymlrunsnpm teston ubuntu-latest and thewindows-*.ymlworkflows filter on other paths — so CI will not exercise this path on Windows either way.