Repository navigation
bug(cache): a demoted Redis primary reads as healthy; Sentinel mode is unconfigured #656
Description
Area: cache (shared Redis backend, #626) · must land before #630 makes the backend selectable
Two failover gaps in RedisCache (#626). Neither is reachable today, since nothing selects the backend yet, but both break the design's promise that a failing cache means a miss or a bypass, never stale rows, and both are decided in #626's code.
1. A demoted primary reads as healthy, so bumps are deferred forever while lookups keep hitting
record (internal/cache/redis.go) counts every error reply as the server being up. When the primary behind a stable address is demoted to a replica:
- the client's existing connection survives the demotion,
SETanswers-READONLY You can't write against a read only replica., andPINGstill answersPONG(measured againstredis:8.10.2-alpine, two containers andREPLICAOF); - rueidis v1.0.78 does not reconnect on
READONLY(read from its source).
So this process's token bumps fail, are deferred, and are retried every ≤10 s for as long as it runs; the breaker never opens (and the probe's PING would close it); lookups keep reading the replica, which follows the new primary. Every write this process ingests leaves every process serving pre-write entries until their TTL (up to an hour), while wavehouse_cache_breaker_open reads 0. The same accounting defers every bump under -OOM with maxmemory-policy noeviction, Redis's default.
This applies to standalone mode against anything whose primary can be demoted behind a stable address, e.g. a planned or manual failover of a managed Redis (inferred; ioredis documents reconnectOnError on READONLY for exactly that case).
Suggested fix: count a READONLY reply as a failure against the breaker, so the process stops serving; recover by re-dialing (a fresh client, so DNS resolves to the new primary) rather than PINGing the same connection, or probe with a write. Add an integration case that REPLICAOFs the test server to a second one mid-test, asserts the breaker opens and lookups stop hitting, then REPLICAOF NO ONE and asserts the pending bumps land. Name the noeviction case in the operator docs for maxmemory-policy.
2. Sentinel mode is accepted but not configured or tested
clientOption's sentinel branch passes the master set name only:
- rueidis builds sentinel connections from
SentinelOption.Username/Password, not the top-level fields (sentinel.goin v1.0.78), so sentinels that require auth (common in packaged charts) refuse the connection and the cache is bypassed for good, logging an error every 30 s; SentinelOption.TopologyRefreshIntervalis left at 0, which rueidis's own documentation warns against: one missed+switch-masterbinds the client to the old master until restart — landing in gap 1;- nothing tests it beyond a unit assertion that the master set is passed through.
Suggested fix: pass credentials to the sentinel options (reuse the top-level ones or add fields the config can expose), set a topology refresh interval (rueidis suggests 5 s), and add a Sentinel integration case — or drop Sentinel from the accepted modes until it is tested.
Found in pre-push review of #626.
Metadata
Metadata
Assignees
Labels
Type
Projects
- StatusShow more project fieldsBacklog
Activity
fix(cache): owed bumps hold lookups; refused writes open the breaker
fix(config): refuse cache.redis.mode=sentinel until #656
feat(cache): shared redis cache, snapshot keys, uncached write pipes (#…