fix: give the app-server broker an idle timeout, and kill the one it replaces - #580
fix: give the app-server broker an idle timeout, and kill the one it replaces#580xoonjaeho wants to merge 11 commits into
Conversation
…ad pids by exit code On Windows runCommand spawns through $SHELL, which is Git Bash when Claude Code runs from it. MSYS then rewrites taskkill's `/PID` flag into a path (`C:/Program Files/Git/PID`) and every cancel fails. Disable path conversion for that child only -- setting it on the node process itself breaks the `/c/...` paths node needs to resolve its own modules. A second defect surfaced once the flags survived: taskkill reports "no such process" with exit code 128 and a *localized* message, so the English-only regex never matched on a non-English Windows and an already-dead pid threw instead of resolving as already-stopped. Verified on ko-KR Windows: live pid -> exit 0, dead pid -> exit 128, and the English regex does not match the translated text. Fixes 3 previously-failing runtime tests (cancel x2, session-end cleanup) with no new failures.
![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: e12997f49e
ℹ️ 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".
| idleTimer = setTimeout(() => { | ||
| shutdown(server).finally(() => process.exit(0)); |
There was a problem hiding this comment.
When this timer exits the broker, shutdown() removes only the socket and pid file; the persisted broker.json for the workspace is left behind. After the shipped 30-minute idle exit, callers that read the stored session without going through ensureBrokerSession (for example setup/auth via reuseExistingBroker) try to connect to the deleted endpoint and can report an app-server auth failure until another command repairs the state. Please clear the broker session before exiting on idle, or make the reuse path validate and remove stale state.
Useful? React with 👍 / 👎.
`taskkill /PID <n> /T /F` walks the tree, and it exits non-zero when it killed the target but could not reap one descendant -- exit 255, "The operation attempted is not supported", which a console host or an already-exiting child produces. `terminateProcessTree` tolerated only exit 0 and exit 128, so that case threw, and the throw propagates out of `handleCancel`: `/codex:cancel` fails on Windows even though the process it was asked to kill is dead. The exit code cannot separate a partial kill from a real failure, and the message is localized -- keying on it is the bug the exit-128 check already exists to avoid. So probe the target with signal 0 instead: if it is gone, the kill did what the caller asked. A target that is still alive keeps throwing. Found by a test that had been dismissed as flaky: `tests/runtime.test.mjs:1740` fails ~1 run in 3 on this host, always here.
9a337af to
06f252f
Compare
`shutdown()` removes the socket and the pid file but not the workspace's `broker.json`. That was harmless while a broker only ever exited on an explicit shutdown or a signal; the idle timeout makes self-exit routine, so the stale record became a routine hazard too. `ensureBrokerSession` probes and repairs it, but the `reuseExistingBroker` callers -- `getCodexAuthStatus` and `interruptAppServerTurn` -- read the stored endpoint without probing. A record left pointing at the dead socket turns into an app-server failure for them, where no record at all makes them fall back to a direct spawn and succeed. Matched on the endpoint rather than the pid, because by the time an idle broker exits a replacement may already own the cwd, and pids get recycled. Reported on openai#580.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2dabb1cbe
ℹ️ 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".
| // files either way, so a broker left running here becomes invisible to every | ||
| // later reaper -- including the SessionEnd hook, which finds brokers through | ||
| // the state file this call just cleared. | ||
| killProcess: options.killProcess ?? terminateProcessTree |
There was a problem hiding this comment.
When the recorded broker has already exited, such as after a SIGKILL or crash before it can remove broker.json/the pid file, this default sends terminateProcessTree to whatever process currently owns the saved PID. Because that state can survive long enough for PIDs to be reused, the next ensureBrokerSession() for the workspace can kill an unrelated process tree before spawning a replacement; please validate that the PID still belongs to the saved broker (or skip killing and only clear stale state when it cannot be verified).
Useful? React with 👍 / 👎.
`d5c0b31` probed the target once, the instant taskkill returned. A process that
is already terminating is still briefly alive at that instant, so the probe said
"alive" and the call threw anyway.
That is the case `/codex:cancel` actually hits: it interrupts the turn first, the
worker starts winding down, and the taskkill that follows loses the race and
reports "Access is denied" (exit 1, localized) rather than the exit 255 the
earlier commit handled. Poll for up to a second instead of once.
This is what `tests/runtime.test.mjs:1740` had really been failing on. It was
written off as a Windows flake, then attributed to exit 255; that was one of two
modes. Captured assertion:
taskkill /PID 1052 /T /F: exit=1: 오류: 액세스가 거부되었습니다.
1 !== 0
`shutdown()` removes the socket and the pid file but not the workspace's `broker.json`. That was harmless while a broker only ever exited on an explicit shutdown or a signal; the idle timeout makes self-exit routine, so the stale record became a routine hazard too. `ensureBrokerSession` probes and repairs it, but the `reuseExistingBroker` callers -- `getCodexAuthStatus` and `interruptAppServerTurn` -- read the stored endpoint without probing. A record left pointing at the dead socket turns into an app-server failure for them, where no record at all makes them fall back to a direct spawn and succeed. Matched on the endpoint rather than the pid, because by the time an idle broker exits a replacement may already own the cwd, and pids get recycled. Reported on openai#580.
Four findings from the reviewer and two adversarial review legs on openai#577/openai#580. `terminateProcessTree` reported success after probing only the root pid, while `/T` was asked to take the tree, and callers make no second pass. `delivered` now answers only "did the target die"; the new `treeConfirmed` answers "is everything under it accounted for". Exit 0 confirms both; exit 128 means there was no process and so no tree in doubt; only the branch that tolerates a non-zero taskkill whose root turned out gone reports `treeConfirmed: false`. `handleCancel` logs that instead of reporting a clean cancellation -- without a consumer this would have been a relabel, not a fix. The stale-broker teardown killed whatever process owned a recorded pid. It now records the broker's OS process start time at spawn and requires it to still match; unverifiable identity skips the kill and only clears the stale files. `session-lifecycle-hook.mjs` killed by recorded pid with no such check at all -- the same defect on the path the first fix never reached -- and now shares the rule. The start-time probe shells out, so `runCommand` grew a `timeout` passthrough and the probe is bounded at 2 s; a timeout is just an unverifiable identity. macOS `ps -o lstart=` has only second granularity, which is recorded at the call site rather than papered over. Making the hook importable put `main()` behind an entry-point guard, so a test runs the file exactly as hooks.json does: get that guard wrong and the whole SessionEnd reaper becomes a silent no-op that nothing else would notice. Suite: 125 tests, 117 pass, 8 fail -- the 8 are the known platform and short-path failures that predate this work, and the `cancel` flake is gone.
…unted for" Review finding on this PR: after a non-zero taskkill the code probed only the root pid and then reported success, but `/T` was asked to take the tree, and callers make no second pass -- so a surviving codex or broker child kept running after a cancellation that reported clean. The two facts are now separate. `delivered` is about the target; `treeConfirmed` is about everything under it. Exit 0 confirms both. Exit 128 means there was no process, so no tree is in doubt. Only the branch that tolerates a non-zero taskkill whose root turned out to be gone -- exit 255's unreapable descendant, and exit 1's race with an already-exiting root -- cannot account for the tree, and it alone reports `treeConfirmed: false`. Windows offers no way to enumerate what survived once the root is gone, so reporting it honestly is as far as this goes. `handleCancel` now writes that to the job log instead of reporting a clean cancellation. Without a consumer this would have been a relabel, not a fix. Also adds a `timeout` passthrough to `runCommand`, which had none.
`shutdown()` removes the socket and the pid file but not the workspace's `broker.json`. That was harmless while a broker only ever exited on an explicit shutdown or a signal; the idle timeout makes self-exit routine, so the stale record became a routine hazard too. `ensureBrokerSession` probes and repairs it, but the `reuseExistingBroker` callers -- `getCodexAuthStatus` and `interruptAppServerTurn` -- read the stored endpoint without probing. A record left pointing at the dead socket turns into an app-server failure for them, where no record at all makes them fall back to a direct spawn and succeed. Matched on the endpoint rather than the pid, because by the time an idle broker exits a replacement may already own the cwd, and pids get recycled. Reported on openai#580.
e2dabb1 to
0073980
Compare
|
Both findings on this PR are fixed. Clear broker state on idle self-shutdown — real, and a regression this PR introduced rather than a pre-existing one: Validate stale broker pid before killing (P1) — also real, and the more serious of the two. Fixed in Two things that came out of fixing it, both worth flagging:
Known residual, recorded at the call site rather than papered over: macOS |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 007398046a
ℹ️ 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".
| pid: child.pid ?? null, | ||
| killProcess: options.killProcess ?? null | ||
| // We spawned this one and it never came up, so it is unambiguously ours to kill. | ||
| killProcess: terminateBrokerProcess |
There was a problem hiding this comment.
When a newly spawned broker exits before opening the endpoint (for example, because codex app-server fails during startup), waitForBrokerEndpoint can still wait up to the timeout before this path unconditionally signals child.pid. Under PID reuse during that window, terminateProcessTree can kill an unrelated process tree; this is a new failure path separate from the persisted stale-state check above, so please re-read/compare the recorded start time immediately before killing or skip the kill once the child is known to have exited.
Useful? React with 👍 / 👎.
A root taskkill cannot find says nothing about the children it spawned before it died, and with the root gone `/T` has nothing left to walk from. `cancel` only reaches this branch for a job the state still calls running -- a worker that died mid-turn -- which is exactly when an orphaned `codex app-server` is left behind. The branch now reports `treeConfirmed: false`, so `handleCancel` warns instead of reporting a clean cancellation.
… one it replaces A broker is spawned detached and exits only on a `broker/shutdown` RPC or a signal. Anything that loses track of one -- a hard-killed parent, a test run with no teardown, or the stale-replacement path below -- strands it and the ~9-process codex stack under it, permanently. One host reached 2257 processes against a ~330 baseline this way. Two independent gaps, both closed here: - The broker now exits after 30 minutes with zero connected sockets. A turn holds its socket open for its whole duration, so zero sockets means nothing is in flight, and the thread itself lives in $CODEX_HOME/sessions and stays resumable -- the next caller just respawns one. - `ensureBrokerSession` now defaults `killProcess` to `terminateProcessTree` when it replaces a stale broker. It already deleted that broker's pid, log and state files unconditionally, so a survivor became invisible to every later reaper, including the SessionEnd hook, which finds brokers through the state file this path clears. The sole production caller passed no `killProcess` at all. The readiness probe that declares a broker stale also retries at 1s after the 150 ms attempt. A healthy broker answers the first probe immediately, so the retry only costs time when the broker is already in trouble -- and without it a merely busy broker on a loaded host gets replaced for missing 150 ms. Depends on the taskkill MSYS fix: without it `terminateProcessTree` silently fails under Git Bash on Windows, and the replaced broker survives.
Three dirs were in play -- the stale broker's, the unreachable endpoint's, and the replacement's -- and only the first was torn down by the code under test. A test in a leak fix does not get to leak.
`shutdown()` removes the socket and the pid file but not the workspace's `broker.json`. That was harmless while a broker only ever exited on an explicit shutdown or a signal; the idle timeout makes self-exit routine, so the stale record became a routine hazard too. `ensureBrokerSession` probes and repairs it, but the `reuseExistingBroker` callers -- `getCodexAuthStatus` and `interruptAppServerTurn` -- read the stored endpoint without probing. A record left pointing at the dead socket turns into an app-server failure for them, where no record at all makes them fall back to a direct spawn and succeed. Matched on the endpoint rather than the pid, because by the time an idle broker exits a replacement may already own the cwd, and pids get recycled. Reported on openai#580.
…own paths Review finding on this PR: when a broker dies without clearing `broker.json` -- SIGKILL, a crash -- the recorded pid can be reused, and the stale teardown sent `terminateProcessTree` to whatever process holds it now. The broker's OS process start time is recorded at spawn and must still match before anything is killed. Unverifiable identity -- no marker, an unreadable one, a mismatch -- skips the kill and only clears the stale files. A live but unreachable broker still gets killed, which is what the kill was added for. `session-lifecycle-hook.mjs` killed by recorded pid with no check at all, so the same defect survived on the path the first fix never reached. Both call sites now share `brokerProcessMatchesRecordedStart`. The probe shells out, so it is bounded at 2 s through `runCommand`'s new timeout (measured ~228 ms on Windows); a timeout is simply an unverifiable identity and takes the same path. macOS `ps -o lstart=` only has second granularity, so a pid recycled inside the same second still verifies -- recorded at the call site rather than papered over, since no portable finer field exists. Making the hook importable put `main()` behind an entry-point guard, so a test runs the file exactly as hooks.json does: get that guard wrong and the whole SessionEnd reaper becomes a silent no-op that nothing else would notice.
`waitForBrokerEndpoint` never watches the child, so a broker that dies at startup -- `codex app-server` failing to launch -- still costs the full timeout before this path fires, and node reaps the child meanwhile. Signalling the recorded pid then can hit whatever was handed that pid in between. The stale teardown already verifies identity before killing; this path never did. It now kills only while node still holds the child open, which is the one case where the pid is unambiguously ours.
0073980 to
125b019
Compare
|
The finding on You were right that this is a separate path from the stale-state check, and I had missed it: Both halves of your premise check out. The fix skips the kill unless node still holds the child open: const childStillRunning = child.exitCode === null && child.signalCode === null;I used this rather than re-reading the start time, for two reasons. It is strictly stronger — while The trade-off, stated plainly: if the child is already gone, anything it managed to spawn underneath is not reaped either. Leaking an orphan beats killing a recycled pid, so that is the side I took.
Two adjacent things I noticed while verifying, neither addressed here — say the word if you want either split out:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 125b019b27
ℹ️ 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".
| process.stderr.write(`${error instanceof Error ? error.message : String(error)}\n`); | ||
| process.exit(1); | ||
| }); | ||
| if (process.argv[1] === fileURLToPath(import.meta.url)) { |
There was a problem hiding this comment.
When the plugin root path used by CLAUDE_PLUGIN_ROOT is a symlink, Node preserves that symlink in process.argv[1] but resolves import.meta.url to the real target path, so this guard is false and the hook exits successfully without running SessionStart or SessionEnd. That leaves the session env unset and skips the broker/job cleanup for symlinked plugin installs; compare realpaths (or otherwise tolerate symlinked argv paths) before deciding the module was invoked directly.
Useful? React with 👍 / 👎.
…oked `process.argv[1]` is resolved to an absolute path but never through symlinks, while `import.meta.url` always is, so a plugin root reached through a symlink makes the guard false and skips `main()` entirely. The whole SessionStart / SessionEnd hook then becomes a no-op that exits 0, which is indistinguishable from a hook with nothing to do -- the session env stays unset and no broker or job is ever reaped. Comparing real paths fixes it; an unreadable path falls back to the literal comparison this replaces.
|
The entry-point guard finding on I reproduced the mechanism directly rather than reasoning about it. A probe script run through a directory junction: So The fix compares real paths and falls back to the literal comparison if either path is unreadable: function invokedAsEntryPoint() {
try {
return fs.realpathSync(process.argv[1] ?? "") === fs.realpathSync(fileURLToPath(import.meta.url));
} catch {
return process.argv[1] === fileURLToPath(import.meta.url);
}
}
I checked for the same pattern elsewhere in the repo — this is the only entry-point guard; |
1 participant
An app-server broker is spawned
detached+unrefand exits only on abroker/shutdownRPC or SIGTERM/SIGINT. Anything that loses track of one strands it — and the ~9-process codex stack under it — permanently. One Windows host reached 2257 processes against a ~330 baseline. Two independent causes:1. No idle timeout. Nothing reaps a broker whose parent was hard-killed. Now it exits after 30 minutes with zero connected sockets. That is safe to do: a turn holds its socket open for its whole duration, so zero sockets means nothing is in flight, and the thread itself lives in
$CODEX_HOME/sessionsand stays resumable — the next caller just respawns a broker. This is the only fix that covers every way a broker gets stranded.2. The stale-replacement path never killed.
teardownBrokerSession's kill is opt-in and the sole production caller (app-server.mjs, viaensureBrokerSession) passed nokillProcess. It deleted the broker's pid, log and state files anyway, so the survivor became invisible to every later reaper — includingsession-lifecycle-hook.mjs, which finds brokers through the state file this path had just cleared. Note that hook already passeskillProcess: terminateProcessTree: same helper, one call site wired, the other not.Observed live, unprompted, mid-investigation: four brokers alive for a single cwd while
broker.jsonnamed one. The session then ended, theSessionEndhook reaped only the named broker, and the other three stayed up with no state file at all.ensureBrokerSessionnow defaultskillProcesstoterminateProcessTreeon both teardown paths — replacing a stale broker, and cleaning up one we just spawned that never became ready.Probe change. The readiness check that declares a broker stale now retries at 1s after the 150 ms attempt. A healthy broker answers the first probe immediately, so the retry only costs time when the broker is already in trouble — and without it a merely busy broker on a loaded host gets replaced for missing 150 ms, which is how a stranded broker gets created in the first place.
Tests (
tests/broker-idle.test.mjs, three, all spawning a real broker over the fake-codex fixture):Each was checked by breaking the line it protects and confirming that test — and only that test — fails. The idle timeout is overridable through
CODEX_COMPANION_BROKER_IDLE_MSso the tests can watch a real broker exit; 30 minutes is the shipped value.Full suite on Windows: 116 tests, 107 pass, 9 fail — the 9 are pre-existing and unrelated (platform-specific: unix sockets, temp-dir layout, transfer/native-import).