Fix async cancellation failing to send TDS attention signal

[cheenamalhotra]

Description

Fixes #4424
Fixes #44

When using CancellationToken with async operations like ExecuteReaderAsync/ExecuteNonQueryAsync, cancellation fails to send a TDS attention signal to SQL Server if the server is blocked (e.g., infinite WHILE loop, WAITFOR DELAY, or partial results from RAISERROR WITH NOWAIT followed by a blocking operation). The cancellation hangs until the query completes naturally — which may be forever for infinite loops.

Root Cause

EndExecuteReaderAsync (and the equivalent NonQuery/XmlReader methods) held lock(_stateObj) while calling into EndExecute*Internal. When the server hadn't yet sent result metadata (blocked on a long-running or infinite query), TryConsumeMetaData performed a synchronous network read (_syncOverAsync = true), blocking the thread while holding the monitor lock.

Meanwhile, stateObj.Cancel() uses Monitor.TryEnter(this, 100ms) in a polling loop on the same stateObj instance. Since the lock was held by EndExecute*Async for the entire duration of the server-side wait, Cancel() could never acquire the monitor and never sent the attention signal.

Fix

Remove lock(_stateObj) from the user-facing async end methods:

• EndExecuteReaderAsync
• EndExecuteNonQueryAsync
• EndExecuteXmlReaderAsync

The lock is unnecessary in these paths because concurrent access is already handled by:

1. Cancel() — uses polling Monitor.TryEnter with parser state guards in its loop
2. Connection close — detected by TryRun checking parser Broken/Closed state
3. StateObj internals — handle their own synchronization for read/write operations

Also remove lock(_stateObj) from CreateLocalCompletionTask (the internal-end/retry path used when column encryption is enabled and parameter metadata came from the cache).

An earlier revision of this PR kept that lock and justified it on the grounds that the continuation runs only after the response is buffered. That was wrong. localCompletion is completed from BeginExecuteReaderInternalReadStage -> _stateObj.ReadSni(completion), which resolves as soon as a packet arrives. And FinishExecuteReader only early-returns on the _internalEndExecuteInitiated fast path when !isInternal, so the internal call falls through to a blocking TryRun(RunBehavior.UntilDone, ...). Holding the monitor across that read starves TdsParserStateObject.Cancel(), which polls Monitor.TryEnter for the same monitor to send attention.

Commit ac8acd72 had reverted this because AE TestCancellationToken failed. The exception on the unlocked path is a SqlException with Number == 0 and two errors (A severe error occurred on the current command. then Operation cancelled by user.) — a correct cancellation, where the first error is what TryRun adds on consuming the attention ack. The test asserted an exact whole-message match, so that assertion is relaxed to Assert.Contains rather than reverting the fix.

Testing

Which tests fail without the source fix?

Measured per-test against a live SQL Server, not inferred:

Test	Fails on main?	Evidence
CancellationSendsAttention_WhenPartialResultsReceived	Yes	RAISERROR ... WITH NOWAIT flushes, WAITFOR blocks; reader returned instead of cancelling
CancellationOfInfiniteWhileLoop_DoesNotHang	Yes	loop streams DONE tokens (~30k server writes in 6s), so the End path runs and blocks; cts.Cancel() never returns
CancellationOnInternalEndExecutePath_SendsAttention	Yes	reader returned 60005ms after cancellation was requested
CancellationDuringExecuteXmlReaderAsync_SendsAttention	Yes	reader returned 60014ms after cancellation was requested
CancellationDuringExecuteReaderAsync_SendsAttention	No	WAITFOR first, nothing flushed, End path never runs — genuine guard test

DataReaderCancellationTest totals: 4 failed / 3 passed on main; 7 passed in 1s with the fix.

Note on the infinite WHILE case: it is not merely slow on main. The token callback runs SqlCommand.Cancel() synchronously on the cancelling thread, so CancellationTokenSource.Cancel() itself blocks indefinitely (observed >20 minutes). With the fix it returns in 4ms.

