Fix LoginWithFailover missing parser state check (dotnet#4140)

Author: paulmedynski

.github/copilot-instructions.md

@@ -50,6 +50,7 @@ This project includes several key products and libraries that facilitate SQL Ser
 - **Data Encryption**: Supports data encryption for secure data transmission.
 - **Logging and Diagnostics**: Provides event source tracing diagnostic capabilities for troubleshooting.
 - **Failover Support**: Handles automatic failover scenarios for high availability.
+  - Compatibility switch: `Switch.Microsoft.Data.SqlClient.UseLegacyFailoverAlternationOnLoginSqlErrors` (default `false`) can restore legacy alternation behavior in `LoginWithFailover` for login-phase SQL errors.
 - **Cross-Platform Support**: Compatible with both .NET Framework and .NET Core, allowing applications to run on Windows, Linux, and macOS.
 - **Column Encryption AKV Provider**: Supports Azure Key Vault (AKV) provider for acquiring keys from Azure Key Vault to be used for encryption and decryption.
 

.github/instructions/features.instructions.md

@@ -246,6 +246,7 @@ AppContext switches allow runtime behavior changes without modifying connection
 | `Switch.Microsoft.Data.SqlClient.EnableMultiSubnetFailoverByDefault` | `false` | Sets `MultiSubnetFailover=true` as the default for all connections |
 | `Switch.Microsoft.Data.SqlClient.EnableUserAgent` | varies | Controls sending user agent information to SQL Server |
 | `Switch.Microsoft.Data.SqlClient.IgnoreServerProvidedFailoverPartner` | `false` | Ignores failover partner information sent by the server |
+| `Switch.Microsoft.Data.SqlClient.UseLegacyFailoverAlternationOnLoginSqlErrors` | `false` | Restores legacy `LoginWithFailover` alternation for login-phase SQL errors when parser state is not `Closed` |
 | `Switch.Microsoft.Data.SqlClient.LegacyRowVersionNullBehavior` | `false` | Restores legacy null handling for rowversion columns |
 | `Switch.Microsoft.Data.SqlClient.LegacyVarTimeZeroScaleBehaviour` | `false` | Restores legacy zero-scale behavior for time/datetime2/datetimeoffset |
 | `Switch.Microsoft.Data.SqlClient.MakeReadAsyncBlocking` | `false` | Makes ReadAsync behave synchronously (legacy compat) |

AGENTS.md

@@ -79,10 +79,11 @@ Do **not** create branches directly under `main`, `dev/`, or any other top-level
 ### Bug Fix Workflow
 1. Understand the issue from the bug report
 2. Locate relevant code in `src/Microsoft.Data.SqlClient/src/` (do NOT modify legacy `netcore/src/` or `netfx/src/`)
-3. Write a failing test that reproduces the issue
-4. Implement the fix
-5. Ensure all tests pass
-6. Update documentation if behavior changes
+3. Check `.github/instructions/features.instructions.md` for existing AppContext switches (including failover compatibility switches) before introducing behavior changes
+4. Write a failing test that reproduces the issue
+5. Implement the fix
+6. Ensure all tests pass
+7. Update documentation if behavior changes
 
 ### Feature Implementation
 1. Review the feature specification

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

@@ -3635,7 +3635,17 @@ private void LoginWithFailover(
                         continue;
                     }
 
-                    if (IsDoNotRetryConnectError(sqlex) || timeout.IsExpired)
+                    bool isLoginPhaseSqlError = _parser?.State is not TdsParserState.Closed;
+
+                    // 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 ((isLoginPhaseSqlError && !LocalAppContextSwitches.UseLegacyFailoverAlternationOnLoginSqlErrors) ||
+                        IsDoNotRetryConnectError(sqlex) ||
+                        timeout.IsExpired)
                     {
                         // No more time to try again.
                         // Caller will call LoginFailure()

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/LocalAppContextSwitches.cs

@@ -58,6 +58,13 @@ internal static class LocalAppContextSwitches
     private const string IgnoreServerProvidedFailoverPartnerString =
         "Switch.Microsoft.Data.SqlClient.IgnoreServerProvidedFailoverPartner";
 
+    /// <summary>
+    /// The name of the app context switch that controls whether failover
+    /// alternation should use legacy behavior for login-phase SQL errors.
+    /// </summary>
+    private const string UseLegacyFailoverAlternationOnLoginSqlErrorsString =
+        "Switch.Microsoft.Data.SqlClient.UseLegacyFailoverAlternationOnLoginSqlErrors";
+
     /// <summary>
     /// The name of the app context switch that controls whether to preserve
     /// legacy behavior where Timestamp/RowVersion fields return empty byte
@@ -182,6 +189,11 @@ private enum SwitchValue : byte
     /// </summary>
     private static SwitchValue s_ignoreServerProvidedFailoverPartner = SwitchValue.None;
 
+    /// <summary>
+    /// The cached value of the UseLegacyFailoverAlternationOnLoginSqlErrors switch.
+    /// </summary>
+    private static SwitchValue s_useLegacyFailoverAlternationOnLoginSqlErrors = SwitchValue.None;
+
     /// <summary>
     /// The cached value of the LegacyRowVersionNullBehavior switch.
     /// </summary>
@@ -409,6 +421,19 @@ public static bool GlobalizationInvariantMode
             defaultValue: false,
             ref s_ignoreServerProvidedFailoverPartner);
 
