Fix TryOpenInner InvalidCast race and add unit regression tests (#3314)

[paulmedynski]

Summary

Fixes the InvalidCastException race in SqlConnection.TryOpenInner() reported in #3314 and adds regression tests in UnitTests.

The fix hardens TryOpenInner against concurrent state transitions on a shared SqlConnection instance:

• Capture InnerConnection once into a local variable
• Replace direct cast with a guarded type check
• Throw InvalidOperationException (ADP.ConnectionAlreadyOpen(State)) for non-SqlConnectionInternal transitional states instead of surfacing an opaque InvalidCastException

Root Cause

TryOpenInner previously performed an unsynchronized second read/cast:

var tdsInnerConnection = (SqlConnectionInternal)InnerConnection;

Between TryOpenConnection() and this cast, another thread could transition _innerConnection to DbConnectionClosedConnecting, which is not assignable to SqlConnectionInternal, causing InvalidCastException.

Fix

In SqlConnection.TryOpenInner():

var innerConnection = InnerConnection;
if (innerConnection is not SqlConnectionInternal tdsInnerConnection)
{
    throw ADP.ConnectionAlreadyOpen(State);
}

This removes the crashy cast path and yields a controlled, meaningful exception under concurrent misuse.

Tests

Regression tests were moved from FunctionalTests to UnitTests because they do not require SQL Server or external resources.

Added in SqlConnectionConcurrentOpenTests:

• InnerConnection_DbConnectionClosedConnecting_IsNotAssignableToSqlConnectionInternal
• InnerConnection_InConnectingState_ReportsConnectingState
• Open_WhenAlreadyConnecting_ThrowsInvalidOperation
• TryOpenInner_WhenInnerConnectionRacesToConnectingState_ThrowsInvalidOperation_NotInvalidCast

Validation:

• UnitTests filter run passed on net8.0, net9.0, and net10.0.

Fixes #3314

• Tests added/updated
• Public API changes documented (N/A)
• Verified against customer repro (N/A — race condition confirmed via code-level regression tests)
• Ensure no breaking changes introduced

[Copilot]

Pull request overview

Adds new FunctionalTests that reproduce/confirm the InvalidCastException scenario described in #3314 by forcing SqlConnection’s internal _innerConnection into the DbConnectionClosedConnecting transitional state and validating resulting behavior.

Changes:

• Added reflection-based tests validating that the DbConnectionClosedConnecting singleton is not assignable to SqlConnectionInternal.
• Added a test confirming SqlConnection.State reports ConnectionState.Connecting when _innerConnection is the connecting singleton.
• Added a test confirming Open() throws InvalidOperationException when _innerConnection is already in the connecting state.

[Copilot]

Pull request overview

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

[paulmedynski]

Addressed in commit b077628. Test was renamed to TryOpenInner_WhenInnerConnectionRacesToNonSqlConnectionInternalState_ThrowsInvalidOperation_NotInvalidCast to accurately reflect that it exercises a non-SqlConnectionInternal state (DbConnectionOpenBusy), not a connecting state. Local variables renamed for clarity: connectingSingleton → initialConnectingState, openBusyInstance → racedNonSqlConnectionInternalState. This improves regression readability and avoids confusion about which transitional state the test validates. All 4 concurrent open tests pass on net9.0, net10.0.

[paulmedynski]

Addressed in commit b077628. Exception path now uses innerConnection.State instead of re-reading State, ensuring the thrown message reflects the same snapshot as the type check. This avoids a potential concurrent read under contention and keeps the entire sequence aligned with the captured local variable. See line 2237 in src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs.

[codecov]

Codecov Report

❌ Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 64.28%. Comparing base (3af6e09) to head (b077628).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...ient/src/Microsoft/Data/SqlClient/SqlConnection.cs	80.00%	1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (3af6e09) and HEAD (b077628). Click for more details.

HEAD has 2 uploads less than BASE

Flag	BASE (3af6e09)	HEAD (b077628)
CI-SqlClient	2	0

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4179      +/-   ##
==========================================
- Coverage   73.62%   64.28%   -9.35%     
==========================================
  Files         277      272       -5     
  Lines       42988    65786   +22798     
==========================================
+ Hits        31650    42289   +10639     
- Misses      11338    23497   +12159     

Flag	Coverage Δ
CI-SqlClient	?
PR-SqlClient-Project	64.28% <80.00%> (?)

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.