diff --git a/internal/cron/lock.go b/internal/cron/lock.go index 740fb9ada..b3a20b2e8 100644 --- a/internal/cron/lock.go +++ b/internal/cron/lock.go @@ -5,31 +5,23 @@ import ( "fmt" "os" "path/filepath" - "sync/atomic" "time" "github.com/Gitlawb/zero/internal/lockutil" ) // Cross-process lock tuning. The lock is held only for a single metadata -// read-modify-write (milliseconds), never across a job's exec, so the timeout is -// generous and the stale threshold sits far above any real hold. +// read-modify-write (milliseconds), never across a job's exec, so the timeout +// is generous relative to any real hold. const ( cronLockTimeout = 10 * time.Second - cronLockStaleAfter = 60 * time.Second cronLockRetryDelay = 20 * time.Millisecond ) -var cronLockSeq atomic.Uint64 - -// lockJob takes a cross-process exclusive lock for one job by O_EXCL-creating a -// sibling ".lock" file next to the job directory (so Remove's RemoveAll of -// the job dir never deletes a live lock). It serializes the read-modify-write of -// a job's metadata across concurrent schedulers and commands. The lock file is -// removed on release; a stale lock from a crashed holder (older than -// cronLockStaleAfter) is reclaimed. Wall-clock time.Now is used deliberately -// (not the injectable Store.now) so lock timing never depends on a frozen test -// clock and the stale check compares against real file mtimes. +// lockJob takes a cross-process advisory lock for one job on a stable sibling +// ".lock" file next to the job directory. It serializes the read-modify-write +// of a job's metadata across concurrent schedulers and commands. The kernel +// releases the lock when a holder exits; the file is never renamed or removed. func (s *Store) lockJob(id string) (func(), error) { if !validID(id) { return nil, fmt.Errorf("invalid cron job id %q", id) @@ -38,64 +30,15 @@ func (s *Store) lockJob(id string) (func(), error) { if err := os.MkdirAll(filepath.Dir(lockPath), 0o700); err != nil { return nil, err } - token := fmt.Sprintf("%d-%d-%d", os.Getpid(), time.Now().UnixNano(), cronLockSeq.Add(1)) deadline := time.Now().Add(cronLockTimeout) for { - f, err := os.OpenFile(lockPath, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o600) + lock, err := lockutil.TryAcquireFileLock(lockPath) if err == nil { - // A partial write would leave a lock file without our token, so the - // releaser could never delete it — stranding the lock. Fail closed. - if _, werr := f.WriteString(token); werr != nil { - _ = f.Close() - _ = lockutil.RemoveLockFile(lockPath) - return nil, fmt.Errorf("cron: write job lock: %w", werr) - } - if cerr := f.Close(); cerr != nil { - _ = lockutil.RemoveLockFile(lockPath) - return nil, fmt.Errorf("cron: close job lock: %w", cerr) - } - var released bool - return func() { - if released { - return - } - released = true - if data, rerr := os.ReadFile(lockPath); rerr == nil && string(data) == token { - _ = lockutil.RemoveLockFile(lockPath) - } - }, nil + return func() { _ = lock.Release() }, nil } - // On Windows a concurrent holder's os.Remove leaves the lock file in a - // "delete pending" state, so an O_EXCL create races it with - // ERROR_ACCESS_DENIED (os.ErrPermission) rather than ErrExist. Treat that - // as contention and retry, exactly like ErrExist — otherwise the lock - // spuriously fails under concurrency on Windows. - if !errors.Is(err, os.ErrExist) && !errors.Is(err, os.ErrPermission) { + if !errors.Is(err, lockutil.ErrLockHeld) { return nil, fmt.Errorf("cron: acquire job lock: %w", err) } - // Reclaim a stale lock left by a crashed holder — atomically (H3). A blind - // Remove lets two racers both "reclaim" and recreate, so both hold the lock; - // reclaimStaleLock renames the file aside (only one rename of a given file - // wins) and restores it if it turns out fresh, so a live lock is never deleted - // out from under its holder. - if info, statErr := os.Stat(lockPath); statErr == nil && time.Since(info.ModTime()) > cronLockStaleAfter { - cleared, rerr := lockutil.ReclaimStaleLock(lockPath, token, func(reclaimedPath string) bool { - info, err := os.Stat(reclaimedPath) - return err == nil && time.Since(info.ModTime()) <= cronLockStaleAfter - }) - if rerr != nil { - // Reclaim hit a hard failure: the rename aside failed outright, or a - // live holder's lock could not be put back (the lock path may be - // missing, so re-acquiring would break mutual exclusion). Fail closed - // instead of spinning to the deadline. - return nil, fmt.Errorf("cron: reclaim stale job lock: %w", rerr) - } - if cleared { - continue // cleared a genuinely stale lock; retry the O_EXCL create now - } - // Lost the reclaim race (or it was actually fresh) — fall through to the - // bounded wait instead of hot-spinning on a reclaim that never wins (L13). - } if !time.Now().Before(deadline) { return nil, fmt.Errorf("cron: timed out acquiring job lock for %q", id) } diff --git a/internal/cron/mutate_test.go b/internal/cron/mutate_test.go index 42c27e8dd..45f9a243c 100644 --- a/internal/cron/mutate_test.go +++ b/internal/cron/mutate_test.go @@ -64,7 +64,7 @@ func TestMutateRemovedJobReturnsNotFound(t *testing.T) { if !errors.Is(err, ErrJobNotFound) { t.Fatalf("Mutate on removed job = %v, want ErrJobNotFound", err) } - // And the lock file left no litter / did not resurrect the job dir. + // The stable advisory-lock file does not resurrect the removed job dir. if _, gerr := store.Get(job.ID); !errors.Is(gerr, ErrJobNotFound) { t.Fatalf("job should remain removed, Get = %v", gerr) } diff --git a/internal/daemon/lock.go b/internal/daemon/lock.go index 239ec3d04..71e3ce3e8 100644 --- a/internal/daemon/lock.go +++ b/internal/daemon/lock.go @@ -3,107 +3,60 @@ package daemon import ( "errors" "fmt" - "io/fs" "os" "strconv" "strings" - "sync/atomic" "github.com/Gitlawb/zero/internal/lockutil" ) -// Single-instance lock. Mirrors reference-daemon-code-agent-js/supervisor.js's -// lock file: a PID file created with O_EXCL. A second start fails; a STALE lock -// left by a dead daemon (the recorded PID is no longer alive) is reclaimed so the -// daemon recovers from an unclean shutdown without manual cleanup. +// Single-instance lock. The stable PID file carries a kernel advisory lock for +// the daemon's full lifetime. Process exit releases the lock automatically, so +// stale metadata needs no rename/remove recovery protocol. // ErrAlreadyRunning is returned when a live daemon already holds the lock. var ErrAlreadyRunning = errors.New("daemon: another instance is already running") // fileLock is an acquired single-instance lock. type fileLock struct { - path string + lock *lockutil.FileLock } // processAlive reports whether pid is a live process. Implemented per-platform // (lock_posix.go / lock_windows.go). It is a package var so tests can stub it. var processAlive = osProcessAlive -// acquireLock takes the single-instance lock at path, reclaiming a stale lock -// whose recorded PID is dead. isAlive overrides the liveness check (tests pass a -// stub); nil uses the real processAlive. +// acquireLock takes the single-instance advisory lock at path. isAlive is used +// only to enrich a contention error with the current holder's PID; the kernel +// lock, not PID metadata, is authoritative. func acquireLock(path string, isAlive func(pid int) bool) (*fileLock, error) { if isAlive == nil { isAlive = processAlive } - // At most two passes: create, or detect-stale-then-retry once. - for attempt := 0; attempt < 2; attempt++ { - f, err := os.OpenFile(path, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o600) - if err == nil { - // A failed PID write would leave a malformed lock file that another - // process reads as stale (unparsable PID) and wrongly reclaims, breaking - // the single-instance guarantee — so on write failure, remove it and fail. - if _, werr := fmt.Fprintf(f, "%d\n", os.Getpid()); werr != nil { - _ = f.Close() - _ = lockutil.RemoveLockFile(path) - return nil, werr - } - if cerr := f.Close(); cerr != nil { - _ = lockutil.RemoveLockFile(path) - return nil, cerr - } - return &fileLock{path: path}, nil - } - if !errors.Is(err, fs.ErrExist) && !errors.Is(err, os.ErrPermission) { + lock, err := lockutil.TryAcquireFileLock(path) + if err != nil { + if !errors.Is(err, lockutil.ErrLockHeld) { return nil, err } pid, perr := readPidFile(path) if perr == nil && pid > 0 && isAlive(pid) { return nil, fmt.Errorf("%w (pid %d)", ErrAlreadyRunning, pid) } - // Stale lock (dead PID or unreadable) — reclaim it atomically, then retry the - // O_EXCL create. A blind Remove here races: two daemons starting at once could - // both read the stale PID, both Remove, and then one Removes the OTHER's - // freshly-created lock — leaving both "holding" the single-instance lock. - // reclaimStaleLock renames the file aside so only one racer wins the rename, - // and restores it if a live holder reacquired in the gap (D6). - if _, rerr := reclaimStaleLock(path, isAlive); rerr != nil { - // Reclaim hit a hard failure: the rename aside failed outright, or a - // live holder's lock could not be put back (the lock path may be - // missing, so re-acquiring would break the single-instance guarantee). - // Fail closed instead of spinning to the deadline. - return nil, fmt.Errorf("daemon: reclaim stale lock: %w", rerr) - } + return nil, ErrAlreadyRunning } - return nil, ErrAlreadyRunning -} - -// daemonLockSeq makes each reclaim attempt's sidelined filename unique per process. -var daemonLockSeq atomic.Uint64 - -// reclaimStaleLock atomically reclaims a single-instance lock whose recorded -// PID is dead, via lockutil.ReclaimStaleLock: the lock file is renamed aside -// (only one racer can win the rename) and stolen only if the moved file's PID -// is still dead; a holder that reacquired the lock in the gap between the -// stale check and the rename carries a LIVE pid and is restored rather than -// stolen. Returns true only when a genuinely stale lock was removed. A -// non-nil error means the caller must fail closed instead of re-acquiring -// (see lockutil.ReclaimStaleLock). -func reclaimStaleLock(path string, isAlive func(pid int) bool) (bool, error) { - suffix := fmt.Sprintf("%d-%d", os.Getpid(), daemonLockSeq.Add(1)) - return lockutil.ReclaimStaleLock(path, suffix, func(reclaimedPath string) bool { - pid, err := readPidFile(reclaimedPath) - return err == nil && pid > 0 && isAlive(pid) - }) + if err := lock.WriteMetadata([]byte(fmt.Sprintf("%d\n", os.Getpid()))); err != nil { + return nil, errors.Join(err, lock.Release()) + } + return &fileLock{lock: lock}, nil } -// release removes the lock file. Safe to call once. An already-missing lock -// file is not an error (lockutil.RemoveLockFile swallows it on every platform). +// release drops the kernel lock and closes its handle. The stable PID file is +// deliberately retained so contenders always address the same inode. func (l *fileLock) release() error { - if l == nil || l.path == "" { + if l == nil || l.lock == nil { return nil } - return lockutil.RemoveLockFile(l.path) + return l.lock.Release() } // readPidFile reads and parses the PID recorded in a lock file. @@ -112,5 +65,20 @@ func readPidFile(path string) (int, error) { if err != nil { return 0, err } - return strconv.Atoi(strings.TrimSpace(string(data))) + metadata := strings.TrimSpace(string(data)) + if pid, err := strconv.Atoi(metadata); err == nil { + return pid, nil + } + pidText, sequenceText, ok := strings.Cut(metadata, "-") + if !ok || pidText == "" || sequenceText == "" || strings.Contains(sequenceText, "-") { + return 0, fmt.Errorf("daemon: parse lock PID metadata %q", metadata) + } + if _, err := strconv.ParseUint(sequenceText, 10, 64); err != nil { + return 0, fmt.Errorf("daemon: parse lock holder sequence: %w", err) + } + pid, err := strconv.Atoi(pidText) + if err != nil { + return 0, fmt.Errorf("daemon: parse lock PID: %w", err) + } + return pid, nil } diff --git a/internal/daemon/lock_test.go b/internal/daemon/lock_test.go index e5eb3dd30..65843d875 100644 --- a/internal/daemon/lock_test.go +++ b/internal/daemon/lock_test.go @@ -32,27 +32,48 @@ func TestLockSingleInstance(t *testing.T) { _ = l2.release() } -func TestLockStaleRecovery(t *testing.T) { +// A PID liveness probe is diagnostic only. Even if it reports the recorded PID +// dead, a contended kernel lock proves that the holder is active and must win. +// The old rename-aside protocol trusted this stale observation and admitted a +// second daemon while the first one was still inside its critical section. +func TestLockDoesNotOverrideKernelHolderWithStalePIDProbe(t *testing.T) { path := filepath.Join(t.TempDir(), "daemon.lock") - // Simulate a stale lock from a crashed daemon: a PID file whose process is - // dead. + first, err := acquireLock(path, func(int) bool { return true }) + if err != nil { + t.Fatalf("first acquireLock: %v", err) + } + defer first.release() + + second, err := acquireLock(path, func(int) bool { return false }) + if second != nil { + _ = second.release() + } + if !errors.Is(err, ErrAlreadyRunning) { + t.Fatalf("contended acquire with stale PID probe = %v, want ErrAlreadyRunning", err) + } +} + +func TestLockIgnoresStaleMetadataWhenKernelLockIsFree(t *testing.T) { + path := filepath.Join(t.TempDir(), "daemon.lock") + // A crashed daemon may leave PID metadata behind, but its kernel lock is + // released automatically, so the next daemon can acquire without moving it. if err := os.WriteFile(path, []byte("4242\n"), 0o600); err != nil { t.Fatalf("write stale lock: %v", err) } dead := func(int) bool { return false } l, err := acquireLock(path, dead) if err != nil { - t.Fatalf("stale-lock recovery failed: %v", err) + t.Fatalf("acquire over stale metadata: %v", err) } // The lock now records OUR pid, not the stale one. data, _ := os.ReadFile(path) if strings.TrimSpace(string(data)) != strconv.Itoa(os.Getpid()) { - t.Fatalf("reclaimed lock pid = %q, want %d", strings.TrimSpace(string(data)), os.Getpid()) + t.Fatalf("lock pid = %q, want %d", strings.TrimSpace(string(data)), os.Getpid()) } _ = l.release() } -func TestLockReleaseRemovesFile(t *testing.T) { +func TestLockReleaseKeepsStableFile(t *testing.T) { path := filepath.Join(t.TempDir(), "daemon.lock") l, err := acquireLock(path, func(int) bool { return true }) if err != nil { @@ -61,46 +82,37 @@ func TestLockReleaseRemovesFile(t *testing.T) { if err := l.release(); err != nil { t.Fatalf("release: %v", err) } - if _, err := os.Stat(path); !errors.Is(err, os.ErrNotExist) { - t.Fatalf("lock file still present after release: %v", err) + if _, err := os.Stat(path); err != nil { + t.Fatalf("release removed stable lock file: %v", err) } } -func TestReclaimStaleLockRemovesDeadHolder(t *testing.T) { +func TestReadPidFileMetadataFormats(t *testing.T) { path := filepath.Join(t.TempDir(), "daemon.lock") - if err := os.WriteFile(path, []byte("4242\n"), 0o600); err != nil { - t.Fatalf("write stale lock: %v", err) - } - if ok, err := reclaimStaleLock(path, func(int) bool { return false }); err != nil || !ok { - t.Fatalf("reclaimStaleLock must report a genuinely stale (dead-PID) lock reclaimed (ok=%v err=%v)", ok, err) - } - if _, err := os.Stat(path); !errors.Is(err, os.ErrNotExist) { - t.Fatalf("a reclaimed stale lock must be removed, stat = %v", err) - } -} - -func TestReclaimStaleLockRestoresLiveHolder(t *testing.T) { - // If a holder reacquires the lock in the gap between the stale check and the - // rename, the moved file carries a LIVE pid; reclaim must restore it untouched - // rather than steal it — otherwise two daemons both "hold" the lock (D6). - path := filepath.Join(t.TempDir(), "daemon.lock") - if err := os.WriteFile(path, []byte("4242\n"), 0o600); err != nil { - t.Fatalf("write lock: %v", err) - } - if ok, err := reclaimStaleLock(path, func(int) bool { return true }); err != nil || ok { - t.Fatalf("reclaimStaleLock must NOT report a live-PID lock reclaimed (ok=%v err=%v)", ok, err) - } - data, err := os.ReadFile(path) - if err != nil { - t.Fatalf("live lock must be restored in place, read = %v", err) - } - if strings.TrimSpace(string(data)) != "4242" { - t.Fatalf("restored lock content = %q, want %q (unchanged)", strings.TrimSpace(string(data)), "4242") - } - // No sidelined ".stale" leftovers. - matches, _ := filepath.Glob(path + ".stale.*") - if len(matches) != 0 { - t.Fatalf("reclaim left sidelined files: %v", matches) + for _, test := range []struct { + name string + metadata string + wantPID int + wantErr bool + }{ + {name: "legacy PID", metadata: "4242\n", wantPID: 4242}, + {name: "PID and holder sequence", metadata: "4242-7\n", wantPID: 4242}, + {name: "missing sequence", metadata: "4242-", wantErr: true}, + {name: "non-numeric sequence", metadata: "4242-next", wantErr: true}, + {name: "extra component", metadata: "4242-7-8", wantErr: true}, + } { + t.Run(test.name, func(t *testing.T) { + if err := os.WriteFile(path, []byte(test.metadata), 0o600); err != nil { + t.Fatal(err) + } + got, err := readPidFile(path) + if (err != nil) != test.wantErr { + t.Fatalf("readPidFile() error = %v, wantErr %v", err, test.wantErr) + } + if !test.wantErr && got != test.wantPID { + t.Fatalf("readPidFile() = %d, want %d", got, test.wantPID) + } + }) } } diff --git a/internal/daemon/server_test.go b/internal/daemon/server_test.go index 9b1071631..0fa6c99f7 100644 --- a/internal/daemon/server_test.go +++ b/internal/daemon/server_test.go @@ -124,12 +124,16 @@ func TestServerEndToEnd(t *testing.T) { case <-time.After(3 * time.Second): t.Fatal("Serve did not return after shutdown") } - // Socket, status, and lock files are removed on exit. - for _, p := range []string{paths.Socket, paths.Status, paths.Lock} { + // Runtime endpoints are removed on exit. The advisory lock file remains as + // a stable inode, but the kernel lock on it has been released. + for _, p := range []string{paths.Socket, paths.Status} { if _, err := os.Stat(p); !errors.Is(err, os.ErrNotExist) { t.Fatalf("file %s not cleaned up after shutdown: %v", p, err) } } + if _, err := os.Stat(paths.Lock); err != nil { + t.Fatalf("stable lock file missing after shutdown: %v", err) + } } func TestServerSecondInstanceFails(t *testing.T) { diff --git a/internal/hooks/lock.go b/internal/hooks/lock.go index f763a397a..d769e1448 100644 --- a/internal/hooks/lock.go +++ b/internal/hooks/lock.go @@ -5,93 +5,39 @@ import ( "fmt" "os" "path/filepath" - "sync/atomic" "time" "github.com/Gitlawb/zero/internal/lockutil" ) // Cross-process lock tuning for the audit log. The lock is held only across a -// single read-then-append (milliseconds), so the timeout is generous and the -// stale threshold sits far above any real hold. +// single read-then-append (milliseconds), so the timeout is generous relative +// to any real hold. const ( auditLockTimeout = 10 * time.Second - auditLockStaleAfter = 60 * time.Second auditLockRetryDelay = 20 * time.Millisecond ) -var auditLockSeq atomic.Uint64 - -// lockAudit takes a cross-process exclusive lock on the audit log by -// O_EXCL-creating a sibling ".lock" file (removed on release). It makes +// lockAudit takes a cross-process advisory lock on the audit log through a +// stable sibling ".lock" file. It makes // the lastSequence()+append in append() atomic across processes; the audit log is // a shared global file, so without it two processes can read the same last -// sequence and write duplicate numbers. A stale lock from a crashed holder (older -// than auditLockStaleAfter) is reclaimed. Wall-clock time is used deliberately so -// lock timing never depends on the store's injectable clock and the stale check -// compares against real file mtimes. This mirrors internal/cron/lock.go and -// internal/oauth/lock.go. Uses internal/lockutil for lock restore and reclaim. +// sequence and write duplicate numbers. Kernel lock lifetime provides crash +// recovery without moving or deleting the lock path. func (store *AuditStore) lockAudit() (func(), error) { lockPath := store.auditPath + ".lock" if err := os.MkdirAll(filepath.Dir(lockPath), 0o700); err != nil { return nil, err } - token := fmt.Sprintf("%d-%d-%d", os.Getpid(), time.Now().UnixNano(), auditLockSeq.Add(1)) deadline := time.Now().Add(auditLockTimeout) for { - f, err := os.OpenFile(lockPath, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o600) + lock, err := lockutil.TryAcquireFileLock(lockPath) if err == nil { - // A partial write would leave a lock file without our token, so the - // releaser could never delete it — stranding the lock. Fail closed. - if _, werr := f.WriteString(token); werr != nil { - _ = f.Close() - _ = lockutil.RemoveLockFile(lockPath) - return nil, fmt.Errorf("hooks: write audit lock: %w", werr) - } - if cerr := f.Close(); cerr != nil { - _ = lockutil.RemoveLockFile(lockPath) - return nil, fmt.Errorf("hooks: close audit lock: %w", cerr) - } - var released bool - return func() { - if released { - return - } - released = true - if data, rerr := os.ReadFile(lockPath); rerr == nil && string(data) == token { - _ = lockutil.RemoveLockFile(lockPath) - } - }, nil + return func() { _ = lock.Release() }, nil } - // On Windows a concurrent holder's os.Remove leaves the lock file in a - // "delete pending" state, so an O_EXCL create races it with - // ERROR_ACCESS_DENIED (os.ErrPermission) rather than ErrExist. Treat both - // as contention and retry. - if !errors.Is(err, os.ErrExist) && !errors.Is(err, os.ErrPermission) { + if !errors.Is(err, lockutil.ErrLockHeld) { return nil, fmt.Errorf("hooks: acquire audit lock: %w", err) } - // Reclaim a stale lock left by a crashed holder — atomically (H3). A blind - // Remove lets two racers both reclaim + recreate and so both hold the lock; - // reclaimStaleLock renames the file aside (only one rename wins) and restores - // it if it turns out fresh, so a live lock is never deleted out from under it. - if info, statErr := os.Stat(lockPath); statErr == nil && time.Since(info.ModTime()) > auditLockStaleAfter { - cleared, rerr := lockutil.ReclaimStaleLock(lockPath, token, func(reclaimedPath string) bool { - info, err := os.Stat(reclaimedPath) - return err == nil && time.Since(info.ModTime()) <= auditLockStaleAfter - }) - if rerr != nil { - // Reclaim hit a hard failure: the rename aside failed outright, or a - // live holder's lock could not be put back (the lock path may be - // missing, so re-acquiring would break mutual exclusion). Fail closed - // instead of spinning to the deadline. - return nil, fmt.Errorf("hooks: reclaim stale audit lock: %w", rerr) - } - if cleared { - continue - } - // Lost the reclaim race (or it was actually fresh) — fall through to the - // bounded wait rather than hot-spinning on a reclaim that never wins. - } if !time.Now().Before(deadline) { return nil, fmt.Errorf("hooks: timed out acquiring audit lock") } diff --git a/internal/lockutil/lockutil.go b/internal/lockutil/lockutil.go index 17c1df2e4..da58d5e3b 100644 --- a/internal/lockutil/lockutil.go +++ b/internal/lockutil/lockutil.go @@ -1,64 +1,127 @@ -// Package lockutil provides the platform-specific file primitives behind the -// O_EXCL lock files in cron, daemon, hooks, oauth, and swarm: a no-overwrite -// restore for locks that were sidelined during a stale-reclaim attempt, and a -// lock file remover with one cross-platform contract (missing files are a -// no-op; Windows retries transient sharing violations). +// Package lockutil provides cross-process advisory file locks used by cron, +// daemon, hooks, oauth, and swarm. Locks are held by the kernel for the lifetime +// of an open file handle, so process exit releases them without a stale-file +// reclaim protocol. The lock file itself is stable: holders never rename or +// remove it while another process may still have its inode locked. package lockutil import ( - "io" + "errors" + "fmt" "os" + "path/filepath" + "strings" + "sync" + "sync/atomic" ) -// restoreByCopy restores reclaimed to path without overwriting an existing -// path, as a fallback for when the platform's primary no-replace primitive -// (hard link on POSIX, MoveFileEx on Windows) fails for a reason other than -// the destination existing. It stages a full copy under a private name next -// to reclaimed and publishes it to path with publish (the same no-replace -// primitive the caller's platform uses for the primary restore), so path -// never appears in a partially-copied state: a crash between staging and -// publish leaves path exactly as it was (missing or, if a competing holder -// created it, untouched), never a truncated file that a PID/content-based -// liveness check could mistake for dead. Leaving path missing here would let -// the next O_EXCL create succeed while the sidelined holder is still live, -// breaking mutual exclusion. publish keeps the no-overwrite guarantee: a new -// holder that appeared in the meantime wins and this returns os.ErrExist. The -// copy resets the lock's mtime to now, which only makes the restored lock -// look fresher; that is safe, since it is being handed back to a live holder. -func restoreByCopy(reclaimed, path string, publish func(from, to string) error) error { - staged := reclaimed + ".copy" - src, err := os.Open(reclaimed) +var ( + // ErrLockHeld reports that another process or goroutine currently holds the + // advisory lock. Callers may wait and retry until their own deadline. + ErrLockHeld = errors.New("lockutil: lock is held") + // ErrLockReleased reports an attempt to update metadata after Release. + ErrLockReleased = errors.New("lockutil: lock is released") +) + +var holderSequence atomic.Uint64 + +// FileLock is an exclusive advisory lock held through an open file handle. +// Release is idempotent. The lock path remains on disk after release; deleting +// or replacing it would let different processes lock different inodes. +type FileLock struct { + mu sync.Mutex + file *os.File + state platformLockState + released bool + releaseErr error +} + +// TryAcquireFileLock attempts to take an exclusive advisory lock without +// waiting. A contended lock returns ErrLockHeld. The kernel releases a lock if +// its process exits, so an old unlocked file never needs stale reclamation. +func TryAcquireFileLock(path string) (*FileLock, error) { + return TryAcquireFileLockAt(filepath.Dir(path), path) +} + +// TryAcquireFileLockAt attempts to lock path while confining every component +// below root to a handle-relative traversal. root is the caller's trusted +// boundary and may itself be reached through a legitimate link; links and +// reparse points below it are rejected before metadata can be written. +func TryAcquireFileLockAt(root, path string) (*FileLock, error) { + if root == "" { + return nil, errors.New("lockutil: lock root is empty") + } + relative, err := filepath.Rel(root, path) if err != nil { - return err + return nil, fmt.Errorf("lockutil: resolve lock path relative to root: %w", err) + } + relative = filepath.Clean(relative) + if relative == "." || relative == ".." || filepath.IsAbs(relative) || strings.HasPrefix(relative, ".."+string(filepath.Separator)) { + return nil, fmt.Errorf("lockutil: lock path %q escapes root %q", path, root) } - dst, err := os.OpenFile(staged, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o600) + + file, err := openLockFileAt(root, relative, path) if err != nil { - _ = src.Close() - return err + return nil, fmt.Errorf("lockutil: open lock file: %w", err) } - _, err = io.Copy(dst, src) - // Close the source before removing anything below: Go opens files without - // FILE_SHARE_DELETE on Windows, so deleting reclaimed or staged while src - // is open would fail with a sharing violation. - _ = src.Close() + state, contended, err := tryLockFile(file) if err != nil { - _ = dst.Close() - _ = os.Remove(staged) - return err + _ = file.Close() + return nil, fmt.Errorf("lockutil: acquire advisory lock: %w", err) } - if err := dst.Close(); err != nil { - _ = os.Remove(staged) - return err + if contended { + _ = file.Close() + return nil, ErrLockHeld } - if err := publish(staged, path); err != nil { - _ = os.Remove(staged) - return err + + lock := &FileLock{file: file, state: state} + metadata := []byte(fmt.Sprintf("%d-%d\n", os.Getpid(), holderSequence.Add(1))) + if err := lock.WriteMetadata(metadata); err != nil { + return nil, errors.Join(err, lock.Release()) + } + return lock, nil +} + +// WriteMetadata replaces the diagnostic contents of the lock file while the +// caller holds it. Metadata does not establish ownership; the kernel lock does. +func (lock *FileLock) WriteMetadata(data []byte) error { + if lock == nil { + return ErrLockReleased + } + lock.mu.Lock() + defer lock.mu.Unlock() + if lock.released || lock.file == nil { + return ErrLockReleased + } + if err := lock.file.Truncate(0); err != nil { + return fmt.Errorf("lockutil: truncate lock metadata: %w", err) + } + if _, err := lock.file.Seek(0, 0); err != nil { + return fmt.Errorf("lockutil: seek lock metadata: %w", err) + } + if _, err := lock.file.Write(data); err != nil { + return fmt.Errorf("lockutil: write lock metadata: %w", err) } - // publish may leave staged behind (a hard-link publish keeps both names), - // so remove it explicitly; a move-based publish already made this a no-op. - _ = os.Remove(staged) - // The lock is back at path, so the restore has succeeded; failing to clean - // up the sidelined name must not be reported as a failed restore. - _ = RemoveLockFile(reclaimed) return nil } + +// Release unlocks and closes the held file handle. It deliberately leaves the +// stable lock file in place. Repeated calls return the first release result. +func (lock *FileLock) Release() error { + if lock == nil { + return nil + } + lock.mu.Lock() + defer lock.mu.Unlock() + if lock.released { + return lock.releaseErr + } + lock.released = true + unlockErr := unlockFile(lock.file, &lock.state) + closeErr := lock.file.Close() + lock.file = nil + if err := errors.Join(unlockErr, closeErr); err != nil { + lock.releaseErr = fmt.Errorf("lockutil: release advisory lock: %w", err) + } + return lock.releaseErr +} diff --git a/internal/lockutil/lockutil_other.go b/internal/lockutil/lockutil_other.go index 40b3122b2..1b3c3affc 100644 --- a/internal/lockutil/lockutil_other.go +++ b/internal/lockutil/lockutil_other.go @@ -4,43 +4,92 @@ package lockutil import ( "errors" - "io/fs" + "fmt" "os" + "path/filepath" + "strings" + + "golang.org/x/sys/unix" ) -// RestoreLockFile restores a sidelined lock file on non-Windows platforms. It -// uses os.Link so a competing lock created at path in the meantime wins: the -// link fails with os.ErrExist instead of overwriting it. If the link fails for -// any other reason (hardlink-incapable filesystems such as FAT or some -// FUSE/network mounts, ENOSPC, EPERM), it falls back to an O_EXCL copy rather -// than leaving path missing and the live holder's lock stranded in reclaimed. -// Once the lock is back at path the restore has succeeded, so a failed cleanup -// of the sidelined name is not reported as an error; the leftover file is -// invisible to the lock protocol. -func RestoreLockFile(reclaimed, path string) error { - err := os.Link(reclaimed, path) - if err == nil { - _ = RemoveLockFile(reclaimed) - return nil +type platformLockState struct{} + +func openLockFileAt(root, relative, displayPath string) (_ *os.File, resultErr error) { + parentFD, err := unix.Open(root, unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC, 0) + if err != nil { + return nil, fmt.Errorf("open lock root: %w", err) + } + defer func() { + if parentFD >= 0 { + resultErr = errors.Join(resultErr, unix.Close(parentFD)) + } + }() + + components := strings.Split(filepath.Clean(relative), string(filepath.Separator)) + for _, component := range components[:len(components)-1] { + nextFD, openErr := unix.Openat(parentFD, component, unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC|unix.O_NOFOLLOW, 0) + if openErr != nil { + return nil, fmt.Errorf("open lock path component %q: %w", component, openErr) + } + if closeErr := unix.Close(parentFD); closeErr != nil { + _ = unix.Close(nextFD) + parentFD = -1 + return nil, fmt.Errorf("close lock path component: %w", closeErr) + } + parentFD = nextFD + } + + name := components[len(components)-1] + fd := -1 + // Darwin can transiently return ENOENT when many callers race to create the + // same O_NOFOLLOW entry. The bound parent handle cannot be redirected, so a + // bounded retry preserves first-use contention without weakening no-follow. + for attempt := 0; attempt < 10; attempt++ { + fd, err = unix.Openat(parentFD, name, unix.O_CREAT|unix.O_RDWR|unix.O_CLOEXEC|unix.O_NOFOLLOW, 0o600) + if !errors.Is(err, unix.ENOENT) { + break + } + } + if err != nil { + return nil, err } - if errors.Is(err, os.ErrExist) { - return err + file := os.NewFile(uintptr(fd), displayPath) + if file == nil { + _ = unix.Close(fd) + return nil, errors.New("convert lock handle to file") } - return restoreByCopy(reclaimed, path, os.Link) + var stat unix.Stat_t + if err := unix.Fstat(fd, &stat); err != nil { + return nil, errors.Join(fmt.Errorf("inspect lock file: %w", err), file.Close()) + } + if stat.Mode&unix.S_IFMT != unix.S_IFREG { + return nil, errors.Join(errors.New("refusing non-regular lock file"), file.Close()) + } + if stat.Nlink != 1 { + return nil, errors.Join(errors.New("refusing multiply-linked lock file"), file.Close()) + } + if err := unix.Close(parentFD); err != nil { + parentFD = -1 + return nil, errors.Join(fmt.Errorf("close lock parent: %w", err), file.Close()) + } + parentFD = -1 + return file, nil } -// isReclaimContended reports whether a failed rename-aside of a suspected -// stale lock means it was lost to a racer rather than a hard failure. POSIX -// rename has no contention errno (a lost race surfaces only as ENOENT, which -// ReclaimStaleLock already treats as benign), so nothing extra is benign here. -func isReclaimContended(error) bool { return false } +func tryLockFile(file *os.File) (platformLockState, bool, error) { + err := unix.Flock(int(file.Fd()), unix.LOCK_EX|unix.LOCK_NB) + if err == nil { + return platformLockState{}, false, nil + } + if errors.Is(err, unix.EWOULDBLOCK) || errors.Is(err, unix.EAGAIN) { + return platformLockState{}, true, nil + } + return platformLockState{}, false, err +} -// RemoveLockFile removes a lock file on non-Windows platforms. Removing an -// already-missing file reports nil, matching the Windows implementation, so -// callers see one cross-platform contract. -func RemoveLockFile(path string) error { - if err := os.Remove(path); err != nil && !errors.Is(err, fs.ErrNotExist) { - return err +func unlockFile(file *os.File, _ *platformLockState) error { + if file == nil { + return nil } - return nil + return unix.Flock(int(file.Fd()), unix.LOCK_UN) } diff --git a/internal/lockutil/lockutil_test.go b/internal/lockutil/lockutil_test.go index d0a63bb7a..ad51a25b7 100644 --- a/internal/lockutil/lockutil_test.go +++ b/internal/lockutil/lockutil_test.go @@ -1,116 +1,220 @@ package lockutil import ( + "bufio" "errors" + "fmt" + "io" "os" + "os/exec" "path/filepath" + "runtime" + "strings" + "sync" "testing" ) -func TestRestoreLockFile(t *testing.T) { - tempDir := t.TempDir() - path := filepath.Join(tempDir, "lock") - reclaimed := filepath.Join(tempDir, "lock.stale.token") - - // 1. Successful restore when target does not exist - if err := os.WriteFile(reclaimed, []byte("token"), 0o600); err != nil { - t.Fatal(err) +func TestFileLockSerializesAndKeepsStablePath(t *testing.T) { + path := filepath.Join(t.TempDir(), "lock") + first, err := TryAcquireFileLock(path) + if err != nil { + t.Fatalf("first acquire: %v", err) } - if err := RestoreLockFile(reclaimed, path); err != nil { - t.Fatalf("RestoreLockFile failed: %v", err) + firstInfo, err := os.Stat(path) + if err != nil { + t.Fatalf("stat first lock path: %v", err) } - if data, err := os.ReadFile(path); err != nil || string(data) != "token" { - t.Fatalf("restored lock content = %q, err %v; want %q intact", data, err, "token") + if _, err := TryAcquireFileLock(path); !errors.Is(err, ErrLockHeld) { + t.Fatalf("contended acquire = %v, want ErrLockHeld", err) } - if _, err := os.Stat(reclaimed); !os.IsNotExist(err) { - t.Fatalf("expected sidelined lock to be cleaned up/removed: %v", err) + if err := first.Release(); err != nil { + t.Fatalf("release first: %v", err) } - - // 2. Fail when target already exists (prevent overwrite) - if err := os.WriteFile(reclaimed, []byte("token"), 0o600); err != nil { - t.Fatal(err) + releasedInfo, err := os.Stat(path) + if err != nil { + t.Fatalf("release removed stable lock path: %v", err) } - if err := os.WriteFile(path, []byte("competing"), 0o600); err != nil { - t.Fatal(err) + if !os.SameFile(firstInfo, releasedInfo) { + t.Fatal("release replaced the stable lock file") } - err := RestoreLockFile(reclaimed, path) - if err == nil { - t.Fatal("expected RestoreLockFile to fail when target exists") + second, err := TryAcquireFileLock(path) + if err != nil { + t.Fatalf("acquire after release: %v", err) } - if !errors.Is(err, os.ErrExist) { - t.Fatalf("expected os.ErrExist, got: %v", err) + secondInfo, err := os.Stat(path) + if err != nil { + t.Fatalf("stat second lock path: %v", err) } - - // The sidelined lock must still exist on failure, and the competing lock - // must be untouched. - if _, err := os.Stat(reclaimed); err != nil { - t.Fatalf("expected sidelined lock to still exist: %v", err) + if !os.SameFile(firstInfo, secondInfo) { + t.Fatal("reacquire used a different lock file") } - if data, _ := os.ReadFile(path); string(data) != "competing" { - t.Fatalf("competing lock was clobbered: %q", data) + if err := second.Release(); err != nil { + t.Fatalf("release second: %v", err) + } + if err := second.Release(); err != nil { + t.Fatalf("idempotent release: %v", err) } } -func TestRestoreByCopy(t *testing.T) { - tempDir := t.TempDir() - path := filepath.Join(tempDir, "lock") - reclaimed := filepath.Join(tempDir, "lock.stale.token") - - // 1. Restores content and cleans up the sidelined file when the target is - // free (the fallback taken when the platform's no-replace primitive fails - // for a reason other than the destination existing). - if err := os.WriteFile(reclaimed, []byte("token"), 0o600); err != nil { +func TestFileLockMetadata(t *testing.T) { + path := filepath.Join(t.TempDir(), "lock") + lock, err := TryAcquireFileLock(path) + if err != nil { t.Fatal(err) } - if err := restoreByCopy(reclaimed, path, os.Link); err != nil { - t.Fatalf("restoreByCopy failed: %v", err) + if err := lock.WriteMetadata([]byte("holder\n")); err != nil { + t.Fatalf("write metadata: %v", err) } - if data, err := os.ReadFile(path); err != nil || string(data) != "token" { - t.Fatalf("restored lock content = %q, err %v; want %q intact", data, err, "token") + data, err := os.ReadFile(path) + if err != nil || string(data) != "holder\n" { + t.Fatalf("metadata = %q, err %v", data, err) } - if _, err := os.Stat(reclaimed); !errors.Is(err, os.ErrNotExist) { - t.Fatalf("expected sidelined lock to be cleaned up after copy: %v", err) + if err := lock.Release(); err != nil { + t.Fatal(err) } - if _, err := os.Stat(reclaimed + ".copy"); !errors.Is(err, os.ErrNotExist) { - t.Fatalf("expected staged copy to be cleaned up: %v", err) + if err := lock.WriteMetadata(nil); !errors.Is(err, ErrLockReleased) { + t.Fatalf("write after release = %v, want ErrLockReleased", err) } +} - // 2. Keeps the no-overwrite guarantee: a competing lock at the target wins - // and the sidelined lock survives for the caller's ErrExist cleanup. - if err := os.WriteFile(reclaimed, []byte("token"), 0o600); err != nil { - t.Fatal(err) +func TestFileLockConcurrentFirstAcquisition(t *testing.T) { + path := filepath.Join(t.TempDir(), "lock") + const contenders = 200 + start := make(chan struct{}) + results := make(chan error, contenders) + locks := make(chan *FileLock, contenders) + var ready sync.WaitGroup + ready.Add(contenders) + for range contenders { + go func() { + ready.Done() + <-start + lock, err := TryAcquireFileLock(path) + if lock != nil { + locks <- lock + } + results <- err + }() + } + ready.Wait() + close(start) + acquired := 0 + for range contenders { + err := <-results + switch { + case err == nil: + acquired++ + case !errors.Is(err, ErrLockHeld): + t.Errorf("concurrent first acquire: %v", err) + } + } + close(locks) + for lock := range locks { + if err := lock.Release(); err != nil { + t.Error(err) + } + } + if acquired != 1 { + t.Fatalf("concurrent first acquire: got %d holders, want 1", acquired) } - if err := os.WriteFile(path, []byte("competing"), 0o600); err != nil { +} + +func TestFileLockRefusesMultiplyLinkedEntry(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "target") + lockPath := filepath.Join(dir, "lock") + const sentinel = "do not overwrite" + if err := os.WriteFile(target, []byte(sentinel), 0o600); err != nil { t.Fatal(err) } - if err := restoreByCopy(reclaimed, path, os.Link); !errors.Is(err, os.ErrExist) { - t.Fatalf("expected os.ErrExist when target exists, got: %v", err) + if err := os.Link(target, lockPath); err != nil { + t.Fatalf("create redirected hard link: %v", err) } - if data, _ := os.ReadFile(path); string(data) != "competing" { - t.Fatalf("competing lock was clobbered: %q", data) + if lock, err := TryAcquireFileLock(lockPath); err == nil { + _ = lock.Release() + t.Fatal("TryAcquireFileLock accepted a multiply-linked entry") } - if _, err := os.Stat(reclaimed); err != nil { - t.Fatalf("expected sidelined lock to still exist: %v", err) + data, err := os.ReadFile(target) + if err != nil { + t.Fatal(err) } - if _, err := os.Stat(reclaimed + ".copy"); !errors.Is(err, os.ErrNotExist) { - t.Fatalf("expected staged copy to be cleaned up after a failed publish: %v", err) + if string(data) != sentinel { + t.Fatalf("redirected target was modified: %q", data) } } -func TestRemoveLockFile(t *testing.T) { +func TestFileLockAtRejectsEscapingPath(t *testing.T) { + base := t.TempDir() + outside := filepath.Join(filepath.Dir(base), "outside.lock") + if lock, err := TryAcquireFileLockAt(base, outside); err == nil { + _ = lock.Release() + t.Fatal("TryAcquireFileLockAt accepted a path outside its root") + } + if _, err := os.Stat(outside); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("escaping path was created: %v", err) + } +} + +// TestFileLockReleasedByProcessExit proves crash recovery is provided by the +// kernel lock lifetime. The child exits without calling Release; the next +// process can then lock the same persistent file without moving or deleting it. +func TestFileLockReleasedByProcessExit(t *testing.T) { path := filepath.Join(t.TempDir(), "lock") - if err := os.WriteFile(path, []byte("token"), 0o600); err != nil { + cmd := exec.Command(os.Args[0], "-test.run=^TestFileLockHelperProcess$") + cmd.Env = append(os.Environ(), "ZERO_LOCKUTIL_HELPER=1", "ZERO_LOCKUTIL_PATH="+path) + stdout, err := cmd.StdoutPipe() + if err != nil { + t.Fatal(err) + } + stdin, err := cmd.StdinPipe() + if err != nil { t.Fatal(err) } - if err := RemoveLockFile(path); err != nil { - t.Fatalf("RemoveLockFile failed: %v", err) + cmd.Stderr = os.Stderr + if err := cmd.Start(); err != nil { + t.Fatal(err) + } + + ready, err := bufio.NewReader(stdout).ReadString('\n') + if err != nil || strings.TrimSpace(ready) != "ready" { + _ = cmd.Process.Kill() + _ = cmd.Wait() + t.Fatalf("helper readiness = %q, err %v", ready, err) + } + if _, err := TryAcquireFileLock(path); !errors.Is(err, ErrLockHeld) { + _ = cmd.Process.Kill() + _ = cmd.Wait() + t.Fatalf("acquire while helper holds lock = %v, want ErrLockHeld", err) + } + if err := stdin.Close(); err != nil { + t.Fatal(err) + } + if err := cmd.Wait(); err != nil { + t.Fatalf("helper exit: %v", err) + } + + lock, err := TryAcquireFileLock(path) + if err != nil { + t.Fatalf("acquire after helper exit: %v", err) + } + if err := lock.Release(); err != nil { + t.Fatal(err) + } +} + +func TestFileLockHelperProcess(t *testing.T) { + if os.Getenv("ZERO_LOCKUTIL_HELPER") != "1" { + return } - if _, err := os.Stat(path); !errors.Is(err, os.ErrNotExist) { - t.Fatalf("expected lock file to be removed: %v", err) + lock, err := TryAcquireFileLock(os.Getenv("ZERO_LOCKUTIL_PATH")) + if err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(2) } - // Removing an already-missing lock file is a no-op on every platform. - if err := RemoveLockFile(path); err != nil { - t.Fatalf("RemoveLockFile on a missing file = %v, want nil", err) + if _, err := fmt.Fprintln(os.Stdout, "ready"); err != nil { + os.Exit(3) } + _, _ = io.Copy(io.Discard, os.Stdin) + runtime.KeepAlive(lock) // deliberately exit without Release } diff --git a/internal/lockutil/lockutil_windows.go b/internal/lockutil/lockutil_windows.go index 71ac1ca67..df98aac80 100644 --- a/internal/lockutil/lockutil_windows.go +++ b/internal/lockutil/lockutil_windows.go @@ -4,77 +4,202 @@ package lockutil import ( "errors" + "fmt" "os" - "syscall" - "time" + "path/filepath" + "strings" + "unsafe" "golang.org/x/sys/windows" ) -// RestoreLockFile restores a sidelined lock file on Windows. It moves the file -// with a no-replace MoveFileEx so a competing lock created at path in the -// meantime wins: the move fails with os.ErrExist instead of overwriting it. If -// the move fails for any other reason (ERROR_SHARING_VIOLATION or -// ERROR_ACCESS_DENIED under concurrent access to the file), it falls back to -// an O_EXCL copy rather than leaving path missing and the live holder's lock -// stranded in reclaimed. -func RestoreLockFile(reclaimed, path string) error { - err := moveFileNoReplace(reclaimed, path) - if err == nil || errors.Is(err, os.ErrExist) { - return err - } - return restoreByCopy(reclaimed, path, moveFileNoReplace) +type platformLockState struct { + overlapped windows.Overlapped } -// moveFileNoReplace renames from to to, failing if to already exists. It calls -// MoveFileExW directly via golang.org/x/sys/windows (the standard library's -// syscall package does not export MoveFileEx on this platform) with no flags, -// so an existing destination fails with ERROR_ALREADY_EXISTS, which satisfies -// errors.Is(err, os.ErrExist) (pinned by -// TestMoveFileNoReplaceMapsAlreadyExistsToErrExist), instead of overwriting it -// the way os.Rename does on Windows (it passes MOVEFILE_REPLACE_EXISTING to -// match POSIX rename semantics cross-platform). -func moveFileNoReplace(from, to string) error { - fromPtr, err := windows.UTF16PtrFromString(from) +const lockByteOffsetHigh = 1 + +func openLockFileAt(root, relative, displayPath string) (_ *os.File, resultErr error) { + rootPtr, err := windows.UTF16PtrFromString(root) + if err != nil { + return nil, err + } + parent, err := windows.CreateFile( + rootPtr, + windows.FILE_READ_ATTRIBUTES|windows.FILE_TRAVERSE|windows.SYNCHRONIZE, + windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE|windows.FILE_SHARE_DELETE, + nil, + windows.OPEN_EXISTING, + windows.FILE_FLAG_BACKUP_SEMANTICS, + 0, + ) if err != nil { - return err + return nil, fmt.Errorf("open lock root: %w", err) + } + defer func() { + if parent != 0 { + resultErr = errors.Join(resultErr, windows.CloseHandle(parent)) + } + }() + + components := strings.Split(filepath.Clean(relative), string(filepath.Separator)) + for _, component := range components[:len(components)-1] { + next, openErr := openLockDirectoryAt(parent, component) + if openErr != nil { + return nil, fmt.Errorf("open lock path component %q: %w", component, openErr) + } + if closeErr := windows.CloseHandle(parent); closeErr != nil { + _ = windows.CloseHandle(next) + parent = 0 + return nil, fmt.Errorf("close lock path component: %w", closeErr) + } + parent = next } - toPtr, err := windows.UTF16PtrFromString(to) + + handle, err := openLockFileHandleAt(parent, components[len(components)-1]) if err != nil { - return err + return nil, err + } + file := os.NewFile(uintptr(handle), displayPath) + if file == nil { + _ = windows.CloseHandle(handle) + return nil, errors.New("convert lock handle to file") } - return windows.MoveFileEx(fromPtr, toPtr, 0) + if err := windows.CloseHandle(parent); err != nil { + parent = 0 + return nil, errors.Join(fmt.Errorf("close lock parent: %w", err), file.Close()) + } + parent = 0 + return file, nil } -// isReclaimContended reports whether a failed rename-aside of a suspected -// stale lock means the file was concurrently open or pending deletion (which -// surfaces as ERROR_SHARING_VIOLATION or ERROR_ACCESS_DENIED on Windows) -// rather than a hard failure, so ReclaimStaleLock reports a lost race instead -// of an error. Neither errno satisfies errors.Is(err, os.ErrExist), and -// ERROR_SHARING_VIOLATION does not satisfy errors.Is(err, os.ErrPermission) -// either, so the classification compares raw errnos like RemoveLockFile below. -func isReclaimContended(err error) bool { - var errno syscall.Errno - return errors.As(err, &errno) && (errno == windows.ERROR_SHARING_VIOLATION || errno == windows.ERROR_ACCESS_DENIED) +func openLockDirectoryAt(parent windows.Handle, name string) (windows.Handle, error) { + handle, err := openLockPathAt( + parent, + name, + windows.FILE_READ_ATTRIBUTES|windows.FILE_TRAVERSE|windows.SYNCHRONIZE, + windows.FILE_OPEN, + windows.FILE_ATTRIBUTE_DIRECTORY, + windows.FILE_DIRECTORY_FILE|windows.FILE_SYNCHRONOUS_IO_NONALERT|windows.FILE_OPEN_REPARSE_POINT, + ) + if err != nil { + return 0, err + } + if err := validateWindowsLockHandle(handle, true); err != nil { + return 0, errors.Join(err, windows.CloseHandle(handle)) + } + return handle, nil } -// RemoveLockFile removes a lock file on Windows, retrying on the sharing -// violation or access denied errors that are common under heavy concurrent -// contention. Removing an already-missing file reports nil, matching the -// non-Windows implementation, so callers see one cross-platform contract. -func RemoveLockFile(path string) error { - var err error - for i := 0; i < 15; i++ { - err = os.Remove(path) - if err == nil || os.IsNotExist(err) { - return nil +func openLockFileHandleAt(parent windows.Handle, name string) (windows.Handle, error) { + for attempt := 0; attempt < 10; attempt++ { + handle, err := openLockPathAt( + parent, + name, + windows.GENERIC_READ|windows.GENERIC_WRITE|windows.SYNCHRONIZE, + windows.FILE_OPEN, + windows.FILE_ATTRIBUTE_NORMAL, + windows.FILE_NON_DIRECTORY_FILE|windows.FILE_SYNCHRONOUS_IO_NONALERT|windows.FILE_OPEN_REPARSE_POINT, + ) + if err == nil { + if err := validateWindowsLockHandle(handle, false); err != nil { + return 0, errors.Join(err, windows.CloseHandle(handle)) + } + return handle, nil + } + if !windowsLockPathMissing(err) { + return 0, err } - var errno syscall.Errno - if errors.As(err, &errno) && (errno == windows.ERROR_SHARING_VIOLATION || errno == windows.ERROR_ACCESS_DENIED) { - time.Sleep(5 * time.Millisecond) + + handle, err = openLockPathAt( + parent, + name, + windows.GENERIC_READ|windows.GENERIC_WRITE|windows.SYNCHRONIZE, + windows.FILE_CREATE, + windows.FILE_ATTRIBUTE_NORMAL, + windows.FILE_NON_DIRECTORY_FILE|windows.FILE_SYNCHRONOUS_IO_NONALERT|windows.FILE_OPEN_REPARSE_POINT, + ) + if err == windows.STATUS_OBJECT_NAME_COLLISION || errors.Is(err, windows.ERROR_ALREADY_EXISTS) { continue } - return err + if err != nil { + return 0, err + } + if err := validateWindowsLockHandle(handle, false); err != nil { + return 0, errors.Join(err, windows.CloseHandle(handle)) + } + return handle, nil + } + return 0, errors.New("lock file creation did not stabilize") +} + +func windowsLockPathMissing(err error) bool { + return err == windows.STATUS_NO_SUCH_FILE || + err == windows.STATUS_OBJECT_NAME_NOT_FOUND || + err == windows.STATUS_OBJECT_PATH_NOT_FOUND || + errors.Is(err, windows.ERROR_FILE_NOT_FOUND) || + errors.Is(err, windows.ERROR_PATH_NOT_FOUND) +} + +func openLockPathAt(parent windows.Handle, name string, access, disposition, attributes, options uint32) (windows.Handle, error) { + objectName, err := windows.NewNTUnicodeString(name) + if err != nil { + return 0, err + } + objectAttributes := &windows.OBJECT_ATTRIBUTES{ + Length: uint32(unsafe.Sizeof(windows.OBJECT_ATTRIBUTES{})), + RootDirectory: parent, + ObjectName: objectName, + Attributes: windows.OBJ_CASE_INSENSITIVE | windows.OBJ_DONT_REPARSE, + } + var handle windows.Handle + err = windows.NtCreateFile( + &handle, + access, + objectAttributes, + &windows.IO_STATUS_BLOCK{}, + nil, + attributes, + windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE|windows.FILE_SHARE_DELETE, + disposition, + options, + 0, + 0, + ) + return handle, err +} + +func validateWindowsLockHandle(handle windows.Handle, wantDirectory bool) error { + var info windows.ByHandleFileInformation + if err := windows.GetFileInformationByHandle(handle, &info); err != nil { + return fmt.Errorf("inspect lock path handle: %w", err) + } + isDirectory := info.FileAttributes&windows.FILE_ATTRIBUTE_DIRECTORY != 0 + if info.FileAttributes&windows.FILE_ATTRIBUTE_REPARSE_POINT != 0 || isDirectory != wantDirectory { + return errors.New("refusing non-file or reparse-point lock path") + } + if !wantDirectory && info.NumberOfLinks != 1 { + return errors.New("refusing multiply-linked lock file") + } + return nil +} + +func tryLockFile(file *os.File) (platformLockState, bool, error) { + state := platformLockState{overlapped: windows.Overlapped{OffsetHigh: lockByteOffsetHigh}} + flags := uint32(windows.LOCKFILE_EXCLUSIVE_LOCK | windows.LOCKFILE_FAIL_IMMEDIATELY) + err := windows.LockFileEx(windows.Handle(file.Fd()), flags, 0, 1, 0, &state.overlapped) + if err == nil { + return state, false, nil + } + if errors.Is(err, windows.ERROR_LOCK_VIOLATION) || errors.Is(err, windows.ERROR_SHARING_VIOLATION) { + return platformLockState{}, true, nil + } + return platformLockState{}, false, err +} + +func unlockFile(file *os.File, state *platformLockState) error { + if file == nil { + return nil } - return err + return windows.UnlockFileEx(windows.Handle(file.Fd()), 0, 1, 0, &state.overlapped) } diff --git a/internal/lockutil/lockutil_windows_test.go b/internal/lockutil/lockutil_windows_test.go deleted file mode 100644 index dd76a4166..000000000 --- a/internal/lockutil/lockutil_windows_test.go +++ /dev/null @@ -1,83 +0,0 @@ -//go:build windows - -package lockutil - -import ( - "errors" - "os" - "path/filepath" - "testing" - "time" - - "golang.org/x/sys/windows" -) - -// TestMoveFileNoReplaceMapsAlreadyExistsToErrExist pins the errno coupling -// every caller keys its cleanup on: with no flags, MoveFileEx fails with -// ERROR_ALREADY_EXISTS when the destination exists, and that errno must -// satisfy errors.Is(err, os.ErrExist). -func TestMoveFileNoReplaceMapsAlreadyExistsToErrExist(t *testing.T) { - dir := t.TempDir() - from := filepath.Join(dir, "from") - to := filepath.Join(dir, "to") - if err := os.WriteFile(from, []byte("sidelined"), 0o600); err != nil { - t.Fatal(err) - } - if err := os.WriteFile(to, []byte("competing"), 0o600); err != nil { - t.Fatal(err) - } - err := moveFileNoReplace(from, to) - if !errors.Is(err, windows.ERROR_ALREADY_EXISTS) { - t.Fatalf("MoveFileEx with an existing destination = %v, want ERROR_ALREADY_EXISTS", err) - } - if !errors.Is(err, os.ErrExist) { - t.Fatalf("ERROR_ALREADY_EXISTS must satisfy errors.Is(err, os.ErrExist), got: %v", err) - } - if data, _ := os.ReadFile(to); string(data) != "competing" { - t.Fatalf("destination was overwritten: %q", data) - } -} - -// TestRemoveLockFileRetriesSharingViolation holds the file open briefly (Go -// opens files without FILE_SHARE_DELETE on Windows, so a concurrent DeleteFile -// fails with ERROR_SHARING_VIOLATION while the handle is open); RemoveLockFile -// must ride its retry loop (15 x 5ms) past the window and succeed once the -// handle closes. -func TestRemoveLockFileRetriesSharingViolation(t *testing.T) { - path := filepath.Join(t.TempDir(), "lock") - if err := os.WriteFile(path, []byte("token"), 0o600); err != nil { - t.Fatal(err) - } - f, err := os.Open(path) - if err != nil { - t.Fatal(err) - } - go func() { - time.Sleep(10 * time.Millisecond) - _ = f.Close() - }() - if err := RemoveLockFile(path); err != nil { - t.Fatalf("RemoveLockFile should succeed once the contending handle closes: %v", err) - } - if _, err := os.Stat(path); !errors.Is(err, os.ErrNotExist) { - t.Fatalf("expected lock file to be removed: %v", err) - } -} - -// TestRemoveLockFileSharingViolationExhausts verifies the retry loop is -// bounded: with the contending handle held for the whole call, RemoveLockFile -// surfaces the sharing violation instead of retrying forever. -func TestRemoveLockFileSharingViolationExhausts(t *testing.T) { - path := filepath.Join(t.TempDir(), "lock") - if err := os.WriteFile(path, []byte("token"), 0o600); err != nil { - t.Fatal(err) - } - f, err := os.Open(path) - if err != nil { - t.Fatal(err) - } - defer f.Close() - if err := RemoveLockFile(path); !errors.Is(err, windows.ERROR_SHARING_VIOLATION) { - t.Fatalf("RemoveLockFile with a held handle = %v, want ERROR_SHARING_VIOLATION", err) - } -} diff --git a/internal/lockutil/reclaim.go b/internal/lockutil/reclaim.go deleted file mode 100644 index 6aaa414c4..000000000 --- a/internal/lockutil/reclaim.go +++ /dev/null @@ -1,82 +0,0 @@ -package lockutil - -import ( - "errors" - "os" -) - -// restoreLockFile is swappable so tests can force the fail-closed path of -// ReclaimStaleLock, which requires both the fast restore and its no-replace -// fallback (with its own copy fallback) to fail; that cannot be provoked -// portably on a healthy filesystem. -var restoreLockFile = restoreLiveLock - -// restoreLiveLock puts a lock that turned out to be live back at path after -// ReclaimStaleLock moved it aside to inspect it. It first tries a fast, -// replacing rename straight from reclaimed to path: a single syscall, which -// keeps the window during which path does not exist (and so is open to a -// fourth process's unrelated O_EXCL create landing on it) as short as -// possible. RestoreLockFile's no-replace restore (with its slower copy -// fallback on some failures) is a correctness-preserving fallback for when -// the fast path itself fails (e.g. a cross-device sidelined name): it is a -// much longer version of the identical race, but still detects rather than -// silently clobbers a competing lock, which the fast path's replacing rename -// cannot do. Neither path makes the race impossible, only unlikely; see -// ReclaimStaleLock's doc comment. -func restoreLiveLock(reclaimed, path string) error { - if err := os.Rename(reclaimed, path); err == nil { - return nil - } - return RestoreLockFile(reclaimed, path) -} - -// ReclaimStaleLock atomically reclaims a suspected-stale lock file. It renames -// lockPath aside to ".stale." (only one racer can win the -// rename of a given file, so two racers can never both reclaim the same lock), -// then consults isLive on the moved file; if the lock turns out to be live (a -// holder reacquired it in the gap between the caller's stale check and the -// rename) it is restored rather than stolen. The suffix must be unique per -// acquirer attempt. Returns true only when a genuinely stale lock was removed, -// so the caller knows it is safe to retry its exclusive create immediately; on -// a lost race it returns false. A non-nil error means either the rename aside -// failed for a reason that is not contention (so retrying cannot help and the -// caller should fail fast instead of spinning to its deadline), or a live -// holder's lock could not be restored (both restoreLiveLock's fast path and -// its no-replace fallback failed), so lockPath may be missing; callers must -// fail closed instead of re-acquiring. The sidelined file is removed on every -// restore failure: once the restore has failed it has no protocol function -// (release only consults the lock path), so keeping it would only leak files. -// -// The live-restore path has an inherent, unclosed race: between the rename -// aside above and restoreLiveLock putting the lock back, lockPath does not -// exist, so an unrelated caller's O_EXCL create can legitimately succeed -// there. restoreLiveLock's fast path then silently overwrites that new -// claimant's lock file, which does not corrupt release (it is -// ownership-aware, so the new claimant's later release safely no-ops against -// content it no longer owns) but does mean the new claimant can still run -// its critical section concurrently with the original live holder. Making -// this race actually impossible would need an OS-level advisory lock (flock -// / LockFileEx) held for a holder's whole critical section, checked -// non-destructively instead of by moving the file; restoreLiveLock only -// shrinks the window to roughly one syscall, it does not close it. -func ReclaimStaleLock(lockPath, suffix string, isLive func(reclaimedPath string) bool) (bool, error) { - reclaimed := lockPath + ".stale." + suffix - if err := os.Rename(lockPath, reclaimed); err != nil { - if errors.Is(err, os.ErrNotExist) || isReclaimContended(err) { - return false, nil // another racer already moved/removed it, or it vanished - } - return false, err - } - if isLive(reclaimed) { - // Put the live lock back instead of stealing it, and let the caller wait. - if rerr := restoreLockFile(reclaimed, lockPath); rerr != nil { - _ = RemoveLockFile(reclaimed) - if !errors.Is(rerr, os.ErrExist) { - return false, rerr - } - } - return false, nil - } - _ = RemoveLockFile(reclaimed) - return true, nil -} diff --git a/internal/lockutil/reclaim_other_test.go b/internal/lockutil/reclaim_other_test.go deleted file mode 100644 index 240809fb7..000000000 --- a/internal/lockutil/reclaim_other_test.go +++ /dev/null @@ -1,31 +0,0 @@ -//go:build !windows - -package lockutil - -import ( - "os" - "path/filepath" - "testing" -) - -// TestReclaimStaleLockPropagatesRenameFailure pins the fail-fast contract of -// the rename aside: a failure that is not a lost race must reach the caller. -// Mapping it to the benign lost-race result would leave acquirers spinning -// until their deadline and reporting a timeout instead of the real cause. A -// directory squatting on the sidelined name makes os.Rename fail with EISDIR, -// which matches neither os.ErrNotExist nor a contention errno. -func TestReclaimStaleLockPropagatesRenameFailure(t *testing.T) { - lockPath := filepath.Join(t.TempDir(), "lock") - if err := os.WriteFile(lockPath, []byte("stale-holder"), 0o600); err != nil { - t.Fatal(err) - } - if err := os.Mkdir(lockPath+".stale.tok", 0o755); err != nil { - t.Fatal(err) - } - if ok, err := ReclaimStaleLock(lockPath, "tok", func(string) bool { return false }); err == nil || ok { - t.Fatalf("a hard rename failure must surface an error (ok=%v err=%v)", ok, err) - } - if _, err := os.Stat(lockPath); err != nil { - t.Fatalf("the lock file must be left in place on a failed rename: %v", err) - } -} diff --git a/internal/lockutil/reclaim_test.go b/internal/lockutil/reclaim_test.go deleted file mode 100644 index a4dd992e3..000000000 --- a/internal/lockutil/reclaim_test.go +++ /dev/null @@ -1,112 +0,0 @@ -package lockutil - -import ( - "errors" - "os" - "path/filepath" - "testing" -) - -func TestReclaimStaleLock(t *testing.T) { - lockPath := filepath.Join(t.TempDir(), "lock") - dead := func(string) bool { return false } - live := func(string) bool { return true } - - // A lock the predicate reports dead is reclaimed and removed. - if err := os.WriteFile(lockPath, []byte("crashed-holder"), 0o600); err != nil { - t.Fatal(err) - } - if ok, err := ReclaimStaleLock(lockPath, "tok-a", dead); err != nil || !ok { - t.Fatalf("a dead lock should be reclaimed (ok=%v err=%v)", ok, err) - } - if _, err := os.Stat(lockPath); !errors.Is(err, os.ErrNotExist) { - t.Fatalf("reclaimed dead lock should be gone, stat err=%v", err) - } - - // A LIVE lock (a holder reacquired in the gap) must be RESTORED intact. - if err := os.WriteFile(lockPath, []byte("live-holder"), 0o600); err != nil { - t.Fatal(err) - } - if ok, err := ReclaimStaleLock(lockPath, "tok-b", live); err != nil || ok { - t.Fatalf("a live lock must not be reclaimed (ok=%v err=%v)", ok, err) - } - if data, err := os.ReadFile(lockPath); err != nil || string(data) != "live-holder" { - t.Fatalf("live lock must be left intact, got %q err %v", data, err) - } - - // A missing lock reports no reclaim (nothing to steal). - _ = os.Remove(lockPath) - if ok, err := ReclaimStaleLock(lockPath, "tok-c", live); err != nil || ok { - t.Fatalf("a missing lock should not report a reclaim (ok=%v err=%v)", ok, err) - } -} - -// restoreLiveLock's fast path is a single replacing rename, chosen to -// minimize (not eliminate) the window during which lockPath is absent and -// open to an unrelated O_EXCL create; see ReclaimStaleLock's doc comment for -// why that residual race exists. This test pins the resulting behavior: the -// fast path overwrites a competing file at path rather than detecting it, -// unlike RestoreLockFile's own no-replace contract. -func TestRestoreLiveLockFastPathOverwritesCompetingFile(t *testing.T) { - dir := t.TempDir() - reclaimed := filepath.Join(dir, "lock.stale.tok") - path := filepath.Join(dir, "lock") - - if err := os.WriteFile(reclaimed, []byte("original-holder"), 0o600); err != nil { - t.Fatal(err) - } - if err := os.WriteFile(path, []byte("new-claimant"), 0o600); err != nil { - t.Fatal(err) - } - if err := restoreLiveLock(reclaimed, path); err != nil { - t.Fatalf("restoreLiveLock failed: %v", err) - } - data, err := os.ReadFile(path) - if err != nil || string(data) != "original-holder" { - t.Fatalf("path = %q, err %v; want the original holder's content restored", data, err) - } - if _, err := os.Stat(reclaimed); !errors.Is(err, os.ErrNotExist) { - t.Fatalf("expected reclaimed to be consumed by the rename: %v", err) - } -} - -func TestReclaimStaleLockFailsClosedOnRestoreError(t *testing.T) { - // When both the no-replace restore and its copy fallback fail (only - // provokable via the seam; a healthy filesystem cannot produce it), the - // caller must receive an error so it fails closed instead of re-acquiring a - // missing lock path, and the sidelined file must not leak. - restoreLockFile = func(reclaimed, path string) error { return errors.New("restore failed") } - defer func() { restoreLockFile = restoreLiveLock }() - - lockPath := filepath.Join(t.TempDir(), "lock") - if err := os.WriteFile(lockPath, []byte("live-holder"), 0o600); err != nil { - t.Fatal(err) - } - ok, err := ReclaimStaleLock(lockPath, "tok", func(string) bool { return true }) - if err == nil || ok { - t.Fatalf("a failed restore must surface an error (ok=%v err=%v)", ok, err) - } - if matches, _ := filepath.Glob(lockPath + ".stale.*"); len(matches) != 0 { - t.Fatalf("a failed restore must not leak sidelined files: %v", matches) - } -} - -func TestReclaimStaleLockDropsSidelinedWhenNewHolderWins(t *testing.T) { - // An os.ErrExist restore failure means a new holder recreated the lock - // path; that is not an error for the caller, and the sidelined file is - // dropped rather than leaked. - restoreLockFile = func(reclaimed, path string) error { return os.ErrExist } - defer func() { restoreLockFile = restoreLiveLock }() - - lockPath := filepath.Join(t.TempDir(), "lock") - if err := os.WriteFile(lockPath, []byte("live-holder"), 0o600); err != nil { - t.Fatal(err) - } - ok, err := ReclaimStaleLock(lockPath, "tok", func(string) bool { return true }) - if err != nil || ok { - t.Fatalf("losing to a new holder is not an error (ok=%v err=%v)", ok, err) - } - if matches, _ := filepath.Glob(lockPath + ".stale.*"); len(matches) != 0 { - t.Fatalf("the sidelined file must be dropped when a new holder wins: %v", matches) - } -} diff --git a/internal/lockutil/reclaim_windows_test.go b/internal/lockutil/reclaim_windows_test.go deleted file mode 100644 index 8bee82a92..000000000 --- a/internal/lockutil/reclaim_windows_test.go +++ /dev/null @@ -1,50 +0,0 @@ -//go:build windows - -package lockutil - -import ( - "os" - "path/filepath" - "testing" -) - -// TestReclaimStaleLockTreatsSharingViolationAsLostRace pins the errno -// classification the Windows acquire loops depend on: a rename aside that -// fails because the lock file is concurrently open (Go opens files without -// FILE_SHARE_DELETE, so the rename fails with ERROR_SHARING_VIOLATION or -// ERROR_ACCESS_DENIED) is contention, not a hard failure, and must report a -// benign lost race so the caller keeps waiting. -func TestReclaimStaleLockTreatsSharingViolationAsLostRace(t *testing.T) { - lockPath := filepath.Join(t.TempDir(), "lock") - if err := os.WriteFile(lockPath, []byte("held"), 0o600); err != nil { - t.Fatal(err) - } - f, err := os.Open(lockPath) - if err != nil { - t.Fatal(err) - } - defer f.Close() - if ok, rerr := ReclaimStaleLock(lockPath, "tok", func(string) bool { return true }); rerr != nil || ok { - t.Fatalf("a sharing-violation rename must report a benign lost race (ok=%v err=%v)", ok, rerr) - } - if _, err := os.Stat(lockPath); err != nil { - t.Fatalf("the lock file must be left in place: %v", err) - } -} - -// TestReclaimStaleLockPropagatesRenameFailure pins the fail-fast contract of -// the rename aside on Windows: a failure that is not contention must reach -// the caller instead of masquerading as a lost race. A quote is an invalid -// filename character, so the sidelined name fails with ERROR_INVALID_NAME. -func TestReclaimStaleLockPropagatesRenameFailure(t *testing.T) { - lockPath := filepath.Join(t.TempDir(), "lock") - if err := os.WriteFile(lockPath, []byte("stale-holder"), 0o600); err != nil { - t.Fatal(err) - } - if ok, err := ReclaimStaleLock(lockPath, `bad"name`, func(string) bool { return false }); err == nil || ok { - t.Fatalf("a hard rename failure must surface an error (ok=%v err=%v)", ok, err) - } - if _, err := os.Stat(lockPath); err != nil { - t.Fatalf("the lock file must be left in place on a failed rename: %v", err) - } -} diff --git a/internal/oauth/lock.go b/internal/oauth/lock.go index d1dd344d2..aa0050d10 100644 --- a/internal/oauth/lock.go +++ b/internal/oauth/lock.go @@ -5,24 +5,16 @@ import ( "fmt" "os" "path/filepath" - "sync/atomic" "time" "github.com/Gitlawb/zero/internal/lockutil" ) -const ( - fileLockTimeout = 5 * time.Second - fileLockStaleAfter = 30 * time.Second -) - -var lockSeq atomic.Uint64 +const fileLockTimeout = 5 * time.Second -// acquireFileLock takes a cross-process exclusive lock by creating lockPath with -// O_EXCL. It retries with a short backoff until a timeout, breaking a lock whose -// file is older than fileLockStaleAfter (so a crashed holder cannot deadlock the -// store). Release is ownership-aware: it removes the lock only if it still holds -// our token, so a stale-broken holder cannot delete a newer holder's lock. +// acquireFileLock takes a cross-process advisory lock through a stable lock +// file. It retries with a short backoff until a timeout. Kernel lock lifetime +// provides crash recovery, so the path is never renamed or removed. func acquireFileLock(lockPath string, now func() time.Time) (func(), error) { if now == nil { now = time.Now @@ -30,64 +22,15 @@ func acquireFileLock(lockPath string, now func() time.Time) (func(), error) { if err := os.MkdirAll(filepath.Dir(lockPath), 0o700); err != nil { return nil, err } - token := fmt.Sprintf("%d-%d-%d", os.Getpid(), now().UnixNano(), lockSeq.Add(1)) deadline := now().Add(fileLockTimeout) for { - f, err := os.OpenFile(lockPath, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o600) + lock, err := lockutil.TryAcquireFileLock(lockPath) if err == nil { - // A partial/failed write would leave a lock file without our token, so - // the releaser below could never delete it — stranding the lock for - // other processes. Fail closed: remove the file and surface the error. - if _, werr := f.WriteString(token); werr != nil { - _ = f.Close() - _ = lockutil.RemoveLockFile(lockPath) - return nil, fmt.Errorf("oauth: write token lock: %w", werr) - } - if cerr := f.Close(); cerr != nil { - _ = lockutil.RemoveLockFile(lockPath) - return nil, fmt.Errorf("oauth: close token lock: %w", cerr) - } - var released bool - return func() { - if released { - return - } - released = true - if data, rerr := os.ReadFile(lockPath); rerr == nil && string(data) == token { - _ = lockutil.RemoveLockFile(lockPath) - } - }, nil + return func() { _ = lock.Release() }, nil } - // On Windows a concurrent holder's os.Remove leaves the lock file in a - // "delete pending" state, so an O_EXCL create races it with - // ERROR_ACCESS_DENIED (os.ErrPermission) rather than ErrExist. Treat that - // as contention and retry, exactly like ErrExist — otherwise the lock - // spuriously fails under concurrency on Windows. - if !errors.Is(err, os.ErrExist) && !errors.Is(err, os.ErrPermission) { + if !errors.Is(err, lockutil.ErrLockHeld) { return nil, fmt.Errorf("oauth: acquire token lock: %w", err) } - // Reclaim a stale lock left by a crashed holder — atomically (H3). A blind - // Remove lets two racers both reclaim + recreate and so both hold the lock; - // reclaimStaleLock renames the file aside (only one rename wins) and restores - // it if it turns out fresh, so a live lock is never deleted out from under it. - if info, statErr := os.Stat(lockPath); statErr == nil && time.Since(info.ModTime()) > fileLockStaleAfter { - cleared, rerr := lockutil.ReclaimStaleLock(lockPath, token, func(reclaimedPath string) bool { - info, err := os.Stat(reclaimedPath) - return err == nil && time.Since(info.ModTime()) <= fileLockStaleAfter - }) - if rerr != nil { - // Reclaim hit a hard failure: the rename aside failed outright, or a - // live holder's lock could not be put back (the lock path may be - // missing, so re-acquiring would break mutual exclusion). Fail closed - // instead of spinning to the deadline. - return nil, fmt.Errorf("oauth: reclaim stale token lock: %w", rerr) - } - if cleared { - continue - } - // Lost the reclaim race (or it was actually fresh) — fall through to the - // bounded wait rather than hot-spinning on a reclaim that never wins. - } if now().After(deadline) { return nil, fmt.Errorf("oauth: timed out acquiring token lock %s", filepath.Base(lockPath)) } diff --git a/internal/swarm/lock_other.go b/internal/swarm/lock_other.go deleted file mode 100644 index d7baaf6e9..000000000 --- a/internal/swarm/lock_other.go +++ /dev/null @@ -1,15 +0,0 @@ -//go:build !windows - -package swarm - -import ( - "errors" - "os" -) - -// isLockContended reports whether an O_EXCL lock-create error means the lock is -// currently held (so the caller should retry/wait) rather than a hard failure. -// On POSIX an existing lock surfaces only as os.ErrExist. -func isLockContended(err error) bool { - return errors.Is(err, os.ErrExist) -} diff --git a/internal/swarm/lock_redirect_other_test.go b/internal/swarm/lock_redirect_other_test.go new file mode 100644 index 000000000..9dce81c99 --- /dev/null +++ b/internal/swarm/lock_redirect_other_test.go @@ -0,0 +1,18 @@ +//go:build !windows + +package swarm + +import ( + "os" + "path/filepath" + "testing" +) + +func makeRedirectedLockPath(t *testing.T, root, target string) string { + t.Helper() + lockPath := filepath.Join(root, "redirected.lock") + if err := os.Symlink(target, lockPath); err != nil { + t.Fatalf("create redirected lock path: %v", err) + } + return lockPath +} diff --git a/internal/swarm/lock_redirect_windows_test.go b/internal/swarm/lock_redirect_windows_test.go new file mode 100644 index 000000000..d65428e27 --- /dev/null +++ b/internal/swarm/lock_redirect_windows_test.go @@ -0,0 +1,20 @@ +//go:build windows + +package swarm + +import ( + "os/exec" + "path/filepath" + "strings" + "testing" +) + +func makeRedirectedLockPath(t *testing.T, root, target string) string { + t.Helper() + linkDir := filepath.Join(root, "redirected") + out, err := exec.Command("cmd", "/c", "mklink", "/J", linkDir, filepath.Dir(target)).CombinedOutput() + if err != nil { + t.Fatalf("create redirected lock junction: %v %s", err, strings.TrimSpace(string(out))) + } + return filepath.Join(linkDir, filepath.Base(target)) +} diff --git a/internal/swarm/lock_windows.go b/internal/swarm/lock_windows.go deleted file mode 100644 index 543786cea..000000000 --- a/internal/swarm/lock_windows.go +++ /dev/null @@ -1,30 +0,0 @@ -//go:build windows - -package swarm - -import ( - "errors" - "os" - "syscall" -) - -// Windows error codes that an O_EXCL create (or a racing delete) can surface -// when another writer holds the lock file open, instead of ERROR_FILE_EXISTS. -// Treating them as "contended" makes the lock retry/wait rather than fail the -// whole send/read spuriously under contention. -const ( - errorAccessDenied = syscall.Errno(5) // ERROR_ACCESS_DENIED - errorSharingViolation = syscall.Errno(32) // ERROR_SHARING_VIOLATION -) - -// isLockContended reports whether an O_EXCL lock-create error means the lock is -// currently held (so the caller should retry/wait) rather than a hard failure. -// On Windows a held lock can appear as ERROR_FILE_EXISTS (mapped to os.ErrExist) -// but also as ERROR_ACCESS_DENIED / ERROR_SHARING_VIOLATION when the file is -// concurrently open or pending deletion. -func isLockContended(err error) bool { - if errors.Is(err, os.ErrExist) { - return true - } - return errors.Is(err, errorAccessDenied) || errors.Is(err, errorSharingViolation) -} diff --git a/internal/swarm/mailbox.go b/internal/swarm/mailbox.go index 904c0abcb..4ef25178e 100644 --- a/internal/swarm/mailbox.go +++ b/internal/swarm/mailbox.go @@ -8,7 +8,6 @@ import ( "path/filepath" "regexp" "strings" - "sync/atomic" "time" "github.com/Gitlawb/zero/internal/fsutil" @@ -17,10 +16,10 @@ import ( // Mailbox is a per-agent, per-team message inbox persisted as a JSON array on // disk at //inboxes/.json, written atomically under a -// lock file. It is hardened for untrusted input: +// advisory lock. It is hardened for untrusted input: // // - inbox files and their parent dirs are owner-only (0600/0700); -// - every write takes an exclusive lock file (bounded retry, stale-break); +// - every write takes an exclusive advisory lock (bounded retry); // - message bodies and whole inboxes are size-capped (fail closed on oversize); // - team/agent names are sanitized so a name can never escape the base dir; // - malformed inbox JSON is rejected rather than silently reset (fail closed). @@ -44,7 +43,6 @@ const ( defaultMaxMessageBytes = 64 * 1024 // 64 KiB per message defaultMaxMessages = 1000 // per inbox defaultLockTimeout = 5 * time.Second - lockStaleAfter = 30 * time.Second inboxFileMode = 0o600 inboxDirMode = 0o700 ) @@ -206,7 +204,7 @@ func (m *Mailbox) Send(team, recipient string, msg Message) error { if err := m.ensureInboxDir(path); err != nil { return err } - release, err := acquireLock(path+".lock", m.lockTimeout()) + release, err := acquireLock(m.BaseDir, path+".lock", m.lockTimeout()) if err != nil { return err } @@ -233,12 +231,12 @@ func (m *Mailbox) ReadAndConsume(team, recipient string) ([]Message, error) { if err != nil { return nil, err } - // The lock file lives beside the inbox; create the dir so it can be taken + // The stable lock file lives beside the inbox; create the dir so it can be taken // even on first read of a not-yet-existing inbox. if err := m.ensureInboxDir(path); err != nil { return nil, err } - release, err := acquireLock(path+".lock", m.lockTimeout()) + release, err := acquireLock(m.BaseDir, path+".lock", m.lockTimeout()) if err != nil { return nil, err } @@ -334,63 +332,19 @@ func (m *Mailbox) atomicWriteJSON(path string, data any) error { return nil } -// lockSeq makes each lock token unique within the process even when two -// goroutines acquire in the same nanosecond. -var lockSeq atomic.Uint64 - -// acquireLock takes an exclusive lock by creating lockPath with O_EXCL. It -// retries with a short backoff until timeout, breaking a lock whose file is -// older than lockStaleAfter (so a crashed holder cannot deadlock the swarm). -// -// Each holder writes a unique token into the lock file and release is -// OWNERSHIP-AWARE: it removes the lock only if it still holds our token. After a -// stale-break another writer may legitimately own the lock; unconditionally -// removing it would let a third writer in and create split-brain writers that -// can corrupt or drop mailbox updates. -func acquireLock(lockPath string, timeout time.Duration) (func(), error) { - token := fmt.Sprintf("%d-%d-%d", os.Getpid(), time.Now().UnixNano(), lockSeq.Add(1)) +// acquireLock takes an exclusive advisory lock through a stable lock file. It +// retries with a short backoff until timeout. The kernel releases the lock when +// a holder exits, so crash recovery never moves or deletes the canonical path. +func acquireLock(root, lockPath string, timeout time.Duration) (func(), error) { deadline := time.Now().Add(timeout) for { - f, err := os.OpenFile(lockPath, os.O_CREATE|os.O_EXCL|os.O_WRONLY, inboxFileMode) + lock, err := lockutil.TryAcquireFileLockAt(root, lockPath) if err == nil { - // Record our ownership token; the exclusive create already holds the - // lock, so a write error only weakens diagnostics, not safety. - _, _ = f.WriteString(token) - f.Close() - var released bool - return func() { - if released { - return - } - released = true - if data, rerr := os.ReadFile(lockPath); rerr == nil && string(data) == token { - _ = lockutil.RemoveLockFile(lockPath) - } - }, nil + return func() { _ = lock.Release() }, nil } - if !isLockContended(err) { + if !errors.Is(err, lockutil.ErrLockHeld) { return nil, fmt.Errorf("swarm: acquire lock: %w", err) } - // Lock held: break it if stale via the atomic rename-with-verify in - // lockutil.ReclaimStaleLock (AUDIT-M13), otherwise wait. - if info, statErr := os.Stat(lockPath); statErr == nil && time.Since(info.ModTime()) > lockStaleAfter { - cleared, rerr := lockutil.ReclaimStaleLock(lockPath, token, func(reclaimedPath string) bool { - info, err := os.Stat(reclaimedPath) - return err == nil && time.Since(info.ModTime()) <= lockStaleAfter - }) - if rerr != nil { - // Reclaim hit a hard failure: the rename aside failed outright, or a - // live holder's lock could not be put back (the lock path may be - // missing, so re-acquiring would break mutual exclusion). Fail closed - // instead of spinning to the deadline. - return nil, fmt.Errorf("swarm: reclaim stale lock: %w", rerr) - } - if cleared { - continue // cleared a genuinely stale lock; retry the O_EXCL create now - } - // Lost the reclaim race (or it was actually fresh) — fall through to the - // bounded wait instead of hot-spinning on a reclaim that never wins. - } if time.Now().After(deadline) { return nil, fmt.Errorf("swarm: timed out acquiring lock %s", filepath.Base(lockPath)) } diff --git a/internal/swarm/mailbox_test.go b/internal/swarm/mailbox_test.go index 2e59e4a54..eb60911fe 100644 --- a/internal/swarm/mailbox_test.go +++ b/internal/swarm/mailbox_test.go @@ -219,64 +219,75 @@ func TestMailboxRejectsSymlinkedInboxDir(t *testing.T) { } } -func TestMailboxLockReleaseIsOwnershipAware(t *testing.T) { +func TestMailboxLockReleaseKeepsStablePath(t *testing.T) { mb := newTestMailbox(t) path, _ := mb.inboxPath("team", "bob") if err := mb.ensureInboxDir(path); err != nil { t.Fatalf("ensureInboxDir: %v", err) } lockPath := path + ".lock" - // Writer A acquires the lock. - releaseA, err := acquireLock(lockPath, time.Second) + releaseA, err := acquireLock(mb.BaseDir, lockPath, time.Second) if err != nil { t.Fatalf("acquire A: %v", err) } - // Simulate a stale-break + takeover by writer B: overwrite the lock content - // with B's token (as a fresh acquire after a break would). - if err := os.WriteFile(lockPath, []byte("writer-B-token"), 0o600); err != nil { - t.Fatalf("simulate B takeover: %v", err) - } - // A's release must NOT delete B's lock (ownership-aware). releaseA() if _, err := os.Stat(lockPath); err != nil { - t.Fatalf("A's release deleted B's lock (split-brain): %v", err) + t.Fatalf("release removed stable advisory-lock path: %v", err) } - // B's own release removes it. - os.Remove(lockPath) } -func TestAcquireLockReclaimsStaleLock(t *testing.T) { - // A crashed holder's stale lock (old mtime) must be reclaimed via the atomic - // rename-with-verify path, leaving no sidelined .stale.* file (AUDIT-M13). +func TestAcquireLockIgnoresOldUnlockedFile(t *testing.T) { + // A crashed holder can leave an old file behind, but the kernel releases its + // advisory lock. Acquisition must succeed without renaming the stable path. lockPath := filepath.Join(t.TempDir(), "x.lock") if err := os.WriteFile(lockPath, []byte("dead-holder"), 0o600); err != nil { - t.Fatalf("seed stale lock: %v", err) + t.Fatalf("seed old lock metadata: %v", err) } - old := time.Now().Add(-2 * lockStaleAfter) + old := time.Now().Add(-time.Hour) if err := os.Chtimes(lockPath, old, old); err != nil { t.Fatalf("chtimes: %v", err) } - release, err := acquireLock(lockPath, time.Second) + release, err := acquireLock(filepath.Dir(lockPath), lockPath, time.Second) if err != nil { - t.Fatalf("acquireLock should reclaim a genuinely stale lock, got %v", err) + t.Fatalf("acquire over old unlocked file: %v", err) } release() - if matches, _ := filepath.Glob(lockPath + ".stale.*"); len(matches) != 0 { - t.Fatalf("reclaim left sidelined files: %v", matches) + if _, err := os.Stat(lockPath); err != nil { + t.Fatalf("stable lock path missing after release: %v", err) } } -func TestAcquireLockDoesNotBreakFreshLock(t *testing.T) { - // A fresh, held lock (recent mtime) must never be broken — the stale-break must - // not steal a live lock (AUDIT-M13). +func TestAcquireLockDoesNotBreakHeldLock(t *testing.T) { + // File age is irrelevant while the kernel reports an active holder. lockPath := filepath.Join(t.TempDir(), "x.lock") - release, err := acquireLock(lockPath, time.Second) + release, err := acquireLock(filepath.Dir(lockPath), lockPath, time.Second) if err != nil { t.Fatalf("first acquire: %v", err) } defer release() - if _, err := acquireLock(lockPath, 50*time.Millisecond); err == nil { - t.Fatal("acquireLock broke a fresh, held lock (split-brain risk)") + if _, err := acquireLock(filepath.Dir(lockPath), lockPath, 50*time.Millisecond); err == nil { + t.Fatal("acquireLock admitted a second holder") + } +} + +func TestAcquireLockRefusesRedirectedPath(t *testing.T) { + root := t.TempDir() + target := filepath.Join(t.TempDir(), "target") + const sentinel = "do not overwrite" + if err := os.WriteFile(target, []byte(sentinel), 0o600); err != nil { + t.Fatal(err) + } + lockPath := makeRedirectedLockPath(t, root, target) + if release, err := acquireLock(root, lockPath, time.Second); err == nil { + release() + t.Fatal("acquireLock followed a redirected lock path") + } + data, err := os.ReadFile(target) + if err != nil { + t.Fatal(err) + } + if string(data) != sentinel { + t.Fatalf("redirected target was modified: %q", data) } }