Skip to content

fix(redis): open the circuit breaker on RedisClusterException so a dead cluster cannot hang the worker - #36804

Draft
mubashir1osmani wants to merge 3 commits into
BerriAI:litellm_internal_stagingfrom
mubashir1osmani:litellm_fix_redis_cluster_breaker
Draft

fix(redis): open the circuit breaker on RedisClusterException so a dead cluster cannot hang the worker#36804
mubashir1osmani wants to merge 3 commits into
BerriAI:litellm_internal_stagingfrom
mubashir1osmani:litellm_fix_redis_cluster_breaker

Conversation

@mubashir1osmani

@mubashir1osmani mubashir1osmani commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

TLDR

Problem this solves:

  • A dead Redis Cluster left the proxy accepting HTTP but never responding
  • Staging e2e on 2026-08-13: 72 tests died on 60s ALB read timeouts
  • redis-py wraps connect timeouts as RedisClusterException (not TimeoutError), so the breaker never opened

How it solves it:

  • Count RedisClusterException as a Redis health failure so the breaker opens
  • After 5 misses, later requests skip Redis and return instead of retrying CLUSTER SLOTS
  • Apply fail-fast connect timeout / keepalive / health check on sync and async cluster clients

User Flow

image

Before: a caller whose proxy uses ElastiCache Serverless (cluster mode) hits a node that is not answering, and every request hangs until their client times out

  1. They send POST https://litellm-domain/v1/chat/completions (or POST /model/new) with a valid key
  2. The TCP connection is accepted; no HTTP status comes back
  3. After 60s their client raises a read timeout; /health/liveliness still returns 200
  4. The next request does the same hang — the proxy never takes Redis out of the path

After: the same dead cluster fails fast; the proxy keeps answering

  1. They send the same POST https://litellm-domain/v1/chat/completions or POST /model/new
  2. The first few Redis attempts fail in seconds (connect timeout), not the OS TCP timeout
  3. After consecutive cluster-unreachable errors, later requests skip Redis and return a normal HTTP response (chat 200 / management 2xx, using in-memory state)
  4. /health/liveliness stays 200; the worker is not wedged

Relevant issues

Staging e2e 2026-08-13 (litellm-e2e-1-0-0-main-20260813122400-h6rhc, rev 09889e1986faa7b97d1d213040aa442b2aa393f6): 72 failed, 442 passed, 57 skipped in 13105.79s. Every captured traceback was Read timed out. (read timeout=60.0) against internal-k8s-litellm-litellm-e7f4afb143-1355079179.us-east-1.elb.amazonaws.com. Gateway/backend logs after the window still showed Redis Cluster cannot be connected … Timeout connecting to server with zero circuit breaker OPENED lines.

Follows the hole left by #35273 / #31577 (LIT-4083): those covered TimeoutError/ConnectionError and async reconnect defaults, not RedisClusterException or the sync cluster client.

Linear ticket

Pre-Submission checklist

  • I have added meaningful tests
  • My PR passes all CI/CD checks (e.g., lint, format, unit tests)
  • My PR's scope is as isolated as possible; it only solves 1 specific problem
  • I have received a Greptile Confidence Score of at least 4/5 before requesting a maintainer review (Greptile reviews automatically once the PR is opened; only comment @greptileai to re-request a review after pushing changes)

Screenshots / Proof of Fix

Unit proof at 342cdf628e (this commit), twice:

pytest tests/test_litellm/caching/test_redis_cache.py::test_cluster_connect_timeout_swallowed_set_opens_breaker \
       tests/test_litellm/caching/test_redis_cache.py::test_only_connectivity_failures_open_the_breaker \
       tests/test_litellm/test_redis.py::test_async_cluster_sets_reconnect_defaults \
       tests/test_litellm/test_redis.py::test_sync_cluster_sets_reconnect_defaults \
       tests/test_litellm/test_redis.py::test_async_cluster_reconnect_defaults_are_overridable

10 passed in 0.56s. The new test constructs the production exception text (Redis Cluster cannot be connected … Timeout connecting to server), asserts it is a health failure, opens the breaker, and asserts the next call is skipped immediately.

A full staging e2e re-run is not in this PR. Redis itself was still unreachable on stage after the suite; this change only stops that from freezing the worker.

Type

🐛 Bug Fix

Caveats (if any)

  • Does not fix ElastiCache reachability (TLS / SG / serverless). Budget/RPM/spend e2e that need a live Redis can still fail, but as assertions, not 60s hangs.
  • Stage was 1 replica × NUM_WORKERS=1 during the incident; that is litellm-ops, not this PR.

Final Attestation

  • The tests check the right things, including the edge cases, and regressions in the respective real-world customer use-cases are not possible after this PR

Note

Cursor Bugbot is generating a summary for commit 7ee0b70. Configure here.

… cluster cannot hang the worker

redis-py wraps CLUSTER SLOTS connect timeouts as RedisClusterException
(subclasses Exception, not TimeoutError). The circuit breaker ignored
them, so every cache write retried an unreachable ElastiCache node and
the 2026-08-13 staging e2e suite died on 60s ALB read timeouts.

