Skip to content

Commit 927ea14

Browse files
filimonovclaude
andcommitted
cas: fix false GC-lease steal — heartbeat seq compared across different hb owners
The steal gate's liveness check compared the observed `gc/hb` sequence against the previous observation while matching the hb OWNER against the CURRENT LEASE owner: hb_alive = has_observation && hb.owner == lease.owner && hb.hb_seq > last_seen_hb_seq `hb_seq` values are only comparable under the same hb owner — which is exactly why `last_seen_hb_owner` was stored — but the stored owner was never read (the half-finished check this fixes). The hazard is reachable: a deposed leader's heartbeat thread keeps pulsing (`i_am_leader` resets only on its next round) with `owner = itself`, and `pulseHeartbeat`'s losing CAS writes vanish silently, so a zombie old leader can keep `gc/hb.owner` pointing at itself while the live new leader is pulsing too. On both of a follower's ticks `hb.owner != lease.owner` then made `hb_alive` false, and with the new leader's lease tuple frozen mid-round the follower stole the lease from a live, pulsing leader. The gate is now observation-symmetric with the frozen-lease check: ANY movement of the observed `(owner, hb_seq)` pair between the contender's two ticks is proof of life, and `hb_seq` is compared only under the same remembered hb owner (an owner change re-arms the window). Liveness is preserved — only leaders pulse and a deposed leader stops within one round interval, so a genuinely dead leader still gets stolen from one window later at most. Failing-first regression test `CasGcLease.StaleOwnerHeartbeatDoesNotEnableFalseSteal` reproduces the zombie-masking interleaving deterministically (steal confirmed on the old predicate) and pins both the no-false-steal and the still-steals-when- frozen halves. Validated: full corrected CAS gtest battery 906/907 — the one failure is the pre-existing tracked `RefWriterRecoverySeal` F3-1a fragility (`docs/superpowers/cas/BACKLOG.md`), unrelated to the lease protocol. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 7eb246d commit 927ea14

3 files changed

Lines changed: 55 additions & 3 deletions

File tree

