fix: multi-session broker and state lifecycle bugs

[sublimator]

Authored by Claude (Opus 4.8) + Codex (gpt-5.5) — written and reviewed collaboratively across several passes.

Fixes a family of bugs that appear when more than one Claude Code session shares
a project directory. The broker and state files are keyed by cwd, so sessions
coordinate through shared processes and files — with no coordination primitive.

What this fixes

1. SessionEnd tore down a broker other sessions were using. Any session
   ending shut down the cwd-shared broker with no ownership check, killing other
   sessions' running jobs and leaving records stuck at "running". Teardown is
   now skipped while another session has active jobs; SessionEnd reads job state
   directly (fails closed if unreadable) and removes only its own artifacts.
2. The broker kept answering after its child app-server died (Sequential task calls fail ~50% with "codex app-server connection closed" — broker reused after single-turn exit, shouldRetryDirect misses clean close #402). Health
   probes passed locally, so the next turn failed with a bare connection closed
   in a pass/fail/pass/fail pattern. The broker now exits when its child exits,
   and the client treats connection closed as a broker failure worth a retry.
3. The test suite leaked ~25–30 broker processes per run, and setup tests read
   live workspace state (a live repo-root broker failed five unrelated tests).
   Fixed with a global broker-cleanup hook and temp-cwd isolation.
4. Concurrency-safety hardening: state writes no longer delete a
   queued/running job's files, broker.json is written atomically, and
   terminateProcessTree no longer group-kills pid <= 1.

This ships the safe, self-contained fixes. It does not attempt the larger
refactor the pattern calls for (a state-transaction lock and a broker-acquisition
lease) — that's a design change better done as its own PR. The analysis below
explains why.

Tests

node --test tests/*.test.mjs → 92/92, verified across repeated sequential
runs. (Earlier intermittent hangs were host contention from running multiple
suites at once, not a code issue.) Follow-ups, not in this PR: dedicated
regression tests for the #402 retry path and the pid <= 1 guard.

Related: #380, #402, #286, #416.

---

The core problem — why 19 findings, one cause

codex-plugin-cc coordinates several independent OS processes — interactive
sessions, detached background workers, and lifecycle hooks — through shared JSON
files (state.json, broker.json) and one broker process, all keyed by the
working directory. Nothing serialises them: there is no cross-process lock,
compare-and-swap, or liveness check anywhere in the tree, so every mutation is an
unguarded read-modify-write or check-then-act done as if a single owner held the
files. The tmp+rename writes keep individual writes from tearing but never make
the compound operations atomic — which is why the same defect surfaces
repeatedly: concurrent processes clobber each other's records, spawn duplicate
brokers, and delete files or kill pids out from under a peer still using them.

An eight-surface audit (each finding checked by a separate verifier) confirmed 19
multi-session races. They collapse to a few root causes:

Cluster	Root cause	Findings
A. Unlocked state.json	load → mutate → save is not atomic across processes, and saveState then deletes job files absent from a possibly-stale snapshot	8
B. No broker ownership	cold-start spawn is check-then-act with no lock; broker.json written non-atomically	4
C. BROKER_BUSY handling	write tasks retry direct (two agents on one tree); setup/auth reports a busy broker as logged-out	3
D. PID trusted without identity	stored pids group-killed with no liveness/identity check (PID reuse)	2
E. Cancel clobber	cancel overwrites a job that finished mid-cancel	1
F. Jobless-broker ownership	teardown gate counts only tracked jobs	1

Clusters A and B are twelve of the nineteen, and most come down to one thing:
no lock on a shared file.

The bugs in the source — pre-fix code, pinned permalinks

1. SessionEnd shut the broker down unconditionally — no check for other
sessions or their jobs:

📍 plugins/codex/scripts/session-lifecycle-hook.mjs:101-114 @ 86ea7dd

 101   if (brokerEndpoint) {
 102     await sendBrokerShutdown(brokerEndpoint);
 103   }
 104 
 105   cleanupSessionJobs(cwd, input.session_id || process.env[SESSION_ID_ENV]);
 106   teardownBrokerSession({
 107     endpoint: brokerEndpoint,
 108     pidFile,
 109     logFile,
 110     sessionDir,
 111     pid,
 112     killProcess: terminateProcessTree
 113   });
 114   clearBrokerSession(cwd);

3. Setup tests ran in the real repo root, so a live broker for the repo made
them read live state:

📍 tests/runtime.test.mjs:36-45 @ 86ea7dd

  36   const result = run("node", [SCRIPT, "setup", "--json"], {
  37     cwd: ROOT,
  38     env: buildEnv(binDir)
  39   });
  40 
  41   assert.equal(result.status, 0);
  42   const payload = JSON.parse(result.stdout);
  43   assert.equal(payload.ready, true);
  44   assert.match(payload.codex.detail, /advanced runtime available/);
  45   assert.equal(payload.sessionRuntime.mode, "direct");

The test harness reaped a throwaway sleep process but never the brokers it
spawned:

📍 tests/runtime.test.mjs:1560-1570 @ 86ea7dd

1560   t.after(() => {
1561     try {
1562       process.kill(-sleeper.pid, "SIGTERM");
1563     } catch {
1564       try {
1565         process.kill(sleeper.pid, "SIGTERM");
1566       } catch {
1567         // Ignore missing process.
1568       }
1569     }
1570   });

Full worked write-ups for all four bugs — with more extracts — are in the
dossier
on the analysis/broker-lifecycle branch.

What the fixes reach, and the refactor they don't

The race and the damage are separable, and the damage is the simpler thing to
fix. This PR targets the destruction, not the race:

• broker.json atomic write (was missing; state.json already had it).
• A GC guard so saveState/pruneJobs never delete a queued/running job's
  files — a concurrent worker's output survives a stale write.
• The pid > 1 group-kill guard, the Sequential task calls fail ~50% with "codex app-server connection closed" — broker reused after single-turn exit, shouldRetryDirect misses clean close #402 broker self-teardown, and the
  SessionEnd ownership gate.

The seam the simple fix can't reach: saveState still writes the caller's whole
job array, so a stale write can drop another session's job record (its files
now survive, but the ownership gate can't see it) or resurrect a finished job as
running. Closing that needs the lock, or a merge that knows the caller's
intent. That's the refactor, out of scope here:

1. A locked state transaction around load → mutate → save, with
   cleanupSessionJobs reading fresh state — closes cluster A.
2. The same lock around broker acquisition (one lease, no duplicate
   cold-start brokers) — closes cluster B.
3. Treat BROKER_BUSY as serialisation, not transport failure for write
   tasks — a protocol decision, closes finding 5.
4. PID identity before group-kill, compare-and-set on cancel, broker-side
   shutdown awareness — the tail.

Items 1–2 are the lock and cover about half; 3–4 are separate mechanisms. One
design change plus a few independent fixes — fewer moving parts than nineteen
tickets, but more than one lever.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2ff996dd0c

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Avoid retrying mid-flight broker turns

When a brokered request has already been forwarded and the broker socket then closes (for example the child app-server crashes during turn/start or review/start), this new brokerConnectionClosed branch makes withAppServer re-run the entire fn against a direct app-server. Those requests can already have created a turn or started file-modifying work before the disconnect, so retrying here can launch a duplicate turn instead of surfacing the lost connection; the direct retry should be limited to connect/initialize failures or otherwise prove no request was in flight.

Useful? React with 👍 / 👎.

[sublimator]

@chatgpt-codex-connector Good catch. I am considering narrowing this so a bare connection closed only falls back to direct when no app-server request has been sent yet, and otherwise surfaces the lost broker connection instead of replaying thread/start, turn/start, or review/start.

Does that match the safety boundary you intended, or do you see a better way to handle a broker child death between turns without risking duplicate side effects?

• Codex

[chatgpt-codex-connector]

Note

To use Codex here, create a Codex account and connect to github.

[sublimator]

Implemented in 3649797: the client now tracks whether any non-initialize app-server request was sent on the broker connection, and bare connection closed only takes the direct fallback before that point. Once thread/start, turn/start, review/start, etc. has been attempted, the original connection loss is surfaced instead of replaying the whole operation direct.

Added a regression where a stub broker acknowledges thread/start, receives turn/start, then drops the socket; the task fails and the fake direct app-server is never started.

Verified:

• node --test --test-name-pattern "task does not retry direct" tests/runtime.test.mjs

• node --test tests/runtime.test.mjs (64/64)

• node --test tests/commands.test.mjs tests/render.test.mjs tests/state.test.mjs tests/bump-version.test.mjs tests/git.test.mjs tests/process.test.mjs tests/broker-endpoint.test.mjs (29/29)

• Codex