amadan

carlos / platform Public

host: the held-stage lock's open race times out as busy, and its retries pause

Fix round 2 (rereview).

P2-2: lockNamed checks the deadline at the top of every pass, so an open
that meets a holder's removal after the wait has passed ends in the
caller's busy error, never the open's own. TestHeldStageLockOpenRaceAfter
TheWaitIsBusy drives every open into that race through a new seam
(lockNamedOpenAt).

Replacement spin: a retry after a removal (in the open, or a name that
names another file after the flock) pauses lockNamedRetryPause (10 ms),
capped at what is left of the wait. TestHeldStageLockRetriesEndWithTheWait
now bounds the retries a wait allows.

P2-1, no change by ruling: a flock taken just after the deadline is still
a valid, exclusive lock and is kept. The deadline bounds the wait;
correctness does not depend on it. The comments now say so in place of
the round-1 claim that a late attempt never succeeds.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude 1395e02caddf3ba193b9f0d2790dd6eef850c1f4 parent 91b9f0d
2 files changed, +78 −15
  • internal/host/movesfloor_linux.go +40 −15
  • internal/host/movesheldlock_cleanup_test.go +38 −0
diff --git a/internal/host/movesfloor_linux.go b/internal/host/movesfloor_linux.go
index dbccf7a4b..29a88713d 100644
--- a/internal/host/movesfloor_linux.go
+++ b/internal/host/movesfloor_linux.go
@@ -109,24 +109,34 @@ func (d *movesDir) Lock() error { return d.lockNamed(floorLockFile, floorLockWai
// again on whatever is there now. The same removal can land inside the
// open itself (the name vanishes, or the file opened has no link left);
// that is retried too, while a symlink, a FIFO or a second link at the name
-// is still refused. Every retry is within the one wait: past it, the
-// call fails with busy and holds nothing.
+// is still refused. Each retry pauses (lockNamedRetryPause, capped at what
+// is left of the wait), and none starts once the wait has passed: the call
+// fails with busy then, an open race included, and holds nothing.
+//
+// The deadline bounds the wait; correctness does not depend on it. A flock
+// taken just after the deadline (the check and the syscall are not one
+// step) is still a valid, exclusive lock, and is kept.
func (d *movesDir) lockNamed(name string, wait time.Duration, busy error) error {
deadline := time.Now().Add(wait)
+ timedOut := func() error { return fmt.Errorf("lock %s: %w", filepath.Join(d.path, name), busy) }
for {
- f, err := safefile.OpenFileAt(d.fd, name, os.O_RDONLY|os.O_CREATE, 0o644)
+ if time.Now().After(deadline) {
+ return timedOut()
+ }
+ f, err := lockNamedOpenAt(d.fd, name, os.O_RDONLY|os.O_CREATE, 0o644)
if err != nil {
- if time.Now().Before(deadline) && d.removedDuringOpen(name, err) {
- time.Sleep(floorLockPoll)
- continue
+ if !d.removedDuringOpen(name, err) {
+ return fmt.Errorf("open %s: %w", filepath.Join(d.path, name), err)
}
- return fmt.Errorf("open %s: %w", filepath.Join(d.path, name), err)
+ // A holder's removal landed inside the open: start again.
+ lockNamedPause(deadline)
+ continue
}
lockNamedOpened(name)
if err := flockWithin(f, deadline); err != nil {
f.Close()
if errors.Is(err, syscall.EWOULDBLOCK) {
- err = busy
+ return timedOut()
}
return fmt.Errorf("lock %s: %w", filepath.Join(d.path, name), err)
}
@@ -140,18 +150,25 @@ func (d *movesDir) lockNamed(name string, wait time.Duration, busy error) error
return nil
}
// The holder before us removed this file as it released it. Start
- // again only within the wait: the publication waits inside the
+ // again, within the wait: the publication waits inside the
// registry's freeze, so a name that keeps changing ends in busy.
f.Close()
- if time.Now().After(deadline) {
- return fmt.Errorf("lock %s: %w", filepath.Join(d.path, name), busy)
- }
+ lockNamedPause(deadline)
}
}
-// flockWithin takes an exclusive flock on f, polling until deadline. Once
-// the deadline has passed it takes nothing and returns EWOULDBLOCK, so a
-// retry that reaches it late never succeeds past the wait.
+// lockNamedPause sleeps lockNamedRetryPause, or what is left of the wait
+// when that is less.
+func lockNamedPause(deadline time.Time) {
+ if left := time.Until(deadline); left > 0 {
+ time.Sleep(min(lockNamedRetryPause, left))
+ }
+}
+
+// flockWithin takes an exclusive flock on f, polling until deadline, and
+// once the deadline has passed stops trying and returns EWOULDBLOCK. A
+// flock that lands just after the deadline is kept: it is a valid,
+// exclusive lock, and the deadline only bounds the wait.
func flockWithin(f *os.File, deadline time.Time) error {
for {
if time.Now().After(deadline) {
@@ -229,6 +246,14 @@ func (d *movesDir) unlockRemoving() error {
return nil
}
+// lockNamedRetryPause is the pause between lockNamed's retries after a
+// holder's removal, capped at what is left of the wait.
+const lockNamedRetryPause = 10 * time.Millisecond
+
+// lockNamedOpenAt opens the lock file for lockNamed. A test seam: a test
+// can make every open meet the race with a holder's removal.
+var lockNamedOpenAt = safefile.OpenFileAt
+
// lockNamedOpened runs in lockNamed after each open of the lock file and
// before its flock. A test seam: the window in which the file a waiter has
// open can be removed by the holder before it and replaced by a new one.
diff --git a/internal/host/movesheldlock_cleanup_test.go b/internal/host/movesheldlock_cleanup_test.go
index ca4be04a1..1ab34302d 100644
--- a/internal/host/movesheldlock_cleanup_test.go
+++ b/internal/host/movesheldlock_cleanup_test.go
@@ -350,4 +350,42 @@ func TestHeldStageLockRetriesEndWithTheWait(t *testing.T) {
if replaced.Load() < 2 {
t.Fatalf("only %d files were replaced; the re-check was not exercised", replaced.Load())
}
+ // Fix round 2: replacement retries pause (lockNamedRetryPause, capped
+ // at what is left of the wait), so they cannot spin. A pause is a floor
+ // on each retry's time, so this bound holds however loaded the box.
+ if most := int32(wait/lockNamedRetryPause) + 2; replaced.Load() > most {
+ t.Fatalf("%d replacement retries in a %v wait, want at most %d", replaced.Load(), wait, most)
+ }
+}
+
+// Fix round 2: an open that meets a holder's removal after the wait has
+// passed ends in the caller's busy error, like every other way the wait
+// runs out, never the open's own error. Every open here meets that race:
+// it fails as an unlinked file does and the name is gone.
+func TestHeldStageLockOpenRaceAfterTheWaitIsBusy(t *testing.T) {
+ prevWait, prevOpen := heldLockWait, lockNamedOpenAt
+ t.Cleanup(func() { heldLockWait, lockNamedOpenAt = prevWait, prevOpen })
+ const wait = 150 * time.Millisecond
+ heldLockWait = wait
+ var opens atomic.Int32
+ lockNamedOpenAt = func(dir int, name string, flags int, mode os.FileMode) (*os.File, error) {
+ opens.Add(1)
+ return nil, &os.PathError{Op: "open", Path: name, Err: errors.New("not a regular file with one link")}
+ }
+ ran := false
+ start := time.Now()
+ err := withHeldStageLock(t.TempDir(), hsHost, func() error { ran = true; return nil })
+ elapsed := time.Since(start)
+ if !errors.Is(err, errHeldLockBusy) {
+ t.Fatalf("every open raced a removal until past the wait: %v, want errHeldLockBusy", err)
+ }
+ if ran {
+ t.Fatal("fn ran without the lock")
+ }
+ if elapsed < wait || elapsed > wait+time.Second {
+ t.Fatalf("gave up after %v, want about %v", elapsed, wait)
+ }
+ if opens.Load() < 2 {
+ t.Fatalf("only %d opens; the race was not retried", opens.Load())
+ }
}