Repository navigation
fix(redis): count pool wait timeouts as breaker timeouts - #40764
mateo-berri merged 5 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
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
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Greptile SummaryThis PR updates Redis circuit-breaker classification so blocking-pool wait failures chained from timeout exceptions remain subject to the timeout-duration gate
Confidence Score: 5/5The PR appears safe to merge, with focused classification logic and regression coverage for the reported pool-saturation behavior No actionable failure remains; the breaker still distinguishes hard connectivity failures while recognizing the explicit timeout cause emitted for exhausted async blocking pools
|
| Filename | Overview |
|---|---|
| litellm/caching/redis_cache.py | Adds bounded explicit-cause traversal so Redis pool wait failures are classified as timeouts without treating implicit exception context as one |
| tests/test_litellm/caching/test_redis_cache.py | Adds focused regression tests covering a saturated async blocking pool and explicit-cause-only timeout detection |
Reviews (1): Last reviewed commit: "fix(redis): treat asyncio.TimeoutError a..." | Re-trigger Greptile
…itellm_redis_pool_timeout_counts_as_timeout # Conflicts: # tests/test_litellm/caching/test_redis_cache.py
|
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 51abb95. Configure here.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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 alert behavior, but scope is limited to timeout vs connectivity handling inredis_cache.py.Overview
Fixes false Redis circuit breaker opens when the async connection pool is saturated but Redis is still healthy.
_is_redis_timeout_failureno longer looks only at the outer exception type. It walks the bounded explicit__cause__chain (via new_explicit_causes) and treats the failure as a timeout if any link matches timeout types—includingasyncio.TimeoutError, which is now listed explicitly for Python 3.10 where it is not yet an alias ofTimeoutError. That matches redis-py’s pattern of raisingConnectionError("No connection available.")froma pool-wait timeout, so those errors count as timeout failures (subject totimeout_min_duration) instead of immediate hard connectivity trips.Tests add a fakeredis saturated
BlockingConnectionPoolscenario and assertraise ... fromvs contextualexcepthandling for classification.Reviewed by Cursor Bugbot for commit 51abb95. Bugbot is set up for automated code reviews on this repo. Configure here.
Reopened from #40672 with the same commits so the approval can come from someone other than the author