mysqlctl: prevent closeBackupFiles from cancelling context on successful file close, protecting in-flight S3/Ceph uploads

[jdoupe]

Description

(description generated by Claude Code)

The Bug

The original backupFiles() function had an inline defer block that imposed a timeout on Close() calls
for backup files. That timeout watchdog was flawed in two ways:

1. It cancelled the context when the files closed successfully (too fast). The watchdog waited on
   addFilesCtx.Done() to know when to stop. But addFilesCtx is the same context used by a background upload
   goroutine (started by bh.AddFile()) to upload data to S3/Ceph. If Close() returns before the background
   upload drains, and the watchdog then calls cancel(), it aborts an otherwise-successful upload
   mid-flight.
2. It could deadlock on the addFilesCtx.Done() signal. The old code's select listened on
   addFilesCtx.Done() to know when to stop the watchdog. But cancelAddFiles was deferred after the inline
   defer — meaning if backupFiles succeeded, addFilesCtx would never be cancelled, so the watchdog would
   hang forever (or until its own timer expired).

The Fix

A new closeBackupFiles() function was extracted with a corrected design:

• A separate done channel signals when Close() has finished, not the addFilesCtx. This means successful
  closure never cancels the context.
• The watchdog timer only calls cancel() on a real timeout (when Close() actually hangs). On success, it
  simply stops the timer and returns, leaving the context alive for bh.Wait() / EndBackup() to drain the
  background upload.
• addFilesCtx is also explicitly cancelled early on errors in addStripeFiles, and the top-level defer on
  cancelAddFiles is now only called when there's an actual error — keeping the old "kill straggling
  uploads on failure" behavior intact.

Tests

Two new tests guard both directions:

• TestCloseBackupFilesDoesNotCancelContextOnSuccess — verifies that when files close quickly (success),
  the context is never cancelled, even after the watchdog timeout expires.
• TestCloseBackupFilesCancelsOnRealTimeout — verifies that when Close() genuinely hangs past the
  timeout, the watchdog logs the error and cancels the context so the stuck upload aborts instead of
  hanging forever.

Related Issue(s)

• Fixes: Bug Report: Xtrabackup S3 backups stop unexpectedly #20770

Maybe #16825 and #19853

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

This PR was generated primarily with Claude Code / Sonnet 5. Then reviewed here (as seen below) with Copilot. It was also reviewed with Claude Code / (local model) Qwen3.6-35B-A3B.

[Copilot]

🟢 Ready to approve

The focused lifecycle fix preserves failure behavior and includes regression coverage.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

Ensures the XtraBackup file-close watchdog stops immediately after uploads finish.

Changes:

• Extracts backup-file closing and watchdog cancellation into closeBackupFiles.
• Cancels the AddFile context after every close outcome.
• Adds regression coverage for prompt cancellation.

File summaries

File	Description
go/vt/mysqlctl/xtrabackupengine.go	Stops the close watchdog when file uploads finish.
go/vt/mysqlctl/xtrabackupengine_test.go	Tests successful close and prompt cancellation.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 0
• Review effort level: Medium

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

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

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a2cd5efd74

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[Copilot]

🟡 Not ready to approve

A watchdog scheduling race can still cancel successfully closed uploads.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Human review recommended

Context lifetime changes affect asynchronous uploads across multiple storage backends and warrant final human validation.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[jdoupe]

I feel like this would be helpful to backport to the v24 branch as well.

[Copilot]

🟡 Not ready to approve

Successful data uploads must be drained before cancellation ownership is lost and MANIFEST processing begins.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 965915d4cc

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[Copilot]

🟢 Ready to approve

The upload lifecycle fix is consistent across affected backends and covers both success and timeout paths.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[codecov]

Codecov Report

❌ Patch coverage is 75.00000% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 55.04%. Comparing base (70c7a72) to head (27a1121).
⚠️ Report is 473 commits behind head on main.

Files with missing lines	Patch %	Lines
go/vt/mysqlctl/xtrabackupengine.go	77.14%	8 Missing ⚠️
go/vt/mysqlctl/azblobbackupstorage/azblob.go	66.66%	3 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (70c7a72) and HEAD (27a1121). Click for more details.

HEAD has 1 upload less than BASE

Flag	BASE (70c7a72)	HEAD (27a1121)
	1	0

Additional details and impacted files

@@             Coverage Diff             @@
##             main   #20771       +/-   ##
===========================================
- Coverage   69.67%   55.04%   -14.63%     
===========================================
  Files        1614       46     -1568     
  Lines      216793     7505   -209288     
