Fix github issue 3736 #3912

samsharma2700 · 2026-01-23T21:10:40Z

Description

Fix ExecuteScalar to propagate errors when server sends data followed by error token

Fixes #3736

This PR fixes an issue where ExecuteScalar() swallows server errors that occur after the first result row is returned. When SQL Server sends data followed by an error token (e.g., a conversion error during WHERE clause evaluation), the error was being silently consumed during reader.Close() instead of being thrown to the caller.

Problem

When executing a query like SELECT Id FROM tab1 WHERE Val = 12345 against a table containing values that can't be converted (e.g., '42-43'), SQL Server:

Returns matching rows (where conversion succeeds)
Sends an error token for rows where conversion fails

The original code only read the first result and immediately closed the reader. The Close() method consumed the remaining TDS stream (including the error token) but the error was never propagated to the caller.

This caused serious issues in transactional scenarios:

The transaction would be silently "zombied" by the server
Subsequent commands appeared to succeed but ran outside the transaction
Data integrity issues from partial commits

Solution

In CompleteExecuteScalar(), drain all remaining result sets by calling NextResult() until it returns false before closing the reader. This ensures any pending error tokens are processed and thrown as exceptions.

Changes

SqlCommand.Scalar.cs: Added result draining loop in CompleteExecuteScalar() for the sync path
SqlCommandExecuteScalarTest.cs: Added 4 regression tests

Copilot

Pull request overview

This PR addresses GitHub issue #3736, which reports that ExecuteScalar() swallows server errors that occur after the first result row is returned. This causes transaction "zombification" where the transaction appears to succeed but is actually rolled back by the server, leading to data integrity issues.

Changes:

Adds result draining logic to CompleteExecuteScalar() in the synchronous path to ensure error tokens are processed before returning
Adds comprehensive regression tests for the synchronous ExecuteScalar method, including transaction scenarios
Updates the project file to include the new test file

Reviewed changes

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

File	Description
SqlCommand.Scalar.cs	Adds a while loop to drain remaining result sets in the non-batch sync path, ensuring error tokens are processed
SqlCommandExecuteScalarTest.cs	Adds 4 regression tests covering conversion errors with and without transactions, plus sanity tests for normal functionality
Microsoft.Data.SqlClient.ManualTesting.Tests.csproj	Adds reference to the new test file

Copilot · 2026-01-23T21:15:41Z

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlCommand/SqlCommandExecuteScalarTest.cs

