Add database context reconnection tests and diagnostics

[paulmedynski]

Description

Adds testing and diagnostic instrumentation for database context behavior during transparent reconnection. This pull request investigates the scenario reported in #4108; it does not fix or close that issue.

Driver behavior is unchanged on every path, and there is no new public API.

After a successful session recovery login, CompleteLogin compares the database captured before the connection dropped with the database the server reported during recovery. When they differ it emits a trace through the existing Microsoft.Data.SqlClient.EventSource provider, carrying the connection's object id plus the expected and reported database names. Recovery itself is deliberately left uncorrected, so the reported failure can be observed without being altered.

The trace is written under the provider's existing Trace keyword (2), so it can be collected with EventListener, PerfView or dotnet-trace using the tooling that ships today. No AppContext switch is needed to turn diagnostics on.

The comparison is ordinal. A case-sensitive server can host databases whose names differ only by case, so a case-insensitive match would treat two distinct recovery targets as equal and miss the mismatch.

Issues

Related to #4108. This pull request adds investigation coverage and diagnostics only; it does not resolve the reported issue.

Testing

Unit tests — 13 simulated-server tests. The mismatch scenarios assert the emitted trace through an EventListener subscribed to the real provider and keyword, with no test-only driver hook. The matching-recovery scenario asserts that no trace is emitted, so a comparison that fires when the databases already agree would be caught. Every reconnection test also asserts that no USE batch reaches the server during recovery, proving the recovery path is untouched.

Integration tests — 11 KILL-based tests covering synchronous and asynchronous reconnection workflows. They require configured live-server test infrastructure and are gated on AreConnStringsSetup, IsNotAzureServer and IsNotAzureSynapse, because they create a database and use KILL.

[Copilot]

Pull request overview

Adds simulated-server coverage and supporting test infrastructure to reproduce #4108 (database context reverting after transparent reconnection), along with internal analysis docs and small repo policy/tooling updates.

Changes:

• Added new simulated TDS server unit tests covering USE [db] / ChangeDatabase() and post-disconnect reconnection behavior.
• Extended the TDS test server utilities to forcibly disconnect active clients while keeping the listener running.
• Added analysis documents under plans/database_context/, updated coding-style guidance, and introduced a markdownlint configuration.

Reviewed changes

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

Show a summary per file

