fix: make app-server connection loss terminal - #685
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: a01a9f8bcf
ℹ️ 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".
| assert.ok(firstSession); | ||
| const firstEndpoint = parseBrokerEndpoint(firstSession.endpoint); | ||
| await waitFor( | ||
| () => !processIsAlive(firstSession.pid) && (firstEndpoint.kind !== "unix" || !fs.existsSync(firstEndpoint.path)), |
There was a problem hiding this comment.
On Linux containers where PID 1 does not reap orphaned children, the detached broker becomes a zombie after exiting, so process.kill(pid, 0) continues to succeed indefinitely even though the endpoint has been removed. This makes the new lifecycle test consistently time out at this condition; npm test fails in this environment. Check the process state or rely on endpoint/session cleanup rather than requiring processIsAlive() to return false.
Useful? React with 👍 / 👎.
| env: this.options.env ?? process.env, | ||
| stdio: ["pipe", "pipe", "pipe"], | ||
| shell: process.platform === "win32" ? (process.env.SHELL || true) : false, | ||
| detached: process.platform !== "win32", |
There was a problem hiding this comment.
On POSIX, the broker is already detached into its own process group, and setting detached: true again gives the app-server child a separate group. If the broker is forcibly terminated—for example by failed-session teardown or an external SIGKILL—terminateProcessTree(brokerPid) no longer reaches Codex, leaving the app server and any descendants running without a broker; direct callers interrupted by terminal SIGINT can leak it for the same reason. Preserve group-level ownership so terminating the owning process tree also terminates Codex.
Useful? React with 👍 / 👎.
| this.proc.stdout.on("end", () => { | ||
| if (!this.terminalCause) { | ||
| this.transitionToTerminal(createProtocolError("codex app-server stdout closed before the connection ended.")); | ||
| } |
There was a problem hiding this comment.
When the spawned Codex process exits with a startup, authentication, or configuration error, its stdout end event normally arrives before the child exit event. This handler therefore locks in the generic stdout-closed terminal cause, and the later exit handler cannot replace it with the exit code and captured stderr; a direct reproduction with a child exiting nonzero reports only codex app-server stdout closed before the connection ended. Defer the EOF diagnosis until the child exit status is available so users retain the actionable failure.
Useful? React with 👍 / 👎.
1 participant
Summary
Motivation
The app-server client previously tracked logical protocol failure separately from process closure. Several connection-loss paths could therefore leave later requests or turn-completion waits unsettled, and initialization failure could return without reaping the owned launcher tree.
This change gives the connection one write-once terminal cause and routes every relevant protocol or process failure through it. Initialization and teardown have bounded lifecycle deadlines; normal Codex turns remain unbounded.
Testing
npm test(106 tests)npm run build