fix(redis): re-establish async cluster connections after a node restart - #31577
Conversation
|
|
Greptile SummaryFixes a connection-resurrection bug in the async
Confidence Score: 5/5Safe to merge; the change is narrowly scoped to the async cluster construction path and uses setdefault so existing explicit config is never overwritten. The fix is minimal and well-targeted: two setdefault calls guarded by the existing allow-list, a new constant in the right file, and three mock-only unit tests. No existing tests are modified, no auth or data paths are touched, and user-supplied config always wins over the new defaults. No files require special attention. The sync cluster path (init_redis_cluster) is not updated, which may be worth revisiting as a follow-up.
|
| Filename | Overview |
|---|---|
| litellm/_redis.py | Adds health_check_interval and socket_keepalive to the async RedisCluster allow-list and applies resilient defaults via setdefault, so user-supplied config still wins. Sync path (init_redis_cluster) is unchanged. |
| litellm/constants.py | Adds REDIS_CLUSTER_HEALTH_CHECK_INTERVAL = 25 in the right place; hardcoded (no os.getenv), unlike most peer constants, but overridable via cache_params config. |
| tests/test_litellm/test_redis.py | Three new mock-only tests cover the allow-list addition, default application, and user-override path. No real network calls made. |
Reviews (3): Last reviewed commit: "fix(redis): re-establish async cluster c..." | Re-trigger Greptile
Greptile SummaryThis PR makes async Redis cluster clients recover better after cluster node restarts. The main changes are:
Confidence Score: 5/5The changes are narrowly scoped to Redis async cluster client construction and related allow-listing/tests. No correctness issues were identified in the modified Redis configuration path, and the tests cover both the new defaults and explicit override behavior.
What T-Rex did
Reviews (1): Last reviewed commit: "fix(redis): re-establish async cluster c..." | Re-trigger Greptile |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
fcee651 to
e7359a9
Compare
|
CI note: the three red checks are CircleCI jobs (litellm_router_testing, llm_translation_testing, proxy_pass_through_endpoint_tests) that are also red on several other open PRs against litellm_internal_staging right now, so they read as flaky infra rather than anything from this change. This PR only adds health_check_interval and socket_keepalive defaults to the async Redis cluster client, which none of those suites exercise. All GitHub Actions checks including lint are green, and Greptile is at 5/5 on the current commit. A maintainer rerun of the failed CircleCI jobs should clear them |
When redis_startup_nodes is set the async cluster client was built with no health check and no TCP keepalive, so a connection silently dropped by a cluster restart (e.g. ElastiCache Serverless maintenance) stayed in the pool and got reused while dead; the first command after the restart stalled in re-initialization until the LoggingWorker timeout cancelled it, surfacing as CancelledError then TimeoutError on the spend-counter path Build the async cluster client with a 25s health_check_interval and socket_keepalive so an idle connection is PING-validated and reconnected before reuse, and expose both through the cluster kwarg allow-list so an explicit value from config still wins Resolves LIT-4083
e7359a9 to
88d701f
Compare
…rt (BerriAI#31577) When redis_startup_nodes is set the async cluster client was built with no health check and no TCP keepalive, so a connection silently dropped by a cluster restart (e.g. ElastiCache Serverless maintenance) stayed in the pool and got reused while dead; the first command after the restart stalled in re-initialization until the LoggingWorker timeout cancelled it, surfacing as CancelledError then TimeoutError on the spend-counter path Build the async cluster client with a 25s health_check_interval and socket_keepalive so an idle connection is PING-validated and reconnected before reuse, and expose both through the cluster kwarg allow-list so an explicit value from config still wins Resolves LIT-4083
Relevant issues
Linear ticket
Resolves LIT-4083
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewScreenshots / Proof of Fix
Root cause
When
cache_params.redis_startup_nodesis set, litellm builds a redis-py asyncRedisClusteringet_redis_async_client. That client was constructed with no health check (health_check_interval=0, disabled) and no TCP keepalive (socket_keepalive=False). The async cluster client is created once and reused for the life of the process (cached inin_memory_llm_clients_cache/RedisClusterCache.redis_async_redis_cluster_client), so when a cluster restarts (the customer hit periodic ElastiCache Serverless maintenance restarts) the dropped connection sits in the pool and gets reused while dead. The first command after the restart stalls inRedisCluster.initialize(), and the spend-counter path runs that command under the LoggingWorker'sasyncio.wait_for, so the stall is cancelled and surfaces exactly as reportedThe change
Build the async cluster client with a 25s
health_check_intervalandsocket_keepalive=True, and let an explicit value from config still win. A non-zerohealth_check_intervalis redis-py's documented mechanism for exactly this: before reusing an idle connection it sends aPING, and a dead connection is re-established before the real command runsProof 1 - the constructed client now carries the resilience config (real litellm client, against a real cluster)
Same
get_redis_async_client(startup_nodes=...)the proxy uses, before and after the changeProof 2 - a non-zero health_check_interval actually re-validates an idle connection
Driving the real redis-py async cluster against a live single-node cluster, resetting the server command stats, idling past the interval, then issuing one
GET, and counting health-checkPINGs the server sawProof 3 - live proxy end to end on the failing path
Proxy launched with the cluster cache pointed at a live redis cluster, real Anthropic calls
Same response id on the second call with a 40x latency drop confirms the async cluster cache get/set path (the path in the stack trace) works end to end with the resilient client, and the proxy log shows no
CancelledError/ redis exceptionsA laptop cannot reproduce the precise ElastiCache restart-window stall (a local
docker restartcloses connections gracefully, so redis-py recovers immediately regardless), but the fix activates redis-py's documented idle-connection health check plus keepalive, which is the mechanism that detects and re-establishes a connection silently dropped by a restartType
🐛 Bug Fix
Changes
get_redis_async_clientnow builds the asyncRedisClusterwith reconnection-resilient defaults (health_check_interval=25,socket_keepalive=True) via a small_async_cluster_reconnect_kwargshelper, andhealth_check_interval/socket_keepaliveare added to the cluster kwarg allow-list so they can be tuned or disabled per config. New regression tests intests/test_litellm/test_redis.pyassert the resilient defaults are applied and that an explicit config value overrides them