reparentutil: ERS panic recovery, PRS buffer-pool tiebreaking, and cleanup

[timvaillancourt]

Description

Implements the four fixes flagged in #20204. Each is small but they share a theme — error handling and resource management around ERS/PRS — so I bundled them rather than splitting into 4 x trivial PRs

1. ERS panic recovery (emergency_reparenter.go)

reparentReplicas() wraps both the per-replica handleReplica and the replWg.Wait() waiter goroutine in recover(). A panic in handleReplica is now recorded into the existing AllErrorRecorder instead of taking down the surrounding vtctld/vtctldserver process. A panic in the waiter (e.g. a replWg.Done() over-count) no longer skips allReplicasDoneCancel() / replCancel() — those are now defer-ed before the recover

The pre-existing defer replWg.Done() already guaranteed Wait() would return during a handleReplica panic, but the process was still going down before this change

2. PRS buffer-pool tiebreaking (planned_reparenter.go + util.go)

verifyAllTabletsReachable() no longer swallows the strconv.Atoi error. A tablet whose Innodb_buffer_pool_pages_data is missing (e.g. MariaDB, which doesn't expose this status variable) is omitted from the buffer-pool tiebreaking map — the tablet itself still participates in PRS normally, just without a buffer-pool warmth score. A present-but-empty value is treated the same as a missing key (to avoid log spam on variants that return the row with no value); a present-but-non-numeric value is omitted with a warning log

ElectNewPrimary() only uses buffer-pool data for tiebreaking when every valid tablet has a value — gated on validTablets (post-RPC filtering) rather than candidates, so a backup-taking or excessively-lagged candidate doesn't disable tiebreaking for the rest. If any valid tablet is missing data, innodbBufferPool is left nil and the sorter's existing len(...) != 0 check falls back to durability ordering — same all-or-nothing semantics it has always had

3. Slice preallocation

3 x hot paths with known upper bounds:

• filterValidCandidates: restrictedValidTablets / notPreferredValidTablets capped at len(validTablets)
• ElectNewPrimary: validTablets / tabletPositions capped at len(candidates) and moved out of the var() block to where the bound is known
• stopReplicationAndBuildStatusMaps: reachableTablets capped at len(tabletMap)

One test case in emergency_reparenter_test.go updated to match the resulting nil → empty-slice change for an empty filter result

4. waitForCatchUp cleanup

Collapses the trailing if err != nil { return err }; return nil to a single return tmc.WaitForPosition(...)

Why not backport

These are incremental improvements, not regressions. The exposure for #1 is real but bounded — handleReplica panics aren't observed in practice and the existing defer replWg.Done() already handled replWg accounting

Related Issue(s)

Resolves: #20204
Related: #19849
Related: #19888

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

No new flags or configuration. Behaviour change in #2 is limited to PRS tiebreaking semantics — PRS still runs against all tablet variants. In a mixed cluster where some tablets expose Innodb_buffer_pool_pages_data and some don't, PRS now falls back to durability tiebreaking instead of using 0 as a real value for the tablets that don't expose it. In practice this only flips when there's a tie on GTID position (rare in a steady-state shard). In a pure-MariaDB cluster the outcome is unchanged — pre-fix, all tablets tied at 0 and fell through; post-fix, buffer-pool tiebreaking is skipped and falls through to the same next criterion

AI Disclosure

Claude Code assisted with the implementation, testing, and PR summary. I committed the change manually after reviewing each step

[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.

[codecov]

Codecov Report

❌ Patch coverage is 88.23529% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.26%. Comparing base (70c7a72) to head (84c35dd).
⚠️ Report is 294 commits behind head on main.

Files with missing lines	Patch %	Lines
go/vt/vtctl/reparentutil/emergency_reparenter.go	66.66%	4 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main   #20205       +/-   ##
===========================================
+ Coverage   69.67%   91.26%   +21.59%     
===========================================
  Files        1614        9     -1605     
  Lines      216793     1339   -215454     
===========================================
- Hits       151044     1222   -149822     
+ Misses      65749      117    -65632     

Flag	Coverage Δ
partial	91.26% <88.23%> (?)

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

[promptless]

Promptless prepared a documentation update related to this change.

Triggered by PR #20205

Added a changelog entry documenting the PRS buffer-pool tiebreaking behavior change. Tablets missing Innodb_buffer_pool_pages_data (such as MariaDB) are now excluded from buffer-pool tiebreaking, falling back to durability-based ordering instead of using a misleading zero value.

Review: Add changelog entry for PRS buffer-pool tiebreaking behavior change

[mattlord]

LGTM. There's a residual risk here in that the buffer-pool fallback behavior is subtle and currently has limited direct regression coverage for missing/empty/non-numeric status values and the “skip warmth tiebreaking unless every valid tablet has data” path. I’d prefer a targeted unit test there, but I don’t see a code issue that should block the PR.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.