Reset session isolation level on pool return (fixes #96)

[priyankatiwari08]

Fixes #96.

Problem

SqlTransaction and TransactionScope leave the underlying SQL Server
session with the elevated isolation level after Commit/Rollback. Because
sp_reset_connection does not reset the session isolation level, the next
user of the pooled physical connection silently inherits it.

Repro

• using var c = new SqlConnection(cs); c.Open();
• using var tx = c.BeginTransaction(IsolationLevel.Serializable); tx.Rollback();
• Dispose, reopen on the same pooled SPID — sys.dm_exec_sessions.transaction_isolation_level is still 4 (Serializable).

Reproduced on 5.2.2 and 7.0.1 against SQL Server, on both net8.0 and net472.

Fix

• Track in SqlInternalConnectionTds when a non-default isolation level was set via the TM Begin path (_isolationLevelDirty).
• In Activate() (the pool-checkout path), before enlistment and only when the connection is not already enlisted, issue SET TRANSACTION ISOLATION LEVEL READ COMMITTED; via TdsExecuteSQLBatch, bounded by the connection's Connect Timeout.
• On a plain T-SQL rejection the connection degrades gracefully and is still pooled (some SQL Server-compatible endpoints, e.g. Azure Synapse dedicated SQL pools, only accept READ UNCOMMITTED). Transport-level failures doom the connection so it is destroyed rather than pooled.

Why checkout and not pool return

Two earlier revisions put the batch on the return side — first in
ResetConnection(), then in Deactivate(). Both are wrong, for different
reasons.

ResetConnection() is also called by the pool from
ChannelDbConnectionPool.PutObjectFromTransactedPool, which runs on the
System.Transactions transaction-completion callback thread while holding a
lock on the connection — a call site that deliberately declines even a
non-blocking socket poll on a thread it does not own. ResetConnection()
previously performed no network I/O at all, and that was load-bearing.

Deactivate() avoids that thread, but the connection may still be enlisted
in a live TransactionScope there, because Close() is routinely called inside
the scope. Instrumenting the call site confirmed it:

RESET enlisted=False pool=True  txnRoot=False   <- SqlTransaction test
RESET enlisted=True  pool=True  txnRoot=True    <- TransactionScope test

Issuing SET in that state downgrades the isolation level for the next
connection vended into the same scope from the transacted pool — which is
exactly the defect tracked by #146, except this would make it universal rather
than Azure-only.

Activate() is subject to neither constraint: it always runs on the thread
performing the checkout, the previous transaction has ended by then, and every
vend passes through it. The EnlistedTransaction is null gate keeps it off the
re-attach path that #4335 owns. _isolationLevelDirty is an instance field, so
it survives pool residency, and the pending sp_reset_connection still rides
this batch's TDS header.

Cost

One extra round trip on Open(), paid only when a previous Begin raised
the session isolation level and the connection is actually reused.

An earlier version of this description claimed there was no extra round trip.
That was wrong. PrepareResetConnection performs no I/O of its own — it sets a
flag that TdsParser.CheckResetConnection consumes at the next packet write —
so the legacy close path sent nothing at all. The queued sp_reset_connection
does ride this batch's TDS header rather than the caller's first command, so the
reset is not billed twice, but the batch itself is new cost.

Kill switch

Switch.Microsoft.Data.SqlClient.UseLegacyIsolationLevelBehavior (default false) restores the previous behavior.

Tests

Adds IsolationLevelLeakTest under ManualTests/SQL/TransactionTest/:

• SqlTransaction_SerializableDoesNotLeakAcrossPool
• TransactionScope_SerializableDoesNotLeakAcrossPool
• TransactionScope_SecondConnectionInSameScopeKeepsIsolationLevel — regression guard for the Wrong isolation level with Sql Azure and TransactionScope #146 interaction; verified to fail without the enlistment gate
• LegacySwitch_PreservesOldLeakBehavior (negative test)

Validation

Built clean for net462 / net8.0 / net9.0, 0 warnings. All four tests plus the
nine sibling TransactionTest tests pass against a local SQL Server 2022. With
the legacy switch enabled, [After] is Serializable (kill-switch verified).

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Pull request overview

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

[apoorvdeshmukh]

Can you look into this test failure?
Seems related to the changes

[apoorvdeshmukh]

Can you look into this failure for LegacySwitch_PreservesOldLeakBehavior?
Tests are mostly failing with this one. You can put the PR into draft till this is addressed.
Is this happening only in CI or do you observe it locally under VS as well?

[Copilot]

Pull request overview

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

[edwardneal]

It's worth noting that Azure Synapse Analytics does things slightly differently - it only supports the SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED statement, and the default isolation level can vary based upon a database option.

I think this might also present an issue for #4335.

[priyankatiwari08]

Good catch, thanks. Addressed in 645939a, though with graceful degradation rather than endpoint detection.

ResetSessionIsolationLevel now:

1. Clears _isolationLevelDirty up front, before the try, so an endpoint that rejects the statement isn't retried on every subsequent pool return.
2. Catches SqlException in a new when (!IsConnectionDoomed) clause that traces via SqlClientEventSource and does not doom the connection. The pre-existing catch (Exception e) when (ADP.IsCatchableExceptionType(e)) -> DoomThisConnection() remains for transport-level failures.

This is safe because sp_reset_connection is carried as a header flag that the server processes before the batch body, so whatever reset the endpoint natively performs has already taken effect by the time the SET statement errors.

I went with error handling instead of detecting Synapse because there's no cached EngineEdition on the connection, so detection at deactivate time would cost an extra query. ADP.IsAzureSynapseOnDemandEndpoint exists but only matches on-demand/serverless and Fabric DW endpoints, not dedicated SQL pools.

On the default-level-varies-by-database-option point: agreed, this is a real behavioural difference. Resetting to READ COMMITTED is what sp_reset_connection does on SQL Server / Azure SQL DB, but on an endpoint whose configured default is something else we'd now be normalising to READ COMMITTED instead of that endpoint's default. Happy to gate the whole reset behind an engine-edition check if you'd prefer correctness over the extra round trip - let me know which you'd rather see.

Also worth flagging for #4335.

[edwardneal]

I've tested against a few more unusual TDS endpoints and have three more detailed pieces of information.

First: this specifically only raises an error on a Synapse dedicated SQL pool - the serverless pools are absolutely fine. Your point about ADP.IsAzureSynapseOnDemandEndpoint is completely right. When it does throw an error, its error number 104409: "Setting IsolationLevel to [ReadCommitted/RepeatableRead/Snapshot/Serializable] is not supported".

Second: if I run ALTER DATABASE [] SET READ_COMMITTED_SNAPSHOT ON to change a Synapse database's transaction isolation level, Synapse continues to throw the same error if I run SET TRANSACTION ISOLATION LEVEL READ COMMITTED. I have no way to check what the current transaction level is, but the documentation states:

Once enabled, all transactions in this database are executed under READ COMMITTED SNAPSHOT ISOLATION and the setting READ UNCOMMITTED at the session level isn't honored.

READ COMMITTED thus isn't just a default - it's the only available option. Catching the exception should be absolutely fine, Synapse won't perform any behavioural changes as the result of the statement. Filtering to only catch exceptions where the error number is 104409 seems sensible too.

Third: connecting to the TDS endpoint for Microsoft Dataverse throws an exception when any kind of transaction isolation level is set. I don't think we'll ever hit this code path under that circumstance, and I think #4335 is also safe - the documentation for this endpoint confirms that it doesn't support transactions.

---

With all of the above noted, a top-level guard to avoid the extra server round-trip might be a good idea too. Synapse dedicated SQL pool endpoints end in .sql.azuresynapse.net, but don't end in -ondemand.sql.azuresynapse.net. Could we also return early if this heuristic matches?

[Copilot]

Pull request overview

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

Suppressed comments (3)

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

• ResetSessionIsolationLevel() can run while the connection is being returned to a transaction-preserving pool (EnlistedTransaction != null). In that case, issuing SET TRANSACTION ISOLATION LEVEL READ COMMITTED can change the isolation level for subsequent work in the same ambient transaction, which is a behavioral regression for TransactionScope scenarios that open/close multiple connections within a single scope.

                if (_isolationLevelDirty && !LocalAppContextSwitches.UseLegacyIsolationLevelBehavior)
                {
                    ResetSessionIsolationLevel();
                }

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

• Manual tests are partitioned by the xUnit Trait("Set", ...) filter (see build.proj TestSetFilter). Without a Set trait, these new tests may be skipped in CI when manual tests are run with -p:TestSet=....

    public static class IsolationLevelLeakTest

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

• PR description says: "On failure, the connection is doomed so it's destroyed instead of pooled." But the implementation explicitly does not doom the connection for non-dooming SqlExceptions (it logs and allows pooling). Please align the PR description with the intended behavior, or change the code to doom on this failure if that was the requirement.

            catch (SqlException) when (!IsConnectionDoomed)
            {
                // The server rejected the statement but the session itself is healthy (the parser
                // dooms the connection for transport-level failures, so reaching here means a plain
                // T-SQL error). Not every SQL Server-compatible endpoint accepts this statement:
                // Azure Synapse Analytics dedicated SQL pools, for example, only support
                // SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED.
                //
                // sp_reset_connection still ran, because it is carried as a flag on the header of
                // the batch above and is processed by the server before the batch body. The
                // connection is therefore no less clean than it was under the legacy behavior, so
                // degrade gracefully and let it be pooled rather than destroying it.
                SqlClientEventSource.Log.TryTraceEvent(
                    "<sc.SqlInternalConnectionTds.ResetSessionIsolationLevel|ADV> {0}, " +
                    "server rejected the session isolation level reset; leaving the session " +
                    "isolation level unchanged.",
                    ObjectID);
            }

[codecov]

Codecov Report

❌ Patch coverage is 71.79487% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 62.89%. Comparing base (ee529d4) to head (645939a).
⚠️ Report is 1 commits behind head on main.

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4330      +/-   ##
==========================================
- Coverage   64.78%   62.89%   -1.89%     
==========================================
  Files         288      283       -5     
  Lines       44418    67449   +23031     
==========================================
+ Hits        28774    42422   +13648     
- Misses      15644    25027    +9383     

Flag	Coverage Δ
CI-SqlClient	?
PR-SqlClient-Project	62.89% <71.79%> (?)

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 #4335) explaining why this PR and #4335 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:

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

Neither subsumes the other: this PR never runs while the scope is still open (and would write the wrong level if it did), and #4335 only fires on the re-enlist branch, which the #96 repro — no live transaction, often plain SqlTransaction — never reaches.

The two PRs do overlap textually (same file, same switches helper, same test folder), so they should be sequenced. Suggested order is this PR first (broader blast radius, self-contained on the deactivate path), then rebase #4335 on top.

[Copilot]

Pull request overview

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

Suppressed comments (1)

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

• PR description says that if resetting the isolation level fails "the connection is doomed so it's destroyed instead of pooled", but the implementation intentionally does not doom the connection on a T-SQL SqlException (it logs and allows pooling). Please align the PR description with the actual behavior (or adjust the code if dooming-on-any-failure is the intended contract).

            catch (SqlException) when (!IsConnectionDoomed)
            {
                // The server rejected the statement but the session itself is healthy (the parser
                // dooms the connection for transport-level failures, so reaching here means a plain
                // T-SQL error). Not every SQL Server-compatible endpoint accepts this statement:
                // Azure Synapse Analytics dedicated SQL pools, for example, only support
                // SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED.
                //
                // sp_reset_connection still ran, because it is carried as a flag on the header of
                // the batch above and is processed by the server before the batch body. The
                // connection is therefore no less clean than it was under the legacy behavior, so
                // degrade gracefully and let it be pooled rather than destroying it.
                SqlClientEventSource.Log.TryTraceEvent(
                    "<sc.SqlInternalConnectionTds.ResetSessionIsolationLevel|ADV> {0}, " +
                    "server rejected the session isolation level reset; leaving the session " +
                    "isolation level unchanged.",
                    ObjectID);
            }

[Copilot]

Pull request overview

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

Suppressed comments (1)

src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/LocalAppContextSwitchesTest.cs:38

• TestDefaultAppContextSwitchValues asserts UseOverallConnectTimeoutForPoolWait later in the test, but its cached field is not reset to null in the initial reset block. If another test has already cached or mutated this switch via LocalAppContextSwitchesHelper, this test can become order-dependent. Reset it alongside the other switches.

        switchesHelper.EnableMultiSubnetFailoverByDefault = null;
        switchesHelper.IgnoreServerProvidedFailoverPartner = null;
        switchesHelper.UseLegacyFailoverAlternationOnLoginSqlErrors = null;
        switchesHelper.UseLegacyIsolationLevelBehavior = null;
        switchesHelper.LegacyRowVersionNullBehavior = null;

[Copilot]

Pull request overview

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

Suppressed comments (1)

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

• ResetSessionIsolationLevel intends to bound the round-trip by Connect Timeout, but ConnectTimeout can legally be 0 (no timeout). Passing 0 here will still map to an effectively-infinite timeout (as the comment notes), which means SqlConnection.Close() can hang indefinitely on an unresponsive server. Clamp the timeout to a minimum > 0 (or substitute a sane default when ConnectTimeout==0).

                    // Bounded by the connection's own connect timeout. Passing 0 here would map to
                    // long.MaxValue in TdsParserStateObject.SetTimeoutMilliseconds, which would let
                    // an unresponsive server block SqlConnection.Close indefinitely.
                    timeout: ConnectionOptions.ConnectTimeout,

[Copilot]

Pull request overview

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

Suppressed comments (3)

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

• The comment says the isolation level will be "reset on deactivate", but the implementation actually resets on the next checkout via Activate() (ResetSessionIsolationLevel is called from Activate). Please update the comment to match the actual behavior so future maintenance doesn’t get misled.

                // A successful Begin with a non-default isolation level mutates
                // SQL Server's session transaction_isolation_level. sp_reset_connection
                // will not undo this, so remember it and reset on deactivate.
                if (requestType == TdsEnums.TransactionManagerRequestType.Begin &&

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

• The PR title/description says the isolation level reset happens on pool return / Deactivate(), but the code change performs the reset on pool checkout in Activate() (before enlistment) when _isolationLevelDirty is set. Please update the PR title/description (and the "Fix" section) to match the implementation, or adjust the code if Deactivate-on-return is still the intended behavior.

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

            // sp_reset_connection does not reset the session transaction isolation level, so when a
            // previous Begin raised it we scrub it here, on checkout.
            //
            // This runs on checkout rather than on pool return for two reasons:
            //
            //  - On return the connection may still be enlisted in a live TransactionScope, because
            //    Close is routinely called inside the scope. Issuing SET there would downgrade the
            //    isolation level for any further connection vended into that same scope from the
            //    transacted pool, which is the defect tracked by #146.
            //  - ResetConnection, the other pool-return hook, is also invoked from
            //    PutObjectFromTransactedPool on the System.Transactions transaction-completion
            //    callback thread while holding a lock on the connection. That path deliberately
            //    avoids socket work on a thread it does not own.
            //
            // Activate always runs on the thread performing the checkout, and by then the previous
            // transaction has ended, so neither constraint applies. It also means the cost is only
            // paid by connections that are actually reused.
            //
            // EnlistedTransaction is non-null here only when the connection is being re-vended into
            // a transaction it is already enlisted in; scrubbing then would hit the same #146
            // problem, so it is skipped.
            if (_isolationLevelDirty &&
                !LocalAppContextSwitches.UseLegacyIsolationLevelBehavior &&
                EnlistedTransaction is null &&
                !IsConnectionDoomed)
            {
                ResetSessionIsolationLevel();
            }

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/IsolationLevelLeakTest.cs:20

• This new test file is missing the XML
  documentation required by the repository testing guidelines (see .github/instructions/testing.instructions.md:182-186). Please add

  comments to the test class and each test method (and any helper methods where helpful) so the test intent is preserved for maintainers.

    // SqlTransaction / TransactionScope used to leave the pooled connection
    // with the elevated session isolation level. The next user of the pooled
    // connection would silently inherit it. The fix resets the session
    // isolation level to READ COMMITTED when the connection is next taken out
    // of the pool.
    [Trait("Set", "3")]
    public static class IsolationLevelLeakTest
    {