From 878c86314fce98fd9a7071a0581be87a31e66331 Mon Sep 17 00:00:00 2001 From: Heringer Date: Wed, 22 Jul 2026 17:46:16 -0300 Subject: [PATCH 01/20] Fix test broker leaks, state races, and signal-masked failures - tests/helpers.mjs: track temp workspaces and add a global after() teardown that shuts down any broker a test left running (graceful shutdown, then terminateProcessTree escalation) and removes its session dir; the suite now fails loudly if a broker survives teardown. - tests/runtime.test.mjs: the lazy-broker status test now runs the SessionEnd hook like its neighbors instead of leaking its broker. - lib/locking.mjs (new): minimal mkdir-based advisory lock with stale reclaim, sync and async variants. - lib/state.mjs: serialize saveState/updateState per workspace and write state.json via temp file + fsync + atomic rename, so concurrent read-modify-write cycles no longer lose updates or tear the file. - lib/broker-lifecycle.mjs: serialize ensureBrokerSession's check-then-create sequence per workspace so concurrent callers cannot spawn duplicate brokers. - lib/process.mjs: stop coalescing a null spawnSync status to 0 when the process died from a signal; runCommandChecked now reports it as a failure. - New regression tests: signal-terminated commands, concurrent upsertJob from two processes, concurrent ensureBrokerSession sharing one broker. --- .../codex/scripts/lib/broker-lifecycle.mjs | 11 +++ plugins/codex/scripts/lib/locking.mjs | 88 +++++++++++++++++++ plugins/codex/scripts/lib/process.mjs | 4 +- plugins/codex/scripts/lib/state.mjs | 41 +++++++-- tests/broker-lifecycle.test.mjs | 39 ++++++++ tests/helpers.mjs | 80 ++++++++++++++++- tests/process.test.mjs | 19 +++- tests/runtime.test.mjs | 10 +++ tests/state.test.mjs | 49 ++++++++++- 9 files changed, 331 insertions(+), 10 deletions(-) create mode 100644 plugins/codex/scripts/lib/locking.mjs create mode 100644 tests/broker-lifecycle.test.mjs diff --git a/plugins/codex/scripts/lib/broker-lifecycle.mjs b/plugins/codex/scripts/lib/broker-lifecycle.mjs index ef763819c..c71559a14 100644 --- a/plugins/codex/scripts/lib/broker-lifecycle.mjs +++ b/plugins/codex/scripts/lib/broker-lifecycle.mjs @@ -6,6 +6,7 @@ import process from "node:process"; import { spawn } from "node:child_process"; import { fileURLToPath } from "node:url"; import { createBrokerEndpoint, parseBrokerEndpoint } from "./broker-endpoint.mjs"; +import { withLock } from "./locking.mjs"; import { resolveStateDir } from "./state.mjs"; export const PID_FILE_ENV = "CODEX_COMPANION_APP_SERVER_PID_FILE"; @@ -111,6 +112,16 @@ async function isBrokerEndpointReady(endpoint) { } export async function ensureBrokerSession(cwd, options = {}) { + const stateDir = resolveStateDir(cwd); + fs.mkdirSync(stateDir, { recursive: true }); + // Serialize the check-then-create sequence per workspace so two concurrent + // callers cannot both miss the existing broker and spawn a duplicate. + return withLock(path.join(stateDir, ".broker.lock"), () => ensureBrokerSessionLocked(cwd, options), { + timeoutMs: options.lockTimeoutMs ?? 10000 + }); +} + +async function ensureBrokerSessionLocked(cwd, options = {}) { const existing = loadBrokerSession(cwd); if (existing && (await isBrokerEndpointReady(existing.endpoint))) { return existing; diff --git a/plugins/codex/scripts/lib/locking.mjs b/plugins/codex/scripts/lib/locking.mjs new file mode 100644 index 000000000..395b41ac0 --- /dev/null +++ b/plugins/codex/scripts/lib/locking.mjs @@ -0,0 +1,88 @@ +import fs from "node:fs"; + +const DEFAULT_TIMEOUT_MS = 5000; +const DEFAULT_STALE_MS = 30000; +const RETRY_DELAY_MS = 25; + +function sleepSync(ms) { + Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, ms); +} + +function sleep(ms) { + return new Promise((resolve) => setTimeout(resolve, ms)); +} + +function tryAcquire(lockDir) { + try { + fs.mkdirSync(lockDir); + return true; + } catch (error) { + if (error?.code === "EEXIST") { + return false; + } + throw error; + } +} + +function reclaimIfStale(lockDir, staleMs) { + try { + const age = Date.now() - fs.statSync(lockDir).mtimeMs; + if (age > staleMs) { + fs.rmdirSync(lockDir); + } + } catch { + // Lock released (or reclaimed by someone else) between stat and rmdir. + } +} + +export function acquireLockSync(lockDir, options = {}) { + const timeoutMs = options.timeoutMs ?? DEFAULT_TIMEOUT_MS; + const staleMs = options.staleMs ?? DEFAULT_STALE_MS; + const deadline = Date.now() + timeoutMs; + while (!tryAcquire(lockDir)) { + reclaimIfStale(lockDir, staleMs); + if (Date.now() >= deadline) { + throw new Error(`Timed out waiting for lock: ${lockDir}`); + } + sleepSync(RETRY_DELAY_MS); + } +} + +export async function acquireLock(lockDir, options = {}) { + const timeoutMs = options.timeoutMs ?? DEFAULT_TIMEOUT_MS; + const staleMs = options.staleMs ?? DEFAULT_STALE_MS; + const deadline = Date.now() + timeoutMs; + while (!tryAcquire(lockDir)) { + reclaimIfStale(lockDir, staleMs); + if (Date.now() >= deadline) { + throw new Error(`Timed out waiting for lock: ${lockDir}`); + } + await sleep(RETRY_DELAY_MS); + } +} + +export function releaseLock(lockDir) { + try { + fs.rmdirSync(lockDir); + } catch { + // Already released or reclaimed as stale; nothing left to do. + } +} + +export function withLockSync(lockDir, fn, options = {}) { + acquireLockSync(lockDir, options); + try { + return fn(); + } finally { + releaseLock(lockDir); + } +} + +export async function withLock(lockDir, fn, options = {}) { + await acquireLock(lockDir, options); + try { + return await fn(); + } finally { + releaseLock(lockDir); + } +} diff --git a/plugins/codex/scripts/lib/process.mjs b/plugins/codex/scripts/lib/process.mjs index dd8fc3751..1fa9a8d7f 100644 --- a/plugins/codex/scripts/lib/process.mjs +++ b/plugins/codex/scripts/lib/process.mjs @@ -16,7 +16,9 @@ export function runCommand(command, args = [], options = {}) { return { command, args, - status: result.status ?? 0, + // A null status with a signal means the process was killed; never report + // signal-terminated commands as exit 0. + status: result.status ?? (result.signal != null ? 1 : 0), signal: result.signal ?? null, stdout: result.stdout ?? "", stderr: result.stderr ?? "", diff --git a/plugins/codex/scripts/lib/state.mjs b/plugins/codex/scripts/lib/state.mjs index 2da23498f..ac24ca878 100644 --- a/plugins/codex/scripts/lib/state.mjs +++ b/plugins/codex/scripts/lib/state.mjs @@ -3,6 +3,7 @@ import fs from "node:fs"; import os from "node:os"; import path from "node:path"; +import { withLockSync } from "./locking.mjs"; import { resolveWorkspaceRoot } from "./workspace.mjs"; const STATE_VERSION = 1; @@ -89,9 +90,29 @@ function removeFileIfExists(filePath) { } } -export function saveState(cwd, state) { +function resolveStateLockDir(cwd) { + return path.join(resolveStateDir(cwd), ".state.lock"); +} + +function writeStateFileAtomic(stateFile, nextState) { + const tmpFile = `${stateFile}.${process.pid}.${Math.random().toString(36).slice(2)}.tmp`; + try { + const fd = fs.openSync(tmpFile, "w"); + try { + fs.writeFileSync(fd, `${JSON.stringify(nextState, null, 2)}\n`, "utf8"); + fs.fsyncSync(fd); + } finally { + fs.closeSync(fd); + } + fs.renameSync(tmpFile, stateFile); + } catch (error) { + removeFileIfExists(tmpFile); + throw error; + } +} + +function saveStateLocked(cwd, state) { const previousJobs = loadState(cwd).jobs; - ensureStateDir(cwd); const nextJobs = pruneJobs(state.jobs ?? []); const nextState = { version: STATE_VERSION, @@ -111,14 +132,22 @@ export function saveState(cwd, state) { removeFileIfExists(job.logFile); } - fs.writeFileSync(resolveStateFile(cwd), `${JSON.stringify(nextState, null, 2)}\n`, "utf8"); + writeStateFileAtomic(resolveStateFile(cwd), nextState); return nextState; } +export function saveState(cwd, state) { + ensureStateDir(cwd); + return withLockSync(resolveStateLockDir(cwd), () => saveStateLocked(cwd, state)); +} + export function updateState(cwd, mutate) { - const state = loadState(cwd); - mutate(state); - return saveState(cwd, state); + ensureStateDir(cwd); + return withLockSync(resolveStateLockDir(cwd), () => { + const state = loadState(cwd); + mutate(state); + return saveStateLocked(cwd, state); + }); } export function generateJobId(prefix = "job") { diff --git a/tests/broker-lifecycle.test.mjs b/tests/broker-lifecycle.test.mjs new file mode 100644 index 000000000..2f9f44f29 --- /dev/null +++ b/tests/broker-lifecycle.test.mjs @@ -0,0 +1,39 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +import { buildEnv, installFakeCodex } from "./fake-codex-fixture.mjs"; +import { makeTempDir } from "./helpers.mjs"; +import { + ensureBrokerSession, + loadBrokerSession, + sendBrokerShutdown +} from "../plugins/codex/scripts/lib/broker-lifecycle.mjs"; + +test("concurrent ensureBrokerSession calls share a single broker", async () => { + const workspace = makeTempDir(); + const binDir = makeTempDir(); + installFakeCodex(binDir); + const env = buildEnv(binDir); + + let first = null; + try { + const [left, right] = await Promise.all([ + ensureBrokerSession(workspace, { env }), + ensureBrokerSession(workspace, { env }) + ]); + first = left ?? right; + + assert.ok(left, "first ensureBrokerSession returned no session"); + assert.ok(right, "second ensureBrokerSession returned no session"); + assert.equal(left.endpoint, right.endpoint); + assert.equal(left.pid, right.pid); + + const persisted = loadBrokerSession(workspace); + assert.ok(persisted); + assert.equal(persisted.endpoint, left.endpoint); + } finally { + if (first?.endpoint) { + await sendBrokerShutdown(first.endpoint); + } + } +}); diff --git a/tests/helpers.mjs b/tests/helpers.mjs index d6981197a..15f6b9852 100644 --- a/tests/helpers.mjs +++ b/tests/helpers.mjs @@ -3,11 +3,89 @@ import os from "node:os"; import path from "node:path"; import process from "node:process"; import { spawnSync } from "node:child_process"; +import { after } from "node:test"; + +import { loadBrokerSession, sendBrokerShutdown, teardownBrokerSession } from "../plugins/codex/scripts/lib/broker-lifecycle.mjs"; +import { terminateProcessTree } from "../plugins/codex/scripts/lib/process.mjs"; + +const trackedTempDirs = []; export function makeTempDir(prefix = "codex-plugin-test-") { - return fs.mkdtempSync(path.join(os.tmpdir(), prefix)); + const dir = fs.mkdtempSync(path.join(os.tmpdir(), prefix)); + trackedTempDirs.push(dir); + return dir; +} + +function sleep(ms) { + return new Promise((resolve) => setTimeout(resolve, ms)); +} + +function pidAlive(pid) { + try { + process.kill(pid, 0); + return true; + } catch { + return false; + } } +function readBrokerPid(session) { + if (Number.isFinite(session.pid)) { + return session.pid; + } + try { + const parsed = Number.parseInt(fs.readFileSync(session.pidFile, "utf8").trim(), 10); + return Number.isFinite(parsed) ? parsed : null; + } catch { + return null; + } +} + +// Global teardown: any broker a test started (directly or lazily) and did not +// tear down is shut down here, so the suite never leaves broker/app-server +// processes behind — even when a test fails or returns early. +after(async () => { + const leakedPids = []; + const leakedSessions = []; + for (const dir of trackedTempDirs) { + const session = loadBrokerSession(dir); + if (!session) { + continue; + } + leakedSessions.push(session); + if (session.endpoint) { + await sendBrokerShutdown(session.endpoint); + } + const pid = readBrokerPid(session); + if (Number.isFinite(pid)) { + leakedPids.push(pid); + } + } + + const deadline = Date.now() + 3000; + for (const pid of leakedPids) { + while (pidAlive(pid) && Date.now() < deadline) { + await sleep(50); + } + if (pidAlive(pid)) { + terminateProcessTree(pid); + await sleep(200); + } + if (pidAlive(pid)) { + throw new Error(`Leaked broker process ${pid} survived suite teardown.`); + } + } + + for (const session of leakedSessions) { + teardownBrokerSession({ + endpoint: session.endpoint ?? null, + pidFile: session.pidFile ?? null, + logFile: session.logFile ?? null, + sessionDir: session.sessionDir ?? null + }); + } +}); + export function writeExecutable(filePath, source) { fs.writeFileSync(filePath, source, { encoding: "utf8", mode: 0o755 }); } diff --git a/tests/process.test.mjs b/tests/process.test.mjs index 80e0715b0..382afe133 100644 --- a/tests/process.test.mjs +++ b/tests/process.test.mjs @@ -1,7 +1,24 @@ +import process from "node:process"; import test from "node:test"; import assert from "node:assert/strict"; -import { terminateProcessTree } from "../plugins/codex/scripts/lib/process.mjs"; +import { runCommand, runCommandChecked, terminateProcessTree } from "../plugins/codex/scripts/lib/process.mjs"; + +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" }, () => { + const result = runCommand(process.execPath, ["-e", SELF_TERMINATING_SCRIPT]); + + assert.equal(result.signal, "SIGTERM"); + assert.notEqual(result.status, 0); +}); + +test("runCommandChecked throws when the process dies from a signal", { skip: process.platform === "win32" }, () => { + assert.throws( + () => runCommandChecked(process.execPath, ["-e", SELF_TERMINATING_SCRIPT]), + /signal=SIGTERM/ + ); +}); test("terminateProcessTree uses taskkill on Windows", () => { let captured = null; diff --git a/tests/runtime.test.mjs b/tests/runtime.test.mjs index 8f276835b..f78dc2cfb 100644 --- a/tests/runtime.test.mjs +++ b/tests/runtime.test.mjs @@ -2233,6 +2233,16 @@ test("status reports shared session runtime when a lazy broker is active", () => assert.equal(result.status, 0, result.stderr); assert.match(result.stdout, /Session runtime: shared session/); + + const cleanup = run("node", [SESSION_HOOK, "SessionEnd"], { + cwd: repo, + env: buildEnv(binDir), + input: JSON.stringify({ + hook_event_name: "SessionEnd", + cwd: repo + }) + }); + assert.equal(cleanup.status, 0, cleanup.stderr); }); test("setup and status honor --cwd when reading shared session runtime", () => { diff --git a/tests/state.test.mjs b/tests/state.test.mjs index 0f8f57cea..113df8153 100644 --- a/tests/state.test.mjs +++ b/tests/state.test.mjs @@ -1,11 +1,14 @@ import fs from "node:fs"; import os from "node:os"; 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, pathToFileURL } from "node:url"; import { makeTempDir } from "./helpers.mjs"; -import { resolveJobFile, resolveJobLogFile, resolveStateDir, resolveStateFile, saveState } from "../plugins/codex/scripts/lib/state.mjs"; +import { listJobs, resolveJobFile, resolveJobLogFile, resolveStateDir, resolveStateFile, saveState } from "../plugins/codex/scripts/lib/state.mjs"; test("resolveStateDir uses a temp-backed per-workspace directory", () => { const workspace = makeTempDir(); @@ -103,3 +106,47 @@ test("saveState prunes dropped job artifacts when indexed jobs exceed the cap", .sort() ); }); + +test("concurrent upsertJob calls from separate processes do not lose updates", async () => { + const workspace = makeTempDir(); + const stateModuleUrl = pathToFileURL( + path.join(path.dirname(fileURLToPath(import.meta.url)), "..", "plugins", "codex", "scripts", "lib", "state.mjs") + ).href; + const jobsPerWorker = 15; + + const spawnWorker = (prefix) => + new Promise((resolve, reject) => { + const child = spawn( + process.execPath, + [ + "--input-type=module", + "-e", + `import { upsertJob } from ${JSON.stringify(stateModuleUrl)}; + for (let index = 0; index < ${jobsPerWorker}; index++) { + upsertJob(${JSON.stringify(workspace)}, { id: ${JSON.stringify(prefix)} + "-" + index }); + }` + ], + { stdio: ["ignore", "ignore", "pipe"] } + ); + let stderr = ""; + child.stderr.on("data", (chunk) => { + stderr += chunk; + }); + child.on("error", reject); + child.on("exit", (code, signal) => { + if (code === 0) { + resolve(); + return; + } + reject(new Error(`worker ${prefix} failed (code=${code} signal=${signal}): ${stderr}`)); + }); + }); + + await Promise.all([spawnWorker("left"), spawnWorker("right")]); + + const jobIds = new Set(listJobs(workspace).map((job) => job.id)); + for (let index = 0; index < jobsPerWorker; index++) { + assert.equal(jobIds.has(`left-${index}`), true, `missing left-${index}`); + assert.equal(jobIds.has(`right-${index}`), true, `missing right-${index}`); + } +}); From 96875ff3cef10564b6a24d105c4547c88efa6981 Mon Sep 17 00:00:00 2001 From: Heringer Date: Thu, 23 Jul 2026 17:07:02 -0300 Subject: [PATCH 02/20] Fix broker ownership and lock reclamation --- plugins/codex/scripts/app-server-broker.mjs | 31 +- .../codex/scripts/lib/broker-lifecycle.mjs | 459 ++++++++++++++---- plugins/codex/scripts/lib/locking.mjs | 205 ++++++-- plugins/codex/scripts/lib/process.mjs | 199 +++++++- plugins/codex/scripts/lib/state.mjs | 35 +- plugins/codex/scripts/lib/tracked-jobs.mjs | 7 +- .../codex/scripts/session-lifecycle-hook.mjs | 53 +- tests/broker-lifecycle.test.mjs | 169 ++++++- tests/helpers.mjs | 76 +-- tests/locking.test.mjs | 105 ++++ tests/process.test.mjs | 21 +- tests/state.test.mjs | 29 +- 12 files changed, 1145 insertions(+), 244 deletions(-) create mode 100644 tests/locking.test.mjs diff --git a/plugins/codex/scripts/app-server-broker.mjs b/plugins/codex/scripts/app-server-broker.mjs index 1954274fe..09eb82aa1 100644 --- a/plugins/codex/scripts/app-server-broker.mjs +++ b/plugins/codex/scripts/app-server-broker.mjs @@ -41,28 +41,39 @@ function writePidFile(pidFile) { if (!pidFile) { return; } - fs.mkdirSync(path.dirname(pidFile), { recursive: true }); - fs.writeFileSync(pidFile, `${process.pid}\n`, "utf8"); + fs.mkdirSync(path.dirname(pidFile), { recursive: true, mode: 0o700 }); + fs.writeFileSync(pidFile, `${process.pid}\n`, { encoding: "utf8", mode: 0o600 }); + try { + fs.chmodSync(pidFile, 0o600); + } catch { + // Windows and restrictive filesystems may not implement POSIX modes. + } } async function main() { const [subcommand, ...argv] = process.argv.slice(2); if (subcommand !== "serve") { - throw new Error("Usage: node scripts/app-server-broker.mjs serve --endpoint [--cwd ] [--pid-file ]"); + throw new Error( + "Usage: node scripts/app-server-broker.mjs serve --endpoint --instance-token [--cwd ] [--pid-file ]" + ); } const { options } = parseArgs(argv, { - valueOptions: ["cwd", "pid-file", "endpoint"] + valueOptions: ["cwd", "pid-file", "endpoint", "instance-token"] }); if (!options.endpoint) { throw new Error("Missing required --endpoint."); } + if (!options["instance-token"]) { + throw new Error("Missing required --instance-token."); + } const cwd = options.cwd ? path.resolve(process.cwd(), options.cwd) : process.cwd(); const endpoint = String(options.endpoint); const listenTarget = parseBrokerEndpoint(endpoint); const pidFile = options["pid-file"] ? path.resolve(options["pid-file"]) : null; + const instanceToken = String(options["instance-token"]); writePidFile(pidFile); const appClient = await CodexAppServerClient.connect(cwd, { disableBroker: true }); @@ -158,7 +169,17 @@ async function main() { } if (message.id !== undefined && message.method === "broker/shutdown") { - send(socket, { id: message.id, result: {} }); + if (message.params?.instanceToken !== instanceToken) { + send(socket, { + id: message.id, + error: buildJsonRpcError(-32003, "Broker shutdown identity did not match this instance.") + }); + continue; + } + send(socket, { + id: message.id, + result: { pid: process.pid, instanceToken } + }); await shutdown(server); process.exit(0); } diff --git a/plugins/codex/scripts/lib/broker-lifecycle.mjs b/plugins/codex/scripts/lib/broker-lifecycle.mjs index c71559a14..928fb7b38 100644 --- a/plugins/codex/scripts/lib/broker-lifecycle.mjs +++ b/plugins/codex/scripts/lib/broker-lifecycle.mjs @@ -1,3 +1,4 @@ +import { randomUUID } from "node:crypto"; import fs from "node:fs"; import net from "node:net"; import os from "node:os"; @@ -5,16 +6,40 @@ import path from "node:path"; import process from "node:process"; import { spawn } from "node:child_process"; import { fileURLToPath } from "node:url"; + import { createBrokerEndpoint, parseBrokerEndpoint } from "./broker-endpoint.mjs"; import { withLock } from "./locking.mjs"; +import { + isProcessTreeRunning, + processHasLaunchToken, + terminateProcessTree, + waitForProcessExit +} from "./process.mjs"; import { resolveStateDir } from "./state.mjs"; export const PID_FILE_ENV = "CODEX_COMPANION_APP_SERVER_PID_FILE"; export const LOG_FILE_ENV = "CODEX_COMPANION_APP_SERVER_LOG_FILE"; const BROKER_STATE_FILE = "broker.json"; +const PRIVATE_DIR_MODE = 0o700; +const PRIVATE_FILE_MODE = 0o600; + +function setMode(filePath, mode) { + try { + fs.chmodSync(filePath, mode); + } catch { + // Windows and restrictive filesystems may not implement POSIX modes. + } +} + +function ensurePrivateDir(dir) { + fs.mkdirSync(dir, { recursive: true, mode: PRIVATE_DIR_MODE }); + setMode(dir, PRIVATE_DIR_MODE); +} export function createBrokerSessionDir(prefix = "cxc-") { - return fs.mkdtempSync(path.join(os.tmpdir(), prefix)); + const sessionDir = fs.mkdtempSync(path.join(os.tmpdir(), prefix)); + setMode(sessionDir, PRIVATE_DIR_MODE); + return sessionDir; } function connectToEndpoint(endpoint) { @@ -23,15 +48,22 @@ function connectToEndpoint(endpoint) { } export async function waitForBrokerEndpoint(endpoint, timeoutMs = 2000) { - const start = Date.now(); - while (Date.now() - start < timeoutMs) { + const deadline = Date.now() + timeoutMs; + while (Date.now() < deadline) { const ready = await new Promise((resolve) => { const socket = connectToEndpoint(endpoint); - socket.on("connect", () => { - socket.end(); - resolve(true); - }); - socket.on("error", () => resolve(false)); + let settled = false; + const finish = (value) => { + if (settled) { + return; + } + settled = true; + socket.destroy(); + resolve(value); + }; + socket.setTimeout(Math.max(1, Math.min(150, deadline - Date.now())), () => finish(false)); + socket.on("connect", () => finish(true)); + socket.on("error", () => finish(false)); }); if (ready) { return true; @@ -41,33 +73,90 @@ export async function waitForBrokerEndpoint(endpoint, timeoutMs = 2000) { return false; } -export async function sendBrokerShutdown(endpoint) { - await new Promise((resolve) => { +export async function sendBrokerShutdown(endpoint, options = {}) { + return await new Promise((resolve) => { const socket = connectToEndpoint(endpoint); + let settled = false; + let buffer = ""; + const finish = (result = null) => { + if (settled) { + return; + } + settled = true; + socket.destroy(); + resolve(result); + }; socket.setEncoding("utf8"); + socket.setTimeout(options.timeoutMs ?? 2000, () => finish(null)); socket.on("connect", () => { - socket.write(`${JSON.stringify({ id: 1, method: "broker/shutdown", params: {} })}\n`); + socket.write( + `${JSON.stringify({ + id: 1, + method: "broker/shutdown", + params: { instanceToken: options.instanceToken ?? null } + })}\n` + ); }); - socket.on("data", () => { - socket.end(); - resolve(); + socket.on("data", (chunk) => { + buffer += chunk; + const newlineIndex = buffer.indexOf("\n"); + if (newlineIndex === -1) { + return; + } + try { + const response = JSON.parse(buffer.slice(0, newlineIndex)); + finish({ result: response?.result ?? null, error: response?.error ?? null }); + } catch { + finish(null); + } }); - socket.on("error", resolve); - socket.on("close", resolve); + socket.on("error", () => finish(null)); + socket.on("close", () => finish(null)); }); } -export function spawnBrokerProcess({ scriptPath, cwd, endpoint, pidFile, logFile, env = process.env }) { - const logFd = fs.openSync(logFile, "a"); - const child = spawn(process.execPath, [scriptPath, "serve", "--endpoint", endpoint, "--cwd", cwd, "--pid-file", pidFile], { - cwd, - env, - detached: true, - stdio: ["ignore", logFd, logFd] - }); - child.unref(); - fs.closeSync(logFd); - return child; +function isValidPid(pid) { + return Number.isSafeInteger(pid) && pid > 0; +} + +export function spawnBrokerProcess({ + scriptPath, + cwd, + endpoint, + pidFile, + logFile, + instanceToken, + env = process.env +}) { + const logFd = fs.openSync(logFile, "a", PRIVATE_FILE_MODE); + try { + setMode(logFile, PRIVATE_FILE_MODE); + const child = spawn( + process.execPath, + [ + scriptPath, + "serve", + "--endpoint", + endpoint, + "--cwd", + cwd, + "--pid-file", + pidFile, + "--instance-token", + instanceToken + ], + { + cwd, + env, + detached: true, + stdio: ["ignore", logFd, logFd] + } + ); + child.unref(); + return child; + } finally { + fs.closeSync(logFd); + } } function resolveBrokerStateFile(cwd) { @@ -89,17 +178,205 @@ export function loadBrokerSession(cwd) { export function saveBrokerSession(cwd, session) { const stateDir = resolveStateDir(cwd); - fs.mkdirSync(stateDir, { recursive: true }); - fs.writeFileSync(resolveBrokerStateFile(cwd), `${JSON.stringify(session, null, 2)}\n`, "utf8"); + ensurePrivateDir(stateDir); + const stateFile = resolveBrokerStateFile(cwd); + const tmpFile = `${stateFile}.${process.pid}.${randomUUID()}.tmp`; + try { + const fd = fs.openSync(tmpFile, "wx", PRIVATE_FILE_MODE); + try { + setMode(tmpFile, PRIVATE_FILE_MODE); + fs.writeFileSync(fd, `${JSON.stringify(session, null, 2)}\n`, "utf8"); + fs.fsyncSync(fd); + } finally { + fs.closeSync(fd); + } + fs.renameSync(tmpFile, stateFile); + setMode(stateFile, PRIVATE_FILE_MODE); + } catch (error) { + try { + fs.unlinkSync(tmpFile); + } catch { + // Temp file may not have been created or may already be gone. + } + throw error; + } } export function clearBrokerSession(cwd) { const stateFile = resolveBrokerStateFile(cwd); - if (fs.existsSync(stateFile)) { + try { fs.unlinkSync(stateFile); + } catch (error) { + if (error?.code !== "ENOENT") { + throw error; + } + } +} + +function resolveBrokerPid(session) { + const statePid = isValidPid(session.pid) ? session.pid : null; + let filePid = null; + if (session.pidFile && fs.existsSync(session.pidFile)) { + const rawPid = fs.readFileSync(session.pidFile, "utf8").trim(); + if (/^\d+$/.test(rawPid)) { + const parsedPid = Number(rawPid); + filePid = isValidPid(parsedPid) ? parsedPid : null; + } + } + if (statePid && filePid && statePid !== filePid) { + throw new Error(`Codex app-server broker PID mismatch (${statePid} != ${filePid}).`); + } + return statePid ?? filePid; +} + +function endpointArtifactExists(endpoint) { + if (!endpoint) { + return false; + } + try { + const target = parseBrokerEndpoint(endpoint); + // Named pipes have no filesystem artifact for teardown to unlink. + return target.kind === "unix" && fs.existsSync(target.path); + } catch { + return true; } } +function canDiscardUnownedSession(session, pid) { + const processExited = !isValidPid(pid) || !isProcessTreeRunning(pid); + return processExited && !endpointArtifactExists(session.endpoint); +} + +export async function shutdownBrokerSession(cwd, options = {}) { + const session = options.session ?? loadBrokerSession(cwd); + if (!session) { + return { found: false, exited: true, forced: false }; + } + + const pid = resolveBrokerPid(session); + if (session.endpoint && !session.instanceToken) { + if (canDiscardUnownedSession(session, pid)) { + teardownBrokerSession({ + endpoint: null, + pidFile: session.pidFile ?? null, + logFile: session.logFile ?? null, + sessionDir: session.sessionDir ?? null + }); + clearBrokerSession(cwd); + return { found: true, exited: true, forced: false }; + } + throw new Error("Codex app-server broker ownership could not be verified; persisted state was preserved."); + } + + let shutdownResponse = null; + if (session.endpoint) { + shutdownResponse = await sendBrokerShutdown(session.endpoint, { + timeoutMs: options.timeoutMs, + instanceToken: session.instanceToken + }); + } + if (shutdownResponse?.error) { + throw new Error( + `Codex app-server broker rejected shutdown identity; persisted state was preserved: ${ + shutdownResponse.error.message ?? "unknown error" + }` + ); + } + + const shutdownAck = shutdownResponse?.result ?? null; + const acknowledgedPid = isValidPid(shutdownAck?.pid) ? shutdownAck.pid : null; + const ownershipVerified = + Boolean(session.instanceToken) && + shutdownAck?.instanceToken === session.instanceToken && + acknowledgedPid !== null && + (pid === null || pid === acknowledgedPid); + if (shutdownAck && !ownershipVerified) { + throw new Error("Codex app-server broker shutdown identity did not match persisted state."); + } + + let verifiedPid = ownershipVerified ? acknowledgedPid : null; + let exited = isValidPid(pid) + ? await waitForProcessExit(pid, { + timeoutMs: 0, + intervalMs: options.intervalMs, + killImpl: options.killImpl, + platform: options.platform + }) + : false; + + if (!shutdownAck && isValidPid(pid) && !exited) { + const ownsPersistedProcess = options.verifyProcess + ? options.verifyProcess(pid, session.instanceToken) + : processHasLaunchToken(pid, session.instanceToken, { + marker: "--instance-token", + platform: options.platform, + timeoutMs: options.timeoutMs, + runCommandImpl: options.runCommandImpl + }); + if (!ownsPersistedProcess) { + if (isProcessTreeRunning(pid, options)) { + throw new Error("Codex app-server broker ownership could not be verified; persisted state was preserved."); + } + exited = true; + } else { + verifiedPid = pid; + } + } + + if (!shutdownAck && session.instanceToken && !isValidPid(pid)) { + throw new Error("Codex app-server broker PID is unavailable; persisted ownership state was preserved."); + } + + if (!exited && isValidPid(verifiedPid)) { + exited = await waitForProcessExit(verifiedPid, { + timeoutMs: options.timeoutMs, + intervalMs: options.intervalMs, + killImpl: options.killImpl, + platform: options.platform + }); + } + + let forced = false; + if (!exited && isValidPid(verifiedPid) && options.killProcess) { + const stillOwnsProcess = options.verifyProcess + ? options.verifyProcess(verifiedPid, session.instanceToken) + : processHasLaunchToken(verifiedPid, session.instanceToken, { + marker: "--instance-token", + platform: options.platform, + timeoutMs: options.timeoutMs, + runCommandImpl: options.runCommandImpl + }); + if (!stillOwnsProcess) { + throw new Error("Codex app-server broker process ownership changed before forced shutdown."); + } + options.killProcess(verifiedPid); + forced = true; + exited = await waitForProcessExit(verifiedPid, { + timeoutMs: options.timeoutMs, + intervalMs: options.intervalMs, + killImpl: options.killImpl, + platform: options.platform + }); + } + + if (!exited) { + throw new Error(`Codex app-server broker ${verifiedPid ?? session.endpoint ?? "unknown"} did not exit.`); + } + if (!ownershipVerified && endpointArtifactExists(session.endpoint)) { + throw new Error("Codex app-server broker endpoint ownership could not be verified; persisted state was preserved."); + } + + teardownBrokerSession({ + endpoint: ownershipVerified ? session.endpoint ?? null : null, + pidFile: session.pidFile ?? null, + logFile: session.logFile ?? null, + sessionDir: session.sessionDir ?? null, + ownershipVerified + }); + clearBrokerSession(cwd); + return { found: true, exited: true, forced }; +} + async function isBrokerEndpointReady(endpoint) { if (!endpoint) { return false; @@ -113,12 +390,12 @@ async function isBrokerEndpointReady(endpoint) { export async function ensureBrokerSession(cwd, options = {}) { const stateDir = resolveStateDir(cwd); - fs.mkdirSync(stateDir, { recursive: true }); - // Serialize the check-then-create sequence per workspace so two concurrent - // callers cannot both miss the existing broker and spawn a duplicate. - return withLock(path.join(stateDir, ".broker.lock"), () => ensureBrokerSessionLocked(cwd, options), { - timeoutMs: options.lockTimeoutMs ?? 10000 - }); + ensurePrivateDir(stateDir); + return withLock( + path.join(stateDir, ".broker.lock"), + () => ensureBrokerSessionLocked(cwd, options), + { timeoutMs: options.lockTimeoutMs ?? 10000 } + ); } async function ensureBrokerSessionLocked(cwd, options = {}) { @@ -128,15 +405,16 @@ async function ensureBrokerSessionLocked(cwd, options = {}) { } if (existing) { - teardownBrokerSession({ - endpoint: existing.endpoint ?? null, - pidFile: existing.pidFile ?? null, - logFile: existing.logFile ?? null, - sessionDir: existing.sessionDir ?? null, - pid: existing.pid ?? null, - killProcess: options.killProcess ?? null + await shutdownBrokerSession(cwd, { + session: existing, + killProcess: options.killProcess ?? terminateProcessTree, + verifyProcess: options.verifyProcess, + runCommandImpl: options.runCommandImpl, + killImpl: options.killImpl, + platform: options.platform, + timeoutMs: options.timeoutMs, + intervalMs: options.intervalMs }); - clearBrokerSession(cwd); } const sessionDir = createBrokerSessionDir(); @@ -145,8 +423,8 @@ async function ensureBrokerSessionLocked(cwd, options = {}) { const pidFile = path.join(sessionDir, "broker.pid"); const logFile = path.join(sessionDir, "broker.log"); const scriptPath = - options.scriptPath ?? - fileURLToPath(new URL("../app-server-broker.mjs", import.meta.url)); + options.scriptPath ?? fileURLToPath(new URL("../app-server-broker.mjs", import.meta.url)); + const instanceToken = options.instanceToken ?? randomUUID(); const child = spawnBrokerProcess({ scriptPath, @@ -154,67 +432,84 @@ async function ensureBrokerSessionLocked(cwd, options = {}) { endpoint, pidFile, logFile, + instanceToken, env: options.env ?? process.env }); - const ready = await waitForBrokerEndpoint(endpoint, options.timeoutMs ?? 2000); - if (!ready) { - teardownBrokerSession({ - endpoint, - pidFile, - logFile, - sessionDir, - pid: child.pid ?? null, - killProcess: options.killProcess ?? null - }); - return null; - } - const session = { endpoint, pidFile, logFile, sessionDir, - pid: child.pid ?? null + pid: child.pid ?? null, + instanceToken }; saveBrokerSession(cwd, session); - return session; -} -export function teardownBrokerSession({ endpoint = null, pidFile, logFile, sessionDir = null, pid = null, killProcess = null }) { - if (Number.isFinite(pid) && killProcess) { - try { - killProcess(pid); - } catch { - // Ignore missing or already-exited broker processes. - } + const ready = await waitForBrokerEndpoint(endpoint, options.timeoutMs ?? 2000); + if (!ready) { + await shutdownBrokerSession(cwd, { + session, + killProcess: options.killProcess ?? terminateProcessTree, + verifyProcess: options.verifyProcess, + runCommandImpl: options.runCommandImpl, + killImpl: options.killImpl, + platform: options.platform, + timeoutMs: options.timeoutMs, + intervalMs: options.intervalMs + }); + return null; } - if (pidFile && fs.existsSync(pidFile)) { - fs.unlinkSync(pidFile); + return session; +} + +export function teardownBrokerSession({ + endpoint = null, + pidFile, + logFile, + sessionDir = null, + ownershipVerified = false +}) { + if (endpoint && !ownershipVerified) { + throw new Error("Refusing to remove an unverified broker endpoint."); } - if (logFile && fs.existsSync(logFile)) { - fs.unlinkSync(logFile); + for (const filePath of [pidFile, logFile]) { + if (!filePath) { + continue; + } + try { + fs.unlinkSync(filePath); + } catch (error) { + if (error?.code !== "ENOENT") { + throw error; + } + } } if (endpoint) { - try { - const target = parseBrokerEndpoint(endpoint); - if (target.kind === "unix" && fs.existsSync(target.path)) { + const target = parseBrokerEndpoint(endpoint); + if (target.kind === "unix") { + try { fs.unlinkSync(target.path); + } catch (error) { + if (error?.code !== "ENOENT") { + throw error; + } } - } catch { - // Ignore malformed or already-removed broker endpoints during teardown. } } - const resolvedSessionDir = sessionDir ?? (pidFile ? path.dirname(pidFile) : logFile ? path.dirname(logFile) : null); - if (resolvedSessionDir && fs.existsSync(resolvedSessionDir)) { + const resolvedSessionDir = + sessionDir ?? (pidFile ? path.dirname(pidFile) : logFile ? path.dirname(logFile) : null); + if (resolvedSessionDir) { try { fs.rmdirSync(resolvedSessionDir); - } catch { - // Ignore non-empty or missing directories. + } catch (error) { + if (error?.code !== "ENOENT" && error?.code !== "ENOTEMPTY") { + throw error; + } } } } diff --git a/plugins/codex/scripts/lib/locking.mjs b/plugins/codex/scripts/lib/locking.mjs index 395b41ac0..02f9ca77f 100644 --- a/plugins/codex/scripts/lib/locking.mjs +++ b/plugins/codex/scripts/lib/locking.mjs @@ -1,8 +1,15 @@ +import { randomUUID } from "node:crypto"; import fs from "node:fs"; +import path from "node:path"; +import process from "node:process"; + +import { getProcessIdentity, isProcessRunning } from "./process.mjs"; const DEFAULT_TIMEOUT_MS = 5000; const DEFAULT_STALE_MS = 30000; const RETRY_DELAY_MS = 25; +const OWNER_FILE_PREFIX = "owner-"; +const OWNER_FILE_SUFFIX = ".json"; function sleepSync(ms) { Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, ms); @@ -12,77 +19,215 @@ function sleep(ms) { return new Promise((resolve) => setTimeout(resolve, ms)); } +function ownerFileName(token) { + return `${OWNER_FILE_PREFIX}${token}${OWNER_FILE_SUFFIX}`; +} + +function writePrivateJson(filePath, value) { + const fd = fs.openSync(filePath, "wx", 0o600); + try { + fs.writeFileSync(fd, `${JSON.stringify(value)}\n`, "utf8"); + fs.fsyncSync(fd); + } finally { + fs.closeSync(fd); + } +} + +function removeCandidate(handle) { + try { + fs.unlinkSync(handle.candidateOwnerFile); + } catch { + // Candidate may already have been published or cleaned up. + } + try { + fs.rmdirSync(handle.candidateDir); + } catch { + // Candidate may already have been published or cleaned up. + } +} + function tryAcquire(lockDir) { + const token = randomUUID(); + const candidateDir = `${lockDir}.candidate-${token}`; + const candidateOwnerFile = path.join(candidateDir, ownerFileName(token)); + const handle = { + lockDir, + token, + ownerFile: path.join(lockDir, ownerFileName(token)), + candidateDir, + candidateOwnerFile + }; + + fs.mkdirSync(candidateDir, { mode: 0o700 }); try { - fs.mkdirSync(lockDir); - return true; + writePrivateJson(candidateOwnerFile, { + pid: process.pid, + token, + createdAt: Date.now(), + processIdentity: getProcessIdentity(process.pid) + }); + fs.renameSync(candidateDir, lockDir); + return handle; } catch (error) { - if (error?.code === "EEXIST") { - return false; + removeCandidate(handle); + const targetExists = fs.existsSync(lockDir); + if ( + error?.code === "EEXIST" || + error?.code === "ENOTEMPTY" || + (error?.code === "EPERM" && targetExists) + ) { + return null; } throw error; } } -function reclaimIfStale(lockDir, staleMs) { +function readLockState(lockDir) { + let entries; + let ageMs; try { - const age = Date.now() - fs.statSync(lockDir).mtimeMs; - if (age > staleMs) { - fs.rmdirSync(lockDir); + entries = fs.readdirSync(lockDir); + ageMs = Date.now() - fs.statSync(lockDir).mtimeMs; + } catch { + return { kind: "missing" }; + } + + const ownerFiles = entries.filter( + (entry) => entry.startsWith(OWNER_FILE_PREFIX) && entry.endsWith(OWNER_FILE_SUFFIX) + ); + if (ownerFiles.length === 0 && entries.length === 0) { + return { kind: "empty", ageMs }; + } + if (ownerFiles.length !== 1 || entries.length !== 1) { + return { kind: "corrupt", ageMs }; + } + + const fileName = ownerFiles[0]; + const ownerFile = path.join(lockDir, fileName); + try { + const owner = JSON.parse(fs.readFileSync(ownerFile, "utf8")); + const expectedFileName = ownerFileName(owner.token); + if ( + !Number.isFinite(owner.pid) || + owner.pid <= 0 || + typeof owner.token !== "string" || + fileName !== expectedFileName + ) { + return { kind: "corrupt", ageMs }; } + return { kind: "owned", owner, ownerFile, ageMs }; } catch { - // Lock released (or reclaimed by someone else) between stat and rmdir. + return { kind: "corrupt", ageMs }; } } +function removeOwnedLock(ownerFile, lockDir) { + try { + // The token is part of the filename, so an old owner/reclaimer cannot + // unlink a successor's ownership record. + fs.unlinkSync(ownerFile); + } catch { + return false; + } + try { + fs.rmdirSync(lockDir); + } catch { + // A successor may already have atomically replaced the now-empty dir. + } + return true; +} + +function reclaimAbandonedLock(lockDir, options) { + const state = readLockState(lockDir); + if (state.kind === "missing") { + return true; + } + if (state.kind === "owned") { + const processRunning = options.isProcessRunning ?? isProcessRunning; + if ( + processRunning(state.owner.pid, { + identity: state.owner.processIdentity ?? undefined + }) + ) { + return false; + } + return removeOwnedLock(state.ownerFile, lockDir); + } + if (state.kind === "empty" && state.ageMs > options.staleMs) { + try { + fs.rmdirSync(lockDir); + return true; + } catch { + return false; + } + } + // Unknown non-empty contents fail closed. Published locks always contain + // one complete owner file, so silently deleting anything else is unsafe. + return false; +} + +function normalizeOptions(options) { + return { + timeoutMs: options.timeoutMs ?? DEFAULT_TIMEOUT_MS, + staleMs: options.staleMs ?? DEFAULT_STALE_MS, + retryDelayMs: options.retryDelayMs ?? RETRY_DELAY_MS, + isProcessRunning: options.isProcessRunning + }; +} + export function acquireLockSync(lockDir, options = {}) { - const timeoutMs = options.timeoutMs ?? DEFAULT_TIMEOUT_MS; - const staleMs = options.staleMs ?? DEFAULT_STALE_MS; - const deadline = Date.now() + timeoutMs; - while (!tryAcquire(lockDir)) { - reclaimIfStale(lockDir, staleMs); + const normalized = normalizeOptions(options); + const deadline = Date.now() + normalized.timeoutMs; + while (true) { + const handle = tryAcquire(lockDir); + if (handle) { + return handle; + } + reclaimAbandonedLock(lockDir, normalized); if (Date.now() >= deadline) { throw new Error(`Timed out waiting for lock: ${lockDir}`); } - sleepSync(RETRY_DELAY_MS); + sleepSync(normalized.retryDelayMs); } } export async function acquireLock(lockDir, options = {}) { - const timeoutMs = options.timeoutMs ?? DEFAULT_TIMEOUT_MS; - const staleMs = options.staleMs ?? DEFAULT_STALE_MS; - const deadline = Date.now() + timeoutMs; - while (!tryAcquire(lockDir)) { - reclaimIfStale(lockDir, staleMs); + const normalized = normalizeOptions(options); + const deadline = Date.now() + normalized.timeoutMs; + while (true) { + const handle = tryAcquire(lockDir); + if (handle) { + return handle; + } + reclaimAbandonedLock(lockDir, normalized); if (Date.now() >= deadline) { throw new Error(`Timed out waiting for lock: ${lockDir}`); } - await sleep(RETRY_DELAY_MS); + await sleep(normalized.retryDelayMs); } } -export function releaseLock(lockDir) { - try { - fs.rmdirSync(lockDir); - } catch { - // Already released or reclaimed as stale; nothing left to do. +export function releaseLock(handle) { + if (!handle?.lockDir || !handle?.ownerFile) { + return false; } + return removeOwnedLock(handle.ownerFile, handle.lockDir); } export function withLockSync(lockDir, fn, options = {}) { - acquireLockSync(lockDir, options); + const handle = acquireLockSync(lockDir, options); try { return fn(); } finally { - releaseLock(lockDir); + releaseLock(handle); } } export async function withLock(lockDir, fn, options = {}) { - await acquireLock(lockDir, options); + const handle = await acquireLock(lockDir, options); try { return await fn(); } finally { - releaseLock(lockDir); + releaseLock(handle); } } diff --git a/plugins/codex/scripts/lib/process.mjs b/plugins/codex/scripts/lib/process.mjs index 1fa9a8d7f..40c2e6e50 100644 --- a/plugins/codex/scripts/lib/process.mjs +++ b/plugins/codex/scripts/lib/process.mjs @@ -1,4 +1,5 @@ import { spawnSync } from "node:child_process"; +import fs from "node:fs"; import process from "node:process"; export function runCommand(command, args = [], options = {}) { @@ -8,6 +9,8 @@ export function runCommand(command, args = [], options = {}) { encoding: "utf8", input: options.input, maxBuffer: options.maxBuffer, + timeout: options.timeout, + killSignal: options.killSignal, stdio: options.stdio ?? "pipe", shell: options.shell ?? (process.platform === "win32" ? (process.env.SHELL || true) : false), windowsHide: true @@ -16,9 +19,9 @@ export function runCommand(command, args = [], options = {}) { return { command, args, - // A null status with a signal means the process was killed; never report - // signal-terminated commands as exit 0. - status: result.status ?? (result.signal != null ? 1 : 0), + // Preserve Node's spawnSync contract: signal-terminated commands have a + // null status and a non-null signal. + status: result.status, signal: result.signal ?? null, stdout: result.stdout ?? "", stderr: result.stderr ?? "", @@ -31,7 +34,7 @@ export function runCommandChecked(command, args = [], options = {}) { if (result.error) { throw result.error; } - if (result.status !== 0) { + if (result.signal != null || result.status !== 0) { throw new Error(formatCommandFailure(result)); } return result; @@ -45,8 +48,11 @@ export function binaryAvailable(command, versionArgs = ["--version"], options = if (result.error) { return { available: false, detail: result.error.message }; } - if (result.status !== 0) { - const detail = result.stderr.trim() || result.stdout.trim() || `exit ${result.status}`; + if (result.signal != null || result.status !== 0) { + const detail = + result.stderr.trim() || + result.stdout.trim() || + (result.signal != null ? `signal ${result.signal}` : `exit ${result.status}`); return { available: false, detail }; } return { available: true, detail: result.stdout.trim() || result.stderr.trim() || "ok" }; @@ -56,8 +62,187 @@ function looksLikeMissingProcessMessage(text) { return /not found|no running instance|cannot find|does not exist|no such process/i.test(text); } +function isValidPid(pid) { + return Number.isSafeInteger(pid) && pid > 0; +} + +function readLinuxProcessStat(pid) { + try { + const stat = fs.readFileSync(`/proc/${pid}/stat`, "utf8"); + const commandEnd = stat.lastIndexOf(")"); + if (commandEnd === -1) { + return null; + } + const fields = stat.slice(commandEnd + 1).trim().split(/\s+/); + return { + state: fields[0] ?? null, + processGroup: fields[2] ?? null, + // /proc//stat field 22; fields starts at field 3. + startTime: fields[19] ?? null + }; + } catch { + return null; + } +} + +export function getProcessIdentity(pid, options = {}) { + if (!isValidPid(pid)) { + return null; + } + const platform = options.platform ?? process.platform; + if (platform !== "linux") { + return null; + } + return readLinuxProcessStat(pid)?.startTime ?? null; +} + +export function isProcessRunning(pid, options = {}) { + if (!isValidPid(pid)) { + return false; + } + + const platform = options.platform ?? process.platform; + const killImpl = options.killImpl ?? process.kill.bind(process); + try { + killImpl(pid, 0); + } catch (error) { + if (error?.code === "EPERM") { + return true; + } + if (error?.code === "ESRCH") { + return false; + } + throw error; + } + + if (platform === "linux") { + const readProcessStat = options.readProcessStat ?? readLinuxProcessStat; + const stat = readProcessStat(pid); + if (!stat) { + return false; + } + // Zombies still answer kill(pid, 0), but no longer own a live resource. + if (stat.state === "Z" || stat.state === "X") { + return false; + } + if (options.identity != null && stat.startTime !== String(options.identity)) { + return false; + } + } + + return true; +} + +function isLinuxProcessGroupRunning(pid) { + let entries; + try { + entries = fs.readdirSync("/proc", { withFileTypes: true }); + } catch { + return isProcessRunning(pid); + } + for (const entry of entries) { + if (!entry.isDirectory() || !/^\d+$/.test(entry.name)) { + continue; + } + const stat = readLinuxProcessStat(Number(entry.name)); + if ( + stat?.processGroup === String(pid) && + stat.state !== "Z" && + stat.state !== "X" + ) { + return true; + } + } + return false; +} + +export function isProcessTreeRunning(pid, options = {}) { + if (!isValidPid(pid)) { + return false; + } + const platform = options.platform ?? process.platform; + if (platform === "win32") { + return isProcessRunning(pid, options); + } + if (platform === "linux" && !options.killImpl) { + return isLinuxProcessGroupRunning(pid); + } + + const killImpl = options.killImpl ?? process.kill.bind(process); + try { + killImpl(-pid, 0); + return true; + } catch (error) { + if (error?.code === "ESRCH") { + return false; + } + if (error?.code === "EPERM") { + return true; + } + throw error; + } +} + +export async function waitForProcessExit(pid, options = {}) { + if (!isValidPid(pid)) { + return false; + } + const timeoutMs = options.timeoutMs ?? 2000; + const intervalMs = options.intervalMs ?? 25; + const isRunning = options.tree === false ? isProcessRunning : isProcessTreeRunning; + const deadline = Date.now() + timeoutMs; + while (Date.now() < deadline) { + if (!isRunning(pid, options)) { + return true; + } + await new Promise((resolve) => setTimeout(resolve, intervalMs)); + } + return !isRunning(pid, options); +} + +export function processHasLaunchToken(pid, token, options = {}) { + if (!isValidPid(pid) || typeof token !== "string" || token.length < 16) { + return false; + } + + const marker = options.marker ?? "--worker-token"; + const platform = options.platform ?? process.platform; + if (platform === "linux") { + try { + const argv = fs.readFileSync(`/proc/${pid}/cmdline`, "utf8").split("\0"); + return argv.includes(marker) && argv.includes(token); + } catch { + return false; + } + } + + const runCommandImpl = options.runCommandImpl ?? runCommand; + const result = + platform === "win32" + ? runCommandImpl( + "powershell.exe", + [ + "-NoProfile", + "-NonInteractive", + "-Command", + `$p = Get-CimInstance Win32_Process -Filter 'ProcessId = ${pid}'; if ($null -ne $p) { [Console]::Out.Write($p.CommandLine) }` + ], + { timeout: options.timeoutMs ?? 2000, killSignal: "SIGTERM" } + ) + : runCommandImpl("ps", ["-ww", "-p", String(pid), "-o", "command="], { + timeout: options.timeoutMs ?? 2000, + killSignal: "SIGTERM" + }); + + if (result.error || result.signal != null || result.status !== 0) { + return false; + } + const commandLine = String(result.stdout ?? ""); + return commandLine.includes(marker) && commandLine.includes(token); +} + export function terminateProcessTree(pid, options = {}) { - if (!Number.isFinite(pid)) { + if (!isValidPid(pid)) { return { attempted: false, delivered: false, method: null }; } diff --git a/plugins/codex/scripts/lib/state.mjs b/plugins/codex/scripts/lib/state.mjs index ac24ca878..da2bb5e4c 100644 --- a/plugins/codex/scripts/lib/state.mjs +++ b/plugins/codex/scripts/lib/state.mjs @@ -12,6 +12,8 @@ const FALLBACK_STATE_ROOT_DIR = path.join(os.tmpdir(), "codex-companion"); const STATE_FILE_NAME = "state.json"; const JOBS_DIR_NAME = "jobs"; const MAX_JOBS = 50; +const PRIVATE_DIR_MODE = 0o700; +const PRIVATE_FILE_MODE = 0o600; function nowIso() { return new Date().toISOString(); @@ -53,7 +55,16 @@ export function resolveJobsDir(cwd) { } export function ensureStateDir(cwd) { - fs.mkdirSync(resolveJobsDir(cwd), { recursive: true }); + const stateDir = resolveStateDir(cwd); + const jobsDir = resolveJobsDir(cwd); + fs.mkdirSync(jobsDir, { recursive: true, mode: PRIVATE_DIR_MODE }); + for (const dir of [stateDir, jobsDir]) { + try { + fs.chmodSync(dir, PRIVATE_DIR_MODE); + } catch { + // Windows and restrictive filesystems may not implement POSIX modes. + } + } } export function loadState(cwd) { @@ -97,14 +108,24 @@ function resolveStateLockDir(cwd) { function writeStateFileAtomic(stateFile, nextState) { const tmpFile = `${stateFile}.${process.pid}.${Math.random().toString(36).slice(2)}.tmp`; try { - const fd = fs.openSync(tmpFile, "w"); + const fd = fs.openSync(tmpFile, "wx", PRIVATE_FILE_MODE); try { + try { + fs.fchmodSync(fd, PRIVATE_FILE_MODE); + } catch { + // Windows and restrictive filesystems may not implement POSIX modes. + } fs.writeFileSync(fd, `${JSON.stringify(nextState, null, 2)}\n`, "utf8"); fs.fsyncSync(fd); } finally { fs.closeSync(fd); } fs.renameSync(tmpFile, stateFile); + try { + fs.chmodSync(stateFile, PRIVATE_FILE_MODE); + } catch { + // Windows and restrictive filesystems may not implement POSIX modes. + } } catch (error) { removeFileIfExists(tmpFile); throw error; @@ -195,7 +216,15 @@ export function getConfig(cwd) { export function writeJobFile(cwd, jobId, payload) { ensureStateDir(cwd); const jobFile = resolveJobFile(cwd, jobId); - fs.writeFileSync(jobFile, `${JSON.stringify(payload, null, 2)}\n`, "utf8"); + fs.writeFileSync(jobFile, `${JSON.stringify(payload, null, 2)}\n`, { + encoding: "utf8", + mode: PRIVATE_FILE_MODE + }); + try { + fs.chmodSync(jobFile, PRIVATE_FILE_MODE); + } catch { + // Windows and restrictive filesystems may not implement POSIX modes. + } return jobFile; } diff --git a/plugins/codex/scripts/lib/tracked-jobs.mjs b/plugins/codex/scripts/lib/tracked-jobs.mjs index 902869012..73e51c19f 100644 --- a/plugins/codex/scripts/lib/tracked-jobs.mjs +++ b/plugins/codex/scripts/lib/tracked-jobs.mjs @@ -50,7 +50,12 @@ export function appendLogBlock(logFile, title, body) { export function createJobLogFile(workspaceRoot, jobId, title) { const logFile = resolveJobLogFile(workspaceRoot, jobId); - fs.writeFileSync(logFile, "", "utf8"); + fs.writeFileSync(logFile, "", { encoding: "utf8", mode: 0o600 }); + try { + fs.chmodSync(logFile, 0o600); + } catch { + // Windows and restrictive filesystems may not implement POSIX modes. + } if (title) { appendLogLine(logFile, `Starting ${title}.`); } diff --git a/plugins/codex/scripts/session-lifecycle-hook.mjs b/plugins/codex/scripts/session-lifecycle-hook.mjs index 778571e6c..91e11b26b 100644 --- a/plugins/codex/scripts/session-lifecycle-hook.mjs +++ b/plugins/codex/scripts/session-lifecycle-hook.mjs @@ -4,14 +4,9 @@ import fs from "node:fs"; import process from "node:process"; import { terminateProcessTree } from "./lib/process.mjs"; -import { BROKER_ENDPOINT_ENV } from "./lib/app-server.mjs"; import { - clearBrokerSession, - LOG_FILE_ENV, loadBrokerSession, - PID_FILE_ENV, - sendBrokerShutdown, - teardownBrokerSession + shutdownBrokerSession } from "./lib/broker-lifecycle.mjs"; import { loadState, resolveStateFile, saveState } from "./lib/state.mjs"; import { TRANSCRIPT_PATH_ENV } from "./lib/claude-session-transfer.mjs"; @@ -82,35 +77,27 @@ function handleSessionStart(input) { async function handleSessionEnd(input) { const cwd = input.cwd || process.cwd(); - const brokerSession = - loadBrokerSession(cwd) ?? - (process.env[BROKER_ENDPOINT_ENV] - ? { - endpoint: process.env[BROKER_ENDPOINT_ENV], - pidFile: process.env[PID_FILE_ENV] ?? null, - logFile: process.env[LOG_FILE_ENV] ?? null - } - : null); - const brokerEndpoint = brokerSession?.endpoint ?? null; - const pidFile = brokerSession?.pidFile ?? null; - const logFile = brokerSession?.logFile ?? null; - const sessionDir = brokerSession?.sessionDir ?? null; - const pid = brokerSession?.pid ?? null; - - if (brokerEndpoint) { - await sendBrokerShutdown(brokerEndpoint); + const brokerSession = loadBrokerSession(cwd); + const failures = []; + + try { + cleanupSessionJobs(cwd, input.session_id || process.env[SESSION_ID_ENV]); + } catch (error) { + failures.push(error); } - cleanupSessionJobs(cwd, input.session_id || process.env[SESSION_ID_ENV]); - teardownBrokerSession({ - endpoint: brokerEndpoint, - pidFile, - logFile, - sessionDir, - pid, - killProcess: terminateProcessTree - }); - clearBrokerSession(cwd); + try { + await shutdownBrokerSession(cwd, { + session: brokerSession, + killProcess: terminateProcessTree + }); + } catch (error) { + failures.push(error); + } + + if (failures.length > 0) { + throw new AggregateError(failures, "Codex session cleanup did not finish."); + } } async function main() { diff --git a/tests/broker-lifecycle.test.mjs b/tests/broker-lifecycle.test.mjs index 2f9f44f29..05a776f31 100644 --- a/tests/broker-lifecycle.test.mjs +++ b/tests/broker-lifecycle.test.mjs @@ -1,39 +1,162 @@ -import test from "node:test"; import assert from "node:assert/strict"; +import fs from "node:fs"; +import net from "node:net"; +import path from "node:path"; +import process from "node:process"; +import test from "node:test"; import { buildEnv, installFakeCodex } from "./fake-codex-fixture.mjs"; import { makeTempDir } from "./helpers.mjs"; import { ensureBrokerSession, loadBrokerSession, - sendBrokerShutdown + saveBrokerSession, + sendBrokerShutdown, + shutdownBrokerSession } from "../plugins/codex/scripts/lib/broker-lifecycle.mjs"; +import { + isProcessTreeRunning, + terminateProcessTree +} from "../plugins/codex/scripts/lib/process.mjs"; +import { resolveStateDir } from "../plugins/codex/scripts/lib/state.mjs"; -test("concurrent ensureBrokerSession calls share a single broker", async () => { +test("concurrent ensureBrokerSession calls share a single authenticated broker", async () => { const workspace = makeTempDir(); const binDir = makeTempDir(); installFakeCodex(binDir); const env = buildEnv(binDir); - let first = null; - try { - const [left, right] = await Promise.all([ - ensureBrokerSession(workspace, { env }), - ensureBrokerSession(workspace, { env }) - ]); - first = left ?? right; - - assert.ok(left, "first ensureBrokerSession returned no session"); - assert.ok(right, "second ensureBrokerSession returned no session"); - assert.equal(left.endpoint, right.endpoint); - assert.equal(left.pid, right.pid); - - const persisted = loadBrokerSession(workspace); - assert.ok(persisted); - assert.equal(persisted.endpoint, left.endpoint); - } finally { - if (first?.endpoint) { - await sendBrokerShutdown(first.endpoint); + const [left, right] = await Promise.all([ + ensureBrokerSession(workspace, { env }), + ensureBrokerSession(workspace, { env }) + ]); + + assert.ok(left, "first ensureBrokerSession returned no session"); + assert.ok(right, "second ensureBrokerSession returned no session"); + assert.equal(left.endpoint, right.endpoint); + assert.equal(left.pid, right.pid); + assert.equal(left.instanceToken, right.instanceToken); + + const persisted = loadBrokerSession(workspace); + assert.ok(persisted); + assert.equal(persisted.endpoint, left.endpoint); + assert.equal(persisted.instanceToken, left.instanceToken); + + const outcome = await shutdownBrokerSession(workspace, { + session: left, + killProcess: terminateProcessTree + }); + assert.equal(outcome.exited, true); + assert.equal(loadBrokerSession(workspace), null); +}); + +test("broker rejects a shutdown token that does not identify its instance", async (t) => { + const workspace = makeTempDir(); + const binDir = makeTempDir(); + installFakeCodex(binDir); + const session = await ensureBrokerSession(workspace, { env: buildEnv(binDir) }); + assert.ok(session?.pid); + + t.after(async () => { + if (loadBrokerSession(workspace)) { + await shutdownBrokerSession(workspace, { + session, + killProcess: terminateProcessTree + }); + } + }); + + const response = await sendBrokerShutdown(session.endpoint, { + instanceToken: "wrong-instance-token", + timeoutMs: 500 + }); + + assert.match(response?.error?.message ?? "", /identity did not match/i); + assert.equal(isProcessTreeRunning(session.pid), true); + assert.ok(loadBrokerSession(workspace)); +}); + +test("shutdown request stops waiting at its deadline", { skip: process.platform === "win32" }, async (t) => { + const sessionDir = makeTempDir("cxc-unresponsive-"); + const socketPath = path.join(sessionDir, "broker.sock"); + const sockets = new Set(); + const server = net.createServer((socket) => { + sockets.add(socket); + socket.on("close", () => sockets.delete(socket)); + socket.on("data", () => { + // Deliberately never answer. + }); + }); + await new Promise((resolve) => server.listen(socketPath, resolve)); + t.after(async () => { + for (const socket of sockets) { + socket.destroy(); } - } + await new Promise((resolve) => server.close(resolve)); + }); + + const startedAt = Date.now(); + const response = await sendBrokerShutdown(`unix:${socketPath}`, { + instanceToken: "instance-token-1234567890", + timeoutMs: 40 + }); + + assert.equal(response, null); + assert.ok(Date.now() - startedAt < 500, "shutdown request exceeded its deadline"); + assert.equal(fs.existsSync(socketPath), true); +}); + +test("shutdown preserves an unowned endpoint and its persisted state", { skip: process.platform === "win32" }, async (t) => { + const workspace = makeTempDir(); + const sessionDir = makeTempDir("cxc-unowned-"); + const socketPath = path.join(sessionDir, "fake-broker.sock"); + const server = net.createServer(); + await new Promise((resolve) => server.listen(socketPath, resolve)); + t.after(async () => { + await new Promise((resolve) => server.close(resolve)); + if (fs.existsSync(socketPath)) { + fs.unlinkSync(socketPath); + } + }); + + const session = { + endpoint: `unix:${socketPath}`, + pid: null, + pidFile: null, + logFile: null, + sessionDir + }; + saveBrokerSession(workspace, session); + + await assert.rejects( + shutdownBrokerSession(workspace, { + session, + timeoutMs: 40, + killProcess: terminateProcessTree + }), + /ownership could not be verified/i + ); + + assert.equal(fs.existsSync(socketPath), true); + assert.deepEqual(loadBrokerSession(workspace), session); +}); + +test("broker metadata, pid, and log files are private", { skip: process.platform === "win32" }, async () => { + const workspace = makeTempDir(); + const binDir = makeTempDir(); + installFakeCodex(binDir); + const session = await ensureBrokerSession(workspace, { env: buildEnv(binDir) }); + assert.ok(session); + + const persistedStateFile = path.join(resolveStateDir(workspace), "broker.json"); + + assert.equal(fs.statSync(session.sessionDir).mode & 0o777, 0o700); + assert.equal(fs.statSync(session.pidFile).mode & 0o777, 0o600); + assert.equal(fs.statSync(session.logFile).mode & 0o777, 0o600); + assert.equal(fs.statSync(persistedStateFile).mode & 0o777, 0o600); + + await shutdownBrokerSession(workspace, { + session, + killProcess: terminateProcessTree + }); }); diff --git a/tests/helpers.mjs b/tests/helpers.mjs index 15f6b9852..6b110ee56 100644 --- a/tests/helpers.mjs +++ b/tests/helpers.mjs @@ -5,7 +5,10 @@ import process from "node:process"; import { spawnSync } from "node:child_process"; import { after } from "node:test"; -import { loadBrokerSession, sendBrokerShutdown, teardownBrokerSession } from "../plugins/codex/scripts/lib/broker-lifecycle.mjs"; +import { + loadBrokerSession, + shutdownBrokerSession +} from "../plugins/codex/scripts/lib/broker-lifecycle.mjs"; import { terminateProcessTree } from "../plugins/codex/scripts/lib/process.mjs"; const trackedTempDirs = []; @@ -16,73 +19,32 @@ export function makeTempDir(prefix = "codex-plugin-test-") { return dir; } -function sleep(ms) { - return new Promise((resolve) => setTimeout(resolve, ms)); -} - -function pidAlive(pid) { - try { - process.kill(pid, 0); - return true; - } catch { - return false; - } -} - -function readBrokerPid(session) { - if (Number.isFinite(session.pid)) { - return session.pid; - } - try { - const parsed = Number.parseInt(fs.readFileSync(session.pidFile, "utf8").trim(), 10); - return Number.isFinite(parsed) ? parsed : null; - } catch { - return null; - } -} - // Global teardown: any broker a test started (directly or lazily) and did not // tear down is shut down here, so the suite never leaves broker/app-server // processes behind — even when a test fails or returns early. after(async () => { - const leakedPids = []; - const leakedSessions = []; + const failures = []; for (const dir of trackedTempDirs) { const session = loadBrokerSession(dir); - if (!session) { + // A fixture may deliberately persist an arbitrary endpoint. Only sessions + // with the per-process secret introduced by ensureBrokerSession are ours. + if (!session?.instanceToken) { continue; } - leakedSessions.push(session); - if (session.endpoint) { - await sendBrokerShutdown(session.endpoint); - } - const pid = readBrokerPid(session); - if (Number.isFinite(pid)) { - leakedPids.push(pid); - } - } - - const deadline = Date.now() + 3000; - for (const pid of leakedPids) { - while (pidAlive(pid) && Date.now() < deadline) { - await sleep(50); - } - if (pidAlive(pid)) { - terminateProcessTree(pid); - await sleep(200); - } - if (pidAlive(pid)) { - throw new Error(`Leaked broker process ${pid} survived suite teardown.`); + try { + await shutdownBrokerSession(dir, { + session, + killProcess: terminateProcessTree, + timeoutMs: 1000, + intervalMs: 25 + }); + } catch (error) { + failures.push(error); } } - for (const session of leakedSessions) { - teardownBrokerSession({ - endpoint: session.endpoint ?? null, - pidFile: session.pidFile ?? null, - logFile: session.logFile ?? null, - sessionDir: session.sessionDir ?? null - }); + if (failures.length > 0) { + throw new AggregateError(failures, "Owned broker teardown did not finish."); } }); diff --git a/tests/locking.test.mjs b/tests/locking.test.mjs new file mode 100644 index 000000000..dcfee78f1 --- /dev/null +++ b/tests/locking.test.mjs @@ -0,0 +1,105 @@ +import assert from "node:assert/strict"; +import fs from "node:fs"; +import path from "node:path"; +import process from "node:process"; +import { spawn } from "node:child_process"; +import test from "node:test"; +import { fileURLToPath, pathToFileURL } from "node:url"; + +import { + acquireLock, + acquireLockSync, + releaseLock +} from "../plugins/codex/scripts/lib/locking.mjs"; +import { makeTempDir } from "./helpers.mjs"; + +test("lock records its owner privately and release removes only that generation", () => { + const lockDir = path.join(makeTempDir(), "state.lock"); + const first = acquireLockSync(lockDir); + const owner = JSON.parse(fs.readFileSync(first.ownerFile, "utf8")); + + assert.equal(owner.pid, process.pid); + assert.equal(owner.token, first.token); + if (process.platform !== "win32") { + assert.equal(fs.statSync(lockDir).mode & 0o777, 0o700); + assert.equal(fs.statSync(first.ownerFile).mode & 0o777, 0o600); + } + + assert.equal(releaseLock(first), true); + const successor = acquireLockSync(lockDir); + + assert.equal(releaseLock(first), false); + assert.equal(fs.existsSync(successor.ownerFile), true); + assert.throws( + () => acquireLockSync(lockDir, { timeoutMs: 30, retryDelayMs: 5 }), + /Timed out waiting for lock/ + ); + + assert.equal(releaseLock(successor), true); +}); + +test("lock immediately reclaims an owner process that exited", async () => { + const lockDir = path.join(makeTempDir(), "state.lock"); + const lockingModuleUrl = pathToFileURL( + path.join( + path.dirname(fileURLToPath(import.meta.url)), + "..", + "plugins", + "codex", + "scripts", + "lib", + "locking.mjs" + ) + ).href; + + await new Promise((resolve, reject) => { + const child = spawn( + process.execPath, + [ + "--input-type=module", + "-e", + `import { acquireLockSync } from ${JSON.stringify(lockingModuleUrl)}; + acquireLockSync(${JSON.stringify(lockDir)}); + process.stdout.write("owned");` + ], + { stdio: ["ignore", "pipe", "pipe"] } + ); + let stdout = ""; + let stderr = ""; + child.stdout.on("data", (chunk) => { + stdout += chunk; + }); + child.stderr.on("data", (chunk) => { + stderr += chunk; + }); + child.on("error", reject); + child.on("exit", (code, signal) => { + if (code === 0 && stdout === "owned") { + resolve(); + return; + } + reject(new Error(`owner failed (code=${code} signal=${signal}): ${stderr}`)); + }); + }); + + const startedAt = Date.now(); + const successor = await acquireLock(lockDir, { + timeoutMs: 500, + staleMs: 30000, + retryDelayMs: 5 + }); + assert.ok(Date.now() - startedAt < 500, "dead owner should be reclaimed before stale timeout"); + releaseLock(successor); +}); + +test("unknown lock contents fail closed", () => { + const lockDir = path.join(makeTempDir(), "state.lock"); + fs.mkdirSync(lockDir); + fs.writeFileSync(path.join(lockDir, "unexpected"), "do not delete\n"); + + assert.throws( + () => acquireLockSync(lockDir, { timeoutMs: 30, staleMs: 0, retryDelayMs: 5 }), + /Timed out waiting for lock/ + ); + assert.equal(fs.readFileSync(path.join(lockDir, "unexpected"), "utf8"), "do not delete\n"); +}); diff --git a/tests/process.test.mjs b/tests/process.test.mjs index 382afe133..d22af123c 100644 --- a/tests/process.test.mjs +++ b/tests/process.test.mjs @@ -2,7 +2,12 @@ import process from "node:process"; import test from "node:test"; import assert from "node:assert/strict"; -import { runCommand, runCommandChecked, terminateProcessTree } from "../plugins/codex/scripts/lib/process.mjs"; +import { + isProcessRunning, + runCommand, + runCommandChecked, + terminateProcessTree +} from "../plugins/codex/scripts/lib/process.mjs"; const SELF_TERMINATING_SCRIPT = "process.kill(process.pid, 'SIGTERM'); setInterval(() => {}, 1000);"; @@ -10,7 +15,7 @@ test("runCommand reports a signal-terminated process as a failure", { skip: proc const result = runCommand(process.execPath, ["-e", SELF_TERMINATING_SCRIPT]); assert.equal(result.signal, "SIGTERM"); - assert.notEqual(result.status, 0); + assert.equal(result.status, null); }); test("runCommandChecked throws when the process dies from a signal", { skip: process.platform === "win32" }, () => { @@ -20,6 +25,18 @@ test("runCommandChecked throws when the process dies from a signal", { skip: pro ); }); +test("Linux zombie processes are treated as exited even when signal 0 succeeds", () => { + const running = isProcessRunning(1234, { + platform: "linux", + killImpl() {}, + readProcessStat() { + return { state: "Z", startTime: "42" }; + } + }); + + assert.equal(running, false); +}); + test("terminateProcessTree uses taskkill on Windows", () => { let captured = null; const outcome = terminateProcessTree(1234, { diff --git a/tests/state.test.mjs b/tests/state.test.mjs index 113df8153..6bfae30de 100644 --- a/tests/state.test.mjs +++ b/tests/state.test.mjs @@ -8,7 +8,17 @@ import { spawn } from "node:child_process"; import { fileURLToPath, pathToFileURL } from "node:url"; import { makeTempDir } from "./helpers.mjs"; -import { listJobs, resolveJobFile, resolveJobLogFile, resolveStateDir, resolveStateFile, saveState } from "../plugins/codex/scripts/lib/state.mjs"; +import { + listJobs, + resolveJobFile, + resolveJobLogFile, + resolveJobsDir, + resolveStateDir, + resolveStateFile, + saveState, + writeJobFile +} from "../plugins/codex/scripts/lib/state.mjs"; +import { createJobLogFile } from "../plugins/codex/scripts/lib/tracked-jobs.mjs"; test("resolveStateDir uses a temp-backed per-workspace directory", () => { const workspace = makeTempDir(); @@ -43,6 +53,23 @@ test("resolveStateDir uses CLAUDE_PLUGIN_DATA when it is provided", () => { } }); +test("state, job, and log artifacts are private", { skip: process.platform === "win32" }, () => { + const workspace = makeTempDir(); + saveState(workspace, { + version: 1, + config: { stopReviewGate: false }, + jobs: [] + }); + const jobFile = writeJobFile(workspace, "private-job", { prompt: "sensitive prompt" }); + const logFile = createJobLogFile(workspace, "private-job", "Private Job"); + + assert.equal(fs.statSync(resolveStateDir(workspace)).mode & 0o777, 0o700); + assert.equal(fs.statSync(resolveJobsDir(workspace)).mode & 0o777, 0o700); + assert.equal(fs.statSync(resolveStateFile(workspace)).mode & 0o777, 0o600); + assert.equal(fs.statSync(jobFile).mode & 0o777, 0o600); + assert.equal(fs.statSync(logFile).mode & 0o777, 0o600); +}); + test("saveState prunes dropped job artifacts when indexed jobs exceed the cap", () => { const workspace = makeTempDir(); const stateFile = resolveStateFile(workspace); From 2f6cc6a72100a4c737d5aae2603f4fc632f4922e Mon Sep 17 00:00:00 2001 From: Heringer Date: Thu, 23 Jul 2026 18:20:12 -0300 Subject: [PATCH 03/20] Reclaim stale sockets left by owned brokers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A broker that is killed after binding its Unix socket can never send a shutdown ack, so ownershipVerified stayed false while the socket file survived on disk. shutdownBrokerSession() observed exited === true and still threw, and because ensureBrokerSession() shuts the previous session down before spawning a replacement, a single stale socket made every later command in that workspace fail until the state and socket files were deleted by hand. Reclaim the endpoint instead of throwing, but only on two independent signals: the recorded PID is no longer running AND the endpoint refuses connections (ECONNREFUSED/ENOENT). A missing ack alone is ambiguous, since a live-but-hung broker also fails to answer, and a bare PID check is defeated by PID reuse. Anything else — a timeout, an unexpected error, a malformed endpoint — stays conservative and keeps the existing failure, so a socket owned by an unrelated process is never unlinked. --- .../codex/scripts/lib/broker-lifecycle.mjs | 72 ++++++++- tests/broker-lifecycle.test.mjs | 148 ++++++++++++++++++ 2 files changed, 214 insertions(+), 6 deletions(-) diff --git a/plugins/codex/scripts/lib/broker-lifecycle.mjs b/plugins/codex/scripts/lib/broker-lifecycle.mjs index 928fb7b38..9c9a140a4 100644 --- a/plugins/codex/scripts/lib/broker-lifecycle.mjs +++ b/plugins/codex/scripts/lib/broker-lifecycle.mjs @@ -247,10 +247,60 @@ function canDiscardUnownedSession(session, pid) { return processExited && !endpointArtifactExists(session.endpoint); } +/** + * Resolves false only when nothing can possibly be listening on the endpoint. + * + * A refused connection (or a socket path that is already gone) is the one signal + * that positively rules out a live listener. Every other outcome — including a + * timeout, which is what a live-but-hung broker produces — resolves true so that + * callers stay conservative and never unlink a socket that someone else owns. + */ +function endpointAcceptsConnection(endpoint, timeoutMs = 250) { + return new Promise((resolve) => { + let socket; + try { + socket = connectToEndpoint(endpoint); + } catch { + resolve(true); + return; + } + let settled = false; + const finish = (value) => { + if (settled) { + return; + } + settled = true; + socket.destroy(); + resolve(value); + }; + socket.setTimeout(timeoutMs, () => finish(true)); + socket.on("connect", () => finish(true)); + socket.on("error", (error) => { + const code = error?.code; + finish(!(code === "ECONNREFUSED" || code === "ENOENT")); + }); + }); +} + +/** + * A stale socket left behind by an owned broker that died before acknowledging + * shutdown. Ownership cannot be proven by RPC in that case — the process that + * would answer is gone — so possession is established from two independent + * signals instead: the recorded PID is no longer running AND the endpoint + * refuses connections. Either one alone is ambiguous (PIDs get reused; a hung + * broker also fails to answer), so both are required before unlinking. + */ +async function canReclaimStaleEndpoint(session, pid, options = {}) { + if (isValidPid(pid) && isProcessTreeRunning(pid, options)) { + return false; + } + return !(await endpointAcceptsConnection(session.endpoint, options.reclaimProbeTimeoutMs)); +} + export async function shutdownBrokerSession(cwd, options = {}) { const session = options.session ?? loadBrokerSession(cwd); if (!session) { - return { found: false, exited: true, forced: false }; + return { found: false, exited: true, forced: false, reclaimedStaleEndpoint: false }; } const pid = resolveBrokerPid(session); @@ -263,7 +313,7 @@ export async function shutdownBrokerSession(cwd, options = {}) { sessionDir: session.sessionDir ?? null }); clearBrokerSession(cwd); - return { found: true, exited: true, forced: false }; + return { found: true, exited: true, forced: false, reclaimedStaleEndpoint: false }; } throw new Error("Codex app-server broker ownership could not be verified; persisted state was preserved."); } @@ -362,19 +412,29 @@ export async function shutdownBrokerSession(cwd, options = {}) { if (!exited) { throw new Error(`Codex app-server broker ${verifiedPid ?? session.endpoint ?? "unknown"} did not exit.`); } + // A broker that is killed after binding its socket can never send a shutdown + // ack, so ownershipVerified stays false while the socket file survives. Left + // fatal, that single stale socket wedges every later command in the workspace, + // because ensureBrokerSession() shuts the old session down before starting a + // replacement. Reclaim it when it is provably dead instead of throwing. + let reclaimedStaleEndpoint = false; if (!ownershipVerified && endpointArtifactExists(session.endpoint)) { - throw new Error("Codex app-server broker endpoint ownership could not be verified; persisted state was preserved."); + reclaimedStaleEndpoint = await canReclaimStaleEndpoint(session, pid, options); + if (!reclaimedStaleEndpoint) { + throw new Error("Codex app-server broker endpoint ownership could not be verified; persisted state was preserved."); + } } + const endpointIsOurs = ownershipVerified || reclaimedStaleEndpoint; teardownBrokerSession({ - endpoint: ownershipVerified ? session.endpoint ?? null : null, + endpoint: endpointIsOurs ? session.endpoint ?? null : null, pidFile: session.pidFile ?? null, logFile: session.logFile ?? null, sessionDir: session.sessionDir ?? null, - ownershipVerified + ownershipVerified: endpointIsOurs }); clearBrokerSession(cwd); - return { found: true, exited: true, forced }; + return { found: true, exited: true, forced, reclaimedStaleEndpoint }; } async function isBrokerEndpointReady(endpoint) { diff --git a/tests/broker-lifecycle.test.mjs b/tests/broker-lifecycle.test.mjs index 05a776f31..dab49b2dd 100644 --- a/tests/broker-lifecycle.test.mjs +++ b/tests/broker-lifecycle.test.mjs @@ -4,6 +4,7 @@ import net from "node:net"; import path from "node:path"; import process from "node:process"; import test from "node:test"; +import { spawn } from "node:child_process"; import { buildEnv, installFakeCodex } from "./fake-codex-fixture.mjs"; import { makeTempDir } from "./helpers.mjs"; @@ -160,3 +161,150 @@ test("broker metadata, pid, and log files are private", { skip: process.platform killProcess: terminateProcessTree }); }); + +async function spawnSocketHolder(socketPath) { + const child = spawn( + process.execPath, + [ + "-e", + `const net = require("node:net"); + const server = net.createServer(); + server.listen(${JSON.stringify(socketPath)}, () => process.stdout.write("ready\\n")); + setInterval(() => {}, 60000);` + ], + { stdio: ["ignore", "pipe", "ignore"] } + ); + await new Promise((resolve, reject) => { + child.stdout.on("data", (chunk) => { + if (String(chunk).includes("ready")) { + resolve(); + } + }); + child.once("error", reject); + child.once("exit", () => reject(new Error("socket holder exited before binding"))); + }); + return child; +} + +test("shutdown reclaims the socket of an owned broker that died without acking", { skip: process.platform === "win32" }, async () => { + const workspace = makeTempDir(); + const sessionDir = makeTempDir("cxc-stale-"); + const socketPath = path.join(sessionDir, "broker.sock"); + + // Killing the holder with SIGKILL skips its cleanup, so the socket file stays + // on disk with nothing listening — exactly what a crashed broker leaves behind. + const holder = await spawnSocketHolder(socketPath); + const deadPid = holder.pid; + holder.kill("SIGKILL"); + await new Promise((resolve) => holder.once("exit", resolve)); + assert.equal(fs.existsSync(socketPath), true, "stale socket should survive the kill"); + + const session = { + endpoint: `unix:${socketPath}`, + pid: deadPid, + pidFile: null, + logFile: null, + sessionDir, + instanceToken: "stale-instance-token" + }; + saveBrokerSession(workspace, session); + + const outcome = await shutdownBrokerSession(workspace, { + session, + timeoutMs: 40, + killProcess: terminateProcessTree + }); + + assert.equal(outcome.exited, true); + assert.equal(outcome.reclaimedStaleEndpoint, true); + assert.equal(fs.existsSync(socketPath), false, "stale socket should be unlinked"); + assert.equal(loadBrokerSession(workspace), null); +}); + +test("ensureBrokerSession recovers from a stale socket instead of failing", { skip: process.platform === "win32" }, async (t) => { + const workspace = makeTempDir(); + const sessionDir = makeTempDir("cxc-stale-ensure-"); + const socketPath = path.join(sessionDir, "broker.sock"); + const binDir = makeTempDir(); + installFakeCodex(binDir); + + const holder = await spawnSocketHolder(socketPath); + const deadPid = holder.pid; + holder.kill("SIGKILL"); + await new Promise((resolve) => holder.once("exit", resolve)); + + saveBrokerSession(workspace, { + endpoint: `unix:${socketPath}`, + pid: deadPid, + pidFile: null, + logFile: null, + sessionDir, + instanceToken: "stale-instance-token" + }); + + const session = await ensureBrokerSession(workspace, { env: buildEnv(binDir) }); + t.after(async () => { + if (session) { + await shutdownBrokerSession(workspace, { session, killProcess: terminateProcessTree }); + } + }); + + assert.ok(session, "a stale socket must not block the replacement broker"); + assert.notEqual(session.endpoint, `unix:${socketPath}`); +}); + +test("shutdown still refuses to unlink an endpoint someone is listening on", { skip: process.platform === "win32" }, async (t) => { + const workspace = makeTempDir(); + const sessionDir = makeTempDir("cxc-live-listener-"); + const socketPath = path.join(sessionDir, "broker.sock"); + + // Dead PID, but a live listener owns the path: reclaiming would delete the + // socket of an unrelated process, so the shutdown must keep failing. + const holder = await spawnSocketHolder(socketPath); + const stalePid = holder.pid; + holder.kill("SIGKILL"); + await new Promise((resolve) => holder.once("exit", resolve)); + if (fs.existsSync(socketPath)) { + fs.unlinkSync(socketPath); + } + // A listener that accepts but never answers — the shape of a hung broker. Its + // sockets must be tracked: server.close() waits for every accepted connection + // to end, and the shutdown request leaves one open. + const accepted = new Set(); + const server = net.createServer((socket) => { + accepted.add(socket); + socket.on("close", () => accepted.delete(socket)); + }); + await new Promise((resolve) => server.listen(socketPath, resolve)); + t.after(async () => { + for (const socket of accepted) { + socket.destroy(); + } + await new Promise((resolve) => server.close(resolve)); + if (fs.existsSync(socketPath)) { + fs.unlinkSync(socketPath); + } + }); + + const session = { + endpoint: `unix:${socketPath}`, + pid: stalePid, + pidFile: null, + logFile: null, + sessionDir, + instanceToken: "stale-instance-token" + }; + saveBrokerSession(workspace, session); + + await assert.rejects( + shutdownBrokerSession(workspace, { + session, + timeoutMs: 40, + killProcess: terminateProcessTree + }), + /ownership could not be verified/i + ); + + assert.equal(fs.existsSync(socketPath), true); + assert.deepEqual(loadBrokerSession(workspace), session); +}); From f51a8a442a970fe2dc1b4ad6accf9ea2769847e2 Mon Sep 17 00:00:00 2001 From: Heringer Date: Thu, 23 Jul 2026 18:34:58 -0300 Subject: [PATCH 04/20] Harden stale-socket reclaim after adversarial review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three gaps in the previous commit, found while reviewing it: Confine reclaiming to the session directory. A refused connection only proves nothing is listening at that instant — a process that has bound but not yet called listen() also refuses — so a refused connect plus a dead PID was not enough to call the socket ours. Reclaiming now also requires the endpoint to sit inside the mkdtemp 0700 directory this plugin created, which an unrelated process cannot write into. Stop treating a reused PID as dead. isProcessTreeRunning() inspects the process *group* on Linux, so a recycled PID belonging to another group read as gone. Pair it with the plain PID check before concluding the owner exited. Normalise the probe timeout. socket.setTimeout(0) disables the timer, so an explicit zero could leave the probe promise pending forever. Adds regression tests for the two evidence combinations that must never reclaim: live owner with a refused endpoint, and a fully stale-looking endpoint outside the session directory. --- .../codex/scripts/lib/broker-lifecycle.mjs | 55 ++++++++++++-- tests/broker-lifecycle.test.mjs | 75 +++++++++++++++++++ 2 files changed, 123 insertions(+), 7 deletions(-) diff --git a/plugins/codex/scripts/lib/broker-lifecycle.mjs b/plugins/codex/scripts/lib/broker-lifecycle.mjs index 9c9a140a4..4baa3fc6e 100644 --- a/plugins/codex/scripts/lib/broker-lifecycle.mjs +++ b/plugins/codex/scripts/lib/broker-lifecycle.mjs @@ -10,6 +10,7 @@ import { fileURLToPath } from "node:url"; import { createBrokerEndpoint, parseBrokerEndpoint } from "./broker-endpoint.mjs"; import { withLock } from "./locking.mjs"; import { + isProcessRunning, isProcessTreeRunning, processHasLaunchToken, terminateProcessTree, @@ -255,7 +256,10 @@ function canDiscardUnownedSession(session, pid) { * timeout, which is what a live-but-hung broker produces — resolves true so that * callers stay conservative and never unlink a socket that someone else owns. */ -function endpointAcceptsConnection(endpoint, timeoutMs = 250) { +function endpointAcceptsConnection(endpoint, timeoutMs) { + // socket.setTimeout(0) disables the timer outright, which would leave this + // promise pending forever on a connection that never settles. + const deadlineMs = Number.isFinite(timeoutMs) && timeoutMs > 0 ? timeoutMs : 250; return new Promise((resolve) => { let socket; try { @@ -273,7 +277,7 @@ function endpointAcceptsConnection(endpoint, timeoutMs = 250) { socket.destroy(); resolve(value); }; - socket.setTimeout(timeoutMs, () => finish(true)); + socket.setTimeout(deadlineMs, () => finish(true)); socket.on("connect", () => finish(true)); socket.on("error", (error) => { const code = error?.code; @@ -282,16 +286,53 @@ function endpointAcceptsConnection(endpoint, timeoutMs = 250) { }); } +/** + * True when the endpoint path lives inside the session directory this plugin + * created. Those directories come from mkdtemp with mode 0700, so a path under + * one is ours by construction — an unrelated process cannot have placed its + * socket there. Reclaiming is confined to that subtree so a persisted endpoint + * pointing anywhere else is never unlinked. + */ +function endpointIsInsideSessionDir(session) { + if (!session.sessionDir || !session.endpoint) { + return false; + } + try { + const target = parseBrokerEndpoint(session.endpoint); + if (target.kind !== "unix") { + return false; + } + const sessionDir = path.resolve(session.sessionDir); + const relative = path.relative(sessionDir, path.resolve(target.path)); + return relative !== "" && !relative.startsWith("..") && !path.isAbsolute(relative); + } catch { + return false; + } +} + /** * A stale socket left behind by an owned broker that died before acknowledging * shutdown. Ownership cannot be proven by RPC in that case — the process that - * would answer is gone — so possession is established from two independent - * signals instead: the recorded PID is no longer running AND the endpoint - * refuses connections. Either one alone is ambiguous (PIDs get reused; a hung - * broker also fails to answer), so both are required before unlinking. + * would answer is gone — so possession is established from independent signals + * instead, all of which must hold: + * + * 1. the socket sits inside the 0700 session directory we created; + * 2. neither the recorded PID nor its process group is still running; + * 3. connecting to the endpoint is refused. + * + * No single one is sufficient. A hung broker also fails to answer, PID numbers + * get reused, and a refused connect only proves nothing is listening *right + * now* — a process that has bound but not yet listened also refuses. Requiring + * all three keeps the blast radius inside our own temp directory. */ async function canReclaimStaleEndpoint(session, pid, options = {}) { - if (isValidPid(pid) && isProcessTreeRunning(pid, options)) { + if (!endpointIsInsideSessionDir(session)) { + return false; + } + // isProcessTreeRunning() checks the process *group* on Linux, so a reused PID + // in another group reads as dead. Pair it with the plain PID check before + // treating the owner as gone. + if (isValidPid(pid) && (isProcessTreeRunning(pid, options) || isProcessRunning(pid, options))) { return false; } return !(await endpointAcceptsConnection(session.endpoint, options.reclaimProbeTimeoutMs)); diff --git a/tests/broker-lifecycle.test.mjs b/tests/broker-lifecycle.test.mjs index dab49b2dd..0a89c63ac 100644 --- a/tests/broker-lifecycle.test.mjs +++ b/tests/broker-lifecycle.test.mjs @@ -308,3 +308,78 @@ test("shutdown still refuses to unlink an endpoint someone is listening on", { s assert.equal(fs.existsSync(socketPath), true); assert.deepEqual(loadBrokerSession(workspace), session); }); + +test("a refused endpoint alone does not justify reclaiming while the owner lives", { skip: process.platform === "win32" }, async (t) => { + const workspace = makeTempDir(); + const sessionDir = makeTempDir("cxc-live-owner-"); + const socketPath = path.join(sessionDir, "broker.sock"); + + // Live owner, refused endpoint: the socket file exists but nothing listens on + // it, which on its own looks exactly like the stale case. The recorded PID is + // still running, so the shutdown must not unlink anything. + const holder = await spawnSocketHolder(socketPath); + t.after(() => { + holder.kill("SIGKILL"); + }); + fs.unlinkSync(socketPath); + fs.writeFileSync(socketPath, ""); + + const session = { + endpoint: `unix:${socketPath}`, + pid: holder.pid, + pidFile: null, + logFile: null, + sessionDir, + instanceToken: "live-owner-token" + }; + saveBrokerSession(workspace, session); + + await assert.rejects( + shutdownBrokerSession(workspace, { + session, + timeoutMs: 40, + killProcess: () => {} + }), + /ownership could not be verified|did not exit/i + ); + + assert.equal(fs.existsSync(socketPath), true); + assert.deepEqual(loadBrokerSession(workspace), session); +}); + +test("an endpoint outside the plugin session directory is never reclaimed", { skip: process.platform === "win32" }, async () => { + const workspace = makeTempDir(); + const sessionDir = makeTempDir("cxc-outside-"); + const elsewhere = makeTempDir("not-a-session-dir-"); + const socketPath = path.join(elsewhere, "broker.sock"); + + const holder = await spawnSocketHolder(socketPath); + const deadPid = holder.pid; + holder.kill("SIGKILL"); + await new Promise((resolve) => holder.once("exit", resolve)); + assert.equal(fs.existsSync(socketPath), true); + + // Everything else looks reclaimable — dead PID, refused connect — but the + // endpoint does not live under the session directory we created. + const session = { + endpoint: `unix:${socketPath}`, + pid: deadPid, + pidFile: null, + logFile: null, + sessionDir, + instanceToken: "outside-token" + }; + saveBrokerSession(workspace, session); + + await assert.rejects( + shutdownBrokerSession(workspace, { + session, + timeoutMs: 40, + killProcess: terminateProcessTree + }), + /ownership could not be verified/i + ); + + assert.equal(fs.existsSync(socketPath), true, "foreign socket must survive"); + fs.unlinkSync(socketPath); +}); From c0b5eb243f441460758c7f60a7025b5c667550e0 Mon Sep 17 00:00:00 2001 From: Heringer Date: Fri, 24 Jul 2026 22:17:39 -0300 Subject: [PATCH 05/20] Fix tokenless legacy broker upgrades --- .../codex/scripts/lib/broker-lifecycle.mjs | 58 ++++++++-- plugins/codex/scripts/lib/process.mjs | 56 +++++++++ tests/broker-lifecycle.test.mjs | 106 ++++++++++++++++++ tests/process.test.mjs | 29 +++++ 4 files changed, 242 insertions(+), 7 deletions(-) diff --git a/plugins/codex/scripts/lib/broker-lifecycle.mjs b/plugins/codex/scripts/lib/broker-lifecycle.mjs index 4baa3fc6e..e216d6842 100644 --- a/plugins/codex/scripts/lib/broker-lifecycle.mjs +++ b/plugins/codex/scripts/lib/broker-lifecycle.mjs @@ -12,6 +12,7 @@ import { withLock } from "./locking.mjs"; import { isProcessRunning, isProcessTreeRunning, + processHasLaunchSequence, processHasLaunchToken, terminateProcessTree, waitForProcessExit @@ -310,6 +311,41 @@ function endpointIsInsideSessionDir(session) { } } +function processMatchesLegacyBroker(cwd, session, pid, options = {}) { + const platform = options.platform ?? process.platform; + let expectedEndpoint; + try { + expectedEndpoint = createBrokerEndpoint(session.sessionDir, platform); + } catch { + return false; + } + if ( + !isValidPid(pid) || + session.endpoint !== expectedEndpoint || + typeof session.pidFile !== "string" || + session.pidFile !== path.join(session.sessionDir, "broker.pid") + ) { + return false; + } + return processHasLaunchSequence( + pid, + [ + "serve", + "--endpoint", + session.endpoint, + "--cwd", + cwd, + "--pid-file", + session.pidFile + ], + { + platform, + timeoutMs: options.timeoutMs, + runCommandImpl: options.runCommandImpl + } + ); +} + /** * A stale socket left behind by an owned broker that died before acknowledging * shutdown. Ownership cannot be proven by RPC in that case — the process that @@ -345,7 +381,9 @@ export async function shutdownBrokerSession(cwd, options = {}) { } const pid = resolveBrokerPid(session); - if (session.endpoint && !session.instanceToken) { + const legacySession = Boolean(session.endpoint && !session.instanceToken); + let legacyProcessVerified = false; + if (legacySession) { if (canDiscardUnownedSession(session, pid)) { teardownBrokerSession({ endpoint: null, @@ -356,7 +394,12 @@ export async function shutdownBrokerSession(cwd, options = {}) { clearBrokerSession(cwd); return { found: true, exited: true, forced: false, reclaimedStaleEndpoint: false }; } - throw new Error("Codex app-server broker ownership could not be verified; persisted state was preserved."); + legacyProcessVerified = processMatchesLegacyBroker(cwd, session, pid, options); + const legacyEndpointIsSafelyStale = + isValidPid(pid) && (await canReclaimStaleEndpoint(session, pid, options)); + if (!legacyProcessVerified && !legacyEndpointIsSafelyStale) { + throw new Error("Codex app-server broker ownership could not be verified; persisted state was preserved."); + } } let shutdownResponse = null; @@ -377,15 +420,16 @@ export async function shutdownBrokerSession(cwd, options = {}) { const shutdownAck = shutdownResponse?.result ?? null; const acknowledgedPid = isValidPid(shutdownAck?.pid) ? shutdownAck.pid : null; const ownershipVerified = - Boolean(session.instanceToken) && - shutdownAck?.instanceToken === session.instanceToken && - acknowledgedPid !== null && - (pid === null || pid === acknowledgedPid); + (Boolean(session.instanceToken) && + shutdownAck?.instanceToken === session.instanceToken && + acknowledgedPid !== null && + (pid === null || pid === acknowledgedPid)) || + (legacySession && legacyProcessVerified && Boolean(shutdownAck)); if (shutdownAck && !ownershipVerified) { throw new Error("Codex app-server broker shutdown identity did not match persisted state."); } - let verifiedPid = ownershipVerified ? acknowledgedPid : null; + let verifiedPid = ownershipVerified ? acknowledgedPid ?? pid : null; let exited = isValidPid(pid) ? await waitForProcessExit(pid, { timeoutMs: 0, diff --git a/plugins/codex/scripts/lib/process.mjs b/plugins/codex/scripts/lib/process.mjs index 40c2e6e50..7fe3f2d65 100644 --- a/plugins/codex/scripts/lib/process.mjs +++ b/plugins/codex/scripts/lib/process.mjs @@ -200,6 +200,62 @@ export async function waitForProcessExit(pid, options = {}) { return !isRunning(pid, options); } +export function processHasLaunchSequence(pid, expectedArgs, options = {}) { + if ( + !isValidPid(pid) || + !Array.isArray(expectedArgs) || + expectedArgs.length === 0 || + expectedArgs.some((arg) => typeof arg !== "string" || arg.length === 0) + ) { + return false; + } + + const platform = options.platform ?? process.platform; + if (platform === "linux") { + let argv; + try { + argv = fs.readFileSync(`/proc/${pid}/cmdline`, "utf8").split("\0").filter(Boolean); + } catch { + return false; + } + return argv.some((_, start) => + expectedArgs.every((expected, offset) => argv[start + offset] === expected) + ); + } + + const runCommandImpl = options.runCommandImpl ?? runCommand; + const result = + platform === "win32" + ? runCommandImpl( + "powershell.exe", + [ + "-NoProfile", + "-NonInteractive", + "-Command", + `$p = Get-CimInstance Win32_Process -Filter 'ProcessId = ${pid}'; if ($null -ne $p) { [Console]::Out.Write($p.CommandLine) }` + ], + { timeout: options.timeoutMs ?? 2000, killSignal: "SIGTERM" } + ) + : runCommandImpl("ps", ["-ww", "-p", String(pid), "-o", "command="], { + timeout: options.timeoutMs ?? 2000, + killSignal: "SIGTERM" + }); + + if (result.error || result.signal != null || result.status !== 0) { + return false; + } + const commandLine = String(result.stdout ?? ""); + let cursor = 0; + for (const expected of expectedArgs) { + const index = commandLine.indexOf(expected, cursor); + if (index === -1) { + return false; + } + cursor = index + expected.length; + } + return true; +} + export function processHasLaunchToken(pid, token, options = {}) { if (!isValidPid(pid) || typeof token !== "string" || token.length < 16) { return false; diff --git a/tests/broker-lifecycle.test.mjs b/tests/broker-lifecycle.test.mjs index 0a89c63ac..efd97650a 100644 --- a/tests/broker-lifecycle.test.mjs +++ b/tests/broker-lifecycle.test.mjs @@ -142,6 +142,112 @@ test("shutdown preserves an unowned endpoint and its persisted state", { skip: p assert.deepEqual(loadBrokerSession(workspace), session); }); +test("shutdown retires a live tokenless broker left by the previous version", { skip: process.platform === "win32" }, async (t) => { + const workspace = makeTempDir(); + const sessionDir = makeTempDir("cxc-legacy-"); + const socketPath = path.join(sessionDir, "broker.sock"); + const pidFile = path.join(sessionDir, "broker.pid"); + const endpoint = `unix:${socketPath}`; + const child = spawn( + process.execPath, + [ + "-e", + `const fs = require("node:fs"); + const net = require("node:net"); + const socketPath = ${JSON.stringify(socketPath)}; + const pidFile = ${JSON.stringify(pidFile)}; + fs.writeFileSync(pidFile, String(process.pid)); + const server = net.createServer((socket) => { + socket.setEncoding("utf8"); + let buffer = ""; + socket.on("data", (chunk) => { + buffer += chunk; + if (!buffer.includes("\\n")) return; + const request = JSON.parse(buffer.slice(0, buffer.indexOf("\\n"))); + if (request.method !== "broker/shutdown") return; + socket.end(JSON.stringify({ id: request.id, result: {} }) + "\\n", () => { + server.close(() => process.exit(0)); + }); + }); + }); + server.listen(socketPath, () => process.stdout.write("ready\\n"));`, + "serve", + "--endpoint", + endpoint, + "--cwd", + workspace, + "--pid-file", + pidFile + ], + { detached: true, stdio: ["ignore", "pipe", "ignore"] } + ); + await new Promise((resolve, reject) => { + child.stdout.on("data", (chunk) => { + if (String(chunk).includes("ready")) { + resolve(); + } + }); + child.once("error", reject); + child.once("exit", () => reject(new Error("legacy broker exited before binding"))); + }); + t.after(() => { + if (isProcessTreeRunning(child.pid)) { + terminateProcessTree(child.pid); + } + }); + + const session = { + endpoint, + pid: child.pid, + pidFile, + logFile: null, + sessionDir + }; + saveBrokerSession(workspace, session); + + const outcome = await shutdownBrokerSession(workspace, { + session, + timeoutMs: 500, + intervalMs: 10, + killProcess: terminateProcessTree + }); + + assert.equal(outcome.exited, true); + assert.equal(outcome.forced, false); + assert.equal(loadBrokerSession(workspace), null); + assert.equal(fs.existsSync(socketPath), false); +}); + +test("shutdown does not retire an authenticated broker whose persisted token was lost", { skip: process.platform === "win32" }, async (t) => { + const workspace = makeTempDir(); + const binDir = makeTempDir(); + installFakeCodex(binDir); + const authenticatedSession = await ensureBrokerSession(workspace, { env: buildEnv(binDir) }); + assert.ok(authenticatedSession?.instanceToken); + t.after(async () => { + saveBrokerSession(workspace, authenticatedSession); + await shutdownBrokerSession(workspace, { + session: authenticatedSession, + killProcess: terminateProcessTree + }); + }); + + const { instanceToken: _lostToken, ...tokenlessState } = authenticatedSession; + saveBrokerSession(workspace, tokenlessState); + + await assert.rejects( + shutdownBrokerSession(workspace, { + session: tokenlessState, + timeoutMs: 200, + killProcess: terminateProcessTree + }), + /rejected shutdown identity/i + ); + + assert.equal(isProcessTreeRunning(authenticatedSession.pid), true); + assert.deepEqual(loadBrokerSession(workspace), tokenlessState); +}); + test("broker metadata, pid, and log files are private", { skip: process.platform === "win32" }, async () => { const workspace = makeTempDir(); const binDir = makeTempDir(); diff --git a/tests/process.test.mjs b/tests/process.test.mjs index d22af123c..bb47002b0 100644 --- a/tests/process.test.mjs +++ b/tests/process.test.mjs @@ -4,6 +4,7 @@ import assert from "node:assert/strict"; import { isProcessRunning, + processHasLaunchSequence, runCommand, runCommandChecked, terminateProcessTree @@ -37,6 +38,34 @@ test("Linux zombie processes are treated as exited even when signal 0 succeeds", assert.equal(running, false); }); +test("process launch sequence fallback requires arguments in order", () => { + const runCommandImpl = (command, args) => ({ + command, + args, + status: 0, + signal: null, + stdout: "node app-server-broker.mjs serve --endpoint pipe:broker --cwd /workspace --pid-file /tmp/broker.pid", + stderr: "", + error: null + }); + + assert.equal( + processHasLaunchSequence( + 1234, + ["serve", "--endpoint", "pipe:broker", "--cwd", "/workspace", "--pid-file", "/tmp/broker.pid"], + { platform: "darwin", runCommandImpl } + ), + true + ); + assert.equal( + processHasLaunchSequence(1234, ["--cwd", "/workspace", "--endpoint", "pipe:broker"], { + platform: "darwin", + runCommandImpl + }), + false + ); +}); + test("terminateProcessTree uses taskkill on Windows", () => { let captured = null; const outcome = terminateProcessTree(1234, { From 64fdef1cb0a397c27c51ed2d71e9eaa08e46852c Mon Sep 17 00:00:00 2001 From: Heringer Date: Fri, 24 Jul 2026 22:26:38 -0300 Subject: [PATCH 06/20] Fix reused-PID lock ownership across platforms --- plugins/codex/scripts/lib/locking.mjs | 14 +++++- plugins/codex/scripts/lib/process.mjs | 31 +++++++++++- tests/locking.test.mjs | 28 +++++++++++ tests/process.test.mjs | 70 +++++++++++++++++++++++++++ 4 files changed, 139 insertions(+), 4 deletions(-) diff --git a/plugins/codex/scripts/lib/locking.mjs b/plugins/codex/scripts/lib/locking.mjs index 02f9ca77f..309f091b7 100644 --- a/plugins/codex/scripts/lib/locking.mjs +++ b/plugins/codex/scripts/lib/locking.mjs @@ -10,6 +10,9 @@ const DEFAULT_STALE_MS = 30000; const RETRY_DELAY_MS = 25; const OWNER_FILE_PREFIX = "owner-"; const OWNER_FILE_SUFFIX = ".json"; +// Start-time lookup may spawn `ps` or PowerShell off Linux. Cache it once per +// process instead of paying that cost for every short state update. +const PROCESS_IDENTITY = getProcessIdentity(process.pid); function sleepSync(ms) { Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, ms); @@ -64,7 +67,7 @@ function tryAcquire(lockDir) { pid: process.pid, token, createdAt: Date.now(), - processIdentity: getProcessIdentity(process.pid) + processIdentity: PROCESS_IDENTITY }); fs.renameSync(candidateDir, lockDir); return handle; @@ -144,11 +147,18 @@ function reclaimAbandonedLock(lockDir, options) { } if (state.kind === "owned") { const processRunning = options.isProcessRunning ?? isProcessRunning; + const processIdentity = state.owner.processIdentity ?? null; if ( processRunning(state.owner.pid, { - identity: state.owner.processIdentity ?? undefined + identity: processIdentity ?? undefined }) ) { + // Older locks on macOS and Windows did not record a process identity. + // Once such a short-lived critical section is stale, a live PID alone + // cannot distinguish the original owner from an unrelated reused PID. + if (processIdentity == null && state.ageMs > options.staleMs) { + return removeOwnedLock(state.ownerFile, lockDir); + } return false; } return removeOwnedLock(state.ownerFile, lockDir); diff --git a/plugins/codex/scripts/lib/process.mjs b/plugins/codex/scripts/lib/process.mjs index 7fe3f2d65..62661a037 100644 --- a/plugins/codex/scripts/lib/process.mjs +++ b/plugins/codex/scripts/lib/process.mjs @@ -90,10 +90,31 @@ export function getProcessIdentity(pid, options = {}) { return null; } const platform = options.platform ?? process.platform; - if (platform !== "linux") { + if (platform === "linux") { + return readLinuxProcessStat(pid)?.startTime ?? null; + } + + const runCommandImpl = options.runCommandImpl ?? runCommand; + const result = + platform === "win32" + ? runCommandImpl( + "powershell.exe", + [ + "-NoProfile", + "-NonInteractive", + "-Command", + `$p = Get-Process -Id ${pid} -ErrorAction SilentlyContinue; if ($null -ne $p) { [Console]::Out.Write($p.StartTime.ToUniversalTime().Ticks) }` + ], + { timeout: options.timeoutMs ?? 2000, killSignal: "SIGTERM" } + ) + : runCommandImpl("ps", ["-ww", "-p", String(pid), "-o", "lstart="], { + timeout: options.timeoutMs ?? 2000, + killSignal: "SIGTERM" + }); + if (result.error || result.signal != null || result.status !== 0) { return null; } - return readLinuxProcessStat(pid)?.startTime ?? null; + return String(result.stdout ?? "").trim() || null; } export function isProcessRunning(pid, options = {}) { @@ -128,6 +149,12 @@ export function isProcessRunning(pid, options = {}) { if (options.identity != null && stat.startTime !== String(options.identity)) { return false; } + } else if (options.identity != null) { + const currentIdentity = getProcessIdentity(pid, options); + // Failure to inspect a live process is not proof that it was replaced. + if (currentIdentity != null && currentIdentity !== String(options.identity)) { + return false; + } } return true; diff --git a/tests/locking.test.mjs b/tests/locking.test.mjs index dcfee78f1..c3fbbeed8 100644 --- a/tests/locking.test.mjs +++ b/tests/locking.test.mjs @@ -103,3 +103,31 @@ test("unknown lock contents fail closed", () => { ); assert.equal(fs.readFileSync(path.join(lockDir, "unexpected"), "utf8"), "do not delete\n"); }); + +test("stale legacy lock without process identity does not follow a reused PID forever", () => { + const lockDir = path.join(makeTempDir(), "state.lock"); + const token = "legacy-owner"; + fs.mkdirSync(lockDir, { mode: 0o700 }); + fs.writeFileSync( + path.join(lockDir, `owner-${token}.json`), + `${JSON.stringify({ + pid: process.pid, + token, + createdAt: Date.now() - 60000, + processIdentity: null + })}\n`, + { mode: 0o600 } + ); + const oldTime = new Date(Date.now() - 60000); + fs.utimesSync(lockDir, oldTime, oldTime); + + const successor = acquireLockSync(lockDir, { + timeoutMs: 200, + staleMs: 30000, + retryDelayMs: 5, + isProcessRunning: () => true + }); + + assert.notEqual(successor.token, token); + assert.equal(releaseLock(successor), true); +}); diff --git a/tests/process.test.mjs b/tests/process.test.mjs index bb47002b0..2909e53f1 100644 --- a/tests/process.test.mjs +++ b/tests/process.test.mjs @@ -3,6 +3,7 @@ import test from "node:test"; import assert from "node:assert/strict"; import { + getProcessIdentity, isProcessRunning, processHasLaunchSequence, runCommand, @@ -38,6 +39,75 @@ test("Linux zombie processes are treated as exited even when signal 0 succeeds", assert.equal(running, false); }); +test("macOS process identity uses the process start time", () => { + let captured = null; + const identity = getProcessIdentity(1234, { + platform: "darwin", + runCommandImpl(command, args) { + captured = { command, args }; + return { + command, + args, + status: 0, + signal: null, + stdout: "Fri Jul 25 01:02:03 2026\n", + stderr: "", + error: null + }; + } + }); + + assert.deepEqual(captured, { + command: "ps", + args: ["-ww", "-p", "1234", "-o", "lstart="] + }); + assert.equal(identity, "Fri Jul 25 01:02:03 2026"); +}); + +test("Windows process identity uses PowerShell start-time ticks", () => { + let capturedCommand = null; + const identity = getProcessIdentity(1234, { + platform: "win32", + runCommandImpl(command, args) { + capturedCommand = { command, args }; + return { + command, + args, + status: 0, + signal: null, + stdout: "638890021230000000", + stderr: "", + error: null + }; + } + }); + + assert.equal(capturedCommand.command, "powershell.exe"); + assert.match(capturedCommand.args.at(-1), /Get-Process -Id 1234/); + assert.equal(identity, "638890021230000000"); +}); + +test("non-Linux process identity distinguishes a reused PID", () => { + const running = isProcessRunning(1234, { + platform: "darwin", + identity: "original-start", + killImpl() {}, + runCommandImpl(command, args) { + return { + command, + args, + status: 0, + signal: null, + stdout: "replacement-start\n", + stderr: "", + error: null + }; + } + }); + + assert.equal(running, false); +}); + test("process launch sequence fallback requires arguments in order", () => { const runCommandImpl = (command, args) => ({ command, From c7f2cbee7b8532b3b6f8ecbfa56e34b4d8b629aa Mon Sep 17 00:00:00 2001 From: Heringer Date: Sat, 25 Jul 2026 01:17:19 -0300 Subject: [PATCH 07/20] Refactor private file and process helpers --- plugins/codex/scripts/app-server-broker.mjs | 10 +- .../codex/scripts/lib/broker-lifecycle.mjs | 99 +++++-------------- plugins/codex/scripts/lib/fs.mjs | 46 +++++++++ plugins/codex/scripts/lib/process.mjs | 69 ++++++------- plugins/codex/scripts/lib/state.mjs | 53 ++-------- plugins/codex/scripts/lib/tracked-jobs.mjs | 8 +- tests/locking.test.mjs | 14 +-- 7 files changed, 118 insertions(+), 181 deletions(-) diff --git a/plugins/codex/scripts/app-server-broker.mjs b/plugins/codex/scripts/app-server-broker.mjs index 09eb82aa1..91fff0da2 100644 --- a/plugins/codex/scripts/app-server-broker.mjs +++ b/plugins/codex/scripts/app-server-broker.mjs @@ -8,6 +8,7 @@ import process from "node:process"; import { parseArgs } from "./lib/args.mjs"; import { BROKER_BUSY_RPC_CODE, CodexAppServerClient } from "./lib/app-server.mjs"; import { parseBrokerEndpoint } from "./lib/broker-endpoint.mjs"; +import { ensurePrivateDir, writePrivateFile } from "./lib/fs.mjs"; const STREAMING_METHODS = new Set(["turn/start", "review/start", "thread/compact/start"]); @@ -41,13 +42,8 @@ function writePidFile(pidFile) { if (!pidFile) { return; } - fs.mkdirSync(path.dirname(pidFile), { recursive: true, mode: 0o700 }); - fs.writeFileSync(pidFile, `${process.pid}\n`, { encoding: "utf8", mode: 0o600 }); - try { - fs.chmodSync(pidFile, 0o600); - } catch { - // Windows and restrictive filesystems may not implement POSIX modes. - } + ensurePrivateDir(path.dirname(pidFile)); + writePrivateFile(pidFile, `${process.pid}\n`); } async function main() { diff --git a/plugins/codex/scripts/lib/broker-lifecycle.mjs b/plugins/codex/scripts/lib/broker-lifecycle.mjs index e216d6842..c7ebe22f3 100644 --- a/plugins/codex/scripts/lib/broker-lifecycle.mjs +++ b/plugins/codex/scripts/lib/broker-lifecycle.mjs @@ -8,6 +8,13 @@ import { spawn } from "node:child_process"; import { fileURLToPath } from "node:url"; import { createBrokerEndpoint, parseBrokerEndpoint } from "./broker-endpoint.mjs"; +import { + ensurePrivateDir, + PRIVATE_DIR_MODE, + PRIVATE_FILE_MODE, + setMode, + writeJsonFileAtomic +} from "./fs.mjs"; import { withLock } from "./locking.mjs"; import { isProcessRunning, @@ -22,21 +29,6 @@ import { resolveStateDir } from "./state.mjs"; export const PID_FILE_ENV = "CODEX_COMPANION_APP_SERVER_PID_FILE"; export const LOG_FILE_ENV = "CODEX_COMPANION_APP_SERVER_LOG_FILE"; const BROKER_STATE_FILE = "broker.json"; -const PRIVATE_DIR_MODE = 0o700; -const PRIVATE_FILE_MODE = 0o600; - -function setMode(filePath, mode) { - try { - fs.chmodSync(filePath, mode); - } catch { - // Windows and restrictive filesystems may not implement POSIX modes. - } -} - -function ensurePrivateDir(dir) { - fs.mkdirSync(dir, { recursive: true, mode: PRIVATE_DIR_MODE }); - setMode(dir, PRIVATE_DIR_MODE); -} export function createBrokerSessionDir(prefix = "cxc-") { const sessionDir = fs.mkdtempSync(path.join(os.tmpdir(), prefix)); @@ -181,27 +173,7 @@ export function loadBrokerSession(cwd) { export function saveBrokerSession(cwd, session) { const stateDir = resolveStateDir(cwd); ensurePrivateDir(stateDir); - const stateFile = resolveBrokerStateFile(cwd); - const tmpFile = `${stateFile}.${process.pid}.${randomUUID()}.tmp`; - try { - const fd = fs.openSync(tmpFile, "wx", PRIVATE_FILE_MODE); - try { - setMode(tmpFile, PRIVATE_FILE_MODE); - fs.writeFileSync(fd, `${JSON.stringify(session, null, 2)}\n`, "utf8"); - fs.fsyncSync(fd); - } finally { - fs.closeSync(fd); - } - fs.renameSync(tmpFile, stateFile); - setMode(stateFile, PRIVATE_FILE_MODE); - } catch (error) { - try { - fs.unlinkSync(tmpFile); - } catch { - // Temp file may not have been created or may already be gone. - } - throw error; - } + writeJsonFileAtomic(resolveBrokerStateFile(cwd), session); } export function clearBrokerSession(cwd) { @@ -374,6 +346,16 @@ async function canReclaimStaleEndpoint(session, pid, options = {}) { return !(await endpointAcceptsConnection(session.endpoint, options.reclaimProbeTimeoutMs)); } +function waitForBrokerProcessExit(pid, options, timeoutMs) { + return waitForProcessExit(pid, { ...options, timeoutMs }); +} + +function processMatchesInstanceToken(pid, instanceToken, options) { + return options.verifyProcess + ? options.verifyProcess(pid, instanceToken) + : processHasLaunchToken(pid, instanceToken, { ...options, marker: "--instance-token" }); +} + export async function shutdownBrokerSession(cwd, options = {}) { const session = options.session ?? loadBrokerSession(cwd); if (!session) { @@ -430,24 +412,10 @@ export async function shutdownBrokerSession(cwd, options = {}) { } let verifiedPid = ownershipVerified ? acknowledgedPid ?? pid : null; - let exited = isValidPid(pid) - ? await waitForProcessExit(pid, { - timeoutMs: 0, - intervalMs: options.intervalMs, - killImpl: options.killImpl, - platform: options.platform - }) - : false; + let exited = isValidPid(pid) ? await waitForBrokerProcessExit(pid, options, 0) : false; if (!shutdownAck && isValidPid(pid) && !exited) { - const ownsPersistedProcess = options.verifyProcess - ? options.verifyProcess(pid, session.instanceToken) - : processHasLaunchToken(pid, session.instanceToken, { - marker: "--instance-token", - platform: options.platform, - timeoutMs: options.timeoutMs, - runCommandImpl: options.runCommandImpl - }); + const ownsPersistedProcess = processMatchesInstanceToken(pid, session.instanceToken, options); if (!ownsPersistedProcess) { if (isProcessTreeRunning(pid, options)) { throw new Error("Codex app-server broker ownership could not be verified; persisted state was preserved."); @@ -463,35 +431,22 @@ export async function shutdownBrokerSession(cwd, options = {}) { } if (!exited && isValidPid(verifiedPid)) { - exited = await waitForProcessExit(verifiedPid, { - timeoutMs: options.timeoutMs, - intervalMs: options.intervalMs, - killImpl: options.killImpl, - platform: options.platform - }); + exited = await waitForBrokerProcessExit(verifiedPid, options, options.timeoutMs); } let forced = false; if (!exited && isValidPid(verifiedPid) && options.killProcess) { - const stillOwnsProcess = options.verifyProcess - ? options.verifyProcess(verifiedPid, session.instanceToken) - : processHasLaunchToken(verifiedPid, session.instanceToken, { - marker: "--instance-token", - platform: options.platform, - timeoutMs: options.timeoutMs, - runCommandImpl: options.runCommandImpl - }); + const stillOwnsProcess = processMatchesInstanceToken( + verifiedPid, + session.instanceToken, + options + ); if (!stillOwnsProcess) { throw new Error("Codex app-server broker process ownership changed before forced shutdown."); } options.killProcess(verifiedPid); forced = true; - exited = await waitForProcessExit(verifiedPid, { - timeoutMs: options.timeoutMs, - intervalMs: options.intervalMs, - killImpl: options.killImpl, - platform: options.platform - }); + exited = await waitForBrokerProcessExit(verifiedPid, options, options.timeoutMs); } if (!exited) { diff --git a/plugins/codex/scripts/lib/fs.mjs b/plugins/codex/scripts/lib/fs.mjs index 027522442..89ce85253 100644 --- a/plugins/codex/scripts/lib/fs.mjs +++ b/plugins/codex/scripts/lib/fs.mjs @@ -1,6 +1,24 @@ +import { randomUUID } from "node:crypto"; import fs from "node:fs"; import os from "node:os"; import path from "node:path"; +import process from "node:process"; + +export const PRIVATE_DIR_MODE = 0o700; +export const PRIVATE_FILE_MODE = 0o600; + +export function setMode(filePath, mode) { + try { + fs.chmodSync(filePath, mode); + } catch { + // Windows and restrictive filesystems may not implement POSIX modes. + } +} + +export function ensurePrivateDir(dir) { + fs.mkdirSync(dir, { recursive: true, mode: PRIVATE_DIR_MODE }); + setMode(dir, PRIVATE_DIR_MODE); +} export function ensureAbsolutePath(cwd, maybePath) { return path.isAbsolute(maybePath) ? maybePath : path.resolve(cwd, maybePath); @@ -18,6 +36,34 @@ export function writeJsonFile(filePath, value) { fs.writeFileSync(filePath, `${JSON.stringify(value, null, 2)}\n`, "utf8"); } +export function writePrivateFile(filePath, value) { + fs.writeFileSync(filePath, value, { encoding: "utf8", mode: PRIVATE_FILE_MODE }); + setMode(filePath, PRIVATE_FILE_MODE); +} + +export function writeJsonFileAtomic(filePath, value) { + const temporaryFile = `${filePath}.${process.pid}.${randomUUID()}.tmp`; + try { + const fd = fs.openSync(temporaryFile, "wx", PRIVATE_FILE_MODE); + try { + try { + fs.fchmodSync(fd, PRIVATE_FILE_MODE); + } catch { + // Windows and restrictive filesystems may not implement POSIX modes. + } + fs.writeFileSync(fd, `${JSON.stringify(value, null, 2)}\n`, "utf8"); + fs.fsyncSync(fd); + } finally { + fs.closeSync(fd); + } + fs.renameSync(temporaryFile, filePath); + setMode(filePath, PRIVATE_FILE_MODE); + } catch (error) { + fs.rmSync(temporaryFile, { force: true }); + throw error; + } +} + export function safeReadFile(filePath) { return fs.existsSync(filePath) ? fs.readFileSync(filePath, "utf8") : ""; } diff --git a/plugins/codex/scripts/lib/process.mjs b/plugins/codex/scripts/lib/process.mjs index 62661a037..c3f31f358 100644 --- a/plugins/codex/scripts/lib/process.mjs +++ b/plugins/codex/scripts/lib/process.mjs @@ -227,6 +227,31 @@ export async function waitForProcessExit(pid, options = {}) { return !isRunning(pid, options); } +function readProcessCommandLine(pid, options) { + const platform = options.platform ?? process.platform; + const runCommandImpl = options.runCommandImpl ?? runCommand; + const result = + platform === "win32" + ? runCommandImpl( + "powershell.exe", + [ + "-NoProfile", + "-NonInteractive", + "-Command", + `$p = Get-CimInstance Win32_Process -Filter 'ProcessId = ${pid}'; if ($null -ne $p) { [Console]::Out.Write($p.CommandLine) }` + ], + { timeout: options.timeoutMs ?? 2000, killSignal: "SIGTERM" } + ) + : runCommandImpl("ps", ["-ww", "-p", String(pid), "-o", "command="], { + timeout: options.timeoutMs ?? 2000, + killSignal: "SIGTERM" + }); + if (result.error || result.signal != null || result.status !== 0) { + return null; + } + return String(result.stdout ?? ""); +} + export function processHasLaunchSequence(pid, expectedArgs, options = {}) { if ( !isValidPid(pid) || @@ -250,28 +275,10 @@ export function processHasLaunchSequence(pid, expectedArgs, options = {}) { ); } - const runCommandImpl = options.runCommandImpl ?? runCommand; - const result = - platform === "win32" - ? runCommandImpl( - "powershell.exe", - [ - "-NoProfile", - "-NonInteractive", - "-Command", - `$p = Get-CimInstance Win32_Process -Filter 'ProcessId = ${pid}'; if ($null -ne $p) { [Console]::Out.Write($p.CommandLine) }` - ], - { timeout: options.timeoutMs ?? 2000, killSignal: "SIGTERM" } - ) - : runCommandImpl("ps", ["-ww", "-p", String(pid), "-o", "command="], { - timeout: options.timeoutMs ?? 2000, - killSignal: "SIGTERM" - }); - - if (result.error || result.signal != null || result.status !== 0) { + const commandLine = readProcessCommandLine(pid, options); + if (commandLine == null) { return false; } - const commandLine = String(result.stdout ?? ""); let cursor = 0; for (const expected of expectedArgs) { const index = commandLine.indexOf(expected, cursor); @@ -299,28 +306,10 @@ export function processHasLaunchToken(pid, token, options = {}) { } } - const runCommandImpl = options.runCommandImpl ?? runCommand; - const result = - platform === "win32" - ? runCommandImpl( - "powershell.exe", - [ - "-NoProfile", - "-NonInteractive", - "-Command", - `$p = Get-CimInstance Win32_Process -Filter 'ProcessId = ${pid}'; if ($null -ne $p) { [Console]::Out.Write($p.CommandLine) }` - ], - { timeout: options.timeoutMs ?? 2000, killSignal: "SIGTERM" } - ) - : runCommandImpl("ps", ["-ww", "-p", String(pid), "-o", "command="], { - timeout: options.timeoutMs ?? 2000, - killSignal: "SIGTERM" - }); - - if (result.error || result.signal != null || result.status !== 0) { + const commandLine = readProcessCommandLine(pid, options); + if (commandLine == null) { return false; } - const commandLine = String(result.stdout ?? ""); return commandLine.includes(marker) && commandLine.includes(token); } diff --git a/plugins/codex/scripts/lib/state.mjs b/plugins/codex/scripts/lib/state.mjs index da2bb5e4c..5bb85208d 100644 --- a/plugins/codex/scripts/lib/state.mjs +++ b/plugins/codex/scripts/lib/state.mjs @@ -3,6 +3,11 @@ import fs from "node:fs"; import os from "node:os"; import path from "node:path"; +import { + ensurePrivateDir, + writeJsonFileAtomic, + writePrivateFile +} from "./fs.mjs"; import { withLockSync } from "./locking.mjs"; import { resolveWorkspaceRoot } from "./workspace.mjs"; @@ -12,8 +17,6 @@ const FALLBACK_STATE_ROOT_DIR = path.join(os.tmpdir(), "codex-companion"); const STATE_FILE_NAME = "state.json"; const JOBS_DIR_NAME = "jobs"; const MAX_JOBS = 50; -const PRIVATE_DIR_MODE = 0o700; -const PRIVATE_FILE_MODE = 0o600; function nowIso() { return new Date().toISOString(); @@ -57,13 +60,8 @@ export function resolveJobsDir(cwd) { export function ensureStateDir(cwd) { const stateDir = resolveStateDir(cwd); const jobsDir = resolveJobsDir(cwd); - fs.mkdirSync(jobsDir, { recursive: true, mode: PRIVATE_DIR_MODE }); for (const dir of [stateDir, jobsDir]) { - try { - fs.chmodSync(dir, PRIVATE_DIR_MODE); - } catch { - // Windows and restrictive filesystems may not implement POSIX modes. - } + ensurePrivateDir(dir); } } @@ -105,33 +103,6 @@ function resolveStateLockDir(cwd) { return path.join(resolveStateDir(cwd), ".state.lock"); } -function writeStateFileAtomic(stateFile, nextState) { - const tmpFile = `${stateFile}.${process.pid}.${Math.random().toString(36).slice(2)}.tmp`; - try { - const fd = fs.openSync(tmpFile, "wx", PRIVATE_FILE_MODE); - try { - try { - fs.fchmodSync(fd, PRIVATE_FILE_MODE); - } catch { - // Windows and restrictive filesystems may not implement POSIX modes. - } - fs.writeFileSync(fd, `${JSON.stringify(nextState, null, 2)}\n`, "utf8"); - fs.fsyncSync(fd); - } finally { - fs.closeSync(fd); - } - fs.renameSync(tmpFile, stateFile); - try { - fs.chmodSync(stateFile, PRIVATE_FILE_MODE); - } catch { - // Windows and restrictive filesystems may not implement POSIX modes. - } - } catch (error) { - removeFileIfExists(tmpFile); - throw error; - } -} - function saveStateLocked(cwd, state) { const previousJobs = loadState(cwd).jobs; const nextJobs = pruneJobs(state.jobs ?? []); @@ -153,7 +124,7 @@ function saveStateLocked(cwd, state) { removeFileIfExists(job.logFile); } - writeStateFileAtomic(resolveStateFile(cwd), nextState); + writeJsonFileAtomic(resolveStateFile(cwd), nextState); return nextState; } @@ -216,15 +187,7 @@ export function getConfig(cwd) { export function writeJobFile(cwd, jobId, payload) { ensureStateDir(cwd); const jobFile = resolveJobFile(cwd, jobId); - fs.writeFileSync(jobFile, `${JSON.stringify(payload, null, 2)}\n`, { - encoding: "utf8", - mode: PRIVATE_FILE_MODE - }); - try { - fs.chmodSync(jobFile, PRIVATE_FILE_MODE); - } catch { - // Windows and restrictive filesystems may not implement POSIX modes. - } + writePrivateFile(jobFile, `${JSON.stringify(payload, null, 2)}\n`); return jobFile; } diff --git a/plugins/codex/scripts/lib/tracked-jobs.mjs b/plugins/codex/scripts/lib/tracked-jobs.mjs index 73e51c19f..e6b913aaa 100644 --- a/plugins/codex/scripts/lib/tracked-jobs.mjs +++ b/plugins/codex/scripts/lib/tracked-jobs.mjs @@ -1,6 +1,7 @@ import fs from "node:fs"; import process from "node:process"; +import { writePrivateFile } from "./fs.mjs"; import { readJobFile, resolveJobFile, resolveJobLogFile, upsertJob, writeJobFile } from "./state.mjs"; export const SESSION_ID_ENV = "CODEX_COMPANION_SESSION_ID"; @@ -50,12 +51,7 @@ export function appendLogBlock(logFile, title, body) { export function createJobLogFile(workspaceRoot, jobId, title) { const logFile = resolveJobLogFile(workspaceRoot, jobId); - fs.writeFileSync(logFile, "", { encoding: "utf8", mode: 0o600 }); - try { - fs.chmodSync(logFile, 0o600); - } catch { - // Windows and restrictive filesystems may not implement POSIX modes. - } + writePrivateFile(logFile, ""); if (title) { appendLogLine(logFile, `Starting ${title}.`); } diff --git a/tests/locking.test.mjs b/tests/locking.test.mjs index c3fbbeed8..300fe270b 100644 --- a/tests/locking.test.mjs +++ b/tests/locking.test.mjs @@ -4,7 +4,6 @@ import path from "node:path"; import process from "node:process"; import { spawn } from "node:child_process"; import test from "node:test"; -import { fileURLToPath, pathToFileURL } from "node:url"; import { acquireLock, @@ -40,16 +39,9 @@ test("lock records its owner privately and release removes only that generation" test("lock immediately reclaims an owner process that exited", async () => { const lockDir = path.join(makeTempDir(), "state.lock"); - const lockingModuleUrl = pathToFileURL( - path.join( - path.dirname(fileURLToPath(import.meta.url)), - "..", - "plugins", - "codex", - "scripts", - "lib", - "locking.mjs" - ) + const lockingModuleUrl = new URL( + "../plugins/codex/scripts/lib/locking.mjs", + import.meta.url ).href; await new Promise((resolve, reject) => { From 75b6cda7ad4300843004e72ab41203d6476eea48 Mon Sep 17 00:00:00 2001 From: Heringer Date: Sat, 25 Jul 2026 01:20:31 -0300 Subject: [PATCH 08/20] Serialize broker teardown and session cleanup --- .../codex/scripts/lib/broker-lifecycle.mjs | 16 ++++++-- .../codex/scripts/session-lifecycle-hook.mjs | 41 +++++++------------ tests/broker-lifecycle.test.mjs | 41 +++++++++++++++++++ 3 files changed, 68 insertions(+), 30 deletions(-) diff --git a/plugins/codex/scripts/lib/broker-lifecycle.mjs b/plugins/codex/scripts/lib/broker-lifecycle.mjs index c7ebe22f3..60cc92028 100644 --- a/plugins/codex/scripts/lib/broker-lifecycle.mjs +++ b/plugins/codex/scripts/lib/broker-lifecycle.mjs @@ -357,7 +357,11 @@ function processMatchesInstanceToken(pid, instanceToken, options) { } export async function shutdownBrokerSession(cwd, options = {}) { - const session = options.session ?? loadBrokerSession(cwd); + return withBrokerLock(cwd, options, () => shutdownBrokerSessionLocked(cwd, options)); +} + +async function shutdownBrokerSessionLocked(cwd, options = {}) { + const session = loadBrokerSession(cwd) ?? options.session; if (!session) { return { found: false, exited: true, forced: false, reclaimedStaleEndpoint: false }; } @@ -489,11 +493,15 @@ async function isBrokerEndpointReady(endpoint) { } export async function ensureBrokerSession(cwd, options = {}) { + return withBrokerLock(cwd, options, () => ensureBrokerSessionLocked(cwd, options)); +} + +function withBrokerLock(cwd, options, action) { const stateDir = resolveStateDir(cwd); ensurePrivateDir(stateDir); return withLock( path.join(stateDir, ".broker.lock"), - () => ensureBrokerSessionLocked(cwd, options), + action, { timeoutMs: options.lockTimeoutMs ?? 10000 } ); } @@ -505,7 +513,7 @@ async function ensureBrokerSessionLocked(cwd, options = {}) { } if (existing) { - await shutdownBrokerSession(cwd, { + await shutdownBrokerSessionLocked(cwd, { session: existing, killProcess: options.killProcess ?? terminateProcessTree, verifyProcess: options.verifyProcess, @@ -548,7 +556,7 @@ async function ensureBrokerSessionLocked(cwd, options = {}) { const ready = await waitForBrokerEndpoint(endpoint, options.timeoutMs ?? 2000); if (!ready) { - await shutdownBrokerSession(cwd, { + await shutdownBrokerSessionLocked(cwd, { session, killProcess: options.killProcess ?? terminateProcessTree, verifyProcess: options.verifyProcess, diff --git a/plugins/codex/scripts/session-lifecycle-hook.mjs b/plugins/codex/scripts/session-lifecycle-hook.mjs index 91e11b26b..82c01354c 100644 --- a/plugins/codex/scripts/session-lifecycle-hook.mjs +++ b/plugins/codex/scripts/session-lifecycle-hook.mjs @@ -4,11 +4,8 @@ import fs from "node:fs"; import process from "node:process"; import { terminateProcessTree } from "./lib/process.mjs"; -import { - loadBrokerSession, - shutdownBrokerSession -} from "./lib/broker-lifecycle.mjs"; -import { loadState, resolveStateFile, saveState } from "./lib/state.mjs"; +import { shutdownBrokerSession } from "./lib/broker-lifecycle.mjs"; +import { resolveStateFile, updateState } from "./lib/state.mjs"; import { TRANSCRIPT_PATH_ENV } from "./lib/claude-session-transfer.mjs"; import { resolveWorkspaceRoot } from "./lib/workspace.mjs"; @@ -45,27 +42,21 @@ function cleanupSessionJobs(cwd, sessionId) { return; } - const state = loadState(workspaceRoot); - const removedJobs = state.jobs.filter((job) => job.sessionId === sessionId); - if (removedJobs.length === 0) { - return; - } - - for (const job of removedJobs) { - const stillRunning = job.status === "queued" || job.status === "running"; - if (!stillRunning) { - continue; + updateState(workspaceRoot, (state) => { + const removedJobs = state.jobs.filter((job) => job.sessionId === sessionId); + for (const job of removedJobs) { + const stillRunning = job.status === "queued" || job.status === "running"; + if (!stillRunning) { + continue; + } + try { + terminateProcessTree(job.pid ?? Number.NaN); + } catch { + // Ignore teardown failures during session shutdown. + } } - try { - terminateProcessTree(job.pid ?? Number.NaN); - } catch { - // Ignore teardown failures during session shutdown. - } - } - saveState(workspaceRoot, { - ...state, - jobs: state.jobs.filter((job) => job.sessionId !== sessionId) + state.jobs = state.jobs.filter((job) => job.sessionId !== sessionId); }); } @@ -77,7 +68,6 @@ function handleSessionStart(input) { async function handleSessionEnd(input) { const cwd = input.cwd || process.cwd(); - const brokerSession = loadBrokerSession(cwd); const failures = []; try { @@ -88,7 +78,6 @@ async function handleSessionEnd(input) { try { await shutdownBrokerSession(cwd, { - session: brokerSession, killProcess: terminateProcessTree }); } catch (error) { diff --git a/tests/broker-lifecycle.test.mjs b/tests/broker-lifecycle.test.mjs index efd97650a..c78c096b5 100644 --- a/tests/broker-lifecycle.test.mjs +++ b/tests/broker-lifecycle.test.mjs @@ -19,6 +19,10 @@ import { isProcessTreeRunning, terminateProcessTree } from "../plugins/codex/scripts/lib/process.mjs"; +import { + acquireLock, + releaseLock +} from "../plugins/codex/scripts/lib/locking.mjs"; import { resolveStateDir } from "../plugins/codex/scripts/lib/state.mjs"; test("concurrent ensureBrokerSession calls share a single authenticated broker", async () => { @@ -51,6 +55,43 @@ test("concurrent ensureBrokerSession calls share a single authenticated broker", assert.equal(loadBrokerSession(workspace), null); }); +test("shutdown waits for the broker lock and reloads persisted state", async () => { + const workspace = makeTempDir(); + const stateDir = resolveStateDir(workspace); + fs.mkdirSync(stateDir, { recursive: true }); + const lock = await acquireLock(path.join(stateDir, ".broker.lock")); + let settled = false; + + try { + const shutdown = shutdownBrokerSession(workspace); + shutdown.then( + () => { + settled = true; + }, + () => { + settled = true; + } + ); + await new Promise((resolve) => setTimeout(resolve, 50)); + assert.equal(settled, false, "shutdown bypassed the broker lifecycle lock"); + + saveBrokerSession(workspace, { + endpoint: `unix:${path.join(workspace, "missing.sock")}`, + pid: null, + pidFile: null, + logFile: null, + sessionDir: null + }); + releaseLock(lock); + + const outcome = await shutdown; + assert.equal(outcome.found, true); + assert.equal(loadBrokerSession(workspace), null); + } finally { + releaseLock(lock); + } +}); + test("broker rejects a shutdown token that does not identify its instance", async (t) => { const workspace = makeTempDir(); const binDir = makeTempDir(); From 47e48f84978d74f93dcbc210e38450de606767c2 Mon Sep 17 00:00:00 2001 From: Heringer Date: Sat, 25 Jul 2026 06:53:54 -0300 Subject: [PATCH 09/20] Fix shutdown edge cases and consolidate duplicated helpers Behavioral fixes: - sendBrokerShutdown honors timeoutMs: 0 instead of disabling the timer. - Broker shutdown ACK uses destroySoon and destroys lingering client sockets, so a half-open client no longer leaks the broker process. - Lock owner records fail closed on malformed pid/token/processIdentity. - teardownBrokerSession only removes the canonical broker files inside a session directory this plugin provably created (real cxc-* dir directly under tmpdir, non-symlink), never externally supplied paths. - SessionEnd aggregate error now includes the underlying failure text. Consolidation (no behavior change): - Single isValidPid, ENOENT-tolerant removeFileIfExists, shared ps/PowerShell process-table query, shared /proc cmdline reader, single endpoint connect probe, and one lock acquire step shared by the sync/async paths. - Drop dead writeJsonFile, unused PID/LOG env constants, options.session, and demote broker helpers that no longer have external consumers. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_018dXaCHsST2XTpdu63v4Ty4 --- plugins/codex/scripts/app-server-broker.mjs | 10 +- .../codex/scripts/lib/broker-lifecycle.mjs | 247 +++++++----------- plugins/codex/scripts/lib/fs.mjs | 17 +- plugins/codex/scripts/lib/locking.mjs | 30 ++- plugins/codex/scripts/lib/process.mjs | 99 +++---- plugins/codex/scripts/lib/state.mjs | 15 +- .../codex/scripts/session-lifecycle-hook.mjs | 2 +- tests/broker-lifecycle.test.mjs | 104 ++++++-- tests/helpers.mjs | 1 - tests/locking.test.mjs | 20 ++ tests/runtime.test.mjs | 25 ++ 11 files changed, 313 insertions(+), 257 deletions(-) diff --git a/plugins/codex/scripts/app-server-broker.mjs b/plugins/codex/scripts/app-server-broker.mjs index 91fff0da2..696550c61 100644 --- a/plugins/codex/scripts/app-server-broker.mjs +++ b/plugins/codex/scripts/app-server-broker.mjs @@ -106,9 +106,13 @@ async function main() { } } - async function shutdown(server) { + async function shutdown(server, responseSocket = null) { for (const socket of sockets) { - socket.end(); + if (socket === responseSocket) { + socket.destroySoon(); + } else { + socket.destroy(); + } } await appClient.close().catch(() => {}); await new Promise((resolve) => server.close(resolve)); @@ -176,7 +180,7 @@ async function main() { id: message.id, result: { pid: process.pid, instanceToken } }); - await shutdown(server); + await shutdown(server, socket); process.exit(0); } diff --git a/plugins/codex/scripts/lib/broker-lifecycle.mjs b/plugins/codex/scripts/lib/broker-lifecycle.mjs index 60cc92028..f70c5c970 100644 --- a/plugins/codex/scripts/lib/broker-lifecycle.mjs +++ b/plugins/codex/scripts/lib/broker-lifecycle.mjs @@ -12,6 +12,7 @@ import { ensurePrivateDir, PRIVATE_DIR_MODE, PRIVATE_FILE_MODE, + removeFileIfExists, setMode, writeJsonFileAtomic } from "./fs.mjs"; @@ -19,6 +20,7 @@ import { withLock } from "./locking.mjs"; import { isProcessRunning, isProcessTreeRunning, + isValidPid, processHasLaunchSequence, processHasLaunchToken, terminateProcessTree, @@ -26,11 +28,9 @@ import { } from "./process.mjs"; import { resolveStateDir } from "./state.mjs"; -export const PID_FILE_ENV = "CODEX_COMPANION_APP_SERVER_PID_FILE"; -export const LOG_FILE_ENV = "CODEX_COMPANION_APP_SERVER_LOG_FILE"; const BROKER_STATE_FILE = "broker.json"; -export function createBrokerSessionDir(prefix = "cxc-") { +function createBrokerSessionDir(prefix = "cxc-") { const sessionDir = fs.mkdtempSync(path.join(os.tmpdir(), prefix)); setMode(sessionDir, PRIVATE_DIR_MODE); return sessionDir; @@ -41,34 +41,57 @@ function connectToEndpoint(endpoint) { return net.createConnection({ path: target.path }); } -export async function waitForBrokerEndpoint(endpoint, timeoutMs = 2000) { +/** + * One-shot connection probe. Resolves "connect" when something accepted the + * connection, "timeout" when nothing settled within timeoutMs, "invalid" when + * the endpoint cannot even be parsed, and the error code otherwise. + */ +function probeEndpoint(endpoint, timeoutMs) { + return new Promise((resolve) => { + let socket; + try { + socket = connectToEndpoint(endpoint); + } catch { + resolve("invalid"); + return; + } + let settled = false; + const finish = (value) => { + if (settled) { + return; + } + settled = true; + socket.destroy(); + resolve(value); + }; + // socket.setTimeout(0) disables the timer outright, which would leave this + // promise pending forever on a connection that never settles. + socket.setTimeout(Math.max(1, timeoutMs), () => finish("timeout")); + socket.on("connect", () => finish("connect")); + socket.on("error", (error) => finish(error?.code ?? "error")); + }); +} + +async function waitForBrokerEndpoint(endpoint, timeoutMs = 2000) { const deadline = Date.now() + timeoutMs; while (Date.now() < deadline) { - const ready = await new Promise((resolve) => { - const socket = connectToEndpoint(endpoint); - let settled = false; - const finish = (value) => { - if (settled) { - return; - } - settled = true; - socket.destroy(); - resolve(value); - }; - socket.setTimeout(Math.max(1, Math.min(150, deadline - Date.now())), () => finish(false)); - socket.on("connect", () => finish(true)); - socket.on("error", () => finish(false)); - }); - if (ready) { + const probe = await probeEndpoint(endpoint, Math.min(150, deadline - Date.now())); + if (probe === "connect") { return true; } + if (probe === "invalid") { + return false; + } await new Promise((resolve) => setTimeout(resolve, 50)); } return false; } -export async function sendBrokerShutdown(endpoint, options = {}) { - return await new Promise((resolve) => { +export function sendBrokerShutdown(endpoint, options = {}) { + const timeoutMs = Number.isFinite(options.timeoutMs) + ? Math.max(1, options.timeoutMs) + : 2000; + return new Promise((resolve) => { const socket = connectToEndpoint(endpoint); let settled = false; let buffer = ""; @@ -81,7 +104,7 @@ export async function sendBrokerShutdown(endpoint, options = {}) { resolve(result); }; socket.setEncoding("utf8"); - socket.setTimeout(options.timeoutMs ?? 2000, () => finish(null)); + socket.setTimeout(timeoutMs, () => finish(null)); socket.on("connect", () => { socket.write( `${JSON.stringify({ @@ -109,11 +132,7 @@ export async function sendBrokerShutdown(endpoint, options = {}) { }); } -function isValidPid(pid) { - return Number.isSafeInteger(pid) && pid > 0; -} - -export function spawnBrokerProcess({ +function spawnBrokerProcess({ scriptPath, cwd, endpoint, @@ -176,15 +195,8 @@ export function saveBrokerSession(cwd, session) { writeJsonFileAtomic(resolveBrokerStateFile(cwd), session); } -export function clearBrokerSession(cwd) { - const stateFile = resolveBrokerStateFile(cwd); - try { - fs.unlinkSync(stateFile); - } catch (error) { - if (error?.code !== "ENOENT") { - throw error; - } - } +function clearBrokerSession(cwd) { + removeFileIfExists(resolveBrokerStateFile(cwd)); } function resolveBrokerPid(session) { @@ -229,55 +241,36 @@ function canDiscardUnownedSession(session, pid) { * timeout, which is what a live-but-hung broker produces — resolves true so that * callers stay conservative and never unlink a socket that someone else owns. */ -function endpointAcceptsConnection(endpoint, timeoutMs) { - // socket.setTimeout(0) disables the timer outright, which would leave this - // promise pending forever on a connection that never settles. +async function endpointAcceptsConnection(endpoint, timeoutMs) { const deadlineMs = Number.isFinite(timeoutMs) && timeoutMs > 0 ? timeoutMs : 250; - return new Promise((resolve) => { - let socket; - try { - socket = connectToEndpoint(endpoint); - } catch { - resolve(true); - return; - } - let settled = false; - const finish = (value) => { - if (settled) { - return; - } - settled = true; - socket.destroy(); - resolve(value); - }; - socket.setTimeout(deadlineMs, () => finish(true)); - socket.on("connect", () => finish(true)); - socket.on("error", (error) => { - const code = error?.code; - finish(!(code === "ECONNREFUSED" || code === "ENOENT")); - }); - }); + const probe = await probeEndpoint(endpoint, deadlineMs); + return probe !== "ECONNREFUSED" && probe !== "ENOENT"; } -/** - * True when the endpoint path lives inside the session directory this plugin - * created. Those directories come from mkdtemp with mode 0700, so a path under - * one is ours by construction — an unrelated process cannot have placed its - * socket there. Reclaiming is confined to that subtree so a persisted endpoint - * pointing anywhere else is never unlinked. - */ -function endpointIsInsideSessionDir(session) { - if (!session.sessionDir || !session.endpoint) { +function resolveOwnedSessionDir(sessionDir) { + if (typeof sessionDir !== "string") { + return null; + } + const resolved = path.resolve(sessionDir); + const relative = path.relative(path.resolve(os.tmpdir()), resolved); + if (path.dirname(relative) !== "." || !path.basename(relative).startsWith("cxc-")) { + return null; + } + try { + const stats = fs.lstatSync(resolved); + return stats.isDirectory() && !stats.isSymbolicLink() ? resolved : null; + } catch { + return null; + } +} + +function endpointBelongsToSession(session, platform = process.platform) { + const sessionDir = resolveOwnedSessionDir(session.sessionDir); + if (!sessionDir || !session.endpoint) { return false; } try { - const target = parseBrokerEndpoint(session.endpoint); - if (target.kind !== "unix") { - return false; - } - const sessionDir = path.resolve(session.sessionDir); - const relative = path.relative(sessionDir, path.resolve(target.path)); - return relative !== "" && !relative.startsWith("..") && !path.isAbsolute(relative); + return session.endpoint === createBrokerEndpoint(sessionDir, platform); } catch { return false; } @@ -334,7 +327,7 @@ function processMatchesLegacyBroker(cwd, session, pid, options = {}) { * all three keeps the blast radius inside our own temp directory. */ async function canReclaimStaleEndpoint(session, pid, options = {}) { - if (!endpointIsInsideSessionDir(session)) { + if (!endpointBelongsToSession(session, options.platform)) { return false; } // isProcessTreeRunning() checks the process *group* on Linux, so a reused PID @@ -346,10 +339,6 @@ async function canReclaimStaleEndpoint(session, pid, options = {}) { return !(await endpointAcceptsConnection(session.endpoint, options.reclaimProbeTimeoutMs)); } -function waitForBrokerProcessExit(pid, options, timeoutMs) { - return waitForProcessExit(pid, { ...options, timeoutMs }); -} - function processMatchesInstanceToken(pid, instanceToken, options) { return options.verifyProcess ? options.verifyProcess(pid, instanceToken) @@ -361,7 +350,7 @@ export async function shutdownBrokerSession(cwd, options = {}) { } async function shutdownBrokerSessionLocked(cwd, options = {}) { - const session = loadBrokerSession(cwd) ?? options.session; + const session = loadBrokerSession(cwd); if (!session) { return { found: false, exited: true, forced: false, reclaimedStaleEndpoint: false }; } @@ -416,7 +405,7 @@ async function shutdownBrokerSessionLocked(cwd, options = {}) { } let verifiedPid = ownershipVerified ? acknowledgedPid ?? pid : null; - let exited = isValidPid(pid) ? await waitForBrokerProcessExit(pid, options, 0) : false; + let exited = isValidPid(pid) ? await waitForProcessExit(pid, { ...options, timeoutMs: 0 }) : false; if (!shutdownAck && isValidPid(pid) && !exited) { const ownsPersistedProcess = processMatchesInstanceToken(pid, session.instanceToken, options); @@ -435,7 +424,7 @@ async function shutdownBrokerSessionLocked(cwd, options = {}) { } if (!exited && isValidPid(verifiedPid)) { - exited = await waitForBrokerProcessExit(verifiedPid, options, options.timeoutMs); + exited = await waitForProcessExit(verifiedPid, options); } let forced = false; @@ -450,7 +439,7 @@ async function shutdownBrokerSessionLocked(cwd, options = {}) { } options.killProcess(verifiedPid); forced = true; - exited = await waitForBrokerProcessExit(verifiedPid, options, options.timeoutMs); + exited = await waitForProcessExit(verifiedPid, options); } if (!exited) { @@ -481,17 +470,6 @@ async function shutdownBrokerSessionLocked(cwd, options = {}) { return { found: true, exited: true, forced, reclaimedStaleEndpoint }; } -async function isBrokerEndpointReady(endpoint) { - if (!endpoint) { - return false; - } - try { - return await waitForBrokerEndpoint(endpoint, 150); - } catch { - return false; - } -} - export async function ensureBrokerSession(cwd, options = {}) { return withBrokerLock(cwd, options, () => ensureBrokerSessionLocked(cwd, options)); } @@ -507,22 +485,17 @@ function withBrokerLock(cwd, options, action) { } async function ensureBrokerSessionLocked(cwd, options = {}) { + const shutdownOptions = { + ...options, + killProcess: options.killProcess ?? terminateProcessTree + }; const existing = loadBrokerSession(cwd); - if (existing && (await isBrokerEndpointReady(existing.endpoint))) { + if (existing?.endpoint && (await waitForBrokerEndpoint(existing.endpoint, 150))) { return existing; } if (existing) { - await shutdownBrokerSessionLocked(cwd, { - session: existing, - killProcess: options.killProcess ?? terminateProcessTree, - verifyProcess: options.verifyProcess, - runCommandImpl: options.runCommandImpl, - killImpl: options.killImpl, - platform: options.platform, - timeoutMs: options.timeoutMs, - intervalMs: options.intervalMs - }); + await shutdownBrokerSessionLocked(cwd, shutdownOptions); } const sessionDir = createBrokerSessionDir(); @@ -556,23 +529,14 @@ async function ensureBrokerSessionLocked(cwd, options = {}) { const ready = await waitForBrokerEndpoint(endpoint, options.timeoutMs ?? 2000); if (!ready) { - await shutdownBrokerSessionLocked(cwd, { - session, - killProcess: options.killProcess ?? terminateProcessTree, - verifyProcess: options.verifyProcess, - runCommandImpl: options.runCommandImpl, - killImpl: options.killImpl, - platform: options.platform, - timeoutMs: options.timeoutMs, - intervalMs: options.intervalMs - }); + await shutdownBrokerSessionLocked(cwd, shutdownOptions); return null; } return session; } -export function teardownBrokerSession({ +function teardownBrokerSession({ endpoint = null, pidFile, logFile, @@ -583,37 +547,28 @@ export function teardownBrokerSession({ throw new Error("Refusing to remove an unverified broker endpoint."); } - for (const filePath of [pidFile, logFile]) { - if (!filePath) { - continue; - } - try { - fs.unlinkSync(filePath); - } catch (error) { - if (error?.code !== "ENOENT") { - throw error; - } - } + const ownedSessionDir = resolveOwnedSessionDir(sessionDir); + const files = []; + if (ownedSessionDir && pidFile === path.join(ownedSessionDir, "broker.pid")) { + files.push(pidFile); } - - if (endpoint) { + if (ownedSessionDir && logFile === path.join(ownedSessionDir, "broker.log")) { + files.push(logFile); + } + if (ownedSessionDir && endpoint === createBrokerEndpoint(ownedSessionDir)) { const target = parseBrokerEndpoint(endpoint); if (target.kind === "unix") { - try { - fs.unlinkSync(target.path); - } catch (error) { - if (error?.code !== "ENOENT") { - throw error; - } - } + files.push(target.path); } } - const resolvedSessionDir = - sessionDir ?? (pidFile ? path.dirname(pidFile) : logFile ? path.dirname(logFile) : null); - if (resolvedSessionDir) { + for (const filePath of files) { + removeFileIfExists(filePath); + } + + if (ownedSessionDir) { try { - fs.rmdirSync(resolvedSessionDir); + fs.rmdirSync(ownedSessionDir); } catch (error) { if (error?.code !== "ENOENT" && error?.code !== "ENOTEMPTY") { throw error; diff --git a/plugins/codex/scripts/lib/fs.mjs b/plugins/codex/scripts/lib/fs.mjs index 89ce85253..730259831 100644 --- a/plugins/codex/scripts/lib/fs.mjs +++ b/plugins/codex/scripts/lib/fs.mjs @@ -32,10 +32,6 @@ export function readJsonFile(filePath) { return JSON.parse(fs.readFileSync(filePath, "utf8")); } -export function writeJsonFile(filePath, value) { - fs.writeFileSync(filePath, `${JSON.stringify(value, null, 2)}\n`, "utf8"); -} - export function writePrivateFile(filePath, value) { fs.writeFileSync(filePath, value, { encoding: "utf8", mode: PRIVATE_FILE_MODE }); setMode(filePath, PRIVATE_FILE_MODE); @@ -64,6 +60,19 @@ export function writeJsonFileAtomic(filePath, value) { } } +export function removeFileIfExists(filePath) { + if (!filePath) { + return; + } + try { + fs.unlinkSync(filePath); + } catch (error) { + if (error?.code !== "ENOENT") { + throw error; + } + } +} + export function safeReadFile(filePath) { return fs.existsSync(filePath) ? fs.readFileSync(filePath, "utf8") : ""; } diff --git a/plugins/codex/scripts/lib/locking.mjs b/plugins/codex/scripts/lib/locking.mjs index 309f091b7..849ad9b52 100644 --- a/plugins/codex/scripts/lib/locking.mjs +++ b/plugins/codex/scripts/lib/locking.mjs @@ -111,9 +111,11 @@ function readLockState(lockDir) { const owner = JSON.parse(fs.readFileSync(ownerFile, "utf8")); const expectedFileName = ownerFileName(owner.token); if ( - !Number.isFinite(owner.pid) || + !Number.isSafeInteger(owner.pid) || owner.pid <= 0 || typeof owner.token !== "string" || + owner.token.length === 0 || + (owner.processIdentity != null && typeof owner.processIdentity !== "string") || fileName !== expectedFileName ) { return { kind: "corrupt", ageMs }; @@ -185,18 +187,28 @@ function normalizeOptions(options) { }; } +// Returns a handle when the lock was acquired, null when the caller should +// sleep and retry, and throws once the deadline has passed. +function tryAcquireOnce(lockDir, normalized, deadline) { + const handle = tryAcquire(lockDir); + if (handle) { + return handle; + } + reclaimAbandonedLock(lockDir, normalized); + if (Date.now() >= deadline) { + throw new Error(`Timed out waiting for lock: ${lockDir}`); + } + return null; +} + export function acquireLockSync(lockDir, options = {}) { const normalized = normalizeOptions(options); const deadline = Date.now() + normalized.timeoutMs; while (true) { - const handle = tryAcquire(lockDir); + const handle = tryAcquireOnce(lockDir, normalized, deadline); if (handle) { return handle; } - reclaimAbandonedLock(lockDir, normalized); - if (Date.now() >= deadline) { - throw new Error(`Timed out waiting for lock: ${lockDir}`); - } sleepSync(normalized.retryDelayMs); } } @@ -205,14 +217,10 @@ export async function acquireLock(lockDir, options = {}) { const normalized = normalizeOptions(options); const deadline = Date.now() + normalized.timeoutMs; while (true) { - const handle = tryAcquire(lockDir); + const handle = tryAcquireOnce(lockDir, normalized, deadline); if (handle) { return handle; } - reclaimAbandonedLock(lockDir, normalized); - if (Date.now() >= deadline) { - throw new Error(`Timed out waiting for lock: ${lockDir}`); - } await sleep(normalized.retryDelayMs); } } diff --git a/plugins/codex/scripts/lib/process.mjs b/plugins/codex/scripts/lib/process.mjs index c3f31f358..3d0a0c37f 100644 --- a/plugins/codex/scripts/lib/process.mjs +++ b/plugins/codex/scripts/lib/process.mjs @@ -62,7 +62,7 @@ function looksLikeMissingProcessMessage(text) { return /not found|no running instance|cannot find|does not exist|no such process/i.test(text); } -function isValidPid(pid) { +export function isValidPid(pid) { return Number.isSafeInteger(pid) && pid > 0; } @@ -85,36 +85,37 @@ function readLinuxProcessStat(pid) { } } -export function getProcessIdentity(pid, options = {}) { - if (!isValidPid(pid)) { - return null; - } - const platform = options.platform ?? process.platform; - if (platform === "linux") { - return readLinuxProcessStat(pid)?.startTime ?? null; - } - +function queryProcessTable(pid, powershellCommand, psFormat, options) { const runCommandImpl = options.runCommandImpl ?? runCommand; + const spawnOptions = { timeout: options.timeoutMs ?? 2000, killSignal: "SIGTERM" }; const result = - platform === "win32" + (options.platform ?? process.platform) === "win32" ? runCommandImpl( "powershell.exe", - [ - "-NoProfile", - "-NonInteractive", - "-Command", - `$p = Get-Process -Id ${pid} -ErrorAction SilentlyContinue; if ($null -ne $p) { [Console]::Out.Write($p.StartTime.ToUniversalTime().Ticks) }` - ], - { timeout: options.timeoutMs ?? 2000, killSignal: "SIGTERM" } + ["-NoProfile", "-NonInteractive", "-Command", powershellCommand], + spawnOptions ) - : runCommandImpl("ps", ["-ww", "-p", String(pid), "-o", "lstart="], { - timeout: options.timeoutMs ?? 2000, - killSignal: "SIGTERM" - }); + : runCommandImpl("ps", ["-ww", "-p", String(pid), "-o", psFormat], spawnOptions); if (result.error || result.signal != null || result.status !== 0) { return null; } - return String(result.stdout ?? "").trim() || null; + return String(result.stdout ?? ""); +} + +export function getProcessIdentity(pid, options = {}) { + if (!isValidPid(pid)) { + return null; + } + if ((options.platform ?? process.platform) === "linux") { + return readLinuxProcessStat(pid)?.startTime ?? null; + } + const output = queryProcessTable( + pid, + `$p = Get-Process -Id ${pid} -ErrorAction SilentlyContinue; if ($null -ne $p) { [Console]::Out.Write($p.StartTime.ToUniversalTime().Ticks) }`, + "lstart=", + options + ); + return output?.trim() || null; } export function isProcessRunning(pid, options = {}) { @@ -228,28 +229,20 @@ export async function waitForProcessExit(pid, options = {}) { } function readProcessCommandLine(pid, options) { - const platform = options.platform ?? process.platform; - const runCommandImpl = options.runCommandImpl ?? runCommand; - const result = - platform === "win32" - ? runCommandImpl( - "powershell.exe", - [ - "-NoProfile", - "-NonInteractive", - "-Command", - `$p = Get-CimInstance Win32_Process -Filter 'ProcessId = ${pid}'; if ($null -ne $p) { [Console]::Out.Write($p.CommandLine) }` - ], - { timeout: options.timeoutMs ?? 2000, killSignal: "SIGTERM" } - ) - : runCommandImpl("ps", ["-ww", "-p", String(pid), "-o", "command="], { - timeout: options.timeoutMs ?? 2000, - killSignal: "SIGTERM" - }); - if (result.error || result.signal != null || result.status !== 0) { + return queryProcessTable( + pid, + `$p = Get-CimInstance Win32_Process -Filter 'ProcessId = ${pid}'; if ($null -ne $p) { [Console]::Out.Write($p.CommandLine) }`, + "command=", + options + ); +} + +function readLinuxCommandLineArgs(pid) { + try { + return fs.readFileSync(`/proc/${pid}/cmdline`, "utf8").split("\0"); + } catch { return null; } - return String(result.stdout ?? ""); } export function processHasLaunchSequence(pid, expectedArgs, options = {}) { @@ -262,12 +255,9 @@ export function processHasLaunchSequence(pid, expectedArgs, options = {}) { return false; } - const platform = options.platform ?? process.platform; - if (platform === "linux") { - let argv; - try { - argv = fs.readFileSync(`/proc/${pid}/cmdline`, "utf8").split("\0").filter(Boolean); - } catch { + if ((options.platform ?? process.platform) === "linux") { + const argv = readLinuxCommandLineArgs(pid)?.filter(Boolean); + if (!argv) { return false; } return argv.some((_, start) => @@ -296,14 +286,9 @@ export function processHasLaunchToken(pid, token, options = {}) { } const marker = options.marker ?? "--worker-token"; - const platform = options.platform ?? process.platform; - if (platform === "linux") { - try { - const argv = fs.readFileSync(`/proc/${pid}/cmdline`, "utf8").split("\0"); - return argv.includes(marker) && argv.includes(token); - } catch { - return false; - } + if ((options.platform ?? process.platform) === "linux") { + const argv = readLinuxCommandLineArgs(pid); + return argv != null && argv.includes(marker) && argv.includes(token); } const commandLine = readProcessCommandLine(pid, options); diff --git a/plugins/codex/scripts/lib/state.mjs b/plugins/codex/scripts/lib/state.mjs index 5bb85208d..f6fdf5e2a 100644 --- a/plugins/codex/scripts/lib/state.mjs +++ b/plugins/codex/scripts/lib/state.mjs @@ -5,6 +5,7 @@ import path from "node:path"; import { ensurePrivateDir, + removeFileIfExists, writeJsonFileAtomic, writePrivateFile } from "./fs.mjs"; @@ -93,12 +94,6 @@ function pruneJobs(jobs) { .slice(0, MAX_JOBS); } -function removeFileIfExists(filePath) { - if (filePath && fs.existsSync(filePath)) { - fs.unlinkSync(filePath); - } -} - function resolveStateLockDir(cwd) { return path.join(resolveStateDir(cwd), ".state.lock"); } @@ -120,7 +115,7 @@ function saveStateLocked(cwd, state) { if (retainedIds.has(job.id)) { continue; } - removeJobFile(resolveJobFile(cwd, job.id)); + removeFileIfExists(resolveJobFile(cwd, job.id)); removeFileIfExists(job.logFile); } @@ -195,12 +190,6 @@ export function readJobFile(jobFile) { return JSON.parse(fs.readFileSync(jobFile, "utf8")); } -function removeJobFile(jobFile) { - if (fs.existsSync(jobFile)) { - fs.unlinkSync(jobFile); - } -} - export function resolveJobLogFile(cwd, jobId) { ensureStateDir(cwd); return path.join(resolveJobsDir(cwd), `${jobId}.log`); diff --git a/plugins/codex/scripts/session-lifecycle-hook.mjs b/plugins/codex/scripts/session-lifecycle-hook.mjs index 82c01354c..fa4ee67fa 100644 --- a/plugins/codex/scripts/session-lifecycle-hook.mjs +++ b/plugins/codex/scripts/session-lifecycle-hook.mjs @@ -85,7 +85,7 @@ async function handleSessionEnd(input) { } if (failures.length > 0) { - throw new AggregateError(failures, "Codex session cleanup did not finish."); + throw new AggregateError(failures, `Codex session cleanup did not finish: ${failures.join("; ")}`); } } diff --git a/tests/broker-lifecycle.test.mjs b/tests/broker-lifecycle.test.mjs index c78c096b5..0c385074f 100644 --- a/tests/broker-lifecycle.test.mjs +++ b/tests/broker-lifecycle.test.mjs @@ -48,7 +48,6 @@ test("concurrent ensureBrokerSession calls share a single authenticated broker", assert.equal(persisted.instanceToken, left.instanceToken); const outcome = await shutdownBrokerSession(workspace, { - session: left, killProcess: terminateProcessTree }); assert.equal(outcome.exited, true); @@ -102,7 +101,6 @@ test("broker rejects a shutdown token that does not identify its instance", asyn t.after(async () => { if (loadBrokerSession(workspace)) { await shutdownBrokerSession(workspace, { - session, killProcess: terminateProcessTree }); } @@ -118,7 +116,34 @@ test("broker rejects a shutdown token that does not identify its instance", asyn assert.ok(loadBrokerSession(workspace)); }); -test("shutdown request stops waiting at its deadline", { skip: process.platform === "win32" }, async (t) => { +test("shutdown closes idle half-open broker clients", { skip: process.platform === "win32" }, async (t) => { + const workspace = makeTempDir(); + const binDir = makeTempDir(); + installFakeCodex(binDir); + const session = await ensureBrokerSession(workspace, { env: buildEnv(binDir) }); + const idleClient = net.createConnection({ + path: session.endpoint.slice("unix:".length), + allowHalfOpen: true + }); + idleClient.on("end", () => {}); + await new Promise((resolve, reject) => { + idleClient.once("connect", resolve); + idleClient.once("error", reject); + }); + t.after(() => idleClient.destroy()); + + const outcome = await shutdownBrokerSession(workspace, { + timeoutMs: 500, + intervalMs: 10, + killProcess: terminateProcessTree + }); + + assert.equal(outcome.exited, true); + assert.equal(outcome.forced, false); + assert.equal(loadBrokerSession(workspace), null); +}); + +test("shutdown request always uses a finite deadline", { skip: process.platform === "win32" }, async (t) => { const sessionDir = makeTempDir("cxc-unresponsive-"); const socketPath = path.join(sessionDir, "broker.sock"); const sockets = new Set(); @@ -137,14 +162,15 @@ test("shutdown request stops waiting at its deadline", { skip: process.platform await new Promise((resolve) => server.close(resolve)); }); - const startedAt = Date.now(); - const response = await sendBrokerShutdown(`unix:${socketPath}`, { - instanceToken: "instance-token-1234567890", - timeoutMs: 40 - }); - - assert.equal(response, null); - assert.ok(Date.now() - startedAt < 500, "shutdown request exceeded its deadline"); + for (const timeoutMs of [0, 40]) { + const startedAt = Date.now(); + const response = await sendBrokerShutdown(`unix:${socketPath}`, { + instanceToken: "instance-token-1234567890", + timeoutMs + }); + assert.equal(response, null); + assert.ok(Date.now() - startedAt < 500, "shutdown request exceeded its deadline"); + } assert.equal(fs.existsSync(socketPath), true); }); @@ -172,7 +198,6 @@ test("shutdown preserves an unowned endpoint and its persisted state", { skip: p await assert.rejects( shutdownBrokerSession(workspace, { - session, timeoutMs: 40, killProcess: terminateProcessTree }), @@ -183,6 +208,51 @@ test("shutdown preserves an unowned endpoint and its persisted state", { skip: p assert.deepEqual(loadBrokerSession(workspace), session); }); +test("shutdown never removes artifacts outside a private broker session", async () => { + const workspace = makeTempDir(); + const externalDir = makeTempDir("not-a-broker-session-"); + const externalFile = path.join(externalDir, "preserve.txt"); + fs.writeFileSync(externalFile, "preserve me\n"); + saveBrokerSession(workspace, { + endpoint: `unix:${path.join(externalDir, "missing.sock")}`, + pid: null, + pidFile: externalFile, + logFile: null, + sessionDir: externalDir + }); + + const outcome = await shutdownBrokerSession(workspace); + + assert.equal(outcome.exited, true); + assert.equal(fs.readFileSync(externalFile, "utf8"), "preserve me\n"); + assert.equal(loadBrokerSession(workspace), null); +}); + +test("shutdown never follows a private-session symlink", { skip: process.platform === "win32" }, async (t) => { + const workspace = makeTempDir(); + const externalDir = makeTempDir("not-a-broker-session-"); + const externalPidFile = path.join(externalDir, "broker.pid"); + const sessionLink = makeTempDir("cxc-"); + fs.writeFileSync(externalPidFile, "preserve me\n"); + fs.rmdirSync(sessionLink); + fs.symlinkSync(externalDir, sessionLink, "dir"); + t.after(() => fs.unlinkSync(sessionLink)); + + saveBrokerSession(workspace, { + endpoint: `unix:${path.join(sessionLink, "missing.sock")}`, + pid: null, + pidFile: path.join(sessionLink, "broker.pid"), + logFile: null, + sessionDir: sessionLink + }); + + const outcome = await shutdownBrokerSession(workspace); + + assert.equal(outcome.exited, true); + assert.equal(fs.readFileSync(externalPidFile, "utf8"), "preserve me\n"); + assert.equal(loadBrokerSession(workspace), null); +}); + test("shutdown retires a live tokenless broker left by the previous version", { skip: process.platform === "win32" }, async (t) => { const workspace = makeTempDir(); const sessionDir = makeTempDir("cxc-legacy-"); @@ -247,7 +317,6 @@ test("shutdown retires a live tokenless broker left by the previous version", { saveBrokerSession(workspace, session); const outcome = await shutdownBrokerSession(workspace, { - session, timeoutMs: 500, intervalMs: 10, killProcess: terminateProcessTree @@ -268,7 +337,6 @@ test("shutdown does not retire an authenticated broker whose persisted token was t.after(async () => { saveBrokerSession(workspace, authenticatedSession); await shutdownBrokerSession(workspace, { - session: authenticatedSession, killProcess: terminateProcessTree }); }); @@ -278,7 +346,6 @@ test("shutdown does not retire an authenticated broker whose persisted token was await assert.rejects( shutdownBrokerSession(workspace, { - session: tokenlessState, timeoutMs: 200, killProcess: terminateProcessTree }), @@ -304,7 +371,6 @@ test("broker metadata, pid, and log files are private", { skip: process.platform assert.equal(fs.statSync(persistedStateFile).mode & 0o777, 0o600); await shutdownBrokerSession(workspace, { - session, killProcess: terminateProcessTree }); }); @@ -357,7 +423,6 @@ test("shutdown reclaims the socket of an owned broker that died without acking", saveBrokerSession(workspace, session); const outcome = await shutdownBrokerSession(workspace, { - session, timeoutMs: 40, killProcess: terminateProcessTree }); @@ -392,7 +457,7 @@ test("ensureBrokerSession recovers from a stale socket instead of failing", { sk const session = await ensureBrokerSession(workspace, { env: buildEnv(binDir) }); t.after(async () => { if (session) { - await shutdownBrokerSession(workspace, { session, killProcess: terminateProcessTree }); + await shutdownBrokerSession(workspace, { killProcess: terminateProcessTree }); } }); @@ -445,7 +510,6 @@ test("shutdown still refuses to unlink an endpoint someone is listening on", { s await assert.rejects( shutdownBrokerSession(workspace, { - session, timeoutMs: 40, killProcess: terminateProcessTree }), @@ -483,7 +547,6 @@ test("a refused endpoint alone does not justify reclaiming while the owner lives await assert.rejects( shutdownBrokerSession(workspace, { - session, timeoutMs: 40, killProcess: () => {} }), @@ -520,7 +583,6 @@ test("an endpoint outside the plugin session directory is never reclaimed", { sk await assert.rejects( shutdownBrokerSession(workspace, { - session, timeoutMs: 40, killProcess: terminateProcessTree }), diff --git a/tests/helpers.mjs b/tests/helpers.mjs index 6b110ee56..ee5ea89d4 100644 --- a/tests/helpers.mjs +++ b/tests/helpers.mjs @@ -33,7 +33,6 @@ after(async () => { } try { await shutdownBrokerSession(dir, { - session, killProcess: terminateProcessTree, timeoutMs: 1000, intervalMs: 25 diff --git a/tests/locking.test.mjs b/tests/locking.test.mjs index 300fe270b..37eba6e4b 100644 --- a/tests/locking.test.mjs +++ b/tests/locking.test.mjs @@ -96,6 +96,26 @@ test("unknown lock contents fail closed", () => { assert.equal(fs.readFileSync(path.join(lockDir, "unexpected"), "utf8"), "do not delete\n"); }); +test("malformed lock owners fail closed", () => { + const owners = [ + { pid: 1.5, token: "fractional-pid", processIdentity: null }, + { pid: process.pid, token: "object-identity", processIdentity: {} } + ]; + + for (const owner of owners) { + const lockDir = path.join(makeTempDir(), "state.lock"); + const ownerFile = path.join(lockDir, `owner-${owner.token}.json`); + fs.mkdirSync(lockDir); + fs.writeFileSync(ownerFile, `${JSON.stringify(owner)}\n`); + + assert.throws( + () => acquireLockSync(lockDir, { timeoutMs: 30, staleMs: 0, retryDelayMs: 5 }), + /Timed out waiting for lock/ + ); + assert.equal(fs.existsSync(ownerFile), true); + } +}); + test("stale legacy lock without process identity does not follow a reused PID forever", () => { const lockDir = path.join(makeTempDir(), "state.lock"); const token = "legacy-owner"; diff --git a/tests/runtime.test.mjs b/tests/runtime.test.mjs index f78dc2cfb..1ef209d8a 100644 --- a/tests/runtime.test.mjs +++ b/tests/runtime.test.mjs @@ -1923,6 +1923,31 @@ test("session end fully cleans up jobs for the ending session", async (t) => { assert.equal(otherJob.logFile, otherSessionLog); }); +test("session end reports the underlying cleanup failure", (t) => { + const repo = makeTempDir(); + const brokerStateFile = path.join(resolveStateDir(repo), "broker.json"); + t.after(() => fs.rmSync(brokerStateFile, { force: true })); + saveBrokerSession(repo, { + endpoint: "unsupported:broker", + pid: process.pid, + pidFile: null, + logFile: null, + sessionDir: null, + instanceToken: "instance-token-1234567890" + }); + + const result = run("node", [SESSION_HOOK, "SessionEnd"], { + cwd: repo, + input: JSON.stringify({ + hook_event_name: "SessionEnd", + cwd: repo + }) + }); + + assert.equal(result.status, 1); + assert.match(result.stderr, /Unsupported broker endpoint: unsupported:broker/); +}); + test("stop hook runs a stop-time review task and blocks on findings when the review gate is enabled", () => { const repo = makeTempDir(); const binDir = makeTempDir(); From 7f69f4aea77614acb7f09b5dab0923eb1f5bc4d1 Mon Sep 17 00:00:00 2001 From: Heringer Date: Sat, 25 Jul 2026 07:12:18 -0300 Subject: [PATCH 10/20] Keep Linux processes with unreadable /proc metadata running kill(pid, 0) succeeding proves the process exists; a null /proc stat only means inspection failed. Reporting such a process as exited let a stale-lock reclaim remove a live owner's lock. Fail conservative instead: the process stays running until an explicit missing-process signal. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_018dXaCHsST2XTpdu63v4Ty4 --- plugins/codex/scripts/lib/process.mjs | 20 +++++++++++--------- tests/process.test.mjs | 13 +++++++++++++ 2 files changed, 24 insertions(+), 9 deletions(-) diff --git a/plugins/codex/scripts/lib/process.mjs b/plugins/codex/scripts/lib/process.mjs index 3d0a0c37f..f421e08c1 100644 --- a/plugins/codex/scripts/lib/process.mjs +++ b/plugins/codex/scripts/lib/process.mjs @@ -140,15 +140,17 @@ export function isProcessRunning(pid, options = {}) { if (platform === "linux") { const readProcessStat = options.readProcessStat ?? readLinuxProcessStat; const stat = readProcessStat(pid); - if (!stat) { - return false; - } - // Zombies still answer kill(pid, 0), but no longer own a live resource. - if (stat.state === "Z" || stat.state === "X") { - return false; - } - if (options.identity != null && stat.startTime !== String(options.identity)) { - return false; + // A null stat means /proc could not be inspected even though kill(pid, 0) + // proved the process exists. Failure to inspect is not proof of exit or + // replacement, so stay conservative and keep reporting it as running. + if (stat) { + // Zombies still answer kill(pid, 0), but no longer own a live resource. + if (stat.state === "Z" || stat.state === "X") { + return false; + } + if (options.identity != null && stat.startTime !== String(options.identity)) { + return false; + } } } else if (options.identity != null) { const currentIdentity = getProcessIdentity(pid, options); diff --git a/tests/process.test.mjs b/tests/process.test.mjs index 2909e53f1..d99bf37d4 100644 --- a/tests/process.test.mjs +++ b/tests/process.test.mjs @@ -39,6 +39,19 @@ test("Linux zombie processes are treated as exited even when signal 0 succeeds", assert.equal(running, false); }); +test("Linux processes with unreadable /proc metadata stay running", () => { + const running = isProcessRunning(1234, { + platform: "linux", + identity: "42", + killImpl() {}, + readProcessStat() { + return null; + } + }); + + assert.equal(running, true); +}); + test("macOS process identity uses the process start time", () => { let captured = null; const identity = getProcessIdentity(1234, { From f923bbfcd77c02317d4bc427b33770c6228abb17 Mon Sep 17 00:00:00 2001 From: Heringer Date: Sat, 25 Jul 2026 07:31:12 -0300 Subject: [PATCH 11/20] Tighten remaining duplication in broker entry points - Register the SIGTERM/SIGINT shutdown handler in one loop. - Reuse removeFileIfExists for broker socket/pid cleanup, closing the exists/unlink race. - loadBrokerSession relies on its catch instead of a pre-existence check. - canDiscardUnownedSession forwards caller options to the process-tree check like every other ownership probe. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_018dXaCHsST2XTpdu63v4Ty4 --- plugins/codex/scripts/app-server-broker.mjs | 25 ++++++++----------- .../codex/scripts/lib/broker-lifecycle.mjs | 13 +++------- 2 files changed, 14 insertions(+), 24 deletions(-) diff --git a/plugins/codex/scripts/app-server-broker.mjs b/plugins/codex/scripts/app-server-broker.mjs index 696550c61..17d50a85d 100644 --- a/plugins/codex/scripts/app-server-broker.mjs +++ b/plugins/codex/scripts/app-server-broker.mjs @@ -8,7 +8,7 @@ import process from "node:process"; import { parseArgs } from "./lib/args.mjs"; import { BROKER_BUSY_RPC_CODE, CodexAppServerClient } from "./lib/app-server.mjs"; import { parseBrokerEndpoint } from "./lib/broker-endpoint.mjs"; -import { ensurePrivateDir, writePrivateFile } from "./lib/fs.mjs"; +import { ensurePrivateDir, removeFileIfExists, writePrivateFile } from "./lib/fs.mjs"; const STREAMING_METHODS = new Set(["turn/start", "review/start", "thread/compact/start"]); @@ -116,12 +116,10 @@ async function main() { } await appClient.close().catch(() => {}); await new Promise((resolve) => server.close(resolve)); - if (listenTarget.kind === "unix" && fs.existsSync(listenTarget.path)) { - fs.unlinkSync(listenTarget.path); - } - if (pidFile && fs.existsSync(pidFile)) { - fs.unlinkSync(pidFile); + if (listenTarget.kind === "unix") { + removeFileIfExists(listenTarget.path); } + removeFileIfExists(pidFile); } appClient.setNotificationHandler(routeNotification); @@ -254,15 +252,12 @@ async function main() { }); }); - process.on("SIGTERM", async () => { - await shutdown(server); - process.exit(0); - }); - - process.on("SIGINT", async () => { - await shutdown(server); - process.exit(0); - }); + for (const signal of ["SIGTERM", "SIGINT"]) { + process.on(signal, async () => { + await shutdown(server); + process.exit(0); + }); + } server.listen(listenTarget.path); } diff --git a/plugins/codex/scripts/lib/broker-lifecycle.mjs b/plugins/codex/scripts/lib/broker-lifecycle.mjs index f70c5c970..b28b153a1 100644 --- a/plugins/codex/scripts/lib/broker-lifecycle.mjs +++ b/plugins/codex/scripts/lib/broker-lifecycle.mjs @@ -177,13 +177,8 @@ function resolveBrokerStateFile(cwd) { } export function loadBrokerSession(cwd) { - const stateFile = resolveBrokerStateFile(cwd); - if (!fs.existsSync(stateFile)) { - return null; - } - try { - return JSON.parse(fs.readFileSync(stateFile, "utf8")); + return JSON.parse(fs.readFileSync(resolveBrokerStateFile(cwd), "utf8")); } catch { return null; } @@ -228,8 +223,8 @@ function endpointArtifactExists(endpoint) { } } -function canDiscardUnownedSession(session, pid) { - const processExited = !isValidPid(pid) || !isProcessTreeRunning(pid); +function canDiscardUnownedSession(session, pid, options = {}) { + const processExited = !isValidPid(pid) || !isProcessTreeRunning(pid, options); return processExited && !endpointArtifactExists(session.endpoint); } @@ -359,7 +354,7 @@ async function shutdownBrokerSessionLocked(cwd, options = {}) { const legacySession = Boolean(session.endpoint && !session.instanceToken); let legacyProcessVerified = false; if (legacySession) { - if (canDiscardUnownedSession(session, pid)) { + if (canDiscardUnownedSession(session, pid, options)) { teardownBrokerSession({ endpoint: null, pidFile: session.pidFile ?? null, From b5362a740bba73b80848d30b00771aad87256ca6 Mon Sep 17 00:00:00 2001 From: Heringer Date: Sat, 25 Jul 2026 07:38:34 -0300 Subject: [PATCH 12/20] Verify legacy brokers by session artifacts, not launch cwd A tokenless broker may have been launched with a --cwd that addressed the same workspace through a different path (subdirectory, symlink). Requiring the current invocation path to match that argument made ownership verification fail and wedged the workspace. Prove ownership with the launch artifacts unique to the session instead: its endpoint and its pid file inside the mkdtemp session directory. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_018dXaCHsST2XTpdu63v4Ty4 --- .../codex/scripts/lib/broker-lifecycle.mjs | 32 +++++------- tests/broker-lifecycle.test.mjs | 51 ++++++++++++++++--- 2 files changed, 58 insertions(+), 25 deletions(-) diff --git a/plugins/codex/scripts/lib/broker-lifecycle.mjs b/plugins/codex/scripts/lib/broker-lifecycle.mjs index b28b153a1..80c2b3d8c 100644 --- a/plugins/codex/scripts/lib/broker-lifecycle.mjs +++ b/plugins/codex/scripts/lib/broker-lifecycle.mjs @@ -271,7 +271,7 @@ function endpointBelongsToSession(session, platform = process.platform) { } } -function processMatchesLegacyBroker(cwd, session, pid, options = {}) { +function processMatchesLegacyBroker(session, pid, options = {}) { const platform = options.platform ?? process.platform; let expectedEndpoint; try { @@ -287,22 +287,18 @@ function processMatchesLegacyBroker(cwd, session, pid, options = {}) { ) { return false; } - return processHasLaunchSequence( - pid, - [ - "serve", - "--endpoint", - session.endpoint, - "--cwd", - cwd, - "--pid-file", - session.pidFile - ], - { - platform, - timeoutMs: options.timeoutMs, - runCommandImpl: options.runCommandImpl - } + const probeOptions = { + platform, + timeoutMs: options.timeoutMs, + runCommandImpl: options.runCommandImpl + }; + // The broker's original --cwd argument is not persisted, and the current + // invocation may address the same workspace through a different path, so + // ownership is proven by the launch artifacts unique to this session: its + // endpoint and its pid file inside the mkdtemp session directory. + return ( + processHasLaunchSequence(pid, ["serve", "--endpoint", session.endpoint], probeOptions) && + processHasLaunchSequence(pid, ["--pid-file", session.pidFile], probeOptions) ); } @@ -364,7 +360,7 @@ async function shutdownBrokerSessionLocked(cwd, options = {}) { clearBrokerSession(cwd); return { found: true, exited: true, forced: false, reclaimedStaleEndpoint: false }; } - legacyProcessVerified = processMatchesLegacyBroker(cwd, session, pid, options); + legacyProcessVerified = processMatchesLegacyBroker(session, pid, options); const legacyEndpointIsSafelyStale = isValidPid(pid) && (await canReclaimStaleEndpoint(session, pid, options)); if (!legacyProcessVerified && !legacyEndpointIsSafelyStale) { diff --git a/tests/broker-lifecycle.test.mjs b/tests/broker-lifecycle.test.mjs index 0c385074f..2851aa447 100644 --- a/tests/broker-lifecycle.test.mjs +++ b/tests/broker-lifecycle.test.mjs @@ -253,12 +253,7 @@ test("shutdown never follows a private-session symlink", { skip: process.platfor assert.equal(loadBrokerSession(workspace), null); }); -test("shutdown retires a live tokenless broker left by the previous version", { skip: process.platform === "win32" }, async (t) => { - const workspace = makeTempDir(); - const sessionDir = makeTempDir("cxc-legacy-"); - const socketPath = path.join(sessionDir, "broker.sock"); - const pidFile = path.join(sessionDir, "broker.pid"); - const endpoint = `unix:${socketPath}`; +async function spawnLegacyBroker(t, { socketPath, pidFile, endpoint, cwdArg }) { const child = spawn( process.execPath, [ @@ -286,7 +281,7 @@ test("shutdown retires a live tokenless broker left by the previous version", { "--endpoint", endpoint, "--cwd", - workspace, + cwdArg, "--pid-file", pidFile ], @@ -306,6 +301,16 @@ test("shutdown retires a live tokenless broker left by the previous version", { terminateProcessTree(child.pid); } }); + return child; +} + +test("shutdown retires a live tokenless broker left by the previous version", { skip: process.platform === "win32" }, async (t) => { + const workspace = makeTempDir(); + const sessionDir = makeTempDir("cxc-legacy-"); + const socketPath = path.join(sessionDir, "broker.sock"); + const pidFile = path.join(sessionDir, "broker.pid"); + const endpoint = `unix:${socketPath}`; + const child = await spawnLegacyBroker(t, { socketPath, pidFile, endpoint, cwdArg: workspace }); const session = { endpoint, @@ -328,6 +333,38 @@ test("shutdown retires a live tokenless broker left by the previous version", { assert.equal(fs.existsSync(socketPath), false); }); +test("shutdown retires a legacy broker launched from a different workspace path", { skip: process.platform === "win32" }, async (t) => { + const workspace = makeTempDir(); + const sessionDir = makeTempDir("cxc-legacy-"); + const socketPath = path.join(sessionDir, "broker.sock"); + const pidFile = path.join(sessionDir, "broker.pid"); + const endpoint = `unix:${socketPath}`; + // The previous plugin version may have launched the broker with a --cwd + // that addressed this workspace through another path (subdirectory, + // symlink). Ownership must not depend on the current invocation path. + const originalCwd = path.join(workspace, "nested", "launch-dir"); + fs.mkdirSync(originalCwd, { recursive: true }); + const child = await spawnLegacyBroker(t, { socketPath, pidFile, endpoint, cwdArg: originalCwd }); + + saveBrokerSession(workspace, { + endpoint, + pid: child.pid, + pidFile, + logFile: null, + sessionDir + }); + + const outcome = await shutdownBrokerSession(workspace, { + timeoutMs: 500, + intervalMs: 10, + killProcess: terminateProcessTree + }); + + assert.equal(outcome.exited, true); + assert.equal(loadBrokerSession(workspace), null); + assert.equal(fs.existsSync(socketPath), false); +}); + test("shutdown does not retire an authenticated broker whose persisted token was lost", { skip: process.platform === "win32" }, async (t) => { const workspace = makeTempDir(); const binDir = makeTempDir(); From 422fe240d67464d87a6e6c0d275ea0dd86ddfd66 Mon Sep 17 00:00:00 2001 From: Heringer Date: Sat, 25 Jul 2026 07:43:51 -0300 Subject: [PATCH 13/20] Honor verified legacy ownership during forced shutdown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A verified tokenless broker that stops acknowledging shutdown was funneled into instance-token verification, which legacy sessions can never pass, so cleanup threw and the workspace stayed wedged on the stale broker. Re-verify ownership by the method that fits the session — launch artifacts for legacy brokers, the instance token otherwise — both before trusting the persisted PID and immediately before force-killing it. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_018dXaCHsST2XTpdu63v4Ty4 --- .../codex/scripts/lib/broker-lifecycle.mjs | 16 +++++--- tests/broker-lifecycle.test.mjs | 39 ++++++++++++++++++- 2 files changed, 47 insertions(+), 8 deletions(-) diff --git a/plugins/codex/scripts/lib/broker-lifecycle.mjs b/plugins/codex/scripts/lib/broker-lifecycle.mjs index 80c2b3d8c..c216a6441 100644 --- a/plugins/codex/scripts/lib/broker-lifecycle.mjs +++ b/plugins/codex/scripts/lib/broker-lifecycle.mjs @@ -336,6 +336,14 @@ function processMatchesInstanceToken(pid, instanceToken, options) { : processHasLaunchToken(pid, instanceToken, { ...options, marker: "--instance-token" }); } +// Legacy sessions have no instance token, so their processes are re-verified +// by launch artifacts; tokened sessions are re-verified by the token. +function ownsBrokerProcess(session, pid, legacySession, options) { + return legacySession + ? processMatchesLegacyBroker(session, pid, options) + : processMatchesInstanceToken(pid, session.instanceToken, options); +} + export async function shutdownBrokerSession(cwd, options = {}) { return withBrokerLock(cwd, options, () => shutdownBrokerSessionLocked(cwd, options)); } @@ -399,7 +407,7 @@ async function shutdownBrokerSessionLocked(cwd, options = {}) { let exited = isValidPid(pid) ? await waitForProcessExit(pid, { ...options, timeoutMs: 0 }) : false; if (!shutdownAck && isValidPid(pid) && !exited) { - const ownsPersistedProcess = processMatchesInstanceToken(pid, session.instanceToken, options); + const ownsPersistedProcess = ownsBrokerProcess(session, pid, legacySession, options); if (!ownsPersistedProcess) { if (isProcessTreeRunning(pid, options)) { throw new Error("Codex app-server broker ownership could not be verified; persisted state was preserved."); @@ -420,11 +428,7 @@ async function shutdownBrokerSessionLocked(cwd, options = {}) { let forced = false; if (!exited && isValidPid(verifiedPid) && options.killProcess) { - const stillOwnsProcess = processMatchesInstanceToken( - verifiedPid, - session.instanceToken, - options - ); + const stillOwnsProcess = ownsBrokerProcess(session, verifiedPid, legacySession, options); if (!stillOwnsProcess) { throw new Error("Codex app-server broker process ownership changed before forced shutdown."); } diff --git a/tests/broker-lifecycle.test.mjs b/tests/broker-lifecycle.test.mjs index 2851aa447..69ae7278d 100644 --- a/tests/broker-lifecycle.test.mjs +++ b/tests/broker-lifecycle.test.mjs @@ -253,7 +253,7 @@ test("shutdown never follows a private-session symlink", { skip: process.platfor assert.equal(loadBrokerSession(workspace), null); }); -async function spawnLegacyBroker(t, { socketPath, pidFile, endpoint, cwdArg }) { +async function spawnLegacyBroker(t, { socketPath, pidFile, endpoint, cwdArg, ackShutdown = true }) { const child = spawn( process.execPath, [ @@ -262,6 +262,7 @@ async function spawnLegacyBroker(t, { socketPath, pidFile, endpoint, cwdArg }) { const net = require("node:net"); const socketPath = ${JSON.stringify(socketPath)}; const pidFile = ${JSON.stringify(pidFile)}; + const ackShutdown = ${JSON.stringify(ackShutdown)}; fs.writeFileSync(pidFile, String(process.pid)); const server = net.createServer((socket) => { socket.setEncoding("utf8"); @@ -270,7 +271,7 @@ async function spawnLegacyBroker(t, { socketPath, pidFile, endpoint, cwdArg }) { buffer += chunk; if (!buffer.includes("\\n")) return; const request = JSON.parse(buffer.slice(0, buffer.indexOf("\\n"))); - if (request.method !== "broker/shutdown") return; + if (request.method !== "broker/shutdown" || !ackShutdown) return; socket.end(JSON.stringify({ id: request.id, result: {} }) + "\\n", () => { server.close(() => process.exit(0)); }); @@ -365,6 +366,40 @@ test("shutdown retires a legacy broker launched from a different workspace path" assert.equal(fs.existsSync(socketPath), false); }); +test("shutdown force-kills a verified legacy broker that ignores shutdown requests", { skip: process.platform === "win32" }, async (t) => { + const workspace = makeTempDir(); + const sessionDir = makeTempDir("cxc-legacy-"); + const socketPath = path.join(sessionDir, "broker.sock"); + const pidFile = path.join(sessionDir, "broker.pid"); + const endpoint = `unix:${socketPath}`; + const child = await spawnLegacyBroker(t, { + socketPath, + pidFile, + endpoint, + cwdArg: workspace, + ackShutdown: false + }); + + saveBrokerSession(workspace, { + endpoint, + pid: child.pid, + pidFile, + logFile: null, + sessionDir + }); + + const outcome = await shutdownBrokerSession(workspace, { + timeoutMs: 500, + intervalMs: 10, + killProcess: terminateProcessTree + }); + + assert.equal(outcome.exited, true); + assert.equal(outcome.forced, true); + assert.equal(loadBrokerSession(workspace), null); + assert.equal(fs.existsSync(socketPath), false); +}); + test("shutdown does not retire an authenticated broker whose persisted token was lost", { skip: process.platform === "win32" }, async (t) => { const workspace = makeTempDir(); const binDir = makeTempDir(); From 958e7d390b50928c34b46c3f1c968a532bbfb0e6 Mon Sep 17 00:00:00 2001 From: Heringer Date: Sat, 25 Jul 2026 07:51:56 -0300 Subject: [PATCH 14/20] Trust proven process ownership for endpoint cleanup Ownership proven against the live process (instance token or legacy launch artifacts) already covers the session endpoint, so skip the post-kill connection probe, which can race the kernel right after a forced termination and wrongly refuse cleanup. The probe remains for leftovers whose owner was never verified. Also raise the SessionEnd hook timeout to 30s: an unresponsive broker can legitimately take ~2s each for the shutdown RPC, the exit wait, and the post-kill wait, plus up to 10s waiting on the broker lock, so the previous 5s ceiling could kill the hook mid-teardown and leave the orphan it was meant to clean up. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_018dXaCHsST2XTpdu63v4Ty4 --- plugins/codex/hooks/hooks.json | 2 +- plugins/codex/scripts/lib/broker-lifecycle.mjs | 12 +++++++++--- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/plugins/codex/hooks/hooks.json b/plugins/codex/hooks/hooks.json index 19e33b818..1917ac986 100644 --- a/plugins/codex/hooks/hooks.json +++ b/plugins/codex/hooks/hooks.json @@ -18,7 +18,7 @@ { "type": "command", "command": "node \"${CLAUDE_PLUGIN_ROOT}/scripts/session-lifecycle-hook.mjs\" SessionEnd", - "timeout": 5 + "timeout": 30 } ] } diff --git a/plugins/codex/scripts/lib/broker-lifecycle.mjs b/plugins/codex/scripts/lib/broker-lifecycle.mjs index c216a6441..c95d3c313 100644 --- a/plugins/codex/scripts/lib/broker-lifecycle.mjs +++ b/plugins/codex/scripts/lib/broker-lifecycle.mjs @@ -406,6 +406,7 @@ async function shutdownBrokerSessionLocked(cwd, options = {}) { let verifiedPid = ownershipVerified ? acknowledgedPid ?? pid : null; let exited = isValidPid(pid) ? await waitForProcessExit(pid, { ...options, timeoutMs: 0 }) : false; + let processOwnershipProven = false; if (!shutdownAck && isValidPid(pid) && !exited) { const ownsPersistedProcess = ownsBrokerProcess(session, pid, legacySession, options); if (!ownsPersistedProcess) { @@ -415,6 +416,7 @@ async function shutdownBrokerSessionLocked(cwd, options = {}) { exited = true; } else { verifiedPid = pid; + processOwnershipProven = true; } } @@ -432,6 +434,7 @@ async function shutdownBrokerSessionLocked(cwd, options = {}) { if (!stillOwnsProcess) { throw new Error("Codex app-server broker process ownership changed before forced shutdown."); } + processOwnershipProven = true; options.killProcess(verifiedPid); forced = true; exited = await waitForProcessExit(verifiedPid, options); @@ -444,16 +447,19 @@ async function shutdownBrokerSessionLocked(cwd, options = {}) { // ack, so ownershipVerified stays false while the socket file survives. Left // fatal, that single stale socket wedges every later command in the workspace, // because ensureBrokerSession() shuts the old session down before starting a - // replacement. Reclaim it when it is provably dead instead of throwing. + // replacement. Ownership proven against the live process (token or launch + // artifacts) already covers the endpoint; only an unproven leftover needs the + // connection probe, which can race the kernel right after a forced kill. + const endpointProven = ownershipVerified || processOwnershipProven; let reclaimedStaleEndpoint = false; - if (!ownershipVerified && endpointArtifactExists(session.endpoint)) { + if (!endpointProven && endpointArtifactExists(session.endpoint)) { reclaimedStaleEndpoint = await canReclaimStaleEndpoint(session, pid, options); if (!reclaimedStaleEndpoint) { throw new Error("Codex app-server broker endpoint ownership could not be verified; persisted state was preserved."); } } - const endpointIsOurs = ownershipVerified || reclaimedStaleEndpoint; + const endpointIsOurs = endpointProven || reclaimedStaleEndpoint; teardownBrokerSession({ endpoint: endpointIsOurs ? session.endpoint ?? null : null, pidFile: session.pidFile ?? null, From df3b4601e3ec46d68e9411038ce12d60e4c27808 Mon Sep 17 00:00:00 2001 From: Heringer Date: Sat, 25 Jul 2026 10:17:38 -0300 Subject: [PATCH 15/20] Consolidate broker teardown and defer process identity lookup Extract teardownAndClear() for the two teardown+clear call sites in shutdownBrokerSessionLocked, keeping the legacy-discard path pinned to an unverified endpoint so the unverified-endpoint guard in teardownBrokerSession stays load-bearing. Compute the lock owner's process identity lazily on first acquisition instead of at module import, so CLI invocations that never take a lock no longer spawn ps/PowerShell on macOS and Windows. Read the broker pid file in a single syscall with failures treated as an absent pid, removing the exists/read TOCTOU window; an unreadable pid file now degrades to the persisted state pid instead of aborting shutdown. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01S29rWAKA85b1N7zAFC6NtB --- .../codex/scripts/lib/broker-lifecycle.mjs | 43 ++++++++++--------- plugins/codex/scripts/lib/locking.mjs | 15 +++++-- 2 files changed, 34 insertions(+), 24 deletions(-) diff --git a/plugins/codex/scripts/lib/broker-lifecycle.mjs b/plugins/codex/scripts/lib/broker-lifecycle.mjs index c95d3c313..f514c67c2 100644 --- a/plugins/codex/scripts/lib/broker-lifecycle.mjs +++ b/plugins/codex/scripts/lib/broker-lifecycle.mjs @@ -197,13 +197,18 @@ function clearBrokerSession(cwd) { function resolveBrokerPid(session) { const statePid = isValidPid(session.pid) ? session.pid : null; let filePid = null; - if (session.pidFile && fs.existsSync(session.pidFile)) { - const rawPid = fs.readFileSync(session.pidFile, "utf8").trim(); - if (/^\d+$/.test(rawPid)) { - const parsedPid = Number(rawPid); - filePid = isValidPid(parsedPid) ? parsedPid : null; + let rawPid = null; + if (session.pidFile) { + try { + rawPid = fs.readFileSync(session.pidFile, "utf8").trim(); + } catch { + // A pid file that vanished or cannot be read is treated as absent. } } + if (rawPid !== null && /^\d+$/.test(rawPid)) { + const parsedPid = Number(rawPid); + filePid = isValidPid(parsedPid) ? parsedPid : null; + } if (statePid && filePid && statePid !== filePid) { throw new Error(`Codex app-server broker PID mismatch (${statePid} != ${filePid}).`); } @@ -359,13 +364,7 @@ async function shutdownBrokerSessionLocked(cwd, options = {}) { let legacyProcessVerified = false; if (legacySession) { if (canDiscardUnownedSession(session, pid, options)) { - teardownBrokerSession({ - endpoint: null, - pidFile: session.pidFile ?? null, - logFile: session.logFile ?? null, - sessionDir: session.sessionDir ?? null - }); - clearBrokerSession(cwd); + teardownAndClear(cwd, session, false); return { found: true, exited: true, forced: false, reclaimedStaleEndpoint: false }; } legacyProcessVerified = processMatchesLegacyBroker(session, pid, options); @@ -460,14 +459,7 @@ async function shutdownBrokerSessionLocked(cwd, options = {}) { } const endpointIsOurs = endpointProven || reclaimedStaleEndpoint; - teardownBrokerSession({ - endpoint: endpointIsOurs ? session.endpoint ?? null : null, - pidFile: session.pidFile ?? null, - logFile: session.logFile ?? null, - sessionDir: session.sessionDir ?? null, - ownershipVerified: endpointIsOurs - }); - clearBrokerSession(cwd); + teardownAndClear(cwd, session, endpointIsOurs); return { found: true, exited: true, forced, reclaimedStaleEndpoint }; } @@ -537,6 +529,17 @@ async function ensureBrokerSessionLocked(cwd, options = {}) { return session; } +function teardownAndClear(cwd, session, endpointIsOurs) { + teardownBrokerSession({ + endpoint: endpointIsOurs ? session.endpoint ?? null : null, + pidFile: session.pidFile ?? null, + logFile: session.logFile ?? null, + sessionDir: session.sessionDir ?? null, + ownershipVerified: endpointIsOurs + }); + clearBrokerSession(cwd); +} + function teardownBrokerSession({ endpoint = null, pidFile, diff --git a/plugins/codex/scripts/lib/locking.mjs b/plugins/codex/scripts/lib/locking.mjs index 849ad9b52..ff569861b 100644 --- a/plugins/codex/scripts/lib/locking.mjs +++ b/plugins/codex/scripts/lib/locking.mjs @@ -10,9 +10,16 @@ const DEFAULT_STALE_MS = 30000; const RETRY_DELAY_MS = 25; const OWNER_FILE_PREFIX = "owner-"; const OWNER_FILE_SUFFIX = ".json"; -// Start-time lookup may spawn `ps` or PowerShell off Linux. Cache it once per -// process instead of paying that cost for every short state update. -const PROCESS_IDENTITY = getProcessIdentity(process.pid); +// Start-time lookup may spawn `ps` or PowerShell off Linux. Compute it lazily, +// on first lock acquisition, and cache it for the rest of the process instead +// of paying that cost on import for every short state update. +let processIdentity; +function cachedProcessIdentity() { + if (processIdentity === undefined) { + processIdentity = getProcessIdentity(process.pid); + } + return processIdentity; +} function sleepSync(ms) { Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, ms); @@ -67,7 +74,7 @@ function tryAcquire(lockDir) { pid: process.pid, token, createdAt: Date.now(), - processIdentity: PROCESS_IDENTITY + processIdentity: cachedProcessIdentity() }); fs.renameSync(candidateDir, lockDir); return handle; From dc263cbb3941ed61173ff41c0293e970212ce4d6 Mon Sep 17 00:00:00 2001 From: Heringer Date: Sat, 25 Jul 2026 10:43:06 -0300 Subject: [PATCH 16/20] Drop unused verifyProcess override from token matching The verifyProcess escape hatch was threaded through earlier revisions of this branch, but its last caller was refactored away during the helper consolidation, so the ternary can never take the override arm. Collapse processMatchesInstanceToken to the real launch-token probe. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01S29rWAKA85b1N7zAFC6NtB --- plugins/codex/scripts/lib/broker-lifecycle.mjs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/plugins/codex/scripts/lib/broker-lifecycle.mjs b/plugins/codex/scripts/lib/broker-lifecycle.mjs index f514c67c2..8defb42d6 100644 --- a/plugins/codex/scripts/lib/broker-lifecycle.mjs +++ b/plugins/codex/scripts/lib/broker-lifecycle.mjs @@ -336,9 +336,7 @@ async function canReclaimStaleEndpoint(session, pid, options = {}) { } function processMatchesInstanceToken(pid, instanceToken, options) { - return options.verifyProcess - ? options.verifyProcess(pid, instanceToken) - : processHasLaunchToken(pid, instanceToken, { ...options, marker: "--instance-token" }); + return processHasLaunchToken(pid, instanceToken, { ...options, marker: "--instance-token" }); } // Legacy sessions have no instance token, so their processes are re-verified From 7186d7938e76fecd7d2c016cce7f748440dc362c Mon Sep 17 00:00:00 2001 From: Heringer Date: Sat, 25 Jul 2026 11:10:05 -0300 Subject: [PATCH 17/20] Confirm broker spawn before persisting session state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit child_process.spawn() reports failures like a vanished workspace directory asynchronously, as an "error" event on the next tick. ensureBrokerSession persisted broker.json before that event could fire, so a failed spawn crashed the invocation via the unhandled event and left behind a tokenized session with pid: null — a combination shutdownBrokerSessionLocked() rejects fatally before any reclaim path runs, wedging every later invocation in that workspace. Await the child's spawn/error outcome before writing state: on failure, tear down the just-created session artifacts and surface a descriptive error instead. Regression test covers the rejection, the absence of persisted state, and the cleanup of the temporary session directory. Reported by the Codex GitHub reviewer on this branch. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01S29rWAKA85b1N7zAFC6NtB --- .../codex/scripts/lib/broker-lifecycle.mjs | 32 +++++++++++++++++++ tests/broker-lifecycle.test.mjs | 24 ++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/plugins/codex/scripts/lib/broker-lifecycle.mjs b/plugins/codex/scripts/lib/broker-lifecycle.mjs index 8defb42d6..d7ee20f78 100644 --- a/plugins/codex/scripts/lib/broker-lifecycle.mjs +++ b/plugins/codex/scripts/lib/broker-lifecycle.mjs @@ -172,6 +172,19 @@ function spawnBrokerProcess({ } } +// child_process.spawn() can fail asynchronously (e.g. ENOENT when `cwd` does +// not exist): Node emits an "error" event on the next tick instead of +// throwing. Without a listener that event crashes the process, and callers +// that raced ahead to persist a tokenized session would leave a wedged +// broker.json (pid: null) behind. Callers must await this before treating the +// child as spawned. +function waitForBrokerSpawn(child) { + return new Promise((resolve, reject) => { + child.once("spawn", resolve); + child.once("error", reject); + }); +} + function resolveBrokerStateFile(cwd) { return path.join(resolveStateDir(cwd), BROKER_STATE_FILE); } @@ -508,6 +521,25 @@ async function ensureBrokerSessionLocked(cwd, options = {}) { env: options.env ?? process.env }); + // Persistence is deferred until the spawn is confirmed, so a session that + // never actually started never gets written with pid: null + a live + // instanceToken — that combination poisons shutdownBrokerSessionLocked() + // for every later invocation against this cwd (it throws "PID is + // unavailable" before any reclaim path runs), wedging the workspace. + try { + await waitForBrokerSpawn(child); + } catch (error) { + teardownBrokerSession({ + pidFile, + logFile, + sessionDir, + ownershipVerified: false + }); + throw new Error( + `Codex app-server broker failed to spawn: ${error?.message ?? "unknown error"}` + ); + } + const session = { endpoint, pidFile, diff --git a/tests/broker-lifecycle.test.mjs b/tests/broker-lifecycle.test.mjs index 69ae7278d..a0104fc8d 100644 --- a/tests/broker-lifecycle.test.mjs +++ b/tests/broker-lifecycle.test.mjs @@ -1,6 +1,7 @@ import assert from "node:assert/strict"; import fs from "node:fs"; import net from "node:net"; +import os from "node:os"; import path from "node:path"; import process from "node:process"; import test from "node:test"; @@ -537,6 +538,29 @@ test("ensureBrokerSession recovers from a stale socket instead of failing", { sk assert.notEqual(session.endpoint, `unix:${socketPath}`); }); +test("ensureBrokerSession fails cleanly when the broker process cannot spawn", async () => { + const workspace = makeTempDir(); + // spawn() launches the broker with `cwd` as the child's working directory. + // Deleting it after makeTempDir() reproduces a workspace that vanishes + // between the caller's check and the spawn: Node cannot fail synchronously + // here, it emits an asynchronous "error" event (ENOENT) instead. + fs.rmSync(workspace, { recursive: true, force: true }); + + const before = new Set(fs.readdirSync(os.tmpdir()).filter((name) => name.startsWith("cxc-"))); + + await assert.rejects(() => ensureBrokerSession(workspace, { env: process.env }), /failed to spawn/); + + assert.equal( + loadBrokerSession(workspace), + null, + "a spawn failure must never persist a tokenized session with pid: null, or shutdown wedges permanently" + ); + + const after = new Set(fs.readdirSync(os.tmpdir()).filter((name) => name.startsWith("cxc-"))); + const leaked = [...after].filter((name) => !before.has(name)); + assert.deepEqual(leaked, [], "the failed attempt's temporary session directory must not be left behind"); +}); + test("shutdown still refuses to unlink an endpoint someone is listening on", { skip: process.platform === "win32" }, async (t) => { const workspace = makeTempDir(); const sessionDir = makeTempDir("cxc-live-listener-"); From ad3d852593978cae6aa415c3d7264534a728071d Mon Sep 17 00:00:00 2001 From: Heringer Date: Sat, 25 Jul 2026 12:23:12 -0300 Subject: [PATCH 18/20] Fail closed on pid file errors and unwind failed session persists Final-audit fixes on the broker lifecycle and locking paths: - resolveBrokerPid() swallowed every pid file read error, so an unreadable file (EACCES, EISDIR) silently skipped the PID mismatch check and fell back to session.pid alone. Only ENOENT is treated as an absent file now; any other failure aborts the shutdown path with the state preserved. - ensureBrokerSessionLocked() persisted broker.json without guarding the write. A persist failure (ENOSPC, EACCES) after a confirmed spawn left the detached child running with no state pointing at it, so later invocations could stack a second broker on top. On failure the child tree is killed, the session artifacts are torn down, and the original error is rethrown. - tryAcquire() resolved the process identity between creating the candidate directory and publishing it, so a crash during a slow ps/PowerShell lookup could orphan the candidate. The lookup now happens before mkdir, shrinking the window back to the write-and-rename itself. - Reworded the stale null-identity reclaim comment: this module never shipped without identity records, so null means the lookup failed at acquire time, and reclaiming after staleMs remains the deliberate lesser risk versus wedging every later caller behind a reused PID. - Dropped the dead node:fs import from app-server-broker.mjs. Regression tests cover the fail-closed pid file read and the kill-and-teardown unwind after a failed persist. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_018uAqG4qqx6YKWzXjd7YMDG --- plugins/codex/scripts/app-server-broker.mjs | 1 - .../codex/scripts/lib/broker-lifecycle.mjs | 29 ++++++++- plugins/codex/scripts/lib/locking.mjs | 13 ++-- tests/broker-lifecycle.test.mjs | 65 +++++++++++++++++++ 4 files changed, 100 insertions(+), 8 deletions(-) diff --git a/plugins/codex/scripts/app-server-broker.mjs b/plugins/codex/scripts/app-server-broker.mjs index 17d50a85d..b55c14ff7 100644 --- a/plugins/codex/scripts/app-server-broker.mjs +++ b/plugins/codex/scripts/app-server-broker.mjs @@ -1,6 +1,5 @@ #!/usr/bin/env node -import fs from "node:fs"; import net from "node:net"; import path from "node:path"; import process from "node:process"; diff --git a/plugins/codex/scripts/lib/broker-lifecycle.mjs b/plugins/codex/scripts/lib/broker-lifecycle.mjs index d7ee20f78..d85578805 100644 --- a/plugins/codex/scripts/lib/broker-lifecycle.mjs +++ b/plugins/codex/scripts/lib/broker-lifecycle.mjs @@ -214,8 +214,12 @@ function resolveBrokerPid(session) { if (session.pidFile) { try { rawPid = fs.readFileSync(session.pidFile, "utf8").trim(); - } catch { - // A pid file that vanished or cannot be read is treated as absent. + } catch (error) { + if (error?.code !== "ENOENT") { + throw error; + } + // A pid file that vanished is treated as absent; any other failure is + // an integrity problem and must abort instead of masking a mismatch. } } if (rawPid !== null && /^\d+$/.test(rawPid)) { @@ -548,7 +552,26 @@ async function ensureBrokerSessionLocked(cwd, options = {}) { pid: child.pid ?? null, instanceToken }; - saveBrokerSession(cwd, session); + try { + saveBrokerSession(cwd, session); + } catch (error) { + // The spawn already succeeded, so a failed persist would otherwise leave + // an untracked broker running with no state pointing at it. + try { + shutdownOptions.killProcess(child.pid); + } catch { + // Teardown must still run and the original persist error must still + // surface; a failure to kill the child is secondary to both. + } + teardownBrokerSession({ + endpoint, + pidFile, + logFile, + sessionDir, + ownershipVerified: true + }); + throw error; + } const ready = await waitForBrokerEndpoint(endpoint, options.timeoutMs ?? 2000); if (!ready) { diff --git a/plugins/codex/scripts/lib/locking.mjs b/plugins/codex/scripts/lib/locking.mjs index ff569861b..9074aac62 100644 --- a/plugins/codex/scripts/lib/locking.mjs +++ b/plugins/codex/scripts/lib/locking.mjs @@ -68,13 +68,16 @@ function tryAcquire(lockDir) { candidateOwnerFile }; + // Resolve identity before creating the candidate so a crash during a slow + // ps/PowerShell lookup cannot leave an orphaned candidate directory behind. + const identity = cachedProcessIdentity(); fs.mkdirSync(candidateDir, { mode: 0o700 }); try { writePrivateJson(candidateOwnerFile, { pid: process.pid, token, createdAt: Date.now(), - processIdentity: cachedProcessIdentity() + processIdentity: identity }); fs.renameSync(candidateDir, lockDir); return handle; @@ -162,9 +165,11 @@ function reclaimAbandonedLock(lockDir, options) { identity: processIdentity ?? undefined }) ) { - // Older locks on macOS and Windows did not record a process identity. - // Once such a short-lived critical section is stale, a live PID alone - // cannot distinguish the original owner from an unrelated reused PID. + // When the identity lookup failed at acquire time the owner records + // null. Once such a short-lived critical section looks stale, a live + // PID alone cannot distinguish the original owner from an unrelated + // process that reused the PID, so reclaiming is the lesser risk: + // refusing would wedge every later caller behind a reused PID. if (processIdentity == null && state.ageMs > options.staleMs) { return removeOwnedLock(state.ownerFile, lockDir); } diff --git a/tests/broker-lifecycle.test.mjs b/tests/broker-lifecycle.test.mjs index a0104fc8d..3fda921f8 100644 --- a/tests/broker-lifecycle.test.mjs +++ b/tests/broker-lifecycle.test.mjs @@ -688,3 +688,68 @@ test("an endpoint outside the plugin session directory is never reclaimed", { sk assert.equal(fs.existsSync(socketPath), true, "foreign socket must survive"); fs.unlinkSync(socketPath); }); + +test("shutdown aborts instead of silently skipping an unreadable pid file", async () => { + const workspace = makeTempDir(); + const sessionDir = makeTempDir("cxc-unreadable-pid-"); + const pidFile = path.join(sessionDir, "broker.pid"); + // A directory at the pid file path makes the read fail with EISDIR instead + // of ENOENT, exercising the "any other failure must abort" branch of + // resolveBrokerPid() rather than the "file genuinely absent" branch. + fs.mkdirSync(pidFile); + + // Deliberately no instanceToken: this session can never be cleaned up + // through shutdownBrokerSession() (the unreadable pid file always aborts + // it, by design), so it must stay invisible to the global owned-broker + // teardown in tests/helpers.mjs, which only acts on sessions carrying one. + const session = { + endpoint: `unix:${path.join(sessionDir, "broker.sock")}`, + pid: null, + pidFile, + logFile: null, + sessionDir + }; + saveBrokerSession(workspace, session); + + await assert.rejects( + shutdownBrokerSession(workspace, { timeoutMs: 40 }), + (error) => error?.code === "EISDIR" + ); + + assert.deepEqual(loadBrokerSession(workspace), session); +}); + +test("ensureBrokerSession tears down a spawned child when persisting the session fails", { skip: process.platform === "win32" }, async () => { + const workspace = makeTempDir(); + const binDir = makeTempDir(); + installFakeCodex(binDir); + + const stateDir = resolveStateDir(workspace); + fs.mkdirSync(stateDir, { recursive: true }); + // Occupies the broker.json path with a directory so saveBrokerSession's + // rename-into-place fails with EISDIR only after the child has already + // spawned successfully. + const brokerStateFile = path.join(stateDir, "broker.json"); + fs.mkdirSync(brokerStateFile); + + const killedPids = []; + const killProcess = (pid) => { + killedPids.push(pid); + return terminateProcessTree(pid); + }; + + const before = new Set(fs.readdirSync(os.tmpdir()).filter((name) => name.startsWith("cxc-"))); + + await assert.rejects(ensureBrokerSession(workspace, { env: buildEnv(binDir), killProcess })); + + assert.equal(killedPids.length, 1, "the spawned child must be killed exactly once"); + assert.ok(Number.isSafeInteger(killedPids[0]) && killedPids[0] > 0); + + const after = new Set(fs.readdirSync(os.tmpdir()).filter((name) => name.startsWith("cxc-"))); + const leaked = [...after].filter((name) => !before.has(name)); + assert.deepEqual(leaked, [], "the session directory of the failed persist must not be left behind"); + + assert.equal(loadBrokerSession(workspace), null); + assert.equal(fs.existsSync(brokerStateFile), true, "the blocking directory itself is left untouched"); + assert.equal(fs.statSync(brokerStateFile).isDirectory(), true); +}); From d99093734e582bf08818a0cf91d59245a1ebdb45 Mon Sep 17 00:00:00 2001 From: Heringer Date: Sat, 25 Jul 2026 13:57:28 -0300 Subject: [PATCH 19/20] Pin broker liveness checks to a persisted process identity The Codex GitHub reviewer flagged two P2 gaps on the previous commit: sessions persist only a PID, so a recycled PID wedges the workspace (a group-leader lookalike makes shutdown throw "ownership could not be verified" forever, and any live holder of the PID blocks stale-socket reclaim), and the failed-persist unwind killed and deleted artifacts without confirming the child actually exited. Persist the broker's process identity (its start time) in broker.json at spawn time and pin every liveness and exit check on the persisted pid to it. The identity follows the pid+creation-time convention (psutil, OCSF common-process-id) and fails closed end to end: - On Linux the identity is boot_id:starttime -- starttime alone is in clock ticks since boot and would collide across reboots. When boot_id cannot be read, no identity is recorded and none is compared; a bare tick count is never emitted or matched, and a transient read failure is not cached. - isProcessTreeRunning() only treats the tree as gone on a CONFIRMED replacement (pid alive with a different readable identity). POSIX keeps a group leader's pid reserved while its group has members, so a confirmed replacement also proves the recorded group is gone; an absent or unreadable leader still falls through to the group check, keeping live descendants visible. - ps-based process-table queries run under LC_ALL=C TZ=UTC so the recorded lstart rendering cannot drift with the caller's locale or timezone and falsely mismatch a still-live broker. - A persisted identity is honored only when it is a non-empty string; malformed values fall back to the legacy no-identity behavior instead of reading a live broker as replaced. The failed-persist unwind now mirrors the ownership-before-kill invariant of the forced-shutdown path: it only signals the child when its argv still carries this session's launch token, waits for a confirmed exit pinned to the recorded identity, and preserves the session artifacts with a descriptive error when the exit cannot be confirmed instead of deleting them out from under a live process. Regression tests cover the reused-pid mismatch, the dead-leader with a surviving descendant, the preserved-artifacts branch, and the confirmed teardown, each verified to fail against the unfixed code; test cleanups now assert the processes they spawn actually exited. Reported by the Codex GitHub reviewer on this branch. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_018uAqG4qqx6YKWzXjd7YMDG --- .../codex/scripts/lib/broker-lifecycle.mjs | 72 ++++++-- plugins/codex/scripts/lib/process.mjs | 81 ++++++++- tests/broker-lifecycle.test.mjs | 158 +++++++++++++++++- tests/process.test.mjs | 69 +++++++- 4 files changed, 356 insertions(+), 24 deletions(-) diff --git a/plugins/codex/scripts/lib/broker-lifecycle.mjs b/plugins/codex/scripts/lib/broker-lifecycle.mjs index d85578805..1c8681cf0 100644 --- a/plugins/codex/scripts/lib/broker-lifecycle.mjs +++ b/plugins/codex/scripts/lib/broker-lifecycle.mjs @@ -18,6 +18,7 @@ import { } from "./fs.mjs"; import { withLock } from "./locking.mjs"; import { + getProcessIdentity, isProcessRunning, isProcessTreeRunning, isValidPid, @@ -375,16 +376,29 @@ async function shutdownBrokerSessionLocked(cwd, options = {}) { } const pid = resolveBrokerPid(session); + // Liveness/exit checks against the persisted pid must be pinned to the + // process identity recorded at spawn time (when available), so a reused + // PID reads as exited instead of being mistaken for the original broker. + // Ownership checks (token or launch-artifact matching) are a separate, + // stronger proof and are deliberately left on the caller-supplied options. + // `!= null` alone would also accept "" or a non-string value that + // String()-coerces to something no live process could ever match, which + // would misread a live broker as replaced -- validate the shape first. + const recordedIdentity = + typeof session.processIdentity === "string" && session.processIdentity.length > 0 + ? session.processIdentity + : null; + const livenessOptions = recordedIdentity != null ? { ...options, identity: recordedIdentity } : options; const legacySession = Boolean(session.endpoint && !session.instanceToken); let legacyProcessVerified = false; if (legacySession) { - if (canDiscardUnownedSession(session, pid, options)) { + if (canDiscardUnownedSession(session, pid, livenessOptions)) { teardownAndClear(cwd, session, false); return { found: true, exited: true, forced: false, reclaimedStaleEndpoint: false }; } legacyProcessVerified = processMatchesLegacyBroker(session, pid, options); const legacyEndpointIsSafelyStale = - isValidPid(pid) && (await canReclaimStaleEndpoint(session, pid, options)); + isValidPid(pid) && (await canReclaimStaleEndpoint(session, pid, livenessOptions)); if (!legacyProcessVerified && !legacyEndpointIsSafelyStale) { throw new Error("Codex app-server broker ownership could not be verified; persisted state was preserved."); } @@ -418,13 +432,15 @@ async function shutdownBrokerSessionLocked(cwd, options = {}) { } let verifiedPid = ownershipVerified ? acknowledgedPid ?? pid : null; - let exited = isValidPid(pid) ? await waitForProcessExit(pid, { ...options, timeoutMs: 0 }) : false; + let exited = isValidPid(pid) + ? await waitForProcessExit(pid, { ...livenessOptions, timeoutMs: 0 }) + : false; let processOwnershipProven = false; if (!shutdownAck && isValidPid(pid) && !exited) { const ownsPersistedProcess = ownsBrokerProcess(session, pid, legacySession, options); if (!ownsPersistedProcess) { - if (isProcessTreeRunning(pid, options)) { + if (isProcessTreeRunning(pid, livenessOptions)) { throw new Error("Codex app-server broker ownership could not be verified; persisted state was preserved."); } exited = true; @@ -439,7 +455,7 @@ async function shutdownBrokerSessionLocked(cwd, options = {}) { } if (!exited && isValidPid(verifiedPid)) { - exited = await waitForProcessExit(verifiedPid, options); + exited = await waitForProcessExit(verifiedPid, livenessOptions); } let forced = false; @@ -451,7 +467,7 @@ async function shutdownBrokerSessionLocked(cwd, options = {}) { processOwnershipProven = true; options.killProcess(verifiedPid); forced = true; - exited = await waitForProcessExit(verifiedPid, options); + exited = await waitForProcessExit(verifiedPid, livenessOptions); } if (!exited) { @@ -467,7 +483,7 @@ async function shutdownBrokerSessionLocked(cwd, options = {}) { const endpointProven = ownershipVerified || processOwnershipProven; let reclaimedStaleEndpoint = false; if (!endpointProven && endpointArtifactExists(session.endpoint)) { - reclaimedStaleEndpoint = await canReclaimStaleEndpoint(session, pid, options); + reclaimedStaleEndpoint = await canReclaimStaleEndpoint(session, pid, livenessOptions); if (!reclaimedStaleEndpoint) { throw new Error("Codex app-server broker endpoint ownership could not be verified; persisted state was preserved."); } @@ -550,18 +566,48 @@ async function ensureBrokerSessionLocked(cwd, options = {}) { logFile, sessionDir, pid: child.pid ?? null, + // Cheap on Linux (one /proc read); a one-time ps/PowerShell call + // elsewhere. A null fallback degrades gracefully to today's behavior + // (no identity check) rather than failing session creation over it. + processIdentity: getProcessIdentity(child.pid) ?? null, instanceToken }; try { saveBrokerSession(cwd, session); } catch (error) { // The spawn already succeeded, so a failed persist would otherwise leave - // an untracked broker running with no state pointing at it. - try { - shutdownOptions.killProcess(child.pid); - } catch { - // Teardown must still run and the original persist error must still - // surface; a failure to kill the child is secondary to both. + // an untracked broker running with no state pointing at it. Killing and + // confirming exit before discarding artifacts avoids trading that leak + // for a worse one: deleting the socket/session dir out from under a + // child that is actually still alive because the kill was denied or slow. + // + // Build explicit options here instead of reusing shutdownOptions as-is: + // a caller-supplied identity must never leak into this check (we only + // ever want to compare against the identity we just recorded for this + // child), and shutdownOptions carries whatever the caller passed in. + const unwindOptions = { ...shutdownOptions, identity: session.processIdentity ?? undefined }; + // Only signal a pid that is provably still our child, mirroring the same + // ownership-before-kill invariant the forced-shutdown path already uses + // (see stillOwnsProcess above): the launch token in its argv is proof of + // possession, since an unrelated process that merely reused the pid + // cannot carry it. If the probe can't read the process at all (already + // gone, or unreadable), the gate simply skips the kill and the + // wait/preserve branch below handles it loudly either way. + if (processMatchesInstanceToken(child.pid, instanceToken, options)) { + try { + shutdownOptions.killProcess(child.pid); + } catch { + // A failure to signal the child is secondary to the persist error; + // waitForProcessExit below still tells us whether it actually died. + } + } + const childExited = await waitForProcessExit(child.pid, unwindOptions); + if (!childExited) { + throw new Error( + `Codex app-server broker session could not be persisted (${ + error?.message ?? "unknown error" + }), and the spawned process ${child.pid} did not exit; session artifacts were preserved.` + ); } teardownBrokerSession({ endpoint, diff --git a/plugins/codex/scripts/lib/process.mjs b/plugins/codex/scripts/lib/process.mjs index f421e08c1..c7396915b 100644 --- a/plugins/codex/scripts/lib/process.mjs +++ b/plugins/codex/scripts/lib/process.mjs @@ -87,7 +87,16 @@ function readLinuxProcessStat(pid) { function queryProcessTable(pid, powershellCommand, psFormat, options) { const runCommandImpl = options.runCommandImpl ?? runCommand; - const spawnOptions = { timeout: options.timeoutMs ?? 2000, killSignal: "SIGTERM" }; + const spawnOptions = { + timeout: options.timeoutMs ?? 2000, + killSignal: "SIGTERM", + // ps -o lstart= renders via the C library's %c, which is locale- and + // TZ-dependent: the same instant can print differently between the + // recording and the checking invocation, producing a false mismatch on a + // still-live broker. Force a fixed, unambiguous rendering. Harmless for + // the PowerShell path (already UTC ticks) and for command-line reads. + env: { ...(options.env ?? process.env), LC_ALL: "C", TZ: "UTC" } + }; const result = (options.platform ?? process.platform) === "win32" ? runCommandImpl( @@ -102,12 +111,46 @@ function queryProcessTable(pid, powershellCommand, psFormat, options) { return String(result.stdout ?? ""); } +// /proc//stat's starttime is in clock ticks since BOOT, not since the +// epoch, so the bare value collides across reboots (a reused pid after a +// reboot can present the same tick count as an earlier, unrelated process). +// Prefixing with the boot id disambiguates across reboots while keeping +// identities comparable within one. Read once and cache for the process +// lifetime -- it cannot change without a reboot, which also invalidates any +// identity recorded before it. Only a SUCCESSFUL read is cached: a transient +// failure (e.g. a sandboxed /proc) must not poison every later call for the +// rest of the process's lifetime -- each subsequent call gets another chance +// to read it. +let cachedBootId; +function bootId() { + if (cachedBootId === undefined) { + try { + cachedBootId = fs.readFileSync("/proc/sys/kernel/random/boot_id", "utf8").trim(); + } catch { + return ""; + } + } + return cachedBootId; +} + export function getProcessIdentity(pid, options = {}) { if (!isValidPid(pid)) { return null; } if ((options.platform ?? process.platform) === "linux") { - return readLinuxProcessStat(pid)?.startTime ?? null; + const startTime = readLinuxProcessStat(pid)?.startTime ?? null; + if (startTime == null) { + return null; + } + const boot = bootId(); + if (!boot) { + // boot_id unreadable: a raw tick count is boot-relative and would both + // collide across boots and falsely mismatch against composite values + // recorded when boot_id WAS readable. Unknown must never masquerade + // as a comparable identity. + return null; + } + return `${boot}:${startTime}`; } const output = queryProcessTable( pid, @@ -148,8 +191,25 @@ export function isProcessRunning(pid, options = {}) { if (stat.state === "Z" || stat.state === "X") { return false; } - if (options.identity != null && stat.startTime !== String(options.identity)) { - return false; + if (options.identity != null) { + // getProcessIdentity() prefixes Linux identities with the boot id + // (see its comment); build the same composite here so this direct + // stat.startTime comparison stays consistent with values recorded + // through getProcessIdentity(). Only compare when boot_id is + // currently readable and the composite can actually be formed -- + // a bare tick count is boot-relative and must never be compared + // directly, which would both collide across boots and falsely + // mismatch a still-live process against a composite identity + // recorded when boot_id WAS readable. + const boot = bootId(); + if (boot) { + const currentIdentity = `${boot}:${stat.startTime}`; + if (currentIdentity !== String(options.identity)) { + return false; + } + } + // boot_id unreadable right now: the comparison cannot be formed, so + // it proves nothing. Failure to inspect is not proof of replacement. } } } else if (options.identity != null) { @@ -194,6 +254,19 @@ export function isProcessTreeRunning(pid, options = {}) { if (platform === "win32") { return isProcessRunning(pid, options); } + if (options.identity != null) { + const currentIdentity = getProcessIdentity(pid, options); + if (currentIdentity != null && currentIdentity !== String(options.identity)) { + // The pid exists and belongs to a different process. POSIX keeps a group + // leader's pid reserved while its group still has members, so a confirmed + // replacement also proves the recorded group is gone. + return false; + } + // An absent or unreadable pid proves nothing about survivors in its group; + // fall through to the group checks below. In particular, a leader that + // has already exited (identity now unreadable) must not short-circuit + // here: its process group can still have live descendants. + } if (platform === "linux" && !options.killImpl) { return isLinuxProcessGroupRunning(pid); } diff --git a/tests/broker-lifecycle.test.mjs b/tests/broker-lifecycle.test.mjs index 3fda921f8..0c1f03baf 100644 --- a/tests/broker-lifecycle.test.mjs +++ b/tests/broker-lifecycle.test.mjs @@ -9,6 +9,7 @@ import { spawn } from "node:child_process"; import { buildEnv, installFakeCodex } from "./fake-codex-fixture.mjs"; import { makeTempDir } from "./helpers.mjs"; +import { createBrokerEndpoint } from "../plugins/codex/scripts/lib/broker-endpoint.mjs"; import { ensureBrokerSession, loadBrokerSession, @@ -18,7 +19,8 @@ import { } from "../plugins/codex/scripts/lib/broker-lifecycle.mjs"; import { isProcessTreeRunning, - terminateProcessTree + terminateProcessTree, + waitForProcessExit } from "../plugins/codex/scripts/lib/process.mjs"; import { acquireLock, @@ -738,18 +740,162 @@ test("ensureBrokerSession tears down a spawned child when persisting the session return terminateProcessTree(pid); }; - const before = new Set(fs.readdirSync(os.tmpdir()).filter((name) => name.startsWith("cxc-"))); + // Capture the exact session directory ensureBrokerSessionLocked creates + // via the same injection point other tests use, instead of diffing + // os.tmpdir() -- concurrent test files also create cxc-* directories, so a + // tmpdir-wide before/after snapshot races with them. + let capturedSessionDir = null; + const createBrokerEndpointSpy = (sessionDir, platform) => { + capturedSessionDir = sessionDir; + return createBrokerEndpoint(sessionDir, platform); + }; - await assert.rejects(ensureBrokerSession(workspace, { env: buildEnv(binDir), killProcess })); + await assert.rejects( + ensureBrokerSession(workspace, { env: buildEnv(binDir), killProcess, createBrokerEndpoint: createBrokerEndpointSpy }) + ); assert.equal(killedPids.length, 1, "the spawned child must be killed exactly once"); assert.ok(Number.isSafeInteger(killedPids[0]) && killedPids[0] > 0); - const after = new Set(fs.readdirSync(os.tmpdir()).filter((name) => name.startsWith("cxc-"))); - const leaked = [...after].filter((name) => !before.has(name)); - assert.deepEqual(leaked, [], "the session directory of the failed persist must not be left behind"); + assert.ok(capturedSessionDir, "ensureBrokerSession must have created a session directory"); + assert.equal( + fs.existsSync(capturedSessionDir), + false, + "the session directory of the failed persist must not be left behind" + ); assert.equal(loadBrokerSession(workspace), null); assert.equal(fs.existsSync(brokerStateFile), true, "the blocking directory itself is left untouched"); assert.equal(fs.statSync(brokerStateFile).isDirectory(), true); }); + +test("shutdown treats a reused PID as exited once the recorded identity stops matching", { skip: process.platform === "win32" }, async (t) => { + const workspace = makeTempDir(); + const sessionDir = makeTempDir("cxc-reused-pid-"); + const socketPath = path.join(sessionDir, "broker.sock"); + + // A detached process is its own process-group leader (unlike this test's + // own process under `node --test`, which is not) -- exactly the shape of + // an impostor that reused a dead broker's PID and became an unrelated new + // group leader. Its real identity can never equal the bogus one recorded + // below, so liveness must be resolved from session.processIdentity rather + // than group membership alone, which would otherwise read it as running. + const dummy = spawn(process.execPath, ["-e", "setInterval(() => {}, 1000)"], { + detached: true, + stdio: "ignore" + }); + await new Promise((resolve, reject) => { + dummy.once("spawn", resolve); + dummy.once("error", reject); + }); + t.after(async () => { + if (!isProcessTreeRunning(dummy.pid)) { + return; + } + terminateProcessTree(dummy.pid); + let exited = await waitForProcessExit(dummy.pid, { timeoutMs: 2000 }); + if (!exited) { + try { + process.kill(-dummy.pid, "SIGKILL"); + } catch { + // Already gone between the check above and here. + } + exited = await waitForProcessExit(dummy.pid, { timeoutMs: 2000 }); + } + assert.equal(exited, true, "cleanup must confirm the test process exited"); + }); + + const session = { + endpoint: `unix:${socketPath}`, + pid: dummy.pid, + pidFile: null, + logFile: null, + sessionDir, + instanceToken: "reused-pid-token", + processIdentity: "reused-pid-identity-mismatch" + }; + saveBrokerSession(workspace, session); + + const outcome = await shutdownBrokerSession(workspace, { timeoutMs: 40 }); + + assert.equal(outcome.found, true); + assert.equal(outcome.exited, true); + assert.equal(outcome.forced, false); + assert.equal(loadBrokerSession(workspace), null); +}); + +test("ensureBrokerSession preserves session artifacts when the killed child does not actually exit", { skip: process.platform === "win32" }, async () => { + const workspace = makeTempDir(); + const binDir = makeTempDir(); + installFakeCodex(binDir); + + const stateDir = resolveStateDir(workspace); + fs.mkdirSync(stateDir, { recursive: true }); + // Same EISDIR trigger as the sibling test above, just paired here with a + // killProcess that does not actually stop the child, to exercise the + // "kill was denied or too slow" branch of the failed-persist unwind. + const brokerStateFile = path.join(stateDir, "broker.json"); + fs.mkdirSync(brokerStateFile); + + let killedPid = null; + const killProcess = (pid) => { + killedPid = pid; + // Deliberately a no-op: simulates a kill signal that was denied or did + // not land in time. The child stays alive. + }; + + // Capture the exact session directory via the same injection point other + // tests use, instead of diffing os.tmpdir() -- concurrent test files also + // create cxc-* directories, so a tmpdir-wide before/after snapshot races + // with them. + let sessionDir = null; + const createBrokerEndpointSpy = (dir, platform) => { + sessionDir = dir; + return createBrokerEndpoint(dir, platform); + }; + + try { + await assert.rejects( + ensureBrokerSession(workspace, { + env: buildEnv(binDir), + killProcess, + timeoutMs: 150, + createBrokerEndpoint: createBrokerEndpointSpy + }), + /did not exit.*artifacts were preserved/i + ); + + assert.ok(Number.isSafeInteger(killedPid) && killedPid > 0, "killProcess must have been invoked with the child pid"); + assert.equal(isProcessTreeRunning(killedPid), true, "the child must still be alive for this branch to be exercised"); + + assert.ok(sessionDir, "ensureBrokerSession must have created a session directory"); + assert.equal(fs.existsSync(sessionDir), true, "the session directory must not be torn down while the child is still alive"); + + assert.equal(fs.existsSync(brokerStateFile), true, "the blocking directory itself is left untouched"); + assert.equal(fs.statSync(brokerStateFile).isDirectory(), true); + } finally { + // The injected killProcess deliberately did nothing, so the real child is + // still running: terminate and reap it for real before the test exits, + // then remove the preserved session directory ourselves so this test's + // deliberate leak does not confuse the leak-detection windows of other + // tests running in the same tmpdir. Only rmSync after confirming exit -- + // otherwise a still-alive process could recreate files under sessionDir + // after we remove it. + if (killedPid) { + terminateProcessTree(killedPid); + let exited = await waitForProcessExit(killedPid, { timeoutMs: 2000 }); + if (!exited) { + try { + process.kill(-killedPid, "SIGKILL"); + } catch { + // Already gone between the check above and here. + } + exited = await waitForProcessExit(killedPid, { timeoutMs: 2000 }); + } + assert.equal(exited, true, "cleanup must confirm the test process exited"); + } + if (sessionDir) { + fs.rmSync(sessionDir, { recursive: true, force: true }); + } + } +}); diff --git a/tests/process.test.mjs b/tests/process.test.mjs index d99bf37d4..87da53291 100644 --- a/tests/process.test.mjs +++ b/tests/process.test.mjs @@ -1,14 +1,17 @@ import process from "node:process"; import test from "node:test"; import assert from "node:assert/strict"; +import { spawn } from "node:child_process"; import { getProcessIdentity, isProcessRunning, + isProcessTreeRunning, processHasLaunchSequence, runCommand, runCommandChecked, - terminateProcessTree + terminateProcessTree, + waitForProcessExit } from "../plugins/codex/scripts/lib/process.mjs"; const SELF_TERMINATING_SCRIPT = "process.kill(process.pid, 'SIGTERM'); setInterval(() => {}, 1000);"; @@ -199,3 +202,67 @@ test("terminateProcessTree treats missing Windows processes as already stopped", assert.equal(outcome.result.status, 128); assert.match(outcome.result.stdout, /not found/i); }); + +test("a dead group leader with a surviving descendant still counts as a running tree", { skip: process.platform === "win32" }, async () => { + // The leader spawns a grandchild without detaching it: an un-detached + // child inherits its parent's process group (standard POSIX fork/exec + // behavior), so the grandchild keeps the leader's original group alive + // long after the leader itself has exited and been reaped. + const leaderScript = ` + const { spawn } = require("node:child_process"); + const grandchild = spawn(process.execPath, ["-e", "setTimeout(() => {}, 30000)"], { stdio: "ignore" }); + grandchild.unref(); + setTimeout(() => process.exit(0), 200); + `; + const leader = spawn(process.execPath, ["-e", leaderScript], { + detached: true, + stdio: "ignore" + }); + await new Promise((resolve, reject) => { + leader.once("spawn", resolve); + leader.once("error", reject); + }); + const leaderPid = leader.pid; + + try { + const recordedIdentity = getProcessIdentity(leaderPid); + assert.ok(recordedIdentity, "expected a recordable identity while the leader is alive"); + + // Poll for the leader's own exit instead of a fixed sleep: its script + // exits itself after ~200ms, but scheduling jitter under test-suite load + // must not make this flaky. + const deadline = Date.now() + 5000; + while (Date.now() < deadline && isProcessRunning(leaderPid)) { + await new Promise((resolve) => setTimeout(resolve, 25)); + } + assert.equal(isProcessRunning(leaderPid), false, "the leader should have exited and been reaped by now"); + + // The leader is gone, but the grandchild it left behind is still in the + // leader's original process group. A live descendant must still read as + // a running tree, matched against the identity recorded while the + // leader itself was alive. + assert.equal( + isProcessTreeRunning(leaderPid, { identity: recordedIdentity }), + true, + "a surviving descendant must keep the recorded process group alive" + ); + } finally { + // The leader is already dead; -leaderPid still addresses the process + // group as long as at least one member (the grandchild) survives. + try { + process.kill(-leaderPid, "SIGTERM"); + } catch { + // Group may already be gone. + } + let exited = await waitForProcessExit(leaderPid, { timeoutMs: 2000 }); + if (!exited) { + try { + process.kill(-leaderPid, "SIGKILL"); + } catch { + // Already gone between the check above and here. + } + exited = await waitForProcessExit(leaderPid, { timeoutMs: 2000 }); + } + assert.equal(exited, true, "cleanup must confirm the test process exited"); + } +}); From f3204e8db58286a69fe7ae6abc5019a78457a214 Mon Sep 17 00:00:00 2001 From: Heringer Date: Sat, 25 Jul 2026 14:27:35 -0300 Subject: [PATCH 20/20] Reclaim sessions whose dead leader left an orphaned process group The Codex GitHub reviewer flagged the remaining wedge on this branch: when the broker leader dies but its codex app-server child survives in the detached process group, the tree correctly reads as running, yet ownership can only be proven against the dead leader's argv. Shutdown therefore threw "ownership could not be verified" on every invocation until the orphan happened to exit, wedging the workspace. Recognize that state explicitly instead. When the session carries an instance token and the recorded leader is confirmed gone (or provably replaced, per the persisted identity) while its group still has members, shutdown now reclaims the session state without signaling anything: POSIX reserves a leader's pid for as long as its group survives, but a single observation cannot tell the original broker's descendants from a later generation that recycled the same pgid, so the group is deliberately left untouched -- it may not even be idle. Nothing can be accepting on a refused endpoint, so the socket, pid file, log file and broker.json are safe to remove, and the next ensureBrokerSession starts a fresh broker in a fresh session directory. canReclaimStaleEndpoint gains an explicit allowOrphanedTree flag for exactly this caller; legacy sessions keep the full process-group veto and still fail closed. Regression tests cover the reclaim (kill spy proves nothing is ever signaled, the orphan survives, all artifacts are removed), the subsequent ensureBrokerSession with the orphan still alive, and the unchanged legacy behavior; all verified to fail against the previous code. Reported by the Codex GitHub reviewer on this branch. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_018uAqG4qqx6YKWzXjd7YMDG --- .../codex/scripts/lib/broker-lifecycle.mjs | 49 ++++- tests/broker-lifecycle.test.mjs | 189 ++++++++++++++++++ 2 files changed, 232 insertions(+), 6 deletions(-) diff --git a/plugins/codex/scripts/lib/broker-lifecycle.mjs b/plugins/codex/scripts/lib/broker-lifecycle.mjs index 1c8681cf0..978509598 100644 --- a/plugins/codex/scripts/lib/broker-lifecycle.mjs +++ b/plugins/codex/scripts/lib/broker-lifecycle.mjs @@ -332,22 +332,37 @@ function processMatchesLegacyBroker(session, pid, options = {}) { * instead, all of which must hold: * * 1. the socket sits inside the 0700 session directory we created; - * 2. neither the recorded PID nor its process group is still running; + * 2. the recorded leader is absent or replaced (see below for the + * process-group exception); * 3. connecting to the endpoint is refused. * * No single one is sufficient. A hung broker also fails to answer, PID numbers * get reused, and a refused connect only proves nothing is listening *right * now* — a process that has bound but not yet listened also refuses. Requiring * all three keeps the blast radius inside our own temp directory. + * + * `allowOrphanedTree` (default false) relaxes signal 2 from "process group + * gone" to "leader gone": the caller passes it only after already having + * established the abandoned-tokened-orphan state itself (a confirmed-dead or + * -replaced leader whose group still has members we deliberately never + * signal). It is safe here specifically because signal 3 still independently + * rules out any live listener on the endpoint, and a surviving group member + * never knew the socket path to begin with -- it was never handed the + * endpoint, so it cannot be the one refusing or accepting the connection. + * Legacy callers do not set the flag and keep the full process-group veto. */ async function canReclaimStaleEndpoint(session, pid, options = {}) { if (!endpointBelongsToSession(session, options.platform)) { return false; } + if (isValidPid(pid) && isProcessRunning(pid, options)) { + return false; + } // isProcessTreeRunning() checks the process *group* on Linux, so a reused PID - // in another group reads as dead. Pair it with the plain PID check before - // treating the owner as gone. - if (isValidPid(pid) && (isProcessTreeRunning(pid, options) || isProcessRunning(pid, options))) { + // in another group reads as dead. Pair it with the plain PID check above + // before treating the owner as gone -- unless the caller has already + // established the abandoned-orphan exception documented above. + if (!options.allowOrphanedTree && isValidPid(pid) && isProcessTreeRunning(pid, options)) { return false; } return !(await endpointAcceptsConnection(session.endpoint, options.reclaimProbeTimeoutMs)); @@ -389,6 +404,11 @@ async function shutdownBrokerSessionLocked(cwd, options = {}) { ? session.processIdentity : null; const livenessOptions = recordedIdentity != null ? { ...options, identity: recordedIdentity } : options; + // Set when the persisted leader is confirmed gone (or replaced) while its + // process group still has survivors -- an abandoned, tokened orphan whose + // own state we may still reclaim without ever signaling the group. See the + // ownership-check branch below for the full rationale. + let abandonedOrphanedTree = false; const legacySession = Boolean(session.endpoint && !session.instanceToken); let legacyProcessVerified = false; if (legacySession) { @@ -441,7 +461,21 @@ async function shutdownBrokerSessionLocked(cwd, options = {}) { const ownsPersistedProcess = ownsBrokerProcess(session, pid, legacySession, options); if (!ownsPersistedProcess) { if (isProcessTreeRunning(pid, livenessOptions)) { - throw new Error("Codex app-server broker ownership could not be verified; persisted state was preserved."); + if (!session.instanceToken || isProcessRunning(pid, livenessOptions)) { + throw new Error("Codex app-server broker ownership could not be verified; persisted state was preserved."); + } + // The leader is confirmed gone (or provably replaced) while its process + // group still has members. POSIX reserves a leader's pid for as long as + // the group survives, but a single observation cannot tell whether these + // members are the original broker's descendants or a later generation + // that recycled the same pgid after the original group died out -- so + // the group is deliberately never signaled. Reclaiming our own session + // state is still sound: nothing can be accepting on a refused endpoint, + // and the orphaned tree (which may still be finishing in-flight work, + // not necessarily idle) keeps running untouched. The alternative is + // wedging every later invocation in this workspace until the orphan + // exits on its own. + abandonedOrphanedTree = true; } exited = true; } else { @@ -483,7 +517,10 @@ async function shutdownBrokerSessionLocked(cwd, options = {}) { const endpointProven = ownershipVerified || processOwnershipProven; let reclaimedStaleEndpoint = false; if (!endpointProven && endpointArtifactExists(session.endpoint)) { - reclaimedStaleEndpoint = await canReclaimStaleEndpoint(session, pid, livenessOptions); + reclaimedStaleEndpoint = await canReclaimStaleEndpoint(session, pid, { + ...livenessOptions, + allowOrphanedTree: abandonedOrphanedTree + }); if (!reclaimedStaleEndpoint) { throw new Error("Codex app-server broker endpoint ownership could not be verified; persisted state was preserved."); } diff --git a/tests/broker-lifecycle.test.mjs b/tests/broker-lifecycle.test.mjs index 0c1f03baf..f2a047e55 100644 --- a/tests/broker-lifecycle.test.mjs +++ b/tests/broker-lifecycle.test.mjs @@ -18,6 +18,8 @@ import { shutdownBrokerSession } from "../plugins/codex/scripts/lib/broker-lifecycle.mjs"; import { + getProcessIdentity, + isProcessRunning, isProcessTreeRunning, terminateProcessTree, waitForProcessExit @@ -899,3 +901,190 @@ test("ensureBrokerSession preserves session artifacts when the killed child does } } }); + +// Spawns a detached leader (its own process group) that binds the broker +// endpoint socket and spawns a non-detached grandchild -- inheriting the +// leader's process group, per standard POSIX fork/exec -- so the grandchild +// keeps the group alive long after the leader itself is killed. This is the +// exact "abandoned orphaned tree" shape CHANGE 1/2 exist to reclaim from: a +// dead leader whose launch-token proof died with it, but whose process group +// still has live, unrelated-looking members. +async function spawnOrphanLeader(socketPath) { + const leader = spawn( + process.execPath, + [ + "-e", + `const net = require("node:net"); + const { spawn } = require("node:child_process"); + const socketPath = ${JSON.stringify(socketPath)}; + const grandchild = spawn(process.execPath, ["-e", "setTimeout(() => {}, 30000)"], { stdio: "ignore" }); + grandchild.unref(); + grandchild.once("spawn", () => { + const server = net.createServer(); + server.listen(socketPath, () => { + process.stdout.write("ready " + grandchild.pid + "\\n"); + }); + }); + setInterval(() => {}, 60000);` + ], + { detached: true, stdio: ["ignore", "pipe", "ignore"] } + ); + const grandchildPid = await new Promise((resolve, reject) => { + leader.stdout.on("data", (chunk) => { + const match = String(chunk).match(/ready (\d+)/); + if (match) { + resolve(Number(match[1])); + } + }); + leader.once("error", reject); + leader.once("exit", () => reject(new Error("orphan leader exited before binding"))); + }); + return { leader, grandchildPid }; +} + +// Builds the abandoned-orphan fixture and persists a session for it. Passing +// no instanceToken produces a legacy session (the key is omitted entirely, +// matching how genuinely legacy sessions are built elsewhere in this file). +async function setupOrphanedSession(workspace, { instanceToken } = {}) { + const sessionDir = makeTempDir("cxc-orphan-"); + const socketPath = path.join(sessionDir, "broker.sock"); + const pidFile = path.join(sessionDir, "broker.pid"); + + const { leader, grandchildPid } = await spawnOrphanLeader(socketPath); + const leaderPid = leader.pid; + fs.writeFileSync(pidFile, String(leaderPid)); + + // Captured while the leader is alive -- this is what + // ensureBrokerSessionLocked would have persisted at spawn time. + const recordedIdentity = getProcessIdentity(leaderPid); + assert.ok(recordedIdentity, "expected a recordable identity while the leader is alive"); + + const session = { + endpoint: `unix:${socketPath}`, + pid: leaderPid, + pidFile, + logFile: null, + sessionDir, + processIdentity: recordedIdentity, + ...(instanceToken !== undefined ? { instanceToken } : {}) + }; + saveBrokerSession(workspace, session); + + // Kill only the leader: SIGKILL skips its own cleanup, so the socket file + // stays on disk with nothing listening, exactly like a crashed broker. + leader.kill("SIGKILL"); + const deadline = Date.now() + 5000; + while (Date.now() < deadline && isProcessRunning(leaderPid)) { + await new Promise((resolve) => setTimeout(resolve, 25)); + } + assert.equal(isProcessRunning(leaderPid), false, "the leader should have exited by now"); + assert.equal(fs.existsSync(socketPath), true, "the stale socket must survive the leader's death"); + assert.equal(isProcessTreeRunning(leaderPid), true, "the grandchild must keep the process group alive"); + + return { sessionDir, socketPath, pidFile, session, leaderPid, grandchildPid }; +} + +async function cleanupOrphanGroup(leaderPid, grandchildPid) { + try { + process.kill(-leaderPid, "SIGTERM"); + } catch { + // Group may already be gone. + } + // The grandchild, not the (already dead) leader, is what a plain + // isProcessTreeRunning(grandchildPid) cannot see -- it inherited the + // leader's pgid rather than being a group leader itself -- so poll its own + // liveness directly (tree: false) instead. + let exited = await waitForProcessExit(grandchildPid, { timeoutMs: 2000, tree: false }); + if (!exited) { + try { + process.kill(-leaderPid, "SIGKILL"); + } catch { + // Already gone between the check above and here. + } + exited = await waitForProcessExit(grandchildPid, { timeoutMs: 2000, tree: false }); + } + assert.equal(exited, true, "cleanup must confirm the test process exited"); +} + +test("shutdown reclaims a tokened session whose dead leader left an orphaned group", { skip: process.platform === "win32" }, async () => { + const workspace = makeTempDir(); + const { socketPath, pidFile, leaderPid, grandchildPid } = await setupOrphanedSession(workspace, { + instanceToken: "orphan-leader-token" + }); + + let killProcessCalled = false; + const killProcess = () => { + killProcessCalled = true; + }; + + try { + const outcome = await shutdownBrokerSession(workspace, { timeoutMs: 40, killProcess }); + + assert.equal(killProcessCalled, false, "an abandoned orphaned tree must never be signaled"); + assert.deepEqual(outcome, { found: true, exited: true, forced: false, reclaimedStaleEndpoint: true }); + assert.equal(isProcessRunning(grandchildPid), true, "the grandchild must be left running untouched"); + assert.equal(fs.existsSync(socketPath), false, "the stale socket must be removed"); + assert.equal(fs.existsSync(pidFile), false, "the pid file must be removed"); + assert.equal(loadBrokerSession(workspace), null); + } finally { + await cleanupOrphanGroup(leaderPid, grandchildPid); + } +}); + +test("ensureBrokerSession recovers after reclaiming an orphaned group", { skip: process.platform === "win32" }, async () => { + const workspace = makeTempDir(); + const binDir = makeTempDir(); + installFakeCodex(binDir); + + const { + sessionDir: oldSessionDir, + socketPath: oldSocketPath, + session: oldSession, + leaderPid, + grandchildPid + } = await setupOrphanedSession(workspace, { instanceToken: "orphan-leader-token" }); + + try { + const session = await ensureBrokerSession(workspace, { env: buildEnv(binDir) }); + + assert.ok(session, "ensureBrokerSession must recover instead of wedging on the orphaned tree"); + assert.notEqual(session.pid, leaderPid); + assert.notEqual(session.sessionDir, oldSessionDir); + assert.notEqual(session.endpoint, oldSession.endpoint); + + const probe = net.createConnection({ path: session.endpoint.slice("unix:".length) }); + await new Promise((resolve, reject) => { + probe.once("connect", resolve); + probe.once("error", reject); + }); + probe.destroy(); + + await shutdownBrokerSession(workspace, { killProcess: terminateProcessTree }); + assert.equal(loadBrokerSession(workspace), null); + + assert.equal(fs.existsSync(oldSocketPath), false, "the reclaimed stale socket must have been removed"); + assert.equal(isProcessRunning(grandchildPid), true, "the orphaned grandchild must be left running throughout"); + } finally { + await cleanupOrphanGroup(leaderPid, grandchildPid); + } +}); + +test("a legacy orphaned group still preserves state", { skip: process.platform === "win32" }, async () => { + const workspace = makeTempDir(); + // No instanceToken: pins the CHANGE 1/2 relaxation strictly to the tokened + // path -- a legacy session hitting the identical dead-leader/live-group + // shape must still refuse and preserve state. + const { socketPath, leaderPid, grandchildPid } = await setupOrphanedSession(workspace); + + try { + await assert.rejects( + shutdownBrokerSession(workspace, { timeoutMs: 40, killProcess: terminateProcessTree }), + /ownership could not be verified/i + ); + + assert.equal(fs.existsSync(socketPath), true, "the socket must be preserved for a legacy orphaned group"); + assert.ok(loadBrokerSession(workspace), "broker.json must be preserved for a legacy orphaned group"); + } finally { + await cleanupOrphanGroup(leaderPid, grandchildPid); + } +});