Skip to content

redis: optionally close pooled connections on READONLY error replies#1191

Open
OS-kiranmalsetty wants to merge 1 commit into
envoyproxy:mainfrom
OS-kiranmalsetty:redis-close-connection-on-readonly
Open

redis: optionally close pooled connections on READONLY error replies#1191
OS-kiranmalsetty wants to merge 1 commit into
envoyproxy:mainfrom
OS-kiranmalsetty:redis-close-connection-on-readonly

Conversation

@OS-kiranmalsetty

@OS-kiranmalsetty OS-kiranmalsetty commented Jul 15, 2026

Copy link
Copy Markdown

Problem

After a master→replica failover in deployments that fail over by repointing an address at the new master (a Kubernetes Service, DNS, or a proxy — e.g. Redis without Sentinel, Dragonfly, KeyDB), the demoted master keeps already-established connections open. radix only discards pooled connections on IO errors, so those stale connections are reused forever and every write on them keeps failing with:

READONLY You can't write against a read only replica.

until the process restarts — turning a routine failover into a permanent rate limiting outage. We hit this in production running ratelimit against Dragonfly managed by dragonfly-operator (Dragonfly declined to drop client connections on demotion: dragonflydb/dragonfly#5160), but the failure mode applies to any proxy/DNS-based Redis failover.

Change

Adds an opt-in setting REDIS_CLOSE_CONNECTION_ON_READONLY_ERROR (default false). When enabled, pooled connections are wrapped so that a READONLY error reply strips radix's resp.ErrConnUsable wrapper from the returned error. That is radix v4's native signal that a connection is no longer usable: the pool discards it and re-dials through the configured address, which reaches the current master.

  • The failing command still returns its error to the caller — only the connection handling changes.
  • Ordinary application errors (ERR ..., WRONGTYPE ...) keep the ErrConnUsable wrapper and do not affect the connection.
  • The wrapper is installed via Dialer.CustomConn after all other dialer fields are final (TLS, auth, write buffering), since CustomConn replaces radix's own dialing.
  • Applies to the main and per-second clients across single, cluster, and sentinel modes.

Testing

  • Unit tests for the READONLY detection and the error-unwrap behavior using radix.NewStubConn.
  • A pool-level test proving the end-to-end mechanism: a READONLY reply on a pooled connection causes the pool to discard it, re-dial, and succeed on the next command.
  • A driver-level test against miniredis showing normal operation is unaffected with the flag enabled.
  • go build ./..., gofmt, and go vet clean; existing test/redis driver tests pass with the new parameter.

Notes for reviewers

  • Default is false to keep existing behavior; happy to flip the default if maintainers prefer, since continuing to use a connection that answers READONLY is never useful for ratelimit's write-heavy workload.
  • Naming of the env var is open to bikeshedding.

Local reproduction (before/after)

Reproduced the failure mode locally with two Redis 7 containers (A = master, B = standby) and a small TCP proxy standing in for the Kubernetes Service. Like real conntrack, the proxy keeps established connections piped to the old backend and only routes new connections to the new one. A driver-level loop ran INCRBY through redis.NewClientImpl; mid-run, A was demoted (REPLICAOF B) and the proxy repointed at B — the same sequence a Service-based failover performs.

Without this change — permanent outage; the pool reuses its healthy-looking TCP connections to the demoted master forever:

14:14:06.665 INCRBY -> 4      OK
14:14:07.276 >>> FAILOVER: redis A demoted to replica, service now points at B
14:14:07.278 INCRBY -> 0      ERR response returned from Conn: READONLY You can't write against a read only replica.
14:14:07.782 INCRBY -> 0      ERR response returned from Conn: READONLY You can't write against a read only replica.
... (every subsequent command fails identically)
=== after failover: 0 ok, 16 errors -> NEVER RECOVERED ===

With REDIS_CLOSE_CONNECTION_ON_READONLY_ERROR=true — exactly one error on the stale connection, then the pool discards it, re-dials through the service address, and reaches the new master:

14:14:39.277 INCRBY -> 4      OK
14:14:39.893 >>> FAILOVER: redis A demoted to replica, service now points at B
14:14:39.894 INCRBY -> 0      ERR READONLY You can't write against a read only replica.
14:14:40.397 INCRBY -> 1      OK
14:14:40.899 INCRBY -> 2      OK
... (all subsequent commands succeed against the new master)
=== after failover: 15 ok, 1 errors -> RECOVERED ===

After a master->replica failover in deployments that fail over by
repointing an address at the new master (a Kubernetes Service, DNS, or a
proxy - e.g. Redis without Sentinel, Dragonfly, KeyDB), the demoted
master keeps already-established connections open. radix only discards
pooled connections on IO errors, so those stale connections are reused
forever and every write on them keeps failing with READONLY until the
process restarts, turning a routine failover into a permanent rate
limiting outage.

Add an opt-in setting, REDIS_CLOSE_CONNECTION_ON_READONLY_ERROR
(default false), that wraps pooled connections so a READONLY error reply
strips radix's resp.ErrConnUsable wrapper. The pool then discards the
connection and re-dials through the configured address, reaching the
current master. The failing command still returns its error to the
caller; only the connection handling changes. Applies to the main and
per-second clients across single, cluster, and sentinel modes.

Signed-off-by: kiran malsetty <kiran.malsetty@outsystems.com>
@OS-kiranmalsetty
OS-kiranmalsetty marked this pull request as ready for review July 15, 2026 16:53
@OS-kiranmalsetty

OS-kiranmalsetty commented Jul 15, 2026

Copy link
Copy Markdown
Author

@envoyproxy/ratelimit-maintainers(@agrawroh , @collin-lee ) this is ready for review — could you also approve the CI workflow runs (first-time contributor)? DCO is passing.

Summary: opt-in REDIS_CLOSE_CONNECTION_ON_READONLY_ERROR closes pooled connections that answer READONLY (master demoted to replica behind a Service/DNS/proxy address) so the pool re-dials and reaches the current master. Includes unit/pool/miniredis tests and a before/after reproduction in the description. Happy to rename the setting or adjust the default if you'd prefer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant