From 25096b39d8ea312d0b0da8abe3b77a2eb205d07b Mon Sep 17 00:00:00 2001 From: Neil Date: Thu, 10 Sep 2026 17:56:17 -0700 Subject: [PATCH] fix(relay): the demand wake signal must not fail the grant it wakes for MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `withTransientDemand` called `refreshDemand()` bare on both sides of the operation. It reaches the device registry (`nextPendingExpiry` -> `listDevices`) and the settings store (`hasDemand` -> `isRelayAllowedForDevice`), so it can fail on its own — and measured, each call site failed the operation instead: - the pre-call threw with the transient ref already acquired, so the ref was never released and the operation never ran. Transient refs have no expiry, so that ref holds relay demand for the rest of the process. - the teardown call replaced the operation's own result in both directions: a named mint failure arrived as `listDevices exploded`, and a SUCCESSFUL mint arrived as a rejection. Both are wake signals; the liveness tick and the next refresh re-ask, so a lost signal is recoverable where a stranded ref is not. The containment lives beside the ledger that owns the ref, because desktop-relay-service.ts is at its max-lines ceiling and this must not cost it a line. Also carries the original error as the `cause` of the mid-operation policy flip rewrite, which was discarding it — the one place in a commit about naming causes that destroyed one. --- ...-service-transient-demand-teardown.test.ts | 113 ++++++++++++++++++ .../runtime/relay/desktop-relay-service.ts | 8 +- src/main/runtime/relay/relay-demand-ledger.ts | 19 +++ 3 files changed, 136 insertions(+), 4 deletions(-) create mode 100644 src/main/runtime/relay/desktop-relay-service-transient-demand-teardown.test.ts diff --git a/src/main/runtime/relay/desktop-relay-service-transient-demand-teardown.test.ts b/src/main/runtime/relay/desktop-relay-service-transient-demand-teardown.test.ts new file mode 100644 index 00000000000..8ac7e62549f --- /dev/null +++ b/src/main/runtime/relay/desktop-relay-service-transient-demand-teardown.test.ts @@ -0,0 +1,113 @@ +import { describe, expect, it, vi } from 'vitest' +import { DesktopRelayService } from './desktop-relay-service' +import type { MobilePairingConnectionMode } from '../../../shared/mobile-pairing-connection-mode' + +/** + * `withTransientDemand`'s teardown runs work that can fail on its own. + * + * `refreshDemand` reaches the device registry (`nextPendingExpiry` -> `listDevices`) and the + * settings store (`hasDemand` -> `isRelayAllowedForDevice`). Running it bare in the `finally` let + * its failure replace the operation's result in BOTH directions: a named mint failure arrived as + * an unrelated message, and a successful mint arrived as a rejection. Before the operation it was + * worse still: the throw landed after the transient ref was acquired, so the ref was never + * released, and transient refs have no expiry. + */ +type TransientDemandHost = { + withTransientDemand: ( + kind: string, + deviceId: string, + operation: () => Promise + ) => Promise +} + +function serviceWithTeardown(options: { + release?: () => void + refreshDemand?: () => void +}): (operation: () => Promise) => Promise { + const service = Object.create(DesktopRelayService.prototype) as DesktopRelayService + Object.assign(service, { + hostMobilePairingConnectionMode: () => 'automatic' as MobilePairingConnectionMode, + runtimeRpc: { + getDeviceRegistry: () => ({ getMobilePairingConnectionMode: () => 'automatic' }) + }, + demandLedger: { acquireTransient: () => options.release ?? ((): void => {}) }, + refreshDemand: options.refreshDemand ?? ((): void => {}) + }) + const host = service as unknown as TransientDemandHost + return (operation) => host.withTransientDemand.call(service, 'pairing', 'device-1', operation) +} + +describe('DesktopRelayService transient-demand teardown', () => { + it('keeps the operation failure when the demand refresh throws', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + const run = serviceWithTeardown({ + refreshDemand: () => { + throw new Error('listDevices exploded') + } + }) + + await expect( + run(async () => { + throw new Error('relay_broker_rejected') + }) + ).rejects.toThrow('relay_broker_rejected') + expect(warn).toHaveBeenCalled() + warn.mockRestore() + }) + + it('does not strand the demand ref when the pre-operation refresh throws', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + const release = vi.fn() + const operation = vi.fn(async () => 'minted') + const run = serviceWithTeardown({ + release, + refreshDemand: () => { + throw new Error('listDevices exploded') + } + }) + + // The ref has no expiry, so failing the wake signal must not take the release with it. + await expect(run(operation)).resolves.toBe('minted') + expect(operation).toHaveBeenCalledTimes(1) + expect(release).toHaveBeenCalledTimes(1) + warn.mockRestore() + }) + + it('keeps a successful mint successful when the demand refresh throws', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + const run = serviceWithTeardown({ + refreshDemand: () => { + throw new Error('listDevices exploded') + } + }) + + await expect(run(async () => 'minted')).resolves.toBe('minted') + warn.mockRestore() + }) + + it('carries the original failure as the cause of a mid-operation policy flip', async () => { + const mode = { current: 'automatic' as MobilePairingConnectionMode } + const service = Object.create(DesktopRelayService.prototype) as DesktopRelayService + Object.assign(service, { + hostMobilePairingConnectionMode: () => mode.current, + runtimeRpc: { + getDeviceRegistry: () => ({ getMobilePairingConnectionMode: () => 'automatic' }) + }, + demandLedger: { acquireTransient: () => (): void => {} }, + refreshDemand: () => {} + }) + const host = service as unknown as TransientDemandHost + + const rejection = await host.withTransientDemand + .call(service, 'pairing', 'device-1', async () => { + mode.current = 'local-only' + throw new Error('relay_token_exchange_failed_503') + }) + .catch((error: unknown) => error) + + expect((rejection as Error).message).toBe('relay_disabled_for_device') + expect(((rejection as Error).cause as Error | undefined)?.message).toBe( + 'relay_token_exchange_failed_503' + ) + }) +}) diff --git a/src/main/runtime/relay/desktop-relay-service.ts b/src/main/runtime/relay/desktop-relay-service.ts index f108fda2968..cba87beaa52 100644 --- a/src/main/runtime/relay/desktop-relay-service.ts +++ b/src/main/runtime/relay/desktop-relay-service.ts @@ -18,7 +18,7 @@ import type { RelayRevokeOutboxItem } from './relay-revoke-outbox' import { deriveRelayHostId } from './relay-http-client' -import { RelayDemandLedger } from './relay-demand-ledger' +import { RelayDemandLedger, refreshRelayDemandBestEffort } from './relay-demand-ledger' import { createRelayRegionPreferenceReader } from './relay-region-preference' import { pairingAuthorizationForContext } from './relay-pairing-authorization' import { buildPairingEndpointsResult } from './relay-pairing-endpoints-result' @@ -292,7 +292,7 @@ export class DesktopRelayService { throw new Error('relay_disabled_for_device') } const release = this.demandLedger.acquireTransient(`${kind}:${deviceId}`, deviceId) - this.refreshDemand() + refreshRelayDemandBestEffort(() => this.refreshDemand()) try { return await operation() } catch (error) { @@ -301,12 +301,12 @@ export class DesktopRelayService { // reaches `standby` and clears the offline reason. The wait then ends with no cause at all // — the generic `relay_control_not_active` — when the flip is exactly the cause. if (!this.isRelayAllowedForDevice(deviceId)) { - throw new Error('relay_disabled_for_device') + throw new Error('relay_disabled_for_device', { cause: error }) } throw error } finally { release() - this.refreshDemand() + refreshRelayDemandBestEffort(() => this.refreshDemand()) } } diff --git a/src/main/runtime/relay/relay-demand-ledger.ts b/src/main/runtime/relay/relay-demand-ledger.ts index 817fbc37e37..ea5ee4e14e4 100644 --- a/src/main/runtime/relay/relay-demand-ledger.ts +++ b/src/main/runtime/relay/relay-demand-ledger.ts @@ -13,6 +13,25 @@ type RelayDemandLedgerOptions = { type TransientRef = { deviceId: string; count: number } +/** + * Run a demand refresh as the wake signal it is. + * + * A refresh reaches the device registry (`nextPendingExpiry` -> `listDevices`) and the settings + * store (`hasDemand` -> the host pairing mode), so it can fail on its own. Bare, it failed the + * caller it was waking for: before an operation it threw with a transient ref already acquired — + * and those have no expiry, so the ref held relay demand for the rest of the process — and after + * one it replaced the operation's own result, turning a named mint failure into an unrelated + * message and a successful mint into a rejection. A lost wake signal is recoverable; the liveness + * tick and the next refresh both re-ask. + */ +export function refreshRelayDemandBestEffort(refresh: () => void): void { + try { + refresh() + } catch (error) { + console.warn('[relay] demand refresh failed:', error) + } +} + export class RelayDemandLedger { private readonly options: RelayDemandLedgerOptions private readonly transientRefs = new Map()