Reap leaked broker sessions across working directories - #660
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0cc9d4ecff
ℹ️ 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".
| const ownsIt = alive && ownerSessionId !== null && readBrokerOwner(sessionDir) === ownerSessionId; | ||
| if (alive && !ownsIt) { | ||
| continue; // a live broker owned by another (or unknown) session — leave it |
There was a problem hiding this comment.
When session B uses the same working directory as session A, ensureBrokerSession returns A's already-live broker without changing its owner. If A later ends from another working directory, this owner comparison treats the shared broker as exclusively A's and shuts it down or kills it, potentially interrupting B's in-flight request. Tracking only the creator is therefore insufficient; the reaper must account for other sessions currently reusing the broker.
Useful? React with 👍 / 👎.
| const pid = readBrokerPid(sessionDir); // null when the pid is dead/invalid | ||
| const alive = pid !== null && isPidAlive(pid); | ||
| const ownsIt = alive && ownerSessionId !== null && readBrokerOwner(sessionDir) === ownerSessionId; |
There was a problem hiding this comment.
If a broker exits without removing broker.pid and the OS reuses that PID before its owning session ends, process.kill(pid, 0) reports the unrelated process as alive; the matching owner file then causes terminateProcessTree to kill that unrelated PID or process group. Confirm that the endpoint is actually served by the recorded broker, or persist and validate stronger process identity, before passing this PID to killProcess.
Useful? React with 👍 / 👎.
A broker is spawned per working directory and reused across sessions, but teardownBrokerSession only ran for the current cwd at SessionEnd, so brokers for every other cwd leaked as orphaned broker + app-server processes plus os.tmpdir()/cxc-* socket dirs (tens of processes / ~1 GB after a few days of multi-repo use). Give the broker ownership of its own lifecycle instead of having sessions guess when it is safe to kill: - app-server-broker.mjs: the broker exits itself once it has had no connections and no in-flight work for CODEX_BROKER_IDLE_MS (default 30 min; <= 0 disables). Callers already respawn a broker on demand, so self-exit when idle is safe. - broker-lifecycle.mjs: reapBrokerSessions() now only GCs the *directories* of brokers that are already gone (dead PID, or a live PID whose broker endpoint is unreachable — a reused/stale PID). It never signals a PID and never touches a live, reachable broker, so it cannot interrupt a session sharing a broker or kill an unrelated process that reused a stale PID. - session-lifecycle-hook.mjs: SessionStart/SessionEnd run the dead-only GC; SessionEnd no longer force-kills or shuts down the current cwd's broker (that was unsafe when the broker is shared) — it just drops the session's pointer and lets the broker self-exit when idle. Addresses two review notes on the earlier revision: killing a broker a concurrent session reuses, and signalling a possibly-reused PID. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
0cc9d4e to
fe08482
Compare
|
Thanks for the review — both points were spot on. I reworked the fix rather than patch around them. Instead of an external reaper deciding when to kill a broker (which can't know about a concurrent session reusing it, and can mis-signal a reused PID), the broker now owns its own lifecycle:
Both original P1s (killing a broker a concurrent session reuses; signalling a possibly-reused PID) are resolved by never killing a live broker at all. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe084821e4
ℹ️ 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".
| // Do not shut down or kill this cwd's broker: it may be shared with a | ||
| // concurrent session, and it exits itself once idle (app-server-broker.mjs). | ||
| // Just drop this session's pointer to it. | ||
| clearBrokerSession(cwd); |
There was a problem hiding this comment.
When two Claude sessions share a working directory and session A ends while session B has a background turn running, this deletes the workspace-wide broker.json, not a session-local pointer. The revised code no longer kills B's broker, but fresh evidence here shows that B's subsequent /codex:cancel uses reuseExistingBroker with no endpoint and sends its interrupt to a new direct app server rather than the broker executing the turn; terminating B's client then leaves the underlying turn running until idle shutdown. A normal subsequent command also starts a second broker while the original is still active, recreating the process contention this change is intended to prevent.
Useful? React with 👍 / 👎.
| idleTimer = setTimeout(() => { | ||
| if (isIdle()) { | ||
| shutdown(server).finally(() => process.exit(0)); |
There was a problem hiding this comment.
After a session remains open but makes no broker connections for the default 30 minutes, this shutdown removes the socket/session directory while leaving the workspace's broker.json intact. Consumers such as getCodexAuthStatus and getSessionRuntimeStatus load that file without checking endpoint readiness, so /codex:setup connects to the stale socket and can report an authenticated user as logged out, while /codex:status continues claiming a shared runtime is active; either remove the matching state record during shutdown or make these reuse/status paths validate it.
Useful? React with 👍 / 👎.
The state record now lives and dies with the broker it points at: - SessionEnd no longer clears the workspace-wide broker.json. The record is shared by every session on the cwd, so a session ending must not delete it out from under a peer whose turn is still running. - The broker retires its own record at the start of shutdown(), guarded on the endpoint still matching, so a clean exit (idle, broker/shutdown, SIGTERM/SIGINT) leaves no stale pointer and a record that was already replaced is never touched. - The reuseExistingBroker path validates the recorded endpoint before handing it out, so a record left by an uncleanly killed broker degrades to a direct app server instead of erroring against a dead socket. - getSessionRuntimeStatus checks the unix socket file exists before reporting a shared runtime. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Both points confirmed and fixed in fa1e404. The root of both was the same: broker.json's lifecycle didn't follow the broker-owns-itself model the rest of the fix moved to. P1 (SessionEnd deleting the shared record): P2 (stale record after idle shutdown): three layers, in order of preference:
Functionally tested: SessionEnd leaves a live broker and its record untouched; idle shutdown removes process, session dir, and record; a stale record reports direct startup and a reused connect degrades to direct spawn; a newer broker's record survives an older broker's shutdown. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa1e40441b
ℹ️ 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".
| await sendBrokerShutdown(brokerEndpoint); | ||
| } | ||
|
|
||
| cleanupSessionJobs(cwd, input.session_id || process.env[SESSION_ID_ENV]); |
There was a problem hiding this comment.
When SessionEnd fires while this session has a running background task, cleanupSessionJobs only terminates the runner and removes its job metadata; unlike handleCancel, it never sends turn/interrupt. Fresh evidence in this revision is that the broker/app-server is now deliberately left alive afterward, so an already-started Codex turn—potentially a --write task—continues without its client and may keep modifying the workspace until completion or idle shutdown, while its notifications can be routed to a later broker client. Interrupt each owned job using its saved threadId and turnId before terminating the runner.
Useful? React with 👍 / 👎.
Now that the shared app server deliberately outlives a session, killing a background job's runner at SessionEnd left its already-started Codex turn running clientless in the broker (potentially a --write task mutating the workspace) until it completed or the broker idled out. cleanupSessionJobs now mirrors handleCancel: for each still-running job owned by the ending session it sends turn/interrupt with the threadId and turnId from the per-job file (falling back to the state entry) before terminating the runner. The interrupt is skipped when no live broker endpoint exists, since the turn died with its app server. A corrupt job file falls back to the state entry instead of failing the whole hook. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Confirmed and fixed in 4cef183. This was a real regression the redesign introduced: previously the SessionEnd broker kill took any in-flight turn down with it, and removing the kill silently removed that guarantee.
Functionally tested: with a live broker, the hook interrupts and exits 0, the job is removed, and the broker plus its record survive; with no broker, the fast path skips the interrupt (~150ms); missing thread/turn ids and corrupt job files degrade gracefully. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4cef1830e0
ℹ️ 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".
| const { threadId, turnId } = readJobTurn(workspaceRoot, job); | ||
| if (threadId && turnId) { | ||
| try { | ||
| await interruptAppServerTurn(cwd, { threadId, turnId }); |
There was a problem hiding this comment.
When a live broker accepts the connection but its app server never answers turn/interrupt, this await never settles because interruptAppServerTurn and the underlying request have no timeout. The SessionEnd hook itself has a five-second timeout in plugins/codex/hooks/hooks.json, so Claude eventually kills the hook before terminateProcessTree or the state cleanup runs, leaving exactly the runner and turn this path is intended to stop. Apply a short deadline to the interrupt attempt and always proceed with terminating the runner.
Useful? React with 👍 / 👎.
| export function getSessionRuntimeStatus(env = process.env, cwd = process.cwd()) { | ||
| const endpoint = env?.[BROKER_ENDPOINT_ENV] ?? loadBrokerSession(cwd)?.endpoint ?? null; | ||
| const recorded = env?.[BROKER_ENDPOINT_ENV] ?? loadBrokerSession(cwd)?.endpoint ?? null; | ||
| const endpoint = recorded && isBrokerEndpointPresent(recorded) ? recorded : null; |
There was a problem hiding this comment.
This new endpoint-existence gate deterministically breaks the existing setup and status honor --cwd when reading shared session runtime test: its saved endpoint is unix:/tmp/fake-broker.sock, so npm test now reports direct startup instead of the asserted shared session. Update that fixture to provide an endpoint satisfying the new validity contract, or preserve the previous contract, so the repository test suite passes.
Useful? React with 👍 / 👎.
| function isBrokerEndpointPresent(endpoint) { | ||
| try { | ||
| const target = parseBrokerEndpoint(endpoint); | ||
| return target.kind === "unix" ? fs.existsSync(target.path) : true; |
There was a problem hiding this comment.
When a broker crashes during an active Claude session, its Unix socket and broker.json remain until another SessionStart/SessionEnd reaper runs, so mere filesystem presence makes /codex:status continue claiming a shared runtime is active even though no process is listening; stale Windows pipe endpoints are unconditionally treated as active as well. Fresh evidence beyond the earlier idle-shutdown issue is this unclean-death path, because clean idle shutdown now removes the record. Validate connectivity or recorded process identity rather than treating the endpoint's representation as proof of liveness.
Useful? React with 👍 / 👎.
The SessionEnd hook runs under a 5-second harness timeout, so the new per-job turn interrupts are raced against a shared 2s budget: a broker whose app server never answers turn/interrupt can no longer get the hook killed before runner termination and state cleanup run. main() exits explicitly, since an abandoned interrupt attempt can hold handles that would otherwise keep the hook alive until the harness kills it. getSessionRuntimeStatus now judges a recorded session live by its recorded broker pid (signal 0, EPERM counts as alive), falling back to unix-socket presence only when no usable pid is recorded. A broker that crashed mid-session leaves its socket and record on disk until a reaper runs, so file presence alone overstated liveness; the pid check also covers Windows pipe endpoints, which cannot be stat'd meaningfully. Env overrides keep their previous semantics, including an empty value masking the recorded session. The --cwd runtime-status test fixture records a live pid to satisfy the new validity contract. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f89e894922
ℹ️ 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".
| const brokerEndpoint = loadBrokerSession(cwd)?.endpoint ?? null; | ||
| brokerAlive = brokerEndpoint | ||
| ? await waitForBrokerEndpoint(brokerEndpoint, 150).catch(() => false) | ||
| : false; |
There was a problem hiding this comment.
When jobs use CODEX_COMPANION_APP_SERVER_ENDPOINT without a matching local broker.json, this gate sets brokerAlive to false, so SessionEnd kills the runner and removes its metadata without sending turn/interrupt; a write-capable turn can therefore continue in the externally configured broker. Fresh evidence beyond the prior interrupt finding is that CodexAppServerClient.connect prioritizes the environment endpoint, while this new liveness check consults only loadBrokerSession(cwd). Select and probe the endpoint using the same precedence as the client before deciding to skip interruption.
Useful? React with 👍 / 👎.
Findings from an exhaustive multi-lens adversarial review of the cumulative diff, plus a cross-model review pass on the fixes: - A client that disconnected during its awaited streaming request could be assigned to activeStreamSocket afterwards, stranding a dead socket that kept isIdle() false forever and busy-rejected other clients; the assignment now checks socket.destroyed, and releasing stream ownership on turn/completed reschedules the idle timer. - The broker now observes its codex app-server client's exit and shuts itself down: a dead child previously left a listening zombie that probes passed, the reaper skipped, and nothing could kill. - Forwarded requests are bounded (CODEX_BROKER_REQUEST_TIMEOUT_MS, default 10 min); a request unanswered that long means a wedged app server, so the broker terminates rather than releasing ownership of a possibly still-executing turn to a later client. - Recursive session-dir removal on shutdown is restricted to cxc- dirs directly under the OS temp dir; a manual --pid-file anywhere else has only the broker's own files unlinked. - All shutdown paths share one promise, so a signal landing during an idle shutdown cannot process.exit() mid-cleanup. - withAppServer retries direct whenever connect() itself fails (fn never ran, always safe: covers ENOENT/ECONNREFUSED/ECONNRESET and initialize-phase closes when racing an idle shutdown); post-connect retries remain limited to the broker-busy rejection, since replaying fn after a mid-flight failure could repeat side effects. - The SessionEnd interrupt skips the synchronous availability probe (a responding broker proves the runtime), so the 2s budget race actually bounds wall-clock; the broker probe honors the env endpoint precedence that connect() uses. - The reaper leaves a directory whose broker.pid is empty or unparseable alone (possible torn write from a starting broker). - CODEX_BROKER_IDLE_MS parsing is strict-integer with a 2^31-1 clamp: parseInt truncated "30m" to 30ms, and larger values overflowed setTimeout to ~1ms, inverting "effectively never" into instant. - Connections landing mid-shutdown are destroyed instead of holding server.close() open. - New tests cover the reaper GC matrix and pid-based runtime-status liveness; the test env caps broker idle at 30s so suite runs stop leaving broker + fake-codex pairs alive for 30 minutes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Following the round-4 fixes we ran an exhaustive multi-lens adversarial review of the cumulative diff (7 independent reviewers over consumer, concurrency, harness-constraint, portability, failure-injection, and maintainer lenses; every finding attacked by 2 skeptics). It confirmed several defects in this PR beyond what has been raised here, all fixed in this push. The broker's idle self-shutdown could be permanently disarmed, making a wedged broker immortal now that no external path kills brokers:
Other fixes in this push:
Verified: full test suite unchanged against the pre-change baseline; functional runs against real brokers confirm idle exit removes process, dir, and record; SessionEnd leaves live brokers untouched; and a SIGKILLed app-server child now takes its broker, record, and session dir down within ~2s. Known, intentionally accepted: status/auth reads against a stale record now fall back to spawning a direct app server (correct answer at a cost, where pre-diff they failed fast against a dead socket); a broker reused across sessions holds its spawn-time environment until it idles out; sub-second probe-vs-shutdown windows remain and now cost a single direct retry. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c756492d8a
ℹ️ 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".
| return ( | ||
| path.basename(dir).startsWith("cxc-") && | ||
| fs.realpathSync(path.dirname(dir)) === fs.realpathSync(os.tmpdir()) | ||
| ); |
There was a problem hiding this comment.
When the broker is invoked manually with --pid-file /tmp/cxc-work/broker.pid or a Unix endpoint beneath /tmp/cxc-work, this predicate treats that caller-selected directory as plugin-managed solely because of its name and parent. performShutdown then recursively deletes the entire directory, including unrelated user files, despite the preceding comment promising manual directories are preserved. Record whether the directory was actually created by the plugin, or limit manual cleanup to the socket and PID files.
Useful? React with 👍 / 👎.
A cxc- name directly under the OS temp dir does not prove the plugin created the directory: a manual serve invocation with --pid-file /tmp/cxc-work/broker.pid would have had /tmp/cxc-work recursively deleted on shutdown, user files included. spawnBrokerProcess now passes --managed-session-dir after creating the directory via mkdtemp, and the broker only removes a directory recursively when that flag was given AND the path matches the plugin's own layout; any other invocation has only the broker's pid file and socket unlinked. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Confirmed and fixed in 1cc837a. Name plus location was still an inference about ownership, not a record of it. Functionally verified both ways: a manual serve into a user-created |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1cc837aa25
ℹ️ 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".
| // The broker process is gone but left its directory behind (killed, not a | ||
| // clean exit). Remove the leftover; the PID is dead so nothing is signalled. | ||
| try { | ||
| fs.rmSync(sessionDir, { recursive: true, force: true }); |
There was a problem hiding this comment.
When a broker is manually invoked with a PID file such as /tmp/cxc-work/broker.pid and is then killed uncleanly, the next session hook sees the dead PID and recursively deletes /tmp/cxc-work, including unrelated caller files. The new --managed-session-dir check protects clean broker shutdown, but fresh evidence here is that this reaper never checks or persists that ownership signal; require a spawner-created ownership marker before recursively removing the directory.
Useful? React with 👍 / 👎.
The reaper had the same ownership gap shutdown had one commit earlier: a manual --pid-file /tmp/cxc-work/broker.pid whose broker died uncleanly would have /tmp/cxc-work recursively deleted at the next session hook. createBrokerSessionDir now writes a broker.managed marker into the mkdtemp directory, and the reaper deletes only marked directories. teardownBrokerSession unlinks the marker so its non-recursive rmdir still succeeds. With ownership persisted at creation and checked by every deleter, no code path removes a directory the plugin did not provably create. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Confirmed and fixed in 77db721. Ownership is persisted once at creation and checked by both recursive-removal paths (the broker's own shutdown via |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 77db721fbe
ℹ️ 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".
| for (const socket of sockets) { | ||
| socket.end(); | ||
| } | ||
| await appClient.close().catch(() => {}); |
There was a problem hiding this comment.
When the Codex app-server does not exit after stdin is closed and the subsequent SIGTERM, SpawnedCodexAppServerClient.close() waits on exitPromise indefinitely. Since performShutdown has already cleared broker.json and blocks here before closing the server or removing the session directory, an idle or signal-triggered shutdown can leave both processes running while the next command spawns another broker; the five-second backstop currently exists only for request-timeout shutdowns. Apply a forced-termination deadline to this close path as well.
Useful? React with 👍 / 👎.
SpawnedCodexAppServerClient.close() escalates only to SIGTERM and then awaits the child's exit unbounded, so an app server ignoring both stdin-close and SIGTERM wedged performShutdown after broker.json was already cleared, leaving the old broker and its child alive while the next command spawned a replacement. The close is now raced against a 5s deadline that force-kills the child's process tree (its exit settles the dangling close), and every shutdown path gets the 15s process-exit backstop that previously existed only for request timeouts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Confirmed and fixed. performShutdown now races appClient.close() against a 5s deadline that force-kills the child's process tree (the child's exit settles the dangling close), and every shutdown path carries a 15s process-exit backstop that previously existed only for the request-timeout path. A SIGTERM-immune app server can no longer wedge a shutdown after the record was cleared. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0fb97bbd27
ℹ️ 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".
| if (appClient.proc?.pid) { | ||
| terminateProcessTree(appClient.proc.pid); | ||
| } |
There was a problem hiding this comment.
When the app-server ignores stdin closure and SIGTERM—the exact case this deadline is intended to handle—this does not terminate it on POSIX. SpawnedCodexAppServerClient.initialize() creates a non-detached child, while terminateProcessTree() first signals the process group -pid and returns without signaling the PID when that nonexistent group yields ESRCH; moreover, its signal is still only SIGTERM, not a force-kill. The timeout promise therefore resolves, allowing the broker to exit while leaving the wedged app-server orphaned and recreating the process/log-store contention this change is meant to prevent.
Useful? React with 👍 / 👎.
terminateProcessTree was a verified no-op here on POSIX: the child is not detached, so the process-group signal hits ESRCH and nothing falls back to the pid, and the signal was only SIGTERM regardless (which the wedged child ignores by hypothesis). The deadline now sends SIGKILL to the child pid on POSIX (empirically verified against a SIGTERM-immune child) and keeps taskkill /T /F on Windows where the child is a cmd.exe wrapper needing a tree kill. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Confirmed and fixed in 8f4eec4, with the mechanics verified empirically this time: against a SIGTERM-trapping child, |
1 participant
Problem
Broker sessions leak when the plugin is used across more than one working directory. A broker is spawned per cwd (
ensureBrokerSessionkeys its state by cwd), but teardown only ran for the current cwd atSessionEnd, so brokers for every other cwd were never cleaned up, accumulating as orphanedbroker+app-serverprocesses plus leftoveros.tmpdir()/cxc-*socket directories (tens of processes / ~1 GB after a few days of multi-repo use). They also contend on the shared codex log store, which can make later app-server startups hang.Approach
Give the broker ownership of its own lifecycle instead of having sessions guess when it is safe to kill it.
app-server-broker.mjs: the broker ends itself, and only itself. It exits afterCODEX_BROKER_IDLE_MS(default 30 min, strict-integer parsing,<= 0disables) with no connections, no in-flight work, and no active stream; it also exits when its codex app-server client exits (a dead child would otherwise leave an unkillable listening zombie) and when a forwarded request exceedsCODEX_BROKER_REQUEST_TIMEOUT_MS(default 10 min, a wedged app server). On every exit path it retires its ownbroker.jsonrecord (guarded on the endpoint still matching) and removes its own session directory; recursive removal is restricted tocxc-dirs directly under the OS temp dir. All shutdown paths share one promise, so a signal cannot abort cleanup mid-flight.broker-lifecycle.mjs: dead-only GC.reapBrokerSessions()removes only directories whose recorded broker PID is provably dead. It never inspects or signals a live PID, ignores directories without a readable validbroker.pid(an empty or torn file may be a broker mid-startup), and never touches non-brokercxc-*dirs.session-lifecycle-hook.mjs: sessions leave shared state alone.SessionStart/SessionEndrun the dead-only GC.SessionEndno longer kills brokers or clears the sharedbroker.json; it interrupts the Codex turns owned by its own background jobs (mirroring/codex:cancel, bounded by a 2s budget under the hook's 5s timeout) before terminating their runners.reuseExistingBrokerprobes the recorded endpoint before use;getSessionRuntimeStatusjudges liveness by the recorded broker pid;withAppServerretries direct on any connect-phase failure, so racing an idle shutdown costs a retry rather than a failed command.Safety
Nothing ever kills or signals a live broker. A broker shared by a concurrent session is never interrupted, no PID is ever signalled by the GC, and a broker that can no longer serve (dead or wedged app server) removes itself rather than waiting to be found.
Testing
tests/broker-lifecycle.test.mjs(new) covers the reaper GC matrix (dead pid removed; live pid, missing/torn/garbage/zero pid file, and non-cxc-dirs all left alone) and pid-based runtime-status liveness; the test env caps broker idle at 30s so suite runs no longer leave broker + fake-codex pairs behind. Functional runs against real brokers verified: idle exit removes process, session dir, and record;SessionEndleaves a live shared broker and its record untouched while interrupting owned turns; a SIGKILLed app-server child takes its broker, record, and session dir down within ~2s. Full suite unchanged against the pre-change baseline.Supersedes an earlier owner-tracking revision; see the discussion for the review history (shared-broker ownership,
broker.jsonlifecycle, orphaned turns, hook time-budget, and the idle-shutdown disarm paths found by a multi-lens adversarial review).