fix(core): identify Kopia snapshots by a stable identity in verify and delete

AGENTS.md

@@ -198,8 +198,8 @@ confirm after that warning.
   canonical case: `kopia snapshot pin` rewrites the snapshot manifest to a *new*
   ID and deletes the old one, so without a refresh the server keeps serving the
   now-deleted ID for a backup that still exists, and a later client
-  `klio backup delete` deletes the wrong ID — leaving the real backup (and its
-  WALs) pinned forever.
+  `klio backup delete` asks Kopia to delete an ID that no longer matches
+  anything: the command fails and the real backup (and its WALs) stay pinned.
 - A direct write that only **deletes** snapshots (the tier1/tier2 retention
   apply) does **not** need a refresh: it removes IDs the server may still list,
   but it never rewrites a live backup's ID, and WAL retention is recomputed from
@@ -212,6 +212,36 @@ Do not introduce direct-write paths anywhere else. If, after warning the user, a
 new direct write is genuinely unavoidable, it must be paired with a server
 refresh of the affected tier.
 
+### Snapshot identity: manifest ID vs root object ID
+
+A snapshot's **manifest ID is not a stable identity**. `kopia snapshot pin`
+(the tier1 unpin above) rewrites a snapshot's manifest under a new ID and
+deletes the old one, so any code that lists snapshots and then acts on them a
+moment later can be holding an ID that no longer exists. Pick the identity by
+what the operation does:
+
+- **Reads that must survive a concurrent rewrite** use the root object ID
+  (`Manifest.RootEntry.ObjID`), which the rewrite leaves untouched. Backup
+  verification (`core/internal/client/klioclient/kopia/verify.go`) does this.
+  Root objects come in two kinds and take different flags: `--directory-id`
+  for the pgdata and metadata snapshots, `--file-id` for the control data
+  file, which is snapshotted on its own. Passing a file root to
+  `--directory-id` makes Kopia parse file content as a directory listing and
+  report healthy data as corrupt.
+- **Deletions must NOT use the root object ID.** Unchanged content dedupes to
+  the same root across backups (two backups of an idle tablespace share one), and
+  `kopia snapshot delete` removes *every* snapshot matching the ID it is given,
+  so deleting one backup by root ID can take another backup's snapshot with it.
+  Delete by manifest ID, and on failure re-list and retry so a concurrent
+  rewrite is picked up (`DeleteBackup` in the same package).
+- **The tier1 unpin is a write, not a read, and knowingly accepts the same
+  collision as delete.** The consumer's `getPinnedSnapshots`/`maintainTier2`
+  (`core/internal/consumer/backup.go`) also targets the root object ID, so a
+  root shared with another backup gets unpinned too. This is tolerated only
+  because the step is best-effort and the affected snapshot would be unpinned
+  anyway on the next tier2 migration — it is not a safe pattern to copy for
+  anything that isn't equally tolerant of that collision.
+
 ### Dagger caching issues
 
 When running e2e tests, Dagger may cache Helm repo indexes. If a new version of

core/internal/client/klioclient/kopia/delete.go

@@ -23,46 +23,140 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"time"
 
 	"github.com/cloudnative-pg/machinery/pkg/log"
 
 	"github.com/cloudnative-pg/klio/core/internal/client/klioclient"
+	"github.com/cloudnative-pg/klio/core/internal/kopia"
 )
 
 // ErrBackupNotFound is returned when attempting to delete a backup that does not exist.
 var ErrBackupNotFound = errors.New("backup not found")
 
+// deleteBackupAttempts bounds how many times DeleteBackup re-resolves the
+// snapshots of a backup before giving up.
+const deleteBackupAttempts = 3
+
+// deleteBackupRetryDelay is the wait between DeleteBackup attempts, giving a
+// concurrent "kopia snapshot pin" rewrite time to settle before the next
+// attempt re-resolves snapshot IDs. Tests override this to keep the suite
+// fast.
+//
+//nolint:gochecknoglobals
+var deleteBackupRetryDelay = 200 * time.Millisecond
+
+// snapshotStore is the subset of the Kopia client that DeleteBackup needs.
+type snapshotStore interface {
+	ListSnapshots(ctx context.Context, tags map[string]string, logFn kopia.LogFunc) ([]kopia.Manifest, error)
+	DeleteSnapshot(ctx context.Context, id string) error
+}
+
 // DeleteBackup removes all snapshots associated with the backup with the provided name.
 func (s *Connection) DeleteBackup(ctx context.Context, hostname string, name string) error {
-	contextLogger := log.FromContext(ctx)
+	return deleteBackupSnapshots(ctx, s.kopia, hostname, name)
+}
 
