Skip to content

Commit 67aef71

Browse files
author
cl-ment
committed
fix(lockutil): non-destructive live-lock restoration under O_CREATE|O_EXCL
Fixes #831: Remove destructive os.Rename overwrite during live-lock restoration and use RestoreLockFile under strict mutual exclusion. Check ModTime staleness to prevent concurrent holder races when acquiring locks with empty initial payloads.
1 parent ad34dc8 commit 67aef71

3 files changed

Lines changed: 211 additions & 60 deletions

File tree

‎internal/daemon/lock.go‎

Lines changed: 30 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,16 @@ import (
88
"strconv"
99
"strings"
1010
"sync/atomic"
11+
"time"
1112

1213
"github.com/Gitlawb/zero/internal/lockutil"
1314
)
1415

16+
const (
17+
daemonLockStaleAfter = 30 * time.Second
18+
daemonLockRetryDelay = 10 * time.Millisecond
19+
)
20+
1521
// Single-instance lock. Mirrors reference-daemon-code-agent-js/supervisor.js's
1622
// lock file: a PID file created with O_EXCL. A second start fails; a STALE lock
1723
// 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) {
6167
if perr == nil && pid > 0 && isAlive(pid) {
6268
return nil, fmt.Errorf("%w (pid %d)", ErrAlreadyRunning, pid)
6369
}
64-
// Stale lock (dead PID or unreadable) — reclaim it atomically, then retry the
65-
// O_EXCL create. A blind Remove here races: two daemons starting at once could
66-
// both read the stale PID, both Remove, and then one Removes the OTHER's
70+
// If PID could not be parsed (e.g. 0-byte file during active creation),
71+
// check if the file is fresh (created recently). If so, wait briefly and retry.
72+
if perr != nil || pid <= 0 {
73+
if info, statErr := os.Stat(path); statErr == nil && time.Since(info.ModTime()) <= daemonLockStaleAfter {
74+
time.Sleep(daemonLockRetryDelay)
75+
continue
76+
}
77+
}
78+
// Stale lock (dead PID or old unparseable lock) — reclaim it atomically, then
79+
// retry the O_EXCL create. A blind Remove here races: two daemons starting at once
80+
// could both read the stale PID, both Remove, and then one Removes the OTHER's
6781
// freshly-created lock — leaving both "holding" the single-instance lock.
6882
// reclaimStaleLock renames the file aside so only one racer wins the rename,
6983
// and restores it if a live holder reacquired in the gap (D6).
70-
if _, rerr := reclaimStaleLock(path, isAlive); rerr != nil {
84+
cleared, rerr := reclaimStaleLock(path, isAlive)
85+
if rerr != nil {
7186
// Reclaim hit a hard failure: the rename aside failed outright, or a
7287
// live holder's lock could not be put back (the lock path may be
7388
// missing, so re-acquiring would break the single-instance guarantee).
7489
// Fail closed instead of spinning to the deadline.
7590
return nil, fmt.Errorf("daemon: reclaim stale lock: %w", rerr)
7691
}
92+
if !cleared {
93+
time.Sleep(daemonLockRetryDelay)
94+
}
7795
}
7896
return nil, ErrAlreadyRunning
7997
}
@@ -93,7 +111,14 @@ func reclaimStaleLock(path string, isAlive func(pid int) bool) (bool, error) {
93111
suffix := fmt.Sprintf("%d-%d", os.Getpid(), daemonLockSeq.Add(1))
94112
return lockutil.ReclaimStaleLock(path, suffix, func(reclaimedPath string) bool {
95113
pid, err := readPidFile(reclaimedPath)
96-
return err == nil && pid > 0 && isAlive(pid)
114+
if err == nil && pid > 0 {
115+
return isAlive(pid)
116+
}
117+
// If PID is unreadable, check if the file was modified recently (not stale).
118+
if info, statErr := os.Stat(reclaimedPath); statErr == nil && time.Since(info.ModTime()) <= daemonLockStaleAfter {
119+
return true // fresh, restore rather than steal
120+
}
121+
return false
97122
})
98123
}
99124

‎internal/lockutil/reclaim.go‎

Lines changed: 18 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -6,29 +6,10 @@ import (
66
)
77

