fix(leader): reject lease timings that make split-brain deterministic

.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

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 {

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<T>(_script: string, _keys: string[], args: (string | number)[]): Promise<T> {
+      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();
+  });
+});