diff --git a/README.md b/README.md index 3534a799f..e0ea536dc 100644 --- a/README.md +++ b/README.md @@ -66,6 +66,9 @@ say what bounds those windows and where the files are. - Usage will contribute to your Codex usage limits. [Learn more](https://developers.openai.com/codex/pricing). - **Node.js 18.18 or later.** - It does not have to be on the system PATH: the hooks resolve Node through `scripts/run-node.sh`, which also looks in nvm, fnm, asdf, mise, Volta and Homebrew toolchains, preferring one that ships `codex` alongside it. Set `CODEX_COMPANION_NODE` to an executable path to pin a specific one. +- **On Windows: Git Bash** (the shell Git for Windows installs), because the hooks run `scripts/run-node.sh` through it. + - Nothing else is spawned through a shell. `codex` and `npm` are `.cmd` shims, so they are invoked as an explicit `cmd.exe /d /s /c call`; everything else — `git`, `taskkill`, `powershell.exe`, the background worker — is spawned directly. That matters: routing through a POSIX shell makes MSYS rewrite Windows-style switches, which is why `taskkill /PID … /T /F` used to fail and background workers could not be stopped. + - The shared broker listens on a named pipe rather than a Unix socket, and has no filesystem artifact to clean up. ## Install @@ -417,6 +420,8 @@ Broker and background-job lifecycle: | [#659](https://github.com/openai/codex-plugin-cc/pull/659) | state written under one `CLAUDE_PLUGIN_DATA` root is no longer invisible to an invocation that resolves to another, which orphaned brokers and hid jobs | | [#707](https://github.com/openai/codex-plugin-cc/pull/707) | the broker releases its app-server thread subscriptions when a client disconnects, instead of leaking them for its whole lifetime | | [#728](https://github.com/openai/codex-plugin-cc/pull/728) | a job whose worker died no longer reads as "running" forever; `/codex:status` reconciles the record against the live process | +| [#725](https://github.com/openai/codex-plugin-cc/pull/725) | nothing is spawned through the user's shell on Windows, where MSYS path conversion mangled switches like `taskkill /PID` and left background workers unkillable under Git Bash (this supersedes [#735](https://github.com/openai/codex-plugin-cc/pull/735)) | +| [#656](https://github.com/openai/codex-plugin-cc/pull/656) | `/codex:cancel` exits non-zero when neither the turn interrupt nor the worker kill confirmed the job stopped, instead of reporting a cancellation nothing proved | Commands and flags: @@ -457,6 +462,11 @@ Beyond the imports, this fork carries fixes for defects the imports themselves s - disabling the review gate is not outvoted by a stale enable left under another plugin-data root - `CLAUDE_ENV_FILE` is only ever appended to: it is shared with other plugins' hooks, and rewriting it dropped whatever they had just written +- teardown never force-kills through a negative pid: a process group is POSIX-only, so on Windows + that was an invalid handle and the fallback killed the worker alone, leaving its app-server — and + every MCP server under it — running. `taskkill /T /F` walks the tree there instead +- a state write that Windows briefly refuses — a scanner or indexer holding the file open, which + surfaces as `EPERM`/`EBUSY` on the replacing rename — is retried instead of losing the record - the app-server typecheck (`npm run build`) passes Each of those came out of an adversarial review of the merges, re-run after every round of fixes; diff --git a/plugins/codex/scripts/codex-companion.mjs b/plugins/codex/scripts/codex-companion.mjs index fc4fe1c56..e4760f568 100644 --- a/plugins/codex/scripts/codex-companion.mjs +++ b/plugins/codex/scripts/codex-companion.mjs @@ -43,7 +43,8 @@ import { reconcileJobsLiveness, resolveCancelableJob, resolveResultJob, - sortJobsNewestFirst + sortJobsNewestFirst, + wasCancellationConfirmed } from "./lib/job-control.mjs"; import { appendLogLine, @@ -1369,6 +1370,12 @@ async function handleCancel(argv) { const effectiveStatus = orphanAdopted ? readStoredJob(workspaceRoot, job.id)?.status ?? "cancelled" : "cancelled"; + // Neither the turn interrupt nor the worker kill proved the job stopped: + // a partial `taskkill /T` on Windows can refuse a subset of the tree. The + // record is already written (record-first, above), so the cancellation is + // not rewound — but the command must not exit as though it had worked while + // a write-capable task may still be editing the workspace. + const cancellationConfirmed = wasCancellationConfirmed(interrupt, workerKillError == null); const payload = { jobId: job.id, status: effectiveStatus, @@ -1377,7 +1384,8 @@ async function handleCancel(argv) { turnInterrupted: interrupt.interrupted, // A failed kill leaves the worker alive even though the record is // cancelled; the caller must be able to tell that from a clean cancel. - workerTerminated: workerKillError == null + workerTerminated: workerKillError == null, + cancellationConfirmed }; outputCommandResult( @@ -1385,6 +1393,13 @@ async function handleCancel(argv) { renderCancelReport({ ...nextJob, status: effectiveStatus }, { workerTerminated: workerKillError == null }), options.json ); + + if (!cancellationConfirmed) { + throw new Error( + `Could not confirm job ${job.id} stopped: the turn interrupt did not succeed and terminating the worker process tree failed. ` + + "The job is recorded as cancelled, but its worker may still be running — check it before relying on the workspace." + ); + } } async function main() { diff --git a/plugins/codex/scripts/lib/app-server.mjs b/plugins/codex/scripts/lib/app-server.mjs index d7a912f75..91961e0ee 100644 --- a/plugins/codex/scripts/lib/app-server.mjs +++ b/plugins/codex/scripts/lib/app-server.mjs @@ -14,7 +14,7 @@ import { spawn } from "node:child_process"; import readline from "node:readline"; import { parseBrokerEndpoint } from "./broker-endpoint.mjs"; import { ensureBrokerSession, isBrokerEndpointReady, loadBrokerSession } from "./broker-lifecycle.mjs"; -import { terminateProcessTree } from "./process.mjs"; +import { commandWithWindowsShim, terminateProcessTree } from "./process.mjs"; const PLUGIN_MANIFEST_URL = new URL("../../.claude-plugin/plugin.json", import.meta.url); const PLUGIN_MANIFEST = JSON.parse(fs.readFileSync(PLUGIN_MANIFEST_URL, "utf8")); @@ -216,11 +216,15 @@ class SpawnedCodexAppServerClient extends AppServerClientBase { } async initialize() { - this.proc = spawn("codex", ["app-server"], { + const invocation = commandWithWindowsShim("codex", ["app-server"]); + this.proc = spawn(invocation.command, invocation.args, { cwd: this.cwd, env: this.options.env ?? process.env, stdio: ["pipe", "pipe", "pipe"], - shell: process.platform === "win32" ? (process.env.SHELL || true) : false, + // See runCommand(): SHELL is a POSIX convention and is never consulted + // for Windows process creation. `codex` is a .cmd shim there, so the + // invocation above wraps it in an explicit cmd.exe call instead. + shell: invocation.shell, windowsHide: true }); @@ -274,9 +278,10 @@ class SpawnedCodexAppServerClient extends AppServerClientBase { this.proc.stdin.end(); setTimeout(() => { if (this.proc && !this.proc.killed && this.proc.exitCode === null) { - // On Windows with shell: true, the direct child is cmd.exe. - // Use terminateProcessTree to kill the entire tree including - // the grandchild node process. + // On Windows the direct child is cmd.exe — `codex` is a .cmd shim, so + // commandWithWindowsShim() spawns `cmd.exe /d /s /c call codex …`. + // Killing that child alone would leave the app-server grandchild + // running, so tear down the whole tree. if (process.platform === "win32") { try { terminateProcessTree(this.proc.pid); @@ -306,9 +311,9 @@ class SpawnedCodexAppServerClient extends AppServerClientBase { if (this.proc && !this.proc.killed) { try { if (process.platform === "win32") { - // With shell: true the direct child is cmd.exe; kill the whole - // tree so the codex app-server grandchild does not survive the - // timeout (mirrors the graceful close() path). + // Same as close(): on Windows the direct child is the cmd.exe that + // runs the `codex` shim, so the whole tree has to go or the + // app-server grandchild survives the timeout. terminateProcessTree(this.proc.pid); } else { this.proc.kill("SIGKILL"); diff --git a/plugins/codex/scripts/lib/fs.mjs b/plugins/codex/scripts/lib/fs.mjs index 730259831..92363c09c 100644 --- a/plugins/codex/scripts/lib/fs.mjs +++ b/plugins/codex/scripts/lib/fs.mjs @@ -37,6 +37,43 @@ export function writePrivateFile(filePath, value) { setMode(filePath, PRIVATE_FILE_MODE); } +// Windows can refuse to replace a file that something else has open — an on-access virus scanner +// or a search indexer opening the target for a moment is enough, and it surfaces as EPERM/EACCES/ +// EBUSY rather than as anything the caller could act on. POSIX rename has no such failure mode, so +// this retry is Windows-only: a handful of short waits, after which the error is real and is thrown. +const WINDOWS_RENAME_RETRY_DELAYS_MS = [5, 15, 40, 100]; +const WINDOWS_RENAME_RETRY_CODES = new Set(["EPERM", "EACCES", "EBUSY"]); + +function sleepSync(ms) { + Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, ms); +} + +/** + * @param {string} from + * @param {string} to + * @param {{ platform?: string, renameImpl?: Function, sleepImpl?: Function }} [options] + */ +export function renameReplacing(from, to, options = {}) { + const renameImpl = options.renameImpl ?? fs.renameSync; + if ((options.platform ?? process.platform) !== "win32") { + renameImpl(from, to); + return; + } + + const sleepImpl = options.sleepImpl ?? sleepSync; + for (let attempt = 0; ; attempt += 1) { + try { + renameImpl(from, to); + return; + } catch (error) { + if (attempt >= WINDOWS_RENAME_RETRY_DELAYS_MS.length || !WINDOWS_RENAME_RETRY_CODES.has(error?.code)) { + throw error; + } + sleepImpl(WINDOWS_RENAME_RETRY_DELAYS_MS[attempt]); + } + } +} + export function writeJsonFileAtomic(filePath, value) { const temporaryFile = `${filePath}.${process.pid}.${randomUUID()}.tmp`; try { @@ -52,7 +89,7 @@ export function writeJsonFileAtomic(filePath, value) { } finally { fs.closeSync(fd); } - fs.renameSync(temporaryFile, filePath); + renameReplacing(temporaryFile, filePath); setMode(filePath, PRIVATE_FILE_MODE); } catch (error) { fs.rmSync(temporaryFile, { force: true }); diff --git a/plugins/codex/scripts/lib/job-control.mjs b/plugins/codex/scripts/lib/job-control.mjs index 68dd0150f..0a7965cfc 100644 --- a/plugins/codex/scripts/lib/job-control.mjs +++ b/plugins/codex/scripts/lib/job-control.mjs @@ -377,3 +377,22 @@ export function resolveCancelableJob(cwd, reference, options = {}) { throw new Error("No active Codex jobs to cancel."); } + +/** + * Whether a cancel attempt actually stopped the job's work, and it's safe + * to record the job as cancelled and clear its pid. + * + * A successful turn interrupt is sufficient on its own -- Codex has already + * stopped acting on this turn regardless of what process-tree termination + * does afterward. Otherwise, termination must have completed without an + * unexpected throw: terminateProcessTree() already treats "process already + * gone" as non-fatal without throwing, so a throw here means the outcome is + * genuinely unknown, not just "already stopped." Reporting cancellation + * confirmed in that case (interrupt didn't succeed, and termination outcome + * is unknown) would let a write-capable task keep modifying the workspace + * unsupervised, with its later completion able to overwrite the fabricated + * cancelled status. + */ +export function wasCancellationConfirmed(interrupt, terminationOutcomeKnown) { + return Boolean(interrupt?.interrupted) || Boolean(terminationOutcomeKnown); +} diff --git a/plugins/codex/scripts/lib/process.mjs b/plugins/codex/scripts/lib/process.mjs index 1f42896aa..914c7387a 100644 --- a/plugins/codex/scripts/lib/process.mjs +++ b/plugins/codex/scripts/lib/process.mjs @@ -15,7 +15,13 @@ export function runCommand(command, args = [], options = {}) { timeout: options.timeout, killSignal: options.killSignal, stdio: options.stdio ?? "pipe", - shell: options.shell ?? (process.platform === "win32" ? (process.env.SHELL || true) : false), + // `process.env.SHELL` is a POSIX convention with no meaning for native + // Windows process creation; consulting it routed commands through whatever + // POSIX shell happened to be set (Git Bash, which Claude Code's own Bash + // tool sets), and MSYS path conversion then mangled Windows-style flags + // like `/PID`. Nothing is spawned through a shell here; a Windows `.cmd` + // shim goes through commandWithWindowsShim() instead. + shell: options.shell ?? false, windowsHide: true }); @@ -43,8 +49,33 @@ export function runCommandChecked(command, args = [], options = {}) { return result; } +export function commandWithWindowsShim(command, args = [], options = {}) { + const platform = options.platform ?? process.platform; + if (platform !== "win32") { + return { command, args, shell: false }; + } + + return { + command: options.comspec ?? process.env.ComSpec ?? "cmd.exe", + args: ["/d", "/s", "/c", "call", command, ...args], + shell: false + }; +} + export function binaryAvailable(command, versionArgs = ["--version"], options = {}) { - const result = runCommand(command, versionArgs, options); + const runCommandImpl = options.runCommandImpl ?? runCommand; + let result; + + if (options.shell !== undefined) { + result = runCommandImpl(command, versionArgs, options); + } else { + const invocation = commandWithWindowsShim(command, versionArgs, options); + result = runCommandImpl(invocation.command, invocation.args, { + cwd: options.cwd, + env: options.env, + shell: invocation.shell + }); + } if (result.error && /** @type {NodeJS.ErrnoException} */ (result.error).code === "ENOENT") { return { available: false, detail: "not found" }; } @@ -389,6 +420,52 @@ export function processHasLaunchToken(pid, token, options = {}) { return commandLine.includes(marker) && commandLine.includes(token); } +/** + * Force-kill a process and everything under it, for callers that have already tried a graceful stop. + * + * POSIX has a harder signal to escalate to, and the process group addresses the descendants: + * SIGKILL the group, falling back to the process alone for a caller that never was a group leader. + * + * Windows has neither. There are no process groups — a negative pid is just an invalid handle, so + * `process.kill(-pid)` throws ESRCH and a naive catch-and-retry silently degrades to killing the + * direct process while its descendants (the app-server, and every MCP server under it) keep + * running. taskkill's own tree walk is the only way to reach them, and `/F` is already the hardest + * stop there is, so the Windows branch is the same call as the graceful one. + */ +/** + * @param {number} pid + * @param {{ platform?: string, killImpl?: Function, runCommandImpl?: Function }} [options] + * @returns {{ attempted: boolean, delivered: boolean, method: string | null }} + */ +export function forceKillProcessTree(pid, options = {}) { + if (!isValidPid(pid)) { + return { attempted: false, delivered: false, method: null }; + } + + if ((options.platform ?? process.platform) === "win32") { + try { + const outcome = terminateProcessTree(pid, options); + return { attempted: outcome.attempted, delivered: outcome.delivered, method: outcome.method }; + } catch { + // Teardown is best effort: a taskkill that fails outright must not throw at the caller. + return { attempted: true, delivered: false, method: "taskkill" }; + } + } + + const killImpl = options.killImpl ?? process.kill.bind(process); + try { + killImpl(-pid, "SIGKILL"); + return { attempted: true, delivered: true, method: "process-group" }; + } catch { + try { + killImpl(pid, "SIGKILL"); + return { attempted: true, delivered: true, method: "process" }; + } catch { + return { attempted: true, delivered: false, method: "process" }; + } + } +} + /** * Terminate a process tree and make sure it is actually gone. * @@ -419,11 +496,9 @@ export function terminateProcessTreeAndExit(pid, { graceMs = 5000, exitCode = 1, } catch { // Never let bookkeeping stop the kill. } - try { - process.kill(-pid, "SIGKILL"); - } catch { - // Nothing left in the group, or the caller was never its leader. - } + // Nothing left in the group, a caller that was never its leader, or a Windows tree that + // taskkill could not reach: none of them may stop us from exiting. + forceKillProcessTree(pid); process.exit(exitCode); }, graceMs); } @@ -440,7 +515,8 @@ export function terminateProcessTree(pid, options = {}) { if (platform === "win32") { const result = runCommandImpl("taskkill", ["/PID", String(pid), "/T", "/F"], { cwd: options.cwd, - env: options.env + env: options.env, + shell: false }); if (!result.error && result.status === 0) { @@ -448,7 +524,7 @@ export function terminateProcessTree(pid, options = {}) { } const combinedOutput = `${result.stderr}\n${result.stdout}`.trim(); - if (!result.error && looksLikeMissingProcessMessage(combinedOutput)) { + if (!result.error && (result.status === 128 || looksLikeMissingProcessMessage(combinedOutput))) { return { attempted: true, delivered: false, method: "taskkill", result }; } diff --git a/plugins/codex/scripts/session-lifecycle-hook.mjs b/plugins/codex/scripts/session-lifecycle-hook.mjs index 114fcfc98..28e0b16d6 100644 --- a/plugins/codex/scripts/session-lifecycle-hook.mjs +++ b/plugins/codex/scripts/session-lifecycle-hook.mjs @@ -3,7 +3,7 @@ import fs from "node:fs"; import process from "node:process"; -import { isPidAlive, terminateProcessTree } from "./lib/process.mjs"; +import { forceKillProcessTree, isPidAlive, terminateProcessTree } from "./lib/process.mjs"; import { reconcileJobLiveness } from "./lib/job-control.mjs"; import { brokerIdleShutdownMs } from "./lib/lifecycle-limits.mjs"; import { BROKER_ENDPOINT_ENV } from "./lib/app-server.mjs"; @@ -50,15 +50,9 @@ async function waitForWorkerExits(pids) { return []; } for (const pid of stragglers) { - try { - process.kill(-pid, "SIGKILL"); - } catch { - try { - process.kill(pid, "SIGKILL"); - } catch { - // Ignore missing process. - } - } + // Not `process.kill(-pid)`: on Windows that is an invalid handle rather than a process group, + // and falling back to the bare pid would leave the worker's app-server subtree behind. + forceKillProcessTree(pid); } // Give the forced kill a beat to release the sockets. await new Promise((resolve) => setTimeout(resolve, 100)); diff --git a/tests/job-control.test.mjs b/tests/job-control.test.mjs index 576be7246..daf4beda6 100644 --- a/tests/job-control.test.mjs +++ b/tests/job-control.test.mjs @@ -10,7 +10,8 @@ import { buildStatusSnapshot, reconcileJobLiveness, resolveCancelableJob, - resolveResultJob + resolveResultJob, + wasCancellationConfirmed } from "../plugins/codex/scripts/lib/job-control.mjs"; function activeJob(overrides = {}) { @@ -199,3 +200,21 @@ test("implicit result ignores a newer dead worker in favor of stored final outpu fs.rmSync(root, { recursive: true, force: true }); } }); + +test("wasCancellationConfirmed is true when the turn interrupt succeeded, regardless of termination outcome", () => { + assert.equal(wasCancellationConfirmed({ interrupted: true }, false), true); + assert.equal(wasCancellationConfirmed({ interrupted: true }, true), true); +}); + +test("wasCancellationConfirmed is true when termination completed without throwing, even if the interrupt did not succeed", () => { + assert.equal(wasCancellationConfirmed({ interrupted: false }, true), true); +}); + +test("wasCancellationConfirmed is false when neither the interrupt succeeded nor termination's outcome is known", () => { + assert.equal(wasCancellationConfirmed({ interrupted: false }, false), false); +}); + +test("wasCancellationConfirmed treats a missing interrupt result as not interrupted", () => { + assert.equal(wasCancellationConfirmed(null, false), false); + assert.equal(wasCancellationConfirmed(undefined, true), true); +}); diff --git a/tests/process.test.mjs b/tests/process.test.mjs index 9b1bd8f98..c9f7f2924 100644 --- a/tests/process.test.mjs +++ b/tests/process.test.mjs @@ -1,9 +1,15 @@ +import fs from "node:fs"; +import path from "node:path"; import process from "node:process"; import test from "node:test"; import assert from "node:assert/strict"; import { spawn } from "node:child_process"; +import { fileURLToPath } from "node:url"; import { + binaryAvailable, + commandWithWindowsShim, + forceKillProcessTree, getProcessIdentity, isProcessRunning, isProcessTreeRunning, @@ -14,6 +20,8 @@ import { waitForProcessExit } from "../plugins/codex/scripts/lib/process.mjs"; +const ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), ".."); + const SELF_TERMINATING_SCRIPT = "process.kill(process.pid, 'SIGTERM'); setInterval(() => {}, 1000);"; test("runCommand reports a signal-terminated process as a failure", { skip: process.platform === "win32" }, () => { @@ -178,8 +186,8 @@ test("terminateProcessTree uses taskkill on Windows", () => { let captured = null; const outcome = terminateProcessTree(1234, { platform: "win32", - runCommandImpl(command, args) { - captured = { command, args }; + runCommandImpl(command, args, options) { + captured = { command, args, shell: options?.shell }; return { command, args, @@ -197,7 +205,11 @@ test("terminateProcessTree uses taskkill on Windows", () => { assert.deepEqual(captured, { command: "taskkill", - args: ["/PID", "1234", "/T", "/F"] + args: ["/PID", "1234", "/T", "/F"], + // Never through a shell: under Git Bash, MSYS path conversion rewrites + // /PID into a filesystem path and every kill fails with "Invalid + // argument/option", leaving background workers unkillable. + shell: false }); assert.equal(outcome.delivered, true); assert.equal(outcome.method, "taskkill"); @@ -288,3 +300,162 @@ test("a dead group leader with a surviving descendant still counts as a running assert.equal(exited, true, "cleanup must confirm the test process exited"); } }); + +test("commandWithWindowsShim avoids shell:true on Windows", () => { + assert.deepEqual( + commandWithWindowsShim("codex", ["app-server"], { + platform: "win32", + comspec: "C:\\Windows\\System32\\cmd.exe" + }), + { + command: "C:\\Windows\\System32\\cmd.exe", + args: ["/d", "/s", "/c", "call", "codex", "app-server"], + shell: false + } + ); +}); + +test("binaryAvailable uses cmd.exe explicitly for Windows command shims", () => { + let captured = null; + const outcome = binaryAvailable("npm", ["--version"], { + platform: "win32", + comspec: "C:\\Windows\\System32\\cmd.exe", + runCommandImpl(command, args, options) { + captured = { command, args, options }; + return { + command, + args, + status: 0, + signal: null, + stdout: "11.16.0\n", + stderr: "", + error: null + }; + } + }); + + assert.deepEqual(captured, { + command: "C:\\Windows\\System32\\cmd.exe", + args: ["/d", "/s", "/c", "call", "npm", "--version"], + options: { cwd: undefined, env: undefined, shell: false } + }); + assert.deepEqual(outcome, { available: true, detail: "11.16.0" }); +}); + +test("forceKillProcessTree reaches descendants on Windows instead of signalling a negative pid", () => { + let captured = null; + const outcome = forceKillProcessTree(1234, { + platform: "win32", + runCommandImpl(command, args, options) { + captured = { command, args, shell: options?.shell }; + return { command, args, status: 0, signal: null, stdout: "", stderr: "", error: null }; + }, + killImpl(pid) { + // Windows has no process groups: OpenProcess on a negative pid fails, so a + // catch-and-retry would degrade to killing the worker alone and orphan the + // app-server (and every MCP server) underneath it. + throw new Error(`process.kill must not be used on Windows (called with ${pid})`); + } + }); + + assert.deepEqual(captured, { + command: "taskkill", + args: ["/PID", "1234", "/T", "/F"], + shell: false + }); + assert.deepEqual(outcome, { attempted: true, delivered: true, method: "taskkill" }); +}); + +test("forceKillProcessTree never throws when the Windows tree cannot be reached", () => { + const outcome = forceKillProcessTree(1234, { + platform: "win32", + runCommandImpl(command, args) { + return { + command, + args, + status: 1, + signal: null, + stdout: "", + stderr: "ERROR: Access is denied.", + error: null + }; + } + }); + + assert.deepEqual(outcome, { attempted: true, delivered: false, method: "taskkill" }); +}); + +test("forceKillProcessTree SIGKILLs the process group on POSIX", () => { + const signalled = []; + const outcome = forceKillProcessTree(1234, { + platform: "linux", + killImpl(pid, signal) { + signalled.push({ pid, signal }); + } + }); + + assert.deepEqual(signalled, [{ pid: -1234, signal: "SIGKILL" }]); + assert.equal(outcome.method, "process-group"); + assert.equal(outcome.delivered, true); +}); + +test("forceKillProcessTree falls back to the process when it leads no group", () => { + const signalled = []; + const outcome = forceKillProcessTree(1234, { + platform: "linux", + killImpl(pid, signal) { + signalled.push({ pid, signal }); + if (pid < 0) { + const error = new Error("No such process"); + error.code = "ESRCH"; + throw error; + } + } + }); + + assert.deepEqual(signalled, [ + { pid: -1234, signal: "SIGKILL" }, + { pid: 1234, signal: "SIGKILL" } + ]); + assert.equal(outcome.method, "process"); + assert.equal(outcome.delivered, true); +}); + +test("no plugin script force-kills through a negative pid outside the platform-guarded helpers", () => { + // `kill(-pid, …)` addresses a process group, which exists only on POSIX; on Windows it is + // an invalid handle, and a catch-and-retry on the bare pid silently orphans the descendants. + // So a direct `process.kill(-pid)` is banned outright: group signalling belongs to the + // platform-guarded helpers in process.mjs, which send it through an injectable `killImpl` + // and pick taskkill on Windows. + const scriptsDir = path.join(ROOT, "plugins", "codex", "scripts"); + const direct = new Set(); + const viaKillImpl = new Set(); + const walk = (dir) => { + for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) { + walk(full); + continue; + } + if (!entry.name.endsWith(".mjs")) { + continue; + } + const relative = path.relative(ROOT, full).split(path.sep).join("/"); + for (const line of fs.readFileSync(full, "utf8").split("\n")) { + const code = line.trim(); + if (code.startsWith("//") || code.startsWith("*") || code.startsWith("/*")) { + continue; + } + if (/\bprocess\.kill\(\s*-/.test(code)) { + direct.add(relative); + } else if (/\bkill\w*\(\s*-/.test(code)) { + viaKillImpl.add(relative); + } + } + } + }; + walk(scriptsDir); + + assert.deepEqual([...direct], []); + assert.deepEqual([...viaKillImpl], ["plugins/codex/scripts/lib/process.mjs"]); +}); diff --git a/tests/state.test.mjs b/tests/state.test.mjs index 0097b069a..51b3c0ccd 100644 --- a/tests/state.test.mjs +++ b/tests/state.test.mjs @@ -9,6 +9,9 @@ import { fileURLToPath, pathToFileURL } from "node:url"; import { makeTempDir } from "./helpers.mjs"; import { readStoredJob } from "../plugins/codex/scripts/lib/job-control.mjs"; +import { renameReplacing } from "../plugins/codex/scripts/lib/fs.mjs"; + +const REPO_ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), ".."); import { acquireLockSync, releaseLock } from "../plugins/codex/scripts/lib/locking.mjs"; import { getConfig, @@ -720,3 +723,125 @@ test("disabling the review gate is not outvoted by a stranded enable in another assert.equal(getConfig(workspace).stopReviewGate, false); }); }); + +test("an atomic write retries a Windows sharing violation instead of losing the record", () => { + const attempts = []; + const waits = []; + renameReplacing("state.json.tmp", "state.json", { + platform: "win32", + renameImpl(from, to) { + attempts.push({ from, to }); + if (attempts.length <= 2) { + // What a scanner or indexer holding the target open looks like from here. + const error = new Error("EPERM: operation not permitted, rename"); + error.code = "EPERM"; + throw error; + } + }, + sleepImpl(ms) { + waits.push(ms); + } + }); + + assert.equal(attempts.length, 3); + assert.deepEqual(attempts.at(-1), { from: "state.json.tmp", to: "state.json" }); + assert.deepEqual(waits, [5, 15]); +}); + +test("an atomic write gives up on a Windows sharing violation that never clears", () => { + let attempts = 0; + assert.throws( + () => + renameReplacing("state.json.tmp", "state.json", { + platform: "win32", + renameImpl() { + attempts += 1; + const error = new Error("EBUSY: resource busy or locked, rename"); + error.code = "EBUSY"; + throw error; + }, + sleepImpl() {} + }), + /EBUSY/ + ); + + // Four waits, five attempts: the budget is bounded, and the last failure is the caller's. + assert.equal(attempts, 5); +}); + +test("an atomic write does not retry errors that are not sharing violations", () => { + for (const platform of ["win32", "linux"]) { + let attempts = 0; + assert.throws( + () => + renameReplacing("state.json.tmp", "state.json", { + platform, + renameImpl() { + attempts += 1; + const error = new Error("ENOENT: no such file or directory, rename"); + error.code = "ENOENT"; + throw error; + }, + sleepImpl() { + throw new Error("must not wait"); + } + }), + /ENOENT/ + ); + assert.equal(attempts, 1, `${platform} must fail on the first attempt`); + } +}); + +test("outside Windows an atomic write renames once and reports the failure", () => { + let attempts = 0; + assert.throws( + () => + renameReplacing("state.json.tmp", "state.json", { + platform: "linux", + renameImpl() { + attempts += 1; + const error = new Error("EPERM: operation not permitted, rename"); + error.code = "EPERM"; + throw error; + }, + sleepImpl() { + throw new Error("POSIX rename has no sharing violation to wait out"); + } + }), + /EPERM/ + ); + assert.equal(attempts, 1); +}); + +test("plugin scripts replace files through the Windows-aware rename", () => { + // A bare fs.renameSync() over an existing file is the call that Windows can refuse with a + // sharing violation, so file replacement goes through renameReplacing(), which retries it. + // locking.mjs is the exception: its directory rename already reads EPERM as "someone else + // holds the lock", which is exactly what it means there. + const scriptsDir = path.join(REPO_ROOT, "plugins", "codex", "scripts"); + const callers = new Set(); + const walk = (dir) => { + for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) { + walk(full); + continue; + } + if (!entry.name.endsWith(".mjs")) { + continue; + } + for (const line of fs.readFileSync(full, "utf8").split("\n")) { + const code = line.trim(); + if (code.startsWith("//") || code.startsWith("*") || code.startsWith("/*")) { + continue; + } + if (/\bfs\.renameSync\s*\(/.test(code)) { + callers.add(path.relative(REPO_ROOT, full).split(path.sep).join("/")); + } + } + } + }; + walk(scriptsDir); + + assert.deepEqual([...callers].sort(), ["plugins/codex/scripts/lib/locking.mjs"]); +});