diff --git a/plugins/codex/scripts/app-server-broker.mjs b/plugins/codex/scripts/app-server-broker.mjs index 1954274f..5b765a4e 100644 --- a/plugins/codex/scripts/app-server-broker.mjs +++ b/plugins/codex/scripts/app-server-broker.mjs @@ -70,6 +70,10 @@ async function main() { let activeStreamSocket = null; let activeStreamThreadIds = null; const sockets = new Set(); + // Sockets whose request is a broker/shutdown: two racing session-end hooks + // must not count each other as busy clients, or both back off and the idle + // broker leaks with no future event to retire it. + const shutdownRequesters = new Set(); function clearSocketOwnership(socket) { if (activeRequestSocket === socket) { @@ -100,11 +104,15 @@ async function main() { } async function shutdown(server) { + // Stop admitting new clients before anything else: the app-server close + // below can take a while, and a worker admitted during it would have its + // turn torn down despite the busy gate having passed. + const serverClosed = new Promise((resolve) => server.close(resolve)); for (const socket of sockets) { socket.end(); } await appClient.close().catch(() => {}); - await new Promise((resolve) => server.close(resolve)); + await serverClosed; if (listenTarget.kind === "unix" && fs.existsSync(listenTarget.path)) { fs.unlinkSync(listenTarget.path); } @@ -158,6 +166,27 @@ async function main() { } if (message.id !== undefined && message.method === "broker/shutdown") { + shutdownRequesters.add(socket); + // Teardown must be atomic with client admission: a client that + // connected between a session-end guard check and this shutdown + // request must not have the broker killed under it — including a + // worker that is between requests, when the per-request + // serialization variables are momentarily clear. Peer shutdown + // requesters are not work and never count as busy. + let busyWithAnotherConnection = false; + for (const other of sockets) { + if (other !== socket && !other.destroyed && !shutdownRequesters.has(other)) { + busyWithAnotherConnection = true; + break; + } + } + if (busyWithAnotherConnection) { + send(socket, { + id: message.id, + error: buildJsonRpcError(BROKER_BUSY_RPC_CODE, "Shared Codex broker is busy.") + }); + continue; + } send(socket, { id: message.id, result: {} }); await shutdown(server); process.exit(0); @@ -224,11 +253,13 @@ async function main() { socket.on("close", () => { sockets.delete(socket); + shutdownRequesters.delete(socket); clearSocketOwnership(socket); }); socket.on("error", () => { sockets.delete(socket); + shutdownRequesters.delete(socket); clearSocketOwnership(socket); }); }); diff --git a/plugins/codex/scripts/codex-companion.mjs b/plugins/codex/scripts/codex-companion.mjs index 83df468a..cd9064a6 100644 --- a/plugins/codex/scripts/codex-companion.mjs +++ b/plugins/codex/scripts/codex-companion.mjs @@ -24,7 +24,7 @@ import { import { resolveClaudeSessionPath } from "./lib/claude-session-transfer.mjs"; import { readStdinIfPiped } from "./lib/fs.mjs"; import { collectReviewContext, ensureGitRepository, resolveReviewTarget } from "./lib/git.mjs"; -import { binaryAvailable, terminateProcessTree } from "./lib/process.mjs"; +import { binaryAvailable, isPidAlive, terminateProcessTree } from "./lib/process.mjs"; import { loadPromptTemplate, interpolateTemplate } from "./lib/prompts.mjs"; import { generateJobId, @@ -44,7 +44,11 @@ import { } from "./lib/job-control.mjs"; import { appendLogLine, + claimTerminalStatus, createJobLogFile, + readTerminalClaim, + reassertTerminalClaim, + waitForTurnIdentity, createJobProgressUpdater, createJobRecord, createProgressReporter, @@ -68,6 +72,8 @@ const ROOT_DIR = path.resolve(fileURLToPath(new URL("..", import.meta.url))); const REVIEW_SCHEMA = path.join(ROOT_DIR, "schemas", "review-output.schema.json"); const DEFAULT_STATUS_WAIT_TIMEOUT_MS = 240000; const DEFAULT_STATUS_POLL_INTERVAL_MS = 2000; +const CANCEL_TURN_INTERRUPT_TIMEOUT_MS = 5000; +const CANCEL_TURN_IDENTITY_WAIT_MS = 3000; const VALID_REASONING_EFFORTS = new Set(["none", "minimal", "low", "medium", "high", "xhigh"]); const MODEL_ALIASES = new Map([["spark", "gpt-5.3-codex-spark"]]); const STOP_REVIEW_TASK_MARKER = "Run a stop-gate review of the previous Claude turn."; @@ -677,6 +683,10 @@ function spawnDetachedTaskWorker(cwd, jobId) { stdio: "ignore", windowsHide: true }); + // Spawn failures (EMFILE/EAGAIN) surface as an async 'error' event; without + // a listener that becomes an uncaught exception. The synchronous pid check + // in enqueueBackgroundTask reports the failure. + child.on("error", () => {}); child.unref(); return child; } @@ -685,18 +695,47 @@ function enqueueBackgroundTask(cwd, job, request) { const { logFile } = createTrackedProgress(job); appendLogLine(logFile, "Queued for background execution."); - const child = spawnDetachedTaskWorker(cwd, job.id); + // Persist the queued record BEFORE spawning: the job must be visible to + // session-end guards (and to its own worker) from the first instant — + // spawning first leaves a window in which the job has neither a state + // record nor a broker socket, so a racing SessionEnd passes every guard + // and tears the runtime down under the brand-new job. const queuedRecord = { ...job, status: "queued", phase: "queued", - pid: child.pid ?? null, + pid: null, logFile, request }; writeJobFile(job.workspaceRoot, job.id, queuedRecord); upsertJob(job.workspaceRoot, queuedRecord); + const child = spawnDetachedTaskWorker(cwd, job.id); + if (child.pid == null) { + // The spawn failed before the worker ever existed; a queued record with + // no pid would count as active forever and pin the shared broker. + const errorMessage = "Failed to spawn the background task worker."; + const failedPatch = { + status: "failed", + phase: "failed", + pid: null, + completedAt: nowIso(), + errorMessage + }; + writeJobFile(job.workspaceRoot, job.id, { ...queuedRecord, ...failedPatch }); + upsertJob(job.workspaceRoot, { id: job.id, ...failedPatch }); + appendLogLine(logFile, errorMessage); + throw new Error(errorMessage); + } + // Record the worker pid; merge over the freshest stored record in case + // the worker already flipped the job to running. + upsertJob(job.workspaceRoot, { id: job.id, pid: child.pid }); + const storedAfterSpawn = readStoredJob(job.workspaceRoot, job.id); + if (storedAfterSpawn && storedAfterSpawn.status === "queued") { + writeJobFile(job.workspaceRoot, job.id, { ...storedAfterSpawn, pid: child.pid }); + } + return { payload: { jobId: job.id, @@ -970,22 +1009,73 @@ async function handleCancel(argv) { const reference = positionals[0] ?? ""; const { workspaceRoot, job } = resolveCancelableJob(cwd, reference, { env: process.env }); const existing = readStoredJob(workspaceRoot, job.id) ?? {}; - const threadId = existing.threadId ?? job.threadId ?? null; - const turnId = existing.turnId ?? job.turnId ?? null; - - const interrupt = await interruptAppServerTurn(cwd, { threadId, turnId }); - if (interrupt.attempted) { - appendLogLine( - job.logFile, - interrupt.interrupted - ? `Requested Codex turn interrupt for ${turnId} on ${threadId}.` - : `Codex turn interrupt failed${interrupt.detail ? `: ${interrupt.detail}` : "."}` - ); + let threadId = existing.threadId ?? job.threadId ?? null; + let turnId = existing.turnId ?? job.turnId ?? null; + // The state snapshot can carry the queued record's pid: null while the + // worker has since written its real pid to the job file — and the terminal + // writes below null the pid field, so capture the fresher value first. + let workerPid = existing.pid ?? job.pid ?? Number.NaN; + + // Claim the terminal status first: if the worker finished in the meantime, + // its completed/failed record stands and there is nothing left to cancel. + // That race is benign, so report the job's terminal outcome as a normal + // result instead of failing the command — after a brief wait for the + // winner's record write to land, so the reported status is not stale. + let orphanAdopted = false; + const claimOwned = claimTerminalStatus(workspaceRoot, job.id); + if (!claimOwned) { + await new Promise((resolve) => setTimeout(resolve, 200)); + const finished = readStoredJob(workspaceRoot, job.id) ?? job; + const finishedStatus = finished.status ?? "unknown"; + const claimant = readTerminalClaim(workspaceRoot, job.id); + const claimantAlive = claimant?.pid != null && isPidAlive(claimant.pid); + if (finishedStatus !== "queued" && finishedStatus !== "running") { + // The claimant may have died between its terminal job-file write and + // its state.json update; converge the index to the file's terminal + // outcome so the job does not stay listed as running with a dead pid. + reassertTerminalClaim(workspaceRoot, job.id, finished); + const payload = { + jobId: job.id, + status: finishedStatus, + title: job.title, + alreadyFinished: true + }; + outputCommandResult( + payload, + `Job ${job.id} already finished (${finishedStatus}); nothing to cancel.\n`, + options.json + ); + return; + } + if (claimantAlive) { + // The claim owner is still finalizing the job (e.g. the worker is + // writing its completed record); leave it to finish. + const payload = { + jobId: job.id, + status: finishedStatus, + title: job.title, + alreadyFinished: true + }; + outputCommandResult( + payload, + `Job ${job.id} is being finalized (${finishedStatus}); nothing to cancel.\n`, + options.json + ); + return; + } + // The claim is orphaned: its owner died before persisting a terminal + // record. Repair the records (the claim's recorded intent decides + // failed vs cancelled) and proceed with the interrupt and the worker + // kill below — otherwise a hung worker could never be cancelled, + // because every retry would lose the same claim. + workerPid = finished?.pid ?? workerPid; + reassertTerminalClaim(workspaceRoot, job.id, finished); + orphanAdopted = true; } - terminateProcessTree(job.pid ?? Number.NaN); - appendLogLine(job.logFile, "Cancelled by user."); - + // Persist the terminal record before touching the turn or the worker: a + // crash partway through must never leave an interrupted turn behind with no + // recorded outcome. const completedAt = nowIso(); const nextJob = { ...job, @@ -996,29 +1086,112 @@ async function handleCancel(argv) { errorMessage: "Cancelled by user." }; - writeJobFile(workspaceRoot, job.id, { - ...existing, - ...nextJob, - cancelledAt: completedAt - }); - upsertJob(workspaceRoot, { - id: job.id, - status: "cancelled", - phase: "cancelled", - pid: null, - errorMessage: "Cancelled by user.", - completedAt + // An adopted orphan already carries the record the claim's intent calls + // for (failed for a dead worker's own claim); don't overwrite it. + if (!orphanAdopted) { + // Re-read at write time and apply only the terminal fields: the early + // snapshot must not erase a turn identity the worker persisted since + // (the updater de-duplicates ids and would never re-send them). + const freshStored = readStoredJob(workspaceRoot, job.id) ?? existing; + workerPid = freshStored?.pid ?? workerPid; + writeJobFile(workspaceRoot, job.id, { + ...freshStored, + status: "cancelled", + phase: "cancelled", + pid: null, + completedAt, + errorMessage: "Cancelled by user.", + cancelledAt: completedAt + }); + upsertJob(workspaceRoot, { + id: job.id, + status: "cancelled", + phase: "cancelled", + pid: null, + errorMessage: "Cancelled by user.", + completedAt + }); + } + + // A cancel can land before the worker persisted the turn identity; wait + // for it (while the worker is alive to produce it, bounded so a wedged + // worker cannot stall the cancel) rather than skipping the interrupt and + // orphaning the turn. + { + // The wait also refreshes the worker pid: with record-before-spawn the + // snapshot can carry pid null, and the kill below must target the real + // worker, not NaN. + const identity = await waitForTurnIdentity(workspaceRoot, job.id, { + threadId, + turnId, + deadline: Date.now() + CANCEL_TURN_IDENTITY_WAIT_MS, + workerPid: job.pid ?? null + }); + threadId = identity.threadId; + turnId = identity.turnId; + workerPid = identity.workerPid ?? workerPid; + } + + // Bounded like the session-end path: the terminal records are already + // written, so a turn/interrupt that never replies must not hang the + // command before the worker kill below runs — that would strand a live + // worker behind a cancelled record it can no longer overwrite. + const interrupt = await interruptAppServerTurn(cwd, { + threadId, + turnId, + timeoutMs: CANCEL_TURN_INTERRUPT_TIMEOUT_MS }); + let workerKillError = null; + try { + terminateProcessTree(workerPid); + } catch (error) { + // The cancellation is already recorded; a failed kill (EPERM, taskkill + // access denied) must not turn it into a CLI crash with no payload. + workerKillError = error; + } + + // Log appends are best-effort; an unwritable log must not fail the cancel. + try { + if (workerKillError) { + appendLogLine( + job.logFile, + `Worker termination failed: ${workerKillError instanceof Error ? workerKillError.message : String(workerKillError)}` + ); + } + if (interrupt.attempted) { + appendLogLine( + job.logFile, + interrupt.interrupted + ? `Requested Codex turn interrupt for ${turnId} on ${threadId}.` + : `Codex turn interrupt failed${interrupt.detail ? `: ${interrupt.detail}` : "."}` + ); + } + appendLogLine(job.logFile, "Cancelled by user."); + } catch { + // Ignore log write failures after the cancellation is already recorded. + } + // An adopted orphan keeps the status the claim's intent produced (e.g. + // failed for a dead worker's own claim) instead of reporting cancelled. + const effectiveStatus = orphanAdopted + ? readStoredJob(workspaceRoot, job.id)?.status ?? "cancelled" + : "cancelled"; const payload = { jobId: job.id, - status: "cancelled", + status: effectiveStatus, title: job.title, turnInterruptAttempted: interrupt.attempted, - turnInterrupted: interrupt.interrupted + turnInterrupted: interrupt.interrupted, + // A failed kill leaves the worker alive even though the record is + // cancelled; the caller must be able to tell that from a clean cancel. + workerTerminated: workerKillError == null }; - outputCommandResult(payload, renderCancelReport(nextJob), options.json); + outputCommandResult( + payload, + renderCancelReport({ ...nextJob, status: effectiveStatus }, { workerTerminated: workerKillError == null }), + options.json + ); } async function main() { diff --git a/plugins/codex/scripts/lib/app-server.mjs b/plugins/codex/scripts/lib/app-server.mjs index 72b30a76..02358831 100644 --- a/plugins/codex/scripts/lib/app-server.mjs +++ b/plugins/codex/scripts/lib/app-server.mjs @@ -20,7 +20,7 @@ const PLUGIN_MANIFEST_URL = new URL("../../.claude-plugin/plugin.json", import.m const PLUGIN_MANIFEST = JSON.parse(fs.readFileSync(PLUGIN_MANIFEST_URL, "utf8")); export const BROKER_ENDPOINT_ENV = "CODEX_COMPANION_APP_SERVER_ENDPOINT"; -export const BROKER_BUSY_RPC_CODE = -32001; +export { BROKER_BUSY_RPC_CODE } from "./broker-endpoint.mjs"; /** @type {ClientInfo} */ const DEFAULT_CLIENT_INFO = { @@ -265,6 +265,30 @@ class SpawnedCodexAppServerClient extends AppServerClientBase { await this.exitPromise; } + // Immediate teardown for callers that cannot wait for a graceful close + // (e.g. a timed-out request whose response may never come): kill the + // process so the exit handler rejects any pending requests. + destroy() { + this.closed = true; + if (this.readline) { + this.readline.close(); + } + if (this.proc && !this.proc.killed) { + try { + if (process.platform === "win32") { + // With shell: true the direct child is cmd.exe; kill the whole + // tree so the codex app-server grandchild does not survive the + // timeout (mirrors the graceful close() path). + terminateProcessTree(this.proc.pid); + } else { + this.proc.kill("SIGKILL"); + } + } catch { + // Ignore missing process. + } + } + } + sendMessage(message) { const line = `${JSON.stringify(message)}\n`; const stdin = this.proc?.stdin; @@ -322,6 +346,16 @@ class BrokerCodexAppServerClient extends AppServerClientBase { await this.exitPromise; } + // Immediate teardown for callers that cannot wait for a graceful close: a + // half-closed socket with a pending request would stay registered as an + // active request on the broker and keep this process's event loop alive. + destroy() { + this.closed = true; + if (this.socket) { + this.socket.destroy(); + } + } + sendMessage(message) { const line = `${JSON.stringify(message)}\n`; const socket = this.socket; @@ -348,6 +382,10 @@ export class CodexAppServerClient { const client = brokerEndpoint ? new BrokerCodexAppServerClient(cwd, { ...options, brokerEndpoint }) : new SpawnedCodexAppServerClient(cwd, options); + // Hand the instance out before initialize: a caller racing connect + // against a deadline must be able to destroy a client wedged inside + // initialize, whose socket/child would otherwise outlive the timeout. + options.onClientCreated?.(client); await client.initialize(); return client; } diff --git a/plugins/codex/scripts/lib/broker-endpoint.mjs b/plugins/codex/scripts/lib/broker-endpoint.mjs index 8abdcc71..56354633 100644 --- a/plugins/codex/scripts/lib/broker-endpoint.mjs +++ b/plugins/codex/scripts/lib/broker-endpoint.mjs @@ -1,6 +1,8 @@ import path from "node:path"; import process from "node:process"; +export const BROKER_BUSY_RPC_CODE = -32001; + function sanitizePipeName(value) { return String(value ?? "") .replace(/[^A-Za-z0-9._-]/g, "-") diff --git a/plugins/codex/scripts/lib/broker-lifecycle.mjs b/plugins/codex/scripts/lib/broker-lifecycle.mjs index ef763819..244dd78c 100644 --- a/plugins/codex/scripts/lib/broker-lifecycle.mjs +++ b/plugins/codex/scripts/lib/broker-lifecycle.mjs @@ -5,7 +5,7 @@ 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 { BROKER_BUSY_RPC_CODE, createBrokerEndpoint, parseBrokerEndpoint } from "./broker-endpoint.mjs"; import { resolveStateDir } from "./state.mjs"; export const PID_FILE_ENV = "CODEX_COMPANION_APP_SERVER_PID_FILE"; @@ -41,18 +41,57 @@ export async function waitForBrokerEndpoint(endpoint, timeoutMs = 2000) { } export async function sendBrokerShutdown(endpoint) { - await new Promise((resolve) => { + const first = await sendBrokerShutdownOnce(endpoint); + if (!first.refused) { + return first; + } + // Two racing session-end hooks can refuse each other before the broker has + // marked either as a shutdown requester; back off briefly and retry once. + // If the peer's shutdown won, the retry reports unreachable and teardown + // proceeds; a genuinely busy broker refuses again. + await new Promise((resolve) => setTimeout(resolve, 150 + Math.floor(Math.random() * 150))); + return await sendBrokerShutdownOnce(endpoint); +} + +async function sendBrokerShutdownOnce(endpoint) { + return await new Promise((resolve) => { const socket = connectToEndpoint(endpoint); socket.setEncoding("utf8"); + let buffer = ""; + let settled = false; + let connected = false; + const finish = (outcome) => { + if (!settled) { + settled = true; + resolve(outcome); + } + }; socket.on("connect", () => { + connected = true; socket.write(`${JSON.stringify({ id: 1, method: "broker/shutdown", params: {} })}\n`); }); - socket.on("data", () => { + socket.on("data", (chunk) => { + buffer += chunk; + const newlineIndex = buffer.indexOf("\n"); + if (newlineIndex === -1) { + return; + } + // The broker refuses shutdown while another connection is mid-turn, + // so a shutdown that raced a fresh turn admission cannot kill it. + let refused = false; + try { + refused = JSON.parse(buffer.slice(0, newlineIndex))?.error?.code === BROKER_BUSY_RPC_CODE; + } catch { + refused = false; + } socket.end(); - resolve(); + finish({ delivered: !refused, refused, unreachable: false }); }); - socket.on("error", resolve); - socket.on("close", resolve); + // A connection lost mid-exchange is ambiguous: the broker may be alive + // and busy but its refusal reply was lost. Only a failure to connect at + // all marks the broker unreachable (safe to reap). + socket.on("error", () => finish({ delivered: false, refused: false, unreachable: !connected })); + socket.on("close", () => finish({ delivered: false, refused: false, unreachable: !connected })); }); } diff --git a/plugins/codex/scripts/lib/codex.mjs b/plugins/codex/scripts/lib/codex.mjs index fead00cc..2c4dc43e 100644 --- a/plugins/codex/scripts/lib/codex.mjs +++ b/plugins/codex/scripts/lib/codex.mjs @@ -556,7 +556,7 @@ function applyTurnNotification(state, message) { } } -async function captureTurn(client, threadId, startRequest, options = {}) { +export async function captureTurn(client, threadId, startRequest, options = {}) { const state = createTurnCaptureState(threadId, options); const previousHandler = client.notificationHandler; @@ -587,6 +587,13 @@ async function captureTurn(client, threadId, startRequest, options = {}) { state.turnId = response.turn?.id ?? null; if (state.turnId) { state.threadTurnIds.set(state.threadId, state.turnId); + // Persist the turn id from the response itself: the turn/started + // notification can lag or go missing, and cancel/session-end cleanup + // can only interrupt a turn whose id made it into the stored record. + emitProgress(state.onProgress, `Turn accepted (${state.turnId}).`, "starting", { + threadId: state.threadId, + turnId: state.turnId + }); } for (const message of state.bufferedNotifications) { if (belongsToTurn(state, message)) { @@ -603,7 +610,30 @@ async function captureTurn(client, threadId, startRequest, options = {}) { completeTurn(state, response.turn); } - return await state.completion; + // The turn outcome arrives via notifications. If the transport dies first + // (broker shutdown, app-server crash), no `turn/completed` will ever come, + // so fail fast instead of waiting forever on a dead connection. + const connectionClosed = client.exitPromise.then(() => { + // A clean close can outrun the 250 ms inferred-completion timer; if the + // final answer already arrived and no subagent work is pending, the + // turn is done — don't turn a finished run into a transport failure. + if ( + !client.exitError && + !state.completed && + !state.finalTurn && + state.finalAnswerSeen && + state.pendingCollaborations.size === 0 && + state.activeSubagentTurns.size === 0 + ) { + completeTurn(state, null, { inferred: true }); + return state; + } + throw ( + client.exitError ?? + new Error("codex app-server connection closed before the turn completed.") + ); + }); + return await Promise.race([state.completion, connectionClosed]); } finally { clearCompletionTimer(state); client.setNotificationHandler(previousHandler ?? null); @@ -883,7 +913,24 @@ async function getCodexAuthStatusFromClient(client, cwd) { } } +// The probes below spawn the codex binary synchronously (hundreds of ms on +// shimmed installs). Callers that loop — the SessionEnd hook interrupts one +// turn per job under a 5-second hook timeout — must not pay that per call, +// so the result is memoized per cwd for the life of the process. +const codexAvailabilityCache = new Map(); + export function getCodexAvailability(cwd) { + const cacheKey = String(cwd ?? ""); + const cached = codexAvailabilityCache.get(cacheKey); + if (cached) { + return cached; + } + const result = probeCodexAvailability(cwd); + codexAvailabilityCache.set(cacheKey, result); + return result; +} + +function probeCodexAvailability(cwd) { const versionStatus = binaryAvailable("codex", ["--version"], { cwd }); if (!versionStatus.available) { return versionStatus; @@ -957,7 +1004,11 @@ export async function getCodexAuthStatus(cwd, options = {}) { } } -export async function interruptAppServerTurn(cwd, { threadId, turnId }) { +/** + * @param {string} cwd + * @param {{ threadId?: string | null, turnId?: string | null, timeoutMs?: number | null, skipAvailabilityProbe?: boolean }} [options] + */ +export async function interruptAppServerTurn(cwd, { threadId, turnId, timeoutMs = null, skipAvailabilityProbe = false } = {}) { if (!threadId || !turnId) { return { attempted: false, @@ -967,20 +1018,105 @@ export async function interruptAppServerTurn(cwd, { threadId, turnId }) { }; } - const availability = getCodexAvailability(cwd); - if (!availability.available) { - return { - attempted: false, - interrupted: false, - transport: null, - detail: availability.detail - }; + // The probe spawns the codex binary synchronously, outside any deadline; + // deadline-critical callers that already know a broker session exists + // (the SessionEnd hook) skip it — a dead endpoint fails fast in connect. + if (!skipAvailabilityProbe) { + const availability = getCodexAvailability(cwd); + if (!availability.available) { + return { + attempted: false, + interrupted: false, + transport: null, + detail: availability.detail + }; + } } + // The whole attempt — connect and RPC — shares the deadline, and a timeout + // must tear the connection down, not merely abandon the promise: a + // half-closed socket with a pending request stays registered as an active + // request on the broker (blocking a later broker/shutdown) and keeps the + // caller's event loop alive. + const deadline = timeoutMs == null ? null : Date.now() + timeoutMs; + const raceBudget = async (promise) => { + if (deadline == null) { + try { + return { outcome: "ok", value: await promise }; + } catch (error) { + return { outcome: "failed", error }; + } + } + const budget = deadline - Date.now(); + if (budget <= 0) { + // The promise was already created; the finally-destroy will reject it, + // and without a handler that becomes an unhandled rejection that can + // kill the caller (e.g. the SessionEnd hook mid-cleanup). + promise.catch(() => {}); + return { outcome: "timeout" }; + } + let timer = null; + return await Promise.race([ + promise.then( + (value) => ({ outcome: "ok", value }), + (error) => ({ outcome: "failed", error }) + ), + new Promise((resolve) => { + timer = setTimeout(() => resolve({ outcome: "timeout" }), budget); + timer.unref?.(); + }) + ]).finally(() => clearTimeout(timer)); + }; + let client = null; + /** @type {{ destroy: () => void } | null} */ + let pendingClient = null; + let timedOut = false; try { - client = await CodexAppServerClient.connect(cwd, { reuseExistingBroker: true }); - await client.request("turn/interrupt", { threadId, turnId }); + const connectPromise = CodexAppServerClient.connect(cwd, { + reuseExistingBroker: true, + onClientCreated: (created) => { + pendingClient = created; + } + }); + const connected = await raceBudget(connectPromise); + if (connected.outcome === "timeout") { + timedOut = true; + // Destroy the in-progress client immediately: a connect wedged inside + // initialize (endpoint accepts but never answers) would otherwise hold + // its socket or child process past the timeout and keep the caller's + // event loop alive. The late chain is a fallback for a connect that + // resolves after the race. + pendingClient?.destroy(); + connectPromise.then( + (lateClient) => lateClient.destroy(), + () => {} + ); + return { + attempted: true, + interrupted: false, + transport: null, + detail: `Timed out after ${timeoutMs}ms connecting for turn/interrupt.` + }; + } + if (connected.outcome === "failed") { + throw connected.error; + } + client = connected.value; + + const requested = await raceBudget(client.request("turn/interrupt", { threadId, turnId })); + if (requested.outcome === "timeout") { + timedOut = true; + return { + attempted: true, + interrupted: false, + transport: client.transport, + detail: `Timed out after ${timeoutMs}ms waiting for turn/interrupt.` + }; + } + if (requested.outcome === "failed") { + throw requested.error; + } return { attempted: true, interrupted: true, @@ -995,7 +1131,13 @@ export async function interruptAppServerTurn(cwd, { threadId, turnId }) { detail: error instanceof Error ? error.message : String(error) }; } finally { - await client?.close().catch(() => {}); + if (client) { + if (timedOut) { + client.destroy(); + } else { + await client.close().catch(() => {}); + } + } } } diff --git a/plugins/codex/scripts/lib/process.mjs b/plugins/codex/scripts/lib/process.mjs index dd8fc375..112ded8d 100644 --- a/plugins/codex/scripts/lib/process.mjs +++ b/plugins/codex/scripts/lib/process.mjs @@ -50,6 +50,19 @@ export function binaryAvailable(command, versionArgs = ["--version"], options = return { available: true, detail: result.stdout.trim() || result.stderr.trim() || "ok" }; } +export function isPidAlive(pid) { + if (!Number.isFinite(pid)) { + return false; + } + try { + process.kill(pid, 0); + return true; + } catch (error) { + // EPERM means the pid exists but belongs to another user. + return /** @type {NodeJS.ErrnoException} */ (error)?.code === "EPERM"; + } +} + function looksLikeMissingProcessMessage(text) { return /not found|no running instance|cannot find|does not exist|no such process/i.test(text); } diff --git a/plugins/codex/scripts/lib/render.mjs b/plugins/codex/scripts/lib/render.mjs index 2ec18523..cac73b9d 100644 --- a/plugins/codex/scripts/lib/render.mjs +++ b/plugins/codex/scripts/lib/render.mjs @@ -445,11 +445,18 @@ export function renderStoredJobResult(job, storedJob) { return `${lines.join("\n").trimEnd()}\n`; } -export function renderCancelReport(job) { +export function renderCancelReport(job, options = {}) { + // An adopted orphan claim can resolve to a status other than cancelled + // (e.g. failed when the worker died before recording its outcome); the + // text path must report that outcome, not a cancellation. + const headline = + job.status && job.status !== "cancelled" + ? `Cleaned up ${job.id}: recorded as ${job.status} (its previous finalizer died before recording the outcome).` + : `Cancelled ${job.id}.`; const lines = [ "# Codex Cancel", "", - `Cancelled ${job.id}.`, + headline, "" ]; @@ -459,6 +466,9 @@ export function renderCancelReport(job) { if (job.summary) { lines.push(`- Summary: ${job.summary}`); } + if (options.workerTerminated === false) { + lines.push("- Warning: the worker process could not be terminated and may still be running; the job is recorded as cancelled."); + } lines.push("- Check `/codex:status` for the updated queue."); return `${lines.join("\n").trimEnd()}\n`; diff --git a/plugins/codex/scripts/lib/state.mjs b/plugins/codex/scripts/lib/state.mjs index 2da23498..aa3673b9 100644 --- a/plugins/codex/scripts/lib/state.mjs +++ b/plugins/codex/scripts/lib/state.mjs @@ -11,6 +11,7 @@ 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 MIN_TERMINAL_JOBS = 10; function nowIso() { return new Date().toISOString(); @@ -77,10 +78,36 @@ export function loadState(cwd) { } } +// Shared by the pruner, the session-end broker guard, and the dead-worker +// reaper: "what pins the broker" and "what survives pruning" must agree. +export function isActiveJob(job) { + return job.status === "queued" || job.status === "running"; +} + function pruneJobs(jobs) { - return [...jobs] - .sort((left, right) => String(right.updatedAt ?? "").localeCompare(String(left.updatedAt ?? ""))) - .slice(0, MAX_JOBS); + const sorted = [...jobs].sort((left, right) => + String(right.updatedAt ?? "").localeCompare(String(left.updatedAt ?? "")) + ); + if (sorted.length <= MAX_JOBS) { + return sorted; + } + // Never prune active records, however old: dropping one would hide + // in-flight work from the session-end broker guard and delete the running + // worker's files out from under it. Only terminal records age out — and a + // floor keeps the newest terminal records retained even when active jobs + // consume the whole cap, so a job finishing alongside many active peers + // does not vanish the moment it completes. + let terminalBudget = Math.max(MIN_TERMINAL_JOBS, MAX_JOBS - sorted.filter(isActiveJob).length); + return sorted.filter((job) => { + if (isActiveJob(job)) { + return true; + } + if (terminalBudget > 0) { + terminalBudget -= 1; + return true; + } + return false; + }); } function removeFileIfExists(filePath) { @@ -108,6 +135,7 @@ export function saveState(cwd, state) { continue; } removeJobFile(resolveJobFile(cwd, job.id)); + removeFileIfExists(resolveJobClaimFile(cwd, job.id)); removeFileIfExists(job.logFile); } @@ -166,7 +194,13 @@ 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"); + // Write-then-rename so concurrent readers never see a torn record: the + // enqueue rewrites this file (merging the worker pid) at the same moment + // the detached worker's startup reads it, and a truncate-in-place write + // hands that reader invalid JSON. + const tempFile = `${jobFile}.${process.pid}.tmp`; + fs.writeFileSync(tempFile, `${JSON.stringify(payload, null, 2)}\n`, "utf8"); + fs.renameSync(tempFile, jobFile); return jobFile; } @@ -189,3 +223,8 @@ export function resolveJobFile(cwd, jobId) { ensureStateDir(cwd); return path.join(resolveJobsDir(cwd), `${jobId}.json`); } + +export function resolveJobClaimFile(cwd, jobId) { + ensureStateDir(cwd); + return path.join(resolveJobsDir(cwd), `${jobId}.terminal`); +} diff --git a/plugins/codex/scripts/lib/tracked-jobs.mjs b/plugins/codex/scripts/lib/tracked-jobs.mjs index 90286901..de4b3c7e 100644 --- a/plugins/codex/scripts/lib/tracked-jobs.mjs +++ b/plugins/codex/scripts/lib/tracked-jobs.mjs @@ -1,10 +1,13 @@ import fs from "node:fs"; import process from "node:process"; -import { readJobFile, resolveJobFile, resolveJobLogFile, upsertJob, writeJobFile } from "./state.mjs"; +import { isPidAlive } from "./process.mjs"; +import { loadState, readJobFile, resolveJobClaimFile, resolveJobFile, resolveJobLogFile, upsertJob, writeJobFile } from "./state.mjs"; export const SESSION_ID_ENV = "CODEX_COMPANION_SESSION_ID"; +const TERMINAL_JOB_STATUSES = new Set(["completed", "failed", "cancelled"]); + export function nowIso() { return new Date().toISOString(); } @@ -99,18 +102,56 @@ export function createJobProgressUpdater(workspaceRoot, jobId) { return; } - upsertJob(workspaceRoot, patch); - const jobFile = resolveJobFile(workspaceRoot, jobId); - if (!fs.existsSync(jobFile)) { + const storedJob = fs.existsSync(jobFile) ? readJobFile(jobFile) : null; + // A cancel can land while the turn is still streaming notifications; + // a terminal record must not regain a live phase or a fresher updatedAt. + // The claim file appears before the terminal record is written, so it + // covers the window in which the cancelled record itself is not yet + // visible. The turn identity is the exception: cancel and session-end + // can only interrupt the server-side turn with threadId/turnId, and + // this updater may hold the only copy — persist those fields alone. + if ((storedJob && TERMINAL_JOB_STATUSES.has(storedJob.status)) || terminalClaimTaken(workspaceRoot, jobId)) { + const identityPatch = {}; + if (patch.threadId && !storedJob?.threadId) { + identityPatch.threadId = patch.threadId; + } + if (patch.turnId && !storedJob?.turnId) { + identityPatch.turnId = patch.turnId; + } + if (Object.keys(identityPatch).length > 0) { + upsertJob(workspaceRoot, { id: jobId, ...identityPatch }); + // Merge onto a fresh read, not the earlier snapshot: the claimant's + // terminal record may have landed in between, and re-writing the + // stale snapshot would revert it to running. + const freshJob = fs.existsSync(jobFile) ? readJobFile(jobFile) : null; + if (freshJob) { + writeJobFile(workspaceRoot, jobId, { ...freshJob, ...identityPatch }); + } + // Converge if the write above still raced the claimant. + if (terminalClaimTaken(workspaceRoot, jobId)) { + reassertTerminalClaim(workspaceRoot, jobId, freshJob); + } + } return; } - const storedJob = readJobFile(jobFile); - writeJobFile(workspaceRoot, jobId, { - ...storedJob, - ...patch - }); + upsertJob(workspaceRoot, patch); + + if (storedJob) { + writeJobFile(workspaceRoot, jobId, { + ...storedJob, + ...patch + }); + } + + // A cancel may have claimed the terminal status while the writes above + // were in flight, in which case the stale progress write just reverted + // its record; converge back to terminal rather than leaving the stale + // write as the last word. + if (terminalClaimTaken(workspaceRoot, jobId)) { + reassertTerminalClaim(workspaceRoot, jobId, storedJob); + } }; } @@ -139,7 +180,155 @@ function readStoredJobOrNull(workspaceRoot, jobId) { return readJobFile(jobFile); } +function isJobCancelled(workspaceRoot, jobId) { + return readStoredJobOrNull(workspaceRoot, jobId)?.status === "cancelled"; +} + +export function terminalClaimTaken(workspaceRoot, jobId) { + return fs.existsSync(resolveJobClaimFile(workspaceRoot, jobId)); +} + +// A cancellation can land before the worker persisted the turn identity; +// the updater still records threadId/turnId after the claim (job file and +// state index receive it through separate writes), so wait for it — while +// the worker is alive to produce it, and bounded by the caller's deadline — +// rather than skipping the interrupt and orphaning the turn. +export async function waitForTurnIdentity(workspaceRoot, jobId, { threadId = null, turnId = null, deadline, workerPid = null } = {}) { + // The caller's snapshot can predate the worker: with record-before-spawn + // the initial record has pid null, so the pid itself must be refreshed + // from the stores alongside the identity — gating on the stale NaN would + // end the wait after one read while the worker (and its turn) live on. + let pid = workerPid; + const refresh = () => { + const stored = readStoredJobOrNull(workspaceRoot, jobId); + threadId = stored?.threadId ?? threadId; + turnId = stored?.turnId ?? turnId; + pid = stored?.pid ?? pid; + if (!threadId || !turnId || pid == null) { + const indexed = loadState(workspaceRoot).jobs.find((candidate) => candidate.id === jobId); + threadId = indexed?.threadId ?? threadId; + turnId = indexed?.turnId ?? turnId; + pid = pid ?? indexed?.pid ?? null; + } + }; + // Always read once up front: the worker may have persisted the identity + // and then exited — a dead worker must not mean the ids are unread. + refresh(); + while ((!threadId || !turnId) && Date.now() < deadline) { + if (Number.isFinite(pid) && !isPidAlive(pid)) { + // A known-dead worker will never publish more; an unknown pid keeps + // polling — the record may still gain it (bounded by the deadline). + break; + } + await new Promise((resolve) => setTimeout(resolve, 100)); + refresh(); + } + return { threadId, turnId, workerPid: Number.isFinite(pid) ? pid : null }; +} + +export function readTerminalClaim(workspaceRoot, jobId) { + try { + const raw = fs.readFileSync(resolveJobClaimFile(workspaceRoot, jobId), "utf8"); + const [pidToken, intentToken] = raw.trim().split(/\s+/); + const pid = Number.parseInt(pidToken ?? "", 10); + return { + pid: Number.isFinite(pid) ? pid : null, + intent: intentToken ?? "cancel" + }; + } catch { + return null; + } +} + +// A taken claim whose terminal record has not landed yet (or was overwritten +// by a racing non-terminal write) is repaired here: converge the stored +// records back to cancelled. Any racing writer that owns the claim writes its +// own terminal record after claiming, so the later write wins either way and +// both orders end terminal. +export function reassertTerminalClaim(workspaceRoot, jobId, fallbackRecord = null, { force = false } = {}) { + const stored = readStoredJobOrNull(workspaceRoot, jobId) ?? fallbackRecord; + if (stored && TERMINAL_JOB_STATUSES.has(stored.status)) { + // The job file is already terminal, but a racing non-terminal upsert + // (e.g. the worker's running record landing after the cancel's write) + // may have reverted the state.json index; synchronize it. + upsertJob(workspaceRoot, { + id: jobId, + status: stored.status, + phase: stored.phase ?? stored.status, + pid: null, + ...(stored.completedAt ? { completedAt: stored.completedAt } : {}), + ...(stored.errorMessage ? { errorMessage: stored.errorMessage } : {}) + }); + return; + } + const claim = readTerminalClaim(workspaceRoot, jobId); + if (!force && claim?.intent === "worker" && claim.pid != null && isPidAlive(claim.pid)) { + // A live worker is finalizing its own outcome right now; don't preempt + // its completed/failed write with a repair record. Callers that just + // force-killed the claimant pass force: the pid can linger as a zombie + // and would otherwise read as alive. + return; + } + // The claim records who took it: a claim taken for the worker's own + // terminal write (or by the reaper for a dead worker) means the job + // actually ran and its outcome was lost — that is a failure, not a + // cancellation. + const intent = claim?.intent ?? "cancel"; + const terminalStatus = intent === "worker" || intent === "reaper" ? "failed" : "cancelled"; + const patch = { + status: terminalStatus, + phase: terminalStatus, + pid: null, + completedAt: nowIso(), + ...(stored?.errorMessage + ? {} + : { + errorMessage: + terminalStatus === "failed" + ? "Failed: the worker died before recording its outcome." + : "Cancelled: a cancellation was recorded while the job was starting or running." + }) + }; + try { + writeJobFile(workspaceRoot, jobId, { ...(stored ?? { id: jobId }), ...patch }); + } catch { + // Best-effort: the state.json record below is the canonical outcome. + } + upsertJob(workspaceRoot, { id: jobId, ...patch }); +} + +export function claimTerminalStatus(workspaceRoot, jobId, intent = "cancel") { + // First writer wins: creating the claim file atomically decides whether the + // worker's completed/failed write or a cancellation owns the job's terminal + // status. Checking the stored record alone is not enough — a cancellation + // can land between that check and the terminal write. The recorded intent + // lets an orphaned claim be repaired to the right terminal status. + try { + fs.writeFileSync(resolveJobClaimFile(workspaceRoot, jobId), `${process.pid} ${intent}\n`, { flag: "wx" }); + return true; + } catch (error) { + if (error?.code === "EEXIST") { + return false; + } + // If the claim file cannot be created at all, fall back to the previous + // last-writer-wins behavior rather than blocking the terminal write. + return true; + } +} + export async function runTrackedJob(job, runner, options = {}) { + // A cancellation may have been recorded before the worker got this far + // (e.g. cancel raced worker startup); never resurrect a cancelled job. A + // taken claim without a cancelled record (the claimant died or its record + // write hasn't landed) counts the same and is repaired to cancelled. + if (isJobCancelled(job.workspaceRoot, job.id)) { + throw new Error(`Job ${job.id} was cancelled before it started.`); + } + if (terminalClaimTaken(job.workspaceRoot, job.id)) { + reassertTerminalClaim(job.workspaceRoot, job.id, job); + throw new Error(`Job ${job.id} was cancelled before it started.`); + } + const runningRecord = { ...job, status: "running", @@ -150,9 +339,31 @@ export async function runTrackedJob(job, runner, options = {}) { }; writeJobFile(job.workspaceRoot, job.id, runningRecord); upsertJob(job.workspaceRoot, runningRecord); + // A cancel may have claimed the terminal status between the checks above + // and the running writes, which would have just overwritten its cancelled + // record. Re-check after writing: a claim that lands after this point is + // followed by the claimant's own record writes, which land after ours. + if (terminalClaimTaken(job.workspaceRoot, job.id)) { + reassertTerminalClaim(job.workspaceRoot, job.id, runningRecord); + throw new Error(`Job ${job.id} was cancelled before it started.`); + } try { const execution = await runner(); + // Cancellation is terminal: if it was recorded while the turn was + // finishing (cancel awaits turn/interrupt before killing the worker, so + // the turn can complete during that window), keep the cancelled record + // instead of overwriting it with completed/failed. The terminal claim + // closes the remaining gap where the cancel lands after this check but + // before the write below. + if (isJobCancelled(job.workspaceRoot, job.id) || !claimTerminalStatus(job.workspaceRoot, job.id, "worker")) { + // The cancellation owns the terminal status — but if the claimant + // crashed before its record writes landed, backing off here would + // leave both stores at running/stale-pid forever; converge them. + reassertTerminalClaim(job.workspaceRoot, job.id, runningRecord); + appendLogBlock(options.logFile ?? job.logFile ?? null, "Final output (after cancellation)", execution.rendered); + return execution; + } const completionStatus = execution.exitStatus === 0 ? "completed" : "failed"; const completedAt = nowIso(); writeJobFile(job.workspaceRoot, job.id, { @@ -179,8 +390,14 @@ export async function runTrackedJob(job, runner, options = {}) { appendLogBlock(options.logFile ?? job.logFile ?? null, "Final output", execution.rendered); return execution; } catch (error) { - const errorMessage = error instanceof Error ? error.message : String(error); const existing = readStoredJobOrNull(job.workspaceRoot, job.id) ?? runningRecord; + if (existing.status === "cancelled" || !claimTerminalStatus(job.workspaceRoot, job.id, "worker")) { + // Same convergence as the success path: a claim whose record writes + // never landed must not leave running/stale-pid records behind. + reassertTerminalClaim(job.workspaceRoot, job.id, existing); + throw error; + } + const errorMessage = error instanceof Error ? error.message : String(error); const completedAt = nowIso(); writeJobFile(job.workspaceRoot, job.id, { ...existing, diff --git a/plugins/codex/scripts/session-lifecycle-hook.mjs b/plugins/codex/scripts/session-lifecycle-hook.mjs index 778571e6..7d549c02 100644 --- a/plugins/codex/scripts/session-lifecycle-hook.mjs +++ b/plugins/codex/scripts/session-lifecycle-hook.mjs @@ -3,7 +3,7 @@ import fs from "node:fs"; import process from "node:process"; -import { terminateProcessTree } from "./lib/process.mjs"; +import { isPidAlive, terminateProcessTree } from "./lib/process.mjs"; import { BROKER_ENDPOINT_ENV } from "./lib/app-server.mjs"; import { clearBrokerSession, @@ -13,13 +13,57 @@ import { sendBrokerShutdown, teardownBrokerSession } from "./lib/broker-lifecycle.mjs"; -import { loadState, resolveStateFile, saveState } from "./lib/state.mjs"; +import { interruptAppServerTurn } from "./lib/codex.mjs"; +import { isActiveJob, loadState, readJobFile, resolveJobFile, resolveStateFile, updateState, upsertJob, writeJobFile } from "./lib/state.mjs"; +import { claimTerminalStatus, readTerminalClaim, reassertTerminalClaim, waitForTurnIdentity } from "./lib/tracked-jobs.mjs"; import { TRANSCRIPT_PATH_ENV } from "./lib/claude-session-transfer.mjs"; import { resolveWorkspaceRoot } from "./lib/workspace.mjs"; export const SESSION_ID_ENV = "CODEX_COMPANION_SESSION_ID"; const PLUGIN_DATA_ENV = "CLAUDE_PLUGIN_DATA"; +// The SessionEnd hook runs under a 5-second timeout (hooks.json). The +// budgets below must sum comfortably under it: brokered turn interrupts +// (identity waits are capped per job and reserve room for the interrupt +// RPCs themselves), then a short exit grace for killed workers, then the +// shutdown exchange with its one retry. +const TURN_INTERRUPT_BUDGET_MS = 2200; +const TURN_IDENTITY_WAIT_MS = 500; +const TURN_INTERRUPT_RESERVE_MS = 1000; + +// After killing its own workers the hook sends broker/shutdown; a worker +// that is slow to exit still holds its broker socket and would make the +// broker refuse that shutdown. Give workers a short grace to exit, then +// SIGKILL stragglers that ignored SIGTERM (or wedged while finalizing). +const WORKER_EXIT_GRACE_MS = 800; + +// Returns the pids still alive after the grace, having force-killed them. +async function waitForWorkerExits(pids) { + const deadline = Date.now() + WORKER_EXIT_GRACE_MS; + let stragglers = pids.filter((pid) => isPidAlive(pid)); + while (stragglers.length > 0 && Date.now() < deadline) { + await new Promise((resolve) => setTimeout(resolve, 50)); + stragglers = stragglers.filter((pid) => isPidAlive(pid)); + } + if (stragglers.length === 0) { + return []; + } + for (const pid of stragglers) { + try { + process.kill(-pid, "SIGKILL"); + } catch { + try { + process.kill(pid, "SIGKILL"); + } catch { + // Ignore missing process. + } + } + } + // Give the forced kill a beat to release the sockets. + await new Promise((resolve) => setTimeout(resolve, 100)); + return stragglers; +} + function readHookInput() { const raw = fs.readFileSync(0, "utf8").trim(); if (!raw) { @@ -39,7 +83,135 @@ function appendEnvVar(name, value) { fs.appendFileSync(process.env.CLAUDE_ENV_FILE, `export ${name}=${shellEscape(value)}\n`, "utf8"); } -function cleanupSessionJobs(cwd, sessionId) { +// A pid-less active record has no liveness signal at all (current code +// always records a worker pid, but records written by older versions or a +// torn write may not); without a bound such a record would pin the broker +// forever if its session never ran SessionEnd. Reap it once it is a day old. +const ACTIVE_JOB_STALENESS_MS = 24 * 60 * 60 * 1000; + +function isStaleJobRecord(job) { + const reference = job.updatedAt ?? job.createdAt ?? null; + const timestamp = reference ? Date.parse(reference) : Number.NaN; + if (!Number.isFinite(timestamp)) { + // Unparseable timestamps stay conservative: not stale. + return false; + } + return Date.now() - timestamp > ACTIVE_JOB_STALENESS_MS; +} + +// Nothing else transitions the record of a worker that died without its +// SessionEnd ever running (SIGKILL, OOM, reboot): reap it to failed here so +// the broker guard, the pruner, and status queries all agree, instead of a +// zombie active record accumulating forever and — after pid reuse — pinning +// the shared broker indefinitely. +async function reapDeadWorkerJobs(workspaceRoot, { excludeSessionId = null, cwd = null, interruptTurns = false, interruptDeadline = 0 } = {}) { + const stateFile = resolveStateFile(workspaceRoot); + if (!fs.existsSync(stateFile)) { + return; + } + + const jobs = loadState(workspaceRoot).jobs; + // Every interrupt this SessionEnd may still have to send shares one + // deadline: the remaining reap candidates here plus the ending session's + // own active jobs, which cleanupSessionJobs interrupts right after this + // reaper. Each attempt takes an equal share of what remains (own jobs + // stay counted throughout — their turn comes later), so one hung reap + // RPC cannot drain the budget and leave the session's own server-side + // turns running while their records read cancelled. + let jobsAwaitingInterrupt = jobs.filter((job) => isActiveJob(job)).length; + + for (const job of jobs) { + if (!isActiveJob(job)) { + continue; + } + // The ending session's own jobs are cleaned up by cleanupSessionJobs, + // which retains a cancelled record; reaping them to failed first would + // make that cleanup drop them as old terminal jobs — erasing the record + // and its files, the exact "No job found" outcome this PR removes. + if (excludeSessionId && job.sessionId === excludeSessionId) { + continue; + } + jobsAwaitingInterrupt -= 1; + const workerDead = job.pid != null ? !isPidAlive(job.pid) : isStaleJobRecord(job); + if (!workerDead) { + continue; + } + if (!claimTerminalStatus(workspaceRoot, job.id, "reaper")) { + // A claimant died between taking the claim and writing its record (or + // is writing it right now); converge the stores to a terminal state. + reassertTerminalClaim(workspaceRoot, job.id, job); + continue; + } + // The dead worker was only the relay: its server-side turn may still be + // running. Interrupt it before failing the record — once failed, the + // job leaves the active set and nothing else will ever interrupt it. + if (interruptTurns && cwd) { + const identity = await waitForTurnIdentity(workspaceRoot, job.id, { + threadId: job.threadId ?? null, + turnId: job.turnId ?? null, + deadline: 0, + workerPid: Number.NaN + }); + const attemptMs = Math.floor((interruptDeadline - Date.now()) / (jobsAwaitingInterrupt + 1)); + if (identity.threadId && identity.turnId && attemptMs > 0) { + try { + await interruptAppServerTurn(cwd, { + threadId: identity.threadId, + turnId: identity.turnId, + timeoutMs: attemptMs, + skipAvailabilityProbe: true + }); + } catch { + // Best-effort: a failed interrupt must not block the reap. + } + } + } + const completedAt = new Date().toISOString(); + const failedPatch = { + status: "failed", + phase: "failed", + pid: null, + completedAt, + errorMessage: "Failed: the worker process died without recording an outcome." + }; + try { + const jobFile = resolveJobFile(workspaceRoot, job.id); + const storedJob = fs.existsSync(jobFile) ? readJobFile(jobFile) : job; + writeJobFile(workspaceRoot, job.id, { ...storedJob, ...failedPatch }); + } catch { + // Best-effort: the state.json record below is the canonical outcome. + } + upsertJob(workspaceRoot, { id: job.id, ...failedPatch }); + } +} + +function hasActiveJobsFromOtherSessions(workspaceRoot, sessionId) { + const stateFile = resolveStateFile(workspaceRoot); + if (!fs.existsSync(stateFile)) { + return false; + } + + return loadState(workspaceRoot).jobs.some((job) => { + if (!isActiveJob(job)) { + return false; + } + if (sessionId && job.sessionId === sessionId) { + return false; + } + // Both queued and running records carry the worker pid; a dead worker + // (e.g. one that crashed at startup, leaving a permanently queued record) + // must not pin the broker. Liveness is authoritative when a pid exists: + // task runtime is unbounded and updatedAt is not a heartbeat, so a live + // worker must never be expired by record age. Only pid-less records, + // which have no liveness signal, fall back to the staleness bound. + if (job.pid != null) { + return isPidAlive(job.pid); + } + return !isStaleJobRecord(job); + }); +} + +async function cleanupSessionJobs(cwd, sessionId, { interruptTurns = false, interruptDeadline = null } = {}) { if (!cwd || !sessionId) { return; } @@ -50,27 +222,198 @@ function cleanupSessionJobs(cwd, sessionId) { return; } - const state = loadState(workspaceRoot); - const removedJobs = state.jobs.filter((job) => job.sessionId === sessionId); - if (removedJobs.length === 0) { + const sessionJobs = loadState(workspaceRoot).jobs.filter((job) => job.sessionId === sessionId); + if (sessionJobs.length === 0) { return; } - for (const job of removedJobs) { + const completedAt = new Date().toISOString(); + const cancelPatch = { + status: "cancelled", + phase: "cancelled", + pid: null, + completedAt, + errorMessage: "Cancelled: the Claude session ended while the job was still running." + }; + const cancelledIds = new Set(); + const killedPids = []; + const finishingJobs = []; + interruptDeadline = interruptDeadline ?? Date.now() + TURN_INTERRUPT_BUDGET_MS; + // Jobs that have not had their interrupt attempt yet, the current one + // included. Each attempt gets an equal share of whatever budget is left: + // one hung interrupt RPC must not consume the shared deadline and leave + // every later job killed (and recorded cancelled) with its server-side + // turn never interrupted. + let jobsAwaitingInterrupt = sessionJobs.filter(isActiveJob).length; + + for (const job of sessionJobs) { const stillRunning = job.status === "queued" || job.status === "running"; if (!stillRunning) { continue; } + jobsAwaitingInterrupt -= 1; + // The state snapshot can carry the queued record's pid: null while the + // worker has since written its real pid (and turn identity) to the job + // file — and the terminal patches below null the pid field, destroying + // the only copy. Capture the fresher values first. + let workerPid = job.pid ?? null; + let threadId = job.threadId ?? null; + let turnId = job.turnId ?? null; try { - terminateProcessTree(job.pid ?? Number.NaN); + const freshFile = resolveJobFile(workspaceRoot, job.id); + const freshJob = fs.existsSync(freshFile) ? readJobFile(freshFile) : null; + workerPid = freshJob?.pid ?? workerPid; + threadId = freshJob?.threadId ?? threadId; + turnId = freshJob?.turnId ?? turnId; + } catch { + // Keep the snapshot values. + } + // The worker may be recording its own terminal outcome right now; only + // cancel jobs whose terminal status this hook wins — unless the claim is + // orphaned (its owner died before writing a terminal record), in which + // case adopt it and proceed, exactly like the cancel path; skipping + // would let the worker survive session shutdown and pin the broker. + let adoptedOrphan = false; + if (!claimTerminalStatus(workspaceRoot, job.id)) { + const claimant = readTerminalClaim(workspaceRoot, job.id); + if (claimant?.pid != null && isPidAlive(claimant.pid)) { + // A live finalizer is writing the job's outcome — let it finish, + // but its worker must not outlive this hook's broker shutdown: it + // is waited for below and force-killed (with a claim repair) if it + // wedges, or the last session out would leak the broker. + if (Number.isFinite(workerPid)) { + finishingJobs.push({ jobId: job.id, pid: workerPid }); + } + continue; + } + // The claim's recorded intent decides failed vs cancelled; don't + // overwrite the repaired record with a cancellation below. + reassertTerminalClaim(workspaceRoot, job.id, job); + adoptedOrphan = true; + } + // Record-first, like handleCancel: hook timeouts are short and the + // interrupt below is a broker round-trip, so persist the terminal outcome + // immediately after winning the claim. Keeping a terminal record for + // in-flight jobs (instead of erasing them) lets status queries see a + // cause rather than "No job found". + cancelledIds.add(job.id); + if (!adoptedOrphan) { + upsertJob(workspaceRoot, { id: job.id, ...cancelPatch }); + try { + const jobFile = resolveJobFile(workspaceRoot, job.id); + const storedJob = fs.existsSync(jobFile) ? readJobFile(jobFile) : job; + // The worker may have published its pid and turn identity between + // the pre-claim read above and this hook winning the claim — and the + // cancel patch nulls the pid, so this reread holds the last copy. + // Capture it before the write, or the worker is neither interrupted + // nor killed and its open socket pins the broker. + workerPid = storedJob.pid ?? workerPid; + threadId = storedJob.threadId ?? threadId; + turnId = storedJob.turnId ?? turnId; + writeJobFile(workspaceRoot, job.id, { ...storedJob, ...cancelPatch }); + } catch { + // Best-effort: the state.json record above is the canonical outcome. + } + } + // The worker only relays a brokered turn: killing it leaves the turn + // running inside the shared app-server, so interrupt the turn first. + // The interrupts share a deadline well inside the SessionEnd hook + // timeout, so a hung broker RPC cannot starve the worker kills below; + // the timeout lives inside the helper, which tears its connection down + // so an abandoned request cannot linger on the broker or keep this + // hook process alive. + if (interruptTurns && (!threadId || !turnId)) { + // The identity wait is capped per job and always leaves a reserve for + // the interrupt RPCs themselves: one id-less job must not consume the + // shared budget and starve every job's interrupt. + const identityDeadline = Math.min( + interruptDeadline - TURN_INTERRUPT_RESERVE_MS, + Date.now() + TURN_IDENTITY_WAIT_MS + ); + const identity = await waitForTurnIdentity(workspaceRoot, job.id, { + threadId, + turnId, + deadline: identityDeadline, + workerPid + }); + threadId = identity.threadId; + turnId = identity.turnId; + // The wait also refreshes the worker pid: with record-before-spawn + // the snapshot can carry pid null, and the kill below must target + // the real worker, not NaN. + workerPid = identity.workerPid ?? workerPid; + } + if (interruptTurns && threadId && turnId) { + // This job's fair share of the remaining budget: itself plus the jobs + // still waiting behind it. Shares are computed against what actually + // remains, so time an earlier job did not use flows to later ones. + const attemptMs = Math.floor((interruptDeadline - Date.now()) / (jobsAwaitingInterrupt + 1)); + if (attemptMs > 0) { + try { + await interruptAppServerTurn(cwd, { + threadId, + turnId, + timeoutMs: attemptMs, + skipAvailabilityProbe: true + }); + } catch { + // Best-effort: a failed interrupt must not block session cleanup. + } + } + } + try { + terminateProcessTree(workerPid ?? Number.NaN); + if (Number.isFinite(workerPid)) { + killedPids.push(workerPid); + } } catch { // Ignore teardown failures during session shutdown. } } - saveState(workspaceRoot, { - ...state, - jobs: state.jobs.filter((job) => job.sessionId !== sessionId) + // A dying (or wedged-while-finalizing) worker's still-open broker socket + // must not veto the broker shutdown that may follow this cleanup. + if (interruptTurns && (killedPids.length > 0 || finishingJobs.length > 0)) { + const stragglers = await waitForWorkerExits([...killedPids, ...finishingJobs.map((entry) => entry.pid)]); + for (const entry of finishingJobs) { + const jobFile = resolveJobFile(workspaceRoot, entry.jobId); + const stored = fs.existsSync(jobFile) ? readJobFile(jobFile) : null; + const workerWroteOutcome = stored != null && !isActiveJob(stored); + if (workerWroteOutcome && !stragglers.includes(entry.pid)) { + // The finalizer finished its job-file write and exited on its own — + // but it may have died between its two store writes (job file first, + // state.json second), leaving the index still running with a dead + // pid. Converge the index to the file's terminal outcome (a no-op + // when the finalizer got both writes out); the filter below then + // erases it like the session's other finished jobs. + reassertTerminalClaim(workspaceRoot, entry.jobId, stored); + continue; + } + // The finalizer was force-killed mid-write, or died during the grace + // wait without recording an outcome; converge its claim to a terminal + // record so the job does not stay running forever — and retain that + // record below: the user needs the outcome, not "No job found". + reassertTerminalClaim(workspaceRoot, entry.jobId, stored, { force: true }); + cancelledIds.add(entry.jobId); + } + } + + // Drop the session's finished jobs, but keep the records cancelled above + // and any still-active job whose terminal claim its worker won mid-write + // (deleting those files would pull them out from under the live worker). + // Fresh read-modify-write: records other sessions created or updated while + // the interrupts above were awaited must not be clobbered by writing back + // a stale snapshot. + updateState(workspaceRoot, (state) => { + state.jobs = state.jobs.filter((job) => { + if (job.sessionId !== sessionId) { + return true; + } + if (cancelledIds.has(job.id)) { + return true; + } + return isActiveJob(job); + }); }); } @@ -82,6 +425,7 @@ function handleSessionStart(input) { async function handleSessionEnd(input) { const cwd = input.cwd || process.cwd(); + const sessionId = input.session_id || process.env[SESSION_ID_ENV]; const brokerSession = loadBrokerSession(cwd) ?? (process.env[BROKER_ENDPOINT_ENV] @@ -97,11 +441,42 @@ async function handleSessionEnd(input) { const sessionDir = brokerSession?.sessionDir ?? null; const pid = brokerSession?.pid ?? null; + // The reaper's dead-turn interrupts and the cleanup's own interrupts share + // one budget so they cannot stack past the hook timeout. + const interruptTurns = Boolean(brokerEndpoint); + const interruptDeadline = Date.now() + TURN_INTERRUPT_BUDGET_MS; + await reapDeadWorkerJobs(resolveWorkspaceRoot(cwd), { + excludeSessionId: sessionId, + cwd, + interruptTurns, + interruptDeadline + }); + await cleanupSessionJobs(cwd, sessionId, { interruptTurns, interruptDeadline }); + + // The broker and state dir are workspace-shared, not session-owned. If any + // other session still has work in flight, tearing the broker down would + // abort its turn mid-flight, so leave the runtime for the survivors. + if (hasActiveJobsFromOtherSessions(resolveWorkspaceRoot(cwd), sessionId)) { + return; + } + if (brokerEndpoint) { - await sendBrokerShutdown(brokerEndpoint); + const shutdown = await sendBrokerShutdown(brokerEndpoint); + // The broker refuses shutdown while another client is connected: work + // admitted between the guard check above and this request must not be + // killed. Leave the runtime up for it; a later session end retires it. + if (shutdown?.refused) { + return; + } + // An ambiguous outcome (connection lost mid-exchange — the refusal may + // have been dropped) must fail closed: don't kill a possibly-busy + // broker. Only a confirmed shutdown or an unreachable endpoint (nothing + // listening) proceeds to teardown. + if (!shutdown?.delivered && !shutdown?.unreachable) { + return; + } } - cleanupSessionJobs(cwd, input.session_id || process.env[SESSION_ID_ENV]); teardownBrokerSession({ endpoint: brokerEndpoint, pidFile, @@ -110,7 +485,13 @@ async function handleSessionEnd(input) { pid, killProcess: terminateProcessTree }); - clearBrokerSession(cwd); + // Only clear the session record if it still describes the broker that was + // just torn down: on the unreachable path (e.g. a racing peer's shutdown + // won), a surviving session may already have spawned and recorded a new + // broker, and clearing that record would orphan the new broker forever. + if (loadBrokerSession(cwd)?.endpoint === brokerEndpoint) { + clearBrokerSession(cwd); + } } async function main() { diff --git a/tests/capture-turn.test.mjs b/tests/capture-turn.test.mjs new file mode 100644 index 00000000..394ad4db --- /dev/null +++ b/tests/capture-turn.test.mjs @@ -0,0 +1,71 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +import { captureTurn } from "../plugins/codex/scripts/lib/codex.mjs"; + +function makeStubClient() { + const client = { + notificationHandler: null, + setNotificationHandler(handler) { + this.notificationHandler = handler; + }, + exitError: null, + exitPromise: null, + resolveExit: null + }; + client.exitPromise = new Promise((resolve) => { + client.resolveExit = resolve; + }); + return client; +} + +function finalAnswerNotification(threadId, turnId) { + return { + method: "item/completed", + params: { + threadId, + turnId, + item: { type: "agentMessage", id: `msg_${turnId}`, text: "final answer", phase: "final_answer" } + } + }; +} + +test("captureTurn treats a clean close after the final answer as completion, not failure", async () => { + const client = makeStubClient(); + const promise = captureTurn(client, "thr_1", async () => ({ turn: { id: "turn_1", status: "inProgress" } })); + + // Let captureTurn record the turn id before notifications arrive. + await new Promise((resolve) => setImmediate(resolve)); + client.notificationHandler(finalAnswerNotification("thr_1", "turn_1")); + + // The connection closes cleanly before the 250 ms inferred-completion + // timer fires; the already-delivered final answer must win. + client.resolveExit(); + + const state = await promise; + assert.equal(state.completed, true); + assert.equal(state.lastAgentMessage, "final answer"); +}); + +test("captureTurn still fails fast when the connection closes with no final answer", async () => { + const client = makeStubClient(); + const promise = captureTurn(client, "thr_1", async () => ({ turn: { id: "turn_1", status: "inProgress" } })); + + await new Promise((resolve) => setImmediate(resolve)); + client.resolveExit(); + + await assert.rejects(promise, /connection closed before the turn completed/); +}); + +test("captureTurn surfaces the transport error even when the final answer arrived", async () => { + const client = makeStubClient(); + const promise = captureTurn(client, "thr_1", async () => ({ turn: { id: "turn_1", status: "inProgress" } })); + + await new Promise((resolve) => setImmediate(resolve)); + client.notificationHandler(finalAnswerNotification("thr_1", "turn_1")); + + client.exitError = new Error("codex app-server exited unexpectedly (exit 1)."); + client.resolveExit(); + + await assert.rejects(promise, /exited unexpectedly/); +}); diff --git a/tests/fake-codex-fixture.mjs b/tests/fake-codex-fixture.mjs index f83c96a0..b547a688 100644 --- a/tests/fake-codex-fixture.mjs +++ b/tests/fake-codex-fixture.mjs @@ -585,8 +585,10 @@ rl.on("line", (line) => { } ]; - if (BEHAVIOR === "interruptible-slow-task") { - send({ method: "turn/started", params: { threadId: thread.id, turn: buildTurn(turnId) } }); + if (BEHAVIOR === "interruptible-slow-task" || BEHAVIOR === "interrupt-hang" || BEHAVIOR === "no-turn-started") { + if (BEHAVIOR !== "no-turn-started") { + send({ method: "turn/started", params: { threadId: thread.id, turn: buildTurn(turnId) } }); + } const timer = setTimeout(() => { if (!interruptibleTurns.has(turnId)) { return; @@ -600,6 +602,16 @@ rl.on("line", (line) => { send({ method: "turn/completed", params: { threadId: thread.id, turn: buildTurn(turnId, "completed") } }); }, 5000); interruptibleTurns.set(turnId, { threadId: thread.id, timer }); + } else if (BEHAVIOR === "final-answer-then-exit") { + // Emit the final answer but never turn/completed, then close the + // connection well before the inferred-completion timer fires. + send({ method: "turn/started", params: { threadId: thread.id, turn: buildTurn(turnId) } }); + for (const entry of items) { + if (entry && entry.completed) { + send({ method: "item/completed", params: { threadId: thread.id, turnId, item: entry.completed } }); + } + } + setTimeout(() => process.exit(0), 20); } else if (BEHAVIOR === "slow-task") { emitTurnCompletedLater(thread.id, turnId, items, 400); } else { @@ -614,6 +626,11 @@ rl.on("line", (line) => { turnId: message.params.turnId }; saveState(state); + if (BEHAVIOR === "interrupt-hang") { + // Never answer the interrupt: callers must time out and tear + // their connection down instead of waiting forever. + break; + } const pending = interruptibleTurns.get(message.params.turnId); if (pending) { clearTimeout(pending.timer); diff --git a/tests/interrupt-turn.test.mjs b/tests/interrupt-turn.test.mjs new file mode 100644 index 00000000..2dac1651 --- /dev/null +++ b/tests/interrupt-turn.test.mjs @@ -0,0 +1,50 @@ +import net from "node:net"; +import path from "node:path"; +import process from "node:process"; +import test from "node:test"; +import assert from "node:assert/strict"; + +import { installFakeCodex } from "./fake-codex-fixture.mjs"; +import { initGitRepo, makeTempDir } from "./helpers.mjs"; +import { saveBrokerSession } from "../plugins/codex/scripts/lib/broker-lifecycle.mjs"; +import { interruptAppServerTurn } from "../plugins/codex/scripts/lib/codex.mjs"; + +test("interruptAppServerTurn tears down a connect wedged during initialize", async (t) => { + if (process.platform === "win32") { + return; + } + + const repo = makeTempDir(); + initGitRepo(repo); + const binDir = makeTempDir(); + installFakeCodex(binDir); + // The availability probe runs against the test process's own PATH. + process.env.PATH = `${binDir}${path.delimiter}${process.env.PATH}`; + + // A broker that accepts connections but never answers `initialize`. + const sockPath = path.join(makeTempDir(), "wedged-broker.sock"); + const server = net.createServer(() => {}); + await new Promise((resolve) => server.listen(sockPath, resolve)); + t.after(() => server.close()); + + saveBrokerSession(repo, { + endpoint: `unix:${sockPath}`, + pidFile: null, + logFile: null, + sessionDir: null, + pid: null + }); + + const started = Date.now(); + const result = await interruptAppServerTurn(repo, { + threadId: "thr_1", + turnId: "turn_1", + timeoutMs: 500 + }); + + // The timeout must both return promptly and destroy the wedged in-progress + // client — a lingering socket would keep this test process alive forever. + assert.equal(result.interrupted, false); + assert.match(result.detail, /Timed out/); + assert.ok(Date.now() - started < 5000, "the interrupt must return within its budget"); +}); diff --git a/tests/render.test.mjs b/tests/render.test.mjs index ab68038e..dbdee472 100644 --- a/tests/render.test.mjs +++ b/tests/render.test.mjs @@ -1,7 +1,26 @@ import test from "node:test"; import assert from "node:assert/strict"; -import { renderReviewResult, renderStoredJobResult } from "../plugins/codex/scripts/lib/render.mjs"; +import { renderCancelReport, renderReviewResult, renderStoredJobResult } from "../plugins/codex/scripts/lib/render.mjs"; + +test("renderCancelReport reports a non-cancelled adopted outcome instead of a cancel stub", () => { + const failed = renderCancelReport({ id: "task-1", title: "Codex Task", status: "failed" }); + assert.match(failed, /recorded as failed/); + assert.doesNotMatch(failed, /Cancelled task-1\./); + + const cancelled = renderCancelReport({ id: "task-1", title: "Codex Task", status: "cancelled" }); + assert.match(cancelled, /Cancelled task-1\./); +}); + +test("renderCancelReport warns when the worker could not be terminated", () => { + const job = { id: "task-1", title: "Codex Task" }; + + const clean = renderCancelReport(job, { workerTerminated: true }); + assert.doesNotMatch(clean, /Warning/); + + const survived = renderCancelReport(job, { workerTerminated: false }); + assert.match(survived, /Warning: the worker process could not be terminated/); +}); test("renderReviewResult degrades gracefully when JSON is missing required review fields", () => { const output = renderReviewResult( diff --git a/tests/runtime.test.mjs b/tests/runtime.test.mjs index 8f276835..7203b814 100644 --- a/tests/runtime.test.mjs +++ b/tests/runtime.test.mjs @@ -7,6 +7,7 @@ import { fileURLToPath } from "node:url"; import { buildEnv, installFakeCodex } from "./fake-codex-fixture.mjs"; import { initGitRepo, makeTempDir, run } from "./helpers.mjs"; +import { CodexAppServerClient } from "../plugins/codex/scripts/lib/app-server.mjs"; import { loadBrokerSession, saveBrokerSession } from "../plugins/codex/scripts/lib/broker-lifecycle.mjs"; import { resolveStateDir } from "../plugins/codex/scripts/lib/state.mjs"; @@ -890,6 +891,26 @@ test("task can finish after subagent work even if the parent turn/completed even assert.equal(result.stdout, "Handled the requested task.\nTask prompt accepted.\n"); }); +test("task completes when the app server exits right after the final answer", () => { + const repo = makeTempDir(); + const binDir = makeTempDir(); + installFakeCodex(binDir, "final-answer-then-exit"); + initGitRepo(repo); + fs.writeFileSync(path.join(repo, "README.md"), "hello\n"); + run("git", ["add", "README.md"], { cwd: repo }); + run("git", ["commit", "-m", "init"], { cwd: repo }); + + // The connection closes cleanly before the inferred-completion timer fires; + // the already-delivered final answer must win over the transport loss. + const result = run("node", [SCRIPT, "task", "challenge the current design"], { + cwd: repo, + env: buildEnv(binDir) + }); + + assert.equal(result.status, 0, result.stderr); + assert.equal(result.stdout, "Handled the requested task.\nTask prompt accepted.\n"); +}); + test("task using the shared broker still completes when Codex spawns subagents", () => { const repo = makeTempDir(); const binDir = makeTempDir(); @@ -1801,7 +1822,116 @@ test("cancel sends turn interrupt to the shared app-server before killing a brok assert.equal(cleanup.status, 0, cleanup.stderr); }); -test("session end fully cleans up jobs for the ending session", async (t) => { +test("session end interrupts a brokered turn before killing its worker", async (t) => { + const repo = makeTempDir(); + const binDir = makeTempDir(); + const fakeStatePath = path.join(binDir, "fake-codex-state.json"); + installFakeCodex(binDir, "interruptible-slow-task"); + initGitRepo(repo); + fs.writeFileSync(path.join(repo, "README.md"), "hello\n"); + run("git", ["add", "README.md"], { cwd: repo }); + run("git", ["commit", "-m", "init"], { cwd: repo }); + + const env = { ...buildEnv(binDir), CODEX_COMPANION_SESSION_ID: "sess-ending" }; + const launched = run("node", [SCRIPT, "task", "--background", "--json", "investigate the flaky worker timeout"], { + cwd: repo, + env + }); + assert.equal(launched.status, 0, launched.stderr); + const jobId = JSON.parse(launched.stdout).jobId; + assert.ok(jobId); + + const stateDir = resolveStateDir(repo); + const statePath = path.join(stateDir, "state.json"); + const runningJob = await waitFor(() => { + const state = JSON.parse(fs.readFileSync(statePath, "utf8")); + const job = state.jobs.find((candidate) => candidate.id === jobId); + if (job?.status === "running" && job.threadId && job.turnId) { + return job; + } + return null; + }, { timeoutMs: 15000 }); + + if (!loadBrokerSession(repo)) { + run("node", [SCRIPT, "cancel", jobId, "--json"], { cwd: repo, env }); + return; + } + + // Pin the broker with another session's live job: the ending session's + // cleanup is then the only thing that can stop its own turn. + const sleeper = spawn(process.execPath, ["-e", "setInterval(() => {}, 1000)"], { + cwd: repo, + detached: true, + stdio: "ignore" + }); + sleeper.unref(); + t.after(() => { + try { + process.kill(sleeper.pid, "SIGTERM"); + } catch { + // Ignore missing process. + } + }); + + const pinnedState = JSON.parse(fs.readFileSync(statePath, "utf8")); + pinnedState.jobs.push({ + id: "task-other-session", + status: "running", + title: "Codex Task", + sessionId: "sess-other", + pid: sleeper.pid, + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString() + }); + fs.writeFileSync(statePath, `${JSON.stringify(pinnedState, null, 2)}\n`, "utf8"); + + const sessionEnd = run("node", [SESSION_HOOK, "SessionEnd"], { + cwd: repo, + env, + input: JSON.stringify({ + hook_event_name: "SessionEnd", + session_id: "sess-ending", + cwd: repo + }) + }); + assert.equal(sessionEnd.status, 0, sessionEnd.stderr); + + const fakeState = JSON.parse(fs.readFileSync(fakeStatePath, "utf8")); + assert.deepEqual( + fakeState.lastInterrupt, + { + threadId: runningJob.threadId, + turnId: runningJob.turnId + }, + "session end must interrupt the brokered turn, not just kill the relay worker" + ); + + const afterState = JSON.parse(fs.readFileSync(statePath, "utf8")); + const cancelledJob = afterState.jobs.find((candidate) => candidate.id === jobId); + assert.equal(cancelledJob.status, "cancelled"); + + assert.ok(loadBrokerSession(repo), "the broker must survive for the other session"); + + // Tear the shared broker down for cleanup. + fs.writeFileSync( + statePath, + `${JSON.stringify({ version: 1, config: { stopReviewGate: false }, jobs: [] }, null, 2)}\n`, + "utf8" + ); + const finalEnd = run("node", [SESSION_HOOK, "SessionEnd"], { + cwd: repo, + env, + input: JSON.stringify({ + hook_event_name: "SessionEnd", + session_id: "sess-other", + cwd: repo + }) + }); + assert.equal(finalEnd.status, 0, finalEnd.stderr); + assert.equal(loadBrokerSession(repo), null); +}); + +test("session end removes finished jobs and records a terminal state for running ones", async (t) => { const repo = makeTempDir(); initGitRepo(repo); fs.writeFileSync(path.join(repo, "README.md"), "hello\n"); @@ -1903,10 +2033,8 @@ test("session end fully cleans up jobs for the ending session", async (t) => { assert.equal(result.status, 0, result.stderr); assert.equal(fs.existsSync(otherSessionLog), true); assert.equal(fs.existsSync(otherJobFile), true); - assert.deepEqual( - fs.readdirSync(path.dirname(otherJobFile)).sort(), - [path.basename(otherJobFile), path.basename(otherSessionLog)].sort() - ); + assert.equal(fs.existsSync(completedLog), false); + assert.equal(fs.existsSync(completedJobFile), false); await waitFor(() => { try { @@ -1918,11 +2046,2223 @@ test("session end fully cleans up jobs for the ending session", async (t) => { }); const state = JSON.parse(fs.readFileSync(path.join(stateDir, "state.json"), "utf8")); - assert.deepEqual(state.jobs.map((job) => job.id), ["review-other"]); - const otherJob = state.jobs[0]; + assert.deepEqual(state.jobs.map((job) => job.id).sort(), ["review-other", "review-running"]); + + const cancelledJob = state.jobs.find((job) => job.id === "review-running"); + assert.equal(cancelledJob.status, "cancelled"); + assert.equal(cancelledJob.pid, null); + assert.ok(cancelledJob.completedAt); + assert.match(cancelledJob.errorMessage, /session ended/i); + assert.equal(fs.existsSync(runningLog), true); + const storedCancelled = JSON.parse(fs.readFileSync(runningJobFile, "utf8")); + assert.equal(storedCancelled.status, "cancelled"); + + const otherJob = state.jobs.find((job) => job.id === "review-other"); assert.equal(otherJob.logFile, otherSessionLog); }); +test("session end leaves a job intact when its worker already claimed the terminal status", () => { + const repo = makeTempDir(); + initGitRepo(repo); + + const stateDir = resolveStateDir(repo); + const jobsDir = path.join(stateDir, "jobs"); + fs.mkdirSync(jobsDir, { recursive: true }); + + const jobId = "task-finishing"; + const jobFile = path.join(jobsDir, `${jobId}.json`); + const logFile = path.join(jobsDir, `${jobId}.log`); + const claimFile = path.join(jobsDir, `${jobId}.terminal`); + fs.writeFileSync(jobFile, JSON.stringify({ id: jobId, status: "running" }, null, 2), "utf8"); + fs.writeFileSync(logFile, "running\n", "utf8"); + // The worker has just won the terminal claim and is mid-way through + // writing its completed record; the claimant must be a live pid, or the + // hook would rightly adopt the claim as orphaned. + fs.writeFileSync(claimFile, `${process.pid} worker\n`, "utf8"); + + fs.writeFileSync( + path.join(stateDir, "state.json"), + `${JSON.stringify( + { + version: 1, + config: { stopReviewGate: false }, + jobs: [ + { + id: jobId, + status: "running", + title: "Codex Task", + sessionId: "sess-current", + logFile, + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString() + } + ] + }, + null, + 2 + )}\n`, + "utf8" + ); + + const result = run("node", [SESSION_HOOK, "SessionEnd"], { + cwd: repo, + input: JSON.stringify({ + hook_event_name: "SessionEnd", + session_id: "sess-current", + cwd: repo + }) + }); + assert.equal(result.status, 0, result.stderr); + + // The job must not be cancelled, dropped, or have its files deleted out + // from under the live worker that is finishing it. + assert.equal(fs.existsSync(jobFile), true); + assert.equal(fs.existsSync(logFile), true); + assert.equal(fs.existsSync(claimFile), true); + const state = JSON.parse(fs.readFileSync(path.join(stateDir, "state.json"), "utf8")); + const job = state.jobs.find((candidate) => candidate.id === jobId); + assert.equal(job?.status, "running"); +}); + +test("broker refuses shutdown while another connection is mid-turn", async (t) => { + const repo = makeTempDir(); + const binDir = makeTempDir(); + installFakeCodex(binDir, "interruptible-slow-task"); + initGitRepo(repo); + fs.writeFileSync(path.join(repo, "README.md"), "hello\n"); + run("git", ["add", "README.md"], { cwd: repo }); + run("git", ["commit", "-m", "init"], { cwd: repo }); + fs.writeFileSync(path.join(repo, "README.md"), "hello again\n"); + + const env = buildEnv(binDir); + const review = run("node", [SCRIPT, "review"], { cwd: repo, env }); + assert.equal(review.status, 0, review.stderr); + + const brokerSession = loadBrokerSession(repo); + if (!brokerSession) { + return; + } + t.after(() => { + const surviving = loadBrokerSession(repo); + if (surviving?.pid) { + try { + process.kill(surviving.pid, "SIGTERM"); + } catch { + // Ignore missing process. + } + } + }); + + // Start a turn directly on the broker, simulating a job admitted after a + // session-end guard check: the turn stays in flight for several seconds. + const client = await CodexAppServerClient.connect(repo, { brokerEndpoint: brokerSession.endpoint }); + const thread = await client.request("thread/start", { cwd: repo }); + const turn = await client.request("turn/start", { + threadId: thread.thread.id, + input: [{ type: "text", text: "investigate the flaky worker timeout" }] + }); + + // A session end racing that admission must not kill the in-flight turn. + const sessionEnd = run("node", [SESSION_HOOK, "SessionEnd"], { + cwd: repo, + env, + input: JSON.stringify({ + hook_event_name: "SessionEnd", + session_id: "sess-ending", + cwd: repo + }) + }); + assert.equal(sessionEnd.status, 0, sessionEnd.stderr); + + const surviving = loadBrokerSession(repo); + assert.ok(surviving, "broker session record must survive a shutdown raced by a live turn"); + assert.doesNotThrow(() => process.kill(surviving.pid, 0), "broker process must still be alive"); + + // Finish the turn. Even between requests (no active turn), a connected + // client must keep refusing shutdown — the serialization variables being + // momentarily clear is not the same as no client being admitted. + await client.request("turn/interrupt", { threadId: thread.thread.id, turnId: turn.turn.id }); + + const idleEnd = run("node", [SESSION_HOOK, "SessionEnd"], { + cwd: repo, + env, + input: JSON.stringify({ + hook_event_name: "SessionEnd", + session_id: "sess-ending", + cwd: repo + }) + }); + assert.equal(idleEnd.status, 0, idleEnd.stderr); + assert.ok(loadBrokerSession(repo), "an idle but connected client must still block shutdown"); + + await client.close(); + + const finalEnd = run("node", [SESSION_HOOK, "SessionEnd"], { + cwd: repo, + env, + input: JSON.stringify({ + hook_event_name: "SessionEnd", + session_id: "sess-ending", + cwd: repo + }) + }); + assert.equal(finalEnd.status, 0, finalEnd.stderr); + assert.equal(loadBrokerSession(repo), null); +}); + +test("session end escalates so its own dying worker cannot veto teardown", async (t) => { + if (process.platform === "win32") { + return; + } + const repo = makeTempDir(); + const binDir = makeTempDir(); + installFakeCodex(binDir); + initGitRepo(repo); + fs.writeFileSync(path.join(repo, "README.md"), "hello\n"); + run("git", ["add", "README.md"], { cwd: repo }); + run("git", ["commit", "-m", "init"], { cwd: repo }); + fs.writeFileSync(path.join(repo, "README.md"), "hello again\n"); + + const env = buildEnv(binDir); + const review = run("node", [SCRIPT, "review"], { cwd: repo, env }); + assert.equal(review.status, 0, review.stderr); + + const brokerSession = loadBrokerSession(repo); + if (!brokerSession) { + return; + } + const socketPath = brokerSession.endpoint.replace(/^unix:/, ""); + + // A worker that ignores SIGTERM while holding a live broker connection: + // its lingering socket must not make the broker refuse the shutdown that + // follows the session's own cleanup. + const stubborn = spawn( + process.execPath, + [ + "-e", + `const net = require("node:net"); process.on("SIGTERM", () => {}); net.createConnection({ path: ${JSON.stringify(socketPath)} }); setInterval(() => {}, 1000);` + ], + { cwd: repo, detached: true, stdio: "ignore" } + ); + stubborn.unref(); + t.after(() => { + try { + process.kill(-stubborn.pid, "SIGKILL"); + } catch { + try { + process.kill(stubborn.pid, "SIGKILL"); + } catch { + // Ignore missing process. + } + } + const surviving = loadBrokerSession(repo); + if (surviving?.pid) { + try { + process.kill(surviving.pid, "SIGTERM"); + } catch { + // Ignore missing process. + } + } + }); + // Give the stubborn worker a beat to connect to the broker. + await new Promise((resolve) => setTimeout(resolve, 500)); + + const stateDir = resolveStateDir(repo); + fs.writeFileSync( + path.join(stateDir, "state.json"), + `${JSON.stringify( + { + version: 1, + config: { stopReviewGate: false }, + jobs: [ + { + id: "task-stubborn", + status: "running", + title: "Codex Task", + sessionId: "sess-current", + pid: stubborn.pid, + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString() + } + ] + }, + null, + 2 + )}\n`, + "utf8" + ); + + const result = run("node", [SESSION_HOOK, "SessionEnd"], { + cwd: repo, + env, + input: JSON.stringify({ + hook_event_name: "SessionEnd", + session_id: "sess-current", + cwd: repo + }) + }); + assert.equal(result.status, 0, result.stderr); + assert.equal(loadBrokerSession(repo), null, "the dying worker must not veto the final teardown"); +}); + +test("session end escalates a wedged finalizer so it cannot veto teardown", async (t) => { + if (process.platform === "win32") { + return; + } + const repo = makeTempDir(); + const binDir = makeTempDir(); + installFakeCodex(binDir); + initGitRepo(repo); + fs.writeFileSync(path.join(repo, "README.md"), "hello\n"); + run("git", ["add", "README.md"], { cwd: repo }); + run("git", ["commit", "-m", "init"], { cwd: repo }); + fs.writeFileSync(path.join(repo, "README.md"), "hello again\n"); + + const env = buildEnv(binDir); + const review = run("node", [SCRIPT, "review"], { cwd: repo, env }); + assert.equal(review.status, 0, review.stderr); + + const brokerSession = loadBrokerSession(repo); + if (!brokerSession) { + return; + } + const socketPath = brokerSession.endpoint.replace(/^unix:/, ""); + + // A worker that claimed its own terminal status (live finalizer) but then + // wedged: ignores SIGTERM and holds a broker connection. The last session + // out must still be able to tear the broker down. + const wedged = spawn( + process.execPath, + [ + "-e", + `const net = require("node:net"); process.on("SIGTERM", () => {}); net.createConnection({ path: ${JSON.stringify(socketPath)} }); setInterval(() => {}, 1000);` + ], + { cwd: repo, detached: true, stdio: "ignore" } + ); + wedged.unref(); + t.after(() => { + try { + process.kill(-wedged.pid, "SIGKILL"); + } catch { + try { + process.kill(wedged.pid, "SIGKILL"); + } catch { + // Ignore missing process. + } + } + const surviving = loadBrokerSession(repo); + if (surviving?.pid) { + try { + process.kill(surviving.pid, "SIGTERM"); + } catch { + // Ignore missing process. + } + } + }); + await new Promise((resolve) => setTimeout(resolve, 500)); + + const stateDir = resolveStateDir(repo); + const jobId = "task-wedged-finalizer"; + fs.mkdirSync(path.join(stateDir, "jobs"), { recursive: true }); + fs.writeFileSync( + path.join(stateDir, "jobs", `${jobId}.json`), + JSON.stringify({ id: jobId, status: "running", pid: wedged.pid }, null, 2), + "utf8" + ); + fs.writeFileSync(path.join(stateDir, "jobs", `${jobId}.terminal`), `${wedged.pid} worker\n`, "utf8"); + fs.writeFileSync( + path.join(stateDir, "state.json"), + `${JSON.stringify( + { + version: 1, + config: { stopReviewGate: false }, + jobs: [ + { + id: jobId, + status: "running", + title: "Codex Task", + sessionId: "sess-current", + pid: wedged.pid, + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString() + } + ] + }, + null, + 2 + )}\n`, + "utf8" + ); + + const result = run("node", [SESSION_HOOK, "SessionEnd"], { + cwd: repo, + env, + input: JSON.stringify({ + hook_event_name: "SessionEnd", + session_id: "sess-current", + cwd: repo + }) + }); + assert.equal(result.status, 0, result.stderr); + assert.equal(loadBrokerSession(repo), null, "a wedged finalizer must not veto the final teardown"); + + // The force-killed finalizer's repaired record must be retained, not + // erased as an ordinary finished job. + const state = JSON.parse(fs.readFileSync(path.join(stateDir, "state.json"), "utf8")); + const repaired = state.jobs.find((candidate) => candidate.id === jobId); + assert.equal(repaired?.status, "failed", "the forced-kill outcome must stay visible"); +}); + +test("session end kills a worker whose pid lives only in the job file", async (t) => { + const repo = makeTempDir(); + initGitRepo(repo); + + const stateDir = resolveStateDir(repo); + fs.mkdirSync(path.join(stateDir, "jobs"), { recursive: true }); + + const sleeper = spawn(process.execPath, ["-e", "setInterval(() => {}, 1000)"], { + cwd: repo, + detached: true, + stdio: "ignore" + }); + sleeper.unref(); + t.after(() => { + try { + process.kill(sleeper.pid, "SIGTERM"); + } catch { + // Ignore missing process. + } + }); + + const jobId = "task-pid-in-file-only"; + // Record-before-spawn: the state index still has the queued pid: null + // snapshot, while the worker has since written its real pid to the job + // file. The terminal patch must not destroy that only copy before the + // kill reads it. + fs.writeFileSync( + path.join(stateDir, "jobs", `${jobId}.json`), + JSON.stringify({ id: jobId, status: "running", pid: sleeper.pid }, null, 2), + "utf8" + ); + fs.writeFileSync( + path.join(stateDir, "state.json"), + `${JSON.stringify( + { + version: 1, + config: { stopReviewGate: false }, + jobs: [ + { + id: jobId, + status: "queued", + title: "Codex Task", + sessionId: "sess-current", + pid: null, + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString() + } + ] + }, + null, + 2 + )}\n`, + "utf8" + ); + + const result = run("node", [SESSION_HOOK, "SessionEnd"], { + cwd: repo, + input: JSON.stringify({ + hook_event_name: "SessionEnd", + session_id: "sess-current", + cwd: repo + }) + }); + assert.equal(result.status, 0, result.stderr); + + await waitFor(() => { + try { + process.kill(sleeper.pid, 0); + return false; + } catch (error) { + return error?.code === "ESRCH"; + } + }); + + const state = JSON.parse(fs.readFileSync(path.join(stateDir, "state.json"), "utf8")); + assert.equal(state.jobs.find((candidate) => candidate.id === jobId)?.status, "cancelled"); +}); + +test("session end repairs a finalizer that dies during the grace wait", async (t) => { + if (process.platform === "win32") { + return; + } + const repo = makeTempDir(); + const binDir = makeTempDir(); + installFakeCodex(binDir); + initGitRepo(repo); + fs.writeFileSync(path.join(repo, "README.md"), "hello\n"); + run("git", ["add", "README.md"], { cwd: repo }); + run("git", ["commit", "-m", "init"], { cwd: repo }); + fs.writeFileSync(path.join(repo, "README.md"), "hello again\n"); + + const env = buildEnv(binDir); + const review = run("node", [SCRIPT, "review"], { cwd: repo, env }); + assert.equal(review.status, 0, review.stderr); + if (!loadBrokerSession(repo)) { + return; + } + t.after(() => { + const surviving = loadBrokerSession(repo); + if (surviving?.pid) { + try { + process.kill(surviving.pid, "SIGTERM"); + } catch { + // Ignore missing process. + } + } + }); + + // A finalizer that dies on its own ~500ms in — during the hook's grace + // wait — without ever writing its terminal record. + const dying = spawn( + process.execPath, + ["-e", "setTimeout(() => process.exit(0), 500); setInterval(() => {}, 100);"], + { cwd: repo, detached: true, stdio: "ignore" } + ); + dying.unref(); + + const stateDir = resolveStateDir(repo); + const jobId = "task-dying-finalizer"; + fs.mkdirSync(path.join(stateDir, "jobs"), { recursive: true }); + fs.writeFileSync( + path.join(stateDir, "jobs", `${jobId}.json`), + JSON.stringify({ id: jobId, status: "running", pid: dying.pid }, null, 2), + "utf8" + ); + fs.writeFileSync(path.join(stateDir, "jobs", `${jobId}.terminal`), `${dying.pid} worker\n`, "utf8"); + fs.writeFileSync( + path.join(stateDir, "state.json"), + `${JSON.stringify( + { + version: 1, + config: { stopReviewGate: false }, + jobs: [ + { + id: jobId, + status: "running", + title: "Codex Task", + sessionId: "sess-current", + pid: dying.pid, + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString() + } + ] + }, + null, + 2 + )}\n`, + "utf8" + ); + + const result = run("node", [SESSION_HOOK, "SessionEnd"], { + cwd: repo, + env, + input: JSON.stringify({ + hook_event_name: "SessionEnd", + session_id: "sess-current", + cwd: repo + }) + }); + assert.equal(result.status, 0, result.stderr); + + // Whether the hook saw the finalizer alive (grace-wait death) or already + // dead (orphan adoption), the record must end terminal and retained. + const state = JSON.parse(fs.readFileSync(path.join(stateDir, "state.json"), "utf8")); + assert.equal(state.jobs.find((candidate) => candidate.id === jobId)?.status, "failed"); + assert.equal(loadBrokerSession(repo), null); +}); + +test("racing shutdown requesters do not deadlock an idle broker", async (t) => { + const repo = makeTempDir(); + const binDir = makeTempDir(); + installFakeCodex(binDir); + initGitRepo(repo); + fs.writeFileSync(path.join(repo, "README.md"), "hello\n"); + run("git", ["add", "README.md"], { cwd: repo }); + run("git", ["commit", "-m", "init"], { cwd: repo }); + fs.writeFileSync(path.join(repo, "README.md"), "hello again\n"); + + const env = buildEnv(binDir); + const review = run("node", [SCRIPT, "review"], { cwd: repo, env }); + assert.equal(review.status, 0, review.stderr); + + const brokerSession = loadBrokerSession(repo); + if (!brokerSession) { + return; + } + t.after(() => { + if (brokerSession.pid) { + try { + process.kill(brokerSession.pid, "SIGTERM"); + } catch { + // Ignore missing process. + } + } + }); + + // Two session-end hooks racing after all jobs are done: each holds a + // shutdown connection. They must not refuse each other into a deadlock + // that leaks the idle broker. + const clientA = await CodexAppServerClient.connect(repo, { brokerEndpoint: brokerSession.endpoint }); + const clientB = await CodexAppServerClient.connect(repo, { brokerEndpoint: brokerSession.endpoint }); + + // B is refused: A is connected and not yet known to be a peer shutdown. + await assert.rejects(clientB.request("broker/shutdown", {}), /busy/i); + + // A must now succeed: B is a marked peer shutdown requester, not work. + await clientA.request("broker/shutdown", {}); + await clientA.close().catch(() => {}); + await clientB.close().catch(() => {}); + + // Assert the shutdown's observable filesystem effect rather than pid + // liveness: in a container without an init reaper the exited detached + // broker lingers as a zombie and kill(pid, 0) keeps succeeding. Windows + // pipes are never unlinked, so fall back to the pid probe there. + if (process.platform === "win32") { + await waitFor(() => { + try { + process.kill(brokerSession.pid, 0); + return false; + } catch (error) { + return error?.code === "ESRCH"; + } + }, { timeoutMs: 10000 }); + } else { + const socketPath = brokerSession.endpoint.replace(/^unix:/, ""); + await waitFor(() => !fs.existsSync(socketPath), { timeoutMs: 10000 }); + } +}); + +test("session end never expires a live worker by record age", async (t) => { + const repo = makeTempDir(); + const binDir = makeTempDir(); + installFakeCodex(binDir); + initGitRepo(repo); + fs.writeFileSync(path.join(repo, "README.md"), "hello\n"); + run("git", ["add", "README.md"], { cwd: repo }); + run("git", ["commit", "-m", "init"], { cwd: repo }); + fs.writeFileSync(path.join(repo, "README.md"), "hello again\n"); + + const env = buildEnv(binDir); + const review = run("node", [SCRIPT, "review"], { cwd: repo, env }); + assert.equal(review.status, 0, review.stderr); + + if (!loadBrokerSession(repo)) { + return; + } + + // Task runtime is unbounded and updatedAt is not a heartbeat: a worker + // that is demonstrably alive must pin the broker however old its record. + const sleeper = spawn(process.execPath, ["-e", "setInterval(() => {}, 1000)"], { + cwd: repo, + detached: true, + stdio: "ignore" + }); + sleeper.unref(); + t.after(() => { + try { + process.kill(sleeper.pid, "SIGTERM"); + } catch { + // Ignore missing process. + } + const surviving = loadBrokerSession(repo); + if (surviving?.pid) { + try { + process.kill(surviving.pid, "SIGTERM"); + } catch { + // Ignore missing process. + } + } + }); + + const staleTimestamp = new Date(Date.now() - 48 * 60 * 60 * 1000).toISOString(); + const stateDir = resolveStateDir(repo); + fs.writeFileSync( + path.join(stateDir, "state.json"), + `${JSON.stringify( + { + version: 1, + config: { stopReviewGate: false }, + jobs: [ + { + id: "task-long-running", + status: "running", + title: "Codex Task", + sessionId: "sess-other", + pid: sleeper.pid, + createdAt: staleTimestamp, + updatedAt: staleTimestamp + } + ] + }, + null, + 2 + )}\n`, + "utf8" + ); + + const result = run("node", [SESSION_HOOK, "SessionEnd"], { + cwd: repo, + env, + input: JSON.stringify({ + hook_event_name: "SessionEnd", + session_id: "sess-current", + cwd: repo + }) + }); + assert.equal(result.status, 0, result.stderr); + assert.ok(loadBrokerSession(repo), "a live worker must pin the broker regardless of record age"); +}); + +test("session end completes and tears down even when a turn interrupt hangs", async (t) => { + const repo = makeTempDir(); + const binDir = makeTempDir(); + installFakeCodex(binDir, "interrupt-hang"); + initGitRepo(repo); + fs.writeFileSync(path.join(repo, "README.md"), "hello\n"); + run("git", ["add", "README.md"], { cwd: repo }); + run("git", ["commit", "-m", "init"], { cwd: repo }); + + const env = { ...buildEnv(binDir), CODEX_COMPANION_SESSION_ID: "sess-hang" }; + const launched = run("node", [SCRIPT, "task", "--background", "--json", "investigate the flaky worker timeout"], { + cwd: repo, + env + }); + assert.equal(launched.status, 0, launched.stderr); + const jobId = JSON.parse(launched.stdout).jobId; + + const stateDir = resolveStateDir(repo); + const statePath = path.join(stateDir, "state.json"); + await waitFor(() => { + const state = JSON.parse(fs.readFileSync(statePath, "utf8")); + const job = state.jobs.find((candidate) => candidate.id === jobId); + return job?.status === "running" && job.threadId && job.turnId ? job : null; + }, { timeoutMs: 15000 }); + + if (!loadBrokerSession(repo)) { + run("node", [SCRIPT, "cancel", jobId, "--json"], { cwd: repo, env }); + return; + } + t.after(() => { + const surviving = loadBrokerSession(repo); + if (surviving?.pid) { + try { + process.kill(surviving.pid, "SIGTERM"); + } catch { + // Ignore missing process. + } + } + }); + + // The fake app-server never answers turn/interrupt. The hook must time the + // interrupt out, tear its own connection down (so the abandoned request + // neither blocks broker/shutdown nor keeps the hook process alive), kill + // the worker, record the cancellation, and still tear the broker down. + const sessionEnd = run("node", [SESSION_HOOK, "SessionEnd"], { + cwd: repo, + env, + input: JSON.stringify({ + hook_event_name: "SessionEnd", + session_id: "sess-hang", + cwd: repo + }) + }); + assert.equal(sessionEnd.status, 0, sessionEnd.stderr); + + const state = JSON.parse(fs.readFileSync(statePath, "utf8")); + assert.equal(state.jobs.find((candidate) => candidate.id === jobId)?.status, "cancelled"); + assert.equal(loadBrokerSession(repo), null, "the broker must still be torn down after a hung interrupt"); +}); + +test("session end keeps a terminal record for the ending session's dead worker", () => { + const repo = makeTempDir(); + initGitRepo(repo); + + const stateDir = resolveStateDir(repo); + const jobsDir = path.join(stateDir, "jobs"); + fs.mkdirSync(jobsDir, { recursive: true }); + + const deadWorker = run(process.execPath, ["-e", ""], { cwd: repo }); + assert.equal(deadWorker.status, 0); + + const jobId = "task-own-dead"; + const jobFile = path.join(jobsDir, `${jobId}.json`); + fs.writeFileSync(jobFile, JSON.stringify({ id: jobId, status: "running", pid: deadWorker.pid }, null, 2), "utf8"); + fs.writeFileSync( + path.join(stateDir, "state.json"), + `${JSON.stringify( + { + version: 1, + config: { stopReviewGate: false }, + jobs: [ + { + id: jobId, + status: "running", + title: "Codex Task", + sessionId: "sess-current", + pid: deadWorker.pid, + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString() + } + ] + }, + null, + 2 + )}\n`, + "utf8" + ); + + const result = run("node", [SESSION_HOOK, "SessionEnd"], { + cwd: repo, + input: JSON.stringify({ + hook_event_name: "SessionEnd", + session_id: "sess-current", + cwd: repo + }) + }); + assert.equal(result.status, 0, result.stderr); + + // The ending session's dead-worker job must keep a terminal record and its + // job file — not be reaped to failed and then erased as an old terminal + // job, which would recreate the "No job found" outcome. + const state = JSON.parse(fs.readFileSync(path.join(stateDir, "state.json"), "utf8")); + const retained = state.jobs.find((candidate) => candidate.id === jobId); + assert.equal(retained?.status, "cancelled"); + assert.equal(fs.existsSync(jobFile), true); +}); + +test("session end kills a worker whose pid was published only after the terminal claim", async (t) => { + const repo = makeTempDir(); + initGitRepo(repo); + + const stateDir = resolveStateDir(repo); + const jobsDir = path.join(stateDir, "jobs"); + fs.mkdirSync(jobsDir, { recursive: true }); + + const jobId = "task-late-pid"; + const jobFile = path.join(jobsDir, `${jobId}.json`); + const claimFile = path.join(jobsDir, `${jobId}.terminal`); + + const sleeper = spawn(process.execPath, ["-e", "setTimeout(() => {}, 600000);"], { + detached: true, + stdio: "ignore" + }); + sleeper.unref(); + t.after(() => { + try { + process.kill(-sleeper.pid, "SIGKILL"); + } catch { + try { + process.kill(sleeper.pid, "SIGKILL"); + } catch { + // Ignore missing process. + } + } + }); + + // Record-before-spawn: the stores know the job but not the worker's pid yet. + fs.writeFileSync(jobFile, JSON.stringify({ id: jobId, status: "running", pid: null }, null, 2), "utf8"); + + // The worker publishes its pid only after the hook wins the terminal claim + // (the claim file is the observable boundary between the hook's pre-claim + // read and its post-claim reread). Filler records fatten state.json so the + // hook's post-claim index update leaves the publisher a comfortable window; + // they stay below the prune cap so the write does not also churn job files. + const filler = Array.from({ length: 49 }, (unused, index) => ({ + id: `filler-${index}`, + status: "completed", + title: "Codex Task", + sessionId: "sess-filler", + summary: "x".repeat(200000), + createdAt: "2026-03-18T15:30:00.000Z", + updatedAt: "2026-03-18T15:30:00.000Z" + })); + fs.writeFileSync( + path.join(stateDir, "state.json"), + `${JSON.stringify( + { + version: 1, + config: { stopReviewGate: false }, + jobs: [ + { + id: jobId, + status: "running", + title: "Codex Task", + sessionId: "sess-current", + pid: null, + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString() + }, + ...filler + ] + }, + null, + 2 + )}\n`, + "utf8" + ); + + const publisherScript = path.join(makeTempDir(), "publish-pid.mjs"); + fs.writeFileSync( + publisherScript, + [ + 'import fs from "node:fs";', + "const [claimFile, jobFile, jobId, pid] = process.argv.slice(2);", + "function poll() {", + " if (fs.existsSync(claimFile)) {", + ' fs.writeFileSync(jobFile, JSON.stringify({ id: jobId, status: "running", pid: Number(pid) }, null, 2));', + " process.exit(0);", + " }", + " setTimeout(poll, 2);", + "}", + "poll();", + "setTimeout(() => process.exit(1), 30000);" + ].join("\n"), + "utf8" + ); + const publisher = spawn(process.execPath, [publisherScript, claimFile, jobFile, jobId, String(sleeper.pid)], { + stdio: "ignore" + }); + t.after(() => { + try { + publisher.kill("SIGKILL"); + } catch { + // Ignore missing process. + } + }); + + const result = run("node", [SESSION_HOOK, "SessionEnd"], { + cwd: repo, + input: JSON.stringify({ + hook_event_name: "SessionEnd", + session_id: "sess-current", + cwd: repo + }) + }); + assert.equal(result.status, 0, result.stderr); + + // The cancel patch nulls the pid, so the post-claim reread held the only + // copy: the hook must have captured it and killed the worker — a survivor + // would hold its broker socket and pin the shared broker after session end. + await waitFor(() => { + try { + process.kill(sleeper.pid, 0); + return false; + } catch (error) { + return error?.code === "ESRCH"; + } + }); + + const storedJob = JSON.parse(fs.readFileSync(jobFile, "utf8")); + assert.equal(storedJob.status, "cancelled"); + assert.equal(storedJob.pid, null); +}); + +test("session end converges the index when a finalizer dies between its two store writes", async (t) => { + const repo = makeTempDir(); + initGitRepo(repo); + + const stateDir = resolveStateDir(repo); + const jobsDir = path.join(stateDir, "jobs"); + fs.mkdirSync(jobsDir, { recursive: true }); + + const jobId = "task-torn-finalizer"; + const jobFile = path.join(jobsDir, `${jobId}.json`); + const claimFile = path.join(jobsDir, `${jobId}.terminal`); + + // A finalizer that wins its own terminal claim, writes the terminal job + // file — and dies before the state.json update that normally follows it. + // It must be alive when the hook checks its claim and dead by the end of + // the exit grace: a decoy job right behind it in the loop provides the + // synchronization signal — the decoy's cancelled record proves the + // finalizer's own claim check has already passed. + const finalizerScript = path.join(makeTempDir(), "torn-finalizer.mjs"); + fs.writeFileSync( + finalizerScript, + [ + 'import fs from "node:fs";', + "const [jobFile, jobId, stateFile] = process.argv.slice(2);", + "function poll() {", + " let cancelled = false;", + " try {", + ' const state = JSON.parse(fs.readFileSync(stateFile, "utf8"));', + ' cancelled = state.jobs.some((job) => job.id === "task-decoy" && job.status === "cancelled");', + " } catch {", + " // Mid-write state file; retry.", + " }", + " if (cancelled) {", + " fs.writeFileSync(jobFile, JSON.stringify({", + " id: jobId,", + ' status: "completed",', + ' phase: "done",', + " pid: null,", + " completedAt: new Date().toISOString()", + " }, null, 2));", + " process.exit(0);", + " }", + " setTimeout(poll, 10);", + "}", + "poll();", + "setTimeout(() => process.exit(1), 30000);" + ].join("\n"), + "utf8" + ); + // The finalizer runs under a launcher rather than as this test's own child: + // the test blocks in spawnSync while the hook runs, so a direct child would + // linger as an unreaped zombie that still reads as alive — masking the exit + // the hook's grace wait must observe. The launcher's free event loop reaps + // the finalizer the moment it dies. + const launcherScript = path.join(makeTempDir(), "finalizer-launcher.mjs"); + fs.writeFileSync( + launcherScript, + [ + 'import fs from "node:fs";', + 'import { spawn } from "node:child_process";', + "const [finalizerScript, jobFile, jobId, stateFile, pidFile] = process.argv.slice(2);", + 'const child = spawn(process.execPath, [finalizerScript, jobFile, jobId, stateFile], { stdio: "ignore" });', + "fs.writeFileSync(pidFile, String(child.pid));", + 'child.on("exit", () => process.exit(0));', + "setTimeout(() => {", + ' try { child.kill("SIGKILL"); } catch {}', + " process.exit(1);", + "}, 30000);" + ].join("\n"), + "utf8" + ); + const pidFile = path.join(stateDir, "finalizer.pid"); + const launcher = spawn( + process.execPath, + [launcherScript, finalizerScript, jobFile, jobId, path.join(stateDir, "state.json"), pidFile], + { detached: true, stdio: "ignore" } + ); + launcher.unref(); + t.after(() => { + try { + process.kill(-launcher.pid, "SIGKILL"); + } catch { + try { + process.kill(launcher.pid, "SIGKILL"); + } catch { + // Ignore missing process. + } + } + }); + const finalizerPid = Number(await waitFor(() => (fs.existsSync(pidFile) ? fs.readFileSync(pidFile, "utf8").trim() : null))); + + fs.writeFileSync(jobFile, JSON.stringify({ id: jobId, status: "running", pid: finalizerPid }, null, 2), "utf8"); + fs.writeFileSync(claimFile, `${finalizerPid} worker\n`, "utf8"); + // The decoy carries a turn identity so its own processing does not stall + // on an identity wait; its interrupt fails fast against the dead endpoint. + fs.writeFileSync( + path.join(jobsDir, "task-decoy.json"), + JSON.stringify({ id: "task-decoy", status: "running", pid: null, threadId: "th-decoy", turnId: "turn-decoy" }, null, 2), + "utf8" + ); + fs.writeFileSync( + path.join(stateDir, "state.json"), + `${JSON.stringify( + { + version: 1, + config: { stopReviewGate: false }, + jobs: [ + { + id: jobId, + status: "running", + title: "Codex Task", + sessionId: "sess-current", + pid: finalizerPid, + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString() + }, + { + id: "task-decoy", + status: "running", + title: "Codex Task", + sessionId: "sess-current", + pid: null, + threadId: "th-decoy", + turnId: "turn-decoy", + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString() + } + ] + }, + null, + 2 + )}\n`, + "utf8" + ); + + // A (dead) broker endpoint turns the interrupt/grace-wait path on; the + // shutdown probe against it reports unreachable, which is fine here. + const result = run("node", [SESSION_HOOK, "SessionEnd"], { + cwd: repo, + env: { + ...process.env, + CODEX_COMPANION_APP_SERVER_ENDPOINT: `unix:${path.join(stateDir, "dead-broker.sock")}` + }, + input: JSON.stringify({ + hook_event_name: "SessionEnd", + session_id: "sess-current", + cwd: repo + }) + }); + assert.equal(result.status, 0, result.stderr); + + // The index must not retain a running record with a dead pid — the torn + // write converges to the job file's completed outcome, after which the job + // is erased like the session's other finished jobs (files included). + if (fs.existsSync(jobFile)) { + assert.equal(JSON.parse(fs.readFileSync(jobFile, "utf8")).status, "completed"); + } + const state = JSON.parse(fs.readFileSync(path.join(stateDir, "state.json"), "utf8")); + const record = state.jobs.find((candidate) => candidate.id === jobId); + assert.ok( + !record || (record.status !== "running" && record.status !== "queued"), + `index must not keep a running record for a dead finalizer: ${JSON.stringify(record)}` + ); +}); + +test("a hung turn interrupt cannot starve later jobs' interrupt attempts", async (t) => { + const repo = makeTempDir(); + initGitRepo(repo); + + const stateDir = resolveStateDir(repo); + const jobsDir = path.join(stateDir, "jobs"); + fs.mkdirSync(jobsDir, { recursive: true }); + + const socketPath = path.join(makeTempDir("cx-sock-"), "broker.sock"); + const recordFile = path.join(stateDir, "interrupts.jsonl"); + const readyFile = path.join(stateDir, "broker-ready"); + + // A fake broker that answers initialize, records every turn/interrupt, + // never responds for the first job's turn, and refuses broker/shutdown. + const brokerScript = path.join(makeTempDir(), "hanging-broker.mjs"); + fs.writeFileSync( + brokerScript, + [ + 'import fs from "node:fs";', + 'import net from "node:net";', + "const [socketPath, recordFile, readyFile] = process.argv.slice(2);", + "const server = net.createServer((socket) => {", + ' socket.setEncoding("utf8");', + ' let buffer = "";', + ' socket.on("error", () => {});', + ' socket.on("data", (chunk) => {', + " buffer += chunk;", + " let index;", + ' while ((index = buffer.indexOf("\\n")) !== -1) {', + " const line = buffer.slice(0, index);", + " buffer = buffer.slice(index + 1);", + " if (!line.trim()) continue;", + " let message;", + " try { message = JSON.parse(line); } catch { continue; }", + ' if (message.method === "turn/interrupt") {', + " fs.appendFileSync(recordFile, `${JSON.stringify(message.params)}\\n`);", + ' if (message.params?.turnId === "turn-hang") continue;', + " socket.write(`${JSON.stringify({ id: message.id, result: {} })}\\n`);", + ' } else if (message.method === "broker/shutdown") {', + ' socket.write(`${JSON.stringify({ id: message.id, error: { code: -32001, message: "busy" } })}\\n`);', + " } else if (message.id != null) {", + " socket.write(`${JSON.stringify({ id: message.id, result: {} })}\\n`);", + " }", + " }", + " });", + "});", + 'server.listen(socketPath, () => fs.writeFileSync(readyFile, "ready"));' + ].join("\n"), + "utf8" + ); + const broker = spawn(process.execPath, [brokerScript, socketPath, recordFile, readyFile], { stdio: "ignore" }); + t.after(() => { + try { + broker.kill("SIGKILL"); + } catch { + // Ignore missing process. + } + }); + await waitFor(() => fs.existsSync(readyFile)); + + const jobs = [ + { id: "task-hang", threadId: "th-hang", turnId: "turn-hang" }, + { id: "task-late", threadId: "th-late", turnId: "turn-late" } + ]; + for (const job of jobs) { + fs.writeFileSync( + path.join(jobsDir, `${job.id}.json`), + JSON.stringify({ id: job.id, status: "running", pid: null, threadId: job.threadId, turnId: job.turnId }, null, 2), + "utf8" + ); + } + fs.writeFileSync( + path.join(stateDir, "state.json"), + `${JSON.stringify( + { + version: 1, + config: { stopReviewGate: false }, + jobs: jobs.map((job) => ({ + id: job.id, + status: "running", + title: "Codex Task", + sessionId: "sess-current", + pid: null, + threadId: job.threadId, + turnId: job.turnId, + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString() + })) + }, + null, + 2 + )}\n`, + "utf8" + ); + + const result = run("node", [SESSION_HOOK, "SessionEnd"], { + cwd: repo, + env: { + ...process.env, + CODEX_COMPANION_APP_SERVER_ENDPOINT: `unix:${socketPath}` + }, + input: JSON.stringify({ + hook_event_name: "SessionEnd", + session_id: "sess-current", + cwd: repo + }) + }); + assert.equal(result.status, 0, result.stderr); + + // The first job's interrupt hangs until its budget share runs out; the + // second job must still get its own bounded attempt, not be recorded + // cancelled while its server-side turn runs on uninterrupted. + const attempts = fs + .readFileSync(recordFile, "utf8") + .trim() + .split("\n") + .map((line) => JSON.parse(line).turnId) + .sort(); + assert.deepEqual(attempts, ["turn-hang", "turn-late"]); + + const state = JSON.parse(fs.readFileSync(path.join(stateDir, "state.json"), "utf8")); + for (const job of jobs) { + assert.equal(state.jobs.find((candidate) => candidate.id === job.id)?.status, "cancelled"); + } +}); + +test("a hung dead-worker reap interrupt cannot starve the session's own interrupts", async (t) => { + const repo = makeTempDir(); + initGitRepo(repo); + + const stateDir = resolveStateDir(repo); + const jobsDir = path.join(stateDir, "jobs"); + fs.mkdirSync(jobsDir, { recursive: true }); + + const socketPath = path.join(makeTempDir("cx-sock-"), "broker.sock"); + const recordFile = path.join(stateDir, "interrupts.jsonl"); + const readyFile = path.join(stateDir, "broker-ready"); + + // Same fake broker as above: answers initialize, records every + // turn/interrupt, never responds for the reaped job's turn, and refuses + // broker/shutdown. + const brokerScript = path.join(makeTempDir(), "hanging-broker.mjs"); + fs.writeFileSync( + brokerScript, + [ + 'import fs from "node:fs";', + 'import net from "node:net";', + "const [socketPath, recordFile, readyFile] = process.argv.slice(2);", + "const server = net.createServer((socket) => {", + ' socket.setEncoding("utf8");', + ' let buffer = "";', + ' socket.on("error", () => {});', + ' socket.on("data", (chunk) => {', + " buffer += chunk;", + " let index;", + ' while ((index = buffer.indexOf("\\n")) !== -1) {', + " const line = buffer.slice(0, index);", + " buffer = buffer.slice(index + 1);", + " if (!line.trim()) continue;", + " let message;", + " try { message = JSON.parse(line); } catch { continue; }", + ' if (message.method === "turn/interrupt") {', + " fs.appendFileSync(recordFile, `${JSON.stringify(message.params)}\\n`);", + ' if (message.params?.turnId === "turn-reap-hang") continue;', + " socket.write(`${JSON.stringify({ id: message.id, result: {} })}\\n`);", + ' } else if (message.method === "broker/shutdown") {', + ' socket.write(`${JSON.stringify({ id: message.id, error: { code: -32001, message: "busy" } })}\\n`);', + " } else if (message.id != null) {", + " socket.write(`${JSON.stringify({ id: message.id, result: {} })}\\n`);", + " }", + " }", + " });", + "});", + 'server.listen(socketPath, () => fs.writeFileSync(readyFile, "ready"));' + ].join("\n"), + "utf8" + ); + const broker = spawn(process.execPath, [brokerScript, socketPath, recordFile, readyFile], { stdio: "ignore" }); + t.after(() => { + try { + broker.kill("SIGKILL"); + } catch { + // Ignore missing process. + } + }); + await waitFor(() => fs.existsSync(readyFile)); + + // Another session's worker is dead with a persisted turn identity: the + // reaper interrupts that turn before failing the record — and that RPC + // hangs. The ending session's own running job must still get its own + // bounded interrupt attempt from the shared deadline. + const deadWorker = run(process.execPath, ["-e", ""], { cwd: repo }); + assert.equal(deadWorker.status, 0); + + const reapedJob = { id: "task-reaped", threadId: "th-reap", turnId: "turn-reap-hang" }; + const ownJob = { id: "task-own", threadId: "th-own", turnId: "turn-own" }; + fs.writeFileSync( + path.join(jobsDir, `${reapedJob.id}.json`), + JSON.stringify({ id: reapedJob.id, status: "running", pid: deadWorker.pid, threadId: reapedJob.threadId, turnId: reapedJob.turnId }, null, 2), + "utf8" + ); + fs.writeFileSync( + path.join(jobsDir, `${ownJob.id}.json`), + JSON.stringify({ id: ownJob.id, status: "running", pid: null, threadId: ownJob.threadId, turnId: ownJob.turnId }, null, 2), + "utf8" + ); + fs.writeFileSync( + path.join(stateDir, "state.json"), + `${JSON.stringify( + { + version: 1, + config: { stopReviewGate: false }, + jobs: [ + { + id: reapedJob.id, + status: "running", + title: "Codex Task", + sessionId: "sess-other", + pid: deadWorker.pid, + threadId: reapedJob.threadId, + turnId: reapedJob.turnId, + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString() + }, + { + id: ownJob.id, + status: "running", + title: "Codex Task", + sessionId: "sess-current", + pid: null, + threadId: ownJob.threadId, + turnId: ownJob.turnId, + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString() + } + ] + }, + null, + 2 + )}\n`, + "utf8" + ); + + const result = run("node", [SESSION_HOOK, "SessionEnd"], { + cwd: repo, + env: { + ...process.env, + CODEX_COMPANION_APP_SERVER_ENDPOINT: `unix:${socketPath}` + }, + input: JSON.stringify({ + hook_event_name: "SessionEnd", + session_id: "sess-current", + cwd: repo + }) + }); + assert.equal(result.status, 0, result.stderr); + + const attempts = fs + .readFileSync(recordFile, "utf8") + .trim() + .split("\n") + .map((line) => JSON.parse(line).turnId) + .sort(); + assert.deepEqual(attempts, ["turn-own", "turn-reap-hang"]); + + const state = JSON.parse(fs.readFileSync(path.join(stateDir, "state.json"), "utf8")); + assert.equal(state.jobs.find((candidate) => candidate.id === reapedJob.id)?.status, "failed"); + assert.equal(state.jobs.find((candidate) => candidate.id === ownJob.id)?.status, "cancelled"); +}); + +test("background task records the turn id even when turn/started never arrives", async (t) => { + const repo = makeTempDir(); + const binDir = makeTempDir(); + installFakeCodex(binDir, "no-turn-started"); + initGitRepo(repo); + fs.writeFileSync(path.join(repo, "README.md"), "hello\n"); + run("git", ["add", "README.md"], { cwd: repo }); + run("git", ["commit", "-m", "init"], { cwd: repo }); + + const env = { ...buildEnv(binDir), CODEX_COMPANION_SESSION_ID: "sess-noturn" }; + const launched = run("node", [SCRIPT, "task", "--background", "--json", "investigate the flaky worker timeout"], { + cwd: repo, + env + }); + assert.equal(launched.status, 0, launched.stderr); + const jobId = JSON.parse(launched.stdout).jobId; + + t.after(() => { + run("node", [SESSION_HOOK, "SessionEnd"], { + cwd: repo, + env, + input: JSON.stringify({ hook_event_name: "SessionEnd", session_id: "sess-noturn", cwd: repo }) + }); + }); + + // Cleanup can only interrupt a turn whose id reached the stored record; + // the id must come from the turn/start response, not just notifications. + const statePath = path.join(resolveStateDir(repo), "state.json"); + const runningJob = await waitFor(() => { + const state = JSON.parse(fs.readFileSync(statePath, "utf8")); + const job = state.jobs.find((candidate) => candidate.id === jobId); + return job?.threadId && job.turnId ? job : null; + }, { timeoutMs: 15000 }); + assert.ok(runningJob.turnId); +}); + +test("session end reaps a dead worker's active record to failed", () => { + const repo = makeTempDir(); + initGitRepo(repo); + + const stateDir = resolveStateDir(repo); + fs.mkdirSync(path.join(stateDir, "jobs"), { recursive: true }); + + const deadWorker = run(process.execPath, ["-e", ""], { cwd: repo }); + assert.equal(deadWorker.status, 0); + + // A worker SIGKILLed/OOMed without its SessionEnd ever running has no + // other reaper; the zombie record must not stay active forever. + fs.writeFileSync( + path.join(stateDir, "state.json"), + `${JSON.stringify( + { + version: 1, + config: { stopReviewGate: false }, + jobs: [ + { + id: "task-zombie", + status: "running", + title: "Codex Task", + sessionId: "sess-gone", + pid: deadWorker.pid, + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString() + } + ] + }, + null, + 2 + )}\n`, + "utf8" + ); + + const result = run("node", [SESSION_HOOK, "SessionEnd"], { + cwd: repo, + input: JSON.stringify({ + hook_event_name: "SessionEnd", + session_id: "sess-current", + cwd: repo + }) + }); + assert.equal(result.status, 0, result.stderr); + + const state = JSON.parse(fs.readFileSync(path.join(stateDir, "state.json"), "utf8")); + const reaped = state.jobs.find((candidate) => candidate.id === "task-zombie"); + assert.equal(reaped?.status, "failed", "a dead worker's record must be reaped to failed"); + assert.equal(reaped?.pid, null); +}); + +test("session end interrupts a reaped dead worker's turn before failing it", async (t) => { + const repo = makeTempDir(); + const binDir = makeTempDir(); + const fakeStatePath = path.join(binDir, "fake-codex-state.json"); + installFakeCodex(binDir); + initGitRepo(repo); + fs.writeFileSync(path.join(repo, "README.md"), "hello\n"); + run("git", ["add", "README.md"], { cwd: repo }); + run("git", ["commit", "-m", "init"], { cwd: repo }); + fs.writeFileSync(path.join(repo, "README.md"), "hello again\n"); + + const env = buildEnv(binDir); + const review = run("node", [SCRIPT, "review"], { cwd: repo, env }); + assert.equal(review.status, 0, review.stderr); + + if (!loadBrokerSession(repo)) { + return; + } + + // Pin the broker with a live job so it survives this session end — the + // reaped dead worker's server-side turn would otherwise keep running. + const sleeper = spawn(process.execPath, ["-e", "setInterval(() => {}, 1000)"], { + cwd: repo, + detached: true, + stdio: "ignore" + }); + sleeper.unref(); + t.after(() => { + try { + process.kill(sleeper.pid, "SIGTERM"); + } catch { + // Ignore missing process. + } + const surviving = loadBrokerSession(repo); + if (surviving?.pid) { + try { + process.kill(surviving.pid, "SIGTERM"); + } catch { + // Ignore missing process. + } + } + }); + + const deadWorker = run(process.execPath, ["-e", ""], { cwd: repo }); + assert.equal(deadWorker.status, 0); + + const stateDir = resolveStateDir(repo); + fs.writeFileSync( + path.join(stateDir, "state.json"), + `${JSON.stringify( + { + version: 1, + config: { stopReviewGate: false }, + jobs: [ + { + id: "task-dead-with-turn", + status: "running", + title: "Codex Task", + sessionId: "sess-other", + pid: deadWorker.pid, + threadId: "thr_dead", + turnId: "turn_dead", + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString() + }, + { + id: "task-live-pin", + status: "running", + title: "Codex Task", + sessionId: "sess-other2", + pid: sleeper.pid, + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString() + } + ] + }, + null, + 2 + )}\n`, + "utf8" + ); + + const result = run("node", [SESSION_HOOK, "SessionEnd"], { + cwd: repo, + env, + input: JSON.stringify({ + hook_event_name: "SessionEnd", + session_id: "sess-current", + cwd: repo + }) + }); + assert.equal(result.status, 0, result.stderr); + + const state = JSON.parse(fs.readFileSync(path.join(stateDir, "state.json"), "utf8")); + assert.equal(state.jobs.find((candidate) => candidate.id === "task-dead-with-turn")?.status, "failed"); + assert.ok(loadBrokerSession(repo), "the live job must keep the broker running"); + + // The reaper must have interrupted the dead worker's server-side turn + // before dropping the record out of the active set. + const fakeState = JSON.parse(fs.readFileSync(fakeStatePath, "utf8")); + assert.deepEqual(fakeState.lastInterrupt, { threadId: "thr_dead", turnId: "turn_dead" }); +}); + +test("cancel reports a benign already-finished result when the worker owns the terminal claim", async (t) => { + const repo = makeTempDir(); + initGitRepo(repo); + + const stateDir = resolveStateDir(repo); + fs.mkdirSync(path.join(stateDir, "jobs"), { recursive: true }); + + const sleeper = spawn(process.execPath, ["-e", "setInterval(() => {}, 1000)"], { + cwd: repo, + detached: true, + stdio: "ignore" + }); + sleeper.unref(); + t.after(() => { + try { + process.kill(sleeper.pid, "SIGTERM"); + } catch { + // Ignore missing process. + } + }); + + const jobId = "task-finishing-now"; + fs.writeFileSync( + path.join(stateDir, "jobs", `${jobId}.json`), + JSON.stringify({ id: jobId, status: "running", pid: sleeper.pid }, null, 2), + "utf8" + ); + fs.writeFileSync( + path.join(stateDir, "state.json"), + `${JSON.stringify( + { + version: 1, + config: { stopReviewGate: false }, + jobs: [ + { + id: jobId, + status: "running", + title: "Codex Task", + sessionId: "sess-current", + pid: sleeper.pid, + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString() + } + ] + }, + null, + 2 + )}\n`, + "utf8" + ); + // The worker won the terminal claim as its turn completed. + fs.writeFileSync(path.join(stateDir, "jobs", `${jobId}.terminal`), `${sleeper.pid} worker\n`, "utf8"); + + const result = run("node", [SCRIPT, "cancel", jobId, "--json"], { + cwd: repo, + env: { ...process.env, CODEX_COMPANION_SESSION_ID: "sess-current" } + }); + assert.equal(result.status, 0, result.stderr); + const payload = JSON.parse(result.stdout); + assert.equal(payload.alreadyFinished, true); + assert.equal(payload.jobId, jobId); +}); + +test("cancel syncs a stale running index when the finished worker died before its state.json write", () => { + const repo = makeTempDir(); + initGitRepo(repo); + + const stateDir = resolveStateDir(repo); + fs.mkdirSync(path.join(stateDir, "jobs"), { recursive: true }); + + const deadWorker = run(process.execPath, ["-e", ""], { cwd: repo }); + assert.equal(deadWorker.status, 0); + + // The worker won the terminal claim, wrote its terminal job file, and died + // before the upsertJob state-index write landed: state.json still says + // running with the dead pid. + const jobId = "task-index-stale"; + fs.writeFileSync( + path.join(stateDir, "jobs", `${jobId}.json`), + JSON.stringify( + { id: jobId, status: "completed", phase: "completed", pid: null, completedAt: new Date().toISOString() }, + null, + 2 + ), + "utf8" + ); + fs.writeFileSync( + path.join(stateDir, "state.json"), + `${JSON.stringify( + { + version: 1, + config: { stopReviewGate: false }, + jobs: [ + { + id: jobId, + status: "running", + title: "Codex Task", + sessionId: "sess-current", + pid: deadWorker.pid, + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString() + } + ] + }, + null, + 2 + )}\n`, + "utf8" + ); + fs.writeFileSync(path.join(stateDir, "jobs", `${jobId}.terminal`), `${deadWorker.pid} worker\n`, "utf8"); + + const result = run("node", [SCRIPT, "cancel", jobId, "--json"], { + cwd: repo, + env: { ...process.env, CODEX_COMPANION_SESSION_ID: "sess-current" } + }); + assert.equal(result.status, 0, result.stderr); + const payload = JSON.parse(result.stdout); + assert.equal(payload.alreadyFinished, true); + assert.equal(payload.status, "completed"); + + // The already-finished return must have repaired the index, not just read + // the job file: otherwise the job stays listed as running with a dead pid. + const state = JSON.parse(fs.readFileSync(path.join(stateDir, "state.json"), "utf8")); + const indexed = state.jobs.find((entry) => entry.id === jobId); + assert.equal(indexed.status, "completed"); + assert.equal(indexed.pid, null); +}); + +test("cancel repairs an orphaned terminal claim and still stops the worker", async (t) => { + const repo = makeTempDir(); + initGitRepo(repo); + + const stateDir = resolveStateDir(repo); + fs.mkdirSync(path.join(stateDir, "jobs"), { recursive: true }); + + const sleeper = spawn(process.execPath, ["-e", "setInterval(() => {}, 1000)"], { + cwd: repo, + detached: true, + stdio: "ignore" + }); + sleeper.unref(); + t.after(() => { + try { + process.kill(sleeper.pid, "SIGTERM"); + } catch { + // Ignore missing process. + } + }); + + const deadClaimant = run(process.execPath, ["-e", ""], { cwd: repo }); + assert.equal(deadClaimant.status, 0); + + const jobId = "task-orphan-claim"; + fs.writeFileSync( + path.join(stateDir, "jobs", `${jobId}.json`), + JSON.stringify({ id: jobId, status: "running", pid: sleeper.pid }, null, 2), + "utf8" + ); + fs.writeFileSync( + path.join(stateDir, "state.json"), + `${JSON.stringify( + { + version: 1, + config: { stopReviewGate: false }, + jobs: [ + { + id: jobId, + status: "running", + title: "Codex Task", + sessionId: "sess-current", + pid: sleeper.pid, + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString() + } + ] + }, + null, + 2 + )}\n`, + "utf8" + ); + // A previous cancel claimed the terminal status and died before writing + // any record: the worker is still alive and must remain cancellable. + fs.writeFileSync(path.join(stateDir, "jobs", `${jobId}.terminal`), `${deadClaimant.pid} cancel\n`, "utf8"); + + const result = run("node", [SCRIPT, "cancel", jobId, "--json"], { + cwd: repo, + env: { ...process.env, CODEX_COMPANION_SESSION_ID: "sess-current" } + }); + assert.equal(result.status, 0, result.stderr); + const payload = JSON.parse(result.stdout); + assert.equal(payload.status, "cancelled"); + assert.notEqual(payload.alreadyFinished, true); + + await waitFor(() => { + try { + process.kill(sleeper.pid, 0); + return false; + } catch (error) { + return error?.code === "ESRCH"; + } + }); + + const state = JSON.parse(fs.readFileSync(path.join(stateDir, "state.json"), "utf8")); + assert.equal(state.jobs.find((candidate) => candidate.id === jobId)?.status, "cancelled"); +}); + +test("cancel reports failed for a dead worker's own orphaned claim", () => { + const repo = makeTempDir(); + initGitRepo(repo); + + const stateDir = resolveStateDir(repo); + fs.mkdirSync(path.join(stateDir, "jobs"), { recursive: true }); + + const deadWorker = run(process.execPath, ["-e", ""], { cwd: repo }); + assert.equal(deadWorker.status, 0); + + const jobId = "task-worker-claim-died"; + fs.writeFileSync( + path.join(stateDir, "jobs", `${jobId}.json`), + JSON.stringify({ id: jobId, status: "running", pid: deadWorker.pid }, null, 2), + "utf8" + ); + fs.writeFileSync( + path.join(stateDir, "state.json"), + `${JSON.stringify( + { + version: 1, + config: { stopReviewGate: false }, + jobs: [ + { + id: jobId, + status: "running", + title: "Codex Task", + sessionId: "sess-current", + pid: deadWorker.pid, + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString() + } + ] + }, + null, + 2 + )}\n`, + "utf8" + ); + // The worker claimed for its own terminal write and died before it landed; + // the job actually ran, so the outcome is failed, not cancelled. + fs.writeFileSync(path.join(stateDir, "jobs", `${jobId}.terminal`), `${deadWorker.pid} worker\n`, "utf8"); + + const result = run("node", [SCRIPT, "cancel", jobId, "--json"], { + cwd: repo, + env: { ...process.env, CODEX_COMPANION_SESSION_ID: "sess-current" } + }); + assert.equal(result.status, 0, result.stderr); + const payload = JSON.parse(result.stdout); + assert.equal(payload.status, "failed", "the claim's intent decides the terminal status"); + + const stored = JSON.parse(fs.readFileSync(path.join(stateDir, "jobs", `${jobId}.json`), "utf8")); + assert.equal(stored.status, "failed"); +}); + +test("session end adopts an orphaned claim on its own job and still stops the worker", async (t) => { + const repo = makeTempDir(); + initGitRepo(repo); + + const stateDir = resolveStateDir(repo); + const jobsDir = path.join(stateDir, "jobs"); + fs.mkdirSync(jobsDir, { recursive: true }); + + const sleeper = spawn(process.execPath, ["-e", "setInterval(() => {}, 1000)"], { + cwd: repo, + detached: true, + stdio: "ignore" + }); + sleeper.unref(); + t.after(() => { + try { + process.kill(sleeper.pid, "SIGTERM"); + } catch { + // Ignore missing process. + } + }); + + const deadClaimant = run(process.execPath, ["-e", ""], { cwd: repo }); + assert.equal(deadClaimant.status, 0); + + const jobId = "task-orphan-at-end"; + fs.writeFileSync( + path.join(jobsDir, `${jobId}.json`), + JSON.stringify({ id: jobId, status: "running", pid: sleeper.pid }, null, 2), + "utf8" + ); + fs.writeFileSync( + path.join(stateDir, "state.json"), + `${JSON.stringify( + { + version: 1, + config: { stopReviewGate: false }, + jobs: [ + { + id: jobId, + status: "running", + title: "Codex Task", + sessionId: "sess-current", + pid: sleeper.pid, + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString() + } + ] + }, + null, + 2 + )}\n`, + "utf8" + ); + // A previous cancel claimed the terminal status and died before writing + // any record; session end must adopt the claim, not skip the job. + fs.writeFileSync(path.join(jobsDir, `${jobId}.terminal`), `${deadClaimant.pid} cancel\n`, "utf8"); + + const result = run("node", [SESSION_HOOK, "SessionEnd"], { + cwd: repo, + input: JSON.stringify({ + hook_event_name: "SessionEnd", + session_id: "sess-current", + cwd: repo + }) + }); + assert.equal(result.status, 0, result.stderr); + + await waitFor(() => { + try { + process.kill(sleeper.pid, 0); + return false; + } catch (error) { + return error?.code === "ESRCH"; + } + }); + + const state = JSON.parse(fs.readFileSync(path.join(stateDir, "state.json"), "utf8")); + assert.equal(state.jobs.find((candidate) => candidate.id === jobId)?.status, "cancelled"); +}); + +test("session end tears down the broker when the other session's active record is stale", async (t) => { + const repo = makeTempDir(); + const binDir = makeTempDir(); + installFakeCodex(binDir); + initGitRepo(repo); + fs.writeFileSync(path.join(repo, "README.md"), "hello\n"); + run("git", ["add", "README.md"], { cwd: repo }); + run("git", ["commit", "-m", "init"], { cwd: repo }); + fs.writeFileSync(path.join(repo, "README.md"), "hello again\n"); + + const env = buildEnv(binDir); + const review = run("node", [SCRIPT, "review"], { cwd: repo, env }); + assert.equal(review.status, 0, review.stderr); + + if (!loadBrokerSession(repo)) { + return; + } + + // A pid-less active record has no liveness signal; once it is a day old + // it must not pin the broker (a live pid always wins over record age). + const staleTimestamp = new Date(Date.now() - 48 * 60 * 60 * 1000).toISOString(); + const stateDir = resolveStateDir(repo); + fs.writeFileSync( + path.join(stateDir, "state.json"), + `${JSON.stringify( + { + version: 1, + config: { stopReviewGate: false }, + jobs: [ + { + id: "task-stale-record", + status: "running", + title: "Codex Task", + sessionId: "sess-other", + pid: null, + createdAt: staleTimestamp, + updatedAt: staleTimestamp + } + ] + }, + null, + 2 + )}\n`, + "utf8" + ); + + const result = run("node", [SESSION_HOOK, "SessionEnd"], { + cwd: repo, + env, + input: JSON.stringify({ + hook_event_name: "SessionEnd", + session_id: "sess-current", + cwd: repo + }) + }); + assert.equal(result.status, 0, result.stderr); + assert.equal(loadBrokerSession(repo), null, "a stale active record must not pin the shared broker"); +}); + +test("session end leaves the shared broker running while another session has an active job", async (t) => { + const repo = makeTempDir(); + const binDir = makeTempDir(); + installFakeCodex(binDir); + initGitRepo(repo); + fs.writeFileSync(path.join(repo, "README.md"), "hello\n"); + run("git", ["add", "README.md"], { cwd: repo }); + run("git", ["commit", "-m", "init"], { cwd: repo }); + fs.writeFileSync(path.join(repo, "README.md"), "hello again\n"); + + const env = buildEnv(binDir); + const review = run("node", [SCRIPT, "review"], { cwd: repo, env }); + assert.equal(review.status, 0, review.stderr); + + const brokerSession = loadBrokerSession(repo); + if (!brokerSession) { + return; + } + + const sleeper = spawn(process.execPath, ["-e", "setInterval(() => {}, 1000)"], { + cwd: repo, + detached: true, + stdio: "ignore" + }); + sleeper.unref(); + t.after(() => { + try { + process.kill(sleeper.pid, "SIGTERM"); + } catch { + // Ignore missing process. + } + }); + + const stateDir = resolveStateDir(repo); + const writeJobs = (jobs) => { + fs.writeFileSync( + path.join(stateDir, "state.json"), + `${JSON.stringify({ version: 1, config: { stopReviewGate: false }, jobs }, null, 2)}\n`, + "utf8" + ); + }; + writeJobs([ + { + id: "task-other-session", + status: "running", + title: "Codex Task", + sessionId: "sess-other", + pid: sleeper.pid, + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString() + } + ]); + + const firstEnd = run("node", [SESSION_HOOK, "SessionEnd"], { + cwd: repo, + env, + input: JSON.stringify({ + hook_event_name: "SessionEnd", + session_id: "sess-current", + cwd: repo + }) + }); + assert.equal(firstEnd.status, 0, firstEnd.stderr); + + const survivingSession = loadBrokerSession(repo); + assert.ok(survivingSession, "broker session record should survive while another session's job is running"); + assert.doesNotThrow(() => process.kill(survivingSession.pid, 0), "broker process should still be alive"); + + // Once the other session's job is finished, the next session end tears the broker down. + writeJobs([ + { + id: "task-other-session", + status: "completed", + title: "Codex Task", + sessionId: "sess-other", + pid: null, + createdAt: "2026-03-18T15:30:00.000Z", + updatedAt: "2026-03-18T15:32:00.000Z" + } + ]); + + const secondEnd = run("node", [SESSION_HOOK, "SessionEnd"], { + cwd: repo, + env, + input: JSON.stringify({ + hook_event_name: "SessionEnd", + session_id: "sess-other", + cwd: repo + }) + }); + assert.equal(secondEnd.status, 0, secondEnd.stderr); + assert.equal(loadBrokerSession(repo), null); +}); + +test("session end still tears down the broker when the other session's job worker is dead", async () => { + const repo = makeTempDir(); + const binDir = makeTempDir(); + installFakeCodex(binDir); + initGitRepo(repo); + fs.writeFileSync(path.join(repo, "README.md"), "hello\n"); + run("git", ["add", "README.md"], { cwd: repo }); + run("git", ["commit", "-m", "init"], { cwd: repo }); + fs.writeFileSync(path.join(repo, "README.md"), "hello again\n"); + + const env = buildEnv(binDir); + const review = run("node", [SCRIPT, "review"], { cwd: repo, env }); + assert.equal(review.status, 0, review.stderr); + + const brokerSession = loadBrokerSession(repo); + if (!brokerSession) { + return; + } + + // A worker that crashed at startup leaves a permanently queued record with + // a dead pid; it must not pin the broker. + const deadWorker = run(process.execPath, ["-e", ""], { cwd: repo }); + assert.equal(deadWorker.status, 0); + const deadPid = deadWorker.pid; + + const stateDir = resolveStateDir(repo); + fs.writeFileSync( + path.join(stateDir, "state.json"), + `${JSON.stringify( + { + version: 1, + config: { stopReviewGate: false }, + jobs: [ + { + id: "task-dead-worker", + status: "queued", + title: "Codex Task", + sessionId: "sess-other", + pid: deadPid, + createdAt: "2026-03-18T15:30:00.000Z", + updatedAt: "2026-03-18T15:31:00.000Z" + } + ] + }, + null, + 2 + )}\n`, + "utf8" + ); + + const end = run("node", [SESSION_HOOK, "SessionEnd"], { + cwd: repo, + env, + input: JSON.stringify({ + hook_event_name: "SessionEnd", + session_id: "sess-current", + cwd: repo + }) + }); + assert.equal(end.status, 0, end.stderr); + assert.equal(loadBrokerSession(repo), null, "dead queued worker must not keep the broker alive"); +}); + +test("background task records a failure instead of hanging when the broker dies mid-turn", async () => { + const repo = makeTempDir(); + const binDir = makeTempDir(); + installFakeCodex(binDir, "interruptible-slow-task"); + initGitRepo(repo); + fs.writeFileSync(path.join(repo, "README.md"), "hello\n"); + run("git", ["add", "README.md"], { cwd: repo }); + run("git", ["commit", "-m", "init"], { cwd: repo }); + + const env = buildEnv(binDir); + const launched = run("node", [SCRIPT, "task", "--background", "--json", "investigate the flaky worker timeout"], { + cwd: repo, + env + }); + assert.equal(launched.status, 0, launched.stderr); + const jobId = JSON.parse(launched.stdout).jobId; + assert.ok(jobId); + + const stateDir = resolveStateDir(repo); + const readJob = () => { + const state = JSON.parse(fs.readFileSync(path.join(stateDir, "state.json"), "utf8")); + return state.jobs.find((candidate) => candidate.id === jobId) ?? null; + }; + + await waitFor(() => { + const job = readJob(); + return job?.status === "running" && job.threadId && job.turnId ? job : null; + }, { timeoutMs: 15000 }); + + const brokerSession = loadBrokerSession(repo); + if (!brokerSession) { + return; + } + + process.kill(brokerSession.pid, "SIGKILL"); + + const failedJob = await waitFor(() => { + const job = readJob(); + return job?.status === "failed" ? job : null; + }, { timeoutMs: 15000 }); + + assert.equal(failedJob.pid, null); + assert.ok(failedJob.completedAt); + assert.ok(failedJob.errorMessage, "failed job should record why it failed"); + + const storedJob = JSON.parse(fs.readFileSync(path.join(stateDir, "jobs", `${jobId}.json`), "utf8")); + assert.equal(storedJob.status, "failed"); + assert.ok(storedJob.errorMessage); +}); + +test("cancel records the terminal state even when the job log is unwritable", async (t) => { + const repo = makeTempDir(); + initGitRepo(repo); + fs.writeFileSync(path.join(repo, "README.md"), "hello\n"); + run("git", ["add", "README.md"], { cwd: repo }); + run("git", ["commit", "-m", "init"], { cwd: repo }); + + const stateDir = resolveStateDir(repo); + const jobsDir = path.join(stateDir, "jobs"); + fs.mkdirSync(jobsDir, { recursive: true }); + + const sleeper = spawn(process.execPath, ["-e", "setInterval(() => {}, 1000)"], { + cwd: repo, + detached: true, + stdio: "ignore" + }); + sleeper.unref(); + t.after(() => { + try { + process.kill(sleeper.pid, "SIGTERM"); + } catch { + // Ignore missing process. + } + }); + + const unwritableLog = path.join(stateDir, "missing-dir", "task.log"); + const job = { + id: "task-unwritable-log", + status: "running", + title: "Codex Task", + sessionId: "sess-current", + pid: sleeper.pid, + logFile: unwritableLog, + createdAt: "2026-03-18T15:30:00.000Z", + updatedAt: "2026-03-18T15:31:00.000Z" + }; + fs.writeFileSync(path.join(jobsDir, `${job.id}.json`), `${JSON.stringify(job, null, 2)}\n`, "utf8"); + fs.writeFileSync( + path.join(stateDir, "state.json"), + `${JSON.stringify({ version: 1, config: { stopReviewGate: false }, jobs: [job] }, null, 2)}\n`, + "utf8" + ); + + const result = run("node", [SCRIPT, "cancel", job.id, "--json"], { + cwd: repo, + env: process.env + }); + + assert.equal(result.status, 0, result.stderr); + const payload = JSON.parse(result.stdout); + assert.equal(payload.status, "cancelled"); + + const state = JSON.parse(fs.readFileSync(path.join(stateDir, "state.json"), "utf8")); + const cancelled = state.jobs.find((candidate) => candidate.id === job.id); + assert.equal(cancelled.status, "cancelled"); + assert.ok(cancelled.completedAt); + + const storedJob = JSON.parse(fs.readFileSync(path.join(jobsDir, `${job.id}.json`), "utf8")); + assert.equal(storedJob.status, "cancelled"); +}); + 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(); diff --git a/tests/state.test.mjs b/tests/state.test.mjs index 0f8f57ce..d889616b 100644 --- a/tests/state.test.mjs +++ b/tests/state.test.mjs @@ -103,3 +103,109 @@ test("saveState prunes dropped job artifacts when indexed jobs exceed the cap", .sort() ); }); + +test("saveState never prunes active jobs, however old their records are", () => { + const workspace = makeTempDir(); + const stateFile = resolveStateFile(workspace); + fs.mkdirSync(path.dirname(stateFile), { recursive: true }); + + const jobs = Array.from({ length: 52 }, (_, index) => { + const jobId = `job-${index}`; + const updatedAt = new Date(Date.UTC(2026, 0, 1, 0, index, 0)).toISOString(); + const jobFile = resolveJobFile(workspace, jobId); + // The two oldest records belong to another session's in-flight work; + // pruning them would hide the jobs from the session-end broker guard. + const status = index <= 1 ? "running" : "completed"; + fs.writeFileSync(jobFile, JSON.stringify({ id: jobId, status }, null, 2), "utf8"); + return { + id: jobId, + status, + updatedAt, + createdAt: updatedAt + }; + }); + + const savedState = saveState(workspace, { + version: 1, + config: { stopReviewGate: false }, + jobs + }); + + assert.equal(savedState.jobs.length, 50); + const retainedIds = new Set(savedState.jobs.map((job) => job.id)); + assert.equal(retainedIds.has("job-0"), true, "oldest running job must survive the prune"); + assert.equal(retainedIds.has("job-1"), true, "second running job must survive the prune"); + assert.equal(retainedIds.has("job-2"), false, "oldest terminal jobs age out instead"); + assert.equal(retainedIds.has("job-3"), false); + assert.equal(fs.existsSync(resolveJobFile(workspace, "job-0")), true); +}); + +test("saveState keeps a newly terminal job even when active jobs fill the cap", () => { + const workspace = makeTempDir(); + const stateFile = resolveStateFile(workspace); + fs.mkdirSync(path.dirname(stateFile), { recursive: true }); + + const jobs = Array.from({ length: 51 }, (_, index) => { + const jobId = `job-${index}`; + const updatedAt = new Date(Date.UTC(2026, 0, 1, 0, index, 0)).toISOString(); + // The newest record is a job that just finished among 50 active peers; + // it must not vanish the moment it completes. + const status = index === 50 ? "completed" : "running"; + fs.writeFileSync(resolveJobFile(workspace, jobId), JSON.stringify({ id: jobId, status }, null, 2), "utf8"); + return { id: jobId, status, updatedAt, createdAt: updatedAt }; + }); + + const savedState = saveState(workspace, { + version: 1, + config: { stopReviewGate: false }, + jobs + }); + + const retainedIds = new Set(savedState.jobs.map((job) => job.id)); + assert.equal(retainedIds.has("job-50"), true, "the newly completed job must survive the prune"); + assert.equal(fs.existsSync(resolveJobFile(workspace, "job-50")), true); + assert.equal(savedState.jobs.length, 51, "all 50 active jobs plus the fresh terminal record are retained"); +}); + +test("writeJobFile never exposes a torn record to a concurrent reader", async (t) => { + const { writeJobFile, readJobFile } = await import("../plugins/codex/scripts/lib/state.mjs"); + const workspace = makeTempDir(); + const jobId = "job-atomic"; + const jobFile = resolveJobFile(workspace, jobId); + fs.mkdirSync(path.dirname(jobFile), { recursive: true }); + // A payload large enough that a truncate-in-place write leaves a torn + // window a concurrent reader can observe (the enqueue rewrites this file + // while the detached worker's startup reads it). + const payload = { id: jobId, status: "queued", filler: "x".repeat(64 * 1024) }; + writeJobFile(workspace, jobId, payload); + + const { spawn } = await import("node:child_process"); + const readerScript = ` + const fs = require("node:fs"); + const deadline = Date.now() + 2000; + let reads = 0; + while (Date.now() < deadline) { + try { + JSON.parse(fs.readFileSync(${JSON.stringify(jobFile)}, "utf8")); + reads++; + } catch (error) { + console.error("TORN-READ after " + reads + " reads: " + error.message); + process.exit(1); + } + } + console.log(reads); + `; + const reader = spawn(process.execPath, ["-e", readerScript], { stdio: ["ignore", "pipe", "pipe"] }); + let readerErr = ""; + reader.stderr.on("data", (chunk) => (readerErr += chunk)); + const done = new Promise((resolve) => reader.on("exit", resolve)); + + const writeDeadline = Date.now() + 1900; + while (Date.now() < writeDeadline) { + writeJobFile(workspace, jobId, { ...payload, updatedAt: new Date().toISOString() }); + } + + const exitCode = await done; + assert.equal(exitCode, 0, `concurrent reader observed a torn job record: ${readerErr.trim()}`); + assert.deepEqual(readJobFile(jobFile).id, jobId); +}); diff --git a/tests/tracked-jobs.test.mjs b/tests/tracked-jobs.test.mjs new file mode 100644 index 00000000..83690396 --- /dev/null +++ b/tests/tracked-jobs.test.mjs @@ -0,0 +1,323 @@ +import fs from "node:fs"; +import test from "node:test"; +import assert from "node:assert/strict"; + +import { makeTempDir } from "./helpers.mjs"; +import { claimTerminalStatus, createJobProgressUpdater, reassertTerminalClaim, runTrackedJob, waitForTurnIdentity } from "../plugins/codex/scripts/lib/tracked-jobs.mjs"; +import { readJobFile, resolveJobClaimFile, resolveJobFile, resolveStateFile, upsertJob, writeJobFile } from "../plugins/codex/scripts/lib/state.mjs"; + +// A pid that cannot belong to a live process on any supported platform. +const DEAD_PID = 999999999; + +function successExecution() { + return { + exitStatus: 0, + payload: { ok: true }, + rendered: "done", + summary: "done", + threadId: "thr_1", + turnId: "turn_1" + }; +} + +test("runTrackedJob does not resurrect a job cancelled before the worker started", async () => { + const workspaceRoot = makeTempDir(); + const job = { id: "task-cancelled-early", workspaceRoot, title: "Codex Task" }; + const cancelledRecord = { ...job, status: "cancelled", phase: "cancelled", pid: null }; + writeJobFile(workspaceRoot, job.id, cancelledRecord); + upsertJob(workspaceRoot, cancelledRecord); + + let runnerCalled = false; + await assert.rejects( + runTrackedJob(job, async () => { + runnerCalled = true; + return successExecution(); + }), + /cancelled before it started/ + ); + + assert.equal(runnerCalled, false, "runner must not execute for a cancelled job"); + const stored = readJobFile(resolveJobFile(workspaceRoot, job.id)); + assert.equal(stored.status, "cancelled"); +}); + +test("runTrackedJob keeps a cancellation recorded while the turn was finishing", async () => { + const workspaceRoot = makeTempDir(); + const job = { id: "task-cancelled-mid-turn", workspaceRoot, title: "Codex Task" }; + + const execution = await runTrackedJob(job, async () => { + // Simulate `cancel` landing while the worker awaits the turn outcome: + // it persists the terminal cancelled record before interrupting. + const cancelledRecord = { ...job, status: "cancelled", phase: "cancelled", pid: null }; + writeJobFile(workspaceRoot, job.id, cancelledRecord); + upsertJob(workspaceRoot, cancelledRecord); + return successExecution(); + }); + + assert.equal(execution.exitStatus, 0, "the worker still returns its execution result"); + const stored = readJobFile(resolveJobFile(workspaceRoot, job.id)); + assert.equal(stored.status, "cancelled", "completed must not overwrite cancelled"); + + const state = JSON.parse(fs.readFileSync(resolveStateFile(workspaceRoot), "utf8")); + const indexed = state.jobs.find((candidate) => candidate.id === job.id); + assert.equal(indexed.status, "cancelled"); +}); + +test("runTrackedJob keeps a cancellation recorded when the worker fails afterwards", async () => { + const workspaceRoot = makeTempDir(); + const job = { id: "task-cancelled-then-error", workspaceRoot, title: "Codex Task" }; + + await assert.rejects( + runTrackedJob(job, async () => { + const cancelledRecord = { ...job, status: "cancelled", phase: "cancelled", pid: null }; + writeJobFile(workspaceRoot, job.id, cancelledRecord); + upsertJob(workspaceRoot, cancelledRecord); + throw new Error("transport died"); + }), + /transport died/ + ); + + const stored = readJobFile(resolveJobFile(workspaceRoot, job.id)); + assert.equal(stored.status, "cancelled", "failed must not overwrite cancelled"); +}); + +test("runTrackedJob refuses to start over an orphaned terminal claim and repairs the record", async () => { + const workspaceRoot = makeTempDir(); + const job = { id: "task-orphaned-claim", workspaceRoot, title: "Codex Task" }; + const queuedRecord = { ...job, status: "queued", phase: "queued", pid: 4242 }; + writeJobFile(workspaceRoot, job.id, queuedRecord); + upsertJob(workspaceRoot, queuedRecord); + // A cancel claimed the terminal status but died before writing its record. + assert.equal(claimTerminalStatus(workspaceRoot, job.id), true); + + let runnerCalled = false; + await assert.rejects( + runTrackedJob(job, async () => { + runnerCalled = true; + return successExecution(); + }), + /cancelled before it started/ + ); + + assert.equal(runnerCalled, false, "runner must not execute under a taken claim"); + const stored = readJobFile(resolveJobFile(workspaceRoot, job.id)); + assert.equal(stored.status, "cancelled", "the orphaned claim must be repaired to a terminal record"); + const state = JSON.parse(fs.readFileSync(resolveStateFile(workspaceRoot), "utf8")); + assert.equal(state.jobs.find((candidate) => candidate.id === job.id)?.status, "cancelled"); +}); + +test("reassertTerminalClaim synchronizes state.json when the job file is already terminal", () => { + const workspaceRoot = makeTempDir(); + const jobId = "task-index-reverted"; + // The interleaving: worker writes its running job file, cancel claims and + // writes cancelled to both stores, then the worker's running upsert lands + // last and reverts state.json. The job file stays cancelled. + writeJobFile(workspaceRoot, jobId, { + id: jobId, + status: "cancelled", + phase: "cancelled", + pid: null, + completedAt: "2026-03-18T15:31:00.000Z", + errorMessage: "Cancelled by user." + }); + upsertJob(workspaceRoot, { id: jobId, status: "running", phase: "starting", pid: 4242 }); + assert.equal(claimTerminalStatus(workspaceRoot, jobId), true); + + reassertTerminalClaim(workspaceRoot, jobId); + + const state = JSON.parse(fs.readFileSync(resolveStateFile(workspaceRoot), "utf8")); + const indexed = state.jobs.find((candidate) => candidate.id === jobId); + assert.equal(indexed.status, "cancelled", "the index must be resynchronized to the terminal job file"); + assert.equal(indexed.pid, null); + assert.equal(indexed.errorMessage, "Cancelled by user."); +}); + +test("waitForTurnIdentity reads persisted ids even when the worker is already dead", async () => { + const workspaceRoot = makeTempDir(); + const jobId = "task-ids-then-death"; + // The worker persisted the turn identity and then exited/crashed; a + // caller whose snapshot predates the persist must still find the ids. + writeJobFile(workspaceRoot, jobId, { id: jobId, status: "running", threadId: "thr_5", turnId: "turn_5", pid: DEAD_PID }); + upsertJob(workspaceRoot, { id: jobId, status: "running" }); + + const identity = await waitForTurnIdentity(workspaceRoot, jobId, { + deadline: Date.now() + 1000, + workerPid: DEAD_PID + }); + assert.equal(identity.threadId, "thr_5"); + assert.equal(identity.turnId, "turn_5"); +}); + +test("waitForTurnIdentity keeps polling when the pid is not yet known and picks up late ids", async () => { + const workspaceRoot = makeTempDir(); + const jobId = "task-late-spawn"; + // Record-before-spawn: the snapshot the canceller holds has pid null. The + // worker publishes pid and turn identity a moment later; the wait must + // not end after one read just because the initial pid was unknown. + writeJobFile(workspaceRoot, jobId, { id: jobId, status: "queued", pid: null }); + upsertJob(workspaceRoot, { id: jobId, status: "queued", pid: null }); + + setTimeout(() => { + const record = { id: jobId, status: "running", pid: process.pid, threadId: "thr_9", turnId: "turn_9" }; + writeJobFile(workspaceRoot, jobId, record); + upsertJob(workspaceRoot, record); + }, 300); + + const identity = await waitForTurnIdentity(workspaceRoot, jobId, { + deadline: Date.now() + 2000, + workerPid: null + }); + assert.equal(identity.threadId, "thr_9"); + assert.equal(identity.turnId, "turn_9"); + assert.equal(identity.workerPid, process.pid, "the refreshed worker pid must be returned for termination"); +}); + +test("claimTerminalStatus grants the terminal status to the first claimant only", () => { + const workspaceRoot = makeTempDir(); + assert.equal(claimTerminalStatus(workspaceRoot, "task-claim"), true); + assert.equal(claimTerminalStatus(workspaceRoot, "task-claim"), false); +}); + +test("reassertTerminalClaim repairs a worker-owned orphan claim to failed, not cancelled", () => { + const workspaceRoot = makeTempDir(); + const jobId = "task-worker-died"; + const runningRecord = { id: jobId, status: "running", phase: "working", pid: DEAD_PID }; + writeJobFile(workspaceRoot, jobId, runningRecord); + upsertJob(workspaceRoot, runningRecord); + // The worker claimed the terminal status for its own completed/failed + // write, then died before the write landed: the job ran, so repairing it + // as a deliberate cancellation would misreport the outcome. + fs.writeFileSync(resolveJobClaimFile(workspaceRoot, jobId), `${DEAD_PID} worker\n`, "utf8"); + + reassertTerminalClaim(workspaceRoot, jobId); + + const stored = readJobFile(resolveJobFile(workspaceRoot, jobId)); + assert.equal(stored.status, "failed"); + assert.match(stored.errorMessage, /worker died before recording/); + // The repair cause must reach the canonical index too. + const state = JSON.parse(fs.readFileSync(resolveStateFile(workspaceRoot), "utf8")); + const indexed = state.jobs.find((candidate) => candidate.id === jobId); + assert.equal(indexed.status, "failed"); + assert.match(indexed.errorMessage ?? "", /worker died before recording/); +}); + +test("reassertTerminalClaim does not preempt a live worker finalizing its own outcome", () => { + const workspaceRoot = makeTempDir(); + const jobId = "task-live-finalizer"; + const runningRecord = { id: jobId, status: "running", phase: "working", pid: process.pid }; + writeJobFile(workspaceRoot, jobId, runningRecord); + upsertJob(workspaceRoot, runningRecord); + // The worker (this test process) claimed for its own terminal write and + // is still alive: a repair must not race its completed/failed record. + assert.equal(claimTerminalStatus(workspaceRoot, jobId, "worker"), true); + + reassertTerminalClaim(workspaceRoot, jobId); + + const stored = readJobFile(resolveJobFile(workspaceRoot, jobId)); + assert.equal(stored.status, "running", "a live finalizer must be left to write its own outcome"); +}); + +test("runTrackedJob preserves a cancellation that claimed terminal status between check and write", async () => { + const workspaceRoot = makeTempDir(); + const job = { id: "task-cancel-claim-race", workspaceRoot, title: "Codex Task" }; + + const execution = await runTrackedJob(job, async () => { + // Simulate cancel winning the terminal claim while its job-file write has + // not landed yet (or failed): the job file still says "running", so the + // stored-record check alone would let the worker record "completed". + assert.equal(claimTerminalStatus(workspaceRoot, job.id), true); + upsertJob(workspaceRoot, { id: job.id, status: "cancelled", phase: "cancelled", pid: null }); + return successExecution(); + }); + + assert.equal(execution.exitStatus, 0, "the worker still returns its execution result"); + const stored = readJobFile(resolveJobFile(workspaceRoot, job.id)); + assert.notEqual(stored.status, "completed", "the worker must not claim the terminal status"); + + const state = JSON.parse(fs.readFileSync(resolveStateFile(workspaceRoot), "utf8")); + const indexed = state.jobs.find((candidate) => candidate.id === job.id); + assert.equal(indexed.status, "cancelled"); +}); + +test("runTrackedJob repairs the records when a bare claim has no terminal write behind it", async () => { + const workspaceRoot = makeTempDir(); + const job = { id: "task-crashed-cancel", workspaceRoot, title: "Codex Task" }; + + await runTrackedJob(job, async () => { + // The cancel claimed the terminal status and then crashed before writing + // either record: nothing but the claim file exists. + assert.equal(claimTerminalStatus(workspaceRoot, job.id), true); + return successExecution(); + }); + + // Backing off without repair would leave running/stale-pid records forever. + const stored = readJobFile(resolveJobFile(workspaceRoot, job.id)); + assert.equal(stored.status, "cancelled"); + const state = JSON.parse(fs.readFileSync(resolveStateFile(workspaceRoot), "utf8")); + assert.equal(state.jobs.find((candidate) => candidate.id === job.id)?.status, "cancelled"); +}); + +test("progress updates stop once the job's terminal status has been claimed", () => { + const workspaceRoot = makeTempDir(); + const jobId = "task-claimed-progress"; + const runningRecord = { id: jobId, status: "running", phase: "working", pid: 1234 }; + writeJobFile(workspaceRoot, jobId, runningRecord); + upsertJob(workspaceRoot, runningRecord); + // A cancel claims the terminal status before writing its record; progress + // events arriving in that window must not touch the job. + assert.equal(claimTerminalStatus(workspaceRoot, jobId), true); + + const update = createJobProgressUpdater(workspaceRoot, jobId); + update({ message: "still streaming", phase: "finalizing" }); + + const stored = readJobFile(resolveJobFile(workspaceRoot, jobId)); + assert.equal(stored.phase, "working", "a claimed job must not receive further progress writes"); +}); + +test("progress updates persist the turn identity even after the terminal claim is taken", () => { + const workspaceRoot = makeTempDir(); + const jobId = "task-late-identity"; + const cancelledRecord = { id: jobId, status: "cancelled", phase: "cancelled", pid: null }; + writeJobFile(workspaceRoot, jobId, cancelledRecord); + upsertJob(workspaceRoot, cancelledRecord); + assert.equal(claimTerminalStatus(workspaceRoot, jobId), true); + + // Cancellation needs threadId/turnId to interrupt the server-side turn, + // and the worker's updater may hold the only copy: identity fields must + // land, while the live phase must not revive the terminal record. + const update = createJobProgressUpdater(workspaceRoot, jobId); + update({ message: "turn accepted", phase: "starting", threadId: "thr_7", turnId: "turn_7" }); + + const stored = readJobFile(resolveJobFile(workspaceRoot, jobId)); + assert.equal(stored.threadId, "thr_7"); + assert.equal(stored.turnId, "turn_7"); + assert.equal(stored.phase, "cancelled", "the live phase must not revive the terminal record"); + const state = JSON.parse(fs.readFileSync(resolveStateFile(workspaceRoot), "utf8")); + const indexed = state.jobs.find((candidate) => candidate.id === jobId); + assert.equal(indexed.turnId, "turn_7"); + assert.equal(indexed.status, "cancelled"); +}); + +test("progress updates do not touch a job that already reached a terminal status", () => { + const workspaceRoot = makeTempDir(); + const jobId = "task-cancelled-progress"; + const cancelledRecord = { + id: jobId, + status: "cancelled", + phase: "cancelled", + pid: null, + updatedAt: "2026-03-18T15:30:00.000Z" + }; + writeJobFile(workspaceRoot, jobId, cancelledRecord); + upsertJob(workspaceRoot, cancelledRecord); + const stateBefore = fs.readFileSync(resolveStateFile(workspaceRoot), "utf8"); + + const update = createJobProgressUpdater(workspaceRoot, jobId); + // Phase-only event: turn identity fields are the one thing that may still + // land on a terminal record (covered by a separate test). + update({ message: "interrupt landed", phase: "finalizing" }); + + const stored = readJobFile(resolveJobFile(workspaceRoot, jobId)); + assert.equal(stored.phase, "cancelled", "a terminal record must not regain a live phase"); + assert.equal(fs.readFileSync(resolveStateFile(workspaceRoot), "utf8"), stateBefore, "state.json must stay untouched"); +});