reparentutil: fix goroutine count and improve logging

[timvaillancourt]

Description

2 x small fixes found during an audit of the reparentutil package:

1. Incorrect goroutine count in stopReplicationAndBuildStatusMaps() — numGoRoutines was computed as len(tabletMap) - ignoredTablets.Len(), but ignoredTablets can contain aliases not present in tabletMap (e.g., stale entries), making the subtraction wrong. Now counts goroutines directly in the launch loop. Also added an early FAILED_PRECONDITION return when tablets exist but all are excluded by IgnoreReplicas, which previously produced requiredSuccesses = -1 (invalid ErrorGroup parameters)

2. RefreshState warning missing tablet alias — The RefreshState best-effort warning in reparentShardLocked() didn't include the tablet alias, making it hard to debug. Added the alias and a rationale comment explaining why this intentionally does not return an error (VTOrc and other callers rely on nil returns from successful reparents)

Related Issue(s)

Closes: #19896

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

N/A

AI Disclosure

Development assisted by Claude. Claude prepared this PR summary

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

[Copilot]

Pull request overview

This PR applies a set of safety and observability fixes in reparentutil, primarily around replication-stop coordination, replica reparent concurrency, and troubleshooting logs during planned reparents.

Changes:

• Adds a precondition guard in stopReplicationAndBuildStatusMaps to prevent invalid “no tablets to act on” scenarios.
• Improves mutex safety in ERS replica handling by using defer to ensure unlock on early exits/panics.
• Enhances PRS logging and adds rationale around best-effort RefreshState behavior.

Reviewed changes

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

File	Description
go/vt/vtctl/reparentutil/replication.go	Adds guard logic around the computed goroutine count when stopping replication across tablets.
go/vt/vtctl/reparentutil/planned_reparenter.go	Adds rationale for best-effort RefreshState and includes tablet alias in warning logs.
go/vt/vtctl/reparentutil/emergency_reparenter.go	Uses defer for mutex unlock and clarifies background goroutine / cancellation behavior.

[Copilot]

Pull request overview

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.38%. Comparing base (70c7a72) to head (1ac5b6b).
⚠️ Report is 188 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main   #19888       +/-   ##
===========================================
+ Coverage   69.67%   91.38%   +21.70%     
===========================================
  Files        1614        9     -1605     
  Lines      216793     1311   -215482     
===========================================
- Hits       151044     1198   -149846     
+ Misses      65749      113    -65636     

Flag	Coverage Δ
partial	91.38% <100.00%> (?)

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.

[mattlord]

The changes seem OK (not sure why this warrants a PR by itself as there is an org/project cost for PRs), but I don't like having PRs w/o an issue and w/o any tests which demonstrate the problem and the fix.

The PR changes the goroutine counting logic and adds a new early FAILED_PRECONDITION branch in replication.go (line 371), but there’s no corresponding coverage added in replication_test.go (line 272). In particular, I’d want two explicit test cases:

1. IgnoreReplicas contains a stale alias that is not present in tabletMap
2. All real tablets are filtered out by IgnoreReplicas, so the new precondition path fires

That’s the exact behavior this PR seems to be fixing, and right now it’s still unprotected by regression tests.

[arthurschreiber]

The changes LGTM, but as @mattlord pointed out, tests that cover this would be nice.

[mattlord]

Thanks, @timvaillancourt !

[nickvanw]

LGTM. The counting-in-loop fix is the right shape — the previous len-based subtraction could silently mismatch reality when ignoredTablets had stale aliases, leaking goroutines on the unbuffered errChan. Tests cover the two scenarios that matter.