Suggested release note entry

Release notes in this repo are batched into dedicated release PRs rather than added per bugfix, so no file is added here. Suggested entry:

Fixed — Fixed asynchronous cancellation failing to send a TDS attention signal when the server had already flushed part of a response, which caused CancellationToken cancellation of ExecuteReaderAsync, ExecuteNonQueryAsync and ExecuteXmlReaderAsync to be ignored until the command completed on its own. In the worst case CancellationTokenSource.Cancel() itself would block indefinitely. (#4424, #4435)

• Added manual test for partial results scenario (severity 10 RAISERROR WITH NOWAIT + WAITFOR + CancellationToken) — fails pre-fix
• Added manual test for cancellation during ExecuteReaderAsync (WAITFOR as first statement)
• Added manual test for the infinite WHILE loop repro from Canceling SQL Server query with while loop hangs forever #44 — fails pre-fix
• Added AE manual test exercising EndExecuteReaderAsync with AE-enabled commands (cache warmup + parameterized WAITFOR delay)
• Added CancellationOnInternalEndExecutePath_SendsAttention (forces the internal-end path via the DEBUG _forceInternalEndQuery hook, so it runs without an AE setup and skips against a Release driver) — fails pre-fix
• Added CancellationDuringExecuteXmlReaderAsync_SendsAttention, closing the ExecuteXmlReaderAsync coverage gap (FOR XML RAW; FOR XML AUTO fails at compile time before RAISERROR runs) — fails pre-fix
• Relaxed AE TestCancellationToken exact-message assertion to Assert.Contains (attention ack adds a leading error)
• Observed the abandoned execTask on the watchdog branch to avoid unobserved task exceptions leaking into unrelated tests
• Cancellation in the partial-results test is synchronized on InfoMessage arrival (asserted), not a fixed timer
• All existing unit tests pass (904/908; 4 pre-existing failures unrelated to this change)
• Existing AE cancellation tests (TestSqlCommandCancellationToken) pass
• Public API unchanged — no ref assembly updates needed
• Verified against customer repro

Changes

File	Change
SqlCommand.Reader.cs	Removed lock(_stateObj) in EndExecuteReaderAsync
SqlCommand.NonQuery.cs	Removed lock(_stateObj) in EndExecuteNonQueryAsync
SqlCommand.Xml.cs	Removed lock(_stateObj) in EndExecuteXmlReaderAsync
SqlCommand.cs	Removed lock(_stateObj) in CreateLocalCompletionTask (internal-end path)
DataReaderCancellationTest.cs	Added CancellationSendsAttention_WhenPartialResultsReceived, CancellationDuringExecuteReaderAsync_SendsAttention, CancellationOfInfiniteWhileLoop_DoesNotHang, CancellationOnInternalEndExecutePath_SendsAttention and CancellationDuringExecuteXmlReaderAsync_SendsAttention tests
CommandHelper.cs / DataTestUtility.cs	Exposed the DEBUG _forceInternalEndQuery hook so the internal-end path can be tested without an AE setup
ApiShould.cs	Added TestAsyncCancellationSendsAttention_WithAlwaysEncryptedCommand test

[Copilot]

Pull request overview

This PR fixes a deadlock/hang in async cancellation by removing lock(_stateObj) from the async EndExecute* paths so that TdsParserStateObject.Cancel() can acquire the monitor and send a TDS attention signal even when the async end-path is blocked on synchronous network reads (e.g., partial results + WAITFOR).

Changes:

• Removed lock(_stateObj) from EndExecuteReaderAsync, EndExecuteNonQueryAsync, and EndExecuteXmlReaderAsync to avoid blocking cancellation’s attention-send path.
• Added a new manual regression test intended to reproduce the “partial results + server blocked” cancellation hang scenario from #4424.

Reviewed changes

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

File	Description
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.Reader.cs	Removes lock(_stateObj) from EndExecuteReaderAsync and documents rationale tied to #4424.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.NonQuery.cs	Removes lock(_stateObj) from EndExecuteNonQueryAsync to keep cancellation from being blocked by the monitor.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.Xml.cs	Removes lock(_stateObj) from EndExecuteXmlReaderAsync consistent with reader/nonquery.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderCancellationTest.cs	Adds a manual regression test for cancellation sending attention after partial results are received.

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[mdaigle]

Agreed on tests. But code looks fine.

[Copilot]

Pull request overview

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

[paulmedynski]

Maybe I'm misunderstanding, but I don't see how these tests confirm what we're trying to confirm. Would it be possible to use the simulated server to check that we sent ATTENTION as expected?

[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/tests/ManualTests/SQL/DataReaderTest/DataReaderCancellationTest.cs:130

• The InfoMessage handler is attached after ExecuteReaderAsync is started, so the RAISERROR WITH NOWAIT message can be received before the handler is subscribed. That makes the cancellation timing nondeterministic and can let the test cancel without ever confirming the “partial results received” state that #4424 regressed. Subscribe to InfoMessage (and create the TaskCompletionSource) before starting ExecuteReaderAsync, then start the exec task.

                    Task<SqlDataReader> execTask = command.ExecuteReaderAsync(cts.Token);

                    // Cancel only after the server has flushed the RAISERROR NOWAIT packet so we know we're in the
                    // "partial results received" state that regressed in #4424.
                    var infoMessageReceived = new TaskCompletionSource<bool>(TaskCreationOptions.RunContinuationsAsynchronously);

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.cs:2461

• The new comment claims the internal-end continuation runs only after “the data is already buffered”, but localCompletion is completed after a single ReadSni callback processes one packet; endFunc can still perform synchronous reads (e.g., FinishExecuteReader/TryRun) while holding lock(_stateObj). That lock can therefore still delay stateObj.Cancel() sending attention, so the current wording is misleading. Update the comment to reflect that the lock may still cover blocking reads and can delay cancellation.

                        // because this continuation runs after the initial async I/O has
                        // already completed — the blocking metadata read that caused #4424
                        // in the user-facing path does not apply here since the data is
                        // already buffered by the time this continuation fires.

[priyankatiwari08]

Three smaller items:

• No ExecuteXmlReaderAsync coverage. SqlCommand.Xml.cs gets the same lock removal but no test. One partial-results test with FOR XML AUTO would bring the three paths to parity.
• Unobserved task. On the Assert.Fail branches execTask is abandoned; if it faults later it surfaces in an unrelated test. finally { try { await execTask; } catch { } } fixes it.
• Release note. Add an entry under release-notes/ per repo convention — including the scope caveat if the AE/internal-end path isn''t covered here.

[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/SqlCommand.cs:2461

• The rationale in this comment is inaccurate: CreateLocalCompletionTask runs after the initial async SNI read stage (a single response packet) completes, which can still be a partial response (e.g., an INFO token from RAISERROR ... WITH NOWAIT). In that case, endFunc may still need additional reads, and if stateObj._syncOverAsync becomes true (e.g., InfoMessage handler is invoked), those reads can become synchronous while this lock is held—potentially blocking Cancel() again in the same way as #4424. Please reword to avoid asserting that the blocking metadata read “does not apply” here, and clarify the actual constraint (avoid sync-over-async work while holding this lock).

                        // This is retained here (unlike the user-facing EndExecute* methods)
                        // because this continuation runs after the initial async I/O has
                        // already completed — the blocking metadata read that caused #4424
                        // in the user-facing path does not apply here since the data is
                        // already buffered by the time this continuation fires.

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderCancellationTest.cs:143

• This regression test can still take the full 60s WAITFOR duration when cancellation/attention regresses, because it awaits execTask directly without a watchdog. Adding a hard timeout (similar to CancellationOfInfiniteWhileLoop_DoesNotHang) will fail faster and avoid long manual-test hangs, while still preserving the <30s correctness assertion.

                        using (var reader = await execTask)
                        {
                            // If we reach here, cancellation failed to abort ExecuteReaderAsync while it was waiting
                            // for metadata after a partial response (e.g., RAISERROR WITH NOWAIT).
                            Assert.Fail("ExecuteReaderAsync should have been cancelled before returning a reader.");
                        }

[mdaigle]

It seems possible to construct a query that blocks on the lock(_stateObj) within CreateLocalCompletionTask. It requires returning partial results before a long-running query begins:
https://github.com/dotnet/SqlClient/pull/4546/changes#diff-0d3a32e4d3e4275647635a2e33c065b8e39d69a47ec53655303a6e26a0667d85

I'll leave it up to you if you're comfortable touching that lock right now. The existing changes are still an improvement as-is, but I think we should either include a fix for this case as well, or modify the comment on the CreateLocalCompletionTask lock to indicate that this is future work instead of stating that the lock is desired behavior.

[cheenamalhotra]

Thanks Malcolm — you were right, and I've included the fix rather than downgrading the comment to "future work". My justification comment was simply wrong, and I've verified that against a live SQL Server rather than reasoning about it.

Where my comment was wrong. I claimed the continuation "runs after the initial async I/O has already completed... the data is already buffered". localCompletion is completed from BeginExecuteReaderInternalReadStage → _stateObj.ReadSni(completion), which resolves as soon as a packet arrives, not once the response is buffered. And FinishExecuteReader only early-returns on the _internalEndExecuteInitiated fast path when !isInternal, so the internal call falls straight through to TryRun(RunBehavior.UntilDone, ...) under Debug.Assert(_stateObj._syncOverAsync). That is a blocking read, and TdsParserStateObject.Cancel() needs the very same monitor (Monitor.TryEnter(this, ...) polling loop) to send attention.

Measured, cancelling ~1s into RAISERROR(...) WITH NOWAIT; WAITFOR DELAY '00:01:00'; SELECT ... on the internal-end path:

	result
lock retained	60005ms, no exception, ExecuteReaderAsync returns a reader
lock removed	cancels promptly, SqlException with Number == 0

Full DataReaderCancellationTest class: 4 failed / 3 passed against main, 7 passed in 1s with the fix.

On the ac8acd72 revert. That commit reverted this lock because TestCancellationToken failed. I probed the actual exception on the unlocked path: SqlException, Number == 0, two errors — A severe error occurred on the current command. followed by Operation cancelled by user. That is a correct cancellation (the first error is what TryRun adds when it consumes the attention ack), and the test was asserting an exact whole-message match. I've relaxed it to Assert.Contains("Operation cancelled by user.", ...) rather than reverting the fix again.

Tests for this path came from your linked #4546 (the _forceInternalEndQuery DEBUG hook one runs without an AE setup, and skips against a Release driver so the lost coverage is visible). I've merged those in here so the fix and its proof land together — #4546 can be closed.

[cheenamalhotra]

All three addressed — thanks.

1. ExecuteXmlReaderAsync coverage — added CancellationDuringExecuteXmlReaderAsync_SendsAttention, which brings the three paths to parity. It fails on main (reader returned 60014ms after cancellation) and passes with the fix.

One gotcha worth recording: FOR XML AUTO doesn't work for this test. It fails at compile time with "FOR XML AUTO requires at least one table for generating XML tags", so the batch never executes and RAISERROR ... WITH NOWAIT never fires — the test then silently wasn't in the partial-results state. The InfoMessage assertion added earlier caught exactly that. Switched to FOR XML RAW, verified: InfoMessage at 25ms, reader blocked until the delay expired.

2. Unobserved task — fixed on the Assert.Fail/watchdog branch. I used a finally that only observes the fault (ContinueWith(static t => _ = t.Exception)) rather than await execTask, since awaiting there would re-block for the full duration in precisely the regression case the watchdog exists to escape.

3. Release note — I checked the convention before adding a file, and release notes here are batched into dedicated release PRs ([v7.1.0-preview2] Release Notes and vBump #4422, Add release notes for 7.0.2 #4403), not added per bugfix PR. The last shipped version is 7.1.0-preview2 and there's no open file for the next one, so creating 7.1.0-preview3.md here would guess the version and collide with that process. Per the repo AI instructions ("add a suggested release-note entry in the PR description or via the release-notes workflow/prompt") I've put the entry in the PR description instead:

Fixed — Fixed asynchronous cancellation failing to send a TDS attention signal when the server had already flushed part of a response, which caused CancellationToken cancellation of ExecuteReaderAsync, ExecuteNonQueryAsync and ExecuteXmlReaderAsync to be ignored until the command completed on its own. In the worst case CancellationTokenSource.Cancel() itself would block indefinitely. (#4424, #4435)

Happy to add the file if you'd rather I pick a version.

On scope: the AE/internal-end path is now covered and fixed in this PR rather than deferred, so the caveat you mentioned no longer applies.

[Copilot]

Pull request overview

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

Suppressed comments (5)

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderCancellationTest.cs:409

• This comment assumes the internal-end continuation is running "inside lock (_stateObj)", but the lock has been removed. Updating this avoids misleading future investigations of failures/flakiness.

                        // Let the RAISERROR NOWAIT packet land so the internal-end continuation is
                        // inside endFunc, and therefore inside lock (_stateObj), before we cancel.
                        await Task.WhenAny(infoMessageReceived.Task, Task.Delay(System.TimeSpan.FromSeconds(10)));

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderCancellationTest.cs:438

• The failure message for this assertion attributes a late reader to the internal-end path "held lock (_stateObj)", but the lock has been removed. The message should describe the observed behavior (attention not aborting promptly) rather than a no-longer-true implementation detail.

                        Assert.False(readerReturned,
                            $"ExecuteReaderAsync returned a reader {latency}ms after cancellation was requested. " +
                            "The internal-end path held lock (_stateObj) across a blocking read, so " +
                            "TdsParserStateObject.Cancel() could not send the attention signal.");

src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/ApiShould.cs:2494

• This assertion message blames a "retained lock (_stateObj)" for blocking attention, but that lock is removed in the current implementation. Updating the message to focus on observable behavior will keep test failures actionable.

                        Assert.False(readerReturned,
                            $"ExecuteReaderAsync returned a reader {latency}ms after cancellation was requested. " +
                            "The retained lock (_stateObj) in CreateLocalCompletionTask blocked " +
                            "TdsParserStateObject.Cancel() from sending the attention signal.");

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderCancellationTest.cs:367

• The summary claims CreateLocalCompletionTask "still takes lock (_stateObj)" and that cancellation is dropped, but the production code change removes that lock. This doc should be updated so it remains accurate and doesn’t imply the test is validating behavior that no longer exists.

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

• line 407
• line 435

        /// <summary>
        /// Regression test for the CreateLocalCompletionTask "internal end" path, which still
        /// takes lock (_stateObj) while calling endFunc. That continuation fires as soon as the
        /// first packet arrives, not once the whole result is buffered, so when a query flushes
        /// partial results (RAISERROR WITH NOWAIT) and then blocks (WAITFOR), endFunc performs a

src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/ApiShould.cs:2409

• This summary refers to a "retained lock" in CreateLocalCompletionTask blocking Cancel(), but the lock has been removed in the implementation. The comment should be updated so it stays accurate and doesn’t mislead readers about the current synchronization strategy.

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

        /// Flushing a partial result before the delay is what exposes the retained lock:
        /// localCompletion completes on that first packet, the continuation enters endFunc while
        /// holding the monitor, and blocks reading the rest. TdsParserStateObject.Cancel() needs
        /// that same monitor to send the attention signal, so cancellation is silently dropped
        /// and ExecuteReaderAsync returns a reader once WAITFOR expires.