amadan

carlos / platform Public

host: the held-stage lock's re-check retries end with the wait

Fix round 1 (review: unbounded retry). When the name named another file
after the flock, lockNamed closed it and started again with no deadline
check, and flockWithin returned success whenever the flock was free, even
past the deadline. A name replaced on every open kept a waiter retrying
forever, and the publication waits inside the registry's freeze.

A stale-inode retry now checks the deadline first and fails with the
caller's busy error once it has passed; flockWithin takes nothing past
the deadline. TestHeldStageLockRetriesEndWithTheWait replaces every file
the waiter opens (through the lockNamedOpened seam) and asserts busy
within the wait with fn never run.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude 91b9f0d3631718ade6c95ca6489855213648f1d4 parent b871072
2 files changed, +74 −5
  • internal/host/movesfloor_linux.go +15 −5
  • internal/host/movesheldlock_cleanup_test.go +59 −0
diff --git a/internal/host/movesfloor_linux.go b/internal/host/movesfloor_linux.go
index 5a0d583d3..dbccf7a4b 100644
--- a/internal/host/movesfloor_linux.go
+++ b/internal/host/movesfloor_linux.go
@@ -109,7 +109,8 @@ 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.
+// is still refused. Every retry is within the one wait: past it, the
+// call fails with busy and holds nothing.
func (d *movesDir) lockNamed(name string, wait time.Duration, busy error) error {
deadline := time.Now().Add(wait)
for {
@@ -138,17 +139,26 @@ func (d *movesDir) lockNamed(name string, wait time.Duration, busy error) error
d.lock, d.lockName = f, name
return nil
}
- // The holder before us removed this file as it released it.
+ // The holder before us removed this file as it released it. Start
+ // again only 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)
+ }
}
}
-// flockWithin takes an exclusive flock on f, polling until deadline. Still
-// held then is EWOULDBLOCK.
+// 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.
func flockWithin(f *os.File, deadline time.Time) error {
for {
+ if time.Now().After(deadline) {
+ return syscall.EWOULDBLOCK
+ }
err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX|syscall.LOCK_NB)
- if err == nil || !errors.Is(err, syscall.EWOULDBLOCK) || time.Now().After(deadline) {
+ if err == nil || !errors.Is(err, syscall.EWOULDBLOCK) {
return err
}
time.Sleep(floorLockPoll)
diff --git a/internal/host/movesheldlock_cleanup_test.go b/internal/host/movesheldlock_cleanup_test.go
index d8bd1cb5d..ca4be04a1 100644
--- a/internal/host/movesheldlock_cleanup_test.go
+++ b/internal/host/movesheldlock_cleanup_test.go
@@ -292,3 +292,62 @@ func TestHeldStageLockOpenRetriesOnlyTheRemovalRace(t *testing.T) {
t.Error("a removed .moves was retried")
}
}
+
+// Fix round 1: the re-check's retries are bounded by the one wait. With
+// every file the waiter opens replaced at the name before its flock, each
+// re-check finds another file there; the call still ends with the busy
+// error once the wait has passed, and never runs fn. Publication waits
+// inside the registry's freeze, so the wait must hold.
+func TestHeldStageLockRetriesEndWithTheWait(t *testing.T) {
+ prevWait, prevHook := heldLockWait, lockNamedOpened
+ const wait = 200 * time.Millisecond
+ heldLockWait = wait
+ data := t.TempDir()
+ path := filepath.Join(data, ".moves", heldStageLockFile(hsHost))
+ var stop atomic.Bool
+ var replaced atomic.Int32
+ lockNamedOpened = func(name string) {
+ if name != heldStageLockFile(hsHost) || stop.Load() {
+ return
+ }
+ if err := os.Remove(path); err != nil {
+ return
+ }
+ if err := os.WriteFile(path, nil, 0o644); err == nil {
+ replaced.Add(1)
+ }
+ }
+ var ran atomic.Bool
+ done := make(chan error, 1)
+ start := time.Now()
+ go func() { done <- withHeldStageLock(data, hsHost, func() error { ran.Store(true); return nil }) }()
+ t.Cleanup(func() {
+ // A waiter still retrying is let through before the seam goes.
+ stop.Store(true)
+ select {
+ case <-done:
+ case <-time.After(10 * time.Second):
+ }
+ heldLockWait, lockNamedOpened = prevWait, prevHook
+ })
+ var err error
+ select {
+ case err = <-done:
+ done <- err
+ case <-time.After(5 * time.Second):
+ t.Fatalf("still retrying %v after a %v wait (%d files replaced, ran %v)", time.Since(start), wait, replaced.Load(), ran.Load())
+ }
+ elapsed := time.Since(start)
+ if !errors.Is(err, errHeldLockBusy) {
+ t.Fatalf("with every opened file replaced: %v, want errHeldLockBusy", err)
+ }
+ if ran.Load() {
+ t.Fatal("fn ran without the lock")
+ }
+ if elapsed > wait+2*time.Second {
+ t.Fatalf("gave up after %v, want about %v", elapsed, wait)
+ }
+ if replaced.Load() < 2 {
+ t.Fatalf("only %d files were replaced; the re-check was not exercised", replaced.Load())
+ }
+}