fix(cache): QueryCacheManager.get() should fail open on cache backend errors#42252
Conversation
… errors Reading from the query cache in QueryCacheManager.get() was not wrapped in error handling. If the configured cache backend (e.g. Redis) raised a connection or timeout error on read, the exception propagated all the way up through the chart/dashboard data API and surfaced as an unhandled 500, instead of falling back to querying live data the same way a normal cache miss already does. Wrap the cache read in a try/except, log the failure, and treat it exactly like a cache miss so callers gracefully fall back to querying live data.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42252 +/- ##
==========================================
- Coverage 65.22% 65.22% -0.01%
==========================================
Files 2768 2768
Lines 156280 156285 +5
Branches 35774 35774
==========================================
- Hits 101933 101932 -1
- Misses 52382 52387 +5
- Partials 1965 1966 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
dosubot
Bot
added
the
infra:caching
Code Review Agent Run #438967Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
1 participant
SUMMARY
QueryCacheManager.get()reads from the configured cache backend with:This read is not wrapped in any error handling. If the cache backend (e.g. Redis) raises a connection, timeout, or other backend-specific error on read, the exception propagates all the way up through the chart/dashboard data API (
POST /api/v1/chart/data) and surfaces as an unhandled 500, instead of gracefully falling back to querying live data — which is exactly what already happens one branch up on a normal cache miss (if not key or not _cache[region] or force_query: return query_cache).In other words: any transient cache backend hiccup — not just a full outage — turns into hard failures for chart and dashboard rendering, even though the data can still be queried live.
This PR wraps the cache read in a try/except, logs the failure at a level appropriate for a potentially noisy/flaky backend, and falls through to the existing cache-miss path so the behavior matches a normal cache miss rather than raising.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend error-handling change, no UI impact.
TESTING INSTRUCTIONS
tests/unit_tests/common/test_query_cache_manager.py:test_get_cache_miss— baseline: a normal cache miss returns a non-loadedQueryCacheManager.test_get_cache_backend_error_fails_open— mocks the cache backend's.get()to raise aConnectionError, and assertsQueryCacheManager.get()returns the same result as a normal cache miss instead of propagating the exception.CACHE_CONFIG/DATA_CACHE_CONFIGat a Redis instance, then stop/block Redis while callingPOST /api/v1/chart/datafor a datasource with caching enabled. Before this change: request fails with a 500. After this change: request falls back to querying live data.ADDITIONAL INFORMATION