From 1cace619ee0d7faa4eb20452e71e9c6804880135 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Fri, 14 Aug 2026 12:57:13 +0100 Subject: [PATCH 1/2] Always issue HELLO when the server should understand it, not just for RESP3 Fixes #2968 (remaining concern). When INFO is unavailable, discovery fell back to `SET {guid} replica-read-only PX 1 NX` to detect a read-only replica. That probe writes a random, unprefixable key, so it can never be allow-listed by an ACL key pattern - which is exactly the situation that makes INFO unavailable in the first place (INFO and CONFIG are both @dangerous). HELLO's reply carries "role", needs no key, and is in the @connection category. So: issue HELLO whenever it is available and the assumed server version is 6.0+, and skip the key-based probe when HELLO is going to tell us (or has already told us) the role. Two independent opt-outs remain: `$hello=` in the command map, and a sub-6.0 `defaultVersion`. As a bonus, RESP2 connections now learn the server version, mode and connection id from the handshake as well. The RESP2 HELLO is deliberately not the same message as the RESP3 one: - `HELLO 3` stays first-in-pipeline and carries the credentials, as it must for RESP3 negotiation to happen at all on a secured server. - `HELLO 2` is a bare HELLO issued *after* AUTH, on the interactive connection only. It isn't negotiating anything, and folding credentials in would change how credential failures surface (there is a pre-existing difference in behaviour between AUTH failing on its own and AUTH failing inside HELLO; that is a separate bug, not one to inherit here). Also excludes HELLO from the twemproxy and envoyproxy command maps, and verifies against both proxies locally: - twemproxy 0.5.0 (the newest release) *closes the connection* on an unsupported command, so HELLO is fatal. Since v3 raised the assumed default version to 6.0, RESP3 - and therefore HELLO - became the default, and a default-configured twemproxy connection never became usable at all. That is a v3 regression against v2, where the assumed version of 3.0 meant no HELLO was ever sent. - envoy up to ~1.31 answers "unsupported command"; 1.39 instead proxies HELLO to an arbitrary backend node, so the version/role/mode in the reply describe some other server. Taking that at face value would flip a proxy endpoint into cluster mode or mark it as a replica, so HELLO's mode/role are now only applied to server types that support auto-configure - proxies are excluded even if a hand-rolled command map re-enables the command. Bumps the test-topology envoy pin from v1.31 to v1.39 (8 minors stale, and the two versions behave differently here). Tests use a recording in-process server to pin exactly which commands the handshake issues: HELLO at the right protocol level, absent when disabled either way, and the replica probe present only when HELLO cannot tell us the role. The Resp3HandshakeTests matrix no longer skips RESP2 clients (they issue HELLO now too), and asserts the negotiated protocol. --- docs/Configuration.md | 14 +- src/StackExchange.Redis/CommandMap.cs | 8 ++ .../ConfigurationOptions.cs | 19 +++ src/StackExchange.Redis/Enums/ServerType.cs | 2 +- src/StackExchange.Redis/LoggerExtensions.cs | 6 + .../PublicAPI/PublicAPI.Unshipped.txt | 3 +- src/StackExchange.Redis/RedisFeatures.cs | 7 + src/StackExchange.Redis/ResultProcessor.cs | 11 +- src/StackExchange.Redis/ServerEndPoint.cs | 56 +++++++- tests/RedisConfigs/.docker/Envoy/Dockerfile | 2 +- .../CommandMapUnitTests.cs | 13 ++ .../HelloHandshakeTests.cs | 133 ++++++++++++++++++ .../Resp3HandshakeTests.cs | 20 +-- 13 files changed, 268 insertions(+), 26 deletions(-) create mode 100644 tests/StackExchange.Redis.Tests/HelloHandshakeTests.cs diff --git a/docs/Configuration.md b/docs/Configuration.md index 5baf66760..4ad94935b 100644 --- a/docs/Configuration.md +++ b/docs/Configuration.md @@ -299,13 +299,17 @@ config.ReconnectRetryPolicy = new LinearRetry(5000); ## Redis protocol -Without specific configuration, StackExchange.Redis will use the RESP2 protocol; this means that pub/sub requires a separate connection to the server. RESP3 is a newer protocol -(usually, but not always, available on v6 servers and above) which allows (among other changes) pub/sub messages to be communicated on the *same* connection - which can be very -desirable in servers with a large number of clients. The protocol handshake needs to happen very early in the connection, so *by default* the library does not attempt a RESP3 connection -unless it has reason to expect it to work. +RESP3 is a newer protocol (available on v6 servers and above) which allows (among other changes) pub/sub messages to be communicated on the *same* connection - which can be very +desirable in servers with a large number of clients; under RESP2, pub/sub requires a separate connection to the server. The protocol handshake needs to happen very early in the +connection, so the library only attempts RESP3 when it has reason to expect it to work. The library determines whether to use RESP3 by: - The `HELLO` command has been disabled: RESP2 is used - A protocol *other than* `resp3` or `3` is specified: RESP2 is used - A protocol of `resp3` or `3` is specified: RESP3 is attempted (with fallback if it fails) -- In all other scenarios: RESP2 is used +- 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 +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/CommandMap.cs b/src/StackExchange.Redis/CommandMap.cs index 24a1aec97..4b1386432 100644 --- a/src/StackExchange.Redis/CommandMap.cs +++ b/src/StackExchange.Redis/CommandMap.cs @@ -46,6 +46,9 @@ private CommandMap(CommandBytes[] map, byte[] bytes) RedisCommand.ECHO, RedisCommand.SELECT, + // twemproxy *closes the connection* on an unsupported command, so HELLO is fatal, not just useless + RedisCommand.HELLO, + RedisCommand.BGREWRITEAOF, RedisCommand.BGSAVE, RedisCommand.CLIENT, RedisCommand.CLUSTER, RedisCommand.CONFIG, RedisCommand.DBSIZE, RedisCommand.DEBUG, RedisCommand.FLUSHALL, RedisCommand.FLUSHDB, RedisCommand.INFO, RedisCommand.LASTSAVE, RedisCommand.MONITOR, RedisCommand.REPLICAOF, RedisCommand.SAVE, RedisCommand.SHUTDOWN, RedisCommand.SLAVEOF, RedisCommand.SLOWLOG, RedisCommand.SYNC, RedisCommand.TIME, RedisCommand.HOTKEYS, @@ -70,6 +73,11 @@ private CommandMap(CommandBytes[] map, byte[] bytes) RedisCommand.SELECT, + // envoy either rejects HELLO ("unsupported command", up to ~1.31) or proxies it to an arbitrary + // backend node (1.39+), so the version/role/mode it reports describe *some* server, not the endpoint + // we're talking to; either way we don't want it + RedisCommand.HELLO, + RedisCommand.BGREWRITEAOF, RedisCommand.BGSAVE, RedisCommand.CLIENT, RedisCommand.CLUSTER, RedisCommand.CONFIG, RedisCommand.DBSIZE, RedisCommand.DEBUG, RedisCommand.FLUSHALL, RedisCommand.FLUSHDB, RedisCommand.INFO, RedisCommand.LASTSAVE, RedisCommand.MONITOR, RedisCommand.REPLICAOF, RedisCommand.SAVE, RedisCommand.SHUTDOWN, RedisCommand.SLAVEOF, RedisCommand.SLOWLOG, RedisCommand.SYNC, RedisCommand.TIME, RedisCommand.HOTKEYS, diff --git a/src/StackExchange.Redis/ConfigurationOptions.cs b/src/StackExchange.Redis/ConfigurationOptions.cs index 4860c5b07..12db0bac1 100644 --- a/src/StackExchange.Redis/ConfigurationOptions.cs +++ b/src/StackExchange.Redis/ConfigurationOptions.cs @@ -1463,6 +1463,25 @@ internal bool TryResp3() return use3 && CommandMap.IsAvailable(RedisCommand.HELLO); } + /// + /// Determines whether to issue HELLO as part of the handshake, and at which protocol level. + /// + /// We want HELLO even when staying on RESP2, because the reply tells us the server version, + /// role, mode and connection identifier - none of which we would otherwise know without INFO + /// or CONFIG, which are commonly restricted by ACLs (both are @dangerous). + internal bool TryHello(out int protocolVersion) + { + // HELLO arrived in 6.0, at the same time as RESP3; the command-map and the assumed server + // version are therefore both ways of opting out (`$hello=` and `defaultVersion=5.0`, say) + if (CommandMap.IsAvailable(RedisCommand.HELLO) && new RedisFeatures(DefaultVersion).Hello) + { + protocolVersion = TryResp3() ? 3 : 2; + return true; + } + protocolVersion = 0; + return false; + } + internal static bool TryParseRedisProtocol(string? value, out RedisProtocol protocol) { // accept raw integers too, but only trust them if we recognize them diff --git a/src/StackExchange.Redis/Enums/ServerType.cs b/src/StackExchange.Redis/Enums/ServerType.cs index f364765e9..e7a84ad0a 100644 --- a/src/StackExchange.Redis/Enums/ServerType.cs +++ b/src/StackExchange.Redis/Enums/ServerType.cs @@ -70,7 +70,7 @@ internal static class ServerTypeExtensions }; /// - /// Whether a server type supports . + /// Whether a server type supports . /// internal static bool SupportsAutoConfigure(this ServerType type) => type switch { diff --git a/src/StackExchange.Redis/LoggerExtensions.cs b/src/StackExchange.Redis/LoggerExtensions.cs index 381ee4426..6afa716ec 100644 --- a/src/StackExchange.Redis/LoggerExtensions.cs +++ b/src/StackExchange.Redis/LoggerExtensions.cs @@ -709,4 +709,10 @@ internal static void LogWithThreadPoolStats(this ILogger? log, string message) EventId = 109, Message = "Service name not defined.")] internal static partial void LogInformationServiceNameNotDefined(this ILogger logger); + + [LoggerMessage( + Level = LogLevel.Information, + EventId = 110, + Message = "{Server}: Requesting server details via HELLO")] + internal static partial void LogInformationDiscoveryViaHello(this ILogger logger, ServerEndPointLogValue server); } diff --git a/src/StackExchange.Redis/PublicAPI/PublicAPI.Unshipped.txt b/src/StackExchange.Redis/PublicAPI/PublicAPI.Unshipped.txt index 821995702..f05af89aa 100644 --- a/src/StackExchange.Redis/PublicAPI/PublicAPI.Unshipped.txt +++ b/src/StackExchange.Redis/PublicAPI/PublicAPI.Unshipped.txt @@ -1,6 +1,7 @@ -#nullable enable +#nullable enable StackExchange.Redis.ConfigurationOptions.SentinelPassword.get -> string? StackExchange.Redis.ConfigurationOptions.SentinelPassword.set -> void StackExchange.Redis.ConfigurationOptions.SentinelUser.get -> string? StackExchange.Redis.ConfigurationOptions.SentinelUser.set -> void +StackExchange.Redis.RedisFeatures.Hello.get -> bool [SER009]virtual StackExchange.Redis.Configuration.Tunnel.ConnectTransportAsync(System.Net.EndPoint! endpoint, StackExchange.Redis.ConnectionType connectionType, System.Threading.CancellationToken cancellationToken) -> System.Threading.Tasks.ValueTask diff --git a/src/StackExchange.Redis/RedisFeatures.cs b/src/StackExchange.Redis/RedisFeatures.cs index a2a0c0726..3e30a65dc 100644 --- a/src/StackExchange.Redis/RedisFeatures.cs +++ b/src/StackExchange.Redis/RedisFeatures.cs @@ -289,6 +289,13 @@ public RedisFeatures(Version version) /// public bool Resp3 => Version.IsAtLeast(v6_0_0); + /// + /// Is the HELLO handshake available? + /// + /// This is useful even when staying on RESP2; HELLO 2 reports the server + /// version, role, mode and connection identifier without needing INFO or CONFIG. + public bool Hello => Version.IsAtLeast(v6_0_0); + /// /// Are the IF* modifiers on SET available? /// diff --git a/src/StackExchange.Redis/ResultProcessor.cs b/src/StackExchange.Redis/ResultProcessor.cs index 45a09cfbc..36e3045c9 100644 --- a/src/StackExchange.Redis/ResultProcessor.cs +++ b/src/StackExchange.Redis/ResultProcessor.cs @@ -1103,14 +1103,21 @@ protected override bool SetResultCore(PhysicalConnection connection, Message mes connection.ConnectionId = i64; Log?.LogInformationAutoConfiguredHelloConnectionId(new(server), i64); break; + // 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 case HelloField.Mode - when iter.Value.TryParseScalar(&ServerTypeMetadata.TryParse, out ServerType serverType): + when server.ServerType.SupportsAutoConfigure() + && iter.Value.TryParseScalar(&ServerTypeMetadata.TryParse, out ServerType serverType): server.ServerType = serverType; Log?.LogInformationAutoConfiguredHelloServerType(new(server), serverType); break; case HelloField.Role - when iter.Value.TryParseScalar(&KnownRoleMetadata.TryParse, out bool isReplica): + when 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 Log?.LogInformationAutoConfiguredHelloRole( new(server), isReplica ? "replica" : "primary"); diff --git a/src/StackExchange.Redis/ServerEndPoint.cs b/src/StackExchange.Redis/ServerEndPoint.cs index ebf76262b..833850219 100644 --- a/src/StackExchange.Redis/ServerEndPoint.cs +++ b/src/StackExchange.Redis/ServerEndPoint.cs @@ -388,7 +388,23 @@ internal void AddScript(string script, byte[] hash) } } - internal async Task AutoConfigureAsync(PhysicalConnection? connection, ILogger? log = null, CommandFlags extraFlags = CommandFlags.None) + /// + /// Whether HELLO has told us our replication role (and server mode) on the current connection. + /// + /// Reset at the start of each handshake, and set when the HELLO reply is processed. + internal bool RoleKnownFromHello { get; set; } + + /// + /// Issues the topology/configuration discovery commands for this server. + /// + /// The connection to write to; null for an already-established connection. + /// The log to write handshake details to. + /// Additional flags to apply to the messages issued. + /// + /// Whether a HELLO has been written to this same batch, but not yet answered; in that case + /// we expect to learn our role from it, so we can skip the key-based fallback probe. + /// + internal async Task AutoConfigureAsync(PhysicalConnection? connection, ILogger? log = null, CommandFlags extraFlags = CommandFlags.None, bool helloPending = false) { if (!serverType.SupportsAutoConfigure()) { @@ -452,9 +468,11 @@ internal async Task AutoConfigureAsync(PhysicalConnection? connection, ILogger? await WriteDirectOrQueueFireAndForgetAsync(connection, msg, autoConfigProcessor).ForAwait(); } } - else if (commandMap.IsAvailable(RedisCommand.SET)) + else if (commandMap.IsAvailable(RedisCommand.SET) && !(helloPending || RoleKnownFromHello)) { // 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 + // unlike this probe it doesn't need a key - which matters when ACLs restrict key patterns; see #2968) RedisKey key = Multiplexer.UniqueId; // The actual value here doesn't matter (we detect the error code if it fails). // The value here is to at least give some indication to anyone watching via "monitor", @@ -1002,10 +1020,24 @@ private async Task HandshakeAsync(PhysicalConnection connection, ILogger? log) // the various tasks and just `return connection.FlushAsync();` - however, since handshake is low // volume, we can afford to optimize for a good stack-trace rather than avoiding state machines. ResultProcessor? autoConfig = null; - if (Multiplexer.RawConfig.TryResp3()) // note this includes an availability check on HELLO + bool isInteractive = connection.BridgeCouldBeNull?.ConnectionType == ConnectionType.Interactive; + if (isInteractive) + { + // forget what the previous connection's HELLO told us; re-established below, if this one repeats it + // (the subscription handshake is deliberately left out of this: it doesn't do the discovery step) + RoleKnownFromHello = false; + } + + // HELLO comes in two flavours: as the RESP3 negotiation (which has to be the *first* command, and has to + // carry the credentials), or - when we're staying on RESP2 - purely for discovery, in which case it is + // issued *after* AUTH, below; see #2968 for why we want it even on RESP2 + bool helloAvailable = Multiplexer.RawConfig.TryHello(out int helloProtocol); // includes an availability check on HELLO + bool negotiateResp3 = helloAvailable && helloProtocol >= 3; + bool discoveryHello = helloAvailable && !negotiateResp3 && isInteractive; + if (negotiateResp3) { log?.LogInformationAuthenticatingViaHello(new(this)); - var hello = Message.CreateHello(3, user, password, clientName, CommandFlags.FireAndForget | Message.NoFlushFlag); + var hello = Message.CreateHello(helloProtocol, user, password, clientName, CommandFlags.FireAndForget | Message.NoFlushFlag); hello.SetInternalCall(); await WriteDirectOrQueueFireAndForgetAsync(connection, hello, autoConfig ??= ResultProcessor.AutoConfigureProcessor.Create(log)).ForAwait(); @@ -1014,7 +1046,7 @@ private async Task HandshakeAsync(PhysicalConnection connection, ILogger? log) } else { - // if we're not even issuing HELLO, we're RESP2 + // whether or not we issue HELLO for discovery below, we're RESP2 connection.SetProtocol(RedisProtocol.Resp2); } @@ -1082,7 +1114,19 @@ private async Task HandshakeAsync(PhysicalConnection connection, ILogger? log) var connType = bridge.ConnectionType; if (connType == ConnectionType.Interactive) { - await AutoConfigureAsync(connection, log, extraFlags: Message.NoFlushFlag).ForAwait(); + if (discoveryHello) + { + // a bare HELLO (no AUTH clause of its own): we're already authenticated by this point, and + // folding the credentials in here would change how credential failures surface. We only want + // the reply, which reports version/role/mode/connection-id without INFO or CONFIG (both + // @dangerous, hence often ACL-restricted); see #2968 + log?.LogInformationDiscoveryViaHello(new(this)); + var hello = Message.CreateHello(helloProtocol, null, null, null, CommandFlags.FireAndForget | Message.NoFlushFlag); + hello.SetInternalCall(); + await WriteDirectOrQueueFireAndForgetAsync(connection, hello, autoConfig ??= ResultProcessor.AutoConfigureProcessor.Create(log)).ForAwait(); + } + + await AutoConfigureAsync(connection, log, extraFlags: Message.NoFlushFlag, helloPending: negotiateResp3 || discoveryHello).ForAwait(); } // note that the final messages *are* flushed (no Message.NoFlushFlag) diff --git a/tests/RedisConfigs/.docker/Envoy/Dockerfile b/tests/RedisConfigs/.docker/Envoy/Dockerfile index 5c20d350c..c3a117e9d 100644 --- a/tests/RedisConfigs/.docker/Envoy/Dockerfile +++ b/tests/RedisConfigs/.docker/Envoy/Dockerfile @@ -1,4 +1,4 @@ -FROM envoyproxy/envoy:v1.31-latest +FROM envoyproxy/envoy:v1.39-latest COPY envoy.yaml /etc/envoy/envoy.yaml RUN chmod go+r /etc/envoy/envoy.yaml diff --git a/tests/StackExchange.Redis.Tests/CommandMapUnitTests.cs b/tests/StackExchange.Redis.Tests/CommandMapUnitTests.cs index b3c9a7cfd..175c8d395 100644 --- a/tests/StackExchange.Redis.Tests/CommandMapUnitTests.cs +++ b/tests/StackExchange.Redis.Tests/CommandMapUnitTests.cs @@ -63,4 +63,17 @@ public void TryParseCI_RejectsNonCommands(string name) ReadOnlySpan bytes = Encoding.ASCII.GetBytes(name); Assert.False(RedisCommandMetadata.TryParseCI(bytes, out _), $"byte parse unexpectedly succeeded for '{name}'"); } + + /// + /// We now issue HELLO whenever it is available (RESP2 included), so the proxy maps must exclude it: + /// twemproxy 0.5.0 *closes the connection* on an unsupported command, and envoy (1.39, at least) forwards + /// HELLO to an arbitrary backend node, so its version/role/mode describe the wrong server. + /// + [Fact] + public void ProxyCommandMapsExcludeHello() + { + Assert.False(CommandMap.Twemproxy.IsAvailable(RedisCommand.HELLO), "twemproxy"); + Assert.False(CommandMap.Envoyproxy.IsAvailable(RedisCommand.HELLO), "envoyproxy"); + Assert.True(CommandMap.Default.IsAvailable(RedisCommand.HELLO), "default"); + } } diff --git a/tests/StackExchange.Redis.Tests/HelloHandshakeTests.cs b/tests/StackExchange.Redis.Tests/HelloHandshakeTests.cs new file mode 100644 index 000000000..1b929802c --- /dev/null +++ b/tests/StackExchange.Redis.Tests/HelloHandshakeTests.cs @@ -0,0 +1,133 @@ +using System; +using System.Collections.Concurrent; +using System.Collections.Generic; +using System.Linq; +using System.Threading.Tasks; +using StackExchange.Redis.Server; +using Xunit; + +namespace StackExchange.Redis.Tests; + +/// +/// We issue HELLO whenever we expect the server to understand it, even when staying on RESP2: +/// the reply tells us the version/role/mode without needing INFO or CONFIG, both of +/// which are @dangerous and commonly restricted by ACLs; see issue #2968. +/// +public class HelloHandshakeTests(ITestOutputHelper log) +{ + [Theory] + [InlineData(RedisProtocol.Resp2, "2")] + [InlineData(RedisProtocol.Resp3, "3")] + public async Task HelloIsIssuedForBothProtocols(RedisProtocol protocol, string expectedProtover) + { + using var server = new RecordingServer(log); + var config = server.GetClientConfig(); + config.Protocol = protocol; + + await using var conn = await ConnectionMultiplexer.ConnectAsync(config); + + var hellos = server.Recorded("HELLO"); + Assert.NotEmpty(hellos); + Assert.All(hellos, args => Assert.Equal(expectedProtover, args.FirstOrDefault())); + Assert.Equal(protocol, conn.GetServerSnapshot()[0].Protocol); + } + + [Fact] + public async Task NoHelloWhenDisabledViaCommandMap() + { + using var server = new RecordingServer(log); + var config = server.GetClientConfig(); + config.Protocol = RedisProtocol.Resp2; + config.CommandMap = server.CreateCommandMap(except: "HELLO"); + + await using var conn = await ConnectionMultiplexer.ConnectAsync(config); + + Assert.Empty(server.Recorded("HELLO")); + Assert.Equal(RedisProtocol.Resp2, conn.GetServerSnapshot()[0].Protocol); + } + + [Fact] + public async Task NoHelloWhenServerIsAssumedDownLevel() + { + using var server = new RecordingServer(log); + var config = server.GetClientConfig(); + config.Protocol = RedisProtocol.Resp2; + config.DefaultVersion = new Version(5, 0, 0); // HELLO arrived in 6.0 + + await using var conn = await ConnectionMultiplexer.ConnectAsync(config); + + Assert.Empty(server.Recorded("HELLO")); + Assert.Equal(RedisProtocol.Resp2, conn.GetServerSnapshot()[0].Protocol); + } + + /// + /// The point of issue #2968: with INFO and CONFIG unavailable we used to fall back to + /// SET {random-guid} replica-read-only PX 1 NX to detect a read-only replica - a key write that + /// cannot be allow-listed in an ACL. HELLO reports the role, so the probe isn't needed. + /// + [Theory] + [InlineData(RedisProtocol.Resp2)] + [InlineData(RedisProtocol.Resp3)] + public async Task NoReplicaProbeWhenHelloTellsUsTheRole(RedisProtocol protocol) + { + using var server = new RecordingServer(log); + var config = server.GetClientConfig(); + config.Protocol = protocol; + config.CommandMap = server.CreateCommandMap(except: ["INFO", "CONFIG"]); + + await using var conn = await ConnectionMultiplexer.ConnectAsync(config); + + Assert.NotEmpty(server.Recorded("HELLO")); + Assert.Empty(server.Recorded("SET")); + Assert.False(conn.GetServerSnapshot()[0].IsReplica); // from HELLO's "role" + } + + /// + /// The converse of : when HELLO isn't available + /// either, the key-based probe is still the only signal we have, so it must still be issued. + /// + [Fact] + public async Task ReplicaProbeStillUsedWhenHelloUnavailable() + { + using var server = new RecordingServer(log); + var config = server.GetClientConfig(); + config.Protocol = RedisProtocol.Resp2; + config.CommandMap = server.CreateCommandMap(except: ["INFO", "CONFIG", "HELLO"]); + + await using var conn = await ConnectionMultiplexer.ConnectAsync(config); + + Assert.Empty(server.Recorded("HELLO")); + var sets = server.Recorded("SET"); + Assert.NotEmpty(sets); + Assert.All(sets, args => Assert.Equal("replica-read-only", args.Skip(1).FirstOrDefault())); + } + + private sealed class RecordingServer(ITestOutputHelper log) : InProcessTestServer(log) + { + private readonly ConcurrentQueue<(string Command, string[] Args)> _commands = new(); + + public override TypedRedisValue Execute(RedisClient client, in RedisRequest request) + { + var args = new string[Math.Max(request.Count - 1, 0)]; + for (int i = 0; i < args.Length; i++) + { + args[i] = request.GetString(i + 1); + } + _commands.Enqueue((request.GetString(0).ToUpperInvariant(), args)); + return base.Execute(client, in request); + } + + public List Recorded(string command) + => _commands.Where(x => x.Command == command).Select(x => x.Args).ToList(); + + public CommandMap CreateCommandMap(params string[] except) + { + var commands = GetCommands(); + foreach (var command in except) + { + commands.Remove(command); + } + return CommandMap.Create(commands); + } + } +} diff --git a/tests/StackExchange.Redis.Tests/Resp3HandshakeTests.cs b/tests/StackExchange.Redis.Tests/Resp3HandshakeTests.cs index 167f8996d..5a8c72979 100644 --- a/tests/StackExchange.Redis.Tests/Resp3HandshakeTests.cs +++ b/tests/StackExchange.Redis.Tests/Resp3HandshakeTests.cs @@ -38,17 +38,12 @@ public static IEnumerable GetHandshakeParameters() { foreach (var server in servers) { - if (client is RedisProtocol.Resp2 & server is not ServerResponse.Resp2) + // note that every combination is meaningful: we issue HELLO even for RESP2 clients + // (as HELLO 2), so "server doesn't understand HELLO" etc still needs exercising + int count = 1 << HandshakeFlagsCount; + for (int i = 0; i < count; i++) { - // we don't issue HELLO for this, nothing to test - } - else - { - int count = 1 << HandshakeFlagsCount; - for (int i = 0; i < count; i++) - { - yield return [client, server, (HandshakeFlags)i]; - } + yield return [client, server, (HandshakeFlags)i]; } } } @@ -67,6 +62,11 @@ public async Task Handshake(RedisProtocol client, ServerResponse server, Handsha await using var clientObj = await ConnectionMultiplexer.ConnectAsync(config); + // RESP3 requires both sides to want it; anything else lands on RESP2 + var expectedProtocol = client is RedisProtocol.Resp3 && server is ServerResponse.Resp3 + ? RedisProtocol.Resp3 : RedisProtocol.Resp2; + Assert.Equal(expectedProtocol, clientObj.GetServerSnapshot()[0].Protocol); + var sub = clientObj.GetSubscriber(); var db = clientObj.GetDatabase(); ConcurrentBag received = []; From 5c1972a0a871a5e76ff05c18b57099213f24c6e1 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Fri, 14 Aug 2026 13:11:10 +0100 Subject: [PATCH 2/2] Drop the discovery-HELLO logger message EventId 110 is in use in another in-progress PR, and this doesn't warrant a logger message of its own - the handshake HELLO is already visible in the detail/parse logs. --- src/StackExchange.Redis/LoggerExtensions.cs | 6 ------ src/StackExchange.Redis/ServerEndPoint.cs | 1 - 2 files changed, 7 deletions(-) diff --git a/src/StackExchange.Redis/LoggerExtensions.cs b/src/StackExchange.Redis/LoggerExtensions.cs index 6afa716ec..381ee4426 100644 --- a/src/StackExchange.Redis/LoggerExtensions.cs +++ b/src/StackExchange.Redis/LoggerExtensions.cs @@ -709,10 +709,4 @@ internal static void LogWithThreadPoolStats(this ILogger? log, string message) EventId = 109, Message = "Service name not defined.")] internal static partial void LogInformationServiceNameNotDefined(this ILogger logger); - - [LoggerMessage( - Level = LogLevel.Information, - EventId = 110, - Message = "{Server}: Requesting server details via HELLO")] - internal static partial void LogInformationDiscoveryViaHello(this ILogger logger, ServerEndPointLogValue server); } diff --git a/src/StackExchange.Redis/ServerEndPoint.cs b/src/StackExchange.Redis/ServerEndPoint.cs index 833850219..ee5cd3281 100644 --- a/src/StackExchange.Redis/ServerEndPoint.cs +++ b/src/StackExchange.Redis/ServerEndPoint.cs @@ -1120,7 +1120,6 @@ private async Task HandshakeAsync(PhysicalConnection connection, ILogger? log) // folding the credentials in here would change how credential failures surface. We only want // the reply, which reports version/role/mode/connection-id without INFO or CONFIG (both // @dangerous, hence often ACL-restricted); see #2968 - log?.LogInformationDiscoveryViaHello(new(this)); var hello = Message.CreateHello(helloProtocol, null, null, null, CommandFlags.FireAndForget | Message.NoFlushFlag); hello.SetInternalCall(); await WriteDirectOrQueueFireAndForgetAsync(connection, hello, autoConfig ??= ResultProcessor.AutoConfigureProcessor.Create(log)).ForAwait();