ChannelDbConnectionPool transaction support by mdaigle · Pull Request #4487 · dotnet/SqlClient

mdaigle · 2026-07-29T17:44:38Z

Rebased onto main now that #4429 has merged. Unit tests: 834 passed / 0 failed.

Summary

Implements transaction support in ChannelDbConnectionPool, using WaitHandleDbConnectionPool as the reference for correct behavior. Before this change the channel pool constructed a TransactedConnectionPool but never used it, and three IDbConnectionPool members threw NotImplementedException.

Changes

PutObjectFromTransactedPool (was NotImplementedException) — returns a connection to general circulation once its transaction has ended, or destroys it if the pool is no longer running or the connection can't be pooled.
TransactionEnded (was NotImplementedException) — delegates to TransactedConnectionPool.TransactionEnded, which calls back into PutObjectFromTransactedPool.
ReturnInternalConnection — rewritten to mirror WaitHandleDbConnectionPool.DeactivateObject. It now deactivates first (deactivation is what detaches a completed transaction, so reading EnlistedTransaction beforehand could park a connection under an already-ended transaction), then decides under the connection lock between the transacted pool, stasis, the idle channel, and destruction. The idle-channel path moved into a new PutConnectionInIdleChannel helper.
GetFromTransactedPool (new) — vends a connection already enlisted in the ambient transaction. Transacted connections are exempt from idle-timeout and clear-generation eviction, since closing them would abort a possibly-distributed transaction, so only liveness is checked. A dead transaction root rethrows rather than silently retrying, because its delegated transaction cannot be recovered on another connection.
GetInternalConnection / PrepareConnection — consult the transacted pool when the pool group has transaction affinity, and pass the ambient transaction through to ActivateConnection.
Async acquisition path — takes the ambient transaction from taskCompletionSource.Task.AsyncState and threads it explicitly through the open, rather than assigning Transaction.Current on the thread pool thread. See the section below.
RemoveConnection — no longer disposes a transaction root that is still waiting for its delegated transaction to end (parity with DestroyObject). It comes back through PutObjectFromTransactedPool when the transaction completes.
ReplaceConnection — no functional change; the two TODO: Full transaction enlistment support (Story 2) markers from ChannelDbConnectionPool replace connection #4429 are removed now that enlistment is wired through.

How connections move between the idle channel and the transacted store

The transacted store (TransactedConnectionPool, keyed by Transaction) reserves a connection for one specific transaction, so reusing it avoids promoting that transaction to a distributed one. The only edge into it is a return while still enlisted; the only edges out are a pop by a caller in the same transaction, or the transaction ending.

Return path (`ReturnInternalConnection`)

flowchart TD
    R["ReturnInternalConnection"] --> V["ValidateOwnershipAndSetPoolingState"]
    V --> D["DeactivateConnection"]
    D --> Doomed{"IsConnectionDoomed?"}
    Doomed -- yes --> Destroy["RemoveConnection (Destroy)"]
    Doomed -- no --> Poolable{"State is Running and CanBePooled?"}

    Poolable -- no --> Root{"IsTransactionRoot?"}
    Root -- yes --> Stasis["SetInStasis (HeldByTransaction)"]
    Root -- no --> Destroy

    Poolable -- yes --> Enl{"EnlistedTransaction is not null?"}
    Enl -- yes --> Park["PutTransactedObject (HeldByTransaction)"]
    Enl -- no --> Reuse["PutConnectionInIdleChannel (Reuse)"]

DeactivateConnection runs before EnlistedTransaction is read, because deactivation is what detaches an already-completed transaction. Reading first would park the connection under a transaction that has already ended, and it would never be released.

Transaction end: back to general circulation

flowchart TD
    Sig["System.Transactions signals completion"] --> Where{"where is the connection?"}
    Where -- "parked in the transacted store" --> TE["pool.TransactionEnded"]
    TE --> TCP["TransactedConnectionPool.TransactionEnded removes it from the list"]
    TCP --> Put["PutObjectFromTransactedPool"]
    Where -- "in stasis" --> DTE["DelegatedTransactionEnded then TerminateStasis(true)"]
    DTE --> Put
    Where -- "never parked, still checked out" --> NoOp["no-op: stays with its owner"]

    Put --> Ok{"State is Running and CanBePooled?"}
    Ok -- yes --> Reset["ResetConnection then PutConnectionInIdleChannel"]
    Ok -- no --> Rm["RemoveConnection"]

