Conversation
Signed-off-by: Gregory Giecold <ggiecold@nvidia.com>
NVGreg
force-pushed
the
redis-close-on-cancel-20260928
branch
from
September 28, 2026 09:06
9210203 to
4edc131
Compare
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.
Outcome
Add opt-in
REDIS_CLOSE_ON_CANCELandREDIS_PERSECOND_CLOSE_ON_CANCELsettings (both defaultfalse). When a Redis command's context is canceled, the wrapper closes its pooled socket before Radix can remain blocked discarding a late reply. This covers application commands, pool maintenancePING, and Redis Cluster topology checks. A bounded startup attempt also covers single/cluster initialization and pooledAUTH/READONLYbootstrap.Why
With a Redis peer that accepts writes but never replies, Radix v4.1.4 can clear its read deadline and wait indefinitely while discarding the canceled response. Later commands sharing that connection can remain stuck.
REDIS_TIMEOUTis a TCP dial timeout and does not bound established-socket I/O.This change needs a deadline or cancellation on the command context. #1241 adds a service request deadline and admission controls for deployments whose callers do not supply one. This PR is independent of #1241; both need joint Stage qualification for the intended recovery path.
Verification
go test ./...passed on this branch.go test -race ./src/redis -run '^TestExperiment' -count=1 -timeout=40spassed.net.Pipeand loopback tests cover withheld replies, blocked writes,AUTH/READONLYbootstrap, TLS and write buffering, pre-canceled contexts, healthy reuse, and collateral failure of a concurrent shared command.PING, and Cluster Sync black-hole recovery tests with it. Those service tests are not in this standalone PR.Rollout limits
This is a Draft for review and Stage testing. Closing a shared socket can fail other commands in flight even when their own deadlines have not expired. Frequent cancellations may raise reconnect, CPU, and GC load; commands without a deadline remain unbounded. Sentinel control-connection bootstrap is outside this wrapper. Before enabling the flags in production, measure request errors, connection churn, throughput, and latency at representative Stage load, including with #1241 enabled. No deployment or runtime qualification is claimed here.