-	// List all snapshots for this backup (all content types)
-	entries, err := s.kopia.ListSnapshots(ctx, map[string]string{
-		klioclient.BackupNameTagName: name,
-	}, contextLogger.Debug)
-	if err != nil {
-		return fmt.Errorf("while listing snapshots: %w", err)
-	}
+// deleteBackupSnapshots removes every snapshot of a backup on the given host.
+//
+// Snapshots are deleted by manifest ID, which post-backup maintenance can
+// rewrite underneath us: "kopia snapshot pin" replaces a snapshot's manifest
+// with a new ID, and deleting a replaced ID matches nothing and fails, leaving
+// the backup in place. Deleting by root object ID is not an option, because
+// unchanged content dedupes to the same root across backups and Kopia deletes
+// every snapshot matching the ID it is given. Instead, resolve the snapshots
+// again and retry: each attempt lists the current IDs, and snapshots deleted by
+// an earlier attempt are simply no longer listed.
+func deleteBackupSnapshots(ctx context.Context, store snapshotStore, hostname, name string) error {
+	contextLogger := log.FromContext(ctx)
 
 	var deleted int
-	for _, entry := range entries {
-		if entry.Source.Host == hostname {
-			contextLogger.Info("DeleteBackup: deleting snapshot", "snapshotID", entry.ID)
-			if deleteErr := s.kopia.DeleteSnapshot(ctx, entry.ID); deleteErr != nil {
-				err = errors.Join(err, deleteErr)
-			} else {
-				deleted++
+
+	var lastErr error
+
+	for attempt := 1; attempt <= deleteBackupAttempts; attempt++ {
+		// List all snapshots for this backup (all content types)
+		entries, err := store.ListSnapshots(ctx, map[string]string{
+			klioclient.BackupNameTagName: name,
+		}, contextLogger.Debug)
+		if err != nil {
+			return fmt.Errorf("while listing snapshots: %w", err)
+		}
+
+		pending, deletedNow, attemptErr := deleteHostSnapshots(ctx, store, contextLogger, hostname, entries)
+		deleted += deletedNow
+
+		// Nothing left to delete: either we removed everything, or the backup
+		// was not there to begin with.
+		if pending == 0 {
+			if deleted > 0 {
+				return nil
 			}
+
+			return fmt.Errorf("%w: %s", ErrBackupNotFound, name)
+		}
+
+		if attemptErr == nil {
+			return nil
 		}
-	}
 
-	if err != nil {
-		return err
+		lastErr = attemptErr
+
+		contextLogger.Info("DeleteBackup: retrying with freshly resolved snapshot IDs",
+			"backupName", name, "attempt", attempt, "error", attemptErr)
+
+		if attempt < deleteBackupAttempts {
+			if waitErr := waitBeforeDeleteRetry(ctx); waitErr != nil {
+				return waitErr
+			}
+		}
 	}
 
-	if deleted == 0 {
-		return fmt.Errorf("%w: %s", ErrBackupNotFound, name)
+	return lastErr
+}
+
+// deleteHostSnapshots deletes every entry belonging to hostname, returning how
+// many of them matched the host (pending), how many were deleted, and the
+// deletion errors joined together.
+func deleteHostSnapshots(
+	ctx context.Context,
+	store snapshotStore,
+	contextLogger log.Logger,
+	hostname string,
+	entries []kopia.Manifest,
+) (int, int, error) {
+	var pending, deleted int
+
+	var err error
+
+	for _, entry := range entries {
+		if entry.Source.Host != hostname {
+			continue
+		}
+
+		pending++
+
+		contextLogger.Info("DeleteBackup: deleting snapshot", "snapshotID", entry.ID)
+		if deleteErr := store.DeleteSnapshot(ctx, entry.ID); deleteErr != nil {
+			err = errors.Join(err, deleteErr)
+
+			continue
+		}
+
+		deleted++
 	}
 
-	return nil
+	return pending, deleted, err
+}
+
+// waitBeforeDeleteRetry pauses deleteBackupRetryDelay before the next
+// DeleteBackup attempt, returning early with ctx's error if it is cancelled
+// first.
+func waitBeforeDeleteRetry(ctx context.Context) error {
+	select {
+	case <-ctx.Done():
+		return ctx.Err()
+	case <-time.After(deleteBackupRetryDelay):
+		return nil
+	}
 }

core/internal/client/klioclient/kopia/delete_test.go

@@ -0,0 +1,154 @@
+/*
+Copyright © contributors to CloudNativePG, established as
+CloudNativePG a Series of LF Projects, LLC.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+
+SPDX-License-Identifier: Apache-2.0
+*/
+
+package kopia
+
+import (
+	"context"
+	"errors"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/cloudnative-pg/klio/core/internal/kopia"
+)
+
+var errDeleteFailed = errors.New("no snapshots matched")
+
+// fakeSnapshotStore serves a scripted sequence of snapshot listings and records
+// the IDs it was asked to delete. Listings are consumed one per call so a test
+// can model the manifest IDs changing between attempts.
+type fakeSnapshotStore struct {
+	listings  [][]kopia.Manifest
+	listCalls int
+
+	// failDeleteOf holds the IDs whose deletion fails.
+	failDeleteOf map[string]bool
+
+	deleted []string
+	listErr error
+}
+
+func (f *fakeSnapshotStore) ListSnapshots(
+	_ context.Context,
+	_ map[string]string,
+	_ kopia.LogFunc,
+) ([]kopia.Manifest, error) {
+	if f.listErr != nil {
+		return nil, f.listErr
+	}
+
+	f.listCalls++
+
+	// The last scripted listing is reused for any further attempt.
+	idx := min(f.listCalls-1, len(f.listings)-1)
+
+	return f.listings[idx], nil
+}
+
+func (f *fakeSnapshotStore) DeleteSnapshot(_ context.Context, id string) error {
+	if f.failDeleteOf[id] {
+		return errDeleteFailed
+	}
+
+	f.deleted = append(f.deleted, id)
+
+	return nil
+}
+
+func manifest(id, host string) kopia.Manifest {
+	return kopia.Manifest{ID: id, Source: kopia.SourceInfo{Host: host}}
+}
+
+func TestDeleteBackupSnapshots(t *testing.T) {
+	ctx := context.Background()
+
+	oldDelay := deleteBackupRetryDelay
+	deleteBackupRetryDelay = 0
+	t.Cleanup(func() { deleteBackupRetryDelay = oldDelay })
+
+	t.Run("deletes every snapshot of the backup on the host", func(t *testing.T) {
+		store := &fakeSnapshotStore{
+			listings: [][]kopia.Manifest{
+				{manifest("a", "cluster"), manifest("b", "cluster")},
+				{},
+			},
+		}
+
+		require.NoError(t, deleteBackupSnapshots(ctx, store, "cluster", "backup-1"))
+		assert.Equal(t, []string{"a", "b"}, store.deleted)
+		assert.Equal(t, 1, store.listCalls)
+	})
+
+	t.Run("ignores snapshots belonging to another host", func(t *testing.T) {
+		store := &fakeSnapshotStore{
+			listings: [][]kopia.Manifest{{manifest("a", "other-cluster")}},
+		}
+
+		err := deleteBackupSnapshots(ctx, store, "cluster", "backup-1")
+
+		require.ErrorIs(t, err, ErrBackupNotFound)
+		assert.Empty(t, store.deleted)
+	})
+
+	t.Run("returns not found when the backup has no snapshots", func(t *testing.T) {
+		store := &fakeSnapshotStore{listings: [][]kopia.Manifest{{}}}
+
+		require.ErrorIs(t, deleteBackupSnapshots(ctx, store, "cluster", "backup-1"), ErrBackupNotFound)
+	})
+
+	// A concurrent "kopia snapshot pin" rewrites a snapshot's manifest under a
+	// new ID, so deleting the ID we listed matches nothing. Re-resolving must
+	// pick up the new ID and finish the deletion.
+	t.Run("retries with the rewritten manifest ID", func(t *testing.T) {
+		store := &fakeSnapshotStore{
+			listings: [][]kopia.Manifest{
+				{manifest("a", "cluster"), manifest("stale", "cluster")},
+				{manifest("rewritten", "cluster")},
+				{},
+			},
+			failDeleteOf: map[string]bool{"stale": true},
+		}
+
+		require.NoError(t, deleteBackupSnapshots(ctx, store, "cluster", "backup-1"))
+		assert.Equal(t, []string{"a", "rewritten"}, store.deleted)
+		assert.Equal(t, 2, store.listCalls)
+	})
+
+	t.Run("gives up after the attempt budget and reports the failure", func(t *testing.T) {
+		store := &fakeSnapshotStore{
+			listings:     [][]kopia.Manifest{{manifest("stuck", "cluster")}},
+			failDeleteOf: map[string]bool{"stuck": true},
+		}
+
+		err := deleteBackupSnapshots(ctx, store, "cluster", "backup-1")
+
+		require.ErrorIs(t, err, errDeleteFailed)
+		assert.Equal(t, deleteBackupAttempts, store.listCalls)
+		assert.Empty(t, store.deleted)
+	})
+
+	t.Run("a listing failure is returned as is", func(t *testing.T) {
+		listErr := errors.New("connection refused")
+		store := &fakeSnapshotStore{listErr: listErr}
+
+		require.ErrorIs(t, deleteBackupSnapshots(ctx, store, "cluster", "backup-1"), listErr)
+	})
+}