+    /// <summary>
+    /// When set to true, LoginWithFailover preserves legacy behavior and may
+    /// alternate to the failover partner on login-phase SQL errors where the
+    /// parser state is not Closed.
+    ///
+    /// The default value of this switch is false.
+    /// </summary>
+    public static bool UseLegacyFailoverAlternationOnLoginSqlErrors =>
+        AcquireAndReturn(
+            UseLegacyFailoverAlternationOnLoginSqlErrorsString,
+            defaultValue: false,
+            ref s_useLegacyFailoverAlternationOnLoginSqlErrors);
+
     /// <summary>
     /// In System.Data.SqlClient and Microsoft.Data.SqlClient prior to 3.0.0 a
     /// field with type Timestamp/RowVersion would return an empty byte array.

src/Microsoft.Data.SqlClient/tests/Common/LocalAppContextSwitchesHelper.cs

@@ -47,6 +47,7 @@ public sealed class LocalAppContextSwitchesHelper : IDisposable
     private readonly bool? _globalizationInvariantModeOriginal;
     #endif
     private readonly bool? _ignoreServerProvidedFailoverPartnerOriginal;
+    private readonly bool? _useLegacyFailoverAlternationOnLoginSqlErrorsOriginal;
     private readonly bool? _legacyRowVersionNullBehaviorOriginal;
     private readonly bool? _legacyVarTimeZeroScaleBehaviourOriginal;
     private readonly bool? _makeReadAsyncBlockingOriginal;
@@ -98,6 +99,8 @@ public LocalAppContextSwitchesHelper()
             #endif
             _ignoreServerProvidedFailoverPartnerOriginal =
                 GetSwitchValue("s_ignoreServerProvidedFailoverPartner");
+            _useLegacyFailoverAlternationOnLoginSqlErrorsOriginal =
+                GetSwitchValue("s_useLegacyFailoverAlternationOnLoginSqlErrors");
             _legacyRowVersionNullBehaviorOriginal =
                 GetSwitchValue("s_legacyRowVersionNullBehavior");
             _legacyVarTimeZeroScaleBehaviourOriginal =
@@ -154,6 +157,9 @@ public void Dispose()
             SetSwitchValue(
                 "s_ignoreServerProvidedFailoverPartner",
                 _ignoreServerProvidedFailoverPartnerOriginal);
+            SetSwitchValue(
+                "s_useLegacyFailoverAlternationOnLoginSqlErrors",
+                _useLegacyFailoverAlternationOnLoginSqlErrorsOriginal);
             SetSwitchValue(
                 "s_legacyRowVersionNullBehavior", 
                 _legacyRowVersionNullBehaviorOriginal);
@@ -242,6 +248,15 @@ public bool? IgnoreServerProvidedFailoverPartner
         set => SetSwitchValue("s_ignoreServerProvidedFailoverPartner", value);
     }
 
+    /// <summary>
+    /// Get or set the UseLegacyFailoverAlternationOnLoginSqlErrors switch value.
+    /// </summary>
+    public bool? UseLegacyFailoverAlternationOnLoginSqlErrors
+    {
+        get => GetSwitchValue("s_useLegacyFailoverAlternationOnLoginSqlErrors");
+        set => SetSwitchValue("s_useLegacyFailoverAlternationOnLoginSqlErrors", value);
+    }
+
     /// <summary>
     /// Get or set the LegacyRowVersionNullBehavior switch value.
     /// </summary>

src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/LocalAppContextSwitchesTest.cs

@@ -28,6 +28,7 @@ public void TestDefaultAppContextSwitchValues()
         Assert.False(LocalAppContextSwitches.UseConnectionPoolV2);
         Assert.False(LocalAppContextSwitches.TruncateScaledDecimal);
         Assert.False(LocalAppContextSwitches.IgnoreServerProvidedFailoverPartner);
+        Assert.False(LocalAppContextSwitches.UseLegacyFailoverAlternationOnLoginSqlErrors);
         Assert.False(LocalAppContextSwitches.EnableMultiSubnetFailoverByDefault);
         #if NET
         Assert.False(LocalAppContextSwitches.GlobalizationInvariantMode);