fix(core): retry pre-request WebSocket handshake failures - #4780
fix(core): retry pre-request WebSocket handshake failures#4780CoralGarden52 wants to merge 2 commits into
Conversation
seratch
left a comment
There was a problem hiding this comment.
Thanks for the contribution. Moving connection acquisition into the retry boundary addresses the reported pre-request disconnect.
Please narrow the new InvalidMessage handling to failures caused by EOFError, matching the transient-handshake classification in websockets. Other malformed HTTP responses should retain their existing exception behavior without automatically suggesting retries.
Please update the regression coverage to distinguish these cases, verify that repeated EOF failures exhaust the single internal retry without sending a request, and cover close() during a failing handshake so it cannot trigger another connection attempt.
|
Thank you for the detailed guidance, @seratch. I’ve submitted a follow-up commit that narrows InvalidMessage retry handling to cases directly caused by EOFError, matching websockets’ transient-handshake classification. Other malformed HTTP responses retain their existing exception behavior without retry advice. I also updated the regression coverage for retry exhaustion without sending a request and for close() during a failing handshake. Focused and full verification passed. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 822d8b81b3
ℹ️ 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".
| yielded_terminal_event = False | ||
| sent_request_frame = False | ||
| try: | ||
| connection = await self._await_websocket_with_timeout( |
There was a problem hiding this comment.
Recheck close generation after the retry handshake
When the first EOF handshake failure triggers the new retry and close() runs while the second handshake is pending, close() sees the request lock but no cached connection and returns after incrementing the generation. If this awaited handshake then succeeds, _ensure_websocket_connection() caches the new socket and execution sends the request because the generation is checked only in the exception path. Thus a request and persistent connection can survive an explicit completed close(); revalidate request_close_generation immediately after connection acquisition and dispose the newly opened connection before sending when it changed.
AGENTS.md reference: AGENTS.md:L149-L149
Useful? React with 👍 / 👎.
Summary
OpenAIResponsesWSModelcould fail immediately when the peer closed during the WebSocket HTTP upgrade. With the supportedwebsockets>=15client, a close before a valid HTTP 101 response is reported aswebsockets.exceptions.InvalidMessage. That exception was raised before_ensure_websocket_connectionreturned, so the existing pre-event retry path never handled it.This change:
InvalidMessagecase;Test plan
response.createframe, andresponse_id=resp_local_probe.make tests: 9292 passed, 29 skipped; serial tests: 77 passed, 4 skipped..agents/skills/code-change-verification/scripts/run.sh: passed (format, lint, typecheck, and tests).InvalidMessage,websocket handshake,pre-event websocket, andwebsocket retry; no duplicate open PR was found.Issue number
No existing issue found; the failure was reproduced locally against the real
websocketshandshake path.Checks
.agents/skills/code-change-verification/scripts/run.shThis PR is intentionally opened as a draft pending maintainer review.