From ff0744b3af3c4697cecb6dd6c664eeae64d46839 Mon Sep 17 00:00:00 2001
From: Michael Eischer <michael.eischer@fau.de>
Date: Thu, 9 May 2024 22:12:53 +0200
Subject: [PATCH] check: test checkPack retries

---
 internal/checker/checker.go      | 24 +++++---
 internal/checker/checker_test.go | 98 ++++++++++++++++++++++++--------
 2 files changed, 89 insertions(+), 33 deletions(-)

diff --git a/internal/checker/checker.go b/internal/checker/checker.go
index 1ae6b23f1..1cee4355c 100644
--- a/internal/checker/checker.go
+++ b/internal/checker/checker.go
@@ -532,6 +532,21 @@ func (e *partialReadError) Error() string {
 
 // checkPack reads a pack and checks the integrity of all blobs.
 func checkPack(ctx context.Context, r restic.Repository, id restic.ID, blobs []restic.Blob, size int64, bufRd *bufio.Reader, dec *zstd.Decoder) error {
+	err := checkPackInner(ctx, r, id, blobs, size, bufRd, dec)
+	if err != nil {
+		// retry pack verification to detect transient errors
+		err2 := checkPackInner(ctx, r, id, blobs, size, bufRd, dec)
+		if err2 != nil {
+			err = err2
+		} else {
+			err = fmt.Errorf("check successful on second attempt, original error %w", err)
+		}
+	}
+	return err
+}
+
+func checkPackInner(ctx context.Context, r restic.Repository, id restic.ID, blobs []restic.Blob, size int64, bufRd *bufio.Reader, dec *zstd.Decoder) error {
+
 	debug.Log("checking pack %v", id.String())
 
 	if len(blobs) == 0 {
@@ -725,15 +740,6 @@ func (c *Checker) ReadPacks(ctx context.Context, packs map[restic.ID]int64, p *p
 				}
 
 				err := checkPack(ctx, c.repo, ps.id, ps.blobs, ps.size, bufRd, dec)
-				if err != nil {
-					// retry pack verification to detect transient errors
-					err2 := checkPack(ctx, c.repo, ps.id, ps.blobs, ps.size, bufRd, dec)
-					if err2 != nil {
-						err = err2
-					} else {
-						err = fmt.Errorf("second check successful, original error %w", err)
-					}
-				}
 
 				p.Add(1)
 				if err == nil {
diff --git a/internal/checker/checker_test.go b/internal/checker/checker_test.go
index 9746e9f5d..5fc82eed0 100644
--- a/internal/checker/checker_test.go
+++ b/internal/checker/checker_test.go
@@ -8,6 +8,7 @@ import (
 	"path/filepath"
 	"sort"
 	"strconv"
+	"strings"
 	"sync"
 	"testing"
 	"time"
@@ -325,42 +326,91 @@ func induceError(data []byte) {
 	data[pos] ^= 1
 }
 
+// errorOnceBackend randomly modifies data when reading a file for the first time.
+type errorOnceBackend struct {
+	backend.Backend
+	m sync.Map
+}
+
+func (b *errorOnceBackend) Load(ctx context.Context, h backend.Handle, length int, offset int64, consumer func(rd io.Reader) error) error {
+	_, isRetry := b.m.Swap(h, struct{}{})
+	return b.Backend.Load(ctx, h, length, offset, func(rd io.Reader) error {
+		if !isRetry && h.Type != restic.ConfigFile {
+			return consumer(errorReadCloser{rd})
+		}
+		return consumer(rd)
+	})
+}
+
 func TestCheckerModifiedData(t *testing.T) {
 	repo := repository.TestRepository(t)
 	sn := archiver.TestSnapshot(t, repo, ".", nil)
 	t.Logf("archived as %v", sn.ID().Str())
 
-	beError := &errorBackend{Backend: repo.Backend()}
-	checkRepo := repository.TestOpenBackend(t, beError)
+	errBe := &errorBackend{Backend: repo.Backend()}
 
-	chkr := checker.New(checkRepo, false)
+	for _, test := range []struct {
+		name   string
+		be     backend.Backend
+		damage func()
+		check  func(t *testing.T, err error)
+	}{
+		{
+			"errorBackend",
+			errBe,
+			func() {
+				errBe.ProduceErrors = true
+			},
+			func(t *testing.T, err error) {
+				if err == nil {
+					t.Fatal("no error found, checker is broken")
+				}
+			},
+		},
+		{
+			"errorOnceBackend",
+			&errorOnceBackend{Backend: repo.Backend()},
+			func() {},
+			func(t *testing.T, err error) {
+				if !strings.Contains(err.Error(), "check successful on second attempt, original error pack") {
+					t.Fatalf("wrong error found, got %v", err)
+				}
+			},
+		},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			checkRepo := repository.TestOpenBackend(t, test.be)
 
-	hints, errs := chkr.LoadIndex(context.TODO(), nil)
-	if len(errs) > 0 {
-		t.Fatalf("expected no errors, got %v: %v", len(errs), errs)
-	}
+			chkr := checker.New(checkRepo, false)
 
-	if len(hints) > 0 {
-		t.Errorf("expected no hints, got %v: %v", len(hints), hints)
-	}
+			hints, errs := chkr.LoadIndex(context.TODO(), nil)
+			if len(errs) > 0 {
+				t.Fatalf("expected no errors, got %v: %v", len(errs), errs)
+			}
 
-	beError.ProduceErrors = true
-	errFound := false
-	for _, err := range checkPacks(chkr) {
-		t.Logf("pack error: %v", err)
-	}
+			if len(hints) > 0 {
+				t.Errorf("expected no hints, got %v: %v", len(hints), hints)
+			}
 
-	for _, err := range checkStruct(chkr) {
-		t.Logf("struct error: %v", err)
-	}
+			test.damage()
+			var err error
+			for _, err := range checkPacks(chkr) {
+				t.Logf("pack error: %v", err)
+			}
 
-	for _, err := range checkData(chkr) {
-		t.Logf("data error: %v", err)
-		errFound = true
-	}
+			for _, err := range checkStruct(chkr) {
+				t.Logf("struct error: %v", err)
+			}
 
-	if !errFound {
-		t.Fatal("no error found, checker is broken")
+			for _, cerr := range checkData(chkr) {
+				t.Logf("data error: %v", cerr)
+				if err == nil {
+					err = cerr
+				}
+			}
+
+			test.check(t, err)
+		})
 	}
 }