Harden SqlConnection state transitions with CAS guards

[paulmedynski]

Summary

• Hardened SqlConnection inner-state transitions with targeted Interlocked.CompareExchange guards.
• Replaced opportunistic transition writes with expected-state transition validation in critical paths.
• Added focused unit tests for race-path determinism, transition validity, and side-effect invariants (state-change events and close-count behavior).

Key Changes

• TryOpenInner now snapshots and type-checks the inner connection before parser access.
• SetInnerConnectionEvent now:
  • validates expected source states,
  • uses CAS to commit transition,
  • applies side effects only after a successful CAS commit.
• SetInnerConnectionTo now:
  • validates expected source states,
  • uses CAS to avoid stale overwrites.
• ConnectionString_Set now releases Busy -> Closed via guarded helper instead of direct write.
• Added XML docs and inline comments clarifying transition intent and guard rationale.

Tests

• Added/updated: SqlConnectionStateTransitionTests
• Verified focused suite passes:
  • dotnet test src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft.Data.SqlClient.UnitTests.csproj --filter "FullyQualifiedName~SqlConnectionStateTransitionTests" -p:Configuration=Debug -p:ReferenceType=Project

App-visible Behavior Analysis

Potential app-visible behavior changes are limited to race paths; normal single-threaded use should be unchanged.

1. Open race now fails with a stable InvalidOperationException instead of InvalidCastException

• In SqlConnection.TryOpenInner, the method now snapshots and type-checks the inner connection after open/replace.
• Before: a narrow race could surface InvalidCastException.
• After: the same race surfaces the normal closed-connection InvalidOperationException path.
• Visible effect: apps that were catching InvalidCastException from this race would now see InvalidOperationException.

2. Invalid/stale transition writers are now rejected instead of silently overwriting state

• SetInnerConnectionEvent and SetInnerConnectionTo now enforce expected source states and use CAS.
• Before: stale or unexpected writers could sometimes overwrite state opportunistically.
• After: those paths throw deterministic internal-connection InvalidOperationException errors in contention cases.
• Visible effect: rare concurrent misuse/race scenarios may now fail earlier and more consistently, rather than progressing with inconsistent state.

3. Transition side effects are tied to committed transitions only

• Close-count and state-change side effects now occur only after CAS success.
• Visible effect: under heavy concurrency, apps observing StateChange patterns may see fewer spurious side effects from losing writers.

4. Connection-string update release path is now guarded

• ConnectionString_Set now releases Busy -> Closed through the guarded helper rather than direct assignment.
• Visible effect: concurrent open/set-connection-string races are more likely to produce deterministic transition errors instead of silent state drift.

5. Non-visible change

• TryOpenInner access changed from private to internal for testability.
• This is not part of public API surface and should not affect application source compatibility.

Checklist

• Tests added or updated
• Public API changes documented
• Verified against targeted repro/tests
• No intentional breaking public API changes

Observability Notes

• Introduced dedicated ADP.ConnectionError values for guarded transition failures:
  • InvalidSourceStateForSetInnerConnectionTo
  • FailedToCommitSetInnerConnectionTo
  • InvalidSourceStateForSetInnerConnectionEvent
  • FailedToCommitSetInnerConnectionEvent
• This improves diagnostics by separating source-state validation failures from CAS contention/commit failures in SetInnerConnectionTo and SetInnerConnectionEvent.
• App-facing exception type remains InvalidOperationException; however, the internal connection error code embedded in the message (Internal DbConnection Error: {0}) is now more specific for these paths.

[Copilot]

Pull request overview

This pull request hardens SqlConnection’s internal state machine by guarding critical inner-connection transitions with Interlocked.CompareExchange (CAS), making contention/race paths deterministic and ensuring side effects (close-count, StateChange) occur only for committed transitions.

Changes:

• Made TryOpenInner snapshot/type-check the active inner connection before accessing parser state (and exposed it as internal for unit testing).
• Updated SetInnerConnectionEvent / SetInnerConnectionTo to validate expected source states, commit with CAS, and apply side effects only after a successful CAS.
• Added new unit tests covering race-path determinism and transition side-effect invariants.

Reviewed changes

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

File	Description
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs	Adds CAS-based guards + expected-state validation for key inner-connection transitions; snapshots/type-checks inner connection in TryOpenInner.
src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionStateTransitionTests.cs	Adds regression tests for race-sensitive state transitions and side-effect invariants.

[codecov]

Codecov Report

❌ Patch coverage is 78.26087% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.38%. Comparing base (be95ca2) to head (cdfd27a).
⚠️ Report is 5 commits behind head on main.

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4267      +/-   ##
==========================================
- Coverage   65.96%   64.38%   -1.58%     
==========================================
  Files         275      270       -5     
  Lines       42993    65794   +22801     
==========================================
+ Hits        28361    42363   +14002     
- Misses      14632    23431    +8799     

Flag	Coverage Δ
CI-SqlClient	?
PR-SqlClient-Project	64.38% <78.26%> (?)

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.