Re-issue session isolation level on TransactionScope re-enlistment

[priyankatiwari08]

Summary

Fixes #146 by preserving the ambient TransactionScope isolation level when an enlisted physical connection is reused from the transacted pool.

On Azure SQL DB, sp_reset_connection_keep_transaction can reset the session isolation level. Re-enlistment in the same transaction previously short-circuited without restoring it, causing later opens in the scope to run at the database default.

Changes

• Re-assert the ambient isolation level when reattaching the same transaction with a reset pending.
• Skip ReadCommitted, where re-assertion is unnecessary.
• Use the connect timeout during open and the command timeout for explicit enlistment.
• Add manual tests: all supported isolation levels, session-override and Snapshot re-checkout, each across sync/async, MARS on/off and pool V1/V2.
• Add a benchmark for the re-assertion path.

The fix is unconditional; the previous behavior is a correctness bug and has no compatibility switch.

Performance

Non-ReadCommitted re-checkouts add one round trip. The benchmark measured approximately 0.15 ms per re-assertion on localhost; network latency determines the real-world cost.

Checklist

• Tests added or updated
• Public API changes documented — N/A, no public API changes
• Verified against customer repro
• Ensure no breaking changes introduced

[Copilot]

Pull request overview

Fixes an Azure SQL DB-specific TransactionScope pooling regression where session isolation level can revert to the database default after transacted-pool re-checkout by re-asserting the ambient isolation level during re-enlistment.

Changes:

• Added a new AppContext switch (Switch.Microsoft.Data.SqlClient.UseLegacyTransactionScopeIsolationBehavior) to gate the new re-assert behavior.
• Updated SqlInternalConnectionTds.Enlist(Transaction) to re-issue SET TRANSACTION ISOLATION LEVEL ... on the “same transaction” short-circuit path (intended to piggyback on the pending reset).
• Added new Azure-gated ManualTests and wired them into the ManualTests project.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File	Description
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs	Adds re-attach logic to re-assert session isolation level when re-enlisting into the same ambient transaction.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/LocalAppContextSwitches.cs	Introduces a new AppContext switch to enable legacy behavior.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/TransactionScopeIsolationReassertTest.cs	Adds ManualTests validating isolation level stability across pooled re-opens inside a TransactionScope (Azure-only).
src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTests.csproj	Includes the new ManualTests source file in the build.

[Copilot]

Pull request overview

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

Suppressed comments (2)

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

