fix(redis): stop an unreachable Redis from blocking every request - #35273
Conversation
|
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile SummaryThe PR prevents unreachable Redis instances from indefinitely blocking requests.
Confidence Score: 5/5The PR appears safe to merge. The ContextVar-based tracking isolates each asynchronous call’s swallowed failures, so the previously reported concurrent-success issue no longer remains, and no blocking failure remains.
|
| Filename | Overview |
|---|---|
| litellm/_redis.py | Derives URL-compatible kwargs from Redis connection classes and preserves timeout settings when constructing URL-based clients and pools. |
| litellm/caching/redis_cache.py | Adds per-context swallowed-failure tracking and routes guarded methods and Lua execution through a shared circuit-breaker helper. |
| tests/test_litellm/caching/test_redis_cache.py | Covers swallowed failures, successful resets, concurrent success/failure isolation, Lua execution, and health-error classification. |
| tests/test_litellm/test_redis.py | Verifies timeout propagation and rejection of unsupported kwargs across URL and host/port configurations. |
Reviews (3): Last reviewed commit: "fix(redis): stop an unreachable Redis fr..." | Re-trigger Greptile
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
43101bc to
ec8f7b8
Compare
|
The concurrency finding was correct, thanks. I reproduced it before changing anything: two calls in flight against one breaker, where the failure lands while the other call is still awaiting, and a Redis that answered a call in every single round was still evicted after 5 rounds. Root cause was the marker itself. Fixed by moving the per-call marker to a Same scenario after the change: Also added @greptileai please review the current head |
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 1 · PR risk: 0/10 |
Two defects combined to make a Redis outage take the proxy down rather than degrade it. First, connection kwargs were dropped whenever Redis was configured by url. _get_redis_url_kwargs built its allowlist from inspect.getfullargspec(redis.Redis.from_url); from_url is declared (cls, url, **kwargs), so the argspec carried no connection kwargs and the function returned ['cls', 'url', 'url']. socket_timeout went with the rest, and socket_connect_timeout falls back to it, so both ended up None and a Redis host that drops packets rather than refusing them blocked callers indefinitely. get_redis_connection_pool's url branch lost the same kwargs by a different route, rebuilding its pool kwargs from scratch. The allowlist now comes from the connection class redis-py actually forwards those kwargs to, walking the MRO because redis-py splits them between AbstractConnection and its subclasses. Deriving it from the client instead would admit client-only settings such as single_connection_client and the SSLConnection-only ssl_* family, which reach AbstractConnection and raise TypeError on first connect. Second, the circuit breaker could not trip even once calls failed fast. _redis_circuit_breaker_guard inferred success from the method returning, but async_get_cache, async_batch_get_cache, async_set_cache, async_set_cache_pipeline, async_set_cache_sadd and async_get_ttl catch their own connection errors and return a default so callers degrade. Each failed call therefore reset the failure streak and the breaker never opened, so an unreachable Redis stayed in the pool and every request kept paying a full socket timeout on it. Those methods now mark the failure and the guard records success only when nothing failed while the method ran. Lua script execution went through none of this, which mattered most because the rate limiter issues all of its Redis traffic that way, so the guard is now a small helper shared by both. The per-call marker is a ContextVar rather than a counter on the breaker. Breakers are shared by every concurrent caller, so a shared counter cannot tell "my call failed" from "some other in-flight call failed", and a success overlapping someone else's failure would be discarded until a Redis that was still answering got evicted from the pool anyway. Only connectivity failures feed the breaker. Command and data errors say nothing about whether Redis is reachable, and counting them would let a caller provoke evictions on demand (an INCR against a non-numeric value, say), dropping rate limiting to per-process counters that spreading traffic across replicas can outrun.
ec8f7b8 to
196ae5d
Compare
|
Took the veria finding about request errors opening the shared breaker; it is correct and it is on this PR. Before these changes the breaker could never open at all, so the path was unreachable in practice. Making it functional is exactly what makes it reachable, which is my regression to close, not a pre-existing one to wave through. The distinction I went with is connectivity vs request:
Note The import of Covered by @greptileai please review the current head |
3 participants
TLDR
Problem this solves:
How it solves it:
Relevant issues
Linear ticket
Resolves LIT-4930
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@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
Live proxy on a real Postgres, calling the real OpenAI API. Redis is a real server that gets frozen mid-flight with
docker pause, which is what a server under maintenance looks like from the client's side: the TCP connection stays open and nothing ever replies. Stopping the container instead would be a much weaker test, since a refused connection fails instantly and none of this code is exercised.Config used for both runs (proxy-level cache and coordination Redis both on a url, which is the shape that was broken):
Before, at
4d54324515. Requests are sent with an rpm-limited key so the coordination path is live. Redis is healthy for the first two, then paused:The proxy never answers again. What makes this hard to catch in production is that it still looks alive:
A load balancer keeps the pod in rotation, and every inference request hangs.
Starting a proxy while Redis is already unreachable was equally fatal, because
RedisCache.__init__makes a synchronous call with the same missing timeout:After, at
196ae5ddd6. Same config, same script, same paused Redis:Every request is answered. The first two pay the socket timeout while the failures accumulate, then the breaker opens and latency returns to the healthy baseline of roughly one second with Redis skipped entirely:
Request 7 is a breaker half-opening after their 60s recovery window and sending a probe through. That is the intended design, and it is why the outage numbers are uneven rather than flat: each cache holds its own breaker, so during a sustained outage most requests are served from memory at roughly a second while an occasional probe pays one timeout to find out whether Redis is back.
Unpausing Redis puts it back in the pool on the next probe, so the degradation is not sticky:
Startup with Redis already unreachable now completes instead of hanging:
The isolated mechanism, measured against a blackholed host (
10.255.255.1, packets dropped) so the two config styles can be compared directly:Note on behaviour during an outage: rate limiting falls back to per-worker in-memory counters, so limits fail open rather than rejecting traffic. That is the existing behaviour of the v3 limiter's fallbacks, not something this PR changes, but it is worth stating explicitly since this PR is what makes that fallback actually reachable. For the same reason only connectivity failures feed the breaker; counting command and data errors would let a caller provoke that fallback on demand.
Type
🐛 Bug Fix
Changes
Two independent defects had to line up for a Redis outage to take the proxy down rather than degrade it.
Connection kwargs were dropped on url configs.
_get_redis_url_kwargsbuilt its allowlist frominspect.getfullargspec(redis.Redis.from_url).from_urlis declared(cls, url, **kwargs), so the argspec carries no connection kwargs at all and the function returned['cls', 'url', 'url']. Everything was stripped,socket_timeoutincluded, andsocket_connect_timeoutfalls back tosocket_timeout, so both ended upNoneand redis-py blocked until the OS gave up.get_redis_connection_poollost the same kwargs by a different route, rebuilding its pool kwargs from scratch as{timeout, url, max_connections}. Host/port configs were unaffected, which is why this survived so long.The allowlist now comes from the connection class that redis-py ultimately hands those kwargs to, walking the MRO because redis-py splits them between
AbstractConnectionand its subclasses. Taking the client's signature instead would look reasonable and be wrong: it admits client-only settings such assingle_connection_clientandauto_close_connection_pool, plus thessl_*family that onlySSLConnectionaccepts, and all of those reachAbstractConnectionand raiseTypeErroron first connect. TLS on a url config comes from therediss://scheme, which selectsSSLConnectionon its own.The circuit breaker could not open.
_redis_circuit_breaker_guardinferred health from the method returning. Butasync_get_cache,async_batch_get_cache,async_set_cache,async_set_cache_pipeline,async_set_cache_saddandasync_get_ttlall catch their own connection errors and return a default so callers degrade instead of failing, which is correct behaviour that happens to be indistinguishable from success at the decorator boundary. Every failed call therefore reset the failure streak, the threshold was never reached, and an unreachable Redis stayed in the pool with every request paying a full timeout on it. Measured directly: six consecutive failing calls leftfailure_count=0, state=closed.Those methods now record the failure, and success is recorded only when nothing failed while the method ran. Separately, Lua execution bypassed the breaker entirely, which mattered most because the v3 rate limiter issues all of its Redis traffic that way; the guard is now a small module-level helper shared by the decorator and the script executor.
QA runbook
Final Attestation