Skip to content

Feature | Fix enhanced routing ack handling.#3973

Open
mdaigle wants to merge 1 commit intomainfrom
dev/mdaigle/enhanced-routing-3-redo
Open

Feature | Fix enhanced routing ack handling.#3973
mdaigle wants to merge 1 commit intomainfrom
dev/mdaigle/enhanced-routing-3-redo

Conversation

@mdaigle
Copy link
Contributor

@mdaigle mdaigle commented Feb 20, 2026

Ports #3922 to main

Copilot AI review requested due to automatic review settings February 20, 2026 20:01
@mdaigle mdaigle closed this Feb 20, 2026
@mdaigle mdaigle reopened this Feb 20, 2026
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request fixes the handling of enhanced routing acknowledgment in SQL Server connections. The PR ensures that enhanced routing information (which includes a database name) is only used when the server explicitly acknowledges support for the FEATUREEXT_ENHANCEDROUTINGSUPPORT feature extension. If the server doesn't acknowledge the feature or explicitly disables it, the routing information is ignored and the connection stays with the current server.

Changes:

  • Added validation logic to ignore enhanced routing info when DatabaseName is present but IsEnhancedRoutingSupportEnabled is false
  • Updated OnFeatureExtAck to process enhanced routing feature acknowledgment even during routing
  • Refactored unit tests to use a helper class pattern and cover both sync and async code paths

Reviewed changes

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

File Description
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs Added enhanced routing acknowledgment validation in LoginNoFailover and LoginWithFailover methods; updated OnFeatureExtAck to allow processing of enhanced routing feature
src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionEnhancedRoutingTests.cs Refactored tests to use TestRoutingServers helper class; converted to Theory-based tests covering sync/async paths and multiple server behaviors

$"SqlInternalConnectionTds.LoginWithFailover | " +
$"Ignoring enhanced routing info because the server did not acknowledge the feature.");
RoutingInfo = null;
continue;
Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of continue here is inconsistent with the equivalent logic in LoginNoFailover (line 3299), which uses break. Both are functionally equivalent since RoutingInfo is set to null immediately before, but break is more explicit and clearer about the intent to exit the routing loop. Consider changing continue to break for consistency.

Suggested change
continue;
break;

Copilot uses AI. Check for mistakes.
@mdaigle mdaigle linked an issue Feb 20, 2026 that may be closed by this pull request
@mdaigle mdaigle marked this pull request as ready for review February 20, 2026 20:05
@mdaigle mdaigle requested a review from a team as a code owner February 20, 2026 20:05
@mdaigle mdaigle added this to the 7.0.0-preview4 milestone Feb 24, 2026
paulmedynski
paulmedynski previously approved these changes Feb 24, 2026
@paulmedynski paulmedynski self-assigned this Feb 24, 2026
samsharma2700
samsharma2700 previously approved these changes Feb 24, 2026
@samsharma2700 samsharma2700 self-assigned this Feb 24, 2026
@mdaigle mdaigle force-pushed the dev/mdaigle/enhanced-routing-2 branch 2 times, most recently from 94e7051 to 0db8b2b Compare February 26, 2026 17:09
@paulmedynski
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command.

Base automatically changed from dev/mdaigle/enhanced-routing-2 to main February 26, 2026 21:10
@mdaigle mdaigle dismissed stale reviews from samsharma2700 and paulmedynski February 26, 2026 21:10

The base branch was changed.

@mdaigle mdaigle force-pushed the dev/mdaigle/enhanced-routing-3-redo branch from 96a5e18 to 28f7ef2 Compare February 26, 2026 21:23
{
RoutingTCPHost = "localhost",
RoutingTCPPort = (ushort)TargetServer.EndPoint.Port,
RoutingDatabaseName = RoutingDatabaseName,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RoutingTdsServers always send a routing or enhanced routing envchange token. They only acknowledge the feature extension based on the specific feature extension behavior.

@codecov
Copy link

codecov bot commented Feb 27, 2026

Codecov Report

❌ Patch coverage is 53.33333% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.58%. Comparing base (8a22d4f) to head (28f7ef2).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
...Data/SqlClient/Connection/SqlConnectionInternal.cs 53.33% 7 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (8a22d4f) and HEAD (28f7ef2). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (8a22d4f) HEAD (28f7ef2)
CI-SqlClient 1 0
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3973      +/-   ##
==========================================
- Coverage   74.20%   64.58%   -9.63%     
==========================================
  Files         287      282       -5     
  Lines       43881    66034   +22153     
==========================================
+ Hits        32562    42645   +10083     
- Misses      11319    23389   +12070     
Flag Coverage Δ
CI-SqlClient ?
PR-SqlClient-Project 64.58% <53.33%> (?)

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support routing to named read replicas

4 participants