Fixup manifest selection with fallback logic (#3097)

Fixup errors in manifest search logic that would cause
Corso to fallback to a full backup

* don't request fallback manifests if the user display name is the same as the user ID while Corso transitions to using user IDs
* dedupe manifests that are selected for multiple reasons

---

#### 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
- [x] 🐛 Bugfix
- [ ] 🗺️ Documentation
- [ ] 🤖 Supportability/Tests
- [ ] 💻 CI/Deployment
- [ ] 🧹 Tech Debt/Cleanup

#### Issue(s)

* closes #3089

#### Test Plan

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

src/internal/operations/backup.go

@@ -302,7 +302,8 @@ func (op *BackupOperation) do(
 }
 
 func makeFallbackReasons(sel selectors.Selector) []kopia.Reason {
-	if sel.PathService() != path.SharePointService {
+	if sel.PathService() != path.SharePointService &&
+		sel.DiscreteOwner != sel.DiscreteOwnerName {
 		return selectorToReasons(sel, true)
 	}
 

src/internal/operations/manifests.go

@@ -6,6 +6,7 @@ import (
 	"github.com/alcionai/clues"
 	"github.com/kopia/kopia/repo/manifest"
 	"github.com/pkg/errors"
+	"golang.org/x/exp/maps"
 
 	"github.com/alcionai/corso/src/internal/connector/graph"
 	"github.com/alcionai/corso/src/internal/data"
@@ -76,7 +77,7 @@ func produceManifestsAndMetadata(
 
 	// Also check distinct bases for the fallback set.
 	if err := verifyDistinctBases(ctx, fbms); err != nil {
-		logger.CtxErr(ctx, err).Info("base snapshot collision, falling back to full backup")
+		logger.CtxErr(ctx, err).Info("fallback snapshot collision, falling back to full backup")
 		return ms, nil, false, nil
 	}
 
@@ -100,7 +101,7 @@ func produceManifestsAndMetadata(
 	// details from previous snapshots when using kopia-assisted incrementals.
 	if err := verifyDistinctBases(ctx, ms); err != nil {
 		logger.Ctx(ctx).With("error", err).Infow(
-			"base snapshot collision, falling back to full backup",
+			"unioned snapshot collision, falling back to full backup",
 			clues.In(ctx).Slice()...)
 
 		return ms, nil, false, nil
@@ -255,19 +256,19 @@ func unionManifests(
 	}
 
 	// collect the results into a single slice of manifests
-	ms := []*kopia.ManifestEntry{}
+	ms := map[string]*kopia.ManifestEntry{}
 
 	for _, m := range tups {
 		if m.complete != nil {
-			ms = append(ms, m.complete)
+			ms[string(m.complete.ID)] = m.complete
 		}
 
 		if m.incomplete != nil {
-			ms = append(ms, m.incomplete)
+			ms[string(m.incomplete.ID)] = m.incomplete
 		}
 	}
 
-	return ms
+	return maps.Values(ms)
 }
 
 // verifyDistinctBases is a validation checker that ensures, for a given slice

src/internal/operations/manifests_test.go

@@ -10,6 +10,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"github.com/stretchr/testify/suite"
+	"golang.org/x/exp/maps"
 
 	"github.com/alcionai/corso/src/internal/data"
 	"github.com/alcionai/corso/src/internal/kopia"
@@ -35,13 +36,13 @@ func (mmr mockManifestRestorer) FetchPrevSnapshotManifests(
 	reasons []kopia.Reason,
 	tags map[string]string,
 ) ([]*kopia.ManifestEntry, error) {
-	mans := []*kopia.ManifestEntry{}
+	mans := map[string]*kopia.ManifestEntry{}
 
 	for _, r := range reasons {
 		for _, m := range mmr.mans {
 			for _, mr := range m.Reasons {
 				if mr.ResourceOwner == r.ResourceOwner {
-					mans = append(mans, m)
+					mans[string(m.ID)] = m
 					break
 				}
 			}
@@ -49,10 +50,10 @@ func (mmr mockManifestRestorer) FetchPrevSnapshotManifests(
 	}
 
 	if len(mans) == 0 && len(reasons) == 0 {
-		mans = mmr.mans
+		return mmr.mans, mmr.mrErr
 	}
 
-	return mans, mmr.mrErr
+	return maps.Values(mans), mmr.mrErr
 }
 
 type mockGetBackuper struct {
@@ -479,7 +480,7 @@ func (suite *OperationsManifestsUnitSuite) TestProduceManifestsAndMetadata() {
 			name: "don't get metadata",
 			mr: mockManifestRestorer{
 				mockRestoreProducer: mockRestoreProducer{},
-				mans:                []*kopia.ManifestEntry{makeMan(path.EmailCategory, "", "", "")},
+				mans:                []*kopia.ManifestEntry{makeMan(path.EmailCategory, "id1", "", "")},
 			},
 			gb:        mockGetBackuper{detailsID: did},
 			reasons:   []kopia.Reason{},
@@ -492,7 +493,7 @@ func (suite *OperationsManifestsUnitSuite) TestProduceManifestsAndMetadata() {
 			name: "don't get metadata, incomplete manifest",
 			mr: mockManifestRestorer{
 				mockRestoreProducer: mockRestoreProducer{},
-				mans:                []*kopia.ManifestEntry{makeMan(path.EmailCategory, "", "ir", "")},
+				mans:                []*kopia.ManifestEntry{makeMan(path.EmailCategory, "id1", "ir", "")},
 			},
 			gb:        mockGetBackuper{detailsID: did},
 			reasons:   []kopia.Reason{},
@@ -519,8 +520,8 @@ func (suite *OperationsManifestsUnitSuite) TestProduceManifestsAndMetadata() {
 			mr: mockManifestRestorer{
 				mockRestoreProducer: mockRestoreProducer{},
 				mans: []*kopia.ManifestEntry{
-					makeMan(path.EmailCategory, "", "", ""),
-					makeMan(path.EmailCategory, "", "", ""),
+					makeMan(path.EmailCategory, "id1", "", ""),
+					makeMan(path.EmailCategory, "id2", "", ""),
 				},
 			},
 			gb:        mockGetBackuper{detailsID: did},
@@ -548,8 +549,8 @@ func (suite *OperationsManifestsUnitSuite) TestProduceManifestsAndMetadata() {
 			mr: mockManifestRestorer{
 				mockRestoreProducer: mockRestoreProducer{},
 				mans: []*kopia.ManifestEntry{
-					makeMan(path.EmailCategory, "", "ir", ""),
-					makeMan(path.ContactsCategory, "", "ir", ""),
+					makeMan(path.EmailCategory, "id1", "ir", ""),
+					makeMan(path.ContactsCategory, "id2", "ir", ""),
 				},
 			},
 			gb:        mockGetBackuper{detailsID: did},
@@ -580,7 +581,7 @@ func (suite *OperationsManifestsUnitSuite) TestProduceManifestsAndMetadata() {
 			name: "backup missing details id",
 			mr: mockManifestRestorer{
 				mockRestoreProducer: mockRestoreProducer{},
-				mans:                []*kopia.ManifestEntry{makeMan(path.EmailCategory, "", "", "bid")},
+				mans:                []*kopia.ManifestEntry{makeMan(path.EmailCategory, "id1", "", "bid")},
 			},
 			gb:        mockGetBackuper{},
 			reasons:   []kopia.Reason{},
@@ -654,7 +655,7 @@ func (suite *OperationsManifestsUnitSuite) TestProduceManifestsAndMetadata() {
 			name: "error collecting metadata",
 			mr: mockManifestRestorer{
 				mockRestoreProducer: mockRestoreProducer{err: assert.AnError},
-				mans:                []*kopia.ManifestEntry{makeMan(path.EmailCategory, "", "", "bid")},
+				mans:                []*kopia.ManifestEntry{makeMan(path.EmailCategory, "id1", "", "bid")},
 			},
 			gb:            mockGetBackuper{detailsID: did},
 			reasons:       []kopia.Reason{},
@@ -738,95 +739,162 @@ func (suite *OperationsManifestsUnitSuite) TestProduceManifestsAndMetadata_fallb
 		fbIncomplete = "fb_incmpl"
 	)
 
-	makeMan := func(owner, id, incmpl string) *kopia.ManifestEntry {
+	makeMan := func(id, incmpl string, reasons []kopia.Reason) *kopia.ManifestEntry {
 		return &kopia.ManifestEntry{
 			Manifest: &snapshot.Manifest{
 				ID:               manifest.ID(id),
 				IncompleteReason: incmpl,
 				Tags:             map[string]string{},
 			},
-			Reasons: []kopia.Reason{
-				{
-					ResourceOwner: owner,
-					Service:       path.ExchangeService,
-					Category:      path.EmailCategory,
-				},
-			},
+			Reasons: reasons,
 		}
 	}
 
-	var (
-		manReason = kopia.Reason{
-			ResourceOwner: ro,
-			Service:       path.ExchangeService,
-			Category:      path.EmailCategory,
+	type testInput struct {
+		id         string
+		incomplete bool
 	}
-		fbReason = kopia.Reason{
-			ResourceOwner: fbro,
-			Service:       path.ExchangeService,
-			Category:      path.EmailCategory,
-		}
-		mc  = makeMan(ro, manComplete, "")
-		mi  = makeMan(ro, manIncomplete, "ir")
-		fbc = makeMan(fbro, fbComplete, "")
-		fbi = makeMan(fbro, fbIncomplete, "ir")
-	)
 
 	table := []struct {
 		name            string
-		mr            mockManifestRestorer
+		main            []testInput
+		fallback        []testInput
+		reasons         []kopia.Reason
+		fallbackReasons []kopia.Reason
+		categories      []path.CategoryType
 		assertErr       assert.ErrorAssertionFunc
-		expectMans    []string
+		expectManIDs    []string
 		expectNilMans   bool
 	}{
 		{
 			name: "only mans, no fallbacks",
-			mr: mockManifestRestorer{
-				mans: []*kopia.ManifestEntry{mc, mi},
+			main: []testInput{
+				{
+					id: manComplete,
 				},
-			expectMans: []string{manComplete, manIncomplete},
+				{
+					id:         manIncomplete,
+					incomplete: true,
+				},
+			},
+			categories:   []path.CategoryType{path.EmailCategory},
+			expectManIDs: []string{manComplete, manIncomplete},
 		},
 		{
 			name: "no mans, only fallbacks",
-			mr: mockManifestRestorer{
-				mans: []*kopia.ManifestEntry{fbc, fbi},
+			fallback: []testInput{
+				{
+					id: fbComplete,
 				},
-			expectMans: []string{fbComplete, fbIncomplete},
+				{
+					id:         fbIncomplete,
+					incomplete: true,
+				},
+			},
+			categories:   []path.CategoryType{path.EmailCategory},
+			expectManIDs: []string{fbComplete, fbIncomplete},
 		},
 		{
 			name: "complete mans and fallbacks",
-			mr: mockManifestRestorer{
-				mans: []*kopia.ManifestEntry{mc, fbc},
+			main: []testInput{
+				{
+					id: manComplete,
 				},
-			expectMans: []string{manComplete},
+			},
+			fallback: []testInput{
+				{
+					id: fbComplete,
+				},
+			},
+			categories:   []path.CategoryType{path.EmailCategory},
+			expectManIDs: []string{manComplete},
 		},
 		{
 			name: "incomplete mans and fallbacks",
-			mr: mockManifestRestorer{
-				mans: []*kopia.ManifestEntry{mi, fbi},
+			main: []testInput{
+				{
+					id:         manIncomplete,
+					incomplete: true,
 				},
-			expectMans: []string{manIncomplete},
+			},
+			fallback: []testInput{
+				{
+					id:         fbIncomplete,
+					incomplete: true,
+				},
+			},
+			categories:   []path.CategoryType{path.EmailCategory},
+			expectManIDs: []string{manIncomplete},
 		},
 		{
 			name: "complete and incomplete mans and fallbacks",
-			mr: mockManifestRestorer{
-				mans: []*kopia.ManifestEntry{mc, mi, fbc, fbi},
+			main: []testInput{
+				{
+					id: manComplete,
 				},
-			expectMans: []string{manComplete, manIncomplete},
+				{
+					id:         manIncomplete,
+					incomplete: true,
+				},
+			},
+			fallback: []testInput{
+				{
+					id: fbComplete,
+				},
+				{
+					id:         fbIncomplete,
+					incomplete: true,
+				},
+			},
+			categories:   []path.CategoryType{path.EmailCategory},
+			expectManIDs: []string{manComplete, manIncomplete},
 		},
 		{
 			name: "incomplete mans, complete fallbacks",
-			mr: mockManifestRestorer{
-				mans: []*kopia.ManifestEntry{mi, fbc},
+			main: []testInput{
+				{
+					id:         manIncomplete,
+					incomplete: true,
 				},
-			expectMans: []string{fbComplete, manIncomplete},
+			},
+			fallback: []testInput{
+				{
+					id: fbComplete,
+				},
+			},
+			categories:   []path.CategoryType{path.EmailCategory},
+			expectManIDs: []string{fbComplete, manIncomplete},
 		},
 		{
 			name: "complete mans, incomplete fallbacks",
-			mr: mockManifestRestorer{
-				mans: []*kopia.ManifestEntry{mc, fbi},
+			main: []testInput{
+				{
+					id: manComplete,
 				},
-			expectMans: []string{manComplete},
+			},
+			fallback: []testInput{
+				{
+					id:         fbIncomplete,
+					incomplete: true,
+				},
+			},
+			categories:   []path.CategoryType{path.EmailCategory},
+			expectManIDs: []string{manComplete},
+		},
+		{
+			name: "complete mans, complete fallbacks, multiple reasons",
+			main: []testInput{
+				{
+					id: manComplete,
+				},
+			},
+			fallback: []testInput{
+				{
+					id: fbComplete,
+				},
+			},
+			categories:   []path.CategoryType{path.EmailCategory, path.ContactsCategory},
+			expectManIDs: []string{manComplete},
 		},
 	}
 	for _, test := range table {
@@ -836,12 +904,61 @@ func (suite *OperationsManifestsUnitSuite) TestProduceManifestsAndMetadata_fallb
 			ctx, flush := tester.NewContext()
 			defer flush()
 
+			mainReasons := []kopia.Reason{}
+			fbReasons := []kopia.Reason{}
+
+			for _, cat := range test.categories {
+				mainReasons = append(
+					mainReasons,
+					kopia.Reason{
+						ResourceOwner: ro,
+						Service:       path.ExchangeService,
+						Category:      cat,
+					})
+
+				fbReasons = append(
+					fbReasons,
+					kopia.Reason{
+						ResourceOwner: fbro,
+						Service:       path.ExchangeService,
+						Category:      cat,
+					})
+			}
+
+			mans := []*kopia.ManifestEntry{}
+
+			for _, m := range test.main {
+				incomplete := ""
+				if m.incomplete {
+					incomplete = "ir"
+				}
+
+				mn := makeMan(m.id, incomplete, mainReasons)
+				t.Logf("adding manifest (%p)\n%v\n%v\n\n", mn, *mn.Manifest, mn.Reasons)
+
+				mans = append(mans, makeMan(m.id, incomplete, mainReasons))
+			}
+
+			for _, m := range test.fallback {
+				incomplete := ""
+				if m.incomplete {
+					incomplete = "ir"
+				}
+
+				mn := makeMan(m.id, incomplete, fbReasons)
+				t.Logf("adding manifest (%p)\n%v\n%v\n\n", mn, *mn.Manifest, mn.Reasons)
+
+				mans = append(mans, makeMan(m.id, incomplete, fbReasons))
+			}
+
+			mr := mockManifestRestorer{mans: mans}
+
 			mans, _, b, err := produceManifestsAndMetadata(
 				ctx,
-				&test.mr,
+				&mr,
 				nil,
-				[]kopia.Reason{manReason},
-				[]kopia.Reason{fbReason},
+				mainReasons,
+				fbReasons,
 				"tid",
 				false)
 			require.NoError(t, err, clues.ToCore(err))
@@ -852,7 +969,7 @@ func (suite *OperationsManifestsUnitSuite) TestProduceManifestsAndMetadata_fallb
 				manIDs = append(manIDs, string(m.ID))
 			}
 
-			assert.ElementsMatch(t, test.expectMans, manIDs)
+			assert.ElementsMatch(t, test.expectManIDs, manIDs)
 		})
 	}
 }