A connection in stasis reaches PutObjectFromTransactedPool too, but by definition it got there because the pool was stopping or it was unpoolable, so it always takes the RemoveConnection branch.

A parked connection keeps its _connectionSlots reservation, so it still counts toward Count and MaxPoolSize, but not IdleCount. Only RemoveConnection releases the slot.

Tests

New ChannelDbConnectionPoolTransactionTest (18 tests): return routing (enlisted, not enlisted, completed transaction, Enlist=false, shut-down pool), vending from the transacted store under the same and different transactions, commit/rollback, completion after shutdown, TransactionEnded for a connection that was never parked, enlistment carry-over through ReplaceConnection, and the ambient-transaction flow cases below. Every test asserts full pool state (Count, IdleCount, transacted count) after each step, and the pool is built with a frozen TimeProvider.
Removed the stale tests asserting NotImplementedException from the three now-implemented members.

Validation

Unit tests: 834 passed / 0 failed on net9.0.

Manual/integration tests were run against a local SQL Server with Switch.Microsoft.Data.SqlClient.UseConnectionPoolV2 enabled, across TransactionEnlistmentTest, TransactionPoolTest, SQL.TransactionTest, ParallelTransactionsTest, ConnectionPoolTest, PoolBlockPeriodTest and DistributedTransactionTest (48 tests):

Run	Passed	Failed
V1 (WaitHandle) baseline	39	6
V2 without this change	36	9
V2 with this change	37	8

This change fixes three previously failing tests: TestAutoEnlistment_TxScopeNonComplete, TestManualEnlistment_Enlist and TestManualEnlistment_Enlist_TxScopeComplete.

The 6 failures shared with the V1 baseline are all PlatformNotSupportedException: This platform does not support distributed transactions — MSDTC isn't available on the test machine. The 2 remaining failures (ConnectionPoolTest.ReclaimEmancipatedOnOpenTest) are a pre-existing V2 gap: ReclaimEmancipatedObjects has never been implemented in ChannelDbConnectionPool. Both were confirmed by reverting this change and reproducing.

A broader V2 sweep (SqlCommand, AsyncTest, MARSTest, DataReaderTest, ConnectivityTests, WeakRefTest, AdapterTest, ExceptionTest, RetryLogic) gave 213 passed / 9 failed, where all 9 are named-pipe tests that fail identically on V1.

Ambient transaction flow on the async path

Transaction.Current does not flow into a Task.Run unless the TransactionScope was created with TransactionScopeAsyncFlowOption.Enabled (the default is Suppress, which keeps the ambient transaction in thread-static storage). The async open path therefore cannot simply read Transaction.Current — it has to take the transaction from the TaskCompletionSource's AsyncState, which is where SqlConnection.InternalOpenAsync captures it. That is the only site in the repo that constructs a TaskCompletionSource<DbConnectionInternal>, and it always passes the ambient transaction, so the mechanism is reliable (including across OpenAsyncRetry.Retry, which reuses the same TCS).

The original approach was to restore it by assigning ADP.SetCurrentTransaction(...) inside the Task.Run. That is unsafe here: assigning Transaction.Current writes to thread-static storage that ExecutionContext does not unwind, so the transaction outlives the open and is observable by unrelated work later scheduled onto the same thread pool thread — most notably the login-time auto-enlistment that non-pooled connections perform against Transaction.Current. A try/finally restore doesn't fix it either, because the async continuation may resume on a different thread than the one that was polluted. (WaitHandleDbConnectionPool does the same assignment safely because WaitForPendingOpen asserts it is not on a thread pool thread.)

The fix is to never mutate Transaction.Current in the pool: the transaction is captured on the caller's thread and threaded explicitly through GetInternalConnection into GetFromTransactedPool and PrepareConnection. The sync path passes ADP.GetCurrentTransaction() directly, since it runs on the caller's thread.

Four regression tests cover this:

