Repository navigation
feat(api): improve WebSocket recovery and delivery reporting - #4213
markstuart-oai wants to merge 1 commit into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 28da09aebd
ℹ️ 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".
28da09a to
1047dfd
Compare
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
@codex review |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1047dfd0db
ℹ️ 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".
1047dfd to
0c98073
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Approved 0c98073 after coordinator and independent static review. No actionable findings.
Complete Git tree is identical to the previously reviewed and approved internal candidate https://github.com/openai/openai-python-internal/pull/381 at d538d364be0eb98826c6c535dd99f4dc55e9ef82. Coordinator tree-ID comparison and independent recursive blob comparison confirm zero file differences. Reviewed current public feedback and retained the prior static review conclusions.
Hosted CI: {'SUCCESS': 17, 'SKIPPED': 9}. No candidate code or tests executed locally. This is an automated review on Justin’s behalf; it does not constitute a separate human-only review or authorize a merge/release.
0c98073 to
35598e8
Compare
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Approved 35598e8. Static re-review of the exact tree increment and relevant source, with an independent reviewer: recovery exclusively owns the reconnecting transition, and flushers recheck ownership/current transport after acquiring the batch. Declined preparation retains unconsumed frames and their byte budget. Idle conditional enqueue avoids redundant UTF-8 allocation while preserving the locked admission recheck. The regression tests cover the important overlapping-flush, stale-waiter, cancellation and encoding cases. No remaining findings.
Hosted checks at review: {'SUCCESS': 16, 'SKIPPED': 9, 'IN_PROGRESS': 1}. Retaining CI follow-up. No candidate code or tests executed locally.
Castiron-Internal-PR: openai/openai-python-internal#381 Castiron-Source-SHA: 4343ad7f8f297487a52e0f24c50a2e6f4335ca9b Castiron-Public-Base-SHA: 0599203
35598e8 to
528c49c
Compare
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Approved at 528c49c after coordinator and independent static re-review. Exact tree comparison against the previously reviewed commit is limited to the queue, connection wrapper, and two regression-test files. Per-frame preparation fixes the stale-transport finding: an ordinary flusher yields its unattempted tail when recovery owns the connection. Per-frame receipts preserve the correct caller's error, reentrant sends avoid waiting on their own flush, and terminal close evacuates unattempted frames while retaining them for reader reporting. No remaining actionable findings in this increment.
Hosted CI is still in progress; retaining follow-up. No candidate code or tests executed locally.
| release.set() | ||
| if owner_fails: | ||
| with pytest.raises(OSError) as got: | ||
| await owner |
| await owner | ||
| assert got.value is error | ||
| else: | ||
| await owner |
| else: | ||
| await owner | ||
| if joiner is not None: | ||
| await joiner |
| await asyncio.wait_for(joined.wait(), timeout=3) | ||
| joiner.cancel() | ||
| with pytest.raises(asyncio.CancelledError): | ||
| await joiner |
| assert ws.attempts == ["A", "B"] | ||
| finally: | ||
| release.set() | ||
| await owner |
| finally: | ||
| release.set() | ||
| with pytest.raises(OSError, match="older write closed transport"): | ||
| await owner |
| await owner | ||
| if joiner is not None: | ||
| with pytest.raises(ConnectionClosedOK): | ||
| await joiner |
| await asyncio.wait_for(joined[index].wait(), timeout=3) | ||
| release.set() | ||
| with pytest.raises(OSError) as got: | ||
| await owner |
| await owner | ||
| assert got.value is first_error | ||
| with pytest.raises(OSError) as got: | ||
| await waiters[0] |
| for index, waiter in enumerate(waiters[1:], start=2): | ||
| if index == failed_index: | ||
| with pytest.raises(OSError) as got: | ||
| await waiter |
Summary
Apply the configured recovery policy to eligible WebSocket closures and report queued frames that could not be sent.
Changes
Compatibility
This update is intended for a minor release. If your application treats close codes 1001 or 1005 as completion, disable recovery or abort the connection when that work is complete. Handle WebSocketConnectionClosedError and inspect unsent_frames (or unsent_messages for text) when queued work cannot be delivered. Messages retained after an initial write fails count toward the queue's byte limit. Handle WebSocketQueueFullError when a later message cannot fit. Existing typed-send behavior with overridden raw-send methods is preserved.
Validation