fix(coordination): make Lease.checkAlive() honour the TTL, re-validate LeaseMajority wins (#937) by Nevs08 · Pull Request #1658 · pathosDev/actor-ts

Nevs08 · 2026-10-06T18:28:21Z

Summary

Fixes #937. Lease.checkAlive() returned a cached held flag in both backends, and only a renewal that noticed the loss ever cleared it. A holder whose event loop stalled past the TTL (GC pause, starved container) never ran that renewal, so it kept reporting ownership after another node had legitimately taken the lease — and the renewal that finally ran extended the lapsed record if nobody had claimed it yet. LeaseMajority replayed its cached win on top of that, so both halves of a split could act as the survivor.

Changes (every commit green on its own)

Commits 1–4 are the fix. Commits 5–12 come from independent verification rounds:

a guard for late renewal answers;
tests that pin what moves the Kubernetes deadline, on the renewal and acquire paths;
tests that pin the record's renewTime and the local deadline sharing one stamp;
tests that a lost re-acquire leaves the deadline alone;
tests for the 409 re-read guard, the dropped LeaseMajority win during re-arbitration, the disarmed loop after expiry, and the renewal-interval caps;
wording fixes.

Every mutant those rounds named is now killed by a test.

InMemoryLease
- checkAlive() compares against the record's expiry on the lease's clock (scheduler, else wall clock) and turns false at the deadline itself.
- A renewal that comes round after the deadline gives the lease up (onLost) instead of extending the lapsed record. InMemoryLeaseStore.renew(…, now) refuses a lapsed record; like tryAcquire since [Feature] No Clock contract — six ad-hoc time seams under three names, and 213 direct Date.now() reads the ManualScheduler cannot reach #1424, it takes the caller's now.
- The renewal loop is armed once. A re-acquire() on a held instance used to leave a second interval armed, and LeaseMajority does exactly that.
- The derived renewal interval is capped at half the TTL. The 100 ms floor alone would now lose any TTL ≤ 100 ms on its first tick.
KubernetesLease
- The TTL is measured from when the last write the API server accepted was sent (the renewTime stamp), on both performance.now() and the wall clock; either one running out ends it. The monotonic clock guards against NTP steps, the wall clock against a suspended host.
- A renewal tick past the deadline fires onLost and sends no PUT. The check sits ahead of the [Security] The renewal setInterval has no in-flight guard, so two PUTs carrying the same resourceVersion overlap under ordinary API latency and the holder CAS-conflicts with itself and fires onLost #761 in-flight guard, so a PUT hanging for up to operationTimeoutMs no longer delays the report.
- A renewal answer (success, 409 re-read or error) is only acted on while the lease it renewed is still the current one. A late answer after the lease was given up and re-acquired on the same instance can no longer install a stale resourceVersion and an earlier deadline, or report the new lease as lost.
- The derived renewal interval is capped at half the TTL, as above.
- The two bare new Date() reads moved onto the same reading. The wall-clock ratchet ledger drops from 6 to 4, and InMemoryLease's from 4 to its actual count of 1.
LeaseMajority: a cached won arbitration is returned only while lease.checkAlive() holds; otherwise it is dropped and arbitrated afresh. The fresh acquire loses if the other side holds the lease and wins it back if nobody took it. Lost arbitrations are not re-asked. The Lease contract JSDoc now says what checkAlive() must mean.
Docs (EN + DE) + CHANGELOG
- Pages: lease API, in-memory lease, overview, Kubernetes lease, downing strategies (new section "When a win outlives its lease"), and the renewal default in reference.conf / configuration reference.
- CHANGELOG: Security, Changed and Fixed entries.

Notes for review

Acceptance criterion 5 was already met before this PR. Re-enabling tests/multi-node/LeaseMajority.test.ts happened via [Feature] Split-brain stability window — stable-after, down-all-when-unstable, down-removal-margin #839; the real cause there was staggered failure detection, not renewal starvation. The test runs and passes here.
Behaviour change for onLost consumers. The singleton manager, the sharding coordinator and persistence fencing now get onLost after a stall longer than the TTL. Before, the lapsed lease was silently extended. Persistence fencing does not re-acquire, so it stays in observer mode, as after any other loss.
Custom Lease implementations. LeaseMajority now calls checkAlive(). An implementation that returns a constant false never keeps a win. This is documented under Changed.
Deliberately out of scope (follow-up issues: [Security] KubernetesLease.release() deletes the Lease object unconditionally, so a holder stepping down on a stale held flag can delete its successor's record #1659, [Security] An applied LeaseMajority win is never re-checked, so a winner that stalls past the TTL afterwards survives beside the side that took the lease #1660, [Bug] KubernetesLease stamps an acquire before its GET, so a slow GET shortens the first lease period — and one slower than the TTL yields a win checkAlive() denies at once #1661):
- A decision applied before the stall. → [Security] An applied LeaseMajority win is never re-checked, so a winner that stalls past the TTL afterwards survives beside the side that took the lease #1660. If LeaseMajority already acted on its win before the holder stalled, the winner has tombstoned the other side and no longer asks the strategy. If that side arbitrates after the lease expired, both sides survive.
- onLost-only consumers keep working after a stall until their next renewal tick.
- No clock-skew margin between pods for KubernetesLease. A client-go-style renew deadline would need a new option.
- An acquire slower than the TTL can never produce a usable win. LeaseMajority keeps waiting, which is safe; this is documented.
- KubernetesLease.release() deletes the Lease object unconditionally (pre-existing) → [Security] KubernetesLease.release() deletes the Lease object unconditionally, so a holder stepping down on a stale held flag can delete its successor's record #1659. A consumer that steps down while its held flag is stale, after a stall but before the renewal tick reported the loss, can delete a successor's record. InMemoryLease.release() checks the owner. A fix would skip the DELETE once checkAlive() is false, or make it conditional on the resourceVersion.
- Pre-existing [Security] The renewal setInterval has no in-flight guard, so two PUTs carrying the same resourceVersion overlap under ordinary API latency and the holder CAS-conflicts with itself and fires onLost #761 in-flight guard (liveness only). After a give-up, a renewal PUT still on the wire delays the first renewal of a lease re-acquired on the same instance until that PUT settles.

Verification

bun run typecheck and bun run typecheck:dev are green on every one of the 12 commits.
Full bun test at HEAD: 13 121 pass, 18 skip, 0 fail.
Coverage gate: 94.82 % overall (src/cluster/ 97.83 %, src/persistence/ 96.16 %), measured at commit 4.
New tests against develop: with HEAD's changed test files on develop code, 26 fail. They are every [Security] Lease.checkAlive() returns a cached boolean instead of comparing against expiresAt, and has no callers, so two nodes can both believe they hold the lease after an event-loop stall #937 case plus the ratchet ledger.
Multi-node: LeaseMajority, DowningStabilityWindow and ShardingLeaseSplitBrain pass.
Flake probe: --rerun-each 10 over the changed lease test files gave 1 150 pass and 0 fail, both idle and under load (32 CPU burners, load average about 25).
bun run test:examples: all 70 runnable examples pass.
check:doc-samples is red, but only on pages and sections this PR does not touch; none of its errors fall in the edited regions.

Review loop

Stage 1, independent verification (Opus 5.5), each round a fresh agent that runs every gate plus a mutation sweep over the changed logic:
- Rounds 1–4: FAIL. Each time, test gaps only; none found a production defect after round 1's late-answer guard. Each round's gaps were closed in commits 5–12.
- Round 5: PASS at 0b22f73d. 83 mutants, 66 killed; the 17 survivors were each argued equivalent or benign.
Stage 2, PR review (Fable 5.1): PASS at 0b22f73d. No blocker or should-fix findings. The reviewer re-ran the typechecks, the changed and consumer suites, the multi-node suites and an 18-mutant sweep (17 killed; the survivor is the benign "stamp after the GET" variant).

Nits from the review, not applied (kept out so both stages judge the same head):

KubernetesLease.test.ts "two holders never both report alive" depends on real time. Its final re-acquire assertion would flip only after a > 400 ms pause between two awaits (4 × the stalled holder's 100 ms TTL). That is theoretical, but a larger TTL there, or asserting on the record instead, would remove it.
Ratchet: the two renewTime stamps are genuinely wall-clock values. Routing them through systemClock takes them off the WallClockRatchet ledger (6 → 4) without making them injectable. Either keep Date.now() there and the ledger at 6 with a "deliberately wall-clock" note, or keep the alias. Maintainer's call.
Follow-up (pre-existing) → [Bug] KubernetesLease stamps an acquire before its GET, so a slow GET shortens the first lease period — and one slower than the TTL yields a win checkAlive() denies at once #1661: stamping the acquire after the GET, just before the CREATE/PUT, would reclaim the GET's latency from the first lease period while keeping the shared-stamp invariant.

CI: 61 checks pass. The 3 red checks are unrelated to this PR:

package-health (×2): bun audit reports new high advisories for brace-expansion, pulled in via @fastify/static › glob › minimatch. The same workflow is already red on develop at this PR's base 0785f82d.
MinIO (s3): the Docker image pull fails with unauthorized.

Optional nits from the last round, not applied (kept out so both stages judge the same head):

A deterministic unit pin for "lose() clears held". Today only a wall-clock integration test covers it.
overview.mdx should say the TTL counts from when the last accepted write was sent.
The EtcdLease sample measures its deadline with Date.now() only.
The commit scope (coordination, cluster/downing) has a space after the comma.

🤖 Generated with Claude Code

checkAlive() returned the held flag, and only a renewal that noticed the loss ever cleared it. A holder whose event loop stalled past the TTL - a GC pause, a starved container - never ran that renewal, so it went on answering true while another owner legitimately took the record (#937). It now compares against the expiry of the record it last wrote, on the lease's own clock, and turns false at the deadline itself: the instant the store lets another owner in. The renewal that finally runs after such a stall used to extend the lapsed record whenever nobody had taken it yet. It now gives the lease up and fires onLost instead, and InMemoryLeaseStore.renew() refuses a lapsed record outright; like tryAcquire since #1424, it takes the caller's now. Two consequences in the same file: - The derived renewal interval is capped below half the TTL. Its 100 ms floor alone renewed a TTL of 100 ms or less after the record lapsed, which only worked while a late renewal extended it. - startRenewalLoop() arms once. A re-acquire on a lease the instance already holds overwrote the timer handle and left the first interval renewing forever, past release(). LeaseMajority re-acquires exactly like that, since it never releases a lease it won. Two wall-clock tests that a loaded runner could now push past their TTL move to virtual time, or to a TTL no stall crosses. Refs #937 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…d write checkAlive() returned the held flag here too (#937). It now measures the TTL from when the last write the API server accepted was sent - the instant its renewTime was stamped - so the local deadline is never later than the one other pods judge the record by. The TTL is read off two clocks and runs out when either says so. performance.now() cannot be stepped back by NTP, which would stretch it; the wall clock still counts the time a suspended host lost, which the monotonic clock does not. The scheduler keeps pacing only the renewals (#1424). A renewal tick past the deadline fires onLost and sends no PUT. The check sits ahead of the in-flight guard (#761): behind it, a renewal PUT hanging for up to operationTimeoutMs held the report back for as long. A successful renewal is adopted only into the lease snapshot it renewed, so a late answer after release + re-acquire cannot put back a stale resourceVersion and an earlier deadline. The derived renewal interval is capped below half the TTL, as for InMemoryLease. The renewTime stamps now come from that same reading, which takes two bare `new Date()` reads off the wall-clock ratchet; its ledger drops to the counts both lease files actually have. Refs #937 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

LeaseMajority cached the decision its lease acquire produced and returned it for as long as its partition fingerprint held. Cluster asks every failure-detector tick once the stability window opens, and its view moves on changes this fingerprint ignores, so a win could be replayed long after it was computed - by a holder that had stalled past the TTL in between and lost the lease to the other side, which was by then downing this one. Both halves acted as the survivor (#937). A cached win is now returned only while lease.checkAlive() holds; otherwise it is dropped and arbitrated afresh. The fresh acquire loses if the other side holds the lease and wins it back if nobody took it. A lost arbitration is not re-asked: it does not rest on the lease. The Lease contract now says what checkAlive() has to mean for that to work, and the FakeLease double in DowningStrategies stops answering a constant false. Refs #937 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

The lease pages described checkAlive() as a flag at most one missed renewal stale, and said the shard coordinator called it before each allocation. Neither was true, and since #937 the first is not what the backends do either. The pages now say what it answers - held, and the TTL not run out since the last accepted write was sent - when onLost fires for a TTL that ran out, and what a custom backend has to get right for LeaseMajority to rely on it. The custom-backend sample compares against a deadline instead of returning a flag. downing-strategies gains a section on a win that outlives its lease, beside the one on abandoned acquires. The in-memory page drops the claim that there is no scheduler hook (#1424 added one), and the renewal default is stated as it now is, in reference.conf and in both configuration references. English and German throughout. CHANGELOG: Security, Changed and Fixed entries for #937. Closes #937 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…he lease it renewed The deadline check in renewOnce gives the lease up while a renewal PUT may still be on the wire, on purpose (#937). A consumer that re-acquires on the same instance, as ClusterSingletonManager does, then had that old PUT's answer land on the new lease: a late success put back a stale resourceVersion and the old send time as the deadline, and a late failure fired onLost for a lease that was fine. renewalPass and the 409 re-read now act only while the lease they renewed is still the current one - the same identity rule the success branch already had, now on every outcome. Independent verification found the healthy half of the deadline untested: deleting the line that moves it after an accepted renewal broke nothing. A second describe block drives both of the deadline's clocks by hand next to the ManualScheduler cadence and pins what moves it and what does not - three TTLs of healthy renewals, the send time rather than the answer time, a 409 re-read leaving it alone, the derived interval under the 500 ms floor - plus the late answers above, landed and failed. Refs #937 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

checkAlive() is `held && now < expiresAt`, and nothing tested the first half: without it a holder that released would go on reporting alive until its old expiry while any other owner may take the record at once. The two LeaseMajority cases added for #937 spell their locals out as `strategy`, as AGENTS.md asks of new code. Refs #937 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Not below it: Math.min(..., floor(ttl / 2)) allows exactly half, which is what the docs and reference.conf already say. Refs #937 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…uard A second verification round found two KubernetesLease rules that no test held: - The acquire path counts its deadline from when the CREATE or takeover PUT was sent, like a renewal does. Taking it from the answer instead kept a holder alive for the length of the round trip past the moment another pod, reading the record's renewTime, could take the lease - two holders alive at once, with the suite green. - The 409 re-read only speaks for the lease it re-read. An answer that straddles a give-up and re-acquire on the same instance would report the new lease lost. The held-back client now holds the next request of any method, and both rules have a case. The frozen-clock block also restores every mock after each case, as a net under the per-case finally: with performance.now() frozen, an awaitCondition there could never time out. Refs #937 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…ed timer accurately LeaseOptions, ConfigKeys and the KubernetesLease lifecycle comment still gave the derived renewal interval as max(500ms, ttl/3) or ttl/3; it is a third of the TTL with each backend's floor, capped at half the TTL. The leaked InMemoryLease timer did not renew past release(): it fired on nothing there - while keeping a real-timer process alive - and renewed again beside the second loop after the next acquire. The comment and the CHANGELOG entry now say so. Refs #937 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…d loop, the cap and the shared stamp A third verification round found four rules that no test held: - LeaseMajority keeps a dropped win dropped on every tick while the re-arbitration acquire is on the wire. Deleting `decision = null` let the stale "down them" set come back on the next tick, which Cluster applies at once - #937's both-survivors shape, for the length of the acquire. - An InMemoryLease lost to expiry disarms its renewal loop. release() returns early once the lease is lost, so it would not stop an interval left running. - InMemoryLease's derived interval is capped at half the TTL: the first renewal of a 90 ms lease lands at 45 ms. - A KubernetesLease acquire's deadline is the renewTime it wrote plus the TTL, however late the GET before it was answered. When the stamp is taken matters less than that the record and the deadline share it. Refs #937 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

lease-api told readers to stop work "the moment ownership goes" through onLost, which now fires at the first renewal after the deadline - up to one renewal interval after checkAlive() turned false. It now says which answers when. On the Kubernetes page the new checkAlive paragraph sat between "onLost fires when" and "It does not fire", so the pronoun read as checkAlive; it moves below, and the sentence names onLost. The KubernetesLease lifecycle comment states the derived interval the way every other place does. Refs #937 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…d the K8s cap A fourth verification round found two safety rules no test held: - A KubernetesLease renewal writes the same instant into the record's renewTime that its deadline counts from. Writing an earlier one - the previous accepted write's stamp, say - left the local deadline outliving the record as other pods judge it, by up to a renewal interval: two holders alive after a stall, with the suite green. - A lost (re-)acquire leaves the deadline where it was, in both backends. A stalled holder whose held flag is still set - the state LeaseMajority re-arbitrates from - loses the re-acquire to the new owner; recording the deadline before knowing it had won made it report alive for a full TTL beside that owner. The KubernetesLease derived interval is also pinned at half the TTL: no renewal before 200 ms of a 400 ms lease, one at 200 ms. Refs #937 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

pathosDev

Thanks — this is a careful fix. I re-verified it independently before merging; findings below.

Verification

#937's diagnosis holds on develop: checkAlive() returned the cached held flag in both backends with no caller in src/, and LeaseMajority replayed its cached decision unchecked. As you note, acceptance criterion 5 was already met via #839; the issue's guess that the multi-node flake came from renewal-timer starvation was wrong.
The new tests discriminate. I ran the changed test files against develop's source: exactly the 26 you name fail (every #937 case plus the ratchet ledger), and all pass on the PR head.
All five acceptance criteria are met, each pinned by a test:
- LeaseClock.test.ts and the #937 blocks in KubernetesLease.test.ts;
- the store refusing a lapsed renew;
- the three LeaseMajority cases in DowningStrategies.test.ts;
- the multi-node suite.
Integrated with the current local develop (well ahead of origin/develop): only CHANGELOG.md conflicts, three insert-insert hunks, resolved as a union. On the merge result everything is green:
- typecheck and typecheck:dev;
- bun test (14 755 pass, 0 fail);
- coverage gate 94.97 % (cluster 97.98 %, persistence 96.39 %);
- the LeaseMajority, ShardingLeaseSplitBrain and DowningStabilityWindow multi-node suites;
- all 70 runnable examples;
- lint:audit.
The red package-health check is already fixed on develop by the lockfile refresh. MinIO (s3) is the known registry 401. Neither is caused by this PR.

Follow-ups I'm adding on top (three commits)

BREAKING marker. LeaseMajority now depends on checkAlive(), which tightens the contract for custom Lease implementations. Pre-1.0 policy wants that flagged, so the Changed entry now carries BREAKING and a one-line migration note.
Wall-clock ratchet. The wall half of instantNow() is the renewTime other pods judge the record by, so it is genuinely wall-clock. WallClockRatchet.test.ts's header says such values stay on the ledger rather than moving behind systemClock, which takes them off the count without making them injectable. It now reads Date.now() with a comment saying why, and the ledger for KubernetesLease.ts is 5: one below the original 6, because the two new Date() stamps really did collapse into one read. InMemoryLease's 4 → 1 is a genuine move onto the injectable clock and stays.
Stale wording. A few places still described the derived renewal interval as "ttl/3":
- LeaseOptions.ts (field and builder JSDoc);
- the overview.mdx sample comment (EN + DE);
- a comment in LeaseConfigDefaults.test.ts.
They now match the reference.conf wording.

The commit-scope nit (coordination, cluster/downing with a space) stays as is; it is not worth rewriting published history.

The follow-up issues check out too: I commented on #1660. #1659 and #1661 are accurate as filed.

Nevs08 and others added 12 commits October 6, 2026 16:35

docs(changelog): the derived renewal interval is capped at half the TTL

66a9438

Not below it: Math.min(..., floor(ttl / 2)) allows exactly half, which is what the docs and reference.conf already say. Refs #937 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Nevs08 requested a review from pathosDev October 6, 2026 20:40

Nevs08 assigned pathosDev Oct 6, 2026

pathosDev reviewed Oct 7, 2026

View reviewed changes

Navigation Menu

Repository navigation

fix(coordination): make Lease.checkAlive() honour the TTL, re-validate LeaseMajority wins (#937) - #1658

fix(coordination): make Lease.checkAlive() honour the TTL, re-validate LeaseMajority wins (#937)#1658
Nevs08 wants to merge 12 commits into
developfrom
features/fix-lease-check-alive

Nevs08 commented Oct 6, 2026 •

edited

Loading

Uh oh!

pathosDev left a comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

Nevs08 commented Oct 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes (every commit green on its own)

Notes for review

Verification

Review loop

Uh oh!

pathosDev left a comment

Choose a reason for hiding this comment

Verification

Follow-ups I'm adding on top (three commits)

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Nevs08 commented Oct 6, 2026 •

edited

Loading