fix(aiohttp): dispose recycled client sessions deterministically - #32003
fix(aiohttp): dispose recycled client sessions deterministically#32003anmolg1997 wants to merge 1 commit into
Conversation
Merging this PR will not alter performance
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | test_completion_simple_message |
3.2 ms | 4.2 ms | -23.47% |
| ⚡ | test_completion_multi_turn |
4.2 ms | 3.1 ms | +33.31% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing anmolg1997:fix/aiohttp-recycled-session-leak (18e8ef0) with main (88e03e5)
Greptile SummaryThis PR fixes deterministic disposal of recycled
Confidence Score: 5/5The change is a targeted, self-contained cleanup fix that does not touch any request-processing hot path or modify any public API; the happy path (valid session on the current running loop) is structurally unchanged. All three session-replacement sites now deterministically dispose the recycled session, with correct handling for every loop lifecycle (current, foreign-running, stopped, closed). The concurrent-replacement race in the exception handler is resolved by operating on the captured local variable instead of No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/llms/custom_httpx/aiohttp_transport.py | Adds _close_recycled_session helper and _background_close_tasks class registry to deterministically dispose replaced ClientSession objects across all three replacement sites (loop-mismatch recycle, fallback branch, "Session is closed" retry). All three lifecycle branches (current loop, foreign running loop, stopped/closed loop) are handled correctly. |
| tests/test_litellm/llms/custom_httpx/test_aiohttp_transport.py | Adds seven regression tests for the session-leak fix: fallback branch closure, GC warning suppression, dead-loop synchronous disposal, strong-reference registry, foreign-loop threadsafe close, concurrent-replacement safety, and stopped-loop synchronous disposal. All tests are properly mocked with no real network calls. |
Reviews (5): Last reviewed commit: "fix(aiohttp): dispose recycled client se..." | Re-trigger Greptile
Greptile SummaryThis PR fixes aiohttp
Confidence Score: 3/5The core fix is sound for its primary target scenarios, but the "Session is closed" retry path in handle_async_request can close an unrelated healthy session under concurrent load, causing failures on subsequent requests. The disposal helper itself is well-designed and the tests validate each lifecycle branch. The risk is in handle_async_request: old_session is read from self.client after an await point, so a concurrent task that already replaced self.client can cause the exception handler to close the freshly-created replacement session rather than the one that actually errored. litellm/llms/custom_httpx/aiohttp_transport.py — specifically the exception handler in handle_async_request around line 410.
|
| Filename | Overview |
|---|---|
| litellm/llms/custom_httpx/aiohttp_transport.py | Adds _close_recycled_session helper that correctly handles three lifecycle paths (async on current loop, threadsafe on foreign loop, sync on dead loop); however the "Session is closed" retry branch in handle_async_request captures self.client instead of the local client_session variable, which can close the wrong session under concurrent load. |
| tests/test_litellm/llms/custom_httpx/test_aiohttp_transport.py | Adds five well-scoped regression tests covering all three disposal paths and the task-registry lifecycle; no real network calls; test_close_task_strongly_referenced_until_done asserts on class-level state (_background_close_tasks) without resetting it first, which could be noisy in a parallel test run. |
Reviews (2): Last reviewed commit: "fix(aiohttp): dispose recycled client se..." | Re-trigger Greptile
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
18e8ef0 to
cd7da48
Compare
2247681 to
f29611b
Compare
LiteLLMAiohttpTransport replaced its cached aiohttp.ClientSession on
loop-mismatch, loop-inspection failure, and "Session is closed" retry
without reliably closing the previous session:
- the close task from asyncio.create_task() was never referenced, so
it could be garbage-collected before running;
- the (RuntimeError, AttributeError) fallback branch replaced the
session without closing it at all;
- sessions bound to a closed event loop were abandoned to the GC
("rely on GC"), and sessions bound to a loop running in another
thread were closed from the wrong loop.
Replaced sessions surfaced as intermittent "Unclosed client session" /
"Unclosed connector" errors from the event-loop exception handler at
GC time.
_close_recycled_session() now covers the three lifecycles a recycled
session can be in: same-loop closes keep a strong task reference until
completion; sessions owned by a loop running elsewhere are closed on
their own loop via run_coroutine_threadsafe; sessions whose loop is
gone are disposed synchronously through the connector teardown that
aiohttp's own finalizer uses, which releases pooled connections and
silences the finalizer warnings.
Fixes BerriAI#24230
f29611b to
a471775
Compare
|
@greptileai review the latest commit please - addressed all three findings: the retry handler now disposes the local faulted session instead of self.client (with a concurrency regression test), stopped-but-not-closed foreign loops take the synchronous teardown path, and the degraded private-API path logs instead of silently no-op'ing. |
|
@greptileai review |
|
Hi @yassin-berriai, it would be great if you could review this, to unblock us! Thanks! |
Relevant issues
Fixes #24230
Builds on the analysis in #24231 (credit to @alilxxey for the report and the first PR, closed as stale with an unsigned CLA). This PR additionally disposes of sessions in the loop-inspection fallback branch and handles the cross-loop lifecycles that were previously left to GC.
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewScreenshots / Proof of Fix
1. Verbatim repro script from #24230
Before (current main / v1.88.1):
After (this branch):
2. End-to-end A/B harness - real HTTP requests through
LiteLLMAiohttpTransport(viahttpx.AsyncClient(transport=...)) against a local aiohttp server, across 20 event-loop churn cycles (asyncio.runper cycle, module-cached transport, like proxy/eval workloads), with 5 loop-inspection failures induced exactly as in the issue repro:Unclosed client session/Unclosed connectordumps on stderrThe same stderr dumps are what production deployments see as ERROR-severity
asynciolog entries (our GKE deployment surfaced them daily; Cloud Logging classifies them red).3. Test suite
pytest tests/test_litellm/llms/custom_httpx/test_aiohttp_transport.py-> 24 passed (5 new regression tests; the two GC-behavior tests fail on current main and pass with the fix)pytest tests/test_litellm/llms/custom_httpx/-> 174 passedruff check/ruff format --checkclean on both changed filesType
🐛 Bug Fix
Changes
LiteLLMAiohttpTransportreplaced its cachedaiohttp.ClientSessionin three places without reliably closing the previous session:asyncio.create_task(old_session.close())discarded the task reference, so the close task could be garbage-collected before it ran;(RuntimeError, AttributeError)fallback branch: replaced the session without closing it at all (the exact repro in [Bug]: LiteLLMAiohttpTransport can leak recycled aiohttp ClientSession instances #24230);This PR adds one disposal helper,
_close_recycled_session(), used by all three replacement sites (loop-mismatch, fallback branch, and the "Session is closed" retry inhandle_async_request). It handles the three lifecycles a recycled session can be in:asyncio.run_coroutine_threadsafe;BaseConnector._close), which releases pooled connections and flips theclosedflags thatClientSession.__del__/BaseConnector.__del__check - so nothing is left for the GC to report.No public API changes; the happy path (valid session on the current loop) is untouched.