From 3ee465d36368d7f757c726c866edfb37aff11930 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sun, 7 Jun 2026 13:05:50 +0200 Subject: [PATCH 01/15] repository: rename Lock function to LockRepo Rename `Lock()` to `LockRepo()` to make room for the `Lock` struct. --- cmd/restic/lock.go | 2 +- internal/repository/lock.go | 2 +- internal/repository/lock_test.go | 16 ++++++++-------- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/cmd/restic/lock.go b/cmd/restic/lock.go index 38fc79a0c..8264c6b9e 100644 --- a/cmd/restic/lock.go +++ b/cmd/restic/lock.go @@ -18,7 +18,7 @@ func internalOpenWithLocked(ctx context.Context, gopts global.Options, dryRun bo if !dryRun { var lock repository.Unlocker - lock, ctx, err = repository.Lock(ctx, repo, exclusive, gopts.RetryLock, func(msg string) { + lock, ctx, err = repository.LockRepo(ctx, repo, exclusive, gopts.RetryLock, func(msg string) { if !gopts.JSON { printer.P("%s", msg) } diff --git a/internal/repository/lock.go b/internal/repository/lock.go index d794f03e7..3267e933f 100644 --- a/internal/repository/lock.go +++ b/internal/repository/lock.go @@ -37,7 +37,7 @@ var lockerInst = &locker{ refreshabilityTimeout: restic.StaleLockTimeout - defaultRefreshInterval*3/2, } -func Lock(ctx context.Context, repo *Repository, exclusive bool, retryLock time.Duration, printRetry func(msg string), logger func(format string, args ...interface{})) (Unlocker, context.Context, error) { +func LockRepo(ctx context.Context, repo *Repository, exclusive bool, retryLock time.Duration, printRetry func(msg string), logger func(format string, args ...interface{})) (Unlocker, context.Context, error) { return lockerInst.Lock(ctx, repo, exclusive, retryLock, printRetry, logger) } diff --git a/internal/repository/lock_test.go b/internal/repository/lock_test.go index bdfb6573b..5a2813e75 100644 --- a/internal/repository/lock_test.go +++ b/internal/repository/lock_test.go @@ -76,10 +76,10 @@ func TestLockConflict(t *testing.T) { repo, be := openLockTestRepo(t, nil) repo2 := TestOpenBackend(t, be) - lock, _, err := Lock(context.Background(), repo, true, 0, func(msg string) {}, func(format string, args ...interface{}) {}) + lock, _, err := LockRepo(context.Background(), repo, true, 0, func(msg string) {}, func(format string, args ...interface{}) {}) rtest.OK(t, err) defer lock.Unlock() - _, _, err = Lock(context.Background(), repo2, false, 0, func(msg string) {}, func(format string, args ...interface{}) {}) + _, _, err = LockRepo(context.Background(), repo2, false, 0, func(msg string) {}, func(format string, args ...interface{}) {}) if err == nil { t.Fatal("second lock should have failed") } @@ -240,14 +240,14 @@ func TestLockWaitTimeout(t *testing.T) { t.Parallel() repo, _ := openLockTestRepo(t, nil) - elock, _, err := Lock(context.TODO(), repo, true, 0, func(msg string) {}, func(format string, args ...interface{}) {}) + elock, _, err := LockRepo(context.TODO(), repo, true, 0, func(msg string) {}, func(format string, args ...interface{}) {}) rtest.OK(t, err) defer elock.Unlock() retryLock := 200 * time.Millisecond start := time.Now() - _, _, err = Lock(context.TODO(), repo, false, retryLock, func(msg string) {}, func(format string, args ...interface{}) {}) + _, _, err = LockRepo(context.TODO(), repo, false, retryLock, func(msg string) {}, func(format string, args ...interface{}) {}) duration := time.Since(start) rtest.Assert(t, err != nil, @@ -262,7 +262,7 @@ func TestLockWaitCancel(t *testing.T) { t.Parallel() repo, _ := openLockTestRepo(t, nil) - elock, _, err := Lock(context.TODO(), repo, true, 0, func(msg string) {}, func(format string, args ...interface{}) {}) + elock, _, err := LockRepo(context.TODO(), repo, true, 0, func(msg string) {}, func(format string, args ...interface{}) {}) rtest.OK(t, err) defer elock.Unlock() @@ -273,7 +273,7 @@ func TestLockWaitCancel(t *testing.T) { ctx, cancel := context.WithCancel(context.TODO()) time.AfterFunc(cancelAfter, cancel) - _, _, err = Lock(ctx, repo, false, retryLock, func(msg string) {}, func(format string, args ...interface{}) {}) + _, _, err = LockRepo(ctx, repo, false, retryLock, func(msg string) {}, func(format string, args ...interface{}) {}) duration := time.Since(start) rtest.Assert(t, err != nil, @@ -288,7 +288,7 @@ func TestLockWaitSuccess(t *testing.T) { t.Parallel() repo, _ := openLockTestRepo(t, nil) - elock, _, err := Lock(context.TODO(), repo, true, 0, func(msg string) {}, func(format string, args ...interface{}) {}) + elock, _, err := LockRepo(context.TODO(), repo, true, 0, func(msg string) {}, func(format string, args ...interface{}) {}) rtest.OK(t, err) retryLock := 200 * time.Millisecond @@ -298,7 +298,7 @@ func TestLockWaitSuccess(t *testing.T) { elock.Unlock() }) - lock, _, err := Lock(context.TODO(), repo, false, retryLock, func(msg string) {}, func(format string, args ...interface{}) {}) + lock, _, err := LockRepo(context.TODO(), repo, false, retryLock, func(msg string) {}, func(format string, args ...interface{}) {}) rtest.OK(t, err) lock.Unlock() } From 7015da44ad82d9dbd30b0f6d65f96b8ad63e7d71 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sun, 7 Jun 2026 13:06:04 +0200 Subject: [PATCH 02/15] restic: extract UidGidInt into separate files Extract UidGidInt to simplify moving the locking code to the repository. --- internal/restic/lock_unix.go | 18 ------------------ internal/restic/lock_windows.go | 6 ------ internal/restic/uid_unix.go | 25 +++++++++++++++++++++++++ internal/restic/uid_windows.go | 10 ++++++++++ 4 files changed, 35 insertions(+), 24 deletions(-) create mode 100644 internal/restic/uid_unix.go create mode 100644 internal/restic/uid_windows.go diff --git a/internal/restic/lock_unix.go b/internal/restic/lock_unix.go index ca1c74df2..71f43fd36 100644 --- a/internal/restic/lock_unix.go +++ b/internal/restic/lock_unix.go @@ -4,29 +4,11 @@ package restic import ( "os" - "os/user" - "strconv" "syscall" "github.com/restic/restic/internal/debug" - "github.com/restic/restic/internal/errors" ) -// UidGidInt returns uid, gid of the user as a number. -// -//nolint:revive // capitalization is correct as is -func UidGidInt(u *user.User) (uid, gid uint32, err error) { - ui, err := strconv.ParseUint(u.Uid, 10, 32) - if err != nil { - return 0, 0, errors.Errorf("invalid UID %q", u.Uid) - } - gi, err := strconv.ParseUint(u.Gid, 10, 32) - if err != nil { - return 0, 0, errors.Errorf("invalid GID %q", u.Gid) - } - return uint32(ui), uint32(gi), nil -} - // checkProcess will check if the process retaining the lock // exists and responds to SIGHUP signal. // Returns true if the process exists and responds. diff --git a/internal/restic/lock_windows.go b/internal/restic/lock_windows.go index 3cd7c3517..442b08397 100644 --- a/internal/restic/lock_windows.go +++ b/internal/restic/lock_windows.go @@ -2,16 +2,10 @@ package restic import ( "os" - "os/user" "github.com/restic/restic/internal/debug" ) -// UidGidInt always returns 0 on Windows, since uid isn't numbers -func UidGidInt(_ *user.User) (uid, gid uint32, err error) { - return 0, 0, nil -} - // checkProcess will check if the process retaining the lock exists. // Returns true if the process exists. func (l *Lock) processExists() bool { diff --git a/internal/restic/uid_unix.go b/internal/restic/uid_unix.go new file mode 100644 index 000000000..4072562ef --- /dev/null +++ b/internal/restic/uid_unix.go @@ -0,0 +1,25 @@ +//go:build !windows + +package restic + +import ( + "os/user" + "strconv" + + "github.com/restic/restic/internal/errors" +) + +// UidGidInt returns uid, gid of the user as a number. +// +//nolint:revive // capitalization is correct as is +func UidGidInt(u *user.User) (uid, gid uint32, err error) { + ui, err := strconv.ParseUint(u.Uid, 10, 32) + if err != nil { + return 0, 0, errors.Errorf("invalid UID %q", u.Uid) + } + gi, err := strconv.ParseUint(u.Gid, 10, 32) + if err != nil { + return 0, 0, errors.Errorf("invalid GID %q", u.Gid) + } + return uint32(ui), uint32(gi), nil +} diff --git a/internal/restic/uid_windows.go b/internal/restic/uid_windows.go new file mode 100644 index 000000000..320092614 --- /dev/null +++ b/internal/restic/uid_windows.go @@ -0,0 +1,10 @@ +package restic + +import ( + "os/user" +) + +// UidGidInt always returns 0 on Windows, since uid isn't numbers +func UidGidInt(_ *user.User) (uid, gid uint32, err error) { + return 0, 0, nil +} From b892b1a1503b665d45d2f7dfb26440906607d21f Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sun, 7 Jun 2026 13:06:02 +0200 Subject: [PATCH 03/15] repository: move lock file handling from restic package --- cmd/restic/cmd_cat.go | 2 +- cmd/restic/cmd_init_integration_test.go | 2 +- cmd/restic/main.go | 5 +- internal/repository/lock.go | 20 ++--- .../lock.go => repository/lock_file.go} | 46 +++++------ .../lock_file_test.go} | 81 +++++++++---------- .../lock_file_unix.go} | 2 +- .../lock_file_windows.go} | 2 +- internal/repository/lock_test.go | 8 +- internal/repository/testing.go | 5 -- 10 files changed, 83 insertions(+), 90 deletions(-) rename internal/{restic/lock.go => repository/lock_file.go} (87%) rename internal/{restic/lock_test.go => repository/lock_file_test.go} (69%) rename internal/{restic/lock_unix.go => repository/lock_file_unix.go} (97%) rename internal/{restic/lock_windows.go => repository/lock_file_windows.go} (96%) diff --git a/cmd/restic/cmd_cat.go b/cmd/restic/cmd_cat.go index 437c4437b..f54a5ce44 100644 --- a/cmd/restic/cmd_cat.go +++ b/cmd/restic/cmd_cat.go @@ -142,7 +142,7 @@ func runCat(ctx context.Context, gopts global.Options, args []string, term ui.Te printer.S(string(buf)) return nil case "lock": - lock, err := restic.LoadLock(ctx, repo, id) + lock, err := repository.LoadLock(ctx, repo, id) if err != nil { return err } diff --git a/cmd/restic/cmd_init_integration_test.go b/cmd/restic/cmd_init_integration_test.go index 5d8a8c64c..d30a7c078 100644 --- a/cmd/restic/cmd_init_integration_test.go +++ b/cmd/restic/cmd_init_integration_test.go @@ -16,7 +16,7 @@ import ( func testRunInit(t testing.TB, gopts global.Options) { repository.TestUseLowSecurityKDFParameters(t) restic.TestDisableCheckPolynomial(t) - restic.TestSetLockTimeout(t, 0) + repository.TestSetLockTimeout(t, 0) err := withTermStatus(t, gopts, func(ctx context.Context, gopts global.Options) error { return runInit(ctx, InitOptions{}, gopts, nil, gopts.Term) diff --git a/cmd/restic/main.go b/cmd/restic/main.go index 619eee642..09a64e8db 100644 --- a/cmd/restic/main.go +++ b/cmd/restic/main.go @@ -20,7 +20,6 @@ import ( "github.com/restic/restic/internal/feature" "github.com/restic/restic/internal/global" "github.com/restic/restic/internal/repository" - "github.com/restic/restic/internal/restic" "github.com/restic/restic/internal/ui/termstatus" ) @@ -198,7 +197,7 @@ func main() { var exitMessage string switch { - case restic.IsAlreadyLocked(err): + case repository.IsAlreadyLocked(err): exitMessage = fmt.Sprintf("%v\nthe `unlock` command can be used to remove stale locks", err) case err == ErrInvalidSourceData: exitMessage = fmt.Sprintf("Warning: %v", err) @@ -228,7 +227,7 @@ func main() { exitCode = 3 case errors.Is(err, global.ErrNoRepository): exitCode = 10 - case restic.IsAlreadyLocked(err): + case repository.IsAlreadyLocked(err): exitCode = 11 case errors.Is(err, repository.ErrNoKeyFound): exitCode = 12 diff --git a/internal/repository/lock.go b/internal/repository/lock.go index 3267e933f..488aca022 100644 --- a/internal/repository/lock.go +++ b/internal/repository/lock.go @@ -14,7 +14,7 @@ import ( ) type lockContext struct { - lock *restic.Lock + lock *Lock cancel context.CancelFunc refreshWG sync.WaitGroup } @@ -34,7 +34,7 @@ var lockerInst = &locker{ refreshInterval: defaultRefreshInterval, // consider a lock refresh failed a bit before the lock actually becomes stale // the difference allows to compensate for a small time drift between clients. - refreshabilityTimeout: restic.StaleLockTimeout - defaultRefreshInterval*3/2, + refreshabilityTimeout: StaleLockTimeout - defaultRefreshInterval*3/2, } func LockRepo(ctx context.Context, repo *Repository, exclusive bool, retryLock time.Duration, printRetry func(msg string), logger func(format string, args ...interface{})) (Unlocker, context.Context, error) { @@ -43,8 +43,8 @@ func LockRepo(ctx context.Context, repo *Repository, exclusive bool, retryLock t // Lock wraps the ctx such that it is cancelled when the repository is unlocked // cancelling the original context also stops the lock refresh -func (l *locker) Lock(ctx context.Context, r *Repository, exclusive bool, retryLock time.Duration, printRetry func(msg string), logger func(format string, args ...interface{})) (Unlocker, context.Context, error) { - var lock *restic.Lock +func (l *locker) Lock(ctx context.Context, r *Repository, exclusive bool, retryLock time.Duration, printRetry func(msg string), logger func(format string, args ...interface{})) (*unlocker, context.Context, error) { + var lock *Lock var err error retrySleep := minDuration(l.retrySleepStart, retryLock) @@ -55,8 +55,8 @@ func (l *locker) Lock(ctx context.Context, r *Repository, exclusive bool, retryL retryLoop: for { - lock, err = restic.NewLock(ctx, repo, exclusive) - if err != nil && restic.IsAlreadyLocked(err) { + lock, err = NewLock(ctx, repo, exclusive) + if err != nil && IsAlreadyLocked(err) { if !retryMessagePrinted { printRetry(fmt.Sprintf("repo already locked, waiting up to %s for the lock\n", retryLock)) @@ -72,7 +72,7 @@ retryLoop: case <-retryTimeout: debug.Log("repo already locked, timeout expired") // Last lock attempt - lock, err = restic.NewLock(ctx, repo, exclusive) + lock, err = NewLock(ctx, repo, exclusive) break retryLoop case <-retrySleepCh: retrySleep = minDuration(retrySleep*2, l.retrySleepMax) @@ -82,7 +82,7 @@ retryLoop: break retryLoop } } - if restic.IsInvalidLock(err) { + if 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 { @@ -242,7 +242,7 @@ func (l *locker) monitorLockRefresh(ctx context.Context, lockInfo *lockContext, } } -func tryRefreshStaleLock(ctx context.Context, be backend.Backend, lock *restic.Lock, cancel context.CancelFunc, logger func(format string, args ...interface{})) bool { +func tryRefreshStaleLock(ctx context.Context, be backend.Backend, lock *Lock, cancel context.CancelFunc, logger func(format string, args ...interface{})) bool { freeze := backend.AsBackend[backend.FreezeBackend](be) if freeze != nil { debug.Log("freezing backend") @@ -277,7 +277,7 @@ func (l *unlocker) Unlock() { // RemoveStaleLocks deletes all locks detected as stale from the repository. func RemoveStaleLocks(ctx context.Context, repo *Repository) (uint, error) { var processed uint - err := restic.ForAllLocks(ctx, repo, nil, func(id restic.ID, lock *restic.Lock, err error) error { + err := ForAllLocks(ctx, repo, nil, func(id restic.ID, lock *Lock, err error) error { if err != nil { // ignore locks that cannot be loaded debug.Log("ignore lock %v: %v", id, err) diff --git a/internal/restic/lock.go b/internal/repository/lock_file.go similarity index 87% rename from internal/restic/lock.go rename to internal/repository/lock_file.go index 7b7f04af8..b0a0b3ede 100644 --- a/internal/restic/lock.go +++ b/internal/repository/lock_file.go @@ -1,4 +1,4 @@ -package restic +package repository import ( "context" @@ -11,9 +11,9 @@ import ( "testing" "time" - "github.com/restic/restic/internal/errors" - "github.com/restic/restic/internal/debug" + "github.com/restic/restic/internal/errors" + "github.com/restic/restic/internal/restic" ) // UnlockCancelDelay bounds the duration how long lock cleanup operations will wait @@ -38,8 +38,8 @@ type Lock struct { UID uint32 `json:"uid,omitempty"` GID uint32 `json:"gid,omitempty"` - repo Unpacked[FileType] - lockID *ID + repo restic.Unpacked[restic.FileType] + lockID *restic.ID } // alreadyLockedError is returned when NewLock or NewExclusiveLock are unable to @@ -102,7 +102,7 @@ func TestSetLockTimeout(t testing.TB, d time.Duration) { // exclusive lock is already held by another process, it returns an error // that satisfies IsAlreadyLocked. If the new lock is exclude, then other // non-exclusive locks also result in an IsAlreadyLocked error. -func NewLock(ctx context.Context, repo Unpacked[FileType], exclusive bool) (*Lock, error) { +func NewLock(ctx context.Context, repo restic.Unpacked[restic.FileType], exclusive bool) (*Lock, error) { lock := &Lock{ Time: time.Now(), PID: os.Getpid(), @@ -147,7 +147,7 @@ func (l *Lock) fillUserInfo() error { } l.Username = usr.Username - l.UID, l.GID, err = UidGidInt(usr) + l.UID, l.GID, err = restic.UidGidInt(usr) return err } @@ -159,7 +159,7 @@ func (l *Lock) fillUserInfo() error { // exclusive lock is found. func (l *Lock) checkForOtherLocks(ctx context.Context) error { var err error - checkedIDs := NewIDSet() + checkedIDs := restic.NewIDSet() if l.lockID != nil { checkedIDs.Insert(*l.lockID) } @@ -177,7 +177,7 @@ func (l *Lock) checkForOtherLocks(ctx context.Context) error { // Store updates in new IDSet to prevent data races var m sync.Mutex newCheckedIDs := checkedIDs.Clone() - err = ForAllLocks(ctx, l.repo, checkedIDs, func(id ID, lock *Lock, err error) error { + err = ForAllLocks(ctx, l.repo, checkedIDs, func(id restic.ID, lock *Lock, err error) error { if err != nil { // 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 @@ -209,7 +209,7 @@ func (l *Lock) checkForOtherLocks(ctx context.Context) error { return err } } - if errors.Is(err, ErrInvalidData) { + if errors.Is(err, restic.ErrInvalidData) { return &invalidLockError{err} } return err @@ -228,10 +228,10 @@ func cancelableDelay(ctx context.Context, delay time.Duration) error { } // createLock acquires the lock by creating a file in the repository. -func (l *Lock) createLock(ctx context.Context) (ID, error) { - id, err := SaveJSONUnpacked(ctx, l.repo, LockFile, l) +func (l *Lock) createLock(ctx context.Context) (restic.ID, error) { + id, err := restic.SaveJSONUnpacked(ctx, l.repo, restic.LockFile, l) if err != nil { - return ID{}, err + return restic.ID{}, err } return id, nil @@ -246,7 +246,7 @@ func (l *Lock) Unlock(ctx context.Context) error { ctx, cancel := delayedCancelContext(ctx, UnlockCancelDelay) defer cancel() - return l.repo.RemoveUnpacked(ctx, LockFile, *l.lockID) + return l.repo.RemoveUnpacked(ctx, restic.LockFile, *l.lockID) } var StaleLockTimeout = 30 * time.Minute @@ -326,7 +326,7 @@ func (l *Lock) Refresh(ctx context.Context) error { ctx, cancel := delayedCancelContext(ctx, UnlockCancelDelay) defer cancel() - return l.repo.RemoveUnpacked(ctx, LockFile, *oldLockID) + return l.repo.RemoveUnpacked(ctx, restic.LockFile, *oldLockID) } // RefreshStaleLock is an extended variant of Refresh that can also refresh stale lock files. @@ -359,13 +359,13 @@ func (l *Lock) RefreshStaleLock(ctx context.Context) error { if err != nil { // cleanup replacement lock - _ = l.repo.RemoveUnpacked(ctx, LockFile, id) + _ = l.repo.RemoveUnpacked(ctx, restic.LockFile, id) return err } if !exists { // cleanup replacement lock - _ = l.repo.RemoveUnpacked(ctx, LockFile, id) + _ = l.repo.RemoveUnpacked(ctx, restic.LockFile, id) return ErrRemovedLock } @@ -376,7 +376,7 @@ func (l *Lock) RefreshStaleLock(ctx context.Context) error { oldLockID := l.lockID l.lockID = &id - return l.repo.RemoveUnpacked(ctx, LockFile, *oldLockID) + return l.repo.RemoveUnpacked(ctx, restic.LockFile, *oldLockID) } func (l *Lock) checkExistence(ctx context.Context) (bool, error) { @@ -385,7 +385,7 @@ func (l *Lock) checkExistence(ctx context.Context) (bool, error) { exists := false - err := l.repo.List(ctx, LockFile, func(id ID, _ int64) error { + err := l.repo.List(ctx, restic.LockFile, func(id restic.ID, _ int64) error { if id.Equal(*l.lockID) { exists = true } @@ -423,9 +423,9 @@ func init() { } // LoadLock loads and unserializes a lock from a repository. -func LoadLock(ctx context.Context, repo LoaderUnpacked, id ID) (*Lock, error) { +func LoadLock(ctx context.Context, repo restic.LoaderUnpacked, id restic.ID) (*Lock, error) { lock := &Lock{} - if err := LoadJSONUnpacked(ctx, repo, LockFile, id, lock); err != nil { + if err := restic.LoadJSONUnpacked(ctx, repo, restic.LockFile, id, lock); err != nil { return nil, err } lock.lockID = &id @@ -437,11 +437,11 @@ func LoadLock(ctx context.Context, repo LoaderUnpacked, id ID) (*Lock, error) { // It is guaranteed that the function is not run concurrently. If the // callback returns an error, this function is cancelled and also returns that error. // If a lock ID is passed via excludeID, it will be ignored. -func ForAllLocks(ctx context.Context, repo ListerLoaderUnpacked, excludeIDs IDSet, fn func(ID, *Lock, error) error) error { +func ForAllLocks(ctx context.Context, repo restic.ListerLoaderUnpacked, excludeIDs restic.IDSet, fn func(restic.ID, *Lock, error) error) error { var m sync.Mutex // For locks decoding is nearly for free, thus just assume were only limited by IO - return ParallelList(ctx, repo, LockFile, repo.Connections(), func(ctx context.Context, id ID, size int64) error { + return restic.ParallelList(ctx, repo, restic.LockFile, repo.Connections(), func(ctx context.Context, id restic.ID, size int64) error { if excludeIDs.Has(id) { return nil } diff --git a/internal/restic/lock_test.go b/internal/repository/lock_file_test.go similarity index 69% rename from internal/restic/lock_test.go rename to internal/repository/lock_file_test.go index 67d2b9a46..5443c3ca7 100644 --- a/internal/restic/lock_test.go +++ b/internal/repository/lock_file_test.go @@ -1,4 +1,4 @@ -package restic_test +package repository import ( "context" @@ -10,26 +10,25 @@ import ( "github.com/restic/restic/internal/backend" "github.com/restic/restic/internal/backend/mem" - "github.com/restic/restic/internal/repository" "github.com/restic/restic/internal/restic" rtest "github.com/restic/restic/internal/test" ) -func TestLock(t *testing.T) { - repo := repository.TestRepository(t) - restic.TestSetLockTimeout(t, 5*time.Millisecond) +func TestLockFile(t *testing.T) { + repo := TestRepository(t) + TestSetLockTimeout(t, 5*time.Millisecond) - lock, err := repository.TestNewLock(t, repo, false) + lock, err := NewLock(context.TODO(), &internalRepository{repo}, false) rtest.OK(t, err) rtest.OK(t, lock.Unlock(context.TODO())) } func TestDoubleUnlock(t *testing.T) { - repo := repository.TestRepository(t) - restic.TestSetLockTimeout(t, 5*time.Millisecond) + repo := TestRepository(t) + TestSetLockTimeout(t, 5*time.Millisecond) - lock, err := repository.TestNewLock(t, repo, false) + lock, err := NewLock(context.TODO(), &internalRepository{repo}, false) rtest.OK(t, err) rtest.OK(t, lock.Unlock(context.TODO())) @@ -40,13 +39,13 @@ func TestDoubleUnlock(t *testing.T) { } func TestMultipleLock(t *testing.T) { - repo := repository.TestRepository(t) - restic.TestSetLockTimeout(t, 5*time.Millisecond) + repo := TestRepository(t) + TestSetLockTimeout(t, 5*time.Millisecond) - lock1, err := repository.TestNewLock(t, repo, false) + lock1, err := NewLock(context.TODO(), &internalRepository{repo}, false) rtest.OK(t, err) - lock2, err := repository.TestNewLock(t, repo, false) + lock2, err := NewLock(context.TODO(), &internalRepository{repo}, false) rtest.OK(t, err) rtest.OK(t, lock1.Unlock(context.TODO())) @@ -66,37 +65,37 @@ func (be *failLockLoadingBackend) Load(ctx context.Context, h backend.Handle, le func TestMultipleLockFailure(t *testing.T) { be := &failLockLoadingBackend{Backend: mem.New()} - repo, _ := repository.TestRepositoryWithBackend(t, be, 0, repository.Options{}) - restic.TestSetLockTimeout(t, 5*time.Millisecond) + repo, _ := TestRepositoryWithBackend(t, be, 0, Options{}) + TestSetLockTimeout(t, 5*time.Millisecond) - lock1, err := repository.TestNewLock(t, repo, false) + lock1, err := NewLock(context.TODO(), &internalRepository{repo}, false) rtest.OK(t, err) - _, err = repository.TestNewLock(t, repo, false) + _, err = NewLock(context.TODO(), &internalRepository{repo}, false) rtest.Assert(t, err != nil, "unreadable lock file did not result in an error") rtest.OK(t, lock1.Unlock(context.TODO())) } func TestLockExclusive(t *testing.T) { - repo := repository.TestRepository(t) + repo := TestRepository(t) - elock, err := repository.TestNewLock(t, repo, true) + elock, err := NewLock(context.TODO(), &internalRepository{repo}, true) rtest.OK(t, err) rtest.OK(t, elock.Unlock(context.TODO())) } func TestLockOnExclusiveLockedRepo(t *testing.T) { - repo := repository.TestRepository(t) - restic.TestSetLockTimeout(t, 5*time.Millisecond) + repo := TestRepository(t) + TestSetLockTimeout(t, 5*time.Millisecond) - elock, err := repository.TestNewLock(t, repo, true) + elock, err := NewLock(context.TODO(), &internalRepository{repo}, true) rtest.OK(t, err) - lock, err := repository.TestNewLock(t, repo, false) + lock, err := NewLock(context.TODO(), &internalRepository{repo}, false) rtest.Assert(t, err != nil, "create normal lock with exclusively locked repo didn't return an error") - rtest.Assert(t, restic.IsAlreadyLocked(err), + rtest.Assert(t, IsAlreadyLocked(err), "create normal lock with exclusively locked repo didn't return the correct error") rtest.OK(t, lock.Unlock(context.TODO())) @@ -104,16 +103,16 @@ func TestLockOnExclusiveLockedRepo(t *testing.T) { } func TestExclusiveLockOnLockedRepo(t *testing.T) { - repo := repository.TestRepository(t) - restic.TestSetLockTimeout(t, 5*time.Millisecond) + repo := TestRepository(t) + TestSetLockTimeout(t, 5*time.Millisecond) - elock, err := repository.TestNewLock(t, repo, false) + elock, err := NewLock(context.TODO(), &internalRepository{repo}, false) rtest.OK(t, err) - lock, err := repository.TestNewLock(t, repo, true) + lock, err := NewLock(context.TODO(), &internalRepository{repo}, true) rtest.Assert(t, err != nil, "create normal lock with exclusively locked repo didn't return an error") - rtest.Assert(t, restic.IsAlreadyLocked(err), + rtest.Assert(t, IsAlreadyLocked(err), "create normal lock with exclusively locked repo didn't return the correct error") rtest.OK(t, lock.Unlock(context.TODO())) @@ -159,7 +158,7 @@ func TestLockStale(t *testing.T) { otherHostname := "other-" + hostname for i, test := range staleLockTests { - lock := restic.Lock{ + lock := Lock{ Time: test.timestamp, PID: test.pid, Hostname: hostname, @@ -195,11 +194,11 @@ func checkSingleLock(t *testing.T, repo restic.Lister) restic.ID { return *lockID } -func testLockRefresh(t *testing.T, refresh func(lock *restic.Lock) error) { - repo := repository.TestRepository(t) - restic.TestSetLockTimeout(t, 5*time.Millisecond) +func testLockRefresh(t *testing.T, refresh func(lock *Lock) error) { + repo := TestRepository(t) + TestSetLockTimeout(t, 5*time.Millisecond) - lock, err := repository.TestNewLock(t, repo, false) + lock, err := NewLock(context.TODO(), &internalRepository{repo}, false) rtest.OK(t, err) time0 := lock.Time @@ -212,7 +211,7 @@ func testLockRefresh(t *testing.T, refresh func(lock *restic.Lock) error) { rtest.Assert(t, !lockID.Equal(lockID2), "expected a new ID after lock refresh, got the same") - lock2, err := restic.LoadLock(context.TODO(), repo, lockID2) + lock2, err := LoadLock(context.TODO(), repo, lockID2) rtest.OK(t, err) rtest.Assert(t, lock2.Time.After(time0), "expected a later timestamp after lock refresh") @@ -220,22 +219,22 @@ func testLockRefresh(t *testing.T, refresh func(lock *restic.Lock) error) { } func TestLockRefresh(t *testing.T) { - testLockRefresh(t, func(lock *restic.Lock) error { + testLockRefresh(t, func(lock *Lock) error { return lock.Refresh(context.TODO()) }) } func TestLockRefreshStale(t *testing.T) { - testLockRefresh(t, func(lock *restic.Lock) error { + testLockRefresh(t, func(lock *Lock) error { return lock.RefreshStaleLock(context.TODO()) }) } func TestLockRefreshStaleMissing(t *testing.T) { - repo, _, be := repository.TestRepositoryWithVersion(t, 0) - restic.TestSetLockTimeout(t, 5*time.Millisecond) + repo, _, be := TestRepositoryWithVersion(t, 0) + TestSetLockTimeout(t, 5*time.Millisecond) - lock, err := repository.TestNewLock(t, repo, false) + lock, err := NewLock(context.TODO(), &internalRepository{repo}, false) rtest.OK(t, err) lockID := checkSingleLock(t, repo) @@ -243,5 +242,5 @@ func TestLockRefreshStaleMissing(t *testing.T) { rtest.OK(t, be.Remove(context.TODO(), backend.Handle{Type: restic.LockFile, Name: lockID.String()})) time.Sleep(time.Millisecond) err = lock.RefreshStaleLock(context.TODO()) - rtest.Assert(t, err == restic.ErrRemovedLock, "unexpected error, expected %v, got %v", restic.ErrRemovedLock, err) + rtest.Assert(t, err == ErrRemovedLock, "unexpected error, expected %v, got %v", ErrRemovedLock, err) } diff --git a/internal/restic/lock_unix.go b/internal/repository/lock_file_unix.go similarity index 97% rename from internal/restic/lock_unix.go rename to internal/repository/lock_file_unix.go index 71f43fd36..5a9d0f33d 100644 --- a/internal/restic/lock_unix.go +++ b/internal/repository/lock_file_unix.go @@ -1,6 +1,6 @@ //go:build !windows -package restic +package repository import ( "os" diff --git a/internal/restic/lock_windows.go b/internal/repository/lock_file_windows.go similarity index 96% rename from internal/restic/lock_windows.go rename to internal/repository/lock_file_windows.go index 442b08397..298d59e37 100644 --- a/internal/restic/lock_windows.go +++ b/internal/repository/lock_file_windows.go @@ -1,4 +1,4 @@ -package restic +package repository import ( "os" diff --git a/internal/repository/lock_test.go b/internal/repository/lock_test.go index 5a2813e75..a9c269738 100644 --- a/internal/repository/lock_test.go +++ b/internal/repository/lock_test.go @@ -38,7 +38,7 @@ func checkedLockRepo(ctx context.Context, t *testing.T, repo *Repository, locker lock, wrappedCtx, err := lockerInst.Lock(ctx, repo, false, retryLock, func(msg string) {}, func(format string, args ...interface{}) {}) rtest.OK(t, err) rtest.OK(t, wrappedCtx.Err()) - if lock.(*unlocker).info.lock.Stale() { + if lock.info.lock.Stale() { t.Fatal("lock returned stale lock") } return lock, wrappedCtx @@ -83,7 +83,7 @@ func TestLockConflict(t *testing.T) { if err == nil { t.Fatal("second lock should have failed") } - rtest.Assert(t, restic.IsAlreadyLocked(err), "unexpected error %v", err) + rtest.Assert(t, IsAlreadyLocked(err), "unexpected error %v", err) } type writeOnceBackend struct { @@ -309,8 +309,8 @@ func createFakeLock(repo *Repository, t time.Time, pid int) (restic.ID, error) { return restic.ID{}, err } - newLock := &restic.Lock{Time: t, PID: pid, Hostname: hostname} - return restic.SaveJSONUnpacked(context.TODO(), &internalRepository{repo}, restic.LockFile, &newLock) + newLock := &Lock{Time: t, PID: pid, Hostname: hostname} + return restic.SaveJSONUnpacked(context.TODO(), &internalRepository{repo}, restic.LockFile, newLock) } func lockExists(repo restic.Lister, t testing.TB, lockID restic.ID) bool { diff --git a/internal/repository/testing.go b/internal/repository/testing.go index 67b4b30ab..8ca2819f4 100644 --- a/internal/repository/testing.go +++ b/internal/repository/testing.go @@ -158,11 +158,6 @@ func BenchmarkAllVersions(b *testing.B, bench VersionedBenchmark) { } } -func TestNewLock(_ *testing.T, repo *Repository, exclusive bool) (*restic.Lock, error) { - // TODO get rid of this test helper - return restic.NewLock(context.TODO(), &internalRepository{repo}, exclusive) -} - // TestCheckRepo runs the checker on repo. func TestCheckRepo(t testing.TB, repo *Repository) { chkr := newChecker(repo) From 637c1cfb6629f57fa5621284dd0047937072baba Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sun, 7 Jun 2026 13:06:34 +0200 Subject: [PATCH 04/15] repository: unexport internal lock file helpers Unexport UnlockCancelDelay, IsInvalidLock, ErrRemovedLock, NewLock, StaleLockTimeout and ForAllLocks --- internal/repository/lock.go | 10 +++---- internal/repository/lock_file.go | 39 +++++++++++++-------------- internal/repository/lock_file_test.go | 28 +++++++++---------- 3 files changed, 38 insertions(+), 39 deletions(-) diff --git a/internal/repository/lock.go b/internal/repository/lock.go index 488aca022..a91341f42 100644 --- a/internal/repository/lock.go +++ b/internal/repository/lock.go @@ -34,7 +34,7 @@ var lockerInst = &locker{ refreshInterval: defaultRefreshInterval, // consider a lock refresh failed a bit before the lock actually becomes stale // the difference allows to compensate for a small time drift between clients. - refreshabilityTimeout: StaleLockTimeout - defaultRefreshInterval*3/2, + refreshabilityTimeout: staleLockTimeout - defaultRefreshInterval*3/2, } func LockRepo(ctx context.Context, repo *Repository, exclusive bool, retryLock time.Duration, printRetry func(msg string), logger func(format string, args ...interface{})) (Unlocker, context.Context, error) { @@ -55,7 +55,7 @@ func (l *locker) Lock(ctx context.Context, r *Repository, exclusive bool, retryL retryLoop: for { - lock, err = NewLock(ctx, repo, exclusive) + lock, err = newLock(ctx, repo, exclusive) if err != nil && IsAlreadyLocked(err) { if !retryMessagePrinted { @@ -72,7 +72,7 @@ retryLoop: case <-retryTimeout: debug.Log("repo already locked, timeout expired") // Last lock attempt - lock, err = NewLock(ctx, repo, exclusive) + lock, err = newLock(ctx, repo, exclusive) break retryLoop case <-retrySleepCh: retrySleep = minDuration(retrySleep*2, l.retrySleepMax) @@ -82,7 +82,7 @@ retryLoop: break retryLoop } } - if IsInvalidLock(err) { + if 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 { @@ -277,7 +277,7 @@ func (l *unlocker) Unlock() { // RemoveStaleLocks deletes all locks detected as stale from the repository. func RemoveStaleLocks(ctx context.Context, repo *Repository) (uint, error) { var processed uint - err := ForAllLocks(ctx, repo, nil, func(id restic.ID, lock *Lock, err error) error { + err := forAllLocks(ctx, repo, nil, func(id restic.ID, lock *Lock, err error) error { if err != nil { // ignore locks that cannot be loaded debug.Log("ignore lock %v: %v", id, err) diff --git a/internal/repository/lock_file.go b/internal/repository/lock_file.go index b0a0b3ede..78d60979f 100644 --- a/internal/repository/lock_file.go +++ b/internal/repository/lock_file.go @@ -16,9 +16,9 @@ import ( "github.com/restic/restic/internal/restic" ) -// UnlockCancelDelay bounds the duration how long lock cleanup operations will wait +// unlockCancelDelay bounds the duration how long lock cleanup operations will wait // if the passed in context was canceled. -const UnlockCancelDelay = 1 * time.Minute +const unlockCancelDelay = 1 * time.Minute // Lock represents a process locking the repository for an operation. // @@ -63,8 +63,7 @@ func IsAlreadyLocked(err error) bool { return errors.As(err, &e) } -// invalidLockError is returned when NewLock or NewExclusiveLock fail due -// to an invalid lock. +// invalidLockError is returned when newLock fails due to an invalid lock. type invalidLockError struct { err error } @@ -77,14 +76,14 @@ func (e *invalidLockError) Unwrap() error { return e.err } -// IsInvalidLock returns true iff err indicates that locking failed due to +// isInvalidLock returns true iff err indicates that locking failed due to // an invalid lock. -func IsInvalidLock(err error) bool { +func isInvalidLock(err error) bool { var e *invalidLockError return errors.As(err, &e) } -var ErrRemovedLock = errors.New("lock file was removed in the meantime") +var errRemovedLock = errors.New("lock file was removed in the meantime") var waitBeforeLockCheck = 200 * time.Millisecond @@ -98,11 +97,11 @@ func TestSetLockTimeout(t testing.TB, d time.Duration) { initialWaitBetweenLockRetries = d } -// NewLock returns a new lock for the repository. If an +// newLock returns a new lock for the repository. If an // exclusive lock is already held by another process, it returns an error -// that satisfies IsAlreadyLocked. If the new lock is exclude, then other +// that satisfies IsAlreadyLocked. If the new lock is exclusive, then other // non-exclusive locks also result in an IsAlreadyLocked error. -func NewLock(ctx context.Context, repo restic.Unpacked[restic.FileType], exclusive bool) (*Lock, error) { +func newLock(ctx context.Context, repo restic.Unpacked[restic.FileType], exclusive bool) (*Lock, error) { lock := &Lock{ Time: time.Now(), PID: os.Getpid(), @@ -177,7 +176,7 @@ func (l *Lock) checkForOtherLocks(ctx context.Context) error { // Store updates in new IDSet to prevent data races var m sync.Mutex newCheckedIDs := checkedIDs.Clone() - err = ForAllLocks(ctx, l.repo, checkedIDs, func(id restic.ID, lock *Lock, err error) error { + err = forAllLocks(ctx, l.repo, checkedIDs, func(id restic.ID, lock *Lock, err error) error { if err != nil { // 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 @@ -243,13 +242,13 @@ func (l *Lock) Unlock(ctx context.Context) error { return nil } - ctx, cancel := delayedCancelContext(ctx, UnlockCancelDelay) + ctx, cancel := delayedCancelContext(ctx, unlockCancelDelay) defer cancel() return l.repo.RemoveUnpacked(ctx, restic.LockFile, *l.lockID) } -var StaleLockTimeout = 30 * time.Minute +var staleLockTimeout = 30 * time.Minute // Stale returns true if the lock is stale. A lock is stale if the timestamp is // older than 30 minutes or if it was created on the current machine and the @@ -258,7 +257,7 @@ func (l *Lock) Stale() bool { l.lock.Lock() defer l.lock.Unlock() debug.Log("testing if lock %v for process %d is stale", l.lockID, l.PID) - if time.Since(l.Time) > StaleLockTimeout { + if time.Since(l.Time) > staleLockTimeout { debug.Log("lock is stale, timestamp is too old: %v\n", l.Time) return true } @@ -323,7 +322,7 @@ func (l *Lock) Refresh(ctx context.Context) error { oldLockID := l.lockID l.lockID = &id - ctx, cancel := delayedCancelContext(ctx, UnlockCancelDelay) + ctx, cancel := delayedCancelContext(ctx, unlockCancelDelay) defer cancel() return l.repo.RemoveUnpacked(ctx, restic.LockFile, *oldLockID) @@ -339,7 +338,7 @@ func (l *Lock) RefreshStaleLock(ctx context.Context) error { if err != nil { return err } else if !exists { - return ErrRemovedLock + return errRemovedLock } l.lock.Lock() @@ -354,7 +353,7 @@ func (l *Lock) RefreshStaleLock(ctx context.Context) error { exists, err = l.checkExistence(ctx) - ctx, cancel := delayedCancelContext(ctx, UnlockCancelDelay) + ctx, cancel := delayedCancelContext(ctx, unlockCancelDelay) defer cancel() if err != nil { @@ -366,7 +365,7 @@ func (l *Lock) RefreshStaleLock(ctx context.Context) error { if !exists { // cleanup replacement lock _ = l.repo.RemoveUnpacked(ctx, restic.LockFile, id) - return ErrRemovedLock + return errRemovedLock } l.lock.Lock() @@ -433,11 +432,11 @@ func LoadLock(ctx context.Context, repo restic.LoaderUnpacked, id restic.ID) (*L return lock, nil } -// ForAllLocks reads all locks in parallel and calls the given callback. +// forAllLocks reads all locks in parallel and calls the given callback. // It is guaranteed that the function is not run concurrently. If the // callback returns an error, this function is cancelled and also returns that error. // If a lock ID is passed via excludeID, it will be ignored. -func ForAllLocks(ctx context.Context, repo restic.ListerLoaderUnpacked, excludeIDs restic.IDSet, fn func(restic.ID, *Lock, error) error) error { +func forAllLocks(ctx context.Context, repo restic.ListerLoaderUnpacked, excludeIDs restic.IDSet, fn func(restic.ID, *Lock, error) error) error { var m sync.Mutex // For locks decoding is nearly for free, thus just assume were only limited by IO diff --git a/internal/repository/lock_file_test.go b/internal/repository/lock_file_test.go index 5443c3ca7..1e2256860 100644 --- a/internal/repository/lock_file_test.go +++ b/internal/repository/lock_file_test.go @@ -18,7 +18,7 @@ func TestLockFile(t *testing.T) { repo := TestRepository(t) TestSetLockTimeout(t, 5*time.Millisecond) - lock, err := NewLock(context.TODO(), &internalRepository{repo}, false) + lock, err := newLock(context.TODO(), &internalRepository{repo}, false) rtest.OK(t, err) rtest.OK(t, lock.Unlock(context.TODO())) @@ -28,7 +28,7 @@ func TestDoubleUnlock(t *testing.T) { repo := TestRepository(t) TestSetLockTimeout(t, 5*time.Millisecond) - lock, err := NewLock(context.TODO(), &internalRepository{repo}, false) + lock, err := newLock(context.TODO(), &internalRepository{repo}, false) rtest.OK(t, err) rtest.OK(t, lock.Unlock(context.TODO())) @@ -42,10 +42,10 @@ func TestMultipleLock(t *testing.T) { repo := TestRepository(t) TestSetLockTimeout(t, 5*time.Millisecond) - lock1, err := NewLock(context.TODO(), &internalRepository{repo}, false) + lock1, err := newLock(context.TODO(), &internalRepository{repo}, false) rtest.OK(t, err) - lock2, err := NewLock(context.TODO(), &internalRepository{repo}, false) + lock2, err := newLock(context.TODO(), &internalRepository{repo}, false) rtest.OK(t, err) rtest.OK(t, lock1.Unlock(context.TODO())) @@ -68,10 +68,10 @@ func TestMultipleLockFailure(t *testing.T) { repo, _ := TestRepositoryWithBackend(t, be, 0, Options{}) TestSetLockTimeout(t, 5*time.Millisecond) - lock1, err := NewLock(context.TODO(), &internalRepository{repo}, false) + lock1, err := newLock(context.TODO(), &internalRepository{repo}, false) rtest.OK(t, err) - _, err = NewLock(context.TODO(), &internalRepository{repo}, false) + _, err = newLock(context.TODO(), &internalRepository{repo}, false) rtest.Assert(t, err != nil, "unreadable lock file did not result in an error") rtest.OK(t, lock1.Unlock(context.TODO())) @@ -80,7 +80,7 @@ func TestMultipleLockFailure(t *testing.T) { func TestLockExclusive(t *testing.T) { repo := TestRepository(t) - elock, err := NewLock(context.TODO(), &internalRepository{repo}, true) + elock, err := newLock(context.TODO(), &internalRepository{repo}, true) rtest.OK(t, err) rtest.OK(t, elock.Unlock(context.TODO())) } @@ -89,10 +89,10 @@ func TestLockOnExclusiveLockedRepo(t *testing.T) { repo := TestRepository(t) TestSetLockTimeout(t, 5*time.Millisecond) - elock, err := NewLock(context.TODO(), &internalRepository{repo}, true) + elock, err := newLock(context.TODO(), &internalRepository{repo}, true) rtest.OK(t, err) - lock, err := NewLock(context.TODO(), &internalRepository{repo}, false) + lock, err := newLock(context.TODO(), &internalRepository{repo}, false) rtest.Assert(t, err != nil, "create normal lock with exclusively locked repo didn't return an error") rtest.Assert(t, IsAlreadyLocked(err), @@ -106,10 +106,10 @@ func TestExclusiveLockOnLockedRepo(t *testing.T) { repo := TestRepository(t) TestSetLockTimeout(t, 5*time.Millisecond) - elock, err := NewLock(context.TODO(), &internalRepository{repo}, false) + elock, err := newLock(context.TODO(), &internalRepository{repo}, false) rtest.OK(t, err) - lock, err := NewLock(context.TODO(), &internalRepository{repo}, true) + lock, err := newLock(context.TODO(), &internalRepository{repo}, true) rtest.Assert(t, err != nil, "create normal lock with exclusively locked repo didn't return an error") rtest.Assert(t, IsAlreadyLocked(err), @@ -198,7 +198,7 @@ func testLockRefresh(t *testing.T, refresh func(lock *Lock) error) { repo := TestRepository(t) TestSetLockTimeout(t, 5*time.Millisecond) - lock, err := NewLock(context.TODO(), &internalRepository{repo}, false) + lock, err := newLock(context.TODO(), &internalRepository{repo}, false) rtest.OK(t, err) time0 := lock.Time @@ -234,7 +234,7 @@ func TestLockRefreshStaleMissing(t *testing.T) { repo, _, be := TestRepositoryWithVersion(t, 0) TestSetLockTimeout(t, 5*time.Millisecond) - lock, err := NewLock(context.TODO(), &internalRepository{repo}, false) + lock, err := newLock(context.TODO(), &internalRepository{repo}, false) rtest.OK(t, err) lockID := checkSingleLock(t, repo) @@ -242,5 +242,5 @@ func TestLockRefreshStaleMissing(t *testing.T) { rtest.OK(t, be.Remove(context.TODO(), backend.Handle{Type: restic.LockFile, Name: lockID.String()})) time.Sleep(time.Millisecond) err = lock.RefreshStaleLock(context.TODO()) - rtest.Assert(t, err == ErrRemovedLock, "unexpected error, expected %v, got %v", ErrRemovedLock, err) + rtest.Assert(t, err == errRemovedLock, "unexpected error, expected %v, got %v", errRemovedLock, err) } From 81b6414c559de5a779a71ec49d9801049740118a Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sun, 7 Jun 2026 13:06:40 +0200 Subject: [PATCH 05/15] repository: hide Lock methods Unexport lock file methods except String. Lock file operations are only used within the repository package. --- internal/repository/lock.go | 8 +++---- internal/repository/lock_file.go | 18 +++++++------- internal/repository/lock_file_test.go | 34 +++++++++++++-------------- internal/repository/lock_test.go | 2 +- 4 files changed, 31 insertions(+), 31 deletions(-) diff --git a/internal/repository/lock.go b/internal/repository/lock.go index a91341f42..303ef082e 100644 --- a/internal/repository/lock.go +++ b/internal/repository/lock.go @@ -129,7 +129,7 @@ func (l *locker) refreshLocks(ctx context.Context, backend backend.Backend, lock // remove the lock from the repo debug.Log("unlocking repository with lock %v", lock) - if err := lock.Unlock(ctx); err != nil { + if err := lock.unlock(ctx); err != nil { debug.Log("error while unlocking: %v", err) logger("error while unlocking: %v", err) } @@ -165,7 +165,7 @@ func (l *locker) refreshLocks(ctx context.Context, backend backend.Backend, lock } debug.Log("refreshing locks") - err := lock.Refresh(context.TODO()) + err := lock.refresh(context.TODO()) if err != nil { logger("unable to refresh lock: %v\n", err) } else { @@ -250,7 +250,7 @@ func tryRefreshStaleLock(ctx context.Context, be backend.Backend, lock *Lock, ca defer freeze.Unfreeze() } - err := lock.RefreshStaleLock(ctx) + err := lock.refreshStaleLock(ctx) if err != nil { logger("failed to refresh stale lock: %v\n", err) // cancel context while the backend is still frozen to prevent accidental modifications @@ -284,7 +284,7 @@ func RemoveStaleLocks(ctx context.Context, repo *Repository) (uint, error) { return nil } - if lock.Stale() { + if lock.stale() { err = (&internalRepository{repo}).RemoveUnpacked(ctx, restic.LockFile, id) if err == nil { processed++ diff --git a/internal/repository/lock_file.go b/internal/repository/lock_file.go index 78d60979f..b2e28435c 100644 --- a/internal/repository/lock_file.go +++ b/internal/repository/lock_file.go @@ -132,7 +132,7 @@ func newLock(ctx context.Context, repo restic.Unpacked[restic.FileType], exclusi time.Sleep(waitBeforeLockCheck) if err = lock.checkForOtherLocks(ctx); err != nil { - _ = lock.Unlock(ctx) + _ = lock.unlock(ctx) return nil, err } @@ -236,8 +236,8 @@ func (l *Lock) createLock(ctx context.Context) (restic.ID, error) { return id, nil } -// Unlock removes the lock from the repository. -func (l *Lock) Unlock(ctx context.Context) error { +// unlock removes the lock from the repository. +func (l *Lock) unlock(ctx context.Context) error { if l == nil || l.lockID == nil { return nil } @@ -250,10 +250,10 @@ func (l *Lock) Unlock(ctx context.Context) error { var staleLockTimeout = 30 * time.Minute -// Stale returns true if the lock is stale. A lock is stale if the timestamp is +// stale returns true if the lock is stale. A lock is stale if the timestamp is // older than 30 minutes or if it was created on the current machine and the // process isn't alive any more. -func (l *Lock) Stale() bool { +func (l *Lock) stale() bool { l.lock.Lock() defer l.lock.Unlock() debug.Log("testing if lock %v for process %d is stale", l.lockID, l.PID) @@ -303,9 +303,9 @@ func delayedCancelContext(parentCtx context.Context, delay time.Duration) (conte return ctx, cancel } -// Refresh refreshes the lock by creating a new file in the backend with a new +// refresh refreshes the lock by creating a new file in the backend with a new // timestamp. Afterwards the old lock is removed. -func (l *Lock) Refresh(ctx context.Context) error { +func (l *Lock) refresh(ctx context.Context) error { debug.Log("refreshing lock %v", l.lockID) l.lock.Lock() l.Time = time.Now() @@ -328,8 +328,8 @@ func (l *Lock) Refresh(ctx context.Context) error { return l.repo.RemoveUnpacked(ctx, restic.LockFile, *oldLockID) } -// RefreshStaleLock is an extended variant of Refresh that can also refresh stale lock files. -func (l *Lock) RefreshStaleLock(ctx context.Context) error { +// refreshStaleLock is an extended variant of refresh that can also refresh stale lock files. +func (l *Lock) refreshStaleLock(ctx context.Context) error { debug.Log("refreshing stale lock %v", l.lockID) // refreshing a stale lock is possible if it still exists and continues to do // so until after creating a new lock. The initial check avoids creating a new diff --git a/internal/repository/lock_file_test.go b/internal/repository/lock_file_test.go index 1e2256860..af3cbc689 100644 --- a/internal/repository/lock_file_test.go +++ b/internal/repository/lock_file_test.go @@ -21,7 +21,7 @@ func TestLockFile(t *testing.T) { lock, err := newLock(context.TODO(), &internalRepository{repo}, false) rtest.OK(t, err) - rtest.OK(t, lock.Unlock(context.TODO())) + rtest.OK(t, lock.unlock(context.TODO())) } func TestDoubleUnlock(t *testing.T) { @@ -31,9 +31,9 @@ func TestDoubleUnlock(t *testing.T) { lock, err := newLock(context.TODO(), &internalRepository{repo}, false) rtest.OK(t, err) - rtest.OK(t, lock.Unlock(context.TODO())) + rtest.OK(t, lock.unlock(context.TODO())) - err = lock.Unlock(context.TODO()) + err = lock.unlock(context.TODO()) rtest.Assert(t, err != nil, "double unlock didn't return an error, got %v", err) } @@ -48,8 +48,8 @@ func TestMultipleLock(t *testing.T) { lock2, err := newLock(context.TODO(), &internalRepository{repo}, false) rtest.OK(t, err) - rtest.OK(t, lock1.Unlock(context.TODO())) - rtest.OK(t, lock2.Unlock(context.TODO())) + rtest.OK(t, lock1.unlock(context.TODO())) + rtest.OK(t, lock2.unlock(context.TODO())) } type failLockLoadingBackend struct { @@ -74,7 +74,7 @@ func TestMultipleLockFailure(t *testing.T) { _, err = newLock(context.TODO(), &internalRepository{repo}, false) rtest.Assert(t, err != nil, "unreadable lock file did not result in an error") - rtest.OK(t, lock1.Unlock(context.TODO())) + rtest.OK(t, lock1.unlock(context.TODO())) } func TestLockExclusive(t *testing.T) { @@ -82,7 +82,7 @@ func TestLockExclusive(t *testing.T) { elock, err := newLock(context.TODO(), &internalRepository{repo}, true) rtest.OK(t, err) - rtest.OK(t, elock.Unlock(context.TODO())) + rtest.OK(t, elock.unlock(context.TODO())) } func TestLockOnExclusiveLockedRepo(t *testing.T) { @@ -98,8 +98,8 @@ func TestLockOnExclusiveLockedRepo(t *testing.T) { rtest.Assert(t, IsAlreadyLocked(err), "create normal lock with exclusively locked repo didn't return the correct error") - rtest.OK(t, lock.Unlock(context.TODO())) - rtest.OK(t, elock.Unlock(context.TODO())) + rtest.OK(t, lock.unlock(context.TODO())) + rtest.OK(t, elock.unlock(context.TODO())) } func TestExclusiveLockOnLockedRepo(t *testing.T) { @@ -115,8 +115,8 @@ func TestExclusiveLockOnLockedRepo(t *testing.T) { rtest.Assert(t, IsAlreadyLocked(err), "create normal lock with exclusively locked repo didn't return the correct error") - rtest.OK(t, lock.Unlock(context.TODO())) - rtest.OK(t, elock.Unlock(context.TODO())) + rtest.OK(t, lock.unlock(context.TODO())) + rtest.OK(t, elock.unlock(context.TODO())) } var staleLockTests = []struct { @@ -164,12 +164,12 @@ func TestLockStale(t *testing.T) { Hostname: hostname, } - rtest.Assert(t, lock.Stale() == test.stale, + rtest.Assert(t, lock.stale() == test.stale, "TestStaleLock: test %d failed: expected stale: %v, got %v", i, test.stale, !test.stale) lock.Hostname = otherHostname - rtest.Assert(t, lock.Stale() == test.staleOnOtherHost, + rtest.Assert(t, lock.stale() == test.staleOnOtherHost, "TestStaleLock: test %d failed: expected staleOnOtherHost: %v, got %v", i, test.staleOnOtherHost, !test.staleOnOtherHost) } @@ -215,18 +215,18 @@ func testLockRefresh(t *testing.T, refresh func(lock *Lock) error) { rtest.OK(t, err) rtest.Assert(t, lock2.Time.After(time0), "expected a later timestamp after lock refresh") - rtest.OK(t, lock.Unlock(context.TODO())) + rtest.OK(t, lock.unlock(context.TODO())) } func TestLockRefresh(t *testing.T) { testLockRefresh(t, func(lock *Lock) error { - return lock.Refresh(context.TODO()) + return lock.refresh(context.TODO()) }) } func TestLockRefreshStale(t *testing.T) { testLockRefresh(t, func(lock *Lock) error { - return lock.RefreshStaleLock(context.TODO()) + return lock.refreshStaleLock(context.TODO()) }) } @@ -241,6 +241,6 @@ func TestLockRefreshStaleMissing(t *testing.T) { // refresh must fail if lock was removed rtest.OK(t, be.Remove(context.TODO(), backend.Handle{Type: restic.LockFile, Name: lockID.String()})) time.Sleep(time.Millisecond) - err = lock.RefreshStaleLock(context.TODO()) + err = lock.refreshStaleLock(context.TODO()) rtest.Assert(t, err == errRemovedLock, "unexpected error, expected %v, got %v", errRemovedLock, err) } diff --git a/internal/repository/lock_test.go b/internal/repository/lock_test.go index a9c269738..2fb9025a1 100644 --- a/internal/repository/lock_test.go +++ b/internal/repository/lock_test.go @@ -38,7 +38,7 @@ func checkedLockRepo(ctx context.Context, t *testing.T, repo *Repository, locker lock, wrappedCtx, err := lockerInst.Lock(ctx, repo, false, retryLock, func(msg string) {}, func(format string, args ...interface{}) {}) rtest.OK(t, err) rtest.OK(t, wrappedCtx.Err()) - if lock.info.lock.Stale() { + if lock.info.lock.stale() { t.Fatal("lock returned stale lock") } return lock, wrappedCtx From c87da70af9c99cdca2848ed13949bf3c788749d1 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sun, 7 Jun 2026 13:34:36 +0200 Subject: [PATCH 06/15] repository: separate Lock in-repository from lock handle Reduce Lock to a pure data transfer object and move the logic to lockHandle. --- internal/repository/lock.go | 8 +- internal/repository/lock_file.go | 101 ++++++++++++----------- internal/repository/lock_file_test.go | 16 ++-- internal/repository/lock_file_unix.go | 2 +- internal/repository/lock_file_windows.go | 2 +- 5 files changed, 67 insertions(+), 62 deletions(-) diff --git a/internal/repository/lock.go b/internal/repository/lock.go index 303ef082e..6d9fd6302 100644 --- a/internal/repository/lock.go +++ b/internal/repository/lock.go @@ -14,7 +14,7 @@ import ( ) type lockContext struct { - lock *Lock + lock *lockHandle cancel context.CancelFunc refreshWG sync.WaitGroup } @@ -44,7 +44,7 @@ func LockRepo(ctx context.Context, repo *Repository, exclusive bool, retryLock t // Lock wraps the ctx such that it is cancelled when the repository is unlocked // cancelling the original context also stops the lock refresh func (l *locker) Lock(ctx context.Context, r *Repository, exclusive bool, retryLock time.Duration, printRetry func(msg string), logger func(format string, args ...interface{})) (*unlocker, context.Context, error) { - var lock *Lock + var lock *lockHandle var err error retrySleep := minDuration(l.retrySleepStart, retryLock) @@ -242,7 +242,7 @@ func (l *locker) monitorLockRefresh(ctx context.Context, lockInfo *lockContext, } } -func tryRefreshStaleLock(ctx context.Context, be backend.Backend, lock *Lock, cancel context.CancelFunc, logger func(format string, args ...interface{})) bool { +func tryRefreshStaleLock(ctx context.Context, be backend.Backend, lock *lockHandle, cancel context.CancelFunc, logger func(format string, args ...interface{})) bool { freeze := backend.AsBackend[backend.FreezeBackend](be) if freeze != nil { debug.Log("freezing backend") @@ -277,7 +277,7 @@ func (l *unlocker) Unlock() { // RemoveStaleLocks deletes all locks detected as stale from the repository. func RemoveStaleLocks(ctx context.Context, repo *Repository) (uint, error) { var processed uint - err := forAllLocks(ctx, repo, nil, func(id restic.ID, lock *Lock, err error) error { + err := forAllLocks(ctx, repo, nil, func(id restic.ID, lock *lockHandle, err error) error { if err != nil { // ignore locks that cannot be loaded debug.Log("ignore lock %v: %v", id, err) diff --git a/internal/repository/lock_file.go b/internal/repository/lock_file.go index b2e28435c..7cf5c549e 100644 --- a/internal/repository/lock_file.go +++ b/internal/repository/lock_file.go @@ -20,16 +20,13 @@ import ( // if the passed in context was canceled. const unlockCancelDelay = 1 * time.Minute -// Lock represents a process locking the repository for an operation. -// +// Lock is the in-repository representation of a repository lock file. // There are two types of locks: exclusive and non-exclusive. There may be many // different non-exclusive locks, but at most one exclusive lock, which can // only be acquired while no non-exclusive lock is held. // -// A lock must be refreshed regularly to not be considered stale, this must be -// triggered by regularly calling Refresh. +// A lock must be refreshed regularly to not be considered stale. type Lock struct { - lock sync.Mutex Time time.Time `json:"time"` Exclusive bool `json:"exclusive"` Hostname string `json:"hostname"` @@ -37,15 +34,19 @@ type Lock struct { PID int `json:"pid"` UID uint32 `json:"uid,omitempty"` GID uint32 `json:"gid,omitempty"` +} +// lockHandle is a reference to a lock file in the repository. +type lockHandle struct { + mu sync.Mutex + Lock repo restic.Unpacked[restic.FileType] lockID *restic.ID } -// alreadyLockedError is returned when NewLock or NewExclusiveLock are unable to -// acquire the desired lock. +// alreadyLockedError is returned when newLock is unable to acquire the desired lock. type alreadyLockedError struct { - otherLock *Lock + otherLock *lockHandle } func (e *alreadyLockedError) Error() string { @@ -101,12 +102,14 @@ func TestSetLockTimeout(t testing.TB, d time.Duration) { // exclusive lock is already held by another process, it returns an error // that satisfies IsAlreadyLocked. If the new lock is exclusive, then other // non-exclusive locks also result in an IsAlreadyLocked error. -func newLock(ctx context.Context, repo restic.Unpacked[restic.FileType], exclusive bool) (*Lock, error) { - lock := &Lock{ - Time: time.Now(), - PID: os.Getpid(), - Exclusive: exclusive, - repo: repo, +func newLock(ctx context.Context, repo restic.Unpacked[restic.FileType], exclusive bool) (*lockHandle, error) { + lock := &lockHandle{ + Lock: Lock{ + Time: time.Now(), + PID: os.Getpid(), + Exclusive: exclusive, + }, + repo: repo, } hn, err := os.Hostname() @@ -139,7 +142,7 @@ func newLock(ctx context.Context, repo restic.Unpacked[restic.FileType], exclusi return lock, nil } -func (l *Lock) fillUserInfo() error { +func (l *lockHandle) fillUserInfo() error { usr, err := user.Current() if err != nil { return nil @@ -156,7 +159,7 @@ func (l *Lock) fillUserInfo() error { // if there are any other locks, regardless if exclusive or not. If a // non-exclusive lock is to be created, an error is only returned when an // exclusive lock is found. -func (l *Lock) checkForOtherLocks(ctx context.Context) error { +func (l *lockHandle) checkForOtherLocks(ctx context.Context) error { var err error checkedIDs := restic.NewIDSet() if l.lockID != nil { @@ -176,7 +179,7 @@ func (l *Lock) checkForOtherLocks(ctx context.Context) error { // Store updates in new IDSet to prevent data races var m sync.Mutex newCheckedIDs := checkedIDs.Clone() - err = forAllLocks(ctx, l.repo, checkedIDs, func(id restic.ID, lock *Lock, err error) error { + err = forAllLocks(ctx, l.repo, checkedIDs, func(id restic.ID, lock *lockHandle, err error) error { if err != nil { // 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 @@ -227,8 +230,8 @@ func cancelableDelay(ctx context.Context, delay time.Duration) error { } // createLock acquires the lock by creating a file in the repository. -func (l *Lock) createLock(ctx context.Context) (restic.ID, error) { - id, err := restic.SaveJSONUnpacked(ctx, l.repo, restic.LockFile, l) +func (l *lockHandle) createLock(ctx context.Context) (restic.ID, error) { + id, err := restic.SaveJSONUnpacked(ctx, l.repo, restic.LockFile, &l.Lock) if err != nil { return restic.ID{}, err } @@ -237,7 +240,7 @@ func (l *Lock) createLock(ctx context.Context) (restic.ID, error) { } // unlock removes the lock from the repository. -func (l *Lock) unlock(ctx context.Context) error { +func (l *lockHandle) unlock(ctx context.Context) error { if l == nil || l.lockID == nil { return nil } @@ -253,9 +256,9 @@ var staleLockTimeout = 30 * time.Minute // stale returns true if the lock is stale. A lock is stale if the timestamp is // older than 30 minutes or if it was created on the current machine and the // process isn't alive any more. -func (l *Lock) stale() bool { - l.lock.Lock() - defer l.lock.Unlock() +func (l *lockHandle) stale() bool { + l.mu.Lock() + defer l.mu.Unlock() debug.Log("testing if lock %v for process %d is stale", l.lockID, l.PID) if time.Since(l.Time) > staleLockTimeout { debug.Log("lock is stale, timestamp is too old: %v\n", l.Time) @@ -305,18 +308,18 @@ func delayedCancelContext(parentCtx context.Context, delay time.Duration) (conte // refresh refreshes the lock by creating a new file in the backend with a new // timestamp. Afterwards the old lock is removed. -func (l *Lock) refresh(ctx context.Context) error { +func (l *lockHandle) refresh(ctx context.Context) error { debug.Log("refreshing lock %v", l.lockID) - l.lock.Lock() + l.mu.Lock() l.Time = time.Now() - l.lock.Unlock() + l.mu.Unlock() id, err := l.createLock(ctx) if err != nil { return err } - l.lock.Lock() - defer l.lock.Unlock() + l.mu.Lock() + defer l.mu.Unlock() debug.Log("new lock ID %v", id) oldLockID := l.lockID @@ -329,7 +332,7 @@ func (l *Lock) refresh(ctx context.Context) error { } // refreshStaleLock is an extended variant of refresh that can also refresh stale lock files. -func (l *Lock) refreshStaleLock(ctx context.Context) error { +func (l *lockHandle) refreshStaleLock(ctx context.Context) error { debug.Log("refreshing stale lock %v", l.lockID) // refreshing a stale lock is possible if it still exists and continues to do // so until after creating a new lock. The initial check avoids creating a new @@ -341,9 +344,9 @@ func (l *Lock) refreshStaleLock(ctx context.Context) error { return errRemovedLock } - l.lock.Lock() + l.mu.Lock() l.Time = time.Now() - l.lock.Unlock() + l.mu.Unlock() id, err := l.createLock(ctx) if err != nil { return err @@ -368,8 +371,8 @@ func (l *Lock) refreshStaleLock(ctx context.Context) error { return errRemovedLock } - l.lock.Lock() - defer l.lock.Unlock() + l.mu.Lock() + defer l.mu.Unlock() debug.Log("new lock ID %v", id) oldLockID := l.lockID @@ -378,9 +381,9 @@ func (l *Lock) refreshStaleLock(ctx context.Context) error { return l.repo.RemoveUnpacked(ctx, restic.LockFile, *oldLockID) } -func (l *Lock) checkExistence(ctx context.Context) (bool, error) { - l.lock.Lock() - defer l.lock.Unlock() +func (l *lockHandle) checkExistence(ctx context.Context) (bool, error) { + l.mu.Lock() + defer l.mu.Unlock() exists := false @@ -394,9 +397,9 @@ func (l *Lock) checkExistence(ctx context.Context) (bool, error) { return exists, err } -func (l *Lock) String() string { - l.lock.Lock() - defer l.lock.Unlock() +func (l *lockHandle) String() string { + l.mu.Lock() + defer l.mu.Unlock() text := fmt.Sprintf("PID %d on %s by %s (UID %d, GID %d)\nlock was created at %s (%s ago)\nstorage ID %v", l.PID, l.Hostname, l.Username, l.UID, l.GID, @@ -422,21 +425,17 @@ func init() { } // LoadLock loads and unserializes a lock from a repository. -func LoadLock(ctx context.Context, repo restic.LoaderUnpacked, id restic.ID) (*Lock, error) { - lock := &Lock{} - if err := restic.LoadJSONUnpacked(ctx, repo, restic.LockFile, id, lock); err != nil { - return nil, err - } - lock.lockID = &id - - return lock, nil +func LoadLock(ctx context.Context, repo restic.LoaderUnpacked, id restic.ID) (Lock, error) { + var lock Lock + err := restic.LoadJSONUnpacked(ctx, repo, restic.LockFile, id, &lock) + return lock, err } // forAllLocks reads all locks in parallel and calls the given callback. // It is guaranteed that the function is not run concurrently. If the // callback returns an error, this function is cancelled and also returns that error. // If a lock ID is passed via excludeID, it will be ignored. -func forAllLocks(ctx context.Context, repo restic.ListerLoaderUnpacked, excludeIDs restic.IDSet, fn func(restic.ID, *Lock, error) error) error { +func forAllLocks(ctx context.Context, repo restic.ListerLoaderUnpacked, excludeIDs restic.IDSet, fn func(restic.ID, *lockHandle, error) error) error { var m sync.Mutex // For locks decoding is nearly for free, thus just assume were only limited by IO @@ -451,9 +450,13 @@ func forAllLocks(ctx context.Context, repo restic.ListerLoaderUnpacked, excludeI return nil } lock, err := LoadLock(ctx, repo, id) + var handle *lockHandle + if err == nil { + handle = &lockHandle{Lock: lock, lockID: &id} + } m.Lock() defer m.Unlock() - return fn(id, lock, err) + return fn(id, handle, err) }) } diff --git a/internal/repository/lock_file_test.go b/internal/repository/lock_file_test.go index af3cbc689..2c4790caf 100644 --- a/internal/repository/lock_file_test.go +++ b/internal/repository/lock_file_test.go @@ -158,10 +158,12 @@ func TestLockStale(t *testing.T) { otherHostname := "other-" + hostname for i, test := range staleLockTests { - lock := Lock{ - Time: test.timestamp, - PID: test.pid, - Hostname: hostname, + lock := lockHandle{ + Lock: Lock{ + Time: test.timestamp, + PID: test.pid, + Hostname: hostname, + }, } rtest.Assert(t, lock.stale() == test.stale, @@ -194,7 +196,7 @@ func checkSingleLock(t *testing.T, repo restic.Lister) restic.ID { return *lockID } -func testLockRefresh(t *testing.T, refresh func(lock *Lock) error) { +func testLockRefresh(t *testing.T, refresh func(lock *lockHandle) error) { repo := TestRepository(t) TestSetLockTimeout(t, 5*time.Millisecond) @@ -219,13 +221,13 @@ func testLockRefresh(t *testing.T, refresh func(lock *Lock) error) { } func TestLockRefresh(t *testing.T) { - testLockRefresh(t, func(lock *Lock) error { + testLockRefresh(t, func(lock *lockHandle) error { return lock.refresh(context.TODO()) }) } func TestLockRefreshStale(t *testing.T) { - testLockRefresh(t, func(lock *Lock) error { + testLockRefresh(t, func(lock *lockHandle) error { return lock.refreshStaleLock(context.TODO()) }) } diff --git a/internal/repository/lock_file_unix.go b/internal/repository/lock_file_unix.go index 5a9d0f33d..38b6c5016 100644 --- a/internal/repository/lock_file_unix.go +++ b/internal/repository/lock_file_unix.go @@ -12,7 +12,7 @@ import ( // checkProcess will check if the process retaining the lock // exists and responds to SIGHUP signal. // Returns true if the process exists and responds. -func (l *Lock) processExists() bool { +func (l *lockHandle) processExists() bool { proc, err := os.FindProcess(l.PID) if err != nil { debug.Log("error searching for process %d: %v\n", l.PID, err) diff --git a/internal/repository/lock_file_windows.go b/internal/repository/lock_file_windows.go index 298d59e37..b3aabc1a5 100644 --- a/internal/repository/lock_file_windows.go +++ b/internal/repository/lock_file_windows.go @@ -8,7 +8,7 @@ import ( // checkProcess will check if the process retaining the lock exists. // Returns true if the process exists. -func (l *Lock) processExists() bool { +func (l *lockHandle) processExists() bool { proc, err := os.FindProcess(l.PID) if err != nil { debug.Log("error searching for process %d: %v\n", l.PID, err) From 4838b0ae3e69a4d3e11e6610401dc50f631385cb Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Mon, 8 Jun 2026 21:17:00 +0200 Subject: [PATCH 07/15] repository: cleanup naming of lockCtx --- internal/repository/lock.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/repository/lock.go b/internal/repository/lock.go index 6d9fd6302..a4f52a019 100644 --- a/internal/repository/lock.go +++ b/internal/repository/lock.go @@ -91,18 +91,18 @@ retryLoop: debug.Log("create lock %p (exclusive %v)", lock, exclusive) ctx, cancel := context.WithCancel(ctx) - lockInfo := &lockContext{ + lockCtx := &lockContext{ lock: lock, cancel: cancel, } - lockInfo.refreshWG.Add(2) + lockCtx.refreshWG.Add(2) refreshChan := make(chan struct{}) forceRefreshChan := make(chan refreshLockRequest) - go l.refreshLocks(ctx, repo.be, lockInfo, refreshChan, forceRefreshChan, logger) - go l.monitorLockRefresh(ctx, lockInfo, refreshChan, forceRefreshChan, logger) + go l.refreshLocks(ctx, repo.be, lockCtx, refreshChan, forceRefreshChan, logger) + go l.monitorLockRefresh(ctx, lockCtx, refreshChan, forceRefreshChan, logger) - return &unlocker{lockInfo}, ctx, nil + return &unlocker{lockCtx}, ctx, nil } func minDuration(a, b time.Duration) time.Duration { From b7f3a1367af4af5b6914581a02a151e8e2594fb4 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Mon, 8 Jun 2026 21:17:36 +0200 Subject: [PATCH 08/15] repository: add SIGHUP handler only on Unix systems --- internal/repository/lock_file.go | 17 ----------------- internal/repository/lock_file_unix.go | 17 +++++++++++++++++ 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/internal/repository/lock_file.go b/internal/repository/lock_file.go index 7cf5c549e..ac83cd9e0 100644 --- a/internal/repository/lock_file.go +++ b/internal/repository/lock_file.go @@ -4,10 +4,8 @@ import ( "context" "fmt" "os" - "os/signal" "os/user" "sync" - "syscall" "testing" "time" @@ -409,21 +407,6 @@ func (l *lockHandle) String() string { return text } -// listen for incoming SIGHUP and ignore -var ignoreSIGHUP sync.Once - -func init() { - ignoreSIGHUP.Do(func() { - go func() { - c := make(chan os.Signal, 1) - signal.Notify(c, syscall.SIGHUP) - for s := range c { - debug.Log("Signal received: %v\n", s) - } - }() - }) -} - // LoadLock loads and unserializes a lock from a repository. func LoadLock(ctx context.Context, repo restic.LoaderUnpacked, id restic.ID) (Lock, error) { var lock Lock diff --git a/internal/repository/lock_file_unix.go b/internal/repository/lock_file_unix.go index 38b6c5016..0133c2fa9 100644 --- a/internal/repository/lock_file_unix.go +++ b/internal/repository/lock_file_unix.go @@ -4,11 +4,28 @@ package repository import ( "os" + "os/signal" + "sync" "syscall" "github.com/restic/restic/internal/debug" ) +// listen for incoming SIGHUP and ignore +var ignoreSIGHUP sync.Once + +func init() { + ignoreSIGHUP.Do(func() { + go func() { + c := make(chan os.Signal, 1) + signal.Notify(c, syscall.SIGHUP) + for s := range c { + debug.Log("Signal received: %v\n", s) + } + }() + }) +} + // checkProcess will check if the process retaining the lock // exists and responds to SIGHUP signal. // Returns true if the process exists and responds. From 5989671f87f0014a2356717dc7b49d3b1ef3c06f Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Mon, 8 Jun 2026 21:22:12 +0200 Subject: [PATCH 09/15] repository: deduplicate error handling in checkForOtherLocks --- internal/repository/lock_file.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/internal/repository/lock_file.go b/internal/repository/lock_file.go index ac83cd9e0..45685a165 100644 --- a/internal/repository/lock_file.go +++ b/internal/repository/lock_file.go @@ -185,11 +185,7 @@ func (l *lockHandle) checkForOtherLocks(ctx context.Context) error { return err } - if l.Exclusive { - return &alreadyLockedError{otherLock: lock} - } - - if !l.Exclusive && lock.Exclusive { + if l.Exclusive || lock.Exclusive { return &alreadyLockedError{otherLock: lock} } From a5f325662628479eaba9d178f98fc52b064778ba Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Mon, 8 Jun 2026 21:40:23 +0200 Subject: [PATCH 10/15] repository: partially deduplicate refresh and refreshStaleLock --- internal/repository/lock_file.go | 39 +++++++++++++++----------------- 1 file changed, 18 insertions(+), 21 deletions(-) diff --git a/internal/repository/lock_file.go b/internal/repository/lock_file.go index 45685a165..9e467f3db 100644 --- a/internal/repository/lock_file.go +++ b/internal/repository/lock_file.go @@ -304,25 +304,32 @@ func delayedCancelContext(parentCtx context.Context, delay time.Duration) (conte // timestamp. Afterwards the old lock is removed. func (l *lockHandle) refresh(ctx context.Context) error { debug.Log("refreshing lock %v", l.lockID) - l.mu.Lock() - l.Time = time.Now() - l.mu.Unlock() - id, err := l.createLock(ctx) + id, err := l.createReplacementLock(ctx) if err != nil { return err } + ctx, cancel := delayedCancelContext(ctx, unlockCancelDelay) + defer cancel() + return l.adoptReplacementLock(ctx, id) +} + +func (l *lockHandle) createReplacementLock(ctx context.Context) (restic.ID, error) { + l.mu.Lock() + l.Time = time.Now() + l.mu.Unlock() + return l.createLock(ctx) +} + +func (l *lockHandle) adoptReplacementLock(ctx context.Context, id restic.ID) error { l.mu.Lock() defer l.mu.Unlock() debug.Log("new lock ID %v", id) - oldLockID := l.lockID + oldID := *l.lockID l.lockID = &id - ctx, cancel := delayedCancelContext(ctx, unlockCancelDelay) - defer cancel() - - return l.repo.RemoveUnpacked(ctx, restic.LockFile, *oldLockID) + return l.repo.RemoveUnpacked(ctx, restic.LockFile, oldID) } // refreshStaleLock is an extended variant of refresh that can also refresh stale lock files. @@ -338,10 +345,7 @@ func (l *lockHandle) refreshStaleLock(ctx context.Context) error { return errRemovedLock } - l.mu.Lock() - l.Time = time.Now() - l.mu.Unlock() - id, err := l.createLock(ctx) + id, err := l.createReplacementLock(ctx) if err != nil { return err } @@ -365,14 +369,7 @@ func (l *lockHandle) refreshStaleLock(ctx context.Context) error { return errRemovedLock } - l.mu.Lock() - defer l.mu.Unlock() - - debug.Log("new lock ID %v", id) - oldLockID := l.lockID - l.lockID = &id - - return l.repo.RemoveUnpacked(ctx, restic.LockFile, *oldLockID) + return l.adoptReplacementLock(ctx, id) } func (l *lockHandle) checkExistence(ctx context.Context) (bool, error) { From b86568b79008bb88c1be5df1316ea08fff22bf7e Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sun, 7 Jun 2026 21:48:09 +0200 Subject: [PATCH 11/15] repository: merge Unlocker and lockContext --- internal/repository/lock.go | 52 +++++++++++++++----------------- internal/repository/lock_test.go | 2 +- 2 files changed, 26 insertions(+), 28 deletions(-) diff --git a/internal/repository/lock.go b/internal/repository/lock.go index a4f52a019..d120ee943 100644 --- a/internal/repository/lock.go +++ b/internal/repository/lock.go @@ -13,12 +13,23 @@ import ( "github.com/restic/restic/internal/restic" ) -type lockContext struct { +type Unlocker interface { + Unlock() +} + +type unlocker struct { lock *lockHandle cancel context.CancelFunc refreshWG sync.WaitGroup } +func (l *unlocker) Unlock() { + l.cancel() + l.refreshWG.Wait() +} + +var _ Unlocker = &unlocker{} + type locker struct { retrySleepStart time.Duration retrySleepMax time.Duration @@ -91,18 +102,18 @@ retryLoop: debug.Log("create lock %p (exclusive %v)", lock, exclusive) ctx, cancel := context.WithCancel(ctx) - lockCtx := &lockContext{ + unlocker := &unlocker{ lock: lock, cancel: cancel, } - lockCtx.refreshWG.Add(2) + unlocker.refreshWG.Add(2) refreshChan := make(chan struct{}) forceRefreshChan := make(chan refreshLockRequest) - go l.refreshLocks(ctx, repo.be, lockCtx, refreshChan, forceRefreshChan, logger) - go l.monitorLockRefresh(ctx, lockCtx, refreshChan, forceRefreshChan, logger) + go l.refreshLocks(ctx, repo.be, unlocker, refreshChan, forceRefreshChan, logger) + go l.monitorLockRefresh(ctx, unlocker, refreshChan, forceRefreshChan, logger) - return &unlocker{lockCtx}, ctx, nil + return unlocker, ctx, nil } func minDuration(a, b time.Duration) time.Duration { @@ -116,16 +127,16 @@ type refreshLockRequest struct { result chan bool } -func (l *locker) refreshLocks(ctx context.Context, backend backend.Backend, lockInfo *lockContext, refreshed chan<- struct{}, forceRefresh <-chan refreshLockRequest, logger func(format string, args ...interface{})) { +func (l *locker) refreshLocks(ctx context.Context, backend backend.Backend, unlocker *unlocker, refreshed chan<- struct{}, forceRefresh <-chan refreshLockRequest, logger func(format string, args ...interface{})) { debug.Log("start") - lock := lockInfo.lock + lock := unlocker.lock ticker := time.NewTicker(l.refreshInterval) lastRefresh := lock.Time defer func() { ticker.Stop() // ensure that the context was cancelled before removing the lock - lockInfo.cancel() + unlocker.cancel() // remove the lock from the repo debug.Log("unlocking repository with lock %v", lock) @@ -134,7 +145,7 @@ func (l *locker) refreshLocks(ctx context.Context, backend backend.Backend, lock logger("error while unlocking: %v", err) } - lockInfo.refreshWG.Done() + unlocker.refreshWG.Done() }() for { @@ -146,7 +157,7 @@ func (l *locker) refreshLocks(ctx context.Context, backend backend.Backend, lock case req := <-forceRefresh: debug.Log("trying to refresh stale lock") // keep on going if our current lock still exists - success := tryRefreshStaleLock(ctx, backend, lock, lockInfo.cancel, logger) + success := tryRefreshStaleLock(ctx, backend, lock, unlocker.cancel, logger) // inform refresh goroutine about forced refresh select { case <-ctx.Done(): @@ -180,7 +191,7 @@ func (l *locker) refreshLocks(ctx context.Context, backend backend.Backend, lock } } -func (l *locker) monitorLockRefresh(ctx context.Context, lockInfo *lockContext, refreshed <-chan struct{}, forceRefresh chan<- refreshLockRequest, logger func(format string, args ...interface{})) { +func (l *locker) monitorLockRefresh(ctx context.Context, unlocker *unlocker, refreshed <-chan struct{}, forceRefresh chan<- refreshLockRequest, logger func(format string, args ...interface{})) { // time.Now() might use a monotonic timer which is paused during standby // convert to unix time to ensure we compare real time values lastRefresh := time.Now().UnixNano() @@ -195,8 +206,8 @@ func (l *locker) monitorLockRefresh(ctx context.Context, lockInfo *lockContext, ticker := time.NewTicker(pollDuration) defer func() { ticker.Stop() - lockInfo.cancel() - lockInfo.refreshWG.Done() + unlocker.cancel() + unlocker.refreshWG.Done() }() var refreshStaleLockResult chan bool @@ -261,19 +272,6 @@ func tryRefreshStaleLock(ctx context.Context, be backend.Backend, lock *lockHand return true } -type Unlocker interface { - Unlock() -} - -type unlocker struct { - info *lockContext -} - -func (l *unlocker) Unlock() { - l.info.cancel() - l.info.refreshWG.Wait() -} - // RemoveStaleLocks deletes all locks detected as stale from the repository. func RemoveStaleLocks(ctx context.Context, repo *Repository) (uint, error) { var processed uint diff --git a/internal/repository/lock_test.go b/internal/repository/lock_test.go index 2fb9025a1..bad25d52e 100644 --- a/internal/repository/lock_test.go +++ b/internal/repository/lock_test.go @@ -38,7 +38,7 @@ func checkedLockRepo(ctx context.Context, t *testing.T, repo *Repository, locker lock, wrappedCtx, err := lockerInst.Lock(ctx, repo, false, retryLock, func(msg string) {}, func(format string, args ...interface{}) {}) rtest.OK(t, err) rtest.OK(t, wrappedCtx.Err()) - if lock.info.lock.stale() { + if lock.lock.stale() { t.Fatal("lock returned stale lock") } return lock, wrappedCtx From 151903d4d4f84231fca4609ca06027b7d8c0fa15 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sun, 7 Jun 2026 21:54:22 +0200 Subject: [PATCH 12/15] repository: fix goroutine leak due to delayedCancelContext in tests --- internal/repository/lock_file.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/repository/lock_file.go b/internal/repository/lock_file.go index 9e467f3db..5f2164652 100644 --- a/internal/repository/lock_file.go +++ b/internal/repository/lock_file.go @@ -293,7 +293,7 @@ func delayedCancelContext(parentCtx context.Context, delay time.Duration) (conte return } - time.Sleep(delay) + _ = cancelableDelay(ctx, delay) cancel() }() From 1a6d43286a3438c05bc94e24f57a177142018a4c Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Mon, 8 Jun 2026 21:47:56 +0200 Subject: [PATCH 13/15] repository: comment cleanups --- internal/repository/lock.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/repository/lock.go b/internal/repository/lock.go index d120ee943..6f43b5e6a 100644 --- a/internal/repository/lock.go +++ b/internal/repository/lock.go @@ -48,12 +48,12 @@ var lockerInst = &locker{ refreshabilityTimeout: staleLockTimeout - defaultRefreshInterval*3/2, } +// LockRepo acquires a repository lock. The returned context is cancelled when +// Unlock is called; cancelling the original context stops lock refresh. func LockRepo(ctx context.Context, repo *Repository, exclusive bool, retryLock time.Duration, printRetry func(msg string), logger func(format string, args ...interface{})) (Unlocker, context.Context, error) { return lockerInst.Lock(ctx, repo, exclusive, retryLock, printRetry, logger) } -// Lock wraps the ctx such that it is cancelled when the repository is unlocked -// cancelling the original context also stops the lock refresh func (l *locker) Lock(ctx context.Context, r *Repository, exclusive bool, retryLock time.Duration, printRetry func(msg string), logger func(format string, args ...interface{})) (*unlocker, context.Context, error) { var lock *lockHandle var err error @@ -158,7 +158,7 @@ func (l *locker) refreshLocks(ctx context.Context, backend backend.Backend, unlo debug.Log("trying to refresh stale lock") // keep on going if our current lock still exists success := tryRefreshStaleLock(ctx, backend, lock, unlocker.cancel, logger) - // inform refresh goroutine about forced refresh + // inform monitor goroutine about forced refresh select { case <-ctx.Done(): case req.result <- success: From 6fe10a72eb1a4387355a226473c89ea38918f755 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Mon, 8 Jun 2026 21:48:25 +0200 Subject: [PATCH 14/15] repository: drop mutex from lockHandle --- internal/repository/lock_file.go | 14 -------------- internal/repository/lock_test.go | 3 --- 2 files changed, 17 deletions(-) diff --git a/internal/repository/lock_file.go b/internal/repository/lock_file.go index 5f2164652..8002aaf7e 100644 --- a/internal/repository/lock_file.go +++ b/internal/repository/lock_file.go @@ -36,7 +36,6 @@ type Lock struct { // lockHandle is a reference to a lock file in the repository. type lockHandle struct { - mu sync.Mutex Lock repo restic.Unpacked[restic.FileType] lockID *restic.ID @@ -251,8 +250,6 @@ var staleLockTimeout = 30 * time.Minute // older than 30 minutes or if it was created on the current machine and the // process isn't alive any more. func (l *lockHandle) stale() bool { - l.mu.Lock() - defer l.mu.Unlock() debug.Log("testing if lock %v for process %d is stale", l.lockID, l.PID) if time.Since(l.Time) > staleLockTimeout { debug.Log("lock is stale, timestamp is too old: %v\n", l.Time) @@ -315,16 +312,11 @@ func (l *lockHandle) refresh(ctx context.Context) error { } func (l *lockHandle) createReplacementLock(ctx context.Context) (restic.ID, error) { - l.mu.Lock() l.Time = time.Now() - l.mu.Unlock() return l.createLock(ctx) } func (l *lockHandle) adoptReplacementLock(ctx context.Context, id restic.ID) error { - l.mu.Lock() - defer l.mu.Unlock() - debug.Log("new lock ID %v", id) oldID := *l.lockID l.lockID = &id @@ -373,9 +365,6 @@ func (l *lockHandle) refreshStaleLock(ctx context.Context) error { } func (l *lockHandle) checkExistence(ctx context.Context) (bool, error) { - l.mu.Lock() - defer l.mu.Unlock() - exists := false err := l.repo.List(ctx, restic.LockFile, func(id restic.ID, _ int64) error { @@ -389,9 +378,6 @@ func (l *lockHandle) checkExistence(ctx context.Context) (bool, error) { } func (l *lockHandle) String() string { - l.mu.Lock() - defer l.mu.Unlock() - text := fmt.Sprintf("PID %d on %s by %s (UID %d, GID %d)\nlock was created at %s (%s ago)\nstorage ID %v", l.PID, l.Hostname, l.Username, l.UID, l.GID, l.Time.Format("2006-01-02 15:04:05"), time.Since(l.Time), diff --git a/internal/repository/lock_test.go b/internal/repository/lock_test.go index bad25d52e..6f29b7a18 100644 --- a/internal/repository/lock_test.go +++ b/internal/repository/lock_test.go @@ -38,9 +38,6 @@ func checkedLockRepo(ctx context.Context, t *testing.T, repo *Repository, locker lock, wrappedCtx, err := lockerInst.Lock(ctx, repo, false, retryLock, func(msg string) {}, func(format string, args ...interface{}) {}) rtest.OK(t, err) rtest.OK(t, wrappedCtx.Err()) - if lock.lock.stale() { - t.Fatal("lock returned stale lock") - } return lock, wrappedCtx } From 406dec29a742b49438e8d79a8a6a5d87a0518234 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Mon, 8 Jun 2026 22:14:12 +0200 Subject: [PATCH 15/15] repository: remove redundant mutex in checkForOtherLocks forAllLocks already serializes the callback calls. Still has to create a copy of checkedIDs to prevent a data race between forAllLocks and the callback. --- internal/repository/lock_file.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/internal/repository/lock_file.go b/internal/repository/lock_file.go index 8002aaf7e..cdc295f0d 100644 --- a/internal/repository/lock_file.go +++ b/internal/repository/lock_file.go @@ -173,8 +173,7 @@ func (l *lockHandle) checkForOtherLocks(ctx context.Context) error { delay *= 2 } - // Store updates in new IDSet to prevent data races - var m sync.Mutex + // Store updates in new IDSet to prevent data races with Has() check in forAllLocks newCheckedIDs := checkedIDs.Clone() err = forAllLocks(ctx, l.repo, checkedIDs, func(id restic.ID, lock *lockHandle, err error) error { if err != nil { @@ -189,9 +188,7 @@ func (l *lockHandle) checkForOtherLocks(ctx context.Context) error { } // valid locks will remain valid - m.Lock() newCheckedIDs.Insert(id) - m.Unlock() return nil }) checkedIDs = newCheckedIDs