Fix | Preserve delegated transactions when resetting a pooled connection

[priyankatiwari08]

Fixes #4001

📄 Full root cause analysis: doc/design-notes/4001-delegated-transaction-reset.md

Summary

A pooled connection could be permanently broken after a TransactionScope rollback, surfacing to callers as:

InvalidOperationException: The requested operation cannot be completed because the connection has been broken.

This is a regression introduced in 6.1.0 by #3019. Confirmed by bisect: 6.0.5 ✅ · 6.1.0 ❌ · 6.1.1 ❌ · 6.1.4 ❌ · main ❌.

Root cause

On this code path a connection can be tied to a transaction in one of two mutually exclusive ways:

Case	Meaning	IsTransactionRoot	EnlistedTransaction
Delegated root	The transaction was delegated down to this connection and lives on it	true	null
Enlisted participant	The connection joined a transaction owned elsewhere	false	set

ResetConnection() decides whether to preserve the server-side transaction across a pool reset:

- _parser.PrepareResetConnection(IsTransactionRoot && !IsNonPoolableTransactionRoot);
+ _parser.PrepareResetConnection(EnlistedTransaction is not null && Pool is not null);

The old helper reduced to IsTransactionRoot && Pool != null, so #3019 swapped one case for the other rather than covering both. It genuinely fixed #2970 and traded it for #4001.

When a connection is returned to the pool while still the root of a live delegated transaction, preserveTransaction is false, and the TDS reset wipes the server-side transaction while System.Transactions still believes it exists. When the scope later rolls back, SqlDelegatedTransaction.Rollback fails and calls DoomThisConnection(). With a small pool that same doomed connection is handed straight back out.

Runtime instrumentation at the failing reset:

[RESET] preserve=False  root=True  delegated.IsActive=True  enlisted=null  pool=set
[DOOM]  <- SqlDelegatedTransaction.Rollback  <- Transaction.Rollback  <- TransactionScope.Dispose

The fix

_parser.PrepareResetConnection(
    Pool is not null &&
    (IsTransactionRoot || EnlistedTransaction is not null));

Why this cannot reintroduce #2970

IsTransactionRoot	EnlistedTransaction	Pre-#3019	#3019	This fix
false	null	false	false	false
true	null	✅ true	❌ false ← #4001	✅ true
false	set	❌ false ← #2970	✅ true	✅ true
true	set	true	true	true

The condition is exactly OLD || NEW — a strict superset of both prior behaviors. The #2970 row still evaluates true; reintroducing it would require A || B to be false while B is true. The guarantee is structural, not empirical.

Verification

Reporter's repro (NHibernate 5.5.2, MaxPoolSize=1, TransactionScope with a failed DTC promotion), on both pool implementations, both directions:

	without fix	with fix
WaitHandleDbConnectionPool (default)	❌ reproduces	✅ passes
ChannelDbConnectionPool (UseConnectionPoolV2)	❌ reproduces	✅ passes

The left-hand column was produced by stashing the fix and rebuilding — without it, a pass on V2 could just mean V2 never reaches this code path.

⚠️ What the existing test suite actually verifies

--filter "FullyQualifiedName~TransactionTest" reports 9/9 passing. That number is easy to over-read, so I mutation-tested the suite by compiling in each known-buggy condition:

Condition compiled in	Bug it contains	Suite result
Pre-#3019	#2970	9/9 passed
#3019 (current main)	#4001	9/9 passed
This fix	none	9/9 passed

The suite passes on all three — it does not guard this line at all. Test_EnlistedTransactionPreservedWhilePooled, added by #3019 specifically for #2970, is tagged [Trait("Category", "flaky")] and passes against code carrying the #2970 bug.

So the 9/9 result is evidence of no collateral damage, not evidence that the fix works. The evidence that it works is the reproduction matrix above and the structural argument. Flagging this explicitly so it isn't mistaken for coverage.

Checklist

• Tests added or updated — no; see the section above
• Public API changes documented — none, this is an internal behavior fix
• Verified against customer repro
• Ensure no breaking changes introduced

Related

• Azure SQL DTC bug #2970 — the issue Preserve distributed transactions on pooled connection reset #3019 was fixing; fully preserved by this change
• Microsoft.Data.SqlClient 5.1.2 throws InvalidOperationException: The requested operation cannot be completed because the connection has been broken #2285 — reports the same exception without a repro; plausibly the same root cause

[Copilot]

Pull request overview

Fixes a regression in pooled-connection reset behavior where a connection that is the root of a delegated TransactionScope transaction could have its server-side transaction unintentionally cleared during reset, leading to a later rollback dooming the physical connection and surfacing as “connection has been broken”.

Changes:

