Repository navigation
fix(app-server): bound request() so a wedged spawned app-server can't hang the client (closes #31) - #32
Merged
Conversation
… hang the client (closes #31) SpawnedCodexAppServerClient.initialize had the same unbounded handshake shape PR #30 fixed only on the broker path: request("initialize") had no deadline, so a spawned codex app-server that accepted stdin but never answered hung the client forever. RED-first: a stalled-initialize fake-codex behavior reproduces the hang (connect() never settles within 2s). Deeper fix (as promised in #31 / upstream openai#509), not a third copy of the bounded-handshake pattern: - AppServerClientBase.request(method, params, options) gains an optional timeoutMs (+ onTimeout for transport cleanup). Optional/undefined-default, so all existing 2-arg call sites are unchanged. Timer is unref'd, cleared in handleLine/handleExit. Timeout rejects with BROKER_HANDSHAKE_TIMEOUT_CODE and removes the pending entry; a late answer for that id is a no-op in handleLine. - SpawnedCodexAppServerClient.initialize now passes timeoutMs (10s default, spawnedInitializeTimeoutMs override), and kills the child on handshake failure so a timeout doesn't orphan the spawned process. - BrokerCodexAppServerClient.initialize drops its inline Promise.race and uses request(timeoutMs, onTimeout=socket.destroy) — one mechanism, both transports. - CodexAppServerClient.connect tags broker-transport init failures with brokerFatal; withAppServer's fallback OR-chain collapses to (broker-busy) || (brokerFatal). Verified: RED→GREEN (304ms, EBROKERTIMEOUT); broker suite 11/11 green; npm test unchanged (4 pre-existing failures). Co-Authored-By: Claude <[email protected]>
…e-review critical) Codex adversarial review caught that collapsing withAppServer's fallback onto error.transport broke the #29 path: broker/busy (-32001) arrives AFTER a successful handshake, while the error is created by createProtocolError with rpcCode but NO transport (transport is tagged only on handshake failure in connect()'s catch). So every overlapping /codex:* command failed for the duration of a long-running broker turn instead of falling back to direct. Restore client?.transport === "broker" for the busy branch (post-handshake, client is assigned); keep error.brokerFatal for handshake failures (client is null, tagged in connect). Adds spawnBusyBroker helper + an end-to-end test that drives a successful initialize, then -32001 on the first RPC, and verifies the operation completes via the direct fallback. Co-Authored-By: Claude <[email protected]>
…-review critical) Codex adversarial review caught that `error.code = REQUEST_TIMEOUT_CODE` on a plain `new Error(...)` fails the repo's mandatory `npm run build` (TS2339: Property 'code' does not exist on type 'Error'). CI runs this build, so the head could not pass its shipping gate. The same pre-existing breakage in BrokerCodexAppServerClient's handshakeError (landed in #30) is fixed too. Cast the errors to ProtocolError (extended to include `code?: string`), the same pattern createProtocolError already uses. Co-Authored-By: Claude <[email protected]>
Owner
Author
📋 Review summary — all cycles
Totals: 2 FIX (both resolved), 0 SKIP, 0 UNVERIFIED. |
axisrow
added a commit
that referenced
this pull request
Jul 21, 2026
Ships accumulated fixes since fork.4: - #29 broker-busy hang (PR #30): bound broker handshake - #31 spawned-app-server handshake (PR #32): bound request() primitive - openai#508 config-driven sandbox + --read-only (PR #33) + resume-sandbox fail-closed - #34 model/effort in /codex:rescue launch line (PR #35) - #28 turn-timeout gaps (already in fork.4 base) Co-Authored-By: Claude <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
SpawnedCodexAppServerClient.initialize()had the same unbounded handshake shape PR #30 fixed only on the broker path:request("initialize")had no deadline, so a spawnedcodex app-serverthat accepted stdin but never answered hung the client forever. This is the "identical twin" Codex flagged on #30, filed as #31.Reproduced first (not guessed)
A new
stalled-initializefake-codex behavior (accepts stdin, never answersinitialize) reproduces the hang end-to-end:CodexAppServerClient.connect({ disableBroker: true })never settles within 2s. The bug is real — the spawned child does not self-resolve via exit/close (it holds stdin open through readline).Fix (deeper, as promised in #31 / upstream openai#509)
Instead of a third copy of the bounded-handshake pattern, the deadline moves onto the shared RPC primitive:
AppServerClientBase.request(method, params, options)gains an optionaltimeoutMs(+onTimeoutfor transport cleanup). Optional / undefined-default → all existing 2-arg call sites are unchanged; the 12 hot-path call sites never allocate a timer.SpawnedCodexAppServerClient.initializepassestimeoutMs(10s default,spawnedInitializeTimeoutMsoverride) and kills the child on handshake failure so a timeout doesn't orphan the spawned process.BrokerCodexAppServerClient.initializedrops its inlinePromise.raceand usesrequest(timeoutMs, onTimeout=socket.destroy)— one mechanism, both transports.CodexAppServerClient.connecttags broker-transport init failures withbrokerFatal;withAppServer's fallback OR-chain collapses from 3 branches to(broker-busy) || (brokerFatal).Shared
killChildNow()extracted from the duplicated platform-branched kill ininitializecatch +close. RenamedBROKER_HANDSHAKE_TIMEOUT_CODE→REQUEST_TIMEOUT_CODE(value"EBROKERTIMEOUT"unchanged) since the code is now transport-agnostic.Verification
tests/spawned-client-initialize-timeout.test.mjs(304ms,EBROKERTIMEOUT).initializedid not regress fix(broker): bound broker handshake so a wedged broker can't hang the client (closes #29) #30.npm test: no new failures vsmain(4 pre-existing unchanged).Scope
Closes #31. The spawned-path handshake is now bounded; the broker path keeps its #30 behavior via the shared primitive.
captureTurn's turn-level deadline is intentionally separate (lifecycle bound, not per-RPC).🤖 Generated with Claude Code