Handle protected manifests

[Michal-Leszczynski]

This PR adds the notion of protected manifest - a manifest which has either event based hold set or is under not expired retention period.
Such manifests shouldn't be skipped during purge and explicit removal (sctool backup delete) of such manifests should return an error.
This is especially needed for event based hold backup, where we expect backup retention policy (--retention, --retention-days) to drift from the actual manifest protection, as:

• holds are released only during the next backup task execution
• we can't set exact retention date after hold is released - it simply starts the timer with the default bucket retention policy as soon as the hold is removed

Having said that, that's also useful for other WORM backup approaches where backup task retention policy has changed in between backup task executions.

Fixes https://scylladb.atlassian.net/browse/CLOUD-3212

[Copilot]

Pull request overview

Note

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Adds support for “protected manifests” by detecting retention-lock/event-based-hold protection on remote manifest objects and preventing purge/snapshot deletion from removing protected snapshots; also introduces event-based-hold reconciliation (batched) during backup.

Changes:

• Add remote manifest listing that includes retention / event-based-hold metadata and use it to block purge & DeleteSnapshot for protected tags.
• Implement eventBasedHoldHandler to reconcile event-based holds in streaming/batched fashion, and integrate it into deduplication + manifest/schema handling.
• Update unit/integration tests to cover protected-tag behavior and event-based-hold flows.

Reviewed changes

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

Show a summary per file

File	Description
pkg/service/backup/worker_retention_lock_test.go	Adds unit tests for eventBasedHoldHandler; switches test package; updates retention-lock-until call site.
pkg/service/backup/worker_retention_lock.go	Adds event-based-hold batching/worker, integrates hold application for schema/manifests, refactors waiting logic.
pkg/service/backup/worker_purge.go	Uses remote manifest metadata to skip purging protected snapshot tags.
pkg/service/backup/worker_deduplicate.go	Reconciles event-based holds while listing remote sstables/manifests; blocks incompatible “versioned sstable” scenario for event-based-hold.
pkg/service/backup/worker.go	Persists RetentionLockMode on worker tools for downstream stages.
pkg/service/backup/service_retention_lock_integration_test.go	Adds integration coverage ensuring protected snapshots aren’t purged/deleted until protection is cleared.
pkg/service/backup/service.go	Prevents DeleteSnapshot on protected tags; uses shared manifest listing across hosts/locations.
pkg/service/backup/purger_test.go	Updates stale-tag logic tests; adds tests for protected-tag detection.
pkg/service/backup/purger.go	Splits “stale tags” from “oldest kept tag” calculation.
pkg/service/backup/list_test.go	Updates list tests to use remote-manifest listing wrapper and map back to manifest infos.
pkg/service/backup/list.go	Introduces remoteManifestInfo, protected-tag detection, remote listing with retention/hold metadata, and grouping by location.
pkg/service/backup/event_based_hold_interceptor_integration_test.go	Adds integration test for event-based-hold backup behavior and metadata assertions.
pkg/scyllaclient/client_rclone.go	Adds RetentionLockEventBasedHold mode and updates parameter mapping/check-permissions + retention-lock APIs accordingly.

---

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

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (2)

pkg/service/backup/list_test.go:28

• The local variable name error shadows Go’s predeclared error identifier, which reduces readability and can confuse tooling. Rename it to err (and update the subsequent if/t.Fatal accordingly).

		remoteManifests, error := listRemoteManifests(ctx, client, scyllaclienttest.TestHost,

pkg/service/backup/list_test.go:34

• The local variable name error shadows Go’s predeclared error identifier, which reduces readability and can confuse tooling. Rename it to err (and update the subsequent if/t.Fatal accordingly).

		if error != nil {
			t.Fatal("listManifests() error", error)
		}

[karol-kokoszka]

Just two nits.
Thanks ! 👍

[Michal-Leszczynski]

v2:

• added correct snapshot tag in test failure - squashed into b2b73bc (comment)
• decoupled backup and scyllaclient retention modes to avoid type overloading and confusion - b6a65be
• check retention mode when checking manifest protection - 208023e (comment)

[Copilot]

Pull request overview

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

Suppressed comments (4)

pkg/service/backup/service_retention_lock_integration_test.go:516

• As above, checking only err != nil lets each iteration pass for an unrelated failure and does not verify the new explicit-deletion guard. Check that the error identifies protected snapshots.

				if err := h.service.DeleteSnapshot(ctx, h.ClusterID, []backupspec.Location{location}, []string{tag}); err == nil {
					t.Fatalf("Expected protected snapshot %s deletion to fail", tag)
				}

pkg/service/backup/service_retention_lock_integration_test.go:499

• This assertion accepts any error, so the test can pass on an unrelated listing/purge/backend failure without proving that DeleteSnapshot rejected the protected manifest. Assert that the returned error is the new protected-snapshot error.

This issue also appears on line 514 of the same file.

			if err := h.service.DeleteSnapshot(ctx, h.ClusterID, []backupspec.Location{location}, []string{tagA}); err == nil {
				t.Fatalf("Expected protected snapshot %s deletion to fail", tagA)
			}

pkg/service/backup/worker_purge.go:30

• This introduces user-visible purge behavior, but the retention-lock documentation does not explain that stale snapshot tags are skipped while any manifest remains protected. Document this behavior, including that the snapshot is reconsidered after protection expires or is cleared.

	protected := protectedTags(remoteManifests).List()
	if len(protected) > 0 {
		w.Logger.Info(ctx, "Skipping tags with manifests protected by retention lock or event based hold", "tags", protected)
	}
	tags.Remove(protected...)

pkg/service/backup/service.go:1276

• The new sctool backup delete failure mode is user-facing but is not documented. Add the protected-manifest rejection (and the all-or-nothing result when a request includes a protected tag) to the retention-lock documentation.

	protected := protectedTags(remoteManifests)
	if protectedRemoved := strset.Intersection(protected, tags); !protectedRemoved.IsEmpty() {
		return errors.Errorf("snapshots protected by retention lock or event based hold cannot be deleted: %v", protectedRemoved.List())

[Michal-Leszczynski]

@karol-kokoszka does the refactor look good to you?