• Widened the PrepareResetConnection preserve-transaction condition to include delegated transaction roots (IsTransactionRoot) in addition to enlisted transactions (EnlistedTransaction != null).
• Expanded inline comments in ResetConnection() to document the two mutually exclusive transaction-participation cases and link the regression root cause (#4001).

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

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

Suppressed comments (4)

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

• This removes the pre-#3019 Is2008OrNewer safeguard, but SQL Server 2005 is still an accepted server version (tests/UnitTests/SimulatedServerTests/ConnectionTests.cs:768 and TdsEnums.SQL2005_VERSION). The old helper intentionally did not request transaction-preserving reset for pre-2008 servers; this condition now sends that preserve flag for a pooled delegated root on SQL Server 2005, where the reset protocol does not support it. Retain the server-version guard for the root case while preserving enlisted participants on all supported servers.

                _parser.PrepareResetConnection(
                    Pool is not null &&
                    (IsTransactionRoot || EnlistedTransaction is not null));

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

• Preserving the TDS transaction here is unsafe unless the delegated root is also kept out of general circulation. In the exact state this fix targets (IsTransactionRoot == true, EnlistedTransaction == null), both pool implementations use only EnlistedTransaction to choose the transacted pool, so this connection is returned to the general idle pool. A later caller with no ambient transaction can then reuse the still-live server transaction because Activate(null) deliberately leaves an active delegated root alone. Please update the pool return/stasis path to hold active delegated roots until transaction completion, rather than changing only the reset flag.

                _parser.PrepareResetConnection(
                    Pool is not null &&
                    (IsTransactionRoot || EnlistedTransaction is not null));

doc/design-notes/4001-delegated-transaction-reset.md:31

• The note says a delegated root always has EnlistedTransaction == null, but SqlInternalConnectionTds.EnlistNonNull assigns EnlistedTransaction = transaction even when promotable delegation succeeds. null is specific to the detached half-state involved in #4001, not to every delegated root; leaving this as an invariant obscures why the pool normally parks roots and why this failure window is exceptional. Please scope the statement to the post-detachment state.

- `IsTransactionRoot` → `true`
- `EnlistedTransaction` → **`null`**

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

• This fixes a serious pool-corruption regression, but no regression test is included for the new delegated-root branch. The existing transaction suite passes with both the pre-#3019 and #3019 conditions, so it cannot detect either side of this union. Please add a deterministic test (or a focused harness) that fails with both buggy conditions and verifies rollback leaves the pooled connection usable; otherwise this behavior can regress again without CI detecting it.

This issue also appears in the following locations of the same file:

• line 3948
• line 3948

                _parser.PrepareResetConnection(
                    Pool is not null &&
                    (IsTransactionRoot || EnlistedTransaction is not null));

[Copilot]

Pull request overview

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

Suppressed comments (4)

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

• Please retain the pre-#3019 server-version guard on the delegated-root branch. The old IsNonPoolableTransactionRoot logic required Is2008OrNewer && Pool != null for a root to request transaction-preserving reset; this union removes that guard, so a pooled delegated root connected to supported SQL Server 2005 (Is2008OrNewer == false) now sends ST_RESET_CONNECTION_PRESERVE_TRANSACTION. Keep the guard only for IsTransactionRoot while leaving the enlisted-participant branch unchanged.

                _parser.PrepareResetConnection(
                    Pool is not null &&
                    (IsTransactionRoot || EnlistedTransaction is not null));

doc/design-notes/4001-delegated-transaction-reset.md:48

• This note states that delegated roots always have a null EnlistedTransaction, but SqlConnectionInternal.EnlistNonNull assigns that property after a successful delegated enlistment; the same note also lists the true/set combination as reachable. The null value is the edge state relevant to #4001, not a general invariant, so please rewrite this section to distinguish the cases by their intended role and explain why both indicators are checked.

- `IsTransactionRoot` → `true`
- `EnlistedTransaction` → **`null`**

The `null` is not an oversight. There is no external transaction object to point at,
because the transaction *is here*.

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

• The normal EnlistNonNull path assigns EnlistedTransaction after both successful delegation and participant enlistment, so a delegated root is not intrinsically defined by a null value here. The null state can occur during the cleanup/detach transition described by #4001, but this comment's unconditional wording conflicts with the implementation and can mislead future changes; describe it as an edge state that requires checking IsTransactionRoot rather than as an invariant.

                //  - This connection is the root of a delegated transaction. The transaction has been
                //    delegated to (and lives on) this connection, so it has no EnlistedTransaction.
                //  - This connection merely enlisted in someone else's transaction, in which case
                //    EnlistedTransaction is set but the connection is not the root.

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

• The changed predicate has no regression test for the delegated-root close/reset/rollback state. The existing transaction suite passes with both the pre-#3019 and #3019 predicates, as documented in this PR, so CI will not detect a reintroduction of #4001. Add a deterministic regression test for the IsTransactionRoot == true case (and retain coverage for the enlisted-participant case), or extract the decision into a unit-testable helper and cover both combinations.

                _parser.PrepareResetConnection(
                    Pool is not null &&
                    (IsTransactionRoot || EnlistedTransaction is not null));