From bf81bf079586a626a5233ed2099b87aab69aa7e6 Mon Sep 17 00:00:00 2001
From: Michael Eischer <michael.eischer@fau.de>
Date: Sun, 5 Jun 2022 21:57:16 +0200
Subject: [PATCH] repository: Properly set id for finalized index

As MergeFinalIndex and index uploads can occur concurrently, it is
necessary for MergeFinalIndex to check whether the IDs for an index were
already set before merging it. Otherwise, we'd loose the ID of an index
which is set _after_ uploading it.
---
 internal/repository/master_index.go      | 12 ++++++++++--
 internal/repository/master_index_test.go | 22 ++++------------------
 internal/repository/repository.go        |  6 ------
 internal/repository/testing.go           | 10 ++++++++++
 4 files changed, 24 insertions(+), 26 deletions(-)

diff --git a/internal/repository/master_index.go b/internal/repository/master_index.go
index 4fdd4a0ce..660fb40aa 100644
--- a/internal/repository/master_index.go
+++ b/internal/repository/master_index.go
@@ -287,7 +287,9 @@ func (mi *MasterIndex) MergeFinalIndexes() error {
 		idx := mi.idx[i]
 		// clear reference in masterindex as it may become stale
 		mi.idx[i] = nil
-		if !idx.Final() {
+		// do not merge indexes that have no id set
+		ids, _ := idx.IDs()
+		if !idx.Final() || len(ids) == 0 {
 			newIdx = append(newIdx, idx)
 		} else {
 			err := mi.idx[0].merge(idx)
@@ -404,7 +406,13 @@ func SaveIndex(ctx context.Context, repo restic.SaverUnpacked, index *Index) (re
 		return restic.ID{}, err
 	}
 
-	return repo.SaveUnpacked(ctx, restic.IndexFile, buf.Bytes())
+	id, err := repo.SaveUnpacked(ctx, restic.IndexFile, buf.Bytes())
+	ierr := index.SetID(id)
+	if ierr != nil {
+		// logic bug
+		panic(ierr)
+	}
+	return id, err
 }
 
 // saveIndex saves all indexes in the backend.
diff --git a/internal/repository/master_index_test.go b/internal/repository/master_index_test.go
index 0aeaf5eb1..ffdaf7af9 100644
--- a/internal/repository/master_index_test.go
+++ b/internal/repository/master_index_test.go
@@ -157,16 +157,9 @@ func TestMasterMergeFinalIndexes(t *testing.T) {
 	mIdx.Insert(idx1)
 	mIdx.Insert(idx2)
 
-	finalIndexes := mIdx.FinalizeNotFinalIndexes()
+	finalIndexes, idxCount := repository.TestMergeIndex(t, mIdx)
 	rtest.Equals(t, []*repository.Index{idx1, idx2}, finalIndexes)
-
-	err := mIdx.MergeFinalIndexes()
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	allIndexes := mIdx.All()
-	rtest.Equals(t, 1, len(allIndexes))
+	rtest.Equals(t, 1, idxCount)
 
 	blobCount := 0
 	for range mIdx.Each(context.TODO()) {
@@ -189,16 +182,9 @@ func TestMasterMergeFinalIndexes(t *testing.T) {
 	idx3.StorePack(blob2.PackID, []restic.Blob{blob2.Blob})
 
 	mIdx.Insert(idx3)
-	finalIndexes = mIdx.FinalizeNotFinalIndexes()
+	finalIndexes, idxCount = repository.TestMergeIndex(t, mIdx)
 	rtest.Equals(t, []*repository.Index{idx3}, finalIndexes)
-
-	err = mIdx.MergeFinalIndexes()
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	allIndexes = mIdx.All()
-	rtest.Equals(t, 1, len(allIndexes))
+	rtest.Equals(t, 1, idxCount)
 
 	// Index should have same entries as before!
 	blobs = mIdx.Lookup(bhInIdx1)
diff --git a/internal/repository/repository.go b/internal/repository/repository.go
index 77cf01989..01a7cad8a 100644
--- a/internal/repository/repository.go
+++ b/internal/repository/repository.go
@@ -582,12 +582,6 @@ func (r *Repository) LoadIndex(ctx context.Context) error {
 		if err != nil {
 			return err
 		}
-
-		_, err = idx.IDs()
-		if err != nil {
-			return err
-		}
-
 		r.idx.Insert(idx)
 		return nil
 	})
diff --git a/internal/repository/testing.go b/internal/repository/testing.go
index 05dfab64d..a24148a1b 100644
--- a/internal/repository/testing.go
+++ b/internal/repository/testing.go
@@ -132,3 +132,13 @@ func BenchmarkAllVersions(b *testing.B, bench VersionedBenchmark) {
 		})
 	}
 }
+
+func TestMergeIndex(t testing.TB, mi *MasterIndex) ([]*Index, int) {
+	finalIndexes := mi.FinalizeNotFinalIndexes()
+	for _, idx := range finalIndexes {
+		test.OK(t, idx.SetID(restic.NewRandomID()))
+	}
+
+	test.OK(t, mi.MergeFinalIndexes())
+	return finalIndexes, len(mi.idx)
+}