fix(http_handler): dispose aiohttp session when AsyncHTTPHandler is finalized without a running loop - #36670
Conversation
Greptile SummaryThis PR adds a loop-less synchronous disposal path for handler-owned aiohttp sessions and retains finalizer-scheduled asynchronous close tasks until completion.
Confidence Score: 4/5The PR appears safe to merge after addressing the non-blocking cleanup-task error handling and repository-guidance concerns. The synchronous aiohttp teardown is supported by the pinned connector API and ownership checks, but failed asynchronous close tasks currently produce unretrieved-task warnings, while the task registry and comments conflict with repository conventions. Files Needing Attention: litellm/llms/custom_httpx/http_handler.py
|
| Filename | Overview |
|---|---|
| litellm/llms/custom_httpx/http_handler.py | Adds synchronous aiohttp finalizer cleanup and close-task retention; close-task exceptions are not retrieved, and the implementation conflicts with repository mutation/comment guidance. |
| tests/test_litellm/llms/custom_httpx/test_http_handler.py | Adds focused, local-only regression tests covering dead-loop cleanup, task retention, and session ownership without weakening existing assertions. |
Reviews (1): Last reviewed commit: "fix(http_handler): dispose aiohttp sessi..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…unning loop AsyncHTTPHandler.__del__ can only schedule an async close when a running event loop exists at finalization time; in any other context (worker threads whose loop has closed, sync contexts, interpreter shutdown) the RuntimeError from get_running_loop() is swallowed and the underlying aiohttp ClientSession is abandoned to GC, emitting 'Unclosed client session' / 'Unclosed connector' warnings. This is the disposal gap left after the recycle-time fix: clients created for short-lived event loops (the loop-id-keyed LLM client cache mints one handler per loop) are never recycled - they live and die with their loop, and their finalization is precisely the loop-less case. Fix: - no running loop: fall back to the connector's synchronous teardown via LiteLLMAiohttpTransport._mark_connector_closed - the same finalizer-safe path used for dead-loop recycles - honoring _owns_session so a shared session is never closed. - running loop: keep the async close, but hold a strong reference to the scheduled task until it completes (a bare create_task() result may be collected before running), mirroring _background_close_tasks. Tests: loop-less finalization closes a dead-loop session; running-loop finalization registers and drains the close task; the sync fallback respects session ownership. All three fail without the fix.
Final on the five never-rebound locals (LIT010); the class-level task registry keeps its mutable set with the sanctioned mutable-ok reason, mirroring the aiohttp transport's registry (LIT001).
21a8b22 to
04891b7
Compare
The handler deliberately reuses the transport's finalizer-safe connector teardown; no public seam exists and an async close can never run at loop-less finalization. Clears the net-new reportPrivateUsage the basedpyright budget gate flagged once the LIT stage passed.
A bare discard done-callback dropped the task without consuming its exception, so a failing aclose() emitted "Task exception was never retrieved" at GC, the same noise class this path exists to remove. Mirror the transport's _on_close_task_done: discard, early-return on cancellation, retrieve and debug-log the exception.
Title
fix(http_handler): dispose aiohttp session when
AsyncHTTPHandleris finalized without a running loopRelevant issues
Follow-up to the recycle-time disposal fix (#33428 / #32003). That fix covers sessions replaced by
_get_valid_client_session(); this PR covers the clients that are never recycled and still leak.Pre-Submission checklist
tests/test_litellm/directorymake test-unitType
🐛 Bug Fix
Changes
Problem.
AsyncHTTPHandler.__del__can only schedule an async close when a running event loop exists at finalization time:In any loop-less context — worker threads whose event loop has already closed, sync code paths, interpreter/worker shutdown —
get_running_loop()raises, the exception is swallowed, and the underlying aiohttpClientSessionis abandoned to GC, emittingUnclosed client session/Unclosed connectorwarnings.This is exactly the lifecycle of clients minted for short-lived event loops:
LLMClientCachekeys clients byid(running_loop), so each ephemeral loop gets its own handler; those handlers live and die with their loop and are only ever finalized loop-lessly. Measured in production (FastAPI service running background eval workers with per-task event loops): a steady residual of these warnings survives the recycle-time fix, because these sessions never reach_get_valid_client_session()again.Fix (all inside
AsyncHTTPHandler):LiteLLMAiohttpTransport._mark_connector_closed— the same finalizer-safe path the transport already uses for dead-loop recycles. It releases pooled connections and flips the closed flags thatClientSession.__del__/ connector__del__check, so no warnings fire at GC. The fallback honors_owns_session, so a shared session (e.g. the proxy's) is never closed by a handler.create_task()result may be garbage-collected before it runs. MirrorsLiteLLMAiohttpTransport._background_close_tasks.Tests (
tests/test_litellm/llms/custom_httpx/test_http_handler.py):test_finalizer_without_running_loop_closes_dead_loop_session— a handler whose session was created on a since-closed loop is finalized with no running loop; the session must end up closed.test_finalizer_with_running_loop_schedules_close_and_holds_task_ref— the close task is registered, retained, and drains the registry on completion.test_sync_close_helper_respects_session_ownership— owned session closed; shared session untouched.All three fail without the fix and pass with it. Existing
custom_httpxsuites (test_http_handler.py,test_aiohttp_transport.py,test_async_client_cleanup.py,test_gemini_session_leak.py) pass: 98/98.Behavioral A/B on the repro (5 clients used on ephemeral loops, refs dropped with no loop running, forced GC): 10 unclosed-session warnings before → 0 after.