Skip to content

redis: discard readonly pipeline connections - #1256

Draft
NVGreg wants to merge 1 commit into
envoyproxy:mainfrom
NVGreg:redis-readonly-pipeline-reconnect
Draft

NVGreg wants to merge 1 commit into
envoyproxy:mainfrom
NVGreg:redis-readonly-pipeline-reconnect

Conversation

@NVGreg

@NVGreg NVGreg commented Sep 28, 2026

Copy link
Copy Markdown

Outcome

Fixes REDIS_CLOSE_CONNECTION_ON_READONLY_ERROR=true so Redis READONLY replies inside pipelined commands also cause the pooled connection to be discarded and re-dialed.

Impact

Deployments that fail over by repointing an address at the new master can recover stale pooled connections even when the failing write is part of the normal INCRBY/EXPIRE pipeline. The failing operation still returns its READONLY error to the caller; only connection handling changes.

Scope

  • Adds a read-only-aware pipeline action used only when REDIS_CLOSE_CONNECTION_ON_READONLY_ERROR is enabled.
  • Leaves default flag-off behavior on native radix.NewPipeline().
  • Preserves ordinary per-command Redis errors as connection-usable.
  • Covers single/sentinel pipelined writes and cluster grouped pipelined writes.
  • Updates README/settings docs to state that pipelined commands are covered.

Remaining gate

This is Draft for maintainer review. It was intentionally based on older commit 8fe6ea421048bdb2a8873d5f676b3f6eac76a997 to keep the branch push-compatible with the current GitHub token permissions. It should be reviewed with #1255 as compatible but separate work; #1255 handles closing connections on context cancellation, while this PR handles READONLY replies inside pipelines.

Compatibility with #1255

Locally merged this branch with #1255 (tmp/pr-1255-close-on-cancel) without conflicts. Focused Redis tests and race tests passed on the composed branch.

Verification

go test ./src/redis -run 'Test(ReadOnlyAwarePipelineDelegatesDoToEmbeddedConn|ReadOnly|PoolDiscardsConnOnReadOnlyPipeline|ExecuteGroupedPipelineDiscardsConnOnReadOnly)' -count=1
go test ./src/redis -run '^$' -bench BenchmarkNewPipelineAppendTwoActions -benchmem -count=3
go test ./src/redis ./test/redis -count=1
go test -race ./src/redis -run 'Test(ReadOnlyAwarePipeline|PoolDiscardsConnOnReadOnlyPipeline|ExecuteGroupedPipelineDiscardsConnOnReadOnly|ExecuteGroupedPipeline)' -count=1
go test ./...
git diff --check 8fe6ea42..HEAD

Composed with #1255:

go test ./src/redis -run 'Test(ReadOnly|PoolDiscardsConnOnReadOnlyPipeline|ExecuteGroupedPipelineDiscardsConnOnReadOnly|Close|Cancel)' -count=1
go test -race ./src/redis -run 'Test(ReadOnlyAwarePipeline|PoolDiscardsConnOnReadOnlyPipeline|ExecuteGroupedPipelineDiscardsConnOnReadOnly|Close|Cancel)' -count=1
git diff --check

Signed-off-by: Gregory Giecold <ggiecold@nvidia.com>
(cherry picked from commit 276cddd204f662bc2e3d97881ac7315d35b3f7fc)
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