Route as a cluster only with a slot map; take HELLO mode/role from the interactive connection only

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.

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<bool> 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.
+                    bool noSlotMap = ServerSelectionStrategy.IsSlotMapEmpty;
+                    if (clusterCount > 0 && noSlotMap)
+                    {
+                        log?.LogInformationClusterWithoutSlotMap(clusterCount);
+                    }
+
+                    if (clusterCount == 0 || noSlotMap)
                     {
                         // Set the serverSelectionStrategy
                         if (RawConfig.Proxy == Proxy.Twemproxy)
@@ -2068,7 +2080,7 @@ internal async Task<bool> 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<bool> ReconfigureAsync(bool first, bool reconfigureAll, ILog
                     else
                     {
                         ServerSelectionStrategy.ServerType = ServerType.Cluster;
-                        long coveredSlots = ServerSelectionStrategy.CountCoveredSlots();
-                        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
+                        foreach (var primary in primaries)
+                        {
+                            primary.ClearUnselectable(UnselectableFlags.RedundantPrimary);
+                        }
                     }
                     if (!first)
                     {

src/StackExchange.Redis/Enums/ServerType.cs

@@ -78,5 +78,16 @@ internal static class ServerTypeExtensions
             ServerType.Envoyproxy => false,
             _ => true,
         };
+
+        /// <summary>
+        /// Whether a server of this type can be sent commands when the multiplexer is routing as <paramref name="routing"/>.
+        /// </summary>
+        /// <remarks>
+        /// 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.
+        /// </remarks>
+        internal static bool IsRoutableAs(this ServerType server, ServerType routing)
+            => server == routing || (routing == ServerType.Standalone && server == ServerType.Cluster);
     }
 }

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,

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

src/StackExchange.Redis/ServerSelectionStrategy.cs

@@ -322,6 +322,18 @@ internal bool OwnsAnySlot(ServerEndPoint server)
             return false;
         }
 
+        /// <summary>
+        /// Whether no slot has an owner, i.e. we hold no cluster topology to route by.
+        /// </summary>
+        /// <remarks>
+        /// Derived from the map rather than tracked separately, which is sound because the map is allocated only by
+        /// <see cref="MapForMutation"/>, immediately before a write; the writers never store <c>null</c> and refuse
+        /// empty ranges (see <see cref="UpdateClusterRange(int, int, ServerEndPoint)"/>); and nothing ever clears an entry -
+        /// retirement declines to drop a server the map still references (<see cref="OwnsAnySlot"/>). So a non-null
+        /// map always covers at least one slot. O(1); <see cref="CountCoveredSlots"/> is the scan, for logging.
+        /// </remarks>
+        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++)
             {

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"]);
     }
 }

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;
+
+/// <summary>
+/// A proxied deployment can answer <c>HELLO</c> with <c>mode: cluster</c> while <c>INFO</c> says
+/// <c>redis_mode:standalone</c> and every <c>CLUSTER</c> 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 <c>HELLO</c> without the <c>INFO</c> that follows
+/// on the interactive connection.
+/// </summary>
+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<bool>(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<bool> 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);
+        }
+    }
+}

tests/StackExchange.Redis.Tests/SlotMapUnitTests.cs

@@ -0,0 +1,33 @@
+using System.Threading.Tasks;
+using Xunit;
+
+namespace StackExchange.Redis.Tests;
+
+/// <summary>
+/// <see cref="ServerSelectionStrategy.IsSlotMapEmpty"/> 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).
+/// </summary>
+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());
+    }
+}

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
         /// </summary>
         protected virtual IReadOnlyList<KeyValuePair<string, string>> HelloExtraFields => [];
 
+        /// <summary>
+        /// The <c>mode</c> reported by <c>HELLO</c>; by default the same as <c>INFO</c>'s <c>redis_mode</c>, but a
+        /// proxy can disagree with itself (see #3284), so tests can override this independently.
+        /// </summary>
+        protected virtual string HelloModeValue => ServerModeValue;
+
         [RedisCommand(-3)]
         protected virtual TypedRedisValue Sadd(RedisClient client, in RedisRequest request)
         {