Merge pull request #2068 from restic/prune-check-load-error

prune: Correctly handle errors returned by Load()

changelog/unreleased/pull-2068

@@ -0,0 +1,6 @@
+Bugfix: Correctly return error loading data
+
+In one case during `prune` and `check`, an error loading data from the backend is not returned properly. This is now corrected.
+
+https://github.com/restic/restic/pull/2068
+https://github.com/restic/restic/issues/1999#issuecomment-433737921

internal/checker/checker.go

@@ -7,13 +7,12 @@ import (
 	"os"
 	"sync"
 
-	"github.com/restic/restic/internal/errors"
-	"github.com/restic/restic/internal/restic"
-	"golang.org/x/sync/errgroup"
-
 	"github.com/restic/restic/internal/debug"
+	"github.com/restic/restic/internal/errors"
 	"github.com/restic/restic/internal/pack"
 	"github.com/restic/restic/internal/repository"
+	"github.com/restic/restic/internal/restic"
+	"golang.org/x/sync/errgroup"
 )
 
 // Checker runs various checks on a repository. It is advisable to create an
@@ -652,7 +651,7 @@ func checkPack(ctx context.Context, r restic.Repository, id restic.ID) error {
 	debug.Log("checking pack %v", id)
 	h := restic.Handle{Type: restic.DataFile, Name: id.String()}
 
-	packfile, hash, size, err := repository.DownloadAndHash(ctx, r, h)
+	packfile, hash, size, err := repository.DownloadAndHash(ctx, r.Backend(), h)
 	if err != nil {
 		return errors.Wrap(err, "checkPack")
 	}

internal/repository/repack.go

@@ -6,11 +6,10 @@ import (
 	"os"
 
 	"github.com/restic/restic/internal/debug"
+	"github.com/restic/restic/internal/errors"
 	"github.com/restic/restic/internal/fs"
 	"github.com/restic/restic/internal/pack"
 	"github.com/restic/restic/internal/restic"
-
-	"github.com/restic/restic/internal/errors"
 )
 
 // Repack takes a list of packs together with a list of blobs contained in
@@ -24,7 +23,7 @@ func Repack(ctx context.Context, repo restic.Repository, packs restic.IDSet, kee
 		// load the complete pack into a temp file
 		h := restic.Handle{Type: restic.DataFile, Name: packID.String()}
 
-		tempfile, hash, packLength, err := DownloadAndHash(ctx, repo, h)
+		tempfile, hash, packLength, err := DownloadAndHash(ctx, repo.Backend(), h)
 		if err != nil {
 			return nil, errors.Wrap(err, "Repack")
 		}

internal/repository/repository.go

@@ -9,16 +9,15 @@ import (
 	"io"
 	"os"
 
+	"github.com/restic/restic/internal/backend"
 	"github.com/restic/restic/internal/cache"
+	"github.com/restic/restic/internal/crypto"
+	"github.com/restic/restic/internal/debug"
 	"github.com/restic/restic/internal/errors"
 	"github.com/restic/restic/internal/fs"
 	"github.com/restic/restic/internal/hashing"
-	"github.com/restic/restic/internal/restic"
-
-	"github.com/restic/restic/internal/backend"
-	"github.com/restic/restic/internal/crypto"
-	"github.com/restic/restic/internal/debug"
 	"github.com/restic/restic/internal/pack"
+	"github.com/restic/restic/internal/restic"
 )
 
 // Repository is used to access a repository in a backend.
@@ -694,16 +693,21 @@ func (r *Repository) SaveTree(ctx context.Context, t *restic.Tree) (restic.ID, e
 	return id, err
 }
 
+// Loader allows loading data from a backend.
+type Loader interface {
+	Load(ctx context.Context, h restic.Handle, length int, offset int64, fn func(rd io.Reader) error) error
+}
+
 // DownloadAndHash is all-in-one helper to download content of the file at h to a temporary filesystem location
 // and calculate ID of the contents. Returned (temporary) file is positioned at the beginning of the file;
 // it is reponsibility of the caller to close and delete the file.
-func DownloadAndHash(ctx context.Context, repo restic.Repository, h restic.Handle) (tmpfile *os.File, hash restic.ID, size int64, err error) {
+func DownloadAndHash(ctx context.Context, be Loader, h restic.Handle) (tmpfile *os.File, hash restic.ID, size int64, err error) {
 	tmpfile, err = fs.TempFile("", "restic-temp-")
 	if err != nil {
 		return nil, restic.ID{}, -1, errors.Wrap(err, "TempFile")
 	}
 
-	err = repo.Backend().Load(ctx, h, 0, 0, func(rd io.Reader) (ierr error) {
+	err = be.Load(ctx, h, 0, 0, func(rd io.Reader) (ierr error) {
 		_, ierr = tmpfile.Seek(0, io.SeekStart)
 		if ierr == nil {
 			ierr = tmpfile.Truncate(0)
@@ -716,6 +720,11 @@ func DownloadAndHash(ctx context.Context, repo restic.Repository, h restic.Handl
 		hash = restic.IDFromHash(hrd.Sum(nil))
 		return ierr
 	})
+	if err != nil {
+		tmpfile.Close()
+		os.Remove(tmpfile.Name())
+		return nil, restic.ID{}, -1, errors.Wrap(err, "Load")
+	}
 
 	_, err = tmpfile.Seek(0, io.SeekStart)
 	if err != nil {

internal/repository/repository_test.go

@@ -11,6 +11,8 @@ import (
 	"time"
 
 	"github.com/restic/restic/internal/archiver"
+	"github.com/restic/restic/internal/errors"
+	"github.com/restic/restic/internal/fs"
 	"github.com/restic/restic/internal/repository"
 	"github.com/restic/restic/internal/restic"
 	rtest "github.com/restic/restic/internal/test"
@@ -392,3 +394,107 @@ func TestRepositoryIncrementalIndex(t *testing.T) {
 		}
 	}
 }
+
+type backend struct {
+	rd io.Reader
+}
+
+func (be backend) Load(ctx context.Context, h restic.Handle, length int, offset int64, fn func(rd io.Reader) error) error {
+	return fn(be.rd)
+}
+
+type retryBackend struct {
+	buf []byte
+}
+
+func (be retryBackend) Load(ctx context.Context, h restic.Handle, length int, offset int64, fn func(rd io.Reader) error) error {
+	err := fn(bytes.NewReader(be.buf[:len(be.buf)/2]))
+	if err != nil {
+		return err
+	}
+
+	return fn(bytes.NewReader(be.buf))
+}
+
+func TestDownloadAndHash(t *testing.T) {
+	buf := make([]byte, 5*1024*1024+881)
+	_, err := io.ReadFull(rnd, buf)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	var tests = []struct {
+		be   repository.Loader
+		want []byte
+	}{
+		{
+			be:   backend{rd: bytes.NewReader(buf)},
+			want: buf,
+		},
+		{
+			be:   retryBackend{buf: buf},
+			want: buf,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run("", func(t *testing.T) {
+			f, id, size, err := repository.DownloadAndHash(context.TODO(), test.be, restic.Handle{})
+			if err != nil {
+				t.Error(err)
+			}
+
+			want := restic.Hash(test.want)
+			if !want.Equal(id) {
+				t.Errorf("wrong hash returned, want %v, got %v", want.Str(), id.Str())
+			}
+
+			if size != int64(len(test.want)) {
+				t.Errorf("wrong size returned, want %v, got %v", test.want, size)
+			}
+
+			err = f.Close()
+			if err != nil {
+				t.Error(err)
+			}
+
+			err = fs.RemoveIfExists(f.Name())
+			if err != nil {
+				t.Fatal(err)
+			}
+		})
+	}
+}
+
+type errorReader struct {
+	err error
+}
+
+func (er errorReader) Read(p []byte) (n int, err error) {
+	return 0, er.err
+}
+
+func TestDownloadAndHashErrors(t *testing.T) {
+	var tests = []struct {
+		be  repository.Loader
+		err string
+	}{
+		{
+			be:  backend{rd: errorReader{errors.New("test error 1")}},
+			err: "test error 1",
+		},
+	}
+
+	for _, test := range tests {
+		t.Run("", func(t *testing.T) {
+			_, _, _, err := repository.DownloadAndHash(context.TODO(), test.be, restic.Handle{})
+			if err == nil {
+				t.Fatalf("wanted error %q, got nil", test.err)
+			}
+
+			if errors.Cause(err).Error() != test.err {
+				t.Fatalf("wanted error %q, got %q", test.err, err)
+			}
+		})
+	}
+}