fix(proxy): close common streaming responses on exit - #28353
Conversation
|
|
Greptile SummaryThis PR adds a
Confidence Score: 5/5Safe to merge — the change is tightly scoped to cleanup logic with no effect on the happy-path data flow. The fix is minimal and correct: a shielded finally block that closes a resource after the generator exits. anyio is already a project dependency used elsewhere in the proxy. The inner try/except BaseException ensures any double-close or already-closed errors are absorbed without surfacing to callers. The three new tests exercise every exit path and all pass locally per the PR description. No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/common_request_processing.py | Adds a shielded finally block to async_streaming_data_generator to call response.aclose() on exit, preventing upstream connection leaks on early client disconnect. |
| tests/test_litellm/test_common_request_processing_streaming_cleanup.py | New test file covering early exit, normal completion, and mid-stream error paths — all mock-based, no real network calls. |
Reviews (1): Last reviewed commit: "fix(proxy): close common streaming respo..." | Re-trigger Greptile
Greptile SummaryThis PR fixes a resource leak in
Confidence Score: 4/5The change is a targeted cleanup addition with no effect on the data path; safe to merge once the double-close consideration is acknowledged. The fix is logically correct and the three new tests validate the key paths. The one open question is whether any No files require special attention beyond the
|
| Filename | Overview |
|---|---|
| litellm/proxy/common_request_processing.py | Adds a shielded finally block to async_streaming_data_generator that calls response.aclose() on exit, preventing leaked upstream streaming connections on early client disconnect. |
| tests/test_litellm/test_common_request_processing_streaming_cleanup.py | New test file covering three paths for the cleanup fix: early exit via aclose(), normal completion, and mid-stream exception. All use mocks with no real network calls. |
Reviews (2): Last reviewed commit: "fix(proxy): close common streaming respo..." | Re-trigger Greptile
| finally: | ||
| with anyio.CancelScope(shield=True): | ||
| if hasattr(response, "aclose"): | ||
| try: | ||
| await response.aclose() | ||
| except BaseException as e: | ||
| verbose_proxy_logger.debug( | ||
| "async_streaming_data_generator: error closing response stream: %s", | ||
| e, | ||
| ) |
There was a problem hiding this comment.
Potential silent double-close if hooks drain the stream
async_post_call_streaming_iterator_hook receives the raw response object and iterates it. If any registered hook implementation calls response.aclose() internally after draining (e.g. a logging or billing hook), the finally block will call aclose() a second time. The except BaseException swallows that silently, so there is no crash, but the debug log will fire on every normal request through such hooks. It is worth confirming that no hook implementation closes the stream, or alternatively documenting that response must not be closed by callers of the hook.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
🤖 litellm-agent: This PR is currently BLOCKED from merge. Score: 4/5 ❌ Why blocked:
Details: Score docked for: 1 unresolved reviewer concern (greptile). Fix the issues above and push an update — the bot will re-review automatically.
|
|
Closing this PR to reopen it from nhcf/litellm-nanhu so future updates can be pushed from the organization fork. |
Relevant issues
Related to #25776.
Linear ticket
N/A
Pre-Submission checklist
tests/test_litellm/directory, Adding at least 1 test is a hard requirement - see detailsmake test-unit@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewDelays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
CI (LiteLLM team)
Branch creation CI run
Link:
CI run for the last commit
Link:
Merge / cherry-pick CI run
Links:
Screenshots / Proof of Fix
Targeted tests pass locally:
uv run --no-sync pytest tests/test_litellm/test_common_request_processing_streaming_cleanup.py # 3 passedLint on changed files passes locally:
uv run --no-sync ruff check litellm/proxy/common_request_processing.py tests/test_litellm/test_common_request_processing_streaming_cleanup.py # All checks passedType
🐛 Bug Fix
✅ Test
Changes
response.aclose()fromProxyBaseLLMRequestProcessing.async_streaming_data_generatorin a shieldedfinallyblock.