Put MySQL replicas in crash safe state before shutdown

[mattlord]

Description

When gracefully shutting down a REPLICA/RDONLY MySQL instance, we now proactively place it in a crash-safe state first: full commit durability is restored (innodb_flush_log_at_trx_commit=1, sync_binlog=1, sync_relay_log=1), the already-written InnoDB redo, binary log, and relay log tails are flushed to be durable, and the replication threads are stopped so the multi-threaded applier drains to a clean, gap-free point.

These settings are often relaxed on replicas — for example to speed up a catch-up after a restore — and if a shutdown is then interrupted (e.g. a SIGKILL after a hung shutdown, or a k8s volume detach) the replica can lose acknowledged writes or re-apply transactions on restart. We hit this in production as duplicate key errors that broke replication until manual repair.

The preparation is best effort and bounded, so it can never block or fail the shutdown itself. If the shutdown fails while mysqld keeps running, the previous replication and durability state is restored so the replica isn't left with replication stopped. Concurrent and retried shutdown attempts are serialized and honor caller cancellation.

Related Issue(s)

Fixes: #20688
Fixes: #19625

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

I worked with Claude and Fable 5 on the work and Codex and GPT-5.6 on the reviews.

[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

Adds crash-safe replica preparation before MySQL shutdown.

Changes:

• Makes relay logs durable and stops replication threads.
• Adds bounded shutdown preparation and error reporting.
• Adds unit and end-to-end coverage.

Reviewed changes

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

File	Description
go/vt/mysqlctl/replication.go	Implements replica shutdown preparation.
go/vt/mysqlctl/replication_test.go	Tests preparation behavior and failures.
go/vt/mysqlctl/mysqld.go	Integrates preparation into shutdown.
go/test/endtoend/mysqlctl/crashsafeshutdown/crash_safe_shutdown_test.go	Verifies the shutdown durability fence.

[Copilot]

Pull request overview

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

[codecov]

Codecov Report

❌ Patch coverage is 87.63066% with 71 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.35%. Comparing base (70c7a72) to head (452fb21).
⚠️ Report is 473 commits behind head on main.

Files with missing lines	Patch %	Lines
go/vt/mysqlctl/mysqld.go	83.93%	31 Missing ⚠️
go/vt/mysqlctl/shutdown_flock.go	81.48%	15 Missing ⚠️
go/vt/mysqlctl/replication.go	94.29%	13 Missing ⚠️
go/vt/mysqlctl/query.go	94.91%	3 Missing ⚠️
go/mysql/flavor_filepos.go	0.00%	2 Missing ⚠️
go/mysql/flavor_mariadb.go	0.00%	2 Missing ⚠️
go/mysql/flavor_mysql_legacy.go	0.00%	2 Missing ⚠️
go/mysql/flavor_mysqlgr.go	0.00%	2 Missing ⚠️
go/cmd/mysqlctld/cli/mysqlctld.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main   #20599       +/-   ##
===========================================
- Coverage   69.67%   68.35%    -1.33%     
===========================================
  Files        1614      222     -1392     
  Lines      216793    32803   -183990     
===========================================
- Hits       151044    22421   -128623     
+ Misses      65749    10382    -55367     

Flag	Coverage Δ
partial	68.35% <87.63%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 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.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

[chatgpt-codex-connector]

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Repo admins can enable using credits for code reviews in their settings.

[timvaillancourt]

Really nicely done — the defensiveness and test coverage here are excellent. Two best-effort edge cases I wanted to check on before approving:

1. The durability fence runs as one fail-fast batch (replication.go:280), so if FLUSH ... BINARY LOGS fails (e.g. a replica with binlog off, or any transient flush error) it returns before the FLUSH ... RELAY LOGS and the receiver/applier stops — which is exactly the relay-durability + drain the PR is closing the gap on. Worth running the relay flush + thread stops independently of the binary-log flush (or ordering them first) so a benign earlier failure can't skip them?

2. restoreReplicaAfterFailedShutdown (replication.go:431) gives up permanently if the initial GetDbaConnection fails (or an in-loop status read errors), even though it has the generous 10-min budget. If mysqld is briefly unreachable right after a failed shutdown, the restore never runs and the replica is left with replication stopped. Intentional (treating connect-failure as "mysqld is exiting anyway"), or should the connect/status reads retry within the budget like START does?

Also tiny: the cmd != "" guard skips GR but not the filePos flavor's "unsupported", so file:pos replicas log two failed-stop warnings each shutdown.

[mattlord]

Thanks @timvaillancourt — all three were real issues, and they're addressed in 1fa9229:

1. Fence fail-fast: the fence statements now run independently — a failure in any one of them (e.g. a binary log rotation failing on a full disk) no longer skips the relay-log flush or the thread stops. Failures are collected and surfaced together once everything has been attempted. The fence-failure cases in TestPrepareReplicaForShutdown now pin that everything downstream still runs.

2. Restore giving up on connect/status failures: not intentional — it retries now. The restore is a single reconcile loop: failed connects retry on the poll interval, and a failed status read drops the (possibly broken) connection, reconnects, and retries within the 10-minute budget. One deliberate bound: if mysqld stays continuously unreachable for a minute (replicaRestoreConnectTimeout, reset by any successful connect), we treat it as exiting after all and give up — otherwise a mysqld that's really gone would hold Close()'s bounded wait on pending restores for the full 10 minutes (e.g. a failed mysqlctl shutdown would hang the CLI on exit). Covered by TestRestoreRetriesConnectUntilMysqldReturns, TestRestoreBoundsConnectRetries, and TestRestoreRetriesStatusRead.

3. filePos "unsupported": the guards now go through a replicationThreadCommandAvailable() helper that skips both GR's "" and filePos's "unsupported" — in the preparation's stops and in the restore's start and cycle-stop. This turned out to be worse than the two warnings you spotted: a file:pos replica whose restore needed a thread start would have retried the literal query unsupported for the whole restore budget. Covered by TestReplicaShutdownSkipsUnavailableThreadCommands.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

go/vt/mysqlctl/mysqld.go:985

• shutdownErr does not prove that shutdown was never initiated. executeShutdown can return the caller deadline from waitForMysqldExit after mysqladmin already delivered SHUTDOWN (it explicitly recognizes this at lines 1087-1106). This branch then restores relaxed durability and restarts replication while mysqld may still be shutting down. If that hung shutdown is subsequently killed—the failure mode this fence is intended to protect—the fence may have been removed immediately before the crash. Preserve the fenced state when shutdown was accepted/in progress, and restore only when it is known that mysqld will remain running.

	if replicaState != nil {
		mysqld.armReplicaRestore(ctx, replicaState, nil)

[timvaillancourt]

Thanks — verified all three fixes from my last round and they hold up (the independent fence statements, the reconcile-based restore, the replicationThreadCommandAvailable guard). Another pass over the restore's failure paths found two more, both confirmed by running:

1. A takeover that can't reconnect drops restore ownership: prepareReplicaForShutdown cancels the previous restoration and inherits its state, but a failed DBA connect returns nil before that state is retained; if the shutdown then also fails, no replacement restore is armed and the replica stays fenced (1/1/1, threads stopped) until someone notices. Observed with a scratch test: pending restores=0 after the failed takeover

2. The restore loop doesn't recognize mysql.ErrNotReplica, so if the server stops being a replica mid-restore (promotion, RESET REPLICA ALL) it retries SHOW REPLICA STATUS for the full 10-minute budget and Close hangs with it — ~200 retries in 2s in a scratch test. The preparation handles this case at replication.go:249; the restore needs the same check (and errors.Is rather than == while there)

Smaller: settingsRestored is set true even when the SET GLOBALs failed, so a transient error during the settings restore is never retried on the reconnected loop, unlike the thread restarts. Fail direction is safe (the fenced values are stricter), so maybe just a comment if the asymmetry is intentional

[Copilot]

Pull request overview

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

[mattlord]

Thanks @timvaillancourt — all three were real, and they're addressed in ef05cbb:

1. Takeover dropping restore ownership: prepareReplicaForShutdown now publishes and returns the inherited state before anything that can fail, including the DBA connect. The caller cancelled the previous restoration when it inherited, so the preparation owns that state from the moment it's called and must hand it back even when it never reaches mysqld — a subsequently failed shutdown then arms a replacement restoration as usual. Covered directly (TestPrepareKeepsInheritedStateOnConnectFailure) and end-to-end with your scenario (TestTakeoverConnectFailureKeepsRestoreOwnership: pending restore, takeover while mysqld refuses connections, second failed shutdown, then the replacement restoration converges once mysqld is reachable again).

2. ErrNotReplica mid-restore: the restoration's status-read path now ends with a clear log when the server stops being a replica (promotion, RESET REPLICA ALL) instead of retrying for the whole budget. TestRestoreStopsWhenNoLongerReplica pins exactly one status read — the red run reproduced your ~200-retries-in-2s measurement. Also switched the preparation's check to errors.Is while there.

3. Settings asymmetry: went with the fix rather than a comment — settingsRestored is only set once all three SETs succeed in a pass, so a transient failure is retried like the thread restarts are (TestRestoreRetriesSettingsRestore, and the existing settings-failure case now expects the retry on the convergence pass).

Copilot then caught a follow-on in 3: with no threads to reconcile, a settings retry could spin on a broken connection (the status probe that resets one is never reached on that path) — fixed in f01f74c by reconnecting there too.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

go/vt/mysqlctl/replication.go:607

• This completion condition ignores settingsRestored. If a durability SET fails while the replication threads already report their desired state (for example, preparation timed out before reaching the stops), the function returns here and never performs the documented retry, leaving the live replica permanently on the fence's forced settings. Require settingsRestored before declaring convergence.

		if !needReceiver && !needApplier && !pendingReceiver && !pendingApplier && !cycleReceiver {

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

Suppressed comments (1)

go/vt/mysqlctl/replication.go:544

• These SETs run before checking whether the server is still a replica. If a failed-shutdown restoration races a promotion/RESET REPLICA ALL, they can apply the replica's old relaxed values (for example sync_binlog=0 and innodb_flush_log_at_trx_commit=2) to the new primary; the later ErrNotReplica branch then returns with unsafe primary durability. Check replication status before restoring these values, and retain the full-durability fence when the server is no longer a replica.

			for _, query := range []string{
				"SET GLOBAL innodb_flush_log_at_trx_commit = " + state.flushLogAtTrxCommit,
				"SET GLOBAL sync_binlog = " + state.syncBinlog,
				"SET GLOBAL sync_relay_log = " + state.syncRelayLog,
			} {

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

Suppressed comments (1)

go/vt/mysqlctl/shutdown_flock.go:99

• The flock is retained until Mysqld.Close, even after a shutdown attempt and any restoration have completed. This makes the lock effectively permanent for long-lived owners: vttablet keeps its Mysqld until process exit (go/cmd/vttablet/cli/cli.go:152-154), while backup shuts down and then restarts that same instance (go/vt/mysqlctl/builtinbackupengine.go:600,614). After the first such cycle, a separate mysqlctl/Mysqld process can no longer shut down the restarted server; it waits on this stale flock until its context expires. Retain the lock while an attempt or pending restore is active, but release it once that work reaches idle rather than only on object destruction.

			mysqld.shutdownFlockMu.Lock()
			mysqld.shutdownFlock = f
			mysqld.shutdownFlockMu.Unlock()

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

Suppressed comments (1)

go/vt/mysqlctl/mysqld.go:773

• Lock contention is not bounded by preparationBudget; it waits only on the caller context. Some existing callers pass context.Background() (for example go/vt/mysqlctl/backup.go:481), while another process can retain this flock for the full 10-minute restore or longer while waiting for an unresolved preparation. Consequently this best-effort feature can delay the underlying shutdown far beyond shutdownTimeout, including past a pod termination grace period and into the SIGKILL failure mode this PR is intended to prevent. Bound cross-process waiting independently of caller context while preserving safe serialization or an explicit best-effort fallback.

	preparationBudget := replicaShutdownPreparationBudget(shutdownTimeout)
	if preparationBudget > 0 {
		releaseFlock, err := mysqld.acquireShutdownFlock(ctx, cnf)
		if err != nil {
			return err