fix: make cancel work under Git Bash on a non-English Windows (#423) - #577
fix: make cancel work under Git Bash on a non-English Windows (#423)#577xoonjaeho wants to merge 5 commits into
Conversation
…ad pids by exit code On Windows runCommand spawns through $SHELL, which is Git Bash when Claude Code runs from it. MSYS then rewrites taskkill's `/PID` flag into a path (`C:/Program Files/Git/PID`) and every cancel fails. Disable path conversion for that child only -- setting it on the node process itself breaks the `/c/...` paths node needs to resolve its own modules. A second defect surfaced once the flags survived: taskkill reports "no such process" with exit code 128 and a *localized* message, so the English-only regex never matched on a non-English Windows and an already-dead pid threw instead of resolving as already-stopped. Verified on ko-KR Windows: live pid -> exit 0, dead pid -> exit 128, and the English regex does not match the translated text. Fixes 3 previously-failing runtime tests (cancel x2, session-end cleanup) with no new failures.
`taskkill /PID <n> /T /F` walks the tree, and it exits non-zero when it killed the target but could not reap one descendant -- exit 255, "The operation attempted is not supported", which a console host or an already-exiting child produces. `terminateProcessTree` tolerated only exit 0 and exit 128, so that case threw, and the throw propagates out of `handleCancel`: `/codex:cancel` fails on Windows even though the process it was asked to kill is dead. The exit code cannot separate a partial kill from a real failure, and the message is localized -- keying on it is the bug the exit-128 check already exists to avoid. So probe the target with signal 0 instead: if it is gone, the kill did what the caller asked. A target that is still alive keeps throwing. Found by a test that had been dismissed as flaky: `tests/runtime.test.mjs:1740` fails ~1 run in 3 on this host, always here.
|
Pushed a second commit to this branch:
The exit code cannot separate a partial kill from a real failure, and that message is localized too, so keying on it would reintroduce exactly the bug the exit-128 check in this PR exists to avoid. It probes the target with signal 0 instead: gone ⇒ the kill did what the caller asked; still alive ⇒ throws as before. Two tests, each checked by breaking the line it protects. How it was found is worth a line: |
![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: d5c0b31c16
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (!result.error && processIsGone(pid, killImpl)) { | ||
| return { attempted: true, delivered: true, method: "taskkill", result }; |
There was a problem hiding this comment.
When taskkill exits non-zero because a descendant could not be killed (the case this branch is trying to tolerate), this branch probes only the root pid and then returns delivered: true. Callers such as cancellation and session teardown rely on terminateProcessTree to stop the whole job tree and do not make a second cleanup pass, so a surviving Codex/broker child can keep running after a reported cancellation; treat this as a partial failure unless the remaining tree is also gone.
Useful? React with 👍 / 👎.
`d5c0b31` probed the target once, the instant taskkill returned. A process that
is already terminating is still briefly alive at that instant, so the probe said
"alive" and the call threw anyway.
That is the case `/codex:cancel` actually hits: it interrupts the turn first, the
worker starts winding down, and the taskkill that follows loses the race and
reports "Access is denied" (exit 1, localized) rather than the exit 255 the
earlier commit handled. Poll for up to a second instead of once.
This is what `tests/runtime.test.mjs:1740` had really been failing on. It was
written off as a Windows flake, then attributed to exit 255; that was one of two
modes. Captured assertion:
taskkill /PID 1052 /T /F: exit=1: 오류: 액세스가 거부되었습니다.
1 !== 0
Four findings from the reviewer and two adversarial review legs on openai#577/openai#580. `terminateProcessTree` reported success after probing only the root pid, while `/T` was asked to take the tree, and callers make no second pass. `delivered` now answers only "did the target die"; the new `treeConfirmed` answers "is everything under it accounted for". Exit 0 confirms both; exit 128 means there was no process and so no tree in doubt; only the branch that tolerates a non-zero taskkill whose root turned out gone reports `treeConfirmed: false`. `handleCancel` logs that instead of reporting a clean cancellation -- without a consumer this would have been a relabel, not a fix. The stale-broker teardown killed whatever process owned a recorded pid. It now records the broker's OS process start time at spawn and requires it to still match; unverifiable identity skips the kill and only clears the stale files. `session-lifecycle-hook.mjs` killed by recorded pid with no such check at all -- the same defect on the path the first fix never reached -- and now shares the rule. The start-time probe shells out, so `runCommand` grew a `timeout` passthrough and the probe is bounded at 2 s; a timeout is just an unverifiable identity. macOS `ps -o lstart=` has only second granularity, which is recorded at the call site rather than papered over. Making the hook importable put `main()` behind an entry-point guard, so a test runs the file exactly as hooks.json does: get that guard wrong and the whole SessionEnd reaper becomes a silent no-op that nothing else would notice. Suite: 125 tests, 117 pass, 8 fail -- the 8 are the known platform and short-path failures that predate this work, and the `cancel` flake is gone.
…unted for" Review finding on this PR: after a non-zero taskkill the code probed only the root pid and then reported success, but `/T` was asked to take the tree, and callers make no second pass -- so a surviving codex or broker child kept running after a cancellation that reported clean. The two facts are now separate. `delivered` is about the target; `treeConfirmed` is about everything under it. Exit 0 confirms both. Exit 128 means there was no process, so no tree is in doubt. Only the branch that tolerates a non-zero taskkill whose root turned out to be gone -- exit 255's unreapable descendant, and exit 1's race with an already-exiting root -- cannot account for the tree, and it alone reports `treeConfirmed: false`. Windows offers no way to enumerate what survived once the root is gone, so reporting it honestly is as far as this goes. `handleCancel` now writes that to the job log instead of reporting a clean cancellation. Without a consumer this would have been a relabel, not a fix. Also adds a `timeout` passthrough to `runCommand`, which had none.
|
The finding on this PR — reporting tree termination after checking only the parent — is real and fixed in You are right that the branch conflated two different facts. They are separate now: Windows offers no way to enumerate what survived once the root is gone, so honest reporting is as far as this can go. To keep that from being a pure relabel, Also carries a |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bfdf8db271
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| !result.error && | ||
| (result.status === TASKKILL_PROCESS_NOT_FOUND || looksLikeMissingProcessMessage(combinedOutput)) | ||
| ) { | ||
| return { attempted: true, delivered: false, treeConfirmed: true, method: "taskkill", result }; |
There was a problem hiding this comment.
When Windows taskkill reports the recorded PID as missing, this branch now returns treeConfirmed: true, but that is only safe if the stored worker PID could not have left descendants behind. In this repo the worker/broker paths can spawn detached, unref'd children, so a stale job PID can be gone while a Codex/broker child is still running; handleCancel only warns on !treeConfirmed and then clears the PID. In that stale-PID case cancellation reports cleanly while the orphan becomes invisible, so the missing-root branch should remain unconfirmed unless descendants are otherwise verified.
Useful? React with 👍 / 👎.
A root taskkill cannot find says nothing about the children it spawned before it died, and with the root gone `/T` has nothing left to walk from. `cancel` only reaches this branch for a job the state still calls running -- a worker that died mid-turn -- which is exactly when an orphaned `codex app-server` is left behind. The branch now reports `treeConfirmed: false`, so `handleCancel` warns instead of reporting a clean cancellation.
|
The finding on You were right that the previous round drew the line in the wrong place. One thing worth recording, since it also answers the obvious objection that this makes cancellation warn all the time: it does not, and the reason is upstream of The POSIX side keeps
|
1 participant
Problem
Two defects make
/codex:cancelunusable when Claude Code runs from Git Bash on a non-English Windows. The second one is invisible until the first is fixed, which is why they are in one PR.1. MSYS rewrites
taskkill's/PIDflag into a path.runCommandspawns through$SHELLon Windows (process.mjs:12), which is Git Bash in this setup. MSYS then converts the/PIDargument beforetaskkill.exesees it:Every cancel fails. This is the same defect as #411 and #220, and this PR uses #411's approach —
MSYS_NO_PATHCONV=1+MSYS2_ARG_CONV_EXCL=*scoped to thetaskkillchild only. Scoping matters: setting those on the node process instead breaks the/c/...path translation node needs to resolve its own modules.2. Dead-pid detection is English-only, so an already-stopped process throws.
Once the flags survive,
taskkillruns and reports "no such process" — butlooksLikeMissingProcessMessagematches English text only. On a localized Windows nothing matches, soterminateProcessTreefalls through tothrow new Error(formatCommandFailure(result))instead of returningdelivered: false. This is #423; #424 also addresses it but currently conflicts and carries unrelated changes.The fix uses the exit code, which is locale-independent.
Verification
Measured on ko-KR Windows 11, Git Bash, node v24.14.0:
0성공: PID 35580인 프로세스가 종료되었습니다128오류: 프로세스 "35580"을(를) 찾을 수 없습니다Before the env change, the same call fails with
Invalid argument/option - 'C:/Program Files/Git/PID'. After it, the flags arrive intact.Test suite on the same host: 92 tests / 83 pass / 9 fail, against a pre-change baseline of 91 / 79 / 12. No new failures, and three previously-failing tests now pass:
cancel stops an active background job and marks it cancelledcancel sends turn interrupt to the shared app-server before killing a brokered tasksession end fully cleans up jobs for the ending session(The 9 remaining failures are pre-existing on Windows — Unix-socket, temp-dir and real-CLI assertions. CI is ubuntu-only so they do not appear there.)
Both changes were mutation-checked: reverting either production line makes its test fail, and restoring makes it pass.
Tests
terminateProcessTree uses taskkill on Windowsto assert the spawn env (from fix: preserve taskkill flags under Git Bash #411)terminateProcessTree treats exit code 128 as already stopped on a non-English WindowsBoth inject
platform,runCommandImplandkillImpl, so they perform no real spawn and run identically on Linux CI.Relation to existing PRs
This supersedes #411 (defect 1 only) and #424 (defect 2 only, currently conflicting). Credit to #411 for the env approach — I verified it empirically and kept it as-is. Happy to rebase, split, or close this in favour of either if a maintainer prefers.
Fixes #423
Why this matters beyond
cancelterminateProcessTreeis not only reached from/codex:cancel. It is also the killersession-lifecycle-hook.mjs:105passes askillProcesswhen theSessionEndhook reaps abroker. On a Windows host running Claude Code from Git Bash, that reap is silently a no-op
today for the same MSYS reason —
taskkillnever receives/PID.That turns out to matter for the app-server broker leak.
broker-lifecycle.mjsspawns brokersdetached+unref, the broker has no idle timeout, and the stale-teardown path atbroker-lifecycle.mjs:120callsteardownBrokerSessionwithoutkillProcess, so a brokerthat misses the 150 ms endpoint probe is replaced while the old process keeps running and its
state files are deleted — invisible to the reaper forever. On this host that produced four live
brokers for a single
cwdwherebroker.jsonnamed one, and a per-orphan footprint of ~9processes.
Wiring
terminateProcessTreeinto that teardown is the natural fix for the leak — but onWindows + Git Bash it would still be a no-op without the patch in this PR. So this change is a
prerequisite for that fix landing effectively on Windows, not just a
cancelbug. Filing thebroker fix separately; flagging the dependency here so the two are not evaluated in isolation.