Skip to content

fix: close Chat Completions streams on early exit#3689

Open
fallintoplace wants to merge 3 commits into
openai:mainfrom
fallintoplace:fix/chat-completions-stream-close
Open

fix: close Chat Completions streams on early exit#3689
fallintoplace wants to merge 3 commits into
openai:mainfrom
fallintoplace:fix/chat-completions-stream-close

Conversation

@fallintoplace

Copy link
Copy Markdown
Contributor

Summary

  • Add Chat Completions stream cleanup helpers matching the Responses model behavior.
  • Close the underlying provider stream in stream_response() when iteration ends early or normally.
  • Schedule provider stream cleanup in the background when caller cancellation is in progress.
  • Add a regression test that closes the async generator early and verifies the provider stream is closed.

Why

The Chat Completions streaming path previously yielded converted events from the provider stream without closing the underlying HTTP/SSE stream if the caller stopped early. That can leave provider resources open longer than intended.

Tests

  • uv run pytest tests/models/test_openai_chatcompletions_stream.py::test_stream_response_close_closes_provider_stream
  • uv run pytest tests/models/test_openai_chatcompletions_stream.py
  • uv run ruff check src/agents/models/openai_chatcompletions.py tests/models/test_openai_chatcompletions_stream.py

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3ec5174fe5

ℹ️ 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".

model=self.model,
strict_feature_validation=self._strict_feature_validation,
):
yield chunk

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Set terminal state before yielding completed events

When a consumer breaks or closes the async generator immediately after receiving response.completed, execution is still suspended at this yield, so the bookkeeping on lines 348-350 never runs. In that common early-exit-after-terminal scenario, yielded_terminal_event remains false and any provider-stream close error is re-raised instead of being ignored for an already terminal stream; move the completed-event bookkeeping before yielding the chunk.

Useful? React with 👍 / 👎.

@seratch seratch changed the title Close Chat Completions streams on early exit fix: close Chat Completions streams on early exit Jun 25, 2026

@seratch seratch left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution. The implementation aligns with the existing Responses cleanup behavior and looks sound.

Before merging, please update the regression test to exercise asynchronous close(), which is the method exposed by the actual OpenAI AsyncStream, rather than only testing aclose(). The broader cancellation behavior is already covered by the equivalent Responses tests and does not need to be duplicated here.

Once that focused test change is made, this should be ready to merge.

@seratch seratch added this to the 0.17.x milestone Jun 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants