vtorc: add StaleTopoPrimary analysis and recovery

[mhamza15]

Description

When demoting a primary during ERS, if the update of the tablet's type in the topology fails and it remains PRIMARY, the tablet will remain PRIMARY indefinitely, and vtorc will continuously run LockedSemiSyncPrimaryHypothesis. This adds a new analysis StaleTopoPrimary, which finds tablets of type PRIMARY in the topology, but there exists a tablet with a more recent primary term start time.

The recovery will demote this primary (forcefully, as this usually means there are pending acks), attempt to reattach the tablet to the current primary (and likely fail), and update the topology with the correct type REPLICA.

Related Issue(s)

Closes #19172

Checklist

• "Backport to:" labels have been added if this change should be back-ported to release branches
• If this change is to be back-ported to previous releases, a justification is included in the PR description
• Tests were added or are not required
• Did the new or modified tests pass consistently locally and on CI?
• Documentation was added or is not required

Deployment Notes

AI Disclosure

Help from GPT 5.2 Codex.

[vitess-bot]

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

• Ensure that the Pull Request has a descriptive title.
• Ensure there is a link to an issue (except for internal cleanup and flaky test fixes), new features should have an RFC that documents use cases and test cases.

Tests

• Bug fixes should have at least one unit or end-to-end test, enhancement and new features should have a sufficient number of tests.

Documentation

• Apply the release notes (needs details) label if users need to know about this change.
• New features should be documented.
• There should be some code comments as to why things are implemented the way they are.
• There should be a comment at the top of each new or modified test to explain what the test does.

New flags

• Is this flag really necessary?
• Flag names must be clear and intuitive, use dashes (-), and have a clear help text.

If a workflow is added or modified:

• Each item in Jobs should be named in order to mark it as required.
• If the workflow needs to be marked as required, the maintainer team must be notified.

Backward compatibility

• Protobuf changes should be wire-compatible.
• Changes to _vt tables and RPCs need to be backward compatible.
• RPC changes should be compatible with vitess-operator
• If a flag is removed, then it should also be removed from vitess-operator and arewefastyet, if used there.
• vtctl command output order should be stable and awk-able.

[promptless]

📝 Documentation updates detected!

New suggestion: Document StaleTopoPrimary VTOrc analysis and recovery

[mhamza15]

This was needed because we seem to be storing the timestamp in RFC3339Nano format, but then reading it as DateTimeFormat (ISO8601). I find this hard to believe because somehow that means we haven't needed to read timestamps back out at all? But it seems like that's the case 🤷

[mattlord]

We use that format for histograms buckets in the stats package. I'm not aware of any other usage. Where was this needed here?

[mhamza15]

When vtorc is reading the primary term start time, I was getting parsing errors leading to it being parsed as the zero value.

[timvaillancourt]

@mhamza15 I haven't done a full review, but this feels like it might overlap the existing stale PRIMARYs fix in VTOrc. Does that not resolve the problem? If it doesn't, I wonder if there is a bug in the current implementation, or perhaps our first approach wasn't ideal

Existing problem here: https://github.com/vitessio/vitess/blob/main/go/vt/vtorc/inst/analysis_dao.go#L459

[mhamza15]

@mhamza15 I haven't done a full review, but this feels like it might overlap the existing stale PRIMARYs fix in VTOrc. Does that not resolve the problem? If it doesn't, I wonder if there is a bug in the current implementation, or perhaps our first approach wasn't ideal

Existing problem here: https://github.com/vitessio/vitess/blob/main/go/vt/vtorc/inst/analysis_dao.go#L459

That one looks to be the inverse, where the current primary is not running as a primary tablet. The new one added here is for a primary that isn't the current primary but still has a tablet type of primary in the topology. Since there exists a more recent and currently active primary, we can demote the stale one.

[codecov]

Codecov Report

❌ Patch coverage is 18.86792% with 43 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.89%. Comparing base (66b1671) to head (f20e9d6).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
go/vt/vtorc/logic/topology_recovery.go	0.00%	43 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #19173      +/-   ##
==========================================
- Coverage   69.91%   69.89%   -0.03%     
==========================================
  Files        1611     1611              
  Lines      215958   216011      +53     
==========================================
- Hits       150988   150981       -7     
- Misses      64970    65030      +60     

☔ 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.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[mattlord]

@mhamza15 is this a bug or an enhancement? If it's truly a bug, we might want to backport it to v23. What do you think?

[mhamza15]

@mhamza15 is this a bug or an enhancement? If it's truly a bug, we might want to backport it to v23. What do you think?

I think it makes sense to backport it! Any reason you didn't include v22?

[mattlord]

This seems fine to me. I do wonder though if the underlying cause is often that the tablet is unable to successfully complete the state transition for some reason. Do you have any thoughts on that?

[mattlord]

We use that format for histograms buckets in the stats package. I'm not aware of any other usage. Where was this needed here?

[mattlord]

What if this is the problem, the demotion is stuck for some reason. Do we have any other more forceful means at our disposal?

[mattlord]

Ah, never mind! 🤣 I was forgetting that we/Arthur recently added the "force" option to DemotePrimary and we are setting that to true here. ❤️

[mhamza15]

Yep, and this would likely fail without it as the stale primary will more often than not have errant GTIDs.

[nickvanw]

one question about a comment.

[nickvanw]

We say that this method repoints replication, but I don't think this is right - it doesn't actually call SetReplicationSource I think, right?

I'm guessing we expect a later analysis to fix replication, but it might be worth making the comment right or calling setReplicationSource.

[mhamza15]

Good catch, it must've slipped through. I'll update it to actually do what it says it does.

[promptless]

📝 Documentation updates detected!

Updated existing suggestion: Document StaleTopoPrimary VTOrc analysis and recovery

[eseokoh]

I think demoteStaleTopoPrimary also needs setReadOnly, right?
If the tablet was demoted as writable, I believe fixReplica will resolve it later, but we can do it here to make it sure.

[mhamza15]

I think demoteStaleTopoPrimary also needs setReadOnly, right? If the tablet was demoted as writable, I believe fixReplica will resolve it later, but we can do it here to make it sure.

The DemotePrimary RPC handles that for us:

vitess/go/vt/vttablet/tabletmanager/rpc_replication.go

Lines 536 to 551 in 6d49e7b

	// DemotePrimary prepares a PRIMARY tablet to give up leadership to another tablet.
	//
	// It attempts to idempotently ensure the following guarantees upon returning
	// successfully:
	// - No future writes will be accepted.
	// - No writes are in-flight.
	// - MySQL is in read-only mode.
	// - Semi-sync settings are consistent with a REPLICA tablet.
	//
	// If necessary, it waits for all in-flight writes to complete or time out.
	//
	// It should be safe to call this on a PRIMARY tablet that was already demoted,
	// or on a tablet that already transitioned to REPLICA.
	//
	// If a step fails in the middle, it will try to undo any changes it made.
	func (tm *TabletManager) DemotePrimary(ctx context.Context, force bool) (*replicationdatapb.PrimaryStatus, error) {