From abc5ec19420ed0c65050e4a372513c4e2de7f2d9 Mon Sep 17 00:00:00 2001
From: Abhishek Pandey <abhishek@alcion.ai>
Date: Tue, 26 Dec 2023 21:30:57 -0800
Subject: [PATCH] Skip group mailbox items that were deleted in flight (#4939)

<!-- PR description-->

Discovered an issue with how we were handling items deleted in flight with the lazy item reader. This needs to go in before integration PR. https://github.com/alcionai/corso/pull/4921

Here are the details.

**Orig:**
* For such items, we were using the exchange approach to persist an empty data file (with serialized version) and not persist it in details file.
* Exchange discovers the deleted items during next backup during delta query and excludes them.

**Problem:**
* Group mailbox doesn't support delta queries. Also, there is no graph API to discover/fetch deleted items.
* So if we use the exchange lazy reader approach like above, the ongoing backup will succeed. However, the next incremental backup fails during details merge with ` running backup: merging details: incomplete migration of backup details`.
* Failure is [here](https://github.com/alcionai/corso/blob/c58cd9302fd0e9bd011e7e7ecfac3211d2a7a74b/src/internal/operations/backup.go#L837). It's because the deleted item reporefs are still present in the merge base. Since there is no delta query, there is no `GetDeleted()` to exclude the item.

**Mod:**
* Using the approach drive lazy reader code uses to mark an item as skipped, so that kopia doesn't error out during upload.

---

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

- [ ] :white_check_mark: Yes, it's included
- [ ] :clock1: Yes, but in a later PR
- [x] :no_entry: No

#### Type of change

<!--- Please check the type of change your PR introduces: --->
- [ ] :sunflower: Feature
- [x] :bug: Bugfix
- [ ] :world_map: Documentation
- [ ] :robot: Supportability/Tests
- [ ] :computer: CI/Deployment
- [ ] :broom: Tech Debt/Cleanup

#### Issue(s)

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

#### Test Plan

<!-- How will this be tested prior to merging.-->
- [x] :muscle: Manual
- [x] :zap: Unit test
- [ ] :green_heart: E2E
---
 .../m365/collection/groups/collection.go       | 18 +++++++++---------
 .../m365/collection/groups/collection_test.go  | 15 ++-------------
 2 files changed, 11 insertions(+), 22 deletions(-)

diff --git a/src/internal/m365/collection/groups/collection.go b/src/internal/m365/collection/groups/collection.go
index e24b46859..f861010eb 100644
--- a/src/internal/m365/collection/groups/collection.go
+++ b/src/internal/m365/collection/groups/collection.go
@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"context"
 	"io"
+	"net/http"
 	"sync"
 	"sync/atomic"
 	"time"
@@ -402,15 +403,14 @@ func (lig *lazyItemGetter[C, I]) GetData(
 		lig.containerIDs,
 		lig.itemID)
 	if err != nil {
-		// If an item was deleted then return an empty file so we don't fail
-		// the backup. Also return delInFlight as true so that kopia skips
-		// adding ItemInfo to details.
-		//
-		// The item will be deleted from kopia on the next backup when the
-		// delta token shows it's removed.
-		if graph.IsErrDeletedInFlight(err) {
-			logger.CtxErr(ctx, err).Info("item not found")
-			return nil, nil, true, nil
+		// For items that were deleted in flight, add the skip label so that
+		// they don't lead to recoverable failures during backup.
+		if clues.HasLabel(err, graph.LabelStatus(http.StatusNotFound)) || graph.IsErrDeletedInFlight(err) {
+			logger.CtxErr(ctx, err).Info("item deleted in flight. skipping")
+
+			// Returning delInFlight as true here for correctness, although the caller is going
+			// to ignore it since we are returning an error.
+			return nil, nil, true, clues.Wrap(err, "deleted item").Label(graph.LabelsSkippable)
 		}
 
 		err = clues.WrapWC(ctx, err, "getting item data").Label(fault.LabelForceNoBackupCreation)
diff --git a/src/internal/m365/collection/groups/collection_test.go b/src/internal/m365/collection/groups/collection_test.go
index 7a7bda7f6..69f30a864 100644
--- a/src/internal/m365/collection/groups/collection_test.go
+++ b/src/internal/m365/collection/groups/collection_test.go
@@ -551,19 +551,8 @@ func (suite *CollectionUnitSuite) TestLazyItem_ReturnsEmptyReaderOnDeletedInFlig
 		li.ModTime(),
 		"item mod time")
 
-	r, err := readers.NewVersionedRestoreReader(li.ToReader())
-	require.NoError(t, err, clues.ToCore(err))
-
-	assert.Equal(t, readers.DefaultSerializationVersion, r.Format().Version)
-	assert.True(t, r.Format().DelInFlight)
-
-	readData, err := io.ReadAll(r)
-	assert.NoError(t, err, "reading item data: %v", clues.ToCore(err))
-
-	assert.Empty(t, readData, "read item data")
-
-	_, err = li.Info()
-	assert.ErrorIs(t, err, data.ErrNotFound, "Info() error")
+	_, err := readers.NewVersionedRestoreReader(li.ToReader())
+	assert.ErrorIs(t, err, graph.ErrDeletedInFlight, "item should be marked deleted in flight")
 }
 
 func (suite *CollectionUnitSuite) TestLazyItem() {