From 564e0db0f149b121663ad24b8a8dfd108973e2b0 Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Wed, 8 Jul 2026 20:14:53 -0700 Subject: [PATCH 01/11] Fix async cancellation failing to send TDS attention signal Remove lock(_stateObj) from EndExecuteReaderAsync, EndExecuteNonQueryAsync, and EndExecuteXmlReaderAsync. The lock prevented Cancel() from acquiring the stateObj monitor to send a TDS attention signal when the async completion path was blocked on a synchronous network read (e.g., waiting for metadata during WAITFOR). This caused cancellation via CancellationToken to hang until the query completed naturally. Concurrent close/cancel safety is maintained by: - Parser state checks within TryRun (detects Broken/Closed state) - Cancel()'s polling loop via Monitor.TryEnter with parser state guards - The stateObj's internal synchronization mechanisms Fixes #4424 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../Data/SqlClient/SqlCommand.NonQuery.cs | 12 ++---------- .../Data/SqlClient/SqlCommand.Reader.cs | 19 ++++++++++--------- .../Data/SqlClient/SqlCommand.Xml.cs | 13 ++----------- 3 files changed, 14 insertions(+), 30 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.NonQuery.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.NonQuery.cs index 311c26b4e5..884a3e41af 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.NonQuery.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.NonQuery.cs @@ -360,17 +360,9 @@ private int EndExecuteNonQueryAsync(IAsyncResult asyncResult) } ThrowIfReconnectionHasBeenCanceled(); - // lock on _stateObj prevents races with close/cancel. - // If we have already initiated the End call internally, we have already done that, so - // no point doing it again. - if (!_internalEndExecuteInitiated) - { - lock (_stateObj) - { - return EndExecuteNonQueryInternal(asyncResult); - } - } + // Note: We intentionally do NOT lock on _stateObj here. + // See comment in EndExecuteReaderAsync and GitHub issue #4424 for details. return EndExecuteNonQueryInternal(asyncResult); } diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.Reader.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.Reader.cs index bffe17baf9..acbea38b52 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.Reader.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.Reader.cs @@ -671,15 +671,16 @@ private SqlDataReader EndExecuteReaderAsync(IAsyncResult asyncResult) ThrowIfReconnectionHasBeenCanceled(); - // Lock on _stateObj prevents race with close/cancel - if (!_internalEndExecuteInitiated) - { - lock (_stateObj) - { - return EndExecuteReaderInternal(asyncResult); - } - } - + // Note: We intentionally do NOT lock on _stateObj here. + // Taking lock(_stateObj) would prevent Cancel() from acquiring the stateObj + // monitor to send a TDS attention signal while FinishExecuteReader may be + // blocked on a synchronous network read (e.g., waiting for metadata after + // partial results like RAISERROR WITH NOWAIT). This caused cancellation to + // hang until the full query completed. See GitHub issue #4424. + // + // Concurrent close is handled by parser state checks within TryRun + // (detects Broken/Closed state). Cancel() uses Monitor.TryEnter with polling + // and checks parser state in its loop, so it handles concurrency safely. return EndExecuteReaderInternal(asyncResult); } diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.Xml.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.Xml.cs index 447627375e..4e1971175e 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.Xml.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.Xml.cs @@ -393,17 +393,8 @@ private XmlReader EndExecuteXmlReaderAsync(IAsyncResult asyncResult) ThrowIfReconnectionHasBeenCanceled(); - // Locking _stateObj prevents races with close/cancel. - // If we have already initiated the End call internally, we have already done that, so - // no point doing it again. - if (!_internalEndExecuteInitiated) - { - lock (_stateObj) - { - return EndExecuteXmlReaderInternal(asyncResult); - } - } - + // Note: We intentionally do NOT lock on _stateObj here. + // See comment in EndExecuteReaderAsync and GitHub issue #4424 for details. return EndExecuteXmlReaderInternal(asyncResult); } From 71d63e39f73ccc8b2bb887c865f2af239fb1f1bb Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Wed, 8 Jul 2026 20:15:56 -0700 Subject: [PATCH 02/11] Add manual test for async cancellation with partial results Validates that CancellationToken triggers TDS attention when the server has sent partial results (RAISERROR WITH NOWAIT) but is blocked on WAITFOR. This test would previously hang for 60+ seconds without the fix. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../DataReaderCancellationTest.cs | 51 +++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderCancellationTest.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderCancellationTest.cs index 4b0ef2e8a1..1e6957d8dc 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderCancellationTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderCancellationTest.cs @@ -82,5 +82,56 @@ await Assert.ThrowsAsync(async () => Assert.True(stopwatch.ElapsedMilliseconds < 10000, "Cancellation did not trigger on time."); } } + /// + /// Validates that async cancellation sends a TDS attention signal to SQL Server + /// when the server has sent partial results (RAISERROR WITH NOWAIT) followed by + /// a blocking operation (WAITFOR). Without the fix for GitHub issue #4424, + /// cancellation would hang until WAITFOR completed naturally. + /// Synapse: Incompatible query. + /// + [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup), nameof(DataTestUtility.IsNotAzureSynapse))] + public static async Task CancellationSendsAttention_WhenPartialResultsReceived() + { + // This query sends a partial response (RAISERROR WITH NOWAIT), then blocks + // for 60 seconds. Cancellation should send attention and abort within seconds. + const string query = @" +RAISERROR('partial result', 0, 1) WITH NOWAIT; +WAITFOR DELAY '00:01:00'; +SELECT 1 AS Result;"; + + using (var cts = new CancellationTokenSource()) + using (var connection = new SqlConnection(DataTestUtility.TCPConnectionString)) + { + await connection.OpenAsync(); + + using (var command = new SqlCommand(query, connection)) + { + command.CommandTimeout = 90; + Stopwatch stopwatch = Stopwatch.StartNew(); + + await Assert.ThrowsAnyAsync(async () => + { + using (var reader = await command.ExecuteReaderAsync(cts.Token)) + { + // Cancel after a short delay to ensure the server has sent + // the RAISERROR partial result and is blocked on WAITFOR. + cts.CancelAfter(System.TimeSpan.FromSeconds(2)); + // ReadAsync should be cancelled by the token sending attention + while (await reader.ReadAsync(cts.Token)) + { } + // Advance to next result set (blocked by WAITFOR) + await reader.NextResultAsync(cts.Token); + } + }); + + stopwatch.Stop(); + // The key assertion: cancellation should complete well before the + // 60-second WAITFOR. Allow up to 30 seconds for CI variability. + Assert.True(stopwatch.ElapsedMilliseconds < 30000, + $"Cancellation took {stopwatch.ElapsedMilliseconds}ms, expected < 30000ms. " + + "Attention signal may not have been sent to the server."); + } + } + } } } From 5ca7e0f2fd7ae820b2d0c4d7fcf700a1bf1b2685 Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Wed, 8 Jul 2026 23:17:49 -0700 Subject: [PATCH 03/11] Address review: fix test to cancel before ExecuteReaderAsync and accept SqlException Move CancelAfter() before ExecuteReaderAsync so cancellation fires during the async completion path (not just ReadAsync). Also accept SqlException in addition to OperationCanceledException since attention acknowledgment from the server surfaces as SqlException. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../DataReaderCancellationTest.cs | 28 +++++++++++++++---- 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderCancellationTest.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderCancellationTest.cs index 1e6957d8dc..3d2405ada7 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderCancellationTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderCancellationTest.cs @@ -107,24 +107,40 @@ public static async Task CancellationSendsAttention_WhenPartialResultsReceived() using (var command = new SqlCommand(query, connection)) { command.CommandTimeout = 90; + + // Schedule cancellation BEFORE ExecuteReaderAsync so it fires while + // the async completion path may still be consuming metadata or while + // ReadAsync/NextResultAsync is blocked waiting for the server. + cts.CancelAfter(System.TimeSpan.FromSeconds(2)); + Stopwatch stopwatch = Stopwatch.StartNew(); - await Assert.ThrowsAnyAsync(async () => + // Cancellation during async read may surface as either + // OperationCanceledException or SqlException (attention ack). + System.Exception caughtException = null; + try { using (var reader = await command.ExecuteReaderAsync(cts.Token)) { - // Cancel after a short delay to ensure the server has sent - // the RAISERROR partial result and is blocked on WAITFOR. - cts.CancelAfter(System.TimeSpan.FromSeconds(2)); - // ReadAsync should be cancelled by the token sending attention while (await reader.ReadAsync(cts.Token)) { } // Advance to next result set (blocked by WAITFOR) await reader.NextResultAsync(cts.Token); } - }); + } + catch (System.OperationCanceledException ex) + { + caughtException = ex; + } + catch (SqlException ex) + { + // Attention acknowledgment from server manifests as SqlException + caughtException = ex; + } stopwatch.Stop(); + + Assert.NotNull(caughtException); // The key assertion: cancellation should complete well before the // 60-second WAITFOR. Allow up to 30 seconds for CI variability. Assert.True(stopwatch.ElapsedMilliseconds < 30000, From 89b51a94f278ce4a86cbd8fbaccf861e24e8914b Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Wed, 8 Jul 2026 23:39:29 -0700 Subject: [PATCH 04/11] Add AE test for async cancellation via CreateLocalCompletionTask path Adds TestAsyncCancellationSendsAttention_WithAlwaysEncryptedCommand that exercises the internal-end path in CreateLocalCompletionTask by warming the query metadata cache first, then running a long-running AE query with CancellationToken. Also removes the lock from CreateLocalCompletionTask to fix the same contention issue in the AE retry path. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../Microsoft/Data/SqlClient/SqlCommand.cs | 8 +- .../ManualTests/AlwaysEncrypted/ApiShould.cs | 92 +++++++++++++++++++ .../DataReaderCancellationTest.cs | 53 +++++++++++ 3 files changed, 148 insertions(+), 5 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.cs index 76399749a0..6ebaf45aec 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.cs @@ -2451,11 +2451,9 @@ private void CreateLocalCompletionTask( Debug.Assert(!_internalEndExecuteInitiated); _internalEndExecuteInitiated = true; - // Lock on _stateObj prevents races with close/cancel - lock (_stateObj) - { - endFunc(this, task, /*isInternal:*/ true, endMethod); - } + // Note: We intentionally do NOT lock on _stateObj here. + // See comment in EndExecuteReaderAsync and GitHub issue #4424. + endFunc(this, task, /*isInternal:*/ true, endMethod); globalCompletion.TrySetResult(task.Result); } diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/ApiShould.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/ApiShould.cs index 6f564250ce..80a86fe720 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/ApiShould.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/ApiShould.cs @@ -2276,6 +2276,98 @@ public void TestSqlCommandCancellationToken(string connection, int initalValue, } + /// + /// Validates that async cancellation via CancellationToken sends a TDS attention signal + /// when an AE-enabled command is blocked on a server-side wait (e.g., WAITFOR DELAY). + /// This covers the internal-end path in CreateLocalCompletionTask where the lock on + /// _stateObj previously prevented Cancel() from sending attention. + /// See GitHub issue #4424. + /// Synapse: Incompatible query. + /// + [ConditionalTheory(typeof(DataTestUtility), nameof(DataTestUtility.IsTargetReadyForAeWithKeyStore))] + [ClassData(typeof(AEConnectionStringProvider))] + public async Task TestAsyncCancellationSendsAttention_WithAlwaysEncryptedCommand(string connection) + { + CleanUpTable(connection, _tableName); + + IList values = GetValues(dataHint: 60); + int numberOfRows = 10; + int rowsAffected = InsertRows(tableName: _tableName, numberofRows: numberOfRows, values: values, connection: connection); + Assert.True(rowsAffected == numberOfRows, "number of rows affected is unexpected."); + + using (SqlConnection sqlConnection = new SqlConnection(connection)) + { + await sqlConnection.OpenAsync(); + + // First query to warm the metadata cache (so subsequent calls use the internal-end path) + using (SqlCommand warmupCmd = new SqlCommand( + $"SELECT CustomerId, FirstName, LastName FROM [{_tableName}] WHERE FirstName = @FirstName AND CustomerId = @CustomerId", + sqlConnection, null, SqlCommandColumnEncryptionSetting.Enabled)) + { + warmupCmd.Parameters.AddWithValue("@CustomerId", values[0]); + warmupCmd.Parameters.AddWithValue("@FirstName", values[1]); + + using (var reader = await warmupCmd.ExecuteReaderAsync()) + { + while (await reader.ReadAsync()) { } + } + } + + // Now execute a long-running query with AE enabled and cancel it. + // The WAITFOR ensures the server blocks after initial metadata/results are sent. + // With cached metadata, this goes through CreateLocalCompletionTask's internal-end path. + using (SqlCommand sqlCommand = new SqlCommand( + $"SELECT CustomerId, FirstName, LastName FROM [{_tableName}] WHERE FirstName = @FirstName AND CustomerId = @CustomerId; WAITFOR DELAY '00:01:00';", + sqlConnection, null, SqlCommandColumnEncryptionSetting.Enabled)) + { + sqlCommand.Parameters.AddWithValue("@CustomerId", values[0]); + sqlCommand.Parameters.AddWithValue("@FirstName", values[1]); + sqlCommand.CommandTimeout = 90; + + using (CancellationTokenSource cts = new CancellationTokenSource(TimeSpan.FromSeconds(3))) + { + System.Diagnostics.Stopwatch stopwatch = System.Diagnostics.Stopwatch.StartNew(); + + Exception caughtException = null; + try + { + using (SqlDataReader reader = await sqlCommand.ExecuteReaderAsync(cts.Token)) + { + while (await reader.ReadAsync(cts.Token)) { } + // NextResultAsync will block on WAITFOR — cancellation should abort it + await reader.NextResultAsync(cts.Token); + } + } + catch (OperationCanceledException ex) + { + caughtException = ex; + } + catch (SqlException ex) + { + // Attention ack from server manifests as SqlException + caughtException = ex; + } + + stopwatch.Stop(); + + Assert.NotNull(caughtException); + // Cancellation should complete well before the 60-second WAITFOR. + Assert.True(stopwatch.ElapsedMilliseconds < 30000, + $"Cancellation took {stopwatch.ElapsedMilliseconds}ms, expected < 30000ms. " + + "Attention signal may not have been sent during AE async execution."); + } + } + + // Verify the connection is still usable after cancellation. + using (SqlCommand verifyCmd = new SqlCommand( + $"SELECT COUNT(*) FROM [{_tableName}]", sqlConnection)) + { + object result = await verifyCmd.ExecuteScalarAsync(); + Assert.Equal(numberOfRows, (int)result); + } + } + } + [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.IsSGXEnclaveConnStringSetup))] public void TestNoneAttestationProtocolWithSGXEnclave() { diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderCancellationTest.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderCancellationTest.cs index 3d2405ada7..f4fd8ce7b9 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderCancellationTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderCancellationTest.cs @@ -149,5 +149,58 @@ public static async Task CancellationSendsAttention_WhenPartialResultsReceived() } } } + + /// + /// Validates that cancellation during ExecuteReaderAsync itself (not just ReadAsync) + /// sends a TDS attention signal. This covers the case where the async completion path + /// in EndExecuteReaderInternal is blocked on synchronous metadata consumption while + /// the server is still processing (e.g., a long-running batch where initial metadata + /// is delayed). This also covers the internal-end path used by Always Encrypted retry. + /// Synapse: Incompatible query. + /// + [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup), nameof(DataTestUtility.IsNotAzureSynapse))] + public static async Task CancellationDuringExecuteReaderAsync_SendsAttention() + { + // Use a query that blocks immediately so cancellation must fire during + // ExecuteReaderAsync's async completion (before any reader is returned). + const string query = "WAITFOR DELAY '00:01:00'; SELECT 1 AS Result;"; + + using (var cts = new CancellationTokenSource(System.TimeSpan.FromSeconds(2))) + using (var connection = new SqlConnection(DataTestUtility.TCPConnectionString)) + { + await connection.OpenAsync(); + + using (var command = new SqlCommand(query, connection)) + { + command.CommandTimeout = 90; + Stopwatch stopwatch = Stopwatch.StartNew(); + + System.Exception caughtException = null; + try + { + // ExecuteReaderAsync itself should be cancelled via attention + using (var reader = await command.ExecuteReaderAsync(cts.Token)) + { + await reader.ReadAsync(cts.Token); + } + } + catch (System.OperationCanceledException ex) + { + caughtException = ex; + } + catch (SqlException ex) + { + caughtException = ex; + } + + stopwatch.Stop(); + + Assert.NotNull(caughtException); + Assert.True(stopwatch.ElapsedMilliseconds < 30000, + $"Cancellation took {stopwatch.ElapsedMilliseconds}ms, expected < 30000ms. " + + "Attention signal may not have been sent during ExecuteReaderAsync."); + } + } + } } } From 7590ffef29d9a86a87141b6708a50924e8401c30 Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Wed, 8 Jul 2026 23:50:37 -0700 Subject: [PATCH 05/11] Address review feedback: fix test accuracy - Use severity 10 in RAISERROR (matches real-world repro from #4424) - Assert.Fail if reader is returned in WAITFOR-first test (should never happen) - Fix AE test cache key: use same CommandText with parameterized @Delay so warmup and cancel executions share the metadata cache entry Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ManualTests/AlwaysEncrypted/ApiShould.cs | 20 +++++++---- .../DataReaderCancellationTest.cs | 34 +++++++++++-------- 2 files changed, 32 insertions(+), 22 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/ApiShould.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/ApiShould.cs index 80a86fe720..9e08d2ad05 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/ApiShould.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/ApiShould.cs @@ -2299,29 +2299,35 @@ public async Task TestAsyncCancellationSendsAttention_WithAlwaysEncryptedCommand { await sqlConnection.OpenAsync(); - // First query to warm the metadata cache (so subsequent calls use the internal-end path) + // Use the same CommandText for both warmup and cancellation test so the + // query metadata cache key matches and the second execution goes through + // the CreateLocalCompletionTask internal-end path. + string commandText = $"SELECT CustomerId, FirstName, LastName FROM [{_tableName}] WHERE FirstName = @FirstName AND CustomerId = @CustomerId; WAITFOR DELAY @Delay;"; + + // Warmup: execute with a short delay to populate the metadata cache. using (SqlCommand warmupCmd = new SqlCommand( - $"SELECT CustomerId, FirstName, LastName FROM [{_tableName}] WHERE FirstName = @FirstName AND CustomerId = @CustomerId", - sqlConnection, null, SqlCommandColumnEncryptionSetting.Enabled)) + commandText, sqlConnection, null, SqlCommandColumnEncryptionSetting.Enabled)) { warmupCmd.Parameters.AddWithValue("@CustomerId", values[0]); warmupCmd.Parameters.AddWithValue("@FirstName", values[1]); + warmupCmd.Parameters.AddWithValue("@Delay", "00:00:00"); using (var reader = await warmupCmd.ExecuteReaderAsync()) { while (await reader.ReadAsync()) { } + // Consume all result sets to complete cleanly + while (await reader.NextResultAsync()) { } } } - // Now execute a long-running query with AE enabled and cancel it. - // The WAITFOR ensures the server blocks after initial metadata/results are sent. + // Now execute the same command with a long delay and cancel it. // With cached metadata, this goes through CreateLocalCompletionTask's internal-end path. using (SqlCommand sqlCommand = new SqlCommand( - $"SELECT CustomerId, FirstName, LastName FROM [{_tableName}] WHERE FirstName = @FirstName AND CustomerId = @CustomerId; WAITFOR DELAY '00:01:00';", - sqlConnection, null, SqlCommandColumnEncryptionSetting.Enabled)) + commandText, sqlConnection, null, SqlCommandColumnEncryptionSetting.Enabled)) { sqlCommand.Parameters.AddWithValue("@CustomerId", values[0]); sqlCommand.Parameters.AddWithValue("@FirstName", values[1]); + sqlCommand.Parameters.AddWithValue("@Delay", "00:01:00"); sqlCommand.CommandTimeout = 90; using (CancellationTokenSource cts = new CancellationTokenSource(TimeSpan.FromSeconds(3))) diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderCancellationTest.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderCancellationTest.cs index f4fd8ce7b9..9c360145bf 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderCancellationTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderCancellationTest.cs @@ -84,18 +84,19 @@ await Assert.ThrowsAsync(async () => } /// /// Validates that async cancellation sends a TDS attention signal to SQL Server - /// when the server has sent partial results (RAISERROR WITH NOWAIT) followed by - /// a blocking operation (WAITFOR). Without the fix for GitHub issue #4424, + /// when the server has sent partial results (RAISERROR WITH NOWAIT at severity 10) + /// followed by a blocking operation (WAITFOR). Without the fix for GitHub issue #4424, /// cancellation would hang until WAITFOR completed naturally. /// Synapse: Incompatible query. /// [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup), nameof(DataTestUtility.IsNotAzureSynapse))] public static async Task CancellationSendsAttention_WhenPartialResultsReceived() { - // This query sends a partial response (RAISERROR WITH NOWAIT), then blocks - // for 60 seconds. Cancellation should send attention and abort within seconds. + // Severity 10 informational message flushed via NOWAIT sends a partial TDS + // response, then WAITFOR blocks for 60s. Cancellation should send attention + // and abort within seconds. const string query = @" -RAISERROR('partial result', 0, 1) WITH NOWAIT; +RAISERROR('partial result', 10, 1) WITH NOWAIT; WAITFOR DELAY '00:01:00'; SELECT 1 AS Result;"; @@ -122,9 +123,11 @@ public static async Task CancellationSendsAttention_WhenPartialResultsReceived() { using (var reader = await command.ExecuteReaderAsync(cts.Token)) { + // The RAISERROR result is consumed as an informational message. + // ReadAsync will block waiting for WAITFOR to complete (next result). + // Cancellation should interrupt this via attention signal. while (await reader.ReadAsync(cts.Token)) { } - // Advance to next result set (blocked by WAITFOR) await reader.NextResultAsync(cts.Token); } } @@ -151,18 +154,17 @@ public static async Task CancellationSendsAttention_WhenPartialResultsReceived() } /// - /// Validates that cancellation during ExecuteReaderAsync itself (not just ReadAsync) - /// sends a TDS attention signal. This covers the case where the async completion path - /// in EndExecuteReaderInternal is blocked on synchronous metadata consumption while - /// the server is still processing (e.g., a long-running batch where initial metadata - /// is delayed). This also covers the internal-end path used by Always Encrypted retry. + /// Validates that cancellation during ExecuteReaderAsync itself sends a TDS attention + /// signal when the server is blocked before returning any result set metadata. + /// With WAITFOR as the first statement, ExecuteReaderAsync should never return a + /// reader — cancellation must abort the operation during the await. /// Synapse: Incompatible query. /// [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup), nameof(DataTestUtility.IsNotAzureSynapse))] public static async Task CancellationDuringExecuteReaderAsync_SendsAttention() { - // Use a query that blocks immediately so cancellation must fire during - // ExecuteReaderAsync's async completion (before any reader is returned). + // WAITFOR as the first statement means no metadata is returned until it + // completes. ExecuteReaderAsync will be blocked in the async completion path. const string query = "WAITFOR DELAY '00:01:00'; SELECT 1 AS Result;"; using (var cts = new CancellationTokenSource(System.TimeSpan.FromSeconds(2))) @@ -178,10 +180,12 @@ public static async Task CancellationDuringExecuteReaderAsync_SendsAttention() System.Exception caughtException = null; try { - // ExecuteReaderAsync itself should be cancelled via attention + // ExecuteReaderAsync should be cancelled via attention before + // a reader is ever returned. using (var reader = await command.ExecuteReaderAsync(cts.Token)) { - await reader.ReadAsync(cts.Token); + // If we reach here, cancellation failed to abort ExecuteReaderAsync. + Assert.Fail("ExecuteReaderAsync should have been cancelled before returning a reader."); } } catch (System.OperationCanceledException ex) From b30baf4b47ccb4632e8611125ff6eef9d46198f1 Mon Sep 17 00:00:00 2001 From: Cheena Malhotra <13396919+cheenamalhotra@users.noreply.github.com> Date: Wed, 8 Jul 2026 23:57:14 -0700 Subject: [PATCH 06/11] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .../SQL/DataReaderTest/DataReaderCancellationTest.cs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderCancellationTest.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderCancellationTest.cs index 9c360145bf..3c3c2a4f69 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderCancellationTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderCancellationTest.cs @@ -123,12 +123,9 @@ public static async Task CancellationSendsAttention_WhenPartialResultsReceived() { using (var reader = await command.ExecuteReaderAsync(cts.Token)) { - // The RAISERROR result is consumed as an informational message. - // ReadAsync will block waiting for WAITFOR to complete (next result). - // Cancellation should interrupt this via attention signal. - while (await reader.ReadAsync(cts.Token)) - { } - await reader.NextResultAsync(cts.Token); + // 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."); } } catch (System.OperationCanceledException ex) From c4ab8ec39535d32df836a4ad20b483953d425ce1 Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Thu, 9 Jul 2026 00:06:41 -0700 Subject: [PATCH 07/11] Address review: add CTS assertions, fix AE test query order - Assert cts.IsCancellationRequested in all tests to guard against false positives from unrelated SqlExceptions - Reorder AE test query to WAITFOR DELAY @Delay; SELECT ... so that EndExecuteReaderInternal blocks on metadata (exercises the CreateLocalCompletionTask internal-end path as intended) - Add Assert.Fail in AE test if reader is unexpectedly returned Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ManualTests/AlwaysEncrypted/ApiShould.cs | 18 +++++++++++------- .../DataReaderCancellationTest.cs | 6 ++++++ 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/ApiShould.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/ApiShould.cs index 9e08d2ad05..0466a99aa8 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/ApiShould.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/ApiShould.cs @@ -2302,9 +2302,12 @@ public async Task TestAsyncCancellationSendsAttention_WithAlwaysEncryptedCommand // Use the same CommandText for both warmup and cancellation test so the // query metadata cache key matches and the second execution goes through // the CreateLocalCompletionTask internal-end path. - string commandText = $"SELECT CustomerId, FirstName, LastName FROM [{_tableName}] WHERE FirstName = @FirstName AND CustomerId = @CustomerId; WAITFOR DELAY @Delay;"; + // WAITFOR is placed BEFORE SELECT so that EndExecuteReaderInternal blocks + // waiting for result-set metadata — this is the exact CreateLocalCompletionTask + // code path we need to exercise. + string commandText = $"WAITFOR DELAY @Delay; SELECT CustomerId, FirstName, LastName FROM [{_tableName}] WHERE FirstName = @FirstName AND CustomerId = @CustomerId;"; - // Warmup: execute with a short delay to populate the metadata cache. + // Warmup: execute with zero delay to populate the metadata cache. using (SqlCommand warmupCmd = new SqlCommand( commandText, sqlConnection, null, SqlCommandColumnEncryptionSetting.Enabled)) { @@ -2315,13 +2318,14 @@ public async Task TestAsyncCancellationSendsAttention_WithAlwaysEncryptedCommand using (var reader = await warmupCmd.ExecuteReaderAsync()) { while (await reader.ReadAsync()) { } - // Consume all result sets to complete cleanly while (await reader.NextResultAsync()) { } } } // Now execute the same command with a long delay and cancel it. // With cached metadata, this goes through CreateLocalCompletionTask's internal-end path. + // The WAITFOR blocks before any result-set metadata is returned, so + // ExecuteReaderAsync should be cancelled before a reader is produced. using (SqlCommand sqlCommand = new SqlCommand( commandText, sqlConnection, null, SqlCommandColumnEncryptionSetting.Enabled)) { @@ -2339,9 +2343,8 @@ public async Task TestAsyncCancellationSendsAttention_WithAlwaysEncryptedCommand { using (SqlDataReader reader = await sqlCommand.ExecuteReaderAsync(cts.Token)) { - while (await reader.ReadAsync(cts.Token)) { } - // NextResultAsync will block on WAITFOR — cancellation should abort it - await reader.NextResultAsync(cts.Token); + // If we reach here, cancellation failed during EndExecuteReaderInternal. + Assert.Fail("ExecuteReaderAsync should have been cancelled before returning a reader."); } } catch (OperationCanceledException ex) @@ -2357,7 +2360,8 @@ public async Task TestAsyncCancellationSendsAttention_WithAlwaysEncryptedCommand stopwatch.Stop(); Assert.NotNull(caughtException); - // Cancellation should complete well before the 60-second WAITFOR. + Assert.True(cts.IsCancellationRequested, + "CancellationTokenSource was not cancelled; exception may be unrelated to cancellation."); Assert.True(stopwatch.ElapsedMilliseconds < 30000, $"Cancellation took {stopwatch.ElapsedMilliseconds}ms, expected < 30000ms. " + "Attention signal may not have been sent during AE async execution."); diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderCancellationTest.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderCancellationTest.cs index 3c3c2a4f69..1fd9c0866c 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderCancellationTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderCancellationTest.cs @@ -141,6 +141,10 @@ public static async Task CancellationSendsAttention_WhenPartialResultsReceived() stopwatch.Stop(); Assert.NotNull(caughtException); + // Ensure the CTS actually fired — guards against false positives + // from unrelated SqlExceptions. + Assert.True(cts.IsCancellationRequested, + "CancellationTokenSource was not cancelled; exception may be unrelated to cancellation."); // The key assertion: cancellation should complete well before the // 60-second WAITFOR. Allow up to 30 seconds for CI variability. Assert.True(stopwatch.ElapsedMilliseconds < 30000, @@ -197,6 +201,8 @@ public static async Task CancellationDuringExecuteReaderAsync_SendsAttention() stopwatch.Stop(); Assert.NotNull(caughtException); + Assert.True(cts.IsCancellationRequested, + "CancellationTokenSource was not cancelled; exception may be unrelated to cancellation."); Assert.True(stopwatch.ElapsedMilliseconds < 30000, $"Cancellation took {stopwatch.ElapsedMilliseconds}ms, expected < 30000ms. " + "Attention signal may not have been sent during ExecuteReaderAsync."); From ac8acd720364f8133bff0d4703d8cf80f4caa06d Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Thu, 9 Jul 2026 00:36:20 -0700 Subject: [PATCH 08/11] Revert CreateLocalCompletionTask lock removal to fix AE test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The lock in CreateLocalCompletionTask (AE internal-end/retry path) is intentionally needed — that path handles cancellation via the task's IsCanceled state, not via stateObj.Cancel(). Removing it caused TestSqlCommandCancellationToken to fail with unexpected exception types. The fix for #4424 only needs lock removal in the user-facing EndExecuteReaderAsync/NonQuery/XmlReader paths where Cancel() contends with synchronous network reads. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../src/Microsoft/Data/SqlClient/SqlCommand.cs | 12 +++++++++--- .../tests/ManualTests/AlwaysEncrypted/ApiShould.cs | 5 ++--- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.cs index 6ebaf45aec..26cb81593b 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.cs @@ -2451,9 +2451,15 @@ private void CreateLocalCompletionTask( Debug.Assert(!_internalEndExecuteInitiated); _internalEndExecuteInitiated = true; - // Note: We intentionally do NOT lock on _stateObj here. - // See comment in EndExecuteReaderAsync and GitHub issue #4424. - endFunc(this, task, /*isInternal:*/ true, endMethod); + // Lock on _stateObj prevents races with close/cancel during the + // internal-end path (AE retry/cache). Unlike the user-facing + // EndExecute* methods, this path is invoked internally after the + // first async operation completes and cancellation is handled via + // the localCompletion task state (IsCanceled check above). + lock (_stateObj) + { + endFunc(this, task, /*isInternal:*/ true, endMethod); + } globalCompletion.TrySetResult(task.Result); } diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/ApiShould.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/ApiShould.cs index 0466a99aa8..98702ea673 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/ApiShould.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/ApiShould.cs @@ -2279,9 +2279,8 @@ public void TestSqlCommandCancellationToken(string connection, int initalValue, /// /// Validates that async cancellation via CancellationToken sends a TDS attention signal /// when an AE-enabled command is blocked on a server-side wait (e.g., WAITFOR DELAY). - /// This covers the internal-end path in CreateLocalCompletionTask where the lock on - /// _stateObj previously prevented Cancel() from sending attention. - /// See GitHub issue #4424. + /// This exercises the EndExecuteReaderAsync path where the lock on _stateObj previously + /// prevented Cancel() from sending attention. See GitHub issue #4424. /// Synapse: Incompatible query. /// [ConditionalTheory(typeof(DataTestUtility), nameof(DataTestUtility.IsTargetReadyForAeWithKeyStore))] From 9431172943b9186ca968acc050321119204ab844 Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Thu, 9 Jul 2026 10:29:13 -0700 Subject: [PATCH 09/11] Address review: add infinite WHILE loop test, fix CTS timing, clarify comment - Add CancellationOfInfiniteWhileLoop_DoesNotHang test (repro from #44) - Fix CTS timing: start CancelAfter after OpenAsync to avoid false positives when connection open is slow - Clarify CreateLocalCompletionTask lock comment to accurately describe why the lock is retained (timing differs from user-facing path) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../Microsoft/Data/SqlClient/SqlCommand.cs | 11 +-- .../DataReaderCancellationTest.cs | 68 ++++++++++++++++++- 2 files changed, 74 insertions(+), 5 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.cs index 26cb81593b..44133a279a 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.cs @@ -2452,10 +2452,13 @@ private void CreateLocalCompletionTask( _internalEndExecuteInitiated = true; // Lock on _stateObj prevents races with close/cancel during the - // internal-end path (AE retry/cache). Unlike the user-facing - // EndExecute* methods, this path is invoked internally after the - // first async operation completes and cancellation is handled via - // the localCompletion task state (IsCanceled check above). + // internal-end path (AE retry/cache). This lock is retained here + // because cancellation in the user-facing path (EndExecute*Async) + // sends attention via stateObj.Cancel(), which can proceed without + // this lock since it only contends with the user-facing EndExecute* + // methods (where the lock was removed). The internal-end path runs + // on a ContinueWith continuation after the initial async operation + // completes, so the timing differs from the user-facing path. lock (_stateObj) { endFunc(this, task, /*isInternal:*/ true, endMethod); diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderCancellationTest.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderCancellationTest.cs index 1fd9c0866c..85b2031d29 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderCancellationTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderCancellationTest.cs @@ -168,11 +168,15 @@ public static async Task CancellationDuringExecuteReaderAsync_SendsAttention() // completes. ExecuteReaderAsync will be blocked in the async completion path. const string query = "WAITFOR DELAY '00:01:00'; SELECT 1 AS Result;"; - using (var cts = new CancellationTokenSource(System.TimeSpan.FromSeconds(2))) + using (var cts = new CancellationTokenSource()) using (var connection = new SqlConnection(DataTestUtility.TCPConnectionString)) { await connection.OpenAsync(); + // Start cancellation timer AFTER connection is open to avoid + // false positives if OpenAsync is slow. + cts.CancelAfter(System.TimeSpan.FromSeconds(2)); + using (var command = new SqlCommand(query, connection)) { command.CommandTimeout = 90; @@ -209,5 +213,67 @@ public static async Task CancellationDuringExecuteReaderAsync_SendsAttention() } } } + + /// + /// Validates that cancelling an infinite WHILE loop via CancellationToken does not + /// hang forever. This is the exact repro from GitHub issue #44. + /// Synapse: Incompatible query. + /// + [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup), nameof(DataTestUtility.IsNotAzureSynapse))] + public static async Task CancellationOfInfiniteWhileLoop_DoesNotHang() + { + // Infinite loop that never completes — only cancellation via attention can stop it. + const string query = @" +WHILE 1 = 1 +BEGIN + DECLARE @x INT = 1 +END"; + + using (var cts = new CancellationTokenSource()) + using (var connection = new SqlConnection(DataTestUtility.TCPConnectionString)) + { + await connection.OpenAsync(); + cts.CancelAfter(System.TimeSpan.FromSeconds(2)); + + using (var command = new SqlCommand(query, connection)) + { + command.CommandTimeout = 0; // No timeout — rely solely on cancellation + + Stopwatch stopwatch = Stopwatch.StartNew(); + + System.Exception caughtException = null; + try + { + await command.ExecuteNonQueryAsync(cts.Token); + Assert.Fail("ExecuteNonQueryAsync should have been cancelled."); + } + catch (System.OperationCanceledException ex) + { + caughtException = ex; + } + catch (SqlException ex) + { + caughtException = ex; + } + + stopwatch.Stop(); + + Assert.NotNull(caughtException); + Assert.True(cts.IsCancellationRequested, + "CancellationTokenSource was not cancelled; exception may be unrelated to cancellation."); + // Must complete well within 30s — without the fix this hangs forever. + Assert.True(stopwatch.ElapsedMilliseconds < 30000, + $"Cancellation took {stopwatch.ElapsedMilliseconds}ms, expected < 30000ms. " + + "Attention signal may not have been sent for infinite WHILE loop."); + } + + // Verify the connection is still usable after cancellation. + using (var verifyCmd = new SqlCommand("SELECT 1", connection)) + { + object result = await verifyCmd.ExecuteScalarAsync(); + Assert.Equal(1, (int)result); + } + } + } } } From 22528a8ccffd560fde2577bb7aa0f3b5b0f12817 Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Thu, 9 Jul 2026 13:43:44 -0700 Subject: [PATCH 10/11] Address review: clarify lock comment, add watchdog to while-loop test - Reword CreateLocalCompletionTask lock comment to accurately state that this lock CAN block Cancel(), and explain why it's retained (data is already buffered when this continuation fires, unlike the user-facing path where blocking reads caused #4424) - Add 45s watchdog timeout to CancellationOfInfiniteWhileLoop test with best-effort cleanup (Cancel/Close) to prevent hanging the test suite if the regression reappears Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../src/Microsoft/Data/SqlClient/SqlCommand.cs | 16 ++++++++-------- .../DataReaderTest/DataReaderCancellationTest.cs | 16 +++++++++++++++- 2 files changed, 23 insertions(+), 9 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.cs index 44133a279a..e65c8f4520 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.cs @@ -2451,14 +2451,14 @@ private void CreateLocalCompletionTask( Debug.Assert(!_internalEndExecuteInitiated); _internalEndExecuteInitiated = true; - // Lock on _stateObj prevents races with close/cancel during the - // internal-end path (AE retry/cache). This lock is retained here - // because cancellation in the user-facing path (EndExecute*Async) - // sends attention via stateObj.Cancel(), which can proceed without - // this lock since it only contends with the user-facing EndExecute* - // methods (where the lock was removed). The internal-end path runs - // on a ContinueWith continuation after the initial async operation - // completes, so the timing differs from the user-facing path. + // Lock on _stateObj serializes this internal-end path with + // close/cancel. Note: stateObj.Cancel() also acquires this monitor, + // so this lock CAN block Cancel() while endFunc is executing. + // 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. lock (_stateObj) { endFunc(this, task, /*isInternal:*/ true, endMethod); diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderCancellationTest.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderCancellationTest.cs index 85b2031d29..300d386ef9 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderCancellationTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderCancellationTest.cs @@ -244,7 +244,21 @@ public static async Task CancellationOfInfiniteWhileLoop_DoesNotHang() System.Exception caughtException = null; try { - await command.ExecuteNonQueryAsync(cts.Token); + // Watchdog: if cancellation regresses, don't hang the test suite. + // Use Task.WhenAny with a 45s delay as a hard timeout. + Task execTask = command.ExecuteNonQueryAsync(cts.Token); + Task completed = await Task.WhenAny(execTask, Task.Delay(System.TimeSpan.FromSeconds(45))); + + if (completed != execTask) + { + // Watchdog fired — best-effort cleanup + command.Cancel(); + connection.Close(); + Assert.Fail("ExecuteNonQueryAsync did not complete within 45s watchdog timeout. " + + "Cancellation via attention signal likely failed."); + } + + await execTask; // Propagate any exception Assert.Fail("ExecuteNonQueryAsync should have been cancelled."); } catch (System.OperationCanceledException ex) From bd0f7e51cc6f28ebb2324dd7604f7b398184f9cd Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Wed, 15 Jul 2026 08:58:30 -0700 Subject: [PATCH 11/11] Address flakiness by canceling from another thread. --- .../ManualTests/AlwaysEncrypted/ApiShould.cs | 21 +++++- .../DataReaderCancellationTest.cs | 66 +++++++++++++++---- 2 files changed, 72 insertions(+), 15 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/ApiShould.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/ApiShould.cs index 98702ea673..6af7b84dda 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/ApiShould.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/ApiShould.cs @@ -2333,14 +2333,30 @@ public async Task TestAsyncCancellationSendsAttention_WithAlwaysEncryptedCommand sqlCommand.Parameters.AddWithValue("@Delay", "00:01:00"); sqlCommand.CommandTimeout = 90; - using (CancellationTokenSource cts = new CancellationTokenSource(TimeSpan.FromSeconds(3))) + using (CancellationTokenSource cts = new CancellationTokenSource()) { System.Diagnostics.Stopwatch stopwatch = System.Diagnostics.Stopwatch.StartNew(); + // Start ExecuteReaderAsync without awaiting so we can trigger + // cancellation from a separate thread once the query is in flight. + // The 60-second WAITFOR gives a wide window during which + // cancellation must send a TDS attention signal to the server. + Task execTask = sqlCommand.ExecuteReaderAsync(cts.Token); + + // Cancel from another thread after briefly yielding to ensure the + // async operation has been dispatched and reached the server-side + // WAITFOR. This avoids the flakiness of a preemptive timer that + // could fire before the query is actually in flight. + Task cancelTask = Task.Run(async () => + { + await Task.Delay(TimeSpan.FromMilliseconds(500)); + cts.Cancel(); + }); + Exception caughtException = null; try { - using (SqlDataReader reader = await sqlCommand.ExecuteReaderAsync(cts.Token)) + using (SqlDataReader reader = await execTask) { // If we reach here, cancellation failed during EndExecuteReaderInternal. Assert.Fail("ExecuteReaderAsync should have been cancelled before returning a reader."); @@ -2356,6 +2372,7 @@ public async Task TestAsyncCancellationSendsAttention_WithAlwaysEncryptedCommand caughtException = ex; } + await cancelTask; stopwatch.Stop(); Assert.NotNull(caughtException); diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderCancellationTest.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderCancellationTest.cs index 300d386ef9..b757dcf546 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderCancellationTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderCancellationTest.cs @@ -109,19 +109,30 @@ public static async Task CancellationSendsAttention_WhenPartialResultsReceived() { command.CommandTimeout = 90; - // Schedule cancellation BEFORE ExecuteReaderAsync so it fires while - // the async completion path may still be consuming metadata or while - // ReadAsync/NextResultAsync is blocked waiting for the server. - cts.CancelAfter(System.TimeSpan.FromSeconds(2)); - Stopwatch stopwatch = Stopwatch.StartNew(); + // Start ExecuteReaderAsync without awaiting so we can trigger + // cancellation from a separate thread once the query is in flight. + // The 60-second WAITFOR gives a wide window during which + // cancellation must send a TDS attention signal to the server. + Task execTask = command.ExecuteReaderAsync(cts.Token); + + // Cancel from another thread after briefly yielding to ensure the + // async operation has been dispatched and reached the server-side + // WAITFOR. This avoids the flakiness of a preemptive timer that + // could fire before the query is actually in flight. + Task cancelTask = Task.Run(async () => + { + await Task.Delay(System.TimeSpan.FromMilliseconds(500)); + cts.Cancel(); + }); + // Cancellation during async read may surface as either // OperationCanceledException or SqlException (attention ack). System.Exception caughtException = null; try { - using (var reader = await command.ExecuteReaderAsync(cts.Token)) + 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). @@ -138,6 +149,7 @@ public static async Task CancellationSendsAttention_WhenPartialResultsReceived() caughtException = ex; } + await cancelTask; stopwatch.Stop(); Assert.NotNull(caughtException); @@ -173,21 +185,33 @@ public static async Task CancellationDuringExecuteReaderAsync_SendsAttention() { await connection.OpenAsync(); - // Start cancellation timer AFTER connection is open to avoid - // false positives if OpenAsync is slow. - cts.CancelAfter(System.TimeSpan.FromSeconds(2)); - using (var command = new SqlCommand(query, connection)) { command.CommandTimeout = 90; Stopwatch stopwatch = Stopwatch.StartNew(); + // Start ExecuteReaderAsync without awaiting so we can trigger + // cancellation from a separate thread once the query is in flight. + // WAITFOR as the first statement blocks for 60s, giving a wide + // window during which cancellation must send TDS attention. + Task execTask = command.ExecuteReaderAsync(cts.Token); + + // Cancel from another thread after briefly yielding to ensure the + // async operation has been dispatched and reached the server-side + // WAITFOR. This avoids the flakiness of a preemptive timer that + // could fire before the query is actually in flight. + Task cancelTask = Task.Run(async () => + { + await Task.Delay(System.TimeSpan.FromMilliseconds(500)); + cts.Cancel(); + }); + System.Exception caughtException = null; try { // ExecuteReaderAsync should be cancelled via attention before // a reader is ever returned. - using (var reader = await command.ExecuteReaderAsync(cts.Token)) + using (var reader = await execTask) { // If we reach here, cancellation failed to abort ExecuteReaderAsync. Assert.Fail("ExecuteReaderAsync should have been cancelled before returning a reader."); @@ -202,6 +226,7 @@ public static async Task CancellationDuringExecuteReaderAsync_SendsAttention() caughtException = ex; } + await cancelTask; stopwatch.Stop(); Assert.NotNull(caughtException); @@ -233,7 +258,6 @@ public static async Task CancellationOfInfiniteWhileLoop_DoesNotHang() using (var connection = new SqlConnection(DataTestUtility.TCPConnectionString)) { await connection.OpenAsync(); - cts.CancelAfter(System.TimeSpan.FromSeconds(2)); using (var command = new SqlCommand(query, connection)) { @@ -241,12 +265,27 @@ public static async Task CancellationOfInfiniteWhileLoop_DoesNotHang() Stopwatch stopwatch = Stopwatch.StartNew(); + // Start ExecuteNonQueryAsync without awaiting so we can trigger + // cancellation from a separate thread once the query is in flight. + // The infinite WHILE loop guarantees the server will remain busy + // until an attention signal aborts it. + Task execTask = command.ExecuteNonQueryAsync(cts.Token); + + // Cancel from another thread after briefly yielding to ensure the + // async operation has been dispatched and reached the server-side + // WHILE loop. This avoids the flakiness of a preemptive timer that + // could fire before the query is actually in flight. + Task cancelTask = Task.Run(async () => + { + await Task.Delay(System.TimeSpan.FromMilliseconds(500)); + cts.Cancel(); + }); + System.Exception caughtException = null; try { // Watchdog: if cancellation regresses, don't hang the test suite. // Use Task.WhenAny with a 45s delay as a hard timeout. - Task execTask = command.ExecuteNonQueryAsync(cts.Token); Task completed = await Task.WhenAny(execTask, Task.Delay(System.TimeSpan.FromSeconds(45))); if (completed != execTask) @@ -270,6 +309,7 @@ public static async Task CancellationOfInfiniteWhileLoop_DoesNotHang() caughtException = ex; } + await cancelTask; stopwatch.Stop(); Assert.NotNull(caughtException);