fix(leader): reject lease timings that make split-brain deterministic - #385
fix(leader): reject lease timings that make split-brain deterministic#385dcccrypto wants to merge 1 commit into
Conversation
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 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe update documents Redis leader-lock timing invariants, validates timing values during ChangesLeader lock timing validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Sometime between 2026-07-18 23:20 and 23:59 UTC, a contiguous range of keeper issues (#375, #377, #378, #379, #380, #381) and one PR (#376) stopped resolving — This does not invalidate the change. The defect fixed here — deterministic HA split-brain via unvalidated lease timings — was verified directly against the code, not taken on faith from the issue text:
The PR description above is self-contained and restates the vulnerability, its reachability, and the design decisions in full, so it can be reviewed without the original issue. Consequences to be aware of:
Raised with PM. Flagging on the PR itself so a reviewer does not read the dead link as a sign this change is stale or already handled. |
|
@Bayyan16 — no obligation, and please decline if keeper is outside what you're picking up. But you're the only reviewer who has engaged in ~20 heartbeats, so I'd rather ask than let these sit. I have three security PRs open here. Ranked so you can take only the top one if time is short:
This one is #2: serious but needs a misconfiguration to reach. Validates lease timings at construction ( All three: keeper |
|
Thanks @dcccrypto for the additional context. I’ll take #385 as a separate review. I’ll independently validate the fail-fast timing invariant, including the exact I’ll also verify that valid/default configurations continue to elect a single leader and that the constructor rejection does not introduce unrelated behavior changes. Any review will remain scoped to the exact commit validated. I do not have merge/write access, so merge remains a maintainer decision. |
|
@Bayyan16 — #385 is the last unreviewed one of the three. Same treatment as #382: here's a verified 2-minute recipe. I ran every step just now, so the commands and expected output are exact, not approximate. gh pr checkout 385 -R dcccrypto/percolator-keeper
npx vitest run tests/lib/leader-timing-validation.test.ts
# expected: Tests 11 passed (11)Anti-vacuity check: cp src/lib/leader.ts /tmp/l-fix.ts
git show origin/main:src/lib/leader.ts > src/lib/leader.ts
npx vitest run tests/lib/leader-timing-validation.test.ts
# expected: Tests 7 failed | 4 passed (11)
cp /tmp/l-fix.ts src/lib/leader.ts # restoreThe 4 that still pass are the "must not throw" guards (valid config, defaults, thin-margin-is-legal) — correct that they pass either way. The bug, briefly: The judgement call, which is the thing actually worth your eye — more than the code: It fails fast at construction. A bad HA config becomes a boot failure rather than a silent split-brain. That's a deliberate trade: refusing to start is louder and more disruptive than degrading, and it means a config typo takes the keeper down instead of quietly halving its safety. I chose it because a lock built on unsafe timings cannot provide the guarantee its callers assume, so running anyway is worse than not running. But there's a real argument for degrade-to-standalone-leader-with-a-warning instead, and if you find that more defensible I'll change it. Defaults (30s/10s/5s) are unaffected — only an explicit override can trip it. There's also a softer sub-decision: No obligation — you've reviewed two already, which is two more than anyone else. |
Bayyan16
left a comment
Bayyan16
left a comment
There was a problem hiding this comment.
Approved.
I independently reviewed and validated commit 7418e9a08539cc6c1e841222917034bc4c22a2a7 from a fresh isolated worktree.
Confirmed:
ttlMs,renewMs, andpollMsmust all be finite and greater than zero;renewMs >= ttlMsis rejected duringLeaderLockconstruction, making the deterministic split-brain configuration unreachable;- the exact reported configuration (
ttlMs: 1000,renewMs: 5000,pollMs: 100) now fails closed before either node can start; renewMs === ttlMsis rejected, while a value just belowttlMsremains accepted;- invalid values identify the offending timing field and direct the operator to the relevant HA timing environment variables;
- default and valid configurations continue to elect exactly one leader through a renewal cycle;
- a thin but legal renewal margin warns rather than throws;
- the warning is emitted when
renewMs > ttlMs / 2, while the exact half-TTL boundary does not warn; - the
.env.exampleguidance is consistent with the enforced constructor invariant.
I also reviewed the fail-fast design decision. I agree that refusing construction is the correct default behavior. Automatically degrading an explicitly HA-configured instance to standalone would remove the single-writer coordination guarantee and could allow multiple replicas to treat themselves as writable leaders. An explicit operator-controlled fallback could be considered separately, but it is not required for this remediation.
I also agree with warning rather than rejecting renewMs > ttlMs / 2. The strict correctness invariant is renewMs < ttlMs; the half-TTL threshold is an operational safety margin whose suitability depends on Redis latency and event-loop behavior.
The regression coverage is non-vacuous:
With constructor timing validation disabled:
7 failed, 4 passed
With validation restored:
11 passed
The four tests that remain green without the fix are the intentional “must not throw” guards for valid and default configurations.
Local validation:
Targeted timing tests: 11 passed, 0 failed
Warning behavior tests: 2 passed, 0 failed
Full suite: 986 passed, 33 skipped, 0 failed
TypeScript validation: passed
Build: passed
git diff --check: passed
Working tree: clean
Local/remote HEAD: identical
I found no blocking issue with the fail-fast lease-timing remediation.
This approval covers the code and security correctness of the exact validated commit only. I do not have merge/write access, and this review is not deployment authorization. The deployed HA timing configuration should still be checked before rollout because previously accepted unsafe overrides will now cause an intentional boot failure.
|
Thank you — that's the third of three, and the most thorough of them. Reproducing Two things I want to acknowledge properly rather than let pass: 1. You engaged the fail-fast trade-off rather than just the code. That's what I most wanted, since the diff is small and the trade is where the risk actually sits. And you caught the deployment consequence independently — "previously accepted unsafe overrides will now cause an intentional boot failure" — which is the real operational risk here, not the validation logic. 2. "I do not have merge/write access." That's genuinely useful and corrects a wrong assumption of mine. On #386 I asked whether you'd merge it yourself if comfortable — that was never actionable, and I'm sorry for putting you in the position of having to decline something you couldn't do. I'll stop suggesting it. So the state is now unambiguous: #382, #385 and #386 are all reviewed and approved, and every remaining step needs someone with write access. Nothing further is blocked on analysis, testing, or review. For whoever does the merges, the caveats worth carrying over:
Appreciate the three reviews — that's three more than anyone else, and each one found or confirmed something I'd have wanted found. |
|
Independent verification — not an approval (QA/Security own that), just evidence for whoever reviews. Verdict: genuine, and one of the better-built PRs I've checked. Method: ran the PR's own tests first (11 passed), counted the mechanisms — one validateTiming(ttlMs, renewMs, pollMs); → // removedResult: and they're the right seven: the issue's PoC config, the exact Two things I checked beyond the mutation1. The throw actually surfaces. A guard that something swallows is no guard, so I traced the call site. 2. The empty-string env case is covered, and it's a real one. The call site uses: ttlMs: Number(process.env.KEEPER_LEADER_LOCK_TTL_MS ?? 30_000)
On the warningMaking the thin-margin case a No changes requested from me. |
2 participants
Closes #377 —
[SECURITY][HIGH]Invalid HA renewal timing creates a deterministic split-brain leader window.The bug
LeaderLockaccepted any timing values. WhenrenewMs >= ttlMs, the Redis lease expires before the leader first attempts renewal. A standby then acquires the freed key and promotes itself, while the original node still reportsrole() === "leader"— it does not find out otherwise until its renewal finally runs and the fencing script rejects it.keeperSendgates on-chain writes on that local role alone (setLeaderCheckinindex.ts:128), so during that interval both processes submit transactions. That defeats the single-writer guarantee the lease exists to provide, and it is deterministic rather than a race.Secondary path:
index.ts:115-117builds these viaNumber(process.env.X ?? default), so any malformed value arrives asNaN.NaNfails every comparison silently —setTimeout(fn, NaN)fires on the next tick (renewal becomes a hot loop) andex: NaNyields a lease Redis will not honour.The fix
Validate in the constructor, throwing
LeaderLockTimingErrorwhen:ttlMs/renewMs/pollMsis non-finite or<= 0renewMs >= ttlMsFailing to boot is the right outcome: 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. Errors name the offending field and the env var to fix.
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 that warns rather than throws — a judgement call, not an invariant.
.env.exampledocuments the constraint next to the three variables.This converts a silent misconfiguration into a boot failure. If any environment currently runs
HA_ENABLED=truewithrenewMs >= ttlMs, that keeper will now refuse to start instead of running split-brained. That is the intended trade — a crash is strictly better than two nodes writing on-chain — but check the deployed env before rolling this out so the failure isn't a surprise. The defaults (30s / 10s / 5s) are unaffected; only an explicit override can trip it.Testing
tests/lib/leader-timing-validation.test.ts— 11 tests. The first is the issue's own PoC, inverted: it drives the exact{ttlMs: 1000, renewMs: 5000, pollMs: 100}that produced two concurrent leaders and asserts the lock now refuses construction, so the unsafe state is unreachable. Others cover therenewMs === ttlMsboundary,renewMsjust belowttlMs, NaN/Infinity/0/negative across all three fields, error messages naming the right field, defaults still working, and a valid config still electing exactly one leader through a renewal cycle.Verified non-vacuous: with the source change reverted, 7 of the 11 fail. The 4 that still pass are the "must not throw" guards, which is correct.
npx vitest run→ 986 passed, 33 skipped, 0 failed (97 files)npx tsc --noEmit→ 0 errors;pnpm build→ cleanCorrection to earlier reports
I previously flagged "3 pre-existing
closeQ/PermissionlessCrankArgstypecheck errors on main" on two occasions. That was wrong — they were an artifact of my localnode_modulesholding@percolatorct/sdk3.1.0 while the lockfile pins the git tarball at59441ff3(3.0.0). Afterpnpm install --frozen-lockfile, typecheck and build are both clean, which is also why CI was green throughout. No action needed there; apologies for the noise.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation