From 40252d799874c262f65cdf77b30c0e4e1de3476f Mon Sep 17 00:00:00 2001 From: Joseph Mearman Date: Fri, 2 Oct 2026 06:39:42 +0100 Subject: [PATCH] fix(core)!: reject a coordinator term a later claim cannot supersede claim() mints the incumbent's term plus one, which stops being a larger number once the term reaches Number.MAX_SAFE_INTEGER, so an incumbent gossiped at such a term could never be superseded. evaluate now returns a "rejected" outcome for a term that is not a non-negative safe integer below that ceiling, leaving the incumbent untouched, and claim() throws a RangeError instead of minting a term peers would reject. BREAKING CHANGE: EvaluationOutcome gains a "rejected" variant that carries no incumbent, so a caller reading result.incumbent must narrow on result.outcome first. --- .../core/src/domain/coordinator-election.ts | 27 ++++++-- .../test/coordinator-election.unit.test.ts | 66 ++++++++++++++++++- .../mesh-session-coordinator.unit.test.ts | 2 +- 3 files changed, 86 insertions(+), 9 deletions(-) diff --git a/ts/packages/core/src/domain/coordinator-election.ts b/ts/packages/core/src/domain/coordinator-election.ts index 6e563584..e91304f0 100644 --- a/ts/packages/core/src/domain/coordinator-election.ts +++ b/ts/packages/core/src/domain/coordinator-election.ts @@ -25,10 +25,18 @@ export function compareDeviceIds(a: DeviceId, b: DeviceId): number { return a.length - b.length; } -/** What evaluating an incoming claim concluded, so the caller can react without re-deriving the comparison: "accepted" means this claim is now the incumbent (it superseded a previous one, or there was none), "retained" means the incumbent survived (the incoming claim was stale, or lost the equal-term tiebreak), and the incumbent is returned either way so a caller squashing a stale claim re-announces exactly what it already holds. */ +/** Whether a claim at this term can be superseded by a later claim: a non-negative safe integer strictly below Number.MAX_SAFE_INTEGER, so that term + 1 is still an exactly represented, strictly greater integer. Above that ceiling a double cannot count on (2 ** 53 + 1 === 2 ** 53), and an incumbent there could never be superseded, so a device that named itself holder would keep the role for good even after it had gone. */ +export function isSupersedableTerm(term: number): boolean { + return ( + Number.isSafeInteger(term) && term >= 0 && term < Number.MAX_SAFE_INTEGER + ); +} + +/** What evaluating an incoming claim concluded, so the caller can react without re-deriving the comparison: "accepted" means this claim is now the incumbent (it superseded a previous one, or there was none), "retained" means the incumbent survived (the incoming claim was stale, or lost the equal-term tiebreak), and the incumbent is returned either way so a caller squashing a stale claim re-announces exactly what it already holds. "rejected" means the claim's term is not one a later claim can supersede (isSupersedableTerm), so it was never compared: the incumbent, if any, is untouched, and there is nothing to answer it with. */ export type EvaluationOutcome = | { outcome: "accepted"; incumbent: CoordinatorClaim } - | { outcome: "retained"; incumbent: CoordinatorClaim }; + | { outcome: "retained"; incumbent: CoordinatorClaim } + | { outcome: "rejected" }; export interface CoordinatorElectionOptions { /** This device's own id: the coordinator named by a claim this side mints. */ @@ -57,13 +65,19 @@ export class CoordinatorElection { } /** - * Claims the role for this side's own device at a term above every term seen so far (lastTerm + 1, so a first-ever claim is term 0), and returns the frame to gossip. Claiming over a claim this side already holds is a deliberate takeover: it raises the term, which is exactly what a peer recovering the role after losing track of the mesh should do, and what a routine refresh should not (announceCurrent exists for that). + * Claims the role for this side's own device at a term above every term seen so far (lastTerm + 1, so a first-ever claim is term 0), and returns the frame to gossip. Throws a RangeError when that term is not one peers accept (isSupersedableTerm), which only happens when the incumbent already sits at the highest term the election accepts. Claiming over a claim this side already holds is a deliberate takeover: it raises the term, which is exactly what a peer recovering the role after losing track of the mesh should do, and what a routine refresh should not (announceCurrent exists for that). */ claim(capacityHint?: number): CoordinatorFrame { const lastTerm = this.incumbent?.term ?? -1; + const term = lastTerm + 1; + if (!isSupersedableTerm(term)) { + throw new RangeError( + `the incumbent's term ${String(lastTerm)} leaves no term a claim can be raised to`, + ); + } const frame: CoordinatorFrame = { type: "coordinator", - term: lastTerm + 1, + term, coordinator: this.ownDevice, ...(capacityHint !== undefined ? { "capacity-hint": capacityHint } : {}), }; @@ -91,9 +105,12 @@ export class CoordinatorElection { } /** - * Evaluates an incoming claim against the one this side currently accepts: a strictly higher term always wins; an equal term breaks by lowest device-id, so an incoming claim naming a lower device-id than the incumbent's takes the role and one naming a higher device-id loses; a lower term never wins. "accepted" means the incumbent changed (the caller should gossip the new incumbent onward so the supersession propagates); "retained" means it did not (the caller may answer a stale claim by re-gossiping announceCurrent(), and should drop a lost equal-term claim it originated, since the tiebreak has settled it). + * Evaluates an incoming claim against the one this side currently accepts: a strictly higher term always wins; an equal term breaks by lowest device-id, so an incoming claim naming a lower device-id than the incumbent's takes the role and one naming a higher device-id loses; a lower term never wins. "accepted" means the incumbent changed (the caller should gossip the new incumbent onward so the supersession propagates); "retained" means it did not (the caller may answer a stale claim by re-gossiping announceCurrent(), and should drop a lost equal-term claim it originated, since the tiebreak has settled it); "rejected" means the claim's term is not supersedable (isSupersedableTerm), so it is dropped unevaluated and the caller should neither gossip nor answer it. */ evaluate(frame: Readonly): EvaluationOutcome { + if (!isSupersedableTerm(frame.term)) { + return { outcome: "rejected" }; + } const previous = this.incumbent; if ( previous === undefined || diff --git a/ts/packages/core/test/coordinator-election.unit.test.ts b/ts/packages/core/test/coordinator-election.unit.test.ts index 19bae7e0..02199e75 100644 --- a/ts/packages/core/test/coordinator-election.unit.test.ts +++ b/ts/packages/core/test/coordinator-election.unit.test.ts @@ -18,6 +18,8 @@ const TERM_HIGH = 5; const TERM_HIGHEST = 6; const CAPACITY_HINT = 7; const LATE_CAPACITY_HINT = 9; +/** A fractional offset, to make a term that is not an integer. */ +const HALF = 0.5; /** A coordinator-frame naming `device` at `term`, the shape every evaluate test feeds. */ function claim(device: DeviceId, term: number): CoordinatorFrame { @@ -53,8 +55,10 @@ describe("CoordinatorElection.evaluate", () => { it("accepts the first claim it hears", () => { const election = new CoordinatorElection({ ownDevice: OWN }); const outcome = election.evaluate(claim(LOWER, TERM_LOW)); - expect(outcome.outcome).toBe("accepted"); - expect(outcome.incumbent).toEqual({ term: TERM_LOW, coordinator: LOWER }); + expect(outcome).toEqual({ + outcome: "accepted", + incumbent: { term: TERM_LOW, coordinator: LOWER }, + }); }); it("supersedes the incumbent on a strictly higher term", () => { @@ -95,7 +99,63 @@ describe("CoordinatorElection.evaluate", () => { ...claim(LOWER, TERM_MIDDLE), "capacity-hint": LATE_CAPACITY_HINT, }); - expect(outcome.incumbent.capacityHint).toBe(LATE_CAPACITY_HINT); + expect(election.current()?.capacityHint).toBe(LATE_CAPACITY_HINT); + expect(outcome.outcome).toBe("accepted"); + }); +}); + +describe("CoordinatorElection term ceiling", () => { + // The highest term evaluate accepts: its successor, term + 1, is still an exactly represented integer, so it is strictly greater. + const LAST_RAISABLE_TERM = Number.MAX_SAFE_INTEGER - 1; + const UNRAISABLE_TERMS = [ + Number.MAX_SAFE_INTEGER, + Number.MAX_SAFE_INTEGER + 1, + Number.POSITIVE_INFINITY, + Number.NaN, + -1, + TERM_LOW + HALF, + ]; + + it.each(UNRAISABLE_TERMS)( + "rejects a claim at term %s without making it the incumbent", + (term) => { + const election = new CoordinatorElection({ ownDevice: OWN }); + expect(election.evaluate(claim(LOWER, term))).toEqual({ + outcome: "rejected", + }); + expect(election.current()).toBeUndefined(); + }, + ); + + it("keeps the incumbent when an unraisable claim arrives", () => { + const election = new CoordinatorElection({ ownDevice: OWN }); + election.evaluate(claim(HIGHER, TERM_LOW)); + expect( + election.evaluate(claim(LOWER, Number.MAX_SAFE_INTEGER)).outcome, + ).toBe("rejected"); + expect(election.current()).toEqual({ + term: TERM_LOW, + coordinator: HIGHER, + }); + expect(election.claim().term).toBe(TERM_LOW + 1); + }); + + it("accepts a claim at the highest usable term, after which claim() refuses rather than mint a term peers would reject", () => { + const election = new CoordinatorElection({ ownDevice: OWN }); + expect(election.evaluate(claim(HIGHER, LAST_RAISABLE_TERM)).outcome).toBe( + "accepted", + ); + expect(() => election.claim()).toThrow(RangeError); + expect(election.current()?.coordinator).toEqual(HIGHER); + }); + + it("mints a claim at the highest usable term that a peer accepts", () => { + const election = new CoordinatorElection({ ownDevice: OWN }); + election.evaluate(claim(HIGHER, LAST_RAISABLE_TERM - 1)); + const frame = election.claim(); + expect(frame.term).toBe(LAST_RAISABLE_TERM); + const peer = new CoordinatorElection({ ownDevice: HIGHER }); + expect(peer.evaluate(frame).outcome).toBe("accepted"); }); }); diff --git a/ts/packages/core/test/mesh-session-coordinator.unit.test.ts b/ts/packages/core/test/mesh-session-coordinator.unit.test.ts index 87f797ae..5b24bd5a 100644 --- a/ts/packages/core/test/mesh-session-coordinator.unit.test.ts +++ b/ts/packages/core/test/mesh-session-coordinator.unit.test.ts @@ -59,7 +59,7 @@ describe("coordinator-frame plumbing", () => { expect(frame.term).toBe(TERM_REMOTE_HEARD); const outcome = election.evaluate(frame); expect(outcome.outcome).toBe("accepted"); - expect(outcome.incumbent.coordinator).toEqual(OTHER); + expect(election.current()?.coordinator).toEqual(OTHER); await session.close(); }); });