carlos / platform Public

offline-move: lift the tombstone by conditional PUT; leasetest enforces If-Match on DELETE

LiftTombstone rewrites the lease as already expired (same owner and
generation) with If-Match on the ETag read, never a DELETE. IsLiftedTombstone
reads that state. Tombstone and Reassert use Acquire's free predicate (no
RenewMargin), a lost write with no readable winner is ErrTombstoneChanged,
and leasetest's S3 server now enforces If-Match on DeleteObject.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

claude 5 days ago c4ea804075df6e98242de818fe4b2e509b17cb7f parent a1ec053

4 files changed, +322 −86

internal/lease/leasetest/s3server.go +11 −0
internal/lease/leasetest/s3server_test.go +32 −0
internal/lease/tombstone.go +95 −28
internal/lease/tombstone_test.go +184 −58

diff --git a/internal/lease/leasetest/s3server.go b/internal/lease/leasetest/s3server.go

index 0b07ae29d..051b9f391 100644

--- a/internal/lease/leasetest/s3server.go

+++ b/internal/lease/leasetest/s3server.go

@@ -217,6 +217,17 @@ func NewS3Server(f *VersionedBucket, serves ...string) (http.Handler, func() int

}

w.WriteHeader(http.StatusNoContent)

case r.Method == http.MethodDelete && key != "":

