From 6b029820cc98ff2726657141b82f1d13e4ad2f6c Mon Sep 17 00:00:00 2001 From: Neil Date: Thu, 10 Sep 2026 17:41:56 -0700 Subject: [PATCH] docs(relay): the live-broker wait budget does not bound the call `LIVE_BROKER_WAIT_BUDGET_MS` carried two claims. One is true and load-bearing: 20s bounds how long a waiter sits through armed retries. The other was false: "stays inside the phone's 30s request budget". It cannot. The deadline is consulted only AFTER `await pending`, which is unbounded on purpose, and a reconcile's own ceiling is `readContext`'s cloud-refresh timeout (60s, plus one retry for a definitive 5xx) followed by the broker open. Measured: with a reconcile in flight, waitForLiveBrokerResult() at its default 20s budget has not settled at 45s. The consequence is not academic. `pairing.provisionRelay` reaches this through requireActiveBroker, so it is a phone-facing call: the phone gives up and retries while the desktop is still holding a transient demand ref for the request it abandoned. Behaviour unchanged. Bounding the reconcile would fail a slow-but-succeeding open, which is the worse trade and is what the adjacent comment already argues. What changes is that the false half of the claim is removed and the real ceiling is written down, with a test pinning it so the claim cannot drift back. Also corrected the inner comment: "a reconcile always settles (opens carry HTTP deadlines)" is true, but the parenthetical named only the smaller of its two deadlines and omitted readContext's, which is the one that actually dominates. "Settles" is not "settles soon", and reading it as the latter is how the 30s claim survived. Mutation: bounding `await pending` with the deadline fails the new test. --- ...relay-auth-coordinator-wait-budget.test.ts | 57 +++++++++++++++++++ .../runtime/relay/relay-auth-coordinator.ts | 24 +++++--- 2 files changed, 74 insertions(+), 7 deletions(-) create mode 100644 src/main/runtime/relay/relay-auth-coordinator-wait-budget.test.ts diff --git a/src/main/runtime/relay/relay-auth-coordinator-wait-budget.test.ts b/src/main/runtime/relay/relay-auth-coordinator-wait-budget.test.ts new file mode 100644 index 00000000000..e46431f42f6 --- /dev/null +++ b/src/main/runtime/relay/relay-auth-coordinator-wait-budget.test.ts @@ -0,0 +1,57 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import { RelayAuthCoordinator, type RelayAuthContext } from './relay-auth-coordinator' + +const context: RelayAuthContext = { + identity: { userId: 'user-1', profileId: 'profile-1', organizationId: 'org-1' }, + accessToken: 'access-1', + relayEntitled: true +} + +/** + * What the live-broker wait budget does and does not bound. + * + * `LIVE_BROKER_WAIT_BUDGET_MS` is 20s and the deadline is checked only AFTER `await pending`, which + * is deliberately unbounded — cutting a slow-but-succeeding open short would fail a pairing that + * was about to work. So the budget bounds the armed-retry chain and nothing else. + * + * That is worth a test rather than a comment because the consequence lands on the phone: + * `pairing.provisionRelay` reaches this through `requireActiveBroker`, and `readContext`'s own + * ceiling is the cloud refresh timeout (60s, with one retry for a definitive 5xx) — several times + * the phone's request budget. The phone gives up and retries while the desktop is still holding a + * transient demand ref for the call it abandoned. + */ +describe('live-broker wait budget', () => { + afterEach(() => { + vi.useRealTimers() + }) + + it('does not bound a reconcile already in flight, so it can outlast the phone request budget', async () => { + vi.useFakeTimers() + let releaseContext = (): void => {} + const coordinator = new RelayAuthCoordinator({ + readContext: () => + new Promise((resolve) => { + releaseContext = (): void => resolve(context) + }), + openBroker: async () => ({ closeNow: vi.fn() }), + onStatus: vi.fn() + }) + coordinator.reconcile() + await Promise.resolve() + + let settled = false + const wait = coordinator.waitForLiveBrokerResult().then((result) => { + settled = true + return result + }) + + // Well past both the 20s budget and the phone's 30s request budget. + await vi.advanceTimersByTimeAsync(45_000) + expect(settled).toBe(false) + + releaseContext() + await vi.advanceTimersByTimeAsync(0) + await expect(wait).resolves.toEqual({ broker: expect.anything() }) + expect(settled).toBe(true) + }) +}) diff --git a/src/main/runtime/relay/relay-auth-coordinator.ts b/src/main/runtime/relay/relay-auth-coordinator.ts index a284618ac72..c5f82395af4 100644 --- a/src/main/runtime/relay/relay-auth-coordinator.ts +++ b/src/main/runtime/relay/relay-auth-coordinator.ts @@ -31,10 +31,18 @@ type BrokerOwnership = { } export class RelayAuthCoordinator { - // Why 20s: bounds only how long a waiter sits through armed retries, never - // an open already in flight. It spans the first few rungs of the backoff - // ladder and stays inside the phone's 30s request budget, so a sustained - // outage fails the caller with its cause instead of parking the demand ref. + // Why 20s: bounds only how long a waiter sits through armed retries, never a reconcile already + // in flight. It spans the first few rungs of the backoff ladder, so a sustained outage fails the + // caller with its cause instead of parking the demand ref. + // + // It does NOT bound the call. The deadline is only consulted after `await pending` below, and a + // reconcile's own ceiling is `readContext`'s cloud-refresh timeout (60s, plus one retry for a + // definitive 5xx) followed by the broker open — several times the phone's 30s request budget, + // which an earlier version of this comment claimed it stayed inside. A phone whose + // pairing.provisionRelay reaches this through requireActiveBroker can therefore give up and + // retry while the desktop still holds a transient demand ref for the call it abandoned. + // Bounding the reconcile instead would fail a slow-but-succeeding open, which is the worse trade; + // relay-auth-coordinator-wait-budget.test.ts pins the behaviour so the claim cannot drift back. private static readonly LIVE_BROKER_WAIT_BUDGET_MS = 20_000 private readonly options: RelayAuthCoordinatorOptions private authEpoch = 0 @@ -145,9 +153,11 @@ export class RelayAuthCoordinator { return { broker } } const pending = this.latestReconcile - // Why unbounded: a reconcile always settles (opens carry HTTP deadlines), - // and cutting a slow-but-succeeding open short would fail a pairing that - // was about to work. The budget bounds only the retry chain below. + // Why unbounded: a reconcile always settles — BOTH its awaits carry a deadline, the broker + // open and `readContext`'s cloud refresh (the larger of the two, and the one the original + // parenthetical here left out) — and cutting a slow-but-succeeding open short would fail a + // pairing that was about to work. "Settles" is not "settles soon": see the ceiling noted on + // LIVE_BROKER_WAIT_BUDGET_MS. The budget bounds only the retry chain below. await pending if (pending !== this.latestReconcile) { continue