From c6fd13425b21bba4ee608eb7ec04f9a6b5546390 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sun, 9 Aug 2020 13:36:02 +0200 Subject: [PATCH 1/3] remember the refreshed lock file even if removal failed This ensures that restic won't create lots of new lock files without deleting them later on. In some cases a Delete operation on a backend can return a "File does not exist" error even though the Delete operation succeeded. This can for example be caused by request retries. This caused restic to forget about the new lock file and continue trying to remove the old (already deleted) lock file. --- internal/restic/lock.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/internal/restic/lock.go b/internal/restic/lock.go index e79882f80..9a5fd841d 100644 --- a/internal/restic/lock.go +++ b/internal/restic/lock.go @@ -223,15 +223,11 @@ func (l *Lock) Refresh(ctx context.Context) error { return err } - err = l.repo.Backend().Remove(context.TODO(), Handle{Type: LockFile, Name: l.lockID.String()}) - if err != nil { - return err - } - debug.Log("new lock ID %v", id) + oldLockID := l.lockID l.lockID = &id - return nil + return l.repo.Backend().Remove(context.TODO(), Handle{Type: LockFile, Name: oldLockID.String()}) } func (l Lock) String() string { From d72181c8c1e49d48b7219e105291895b49f1e54a Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sun, 9 Aug 2020 13:54:39 +0200 Subject: [PATCH 2/3] Ensure that the lock cleanup handler is run after the global one cleanup handlers run in the order in which they are added. As Go calls init() functions in lexical order, the cleanup handler from global.go was registered before that from lock.go, which is the correct order. Make this order explicit to ensure that this won't break accidentally. --- cmd/restic/global.go | 2 ++ cmd/restic/lock.go | 11 +++++++---- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/cmd/restic/global.go b/cmd/restic/global.go index 9f42f851a..e3b4020d6 100644 --- a/cmd/restic/global.go +++ b/cmd/restic/global.go @@ -98,6 +98,8 @@ func init() { var cancel context.CancelFunc globalOptions.ctx, cancel = context.WithCancel(context.Background()) AddCleanupHandler(func() error { + // Must be called before the unlock cleanup handler to ensure that the latter is + // not blocked due to limited number of backend connections, see #1434 cancel() return nil }) diff --git a/cmd/restic/lock.go b/cmd/restic/lock.go index 822059943..64f82cf52 100644 --- a/cmd/restic/lock.go +++ b/cmd/restic/lock.go @@ -16,6 +16,7 @@ var globalLocks struct { cancelRefresh chan struct{} refreshWG sync.WaitGroup sync.Mutex + sync.Once } func lockRepo(ctx context.Context, repo *repository.Repository) (*restic.Lock, error) { @@ -27,6 +28,12 @@ func lockRepoExclusive(ctx context.Context, repo *repository.Repository) (*resti } func lockRepository(ctx context.Context, repo *repository.Repository, exclusive bool) (*restic.Lock, error) { + // make sure that a repository is unlocked properly and after cancel() was + // called by the cleanup handler in global.go + globalLocks.Do(func() { + AddCleanupHandler(unlockAll) + }) + lockFn := restic.NewLock if exclusive { lockFn = restic.NewExclusiveLock @@ -128,7 +135,3 @@ func unlockAll() error { return nil } - -func init() { - AddCleanupHandler(unlockAll) -} From aebd24e41450df05db16db035a28e9d2e55793b8 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Wed, 15 Sep 2021 21:56:45 +0200 Subject: [PATCH 3/3] Add changelog --- changelog/unreleased/issue-2452 | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 changelog/unreleased/issue-2452 diff --git a/changelog/unreleased/issue-2452 b/changelog/unreleased/issue-2452 new file mode 100644 index 000000000..3a82e9b1b --- /dev/null +++ b/changelog/unreleased/issue-2452 @@ -0,0 +1,11 @@ +Bugfix: Improve error handling of repository locking + +When the lock refresh failed to delete the old lock file, it forgot about the +newly created one. Instead it continued trying to delete the old (usually no +longer existing) lock file and thus over time lots of lock files accumulated. +This has been fixed. + +https://github.com/restic/restic/issues/2452 +https://github.com/restic/restic/issues/2473 +https://github.com/restic/restic/issues/2562 +https://github.com/restic/restic/pull/3512