Return tombstone instead of canUsePreviousBackup (#4394)

This is necessary to fix the correctness of the backup.

---

#### 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

<!--- Please check the type of change your PR introduces: --->
- [ ] 🌻 Feature
- [x] 🐛 Bugfix
- [ ] 🗺️ Documentation
- [ ] 🤖 Supportability/Tests
- [ ] 💻 CI/Deployment
- [ ] 🧹 Tech Debt/Cleanup

#### Issue(s)

<!-- Can reference multiple issues. Use one of the following "magic words" - "closes, fixes" to auto-close the Github issue. -->
* fixes https://github.com/alcionai/corso/issues/4371

#### Test Plan

<!-- How will this be tested prior to merging.-->
- [ ] 💪 Manual
- [ ] ⚡ Unit test
- [x] 💚 E2E

src/internal/m365/backup.go

@@ -100,7 +100,7 @@ func (ctrl *Controller) ProduceBackupCollections(
 		}
 
 	case path.GroupsService:
-		colls, ssmb, canUsePreviousBackup, err = groups.ProduceBackupCollections(
+		colls, ssmb, err = groups.ProduceBackupCollections(
 			ctx,
 			bpc,
 			ctrl.AC,
@@ -111,6 +111,10 @@ func (ctrl *Controller) ProduceBackupCollections(
 			return nil, nil, false, err
 		}
 
+		// canUsePreviousBacukp can be always returned true for groups as we
+		// return a tombstone collection in case the metadata read fails
+		canUsePreviousBackup = true
+
 	default:
 		return nil, nil, false, clues.Wrap(clues.New(service.String()), "service not supported").WithClues(ctx)
 	}

src/internal/m365/backup_test.go

@@ -11,6 +11,9 @@ import (
 	"github.com/stretchr/testify/suite"
 
 	inMock "github.com/alcionai/corso/src/internal/common/idname/mock"
+	"github.com/alcionai/corso/src/internal/common/ptr"
+	"github.com/alcionai/corso/src/internal/data"
+	"github.com/alcionai/corso/src/internal/data/mock"
 	"github.com/alcionai/corso/src/internal/m365/service/exchange"
 	odConsts "github.com/alcionai/corso/src/internal/m365/service/onedrive/consts"
 	"github.com/alcionai/corso/src/internal/m365/service/sharepoint"
@@ -574,3 +577,123 @@ func (suite *GroupsCollectionIntgSuite) TestCreateGroupsCollection_SharePoint()
 	assert.NotZero(t, status.Successes)
 	t.Log(status.String())
 }
+
+func (suite *GroupsCollectionIntgSuite) TestCreateGroupsCollection_SharePoint_InvalidMetadata() {
+	t := suite.T()
+
+	ctx, flush := tester.NewContext(t)
+	defer flush()
+
+	var (
+		groupID  = tconfig.M365GroupID(t)
+		ctrl     = newController(ctx, t, path.GroupsService)
+		groupIDs = []string{groupID}
+	)
+
+	id, name, err := ctrl.PopulateProtectedResourceIDAndName(ctx, groupID, nil)
+	require.NoError(t, err, clues.ToCore(err))
+
+	sel := selectors.NewGroupsBackup(groupIDs)
+	sel.Include(sel.LibraryFolders([]string{"test"}, selectors.PrefixMatch()))
+
+	sel.SetDiscreteOwnerIDName(id, name)
+
+	site, err := suite.connector.AC.Groups().GetRootSite(ctx, groupID)
+	require.NoError(t, err, clues.ToCore(err))
+
+	pth, err := path.Build(
+		suite.tenantID,
+		groupID,
+		path.GroupsService,
+		path.LibrariesCategory,
+		true,
+		odConsts.SitesPathDir,
+		ptr.Val(site.GetId()))
+	require.NoError(t, err, clues.ToCore(err))
+
+	mmc := []data.RestoreCollection{
+		mock.Collection{
+			Path: pth,
+			ItemData: []data.Item{
+				&mock.Item{
+					ItemID: "previouspath",
+					Reader: io.NopCloser(bytes.NewReader([]byte("invalid"))),
+				},
+			},
+		},
+	}
+
+	bpc := inject.BackupProducerConfig{
+		LastBackupVersion:   version.NoBackup,
+		Options:             control.DefaultOptions(),
+		ProtectedResource:   inMock.NewProvider(id, name),
+		Selector:            sel.Selector,
+		MetadataCollections: mmc,
+	}
+
+	collections, excludes, canUsePreviousBackup, err := ctrl.ProduceBackupCollections(
+		ctx,
+		bpc,
+		fault.New(true))
+	require.NoError(t, err, clues.ToCore(err))
+	assert.True(t, canUsePreviousBackup, "can use previous backup")
+	// No excludes yet as this isn't an incremental backup.
+	assert.True(t, excludes.Empty())
+
+	// we don't know an exact count of drives this will produce,
+	// but it should be more than one.
+	assert.Greater(t, len(collections), 1)
+
+	p, err := path.BuildMetadata(
+		suite.tenantID,
+		groupID,
+		path.GroupsService,
+		path.LibrariesCategory,
+		false)
+	require.NoError(t, err, clues.ToCore(err))
+
+	p, err = p.Append(false, odConsts.SitesPathDir)
+	require.NoError(t, err, clues.ToCore(err))
+
+	foundSitesMetadata := false
+	foundRootTombstone := false
+
+	sp, err := path.BuildPrefix(
+		suite.tenantID,
+		groupID,
+		path.GroupsService,
+		path.LibrariesCategory)
+	require.NoError(t, err, clues.ToCore(err))
+
+	sp, err = sp.Append(false, odConsts.SitesPathDir, ptr.Val(site.GetId()))
+	require.NoError(t, err, clues.ToCore(err))
+
+	for _, coll := range collections {
+		if coll.State() == data.DeletedState {
+			if coll.PreviousPath() != nil && coll.PreviousPath().String() == sp.String() {
+				foundRootTombstone = true
+			}
+
+			continue
+		}
+
+		sitesMetadataCollection := coll.FullPath().String() == p.String()
+
+		for object := range coll.Items(ctx, fault.New(true)) {
+			if object.ID() == "previouspath" && sitesMetadataCollection {
+				foundSitesMetadata = true
+			}
+
+			buf := &bytes.Buffer{}
+			_, err := buf.ReadFrom(object.ToReader())
+			assert.NoError(t, err, "reading item", clues.ToCore(err))
+		}
+	}
+
+	assert.True(t, foundSitesMetadata, "missing sites metadata")
+	assert.True(t, foundRootTombstone, "missing root tombstone")
+
+	status := ctrl.Wait()
+	assert.NotZero(t, status.Successes)
+	t.Log(status.String())
+}

src/internal/m365/collection/drive/collections.go

@@ -135,11 +135,6 @@ func deserializeMetadata(
 					continue
 				}
 
-				if err == nil {
-					// Successful decode.
-					continue
-				}
-
 				// This is conservative, but report an error if either any of the items
 				// for any of the deserialized maps have duplicate drive IDs or there's
 				// some other problem deserializing things. This will cause the entire
@@ -147,7 +142,9 @@ func deserializeMetadata(
 				// these cases. We can make the logic for deciding when to continue vs.
 				// when to fail less strict in the future if needed.
 				if err != nil {
-					return nil, nil, false, clues.Stack(err).WithClues(ictx)
+					errs.Fail(clues.Stack(err).WithClues(ictx))
+
+					return map[string]string{}, map[string]map[string]string{}, false, nil
 				}
 			}
 		}

src/internal/m365/collection/drive/collections_test.go

@@ -978,7 +978,9 @@ func (suite *OneDriveCollectionsUnitSuite) TestDeserializeMetadata() {
 		{
 			// Bad formats are logged but skip adding entries to the maps and don't
 			// return an error.
-			name: "BadFormat",
+			name:           "BadFormat",
+			expectedDeltas: map[string]string{},
+			expectedPaths:  map[string]map[string]string{},
 			cols: []func() []graph.MetadataCollectionEntry{
 				func() []graph.MetadataCollectionEntry {
 					return []graph.MetadataCollectionEntry{
@@ -989,7 +991,7 @@ func (suite *OneDriveCollectionsUnitSuite) TestDeserializeMetadata() {
 				},
 			},
 			canUsePreviousBackup: false,
-			errCheck:             assert.Error,
+			errCheck:             assert.NoError,
 		},
 		{
 			// Unexpected files are logged and skipped. They don't cause an error to
@@ -1054,10 +1056,10 @@ func (suite *OneDriveCollectionsUnitSuite) TestDeserializeMetadata() {
 					}
 				},
 			},
-			expectedDeltas:       nil,
+			expectedDeltas:       map[string]string{},
-			expectedPaths:        nil,
+			expectedPaths:        map[string]map[string]string{},
 			canUsePreviousBackup: false,
-			errCheck:             assert.Error,
+			errCheck:             assert.NoError,
 		},
 		{
 			name: "DriveAlreadyFound_Deltas",
@@ -1084,10 +1086,10 @@ func (suite *OneDriveCollectionsUnitSuite) TestDeserializeMetadata() {
 					}
 				},
 			},
-			expectedDeltas:       nil,
+			expectedDeltas:       map[string]string{},
-			expectedPaths:        nil,
+			expectedPaths:        map[string]map[string]string{},
 			canUsePreviousBackup: false,
-			errCheck:             assert.Error,
+			errCheck:             assert.NoError,
 		},
 	}
 