+        public static void ExecuteScalar_ShouldThrowOnConversionError_GH3736()
+        {
+            string tab1Name = GenerateTableName("tab1");
+            string tab2Name = GenerateTableName("tab2");


The variable tab2Name is generated but never used in this test. It's only needed in ExecuteScalar_TransactionShouldRollbackOnError_GH3736. Consider removing this unused variable to avoid confusion.

Copilot · 2026-01-23T21:15:41Z

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlCommand/SqlCommandExecuteScalarTest.cs

+                    CREATE TABLE {tab2Name} (
+                        [Id] INT IDENTITY(1,1) NOT NULL,
+                        [Val1] INT NOT NULL,
+                        [Val2] INT NOT NULL
+                    );


The table tab2Name is created but never used in this test. The table creation on lines 46-50 can be removed since this test only needs tab1Name to verify the conversion error behavior.

Copilot · 2026-01-23T21:15:42Z

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.Scalar.cs

+                // Drain remaining results to ensure all error tokens are processed
+                // before returning the result (fix for GH issue #3736).
+                if (!returnLastResult)
+                {
+                    while (reader.NextResult())
+                    { }
+                }


The fix for GH issue #3736 only addresses the synchronous ExecuteScalar path but does not address the asynchronous path. The async method ExecuteScalarAsyncInternal (lines 226-343) has the same issue: it reads once and then disposes the reader without draining remaining result sets. This means ExecuteScalarAsync will still exhibit the bug where conversion errors after the first result row are swallowed, causing transaction zombification.

The async path should also drain remaining results by calling NextResultAsync in a loop before disposing the reader, similar to the fix applied here for the sync path.

Copilot · 2026-01-23T21:15:42Z

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlCommand/SqlCommandExecuteScalarTest.cs

+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+using System;
+using Xunit;
+
+namespace Microsoft.Data.SqlClient.ManualTesting.Tests
+{
+    /// <summary>
+    /// Tests for ExecuteScalar to verify proper exception handling.
+    /// </summary>
+    public static class SqlCommandExecuteScalarTest
+    {
+
+        private static string GenerateTableName(string prefix) =>
+            $"##{prefix}_{Guid.NewGuid():N}";
+
+        /// <summary>
+        /// Regression test for GitHub issue #3736: https://github.com/dotnet/SqlClient/issues/3736
+        /// ExecuteScalar should properly propagate conversion errors that occur after the first result.
+        /// 
+        /// Without the fix, the conversion error is swallowed during reader.Close(), causing:
+        /// 1. The transaction to become "zombied" without throwing an exception
+        /// 2. Subsequent commands to execute on a dead transaction
+        /// 3. Partial commits when the transaction commit fails
+        /// </summary>
+        [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))]
+        public static void ExecuteScalar_ShouldThrowOnConversionError_GH3736()
+        {
+            string tab1Name = GenerateTableName("tab1");
+            string tab2Name = GenerateTableName("tab2");
+
+            using SqlConnection connection = new(DataTestUtility.TCPConnectionString);
+            connection.Open();
+
+            // Setup: Create test tables (temp tables auto-cleanup)
+            using (SqlCommand setupCmd = connection.CreateCommand())
+            {
+                setupCmd.CommandText = $@"
+                    CREATE TABLE {tab1Name} (
+                        [Id] INT IDENTITY(1,1) NOT NULL,
+                        [Val] VARCHAR(10) NOT NULL
+                    );
+
+                    CREATE TABLE {tab2Name} (
+                        [Id] INT IDENTITY(1,1) NOT NULL,
+                        [Val1] INT NOT NULL,
+                        [Val2] INT NOT NULL
+                    );
+
+                    INSERT INTO {tab1Name} (Val) VALUES ('12345');
+                    INSERT INTO {tab1Name} (Val) VALUES ('42-43');"; // This will cause conversion error
+                setupCmd.ExecuteNonQuery();
+            }
+
+            // Test: Execute a query that will cause a conversion error after returning a valid row
+            // The query "SELECT Id FROM tab1 WHERE Val = 12345" will:
+            // 1. Return row with Id=1 (Val='12345' converts to 12345)
+            // 2. Fail on row with Id=2 (Val='42-43' cannot convert to int)
+            // 
+            // Before the fix: ExecuteScalar returns 1 without throwing an exception
+            // After the fix: ExecuteScalar throws SqlException with the conversion error
+            using SqlCommand cmd = new($"SELECT Id FROM {tab1Name} WHERE Val = 12345", connection);
+
+            SqlException ex = Assert.Throws<SqlException>(() => cmd.ExecuteScalar());
+
+            // Error 245: Conversion failed when converting the varchar value '42-43' to data type int.
+            Assert.Equal(245, ex.Number);
+            Assert.Contains("Conversion failed", ex.Message);
+        }
+
+        /// <summary>
+        /// Regression test for GitHub issue #3736:
+        /// Verifies the transaction scenario from the original issue report.
+        /// 
+        /// In the original bug, a transaction would be zombied without notification, causing:
+        /// - INSERT before the error to be rolled back (correct)
+        /// - INSERT after the error to be committed outside transaction (incorrect)
+        /// </summary>
+        [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))]
+        public static void ExecuteScalar_TransactionShouldRollbackOnError_GH3736()
+        {
+            string tab1Name = GenerateTableName("tab1");
+            string tab2Name = GenerateTableName("tab2");
+
+            using SqlConnection connection = new(DataTestUtility.TCPConnectionString);
+            connection.Open();
+
+            // Setup: Create test tables (temp tables auto-cleanup)
+            using (SqlCommand setupCmd = connection.CreateCommand())
+            {
+                setupCmd.CommandText = $@"
+                    CREATE TABLE {tab1Name} (
+                        [Id] INT IDENTITY(1,1) NOT NULL,
+                        [Val] VARCHAR(10) NOT NULL
+                    );
+
+                    CREATE TABLE {tab2Name} (
+                        [Id] INT IDENTITY(1,1) NOT NULL,
+                        [Val1] INT NOT NULL,
+                        [Val2] INT NOT NULL
+                    );
+
+                    INSERT INTO {tab1Name} (Val) VALUES ('12345');
+                    INSERT INTO {tab1Name} (Val) VALUES ('42-43');";
+                setupCmd.ExecuteNonQuery();
+            }
+
+            // Test: Execute queries in a transaction where one will cause an error
+            bool exceptionThrown = false;
+            try
+            {
+                using SqlTransaction transaction = connection.BeginTransaction();
+
+                // First insert - should be rolled back if transaction fails
+                using (SqlCommand cmd1 = new($"INSERT INTO {tab2Name} (Val1, Val2) VALUES (42, 43)", connection, transaction))
+                {
+                    cmd1.ExecuteNonQuery();
+                }
+
+                // This should throw due to conversion error
+                using (SqlCommand cmd2 = new($"SELECT Id FROM {tab1Name} WHERE Val = 12345", connection, transaction))
+                {
+                    cmd2.ExecuteScalar();
+                }
+
+                // This should never execute if the fix is working
+                using (SqlCommand cmd3 = new($"INSERT INTO {tab2Name} (Val1, Val2) VALUES (100, 200)", connection, transaction))
+                {
+                    cmd3.ExecuteNonQuery();
+                }
+
+                transaction.Commit();
+            }
+            catch (SqlException ex) when (ex.Number == 245)
+            {
+                // Expected: Conversion error should be thrown
+                exceptionThrown = true;
+            }
+
+            Assert.True(exceptionThrown, "Expected SqlException with conversion error (245) was not thrown");
+
+            // Verify: No rows should be in tab2 (transaction should have been rolled back)
+            using (SqlCommand verifyCmd = new($"SELECT COUNT(*) FROM {tab2Name}", connection))
+            {
+                int count = (int)verifyCmd.ExecuteScalar();
+                Assert.Equal(0, count);
+            }
+        }
+
+        /// <summary>
+        /// Verifies that ExecuteScalar works correctly when there is no error.
+        /// This is a sanity check to ensure the fix doesn't break normal functionality.
+        /// </summary>
+        [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))]
+        public static void ExecuteScalar_ShouldWorkCorrectlyWithoutError()
+        {
+            using SqlConnection connection = new(DataTestUtility.TCPConnectionString);
+            connection.Open();
+
+            using SqlCommand cmd = new("SELECT 42", connection);
+            object result = cmd.ExecuteScalar();
+
+            Assert.Equal(42, result);
+        }
+
+        /// <summary>
+        /// Verifies that ExecuteScalar returns null when there are no rows.
+        /// </summary>
+        [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))]
+        public static void ExecuteScalar_ShouldReturnNullWhenNoRows()
+        {
+            using SqlConnection connection = new(DataTestUtility.TCPConnectionString);
+            connection.Open();
+
+            using SqlCommand cmd = new("SELECT 1 WHERE 1 = 0", connection);
+            object result = cmd.ExecuteScalar();
+
+            Assert.Null(result);
+        }
+    }
+}


