Repository navigation
fix(redis): count pool wait timeouts as breaker timeouts - #40672
mateo-berri wants to merge 4 commits into
Conversation
redis-py's blocking pool reports a saturated pool as ConnectionError chained from asyncio.TimeoutError. The circuit breaker classified that as a hard connectivity failure and opened at once while Redis was healthy. Follow the explicit cause chain so it counts as a timeout and stays behind the timeout_min_duration gate
…itellm_redis_pool_timeout_counts_as_timeout
Greptile SummaryThis PR updates Redis circuit-breaker timeout classification to follow explicit exception causes, allowing saturated connection-pool waits to respect the configured timeout-duration gate. The latest revision also classifies Confidence Score: 5/5The PR appears safe to merge, with no outstanding correctness, security, or repository-rule issues identified Pool wait errors chained from timeouts are now duration-gated, while refused or reset connections remain immediate connectivity failures
|
| Filename | Overview |
|---|---|
| litellm/caching/redis_cache.py | Adds bounded explicit-cause traversal and cross-version asyncio timeout classification |
| tests/test_litellm/caching/test_redis_cache.py | Adds focused regression coverage for saturated pool waits and exception-chain classification |
Reviews (3): Last reviewed commit: "fix(redis): treat asyncio.TimeoutError a..." | Re-trigger Greptile
The recursion detector flags any unignored recursive function, so the timeout classification now walks the explicit cause chain with a bounded generator instead of calling itself
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Timeout check misses 3.10 asyncio errors
- The timeout classifier now includes asyncio.TimeoutError so 3.10 pool-wait ConnectionErrors stay behind the duration gate, with a regression test that simulates that distinct class on 3.12 CI.
Or push these changes by commenting:
@cursor push c4dd57b515
Preview (c4dd57b515)
diff --git a/tests/test_litellm/caching/test_redis_cache.py b/tests/test_litellm/caching/test_redis_cache.py
--- a/tests/test_litellm/caching/test_redis_cache.py
+++ b/tests/test_litellm/caching/test_redis_cache.py
@@ -1262,3 +1262,36 @@
assert _is_redis_timeout_failure(chained.value) is True
assert _is_redis_timeout_failure(contextual.value) is False
assert _is_redis_timeout_failure(RedisConnectionError("refused")) is False
+
+
+def test_py310_asyncio_timeout_error_cause_is_a_timeout_failure(monkeypatch):
+ """On py3.10 asyncio.TimeoutError is not TimeoutError, so listing only TimeoutError misses it.
+
+ CI is 3.12, where the two names are the same class and the existing cause-chain test would
+ pass without the explicit entry. Substitute a distinct class to lock the 3.10 path.
+ """
+ import importlib
+
+ from redis.exceptions import ConnectionError as RedisConnectionError
+
+ from litellm.caching.redis_cache import _is_redis_timeout_failure, _redis_timeout_error_types
+
+ class Py310AsyncioTimeoutError(Exception):
+ pass
+
+ redis_cache = importlib.import_module("litellm.caching.redis_cache")
+ _redis_timeout_error_types.cache_clear()
+ monkeypatch.setattr(redis_cache.asyncio, "TimeoutError", Py310AsyncioTimeoutError)
+
+ def raise_pool_wait_timeout() -> None:
+ try:
+ raise Py310AsyncioTimeoutError()
+ except Py310AsyncioTimeoutError as err:
+ raise RedisConnectionError("No connection available.") from err
+
+ try:
+ with pytest.raises(RedisConnectionError) as chained:
+ raise_pool_wait_timeout()
+ assert _is_redis_timeout_failure(chained.value) is True
+ finally:
+ _redis_timeout_error_types.cache_clear()You can send follow-ups to the cloud agent here.
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit cf1f709. Configure here.
|
Closing this so Devin can reopen the same branch and commits under its own authorship, which lets me approve it myself |
1 participant
TLDR
Problem this solves:
ConnectionErrorraised from a timeoutHow it solves it:
asyncio.TimeoutErroris listed as a timeout explicitly, since on Python 3.10 it is not yet an alias ofTimeoutErrorUser Flow
Before: an operator running two proxy instances with Redis caching sees caching go dark for a minute the moment a burst fills the Redis connection pool, even though Redis is healthy
cache: true,cache_params.type: redis,max_connections: 2, and a 1 s pool wait timeout, both pointed at one Redis reached over a slow linkPOST https://litellm-domain/v1/chat/completionsrequests forhaikuat 40-way concurrency, alternating between the two instances, and every one returns 200 with a freshchatcmpl-...idRedis circuit breaker OPENED after 5 consecutive failures (5 hard connectivity)warning and fast-fails Redis calls for 60 s, whileredis-cli PINGagainst that Redis keeps answeringPONGchatcmpl-...ids and nox-litellm-cache-keyheader, so nothing is cached on any worker for the next 60 sAfter: the same burst leaves the breaker closed, and a response cached through one instance is served by the other
cache: true,cache_params.type: redis,max_connections: 2, and a 1 s pool wait timeout, both pointed at one Redis reached over a slow linkPOST https://litellm-domain/v1/chat/completionsrequests forhaikuat 40-way concurrency, alternating between the two instances, and every one returns 200 with a freshchatcmpl-...idRedis circuit breaker OPENEDline, whileredis-cli PINGagainst that Redis keeps answeringPONGchatcmpl-...id, and B answers 200 with that same id plus anx-litellm-cache-keyheader, so the response cached through A came back from BRelevant issues
None, tracked in Linear only
Linear ticket
Resolves LIT-7542
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
uv run pytest tests/test_litellm/<your_test_file>.py -v. Leave the suites (make test-unit-*,make test-unit) to CI: it finishes in ~15 minutes where a laptop takes an hour or more@greptileaito re-request a review after pushing changes)Delays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
Rig shared by both legs: a real local
redis-serverbehind a small TCP relay that adds 0.3 s of delay per direction, so every Redis round trip takes about 0.75 s (time redis-cli -p 24775 PINGreports 0.76 s), a cache pool capped at 2 connections, and a 1 s pool wait timeout. That is the ticket's shape: a healthy but slow Redis whose pool runs dry under a burst. Both legs boot two proxy instances, each with--num_workers 2, sharing that Redis, and every request alternates between the two instances. Real Anthropic spend onclaude-haiku-4-5(x-litellm-response-cost: 3.6e-05per uncached call)relay.py
config.yaml:Two proxy instances per leg, A and B, each started as (ports are random and named per leg below):
The burst: 300 chat completions at 40-way concurrency, odd requests to A and even to B (
req.shis the curl below, printing status, latency, and port). The Redis sampler runs alongside it, once a secondThe cache probe right after the burst, run twice: the same prompt to A and then to B, printing the response id and the cache header
Before (ff4b558)
Instance A on port 35998 and instance B on port 32205, then the burst. Every request succeeded, and the burst ran from 20:19:57 to 20:20:04
Redis stayed healthy the whole time, answering every PING with the same 13 clients connected
The breaker opened on all four workers within 1 to 4 s of the burst starting, counting the pool waits as hard connectivity failures
The cache probe, twice in a row: four fresh ids, no cache key, full provider cost every time. Caching is dark on every worker while Redis is up
After (cf1f709)
Instance A on port 55096 and instance B on port 24274, then the burst. Every request succeeded, and the burst ran from 21:00:04 to 21:00:19
Redis stayed healthy the whole time, answering every PING with the same 13 clients connected
The breaker never opened on any worker
The cache probe, twice in a row: A misses once and caches, then B and every later call serve that same id with the cache key
Type
🐛 Bug Fix
Caveats (if any)
Medium
max_connectionsfor the burst avoids bothLow
raise ... fromchain is inspected, not__context__BlockingConnectionPoolnot covered; LiteLLM only uses the async onex-litellm-response-cost; pre-existingfailure_class="timeout"onlitellm_redis_circuit_breaker_failures_total, not"connectivity"; alerts keyed on the old label need updatinguv run --no-project --python 3.10 --with redis==5.3.1 --with fakeredis==2.26.2 python check.py, wherecheck.pysaturates a one-connectionBlockingConnectionPooland inspects the pool wait error's__cause__cause type: asyncio.exceptions.TimeoutError,isinstance(cause, (RedisTimeoutError, TimeoutError)): False,isinstance(cause, asyncio.TimeoutError): Truecause type: builtins.TimeoutError, both checksTrueFinal Attestation
Note
Medium Risk
Changes how Redis health failures are classified for the shared circuit breaker and Prometheus
failure_classlabels; misclassification could delay opening on real outages or change alerting keyed onconnectivity.Overview
Fixes the Redis circuit breaker treating saturated connection-pool waits as immediate hard connectivity failures when Redis is still healthy.
_is_redis_timeout_failurenow walks the exception’s explicitraise … fromchain (via_explicit_causes) and classifies a failure as a timeout if any link matches the timeout types. That covers redis-py’s asyncBlockingConnectionPoolpattern:ConnectionError("No connection available.")chained fromasyncio.TimeoutError, which should follow the timeout path (duration-gated) instead of opening the breaker at once._redis_timeout_error_typesalso listsasyncio.TimeoutErrorexplicitly for Python 3.10, where it is not yet an alias ofTimeoutError.Tests assert a busy one-connection pool does not open the breaker after many failures, and that only explicit causes count—not incidental
__context__from handling a timeout.Reviewed by Cursor Bugbot for commit cf1f709. Bugbot is set up for automated code reviews on this repo. Configure here.