openai-v2: await AsyncStream.close on default async streaming path#4751
Open
yulin-li wants to merge 3 commits into
Open
openai-v2: await AsyncStream.close on default async streaming path#4751yulin-li wants to merge 3 commits into
yulin-li wants to merge 3 commits into
Conversation
yulin-li
force-pushed
the
yulin-li-fix-async-stream-close-leak
branch
3 times, most recently
from
4e7d3e1 to
958c563
Compare
BaseStreamWrapper.close is synchronous and calls self.stream.close() without awaiting it. On the default (non-experimental) async streaming path the wrapped stream is an openai.AsyncStream whose close() is a coroutine, so the underlying httpx response/connection was never closed (leak) and a "coroutine ... was never awaited" RuntimeWarning was emitted. Add AsyncLegacyChatStreamWrapper with an awaitable close() that awaits the underlying AsyncStream.close(), and use it for async streaming chat completions on the legacy path. Mirrors the existing sync/async split in response_wrappers. Fixes open-telemetry#4710 Assisted-by: Claude Opus 4.8 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
yulin-li
force-pushed
the
yulin-li-fix-async-stream-close-leak
branch
from
958c563 to
87e6cda
Compare
xrmx
reviewed
Jul 1, 2026
xrmx
approved these changes
Jul 1, 2026
xrmx
left a comment
xrmx
left a comment
Contributor
There was a problem hiding this comment.
I was going to cut a new release for openai-v2 thinking this was fixed in main, but isn't. So this LGTM and I think is small enough to merge even if the work has been moved to the genai repo.
openai-v2 is now released independently, so its changelog fragments live in instrumentation-genai/opentelemetry-instrumentation-openai-v2/.changelog rather than the repo root. Move 4751.fixed there per review feedback. Assisted-by: Claude Opus 4.8 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
BaseStreamWrapper.close()is synchronous and callsself.stream.close()without awaiting it. On the default (non-experimental) async streaming path,async_chat_completions_create_v_oldreturns aLegacyChatStreamWrapperwrapping anopenai.AsyncStream, whoseclose()is a coroutine. Calling it withoutawaittherefore:RuntimeWarning: coroutine 'AsyncStream.close' was never awaited, andThis also means a correct caller doing
await wrapper.close()gotNoneback, so they could not close the real stream either.The experimental path (
gen_ai_latest_experimentalopt-in) was already fixed by #4500 via util-genai'sAsyncStreamWrapper, but the default path remained affected.This PR adds
AsyncLegacyChatStreamWrapper(LegacyChatStreamWrapper)with an awaitableclose()that awaits the underlyingAsyncStream.close(), and uses it for async streaming chat completions on the legacy path. This mirrors the existing sync/async split already present inresponse_wrappers.py(AsyncResponseStreamWrapper).Fixes #4710
Type of change
How Has This Been Tested?
test_async_chat_completion_streaming_close_awaits_underlying_stream, which asserts the legacy async wrapper'sclose()awaits the underlyingAsyncStream.close()and finalizes the span.test_async_chat_completion_streaming_not_completetoawait response.close()on the legacy path.-W error::RuntimeWarning; thecoroutine ... was never awaitedwarning is gone and all tests pass.Does This PR Require a Core Repo Change?
Checklist: