From b2b14fccaa1955a1f48fe81ff31003d24ab7e5f4 Mon Sep 17 00:00:00 2001 From: Yuan Chen Date: Sun, 9 Aug 2026 18:33:10 -0700 Subject: [PATCH] fix: stop session end from tearing down the shared broker under other sessions' jobs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Any session ending (including /clear) shut down the workspace-shared broker while other sessions' background jobs were mid-turn: the turn was aborted, the detached worker hung forever on the dead socket, and the job eventually vanished with no terminal record (#612). - SessionEnd now skips broker shutdown/teardown while another session has queued or running jobs; any active job with a recorded worker pid is liveness-checked, so a dead worker (e.g. one that crashed at startup, leaving a permanently queued record) cannot pin the broker. Liveness is authoritative — a live worker is never expired by record age; only pid-less records, which have no liveness signal, fall back to a one-day staleness bound. SessionEnd also reaps dead workers' active records to failed — interrupting their persisted server-side turns first, since a failed record leaves the active set and nothing else would ever interrupt them (the ending session's own jobs are excluded so its cleanup can retain cancelled records for them) — so zombie records cannot accumulate or — after pid reuse — pin the broker indefinitely. The last session out still tears everything down. - Active records are never pruned from state.json by the job cap, so long-lived in-flight work stays visible to the guard; 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. - The broker refuses broker/shutdown while any other client is connected (not merely mid-request), stops listening before it begins tearing the app-server down (so a worker cannot be admitted mid-shutdown), and SessionEnd honors the refusal, so work admitted between the guard check and the shutdown request cannot be killed. Peer shutdown requesters are excluded from the busy check (with one client-side retry for the ordering race), so two session-end hooks racing after all jobs are done cannot refuse each other into leaking the idle broker, and killed workers and live finalizers get a short exit grace (SIGKILL plus forced claim repair for stragglers and for finalizers that die during the grace without recording an outcome, whose failed records are retained rather than erased with the session's finished jobs; a finalizer that died between its job-file write and the index update has the index converged to the file's terminal outcome instead of leaving a running record with a dead pid) so a dying or wedged worker's lingering socket cannot veto the final teardown, and the session record is only cleared when it still describes the broker that was torn down, so a racing survivor's freshly spawned broker is never orphaned. An ambiguous shutdown outcome (connection lost mid-exchange, where the refusal may have been dropped) fails closed: only a confirmed shutdown or an unreachable endpoint proceeds to teardown. - Session-end job cleanup marks the ending session's in-flight jobs cancelled instead of erasing them, and interrupts their brokered turns before killing the relay workers: killing the worker alone left the turn running (and editing files) inside the surviving shared app-server. The cancelled record is persisted per job before the interrupt (record-first, like cancel, with the worker's fresher pid and turn identity captured from the job file — both before claiming and again from the post-claim reread, which can hold the only copy when the worker published them while the claim was being taken — before the terminal patch nulls the pid field); the interrupts share a deadline well inside the SessionEnd hook timeout that covers the connect as well as the RPC (identity waits are capped per job with a reserve so they cannot starve the interrupts, every interrupt the hook may still send — dead-worker reaps included — gets an equal share of the remaining budget so one hung interrupt RPC cannot consume the shared deadline and leave later jobs killed (or reaped) without any interrupt attempt, and the hook skips the synchronous codex availability probe entirely), and a timed-out interrupt tears its connection down — including a client still wedged inside connect/initialize — so an abandoned request cannot linger on the broker or keep the caller alive. The final state rewrite is a fresh read-modify-write, so records other sessions create while the interrupts are awaited are not clobbered by a stale snapshot. A job whose worker is concurrently recording its own outcome is left intact rather than having its files deleted under the live worker, while an orphaned claim (owner died before writing its record) is adopted so the job still reaches a terminal record and its worker is killed; the claim's recorded intent decides that record (failed for a dead worker's own claim, cancelled otherwise), a live finalizer is never preempted, the repair cause is written to both the job file and the index, and the cancel command's text output reports the adopted outcome instead of a cancellation stub. - Terminal job status is claimed atomically through a first-writer-wins claim file that records the claimant's intent, closing the window where a cancellation recorded between the worker's cancelled-check and its terminal write was overwritten with completed/failed. The worker re-checks the claim after writing its running record (and refuses to start over a taken claim, repairing it to a terminal record), so a cancel landing during startup cannot be resurrected to running; the repair synchronizes state.json when the job file is already terminal, and repairs a worker-owned orphan claim to failed rather than misreporting a cancellation. The progress updater re-checks the claim after its writes, so a still-streaming turn cannot revert a cancelled record. A worker that loses the claim mid-turn repairs a bare claim whose record writes never landed (claimant crashed mid-cancel) instead of leaving running records behind, and the turn id is persisted from the turn/start response itself — landing even on an already-claimed record (merged onto a fresh read so a stale snapshot cannot revert a terminal record), with cancel and session-end polling both stores for it (reading once before the worker-liveness gate, refreshing the worker pid itself so a record-before-spawn snapshot's null pid neither ends the wait early nor sends the kill to nothing, and keeping ids persisted by a worker that then died) while bounded by their budgets, and terminal record writes re-reading at write time so a stale snapshot cannot erase a freshly persisted identity — so cleanup can interrupt a turn even when the cancellation raced the turn's startup. - captureTurn races turn completion against transport death, so a worker whose broker/app-server dies mid-turn fails fast and writes a terminal failed record instead of hanging; a clean close that lands after the final answer but before the inferred-completion timer now completes the turn instead of failing it. - cancel claims the terminal status and persists the cancelled record before interrupting the turn; log appends and the worker kill are best-effort and the turn interrupt is bounded by the same timeout mechanism as session cleanup, so a crash partway through can no longer interrupt a turn while recording nothing, and a failed kill can no longer crash the CLI after the cancellation was already recorded. Both the JSON payload (workerTerminated) and the rendered report surface a failed kill, and a cancel that loses the terminal claim to a live finalizer reports the benign already-finished outcome instead of failing the command — converging a stale running index to the job file's terminal outcome when the finished worker died before its state.json write, so the job does not stay listed as running with a dead pid until the next session end — while an orphaned claim (owner died before writing its record) is repaired and the cancel proceeds, so a hung worker stays cancellable. - Enqueueing a background task persists the queued record before spawning the worker, closing the window in which the new job had neither a state record nor a broker socket and a racing session end passed every guard; a failed spawn records a failed job instead of leaving a pid-less queued record that would pin the broker forever, and the spawn 'error' event no longer crashes the CLI. - writeJobFile writes through a temp file and renames it into place, so a concurrent reader never observes a torn record. The enqueue's post-spawn pid-merge rewrite of the job file lands exactly when the detached worker's startup reads it; with a truncate-in-place write the worker could read partial JSON, die before recording anything, and leave the job queued forever with a dead pid. Fixes #612 Signed-off-by: Yuan Chen --- plugins/codex/scripts/app-server-broker.mjs | 33 +- plugins/codex/scripts/codex-companion.mjs | 237 +- plugins/codex/scripts/lib/app-server.mjs | 40 +- plugins/codex/scripts/lib/broker-endpoint.mjs | 2 + .../codex/scripts/lib/broker-lifecycle.mjs | 51 +- plugins/codex/scripts/lib/codex.mjs | 170 +- plugins/codex/scripts/lib/process.mjs | 13 + plugins/codex/scripts/lib/render.mjs | 14 +- plugins/codex/scripts/lib/state.mjs | 47 +- plugins/codex/scripts/lib/tracked-jobs.mjs | 237 +- .../codex/scripts/session-lifecycle-hook.mjs | 409 ++- tests/capture-turn.test.mjs | 71 + tests/fake-codex-fixture.mjs | 21 +- tests/interrupt-turn.test.mjs | 50 + tests/render.test.mjs | 21 +- tests/runtime.test.mjs | 2354 ++++++++++++++++- tests/state.test.mjs | 106 + tests/tracked-jobs.test.mjs | 323 +++ 18 files changed, 4105 insertions(+), 94 deletions(-) create mode 100644 tests/capture-turn.test.mjs create mode 100644 tests/interrupt-turn.test.mjs create mode 100644 tests/tracked-jobs.test.mjs 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"); +});