redis: optionally close pooled connections on READONLY error replies#1191
Open
OS-kiranmalsetty wants to merge 1 commit into
Open
redis: optionally close pooled connections on READONLY error replies#1191OS-kiranmalsetty wants to merge 1 commit into
OS-kiranmalsetty wants to merge 1 commit into
Conversation
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
marked this pull request as ready for review
July 15, 2026 16:53
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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
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(defaultfalse). When enabled, pooled connections are wrapped so that a READONLY error reply strips radix'sresp.ErrConnUsablewrapper 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.ERR ...,WRONGTYPE ...) keep theErrConnUsablewrapper and do not affect the connection.Dialer.CustomConnafter all other dialer fields are final (TLS, auth, write buffering), sinceCustomConnreplaces radix's own dialing.Testing
radix.NewStubConn.go build ./...,gofmt, andgo vetclean; existingtest/redisdriver tests pass with the new parameter.Notes for reviewers
falseto 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.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
INCRBYthroughredis.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:
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: