Fix failover parser-state handling and add targeted regression tests

Author: paulmedynski

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs

@@ -3635,7 +3635,15 @@ private void LoginWithFailover(
                         continue;
                     }
 
-                    if (IsDoNotRetryConnectError(sqlex) || timeout.IsExpired)
+                    // If state != closed, indicates that the parser encountered an error while
+                    // processing the login response (e.g. an explicit error token). Transient
+                    // network errors that impact connectivity will result in parser state being
+                    // closed. Only network-level errors should trigger failover alternation;
+                    // login-phase errors (like transient errors) should be thrown so they can
+                    // be handled by the outer ConnectRetryCount loop.
+                    if (_parser?.State is not TdsParserState.Closed ||
+                        IsDoNotRetryConnectError(sqlex) ||
+                        timeout.IsExpired)
                     {
                         // No more time to try again.
                         // Caller will call LoginFailure()

src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionFailoverTests.cs

@@ -4,6 +4,7 @@
 
 using System;
 using System.Data;
+using System.Threading.Tasks;
 using Microsoft.Data.SqlClient.Connection;
 using Microsoft.Data.SqlClient.Tests.Common;
 using Microsoft.SqlServer.TDS.Servers;
@@ -172,7 +173,7 @@ public void NetworkTimeout_ShouldFail()
                 InitialCatalog = "master",// Required for failover partner to work
                 ConnectTimeout = 1,
                 ConnectRetryInterval = 1,
-                ConnectRetryCount = 0, // Disable retry 
+                ConnectRetryCount = 0, // Disable retry
                 Encrypt = false,
                 MultiSubnetFailover = false,
 #if NETFRAMEWORK
@@ -336,6 +337,10 @@ public void NetworkError_WithUserProvidedPartner_RetryEnabled_ShouldConnectToFai
             Assert.Equal(1, failoverServer.PreLoginCount - failoverServer.AbandonedPreLoginCount);
         }
 
+        /// <summary>
+        /// Verifies login-phase transient SQL errors are retried on the primary endpoint and
+        /// do not trigger failover-partner alternation.
+        /// </summary>
         [Theory]
         [InlineData(40613)]
         [InlineData(42108)]
@@ -372,6 +377,8 @@ public void TransientFault_ShouldConnectToPrimary(uint errorCode)
             using SqlConnection connection = new(builder.ConnectionString);
 
             // Act
+            // First login receives the transient token; outer connect retry opens a fresh parser
+            // and retries against the same primary endpoint.
             connection.Open();
 
             // Assert
@@ -380,6 +387,8 @@ public void TransientFault_ShouldConnectToPrimary(uint errorCode)
 
             // Failures should prompt the client to return to the original server, resulting in a login count of 2
             Assert.Equal(2, server.PreLoginCount - server.AbandonedPreLoginCount);
+            // The fix: login-phase errors must NOT trigger failover alternation
+            Assert.Equal(0, failoverServer.PreLoginCount);
         }
 
         [Theory]
@@ -430,6 +439,10 @@ public void TransientFault_RetryDisabled_ShouldFail(uint errorCode)
             Assert.Fail();
         }
 
+        /// <summary>
+        /// Verifies user-provided failover partner does not change behavior for login-phase
+        /// transient SQL errors; retries stay on primary.
+        /// </summary>
         [Theory]
         [InlineData(40613)]
         [InlineData(42108)]
@@ -467,6 +480,8 @@ public void TransientFault_WithUserProvidedPartner_ShouldConnectToPrimary(uint e
             using SqlConnection connection = new(builder.ConnectionString);
 
             // Act
+            // Even with a configured partner, this path should use outer connect retry
+            // against primary rather than alternation inside LoginWithFailover.
             connection.Open();
 
             // Assert
@@ -475,6 +490,8 @@ public void TransientFault_WithUserProvidedPartner_ShouldConnectToPrimary(uint e
 
             // Failures should prompt the client to return to the original server, resulting in a login count of 2
             Assert.Equal(2, server.PreLoginCount - server.AbandonedPreLoginCount);
+            // The fix: login-phase errors must NOT trigger failover alternation
+            Assert.Equal(0, failoverServer.PreLoginCount);
         }
 
         [Theory]
@@ -591,5 +608,265 @@ public void TransientFault_IgnoreServerProvidedFailoverPartner_ShouldConnectToUs
             // 1 for the failover connection
             Assert.Equal(1, failoverServer.PreLoginCount - failoverServer.AbandonedPreLoginCount);
         }