src/internal/m365/collection/groups/channel_handler.go

@@ -67,6 +67,15 @@ func (bh channelsBackupHandler) canonicalPath(
 			false)
 }
 
+func (bh channelsBackupHandler) PathPrefix(tenantID string) (path.Path, error) {
+	return path.Build(
+		tenantID,
+		bh.protectedResource,
+		path.GroupsService,
+		path.ChannelMessagesCategory,
+		false)
+}
+
 func (bh channelsBackupHandler) GetChannelMessage(
 	ctx context.Context,
 	teamID, channelID, itemID string,

src/internal/m365/service/groups/backup.go

@@ -22,6 +22,7 @@ import (
 	"github.com/alcionai/corso/src/pkg/account"
 	"github.com/alcionai/corso/src/pkg/backup/identity"
 	"github.com/alcionai/corso/src/pkg/backup/metadata"
+	"github.com/alcionai/corso/src/pkg/control"
 	"github.com/alcionai/corso/src/pkg/fault"
 	"github.com/alcionai/corso/src/pkg/logger"
 	"github.com/alcionai/corso/src/pkg/path"
@@ -35,19 +36,18 @@ func ProduceBackupCollections(
 	creds account.M365Config,
 	su support.StatusUpdater,
 	errs *fault.Bus,
-) ([]data.BackupCollection, *prefixmatcher.StringSetMatcher, bool, error) {
+) ([]data.BackupCollection, *prefixmatcher.StringSetMatcher, error) {
 	b, err := bpc.Selector.ToGroupsBackup()
 	if err != nil {
-		return nil, nil, false, clues.Wrap(err, "groupsDataCollection: parsing selector")
+		return nil, nil, clues.Wrap(err, "groupsDataCollection: parsing selector")
 	}
 
 	var (
-		el                   = errs.Local()
+		el                 = errs.Local()
-		collections          = []data.BackupCollection{}
+		collections        = []data.BackupCollection{}
-		categories           = map[path.CategoryType]struct{}{}
+		categories         = map[path.CategoryType]struct{}{}
-		ssmb                 = prefixmatcher.NewStringSetBuilder()
+		ssmb               = prefixmatcher.NewStringSetBuilder()
-		canUsePreviousBackup bool
+		sitesPreviousPaths = map[string]string{}
-		sitesPreviousPaths   = map[string]string{}
 	)
 
 	ctx = clues.Add(
@@ -60,7 +60,7 @@ func ProduceBackupCollections(
 		bpc.ProtectedResource.ID(),
 		api.CallConfig{})
 	if err != nil {
-		return nil, nil, false, clues.Wrap(err, "getting group").WithClues(ctx)
+		return nil, nil, clues.Wrap(err, "getting group").WithClues(ctx)
 	}
 
 	isTeam := api.IsTeam(ctx, group)
@@ -81,7 +81,7 @@ func ProduceBackupCollections(
 		case path.LibrariesCategory:
 			sites, err := ac.Groups().GetAllSites(ctx, bpc.ProtectedResource.ID(), errs)
 			if err != nil {
-				return nil, nil, false, err
+				return nil, nil, err
 			}
 
 			siteMetadataCollection := map[string][]data.RestoreCollection{}
@@ -108,14 +108,14 @@ func ProduceBackupCollections(
 					ac.Drives(),
 					scope)
 
-				cp, err := bh.SitePathPrefix(creds.AzureTenantID)
+				sp, err := bh.SitePathPrefix(creds.AzureTenantID)
 				if err != nil {
-					return nil, nil, false, clues.Wrap(err, "getting canonical path")
+					return nil, nil, clues.Wrap(err, "getting site path")
 				}
 
-				sitesPreviousPaths[ptr.Val(s.GetId())] = cp.String()
+				sitesPreviousPaths[ptr.Val(s.GetId())] = sp.String()
 
-				cs, cupb, err := site.CollectLibraries(
+				cs, canUsePreviousBackup, err := site.CollectLibraries(
 					ctx,
 					sbpc,
 					bh,
@@ -128,11 +128,11 @@ func ProduceBackupCollections(
 					continue
 				}
 
-				dbcs = append(dbcs, cs...)
+				if !canUsePreviousBackup {
+					dbcs = append(dbcs, data.NewTombstoneCollection(sp, control.Options{}))
+				}
 
-				// FIXME(meain): This can cause incorrect backup
+				dbcs = append(dbcs, cs...)
-				// https://github.com/alcionai/corso/issues/4371
-				canUsePreviousBackup = canUsePreviousBackup || cupb
 			}
 
 		case path.ChannelMessagesCategory:
@@ -140,10 +140,12 @@ func ProduceBackupCollections(
 				continue
 			}
 
-			dbcs, canUsePreviousBackup, err = groups.CreateCollections(
+			bh := groups.NewChannelBackupHandler(bpc.ProtectedResource.ID(), ac.Channels())
+
+			cs, canUsePreviousBackup, err := groups.CreateCollections(
 				ctx,
 				bpc,
-				groups.NewChannelBackupHandler(bpc.ProtectedResource.ID(), ac.Channels()),
+				bh,
 				creds.AzureTenantID,
 				scope,
 				su,
@@ -152,6 +154,17 @@ func ProduceBackupCollections(
 				el.AddRecoverable(ctx, err)
 				continue
 			}
+
+			if !canUsePreviousBackup {
+				tp, err := bh.PathPrefix(creds.AzureTenantID)
+				if err != nil {
+					return nil, nil, clues.Wrap(err, "getting message path")
+				}
+
+				dbcs = append(dbcs, data.NewTombstoneCollection(tp, control.Options{}))
+			}
+
+			dbcs = append(dbcs, cs...)
 		}
 
 		collections = append(collections, dbcs...)
@@ -170,7 +183,7 @@ func ProduceBackupCollections(
 			su,
 			errs)
 		if err != nil {
-			return nil, nil, false, err
+			return nil, nil, err
 		}
 
 		collections = append(collections, baseCols...)
@@ -183,12 +196,12 @@ func ProduceBackupCollections(
 		sitesPreviousPaths,
 		su)
 	if err != nil {
-		return nil, nil, false, err
+		return nil, nil, err
 	}
 
 	collections = append(collections, md)
 
-	return collections, ssmb.ToReader(), canUsePreviousBackup, el.Failure()
+	return collections, ssmb.ToReader(), el.Failure()
 }
 
 func getSitesMetadataCollection(