mirror of
https://github.com/BerriAI/litellm.git
synced 2026-10-10 03:28:53 +00:00
fix: don't close httpx clients on cache eviction (fixes 'client has been closed' errors)
Root cause: LLMClientCache._remove_key() was calling aclose()/close() on
httpx clients when their cache entries expired (TTL) or were evicted (LRU).
However, callers of get_async_httpx_client() hold direct references to
these client objects (e.g. litellm.module_level_aclient, streaming handlers,
passthrough request handlers). Closing the underlying httpx client while
these references are still in use causes:
RuntimeError: Cannot send a request, as the client has been closed.
This was introduced in commits 6931fea9 and bfeed0e5 (Feb 20, 2026) which
added close-on-eviction to prevent connection pool leaks. The fix removes
the close-on-eviction behavior: when a cache entry is evicted, only the
cache's reference is dropped. Python GC will finalize transport/connection
pool resources when all external references are released.
The bug manifests after ~1 hour of traffic (_DEFAULT_TTL_FOR_HTTPX_CLIENTS
= 3600s) when cache entries expire, or earlier under high concurrency if
the 200-entry LRU limit is reached. It affects all providers using the
shared httpx client cache (Vertex AI, Bedrock, etc.).
Co-authored-by: Ishaan Jaff <ishaan-jaff@users.noreply.github.com>
This commit is contained in:
parent
ae13a40c01
commit
eb1a13f7a7
3 changed files with 174 additions and 20 deletions
|
|
@ -9,23 +9,20 @@ from .in_memory_cache import InMemoryCache
|
|||
|
||||
class LLMClientCache(InMemoryCache):
|
||||
def _remove_key(self, key: str) -> None:
|
||||
"""Close async clients before evicting them to prevent connection pool leaks."""
|
||||
value = self.cache_dict.get(key)
|
||||
"""Remove key from cache WITHOUT closing the client.
|
||||
|
||||
Callers of get_async_httpx_client / _get_httpx_client hold direct
|
||||
references to the returned client objects (e.g. litellm.module_level_aclient,
|
||||
streaming handlers, passthrough request handlers). If we close the
|
||||
underlying httpx client here, those callers will receive:
|
||||
|
||||
RuntimeError: Cannot send a request, as the client has been closed.
|
||||
|
||||
Instead we simply drop the cache's own reference. When all external
|
||||
references are released the normal Python GC cycle will finalize the
|
||||
transport / connection-pool resources.
|
||||
"""
|
||||
super()._remove_key(key)
|
||||
if value is not None:
|
||||
close_fn = getattr(value, "aclose", None) or getattr(
|
||||
value, "close", None
|
||||
)
|
||||
if close_fn and asyncio.iscoroutinefunction(close_fn):
|
||||
try:
|
||||
asyncio.get_running_loop().create_task(close_fn())
|
||||
except RuntimeError:
|
||||
pass
|
||||
elif close_fn and callable(close_fn):
|
||||
try:
|
||||
close_fn()
|
||||
except Exception:
|
||||
pass
|
||||
|
||||
def update_cache_key_with_event_loop(self, key):
|
||||
"""
|
||||
|
|
|
|||
|
|
@ -132,9 +132,10 @@ async def test_disconnect_idempotent():
|
|||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_eviction_calls_aclose():
|
||||
async def test_eviction_does_not_close_client():
|
||||
"""When an async client is evicted from LLMClientCache, its aclose()
|
||||
should be scheduled via create_task."""
|
||||
must NOT be called — callers may still hold references to the client.
|
||||
Closing it would cause 'Cannot send a request, as the client has been closed.'"""
|
||||
cache = LLMClientCache(max_size_in_memory=2, default_ttl=600)
|
||||
|
||||
client = AsyncMock()
|
||||
|
|
@ -145,10 +146,10 @@ async def test_eviction_calls_aclose():
|
|||
# Third insert triggers eviction of client-0
|
||||
cache.set_cache(key="trigger", value="y")
|
||||
|
||||
# Let the scheduled task run
|
||||
# Let any scheduled tasks run
|
||||
await asyncio.sleep(0.05)
|
||||
|
||||
assert client.aclose.await_count > 0
|
||||
client.aclose.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
|
|
|
|||
|
|
@ -0,0 +1,156 @@
|
|||
"""
|
||||
Test that httpx clients are NOT closed when evicted from the LLMClientCache.
|
||||
|
||||
Reproduces the bug: "Cannot send a request, as the client has been closed."
|
||||
Root cause: LLMClientCache._remove_key() was closing async/sync httpx clients
|
||||
on cache eviction (TTL expiry or LRU eviction), but callers still held
|
||||
references to those clients.
|
||||
|
||||
See: https://github.com/BerriAI/litellm/issues/XXXX
|
||||
"""
|
||||
|
||||
import asyncio
|
||||
import os
|
||||
import sys
|
||||
import time
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
|
||||
import httpx
|
||||
import pytest
|
||||
|
||||
sys.path.insert(0, os.path.abspath("../../../.."))
|
||||
|
||||
import litellm
|
||||
from litellm.caching.llm_caching_handler import LLMClientCache
|
||||
from litellm.llms.custom_httpx.http_handler import (
|
||||
AsyncHTTPHandler,
|
||||
HTTPHandler,
|
||||
get_async_httpx_client,
|
||||
)
|
||||
|
||||
|
||||
class TestLLMClientCacheEvictionDoesNotCloseClients:
|
||||
"""Verify that cache eviction does not close clients that may still be in use."""
|
||||
|
||||
def test_async_client_not_closed_on_ttl_expiry(self):
|
||||
"""
|
||||
Simulate TTL expiry: a cached async client should NOT be closed when
|
||||
its cache entry expires, because callers may still hold a reference.
|
||||
"""
|
||||
cache = LLMClientCache()
|
||||
|
||||
mock_client = AsyncMock(spec=AsyncHTTPHandler)
|
||||
mock_client.close = AsyncMock()
|
||||
mock_client.aclose = AsyncMock()
|
||||
|
||||
cache.set_cache("test_async_client", mock_client, ttl=1)
|
||||
|
||||
cached = cache.get_cache("test_async_client")
|
||||
assert cached is mock_client
|
||||
|
||||
time.sleep(1.5)
|
||||
|
||||
expired = cache.get_cache("test_async_client")
|
||||
assert expired is None
|
||||
|
||||
mock_client.close.assert_not_called()
|
||||
mock_client.aclose.assert_not_called()
|
||||
|
||||
def test_sync_client_not_closed_on_ttl_expiry(self):
|
||||
"""
|
||||
Simulate TTL expiry for a sync client: should NOT be closed on eviction.
|
||||
"""
|
||||
cache = LLMClientCache()
|
||||
|
||||
mock_client = MagicMock(spec=HTTPHandler)
|
||||
mock_client.close = MagicMock()
|
||||
|
||||
cache.set_cache("test_sync_client", mock_client, ttl=1)
|
||||
|
||||
cached = cache.get_cache("test_sync_client")
|
||||
assert cached is mock_client
|
||||
|
||||
time.sleep(1.5)
|
||||
|
||||
expired = cache.get_cache("test_sync_client")
|
||||
assert expired is None
|
||||
|
||||
mock_client.close.assert_not_called()
|
||||
|
||||
def test_client_not_closed_on_lru_eviction(self):
|
||||
"""
|
||||
When cache hits max_size and evicts LRU entries, evicted clients
|
||||
should NOT be closed.
|
||||
"""
|
||||
cache = LLMClientCache(max_size_in_memory=2)
|
||||
|
||||
client_a = AsyncMock(spec=AsyncHTTPHandler)
|
||||
client_a.aclose = AsyncMock()
|
||||
client_b = AsyncMock(spec=AsyncHTTPHandler)
|
||||
client_b.aclose = AsyncMock()
|
||||
client_c = AsyncMock(spec=AsyncHTTPHandler)
|
||||
client_c.aclose = AsyncMock()
|
||||
|
||||
cache.set_cache("client_a", client_a, ttl=3600)
|
||||
cache.set_cache("client_b", client_b, ttl=3600)
|
||||
cache.set_cache("client_c", client_c, ttl=3600)
|
||||
|
||||
client_a.aclose.assert_not_called()
|
||||
client_b.aclose.assert_not_called()
|
||||
client_c.aclose.assert_not_called()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_module_level_aclient_survives_cache_eviction(self):
|
||||
"""
|
||||
Reproduce the exact production scenario:
|
||||
1. get_async_httpx_client() creates a client and caches it
|
||||
2. The client is also stored as litellm.module_level_aclient
|
||||
3. Cache TTL expires, entry is evicted
|
||||
4. The module-level reference should still point to a USABLE client
|
||||
"""
|
||||
cache = LLMClientCache()
|
||||
|
||||
with patch.object(litellm, "in_memory_llm_clients_cache", cache):
|
||||
client = get_async_httpx_client(
|
||||
llm_provider="test_provider",
|
||||
params={"timeout": 10, "client_alias": "test"},
|
||||
)
|
||||
|
||||
assert client is not None
|
||||
assert not client.client.is_closed
|
||||
|
||||
for key in list(cache.cache_dict.keys()):
|
||||
cache.ttl_dict[key] = time.time() - 1
|
||||
|
||||
cache.evict_cache()
|
||||
|
||||
assert not client.client.is_closed, (
|
||||
"Client must NOT be closed after cache eviction - "
|
||||
"callers still hold references to it"
|
||||
)
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_async_httpx_client_returns_new_client_after_eviction(self):
|
||||
"""
|
||||
After cache eviction, get_async_httpx_client should return a new client
|
||||
while the old client remains usable (not closed).
|
||||
"""
|
||||
cache = LLMClientCache()
|
||||
|
||||
with patch.object(litellm, "in_memory_llm_clients_cache", cache):
|
||||
old_client = get_async_httpx_client(
|
||||
llm_provider="test_provider2",
|
||||
params={"timeout": 10, "client_alias": "test2"},
|
||||
)
|
||||
|
||||
for key in list(cache.cache_dict.keys()):
|
||||
cache.ttl_dict[key] = time.time() - 1
|
||||
|
||||
new_client = get_async_httpx_client(
|
||||
llm_provider="test_provider2",
|
||||
params={"timeout": 10, "client_alias": "test2"},
|
||||
)
|
||||
|
||||
assert old_client is not new_client, "Should create a new client after eviction"
|
||||
assert not old_client.client.is_closed, "Old client must remain usable"
|
||||
assert not new_client.client.is_closed, "New client must be usable"
|
||||
Loading…
Add table
Reference in a new issue