Skip to content

[5.1] Backport BAG failover fix. Ignore server provided failover partner. #3704

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 10 commits into from
Oct 28, 2025
Merged

[5.1] Backport BAG failover fix. Ignore server provided failover partner. #3704

merged 10 commits into from
Oct 28, 2025

Conversation

@mdaigle
Copy link
Contributor

@mdaigle mdaigle commented Oct 20, 2025 •
edited
Loading

Description

Backports #3625

@mdaigle
Fix #3400, ignore server-provided failover partner (#3625)
86658d8 
* Add IgnoreServerProvidedFailoverPartner app context switch.

* Add behavior skip to netfx.

* Consolidate to single property for failover partner value.

* Rework checks to preserve server provided value, but ignore it.

* Fix import.

* Skip server failover partner override in �LoginNoFailover method. Add simulated server test coverage.
@mdaigle
Fix compilation issues.
4daba6f
@mdaigle
Clean up duplicate line. Adjust app context switch logic to match exi…
6e2732e 
…sting.
@mdaigle mdaigle marked this pull request as ready for review October 20, 2025 22:12
@mdaigle mdaigle requested review from a team and Copilot October 20, 2025 22:12
Copilot AI reviewed Oct 20, 2025
View reviewed changes
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 PR backports a fix from PR #3625 to the 5.1 branch that addresses Basic Availability Group (BAG) failover issues by adding an option to ignore server-provided failover partners. The implementation adds a new AppContext switch that allows applications to explicitly control failover behavior instead of relying on server-provided values.

Key Changes:

  • Introduced IgnoreServerProvidedFailoverPartner AppContext switch to control failover partner behavior
  • Refactored ServerProvidedFailOverPartner property from a field-backed read-only property to an auto-property with consistent naming
  • Updated failover logic to conditionally use server-provided values based on the new switch

Reviewed Changes

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

Show a summary per file
File Description
LocalAppContextSwitches.cs Adds the new IgnoreServerProvidedFailoverPartner switch with documentation
SqlInternalConnectionTds.cs (netfx) Refactors failover partner property and implements conditional logic based on the new switch
SqlInternalConnectionTds.cs (netcore) Same changes as netfx version for .NET Core platform
SqlCommand.cs (netfx) Updates property reference to use corrected naming
SqlCommand.cs (netcore) Updates property reference to use corrected naming

@mdaigle mdaigle added this to the 5.1.8 milestone Oct 20, 2025
@codecov
Copy link

codecov bot commented Oct 20, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 63.63636% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.99%. Comparing base (1286cfe) to head (228009d).
⚠️ Report is 2 commits behind head on release/5.1.

Files with missing lines Patch % Lines
...crosoft/Data/SqlClient/SqlInternalConnectionTds.cs 52.38% 10 Missing ⚠️
...crosoft/Data/SqlClient/SqlInternalConnectionTds.cs 64.70% 6 Missing ⚠️
Additional details and impacted files 
@@               Coverage Diff               @@
##           release/5.1    #3704      +/-   ##
===============================================
+ Coverage        71.82%   71.99%   +0.16%     
===============================================
  Files              293      293              
  Lines            61659    61610      -49     
===============================================
+ Hits             44289    44354      +65     
+ Misses           17370    17256     -114
Flag Coverage Δ
addons 92.38% <ø> (ø)
netcore 76.11% <72.72%> (+0.15%) ⬆️
netfx 69.66% <61.53%> (+0.14%) ⬆️

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.

paulmedynski
paulmedynski requested changes Oct 21, 2025
View reviewed changes
Copy link
Contributor

@paulmedynski paulmedynski left a comment

Choose a reason for hiding this comment

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

I think these changes are risky without the accompanying tests.

benrr101
benrr101 previously approved these changes Oct 21, 2025
View reviewed changes
Copy link
Contributor

@benrr101 benrr101 left a comment

Choose a reason for hiding this comment

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

Paul's comments are good - Since the overall approval is gated on his, I'll approve.

@mdaigle
Copy link
Contributor Author

mdaigle commented Oct 22, 2025

@benrr101 @paulmedynski comments addressed and I backported the test included in the original PR.

paulmedynski
paulmedynski requested changes Oct 23, 2025
View reviewed changes
Copy link
Contributor

@paulmedynski paulmedynski left a comment

Choose a reason for hiding this comment

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

Asking for one clarification.

@paulmedynski paulmedynski self-assigned this Oct 23, 2025
paulmedynski
paulmedynski previously approved these changes Oct 23, 2025
View reviewed changes
@mdaigle mdaigle merged commit 11c1f15 into release/5.1 Oct 28, 2025
80 checks passed
@mdaigle mdaigle deleted the dev/mdaigle/5.1-bag-failover-fix branch October 28, 2025 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@paulmedynski paulmedynski paulmedynski approved these changes

Copilot code review Copilot Copilot left review comments

@benrr101 benrr101 benrr101 approved these changes

Assignees

@paulmedynski paulmedynski

Labels

None yet

Projects

None yet

Milestone

5.1.8

Development

Successfully merging this pull request may close these issues.

4 participants

@mdaigle @benrr101 @paulmedynski