+ if match := r.Header.Get("If-Match"); match != "" { // as S3 enforces it on DeleteObject

+ current := f.buckets[bucket][key]

+ if len(current) == 0 || current[0].deleteMarker {

+ s3err(w, http.StatusNotFound, "NoSuchKey")

+ return

+ }

+ if current[0].etag != strings.Trim(match, `"`) {

+ s3err(w, http.StatusPreconditionFailed, "PreconditionFailed")

+ return

+ }

id, err := f.bareDeleteLocked(bucket, key) // versioning on: a delete marker

if err != nil {

writeErr(w, err)

diff --git a/internal/lease/leasetest/s3server_test.go b/internal/lease/leasetest/s3server_test.go

index b4546f938..3b3581c2b 100644

--- a/internal/lease/leasetest/s3server_test.go

+++ b/internal/lease/leasetest/s3server_test.go

@@ -255,3 +255,35 @@ func TestS3ServerConditionalReplaceUsesReadETag(t *testing.T) {

t.Fatal("CAS over delete marker accepted", got.Code)

}

+// DeleteObject honours If-Match as S3 does: a stale ETag is a 412 and removes

+// nothing, a missing key is a 404, the current ETag deletes, and a plain

+// delete (no header) is unchanged.

+func TestS3ServerConditionalDeleteUsesReadETag(t *testing.T) {

+ m := NewVersionedBucket()

+ m.AddBucket("data")

+ h, _ := NewS3Server(m)

+ do(t, h, http.MethodPut, "/data/lock.json", "one", map[string]string{"If-None-Match": "*"})

+ stale := do(t, h, http.MethodGet, "/data/lock.json", "", nil).Header().Get("ETag")

+ do(t, h, http.MethodPut, "/data/lock.json", "two", map[string]string{"If-Match": stale})

+ current := do(t, h, http.MethodGet, "/data/lock.json", "", nil).Header().Get("ETag")

+ before := m.Snapshot()

+ if got := do(t, h, http.MethodDelete, "/data/lock.json", "", map[string]string{"If-Match": stale}); got.Code != http.StatusPreconditionFailed {

+ t.Fatal("stale conditional delete accepted", got.Code)

+ }

+ if m.Snapshot() != before {

+ t.Fatal("stale conditional delete mutated versions")

+ }

+ if got := do(t, h, http.MethodDelete, "/data/lock.json", "", map[string]string{"If-Match": current}); got.Code != http.StatusNoContent {

+ t.Fatal("current conditional delete refused", got.Code)

+ }

+ if got := do(t, h, http.MethodDelete, "/data/lock.json", "", map[string]string{"If-Match": current}); got.Code != http.StatusNotFound {

+ t.Fatal("conditional delete of a deleted key", got.Code)

+ }

+ if got := do(t, h, http.MethodDelete, "/data/never", "", map[string]string{"If-Match": current}); got.Code != http.StatusNotFound {

+ t.Fatal("conditional delete of a missing key", got.Code)

+ }

+ if got := do(t, h, http.MethodDelete, "/data/never", "", nil); got.Code != http.StatusNoContent {

+ t.Fatal("plain delete changed", got.Code)

+ }

diff --git a/internal/lease/tombstone.go b/internal/lease/tombstone.go

index 293ec9ec3..c0054208f 100644

--- a/internal/lease/tombstone.go

+++ b/internal/lease/tombstone.go

@@ -14,19 +14,25 @@ package lease

// - Tombstone writes it. It never replaces a live holder's lease.

// - Reassert pushes the expiry out again, only on a lease the same move

// owns, with the ETag it just read.

-// - LiftTombstone deletes it, only when the owner is a moved:<id> lease the

-// caller claims as its own, with the ETag it just read.

+// - LiftTombstone lifts it, only when the owner is a moved:<id> lease the

+// caller claims as its own, with the ETag it just read. It lifts by a

+// conditional PUT that rewrites the lease as already expired, never by a

+// DELETE: Ceph RGW and MinIO are not known to enforce If-Match on a

+// DELETE, and the conditional PUT is the primitive the rest of this

+// package relies on. IsLiftedTombstone reads that state back.

-// "Live" is the reading LockState gives: a lease counts as held until its

-// expiry has passed by more than RenewMargin, so a holder whose clock runs a

-// little behind is never displaced.

+// "Free" is exactly what Acquire means by it (leaseFree): the lease's expiry

+// has passed. There is no RenewMargin here. A tombstone that is free to a

+// wake is free to the next Tombstone, and the two cannot drift apart.

import (

+ "bytes"

"context"

"encoding/json"

"errors"

"fmt"

"io"

+ "net/http"

"strings"

"time"

@@ -56,6 +62,28 @@ var (

ErrTombstoneChanged = errors.New("lease: the lock changed under the tombstone write")

)

+// leaseFree is the predicate litestream's AcquireLease applies to the lease

+// it reads (Lease.IsExpired: now is after the expiry), which is what decides

+// whether lease.Acquire takes a lock over. Tombstone, Reassert and

+// IsLiftedTombstone use it so a lease is free to them exactly when it is free

+// to a wake. TestTombstoneAndAcquireAgreeOnFree pins the two together.

+func leaseFree(expires, now time.Time) bool { return now.After(expires) }

+// liftedExpiry is the expiry LiftTombstone writes: long past, so every

+// reading (leaseFree, and LockState's RenewMargin) agrees the lease is free.

+var liftedExpiry = time.Unix(0, 0).UTC()

+// IsLiftedTombstone reports whether a lock read as owner and expires (from

+// LockState) is a lifted tombstone: a moved:<id> lease that has expired. That

+// is what LiftTombstone leaves behind, and it is free to every Acquire (which

+// already treats any expired lease as free) and to the next Tombstone. It is

+// false for an ordinary process's lease, expired or not, and for a tombstone

+// that is still unexpired. An absent lock is not "lifted" here: callers read

+// absence from LockState's zero owner and expiry and treat it the same way.

+func IsLiftedTombstone(owner string, expires time.Time) bool {

+ return tombstoneOwnerOK(owner) && !expires.IsZero() && leaseFree(expires, time.Now())

func tombstoneOwnerOK(owner string) bool {

return strings.HasPrefix(owner, TombstonePrefix) && len(owner) > len(TombstonePrefix)

}

@@ -123,10 +151,13 @@ func heldErr(l *litestream.Lease) error {

// Tombstone writes the lease at s3://bucket/<location>/lock.json owned by

// owner (moved:<id>), expiring ttl from now (at most MaxTombstoneTTL).

-// - absent, or expired past RenewMargin: created (IfNoneMatch "*") or

+// - absent, or expired (as Acquire reads it): created (IfNoneMatch "*") or

// replaced (IfMatch on the expired lease's ETag);

-// - held by owner: nil and no write; the caller re-asserts with Reassert;

-// - held by anyone else, live: a *HeldError, nothing written.

+// - live and held by owner: nil and no write; the caller re-asserts with

+// Reassert;

+// - live and held by anyone else: a *HeldError, nothing written;

+// - the lock changed under the write and cannot be read back:

+// ErrTombstoneChanged.

// A live holder is never replaced: the replace is conditional on the ETag of

// the lease that was read as expired, so a holder that renews in between

@@ -146,7 +177,7 @@ func Tombstone(ctx context.Context, bucket, location, region, owner string, ttl

if err != nil {

return err

}

- if !absent && lockHeld(cur.ExpiresAt, time.Now()) {

+ if !absent && !leaseFree(cur.ExpiresAt, time.Now()) {

if cur.Owner == owner {

return nil

}

@@ -158,7 +189,13 @@ func Tombstone(ctx context.Context, bucket, location, region, owner string, ttl

if _, err := leaser.AcquireLease(ctx); err != nil {

var lexists *litestream.LeaseExistsError

if errors.As(err, &lexists) {

- if lexists.Owner == owner {

+ switch lexists.Owner {

+ case "":

+ // The write lost its condition and AcquireLease could not

+ // read the winner back (it was removed again, or the read

+ // failed). There is no holder to name.

+ return fmt.Errorf("tombstone lease: %w", ErrTombstoneChanged)

+ case owner:

return nil

}

return fmt.Errorf("acquire lease: %w", &HeldError{Owner: lexists.Owner, ExpiresAt: lexists.ExpiresAt})

@@ -170,10 +207,16 @@ func Tombstone(ctx context.Context, bucket, location, region, owner string, ttl

// Reassert moves the expiry of a tombstone owner already holds to now+ttl:

-// - held by owner: rewritten with IfMatch on the ETag just read. If the

-// lock changed since, ErrTombstoneChanged and nothing is written;

-// - absent, or expired past RenewMargin: Tombstone;

-// - held by anyone else, live: a *HeldError, nothing written.

+// - 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;

+// - 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.

func Reassert(ctx context.Context, bucket, location, region, owner string, ttl time.Duration) error {

if !tombstoneOwnerOK(owner) {

return ErrTombstoneOwner

@@ -189,7 +232,7 @@ func Reassert(ctx context.Context, bucket, location, region, owner string, ttl t

if err != nil {

return err

}

- if absent || !lockHeld(cur.ExpiresAt, time.Now()) {

+ if absent || leaseFree(cur.ExpiresAt, time.Now()) {

return Tombstone(ctx, bucket, location, region, owner, ttl)

}

if cur.Owner != owner {

@@ -204,15 +247,23 @@ func Reassert(ctx context.Context, bucket, location, region, owner string, ttl t

return nil

}

-// LiftTombstone deletes the tombstone at s3://bucket/<location>/lock.json.

+// LiftTombstone lifts the tombstone at s3://bucket/<location>/lock.json by

+// rewriting it as an expired lease of the same owner and generation, with

+// IfMatch on the ETag just read. It never deletes the object.

-// - absent: (false, nil);

+// - absent: (false, nil), and mine is not consulted;

// - owner is not a moved:<id> that mine accepts: (false, *HeldError),

-// nothing deleted. This holds for an expired lease too: only the move

+// nothing written. This holds for an expired lease too: only the move

// that wrote a tombstone lifts it;

-// - mine: deleted with IfMatch on the ETag just read, (true, nil). A lock

-// that was rewritten since is (false, ErrTombstoneChanged); one that

-// vanished since is (false, nil).

+// - mine and already lifted (expired, IsLiftedTombstone): (false, nil),

+// nothing written;

+// - mine and live: rewritten, (true, nil). Acquire takes the lease from

+// here, as it takes any expired lease, and so does Tombstone;

+// - the lock was rewritten since the read: (false, ErrTombstoneChanged),

+// and the rewrite stands. One that was removed since is (false, nil).

+//

+// After a lift, LockState reports held=false with the move's owner and a

+// past expiry; IsLiftedTombstone recognises that. Never Reassert after it.

func LiftTombstone(ctx context.Context, bucket, location, region string, mine func(owner string) bool) (lifted bool, err error) {

leaser, key, err := tombstoneLeaser(ctx, bucket, location, region, "", 0)

if err != nil {

@@ -228,14 +279,30 @@ func LiftTombstone(ctx context.Context, bucket, location, region string, mine fu

if !tombstoneOwnerOK(cur.Owner) || mine == nil || !mine(cur.Owner) {

return false, heldErr(cur)

}

- switch err := leaser.ReleaseLease(ctx, cur); {

- case err == nil:

- return true, nil

- case errors.Is(err, lss3.ErrLeaseAlreadyReleased):

+ if IsLiftedTombstone(cur.Owner, cur.ExpiresAt) {

return false, nil

- case errors.Is(err, litestream.ErrLeaseNotHeld):

- return false, fmt.Errorf("lift tombstone: %w", ErrTombstoneChanged)

- default:

+ }

+ body, err := json.Marshal(&litestream.Lease{Generation: cur.Generation, ExpiresAt: liftedExpiry, Owner: cur.Owner})

+ if err != nil {

return false, fmt.Errorf("lift tombstone: %w", err)

}

+ _, err = leaser.Client().PutObject(ctx, &awss3.PutObjectInput{

+ Bucket: aws.String(leaser.Bucket),

+ Key: aws.String(key),

+ Body: bytes.NewReader(body),

+ ContentType: aws.String("application/json"),

+ IfMatch: aws.String(cur.ETag),

+ })

+ if err == nil {

+ return true, nil

+ }

+ if code, status := apiCode(err); code == "PreconditionFailed" || status == http.StatusPreconditionFailed || code == "NoSuchKey" || status == http.StatusNotFound {

+ // Rewritten, or gone (AWS answers a conditional PUT on a missing key

+ // 404, some stores 412). Read it back to tell which.

+ if _, gone, rerr := readLock(ctx, leaser, key); rerr == nil && gone {

+ return false, nil

+ }

+ return false, fmt.Errorf("lift tombstone: %w", ErrTombstoneChanged)

+ }

+ return false, fmt.Errorf("lift tombstone: %w", err)

}

diff --git a/internal/lease/tombstone_test.go b/internal/lease/tombstone_test.go

index 363acaa68..9dc522871 100644

--- a/internal/lease/tombstone_test.go

+++ b/internal/lease/tombstone_test.go

@@ -2,10 +2,8 @@ package lease_test

import (

"context"

- "crypto/md5"

"encoding/json"

"errors"

- "fmt"

"net/http"

"net/http/httptest"

"strings"

@@ -24,11 +22,10 @@ const (

tombRegion = "test-region"

)

-// tombStore is leasetest's S3 server behind a wrapper that does two things the

-// server does not: it enforces If-Match on DELETE (as S3 does; the leaser's

-// release depends on it), and it runs a hook in the window between the

-// code under test's read of lock.json and its conditional write, so a race is

-// a request ordering, not a sleep.

+// tombStore is leasetest's S3 server behind a wrapper that runs a hook in the

+// window between the code under test's read of lock.json and its conditional

+// write, so a race is a request ordering, not a sleep. The conditions

+// themselves (If-Match, If-None-Match) are leasetest's own.

type tombStore struct {

*leasetest.VersionedBucket

mu sync.Mutex

@@ -60,19 +57,6 @@ func tombFixture(t *testing.T) *tombStore {

if hook != nil {

hook(r.Method)

}

- if r.Method == http.MethodDelete {

- match := strings.Trim(r.Header.Get("If-Match"), `"`)

- cur, ok := bucket.Current("data", key)

- switch {

- case match == "":

- case !ok:

- tombXMLErr(w, http.StatusNotFound, "NoSuchKey")

- return

- case fmt.Sprintf("%x", md5.Sum(cur)) != match:

- tombXMLErr(w, http.StatusPreconditionFailed, "PreconditionFailed")

- return

- }

}

inner.ServeHTTP(w, r)

}))

@@ -87,10 +71,10 @@ func tombFixture(t *testing.T) *tombStore {

return s

}

-func tombXMLErr(w http.ResponseWriter, status int, code string) {

- w.Header().Set("Content-Type", "application/xml")

- w.WriteHeader(status)

- fmt.Fprintf(w, `<?xml version="1.0" encoding="UTF-8"?><Error><Code>%s</Code><Message>%s</Message></Error>`, code, code)

+func (s *tombStore) methods() []string {

+ s.mu.Lock()

+ defer s.mu.Unlock()

+ return append([]string(nil), s.writes...)

}

func (s *tombStore) writeCount() int {

@@ -208,16 +192,91 @@ func TestTombstoneNeverReplacesALiveHolder(t *testing.T) {

}

-// A holder whose expiry passed less than RenewMargin ago still counts as

-// holding, as LockState reads it.

-func TestTombstoneRespectsTheRenewMargin(t *testing.T) {

+// Free means what Acquire means by it: the expiry has passed. There is no

+// RenewMargin grace, so a lease that lapsed a moment ago is taken by Tombstone

+// exactly as lease.Acquire would take it. The two are run side by side on

+// identical locks so they cannot drift.

+func TestTombstoneAndAcquireAgreeOnFree(t *testing.T) {

+ ctx := context.Background()

+ for name, tc := range map[string]struct {

+ expires time.Time

+ free bool

+ }{

+ "expired a second ago": {time.Now().Add(-time.Second), true},

+ "expired within the margin": {time.Now().Add(-lease.RenewMargin / 2), true},

+ "expires in a minute": {time.Now().Add(time.Minute), false},

+ } {

+ t.Run(name, func(t *testing.T) {

+ s := tombFixture(t)

+ s.putLock(t, tombLoc, "box-a:1", 3, tc.expires)

+ _, before, _ := s.lock(t, tombLoc)

+ acq, acqErr := lease.Acquire(ctx, "data", tombLoc, tombRegion)

+ if acqErr == nil {

+ _ = acq.Release(ctx)

+ }

+ // Put the same lock back for Tombstone.

+ s.putLock(t, tombLoc, "box-a:1", 3, tc.expires)

+ tombErr := lease.Tombstone(ctx, "data", tombLoc, tombRegion, tombMove, time.Hour)

+ if (acqErr == nil) != tc.free || (tombErr == nil) != tc.free {

+ t.Fatalf("free %v: Acquire err %v, Tombstone err %v", tc.free, acqErr, tombErr)

+ }

+ if !tc.free {

+ wantHeld(t, tombErr, "box-a:1")

+ if _, after, _ := s.lock(t, tombLoc); string(after) != string(before) {

+ t.Fatal("a live lease was replaced")

+ }

+ })

+ }

+// An own tombstone that has expired is free like any other: Tombstone writes

+// a fresh one rather than reporting the dead one held.

+func TestTombstoneRewritesItsOwnExpiredTombstone(t *testing.T) {

s := tombFixture(t)

- s.putLock(t, tombLoc, "box-a:1", 3, time.Now().Add(-lease.RenewMargin/2))

- _, before, _ := s.lock(t, tombLoc)

+ s.putLock(t, tombLoc, tombMove, 4, time.Now().Add(-time.Second))

+ if err := lease.Tombstone(context.Background(), "data", tombLoc, tombRegion, tombMove, time.Hour); err != nil {

+ t.Fatal(err)

+ }

+ l, _, _ := s.lock(t, tombLoc)

+ if l.Owner != tombMove || l.Generation != 5 {

+ t.Fatalf("lock = %+v, want owner %s at generation 5", l, tombMove)

+ }

+ near(t, l.ExpiresAt, time.Hour)

+// The write loses its condition and AcquireLease cannot read a winner back:

+// there is no holder to name, so the answer is ErrTombstoneChanged, never a

+// HeldError with an empty owner.

+func TestTombstoneWriteLostThenReadLostIsChanged(t *testing.T) {

+ s := tombFixture(t)

+ s.putLock(t, tombLoc, "box-a:1", 3, time.Now().Add(-time.Hour))

+ s.beforeWrite(func(string) { s.BareDelete("data", tombLoc+"/lock.json") })

err := lease.Tombstone(context.Background(), "data", tombLoc, tombRegion, tombMove, time.Hour)

- wantHeld(t, err, "box-a:1")

- if _, after, _ := s.lock(t, tombLoc); string(after) != string(before) {

- t.Fatal("a lease inside the margin was replaced")

+ var he *lease.HeldError

+ if !errors.Is(err, lease.ErrTombstoneChanged) || errors.As(err, &he) {

+ t.Fatalf("err = %v, want ErrTombstoneChanged and no HeldError", err)

+ }

+ if _, _, ok := s.lock(t, tombLoc); ok {

+ t.Fatal("Tombstone wrote over the vanished lock")

+ }

+// The write loses its condition and the read-back shows this very owner

+// (another pass of the same move got there first): nothing to do.

+func TestTombstoneWriteLostToItsOwnOwnerIsNil(t *testing.T) {

+ s := tombFixture(t)

+ s.putLock(t, tombLoc, "box-a:1", 3, time.Now().Add(-time.Hour))

+ var theirs []byte

+ s.beforeWrite(func(string) {

+ s.putLock(t, tombLoc, tombMove, 4, time.Now().Add(2*time.Hour))

+ _, theirs, _ = s.lock(t, tombLoc)

+ })

+ if err := lease.Tombstone(context.Background(), "data", tombLoc, tombRegion, tombMove, time.Hour); err != nil {

+ t.Fatalf("err = %v, want nil", err)

+ }

+ if _, now, _ := s.lock(t, tombLoc); string(now) != string(theirs) {

+ t.Fatalf("the winner's lock was overwritten: %s -> %s", theirs, now)

}

@@ -446,7 +505,7 @@ func TestLiftTombstone(t *testing.T) {

if err := lease.Tombstone(ctx, "data", tombLoc, tombRegion, tombMove, time.Hour); err != nil {

t.Fatal(err)

}

- _, before, _ := s.lock(t, tombLoc)

+ live, before, _ := s.lock(t, tombLoc)

// Another move's tombstone is not ours to lift.

lifted, err := lease.LiftTombstone(ctx, "data", tombLoc, tombRegion, isMine(tombOther))

@@ -462,14 +521,24 @@ func TestLiftTombstone(t *testing.T) {

t.Fatalf("nil mine: lifted %v err %v", lifted, err)

}

- // Ours lifts, and the host can be acquired again.

+ // Ours lifts: by a conditional PUT, leaving the same owner and generation

+ // with a long-past expiry, never by a DELETE.

+ writes := s.writeCount()

lifted, err = lease.LiftTombstone(ctx, "data", tombLoc, tombRegion, mine)

if err != nil || !lifted || called != tombMove {

t.Fatalf("lift: lifted %v err %v consulted %q", lifted, err, called)

}

- if _, _, ok := s.lock(t, tombLoc); ok {

- t.Fatal("lock.json is still there")

+ if got := s.methods()[writes:]; len(got) != 1 || got[0] != http.MethodPut {

+ t.Fatalf("lift wrote %v, want one PUT", got)

}

+ l, _, ok := s.lock(t, tombLoc)

+ if !ok || l.Owner != tombMove || l.Generation != live.Generation || !l.ExpiresAt.Before(time.Now().Add(-lease.RenewMargin)) {

+ t.Fatalf("after the lift: %+v present %v, want %s at generation %d, long expired", l, ok, tombMove, live.Generation)

+ }

+ if held, owner, expires, err := lease.LockState(ctx, "data", tombLoc, tombRegion); err != nil || held || !lease.IsLiftedTombstone(owner, expires) {

+ t.Fatalf("LockState after the lift: held %v owner %q expires %v err %v", held, owner, expires, err)

+ }

+ // The host can be acquired again, and so can a new tombstone.

held, err := lease.Acquire(ctx, "data", tombLoc, tombRegion)

if err != nil {

t.Fatalf("Acquire after the lift: %v", err)

@@ -477,17 +546,81 @@ func TestLiftTombstone(t *testing.T) {

if err := held.Release(ctx); err != nil {

t.Fatal(err)

}

- // Already lifted: (false, nil), however often it is asked.

+func TestLiftedTombstoneIsFreeToTheNextMove(t *testing.T) {

+ s := tombFixture(t)

+ ctx := context.Background()

+ if err := lease.Tombstone(ctx, "data", tombLoc, tombRegion, tombMove, time.Hour); err != nil {

+ t.Fatal(err)

+ }

+ if lifted, err := lease.LiftTombstone(ctx, "data", tombLoc, tombRegion, isMine(tombMove)); err != nil || !lifted {

+ t.Fatalf("lifted %v err %v", lifted, err)

+ }

+ if err := lease.Tombstone(ctx, "data", tombLoc, tombRegion, tombOther, time.Hour); err != nil {

+ t.Fatalf("Tombstone over a lifted one: %v", err)

+ }

+ if l, _, _ := s.lock(t, tombLoc); l.Owner != tombOther {

+ t.Fatalf("lock = %+v", l)

+ }

+// Already lifted, however it came about, is (false, nil) with nothing written.

+func TestLiftTombstoneAlreadyLifted(t *testing.T) {

+ s := tombFixture(t)

+ ctx := context.Background()

+ if err := lease.Tombstone(ctx, "data", tombLoc, tombRegion, tombMove, time.Hour); err != nil {

+ t.Fatal(err)

+ }

+ if lifted, err := lease.LiftTombstone(ctx, "data", tombLoc, tombRegion, isMine(tombMove)); err != nil || !lifted {

+ t.Fatalf("first lift: %v %v", lifted, err)

+ }

+ writes := s.writeCount()

for i := 0; i < 2; i++ {

- if lifted, err := lease.LiftTombstone(ctx, "data", tombLoc, tombRegion, mine); lifted || err != nil {

+ if lifted, err := lease.LiftTombstone(ctx, "data", tombLoc, tombRegion, isMine(tombMove)); lifted || err != nil {

t.Fatalf("already lifted: lifted %v err %v", lifted, err)

}

+ // An own tombstone that simply ran out is lifted too.

+ s.putLock(t, tombLoc, tombMove, 4, time.Now().Add(-48*time.Hour))

+ writes = s.writeCount()

+ if lifted, err := lease.LiftTombstone(ctx, "data", tombLoc, tombRegion, isMine(tombMove)); lifted || err != nil {

+ t.Fatalf("expired own: lifted %v err %v", lifted, err)

+ }

+ if s.writeCount() != writes {

+ t.Fatal("lifting an already-lifted tombstone wrote")

+ }

+ // And so is an absent one.

+ s.BareDelete("data", tombLoc+"/lock.json")

+ if lifted, err := lease.LiftTombstone(ctx, "data", tombLoc, tombRegion, isMine(tombMove)); lifted || err != nil {

+ t.Fatalf("absent: lifted %v err %v", lifted, err)

+ }

+func TestIsLiftedTombstone(t *testing.T) {

+ past, future := time.Now().Add(-time.Second), time.Now().Add(time.Hour)

+ for name, tc := range map[string]struct {

+ owner string

+ expires time.Time

+ want bool

+ }{

+ "expired move": {tombMove, past, true},

+ "long expired move": {tombMove, time.Unix(0, 0), true},

+ "live move": {tombMove, future, false},

+ "expired ordinary lease": {"box-a:1", past, false},

+ "live ordinary lease": {"box-a:1", future, false},

+ "absent (zero owner/exp)": {"", time.Time{}, false},

+ "bare prefix": {"moved:", past, false},

+ "zero expiry": {tombMove, time.Time{}, false},

+ } {

+ if got := lease.IsLiftedTombstone(tc.owner, tc.expires); got != tc.want {

+ t.Errorf("%s: IsLiftedTombstone(%q, %v) = %v, want %v", name, tc.owner, tc.expires, got, tc.want)

+ }

}

// Whatever mine says, only a moved:<id> lease is lifted: a live process's

-// lease is never this function's to delete.

+// lease is never this function's to touch.

func TestLiftTombstoneLeavesOrdinaryLeasesAlone(t *testing.T) {

s := tombFixture(t)

ctx := context.Background()

@@ -503,25 +636,11 @@ func TestLiftTombstoneLeavesOrdinaryLeasesAlone(t *testing.T) {

t.Fatalf("%s: the lease changed", name)

}

- if s.writeCount() != 0 {

+ if s.writeCount() != 0 { // putLock writes the bucket directly; only requests count

t.Fatal("a refused lift wrote")

}

-// Our own tombstone is lifted whether or not it has run out: the move that

-// wrote it is the one that says so.

-func TestLiftTombstoneLiftsAnExpiredOwn(t *testing.T) {

- s := tombFixture(t)

- s.putLock(t, tombLoc, tombMove, 4, time.Now().Add(-48*time.Hour))

- lifted, err := lease.LiftTombstone(context.Background(), "data", tombLoc, tombRegion, isMine(tombMove))

- if err != nil || !lifted {

- t.Fatalf("lifted %v err %v", lifted, err)

- }

- if _, _, ok := s.lock(t, tombLoc); ok {

- t.Fatal("lock.json is still there")

- }

func TestLiftTombstoneFailsWhenTheLockChangesUnderIt(t *testing.T) {

ctx := context.Background()

t.Run("rewritten", func(t *testing.T) {

@@ -531,7 +650,7 @@ func TestLiftTombstoneFailsWhenTheLockChangesUnderIt(t *testing.T) {

}

var after []byte

s.beforeWrite(func(method string) {

- if method != http.MethodDelete {

+ if method != http.MethodPut {

t.Errorf("LiftTombstone sent a %s", method)

}

// Still ours by owner, but a different lock: Reassert beat the lift.

@@ -543,12 +662,16 @@ func TestLiftTombstoneFailsWhenTheLockChangesUnderIt(t *testing.T) {

t.Fatalf("lifted %v err %v, want (false, ErrTombstoneChanged)", lifted, err)

}

if _, now, ok := s.lock(t, tombLoc); !ok || string(now) != string(after) {

- t.Fatalf("the lift deleted the rewrite: %s", now)

+ t.Fatalf("the lift overwrote the rewrite: %s", now)

}

})

t.Run("taken over by a holder", func(t *testing.T) {

s := tombFixture(t)

- s.putLock(t, tombLoc, tombMove, 4, time.Now().Add(-time.Hour))

+ // Live tombstone read; before the write a holder cannot legitimately

+ // appear, but a rewrite by anything must still stand.

+ if err := lease.Tombstone(ctx, "data", tombLoc, tombRegion, tombMove, time.Hour); err != nil {

+ t.Fatal(err)

+ }

var after []byte

s.beforeWrite(func(string) {

s.putLock(t, tombLoc, "box-b:2", 5, time.Now().Add(15*time.Minute))

@@ -559,7 +682,7 @@ func TestLiftTombstoneFailsWhenTheLockChangesUnderIt(t *testing.T) {

t.Fatalf("lifted %v err %v, want (false, ErrTombstoneChanged)", lifted, err)

}

if _, now, _ := s.lock(t, tombLoc); string(now) != string(after) {

- t.Fatalf("the lift deleted a live holder's lock: %s", now)

+ t.Fatalf("the lift overwrote a live holder's lock: %s", now)

}

})

t.Run("removed", func(t *testing.T) {

@@ -571,6 +694,9 @@ func TestLiftTombstoneFailsWhenTheLockChangesUnderIt(t *testing.T) {

if lifted, err := lease.LiftTombstone(ctx, "data", tombLoc, tombRegion, isMine(tombMove)); lifted || err != nil {

t.Fatalf("lifted %v err %v, want (false, nil): already gone", lifted, err)

}

+ if _, _, ok := s.lock(t, tombLoc); ok {

+ t.Fatal("the lift re-created a removed lock")

+ }

})

}