fix(relay): clear the demand-expiry timeout on a fence, not only in stop()

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.
This commit is contained in:
Neil
2026-09-10 17:28:44 -07:00
parent de90e89297
commit fcea70fe3c
3 changed files with 64 additions and 12 deletions
@@ -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()
})
})
+15 -12
View File
@@ -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()
}
@@ -150,6 +150,10 @@ export class RelayAuthCoordinator {
// was about to work. The budget bounds only the retry chain below.
await pending
if (pending !== this.latestReconcile) {
// Known gap: this arm skips the deadline check, so under continuous reconcile churn the
// budget above 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, not unbounded.
continue
}
// Why: a reconcile that failed transiently has already armed its own