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,
SET answers -READONLY You can't write against a read only replica., and PING still answers PONG (measured against redis:8.10.2-alpine, two containers and REPLICAOF);
- 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.go in 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.TopologyRefreshInterval is left at 0, which rueidis's own documentation warns against: one missed +switch-master binds 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.
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:SETanswers-READONLY You can't write against a read only replica., andPINGstill answersPONG(measured againstredis:8.10.2-alpine, two containers andREPLICAOF);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
PINGwould 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), whilewavehouse_cache_breaker_openreads 0. The same accounting defers every bump under-OOMwithmaxmemory-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
reconnectOnErroronREADONLYfor exactly that case).Suggested fix: count a
READONLYreply 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 thanPINGing the same connection, or probe with a write. Add an integration case thatREPLICAOFs the test server to a second one mid-test, asserts the breaker opens and lookups stop hitting, thenREPLICAOF NO ONEand asserts the pending bumps land. Name thenoevictioncase in the operator docs formaxmemory-policy.2. Sentinel mode is accepted but not configured or tested
clientOption's sentinel branch passes the master set name only: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;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.