Merge pull request #3449 from MichaelEischer/fix-restore-retries

restore: Correctly handle partial pack download errors

changelog/unreleased/pull-3449

@@ -0,0 +1,8 @@
+Bugfix: Correctly handle download errors during `restore`
+
+Due to a regression in restic 0.12.0 the `restore` command in some cases
+did not retry download errors and only printed a warning. This has been
+fixed by retrying incomplete data downloads.
+
+https://github.com/restic/restic/issues/3439
+https://github.com/restic/restic/pull/3449

internal/restorer/filerestorer.go

@@ -258,7 +258,11 @@ func (r *fileRestorer) downloadPack(ctx context.Context, pack *packInfo) error {
 			if err != nil {
 				return err
 			}
-			blobData, buf, err = r.loadBlob(bufRd, blobID, blob.length, buf)
+			buf, err = r.downloadBlob(bufRd, blobID, blob.length, buf)
+			if err != nil {
+				return err
+			}
+			blobData, err = r.decryptBlob(blobID, buf)
 			if err != nil {
 				for file := range blob.files {
 					if errFile := sanitizeError(file, err); errFile != nil {
@@ -309,7 +313,7 @@ func (r *fileRestorer) downloadPack(ctx context.Context, pack *packInfo) error {
 	return nil
 }
 
-func (r *fileRestorer) loadBlob(rd io.Reader, blobID restic.ID, length int, buf []byte) ([]byte, []byte, error) {
+func (r *fileRestorer) downloadBlob(rd io.Reader, blobID restic.ID, length int, buf []byte) ([]byte, error) {
 	// TODO reconcile with Repository#loadBlob implementation
 
 	if cap(buf) < length {
@@ -320,24 +324,29 @@ func (r *fileRestorer) loadBlob(rd io.Reader, blobID restic.ID, length int, buf
 
 	n, err := io.ReadFull(rd, buf)
 	if err != nil {
-		return nil, nil, err
+		return nil, err
 	}
 
 	if n != length {
-		return nil, nil, errors.Errorf("error loading blob %v: wrong length returned, want %d, got %d", blobID.Str(), length, n)
+		return nil, errors.Errorf("error loading blob %v: wrong length returned, want %d, got %d", blobID.Str(), length, n)
 	}
+	return buf, nil
+}
+
+func (r *fileRestorer) decryptBlob(blobID restic.ID, buf []byte) ([]byte, error) {
+	// TODO reconcile with Repository#loadBlob implementation
 
 	// decrypt
 	nonce, ciphertext := buf[:r.key.NonceSize()], buf[r.key.NonceSize():]
 	plaintext, err := r.key.Open(ciphertext[:0], nonce, ciphertext, nil)
 	if err != nil {
-		return nil, nil, errors.Errorf("decrypting blob %v failed: %v", blobID, err)
+		return nil, errors.Errorf("decrypting blob %v failed: %v", blobID, err)
 	}
 
 	// check hash
 	if !restic.Hash(plaintext).Equal(blobID) {
-		return nil, nil, errors.Errorf("blob %v returned invalid hash", blobID)
+		return nil, errors.Errorf("blob %v returned invalid hash", blobID)
 	}
 
-	return plaintext, buf, nil
+	return plaintext, nil
 }

internal/restorer/filerestorer_test.go

@@ -3,6 +3,7 @@ package restorer
 import (
 	"bytes"
 	"context"
+	"fmt"
 	"io"
 	"io/ioutil"
 	"testing"
@@ -163,6 +164,10 @@ func restoreAndVerify(t *testing.T, tempdir string, content []TestFile, files ma
 	err := r.restoreFiles(context.TODO())
 	rtest.OK(t, err)
 
+	verifyRestore(t, r, repo)
+}
+
+func verifyRestore(t *testing.T, r *fileRestorer, repo *TestRepo) {
 	for _, file := range r.files {
 		target := r.targetPath(file.location)
 		data, err := ioutil.ReadFile(target)
@@ -264,3 +269,49 @@ func TestErrorRestoreFiles(t *testing.T) {
 	err := r.restoreFiles(context.TODO())
 	rtest.Equals(t, loadError, err)
 }
+
+func TestDownloadError(t *testing.T) {
+	for i := 0; i < 100; i += 10 {
+		testPartialDownloadError(t, i)
+	}
+}
+
+func testPartialDownloadError(t *testing.T, part int) {
+	tempdir, cleanup := rtest.TempDir(t)
+	defer cleanup()
+	content := []TestFile{
+		{
+			name: "file1",
+			blobs: []TestBlob{
+				{"data1-1", "pack1"},
+				{"data1-2", "pack1"},
+				{"data1-3", "pack1"},
+			},
+		}}
+
+	repo := newTestRepo(content)
+
+	// loader always returns an error
+	loader := repo.loader
+	repo.loader = func(ctx context.Context, h restic.Handle, length int, offset int64, fn func(rd io.Reader) error) error {
+		// only load partial data to execise fault handling in different places
+		err := loader(ctx, h, length*part/100, offset, fn)
+		if err == nil {
+			return nil
+		}
+		fmt.Println("Retry after error", err)
+		return loader(ctx, h, length, offset, fn)
+	}
+
+	r := newFileRestorer(tempdir, repo.loader, repo.key, repo.Lookup)
+	r.files = repo.files
+	r.Error = func(s string, e error) error {
+		// ignore errors as in the `restore` command
+		fmt.Println("error during restore", s, e)
+		return nil
+	}
+
+	err := r.restoreFiles(context.TODO())
+	rtest.OK(t, err)
+	verifyRestore(t, r, repo)
+}