Repository navigation
fix(core): retry pre-request WebSocket handshake failures - #4780
Conversation
seratch
left a comment
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.
seratch
changed the title
|
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. |
![chatgpt-codex-connector[bot]](/web-api/iframe-proxy?url=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: 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".
seratch
left a comment
seratch
left a comment
There was a problem hiding this comment.
Can you rebase the branch onto the latest main branch?
ca4f52c to
1dac0df
Compare
|
@seratch I have rebased the branch onto the latest |
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Reviewed 1dac0df809de2dfba38eae2432bc0e65346543dd in its pinned review worktree.
EOF during the HTTP upgrade occurs before a request frame is sent and was outside the retry boundary. The current head permits one retry only for the EOF-caused InvalidMessage case, retains malformed-handshake behavior, marks a frame potentially sent before awaiting send, and checks close generations to prevent replay or reconnection after explicit close. The regression coverage includes exhaustion and the relevant close/acquisition interleavings.
Validation: complete-diff and supported-path desk review, two independent fresh-context review rounds, and git diff --check. GitHub currently reports no hosted checks for this head. No local runtime tests were executed.
3 participants
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.