Test	What it pins
`GetConnection_Sync_UsesAmbientTransactionFromCallersThread`	the sync path reads `Transaction.Current`, which is correct because it runs on the caller's thread
`GetConnectionAsync_UsesAmbientTransactionCapturedOnCallersThread`	a scope with `AsyncFlowOption.Enabled` still enlists
`GetConnectionAsync_WithAsyncFlowDisabled_StillEnlistsInAmbientTransaction`	a default (`Suppress`) scope still enlists, even though the transaction provably does not flow off the caller's thread
`GetConnectionAsync_EnteredFromThreadWithoutAmbientTransaction_EnlistsFromAsyncState`	the transaction comes from `AsyncState`, not from whatever is ambient on the entering thread — this is the `OpenAsyncRetry.Retry` case

The last one is load-bearing and 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 distinguishes them. 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.

Checklist

Tests added or updated
Public API changes documented — n/a, no public API change
Verified against customer repro (if applicable) — n/a
Ensure no breaking changes introduced — behavior is behind the existing UseConnectionPoolV2 switch, which defaults to off

Notes

ReclaimEmancipatedObjects remains unimplemented in the channel pool; it is orthogonal to transactions and left for a follow-up.

Copilot

Pull request overview

Adds full transaction enlistment/routing support to the V2 ChannelDbConnectionPool, aligning behavior with the legacy WaitHandleDbConnectionPool so pooled connections correctly participate in ambient System.Transactions flows (including async acquisition).

Changes:

Implemented transaction lifecycle plumbing in ChannelDbConnectionPool (PutObjectFromTransactedPool, TransactionEnded, transacted acquisition path, and updated return/deactivation logic).
Enabled async acquisition to restore the captured ambient transaction on the worker thread (ADP.SetCurrentTransaction(...)).
Added a comprehensive unit test suite for channel-pool transaction behavior and removed the now-stale NotImplementedException assertions.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs	New transaction-focused unit tests for the channel-based pool, mirroring WaitHandle pool coverage and adding channel-specific scenarios.
src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs	Removes tests that asserted transaction methods were unimplemented (now implemented).
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs	Implements transaction support: transacted pool vending/parking, correct return paths for enlisted connections, and async ambient transaction propagation.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs:649

task2Done is declared but never used, which adds noise and makes the synchronization intent harder to follow. Remove it (or use it if it was meant to assert task2 completion).

        using var task2Done = new ManualResetEventSlim(false);

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs:1206

GetInternalConnection creates a CancellationTokenSource even when a connection was successfully retrieved from the transacted pool, which adds avoidable allocations on that hot path. Consider returning early after GetFromTransactedPool succeeds so the CTS/loop is skipped entirely.

            // 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;

mdaigle

Overall, the tests need a lot of cleanup. Verify pool count, idle, general stats and metrics at each step. Remove tests that are a strict subset of other tests. Add comments, think deeply about which tests are really required and provide good coverage. Use code coverage metrics to guide your decisions.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs:260

The new XML doc for HasTransactionAffinity says connections may be "vended from (and parked in)" the TransactedConnectionPool when enabled. The code still parks connections based on connection.EnlistedTransaction in DecideReturnDisposition even when transaction affinity is disabled (e.g., manually enlisted connections), so the doc is misleading about the parking behavior. Consider rewording to clarify that this flag controls automatic transaction affinity (consulting the transacted pool / auto-enlisting on activation), not whether parking can occur at all.

        /// <summary>
        /// Indicates whether connections may be vended from (and parked in) the
        /// <see cref="TransactedConnectionPool"/>. 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.
        /// </summary>
        private bool HasTransactionAffinity => PoolGroupOptions.HasTransactionAffinity;

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs:260

The HasTransactionAffinity doc comment currently states the transacted store “must not be consulted” when enlistment is disabled, but the return path still parks any manually-enlisted connection (connection.EnlistedTransaction != null) in the transacted store (matching WaitHandle behavior). The summary should be narrowed to describe ambient-transaction consultation/activation only, to avoid misleading future maintainers.

        /// Indicates whether connections may be vended from (and parked in) the
        /// <see cref="TransactedConnectionPool"/>. 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.
        /// </summary>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs:258

The HasTransactionAffinity summary is misleading: the pool can still park explicitly-enlisted connections in the TransactedConnectionPool even when transaction affinity (ambient auto-enlist) is disabled. The property is only used to decide whether to consult the transacted store for the ambient transaction and pass it to activation.

        /// <summary>
        /// Indicates whether connections may be vended from (and parked in) the
        /// <see cref="TransactedConnectionPool"/>. 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.

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs:1366

