fix: Windows SHELL env var breaks taskkill; handleCancel aborts before updating job state on a partial kill failure - #656
Conversation
…e updating job state on a partial kill failure Bug 1: runCommand and SpawnedCodexAppServerClient.initialize both consulted process.env.SHELL when deciding the shell: option on Windows. SHELL is a POSIX convention with no meaning for native Windows process creation -- on a machine where it's set to a POSIX shell path (e.g. Git Bash, which Claude Code's own Bash tool sets), spawn/spawnSync routes commands through that shell instead of cmd.exe, and MSYS's automatic POSIX-path conversion mangles Windows-style flags like taskkill's /PID. Fixed by always using true on win32, never consulting SHELL. Bug 2: terminateProcessTree throws whenever taskkill exits non-zero for a reason other than "process not found". handleCancel called it unguarded, so a partial /T tree-kill failure on Windows aborted the whole cancel before the job's on-disk status was ever updated, leaving it stuck at running/finalizing forever even though the turn interrupt had already succeeded. Fixed by wrapping the call in try/catch and logging-but-continuing, matching terminateProcessTree's own best-effort semantics for the already-gone case. Fixes openai#647
![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: c79ac1c133
ℹ️ 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".
| terminateProcessTree(job.pid ?? Number.NaN); | ||
| try { | ||
| terminateProcessTree(job.pid ?? Number.NaN); | ||
| } catch (error) { |
There was a problem hiding this comment.
When the turn interrupt is unavailable or fails and terminateProcessTree also throws, this unconditional catch still reports success, marks the job cancelled, and clears its PID even though the worker may remain alive. A write-capable task can therefore keep modifying the workspace, and its later runTrackedJob completion can overwrite the cancelled state. Only suppress a failure proven to have stopped the root worker (or when the turn interrupt succeeded); otherwise preserve the PID and surface the cancellation failure.
Useful? React with 👍 / 👎.
…termination succeeded handleCancel's catch around terminateProcessTree unconditionally continued and reported the job as cancelled, clearing its pid, even when the turn interrupt was also unavailable/failed. terminateProcessTree already treats "process already gone" as non-fatal without throwing, so reaching the catch means the outcome is genuinely unknown, not just "already stopped." In that case, with the interrupt also not confirmed, nothing had actually proven the worker stopped -- reporting cancelled and clearing pid would let a write-capable task keep modifying the workspace unsupervised, with its later completion able to overwrite the fabricated cancelled status. Extracted the decision (wasCancellationConfirmed) into job-control.mjs so it's directly unit-testable, since codex-companion.mjs's handlers aren't exported and forcing terminateProcessTree to genuinely throw via a real subprocess integration test isn't reliably engineerable. When neither path confirms the stop, the job's status/pid are left unchanged and the command throws instead of reporting a cancellation that may not have happened. Found via Codex Review on the PR.
|
Correct, thanks -- and sorry for the delay getting to this. The catch was treating an unknown termination outcome the same as a confirmed one. Only report cancelled/clear pid now when the turn interrupt succeeded or termination completed without throwing (terminateProcessTree already handles the "already gone" case gracefully without throwing, so reaching the catch means the outcome is genuinely unconfirmed); otherwise the command throws instead of fabricating a cancelled status. |
2 participants
Fixes #647.
Bug 1 —
process.env.SHELLbreaks Windows process spawningrunCommand(lib/process.mjs) andSpawnedCodexAppServerClient.initialize(lib/app-server.mjs) both consultedprocess.env.SHELLwhen deciding theshell:option on Windows.SHELLis a POSIX convention with no meaning for native Windows process creation — on a machine where it happens to be set to a POSIX shell path (e.g. Git Bash, which Claude Code's own Bash tool sets),spawn/spawnSyncroutes commands through that shell instead ofcmd.exe, and MSYS's automatic POSIX-path conversion mangles Windows-style flags liketaskkill's/PID. Fixed by always usingtrueonwin32, never consultingSHELL.Bug 2 —
handleCancelaborts before persisting job state on a partial kill failureterminateProcessTreethrows whenevertaskkillexits non-zero for a reason other than "process not found" (matched via a narrow regex). On Windows,taskkill /Tcan fail to kill a subset of grandchild processes with a message that doesn't match that regex.handleCancelcalled it unguarded, so that throw aborted the whole cancel before the job's on-disk status was ever updated — leaving it permanently stuck at"running"/"finalizing"even though the turn interrupt (the part that actually matters) had already succeeded. Fixed by wrapping the call in try/catch and logging-but-continuing, matchingterminateProcessTree's own "process already gone" best-effort semantics for this case too.Testing
process.test.mjscase provingterminateProcessTreestill correctly throws for a genuine (non-"missing process") Windowstaskkillfailure — locking in the exact mechanismhandleCancel's new guard depends on.node --test tests/*.test.mjs): 92/92 pass, including all existingcancelend-to-end tests (confirming the happy path is unaffected).npx tsc -p tsconfig.app-server.json(covers both changedlib/files): clean.taskkillinvocation, a genuine partial-tree-kill throw) can't be exercised end-to-end from this environment — verified via the injectable-dependency unit test above plus careful manual review, matching the reporter's own locally-patched-and-confirmed fix (they state they applied this same change to their local plugin cache and confirmed both symptoms resolved).