Fix #3736 | Propagate Errors from ExecuteScalar

[samsharma2700]

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

[benrr101]

I think this change needs some work to ensure we're fixing it in sync and async, as well as ensure we're testing what we actually needs to be tested.

[benrr101]

Can we verify a test like this doesn't already exist? I'd be astounded if it doesn't.

[benrr101]

If it doesn't, I have more comments for this test :)

[samsharma2700]

We have DiagnosticTest.ExecuteScalarTest and DiagnosticTest.ExecuteScalarAsyncTest, which verify diagnostic events but don't validate return values. Additionally, those tests use a mock TDS Server, whereas mine use a real server. I believe it's important to keep these sanity tests for clear, isolated verification of this particular change.

[benrr101]

1. I'm not sure we want effectively dead code in here. I'm hoping there's a mechanism we can tap into we can use to check if the transaction was rolled back without querying the table.
2. Again, try not to think of this as "a test for a fix", this is just expected behavior, right? It's more like "This should never execute because the above should throw a conversion error"

[samsharma2700]

I am trying to think of such mechanism, right now the tests are making sure the repro problem is handled correctly.

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 0.00%. Comparing base (88de2b9) to head (47fb5e5).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #3912       +/-   ##
==========================================
- Coverage   74.93%       0   -74.94%     
==========================================
  Files         269       0      -269     
  Lines       43247       0    -43247     
==========================================
- Hits        32407       0    -32407     
+ Misses      10840       0    -10840     

Flag	Coverage Δ
addons	?
netcore	?
netfx	?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.