add namespace to fault items (#3415)

prevent fault items and skips from clobbering eachother by adding a namespace to the fault item that defines a deduplication boundary.  This allows multiple items in a single service operation to have identical ids so long as their namespace differs.

---

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

- [x] ⛔ No

#### Type of change

- [x] 🐛 Bugfix
- [x] 🤖 Supportability/Tests

#### Issue(s)

* #3283

#### Test Plan

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

src/internal/connector/onedrive/collection.go

@@ -318,6 +318,7 @@ func (i *Item) ModTime() time.Time      { return i.info.Modified() }
 // getDriveItemContent fetch drive item's contents with retries
 func (oc *Collection) getDriveItemContent(
 	ctx context.Context,
+	driveID string,
 	item models.DriveItemable,
 	errs *fault.Bus,
 ) (io.ReadCloser, error) {
@@ -338,14 +339,14 @@ func (oc *Collection) getDriveItemContent(
 	if err != nil {
 		if clues.HasLabel(err, graph.LabelsMalware) || (item != nil && item.GetMalware() != nil) {
 			logger.CtxErr(ctx, err).With("skipped_reason", fault.SkipMalware).Info("item flagged as malware")
-			el.AddSkip(fault.FileSkip(fault.SkipMalware, itemID, itemName, graph.ItemInfo(item)))
+			el.AddSkip(fault.FileSkip(fault.SkipMalware, driveID, itemID, itemName, graph.ItemInfo(item)))
 
 			return nil, clues.Wrap(err, "malware item").Label(graph.LabelsSkippable)
 		}
 
 		if clues.HasLabel(err, graph.LabelStatus(http.StatusNotFound)) || graph.IsErrDeletedInFlight(err) {
 			logger.CtxErr(ctx, err).With("skipped_reason", fault.SkipNotFound).Info("item not found")
-			el.AddSkip(fault.FileSkip(fault.SkipNotFound, itemID, itemName, graph.ItemInfo(item)))
+			el.AddSkip(fault.FileSkip(fault.SkipNotFound, driveID, itemID, itemName, graph.ItemInfo(item)))
 
 			return nil, clues.Wrap(err, "deleted item").Label(graph.LabelsSkippable)
 		}
@@ -360,7 +361,7 @@ func (oc *Collection) getDriveItemContent(
 			// restore, or we have to handle it separately by somehow
 			// deleting the entire collection.
 			logger.CtxErr(ctx, err).With("skipped_reason", fault.SkipBigOneNote).Info("max OneNote file size exceeded")
-			el.AddSkip(fault.FileSkip(fault.SkipBigOneNote, itemID, itemName, graph.ItemInfo(item)))
+			el.AddSkip(fault.FileSkip(fault.SkipBigOneNote, driveID, itemID, itemName, graph.ItemInfo(item)))
 
 			return nil, clues.Wrap(err, "max oneNote item").Label(graph.LabelsSkippable)
 		}
@@ -528,7 +529,7 @@ func (oc *Collection) populateItems(ctx context.Context, errs *fault.Bus) {
 				// attempts to read bytes.  Assumption is that kopia will check things
 				// like file modtimes before attempting to read.
 				itemReader := lazy.NewLazyReadCloser(func() (io.ReadCloser, error) {
-					itemData, err := oc.getDriveItemContent(ctx, item, errs)
+					itemData, err := oc.getDriveItemContent(ctx, oc.driveID, item, errs)
 					if err != nil {
 						return nil, err
 					}

src/internal/connector/onedrive/collection_test.go

@@ -710,7 +710,7 @@ func (suite *GetDriveItemUnitTestSuite) TestGetDriveItem_error() {
 				return item, nil
 			}
 
-			_, err := col.getDriveItemContent(ctx, item, errs)
+			_, err := col.getDriveItemContent(ctx, "driveID", item, errs)
 			if test.err == nil {
 				assert.NoError(t, err, clues.ToCore(err))
 				return

src/internal/connector/onedrive/collections.go

@@ -717,10 +717,10 @@ func (c *Collections) UpdateCollections(
 
 		if item.GetMalware() != nil {
 			addtl := graph.ItemInfo(item)
-			skip := fault.FileSkip(fault.SkipMalware, itemID, itemName, addtl)
+			skip := fault.FileSkip(fault.SkipMalware, driveID, itemID, itemName, addtl)
 
 			if isFolder {
-				skip = fault.ContainerSkip(fault.SkipMalware, itemID, itemName, addtl)
+				skip = fault.ContainerSkip(fault.SkipMalware, driveID, itemID, itemName, addtl)
 			}
 
 			errs.AddSkip(skip)

src/internal/streamstore/collectables_test.go

@@ -108,8 +108,8 @@ func (suite *StreamStoreIntgSuite) TestStreamer() {
 				bus := fault.New(false)
 				bus.Fail(clues.New("foo"))
 				bus.AddRecoverable(clues.New("bar"))
-				bus.AddRecoverable(fault.FileErr(clues.New("file"), "file-id", "file-name", map[string]any{"foo": "bar"}))
+				bus.AddRecoverable(fault.FileErr(clues.New("file"), "ns", "file-id", "file-name", map[string]any{"foo": "bar"}))
-				bus.AddSkip(fault.FileSkip(fault.SkipMalware, "file-id", "file-name", map[string]any{"foo": "bar"}))
+				bus.AddSkip(fault.FileSkip(fault.SkipMalware, "ns", "file-id", "file-name", map[string]any{"foo": "bar"}))
 
 				fe := bus.Errors()
 				return fe
@@ -137,8 +137,8 @@ func (suite *StreamStoreIntgSuite) TestStreamer() {
 				bus := fault.New(false)
 				bus.Fail(clues.New("foo"))
 				bus.AddRecoverable(clues.New("bar"))
-				bus.AddRecoverable(fault.FileErr(clues.New("file"), "file-id", "file-name", map[string]any{"foo": "bar"}))
+				bus.AddRecoverable(fault.FileErr(clues.New("file"), "ns", "file-id", "file-name", map[string]any{"foo": "bar"}))
-				bus.AddSkip(fault.FileSkip(fault.SkipMalware, "file-id", "file-name", map[string]any{"foo": "bar"}))
+				bus.AddSkip(fault.FileSkip(fault.SkipMalware, "ns", "file-id", "file-name", map[string]any{"foo": "bar"}))
 
 				fe := bus.Errors()
 				return fe

src/pkg/fault/example_fault_test.go

@@ -428,6 +428,7 @@ func ExampleBus_AddSkip() {
 	// error.  Our only option is to skip it.
 	errs.AddSkip(fault.FileSkip(
 		fault.SkipMalware,
+		"deduplication-namespace",
 		"file-id",
 		"file-name",
 		map[string]any{"foo": "bar"},

src/pkg/fault/fault.go

@@ -204,7 +204,7 @@ type Errors struct {
 
 	// Items are the reduction of all errors (both the failure and the
 	// recovered values) in the Errors struct into a slice of items,
-	// deduplicated by their ID.
+	// deduplicated by their Namespace + ID.
 	Items []Item `json:"items"`
 
 	// Skipped is the accumulation of skipped items.  Skipped items
@@ -218,7 +218,8 @@ type Errors struct {
 }
 
 // itemsIn reduces all errors (both the failure and recovered values)
-// in the Errors struct into a slice of items, deduplicated by their ID.
+// in the Errors struct into a slice of items, deduplicated by their
+// Namespace + ID.
 // Any non-item error is serialized to a clues.ErrCore and returned in
 // the second list.
 func itemsIn(failure error, recovered []error) ([]Item, []*clues.ErrCore) {
@@ -234,12 +235,12 @@ func itemsIn(failure error, recovered []error) ([]Item, []*clues.ErrCore) {
 			continue
 		}
 
-		is[ie.ID] = *ie
+		is[ie.dedupeID()] = *ie
 	}
 
 	var ie *Item
 	if errors.As(failure, &ie) {
-		is[ie.ID] = *ie
+		is[ie.dedupeID()] = *ie
 	}
 
 	return maps.Values(is), non

src/pkg/fault/fault_test.go

@@ -193,7 +193,7 @@ func (suite *FaultErrorsUnitSuite) TestAddSkip() {
 	n.AddRecoverable(assert.AnError)
 	assert.Len(t, n.Skipped(), 0)
 
-	n.AddSkip(fault.OwnerSkip(fault.SkipMalware, "id", "name", nil))
+	n.AddSkip(fault.OwnerSkip(fault.SkipMalware, "ns", "id", "name", nil))
 	assert.Len(t, n.Skipped(), 1)
 }
 
@@ -262,12 +262,12 @@ func (suite *FaultErrorsUnitSuite) TestErrors_Items() {
 			name: "failure item",
 			errs: func() *fault.Errors {
 				b := fault.New(false)
-				b.Fail(fault.OwnerErr(ae, "id", "name", addtl))
+				b.Fail(fault.OwnerErr(ae, "ns", "id", "name", addtl))
 				b.AddRecoverable(ae)
 
 				return b.Errors()
 			},
-			expectItems:       []fault.Item{*fault.OwnerErr(ae, "id", "name", addtl)},
+			expectItems:       []fault.Item{*fault.OwnerErr(ae, "ns", "id", "name", addtl)},
 			expectRecoverable: noncore,
 		},
 		{
@@ -275,25 +275,40 @@ func (suite *FaultErrorsUnitSuite) TestErrors_Items() {
 			errs: func() *fault.Errors {
 				b := fault.New(false)
 				b.Fail(ae)
-				b.AddRecoverable(fault.OwnerErr(ae, "id", "name", addtl))
+				b.AddRecoverable(fault.OwnerErr(ae, "ns", "id", "name", addtl))
 
 				return b.Errors()
 			},
-			expectItems:       []fault.Item{*fault.OwnerErr(ae, "id", "name", addtl)},
+			expectItems:       []fault.Item{*fault.OwnerErr(ae, "ns", "id", "name", addtl)},
 			expectRecoverable: []*clues.ErrCore{},
 		},
 		{
 			name: "two items",
 			errs: func() *fault.Errors {
 				b := fault.New(false)
-				b.Fail(fault.OwnerErr(ae, "oid", "name", addtl))
+				b.Fail(fault.OwnerErr(ae, "ns", "oid", "name", addtl))
-				b.AddRecoverable(fault.FileErr(ae, "fid", "name", addtl))
+				b.AddRecoverable(fault.FileErr(ae, "ns", "fid", "name", addtl))
 
 				return b.Errors()
 			},
 			expectItems: []fault.Item{
-				*fault.OwnerErr(ae, "oid", "name", addtl),
+				*fault.OwnerErr(ae, "ns", "oid", "name", addtl),
-				*fault.FileErr(ae, "fid", "name", addtl),
+				*fault.FileErr(ae, "ns", "fid", "name", addtl),
+			},
+			expectRecoverable: []*clues.ErrCore{},
+		},
+		{
+			name: "two items - diff namespace same id",
+			errs: func() *fault.Errors {
+				b := fault.New(false)
+				b.Fail(fault.OwnerErr(ae, "ns", "id", "name", addtl))
+				b.AddRecoverable(fault.FileErr(ae, "ns2", "id", "name", addtl))
+
+				return b.Errors()
+			},
+			expectItems: []fault.Item{
+				*fault.OwnerErr(ae, "ns", "id", "name", addtl),
+				*fault.FileErr(ae, "ns2", "id", "name", addtl),
 			},
 			expectRecoverable: []*clues.ErrCore{},
 		},
@@ -301,13 +316,13 @@ func (suite *FaultErrorsUnitSuite) TestErrors_Items() {
 			name: "duplicate items - failure priority",
 			errs: func() *fault.Errors {
 				b := fault.New(false)
-				b.Fail(fault.OwnerErr(ae, "id", "name", addtl))
+				b.Fail(fault.OwnerErr(ae, "ns", "id", "name", addtl))
-				b.AddRecoverable(fault.FileErr(ae, "id", "name", addtl))
+				b.AddRecoverable(fault.FileErr(ae, "ns", "id", "name", addtl))
 
 				return b.Errors()
 			},
 			expectItems: []fault.Item{
-				*fault.OwnerErr(ae, "id", "name", addtl),
+				*fault.OwnerErr(ae, "ns", "id", "name", addtl),
 			},
 			expectRecoverable: []*clues.ErrCore{},
 		},
@@ -316,13 +331,13 @@ func (suite *FaultErrorsUnitSuite) TestErrors_Items() {
 			errs: func() *fault.Errors {
 				b := fault.New(false)
 				b.Fail(ae)
-				b.AddRecoverable(fault.FileErr(ae, "fid", "name", addtl))
+				b.AddRecoverable(fault.FileErr(ae, "ns", "fid", "name", addtl))
-				b.AddRecoverable(fault.FileErr(ae, "fid", "name2", addtl))
+				b.AddRecoverable(fault.FileErr(ae, "ns", "fid", "name2", addtl))
 
 				return b.Errors()
 			},
 			expectItems: []fault.Item{
-				*fault.FileErr(ae, "fid", "name2", addtl),
+				*fault.FileErr(ae, "ns", "fid", "name2", addtl),
 			},
 			expectRecoverable: []*clues.ErrCore{},
 		},
@@ -331,13 +346,13 @@ func (suite *FaultErrorsUnitSuite) TestErrors_Items() {
 			errs: func() *fault.Errors {
 				b := fault.New(false)
 				b.Fail(ae)
-				b.AddRecoverable(fault.FileErr(ae, "fid", "name", addtl))
+				b.AddRecoverable(fault.FileErr(ae, "ns", "fid", "name", addtl))
 				b.AddRecoverable(ae)
 
 				return b.Errors()
 			},
 			expectItems: []fault.Item{
-				*fault.FileErr(ae, "fid", "name", addtl),
+				*fault.FileErr(ae, "ns", "fid", "name", addtl),
 			},
 			expectRecoverable: noncore,
 		},

src/pkg/fault/item.go

@@ -49,6 +49,12 @@ var (
 // by the end user (cli or sdk) for surfacing human-readable and
 // identifiable points of failure.
 type Item struct {
+	// deduplication namespace; the maximally-unique boundary of the
+	// item ID.  The scope of this boundary depends on the service.
+	// ex: exchange items are unique within their category, drive items
+	// are only unique within a given drive.
+	Namespace string `json:"namespace"`
+
 	// deduplication identifier; the ID of the observed item.
 	ID string `json:"id"`
 
@@ -72,6 +78,12 @@ type Item struct {
 	Additional map[string]any `json:"additional"`
 }
 
+// dedupeID is the id used to deduplicate items when aggreagating
+// errors in fault.Errors().
+func (i *Item) dedupeID() string {
+	return i.Namespace + i.ID
+}
+
 // Error complies with the error interface.
 func (i *Item) Error() string {
 	if i == nil {
@@ -111,23 +123,24 @@ func (i Item) Values() []string {
 }
 
 // ContainerErr produces a Container-type Item for tracking erroneous items
-func ContainerErr(cause error, id, name string, addtl map[string]any) *Item {
+func ContainerErr(cause error, namespace, id, name string, addtl map[string]any) *Item {
-	return itemErr(ContainerType, cause, id, name, addtl)
+	return itemErr(ContainerType, cause, namespace, id, name, addtl)
 }
 
 // FileErr produces a File-type Item for tracking erroneous items.
-func FileErr(cause error, id, name string, addtl map[string]any) *Item {
+func FileErr(cause error, namespace, id, name string, addtl map[string]any) *Item {
-	return itemErr(FileType, cause, id, name, addtl)
+	return itemErr(FileType, cause, namespace, id, name, addtl)
 }
 
 // OnwerErr produces a ResourceOwner-type Item for tracking erroneous items.
-func OwnerErr(cause error, id, name string, addtl map[string]any) *Item {
+func OwnerErr(cause error, namespace, id, name string, addtl map[string]any) *Item {
-	return itemErr(ResourceOwnerType, cause, id, name, addtl)
+	return itemErr(ResourceOwnerType, cause, namespace, id, name, addtl)
 }
 
 // itemErr produces a Item of the provided type for tracking erroneous items.
-func itemErr(t itemType, cause error, id, name string, addtl map[string]any) *Item {
+func itemErr(t itemType, cause error, namespace, id, name string, addtl map[string]any) *Item {
 	return &Item{
+		Namespace:  namespace,
 		ID:         id,
 		Name:       name,
 		Type:       t,
@@ -228,24 +241,25 @@ func (s Skipped) Values() []string {
 }
 
 // ContainerSkip produces a Container-kind Item for tracking skipped items.
-func ContainerSkip(cause skipCause, id, name string, addtl map[string]any) *Skipped {
+func ContainerSkip(cause skipCause, namespace, id, name string, addtl map[string]any) *Skipped {
-	return itemSkip(ContainerType, cause, id, name, addtl)
+	return itemSkip(ContainerType, cause, namespace, id, name, addtl)
 }
 
 // FileSkip produces a File-kind Item for tracking skipped items.
-func FileSkip(cause skipCause, id, name string, addtl map[string]any) *Skipped {
+func FileSkip(cause skipCause, namespace, id, name string, addtl map[string]any) *Skipped {
-	return itemSkip(FileType, cause, id, name, addtl)
+	return itemSkip(FileType, cause, namespace, id, name, addtl)
 }
 
 // OnwerSkip produces a ResourceOwner-kind Item for tracking skipped items.
-func OwnerSkip(cause skipCause, id, name string, addtl map[string]any) *Skipped {
+func OwnerSkip(cause skipCause, namespace, id, name string, addtl map[string]any) *Skipped {
-	return itemSkip(ResourceOwnerType, cause, id, name, addtl)
+	return itemSkip(ResourceOwnerType, cause, namespace, id, name, addtl)
 }
 
 // itemSkip produces a Item of the provided type for tracking skipped items.
-func itemSkip(t itemType, cause skipCause, id, name string, addtl map[string]any) *Skipped {
+func itemSkip(t itemType, cause skipCause, namespace, id, name string, addtl map[string]any) *Skipped {
 	return &Skipped{
 		Item: Item{
+			Namespace:  namespace,
 			ID:         id,
 			Name:       name,
 			Type:       t,

src/pkg/fault/item_test.go

@@ -36,9 +36,10 @@ func (suite *ItemUnitSuite) TestItem_Error() {
 func (suite *ItemUnitSuite) TestContainerErr() {
 	t := suite.T()
 	addtl := map[string]any{"foo": "bar"}
-	i := ContainerErr(clues.New("foo"), "id", "name", addtl)
+	i := ContainerErr(clues.New("foo"), "ns", "id", "name", addtl)
 
 	expect := Item{
+		Namespace:  "ns",
 		ID:         "id",
 		Name:       "name",
 		Type:       ContainerType,
@@ -52,9 +53,10 @@ func (suite *ItemUnitSuite) TestContainerErr() {
 func (suite *ItemUnitSuite) TestFileErr() {
 	t := suite.T()
 	addtl := map[string]any{"foo": "bar"}
-	i := FileErr(clues.New("foo"), "id", "name", addtl)
+	i := FileErr(clues.New("foo"), "ns", "id", "name", addtl)
 
 	expect := Item{
+		Namespace:  "ns",
 		ID:         "id",
 		Name:       "name",
 		Type:       FileType,
@@ -68,9 +70,10 @@ func (suite *ItemUnitSuite) TestFileErr() {
 func (suite *ItemUnitSuite) TestOwnerErr() {
 	t := suite.T()
 	addtl := map[string]any{"foo": "bar"}
-	i := OwnerErr(clues.New("foo"), "id", "name", addtl)
+	i := OwnerErr(clues.New("foo"), "ns", "id", "name", addtl)
 
 	expect := Item{
+		Namespace:  "ns",
 		ID:         "id",
 		Name:       "name",
 		Type:       ResourceOwnerType,
@@ -127,17 +130,17 @@ func (suite *ItemUnitSuite) TestItem_HeadersValues() {
 	}{
 		{
 			name:   "file",
-			item:   FileErr(assert.AnError, "id", "name", addtl),
+			item:   FileErr(assert.AnError, "ns", "id", "name", addtl),
 			expect: []string{"Error", FileType.Printable(), "name", "cname", cause},
 		},
 		{
 			name:   "container",
-			item:   ContainerErr(assert.AnError, "id", "name", addtl),
+			item:   ContainerErr(assert.AnError, "ns", "id", "name", addtl),
 			expect: []string{"Error", ContainerType.Printable(), "name", "cname", cause},
 		},
 		{
 			name:   "owner",
-			item:   OwnerErr(assert.AnError, "id", "name", nil),
+			item:   OwnerErr(assert.AnError, "ns", "id", "name", nil),
 			expect: []string{"Error", ResourceOwnerType.Printable(), "name", "", cause},
 		},
 	}
@@ -169,9 +172,10 @@ func (suite *ItemUnitSuite) TestSkipped_String() {
 func (suite *ItemUnitSuite) TestContainerSkip() {
 	t := suite.T()
 	addtl := map[string]any{"foo": "bar"}
-	i := ContainerSkip(SkipMalware, "id", "name", addtl)
+	i := ContainerSkip(SkipMalware, "ns", "id", "name", addtl)
 
 	expect := Item{
+		Namespace:  "ns",
 		ID:         "id",
 		Name:       "name",
 		Type:       ContainerType,
@@ -185,9 +189,10 @@ func (suite *ItemUnitSuite) TestContainerSkip() {
 func (suite *ItemUnitSuite) TestFileSkip() {
 	t := suite.T()
 	addtl := map[string]any{"foo": "bar"}
-	i := FileSkip(SkipMalware, "id", "name", addtl)
+	i := FileSkip(SkipMalware, "ns", "id", "name", addtl)
 
 	expect := Item{
+		Namespace:  "ns",
 		ID:         "id",
 		Name:       "name",
 		Type:       FileType,
@@ -201,9 +206,10 @@ func (suite *ItemUnitSuite) TestFileSkip() {
 func (suite *ItemUnitSuite) TestOwnerSkip() {
 	t := suite.T()
 	addtl := map[string]any{"foo": "bar"}
-	i := OwnerSkip(SkipMalware, "id", "name", addtl)
+	i := OwnerSkip(SkipMalware, "ns", "id", "name", addtl)
 
 	expect := Item{
+		Namespace:  "ns",
 		ID:         "id",
 		Name:       "name",
 		Type:       ResourceOwnerType,
@@ -227,17 +233,17 @@ func (suite *ItemUnitSuite) TestSkipped_HeadersValues() {
 	}{
 		{
 			name:   "file",
-			skip:   FileSkip(SkipMalware, "id", "name", addtl),
+			skip:   FileSkip(SkipMalware, "ns", "id", "name", addtl),
 			expect: []string{"Skip", FileType.Printable(), "name", "cname", string(SkipMalware)},
 		},
 		{
 			name:   "container",
-			skip:   ContainerSkip(SkipMalware, "id", "name", addtl),
+			skip:   ContainerSkip(SkipMalware, "ns", "id", "name", addtl),
 			expect: []string{"Skip", ContainerType.Printable(), "name", "cname", string(SkipMalware)},
 		},
 		{
 			name:   "owner",
-			skip:   OwnerSkip(SkipMalware, "id", "name", nil),
+			skip:   OwnerSkip(SkipMalware, "ns", "id", "name", nil),
 			expect: []string{"Skip", ResourceOwnerType.Printable(), "name", "", string(SkipMalware)},
 		},
 	}

src/pkg/fault/testdata/testdata.go

@@ -19,7 +19,7 @@ func MakeErrors(failure, recovered, skipped bool) fault.Errors {
 	}
 
 	if skipped {
-		fe.Skipped = []fault.Skipped{*fault.FileSkip(fault.SkipMalware, "id", "name", nil)}
+		fe.Skipped = []fault.Skipped{*fault.FileSkip(fault.SkipMalware, "ns", "id", "name", nil)}
 	}
 
 	return fe

src/pkg/repository/repository_unexported_test.go

@@ -434,8 +434,8 @@ func (suite *RepositoryModelIntgSuite) TestGetBackupErrors() {
 	var (
 		err  = clues.Wrap(assert.AnError, "wrap")
 		cec  = err.Core()
-		item = fault.FileErr(err, "file-id", "file-name", map[string]any{"foo": "bar"})
+		item = fault.FileErr(err, "ns", "file-id", "file-name", map[string]any{"foo": "bar"})
-		skip = fault.FileSkip(fault.SkipMalware, "s-file-id", "s-file-name", map[string]any{"foo": "bar"})
+		skip = fault.FileSkip(fault.SkipMalware, "ns", "s-file-id", "s-file-name", map[string]any{"foo": "bar"})
 		info = details.ItemInfo{
 			Exchange: &details.ExchangeInfo{
 				ItemType: details.ExchangeMail,