fix: bound the lifetime of detached brokers and task workers - #652
fix: bound the lifetime of detached brokers and task workers#652principalwater wants to merge 21 commits into
Conversation
Background task workers are spawned detached, and the broker they use outlives the client that started it. Neither has a ceiling, a parent to watch, or anything that reaps it, so a wedged one survives indefinitely — holding its `codex app-server` and every MCP server underneath it. Observed on one machine: twelve such trees still alive after two to three days, together holding 51 MCP server processes and roughly 37 GB. Adds the missing limits: - The broker shuts down after ten minutes with no connected client, using its existing `shutdown()`. This also reclaims a broker orphaned by a crashed or timed-out SessionEnd hook. - The broker bounds its own startup. Until it is listening there is no idle timer, and the spawning client stops waiting after two seconds without killing anything, so a wedged app-server start would strand the tree. - A detached worker terminates its process group once it outruns a wall-clock ceiling, recording a terminal job status first so the registry does not keep a `running` entry pointing at a dead pid. - `CodexAppServerClient.connect()` closes the client when `initialize()` fails. By then the app-server is usually already spawned, and the caller never receives the object, so nothing else can reclaim it. Durations are clamped to what `setTimeout` can hold: an operator asking for 30 days would otherwise overflow to about a millisecond and kill precisely the long-running work they meant to protect. All three limits are configurable and can be disabled with `0`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: principalwater <kuzmin.vv@yahoo.com>
![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: f7cd86a1cc
ℹ️ 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".
`0` is the documented way to switch each limit off, but `setTimeout(fn, 0)` means "next tick". The broker startup guard passed the configured value straight through, so `CODEX_BROKER_STARTUP_TIMEOUT_MS=0` terminated the process group during startup instead of removing the ceiling — every broker would die on launch and callers would fall back to direct app-server connections. Reported by the Codex review bot on openai#652. The idle and worker limits already guarded this separately. Rather than add a third guard, route all three through `armTimeout`, which returns `null` for a disabled limit, and `disarmTimeout`, which tolerates it — so the contract is stated once and cannot drift per call site. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: principalwater <kuzmin.vv@yahoo.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c24d41b6f6
ℹ️ 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".
Idle shutdown removed the socket and the pid file but left `broker.json` behind. `getSessionRuntimeStatus()` then keeps reporting a shared runtime, and `reuseExistingBroker` readers such as `getCodexAuthStatus()` load that endpoint without probing it — so a perfectly normal idle shutdown surfaced as an ENOENT/ECONNREFUSED authentication failure and made `setup` report Codex as not ready. Reported by the Codex review bot on openai#652. The broker now drops its own record as part of shutting down, but only when that record still points at its own endpoint: a newer broker may already have claimed the workspace, and this one must not delete its successor's session on the way out. Readers that load a session without probing remain a robustness gap for brokers that die without running shutdown, and are left alone here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: principalwater <kuzmin.vv@yahoo.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44e427c453
ℹ️ 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".
Clearing the session on shutdown only covers brokers that get to run their shutdown path. One killed outright — SIGKILL, a crash, a reboot — leaves its record behind, and `reuseExistingBroker` callers took that endpoint on faith. Connecting to it then surfaced ENOENT/ECONNREFUSED as a failure of whatever the caller was doing, most visibly an authentication check reporting Codex as not ready, rather than as a broker that is simply gone. The reuse path now probes the endpoint and discards the record when nothing answers, falling through to spawning as it would with no record at all. The probe is the same 150ms check `ensureBrokerSession` already applies on the path that does not reuse. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: principalwater <kuzmin.vv@yahoo.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1371af56b
ℹ️ 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".
…deleting a record Two follow-ups from the Codex review bot on openai#652. Idle shutdown left its log and session directory behind. Nothing runs teardownBrokerSession() for a broker that exits on its own timer — no session-end hook follows — and clearing the session record destroyed the only thing naming those paths, so each expiry stranded a broker.log and a non-empty cxc-* directory for good. Shutdown now runs the existing teardown with the record's own paths before dropping it, which also replaces the hand-rolled socket and pid-file removal. The reuse path deleted the record after an awaited probe without rechecking it. Another process can start a broker and replace broker.json during that await, so the deletion removed the new record instead of the stale one, leaving a healthy broker untracked — duplicates on the next command, and cleanup unable to find it. It now re-reads and only clears a record that is still the one it probed. That is the same guard already applied when the broker clears its own record on shutdown; it belonged on both sides of the await and was missing here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: principalwater <kuzmin.vv@yahoo.com>
💡 Codex Reviewcodex-plugin-cc/plugins/codex/scripts/lib/app-server.mjs Lines 353 to 357 in 3ea94be  Remove stale broker artifacts before dropping their record
When a broker is killed ungracefully, this stale-session path deletes only ℹ️ 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". |
… gone Dropping a stale record removed the only thing naming that broker's pid file, log, socket and session directory, so those became undiscoverable: every later call sees no session to hand to teardownBrokerSession. The reuse path now tears them down before clearing the record. Reported by the Codex review bot on openai#652. Gated on the process actually being gone, which the report did not ask for but the naive form needs: the readiness probe waits 150ms, and a live but busy broker can miss it. Deleting a running broker's socket would leave it unreachable while it still holds its app-server and every MCP server under it — reintroducing, from the other side, the leak this PR exists to fix. Dropping the record for an unresponsive broker is what ensureBrokerSession already does; deleting its files is not, so that part waits for proof. isProcessAlive treats EPERM as alive: a broker owned by another user still holds its socket. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: principalwater <kuzmin.vv@yahoo.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98669f6927
ℹ️ 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 previous commit skipped teardown for a live-but-unresponsive broker but still dropped its record, which orphans exactly what this PR is about: the next call starts a second broker while the first keeps its app-server and every MCP server under it, and nothing can find it to clean it up. Reported by the Codex review bot on openai#652. A failed 150ms probe is not proof of death, so nothing is removed unless the process is confirmed gone. When it is alive the record and its files are left untouched; this call simply falls through to spawning, because it could not reach it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: principalwater <kuzmin.vv@yahoo.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69a72b7225
ℹ️ 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".
Closing a client whose initialization failed before a transport existed hung forever. `close()` ends by awaiting `exitPromise`, which only a live socket or child process resolves, and a malformed endpoint rejects while being parsed — before the socket is created. The cleanup added earlier in this PR then turned a configuration error into a hang: `connect()` simply never settled. Reported by the Codex review bot on openai#652. Both clients now settle the exit state when there is nothing to close, so the caller gets the original error. Covered by a test that fails with a five-second deadline rather than stalling the run; removing the fix makes it hang, which is how it was verified. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: principalwater <kuzmin.vv@yahoo.com>
ensureBrokerSession tore down the existing broker's socket, pid file, log and directory whenever the readiness probe failed, then spawned a replacement. On the normal path `killProcess` is null, so this never stopped the old process — it only removed the files that made it reachable and findable. A broker that was merely busy therefore became an untracked, unreachable process still holding its app-server and every MCP server underneath, with a duplicate started alongside it. This is the same defect the review bot found in the reuse path in this PR; found by checking whether the neighbouring function made the same assumption, which it did. It predates this PR — but only just became safe to fix, because a broker left alone now reclaims itself through the idle shutdown added here. Teardown now waits for proof the process is gone. A live one is left exactly as it is: once the replacement takes over it has no clients, so its own idle timer retires it and its files. Killing the recorded pid instead was considered and rejected: pids get reused, and terminating a process group on the strength of a stale record risks someone else's processes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: principalwater <kuzmin.vv@yahoo.com>
|
Pushed one more fix that no review comment asked for, because it is the same defect the bot found, in the function next door. What
So a broker that was merely busy became an untracked, unreachable process still holding its How it turned upWhile addressing "Recheck the broker record before deleting it" I added a liveness guard to the reuse path, then checked whether anything else made the same assumption. Why it is fixed here and not left aloneI had held off on it earlier, because neither obvious option was safe on its own:
The second option is safe now, and only because of this PR: a broker with no clients retires itself through the idle shutdown added here, tearing down its own socket, pid file and directory on the way out. Once the replacement takes over, the old one has no clients. So teardown now waits for proof the process is gone, and a live one is left exactly as it is. Coverage
Verified by reverting the guard: the first case then fails with One residual, deliberately left: a broker that is replaced while still alive keeps its |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ef1c57856
ℹ️ 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".
A broker that is still alive when it misses the readiness probe gets superseded: a replacement is spawned and broker.json comes to name that one instead. When the old broker later retires on its idle timer, the record is no longer its own, so the shutdown path passed no logFile and no sessionDir and left broker.log and a non-empty cxc-* directory behind for good. Reported by the Codex review bot on openai#652, which correctly pushed back on leaving this as a known residual. The broker is now told its log path at startup, alongside the pid file it already received, so its own artifacts no longer depend on a shared record that may have moved on. Teardown of those is unconditional — they are ours either way — while the record is still only cleared when it points at us, because that one genuinely belongs to whoever succeeded us. Covered by asserting spawnBrokerProcess passes --log-file. An older broker started without it still shuts down cleanly; the log is simply skipped, as before. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: principalwater <kuzmin.vv@yahoo.com>
💡 Codex Review Interrupt brokered work before terminating the worker
When a background task reaches its TTL while using the normal shared runtime, this only signals the worker's process group; ℹ️ 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". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: efe1cf2e23
ℹ️ 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".
…-turn A client that disconnects while turn/start, review/start or thread/compact/start is still awaiting the app-server left the broker permanently busy. The close handler cleared ownership and armed the idle timer, but the request continuation then assigned the now-closed socket to activeStreamSocket, so isBrokerBusy() stayed true: the armed timer refused to reschedule itself, and turn/completed released ownership without arming a new one. A wedged turn never released it at all. Either way the broker and its app-server and MCP tree stayed up for good — the leak this PR exists to close, reached through the very timer meant to close it. Reported by the Codex review bot on openai#652. Both halves are fixed: the stream is only handed to a socket still in the connected set, and releasing ownership on turn/completed arms the timer, since that can be the moment the broker becomes idle and nothing fires afterwards. armIdleShutdown now closes over the server rather than taking it as an argument, so it can be called from the notification handler. Not covered by tests: exercising this needs a live broker and app-server, and there is no harness for that here. The other ownership-release points were audited by hand — they are reached only while the socket is still connected, where the socket set already keeps the broker busy. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: principalwater <kuzmin.vv@yahoo.com>
A full review of this branch turned up several more instances of the family it is about — cleanup and timers that themselves leak. Including one this branch introduced. Abandoned turns no longer follow whoever connects next. Declining to hand a stream to a departed client fixed the broker staying busy forever, but left the turn running: routing follows current ownership, so its notifications reached the next client instead. Such a turn is now interrupted and its notifications are dropped. A turn that completes before its stream is handed over no longer takes ownership. The app-server can deliver a response and its turn/completed in one chunk, which lands before the request continuation runs; ownership was then taken for a turn already over and held until the client disconnected. Requests are rejected once the transport has reported its exit. A socket can die between a successful connect and the next request, and anything registered afterwards waited on an exit that had already been reported. Shutdown runs once and cannot outlive a wedged app-server. Every entry point could re-enter it, sockets closing during it re-armed the idle timer, and awaiting appClient.close() unconditionally let precisely the wedged backend this guard exists for survive the guard. It is now a single pass with a grace period. Fatal errors after the app-server is spawned take the tree with them. A bare `null` is valid JSON and threw while being dereferenced, killing this detached process and stranding its tree; listen failures never reached main()'s handler at all. The worker TTL writes a current job record rather than the day-old snapshot the timer closed over, and each best-effort write is isolated so a missing log cannot skip the terminal status. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: principalwater <kuzmin.vv@yahoo.com>
|
I put this branch through a full review pass, which turned up more of the family it is about — cleanup and timers that leak on their own — including one this branch introduced. The tractable ones are fixed in the commit above. The rest are structural, predate this branch, and I have deliberately left them alone; they are listed here so they are not lost. Fixed in the commit aboveAn abandoned turn followed whoever connected next. This one was mine. Declining to hand a stream to a client that disconnected mid-turn stopped the broker from staying busy forever, but the turn itself kept running, and routing follows current ownership rather than the turn that produced the notifications — so they were delivered to the next client to connect. Such a turn is now interrupted and its notifications dropped. A turn that completed before its stream was handed over still took ownership. The app-server can put a response and its Requests registered after the transport reported its exit never settled. A socket can die between a successful connect and the next request; anything registered afterwards waited for an exit that had already been delivered. Shutdown could re-enter itself, be re-armed while running, and wait forever on a wedged app-server — which is to say the exact backend this guard exists to reclaim could outlive the guard. It is now a single pass with a grace period. Fatal errors after the app-server was spawned left the tree behind. A bare The worker TTL wrote the day-old snapshot the timer closed over, and one failed log write skipped the terminal status with it. Left for maintainersThese need decisions about the broker's concurrency model rather than another guard, and I did not want to make them inside a PR about lifetime limits. Session state has no locking or identity.
Idle is measured by connected sockets, not activity. A client that connects and then goes quiet holds the whole tree indefinitely, because a socket in the set counts as busy before it has issued anything. Ownership is a single socket pointer. Two concurrent requests on one socket, or a failed non-streaming request, can release ownership belonging to work still in flight. An operation token would model this properly. The worker TTL does not reach the shared broker, contrary to what its comment implies: the broker runs in its own process group, so A worker can read its job record before the parent writes it, exit, and leave a Note on coverageNone of the runtime races here have automated coverage, and I have not added any: exercising them needs a live broker and app-server, and there is no harness for that in this repo. What is covered is the pure logic — the limit parsing and clamping, the process-liveness predicate, connect-failure cleanup, and the reclaim guard around a live versus dead broker. Everything else was reasoned through by hand, and I would treat the structural list above as unproven until something can actually drive a broker under concurrency. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79790e2d95
ℹ️ 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 grace period added in the previous commit stopped shutdown waiting forever on the app-server, but nothing acted on it expiring. Two ways out remained. Nothing terminated the tree once the grace lapsed. On POSIX the client's own fallback signals the app-server pid alone, so its MCP servers outlive the broker — the lifetime bound this branch is for, defeated at the last step. The broker now takes its process group down when the backend never acknowledged the close, after the artifacts and the record are cleaned up, since that call ends this process too. Waiting for client sockets was still unbounded. `end()` only half-closes, so a client holding its read side open keeps `server.close()` pending for as long as it likes, hanging SIGTERM and broker/shutdown alike. That wait is now bounded too, and whatever is left is destroyed. Both reported by the Codex review bot on openai#652, on the commit that introduced the grace period. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: principalwater <kuzmin.vv@yahoo.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a397c88d57
ℹ️ 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".
…eardown Comparing this branch against main on the runtime suite showed one failure main does not have: `setup and status honor --cwd when reading shared session runtime`. The test records a broker on an endpoint that deliberately does not answer and expects `status` and `setup` to report it as shared — these commands report recorded configuration, not reachability. Probing before reuse was right; deleting the record was not, and was never what the report asked for. The complaint was that an unreachable endpoint surfaced as an authentication failure rather than as a missing broker, and falling through to spawning fixes that on its own. Reclaiming a dead broker's files stays in ensureBrokerSession, the path whose job is to replace it, where the liveness guard already lives. Also narrows two edges found while re-reading this branch: - The fatal-error teardown fired even when nothing had been spawned yet, so a bad argument would signal a process group that does not exist. It is now tied to the app-server actually being up. - The set remembering turns that completed before their handoff is bounded. It is only ever drained by a handoff racing it, and one that never arrives would have left an entry for the life of the broker. Runtime suite now matches main exactly: 58 passing, the same 3 failing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: principalwater <kuzmin.vv@yahoo.com>
A streaming client can disconnect while its turn/completed is already in flight. Both conditions then held, and the departed-client branch won: the thread was marked abandoned after it had finished. Interrupting a finished turn need not produce another completion, so the mark was permanent, and every later turn on that thread had its notifications discarded — including the completion its caller was waiting for. Reported by the Codex review bot on openai#652. A turn that has already finished is now neither handed over nor abandoned; there is nothing left to do with it. The abandoned set is bounded as well, so a missing completion can no longer strand an entry indefinitely. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: principalwater <kuzmin.vv@yahoo.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9e306501d
ℹ️ 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 mirror of the previous fix. A client that disconnects after handoff but before its turn finishes leaves no stream owner, so that turn's completion also satisfied the "completed before handoff" condition — though no start was waiting for it. The entry then stayed, and the next client to use the same persistent thread consumed it: its own turn was treated as already finished, it was never made stream owner, its notifications were dropped, and it hung. Reported by the Codex review bot on openai#652. A completion now counts only while that thread's streaming start is actually in flight. The thread is marked before the await and cleared in a finally, so a completion belonging to some earlier turn cannot be mistaken for one racing a handoff. Runtime suite still matches main: 58 passing, the same 3 failing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: principalwater <kuzmin.vv@yahoo.com>
|
This is where I am stopping, and I think the reason is worth stating plainly, because it is also the argument for where the remaining work belongs. Where the branch standsIt closes the leak it set out to close — the detached worker and the broker both have a bounded lifetime now, and the shutdown path reclaims its own process, artifacts and record — plus every hang and orphan path reachable without changing how the broker models ownership. The runtime suite matches Why I am not continuingThe review bot has now found a defect in freshly written code five rounds running, and the last two were mirror images of each other:
Both are the same shape. Ownership is a single socket pointer, and completion is matched against whatever that pointer happens to hold, so every guard that fixes one direction leaves the symmetric case open in the other. I can keep closing them one at a time, but the supply is a property of the model, not of my attention. That is the argument for the structural list in my earlier comment rather than a caveat about it. An operation token — ownership belonging to a turn rather than to whichever socket is current — collapses most of that list at once, and it is the piece I deliberately did not build here: it changes whether the broker is single-client by design, and that is your call, not something a lifetime-limits PR should decide. What that leaves youEverything in this branch is a bounded guard around behaviour you already have. Nothing changes the broker's concurrency model, the shared-broker semantics, or what Two things worth knowing before merging: Runtime races here have no automated coverage and I did not add any: exercising them needs a live broker against an app-server, and there is no harness for that. The three failures shared with |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da91429931
ℹ️ 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 TTL sent SIGTERM to its process group and exited immediately. terminateProcessTree does not escalate on POSIX, so a descendant that traps or ignores SIGTERM simply kept running — and with the worker gone, nothing was left to escalate. The headline guarantee of this branch did not hold for precisely the runaway tree it exists to bound. Reported by the Codex review bot on openai#652. The worker now stops dying on its own group signal, gives the tree a grace period to leave on its own, and SIGKILLs whatever remains. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: principalwater <kuzmin.vv@yahoo.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f8d7c5a1c
ℹ️ 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".
…settles Two ways the guards in this branch were not doing what they claimed. The interrupt for an abandoned turn carried only the thread id. turn/interrupt also requires the turn id — interruptAppServerTurn in lib/codex.mjs shows the shape — so the request was rejected and the catch swallowed it. The turn we meant to stop kept running while its thread stayed marked abandoned, which also meant the next client on that thread lost its notifications until the old turn finished. Releasing request ownership did not rearm the idle timer. The close handler runs while the request still owns the broker, so it finds it busy and arms nothing; the continuation then clears that ownership, and for a non-streaming request no notification follows to arm it later. That release is the last event the broker ever sees, and it stayed alive indefinitely. Both reported by the Codex review bot on openai#652. The second contradicts a hand-audit two commits ago that concluded these paths needed no rearm: the defect is in the ordering between the close handler and the await continuation, which checking each site in isolation does not reveal. Runtime suite still matches main: 58 passing, the same 3 failing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: principalwater <kuzmin.vv@yahoo.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17649c2564
ℹ️ 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 review bot has been finding the same two shapes repeatedly because each round fixed the line it named rather than the class. Both are now handled in one place each, so there is no second site left to find. Termination without escalation. terminateProcessTree only sends SIGTERM, so a descendant that traps it survives — and when the tree is our own, we die with the signal and nothing remains to escalate. All four self-terminations (the broker's startup bound, its shutdown fallback, its fatal-error path, and the worker TTL) now go through terminateProcessTreeAndExit, which survives its own group signal, waits out a grace period and SIGKILLs the remainder. Marking a thread that will never report. A detached review streams on the review thread it creates, not the source thread it was launched from. Both need routing, but abandoning both left the source marked for good, so any later turn resumed on it had its notifications discarded. Routing and abandonment are now separate ideas: turnThreadId names the one thread the turn actually runs on, and abandonStream takes that single thread, which makes marking a silent thread structurally impossible rather than merely avoided. Reported by the Codex review bot on openai#652 — the shutdown escalation and the review source thread respectively; the other three termination sites were found by looking for the rest of each class. Runtime suite still matches main: 58 passing, the same 3 failing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: principalwater <kuzmin.vv@yahoo.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b49b12bee
ℹ️ 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".
Tracking "a start is awaiting handoff" by thread id cannot work for a detached review: it streams on a thread the server creates and names only in its response, so at the moment the mark went on, that id did not exist yet. Its completion was therefore never recognised as racing the handoff, and the continuation either took ownership of a turn already over — blocking other clients — or, if the client had gone, marked the review thread abandoned after it had completed, discarding every later notification on it. Reported by the Codex review bot on openai#652. Completions are now recorded for the duration of a streaming start and the record is cleared when that start settles, so the continuation can match them against the threads the response actually names. Nothing can outlive the start it raced, which is what the previous design needed a bounded set and consume-on-read to approximate; both are gone. Runtime suite still matches main: 58 passing, the same 3 failing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: principalwater <kuzmin.vv@yahoo.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3180be7c14
ℹ️ 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".
Both are costs of surviving our own signal in order to escalate. A worker that outlives its SIGTERM keeps running for the grace period, so a job finishing inside that window recorded a success over the TTL failure. The job then looked completed although its tree was killed moments later. The expiry is now written again as the last act before SIGKILL, through a beforeKill hook on the shared helper so this stays one mechanism rather than a special case in the worker. Cleanup after a failed connect waited on the transport reporting its exit, with no bound. That is fine when no transport exists — the earlier fix settles that case — but an app-server that answers with an error and then ignores EOF and SIGTERM left connect() waiting forever and swallowed the original failure. The wait is bounded now, and a child still running afterwards is killed directly by handle: it is spawned without detach, so it shares this process group and a group signal would be aimed at us. Both reported by the Codex review bot on openai#652. Runtime suite still matches main: 58 passing, the same 3 failing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: principalwater <kuzmin.vv@yahoo.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: efdfdeb0b8
ℹ️ 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 (client.proc && client.proc.exitCode === null && !client.proc.killed) { | ||
| try { | ||
| client.proc.kill("SIGKILL"); |
There was a problem hiding this comment.
When a directly spawned app-server ignores SIGTERM after initialization fails, SpawnedCodexAppServerClient.close() has already called proc.kill("SIGTERM"), which sets proc.killed when the signal is sent—not when the process exits. The new !client.proc.killed gate is fresh evidence that the five-second fallback consequently skips SIGKILL, after which the caller exits and can leave the app-server and its MCP descendants running; determine liveness from process exit and escalate the whole tree instead.
Useful? React with 👍 / 👎.
| terminateProcessTreeAndExit(process.pid, { | ||
| graceMs: WORKER_TERMINATION_GRACE_MS, | ||
| beforeKill: recordExpiry |
There was a problem hiding this comment.
When the task settles during the five-second post-TTL grace period, runTrackedJob() can overwrite the recorded timeout with a successful terminal result, and status --wait may observe that success and return before beforeKill changes it back to failed. The newly added delayed beforeKill: recordExpiry is fresh evidence that the earlier overwrite is only repaired at kill time; latch the expiry immediately so normal completion bookkeeping cannot publish a contradictory result during the grace window.
Useful? React with 👍 / 👎.
| if (loadBrokerSession(cwd)?.endpoint === endpoint) { | ||
| clearBrokerSession(cwd); |
There was a problem hiding this comment.
When an old broker shuts down while a replacement broker is saving broker.json, the endpoint comparison and deletion are separate filesystem operations: the process can read its own old endpoint, be preempted while the replacement writes its record, and then unlink the replacement's record. The newly added compare-then-clear pair is fresh evidence that the concurrent replacement race remains possible, leaving the healthy replacement untracked and allowing later callers to start another broker; serialize or atomically condition this deletion on the record still matching.
Useful? React with 👍 / 👎.
1 participant
Problem
Background task workers are spawned fully detached (
detached: true,stdio: "ignore",child.unref()), and the per-workspace broker they use outlives the client that started it. That is intentional — a background task should survive the session that queued it — but neither has a wall-clock ceiling, a parent to watch, or anything that reaps it. A wedged one therefore survives indefinitely, holding itscodex app-serverand every MCP server underneath it.Found while investigating memory pressure on a 16 GB machine:
The MCP servers in question happened to be memory servers that load an ONNX model, so each cost several hundred MB — but the leak itself is independent of what the MCP servers do.
Verified along the way that this is not an fd-inheritance problem:
lsofshows exactly one holder of each server's stdin write end, soStdio::piped()hygiene is correct. The servers stay alive simply because the client that owns them never exits.Changes
Broker idle shutdown. After ten minutes with no connected client and no in-flight request or stream, the broker shuts itself down through its existing
shutdown(). This also reclaims a broker orphaned by a crashed or timed-out SessionEnd hook, and the losing side of a concurrentensureBrokerSessionrace.Broker shutdown is now atomic with respect to new clients.
shutdown()previously awaitedappClient.close()beforeserver.close(), so a client could connect in that window and reach a broker whose backend was already going away. The listener now closes and ashuttingDownflag is set before the firstawait, and connections arriving during shutdown are refused so the caller falls back to starting its own broker.Broker startup is bounded. Until the broker is listening there is no idle timer, and the spawning client stops waiting after two seconds without killing anything (
killProcessisnullon the normal path). A wedged app-server or MCP startup would strand the whole tree permanently. The broker now bounds its own startup and takes its process group down if it overruns.Detached workers get a wall-clock ceiling. On expiry the worker terminates its process group via the existing
terminateProcessTree, after recording a terminal job status — otherwise the registry keeps arunningentry pointing at a dead pid, which is what makes these leaks invisible instatus. The default is deliberately long: this is a runaway guard, not a task deadline.A parent watcher was considered and rejected: the worker's immediate parent exits right after enqueue by design, so watching it would kill healthy background tasks.
CodexAppServerClient.connect()closes on failure. Ifinitialize()throws, the app-server has usually already been spawned and the caller never receives the object, so nothing can close it.Configuration
CODEX_BROKER_IDLE_SHUTDOWN_MSCODEX_BROKER_STARTUP_TIMEOUT_MSCODEX_TASK_WORKER_TTL_MSAll three accept
0to disable. Values are clamped to whatsetTimeoutcan actually hold — an operator asking for 30 days would otherwise overflow to about a millisecond and kill precisely the long-running work they meant to protect.Tests
tests/lifecycle-limits.test.mjscovers the defaults, overrides, disabling, rejection of unusable input, and the clamp.node --test tests/lifecycle-limits.test.mjspasses 8/8.One pre-existing failure in
tests/state.test.mjs(resolveStateDir uses a temp-backed per-workspace directory) reproduces on a clean checkout in this environment and is unrelated to these changes.Not covered by automated tests, and worth a maintainer's eye: reconnect-during-shutdown, wedged broker startup, and real process-group termination all need integration coverage that does not exist here yet.