amadan

carlos / platform Public

lease: Reassert reports a lost tombstone, never takes a new one

Reassert fell back to Tombstone when the lock was absent or expired.
Between a caller's LockState and Reassert's own read, a tombstone that
was deleted, or taken, written and released by a wake, was then created
again, and the lost stop went unseen (T4 review I1).

Absent or expired, whoever wrote it, is now ErrTombstoneLost and nothing
is written. The test that expected the fallback is inverted: absent, its
own expired or lifted tombstone, someone else's and an ordinary expired
lease all return ErrTombstoneLost with the lock byte for byte unchanged.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude eceb0df1febbe2ab9ee5bc9697dcaa9c5f9763b4 parent d301713
2 files changed, +44 −30
  • internal/lease/tombstone.go +15 −8
  • internal/lease/tombstone_test.go +29 −22
diff --git a/internal/lease/tombstone.go b/internal/lease/tombstone.go
index c0054208f..7f9969a21 100644
--- a/internal/lease/tombstone.go
+++ b/internal/lease/tombstone.go
@@ -60,6 +60,9 @@ var (
// between the read and the conditional write. Nothing was overwritten;
// the caller's next pass reads it afresh.
ErrTombstoneChanged = errors.New("lease: the lock changed under the tombstone write")
+ // ErrTombstoneLost reports that the tombstone Reassert was asked to
+ // keep is absent or expired: the stop was lost. Nothing was written.
+ ErrTombstoneLost = errors.New("lease: the tombstone is absent or expired; the stop was lost")
)
// leaseFree is the predicate litestream's AcquireLease applies to the lease
@@ -209,14 +212,15 @@ func Tombstone(ctx context.Context, bucket, location, region, owner string, ttl
//
// - live and held by owner: rewritten with IfMatch on the ETag just read.
// If the lock changed since, ErrTombstoneChanged and nothing is written;
-// - absent, or expired (including lifted): Tombstone, which writes a new
-// one;
+// - absent, or expired (including lifted), whoever wrote it:
+// ErrTombstoneLost, and nothing is written. The stop was lost: in
+// between a wake may have acquired, written and released the lease, so
+// a tombstone is never taken again here;
// - live and held by anyone else: a *HeldError, nothing written.
//
-// Callers must read LockState first and treat absent or expired as a lost
-// stop; never Reassert after Lift. Reassert cannot tell a tombstone that was
-// kept from one that lapsed and was re-created, and in between a wake may
-// have acquired, written and released the lease.
+// Callers read LockState first and treat absent or expired as a lost stop;
+// ErrTombstoneLost is the same verdict for a lock that went between that
+// read and this one. Never Reassert after Lift.
func Reassert(ctx context.Context, bucket, location, region, owner string, ttl time.Duration) error {
if !tombstoneOwnerOK(owner) {
return ErrTombstoneOwner
@@ -232,8 +236,11 @@ func Reassert(ctx context.Context, bucket, location, region, owner string, ttl t
if err != nil {
return err
}
- if absent || leaseFree(cur.ExpiresAt, time.Now()) {
- return Tombstone(ctx, bucket, location, region, owner, ttl)
+ if absent {
+ return fmt.Errorf("reassert tombstone: %w", ErrTombstoneLost)
+ }
+ if leaseFree(cur.ExpiresAt, time.Now()) {
+ return fmt.Errorf("reassert tombstone: %w (%s, expired %s)", ErrTombstoneLost, cur.Owner, cur.ExpiresAt.UTC().Format(time.RFC3339))
}
if cur.Owner != owner {
return heldErr(cur)
diff --git a/internal/lease/tombstone_test.go b/internal/lease/tombstone_test.go
index 9dc522871..abdc5e70b 100644
--- a/internal/lease/tombstone_test.go
+++ b/internal/lease/tombstone_test.go
@@ -1,6 +1,7 @@
package lease_test
import (
+ "bytes"
"context"
"encoding/json"
"errors"
@@ -387,32 +388,38 @@ func TestReassertMovesTheExpiry(t *testing.T) {
wantHeld(t, err, tombMove)
}
-func TestReassertWithNothingToReassertTombstones(t *testing.T) {
+// With nothing live to keep, Reassert reports the stop lost and writes
+// nothing: absent, its own tombstone expired or lifted, and someone else's
+// expired lease. It never takes a tombstone again (a wake may have used the
+// lease in between).
+func TestReassertWithNothingToReassertReportsTheStopLost(t *testing.T) {
s := tombFixture(t)
ctx := context.Background()
- if err := lease.Reassert(ctx, "data", tombLoc, tombRegion, tombMove, time.Hour); err != nil {
- t.Fatalf("absent: %v", err)
- }
- if l, _, ok := s.lock(t, tombLoc); !ok || l.Owner != tombMove {
- t.Fatalf("absent: lock = %+v present %v", l, ok)
- }
- // An expired tombstone of its own is replaced and live again.
- s.putLock(t, tombLoc, tombMove, 5, time.Now().Add(-time.Hour))
- if err := lease.Reassert(ctx, "data", tombLoc, tombRegion, tombMove, time.Hour); err != nil {
- t.Fatalf("expired: %v", err)
- }
- l, _, _ := s.lock(t, tombLoc)
- if l.Owner != tombMove || l.Generation != 6 {
- t.Fatalf("expired: lock = %+v, want owner %s at generation 6", l, tombMove)
+ if err := lease.Reassert(ctx, "data", tombLoc, tombRegion, tombMove, time.Hour); !errors.Is(err, lease.ErrTombstoneLost) {
+ t.Fatalf("absent: %v, want ErrTombstoneLost", err)
}
- near(t, l.ExpiresAt, time.Hour)
- // An expired lease of someone else is replaced the same way.
- s.putLock(t, tombLoc, tombOther, 2, time.Now().Add(-time.Hour))
- if err := lease.Reassert(ctx, "data", tombLoc, tombRegion, tombMove, time.Hour); err != nil {
- t.Fatalf("expired other: %v", err)
+ if _, _, ok := s.lock(t, tombLoc); ok || s.writeCount() != 0 {
+ t.Fatalf("absent: a lock was written (%d writes)", s.writeCount())
}
- if l, _, _ := s.lock(t, tombLoc); l.Owner != tombMove {
- t.Fatalf("expired other: lock = %+v", l)
+ for _, c := range []struct {
+ name string
+ owner string
+ expires time.Time
+ }{
+ {"its own, expired", tombMove, time.Now().Add(-time.Hour)},
+ {"its own, lifted", tombMove, time.Unix(0, 0)},
+ {"someone else's, expired", tombOther, time.Now().Add(-time.Hour)},
+ {"an ordinary lease, expired", "box-a:1", time.Now().Add(-time.Second)},
+ } {
+ s.putLock(t, tombLoc, c.owner, 5, c.expires)
+ _, before, _ := s.lock(t, tombLoc)
+ writes := s.writeCount()
+ if err := lease.Reassert(ctx, "data", tombLoc, tombRegion, tombMove, time.Hour); !errors.Is(err, lease.ErrTombstoneLost) {
+ t.Fatalf("%s: %v, want ErrTombstoneLost", c.name, err)
+ }
+ if _, after, _ := s.lock(t, tombLoc); !bytes.Equal(before, after) || s.writeCount() != writes {
+ t.Fatalf("%s: the lock was rewritten", c.name)
+ }
}
}