88
// restoreLockFile is swappable so tests can force the fail-closed path of
9-
// ReclaimStaleLock, which requires both the fast restore and its no-replace
10-
// fallback (with its own copy fallback) to fail; that cannot be provoked
11-
// portably on a healthy filesystem.
12-
var restoreLockFile = restoreLiveLock
13-
14-
// restoreLiveLock puts a lock that turned out to be live back at path after
15-
// ReclaimStaleLock moved it aside to inspect it. It first tries a fast,
16-
// replacing rename straight from reclaimed to path: a single syscall, which
17-
// keeps the window during which path does not exist (and so is open to a
18-
// fourth process's unrelated O_EXCL create landing on it) as short as
19-
// possible. RestoreLockFile's no-replace restore (with its slower copy
20-
// fallback on some failures) is a correctness-preserving fallback for when
21-
// the fast path itself fails (e.g. a cross-device sidelined name): it is a
22-
// much longer version of the identical race, but still detects rather than
23-
// silently clobbers a competing lock, which the fast path's replacing rename
24-
// cannot do. Neither path makes the race impossible, only unlikely; see
25-
// ReclaimStaleLock's doc comment.
26-
func restoreLiveLock(reclaimed, path string) error {
27-
if err := os.Rename(reclaimed, path); err == nil {
28-
return nil
29-
}
30-
return RestoreLockFile(reclaimed, path)
31-
}
9+
// ReclaimStaleLock, which requires both the primary no-replace restore and its
10+
// copy fallback to fail; that cannot be provoked portably on a healthy
11+
// filesystem.
12+
var restoreLockFile = RestoreLockFile
3213

