From 5aa7366bc504c25dd73d7edb8468b2c365075e3f Mon Sep 17 00:00:00 2001 From: xoonjaeho Date: Sat, 1 Aug 2026 09:39:27 +0900 Subject: [PATCH 1/5] fix(process): keep taskkill flags intact under Git Bash and detect dead 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. --- plugins/codex/scripts/lib/process.mjs | 24 +++++++++++++-- tests/process.test.mjs | 42 +++++++++++++++++++++++++-- 2 files changed, 61 insertions(+), 5 deletions(-) diff --git a/plugins/codex/scripts/lib/process.mjs b/plugins/codex/scripts/lib/process.mjs index dd8fc3751..bc65ead8c 100644 --- a/plugins/codex/scripts/lib/process.mjs +++ b/plugins/codex/scripts/lib/process.mjs @@ -54,6 +54,23 @@ function looksLikeMissingProcessMessage(text) { return /not found|no running instance|cannot find|does not exist|no such process/i.test(text); } +// taskkill's exit code for "no such process". Locale-independent, unlike the message +// text: on a non-English Windows the "not found" wording is translated and the regex +// above never matches, so an already-dead pid would surface as a thrown error. +const TASKKILL_PROCESS_NOT_FOUND = 128; + +// 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 the call fails. Disable the conversion for this child only -- setting it on the +// node process itself would break the `/c/...` paths node needs to resolve. +function envWithoutMsysPathConversion(env) { + return { + ...(env ?? process.env), + MSYS_NO_PATHCONV: "1", + MSYS2_ARG_CONV_EXCL: "*" + }; +} + export function terminateProcessTree(pid, options = {}) { if (!Number.isFinite(pid)) { return { attempted: false, delivered: false, method: null }; @@ -66,7 +83,7 @@ export function terminateProcessTree(pid, options = {}) { if (platform === "win32") { const result = runCommandImpl("taskkill", ["/PID", String(pid), "/T", "/F"], { cwd: options.cwd, - env: options.env + env: envWithoutMsysPathConversion(options.env) }); if (!result.error && result.status === 0) { @@ -74,7 +91,10 @@ export function terminateProcessTree(pid, options = {}) { } const combinedOutput = `${result.stderr}\n${result.stdout}`.trim(); - if (!result.error && looksLikeMissingProcessMessage(combinedOutput)) { + if ( + !result.error && + (result.status === TASKKILL_PROCESS_NOT_FOUND || looksLikeMissingProcessMessage(combinedOutput)) + ) { return { attempted: true, delivered: false, method: "taskkill", result }; } diff --git a/tests/process.test.mjs b/tests/process.test.mjs index 80e0715b0..02b54004e 100644 --- a/tests/process.test.mjs +++ b/tests/process.test.mjs @@ -7,8 +7,9 @@ test("terminateProcessTree uses taskkill on Windows", () => { let captured = null; const outcome = terminateProcessTree(1234, { platform: "win32", - runCommandImpl(command, args) { - captured = { command, args }; + env: { PATH: "C:\\Windows\\System32", SHELL: "C:\\Program Files\\Git\\bin\\bash.exe" }, + runCommandImpl(command, args, options) { + captured = { command, args, options }; return { command, args, @@ -26,12 +27,47 @@ test("terminateProcessTree uses taskkill on Windows", () => { assert.deepEqual(captured, { command: "taskkill", - args: ["/PID", "1234", "/T", "/F"] + args: ["/PID", "1234", "/T", "/F"], + options: { + cwd: undefined, + env: { + PATH: "C:\\Windows\\System32", + SHELL: "C:\\Program Files\\Git\\bin\\bash.exe", + MSYS_NO_PATHCONV: "1", + MSYS2_ARG_CONV_EXCL: "*" + } + } }); assert.equal(outcome.delivered, true); assert.equal(outcome.method, "taskkill"); }); +test("terminateProcessTree treats exit code 128 as already stopped on a non-English Windows", () => { + // Korean Windows: taskkill translates "not found", so the English regex cannot match. + // Only the exit code identifies the case. Without it this call throws. + const outcome = terminateProcessTree(1234, { + platform: "win32", + runCommandImpl(command, args) { + return { + command, + args, + status: 128, + signal: null, + stdout: "", + stderr: "오류: 프로세스 \"1234\"을(를) 찾을 수 없습니다.", + error: null + }; + }, + killImpl() { + throw new Error("kill fallback should not run"); + } + }); + + assert.equal(outcome.attempted, true); + assert.equal(outcome.delivered, false); + assert.equal(outcome.method, "taskkill"); +}); + test("terminateProcessTree treats missing Windows processes as already stopped", () => { const outcome = terminateProcessTree(1234, { platform: "win32", From d5c0b31c16d54d845a108995ddc5f8da556cd4f2 Mon Sep 17 00:00:00 2001 From: xoonjaeho Date: Sat, 1 Aug 2026 16:45:52 +0900 Subject: [PATCH 2/5] fix(process): accept a taskkill failure whose target is already gone `taskkill /PID /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. --- plugins/codex/scripts/lib/process.mjs | 21 ++++++++++++ tests/process.test.mjs | 49 +++++++++++++++++++++++++++ 2 files changed, 70 insertions(+) diff --git a/plugins/codex/scripts/lib/process.mjs b/plugins/codex/scripts/lib/process.mjs index bc65ead8c..9e5be1cd9 100644 --- a/plugins/codex/scripts/lib/process.mjs +++ b/plugins/codex/scripts/lib/process.mjs @@ -71,6 +71,17 @@ function envWithoutMsysPathConversion(env) { }; } +// Signal 0 tests for existence without touching the process. EPERM means it is alive +// under another owner, so anything but ESRCH counts as still running. +function processIsGone(pid, killImpl) { + try { + killImpl(pid, 0); + return false; + } catch (error) { + return error?.code === "ESRCH"; + } +} + export function terminateProcessTree(pid, options = {}) { if (!Number.isFinite(pid)) { return { attempted: false, delivered: false, method: null }; @@ -98,6 +109,16 @@ export function terminateProcessTree(pid, options = {}) { return { attempted: true, delivered: false, method: "taskkill", result }; } + // `/T` makes taskkill walk the tree, and it exits non-zero when it killed the target + // but could not reap one descendant -- "The operation attempted is not supported", + // exit 255, which is what a console host or an already-exiting child produces. The + // exit code does not separate that from a genuine failure, and the message is + // localized, so ask the OS whether the target itself is gone rather than parsing + // either. The target being dead is what every caller of this actually asked for. + if (!result.error && processIsGone(pid, killImpl)) { + return { attempted: true, delivered: true, method: "taskkill", result }; + } + if (result.error?.code === "ENOENT") { try { killImpl(pid); diff --git a/tests/process.test.mjs b/tests/process.test.mjs index 02b54004e..440cbee94 100644 --- a/tests/process.test.mjs +++ b/tests/process.test.mjs @@ -68,6 +68,55 @@ test("terminateProcessTree treats exit code 128 as already stopped on a non-Engl assert.equal(outcome.method, "taskkill"); }); +// The real message, from a Korean Windows host: taskkill killed the target but could not +// reap one descendant. Keying on the text would reintroduce the locale bug this file +// exists for, so the target's own liveness has to decide it. +test("terminateProcessTree accepts a taskkill failure whose target is nonetheless gone", () => { + let probedSignal; + const outcome = terminateProcessTree(1234, { + platform: "win32", + runCommandImpl(command, args) { + return { + command, + args, + status: 255, + signal: null, + stdout: "", + stderr: + "오류: PID 27048인 프로세스(PID 1234인 자식 프로세스)를 종료할 수 없습니다.\n오류: 시도한 작업은 지원되지 않습니다.", + error: null + }; + }, + killImpl(pid, signal) { + probedSignal = signal; + const error = new Error("no such process"); + error.code = "ESRCH"; + throw error; + } + }); + + assert.equal(outcome.attempted, true); + assert.equal(outcome.delivered, true); + assert.equal(outcome.method, "taskkill"); + assert.equal(probedSignal, 0, "the target must be probed, not signalled"); +}); + +test("terminateProcessTree still throws when taskkill fails and the target is alive", () => { + assert.throws( + () => + terminateProcessTree(1234, { + platform: "win32", + runCommandImpl(command, args) { + return { command, args, status: 1, signal: null, stdout: "", stderr: "Access is denied.", error: null }; + }, + killImpl() { + return true; // the probe finds it running + } + }), + /Access is denied|taskkill/ + ); +}); + test("terminateProcessTree treats missing Windows processes as already stopped", () => { const outcome = terminateProcessTree(1234, { platform: "win32", From bfb60eb9aff91e1cf33c920151f7bc0292faa0ad Mon Sep 17 00:00:00 2001 From: xoonjaeho Date: Sat, 1 Aug 2026 17:33:22 +0900 Subject: [PATCH 3/5] fix(process): wait out a target that taskkill raced while it was exiting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 --- plugins/codex/scripts/lib/process.mjs | 33 +++++++++++++++++++++------ tests/process.test.mjs | 31 +++++++++++++++++++++++-- 2 files changed, 55 insertions(+), 9 deletions(-) diff --git a/plugins/codex/scripts/lib/process.mjs b/plugins/codex/scripts/lib/process.mjs index 9e5be1cd9..ad85def75 100644 --- a/plugins/codex/scripts/lib/process.mjs +++ b/plugins/codex/scripts/lib/process.mjs @@ -82,6 +82,24 @@ function processIsGone(pid, killImpl) { } } +// A process that is already on its way out -- taskkill losing the race with a turn that +// was just interrupted reports "Access is denied", exit 1 -- is still briefly alive, so a +// single instantaneous probe calls it a failure. Give it a bounded moment to finish. +// This whole function is synchronous, hence the Atomics sleep rather than a timer. +function waitForProcessGone(pid, killImpl, timeoutMs) { + const deadline = Date.now() + timeoutMs; + const idle = new Int32Array(new SharedArrayBuffer(4)); + for (;;) { + if (processIsGone(pid, killImpl)) { + return true; + } + if (Date.now() >= deadline) { + return false; + } + Atomics.wait(idle, 0, 0, Math.min(50, Math.max(1, deadline - Date.now()))); + } +} + export function terminateProcessTree(pid, options = {}) { if (!Number.isFinite(pid)) { return { attempted: false, delivered: false, method: null }; @@ -109,13 +127,14 @@ export function terminateProcessTree(pid, options = {}) { return { attempted: true, delivered: false, method: "taskkill", result }; } - // `/T` makes taskkill walk the tree, and it exits non-zero when it killed the target - // but could not reap one descendant -- "The operation attempted is not supported", - // exit 255, which is what a console host or an already-exiting child produces. The - // exit code does not separate that from a genuine failure, and the message is - // localized, so ask the OS whether the target itself is gone rather than parsing - // either. The target being dead is what every caller of this actually asked for. - if (!result.error && processIsGone(pid, killImpl)) { + // taskkill exits non-zero in two cases where the target still ends up dead: it killed + // the target but could not reap a descendant ("The operation attempted is not + // supported", exit 255), and it raced a process that was already terminating + // ("Access is denied", exit 1 -- what `cancel` hits after interrupting the turn). + // The exit code separates neither from a genuine failure, and both messages are + // localized, so ask the OS whether the target is gone instead of parsing either. + // That is what every caller of this actually asked for. + if (!result.error && waitForProcessGone(pid, killImpl, options.killWaitMs ?? 1000)) { return { attempted: true, delivered: true, method: "taskkill", result }; } diff --git a/tests/process.test.mjs b/tests/process.test.mjs index 440cbee94..965e15da7 100644 --- a/tests/process.test.mjs +++ b/tests/process.test.mjs @@ -101,16 +101,43 @@ test("terminateProcessTree accepts a taskkill failure whose target is nonetheles assert.equal(probedSignal, 0, "the target must be probed, not signalled"); }); -test("terminateProcessTree still throws when taskkill fails and the target is alive", () => { +// The real failure behind `/codex:cancel`: the turn was just interrupted, so the worker +// is already terminating and taskkill loses the race with "Access is denied". The target +// is not gone at the instant taskkill returns -- only a moment later. +test("terminateProcessTree waits out a target that is still exiting", () => { + let probes = 0; + const outcome = terminateProcessTree(1234, { + platform: "win32", + killWaitMs: 500, + runCommandImpl(command, args) { + return { command, args, status: 1, signal: null, stdout: "", stderr: "오류: 액세스가 거부되었습니다.", error: null }; + }, + killImpl() { + probes += 1; + if (probes < 3) { + return true; // still winding down + } + const error = new Error("no such process"); + error.code = "ESRCH"; + throw error; + } + }); + + assert.equal(outcome.delivered, true); + assert.ok(probes >= 3, "it gave up after a single probe"); +}); + +test("terminateProcessTree still throws when taskkill fails and the target stays alive", () => { assert.throws( () => terminateProcessTree(1234, { platform: "win32", + killWaitMs: 0, runCommandImpl(command, args) { return { command, args, status: 1, signal: null, stdout: "", stderr: "Access is denied.", error: null }; }, killImpl() { - return true; // the probe finds it running + return true; // never goes away } }), /Access is denied|taskkill/ From bfdf8db271e9e66b160071d170b19816fba42759 Mon Sep 17 00:00:00 2001 From: xoonjaeho Date: Sat, 1 Aug 2026 18:53:10 +0900 Subject: [PATCH 4/5] fix(process): separate "the target died" from "the whole tree is accounted 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. --- plugins/codex/scripts/codex-companion.mjs | 12 ++++++- plugins/codex/scripts/lib/process.mjs | 39 +++++++++++++---------- tests/process.test.mjs | 6 +++- 3 files changed, 38 insertions(+), 19 deletions(-) diff --git a/plugins/codex/scripts/codex-companion.mjs b/plugins/codex/scripts/codex-companion.mjs index 83df468ad..a2b965785 100644 --- a/plugins/codex/scripts/codex-companion.mjs +++ b/plugins/codex/scripts/codex-companion.mjs @@ -983,7 +983,17 @@ async function handleCancel(argv) { ); } - terminateProcessTree(job.pid ?? Number.NaN); + const termination = terminateProcessTree(job.pid ?? Number.NaN); + if (termination.attempted && !termination.treeConfirmed) { + // The job's own process is gone, but the kill could not account for what was under + // it. Say so rather than reporting a clean cancellation: a surviving codex or broker + // child keeps running, and nothing else makes a second pass at it. + appendLogLine( + job.logFile, + `Cancelled, but the process tree under pid ${job.pid} could not be fully confirmed stopped. ` + + "Check for leftover codex processes if the workspace behaves oddly." + ); + } appendLogLine(job.logFile, "Cancelled by user."); const completedAt = nowIso(); diff --git a/plugins/codex/scripts/lib/process.mjs b/plugins/codex/scripts/lib/process.mjs index ad85def75..1777b969e 100644 --- a/plugins/codex/scripts/lib/process.mjs +++ b/plugins/codex/scripts/lib/process.mjs @@ -8,6 +8,7 @@ export function runCommand(command, args = [], options = {}) { encoding: "utf8", input: options.input, maxBuffer: options.maxBuffer, + timeout: options.timeout, stdio: options.stdio ?? "pipe", shell: options.shell ?? (process.platform === "win32" ? (process.env.SHELL || true) : false), windowsHide: true @@ -102,7 +103,7 @@ function waitForProcessGone(pid, killImpl, timeoutMs) { export function terminateProcessTree(pid, options = {}) { if (!Number.isFinite(pid)) { - return { attempted: false, delivered: false, method: null }; + return { attempted: false, delivered: false, treeConfirmed: true, method: null }; } const platform = options.platform ?? process.platform; @@ -116,7 +117,7 @@ export function terminateProcessTree(pid, options = {}) { }); if (!result.error && result.status === 0) { - return { attempted: true, delivered: true, method: "taskkill", result }; + return { attempted: true, delivered: true, treeConfirmed: true, method: "taskkill", result }; } const combinedOutput = `${result.stderr}\n${result.stdout}`.trim(); @@ -124,27 +125,31 @@ export function terminateProcessTree(pid, options = {}) { !result.error && (result.status === TASKKILL_PROCESS_NOT_FOUND || looksLikeMissingProcessMessage(combinedOutput)) ) { - return { attempted: true, delivered: false, method: "taskkill", result }; + return { attempted: true, delivered: false, treeConfirmed: true, method: "taskkill", result }; } - // taskkill exits non-zero in two cases where the target still ends up dead: it killed - // the target but could not reap a descendant ("The operation attempted is not - // supported", exit 255), and it raced a process that was already terminating - // ("Access is denied", exit 1 -- what `cancel` hits after interrupting the turn). - // The exit code separates neither from a genuine failure, and both messages are - // localized, so ask the OS whether the target is gone instead of parsing either. - // That is what every caller of this actually asked for. + // taskkill exits non-zero both after failing to reap a descendant ("The operation + // attempted is not supported", 255) and when it races a root that was already + // terminating ("Access is denied", 1 -- what `cancel` hits after interrupting the + // turn). Both messages are localized, so the root's own liveness is the only usable + // signal: it is gone, and the kill should not throw for either case. + // + // But a non-zero taskkill never establishes that the REST of the tree went with it, + // and `/T` leaves no way to enumerate what survived once the root is gone. So the + // two facts are reported separately -- `delivered` is about the target, and + // `treeConfirmed` is about everything under it. This is the one branch that cannot + // account for the whole tree. if (!result.error && waitForProcessGone(pid, killImpl, options.killWaitMs ?? 1000)) { - return { attempted: true, delivered: true, method: "taskkill", result }; + return { attempted: true, delivered: true, treeConfirmed: false, method: "taskkill", result }; } if (result.error?.code === "ENOENT") { try { killImpl(pid); - return { attempted: true, delivered: true, method: "kill" }; + return { attempted: true, delivered: true, treeConfirmed: true, method: "kill" }; } catch (error) { if (error?.code === "ESRCH") { - return { attempted: true, delivered: false, method: "kill" }; + return { attempted: true, delivered: false, treeConfirmed: true, method: "kill" }; } throw error; } @@ -159,21 +164,21 @@ export function terminateProcessTree(pid, options = {}) { try { killImpl(-pid, "SIGTERM"); - return { attempted: true, delivered: true, method: "process-group" }; + return { attempted: true, delivered: true, treeConfirmed: true, method: "process-group" }; } catch (error) { if (error?.code !== "ESRCH") { try { killImpl(pid, "SIGTERM"); - return { attempted: true, delivered: true, method: "process" }; + return { attempted: true, delivered: true, treeConfirmed: true, method: "process" }; } catch (innerError) { if (innerError?.code === "ESRCH") { - return { attempted: true, delivered: false, method: "process" }; + return { attempted: true, delivered: false, treeConfirmed: true, method: "process" }; } throw innerError; } } - return { attempted: true, delivered: false, method: "process-group" }; + return { attempted: true, delivered: false, treeConfirmed: true, method: "process-group" }; } } diff --git a/tests/process.test.mjs b/tests/process.test.mjs index 965e15da7..6bfd62cea 100644 --- a/tests/process.test.mjs +++ b/tests/process.test.mjs @@ -39,6 +39,7 @@ test("terminateProcessTree uses taskkill on Windows", () => { } }); assert.equal(outcome.delivered, true); + assert.equal(outcome.treeConfirmed, true, "a clean taskkill accounts for the whole tree"); assert.equal(outcome.method, "taskkill"); }); @@ -65,13 +66,14 @@ test("terminateProcessTree treats exit code 128 as already stopped on a non-Engl assert.equal(outcome.attempted, true); assert.equal(outcome.delivered, false); + assert.equal(outcome.treeConfirmed, true, "there was no process, so no tree is in doubt"); assert.equal(outcome.method, "taskkill"); }); // The real message, from a Korean Windows host: taskkill killed the target but could not // reap one descendant. Keying on the text would reintroduce the locale bug this file // exists for, so the target's own liveness has to decide it. -test("terminateProcessTree accepts a taskkill failure whose target is nonetheless gone", () => { +test("terminateProcessTree reports a partial taskkill failure when only the root is known gone", () => { let probedSignal; const outcome = terminateProcessTree(1234, { platform: "win32", @@ -97,6 +99,7 @@ test("terminateProcessTree accepts a taskkill failure whose target is nonetheles assert.equal(outcome.attempted, true); assert.equal(outcome.delivered, true); + assert.equal(outcome.treeConfirmed, false, "a partial kill must not claim the tree"); assert.equal(outcome.method, "taskkill"); assert.equal(probedSignal, 0, "the target must be probed, not signalled"); }); @@ -124,6 +127,7 @@ test("terminateProcessTree waits out a target that is still exiting", () => { }); assert.equal(outcome.delivered, true); + assert.equal(outcome.treeConfirmed, false, "a raced kill cannot account for the tree"); assert.ok(probes >= 3, "it gave up after a single probe"); }); From f9d718de731150ef00e198dddf7e7896badcdb68 Mon Sep 17 00:00:00 2001 From: xoonjaeho Date: Sun, 2 Aug 2026 09:06:23 +0900 Subject: [PATCH 5/5] fix(process): stop confirming the tree when taskkill finds no root 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. --- plugins/codex/scripts/lib/process.mjs | 11 ++++++++--- tests/process.test.mjs | 6 +++++- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/plugins/codex/scripts/lib/process.mjs b/plugins/codex/scripts/lib/process.mjs index 1777b969e..e1b7a35d8 100644 --- a/plugins/codex/scripts/lib/process.mjs +++ b/plugins/codex/scripts/lib/process.mjs @@ -125,7 +125,12 @@ export function terminateProcessTree(pid, options = {}) { !result.error && (result.status === TASKKILL_PROCESS_NOT_FOUND || looksLikeMissingProcessMessage(combinedOutput)) ) { - return { attempted: true, delivered: false, treeConfirmed: true, method: "taskkill", result }; + // "No such process" is about the root at this instant, not about what it spawned + // while it was alive. A worker that died mid-turn leaves its `codex app-server` + // child orphaned, and with the root gone `/T` has nothing left to walk from. That + // is not a corner case here: `cancel` only reaches this branch for a job the state + // still calls running, which is exactly the crashed-worker case. + return { attempted: true, delivered: false, treeConfirmed: false, method: "taskkill", result }; } // taskkill exits non-zero both after failing to reap a descendant ("The operation @@ -137,8 +142,8 @@ export function terminateProcessTree(pid, options = {}) { // But a non-zero taskkill never establishes that the REST of the tree went with it, // and `/T` leaves no way to enumerate what survived once the root is gone. So the // two facts are reported separately -- `delivered` is about the target, and - // `treeConfirmed` is about everything under it. This is the one branch that cannot - // account for the whole tree. + // `treeConfirmed` is about everything under it. Like the missing-root branch above, + // this one cannot account for the whole tree. if (!result.error && waitForProcessGone(pid, killImpl, options.killWaitMs ?? 1000)) { return { attempted: true, delivered: true, treeConfirmed: false, method: "taskkill", result }; } diff --git a/tests/process.test.mjs b/tests/process.test.mjs index 6bfd62cea..e36210c6d 100644 --- a/tests/process.test.mjs +++ b/tests/process.test.mjs @@ -66,7 +66,11 @@ test("terminateProcessTree treats exit code 128 as already stopped on a non-Engl assert.equal(outcome.attempted, true); assert.equal(outcome.delivered, false); - assert.equal(outcome.treeConfirmed, true, "there was no process, so no tree is in doubt"); + assert.equal( + outcome.treeConfirmed, + false, + "a root that is already gone says nothing about the children it left behind" + ); assert.equal(outcome.method, "taskkill"); });