src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Gc/CasGc.cpp

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2322,9 +2322,18 @@ bool Gc::acquireOrRenewLease(GcState & state, Token & state_token, bool allow_st
23222322
GcHeartbeat hb;
23232323
if (const auto hb_got = store->backend().get(store->layout().gcHbKey()))
23242324
hb = decodeGcHeartbeat(hb_got->bytes);
2325+
/// Observation-based heartbeat liveness, symmetric with the frozen-lease-tuple check below:
2326+
/// ANY movement of the observed (owner, hb_seq) pair between this contender's two ticks is
2327+
/// proof of life, and `hb_seq` values are comparable only under the SAME remembered hb owner.
2328+
/// Deliberately NOT compared against `current.lease.owner`: a deposed leader's heartbeat
2329+
/// thread keeps pulsing (with `owner = itself`) until its next round resets `i_am_leader`,
2330+
/// and its writes can race out the live new leader's pulses — an hb pair that keeps moving
2331+
/// under the OLD owner's name must still read as "alive", or a live, pulsing new leader gets
2332+
/// its lease stolen. An hb owner change re-arms the window (this tick's pair is remembered
2333+
/// below); a steal happens only once the lease tuple AND the hb pair are both frozen across
2334+
/// a full window.
23252335
const bool hb_alive = has_observation
2326-
&& hb.owner == current.lease.owner
2327-
&& hb.hb_seq > last_seen_hb_seq;
2336+
&& (hb.owner != last_seen_hb_owner || hb.hb_seq > last_seen_hb_seq);
23282337

23292338
const bool incumbent_renewed = !has_observation
23302339
|| current.lease.owner != last_seen_owner

src/Disks/DiskObjectStorage/MetadataStorages/ContentAddressed/Gc/CasGc.h

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -374,7 +374,9 @@ class Gc
374374
bool has_observation = false;
375375
UInt128 last_seen_owner{};
376376
uint64_t last_seen_seq = 0;
377-
/// Heartbeat observed alongside the lease (gates the steal).
377+
/// Heartbeat pair observed alongside the lease (gates the steal): a steal requires the lease
378+
/// tuple AND this (owner, hb_seq) pair to be frozen across a full window. `hb_seq` is compared
379+
/// only when the remembered hb owner matches; an owner change counts as movement (alive).
378380
UInt128 last_seen_hb_owner{};
379381
uint64_t last_seen_hb_seq = 0;
380382

src/Disks/tests/gtest_cas_gc_round.cpp

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@ namespace
4646
const UInt128 kGc = hexToU128("00000000000000000000000000000001");
4747
const UInt128 kGcA = hexToU128("0000000000000000000000000000000a");
4848
const UInt128 kGcB = hexToU128("0000000000000000000000000000000b");
49+
const UInt128 kGcC = hexToU128("0000000000000000000000000000000c");
4950

5051
ManifestRef ref(uint64_t seq, uint64_t inst)
5152
{
@@ -329,6 +330,46 @@ TEST(CasGcLease, AcquireTimePulseProtectsNewLeadersFirstRound)
329330
EXPECT_EQ(readState(*b, *s).lease.owner, kGcA); /// gc1 keeps the lease through its whole first round
330331
}
331332

333+
TEST(CasGcLease, StaleOwnerHeartbeatDoesNotEnableFalseSteal)
334+
{
335+
/// A deposed leader's heartbeat thread keeps pulsing until its next round notices the lost lease
336+
/// (`i_am_leader` is only reset there), and `pulseHeartbeat` stamps `owner = self` while a losing
337+
/// CAS write silently vanishes — so a zombie old leader can keep `gc/hb.owner` pointing at ITSELF
338+
/// even while the live new leader is pulsing too. The liveness gate must therefore treat ANY
339+
/// movement of the observed (owner, hb_seq) pair between a follower's two ticks as "someone is
340+
/// alive": comparing hb_seq is only meaningful against the SAME remembered hb owner. The old
341+
/// predicate compared `hb.owner` with the LEASE owner instead, so a zombie-owned hb read as
342+
/// "not the leader's heartbeat" on both ticks and a live, pulsing new leader got its lease stolen.
343+
std::shared_ptr<InMemoryBackend> b;
344+
auto s = openTestPool(b);
345+
Gc gc1(s, kGcA);
346+
Gc gc2(s, kGcB);
347+
Gc gc3(s, kGcC);
348+
349+
/// gc1 leads, beats, then dies mid-round; gc2 legitimately steals the lease.
350+
ASSERT_TRUE(gc1.runRegularRound().acquired_lease);
351+
Gc::pulseHeartbeat(*s, kGcA);
352+
EXPECT_FALSE(gc2.runRegularRound().acquired_lease); /// obs #1 of gc1's frozen tuple
353+
EXPECT_TRUE(gc2.runRegularRound().acquired_lease); /// obs #2: frozen lease + frozen hb => steal
354+
ASSERT_EQ(readState(*b, *s).lease.owner, kGcB);
355+
356+
/// gc2 is now mid-long-round (lease tuple frozen) and PULSING — but gc1's zombie heartbeat
357+
/// thread interleaves after every gc2 pulse, so the follower gc3 only ever OBSERVES gc1-owned
358+
/// heartbeats. The pair keeps moving, which is proof of life.
359+
Gc::pulseHeartbeat(*s, kGcB);
360+
Gc::pulseHeartbeat(*s, kGcA); /// zombie masks gc2's pulse
361+
EXPECT_FALSE(gc3.runRegularRound().acquired_lease); /// obs #1: records (hb owner=A, seq)
362+
Gc::pulseHeartbeat(*s, kGcB);
363+
Gc::pulseHeartbeat(*s, kGcA); /// zombie masks again
364+
EXPECT_FALSE(gc3.runRegularRound().acquired_lease); /// obs #2: hb pair MOVED => alive => NO steal
365+
EXPECT_EQ(readState(*b, *s).lease.owner, kGcB); /// the live leader keeps its lease
366+
367+
/// Liveness is preserved: once everything genuinely freezes (gc2 dead, zombie gone), the next
368+
/// tick completes the window — obs #2 above already re-armed on the now-frozen (lease, hb) pair.
369+
EXPECT_TRUE(gc3.runRegularRound().acquired_lease); /// still frozen a full tick later => steal
370+
EXPECT_EQ(readState(*b, *s).lease.owner, kGcC);
371+
}
372+
332373
TEST(CasGcLease, FailoverStealOnceHeartbeatStops)
333374
{
334375
/// B160: once the incumbent stops heartbeating (it died), a follower observing the now-frozen

0 commit comments

Comments
 (0)