diff --git a/internal/daemon/lock.go b/internal/daemon/lock.go index 239ec3d04..a35e0627e 100644 --- a/internal/daemon/lock.go +++ b/internal/daemon/lock.go @@ -8,10 +8,16 @@ import ( "strconv" "strings" "sync/atomic" + "time" "github.com/Gitlawb/zero/internal/lockutil" ) +const ( + daemonLockStaleAfter = 30 * time.Second + daemonLockRetryDelay = 10 * time.Millisecond +) + // 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 @@ -61,19 +67,31 @@ func acquireLock(path string, isAlive func(pid int) bool) (*fileLock, error) { 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 + // If PID could not be parsed (e.g. 0-byte file during active creation), + // check if the file is fresh (created recently). If so, wait briefly and retry. + if perr != nil || pid <= 0 { + if info, statErr := os.Stat(path); statErr == nil && time.Since(info.ModTime()) <= daemonLockStaleAfter { + time.Sleep(daemonLockRetryDelay) + continue + } + } + // Stale lock (dead PID or old unparseable lock) — 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 { + cleared, rerr := reclaimStaleLock(path, isAlive) + 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 the single-instance guarantee). // Fail closed instead of spinning to the deadline. return nil, fmt.Errorf("daemon: reclaim stale lock: %w", rerr) } + if !cleared { + time.Sleep(daemonLockRetryDelay) + } } return nil, ErrAlreadyRunning } @@ -93,7 +111,14 @@ 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 == nil && pid > 0 { + return isAlive(pid) + } + // If PID is unreadable, check if the file was modified recently (not stale). + if info, statErr := os.Stat(reclaimedPath); statErr == nil && time.Since(info.ModTime()) <= daemonLockStaleAfter { + return true // fresh, restore rather than steal + } + return false }) } diff --git a/internal/lockutil/reclaim.go b/internal/lockutil/reclaim.go index 6aaa414c4..f7a10b937 100644 --- a/internal/lockutil/reclaim.go +++ b/internal/lockutil/reclaim.go @@ -6,29 +6,10 @@ import ( ) // 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, which requires both the primary no-replace restore and its +// copy fallback to fail; that cannot be provoked portably on a healthy +// filesystem. +var restoreLockFile = RestoreLockFile // ReclaimStaleLock atomically reclaims a suspected-stale lock file. It renames // lockPath aside to ".stale." (only one racer can win the @@ -41,24 +22,21 @@ func restoreLiveLock(reclaimed, path string) error { // 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. +// holder's lock could not be restored (both RestoreLockFile's primary +// no-replace primitive and its copy fallback failed), so lockPath may be +// missing; callers must fail closed instead of re-acquiring. The sidelined +// file is removed on every restore failure or when a new holder wins: once the +// restore has completed or 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. +// When a live lock is restored, RestoreLockFile uses no-replace semantics +// (os.Link on POSIX, MoveFileEx without REPLACE_EXISTING on Windows, or an +// O_EXCL copy fallback). If an unrelated caller's O_EXCL create succeeded at +// lockPath in the window between the rename aside and the restore, the +// restore fails with os.ErrExist rather than silently clobbering that new +// claimant's lock file. ReclaimStaleLock treats this as a lost race (returns +// false, nil) and cleans up the sidelined file, preventing two concurrent +// holders from running simultaneously. func ReclaimStaleLock(lockPath, suffix string, isLive func(reclaimedPath string) bool) (bool, error) { reclaimed := lockPath + ".stale." + suffix if err := os.Rename(lockPath, reclaimed); err != nil { @@ -71,7 +49,7 @@ func ReclaimStaleLock(lockPath, suffix string, isLive func(reclaimedPath string) // 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) { + if !errors.Is(rerr, os.ErrExist) && !errors.Is(rerr, os.ErrNotExist) && !isReclaimContended(rerr) { return false, rerr } } diff --git a/internal/lockutil/reclaim_test.go b/internal/lockutil/reclaim_test.go index a4dd992e3..25cd9b389 100644 --- a/internal/lockutil/reclaim_test.go +++ b/internal/lockutil/reclaim_test.go @@ -2,8 +2,11 @@ package lockutil import ( "errors" + "fmt" "os" "path/filepath" + "sync" + "sync/atomic" "testing" ) @@ -41,13 +44,12 @@ func TestReclaimStaleLock(t *testing.T) { } } -// 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) { +// Restoring a live lock uses no-replace semantics (RestoreLockFile). +// If a new claimant created lockPath in the gap between rename-aside and restore, +// the restore must NOT overwrite the new claimant's lock file; instead the restore +// fails with os.ErrExist, the new claimant's lock is preserved intact, and ReclaimStaleLock +// cleans up the sidelined file and reports a lost race (ok=false, err=nil). +func TestRestoreLiveLockPreservesCompetingHolder(t *testing.T) { dir := t.TempDir() reclaimed := filepath.Join(dir, "lock.stale.tok") path := filepath.Join(dir, "lock") @@ -58,15 +60,16 @@ func TestRestoreLiveLockFastPathOverwritesCompetingFile(t *testing.T) { 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) + err := RestoreLockFile(reclaimed, path) + if !errors.Is(err, os.ErrExist) { + t.Fatalf("RestoreLockFile on existing destination = %v, want os.ErrExist", 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 != nil || string(data) != "new-claimant" { + t.Fatalf("path = %q, err %v; want the new claimant's content preserved intact", data, err) } - if _, err := os.Stat(reclaimed); !errors.Is(err, os.ErrNotExist) { - t.Fatalf("expected reclaimed to be consumed by the rename: %v", err) + if _, err := os.Stat(reclaimed); err != nil { + t.Fatalf("expected reclaimed file to still exist after failed restore: %v", err) } } @@ -76,7 +79,7 @@ func TestReclaimStaleLockFailsClosedOnRestoreError(t *testing.T) { // 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 }() + defer func() { restoreLockFile = RestoreLockFile }() lockPath := filepath.Join(t.TempDir(), "lock") if err := os.WriteFile(lockPath, []byte("live-holder"), 0o600); err != nil { @@ -96,7 +99,7 @@ func TestReclaimStaleLockDropsSidelinedWhenNewHolderWins(t *testing.T) { // 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 }() + defer func() { restoreLockFile = RestoreLockFile }() lockPath := filepath.Join(t.TempDir(), "lock") if err := os.WriteFile(lockPath, []byte("live-holder"), 0o600); err != nil { @@ -110,3 +113,164 @@ func TestReclaimStaleLockDropsSidelinedWhenNewHolderWins(t *testing.T) { t.Fatalf("the sidelined file must be dropped when a new holder wins: %v", matches) } } + +func TestReclaimStaleLockBenignRestoreLostRace(t *testing.T) { + // An os.ErrNotExist restore failure means a competing racer already moved + // or restored the file; that is a benign lost race and should return false, nil. + restoreLockFile = func(reclaimed, path string) error { return os.ErrNotExist } + defer func() { restoreLockFile = RestoreLockFile }() + + 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 race during restore is not an error (ok=%v err=%v)", ok, err) + } +} + +func TestReclaimStaleLockConcurrentDeadReclaim(t *testing.T) { + const goroutines = 24 + dir := t.TempDir() + lockPath := filepath.Join(dir, "lock") + + if err := os.WriteFile(lockPath, []byte("crashed-process"), 0o600); err != nil { + t.Fatal(err) + } + + var wg sync.WaitGroup + var reclaimWins atomic.Int64 + var errCount atomic.Int64 + + start := make(chan struct{}) + for i := 0; i < goroutines; i++ { + wg.Add(1) + go func(id int) { + defer wg.Done() + <-start + ok, err := ReclaimStaleLock(lockPath, fmt.Sprintf("tok-%d", id), func(string) bool { + return false // dead + }) + if err != nil { + errCount.Add(1) + return + } + if ok { + reclaimWins.Add(1) + } + }(i) + } + + close(start) + wg.Wait() + + if errs := errCount.Load(); errs != 0 { + t.Fatalf("unexpected errors during concurrent dead reclaim: %d", errs) + } + if wins := reclaimWins.Load(); wins != 1 { + t.Fatalf("expected exactly 1 winner for dead lock reclaim, got %d", wins) + } + if _, err := os.Stat(lockPath); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("expected lock file to be removed, stat = %v", err) + } + if matches, _ := filepath.Glob(lockPath + ".stale.*"); len(matches) != 0 { + t.Fatalf("leaked sidelined files: %v", matches) + } +} + +func TestReclaimStaleLockConcurrentLiveRestoration(t *testing.T) { + const goroutines = 24 + dir := t.TempDir() + lockPath := filepath.Join(dir, "lock") + + if err := os.WriteFile(lockPath, []byte("live-holder-token"), 0o600); err != nil { + t.Fatal(err) + } + + var wg sync.WaitGroup + var reclaimWins atomic.Int64 + var errCount atomic.Int64 + + start := make(chan struct{}) + for i := 0; i < goroutines; i++ { + wg.Add(1) + go func(id int) { + defer wg.Done() + <-start + ok, err := ReclaimStaleLock(lockPath, fmt.Sprintf("tok-%d", id), func(string) bool { + return true // live + }) + if err != nil { + errCount.Add(1) + return + } + if ok { + reclaimWins.Add(1) + } + }(i) + } + + close(start) + wg.Wait() + + if errs := errCount.Load(); errs != 0 { + t.Fatalf("unexpected errors during concurrent live restore: %d", errs) + } + if wins := reclaimWins.Load(); wins != 0 { + t.Fatalf("expected 0 winners for live lock reclaim, got %d", wins) + } + data, err := os.ReadFile(lockPath) + if err != nil || string(data) != "live-holder-token" { + t.Fatalf("expected live lock file intact at %q, got %q (err %v)", lockPath, data, err) + } + if matches, _ := filepath.Glob(lockPath + ".stale.*"); len(matches) != 0 { + t.Fatalf("leaked sidelined files: %v", matches) + } +} + +func TestReclaimStaleLockRaceWithNewClaimant(t *testing.T) { + // Exercise the exact race condition: + // A live lock is sidelined for inspection. + // Before restore completes, an O_EXCL claimant creates lockPath. + // The restore must NOT overwrite the new claimant. + dir := t.TempDir() + lockPath := filepath.Join(dir, "lock") + + if err := os.WriteFile(lockPath, []byte("original-live-holder"), 0o600); err != nil { + t.Fatal(err) + } + + ok, err := ReclaimStaleLock(lockPath, "race-tok", func(reclaimedPath string) bool { + // Inside isLive callback (file is currently sidelined to reclaimedPath): + // An unrelated process creates a new lock at lockPath via O_EXCL. + f, err := os.OpenFile(lockPath, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o600) + if err != nil { + t.Errorf("new claimant OpenFile failed: %v", err) + return false + } + if _, err := f.WriteString("new-concurrent-claimant"); err != nil { + t.Errorf("new claimant WriteString failed: %v", err) + } + _ = f.Close() + return true // original holder was also live + }) + + if err != nil { + t.Fatalf("ReclaimStaleLock should handle ErrExist cleanly, got err: %v", err) + } + if ok { + t.Fatalf("ReclaimStaleLock should return ok=false when live holder lost restore race, got ok=true") + } + + // Verify the new claimant's file was NOT overwritten + data, err := os.ReadFile(lockPath) + if err != nil || string(data) != "new-concurrent-claimant" { + t.Fatalf("lock file was overwritten or corrupted: got %q (err %v), want %q", data, err, "new-concurrent-claimant") + } + + // Verify the sidelined file was cleaned up and not leaked + if matches, _ := filepath.Glob(lockPath + ".stale.*"); len(matches) != 0 { + t.Fatalf("sidelined file leaked after race: %v", matches) + } +}