Merge pull request #3170 from greatroar/dont-retry

Don't retry permanent errors in backends

changelog/unreleased/issue-2453

@@ -0,0 +1,12 @@
+Enhancement: Improve handling of permanent backend errors
+
+When encountering errors in reading from or writing to storage backends,
+restic retries the failing operation up to nine times (for a total of ten
+attempts). It used to retry all backend operations, but now detects some
+permanent error conditions so it can report fatal errors earlier.
+
+Permanent failures include local disks being full and SSH connections
+dropping.
+
+https://github.com/restic/restic/issues/2453
+https://github.com/restic/restic/pull/3170

internal/backend/azure/azure.go

@@ -10,11 +10,13 @@ import (
 	"path"
 	"strings"
 
-	"github.com/Azure/azure-sdk-for-go/storage"
 	"github.com/restic/restic/internal/backend"
 	"github.com/restic/restic/internal/debug"
 	"github.com/restic/restic/internal/errors"
 	"github.com/restic/restic/internal/restic"
+
+	"github.com/Azure/azure-sdk-for-go/storage"
+	"github.com/cenkalti/backoff/v4"
 )
 
 // Backend stores data on an azure endpoint.
@@ -119,7 +121,7 @@ func (be *Backend) Path() string {
 // Save stores data in the backend at the handle.
 func (be *Backend) Save(ctx context.Context, h restic.Handle, rd restic.RewindReader) error {
 	if err := h.Valid(); err != nil {
-		return err
+		return backoff.Permanent(err)
 	}
 
 	objName := be.Filename(h)
@@ -219,7 +221,7 @@ func (be *Backend) Load(ctx context.Context, h restic.Handle, length int, offset
 func (be *Backend) openReader(ctx context.Context, h restic.Handle, length int, offset int64) (io.ReadCloser, error) {
 	debug.Log("Load %v, length %v, offset %v from %v", h, length, offset, be.Filename(h))
 	if err := h.Valid(); err != nil {
-		return nil, err
+		return nil, backoff.Permanent(err)
 	}
 
 	if offset < 0 {

internal/backend/b2/b2.go

@@ -11,6 +11,7 @@ import (
 	"github.com/restic/restic/internal/errors"
 	"github.com/restic/restic/internal/restic"
 
+	"github.com/cenkalti/backoff/v4"
 	"github.com/kurin/blazer/b2"
 )
 
@@ -150,7 +151,7 @@ func (be *b2Backend) Load(ctx context.Context, h restic.Handle, length int, offs
 func (be *b2Backend) openReader(ctx context.Context, h restic.Handle, length int, offset int64) (io.ReadCloser, error) {
 	debug.Log("Load %v, length %v, offset %v from %v", h, length, offset, be.Filename(h))
 	if err := h.Valid(); err != nil {
-		return nil, err
+		return nil, backoff.Permanent(err)
 	}
 
 	if offset < 0 {
@@ -189,7 +190,7 @@ func (be *b2Backend) Save(ctx context.Context, h restic.Handle, rd restic.Rewind
 	defer cancel()
 
 	if err := h.Valid(); err != nil {
-		return err
+		return backoff.Permanent(err)
 	}
 
 	be.sem.GetToken()

internal/backend/local/local.go

@@ -13,6 +13,8 @@ import (
 	"github.com/restic/restic/internal/backend"
 	"github.com/restic/restic/internal/debug"
 	"github.com/restic/restic/internal/fs"
+
+	"github.com/cenkalti/backoff/v4"
 )
 
 // Local is a backend in a local directory.
@@ -80,16 +82,24 @@ func (b *Local) IsNotExist(err error) bool {
 }
 
 // Save stores data in the backend at the handle.
-func (b *Local) Save(ctx context.Context, h restic.Handle, rd restic.RewindReader) error {
+func (b *Local) Save(ctx context.Context, h restic.Handle, rd restic.RewindReader) (err error) {
 	debug.Log("Save %v", h)
 	if err := h.Valid(); err != nil {
-		return err
+		return backoff.Permanent(err)
 	}
 
 	filename := b.Filename(h)
 
+	defer func() {
+		// Mark non-retriable errors as such (currently only
+		// "no space left on device").
+		if errors.Is(err, syscall.ENOSPC) {
+			err = backoff.Permanent(err)
+		}
+	}()
+
 	// create new file
-	f, err := fs.OpenFile(filename, os.O_CREATE|os.O_EXCL|os.O_WRONLY, backend.Modes.File)
+	f, err := openFile(filename, os.O_CREATE|os.O_EXCL|os.O_WRONLY, backend.Modes.File)
 
 	if b.IsNotExist(err) {
 		debug.Log("error %v: creating dir", err)
@@ -100,7 +110,7 @@ func (b *Local) Save(ctx context.Context, h restic.Handle, rd restic.RewindReade
 			debug.Log("error creating dir %v: %v", filepath.Dir(filename), mkdirErr)
 		} else {
 			// try again
-			f, err = fs.OpenFile(filename, os.O_CREATE|os.O_EXCL|os.O_WRONLY, backend.Modes.File)
+			f, err = openFile(filename, os.O_CREATE|os.O_EXCL|os.O_WRONLY, backend.Modes.File)
 		}
 	}
 
@@ -141,6 +151,8 @@ func (b *Local) Save(ctx context.Context, h restic.Handle, rd restic.RewindReade
 	return nil
 }
 
+var openFile = fs.OpenFile // Overridden by test.
+
 // Load runs fn with a reader that yields the contents of the file at h at the
 // given offset.
 func (b *Local) Load(ctx context.Context, h restic.Handle, length int, offset int64, fn func(rd io.Reader) error) error {
@@ -150,7 +162,7 @@ func (b *Local) Load(ctx context.Context, h restic.Handle, length int, offset in
 func (b *Local) openReader(ctx context.Context, h restic.Handle, length int, offset int64) (io.ReadCloser, error) {
 	debug.Log("Load %v, length %v, offset %v", h, length, offset)
 	if err := h.Valid(); err != nil {
-		return nil, err
+		return nil, backoff.Permanent(err)
 	}
 
 	if offset < 0 {
@@ -181,7 +193,7 @@ func (b *Local) openReader(ctx context.Context, h restic.Handle, length int, off
 func (b *Local) Stat(ctx context.Context, h restic.Handle) (restic.FileInfo, error) {
 	debug.Log("Stat %v", h)
 	if err := h.Valid(); err != nil {
-		return restic.FileInfo{}, err
+		return restic.FileInfo{}, backoff.Permanent(err)
 	}
 
 	fi, err := fs.Stat(b.Filename(h))

internal/backend/local/local_internal_test.go

@@ -0,0 +1,42 @@
+package local
+
+import (
+	"context"
+	"errors"
+	"os"
+	"syscall"
+	"testing"
+
+	"github.com/restic/restic/internal/restic"
+	rtest "github.com/restic/restic/internal/test"
+
+	"github.com/cenkalti/backoff/v4"
+)
+
+func TestNoSpacePermanent(t *testing.T) {
+	oldOpenFile := openFile
+	defer func() {
+		openFile = oldOpenFile
+	}()
+
+	openFile = func(name string, flags int, mode os.FileMode) (*os.File, error) {
+		// The actual error from os.OpenFile is *os.PathError.
+		// Other functions called inside Save may return *os.SyscallError.
+		return nil, os.NewSyscallError("open", syscall.ENOSPC)
+	}
+
+	dir, cleanup := rtest.TempDir(t)
+	defer cleanup()
+
+	be, err := Open(context.Background(), Config{Path: dir})
+	rtest.OK(t, err)
+	defer be.Close()
+
+	h := restic.Handle{Type: restic.ConfigFile}
+	err = be.Save(context.Background(), h, nil)
+	_, ok := err.(*backoff.PermanentError)
+	rtest.Assert(t, ok,
+		"error type should be backoff.PermanentError, got %T", err)
+	rtest.Assert(t, errors.Is(err, syscall.ENOSPC),
+		"could not recover original ENOSPC error")
+}

internal/backend/mem/mem_backend.go

@@ -8,10 +8,11 @@ import (
 	"sync"
 
 	"github.com/restic/restic/internal/backend"
+	"github.com/restic/restic/internal/debug"
 	"github.com/restic/restic/internal/errors"
 	"github.com/restic/restic/internal/restic"
 
-	"github.com/restic/restic/internal/debug"
+	"github.com/cenkalti/backoff/v4"
 )
 
 type memMap map[restic.Handle][]byte
@@ -61,7 +62,7 @@ func (be *MemoryBackend) IsNotExist(err error) bool {
 // Save adds new Data to the backend.
 func (be *MemoryBackend) Save(ctx context.Context, h restic.Handle, rd restic.RewindReader) error {
 	if err := h.Valid(); err != nil {
-		return err
+		return backoff.Permanent(err)
 	}
 
 	be.m.Lock()
@@ -94,7 +95,7 @@ func (be *MemoryBackend) Load(ctx context.Context, h restic.Handle, length int,
 
 func (be *MemoryBackend) openReader(ctx context.Context, h restic.Handle, length int, offset int64) (io.ReadCloser, error) {
 	if err := h.Valid(); err != nil {
-		return nil, err
+		return nil, backoff.Permanent(err)
 	}
 
 	be.m.Lock()
@@ -133,7 +134,7 @@ func (be *MemoryBackend) Stat(ctx context.Context, h restic.Handle) (restic.File
 	defer be.m.Unlock()
 
 	if err := h.Valid(); err != nil {
-		return restic.FileInfo{}, err
+		return restic.FileInfo{}, backoff.Permanent(err)
 	}
 
 	if h.Type == restic.ConfigFile {

internal/backend/rest/rest.go

@@ -13,11 +13,12 @@ import (
 
 	"golang.org/x/net/context/ctxhttp"
 
+	"github.com/restic/restic/internal/backend"
 	"github.com/restic/restic/internal/debug"
 	"github.com/restic/restic/internal/errors"
 	"github.com/restic/restic/internal/restic"
 
-	"github.com/restic/restic/internal/backend"
+	"github.com/cenkalti/backoff/v4"
 )
 
 // make sure the rest backend implements restic.Backend
@@ -109,7 +110,7 @@ func (b *Backend) Location() string {
 // Save stores data in the backend at the handle.
 func (b *Backend) Save(ctx context.Context, h restic.Handle, rd restic.RewindReader) error {
 	if err := h.Valid(); err != nil {
-		return err
+		return backoff.Permanent(err)
 	}
 
 	ctx, cancel := context.WithCancel(ctx)
@@ -204,7 +205,7 @@ func (b *Backend) Load(ctx context.Context, h restic.Handle, length int, offset
 func (b *Backend) openReader(ctx context.Context, h restic.Handle, length int, offset int64) (io.ReadCloser, error) {
 	debug.Log("Load %v, length %v, offset %v", h, length, offset)
 	if err := h.Valid(); err != nil {
-		return nil, err
+		return nil, backoff.Permanent(err)
 	}
 
 	if offset < 0 {
@@ -256,7 +257,7 @@ func (b *Backend) openReader(ctx context.Context, h restic.Handle, length int, o
 // Stat returns information about a blob.
 func (b *Backend) Stat(ctx context.Context, h restic.Handle) (restic.FileInfo, error) {
 	if err := h.Valid(); err != nil {
-		return restic.FileInfo{}, err
+		return restic.FileInfo{}, backoff.Permanent(err)
 	}
 
 	req, err := http.NewRequest(http.MethodHead, b.Filename(h), nil)
@@ -311,7 +312,7 @@ func (b *Backend) Test(ctx context.Context, h restic.Handle) (bool, error) {
 // Remove removes the blob with the given name and type.
 func (b *Backend) Remove(ctx context.Context, h restic.Handle) error {
 	if err := h.Valid(); err != nil {
-		return err
+		return backoff.Permanent(err)
 	}
 
 	req, err := http.NewRequest("DELETE", b.Filename(h), nil)

internal/backend/s3/s3.go

@@ -12,13 +12,13 @@ import (
 	"time"
 
 	"github.com/restic/restic/internal/backend"
+	"github.com/restic/restic/internal/debug"
 	"github.com/restic/restic/internal/errors"
 	"github.com/restic/restic/internal/restic"
 
+	"github.com/cenkalti/backoff/v4"
 	"github.com/minio/minio-go/v7"
 	"github.com/minio/minio-go/v7/pkg/credentials"
-
-	"github.com/restic/restic/internal/debug"
 )
 
 // Backend stores data on an S3 endpoint.
@@ -260,7 +260,7 @@ func (be *Backend) Save(ctx context.Context, h restic.Handle, rd restic.RewindRe
 	debug.Log("Save %v", h)
 
 	if err := h.Valid(); err != nil {
-		return err
+		return backoff.Permanent(err)
 	}
 
 	objName := be.Filename(h)
@@ -300,7 +300,7 @@ func (be *Backend) Load(ctx context.Context, h restic.Handle, length int, offset
 func (be *Backend) openReader(ctx context.Context, h restic.Handle, length int, offset int64) (io.ReadCloser, error) {
 	debug.Log("Load %v, length %v, offset %v from %v", h, length, offset, be.Filename(h))
 	if err := h.Valid(); err != nil {
-		return nil, err
+		return nil, backoff.Permanent(err)
 	}
 
 	if offset < 0 {

internal/backend/sftp/sftp.go

@@ -16,6 +16,7 @@ import (
 	"github.com/restic/restic/internal/backend"
 	"github.com/restic/restic/internal/debug"
 
+	"github.com/cenkalti/backoff/v4"
 	"github.com/pkg/sftp"
 )
 
@@ -99,7 +100,7 @@ func (r *SFTP) clientError() error {
 	select {
 	case err := <-r.result:
 		debug.Log("client has exited with err %v", err)
-		return err
+		return backoff.Permanent(err)
 	default:
 	}
 
@@ -263,7 +264,7 @@ func (r *SFTP) Save(ctx context.Context, h restic.Handle, rd restic.RewindReader
 	}
 
 	if err := h.Valid(); err != nil {
-		return err
+		return backoff.Permanent(err)
 	}
 
 	filename := r.Filename(h)
@@ -310,7 +311,7 @@ func (r *SFTP) Load(ctx context.Context, h restic.Handle, length int, offset int
 func (r *SFTP) openReader(ctx context.Context, h restic.Handle, length int, offset int64) (io.ReadCloser, error) {
 	debug.Log("Load %v, length %v, offset %v", h, length, offset)
 	if err := h.Valid(); err != nil {
-		return nil, err
+		return nil, backoff.Permanent(err)
 	}
 
 	if offset < 0 {
@@ -345,7 +346,7 @@ func (r *SFTP) Stat(ctx context.Context, h restic.Handle) (restic.FileInfo, erro
 	}
 
 	if err := h.Valid(); err != nil {
-		return restic.FileInfo{}, err
+		return restic.FileInfo{}, backoff.Permanent(err)
 	}
 
 	fi, err := r.c.Lstat(r.Filename(h))

internal/backend/swift/swift.go

@@ -14,6 +14,7 @@ import (
 	"github.com/restic/restic/internal/errors"
 	"github.com/restic/restic/internal/restic"
 
+	"github.com/cenkalti/backoff/v4"
 	"github.com/ncw/swift"
 )
 
@@ -122,7 +123,7 @@ func (be *beSwift) Load(ctx context.Context, h restic.Handle, length int, offset
 func (be *beSwift) openReader(ctx context.Context, h restic.Handle, length int, offset int64) (io.ReadCloser, error) {
 	debug.Log("Load %v, length %v, offset %v", h, length, offset)
 	if err := h.Valid(); err != nil {
-		return nil, err
+		return nil, backoff.Permanent(err)
 	}
 
 	if offset < 0 {
@@ -162,7 +163,7 @@ func (be *beSwift) openReader(ctx context.Context, h restic.Handle, length int,
 // Save stores data in the backend at the handle.
 func (be *beSwift) Save(ctx context.Context, h restic.Handle, rd restic.RewindReader) error {
 	if err := h.Valid(); err != nil {
-		return err
+		return backoff.Permanent(err)
 	}
 
 	objName := be.Filename(h)

internal/errors/errors.go

@@ -3,6 +3,7 @@ package errors
 import (
 	"net/url"
 
+	"github.com/cenkalti/backoff/v4"
 	"github.com/pkg/errors"
 )
 
@@ -34,20 +35,19 @@ func Cause(err error) error {
 	}
 
 	for {
-		// unwrap *url.Error
+		switch e := err.(type) {
-		if urlErr, ok := err.(*url.Error); ok {
+		case Causer: // github.com/pkg/errors
-			err = urlErr.Err
+			err = e.Cause()
-			continue
+		case *backoff.PermanentError:
+			err = e.Err
+		case *url.Error:
+			err = e.Err
+		default:
+			return err
 		}
-
-		// if err is a Causer, return the cause for this error.
-		if c, ok := err.(Causer); ok {
-			err = c.Cause()
-			continue
-		}
-
-		break
 	}
-
-	return err
 }
+
+// Go 1.13-style error handling.
+
+func Is(x, y error) bool { return errors.Is(x, y) }

internal/restic/backend.go

@@ -6,6 +6,12 @@ import (
 )
 
 // Backend is used to store and access data.
+//
+// Backend operations that return an error will be retried when a Backend is
+// wrapped in a RetryBackend. To prevent that from happening, the operations
+// should return a github.com/cenkalti/backoff/v4.PermanentError. Errors from
+// the context package need not be wrapped, as context cancellation is checked
+// separately by the retrying logic.
 type Backend interface {
 	// Location returns a string that describes the type and location of the
 	// repository.