fix(caching): propagate redis errors from set/get so the circuit breaker can trip - #34300
Conversation
…ker can trip async_set_cache, async_set_cache_pipeline, async_set_cache_sadd, and async_get_cache in RedisCache all caught their own Redis exception, logged it, and returned without re-raising. Each is wrapped by _redis_circuit_breaker_guard, whose record_failure()/record_success() bookkeeping can only see an exception if the wrapped method actually raises one -- since these four swallowed it internally, the guard saw every call as a success and reset the failure counter every time. The breaker could never open for these methods no matter how many consecutive real failures occurred. Fix: re-raise after logging, matching the pattern already used correctly elsewhere in the same file (_set_cache_sadd_helper, async_increment, async_rpush, async_scan_iter, etc.).
|
|
Greptile SummaryThis PR fixes a bug in
Confidence Score: 4/5Safe to merge — the four one-line fixes are minimal and match the existing error-propagation pattern used by every other guarded method in the file; DualCache wraps all four callers with exception handlers that keep failures from surfacing to LLM completion paths. The code change restores the intended contract of the circuit-breaker decorator, and the impact on callers was traced by the author. The only gap is that three of the four patched methods have no circuit-breaker integration test, leaving the door open for silent regressions on those paths. tests/test_litellm/caching/test_redis_cache.py — consider adding circuit-breaker tests for
|
| Filename | Overview |
|---|---|
| litellm/caching/redis_cache.py | Adds raise e after logging in the except blocks of async_set_cache, async_set_cache_pipeline, async_set_cache_sadd, and async_get_cache — matching the pattern already used in other decorated methods — so that _redis_circuit_breaker_guard can record failures and open the breaker. |
| tests/test_litellm/caching/test_redis_cache.py | New test test_async_set_cache_failures_trip_circuit_breaker correctly verifies that repeated async_set_cache failures trip the circuit breaker and that subsequent calls fast-fail without contacting Redis; the three other patched methods (async_set_cache_pipeline, async_set_cache_sadd, async_get_cache) have no equivalent circuit-breaker integration test. |
Reviews (1): Last reviewed commit: "fix(caching): propagate redis errors fro..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @pytest.mark.asyncio | ||
| async def test_async_set_cache_failures_trip_circuit_breaker( | ||
| monkeypatch, redis_no_ping | ||
| ): | ||
| """async_set_cache must propagate Redis errors so its circuit breaker guard | ||
| can see the failure and open after enough consecutive failures -- if the | ||
| error is swallowed instead, the guard always records a success and the | ||
| breaker can never open.""" | ||
| monkeypatch.setenv("REDIS_HOST", "https://my-test-host") | ||
| redis_cache = RedisCache() | ||
|
|
||
| mock_redis_instance = AsyncMock() | ||
| mock_redis_instance.set = AsyncMock(side_effect=ConnectionError("Redis down")) | ||
|
|
||
| with patch.object( | ||
| redis_cache, "init_async_client", return_value=mock_redis_instance | ||
| ): | ||
| for _ in range(redis_cache._circuit_breaker.failure_threshold): | ||
| with pytest.raises(ConnectionError, match="Redis down"): | ||
| await redis_cache.async_set_cache("key", "value") | ||
|
|
||
| assert redis_cache._circuit_breaker.is_open() | ||
|
|
||
| # Once open, further calls must fast-fail without touching Redis. | ||
| mock_redis_instance.set.reset_mock() | ||
| with pytest.raises(Exception, match="circuit breaker is open"): | ||
| await redis_cache.async_set_cache("key", "value") | ||
| mock_redis_instance.set.assert_not_called() | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_async_lpop_pipeline_single_round_trip(monkeypatch, redis_no_ping): |
There was a problem hiding this comment.
The new test exercises async_set_cache, but the same raise e fix was applied to async_set_cache_pipeline, async_set_cache_sadd, and async_get_cache — and none of those have an analogous circuit-breaker integration test. If a future refactor inadvertently removes the raise from one of those methods, there is no test that would catch the regression. async_get_cache is especially worth covering because it sits on the hot path for cache lookups.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Valid, they follow the same pattern, added in latest commit.
…sion Extend the previous commit's single async_set_cache test with the same check for async_get_cache, async_set_cache_sadd, and async_set_cache_pipeline -- all four had the identical swallow bug.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…r-swallowed-exceptions
![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 |
|
Closing — superseded by #35273, which fixes the same root cause (swallowed exceptions defeating |
2 participants
TLDR
Problem this solves:
RedisCache.async_set_cache/async_set_cache_pipeline/async_set_cache_sadd/async_get_cacheswallow Redis exceptions_redis_circuit_breaker_guardnever sees a failure, so the breaker can never openHow it solves it:
Relevant issues
Fixes #34299
Linear ticket
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
make lint-format-check-changed,make lint-ruff, and the actual CI matrix group covering this code --test-unit-other: 2230 passed, 26 skipped, 2 failed. The 2 failures aretest_s3_cache_async_set_cache_pipeline/test_s3_cache_concurrent_async_operations, unrelated flaky/order-dependent S3 mock assertions -- confirmed present on the base commit before this PR's changes too)Screenshots / Proof of Fix
I don't have a live proxy + Redis + real LLM provider credentials to produce a full e2e proof costing real $. What I do have is 4 unit tests (one per patched method) that each reproduce the exact regression and pass only with the fix, captured at specific commits:
Before fix (base commit
0fcaadf11c+ just the 4 new tests,litellm/caching/redis_cache.pyreverted):After fix (commit
648add819c):If a maintainer wants a live e2e confirmation (real Redis, forced connection failure, watching for the "circuit breaker OPENED" log line), I'm happy to help set that up -- just don't have the credentials/environment for it myself.
Type
🐛 Bug Fix
✅ Test
Changes
litellm/caching/redis_cache.py: addraise eafter logging in the except blocks ofasync_set_cache,async_set_cache_pipeline,async_set_cache_sadd(its second except block), andasync_get_cache.tests/test_litellm/caching/test_redis_cache.py: add one test per patched method (test_async_set_cache_failures_trip_circuit_breaker,test_async_get_cache_failures_trip_circuit_breaker,test_async_set_cache_sadd_failures_trip_circuit_breaker,test_async_set_cache_pipeline_failures_trip_circuit_breaker), each asserting the breaker opens afterfailure_thresholdconsecutive failures and fast-fails (without touching Redis) once open.QA runbook
Not applicable -- no
tests/e2efiles added or changed.Final Attestation
DualCache,CooldownCache, the proxy's spend-tracking path viaGLOBAL_LOGGING_WORKER) to confirm none can now cause an actual LLM completion request to fail -- the only caller that doesn't already have a safety net (DualCache.async_set_cache_sadd->dynamic_rate_limiterhook) requires a config flag (callbacks: ["dynamic_rate_limiter"]) that's opt-in and off by default.