Sentinel soft-fail follow-ups: discovery failure as RedisConnectionException, late sentinel init, clear LastException

docs/Configuration.md

@@ -36,6 +36,11 @@ pointing to that primary server that will automatically be updated if the primar
 var conn = ConnectionMultiplexer.Connect("localhost,serviceName=myprimary");
 ```
 
+`abortConnect` applies to sentinel connections the same way it applies to everything else. With `abortConnect=true` (the default), `Connect`
+throws if the sentinels cannot be reached or do not know the service. With `abortConnect=false`, `Connect` returns a multiplexer even then:
+it is not connected, operations fail with a connection exception that names the problem, and it connects itself to the primary once the
+sentinels become reachable and name one. If you want startup to fail when the sentinels are unavailable, keep `abortConnect=true`.
+
 An overview of mapping between the `string` and `ConfigurationOptions` representation is shown below, but you can switch between them trivially:
 
 ```csharp

src/StackExchange.Redis/ConnectionMultiplexer.Sentinel.cs

@@ -15,14 +15,21 @@ public partial class ConnectionMultiplexer
     internal Timer? sentinelPrimaryReconnectTimer;
     internal readonly Dictionary<string, ConnectionMultiplexer> sentinelConnectionChildren = new();
     internal ConnectionMultiplexer? sentinelConnection;
+    private int _sentinelInitialized;
 
     /// <summary>
     /// Initializes the connection as a Sentinel connection and adds the necessary event handlers to track changes to the managed primaries.
     /// </summary>
+    /// <remarks>
+    /// Runs once. It is reached from the connect paths, and also from the reconfigure loop when a connection
+    /// that soft-failed its initial connect (<see cref="ConfigurationOptions.AbortOnConnectFail"/> is <c>false</c>) later
+    /// reaches a sentinel: without that, a connection that started while the sentinels were down would never subscribe
+    /// to <c>+switch-master</c>, and would track failovers only through the fallback polling.
+    /// </remarks>
     /// <param name="log">The <see cref="ILogger"/> to log to, if any.</param>
     internal void InitializeSentinel(ILogger? log)
     {
-        if (!_isSentinel)
+        if (!_isSentinel || Interlocked.Exchange(ref _sentinelInitialized, 1) != 0)
         {
             return;
         }
@@ -253,8 +260,7 @@ public ConnectionMultiplexer GetSentinelMasterConnection(ConfigurationOptions co
                     }
 
                     if (connection is not null) try { connection.Dispose(); } catch { }
-                    endpoints = config.EndPoints.Clone();
-                    connection = ConnectImpl(config, log, endpoints: endpoints);
+                    connection = SeedConnection(failure);
                     break;
                 }
 
@@ -330,11 +336,7 @@ public ConnectionMultiplexer GetSentinelMasterConnection(ConfigurationOptions co
                     throw failure;
                 }
 
-                if (connection is null)
-                {
-                    endpoints = config.EndPoints.Clone();
-                    connection = ConnectImpl(config, log, endpoints: endpoints);
-                }
+                connection ??= SeedConnection(failure);
 
                 connection.LastException = failure;
                 connection.Logger?.LogErrorSyncConnectTimeout(failure, failure.Message);
@@ -385,6 +387,20 @@ public ConnectionMultiplexer GetSentinelMasterConnection(ConfigurationOptions co
             }
             throw;
         }
+
+        // AbortOnConnectFail=false with no primary discovered: hand back a multiplexer seeded with whatever endpoints
+        // the caller gave us (the sentinel addresses, via Connect), for SwitchPrimary to rebuild once discovery works.
+        // A config with no endpoints of its own has nothing to seed with, and that is the discovery failure the caller
+        // should see - not the "No nodes to consider" that ConnectImpl would throw at an empty endpoint list.
+        ConnectionMultiplexer SeedConnection(RedisConnectionException discoveryFailure)
+        {
+            if (config.EndPoints.Count == 0)
+            {
+                throw discoveryFailure;
+            }
+            endpoints = config.EndPoints.Clone();
+            return ConnectImpl(config, log, endpoints: endpoints);
+        }
     }
 
     [System.Diagnostics.CodeAnalysis.SuppressMessage("Roslynator", "RCS1075:Avoid empty catch clause that catches System.Exception.", Justification = "We don't care.")]
