fix: drain remaining stream bytes after [DONE] event to prevent connection force-close#3510
fix: drain remaining stream bytes after [DONE] event to prevent connection force-close#3510zerafachris wants to merge 3 commits into
Conversation
…ction force-close
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5dc9a3fc10
ℹ️ 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".
The drain added in the previous commit ran unconditionally in the `finally` block, so it also executed when the caller stopped before the `[DONE]` event -- e.g. breaking out of iteration and letting the stream be collected (GeneratorExit), an APIError, or async cancellation. In those cases it consumed the rest of the network stream instead of closing it, which for a long-running or stalled completion blocks until the server finishes and defeats the close-on-incomplete-consumption behaviour. Track whether `[DONE]` was actually observed and only drain on that path; otherwise close the response as before. Applied to both Stream and AsyncStream. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c789ceb105
ℹ️ 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 terminated: | ||
| for _ in iterator: | ||
| pass | ||
| response.close() |
There was a problem hiding this comment.
Preserve successful completion when trailing drain fails
If the server has already sent a valid [DONE] event but closes/reset the connection before its final transport bytes arrive, advancing iterator here can raise an httpx transport/protocol error. That turns a semantically completed stream into a failure during list(stream)/normal iteration, and because the exception exits this finally, response.close() is skipped as well. Drain failures after [DONE] should still close the response and not replace the successful completion (the async drain has the same issue).
Useful? React with 👍 / 👎.
Problem
When streaming SSE responses, after receiving
data: [DONE], theStream.__stream__method immediately callsresponse.close()without draining remaining bytes from the iterator. This means the HTTP/1.1 chunked terminator (0\r\n\r\n) may still be buffered, causing httpcore to destroy the connection instead of gracefully returning it to the pool.This is a regression from commit
6132922cwhich removed the drain logic. Thefinallyblock callsresponse.close()without first consuming remaining bytes.Fix
In the
finallyblock of bothStream.__stream__andAsyncStream.__stream__, before callingresponse.close()/await response.aclose(), drain any remaining bytes from the iterator to ensure the chunked transfer completes cleanly.Changes
src/openai/_streaming.py: Drainiteratorin finally block for syncStream.__stream__(addfor _ in iterator: pass)src/openai/_streaming.py: Drainiteratorin finally block for asyncAsyncStream.__stream__(addasync for _ in iterator: pass)