From 1c2a4725c860476b37027cb2b69b69aefb810b17 Mon Sep 17 00:00:00 2001 From: Gabriel Harnagea Date: Tue, 18 Aug 2026 22:06:09 +0200 Subject: [PATCH 1/3] Make fallback discovery and keep-alive probes cluster-slot aware 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 #2970. --- src/StackExchange.Redis/ServerEndPoint.cs | 27 ++++++-- .../ServerSelectionStrategy.HashTags.cs | 19 +++++- .../ServerSelectionStrategy.cs | 2 + .../HashTagUnitTests.cs | 16 ++++- .../ServerEndPointClusterProbeUnitTests.cs | 66 +++++++++++++++++++ 5 files changed, 124 insertions(+), 6 deletions(-) create mode 100644 tests/StackExchange.Redis.Tests/ServerEndPointClusterProbeUnitTests.cs diff --git a/src/StackExchange.Redis/ServerEndPoint.cs b/src/StackExchange.Redis/ServerEndPoint.cs index 20968261b..278d98193 100644 --- a/src/StackExchange.Redis/ServerEndPoint.cs +++ b/src/StackExchange.Redis/ServerEndPoint.cs @@ -355,7 +355,7 @@ public void SetClusterConfiguration(ClusterConfiguration configuration) public void UpdateNodeRelations(ClusterConfiguration configuration) { - var thisNode = configuration.Nodes.FirstOrDefault(x => x.EndPoint?.Equals(EndPoint) == true); + var thisNode = GetClusterNode(configuration); if (thisNode != null) { Multiplexer.Trace($"Updating node relations for {Format.ToString(thisNode.EndPoint)}..."); @@ -379,6 +379,9 @@ public void UpdateNodeRelations(ClusterConfiguration configuration) } } + private ClusterNode? GetClusterNode(ClusterConfiguration? configuration) => + configuration?.Nodes.FirstOrDefault(x => x.EndPoint?.Equals(EndPoint) == true); + public void SetUnselectable(UnselectableFlags flags) { if (flags != 0) @@ -502,7 +505,9 @@ internal async Task AutoConfigureAsync(PhysicalConnection? connection, ILogger? await WriteDirectOrQueueFireAndForgetAsync(connection, msg, autoConfigProcessor).ForAwait(); } } - else if (commandMap.IsAvailable(RedisCommand.SET) && !(helloPending || RoleKnownFromHello)) + else if (commandMap.IsAvailable(RedisCommand.SET) + && !(helloPending || RoleKnownFromHello) + && !(ServerType == ServerType.Cluster && GetClusterNode(ClusterConfiguration) is not null)) { // This is a nasty way to find if we are a replica, and it will only work on up-level servers, but... // (note we only get here when HELLO isn't going to tell us: the HELLO reply carries "role", and @@ -535,7 +540,9 @@ internal async Task AutoConfigureAsync(PhysicalConnection? connection, ILogger? } // If we are going to fetch a tie breaker, do so last and we'll get it in before the tracer fires completing the connection // But if GETs are disabled on this, do not fail the connection - we just don't get tiebreaker benefits - if (Multiplexer.RawConfig.TryGetTieBreaker(out var tieBreakerKey) && Multiplexer.CommandMap.IsAvailable(RedisCommand.GET)) + if (ServerType != ServerType.Cluster + && Multiplexer.RawConfig.TryGetTieBreaker(out var tieBreakerKey) + && Multiplexer.CommandMap.IsAvailable(RedisCommand.GET)) { log?.LogInformationRequestingTieBreak(new(EndPoint), tieBreakerKey); msg = Message.Create(0, flags, RedisCommand.GET, tieBreakerKey); @@ -680,12 +687,24 @@ internal Message GetTracerMessage(bool checkResponse) else { map.AssertAvailable(RedisCommand.EXISTS); - msg = Message.Create(0, flags, RedisCommand.EXISTS, (RedisValue)Multiplexer.UniqueId); + msg = Message.Create(0, flags, RedisCommand.EXISTS, GetTracerKey()); } msg.SetInternalCall(); return msg; } + internal RedisKey GetTracerKey() + { + RedisKey key = Multiplexer.UniqueId; + if (ServerType == ServerType.Cluster + && GetClusterNode(ClusterConfiguration) is { } node + && node.Slots.Count > 0) + { + key = key.Prepend(ServerSelectionStrategy.GetHashTagPrefix(node.Slots[0].From)); + } + return key; + } + internal UnselectableFlags GetUnselectableFlags() => unselectableReasons; internal bool IsSelectable(RedisCommand command, bool allowDisconnected = false) diff --git a/src/StackExchange.Redis/ServerSelectionStrategy.HashTags.cs b/src/StackExchange.Redis/ServerSelectionStrategy.HashTags.cs index 4cbd4538c..a1751f50a 100644 --- a/src/StackExchange.Redis/ServerSelectionStrategy.HashTags.cs +++ b/src/StackExchange.Redis/ServerSelectionStrategy.HashTags.cs @@ -1,6 +1,7 @@ -using System; +using System; using System.Diagnostics; using System.Text; +using System.Threading; namespace StackExchange.Redis; @@ -10,9 +11,25 @@ internal sealed partial class ServerSelectionStrategy private static class HashTags { private static readonly string[] Cache = Populate(); + private static readonly byte[]?[] PrefixCache = new byte[TotalSlots][]; + private static readonly object PrefixCacheLock = new(); + public static ReadOnlySpan Tags => Cache; public static string Get(int slot) => Cache[slot]; + public static byte[] GetPrefix(int slot) + { + var prefix = Volatile.Read(ref PrefixCache[slot]); + if (prefix is null) + { + lock (PrefixCacheLock) + { + prefix = PrefixCache[slot] ??= Encoding.ASCII.GetBytes("{" + Get(slot) + "}"); + } + } + return prefix; + } + private static string[] Populate() { // Via testing, we know that 3 characters is sufficient to populate all slots diff --git a/src/StackExchange.Redis/ServerSelectionStrategy.cs b/src/StackExchange.Redis/ServerSelectionStrategy.cs index 80c9b9efe..8c0623e67 100644 --- a/src/StackExchange.Redis/ServerSelectionStrategy.cs +++ b/src/StackExchange.Redis/ServerSelectionStrategy.cs @@ -442,5 +442,7 @@ internal string GetHashTag(ServerEndPoint endpoint) /// Gets a string that can be used as a hash-tag to reference a specific slot. /// internal static string GetHashTag(int slot) => slot < 0 ? "" : HashTags.Get(slot); + + internal static RedisKey GetHashTagPrefix(int slot) => HashTags.GetPrefix(slot); } } diff --git a/tests/StackExchange.Redis.Tests/HashTagUnitTests.cs b/tests/StackExchange.Redis.Tests/HashTagUnitTests.cs index 91b63d980..6709da826 100644 --- a/tests/StackExchange.Redis.Tests/HashTagUnitTests.cs +++ b/tests/StackExchange.Redis.Tests/HashTagUnitTests.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections.Generic; using System.Text; using Xunit; @@ -26,4 +26,18 @@ public void TestHashTagCoverage() } Assert.Equal(ServerSelectionStrategy.TotalSlots, uniques.Count); } + + [Theory] + [InlineData(0)] + [InlineData(1)] + [InlineData(8191)] + [InlineData(16383)] + public void TestHashTagPrefixTargetsSlot(int slot) + { + var prefix = ServerSelectionStrategy.GetHashTagPrefix(slot); + RedisKey key = ((RedisKey)"probe-id").Prepend(prefix); + + Assert.Equal(slot, ServerSelectionStrategy.GetHashSlot(key)); + Assert.Same((byte[]?)prefix, (byte[]?)ServerSelectionStrategy.GetHashTagPrefix(slot)); + } } diff --git a/tests/StackExchange.Redis.Tests/ServerEndPointClusterProbeUnitTests.cs b/tests/StackExchange.Redis.Tests/ServerEndPointClusterProbeUnitTests.cs new file mode 100644 index 000000000..3ff088ceb --- /dev/null +++ b/tests/StackExchange.Redis.Tests/ServerEndPointClusterProbeUnitTests.cs @@ -0,0 +1,66 @@ +using System.Linq; +using System.Net; +using System.Threading.Tasks; +using Xunit; + +namespace StackExchange.Redis.Tests; + +public class ServerEndPointClusterProbeUnitTests +{ + [Fact] + public async Task ExistsTracerUsesOwnedClusterSlot() + { + using var server = new InProcessTestServer { ServerType = ServerType.Cluster }; + var config = server.GetClientConfig(defaultOnly: true); + var commands = server.GetCommands(); + commands.Remove(nameof(RedisCommand.ECHO)); + commands.Remove(nameof(RedisCommand.PING)); + commands.Remove(nameof(RedisCommand.TIME)); + config.CommandMap = CommandMap.Create(commands); + + await using var connection = await ConnectionMultiplexer.ConnectAsync(config); + var endpoint = connection.GetServerEndPoint(server.DefaultEndPoint); + var node = endpoint.ClusterConfiguration?.Nodes.Single(x => x.EndPoint?.Equals(endpoint.EndPoint) == true); + Assert.NotNull(node); + var targetSlot = node.Slots[0].From; + + var message = endpoint.GetTracerMessage(checkResponse: true); + + Assert.Equal(RedisCommand.EXISTS, message.Command); + Assert.Equal(targetSlot, message.GetHashSlot(connection.ServerSelectionStrategy)); + var key = endpoint.GetTracerKey(); + var keyBytes = (byte[]?)key; + var prefixBytes = (byte[]?)ServerSelectionStrategy.GetHashTagPrefix(targetSlot); + Assert.NotNull(keyBytes); + Assert.NotNull(prefixBytes); + Assert.True(keyBytes.Take(prefixBytes.Length).SequenceEqual(prefixBytes)); + Assert.True(keyBytes.Skip(keyBytes.Length - connection.UniqueId.Length).SequenceEqual(connection.UniqueId)); + } + + [Theory] + [InlineData(ServerType.Standalone)] + [InlineData(ServerType.Cluster)] + public async Task ExistsTracerUsesPlainKeyWithoutKnownOwnedSlots(ServerType serverType) + { + using var server = new InProcessTestServer(); + var config = server.GetClientConfig(defaultOnly: true); + var commands = server.GetCommands(); + commands.Remove(nameof(RedisCommand.ECHO)); + commands.Remove(nameof(RedisCommand.PING)); + commands.Remove(nameof(RedisCommand.TIME)); + config.CommandMap = CommandMap.Create(commands); + + await using var connection = await ConnectionMultiplexer.ConnectAsync(config); + var endpoint = new ServerEndPoint(connection, new IPEndPoint(IPAddress.Loopback, 12345)) + { + ServerType = serverType, + }; + + var message = endpoint.GetTracerMessage(checkResponse: true); + var clusterStrategy = new ServerSelectionStrategy(null) { ServerType = ServerType.Cluster }; + + Assert.Equal(RedisCommand.EXISTS, message.Command); + Assert.Equal(ServerSelectionStrategy.GetHashSlot((RedisKey)connection.UniqueId), message.GetHashSlot(clusterStrategy)); + Assert.Equal(connection.UniqueId, (byte[]?)endpoint.GetTracerKey()); + } +} From 19103eef09e1386d9d90b6a1bd0bb28e6f01961b Mon Sep 17 00:00:00 2001 From: Gabriel Harnagea Date: Tue, 18 Aug 2026 22:13:48 +0200 Subject: [PATCH 2/3] Fix Sentinel connection leak and AbortOnConnectFail=false handling 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 #2980 --- .../ConnectionMultiplexer.Sentinel.cs | 252 +++++++++++------- .../SentinelConfigTests.cs | 61 +++++ 2 files changed, 223 insertions(+), 90 deletions(-) diff --git a/src/StackExchange.Redis/ConnectionMultiplexer.Sentinel.cs b/src/StackExchange.Redis/ConnectionMultiplexer.Sentinel.cs index 9e82d447f..64917a872 100644 --- a/src/StackExchange.Redis/ConnectionMultiplexer.Sentinel.cs +++ b/src/StackExchange.Redis/ConnectionMultiplexer.Sentinel.cs @@ -150,12 +150,19 @@ private static ConnectionMultiplexer SentinelPrimaryConnect(ConfigurationOptions sentinelConfig.Password = configuration.SentinelPassword; var sentinelConnection = SentinelConnect(sentinelConfig, log); + try + { + var muxer = sentinelConnection.GetSentinelMasterConnection(configuration, log); + // Set reference to sentinel connection so that we can dispose it + muxer.sentinelConnection = sentinelConnection; - var muxer = sentinelConnection.GetSentinelMasterConnection(configuration, log); - // Set reference to sentinel connection so that we can dispose it - muxer.sentinelConnection = sentinelConnection; - - return muxer; + return muxer; + } + catch + { + try { sentinelConnection.Dispose(); } catch { } + throw; + } } /// @@ -172,12 +179,19 @@ private static async Task SentinelPrimaryConnectAsync(Con sentinelConfig.Password = configuration.SentinelPassword; var sentinelConnection = await SentinelConnectAsync(sentinelConfig, writer).ForAwait(); + try + { + var muxer = sentinelConnection.GetSentinelMasterConnection(configuration, writer); + // Set reference to sentinel connection so that we can dispose it + muxer.sentinelConnection = sentinelConnection; - var muxer = sentinelConnection.GetSentinelMasterConnection(configuration, writer); - // Set reference to sentinel connection so that we can dispose it - muxer.sentinelConnection = sentinelConnection; - - return muxer; + return muxer; + } + catch + { + try { sentinelConnection.Dispose(); } catch { } + throw; + } } /// @@ -187,7 +201,8 @@ private static async Task SentinelPrimaryConnectAsync(Con /// The writer to log to, if any. public ConnectionMultiplexer GetSentinelMasterConnection(ConfigurationOptions config, TextWriter? log = null) { - if (ServerSelectionStrategy.ServerType != ServerType.Sentinel) + // A soft-failed SentinelConnect cannot detect a server type, but _isSentinel still records its intent. + if (ServerSelectionStrategy.ServerType != ServerType.Sentinel && (config.AbortOnConnectFail || !_isSentinel)) { throw new RedisConnectionException( ConnectionFailureType.UnableToConnect, @@ -210,110 +225,162 @@ public ConnectionMultiplexer GetSentinelMasterConnection(ConfigurationOptions co bool success = false; ConnectionMultiplexer? connection = null; EndPointCollection? endpoints = null; + RedisConnectionException? failure = null; - var sw = ValueStopwatch.StartNew(); - do + try { - // Sentinel has some fun race behavior internally - give things a few shots for a quicker overall connect. - const int queryAttempts = 2; - - EndPoint? newPrimaryEndPoint = null; - for (int i = 0; i < queryAttempts && newPrimaryEndPoint is null; i++) + var sw = ValueStopwatch.StartNew(); + do { - newPrimaryEndPoint = GetConfiguredPrimaryForService(serviceName); - } + // Sentinel has some fun race behavior internally - give things a few shots for a quicker overall connect. + const int queryAttempts = 2; - if (newPrimaryEndPoint is null) - { - throw new RedisConnectionException( - ConnectionFailureType.UnableToConnect, - CommandFlags.None, - $"Sentinel: Failed connecting to configured primary for service: {config.ServiceName}"); - } + EndPoint? newPrimaryEndPoint = null; + for (int i = 0; i < queryAttempts && newPrimaryEndPoint is null; i++) + { + newPrimaryEndPoint = GetConfiguredPrimaryForService(serviceName); + } - EndPoint[]? replicaEndPoints = null; - for (int i = 0; i < queryAttempts && replicaEndPoints is null; i++) - { - replicaEndPoints = GetReplicasForService(serviceName); - } + if (newPrimaryEndPoint is null) + { + failure = new RedisConnectionException( + ConnectionFailureType.UnableToConnect, + CommandFlags.None, + $"Sentinel: Failed connecting to configured primary for service: {config.ServiceName}"); + if (config.AbortOnConnectFail) + { + throw failure; + } - endpoints = config.EndPoints.Clone(); + if (connection is not null) try { connection.Dispose(); } catch { } + endpoints = config.EndPoints.Clone(); + connection = ConnectImpl(config, log, endpoints: endpoints); + break; + } - // Replace the primary endpoint, if we found another one - // If not, assume the last state is the best we have and minimize the race - if (endpoints.Count == 1) - { - endpoints[0] = newPrimaryEndPoint; - } - else - { - endpoints.Clear(); - endpoints.TryAdd(newPrimaryEndPoint); - } + EndPoint[]? replicaEndPoints = null; + for (int i = 0; i < queryAttempts && replicaEndPoints is null; i++) + { + replicaEndPoints = GetReplicasForService(serviceName); + } - if (replicaEndPoints is not null) - { - foreach (var replicaEndPoint in replicaEndPoints) + endpoints = config.EndPoints.Clone(); + + // Replace the primary endpoint, if we found another one + // If not, assume the last state is the best we have and minimize the race + if (endpoints.Count == 1) { - endpoints.TryAdd(replicaEndPoint); + endpoints[0] = newPrimaryEndPoint; + } + else + { + endpoints.Clear(); + endpoints.TryAdd(newPrimaryEndPoint); } - } - connection = ConnectImpl(config, log, endpoints: endpoints); + if (replicaEndPoints is not null) + { + foreach (var replicaEndPoint in replicaEndPoints) + { + endpoints.TryAdd(replicaEndPoint); + } + } - // verify role is primary according to: - // https://redis.io/topics/sentinel-clients - bool isPrimary; - var server = connection.GetServer(newPrimaryEndPoint); - // ReSharper disable once ConditionIsAlwaysTrueOrFalseAccordingToNullableAPIContract - if (server is { }) - { - try + if (connection is not null) try { connection.Dispose(); } catch { } + connection = ConnectImpl(config, log, endpoints: endpoints); + + // verify role is primary according to: + // https://redis.io/topics/sentinel-clients + bool isPrimary; + var server = connection.GetServer(newPrimaryEndPoint); + // ReSharper disable once ConditionIsAlwaysTrueOrFalseAccordingToNullableAPIContract + if (server is { }) { - isPrimary = connection.CommandMap.IsAvailable(RedisCommand.ROLE) - ? server.Role()?.Value == Role.LabelForMaster - : !server.IsReplica; + try + { + isPrimary = connection.CommandMap.IsAvailable(RedisCommand.ROLE) + ? server.Role()?.Value == Role.LabelForMaster + : !server.IsReplica; + } + catch + { + // fallback if ROLE unavailable but not declared; see #3064 + isPrimary = !server.IsReplica; + } + + if (isPrimary) + { + success = true; + break; + } } - catch + + Thread.Sleep(100); + } + while (sw.ElapsedMilliseconds < config.ConnectTimeout); + + if (!success) + { + failure ??= new RedisConnectionException( + ConnectionFailureType.UnableToConnect, + CommandFlags.None, + $"Sentinel: Failed connecting to configured primary for service: {config.ServiceName}"); + if (config.AbortOnConnectFail) { - // fallback if ROLE unavailable but not declared; see #3064 - isPrimary = !server.IsReplica; + throw failure; } - if (isPrimary) + if (connection is null) { - success = true; - break; + endpoints = config.EndPoints.Clone(); + connection = ConnectImpl(config, log, endpoints: endpoints); } + + connection.LastException = failure; + connection.Logger?.LogErrorSyncConnectTimeout(failure, failure.Message); } - Thread.Sleep(100); - } - while (sw.ElapsedMilliseconds < config.ConnectTimeout); + // Attach to reconnect event to ensure proper connection to the new primary + connection.ConnectionRestored += OnManagedConnectionRestored; - if (!success) - { - throw new RedisConnectionException( - ConnectionFailureType.UnableToConnect, - CommandFlags.None, - $"Sentinel: Failed connecting to configured primary for service: {config.ServiceName}"); - } + // If we lost the connection, run a switch to a least try and get updated info about the primary + connection.ConnectionFailed += OnManagedConnectionFailed; - // Attach to reconnect event to ensure proper connection to the new primary - connection.ConnectionRestored += OnManagedConnectionRestored; + lock (sentinelConnectionChildren) + { + sentinelConnectionChildren[serviceName] = connection; + } - // If we lost the connection, run a switch to a least try and get updated info about the primary - connection.ConnectionFailed += OnManagedConnectionFailed; + // Perform the initial switchover + var switchBlame = endpoints is { Count: > 0 } ? endpoints[0] : null; + try + { + SwitchPrimary(switchBlame, connection, log); + } + catch when (!success && !config.AbortOnConnectFail) + { + ScheduleSentinelPrimaryReconnect(connection, switchBlame); + } - lock (sentinelConnectionChildren) + return connection; + } + catch { - sentinelConnectionChildren[serviceName] = connection; + if (connection is not null) + { + connection.ConnectionRestored -= OnManagedConnectionRestored; + connection.ConnectionFailed -= OnManagedConnectionFailed; + lock (sentinelConnectionChildren) + { + if (sentinelConnectionChildren.TryGetValue(serviceName, out var child) && ReferenceEquals(child, connection)) + { + sentinelConnectionChildren.Remove(serviceName); + } + } + try { connection.Dispose(); } catch { } + } + throw; } - - // Perform the initial switchover - SwitchPrimary(endpoints[0], connection, log); - - return connection; } [System.Diagnostics.CodeAnalysis.SuppressMessage("Roslynator", "RCS1075:Avoid empty catch clause that catches System.Exception.", Justification = "We don't care.")] @@ -360,7 +427,6 @@ internal void OnManagedConnectionRestored(object? sender, ConnectionFailedEventA } } - [System.Diagnostics.CodeAnalysis.SuppressMessage("Roslynator", "RCS1075:Avoid empty catch clause that catches System.Exception.", Justification = "We don't care.")] internal void OnManagedConnectionFailed(object? sender, ConnectionFailedEventArgs e) { if (sender is not ConnectionMultiplexer connection) @@ -368,6 +434,12 @@ internal void OnManagedConnectionFailed(object? sender, ConnectionFailedEventArg return; // This should never happen - called from non-nullable ConnectionFailedEventArgs } + ScheduleSentinelPrimaryReconnect(connection, e.EndPoint); + } + + [System.Diagnostics.CodeAnalysis.SuppressMessage("Roslynator", "RCS1075:Avoid empty catch clause that catches System.Exception.", Justification = "We don't care.")] + private void ScheduleSentinelPrimaryReconnect(ConnectionMultiplexer connection, EndPoint? endPoint) + { // Periodically check to see if we can reconnect to the proper primary. // This is here in case we lost our subscription to a good sentinel instance // or if we miss the published primary change. @@ -379,7 +451,7 @@ internal void OnManagedConnectionFailed(object? sender, ConnectionFailedEventArg try { // Attempt, but do not fail here - SwitchPrimary(e.EndPoint, connection); + SwitchPrimary(endPoint, connection); } catch (Exception) { diff --git a/tests/StackExchange.Redis.Tests/SentinelConfigTests.cs b/tests/StackExchange.Redis.Tests/SentinelConfigTests.cs index 2e659eea6..814f99719 100644 --- a/tests/StackExchange.Redis.Tests/SentinelConfigTests.cs +++ b/tests/StackExchange.Redis.Tests/SentinelConfigTests.cs @@ -1,4 +1,6 @@ using System; +using System.Net; +using System.Threading.Tasks; using Xunit; namespace StackExchange.Redis.Tests; @@ -44,4 +46,63 @@ public void Clone_Preserves_SentinelCredentials() Assert.Equal(options.SentinelUser, clone.SentinelUser); Assert.Equal(options.SentinelPassword, clone.SentinelPassword); } + + [Fact] + public void Connect_UnreachableSentinel_AbortDisabled_ReturnsDisconnectedMultiplexer() + { + var options = GetUnreachableSentinelOptions(abortOnConnectFail: false); + + using var connection = ConnectionMultiplexer.Connect(options); + + Assert.False(connection.IsConnected); + var exception = Assert.IsType(connection.LastException); + Assert.Equal(ConnectionFailureType.UnableToConnect, exception.FailureType); + Assert.Equal("Sentinel: Failed connecting to configured primary for service: unreachable-primary", exception.Message); + Assert.NotNull(connection.sentinelConnection); + } + + [Fact] + public async Task ConnectAsync_UnreachableSentinel_AbortDisabled_ReturnsDisconnectedMultiplexer() + { + var options = GetUnreachableSentinelOptions(abortOnConnectFail: false); + + await using var connection = await ConnectionMultiplexer.ConnectAsync(options); + + Assert.False(connection.IsConnected); + var exception = Assert.IsType(connection.LastException); + Assert.Equal(ConnectionFailureType.UnableToConnect, exception.FailureType); + Assert.Equal("Sentinel: Failed connecting to configured primary for service: unreachable-primary", exception.Message); + Assert.NotNull(connection.sentinelConnection); + } + + [Fact] + public void SentinelConnect_UnreachableSentinel_AbortDisabled_ReturnsDisconnectedMultiplexer() + { + var options = GetUnreachableSentinelOptions(abortOnConnectFail: false); + + using var connection = ConnectionMultiplexer.SentinelConnect(options); + + Assert.False(connection.IsConnected); + } + + [Fact] + public void Connect_UnreachableSentinel_AbortEnabled_Throws() + { + var options = GetUnreachableSentinelOptions(abortOnConnectFail: true); + + Assert.Throws(() => ConnectionMultiplexer.Connect(options)); + } + + private static ConfigurationOptions GetUnreachableSentinelOptions(bool abortOnConnectFail) + { + var options = new ConfigurationOptions + { + AbortOnConnectFail = abortOnConnectFail, + ConnectRetry = 0, + ConnectTimeout = 100, + ServiceName = "unreachable-primary", + }; + options.EndPoints.Add(IPAddress.Loopback, 1); + return options; + } } From a98b42a0c7f356edc7c15aa7cfa72212ac7478ef Mon Sep 17 00:00:00 2001 From: Gabriel Harnagea Date: Mon, 24 Aug 2026 12:15:02 +0200 Subject: [PATCH 3/3] Apply #3185's hash-tag/slot structural fixes to this branch's shared code This branch carries commit 1c2a4725 ("Make fallback discovery and keep-alive probes cluster-slot aware") in its history, so mgravell's review of #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/StackExchange.Redis#3185 for the full review and the matching commits on that PR's branch. --- .../ClusterConfiguration.cs | 2 +- src/StackExchange.Redis/RedisServer.cs | 6 ++-- src/StackExchange.Redis/ServerEndPoint.cs | 31 +++++++++++++------ .../ServerSelectionStrategy.HashTags.cs | 17 ---------- .../ServerSelectionStrategy.cs | 23 ++------------ .../HashTagUnitTests.cs | 6 ++-- .../ServerEndPointClusterProbeUnitTests.cs | 7 +---- 7 files changed, 32 insertions(+), 60 deletions(-) diff --git a/src/StackExchange.Redis/ClusterConfiguration.cs b/src/StackExchange.Redis/ClusterConfiguration.cs index 2880cf6ac..57e2bb0c9 100644 --- a/src/StackExchange.Redis/ClusterConfiguration.cs +++ b/src/StackExchange.Redis/ClusterConfiguration.cs @@ -467,7 +467,7 @@ public IList Children /// /// Gets the parent node of the current node. /// - public ClusterNode? Parent => (parent is not null) ? parent = configuration[ParentNodeId!] : null; + public ClusterNode? Parent => ParentNodeId is null ? null : (parent ??= configuration[ParentNodeId]); /// /// Gets the unique node-id of the parent of the current node. diff --git a/src/StackExchange.Redis/RedisServer.cs b/src/StackExchange.Redis/RedisServer.cs index f8cfcdc05..7a383a9cd 100644 --- a/src/StackExchange.Redis/RedisServer.cs +++ b/src/StackExchange.Redis/RedisServer.cs @@ -65,9 +65,9 @@ public RedisKey InventKey(RedisKey prefix = default) var guid = Guid.NewGuid(); if (server.ServerType is ServerType.Cluster) { - var hashTag = multiplexer.ServerSelectionStrategy.GetHashTag(server); - if (string.IsNullOrEmpty(hashTag)) return RedisKey.Null; - return prefix.Append($"{guid}:{{{hashTag}}}"); + var slot = server.GetServableSlot(); + if (slot is null) return RedisKey.Null; + return ServerSelectionStrategy.CreateKeyForSlot(slot.Value, guid.ToString()).Prepend(prefix); } return prefix.Append(guid.ToString()); } diff --git a/src/StackExchange.Redis/ServerEndPoint.cs b/src/StackExchange.Redis/ServerEndPoint.cs index 278d98193..c74981738 100644 --- a/src/StackExchange.Redis/ServerEndPoint.cs +++ b/src/StackExchange.Redis/ServerEndPoint.cs @@ -2,7 +2,6 @@ using System.Collections; using System.Collections.Generic; using System.Diagnostics; -using System.Linq; using System.Net; using System.Runtime.CompilerServices; using System.Text; @@ -37,6 +36,8 @@ internal sealed partial class ServerEndPoint : IDisposable private bool isDisposed, replicaReadOnly, isReplica, allowReplicaWrites; private bool? supportsDatabases, supportsPrimaryWrites; private ServerType serverType; + private RedisKey tracerKey; + private int? tracerKeySlot = ServerSelectionStrategy.MultipleSlots; private volatile UnselectableFlags unselectableReasons; private Version version; @@ -380,7 +381,14 @@ public void UpdateNodeRelations(ClusterConfiguration configuration) } private ClusterNode? GetClusterNode(ClusterConfiguration? configuration) => - configuration?.Nodes.FirstOrDefault(x => x.EndPoint?.Equals(EndPoint) == true); + configuration?[EndPoint]; + + internal int? GetServableSlot() + { + if (ServerType != ServerType.Cluster || GetClusterNode(ClusterConfiguration) is not { } node) return null; + if (node.Slots.Count == 0 && node.Parent is { } parent) node = parent; + return node.Slots.Count == 0 ? null : node.Slots[0].From; + } public void SetUnselectable(UnselectableFlags flags) { @@ -505,9 +513,13 @@ internal async Task AutoConfigureAsync(PhysicalConnection? connection, ILogger? await WriteDirectOrQueueFireAndForgetAsync(connection, msg, autoConfigProcessor).ForAwait(); } } + // Cluster replicas return MOVED rather than READONLY for writes to their primary's slots, so no + // hash tag can make this role probe reliable; skip it whenever cluster mode is already known. + // On the first handshake, serverType is seeded as Standalone until the CLUSTER NODES reply is + // processed, so neither this guard nor the tie-breaker GET guard below suppresses their initial probes. else if (commandMap.IsAvailable(RedisCommand.SET) && !(helloPending || RoleKnownFromHello) - && !(ServerType == ServerType.Cluster && GetClusterNode(ClusterConfiguration) is not null)) + && ServerType != ServerType.Cluster) { // This is a nasty way to find if we are a replica, and it will only work on up-level servers, but... // (note we only get here when HELLO isn't going to tell us: the HELLO reply carries "role", and @@ -695,14 +707,15 @@ internal Message GetTracerMessage(bool checkResponse) internal RedisKey GetTracerKey() { - RedisKey key = Multiplexer.UniqueId; - if (ServerType == ServerType.Cluster - && GetClusterNode(ClusterConfiguration) is { } node - && node.Slots.Count > 0) + var slot = GetServableSlot(); + if (tracerKeySlot != slot) { - key = key.Prepend(ServerSelectionStrategy.GetHashTagPrefix(node.Slots[0].From)); + tracerKey = slot is int value + ? ServerSelectionStrategy.CreateKeyForSlot(value, Multiplexer.UniqueId) + : Multiplexer.UniqueId; + tracerKeySlot = slot; } - return key; + return tracerKey; } internal UnselectableFlags GetUnselectableFlags() => unselectableReasons; diff --git a/src/StackExchange.Redis/ServerSelectionStrategy.HashTags.cs b/src/StackExchange.Redis/ServerSelectionStrategy.HashTags.cs index a1751f50a..7e190ef91 100644 --- a/src/StackExchange.Redis/ServerSelectionStrategy.HashTags.cs +++ b/src/StackExchange.Redis/ServerSelectionStrategy.HashTags.cs @@ -1,7 +1,6 @@ using System; using System.Diagnostics; using System.Text; -using System.Threading; namespace StackExchange.Redis; @@ -11,25 +10,9 @@ internal sealed partial class ServerSelectionStrategy private static class HashTags { private static readonly string[] Cache = Populate(); - private static readonly byte[]?[] PrefixCache = new byte[TotalSlots][]; - private static readonly object PrefixCacheLock = new(); - public static ReadOnlySpan Tags => Cache; public static string Get(int slot) => Cache[slot]; - public static byte[] GetPrefix(int slot) - { - var prefix = Volatile.Read(ref PrefixCache[slot]); - if (prefix is null) - { - lock (PrefixCacheLock) - { - prefix = PrefixCache[slot] ??= Encoding.ASCII.GetBytes("{" + Get(slot) + "}"); - } - } - return prefix; - } - private static string[] Populate() { // Via testing, we know that 3 characters is sufficient to populate all slots diff --git a/src/StackExchange.Redis/ServerSelectionStrategy.cs b/src/StackExchange.Redis/ServerSelectionStrategy.cs index 8c0623e67..e039dca54 100644 --- a/src/StackExchange.Redis/ServerSelectionStrategy.cs +++ b/src/StackExchange.Redis/ServerSelectionStrategy.cs @@ -4,6 +4,7 @@ using System.Net; using System.Runtime.CompilerServices; using System.Runtime.InteropServices; +using System.Text; using System.Threading; namespace StackExchange.Redis @@ -419,30 +420,12 @@ internal bool CanServeSlot(ServerEndPoint server, int slot) return false; } - /// - /// Gets a string that can be used as a hash-tag to reference a specific slot. - /// - internal string GetHashTag(ServerEndPoint endpoint) - { - if (map is { } arr) - { - // inefficient way of finding a slot for a given endpoint, but: it'll work - for (int i = 0; i < arr.Length; i++) - { - if (arr[i] == endpoint) - { - return HashTags.Get(i); - } - } - } - return ""; - } - /// /// Gets a string that can be used as a hash-tag to reference a specific slot. /// internal static string GetHashTag(int slot) => slot < 0 ? "" : HashTags.Get(slot); - internal static RedisKey GetHashTagPrefix(int slot) => HashTags.GetPrefix(slot); + internal static RedisKey CreateKeyForSlot(int slot, RedisKey suffix) => + suffix.Prepend(Encoding.ASCII.GetBytes("{" + HashTags.Get(slot) + "}")); } } diff --git a/tests/StackExchange.Redis.Tests/HashTagUnitTests.cs b/tests/StackExchange.Redis.Tests/HashTagUnitTests.cs index 6709da826..870720a0d 100644 --- a/tests/StackExchange.Redis.Tests/HashTagUnitTests.cs +++ b/tests/StackExchange.Redis.Tests/HashTagUnitTests.cs @@ -32,12 +32,10 @@ public void TestHashTagCoverage() [InlineData(1)] [InlineData(8191)] [InlineData(16383)] - public void TestHashTagPrefixTargetsSlot(int slot) + public void TestCreateKeyForSlotTargetsSlot(int slot) { - var prefix = ServerSelectionStrategy.GetHashTagPrefix(slot); - RedisKey key = ((RedisKey)"probe-id").Prepend(prefix); + var key = ServerSelectionStrategy.CreateKeyForSlot(slot, "probe-id"); Assert.Equal(slot, ServerSelectionStrategy.GetHashSlot(key)); - Assert.Same((byte[]?)prefix, (byte[]?)ServerSelectionStrategy.GetHashTagPrefix(slot)); } } diff --git a/tests/StackExchange.Redis.Tests/ServerEndPointClusterProbeUnitTests.cs b/tests/StackExchange.Redis.Tests/ServerEndPointClusterProbeUnitTests.cs index 3ff088ceb..a9ee3be7a 100644 --- a/tests/StackExchange.Redis.Tests/ServerEndPointClusterProbeUnitTests.cs +++ b/tests/StackExchange.Redis.Tests/ServerEndPointClusterProbeUnitTests.cs @@ -29,12 +29,7 @@ public async Task ExistsTracerUsesOwnedClusterSlot() Assert.Equal(RedisCommand.EXISTS, message.Command); Assert.Equal(targetSlot, message.GetHashSlot(connection.ServerSelectionStrategy)); var key = endpoint.GetTracerKey(); - var keyBytes = (byte[]?)key; - var prefixBytes = (byte[]?)ServerSelectionStrategy.GetHashTagPrefix(targetSlot); - Assert.NotNull(keyBytes); - Assert.NotNull(prefixBytes); - Assert.True(keyBytes.Take(prefixBytes.Length).SequenceEqual(prefixBytes)); - Assert.True(keyBytes.Skip(keyBytes.Length - connection.UniqueId.Length).SequenceEqual(connection.UniqueId)); + Assert.Equal(ServerSelectionStrategy.CreateKeyForSlot(targetSlot, connection.UniqueId), key); } [Theory]