• The PR description states the isolation re-assert is queued onto the same batch as the pending reset with no extra round trip, but this implementation runs a synchronous TdsExecuteSQLBatch + Run during Open/Enlist, which introduces an additional server round trip on the re-attach path. Please reconcile the PR description with the actual behavior, or adjust the implementation to truly piggyback without an extra Open-time execute.

            else if (!LocalAppContextSwitches.UseLegacyTransactionScopeIsolationBehavior
                     && _parser._fResetConnection)
            {
                // Same System.Transactions transaction being re-attached to the same
                // pooled physical connection (transacted-pool re-checkout inside an
                // open TransactionScope). The queued sp_reset_connection_keep_transaction
                // does not preserve the SQL Server session isolation level on every
                // server (notably Azure SQL DB), so without re-asserting the level the
                // second and later opens inside the scope would silently run at the
                // database default. The queued reset piggybacks this batch's TDS
                // header, so the reset itself costs nothing extra, but the SET batch
                // is an additional round trip on re-checkout.
                ReassertSessionIsolationLevel(transaction.IsolationLevel);

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/TransactionScopeIsolationReassertTest.cs:19

• ManualTests in this folder are partitioned with [Trait("Set", "3")] (see TransactionTest.cs / TransactionEnlistmentTest.cs / DistributedTransactionTest.cs). This new test class is missing the trait, which can cause it to run outside the intended ManualTests set partitioning.

    public static class TransactionScopeIsolationReassertTest
    {

[codecov]

Codecov Report

❌ Patch coverage is 82.92683% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.55%. Comparing base (671d010) to head (d8f6045).
⚠️ Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
...Data/SqlClient/Connection/SqlConnectionInternal.cs	82.92%	7 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (671d010) and HEAD (d8f6045). Click for more details.

HEAD has 1 upload less than BASE

Flag	BASE (671d010)	HEAD (d8f6045)
CI-SqlClient	1	0

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4335      +/-   ##
==========================================
- Coverage   71.96%   64.55%   -7.41%     
==========================================
  Files         291      285       -6     
  Lines       45110    68100   +22990     
==========================================
+ Hits        32462    43965   +11503     
- Misses      12648    24135   +11487     

Flag	Coverage Δ
CI-SqlClient	?
PR-SqlClient-Project	64.55% <82.92%> (?)

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.

[priyankatiwari08]

Added a design note at specs/007-session-isolation-level/design.md (identical file in #4330) explaining why this PR and #4330 are not duplicate fixes.

Short version: both bugs come from the same fact — sp_reset_connection clears the session transaction_isolation_level on Azure SQL DB but not on on-prem SQL Server — but they sit on opposite sides of it:

	#146 / this PR	#96 / #4330
Failure	Level is cleared when it should persist (silent downgrade)	Level persists when it should be cleared (leak)
Transaction state	Still open / ambient	Already completed
Who is harmed	The same caller, next Open() in the scope	An unrelated later pool consumer
Servers	Azure SQL DB only	On-prem SQL Server
API surface	TransactionScope only	SqlTransaction and TransactionScope
Code path	Enlist() — pool checkout	ResetConnection() — pool return
T-SQL emitted	SET ... <ambient level> (dynamic)	SET ... READ COMMITTED (fixed)
Direction	Re-assert session state	Scrub session state
Snapshot	Deliberately skipped (would roll back the open txn)	Reset like any other level

Neither subsumes the other: this PR only fires on the Enlist() same-transaction re-attach branch, which the #96 repro — no live transaction, often plain SqlTransaction — never reaches; and #4330 only runs on pool return and only ever writes READ COMMITTED, which is the wrong level for the ambient scope here.

The two PRs do overlap textually (same file, same switches helper, same test folder). Suggested sequencing is #4330 first, then rebase this one on top and settle the open perf question (unconditional SET vs. Azure-gated vs. deferring the SET to prefix the user's next batch).

[Copilot]

Pull request overview

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

Suppressed comments (1)

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/TransactionScopeIsolationReassertTest.cs:18

• ManualTests are partitioned by the xUnit Set trait (see build.proj TestSetFilter). This new test class doesn’t declare a Set, so it may be skipped when CI runs specific manual test sets (e.g., Set=1/2/3). Add the same [Trait("Set", "3")] used by other transaction manual tests so these new tests reliably execute in the manual test matrix.

    public static class TransactionScopeIsolationReassertTest

[Copilot]

Pull request overview

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

[mdaigle]

I want to add a comprehensive test suite that covers the entire possible public API surface. We should test every possible interleaving of calls and assert the expected outcome. That will help us ensure we don't regress any other paths during this fix. I'll post a PR for that suite when it's ready and tag it here.

[priyankatiwari08]

Synapse / Fabric DW validation — decision

#4330 needed a live Synapse + Fabric DW run because its reset forces SET TRANSACTION ISOLATION LEVEL READ COMMITTED, which a dedicated SQL pool rejects with error 104409. This PR does not need the same run, for three reasons:

1. The re-assert fires only when re-enlisting into a transaction the connection is already enlisted in, and it re-asserts the ambient scope's own level — it never introduces a level the caller didn't ask for.
2. It early-returns for ReadCommitted, Unspecified and Chaos, so the only levels it can ever SET are ReadUncommitted, RepeatableRead, Serializable and Snapshot.
3. Synapse dedicated pools, Synapse serverless and Fabric DW all support only READ UNCOMMITTED (dedicated additionally exposes READ_COMMITTED_SNAPSHOT as a database option, not a session level). RepeatableRead/Serializable/Snapshot cannot be established on those endpoints in the first place, so the re-assert is unreachable there; the one reachable level is the endpoint default, making it a no-op.

So there is no code path this PR adds that a Synapse or Fabric DW endpoint can reach, and no endpoint guard is warranted — adding one would suppress nothing. This supersedes my earlier note deferring the Synapse run. Automated endpoint coverage stays tracked in #4580.

Test suite

TransactionScopeIsolationReassertTest now covers 24 cases, all passing locally against SQL Server 2022 (16.0.1200.5):

Theory	Matrix
Base re-assert, sync + async	4 isolation levels each
Session override, sync + async	MARS on/off x pool V1/V2
Snapshot across pool reuse, sync + async	MARS on/off x pool V1/V2

This ports the meaningful coverage from @mdaigle's draft suite in #4636.

Two server behaviours shaped the final shape of these tests and are worth recording:

• MARS batch scoping. Under MultipleActiveResultSets=true, a SET TRANSACTION ISOLATION LEVEL issued inside a multi-statement batch applies only to that batch's execution environment and does not persist to the session. The override helpers therefore issue the SET as its own command before reading the level back. This is a test-harness detail, not a driver behaviour.
• Snapshot is sticky mid-transaction. SQL Server defers a SET TRANSACTION ISOLATION LEVEL issued inside an open snapshot transaction until that transaction ends, without raising an error. A session therefore cannot be moved off Snapshot mid-scope, so a hostile session-override variant is impossible for Snapshot; the valid coverage is repeated re-checkout under a Snapshot scope, which also proves the re-issued SET ... SNAPSHOT does not raise error 3951.

Each theory uses a distinct Application Name so it gets its own pool under Max Pool Size=1.

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate review findings block approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 · 1 · 1

Open (3)

• ReadCommitted reset check can retain an altered isolation level · New
• CommandTimeout is ignored during explicit transaction enlistment · New
• Add required XML documentation to the new manual tests · New

Resolved since last review (1)

• The inline comment and method summary refer to sp_reset_connection, but this re-attach path is…

[Copilot]

Copilot review overview

🟡 Changes recommended

Update the pool-version test scoping; timeout documentation and Unicode-style nits also remain.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 · 1

Open (2)

• Use ConnectionPoolVersionScope to isolate pool version tests · New
• Escape literal em dashes in source comments · New

Resolved since last review (3)

• ReadCommitted reset check can retain an altered isolation level
• CommandTimeout is ignored during explicit transaction enlistment
• Add required XML documentation to the new manual tests