Merge pull request #4152 from MichaelEischer/invalid-locks

Improve handling of invalid locks

changelog/unreleased/pull-4152

@@ -0,0 +1,16 @@
+Enhancement: Ignore empty lock files
+
+With restic 0.15.0 the checks for stale locks became much stricter than before.
+In particular, empty or unreadable locks were no longer ignored. This caused
+restic to complain about `Load(<lock/1234567812>, 0, 0) returned error,
+retrying after 552.330144ms: load(<lock/1234567812>): invalid data returned`
+and fail in the end.
+
+We have clarified the error message and changed the implementation to ignore
+empty lock files which are sometimes created as the result of a failed upload
+on some backends. Unreadable lock files still have to cleaned up manually. To
+do so, you can run `restic unlock --remove-all` which removes all existing lock
+files. But first make sure that no other restic process is currently running.
+
+https://github.com/restic/restic/issues/4143
+https://github.com/restic/restic/pull/4152

cmd/restic/lock.go

@@ -2,11 +2,11 @@ package main
 
 import (
 	"context"
-	"fmt"
 	"sync"
 	"time"
 
 	"github.com/restic/restic/internal/debug"
+	"github.com/restic/restic/internal/errors"
 	"github.com/restic/restic/internal/restic"
 )
 
@@ -44,8 +44,11 @@ func lockRepository(ctx context.Context, repo restic.Repository, exclusive bool)
 	}
 
 	lock, err := lockFn(ctx, repo)
+	if restic.IsInvalidLock(err) {
+		return nil, ctx, errors.Fatalf("%v\n\nthe `unlock --remove-all` command can be used to remove invalid locks. Make sure that no other restic process is accessing the repository when running the command", err)
+	}
 	if err != nil {
-		return nil, ctx, fmt.Errorf("unable to create lock in backend: %w", err)
+		return nil, ctx, errors.Fatalf("unable to create lock in backend: %v", err)
 	}
 	debug.Log("create lock %p (exclusive %v)", lock, exclusive)
 

internal/repository/repository.go

@@ -188,6 +188,7 @@ func (r *Repository) LoadUnpacked(ctx context.Context, t restic.FileType, id res
 
 	h := restic.Handle{Type: t, Name: id.String()}
 	retriedInvalidData := false
+	var dataErr error
 	err := r.be.Load(ctx, h, 0, 0, func(rd io.Reader) error {
 		// make sure this call is idempotent, in case an error occurs
 		wr := bytes.NewBuffer(buf[:0])
@@ -202,13 +203,20 @@ func (r *Repository) LoadUnpacked(ctx context.Context, t restic.FileType, id res
 			if !retriedInvalidData {
 				retriedInvalidData = true
 			} else {
+				// with a canceled context there is not guarantee which error will
+				// be returned by `be.Load`.
+				dataErr = fmt.Errorf("load(%v): %w", h, restic.ErrInvalidData)
 				cancel()
 			}
-			return errors.Errorf("load(%v): invalid data returned", h)
+			return restic.ErrInvalidData
+
 		}
 		return nil
 	})
 
+	if dataErr != nil {
+		return nil, dataErr
+	}
 	if err != nil {
 		return nil, err
 	}

internal/repository/testing.go

@@ -8,6 +8,7 @@ import (
 
 	"github.com/restic/restic/internal/backend/local"
 	"github.com/restic/restic/internal/backend/mem"
+	"github.com/restic/restic/internal/backend/retry"
 	"github.com/restic/restic/internal/crypto"
 	"github.com/restic/restic/internal/restic"
 	"github.com/restic/restic/internal/test"
@@ -97,11 +98,14 @@ func TestRepositoryWithVersion(t testing.TB, version uint) restic.Repository {
 
 // TestOpenLocal opens a local repository.
 func TestOpenLocal(t testing.TB, dir string) (r restic.Repository) {
+	var be restic.Backend
 	be, err := local.Open(context.TODO(), local.Config{Path: dir, Connections: 2})
 	if err != nil {
 		t.Fatal(err)
 	}
 
+	be = retry.New(be, 3, nil, nil)
+
 	repo, err := New(be, Options{})
 	if err != nil {
 		t.Fatal(err)

internal/restic/lock.go

@@ -60,6 +60,27 @@ func IsAlreadyLocked(err error) bool {
 	return errors.As(err, &e)
 }
 
+// invalidLockError is returned when NewLock or NewExclusiveLock fail due
+// to an invalid lock.
+type invalidLockError struct {
+	err error
+}
+
+func (e *invalidLockError) Error() string {
+	return fmt.Sprintf("invalid lock file: %v", e.err)
+}
+
+func (e *invalidLockError) Unwrap() error {
+	return e.err
+}
+
+// IsInvalidLock returns true iff err indicates that locking failed due to
+// an invalid lock.
+func IsInvalidLock(err error) bool {
+	var e *invalidLockError
+	return errors.As(err, &e)
+}
+
 // NewLock returns a new, non-exclusive lock for the repository. If an
 // exclusive lock is already held by another process, it returns an error
 // that satisfies IsAlreadyLocked.
@@ -146,7 +167,7 @@ func (l *Lock) checkForOtherLocks(ctx context.Context) error {
 				// if we cannot load a lock then it is unclear whether it can be ignored
 				// it could either be invalid or just unreadable due to network/permission problems
 				debug.Log("ignore lock %v: %v", id, err)
-				return errors.Fatal(err.Error())
+				return err
 			}
 
 			if l.Exclusive {
@@ -168,6 +189,9 @@ func (l *Lock) checkForOtherLocks(ctx context.Context) error {
 			return err
 		}
 	}
+	if errors.Is(err, ErrInvalidData) {
+		return &invalidLockError{err}
+	}
 	return err
 }
 
@@ -336,6 +360,12 @@ func ForAllLocks(ctx context.Context, repo Repository, excludeID *ID, fn func(ID
 		if excludeID != nil && id.Equal(*excludeID) {
 			return nil
 		}
+		if size == 0 {
+			// Ignore empty lock files as some backends do not guarantee atomic uploads.
+			// These may leave empty files behind if an upload was interrupted between
+			// creating the file and writing its data.
+			return nil
+		}
 		lock, err := LoadLock(ctx, repo, id)
 
 		m.Lock()

internal/restic/repository.go

@@ -4,10 +4,14 @@ import (
 	"context"
 
 	"github.com/restic/restic/internal/crypto"
+	"github.com/restic/restic/internal/errors"
 	"github.com/restic/restic/internal/ui/progress"
 	"golang.org/x/sync/errgroup"
 )
 
+// ErrInvalidData is used to report that a file is corrupted
+var ErrInvalidData = errors.New("invalid data returned")
+
 // Repository stores data in a backend. It provides high-level functions and
 // transparently encrypts/decrypts data.
 type Repository interface {