From 8d872f153f808812f13543dc90748240cdf45140 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 29 Jul 2026 10:26:54 -0700 Subject: [PATCH 01/16] Implement transaction support in ChannelDbConnectionPool Ports the transacted-pool state machine from WaitHandleDbConnectionPool so the channel pool honors ambient System.Transactions enlistment: - Implement PutObjectFromTransactedPool and TransactionEnded (previously NotImplementedException). - Rewrite ReturnInternalConnection to mirror DeactivateObject: deactivate first, then route the connection to the transacted pool, stasis, the idle channel, or destruction under the connection lock. - Vend connections already enlisted in the ambient transaction via a new GetFromTransactedPool helper, and pass the transaction through to PrepareConnection/ActivateConnection. - Set the ambient transaction on the async acquisition path from the TaskCompletionSource's AsyncState. - Guard RemoveConnection against disposing a transaction root that is still waiting for its delegated transaction to end. Adds ChannelDbConnectionPoolTransactionTest mirroring the WaitHandle pool's transaction test suite, and drops the stale NotImplementedException tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 285 ++++- .../ChannelDbConnectionPoolTest.cs | 31 - .../ChannelDbConnectionPoolTransactionTest.cs | 1079 +++++++++++++++++ 3 files changed, 1337 insertions(+), 58 deletions(-) create mode 100644 src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 3c6347da54..3f28a2316a 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -5,6 +5,7 @@ using System.Collections.Concurrent; using System.Collections.ObjectModel; using System.Data.Common; +using System.Diagnostics; using System.Runtime.CompilerServices; using System.Threading; using System.Threading.Channels; @@ -250,6 +251,14 @@ public ConcurrentDictionary< private int MinPoolSize => PoolGroupOptions.MinPoolSize; + /// + /// Indicates whether connections may be vended from (and parked in) the + /// . This mirrors automatic transaction enlistment: + /// when enlistment is disabled a connection is never bound to an ambient transaction, so + /// the transacted store must not be consulted. + /// + private bool HasTransactionAffinity => PoolGroupOptions.HasTransactionAffinity; + /// /// The most recently launched warmup/replenishment loop task, exposed so tests can await a /// warmup pass to a deterministic completion instead of polling pool counters. May be null @@ -313,7 +322,31 @@ public void Clear() /// public void PutObjectFromTransactedPool(DbConnectionInternal connection) { - throw new NotImplementedException(); + Debug.Assert(connection is not null, "null connection?"); + Debug.Assert(connection.EnlistedTransaction is null, "connection is still enlisted?"); + + // Called by the transacted connection pool once it has removed the connection from its + // list. We put the connection back into general circulation. + // + // NOTE: no locking is required here because if we're in this method we can safely + // presume that the caller is the only one using the connection, that all pre-push logic + // has been done, and that all transactions have ended. + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Connection {1}, Transaction has ended.", + Id, + connection.ObjectID); + + if (State is Running && connection.CanBePooled) + { + connection.ResetConnection(); + PutConnectionInIdleChannel(connection); + } + else + { + // RemoveConnection triggers replenishment, which is the channel pool's equivalent + // of the wait handle pool's QueuePoolCreateRequest. + RemoveConnection(connection); + } } /// @@ -331,7 +364,8 @@ public DbConnectionInternal ReplaceConnection( if (newConnection is not null) { - // TODO: Full transaction enlistment support (Story 2). + // Carry the old connection's enlistment over to the replacement so that a connection + // replaced mid-transaction stays bound to the same transaction. PrepareConnection(owningObject, newConnection, oldConnection.EnlistedTransaction); oldConnection.DeactivateConnection(); RemoveConnection(oldConnection); @@ -373,7 +407,8 @@ public DbConnectionInternal ReplaceConnection( newConnection.PostPop(owningObject); } - // TODO: Full transaction enlistment support (Story 2). + // Carry the old connection's enlistment over to the replacement so that a + // connection replaced mid-transaction stays bound to the same transaction. newConnection.ActivateConnection(oldConnection.EnlistedTransaction); // Place new into old's slot @@ -422,6 +457,116 @@ public void ReturnInternalConnection(DbConnectionInternal connection, DbConnecti { ValidateOwnershipAndSetPoolingState(connection, owningObject); + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Connection {1}, Deactivating.", + Id, + connection.ObjectID); + + // Deactivate before inspecting the connection's transaction state. Deactivation is what + // detaches a completed transaction, so reading EnlistedTransaction beforehand could park + // a connection in the transacted pool under a transaction that has already ended. + connection.DeactivateConnection(); + + if (connection.IsConnectionDoomed) + { + // The connection is not fit for reuse -- just dispose of it. + RemoveConnection(connection); + return; + } + + bool returnToGeneralPool = false; + bool destroyConnection = false; + bool rootTxn = false; + + // A connection with a delegated transaction cannot be handed to a different customer + // until the transaction actually completes, so we send it into stasis -- the + // System.Transactions transaction object keeps it owned (not lost) and is certain to + // put it back into the pool. The decision is made under the connection lock so that a + // transaction completing asynchronously on another thread cannot race with us. + lock (connection) + { + if (State is ShuttingDown) + { + if (connection.IsTransactionRoot) + { + // Connections affiliated with a root transaction that happen to live in a + // pool being shut down must be put in stasis so the root transaction isn't + // orphaned with no means to promote itself to a full delegated transaction + // or to Commit/Rollback. + connection.SetInStasis(); + rootTxn = true; + } + else + { + destroyConnection = true; + } + } + else if (connection.IsTransactionRoot && connection.Pool is null) + { + connection.SetInStasis(); + rootTxn = true; + } + else if (connection.CanBePooled) + { + Transaction? transaction = connection.EnlistedTransaction; + if (transaction is not null) + { + // NOTE: we're not locking on State, so its value could change between the + // conditional check above and here. Although perhaps not ideal, this is OK + // because the DelegatedTransactionEnded event will clean up the connection + // appropriately regardless of the pool state. + // + // Transacting connections are held in their own store and are never + // proactively closed (doing so would abort the transaction, which can be + // distributed). Idle-timeout enforcement does not apply here, so we do not + // stamp the returned time when parking the connection in the transacted pool. + TransactedConnectionPool.PutTransactedObject(transaction, connection); + rootTxn = true; + } + else + { + returnToGeneralPool = true; + } + } + else if (connection.IsTransactionRoot) + { + // The connection cannot be pooled but is a transaction root, so we must have + // hit a race condition: either the pool was shut down or the load balancing + // timeout expired and marked the connection as non-poolable while we were + // processing within this lock. Put it in stasis so the root transaction isn't + // orphaned with no means to promote itself to a full delegated transaction or + // to Commit/Rollback. + connection.SetInStasis(); + rootTxn = true; + } + else + { + destroyConnection = true; + } + } + + if (returnToGeneralPool) + { + Debug.Assert(!destroyConnection, "Connection cannot both be pooled and destroyed."); + PutConnectionInIdleChannel(connection); + } + else if (destroyConnection) + { + RemoveConnection(connection); + } + + // Ensure the connection was processed by exactly one of the paths above. + Debug.Assert(rootTxn || returnToGeneralPool || destroyConnection, + "Returned connection was neither pooled, destroyed, nor placed in stasis."); + } + + /// + /// Places a connection that is fit for general reuse into the idle channel, stamping its + /// idle-return time and dropping it if it is no longer live. + /// + /// The connection to make available to other callers. + private void PutConnectionInIdleChannel(DbConnectionInternal connection) + { // Stamp the return time before IsLiveConnection runs so the idle-expiry gate inside it // measures time-in-pool, not time-since-last-return. Without this, a connection whose // checkout exceeded IdleTimeout (e.g. a long-running query) would be wrongly evicted on @@ -440,27 +585,12 @@ public void ReturnInternalConnection(DbConnectionInternal connection, DbConnecti return; } - SqlClientEventSource.Log.TryPoolerTraceEvent( - " {0}, Connection {1}, Deactivating.", - Id, - connection.ObjectID); - connection.DeactivateConnection(); - - if (connection.IsConnectionDoomed || - !connection.CanBePooled || - State == ShuttingDown) + if (!_idleChannel.TryWrite(connection)) { + // The channel has been completed (pool is shutting down). Race window + // between the State check by the caller and TryWrite: destroy instead of pooling. RemoveConnection(connection); } - else - { - if (!_idleChannel.TryWrite(connection)) - { - // The channel has been completed (pool is shutting down). Race window - // between the State check above and TryWrite: destroy instead of pooling. - RemoveConnection(connection); - } - } } /// @@ -614,7 +744,21 @@ public void Startup() /// public void TransactionEnded(Transaction transaction, DbConnectionInternal transactedObject) { - throw new NotImplementedException(); + Debug.Assert(transaction is not null, "null transaction?"); + Debug.Assert(transactedObject is not null, "null transactedObject?"); + + // Note: the connection may still be associated with the transaction due to the explicit + // unbinding requirement. + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Transaction {1}, Connection {2}, Transaction Completed", + Id, + transaction.GetHashCode(), + transactedObject.ObjectID); + + // If the connection is in the transacted pool, remove it there and return it to general + // circulation. TransactedConnectionPool.TransactionEnded calls back into + // PutObjectFromTransactedPool for us once it has removed the connection from its list. + TransactedConnectionPool.TransactionEnded(transaction, transactedObject); } /// @@ -682,7 +826,8 @@ public bool TryGetConnection( // We're potentially on a new thread, so we need to properly set the ambient transaction. // We rely on the caller to capture the ambient transaction in the TaskCompletionSource's AsyncState // so that we can access it here. Read: area for improvement. - // TODO: ADP.SetCurrentTransaction(taskCompletionSource.Task.AsyncState as Transaction); + ADP.SetCurrentTransaction(taskCompletionSource.Task.AsyncState as Transaction); + DbConnectionInternal? connection = null; try @@ -949,6 +1094,19 @@ private bool IsLiveConnection(DbConnectionInternal connection) /// The connection to be closed. private void RemoveConnection(DbConnectionInternal connection) { + // A connection with a delegated transaction cannot be disposed of until the delegated + // transaction has actually completed; disposing it would abort the (possibly + // distributed) transaction. Leave it alone: when the transaction completes it comes + // back through PutObjectFromTransactedPool, which calls us again. + if (connection.IsTxRootWaitingForTxEnd) + { + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Connection {1}, Has Delegated Transaction, waiting to Dispose.", + Id, + connection.ObjectID); + return; + } + _connectionSlots.TryRemove(connection); // Removing a connection from the pool opens a free slot. @@ -1018,14 +1176,26 @@ private async Task GetInternalConnection( TimeoutTimer timeout) { DbConnectionInternal? connection = null; + Transaction? transaction = null; + + // If automatic transaction enlistment is enabled, we first try to get a connection that + // is already enlisted in the ambient transaction. If enlistment is not enabled we cannot + // vend connections from the transacted pool. + if (HasTransactionAffinity) + { + connection = GetFromTransactedPool(out transaction); + } // Derive a CancellationTokenSource from the TimeoutTimer so pool-internal wait operations // (channel reads, semaphore waits) are cancelled when the overall budget expires. using CancellationTokenSource cancellationTokenSource = timeout.CreateCancellationTokenSource(); CancellationToken cancellationToken = cancellationTokenSource.Token; - // Continue looping until we create or retrieve a connection - do + // Continue looping until we create or retrieve a connection. A connection vended from + // the transacted pool skips this loop entirely: it has already been liveness-checked and + // is exempt from the idle/generation gates, because closing it would abort its + // (possibly distributed) transaction. + while (connection is null) { try { @@ -1070,9 +1240,8 @@ private async Task GetInternalConnection( connection = null; } } - while (connection is null); - PrepareConnection(owningConnection, connection); + PrepareConnection(owningConnection, connection, transaction); return connection; } @@ -1141,6 +1310,68 @@ private void PrepareConnection(DbConnection owningObject, DbConnectionInternal c } } + /// + /// Attempts to retrieve a connection that is already enlisted in the ambient transaction. + /// + /// Receives the ambient transaction, or null when there is none. + /// The caller must enlist whatever connection it ends up using in this transaction, even + /// when no transacted connection was available. + /// A live connection already enlisted in the ambient transaction, or null. + private DbConnectionInternal? GetFromTransactedPool(out Transaction? transaction) + { + transaction = ADP.GetCurrentTransaction(); + if (transaction is null) + { + return null; + } + + DbConnectionInternal? connection = TransactedConnectionPool.GetTransactedObject(transaction); + if (connection is null) + { + return null; + } + + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Connection {1}, Popped from transacted pool.", + Id, + connection.ObjectID); + + SqlClientDiagnostics.Metrics.ExitFreeConnection(); + + // Transacting connections are exempt from idle-timeout and clear-generation eviction + // (closing them would abort the transaction, which may be distributed), so only + // liveness is checked here rather than the full IsLiveConnection gate. + if (connection.IsTransactionRoot) + { + try + { + // A dead transaction root must surface the underlying failure to the caller: + // there is no way to recover the delegated transaction on another connection. + connection.IsConnectionAlive(throwOnException: true); + } + catch + { + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Connection {1}, found dead and removed.", + Id, + connection.ObjectID); + RemoveConnection(connection); + throw; + } + } + else if (!connection.IsConnectionAlive()) + { + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Connection {1}, found dead and removed.", + Id, + connection.ObjectID); + RemoveConnection(connection); + connection = null; + } + + return connection; + } + /// /// Validates that the connection is owned by the provided DbConnection and that it is in a valid state to be returned to the pool. /// diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs index b3c9815eb8..228e949cd0 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs @@ -888,37 +888,6 @@ public void TestReplaceConnection() #endregion - #region Not Implemented Method Tests - - /// - /// Verifies that remains - /// unimplemented and throws . - /// - [Fact] - public void TestPutObjectFromTransactedPool() - { - // Arrange - var pool = ConstructPool(SuccessfulConnectionFactory); - - // Act & Assert - Assert.Throws(() => pool.PutObjectFromTransactedPool(null!)); - } - - /// - /// Verifies that - /// remains unimplemented and throws . - /// - [Fact] - public void TestTransactionEnded() - { - // Arrange - var pool = ConstructPool(SuccessfulConnectionFactory); - - // Act & Assert - Assert.Throws(() => pool.TransactionEnded(null!, null!)); - } - #endregion - #region Pool Clear Tests /// diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs new file mode 100644 index 0000000000..7d2bfb95c5 --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs @@ -0,0 +1,1079 @@ +// 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.Data.Common; +using System.Threading; +using System.Threading.Tasks; +using System.Transactions; +using Microsoft.Data.Common.ConnectionString; +using Microsoft.Data.ProviderBase; +using Microsoft.Data.SqlClient.ConnectionPool; +using Xunit; + +namespace Microsoft.Data.SqlClient.UnitTests.ConnectionPool; + +/// +/// Deterministic tests for ChannelDbConnectionPool transaction functionality. +/// These tests exercise transacted connection pathways with controlled synchronization +/// to verify correct behavior without relying on probabilistic concurrency. +/// +public class ChannelDbConnectionPoolTransactionTest : IDisposable +{ + private const int DefaultMaxPoolSize = 50; + private const int DefaultMinPoolSize = 0; + private const int DefaultCreationTimeoutInMilliseconds = 15000; + + private IDbConnectionPool _pool = null!; + + public ChannelDbConnectionPoolTransactionTest() + { + _pool = CreatePool(); + } + + public void Dispose() + { + // Verify no leaked transactions before cleanup + Assert.Empty(_pool.TransactedConnectionPool.TransactedConnections); + + _pool?.Shutdown(); + _pool?.Clear(); + } + + #region Helper Methods + + private ChannelDbConnectionPool CreatePool( + int maxPoolSize = DefaultMaxPoolSize, + int minPoolSize = DefaultMinPoolSize, + bool hasTransactionAffinity = true) + { + var poolGroupOptions = new DbConnectionPoolGroupOptions( + poolByIdentity: false, + minPoolSize: minPoolSize, + maxPoolSize: maxPoolSize, + creationTimeout: DefaultCreationTimeoutInMilliseconds, + loadBalanceTimeout: 0, + hasTransactionAffinity: hasTransactionAffinity, + idleTimeout: 0 + ); + + var dbConnectionPoolGroup = new DbConnectionPoolGroup( + new SqlConnectionOptions("Data Source=localhost;"), + new ConnectionPoolKey("TestDataSource", credential: null, accessToken: null, accessTokenCallback: null, sspiContextProvider: null), + poolGroupOptions + ); + + var connectionFactory = new MockSqlConnectionFactory(); + + var pool = new ChannelDbConnectionPool( + connectionFactory, + dbConnectionPoolGroup, + DbConnectionPoolIdentity.NoIdentity, + new DbConnectionPoolProviderInfo() + ); + + pool.Startup(); + return pool; + } + + private DbConnectionInternal GetConnection(SqlConnection owner) + { + _pool.TryGetConnection( + owner, + taskCompletionSource: null, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), + out DbConnectionInternal? connection); + return connection!; + } + + private async Task GetConnectionAsync( + SqlConnection owner, + Transaction? transaction = null) + { + var tcs = new TaskCompletionSource(transaction); + _pool.TryGetConnection( + owner, + taskCompletionSource: tcs, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), + out DbConnectionInternal? connection); + return connection ?? await tcs.Task; + } + + private void ReturnConnection(DbConnectionInternal connection, SqlConnection owner) + { + _pool.ReturnInternalConnection(connection, owner); + } + + private void AssertPoolMetrics() + { + Assert.True(_pool.Count <= _pool.PoolGroupOptions.MaxPoolSize, + $"Pool count ({_pool.Count}) exceeded max pool size ({_pool.PoolGroupOptions.MaxPoolSize})"); + Assert.True(_pool.Count >= 0, + $"Pool count ({_pool.Count}) is negative"); + Assert.Empty(_pool.TransactedConnectionPool.TransactedConnections); + } + + #endregion + + #region Transaction Routing Tests + + [Fact] + public void GetConnection_UnderTransaction_RoutesToTransactedPool() + { + // Arrange & Act + using var scope = new TransactionScope(); + var transaction = Transaction.Current; + Assert.NotNull(transaction); + + var owner = new SqlConnection(); + var conn = GetConnection(owner); + Assert.NotNull(conn); + + ReturnConnection(conn, owner); + + // Assert - connection should be in the transacted pool + Assert.True(_pool.TransactedConnectionPool.TransactedConnections.ContainsKey(transaction)); + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections[transaction]); + + scope.Complete(); + } + + [Fact] + public void GetConnection_WithoutTransaction_RoutesToGeneralPool() + { + // Arrange & Act (no TransactionScope) + var owner = new SqlConnection(); + var conn = GetConnection(owner); + Assert.NotNull(conn); + + ReturnConnection(conn, owner); + + // Assert - transacted pool should be empty + Assert.Empty(_pool.TransactedConnectionPool.TransactedConnections); + } + + [Fact] + public void GetConnection_UnderTransaction_ReturnsSameConnectionFromTransactedPool() + { + // Arrange + using var scope = new TransactionScope(); + + // Act - first call creates a new connection + var owner1 = new SqlConnection(); + var conn1 = GetConnection(owner1); + Assert.NotNull(conn1); + ReturnConnection(conn1, owner1); + + // Second call should retrieve the SAME connection from the transacted pool (LIFO) + var owner2 = new SqlConnection(); + var conn2 = GetConnection(owner2); + Assert.NotNull(conn2); + Assert.Same(conn1, conn2); + + ReturnConnection(conn2, owner2); + scope.Complete(); + } + + [Fact] + public async Task GetConnectionAsync_UnderTransaction_ReturnsSameConnectionFromTransactedPool() + { + // Arrange + using var scope = new TransactionScope(TransactionScopeAsyncFlowOption.Enabled); + var transaction = Transaction.Current; + + // Act - first call creates a new connection + var owner1 = new SqlConnection(); + var conn1 = await GetConnectionAsync(owner1, transaction: transaction); + Assert.NotNull(conn1); + ReturnConnection(conn1, owner1); + + // Second call should retrieve the SAME connection from the transacted pool + var owner2 = new SqlConnection(); + var conn2 = await GetConnectionAsync(owner2, transaction: transaction); + Assert.NotNull(conn2); + Assert.Same(conn1, conn2); + + ReturnConnection(conn2, owner2); + scope.Complete(); + } + + [Fact] + public void GetConnection_WithTransactionAffinityDisabled_SkipsTransactedPool() + { + // Arrange + _pool.Shutdown(); + _pool.Clear(); + _pool = CreatePool(hasTransactionAffinity: false); + + using var scope = new TransactionScope(); + + // Act + var owner = new SqlConnection(); + var conn = GetConnection(owner); + Assert.NotNull(conn); + ReturnConnection(conn, owner); + + // Assert - even though a transaction is active, transacted pool is not used + Assert.Empty(_pool.TransactedConnectionPool.TransactedConnections); + + scope.Complete(); + } + + #endregion + + #region Transaction Lifecycle Tests + + [Fact] + public void TransactionCommit_ClearsTransactedPool() + { + // Arrange & Act + using (var scope = new TransactionScope()) + { + var owner = new SqlConnection(); + var conn = GetConnection(owner); + Assert.NotNull(conn); + ReturnConnection(conn, owner); + + // While transaction is active, connection should be in transacted pool + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); + + scope.Complete(); + } + + // Assert - after transaction completes, transacted pool should be empty + AssertPoolMetrics(); + } + + [Fact] + public void TransactionRollback_ClearsTransactedPool() + { + // Arrange & Act + using (var scope = new TransactionScope()) + { + var owner = new SqlConnection(); + var conn = GetConnection(owner); + Assert.NotNull(conn); + ReturnConnection(conn, owner); + + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); + + // Don't call scope.Complete() — triggers rollback + } + + // Assert - transacted pool should be empty after rollback too + AssertPoolMetrics(); + } + + [Fact] + public void MultipleGetReturn_SameTransaction_ReusesConnection() + { + // Arrange + using var scope = new TransactionScope(); + var transaction = Transaction.Current; + Assert.NotNull(transaction); + + // Act - get and return multiple times within same transaction + for (int i = 0; i < 10; i++) + { + var owner = new SqlConnection(); + var conn = GetConnection(owner); + Assert.NotNull(conn); + ReturnConnection(conn, owner); + } + + // Assert - only one connection should be in the transacted pool + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections[transaction]); + + scope.Complete(); + } + + [Fact] + public async Task MultipleGetReturn_SameTransaction_Async_ReusesConnection() + { + // Arrange + using var scope = new TransactionScope(TransactionScopeAsyncFlowOption.Enabled); + var transaction = Transaction.Current; + Assert.NotNull(transaction); + + // Act - get and return multiple times within same transaction + for (int i = 0; i < 10; i++) + { + var owner = new SqlConnection(); + var conn = await GetConnectionAsync(owner, transaction: transaction); + Assert.NotNull(conn); + ReturnConnection(conn, owner); + } + + // Assert - only one connection should be in the transacted pool + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections[transaction]); + + scope.Complete(); + } + + [Fact] + public void AlternatingCommitAndRollback_MaintainsConsistentState() + { + // Act - alternate between commit and rollback + for (int i = 0; i < 20; i++) + { + using var scope = new TransactionScope(); + var owner = new SqlConnection(); + var conn = GetConnection(owner); + Assert.NotNull(conn); + ReturnConnection(conn, owner); + + if (i % 2 == 0) + { + scope.Complete(); + } + // else: rollback (no Complete) + } + + // Assert + AssertPoolMetrics(); + } + + #endregion + + #region Nested Transaction Tests + + [Fact] + public void NestedTransaction_Required_SharesSameTransactedEntry() + { + // Arrange + using var outerScope = new TransactionScope(); + var outerTxn = Transaction.Current; + Assert.NotNull(outerTxn); + + var owner1 = new SqlConnection(); + var conn1 = GetConnection(owner1); + Assert.NotNull(conn1); + ReturnConnection(conn1, owner1); + + // Act - nested scope with Required shares the same transaction + using (var innerScope = new TransactionScope(TransactionScopeOption.Required)) + { + Assert.Same(outerTxn, Transaction.Current); + + var owner2 = new SqlConnection(); + var conn2 = GetConnection(owner2); + Assert.NotNull(conn2); + Assert.Same(conn1, conn2); // Same transaction -> same connection from transacted pool + ReturnConnection(conn2, owner2); + + // Only one transaction tracked + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); + + innerScope.Complete(); + } + + outerScope.Complete(); + } + + [Fact] + public void NestedTransaction_RequiresNew_CreatesSeparateTransactedEntry() + { + // Arrange + using var outerScope = new TransactionScope(); + var outerTxn = Transaction.Current; + Assert.NotNull(outerTxn); + + var owner1 = new SqlConnection(); + var conn1 = GetConnection(owner1); + Assert.NotNull(conn1); + ReturnConnection(conn1, owner1); + + // Act - nested scope with RequiresNew creates a new transaction + using (var innerScope = new TransactionScope(TransactionScopeOption.RequiresNew)) + { + var innerTxn = Transaction.Current; + Assert.NotNull(innerTxn); + Assert.NotEqual(outerTxn, innerTxn); + + var owner2 = new SqlConnection(); + var conn2 = GetConnection(owner2); + Assert.NotNull(conn2); + Assert.NotSame(conn1, conn2); // Different transaction -> different connection + ReturnConnection(conn2, owner2); + + // Two separate transactions tracked + Assert.Equal(2, _pool.TransactedConnectionPool.TransactedConnections.Count); + + innerScope.Complete(); + } + + outerScope.Complete(); + } + + [Fact] + public void NestedTransaction_RequiresNew_CompletesIndependently() + { + // Arrange & Act + using (var outerScope = new TransactionScope()) + { + var owner1 = new SqlConnection(); + var conn1 = GetConnection(owner1); + Assert.NotNull(conn1); + ReturnConnection(conn1, owner1); + + using (var innerScope = new TransactionScope(TransactionScopeOption.RequiresNew)) + { + var owner2 = new SqlConnection(); + var conn2 = GetConnection(owner2); + Assert.NotNull(conn2); + ReturnConnection(conn2, owner2); + innerScope.Complete(); + } + + // Inner transaction completed - its entry should be cleared + // Outer transaction entry should still exist + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); + + outerScope.Complete(); + } + + // Both completed + AssertPoolMetrics(); + } + + [Fact] + public void DeeplyNestedTransactions_RequiresNew_AllTrackedSeparately() + { + // Arrange & Act + using var scope1 = new TransactionScope(); + var owner1 = new SqlConnection(); + var conn1 = GetConnection(owner1); + ReturnConnection(conn1, owner1); + + using var scope2 = new TransactionScope(TransactionScopeOption.RequiresNew); + var owner2 = new SqlConnection(); + var conn2 = GetConnection(owner2); + ReturnConnection(conn2, owner2); + + using var scope3 = new TransactionScope(TransactionScopeOption.RequiresNew); + var owner3 = new SqlConnection(); + var conn3 = GetConnection(owner3); + ReturnConnection(conn3, owner3); + + // Assert - three separate transactions tracked + Assert.Equal(3, _pool.TransactedConnectionPool.TransactedConnections.Count); + + scope3.Complete(); + scope2.Complete(); + scope1.Complete(); + } + + [Fact] + public void DeeplyNestedTransactions_Required_AllShareOneEntry() + { + // Arrange & Act + using var scope1 = new TransactionScope(); + var txn = Transaction.Current; + var owner1 = new SqlConnection(); + var conn1 = GetConnection(owner1); + ReturnConnection(conn1, owner1); + + using var scope2 = new TransactionScope(TransactionScopeOption.Required); + Assert.Same(txn, Transaction.Current); + var owner2 = new SqlConnection(); + var conn2 = GetConnection(owner2); + Assert.Same(conn1, conn2); + ReturnConnection(conn2, owner2); + + using var scope3 = new TransactionScope(TransactionScopeOption.Required); + Assert.Same(txn, Transaction.Current); + var owner3 = new SqlConnection(); + var conn3 = GetConnection(owner3); + Assert.Same(conn1, conn3); + ReturnConnection(conn3, owner3); + + // Assert - single transaction entry + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); + + scope3.Complete(); + scope2.Complete(); + scope1.Complete(); + } + + #endregion + + #region Mixed Transacted and Non-Transacted Tests + + [Fact] + public void MixedWorkload_AlternatingTransactedAndNonTransacted() + { + // Act - alternate between transacted and non-transacted + for (int i = 0; i < 10; i++) + { + if (i % 2 == 0) + { + using var scope = new TransactionScope(); + var owner = new SqlConnection(); + var conn = GetConnection(owner); + Assert.NotNull(conn); + ReturnConnection(conn, owner); + scope.Complete(); + } + else + { + var owner = new SqlConnection(); + var conn = GetConnection(owner); + Assert.NotNull(conn); + ReturnConnection(conn, owner); + } + } + + // Assert + AssertPoolMetrics(); + } + + #endregion + + #region Shared Transaction Tests + + [Fact] + public void SharedTransaction_DependentScopes_UseTransactedPool() + { + // Arrange + using var scope = new TransactionScope(TransactionScopeAsyncFlowOption.Enabled); + var transaction = Transaction.Current; + Assert.NotNull(transaction); + + // Act - first connection + var owner1 = new SqlConnection(); + var conn1 = GetConnection(owner1); + Assert.NotNull(conn1); + ReturnConnection(conn1, owner1); + + // Use dependent scope on same transaction + using (var innerScope = new TransactionScope(transaction)) + { + Assert.Same(transaction, Transaction.Current); + var owner2 = new SqlConnection(); + var conn2 = GetConnection(owner2); + Assert.NotNull(conn2); + Assert.Same(conn1, conn2); // Same transaction -> same connection + ReturnConnection(conn2, owner2); + innerScope.Complete(); + } + + // Assert - still one transaction entry + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections[transaction]); + + scope.Complete(); + } + + #endregion + + #region Pool Saturation with Transactions Tests + + [Fact] + public void PoolSaturation_BlocksUntilConnectionAvailable() + { + // Arrange - small pool + _pool.Shutdown(); + _pool.Clear(); + _pool = CreatePool(maxPoolSize: 1); + + using var allAcquired = new ManualResetEventSlim(false); + using var releaseFirst = new ManualResetEventSlim(false); + + var saturatingTask = Task.Run(() => + { + using var scope = new TransactionScope(); + var owner = new SqlConnection(); + var conn = GetConnection(owner); + Assert.NotNull(conn); + + allAcquired.Set(); // Signal that this connection is held + + Assert.True(releaseFirst.Wait(TimeSpan.FromSeconds(15)), + "Timed out waiting for releaseFirst signal."); + + ReturnConnection(conn, owner); + scope.Complete(); + }); + + Assert.True(allAcquired.Wait(TimeSpan.FromSeconds(10)), + "Timed out waiting for connection to be acquired."); + Assert.Equal(1, _pool.Count); + + using var acquired = new ManualResetEventSlim(false); + var waitingTask = Task.Run(() => + { + using var scope = new TransactionScope(); + var owner = new SqlConnection(); + var conn = GetConnection(owner); + Assert.NotNull(conn); + acquired.Set(); + ReturnConnection(conn, owner); + scope.Complete(); + }); + + // Give the waiting task time to block — it should NOT complete yet + Assert.False(acquired.Wait(TimeSpan.FromMilliseconds(500)), + "Waiting task should not have acquired a connection while pool is saturated"); + + // Release one connection to unblock the waiting task + releaseFirst.Set(); + + // Now the waiting task should complete + Assert.True(waitingTask.Wait(TimeSpan.FromSeconds(15)), + "Waiting task should have completed after a connection was released"); + Assert.True(acquired.IsSet); + + // Cleanup remaining held connections + Task.WaitAll(saturatingTask); + } + + #endregion + + #region Controlled Concurrency Tests + + // Flaky under CI load only (never reproduces locally): the two worker tasks are + // scheduled via Task.Run on the thread pool. On a loaded agent the pool can be slow to + // spin up a worker, so task1 starts late and fails to signal task1Returned within + // task2's 10s wait, producing a WaitAll timeout. That is thread-pool starvation, not a + // pool/transaction defect. + [Trait("Category", "flaky")] + [Fact] + public void TwoThreads_SharedTransaction_AccessSameTransactedEntry() + { + // Arrange + // Use 3-phase synchronization so task1 gets AND returns before task2 requests. + // This ensures the connection is back in the transacted pool for task2 to reuse. + using var task1Returned = new ManualResetEventSlim(false); + using var task2Done = new ManualResetEventSlim(false); + DbConnectionInternal? connFromTask1 = null; + DbConnectionInternal? connFromTask2 = null; + + using var scope = new TransactionScope(TransactionScopeAsyncFlowOption.Enabled); + var transaction = Transaction.Current; + Assert.NotNull(transaction); + + // Act - two threads sharing the same transaction, sequenced so the + // transacted pool can vend the same connection to both. + var task1 = Task.Run(() => + { + using var innerScope = new TransactionScope(transaction); + var owner = new SqlConnection(); + connFromTask1 = GetConnection(owner); + Assert.NotNull(connFromTask1); + + // Return the connection so it's available in the transacted pool + ReturnConnection(connFromTask1, owner); + innerScope.Complete(); + + task1Returned.Set(); // Signal: connection is back in the transacted pool + }); + + var task2 = Task.Run(() => + { + // Wait until task1 has returned the connection to the transacted pool + Assert.True(task1Returned.Wait(TimeSpan.FromSeconds(10)), + "Timed out waiting for task1 to return its connection."); + + using var innerScope = new TransactionScope(transaction); + var owner = new SqlConnection(); + connFromTask2 = GetConnection(owner); + Assert.NotNull(connFromTask2); + ReturnConnection(connFromTask2, owner); + innerScope.Complete(); + }); + + Task.WaitAll(task1, task2); + + // Both tasks should have received the same connection via the transacted pool + Assert.Same(connFromTask1, connFromTask2); + scope.Complete(); + } + + [Fact] + public async Task TwoThreads_SeparateTransactions_Async_IsolatedTransactedEntries() + { + // Arrange + using var barrier = new SemaphoreSlim(0, 2); + + // Act + var task1 = Task.Run(async () => + { + using var scope = new TransactionScope(TransactionScopeAsyncFlowOption.Enabled); + var transaction = Transaction.Current; + var owner = new SqlConnection(); + var conn = await GetConnectionAsync(owner, transaction: transaction); + Assert.NotNull(conn); + ReturnConnection(conn, owner); + + barrier.Release(); // Signal ready + await barrier.WaitAsync(); // Wait for other task + + scope.Complete(); + }); + + var task2 = Task.Run(async () => + { + using var scope = new TransactionScope(TransactionScopeAsyncFlowOption.Enabled); + var transaction = Transaction.Current; + var owner = new SqlConnection(); + var conn = await GetConnectionAsync(owner, transaction: transaction); + Assert.NotNull(conn); + ReturnConnection(conn, owner); + + barrier.Release(); // Signal ready + await barrier.WaitAsync(); // Wait for other task + + scope.Complete(); + }); + + await Task.WhenAll(task1, task2); + + // Assert + AssertPoolMetrics(); + } + + #endregion + + #region Pool Shutdown with Transactions Tests + + [Fact] + public void PoolShutdown_AfterTransactionComplete_NoLeaks() + { + // Arrange + using (var scope = new TransactionScope()) + { + var owner = new SqlConnection(); + var conn = GetConnection(owner); + Assert.NotNull(conn); + ReturnConnection(conn, owner); + scope.Complete(); + } + + // Act + _pool.Shutdown(); + + // Assert + AssertPoolMetrics(); + } + + [Fact] + public void PoolShutdown_WhileConnectionHeld_NoException() + { + // Arrange + using var scope = new TransactionScope(); + var owner = new SqlConnection(); + var conn = GetConnection(owner); + Assert.NotNull(conn); + + // Act - shutdown while connection is held (not yet returned) + _pool.Shutdown(); + + // Return after shutdown — the pool deactivates and disposes the connection + // rather than returning it to the pool. Verify this doesn't throw. + ReturnConnection(conn, owner); + + // Assert + // The connection should have been deactivated and disposed (not returned to the pool). + // After Dispose(), IsConnectionDoomed is set to true and Pool is set to null. + Assert.True(conn.IsConnectionDoomed, + "Connection should be doomed after returning to a shut-down pool."); + Assert.Null(conn.Pool); + } + + #endregion + + #region Transaction Complete Before Return Tests + + [Fact] + public void TransactionComplete_ThenReturn_ConnectionStillReturned() + { + // Arrange + var owner = new SqlConnection(); + DbConnectionInternal conn; + + using (var scope = new TransactionScope()) + { + conn = GetConnection(owner); + Assert.NotNull(conn); + scope.Complete(); + } + // Transaction is fully disposed here + + // Act - return connection after transaction ended + ReturnConnection(conn, owner); + + // Assert - no leak, pool metrics consistent + AssertPoolMetrics(); + Assert.True(_pool.Count > 0, "Pool should still have the connection"); + } + + #endregion + + #region Sequential Transaction Isolation Tests + + [Fact] + public void SequentialTransactions_EachGetsOwnTransactedEntry() + { + // Act - create multiple sequential transactions + for (int i = 0; i < 5; i++) + { + using var scope = new TransactionScope(); + var transaction = Transaction.Current; + Assert.NotNull(transaction); + + var owner = new SqlConnection(); + var conn = GetConnection(owner); + Assert.NotNull(conn); + ReturnConnection(conn, owner); + + // Only the current transaction should be tracked + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); + Assert.True(_pool.TransactedConnectionPool.TransactedConnections.ContainsKey(transaction)); + + scope.Complete(); + } + + // Assert - after all are done, pool should be clean + AssertPoolMetrics(); + } + + [Fact] + public async Task SequentialTransactions_Async_EachGetsOwnTransactedEntry() + { + // Act - create multiple sequential transactions + for (int i = 0; i < 5; i++) + { + using var scope = new TransactionScope(TransactionScopeAsyncFlowOption.Enabled); + var transaction = Transaction.Current; + Assert.NotNull(transaction); + + var owner = new SqlConnection(); + var conn = await GetConnectionAsync(owner, transaction: transaction); + Assert.NotNull(conn); + ReturnConnection(conn, owner); + + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); + Assert.True(_pool.TransactedConnectionPool.TransactedConnections.ContainsKey(transaction)); + + scope.Complete(); + } + + // Assert + AssertPoolMetrics(); + } + + [Fact] + public void SequentialTransactions_CanReuseConnections() + { + // Act + DbConnectionInternal conn1; + DbConnectionInternal conn2; + Transaction? txn1; + Transaction? txn2; + + using (var scope1 = new TransactionScope()) + { + txn1 = Transaction.Current; + var owner1 = new SqlConnection(); + conn1 = GetConnection(owner1); + Assert.NotNull(conn1); + ReturnConnection(conn1, owner1); + scope1.Complete(); + } + + using (var scope2 = new TransactionScope()) + { + txn2 = Transaction.Current; + var owner2 = new SqlConnection(); + conn2 = GetConnection(owner2); + Assert.NotNull(conn2); + ReturnConnection(conn2, owner2); + scope2.Complete(); + } + + // Assert + // The connection was returned to the general pool and picked up by the second transaction + Assert.NotSame(txn1, txn2); + Assert.Same(conn1, conn2); + AssertPoolMetrics(); + } + + #endregion + + #region Transacted Pool Plumbing Tests + + [Fact] + public void TransactionCompletion_ReturnsConnectionToIdleChannel() + { + // Arrange - park a connection in the transacted pool. + DbConnectionInternal conn; + using (var scope = new TransactionScope()) + { + var owner = new SqlConnection(); + conn = GetConnection(owner); + Assert.NotNull(conn); + ReturnConnection(conn, owner); + + // While the transaction is live the connection is held by the transacted pool and is + // deliberately absent from the idle channel. + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); + Assert.Equal(0, _pool.IdleCount); + + scope.Complete(); + } + + // Assert - completion drives TransactionEnded -> PutObjectFromTransactedPool, which puts + // the connection back into general circulation. + Assert.Empty(_pool.TransactedConnectionPool.TransactedConnections); + Assert.Equal(1, _pool.IdleCount); + Assert.Equal(1, _pool.Count); + + // The connection is now reusable by a caller with no ambient transaction. + var owner2 = new SqlConnection(); + var conn2 = GetConnection(owner2); + Assert.Same(conn, conn2); + ReturnConnection(conn2, owner2); + } + + [Fact] + public void TransactionCompletion_AfterShutdown_DestroysConnection() + { + // Arrange - park a connection in the transacted pool, then shut the pool down. The + // transacted connection survives the shutdown drain because closing it would abort the + // (possibly distributed) transaction. + DbConnectionInternal conn; + using (var scope = new TransactionScope()) + { + var owner = new SqlConnection(); + conn = GetConnection(owner); + Assert.NotNull(conn); + ReturnConnection(conn, owner); + + // Act + _pool.Shutdown(); + scope.Complete(); + } + + // Assert - a shut-down pool must not re-pool the connection when the transaction ends. + Assert.Empty(_pool.TransactedConnectionPool.TransactedConnections); + Assert.Equal(0, _pool.IdleCount); + Assert.Equal(0, _pool.Count); + Assert.True(conn.IsConnectionDoomed); + } + + [Fact] + public void TransactionEnded_UnknownConnection_DoesNotPoolConnection() + { + // Arrange - a connection that was never parked in the transacted pool. + var owner = new SqlConnection(); + var conn = GetConnection(owner); + Assert.NotNull(conn); + + using var scope = new TransactionScope(); + var transaction = Transaction.Current; + Assert.NotNull(transaction); + + // Act + _pool.TransactionEnded(transaction!, conn); + + // Assert - nothing to remove, so the connection stays checked out. + Assert.Equal(0, _pool.IdleCount); + Assert.Equal(1, _pool.Count); + + ReturnConnection(conn, owner); + scope.Complete(); + } + + [Fact] + public void ReplaceConnection_CarriesEnlistedTransactionToNewConnection() + { + // Arrange + using var scope = new TransactionScope(); + var transaction = Transaction.Current; + Assert.NotNull(transaction); + + var owner = new SqlConnection(); + var oldConnection = GetConnection(owner); + Assert.NotNull(oldConnection); + Assert.Equal(1, _pool.Count); + + // Act + var newConnection = _pool.ReplaceConnection( + owner, + oldConnection, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15))); + + // Assert - a distinct connection took over the old connection's slot and enlistment. + Assert.NotNull(newConnection); + Assert.NotSame(oldConnection, newConnection); + Assert.Equal(1, _pool.Count); + Assert.True(oldConnection.IsConnectionDoomed); + + ReturnConnection(newConnection, owner); + + // The replacement inherited the transaction, so it parks in the transacted pool. + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections[transaction!]); + + scope.Complete(); + } + + #endregion + + #region Mock Classes + + internal class MockSqlConnectionFactory : SqlConnectionFactory + { + protected override DbConnectionInternal CreateConnection( + SqlConnectionOptions options, + ConnectionPoolKey poolKey, + DbConnectionPoolGroupProviderInfo poolGroupProviderInfo, + IDbConnectionPool pool, + DbConnection owningConnection, + TimeoutTimer timeout) + { + return new MockDbConnectionInternal(); + } + } + + internal class MockDbConnectionInternal : DbConnectionInternal + { + private static int s_nextId = 1; + public int MockId { get; } = Interlocked.Increment(ref s_nextId); + + public override string ServerVersion => "Mock"; + + public override ConnectionCapabilities Capabilities => new(); + + public override DbTransaction BeginTransaction(System.Data.IsolationLevel il) + { + throw new NotImplementedException(); + } + + public override void EnlistTransaction(Transaction? transaction) + { + if (transaction != null) + { + EnlistedTransaction = transaction; + } + } + + protected override void Activate(Transaction? transaction) + { + EnlistedTransaction = transaction; + } + + protected override void Deactivate() + { + } + + public override string ToString() => $"MockConnection_{MockId}"; + + internal override void ResetConnection() + { + } + } + + #endregion +} From 5d383c24dd8c4f4bf704905ff52b9b2581c1d755 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 29 Jul 2026 11:57:04 -0700 Subject: [PATCH 02/16] Flow the ambient transaction explicitly on the async open path The async open path ran GetInternalConnection inside a Task.Run and restored the ambient transaction by assigning Transaction.Current on that thread pool thread. That assignment writes to thread-static storage which ExecutionContext does not unwind, so the transaction outlived the open and was observable by unrelated work later scheduled onto the same thread -- including the login-time auto-enlistment that non-pooled connections perform against Transaction.Current. A try/finally restore is not sufficient either, because the continuation may resume on a different thread than the one that was polluted. Instead, capture the ambient transaction on the caller's thread (from the TaskCompletionSource's AsyncState, which is where SqlConnection.OpenAsync puts it) and thread it explicitly through GetInternalConnection into GetFromTransactedPool and PrepareConnection. The sync path passes ADP.GetCurrentTransaction() directly since it runs on the caller's thread. Also gate the transaction on HasTransactionAffinity in one place so a pool without automatic enlistment neither reads from nor writes to the transacted store. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 65 ++++++++------ .../ChannelDbConnectionPoolTransactionTest.cs | 90 +++++++++++++++++++ 2 files changed, 129 insertions(+), 26 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 3f28a2316a..7bc4b5e7cf 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -784,10 +784,12 @@ public bool TryGetConnection( // If taskCompletionSource is null, we are in a sync context. if (taskCompletionSource is null) { + // We're on the caller's thread, so the ambient transaction is directly observable. var task = GetInternalConnection( owningObject, async: false, - timeout); + timeout, + ADP.GetCurrentTransaction()); // When running synchronously, we are guaranteed that the task is already completed. // We don't need to guard the managed threadpool at this spot because we pass the async flag as false @@ -816,6 +818,22 @@ public bool TryGetConnection( // OpenAsync call. This means that we cannot cancel the connection open operation if the caller's token // is cancelled. We can only cancel based on our own timeout, which is set to the owningObject's // ConnectionTimeout. + // The ambient transaction is captured here, on the caller's thread, because + // Transaction.Current does not flow into the Task.Run below: a TransactionScope keeps + // the ambient transaction in thread-static storage unless it was created with + // TransactionScopeAsyncFlowOption.Enabled. We rely on the caller to capture the ambient + // transaction in the TaskCompletionSource's AsyncState, and then hand it to + // GetInternalConnection explicitly. + // + // Note that we deliberately do not assign Transaction.Current on the thread pool + // thread. That assignment writes to thread-static storage which is *not* unwound when + // the ExecutionContext is restored, so it would outlive this open and be observed by + // unrelated work later scheduled onto the same thread pool thread -- including the + // login-time auto-enlistment that non-pooled connections perform against + // Transaction.Current. The WaitHandle pool can get away with assigning it because it + // processes pending opens on a dedicated non-thread-pool thread. + Transaction? ambientTransaction = taskCompletionSource.Task.AsyncState as Transaction; + Task.Run(async () => { if (taskCompletionSource.Task.IsCompleted) @@ -823,11 +841,6 @@ public bool TryGetConnection( return; } - // We're potentially on a new thread, so we need to properly set the ambient transaction. - // We rely on the caller to capture the ambient transaction in the TaskCompletionSource's AsyncState - // so that we can access it here. Read: area for improvement. - ADP.SetCurrentTransaction(taskCompletionSource.Task.AsyncState as Transaction); - DbConnectionInternal? connection = null; try @@ -835,7 +848,8 @@ public bool TryGetConnection( connection = await GetInternalConnection( owningObject, async: true, - timeout + timeout, + ambientTransaction ).ConfigureAwait(false); if (!taskCompletionSource.TrySetResult(connection)) @@ -1161,6 +1175,10 @@ private void RemoveConnection(DbConnectionInternal connection) /// A boolean indicating whether the operation should be asynchronous. /// The overall timeout budget for this connection request. Time spent waiting /// in the pool is deducted from the budget available for physical connection creation. + /// The ambient transaction captured on the caller's thread, or + /// null when the caller is not inside a transaction. It is passed explicitly rather than read + /// from because this method may run on a thread pool thread + /// that the ambient transaction does not flow to. /// Returns a DbConnectionInternal that is retrieved from the pool. /// /// Thrown when an OperationCanceledException is caught, indicating that the timeout period @@ -1173,17 +1191,20 @@ private void RemoveConnection(DbConnectionInternal connection) private async Task GetInternalConnection( DbConnection owningConnection, bool async, - TimeoutTimer timeout) + TimeoutTimer timeout, + Transaction? ambientTransaction) { DbConnectionInternal? connection = null; - Transaction? transaction = null; - // If automatic transaction enlistment is enabled, we first try to get a connection that - // is already enlisted in the ambient transaction. If enlistment is not enabled we cannot - // vend connections from the transacted pool. - if (HasTransactionAffinity) + // When automatic enlistment is disabled, the connection must never be bound to the + // ambient transaction, so we neither consult the transacted store nor hand the + // transaction to activation. HasTransactionAffinity is derived from the connection + // string's Enlist keyword. + Transaction? transaction = HasTransactionAffinity ? ambientTransaction : null; + + if (transaction is not null) { - connection = GetFromTransactedPool(out transaction); + connection = GetFromTransactedPool(transaction); } // Derive a CancellationTokenSource from the TimeoutTimer so pool-internal wait operations @@ -1311,20 +1332,12 @@ private void PrepareConnection(DbConnection owningObject, DbConnectionInternal c } /// - /// Attempts to retrieve a connection that is already enlisted in the ambient transaction. + /// Attempts to retrieve a connection that is already enlisted in the given transaction. /// - /// Receives the ambient transaction, or null when there is none. - /// The caller must enlist whatever connection it ends up using in this transaction, even - /// when no transacted connection was available. - /// A live connection already enlisted in the ambient transaction, or null. - private DbConnectionInternal? GetFromTransactedPool(out Transaction? transaction) + /// The transaction the connection must already be enlisted in. + /// A live connection already enlisted in the transaction, or null. + private DbConnectionInternal? GetFromTransactedPool(Transaction transaction) { - transaction = ADP.GetCurrentTransaction(); - if (transaction is null) - { - return null; - } - DbConnectionInternal? connection = TransactedConnectionPool.GetTransactedObject(transaction); if (connection is null) { diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs index 7d2bfb95c5..53667f418d 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs @@ -1021,6 +1021,96 @@ public void ReplaceConnection_CarriesEnlistedTransactionToNewConnection() #endregion + #region Async Ambient Transaction Flow Tests + + /// + /// A created without + /// keeps the ambient transaction in thread-static storage, so it is not observable from the thread + /// pool thread the pool opens on. The pool must therefore take the transaction from the + /// 's AsyncState, which is where SqlConnection.OpenAsync + /// captures it. + /// + [Fact] + public async Task GetConnectionAsync_AmbientTransactionNotFlowed_StillEnlistsFromAsyncState() + { + // Arrange - a transaction that is never ambient on any thread, so AsyncState is the only + // way the pool can learn about it. + using var transaction = new CommittableTransaction(); + Assert.Null(Transaction.Current); + Assert.Null(await Task.Run(() => Transaction.Current)); + + // Act + var owner = new SqlConnection(); + var connection = await GetConnectionAsync(owner, transaction); + + // Assert + Assert.NotNull(connection); + Assert.Equal(transaction, connection.EnlistedTransaction); + + ReturnConnection(connection, owner); + + // Being enlisted, the connection parks in the transacted pool rather than the idle channel. + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections[transaction]); + Assert.Equal(0, _pool.IdleCount); + + transaction.Rollback(); + } + + /// + /// Assigning writes to thread-static storage that the + /// ExecutionContext does not unwind, so doing it on a thread pool thread would leave a stale + /// transaction behind for unrelated work later scheduled onto that same thread -- including the + /// login-time auto-enlistment that non-pooled connections perform against the ambient + /// transaction. The pool must pass the transaction explicitly instead of assigning it. + /// + [Fact] + public async Task GetConnectionAsync_DoesNotLeakAmbientTransactionOntoThreadPool() + { + // Arrange & Act - several async opens under a transaction, each on a thread pool thread. + for (int i = 0; i < 8; i++) + { + using var transaction = new CommittableTransaction(); + var owner = new SqlConnection(); + var connection = await GetConnectionAsync(owner, transaction); + ReturnConnection(connection, owner); + transaction.Rollback(); + } + + // Assert - no thread pool thread was left with an ambient transaction. + for (int i = 0; i < 16; i++) + { + Assert.Null(await Task.Run(() => Transaction.Current)); + } + } + + /// + /// The synchronous path runs on the caller's thread, where the ambient transaction set by a + /// TransactionScope is directly observable and must still be honored. + /// + [Fact] + public void GetConnection_Sync_UsesAmbientTransactionFromCallersThread() + { + // Arrange + using var scope = new TransactionScope(); + var transaction = Transaction.Current; + Assert.NotNull(transaction); + + // Act - no transaction is handed to the pool explicitly; it must read Transaction.Current. + var owner = new SqlConnection(); + var connection = GetConnection(owner); + + // Assert + Assert.NotNull(connection); + Assert.Equal(transaction, connection.EnlistedTransaction); + + ReturnConnection(connection, owner); + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections[transaction!]); + + scope.Complete(); + } + + #endregion + #region Mock Classes internal class MockSqlConnectionFactory : SqlConnectionFactory From b507d5cab423902939da9a791e335bd8cc25c0fd Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 29 Jul 2026 12:16:23 -0700 Subject: [PATCH 03/16] Address review feedback Source: - Flesh out the PutObjectFromTransactedPool asserts to say what invariant is being violated and why it matters. - Replace the five-way nested branch and three bool flags in ReturnInternalConnection with a ReturnDisposition enum and a single DecideReturnDisposition helper. The "shutting down", "transaction root with no pool" and "no longer poolable" cases all collapse into one reusability test followed by "stasis if it's a transaction root, otherwise destroy", which removes the need for the postcondition assert entirely. - Move the transacted-store lookup inside the acquisition loop so a connection returned to the store while we were looping is preferred over opening a fresh one. It breaks out of the loop to keep skipping the idle/generation gate, which must not apply to a transacted connection. - Document why a parked transacted connection is exempt from idle timeout and when its idle clock actually starts. Tests: - Rewrite the suite around specific behaviors: 16 focused tests replacing 33. Dropped the loop-driven stress tests (alternating commit/rollback, mixed workloads, deeply nested scopes, pool saturation, the flaky two-thread test) and the granular cases that were covered by a round trip. - Name the tests after the operation whose behavior they pin down, and document what invariant each one protects. - Assert full pool accounting (Count / IdleCount / transacted connections) at every step via AssertPoolState rather than spot-checking one collection. - Inject a frozen FakeTimeProvider so background maintenance cannot race the assertions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 212 ++-- .../ChannelDbConnectionPoolTransactionTest.cs | 1068 +++++------------ 2 files changed, 431 insertions(+), 849 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 7bc4b5e7cf..1cb15a85cd 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -322,8 +322,12 @@ public void Clear() /// public void PutObjectFromTransactedPool(DbConnectionInternal connection) { - Debug.Assert(connection is not null, "null connection?"); - Debug.Assert(connection.EnlistedTransaction is null, "connection is still enlisted?"); + Debug.Assert(connection is not null, + "PutObjectFromTransactedPool was called with a null connection."); + Debug.Assert(connection.EnlistedTransaction is null, + "PutObjectFromTransactedPool was called with a connection that is still enlisted. " + + "The transaction must have ended and been detached before the connection returns to " + + "general circulation, otherwise it could be vended to a caller in a different transaction."); // Called by the transacted connection pool once it has removed the connection from its // list. We put the connection back into general circulation. @@ -474,90 +478,110 @@ public void ReturnInternalConnection(DbConnectionInternal connection, DbConnecti return; } - bool returnToGeneralPool = false; - bool destroyConnection = false; - bool rootTxn = false; - - // A connection with a delegated transaction cannot be handed to a different customer - // until the transaction actually completes, so we send it into stasis -- the - // System.Transactions transaction object keeps it owned (not lost) and is certain to - // put it back into the pool. The decision is made under the connection lock so that a - // transaction completing asynchronously on another thread cannot race with us. + // The disposition is decided while holding the connection's lock so that a transaction + // completing asynchronously on another thread cannot race with us, but the resulting + // I/O is performed outside of it. + ReturnDisposition disposition; lock (connection) { - if (State is ShuttingDown) - { - if (connection.IsTransactionRoot) - { - // Connections affiliated with a root transaction that happen to live in a - // pool being shut down must be put in stasis so the root transaction isn't - // orphaned with no means to promote itself to a full delegated transaction - // or to Commit/Rollback. - connection.SetInStasis(); - rootTxn = true; - } - else - { - destroyConnection = true; - } - } - else if (connection.IsTransactionRoot && connection.Pool is null) - { - connection.SetInStasis(); - rootTxn = true; - } - else if (connection.CanBePooled) - { - Transaction? transaction = connection.EnlistedTransaction; - if (transaction is not null) - { - // NOTE: we're not locking on State, so its value could change between the - // conditional check above and here. Although perhaps not ideal, this is OK - // because the DelegatedTransactionEnded event will clean up the connection - // appropriately regardless of the pool state. - // - // Transacting connections are held in their own store and are never - // proactively closed (doing so would abort the transaction, which can be - // distributed). Idle-timeout enforcement does not apply here, so we do not - // stamp the returned time when parking the connection in the transacted pool. - TransactedConnectionPool.PutTransactedObject(transaction, connection); - rootTxn = true; - } - else - { - returnToGeneralPool = true; - } - } - else if (connection.IsTransactionRoot) + disposition = DecideReturnDisposition(connection); + } + + switch (disposition) + { + case ReturnDisposition.Reuse: + PutConnectionInIdleChannel(connection); + break; + + case ReturnDisposition.Destroy: + RemoveConnection(connection); + break; + + case ReturnDisposition.HeldByTransaction: + // Nothing further to do. The connection is parked in the transacted store or + // in stasis, and comes back through PutObjectFromTransactedPool once its + // transaction ends. + break; + } + } + + /// + /// Decides what should happen to a returning connection that has already been deactivated + /// and is not doomed. Must be called while holding the connection's lock: parking a + /// connection in the transacted store has to be atomic with respect to a transaction + /// completing on another thread. + /// + /// The connection being returned. + /// The action the caller must take for this connection. + private ReturnDisposition DecideReturnDisposition(DbConnectionInternal connection) + { + // A connection is only fit to go back into circulation if the pool is still running, + // the connection itself is still poolable, and it isn't a transaction root that has + // already been detached from its pool. + bool isReusable = + State is Running && + connection.CanBePooled && + !(connection.IsTransactionRoot && connection.Pool is null); + + if (!isReusable) + { + // A transaction root that cannot be pooled must be put in stasis rather than + // closed. Closing it would orphan the root transaction with no means to promote + // itself to a full delegated transaction, or to commit or roll back. + // System.Transactions keeps the connection owned (not lost) and is certain to + // hand it back to us when the transaction ends. + if (connection.IsTransactionRoot) { - // The connection cannot be pooled but is a transaction root, so we must have - // hit a race condition: either the pool was shut down or the load balancing - // timeout expired and marked the connection as non-poolable while we were - // processing within this lock. Put it in stasis so the root transaction isn't - // orphaned with no means to promote itself to a full delegated transaction or - // to Commit/Rollback. connection.SetInStasis(); - rootTxn = true; - } - else - { - destroyConnection = true; + return ReturnDisposition.HeldByTransaction; } - } - if (returnToGeneralPool) - { - Debug.Assert(!destroyConnection, "Connection cannot both be pooled and destroyed."); - PutConnectionInIdleChannel(connection); + return ReturnDisposition.Destroy; } - else if (destroyConnection) + + // A connection that is still enlisted cannot be handed to a different customer until + // its transaction actually completes, so it is parked in the transacted store keyed by + // that transaction and comes back via PutObjectFromTransactedPool when it ends. + // + // NOTE: we do not hold a lock on State, so its value could have changed since the + // check above. That is acceptable because the DelegatedTransactionEnded event cleans + // the connection up appropriately regardless of the pool state. + // + // Transacted connections are deliberately not stamped with a returned time: they are + // never proactively closed (doing so would abort a possibly distributed transaction), + // so idle-timeout enforcement does not apply while they are parked. They are stamped + // when they rejoin the idle channel in PutObjectFromTransactedPool. + Transaction? transaction = connection.EnlistedTransaction; + if (transaction is not null) { - RemoveConnection(connection); + TransactedConnectionPool.PutTransactedObject(transaction, connection); + return ReturnDisposition.HeldByTransaction; } - // Ensure the connection was processed by exactly one of the paths above. - Debug.Assert(rootTxn || returnToGeneralPool || destroyConnection, - "Returned connection was neither pooled, destroyed, nor placed in stasis."); + return ReturnDisposition.Reuse; + } + + /// + /// The outcome of evaluating a connection that is being returned to the pool. + /// + private enum ReturnDisposition + { + /// + /// The connection is fit for general reuse and belongs in the idle channel. + /// + Reuse, + + /// + /// The connection cannot be reused and must be closed. + /// + Destroy, + + /// + /// The connection is owned by a live transaction, either parked in the transacted + /// store or held in stasis, and must not be touched by the pool until that + /// transaction ends. + /// + HeldByTransaction, } /// @@ -573,6 +597,13 @@ private void PutConnectionInIdleChannel(DbConnectionInternal connection) // return even though it was actively in use on the wire. The same gating conditions are // applied here as in IsLiveConnection so we avoid the per-return timestamp read when // idle expiry is disabled or the legacy idle-timeout behavior is in effect. + // + // A connection parked in the transacted store does not pass through here, so it is not + // subject to idle timeout for as long as its transaction is live. That is intentional + // and matches WaitHandleDbConnectionPool: closing it would abort a possibly distributed + // transaction. It is stamped here when the transaction ends and + // PutObjectFromTransactedPool returns it to general circulation, so the idle clock + // starts from the moment it actually becomes available to other callers. if (!LocalAppContextSwitches.UseLegacyIdleTimeoutBehavior && PoolGroupOptions.IdleTimeout != TimeSpan.Zero) { @@ -1202,24 +1233,35 @@ private async Task GetInternalConnection( // string's Enlist keyword. Transaction? transaction = HasTransactionAffinity ? ambientTransaction : null; - if (transaction is not null) - { - connection = GetFromTransactedPool(transaction); - } - // Derive a CancellationTokenSource from the TimeoutTimer so pool-internal wait operations // (channel reads, semaphore waits) are cancelled when the overall budget expires. using CancellationTokenSource cancellationTokenSource = timeout.CreateCancellationTokenSource(); CancellationToken cancellationToken = cancellationTokenSource.Token; - // Continue looping until we create or retrieve a connection. A connection vended from - // the transacted pool skips this loop entirely: it has already been liveness-checked and - // is exempt from the idle/generation gates, because closing it would abort its - // (possibly distributed) transaction. + // Continue looping until we create or retrieve a connection. while (connection is null) { try { + // A connection already enlisted in our transaction is always preferred, since + // reusing it avoids promoting the transaction to a distributed one. This is + // re-checked on every iteration so that a connection returned to the transacted + // store while we were looping is picked up rather than being passed over in + // favor of a fresh connection. + if (transaction is not null) + { + connection = GetFromTransactedPool(transaction); + if (connection is not null) + { + // Skip the liveness/idle/generation gate at the bottom of the loop: + // GetFromTransactedPool has already probed liveness, and a transacted + // connection is exempt from idle-timeout, load-balance and + // clear-generation eviction because closing it would abort its + // (possibly distributed) transaction. + break; + } + } + // Optimistically try to get an idle connection from the channel // Doesn't wait if the channel is empty, just returns null. connection ??= GetIdleConnection(); diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs index 53667f418d..a69941bbbf 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs @@ -3,7 +3,6 @@ // See the LICENSE file in the project root for more information. using System; -using System.Data; using System.Data.Common; using System.Threading; using System.Threading.Tasks; @@ -11,14 +10,16 @@ using Microsoft.Data.Common.ConnectionString; using Microsoft.Data.ProviderBase; using Microsoft.Data.SqlClient.ConnectionPool; +using Microsoft.Extensions.Time.Testing; using Xunit; namespace Microsoft.Data.SqlClient.UnitTests.ConnectionPool; /// -/// Deterministic tests for ChannelDbConnectionPool transaction functionality. -/// These tests exercise transacted connection pathways with controlled synchronization -/// to verify correct behavior without relying on probabilistic concurrency. +/// Tests for transaction affinity: routing a returning +/// connection to the transacted store instead of the idle channel, vending an already-enlisted +/// connection back to the same transaction, releasing the connection when the transaction ends, +/// and flowing the ambient transaction on the asynchronous open path. /// public class ChannelDbConnectionPoolTransactionTest : IDisposable { @@ -26,7 +27,7 @@ public class ChannelDbConnectionPoolTransactionTest : IDisposable private const int DefaultMinPoolSize = 0; private const int DefaultCreationTimeoutInMilliseconds = 15000; - private IDbConnectionPool _pool = null!; + private IDbConnectionPool _pool; public ChannelDbConnectionPoolTransactionTest() { @@ -35,15 +36,21 @@ public ChannelDbConnectionPoolTransactionTest() public void Dispose() { - // Verify no leaked transactions before cleanup + // A transaction entry that outlives its transaction is a leak: the connections it holds + // are never returned to general circulation. Assert.Empty(_pool.TransactedConnectionPool.TransactedConnections); - _pool?.Shutdown(); - _pool?.Clear(); + _pool.Shutdown(); + _pool.Clear(); } #region Helper Methods + /// + /// Builds a pool for these tests. A frozen is injected so that + /// time-driven background maintenance (idle-timeout pruning, warmup and replenishment, + /// blocking-period expiry) cannot advance and race the assertions about pool contents. + /// private ChannelDbConnectionPool CreatePool( int maxPoolSize = DefaultMaxPoolSize, int minPoolSize = DefaultMinPoolSize, @@ -65,19 +72,33 @@ private ChannelDbConnectionPool CreatePool( poolGroupOptions ); - var connectionFactory = new MockSqlConnectionFactory(); - var pool = new ChannelDbConnectionPool( - connectionFactory, + new MockSqlConnectionFactory(), dbConnectionPoolGroup, DbConnectionPoolIdentity.NoIdentity, - new DbConnectionPoolProviderInfo() + new DbConnectionPoolProviderInfo(), + timeProvider: new FakeTimeProvider() ); pool.Startup(); return pool; } + /// + /// Tears down the pool built by the constructor and replaces it with one configured + /// differently, for the few tests that need non-default pool options. + /// + private void ReplaceFixturePool(bool hasTransactionAffinity) + { + _pool.Shutdown(); + _pool.Clear(); + _pool = CreatePool(hasTransactionAffinity: hasTransactionAffinity); + } + + /// + /// Opens a connection synchronously. The pool reads the ambient transaction off the calling + /// thread on this path. + /// private DbConnectionInternal GetConnection(SqlConnection owner) { _pool.TryGetConnection( @@ -88,6 +109,12 @@ private DbConnectionInternal GetConnection(SqlConnection owner) return connection!; } + /// + /// Opens a connection asynchronously, carrying in the + /// 's AsyncState exactly as + /// SqlConnection.InternalOpenAsync does. The pool must take the transaction from there, + /// because the open runs on a thread pool thread the ambient transaction may not flow to. + /// private async Task GetConnectionAsync( SqlConnection owner, Transaction? transaction = null) @@ -101,903 +128,417 @@ private async Task GetConnectionAsync( return connection ?? await tcs.Task; } - private void ReturnConnection(DbConnectionInternal connection, SqlConnection owner) - { + private void ReturnConnection(DbConnectionInternal connection, SqlConnection owner) => _pool.ReturnInternalConnection(connection, owner); - } - private void AssertPoolMetrics() + /// + /// Asserts the pool's accounting after a step. + /// + /// Total connections owned by the pool, checked out or not. A connection + /// parked in the transacted store still holds its pool slot and so is counted here. + /// Connections sitting in the idle channel, available to any caller. + /// Connections parked in the transacted store across all + /// transactions. These hold a pool slot but are not available to other callers. + private void AssertPoolState(int count, int idleCount, int transactedCount) { - Assert.True(_pool.Count <= _pool.PoolGroupOptions.MaxPoolSize, - $"Pool count ({_pool.Count}) exceeded max pool size ({_pool.PoolGroupOptions.MaxPoolSize})"); - Assert.True(_pool.Count >= 0, - $"Pool count ({_pool.Count}) is negative"); - Assert.Empty(_pool.TransactedConnectionPool.TransactedConnections); + Assert.Equal(count, _pool.Count); + Assert.Equal(idleCount, _pool.IdleCount); + Assert.Equal(transactedCount, TotalTransactedConnections()); } - #endregion - - #region Transaction Routing Tests - - [Fact] - public void GetConnection_UnderTransaction_RoutesToTransactedPool() + private int TotalTransactedConnections() { - // Arrange & Act - using var scope = new TransactionScope(); - var transaction = Transaction.Current; - Assert.NotNull(transaction); - - var owner = new SqlConnection(); - var conn = GetConnection(owner); - Assert.NotNull(conn); - - ReturnConnection(conn, owner); - - // Assert - connection should be in the transacted pool - Assert.True(_pool.TransactedConnectionPool.TransactedConnections.ContainsKey(transaction)); - Assert.Single(_pool.TransactedConnectionPool.TransactedConnections[transaction]); - - scope.Complete(); + int total = 0; + foreach (var entry in _pool.TransactedConnectionPool.TransactedConnections) + { + total += entry.Value.Count; + } + return total; } - [Fact] - public void GetConnection_WithoutTransaction_RoutesToGeneralPool() - { - // Arrange & Act (no TransactionScope) - var owner = new SqlConnection(); - var conn = GetConnection(owner); - Assert.NotNull(conn); + private int TransactedConnectionsFor(Transaction transaction) => + _pool.TransactedConnectionPool.TransactedConnections.TryGetValue(transaction, out var connections) + ? connections.Count + : 0; - ReturnConnection(conn, owner); + #endregion - // Assert - transacted pool should be empty - Assert.Empty(_pool.TransactedConnectionPool.TransactedConnections); - } + #region Connection Return Routing + /// + /// A connection returned while still enlisted must be parked in the transacted store rather + /// than the idle channel, so it cannot be vended to a caller in a different transaction. It + /// keeps its pool slot while parked. + /// [Fact] - public void GetConnection_UnderTransaction_ReturnsSameConnectionFromTransactedPool() + public void ReturnConnection_WhileEnlisted_ParksInTransactedStoreNotIdleChannel() { // Arrange using var scope = new TransactionScope(); + Transaction? transaction = Transaction.Current; + Assert.NotNull(transaction); - // Act - first call creates a new connection - var owner1 = new SqlConnection(); - var conn1 = GetConnection(owner1); - Assert.NotNull(conn1); - ReturnConnection(conn1, owner1); - - // Second call should retrieve the SAME connection from the transacted pool (LIFO) - var owner2 = new SqlConnection(); - var conn2 = GetConnection(owner2); - Assert.NotNull(conn2); - Assert.Same(conn1, conn2); - - ReturnConnection(conn2, owner2); - scope.Complete(); - } - - [Fact] - public async Task GetConnectionAsync_UnderTransaction_ReturnsSameConnectionFromTransactedPool() - { - // Arrange - using var scope = new TransactionScope(TransactionScopeAsyncFlowOption.Enabled); - var transaction = Transaction.Current; + var owner = new SqlConnection(); + var connection = GetConnection(owner); + Assert.NotNull(connection); + AssertPoolState(count: 1, idleCount: 0, transactedCount: 0); - // Act - first call creates a new connection - var owner1 = new SqlConnection(); - var conn1 = await GetConnectionAsync(owner1, transaction: transaction); - Assert.NotNull(conn1); - ReturnConnection(conn1, owner1); + // Act + ReturnConnection(connection, owner); - // Second call should retrieve the SAME connection from the transacted pool - var owner2 = new SqlConnection(); - var conn2 = await GetConnectionAsync(owner2, transaction: transaction); - Assert.NotNull(conn2); - Assert.Same(conn1, conn2); + // Assert + AssertPoolState(count: 1, idleCount: 0, transactedCount: 1); + Assert.Equal(1, TransactedConnectionsFor(transaction!)); - ReturnConnection(conn2, owner2); scope.Complete(); } + /// + /// Without an ambient transaction there is nothing to enlist in, so a returning connection + /// goes straight back into the idle channel. + /// [Fact] - public void GetConnection_WithTransactionAffinityDisabled_SkipsTransactedPool() + public void ReturnConnection_WithoutTransaction_ReturnsToIdleChannel() { // Arrange - _pool.Shutdown(); - _pool.Clear(); - _pool = CreatePool(hasTransactionAffinity: false); - - using var scope = new TransactionScope(); - - // Act var owner = new SqlConnection(); - var conn = GetConnection(owner); - Assert.NotNull(conn); - ReturnConnection(conn, owner); + var connection = GetConnection(owner); + Assert.NotNull(connection); + AssertPoolState(count: 1, idleCount: 0, transactedCount: 0); - // Assert - even though a transaction is active, transacted pool is not used - Assert.Empty(_pool.TransactedConnectionPool.TransactedConnections); + // Act + ReturnConnection(connection, owner); - scope.Complete(); + // Assert + AssertPoolState(count: 1, idleCount: 1, transactedCount: 0); } - #endregion - - #region Transaction Lifecycle Tests - + /// + /// The pool deactivates a returning connection before reading its enlistment, and deactivation + /// is what detaches a completed transaction. A connection returned after its transaction has + /// already ended must therefore land in the idle channel, not be parked under a dead + /// transaction where nothing would ever release it. + /// [Fact] - public void TransactionCommit_ClearsTransactedPool() + public void ReturnConnection_AfterTransactionCompleted_ReturnsToIdleChannel() { - // Arrange & Act + // Arrange + var owner = new SqlConnection(); + DbConnectionInternal connection; using (var scope = new TransactionScope()) { - var owner = new SqlConnection(); - var conn = GetConnection(owner); - Assert.NotNull(conn); - ReturnConnection(conn, owner); - - // While transaction is active, connection should be in transacted pool - Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); - + connection = GetConnection(owner); + Assert.NotNull(connection); scope.Complete(); } - // Assert - after transaction completes, transacted pool should be empty - AssertPoolMetrics(); - } - - [Fact] - public void TransactionRollback_ClearsTransactedPool() - { - // Arrange & Act - using (var scope = new TransactionScope()) - { - var owner = new SqlConnection(); - var conn = GetConnection(owner); - Assert.NotNull(conn); - ReturnConnection(conn, owner); - - Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); - - // Don't call scope.Complete() — triggers rollback - } + // Act - the transaction is fully disposed by this point. + ReturnConnection(connection, owner); - // Assert - transacted pool should be empty after rollback too - AssertPoolMetrics(); + // Assert + AssertPoolState(count: 1, idleCount: 1, transactedCount: 0); } + /// + /// A pool with automatic enlistment disabled must never bind a connection to the ambient + /// transaction, so the transacted store stays out of the picture entirely. + /// [Fact] - public void MultipleGetReturn_SameTransaction_ReusesConnection() + public void ReturnConnection_WithTransactionAffinityDisabled_ReturnsToIdleChannel() { // Arrange + ReplaceFixturePool(hasTransactionAffinity: false); + using var scope = new TransactionScope(); - var transaction = Transaction.Current; - Assert.NotNull(transaction); + var owner = new SqlConnection(); + var connection = GetConnection(owner); + Assert.NotNull(connection); - // Act - get and return multiple times within same transaction - for (int i = 0; i < 10; i++) - { - var owner = new SqlConnection(); - var conn = GetConnection(owner); - Assert.NotNull(conn); - ReturnConnection(conn, owner); - } + // Act + ReturnConnection(connection, owner); - // Assert - only one connection should be in the transacted pool - Assert.Single(_pool.TransactedConnectionPool.TransactedConnections[transaction]); + // Assert + AssertPoolState(count: 1, idleCount: 1, transactedCount: 0); scope.Complete(); } + /// + /// Returning to a pool that has already shut down destroys the connection instead of pooling + /// it, and must not throw. + /// [Fact] - public async Task MultipleGetReturn_SameTransaction_Async_ReusesConnection() + public void ReturnConnection_ToShutDownPool_DestroysConnection() { // Arrange - using var scope = new TransactionScope(TransactionScopeAsyncFlowOption.Enabled); - var transaction = Transaction.Current; - Assert.NotNull(transaction); - - // Act - get and return multiple times within same transaction - for (int i = 0; i < 10; i++) - { - var owner = new SqlConnection(); - var conn = await GetConnectionAsync(owner, transaction: transaction); - Assert.NotNull(conn); - ReturnConnection(conn, owner); - } - - // Assert - only one connection should be in the transacted pool - Assert.Single(_pool.TransactedConnectionPool.TransactedConnections[transaction]); - - scope.Complete(); - } + using var scope = new TransactionScope(); + var owner = new SqlConnection(); + var connection = GetConnection(owner); + Assert.NotNull(connection); - [Fact] - public void AlternatingCommitAndRollback_MaintainsConsistentState() - { - // Act - alternate between commit and rollback - for (int i = 0; i < 20; i++) - { - using var scope = new TransactionScope(); - var owner = new SqlConnection(); - var conn = GetConnection(owner); - Assert.NotNull(conn); - ReturnConnection(conn, owner); + _pool.Shutdown(); - if (i % 2 == 0) - { - scope.Complete(); - } - // else: rollback (no Complete) - } + // Act + ReturnConnection(connection, owner); - // Assert - AssertPoolMetrics(); + // Assert - Dispose() dooms the connection and clears its pool back-reference. + Assert.True(connection.IsConnectionDoomed, + "A connection returned to a shut-down pool should be destroyed, not pooled."); + Assert.Null(connection.Pool); + AssertPoolState(count: 0, idleCount: 0, transactedCount: 0); } #endregion - #region Nested Transaction Tests - - [Fact] - public void NestedTransaction_Required_SharesSameTransactedEntry() - { - // Arrange - using var outerScope = new TransactionScope(); - var outerTxn = Transaction.Current; - Assert.NotNull(outerTxn); - - var owner1 = new SqlConnection(); - var conn1 = GetConnection(owner1); - Assert.NotNull(conn1); - ReturnConnection(conn1, owner1); - - // Act - nested scope with Required shares the same transaction - using (var innerScope = new TransactionScope(TransactionScopeOption.Required)) - { - Assert.Same(outerTxn, Transaction.Current); - - var owner2 = new SqlConnection(); - var conn2 = GetConnection(owner2); - Assert.NotNull(conn2); - Assert.Same(conn1, conn2); // Same transaction -> same connection from transacted pool - ReturnConnection(conn2, owner2); - - // Only one transaction tracked - Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); - - innerScope.Complete(); - } - - outerScope.Complete(); - } + #region Vending From The Transacted Store + /// + /// Round trip: a second request inside the same transaction must be served the connection that + /// is already enlisted in it, rather than a fresh connection. Reusing it is what keeps the + /// transaction from being promoted to a distributed one. + /// [Fact] - public void NestedTransaction_RequiresNew_CreatesSeparateTransactedEntry() + public void GetConnection_UnderSameTransaction_VendsTheAlreadyEnlistedConnection() { // Arrange - using var outerScope = new TransactionScope(); - var outerTxn = Transaction.Current; - Assert.NotNull(outerTxn); - - var owner1 = new SqlConnection(); - var conn1 = GetConnection(owner1); - Assert.NotNull(conn1); - ReturnConnection(conn1, owner1); - - // Act - nested scope with RequiresNew creates a new transaction - using (var innerScope = new TransactionScope(TransactionScopeOption.RequiresNew)) - { - var innerTxn = Transaction.Current; - Assert.NotNull(innerTxn); - Assert.NotEqual(outerTxn, innerTxn); - - var owner2 = new SqlConnection(); - var conn2 = GetConnection(owner2); - Assert.NotNull(conn2); - Assert.NotSame(conn1, conn2); // Different transaction -> different connection - ReturnConnection(conn2, owner2); - - // Two separate transactions tracked - Assert.Equal(2, _pool.TransactedConnectionPool.TransactedConnections.Count); - - innerScope.Complete(); - } - - outerScope.Complete(); - } - - [Fact] - public void NestedTransaction_RequiresNew_CompletesIndependently() - { - // Arrange & Act - using (var outerScope = new TransactionScope()) - { - var owner1 = new SqlConnection(); - var conn1 = GetConnection(owner1); - Assert.NotNull(conn1); - ReturnConnection(conn1, owner1); - - using (var innerScope = new TransactionScope(TransactionScopeOption.RequiresNew)) - { - var owner2 = new SqlConnection(); - var conn2 = GetConnection(owner2); - Assert.NotNull(conn2); - ReturnConnection(conn2, owner2); - innerScope.Complete(); - } - - // Inner transaction completed - its entry should be cleared - // Outer transaction entry should still exist - Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); - - outerScope.Complete(); - } - - // Both completed - AssertPoolMetrics(); - } + using var scope = new TransactionScope(); + Transaction? transaction = Transaction.Current; + Assert.NotNull(transaction); - [Fact] - public void DeeplyNestedTransactions_RequiresNew_AllTrackedSeparately() - { - // Arrange & Act - using var scope1 = new TransactionScope(); var owner1 = new SqlConnection(); - var conn1 = GetConnection(owner1); - ReturnConnection(conn1, owner1); - - using var scope2 = new TransactionScope(TransactionScopeOption.RequiresNew); - var owner2 = new SqlConnection(); - var conn2 = GetConnection(owner2); - ReturnConnection(conn2, owner2); - - using var scope3 = new TransactionScope(TransactionScopeOption.RequiresNew); - var owner3 = new SqlConnection(); - var conn3 = GetConnection(owner3); - ReturnConnection(conn3, owner3); + var connection1 = GetConnection(owner1); + Assert.NotNull(connection1); + AssertPoolState(count: 1, idleCount: 0, transactedCount: 0); - // Assert - three separate transactions tracked - Assert.Equal(3, _pool.TransactedConnectionPool.TransactedConnections.Count); - - scope3.Complete(); - scope2.Complete(); - scope1.Complete(); - } - - [Fact] - public void DeeplyNestedTransactions_Required_AllShareOneEntry() - { - // Arrange & Act - using var scope1 = new TransactionScope(); - var txn = Transaction.Current; - var owner1 = new SqlConnection(); - var conn1 = GetConnection(owner1); - ReturnConnection(conn1, owner1); + ReturnConnection(connection1, owner1); + AssertPoolState(count: 1, idleCount: 0, transactedCount: 1); - using var scope2 = new TransactionScope(TransactionScopeOption.Required); - Assert.Same(txn, Transaction.Current); + // Act var owner2 = new SqlConnection(); - var conn2 = GetConnection(owner2); - Assert.Same(conn1, conn2); - ReturnConnection(conn2, owner2); - - using var scope3 = new TransactionScope(TransactionScopeOption.Required); - Assert.Same(txn, Transaction.Current); - var owner3 = new SqlConnection(); - var conn3 = GetConnection(owner3); - Assert.Same(conn1, conn3); - ReturnConnection(conn3, owner3); - - // Assert - single transaction entry - Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); - - scope3.Complete(); - scope2.Complete(); - scope1.Complete(); - } - - #endregion - - #region Mixed Transacted and Non-Transacted Tests - - [Fact] - public void MixedWorkload_AlternatingTransactedAndNonTransacted() - { - // Act - alternate between transacted and non-transacted - for (int i = 0; i < 10; i++) - { - if (i % 2 == 0) - { - using var scope = new TransactionScope(); - var owner = new SqlConnection(); - var conn = GetConnection(owner); - Assert.NotNull(conn); - ReturnConnection(conn, owner); - scope.Complete(); - } - else - { - var owner = new SqlConnection(); - var conn = GetConnection(owner); - Assert.NotNull(conn); - ReturnConnection(conn, owner); - } - } + var connection2 = GetConnection(owner2); // Assert - AssertPoolMetrics(); - } - - #endregion - - #region Shared Transaction Tests - - [Fact] - public void SharedTransaction_DependentScopes_UseTransactedPool() - { - // Arrange - using var scope = new TransactionScope(TransactionScopeAsyncFlowOption.Enabled); - var transaction = Transaction.Current; - Assert.NotNull(transaction); - - // Act - first connection - var owner1 = new SqlConnection(); - var conn1 = GetConnection(owner1); - Assert.NotNull(conn1); - ReturnConnection(conn1, owner1); + Assert.Same(connection1, connection2); + AssertPoolState(count: 1, idleCount: 0, transactedCount: 0); - // Use dependent scope on same transaction - using (var innerScope = new TransactionScope(transaction)) - { - Assert.Same(transaction, Transaction.Current); - var owner2 = new SqlConnection(); - var conn2 = GetConnection(owner2); - Assert.NotNull(conn2); - Assert.Same(conn1, conn2); // Same transaction -> same connection - ReturnConnection(conn2, owner2); - innerScope.Complete(); - } - - // Assert - still one transaction entry - Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); - Assert.Single(_pool.TransactedConnectionPool.TransactedConnections[transaction]); + ReturnConnection(connection2, owner2); + AssertPoolState(count: 1, idleCount: 0, transactedCount: 1); + Assert.Equal(1, TransactedConnectionsFor(transaction!)); scope.Complete(); } - #endregion - - #region Pool Saturation with Transactions Tests - - [Fact] - public void PoolSaturation_BlocksUntilConnectionAvailable() - { - // Arrange - small pool - _pool.Shutdown(); - _pool.Clear(); - _pool = CreatePool(maxPoolSize: 1); - - using var allAcquired = new ManualResetEventSlim(false); - using var releaseFirst = new ManualResetEventSlim(false); - - var saturatingTask = Task.Run(() => - { - using var scope = new TransactionScope(); - var owner = new SqlConnection(); - var conn = GetConnection(owner); - Assert.NotNull(conn); - - allAcquired.Set(); // Signal that this connection is held - - Assert.True(releaseFirst.Wait(TimeSpan.FromSeconds(15)), - "Timed out waiting for releaseFirst signal."); - - ReturnConnection(conn, owner); - scope.Complete(); - }); - - Assert.True(allAcquired.Wait(TimeSpan.FromSeconds(10)), - "Timed out waiting for connection to be acquired."); - Assert.Equal(1, _pool.Count); - - using var acquired = new ManualResetEventSlim(false); - var waitingTask = Task.Run(() => - { - using var scope = new TransactionScope(); - var owner = new SqlConnection(); - var conn = GetConnection(owner); - Assert.NotNull(conn); - acquired.Set(); - ReturnConnection(conn, owner); - scope.Complete(); - }); - - // Give the waiting task time to block — it should NOT complete yet - Assert.False(acquired.Wait(TimeSpan.FromMilliseconds(500)), - "Waiting task should not have acquired a connection while pool is saturated"); - - // Release one connection to unblock the waiting task - releaseFirst.Set(); - - // Now the waiting task should complete - Assert.True(waitingTask.Wait(TimeSpan.FromSeconds(15)), - "Waiting task should have completed after a connection was released"); - Assert.True(acquired.IsSet); - - // Cleanup remaining held connections - Task.WaitAll(saturatingTask); - } - - #endregion - - #region Controlled Concurrency Tests - - // Flaky under CI load only (never reproduces locally): the two worker tasks are - // scheduled via Task.Run on the thread pool. On a loaded agent the pool can be slow to - // spin up a worker, so task1 starts late and fails to signal task1Returned within - // task2's 10s wait, producing a WaitAll timeout. That is thread-pool starvation, not a - // pool/transaction defect. - [Trait("Category", "flaky")] + /// + /// The asynchronous path must reuse the enlisted connection exactly as the synchronous path + /// does, even though it runs the open on a thread pool thread. + /// [Fact] - public void TwoThreads_SharedTransaction_AccessSameTransactedEntry() + public async Task GetConnectionAsync_UnderSameTransaction_VendsTheAlreadyEnlistedConnection() { // Arrange - // Use 3-phase synchronization so task1 gets AND returns before task2 requests. - // This ensures the connection is back in the transacted pool for task2 to reuse. - using var task1Returned = new ManualResetEventSlim(false); - using var task2Done = new ManualResetEventSlim(false); - DbConnectionInternal? connFromTask1 = null; - DbConnectionInternal? connFromTask2 = null; - using var scope = new TransactionScope(TransactionScopeAsyncFlowOption.Enabled); - var transaction = Transaction.Current; + Transaction? transaction = Transaction.Current; Assert.NotNull(transaction); - // Act - two threads sharing the same transaction, sequenced so the - // transacted pool can vend the same connection to both. - var task1 = Task.Run(() => - { - using var innerScope = new TransactionScope(transaction); - var owner = new SqlConnection(); - connFromTask1 = GetConnection(owner); - Assert.NotNull(connFromTask1); - - // Return the connection so it's available in the transacted pool - ReturnConnection(connFromTask1, owner); - innerScope.Complete(); - - task1Returned.Set(); // Signal: connection is back in the transacted pool - }); + var owner1 = new SqlConnection(); + var connection1 = await GetConnectionAsync(owner1, transaction); + Assert.NotNull(connection1); + ReturnConnection(connection1, owner1); + AssertPoolState(count: 1, idleCount: 0, transactedCount: 1); - var task2 = Task.Run(() => - { - // Wait until task1 has returned the connection to the transacted pool - Assert.True(task1Returned.Wait(TimeSpan.FromSeconds(10)), - "Timed out waiting for task1 to return its connection."); + // Act + var owner2 = new SqlConnection(); + var connection2 = await GetConnectionAsync(owner2, transaction); - using var innerScope = new TransactionScope(transaction); - var owner = new SqlConnection(); - connFromTask2 = GetConnection(owner); - Assert.NotNull(connFromTask2); - ReturnConnection(connFromTask2, owner); - innerScope.Complete(); - }); + // Assert + Assert.Same(connection1, connection2); + AssertPoolState(count: 1, idleCount: 0, transactedCount: 0); - Task.WaitAll(task1, task2); + ReturnConnection(connection2, owner2); + AssertPoolState(count: 1, idleCount: 0, transactedCount: 1); - // Both tasks should have received the same connection via the transacted pool - Assert.Same(connFromTask1, connFromTask2); scope.Complete(); } + /// + /// A connection enlisted in one transaction must never be handed to a caller in a different + /// transaction; each transaction gets its own entry in the transacted store. + /// [Fact] - public async Task TwoThreads_SeparateTransactions_Async_IsolatedTransactedEntries() + public void GetConnection_UnderDifferentTransaction_DoesNotVendTheEnlistedConnection() { // Arrange - using var barrier = new SemaphoreSlim(0, 2); - - // Act - var task1 = Task.Run(async () => - { - using var scope = new TransactionScope(TransactionScopeAsyncFlowOption.Enabled); - var transaction = Transaction.Current; - var owner = new SqlConnection(); - var conn = await GetConnectionAsync(owner, transaction: transaction); - Assert.NotNull(conn); - ReturnConnection(conn, owner); - - barrier.Release(); // Signal ready - await barrier.WaitAsync(); // Wait for other task + using var outerScope = new TransactionScope(); + Transaction? outerTransaction = Transaction.Current; + Assert.NotNull(outerTransaction); - scope.Complete(); - }); + var owner1 = new SqlConnection(); + var connection1 = GetConnection(owner1); + ReturnConnection(connection1, owner1); + AssertPoolState(count: 1, idleCount: 0, transactedCount: 1); - var task2 = Task.Run(async () => + // Act - RequiresNew starts an unrelated transaction. + using (var innerScope = new TransactionScope(TransactionScopeOption.RequiresNew)) { - using var scope = new TransactionScope(TransactionScopeAsyncFlowOption.Enabled); - var transaction = Transaction.Current; - var owner = new SqlConnection(); - var conn = await GetConnectionAsync(owner, transaction: transaction); - Assert.NotNull(conn); - ReturnConnection(conn, owner); - - barrier.Release(); // Signal ready - await barrier.WaitAsync(); // Wait for other task - - scope.Complete(); - }); + Transaction? innerTransaction = Transaction.Current; + Assert.NotEqual(outerTransaction, innerTransaction); - await Task.WhenAll(task1, task2); - - // Assert - AssertPoolMetrics(); - } + var owner2 = new SqlConnection(); + var connection2 = GetConnection(owner2); - #endregion + // Assert - a fresh connection, because connection1 belongs to the outer transaction. + Assert.NotSame(connection1, connection2); + AssertPoolState(count: 2, idleCount: 0, transactedCount: 1); - #region Pool Shutdown with Transactions Tests + ReturnConnection(connection2, owner2); + AssertPoolState(count: 2, idleCount: 0, transactedCount: 2); + Assert.Equal(1, TransactedConnectionsFor(outerTransaction!)); + Assert.Equal(1, TransactedConnectionsFor(innerTransaction!)); - [Fact] - public void PoolShutdown_AfterTransactionComplete_NoLeaks() - { - // Arrange - using (var scope = new TransactionScope()) - { - var owner = new SqlConnection(); - var conn = GetConnection(owner); - Assert.NotNull(conn); - ReturnConnection(conn, owner); - scope.Complete(); + innerScope.Complete(); } - // Act - _pool.Shutdown(); + // Completing the inner transaction releases only its connection; the outer transaction's + // connection stays parked. + AssertPoolState(count: 2, idleCount: 1, transactedCount: 1); + Assert.Equal(1, TransactedConnectionsFor(outerTransaction!)); - // Assert - AssertPoolMetrics(); - } - - [Fact] - public void PoolShutdown_WhileConnectionHeld_NoException() - { - // Arrange - using var scope = new TransactionScope(); - var owner = new SqlConnection(); - var conn = GetConnection(owner); - Assert.NotNull(conn); - - // Act - shutdown while connection is held (not yet returned) - _pool.Shutdown(); - - // Return after shutdown — the pool deactivates and disposes the connection - // rather than returning it to the pool. Verify this doesn't throw. - ReturnConnection(conn, owner); - - // Assert - // The connection should have been deactivated and disposed (not returned to the pool). - // After Dispose(), IsConnectionDoomed is set to true and Pool is set to null. - Assert.True(conn.IsConnectionDoomed, - "Connection should be doomed after returning to a shut-down pool."); - Assert.Null(conn.Pool); + outerScope.Complete(); } #endregion - #region Transaction Complete Before Return Tests + #region Transaction Completion + /// + /// Committing releases the parked connection back into the idle channel, where it becomes + /// available to any caller. + /// [Fact] - public void TransactionComplete_ThenReturn_ConnectionStillReturned() + public void TransactionCommit_ReturnsParkedConnectionToIdleChannel() { // Arrange - var owner = new SqlConnection(); - DbConnectionInternal conn; - + DbConnectionInternal connection; using (var scope = new TransactionScope()) { - conn = GetConnection(owner); - Assert.NotNull(conn); - scope.Complete(); - } - // Transaction is fully disposed here - - // Act - return connection after transaction ended - ReturnConnection(conn, owner); - - // Assert - no leak, pool metrics consistent - AssertPoolMetrics(); - Assert.True(_pool.Count > 0, "Pool should still have the connection"); - } - - #endregion - - #region Sequential Transaction Isolation Tests - - [Fact] - public void SequentialTransactions_EachGetsOwnTransactedEntry() - { - // Act - create multiple sequential transactions - for (int i = 0; i < 5; i++) - { - using var scope = new TransactionScope(); - var transaction = Transaction.Current; - Assert.NotNull(transaction); - var owner = new SqlConnection(); - var conn = GetConnection(owner); - Assert.NotNull(conn); - ReturnConnection(conn, owner); - - // Only the current transaction should be tracked - Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); - Assert.True(_pool.TransactedConnectionPool.TransactedConnections.ContainsKey(transaction)); - - scope.Complete(); - } - - // Assert - after all are done, pool should be clean - AssertPoolMetrics(); - } - - [Fact] - public async Task SequentialTransactions_Async_EachGetsOwnTransactedEntry() - { - // Act - create multiple sequential transactions - for (int i = 0; i < 5; i++) - { - using var scope = new TransactionScope(TransactionScopeAsyncFlowOption.Enabled); - var transaction = Transaction.Current; - Assert.NotNull(transaction); - - var owner = new SqlConnection(); - var conn = await GetConnectionAsync(owner, transaction: transaction); - Assert.NotNull(conn); - ReturnConnection(conn, owner); - - Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); - Assert.True(_pool.TransactedConnectionPool.TransactedConnections.ContainsKey(transaction)); + connection = GetConnection(owner); + ReturnConnection(connection, owner); + AssertPoolState(count: 1, idleCount: 0, transactedCount: 1); + // Act scope.Complete(); } // Assert - AssertPoolMetrics(); - } + AssertPoolState(count: 1, idleCount: 1, transactedCount: 0); - [Fact] - public void SequentialTransactions_CanReuseConnections() - { - // Act - DbConnectionInternal conn1; - DbConnectionInternal conn2; - Transaction? txn1; - Transaction? txn2; - - using (var scope1 = new TransactionScope()) - { - txn1 = Transaction.Current; - var owner1 = new SqlConnection(); - conn1 = GetConnection(owner1); - Assert.NotNull(conn1); - ReturnConnection(conn1, owner1); - scope1.Complete(); - } - - using (var scope2 = new TransactionScope()) - { - txn2 = Transaction.Current; - var owner2 = new SqlConnection(); - conn2 = GetConnection(owner2); - Assert.NotNull(conn2); - ReturnConnection(conn2, owner2); - scope2.Complete(); - } - - // Assert - // The connection was returned to the general pool and picked up by the second transaction - Assert.NotSame(txn1, txn2); - Assert.Same(conn1, conn2); - AssertPoolMetrics(); + // The released connection is reusable by a caller with no ambient transaction. + var owner2 = new SqlConnection(); + var connection2 = GetConnection(owner2); + Assert.Same(connection, connection2); + ReturnConnection(connection2, owner2); } - #endregion - - #region Transacted Pool Plumbing Tests - + /// + /// Rolling back must release the parked connection just as committing does; otherwise an + /// aborted transaction would strand its connection in the transacted store forever. + /// [Fact] - public void TransactionCompletion_ReturnsConnectionToIdleChannel() + public void TransactionRollback_ReturnsParkedConnectionToIdleChannel() { - // Arrange - park a connection in the transacted pool. - DbConnectionInternal conn; - using (var scope = new TransactionScope()) + // Arrange + using (new TransactionScope()) { var owner = new SqlConnection(); - conn = GetConnection(owner); - Assert.NotNull(conn); - ReturnConnection(conn, owner); - - // While the transaction is live the connection is held by the transacted pool and is - // deliberately absent from the idle channel. - Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); - Assert.Equal(0, _pool.IdleCount); + var connection = GetConnection(owner); + ReturnConnection(connection, owner); + AssertPoolState(count: 1, idleCount: 0, transactedCount: 1); - scope.Complete(); + // Act - leaving the scope without calling Complete rolls the transaction back. } - // Assert - completion drives TransactionEnded -> PutObjectFromTransactedPool, which puts - // the connection back into general circulation. - Assert.Empty(_pool.TransactedConnectionPool.TransactedConnections); - Assert.Equal(1, _pool.IdleCount); - Assert.Equal(1, _pool.Count); - - // The connection is now reusable by a caller with no ambient transaction. - var owner2 = new SqlConnection(); - var conn2 = GetConnection(owner2); - Assert.Same(conn, conn2); - ReturnConnection(conn2, owner2); + // Assert + AssertPoolState(count: 1, idleCount: 1, transactedCount: 0); } + /// + /// A connection parked in the transacted store survives pool shutdown, because closing it + /// would abort a possibly distributed transaction. Once that transaction ends, the shut-down + /// pool must destroy the connection rather than return it to circulation. + /// [Fact] public void TransactionCompletion_AfterShutdown_DestroysConnection() { - // Arrange - park a connection in the transacted pool, then shut the pool down. The - // transacted connection survives the shutdown drain because closing it would abort the - // (possibly distributed) transaction. - DbConnectionInternal conn; + // Arrange + DbConnectionInternal connection; using (var scope = new TransactionScope()) { var owner = new SqlConnection(); - conn = GetConnection(owner); - Assert.NotNull(conn); - ReturnConnection(conn, owner); + connection = GetConnection(owner); + ReturnConnection(connection, owner); + AssertPoolState(count: 1, idleCount: 0, transactedCount: 1); - // Act + // Act - the shutdown drain must leave the transacted connection alone. _pool.Shutdown(); + AssertPoolState(count: 1, idleCount: 0, transactedCount: 1); + scope.Complete(); } - // Assert - a shut-down pool must not re-pool the connection when the transaction ends. - Assert.Empty(_pool.TransactedConnectionPool.TransactedConnections); - Assert.Equal(0, _pool.IdleCount); - Assert.Equal(0, _pool.Count); - Assert.True(conn.IsConnectionDoomed); + // Assert + AssertPoolState(count: 0, idleCount: 0, transactedCount: 0); + Assert.True(connection.IsConnectionDoomed, + "A shut-down pool must destroy a connection released by its transaction."); } + /// + /// TransactionEnded for a connection that was never parked must be a no-op. It must not push a + /// connection that is still checked out into the idle channel, where a second caller could + /// pick it up while the first is still using it. + /// [Fact] - public void TransactionEnded_UnknownConnection_DoesNotPoolConnection() + public void TransactionEnded_ForConnectionThatWasNeverParked_LeavesItCheckedOut() { - // Arrange - a connection that was never parked in the transacted pool. + // Arrange var owner = new SqlConnection(); - var conn = GetConnection(owner); - Assert.NotNull(conn); + var connection = GetConnection(owner); + Assert.NotNull(connection); using var scope = new TransactionScope(); - var transaction = Transaction.Current; + Transaction? transaction = Transaction.Current; Assert.NotNull(transaction); // Act - _pool.TransactionEnded(transaction!, conn); + _pool.TransactionEnded(transaction!, connection); - // Assert - nothing to remove, so the connection stays checked out. - Assert.Equal(0, _pool.IdleCount); - Assert.Equal(1, _pool.Count); + // Assert + AssertPoolState(count: 1, idleCount: 0, transactedCount: 0); - ReturnConnection(conn, owner); + ReturnConnection(connection, owner); scope.Complete(); } + #endregion + + #region Connection Replacement + + /// + /// Replacing a connection mid-transaction must carry the enlistment across, so the replacement + /// is the one that parks in the transacted store when it is returned. + /// [Fact] public void ReplaceConnection_CarriesEnlistedTransactionToNewConnection() { // Arrange using var scope = new TransactionScope(); - var transaction = Transaction.Current; + Transaction? transaction = Transaction.Current; Assert.NotNull(transaction); var owner = new SqlConnection(); var oldConnection = GetConnection(owner); Assert.NotNull(oldConnection); - Assert.Equal(1, _pool.Count); + AssertPoolState(count: 1, idleCount: 0, transactedCount: 0); // Act var newConnection = _pool.ReplaceConnection( @@ -1005,36 +546,36 @@ public void ReplaceConnection_CarriesEnlistedTransactionToNewConnection() oldConnection, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15))); - // Assert - a distinct connection took over the old connection's slot and enlistment. + // Assert - a distinct connection took over the old connection's slot. Assert.NotNull(newConnection); Assert.NotSame(oldConnection, newConnection); - Assert.Equal(1, _pool.Count); Assert.True(oldConnection.IsConnectionDoomed); + AssertPoolState(count: 1, idleCount: 0, transactedCount: 0); ReturnConnection(newConnection, owner); - // The replacement inherited the transaction, so it parks in the transacted pool. - Assert.Single(_pool.TransactedConnectionPool.TransactedConnections[transaction!]); + // The replacement inherited the transaction, so it parks in the transacted store. + AssertPoolState(count: 1, idleCount: 0, transactedCount: 1); + Assert.Equal(1, TransactedConnectionsFor(transaction!)); scope.Complete(); } #endregion - #region Async Ambient Transaction Flow Tests + #region Async Ambient Transaction Flow /// /// A created without - /// keeps the ambient transaction in thread-static storage, so it is not observable from the thread - /// pool thread the pool opens on. The pool must therefore take the transaction from the - /// 's AsyncState, which is where SqlConnection.OpenAsync - /// captures it. + /// keeps the ambient transaction in thread-static storage, so it is not observable from the + /// thread pool thread the pool opens on. The pool must therefore take the transaction from the + /// 's AsyncState. This test uses a transaction that + /// is never ambient anywhere, so AsyncState is the only way the pool can learn about it. /// [Fact] public async Task GetConnectionAsync_AmbientTransactionNotFlowed_StillEnlistsFromAsyncState() { - // Arrange - a transaction that is never ambient on any thread, so AsyncState is the only - // way the pool can learn about it. + // Arrange using var transaction = new CommittableTransaction(); Assert.Null(Transaction.Current); Assert.Null(await Task.Run(() => Transaction.Current)); @@ -1048,10 +589,8 @@ public async Task GetConnectionAsync_AmbientTransactionNotFlowed_StillEnlistsFro Assert.Equal(transaction, connection.EnlistedTransaction); ReturnConnection(connection, owner); - - // Being enlisted, the connection parks in the transacted pool rather than the idle channel. - Assert.Single(_pool.TransactedConnectionPool.TransactedConnections[transaction]); - Assert.Equal(0, _pool.IdleCount); + AssertPoolState(count: 1, idleCount: 0, transactedCount: 1); + Assert.Equal(1, TransactedConnectionsFor(transaction)); transaction.Rollback(); } @@ -1059,8 +598,8 @@ public async Task GetConnectionAsync_AmbientTransactionNotFlowed_StillEnlistsFro /// /// Assigning writes to thread-static storage that the /// ExecutionContext does not unwind, so doing it on a thread pool thread would leave a stale - /// transaction behind for unrelated work later scheduled onto that same thread -- including the - /// login-time auto-enlistment that non-pooled connections perform against the ambient + /// transaction behind for unrelated work later scheduled onto that same thread -- including + /// the login-time auto-enlistment that non-pooled connections perform against the ambient /// transaction. The pool must pass the transaction explicitly instead of assigning it. /// [Fact] @@ -1085,17 +624,18 @@ public async Task GetConnectionAsync_DoesNotLeakAmbientTransactionOntoThreadPool /// /// The synchronous path runs on the caller's thread, where the ambient transaction set by a - /// TransactionScope is directly observable and must still be honored. + /// TransactionScope is directly observable and must still be honored even though no + /// transaction is handed to the pool explicitly. /// [Fact] public void GetConnection_Sync_UsesAmbientTransactionFromCallersThread() { // Arrange using var scope = new TransactionScope(); - var transaction = Transaction.Current; + Transaction? transaction = Transaction.Current; Assert.NotNull(transaction); - // Act - no transaction is handed to the pool explicitly; it must read Transaction.Current. + // Act var owner = new SqlConnection(); var connection = GetConnection(owner); @@ -1104,7 +644,7 @@ public void GetConnection_Sync_UsesAmbientTransactionFromCallersThread() Assert.Equal(transaction, connection.EnlistedTransaction); ReturnConnection(connection, owner); - Assert.Single(_pool.TransactedConnectionPool.TransactedConnections[transaction!]); + AssertPoolState(count: 1, idleCount: 0, transactedCount: 1); scope.Complete(); } From b49adf407ba703368a32c25a52a170b482ca1939 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 3 Aug 2026 15:56:37 -0700 Subject: [PATCH 04/16] Inline the return disposition decision and unify the transacted liveness probe Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 127 +++++++----------- 1 file changed, 49 insertions(+), 78 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 1cb15a85cd..5d51bdb57b 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -478,13 +478,46 @@ public void ReturnInternalConnection(DbConnectionInternal connection, DbConnecti return; } - // The disposition is decided while holding the connection's lock so that a transaction - // completing asynchronously on another thread cannot race with us, but the resulting - // I/O is performed outside of it. + // Note: this logic mirrors WaitHandleDbConnectionPool.ReturnObject ReturnDisposition disposition; lock (connection) { - disposition = DecideReturnDisposition(connection); + if (State is not Running || !connection.CanBePooled) + { + // A transaction root that cannot be pooled must be put in stasis rather than + // closed. Closing it would orphan the root transaction with no means to promote + // itself to a full delegated transaction, or to commit or roll back. + // System.Transactions keeps the connection owned (not lost) and is certain to + // call the appropriate callback when the transaction ends. + if (connection.IsTransactionRoot) + { + connection.SetInStasis(); + disposition = ReturnDisposition.HeldByTransaction; + } + else + { + disposition = ReturnDisposition.Destroy; + } + } + else if (connection.EnlistedTransaction is { } transaction) + { + // A connection that is still enlisted cannot be handed to a different customer + // until its transaction actually completes, so it is parked in the transacted + // store keyed by that transaction and comes back via + // PutObjectFromTransactedPool when it ends. + // + // Transacted connections are deliberately not stamped with a returned time: + // they are never proactively closed (doing so would abort a possibly + // distributed transaction), so idle-timeout enforcement does not apply while + // they are parked. They are stamped when they rejoin the idle channel in + // PutObjectFromTransactedPool. + TransactedConnectionPool.PutTransactedObject(transaction, connection); + disposition = ReturnDisposition.HeldByTransaction; + } + else + { + disposition = ReturnDisposition.Reuse; + } } switch (disposition) @@ -505,62 +538,6 @@ public void ReturnInternalConnection(DbConnectionInternal connection, DbConnecti } } - /// - /// Decides what should happen to a returning connection that has already been deactivated - /// and is not doomed. Must be called while holding the connection's lock: parking a - /// connection in the transacted store has to be atomic with respect to a transaction - /// completing on another thread. - /// - /// The connection being returned. - /// The action the caller must take for this connection. - private ReturnDisposition DecideReturnDisposition(DbConnectionInternal connection) - { - // A connection is only fit to go back into circulation if the pool is still running, - // the connection itself is still poolable, and it isn't a transaction root that has - // already been detached from its pool. - bool isReusable = - State is Running && - connection.CanBePooled && - !(connection.IsTransactionRoot && connection.Pool is null); - - if (!isReusable) - { - // A transaction root that cannot be pooled must be put in stasis rather than - // closed. Closing it would orphan the root transaction with no means to promote - // itself to a full delegated transaction, or to commit or roll back. - // System.Transactions keeps the connection owned (not lost) and is certain to - // hand it back to us when the transaction ends. - if (connection.IsTransactionRoot) - { - connection.SetInStasis(); - return ReturnDisposition.HeldByTransaction; - } - - return ReturnDisposition.Destroy; - } - - // A connection that is still enlisted cannot be handed to a different customer until - // its transaction actually completes, so it is parked in the transacted store keyed by - // that transaction and comes back via PutObjectFromTransactedPool when it ends. - // - // NOTE: we do not hold a lock on State, so its value could have changed since the - // check above. That is acceptable because the DelegatedTransactionEnded event cleans - // the connection up appropriately regardless of the pool state. - // - // Transacted connections are deliberately not stamped with a returned time: they are - // never proactively closed (doing so would abort a possibly distributed transaction), - // so idle-timeout enforcement does not apply while they are parked. They are stamped - // when they rejoin the idle channel in PutObjectFromTransactedPool. - Transaction? transaction = connection.EnlistedTransaction; - if (transaction is not null) - { - TransactedConnectionPool.PutTransactedObject(transaction, connection); - return ReturnDisposition.HeldByTransaction; - } - - return ReturnDisposition.Reuse; - } - /// /// The outcome of evaluating a connection that is being returned to the pool. /// @@ -1396,33 +1373,27 @@ private void PrepareConnection(DbConnection owningObject, DbConnectionInternal c // Transacting connections are exempt from idle-timeout and clear-generation eviction // (closing them would abort the transaction, which may be distributed), so only // liveness is checked here rather than the full IsLiveConnection gate. - if (connection.IsTransactionRoot) + bool isAlive = false; + try { - try - { - // A dead transaction root must surface the underlying failure to the caller: - // there is no way to recover the delegated transaction on another connection. - connection.IsConnectionAlive(throwOnException: true); - } - catch + // A dead transaction root must surface the underlying failure to the caller, since + // there is no way to recover the delegated transaction on another connection. Any + // other dead connection is simply reported so the caller can pick up or open a + // different one. Either way the connection is dropped in the finally below. + isAlive = connection.IsConnectionAlive(throwOnException: connection.IsTransactionRoot); + } + finally + { + if (!isAlive) { SqlClientEventSource.Log.TryPoolerTraceEvent( " {0}, Connection {1}, found dead and removed.", Id, connection.ObjectID); RemoveConnection(connection); - throw; + connection = null; } } - else if (!connection.IsConnectionAlive()) - { - SqlClientEventSource.Log.TryPoolerTraceEvent( - " {0}, Connection {1}, found dead and removed.", - Id, - connection.ObjectID); - RemoveConnection(connection); - connection = null; - } return connection; } From 23edff0533148347cebfc6cb1ffe00ce57cf3ad8 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 3 Aug 2026 16:05:08 -0700 Subject: [PATCH 05/16] Honor a flowed ambient transaction on the async open path The async path only read the transaction out of the TaskCompletionSource's AsyncState, so a caller whose ambient transaction did flow (a TransactionScope created with TransactionScopeAsyncFlowOption.Enabled) but who supplied no AsyncState got no enlistment at all, while the same caller on the sync path did. Fall back to ADP.GetCurrentTransaction(), still read on the caller's thread before the open is scheduled, so both paths agree on the caller's transaction. AsyncState keeps priority because SqlConnection.OpenAsync captures it at the point of the call, which stays correct when a retry re-enters from a continuation on another thread. Test the ambient-transaction leak directly instead of inferring it from thread pool reuse: the mock connection factory runs on exactly the thread the pool does its open work on, so it now records that thread's id and ambient transaction. The test asserts the open really happened off the calling thread and that the pool left that thread's Transaction.Current null while still enlisting the connection. Both this and the new async ambient test were verified to fail when the corresponding defect is reintroduced. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 13 ++- .../ChannelDbConnectionPoolTransactionTest.cs | 92 +++++++++++++++---- 2 files changed, 85 insertions(+), 20 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 5d51bdb57b..0bfbd3b241 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -829,9 +829,13 @@ public bool TryGetConnection( // The ambient transaction is captured here, on the caller's thread, because // Transaction.Current does not flow into the Task.Run below: a TransactionScope keeps // the ambient transaction in thread-static storage unless it was created with - // TransactionScopeAsyncFlowOption.Enabled. We rely on the caller to capture the ambient - // transaction in the TaskCompletionSource's AsyncState, and then hand it to - // GetInternalConnection explicitly. + // TransactionScopeAsyncFlowOption.Enabled. + // + // The TaskCompletionSource's AsyncState takes priority: SqlConnection.OpenAsync + // captures the ambient transaction there at the point of the call, which stays correct + // even when a retry re-enters us from a continuation running on some other thread. + // We fall back to the caller's own ambient transaction so that this path behaves the + // same as the synchronous one for callers that don't supply the state. // // Note that we deliberately do not assign Transaction.Current on the thread pool // thread. That assignment writes to thread-static storage which is *not* unwound when @@ -840,7 +844,8 @@ public bool TryGetConnection( // login-time auto-enlistment that non-pooled connections perform against // Transaction.Current. The WaitHandle pool can get away with assigning it because it // processes pending opens on a dedicated non-thread-pool thread. - Transaction? ambientTransaction = taskCompletionSource.Task.AsyncState as Transaction; + Transaction? ambientTransaction = + taskCompletionSource.Task.AsyncState as Transaction ?? ADP.GetCurrentTransaction(); Task.Run(async () => { diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs index a69941bbbf..f41ddab46b 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs @@ -28,6 +28,7 @@ public class ChannelDbConnectionPoolTransactionTest : IDisposable private const int DefaultCreationTimeoutInMilliseconds = 15000; private IDbConnectionPool _pool; + private MockSqlConnectionFactory _connectionFactory = null!; public ChannelDbConnectionPoolTransactionTest() { @@ -72,8 +73,10 @@ private ChannelDbConnectionPool CreatePool( poolGroupOptions ); + _connectionFactory = new MockSqlConnectionFactory(); + var pool = new ChannelDbConnectionPool( - new MockSqlConnectionFactory(), + _connectionFactory, dbConnectionPoolGroup, DbConnectionPoolIdentity.NoIdentity, new DbConnectionPoolProviderInfo(), @@ -601,25 +604,33 @@ public async Task GetConnectionAsync_AmbientTransactionNotFlowed_StillEnlistsFro /// transaction behind for unrelated work later scheduled onto that same thread -- including /// the login-time auto-enlistment that non-pooled connections perform against the ambient /// transaction. The pool must pass the transaction explicitly instead of assigning it. + /// + /// The connection factory runs on exactly the thread the pool does its open work on, so it is + /// used here to observe that thread's ambient transaction directly rather than inferring the + /// leak from thread pool reuse. /// [Fact] - public async Task GetConnectionAsync_DoesNotLeakAmbientTransactionOntoThreadPool() + public async Task GetConnectionAsync_DoesNotSetAmbientTransactionOnPoolWorkerThread() { - // Arrange & Act - several async opens under a transaction, each on a thread pool thread. - for (int i = 0; i < 8; i++) - { - using var transaction = new CommittableTransaction(); - var owner = new SqlConnection(); - var connection = await GetConnectionAsync(owner, transaction); - ReturnConnection(connection, owner); - transaction.Rollback(); - } + // Arrange - a transaction the pool must enlist in but must not make ambient. + using var transaction = new CommittableTransaction(); + Assert.Null(Transaction.Current); - // Assert - no thread pool thread was left with an ambient transaction. - for (int i = 0; i < 16; i++) - { - Assert.Null(await Task.Run(() => Transaction.Current)); - } + // Act + var owner = new SqlConnection(); + var connection = await GetConnectionAsync(owner, transaction); + + // Assert - the open really did happen off the calling thread, and the pool left that + // thread's ambient transaction alone while still enlisting the connection. + Assert.Equal(1, _connectionFactory.CreateCount); + Assert.NotEqual(Environment.CurrentManagedThreadId, _connectionFactory.CreateThreadId); + Assert.Null(_connectionFactory.AmbientTransactionAtCreate); + Assert.Equal(transaction, connection.EnlistedTransaction); + + ReturnConnection(connection, owner); + AssertPoolState(count: 1, idleCount: 0, transactedCount: 1); + + transaction.Rollback(); } /// @@ -649,12 +660,58 @@ public void GetConnection_Sync_UsesAmbientTransactionFromCallersThread() scope.Complete(); } + /// + /// The asynchronous equivalent of the case above: when the ambient transaction does flow to + /// the caller (a TransactionScope created with + /// ) but the caller supplies no + /// transaction in the TaskCompletionSource's AsyncState, the pool must still pick it up. The + /// capture happens on the caller's thread before the open is scheduled, so the two paths agree + /// on what "the caller's transaction" means. + /// + [Fact] + public async Task GetConnectionAsync_WithoutAsyncState_UsesAmbientTransactionFromCallersThread() + { + // Arrange + using var scope = new TransactionScope(TransactionScopeAsyncFlowOption.Enabled); + Transaction? transaction = Transaction.Current; + Assert.NotNull(transaction); + + // Act - note that no transaction is passed, so AsyncState is null. + var owner = new SqlConnection(); + var connection = await GetConnectionAsync(owner, transaction: null); + + // Assert + Assert.NotNull(connection); + Assert.Equal(transaction, connection.EnlistedTransaction); + + ReturnConnection(connection, owner); + AssertPoolState(count: 1, idleCount: 0, transactedCount: 1); + Assert.Equal(1, TransactedConnectionsFor(transaction!)); + + scope.Complete(); + } + #endregion #region Mock Classes internal class MockSqlConnectionFactory : SqlConnectionFactory { + /// + /// The value of observed on the thread the pool used to + /// create the connection. On the asynchronous path that is the thread pool thread the pool + /// runs its open work on, so this is a direct observation of whether the pool assigned the + /// ambient transaction there. + /// + public Transaction? AmbientTransactionAtCreate { get; private set; } + + /// + /// The managed thread the pool created the connection on. + /// + public int CreateThreadId { get; private set; } + + public int CreateCount { get; private set; } + protected override DbConnectionInternal CreateConnection( SqlConnectionOptions options, ConnectionPoolKey poolKey, @@ -663,6 +720,9 @@ protected override DbConnectionInternal CreateConnection( DbConnection owningConnection, TimeoutTimer timeout) { + AmbientTransactionAtCreate = Transaction.Current; + CreateThreadId = Environment.CurrentManagedThreadId; + CreateCount++; return new MockDbConnectionInternal(); } } From 7e1264cdf6e422d52389069e688eeb2e89abc753 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 3 Aug 2026 16:23:19 -0700 Subject: [PATCH 06/16] Take the async transaction only from AsyncState Reverts the ADP.GetCurrentTransaction() fallback added in 41a689d9. It was both unreachable and unsafe: - SqlConnection.InternalOpenAsync is the only site repo-wide that constructs a TaskCompletionSource, and it always captures the ambient transaction into AsyncState. OpenAsyncRetry.Retry re-enters TryOpen with that same TCS, so AsyncState survives retries. The fallback could never fire in production. - Reading Transaction.Current here is thread-sensitive in exactly the way the rest of this change set set out to eliminate: on a retry we are re-entered from a continuation on an arbitrary thread, so we could enlist in an unrelated ambient transaction. The async equivalent of the sync ambient-transaction test now exercises the real mechanism instead: the GetConnectionAsync helper mirrors InternalOpenAsync by capturing the caller's ambient transaction into AsyncState, and the test asserts enlistment from inside a TransactionScope. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 26 +++++++-------- .../ChannelDbConnectionPoolTransactionTest.cs | 32 +++++++++++-------- 2 files changed, 29 insertions(+), 29 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 0bfbd3b241..cff9051725 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -826,26 +826,22 @@ public bool TryGetConnection( // OpenAsync call. This means that we cannot cancel the connection open operation if the caller's token // is cancelled. We can only cancel based on our own timeout, which is set to the owningObject's // ConnectionTimeout. - // The ambient transaction is captured here, on the caller's thread, because - // Transaction.Current does not flow into the Task.Run below: a TransactionScope keeps - // the ambient transaction in thread-static storage unless it was created with - // TransactionScopeAsyncFlowOption.Enabled. - // - // The TaskCompletionSource's AsyncState takes priority: SqlConnection.OpenAsync - // captures the ambient transaction there at the point of the call, which stays correct - // even when a retry re-enters us from a continuation running on some other thread. - // We fall back to the caller's own ambient transaction so that this path behaves the - // same as the synchronous one for callers that don't supply the state. + // The ambient transaction is captured by the caller, on the caller's thread, and handed + // to us in the TaskCompletionSource's AsyncState (see SqlConnection.InternalOpenAsync). + // We must not read Transaction.Current ourselves here: a TransactionScope keeps the + // ambient transaction in thread-static storage unless it was created with + // TransactionScopeAsyncFlowOption.Enabled, and on a retry we are re-entered from a + // continuation running on an arbitrary thread, so Transaction.Current at this point + // says nothing reliable about the caller's transaction. // // Note that we deliberately do not assign Transaction.Current on the thread pool - // thread. That assignment writes to thread-static storage which is *not* unwound when - // the ExecutionContext is restored, so it would outlive this open and be observed by - // unrelated work later scheduled onto the same thread pool thread -- including the + // thread either. That assignment writes to thread-static storage which is *not* unwound + // when the ExecutionContext is restored, so it would outlive this open and be observed + // by unrelated work later scheduled onto the same thread pool thread -- including the // login-time auto-enlistment that non-pooled connections perform against // Transaction.Current. The WaitHandle pool can get away with assigning it because it // processes pending opens on a dedicated non-thread-pool thread. - Transaction? ambientTransaction = - taskCompletionSource.Task.AsyncState as Transaction ?? ADP.GetCurrentTransaction(); + Transaction? ambientTransaction = taskCompletionSource.Task.AsyncState as Transaction; Task.Run(async () => { diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs index f41ddab46b..20ad9eeb73 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs @@ -113,16 +113,20 @@ private DbConnectionInternal GetConnection(SqlConnection owner) } /// - /// Opens a connection asynchronously, carrying in the - /// 's AsyncState exactly as - /// SqlConnection.InternalOpenAsync does. The pool must take the transaction from there, - /// because the open runs on a thread pool thread the ambient transaction may not flow to. + /// Opens a connection asynchronously the way SqlConnection.InternalOpenAsync does: the ambient + /// transaction is captured here, on the caller's thread, and handed to the pool in the + /// 's AsyncState. The pool must take it from there, + /// because the open itself runs on a thread pool thread the ambient transaction may not flow + /// to, and on a retry the pool is re-entered from a continuation on an arbitrary thread. /// + /// The owning connection. + /// The transaction to hand to the pool. Defaults to the caller's + /// ambient transaction, matching what InternalOpenAsync captures. private async Task GetConnectionAsync( SqlConnection owner, Transaction? transaction = null) { - var tcs = new TaskCompletionSource(transaction); + var tcs = new TaskCompletionSource(transaction ?? Transaction.Current); _pool.TryGetConnection( owner, taskCompletionSource: tcs, @@ -661,24 +665,24 @@ public void GetConnection_Sync_UsesAmbientTransactionFromCallersThread() } /// - /// The asynchronous equivalent of the case above: when the ambient transaction does flow to - /// the caller (a TransactionScope created with - /// ) but the caller supplies no - /// transaction in the TaskCompletionSource's AsyncState, the pool must still pick it up. The - /// capture happens on the caller's thread before the open is scheduled, so the two paths agree - /// on what "the caller's transaction" means. + /// The asynchronous equivalent of the case above. The pool cannot read the caller's ambient + /// transaction itself, because the open runs on a thread pool thread it does not flow to, so + /// the caller captures it and passes it in AsyncState. Even with a scope created with + /// , where the transaction is genuinely + /// ambient on the calling thread, that capture is what has to carry it through. /// [Fact] - public async Task GetConnectionAsync_WithoutAsyncState_UsesAmbientTransactionFromCallersThread() + public async Task GetConnectionAsync_UsesAmbientTransactionCapturedOnCallersThread() { // Arrange using var scope = new TransactionScope(TransactionScopeAsyncFlowOption.Enabled); Transaction? transaction = Transaction.Current; Assert.NotNull(transaction); - // Act - note that no transaction is passed, so AsyncState is null. + // Act - no transaction is passed explicitly, so the helper captures the ambient one on + // this thread exactly as SqlConnection.InternalOpenAsync does. var owner = new SqlConnection(); - var connection = await GetConnectionAsync(owner, transaction: null); + var connection = await GetConnectionAsync(owner); // Assert Assert.NotNull(connection); From d7ea9d355b21406cf956d197e8a2e14dfc469ad0 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 3 Aug 2026 16:37:40 -0700 Subject: [PATCH 07/16] Cover the suppressed-async-flow ambient transaction case Adds GetConnectionAsync_WithAsyncFlowDisabled_StillEnlistsInAmbientTransaction: a TransactionScope created with the default TransactionScopeAsyncFlowOption .Suppress keeps its transaction in thread-static storage, so it is ambient on the caller's thread but does not flow to the thread pool thread the pool opens on. The connection must still enlist. The open is started inside the scope and awaited outside it, because a suppressed scope must be disposed on the thread that created it; an explicit CommittableTransaction keeps the transaction alive past the scope so the pool is still enlisting in a live transaction. Also restores the retry-path coverage as GetConnectionAsync_EnteredFromThreadWithoutAmbientTransaction_EnlistsFromAsyncState. This is not redundant: TryGetConnection reads AsyncState while still on the caller's thread, so in the scope-based tests Transaction.Current happens to agree with AsyncState. Only entering the pool from a thread with no ambient transaction -- as OpenAsyncRetry.Retry does -- distinguishes the two. Verified by mutation: replacing the AsyncState read with null fails 5 tests, and replacing it with ADP.GetCurrentTransaction() fails only the retry-path and no-leak tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ChannelDbConnectionPoolTransactionTest.cs | 74 ++++++++++++++++--- 1 file changed, 62 insertions(+), 12 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs index 20ad9eeb73..6524b21698 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs @@ -573,23 +573,42 @@ public void ReplaceConnection_CarriesEnlistedTransactionToNewConnection() #region Async Ambient Transaction Flow /// - /// A created without - /// keeps the ambient transaction in thread-static storage, so it is not observable from the - /// thread pool thread the pool opens on. The pool must therefore take the transaction from the - /// 's AsyncState. This test uses a transaction that - /// is never ambient anywhere, so AsyncState is the only way the pool can learn about it. + /// The case the AsyncState mechanism exists for. A created + /// without -- the default -- keeps its + /// ambient transaction in thread-static storage, so it is ambient on the calling thread but + /// does not flow across an await onto the thread pool thread the pool opens on. The connection + /// must still enlist, because the transaction is captured on the caller's thread before the + /// open is scheduled (see SqlConnection.InternalOpenAsync). + /// + /// The open is started inside the scope but awaited outside it. That is not incidental: when + /// async flow is suppressed the continuation may resume on another thread, and a + /// TransactionScope must be disposed on the thread that created it. The scope is given an + /// explicit so that leaving the scope ends the scope + /// without ending the transaction the pool is still enlisting in. /// [Fact] - public async Task GetConnectionAsync_AmbientTransactionNotFlowed_StillEnlistsFromAsyncState() + public async Task GetConnectionAsync_WithAsyncFlowDisabled_StillEnlistsInAmbientTransaction() { // Arrange using var transaction = new CommittableTransaction(); - Assert.Null(Transaction.Current); - Assert.Null(await Task.Run(() => Transaction.Current)); - - // Act var owner = new SqlConnection(); - var connection = await GetConnectionAsync(owner, transaction); + Task openTask; + + using (var scope = new TransactionScope(transaction)) + { + Assert.Equal(transaction, Transaction.Current); + + // The transaction really is confined to this thread, so AsyncState is the only way + // the pool can learn about it. + Assert.Null(Task.Run(() => Transaction.Current).GetAwaiter().GetResult()); + + // Act - starting the open captures the ambient transaction synchronously, here, and + // hands the rest of the work to a thread pool thread. + openTask = GetConnectionAsync(owner); + scope.Complete(); + } + + var connection = await openTask; // Assert Assert.NotNull(connection); @@ -603,7 +622,38 @@ public async Task GetConnectionAsync_AmbientTransactionNotFlowed_StillEnlistsFro } /// - /// Assigning writes to thread-static storage that the + /// The pool must take the transaction from AsyncState rather than from whatever is ambient on + /// the thread that happens to enter it. TryGetConnection is normally called synchronously from + /// the caller's thread, where the two agree -- but on the retry path + /// (SqlConnection.OpenAsyncRetry.Retry) the pool is re-entered from a continuation running on + /// an arbitrary thread, which has no ambient transaction. Reading Transaction.Current there + /// would silently drop the enlistment, or worse, pick up an unrelated transaction. + /// + [Fact] + public async Task GetConnectionAsync_EnteredFromThreadWithoutAmbientTransaction_EnlistsFromAsyncState() + { + // Arrange + using var transaction = new CommittableTransaction(); + var owner = new SqlConnection(); + + // Act - enter the pool from a thread pool thread with no ambient transaction, the way a + // retry continuation does, carrying the transaction only in AsyncState. + var connection = await Task.Run(async () => + { + Assert.Null(Transaction.Current); + return await GetConnectionAsync(owner, transaction); + }); + + // Assert + Assert.NotNull(connection); + Assert.Equal(transaction, connection.EnlistedTransaction); + + ReturnConnection(connection, owner); + AssertPoolState(count: 1, idleCount: 0, transactedCount: 1); + Assert.Equal(1, TransactedConnectionsFor(transaction)); + + transaction.Rollback(); + } /// ExecutionContext does not unwind, so doing it on a thread pool thread would leave a stale /// transaction behind for unrelated work later scheduled onto that same thread -- including /// the login-time auto-enlistment that non-pooled connections perform against the ambient From a5bc5cb132b3dc76b738e09a01f00c92475dbac3 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 4 Aug 2026 15:17:25 -0700 Subject: [PATCH 08/16] Fix CS8602 nullability errors under net462 The net462 build failed with three CS8602 errors. Debug.Assert's condition parameter carries [DoesNotReturnIf(false)] on net8.0/net9.0, so the compiler treats the null branch as unreachable, but the net462 reference assembly has no such annotation. There, null-testing an already non-nullable parameter downgrades it to maybe-null for the rest of the method, so the following dereference warns. ChannelDbConnectionPool.cs is #nullable enable and both parameters are declared non-nullable, so these asserts were redundant -- the compiler already enforces the contract at every call site. WaitHandleDbConnectionPool keeps its equivalent asserts because that file is nullable-oblivious. The substantive assert, that a connection returning from the transacted pool is no longer enlisted, is retained. Verified by reproducing the failure locally with -p:TargetOs=Windows_NT -f net462, then confirming net462 builds clean in both Debug and Release, and all three Windows TFMs build via the CI command. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index cff9051725..46491c6071 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -322,8 +322,6 @@ public void Clear() /// public void PutObjectFromTransactedPool(DbConnectionInternal connection) { - Debug.Assert(connection is not null, - "PutObjectFromTransactedPool was called with a null connection."); Debug.Assert(connection.EnlistedTransaction is null, "PutObjectFromTransactedPool was called with a connection that is still enlisted. " + "The transaction must have ended and been detached before the connection returns to " + @@ -752,9 +750,6 @@ public void Startup() /// public void TransactionEnded(Transaction transaction, DbConnectionInternal transactedObject) { - Debug.Assert(transaction is not null, "null transaction?"); - Debug.Assert(transactedObject is not null, "null transactedObject?"); - // Note: the connection may still be associated with the transaction due to the explicit // unbinding requirement. SqlClientEventSource.Log.TryPoolerTraceEvent( From df2405b0ad61a3ac296c44ce117987af27f1c316 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 5 Aug 2026 10:13:28 -0700 Subject: [PATCH 09/16] Fix two net462-only transaction test failures GetConnectionAsync_WithAsyncFlowDisabled_StillEnlistsInAmbientTransaction asserted that the ambient transaction does not flow into Task.Run. .NET Framework flows it through ExecutionContext regardless of the async flow option, so that sanity check only holds on .NET. It is not the behavior under test, so guard it. GetConnectionAsync_DoesNotSetAmbientTransactionOnPoolWorkerThread compared the thread id after the await against the factory's thread. The continuation can resume on the pool's own worker thread, and xunit runs tests on thread pool threads, so the two can legitimately match. Assert that the open ran on a thread pool thread instead, which is the property that makes assigning Transaction.Current there unsafe. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ChannelDbConnectionPoolTransactionTest.cs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs index 6524b21698..41e89b1415 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs @@ -598,9 +598,12 @@ public async Task GetConnectionAsync_WithAsyncFlowDisabled_StillEnlistsInAmbient { Assert.Equal(transaction, Transaction.Current); +#if NET // The transaction really is confined to this thread, so AsyncState is the only way - // the pool can learn about it. + // the pool can learn about it. .NET Framework flows the ambient transaction through + // ExecutionContext whatever the async flow option says, so this only holds on .NET. Assert.Null(Task.Run(() => Transaction.Current).GetAwaiter().GetResult()); +#endif // Act - starting the open captures the ambient transaction synchronously, here, and // hands the rest of the work to a thread pool thread. @@ -674,10 +677,10 @@ public async Task GetConnectionAsync_DoesNotSetAmbientTransactionOnPoolWorkerThr var owner = new SqlConnection(); var connection = await GetConnectionAsync(owner, transaction); - // Assert - the open really did happen off the calling thread, and the pool left that + // Assert - the open really did happen on a thread pool thread, and the pool left that // thread's ambient transaction alone while still enlisting the connection. Assert.Equal(1, _connectionFactory.CreateCount); - Assert.NotEqual(Environment.CurrentManagedThreadId, _connectionFactory.CreateThreadId); + Assert.True(_connectionFactory.CreatedOnThreadPoolThread); Assert.Null(_connectionFactory.AmbientTransactionAtCreate); Assert.Equal(transaction, connection.EnlistedTransaction); @@ -760,9 +763,10 @@ internal class MockSqlConnectionFactory : SqlConnectionFactory public Transaction? AmbientTransactionAtCreate { get; private set; } /// - /// The managed thread the pool created the connection on. + /// Whether the pool created the connection on a thread pool thread, which is what makes + /// assigning the ambient transaction there unsafe. /// - public int CreateThreadId { get; private set; } + public bool CreatedOnThreadPoolThread { get; private set; } public int CreateCount { get; private set; } @@ -775,7 +779,7 @@ protected override DbConnectionInternal CreateConnection( TimeoutTimer timeout) { AmbientTransactionAtCreate = Transaction.Current; - CreateThreadId = Environment.CurrentManagedThreadId; + CreatedOnThreadPoolThread = Thread.CurrentThread.IsThreadPoolThread; CreateCount++; return new MockDbConnectionInternal(); } From 0bf4111e34554e282f36ce3b12324c95f1f96244 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 5 Aug 2026 10:25:30 -0700 Subject: [PATCH 10/16] Probe ambient transaction confinement on a dedicated thread The previous commit blamed the net462 failure on ExecutionContext flowing the ambient transaction. That is wrong: with the default async flow option the transaction lives in [ThreadStatic] storage, which ExecutionContext does not carry, so it cannot flow into Task.Run on either runtime. The real cause is task inlining. Task.InternalWait attempts to inline on an infinite, non-cancelable wait, and ThreadPoolTaskScheduler.TryExecuteTaskInline pops the queued work item and runs it on the waiting thread. The blocking GetAwaiter().GetResult() therefore ran the probe on the transaction-owning thread, whose thread-local storage of course still held the transaction. Both runtimes have that path, so the check was a scheduling race, not a .NET-only invariant. Probe on a dedicated thread instead. It cannot be inlined, so the assertion is deterministic and meaningful on every target framework. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ChannelDbConnectionPoolTransactionTest.cs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs index 41e89b1415..a4560fbe58 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs @@ -598,12 +598,15 @@ public async Task GetConnectionAsync_WithAsyncFlowDisabled_StillEnlistsInAmbient { Assert.Equal(transaction, Transaction.Current); -#if NET // The transaction really is confined to this thread, so AsyncState is the only way - // the pool can learn about it. .NET Framework flows the ambient transaction through - // ExecutionContext whatever the async flow option says, so this only holds on .NET. - Assert.Null(Task.Run(() => Transaction.Current).GetAwaiter().GetResult()); -#endif + // the pool can learn about it. Probed on a dedicated thread rather than with + // Task.Run: blocking on a queued task lets the runtime execute it inline on this + // thread, which would observe this thread's own ambient transaction and prove nothing. + Transaction? observedOnAnotherThread = null; + var probe = new Thread(() => observedOnAnotherThread = Transaction.Current); + probe.Start(); + probe.Join(); + Assert.Null(observedOnAnotherThread); // Act - starting the open captures the ambient transaction synchronously, here, and // hands the rest of the work to a thread pool thread. From cbda33ed652d7e480caedff2b9557759a15fb4ca Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 5 Aug 2026 10:35:36 -0700 Subject: [PATCH 11/16] Drop the ambient transaction confinement probe The probe asserted that a suppressed-flow TransactionScope does not make its transaction visible on another thread. That is System.Transactions' documented contract for the option, not pool behavior, so the suite gains nothing by re-verifying it. It also implied a discrimination this test does not make. The pool reads AsyncState on the caller's thread, where the ambient transaction agrees with it, so this test passes either way. The test that separates the two channels is GetConnectionAsync_EnteredFromThreadWithoutAmbientTransaction_EnlistsFromAsyncState. Point the comment at it rather than overclaiming. Also record why the open is awaited outside the scope: awaiting inside resumes on a thread pool thread and disposing the scope there throws. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ChannelDbConnectionPoolTransactionTest.cs | 37 ++++++++----------- 1 file changed, 16 insertions(+), 21 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs index a4560fbe58..705da1e407 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs @@ -573,18 +573,23 @@ public void ReplaceConnection_CarriesEnlistedTransactionToNewConnection() #region Async Ambient Transaction Flow /// - /// The case the AsyncState mechanism exists for. A created - /// without -- the default -- keeps its - /// ambient transaction in thread-static storage, so it is ambient on the calling thread but - /// does not flow across an await onto the thread pool thread the pool opens on. The connection - /// must still enlist, because the transaction is captured on the caller's thread before the - /// open is scheduled (see SqlConnection.InternalOpenAsync). + /// A created without + /// -- the default -- keeps its ambient + /// transaction in thread-static storage, so it does not flow across an await onto the thread + /// pool thread the pool opens on. The connection must still enlist, because the transaction is + /// captured on the caller's thread before the open is scheduled (see + /// SqlConnection.InternalOpenAsync). This covers that end to end; it does not distinguish + /// AsyncState from the caller's ambient transaction, which agree here. See + /// + /// for the case that separates them. /// - /// The open is started inside the scope but awaited outside it. That is not incidental: when - /// async flow is suppressed the continuation may resume on another thread, and a - /// TransactionScope must be disposed on the thread that created it. The scope is given an - /// explicit so that leaving the scope ends the scope - /// without ending the transaction the pool is still enlisting in. + /// The open is started inside the scope but awaited outside it. That is not incidental: + /// awaiting inside resumes the continuation on a thread pool thread, and disposing the scope + /// there throws "A TransactionScope must be disposed on the same thread that it was created." + /// That is the very limitation TransactionScopeAsyncFlowOption.Enabled exists to remove, so a + /// test of the suppressed case cannot avoid it. The scope is given an explicit + /// so that leaving the scope ends the scope without + /// ending the transaction the pool is still enlisting in. /// [Fact] public async Task GetConnectionAsync_WithAsyncFlowDisabled_StillEnlistsInAmbientTransaction() @@ -598,16 +603,6 @@ public async Task GetConnectionAsync_WithAsyncFlowDisabled_StillEnlistsInAmbient { Assert.Equal(transaction, Transaction.Current); - // The transaction really is confined to this thread, so AsyncState is the only way - // the pool can learn about it. Probed on a dedicated thread rather than with - // Task.Run: blocking on a queued task lets the runtime execute it inline on this - // thread, which would observe this thread's own ambient transaction and prove nothing. - Transaction? observedOnAnotherThread = null; - var probe = new Thread(() => observedOnAnotherThread = Transaction.Current); - probe.Start(); - probe.Join(); - Assert.Null(observedOnAnotherThread); - // Act - starting the open captures the ambient transaction synchronously, here, and // hands the rest of the work to a thread pool thread. openTask = GetConnectionAsync(owner); From 55182613bf1c917c043a66ac2ce4e48c1d2d79ad Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 5 Aug 2026 10:47:10 -0700 Subject: [PATCH 12/16] Record why the async open captures the transaction on the caller's thread With TransactionScopeAsyncFlowOption.Enabled the ambient transaction rides an AsyncLocal and is live on the pool's worker thread, so reading Transaction.Current inside the Task.Run would appear to work. Enabled is not the default, though: a plain TransactionScope keeps the transaction in thread-static storage, which does not flow, and the worker would silently fail to enlist. The WaitHandle pool enlists correctly there, so capturing on the caller's thread is a compatibility requirement as well as a safety net for apps that forget the option. It keeps the connection in the intended transaction rather than letting it run outside one; it does not make the suppressed-flow pattern otherwise work. Verified by mutation: switching the worker to Transaction.Current fails exactly three tests, which the test comment now names. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 18 +++++++++++++----- .../ChannelDbConnectionPoolTransactionTest.cs | 16 ++++++++++------ 2 files changed, 23 insertions(+), 11 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 46491c6071..1443700f23 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -823,11 +823,19 @@ public bool TryGetConnection( // ConnectionTimeout. // The ambient transaction is captured by the caller, on the caller's thread, and handed // to us in the TaskCompletionSource's AsyncState (see SqlConnection.InternalOpenAsync). - // We must not read Transaction.Current ourselves here: a TransactionScope keeps the - // ambient transaction in thread-static storage unless it was created with - // TransactionScopeAsyncFlowOption.Enabled, and on a retry we are re-entered from a - // continuation running on an arbitrary thread, so Transaction.Current at this point - // says nothing reliable about the caller's transaction. + // + // We must not read Transaction.Current inside the Task.Run below instead. A + // TransactionScope created with TransactionScopeAsyncFlowOption.Enabled stores the + // transaction in an AsyncLocal, which does flow onto the pool's worker thread, so that + // would appear to work. But Enabled is not the default: a plain TransactionScope keeps + // the transaction in thread-static storage, which does not flow, and reading + // Transaction.Current on the worker would silently fail to enlist. The WaitHandle pool + // enlists correctly in that case, so this is also a compatibility requirement. + // AsyncState is correct under both options. + // + // This does not make the suppressed-flow pattern work -- the caller's own scope is + // still broken past the first await -- but it keeps the connection in the transaction + // the caller intended rather than silently running outside it. // // Note that we deliberately do not assign Transaction.Current on the thread pool // thread either. That assignment writes to thread-static storage which is *not* unwound diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs index 705da1e407..ef9fab42d2 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs @@ -575,13 +575,17 @@ public void ReplaceConnection_CarriesEnlistedTransactionToNewConnection() /// /// A created without /// -- the default -- keeps its ambient - /// transaction in thread-static storage, so it does not flow across an await onto the thread - /// pool thread the pool opens on. The connection must still enlist, because the transaction is - /// captured on the caller's thread before the open is scheduled (see - /// SqlConnection.InternalOpenAsync). This covers that end to end; it does not distinguish - /// AsyncState from the caller's ambient transaction, which agree here. See + /// transaction in thread-static storage, so it does not flow onto the thread pool thread the + /// pool opens on. The connection must still enlist, because the transaction is captured on the + /// caller's thread before the open is scheduled (see SqlConnection.InternalOpenAsync). + /// + /// This is one of three guards on that capture. With Enabled the transaction rides an + /// AsyncLocal and is live on the pool's worker thread, so reading Transaction.Current there + /// would pass every other test in this class -- and silently stop enlisting for the default + /// option, which the WaitHandle pool handles correctly. This case is the realistic shape of + /// that regression; see also /// - /// for the case that separates them. + /// and . /// /// The open is started inside the scope but awaited outside it. That is not incidental: /// awaiting inside resumes the continuation on a thread pool thread, and disposing the scope From 1d7836d8c93cc591d71898decca5d9f76178e96d Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 5 Aug 2026 10:54:49 -0700 Subject: [PATCH 13/16] Drop thread inspection from the ambient transaction leak test The thread assertion was not load-bearing. Mutation testing confirms the leak is caught by the ambient transaction assertion alone: reinstating ADP.SetCurrentTransaction on the pool's worker fails this test at that assert, and nothing else in the class notices. Observing the transaction the factory sees is an observation of the code under test, not of whichever thread happened to run it, so it does not depend on thread identity or scheduling the way an id comparison or an IsThreadPoolThread check does. Also restore the summary opening on this test's doc comment, which had been truncated. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ChannelDbConnectionPoolTransactionTest.cs | 31 +++++++++---------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs index ef9fab42d2..7037880985 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs @@ -659,14 +659,19 @@ public async Task GetConnectionAsync_EnteredFromThreadWithoutAmbientTransaction_ transaction.Rollback(); } - /// ExecutionContext does not unwind, so doing it on a thread pool thread would leave a stale - /// transaction behind for unrelated work later scheduled onto that same thread -- including - /// the login-time auto-enlistment that non-pooled connections perform against the ambient - /// transaction. The pool must pass the transaction explicitly instead of assigning it. + + /// + /// The pool must enlist the connection in the caller's transaction without making that + /// transaction ambient on the thread it opens on. Assigning Transaction.Current writes to + /// thread-static storage that ExecutionContext does not unwind, so doing it on a thread pool + /// thread would leave a stale transaction behind for unrelated work later scheduled onto that + /// same thread -- including the login-time auto-enlistment that non-pooled connections perform + /// against the ambient transaction. The pool must pass the transaction explicitly instead of + /// assigning it. /// - /// The connection factory runs on exactly the thread the pool does its open work on, so it is - /// used here to observe that thread's ambient transaction directly rather than inferring the - /// leak from thread pool reuse. + /// The connection factory runs inside the pool's own open work, so it observes what the pool + /// made ambient there. That is an observation of the code under test rather than of whichever + /// thread happened to run it, so it does not depend on thread identity or scheduling. /// [Fact] public async Task GetConnectionAsync_DoesNotSetAmbientTransactionOnPoolWorkerThread() @@ -679,10 +684,9 @@ public async Task GetConnectionAsync_DoesNotSetAmbientTransactionOnPoolWorkerThr var owner = new SqlConnection(); var connection = await GetConnectionAsync(owner, transaction); - // Assert - the open really did happen on a thread pool thread, and the pool left that - // thread's ambient transaction alone while still enlisting the connection. + // Assert - the pool left the worker's ambient transaction alone while still enlisting the + // connection. Assert.Equal(1, _connectionFactory.CreateCount); - Assert.True(_connectionFactory.CreatedOnThreadPoolThread); Assert.Null(_connectionFactory.AmbientTransactionAtCreate); Assert.Equal(transaction, connection.EnlistedTransaction); @@ -764,12 +768,6 @@ internal class MockSqlConnectionFactory : SqlConnectionFactory /// public Transaction? AmbientTransactionAtCreate { get; private set; } - /// - /// Whether the pool created the connection on a thread pool thread, which is what makes - /// assigning the ambient transaction there unsafe. - /// - public bool CreatedOnThreadPoolThread { get; private set; } - public int CreateCount { get; private set; } protected override DbConnectionInternal CreateConnection( @@ -781,7 +779,6 @@ protected override DbConnectionInternal CreateConnection( TimeoutTimer timeout) { AmbientTransactionAtCreate = Transaction.Current; - CreatedOnThreadPoolThread = Thread.CurrentThread.IsThreadPoolThread; CreateCount++; return new MockDbConnectionInternal(); } From c2dafb75a99c7eca90ffe38d7035ffffa73d538c Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 5 Aug 2026 10:58:11 -0700 Subject: [PATCH 14/16] Drop the redundant thread hop from the AsyncState enlistment test The test opens no TransactionScope, so its own thread already has no ambient transaction. Wrapping the open in Task.Run added a thread hop without changing what the test discriminates. Verified: mutating the pool to read Transaction.Current on the worker still fails this test. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ChannelDbConnectionPoolTransactionTest.cs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs index 7037880985..fdaa1f4552 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs @@ -633,6 +633,10 @@ public async Task GetConnectionAsync_WithAsyncFlowDisabled_StillEnlistsInAmbient /// (SqlConnection.OpenAsyncRetry.Retry) the pool is re-entered from a continuation running on /// an arbitrary thread, which has no ambient transaction. Reading Transaction.Current there /// would silently drop the enlistment, or worse, pick up an unrelated transaction. + /// + /// No TransactionScope is opened here, so this thread already stands in for that continuation: + /// the transaction exists but is not ambient anywhere, and AsyncState is the only channel + /// carrying it. /// [Fact] public async Task GetConnectionAsync_EnteredFromThreadWithoutAmbientTransaction_EnlistsFromAsyncState() @@ -640,14 +644,10 @@ public async Task GetConnectionAsync_EnteredFromThreadWithoutAmbientTransaction_ // Arrange using var transaction = new CommittableTransaction(); var owner = new SqlConnection(); + Assert.Null(Transaction.Current); - // Act - enter the pool from a thread pool thread with no ambient transaction, the way a - // retry continuation does, carrying the transaction only in AsyncState. - var connection = await Task.Run(async () => - { - Assert.Null(Transaction.Current); - return await GetConnectionAsync(owner, transaction); - }); + // Act - carry the transaction only in AsyncState, the way a retry continuation does. + var connection = await GetConnectionAsync(owner, transaction); // Assert Assert.NotNull(connection); From 9f97de6736b60b39db11250c07fa04a81e5a8575 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 5 Aug 2026 12:47:39 -0700 Subject: [PATCH 15/16] Address review feedback on transaction logging and docs - Move the PutObjectFromTransactedPool trace into the return/destroy branches so each logs what actually happened. - Include the transaction identifier in the GetFromTransactedPool trace. - Explain why TransactionEnded does not call PutObjectFromTransactedPool itself: the callback is conditional on the connection having been parked. - Narrow the HasTransactionAffinity doc to ambient enlistment. Explicitly enlisted connections are parked on return regardless of the flag. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 43 +++++++++++++------ 1 file changed, 30 insertions(+), 13 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 1443700f23..39dfdc360d 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -252,10 +252,13 @@ public ConcurrentDictionary< private int MinPoolSize => PoolGroupOptions.MinPoolSize; /// - /// Indicates whether connections may be vended from (and parked in) the - /// . This mirrors automatic transaction enlistment: - /// when enlistment is disabled a connection is never bound to an ambient transaction, so - /// the transacted store must not be consulted. + /// Indicates whether the pool automatically enlists connections in the ambient transaction. + /// When disabled, the ambient transaction is neither used to consult the + /// nor handed to activation. + /// + /// This governs the ambient transaction only. A caller may still enlist explicitly via + /// SqlConnection.EnlistTransaction, and such a connection is parked in the transacted store + /// on return regardless of this flag, since it is bound to a transaction either way. /// private bool HasTransactionAffinity => PoolGroupOptions.HasTransactionAffinity; @@ -333,18 +336,23 @@ public void PutObjectFromTransactedPool(DbConnectionInternal connection) // NOTE: no locking is required here because if we're in this method we can safely // presume that the caller is the only one using the connection, that all pre-push logic // has been done, and that all transactions have ended. - SqlClientEventSource.Log.TryPoolerTraceEvent( - " {0}, Connection {1}, Transaction has ended.", - Id, - connection.ObjectID); - if (State is Running && connection.CanBePooled) { + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Connection {1}, Transaction has ended; returning connection to pool.", + Id, + connection.ObjectID); + connection.ResetConnection(); PutConnectionInIdleChannel(connection); } else { + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Connection {1}, Transaction has ended; destroying unpoolable connection.", + Id, + connection.ObjectID); + // RemoveConnection triggers replenishment, which is the channel pool's equivalent // of the wait handle pool's QueuePoolCreateRequest. RemoveConnection(connection); @@ -758,9 +766,17 @@ public void TransactionEnded(Transaction transaction, DbConnectionInternal trans transaction.GetHashCode(), transactedObject.ObjectID); - // If the connection is in the transacted pool, remove it there and return it to general - // circulation. TransactedConnectionPool.TransactionEnded calls back into - // PutObjectFromTransactedPool for us once it has removed the connection from its list. + // Removal from the transacted list happens synchronously inside this call, and + // TransactedConnectionPool.TransactionEnded calls back into PutObjectFromTransactedPool + // itself to return the connection to general circulation. + // + // We deliberately do not call PutObjectFromTransactedPool ourselves afterwards: that + // callback is conditional on the connection actually having been found in the list. + // A transaction can complete while the application still holds the connection, in which + // case the connection was never parked, and returning it here would hand a connection + // that is still in use to another caller. In that case the connection instead reaches + // the pool through the normal ReturnInternalConnection path when it is closed. + // This mirrors WaitHandleDbConnectionPool.TransactionEnded. TransactedConnectionPool.TransactionEnded(transaction, transactedObject); } @@ -1368,8 +1384,9 @@ private void PrepareConnection(DbConnection owningObject, DbConnectionInternal c } SqlClientEventSource.Log.TryPoolerTraceEvent( - " {0}, Connection {1}, Popped from transacted pool.", + " {0}, Transaction {1}, Connection {2}, Popped from transacted pool.", Id, + transaction.GetHashCode(), connection.ObjectID); SqlClientDiagnostics.Metrics.ExitFreeConnection(); From 815a12f8f8a04d796d0266958851758219f50f24 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Thu, 6 Aug 2026 13:13:27 -0700 Subject: [PATCH 16/16] Address review feedback on pool comments and transaction assertions Correct the ReturnInternalConnection comment: deactivation does not detach a completed transaction, that happens in CloseConnection before the pool is called. The real constraint is that DeactivateConnection mutates both gates the return path branches on. Explain why ReplaceConnection releases the old connection's slot in the idle branch but not in the create-new branch, and note that the V1 stasis case for a null pool is unreachable and redundant. Assert EnlistedTransaction alongside the pool-state assertions so the tests confirm what a connection is bound to, not just where it was filed. Add transacted store counts for the inner transaction at vend and after completion. Cover a transaction completing while its connection is still checked out, and strengthen the bare TransactionEnded test to use an enlisted connection. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 26 +++- .../ChannelDbConnectionPoolTransactionTest.cs | 122 +++++++++++++++--- 2 files changed, 127 insertions(+), 21 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 39dfdc360d..2feab5faaf 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -377,6 +377,12 @@ public DbConnectionInternal ReplaceConnection( // Carry the old connection's enlistment over to the replacement so that a connection // replaced mid-transaction stays bound to the same transaction. PrepareConnection(owningObject, newConnection, oldConnection.EnlistedTransaction); + + // newConnection came from the idle channel, so it already holds a slot of its own. + // Releasing oldConnection's slot here keeps the pool's count accurate. This is + // deliberately different from the create-new branch below, which hands oldConnection's + // slot to the replacement via _connectionSlots.TryReplace and therefore only disposes + // it -- calling RemoveConnection there would signal a free slot that does not exist. oldConnection.DeactivateConnection(); RemoveConnection(oldConnection); } @@ -472,9 +478,13 @@ public void ReturnInternalConnection(DbConnectionInternal connection, DbConnecti Id, connection.ObjectID); - // Deactivate before inspecting the connection's transaction state. Deactivation is what - // detaches a completed transaction, so reading EnlistedTransaction beforehand could park - // a connection in the transacted pool under a transaction that has already ended. + // Deactivate before inspecting the connection, because DeactivateConnection mutates both + // of the gates we branch on below: + // - Deactivate() dooms the connection when async commands are still outstanding, which + // the IsConnectionDoomed check must observe. + // - DeactivateConnection dooms it via DoNotPoolThisConnection when the load-balance + // timeout has elapsed, which the CanBePooled check must observe. + // WaitHandleDbConnectionPool.DeactivateObject orders it the same way. connection.DeactivateConnection(); if (connection.IsConnectionDoomed) @@ -484,7 +494,12 @@ public void ReturnInternalConnection(DbConnectionInternal connection, DbConnecti return; } - // Note: this logic mirrors WaitHandleDbConnectionPool.ReturnObject + // Note: this logic mirrors WaitHandleDbConnectionPool.ReturnObject, minus one dead + // branch. Its Running path also checks IsTransactionRoot && Pool == null -> SetInStasis, + // under its own "how did we get here if the pool is null?" TODO. A connection cannot + // arrive here without a pool, because this method is called through the connection's own + // Pool reference. The branch is redundant in any case: putting a transaction root in + // stasis is exactly what the first case below does when the connection cannot be pooled. ReturnDisposition disposition; lock (connection) { @@ -837,10 +852,11 @@ public bool TryGetConnection( // OpenAsync call. This means that we cannot cancel the connection open operation if the caller's token // is cancelled. We can only cancel based on our own timeout, which is set to the owningObject's // ConnectionTimeout. + // // The ambient transaction is captured by the caller, on the caller's thread, and handed // to us in the TaskCompletionSource's AsyncState (see SqlConnection.InternalOpenAsync). // - // We must not read Transaction.Current inside the Task.Run below instead. A + // We must not read Transaction.Current inside the Task.Run below. A // TransactionScope created with TransactionScopeAsyncFlowOption.Enabled stores the // transaction in an AsyncLocal, which does flow onto the pool's worker thread, so that // would appear to work. But Enabled is not the default: a plain TransactionScope keeps diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs index fdaa1f4552..e1d56fa004 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs @@ -168,6 +168,19 @@ private int TransactedConnectionsFor(Transaction transaction) => ? connections.Count : 0; + /// + /// Asserts which transaction a connection is bound to, complementing the pool-state assertions: + /// AssertPoolState says where the connection was filed, this says what it is actually enlisted + /// in. Pass null to assert the connection is not enlisted at all. + /// + /// + /// Compared by equality rather than reference because the EnlistedTransaction setter stores a + /// clone, so that the connection does not hold the caller's transaction past the end of its + /// using block. + /// + private static void AssertEnlistedIn(Transaction? expected, DbConnectionInternal connection) => + Assert.Equal(expected, connection.EnlistedTransaction); + #endregion #region Connection Return Routing @@ -188,6 +201,7 @@ public void ReturnConnection_WhileEnlisted_ParksInTransactedStoreNotIdleChannel( var owner = new SqlConnection(); var connection = GetConnection(owner); Assert.NotNull(connection); + AssertEnlistedIn(transaction, connection); AssertPoolState(count: 1, idleCount: 0, transactedCount: 0); // Act @@ -197,6 +211,10 @@ public void ReturnConnection_WhileEnlisted_ParksInTransactedStoreNotIdleChannel( AssertPoolState(count: 1, idleCount: 0, transactedCount: 1); Assert.Equal(1, TransactedConnectionsFor(transaction!)); + // The connection stays bound to the transaction while parked, which is what makes it + // ineligible for a caller in any other transaction. + AssertEnlistedIn(transaction, connection); + scope.Complete(); } @@ -211,20 +229,22 @@ public void ReturnConnection_WithoutTransaction_ReturnsToIdleChannel() var owner = new SqlConnection(); var connection = GetConnection(owner); Assert.NotNull(connection); + AssertEnlistedIn(null, connection); AssertPoolState(count: 1, idleCount: 0, transactedCount: 0); // Act ReturnConnection(connection, owner); // Assert + AssertEnlistedIn(null, connection); AssertPoolState(count: 1, idleCount: 1, transactedCount: 0); } /// - /// The pool deactivates a returning connection before reading its enlistment, and deactivation - /// is what detaches a completed transaction. A connection returned after its transaction has - /// already ended must therefore land in the idle channel, not be parked under a dead - /// transaction where nothing would ever release it. + /// A connection whose transaction has already ended must land in the idle channel, not be + /// parked under a dead transaction where nothing would ever release it. The enlistment is + /// dropped when the transaction completes, so by the time the connection is returned there is + /// nothing left to park it under. /// [Fact] public void ReturnConnection_AfterTransactionCompleted_ReturnsToIdleChannel() @@ -236,9 +256,13 @@ public void ReturnConnection_AfterTransactionCompleted_ReturnsToIdleChannel() { connection = GetConnection(owner); Assert.NotNull(connection); + AssertEnlistedIn(Transaction.Current, connection); scope.Complete(); } + // The completed transaction detached itself from the connection. + AssertEnlistedIn(null, connection); + // Act - the transaction is fully disposed by this point. ReturnConnection(connection, owner); @@ -261,10 +285,15 @@ public void ReturnConnection_WithTransactionAffinityDisabled_ReturnsToIdleChanne var connection = GetConnection(owner); Assert.NotNull(connection); + // The ambient transaction exists but must not reach the connection. + Assert.NotNull(Transaction.Current); + AssertEnlistedIn(null, connection); + // Act ReturnConnection(connection, owner); // Assert + AssertEnlistedIn(null, connection); AssertPoolState(count: 1, idleCount: 1, transactedCount: 0); scope.Complete(); @@ -326,6 +355,7 @@ public void GetConnection_UnderSameTransaction_VendsTheAlreadyEnlistedConnection // Assert Assert.Same(connection1, connection2); + AssertEnlistedIn(transaction, connection2); AssertPoolState(count: 1, idleCount: 0, transactedCount: 0); ReturnConnection(connection2, owner2); @@ -350,6 +380,7 @@ public async Task GetConnectionAsync_UnderSameTransaction_VendsTheAlreadyEnliste var owner1 = new SqlConnection(); var connection1 = await GetConnectionAsync(owner1, transaction); Assert.NotNull(connection1); + AssertEnlistedIn(transaction, connection1); ReturnConnection(connection1, owner1); AssertPoolState(count: 1, idleCount: 0, transactedCount: 1); @@ -359,6 +390,7 @@ public async Task GetConnectionAsync_UnderSameTransaction_VendsTheAlreadyEnliste // Assert Assert.Same(connection1, connection2); + AssertEnlistedIn(transaction, connection2); AssertPoolState(count: 1, idleCount: 0, transactedCount: 0); ReturnConnection(connection2, owner2); @@ -381,13 +413,15 @@ public void GetConnection_UnderDifferentTransaction_DoesNotVendTheEnlistedConnec var owner1 = new SqlConnection(); var connection1 = GetConnection(owner1); + AssertEnlistedIn(outerTransaction, connection1); ReturnConnection(connection1, owner1); AssertPoolState(count: 1, idleCount: 0, transactedCount: 1); // Act - RequiresNew starts an unrelated transaction. + Transaction? innerTransaction; using (var innerScope = new TransactionScope(TransactionScopeOption.RequiresNew)) { - Transaction? innerTransaction = Transaction.Current; + innerTransaction = Transaction.Current; Assert.NotEqual(outerTransaction, innerTransaction); var owner2 = new SqlConnection(); @@ -395,8 +429,15 @@ public void GetConnection_UnderDifferentTransaction_DoesNotVendTheEnlistedConnec // Assert - a fresh connection, because connection1 belongs to the outer transaction. Assert.NotSame(connection1, connection2); + AssertEnlistedIn(innerTransaction, connection2); + AssertEnlistedIn(outerTransaction, connection1); AssertPoolState(count: 2, idleCount: 0, transactedCount: 1); + // The store holds only the outer transaction's connection, so connection2 was newly + // created rather than taken from the store. + Assert.Equal(1, TransactedConnectionsFor(outerTransaction!)); + Assert.Equal(0, TransactedConnectionsFor(innerTransaction!)); + ReturnConnection(connection2, owner2); AssertPoolState(count: 2, idleCount: 0, transactedCount: 2); Assert.Equal(1, TransactedConnectionsFor(outerTransaction!)); @@ -409,6 +450,7 @@ public void GetConnection_UnderDifferentTransaction_DoesNotVendTheEnlistedConnec // connection stays parked. AssertPoolState(count: 2, idleCount: 1, transactedCount: 1); Assert.Equal(1, TransactedConnectionsFor(outerTransaction!)); + Assert.Equal(0, TransactedConnectionsFor(innerTransaction!)); outerScope.Complete(); } @@ -440,10 +482,14 @@ public void TransactionCommit_ReturnsParkedConnectionToIdleChannel() // Assert AssertPoolState(count: 1, idleCount: 1, transactedCount: 0); + // The transaction released it, so it carries no enlistment into general circulation. + AssertEnlistedIn(null, connection); + // The released connection is reusable by a caller with no ambient transaction. var owner2 = new SqlConnection(); var connection2 = GetConnection(owner2); Assert.Same(connection, connection2); + AssertEnlistedIn(null, connection2); ReturnConnection(connection2, owner2); } @@ -455,10 +501,11 @@ public void TransactionCommit_ReturnsParkedConnectionToIdleChannel() public void TransactionRollback_ReturnsParkedConnectionToIdleChannel() { // Arrange + DbConnectionInternal connection; using (new TransactionScope()) { var owner = new SqlConnection(); - var connection = GetConnection(owner); + connection = GetConnection(owner); ReturnConnection(connection, owner); AssertPoolState(count: 1, idleCount: 0, transactedCount: 1); @@ -467,6 +514,7 @@ public void TransactionRollback_ReturnsParkedConnectionToIdleChannel() // Assert AssertPoolState(count: 1, idleCount: 1, transactedCount: 0); + AssertEnlistedIn(null, connection); } /// @@ -500,32 +548,71 @@ public void TransactionCompletion_AfterShutdown_DestroysConnection() } /// - /// TransactionEnded for a connection that was never parked must be a no-op. It must not push a - /// connection that is still checked out into the idle channel, where a second caller could - /// pick it up while the first is still using it. + /// TransactionEnded on its own, with none of the surrounding lifecycle events, must be a no-op + /// for a connection that was never parked. Only parking hands the connection to the pool, so + /// even an enlisted connection is still checked out here, and pushing it into the idle channel + /// would let a second caller pick it up while the first is still using it. /// [Fact] - public void TransactionEnded_ForConnectionThatWasNeverParked_LeavesItCheckedOut() + public void TransactionEnded_ForEnlistedConnectionThatWasNeverParked_LeavesItCheckedOut() { // Arrange + using var scope = new TransactionScope(); + Transaction? transaction = Transaction.Current; + Assert.NotNull(transaction); + var owner = new SqlConnection(); var connection = GetConnection(owner); Assert.NotNull(connection); - using var scope = new TransactionScope(); - Transaction? transaction = Transaction.Current; - Assert.NotNull(transaction); + // The connection is enlisted, but checked out rather than parked. + AssertEnlistedIn(transaction, connection); + AssertPoolState(count: 1, idleCount: 0, transactedCount: 0); - // Act + // Act - the completion notification arrives on its own, with no detach and no return. _pool.TransactionEnded(transaction!, connection); - // Assert + // Assert - nothing to release, and the enlistment is untouched. + AssertEnlistedIn(transaction, connection); AssertPoolState(count: 1, idleCount: 0, transactedCount: 0); + // It still reaches the pool through the normal return path. ReturnConnection(connection, owner); + AssertPoolState(count: 1, idleCount: 0, transactedCount: 1); + scope.Complete(); } + /// + /// A transaction can complete while the application still holds its connection. The connection + /// was never parked, so there is nothing for the completion to release, and pushing it into the + /// idle channel would hand a connection that is still in use to a second caller. + /// + [Fact] + public void TransactionCompletes_WhileConnectionStillCheckedOut_LeavesItCheckedOut() + { + // Arrange + var owner = new SqlConnection(); + DbConnectionInternal connection; + using (var scope = new TransactionScope()) + { + connection = GetConnection(owner); + Assert.NotNull(connection); + AssertEnlistedIn(Transaction.Current, connection); + AssertPoolState(count: 1, idleCount: 0, transactedCount: 0); + + // Act - the transaction completes with the connection still checked out. + scope.Complete(); + } + + // Assert + AssertPoolState(count: 1, idleCount: 0, transactedCount: 0); + + // It reaches the pool only when its owner returns it. + ReturnConnection(connection, owner); + AssertPoolState(count: 1, idleCount: 1, transactedCount: 0); + } + #endregion #region Connection Replacement @@ -545,6 +632,7 @@ public void ReplaceConnection_CarriesEnlistedTransactionToNewConnection() var owner = new SqlConnection(); var oldConnection = GetConnection(owner); Assert.NotNull(oldConnection); + AssertEnlistedIn(transaction, oldConnection); AssertPoolState(count: 1, idleCount: 0, transactedCount: 0); // Act @@ -553,9 +641,11 @@ public void ReplaceConnection_CarriesEnlistedTransactionToNewConnection() oldConnection, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15))); - // Assert - a distinct connection took over the old connection's slot. + // Assert - a distinct connection took over the old connection's slot, carrying the + // enlistment with it. Assert.NotNull(newConnection); Assert.NotSame(oldConnection, newConnection); + AssertEnlistedIn(transaction, newConnection); Assert.True(oldConnection.IsConnectionDoomed); AssertPoolState(count: 1, idleCount: 0, transactedCount: 0);