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

CHANGELOG.md

@@ -2852,6 +2852,20 @@ breaking.  See `ROADMAP.md` for what's coming, and `README.md` →
 
 ### Changed
 
+- **`LeaseMajority` relies on `checkAlive()`, and the derived lease renewal
+  interval is capped at half the TTL** (#937).  `LeaseMajority` re-validates
+  a won arbitration with `lease.checkAlive()` before returning it again, so
+  a custom `Lease` has to answer it against its TTL: `true` while held and
+  the TTL has not run out since the last accepted write was sent.  One that
+  answers a constant `false` never keeps a win; one that answers a held flag
+  keeps working, without the stall protection.
+
+  An unset `renewalIntervalMs` was a third of the TTL with a floor — 100 ms
+  for `InMemoryLease`, 500 ms for `KubernetesLease` — and the floor alone
+  renewed a TTL at or below it only after the record had lapsed.  That now
+  gives the lease up, so the derived interval is capped at half the TTL.  An
+  interval set explicitly is taken as given.
+
 - **Every broker image is pinned to a release, every runner image to a
   digest, and Dependabot watches both — and the .NET benchmark arms**
   (#1597). Sixteen of the seventeen `image:` lines in the integration compose
@@ -3508,6 +3522,14 @@ breaking.  See `ROADMAP.md` for what's coming, and `README.md` →
 
 ### Fixed
 
+- **`InMemoryLease` arms one renewal loop, however often it is acquired**
+  (#937).  A re-`acquire()` on a lease the instance already held overwrote
+  the timer handle and left the first interval armed for good: past
+  `release()` it fired on nothing but kept a real-timer process alive, and
+  after the next acquire it renewed again beside the second loop.
+  `LeaseMajority` re-acquires exactly like that, since it never releases a
+  lease it won.
+
 - **The multi-node integration run went red the moment a node exited
   cleanly — which, since #1567, scenario 13's victim does** (#1594).
   `bun run test:integration` ran compose with `--abort-on-container-exit`,
@@ -4792,6 +4814,37 @@ breaking.  See `ROADMAP.md` for what's coming, and `README.md` →
 
 ### Security
 
+- **A lease holder that stalled past its TTL no longer goes on believing it
+  holds the lease** (#937).  `checkAlive()` answered from a cached flag in
+  both `InMemoryLease` and `KubernetesLease`, and only a renewal that noticed
+  the loss ever cleared it.  A holder whose event loop stalled longer than
+  the TTL — a GC pause, a starved container — never ran that renewal, so it
+  still answered `true` while another node had legitimately taken the lease,
+  and the renewal that finally ran extended the lapsed record whenever nobody
+  had claimed it yet.  `LeaseMajority`, which caches the decision its lease
+  acquire produced, went on replaying a win the other side now held, and both
+  halves of a split acted as the survivor.
+
+  `checkAlive()` now compares against the deadline of the last write the
+  backend accepted, measured from when that write was sent, and turns
+  `false` at the deadline itself.  `KubernetesLease` judges it on two clocks
+  and lets either one end it: `performance.now()`, which NTP cannot step
+  back, and the wall clock, which still counts the time a suspended host
+  lost.  A renewal tick past the deadline fires `onLost` and extends nothing;
+  `KubernetesLease` checks ahead of its in-flight guard, so a renewal PUT
+  hanging for up to `operationTimeoutMs` no longer holds the report back.
+  `LeaseMajority` returns a cached win only while `checkAlive()` holds and
+  otherwise arbitrates again — it loses if the other side has the lease, and
+  wins it back if nobody took it.
+
+  Consumers that react to `onLost` — the singleton manager, the sharding
+  coordinator, persistence fencing — now hear about such a stall at the
+  first renewal after it, where the lapsed lease used to be extended
+  silently.  Fencing does not re-acquire, so a `PersistentActor` stays in
+  observer mode afterwards, as it already did after any other loss.
+  `InMemoryLeaseStore.renew` takes the caller's `now`, as `tryAcquire` has
+  since #1424, and refuses a lapsed record.
+
 - **The DevTools UI toolchain is installed frozen in CI, and the
   frozen-install guard reads through `bun run <script>`** (#1622).  Three
   workflows installed the nested Angular toolchain as `bun run ui:install`,

docs/src/content/docs/cluster/downing-strategies.mdx

@@ -464,6 +464,30 @@ rules cover both:
 The visible symptom of a lease provider having a bad day is
 therefore a cluster that waits, not one that splits.
 
+### When a win outlives its lease
+
+A win is only as good as the lease behind it.  Once the view has
+been stable for `stable-after`, the cluster asks the strategy again
+on every failure-detector tick, so a cached win can be returned
+long after the acquire that produced it.  A node that stalled past
+the lease's TTL in between — a long GC pause, a starved container
+— has lost the lease to the other side, which is by then downing
+*this* one.  Replaying the old win is how both halves end up acting
+as the survivor.
+
+So before a won decision is returned again, the strategy asks
+`lease.checkAlive()`.  If the lease has run out, the win is dropped
+and the side arbitrates afresh: the new acquire loses if the other
+side holds the lease now — and this side then downs itself — and
+wins it back if nobody took it.  A lost arbitration is not re-asked;
+it does not rest on the lease.  This relies on the backend answering
+`checkAlive()` against its TTL rather than from a held flag, which
+both built-in backends do (#937).
+
+An acquire that takes longer than the lease's TTL can never produce
+a usable win: the strategy keeps arbitrating and decides nothing,
+the same posture as while an acquire is pending.
+
 ## Picking a strategy
 
 Three questions in order:

docs/src/content/docs/coordination/in-memory-lease.mdx

@@ -102,19 +102,28 @@ The implementation:
 
 - **`acquire()`** atomically CAS-es the registry slot.  Returns
   `true` if it claimed; `false` if held by another owner.
-- **TTL expiry** — if the holder doesn't renew within `ttlMs`,
-  the registry releases automatically (a setTimeout-driven
-  cleanup).
-- **`onLost`** fires if another holder takes over (via the TTL
-  expiry mechanism) or if the registry is forcibly cleared.
-- **Renewal** runs on a `setInterval` at `renewalIntervalMs`
-  (default `ttl / 3`).
-
-`InMemoryLease` uses real timers (`setInterval`) and exposes no
-scheduler-injection hook — there is no `ManualScheduler` to wire
-in.  For deterministic lease-timing tests, mock `Date.now()`; to
-reset shared lease state between tests, clear the exported
-`inMemoryLeaseStore` singleton directly (`inMemoryLeaseStore._clear()`).
+- **TTL expiry** is lazy — nothing runs at the deadline.  A record
+  past its expiry is simply free: the next `acquire()` by another
+  owner takes it, and `inMemoryLeaseStore.peek()` stops reporting
+  it.
+- **`checkAlive()`** compares against that same expiry, so a holder
+  whose renewal did not come round in time reads `false` from the
+  deadline on, before anything has told it so.
+- **Renewal** runs every `renewalIntervalMs` (default a third of
+  the TTL with a 100 ms floor, capped at half the TTL).  A renewal
+  that only comes round after the deadline gives the lease up and
+  fires `onLost` instead of extending the lapsed record.
+- **`onLost`** fires when a renewal finds the record gone, held by
+  another owner, or past its deadline — the last is what a holder
+  that stalled past its TTL sees.
+
+Timers run on `setInterval` by default.
+`LeaseOptions.withScheduler(...)` puts the TTL, the renewal cadence
+and the acquire retries on a scheduler instead — with a
+`ManualScheduler`, a test advances thirty seconds of TTL without
+waiting for them.  To reset shared lease state between tests, clear
+the exported `inMemoryLeaseStore` singleton directly
+(`inMemoryLeaseStore._clear()`).
 
 ## Example: a singleton test
 

docs/src/content/docs/coordination/kubernetes-lease.mdx

@@ -399,15 +399,24 @@ fires `onLost`: ownership that cannot be confirmed may not be assumed.
 
 - A renewal PUT is rejected **and** the re-read shows a different
   `holderIdentity`, or no lease object at all.
-- The framework observes the lease was modified by someone else
-  (a probe GET before some critical operation).
-- Network partition prevents renewal for longer than `ttlMs`.
-
-It does **not** fire on a CAS conflict whose re-read still names this
-owner.  That is the holder conflicting with one of its own writes —
+- A renewal tick finds `ttlMs` already gone by since the last write
+  the API server accepted was sent — the event loop stalled, a
+  network partition kept renewals out, or the previous renewal PUT
+  is still hanging on the wire.  No PUT is sent: another pod may
+  hold the lease by now (#937).
+- A renewal fails outright (network error, a credential the API
+  server keeps rejecting).
+
+`onLost` does **not** fire on a CAS conflict whose re-read still names
+this owner.  That is the holder conflicting with one of its own writes —
 another controller bumping the object, or a re-`acquire()` racing a
 renewal — and the record is still ours.
 
+`checkAlive()` turns `false` at the TTL deadline itself, without
+waiting for a tick.  The deadline is judged on two clocks and passes
+when either says so: the monotonic one, which NTP cannot step back,
+and the wall clock, which still counts the time a suspended host lost.
+
 The handler should drop ownership state immediately — see
 [Lease API](/coordination/lease-api/) for the contract.
 

docs/src/content/docs/coordination/lease-api.mdx

@@ -130,20 +130,28 @@ if (lease.checkAlive()) {
 ```
 
 A **synchronous, local** check.  No network roundtrip.  Returns
-the holder's most-recent knowledge of "do I still own this?"
-
-Used by the framework to gate ownership-dependent work — e.g.,
-before issuing a shard allocation, the coordinator calls
-`checkAlive()` and aborts if it returns `false`.
-
-Implementations track ownership locally; the backend's renewal
-loop updates the local flag.  This means `checkAlive()` reflects
-**up to one missed renewal of staleness** — a sub-second window
-where the lease might actually be gone but `checkAlive()` still
-returns `true`.
-
-For absolute certainty, use `onLost(...)` and react to the
-notification rather than polling.
+`true` while this process holds the lease **and** its TTL has not
+run out — measured from when the last write the backend accepted
+was *sent*, so the local deadline is never later than the one
+other holders judge the record by.
+
+The answer rests on that deadline, not on a flag.  A flag is only
+cleared by a renewal that notices the loss, and a stalled event
+loop — a GC pause, a starved container — is exactly what keeps
+that renewal from running, while another holder may legitimately
+take the lease in the meantime.  So `checkAlive()` turns `false` at
+the deadline itself, before `onLost` has had a chance to fire
+(#937).
+
+`LeaseMajority` re-validates a won arbitration with it before
+acting on that win again.  The singleton manager, the sharding
+coordinator and persistence fencing react to `onLost` instead.
+
+The two answer at different moments.  `checkAlive()` turns `false`
+at the deadline; `onLost(...)` fires at the first renewal after it,
+up to one renewal interval later.  Gate each piece of work with
+`checkAlive()`, and register `onLost(...)` to stop ongoing work
+rather than polling.
 
 ## `onLost(handler): () => void`
 
@@ -159,13 +167,16 @@ const unsubscribe = lease.onLost((reason) => {
 Register a callback fired when ownership is lost **unexpectedly**:
 
 - The backend reported the lease was taken over by another holder.
-- The TTL expired without successful renewal (e.g., network
-  partition).
+- The TTL ran out without a successful renewal (e.g., network
+  partition, or a stalled event loop).  The first renewal tick
+  after the deadline reports it, and gives the lease up rather
+  than extending the lapsed record — another holder may have it
+  by now.
 - The backend itself reported a state inconsistency.
 
-`onLost` fires **once per loss**.  After it fires, `checkAlive()`
-returns `false` and `acquire()` is needed before regaining
-ownership.
+`onLost` fires **once per loss**.  By then `checkAlive()` already
+returns `false` — on a TTL that ran out, from the deadline on — and
+`acquire()` is needed before regaining ownership.
 
 The handler should **drop ownership state immediately** — stop
 work, release locks, signal interested actors.  Don't await
@@ -210,24 +221,30 @@ flowchart TB
 Same pattern for sharding coordinator:
 
 1. **`lease.acquire`** before processing allocation requests.
-2. **`lease.checkAlive`** before issuing each allocation.
-3. **`onLost`** → reject pending allocations, stop coordinator.
+2. **Its own lease state** gates every allocation: the coordinator
+   acts only while it holds the lease, and `onLost` is what clears
+   that state.
+3. **`onLost`** → step down: cancel in-flight rebalances, drop
+   pending queries, then try to re-acquire while still leader.
 
 ## Writing a custom backend
 
 ```ts
 import type { Lease, LeaseOptionsType } from 'actor-ts/coordination';
 
 class EtcdLease implements Lease {
-  private alive = false;
+  private held = false;
+  /** When the last write etcd accepted was sent, plus the TTL. */
+  private expiresAt = 0;
   private onLostHandlers = new Set<(reason: string) => void>();
   private renewTimer: NodeJS.Timeout | null = null;
 
   constructor(private readonly settings: LeaseOptionsType & { /* etcd-specific */ }) {}
 
   async acquire(): Promise<boolean> {
-    // Try to atomically CAS the etcd key from empty to this owner.
-    // Start a renewal timer on success.
+    // Read the clock, then try to atomically CAS the etcd key from
+    // empty to this owner.  On success: held = true,
+    // expiresAt = that reading + ttlMs, start a renewal timer.
     // ...
   }
 
@@ -238,7 +255,10 @@ class EtcdLease implements Lease {
   }
 
   checkAlive(): boolean {
-    return this.alive;
+    // Never the flag alone: a stalled event loop keeps the renewal
+    // that would clear it from running, while another owner takes
+    // the key.
+    return this.held && Date.now() < this.expiresAt;
   }
 
   onLost(handler: (reason: string) => void): () => void {
@@ -247,24 +267,31 @@ class EtcdLease implements Lease {
   }
 
   private fireOnLost(reason: string): void {
-    this.alive = false;
+    this.held = false;
     for (const h of this.onLostHandlers) {
       try { h(reason); } catch { /* swallow */ }
     }
   }
 }
 ```
 
-Three things any backend needs to get right:
+Four things any backend needs to get right:
 
 1. **Atomicity on `acquire`** — two concurrent `acquire()` calls
    from different owners must produce one winner.  The backend's
    own consistency model has to provide this (CAS, paxos,
    raft-backed).
 2. **Periodic renewal** — keep the lease alive in the backend.
-   Configurable interval, typically `ttl / 3`.
+   Configurable interval, typically `ttl / 3`.  A renewal that only
+   comes round after the deadline gives the lease up rather than
+   extending the record: another owner may hold it by now.
 3. **`onLost` accuracy** — fire when ownership truly transitions
    away, including the TTL-expiry case.
+4. **`checkAlive` against the deadline** — compare against the
+   expiry of the last write the backend accepted, measured from
+   when it was sent.  A held flag alone stays `true` through
+   exactly the stall that lost the lease, and `LeaseMajority`
+   relies on the answer.
 
 Test the implementation against:
 
@@ -275,26 +302,31 @@ Test the implementation against:
 
 import { Aside } from '@astrojs/starlight/components';
 
-<Aside type="caution" title="`checkAlive` is approximate">
+<Aside type="caution" title="`checkAlive` gates; it doesn't prove">
   ```ts
   if (lease.checkAlive()) {
-    await doWork();   // could complete just after lease was lost
+    await doWork();   // could complete just after the deadline passed
   }
   ```
-  Use `checkAlive` to **gate** work, not as proof of ownership.
-  For irreversible actions (committing to an external service),
-  also pass through a backend roundtrip (acquire-already-held
-  is fast) or rely on `onLost` to stop new work.
+  `checkAlive` is right at the moment it answers, and nothing stops
+  the deadline passing while the work it gated runs.  Use it to
+  **gate** work, not as proof of ownership.  For irreversible
+  actions (committing to an external service), also pass through a
+  backend roundtrip (acquire-already-held is fast) or rely on
+  `onLost` to stop new work.
 </Aside>
 
 <Aside type="caution" title="Don't sleep through TTL">
   ```ts
-  // GC pause longer than ttlMs?  Lease silently lost.
+  // GC pause longer than ttlMs?  The lease is gone.
   ```
   TTL-based leases can't tell the difference between "holder
-  crashed" and "holder paused."  If your process can pause longer
-  than the TTL (large GC, suspended VM), set the TTL longer than
-  your worst-case pause.
+  crashed" and "holder paused."  A holder that pauses past its TTL
+  reads `checkAlive()` as `false` when it resumes, and its first
+  renewal reports the loss through `onLost` instead of extending
+  the lapsed record — another holder may have taken it meanwhile.
+  If your process can pause longer than the TTL (large GC,
+  suspended VM), set the TTL longer than your worst-case pause.
 </Aside>
 
 <Aside type="caution" title="`release` after `onLost` is harmless">