Fix | Preserve delegated transactions when resetting a pooled connection - #4557

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

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

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

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

• In this XML doc comment, “server side” should be hyphenated as the compound adjective “server-side” for consistency with other comments (e.g., in TdsParser) and standard usage.

This issue also appears on line 4019 of the same file.

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

• In this XML doc comment, “server side” should be hyphenated as “server-side” (compound adjective).

        /// case resets the server side transaction out from under System.Transactions, which

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Lite

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Lite

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionInternalResetTransactionTests.cs:45

• The PR description checklist says “Tests added or updated — no”, but this PR adds/updates unit tests (e.g., this new SqlConnectionInternalResetTransactionTests suite). Please update the PR description checklist/verification section so it matches the actual changes.

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Lite

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Lite

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Lite