@@ -423,6 +439,11 @@ internal void OnManagedConnectionRestored(object? sender, ConnectionFailedEventA
                 // we are not aware of, so retry
                 SwitchPrimary(e.EndPoint, connection);
             }
+
+            // A soft-failed connect (AbortOnConnectFail=false) records the discovery failure as LastException, and
+            // the exception factory folds that into every "no connection available" message. We are connected to a
+            // sentinel-approved primary again, so stop reporting it.
+            connection.LastException = null;
         }
         catch (Exception)
         {

src/StackExchange.Redis/ConnectionMultiplexer.cs

@@ -2038,6 +2038,13 @@ internal async Task<bool> ReconfigureAsync(bool first, bool reconfigureAll, ILog
                         else if (standaloneCount == 0 && sentinelCount > 0)
                         {
                             ServerSelectionStrategy.ServerType = ServerType.Sentinel;
+                            if (!first)
+                            {
+                                // A sentinel connection whose initial connect soft-failed (AbortOnConnectFail=false) is
+                                // only now reaching a sentinel; the connect path that would have wired up the sentinel
+                                // handlers has long since returned. (The first connect leaves that to ConnectImpl.)
+                                InitializeSentinel(log);
+                            }
                         }
                         else if (standaloneCount > 0)
                         {

tests/StackExchange.Redis.Tests/SentinelConfigTests.cs

@@ -93,6 +93,27 @@ public void Connect_UnreachableSentinel_AbortEnabled_Throws()
         Assert.Throws<RedisConnectionException>(() => ConnectionMultiplexer.Connect(options));
     }
 
+    [Fact]
+    public void GetSentinelMasterConnection_NothingToSeedWith_AbortDisabled_ThrowsTheDiscoveryFailure()
+    {
+        // the direct API: a long-lived SentinelConnect, then a per-service config that carries only the service name
+        // (the data endpoints are sentinel's to supply). With nothing discovered and nothing to seed a multiplexer
+        // with, the caller should see the discovery failure as a RedisConnectionException - not the
+        // InvalidOperationException("No nodes to consider") that connecting an empty endpoint list produces.
+        using var sentinel = ConnectionMultiplexer.SentinelConnect(GetUnreachableSentinelOptions(abortOnConnectFail: false));
+        var config = new ConfigurationOptions
+        {
+            AbortOnConnectFail = false,
+            ConnectRetry = 0,
+            ConnectTimeout = 100,
+            ServiceName = "unreachable-primary",
+        };
+
+        var exception = Assert.Throws<RedisConnectionException>(() => sentinel.GetSentinelMasterConnection(config));
+        Assert.Equal(ConnectionFailureType.UnableToConnect, exception.FailureType);
+        Assert.Equal("Sentinel: Failed connecting to configured primary for service: unreachable-primary", exception.Message);
+    }
+
     private static ConfigurationOptions GetUnreachableSentinelOptions(bool abortOnConnectFail)
     {
         var options = new ConfigurationOptions

tests/StackExchange.Redis.Tests/SentinelTests.cs

@@ -565,6 +565,18 @@ await UntilConditionAsync(
             () => !conn.GetServerSnapshot().ToArray().Any(x => Equals(x.EndPoint, seedEndPoint)));
         Assert.DoesNotContain(seedEndPoint, conn.GetEndPoints());
 
+        // the sentinel connection behind it soft-failed too, so the connect path never wired up its handlers; it
+        // must do that itself on reaching a sentinel, or this connection would never hear +switch-master
+        var sentinelConn = conn.sentinelConnection!;
+        var switchMaster = RedisChannel.Literal("+switch-master");
+        await UntilConditionAsync(
+            TimeSpan.FromSeconds(10),
+            () => sentinelConn.GetSubscriber().SubscribedEndpoint(switchMaster) is not null && conn.LastException is null);
+        Assert.NotNull(sentinelConn.GetSubscriber().SubscribedEndpoint(switchMaster));
+
+        // and the discovery failure recorded at connect time is no longer reported now that we have a primary
+        Assert.Null(conn.LastException);
+
         static bool TryPing(IDatabase db)
         {
             try