Fix backup-finalizer: do not set backup phase to Completed before PutBackupMetadata succeeds

[kaovilai]

Summary

Fixes #9645

• Moves backup.Status.Phase assignment from before PutBackupMetadata/PutBackupContents to after both succeed
• Uses backup.DeepCopy() to encode JSON with the final phase for object storage upload, while keeping the in-memory backup at Finalizing phase until uploads complete
• Adds unit tests covering PutBackupMetadata and PutBackupContents failure scenarios for both Finalizing and FinalizingPartiallyFailed phases

Caveats

• CompletionTimestamp: Now captured before upload but only committed to the API server after upload succeeds. On retry after a transient failure, a new timestamp is generated, so the completion time reflects when the upload finally succeeded rather than when finalization processing completed.
• Metrics: RegisterBackupSuccess/RegisterBackupPartialFailure are now recorded after uploads succeed, so they accurately reflect only fully persisted backups. Previously, metrics could report success even when the backup wasn't fully persisted.
• Object storage metadata: The uploaded JSON contains the final phase and completion timestamp via DeepCopy, so storage state is correct even before the API server is patched.

Test plan

• New test: TestBackupFinalizerReconcile_PutBackupMetadataFail — verifies phase stays Finalizing/FinalizingPartiallyFailed when PutBackupMetadata fails
• New test: TestBackupFinalizerReconcile_PutBackupContentsFail — verifies phase stays Finalizing when PutBackupContents fails
• Both tests assert CompletionTimestamp remains nil on failure
• Existing tests pass unchanged

🤖 Generated with Claude Code via Happy

[Copilot]

Pull request overview

Adjusts backup finalization so backups aren’t marked terminal (Completed/PartiallyFailed) in the API server until persistence to object storage has succeeded, preventing “completed but not persisted” states.

Changes:

• Defers setting backup.Status.Phase / CompletionTimestamp until after persistence calls succeed, while uploading a deep-copied “final” backup object to object storage.
• Moves metric registration to occur only after persistence succeeds.
• Adds unit tests for PutBackupMetadata and PutBackupContents failure paths.

Reviewed changes

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

File	Description
pkg/controller/backup_finalizer_controller.go	Defers terminal status + metrics until after uploads; uploads a deep-copied “final” backup JSON to object storage.
pkg/controller/backup_finalizer_controller_test.go	Adds unit tests for failure scenarios of metadata/content uploads and asserts phase/timestamps remain non-terminal on failure.
changelogs/unreleased/9646-kaovilai	Adds changelog entry describing the fix.

Comments suppressed due to low confidence (1)

pkg/controller/backup_finalizer_controller.go:1

• This uploads terminal backup metadata (Completed/PartiallyFailed + CompletionTimestamp) to object storage before PutBackupContents runs. If PutBackupContents fails, object storage can end up claiming the backup is terminal while final contents were not uploaded, which undermines the goal of only persisting terminal state after all persistence work succeeds. Consider making the persistence atomic from the reader’s perspective by either (a) uploading contents first, then uploading metadata with the final phase/timestamp, or (b) uploading metadata in the current (Finalizing*) phase first and only re-uploading metadata with the terminal phase after PutBackupContents succeeds.

/*

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

finalPhase has no default assignment and is used unconditionally. If backup.Status.Phase is not one of the two handled values in this switch on this code path, finalPhase will be the zero value and could be uploaded/patched as an empty phase. A concrete fix is to add an explicit default/guard (e.g., return early) or initialize finalPhase from backup.Status.Phase and only override it for the handled Finalizing* phases.

[Copilot]

This mock expectation hard-codes framework.BackupItemActionResolverV2{} as an exact argument match, which is brittle if the reconciler passes a non-zero resolver value. Prefer matching that parameter with mock.Anything or mock.MatchedBy(...) so the test only asserts what it actually cares about (that FinalizeBackup is called and returns nil).

Suggested change

	backupper.On("FinalizeBackup", mock.Anything, mock.Anything, mock.Anything, mock.Anything, framework.BackupItemActionResolverV2{}, mock.Anything, mock.Anything).Return(nil)
	backupper.On("FinalizeBackup", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil)

[Copilot]

The new reconciler logic computes a finalPhase for both Finalizing and FinalizingPartiallyFailed, but the PutBackupContents failure test only covers Finalizing. Add a table-driven variant (similar to TestBackupFinalizerReconcile_PutBackupMetadataFail) that also exercises the FinalizingPartiallyFailed starting phase and asserts the phase/timestamp remain unchanged on PutBackupContents failure.

[codecov]

Codecov Report

❌ Patch coverage is 95.23810% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 60.98%. Comparing base (a5391e1) to head (cd86689).

Files with missing lines	Patch %	Lines
pkg/controller/backup_finalizer_controller.go	95.23%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9646      +/-   ##
==========================================
+ Coverage   60.95%   60.98%   +0.02%     
==========================================
  Files         387      387              
  Lines       36647    36658      +11     
==========================================
+ Hits        22340    22355      +15     
+ Misses      12692    12690       -2     
+ Partials     1615     1613       -2     

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