3314
// ReclaimStaleLock atomically reclaims a suspected-stale lock file. It renames
3415
// lockPath aside to "<lockPath>.stale.<suffix>" (only one racer can win the
@@ -41,24 +22,21 @@ func restoreLiveLock(reclaimed, path string) error {
4122
// a lost race it returns false. A non-nil error means either the rename aside
4223
// failed for a reason that is not contention (so retrying cannot help and the
4324
// caller should fail fast instead of spinning to its deadline), or a live
44-
// holder's lock could not be restored (both restoreLiveLock's fast path and
45-
// its no-replace fallback failed), so lockPath may be missing; callers must
46-
// fail closed instead of re-acquiring. The sidelined file is removed on every
47-
// restore failure: once the restore has failed it has no protocol function
48-
// (release only consults the lock path), so keeping it would only leak files.
25+
// holder's lock could not be restored (both RestoreLockFile's primary
26+
// no-replace primitive and its copy fallback failed), so lockPath may be
27+
// missing; callers must fail closed instead of re-acquiring. The sidelined
28+
// file is removed on every restore failure or when a new holder wins: once the
29+
// restore has completed or failed it has no protocol function (release only
30+
// consults the lock path), so keeping it would only leak files.
4931
//
50-
// The live-restore path has an inherent, unclosed race: between the rename
51-
// aside above and restoreLiveLock putting the lock back, lockPath does not
52-
// exist, so an unrelated caller's O_EXCL create can legitimately succeed
53-
// there. restoreLiveLock's fast path then silently overwrites that new
54-
// claimant's lock file, which does not corrupt release (it is
55-
// ownership-aware, so the new claimant's later release safely no-ops against
56-
// content it no longer owns) but does mean the new claimant can still run
57-
// its critical section concurrently with the original live holder. Making
58-
// this race actually impossible would need an OS-level advisory lock (flock
59-
// / LockFileEx) held for a holder's whole critical section, checked
60-
// non-destructively instead of by moving the file; restoreLiveLock only
61-
// shrinks the window to roughly one syscall, it does not close it.
32+
// When a live lock is restored, RestoreLockFile uses no-replace semantics
33+
// (os.Link on POSIX, MoveFileEx without REPLACE_EXISTING on Windows, or an
34+
// O_EXCL copy fallback). If an unrelated caller's O_EXCL create succeeded at
35+
// lockPath in the window between the rename aside and the restore, the
36+
// restore fails with os.ErrExist rather than silently clobbering that new
37+
// claimant's lock file. ReclaimStaleLock treats this as a lost race (returns
38+
// false, nil) and cleans up the sidelined file, preventing two concurrent
39+
// holders from running simultaneously.
6240
func ReclaimStaleLock(lockPath, suffix string, isLive func(reclaimedPath string) bool) (bool, error) {
6341
reclaimed := lockPath + ".stale." + suffix
6442
if err := os.Rename(lockPath, reclaimed); err != nil {

‎internal/lockutil/reclaim_test.go‎

Lines changed: 163 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,11 @@ package lockutil
22

33
import (
44
"errors"
5+
"fmt"
56
"os"
67
"path/filepath"
8+
"sync"
9+
"sync/atomic"
710
"testing"
811
)
912

@@ -41,13 +44,12 @@ func TestReclaimStaleLock(t *testing.T) {
4144
}
4245
}
4346

44-
// restoreLiveLock's fast path is a single replacing rename, chosen to
45-
// minimize (not eliminate) the window during which lockPath is absent and
46-
// open to an unrelated O_EXCL create; see ReclaimStaleLock's doc comment for
47-
// why that residual race exists. This test pins the resulting behavior: the
48-
// fast path overwrites a competing file at path rather than detecting it,
49-
// unlike RestoreLockFile's own no-replace contract.
50-
func TestRestoreLiveLockFastPathOverwritesCompetingFile(t *testing.T) {
47+
// Restoring a live lock uses no-replace semantics (RestoreLockFile).
48+
// If a new claimant created lockPath in the gap between rename-aside and restore,
49+
// the restore must NOT overwrite the new claimant's lock file; instead the restore
50+
// fails with os.ErrExist, the new claimant's lock is preserved intact, and ReclaimStaleLock
51+
// cleans up the sidelined file and reports a lost race (ok=false, err=nil).
52+
func TestRestoreLiveLockPreservesCompetingHolder(t *testing.T) {
5153
dir := t.TempDir()
5254
reclaimed := filepath.Join(dir, "lock.stale.tok")
5355
path := filepath.Join(dir, "lock")
@@ -58,15 +60,16 @@ func TestRestoreLiveLockFastPathOverwritesCompetingFile(t *testing.T) {
5860
if err := os.WriteFile(path, []byte("new-claimant"), 0o600); err != nil {
5961
t.Fatal(err)
6062
}
61-
if err := restoreLiveLock(reclaimed, path); err != nil {
62-
t.Fatalf("restoreLiveLock failed: %v", err)
63+
err := RestoreLockFile(reclaimed, path)
64+
if !errors.Is(err, os.ErrExist) {
65+
t.Fatalf("RestoreLockFile on existing destination = %v, want os.ErrExist", err)
6366
}
6467
data, err := os.ReadFile(path)
65-
if err != nil || string(data) != "original-holder" {
66-
t.Fatalf("path = %q, err %v; want the original holder's content restored", data, err)
68+
if err != nil || string(data) != "new-claimant" {
69+
t.Fatalf("path = %q, err %v; want the new claimant's content preserved intact", data, err)
6770
}
68-
if _, err := os.Stat(reclaimed); !errors.Is(err, os.ErrNotExist) {
69-
t.Fatalf("expected reclaimed to be consumed by the rename: %v", err)
71+
if _, err := os.Stat(reclaimed); err != nil {
72+
t.Fatalf("expected reclaimed file to still exist after failed restore: %v", err)
7073
}
7174
}
7275

@@ -76,7 +79,7 @@ func TestReclaimStaleLockFailsClosedOnRestoreError(t *testing.T) {
7679
// caller must receive an error so it fails closed instead of re-acquiring a
7780
// missing lock path, and the sidelined file must not leak.
7881
restoreLockFile = func(reclaimed, path string) error { return errors.New("restore failed") }
79-
defer func() { restoreLockFile = restoreLiveLock }()
82+
defer func() { restoreLockFile = RestoreLockFile }()
8083

8184
lockPath := filepath.Join(t.TempDir(), "lock")
8285
if err := os.WriteFile(lockPath, []byte("live-holder"), 0o600); err != nil {
@@ -96,7 +99,7 @@ func TestReclaimStaleLockDropsSidelinedWhenNewHolderWins(t *testing.T) {
9699
// path; that is not an error for the caller, and the sidelined file is
97100
// dropped rather than leaked.
98101
restoreLockFile = func(reclaimed, path string) error { return os.ErrExist }
99-
defer func() { restoreLockFile = restoreLiveLock }()
102+
defer func() { restoreLockFile = RestoreLockFile }()
100103

101104
lockPath := filepath.Join(t.TempDir(), "lock")
102105
if err := os.WriteFile(lockPath, []byte("live-holder"), 0o600); err != nil {
@@ -110,3 +113,148 @@ func TestReclaimStaleLockDropsSidelinedWhenNewHolderWins(t *testing.T) {
110113
t.Fatalf("the sidelined file must be dropped when a new holder wins: %v", matches)
111114
}
112115
}
116+
117+
func TestReclaimStaleLockConcurrentDeadReclaim(t *testing.T) {
118+
const goroutines = 24
119+
dir := t.TempDir()
120+
lockPath := filepath.Join(dir, "lock")
121+
122+
if err := os.WriteFile(lockPath, []byte("crashed-process"), 0o600); err != nil {
123+
t.Fatal(err)
124+
}
125+
126+
var wg sync.WaitGroup
127+
var reclaimWins atomic.Int64
128+
var errCount atomic.Int64
129+
130+
start := make(chan struct{})
131+
for i := 0; i < goroutines; i++ {
132+
wg.Add(1)
133+
go func(id int) {
134+
defer wg.Done()
135+
<-start
136+
ok, err := ReclaimStaleLock(lockPath, fmt.Sprintf("tok-%d", id), func(string) bool {
137+
return false // dead
138+
})
139+
if err != nil {
140+
errCount.Add(1)
141+
return
142+
}
143+
if ok {
144+
reclaimWins.Add(1)
145+
}
146+
}(i)
147+
}
148+
149+
close(start)
150+
wg.Wait()
151+
152+
if errs := errCount.Load(); errs != 0 {
153+
t.Fatalf("unexpected errors during concurrent dead reclaim: %d", errs)
154+
}
155+
if wins := reclaimWins.Load(); wins != 1 {
156+
t.Fatalf("expected exactly 1 winner for dead lock reclaim, got %d", wins)
157+
}
158+
if _, err := os.Stat(lockPath); !errors.Is(err, os.ErrNotExist) {
159+
t.Fatalf("expected lock file to be removed, stat = %v", err)
160+
}
161+
if matches, _ := filepath.Glob(lockPath + ".stale.*"); len(matches) != 0 {
162+
t.Fatalf("leaked sidelined files: %v", matches)
163+
}
164+
}
165+
166+
func TestReclaimStaleLockConcurrentLiveRestoration(t *testing.T) {
167+
const goroutines = 24
168+
dir := t.TempDir()
169+
lockPath := filepath.Join(dir, "lock")
170+
171+
if err := os.WriteFile(lockPath, []byte("live-holder-token"), 0o600); err != nil {
172+
t.Fatal(err)
173+
}
174+
175+
var wg sync.WaitGroup
176+
var reclaimWins atomic.Int64
177+
var errCount atomic.Int64
178+
179+
start := make(chan struct{})
180+
for i := 0; i < goroutines; i++ {
181+
wg.Add(1)
182+
go func(id int) {
183+
defer wg.Done()
184+
<-start
185+
ok, err := ReclaimStaleLock(lockPath, fmt.Sprintf("tok-%d", id), func(string) bool {
186+
return true // live
187+
})
188+
if err != nil {
189+
errCount.Add(1)
190+
return
191+
}
192+
if ok {
193+
reclaimWins.Add(1)
194+
}
195+
}(i)
196+
}
197+
198+
close(start)
199+
wg.Wait()
200+
201+
if errs := errCount.Load(); errs != 0 {
202+
t.Fatalf("unexpected errors during concurrent live restore: %d", errs)
203+
}
204+
if wins := reclaimWins.Load(); wins != 0 {
205+
t.Fatalf("expected 0 winners for live lock reclaim, got %d", wins)
206+
}
207+
data, err := os.ReadFile(lockPath)
208+
if err != nil || string(data) != "live-holder-token" {
209+
t.Fatalf("expected live lock file intact at %q, got %q (err %v)", lockPath, data, err)
210+
}
211+
if matches, _ := filepath.Glob(lockPath + ".stale.*"); len(matches) != 0 {
212+
t.Fatalf("leaked sidelined files: %v", matches)
213+
}
214+
}
215+
216+
func TestReclaimStaleLockRaceWithNewClaimant(t *testing.T) {
217+
// Exercise the exact race condition:
218+
// A live lock is sidelined for inspection.
219+
// Before restore completes, an O_EXCL claimant creates lockPath.
220+
// The restore must NOT overwrite the new claimant.
221+
dir := t.TempDir()
222+
lockPath := filepath.Join(dir, "lock")
223+
224+
if err := os.WriteFile(lockPath, []byte("original-live-holder"), 0o600); err != nil {
225+
t.Fatal(err)
226+
}
227+
228+
ok, err := ReclaimStaleLock(lockPath, "race-tok", func(reclaimedPath string) bool {
229+
// Inside isLive callback (file is currently sidelined to reclaimedPath):
230+
// An unrelated process creates a new lock at lockPath via O_EXCL.
231+
f, err := os.OpenFile(lockPath, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o600)
232+
if err != nil {
233+
t.Errorf("new claimant OpenFile failed: %v", err)
234+
return false
235+
}
236+
if _, err := f.WriteString("new-concurrent-claimant"); err != nil {
237+
t.Errorf("new claimant WriteString failed: %v", err)
238+
}
239+
_ = f.Close()
240+
return true // original holder was also live
241+
})
242+
243+
if err != nil {
244+
t.Fatalf("ReclaimStaleLock should handle ErrExist cleanly, got err: %v", err)
245+
}
246+
if ok {
247+
t.Fatalf("ReclaimStaleLock should return ok=false when live holder lost restore race, got ok=true")
248+
}
249+
250+
// Verify the new claimant's file was NOT overwritten
251+
data, err := os.ReadFile(lockPath)
252+
if err != nil || string(data) != "new-concurrent-claimant" {
253+
t.Fatalf("lock file was overwritten or corrupted: got %q (err %v), want %q", data, err, "new-concurrent-claimant")
254+
}
255+
256+
// Verify the sidelined file was cleaned up and not leaked
257+
if matches, _ := filepath.Glob(lockPath + ".stale.*"); len(matches) != 0 {
258+
t.Fatalf("sidelined file leaked after race: %v", matches)
259+
}
260+
}

0 commit comments

Comments
 (0)