The test suite only includes synchronous tests for the GH issue #3736 fix. Since the same bug affects ExecuteScalarAsync (see related comment on SqlCommand.Scalar.cs), tests should be added to verify that ExecuteScalarAsync also properly propagates conversion errors that occur after the first result. This should include async versions of both ExecuteScalar_ShouldThrowOnConversionError_GH3736 and ExecuteScalar_TransactionShouldRollbackOnError_GH3736.

cheenamalhotra · 2026-01-24T01:34:15Z

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlCommand/SqlCommandExecuteScalarTest.cs

+            // Setup: Create test tables (temp tables auto-cleanup)
+            using (SqlCommand setupCmd = connection.CreateCommand())
+            {
+                setupCmd.CommandText = $@"


Please don't leave tables on the server, delete them in finally block.

Fix github issue 3736

ee91dbc

samsharma2700 requested a review from a team as a code owner January 23, 2026 21:10

Copilot AI review requested due to automatic review settings January 23, 2026 21:10

Copilot started reviewing on behalf of samsharma2700 January 23, 2026 21:10 View session

Copilot AI reviewed Jan 23, 2026

View reviewed changes

cheenamalhotra reviewed Jan 24, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix github issue 3736 #3912

Fix github issue 3736 #3912

samsharma2700 commented Jan 23, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

cheenamalhotra Jan 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Fix github issue 3736 #3912

Are you sure you want to change the base?

Fix github issue 3736 #3912

Conversation

samsharma2700 commented Jan 23, 2026

Description

Problem

Solution

Changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

cheenamalhotra Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants