fix(broker): shut down the app-server broker after an idle timeout - #680
fix(broker): shut down the app-server broker after an idle timeout#680cjsteigerwald wants to merge 4 commits into
Conversation
A broker outlives the client that spawned it. Nothing in the protocol tells it the client is gone for good, so a spawned broker stays resident forever holding its socket dir -- roughly 40MB of RSS each, until the machine reboots. This is reproducible from this repo's own test suite. A single `node --test tests/runtime.test.mjs` run leaves ~58 brokers behind: each test creates a fresh temp workspace, ensureBrokerSession spawns a broker per workspace, and nothing ever tears them down. Three suite runs on one machine left 174 idle brokers alive, all at 00:00:00 CPU with no connections, holding ~2.9GB of resident memory between them. The broker now shuts itself down after being idle for --idle-timeout milliseconds (default 30 minutes, overridable with CODEX_COMPANION_BROKER_IDLE_MS, 0 to disable). Idle means no connected sockets and nothing in flight, so an open connection -- including a long streaming turn -- always holds it open. The decision is made by the broker rather than by an external sweep because the broker is the only party that can observe whether it is serving anyone. Scanning the job registry from outside and then killing is racy by construction: a job can start between the scan and the kill. An idle exit leaves broker.json behind, which is already handled -- ensureBrokerSession probes the recorded endpoint and, when it does not answer, tears the session down and respawns. The broker deliberately does not clear broker.json itself: a replacement broker may already have rewritten that record, and clearing it would delete the live broker's endpoint. Blank and whitespace-only values fall back to the default rather than parsing as 0, so idle shutdown can only be disabled explicitly.
![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: e19c574412
ℹ️ 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".
…meout Addresses both review findings on openai#680. Stop accepting before the asynchronous app-server teardown. shutdown() closed appClient first and only then closed the server, so the endpoint kept accepting for the whole of that await. ensureBrokerSession's readiness probe could connect during that window, judge a shutting-down broker "ready" and select it, then lose the connection with no retry -- and an accepted socket could leave server.close() waiting on an already-closed app server. server.close() now runs first, which stops listening immediately and resolves once existing connections drain, so the same reordering also covers the SIGTERM and SIGINT paths. Reject idle timeouts above Node's timer range. setTimeout() overflows past 2^31-1 ms: it emits TimeoutOverflowWarning and then fires after 1ms. A user asking for a 30-day timeout would therefore get an almost immediate shutdown -- the precise opposite of the request. Values above 2147483647 ms are now rejected at startup, consistent with how non-numeric and negative values are handled. Tests: an over-range timeout is rejected, exactly 2147483647 is accepted (the guard is not off by one), and the SIGTERM case now asserts that the socket and pidfile are cleaned up, covering the reordered shutdown on the signal path.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09338b19f4
ℹ️ 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".
Addresses the third review finding on openai#680. A client can disconnect while its streaming request is still awaiting a response. The close handler clears ownership and arms the timer, but the response then assigns the already-closed socket to activeStreamSocket. The pending timer therefore finds the broker "busy", declines to re-arm, and when turn/completed later clears that stale ownership nothing schedules again -- the empty broker stays resident forever, which is the leak this PR exists to close. Two changes, closing two variants: - armIdleTimer() is now also called when ownership is released: in routeNotification on turn/completed, and on both the success and error paths of a request. The socket that owned the stream may already be gone, so no further close handler will fire to schedule it. - isIdle() no longer counts ownership held by a destroyed socket as busy. That covers the second variant, where turn/completed never arrives at all and the stale ownership would otherwise pin the broker open indefinitely. A destroyed socket has nobody listening, so there is no stream left to protect. Test: a client starts a thread, fires turn/start, and destroys the socket without reading the response. Verified non-vacuous against 09338b1, where it fails with "broker stayed resident after the client abandoned a stream".
|
@codex review All three findings from the earlier passes are addressed, but the latest commit (de754b0) has not been reviewed yet — the previous passes covered e19c574 and 09338b1. Please review the current head. Summary of what changed since your last pass:
Both are in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de754b01b7
ℹ️ 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".
Addresses the fourth review finding on openai#680, which is a defect the previous commit introduced. de754b0 taught isIdle() to ignore ownership held by a destroyed socket, but the busy guard and the notification target still tested those fields for non-null. The three checks could therefore disagree: after an abandoned streaming request assigned its destroyed socket to activeStreamSocket, the broker reported itself idle while answering every new client with BROKER_BUSY -- and because a connected client keeps sockets.size above zero, it never shut down either. A shared broker stuck in that state is worse than the leak this PR set out to fix, since callers fall back to direct app servers and the broker never recovers. Rather than teach one more predicate to tolerate a stale owner, the stale owner is now released. releaseDeadOwnership() clears activeRequestSocket, and clears activeStreamSocket together with activeStreamThreadIds, whenever the socket is destroyed. It runs wherever ownership is inspected: when arming and when the timer fires, when a client connects, before the busy guard, and before choosing a notification target. isIdle() goes back to plain null checks, so no check can disagree with another about whether the broker is in use. Test: a client abandons a streaming request, then a second client connects and issues a request, which must be served rather than rejected. Verified non-vacuous against de754b0, where the reconnecting client is refused.
|
@codex review cfc4188 is the current head and has not been reviewed yet — the previous passes covered e19c574, 09338b1 and de754b0. This round is worth a careful look because the last commit introduced the defect you caught in the previous pass. The change replaces the Specifically worth scrutinising:
|
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
Correction to my earlier test-suite claims on this PR. I reported 3 pre-existing failures on
function filterJobsForCurrentClaudeSession(jobs) {
const sessionId = getCurrentClaudeSessionId();
if (!sessionId) return jobs;
return jobs.filter((job) => job.sessionId === sessionId);
}The Unsetting the variable turns all three green, and every branch is fully green: So: there are no pre-existing failures on One thing this does suggest: since this plugin is for Claude Code, contributors are likely to run |
1 participant
Problem
app-server-broker.mjshas no idle timeout and no lifetime cap. It exits on exactly three paths — abroker/shutdownRPC,SIGTERM, orSIGINT. Nothing in the protocol tells a broker that its client is gone for good, so any broker whose client disappears without sendingbroker/shutdownstays resident until the machine reboots, holding its/tmp/cxc-*socket dir and ~40MB of RSS.This reproduces from the repo's own test suite
This is not a rare edge case —
npm testcauses it on every run.Each test in
tests/runtime.test.mjscreates a fresh temp workspace;ensureBrokerSessionspawns one broker per workspace; nothing tears them down when the test ends. A singlenode --test tests/runtime.test.mjsrun leaves ~58 brokers behind.Measured on one machine after three suite runs:
All 174 had been reparented (their spawning shells were long gone) and together held ~2.9GB of unique resident memory on a 7.8GB host, which took available memory from 5.3GB down to 2.3GB.
Fix
The broker now shuts itself down once it has been idle for
--idle-timeoutmilliseconds (default 30 minutes, overridable withCODEX_COMPANION_BROKER_IDLE_MS, explicit0disables).Idle means no connected sockets and nothing in flight (
sockets.size === 0 && activeRequestSocket === null && activeStreamSocket === null). Holding an open connection is sufficient to keep the broker alive, so a long streaming turn can never be cut off mid-flight.Why the broker decides, rather than an external sweeper
The broker is the only party that can observe whether it is currently serving anyone. Scanning a job registry from outside and then killing is racy by construction — a job can start between the scan and the kill — and on a machine running several clients concurrently, the broker processes and the
/tmp/cxc-*namespace are shared, so a pattern-based sweep cannot distinguish one client's orphan from another's live runtime. Putting the decision inside the process removes the race entirely.Why it does not clear
broker.jsonAn idle exit leaves the
broker.jsonsession record behind. That is already handled:ensureBrokerSessionprobes the recorded endpoint viaisBrokerEndpointReadyand, when it does not answer, callsteardownBrokerSession+clearBrokerSessionand spawns a replacement. The cost is a single 150ms probe on next use.The broker deliberately does not clear that record itself, even though it knows its own
--cwd: a replacement broker may already have been spawned for the same cwd and rewritten the record, and clearing it would delete the live broker's endpoint.Input handling
Blank and whitespace-only values fall back to the default rather than parsing as
0, becauseNumber(" ") === 0would otherwise silently mean "never expire" — the exact failure this change exists to prevent. Non-numeric and negative values are rejected loudly instead of quietly defaulting.Known limitation
There is a narrow residual race: a client can connect between the timer's idle re-check and
shutdown()completing. It is benign in practice, because a client that finds a dead endpoint goes through the existing teardown-and-respawn path rather than failing, but it is narrowed rather than eliminated, and I would rather state that than claim otherwise.Tests
Adds
tests/broker-idle.test.mjs(5 tests), all spawning a real broker as a subprocess against the existing fake-codex fixture:shutdown()still removes the socket and pidfile--idle-timeout 0never idles out--idle-timeoutis rejected rather than silently never expiring--idle-timeoutfalls back to the default rather than disablingVerified non-vacuous by running the new tests against the unpatched tree at
db52e28: 3 of the 5 fail without the change. The--idle-timeout 0case passes both ways by design — it is a guard against idle shutdown becoming unconditional, not coverage of the feature.Full suite: 100 passing / 0 failing of 100.
Correction: an earlier revision of this description reported 3 pre-existing failures on
main. That was wrong. They were caused byCODEX_COMPANION_SESSION_IDbeing set in my shell:filterJobsForCurrentClaudeSessionthen filters jobs tojob.sessionId === sessionId, and thestatus/resulttest fixtures are handcrafted without asessionId, so they were all filtered out. With that variable unset the suite is green. There are no pre-existing failures.Compatibility
Default-on with a 30 minute timeout. Because
ensureBrokerSessionalready respawns on a dead endpoint, the worst case for a client that returns after 30 idle minutes is one cold broker start, not a failure. SetCODEX_COMPANION_BROKER_IDLE_MS=0to restore the previous always-resident behaviour.