From d0093fdb173caa241568dd199c7aa8773ee8061f Mon Sep 17 00:00:00 2001
From: Keepers <ryanfkeepers@gmail.com>
Date: Thu, 22 Dec 2022 17:20:16 -0700
Subject: [PATCH] track tombstones by id, not path (#1917)

## Description

Fixes a bug where we could incorrectly mark a previousPath as both
deleted and moved/created at the same time.

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

- [x] :no_entry: No

## Type of change

- [x] :bug: Bugfix

## Issue(s)

* #1780

## Test Plan

- [x] :green_heart: E2E
---
 .../connector/exchange/service_iterators.go   | 32 +++++++++++--------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/src/internal/connector/exchange/service_iterators.go b/src/internal/connector/exchange/service_iterators.go
index b9e04aa6c..dcec1a6dd 100644
--- a/src/internal/connector/exchange/service_iterators.go
+++ b/src/internal/connector/exchange/service_iterators.go
@@ -72,13 +72,9 @@ func filterContainersAndFillCollections(
 		}
 
 		cID := *c.GetId()
+		delete(tombstones, cID)
 
 		currPath, ok := includeContainer(qp, c, scope)
-		if currPath != nil {
-			// this path exists, do not delete it, even if it isn't
-			// included in this backup.
-			delete(tombstones, currPath.String())
-		}
 		// Only create a collection if the path matches the scope.
 		if !ok {
 			continue
@@ -107,7 +103,7 @@ func filterContainersAndFillCollections(
 			if graph.IsErrDeletedInFlight(err) == nil {
 				errs = support.WrapAndAppend(qp.ResourceOwner, err, errs)
 			} else {
-				tombstones[currPath.String()] = struct{}{}
+				tombstones[cID] = dp.path
 			}
 
 			continue
@@ -143,16 +139,24 @@ func filterContainersAndFillCollections(
 	// to the same container (ex: container_1 gets deleted, then container_2
 	// gets created with the same name), it is assumed that the backup consumer
 	// processes deletions before creations, making the combined operation safe.
-	for p := range tombstones {
+	for id, p := range tombstones {
 		service, err := createService(qp.Credentials)
 		if err != nil {
 			errs = support.WrapAndAppend(qp.ResourceOwner, err, errs)
 			continue
 		}
 
+		// only occurs if it was a new folder that we picked up during the container
+		// resolver phase that got deleted in flight by the time we hit this stage.
+		if len(p) == 0 {
+			continue
+		}
+
 		prevPath, err := pathFromPrevString(p)
 		if err != nil {
-			logger.Ctx(ctx).Error(err)
+			// technically shouldn't ever happen.  But just in case, we need to catch
+			// it for protection.
+			logger.Ctx(ctx).Errorw("parsing tombstone path", "err", err)
 			continue
 		}
 
@@ -166,7 +170,7 @@ func filterContainersAndFillCollections(
 			ctrlOpts,
 			false,
 		)
-		collections[p] = &edc
+		collections[id] = &edc
 	}
 
 	entries := []graph.MetadataCollectionEntry{
@@ -193,14 +197,14 @@ func filterContainersAndFillCollections(
 	return errs
 }
 
-// produces a set keyed by path strings from the deltapaths map.
+// produces a set of id:path pairs from the deltapaths map.
 // Each entry in the set will, if not removed, produce a collection
 // that will delete the tombstone by path.
-func makeTombstones(dps DeltaPaths) map[string]struct{} {
-	r := make(map[string]struct{}, len(dps))
+func makeTombstones(dps DeltaPaths) map[string]string {
+	r := make(map[string]string, len(dps))
 
-	for _, v := range dps {
-		r[v.path] = struct{}{}
+	for id, v := range dps {
+		r[id] = v.path
 	}
 
 	return r