Recover Codex websocket requests after upstream resets#3904
Conversation
|
This pull request targeted The base branch has been automatically changed to |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements an HTTP fallback mechanism for the Codex Websockets Executor when a "message too big" error (WebSocket close code 1009) is encountered during execution or streaming. If no payload has been sent yet, the executor falls back to standard HTTP execution or streaming. A test case has been added to verify this fallback behavior. The reviewer identified a critical race condition where the downstream websocket connection could still be closed prematurely due to the upstream disconnect notification being triggered immediately on a 1009 error. A code suggestion was provided to dynamically determine the notification flag based on the error type to prevent this premature closure.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| func (e *CodexWebsocketsExecutor) invalidateUpstreamConn(sess *codexWebsocketSession, conn *websocket.Conn, reason string, err error) { | ||
| e.clearUpstreamConn(sess, conn, reason, err, true) | ||
| } |
There was a problem hiding this comment.
There is a subtle race condition where the downstream websocket connection can still be closed prematurely despite the fallback mechanism.
When readUpstreamLoop encounters a 1009 (CloseMessageTooBig) error, it immediately calls invalidateUpstreamConn, which always invokes clearUpstreamConn with notify = true. Because readUpstreamLoop runs concurrently and triggers this notification immediately after sending the error to readCh, it will almost always execute before ExecuteStream or Execute can receive the error and call clearUpstreamConn(..., false). This triggers sess.notifyUpstreamDisconnect(err), closing sess.upstreamDisconnectCh and signaling the downstream handler to close the downstream websocket before the HTTP fallback stream can be completed.
To resolve this, we should dynamically determine the notify flag in invalidateUpstreamConn based on whether the error is a 1009 message too big error.
| func (e *CodexWebsocketsExecutor) invalidateUpstreamConn(sess *codexWebsocketSession, conn *websocket.Conn, reason string, err error) { | |
| e.clearUpstreamConn(sess, conn, reason, err, true) | |
| } | |
| func (e *CodexWebsocketsExecutor) invalidateUpstreamConn(sess *codexWebsocketSession, conn *websocket.Conn, reason string, err error) { | |
| notify := !isCodexWebsocketMessageTooBigError(err) | |
| e.clearUpstreamConn(sess, conn, reason, err, notify) | |
| } |
02f5664 to
b279372
Compare
![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: 02f566421f
ℹ️ 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".
| terminateReason = "message_too_big_http_fallback" | ||
| helps.RecordAPIWebsocketError(ctx, e.cfg, "read_message_too_big_http_fallback", errRead) | ||
| if sess != nil { | ||
| e.clearUpstreamConn(sess, conn, terminateReason, errRead, false) |
There was a problem hiding this comment.
When a downstream Responses websocket uses an execution session, readUpstreamLoop is the goroutine that receives the close-1009, sends it into readCh, and then immediately calls invalidateUpstreamConn, which notifies UpstreamDisconnectChan; the websocket handler's subscriber closes the client connection before this goroutine reaches this clearUpstreamConn(..., false) fallback path. In that session/passthrough case the HTTP fallback stream is written to a closed downstream websocket, so 1009 still drops the client instead of falling back.
Useful? React with 👍 / 👎.
| if sess != nil { | ||
| e.clearUpstreamConn(sess, conn, terminateReason, errRead, false) | ||
| } | ||
| fallbackResult, errFallback := e.CodexExecutor.ExecuteStream(ctx, auth, req, opts) |
There was a problem hiding this comment.
For downstream websocket passthrough on later turns, req.Payload can be only an incremental response.create/response.append with previous_response_id, not the full transcript. Passing that payload directly to CodexExecutor.ExecuteStream loses the session state because the HTTP Codex executor deletes previous_response_id before POSTing, so a close-1009 on a follow-up turn sends the fallback request without conversation context; the fallback needs to replay/expand the transcript or avoid HTTP fallback for incremental websocket requests.
Useful? React with 👍 / 👎.
b279372 to
4832fe6
Compare
|
Addressed the current automated review feedback in 4832fe6:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4832fe6f96
ℹ️ 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".
| func canFallbackCodexWebsocketRequestToHTTP(body []byte) bool { | ||
| return strings.TrimSpace(gjson.GetBytes(body, "previous_response_id").String()) == "" | ||
| } |
There was a problem hiding this comment.
When a downstream Responses websocket is in passthrough mode, later turns can be response.append messages that carry only the incremental input and no previous_response_id (the handler accepts that shape in normalizeResponsesWebsocketPassthroughRequest). This helper therefore allows the new 1009 HTTP fallback for those append-only turns, but CodexExecutor.ExecuteStream has no access to the reused websocket session and posts only this incremental payload over HTTP, so the fallback response is generated without the prior conversation context. Please also reject type == "response.append" here unless the fallback path can replay the full transcript.
Useful? React with 👍 / 👎.
4832fe6 to
d3aa355
Compare
|
Addressed the latest append-only review feedback in d3aa355:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3aa355613
ℹ️ 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".
| } | ||
|
|
||
| func (e *CodexWebsocketsExecutor) invalidateUpstreamConn(sess *codexWebsocketSession, conn *websocket.Conn, reason string, err error) { | ||
| notify := !isCodexWebsocketMessageTooBigError(err) |
There was a problem hiding this comment.
When a session request receives close-1009 but the new HTTP fallback is not taken (for example previous_response_id/response.append requests, or after sentPayload is already true), ExecuteStream falls through to the normal read-error path. Because this line suppresses UpstreamDisconnectChan solely based on the close code, the Responses websocket subscriber in sdk/api/handlers/openai/openai_responses_websocket.go never closes the downstream client even though the upstream session was cleared; keep the suppression tied to the actual fallback path rather than every 1009 disconnect.
Useful? React with 👍 / 👎.
d3aa355 to
c9bcede
Compare
|
Addressed the latest 1009-disconnect review feedback in c9bcede:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c9bcedea0e
ℹ️ 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".
| if sess != nil { | ||
| e.clearUpstreamConn(sess, conn, terminateReason, errRead, false) | ||
| } | ||
| fallbackResult, errFallback := e.CodexExecutor.ExecuteStream(ctx, auth, req, opts) |
There was a problem hiding this comment.
When a downstream Responses websocket passthrough request hits close-1009 before any payload, this falls back by passing the original websocket event payload to CodexExecutor.ExecuteStream. That payload still contains the transport envelope (type: "response.create", preserved by normalizeResponsesWebsocketPassthroughRequest), while the HTTP Responses POST should receive only the create body; the HTTP executor does not remove the top-level type, so the new 1009 fallback can be rejected for actual websocket clients even though the direct test payload lacks this field. Build a sanitized HTTP request (or strip websocket-only fields) before invoking the HTTP executor.
Useful? React with 👍 / 👎.
c9bcede to
baf3ad6
Compare
|
Addressed the websocket-envelope HTTP fallback feedback in baf3ad6:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: baf3ad6823
ℹ️ 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".
| case ch <- ev: | ||
| delivered = true |
There was a problem hiding this comment.
When the upstream sends a terminal event and then closes before the request goroutine runs its deferred clearActive, this buffered send can enqueue the close error into readCh even though the request has already returned and no one will read it. Because delivered becomes true, readUpstreamLoop skips invalidating the session, leaving sess.conn pointing at a closed socket; the next turn then hits the send-error path, notifies UpstreamDisconnectChan before retrying, and closes the downstream websocket despite the retry potentially succeeding.
Useful? React with 👍 / 👎.
baf3ad6 to
597b123
Compare
|
Addressed the latest terminal-close review feedback in 597b123:
Testing:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 597b123236
ℹ️ 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".
| if terminalDeliveredCh != nil && sess.activeChannelUnchangedOrCleared(terminalDeliveredCh) { | ||
| e.clearUpstreamConn(sess, conn, "upstream_disconnected_after_terminal", errRead, false) | ||
| return |
There was a problem hiding this comment.
When the upstream sends response.completed and then its close frame is processed only after the stream goroutine has cleared the old active channel and the downstream immediately starts another turn, activeCh now points at the new request, so this check is false and the old terminal-close is delivered to that new request at line 1615. That makes the next turn fail and notifies UpstreamDisconnectChan, closing the downstream websocket instead of silently reconnecting; the added test waits for sess.conn == nil before sending the second request, so it misses this fast-follow case.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 597b123236
ℹ️ 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".
| if terminalDeliveredCh != nil && sess.activeChannelUnchangedOrCleared(terminalDeliveredCh) { | ||
| e.clearUpstreamConn(sess, conn, "upstream_disconnected_after_terminal", errRead, false) | ||
| return |
There was a problem hiding this comment.
When the upstream sends response.completed and then its close frame is processed only after the stream goroutine has cleared the old active channel and the downstream immediately starts another turn, activeCh now points at the new request, so this check is false and the old terminal-close is delivered to that new request at line 1615. That makes the next turn fail and notifies UpstreamDisconnectChan, closing the downstream websocket instead of silently reconnecting; the added test waits for sess.conn == nil before sending the second request, so it misses this fast-follow case.
Useful? React with 👍 / 👎.
597b123 to
549a4ad
Compare
|
Addressed the latest fast-follow terminal-close feedback in 549a4ad:
Testing:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 549a4ad449
ℹ️ 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".
| e.clearUpstreamConn(sess, conn, "send_error", errSend, false) | ||
|
|
||
| // Retry once with a new websocket connection for the same execution session. | ||
| connRetry, respHSRetry, errDialRetry := e.ensureUpstreamConn(ctx, auth, sess, authID, wsURL, wsHeaders) |
There was a problem hiding this comment.
When a session write fails because the reused websocket is stale, this clears and closes the old connection but leaves the current readCh accepting events while the retry is being set up. The old readUpstreamLoop is still running; closing conn can make it deliver that old-connection error into the active channel and close it, so after the retry succeeds the stream skips the mismatched event (ev.conn != connRetry) and then fails with session read channel closed instead of reading the retry response. Stop delivery for the old active channel or replace it before closing the stale connection.
Useful? React with 👍 / 👎.
549a4ad to
a9868d0
Compare
|
Addressed the latest stale-send retry feedback in a9868d0:
Testing:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9868d0cfb
ℹ️ 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".
| readCh = make(chan codexWebsocketRead, 4096) | ||
| sess.setActive(readCh) |
There was a problem hiding this comment.
When a non-streaming Execute call uses an execution session and this retry path is taken, the earlier defer sess.clearActive(readCh) has already captured the original channel, so this reassignment leaves the replacement channel registered after a successful retry. In that scenario a later upstream close can be delivered into the abandoned buffered channel and skipped, leaving the session pointing at a closed websocket until the next turn hits the stale-send path; use a deferred closure or explicitly clear the replacement channel before returning.
Useful? React with 👍 / 👎.
a9868d0 to
4ff42dc
Compare
|
Addressed the latest replacement active-channel feedback in 4ff42dc:
Testing:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ff42dc9f9
ℹ️ 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".
| if !sentPayload && isCodexWebsocketMessageTooBigError(errRead) && canFallbackCodexWebsocketRequestToHTTP(upstreamBody) { | ||
| terminateReason = "message_too_big_http_fallback" | ||
| helps.RecordAPIWebsocketError(ctx, e.cfg, "read_message_too_big_http_fallback", errRead) | ||
| if sess != nil { | ||
| e.clearUpstreamConn(sess, conn, terminateReason, errRead, false) |
There was a problem hiding this comment.
When a downstream Responses websocket passthrough first turn hits 1009 here, the HTTP fallback can successfully return a response while clearUpstreamConn leaves no upstream websocket with conversation state. The passthrough handler accepts later response.append messages without tracking/replaying lastResponseID, so the next append-only turn on the same downstream socket is sent to a fresh upstream websocket with only incremental input and loses the prior turn. Please close the downstream websocket after this fallback or force transcript/previous_response_id replay before accepting later append-only turns.
Useful? React with 👍 / 👎.
| } | ||
| terminateReason = "read_error" | ||
| terminateErr = errRead | ||
| if sess != nil && !sentPayload && isCodexWebsocketStaleTerminalCloseError(errRead) { |
There was a problem hiding this comment.
When a passthrough downstream sends a later response.append without previous_response_id, its context exists only on the reused upstream websocket. If a stale terminal close from the previous turn arrives before any bytes from this new turn, this retry path reconnects and resends the request; buildCodexWebsocketRequestBody rewrites the append as response.create without replaying the transcript, so the follow-up is answered as a fresh conversation. Please skip this reconnect retry for append-only requests or expand them before retrying.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f02ac3dac1
ℹ️ 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".
| errType := strings.TrimSpace(gjson.GetBytes(payload, "error.type").String()) | ||
| if errType == "" { | ||
| errType = strings.TrimSpace(gjson.GetBytes(payload, "body.error.type").String()) | ||
| } |
There was a problem hiding this comment.
When Codex sends a statusless top-level error such as {"type":"error","error_type":"invalid_request_error","message":"bad request"}, this status inference never copies error_type into errType even though the payload builder below preserves that field. The error is therefore surfaced as a 500 instead of the intended 400, which makes clients and retry/auth-release logic treat a client-side request error as an upstream server failure.
Useful? React with 👍 / 👎.
| errType := strings.TrimSpace(gjson.GetBytes(payload, "error.type").String()) | ||
| if errType == "" { | ||
| errType = strings.TrimSpace(gjson.GetBytes(payload, "body.error.type").String()) | ||
| } | ||
| if errType == "" { | ||
| topLevelType := strings.TrimSpace(gjson.GetBytes(payload, "type").String()) | ||
| if !strings.EqualFold(topLevelType, wsEventTypeError) { | ||
| errType = topLevelType | ||
| } | ||
| } |
There was a problem hiding this comment.
For downstream websocket passthrough, a statusless error event with top-level error_type (for example invalid_request_error) reaches this parser directly; because type is just error, errType remains empty and the status defaults to 500. That makes recoverable/client-side Codex errors sent in this supported top-level shape look like server failures to the websocket client.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d016a1f060
ℹ️ 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".
| if status <= 0 { | ||
| status = int(gjson.GetBytes(payload, "status_code").Int()) | ||
| } | ||
| errCode := strings.TrimSpace(gjson.GetBytes(payload, "error.code").String()) |
There was a problem hiding this comment.
When Codex sends the statusless form with a bare error string, e.g. {"type":"error","error":"websocket_connection_limit_reached"}, the executor-side detector already treats that error value as a connection-limit code, but this downstream parser only copies error.code/body.error.code/top-level code. The status therefore falls through to 500, so websocket clients see a server error and shouldReleaseResponsesWebsocketPinnedAuth will not release a saturated pinned auth instead of handling it as a 429; read a string-valued error as a code before inferring status.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88ed5c043f
ℹ️ 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".
| current := s.conn | ||
| suppressed := s.suppressReadErrorConn == conn | ||
| s.connMu.Unlock() | ||
| return current != conn || suppressed |
There was a problem hiding this comment.
When a Codex auth is removed (sdk/cliproxy/service.go:753) or a session is otherwise closed, closeCodexWebsocketSession clears sess.conn before closing the socket. With this condition, the reader sees current != conn and suppresses the resulting read error instead of delivering it to the active request channel, so an in-flight request waiting in readCodexWebsocketMessage can hang until the client context or idle timeout rather than failing promptly. Suppress only explicitly marked/stale reader errors, or still notify activeCh for externally closed sockets.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b1c72c4e1
ℹ️ 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".
| continue | ||
| } | ||
|
|
||
| if wsErr, ok := codexWebsocketStatuslessErrorEvent(payload); ok { |
There was a problem hiding this comment.
For downstream websocket passthrough, statusless upstream error events hit the DownstreamWebsocket branch and return on eventType == "error" before this statusless-error handler runs. In cases like {"type":"error","code":"websocket_connection_limit_reached"} the handler may release/retry auth, but the executor leaves the old upstream conn cached; the next turn can then reuse the same failed socket/auth via ensureUpstreamConn instead of clearing it as the statusful error path does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3751163081
ℹ️ 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".
| if forwardErrMsg != nil { | ||
| lastRequest = previousLastRequest | ||
| lastResponseOutput = previousLastResponseOutput | ||
| lastResponseID = previousLastResponseID | ||
| lastResponsePendingToolCallIDs = previousLastResponsePendingToolCallIDs | ||
| passthroughModelName = previousPassthroughModelName |
There was a problem hiding this comment.
When a non-passthrough turn switches to another websocket-capable auth and then returns a normal upstream error that shouldReleaseResponsesWebsocketPinnedAuth does not catch (for example a generic 400 validation error), the selected-auth callback may already have set pinnedAuthID/lastAttemptedAuthID for that failed turn. This rollback restores the previous transcript and passthrough model but leaves those auth fields pointing at the failed model's auth, so the next model-less follow-up derives the previous model from lastRequest while executing with WithPinnedAuthID for the failed model, which can route to the wrong auth or fail auth selection; save and restore or clear the pinned auth state with the rest of the rolled-back turn state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c437e9c9e9
ℹ️ 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".
| if !canRetryCodexWebsocketRequestAfterStaleTerminalClose(sess, upstreamBody) { | ||
| terminateReason = "stale_terminal_close_append_without_retry" | ||
| helps.RecordAPIWebsocketError(ctx, e.cfg, "read_stale_terminal_close_append_without_retry", errRead) | ||
| sess.notifyUpstreamDisconnect(errRead) |
There was a problem hiding this comment.
When the upstream closes just after a previous terminal event and the next follow-up/create turn has already installed a new active reader, readUpstreamLoop marks the session lost and delivers errCodexWebsocketStaleTerminalClose. For live-context requests this branch sends that error and notifies UpstreamDisconnectChan, but the Responses websocket retry logic only intercepts request requires existing websocket session or previous_response_not_found, so the downstream socket is closed instead of replaying the cached transcript. Return an interceptable state-loss error (and avoid the disconnect notification) for this no-fresh-retry case.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea1d805596
ℹ️ 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".
| if codexWebsocketRequestUsesPreviousResponseID(body) || codexWebsocketRequestIsAppendOnly(body) { | ||
| return false | ||
| } | ||
| return codexWebsocketInputLooksFullTranscript(gjson.GetBytes(body, "input")) |
There was a problem hiding this comment.
Fresh evidence after the earlier reset fix: this return still requires an assistant/tool/compaction marker. When the Responses handler sends a normalized replay body with no type and an input containing only user/developer messages after a prior upstream turn completed with no output, this returns false, so resetUpstreamConnForFreshContext is skipped; buildCodexWebsocketRequestBody then sends that full transcript as response.create on top of the old Codex websocket state, duplicating prior context instead of starting from the replayed transcript.
Useful? React with 👍 / 👎.
| } else if strings.EqualFold(errCode, "previous_response_not_found") || strings.EqualFold(errType, "invalid_request_error") { | ||
| status = http.StatusBadRequest |
There was a problem hiding this comment.
This status inference still only checks errCode/errType before defaulting statusless errors to 500. If a downstream passthrough error arrives as a statusless message-only envelope such as {"type":"error","message":"previous_response_id ... not found"}, responsesWebsocketPreviousResponseNotFoundError rejects it because the status is not 400, so the cached transcript replay path is skipped even though the executor-side status inference handles message-only previous-response misses.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 498703c1cc
ℹ️ 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".
| if callerScope == "" { | ||
| return sessionKey |
There was a problem hiding this comment.
Fresh evidence: AuthMiddleware explicitly allows requests without setting userApiKey when no auth providers are configured (internal/api/server.go:1990-1992), so this branch stores/restores transcript state under only the client-supplied session header in that supported mode. In an unauthenticated deployment, any client that reuses or guesses another client's X-Client-Request-Id/Session-Id can inherit cached lastRequest and response output for up to 30 minutes; skip reconnect caching without a caller scope or add another per-client binding.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ead92185bd
ℹ️ 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".
| isTerminalEvent := eventType == "response.completed" || eventType == "response.done" || eventType == "error" | ||
| isTerminalEvent := isCodexWebsocketSuccessfulTerminalEvent(eventType) || eventType == "response.failed" || eventType == "error" | ||
| if eventType == "response.failed" { | ||
| wsErr, _, _ := codexTerminalFailureErr(payload) |
There was a problem hiding this comment.
When Codex reports upstream state loss as a terminal response.failed event whose response.error only carries code: previous_response_not_found or the matching message without type: invalid_request_error, this branch converts it through codexTerminalFailureErr, which defaults that body to 502. The Responses websocket retry guard only replays cached transcript for 400 previous-response errors, so this recoverable state-loss terminal failure is forwarded/release-handled instead of being replayed; preserve/map previous_response_not_found to 400 before returning the stream error.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1faa455a8f
ℹ️ 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".
| default: | ||
| } | ||
| s.clearActive(ch) | ||
| close(ch) |
There was a problem hiding this comment.
When an auth/session close calls closeCodexWebsocketSession while readUpstreamLoop has already captured the same active channel for a text frame, this closes ch from deliverActiveRead; the reader goroutine can then execute its ch <- codexWebsocketRead{...} send on a closed channel and panic the process. Cancel or mark the channel inactive without closing it, or serialize all sends with the close.
Useful? React with 👍 / 👎.
| } else { | ||
| status = http.StatusInternalServerError |
There was a problem hiding this comment.
When downstream passthrough receives a statusless Codex error with error.type: rate_limit_error or code: rate_limit_exceeded, this inference falls through to 500 because only websocket connection limits and previous-response/invalid-request errors are recognized. shouldReleaseResponsesWebsocketPinnedAuth does not release pinned auths on 500, so a rate-limited credential can stay pinned instead of failing over; map those statusless rate-limit shapes to 429.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db741f4475
ℹ️ 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".
| strings.Contains(message, "previous_response_id") && strings.Contains(message, "not found") { | ||
| return http.StatusBadRequest | ||
| } | ||
| return http.StatusInternalServerError |
There was a problem hiding this comment.
In the executor path, a statusless Codex websocket error such as {"type":"error","error":{"type":"rate_limit_error","code":"rate_limit_exceeded"}} (or usage_limit_reached) is not recognized by the cases above and falls through to a 500 here. Non-passthrough Codex websocket callers then see a server failure instead of a retryable 429, and quota errors also lose the existing retry-after handling used by the terminal-error path; add the same rate/quota classification before defaulting to 500.
Useful? React with 👍 / 👎.
…allback # Conflicts: # internal/runtime/executor/codex_websockets_executor.go # internal/runtime/executor/codex_websockets_executor_test.go # sdk/api/handlers/openai/openai_responses_websocket.go
1 participant
Summary
message too big) and repeated pre-payloadwebsocket: close sentfailuresprevious_response_not_foundwebsocket errors and retry the same downstream request once with transcript replay when local transcript state is availableprevious_response_not_foundreplay recoveryTesting
go test ./sdk/api/handlers/openai -run 'TestResponsesWebsocket' -count=1go test ./internal/runtime/executor -run 'TestCodexWebsockets|TestCodexExecutor' -count=1go test ./sdk/api/handlers -run 'TestRequestExecutionMetadata' -count=1CC=/usr/bin/clang go build -o test-output ./cmd/server && rm test-output