From 6bd997b2ccaf42b7d0fe082f93126bfbdfb80d4a Mon Sep 17 00:00:00 2001 From: Neil Date: Thu, 10 Sep 2026 17:28:44 -0700 Subject: [PATCH] fix(relay): clear the demand-expiry timeout on a fence, not only in stop() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Why the existing lifecycle test missed this: it stubs `demandLedger: { nextPendingExpiry: () => null }`, so the second timer this class owns is never armed and the fence is only ever asserted against the liveness interval. Change that one stub to a future expiry and the test fails. `fenceAndCloseNow` cleared `livenessTimer` and left `demandExpiryTimer` armed. `stop()` cleared both — but `stop()` has no production caller, while all three fence sites (quit, sign-out, SIGNED_OUT) go through the path that cleared only one. The consequence is the sharp part. The orphaned timeout fires `refreshDemand`, which reconciles *and* re-installs the liveness interval the fence just tore down. A fence during an outstanding pairing invite therefore undid itself minutes later — the post-fence resurrection the comment two lines above forbids in so many words. Both teardown paths now share one `disarmTimers()`. That also resolves the `max-lines` pressure the extra clause created: the two teardown blocks were byte identical, so deduplicating them is the right fix rather than a `max-lines` disable, which this repo forbids and which the next person under that cap will be tempted by. The new test asserts `vi.getTimerCount() === 0` after the fence rather than inferring from an absence of errors, and pins that a fence is not a stop — the next auth mutation still re-arms both timers. Pre-existing, not introduced by this stack; verified identical at the base SHA. Mutation-tested: removing the `demandExpiryTimer` clause from `disarmTimers` fails the new test at the timer-count assertion. Also carries a note at `waitForLiveBrokerResult`: the `pending !== latestReconcile` arm re-enters the loop without re-checking the deadline, so under continuous reconcile churn the documented 20s budget is not strictly enforced and the caller's transient demand ref is held past it. Not a hang — every iteration awaits a settling promise — so it is bounded by how long churn lasts. --- .../relay/desktop-relay-service.test.ts | 45 +++++++++++++++++++ .../runtime/relay/desktop-relay-service.ts | 27 ++++++----- 2 files changed, 60 insertions(+), 12 deletions(-) diff --git a/src/main/runtime/relay/desktop-relay-service.test.ts b/src/main/runtime/relay/desktop-relay-service.test.ts index 2eba52ae0c8..6a1642221ee 100644 --- a/src/main/runtime/relay/desktop-relay-service.test.ts +++ b/src/main/runtime/relay/desktop-relay-service.test.ts @@ -193,3 +193,48 @@ describe('local-only mobile pairing', () => { ).rejects.toThrow('relay_disabled_for_device') }) }) + +describe('fence clears the pending demand-expiry timeout', () => { + afterEach(() => vi.useRealTimers()) + + // The lifecycle test above stubs `nextPendingExpiry` to null, so it never arms the second timer + // this class owns. With a pairing invite outstanding the fence left that timeout armed, and + // `stop()` — the only method that cleared it — has no production caller. The orphaned timeout + // then ran `refreshDemand`, which reconciles AND re-installs the liveness interval the fence had + // just torn down: the post-fence resurrection the fence comment forbids. + it('leaves no armed timer and cannot reconcile after a fence', () => { + vi.useFakeTimers() + const coordinator = { + reconcile: vi.fn(), + ensureLive: vi.fn(), + fenceAndCloseNow: vi.fn(), + stop: vi.fn() + } + const service = Object.create(DesktopRelayService.prototype) as DesktopRelayService + Object.assign(service, { + coordinator, + // A pending invite: the QR is minted and its server-side expiry is still in the future. + demandLedger: { nextPendingExpiry: () => Date.now() + 30_000 }, + stopped: false, + livenessTimer: null, + demandExpiryTimer: null + }) + + service.start() + expect(coordinator.reconcile).toHaveBeenCalledTimes(1) + expect(vi.getTimerCount()).toBe(2) + + service.fenceAndCloseNow() + expect(vi.getTimerCount()).toBe(0) + + vi.advanceTimersByTime(30 * 60_000) + expect(coordinator.reconcile).toHaveBeenCalledTimes(1) + expect(coordinator.ensureLive).not.toHaveBeenCalled() + + // A fence is not a stop: the next auth mutation still re-arms both timers. + service.authMutated() + expect(coordinator.reconcile).toHaveBeenCalledTimes(2) + expect(vi.getTimerCount()).toBe(2) + service.stop() + }) +}) diff --git a/src/main/runtime/relay/desktop-relay-service.ts b/src/main/runtime/relay/desktop-relay-service.ts index f108fda2968..72d8d0afed6 100644 --- a/src/main/runtime/relay/desktop-relay-service.ts +++ b/src/main/runtime/relay/desktop-relay-service.ts @@ -111,14 +111,24 @@ export class DesktopRelayService { this.refreshDemand() } - fenceAndCloseNow(hostCloseReason?: RelayHostCloseReason): void { - // Why: a fence must be hard — a surviving liveness tick could catch the - // window between the pre-sign-out fence and the profile wipe and briefly - // resurrect a broker. The next auth mutation re-arms via refreshDemand. + /** Disarms both timers this class owns. Every teardown needs both: the expiry timeout fires + * `refreshDemand`, which reconciles and re-installs the liveness interval. */ + private disarmTimers(): void { + if (this.demandExpiryTimer) { + clearTimeout(this.demandExpiryTimer) + this.demandExpiryTimer = null + } if (this.livenessTimer) { clearInterval(this.livenessTimer) this.livenessTimer = null } + } + + fenceAndCloseNow(hostCloseReason?: RelayHostCloseReason): void { + // Why: a fence must be hard — a surviving tick could catch the window between + // the pre-sign-out fence and the profile wipe and briefly resurrect a broker. + // The next auth mutation re-arms via refreshDemand. + this.disarmTimers() this.coordinator.fenceAndCloseNow(hostCloseReason) } @@ -222,14 +232,7 @@ export class DesktopRelayService { stop(): void { this.stopped = true - if (this.demandExpiryTimer) { - clearTimeout(this.demandExpiryTimer) - this.demandExpiryTimer = null - } - if (this.livenessTimer) { - clearInterval(this.livenessTimer) - this.livenessTimer = null - } + this.disarmTimers() this.coordinator.stop() }