fix(models): close the LiteLLM provider stream on exit - #4066
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71d8c75040
ℹ️ 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".
seratch
left a comment
There was a problem hiding this comment.
Thanks for following up. The new code preserves the original stream exception correctly, but this is not a supported or reachable LiteLLM case today. The SDK requires LiteLLM >=1.83.0, acompletion(..., stream=True) returns CustomStreamWrapper, and its aclose() catches BaseException around the underlying close. The new regression test reaches the case only by replacing the private _fetch_response result with a custom stream shape.
Please simplify this PR back to the demonstrated requirement: close the LiteLLM provider stream on normal exhaustion, explicit generator close, and cancellation without delaying cancellation. Remove the stream_error and terminal cleanup-error policy plus their synthetic raising-close tests, while keeping the close and cancellation regression coverage.
|
Sure, make sense. |
LitellmModel.stream_response iterated the litellm.acompletion stream without a try/finally, so CustomStreamWrapper.aclose was never called on any path that finalizes the generator, including normal completion. openai_chatcompletions.py had the same defect until openai#3689 and any_llm_model.py shipped with the fix; litellm was the last adapter without it. Close the stream in a finally, and schedule the close in the background on cancellation so a provider close that waits on transport I/O cannot delay it.
4bab9ca to
a22cd07
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a22cd076d5
ℹ️ 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".
Summary
LitellmModel.stream_response()iterates thelitellm.acompletion(..., stream=True)result with notry/finally, soCustomStreamWrapper.aclose()is never called on any path that finalizes the generator, including normal completion.LiteLLM is the only Chat Completions adapter with this gap:
models/openai_chatcompletions.py:370-398extensions/models/any_llm_model.py:603-604finally: await self._maybe_aclose(stream)extensions/models/litellm_model.py:400-404#3689 touched two files and does not mention LiteLLM. Generator finalization does not cover it either:
ChatCmplStreamHandler.handle_streamcontains no cleanup, which is why #3689 closed the stream in the adapter itself.Impact: after a cancelled or completed streamed run the provider connection stays checked out of the pool until the next gen-2 collection, while the upstream provider keeps generating and transmitting billed tokens.
The fix mirrors the cleanup block
openai_chatcompletions.py:370-398already uses, rather than inventing a narrower variant:finally;CancelledError, schedule the close in the background and re-raise, so a provider close that waits on transport I/O cannot delay cancellation (.agents/references/runner-lifecycle.md);response.completedhas been yielded, log and ignore a cleanup failure; before any terminal event, still raise it. This keeps equivalent streaming paths aligned on errors (.agents/references/model-provider-boundaries.md)._maybe_aclosematchesany_llm_model.py:1091-1101so the adapters share one idiom. Scoped to LiteLLM:any_llm_model.pycloses its stream but has neither guard, which is a separate gap and I did not widen this PR to cover it. A directawait stream.aclose()is not available anyway: the declared typeopenai.AsyncStreamexposesclose, notaclose, so it failsmypy --strict.Test plan
Six tests in
tests/models/test_litellm_chatcompletions_stream.py. Three pin the close itself, scoped to explicitaclose(), normal exhaustion, and cancellation; one pins that a slow provider close does not delay cancellation; two pin the terminal-error semantics. The cancellation test uses a test-ownedasyncio.Eventbarrier with a boundedwait_for, notsleep(0).All six fail on
main:Each guard is mutation-tested:
Replacing the
finallybody withpassfails all six, so the cleanup itself is load-bearing too.Through the public
Runner.run_streamed(...)plusresult.cancel()API, with only the network boundary faked:LITELLM aclose_calls=0,OPENAI-CC aclose_calls=1.Codex
/reviewon this head reports no actionable issues, focused regressions 62/62. Its two earlier[P1]s on this block are what produced the cancellation branch and the terminal-event guard above.Checks
.agents/skills/code-change-verification/scripts/run.sh/reviewbefore submitting this PR