===========================================
- Hits       151044     4131   -146913     
+ Misses      65749     3374    -62375     

Flag	Coverage Δ
partial	55.04% <75.00%> (?)

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]

🟡 Human review recommended

Concurrent cancellation and upload lifecycle changes across cloud backends warrant final human review.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Human review recommended

Cloud-upload lifecycle and timeout concurrency changes warrant final human validation despite no confirmed blocking defect.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[mattlord]

In addition to removing the release summary notes, I think that there are a few minor things that we should address before merging:

1. Update AddFile's doc in backupstorage/interface.go. It still says the context "is valid for the duration of the writes, until the WriteCloser is closed" — this PR's whole premise is that async backends consume it until Wait() returns, and the engine now keeps it live exactly that long. Updating the sentence keeps the next engine author from reintroducing the bug the old wording invites.

2. Drop the redundant cancelAddFiles() on the addStripeFiles error path. finalErr is the named return, so the outer defer already runs cancelAddFiles(); bh.Wait() for that return. Harmless, but it reads as if the defer doesn't cover it.

3. In go/vt/mysqlctl/xtrabackupengine_test.go:252, I think we should use require.ErrorIs instead of assert.ErrorIs; testifylint is currently failing on that line.

4. Worth a note (or follow-up test): the drain-before-MANIFEST ordering itself is unpinned. The unit tests cover closeBackupFiles and mergeCancel nicely, but nothing proves backupFiles drains uploads and checks bh.Error() before the caller writes the MANIFEST. Understandable given backupFiles needs an xtrabackup binary — a fake-handle test of the defer, or just a comment/PR note, would keep that contract from silently regressing.

5. Optional nit: preserve the cancellation cause in mergeCancel. With context.WithCancelCause(parent) + cancel(context.Cause(other)), an upload aborted via the per-file context reports why instead of a bare context.Canceled.

Thanks, @jdoupe ! ❤️

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

[Copilot]

🟡 Human review recommended

The concurrency and cancellation changes span multiple cloud-storage backends and merit final human validation.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[jdoupe]

In addition to removing the release summary notes, I think that there are a few minor things that we should address before merging:

1. Update AddFile's doc in backupstorage/interface.go. It still says the context "is valid for the duration of the writes, until the WriteCloser is closed" — this PR's whole premise is that async backends consume it until Wait() returns, and the engine now keeps it live exactly that long. Updating the sentence keeps the next engine author from reintroducing the bug the old wording invites.
2. Drop the redundant cancelAddFiles() on the addStripeFiles error path. finalErr is the named return, so the outer defer already runs cancelAddFiles(); bh.Wait() for that return. Harmless, but it reads as if the defer doesn't cover it.
3. In go/vt/mysqlctl/xtrabackupengine_test.go:252, I think we should use require.ErrorIs instead of assert.ErrorIs; testifylint is currently failing on that line.
4. Worth a note (or follow-up test): the drain-before-MANIFEST ordering itself is unpinned. The unit tests cover closeBackupFiles and mergeCancel nicely, but nothing proves backupFiles drains uploads and checks bh.Error() before the caller writes the MANIFEST. Understandable given backupFiles needs an xtrabackup binary — a fake-handle test of the defer, or just a comment/PR note, would keep that contract from silently regressing.
5. Optional nit: preserve the cancellation cause in mergeCancel. With context.WithCancelCause(parent) + cancel(context.Cause(other)), an upload aborted via the per-file context reports why instead of a bare context.Canceled.

@mattlord - I believe I've addressed each of these (I'm pretty sure (3) was already done. 🤷 ). Thank you!

[mattlord]

LGTM! Just one small test cleanup thing remains. In go/vt/mysqlctl/xtrabackupengine_test.go:252-259, the new pinned comment is incorrect: this test never invokes a BackupHandle, bh.Error(), backupFiles, or the MANIFEST path. It only proves that the close watchdog cancels the context and that the closer returns that error. The block immediately below then says that the ordering is unpinned, so the two comments contradict each other and seem likely to confuse future readers. I think we should remove both comments, leaving the require.ErrorIs assertion, and track the optional MANIFEST-ordering test separately if desired.

Thanks, @jdoupe ! ❤️

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

[Copilot]

🔵 Human review recommended

The changes alter concurrency and cancellation semantics across multiple backup backends and warrant final human validation.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.