Release idle Codex app-server helper trees - #563
Conversation
Attributed-To: Codex interactive
Attributed-To: Codex interactive
Attributed-To: Codex interactive
Attributed-To: Codex interactive
Attributed-To: Codex interactive
… non-fatal cleanup verification, async non-blocking process cleanup, ps portability with degraded fallback, de-flaked idle-release tests Attributed-To: Codex executor lane (implementation), Claude interactive (verification, test-anchor fix) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MrRkob3GtGnbYqq6aeE5co
![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: 38529f436e
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }; | ||
| } | ||
|
|
||
| export async function terminateProcessTree(pid, options = {}) { |
There was a problem hiding this comment.
After making terminateProcessTree asynchronous, existing production callers still discard its promise (codex-companion.mjs:986, session-lifecycle-hook.mjs:65, and broker-lifecycle.mjs:176). On Unix, teardown yields while polling before it can escalate to SIGKILL, so cancellation or session shutdown can exit and remove job state while a TERM-resistant worker remains alive; additionally, the surrounding synchronous try/catch blocks no longer catch rejected cleanup promises. Update these call paths to await the teardown before reporting or completing cleanup.
Useful? React with 👍 / 👎.
| if (activeStreamSocket === socket) { | ||
| activeStreamSocket = null; | ||
| activeStreamThreadIds = null; | ||
| clearStreamState(); |
There was a problem hiding this comment.
When a client disconnects after a streaming request has returned but before turn/completed, this clears activeStreamRunning along with the socket. The broker consequently accepts another client's request during the still-running turn and schedules the child for idle termination; with a short configured idle interval—or a turn exceeding the five-minute default—the app-server is killed before completing active work. Detach only the notification socket here and retain stream ownership until completion, failure, or child exit.
Useful? React with 👍 / 👎.
| for (const unit of units) { | ||
| delivered = signalVerifiedUnit(unit, signal, processes, options.killImpl) || delivered; |
There was a problem hiding this comment.
In environments where PID 1 does not reap orphaned children, signaling every depth-sorted unit back-to-back kills detached helpers and their app-server parent without giving the parent an opportunity to reap them. The subsequent polling treats Z records as gone, returns verified: true, and leaves permanent zombie PID entries; this is reproducible in the repository's container, where both new helper-cleanup runtime tests time out and the helper PIDs remain as <defunct>. Terminate and wait for descendant units before killing the parent, or otherwise avoid claiming successful cleanup while zombies persist.
Useful? React with 👍 / 👎.
…s, keep stream ownership on socket close, reap descendants before their parent Attributed-To: Claude interactive Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MrRkob3GtGnbYqq6aeE5co
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 16547a0cc8
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| function signalTracked(tracked, signal, options) { | ||
| const processes = readUnixProcessTable(options.runCommandImpl, options); | ||
| mergeTrackedDescendants(tracked, processes, options.rootPid, options.rootIdentity); |
There was a problem hiding this comment.
When PID 1 does not reap orphans, the new two-phase cleanup still kills the parent in the first phase: signalTracked(descendants, ...) calls mergeTrackedDescendants, which always seeds rootPid and then adds that root to the supplied descendants map. buildSignalUnits consequently signals the root process group alongside its helpers, so the parent cannot reap them and the subsequent polling treats their zombie records as gone. Fresh evidence in this revision is that the attempted descendant/root phase split is neutralized by this merge; both new helper-cleanup runtime tests time out with defunct helper PIDs in the container.
Useful? React with 👍 / 👎.
| client.setExitHandler(() => { | ||
| clearStreamState(); | ||
| if (appClient === client) { | ||
| appClient = null; | ||
| } |
There was a problem hiding this comment.
When the app-server crashes after spawning an MCP helper, this exit handler only clears the client reference and allows the next request to start a replacement. Because the app-server is now launched as a detached process-group leader, its surviving helpers are reparented when it exits and are no longer descendants of the broker, so neither idle cleanup nor session teardown can discover them; they can remain alive indefinitely while replacement app-servers accumulate. The captured process group must be cleaned up on this unexpected-exit path before permitting a restart.
Useful? React with 👍 / 👎.
…kill phase, reclaim orphaned process group on unexpected child exit Attributed-To: Claude interactive Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MrRkob3GtGnbYqq6aeE5co
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58d84b1801
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const tracked = new Map(); | ||
| for (const record of collectProcessTree(pid, initialProcesses)) { | ||
| tracked.set(record.identity, record); |
There was a problem hiding this comment.
When an app-server descendant has already exited after leaving a TERM-resistant helper in the app-server's process group, that helper is reparented outside the root's process tree and is omitted from tracked. The root-phase kill(-pid, SIGTERM) still signals it, but once the tracked root exits, polling reports no live records, skips SIGKILL, and returns verified: true while the helper remains alive. Include every observed member of a signaled process group in the tracked set so resistant orphaned members are escalated and verified.
Useful? React with 👍 / 👎.
| appClientStartPromise = CodexAppServerClient.connect(cwd, { disableBroker: true }) | ||
| .then((client) => { |
There was a problem hiding this comment.
When codex app-server responds to initialize with an RPC error but remains running, CodexAppServerClient.connect() rejects before this .then() receives the client, so the broker drops its only reference without calling close(). Because the process is now detached and the broker catches the request error without exiting, every subsequent request can spawn another live app-server and helper tree. Ensure a failed connection initialization terminates its spawned client before clearing appClientStartPromise.
Useful? React with 👍 / 👎.
A TERM-resistant helper whose parent already exited is reparented out of the root's tree, so it was signaled by the group kill but never tracked: polling saw no live records, skipped SIGKILL, and reported verified while the helper was still alive. Group members observed at scan time now enter the shared tracked map, so they escalate and count as survivors. Attributed-To: Codex executor lane Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
connect() rejected before returning the client, so a detached app-server that answered initialize with an RPC error but stayed alive was never closed, and every later request could spawn another live helper tree. connect() now closes its own client on a failed initialize and relays an unverified cleanup outcome to the broker's existing refusal gate. Attributed-To: Codex executor lane Co-Authored-By: Claude Opus 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: 386411c99f
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // The child is a detached process-group leader; on an unexpected | ||
| // exit its surviving helpers reparent away from the broker, so | ||
| // reclaim the group before allowing a replacement to spawn. | ||
| appClientClosePromise = terminateProcessGroup(childPid) |
There was a problem hiding this comment.
When the app-server crashes after a helper has created its own process group, this cleanup still misses that helper: terminateProcessGroup(childPid) initially selects only processes whose PGID equals the dead app-server PID (process.mjs:520-525), while the reparented helper's PGID is its own PID. It then returns verified: true when that selection is empty, allowing a replacement to start while the helper remains alive. Fresh evidence in this revision is that the new crash handler cleans only the root group rather than the independently grouped descendants that motivated the process-tree cleanup.
Useful? React with 👍 / 👎.
1 participant
Stops Codex companion app-server helpers from accumulating after their client or Claude session goes away.
Root cause
The shared broker eagerly kept one heavyweight
codex app-serveralive for its entire session. Some app-server helpers launch in their own process groups, so parent-only cleanup could leave those detached descendants running. Socket closure also erased stream ownership, which could incorrectly make an active turn look idle.What changed
SIGTERM, escalate resistant processes toSIGKILL, and verify the tracked tree is empty.verified: falseinstead of failing the invocation; resolvepsfrom/bin/pswith aPATHfallback.taskkill /T /Fbehavior.SessionEndfor every surviving test broker so the suite cannot reuse a live broker or leave fixtures behind.Second review round (latest commit)
A cold adversarial review of this branch surfaced defects in the new code itself; the latest commit addresses them:
BROKER_BUSYforever. New regression test: client disconnects mid-turn, the turn rejects, a second client must then be served (verified to fail against the prior branch state).close()records{verified, survivors}on the client and warns on stderr instead of throwing, so a successful turn result is never discarded because post-exit cleanup could not be verified. The broker refuses to spawn a replacement child while unverified survivors are still alive (RPC error-32002) and recovers automatically once they are gone.terminateProcessTreeis async (awaited sleeps, one process-table read per poll round instead of a per-unit re-read), so the broker stays responsive during idle release.initialize()failure paths kill the direct child handle instead of signaling the process group without a verified identity.pssample.Behavior notes for reviewers
detached(own process group) so the group can be terminated as a unit. Tradeoff: if the broker is SIGKILLed, the detached child survives until its stdin closes; the graceful-path leak this PR fixes was the more common case.CODEX_COMPANION_BROKER_CHILD_IDLE_MStunes the idle window (0releases immediately; invalid values fall back to the 5-minute default).verified: truefrom Unix cleanup covers same-process-group descendants plus processes observed at scan time; a descendant that callssetsidafter the scan can escape (documented in code).Checks
npm test: 97 passed; the only 4 failures are pre-existing onmain(state-directory/environment tests unrelated to this change).PATHisolation, degraded enumeration fallback, and client disconnect during a live turn.git diff --check main...HEAD: clean.Internal tracking: CC-5219.