carlos / platform Public

lease: assert the dark-but-live refusal is typed, not just an error

Review finding: leg 1 of TestDarkHostCannotReclaimAfterFailover accepted ANY
error as proof that "dark != expired" — a throttle, a transient 5xx or a
credential hiccup would have passed it vacuously, and that leg is the one the
test's own doc comment leans on. It now matches the sibling
TestReplicateWithLease_MutualExclusion: errors.As to *HeldError, plus an
ExpiresAt-in-the-future check so the refusal is provably A's still-live lease
(owner strings can't distinguish the two in-process).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Paul Campbell 2 months ago 9f32bafef6aee89798e26b99cb0ecfcc881cf223 parent 5df73eb

1 file changed, +17 −4

internal/lease/lease_test.go +17 −4

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

index ad090a6b8..8129595d3 100644

--- a/internal/lease/lease_test.go

+++ b/internal/lease/lease_test.go

@@ -269,11 +269,16 @@ func TestDarkHostCannotReclaimAfterFailover(t *testing.T) {

// happen — that is what makes the stop load-bearing.

a.rn.stop()

- // Dark is not expired: A's lock is still in force, so B must be refused.

+ // Dark is not expired: A's lock is still in force, so B must be refused —

+ // and refused BY THE LEASE, not by a throttle, a 5xx or a credential

+ // hiccup, which would let this leg pass while proving nothing. So the

+ // refusal has to be a *HeldError carrying a live expiry, exactly as

+ // TestReplicateWithLease_MutualExclusion demands of its second claimant.

// (Both "hosts" run in this one process and therefore share the leaser's

- // owner string, so the refusal itself — not the name in it — is the

- // evidence here.)

- if peer, err := acquireTTL(ctx, bucket, prefix, region, shortTTL); err == nil {

+ // owner string, so the expiry — not the name — is what identifies the

+ // holder here.)

+ peer, err := acquireTTL(ctx, bucket, prefix, region, shortTTL)

+ if err == nil {

elapsed := time.Since(acquiredAt)

_ = peer.Release(ctx)

if elapsed >= shortTTL {

@@ -281,6 +286,14 @@ func TestDarkHostCannotReclaimAfterFailover(t *testing.T) {

}

t.Fatalf("B acquired after %s while A's un-expired %s lease was still held", elapsed, shortTTL)

}

+ var stillHeld *HeldError

+ if !errors.As(err, &stillHeld) {

+ t.Fatalf("peer refused while A was dark-but-live, but not with *HeldError: %v — an S3 or credential failure here would make this leg vacuous", err)

+ }

+ if !stillHeld.ExpiresAt.After(time.Now()) {

+ t.Fatalf("peer refused by an ALREADY-EXPIRED lease (expires %s, %s after A acquired); want A's still-live lease", stillHeld.ExpiresAt.Format(time.RFC3339), time.Since(acquiredAt))

+ }

+ t.Logf("dark is not expired: peer refused by A's live lease until %s", stillHeld.ExpiresAt.Format(time.RFC3339))

time.Sleep(shortTTL + 5*time.Second) // let A's abandoned lease lapse