fix(lockutil): non-destructive live-lock restoration under O_CREATE|O_EXCL

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
 	})
 }
 

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 "<lockPath>.stale.<suffix>" (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
 			}
 		}

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)
+	}
+}