SqlClientDiagnostics.Metrics free-connection accounting looks inconsistent in ChannelDbConnectionPool: this path decrements free connections when vending from the transacted pool, and TransactedConnectionPool.TransactionEnded also decrements before calling PutObjectFromTransactedPool, but the channel pool never increments/decrements free-connection metrics when writing to/reading from the idle channel (unlike WaitHandleDbConnectionPool.PutNewObject/GetFromGeneralPool). This can leave free-connection telemetry incorrect after transaction completion (and generally makes metrics hard to interpret for the V2 pool).

                connection.ObjectID);

            SqlClientDiagnostics.Metrics.ExitFreeConnection();

            // Transacting connections are exempt from idle-timeout and clear-generation eviction

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

+    /// 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.
+    /// </summary>


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>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

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>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs:662

The XML doc comment for GetConnectionAsync_DoesNotSetAmbientTransactionOnPoolWorkerThread is malformed: it ends with </summary> but is missing the opening /// <summary> tag. If XML doc generation is enabled for this project or analyzer warnings are treated as errors, this can fail the build. Add the missing <summary> opening line so the comment is well-formed.

    /// 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.
    ///

…read 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>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

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>

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>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

paulmedynski · 2026-08-05T18:55:35Z

        public void PutObjectFromTransactedPool(DbConnectionInternal connection)
        {
-            throw new NotImplementedException();
+            Debug.Assert(connection.EnlistedTransaction is null,


I wonder if we need StandaloneConnection and TransactionConnection types to model this, and make it impossible to mix up. For future consideration.

paulmedynski · 2026-08-05T18:58:07Z

+            // 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(
+                "<prov.DbConnectionPool.PutObjectFromTransactedPool|RES|CPOOL> {0}, Connection {1}, Transaction has ended.",


Shouldn't a log about the transaction ending live where it ends, and log its unique ID? I think this log would be more effective down in the if/else blocks: "Returning connection to pool", "Destroying unpoolable connection"

paulmedynski · 2026-08-05T19:10:17Z

+            }
+
+            SqlClientEventSource.Log.TryPoolerTraceEvent(
+                "<prov.DbConnectionPool.GetFromTransactedPool|RES|CPOOL> {0}, Connection {1}, Popped from transacted pool.",


It might be helpful to include the transaction identifier in all logs related to transactions.

- 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>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

codecov · 2026-08-05T21:22:55Z

Codecov Report

❌ Patch coverage is 82.05128% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 62.80%. Comparing base (deabcc2) to head (9f97de6).

Files with missing lines	Patch %	Lines
...qlClient/ConnectionPool/ChannelDbConnectionPool.cs	82.05%	21 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4487      +/-   ##
==========================================
- Coverage   64.71%   62.80%   -1.91%     
==========================================
  Files         288      283       -5     
  Lines       44088    67136   +23048     
==========================================
+ Hits        28532    42167   +13635     
- Misses      15556    24969    +9413

Flag	Coverage Δ
CI-SqlClient	`?`
PR-SqlClient-Project	`62.80% <82.05%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

saurabh500 · 2026-08-05T17:37:22Z

        public void PutObjectFromTransactedPool(DbConnectionInternal connection)
        {
-            throw new NotImplementedException();
+            Debug.Assert(connection.EnlistedTransaction is null,


Out of curiosity, are these Debug Asserts useful, i.e. do we run tests on Debug builds?

It might be good to even have a SqlClientEventSource Log to emit this error (just in case we do get into a bad situation like this and a customer trace and point at this bug)

Yes, our PR pipelines compile and run everything in Debug mode.

This is a programming error within the driver itself - there is no remedy for a customer other than waiting for us to fix it. Throwing here would likely cause the connection to become ophaned and never be cleaned up. We're between a rock and a hard place. Leaving the Debug.Assert() may cause us to put the connection in the idle pool and have an unrelated command re-use it, unknowingly within the still-open transaction - seems pretty bad (data corruption?). Throwing may leak the connection - not good, but better than data corruption.

In reality, I think we just need a test that proves we never actually call this with an enlisted connection, and add a TODO to re-design the interactions between normal pools and transaction pools to completely avoid such a situation.

saurabh500 · 2026-08-05T18:50:23Z

+            // 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.


I think this comment is describing the wrong mechanism, and I want to check we are on the same page before suggesting a change.

DeactivateConnection does not detach a transaction. It calls Deactivate() (SqlConnectionInternal.cs:2015), which never touches EnlistedTransaction. The detach happens earlier and in a different method: DbConnectionInternal.CloseConnection calls DetachCurrentTransactionIfEnded() at line 464, before it calls ReturnInternalConnection at line 472 -- and the comment there even says "ReturnInternalConnection calls Deactivate for us." So by the time we reach this point, a completed transaction is already detached regardless of the ordering here.

That said, I am fairly confident the deactivate-first ordering is still correct, just for a different reason. DeactivateConnection mutates both of the things we branch on immediately after it:

Deactivate() dooms the connection when _asyncCommandCount != 0 (SqlConnectionInternal.cs:2030-2033) and in its catch at 2059 -- so the IsConnectionDoomed check below must run after it.

DeactivateConnection itself calls DoNotPoolThisConnection() on load-balance-timeout expiry (DbConnectionInternal.cs:527-536) -- so the CanBePooled check must run after it too.

Which is exactly why DeactivateObject orders it this way in the WaitHandle pool.

My concern with the wording as-is: it frames the constraint as transaction-specific. Someone optimizing the non-transacted return path later could read this and reasonably conclude it is safe to hoist the doomed/poolable checks above the DeactivateConnection call -- and quietly start pooling doomed connections.

Would you be open to rewording this to the doom/poolability reason instead? Or have I misread the detach path?

Suggested fix

Reword to the doom/poolability reason:

// Deactivate before evaluating the disposition below. Deactivation can change the answer: // SqlConnectionInternal.Deactivate dooms a connection that still has outstanding async // commands, and DeactivateConnection retires one whose load-balance lifetime has expired. // Reading CanBePooled or EnlistedTransaction first would pool or park a connection that // deactivation was about to reject.

The same wording appears on ReturnConnection_AfterTransactionCompleted_ReturnsToIdleChannel in the new test file, so that doc comment would want the same correction -- I have left a separate note there with a test that pins the ordering for the reason above.

saurabh500 · 2026-08-05T18:50:32Z

            {
-                // 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.


Not blocking, and I have already talked myself out of most of this -- posting it for the comment it suggests rather than a code change.

The two ReplaceConnection branches retire oldConnection differently: this one goes through RemoveConnection(oldConnection), while the create-new branch below calls oldConnection.Dispose() directly.

Having read further, the divergence is required, not accidental:

This branch takes newConnection from GetIdleConnection(), so it already occupies its own slot. Two slots are held and one must be released, hence RemoveConnection.

The create-new branch calls _connectionSlots.TryReplace(oldConnection, newConnection) (line 417), which swaps the replacement into the old connection's slot. By the time Dispose() runs, oldConnection is already untracked -- calling RemoveConnection there would be wrong, not merely redundant.

So the paths cannot be unified, and I withdraw the suggestion to do so.

Suggested fix

Just make the reason local, so the next reader does not repeat this trip:

// 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. oldConnection.DeactivateConnection(); RemoveConnection(oldConnection);

Does that match your reasoning, or was there another consideration behind the split?

saurabh500 · 2026-08-05T18:50:33Z

+                    // 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;
+                    }


Checking whether this omission was deliberate rather than asserting it is wrong.

WaitHandleDbConnectionPool.DeactivateObject has one more stasis case that I do not see reproduced here: while Running and CanBePooled, it checks obj.IsTransactionRoot && obj.Pool == null and puts the connection in stasis (lines 630-634). Under this implementation, such a connection would instead land in the transacted store (if enlisted) or the idle channel.

I lean toward this being genuinely dead code in V1 -- it is marked with its own // TODO: how did we get here if the pool is null?, and a connection being returned to this pool should have Pool == this by construction. So I suspect dropping it is correct.

But I would rather hear it from you than infer it: was this consciously dropped as unreachable, or did it just not come across during the port? If it was deliberate, a one-line comment noting that V1 case and why it does not apply here would save the next person the same archaeology.

Suggested fix

If it was deliberate, something like this on the branch below:

// WaitHandleDbConnectionPool.DeactivateObject has a further case here -- // IsTransactionRoot && Pool == null -> SetInStasis (lines 630-634) -- that is not reproduced. // It is unreachable: a connection being returned to this pool has Pool == this by // construction, which is why V1 marks it with its own "how did we get here?" TODO.

If instead you would rather keep V1 parity literally, the equivalent would be an extra clause in the same branch, but I would not push for that -- a dead branch carried forward is worse than a comment explaining why it was dropped.

Copilot AI review requested due to automatic review settings July 29, 2026 17:44

mdaigle requested a review from a team as a code owner July 29, 2026 17:44

github-project-automation Bot added this to SqlClient Board Jul 29, 2026

github-project-automation Bot moved this to To triage in SqlClient Board Jul 29, 2026

Copilot started reviewing on behalf of mdaigle July 29, 2026 17:45 View session

Copilot AI reviewed Jul 29, 2026

View reviewed changes

Comment thread ...soft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs Outdated

mdaigle added this to the 7.1.0-preview3 milestone Jul 29, 2026

github-actions Bot assigned apoorvdeshmukh and cheenamalhotra Jul 29, 2026

mdaigle marked this pull request as draft July 29, 2026 18:08

Copilot AI review requested due to automatic review settings July 29, 2026 18:57

Copilot started reviewing on behalf of mdaigle July 29, 2026 18:57 View session

Copilot AI reviewed Jul 29, 2026

View reviewed changes

mdaigle commented Jul 29, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 29, 2026 19:16

Copilot started reviewing on behalf of mdaigle July 29, 2026 19:16 View session

Copilot AI reviewed Jul 29, 2026

View reviewed changes

mdaigle mentioned this pull request Jul 29, 2026

Reclaim emancipated connections in ChannelDbConnectionPool #4490

Draft

4 tasks

mdaigle commented Aug 3, 2026

View reviewed changes

Comment thread ...soft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs Outdated

Comment thread ...soft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs

Copilot AI review requested due to automatic review settings August 3, 2026 22:56

Copilot started reviewing on behalf of mdaigle August 3, 2026 22:57 View session

Copilot AI reviewed Aug 3, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings August 3, 2026 23:05

Copilot started reviewing on behalf of mdaigle August 3, 2026 23:07 View session

Copilot AI reviewed Aug 3, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings August 3, 2026 23:23

Copilot started reviewing on behalf of mdaigle August 3, 2026 23:24 View session

Copilot AI reviewed Aug 3, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings August 3, 2026 23:37

Copilot started reviewing on behalf of mdaigle August 3, 2026 23:38 View session

Copilot AI review requested due to automatic review settings August 5, 2026 17:13

Copilot started reviewing on behalf of mdaigle August 5, 2026 17:14 View session

Copilot AI reviewed Aug 5, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings August 5, 2026 17:25

Copilot started reviewing on behalf of mdaigle August 5, 2026 17:26 View session

Copilot AI reviewed Aug 5, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings August 5, 2026 17:35

Copilot started reviewing on behalf of mdaigle August 5, 2026 17:36 View session

Copilot AI reviewed Aug 5, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings August 5, 2026 17:47

Copilot started reviewing on behalf of mdaigle August 5, 2026 17:47 View session

Copilot AI reviewed Aug 5, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings August 5, 2026 17:54

Copilot started reviewing on behalf of mdaigle August 5, 2026 17:55 View session

Copilot AI reviewed Aug 5, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings August 5, 2026 18:00

Copilot started reviewing on behalf of mdaigle August 5, 2026 18:00 View session

Copilot AI reviewed Aug 5, 2026

View reviewed changes

paulmedynski reviewed Aug 5, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings August 5, 2026 19:47

Copilot started reviewing on behalf of mdaigle August 5, 2026 19:48 View session

Copilot AI reviewed Aug 5, 2026

View reviewed changes

saurabh500 reviewed Aug 6, 2026

View reviewed changes

Uh oh!

Conversation

mdaigle commented Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

How connections move between the idle channel and the transacted store

Return path (ReturnInternalConnection)

Transaction end: back to general circulation

Tests

Validation

Ambient transaction flow on the async path

Checklist

Notes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

mdaigle left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

mdaigle commented Jul 29, 2026 •

edited

Loading

Return path (`ReturnInternalConnection`)

codecov Bot commented Aug 5, 2026 •

edited

Loading