From 7418e9a08539cc6c1e841222917034bc4c22a2a7 Mon Sep 17 00:00:00 2001 From: dcccrypto Date: Sat, 18 Jul 2026 21:27:27 +0100 Subject: [PATCH] fix(leader): reject lease timings that make split-brain deterministic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit LeaderLock accepted any timing values. When renewMs >= ttlMs the Redis lease expires BEFORE the leader first attempts renewal, so a standby acquires the freed key and promotes itself while the original node still reports role() === "leader" — it does not learn otherwise until its renewal finally runs and the fencing script rejects it. keeperSend gates on-chain writes on that local role alone (index.ts setLeaderCheck), so in that interval both processes submit transactions. This defeats the single-writer guarantee the lease exists to provide, and it is deterministic rather than a race. index.ts builds these from Number(process.env.X ?? default), so a malformed value arrives as NaN. NaN fails every comparison silently: setTimeout(fn, NaN) fires on the next tick, turning renewal into a hot loop, and ex: NaN yields a lease Redis will not honour. Validate in the constructor and throw LeaderLockTimingError on: - any of ttlMs / renewMs / pollMs non-finite or <= 0 - renewMs >= ttlMs Failing to boot is the only safe outcome here: a lock built on unsafe timings cannot provide the guarantee its callers assume, and the alternative is a silently split-brained keeper double-submitting liquidations. Defaults (30s/10s/5s) are unaffected — only an explicit override can trip this. A renew margin thinner than half the TTL is legal but leaves little room for a slow Redis round-trip or an event-loop stall, so it warns rather than throws. That is a judgement call, not an invariant. Also document the constraint in .env.example next to the three vars. Closes #377 Co-Authored-By: Claude Opus 4.8 --- .env.example | 7 ++ src/lib/leader.ts | 79 +++++++++++- tests/lib/leader-timing-validation.test.ts | 139 +++++++++++++++++++++ 3 files changed, 222 insertions(+), 3 deletions(-) create mode 100644 tests/lib/leader-timing-validation.test.ts diff --git a/.env.example b/.env.example index 29371fb..7a9bb76 100644 --- a/.env.example +++ b/.env.example @@ -91,6 +91,13 @@ USE_HELIUS_SENDER=true # NETWORK=mainnet # KEEPER_REDIS_URL=https://your-upstash-rest-url.upstash.io # KEEPER_REDIS_TOKEN= # required when KEEPER_REDIS_URL is set (no empty fallback) +# Lease timings. All three must be finite and > 0, and RENEW_MS must be +# strictly less than TTL_MS — otherwise the lease expires before the leader +# renews it, a standby promotes while the old leader still believes it holds +# the lock, and both submit on-chain transactions (split-brain). The keeper +# refuses to boot on a violation rather than run split-brained (#377). +# Keep RENEW_MS at or below half of TTL_MS so a slow Redis round-trip or an +# event-loop stall cannot let the lease lapse between renewals. # KEEPER_LEADER_LOCK_TTL_MS=30000 # KEEPER_LEADER_LOCK_RENEW_MS=10000 # KEEPER_STANDBY_POLL_MS=5000 diff --git a/src/lib/leader.ts b/src/lib/leader.ts index 26ac00f..1f248ba 100644 --- a/src/lib/leader.ts +++ b/src/lib/leader.ts @@ -20,6 +20,58 @@ end export type LeaderRole = "leader" | "standby" | "starting"; +/** + * #377: thrown when lease timings would make the single-writer guarantee + * unenforceable. Constructing a LeaderLock is the last point at which this is + * still cheap to catch — past it the misconfiguration is silent, and its only + * symptom is two nodes writing on-chain at once. + */ +export class LeaderLockTimingError extends Error { + constructor(message: string) { + super(message); + this.name = "LeaderLockTimingError"; + } +} + +/** + * #377: the lock's correctness rests on timings nothing previously checked. + * `Number(process.env.X ?? default)` in index.ts yields NaN for any malformed + * value, and NaN fails every comparison silently: `setTimeout(fn, NaN)` fires + * on the next tick, turning renewal into a hot loop, while `ex: NaN` produces a + * lease Redis will not honour. + * + * The ordering invariant matters more than the individual bounds. If + * renewMs >= ttlMs the lease expires BEFORE the leader ever tries to renew, so + * a standby acquires the freed key and promotes itself while the original node + * still reports role() === "leader" — it does not learn otherwise until its + * renewal finally runs and the fencing script rejects it. keeperSend gates + * writes on that local role alone, so both nodes submit transactions in the + * interval. Split-brain is then deterministic, not a race. + */ +function validateTiming(ttlMs: number, renewMs: number, pollMs: number): void { + for (const [name, value] of [ + ["ttlMs", ttlMs], + ["renewMs", renewMs], + ["pollMs", pollMs], + ] as const) { + if (!Number.isFinite(value) || value <= 0) { + throw new LeaderLockTimingError( + `LeaderLock ${name} must be a finite positive number of milliseconds, got ${String(value)}. ` + + `Check KEEPER_LEADER_LOCK_TTL_MS / KEEPER_LEADER_LOCK_RENEW_MS / KEEPER_STANDBY_POLL_MS.`, + ); + } + } + + if (renewMs >= ttlMs) { + throw new LeaderLockTimingError( + `LeaderLock renewMs (${renewMs}) must be strictly less than ttlMs (${ttlMs}) — otherwise the ` + + `lease expires before the leader renews it and a standby can promote while this node still ` + + `reports itself leader, allowing both to submit on-chain transactions (split-brain). ` + + `Set KEEPER_LEADER_LOCK_RENEW_MS below KEEPER_LEADER_LOCK_TTL_MS (recommended: at most half).`, + ); + } +} + export interface LeaderLockOptions { ttlMs?: number; renewMs?: number; @@ -56,11 +108,32 @@ export class LeaderLock { private _stopped = false; constructor(redis: RedisLike, identity: string, opts: LeaderLockOptions = {}) { + const ttlMs = opts.ttlMs ?? 30_000; + const renewMs = opts.renewMs ?? 10_000; + const pollMs = opts.pollMs ?? 5_000; + + // #377: fail fast at construction. A lock built on unsafe timings cannot + // provide the guarantee its callers assume, so refusing to boot is the only + // safe outcome — the alternative is a silently split-brained keeper. + validateTiming(ttlMs, renewMs, pollMs); + + // Renewal must also survive being LATE. A renewal firing at renewMs still + // has to complete a Redis round-trip before ttlMs, so a margin thinner than + // half the TTL leaves little room for a slow round-trip or event-loop stall. + // This is a judgement call, not an invariant, so it warns rather than throws. + if (renewMs > ttlMs / 2) { + logger.warn("LeaderLock renew margin is thin — a slow Redis round-trip or event-loop stall could let the lease lapse", { + ttlMs, + renewMs, + recommendedMaxRenewMs: ttlMs / 2, + }); + } + this.redis = redis; this.identity = identity; - this.ttlMs = opts.ttlMs ?? 30_000; - this.renewMs = opts.renewMs ?? 10_000; - this.pollMs = opts.pollMs ?? 5_000; + this.ttlMs = ttlMs; + this.renewMs = renewMs; + this.pollMs = pollMs; } role(): LeaderRole { diff --git a/tests/lib/leader-timing-validation.test.ts b/tests/lib/leader-timing-validation.test.ts new file mode 100644 index 0000000..56249f3 --- /dev/null +++ b/tests/lib/leader-timing-validation.test.ts @@ -0,0 +1,139 @@ +/** + * Regression tests for #377 — invalid HA lease timings create a deterministic + * split-brain leader window. + * + * LeaderLock accepted any timing values. With renewMs >= ttlMs the Redis lease + * expires before the leader attempts renewal, so a standby acquires the freed + * key and promotes itself while the original node still reports + * role() === "leader". keeperSend gates on-chain writes on that local role + * alone, so both nodes submit transactions in the interval. + * + * The first test is the issue's proof-of-concept, inverted: it drives the exact + * timing that produced two concurrent leaders and asserts the lock now refuses + * to be constructed at all, so the unsafe state is unreachable. + */ + +import { describe, it, expect, vi, afterEach } from "vitest"; +import { LeaderLock, LeaderLockTimingError } from "../../src/lib/leader.js"; +import type { RedisLike } from "../../src/lib/redis-client.js"; + +vi.mock("@percolatorct/shared", () => ({ + createLogger: () => ({ info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }), +})); + +/** Redis-compatible in-memory lease WITH expiry semantics (the PoC's harness). */ +function makeExpiringRedis(): RedisLike { + let value: string | null = null; + let expiresAt = 0; + const live = () => { + if (value !== null && Date.now() >= expiresAt) value = null; + return value; + }; + return { + async set(_key: string, next: string, opts: { ex: number; nx?: true }) { + if ("nx" in opts && opts.nx === true && live() !== null) return null; + value = next; + expiresAt = Date.now() + opts.ex * 1000; + return "OK" as const; + }, + async get() { + return live(); + }, + async del() { + value = null; + return 1; + }, + async eval(_script: string, _keys: string[], args: (string | number)[]): Promise { + if (live() !== args[0]) return 0 as T; + expiresAt = Date.now() + Number(args[1]); + return 1 as T; + }, + }; +} + +const VALID = { ttlMs: 30_000, renewMs: 10_000, pollMs: 5_000 }; + +afterEach(() => { + vi.useRealTimers(); +}); + +describe("#377 LeaderLock lease-timing validation", () => { + it("refuses the exact configuration from the issue PoC (renewMs > ttlMs)", async () => { + vi.useFakeTimers(); + const redis = makeExpiringRedis(); + const bad = { ttlMs: 1_000, renewMs: 5_000, pollMs: 100 }; + + // Previously both nodes could be constructed and started, and after the + // 1s lease lapsed BOTH reported "leader". Construction now fails first. + expect(() => new LeaderLock(redis, "node-a", bad)).toThrow(LeaderLockTimingError); + expect(() => new LeaderLock(redis, "node-b", bad)).toThrow(/split-brain/); + }); + + it("still elects exactly one leader under a valid configuration", async () => { + vi.useFakeTimers(); + const redis = makeExpiringRedis(); + const a = new LeaderLock(redis, "node-a", VALID); + const b = new LeaderLock(redis, "node-b", VALID); + const noop = { network: "devnet", onPromote() {}, onDemote() {} }; + + a.start(noop); + b.start(noop); + await vi.advanceTimersByTimeAsync(0); + + expect(a.role()).toBe("leader"); + expect(b.role()).toBe("standby"); + + // Past the point where the bad config split-brained, and past a renewal. + await vi.advanceTimersByTimeAsync(11_000); + expect(a.role()).toBe("leader"); + expect(b.role()).toBe("standby"); + + await a.stop(); + await b.stop(); + }); + + it("rejects renewMs exactly equal to ttlMs (the boundary)", () => { + const redis = makeExpiringRedis(); + expect(() => new LeaderLock(redis, "n", { ...VALID, ttlMs: 10_000, renewMs: 10_000 })).toThrow( + LeaderLockTimingError, + ); + }); + + it("accepts renewMs just below ttlMs", () => { + const redis = makeExpiringRedis(); + expect(() => new LeaderLock(redis, "n", { ttlMs: 10_000, renewMs: 9_999, pollMs: 1_000 })).not.toThrow(); + }); + + // index.ts builds these with Number(process.env.X ?? default), so any + // malformed env value arrives here as NaN rather than being rejected upstream. + it.each([ + ["NaN", NaN], + ["Infinity", Infinity], + ["zero", 0], + ["negative", -1], + ])("rejects %s for each timing field", (_label, bad) => { + const redis = makeExpiringRedis(); + expect(() => new LeaderLock(redis, "n", { ...VALID, ttlMs: bad })).toThrow(LeaderLockTimingError); + expect(() => new LeaderLock(redis, "n", { ...VALID, renewMs: bad })).toThrow(LeaderLockTimingError); + expect(() => new LeaderLock(redis, "n", { ...VALID, pollMs: bad })).toThrow(LeaderLockTimingError); + }); + + it("names the offending field so an operator can fix the right env var", () => { + const redis = makeExpiringRedis(); + expect(() => new LeaderLock(redis, "n", { ...VALID, pollMs: NaN })).toThrow(/pollMs/); + expect(() => new LeaderLock(redis, "n", { ...VALID, ttlMs: NaN })).toThrow(/ttlMs/); + }); + + it("still applies its defaults when no options are supplied", () => { + const redis = makeExpiringRedis(); + expect(() => new LeaderLock(redis, "n")).not.toThrow(); + expect(() => new LeaderLock(redis, "n", {})).not.toThrow(); + }); + + it("accepts a thin renew margin but does not silently endorse it", () => { + const redis = makeExpiringRedis(); + // 0.6 * ttl — legal (renew < ttl) but leaves little room for a slow + // round-trip, so it is warned about rather than rejected. + expect(() => new LeaderLock(redis, "n", { ttlMs: 10_000, renewMs: 6_000, pollMs: 1_000 })).not.toThrow(); + }); +});