From 6d9675c323a093f33d7af8da7c948ce55a3a886b Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sat, 14 Jan 2023 16:04:14 +0100 Subject: [PATCH 1/6] repository: cleanup error message on invalid data The retry printed the filename twice: ``` Load(, 0, 0) returned error, retrying after 720.254544ms: load(): invalid data returned ``` now the warning has changed to ``` Load(, 0, 0) returned error, retrying after 720.254544ms: invalid data returned ``` --- internal/repository/repository.go | 3 ++- internal/restic/repository.go | 4 ++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/internal/repository/repository.go b/internal/repository/repository.go index a8d885a01..a5988b54c 100644 --- a/internal/repository/repository.go +++ b/internal/repository/repository.go @@ -204,7 +204,8 @@ func (r *Repository) LoadUnpacked(ctx context.Context, t restic.FileType, id res } else { cancel() } - return errors.Errorf("load(%v): invalid data returned", h) + return restic.ErrInvalidData + } return nil }) diff --git a/internal/restic/repository.go b/internal/restic/repository.go index c559b5aa3..e01d204e6 100644 --- a/internal/restic/repository.go +++ b/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 { From 1adf28a2b576e59d2c941f850f59776d385da960 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sat, 14 Jan 2023 16:06:35 +0100 Subject: [PATCH 2/6] repository: properly return invalid data error in LoadUnpacked The retry backend does not return the original error, if its execution is interrupted by canceling the context. Thus, we have to manually ensure that the invalid data error gets returned. Additionally, use the retry backend for some of the repository tests, as this is the configuration which will be used by restic. --- internal/repository/repository.go | 7 +++++++ internal/repository/testing.go | 4 ++++ 2 files changed, 11 insertions(+) diff --git a/internal/repository/repository.go b/internal/repository/repository.go index a5988b54c..df8a6fb68 100644 --- a/internal/repository/repository.go +++ b/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,6 +203,9 @@ 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 restic.ErrInvalidData @@ -210,6 +214,9 @@ func (r *Repository) LoadUnpacked(ctx context.Context, t restic.FileType, id res return nil }) + if dataErr != nil { + return nil, dataErr + } if err != nil { return nil, err } diff --git a/internal/repository/testing.go b/internal/repository/testing.go index 5b2e17366..879650336 100644 --- a/internal/repository/testing.go +++ b/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) From c995b5be528db2830fa06e9ab376de34daf05580 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sat, 14 Jan 2023 17:38:20 +0100 Subject: [PATCH 3/6] lock: cleanup error message The error message is now `Fatal: unable to create lock in backend: [...]` instead of `unable to create lock in backend: Fatal: [...]`. --- cmd/restic/lock.go | 4 ++-- internal/restic/lock.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/cmd/restic/lock.go b/cmd/restic/lock.go index e15d63b8d..ea230fa7a 100644 --- a/cmd/restic/lock.go +++ b/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" ) @@ -45,7 +45,7 @@ func lockRepository(ctx context.Context, repo restic.Repository, exclusive bool) lock, err := lockFn(ctx, repo) 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) diff --git a/internal/restic/lock.go b/internal/restic/lock.go index 8f49aee49..10271b778 100644 --- a/internal/restic/lock.go +++ b/internal/restic/lock.go @@ -146,7 +146,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 { From 20ad14e36282294e12047770e17b7e265085fac5 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sat, 14 Jan 2023 17:45:25 +0100 Subject: [PATCH 4/6] lock: add help message how to recover from invalid locks --- cmd/restic/lock.go | 3 +++ internal/restic/lock.go | 24 ++++++++++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/cmd/restic/lock.go b/cmd/restic/lock.go index ea230fa7a..f39a08db6 100644 --- a/cmd/restic/lock.go +++ b/cmd/restic/lock.go @@ -44,6 +44,9 @@ 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, errors.Fatalf("unable to create lock in backend: %v", err) } diff --git a/internal/restic/lock.go b/internal/restic/lock.go index 10271b778..94b1801f2 100644 --- a/internal/restic/lock.go +++ b/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. @@ -168,6 +189,9 @@ func (l *Lock) checkForOtherLocks(ctx context.Context) error { return err } } + if errors.Is(err, ErrInvalidData) { + return &invalidLockError{err} + } return err } From 57acc769b400ece4c3d48415e6f5c4ab7b6884fa Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sat, 14 Jan 2023 18:14:08 +0100 Subject: [PATCH 5/6] lock: Ignore empty lock files These may be left behind by backends which do not guarantee atomic uploads. --- internal/restic/lock.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/internal/restic/lock.go b/internal/restic/lock.go index 94b1801f2..2da3667a3 100644 --- a/internal/restic/lock.go +++ b/internal/restic/lock.go @@ -360,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() From 8f94eb5420adea06cbdd24840bd74a5ebdab0719 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sun, 22 Jan 2023 15:52:05 +0100 Subject: [PATCH 6/6] add changelog for less strict lock handling --- changelog/unreleased/pull-4152 | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 changelog/unreleased/pull-4152 diff --git a/changelog/unreleased/pull-4152 b/changelog/unreleased/pull-4152 new file mode 100644 index 000000000..4eb004f20 --- /dev/null +++ b/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(, 0, 0) returned error, +retrying after 552.330144ms: load(): 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