fix(redis): reset only the failed node on a cluster client timeout, not the whole client - #37863
Conversation
|
|
Greptile SummaryThe PR changes async Redis Cluster timeout handling so only the failed node is disconnected while topology-related errors retain full-cluster reinitialization
Confidence Score: 5/5The PR appears safe to merge No blocking failure remains
|
| Filename | Overview |
|---|---|
| litellm/_redis.py | Async Redis Cluster construction now uses the node-isolating subclass factory |
| litellm/caching/redis_cluster_node_isolation.py | Adds the Redis Cluster override that limits connection-error cleanup to the failed node |
| tests/test_litellm/caching/test_redis_cluster_node_isolation.py | Adds typed regression coverage for node-local resets and unchanged cluster-level error behavior |
| tests/test_litellm/test_redis.py | Updates cluster-construction mocks to patch the new subclass factory |
Reviews (3): Last reviewed commit: "fix(redis): reset only the failed node o..." | Re-trigger Greptile
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@greptileai please review the current head 042d7d5 — addressed the typing feedback on the test scaffolding. |
…ot the whole client A ConnectionError/TimeoutError on one node of the async Redis Cluster client made redis-py tear down every node's connections and force every other concurrent caller through the shared reinit lock, turning one client-side timeout under event-loop saturation into a proxy-wide latency spike while Redis itself stayed healthy. Confirmed live against a local 3-master cluster: pausing one node made 100% of concurrent commands to the other two, untouched nodes stall for the full pause duration; after this change, zero. LiteLLMAsyncRedisCluster overrides only the ConnectionError/TimeoutError branch of _execute_command to reset the one node that failed, mirroring what a plain non-cluster Redis client already does when a pooled connection errors. Every other branch (MOVED, ASK, CLUSTERDOWN, slot-not-covered) is unchanged, since those already carry real evidence the topology changed.
042d7d5 to
368abcd
Compare
|
@greptileai please review the current head 368abcd — rebased onto litellm_internal_staging (picks up the #37864 lint fix) and squashed into a single commit. |
TLDR
Problem this solves:
How it solves it:
User Flow
Before: a customer's proxy pods share one Redis Cluster connection; when one node's response is briefly slow under load, every concurrent request stalls for seconds, not just the ones talking to that node
POST https://proxy-domain/v1/chat/completionswhile one Redis Cluster node is briefly slow to respond (e.g. under host contention)After: only requests actually waiting on the transiently slow node are affected; every other request keeps its normal latency
Relevant issues
Linear ticket
Resolves LIT-5836
Pre-Submission checklist
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
This is a Redis Cluster client-internals fix with no LLM call in its path, so there is no provider spend to demonstrate; the proof instead drives the exact
redis.asynciocluster client construction function the proxy itself calls (litellm._redis.get_redis_async_client), against a real, unmocked local 3-master/3-replica Redis Cluster (redis-server --cluster-enabled yes, no mocks). One master is paused with the real RedisCLIENT PAUSEcommand to force a genuine client-side socket timeout while the server itself stays healthy, then the run measures whether concurrent traffic against the other two, untouched masters is affected.Setup (shared by both runs): a local 3-master/3-replica cluster on
127.0.0.1:27000-27005, socket_timeout=1.0s matching the customer's config, one task looping commands against the node about to be paused (27000) and 40 tasks looping commands against the two untouched masters (27001/27002).Before (9821b45)
python3 repro_storm.pybuilds the cluster client viaget_redis_async_client, pauses node 27000 for 3000ms withCLIENT PAUSE, then drives all 41 tasks concurrently for 4.5sclient type: <class 'redis.asyncio.cluster.RedisCluster'>(the unpatched upstream client)RedisCluster.aclose() was invoked 13 time(s) during the run (each tears down every node's connections)40 tasks hitting HEALTHY, unpaused nodes (27001/27002): ... healthy-node calls >1s: 40/65303andmax single-call latency observed on a healthy-node task: 2.081s— every one of the 40 tasks against the untouched nodes was hit with a multi-second stall caused entirely by node 27000's pauseAfter (368abcd)
client type: <class 'litellm.caching.redis_cluster_node_isolation.get_litellm_async_redis_cluster_class.<locals>.LiteLLMAsyncRedisCluster'>RedisCluster.aclose() was invoked 0 time(s) during the run (each tears down every node's connections)40 tasks hitting HEALTHY, unpaused nodes (27001/27002): ... healthy-node calls >1s: 0/116359andmax single-call latency observed on a healthy-node task: 0.007s— zero collateral impact on the untouched nodes; the paused node's own task still correctly waits out the pause and succeedsType
🐛 Bug Fix
Caveats (if any)
RedisCluster._execute_command); a version guard logs a warning if the installed redis-py version falls outside the set this override was verified againstFinal Attestation