Count that exception as unhealthy, and apply fail-fast reconnect
defaults (connect timeout, keepalive, health check) on both sync and
async RedisCluster clients.
@greptile-apps

greptile-apps Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR makes dead Redis Cluster connections fail fast without treating all cluster exceptions as health failures.

  • Applies reconnect, health-check, keepalive, and timeout defaults consistently to synchronous and asynchronous cluster clients.
  • Classifies wrapped or message-only unreachable-cluster failures while leaving command, slot-layout, and pipeline exceptions outside breaker accounting.
  • Adds regression coverage for cluster timeout handling, breaker activation, excluded exception subclasses, and client defaults.

Confidence Score: 5/5

The PR appears safe to merge.

The previous broad exception-classification defect is resolved, and no blocking failure remains.

Important Files Changed

Filename Overview
litellm/_redis.py Centralizes Redis Cluster reconnect defaults and applies them to both synchronous and asynchronous clients without leaving a blocking issue related to the prior thread.
litellm/caching/redis_cache.py Replaces the broad RedisClusterException classification with cause-chain and unreachable-startup detection, resolving the previously reported false-positive breaker behavior.
tests/test_litellm/caching/test_redis_cache.py Adds coverage proving non-connectivity cluster exceptions do not open the breaker while wrapped and message-only startup connectivity failures do.
tests/test_litellm/test_redis.py Verifies reconnect defaults and explicit override behavior across synchronous and asynchronous Redis Cluster clients.

Reviews (2): Last reviewed commit: "fix(redis): do not treat RedisClusterExc..." | Re-trigger Greptile

Comment thread litellm/caching/redis_cache.py Outdated
…down

SlotNotCoveredError, CrossSlotTransactionError, and InvalidPipelineStack
share that base but are command/slot/pipeline errors on a live cluster.
Matching isinstance(RedisClusterException) opened the breaker and dropped
shared cache and rate limits to per-process state.

Detect the 2026-08-13 hang by walking __cause__ (redis-py raises
RedisClusterException from TimeoutError) and the unreachable-node message
when the cause is stripped.
Comment thread litellm/caching/redis_cache.py Outdated
@mubashir1osmani

Copy link
Copy Markdown
Collaborator Author

Addressed the base-class match: RedisClusterException is no longer in the health-failure tuple.

_is_redis_health_failure now:

  • still treats TimeoutError / ConnectionError / ClusterDownError / OSError as unhealthy
  • walks __cause__ / __context__ so redis-py's raise RedisClusterException(...) from TimeoutError counts
  • matches only the unreachable-node message (Redis Cluster cannot be connected) when the cause was stripped
  • leaves SlotNotCoveredError, CrossSlotTransactionError, InvalidPipelineStack, and a generic RedisClusterException("db must be 0") as command errors — they do not open the breaker

Tests: subclass counter-examples plus the production connect-timeout wrap; 12 passed / 2 skipped (those two subclass names are absent on this redis-py).

@veria-ai

veria-ai Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

PR overview

All previously flagged issues have been addressed. No open security concerns remain on this pull request.

Security review

No open security issues remain on this pull request.

Fixed/addressed: 1 · PR risk: 0/10

@mubashir1osmani

Copy link
Copy Markdown
Collaborator Author

@greptile-apps

@mubashir1osmani

Copy link
Copy Markdown
Collaborator Author

bugbot run

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 7ee0b70. Configure here.

@codecov

codecov Bot commented Aug 13, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.44444% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
litellm/caching/redis_cache.py 93.10% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

@codspeed-hq

codspeed-hq Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

✅ 31 untouched benchmarks


Comparing mubashir1osmani:litellm_fix_redis_cluster_breaker (0f66803) with litellm_internal_staging (c1310de)1

Open in CodSpeed

Footnotes

  1. No successful run was found on litellm_internal_staging (69b0296) during the generation of this report, so c1310de was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

…probe

Fail-fast skips Redis while the cluster is down, but the next probe must
not reuse the wedged NodesManager that hung on CLUSTER SLOTS. Evict the
cached async client (and rebuild the sync one) when the breaker opens and
again when the recovery probe starts, so ElastiCache coming back can
accept traffic again.
@mubashir1osmani

Copy link
Copy Markdown
Collaborator Author

Follow-up on this PR: fail-fast now also lets Redis come back.

When the breaker opens, and again when it enters HALF_OPEN, we drop the cached RedisCluster client (generation bump so Lua script executors miss cache too) and rebuild the sync client. The recovery probe then rediscovers CLUSTER SLOTS instead of poking the same dead NodesManager.

HTTP traffic still fail-opens (in-memory) while OPEN. After REDIS_CIRCUIT_BREAKER_RECOVERY_TIMEOUT (60s) one request probes a new client; success closes the breaker and shared RPM/spend/cache go through Redis again.

mubashir1osmani added a commit to mubashir1osmani/litellm that referenced this pull request Aug 13, 2026
@mubashir1osmani
mubashir1osmani force-pushed the litellm_fix_redis_cluster_breaker branch from 9e23700 to 0f66803 Compare August 13, 2026 20:42
@mubashir1osmani
mubashir1osmani marked this pull request as draft August 13, 2026 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant