Skip to content

Stop orphaned Codex companion brokers#490

Open
ckfchampion wants to merge 1 commit into
openai:mainfrom
ckfchampion:fix/orphaned-brokers-upstream
Open

Stop orphaned Codex companion brokers#490
ckfchampion wants to merge 1 commit into
openai:mainfrom
ckfchampion:fix/orphaned-brokers-upstream

Conversation

@ckfchampion

Copy link
Copy Markdown

Problem

The shared app-server broker outlives the Claude Code session that spawned it. When a session exits without a clean teardown, the broker is reparented to init and keeps listening forever, holding its codex app-server child open with it. Nothing ever reaps them, so they accumulate for as long as the machine stays up.

On one developer machine this had reached 8 orphaned brokers, ~1.3 GB RSS, the oldest alive for 41 hours — every one with PPID=1, i.e. no session attached. Running the plugin's own test suite makes it worse, since each run leaves its helper brokers behind too.

Fix

Shut the broker down after 15 minutes with no connected clients.

  • Foreground and background jobs hold a broker connection for their whole lifetime, so an active job always keeps its broker alive — the timer only ever fires on a broker nobody is using.
  • CODEX_COMPANION_BROKER_IDLE_TIMEOUT_MS overrides the window; 0 disables the timer entirely.
  • No client-side change is needed: ensureBrokerSession() already probes the endpoint and respawns when it finds none, so an idle-exited broker is transparently recreated on the next call. A successful probe also resets the idle timer, so there is no window where a client can connect to a broker that is about to exit.

Two shutdown-path corrections come with it:

  • Close the listener before awaiting async child cleanup. Previously a reconnect could slip into the async shutdown window and inherit a closing app-server client.
  • Reject connections that are delivered after server.close(), rather than adding them to the live socket set.

Tests

Adds tests/broker-idle.test.mjs, covering: exit + runtime-file cleanup after the last client disconnects; idle shutdown deferred while a client is still connected; the timeout being disabled via 0; and clean shutdown while new clients race to connect.

tests/fake-codex-fixture.mjs now sets a 2s idle window so the suite stops leaving detached helpers behind.

Full suite: 91 passing. The 4 failures on this branch (status/result/resolveStateDir) reproduce identically on an unmodified main, so they are pre-existing and unrelated.

A shared app-server broker outlives the Claude Code session that spawned it.
When the session exits without a clean teardown the broker is reparented to
init and stays listening forever, holding its `codex app-server` child open.
These accumulate: on one developer machine, 8 orphaned brokers and their
children held ~1.3 GB RSS, the oldest alive for 41 hours.

Shut the broker down after 15 minutes with no connected clients. Foreground
and background jobs hold a connection for their whole lifetime, so an active
job always keeps its broker alive; `CODEX_COMPANION_BROKER_IDLE_TIMEOUT_MS`
overrides the window and `0` disables the timer. `ensureBrokerSession()`
already probes the endpoint and respawns when it finds none, so an
idle-exited broker is transparently recreated on the next call.

Also close the listener before awaiting async child cleanup, and reject
connections that arrive during shutdown, so a reconnect cannot slip into the
shutdown window and inherit a closing app-server client.

Tests run with a 2s idle window so the suite does not leave detached helpers.
@ckfchampion ckfchampion requested a review from a team July 13, 2026 07:33

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: da16256dbd

ℹ️ 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".

Comment on lines +161 to +162
await shutdown(server);
process.exit(0);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Clear stale broker session on idle exit

In the idle-timeout path, the broker exits after removing only the socket/pid runtime files, leaving the saved broker session in broker.json. After the normal idle timeout, loadBrokerSession(cwd) still points at the deleted socket; /codex:setup then calls getCodexAuthStatus() with reuseExistingBroker: true, attempts that dead endpoint, and reports the workspace as not ready with connect ENOENT instead of starting or reporting a fresh direct runtime. Clear the broker session when this self-shutdown path runs, or make the setup/status path validate and discard stale sessions.

Useful? React with 👍 / 👎.

@axisrow

axisrow commented Jul 14, 2026

Copy link
Copy Markdown

Nice — this overlaps directly with a race I already caught and patched on the fork: the shutdown window where a pid can be reused. Your "close the listener before awaiting async child cleanup" and "reject connections delivered after server.close()" are the right corrections — they're the same class of gap as my PID-reuse guard (only tree-kill by pid when the endpoint probe confirmed it was live).

One thing worth pressure-testing: the interaction between this idle-timer and an active respawn. ensureBrokerSession() respawns when the probe finds nothing — but does a successful probe reset the idle timer in the same atomic step that a racing idle-shutdown might fire? Your description says "a successful probe also resets the idle timer," but if the reset and the fire are on different ticks, there's a window where a client reconnects to a broker mid-self-shutdown. Your tests/broker-idle.test.mjs race case covers new clients connecting during shutdown — does it also cover a probe-reset racing the fire on the same broker? If not, that's the one case I'd add.

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.

2 participants