Fix Sentinel connection leak and AbortOnConnectFail=false handling - #3187
Open
HarnageaGabriel wants to merge 3 commits into
Open
Fix Sentinel connection leak and AbortOnConnectFail=false handling#3187HarnageaGabriel wants to merge 3 commits into
HarnageaGabriel wants to merge 3 commits into
Conversation
On OSS cluster, the direct (NoRedirect) probe messages used during connection setup and keep-alive could target a hash slot the connected node doesn't own, so the server replies MOVED and the probe is dropped instead of following it. - Skip the replica_read_only SET fallback in AutoConfigureAsync once cluster topology already reports our role, since it's both redundant and slot-unsafe there. - Skip the tie-breaker GET fallback in AutoConfigureAsync on cluster, where a tie-breaker key isn't meaningful. - When the ECHO/PING/TIME tracer is unavailable, build the EXISTS fallback key with a hash-tag targeting a slot this endpoint actually owns, reusing the existing hash-tag cache. Fixes StackExchange#2970.
HarnageaGabriel
force-pushed
the
fix-sentinel-abortonconnectfail-leak-2980
branch
from
August 18, 2026 20:18
6e6dd2b to
6f167d7
Compare
When connecting via Sentinel with AbortOnConnectFail=false, a failed initial connect (unreachable sentinels, or no primary discovered within ConnectTimeout) threw instead of returning the multiplexer, and leaked the internally-created ConnectionMultiplexer instances (sentinel monitor connection and per-retry primary candidates) since nothing disposed them. This now mirrors the non-Sentinel Connect path: AbortOnConnectFail=false returns a usable, disposed-free multiplexer with LastException set and background retry wired up, while AbortOnConnectFail=true still throws but disposes everything first. Fixes StackExchange#2980
HarnageaGabriel
force-pushed
the
fix-sentinel-abortonconnectfail-leak-2980
branch
from
August 19, 2026 19:33
6f167d7 to
19103ee
Compare
Collaborator
|
I think the sentinel side of this is reasonable and captures the key scenarios (not quite full test coverage, but probably enough), however the slot stuff shares the same feedback from #3185 (and for good reason: they share a commit) |
…nch's shared code This branch carries commit 1c2a472 ("Make fallback discovery and keep-alive probes cluster-slot aware") in its history, so mgravell's review of StackExchange#3185 pointing out bugs in ServerSelectionStrategy's hash-tag handling and ClusterNode.Parent applies here too. Porting the same fix: - Unify InventKey's O(16384) map-scan and the tracer key's slot lookup into one primitive, ServerEndPoint.GetServableSlot(), which also now falls back to a replica's primary's slots (a replica's own Slots is always empty). - Fix ClusterNode.Parent, which always returned null due to a backwards null-check on its own backing field. - Replace the cached-byte[]-prefix API (a 16384-entry static array publishing shared mutable state as a RedisKey) with ServerSelectionStrategy.CreateKeyForSlot(slot, suffix), composed per call. - Memoize the composed tracer key on ServerEndPoint since GetTracerKey runs on the heartbeat path. - GetClusterNode uses ClusterConfiguration's O(1) endpoint indexer. - Comment AutoConfigureAsync's cluster guards (first-handshake window, why the SET probe can't work on cluster) and drop a redundant clause. See StackExchange#3185 for the full review and the matching commits on that PR's branch.
3 tasks
Contributor
Author
|
Re: "hash-tag / slot feedback common with #3185" - confirmed, this branch carries #3185's commit ( Pushed the same structural fix as a separate commit ( |
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
2 participants
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.
Summary
Fixes #2980.
When connecting via Sentinel (
ConnectionMultiplexer.Connect/ConnectAsync/SentinelConnect) withAbortOnConnectFail=false, a failed initial connect — sentinels unreachable, or no primary discovered withinConnectTimeout— threw aRedisConnectionExceptioninstead of returning a multiplexer to the caller, and leaked the internally-createdConnectionMultiplexerinstances (the sentinel monitor connection, and per-retry primary candidates inGetSentinelMasterConnection's loop), since nothing disposed them on the failure path.This didn't match the non-Sentinel
Connect/ConnectImplbehavior, which honorsAbortOnConnectFail=falseby returning the muxer (withLastExceptionset) instead of throwing.Changes in
ConnectionMultiplexer.Sentinel.cs:AbortOnConnectFail=false: no longer throws. Returns a usable multiplexer withLastExceptionset, the same event wiring (ConnectionRestored/ConnectionFailed) and background retry (SwitchPrimary/reconnect timer) as the success path, so callers can attach handlers and let it reconnect once sentinels/primary become reachable — mirroring the non-Sentinel path.AbortOnConnectFail=true: preserves today's throwing behavior and exception messages (no breaking change), but now disposes the sentinel connection and any superseded/failed candidate connections before the exception propagates, instead of leaking them.AbortOnConnectFail.No public API changes.
Test plan
dotnet build(Release) — 0 errors, 0 warningsSentinelConfigTests.cscoveringAbortOnConnectFail=false(syncConnect, asyncConnectAsync, andSentinelConnect) returning a non-null, non-throwing, disconnected multiplexer withLastExceptionset against unreachable sentinel endpoints, plus a regression guard thatAbortOnConnectFail=truestill throws — all against closed loopback ports, no live topology requiredStackExchange.Redis.Testssuite (net10.0, Release) against the docker-compose Redis/Sentinel/Cluster topology: 5880 passed, 0 failed (the live Sentinel integration tests are gated[SkipOnWindows], unaffected by this change)🤖 Generated with Claude Code