From 0b6202cf59bb68c9f6ac1af0738e03b6b8c45b0e Mon Sep 17 00:00:00 2001 From: Gabriele Quaresima Date: Tue, 11 Aug 2026 11:33:41 +0200 Subject: [PATCH 1/5] fix(core): verify backups by root object ID Tier1 backup verification resolved a backup to its Kopia snapshot manifest IDs and passed them to "kopia snapshot verify". Post-relay tier2 maintenance unpins those same snapshots, and "kopia snapshot pin --remove" rewrites a snapshot manifest under a new ID and deletes the old one, so an ID resolved a moment earlier could already be gone by the time verification ran. Kopia then reported "found 0 of the N requested snapshot IDs to verify", and klio turned that into a corruption failure with exit code 65, failing an intact backup. This is the intermittent WALRetentionQueueAwareness e2e failure, and the same misreport is reachable outside tests. Resolve backups to the root object ID of each snapshot instead. Root object IDs are a stable identity that the pin rewrite leaves untouched, and the server side already identifies the snapshots it unpins the same way. Directory and file roots need different flags, and a backup mixes both: pgdata and metadata are directory snapshots, while the control data file 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, so the root entry type now decides the flag and an unknown type is an error rather than a guess. Also stop classifying an unresolved snapshot set as corruption. Such a result means nothing was verified, so it carries no evidence about repository integrity, and it is now returned as a retryable error. Errors about a missing object or blob keep failing as corruption. Assisted-by: Claude Signed-off-by: Gabriele Quaresima --- .../client/klioclient/kopia/verify.go | 126 +++++++++++++++--- .../client/klioclient/kopia/verify_test.go | 71 ++++++++++ core/internal/kopia/data.go | 15 +++ core/internal/kopia/write.go | 62 +++++++-- core/internal/kopia/write_test.go | 76 +++++++++++ 5 files changed, 323 insertions(+), 27 deletions(-) diff --git a/core/internal/client/klioclient/kopia/verify.go b/core/internal/client/klioclient/kopia/verify.go index 040de624..12022474 100644 --- a/core/internal/client/klioclient/kopia/verify.go +++ b/core/internal/client/klioclient/kopia/verify.go @@ -21,7 +21,9 @@ package kopia import ( "context" + "errors" "fmt" + "strings" "github.com/cloudnative-pg/machinery/pkg/log" @@ -29,6 +31,21 @@ import ( kopiaClient "github.com/cloudnative-pg/klio/core/internal/kopia" ) +var ( + // ErrNoSnapshotsForBackup is returned when no verifiable snapshot can be + // found for a backup name. + ErrNoSnapshotsForBackup = errors.New("no snapshots found for backup") + + // ErrSnapshotsUnresolved is returned when Kopia could not resolve the + // snapshots it was asked to verify, so no integrity check was performed. + // It is retryable and must not be reported as corruption. + ErrSnapshotsUnresolved = errors.New("backup verification could not resolve the requested snapshots") + + // ErrUnsupportedRootEntryType is returned when a snapshot's root entry is + // neither a directory nor a file, so it cannot be verified by object ID. + ErrUnsupportedRootEntryType = errors.New("unsupported snapshot root entry type") +) + // BackupVerificationError is returned when backup verification detects corruption. type BackupVerificationError struct { // Result contains the verification details from Kopia. @@ -80,7 +97,7 @@ func (s *Connection) verifyAllBackups(ctx context.Context, hostname string) erro // verify all snapshots for this hostname. contextLogger.Info("Verifying all backups for hostname", "hostname", hostname) - result, err := s.kopia.VerifySnapshots(ctx) + result, err := s.kopia.VerifySnapshots(ctx, kopiaClient.VerifySnapshotsOptions{}) if err != nil { return classifyVerifyError(ctx, result, err) } @@ -90,22 +107,24 @@ func (s *Connection) verifyAllBackups(ctx context.Context, hostname string) erro return nil } -// verifySpecificBackups verifies the specified backups by resolving their snapshot IDs. +// verifySpecificBackups verifies the specified backups by resolving them to the +// root object IDs of their snapshots. func (s *Connection) verifySpecificBackups(ctx context.Context, hostname string, backupNames []string) error { contextLogger := log.FromContext(ctx) - var allSnapshotIDs []string + var verifyOpts kopiaClient.VerifySnapshotsOptions for _, name := range backupNames { - ids, err := s.getSnapshotIDsForBackup(ctx, hostname, name) + opts, err := s.getRootObjectsForBackup(ctx, hostname, name) if err != nil { return fmt.Errorf("backup %q: %w", name, err) } - allSnapshotIDs = append(allSnapshotIDs, ids...) + verifyOpts.DirectoryIDs = append(verifyOpts.DirectoryIDs, opts.DirectoryIDs...) + verifyOpts.FileIDs = append(verifyOpts.FileIDs, opts.FileIDs...) } - contextLogger.Info("Verifying backups", "backupNames", backupNames, "snapshotCount", len(allSnapshotIDs)) + contextLogger.Info("Verifying backups", "backupNames", backupNames, "snapshotCount", verifyOpts.Len()) - result, err := s.kopia.VerifySnapshots(ctx, allSnapshotIDs...) + result, err := s.kopia.VerifySnapshots(ctx, verifyOpts) if err != nil { return classifyVerifyError(ctx, result, err) } @@ -115,37 +134,98 @@ func (s *Connection) verifySpecificBackups(ctx context.Context, hostname string, return nil } -// getSnapshotIDsForBackup resolves a backup name to its constituent Kopia snapshot IDs. -func (s *Connection) getSnapshotIDsForBackup(ctx context.Context, hostname, backupName string) ([]string, error) { +// getRootObjectsForBackup resolves a backup name to the root objects of its +// constituent Kopia snapshots, split by object kind. +// +// The root object ID is used rather than the snapshot manifest ID because it is +// a stable identity. Post-backup maintenance unpins the snapshots of a backup +// that reached tier2, and "kopia snapshot pin" rewrites the manifest under a +// new ID, so a manifest ID resolved here can already have been replaced by the +// time verification runs. The root object ID is untouched by that rewrite. +// +// A backup mixes both kinds: pgdata and metadata are directory snapshots, while +// the control data file is snapshotted on its own and so has a file root. +func (s *Connection) getRootObjectsForBackup( + ctx context.Context, + hostname, backupName string, +) (kopiaClient.VerifySnapshotsOptions, error) { contextLogger := log.FromContext(ctx) + var opts kopiaClient.VerifySnapshotsOptions + entries, err := s.kopia.ListSnapshots(ctx, map[string]string{ klioclient.BackupNameTagName: backupName, }, contextLogger.Debug) if err != nil { - return nil, err + return opts, err } - var ids []string for _, e := range entries { - if e.Source.Host == hostname { - ids = append(ids, e.ID) + if e.Source.Host != hostname { + continue + } + + // An incomplete snapshot has no root entry to verify. + if e.RootEntry == nil || e.RootEntry.ObjID == "" { + contextLogger.Info("Skipping snapshot without a root object ID", + "backupName", backupName, "snapshotID", e.ID) + + continue + } + + switch e.RootEntry.Type { + case kopiaClient.EntryTypeDirectory: + opts.DirectoryIDs = append(opts.DirectoryIDs, e.RootEntry.ObjID) + case kopiaClient.EntryTypeFile: + opts.FileIDs = append(opts.FileIDs, e.RootEntry.ObjID) + default: + // Verifying with the wrong flag reports a healthy object as + // corrupt, so refuse rather than guess. + return opts, fmt.Errorf("%w: snapshot %q has root entry type %q", + ErrUnsupportedRootEntryType, e.ID, e.RootEntry.Type) } } - if len(ids) == 0 { - return nil, fmt.Errorf("no snapshots found for backup %q", backupName) + if opts.IsEmpty() { + return opts, fmt.Errorf("%w: %q", ErrNoSnapshotsForBackup, backupName) } - return ids, nil + return opts, nil +} + +// unresolvedSnapshotMarker appears in a Kopia verify error when the snapshot +// manifests it was asked to verify could not be resolved, as in +// "found 0 of the 3 requested snapshot IDs to verify". Kopia emits it from the +// manifest lookup that precedes any object walk, so it means "nothing was +// verified", never "the data is damaged". Errors about a missing object or blob +// use different wording and are genuine corruption evidence. +const unresolvedSnapshotMarker = "requested snapshot IDs to verify" + +// isUnresolvedSnapshotSet reports whether every error in the result is Kopia +// failing to resolve the requested snapshots. Such a result carries no +// information about repository integrity: it is retryable, and treating it as +// corruption fails a backup that is in fact intact. +func isUnresolvedSnapshotSet(result kopiaClient.VerifyResult) bool { + if len(result.ErrorStrings) == 0 { + return false + } + + for _, e := range result.ErrorStrings { + if !strings.Contains(e, unresolvedSnapshotMarker) { + return false + } + } + + return true } // classifyVerifyError inspects the verify result to distinguish corruption -// (errorCount > 0) from infrastructure errors. +// (errorCount > 0 with integrity evidence) from errors that say nothing about +// the data, which the caller retries. func classifyVerifyError(ctx context.Context, result kopiaClient.VerifyResult, err error) error { contextLogger := log.FromContext(ctx) - if result.ErrorCount > 0 { + if result.ErrorCount > 0 && !isUnresolvedSnapshotSet(result) { contextLogger.Error(err, "Backup verification detected corruption", "errorCount", result.ErrorCount, "errors", result.ErrorStrings, @@ -157,5 +237,15 @@ func classifyVerifyError(ctx context.Context, result kopiaClient.VerifyResult, e } } + if result.ErrorCount > 0 { + contextLogger.Info("Backup verification could not resolve the requested snapshots, "+ + "nothing was verified; reporting a retryable error rather than corruption", + "errorCount", result.ErrorCount, + "errors", result.ErrorStrings, + ) + + return fmt.Errorf("%w: %w", ErrSnapshotsUnresolved, err) + } + return fmt.Errorf("backup verification encountered an infrastructure error: %w", err) } diff --git a/core/internal/client/klioclient/kopia/verify_test.go b/core/internal/client/klioclient/kopia/verify_test.go index ab6d8064..a17523e0 100644 --- a/core/internal/client/klioclient/kopia/verify_test.go +++ b/core/internal/client/klioclient/kopia/verify_test.go @@ -99,4 +99,75 @@ func TestClassifyVerifyError(t *testing.T) { require.ErrorIs(t, err, infraErr) require.NotErrorAs(t, err, &backupErr) }) + + // An unresolved snapshot set means Kopia never walked any object, so the + // result carries no integrity evidence: reporting corruption there fails an + // intact backup. This is the CNP-9006 failure mode. + t.Run("unresolved snapshot set is retryable, not corruption", func(t *testing.T) { + verifyErr := errors.New("while verifying Kopia snapshots: command failed: exit status 1") + result := kopiaClient.VerifyResult{ + ErrorCount: 1, + ErrorStrings: []string{"found 0 of the 3 requested snapshot IDs to verify"}, + } + + err := classifyVerifyError(ctx, result, verifyErr) + + var backupErr *BackupVerificationError + require.NotErrorAs(t, err, &backupErr) + require.ErrorIs(t, err, ErrSnapshotsUnresolved) + require.ErrorIs(t, err, verifyErr) + }) + + // A missing object or blob is real data loss and must stay fatal, even + // though Kopia reports it with wording that also contains "not found". + t.Run("missing blob is still corruption", func(t *testing.T) { + verifyErr := errors.New("while verifying Kopia snapshots: command failed: exit status 1") + result := kopiaClient.VerifyResult{ + ErrorCount: 1, + ErrorStrings: []string{ + "object 8f848427a18ebe0fbc2f063b4616e362 is backed by missing blob " + + "p79f56bafb33cd7823558352ea947c830-s59fd9cbc252f04f7143", + }, + } + + err := classifyVerifyError(ctx, result, verifyErr) + + var backupErr *BackupVerificationError + require.ErrorAs(t, err, &backupErr) + require.NotErrorIs(t, err, ErrSnapshotsUnresolved) + }) + + t.Run("missing object referenced by a directory id is still corruption", func(t *testing.T) { + verifyErr := errors.New("while verifying Kopia snapshots: command failed: exit status 1") + result := kopiaClient.VerifyResult{ + ErrorCount: 1, + ErrorStrings: []string{ + "error reading directory: unable to open object: kdeadbeef: " + + "content kdeadbeef not found: object not found", + }, + } + + err := classifyVerifyError(ctx, result, verifyErr) + + var backupErr *BackupVerificationError + require.ErrorAs(t, err, &backupErr) + }) + + // A mixed result must not be downgraded: one unresolved snapshot alongside + // genuine damage is still corruption. + t.Run("corruption mixed with an unresolved snapshot stays corruption", func(t *testing.T) { + verifyErr := errors.New("while verifying Kopia snapshots: command failed: exit status 1") + result := kopiaClient.VerifyResult{ + ErrorCount: 2, + ErrorStrings: []string{ + "found 0 of the 3 requested snapshot IDs to verify", + "object abc is backed by missing blob p123", + }, + } + + err := classifyVerifyError(ctx, result, verifyErr) + + var backupErr *BackupVerificationError + require.ErrorAs(t, err, &backupErr) + }) } diff --git a/core/internal/kopia/data.go b/core/internal/kopia/data.go index 3915c69e..9e5fea6a 100644 --- a/core/internal/kopia/data.go +++ b/core/internal/kopia/data.go @@ -54,11 +54,26 @@ type Manifest struct { Pins []string `json:"pins,omitempty"` } +// Entry types reported by Kopia for a directory entry. A snapshot's root entry +// is a directory for a directory snapshot and a file when a single file was +// snapshotted, and the two need different flags when verifying by object ID. +const ( + // EntryTypeDirectory marks an entry backed by a directory object. + EntryTypeDirectory = "d" + + // EntryTypeFile marks an entry backed by a file object. + EntryTypeFile = "f" +) + // DirEntry represents a directory entry as stored in JSON stream. type DirEntry struct { // Name is the name of the file or directory. Name string `json:"name,omitempty"` + // Type is the kind of object the entry points at, one of + // EntryTypeDirectory or EntryTypeFile. + Type string `json:"type,omitempty"` + // FileSize is the size of the file in bytes. FileSize int64 `json:"size,omitempty"` diff --git a/core/internal/kopia/write.go b/core/internal/kopia/write.go index bf1a798f..5db2544f 100644 --- a/core/internal/kopia/write.go +++ b/core/internal/kopia/write.go @@ -239,22 +239,66 @@ func (s *Client) SnapshotFileContent( return nil } -// VerifySnapshots verifies snapshot integrity. When called with no -// snapshotIDs, all snapshots in the repository are verified. -// It uses --json output to distinguish corruption (errorCount > 0) from -// infrastructure errors (command failed but no corruption detected). -func (s *Client) VerifySnapshots(ctx context.Context, snapshotIDs ...string) (VerifyResult, error) { - contextLogger := log.FromContext(ctx) +// VerifySnapshotsOptions selects what a verification run covers. +// +// Root object IDs are used rather than snapshot manifest IDs because they are a +// stable identity: "kopia snapshot pin" rewrites a snapshot's manifest under a +// new ID and deletes the old one, so a manifest ID resolved a moment earlier can +// already be gone by the time verification runs. The root object ID is +// unaffected by that rewrite. +// +// Directory and file objects take different flags: passing a file object to +// --directory-id makes Kopia parse file content as a directory listing and +// report it as corruption. +type VerifySnapshotsOptions struct { + // DirectoryIDs restricts verification to the given directory object IDs. + DirectoryIDs []string + + // FileIDs restricts verification to the given file object IDs. + FileIDs []string +} + +// IsEmpty returns true when no object is selected, meaning every snapshot +// visible to the client is verified. +func (o VerifySnapshotsOptions) IsEmpty() bool { + return len(o.DirectoryIDs) == 0 && len(o.FileIDs) == 0 +} + +// Len returns the number of selected objects. +func (o VerifySnapshotsOptions) Len() int { + return len(o.DirectoryIDs) + len(o.FileIDs) +} - args := make([]string, 0, 5+len(snapshotIDs)) +// buildVerifyArgs builds the "kopia snapshot verify" arguments. +func buildVerifyArgs(configFile string, opts VerifySnapshotsOptions) []string { + args := make([]string, 0, 5+opts.Len()) args = append(args, "snapshot", "verify", "--json", "--disable-file-logging", - "--config-file="+s.ConfigFile, + "--config-file="+configFile, ) - args = append(args, snapshotIDs...) + + for _, id := range opts.DirectoryIDs { + args = append(args, "--directory-id="+id) + } + + for _, id := range opts.FileIDs { + args = append(args, "--file-id="+id) + } + + return args +} + +// VerifySnapshots verifies snapshot integrity. When opts selects nothing, all +// snapshots visible to the client are verified. +// It uses --json output to distinguish corruption (errorCount > 0) from +// infrastructure errors (command failed but no corruption detected). +func (s *Client) VerifySnapshots(ctx context.Context, opts VerifySnapshotsOptions) (VerifyResult, error) { + contextLogger := log.FromContext(ctx) + + args := buildVerifyArgs(s.ConfigFile, opts) contextLogger.Info("Verifying Kopia snapshots", "args", args) diff --git a/core/internal/kopia/write_test.go b/core/internal/kopia/write_test.go index 18c87862..f3389c2a 100644 --- a/core/internal/kopia/write_test.go +++ b/core/internal/kopia/write_test.go @@ -50,3 +50,79 @@ func TestParseVerifyOutput(t *testing.T) { assert.Equal(t, VerifyResult{}, result) }) } + +func TestBuildVerifyArgs(t *testing.T) { + t.Run("no selection verifies everything visible to the client", func(t *testing.T) { + args := buildVerifyArgs("/etc/kopia/config", VerifySnapshotsOptions{}) + + assert.Equal(t, []string{ + "snapshot", + "verify", + "--json", + "--disable-file-logging", + "--config-file=/etc/kopia/config", + }, args) + }) + + // Root object IDs are passed as --directory-id/--file-id rather than as + // positional snapshot manifest IDs, which "kopia snapshot pin" can rewrite + // while a verification is in flight (CNP-9006). + t.Run("directory IDs are passed as --directory-id flags", func(t *testing.T) { + args := buildVerifyArgs("/etc/kopia/config", VerifySnapshotsOptions{ + DirectoryIDs: []string{"kaaa", "kbbb"}, + }) + + assert.Equal(t, []string{ + "snapshot", + "verify", + "--json", + "--disable-file-logging", + "--config-file=/etc/kopia/config", + "--directory-id=kaaa", + "--directory-id=kbbb", + }, args) + assert.NotContains(t, args, "kaaa") + }) + + // A file root passed to --directory-id makes Kopia parse file content as a + // directory listing and report a healthy object as corrupt. + t.Run("file IDs are passed as --file-id flags", func(t *testing.T) { + args := buildVerifyArgs("/etc/kopia/config", VerifySnapshotsOptions{ + DirectoryIDs: []string{"kaaa"}, + FileIDs: []string{"d8fe6706"}, + }) + + assert.Equal(t, []string{ + "snapshot", + "verify", + "--json", + "--disable-file-logging", + "--config-file=/etc/kopia/config", + "--directory-id=kaaa", + "--file-id=d8fe6706", + }, args) + }) +} + +func TestVerifySnapshotsOptions(t *testing.T) { + t.Run("empty selection", func(t *testing.T) { + var opts VerifySnapshotsOptions + assert.True(t, opts.IsEmpty()) + assert.Equal(t, 0, opts.Len()) + }) + + t.Run("counts both kinds of object", func(t *testing.T) { + opts := VerifySnapshotsOptions{ + DirectoryIDs: []string{"kaaa", "kbbb"}, + FileIDs: []string{"ccc"}, + } + assert.False(t, opts.IsEmpty()) + assert.Equal(t, 3, opts.Len()) + }) + + t.Run("file IDs alone are a selection", func(t *testing.T) { + opts := VerifySnapshotsOptions{FileIDs: []string{"ccc"}} + assert.False(t, opts.IsEmpty()) + assert.Equal(t, 1, opts.Len()) + }) +} From d554c906a025f567379d77d8715b486619df6998 Mon Sep 17 00:00:00 2001 From: Gabriele Quaresima Date: Tue, 11 Aug 2026 11:33:51 +0200 Subject: [PATCH 2/5] fix(core): re-resolve snapshot IDs when deleting a backup Backup deletion listed a backup's snapshots and then deleted them by manifest ID, which a concurrent "kopia snapshot pin" can rewrite in between. Deleting a replaced ID matches nothing and fails with "no snapshots matched", so the deletion reports an error and the backup survives. Resolve the snapshots again and retry, up to three attempts. Each attempt lists the current IDs, and snapshots removed by an earlier attempt are no longer listed, so the loop converges without deleting anything twice. Deleting by root object ID, the identity used for verification, would be wrong here: unchanged content dedupes to the same root across backups, and Kopia deletes every snapshot matching the ID it is given. Two backups of an idle tablespace share a root object, so deleting one backup by root ID would take the other backup's snapshot with it. The delete loop moved behind a narrow snapshotStore interface so the retry behaviour is covered by unit tests without reworking how Connection holds its Kopia client. Assisted-by: Claude Signed-off-by: Gabriele Quaresima --- .../client/klioclient/kopia/delete.go | 93 ++++++++--- .../client/klioclient/kopia/delete_test.go | 150 ++++++++++++++++++ 2 files changed, 222 insertions(+), 21 deletions(-) create mode 100644 core/internal/client/klioclient/kopia/delete_test.go diff --git a/core/internal/client/klioclient/kopia/delete.go b/core/internal/client/klioclient/kopia/delete.go index c1c47092..45986404 100644 --- a/core/internal/client/klioclient/kopia/delete.go +++ b/core/internal/client/klioclient/kopia/delete.go @@ -27,42 +27,93 @@ import ( "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 + +// 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 { + + 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) + } + + var pending int + + var attemptErr error + + for _, entry := range entries { + if entry.Source.Host != hostname { + continue + } + + pending++ + 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++ + if deleteErr := store.DeleteSnapshot(ctx, entry.ID); deleteErr != nil { + attemptErr = errors.Join(attemptErr, deleteErr) + + continue } + + deleted++ } - } - if err != nil { - return err - } + // 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 + } + + lastErr = attemptErr - if deleted == 0 { - return fmt.Errorf("%w: %s", ErrBackupNotFound, name) + contextLogger.Info("DeleteBackup: retrying with freshly resolved snapshot IDs", + "backupName", name, "attempt", attempt, "error", attemptErr) } - return nil + return lastErr } diff --git a/core/internal/client/klioclient/kopia/delete_test.go b/core/internal/client/klioclient/kopia/delete_test.go new file mode 100644 index 00000000..2e3788b9 --- /dev/null +++ b/core/internal/client/klioclient/kopia/delete_test.go @@ -0,0 +1,150 @@ +/* +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() + + 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 (CNP-9006). + 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) + }) +} From 175848d9c35dd64947bfde9955f345650a0bfefa Mon Sep 17 00:00:00 2001 From: Gabriele Quaresima Date: Tue, 11 Aug 2026 11:49:01 +0200 Subject: [PATCH 3/5] test(e2e): verify the newest backup on tier1 The WAL retention feature already drives three backups through a full maintenance pass, which unpins their snapshots and so rewrites every snapshot manifest under a new ID. Verifying the newest backup at the end of the feature asserts that verification still resolves it afterwards, and exercises that resolution against a real backup: each backup mixes a file root (the control data file, snapshotted on its own) with directory roots (pgdata and metadata), which need different Kopia flags. Only the newest backup is verified, because "klio backup list" spans both tiers and so also reports the backup the feature deletes, which no longer has tier1 snapshots. Also drop the tracker references from the test comments added with the fix, and describe the failure mode instead. Assisted-by: Claude Signed-off-by: Gabriele Quaresima --- .../client/klioclient/kopia/delete_test.go | 2 +- .../client/klioclient/kopia/verify_test.go | 2 +- core/internal/kopia/write_test.go | 2 +- operator/test/e2e/wal_retention_test.go | 60 +++++++++++++++++++ 4 files changed, 63 insertions(+), 3 deletions(-) diff --git a/core/internal/client/klioclient/kopia/delete_test.go b/core/internal/client/klioclient/kopia/delete_test.go index 2e3788b9..d2aaee59 100644 --- a/core/internal/client/klioclient/kopia/delete_test.go +++ b/core/internal/client/klioclient/kopia/delete_test.go @@ -112,7 +112,7 @@ func TestDeleteBackupSnapshots(t *testing.T) { // 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 (CNP-9006). + // 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{ diff --git a/core/internal/client/klioclient/kopia/verify_test.go b/core/internal/client/klioclient/kopia/verify_test.go index a17523e0..26657f10 100644 --- a/core/internal/client/klioclient/kopia/verify_test.go +++ b/core/internal/client/klioclient/kopia/verify_test.go @@ -102,7 +102,7 @@ func TestClassifyVerifyError(t *testing.T) { // An unresolved snapshot set means Kopia never walked any object, so the // result carries no integrity evidence: reporting corruption there fails an - // intact backup. This is the CNP-9006 failure mode. + // intact backup. t.Run("unresolved snapshot set is retryable, not corruption", func(t *testing.T) { verifyErr := errors.New("while verifying Kopia snapshots: command failed: exit status 1") result := kopiaClient.VerifyResult{ diff --git a/core/internal/kopia/write_test.go b/core/internal/kopia/write_test.go index f3389c2a..864feff2 100644 --- a/core/internal/kopia/write_test.go +++ b/core/internal/kopia/write_test.go @@ -66,7 +66,7 @@ func TestBuildVerifyArgs(t *testing.T) { // Root object IDs are passed as --directory-id/--file-id rather than as // positional snapshot manifest IDs, which "kopia snapshot pin" can rewrite - // while a verification is in flight (CNP-9006). + // while a verification is in flight. t.Run("directory IDs are passed as --directory-id flags", func(t *testing.T) { args := buildVerifyArgs("/etc/kopia/config", VerifySnapshotsOptions{ DirectoryIDs: []string{"kaaa", "kbbb"}, diff --git a/operator/test/e2e/wal_retention_test.go b/operator/test/e2e/wal_retention_test.go index 7a41de88..01d3e6b2 100644 --- a/operator/test/e2e/wal_retention_test.go +++ b/operator/test/e2e/wal_retention_test.go @@ -251,6 +251,47 @@ func (s *walRetentionScenario) deleteBackup( return nil } +// verifyBackups runs "klio backup verify" on tier1 for the given backup names +// using the klio CLI. +// +// The backups it is given have already been relayed to tier2 and unpinned, so +// their Kopia snapshot manifests were rewritten under new IDs after they were +// taken. Verification resolves each snapshot to its root object ID, which the +// rewrite leaves untouched, and routes directory and file roots to different +// Kopia flags: a backup has both, since pgdata and metadata are directory +// snapshots while the control data file is snapshotted on its own. Verifying +// here covers that resolution against real backups. +func (s *walRetentionScenario) verifyBackups( + ctx context.Context, + r *resources.Resources, + backupNames []string, +) error { + const klioConfigPath = "/var/lib/postgresql/klio/klio-archive" + + var stdout, stderr bytes.Buffer + + verifyCmd := make([]string, 0, 6+len(backupNames)) + verifyCmd = append(verifyCmd, + "klio", + "backup", + "verify", + "--config", + klioConfigPath, + "--tiers=tier1", + ) + verifyCmd = append(verifyCmd, backupNames...) + + err := r.ExecInPod( + ctx, s.namespace.Name, s.sourcePrimaryPod.Name, cnpgi.KlioPluginContainerName, verifyCmd, &stdout, &stderr) + if err != nil { + return fmt.Errorf( + "failed to verify backups %v: %w; stdout: %s; stderr: %s", + backupNames, err, stdout.String(), stderr.String()) + } + + return nil +} + // kopiaBackupInfo mirrors the subset of klioclient.BackupMetadata fields // needed to identify and order the backups printed by "klio backup list". type kopiaBackupInfo struct { @@ -450,6 +491,25 @@ func (f *WALRetentionFeature) Run() types.StepFunc { t.Logf("Server-side WAL retention verified: %d WAL files remain, all >= begin WAL %q", len(walFiles), boundary) + // Step 7: the newest backup has been through a full maintenance pass, so + // the tier2 unpin rewrote its snapshot manifests. Verifying it now + // asserts that verification still resolves it, which is what the + // manifest-ID rewrite used to break. + // + // Only the newest backup is verified: "klio backup list" spans both + // tiers, so it also reports the backup deleted in step 4, which no + // longer has tier1 snapshots to verify. + t.Log("Verifying the newest backup on tier1...") + remainingBackups, err := f.scenario.listBackups(ctx, r) + require.NoError(t, err, "failed to list backups before verification") + require.NotEmpty(t, remainingBackups, "no backups left to verify") + + newestBackup := remainingBackups[len(remainingBackups)-1] + require.NoError(t, f.scenario.verifyBackups(ctx, r, []string{newestBackup}), + "verification failed for backup %q", newestBackup) + + t.Logf("Verified newest backup %q on tier1", newestBackup) + return ctx } } From 6bfad69457ccf14c3531896f87c090ee451897ac Mon Sep 17 00:00:00 2001 From: Gabriele Quaresima Date: Tue, 11 Aug 2026 11:49:02 +0200 Subject: [PATCH 4/5] docs(agents): document Kopia snapshot identity choice Add a section on when to identify a snapshot by manifest ID and when by root object ID. A manifest ID is not stable, since "kopia snapshot pin" rewrites it, so reads that must survive concurrent maintenance use the root object ID, while deletions must not: unchanged content dedupes to the same root across backups and Kopia deletes every snapshot matching the ID it is given. Also record that directory and file roots take different verify flags, that a verify result reporting only unresolved snapshot IDs is not corruption, and correct the existing note about a stale manifest ID: a delete of a rewritten ID matches nothing and fails rather than deleting the wrong snapshot. Assisted-by: Claude Signed-off-by: Gabriele Quaresima --- AGENTS.md | 32 ++++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index a3494ba3..014e589c 100644 --- a/AGENTS.md +++ b/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,34 @@ 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`) and the + consumer's unpin (`getPinnedSnapshots`) both do 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). + +Also note that a Kopia verify result reporting only "found 0 of the N requested +snapshot IDs to verify" is **not** corruption: nothing was read, so it carries +no evidence about repository integrity. Treat it as retryable. Errors naming a +missing object or blob are genuine corruption. + ### Dagger caching issues When running e2e tests, Dagger may cache Helm repo indexes. If a new version of From 2410b28de908084049d8152a12c47db49bbfac0f Mon Sep 17 00:00:00 2001 From: Gabriele Quaresima Date: Tue, 11 Aug 2026 15:05:59 +0200 Subject: [PATCH 5/5] fix(core): tighten verify/delete after the root-object-ID switch The unresolved-snapshot verify classification can never trigger: once buildVerifyArgs only emits --directory-id/--file-id flags, Kopia never produces the "requested snapshot IDs to verify" error it matched. Removed it; classifyVerifyError again treats any errorCount > 0 as corruption. Also: - verifySpecificBackups no longer aborts every backup when one fails to resolve; it joins the error and still verifies the rest. - DeleteBackup now waits between retry attempts instead of retrying immediately, matching the backoff already used for the same pin-rewrite race elsewhere. - AGENTS.md no longer calls the tier1 unpin a safe root-ID read: unpinning by root ID mutates every snapshot sharing that root, same as delete. It stays only because the step is best-effort. - Reworded the e2e verification step to say it exercises root-object-ID resolution, not the rewrite race itself. Assisted-by: Claude Signed-off-by: Gabriele Quaresima --- AGENTS.md | 24 +++--- .../client/klioclient/kopia/delete.go | 83 ++++++++++++++----- .../client/klioclient/kopia/delete_test.go | 4 + .../client/klioclient/kopia/verify.go | 69 +++++---------- .../client/klioclient/kopia/verify_test.go | 37 --------- operator/test/e2e/wal_retention_test.go | 8 +- 6 files changed, 107 insertions(+), 118 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 014e589c..f35b55c0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -222,23 +222,25 @@ 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`) and the - consumer's unpin (`getPinnedSnapshots`) both do 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. + 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). - -Also note that a Kopia verify result reporting only "found 0 of the N requested -snapshot IDs to verify" is **not** corruption: nothing was read, so it carries -no evidence about repository integrity. Treat it as retryable. Errors naming a -missing object or blob are genuine corruption. +- **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 diff --git a/core/internal/client/klioclient/kopia/delete.go b/core/internal/client/klioclient/kopia/delete.go index 45986404..bc1a238b 100644 --- a/core/internal/client/klioclient/kopia/delete.go +++ b/core/internal/client/klioclient/kopia/delete.go @@ -23,6 +23,7 @@ import ( "context" "errors" "fmt" + "time" "github.com/cloudnative-pg/machinery/pkg/log" @@ -37,6 +38,14 @@ var ErrBackupNotFound = errors.New("backup not found") // 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) @@ -74,26 +83,8 @@ func deleteBackupSnapshots(ctx context.Context, store snapshotStore, hostname, n return fmt.Errorf("while listing snapshots: %w", err) } - var pending int - - var attemptErr 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 { - attemptErr = errors.Join(attemptErr, deleteErr) - - continue - } - - deleted++ - } + 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. @@ -113,7 +104,59 @@ func deleteBackupSnapshots(ctx context.Context, store snapshotStore, hostname, n 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 + } + } } 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 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 + } +} diff --git a/core/internal/client/klioclient/kopia/delete_test.go b/core/internal/client/klioclient/kopia/delete_test.go index d2aaee59..d059b5ea 100644 --- a/core/internal/client/klioclient/kopia/delete_test.go +++ b/core/internal/client/klioclient/kopia/delete_test.go @@ -80,6 +80,10 @@ func manifest(id, host string) kopia.Manifest { 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{ diff --git a/core/internal/client/klioclient/kopia/verify.go b/core/internal/client/klioclient/kopia/verify.go index 12022474..8b6830d0 100644 --- a/core/internal/client/klioclient/kopia/verify.go +++ b/core/internal/client/klioclient/kopia/verify.go @@ -23,7 +23,6 @@ import ( "context" "errors" "fmt" - "strings" "github.com/cloudnative-pg/machinery/pkg/log" @@ -36,11 +35,6 @@ var ( // found for a backup name. ErrNoSnapshotsForBackup = errors.New("no snapshots found for backup") - // ErrSnapshotsUnresolved is returned when Kopia could not resolve the - // snapshots it was asked to verify, so no integrity check was performed. - // It is retryable and must not be reported as corruption. - ErrSnapshotsUnresolved = errors.New("backup verification could not resolve the requested snapshots") - // ErrUnsupportedRootEntryType is returned when a snapshot's root entry is // neither a directory nor a file, so it cannot be verified by object ID. ErrUnsupportedRootEntryType = errors.New("unsupported snapshot root entry type") @@ -109,24 +103,42 @@ func (s *Connection) verifyAllBackups(ctx context.Context, hostname string) erro // verifySpecificBackups verifies the specified backups by resolving them to the // root object IDs of their snapshots. +// +// A backup whose snapshots fail to resolve does not stop the others from being +// verified: their resolution errors are joined and returned alongside whatever +// verification result the successfully resolved backups produce. func (s *Connection) verifySpecificBackups(ctx context.Context, hostname string, backupNames []string) error { contextLogger := log.FromContext(ctx) var verifyOpts kopiaClient.VerifySnapshotsOptions + + var resolveErr error + for _, name := range backupNames { opts, err := s.getRootObjectsForBackup(ctx, hostname, name) if err != nil { - return fmt.Errorf("backup %q: %w", name, err) + resolveErr = errors.Join(resolveErr, fmt.Errorf("backup %q: %w", name, err)) + + continue } + verifyOpts.DirectoryIDs = append(verifyOpts.DirectoryIDs, opts.DirectoryIDs...) verifyOpts.FileIDs = append(verifyOpts.FileIDs, opts.FileIDs...) } + if verifyOpts.IsEmpty() { + return resolveErr + } + contextLogger.Info("Verifying backups", "backupNames", backupNames, "snapshotCount", verifyOpts.Len()) result, err := s.kopia.VerifySnapshots(ctx, verifyOpts) if err != nil { - return classifyVerifyError(ctx, result, err) + return errors.Join(resolveErr, classifyVerifyError(ctx, result, err)) + } + + if resolveErr != nil { + return resolveErr } contextLogger.Info("All backups verified successfully") @@ -193,39 +205,12 @@ func (s *Connection) getRootObjectsForBackup( return opts, nil } -// unresolvedSnapshotMarker appears in a Kopia verify error when the snapshot -// manifests it was asked to verify could not be resolved, as in -// "found 0 of the 3 requested snapshot IDs to verify". Kopia emits it from the -// manifest lookup that precedes any object walk, so it means "nothing was -// verified", never "the data is damaged". Errors about a missing object or blob -// use different wording and are genuine corruption evidence. -const unresolvedSnapshotMarker = "requested snapshot IDs to verify" - -// isUnresolvedSnapshotSet reports whether every error in the result is Kopia -// failing to resolve the requested snapshots. Such a result carries no -// information about repository integrity: it is retryable, and treating it as -// corruption fails a backup that is in fact intact. -func isUnresolvedSnapshotSet(result kopiaClient.VerifyResult) bool { - if len(result.ErrorStrings) == 0 { - return false - } - - for _, e := range result.ErrorStrings { - if !strings.Contains(e, unresolvedSnapshotMarker) { - return false - } - } - - return true -} - // classifyVerifyError inspects the verify result to distinguish corruption -// (errorCount > 0 with integrity evidence) from errors that say nothing about -// the data, which the caller retries. +// (errorCount > 0) from infrastructure errors. func classifyVerifyError(ctx context.Context, result kopiaClient.VerifyResult, err error) error { contextLogger := log.FromContext(ctx) - if result.ErrorCount > 0 && !isUnresolvedSnapshotSet(result) { + if result.ErrorCount > 0 { contextLogger.Error(err, "Backup verification detected corruption", "errorCount", result.ErrorCount, "errors", result.ErrorStrings, @@ -237,15 +222,5 @@ func classifyVerifyError(ctx context.Context, result kopiaClient.VerifyResult, e } } - if result.ErrorCount > 0 { - contextLogger.Info("Backup verification could not resolve the requested snapshots, "+ - "nothing was verified; reporting a retryable error rather than corruption", - "errorCount", result.ErrorCount, - "errors", result.ErrorStrings, - ) - - return fmt.Errorf("%w: %w", ErrSnapshotsUnresolved, err) - } - return fmt.Errorf("backup verification encountered an infrastructure error: %w", err) } diff --git a/core/internal/client/klioclient/kopia/verify_test.go b/core/internal/client/klioclient/kopia/verify_test.go index 26657f10..492fcd32 100644 --- a/core/internal/client/klioclient/kopia/verify_test.go +++ b/core/internal/client/klioclient/kopia/verify_test.go @@ -100,24 +100,6 @@ func TestClassifyVerifyError(t *testing.T) { require.NotErrorAs(t, err, &backupErr) }) - // An unresolved snapshot set means Kopia never walked any object, so the - // result carries no integrity evidence: reporting corruption there fails an - // intact backup. - t.Run("unresolved snapshot set is retryable, not corruption", func(t *testing.T) { - verifyErr := errors.New("while verifying Kopia snapshots: command failed: exit status 1") - result := kopiaClient.VerifyResult{ - ErrorCount: 1, - ErrorStrings: []string{"found 0 of the 3 requested snapshot IDs to verify"}, - } - - err := classifyVerifyError(ctx, result, verifyErr) - - var backupErr *BackupVerificationError - require.NotErrorAs(t, err, &backupErr) - require.ErrorIs(t, err, ErrSnapshotsUnresolved) - require.ErrorIs(t, err, verifyErr) - }) - // A missing object or blob is real data loss and must stay fatal, even // though Kopia reports it with wording that also contains "not found". t.Run("missing blob is still corruption", func(t *testing.T) { @@ -134,7 +116,6 @@ func TestClassifyVerifyError(t *testing.T) { var backupErr *BackupVerificationError require.ErrorAs(t, err, &backupErr) - require.NotErrorIs(t, err, ErrSnapshotsUnresolved) }) t.Run("missing object referenced by a directory id is still corruption", func(t *testing.T) { @@ -152,22 +133,4 @@ func TestClassifyVerifyError(t *testing.T) { var backupErr *BackupVerificationError require.ErrorAs(t, err, &backupErr) }) - - // A mixed result must not be downgraded: one unresolved snapshot alongside - // genuine damage is still corruption. - t.Run("corruption mixed with an unresolved snapshot stays corruption", func(t *testing.T) { - verifyErr := errors.New("while verifying Kopia snapshots: command failed: exit status 1") - result := kopiaClient.VerifyResult{ - ErrorCount: 2, - ErrorStrings: []string{ - "found 0 of the 3 requested snapshot IDs to verify", - "object abc is backed by missing blob p123", - }, - } - - err := classifyVerifyError(ctx, result, verifyErr) - - var backupErr *BackupVerificationError - require.ErrorAs(t, err, &backupErr) - }) } diff --git a/operator/test/e2e/wal_retention_test.go b/operator/test/e2e/wal_retention_test.go index 01d3e6b2..aa616244 100644 --- a/operator/test/e2e/wal_retention_test.go +++ b/operator/test/e2e/wal_retention_test.go @@ -492,9 +492,11 @@ func (f *WALRetentionFeature) Run() types.StepFunc { len(walFiles), boundary) // Step 7: the newest backup has been through a full maintenance pass, so - // the tier2 unpin rewrote its snapshot manifests. Verifying it now - // asserts that verification still resolves it, which is what the - // manifest-ID rewrite used to break. + // the tier2 unpin has already rewritten its snapshot manifests by the + // time we get here. Verifying it now exercises real resolution by root + // object ID against a backup that mixes directory and file roots. It + // does not reproduce the manifest-rewrite race itself, since maintenance + // has settled long before this step runs. // // Only the newest backup is verified: "klio backup list" spans both // tiers, so it also reports the backup deleted in step 4, which no