Skip to content

fix(broker): stop tearing down a live broker that misses the readiness probe - #768

Open
mzl9039 wants to merge 1 commit into
openai:mainfrom
mzl9039:fix/broker-readiness-race
Open

mzl9039 wants to merge 1 commit into
openai:mainfrom
mzl9039:fix/broker-readiness-race

Conversation

@mzl9039

@mzl9039 mzl9039 commented Sep 18, 2026

Copy link
Copy Markdown

Problem

ensureBrokerSession() probes the persisted broker with a 150 ms budget:

// plugins/codex/scripts/lib/broker-lifecycle.mjs
return await waitForBrokerEndpoint(endpoint, 150);

A broker that is currently serving a turn keeps its event loop busy, so it can easily
fail to accept the probe connection inside that window. The failed probe is then treated
as "the broker is gone" and the session is torn down.

Today that teardown mostly orphans the broker (this is #753). But once killProcess is
wired up — which is exactly what #762 does — the same path kills a perfectly healthy
broker
, taking down its app-server and the in-flight turn with it. The caller sees:

codex app-server exited before the turn completed.

A second, independent race makes it easier to hit: ensureBrokerSession() has no mutual
exclusion, so concurrent clients can all observe loadBrokerSession() === null, each spawn
a broker, and let the last writer win — orphaning the other brokers along with the
app-servers and turns they own.

Reproduction

Submit two background tasks in the same workspace a few seconds apart:

node plugins/codex/scripts/codex-companion.mjs task "<long read-only task>" --background
sleep 3
node plugins/codex/scripts/codex-companion.mjs task "<another long read-only task>" --background
before after
first task failed — codex app-server exited before the turn completed. completed
second task running → completed completed
broker processes replaced each other, then none one, reused
app-servers killed mid-turn yes none

Fix

  1. Raise the readiness probe budget to 3 s, and never tear down a broker whose process
    is still alive
    . A failed probe only proves the broker did not answer in time; only
    ESRCH from process.kill(pid, 0) proves it is gone (EPERM and PID reuse are
    treated conservatively as alive).
  2. Serialize the check-then-create window with a stale-aware lock file, so concurrent
    callers share one broker instead of racing to create several.

If the lock cannot be acquired within its budget the code falls through to the previous
behaviour rather than failing the caller outright.

Tests

tests/broker-lifecycle.test.mjs adds two cases. Both fail on main with the
relevant assertion and pass with this change:

  • a live broker must never be killed after a failed probe
  • concurrent callers must share one broker

Full suite: 93 passing (91 before + 2 new), no regressions.

Note: running the suite inside a live Claude Code session needs
env -u CLAUDE_PLUGIN_DATA -u CODEX_COMPANION_SESSION_ID, otherwise
state.test.mjs picks up the ambient plugin data dir — that is #456, not this change.

Relationship to existing work

…s probe

`ensureBrokerSession()` probes the persisted broker with a 150 ms budget:

    return await waitForBrokerEndpoint(endpoint, 150);

A broker that is serving a turn keeps its event loop busy, so it can easily miss
that window. The probe failure is then treated as "the broker is gone" and the
session is torn down — which, once `killProcess` is wired up, kills a perfectly
healthy broker together with the app-server and the in-flight turn it owns.

Two changes:

1. Raise the readiness probe budget to 3 s, and never tear down a broker whose
   process is still alive. A failed probe only proves the broker did not answer
   in time; only `ESRCH` from `process.kill(pid, 0)` proves it is gone.
2. Serialize the check-then-create window with a stale-aware lock file.
   Concurrent clients could all observe `loadBrokerSession() === null`, each
   spawn a broker, and let the last writer win — orphaning the other brokers
   along with their app-servers and in-flight turns.

Both new tests fail on main with the relevant assertion and pass with the fix.
Full suite: 93 passing (91 before + 2 new).
@mzl9039
mzl9039 requested a review from a team September 18, 2026 09:42
ApexAiOfficial added a commit to ApexAiOfficial/codex-plugin-cc that referenced this pull request Sep 25, 2026
Audited against open upstream reports on openai/codex-plugin-cc; each defect
was reproduced or verified against this fork and has a regression test in
tests/substrate.test.mjs (dedicated fake app-server).

- openai#302 unbounded waits: every RPC has a bounded wall-clock timeout (120s
  default, command/exec aware, CODEX_COMPANION_RPC_TIMEOUT_MS, 0 disables),
  and requests on a dead connection fail immediately. The turn watchdog does
  not use a silence timeout (legitimate turns are silent for long periods):
  it fails fast when the connection closes, and after quiet periods asks the
  app-server via thread/read whether the thread is still active, recovering a
  turn whose completion event was lost and failing only an app-server that
  stops answering.
- openai#453 zombie broker: the broker exits when its app-server child dies, so the
  next caller starts a healthy broker instead of wedging.
- openai#706/openai#707 retained subscriptions: the broker tracks per-socket thread
  ownership (subagents inherit their parent's owners) and sends
  thread/unsubscribe when the last owner disconnects; requests for a thread
  wait (bounded) for its in-flight unsubscribe. Simpler than openai#707 because this
  broker already serializes active requests and streams.
- openai#762 + openai#768 together: broker acquisition is serialized; a broker whose
  process is alive is never torn down or killed because it missed a probe
  (the caller uses a private app-server); metadata is cleared only when the
  process is provably gone; only a just-spawned broker that never became
  ready is killed, identity-checked. SessionEnd's kill is identity-checked.
- openai#574 RC3: non-retryable errors fail a turn even when completion is
  inferred. The upstream fix (any error fails) is wrong: error notifications
  carry willRetry, and retried turns can succeed.
- openai#775: fileChange start events without a change list no longer throw.
- openai#740 reproduced with real Codex 0.144.1: live thread/resume ignores the
  requested sandbox in both directions (write-capable threads stayed writable
  after a read-only resume), and a per-turn override persists to later turns.
  Every turn now sends an explicit sandboxPolicy.

Also fixes a same-process withFileLockAsync bug (a second async holder
treated the first as stale) and makes the test harness stop brokers when a
test process is killed by a signal.

Validation: npm test 156/156, tsc clean, no stray processes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant