[WIP] Litellm client disconnect relay. Routing OSS PR into CI - #27146
[WIP] Litellm client disconnect relay. Routing OSS PR into CI#27146harish-berri wants to merge 8 commits into
Conversation
Greptile SummaryThis PR refactors client disconnect detection by moving
Confidence Score: 3/5Not safe to merge as-is: failure hooks are silently skipped for all cancelled requests, and the always-on background task spawning has unresolved profiling TODOs. Two P1 findings: litellm/proxy/common_request_processing.py — both the
|
| Filename | Overview |
|---|---|
| litellm/proxy/common_request_processing.py | Core change: adds _check_request_disconnection and integrates it unconditionally into make_call; _handle_llm_api_exception early-exits for 499, skipping post_call_failure_hook and failure callbacks. |
| litellm/proxy/proxy_server.py | Removes dead check_request_disconnection function; straightforward cleanup with no logic changes. |
| litellm/constants.py | Adds DEFAULT_CLIENT_DISCONNECT_CHECK_TIMEOUT_SECONDS (env-configurable, default 600s); straightforward constant addition. |
| tests/test_litellm/proxy/test_client_disconnection.py | New unit tests for _check_request_disconnection; uses AsyncMock(side_effect=[False, True]) to correctly simulate disconnect polling — mock setup is accurate and covers both disconnect and cancel paths. |
Reviews (3): Last reviewed commit: "Refactor tests for client disconnection ..." | Re-trigger Greptile
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
|
@greptile review again |
Updated test cases in `test_client_disconnection.py` to use `MagicMock` for `mock_request` with an `is_disconnected` method. Adjusted assertions to verify the correct behavior during client disconnection and normal operation scenarios. Improved clarity of test descriptions to reflect the disconnect handling logic.
|
@greptile the P1 on the test is handled, review again |
| if isinstance(e, HTTPException) and e.status_code == 499: | ||
| verbose_proxy_logger.info("Client disconnected the request (499)") | ||
| raise e |
There was a problem hiding this comment.
post_call_failure_hook bypassed for 499 responses
The early raise e at line 1784 exits the function before post_call_failure_hook is called (line 1790). Any registered failure callbacks — usage logging, billing hooks, alerting integrations — will silently not run for client-disconnected requests. If the proxy has hooks that track spend or emit telemetry on every request failure, those records will be missing for all 499s.
Consider calling post_call_failure_hook before re-raising, or at minimum documenting that failure hooks are intentionally skipped for client disconnects.
| # Execute the task to detect disconnection | ||
| disconnect_event = asyncio.Event() | ||
| disconnect_task = asyncio.create_task( | ||
| _check_request_disconnection(request, llm_responses, disconnect_event) | ||
| ) |
There was a problem hiding this comment.
Disconnect detection always-on with no opt-out flag
The disconnect watcher task is now unconditionally spawned for every request routed through ProxyBaseLLMRequestProcessing.make_call. The PR description itself lists "Regression profiling under high concurrent loads" and "Check for memory leaks when requests are cancelled" as open TODOs, indicating this path hasn't been load-tested yet. Per the team's backward-compatibility rule, risky additive behaviours on the critical request path should be guarded by a user-controlled flag (e.g. litellm.enable_client_disconnect_checking) so operators can disable it if regressions appear, rather than requiring a full version rollback.
Rule Used: What: avoid backwards-incompatible changes without... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
The two open TODOs here (load regression risk and the memory leak note) and the two Greptile P1s are exactly what #30223 addresses: the watcher is opt-in (default off, zero overhead and byte-identical default path) and 499 falls through to post_call_failure_hook so accounting and slot release stay intact. It also carries focused unit tests plus a live curl-and-kill transcript showing the backend aborts within ~1.5s of the client dropping. Feel free to fold any of it into this PR instead if that routes better through CI |
Relevant issues
TODO:
Linear ticket
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
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
Type
🆕 New Feature
🐛 Bug Fix
🧹 Refactoring
📖 Documentation
🚄 Infrastructure
✅ Test
Changes