From 6b102b3d295d092e1c6866715b98b562b3d46d23 Mon Sep 17 00:00:00 2001 From: Praveen Mittal Date: Wed, 19 Aug 2026 08:28:11 +0200 Subject: [PATCH 01/30] fix: SessionEnd/status lookups miss state written under a different CLAUDE_PLUGIN_DATA root, orphaning brokers resolveStateDir() picks the state root from CLAUDE_PLUGIN_DATA when set, falling back to $TMPDIR/codex-companion when it's absent -- same workspace slug/hash either way, only the root differs. State written under one root (e.g. a broker registered while the var was unset) becomes invisible to any later lookup that resolves to the other root, since nothing checked both. For a broker specifically, that means SessionEnd never finds it to shut down -- it's orphaned permanently, and since ensureBrokerSession also can't see it, the next session spawns a duplicate broker for the same workspace, compounding the leak. The same mechanism affects job/status state (state.json, individual job detail files), not just the broker. Reads now check every candidate root (current primary, then the tmpdir fallback), not just the current invocation's primary -- writes are unchanged, still going to the primary root. Applied to loadState() (job list, status, config), readStoredJob() (individual job detail lookups), and loadBrokerSession()/clearBrokerSession(). Known remaining asymmetry: this fixes the direction with concrete evidence in the issue -- state written while CLAUDE_PLUGIN_DATA was unset, later missed by a lookup that has it set. The reverse isn't fixable this way: an unset env var carries no trace of what value it previously held, so there's nothing to check beyond the always-known tmpdir fallback. Fixes #636 --- .../codex/scripts/lib/broker-lifecycle.mjs | 37 ++++++---- plugins/codex/scripts/lib/job-control.mjs | 11 +-- plugins/codex/scripts/lib/state.mjs | 59 ++++++++++++++-- tests/broker-lifecycle.test.mjs | 68 +++++++++++++++++++ tests/state.test.mjs | 61 ++++++++++++++++- 5 files changed, 212 insertions(+), 24 deletions(-) create mode 100644 tests/broker-lifecycle.test.mjs diff --git a/plugins/codex/scripts/lib/broker-lifecycle.mjs b/plugins/codex/scripts/lib/broker-lifecycle.mjs index ef763819c..9d90f4cf7 100644 --- a/plugins/codex/scripts/lib/broker-lifecycle.mjs +++ b/plugins/codex/scripts/lib/broker-lifecycle.mjs @@ -6,7 +6,7 @@ import process from "node:process"; import { spawn } from "node:child_process"; import { fileURLToPath } from "node:url"; import { createBrokerEndpoint, parseBrokerEndpoint } from "./broker-endpoint.mjs"; -import { resolveStateDir } from "./state.mjs"; +import { resolveStateDir, resolveStateDirCandidates } from "./state.mjs"; export const PID_FILE_ENV = "CODEX_COMPANION_APP_SERVER_PID_FILE"; export const LOG_FILE_ENV = "CODEX_COMPANION_APP_SERVER_LOG_FILE"; @@ -73,17 +73,27 @@ function resolveBrokerStateFile(cwd) { return path.join(resolveStateDir(cwd), BROKER_STATE_FILE); } -export function loadBrokerSession(cwd) { - const stateFile = resolveBrokerStateFile(cwd); - if (!fs.existsSync(stateFile)) { - return null; - } +// The state root is derived from ambient environment (CLAUDE_PLUGIN_DATA), +// which can differ between the invocation that registered a broker and a +// later one that looks it up -- checking every candidate root, not just the +// current invocation's primary, is what keeps a broker registered under one +// root from being orphaned by a lookup that resolves to the other. +function resolveBrokerStateFileCandidates(cwd) { + return resolveStateDirCandidates(cwd).map((stateDir) => path.join(stateDir, BROKER_STATE_FILE)); +} - try { - return JSON.parse(fs.readFileSync(stateFile, "utf8")); - } catch { - return null; +export function loadBrokerSession(cwd) { + for (const stateFile of resolveBrokerStateFileCandidates(cwd)) { + if (!fs.existsSync(stateFile)) { + continue; + } + try { + return JSON.parse(fs.readFileSync(stateFile, "utf8")); + } catch { + continue; + } } + return null; } export function saveBrokerSession(cwd, session) { @@ -93,9 +103,10 @@ export function saveBrokerSession(cwd, session) { } export function clearBrokerSession(cwd) { - const stateFile = resolveBrokerStateFile(cwd); - if (fs.existsSync(stateFile)) { - fs.unlinkSync(stateFile); + for (const stateFile of resolveBrokerStateFileCandidates(cwd)) { + if (fs.existsSync(stateFile)) { + fs.unlinkSync(stateFile); + } } } diff --git a/plugins/codex/scripts/lib/job-control.mjs b/plugins/codex/scripts/lib/job-control.mjs index ad152c157..f0638bf61 100644 --- a/plugins/codex/scripts/lib/job-control.mjs +++ b/plugins/codex/scripts/lib/job-control.mjs @@ -1,7 +1,7 @@ import fs from "node:fs"; import { getSessionRuntimeStatus } from "./codex.mjs"; -import { getConfig, listJobs, readJobFile, resolveJobFile } from "./state.mjs"; +import { getConfig, listJobs, readJobFile, resolveJobFileCandidates } from "./state.mjs"; import { SESSION_ID_ENV } from "./tracked-jobs.mjs"; import { resolveWorkspaceRoot } from "./workspace.mjs"; @@ -181,11 +181,12 @@ export function enrichJob(job, options = {}) { } export function readStoredJob(workspaceRoot, jobId) { - const jobFile = resolveJobFile(workspaceRoot, jobId); - if (!fs.existsSync(jobFile)) { - return null; + for (const jobFile of resolveJobFileCandidates(workspaceRoot, jobId)) { + if (fs.existsSync(jobFile)) { + return readJobFile(jobFile); + } } - return readJobFile(jobFile); + return null; } function matchJobReference(jobs, reference, predicate = () => true) { diff --git a/plugins/codex/scripts/lib/state.mjs b/plugins/codex/scripts/lib/state.mjs index 2da23498f..c2c034f24 100644 --- a/plugins/codex/scripts/lib/state.mjs +++ b/plugins/codex/scripts/lib/state.mjs @@ -26,7 +26,7 @@ function defaultState() { }; } -export function resolveStateDir(cwd) { +function workspaceStateDirName(cwd) { const workspaceRoot = resolveWorkspaceRoot(cwd); let canonicalWorkspaceRoot = workspaceRoot; try { @@ -38,9 +38,37 @@ export function resolveStateDir(cwd) { const slugSource = path.basename(workspaceRoot) || "workspace"; const slug = slugSource.replace(/[^a-zA-Z0-9._-]+/g, "-").replace(/^-+|-+$/g, "") || "workspace"; const hash = createHash("sha256").update(canonicalWorkspaceRoot).digest("hex").slice(0, 16); + return `${slug}-${hash}`; +} + +// CLAUDE_PLUGIN_DATA is only present when the current invocation runs as a +// plugin hook; a directly-invoked CLI call (or a hook whose env didn't +// propagate it) resolves to the tmpdir fallback instead. Since the state +// root is derived from ambient environment rather than anything persisted, +// two invocations for the *same* workspace can land on different roots -- +// the primary root is still the write target for new/updated state, but +// reads check every candidate so state written under one root is never +// invisible to a later invocation that resolves to the other. +function stateRootCandidates() { const pluginDataDir = process.env[PLUGIN_DATA_ENV]; - const stateRoot = pluginDataDir ? path.join(pluginDataDir, "state") : FALLBACK_STATE_ROOT_DIR; - return path.join(stateRoot, `${slug}-${hash}`); + return pluginDataDir + ? [path.join(pluginDataDir, "state"), FALLBACK_STATE_ROOT_DIR] + : [FALLBACK_STATE_ROOT_DIR]; +} + +export function resolveStateDir(cwd) { + const [primaryRoot] = stateRootCandidates(); + return path.join(primaryRoot, workspaceStateDirName(cwd)); +} + +/** + * All directories that could hold this workspace's state, primary root + * first. Use for reads that must not miss state written under a different + * root than the current invocation resolves to. + */ +export function resolveStateDirCandidates(cwd) { + const dirName = workspaceStateDirName(cwd); + return stateRootCandidates().map((root) => path.join(root, dirName)); } export function resolveStateFile(cwd) { @@ -55,9 +83,19 @@ export function ensureStateDir(cwd) { fs.mkdirSync(resolveJobsDir(cwd), { recursive: true }); } +function resolveExistingStateFile(cwd) { + for (const stateDir of resolveStateDirCandidates(cwd)) { + const stateFile = path.join(stateDir, STATE_FILE_NAME); + if (fs.existsSync(stateFile)) { + return stateFile; + } + } + return null; +} + export function loadState(cwd) { - const stateFile = resolveStateFile(cwd); - if (!fs.existsSync(stateFile)) { + const stateFile = resolveExistingStateFile(cwd); + if (!stateFile) { return defaultState(); } @@ -189,3 +227,14 @@ export function resolveJobFile(cwd, jobId) { ensureStateDir(cwd); return path.join(resolveJobsDir(cwd), `${jobId}.json`); } + +/** + * Every path a job's detail file could be at, primary root first. A job + * listed via loadState()/listJobs() (which already searches every + * candidate root) may have had its detail file written under a different + * root than resolveJobFile()'s current primary; read lookups should not + * miss it just because it isn't in the root a fresh call resolves to. + */ +export function resolveJobFileCandidates(cwd, jobId) { + return resolveStateDirCandidates(cwd).map((stateDir) => path.join(stateDir, JOBS_DIR_NAME, `${jobId}.json`)); +} diff --git a/tests/broker-lifecycle.test.mjs b/tests/broker-lifecycle.test.mjs new file mode 100644 index 000000000..a4f70b0ed --- /dev/null +++ b/tests/broker-lifecycle.test.mjs @@ -0,0 +1,68 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +import { makeTempDir } from "./helpers.mjs"; +import { + clearBrokerSession, + loadBrokerSession, + saveBrokerSession +} from "../plugins/codex/scripts/lib/broker-lifecycle.mjs"; + +function withPluginDataDir(pluginDataDir, fn) { + const previous = process.env.CLAUDE_PLUGIN_DATA; + if (pluginDataDir == null) { + delete process.env.CLAUDE_PLUGIN_DATA; + } else { + process.env.CLAUDE_PLUGIN_DATA = pluginDataDir; + } + try { + return fn(); + } finally { + if (previous == null) { + delete process.env.CLAUDE_PLUGIN_DATA; + } else { + process.env.CLAUDE_PLUGIN_DATA = previous; + } + } +} + +// A broker registered while CLAUDE_PLUGIN_DATA is unset (the tmpdir +// fallback) can later be looked up by an invocation where it's set, and +// resolves the same workspace slug/hash -- only the root differs, and a +// lookup that only checks the current invocation's root orphans the broker. +// This is the direction with concrete real-world evidence in the issue. The +// reverse isn't fixable this way: an unset env var carries no trace of what +// value it previously held, so there's nothing to check beyond the +// always-known tmpdir fallback. +test("loadBrokerSession finds a session registered without CLAUDE_PLUGIN_DATA when the current invocation has it set", () => { + const workspace = makeTempDir(); + const pluginDataDir = makeTempDir(); + + withPluginDataDir(null, () => { + saveBrokerSession(workspace, { endpoint: "test-endpoint", pid: 1234 }); + }); + + const session = withPluginDataDir(pluginDataDir, () => loadBrokerSession(workspace)); + + assert.deepEqual(session, { endpoint: "test-endpoint", pid: 1234 }); +}); + +test("clearBrokerSession removes a session that was registered without CLAUDE_PLUGIN_DATA, from an invocation that has it set", () => { + const workspace = makeTempDir(); + const pluginDataDir = makeTempDir(); + + withPluginDataDir(null, () => { + saveBrokerSession(workspace, { endpoint: "test-endpoint", pid: 1234 }); + }); + + withPluginDataDir(pluginDataDir, () => { + clearBrokerSession(workspace); + assert.equal(loadBrokerSession(workspace), null); + }); + + // Confirm it's gone from the root it was actually written under too, not + // just invisible from the other one. + withPluginDataDir(null, () => { + assert.equal(loadBrokerSession(workspace), null); + }); +}); diff --git a/tests/state.test.mjs b/tests/state.test.mjs index 0f8f57cea..a088b9557 100644 --- a/tests/state.test.mjs +++ b/tests/state.test.mjs @@ -5,7 +5,15 @@ import test from "node:test"; import assert from "node:assert/strict"; import { makeTempDir } from "./helpers.mjs"; -import { resolveJobFile, resolveJobLogFile, resolveStateDir, resolveStateFile, saveState } from "../plugins/codex/scripts/lib/state.mjs"; +import { + loadState, + resolveJobFile, + resolveJobLogFile, + resolveStateDir, + resolveStateFile, + saveState +} from "../plugins/codex/scripts/lib/state.mjs"; +import { readStoredJob } from "../plugins/codex/scripts/lib/job-control.mjs"; test("resolveStateDir uses a temp-backed per-workspace directory", () => { const workspace = makeTempDir(); @@ -40,6 +48,57 @@ test("resolveStateDir uses CLAUDE_PLUGIN_DATA when it is provided", () => { } }); +// The reverse (state written *with* CLAUDE_PLUGIN_DATA set, later read with +// it unset) isn't fixable this way: an unset env var carries no trace of +// what value it previously held, so there's nothing to check beyond the +// always-known tmpdir fallback. This direction is the one with concrete +// real-world evidence in the issue (a broker registered under the tmpdir +// fallback, later orphaned by a lookup that ran with CLAUDE_PLUGIN_DATA set). +test("loadState finds state written without CLAUDE_PLUGIN_DATA when the current invocation has it set", () => { + const workspace = makeTempDir(); + const pluginDataDir = makeTempDir(); + const previousPluginDataDir = process.env.CLAUDE_PLUGIN_DATA; + + try { + delete process.env.CLAUDE_PLUGIN_DATA; + saveState(workspace, { config: { stopReviewGate: true }, jobs: [] }); + + process.env.CLAUDE_PLUGIN_DATA = pluginDataDir; + const state = loadState(workspace); + + assert.equal(state.config.stopReviewGate, true); + } finally { + if (previousPluginDataDir == null) { + delete process.env.CLAUDE_PLUGIN_DATA; + } else { + process.env.CLAUDE_PLUGIN_DATA = previousPluginDataDir; + } + } +}); + +test("readStoredJob finds a job's detail file written without CLAUDE_PLUGIN_DATA when the current invocation has it set", () => { + const workspace = makeTempDir(); + const pluginDataDir = makeTempDir(); + const previousPluginDataDir = process.env.CLAUDE_PLUGIN_DATA; + + try { + delete process.env.CLAUDE_PLUGIN_DATA; + const jobFile = resolveJobFile(workspace, "job-1"); + fs.writeFileSync(jobFile, JSON.stringify({ id: "job-1", status: "completed" }), "utf8"); + + process.env.CLAUDE_PLUGIN_DATA = pluginDataDir; + const job = readStoredJob(workspace, "job-1"); + + assert.deepEqual(job, { id: "job-1", status: "completed" }); + } finally { + if (previousPluginDataDir == null) { + delete process.env.CLAUDE_PLUGIN_DATA; + } else { + process.env.CLAUDE_PLUGIN_DATA = previousPluginDataDir; + } + } +}); + test("saveState prunes dropped job artifacts when indexed jobs exceed the cap", () => { const workspace = makeTempDir(); const stateFile = resolveStateFile(workspace); From baa5ffdcfe7db046cbbe2ebeda39c4f95e691b69 Mon Sep 17 00:00:00 2001 From: Praveen Mittal Date: Wed, 19 Aug 2026 08:36:48 +0200 Subject: [PATCH 02/30] fix: clearBrokerSession only clears the record loadBrokerSession() returned Both call sites (handleSessionEnd, ensureBrokerSession) act on whatever loadBrokerSession() returns -- tearing that broker down and clearing its record -- but clearBrokerSession deleted every candidate root's broker.json, not just the one that was actually torn down. That's reachable in practice: it's precisely the root-split bug's own historical fallout, where the old lookup could leave a broker registered under one root while a duplicate got spawned under the other. Deleting both records on the next cleanup erases the untorn broker's only metadata, making it permanently untrackable instead of leaving a stale-but-discoverable file behind. Thanks to Codex Review for catching this. --- .../codex/scripts/lib/broker-lifecycle.mjs | 10 +++++ tests/broker-lifecycle.test.mjs | 38 +++++++++++++++++++ 2 files changed, 48 insertions(+) diff --git a/plugins/codex/scripts/lib/broker-lifecycle.mjs b/plugins/codex/scripts/lib/broker-lifecycle.mjs index 9d90f4cf7..c8a6fa8fe 100644 --- a/plugins/codex/scripts/lib/broker-lifecycle.mjs +++ b/plugins/codex/scripts/lib/broker-lifecycle.mjs @@ -102,10 +102,20 @@ export function saveBrokerSession(cwd, session) { fs.writeFileSync(resolveBrokerStateFile(cwd), `${JSON.stringify(session, null, 2)}\n`, "utf8"); } +// Removes only the record loadBrokerSession() would return (the first +// existing candidate), not every candidate. Both call sites act on whatever +// loadBrokerSession() returned -- tearing that broker down and clearing its +// record -- so clearing every candidate here would delete an *other* root's +// broker.json for a broker that was never torn down (a real reachable case: +// this is precisely the root-split bug's own historical fallout, where the +// old lookup spawned a duplicate broker under the other root). Erasing that +// record makes the still-running duplicate permanently untrackable, which +// is worse than leaving a stale-but-discoverable file behind. export function clearBrokerSession(cwd) { for (const stateFile of resolveBrokerStateFileCandidates(cwd)) { if (fs.existsSync(stateFile)) { fs.unlinkSync(stateFile); + return; } } } diff --git a/tests/broker-lifecycle.test.mjs b/tests/broker-lifecycle.test.mjs index a4f70b0ed..68098a967 100644 --- a/tests/broker-lifecycle.test.mjs +++ b/tests/broker-lifecycle.test.mjs @@ -66,3 +66,41 @@ test("clearBrokerSession removes a session that was registered without CLAUDE_PL assert.equal(loadBrokerSession(workspace), null); }); }); + +// Caught in review: this is a real reachable state, not a hypothetical -- +// it's precisely what the old (pre-fix) lookup behavior could leave behind: +// a broker registered under one root, then a *different* broker later +// registered under the other root because the old code couldn't see the +// first one. Only one of the two brokers is ever the one actually acted on +// (whichever loadBrokerSession() returns) and torn down; clearBrokerSession +// must not delete the other root's record too, since that broker was never +// shut down and losing its record would make it permanently untrackable. +test("clearBrokerSession does not delete a distinct session recorded under the other root", () => { + const workspace = makeTempDir(); + const pluginDataDir = makeTempDir(); + + withPluginDataDir(null, () => { + saveBrokerSession(workspace, { endpoint: "fallback-endpoint", pid: 1111 }); + }); + withPluginDataDir(pluginDataDir, () => { + saveBrokerSession(workspace, { endpoint: "plugin-data-endpoint", pid: 2222 }); + }); + + withPluginDataDir(pluginDataDir, () => { + // loadBrokerSession() would return (and a caller would tear down) the + // plugin-data-root session, since it's checked first. + clearBrokerSession(workspace); + }); + + // The fallback-root session must survive untouched -- visible whether + // checked directly (env unset) or as the sole remaining candidate (env + // set, since the plugin-data one is now gone). If clearBrokerSession had + // wrongly deleted it too, this would come back null or the check with the + // env set would find nothing. + withPluginDataDir(null, () => { + assert.deepEqual(loadBrokerSession(workspace), { endpoint: "fallback-endpoint", pid: 1111 }); + }); + withPluginDataDir(pluginDataDir, () => { + assert.deepEqual(loadBrokerSession(workspace), { endpoint: "fallback-endpoint", pid: 1111 }); + }); +}); From dcf9384f53612a259d98c6a48cbe6d2ddea4766f Mon Sep 17 00:00:00 2001 From: Praveen Mittal Date: Wed, 19 Aug 2026 09:24:06 +0200 Subject: [PATCH 03/30] fix: merge jobs across candidate roots; make clearBrokerSession agree with loadBrokerSession on malformed files Two more issues from Codex Review, both real: 1. loadState() returned only the first candidate state.json found, not merged. Unlike a broker session (at most one meaningful record, so 'first found' is correct), jobs are a growing collection -- a job started while CLAUDE_PLUGIN_DATA was set and a different job started while it was unset are both real and non-conflicting. Returning only the first root's job list silently hid whichever root wasn't picked, for every status/result/cancel lookup, any time both roots happened to have a state.json. Now merges jobs from every candidate, keeping the more recently updated copy if the same id somehow appears in more than one. 2. loadBrokerSession() skips a candidate it can't parse and moves on, so it can return a fallback session while a malformed primary file exists. clearBrokerSession() selected by existence alone, so it could delete the unrelated malformed primary while leaving the valid fallback record behind -- the one actually loaded and torn down by the caller. Both functions now share a single selectBrokerState() helper (exists AND parses), so they always agree on which candidate is the selected one. Verified the loadState() merge fix doesn't have a side effect on saveState()'s own previousJobs cleanup diff (its per-job file removal resolves paths against the current-root-only resolveJobFile(), so a job living in another root is a no-op there, not a deletion) -- confirmed empirically with a throwaway repro before concluding no further change was needed there. Thanks again to Codex Review. --- .../codex/scripts/lib/broker-lifecycle.mjs | 46 ++++++++----- plugins/codex/scripts/lib/state.mjs | 65 ++++++++++++------- tests/broker-lifecycle.test.mjs | 38 +++++++++++ tests/state.test.mjs | 39 +++++++++++ 4 files changed, 150 insertions(+), 38 deletions(-) diff --git a/plugins/codex/scripts/lib/broker-lifecycle.mjs b/plugins/codex/scripts/lib/broker-lifecycle.mjs index c8a6fa8fe..4fd39296c 100644 --- a/plugins/codex/scripts/lib/broker-lifecycle.mjs +++ b/plugins/codex/scripts/lib/broker-lifecycle.mjs @@ -82,13 +82,22 @@ function resolveBrokerStateFileCandidates(cwd) { return resolveStateDirCandidates(cwd).map((stateDir) => path.join(stateDir, BROKER_STATE_FILE)); } -export function loadBrokerSession(cwd) { +// The single source of truth for which candidate is "the" active broker +// session: the first one that both exists *and* parses. loadBrokerSession() +// and clearBrokerSession() both build on this so they always agree -- if +// clearBrokerSession() instead selected by existence alone, a malformed +// primary file next to a valid fallback one would make it delete the +// (malformed, unused) primary while loadBrokerSession() actually returned +// and a caller tore down the fallback broker, leaving that broker's now- +// stale record behind. +function selectBrokerState(cwd) { for (const stateFile of resolveBrokerStateFileCandidates(cwd)) { if (!fs.existsSync(stateFile)) { continue; } try { - return JSON.parse(fs.readFileSync(stateFile, "utf8")); + const session = JSON.parse(fs.readFileSync(stateFile, "utf8")); + return { stateFile, session }; } catch { continue; } @@ -96,27 +105,32 @@ export function loadBrokerSession(cwd) { return null; } +export function loadBrokerSession(cwd) { + return selectBrokerState(cwd)?.session ?? null; +} + export function saveBrokerSession(cwd, session) { const stateDir = resolveStateDir(cwd); fs.mkdirSync(stateDir, { recursive: true }); fs.writeFileSync(resolveBrokerStateFile(cwd), `${JSON.stringify(session, null, 2)}\n`, "utf8"); } -// Removes only the record loadBrokerSession() would return (the first -// existing candidate), not every candidate. Both call sites act on whatever -// loadBrokerSession() returned -- tearing that broker down and clearing its -// record -- so clearing every candidate here would delete an *other* root's -// broker.json for a broker that was never torn down (a real reachable case: -// this is precisely the root-split bug's own historical fallout, where the -// old lookup spawned a duplicate broker under the other root). Erasing that -// record makes the still-running duplicate permanently untrackable, which -// is worse than leaving a stale-but-discoverable file behind. +// Removes only the record loadBrokerSession() would return, not every +// candidate. Both call sites act on whatever loadBrokerSession() returned -- +// tearing that broker down and clearing its record -- so clearing every +// candidate here would delete an *other* root's broker.json for a broker +// that was never torn down (a real reachable case: this is precisely the +// root-split bug's own historical fallout, where the old lookup spawned a +// duplicate broker under the other root). Erasing that record makes the +// still-running duplicate permanently untrackable, which is worse than +// leaving a stale-but-discoverable file behind. Built on the same +// selectBrokerState() loadBrokerSession() uses, rather than its own +// existence-only scan, so the two never disagree about which candidate is +// "the" selected one when a malformed file sits in front of a valid one. export function clearBrokerSession(cwd) { - for (const stateFile of resolveBrokerStateFileCandidates(cwd)) { - if (fs.existsSync(stateFile)) { - fs.unlinkSync(stateFile); - return; - } + const selected = selectBrokerState(cwd); + if (selected) { + fs.unlinkSync(selected.stateFile); } } diff --git a/plugins/codex/scripts/lib/state.mjs b/plugins/codex/scripts/lib/state.mjs index c2c034f24..92cb1187c 100644 --- a/plugins/codex/scripts/lib/state.mjs +++ b/plugins/codex/scripts/lib/state.mjs @@ -83,36 +83,57 @@ export function ensureStateDir(cwd) { fs.mkdirSync(resolveJobsDir(cwd), { recursive: true }); } -function resolveExistingStateFile(cwd) { - for (const stateDir of resolveStateDirCandidates(cwd)) { - const stateFile = path.join(stateDir, STATE_FILE_NAME); - if (fs.existsSync(stateFile)) { - return stateFile; - } +function readStateFileIfValid(stateFile) { + if (!fs.existsSync(stateFile)) { + return null; + } + try { + return JSON.parse(fs.readFileSync(stateFile, "utf8")); + } catch { + return null; } - return null; } +// Unlike the broker session (at most one meaningful record per workspace, +// so "first candidate found" is a correct selection), jobs are a growing +// collection that can genuinely differ across roots -- a job started while +// CLAUDE_PLUGIN_DATA was set and another started while it was unset are +// both real and non-conflicting. Returning only the first candidate's job +// list would silently hide whichever root wasn't picked, leaving the exact +// cross-root invisibility this fix targets for status/result/cancel +// whenever *both* roots happen to have a state.json (a reachable legacy +// state after invocations alternated). So every candidate's jobs are +// merged instead, keeping the more recently updated copy if the same job +// id somehow appears in more than one. export function loadState(cwd) { - const stateFile = resolveExistingStateFile(cwd); - if (!stateFile) { + const parsedCandidates = resolveStateDirCandidates(cwd) + .map((stateDir) => readStateFileIfValid(path.join(stateDir, STATE_FILE_NAME))) + .filter((parsed) => parsed != null); + + if (parsedCandidates.length === 0) { return defaultState(); } - try { - const parsed = JSON.parse(fs.readFileSync(stateFile, "utf8")); - return { - ...defaultState(), - ...parsed, - config: { - ...defaultState().config, - ...(parsed.config ?? {}) - }, - jobs: Array.isArray(parsed.jobs) ? parsed.jobs : [] - }; - } catch { - return defaultState(); + const jobsById = new Map(); + for (const parsed of parsedCandidates) { + for (const job of Array.isArray(parsed.jobs) ? parsed.jobs : []) { + const existing = jobsById.get(job.id); + if (!existing || String(job.updatedAt ?? "") > String(existing.updatedAt ?? "")) { + jobsById.set(job.id, job); + } + } } + + const [primary] = parsedCandidates; + return { + ...defaultState(), + ...primary, + config: { + ...defaultState().config, + ...(primary.config ?? {}) + }, + jobs: [...jobsById.values()] + }; } function pruneJobs(jobs) { diff --git a/tests/broker-lifecycle.test.mjs b/tests/broker-lifecycle.test.mjs index 68098a967..abb0555c3 100644 --- a/tests/broker-lifecycle.test.mjs +++ b/tests/broker-lifecycle.test.mjs @@ -1,3 +1,5 @@ +import fs from "node:fs"; +import path from "node:path"; import test from "node:test"; import assert from "node:assert/strict"; @@ -7,6 +9,7 @@ import { loadBrokerSession, saveBrokerSession } from "../plugins/codex/scripts/lib/broker-lifecycle.mjs"; +import { resolveStateDir } from "../plugins/codex/scripts/lib/state.mjs"; function withPluginDataDir(pluginDataDir, fn) { const previous = process.env.CLAUDE_PLUGIN_DATA; @@ -104,3 +107,38 @@ test("clearBrokerSession does not delete a distinct session recorded under the o assert.deepEqual(loadBrokerSession(workspace), { endpoint: "fallback-endpoint", pid: 1111 }); }); }); + +// Caught in review: loadBrokerSession() skips a candidate it can't parse and +// moves on to the next one, so it can return a *fallback* session while a +// *primary* file exists but is malformed. clearBrokerSession() must select +// by the same rule (exists AND parses), not existence alone -- otherwise it +// deletes the unrelated malformed primary while leaving the valid fallback +// record behind, even though a caller just tore down the broker that record +// points to. +test("clearBrokerSession deletes the same record loadBrokerSession() returned, not just the first existing file", () => { + const workspace = makeTempDir(); + const pluginDataDir = makeTempDir(); + + withPluginDataDir(null, () => { + saveBrokerSession(workspace, { endpoint: "fallback-endpoint", pid: 1111 }); + }); + + withPluginDataDir(pluginDataDir, () => { + const primaryBrokerFile = path.join(resolveStateDir(workspace), "broker.json"); + fs.mkdirSync(path.dirname(primaryBrokerFile), { recursive: true }); + fs.writeFileSync(primaryBrokerFile, "{not valid json", "utf8"); + + // loadBrokerSession() skips the malformed primary and returns the valid + // fallback session. + assert.deepEqual(loadBrokerSession(workspace), { endpoint: "fallback-endpoint", pid: 1111 }); + + clearBrokerSession(workspace); + + // The malformed primary file is untouched (clearBrokerSession() doesn't + // garbage-collect unrelated corrupt files, only the selected record)... + assert.equal(fs.existsSync(primaryBrokerFile), true); + // ...but the valid fallback session -- the one actually loaded and torn + // down -- is gone. + assert.equal(loadBrokerSession(workspace), null); + }); +}); diff --git a/tests/state.test.mjs b/tests/state.test.mjs index a088b9557..371b1d454 100644 --- a/tests/state.test.mjs +++ b/tests/state.test.mjs @@ -76,6 +76,45 @@ test("loadState finds state written without CLAUDE_PLUGIN_DATA when the current } }); +// Caught in review: jobs are a growing collection, not a single pointer like +// the broker session -- a job started while CLAUDE_PLUGIN_DATA was set and a +// different job started while it was unset are both real and non- +// conflicting, so loadState() must merge every candidate's jobs rather than +// returning only the first state.json found (which would silently hide +// whichever root wasn't picked, for every status/result/cancel lookup, any +// time both roots happen to have a state.json -- a reachable legacy state +// after invocations alternated). +test("loadState merges jobs from every candidate root instead of only the first found", () => { + const workspace = makeTempDir(); + const pluginDataDir = makeTempDir(); + const previousPluginDataDir = process.env.CLAUDE_PLUGIN_DATA; + + try { + delete process.env.CLAUDE_PLUGIN_DATA; + saveState(workspace, { + config: {}, + jobs: [{ id: "job-fallback", status: "running", updatedAt: "2026-08-19T00:00:00.000Z" }] + }); + + process.env.CLAUDE_PLUGIN_DATA = pluginDataDir; + saveState(workspace, { + config: {}, + jobs: [{ id: "job-plugin-data", status: "running", updatedAt: "2026-08-19T00:01:00.000Z" }] + }); + + const state = loadState(workspace); + const jobIds = state.jobs.map((job) => job.id).sort(); + + assert.deepEqual(jobIds, ["job-fallback", "job-plugin-data"]); + } finally { + if (previousPluginDataDir == null) { + delete process.env.CLAUDE_PLUGIN_DATA; + } else { + process.env.CLAUDE_PLUGIN_DATA = previousPluginDataDir; + } + } +}); + test("readStoredJob finds a job's detail file written without CLAUDE_PLUGIN_DATA when the current invocation has it set", () => { const workspace = makeTempDir(); const pluginDataDir = makeTempDir(); From e349f1c09b102f2b1e9054e37976fb262f19686a Mon Sep 17 00:00:00 2001 From: Praveen Mittal Date: Wed, 19 Aug 2026 09:31:24 +0200 Subject: [PATCH 04/30] fix: persist job deletions across every candidate state root, not just the primary saveState() only ever wrote the new job list to the current primary root. A job that originated entirely in a different root (e.g. added while CLAUDE_PLUGIN_DATA was unset) and later gets filtered out -- cleanupSessionJobs() during SessionEnd loads the merged view, drops jobs for the ending session, and saves the remainder -- never actually disappeared: that other root's own state.json still held its own untouched copy, and the very next loadState() merged it right back in. A removed job could keep reporting as running indefinitely. saveState() now also prunes every other candidate root's own file down to the same retained job-id set (derived from this save's own merged previousJobs diff), so a deletion sticks everywhere. New and updated jobs are unaffected -- they still only ever get written to the primary root, exactly as before; this only ever removes. Also made the individual job-detail-file cleanup in the same loop candidate-aware (resolveJobFileCandidates instead of the primary-only resolveJobFile), for the same reason. Two of the existing tests had to seed their two-root fixtures via direct file writes instead of two independent saveState() calls -- every real caller (updateState()/cleanupSessionJobs()) always derives its job list from a prior loadState(), so seeding via two disjoint, non-full-list saveState() calls doesn't reflect any real call pattern, and (correctly, now) tripped this very fix's own deletion logic during test setup. Thanks again to Codex Review. --- plugins/codex/scripts/lib/state.mjs | 31 ++++++++++++- tests/state.test.mjs | 70 ++++++++++++++++++++++++++++- 2 files changed, 98 insertions(+), 3 deletions(-) diff --git a/plugins/codex/scripts/lib/state.mjs b/plugins/codex/scripts/lib/state.mjs index 92cb1187c..c14e13e85 100644 --- a/plugins/codex/scripts/lib/state.mjs +++ b/plugins/codex/scripts/lib/state.mjs @@ -166,11 +166,40 @@ export function saveState(cwd, state) { if (retainedIds.has(job.id)) { continue; } - removeJobFile(resolveJobFile(cwd, job.id)); + for (const jobFile of resolveJobFileCandidates(cwd, job.id)) { + removeJobFile(jobFile); + } removeFileIfExists(job.logFile); } fs.writeFileSync(resolveStateFile(cwd), `${JSON.stringify(nextState, null, 2)}\n`, "utf8"); + + // previousJobs is the merged view across every candidate root (see + // loadState()), so a job dropped from state.jobs here may have + // originated entirely in a root other than the one just written above. + // Without this, that root's own state.json still holds its own + // untouched copy, and the very next loadState() merges it right back in + // -- deletions could never actually stick for a job that lives only in a + // non-primary root. Prune every other candidate root's own file down to + // the same retained set; new/updated jobs still only ever get written to + // the primary root, above -- this only ever removes, never adds or + // rewrites in place. + const [, ...otherStateDirs] = resolveStateDirCandidates(cwd); + for (const otherStateDir of otherStateDirs) { + const otherStateFile = path.join(otherStateDir, STATE_FILE_NAME); + const otherParsed = readStateFileIfValid(otherStateFile); + const otherJobs = Array.isArray(otherParsed?.jobs) ? otherParsed.jobs : []; + const prunedOtherJobs = otherJobs.filter((job) => retainedIds.has(job.id)); + if (prunedOtherJobs.length === otherJobs.length) { + continue; + } + fs.writeFileSync( + otherStateFile, + `${JSON.stringify({ ...otherParsed, jobs: prunedOtherJobs }, null, 2)}\n`, + "utf8" + ); + } + return nextState; } diff --git a/tests/state.test.mjs b/tests/state.test.mjs index 371b1d454..14f4656f4 100644 --- a/tests/state.test.mjs +++ b/tests/state.test.mjs @@ -84,20 +84,28 @@ test("loadState finds state written without CLAUDE_PLUGIN_DATA when the current // whichever root wasn't picked, for every status/result/cancel lookup, any // time both roots happen to have a state.json -- a reachable legacy state // after invocations alternated). +function writeStateFileDirectly(stateDir, state) { + fs.mkdirSync(stateDir, { recursive: true }); + fs.writeFileSync(path.join(stateDir, "state.json"), `${JSON.stringify(state, null, 2)}\n`, "utf8"); +} + test("loadState merges jobs from every candidate root instead of only the first found", () => { const workspace = makeTempDir(); const pluginDataDir = makeTempDir(); const previousPluginDataDir = process.env.CLAUDE_PLUGIN_DATA; try { + // Written directly (not via saveState()) so this test exercises only + // loadState()'s read-side merge, independent of saveState()'s own + // write/deletion-propagation behavior (covered separately below). delete process.env.CLAUDE_PLUGIN_DATA; - saveState(workspace, { + writeStateFileDirectly(resolveStateDir(workspace), { config: {}, jobs: [{ id: "job-fallback", status: "running", updatedAt: "2026-08-19T00:00:00.000Z" }] }); process.env.CLAUDE_PLUGIN_DATA = pluginDataDir; - saveState(workspace, { + writeStateFileDirectly(resolveStateDir(workspace), { config: {}, jobs: [{ id: "job-plugin-data", status: "running", updatedAt: "2026-08-19T00:01:00.000Z" }] }); @@ -115,6 +123,64 @@ test("loadState merges jobs from every candidate root instead of only the first } }); +// Caught in review: merging reads across roots (the previous test) isn't +// enough on its own -- saveState() only ever wrote the new job list to the +// current primary root, so a job that originated in a *different* root and +// gets filtered out (e.g. cleanupSessionJobs() during SessionEnd, which +// loads the merged view, drops jobs for the ending session, and saves the +// remainder) never actually disappears: the other root's own state.json +// still has its own untouched copy, and the next loadState() merges it +// right back in. A "removed" job could keep reporting as running forever. +test("saveState persists a job removal across every candidate root, not just the current primary", () => { + const workspace = makeTempDir(); + const pluginDataDir = makeTempDir(); + const previousPluginDataDir = process.env.CLAUDE_PLUGIN_DATA; + + try { + // Setup writes both roots directly (not via saveState()), exactly like + // the previous test -- a real caller always derives saveState()'s job + // list from a prior loadState() (see updateState()/cleanupSessionJobs() + // themselves), so seeding two roots via two independent, non-full-list + // saveState() calls wouldn't reflect any real call pattern and would + // trip the very deletion-propagation behavior under test here. + delete process.env.CLAUDE_PLUGIN_DATA; + writeStateFileDirectly(resolveStateDir(workspace), { + config: {}, + jobs: [ + { id: "job-fallback-keep", status: "running", updatedAt: "2026-08-19T00:00:00.000Z" }, + { id: "job-fallback-remove", status: "running", updatedAt: "2026-08-19T00:00:00.000Z" } + ] + }); + + process.env.CLAUDE_PLUGIN_DATA = pluginDataDir; + writeStateFileDirectly(resolveStateDir(workspace), { + config: {}, + jobs: [{ id: "job-plugin-data", status: "running", updatedAt: "2026-08-19T00:01:00.000Z" }] + }); + + // Mirrors cleanupSessionJobs(): load the merged view, drop one job that + // originated entirely in the fallback root, save the remainder -- still + // with CLAUDE_PLUGIN_DATA set, the same as a real SessionEnd hook. + const merged = loadState(workspace); + saveState(workspace, { + ...merged, + jobs: merged.jobs.filter((job) => job.id !== "job-fallback-remove") + }); + + const jobIdsAfterRemoval = loadState(workspace) + .jobs.map((job) => job.id) + .sort(); + + assert.deepEqual(jobIdsAfterRemoval, ["job-fallback-keep", "job-plugin-data"]); + } finally { + if (previousPluginDataDir == null) { + delete process.env.CLAUDE_PLUGIN_DATA; + } else { + process.env.CLAUDE_PLUGIN_DATA = previousPluginDataDir; + } + } +}); + test("readStoredJob finds a job's detail file written without CLAUDE_PLUGIN_DATA when the current invocation has it set", () => { const workspace = makeTempDir(); const pluginDataDir = makeTempDir(); From a88f5d8105fb3a7ff0d2b0c7ef36c79bfa15d978 Mon Sep 17 00:00:00 2001 From: Praveen Mittal Date: Tue, 25 Aug 2026 00:03:40 +0200 Subject: [PATCH 05/30] fix: make SessionEnd check every state candidate, reconcile config across roots cleanupSessionJobs() checked only resolveStateFile()'s (the primary candidate's) existence before deciding whether to look for jobs to clean up. loadState() is candidate-aware, but a session whose jobs live only in the fallback root (e.g. started without CLAUDE_PLUGIN_DATA, with SessionEnd later running with it set, flipping which root is primary) was silently skipped: the early check saw no primary file and returned before loadState() was ever called. Fixed by removing the redundant pre-check -- loadState() already returns an empty job list when nothing exists anywhere, and the existing removedJobs.length === 0 check already short-circuits correctly, without the primary-only blind spot. loadState()'s config merge also only ever read the primary candidate's config, unlike jobs (already merged across every root). A boolean flag like stopReviewGate is an opt-in toward stricter/safer behavior, so any candidate setting it true should win over a stale false elsewhere -- e.g. /codex:setup --enable-review-gate running without CLAUDE_PLUGIN_DATA writes it to the fallback root, invisible to a later invocation whose primary is the plugin-data root. Reconciling by "primary wins" could silently downgrade an explicitly-enabled gate. Both found via Codex Review on the PR. --- plugins/codex/scripts/lib/state.mjs | 24 ++++++++-- .../codex/scripts/session-lifecycle-hook.mjs | 11 ++--- tests/runtime.test.mjs | 48 ++++++++++++++++++- tests/state.test.mjs | 38 +++++++++++++++ 4 files changed, 110 insertions(+), 11 deletions(-) diff --git a/plugins/codex/scripts/lib/state.mjs b/plugins/codex/scripts/lib/state.mjs index c14e13e85..932f8c7c4 100644 --- a/plugins/codex/scripts/lib/state.mjs +++ b/plugins/codex/scripts/lib/state.mjs @@ -124,14 +124,30 @@ export function loadState(cwd) { } } + // Like jobs, config can genuinely differ across roots depending on which + // invocation wrote it -- e.g. `/codex:setup --enable-review-gate` running + // without CLAUDE_PLUGIN_DATA writes stopReviewGate to the fallback root, + // which a later invocation with CLAUDE_PLUGIN_DATA set would never see if + // only the primary candidate's config were read. A boolean flag here is + // an opt-in toward stricter/safer behavior, so any candidate setting it + // true wins over a stale false elsewhere -- reconciling by "primary wins" + // could silently downgrade an explicitly-enabled gate. + const mergedConfig = { ...defaultState().config }; + for (const parsed of parsedCandidates) { + for (const [key, value] of Object.entries(parsed.config ?? {})) { + if (typeof value === "boolean") { + mergedConfig[key] = mergedConfig[key] === true || value === true; + } else if (mergedConfig[key] === undefined) { + mergedConfig[key] = value; + } + } + } + const [primary] = parsedCandidates; return { ...defaultState(), ...primary, - config: { - ...defaultState().config, - ...(primary.config ?? {}) - }, + config: mergedConfig, jobs: [...jobsById.values()] }; } diff --git a/plugins/codex/scripts/session-lifecycle-hook.mjs b/plugins/codex/scripts/session-lifecycle-hook.mjs index 778571e6c..e49964559 100644 --- a/plugins/codex/scripts/session-lifecycle-hook.mjs +++ b/plugins/codex/scripts/session-lifecycle-hook.mjs @@ -13,7 +13,7 @@ import { sendBrokerShutdown, teardownBrokerSession } from "./lib/broker-lifecycle.mjs"; -import { loadState, resolveStateFile, saveState } from "./lib/state.mjs"; +import { loadState, saveState } from "./lib/state.mjs"; import { TRANSCRIPT_PATH_ENV } from "./lib/claude-session-transfer.mjs"; import { resolveWorkspaceRoot } from "./lib/workspace.mjs"; @@ -45,11 +45,10 @@ function cleanupSessionJobs(cwd, sessionId) { } const workspaceRoot = resolveWorkspaceRoot(cwd); - const stateFile = resolveStateFile(workspaceRoot); - if (!fs.existsSync(stateFile)) { - return; - } - + // loadState() is candidate-aware and already returns an empty job list + // when nothing exists in any root; a raw existsSync() against just the + // primary candidate would miss a session whose jobs only live in the + // fallback root. const state = loadState(workspaceRoot); const removedJobs = state.jobs.filter((job) => job.sessionId === sessionId); if (removedJobs.length === 0) { diff --git a/tests/runtime.test.mjs b/tests/runtime.test.mjs index 8f276835b..a3c525c49 100644 --- a/tests/runtime.test.mjs +++ b/tests/runtime.test.mjs @@ -8,7 +8,7 @@ import { fileURLToPath } from "node:url"; import { buildEnv, installFakeCodex } from "./fake-codex-fixture.mjs"; import { initGitRepo, makeTempDir, run } from "./helpers.mjs"; import { loadBrokerSession, saveBrokerSession } from "../plugins/codex/scripts/lib/broker-lifecycle.mjs"; -import { resolveStateDir } from "../plugins/codex/scripts/lib/state.mjs"; +import { loadState, resolveStateDir, saveState } from "../plugins/codex/scripts/lib/state.mjs"; const ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), ".."); const PLUGIN_ROOT = path.join(ROOT, "plugins", "codex"); @@ -2257,3 +2257,49 @@ test("setup and status honor --cwd when reading shared session runtime", () => { assert.equal(payload.sessionRuntime.mode, "shared"); assert.equal(payload.sessionRuntime.endpoint, "unix:/tmp/fake-broker.sock"); }); + +// Caught in review: cleanupSessionJobs() checked only resolveStateFile()'s +// (the primary candidate's) existence before deciding whether to look for +// jobs to clean up -- but loadState() is candidate-aware, so a session +// whose jobs live only in the fallback root (e.g. started without +// CLAUDE_PLUGIN_DATA, with SessionEnd later running with it set, flipping +// which root is primary) would be silently skipped: the early check saw no +// primary file and returned before loadState() was ever called. +test("SessionEnd cleans up a session's jobs even when they exist only in the fallback root", () => { + const workspace = makeTempDir(); + const pluginDataDir = makeTempDir(); + const previousPluginDataDir = process.env.CLAUDE_PLUGIN_DATA; + + try { + delete process.env.CLAUDE_PLUGIN_DATA; + saveState(workspace, { + config: {}, + jobs: [{ id: "job-fallback-only", sessionId: "sess-under-test", status: "completed", updatedAt: "2026-08-19T00:00:00.000Z" }] + }); + + const env = { ...process.env, CLAUDE_PLUGIN_DATA: pluginDataDir }; + const cleanup = run("node", [SESSION_HOOK, "SessionEnd"], { + cwd: workspace, + env, + input: JSON.stringify({ + hook_event_name: "SessionEnd", + cwd: workspace, + session_id: "sess-under-test" + }) + }); + assert.equal(cleanup.status, 0, cleanup.stderr); + + process.env.CLAUDE_PLUGIN_DATA = pluginDataDir; + const state = loadState(workspace); + assert.equal( + state.jobs.some((job) => job.id === "job-fallback-only"), + false + ); + } finally { + if (previousPluginDataDir == null) { + delete process.env.CLAUDE_PLUGIN_DATA; + } else { + process.env.CLAUDE_PLUGIN_DATA = previousPluginDataDir; + } + } +}); diff --git a/tests/state.test.mjs b/tests/state.test.mjs index 14f4656f4..0903e8dcf 100644 --- a/tests/state.test.mjs +++ b/tests/state.test.mjs @@ -123,6 +123,44 @@ test("loadState merges jobs from every candidate root instead of only the first } }); +// Caught in review: config (like jobs) can genuinely differ across roots -- +// e.g. `/codex:setup --enable-review-gate` running without CLAUDE_PLUGIN_DATA +// writes stopReviewGate to the fallback root, which a later invocation with +// CLAUDE_PLUGIN_DATA set (a different primary) would never see if only the +// primary candidate's config were read. Unlike the sibling test above (only +// one root has state.json, so "primary" trivially picks the only candidate +// available either way), this exercises the actual bug: *both* roots have +// state, and the non-primary one is the one with the flag enabled. +test("loadState merges config across roots, preferring an enabled boolean over a stale disabled one", () => { + const workspace = makeTempDir(); + const pluginDataDir = makeTempDir(); + const previousPluginDataDir = process.env.CLAUDE_PLUGIN_DATA; + + try { + delete process.env.CLAUDE_PLUGIN_DATA; + writeStateFileDirectly(resolveStateDir(workspace), { + config: { stopReviewGate: true }, + jobs: [] + }); + + process.env.CLAUDE_PLUGIN_DATA = pluginDataDir; + writeStateFileDirectly(resolveStateDir(workspace), { + config: { stopReviewGate: false }, + jobs: [] + }); + + const state = loadState(workspace); + + assert.equal(state.config.stopReviewGate, true); + } finally { + if (previousPluginDataDir == null) { + delete process.env.CLAUDE_PLUGIN_DATA; + } else { + process.env.CLAUDE_PLUGIN_DATA = previousPluginDataDir; + } + } +}); + // Caught in review: merging reads across roots (the previous test) isn't // enough on its own -- saveState() only ever wrote the new job list to the // current primary root, so a job that originated in a *different* root and From 8e624225412d137cfbed2f6c76d4cb26e18f4a17 Mon Sep 17 00:00:00 2001 From: thossullivan Date: Sat, 29 Aug 2026 09:52:58 -0500 Subject: [PATCH 06/30] fix(app-server): unsubscribe task threads after client disconnect --- plugins/codex/scripts/app-server-broker.mjs | 232 ++++++++++- .../scripts/lib/app-server-protocol.d.ts | 3 + tests/broker-subscriptions.test.mjs | 380 ++++++++++++++++++ tests/fake-codex-fixture.mjs | 59 ++- 4 files changed, 672 insertions(+), 2 deletions(-) create mode 100644 tests/broker-subscriptions.test.mjs diff --git a/plugins/codex/scripts/app-server-broker.mjs b/plugins/codex/scripts/app-server-broker.mjs index 1954274fe..0f420e91f 100644 --- a/plugins/codex/scripts/app-server-broker.mjs +++ b/plugins/codex/scripts/app-server-broker.mjs @@ -10,6 +10,48 @@ import { BROKER_BUSY_RPC_CODE, CodexAppServerClient } from "./lib/app-server.mjs import { parseBrokerEndpoint } from "./lib/broker-endpoint.mjs"; const STREAMING_METHODS = new Set(["turn/start", "review/start", "thread/compact/start"]); +const SUBSCRIBING_METHODS = new Set(["thread/start", "thread/resume", "thread/fork"]); + +function buildSubscriptionThreadIds(method, result) { + const threadIds = new Set(); + if (SUBSCRIBING_METHODS.has(method) && result?.thread?.id) { + threadIds.add(result.thread.id); + } + if (method === "review/start" && result?.reviewThreadId) { + threadIds.add(result.reviewThreadId); + } + return threadIds; +} + +function buildProvisionalSubscriptionThreadIds(method, params) { + const threadIds = new Set(); + if (method === "thread/resume" && params?.threadId) { + threadIds.add(params.threadId); + } + if (method === "review/start" && params?.threadId) { + threadIds.add(params.threadId); + } + return threadIds; +} + +function buildNotificationSubscriptionThreadIds(message) { + const threadIds = new Set(); + const params = message?.params; + if (message?.method === "thread/started" && params?.thread?.id) { + threadIds.add(params.thread.id); + } + if (params?.threadId) { + threadIds.add(params.threadId); + } + if (Array.isArray(params?.item?.receiverThreadIds)) { + for (const threadId of params.item.receiverThreadIds) { + if (threadId) { + threadIds.add(threadId); + } + } + } + return threadIds; +} function buildStreamThreadIds(method, params, result) { const threadIds = new Set(); @@ -70,6 +112,175 @@ async function main() { let activeStreamSocket = null; let activeStreamThreadIds = null; const sockets = new Set(); + // App-server subscriptions belong to the broker's single upstream connection. + // Mirror downstream ownership so one client cannot release another client's thread. + const socketThreadIds = new Map(); + const threadSockets = new Map(); + const pendingUnsubscribes = new Map(); + const socketUnsubscribingThreadIds = new Map(); + + function addThreadOwner(socket, threadId) { + let ownedThreadIds = socketThreadIds.get(socket); + if (!ownedThreadIds) { + ownedThreadIds = new Set(); + socketThreadIds.set(socket, ownedThreadIds); + } + if (ownedThreadIds.has(threadId)) { + return false; + } + ownedThreadIds.add(threadId); + + let owners = threadSockets.get(threadId); + if (!owners) { + owners = new Set(); + threadSockets.set(threadId, owners); + } + owners.add(socket); + return true; + } + + function removeThreadOwner(socket, threadId) { + const ownedThreadIds = socketThreadIds.get(socket); + if (!ownedThreadIds?.delete(threadId)) { + return false; + } + if (ownedThreadIds.size === 0) { + socketThreadIds.delete(socket); + } + + const owners = threadSockets.get(threadId); + owners?.delete(socket); + if (owners?.size === 0) { + threadSockets.delete(threadId); + } + return true; + } + + function setSocketThreadUnsubscribing(socket, threadId, isUnsubscribing) { + let threadIds = socketUnsubscribingThreadIds.get(socket); + if (isUnsubscribing) { + if (!threadIds) { + threadIds = new Set(); + socketUnsubscribingThreadIds.set(socket, threadIds); + } + threadIds.add(threadId); + return; + } + threadIds?.delete(threadId); + if (threadIds?.size === 0) { + socketUnsubscribingThreadIds.delete(socket); + } + } + + function requestThreadUnsubscribe(threadId) { + const pending = pendingUnsubscribes.get(threadId); + if (pending) { + return pending; + } + const request = appClient.request("thread/unsubscribe", { threadId }).then( + (result) => ({ result, error: null }), + (error) => { + process.stderr.write( + `Failed to unsubscribe Codex thread ${threadId}: ${error instanceof Error ? error.message : String(error)}\n` + ); + return { result: null, error }; + } + ); + pendingUnsubscribes.set(threadId, request); + void request.finally(() => { + if (pendingUnsubscribes.get(threadId) === request) { + pendingUnsubscribes.delete(threadId); + } + }); + return request; + } + + async function unsubscribeIfUnowned(threadId) { + if (threadSockets.has(threadId) || appClient.closed) { + return null; + } + return requestThreadUnsubscribe(threadId); + } + + async function releaseThreadOwners(socket, threadIds = socketThreadIds.get(socket) ?? new Set()) { + const releasedThreadIds = []; + for (const threadId of [...threadIds]) { + if (removeThreadOwner(socket, threadId) && !threadSockets.has(threadId)) { + releasedThreadIds.push(threadId); + } + } + await Promise.all(releasedThreadIds.map((threadId) => unsubscribeIfUnowned(threadId))); + } + + function trackSubscriptionResults(socket, method, result, provisionalThreadIds) { + for (const threadId of buildSubscriptionThreadIds(method, result)) { + if (provisionalThreadIds.has(threadId)) { + continue; + } + if (socket.destroyed || !sockets.has(socket)) { + void unsubscribeIfUnowned(threadId); + continue; + } + addThreadOwner(socket, threadId); + } + } + + function trackNotificationSubscriptions(socket, message) { + // App-server auto-subscribes its connection to child threads created by subagents. + // Attribute those notification-only subscriptions to the active downstream client. + for (const threadId of buildNotificationSubscriptionThreadIds(message)) { + if (socket && !socket.destroyed && sockets.has(socket)) { + if (!socketUnsubscribingThreadIds.get(socket)?.has(threadId)) { + addThreadOwner(socket, threadId); + } + } else { + void unsubscribeIfUnowned(threadId); + } + } + } + + async function handleThreadUnsubscribe(socket, params) { + const threadId = params?.threadId; + if (typeof threadId !== "string") { + return appClient.request("thread/unsubscribe", params ?? {}); + } + + const ownedThreadIds = socketThreadIds.get(socket); + if (!ownedThreadIds?.has(threadId)) { + if (threadSockets.has(threadId)) { + return { status: "notSubscribed" }; + } + setSocketThreadUnsubscribing(socket, threadId, true); + try { + const outcome = await requestThreadUnsubscribe(threadId); + if (outcome.error) { + throw outcome.error; + } + return outcome.result; + } finally { + setSocketThreadUnsubscribing(socket, threadId, false); + } + } + + removeThreadOwner(socket, threadId); + if (threadSockets.has(threadId)) { + return { status: "unsubscribed" }; + } + + setSocketThreadUnsubscribing(socket, threadId, true); + try { + const outcome = await unsubscribeIfUnowned(threadId); + if (outcome?.error) { + if (!socket.destroyed && sockets.has(socket)) { + addThreadOwner(socket, threadId); + } + throw outcome.error; + } + return outcome?.result ?? { status: "unsubscribed" }; + } finally { + setSocketThreadUnsubscribing(socket, threadId, false); + } + } function clearSocketOwnership(socket) { if (activeRequestSocket === socket) { @@ -83,6 +294,7 @@ async function main() { function routeNotification(message) { const target = activeRequestSocket ?? activeStreamSocket; + trackNotificationSubscriptions(target, message); if (!target) { return; } @@ -195,10 +407,23 @@ async function main() { } const isStreaming = STREAMING_METHODS.has(message.method); + // Claim known thread ids before awaiting app-server. This prevents another + // client's close handler from unsubscribing a concurrently resumed thread. + const provisionalThreadIds = buildProvisionalSubscriptionThreadIds(message.method, message.params ?? {}); + const addedProvisionalThreadIds = new Set(); + for (const threadId of provisionalThreadIds) { + if (addThreadOwner(socket, threadId)) { + addedProvisionalThreadIds.add(threadId); + } + } activeRequestSocket = socket; try { - const result = await appClient.request(message.method, message.params ?? {}); + const result = + message.method === "thread/unsubscribe" + ? await handleThreadUnsubscribe(socket, message.params ?? {}) + : await appClient.request(message.method, message.params ?? {}); + trackSubscriptionResults(socket, message.method, result, provisionalThreadIds); send(socket, { id: message.id, result }); if (isStreaming) { activeStreamSocket = socket; @@ -208,6 +433,7 @@ async function main() { activeRequestSocket = null; } } catch (error) { + await releaseThreadOwners(socket, addedProvisionalThreadIds); send(socket, { id: message.id, error: buildJsonRpcError(error.rpcCode ?? -32000, error.message) @@ -225,11 +451,15 @@ async function main() { socket.on("close", () => { sockets.delete(socket); clearSocketOwnership(socket); + socketUnsubscribingThreadIds.delete(socket); + void releaseThreadOwners(socket); }); socket.on("error", () => { sockets.delete(socket); clearSocketOwnership(socket); + socketUnsubscribingThreadIds.delete(socket); + void releaseThreadOwners(socket); }); }); diff --git a/plugins/codex/scripts/lib/app-server-protocol.d.ts b/plugins/codex/scripts/lib/app-server-protocol.d.ts index f61a4588e..e023b324f 100644 --- a/plugins/codex/scripts/lib/app-server-protocol.d.ts +++ b/plugins/codex/scripts/lib/app-server-protocol.d.ts @@ -21,6 +21,8 @@ import type { ThreadSetNameResponse, ThreadStartParams as RawThreadStartParams, ThreadStartResponse, + ThreadUnsubscribeParams, + ThreadUnsubscribeResponse, Turn, TurnInterruptParams, TurnInterruptResponse, @@ -63,6 +65,7 @@ export interface AppServerMethodMap { "thread/resume": { params: ThreadResumeParams; result: ThreadResumeResponse }; "thread/name/set": { params: ThreadSetNameParams; result: ThreadSetNameResponse }; "thread/list": { params: ThreadListParams; result: ThreadListResponse }; + "thread/unsubscribe": { params: ThreadUnsubscribeParams; result: ThreadUnsubscribeResponse }; "review/start": { params: ReviewStartParams; result: ReviewStartResponse }; "turn/start": { params: TurnStartParams; result: TurnStartResponse }; "turn/interrupt": { params: TurnInterruptParams; result: TurnInterruptResponse }; diff --git a/tests/broker-subscriptions.test.mjs b/tests/broker-subscriptions.test.mjs new file mode 100644 index 000000000..3277b611d --- /dev/null +++ b/tests/broker-subscriptions.test.mjs @@ -0,0 +1,380 @@ +import assert from "node:assert/strict"; +import fs from "node:fs"; +import net from "node:net"; +import path from "node:path"; +import test from "node:test"; +import { spawn } from "node:child_process"; +import { fileURLToPath } from "node:url"; + +import { buildEnv, installFakeCodex } from "./fake-codex-fixture.mjs"; +import { makeTempDir } from "./helpers.mjs"; + +const ROOT = path.resolve(fileURLToPath(new URL("..", import.meta.url))); +const BROKER = path.join(ROOT, "plugins", "codex", "scripts", "app-server-broker.mjs"); + +function delay(ms) { + return new Promise((resolve) => setTimeout(resolve, ms)); +} + +async function waitFor(predicate, { timeoutMs = 10000, intervalMs = 25 } = {}) { + const startedAt = Date.now(); + while (Date.now() - startedAt < timeoutMs) { + if (await predicate()) { + return true; + } + await delay(intervalMs); + } + return false; +} + +function readState(statePath) { + try { + return JSON.parse(fs.readFileSync(statePath, "utf8")); + } catch { + return null; + } +} + +function startBroker(behavior = "review-ok") { + const binDir = makeTempDir("codex-broker-bin-"); + installFakeCodex(binDir, behavior); + const sessionDir = makeTempDir("codex-broker-subscriptions-"); + const cwd = makeTempDir("codex-broker-cwd-"); + const socketPath = path.join(sessionDir, "broker.sock"); + const pidFile = path.join(sessionDir, "broker.pid"); + const statePath = path.join(binDir, "fake-codex-state.json"); + + const child = spawn( + process.execPath, + [BROKER, "serve", "--endpoint", `unix:${socketPath}`, "--cwd", cwd, "--pid-file", pidFile], + { env: buildEnv(binDir), stdio: ["ignore", "pipe", "pipe"] } + ); + let stderr = ""; + child.stderr.on("data", (chunk) => { + stderr += chunk; + }); + const exited = new Promise((resolve) => { + child.on("exit", (code, signal) => resolve({ code, signal })); + }); + + async function stop() { + if (child.exitCode !== null || child.signalCode !== null) { + await exited; + return; + } + child.kill("SIGTERM"); + const result = await Promise.race([exited, delay(5000).then(() => null)]); + if (!result) { + child.kill("SIGKILL"); + await exited; + } + } + + return { + socketPath, + statePath, + stderr: () => stderr, + listening: () => waitFor(() => fs.existsSync(socketPath)), + stop + }; +} + +async function connectClient(socketPath) { + const socket = await new Promise((resolve, reject) => { + const candidate = net.createConnection({ path: socketPath }); + candidate.on("connect", () => resolve(candidate)); + candidate.on("error", reject); + }); + socket.setEncoding("utf8"); + + let nextId = 1; + let buffer = ""; + const pending = new Map(); + const notifications = []; + const notificationWaiters = new Set(); + const closed = new Promise((resolve) => socket.on("close", resolve)); + + function notifyWaiters(message) { + for (const waiter of [...notificationWaiters]) { + if (waiter.predicate(message)) { + notificationWaiters.delete(waiter); + waiter.resolve(message); + } + } + } + + socket.on("data", (chunk) => { + buffer += chunk; + let newlineIndex = buffer.indexOf("\n"); + while (newlineIndex !== -1) { + const line = buffer.slice(0, newlineIndex); + buffer = buffer.slice(newlineIndex + 1); + newlineIndex = buffer.indexOf("\n"); + if (!line.trim()) { + continue; + } + const message = JSON.parse(line); + if (message.id !== undefined) { + const request = pending.get(message.id); + if (request) { + pending.delete(message.id); + if (message.error) { + request.reject(new Error(message.error.message)); + } else { + request.resolve(message.result); + } + } + continue; + } + notifications.push(message); + notifyWaiters(message); + } + }); + + socket.on("error", (error) => { + for (const request of pending.values()) { + request.reject(error); + } + pending.clear(); + }); + + function request(method, params = {}) { + const id = nextId++; + const response = new Promise((resolve, reject) => { + pending.set(id, { resolve, reject }); + }); + socket.write(`${JSON.stringify({ id, method, params })}\n`); + return response; + } + + async function waitForNotification(predicate, timeoutMs = 10000) { + const existing = notifications.find(predicate); + if (existing) { + return existing; + } + const notification = new Promise((resolve) => { + notificationWaiters.add({ predicate, resolve }); + }); + return Promise.race([notification, delay(timeoutMs).then(() => null)]); + } + + await request("initialize", {}); + return { + request, + waitForNotification, + async end() { + socket.end(); + await closed; + }, + destroy() { + socket.destroy(); + } + }; +} + +async function waitForUnsubscribes(statePath, expectedThreadIds) { + const expected = [...expectedThreadIds].sort(); + const found = await waitFor(() => { + const actual = [...(readState(statePath)?.unsubscribeRequests ?? [])].sort(); + return actual.length === expected.length && actual.every((threadId, index) => threadId === expected[index]); + }); + assert.equal(found, true, `expected unsubscribe requests for ${expected.join(", ")}`); +} + +test("broker unsubscribes a completed task thread when its client closes", async (t) => { + const broker = startBroker(); + t.after(() => broker.stop()); + assert.equal(await broker.listening(), true, `broker never listened: ${broker.stderr()}`); + + const client = await connectClient(broker.socketPath); + const started = await client.request("thread/start", { cwd: process.cwd(), ephemeral: true }); + const threadId = started.thread.id; + await client.request("turn/start", { + threadId, + input: [{ type: "text", text: "test normal completion" }] + }); + const completed = await client.waitForNotification( + (message) => message.method === "turn/completed" && message.params?.threadId === threadId + ); + assert.ok(completed, "task never completed"); + + await client.end(); + await waitForUnsubscribes(broker.statePath, [threadId]); + assert.deepEqual(readState(broker.statePath).subscriptions, []); +}); + +test("broker keeps a resumed thread subscribed until its final client closes", async (t) => { + const broker = startBroker(); + t.after(() => broker.stop()); + assert.equal(await broker.listening(), true, `broker never listened: ${broker.stderr()}`); + + const firstClient = await connectClient(broker.socketPath); + const threadId = (await firstClient.request("thread/start", { cwd: process.cwd(), ephemeral: false })).thread.id; + const secondClient = await connectClient(broker.socketPath); + await secondClient.request("thread/resume", { threadId }); + + await firstClient.end(); + await delay(250); + assert.deepEqual(readState(broker.statePath).unsubscribeRequests, []); + + await secondClient.end(); + await waitForUnsubscribes(broker.statePath, [threadId]); +}); + +test("broker unsubscribes source and detached review threads", async (t) => { + const broker = startBroker(); + t.after(() => broker.stop()); + assert.equal(await broker.listening(), true, `broker never listened: ${broker.stderr()}`); + + const client = await connectClient(broker.socketPath); + const sourceThreadId = (await client.request("thread/start", { cwd: process.cwd(), ephemeral: true })).thread.id; + const review = await client.request("review/start", { + threadId: sourceThreadId, + delivery: "detached", + target: { type: "uncommittedChanges" } + }); + assert.notEqual(review.reviewThreadId, sourceThreadId); + const completed = await client.waitForNotification( + (message) => message.method === "turn/completed" && message.params?.threadId === review.reviewThreadId + ); + assert.ok(completed, "review never completed"); + + await client.end(); + await waitForUnsubscribes(broker.statePath, [sourceThreadId, review.reviewThreadId]); +}); + +test("broker unsubscribes a forked thread when its client closes", async (t) => { + const broker = startBroker(); + t.after(() => broker.stop()); + assert.equal(await broker.listening(), true, `broker never listened: ${broker.stderr()}`); + + const client = await connectClient(broker.socketPath); + const sourceThreadId = (await client.request("thread/start", { cwd: process.cwd(), ephemeral: false })).thread.id; + const forkThreadId = (await client.request("thread/fork", { threadId: sourceThreadId, ephemeral: true })).thread.id; + + await client.end(); + await waitForUnsubscribes(broker.statePath, [sourceThreadId, forkThreadId]); +}); + +test("broker unsubscribes when a client disconnects during an active turn", async (t) => { + const broker = startBroker("slow-task"); + t.after(() => broker.stop()); + assert.equal(await broker.listening(), true, `broker never listened: ${broker.stderr()}`); + + const client = await connectClient(broker.socketPath); + const threadId = (await client.request("thread/start", { cwd: process.cwd(), ephemeral: true })).thread.id; + await client.request("turn/start", { + threadId, + input: [{ type: "text", text: "disconnect this client" }] + }); + client.destroy(); + + await waitForUnsubscribes(broker.statePath, [threadId]); +}); + +test("broker unsubscribes auto-subscribed subagent threads", async (t) => { + const broker = startBroker("with-subagent"); + t.after(() => broker.stop()); + assert.equal(await broker.listening(), true, `broker never listened: ${broker.stderr()}`); + + const client = await connectClient(broker.socketPath); + const threadId = (await client.request("thread/start", { cwd: process.cwd(), ephemeral: true })).thread.id; + await client.request("turn/start", { + threadId, + input: [{ type: "text", text: "delegate this task" }] + }); + const completed = await client.waitForNotification( + (message) => message.method === "turn/completed" && message.params?.threadId === threadId + ); + assert.ok(completed, "task never completed"); + + const subagentThread = readState(broker.statePath).threads.find((thread) => thread.name === "design-challenger"); + assert.ok(subagentThread, "subagent thread was not created"); + + await client.end(); + await waitForUnsubscribes(broker.statePath, [threadId, subagentThread.id]); + assert.deepEqual(readState(broker.statePath).subscriptions, []); +}); + +test("broker unsubscribes a child thread created after its client disconnects", async (t) => { + const broker = startBroker("with-delayed-subagent"); + t.after(() => broker.stop()); + assert.equal(await broker.listening(), true, `broker never listened: ${broker.stderr()}`); + + const client = await connectClient(broker.socketPath); + const threadId = (await client.request("thread/start", { cwd: process.cwd(), ephemeral: true })).thread.id; + await client.request("turn/start", { + threadId, + input: [{ type: "text", text: "disconnect before delegation" }] + }); + client.destroy(); + + const childCreated = await waitFor(() => + readState(broker.statePath)?.threads.some((thread) => thread.name === "delayed-design-challenger") + ); + assert.equal(childCreated, true, "delayed child thread was not created"); + const childThread = readState(broker.statePath).threads.find( + (thread) => thread.name === "delayed-design-challenger" + ); + + await waitForUnsubscribes(broker.statePath, [threadId, childThread.id]); + assert.deepEqual(readState(broker.statePath).subscriptions, []); +}); + +test("broker keeps shared upstream subscriptions when one client explicitly unsubscribes", async (t) => { + const broker = startBroker(); + t.after(() => broker.stop()); + assert.equal(await broker.listening(), true, `broker never listened: ${broker.stderr()}`); + + const firstClient = await connectClient(broker.socketPath); + const threadId = (await firstClient.request("thread/start", { cwd: process.cwd(), ephemeral: false })).thread.id; + const secondClient = await connectClient(broker.socketPath); + await secondClient.request("thread/resume", { threadId }); + + assert.deepEqual(await firstClient.request("thread/unsubscribe", { threadId }), { status: "unsubscribed" }); + assert.deepEqual(readState(broker.statePath).unsubscribeRequests, []); + assert.deepEqual(readState(broker.statePath).subscriptions, [threadId]); + + const thirdClient = await connectClient(broker.socketPath); + assert.deepEqual(await thirdClient.request("thread/unsubscribe", { threadId }), { status: "notSubscribed" }); + assert.deepEqual(readState(broker.statePath).unsubscribeRequests, []); + + await firstClient.end(); + await thirdClient.end(); + await secondClient.end(); + await waitForUnsubscribes(broker.statePath, [threadId]); + assert.deepEqual(readState(broker.statePath).subscriptions, []); +}); + +test("broker does not reclaim ownership from notifications during explicit unsubscribe", async (t) => { + const broker = startBroker("unsubscribe-notifies"); + t.after(() => broker.stop()); + assert.equal(await broker.listening(), true, `broker never listened: ${broker.stderr()}`); + + const client = await connectClient(broker.socketPath); + const threadId = (await client.request("thread/start", { cwd: process.cwd(), ephemeral: true })).thread.id; + assert.deepEqual(await client.request("thread/unsubscribe", { threadId }), { status: "unsubscribed" }); + await client.end(); + await delay(250); + + assert.deepEqual(readState(broker.statePath).unsubscribeRequests, [threadId]); + assert.deepEqual(readState(broker.statePath).subscriptions, []); +}); + +test("broker logs upstream unsubscribe failures", async (t) => { + const broker = startBroker("unsubscribe-fails"); + t.after(() => broker.stop()); + assert.equal(await broker.listening(), true, `broker never listened: ${broker.stderr()}`); + + const client = await connectClient(broker.socketPath); + const threadId = (await client.request("thread/start", { cwd: process.cwd(), ephemeral: true })).thread.id; + await client.end(); + + const requestObserved = await waitFor(() => readState(broker.statePath)?.unsubscribeRequests?.includes(threadId)); + assert.equal(requestObserved, true, "unsubscribe request was not observed"); + const warningObserved = await waitFor( + () => broker.stderr().includes(`Failed to unsubscribe Codex thread ${threadId}: thread unsubscribe failed`) + ); + assert.equal(warningObserved, true, "unsubscribe failure was not logged"); + assert.deepEqual(readState(broker.statePath).subscriptions, [threadId]); +}); diff --git a/tests/fake-codex-fixture.mjs b/tests/fake-codex-fixture.mjs index f83c96a0d..db0efc189 100644 --- a/tests/fake-codex-fixture.mjs +++ b/tests/fake-codex-fixture.mjs @@ -19,7 +19,7 @@ const readline = require("node:readline"); function loadState() { if (!fs.existsSync(STATE_PATH)) { - return { nextThreadId: 1, nextTurnId: 1, appServerStarts: 0, threads: [], capabilities: null, lastInterrupt: null }; + return { nextThreadId: 1, nextTurnId: 1, appServerStarts: 0, threads: [], subscriptions: [], unsubscribeRequests: [], capabilities: null, lastInterrupt: null }; } return JSON.parse(fs.readFileSync(STATE_PATH, "utf8")); } @@ -313,6 +313,8 @@ rl.on("line", (line) => { throw new Error("thread/start.persistFullHistory requires experimentalApi capability"); } const thread = nextThread(state, message.params.cwd, message.params.ephemeral); + state.subscriptions = [...new Set([...(state.subscriptions || []), thread.id])]; + saveState(state); send({ id: message.id, result: { thread: buildThread(thread), model: message.params.model || "gpt-5.4", modelProvider: "openai", serviceTier: null, cwd: thread.cwd, approvalPolicy: "never", sandbox: { type: "readOnly", access: { type: "fullAccess" }, networkAccess: false }, reasoningEffort: null } }); send({ method: "thread/started", params: { thread: { id: thread.id } } }); break; @@ -346,11 +348,47 @@ rl.on("line", (line) => { } const thread = ensureThread(state, message.params.threadId); thread.updatedAt = now(); + state.subscriptions = [...new Set([...(state.subscriptions || []), thread.id])]; saveState(state); send({ id: message.id, result: { thread: buildThread(thread), model: message.params.model || "gpt-5.4", modelProvider: "openai", serviceTier: null, cwd: thread.cwd, approvalPolicy: "never", sandbox: { type: "readOnly", access: { type: "fullAccess" }, networkAccess: false }, reasoningEffort: null } }); break; } + case "thread/fork": { + const sourceThread = ensureThread(state, message.params.threadId); + const thread = nextThread(state, sourceThread.cwd, message.params.ephemeral); + state.subscriptions = [...new Set([...(state.subscriptions || []), thread.id])]; + saveState(state); + send({ id: message.id, result: { thread: buildThread(thread) } }); + send({ method: "thread/started", params: { thread: { id: thread.id } } }); + break; + } + + case "thread/unsubscribe": { + const subscriptions = state.subscriptions || []; + const wasSubscribed = subscriptions.includes(message.params.threadId); + const wasLoaded = state.threads.some((thread) => thread.id === message.params.threadId); + state.unsubscribeRequests = [...(state.unsubscribeRequests || []), message.params.threadId]; + if (BEHAVIOR === "unsubscribe-fails") { + saveState(state); + send({ id: message.id, error: { code: -32000, message: "thread unsubscribe failed" } }); + break; + } + if (BEHAVIOR === "unsubscribe-notifies") { + send({ + method: "thread/status/changed", + params: { threadId: message.params.threadId, status: { type: "idle" } } + }); + } + state.subscriptions = subscriptions.filter((threadId) => threadId !== message.params.threadId); + saveState(state); + send({ + id: message.id, + result: { status: wasSubscribed ? "unsubscribed" : wasLoaded ? "notSubscribed" : "notLoaded" } + }); + break; + } + case "externalAgentConfig/import": { if (BEHAVIOR === "external-import-unsupported") { send({ id: message.id, error: { code: -32601, message: "Unsupported method: externalAgentConfig/import" } }); @@ -409,6 +447,8 @@ rl.on("line", (line) => { let reviewThread = thread; if (message.params.delivery === "detached") { reviewThread = nextThread(state, thread.cwd, true); + state.subscriptions = [...new Set([...(state.subscriptions || []), reviewThread.id])]; + saveState(state); send({ method: "thread/started", params: { thread: { id: reviewThread.id } } }); } const turnId = nextTurnId(state); @@ -458,6 +498,22 @@ rl.on("line", (line) => { ? structuredReviewPayload(prompt) : taskPayload(prompt, thread.name && thread.name.startsWith("Codex Companion Task") && prompt.includes("Continue from the current thread state")); + if (BEHAVIOR === "with-delayed-subagent") { + setTimeout(() => { + const delayedState = loadState(); + const subThread = nextThread(delayedState, thread.cwd, true); + const subThreadRecord = ensureThread(delayedState, subThread.id); + subThreadRecord.name = "delayed-design-challenger"; + delayedState.subscriptions = [...new Set([...(delayedState.subscriptions || []), subThread.id])]; + saveState(delayedState); + const subTurnId = nextTurnId(delayedState); + send({ method: "thread/started", params: { thread: { ...buildThread(subThreadRecord), name: subThreadRecord.name, agentNickname: subThreadRecord.name } } }); + send({ method: "turn/started", params: { threadId: subThread.id, turn: buildTurn(subTurnId) } }); + send({ method: "turn/completed", params: { threadId: subThread.id, turn: buildTurn(subTurnId, "completed") } }); + }, 100); + break; + } + if ( BEHAVIOR === "with-subagent" || BEHAVIOR === "with-late-subagent-message" || @@ -466,6 +522,7 @@ rl.on("line", (line) => { const subThread = nextThread(state, thread.cwd, true); const subThreadRecord = ensureThread(state, subThread.id); subThreadRecord.name = "design-challenger"; + state.subscriptions = [...new Set([...(state.subscriptions || []), subThread.id])]; saveState(state); const subTurnId = nextTurnId(state); From 1203f5d5228f5fa274163e54c433ce628a710377 Mon Sep 17 00:00:00 2001 From: thossullivan Date: Tue, 1 Sep 2026 13:00:28 -0500 Subject: [PATCH 07/30] fix(broker): harden thread subscription ownership --- plugins/codex/scripts/app-server-broker.mjs | 355 +++++++++++--------- tests/broker-subscriptions.test.mjs | 173 +++++++++- tests/fake-codex-fixture.mjs | 32 +- 3 files changed, 371 insertions(+), 189 deletions(-) diff --git a/plugins/codex/scripts/app-server-broker.mjs b/plugins/codex/scripts/app-server-broker.mjs index 0f420e91f..dbcb068d4 100644 --- a/plugins/codex/scripts/app-server-broker.mjs +++ b/plugins/codex/scripts/app-server-broker.mjs @@ -11,6 +11,7 @@ import { parseBrokerEndpoint } from "./lib/broker-endpoint.mjs"; const STREAMING_METHODS = new Set(["turn/start", "review/start", "thread/compact/start"]); const SUBSCRIBING_METHODS = new Set(["thread/start", "thread/resume", "thread/fork"]); +const UNSUBSCRIBE_RETRY_DELAYS_MS = [100, 500, 2000]; function buildSubscriptionThreadIds(method, result) { const threadIds = new Set(); @@ -34,23 +35,22 @@ function buildProvisionalSubscriptionThreadIds(method, params) { return threadIds; } -function buildNotificationSubscriptionThreadIds(message) { - const threadIds = new Set(); - const params = message?.params; - if (message?.method === "thread/started" && params?.thread?.id) { - threadIds.add(params.thread.id); - } - if (params?.threadId) { - threadIds.add(params.threadId); +function buildNotificationSubscriptionRelationships(message) { + const relationships = []; + const thread = message?.method === "thread/started" ? message.params?.thread : null; + if (thread?.id && thread?.parentThreadId) { + relationships.push({ sourceThreadId: thread.parentThreadId, subscribedThreadId: thread.id }); } - if (Array.isArray(params?.item?.receiverThreadIds)) { - for (const threadId of params.item.receiverThreadIds) { + + const item = message?.params?.item; + if (item?.type === "collabAgentToolCall" && item?.senderThreadId && Array.isArray(item.receiverThreadIds)) { + for (const threadId of item.receiverThreadIds) { if (threadId) { - threadIds.add(threadId); + relationships.push({ sourceThreadId: item.senderThreadId, subscribedThreadId: threadId }); } } } - return threadIds; + return relationships; } function buildStreamThreadIds(method, params, result) { @@ -117,9 +117,19 @@ async function main() { const socketThreadIds = new Map(); const threadSockets = new Map(); const pendingUnsubscribes = new Map(); - const socketUnsubscribingThreadIds = new Map(); + const unsubscribeRetryTimers = new Map(); + + function cancelUnsubscribeRetry(threadId) { + const retry = unsubscribeRetryTimers.get(threadId); + if (!retry) { + return; + } + clearTimeout(retry.timer); + unsubscribeRetryTimers.delete(threadId); + } function addThreadOwner(socket, threadId) { + cancelUnsubscribeRetry(threadId); let ownedThreadIds = socketThreadIds.get(socket); if (!ownedThreadIds) { ownedThreadIds = new Set(); @@ -156,22 +166,6 @@ async function main() { return true; } - function setSocketThreadUnsubscribing(socket, threadId, isUnsubscribing) { - let threadIds = socketUnsubscribingThreadIds.get(socket); - if (isUnsubscribing) { - if (!threadIds) { - threadIds = new Set(); - socketUnsubscribingThreadIds.set(socket, threadIds); - } - threadIds.add(threadId); - return; - } - threadIds?.delete(threadId); - if (threadIds?.size === 0) { - socketUnsubscribingThreadIds.delete(socket); - } - } - function requestThreadUnsubscribe(threadId) { const pending = pendingUnsubscribes.get(threadId); if (pending) { @@ -195,11 +189,35 @@ async function main() { return request; } - async function unsubscribeIfUnowned(threadId) { + function scheduleUnsubscribeRetry(threadId, retryIndex) { + if ( + retryIndex >= UNSUBSCRIBE_RETRY_DELAYS_MS.length || + unsubscribeRetryTimers.has(threadId) || + threadSockets.has(threadId) || + appClient.closed + ) { + return; + } + const timer = setTimeout(() => { + unsubscribeRetryTimers.delete(threadId); + void unsubscribeIfUnowned(threadId, { retryOnFailure: true, retryIndex: retryIndex + 1 }); + }, UNSUBSCRIBE_RETRY_DELAYS_MS[retryIndex]); + timer.unref?.(); + unsubscribeRetryTimers.set(threadId, { timer, retryIndex }); + } + + async function unsubscribeIfUnowned(threadId, { retryOnFailure = false, retryIndex = 0 } = {}) { if (threadSockets.has(threadId) || appClient.closed) { + cancelUnsubscribeRetry(threadId); return null; } - return requestThreadUnsubscribe(threadId); + const outcome = await requestThreadUnsubscribe(threadId); + if (outcome.error && retryOnFailure) { + scheduleUnsubscribeRetry(threadId, retryIndex); + } else if (!outcome.error) { + cancelUnsubscribeRetry(threadId); + } + return outcome; } async function releaseThreadOwners(socket, threadIds = socketThreadIds.get(socket) ?? new Set()) { @@ -209,32 +227,35 @@ async function main() { releasedThreadIds.push(threadId); } } - await Promise.all(releasedThreadIds.map((threadId) => unsubscribeIfUnowned(threadId))); + await Promise.all( + releasedThreadIds.map((threadId) => unsubscribeIfUnowned(threadId, { retryOnFailure: true })) + ); } - function trackSubscriptionResults(socket, method, result, provisionalThreadIds) { + function trackSubscriptionResults(socket, method, result) { for (const threadId of buildSubscriptionThreadIds(method, result)) { - if (provisionalThreadIds.has(threadId)) { - continue; - } if (socket.destroyed || !sockets.has(socket)) { - void unsubscribeIfUnowned(threadId); + void unsubscribeIfUnowned(threadId, { retryOnFailure: true }); continue; } addThreadOwner(socket, threadId); } } - function trackNotificationSubscriptions(socket, message) { - // App-server auto-subscribes its connection to child threads created by subagents. - // Attribute those notification-only subscriptions to the active downstream client. - for (const threadId of buildNotificationSubscriptionThreadIds(message)) { - if (socket && !socket.destroyed && sockets.has(socket)) { - if (!socketUnsubscribingThreadIds.get(socket)?.has(threadId)) { - addThreadOwner(socket, threadId); - } - } else { - void unsubscribeIfUnowned(threadId); + function trackNotificationSubscriptions(message) { + // App-server can auto-subscribe its connection to subagent threads. Attribute + // each child to the downstream owners of its causal parent, not the client + // that happens to be active when a delayed notification arrives. + for (const { sourceThreadId, subscribedThreadId } of buildNotificationSubscriptionRelationships(message)) { + const sourceOwners = [...(threadSockets.get(sourceThreadId) ?? [])].filter( + (socket) => !socket.destroyed && sockets.has(socket) + ); + if (sourceOwners.length === 0) { + void unsubscribeIfUnowned(subscribedThreadId, { retryOnFailure: true }); + continue; + } + for (const socket of sourceOwners) { + addThreadOwner(socket, subscribedThreadId); } } } @@ -250,16 +271,11 @@ async function main() { if (threadSockets.has(threadId)) { return { status: "notSubscribed" }; } - setSocketThreadUnsubscribing(socket, threadId, true); - try { - const outcome = await requestThreadUnsubscribe(threadId); - if (outcome.error) { - throw outcome.error; - } - return outcome.result; - } finally { - setSocketThreadUnsubscribing(socket, threadId, false); + const outcome = await unsubscribeIfUnowned(threadId); + if (outcome?.error) { + throw outcome.error; } + return outcome?.result ?? { status: "notSubscribed" }; } removeThreadOwner(socket, threadId); @@ -267,19 +283,14 @@ async function main() { return { status: "unsubscribed" }; } - setSocketThreadUnsubscribing(socket, threadId, true); - try { - const outcome = await unsubscribeIfUnowned(threadId); - if (outcome?.error) { - if (!socket.destroyed && sockets.has(socket)) { - addThreadOwner(socket, threadId); - } - throw outcome.error; + const outcome = await unsubscribeIfUnowned(threadId); + if (outcome?.error) { + if (!socket.destroyed && sockets.has(socket)) { + addThreadOwner(socket, threadId); } - return outcome?.result ?? { status: "unsubscribed" }; - } finally { - setSocketThreadUnsubscribing(socket, threadId, false); + throw outcome.error; } + return outcome?.result ?? { status: "unsubscribed" }; } function clearSocketOwnership(socket) { @@ -294,7 +305,7 @@ async function main() { function routeNotification(message) { const target = activeRequestSocket ?? activeStreamSocket; - trackNotificationSubscriptions(target, message); + trackNotificationSubscriptions(message); if (!target) { return; } @@ -312,6 +323,10 @@ async function main() { } async function shutdown(server) { + for (const { timer } of unsubscribeRetryTimers.values()) { + clearTimeout(timer); + } + unsubscribeRetryTimers.clear(); for (const socket of sockets) { socket.end(); } @@ -331,134 +346,140 @@ async function main() { sockets.add(socket); socket.setEncoding("utf8"); let buffer = ""; + let processing = Promise.resolve(); - socket.on("data", async (chunk) => { - buffer += chunk; - let newlineIndex = buffer.indexOf("\n"); - while (newlineIndex !== -1) { - const line = buffer.slice(0, newlineIndex); - buffer = buffer.slice(newlineIndex + 1); - newlineIndex = buffer.indexOf("\n"); - - if (!line.trim()) { - continue; - } + async function handleLine(line) { + if (!line.trim() || socket.destroyed || !sockets.has(socket)) { + return; + } - let message; - try { - message = JSON.parse(line); - } catch (error) { - send(socket, { - id: null, - error: buildJsonRpcError(-32700, `Invalid JSON: ${error.message}`) - }); - continue; - } + let message; + try { + message = JSON.parse(line); + } catch (error) { + send(socket, { + id: null, + error: buildJsonRpcError(-32700, `Invalid JSON: ${error.message}`) + }); + return; + } - if (message.id !== undefined && message.method === "initialize") { - send(socket, { - id: message.id, - result: { - userAgent: "codex-companion-broker" - } - }); - continue; - } + if (message.id !== undefined && message.method === "initialize") { + send(socket, { + id: message.id, + result: { + userAgent: "codex-companion-broker" + } + }); + return; + } - if (message.method === "initialized" && message.id === undefined) { - continue; - } + if (message.method === "initialized" && message.id === undefined) { + return; + } - if (message.id !== undefined && message.method === "broker/shutdown") { - send(socket, { id: message.id, result: {} }); - await shutdown(server); - process.exit(0); - } + if (message.id !== undefined && message.method === "broker/shutdown") { + send(socket, { id: message.id, result: {} }); + await shutdown(server); + process.exit(0); + } - if (message.id === undefined) { - continue; - } + if (message.id === undefined) { + return; + } - const allowInterruptDuringActiveStream = - isInterruptRequest(message) && activeStreamSocket && activeStreamSocket !== socket && !activeRequestSocket; + const allowInterruptDuringActiveStream = + isInterruptRequest(message) && activeStreamSocket && activeStreamSocket !== socket && !activeRequestSocket; + + if ( + ((activeRequestSocket && activeRequestSocket !== socket) || (activeStreamSocket && activeStreamSocket !== socket)) && + !allowInterruptDuringActiveStream + ) { + send(socket, { + id: message.id, + error: buildJsonRpcError(BROKER_BUSY_RPC_CODE, "Shared Codex broker is busy.") + }); + return; + } - if ( - ((activeRequestSocket && activeRequestSocket !== socket) || (activeStreamSocket && activeStreamSocket !== socket)) && - !allowInterruptDuringActiveStream - ) { + if (allowInterruptDuringActiveStream) { + try { + const result = await appClient.request(message.method, message.params ?? {}); + send(socket, { id: message.id, result }); + } catch (error) { send(socket, { id: message.id, - error: buildJsonRpcError(BROKER_BUSY_RPC_CODE, "Shared Codex broker is busy.") + error: buildJsonRpcError(error.rpcCode ?? -32000, error.message) }); - continue; } + return; + } - if (allowInterruptDuringActiveStream) { - try { - const result = await appClient.request(message.method, message.params ?? {}); - send(socket, { id: message.id, result }); - } catch (error) { - send(socket, { - id: message.id, - error: buildJsonRpcError(error.rpcCode ?? -32000, error.message) - }); - } - continue; + const isStreaming = STREAMING_METHODS.has(message.method); + // Claim known thread ids before awaiting app-server. This prevents another + // client's close handler from unsubscribing a concurrently resumed thread. + const provisionalThreadIds = buildProvisionalSubscriptionThreadIds(message.method, message.params ?? {}); + const addedProvisionalThreadIds = new Set(); + for (const threadId of provisionalThreadIds) { + if (addThreadOwner(socket, threadId)) { + addedProvisionalThreadIds.add(threadId); } + } + activeRequestSocket = socket; - const isStreaming = STREAMING_METHODS.has(message.method); - // Claim known thread ids before awaiting app-server. This prevents another - // client's close handler from unsubscribing a concurrently resumed thread. - const provisionalThreadIds = buildProvisionalSubscriptionThreadIds(message.method, message.params ?? {}); - const addedProvisionalThreadIds = new Set(); - for (const threadId of provisionalThreadIds) { - if (addThreadOwner(socket, threadId)) { - addedProvisionalThreadIds.add(threadId); - } + try { + const result = + message.method === "thread/unsubscribe" + ? await handleThreadUnsubscribe(socket, message.params ?? {}) + : await appClient.request(message.method, message.params ?? {}); + trackSubscriptionResults(socket, message.method, result); + send(socket, { id: message.id, result }); + if (isStreaming && !socket.destroyed && sockets.has(socket)) { + activeStreamSocket = socket; + activeStreamThreadIds = buildStreamThreadIds(message.method, message.params ?? {}, result); } - activeRequestSocket = socket; - - try { - const result = - message.method === "thread/unsubscribe" - ? await handleThreadUnsubscribe(socket, message.params ?? {}) - : await appClient.request(message.method, message.params ?? {}); - trackSubscriptionResults(socket, message.method, result, provisionalThreadIds); - send(socket, { id: message.id, result }); - if (isStreaming) { - activeStreamSocket = socket; - activeStreamThreadIds = buildStreamThreadIds(message.method, message.params ?? {}, result); - } - if (activeRequestSocket === socket) { - activeRequestSocket = null; - } - } catch (error) { - await releaseThreadOwners(socket, addedProvisionalThreadIds); - send(socket, { - id: message.id, - error: buildJsonRpcError(error.rpcCode ?? -32000, error.message) - }); - if (activeRequestSocket === socket) { - activeRequestSocket = null; - } - if (activeStreamSocket === socket && !isStreaming) { - activeStreamSocket = null; - } + if (activeRequestSocket === socket) { + activeRequestSocket = null; } + } catch (error) { + await releaseThreadOwners(socket, addedProvisionalThreadIds); + send(socket, { + id: message.id, + error: buildJsonRpcError(error.rpcCode ?? -32000, error.message) + }); + if (activeRequestSocket === socket) { + activeRequestSocket = null; + } + if (activeStreamSocket === socket && !isStreaming) { + activeStreamSocket = null; + } + } + } + + socket.on("data", (chunk) => { + buffer += chunk; + let newlineIndex = buffer.indexOf("\n"); + while (newlineIndex !== -1) { + const line = buffer.slice(0, newlineIndex); + buffer = buffer.slice(newlineIndex + 1); + newlineIndex = buffer.indexOf("\n"); + processing = processing.then(() => handleLine(line)).catch((error) => { + process.stderr.write( + `Failed to process broker request: ${error instanceof Error ? error.message : String(error)}\n` + ); + }); } }); socket.on("close", () => { sockets.delete(socket); clearSocketOwnership(socket); - socketUnsubscribingThreadIds.delete(socket); void releaseThreadOwners(socket); }); socket.on("error", () => { sockets.delete(socket); clearSocketOwnership(socket); - socketUnsubscribingThreadIds.delete(socket); void releaseThreadOwners(socket); }); }); diff --git a/tests/broker-subscriptions.test.mjs b/tests/broker-subscriptions.test.mjs index 3277b611d..88fa48a9e 100644 --- a/tests/broker-subscriptions.test.mjs +++ b/tests/broker-subscriptions.test.mjs @@ -16,6 +16,20 @@ function delay(ms) { return new Promise((resolve) => setTimeout(resolve, ms)); } +async function waitWithTimeout(promise, timeoutMs) { + let timer; + try { + return await Promise.race([ + promise, + new Promise((resolve) => { + timer = setTimeout(() => resolve(null), timeoutMs); + }) + ]); + } finally { + clearTimeout(timer); + } +} + async function waitFor(predicate, { timeoutMs = 10000, intervalMs = 25 } = {}) { const startedAt = Date.now(); while (Date.now() - startedAt < timeoutMs) { @@ -43,6 +57,7 @@ function startBroker(behavior = "review-ok") { const socketPath = path.join(sessionDir, "broker.sock"); const pidFile = path.join(sessionDir, "broker.pid"); const statePath = path.join(binDir, "fake-codex-state.json"); + const tempDirs = [binDir, sessionDir, cwd]; const child = spawn( process.execPath, @@ -58,15 +73,21 @@ function startBroker(behavior = "review-ok") { }); async function stop() { - if (child.exitCode !== null || child.signalCode !== null) { - await exited; - return; - } - child.kill("SIGTERM"); - const result = await Promise.race([exited, delay(5000).then(() => null)]); - if (!result) { - child.kill("SIGKILL"); - await exited; + try { + if (child.exitCode !== null || child.signalCode !== null) { + await exited; + return; + } + child.kill("SIGTERM"); + const result = await waitWithTimeout(exited, 5000); + if (!result) { + child.kill("SIGKILL"); + await exited; + } + } finally { + for (const tempDir of tempDirs) { + fs.rmSync(tempDir, { recursive: true, force: true }); + } } } @@ -97,8 +118,7 @@ async function connectClient(socketPath) { function notifyWaiters(message) { for (const waiter of [...notificationWaiters]) { if (waiter.predicate(message)) { - notificationWaiters.delete(waiter); - waiter.resolve(message); + waiter.settle(message); } } } @@ -152,10 +172,19 @@ async function connectClient(socketPath) { if (existing) { return existing; } - const notification = new Promise((resolve) => { - notificationWaiters.add({ predicate, resolve }); + return new Promise((resolve) => { + let timer; + const waiter = { + predicate, + settle(message) { + clearTimeout(timer); + notificationWaiters.delete(waiter); + resolve(message); + } + }; + timer = setTimeout(() => waiter.settle(null), timeoutMs); + notificationWaiters.add(waiter); }); - return Promise.race([notification, delay(timeoutMs).then(() => null)]); } await request("initialize", {}); @@ -296,6 +325,30 @@ test("broker unsubscribes auto-subscribed subagent threads", async (t) => { assert.deepEqual(readState(broker.statePath).subscriptions, []); }); +test("broker tracks subagent subscriptions from collaboration items", async (t) => { + const broker = startBroker("with-receiver-only-subagent"); + t.after(() => broker.stop()); + assert.equal(await broker.listening(), true, `broker never listened: ${broker.stderr()}`); + + const client = await connectClient(broker.socketPath); + const threadId = (await client.request("thread/start", { cwd: process.cwd(), ephemeral: true })).thread.id; + await client.request("turn/start", { + threadId, + input: [{ type: "text", text: "delegate without a thread-started notification" }] + }); + const completed = await client.waitForNotification( + (message) => message.method === "turn/completed" && message.params?.threadId === threadId + ); + assert.ok(completed, "task never completed"); + + const subagentThread = readState(broker.statePath).threads.find((thread) => thread.name === "design-challenger"); + assert.ok(subagentThread, "subagent thread was not created"); + + await client.end(); + await waitForUnsubscribes(broker.statePath, [threadId, subagentThread.id]); + assert.deepEqual(readState(broker.statePath).subscriptions, []); +}); + test("broker unsubscribes a child thread created after its client disconnects", async (t) => { const broker = startBroker("with-delayed-subagent"); t.after(() => broker.stop()); @@ -321,6 +374,80 @@ test("broker unsubscribes a child thread created after its client disconnects", assert.deepEqual(readState(broker.statePath).subscriptions, []); }); +test("broker does not assign a delayed child thread to an unrelated active client", async (t) => { + const broker = startBroker("with-delayed-subagent"); + t.after(() => broker.stop()); + assert.equal(await broker.listening(), true, `broker never listened: ${broker.stderr()}`); + + const firstClient = await connectClient(broker.socketPath); + const firstThreadId = ( + await firstClient.request("thread/start", { cwd: process.cwd(), ephemeral: true }) + ).thread.id; + await firstClient.request("turn/start", { + threadId: firstThreadId, + input: [{ type: "text", text: "create a delayed child" }] + }); + firstClient.destroy(); + + const secondClient = await connectClient(broker.socketPath); + const secondThreadId = ( + await secondClient.request("thread/start", { cwd: process.cwd(), ephemeral: true }) + ).thread.id; + await secondClient.request("turn/start", { + threadId: secondThreadId, + input: [{ type: "text", text: "remain active while the first child arrives" }] + }); + + const childrenCreated = await waitFor( + () => readState(broker.statePath)?.threads.filter((thread) => thread.parentThreadId).length === 2 + ); + assert.equal(childrenCreated, true, "delayed child threads were not created"); + + const state = readState(broker.statePath); + const firstChild = state.threads.find((thread) => thread.parentThreadId === firstThreadId); + const secondChild = state.threads.find((thread) => thread.parentThreadId === secondThreadId); + assert.ok(firstChild, "the first client's child thread was not recorded"); + assert.ok(secondChild, "the second client's child thread was not recorded"); + + const firstReleased = await waitFor(() => { + const requests = readState(broker.statePath)?.unsubscribeRequests ?? []; + return requests.includes(firstThreadId) && requests.includes(firstChild.id); + }); + assert.equal(firstReleased, true, "the disconnected client's subscriptions were not released"); + const requestsBeforeSecondClientCloses = readState(broker.statePath).unsubscribeRequests; + assert.equal(requestsBeforeSecondClientCloses.includes(secondThreadId), false); + assert.equal(requestsBeforeSecondClientCloses.includes(secondChild.id), false); + + await secondClient.end(); + await waitForUnsubscribes(broker.statePath, [firstThreadId, firstChild.id, secondThreadId, secondChild.id]); + assert.deepEqual(readState(broker.statePath).subscriptions, []); +}); + +test("broker serializes subscription requests from one downstream client", async (t) => { + const broker = startBroker("overlapping-resume"); + t.after(() => broker.stop()); + assert.equal(await broker.listening(), true, `broker never listened: ${broker.stderr()}`); + + const firstClient = await connectClient(broker.socketPath); + const threadId = (await firstClient.request("thread/start", { cwd: process.cwd(), ephemeral: false })).thread.id; + const secondClient = await connectClient(broker.socketPath); + const results = await Promise.allSettled([ + secondClient.request("thread/resume", { threadId, persistFullHistory: true }), + secondClient.request("thread/resume", { threadId }) + ]); + assert.equal(results[0].status, "rejected"); + assert.match(results[0].reason.message, /forced delayed resume failure/); + assert.equal(results[1].status, "fulfilled"); + + await firstClient.end(); + await delay(250); + assert.deepEqual(readState(broker.statePath).unsubscribeRequests, []); + assert.deepEqual(readState(broker.statePath).subscriptions, [threadId]); + + await secondClient.end(); + await waitForUnsubscribes(broker.statePath, [threadId]); +}); + test("broker keeps shared upstream subscriptions when one client explicitly unsubscribes", async (t) => { const broker = startBroker(); t.after(() => broker.stop()); @@ -378,3 +505,21 @@ test("broker logs upstream unsubscribe failures", async (t) => { assert.equal(warningObserved, true, "unsubscribe failure was not logged"); assert.deepEqual(readState(broker.statePath).subscriptions, [threadId]); }); + +test("broker retries a transient upstream unsubscribe failure", async (t) => { + const broker = startBroker("unsubscribe-fails-once"); + t.after(() => broker.stop()); + assert.equal(await broker.listening(), true, `broker never listened: ${broker.stderr()}`); + + const client = await connectClient(broker.socketPath); + const threadId = (await client.request("thread/start", { cwd: process.cwd(), ephemeral: true })).thread.id; + await client.end(); + + const retried = await waitFor(() => { + const state = readState(broker.statePath); + return state?.unsubscribeRequests?.length === 2 && state.subscriptions.length === 0; + }); + assert.equal(retried, true, "the failed unsubscribe was not retried"); + assert.deepEqual(readState(broker.statePath).unsubscribeRequests, [threadId, threadId]); + assert.match(broker.stderr(), new RegExp(`Failed to unsubscribe Codex thread ${threadId}`)); +}); diff --git a/tests/fake-codex-fixture.mjs b/tests/fake-codex-fixture.mjs index db0efc189..125f1b897 100644 --- a/tests/fake-codex-fixture.mjs +++ b/tests/fake-codex-fixture.mjs @@ -42,6 +42,8 @@ function now() { function buildThread(thread) { return { id: thread.id, + forkedFromId: thread.forkedFromId || null, + parentThreadId: thread.parentThreadId || null, preview: thread.preview || "", ephemeral: Boolean(thread.ephemeral), modelProvider: "openai", @@ -116,9 +118,11 @@ function send(message) { process.stdout.write(JSON.stringify(message) + "\\n"); } -function nextThread(state, cwd, ephemeral) { +function nextThread(state, cwd, ephemeral, { forkedFromId = null, parentThreadId = null } = {}) { const thread = { id: "thr_" + state.nextThreadId++, + forkedFromId, + parentThreadId, cwd: cwd || process.cwd(), name: null, preview: "", @@ -316,7 +320,7 @@ rl.on("line", (line) => { state.subscriptions = [...new Set([...(state.subscriptions || []), thread.id])]; saveState(state); send({ id: message.id, result: { thread: buildThread(thread), model: message.params.model || "gpt-5.4", modelProvider: "openai", serviceTier: null, cwd: thread.cwd, approvalPolicy: "never", sandbox: { type: "readOnly", access: { type: "fullAccess" }, networkAccess: false }, reasoningEffort: null } }); - send({ method: "thread/started", params: { thread: { id: thread.id } } }); + send({ method: "thread/started", params: { thread: buildThread(thread) } }); break; } @@ -343,6 +347,12 @@ rl.on("line", (line) => { } case "thread/resume": { + if (BEHAVIOR === "overlapping-resume" && message.params.persistFullHistory === true) { + setTimeout(() => { + send({ id: message.id, error: { code: -32000, message: "forced delayed resume failure" } }); + }, 100); + break; + } if (requiresExperimental("persistExtendedHistory", message, state) || requiresExperimental("persistFullHistory", message, state)) { throw new Error("thread/resume.persistFullHistory requires experimentalApi capability"); } @@ -356,11 +366,11 @@ rl.on("line", (line) => { case "thread/fork": { const sourceThread = ensureThread(state, message.params.threadId); - const thread = nextThread(state, sourceThread.cwd, message.params.ephemeral); + const thread = nextThread(state, sourceThread.cwd, message.params.ephemeral, { forkedFromId: sourceThread.id }); state.subscriptions = [...new Set([...(state.subscriptions || []), thread.id])]; saveState(state); send({ id: message.id, result: { thread: buildThread(thread) } }); - send({ method: "thread/started", params: { thread: { id: thread.id } } }); + send({ method: "thread/started", params: { thread: buildThread(thread) } }); break; } @@ -369,7 +379,10 @@ rl.on("line", (line) => { const wasSubscribed = subscriptions.includes(message.params.threadId); const wasLoaded = state.threads.some((thread) => thread.id === message.params.threadId); state.unsubscribeRequests = [...(state.unsubscribeRequests || []), message.params.threadId]; - if (BEHAVIOR === "unsubscribe-fails") { + if ( + BEHAVIOR === "unsubscribe-fails" || + (BEHAVIOR === "unsubscribe-fails-once" && state.unsubscribeRequests.length === 1) + ) { saveState(state); send({ id: message.id, error: { code: -32000, message: "thread unsubscribe failed" } }); break; @@ -501,7 +514,7 @@ rl.on("line", (line) => { if (BEHAVIOR === "with-delayed-subagent") { setTimeout(() => { const delayedState = loadState(); - const subThread = nextThread(delayedState, thread.cwd, true); + const subThread = nextThread(delayedState, thread.cwd, true, { parentThreadId: thread.id }); const subThreadRecord = ensureThread(delayedState, subThread.id); subThreadRecord.name = "delayed-design-challenger"; delayedState.subscriptions = [...new Set([...(delayedState.subscriptions || []), subThread.id])]; @@ -516,17 +529,20 @@ rl.on("line", (line) => { if ( BEHAVIOR === "with-subagent" || + BEHAVIOR === "with-receiver-only-subagent" || BEHAVIOR === "with-late-subagent-message" || BEHAVIOR === "with-subagent-no-main-turn-completed" ) { - const subThread = nextThread(state, thread.cwd, true); + const subThread = nextThread(state, thread.cwd, true, { parentThreadId: thread.id }); const subThreadRecord = ensureThread(state, subThread.id); subThreadRecord.name = "design-challenger"; state.subscriptions = [...new Set([...(state.subscriptions || []), subThread.id])]; saveState(state); const subTurnId = nextTurnId(state); - send({ method: "thread/started", params: { thread: { ...buildThread(subThreadRecord), name: "design-challenger", agentNickname: "design-challenger" } } }); + if (BEHAVIOR !== "with-receiver-only-subagent") { + send({ method: "thread/started", params: { thread: { ...buildThread(subThreadRecord), name: "design-challenger", agentNickname: "design-challenger" } } }); + } send({ method: "turn/started", params: { threadId: thread.id, turn: buildTurn(turnId) } }); send({ method: "item/started", From fc227197c7a8de63c1a0cbc8185e0286499876a9 Mon Sep 17 00:00:00 2001 From: thossullivan Date: Wed, 2 Sep 2026 17:11:45 -0500 Subject: [PATCH 08/30] fix(broker): serialize thread unsubscribe against reacquisition A thread resumed while its automatic thread/unsubscribe was still in flight was never released again: the later release reused the stale pending request, so the shared app-server connection stayed subscribed after every client had closed. If the app-server had processed the unsubscribe after the resume, the new owner would also have lost its subscription. Never reuse a pending unsubscribe. A release chains a fresh request behind any in-flight one and re-checks ownership before sending. A resume or review that provisionally claims a thread waits, bounded to five seconds, for an in-flight unsubscribe of that thread before it is dispatched, so a late unsubscribe cannot overtake the new subscription and a hung cleanup cannot wedge the broker. Reply to a failed request and clear the busy state before releasing provisional owners for the same reason. Add the unsubscribe-delayed and resume-fails-unsubscribe-hangs fixture behaviors, four regression tests, and assert the retry bound in the existing failure test. --- plugins/codex/scripts/app-server-broker.mjs | 66 +++++++++-- tests/broker-subscriptions.test.mjs | 118 ++++++++++++++++++++ tests/fake-codex-fixture.mjs | 29 ++++- 3 files changed, 200 insertions(+), 13 deletions(-) diff --git a/plugins/codex/scripts/app-server-broker.mjs b/plugins/codex/scripts/app-server-broker.mjs index dbcb068d4..714454f39 100644 --- a/plugins/codex/scripts/app-server-broker.mjs +++ b/plugins/codex/scripts/app-server-broker.mjs @@ -12,6 +12,23 @@ import { parseBrokerEndpoint } from "./lib/broker-endpoint.mjs"; const STREAMING_METHODS = new Set(["turn/start", "review/start", "thread/compact/start"]); const SUBSCRIBING_METHODS = new Set(["thread/start", "thread/resume", "thread/fork"]); const UNSUBSCRIBE_RETRY_DELAYS_MS = [100, 500, 2000]; +// Upper bound on how long a request waits for an in-flight thread/unsubscribe of +// the same thread. A hung cleanup request must not wedge the shared broker. +const UNSUBSCRIBE_WAIT_TIMEOUT_MS = 5000; + +function settleWithin(promise, timeoutMs) { + let timer; + return Promise.race([ + promise.then( + () => {}, + () => {} + ), + new Promise((resolve) => { + timer = setTimeout(resolve, timeoutMs); + timer.unref?.(); + }) + ]).finally(() => clearTimeout(timer)); +} function buildSubscriptionThreadIds(method, result) { const threadIds = new Set(); @@ -167,13 +184,32 @@ async function main() { } function requestThreadUnsubscribe(threadId) { - const pending = pendingUnsubscribes.get(threadId); - if (pending) { - return pending; - } - const request = appClient.request("thread/unsubscribe", { threadId }).then( - (result) => ({ result, error: null }), + // Never reuse an earlier request: the thread may have been reacquired and + // released again while that request was in flight. Chain behind it so the + // app-server sees one unsubscribe at a time, then re-check ownership. + const previous = pendingUnsubscribes.get(threadId); + const execute = async () => { + if (previous) { + await settleWithin(previous, UNSUBSCRIBE_WAIT_TIMEOUT_MS); + } + if (threadSockets.has(threadId)) { + return { result: null, error: null, skipped: true }; + } + const result = await appClient.request("thread/unsubscribe", { threadId }); + return { result, error: null }; + }; + let request; + request = execute().then( + (outcome) => { + if (pendingUnsubscribes.get(threadId) === request) { + pendingUnsubscribes.delete(threadId); + } + return outcome; + }, (error) => { + if (pendingUnsubscribes.get(threadId) === request) { + pendingUnsubscribes.delete(threadId); + } process.stderr.write( `Failed to unsubscribe Codex thread ${threadId}: ${error instanceof Error ? error.message : String(error)}\n` ); @@ -181,11 +217,6 @@ async function main() { } ); pendingUnsubscribes.set(threadId, request); - void request.finally(() => { - if (pendingUnsubscribes.get(threadId) === request) { - pendingUnsubscribes.delete(threadId); - } - }); return request; } @@ -426,6 +457,15 @@ async function main() { } } activeRequestSocket = socket; + // Let an in-flight unsubscribe for the same thread settle first so it cannot + // overtake the new subscription. The wait is bounded: a hung cleanup request + // must not block this client or keep the broker busy for everyone else. + await Promise.all( + [...provisionalThreadIds].map((threadId) => { + const pending = pendingUnsubscribes.get(threadId); + return pending ? settleWithin(pending, UNSUBSCRIBE_WAIT_TIMEOUT_MS) : null; + }) + ); try { const result = @@ -442,7 +482,6 @@ async function main() { activeRequestSocket = null; } } catch (error) { - await releaseThreadOwners(socket, addedProvisionalThreadIds); send(socket, { id: message.id, error: buildJsonRpcError(error.rpcCode ?? -32000, error.message) @@ -453,6 +492,9 @@ async function main() { if (activeStreamSocket === socket && !isStreaming) { activeStreamSocket = null; } + // Release after replying: a hung upstream unsubscribe must not withhold + // the error or leave the broker busy for other clients. + void releaseThreadOwners(socket, addedProvisionalThreadIds); } } diff --git a/tests/broker-subscriptions.test.mjs b/tests/broker-subscriptions.test.mjs index 88fa48a9e..46750d121 100644 --- a/tests/broker-subscriptions.test.mjs +++ b/tests/broker-subscriptions.test.mjs @@ -250,6 +250,53 @@ test("broker keeps a resumed thread subscribed until its final client closes", a await waitForUnsubscribes(broker.statePath, [threadId]); }); +test("broker serializes a resume behind an in-flight unsubscribe", async (t) => { + const broker = startBroker("unsubscribe-delayed"); + t.after(() => broker.stop()); + assert.equal(await broker.listening(), true, `broker never listened: ${broker.stderr()}`); + + const firstClient = await connectClient(broker.socketPath); + const threadId = (await firstClient.request("thread/start", { cwd: process.cwd(), ephemeral: false })).thread.id; + await firstClient.end(); + const unsubscribeStarted = await waitFor(() => + readState(broker.statePath)?.unsubscribeRequests?.includes(threadId) + ); + assert.equal(unsubscribeStarted, true, "unsubscribe request was not observed"); + + const secondClient = await connectClient(broker.socketPath); + await secondClient.request("thread/resume", { threadId }); + assert.deepEqual(readState(broker.statePath).requestOrder, ["unsubscribe:response", "thread/resume"]); + assert.deepEqual(readState(broker.statePath).subscriptions, [threadId]); + + await secondClient.end(); + await waitForUnsubscribes(broker.statePath, [threadId, threadId]); + assert.deepEqual(readState(broker.statePath).subscriptions, []); +}); + +test("broker cancels a retry when a client reacquires the thread", async (t) => { + const broker = startBroker("unsubscribe-fails-once"); + t.after(() => broker.stop()); + assert.equal(await broker.listening(), true, `broker never listened: ${broker.stderr()}`); + + const firstClient = await connectClient(broker.socketPath); + const threadId = (await firstClient.request("thread/start", { cwd: process.cwd(), ephemeral: false })).thread.id; + await firstClient.end(); + const firstAttemptObserved = await waitFor( + () => readState(broker.statePath)?.unsubscribeRequests?.length === 1 + ); + assert.equal(firstAttemptObserved, true, "initial unsubscribe request was not observed"); + + const secondClient = await connectClient(broker.socketPath); + await secondClient.request("thread/resume", { threadId }); + await delay(3000); + assert.deepEqual(readState(broker.statePath).subscriptions, [threadId]); + + await secondClient.end(); + const unsubscribed = await waitFor(() => readState(broker.statePath)?.subscriptions?.length === 0); + assert.equal(unsubscribed, true, "reacquired thread was not unsubscribed after its final owner closed"); + assert.equal(readState(broker.statePath).unsubscribeRequests.at(-1), threadId); +}); + test("broker unsubscribes source and detached review threads", async (t) => { const broker = startBroker(); t.after(() => broker.stop()); @@ -448,6 +495,40 @@ test("broker serializes subscription requests from one downstream client", async await waitForUnsubscribes(broker.statePath, [threadId]); }); +test("broker replies to a failed resume even when the upstream unsubscribe hangs", async (t) => { + const broker = startBroker("resume-fails-unsubscribe-hangs"); + t.after(() => broker.stop()); + assert.equal(await broker.listening(), true, `broker never listened: ${broker.stderr()}`); + + // Nobody owns this thread, so the failed provisional claim releases it upstream. + const threadId = "thr_unowned"; + const firstClient = await connectClient(broker.socketPath); + const failedResume = await waitWithTimeout( + firstClient.request("thread/resume", { threadId, persistFullHistory: true }).then( + () => ({ status: "fulfilled" }), + (error) => ({ status: "rejected", error }) + ), + 2000 + ); + assert.notEqual(failedResume, null, "failed resume did not receive a response"); + assert.equal(failedResume.status, "rejected"); + assert.match(failedResume.error.message, /forced resume failure/); + + const secondClient = await connectClient(broker.socketPath); + const secondStarted = await waitWithTimeout( + secondClient.request("thread/start", { cwd: process.cwd(), ephemeral: false }), + 2000 + ); + assert.notEqual(secondStarted, null, "broker remained busy after the failed resume"); + + const unsubscribeStarted = await waitFor(() => + readState(broker.statePath)?.unsubscribeRequests?.includes(threadId) + ); + assert.equal(unsubscribeStarted, true, "the released thread was not unsubscribed upstream"); + await secondClient.end(); + await firstClient.end(); +}); + test("broker keeps shared upstream subscriptions when one client explicitly unsubscribes", async (t) => { const broker = startBroker(); t.after(() => broker.stop()); @@ -503,6 +584,13 @@ test("broker logs upstream unsubscribe failures", async (t) => { () => broker.stderr().includes(`Failed to unsubscribe Codex thread ${threadId}: thread unsubscribe failed`) ); assert.equal(warningObserved, true, "unsubscribe failure was not logged"); + const retryBoundReached = await waitFor( + () => readState(broker.statePath)?.unsubscribeRequests?.length === 4, + { timeoutMs: 6000 } + ); + assert.equal(retryBoundReached, true, "unsubscribe retries did not reach the expected bound"); + await delay(500); + assert.equal(readState(broker.statePath).unsubscribeRequests.length, 4); assert.deepEqual(readState(broker.statePath).subscriptions, [threadId]); }); @@ -523,3 +611,33 @@ test("broker retries a transient upstream unsubscribe failure", async (t) => { assert.deepEqual(readState(broker.statePath).unsubscribeRequests, [threadId, threadId]); assert.match(broker.stderr(), new RegExp(`Failed to unsubscribe Codex thread ${threadId}`)); }); + +test("broker bounds the wait for a hung unsubscribe before a resume proceeds", async (t) => { + const broker = startBroker("resume-fails-unsubscribe-hangs"); + t.after(() => broker.stop()); + assert.equal(await broker.listening(), true, `broker never listened: ${broker.stderr()}`); + + const firstClient = await connectClient(broker.socketPath); + const threadId = (await firstClient.request("thread/start", { cwd: process.cwd(), ephemeral: false })).thread.id; + await firstClient.end(); + const unsubscribeStarted = await waitFor(() => + readState(broker.statePath)?.unsubscribeRequests?.includes(threadId) + ); + assert.equal(unsubscribeStarted, true, "unsubscribe request was not observed"); + + const secondClient = await connectClient(broker.socketPath); + const startedAt = Date.now(); + const resumed = await waitWithTimeout(secondClient.request("thread/resume", { threadId }), 8000); + assert.notEqual(resumed, null, "resume never completed while the upstream unsubscribe hung"); + assert.ok(Date.now() - startedAt >= 4000, "resume did not wait for the in-flight unsubscribe"); + assert.deepEqual(readState(broker.statePath).subscriptions, [threadId]); + + const thirdClient = await connectClient(broker.socketPath); + const thirdStarted = await waitWithTimeout( + thirdClient.request("thread/start", { cwd: process.cwd(), ephemeral: false }), + 2000 + ); + assert.notEqual(thirdStarted, null, "broker remained busy after the bounded wait"); + await thirdClient.end(); + await secondClient.end(); +}); diff --git a/tests/fake-codex-fixture.mjs b/tests/fake-codex-fixture.mjs index 125f1b897..3dd3ff7ec 100644 --- a/tests/fake-codex-fixture.mjs +++ b/tests/fake-codex-fixture.mjs @@ -19,7 +19,7 @@ const readline = require("node:readline"); function loadState() { if (!fs.existsSync(STATE_PATH)) { - return { nextThreadId: 1, nextTurnId: 1, appServerStarts: 0, threads: [], subscriptions: [], unsubscribeRequests: [], capabilities: null, lastInterrupt: null }; + return { nextThreadId: 1, nextTurnId: 1, appServerStarts: 0, threads: [], subscriptions: [], unsubscribeRequests: [], requestOrder: [], capabilities: null, lastInterrupt: null }; } return JSON.parse(fs.readFileSync(STATE_PATH, "utf8")); } @@ -347,6 +347,12 @@ rl.on("line", (line) => { } case "thread/resume": { + if (BEHAVIOR === "resume-fails-unsubscribe-hangs" && message.params.persistFullHistory === true) { + setTimeout(() => { + send({ id: message.id, error: { code: -32000, message: "forced resume failure" } }); + }, 50); + break; + } if (BEHAVIOR === "overlapping-resume" && message.params.persistFullHistory === true) { setTimeout(() => { send({ id: message.id, error: { code: -32000, message: "forced delayed resume failure" } }); @@ -358,6 +364,9 @@ rl.on("line", (line) => { } const thread = ensureThread(state, message.params.threadId); thread.updatedAt = now(); + if (BEHAVIOR === "unsubscribe-delayed") { + state.requestOrder = [...(state.requestOrder || []), "thread/resume"]; + } state.subscriptions = [...new Set([...(state.subscriptions || []), thread.id])]; saveState(state); send({ id: message.id, result: { thread: buildThread(thread), model: message.params.model || "gpt-5.4", modelProvider: "openai", serviceTier: null, cwd: thread.cwd, approvalPolicy: "never", sandbox: { type: "readOnly", access: { type: "fullAccess" }, networkAccess: false }, reasoningEffort: null } }); @@ -379,6 +388,10 @@ rl.on("line", (line) => { const wasSubscribed = subscriptions.includes(message.params.threadId); const wasLoaded = state.threads.some((thread) => thread.id === message.params.threadId); state.unsubscribeRequests = [...(state.unsubscribeRequests || []), message.params.threadId]; + if (BEHAVIOR === "resume-fails-unsubscribe-hangs") { + saveState(state); + break; + } if ( BEHAVIOR === "unsubscribe-fails" || (BEHAVIOR === "unsubscribe-fails-once" && state.unsubscribeRequests.length === 1) @@ -387,6 +400,20 @@ rl.on("line", (line) => { send({ id: message.id, error: { code: -32000, message: "thread unsubscribe failed" } }); break; } + if (BEHAVIOR === "unsubscribe-delayed") { + state.subscriptions = subscriptions.filter((threadId) => threadId !== message.params.threadId); + saveState(state); + setTimeout(() => { + const delayedState = loadState(); + delayedState.requestOrder = [...(delayedState.requestOrder || []), "unsubscribe:response"]; + saveState(delayedState); + send({ + id: message.id, + result: { status: wasSubscribed ? "unsubscribed" : wasLoaded ? "notSubscribed" : "notLoaded" } + }); + }, 300); + break; + } if (BEHAVIOR === "unsubscribe-notifies") { send({ method: "thread/status/changed", From f0ddd6b95be216042a23517dc530a9073a641a77 Mon Sep 17 00:00:00 2001 From: thossullivan Date: Wed, 2 Sep 2026 17:26:54 -0500 Subject: [PATCH 09/30] fix(broker): fail a claim whose in-flight unsubscribe outlives the wait When the bounded wait for an in-flight thread/unsubscribe expires, do not send the resume anyway: that would race the outstanding unsubscribe and could leave a tracked owner without an upstream subscription. Reject the request with a retryable error, release the provisional claim, and clear the busy state so other clients continue. --- plugins/codex/scripts/app-server-broker.mjs | 28 ++++++++++++++++----- tests/broker-subscriptions.test.mjs | 15 ++++++++--- 2 files changed, 34 insertions(+), 9 deletions(-) diff --git a/plugins/codex/scripts/app-server-broker.mjs b/plugins/codex/scripts/app-server-broker.mjs index 714454f39..55ccde248 100644 --- a/plugins/codex/scripts/app-server-broker.mjs +++ b/plugins/codex/scripts/app-server-broker.mjs @@ -16,15 +16,16 @@ const UNSUBSCRIBE_RETRY_DELAYS_MS = [100, 500, 2000]; // the same thread. A hung cleanup request must not wedge the shared broker. const UNSUBSCRIBE_WAIT_TIMEOUT_MS = 5000; +// Resolves true once the promise settles, or false if the timeout expires first. function settleWithin(promise, timeoutMs) { let timer; return Promise.race([ promise.then( - () => {}, - () => {} + () => true, + () => true ), new Promise((resolve) => { - timer = setTimeout(resolve, timeoutMs); + timer = setTimeout(() => resolve(false), timeoutMs); timer.unref?.(); }) ]).finally(() => clearTimeout(timer)); @@ -459,13 +460,28 @@ async function main() { activeRequestSocket = socket; // Let an in-flight unsubscribe for the same thread settle first so it cannot // overtake the new subscription. The wait is bounded: a hung cleanup request - // must not block this client or keep the broker busy for everyone else. - await Promise.all( + // must not block this client or keep the broker busy for everyone else. If + // it expires, fail the request instead of racing the outstanding unsubscribe. + const settled = await Promise.all( [...provisionalThreadIds].map((threadId) => { const pending = pendingUnsubscribes.get(threadId); - return pending ? settleWithin(pending, UNSUBSCRIBE_WAIT_TIMEOUT_MS) : null; + return pending ? settleWithin(pending, UNSUBSCRIBE_WAIT_TIMEOUT_MS) : true; }) ); + if (settled.includes(false)) { + send(socket, { + id: message.id, + error: buildJsonRpcError( + -32000, + "Codex thread is still being released upstream; retry the request shortly." + ) + }); + if (activeRequestSocket === socket) { + activeRequestSocket = null; + } + void releaseThreadOwners(socket, addedProvisionalThreadIds); + return; + } try { const result = diff --git a/tests/broker-subscriptions.test.mjs b/tests/broker-subscriptions.test.mjs index 46750d121..04b2a69f8 100644 --- a/tests/broker-subscriptions.test.mjs +++ b/tests/broker-subscriptions.test.mjs @@ -612,7 +612,7 @@ test("broker retries a transient upstream unsubscribe failure", async (t) => { assert.match(broker.stderr(), new RegExp(`Failed to unsubscribe Codex thread ${threadId}`)); }); -test("broker bounds the wait for a hung unsubscribe before a resume proceeds", async (t) => { +test("broker fails a resume when an in-flight unsubscribe outlives the bounded wait", async (t) => { const broker = startBroker("resume-fails-unsubscribe-hangs"); t.after(() => broker.stop()); assert.equal(await broker.listening(), true, `broker never listened: ${broker.stderr()}`); @@ -627,9 +627,18 @@ test("broker bounds the wait for a hung unsubscribe before a resume proceeds", a const secondClient = await connectClient(broker.socketPath); const startedAt = Date.now(); - const resumed = await waitWithTimeout(secondClient.request("thread/resume", { threadId }), 8000); - assert.notEqual(resumed, null, "resume never completed while the upstream unsubscribe hung"); + const resumed = await waitWithTimeout( + secondClient.request("thread/resume", { threadId }).then( + () => ({ status: "fulfilled" }), + (error) => ({ status: "rejected", error }) + ), + 8000 + ); + assert.notEqual(resumed, null, "resume never settled while the upstream unsubscribe hung"); + assert.equal(resumed.status, "rejected"); + assert.match(resumed.error.message, /still being released upstream/); assert.ok(Date.now() - startedAt >= 4000, "resume did not wait for the in-flight unsubscribe"); + // The resume was never sent upstream, so the fake still lists only the original subscription. assert.deepEqual(readState(broker.statePath).subscriptions, [threadId]); const thirdClient = await connectClient(broker.socketPath); From 93b5660d21dff1384c46c214dba25fc042fed25d Mon Sep 17 00:00:00 2001 From: thossullivan Date: Wed, 2 Sep 2026 17:40:55 -0500 Subject: [PATCH 10/30] fix(broker): keep a pending unsubscribe outstanding until it settles A release that followed a timed-out claim installed a fresh cleanup wrapper that waited for the original unsubscribe for only another five seconds, then settled as skipped once a retried claim owned the thread. The retry could then resume upstream while the original unsubscribe was still in flight. Make a wrapper wait for its predecessor without a bound so the pending entry never settles before every underlying request has, and reject retried claims until then. --- plugins/codex/scripts/app-server-broker.mjs | 8 ++++- tests/broker-subscriptions.test.mjs | 38 +++++++++++++++++++++ tests/fake-codex-fixture.mjs | 2 +- 3 files changed, 46 insertions(+), 2 deletions(-) diff --git a/plugins/codex/scripts/app-server-broker.mjs b/plugins/codex/scripts/app-server-broker.mjs index 55ccde248..5c36d0632 100644 --- a/plugins/codex/scripts/app-server-broker.mjs +++ b/plugins/codex/scripts/app-server-broker.mjs @@ -191,7 +191,13 @@ async function main() { const previous = pendingUnsubscribes.get(threadId); const execute = async () => { if (previous) { - await settleWithin(previous, UNSUBSCRIBE_WAIT_TIMEOUT_MS); + // Wait without a bound. The pending entry must not settle while any + // earlier upstream unsubscribe for this thread is still outstanding, + // otherwise a retried claim could slip past a hung cleanup request. + await previous.then( + () => {}, + () => {} + ); } if (threadSockets.has(threadId)) { return { result: null, error: null, skipped: true }; diff --git a/tests/broker-subscriptions.test.mjs b/tests/broker-subscriptions.test.mjs index 04b2a69f8..f5bd9c665 100644 --- a/tests/broker-subscriptions.test.mjs +++ b/tests/broker-subscriptions.test.mjs @@ -650,3 +650,41 @@ test("broker fails a resume when an in-flight unsubscribe outlives the bounded w await thirdClient.end(); await secondClient.end(); }); + +test("broker keeps rejecting claims until a hung unsubscribe settles", async (t) => { + const broker = startBroker("resume-fails-unsubscribe-hangs"); + t.after(() => broker.stop()); + assert.equal(await broker.listening(), true, `broker never listened: ${broker.stderr()}`); + + const firstClient = await connectClient(broker.socketPath); + const threadId = (await firstClient.request("thread/start", { cwd: process.cwd(), ephemeral: false })).thread.id; + await firstClient.end(); + const unsubscribeStarted = await waitFor(() => + readState(broker.statePath)?.unsubscribeRequests?.includes(threadId) + ); + assert.equal(unsubscribeStarted, true, "unsubscribe request was not observed"); + + const attempt = async () => { + const client = await connectClient(broker.socketPath); + const outcome = await waitWithTimeout( + client.request("thread/resume", { threadId }).then( + () => ({ status: "fulfilled" }), + (error) => ({ status: "rejected", error }) + ), + 8000 + ); + await client.end(); + return outcome; + }; + + // The first claim times out on the hung unsubscribe. Its release installs a + // follow-up cleanup that must stay pending; a retry must still be rejected. + const first = await attempt(); + assert.equal(first?.status, "rejected"); + assert.match(first.error.message, /still being released upstream/); + const retry = await attempt(); + assert.equal(retry?.status, "rejected", "a retry slipped past the still-outstanding unsubscribe"); + assert.match(retry.error.message, /still being released upstream/); + assert.deepEqual(readState(broker.statePath).requestOrder, []); + assert.deepEqual(readState(broker.statePath).subscriptions, [threadId]); +}); diff --git a/tests/fake-codex-fixture.mjs b/tests/fake-codex-fixture.mjs index 3dd3ff7ec..89b74def5 100644 --- a/tests/fake-codex-fixture.mjs +++ b/tests/fake-codex-fixture.mjs @@ -364,7 +364,7 @@ rl.on("line", (line) => { } const thread = ensureThread(state, message.params.threadId); thread.updatedAt = now(); - if (BEHAVIOR === "unsubscribe-delayed") { + if (BEHAVIOR === "unsubscribe-delayed" || BEHAVIOR === "resume-fails-unsubscribe-hangs") { state.requestOrder = [...(state.requestOrder || []), "thread/resume"]; } state.subscriptions = [...new Set([...(state.subscriptions || []), thread.id])]; From 68b402952445f577747795cf309a9b2e2d30b7f9 Mon Sep 17 00:00:00 2001 From: thossullivan Date: Wed, 2 Sep 2026 18:25:56 -0500 Subject: [PATCH 11/30] fix(broker): gate every path that waits on pending thread cleanup An explicit thread/unsubscribe for an unowned thread queued behind a hung automatic unsubscribe without a bound while its socket held the busy slot, so every other client stayed busy. Apply the same bounded wait and retryable rejection to explicit unsubscribes that the provisional claims already use. Two related gaps closed in the same sweep: - A release that arrives while an earlier cleanup is still queued now shares that queued request instead of adding another link to the chain. A queued request re-checks ownership when it sends, so this is safe, and repeated retries against a hung upstream no longer grow an unbounded chain of duplicate unsubscribes. - A child thread whose cleanup is still outstanding is no longer handed to new owners of its parent by a later notification. --- plugins/codex/scripts/app-server-broker.mjs | 46 ++++++++++++++------- tests/broker-subscriptions.test.mjs | 37 +++++++++++++++++ 2 files changed, 68 insertions(+), 15 deletions(-) diff --git a/plugins/codex/scripts/app-server-broker.mjs b/plugins/codex/scripts/app-server-broker.mjs index 5c36d0632..e241d77ce 100644 --- a/plugins/codex/scripts/app-server-broker.mjs +++ b/plugins/codex/scripts/app-server-broker.mjs @@ -185,16 +185,21 @@ async function main() { } function requestThreadUnsubscribe(threadId) { - // Never reuse an earlier request: the thread may have been reacquired and - // released again while that request was in flight. Chain behind it so the - // app-server sees one unsubscribe at a time, then re-check ownership. + // A request that has already been sent is never reused: the thread may have + // been reacquired and released again while it was in flight. A request that + // is still queued behind an earlier one re-checks ownership when it sends, so + // every release until then can share it instead of growing the chain. const previous = pendingUnsubscribes.get(threadId); + if (previous && !previous.sent) { + return previous.request; + } + const entry = { request: null, sent: false }; const execute = async () => { if (previous) { // Wait without a bound. The pending entry must not settle while any // earlier upstream unsubscribe for this thread is still outstanding, // otherwise a retried claim could slip past a hung cleanup request. - await previous.then( + await previous.request.then( () => {}, () => {} ); @@ -202,19 +207,19 @@ async function main() { if (threadSockets.has(threadId)) { return { result: null, error: null, skipped: true }; } + entry.sent = true; const result = await appClient.request("thread/unsubscribe", { threadId }); return { result, error: null }; }; - let request; - request = execute().then( + entry.request = execute().then( (outcome) => { - if (pendingUnsubscribes.get(threadId) === request) { + if (pendingUnsubscribes.get(threadId) === entry) { pendingUnsubscribes.delete(threadId); } return outcome; }, (error) => { - if (pendingUnsubscribes.get(threadId) === request) { + if (pendingUnsubscribes.get(threadId) === entry) { pendingUnsubscribes.delete(threadId); } process.stderr.write( @@ -223,8 +228,8 @@ async function main() { return { result: null, error }; } ); - pendingUnsubscribes.set(threadId, request); - return request; + pendingUnsubscribes.set(threadId, entry); + return entry.request; } function scheduleUnsubscribeRetry(threadId, retryIndex) { @@ -292,6 +297,11 @@ async function main() { void unsubscribeIfUnowned(subscribedThreadId, { retryOnFailure: true }); continue; } + if (pendingUnsubscribes.has(subscribedThreadId)) { + // A cleanup request for this child is still outstanding, so its upstream + // subscription is going away. Do not hand it to new owners. + continue; + } for (const socket of sourceOwners) { addThreadOwner(socket, subscribedThreadId); } @@ -465,13 +475,19 @@ async function main() { } activeRequestSocket = socket; // Let an in-flight unsubscribe for the same thread settle first so it cannot - // overtake the new subscription. The wait is bounded: a hung cleanup request - // must not block this client or keep the broker busy for everyone else. If - // it expires, fail the request instead of racing the outstanding unsubscribe. + // overtake the new subscription, and so an explicit unsubscribe does not + // queue behind it while this socket holds the busy slot. The wait is + // bounded: a hung cleanup request must not block this client or keep the + // broker busy for everyone else. If it expires, fail the request instead of + // racing or waiting on the outstanding unsubscribe. + const gatedThreadIds = new Set(provisionalThreadIds); + if (message.method === "thread/unsubscribe" && typeof message.params?.threadId === "string") { + gatedThreadIds.add(message.params.threadId); + } const settled = await Promise.all( - [...provisionalThreadIds].map((threadId) => { + [...gatedThreadIds].map((threadId) => { const pending = pendingUnsubscribes.get(threadId); - return pending ? settleWithin(pending, UNSUBSCRIBE_WAIT_TIMEOUT_MS) : true; + return pending ? settleWithin(pending.request, UNSUBSCRIBE_WAIT_TIMEOUT_MS) : true; }) ); if (settled.includes(false)) { diff --git a/tests/broker-subscriptions.test.mjs b/tests/broker-subscriptions.test.mjs index f5bd9c665..5f98f4a41 100644 --- a/tests/broker-subscriptions.test.mjs +++ b/tests/broker-subscriptions.test.mjs @@ -688,3 +688,40 @@ test("broker keeps rejecting claims until a hung unsubscribe settles", async (t) assert.deepEqual(readState(broker.statePath).requestOrder, []); assert.deepEqual(readState(broker.statePath).subscriptions, [threadId]); }); + +test("broker rejects an explicit unsubscribe that would queue behind a hung cleanup", async (t) => { + const broker = startBroker("resume-fails-unsubscribe-hangs"); + t.after(() => broker.stop()); + assert.equal(await broker.listening(), true, `broker never listened: ${broker.stderr()}`); + + const firstClient = await connectClient(broker.socketPath); + const threadId = (await firstClient.request("thread/start", { cwd: process.cwd(), ephemeral: false })).thread.id; + await firstClient.end(); + const unsubscribeStarted = await waitFor(() => + readState(broker.statePath)?.unsubscribeRequests?.includes(threadId) + ); + assert.equal(unsubscribeStarted, true, "unsubscribe request was not observed"); + + const secondClient = await connectClient(broker.socketPath); + const explicit = await waitWithTimeout( + secondClient.request("thread/unsubscribe", { threadId }).then( + () => ({ status: "fulfilled" }), + (error) => ({ status: "rejected", error }) + ), + 8000 + ); + assert.notEqual(explicit, null, "explicit unsubscribe never settled behind the hung cleanup"); + assert.equal(explicit.status, "rejected"); + assert.match(explicit.error.message, /still being released upstream/); + + const thirdClient = await connectClient(broker.socketPath); + const thirdStarted = await waitWithTimeout( + thirdClient.request("thread/start", { cwd: process.cwd(), ephemeral: false }), + 2000 + ); + assert.notEqual(thirdStarted, null, "broker remained busy after the rejected explicit unsubscribe"); + // Only the original hung request reached upstream; nothing else was queued. + assert.deepEqual(readState(broker.statePath).unsubscribeRequests, [threadId]); + await thirdClient.end(); + await secondClient.end(); +}); From fd45841d782c1e86965f8cea375b3c869254d9ec Mon Sep 17 00:00:00 2001 From: thossullivan Date: Wed, 2 Sep 2026 19:40:01 -0500 Subject: [PATCH 12/30] fix(broker): roll back children inherited through a failed claim A child notification that arrived while a provisional resume or review was waiting or in flight attributed the child to the claiming socket, and a rejected or failed claim released only the parent. The child then stayed owned until the socket closed. Record children inherited through an open claim and release them together with the claim when it is rejected or fails. --- plugins/codex/scripts/app-server-broker.mjs | 29 +++++++++++++++++-- tests/broker-subscriptions.test.mjs | 32 +++++++++++++++++++++ tests/fake-codex-fixture.mjs | 6 ++++ 3 files changed, 64 insertions(+), 3 deletions(-) diff --git a/plugins/codex/scripts/app-server-broker.mjs b/plugins/codex/scripts/app-server-broker.mjs index e241d77ce..27ec62317 100644 --- a/plugins/codex/scripts/app-server-broker.mjs +++ b/plugins/codex/scripts/app-server-broker.mjs @@ -136,6 +136,9 @@ async function main() { const threadSockets = new Map(); const pendingUnsubscribes = new Map(); const unsubscribeRetryTimers = new Map(); + // Threads a socket claimed for a request that has not succeeded yet, plus any + // child threads it inherited through those claims while the request was open. + const provisionalClaims = new Map(); function cancelUnsubscribeRetry(threadId) { const retry = unsubscribeRetryTimers.get(threadId); @@ -303,7 +306,14 @@ async function main() { continue; } for (const socket of sourceOwners) { - addThreadOwner(socket, subscribedThreadId); + if (addThreadOwner(socket, subscribedThreadId)) { + // Ownership inherited through a claim that has not succeeded yet is + // rolled back with that claim. + const claim = provisionalClaims.get(socket); + if (claim?.threadIds.has(sourceThreadId)) { + claim.inheritedThreadIds.add(subscribedThreadId); + } + } } } } @@ -473,6 +483,16 @@ async function main() { addedProvisionalThreadIds.add(threadId); } } + const claim = { threadIds: addedProvisionalThreadIds, inheritedThreadIds: new Set() }; + if (addedProvisionalThreadIds.size > 0) { + provisionalClaims.set(socket, claim); + } + const rollBackClaim = () => { + if (provisionalClaims.get(socket) === claim) { + provisionalClaims.delete(socket); + } + void releaseThreadOwners(socket, new Set([...claim.threadIds, ...claim.inheritedThreadIds])); + }; activeRequestSocket = socket; // Let an in-flight unsubscribe for the same thread settle first so it cannot // overtake the new subscription, and so an explicit unsubscribe does not @@ -501,7 +521,7 @@ async function main() { if (activeRequestSocket === socket) { activeRequestSocket = null; } - void releaseThreadOwners(socket, addedProvisionalThreadIds); + rollBackClaim(); return; } @@ -511,6 +531,9 @@ async function main() { ? await handleThreadUnsubscribe(socket, message.params ?? {}) : await appClient.request(message.method, message.params ?? {}); trackSubscriptionResults(socket, message.method, result); + if (provisionalClaims.get(socket) === claim) { + provisionalClaims.delete(socket); + } send(socket, { id: message.id, result }); if (isStreaming && !socket.destroyed && sockets.has(socket)) { activeStreamSocket = socket; @@ -532,7 +555,7 @@ async function main() { } // Release after replying: a hung upstream unsubscribe must not withhold // the error or leave the broker busy for other clients. - void releaseThreadOwners(socket, addedProvisionalThreadIds); + rollBackClaim(); } } diff --git a/tests/broker-subscriptions.test.mjs b/tests/broker-subscriptions.test.mjs index 5f98f4a41..6dc28e749 100644 --- a/tests/broker-subscriptions.test.mjs +++ b/tests/broker-subscriptions.test.mjs @@ -725,3 +725,35 @@ test("broker rejects an explicit unsubscribe that would queue behind a hung clea await thirdClient.end(); await secondClient.end(); }); + +test("broker rolls back child threads inherited through a failed provisional claim", async (t) => { + const broker = startBroker("with-delayed-subagent"); + t.after(() => broker.stop()); + assert.equal(await broker.listening(), true, `broker never listened: ${broker.stderr()}`); + + const firstClient = await connectClient(broker.socketPath); + const threadId = (await firstClient.request("thread/start", { cwd: process.cwd(), ephemeral: false })).thread.id; + await firstClient.request("turn/start", { + threadId, + input: [{ type: "text", text: "spawn a child while another client is resuming" }] + }); + firstClient.destroy(); + await waitForUnsubscribes(broker.statePath, [threadId]); + + // The resume fails after 250 ms; the delayed child arrives at 100 ms while the + // claim is still open, so the claiming socket inherits it. + const secondClient = await connectClient(broker.socketPath); + await assert.rejects( + secondClient.request("thread/resume", { threadId, persistFullHistory: true }), + /forced resume failure after child arrival/ + ); + const childThread = readState(broker.statePath).threads.find( + (thread) => thread.name === "delayed-design-challenger" + ); + assert.ok(childThread, "delayed child thread was not created"); + + // Both the parent and the inherited child are released while the client stays connected. + await waitForUnsubscribes(broker.statePath, [threadId, threadId, childThread.id]); + assert.deepEqual(readState(broker.statePath).subscriptions, []); + await secondClient.end(); +}); diff --git a/tests/fake-codex-fixture.mjs b/tests/fake-codex-fixture.mjs index 89b74def5..5172b8352 100644 --- a/tests/fake-codex-fixture.mjs +++ b/tests/fake-codex-fixture.mjs @@ -347,6 +347,12 @@ rl.on("line", (line) => { } case "thread/resume": { + if (BEHAVIOR === "with-delayed-subagent" && message.params.persistFullHistory === true) { + setTimeout(() => { + send({ id: message.id, error: { code: -32000, message: "forced resume failure after child arrival" } }); + }, 250); + break; + } if (BEHAVIOR === "resume-fails-unsubscribe-hangs" && message.params.persistFullHistory === true) { setTimeout(() => { send({ id: message.id, error: { code: -32000, message: "forced resume failure" } }); From e7fba1e2d0e7992d824dab984803195ac543aa6c Mon Sep 17 00:00:00 2001 From: thossullivan Date: Wed, 2 Sep 2026 21:47:32 -0500 Subject: [PATCH 13/30] fix(broker): roll back nested descendants inherited through a failed claim A grandchild spawned while a provisional claim was open has an inherited child as its source, not the claimed root, so it was owned but omitted from the rollback. Record descendants whose source is either a claimed root or an already inherited thread. --- plugins/codex/scripts/app-server-broker.mjs | 5 +++-- tests/broker-subscriptions.test.mjs | 12 ++++++++++-- tests/fake-codex-fixture.mjs | 13 +++++++++++++ 3 files changed, 26 insertions(+), 4 deletions(-) diff --git a/plugins/codex/scripts/app-server-broker.mjs b/plugins/codex/scripts/app-server-broker.mjs index 27ec62317..cd4682b57 100644 --- a/plugins/codex/scripts/app-server-broker.mjs +++ b/plugins/codex/scripts/app-server-broker.mjs @@ -308,9 +308,10 @@ async function main() { for (const socket of sourceOwners) { if (addThreadOwner(socket, subscribedThreadId)) { // Ownership inherited through a claim that has not succeeded yet is - // rolled back with that claim. + // rolled back with that claim. The source may itself be an inherited + // child, so nested descendants are recorded too. const claim = provisionalClaims.get(socket); - if (claim?.threadIds.has(sourceThreadId)) { + if (claim && (claim.threadIds.has(sourceThreadId) || claim.inheritedThreadIds.has(sourceThreadId))) { claim.inheritedThreadIds.add(subscribedThreadId); } } diff --git a/tests/broker-subscriptions.test.mjs b/tests/broker-subscriptions.test.mjs index 6dc28e749..3fc171683 100644 --- a/tests/broker-subscriptions.test.mjs +++ b/tests/broker-subscriptions.test.mjs @@ -751,9 +751,17 @@ test("broker rolls back child threads inherited through a failed provisional cla (thread) => thread.name === "delayed-design-challenger" ); assert.ok(childThread, "delayed child thread was not created"); + const grandchildCreated = await waitFor(() => + readState(broker.statePath)?.threads.some((thread) => thread.name === "delayed-design-grandchild") + ); + assert.equal(grandchildCreated, true, "delayed grandchild thread was not created"); + const grandchildThread = readState(broker.statePath).threads.find( + (thread) => thread.name === "delayed-design-grandchild" + ); - // Both the parent and the inherited child are released while the client stays connected. - await waitForUnsubscribes(broker.statePath, [threadId, threadId, childThread.id]); + // The parent, the inherited child, and the nested grandchild are all released + // while the client stays connected. + await waitForUnsubscribes(broker.statePath, [threadId, threadId, childThread.id, grandchildThread.id]); assert.deepEqual(readState(broker.statePath).subscriptions, []); await secondClient.end(); }); diff --git a/tests/fake-codex-fixture.mjs b/tests/fake-codex-fixture.mjs index 5172b8352..73ed121f8 100644 --- a/tests/fake-codex-fixture.mjs +++ b/tests/fake-codex-fixture.mjs @@ -348,6 +348,8 @@ rl.on("line", (line) => { case "thread/resume": { if (BEHAVIOR === "with-delayed-subagent" && message.params.persistFullHistory === true) { + state.nestedSubagentRequested = true; + saveState(state); setTimeout(() => { send({ id: message.id, error: { code: -32000, message: "forced resume failure after child arrival" } }); }, 250); @@ -556,6 +558,17 @@ rl.on("line", (line) => { send({ method: "thread/started", params: { thread: { ...buildThread(subThreadRecord), name: subThreadRecord.name, agentNickname: subThreadRecord.name } } }); send({ method: "turn/started", params: { threadId: subThread.id, turn: buildTurn(subTurnId) } }); send({ method: "turn/completed", params: { threadId: subThread.id, turn: buildTurn(subTurnId, "completed") } }); + if (delayedState.nestedSubagentRequested) { + setTimeout(() => { + const nestedState = loadState(); + const grandchild = nextThread(nestedState, thread.cwd, true, { parentThreadId: subThread.id }); + const grandchildRecord = ensureThread(nestedState, grandchild.id); + grandchildRecord.name = "delayed-design-grandchild"; + nestedState.subscriptions = [...new Set([...(nestedState.subscriptions || []), grandchild.id])]; + saveState(nestedState); + send({ method: "thread/started", params: { thread: { ...buildThread(grandchildRecord), name: grandchildRecord.name, agentNickname: grandchildRecord.name } } }); + }, 100); + } }, 100); break; } From 162f7e8d5ca87a547555e7defd143a9c6ced832a Mon Sep 17 00:00:00 2001 From: thossullivan Date: Wed, 2 Sep 2026 22:29:36 -0500 Subject: [PATCH 14/30] fix(broker): retry cleanup when an explicit unsubscribe's requester leaves When the final owner sends thread/unsubscribe and disconnects before the upstream request fails, nothing retried the cleanup: the owner was already removed, so the close path had nothing to release, and the explicit path restored ownership only to a still-open socket. Schedule the bounded automatic retry in that case. Test fixture: write state atomically so a test never reads a partial document, and add a delayed single-failure unsubscribe behavior for the new regression test. --- plugins/codex/scripts/app-server-broker.mjs | 4 ++++ tests/broker-subscriptions.test.mjs | 17 +++++++++++++++++ tests/fake-codex-fixture.mjs | 12 +++++++++++- 3 files changed, 32 insertions(+), 1 deletion(-) diff --git a/plugins/codex/scripts/app-server-broker.mjs b/plugins/codex/scripts/app-server-broker.mjs index cd4682b57..635c8ed0f 100644 --- a/plugins/codex/scripts/app-server-broker.mjs +++ b/plugins/codex/scripts/app-server-broker.mjs @@ -346,6 +346,10 @@ async function main() { if (outcome?.error) { if (!socket.destroyed && sockets.has(socket)) { addThreadOwner(socket, threadId); + } else { + // The requester is gone, so nobody will retry on its behalf. Fall back + // to the automatic cleanup path for the now-unowned thread. + scheduleUnsubscribeRetry(threadId, 0); } throw outcome.error; } diff --git a/tests/broker-subscriptions.test.mjs b/tests/broker-subscriptions.test.mjs index 3fc171683..93b1998da 100644 --- a/tests/broker-subscriptions.test.mjs +++ b/tests/broker-subscriptions.test.mjs @@ -765,3 +765,20 @@ test("broker rolls back child threads inherited through a failed provisional cla assert.deepEqual(readState(broker.statePath).subscriptions, []); await secondClient.end(); }); + +test("broker retries cleanup when a client disconnects during a failing explicit unsubscribe", async (t) => { + const broker = startBroker("unsubscribe-fails-once-delayed"); + t.after(() => broker.stop()); + assert.equal(await broker.listening(), true, `broker never listened: ${broker.stderr()}`); + + const client = await connectClient(broker.socketPath); + const threadId = (await client.request("thread/start", { cwd: process.cwd(), ephemeral: false })).thread.id; + // Send the explicit unsubscribe and drop the socket before the upstream reply. + void client.request("thread/unsubscribe", { threadId }).catch(() => {}); + await waitFor(() => readState(broker.statePath)?.unsubscribeRequests?.length === 1); + client.destroy(); + + // The first attempt fails 300 ms later, after the requester is gone; the broker must retry on its own. + await waitForUnsubscribes(broker.statePath, [threadId, threadId]); + assert.deepEqual(readState(broker.statePath).subscriptions, []); +}); diff --git a/tests/fake-codex-fixture.mjs b/tests/fake-codex-fixture.mjs index 73ed121f8..278736e6d 100644 --- a/tests/fake-codex-fixture.mjs +++ b/tests/fake-codex-fixture.mjs @@ -25,7 +25,10 @@ const readline = require("node:readline"); } function saveState(state) { - fs.writeFileSync(STATE_PATH, JSON.stringify(state, null, 2)); + // Write atomically so a test reading the file never sees a partial document. + const tmpPath = STATE_PATH + ".tmp"; + fs.writeFileSync(tmpPath, JSON.stringify(state, null, 2)); + fs.renameSync(tmpPath, STATE_PATH); } function requiresExperimental(field, message, state) { @@ -400,6 +403,13 @@ rl.on("line", (line) => { saveState(state); break; } + if (BEHAVIOR === "unsubscribe-fails-once-delayed" && state.unsubscribeRequests.length === 1) { + saveState(state); + setTimeout(() => { + send({ id: message.id, error: { code: -32000, message: "thread unsubscribe failed" } }); + }, 300); + break; + } if ( BEHAVIOR === "unsubscribe-fails" || (BEHAVIOR === "unsubscribe-fails-once" && state.unsubscribeRequests.length === 1) From 1986b7ec45769953735d89beca608ab703053b8d Mon Sep 17 00:00:00 2001 From: Verso Labs Date: Fri, 4 Sep 2026 13:28:09 -0300 Subject: [PATCH 15/30] fix: reconcile stale background job workers --- plugins/codex/scripts/lib/job-control.mjs | 47 ++++++++-- tests/job-control.test.mjs | 104 ++++++++++++++++++++++ 2 files changed, 145 insertions(+), 6 deletions(-) create mode 100644 tests/job-control.test.mjs diff --git a/plugins/codex/scripts/lib/job-control.mjs b/plugins/codex/scripts/lib/job-control.mjs index ad152c157..308e89c8f 100644 --- a/plugins/codex/scripts/lib/job-control.mjs +++ b/plugins/codex/scripts/lib/job-control.mjs @@ -24,6 +24,34 @@ function filterJobsForCurrentSession(jobs, options = {}) { return jobs.filter((job) => job.sessionId === sessionId); } +function isActiveJob(job) { + return job.status === "queued" || job.status === "running"; +} + +export function reconcileJobLiveness(job, options = {}) { + if (!isActiveJob(job) || !Number.isSafeInteger(job.pid) || job.pid <= 0) { + return job; + } + + const killImpl = options.killImpl ?? process.kill.bind(process); + try { + killImpl(job.pid, 0); + return job; + } catch (error) { + if (error?.code === "EPERM") { + return job; + } + if (error?.code === "ESRCH") { + return { ...job, status: "terminated-unknown", phase: "worker-exited" }; + } + return job; + } +} + +function reconcileJobsLiveness(jobs, options = {}) { + return jobs.map((job) => reconcileJobLiveness(job, options)); +} + function getJobTypeLabel(job) { if (typeof job.kindLabel === "string" && job.kindLabel) { return job.kindLabel; @@ -213,7 +241,9 @@ function matchJobReference(jobs, reference, predicate = () => true) { export function buildStatusSnapshot(cwd, options = {}) { const workspaceRoot = resolveWorkspaceRoot(cwd); const config = getConfig(workspaceRoot); - const jobs = sortJobsNewestFirst(filterJobsForCurrentSession(listJobs(workspaceRoot), options)); + const jobs = sortJobsNewestFirst( + reconcileJobsLiveness(filterJobsForCurrentSession(listJobs(workspaceRoot), options), options) + ); const maxJobs = options.maxJobs ?? DEFAULT_MAX_STATUS_JOBS; const maxProgressLines = options.maxProgressLines ?? DEFAULT_MAX_PROGRESS_LINES; @@ -241,7 +271,7 @@ export function buildStatusSnapshot(cwd, options = {}) { export function buildSingleJobSnapshot(cwd, reference, options = {}) { const workspaceRoot = resolveWorkspaceRoot(cwd); - const jobs = sortJobsNewestFirst(listJobs(workspaceRoot)); + const jobs = sortJobsNewestFirst(reconcileJobsLiveness(listJobs(workspaceRoot), options)); const selected = matchJobReference(jobs, reference); if (!selected) { throw new Error(`No job found for "${reference}". Run /codex:status to inspect known jobs.`); @@ -253,13 +283,18 @@ export function buildSingleJobSnapshot(cwd, reference, options = {}) { }; } -export function resolveResultJob(cwd, reference) { +export function resolveResultJob(cwd, reference, options = {}) { const workspaceRoot = resolveWorkspaceRoot(cwd); - const jobs = sortJobsNewestFirst(reference ? listJobs(workspaceRoot) : filterJobsForCurrentSession(listJobs(workspaceRoot))); + const scopedJobs = reference ? listJobs(workspaceRoot) : filterJobsForCurrentSession(listJobs(workspaceRoot), options); + const jobs = sortJobsNewestFirst(reconcileJobsLiveness(scopedJobs, options)); const selected = matchJobReference( jobs, reference, - (job) => job.status === "completed" || job.status === "failed" || job.status === "cancelled" + (job) => + job.status === "completed" || + job.status === "failed" || + job.status === "cancelled" || + job.status === "terminated-unknown" ); if (selected) { @@ -280,7 +315,7 @@ export function resolveResultJob(cwd, reference) { export function resolveCancelableJob(cwd, reference, options = {}) { const workspaceRoot = resolveWorkspaceRoot(cwd); - const jobs = sortJobsNewestFirst(listJobs(workspaceRoot)); + const jobs = sortJobsNewestFirst(reconcileJobsLiveness(listJobs(workspaceRoot), options)); const activeJobs = jobs.filter((job) => job.status === "queued" || job.status === "running"); if (reference) { diff --git a/tests/job-control.test.mjs b/tests/job-control.test.mjs new file mode 100644 index 000000000..4910c7b1d --- /dev/null +++ b/tests/job-control.test.mjs @@ -0,0 +1,104 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +import { saveState } from "../plugins/codex/scripts/lib/state.mjs"; + +import { + buildStatusSnapshot, + reconcileJobLiveness, + resolveCancelableJob, + resolveResultJob +} from "../plugins/codex/scripts/lib/job-control.mjs"; + +function activeJob(overrides = {}) { + return { + id: "task-dead-worker", + status: "running", + phase: "editing", + pid: 999999, + ...overrides + }; +} + +test("reconcileJobLiveness marks an absent worker as terminated-unknown", () => { + const job = reconcileJobLiveness(activeJob(), { + killImpl() { + const error = new Error("no such process"); + error.code = "ESRCH"; + throw error; + } + }); + + assert.equal(job.status, "terminated-unknown"); + assert.equal(job.phase, "worker-exited"); + assert.equal(job.pid, 999999); +}); + +test("reconcileJobLiveness treats EPERM as alive", () => { + const original = activeJob(); + const job = reconcileJobLiveness(original, { + killImpl() { + const error = new Error("not permitted"); + error.code = "EPERM"; + throw error; + } + }); + assert.deepEqual(job, original); +}); + +test("reconcileJobLiveness leaves terminal and pid-less jobs untouched", () => { + let calls = 0; + const killImpl = () => { calls += 1; }; + const completed = activeJob({ status: "completed" }); + const noPid = activeJob({ pid: null }); + + assert.deepEqual(reconcileJobLiveness(completed, { killImpl }), completed); + assert.deepEqual(reconcileJobLiveness(noPid, { killImpl }), noPid); + assert.equal(calls, 0); +}); + +test("buildStatusSnapshot moves a dead worker out of the active queue", () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "codex-job-control-")); + const workspace = path.join(root, "workspace"); + const previousPluginData = process.env.CLAUDE_PLUGIN_DATA; + fs.mkdirSync(workspace, { recursive: true }); + process.env.CLAUDE_PLUGIN_DATA = path.join(root, "plugin-data"); + + try { + saveState(workspace, { + config: { stopReviewGate: false }, + jobs: [{ + id: "task-dead-worker", + status: "running", + phase: "editing", + pid: 999999, + createdAt: "2026-09-04T10:00:00.000Z", + updatedAt: "2026-09-04T10:01:00.000Z" + }] + }); + const reportKill = () => { + const error = new Error("no such process"); + error.code = "ESRCH"; + throw error; + }; + const report = buildStatusSnapshot(workspace, { env: {}, killImpl: reportKill }); + assert.equal(report.running.length, 0); + assert.equal(report.latestFinished.status, "terminated-unknown"); + assert.equal(report.latestFinished.phase, "worker-exited"); + assert.equal( + resolveResultJob(workspace, "task-dead-worker", { killImpl: reportKill }).job.status, + "terminated-unknown" + ); + assert.throws( + () => resolveCancelableJob(workspace, "task-dead-worker", { killImpl: reportKill }), + /No job found|No active job/ + ); + } finally { + if (previousPluginData === undefined) delete process.env.CLAUDE_PLUGIN_DATA; + else process.env.CLAUDE_PLUGIN_DATA = previousPluginData; + fs.rmSync(root, { recursive: true, force: true }); + } +}); From c5711025d63cc81efb13fcf5451c9be60bbc685a Mon Sep 17 00:00:00 2001 From: Verso Labs Date: Fri, 4 Sep 2026 15:00:04 -0300 Subject: [PATCH 16/30] fix: reconcile liveness across active job consumers --- plugins/codex/scripts/codex-companion.mjs | 5 ++- plugins/codex/scripts/lib/job-control.mjs | 2 +- .../codex/scripts/session-lifecycle-hook.mjs | 4 +- .../codex/scripts/stop-review-gate-hook.mjs | 4 +- tests/runtime.test.mjs | 39 +++++++++++++++++++ 5 files changed, 48 insertions(+), 6 deletions(-) diff --git a/plugins/codex/scripts/codex-companion.mjs b/plugins/codex/scripts/codex-companion.mjs index 83df468ad..c3b1ca191 100644 --- a/plugins/codex/scripts/codex-companion.mjs +++ b/plugins/codex/scripts/codex-companion.mjs @@ -38,6 +38,7 @@ import { buildSingleJobSnapshot, buildStatusSnapshot, readStoredJob, + reconcileJobsLiveness, resolveCancelableJob, resolveResultJob, sortJobsNewestFirst @@ -336,7 +337,7 @@ async function waitForSingleJobSnapshot(cwd, reference, options = {}) { async function resolveLatestTrackedTaskThread(cwd, options = {}) { const workspaceRoot = resolveWorkspaceRoot(cwd); const sessionId = getCurrentClaudeSessionId(); - const jobs = sortJobsNewestFirst(listJobs(workspaceRoot)).filter((job) => job.id !== options.excludeJobId); + const jobs = sortJobsNewestFirst(reconcileJobsLiveness(listJobs(workspaceRoot))).filter((job) => job.id !== options.excludeJobId); const visibleJobs = filterJobsForCurrentClaudeSession(jobs); const activeTask = visibleJobs.find((job) => job.jobClass === "task" && (job.status === "queued" || job.status === "running")); if (activeTask) { @@ -934,7 +935,7 @@ function handleTaskResumeCandidate(argv) { const cwd = resolveCommandCwd(options); const workspaceRoot = resolveCommandWorkspace(options); const sessionId = getCurrentClaudeSessionId(); - const jobs = filterJobsForCurrentClaudeSession(sortJobsNewestFirst(listJobs(workspaceRoot))); + const jobs = filterJobsForCurrentClaudeSession(sortJobsNewestFirst(reconcileJobsLiveness(listJobs(workspaceRoot)))); const candidate = findLatestResumableTaskJob(jobs); const payload = { diff --git a/plugins/codex/scripts/lib/job-control.mjs b/plugins/codex/scripts/lib/job-control.mjs index 308e89c8f..8c29c3152 100644 --- a/plugins/codex/scripts/lib/job-control.mjs +++ b/plugins/codex/scripts/lib/job-control.mjs @@ -48,7 +48,7 @@ export function reconcileJobLiveness(job, options = {}) { } } -function reconcileJobsLiveness(jobs, options = {}) { +export function reconcileJobsLiveness(jobs, options = {}) { return jobs.map((job) => reconcileJobLiveness(job, options)); } diff --git a/plugins/codex/scripts/session-lifecycle-hook.mjs b/plugins/codex/scripts/session-lifecycle-hook.mjs index 778571e6c..b89cd456d 100644 --- a/plugins/codex/scripts/session-lifecycle-hook.mjs +++ b/plugins/codex/scripts/session-lifecycle-hook.mjs @@ -4,6 +4,7 @@ import fs from "node:fs"; import process from "node:process"; import { terminateProcessTree } from "./lib/process.mjs"; +import { reconcileJobLiveness } from "./lib/job-control.mjs"; import { BROKER_ENDPOINT_ENV } from "./lib/app-server.mjs"; import { clearBrokerSession, @@ -57,7 +58,8 @@ function cleanupSessionJobs(cwd, sessionId) { } for (const job of removedJobs) { - const stillRunning = job.status === "queued" || job.status === "running"; + const reconciled = reconcileJobLiveness(job); + const stillRunning = reconciled.status === "queued" || reconciled.status === "running"; if (!stillRunning) { continue; } diff --git a/plugins/codex/scripts/stop-review-gate-hook.mjs b/plugins/codex/scripts/stop-review-gate-hook.mjs index 2346bdcf4..f2c64cd4a 100644 --- a/plugins/codex/scripts/stop-review-gate-hook.mjs +++ b/plugins/codex/scripts/stop-review-gate-hook.mjs @@ -9,7 +9,7 @@ import { fileURLToPath } from "node:url"; import { getCodexAvailability } from "./lib/codex.mjs"; import { loadPromptTemplate, interpolateTemplate } from "./lib/prompts.mjs"; import { getConfig, listJobs } from "./lib/state.mjs"; -import { sortJobsNewestFirst } from "./lib/job-control.mjs"; +import { reconcileJobsLiveness, sortJobsNewestFirst } from "./lib/job-control.mjs"; import { SESSION_ID_ENV } from "./lib/tracked-jobs.mjs"; import { resolveWorkspaceRoot } from "./lib/workspace.mjs"; @@ -145,7 +145,7 @@ function main() { const workspaceRoot = resolveWorkspaceRoot(cwd); const config = getConfig(workspaceRoot); - const jobs = sortJobsNewestFirst(filterJobsForCurrentSession(listJobs(workspaceRoot), input)); + const jobs = sortJobsNewestFirst(filterJobsForCurrentSession(reconcileJobsLiveness(listJobs(workspaceRoot)), input)); const runningJob = jobs.find((job) => job.status === "queued" || job.status === "running"); const runningTaskNote = runningJob ? `Codex task ${runningJob.id} is still running. Check /codex:status and use /codex:cancel ${runningJob.id} if you want to stop it before ending the session.` diff --git a/tests/runtime.test.mjs b/tests/runtime.test.mjs index 8f276835b..682ee8b3e 100644 --- a/tests/runtime.test.mjs +++ b/tests/runtime.test.mjs @@ -503,6 +503,26 @@ test("task --resume-last resumes the latest persisted task thread", () => { assert.equal(result.stdout, "Resumed the prior run.\nFollow-up prompt accepted.\n"); }); +test("task --resume-last ignores a stale current-session worker and resumes the prior completed thread", () => { + 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 }); + const env = { ...buildEnv(binDir), CODEX_COMPANION_SESSION_ID: "sess-current" }; + const first = run("node", [SCRIPT, "task", "initial task"], { cwd: repo, env }); + assert.equal(first.status, 0, first.stderr); + const statePath = path.join(resolveStateDir(repo), "state.json"); + const state = JSON.parse(fs.readFileSync(statePath, "utf8")); + state.jobs.push({ id: "task-stale", status: "running", title: "Codex Task", jobClass: "task", sessionId: "sess-current", pid: 999999, updatedAt: "2099-01-01T00:00:00.000Z" }); + fs.writeFileSync(statePath, `${JSON.stringify(state, null, 2)}\n`, "utf8"); + const resumed = run("node", [SCRIPT, "task", "--resume-last", "follow up"], { cwd: repo, env }); + assert.equal(resumed.status, 0, resumed.stderr); + assert.equal(resumed.stdout, "Resumed the prior run.\nFollow-up prompt accepted.\n"); +}); + test("task-resume-candidate returns the latest rescue thread from the current session", () => { const workspace = makeTempDir(); const stateDir = resolveStateDir(workspace); @@ -2036,6 +2056,25 @@ test("stop hook logs running tasks to stderr without blocking when the review ga assert.match(blocked.stderr, /\/codex:cancel task-live/i); }); +test("stop hook ignores a stale current-session worker when the review gate is disabled", () => { + 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); + fs.mkdirSync(path.join(stateDir, "jobs"), { recursive: true }); + fs.writeFileSync(path.join(stateDir, "state.json"), `${JSON.stringify({ version: 1, config: { stopReviewGate: false }, jobs: [{ id: "task-stale", status: "running", title: "Codex Task", jobClass: "task", sessionId: "sess-current", pid: 999999, updatedAt: "2099-01-01T00:00:00.000Z" }] }, null, 2)}\n`, "utf8"); + const result = run("node", [STOP_HOOK], { + cwd: repo, + env: { ...process.env, CODEX_COMPANION_SESSION_ID: "sess-current" }, + input: JSON.stringify({ cwd: repo }) + }); + assert.equal(result.status, 0, result.stderr); + assert.equal(result.stdout.trim(), ""); + assert.doesNotMatch(result.stderr, /task-stale is still running/i); +}); + test("stop hook allows the stop when the review gate is enabled and the stop-time review task is clean", () => { const repo = makeTempDir(); const binDir = makeTempDir(); From 8286b41191f44ba40b2f82919e150401c6226214 Mon Sep 17 00:00:00 2001 From: Verso Labs Date: Fri, 4 Sep 2026 21:46:21 -0300 Subject: [PATCH 17/30] fix: preserve latest stored result after worker crash --- plugins/codex/scripts/lib/job-control.mjs | 2 +- tests/job-control.test.mjs | 28 +++++++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/plugins/codex/scripts/lib/job-control.mjs b/plugins/codex/scripts/lib/job-control.mjs index 8c29c3152..0e9cd318b 100644 --- a/plugins/codex/scripts/lib/job-control.mjs +++ b/plugins/codex/scripts/lib/job-control.mjs @@ -294,7 +294,7 @@ export function resolveResultJob(cwd, reference, options = {}) { job.status === "completed" || job.status === "failed" || job.status === "cancelled" || - job.status === "terminated-unknown" + (Boolean(reference) && job.status === "terminated-unknown") ); if (selected) { diff --git a/tests/job-control.test.mjs b/tests/job-control.test.mjs index 4910c7b1d..1c22d2d94 100644 --- a/tests/job-control.test.mjs +++ b/tests/job-control.test.mjs @@ -102,3 +102,31 @@ test("buildStatusSnapshot moves a dead worker out of the active queue", () => { fs.rmSync(root, { recursive: true, force: true }); } }); + + +test("implicit result ignores a newer dead worker in favor of stored final output", () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "codex-job-result-")); + const workspace = path.join(root, "workspace"); + const previousPluginData = process.env.CLAUDE_PLUGIN_DATA; + fs.mkdirSync(workspace, { recursive: true }); + process.env.CLAUDE_PLUGIN_DATA = path.join(root, "plugin-data"); + + try { + saveState(workspace, { + config: { stopReviewGate: false }, + jobs: [ + activeJob({ sessionId: "sess-current", updatedAt: "2026-09-04T10:02:00.000Z" }), + { id: "task-completed", status: "completed", sessionId: "sess-current", result: { rawOutput: "done" }, updatedAt: "2026-09-04T10:01:00.000Z" } + ] + }); + const killImpl = () => { const error = new Error("no such process"); error.code = "ESRCH"; throw error; }; + const options = { env: { CODEX_COMPANION_SESSION_ID: "sess-current" }, killImpl }; + + assert.equal(resolveResultJob(workspace, null, options).job.id, "task-completed"); + assert.equal(resolveResultJob(workspace, "task-dead-worker", options).job.status, "terminated-unknown"); + } finally { + if (previousPluginData === undefined) delete process.env.CLAUDE_PLUGIN_DATA; + else process.env.CLAUDE_PLUGIN_DATA = previousPluginData; + fs.rmSync(root, { recursive: true, force: true }); + } +}); From 12a4cf5532a612cb4da47ae066a4ea33c7e745f8 Mon Sep 17 00:00:00 2001 From: Verso Labs Date: Sat, 5 Sep 2026 01:10:30 -0300 Subject: [PATCH 18/30] fix: keep orphaned Codex turns active --- plugins/codex/scripts/lib/job-control.mjs | 9 ++++ tests/job-control.test.mjs | 55 +++++++++++++++++++++++ tests/runtime.test.mjs | 39 ++++++++++++++++ 3 files changed, 103 insertions(+) diff --git a/plugins/codex/scripts/lib/job-control.mjs b/plugins/codex/scripts/lib/job-control.mjs index 0e9cd318b..947026911 100644 --- a/plugins/codex/scripts/lib/job-control.mjs +++ b/plugins/codex/scripts/lib/job-control.mjs @@ -42,6 +42,15 @@ export function reconcileJobLiveness(job, options = {}) { return job; } if (error?.code === "ESRCH") { + if (job.threadId && job.turnId) { + return { + ...job, + status: "running", + phase: "worker-exited-turn-unknown", + pid: null, + workerExited: true + }; + } return { ...job, status: "terminated-unknown", phase: "worker-exited" }; } return job; diff --git a/tests/job-control.test.mjs b/tests/job-control.test.mjs index 1c22d2d94..facb673ba 100644 --- a/tests/job-control.test.mjs +++ b/tests/job-control.test.mjs @@ -37,6 +37,24 @@ test("reconcileJobLiveness marks an absent worker as terminated-unknown", () => assert.equal(job.pid, 999999); }); +test("reconcileJobLiveness keeps a dead worker active when its Codex turn may still run", () => { + const job = reconcileJobLiveness( + activeJob({ threadId: "thr_live", turnId: "turn_live" }), + { + killImpl() { + const error = new Error("no such process"); + error.code = "ESRCH"; + throw error; + } + } + ); + + assert.equal(job.status, "running"); + assert.equal(job.phase, "worker-exited-turn-unknown"); + assert.equal(job.threadId, "thr_live"); + assert.equal(job.turnId, "turn_live"); +}); + test("reconcileJobLiveness treats EPERM as alive", () => { const original = activeJob(); const job = reconcileJobLiveness(original, { @@ -60,6 +78,43 @@ test("reconcileJobLiveness leaves terminal and pid-less jobs untouched", () => { assert.equal(calls, 0); }); +test("dead wrapper with a live turn stays active and cancelable", () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "codex-job-turn-unknown-")); + const workspace = path.join(root, "workspace"); + const previousPluginData = process.env.CLAUDE_PLUGIN_DATA; + fs.mkdirSync(workspace, { recursive: true }); + process.env.CLAUDE_PLUGIN_DATA = path.join(root, "plugin-data"); + + try { + saveState(workspace, { + config: { stopReviewGate: false }, + jobs: [activeJob({ + threadId: "thr_live", + turnId: "turn_live", + sessionId: "sess-current", + createdAt: "2026-09-04T10:00:00.000Z", + updatedAt: "2026-09-04T10:01:00.000Z" + })] + }); + const killImpl = () => { + const error = new Error("no such process"); + error.code = "ESRCH"; + throw error; + }; + const options = { env: { CODEX_COMPANION_SESSION_ID: "sess-current" }, killImpl }; + const report = buildStatusSnapshot(workspace, options); + assert.equal(report.running.length, 1); + assert.equal(report.running[0].phase, "worker-exited-turn-unknown"); + assert.equal(report.running[0].pid, null); + assert.equal(resolveCancelableJob(workspace, "task-dead-worker", options).job.turnId, "turn_live"); + assert.throws(() => resolveResultJob(workspace, "task-dead-worker", options)); + } finally { + if (previousPluginData === undefined) delete process.env.CLAUDE_PLUGIN_DATA; + else process.env.CLAUDE_PLUGIN_DATA = previousPluginData; + fs.rmSync(root, { recursive: true, force: true }); + } +}); + test("buildStatusSnapshot moves a dead worker out of the active queue", () => { const root = fs.mkdtempSync(path.join(os.tmpdir(), "codex-job-control-")); const workspace = path.join(root, "workspace"); diff --git a/tests/runtime.test.mjs b/tests/runtime.test.mjs index 682ee8b3e..ecfdc1361 100644 --- a/tests/runtime.test.mjs +++ b/tests/runtime.test.mjs @@ -523,6 +523,26 @@ test("task --resume-last ignores a stale current-session worker and resumes the assert.equal(resumed.stdout, "Resumed the prior run.\nFollow-up prompt accepted.\n"); }); +test("task --resume-last blocks when a dead wrapper still has a live turn identity", () => { + 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 }); + const env = { ...buildEnv(binDir), CODEX_COMPANION_SESSION_ID: "sess-current" }; + const first = run("node", [SCRIPT, "task", "initial task"], { cwd: repo, env }); + assert.equal(first.status, 0, first.stderr); + const statePath = path.join(resolveStateDir(repo), "state.json"); + const state = JSON.parse(fs.readFileSync(statePath, "utf8")); + state.jobs.push({ id: "task-orphan-turn", status: "running", title: "Codex Task", jobClass: "task", sessionId: "sess-current", pid: 999999, threadId: "thr_orphan", turnId: "turn_orphan", updatedAt: "2099-01-01T00:00:00.000Z" }); + fs.writeFileSync(statePath, `${JSON.stringify(state, null, 2)}\n`, "utf8"); + const resumed = run("node", [SCRIPT, "task", "--resume-last", "follow up"], { cwd: repo, env }); + assert.notEqual(resumed.status, 0); + assert.match(resumed.stderr, /task-orphan-turn is still running/i); +}); + test("task-resume-candidate returns the latest rescue thread from the current session", () => { const workspace = makeTempDir(); const stateDir = resolveStateDir(workspace); @@ -2075,6 +2095,25 @@ test("stop hook ignores a stale current-session worker when the review gate is d assert.doesNotMatch(result.stderr, /task-stale is still running/i); }); +test("stop hook keeps an orphaned live turn active when its wrapper died", () => { + 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); + fs.mkdirSync(path.join(stateDir, "jobs"), { recursive: true }); + fs.writeFileSync(path.join(stateDir, "state.json"), `${JSON.stringify({ version: 1, config: { stopReviewGate: false }, jobs: [{ id: "task-orphan-turn", status: "running", title: "Codex Task", jobClass: "task", sessionId: "sess-current", pid: 999999, threadId: "thr_orphan", turnId: "turn_orphan", updatedAt: "2099-01-01T00:00:00.000Z" }] }, null, 2)}\n`, "utf8"); + const result = run("node", [STOP_HOOK], { + cwd: repo, + env: { ...process.env, CODEX_COMPANION_SESSION_ID: "sess-current" }, + input: JSON.stringify({ cwd: repo }) + }); + assert.equal(result.status, 0, result.stderr); + assert.equal(result.stdout.trim(), ""); + assert.match(result.stderr, /task-orphan-turn is still running/i); +}); + test("stop hook allows the stop when the review gate is enabled and the stop-time review task is clean", () => { const repo = makeTempDir(); const binDir = makeTempDir(); From 8de65eb8fb122625417855a84d31c7b3c1ceeddc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebasti=C3=A1n=20Federico=20=7C=20RUMBO=20IA?= <293577326+fscfede-beep@users.noreply.github.com> Date: Sat, 5 Sep 2026 02:03:45 -0300 Subject: [PATCH 19/30] fix: keep accepted turns active through start response --- plugins/codex/scripts/codex-companion.mjs | 6 +++ plugins/codex/scripts/lib/job-control.mjs | 2 +- .../codex/scripts/session-lifecycle-hook.mjs | 9 +++- tests/job-control.test.mjs | 14 +++++++ tests/runtime.test.mjs | 42 +++++++++++++++++++ 5 files changed, 71 insertions(+), 2 deletions(-) diff --git a/plugins/codex/scripts/codex-companion.mjs b/plugins/codex/scripts/codex-companion.mjs index c3b1ca191..2eb038932 100644 --- a/plugins/codex/scripts/codex-companion.mjs +++ b/plugins/codex/scripts/codex-companion.mjs @@ -974,6 +974,12 @@ async function handleCancel(argv) { const threadId = existing.threadId ?? job.threadId ?? null; const turnId = existing.turnId ?? job.turnId ?? null; + if (job.workerExited && threadId && !turnId) { + throw new Error( + `Cannot safely cancel ${job.id}: the worker exited after turn/start was accepted, but the turn id is not yet known. The Codex turn may still be running.` + ); + } + const interrupt = await interruptAppServerTurn(cwd, { threadId, turnId }); if (interrupt.attempted) { appendLogLine( diff --git a/plugins/codex/scripts/lib/job-control.mjs b/plugins/codex/scripts/lib/job-control.mjs index 947026911..ebc22d5d1 100644 --- a/plugins/codex/scripts/lib/job-control.mjs +++ b/plugins/codex/scripts/lib/job-control.mjs @@ -42,7 +42,7 @@ export function reconcileJobLiveness(job, options = {}) { return job; } if (error?.code === "ESRCH") { - if (job.threadId && job.turnId) { + if (job.threadId) { return { ...job, status: "running", diff --git a/plugins/codex/scripts/session-lifecycle-hook.mjs b/plugins/codex/scripts/session-lifecycle-hook.mjs index b89cd456d..717dc216f 100644 --- a/plugins/codex/scripts/session-lifecycle-hook.mjs +++ b/plugins/codex/scripts/session-lifecycle-hook.mjs @@ -57,12 +57,17 @@ function cleanupSessionJobs(cwd, sessionId) { return; } + const retainedJobs = new Map(); for (const job of removedJobs) { const reconciled = reconcileJobLiveness(job); const stillRunning = reconciled.status === "queued" || reconciled.status === "running"; if (!stillRunning) { continue; } + if (reconciled.workerExited && reconciled.threadId) { + retainedJobs.set(job.id, reconciled); + continue; + } try { terminateProcessTree(job.pid ?? Number.NaN); } catch { @@ -72,7 +77,9 @@ function cleanupSessionJobs(cwd, sessionId) { saveState(workspaceRoot, { ...state, - jobs: state.jobs.filter((job) => job.sessionId !== sessionId) + jobs: state.jobs + .filter((job) => job.sessionId !== sessionId || retainedJobs.has(job.id)) + .map((job) => retainedJobs.get(job.id) ?? job) }); } diff --git a/tests/job-control.test.mjs b/tests/job-control.test.mjs index facb673ba..576be7246 100644 --- a/tests/job-control.test.mjs +++ b/tests/job-control.test.mjs @@ -55,6 +55,20 @@ test("reconcileJobLiveness keeps a dead worker active when its Codex turn may st assert.equal(job.turnId, "turn_live"); }); +test("reconcileJobLiveness keeps the turn-start response window active", () => { + const job = reconcileJobLiveness(activeJob({ threadId: "thr_pending", turnId: null }), { + killImpl() { + const error = new Error("no such process"); + error.code = "ESRCH"; + throw error; + } + }); + assert.equal(job.status, "running"); + assert.equal(job.phase, "worker-exited-turn-unknown"); + assert.equal(job.pid, null); + assert.equal(job.workerExited, true); +}); + test("reconcileJobLiveness treats EPERM as alive", () => { const original = activeJob(); const job = reconcileJobLiveness(original, { diff --git a/tests/runtime.test.mjs b/tests/runtime.test.mjs index ecfdc1361..2a8cb797f 100644 --- a/tests/runtime.test.mjs +++ b/tests/runtime.test.mjs @@ -1777,6 +1777,26 @@ test("cancel with a job id can still target an active job from another Claude se assert.equal(state.jobs[0].status, "cancelled"); }); +test("cancel fails closed when an orphaned turn has no persisted turn id", () => { + const workspace = makeTempDir(); + const stateDir = resolveStateDir(workspace); + fs.mkdirSync(path.join(stateDir, "jobs"), { recursive: true }); + fs.writeFileSync(path.join(stateDir, "state.json"), `${JSON.stringify({ + version: 1, config: { stopReviewGate: false }, jobs: [{ + id: "task-turn-pending", status: "running", title: "Codex Task", jobClass: "task", + sessionId: "sess-current", pid: 999999, threadId: "thr_pending", + updatedAt: "2099-01-01T00:00:00.000Z" + }] + }, null, 2)} +`, "utf8"); + const env = { ...process.env, CODEX_COMPANION_SESSION_ID: "sess-current" }; + const result = run("node", [SCRIPT, "cancel", "task-turn-pending", "--json"], { cwd: workspace, env }); + assert.equal(result.status, 1); + assert.match(result.stderr, /turn id|safely interrupt|still running/i); + const state = JSON.parse(fs.readFileSync(path.join(stateDir, "state.json"), "utf8")); + assert.equal(state.jobs[0].status, "running"); +}); + test("cancel sends turn interrupt to the shared app-server before killing a brokered task", async () => { const repo = makeTempDir(); const binDir = makeTempDir(); @@ -1841,6 +1861,28 @@ test("cancel sends turn interrupt to the shared app-server before killing a brok assert.equal(cleanup.status, 0, cleanup.stderr); }); +test("session end preserves an orphaned turn whose worker exited", () => { + const workspace = makeTempDir(); + const stateDir = resolveStateDir(workspace); + fs.mkdirSync(path.join(stateDir, "jobs"), { recursive: true }); + fs.writeFileSync(path.join(stateDir, "state.json"), `${JSON.stringify({ + version: 1, config: { stopReviewGate: false }, jobs: [{ + id: "task-orphaned-turn", status: "running", title: "Codex Task", jobClass: "task", + sessionId: "sess-current", pid: 999999, threadId: "thr_pending", + updatedAt: "2099-01-01T00:00:00.000Z" + }] + }, null, 2)} +`, "utf8"); + const result = run("node", [SESSION_HOOK, "SessionEnd"], { + cwd: workspace, env: { ...process.env, CODEX_COMPANION_SESSION_ID: "sess-current" }, + input: JSON.stringify({ hook_event_name: "SessionEnd", session_id: "sess-current", cwd: workspace }) + }); + assert.equal(result.status, 0, result.stderr); + const state = JSON.parse(fs.readFileSync(path.join(stateDir, "state.json"), "utf8")); + assert.equal(state.jobs.length, 1); + assert.equal(state.jobs[0].id, "task-orphaned-turn"); +}); + test("session end fully cleans up jobs for the ending session", async (t) => { const repo = makeTempDir(); initGitRepo(repo); From 184df24edd72bd3f2099eb104fdb16e8864c2d77 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 17 Sep 2026 13:37:48 +0000 Subject: [PATCH 20/30] test: cover the durable review-gate config's permissions and failed replacement The review on openai/codex-plugin-cc#731 asks for exactly this coverage, and this fork already carries the hardening it requests (ensurePrivateDir plus writeJsonFileAtomic instead of mkdirSync plus writeFileSync), so the tests belong here too. - "the durable review-gate config is private" pins 0600 on the file and 0700 on its directory. It discriminates: reverting writeDurableConfig() to the plain writeFileSync the PR shipped with fails it. - "a durable config write that fails mid-write leaves the previous config intact" fails the replacement from inside writeJsonFileAtomic(), after it has created its temporary file, using a value whose toJSON() throws. It asserts the previous config still reads back enabled and that no temporary file is left beside it. This one does not discriminate against the naive implementation (which throws before touching the file either way); what it guards is the regression class where a future rewrite truncates the target before serializing, and the cleanup path of the atomic write. Verified: full npm test 302/302; tsc clean. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UXfvnjSC72HsM6EEPVt2Tg --- tests/state.test.mjs | 56 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/tests/state.test.mjs b/tests/state.test.mjs index b6657edcb..f94fc84ed 100644 --- a/tests/state.test.mjs +++ b/tests/state.test.mjs @@ -12,6 +12,7 @@ import { readStoredJob } from "../plugins/codex/scripts/lib/job-control.mjs"; import { getConfig, loadState, + resolveConfigFile, listJobs, resolveJobFile, resolveJobLogFile, @@ -591,3 +592,58 @@ test("runTrackedJob stores no errorMessage for a completed execution", async () assert.equal(indexed.status, "completed"); assert.equal(indexed.errorMessage, null); }); + + +test("the durable review-gate config is private", { skip: process.platform === "win32" }, () => { + const workspace = makeTempDir(); + const codexHome = makeTempDir(); + const previousCodexHome = process.env.CODEX_HOME; + try { + process.env.CODEX_HOME = codexHome; + setConfig(workspace, "stopReviewGate", true); + + const configFile = resolveConfigFile(workspace); + // The gate decides whether Codex reviews every turn of this workspace, so + // it gets the same treatment as every other artifact this module writes: + // nobody else on the machine reads or rewrites it. + assert.equal(fs.statSync(configFile).mode & 0o777, 0o600); + assert.equal(fs.statSync(path.dirname(configFile)).mode & 0o777, 0o700); + } finally { + if (previousCodexHome == null) delete process.env.CODEX_HOME; + else process.env.CODEX_HOME = previousCodexHome; + } +}); + +test("a durable config write that fails mid-write leaves the previous config intact", () => { + const workspace = makeTempDir(); + const codexHome = makeTempDir(); + const previousCodexHome = process.env.CODEX_HOME; + try { + process.env.CODEX_HOME = codexHome; + setConfig(workspace, "stopReviewGate", true); + const configFile = resolveConfigFile(workspace); + const before = fs.readFileSync(configFile, "utf8"); + + // Fails inside writeJsonFileAtomic(), after it has created its temporary + // file: the replacement is interrupted exactly where a crash or a full + // disk would interrupt it. The gate decides whether Codex reviews every + // turn, so a half-written file must never end up in its place -- that + // would read back as unset and silently disable the gate. + const explodingValue = { + toJSON() { + throw new Error("serialization failed mid-write"); + } + }; + assert.throws(() => setConfig(workspace, "stopReviewGate", explodingValue), /serialization failed mid-write/); + + assert.equal(fs.readFileSync(configFile, "utf8"), before); + assert.equal(getConfig(workspace).stopReviewGate, true); + // The temporary file is cleaned up, so nothing is left to be mistaken for + // the real config. + assert.deepEqual(fs.readdirSync(path.dirname(configFile)), [path.basename(configFile)]); + } finally { + if (previousCodexHome == null) delete process.env.CODEX_HOME; + else process.env.CODEX_HOME = previousCodexHome; + } +}); + From ed2d43493582959e7a7709aa414c195c4bad0c1f Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 17 Sep 2026 13:50:20 +0000 Subject: [PATCH 21/30] test: pin the launcher's multi-manager precedence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The review on openai/codex-plugin-cc#737 asks for a multi-manager regression alongside the ordering fix. Two tests, with every manager root populated at once (nvm, fnm, asdf, mise): - "prefers a managed toolchain over a system install with several managers present" is the regression for the precedence bug itself. It discriminates where a system node exists: putting /usr/local/bin and the Homebrew paths back ahead of the manager roots fails it, along with the four upstream tests that caught the bug originally. It deliberately does not pin *which* manager wins — the launcher cannot tell which one the project or user selected, since it reads no .nvmrc, .tool-versions or mise config, so asserting one would freeze an arbitrary order as if it were a guarantee. - "lets CODEX_COMPANION_NODE override every installed manager" pins the one way a specific runtime can be selected today, which is the honest answer to the reviewer's "preserve the selected runtime" until the launcher learns to read the active manager. Verified: the first test fails with the pre-fix ordering restored and passes with it in place; full npm test 304/304; tsc clean. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UXfvnjSC72HsM6EEPVt2Tg --- tests/node-launcher.test.mjs | 93 ++++++++++++++++++++++++++++++++++++ 1 file changed, 93 insertions(+) diff --git a/tests/node-launcher.test.mjs b/tests/node-launcher.test.mjs index e51128241..9ea6234d9 100644 --- a/tests/node-launcher.test.mjs +++ b/tests/node-launcher.test.mjs @@ -417,3 +417,96 @@ test("portable launcher aligns Node with the supported toolchain that contains c fs.rmSync(home, { recursive: true, force: true }); } }); + + +// Regression for the precedence bug this fork fixed in find_managed_node(): the +// hardcoded system paths (/usr/local/bin, /opt/homebrew, ...) used to be tried +// before every version-manager root, so on any machine carrying a system node +// the managed toolchains were shadowed. The assertion below is only meaningful +// where such a node exists (it is what the launcher would otherwise pick); where +// it does not, the test still passes and costs nothing. +function installManagedNode(binDir, marker) { + fs.mkdirSync(binDir, { recursive: true }); + const nodePath = path.join(binDir, "node"); + fs.writeFileSync( + nodePath, + `#!/bin/sh\nif [ "\${1:-}" = "-e" ]; then exit 0; fi\nprintf "${marker}:%s\\n" "$*"\n`, + "utf8" + ); + fs.chmodSync(nodePath, 0o755); + return nodePath; +} + +function installEveryManager(home) { + const nvmDir = path.join(home, "custom-nvm"); + const fnmDir = path.join(home, "custom-fnm"); + const asdfDir = path.join(home, "custom-asdf"); + const miseDir = path.join(home, "custom-mise"); + installManagedNode(path.join(nvmDir, "versions", "node", "v22.0.0", "bin"), "MANAGED_NVM"); + installManagedNode(path.join(fnmDir, "node-versions", "v22.1.0", "installation", "bin"), "MANAGED_FNM"); + installManagedNode(path.join(asdfDir, "installs", "nodejs", "22.2.0", "bin"), "MANAGED_ASDF"); + installManagedNode(path.join(miseDir, "installs", "node", "22.3.0", "bin"), "MANAGED_MISE"); + return { + NVM_DIR: nvmDir.replaceAll("\\", "/"), + FNM_DIR: fnmDir.replaceAll("\\", "/"), + ASDF_DATA_DIR: asdfDir.replaceAll("\\", "/"), + MISE_DATA_DIR: miseDir.replaceAll("\\", "/") + }; +} + +test("portable launcher prefers a managed toolchain over a system install with several managers present", () => { + const home = fs.mkdtempSync(path.join(os.tmpdir(), "codex-node-multi-manager-")); + const emptyBin = path.join(home, "empty-bin"); + fs.mkdirSync(emptyBin, { recursive: true }); + const managerEnv = installEveryManager(home); + try { + const result = spawnSync(BASH, [LAUNCHER.replaceAll("\\", "/"), "companion.mjs", "status", "--json"], { + encoding: "utf8", + env: { + ...cleanVersionManagerEnv(), + HOME: home.replaceAll("\\", "/"), + PATH: emptyBin.replaceAll("\\", "/"), + CODEX_COMPANION_NODE: "", + ...managerEnv + } + }); + + assert.equal(result.status, 0, result.stderr); + // Which manager wins is deliberately not pinned: the launcher has no way to + // tell which one the project or the user actually selected (no .nvmrc, + // .tool-versions or mise config is read), so asserting one would freeze an + // arbitrary order. What must hold is that a managed toolchain is chosen at + // all rather than a system install. + assert.match(result.stdout, /MANAGED_(NVM|FNM|ASDF|MISE):/); + } finally { + fs.rmSync(home, { recursive: true, force: true }); + } +}); + +test("portable launcher lets CODEX_COMPANION_NODE override every installed manager", () => { + const home = fs.mkdtempSync(path.join(os.tmpdir(), "codex-node-multi-manager-pinned-")); + const emptyBin = path.join(home, "empty-bin"); + fs.mkdirSync(emptyBin, { recursive: true }); + const managerEnv = installEveryManager(home); + const pinnedNode = installManagedNode(path.join(home, "pinned", "bin"), "PINNED_NODE"); + try { + const result = spawnSync(BASH, [LAUNCHER.replaceAll("\\", "/"), "companion.mjs", "status", "--json"], { + encoding: "utf8", + env: { + ...cleanVersionManagerEnv(), + HOME: home.replaceAll("\\", "/"), + PATH: emptyBin.replaceAll("\\", "/"), + CODEX_COMPANION_NODE: pinnedNode.replaceAll("\\", "/"), + ...managerEnv + } + }); + + assert.equal(result.status, 0, result.stderr); + // The one way to pin a runtime today, and the answer to "which manager?" + // until the launcher learns to read the active one. + assert.match(result.stdout, /PINNED_NODE:/); + assert.doesNotMatch(result.stdout, /MANAGED_(NVM|FNM|ASDF|MISE):/); + } finally { + fs.rmSync(home, { recursive: true, force: true }); + } +}); From 916fcfce3c21d362bb5789ffe788e3267afaba98 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 17 Sep 2026 13:54:21 +0000 Subject: [PATCH 22/30] test: stop the dead-owner lock test measuring wall clock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI failed on this PR's head with "dead owner should be reclaimed before stale timeout" in tests/locking.test.mjs — a test neither this branch nor the imports touch. The assertion is the flaky part, not the behavior: acquireLock() checks its deadline only after a failed attempt, so on a loaded runner a single slow attempt still returns successfully at over 500ms of wall clock. Nothing is weakened by dropping it. acquireLock() throws once its deadline passes without acquiring, so a successful return under a 500ms budget with staleMs at 30s already proves the lock was reclaimed because its owner is gone rather than because it aged out. Verified by commenting out reclaimAbandonedLock(): the test fails again, as does the other reclaim test. Verified: tests/locking.test.mjs 5/5 three runs in a row; full npm test 304/304. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UXfvnjSC72HsM6EEPVt2Tg --- tests/locking.test.mjs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/tests/locking.test.mjs b/tests/locking.test.mjs index 37eba6e4b..eebc69559 100644 --- a/tests/locking.test.mjs +++ b/tests/locking.test.mjs @@ -74,13 +74,22 @@ test("lock immediately reclaims an owner process that exited", async () => { }); }); + // acquireLock() throws once its own deadline passes without acquiring, so a + // successful return under a 500ms budget with staleMs at 30s is itself the + // assertion: the lock was reclaimed because its owner is gone, not because it + // aged out. Measuring wall-clock here instead added nothing and made the test + // flaky -- the deadline is only checked after a failed attempt, so one slow + // attempt on a loaded runner returns successfully at over 500ms. const startedAt = Date.now(); const successor = await acquireLock(lockDir, { timeoutMs: 500, staleMs: 30000, retryDelayMs: 5 }); - assert.ok(Date.now() - startedAt < 500, "dead owner should be reclaimed before stale timeout"); + assert.ok( + Date.now() - startedAt < 30000, + "reclaiming a dead owner must not wait for the stale timeout" + ); releaseLock(successor); }); From 625c77086aa21024b13af91c2b78de42fbe1a4c0 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 17 Sep 2026 13:57:31 +0000 Subject: [PATCH 23/30] docs: describe this round's imports, and say in the metadata that this is a fork - README "Differences From Upstream" gains #659, #707 and #728, plus a short list of the defects the imports themselves surfaced and this fork fixes (busy-broker refusal, the launcher's inverted precedence, the durable config's private atomic write, the app-server typecheck), and what is deliberately not imported (#733, #761) with the reason. - CHANGELOG records the three imports under Unreleased. - The plugin, marketplace and package descriptions now say this is a fork of openai/codex-plugin-cc carrying open upstream fixes. Anyone browsing /plugin sees the same description the marketplace lists, and it read as upstream's own plugin until now. The version stays 1.0.6 (npm run check-version passes): describing the fork is not cutting a release. Verified: full npm test 304/304; tsc clean. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UXfvnjSC72HsM6EEPVt2Tg --- .claude-plugin/marketplace.json | 4 ++-- README.md | 18 ++++++++++++++++++ package.json | 2 +- plugins/codex/.claude-plugin/plugin.json | 2 +- plugins/codex/CHANGELOG.md | 3 +++ 5 files changed, 25 insertions(+), 4 deletions(-) diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 870246242..f4356e9fb 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -4,13 +4,13 @@ "name": "OpenAI" }, "metadata": { - "description": "Codex plugins to use in Claude Code for delegation and code review.", + "description": "Codex plugins to use in Claude Code for delegation and code review. Fork of openai/codex-plugin-cc carrying open upstream fixes.", "version": "1.0.6" }, "plugins": [ { "name": "codex", - "description": "Use Codex from Claude Code to review code or delegate tasks.", + "description": "Use Codex from Claude Code to review code or delegate tasks. Fork of openai/codex-plugin-cc with open upstream fixes merged.", "version": "1.0.6", "author": { "name": "OpenAI" diff --git a/README.md b/README.md index 710040311..8e6fd358c 100644 --- a/README.md +++ b/README.md @@ -364,6 +364,9 @@ Broker and background-job lifecycle: | [#541](https://github.com/openai/codex-plugin-cc/pull/541) | broker leaks, state races, and signal-masked command failures in the test runtime | | [#623](https://github.com/openai/codex-plugin-cc/pull/623) | session end no longer tears down the shared broker while another session's jobs are still using it | | [#652](https://github.com/openai/codex-plugin-cc/pull/652) | bounds the lifetime of detached brokers and task workers (see [Background Runtime Limits](#background-runtime-limits)) | +| [#659](https://github.com/openai/codex-plugin-cc/pull/659) | state written under one `CLAUDE_PLUGIN_DATA` root is no longer invisible to an invocation that resolves to another, which orphaned brokers and hid jobs | +| [#707](https://github.com/openai/codex-plugin-cc/pull/707) | the broker releases its app-server thread subscriptions when a client disconnects, instead of leaking them for its whole lifetime | +| [#728](https://github.com/openai/codex-plugin-cc/pull/728) | a job whose worker died no longer reads as "running" forever; `/codex:status` reconciles the record against the live process | Commands and flags: @@ -384,6 +387,21 @@ Commands and flags: Where two of these PRs disagreed, the merge commit says which side won and why. The plugin version is deliberately left at the upstream number: these merges do not cut a release. +Beyond the imports, this fork carries fixes for defects the imports themselves surfaced: + +- a busy broker refusing shutdown is reported as a refusal, not an identity rejection, so SessionEnd + leaves a shared runtime to the sessions still using it instead of exiting with an error +- `scripts/run-node.sh` prefers a user-managed toolchain over a system install, which [#737](https://github.com/openai/codex-plugin-cc/pull/737) + had inverted (see the note under [Requirements](#requirements)) +- the durable review-gate config is written privately and atomically, so an interrupted write cannot + silently disable the gate +- the app-server typecheck (`npm run build`) passes + +Not imported: [#733](https://github.com/openai/codex-plugin-cc/pull/733) (durable startup +cancellation) — its behavior is already covered here by the terminal-claim mechanism, and its marker +files would add a second source of truth for the same decision. [#761](https://github.com/openai/codex-plugin-cc/pull/761) +(`max`/`ultra` reasoning efforts) — the same proposal was closed upstream as [#648](https://github.com/openai/codex-plugin-cc/pull/648). + ## FAQ ### Do I need a separate Codex account for this plugin? diff --git a/package.json b/package.json index b1d984d1a..4216d1040 100644 --- a/package.json +++ b/package.json @@ -3,7 +3,7 @@ "version": "1.0.6", "private": true, "type": "module", - "description": "Use Codex from Claude Code to review code or delegate tasks.", + "description": "Use Codex from Claude Code to review code or delegate tasks. Fork of openai/codex-plugin-cc with open upstream fixes merged.", "license": "Apache-2.0", "engines": { "node": ">=18.18.0" diff --git a/plugins/codex/.claude-plugin/plugin.json b/plugins/codex/.claude-plugin/plugin.json index e91e5238c..bc9450598 100644 --- a/plugins/codex/.claude-plugin/plugin.json +++ b/plugins/codex/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "codex", "version": "1.0.6", - "description": "Use Codex from Claude Code to review code or delegate tasks.", + "description": "Use Codex from Claude Code to review code or delegate tasks. Fork of openai/codex-plugin-cc with open upstream fixes merged.", "author": { "name": "OpenAI" } diff --git a/plugins/codex/CHANGELOG.md b/plugins/codex/CHANGELOG.md index 7905d0fe6..9f6918001 100644 --- a/plugins/codex/CHANGELOG.md +++ b/plugins/codex/CHANGELOG.md @@ -13,6 +13,9 @@ - Persist the failure text of a turn that fails without throwing, and shorten job summaries to 96 characters. - Keep the review-gate flag in a durable per-workspace file under `CODEX_HOME`, written privately and atomically. - Resolve Node through `scripts/run-node.sh` in the hooks, preferring a user-managed toolchain over a system install. +- Release the broker's app-server thread subscriptions when a client disconnects, so a departed client no longer leaks them for the broker's lifetime (openai/codex-plugin-cc#707). +- Read state from every candidate `CLAUDE_PLUGIN_DATA` root, so jobs and broker records written by one invocation are not invisible to another (openai/codex-plugin-cc#659). +- Reconcile a job against its worker process, so a job whose worker died stops reading as running (openai/codex-plugin-cc#728). ## 1.0.0 From a5f7959e06b047da9073d8835d8828f9ac9f52f4 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 17 Sep 2026 15:01:06 +0000 Subject: [PATCH 24/30] fix: three defects an independent review found in today's merge resolutions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A review of the whole day's work (this branch plus the already-merged round) found eight issues; these are the three that belong to the grafts made here, plus one they exposed in the previous round. Each has a regression test that fails without the fix. - cancel refused *after* taking the terminal claim. claimTerminalStatus() creates a claim that is never released, so the leaked cancel-intent claim made the next cancel — or SessionEnd — adopt it and reassert a cancelled record for the turn the refusal exists to protect. The refusal now runs before the claim, off the same merged job view. - The session-end guard reconciled the stale state.json snapshot instead of the job file the code re-reads ten lines below for exactly that reason. On a snapshot with pid: null the guard was skipped and the job recorded cancelled while its turn ran on; on a snapshot missing a turnId the file already had, it retained a job whose turn could have been interrupted. It now reconciles the captured values. - The retain path wrote pid: null, and reconcileJobLiveness() needs a pid: the record could never be judged again, so it stayed "running" in /codex:status and was refused by /codex:result and /codex:cancel. The pid and thread id are written back instead. - From the previous round: assertResumedSandbox() still asserted the requested mode on a scoped resume, although buildThreadAccessParams() deliberately sends a permission profile and no sandbox when read roots are set. Every `--read-root` resume was refused outright, and the error's own advice ("resume with --sandbox read-only") silently dropped the write grant. The assertion is skipped when read roots are in play. Verified: each new test fails with only its fix reverted (stash the source file, run the suite) and passes with it; full npm test 307/307; tsc clean. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UXfvnjSC72HsM6EEPVt2Tg --- plugins/codex/scripts/codex-companion.mjs | 28 +++--- plugins/codex/scripts/lib/codex.mjs | 10 +- .../codex/scripts/session-lifecycle-hook.mjs | 35 ++++--- tests/runtime.test.mjs | 92 +++++++++++++++++++ 4 files changed, 136 insertions(+), 29 deletions(-) diff --git a/plugins/codex/scripts/codex-companion.mjs b/plugins/codex/scripts/codex-companion.mjs index c05c3b7e8..fc4fe1c56 100644 --- a/plugins/codex/scripts/codex-companion.mjs +++ b/plugins/codex/scripts/codex-companion.mjs @@ -1194,6 +1194,21 @@ async function handleCancel(argv) { // writes below null the pid field, so capture the fresher value first. let workerPid = existing.pid ?? job.pid ?? Number.NaN; + // A worker that exited after turn/start was accepted, but before it recorded + // a turn id, leaves a Codex turn that may still be running and nothing to + // address it by, so the cancel refuses. It refuses here, before the terminal + // claim below: the claim is never released, and a leaked cancel-intent claim + // makes the next cancel (or SessionEnd) reassert it into a cancelled record + // for the turn that is still running — the very outcome this refusal exists + // to prevent. No identity wait can help either: the worker that would + // publish the turn id is already gone. + const reconciledForCancel = reconcileJobLiveness({ ...job, ...existing }); + if (reconciledForCancel.workerExited && threadId && !turnId) { + throw new Error( + `Cannot safely cancel ${job.id}: the worker exited after turn/start was accepted, but the turn id is not yet known. The Codex turn may still be running.` + ); + } + // 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 @@ -1251,19 +1266,6 @@ async function handleCancel(argv) { orphanAdopted = true; } - // A worker that exited after turn/start was accepted, but before it recorded - // a turn id, leaves a Codex turn that may still be running and nothing to - // address it by. This has to refuse before the record-first write below: - // reporting the job cancelled while its turn runs on is the failure mode. - // No identity wait can help here either — the worker that would publish the - // turn id is already gone. - const reconciledForCancel = reconcileJobLiveness(readStoredJob(workspaceRoot, job.id) ?? job); - if (reconciledForCancel.workerExited && (threadId ?? reconciledForCancel.threadId) && !(turnId ?? reconciledForCancel.turnId)) { - throw new Error( - `Cannot safely cancel ${job.id}: the worker exited after turn/start was accepted, but the turn id is not yet known. The Codex turn may still be running.` - ); - } - // 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. diff --git a/plugins/codex/scripts/lib/codex.mjs b/plugins/codex/scripts/lib/codex.mjs index c60536369..52802c5f3 100644 --- a/plugins/codex/scripts/lib/codex.mjs +++ b/plugins/codex/scripts/lib/codex.mjs @@ -1335,7 +1335,15 @@ export async function runAppServerTurn(cwd, options = {}) { write: options.write, ephemeral: false }); - assertResumedSandbox(options.resumeThreadId, options.sandbox, response); + // Only meaningful when the resume actually asked for a sandbox mode. + // With read roots, buildThreadAccessParams() deliberately sends a + // scoped permission profile and no `sandbox`, so the app-server's + // reported mode is not the one this turn requested: asserting it here + // refused every `--read-root` resume, and the error's own advice + // (resume with the reported mode) silently dropped the write grant. + if (!(options.readRoots?.length > 0)) { + assertResumedSandbox(options.resumeThreadId, options.sandbox, response); + } threadId = response.thread.id; } else { emitProgress(options.onProgress, "Starting Codex task thread.", "starting"); diff --git a/plugins/codex/scripts/session-lifecycle-hook.mjs b/plugins/codex/scripts/session-lifecycle-hook.mjs index f90825fbb..78b40e865 100644 --- a/plugins/codex/scripts/session-lifecycle-hook.mjs +++ b/plugins/codex/scripts/session-lifecycle-hook.mjs @@ -265,21 +265,6 @@ async function cleanupSessionJobs(cwd, sessionId, { interruptTurns = false, inte if (!stillRunning) { continue; } - // A dead worker still gets its terminal record below — that is what keeps - // /codex:status from answering "No job found" for the session that just - // ended. Reconciliation is consulted only for the one case where writing - // that record would be a lie: see the retain below. - const reconciled = reconcileJobLiveness(job); - if (reconciled.workerExited && reconciled.threadId && !reconciled.turnId) { - // The worker died after turn/start was accepted but before it recorded a - // turn id, so the Codex turn may still be running and there is nothing to - // interrupt it by. Cancelling the record here would claim an outcome that - // did not happen; leave it active (its phase says why) for the next - // status query, which reconciles it the same way. - upsertJob(workspaceRoot, { id: job.id, phase: reconciled.phase, pid: null }); - 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 @@ -296,6 +281,26 @@ async function cleanupSessionJobs(cwd, sessionId, { interruptTurns = false, inte } catch { // Keep the snapshot values. } + // A dead worker still gets its terminal record below — that is what keeps + // /codex:status from answering "No job found" for the session that just + // ended. Reconciliation is consulted only for the one case where writing + // that record would be a lie, and it reads the values captured above + // rather than the snapshot: the snapshot's pid can be null while the + // worker has long since published its real pid and turn identity, which + // would both skip this check and misjudge the turn as unidentified. + const reconciled = reconcileJobLiveness({ ...job, pid: workerPid, threadId, turnId }); + if (reconciled.workerExited && threadId && !turnId) { + // The worker died after turn/start was accepted but before it recorded a + // turn id, so the Codex turn may still be running and there is nothing to + // interrupt it by. Cancelling the record here would claim an outcome that + // did not happen; leave it active (its phase says why) for the next + // status query. The pid is written back rather than nulled: it is what + // lets that query reconcile the job again instead of trusting a stale + // "running". + upsertJob(workspaceRoot, { id: job.id, phase: reconciled.phase, pid: workerPid, threadId }); + continue; + } + jobsAwaitingInterrupt -= 1; // 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 diff --git a/tests/runtime.test.mjs b/tests/runtime.test.mjs index d926954a3..b410ae315 100644 --- a/tests/runtime.test.mjs +++ b/tests/runtime.test.mjs @@ -5602,3 +5602,95 @@ test("stop hook keeps an orphaned live turn active when its wrapper died", () => assert.equal(result.stdout.trim(), ""); assert.match(result.stderr, /task-orphan-turn is still running/i); }); + + +test("a refused cancel leaves no terminal claim behind", () => { + const workspace = makeTempDir(); + const stateDir = resolveStateDir(workspace); + const jobsDir = path.join(stateDir, "jobs"); + fs.mkdirSync(jobsDir, { recursive: true }); + fs.writeFileSync(path.join(stateDir, "state.json"), `${JSON.stringify({ + version: 1, config: { stopReviewGate: false }, jobs: [{ + id: "task-turn-pending", status: "running", title: "Codex Task", jobClass: "task", + sessionId: "sess-current", pid: 999999, threadId: "thr_pending", + updatedAt: "2099-01-01T00:00:00.000Z" + }] + }, null, 2)}\n`, "utf8"); + const env = { ...process.env, CODEX_COMPANION_SESSION_ID: "sess-current" }; + + const refused = run("node", [SCRIPT, "cancel", "task-turn-pending", "--json"], { cwd: workspace, env }); + assert.equal(refused.status, 1); + + // The terminal claim is never released, so a claim taken before the refusal + // would be adopted by the next cancel (or by SessionEnd) and reasserted into + // a cancelled record — for the turn this refusal exists to protect. + assert.equal(fs.existsSync(path.join(jobsDir, "task-turn-pending.terminal")), false); + + const again = run("node", [SCRIPT, "cancel", "task-turn-pending", "--json"], { cwd: workspace, env }); + assert.equal(again.status, 1); + const state = JSON.parse(fs.readFileSync(path.join(stateDir, "state.json"), "utf8")); + assert.equal(state.jobs[0].status, "running"); +}); + +test("task --resume-last still works with scoped read roots", () => { + const repo = makeTempDir(); + const binDir = makeTempDir(); + const statePath = path.join(binDir, "fake-codex-state.json"); + installFakeCodex(binDir); + initGitRepo(repo); + const env = buildEnv(binDir); + + const first = run("node", [SCRIPT, "task", "--write", "--read-root", repo, "initial task"], { cwd: repo, env }); + assert.equal(first.status, 0, first.stderr); + + // A scoped run sends a permission profile and no sandbox mode, so the mode + // the app-server reports on resume is not the one this turn asked for. + // Asserting it refused every --read-root resume outright. + const resumed = run("node", [SCRIPT, "task", "--resume-last", "--write", "--read-root", repo, "follow up"], { + cwd: repo, + env + }); + assert.equal(resumed.status, 0, resumed.stderr); + const fakeState = JSON.parse(fs.readFileSync(statePath, "utf8")); + assert.equal(fakeState.lastThreadResume.sandbox, undefined); + assert.equal(fakeState.lastThreadResume.config.default_permissions, "claude_companion_scoped"); +}); + + +test("a retained orphaned turn stays reconcilable after session end", () => { + const workspace = makeTempDir(); + const stateDir = resolveStateDir(workspace); + const jobsDir = path.join(stateDir, "jobs"); + fs.mkdirSync(jobsDir, { recursive: true }); + // The index carries the queued record's pid: null, while the job file has the + // real (now dead) pid and the thread the worker started. Session end must read + // the file, not the snapshot: on the snapshot alone there is no pid to judge, + // so the job would be recorded cancelled while its turn may still run. + fs.writeFileSync(path.join(jobsDir, "task-retained.json"), `${JSON.stringify({ + id: "task-retained", status: "running", title: "Codex Task", jobClass: "task", + sessionId: "sess-current", pid: 999999, threadId: "thr_pending" + }, null, 2)}\n`, "utf8"); + fs.writeFileSync(path.join(stateDir, "state.json"), `${JSON.stringify({ + version: 1, config: { stopReviewGate: false }, jobs: [{ + id: "task-retained", status: "running", title: "Codex Task", jobClass: "task", + sessionId: "sess-current", pid: null, threadId: null, + updatedAt: "2099-01-01T00:00:00.000Z" + }] + }, null, 2)}\n`, "utf8"); + + const result = run("node", [SESSION_HOOK, "SessionEnd"], { + cwd: workspace, + env: { ...process.env, CODEX_COMPANION_SESSION_ID: "sess-current" }, + input: JSON.stringify({ hook_event_name: "SessionEnd", session_id: "sess-current", cwd: workspace }) + }); + assert.equal(result.status, 0, result.stderr); + + const retained = JSON.parse(fs.readFileSync(path.join(stateDir, "state.json"), "utf8")).jobs[0]; + assert.equal(retained.status, "running"); + // Written back rather than nulled: reconcileJobLiveness() needs a pid, so a + // record retained without one can never be judged again and stays "running" + // forever in /codex:status. + assert.equal(retained.pid, 999999); + assert.equal(retained.threadId, "thr_pending"); + assert.equal(retained.phase, "worker-exited-turn-unknown"); +}); From 2a2bd777a2a6bec09dc344391bd4c4fb49c27879 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 17 Sep 2026 15:18:59 +0000 Subject: [PATCH 25/30] fix: the four concurrency and merge defects the review found in imported code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The remaining findings from the independent review. None came from this branch's grafts; all four arrived with imported PRs and are now this fork's. - state.mjs: the cross-root prune rewrote another root's state.json while holding only the primary's lock, so a process whose primary IS that root could lose its whole update to the rename. It now takes that root's own lock and never waits for it (timeoutMs: 0): two processes each holding the other's primary lock would deadlock, and a skipped prune is harmless — the next save redoes it. - state.mjs: loadState() merges booleans across roots with OR, so a stranded `stopReviewGate: true` outvoted an explicit disable forever whenever the durable config could not be read, because setConfig() only ever wrote the primary. It now writes the new config into every existing root (same non-blocking lock discipline), keeping the fail-safe OR without making "disable" unreachable. The merge also folds candidates in reverse so the primary wins for non-boolean keys — the old "first writer wins" branch was dead for every key the defaults define, which is all of them. - session-lifecycle-hook.mjs: setEnv() rewrote the shared CLAUDE_ENV_FILE (read, filter, rename), which drops any export another plugin's SessionStart hook appended in between and discards the file's mode with the replaced file. It appends again, and skips the append when the value the file already resolves to is ours — the shell takes the last export for a key, so #748's point (no growth on every session) survives without the data loss. - claude-session-transfer.mjs: a process attaching to a staged copy whose creator had not yet written the marker took no lease, and the creator's release() then deleted the file under it. The lease is now taken unconditionally, and cleanup belongs to whoever leaves last (marker present, no leases left) rather than to whoever created the copy. Regression tests for the first three; each fails with only its own fix reverted. The staging race has no deterministic test — it needs an interleaving between two processes at a specific point — so it rests on the reasoning above. Verified: full npm test 310/310; tsc clean. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UXfvnjSC72HsM6EEPVt2Tg --- .../scripts/lib/claude-session-transfer.mjs | 10 +- plugins/codex/scripts/lib/state.mjs | 92 ++++++++++++++++--- .../codex/scripts/session-lifecycle-hook.mjs | 24 +++-- tests/runtime.test.mjs | 54 +++++++++++ tests/state.test.mjs | 73 +++++++++++++++ 5 files changed, 232 insertions(+), 21 deletions(-) diff --git a/plugins/codex/scripts/lib/claude-session-transfer.mjs b/plugins/codex/scripts/lib/claude-session-transfer.mjs index ae228a80c..ab20d8d57 100644 --- a/plugins/codex/scripts/lib/claude-session-transfer.mjs +++ b/plugins/codex/scripts/lib/claude-session-transfer.mjs @@ -156,15 +156,21 @@ function acquireStagingLease(stagedPath, staged) { } else { managed = markerMatches; } - if (managed) fs.writeFileSync(leasePath, "", { flag: "wx" }); + // The lease is taken whatever `managed` says. A process that attaches to a + // staged copy the creator has not yet marked would otherwise hold no lease, + // and the creator's release() -- seeing no leases -- deletes the file out + // from under it. `managed` only decides whether we may write the marker. + fs.writeFileSync(leasePath, "", { flag: "wx" }); }); return { release() { - if (!managed) return; withStagingLock(stagedPath, () => { try { fs.unlinkSync(leasePath); } catch (error) { if (error?.code !== "ENOENT") throw error; } const activeLeases = fs.readdirSync(directory).filter((name) => name.startsWith(leasePrefix)); + // Cleanup belongs to whoever leaves last, not to whoever created the + // copy: the marker is what proves the staging is the plugin's, and the + // lease count is what proves nobody is still reading it. if (activeLeases.length > 0 || !managedMarkerMatches(markerPath)) return; if (fs.existsSync(stagedPath) && fileSha256(stagedPath) !== staged.sourceSha256) { fs.unlinkSync(markerPath); diff --git a/plugins/codex/scripts/lib/state.mjs b/plugins/codex/scripts/lib/state.mjs index 5c7f2b4e5..1c74e817b 100644 --- a/plugins/codex/scripts/lib/state.mjs +++ b/plugins/codex/scripts/lib/state.mjs @@ -17,6 +17,7 @@ const CODEX_HOME_ENV = "CODEX_HOME"; const CONFIG_DIR_NAME = path.join("plugin-cc", "config"); const FALLBACK_STATE_ROOT_DIR = path.join(os.tmpdir(), "codex-companion"); const STATE_FILE_NAME = "state.json"; +const STATE_LOCK_DIR_NAME = ".state.lock"; const JOBS_DIR_NAME = "jobs"; const MAX_JOBS = 50; const MIN_TERMINAL_JOBS = 10; @@ -149,12 +150,18 @@ export function loadState(cwd) { // an opt-in toward stricter/safer behavior, so any candidate setting it // true wins over a stale false elsewhere -- reconciling by "primary wins" // could silently downgrade an explicitly-enabled gate. + // + // Candidates are folded in reverse (fallback first, primary last) so the + // primary root wins for anything that is not a boolean. The previous + // "first writer wins" rule was dead for every key defaults already define — + // which is all of them — so a non-boolean setting could never be read back + // from any root. const mergedConfig = { ...defaultState().config }; - for (const parsed of parsedCandidates) { + for (const parsed of [...parsedCandidates].reverse()) { for (const [key, value] of Object.entries(parsed.config ?? {})) { if (typeof value === "boolean") { mergedConfig[key] = mergedConfig[key] === true || value === true; - } else if (mergedConfig[key] === undefined) { + } else { mergedConfig[key] = value; } } @@ -202,7 +209,39 @@ function pruneJobs(jobs) { } function resolveStateLockDir(cwd) { - return path.join(resolveStateDir(cwd), ".state.lock"); + return path.join(resolveStateDir(cwd), STATE_LOCK_DIR_NAME); +} + +// Rewriting another root's state.json is a read-modify-write on a file whose +// own lock is not the one saveState() holds: a process whose primary IS that +// root can be mid-update, and the rename would drop everything it just wrote. +// So take that root's lock too -- but never wait for it. Two processes holding +// each other's primary lock would deadlock, and this prune is not urgent: the +// pruned ids stay pruned in this root, and the next save re-runs it. +function pruneOtherStateRoot(otherStateDir, retainedIds) { + const otherStateFile = path.join(otherStateDir, STATE_FILE_NAME); + if (!fs.existsSync(otherStateFile)) { + return; + } + try { + withLockSync( + path.join(otherStateDir, STATE_LOCK_DIR_NAME), + () => { + // Re-read under the lock: the copy this decision was made from could + // have been replaced while the lock was being taken. + const otherParsed = readStateFileIfValid(otherStateFile); + const otherJobs = Array.isArray(otherParsed?.jobs) ? otherParsed.jobs : []; + const prunedOtherJobs = otherJobs.filter((job) => retainedIds.has(job.id)); + if (prunedOtherJobs.length === otherJobs.length) { + return; + } + writeJsonFileAtomic(otherStateFile, { ...otherParsed, jobs: prunedOtherJobs }); + }, + { timeoutMs: 0 } + ); + } catch { + // Busy or unlockable: leave that root alone rather than racing its owner. + } } function saveStateLocked(cwd, state) { @@ -243,14 +282,7 @@ function saveStateLocked(cwd, state) { // root, above. This only ever removes. const [, ...otherStateDirs] = resolveStateDirCandidates(cwd); for (const otherStateDir of otherStateDirs) { - const otherStateFile = path.join(otherStateDir, STATE_FILE_NAME); - const otherParsed = readStateFileIfValid(otherStateFile); - const otherJobs = Array.isArray(otherParsed?.jobs) ? otherParsed.jobs : []; - const prunedOtherJobs = otherJobs.filter((job) => retainedIds.has(job.id)); - if (prunedOtherJobs.length === otherJobs.length) { - continue; - } - writeJsonFileAtomic(otherStateFile, { ...otherParsed, jobs: prunedOtherJobs }); + pruneOtherStateRoot(otherStateDir, retainedIds); } return nextState; @@ -323,11 +355,49 @@ function writeDurableConfig(cwd, config) { return nextConfig; } +// Same lock discipline as pruneOtherStateRoot(): that root's own lock, never +// waited on. A config copy this misses is corrected by the next write, and by +// the durable config, which is the authority. +function syncOtherStateRootConfig(otherStateDir, config) { + const otherStateFile = path.join(otherStateDir, STATE_FILE_NAME); + if (!fs.existsSync(otherStateFile)) { + return; + } + try { + withLockSync( + path.join(otherStateDir, STATE_LOCK_DIR_NAME), + () => { + const otherParsed = readStateFileIfValid(otherStateFile); + if (!otherParsed) { + return; + } + writeJsonFileAtomic(otherStateFile, { + ...otherParsed, + config: { ...(otherParsed.config ?? {}), ...config } + }); + }, + { timeoutMs: 0 } + ); + } catch { + // Busy or unlockable: leave that root alone rather than racing its owner. + } +} + export function setConfig(cwd, key, value) { const nextConfig = writeDurableConfig(cwd, { ...getConfig(cwd), [key]: value }); updateState(cwd, (state) => { state.config = { ...state.config, ...nextConfig }; }); + // The cached copy in every other root has to follow. loadState() merges + // booleans with OR — deliberately, so a gate enabled under one root is not + // downgraded by a stale false under another — which also means a stranded + // true would outvote this write forever whenever the durable config cannot + // be read. Writing the new value everywhere keeps that safety direction + // without making "disable" unreachable. + const [, ...otherStateDirs] = resolveStateDirCandidates(cwd); + for (const otherStateDir of otherStateDirs) { + syncOtherStateRootConfig(otherStateDir, nextConfig); + } return nextConfig; } diff --git a/plugins/codex/scripts/session-lifecycle-hook.mjs b/plugins/codex/scripts/session-lifecycle-hook.mjs index 78b40e865..7ac98f1aa 100644 --- a/plugins/codex/scripts/session-lifecycle-hook.mjs +++ b/plugins/codex/scripts/session-lifecycle-hook.mjs @@ -84,19 +84,27 @@ function setEnv(name, value) { const prefix = `export ${name}=`; const line = `${prefix}${shellEscape(value)}`; - let content = ""; + // CLAUDE_ENV_FILE is shared with every other plugin's SessionStart hook and is + // append-only by convention. Rewriting it (read, filter, rename) drops any + // export another hook appended between the read and the rename, and the + // rename replaces the file, discarding its mode along with it. So append — + // and skip the append when the value the file already resolves to is ours. + // The shell takes the last export for a key, so this keeps the file from + // growing on every session without ever removing a line somebody else wrote. try { - content = fs.readFileSync(envFile, "utf8"); + const existing = fs + .readFileSync(envFile, "utf8") + .split(/\r?\n/) + .filter((entry) => entry.startsWith(prefix)) + .at(-1); + if (existing === line) { + return; + } } catch (err) { if (err.code !== "ENOENT") throw err; } - const lines = content.split(/\r?\n/).filter((l) => l && !l.startsWith(prefix)); - lines.push(line); - - const tmp = `${envFile}.${process.pid}.tmp`; - fs.writeFileSync(tmp, lines.join("\n") + "\n", "utf8"); - fs.renameSync(tmp, envFile); + fs.appendFileSync(envFile, `${line}\n`, "utf8"); } // A pid-less active record has no liveness signal at all (current code diff --git a/tests/runtime.test.mjs b/tests/runtime.test.mjs index b410ae315..0fd3e836f 100644 --- a/tests/runtime.test.mjs +++ b/tests/runtime.test.mjs @@ -5694,3 +5694,57 @@ test("a retained orphaned turn stays reconcilable after session end", () => { assert.equal(retained.threadId, "thr_pending"); assert.equal(retained.phase, "worker-exited-turn-unknown"); }); + + +test("the session start hook appends to CLAUDE_ENV_FILE without rewriting it", () => { + const repo = makeTempDir(); + const envFile = path.join(makeTempDir(), "claude-env.sh"); + const foreignExport = "export SOME_OTHER_PLUGIN_VAR='kept'\n"; + fs.writeFileSync(envFile, foreignExport, "utf8"); + fs.chmodSync(envFile, 0o600); + const pluginDataDir = makeTempDir(); + const transcriptPath = path.join(repo, "session.jsonl"); + const env = { + ...process.env, + CLAUDE_ENV_FILE: envFile, + CLAUDE_PLUGIN_DATA: pluginDataDir + }; + const input = JSON.stringify({ + hook_event_name: "SessionStart", + session_id: "sess-current", + transcript_path: transcriptPath, + cwd: repo + }); + + assert.equal(run("node", [SESSION_HOOK, "SessionStart"], { cwd: repo, env, input }).status, 0); + assert.equal(run("node", [SESSION_HOOK, "SessionStart"], { cwd: repo, env, input }).status, 0); + + const contents = fs.readFileSync(envFile, "utf8"); + // The file is shared with every other plugin's SessionStart hook: a rewrite + // would drop whatever another hook appended, and replacing the file discards + // its mode with it. + assert.match(contents, /export SOME_OTHER_PLUGIN_VAR='kept'/); + assert.equal(fs.statSync(envFile).mode & 0o777, 0o600); + // Re-exporting the same value must not grow the file either. + assert.equal(contents.split("\n").filter((line) => line.startsWith("export CODEX_COMPANION_SESSION_ID=")).length, 1); + + // A changed value is appended; the shell takes the last export for a key. + assert.equal( + run("node", [SESSION_HOOK, "SessionStart"], { + cwd: repo, + env, + input: JSON.stringify({ + hook_event_name: "SessionStart", + session_id: "sess-next", + transcript_path: transcriptPath, + cwd: repo + }) + }).status, + 0 + ); + const sessionExports = fs + .readFileSync(envFile, "utf8") + .split("\n") + .filter((line) => line.startsWith("export CODEX_COMPANION_SESSION_ID=")); + assert.equal(sessionExports.at(-1), "export CODEX_COMPANION_SESSION_ID='sess-next'"); +}); diff --git a/tests/state.test.mjs b/tests/state.test.mjs index f94fc84ed..0097b069a 100644 --- a/tests/state.test.mjs +++ b/tests/state.test.mjs @@ -9,6 +9,7 @@ import { fileURLToPath, pathToFileURL } from "node:url"; import { makeTempDir } from "./helpers.mjs"; import { readStoredJob } from "../plugins/codex/scripts/lib/job-control.mjs"; +import { acquireLockSync, releaseLock } from "../plugins/codex/scripts/lib/locking.mjs"; import { getConfig, loadState, @@ -647,3 +648,75 @@ test("a durable config write that fails mid-write leaves the previous config int } }); + +function withRoots(fn) { + const previousPluginData = process.env.CLAUDE_PLUGIN_DATA; + const previousCodexHome = process.env.CODEX_HOME; + const workspace = makeTempDir(); + const pluginData = makeTempDir(); + const codexHome = makeTempDir(); + process.env.CLAUDE_PLUGIN_DATA = pluginData; + process.env.CODEX_HOME = codexHome; + const primaryDir = resolveStateDir(workspace); + // The second candidate is the tmpdir fallback a CLI invocation without + // CLAUDE_PLUGIN_DATA resolves to. + const fallbackDir = path.join(os.tmpdir(), "codex-companion", path.basename(primaryDir)); + fs.mkdirSync(fallbackDir, { recursive: true }); + try { + return fn({ workspace, primaryDir, fallbackDir, codexHome }); + } finally { + fs.rmSync(fallbackDir, { recursive: true, force: true }); + if (previousPluginData == null) delete process.env.CLAUDE_PLUGIN_DATA; + else process.env.CLAUDE_PLUGIN_DATA = previousPluginData; + if (previousCodexHome == null) delete process.env.CODEX_HOME; + else process.env.CODEX_HOME = previousCodexHome; + } +} + +test("pruning another state root waits for nobody and clobbers nobody", () => { + withRoots(({ workspace, fallbackDir }) => { + const fallbackState = path.join(fallbackDir, "state.json"); + const foreignJob = { id: "task-foreign", status: "completed", updatedAt: "2026-01-01T00:00:00.000Z" }; + fs.writeFileSync( + fallbackState, + `${JSON.stringify({ version: 1, config: { stopReviewGate: false }, jobs: [foreignJob] }, null, 2)}\n`, + "utf8" + ); + + // A process whose primary IS that root, mid-update. + const held = acquireLockSync(path.join(fallbackDir, ".state.lock")); + try { + saveState(workspace, { version: 1, config: { stopReviewGate: false }, jobs: [] }); + // Rewriting it here would have thrown away whatever the lock holder is + // about to write. + const untouched = JSON.parse(fs.readFileSync(fallbackState, "utf8")); + assert.deepEqual(untouched.jobs, [foreignJob]); + } finally { + releaseLock(held); + } + + // Once nobody holds it, the same prune goes through. + saveState(workspace, { version: 1, config: { stopReviewGate: false }, jobs: [] }); + assert.deepEqual(JSON.parse(fs.readFileSync(fallbackState, "utf8")).jobs, []); + }); +}); + +test("disabling the review gate is not outvoted by a stranded enable in another root", () => { + withRoots(({ workspace, fallbackDir, codexHome }) => { + setConfig(workspace, "stopReviewGate", true); + // A root that was written while CLAUDE_PLUGIN_DATA was unset. + fs.writeFileSync( + path.join(fallbackDir, "state.json"), + `${JSON.stringify({ version: 1, config: { stopReviewGate: true }, jobs: [] }, null, 2)}\n`, + "utf8" + ); + + setConfig(workspace, "stopReviewGate", false); + + // With the durable config gone, getConfig() falls back to merging the + // roots, where booleans are ORed. A stranded true would outvote this + // disable forever unless the write reached that root too. + fs.rmSync(path.join(codexHome, "plugin-cc"), { recursive: true, force: true }); + assert.equal(getConfig(workspace).stopReviewGate, false); + }); +}); From af0cdd737fe96b1d7dcc5e1d38fc7004d0d10af1 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 17 Sep 2026 15:38:59 +0000 Subject: [PATCH 26/30] fix: six defects the second review found, four of them in the first round of fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - The retained orphan kept its dead pid, which made it reapable: another session's SessionEnd failed the record without an interrupt and it stopped pinning the broker, tearing the runtime down under the turn the retain protects. The verdict is persisted instead — pid: null plus workerExited — and reconcileJobLiveness() reads that flag, so the record stays truthful in /codex:status while a pid-less active record keeps the broker up under the existing staleness bound. The test now also ends a second session and asserts the job survives it. - Skipping assertResumedSandbox() for scoped resumes also dropped the escalation check. A dedicated one replaces it: a thread running with the sandbox disabled cannot be scoped by a permission profile, so --read-root on it is refused rather than silently promising a scope. - saveState() deleted a dropped job's files from every root while the prune of another root's state.json is best-effort, so a contended prune left a record to be merged back — and rewritten into the primary — with its detail file, claim and log already gone. Each root's files now go with its own record. - setEnv() appended without ensuring the file ends in a newline, so a preceding hook's unterminated line and ours would run together and lose both exports. - The append's own comment (and the README's line for #748) claimed the file no longer grows per session, which is false for values that change every session — the session id and transcript path. Both now say what actually holds: unchanged values are skipped, changed ones append, and the shell takes the last export. Bounded growth is the price of never destroying another plugin's export. - release() took the 5s staging lock even when there was nothing to clean up, from a finally, so a busy lock replaced the import error that was unwinding. It is wrapped now, with a lock-free unlink of our own lease as the fallback. Verified: full npm test 310/310; tsc clean. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UXfvnjSC72HsM6EEPVt2Tg --- README.md | 2 +- .../scripts/lib/claude-session-transfer.mjs | 22 ++++++-- plugins/codex/scripts/lib/codex.mjs | 24 +++++++-- plugins/codex/scripts/lib/job-control.mjs | 12 ++++- plugins/codex/scripts/lib/state.mjs | 30 ++++++++--- .../codex/scripts/session-lifecycle-hook.mjs | 50 ++++++++++++------- tests/runtime.test.mjs | 19 +++++-- 7 files changed, 121 insertions(+), 38 deletions(-) diff --git a/README.md b/README.md index 8e6fd358c..6bc78ee76 100644 --- a/README.md +++ b/README.md @@ -378,7 +378,7 @@ Commands and flags: | [#729](https://github.com/openai/codex-plugin-cc/pull/729) | `/codex:transfer` works with a relocated `CLAUDE_CONFIG_DIR` | | [#742](https://github.com/openai/codex-plugin-cc/pull/742) | `--sandbox ` on `task` and `/codex:rescue` | | [#746](https://github.com/openai/codex-plugin-cc/pull/746) | `--model`/`--effort` on the review commands, and a warning for unrecognised options | -| [#748](https://github.com/openai/codex-plugin-cc/pull/748) | `CLAUDE_ENV_FILE` keeps one export per key instead of growing on every session | +| [#748](https://github.com/openai/codex-plugin-cc/pull/748) | `CLAUDE_ENV_FILE` skips re-exporting an unchanged value (its rewrite-the-file mechanism is not used: the file is shared with other plugins' hooks, so this fork only ever appends to it) | | [#731](https://github.com/openai/codex-plugin-cc/pull/731) | the review-gate flag is persisted outside the transient state dir, so a different `CLAUDE_PLUGIN_DATA` no longer silently disables it | | [#737](https://github.com/openai/codex-plugin-cc/pull/737) | hooks resolve Node through `scripts/run-node.sh`, so nvm/fnm/asdf/mise/Volta/Homebrew toolchains work under the minimal hook PATH | | [#747](https://github.com/openai/codex-plugin-cc/pull/747) | `runCommand` sets an explicit 256 MiB `maxBuffer`, so a large `git diff` is no longer truncated at Node's 1 MiB default | diff --git a/plugins/codex/scripts/lib/claude-session-transfer.mjs b/plugins/codex/scripts/lib/claude-session-transfer.mjs index ab20d8d57..029673eec 100644 --- a/plugins/codex/scripts/lib/claude-session-transfer.mjs +++ b/plugins/codex/scripts/lib/claude-session-transfer.mjs @@ -165,7 +165,20 @@ function acquireStagingLease(stagedPath, staged) { return { release() { - withStagingLock(stagedPath, () => { + // Called from a finally: a staging lock that is busy (5s) or unusable + // must never surface in place of the import error that is unwinding. + // Dropping our own lease needs no lock — the path is unique to us — so + // the fallback still frees the staged copy for whoever leaves last. + try { + releaseLocked(); + } catch { + try { fs.unlinkSync(leasePath); } catch (error) { if (error?.code !== "ENOENT") { /* nothing left to do */ } } + } + } + }; + + function releaseLocked() { + withStagingLock(stagedPath, () => { try { fs.unlinkSync(leasePath); } catch (error) { if (error?.code !== "ENOENT") throw error; } const activeLeases = fs.readdirSync(directory).filter((name) => name.startsWith(leasePrefix)); // Cleanup belongs to whoever leaves last, not to whoever created the @@ -177,10 +190,9 @@ function acquireStagingLease(stagedPath, staged) { return; } try { fs.unlinkSync(stagedPath); } catch (error) { if (error?.code !== "ENOENT") throw error; } - try { fs.unlinkSync(markerPath); } catch (error) { if (error?.code !== "ENOENT") throw error; } - }); - } - }; + try { fs.unlinkSync(markerPath); } catch (error) { if (error?.code !== "ENOENT") throw error; } + }); + } } function isWithin(root, candidate) { diff --git a/plugins/codex/scripts/lib/codex.mjs b/plugins/codex/scripts/lib/codex.mjs index 52802c5f3..2b64e3b11 100644 --- a/plugins/codex/scripts/lib/codex.mjs +++ b/plugins/codex/scripts/lib/codex.mjs @@ -120,6 +120,21 @@ function sandboxModeForPolicy(policy) { return null; } +// A scoped run constrains reads through a permission profile, which cannot +// take back what a thread started with `danger-full-access` already has: the +// sandbox is off for that thread, so the scope would be a promise this plugin +// cannot keep. +function assertScopedResumeNotEscalated(threadId, response) { + const effectiveMode = sandboxModeForPolicy(response?.sandbox); + if (effectiveMode !== "danger-full-access") { + return; + } + throw new Error( + `Thread ${threadId} runs with the Codex sandbox disabled (danger-full-access), so --read-root cannot scope it. ` + + "Start a fresh thread with --fresh to run scoped." + ); +} + function assertResumedSandbox(threadId, requestedMode, response) { if (!requestedMode || !SANDBOX_POLICY_TYPES.has(requestedMode)) { return; @@ -1335,13 +1350,16 @@ export async function runAppServerTurn(cwd, options = {}) { write: options.write, ephemeral: false }); - // Only meaningful when the resume actually asked for a sandbox mode. // With read roots, buildThreadAccessParams() deliberately sends a // scoped permission profile and no `sandbox`, so the app-server's - // reported mode is not the one this turn requested: asserting it here + // reported mode is not the one this turn requested: asserting it // refused every `--read-root` resume, and the error's own advice // (resume with the reported mode) silently dropped the write grant. - if (!(options.readRoots?.length > 0)) { + // The escalation half of that check still applies, though — a thread + // started with the sandbox disabled is not scoped by any profile. + if (options.readRoots?.length > 0) { + assertScopedResumeNotEscalated(options.resumeThreadId, response); + } else { assertResumedSandbox(options.resumeThreadId, options.sandbox, response); } threadId = response.thread.id; diff --git a/plugins/codex/scripts/lib/job-control.mjs b/plugins/codex/scripts/lib/job-control.mjs index c34d56c8e..68dd0150f 100644 --- a/plugins/codex/scripts/lib/job-control.mjs +++ b/plugins/codex/scripts/lib/job-control.mjs @@ -29,7 +29,17 @@ function isActiveJob(job) { } export function reconcileJobLiveness(job, options = {}) { - if (!isActiveJob(job) || !Number.isSafeInteger(job.pid) || job.pid <= 0) { + if (!isActiveJob(job)) { + return job; + } + // A record whose worker was already found gone carries the verdict itself: + // the pid is dropped when that is persisted (a dead pid would let the + // dead-worker reaper fail the job and stop it pinning the broker, under a + // turn that may still be running), so there is nothing left to probe. + if (job.workerExited === true && job.threadId) { + return { ...job, status: "running", phase: "worker-exited-turn-unknown", pid: null, workerExited: true }; + } + if (!Number.isSafeInteger(job.pid) || job.pid <= 0) { return job; } diff --git a/plugins/codex/scripts/lib/state.mjs b/plugins/codex/scripts/lib/state.mjs index 1c74e817b..ad844b6d0 100644 --- a/plugins/codex/scripts/lib/state.mjs +++ b/plugins/codex/scripts/lib/state.mjs @@ -218,6 +218,17 @@ function resolveStateLockDir(cwd) { // So take that root's lock too -- but never wait for it. Two processes holding // each other's primary lock would deadlock, and this prune is not urgent: the // pruned ids stay pruned in this root, and the next save re-runs it. +// Everything a single root holds for one job: its detail file, its terminal +// claim, and its log when the log lives in that root. +function removeJobArtifacts(stateDir, job) { + const jobsDir = path.join(stateDir, JOBS_DIR_NAME); + removeFileIfExists(path.join(jobsDir, `${job.id}.json`)); + removeFileIfExists(path.join(jobsDir, `${job.id}.terminal`)); + if (typeof job.logFile === "string" && job.logFile.startsWith(`${stateDir}${path.sep}`)) { + removeFileIfExists(job.logFile); + } +} + function pruneOtherStateRoot(otherStateDir, retainedIds) { const otherStateFile = path.join(otherStateDir, STATE_FILE_NAME); if (!fs.existsSync(otherStateFile)) { @@ -236,6 +247,11 @@ function pruneOtherStateRoot(otherStateDir, retainedIds) { return; } writeJsonFileAtomic(otherStateFile, { ...otherParsed, jobs: prunedOtherJobs }); + for (const job of otherJobs) { + if (!retainedIds.has(job.id)) { + removeJobArtifacts(otherStateDir, job); + } + } }, { timeoutMs: 0 } ); @@ -257,17 +273,17 @@ function saveStateLocked(cwd, state) { }; const retainedIds = new Set(nextJobs.map((job) => job.id)); + const primaryStateDir = resolveStateDir(cwd); for (const job of previousJobs) { if (retainedIds.has(job.id)) { continue; } - for (const jobFile of resolveJobFileCandidates(cwd, job.id)) { - removeFileIfExists(jobFile); - } - for (const claimFile of resolveJobClaimFileCandidates(cwd, job.id)) { - removeFileIfExists(claimFile); - } - removeFileIfExists(job.logFile); + // Only this root's copies. Pruning another root's state.json is + // best-effort (it needs that root's lock), so deleting its files here + // would leave a record that is merged back in — and rewritten into the + // primary — with its detail file, claim and log already gone. Each root's + // files go when its own record does. + removeJobArtifacts(primaryStateDir, job); } writeJsonFileAtomic(resolveStateFile(cwd), nextState); diff --git a/plugins/codex/scripts/session-lifecycle-hook.mjs b/plugins/codex/scripts/session-lifecycle-hook.mjs index 7ac98f1aa..fcee034c0 100644 --- a/plugins/codex/scripts/session-lifecycle-hook.mjs +++ b/plugins/codex/scripts/session-lifecycle-hook.mjs @@ -87,24 +87,32 @@ function setEnv(name, value) { // CLAUDE_ENV_FILE is shared with every other plugin's SessionStart hook and is // append-only by convention. Rewriting it (read, filter, rename) drops any // export another hook appended between the read and the rename, and the - // rename replaces the file, discarding its mode along with it. So append — - // and skip the append when the value the file already resolves to is ours. - // The shell takes the last export for a key, so this keeps the file from - // growing on every session without ever removing a line somebody else wrote. + // rename replaces the file, discarding its mode along with it. So append. + // + // That means a value that changes every session (the session id, the + // transcript path) adds a line every session: the shell takes the last + // export for a key, so the file stays correct while it grows. Only an + // unchanged value is skipped. Bounded growth is the price of never + // destroying another plugin's export. + let content = ""; try { - const existing = fs - .readFileSync(envFile, "utf8") - .split(/\r?\n/) - .filter((entry) => entry.startsWith(prefix)) - .at(-1); - if (existing === line) { - return; - } + content = fs.readFileSync(envFile, "utf8"); } catch (err) { if (err.code !== "ENOENT") throw err; } - fs.appendFileSync(envFile, `${line}\n`, "utf8"); + const existing = content + .split(/\r?\n/) + .filter((entry) => entry.startsWith(prefix)) + .at(-1); + if (existing === line) { + return; + } + + // A hook that appended without a trailing newline would otherwise have its + // line and ours run together, losing both exports. + const separator = content === "" || content.endsWith("\n") ? "" : "\n"; + fs.appendFileSync(envFile, `${separator}${line}\n`, "utf8"); } // A pid-less active record has no liveness signal at all (current code @@ -302,10 +310,18 @@ async function cleanupSessionJobs(cwd, sessionId, { interruptTurns = false, inte // turn id, so the Codex turn may still be running and there is nothing to // interrupt it by. Cancelling the record here would claim an outcome that // did not happen; leave it active (its phase says why) for the next - // status query. The pid is written back rather than nulled: it is what - // lets that query reconcile the job again instead of trusting a stale - // "running". - upsertJob(workspaceRoot, { id: job.id, phase: reconciled.phase, pid: workerPid, threadId }); + // status query. The verdict is persisted instead of the pid: writing a + // dead pid back would let another session's dead-worker reaper fail the + // record and stop it pinning the broker — tearing the runtime down under + // the very turn this retain protects. A pid-less active record keeps the + // broker up (bounded by the staleness rule) and reconciles from the flag. + upsertJob(workspaceRoot, { + id: job.id, + phase: reconciled.phase, + pid: null, + threadId, + workerExited: true + }); continue; } jobsAwaitingInterrupt -= 1; diff --git a/tests/runtime.test.mjs b/tests/runtime.test.mjs index 0fd3e836f..419ab8f61 100644 --- a/tests/runtime.test.mjs +++ b/tests/runtime.test.mjs @@ -5687,12 +5687,23 @@ test("a retained orphaned turn stays reconcilable after session end", () => { const retained = JSON.parse(fs.readFileSync(path.join(stateDir, "state.json"), "utf8")).jobs[0]; assert.equal(retained.status, "running"); - // Written back rather than nulled: reconcileJobLiveness() needs a pid, so a - // record retained without one can never be judged again and stays "running" - // forever in /codex:status. - assert.equal(retained.pid, 999999); assert.equal(retained.threadId, "thr_pending"); assert.equal(retained.phase, "worker-exited-turn-unknown"); + // The verdict is persisted, not the dead pid: the record has to stay + // reconcilable without handing the dead-worker reaper something to fail. + assert.equal(retained.pid, null); + assert.equal(retained.workerExited, true); + + // Another session ending must not reap it: its turn may still be running, + // and failing the record would also stop it pinning the shared broker. + const other = run("node", [SESSION_HOOK, "SessionEnd"], { + cwd: workspace, + env: { ...process.env, CODEX_COMPANION_SESSION_ID: "sess-other" }, + input: JSON.stringify({ hook_event_name: "SessionEnd", session_id: "sess-other", cwd: workspace }) + }); + assert.equal(other.status, 0, other.stderr); + const afterOther = JSON.parse(fs.readFileSync(path.join(stateDir, "state.json"), "utf8")).jobs[0]; + assert.equal(afterOther.status, "running"); }); From 0c5ddd87c9a8132d22ded46fe7611f09f7012151 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 17 Sep 2026 15:56:56 +0000 Subject: [PATCH 27/30] fix: close the five findings of the third review round MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - A retained orphan belongs to the session that is ending, so hasActiveJobsFromOtherSessions() does not speak for it and the same hook run went on to shut the broker down — killing the turn the retain protects. cleanupSessionJobs() now reports what it retained and SessionEnd leaves the runtime up for it. - With no pid, that record's only exit was the day-long staleness rule, so it would block every session's broker shutdown for a day. Its protection is bounded to the broker's own idle window instead (CODEX_BROKER_IDLE_SHUTDOWN_MS, 10 minutes by default): the turn cannot outlive the broker anyway, since its client is gone and the broker idles out on that same timer. The reaper and the broker guard both use the new bound. - The other root's prune deleted a job's files on a retainedIds set computed before the lock, so a job created in that root since the snapshot could lose its detail file, claim and log while its worker ran. Both the record prune and the deletion are now limited to ids the caller's own snapshot held; anything newer is nobody's to drop here. - The scoped-resume escalation check now also runs on a fresh scoped start: a default config can start a thread with the sandbox disabled, which would have made the resume error's own "--fresh" remedy reproduce the refused condition. - setEnv() always opens its append with a newline rather than deciding from the read above it, which still raced a hook appending an unterminated line in between. The test for the file's contents asserts the exports and their order, since blank lines are nothing to the shell that sources it. Verified: full npm test 310/310; tsc clean. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UXfvnjSC72HsM6EEPVt2Tg --- plugins/codex/scripts/lib/codex.mjs | 6 ++ plugins/codex/scripts/lib/state.mjs | 13 ++-- .../codex/scripts/session-lifecycle-hook.mjs | 63 ++++++++++++++++--- tests/runtime.test.mjs | 14 ++++- 4 files changed, 81 insertions(+), 15 deletions(-) diff --git a/plugins/codex/scripts/lib/codex.mjs b/plugins/codex/scripts/lib/codex.mjs index 2b64e3b11..85d2ef5e2 100644 --- a/plugins/codex/scripts/lib/codex.mjs +++ b/plugins/codex/scripts/lib/codex.mjs @@ -1373,6 +1373,12 @@ export async function runAppServerTurn(cwd, options = {}) { ephemeral: options.persistThread ? false : true, threadName: options.persistThread ? options.threadName : options.threadName ?? null }); + // A default config can start a thread with the sandbox disabled, in + // which case the scope this run asked for would not hold here either — + // and "start a fresh thread" is the advice the resume path gives. + if (options.readRoots?.length > 0) { + assertScopedResumeNotEscalated(response.thread.id, response); + } threadId = response.thread.id; } } catch (error) { diff --git a/plugins/codex/scripts/lib/state.mjs b/plugins/codex/scripts/lib/state.mjs index ad844b6d0..d4d33e353 100644 --- a/plugins/codex/scripts/lib/state.mjs +++ b/plugins/codex/scripts/lib/state.mjs @@ -229,7 +229,7 @@ function removeJobArtifacts(stateDir, job) { } } -function pruneOtherStateRoot(otherStateDir, retainedIds) { +function pruneOtherStateRoot(otherStateDir, retainedIds, knownIds) { const otherStateFile = path.join(otherStateDir, STATE_FILE_NAME); if (!fs.existsSync(otherStateFile)) { return; @@ -242,13 +242,14 @@ function pruneOtherStateRoot(otherStateDir, retainedIds) { // have been replaced while the lock was being taken. const otherParsed = readStateFileIfValid(otherStateFile); const otherJobs = Array.isArray(otherParsed?.jobs) ? otherParsed.jobs : []; - const prunedOtherJobs = otherJobs.filter((job) => retainedIds.has(job.id)); + const dropped = (job) => !retainedIds.has(job.id) && knownIds.has(job.id); + const prunedOtherJobs = otherJobs.filter((job) => !dropped(job)); if (prunedOtherJobs.length === otherJobs.length) { return; } writeJsonFileAtomic(otherStateFile, { ...otherParsed, jobs: prunedOtherJobs }); for (const job of otherJobs) { - if (!retainedIds.has(job.id)) { + if (dropped(job)) { removeJobArtifacts(otherStateDir, job); } } @@ -296,9 +297,13 @@ function saveStateLocked(cwd, state) { // only in a non-primary root. Prune every other candidate root down to the same // retained set; new and updated jobs are still only ever written to the primary // root, above. This only ever removes. + // Ids the caller actually decided about: whatever the merged snapshot at the + // top of this function held. Anything else in another root arrived after it + // and is nobody's to drop here. + const knownIds = new Set(previousJobs.map((job) => job.id)); const [, ...otherStateDirs] = resolveStateDirCandidates(cwd); for (const otherStateDir of otherStateDirs) { - pruneOtherStateRoot(otherStateDir, retainedIds); + pruneOtherStateRoot(otherStateDir, retainedIds, knownIds); } return nextState; diff --git a/plugins/codex/scripts/session-lifecycle-hook.mjs b/plugins/codex/scripts/session-lifecycle-hook.mjs index fcee034c0..a33993d7d 100644 --- a/plugins/codex/scripts/session-lifecycle-hook.mjs +++ b/plugins/codex/scripts/session-lifecycle-hook.mjs @@ -5,6 +5,7 @@ import process from "node:process"; import { isPidAlive, terminateProcessTree } from "./lib/process.mjs"; import { reconcileJobLiveness } from "./lib/job-control.mjs"; +import { brokerIdleShutdownMs } from "./lib/lifecycle-limits.mjs"; import { BROKER_ENDPOINT_ENV } from "./lib/app-server.mjs"; import { LOG_FILE_ENV, @@ -109,10 +110,11 @@ function setEnv(name, value) { return; } - // A hook that appended without a trailing newline would otherwise have its - // line and ours run together, losing both exports. - const separator = content === "" || content.endsWith("\n") ? "" : "\n"; - fs.appendFileSync(envFile, `${separator}${line}\n`, "utf8"); + // Always open with a newline rather than deciding from the read above: a + // hook appending an unterminated line between that read and this write would + // otherwise run into ours, losing both exports. A blank line costs nothing to + // the shell that sources this file. + fs.appendFileSync(envFile, `\n${line}\n`, "utf8"); } // A pid-less active record has no liveness signal at all (current code @@ -131,6 +133,27 @@ function isStaleJobRecord(job) { return Date.now() - timestamp > ACTIVE_JOB_STALENESS_MS; } +// A record retained because its worker died with a turn still possibly running +// has no pid to probe, so the generic pid-less rule (a day) would keep it +// "running" — and keep it pinning the broker — for a day. The turn it protects +// cannot outlive the broker anyway: its client is gone, so the broker idles out +// on its own timer. Bound the protection to that same window. +function retainedOrphanExpired(job, env = process.env) { + if (job?.workerExited !== true) { + return false; + } + const reference = job.updatedAt ?? job.createdAt ?? null; + const timestamp = reference ? Date.parse(reference) : Number.NaN; + if (!Number.isFinite(timestamp)) { + return false; + } + const windowMs = brokerIdleShutdownMs(env); + if (!Number.isFinite(windowMs) || windowMs <= 0) { + return false; + } + return Date.now() - timestamp > windowMs; +} + // 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 @@ -164,7 +187,12 @@ async function reapDeadWorkerJobs(workspaceRoot, { excludeSessionId = null, cwd continue; } jobsAwaitingInterrupt -= 1; - const workerDead = job.pid != null ? !isPidAlive(job.pid) : isStaleJobRecord(job); + const workerDead = + job.pid != null + ? !isPidAlive(job.pid) + : job.workerExited === true + ? retainedOrphanExpired(job) + : isStaleJobRecord(job); if (!workerDead) { continue; } @@ -239,13 +267,16 @@ function hasActiveJobsFromOtherSessions(workspaceRoot, sessionId) { if (job.pid != null) { return isPidAlive(job.pid); } + if (job.workerExited === true) { + return !retainedOrphanExpired(job); + } return !isStaleJobRecord(job); }); } async function cleanupSessionJobs(cwd, sessionId, { interruptTurns = false, interruptDeadline = null } = {}) { if (!cwd || !sessionId) { - return; + return { retainedOrphans: 0 }; } const workspaceRoot = resolveWorkspaceRoot(cwd); @@ -254,7 +285,7 @@ async function cleanupSessionJobs(cwd, sessionId, { interruptTurns = false, inte // alone would miss a session whose jobs only live in the fallback root. const sessionJobs = loadState(workspaceRoot).jobs.filter((job) => job.sessionId === sessionId); if (sessionJobs.length === 0) { - return; + return { retainedOrphans: 0 }; } const completedAt = new Date().toISOString(); @@ -266,6 +297,10 @@ async function cleanupSessionJobs(cwd, sessionId, { interruptTurns = false, inte errorMessage: "Cancelled: the Claude session ended while the job was still running." }; const cancelledIds = new Set(); + // Jobs left active because their turn may still be running: the broker must + // outlive this hook run for them, and hasActiveJobsFromOtherSessions() will + // not speak for them — they belong to the session that is ending. + let retainedOrphans = 0; const killedPids = []; const finishingJobs = []; interruptDeadline = interruptDeadline ?? Date.now() + TURN_INTERRUPT_BUDGET_MS; @@ -322,6 +357,7 @@ async function cleanupSessionJobs(cwd, sessionId, { interruptTurns = false, inte threadId, workerExited: true }); + retainedOrphans += 1; continue; } jobsAwaitingInterrupt -= 1; @@ -472,6 +508,8 @@ async function cleanupSessionJobs(cwd, sessionId, { interruptTurns = false, inte return isActiveJob(job); }); }); + + return { retainedOrphans }; } function handleSessionStart(input) { @@ -504,7 +542,16 @@ async function handleSessionEnd(input) { interruptTurns, interruptDeadline }); - await cleanupSessionJobs(cwd, sessionId, { interruptTurns, interruptDeadline }); + const cleanup = await cleanupSessionJobs(cwd, sessionId, { interruptTurns, interruptDeadline }); + + // A turn this session could not interrupt — its worker died before publishing + // a turn id — may still be running on this broker. The guard below speaks + // only for other sessions' work, so without this the same hook run would + // tear the runtime down under the turn the retain exists to protect. The + // broker idles out on its own timer, which is what bounds the wait. + if (cleanup?.retainedOrphans > 0) { + return; + } // 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 diff --git a/tests/runtime.test.mjs b/tests/runtime.test.mjs index 419ab8f61..6a99f6892 100644 --- a/tests/runtime.test.mjs +++ b/tests/runtime.test.mjs @@ -889,9 +889,17 @@ test("session start hook exports the Claude session id, transcript path, and plu }); assert.equal(result.status, 0, result.stderr); - assert.equal( - fs.readFileSync(envFile, "utf8"), - `export CODEX_COMPANION_SESSION_ID='sess-current'\nexport CODEX_COMPANION_TRANSCRIPT_PATH='${transcriptPath}'\nexport CLAUDE_PLUGIN_DATA='${pluginDataDir}'\n` + // Each export opens with its own newline, so a line another plugin's hook + // appended without one cannot run into ours. Blank lines are nothing to the + // shell that sources this file, so the contract is the exports and their + // order, not byte-for-byte content. + assert.deepEqual( + fs.readFileSync(envFile, "utf8").split("\n").filter((line) => line !== ""), + [ + "export CODEX_COMPANION_SESSION_ID='sess-current'", + `export CODEX_COMPANION_TRANSCRIPT_PATH='${transcriptPath}'`, + `export CLAUDE_PLUGIN_DATA='${pluginDataDir}'` + ] ); }); From 0fdea5fb71c3f57c0bf1604d1316f9ff17f22c88 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 17 Sep 2026 16:09:32 +0000 Subject: [PATCH 28/30] fix: bound the retained orphan even with the idle timer off, and name the right remedy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fourth review round, with one finding it called blocking. - Blocking: with CODEX_BROKER_IDLE_SHUTDOWN_MS=0 — a documented way to disable the broker's idle shutdown — retainedOrphanExpired() never returned true, while SessionEnd now declines to tear the broker down for such a record. The broker, its app-server and its MCP servers would have leaked permanently, and the job stayed "running" with no way to clear it. The window now falls back to the generic staleness bound when the idle timer is disabled, and an unreadable timestamp counts as expired rather than as protected forever: this is the one record that stops a teardown, so "cannot tell" must not mean "keep it alive". Probed both ways: with the timer off a 25h-old orphan is reaped and a 2h-old one is not; with the timer at 10 minutes a 2h-old one is. - The escalation error on a fresh scoped start told the user to "start a fresh thread with --fresh", which is exactly what had just failed. It now names the real cause (the Codex config's sandbox default) and the two real remedies. The message still says "--fresh" on the resume path, where it works. - updateState() passes the snapshot it mutated down to saveStateLocked(), so the other-root prune judges "ids the caller knew about" from that snapshot rather than from a later re-read. A direct saveState() has no snapshot to offer and keeps the re-read, which is now documented. Verified: full npm test 310/310; tsc clean. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UXfvnjSC72HsM6EEPVt2Tg --- plugins/codex/scripts/lib/codex.mjs | 13 +++++++++---- plugins/codex/scripts/lib/state.mjs | 15 +++++++++------ plugins/codex/scripts/session-lifecycle-hook.mjs | 15 ++++++++++----- 3 files changed, 28 insertions(+), 15 deletions(-) diff --git a/plugins/codex/scripts/lib/codex.mjs b/plugins/codex/scripts/lib/codex.mjs index 85d2ef5e2..37380bc28 100644 --- a/plugins/codex/scripts/lib/codex.mjs +++ b/plugins/codex/scripts/lib/codex.mjs @@ -124,14 +124,19 @@ function sandboxModeForPolicy(policy) { // take back what a thread started with `danger-full-access` already has: the // sandbox is off for that thread, so the scope would be a promise this plugin // cannot keep. -function assertScopedResumeNotEscalated(threadId, response) { +function assertScopedNotEscalated(threadId, response, { resumed }) { const effectiveMode = sandboxModeForPolicy(response?.sandbox); if (effectiveMode !== "danger-full-access") { return; } throw new Error( `Thread ${threadId} runs with the Codex sandbox disabled (danger-full-access), so --read-root cannot scope it. ` + - "Start a fresh thread with --fresh to run scoped." + (resumed + ? "Start a fresh thread with --fresh to run scoped." + : // A fresh thread is what just started, so "--fresh" would only repeat + // this. The mode came from the Codex config that applies here. + "It was started that way by the Codex config in effect (sandbox_mode in config.toml), " + + "so drop --read-root or change that default to run scoped.") ); } @@ -1358,7 +1363,7 @@ export async function runAppServerTurn(cwd, options = {}) { // The escalation half of that check still applies, though — a thread // started with the sandbox disabled is not scoped by any profile. if (options.readRoots?.length > 0) { - assertScopedResumeNotEscalated(options.resumeThreadId, response); + assertScopedNotEscalated(options.resumeThreadId, response, { resumed: true }); } else { assertResumedSandbox(options.resumeThreadId, options.sandbox, response); } @@ -1377,7 +1382,7 @@ export async function runAppServerTurn(cwd, options = {}) { // which case the scope this run asked for would not hold here either — // and "start a fresh thread" is the advice the resume path gives. if (options.readRoots?.length > 0) { - assertScopedResumeNotEscalated(response.thread.id, response); + assertScopedNotEscalated(response.thread.id, response, { resumed: false }); } threadId = response.thread.id; } diff --git a/plugins/codex/scripts/lib/state.mjs b/plugins/codex/scripts/lib/state.mjs index d4d33e353..f9464f037 100644 --- a/plugins/codex/scripts/lib/state.mjs +++ b/plugins/codex/scripts/lib/state.mjs @@ -261,7 +261,7 @@ function pruneOtherStateRoot(otherStateDir, retainedIds, knownIds) { } } -function saveStateLocked(cwd, state) { +function saveStateLocked(cwd, state, options = {}) { const previousJobs = loadState(cwd).jobs; const nextJobs = pruneJobs(state.jobs ?? []); const nextState = { @@ -297,10 +297,12 @@ function saveStateLocked(cwd, state) { // only in a non-primary root. Prune every other candidate root down to the same // retained set; new and updated jobs are still only ever written to the primary // root, above. This only ever removes. - // Ids the caller actually decided about: whatever the merged snapshot at the - // top of this function held. Anything else in another root arrived after it - // and is nobody's to drop here. - const knownIds = new Set(previousJobs.map((job) => job.id)); + // Ids the caller actually decided about. updateState() hands down the snapshot + // it mutated, which closes the window between its read and the re-read above; + // a direct saveState() has no such snapshot to offer, so the re-read is the + // best available. Anything in another root outside this set arrived after the + // caller looked and is nobody's to drop here. + const knownIds = options.knownIds ?? new Set(previousJobs.map((job) => job.id)); const [, ...otherStateDirs] = resolveStateDirCandidates(cwd); for (const otherStateDir of otherStateDirs) { pruneOtherStateRoot(otherStateDir, retainedIds, knownIds); @@ -318,8 +320,9 @@ export function updateState(cwd, mutate) { ensureStateDir(cwd); return withLockSync(resolveStateLockDir(cwd), () => { const state = loadState(cwd); + const knownIds = new Set(state.jobs.map((job) => job.id)); mutate(state); - return saveStateLocked(cwd, state); + return saveStateLocked(cwd, state, { knownIds }); }); } diff --git a/plugins/codex/scripts/session-lifecycle-hook.mjs b/plugins/codex/scripts/session-lifecycle-hook.mjs index a33993d7d..d1a86004e 100644 --- a/plugins/codex/scripts/session-lifecycle-hook.mjs +++ b/plugins/codex/scripts/session-lifecycle-hook.mjs @@ -142,15 +142,20 @@ function retainedOrphanExpired(job, env = process.env) { if (job?.workerExited !== true) { return false; } + // Unlike every other record, this one is why SessionEnd declines to tear the + // broker down, so "cannot tell" must not mean "protected forever": a record + // with no readable timestamp has already outlived anything it could protect. const reference = job.updatedAt ?? job.createdAt ?? null; const timestamp = reference ? Date.parse(reference) : Number.NaN; if (!Number.isFinite(timestamp)) { - return false; - } - const windowMs = brokerIdleShutdownMs(env); - if (!Number.isFinite(windowMs) || windowMs <= 0) { - return false; + return true; } + // The broker's idle timer is what actually ends the turn, so it sets the + // window. With that timer disabled (CODEX_BROKER_IDLE_SHUTDOWN_MS=0) nothing + // would end it, so fall back to the generic staleness bound rather than + // pinning the broker — and its app-server and MCP servers — indefinitely. + const idleMs = brokerIdleShutdownMs(env); + const windowMs = Number.isFinite(idleMs) && idleMs > 0 ? idleMs : ACTIVE_JOB_STALENESS_MS; return Date.now() - timestamp > windowMs; } From 84625b1a72851f2fc2e86dfac3dd32f002aa2751 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 17 Sep 2026 16:23:02 +0000 Subject: [PATCH 29/30] test: cover the retained orphan's expiry, and correct two comments about it The fifth review round found nothing blocking. Its two substantive remarks are addressed here: - Both new branches of retainedOrphanExpired() were dead in the suite (the existing retained-orphan test seeds updatedAt in 2099). Two tests now drive them end to end: with the idle timer disabled a two-hour-old orphan is still protected while a 25-hour-old one is reaped, with the timer at ten minutes the two-hour-old one is reaped, and an unreadable timestamp is treated as expired. - Two comments overstated things. The early return in SessionEnd said the broker "idles out on its own timer", which is false precisely in the configuration that made this bound necessary; it now says what ends the wait in both configurations. And the "unlike every other record" framing is gone: isStaleJobRecord() reads an unparseable timestamp conservatively, which is the behavior this function deliberately does not share, not something no other record does. Verified: full npm test 312/312; tsc clean. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UXfvnjSC72HsM6EEPVt2Tg --- .../codex/scripts/session-lifecycle-hook.mjs | 15 +++-- tests/runtime.test.mjs | 60 +++++++++++++++++++ 2 files changed, 70 insertions(+), 5 deletions(-) diff --git a/plugins/codex/scripts/session-lifecycle-hook.mjs b/plugins/codex/scripts/session-lifecycle-hook.mjs index d1a86004e..114fcfc98 100644 --- a/plugins/codex/scripts/session-lifecycle-hook.mjs +++ b/plugins/codex/scripts/session-lifecycle-hook.mjs @@ -142,9 +142,10 @@ function retainedOrphanExpired(job, env = process.env) { if (job?.workerExited !== true) { return false; } - // Unlike every other record, this one is why SessionEnd declines to tear the - // broker down, so "cannot tell" must not mean "protected forever": a record - // with no readable timestamp has already outlived anything it could protect. + // This record is why SessionEnd declines to tear the broker down, so here + // "cannot tell" must not mean "protected forever" — isStaleJobRecord()'s + // conservative reading of an unparseable timestamp would do exactly that. A + // record with no readable timestamp has outlived anything it could protect. const reference = job.updatedAt ?? job.createdAt ?? null; const timestamp = reference ? Date.parse(reference) : Number.NaN; if (!Number.isFinite(timestamp)) { @@ -552,8 +553,12 @@ async function handleSessionEnd(input) { // A turn this session could not interrupt — its worker died before publishing // a turn id — may still be running on this broker. The guard below speaks // only for other sessions' work, so without this the same hook run would - // tear the runtime down under the turn the retain exists to protect. The - // broker idles out on its own timer, which is what bounds the wait. + // tear the runtime down under the turn the retain exists to protect. + // + // What ends the wait: normally the broker's own idle timer, since the turn's + // client is gone. With that timer disabled the next session end in this + // workspace reclaims the broker once the record passes the staleness bound + // (see retainedOrphanExpired), so the runtime is held, never stranded. if (cleanup?.retainedOrphans > 0) { return; } diff --git a/tests/runtime.test.mjs b/tests/runtime.test.mjs index 6a99f6892..8c9dff126 100644 --- a/tests/runtime.test.mjs +++ b/tests/runtime.test.mjs @@ -5767,3 +5767,63 @@ test("the session start hook appends to CLAUDE_ENV_FILE without rewriting it", ( .filter((line) => line.startsWith("export CODEX_COMPANION_SESSION_ID=")); assert.equal(sessionExports.at(-1), "export CODEX_COMPANION_SESSION_ID='sess-next'"); }); + + +function seedRetainedOrphan(workspace, updatedAt) { + const stateDir = resolveStateDir(workspace); + fs.mkdirSync(path.join(stateDir, "jobs"), { recursive: true }); + fs.writeFileSync( + path.join(stateDir, "state.json"), + `${JSON.stringify({ + version: 1, + config: { stopReviewGate: false }, + jobs: [{ + id: "task-orphan", status: "running", title: "Codex Task", jobClass: "task", + sessionId: "sess-owner", pid: null, threadId: "thr_pending", workerExited: true, updatedAt + }] + }, null, 2)}\n`, + "utf8" + ); + return stateDir; +} + +function endSessionFor(workspace, env) { + return run("node", [SESSION_HOOK, "SessionEnd"], { + cwd: workspace, + env: { ...process.env, CODEX_COMPANION_SESSION_ID: "sess-other", ...env }, + input: JSON.stringify({ hook_event_name: "SessionEnd", session_id: "sess-other", cwd: workspace }) + }); +} + +test("a retained orphan expires even when the broker idle timer is disabled", () => { + const workspace = makeTempDir(); + const hoursAgo = (hours) => new Date(Date.now() - hours * 60 * 60 * 1000).toISOString(); + + // Two hours old, idle shutdown off: still protected — nothing has proved the + // turn is over. + let stateDir = seedRetainedOrphan(workspace, hoursAgo(2)); + assert.equal(endSessionFor(workspace, { CODEX_BROKER_IDLE_SHUTDOWN_MS: "0" }).status, 0); + assert.equal(JSON.parse(fs.readFileSync(path.join(stateDir, "state.json"), "utf8")).jobs[0].status, "running"); + + // Past the generic staleness bound, still with the idle timer off: reaped, + // rather than pinning the broker for good. + stateDir = seedRetainedOrphan(workspace, hoursAgo(25)); + assert.equal(endSessionFor(workspace, { CODEX_BROKER_IDLE_SHUTDOWN_MS: "0" }).status, 0); + assert.equal(JSON.parse(fs.readFileSync(path.join(stateDir, "state.json"), "utf8")).jobs[0].status, "failed"); + + // With the timer at ten minutes, the same two-hour-old record is over. + stateDir = seedRetainedOrphan(workspace, hoursAgo(2)); + assert.equal(endSessionFor(workspace, { CODEX_BROKER_IDLE_SHUTDOWN_MS: "600000" }).status, 0); + assert.equal(JSON.parse(fs.readFileSync(path.join(stateDir, "state.json"), "utf8")).jobs[0].status, "failed"); +}); + +test("a retained orphan with an unreadable timestamp does not pin the broker forever", () => { + const workspace = makeTempDir(); + const stateDir = seedRetainedOrphan(workspace, "not-a-timestamp"); + + assert.equal(endSessionFor(workspace, {}).status, 0); + + // This is the one record that stops a teardown, so an unreadable timestamp + // has to count as expired: "cannot tell" must not mean "protected forever". + assert.equal(JSON.parse(fs.readFileSync(path.join(stateDir, "state.json"), "utf8")).jobs[0].status, "failed"); +}); From 877e58b6687fee82161c085329365c1cb4880b00 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 17 Sep 2026 16:28:03 +0000 Subject: [PATCH 30/30] docs: describe what the fork actually guarantees, not just which PRs it carries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The README listed imported PR numbers and flags, but never said what the plugin does now — most of this fork's divergence is behavior under failure, which is invisible from a list of links. - "What The Background Runtime Guarantees" up front: state is checked rather than trusted, session end never reports an outcome that did not happen, one broker per workspace retired by the last session out, a finished job keeps a record with its cause, nothing written is world-readable or half-written, and Node is found where it is actually installed. - /codex:status documents the two reconciled states a dead worker produces, including worker-exited-turn-unknown and why the plugin cannot resolve it. - /codex:result says a non-crashing failure keeps its error text. - /codex:cancel documents its order of operations and the three outcomes that are not a plain cancellation: already finished, a repaired stale record, and the refusal when the turn id was never recorded. - Background Runtime Limits notes that the broker idle window also bounds a retained orphan, and what happens when that timer is disabled. - A new "Where State Lives" table: the two plugin-data roots and why both are real, the durable review-gate config under CODEX_HOME, and that Codex threads and auth stay with the Codex CLI. - The fork-fixes list gains the defects the review rounds found, and says they came from an adversarial review re-run after each round. Verified: full npm test 312/312; npm run check-version passes. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UXfvnjSC72HsM6EEPVt2Tg --- README.md | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 67 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 6bc78ee76..3534a799f 100644 --- a/README.md +++ b/README.md @@ -37,6 +37,29 @@ Accepted `--effort` values are `none`, `minimal`, `low`, `medium`, `high`, and ` unrecognised `--flag` is not silently swallowed into the prompt: the plugin warns on stderr and passes the token through as text. +### What The Background Runtime Guarantees + +Delegated work outlives the Claude session that started it, so most of this fork's divergence from +upstream is about what happens when something dies at the wrong moment. In short: + +- **A job's state is checked, not trusted.** Every status, result and cancel reconciles the record + against the worker process, so a run whose worker died does not read as running forever. +- **Ending a session never reports an outcome that did not happen.** In-flight jobs are interrupted + and recorded as cancelled; a turn that cannot be interrupted keeps its runtime alive instead, on a + bounded window. +- **One broker per workspace, shut down by the last session out.** It is never torn down while + another session has work in flight, and a departed client's Codex threads are unsubscribed rather + than left to leak notifications into the next one. +- **A finished job keeps a record, with its cause.** Sessions end without erasing what ran, so + `/codex:status` answers with an outcome rather than "No job found". +- **Nothing the plugin writes is world-readable, and nothing is half-written.** State, job files, + logs and the review-gate config are 0600 and written through a temp file and a rename. +- **Node is found where you actually installed it.** Hooks resolve a supported toolchain through + `scripts/run-node.sh`, preferring your version manager over a system install. + +[Background Runtime Limits](#background-runtime-limits) and [Where State Lives](#where-state-lives) +say what bounds those windows and where the files are. + ## Requirements - **ChatGPT subscription (incl. Free) or OpenAI API key.** @@ -234,11 +257,20 @@ Use it to: - see the latest completed job - confirm whether a task is still running +A job's status is reconciled against its worker process before it is reported, so a background run whose worker died is not shown as running forever: + +- worker gone, no Codex thread started → `terminated-unknown` +- worker gone while a Codex turn may still be running → still active, with the phase `worker-exited-turn-unknown` + +That second state is the one case the plugin cannot resolve on its own: the turn is server-side and the process that knew its id is gone. It clears when the turn's runtime does — see [Background Runtime Limits](#background-runtime-limits). + ### `/codex:result` Shows the final stored Codex output for a finished job. When available, it also includes the Codex session ID so you can reopen that run directly in Codex with `codex resume `. +A run that failed without crashing — a rejected model, an unsupported parameter — stores the error text too, so a failed job says *why* it failed rather than only that it did. + Examples: ```bash @@ -248,7 +280,7 @@ Examples: ### `/codex:cancel` -Cancels an active background Codex job. +Cancels an active background Codex job: it records the cancellation, interrupts the Codex turn, and then kills the worker — in that order, so a crash midway never leaves an interrupted turn with no recorded outcome. Examples: @@ -257,6 +289,12 @@ Examples: /codex:cancel task-abc123 ``` +**Notes:** + +- a job that finished while you were typing is reported as already finished rather than failing the command +- naming a job whose record went stale (its worker died mid-write) repairs the record and reports the real outcome +- a cancel is refused when the worker exited after Codex accepted the turn but before it recorded the turn id: that turn may still be running and there is nothing to address it by, so reporting it cancelled would be a lie. The job stays active until its runtime goes. + ### `/codex:setup` Checks whether Codex is installed and authenticated. @@ -343,8 +381,20 @@ Set any of these in the environment before starting Claude Code, for example: export CODEX_BROKER_IDLE_SHUTDOWN_MS=1800000 # 30 minutes ``` +`CODEX_BROKER_IDLE_SHUTDOWN_MS` does double duty: it also bounds how long a job whose worker died with a turn still possibly running keeps the runtime up (the `worker-exited-turn-unknown` state above). Session end leaves the broker alive for such a job instead of killing the turn under it; once that window passes, the next session end reclaims both. With the timer disabled (`0`), the fallback is 24 hours rather than forever. + These are advanced knobs for tuning resource usage in long-running or resource-constrained environments; most users will never need to touch them. +### Where State Lives + +| What | Where | Notes | +| --- | --- | --- | +| jobs, logs, broker record | `$CLAUDE_PLUGIN_DATA/state//`, or a temp-dir fallback | `CLAUDE_PLUGIN_DATA` is only set when the plugin runs as a hook, so both locations are real. Reads check both, writes go to the current one. | +| review-gate flag | `$CODEX_HOME/plugin-cc/config/.json` | Durable on purpose: clearing the state dir, or a different `CLAUDE_PLUGIN_DATA`, must not silently turn the gate off. Written privately (0600) and atomically. | +| Codex threads and auth | wherever your Codex CLI keeps them | The plugin never holds a second copy — see the [FAQ](#does-the-plugin-use-a-separate-codex-runtime). | + +One broker serves every session in a workspace. It is shut down by the last session out, never by a session that still has another's work in flight, and it releases its Codex thread subscriptions as clients disconnect, so a departed session's notifications never reach the next one. + ### Moving The Work Over To Codex Delegated tasks and any [stop gate](#what-does-the-review-gate-do) run can also be directly resumed inside Codex by running `codex resume` either with the specific session ID you received from running `/codex:result` or `/codex:status` or by selecting it from the list. @@ -395,8 +445,24 @@ Beyond the imports, this fork carries fixes for defects the imports themselves s had inverted (see the note under [Requirements](#requirements)) - the durable review-gate config is written privately and atomically, so an interrupted write cannot silently disable the gate +- `--read-root` works on a resumed thread: the scoped run sends a permission profile rather than a + sandbox mode, so asserting the mode had refused every scoped resume — and its advice dropped the + write grant. A scoped run on a thread started with the sandbox disabled is still refused +- cancelling refuses *before* taking the job's terminal claim, so a refused cancel leaves nothing + behind for the next one to turn into a bogus `cancelled` record +- a job retained because its turn may still be running keeps the runtime up, stays readable in + `/codex:status`, and expires on a bounded window instead of pinning the broker +- the state a job is deleted from is the state its files are deleted from, so a prune another + process is holding can no longer leave a record without its log and detail file +- disabling the review gate is not outvoted by a stale enable left under another plugin-data root +- `CLAUDE_ENV_FILE` is only ever appended to: it is shared with other plugins' hooks, and rewriting + it dropped whatever they had just written - the app-server typecheck (`npm run build`) passes +Each of those came out of an adversarial review of the merges, re-run after every round of fixes; +the reasoning behind each is in its commit message, and each has a regression test that fails +without it. + Not imported: [#733](https://github.com/openai/codex-plugin-cc/pull/733) (durable startup cancellation) — its behavior is already covered here by the terminal-claim mechanism, and its marker files would add a second source of truth for the same decision. [#761](https://github.com/openai/codex-plugin-cc/pull/761)