From a080f5478061ec25aaee12462bec33b7f6c7f060 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Thu, 22 Oct 2026 10:15:48 +0100 Subject: [PATCH 1/2] Route as a cluster only with a slot map; take HELLO mode/role from the interactive connection (#3284) A proxied deployment can answer HELLO with "mode: cluster" while INFO says redis_mode:standalone and every CLUSTER subcommand is refused (Azure Managed Redis with the Enterprise clustering policy). Under RESP3 pub/sub now has its own connection (#3264), whose handshake is HELLO alone: when that leg alone reconnected, its HELLO retyped the server as a cluster with no INFO to follow, the resulting reconfigure put the multiplexer into cluster mode with no slot map, and cross-slot commands were refused client-side. - Cluster *routing* now requires a slot map: servers that identify as a cluster but give us no map are routed as a single standalone server (new event id 133). The map is never cleared once populated, so an established cluster whose refresh is refused (#3275) keeps routing as one. When a map does arrive, stale redundant-primary flags from an earlier election are cleared. - AnyServer accepts a cluster-typed server while routing as standalone (ServerType.IsRoutableAs), so such a server stays a valid target. - HELLO "mode" and "role" are applied only from the interactive connection; the subscription connection does no discovery and must not overwrite server-level facts. Tests: HelloClusterModeUnitTests reproduces the subscription-only reconnect under both protocols (fails under RESP3 before this change); ClusterCommandsRefusedUnitTests asserts standalone routing and a working cross-slot MGET when CLUSTER is refused. The in-process server gains an overridable HelloModeValue so HELLO and INFO can disagree. --- docs/Configuration.md | 4 +- .../ConnectionMultiplexer.cs | 26 ++++- src/StackExchange.Redis/Enums/ServerType.cs | 11 +++ src/StackExchange.Redis/LoggerExtensions.cs | 6 ++ src/StackExchange.Redis/ResultProcessor.cs | 13 ++- .../ClusterCommandsRefusedUnitTests.cs | 9 ++ .../HelloClusterModeUnitTests.cs | 99 +++++++++++++++++++ .../StackExchange.Redis.Server/RedisServer.cs | 8 +- 8 files changed, 167 insertions(+), 9 deletions(-) create mode 100644 tests/StackExchange.Redis.Tests/HelloClusterModeUnitTests.cs diff --git a/docs/Configuration.md b/docs/Configuration.md index ac044e4dc..1df31a2ae 100644 --- a/docs/Configuration.md +++ b/docs/Configuration.md @@ -422,6 +422,8 @@ The library determines whether to use RESP3 by: - Otherwise: RESP3 is attempted if `defaultVersion` (6.0 unless overridden) is v6 or above Note that `HELLO` is issued either way: `HELLO 2` when staying on RESP2. The reply tells us the server version, replication role, mode (standalone/sentinel/cluster) and connection -identifier, none of which we would otherwise know without `INFO` or `CONFIG GET` - both of which are in the `@dangerous` ACL category, and are commonly restricted. `HELLO` itself is +identifier, none of which we would otherwise know without `INFO` or `CONFIG GET` - both of which are in the `@dangerous` ACL category, and are commonly restricted. The role and mode +are only taken from the interactive connection (the subscription connection does no discovery), and a reported mode of `cluster` only leads to cluster *routing* once a slot map has +been obtained from `CLUSTER SLOTS`/`CLUSTER NODES`; a proxy that announces a cluster but refuses those commands is routed as a single standalone server. `HELLO` itself is in the `@connection` category. If you need to prevent it (for example when talking to a proxy that does not understand it), disable it at the command-map level with `$hello=` in the configuration string - or specify a `defaultVersion` below 6.0, since `HELLO` did not exist before then. diff --git a/src/StackExchange.Redis/ConnectionMultiplexer.cs b/src/StackExchange.Redis/ConnectionMultiplexer.cs index a764e574d..529d7ccc6 100644 --- a/src/StackExchange.Redis/ConnectionMultiplexer.cs +++ b/src/StackExchange.Redis/ConnectionMultiplexer.cs @@ -1518,7 +1518,7 @@ public IDatabase GetDatabase(int db = -1, object? asyncState = null) for (int i = 0; i < len; i++) { var server = tmp[(int)(((uint)i + startOffset) % len)]; - if (server != null && server.ServerType == serverType && server.IsSelectable(command, allowDisconnected)) + if (server != null && server.ServerType.IsRoutableAs(serverType) && server.IsSelectable(command, allowDisconnected)) { if (server.IsReplica) { @@ -2046,7 +2046,19 @@ internal async Task ReconfigureAsync(bool first, bool reconfigureAll, ILog } } - if (clusterCount == 0) + // A server *saying* it is a cluster is not enough to route as one: that needs a slot map, and + // proxied deployments can announce "cluster" while refusing every CLUSTER subcommand (AMR with + // the Enterprise clustering policy, #3284). With no map there is nothing to route by and the + // only thing cluster mode would add is rejecting cross-slot commands client-side, which the + // proxy would have accepted. The map is never cleared once populated, so an established + // cluster whose refresh is refused (#3275) keeps routing as one. + long coveredSlots = clusterCount > 0 ? ServerSelectionStrategy.CountCoveredSlots() : 0; + if (clusterCount > 0 && coveredSlots == 0) + { + log?.LogInformationClusterWithoutSlotMap(clusterCount); + } + + if (clusterCount == 0 || coveredSlots == 0) { // Set the serverSelectionStrategy if (RawConfig.Proxy == Proxy.Twemproxy) @@ -2068,7 +2080,7 @@ internal async Task ReconfigureAsync(bool first, bool reconfigureAll, ILog InitializeSentinel(log); } } - else if (standaloneCount > 0) + else if (standaloneCount > 0 || clusterCount > 0) { ServerSelectionStrategy.ServerType = ServerType.Standalone; } @@ -2097,8 +2109,14 @@ internal async Task ReconfigureAsync(bool first, bool reconfigureAll, ILog else { ServerSelectionStrategy.ServerType = ServerType.Cluster; - long coveredSlots = ServerSelectionStrategy.CountCoveredSlots(); log?.LogInformationClusterSlotsCovered(coveredSlots, ServerSelectionStrategy.TotalSlots); + + // a cluster has many primaries by design; if we routed as standalone before the map + // arrived, the nomination above may have marked all but one of them redundant + foreach (var primary in primaries) + { + primary.ClearUnselectable(UnselectableFlags.RedundantPrimary); + } } if (!first) { diff --git a/src/StackExchange.Redis/Enums/ServerType.cs b/src/StackExchange.Redis/Enums/ServerType.cs index e7a84ad0a..d7dcccd7b 100644 --- a/src/StackExchange.Redis/Enums/ServerType.cs +++ b/src/StackExchange.Redis/Enums/ServerType.cs @@ -78,5 +78,16 @@ internal static class ServerTypeExtensions ServerType.Envoyproxy => false, _ => true, }; + + /// + /// Whether a server of this type can be sent commands when the multiplexer is routing as . + /// + /// + /// Usually the types match, and the check keeps data traffic away from sentinels; but a server that calls itself + /// a cluster while giving us no slot map is routed as a single standalone server (see #3284), so it must remain a + /// target in that mode. + /// + internal static bool IsRoutableAs(this ServerType server, ServerType routing) + => server == routing || (routing == ServerType.Standalone && server == ServerType.Cluster); } } diff --git a/src/StackExchange.Redis/LoggerExtensions.cs b/src/StackExchange.Redis/LoggerExtensions.cs index df661e32e..8916f25de 100644 --- a/src/StackExchange.Redis/LoggerExtensions.cs +++ b/src/StackExchange.Redis/LoggerExtensions.cs @@ -280,6 +280,12 @@ internal static void LogWithThreadPoolStats(this ILogger? log, string message) Message = "Cluster: {CoveredSlots} of {TotalSlots} slots covered")] internal static partial void LogInformationClusterSlotsCovered(this ILogger logger, long coveredSlots, int totalSlots); + [LoggerMessage( + Level = LogLevel.Information, + EventId = 133, + Message = "{ClusterCount} server(s) identify as a cluster but no slot map is available; routing as standalone")] + internal static partial void LogInformationClusterWithoutSlotMap(this ILogger logger, int clusterCount); + [LoggerMessage( Level = LogLevel.Information, EventId = 35, diff --git a/src/StackExchange.Redis/ResultProcessor.cs b/src/StackExchange.Redis/ResultProcessor.cs index cd6992312..e985b10f2 100644 --- a/src/StackExchange.Redis/ResultProcessor.cs +++ b/src/StackExchange.Redis/ResultProcessor.cs @@ -1157,6 +1157,11 @@ protected override bool SetResultCore(PhysicalConnection connection, Message mes // version ("server" => "valkey", "version" => "9.1.2", while INFO still says redis_version:7.2.4), // so we can only decide what to do with "version" once the whole map has been read (#3278). bool serverIsRedis = true; // absent means redis (older servers) + // "mode" and "role" describe the *server*, and only the interactive connection does discovery: + // the subscription connection's handshake is HELLO alone, with no INFO to follow, so a proxy + // whose HELLO disagrees with its INFO (AMR says "cluster" to one and "standalone" to the other, + // #3284) would otherwise have the last word every time pub/sub reconnects on its own + bool helloFromInteractive = connection.BridgeCouldBeNull?.ConnectionType == ConnectionType.Interactive; ProductVariant? helloVariant = null, vendorVariant = null; string? vendorVersion = null; // a snapshot of the "version" value (IsScalar when present), so we can read it as a Version or a @@ -1226,15 +1231,17 @@ protected override bool SetResultCore(PhysicalConnection connection, Message mes // note: "mode" and "role" describe a *server*, so we only trust them when we're // talking to one directly; a proxy may answer HELLO from an arbitrary backend node // (envoy 1.39 does), and taking that at face value would flip us into cluster mode - // or mark the proxy as a replica + // or mark the proxy as a replica - and only from the interactive connection (above) case HelloField.Mode - when server.ServerType.SupportsAutoConfigure() + when helloFromInteractive + && server.ServerType.SupportsAutoConfigure() && iter.Value.TryParseScalar(&ServerTypeMetadata.TryParse, out ServerType serverType): server.ServerType = serverType; Log?.LogInformationAutoConfiguredHelloServerType(new(server), serverType); break; case HelloField.Role - when server.ServerType.SupportsAutoConfigure() + when helloFromInteractive + && server.ServerType.SupportsAutoConfigure() && iter.Value.TryParseScalar(&KnownRoleMetadata.TryParse, out bool isReplica): server.IsReplica = isReplica; server.RoleKnownFromHello = true; // so we don't need the key-based fallback probe diff --git a/tests/StackExchange.Redis.Tests/ClusterCommandsRefusedUnitTests.cs b/tests/StackExchange.Redis.Tests/ClusterCommandsRefusedUnitTests.cs index 1f6e34de5..1932824dc 100644 --- a/tests/StackExchange.Redis.Tests/ClusterCommandsRefusedUnitTests.cs +++ b/tests/StackExchange.Redis.Tests/ClusterCommandsRefusedUnitTests.cs @@ -24,6 +24,9 @@ protected override TypedRedisValue ClusterSlots(RedisClient client, in RedisRequ protected override TypedRedisValue ClusterNodes(RedisClient client, in RedisRequest request) => refuseNodes ? TypedRedisValue.Error("ERR command is not allowed") : base.ClusterNodes(client, request); + + // a proxy that hides the shards accepts cross-slot commands itself + public override bool CheckCrossSlot => false; } [Fact] @@ -71,5 +74,11 @@ public async Task AllClusterCommandsRefusedIsQuiet() // ...and a later explicit reconfigure stays quiet too Assert.True(await conn.ReconfigureAsync("test")); Assert.Empty(logger.Matching("Encountered error while updating cluster config")); + + // with no slot map there is nothing to route by, so we must not *route* as a cluster - in particular, + // cross-slot commands go to the proxy rather than being refused client-side (#3284) + Assert.NotEmpty(logger.Matching("no slot map is available; routing as standalone")); + Assert.Equal(ServerType.Standalone, conn.ServerSelectionStrategy.ServerType); + await conn.GetDatabase().StringGetAsync(["a", "b"]); } } diff --git a/tests/StackExchange.Redis.Tests/HelloClusterModeUnitTests.cs b/tests/StackExchange.Redis.Tests/HelloClusterModeUnitTests.cs new file mode 100644 index 000000000..c5b827969 --- /dev/null +++ b/tests/StackExchange.Redis.Tests/HelloClusterModeUnitTests.cs @@ -0,0 +1,99 @@ +using System; +using System.Threading.Tasks; +using StackExchange.Redis.Server; +using Xunit; + +namespace StackExchange.Redis.Tests; + +/// +/// A proxied deployment can answer HELLO with mode: cluster while INFO says +/// redis_mode:standalone and every CLUSTER subcommand is refused (Azure Managed Redis with the +/// Enterprise clustering policy; see #3284). Such a server gives the client no slot map, so it must not be +/// treated as a cluster - in particular, cross-slot commands must keep working, including after a reconnect +/// of the subscription connection alone, whose handshake is HELLO without the INFO that follows +/// on the interactive connection. +/// +public class HelloClusterModeUnitTests(ITestOutputHelper log) +{ + private sealed class HelloSaysClusterServer(ITestOutputHelper log) : InProcessTestServer(log) + { + protected override string HelloModeValue => "cluster"; + + protected override TypedRedisValue ClusterSlots(RedisClient client, in RedisRequest request) + => TypedRedisValue.Error("ERR command is not allowed"); + + protected override TypedRedisValue ClusterNodes(RedisClient client, in RedisRequest request) + => TypedRedisValue.Error("ERR command is not allowed"); + } + + [Theory] + [InlineData(RedisProtocol.Resp2)] + [InlineData(RedisProtocol.Resp3)] + public async Task CrossSlotSurvivesSubscriptionReconnect(RedisProtocol protocol) + { + using var server = new HelloSaysClusterServer(log); // ServerType stays Standalone: INFO says so + + var config = server.GetClientConfig(defaultOnly: true); + config.Protocol = protocol; + config.AllowSimulateConnectionFailure = true; + config.AbortOnConnectFail = false; + var logger = new CapturingLogger(); + config.LoggerFactory = logger; + await using var conn = await ConnectionMultiplexer.ConnectAsync(config); + var db = conn.GetDatabase(); + + RedisKey[] crossSlot = ["a", "b"]; // different hash slots + await db.StringGetAsync(crossSlot); // sanity: fine on a fresh connection + Assert.Equal(ServerType.Standalone, conn.ServerSelectionStrategy.ServerType); + + var endpoint = conn.GetServerSnapshot()[0]; + await WaitUntilAsync(() => endpoint.IsSubscriberConnected, "subscriber connected"); + + // drop *only* the subscription connection, as the server did in #3284; the reconnect is too quick + // to observe by polling, so watch for the restored event instead + var restored = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + conn.ConnectionRestored += (_, e) => + { + if (e.ConnectionType == ConnectionType.Subscription) restored.TrySetResult(true); + }; + log.WriteLine("dropping the subscription connection"); + endpoint.SimulateConnectionFailure(SimulatedFailureType.AllSubscription); + await restored.Task.WaitAsync(TimeSpan.FromSeconds(10)); + await WaitUntilAsync(() => endpoint.IsSubscriberConnected, "subscriber reconnected"); + + // the restored connection triggers a fire-and-forget reconfigure (and so can a server-type change); + // ReconfigureAsync declines while one is in flight, so insist on one that actually ran + var deadline = DateTime.UtcNow.AddSeconds(10); + while (!await conn.ReconfigureAsync("after subscription reconnect")) + { + if (DateTime.UtcNow > deadline) throw new TimeoutException("Timed out waiting for a reconfigure to run"); + await Task.Delay(50); + } + + try + { + // the user-visible symptom from #3284: cross-slot commands refused client-side + for (int i = 0; i < 5; i++) + { + await db.StringGetAsync(crossSlot); + await Task.Delay(100); + } + Assert.Equal(ServerType.Standalone, endpoint.ServerType); + Assert.Equal(ServerType.Standalone, conn.ServerSelectionStrategy.ServerType); + } + finally + { + log.WriteLine(logger.All); + } + } + + private static async Task WaitUntilAsync(Func condition, string what) + { + var deadline = DateTime.UtcNow.AddSeconds(10); + while (!condition()) + { + if (DateTime.UtcNow > deadline) throw new TimeoutException("Timed out waiting for: " + what); + await Task.Delay(50); + } + } +} diff --git a/toys/StackExchange.Redis.Server/RedisServer.cs b/toys/StackExchange.Redis.Server/RedisServer.cs index a0862ff86..5289b8791 100644 --- a/toys/StackExchange.Redis.Server/RedisServer.cs +++ b/toys/StackExchange.Redis.Server/RedisServer.cs @@ -675,7 +675,7 @@ protected virtual TypedRedisValue Hello(RedisClient client, in RedisRequest requ span[index++] = TypedRedisValue.BulkString("id"); span[index++] = TypedRedisValue.Integer(client.Id); span[index++] = TypedRedisValue.BulkString("mode"); - span[index++] = TypedRedisValue.BulkString(ServerModeValue); + span[index++] = TypedRedisValue.BulkString(HelloModeValue); span[index++] = TypedRedisValue.BulkString("role"); span[index++] = TypedRedisValue.BulkString(GetPrimaryOf(client?.Node) is null ? "master" : "replica"); span[index++] = TypedRedisValue.BulkString("modules"); @@ -699,6 +699,12 @@ protected virtual TypedRedisValue Hello(RedisClient client, in RedisRequest requ /// protected virtual IReadOnlyList> HelloExtraFields => []; + /// + /// The mode reported by HELLO; by default the same as INFO's redis_mode, but a + /// proxy can disagree with itself (see #3284), so tests can override this independently. + /// + protected virtual string HelloModeValue => ServerModeValue; + [RedisCommand(-3)] protected virtual TypedRedisValue Sadd(RedisClient client, in RedisRequest request) { From 8d6e3cc1f85904b31ba3fdf8f37c6e87ff00b5b2 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Thu, 22 Oct 2026 10:55:44 +0100 Subject: [PATCH 2/2] Derive the slot-map check from allocation; reject inverted or out-of-range slot ranges The routing decision in ReconfigureAsync needs only "is there any slot map", not a count. Add ServerSelectionStrategy.IsSlotMapEmpty, derived from whether the map has been allocated: the map is allocated only immediately before a write, writers never store null, and nothing clears an entry, so a non-null map always covers at least one slot. CountCoveredSlots remains for the log line in the cluster branch, where it has always been. That derivation is only sound if nothing can allocate without writing, so UpdateClusterRange now ignores an inverted or out-of-range range instead of allocating and then writing nothing (or indexing out of bounds). SlotRange does not validate ordering and the reply-driven paths build ranges straight from what the server sent. SlotMapUnitTests covers the rejected ranges leaving the map unallocated. --- .../ConnectionMultiplexer.cs | 8 ++--- .../ServerSelectionStrategy.cs | 17 ++++++++++ .../SlotMapUnitTests.cs | 33 +++++++++++++++++++ 3 files changed, 54 insertions(+), 4 deletions(-) create mode 100644 tests/StackExchange.Redis.Tests/SlotMapUnitTests.cs diff --git a/src/StackExchange.Redis/ConnectionMultiplexer.cs b/src/StackExchange.Redis/ConnectionMultiplexer.cs index 529d7ccc6..14b2815b0 100644 --- a/src/StackExchange.Redis/ConnectionMultiplexer.cs +++ b/src/StackExchange.Redis/ConnectionMultiplexer.cs @@ -2052,13 +2052,13 @@ internal async Task ReconfigureAsync(bool first, bool reconfigureAll, ILog // only thing cluster mode would add is rejecting cross-slot commands client-side, which the // proxy would have accepted. The map is never cleared once populated, so an established // cluster whose refresh is refused (#3275) keeps routing as one. - long coveredSlots = clusterCount > 0 ? ServerSelectionStrategy.CountCoveredSlots() : 0; - if (clusterCount > 0 && coveredSlots == 0) + bool noSlotMap = ServerSelectionStrategy.IsSlotMapEmpty; + if (clusterCount > 0 && noSlotMap) { log?.LogInformationClusterWithoutSlotMap(clusterCount); } - if (clusterCount == 0 || coveredSlots == 0) + if (clusterCount == 0 || noSlotMap) { // Set the serverSelectionStrategy if (RawConfig.Proxy == Proxy.Twemproxy) @@ -2109,7 +2109,7 @@ internal async Task ReconfigureAsync(bool first, bool reconfigureAll, ILog else { ServerSelectionStrategy.ServerType = ServerType.Cluster; - log?.LogInformationClusterSlotsCovered(coveredSlots, ServerSelectionStrategy.TotalSlots); + log?.LogInformationClusterSlotsCovered(ServerSelectionStrategy.CountCoveredSlots(), ServerSelectionStrategy.TotalSlots); // a cluster has many primaries by design; if we routed as standalone before the map // arrived, the nomination above may have marked all but one of them redundant diff --git a/src/StackExchange.Redis/ServerSelectionStrategy.cs b/src/StackExchange.Redis/ServerSelectionStrategy.cs index 2c79aa804..a3ea96c2d 100644 --- a/src/StackExchange.Redis/ServerSelectionStrategy.cs +++ b/src/StackExchange.Redis/ServerSelectionStrategy.cs @@ -322,6 +322,18 @@ internal bool OwnsAnySlot(ServerEndPoint server) return false; } + /// + /// Whether no slot has an owner, i.e. we hold no cluster topology to route by. + /// + /// + /// Derived from the map rather than tracked separately, which is sound because the map is allocated only by + /// , immediately before a write; the writers never store null and refuse + /// empty ranges (see ); and nothing ever clears an entry - + /// retirement declines to drop a server the map still references (). So a non-null + /// map always covers at least one slot. O(1); is the scan, for logging. + /// + internal bool IsSlotMapEmpty => map is null; + internal int CountCoveredSlots() { var arr = map; @@ -334,6 +346,11 @@ internal int CountCoveredSlots() internal void UpdateClusterRange(int fromInclusive, int toInclusive, ServerEndPoint server) { + // never allocate without writing (see IsSlotMapEmpty): SlotRange does not validate ordering, and the + // reply-driven paths build ranges straight from what the server sent - an inverted or out-of-range + // range is garbage, so ignore it rather than guess at what was meant (or index out of bounds) + if (fromInclusive < 0 || toInclusive >= RedisClusterSlotCount || fromInclusive > toInclusive) return; + var arr = MapForMutation(); for (int i = fromInclusive; i <= toInclusive; i++) { diff --git a/tests/StackExchange.Redis.Tests/SlotMapUnitTests.cs b/tests/StackExchange.Redis.Tests/SlotMapUnitTests.cs new file mode 100644 index 000000000..3ab329e0d --- /dev/null +++ b/tests/StackExchange.Redis.Tests/SlotMapUnitTests.cs @@ -0,0 +1,33 @@ +using System.Threading.Tasks; +using Xunit; + +namespace StackExchange.Redis.Tests; + +/// +/// is derived from whether the map has been allocated, which is +/// only sound if nothing can allocate it without writing an owner into it (see #3284/#3285). +/// +public class SlotMapUnitTests(ITestOutputHelper log) +{ + [Fact] + public async Task RejectedRangesDoNotAllocateTheMap() + { + using var server = new InProcessTestServer(log); + await using var conn = await ConnectionMultiplexer.ConnectAsync(server.GetClientConfig(withPubSub: false, defaultOnly: true)); + var strategy = conn.ServerSelectionStrategy; + var endpoint = conn.GetServerSnapshot()[0]; + + Assert.True(strategy.IsSlotMapEmpty); // standalone: nothing has been applied + Assert.Equal(0, strategy.CountCoveredSlots()); + + strategy.UpdateClusterRange(5, 3, endpoint); // inverted + strategy.UpdateClusterRange(-1, 3, endpoint); // below range + strategy.UpdateClusterRange(16380, 16384, endpoint); // above range + Assert.True(strategy.IsSlotMapEmpty); + Assert.Equal(0, strategy.CountCoveredSlots()); + + strategy.UpdateClusterRange(3, 5, endpoint); + Assert.False(strategy.IsSlotMapEmpty); + Assert.Equal(3, strategy.CountCoveredSlots()); + } +}