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 👍 / 👎.
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.