diff --git a/changelog/unreleased/issue-2453 b/changelog/unreleased/issue-2453 new file mode 100644 index 000000000..0e91b8c05 --- /dev/null +++ b/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 diff --git a/internal/backend/azure/azure.go b/internal/backend/azure/azure.go index 682534cd2..33162c227 100644 --- a/internal/backend/azure/azure.go +++ b/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 { diff --git a/internal/backend/b2/b2.go b/internal/backend/b2/b2.go index be1adc160..f6d4331c8 100644 --- a/internal/backend/b2/b2.go +++ b/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() diff --git a/internal/backend/local/local.go b/internal/backend/local/local.go index 2a48230fc..acb4751fc 100644 --- a/internal/backend/local/local.go +++ b/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)) diff --git a/internal/backend/local/local_internal_test.go b/internal/backend/local/local_internal_test.go new file mode 100644 index 000000000..a67233903 --- /dev/null +++ b/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") +} diff --git a/internal/backend/mem/mem_backend.go b/internal/backend/mem/mem_backend.go index fc7196f0b..8378630fa 100644 --- a/internal/backend/mem/mem_backend.go +++ b/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 { diff --git a/internal/backend/rest/rest.go b/internal/backend/rest/rest.go index a294cb35f..76feb3a11 100644 --- a/internal/backend/rest/rest.go +++ b/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) diff --git a/internal/backend/s3/s3.go b/internal/backend/s3/s3.go index 08d3b7aa7..83d784813 100644 --- a/internal/backend/s3/s3.go +++ b/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 { diff --git a/internal/backend/sftp/sftp.go b/internal/backend/sftp/sftp.go index 0a2c15e75..e395de60d 100644 --- a/internal/backend/sftp/sftp.go +++ b/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)) diff --git a/internal/backend/swift/swift.go b/internal/backend/swift/swift.go index fcbbec5e5..488ccbd14 100644 --- a/internal/backend/swift/swift.go +++ b/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) diff --git a/internal/errors/errors.go b/internal/errors/errors.go index ffd3d615e..51a425299 100644 --- a/internal/errors/errors.go +++ b/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 - if urlErr, ok := err.(*url.Error); ok { - err = urlErr.Err - continue + switch e := err.(type) { + case Causer: // github.com/pkg/errors + err = e.Cause() + 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) } diff --git a/internal/restic/backend.go b/internal/restic/backend.go index b2fd46b97..cda5c30c7 100644 --- a/internal/restic/backend.go +++ b/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.