+
+        /// <summary>
+        /// Async parity for primary-only retry behavior on login-phase transient SQL errors.
+        /// </summary>
+        [Theory]
+        [InlineData(40613)]
+        [InlineData(42108)]
+        [InlineData(42109)]
+        public async Task TransientFault_Async_ShouldConnectToPrimary_NotFailover(uint errorCode)
+        {
+            // Async parity for TransientFault_ShouldConnectToPrimary.
+            // A transient login-token error must be retried against the primary;
+            // the failover partner must never be contacted.
+
+            using TdsServer failoverServer = new(
+                new TdsServerArguments
+                {
+                    FailoverPartner = "localhost,1234",
+                });
+            failoverServer.Start();
+
+            using TransientTdsErrorTdsServer server = new(
+                new TransientTdsErrorTdsServerArguments()
+                {
+                    IsEnabledTransientError = true,
+                    Number = errorCode,
+                    FailoverPartner = $"localhost,{failoverServer.EndPoint.Port}",
+                });
+            server.Start();
+
+            SqlConnectionStringBuilder builder = new()
+            {
+                DataSource = $"localhost,{server.EndPoint.Port}",
+                InitialCatalog = "master",
+                ConnectTimeout = 30,
+                ConnectRetryInterval = 1,
+                Encrypt = false,
+                Pooling = false,
+            };
+            using SqlConnection connection = new(builder.ConnectionString);
+
+            // Asserts async open follows the same retry and failover-selection rules as sync.
+            await connection.OpenAsync();
+
+            Assert.Equal(ConnectionState.Open, connection.State);
+            Assert.Equal($"localhost,{server.EndPoint.Port}", connection.DataSource);
+            Assert.Equal(2, server.PreLoginCount - server.AbandonedPreLoginCount);
+            // The fix: login-phase errors must NOT trigger failover alternation
+            Assert.Equal(0, failoverServer.PreLoginCount);
+        }
+
+        /// <summary>
+        /// Async parity with user-provided partner: login-phase transient SQL errors should
+        /// still retry on primary without failover alternation.
+        /// </summary>
+        [Theory]
+        [InlineData(40613)]
+        [InlineData(42108)]
+        [InlineData(42109)]
+        public async Task TransientFault_WithUserProvidedPartner_Async_ShouldConnectToPrimary_NotFailover(uint errorCode)
+        {
+            // Async parity for TransientFault_WithUserProvidedPartner_ShouldConnectToPrimary.
+            // Even with a user-provided failover partner, a login-token error must not
+            // cause alternation to the failover server.
+
+            using TdsServer failoverServer = new(
+                new TdsServerArguments
+                {
+                    FailoverPartner = "localhost:1234",
+                });
+            failoverServer.Start();
+
+            using TransientTdsErrorTdsServer server = new(
+                new TransientTdsErrorTdsServerArguments()
+                {
+                    IsEnabledTransientError = true,
+                    Number = errorCode,
+                    FailoverPartner = $"localhost:{failoverServer.EndPoint.Port}",
+                });
+            server.Start();
+
+            SqlConnectionStringBuilder builder = new()
+            {
+                DataSource = $"localhost,{server.EndPoint.Port}",
+                InitialCatalog = "master",
+                ConnectTimeout = 30,
+                ConnectRetryInterval = 1,
+                Encrypt = false,
+                FailoverPartner = $"localhost:{failoverServer.EndPoint.Port}",
+            };
+            using SqlConnection connection = new(builder.ConnectionString);
+
+            // Asserts async open with explicit partner still avoids failover alternation.
+            await connection.OpenAsync();
+
+            Assert.Equal(ConnectionState.Open, connection.State);
+            Assert.Equal($"localhost,{server.EndPoint.Port}", connection.DataSource);
+            Assert.Equal(2, server.PreLoginCount - server.AbandonedPreLoginCount);
+            // The fix: login-phase errors must NOT trigger failover alternation
+            Assert.Equal(0, failoverServer.PreLoginCount);
+        }
+
+        /// <summary>
+        /// Verifies pooled connections are not cleared and failover is not attempted when a
+        /// login-phase transient SQL error occurs with a user-provided failover partner.
+        /// </summary>
+        [Theory]
+        [InlineData(40613)]
+        [InlineData(42108)]
+        [InlineData(42109)]
+        public void TransientFault_WithUserProvidedPartner_Pooling_ShouldNotClearPool_NotFailover(uint errorCode)
+        {
+            // With pooling enabled and a user-provided failover partner, a transient
+            // login-token error must not clear the pool and must not contact the failover server.
+
+            using TdsServer failoverServer = new(
+                new TdsServerArguments
+                {
+                    FailoverPartner = "localhost,1234",
+                });
+            failoverServer.Start();
+
+            // Start with errors disabled so the pool warms up successfully.
+            using TransientTdsErrorTdsServer server = new(
+                new TransientTdsErrorTdsServerArguments()
+                {
+                    FailoverPartner = $"localhost,{failoverServer.EndPoint.Port}",
+                });
+            server.Start();
+
+            SqlConnectionStringBuilder builder = new()
+            {
+                DataSource = $"localhost,{server.EndPoint.Port}",
+                InitialCatalog = "master",
+                ConnectTimeout = 30,
+                ConnectRetryInterval = 1,
+                Encrypt = SqlConnectionEncryptOption.Optional,
+                FailoverPartner = $"localhost,{failoverServer.EndPoint.Port}",
+                Pooling = true,
+            };
+
+            // Warm up the pool.
+            using SqlConnection warmup = new(builder.ConnectionString);
+            warmup.Open();
+            warmup.Close();
+
+            // Enable the transient error for the next login attempt.
+            server.SetErrorBehavior(true, errorCode);
+
+            // ConnectRetryCount > 0 (default 1) so the client retries and succeeds.
+            using SqlConnection connection = new(builder.ConnectionString);
+            connection.Open();
+
+            Assert.Equal(ConnectionState.Open, connection.State);
+            Assert.Equal($"localhost,{server.EndPoint.Port}", connection.DataSource);
+            // Failover server must never have been contacted.
+            Assert.Equal(0, failoverServer.PreLoginCount);
+        }
+
+        /// <summary>
+        /// Verifies ConnectRetryCount=0 propagates login-phase transient SQL errors immediately
+        /// and never attempts failover alternation.
+        /// </summary>
+        [Theory]
+        [InlineData(40613)]
+        [InlineData(42108)]
+        [InlineData(42109)]
+        public void TransientFault_RetryDisabled_WithUserProvidedPartner_ShouldFail_NotFailover(uint errorCode)
+        {
+            // When ConnectRetryCount = 0 and the server returns a login-phase token error,
+            // the exception must propagate immediately and the failover partner must not be
+            // contacted (parser state is not Closed, so the new guard must kick in).
+
+            using TdsServer failoverServer = new(
+                new TdsServerArguments
+                {
+                    FailoverPartner = "localhost:1234",
+                });
+            failoverServer.Start();
+
+            using TransientTdsErrorTdsServer server = new(
+                new TransientTdsErrorTdsServerArguments()
+                {
+                    IsEnabledTransientError = true,
+                    Number = errorCode,
+                    FailoverPartner = $"localhost:{failoverServer.EndPoint.Port}",
+                });
+            server.Start();
+
+            SqlConnectionStringBuilder builder = new()
+            {
+                DataSource = $"localhost,{server.EndPoint.Port}",
+                InitialCatalog = "master",
+                ConnectTimeout = 30,
+                ConnectRetryInterval = 1,
+                ConnectRetryCount = 0,
+                Encrypt = false,
+                FailoverPartner = $"localhost:{failoverServer.EndPoint.Port}",
+            };
+            using SqlConnection connection = new(builder.ConnectionString);
+
+            // No outer connect retry is allowed, so the first transient error should surface.
+            SqlException ex = Assert.Throws<SqlException>(() => connection.Open());
+
+            Assert.Equal((int)errorCode, ex.Number);
+            Assert.Equal(ConnectionState.Closed, connection.State);
+            // The parser was not closed (login-phase error), so the failover alternation branch
+            // must not have been entered.
+            Assert.Equal(0, failoverServer.PreLoginCount);
+        }
+
+        /// <summary>
+        /// Isolates the parser-state guard by using a non-fatal login error token: without the
+        /// guard, LoginWithFailover alternates to partner; with the guard, retry stays on primary.
+        /// </summary>
+        [Fact]
+        public void NonFatalTransientLoginError_WithUserProvidedPartner_ShouldRetryPrimary_NotFailover()
+        {
+            // This test isolates the parser-state guard added to LoginWithFailover.
+            // We emit a transient login error with non-fatal severity so the connection
+            // is not automatically doomed/broken by existing breakConnection logic.
+
+            using TdsServer failoverServer = new(
+                new TdsServerArguments
+                {
+                    FailoverPartner = "localhost,1234",
+                });
+            failoverServer.Start();
+
+            using TransientTdsErrorTdsServer server = new(
+                new TransientTdsErrorTdsServerArguments()
+                {
+                    IsEnabledTransientError = true,
+                    Number = 40613,
+                    // Use non-fatal severity so break/doom logic does not short-circuit the path.
+                    ErrorClass = 16,
+                    RepeatCount = 1,
+                    FailoverPartner = $"localhost,{failoverServer.EndPoint.Port}",
+                });
+            server.Start();
+
+            SqlConnectionStringBuilder builder = new()
+            {
+                DataSource = $"localhost,{server.EndPoint.Port}",
+                InitialCatalog = "master",
+                ConnectTimeout = 30,
+                ConnectRetryInterval = 1,
+                Encrypt = false,
+                Pooling = false,
+                FailoverPartner = $"localhost,{failoverServer.EndPoint.Port}",
+            };
+
+            using SqlConnection connection = new(builder.ConnectionString);
+            connection.Open();
+
+            Assert.Equal(ConnectionState.Open, connection.State);
+            Assert.Equal($"localhost,{server.EndPoint.Port}", connection.DataSource);
+            Assert.Equal(2, server.PreLoginCount - server.AbandonedPreLoginCount);
+            Assert.Equal(0, failoverServer.PreLoginCount - failoverServer.AbandonedPreLoginCount);
+        }
     }
 }

src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.Servers/TransientTdsErrorTdsServer.cs

@@ -89,7 +89,7 @@ private TDSMessageCollection GenerateErrorMessage(TDSMessage request)
             TDSUtilities.Log(Arguments.Log, "Request", request);
 
             // Prepare ERROR token with the denial details
-            TDSErrorToken errorToken = new TDSErrorToken(errorNumber, 1, 20, errorMessage);
+            TDSErrorToken errorToken = new TDSErrorToken(errorNumber, 1, Arguments.ErrorClass, errorMessage);
 
             // Log response
             TDSUtilities.Log(Arguments.Log, "Response", errorToken);

src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.Servers/TransientTdsErrorTdsServerArguments.cs

@@ -25,5 +25,12 @@ public class TransientTdsErrorTdsServerArguments : TdsServerArguments
         /// The number of times the transient error should be raised.
         /// </summary>
         public int RepeatCount { get; set; } = 1;
+
+        /// <summary>
+        /// Error class (severity) to emit in ERROR token.
+        /// Fatal starts at 20 (TdsEnums.FATAL_ERROR_CLASS), so use values below 20
+        /// to avoid automatic break/doom behavior in the client.
+        /// </summary>
+        public byte ErrorClass { get; set; } = 20;
     }
 }