Don't fail deletion on missing backups (#4038)

Allows ignoring missing backups during deletion by
passing in an addition bool param.

Since this changes the API, we can remove the final
commit to make ignoring missing backups the
default behavior. This will change CLI behavior
though

---

#### Does this PR need a docs update or release note?

- [ ] ✅ Yes, it's included
- [ ] 🕐 Yes, but in a later PR
- [x] ⛔ No

#### Type of change

- [ ] 🌻 Feature
- [ ] 🐛 Bugfix
- [ ] 🗺️ Documentation
- [ ] 🤖 Supportability/Tests
- [ ] 💻 CI/Deployment
- [x] 🧹 Tech Debt/Cleanup

#### Issue(s)

* #4019

#### Test Plan

- [ ] 💪 Manual
- [x] ⚡ Unit test
- [ ] 💚 E2E

src/cli/backup/backup.go

@@ -283,7 +283,7 @@ func genericDeleteCommand(
 
 	defer utils.CloseRepo(ctx, r)
 
-	if err := r.DeleteBackups(ctx, bID); err != nil {
+	if err := r.DeleteBackups(ctx, true, bID); err != nil {
 		return Only(ctx, clues.Wrap(err, "Deleting backup "+bID))
 	}
 

src/cmd/longevity_test/longevity.go

@@ -48,7 +48,7 @@ func deleteBackups(
 
 	for _, backup := range backups {
 		if backup.StartAndEndTime.CompletedAt.Before(cutoff) {
-			if err := r.DeleteBackups(ctx, backup.ID.String()); err != nil {
+			if err := r.DeleteBackups(ctx, true, backup.ID.String()); err != nil {
 				return nil, clues.Wrap(
 					err,
 					"deleting backup").

src/pkg/repository/repository.go

@@ -89,7 +89,7 @@ type Repository interface {
 		ctx context.Context,
 		rcOpts ctrlRepo.Retention,
 	) (operations.RetentionConfigOperation, error)
-	DeleteBackups(ctx context.Context, ids ...string) error
+	DeleteBackups(ctx context.Context, failOnMissing bool, ids ...string) error
 	BackupGetter
 	// ConnectToM365 establishes graph api connections
 	// and initializes api client configurations.
@@ -633,16 +633,26 @@ func getBackupErrors(
 // DeleteBackups removes the backups from both the model store and the backup
 // storage.
 //
+// If failOnMissing is true then returns an error if a backup model can't be
+// found. Otherwise ignores missing backup models.
+//
+// Missing models or snapshots during the actual deletion do not cause errors.
+//
 // All backups are delete as an atomic unit so any failures will result in no
 // deletions.
-func (r repository) DeleteBackups(ctx context.Context, ids ...string) error {
+func (r repository) DeleteBackups(
-	return deleteBackups(ctx, store.NewWrapper(r.modelStore), ids...)
+	ctx context.Context,
+	failOnMissing bool,
+	ids ...string,
+) error {
+	return deleteBackups(ctx, store.NewWrapper(r.modelStore), failOnMissing, ids...)
 }
 
-// deleteBackup handles the processing for Backup.
+// deleteBackup handles the processing for backup deletion.
 func deleteBackups(
 	ctx context.Context,
 	sw store.BackupGetterModelDeleter,
+	failOnMissing bool,
 	ids ...string,
 ) error {
 	// Although we haven't explicitly stated it, snapshots are technically
@@ -655,6 +665,10 @@ func deleteBackups(
 	for _, id := range ids {
 		b, err := sw.GetBackup(ctx, model.StableID(id))
 		if err != nil {
+			if !failOnMissing && errors.Is(err, data.ErrNotFound) {
+				continue
+			}
+
 			return clues.Stack(errWrapper(err)).
 				WithClues(ctx).
 				With("delete_backup_id", id)

src/pkg/repository/repository_test.go

@@ -322,7 +322,7 @@ func (suite *RepositoryIntegrationSuite) TestNewBackupAndDelete() {
 
 	backupID := string(bo.Results.BackupID)
 
-	err = r.DeleteBackups(ctx, backupID)
+	err = r.DeleteBackups(ctx, true, backupID)
 	require.NoError(t, err, "deleting backup: %v", clues.ToCore(err))
 
 	// This operation should fail since the backup doesn't exist anymore.

src/pkg/repository/repository_unexported_test.go

@@ -373,46 +373,47 @@ func (m *mockBackupGetterModelDeleter) DeleteWithModelStoreIDs(
 func (suite *RepositoryBackupsUnitSuite) TestDeleteBackups() {
 	bup := &backup.Backup{
 		BaseModel: model.BaseModel{
-			ID:           model.StableID(uuid.NewString()),
+			ID:           model.StableID("current-bup-id"),
-			ModelStoreID: manifest.ID(uuid.NewString()),
+			ModelStoreID: manifest.ID("current-bup-msid"),
 		},
-		SnapshotID:    uuid.NewString(),
+		SnapshotID:    "current-bup-dsid",
-		StreamStoreID: uuid.NewString(),
+		StreamStoreID: "current-bup-ssid",
 	}
 
 	bupLegacy := &backup.Backup{
 		BaseModel: model.BaseModel{
-			ID:           model.StableID(uuid.NewString()),
+			ID:           model.StableID("legacy-bup-id"),
-			ModelStoreID: manifest.ID(uuid.NewString()),
+			ModelStoreID: manifest.ID("legacy-bup-msid"),
 		},
-		SnapshotID: uuid.NewString(),
+		SnapshotID: "legacy-bup-dsid",
-		DetailsID:  uuid.NewString(),
+		DetailsID:  "legacy-bup-did",
 	}
 
 	bupNoSnapshot := &backup.Backup{
 		BaseModel: model.BaseModel{
-			ID:           model.StableID(uuid.NewString()),
+			ID:           model.StableID("ns-bup-id"),
-			ModelStoreID: manifest.ID(uuid.NewString()),
+			ModelStoreID: manifest.ID("ns-bup-id-msid"),
 		},
-		StreamStoreID: uuid.NewString(),
+		StreamStoreID: "ns-bup-ssid",
 	}
 
 	bupNoDetails := &backup.Backup{
 		BaseModel: model.BaseModel{
-			ID:           model.StableID(uuid.NewString()),
+			ID:           model.StableID("nssid-bup-id"),
-			ModelStoreID: manifest.ID(uuid.NewString()),
+			ModelStoreID: manifest.ID("nssid-bup-msid"),
 		},
-		SnapshotID: uuid.NewString(),
+		SnapshotID: "nssid-bup-dsid",
 	}
 
 	table := []struct {
-		name       string
+		name          string
-		inputIDs   []model.StableID
+		inputIDs      []model.StableID
-		gets       []getRes
+		gets          []getRes
-		expectGets []model.StableID
+		expectGets    []model.StableID
-		dels       []error
+		dels          []error
-		expectDels [][]string
+		expectDels    [][]string
-		expectErr  func(t *testing.T, result error)
+		failOnMissing bool
+		expectErr     func(t *testing.T, result error)
 	}{
 		{
 			name: "SingleBackup NoError",
@@ -450,6 +451,7 @@ func (suite *RepositoryBackupsUnitSuite) TestDeleteBackups() {
 			expectGets: []model.StableID{
 				bup.ID,
 			},
+			failOnMissing: true,
 			expectErr: func(t *testing.T, result error) {
 				assert.ErrorIs(t, result, data.ErrNotFound, clues.ToCore(result))
 				assert.ErrorIs(t, result, ErrorBackupNotFound, clues.ToCore(result))
@@ -585,7 +587,7 @@ func (suite *RepositoryBackupsUnitSuite) TestDeleteBackups() {
 			},
 		},
 		{
-			name: "MultipleBackups GetError",
+			name: "MultipleBackups GetError FailOnMissing",
 			inputIDs: []model.StableID{
 				bup.ID,
 				bupLegacy.ID,
@@ -604,11 +606,48 @@ func (suite *RepositoryBackupsUnitSuite) TestDeleteBackups() {
 				bupNoSnapshot.ID,
 				bupNoDetails.ID,
 			},
+			failOnMissing: true,
 			expectErr: func(t *testing.T, result error) {
 				assert.ErrorIs(t, result, data.ErrNotFound, clues.ToCore(result))
 				assert.ErrorIs(t, result, ErrorBackupNotFound, clues.ToCore(result))
 			},
 		},
+		{
+			name: "MultipleBackups GetError NoFailOnMissing",
+			inputIDs: []model.StableID{
+				bup.ID,
+				bupLegacy.ID,
+				bupNoSnapshot.ID,
+				bupNoDetails.ID,
+			},
+			gets: []getRes{
+				{bup: bup},
+				{err: data.ErrNotFound},
+				{bup: bupNoSnapshot},
+				{bup: bupNoDetails},
+			},
+			expectGets: []model.StableID{
+				bup.ID,
+				bupLegacy.ID,
+				bupNoSnapshot.ID,
+				bupNoDetails.ID,
+			},
+			dels: []error{nil},
+			expectDels: [][]string{
+				{
+					string(bup.ModelStoreID),
+					bup.SnapshotID,
+					bup.StreamStoreID,
+					string(bupNoSnapshot.ModelStoreID),
+					bupNoSnapshot.StreamStoreID,
+					string(bupNoDetails.ModelStoreID),
+					bupNoDetails.SnapshotID,
+				},
+			},
+			expectErr: func(t *testing.T, result error) {
+				assert.NoError(t, result, clues.ToCore(result))
+			},
+		},
 	}
 	for _, test := range table {
 		suite.Run(test.name, func() {
@@ -631,7 +670,7 @@ func (suite *RepositoryBackupsUnitSuite) TestDeleteBackups() {
 				strIDs = append(strIDs, string(id))
 			}
 
-			err := deleteBackups(ctx, m, strIDs...)
+			err := deleteBackups(ctx, m, test.failOnMissing, strIDs...)
 			test.expectErr(t, err)
 		})
 	}