Merge pull request #5495 from MichaelEischer/fix-check-retries

check: fix error reporting on download retry

changelog/unreleased/issue-5467

@@ -0,0 +1,27 @@
+Bugfix: Improve handling of download retries in `check` command
+
+In very rare cases, the `check` command could unnecessarily report a
+repository damage if the backend returns incomplete, corrupted data on
+the first download try which is resolved by a download retry.
+
+This could result in an error output like the following:
+```
+Load(<data/34567890ab>, 33918928, 0) returned error, retrying after 871.35598ms: readFull: unexpected EOF
+Load(<data/34567890ab>, 33918928, 0) operation successful after 1 retries
+check successful on second attempt, original error pack 34567890ab[...] contains 6 errors: [blob 12345678[...]: decrypting blob <data/12345678> from 34567890 failed: ciphertext verification failed ...]
+[...]
+Fatal: repository contains errors
+```
+
+This fix only applies to a very specific case. The required condition is
+a `operation successful after 1 retries` error in the log, later followed by
+a `check successful on second attempt, original error` error that only reports
+`ciphertext verification failed` errors in the pack file. If any other errors
+are reported in the pack file, then the repository still has to be considered
+as damaged.
+
+Now, only the check result of the last download retry is reported as
+intended.
+
+https://github.com/restic/restic/issues/5467
+https://github.com/restic/restic/pull/5495

internal/checker/checker_test.go

@@ -293,7 +293,7 @@ type errorBackend struct {
 func (b errorBackend) Load(ctx context.Context, h backend.Handle, length int, offset int64, consumer func(rd io.Reader) error) error {
 	return b.Backend.Load(ctx, h, length, offset, func(rd io.Reader) error {
 		if b.ProduceErrors {
-			return consumer(errorReadCloser{rd})
+			return consumer(errorReadCloser{Reader: rd})
 		}
 		return consumer(rd)
 	})
@@ -301,12 +301,21 @@ func (b errorBackend) Load(ctx context.Context, h backend.Handle, length int, of
 
 type errorReadCloser struct {
 	io.Reader
+	shortenBy      int
+	maxErrorOffset int // if 0, the error can be injected at any offset
 }
 
 func (erd errorReadCloser) Read(p []byte) (int, error) {
 	n, err := erd.Reader.Read(p)
 	if n > 0 {
-		induceError(p[:n])
+		maxOffset := n
+		if erd.maxErrorOffset > 0 {
+			maxOffset = min(erd.maxErrorOffset, maxOffset)
+		}
+		induceError(p[:maxOffset])
+	}
+	if n > erd.shortenBy {
+		n -= erd.shortenBy
 	}
 	return n, err
 }
@@ -320,17 +329,26 @@ func induceError(data []byte) {
 // errorOnceBackend randomly modifies data when reading a file for the first time.
 type errorOnceBackend struct {
 	backend.Backend
-	m sync.Map
+	m              sync.Map
+	shortenBy      int
+	maxErrorOffset int
 }
 
 func (b *errorOnceBackend) Load(ctx context.Context, h backend.Handle, length int, offset int64, consumer func(rd io.Reader) error) error {
 	_, isRetry := b.m.LoadOrStore(h, struct{}{})
-	return b.Backend.Load(ctx, h, length, offset, func(rd io.Reader) error {
+	err := b.Backend.Load(ctx, h, length, offset, func(rd io.Reader) error {
 		if !isRetry && h.Type != restic.ConfigFile {
-			return consumer(errorReadCloser{rd})
+			return consumer(errorReadCloser{Reader: rd, shortenBy: b.shortenBy, maxErrorOffset: b.maxErrorOffset})
 		}
 		return consumer(rd)
 	})
+	if err == nil {
+		return nil
+	}
+	// retry if the consumer returned an error
+	return b.Backend.Load(ctx, h, length, offset, func(rd io.Reader) error {
+		return consumer(rd)
+	})
 }
 
 func TestCheckerModifiedData(t *testing.T) {
@@ -368,6 +386,15 @@ func TestCheckerModifiedData(t *testing.T) {
 				}
 			},
 		},
+		{
+			// ignore if a backend returns incomplete garbled data on the first try
+			"corruptPartialOnceBackend",
+			&errorOnceBackend{Backend: be, shortenBy: 10, maxErrorOffset: 100},
+			func() {},
+			func(t *testing.T, err error) {
+				test.Assert(t, err == nil, "unexpected error found, got %v", err)
+			},
+		},
 	} {
 		t.Run(test.name, func(t *testing.T) {
 			checkRepo := repository.TestOpenBackend(t, test.be)

internal/repository/check.go

@@ -88,10 +88,14 @@ func checkPackInner(ctx context.Context, r *Repository, id restic.ID, blobs []re
 	// calculate hash on-the-fly while reading the pack and capture pack header
 	var hash restic.ID
 	var hdrBuf []byte
+	// must use a separate slice from `errs` here as we're only interested in the last retry
+	var blobErrors []error
 	h := backend.Handle{Type: backend.PackFile, Name: id.String()}
 	err := r.be.Load(ctx, h, int(size), 0, func(rd io.Reader) error {
 		hrd := hashing.NewReader(rd, sha256.New())
 		bufRd.Reset(hrd)
+		// reset blob errors for each retry
+		blobErrors = nil
 
 		it := newPackBlobIterator(id, newBufReader(bufRd), 0, blobs, r.Key(), dec)
 		for {
@@ -108,7 +112,7 @@ func checkPackInner(ctx context.Context, r *Repository, id restic.ID, blobs []re
 			debug.Log("  check blob %v: %v", val.Handle.ID, val.Handle)
 			if val.Err != nil {
 				debug.Log("  error verifying blob %v: %v", val.Handle.ID, val.Err)
-				errs = append(errs, errors.Errorf("blob %v: %v", val.Handle.ID, val.Err))
+				blobErrors = append(blobErrors, errors.Errorf("blob %v: %v", val.Handle.ID, val.Err))
 			}
 		}
 
@@ -134,6 +138,7 @@ func checkPackInner(ctx context.Context, r *Repository, id restic.ID, blobs []re
 		hash = restic.IDFromHash(hrd.Sum(nil))
 		return nil
 	})
+	errs = append(errs, blobErrors...)
 	if err != nil {
 		var e *partialReadError
 		isPartialReadError := errors.As(err, &e)