From b01826fdb5977c58487eff77a4a97fb46184696c Mon Sep 17 00:00:00 2001 From: Sanai Date: Mon, 21 Sep 2026 00:00:16 +0200 Subject: [PATCH 1/2] Fix Windows SessionEnd hang and taskkill false failures sendBrokerShutdown waited forever when the broker accepted the connection but never replied, so the SessionEnd hook never returned (observed as "Hook cancelled"). It now gives up after a timeout and destroys the socket; a regression test covers a mute broker. terminateProcessTree treated any non-zero taskkill status as a failure. On Windows, taskkill /T enumerates the tree and then terminates each entry; a short-lived descendant (git/cmd helpers spawned by the worker) that exits in between makes taskkill report "The operation attempted is not supported" with status 128 even though the root process was killed. The root's liveness is now checked before treating that as an error, which fixes the flaky cancel integration test (reproduced 7/8 runs). Also make the unix endpoint path POSIX-joined and adapt Windows test fixtures: skip the broken-symlink case without Developer Mode, use a node.cmd shim instead of a symlink, and propagate USERPROFILE. Full suite on Windows 11 / Node 26.1.0: 93 passed, 0 failed, 1 skipped. Co-Authored-By: Claude Opus 5 --- plugins/codex/scripts/lib/broker-endpoint.mjs | 2 +- .../codex/scripts/lib/broker-lifecycle.mjs | 17 +++++- plugins/codex/scripts/lib/process.mjs | 16 ++++++ tests/broker-lifecycle.test.mjs | 43 +++++++++++++++ tests/git.test.mjs | 18 +++++- tests/process.test.mjs | 55 +++++++++++++++++++ tests/runtime.test.mjs | 11 +++- 7 files changed, 154 insertions(+), 8 deletions(-) create mode 100644 tests/broker-lifecycle.test.mjs diff --git a/plugins/codex/scripts/lib/broker-endpoint.mjs b/plugins/codex/scripts/lib/broker-endpoint.mjs index 8abdcc71a..664c58494 100644 --- a/plugins/codex/scripts/lib/broker-endpoint.mjs +++ b/plugins/codex/scripts/lib/broker-endpoint.mjs @@ -13,7 +13,7 @@ export function createBrokerEndpoint(sessionDir, platform = process.platform) { return `pipe:\\\\.\\pipe\\${pipeName}`; } - return `unix:${path.join(sessionDir, "broker.sock")}`; + return `unix:${path.posix.join(sessionDir, "broker.sock")}`; } export function parseBrokerEndpoint(endpoint) { diff --git a/plugins/codex/scripts/lib/broker-lifecycle.mjs b/plugins/codex/scripts/lib/broker-lifecycle.mjs index ef763819c..184e98aed 100644 --- a/plugins/codex/scripts/lib/broker-lifecycle.mjs +++ b/plugins/codex/scripts/lib/broker-lifecycle.mjs @@ -40,19 +40,30 @@ export async function waitForBrokerEndpoint(endpoint, timeoutMs = 2000) { return false; } -export async function sendBrokerShutdown(endpoint) { +export async function sendBrokerShutdown(endpoint, timeoutMs = 2000) { await new Promise((resolve) => { const socket = connectToEndpoint(endpoint); + const timer = setTimeout(() => { + socket.destroy(); + resolve(); + }, timeoutMs); socket.setEncoding("utf8"); socket.on("connect", () => { socket.write(`${JSON.stringify({ id: 1, method: "broker/shutdown", params: {} })}\n`); }); socket.on("data", () => { + clearTimeout(timer); socket.end(); resolve(); }); - socket.on("error", resolve); - socket.on("close", resolve); + socket.on("error", () => { + clearTimeout(timer); + resolve(); + }); + socket.on("close", () => { + clearTimeout(timer); + resolve(); + }); }); } diff --git a/plugins/codex/scripts/lib/process.mjs b/plugins/codex/scripts/lib/process.mjs index dd8fc3751..8b4a6370d 100644 --- a/plugins/codex/scripts/lib/process.mjs +++ b/plugins/codex/scripts/lib/process.mjs @@ -54,6 +54,15 @@ function looksLikeMissingProcessMessage(text) { return /not found|no running instance|cannot find|does not exist|no such process/i.test(text); } +function isProcessAlive(pid, killImpl) { + try { + killImpl(pid, 0); + return true; + } catch (error) { + return error?.code !== "ESRCH"; + } +} + export function terminateProcessTree(pid, options = {}) { if (!Number.isFinite(pid)) { return { attempted: false, delivered: false, method: null }; @@ -78,6 +87,13 @@ export function terminateProcessTree(pid, options = {}) { return { attempted: true, delivered: false, method: "taskkill", result }; } + // taskkill /T walks the tree, then terminates each entry; a descendant that exits in + // between (short-lived git/cmd helpers) makes taskkill report "not supported" and a + // non-zero status even though the root was killed. Trust the root's liveness instead. + if (!result.error && !isProcessAlive(pid, killImpl)) { + return { attempted: true, delivered: true, method: "taskkill", result }; + } + if (result.error?.code === "ENOENT") { try { killImpl(pid); diff --git a/tests/broker-lifecycle.test.mjs b/tests/broker-lifecycle.test.mjs new file mode 100644 index 000000000..e9e779d36 --- /dev/null +++ b/tests/broker-lifecycle.test.mjs @@ -0,0 +1,43 @@ +import fs from "node:fs"; +import net from "node:net"; +import os from "node:os"; +import path from "node:path"; +import test from "node:test"; +import assert from "node:assert/strict"; + +import { sendBrokerShutdown } from "../plugins/codex/scripts/lib/broker-lifecycle.mjs"; +import { createBrokerEndpoint, parseBrokerEndpoint } from "../plugins/codex/scripts/lib/broker-endpoint.mjs"; + +test("sendBrokerShutdown times out when the broker accepts but never responds", async (t) => { + const sessionDir = fs.mkdtempSync(path.join(os.tmpdir(), "codex-plugin-broker-test-")); + const endpoint = createBrokerEndpoint(sessionDir); + const { path: endpointPath } = parseBrokerEndpoint(endpoint); + const sockets = new Set(); + const server = net.createServer((socket) => { + sockets.add(socket); + socket.on("close", () => sockets.delete(socket)); + }); + + t.after(async () => { + for (const socket of sockets) { + socket.destroy(); + } + await new Promise((resolve) => server.close(resolve)); + if (process.platform !== "win32" && fs.existsSync(endpointPath)) { + fs.unlinkSync(endpointPath); + } + fs.rmSync(sessionDir, { recursive: true, force: true }); + }); + + await new Promise((resolve, reject) => { + server.once("error", reject); + server.listen(endpointPath, resolve); + }); + + const startedAt = Date.now(); + await sendBrokerShutdown(endpoint, 50); + const elapsedMs = Date.now() - startedAt; + + assert.ok(elapsedMs >= 40, `expected timeout path, completed after ${elapsedMs}ms`); + assert.ok(elapsedMs < 1000, `shutdown timeout exceeded test budget: ${elapsedMs}ms`); +}); diff --git a/tests/git.test.mjs b/tests/git.test.mjs index 5b5c266ee..09e1c367a 100644 --- a/tests/git.test.mjs +++ b/tests/git.test.mjs @@ -132,13 +132,27 @@ test("collectReviewContext skips untracked directories in working tree review", assert.match(context.content, /### \.claude\/worktrees\/agent-test\/\n\(skipped: directory\)/); }); -test("collectReviewContext skips broken untracked symlinks instead of crashing", () => { +test("collectReviewContext skips broken untracked symlinks instead of crashing", (t) => { const cwd = makeTempDir(); initGitRepo(cwd); fs.writeFileSync(path.join(cwd, "app.js"), "console.log('v1');\n"); run("git", ["add", "app.js"], { cwd }); run("git", ["commit", "-m", "init"], { cwd }); - fs.symlinkSync("missing-target", path.join(cwd, "broken-link")); + try { + fs.symlinkSync("missing-target", path.join(cwd, "broken-link")); + } catch (error) { + if (process.platform === "win32" && error?.code === "EPERM") { + t.skip("Windows requires Developer Mode or elevated privileges to create this symlink fixture."); + return; + } + throw error; + } + + const untracked = run("git", ["ls-files", "--others", "--exclude-standard"], { cwd }).stdout; + if (!untracked.split(/\r?\n/).includes("broken-link")) { + t.skip("Git does not report broken symlinks as untracked in this environment."); + return; + } const target = resolveReviewTarget(cwd, {}); const context = collectReviewContext(cwd, target); diff --git a/tests/process.test.mjs b/tests/process.test.mjs index 80e0715b0..b19259005 100644 --- a/tests/process.test.mjs +++ b/tests/process.test.mjs @@ -53,3 +53,58 @@ test("terminateProcessTree treats missing Windows processes as already stopped", assert.equal(outcome.result.status, 128); assert.match(outcome.result.stdout, /not found/i); }); + +test("terminateProcessTree reports delivery when taskkill only failed on already-exiting descendants", () => { + const outcome = terminateProcessTree(1234, { + platform: "win32", + runCommandImpl(command, args) { + return { + command, + args, + status: 128, + signal: null, + stdout: "SUCCESS: The process with PID 1234 has been terminated.", + stderr: + "ERROR: The process with PID 5678 (child process of PID 1234) could not be terminated.\n" + + "Reason: The operation attempted is not supported.", + error: null + }; + }, + killImpl(pid, signal) { + assert.equal(pid, 1234); + assert.equal(signal, 0); + const error = new Error("ESRCH"); + error.code = "ESRCH"; + throw error; + } + }); + + assert.equal(outcome.attempted, true); + assert.equal(outcome.delivered, true); + assert.equal(outcome.method, "taskkill"); + assert.equal(outcome.result.status, 128); +}); + +test("terminateProcessTree still throws when taskkill fails and the root process survives", () => { + assert.throws( + () => + terminateProcessTree(1234, { + platform: "win32", + runCommandImpl(command, args) { + return { + command, + args, + status: 128, + signal: null, + stdout: "", + stderr: "ERROR: The process with PID 1234 could not be terminated.\nReason: Access is denied.", + error: null + }; + }, + killImpl() { + return true; + } + }), + /could not be terminated/ + ); +}); diff --git a/tests/runtime.test.mjs b/tests/runtime.test.mjs index 8f276835b..541834b2d 100644 --- a/tests/runtime.test.mjs +++ b/tests/runtime.test.mjs @@ -47,7 +47,11 @@ test("setup reports ready when fake codex is installed and authenticated", () => test("setup is ready without npm when Codex is already installed and authenticated", () => { const binDir = makeTempDir(); installFakeCodex(binDir); - fs.symlinkSync(process.execPath, path.join(binDir, "node")); + if (process.platform === "win32") { + fs.writeFileSync(path.join(binDir, "node.cmd"), `@echo off\r\n"${process.execPath}" %*\r\n`, "utf8"); + } else { + fs.symlinkSync(process.execPath, path.join(binDir, "node")); + } const result = run("node", [SCRIPT, "setup", "--json"], { cwd: ROOT, @@ -220,6 +224,7 @@ test("transfer delegates the current Claude session directly to native import", env: { ...buildEnv(binDir), HOME: home, + USERPROFILE: home, CODEX_HOME: path.join(home, ".codex"), CODEX_COMPANION_TRANSCRIPT_PATH: sourcePath } @@ -265,6 +270,7 @@ test("transfer reports an actionable upgrade error when native import is unsuppo env: { ...buildEnv(binDir), HOME: home, + USERPROFILE: home, CODEX_HOME: path.join(home, ".codex") } }); @@ -295,6 +301,7 @@ test("transfer fails visibly when native import completes without a ledger recor env: { ...buildEnv(binDir), HOME: home, + USERPROFILE: home, CODEX_HOME: path.join(home, ".codex") } }); @@ -320,7 +327,7 @@ test("transfer rejects sources outside the Claude projects directory", () => { const result = run("node", [SCRIPT, "transfer", "--source", sourcePath], { cwd: repo, - env: { ...buildEnv(binDir), HOME: home } + env: { ...buildEnv(binDir), HOME: home, USERPROFILE: home } }); assert.notEqual(result.status, 0); From 92d9debf10f2d9f3c95723b9a055efc9bbde58db Mon Sep 17 00:00:00 2001 From: Sanai Date: Mon, 21 Sep 2026 03:21:28 +0200 Subject: [PATCH 2/2] Probe root liveness before taskkill instead of parsing its output Review of the previous commit found that terminateProcessTree could report delivered: true for a Windows process that was already gone before the call: looksLikeMissingProcessMessage only recognised English taskkill messages, so on a localised system (French "introuvable") the "not found" status fell through to the liveness check and was taken as a successful kill. terminateProcessTree now probes the root with process.kill(pid, 0) before running taskkill. A root that is already absent returns { attempted: false, delivered: false, method: null } without invoking taskkill, in every system language; looksLikeMissingProcessMessage and the text-based branch are removed. EPERM on the probe still means the process exists, so taskkill runs as before. The post-taskkill liveness check and the ENOENT fallback are unchanged. Comments now state the scope of delivered: it describes the root only, like the process-group SIGTERM on other platforms, and does not prove that every descendant is gone. Narrow TOCTOU windows (root exiting on its own between probe and taskkill, PID reuse before the second probe) are accepted; none of the callers reads the return value. Tests: French taskkill output with liveness probes, taskkill skipped when the root is already absent, EPERM on the probe not treated as absent, ENOENT fallback preserved, non-Windows path unchanged; existing mocks now lock the killImpl(pid, 0) call. Against the previous process.mjs, 4 of the 8 tests fail. Windows 11 / Node 26.1.0, run from native cmd.exe: process.test.mjs 8 passed; cancel integration test 3/3; full suite 98 tests, 97 passed, 0 failed, 1 skipped. Co-Authored-By: Claude Opus 5 --- plugins/codex/scripts/lib/process.mjs | 17 ++- tests/process.test.mjs | 150 ++++++++++++++++++++++++-- 2 files changed, 150 insertions(+), 17 deletions(-) diff --git a/plugins/codex/scripts/lib/process.mjs b/plugins/codex/scripts/lib/process.mjs index 8b4a6370d..ab09ec9f3 100644 --- a/plugins/codex/scripts/lib/process.mjs +++ b/plugins/codex/scripts/lib/process.mjs @@ -50,10 +50,6 @@ export function binaryAvailable(command, versionArgs = ["--version"], options = return { available: true, detail: result.stdout.trim() || result.stderr.trim() || "ok" }; } -function looksLikeMissingProcessMessage(text) { - return /not found|no running instance|cannot find|does not exist|no such process/i.test(text); -} - function isProcessAlive(pid, killImpl) { try { killImpl(pid, 0); @@ -73,6 +69,12 @@ export function terminateProcessTree(pid, options = {}) { const killImpl = options.killImpl ?? process.kill.bind(process); if (platform === "win32") { + // Probe the root before calling taskkill instead of parsing its localized "not found" + // message: a root that is already gone is reported as not delivered in every language. + if (!isProcessAlive(pid, killImpl)) { + return { attempted: false, delivered: false, method: null }; + } + const result = runCommandImpl("taskkill", ["/PID", String(pid), "/T", "/F"], { cwd: options.cwd, env: options.env @@ -82,14 +84,11 @@ export function terminateProcessTree(pid, options = {}) { return { attempted: true, delivered: 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 }; - } - // taskkill /T walks the tree, then terminates each entry; a descendant that exits in // between (short-lived git/cmd helpers) makes taskkill report "not supported" and a // non-zero status even though the root was killed. Trust the root's liveness instead. + // `delivered` therefore describes the root only: like the process-group SIGTERM on + // other platforms, it does not prove that every descendant is gone. if (!result.error && !isProcessAlive(pid, killImpl)) { return { attempted: true, delivered: true, method: "taskkill", result }; } diff --git a/tests/process.test.mjs b/tests/process.test.mjs index b19259005..5e7d608f4 100644 --- a/tests/process.test.mjs +++ b/tests/process.test.mjs @@ -19,8 +19,9 @@ test("terminateProcessTree uses taskkill on Windows", () => { error: null }; }, - killImpl() { - throw new Error("kill fallback should not run"); + killImpl(pid, signal) { + assert.equal(pid, 1234); + assert.equal(signal, 0); } }); @@ -32,7 +33,8 @@ test("terminateProcessTree uses taskkill on Windows", () => { assert.equal(outcome.method, "taskkill"); }); -test("terminateProcessTree treats missing Windows processes as already stopped", () => { +test("terminateProcessTree uses liveness instead of localized taskkill output", () => { + let livenessChecks = 0; const outcome = terminateProcessTree(1234, { platform: "win32", runCommandImpl(command, args) { @@ -41,20 +43,33 @@ test("terminateProcessTree treats missing Windows processes as already stopped", args, status: 128, signal: null, - stdout: "ERROR: The process \"1234\" not found.", - stderr: "", + stdout: "", + stderr: "Erreur : le processus \"1234\" est introuvable.", error: null }; + }, + killImpl(pid, signal) { + assert.equal(pid, 1234); + assert.equal(signal, 0); + livenessChecks += 1; + if (livenessChecks === 1) { + return; + } + const error = new Error("ESRCH"); + error.code = "ESRCH"; + throw error; } }); assert.equal(outcome.attempted, true); + assert.equal(outcome.delivered, true); assert.equal(outcome.method, "taskkill"); assert.equal(outcome.result.status, 128); - assert.match(outcome.result.stdout, /not found/i); + assert.equal(livenessChecks, 2); }); test("terminateProcessTree reports delivery when taskkill only failed on already-exiting descendants", () => { + let livenessChecks = 0; const outcome = terminateProcessTree(1234, { platform: "win32", runCommandImpl(command, args) { @@ -73,6 +88,10 @@ test("terminateProcessTree reports delivery when taskkill only failed on already killImpl(pid, signal) { assert.equal(pid, 1234); assert.equal(signal, 0); + livenessChecks += 1; + if (livenessChecks === 1) { + return; + } const error = new Error("ESRCH"); error.code = "ESRCH"; throw error; @@ -83,6 +102,7 @@ test("terminateProcessTree reports delivery when taskkill only failed on already assert.equal(outcome.delivered, true); assert.equal(outcome.method, "taskkill"); assert.equal(outcome.result.status, 128); + assert.equal(livenessChecks, 2); }); test("terminateProcessTree still throws when taskkill fails and the root process survives", () => { @@ -101,10 +121,124 @@ test("terminateProcessTree still throws when taskkill fails and the root process error: null }; }, - killImpl() { - return true; + killImpl(pid, signal) { + assert.equal(pid, 1234); + assert.equal(signal, 0); } }), /could not be terminated/ ); }); + +test("terminateProcessTree skips taskkill when the Windows process is already absent", () => { + let taskkillCalled = false; + const outcome = terminateProcessTree(1234, { + platform: "win32", + runCommandImpl(command, args) { + taskkillCalled = true; + return { + command, + args, + status: 128, + signal: null, + stdout: "", + stderr: "Erreur : le processus \"1234\" est introuvable.", + error: null + }; + }, + killImpl(pid, signal) { + assert.equal(pid, 1234); + assert.equal(signal, 0); + const error = new Error("ESRCH"); + error.code = "ESRCH"; + throw error; + } + }); + + assert.equal(taskkillCalled, false); + assert.deepEqual(outcome, { + attempted: false, + delivered: false, + method: null + }); +}); + +test("terminateProcessTree does not treat a Windows preflight permission error as missing", () => { + let taskkillCalled = false; + const outcome = terminateProcessTree(1234, { + platform: "win32", + runCommandImpl(command, args) { + taskkillCalled = true; + return { + command, + args, + status: 0, + signal: null, + stdout: "", + stderr: "", + error: null + }; + }, + killImpl(pid, signal) { + assert.equal(pid, 1234); + assert.equal(signal, 0); + const error = new Error("EPERM"); + error.code = "EPERM"; + throw error; + } + }); + + assert.equal(taskkillCalled, true); + assert.equal(outcome.delivered, true); + assert.equal(outcome.method, "taskkill"); +}); + +test("terminateProcessTree preserves the Windows ENOENT fallback", () => { + const killCalls = []; + const outcome = terminateProcessTree(1234, { + platform: "win32", + runCommandImpl(command, args) { + const error = new Error("ENOENT"); + error.code = "ENOENT"; + return { + command, + args, + status: 0, + signal: null, + stdout: "", + stderr: "", + error + }; + }, + killImpl(pid, signal) { + killCalls.push([pid, signal]); + } + }); + + assert.deepEqual(killCalls, [ + [1234, 0], + [1234, undefined] + ]); + assert.deepEqual(outcome, { + attempted: true, + delivered: true, + method: "kill" + }); +}); + +test("terminateProcessTree leaves the non-Windows process-group path unchanged", () => { + const killCalls = []; + const outcome = terminateProcessTree(1234, { + platform: "linux", + killImpl(pid, signal) { + killCalls.push([pid, signal]); + } + }); + + assert.deepEqual(killCalls, [[-1234, "SIGTERM"]]); + assert.deepEqual(outcome, { + attempted: true, + delivered: true, + method: "process-group" + }); +});