File	Description
src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/DatabaseContextReconnectionTests.cs	New unit tests and a custom query engine to simulate USE [db] and validate database context across reconnects.
src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.Servers/GenericTdsServer.cs	Adds DisconnectAllClients() helper to drop all active client connections.
src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.EndPoint/TDSServerEndPoint.cs	Adds DisconnectAll() implementation to dispose all active endpoint connections without stopping the listener.
policy/coding-style.md	Updates style guidance (line wrapping and #region usage).
plans/database_context/00-overview.md	Overview of the database-context reconnection investigation.
plans/database_context/01-architecture.md	Architecture notes on session/database tracking and recovery.
plans/database_context/02-flows.md	Enumerates reconnection flows and whether DB context is preserved.
plans/database_context/03-issues.md	Lists identified issues/gaps and severity.
plans/database_context/04-recommendations.md	Proposed fixes and test recommendations.
plans/database_context/05-reconnection-and-retry-mechanisms.md	Catalogues retry/reconnect mechanisms and DB-context implications.
.markdownlint.jsonc	Adds markdownlint config aligned to the repo’s line-length policy.

[paulmedynski]

We see the expected 3 unit tests failing here:

https://sqlclientdrivers.visualstudio.com/public/_build/results?buildId=145859&view=results

The next commit will contain fixes to the codebase, and those tests should pass.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

src/Microsoft.Data.SqlClient/ref/Microsoft.Data.SqlClient.csproj:58

• MSBuild property PowerShellCommand becomes self-referential here: it’s first set to powershell.exe/pwsh and then overwritten with a value that expands $(PowerShellCommand) ..., which can create a circular property expansion at build time. Consider splitting into two properties (e.g., PowerShellExe + PowerShellArgs/TrimDocsCommand) or use a differently named property for the command line so the executable selection isn’t overwritten.

      <PowerShellCommand Condition="'$(OS)' == 'Windows_NT'">powershell.exe</PowerShellCommand>
      <PowerShellCommand Condition="'$(OS)' != 'Windows_NT'">pwsh</PowerShellCommand>
      <PowerShellCommand>
        $(PowerShellCommand)
          -NonInteractive
          -ExecutionPolicy Unrestricted
          -Command "$(RepoRoot)tools\intellisense\TrimDocs.ps1 -inputFile '$(DocumentationFile)' -outputFile '$(DocumentationFile)'"
      </PowerShellCommand>

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.71%. Comparing base (9162f07) to head (8638119).
⚠️ Report is 8 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4130      +/-   ##
==========================================
- Coverage   66.91%   64.71%   -2.21%     
==========================================
  Files         292      286       -6     
  Lines       45346    68430   +23084     
==========================================
+ Hits        30345    44282   +13937     
- Misses      15001    24148    +9147     

Flag	Coverage Δ
CI-SqlClient	?
PR-SqlClient-Project	64.71% <100.00%> (?)

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

☔ View full report in Codecov by Harness.
📢 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.

[cheenamalhotra]

Every new context switch is a public API, and we have historically never added a new API/switch for enabling diagnostics for a specific usecase. And this switch is doing more than just diagnostics as the PR description says, it changes the connection behavior, it sends USE during login and emits no diagnostic event SqlConnectionInternal.cs:2404–2445. That makes it harder to observe the original failure without also changing the outcome.

There is a separate correctness risk at line 2445: assigning CurrentDatabase unconditionally could report success if the corrective batch completes without confirming the expected database change.

Alternative: Keep recovery behavior unchanged and emit a mismatch event through the existing Microsoft.Data.SqlClient.EventSource provider, using SqlClientEventSource.Log.TryTraceEvent at the comparison point. Users can enable its existing Trace keyword (2) with EventListener or tracing tools; no new AppContext switch is needed to turn on diagnostics. Include the connection identifier and expected/reported database names, and add a listener-based test. If corrective USE is desired, name and review that separately as an opt-in recovery behavior—not as the diagnostic mechanism.

[Copilot]

Copilot review overview

🟡 Changes recommended

The manual tests bypass required RAII table fixtures, and one listener comment documents initialization order incorrectly.

Review effort: Balanced
Findings: 1 · 1

Open (2)

• Use DatabaseObjects.Table RAII fixture for transient table cleanup · New
• Correct initialization-order comment to reflect callback safety constraint · New

[Copilot]

Copilot review overview

🟡 Changes recommended

Unrelated policy/tooling changes should be separated, and several tests omit the advertised no-USE assertion.

Review effort: Balanced
Findings: 4 · 9

Open (13)

• Assert reconnection emits no corrective USE · New
• Verify recovery sends no corrective USE batch · New
• Assert pooled recovery emits no USE batch · New
• Use DatabaseObjects.Table RAII fixture for transient table cleanup
• Separate unrelated Azure DevOps access policy update · New
• Separate unrelated Azure DevOps tool-scope changes · New
• Separate unrelated agent-link guidance change · New
• Separate unrelated Azure DevOps precedence update · New
• Separate unrelated markdownlint configuration change · New
• Separate unrelated coding-style policy changes · New
• Document the spid parameter · New
• Document all three helper parameters · New
• Correct initialization-order comment to reflect callback safety constraint

[paulmedynski]

@cheenamalhotra - Great suggestion, and I have implemented it from scratch, removing the old commits.

[Copilot]

Copilot review overview

🔵 Needs a closer look

The diagnostic can misstate server behavior, manual cleanup violates repository conventions, and unrelated policy changes expand the stated scope.

Review effort: Balanced
Findings: None

Resolved since last review (13)

• Assert pooled recovery emits no USE batch
• Verify recovery sends no corrective USE batch
• Assert reconnection emits no corrective USE
• Use DatabaseObjects.Table RAII fixture for transient table cleanup
• Document all three helper parameters
• Document the spid parameter
• Separate unrelated coding-style policy changes
• Separate unrelated markdownlint configuration change
• Separate unrelated Azure DevOps precedence update
• Separate unrelated agent-link guidance change
• Separate unrelated Azure DevOps tool-scope changes
• Separate unrelated Azure DevOps access policy update
• Correct initialization-order comment to reflect callback safety constraint

Previously missed (1)

In code that hasn't changed since last review

Diagnostic mislabels client database as server-reported

src/​Microsoft.Data.SqlClient/​src/​Microsoft/​Data/​SqlClient/​Connection/​SqlConnectionInternal.cs:2393

CurrentDatabase is not always a database the server reported: when the recovery response omits the database ENV_CHANGE, it retains the login's preinitialized catalog, yet this trace says the server reported that value. The new omitted-token tests exercise exactly this case, so the diagnostic can misidentify server behavior. Either track whether a database ENV_CHANGE was received or label this value as the database observed by the client; update the trace assertions accordingly.

[paulmedynski]

Review body — Copilot, "Previously missed": Diagnostic mislabels client database as server-reported

Valid, and fixed in ddb1ed3.

The trace attributed CurrentDatabase to the server even when the recovery response carried no database ENV_CHANGE. In that case the value is the login's pre-initialized catalog, so the trace named a database the server never reported. The omitted-token tests asserted Assert.Null(server.LastLoginResponseDatabase) and, in the same test, that the trace said the server reported initialdb — the tests encoded the contradiction.

I took the first of the two suggested remedies rather than relabelling the value, because the two cases cannot be told apart by value alone: a server may legitimately report exactly the initial catalog, which is byte-identical to the fallback. SendInitialCatalog and OmitDatabaseEnvChange would otherwise produce an identical trace while representing different server faults, and distinguishing those is the point of this investigation.

CompleteLogin now tracks whether an ENV_DATABASE token arrived while the login response was parsed, and reports the two behaviors distinctly:

• server reported a different database, or
• the recovery response carried no database ENV_CHANGE and the connection fell back.

The flag is reset immediately before that parse and read only while completing the same login, so a later USE cannot leave it stale and a re-login after routing cannot inherit the previous attempt's value.

Mutation-checked: forcing the server-reported branch fails exactly the two omitted-ENV_CHANGE tests and no others. The assertion helper is split so the omitted cases also assert the trace does not claim the server reported anything.

Also in this review: the 13 findings from the previous round are confirmed Resolved since last review, and this review carries no suppressed findings.

[Copilot]

Copilot review overview

🔵 Needs a closer look

The PR description still omits the intentional repository tooling and policy changes included in the diff.

Review effort: Balanced
Findings: None

[Copilot]

@cheenamalhotra I'm unable to start working on this because of repository rules that prevent me from pushing to the branch:

• Changes must be made through a pull request due to repository rules

See the documentation for more details.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Core login recovery instrumentation and extensive network-failure testing warrant final human validation.

Review effort: Balanced
Findings: None