From 9f67fb1e58af5a4b7efd01604319876f45268a14 Mon Sep 17 00:00:00 2001 From: Priyanka Tiwari Date: Mon, 1 Jun 2026 19:22:21 +0530 Subject: [PATCH 1/3] Reset session isolation level on pool return (fixes #96) SqlTransaction and TransactionScope leave the underlying SQL Server session with the elevated isolation level after Commit/Rollback. sp_reset_connection does not reset it, so the next user of the pooled physical connection silently inherits it. This change tracks when a non-default isolation level was set via the TM Begin path and, on pool return, issues a 'SET TRANSACTION ISOLATION LEVEL READ COMMITTED' batch right before sp_reset_connection (piggybacked on the same TDS header, no extra round trip). On failure the connection is doomed instead of returned to the pool. A new AppContext switch 'Switch.Microsoft.Data.SqlClient.UseLegacyIsolationLevelBehavior' preserves previous behavior for callers that depend on it (default: false). Adds IsolationLevelLeakTest under ManualTests covering the two repro scenarios plus a negative test for the kill switch. --- .../Connection/SqlConnectionInternal.cs | 54 +++++++ .../Data/SqlClient/LocalAppContextSwitches.cs | 29 ++++ ...icrosoft.Data.SqlClient.ManualTests.csproj | 1 + .../TransactionTest/IsolationLevelLeakTest.cs | 149 ++++++++++++++++++ 4 files changed, 233 insertions(+) create mode 100644 src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/IsolationLevelLeakTest.cs diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs index 295286b1e5..52c7ef2f08 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs @@ -281,6 +281,14 @@ internal class SqlConnectionInternal : DbConnectionInternal, IDisposable // @TODO: Rename to match naming conventions (remove f prefix) private readonly bool _fResetConnection; + // Tracks whether a SQL Server session transaction isolation level + // change has been issued on this connection (via SqlTransaction or + // System.Transactions enlistment) but not yet undone. SQL Server's + // sp_reset_connection does not reset the session isolation level, so + // without compensation a pooled connection leaks the level to its + // next user. + private bool _isolationLevelDirty; + // @TODO: Rename to match naming conventions @@ -2679,6 +2687,16 @@ private void ExecuteTransaction2005( internalTransaction, stateObj, isDelegateControlRequest); + + // A successful Begin with a non-default isolation level mutates + // SQL Server's session transaction_isolation_level. sp_reset_connection + // will not undo this, so remember it and reset on deactivate. + if (requestType == TdsEnums.TransactionManagerRequestType.Begin && + isoLevel != TdsEnums.TransactionManagerIsolationLevel.Unspecified && + isoLevel != TdsEnums.TransactionManagerIsolationLevel.ReadCommitted) + { + _isolationLevelDirty = true; + } } finally { @@ -3894,6 +3912,42 @@ internal override void ResetConnection() // Reset dictionary values, since calling reset will not send us env_changes. CurrentDatabase = _originalDatabase; _currentLanguage = _originalLanguage; + + // sp_reset_connection (set up by PrepareResetConnection above) does not reset the + // session transaction isolation level. When we know a previous Begin changed the + // level, issue an explicit SET so the next user of this pooled connection starts at + // READ COMMITTED. The batch piggybacks the queued sp_reset_connection on its TDS + // header, so the round trip is shared and no separate reset packet is sent later. + // Gated by an app-context switch for back-compat. + if (_isolationLevelDirty && !LocalAppContextSwitches.UseLegacyIsolationLevelBehavior) + { + ResetSessionIsolationLevel(); + } + } + } + + // Issues "SET TRANSACTION ISOLATION LEVEL READ COMMITTED;" on the physical state object + // so the next user of this pooled connection observes the default session isolation level. + // Called only from ResetConnection when _isolationLevelDirty is set; runs synchronously + // because Deactivate is a synchronous pool-return path. + private void ResetSessionIsolationLevel() + { + try + { + _parser.TdsExecuteSQLBatch( + text: "SET TRANSACTION ISOLATION LEVEL READ COMMITTED;", + timeout: 0, + notificationRequest: null, + stateObj: _parser._physicalStateObj, + sync: true); + _parser.Run(RunBehavior.UntilDone, null, null, null, _parser._physicalStateObj); + _isolationLevelDirty = false; + } + catch (Exception e) when (ADP.IsCatchableExceptionType(e)) + { + // If we cannot scrub the session state, the connection is not safe to pool. + // Dooming forces Deactivate's outer logic to destroy it instead of pooling it. + DoomThisConnection(); } } diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/LocalAppContextSwitches.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/LocalAppContextSwitches.cs index df762ed7ae..0849d87f81 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/LocalAppContextSwitches.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/LocalAppContextSwitches.cs @@ -65,6 +65,14 @@ internal static class LocalAppContextSwitches private const string UseLegacyFailoverAlternationOnLoginSqlErrorsString = "Switch.Microsoft.Data.SqlClient.UseLegacyFailoverAlternationOnLoginSqlErrors"; + /// + /// The name of the app context switch that controls whether pooled connections + /// preserve the legacy behavior of leaking the SQL Server session + /// transaction isolation level across pool reuse. + /// + private const string UseLegacyIsolationLevelBehaviorString = + "Switch.Microsoft.Data.SqlClient.UseLegacyIsolationLevelBehavior"; + /// /// The name of the app context switch that controls whether to preserve /// legacy behavior where Timestamp/RowVersion fields return empty byte @@ -201,6 +209,11 @@ private enum SwitchValue : byte /// private static SwitchValue s_useLegacyFailoverAlternationOnLoginSqlErrors = SwitchValue.None; + /// + /// The cached value of the UseLegacyIsolationLevelBehavior switch. + /// + private static SwitchValue s_useLegacyIsolationLevelBehavior = SwitchValue.None; + /// /// The cached value of the LegacyRowVersionNullBehavior switch. /// @@ -446,6 +459,22 @@ public static bool GlobalizationInvariantMode defaultValue: false, ref s_useLegacyFailoverAlternationOnLoginSqlErrors); + /// + /// When set to true, pooled connections preserve the legacy behavior where + /// the SQL Server session transaction isolation level is not reset on + /// return to the pool. As a result, the next user of the pooled connection + /// may inherit a non-default isolation level. + /// + /// The default value of this switch is false, meaning the driver will + /// reset the isolation level to READ COMMITTED before the next checkout + /// when it knows the level was changed. + /// + public static bool UseLegacyIsolationLevelBehavior => + AcquireAndReturn( + UseLegacyIsolationLevelBehaviorString, + defaultValue: false, + ref s_useLegacyIsolationLevelBehavior); + /// /// 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. diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTests.csproj b/src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTests.csproj index 1145fa4790..a011352d38 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTests.csproj +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTests.csproj @@ -233,6 +233,7 @@ + diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/IsolationLevelLeakTest.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/IsolationLevelLeakTest.cs new file mode 100644 index 0000000000..7ce7c539f7 --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/IsolationLevelLeakTest.cs @@ -0,0 +1,149 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Data; +using System.Transactions; +using Xunit; +using IsolationLevel = System.Data.IsolationLevel; + +namespace Microsoft.Data.SqlClient.ManualTesting.Tests +{ + // SqlTransaction / TransactionScope used to leave the pooled connection + // with the elevated session isolation level. The next user of the pooled + // connection would silently inherit it. The fix resets the session + // isolation level to READ COMMITTED when the connection is returned to + // the pool. + public static class IsolationLevelLeakTest + { + private const string GetIsoSql = @" +SELECT CASE transaction_isolation_level + WHEN 0 THEN 'Unspecified' + WHEN 1 THEN 'ReadUncommitted' + WHEN 2 THEN 'ReadCommitted' + WHEN 3 THEN 'RepeatableRead' + WHEN 4 THEN 'Serializable' + WHEN 5 THEN 'Snapshot' + END +FROM sys.dm_exec_sessions WHERE session_id = @@SPID;"; + + private static string PooledMaxOneConnString => + new SqlConnectionStringBuilder(DataTestUtility.TCPConnectionString) + { + Pooling = true, + MaxPoolSize = 1, + MultipleActiveResultSets = false, + ApplicationName = "IsoLeakTest" + }.ConnectionString; + + private static int GetSpid(SqlConnection c) + { + using SqlCommand cmd = new SqlCommand("SELECT @@SPID;", c); + return Convert.ToInt32(cmd.ExecuteScalar()); + } + + private static string GetIso(SqlConnection c) + { + using SqlCommand cmd = new SqlCommand(GetIsoSql, c); + return (string)cmd.ExecuteScalar(); + } + + [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))] + public static void SqlTransaction_SerializableDoesNotLeakAcrossPool() + { + string cs = PooledMaxOneConnString; + int spid1; + using (SqlConnection c = new SqlConnection(cs)) + { + c.Open(); + spid1 = GetSpid(c); + using SqlTransaction tx = c.BeginTransaction(IsolationLevel.Serializable); + Assert.Equal("Serializable", GetIsoOnTx(c, tx)); + tx.Rollback(); + } + + using (SqlConnection c = new SqlConnection(cs)) + { + c.Open(); + Assert.Equal(spid1, GetSpid(c)); // pool reuse + Assert.Equal("ReadCommitted", GetIso(c)); + } + } + + [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))] + public static void TransactionScope_SerializableDoesNotLeakAcrossPool() + { + string cs = PooledMaxOneConnString; + int spid1; + using (var scope = new TransactionScope( + TransactionScopeOption.RequiresNew, + new TransactionOptions { IsolationLevel = System.Transactions.IsolationLevel.Serializable }, + TransactionScopeAsyncFlowOption.Enabled)) + using (SqlConnection c = new SqlConnection(cs)) + { + c.Open(); + spid1 = GetSpid(c); + Assert.Equal("Serializable", GetIso(c)); + scope.Complete(); + } + + using (SqlConnection c = new SqlConnection(cs)) + { + c.Open(); + Assert.Equal(spid1, GetSpid(c)); + Assert.Equal("ReadCommitted", GetIso(c)); + } + } + + // Negative test: legacy switch ON brings the old leak back. Runs in + // an isolated AppDomain on .NET Framework / process boundary on .NET + // would be ideal, but AppContext switches set before any pool entry + // is created suffice when this test runs first in its own collection. + [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))] + public static void LegacySwitch_PreservesOldLeakBehavior() + { + const string Switch = "Switch.Microsoft.Data.SqlClient.UseLegacyIsolationLevelBehavior"; + AppContext.SetSwitch(Switch, true); + try + { + // Use a distinct app name so this test gets its own pool group + // and isn't affected by entries created by the other tests. + string cs = new SqlConnectionStringBuilder(DataTestUtility.TCPConnectionString) + { + Pooling = true, + MaxPoolSize = 1, + MultipleActiveResultSets = false, + ApplicationName = "IsoLeakTest-Legacy" + }.ConnectionString; + + int spid1; + using (SqlConnection c = new SqlConnection(cs)) + { + c.Open(); + spid1 = GetSpid(c); + using SqlTransaction tx = c.BeginTransaction(IsolationLevel.Serializable); + tx.Rollback(); + } + + using (SqlConnection c = new SqlConnection(cs)) + { + c.Open(); + Assert.Equal(spid1, GetSpid(c)); + Assert.Equal("Serializable", GetIso(c)); // legacy: leaks + } + } + finally + { + AppContext.SetSwitch(Switch, false); + SqlConnection.ClearAllPools(); + } + } + + private static string GetIsoOnTx(SqlConnection c, SqlTransaction tx) + { + using SqlCommand cmd = new SqlCommand(GetIsoSql, c, tx); + return (string)cmd.ExecuteScalar(); + } + } +} From abc07ca44b16c919872b9f8d07abb71a599f511f Mon Sep 17 00:00:00 2001 From: Priyanka Tiwari Date: Wed, 3 Jun 2026 13:34:36 +0530 Subject: [PATCH 2/3] Address PR review: fix LegacySwitch test cached-switch flake - IsolationLevelLeakTest: switch to LocalAppContextSwitchesHelper so the cached LocalAppContextSwitches value is forced, not just set via AppContext.SetSwitch (production code reads the cache after first use, which made the negative test order-dependent and caused the CI failure). - Helper restores the previous value on dispose, so global state does not leak to other tests. - TransactionScope test: set Enlist=true explicitly on the pooled connection string so the test does not depend on the user's local DataTestUtility.TCPConnectionString defaults. - LocalAppContextSwitchesHelper: expose UseLegacyIsolationLevelBehavior get/set + capture/restore wired through the standard reflection helpers. --- .../Common/LocalAppContextSwitchesHelper.cs | 15 ++++++++ .../TransactionTest/IsolationLevelLeakTest.cs | 37 ++++++++----------- 2 files changed, 31 insertions(+), 21 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/Common/LocalAppContextSwitchesHelper.cs b/src/Microsoft.Data.SqlClient/tests/Common/LocalAppContextSwitchesHelper.cs index 430d6645a8..c68bde6c81 100644 --- a/src/Microsoft.Data.SqlClient/tests/Common/LocalAppContextSwitchesHelper.cs +++ b/src/Microsoft.Data.SqlClient/tests/Common/LocalAppContextSwitchesHelper.cs @@ -46,6 +46,7 @@ public sealed class LocalAppContextSwitchesHelper : IDisposable #endif private readonly bool? _ignoreServerProvidedFailoverPartnerOriginal; private readonly bool? _useLegacyFailoverAlternationOnLoginSqlErrorsOriginal; + private readonly bool? _useLegacyIsolationLevelBehaviorOriginal; private readonly bool? _legacyRowVersionNullBehaviorOriginal; private readonly bool? _legacyVarTimeZeroScaleBehaviourOriginal; private readonly bool? _makeReadAsyncBlockingOriginal; @@ -100,6 +101,8 @@ public LocalAppContextSwitchesHelper() GetSwitchValue("s_ignoreServerProvidedFailoverPartner"); _useLegacyFailoverAlternationOnLoginSqlErrorsOriginal = GetSwitchValue("s_useLegacyFailoverAlternationOnLoginSqlErrors"); + _useLegacyIsolationLevelBehaviorOriginal = + GetSwitchValue("s_useLegacyIsolationLevelBehavior"); _legacyRowVersionNullBehaviorOriginal = GetSwitchValue("s_legacyRowVersionNullBehavior"); _legacyVarTimeZeroScaleBehaviourOriginal = @@ -161,6 +164,9 @@ public void Dispose() SetSwitchValue( "s_useLegacyFailoverAlternationOnLoginSqlErrors", _useLegacyFailoverAlternationOnLoginSqlErrorsOriginal); + SetSwitchValue( + "s_useLegacyIsolationLevelBehavior", + _useLegacyIsolationLevelBehaviorOriginal); SetSwitchValue( "s_legacyRowVersionNullBehavior", _legacyRowVersionNullBehaviorOriginal); @@ -261,6 +267,15 @@ public bool? UseLegacyFailoverAlternationOnLoginSqlErrors set => SetSwitchValue("s_useLegacyFailoverAlternationOnLoginSqlErrors", value); } + /// + /// Get or set the UseLegacyIsolationLevelBehavior switch value. + /// + public bool? UseLegacyIsolationLevelBehavior + { + get => GetSwitchValue("s_useLegacyIsolationLevelBehavior"); + set => SetSwitchValue("s_useLegacyIsolationLevelBehavior", value); + } + /// /// Get or set the LegacyRowVersionNullBehavior switch value. /// diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/IsolationLevelLeakTest.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/IsolationLevelLeakTest.cs index 7ce7c539f7..8baef6f374 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/IsolationLevelLeakTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/IsolationLevelLeakTest.cs @@ -5,6 +5,7 @@ using System; using System.Data; using System.Transactions; +using Microsoft.Data.SqlClient.Tests.Common; using Xunit; using IsolationLevel = System.Data.IsolationLevel; @@ -28,13 +29,14 @@ WHEN 5 THEN 'Snapshot' END FROM sys.dm_exec_sessions WHERE session_id = @@SPID;"; - private static string PooledMaxOneConnString => + private static string BuildPooledConnString(string appName) => new SqlConnectionStringBuilder(DataTestUtility.TCPConnectionString) { Pooling = true, MaxPoolSize = 1, MultipleActiveResultSets = false, - ApplicationName = "IsoLeakTest" + Enlist = true, + ApplicationName = appName }.ConnectionString; private static int GetSpid(SqlConnection c) @@ -52,7 +54,7 @@ private static string GetIso(SqlConnection c) [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))] public static void SqlTransaction_SerializableDoesNotLeakAcrossPool() { - string cs = PooledMaxOneConnString; + string cs = BuildPooledConnString("IsoLeakTest-SqlTx"); int spid1; using (SqlConnection c = new SqlConnection(cs)) { @@ -74,7 +76,7 @@ public static void SqlTransaction_SerializableDoesNotLeakAcrossPool() [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))] public static void TransactionScope_SerializableDoesNotLeakAcrossPool() { - string cs = PooledMaxOneConnString; + string cs = BuildPooledConnString("IsoLeakTest-TxScope"); int spid1; using (var scope = new TransactionScope( TransactionScopeOption.RequiresNew, @@ -96,27 +98,21 @@ public static void TransactionScope_SerializableDoesNotLeakAcrossPool() } } - // Negative test: legacy switch ON brings the old leak back. Runs in - // an isolated AppDomain on .NET Framework / process boundary on .NET - // would be ideal, but AppContext switches set before any pool entry - // is created suffice when this test runs first in its own collection. + // Negative test: legacy switch ON brings the old leak back. + // Uses LocalAppContextSwitchesHelper to force the cached switch value + // in LocalAppContextSwitches (production code reads the cache, not + // AppContext, after first use). The helper restores the previous + // value on dispose. [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))] public static void LegacySwitch_PreservesOldLeakBehavior() { - const string Switch = "Switch.Microsoft.Data.SqlClient.UseLegacyIsolationLevelBehavior"; - AppContext.SetSwitch(Switch, true); + using LocalAppContextSwitchesHelper switchesHelper = new(); + switchesHelper.UseLegacyIsolationLevelBehavior = true; + + // Distinct app name so this test gets its own pool group. + string cs = BuildPooledConnString("IsoLeakTest-Legacy"); try { - // Use a distinct app name so this test gets its own pool group - // and isn't affected by entries created by the other tests. - string cs = new SqlConnectionStringBuilder(DataTestUtility.TCPConnectionString) - { - Pooling = true, - MaxPoolSize = 1, - MultipleActiveResultSets = false, - ApplicationName = "IsoLeakTest-Legacy" - }.ConnectionString; - int spid1; using (SqlConnection c = new SqlConnection(cs)) { @@ -135,7 +131,6 @@ public static void LegacySwitch_PreservesOldLeakBehavior() } finally { - AppContext.SetSwitch(Switch, false); SqlConnection.ClearAllPools(); } } From dbefb9c348d451b659b5c7368b528a6a56364afb Mon Sep 17 00:00:00 2001 From: Priyanka Tiwari Date: Wed, 3 Jun 2026 17:15:23 +0530 Subject: [PATCH 3/3] Gate LegacySwitch_PreservesOldLeakBehavior to non-Azure The leak only manifests on on-prem SQL Server: Azure SQL DB resets the session isolation level inside sp_reset_connection, so the legacy switch becomes a no-op there and the assertion (expects 'Serializable' to leak) fails on Azure CI legs. --- .../SQL/TransactionTest/IsolationLevelLeakTest.cs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/IsolationLevelLeakTest.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/IsolationLevelLeakTest.cs index 8baef6f374..055d7e4439 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/IsolationLevelLeakTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/IsolationLevelLeakTest.cs @@ -99,11 +99,17 @@ public static void TransactionScope_SerializableDoesNotLeakAcrossPool() } // Negative test: legacy switch ON brings the old leak back. + // Only meaningful on on-prem SQL Server: Azure SQL DB resets the + // session isolation level inside sp_reset_connection, so the leak + // never materializes there regardless of the switch. // Uses LocalAppContextSwitchesHelper to force the cached switch value // in LocalAppContextSwitches (production code reads the cache, not // AppContext, after first use). The helper restores the previous // value on dispose. - [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))] + [ConditionalFact( + typeof(DataTestUtility), + nameof(DataTestUtility.AreConnStringsSetup), + nameof(DataTestUtility.IsNotAzureServer))] public static void LegacySwitch_PreservesOldLeakBehavior() { using LocalAppContextSwitchesHelper switchesHelper = new();