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 dd8fc3751..e1b7a35d8 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 @@ -54,9 +55,55 @@ 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: "*" + }; +} + +// 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"; + } +} + +// 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 }; + return { attempted: false, delivered: false, treeConfirmed: true, method: null }; } const platform = options.platform ?? process.platform; @@ -66,25 +113,48 @@ 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) { - 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(); - if (!result.error && looksLikeMissingProcessMessage(combinedOutput)) { - return { attempted: true, delivered: false, method: "taskkill", result }; + if ( + !result.error && + (result.status === TASKKILL_PROCESS_NOT_FOUND || looksLikeMissingProcessMessage(combinedOutput)) + ) { + // "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 + // 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. 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 }; } 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; } @@ -99,21 +169,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 80e0715b0..e36210c6d 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,131 @@ 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.treeConfirmed, true, "a clean taskkill accounts for the whole tree"); + 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.treeConfirmed, + false, + "a root that is already gone says nothing about the children it left behind" + ); 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 reports a partial taskkill failure when only the root is known 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.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"); +}); + +// 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.equal(outcome.treeConfirmed, false, "a raced kill cannot account for the tree"); + 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; // never goes away + } + }), + /Access is denied|taskkill/ + ); +}); + test("terminateProcessTree treats missing Windows processes as already stopped", () => { const outcome = terminateProcessTree(1234, { platform: "win32",