Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 599b7cb45f
ℹ️ 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".
|
|
||
| export async function ensureBrokerSession(cwd, options = {}) { | ||
| const existing = loadBrokerSession(cwd); | ||
| const killProcess = options.killProcess ?? terminateProcessTree; |
There was a problem hiding this comment.
When a broker exits without normal session-end cleanup and the OS later reuses the PID retained in broker.json, the failed endpoint check reaches this default and terminateProcessTree kills that PID without confirming it still belongs to the broker. On Windows this can terminate an unrelated process tree, while on Unix a reused process-group ID can terminate an unrelated group. Preserve and verify process identity/start time before killing a persisted PID, or limit unconditional termination to the newly spawned child handle.
Useful? React with 👍 / 👎.
ff71e37 to
599b7cb
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff71e37813
ℹ️ 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".
| status: result.status, | ||
| failureMessage: result.error?.message ?? result.stderr | ||
| }); | ||
| const failureMessage = result.error?.message ?? result.stderr ?? parsed.parseError ?? ""; |
There was a problem hiding this comment.
When an adversarial review returns malformed or no structured output without an app-server error notification, cleanCodexStderr() supplies ""; because nullish coalescing treats that as present, parsed.parseError is never selected. The failed job consequently persists a null errorMessage, and its summary falls back to the raw output's first line—often just {—instead of the actionable parse failure. Use the first non-empty diagnostic rather than the first non-nullish value.
Useful? React with 👍 / 👎.
Audited against open upstream reports on openai/codex-plugin-cc; each defect was reproduced or verified against this fork and has a regression test in tests/substrate.test.mjs (dedicated fake app-server). - openai#302 unbounded waits: every RPC has a bounded wall-clock timeout (120s default, command/exec aware, CODEX_COMPANION_RPC_TIMEOUT_MS, 0 disables), and requests on a dead connection fail immediately. The turn watchdog does not use a silence timeout (legitimate turns are silent for long periods): it fails fast when the connection closes, and after quiet periods asks the app-server via thread/read whether the thread is still active, recovering a turn whose completion event was lost and failing only an app-server that stops answering. - openai#453 zombie broker: the broker exits when its app-server child dies, so the next caller starts a healthy broker instead of wedging. - openai#706/openai#707 retained subscriptions: the broker tracks per-socket thread ownership (subagents inherit their parent's owners) and sends thread/unsubscribe when the last owner disconnects; requests for a thread wait (bounded) for its in-flight unsubscribe. Simpler than openai#707 because this broker already serializes active requests and streams. - openai#762 + openai#768 together: broker acquisition is serialized; a broker whose process is alive is never torn down or killed because it missed a probe (the caller uses a private app-server); metadata is cleared only when the process is provably gone; only a just-spawned broker that never became ready is killed, identity-checked. SessionEnd's kill is identity-checked. - openai#574 RC3: non-retryable errors fail a turn even when completion is inferred. The upstream fix (any error fails) is wrong: error notifications carry willRetry, and retried turns can succeed. - openai#775: fileChange start events without a change list no longer throw. - openai#740 reproduced with real Codex 0.144.1: live thread/resume ignores the requested sandbox in both directions (write-capable threads stayed writable after a read-only resume), and a per-turn override persists to later turns. Every turn now sends an explicit sandboxPolicy. Also fixes a same-process withFileLockAsync bug (a second async holder treated the first as stale) and makes the test harness stop brokers when a test process is killed by a signal. Validation: npm test 156/156, tsc clean, no stray processes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 participant
Summary
Fixes #753: the only production caller of
ensureBrokerSession()passed nokillProcess, soteardownBrokerSession()unlinked the pid file, log file, and session dir without terminating anything. The broker process survived, and deletingpidFiledestroyed the last handle anyone had on it — a permanently unreachable orphan.Changes
plugins/codex/scripts/lib/app-server.mjs— passkillProcess: terminateProcessTreeat the only production call site (CodexAppServerClient.connect), matching whatsession-lifecycle-hook.mjsalready does.plugins/codex/scripts/lib/broker-lifecycle.mjs— additionally defaultkillProcesstoterminateProcessTreeinsideensureBrokerSession()so no future caller can omit it. This is the "better still" option from the issue:teardownBrokerSession()already ignores a missing process, so the default is safe.This covers both teardown branches that previously leaked:
existing-but-unresponsive broker branch (150ms endpoint check).Verification
node --test tests/process.test.mjs tests/commands.test.mjs— 10/10 pass.broker-lifecycle.mjsnow importsterminateProcessTreefrom./process.mjs, which has no reverse dependency.