From eca1bb4cc6b1b2ce1adf775dcb127f600f21ba2d Mon Sep 17 00:00:00 2001 From: Neil Date: Thu, 10 Sep 2026 18:33:41 -0700 Subject: [PATCH 1/9] refactor(relay): extract the revoke-outbox flush into its own module MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Behaviour-preserving move, no logic change: flushRevokeOutbox and flushRevoke leave DesktopRelayService as RelayRevokeOutboxFlusher.flushAll/flushItem. The two fixes that follow each add lines to this file, and it has no headroom under the 300-line cap; a max-lines disable is not an option. Why a new file rather than reusing something: nothing existing drains the outbox. RelayRevokeOutbox is the durable store — making it drain itself over a control would give the persistence layer a dependency on a broker. RelayControlRequests dedups in-flight reqIds on a single socket, not queued items across reconnects. RelayDemandLedger reads the outbox for demand and never mutates it. The drain only ever existed inline here, so this is an extraction, not a parallel implementation; the only new code is the options type and the class shell. --- .../runtime/relay/desktop-relay-service.ts | 41 +++++------------- .../relay/relay-revoke-outbox-flush.ts | 42 +++++++++++++++++++ 2 files changed, 53 insertions(+), 30 deletions(-) create mode 100644 src/main/runtime/relay/relay-revoke-outbox-flush.ts diff --git a/src/main/runtime/relay/desktop-relay-service.ts b/src/main/runtime/relay/desktop-relay-service.ts index f108fda29689..338d906a444f 100644 --- a/src/main/runtime/relay/desktop-relay-service.ts +++ b/src/main/runtime/relay/desktop-relay-service.ts @@ -12,11 +12,8 @@ import { RelayAuthCoordinator } from './relay-auth-coordinator' import { relayOfflineReasonMintFailureCode } from './relay-offline-reason' import { RelaySessionBroker, type RelayBrokerStatus } from './relay-session-broker' import type { PairingRelay } from '../../../shared/mobile-relay-pairing-offer' -import type { - RelayRevokeOutbox, - RelayDeviceBinding, - RelayRevokeOutboxItem -} from './relay-revoke-outbox' +import type { RelayDeviceBinding, RelayRevokeOutboxItem } from './relay-revoke-outbox' +import { RelayRevokeOutboxFlusher } from './relay-revoke-outbox-flush' import { deriveRelayHostId } from './relay-http-client' import { RelayDemandLedger } from './relay-demand-ledger' import { createRelayRegionPreferenceReader } from './relay-region-preference' @@ -45,7 +42,7 @@ const RELAY_LIVENESS_INTERVAL_MS = 5 * 60_000 export class DesktopRelayService { private readonly coordinator: RelayAuthCoordinator - private readonly revokeOutbox: RelayRevokeOutbox + private readonly revokeFlusher: RelayRevokeOutboxFlusher private readonly runtimeRpc: OrcaRuntimeRpcServer private readonly demandLedger: RelayDemandLedger private readonly hostMobilePairingConnectionMode?: () => MobilePairingConnectionMode @@ -60,11 +57,15 @@ export class DesktopRelayService { throw new Error('mobile_runtime_not_ready') } this.runtimeRpc = options.runtimeRpc - this.revokeOutbox = options.runtimeRpc.getRelayRevokeOutbox() + const revokeOutbox = options.runtimeRpc.getRelayRevokeOutbox() + this.revokeFlusher = new RelayRevokeOutboxFlusher({ + outbox: revokeOutbox, + onDrained: () => this.refreshDemand() + }) this.hostMobilePairingConnectionMode = options.hostMobilePairingConnectionMode this.demandLedger = new RelayDemandLedger({ deviceRegistry: options.runtimeRpc.getDeviceRegistry()!, - revokeOutbox: this.revokeOutbox, + revokeOutbox, relayHostId: deriveRelayHostId(keypair.publicKey), isRelayAllowedForDevice: (deviceId) => this.isRelayAllowedForDevice(deviceId) }) @@ -89,7 +90,7 @@ export class DesktopRelayService { onAssignedCellActive: regionPreference.noteAssignedCell, onStatus: options.onStatus }) - void this.flushRevokeOutbox(broker) + void this.revokeFlusher.flushAll(broker) return broker }, onStatus: options.onStatus @@ -148,7 +149,7 @@ export class DesktopRelayService { broker.hostId === item.relayHostId && broker.ownerIdentityKey === item.ownerIdentityKey ) { - void this.flushRevoke(broker, item) + void this.revokeFlusher.flushItem(broker, item) } } @@ -233,12 +234,6 @@ export class DesktopRelayService { this.coordinator.stop() } - private async flushRevokeOutbox(broker: RelaySessionBroker): Promise { - for (const item of this.revokeOutbox.pendingFor(broker.ownerIdentityKey, broker.hostId)) { - await this.flushRevoke(broker, item) - } - } - private requireMobileDevice(deviceId: string): void { if (this.runtimeRpc.getDeviceRegistry()?.getDevice(deviceId)?.scope !== 'mobile') { throw new Error('mobile_device_not_found') @@ -257,20 +252,6 @@ export class DesktopRelayService { } } - private async flushRevoke( - broker: RelaySessionBroker, - item: RelayRevokeOutboxItem - ): Promise { - try { - await broker.revokeDevice(item.relayDeviceId, item.reqId) - this.revokeOutbox.remove(item.reqId) - this.refreshDemand() - } catch { - // Why: the durable item is the source of truth; reconnecting the same - // account/control retries this stable reqId without delaying local revoke. - } - } - // Restrictive-only: the host setting withdraws Relay from an `automatic` // device, never grants it to a `local-only` one. private isRelayAllowedForDevice(deviceId: string): boolean { diff --git a/src/main/runtime/relay/relay-revoke-outbox-flush.ts b/src/main/runtime/relay/relay-revoke-outbox-flush.ts new file mode 100644 index 000000000000..1bab4e732312 --- /dev/null +++ b/src/main/runtime/relay/relay-revoke-outbox-flush.ts @@ -0,0 +1,42 @@ +import type { RelayRevokeOutbox, RelayRevokeOutboxItem } from './relay-revoke-outbox' + +type RevokingBroker = { + hostId: string + ownerIdentityKey: string + revokeDevice(relayDeviceId: string, reqId: string): Promise +} + +type RelayRevokeOutboxFlushOptions = { + outbox: RelayRevokeOutbox + /** Removing an item can retire the demand holding the broker open. */ + onDrained: () => void +} + +/** + * Drains queued device revocations over a live relay control. Runs unawaited from + * inside the coordinator's broker open, so it outlives its own caller. + */ +export class RelayRevokeOutboxFlusher { + private readonly options: RelayRevokeOutboxFlushOptions + + constructor(options: RelayRevokeOutboxFlushOptions) { + this.options = options + } + + async flushAll(broker: RevokingBroker): Promise { + for (const item of this.options.outbox.pendingFor(broker.ownerIdentityKey, broker.hostId)) { + await this.flushItem(broker, item) + } + } + + async flushItem(broker: RevokingBroker, item: RelayRevokeOutboxItem): Promise { + try { + await broker.revokeDevice(item.relayDeviceId, item.reqId) + this.options.outbox.remove(item.reqId) + this.options.onDrained() + } catch { + // Why: the durable item is the source of truth; reconnecting the same + // account/control retries this stable reqId without delaying local revoke. + } + } +} From ac81bcd63f07a16944fc87709d003c6d081308b9 Mon Sep 17 00:00:00 2001 From: Neil Date: Thu, 10 Sep 2026 18:34:26 -0700 Subject: [PATCH 2/9] fix(relay): make the quit fence hard, and route quit to the terminal stop() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DesktopRelayService.stop() has zero production callers. Quit, relaunch and pre-sign-out all go through fenceAndCloseNow() (main-process-quit.ts, main-window-core-services.ts). That matters because the two are not equivalent: stop() sets `stopped` and clears both timers, while the fence cleared only `livenessTimer`, left `demandExpiryTimer` armed, and left `stopped` false. Its comment — "the next auth mutation re-arms via refreshDemand" — is right for sign-out and exactly wrong for quit. The path production takes was the one that permitted a resurrection, and the suite was exercising the other one, which is why this survived. Three doors re-armed the fence, each measured against a fresh service: - a pending invite-expiry wake: getTimerCount() is 1 immediately after the fence, and the timer fires refreshDemand and reopens the broker - a mint settling after the fence, through withTransientDemand's finally: 0 timers at the fence, 1 once the mint settles, and connect runs again - a power-resume ensureLive(), gated only on `stopped`: connect called twice, a new relay control session opened after quit Two intents, kept apart rather than unified. fenceAndCloseNow gains a `fenced` latch cleared only by start()/authMutated(), so sign-out and relaunch keep the re-arm the old comment protects while the dangerous window between the pre-sign-out fence and the profile wipe stays shut. Quit calls stop() instead, which is terminal; the wire behaviour is unchanged, since coordinator.stop() fences reasonlessly exactly as before. The outbox flush gets the same latch, because it runs unawaited from inside the broker open and a fence can land between two revokes. Deliberate on a vetoed quit: Relay stays down for the session. before-quit already nulls the mobile pairing provider unconditionally, so pairing is dead there regardless — a re-armed broker would be a zombie holding a control session nothing can use. Not fixed here: `fenced` is not set by stop(), so the two latches stay independent and a future caller can still pick the wrong one. --- .../runtime/relay/desktop-relay-service.ts | 31 ++- .../relay/relay-lifecycle-quit-fence.test.ts | 251 ++++++++++++++++++ .../relay/relay-revoke-outbox-flush.ts | 8 + src/main/startup/main-process-quit.ts | 6 +- 4 files changed, 285 insertions(+), 11 deletions(-) create mode 100644 src/main/runtime/relay/relay-lifecycle-quit-fence.test.ts diff --git a/src/main/runtime/relay/desktop-relay-service.ts b/src/main/runtime/relay/desktop-relay-service.ts index 338d906a444f..331ef4851bc6 100644 --- a/src/main/runtime/relay/desktop-relay-service.ts +++ b/src/main/runtime/relay/desktop-relay-service.ts @@ -49,6 +49,8 @@ export class DesktopRelayService { private demandExpiryTimer: ReturnType | null = null private livenessTimer: ReturnType | null = null private stopped = false + // Cleared only by an explicit re-arm (start/authMutated); see fenceAndCloseNow. + private fenced = false constructor(options: DesktopRelayServiceOptions) { const keypair = options.runtimeRpc.getE2EEKeypair() @@ -60,6 +62,7 @@ export class DesktopRelayService { const revokeOutbox = options.runtimeRpc.getRelayRevokeOutbox() this.revokeFlusher = new RelayRevokeOutboxFlusher({ outbox: revokeOutbox, + isHalted: () => this.stopped || this.fenced, onDrained: () => this.refreshDemand() }) this.hostMobilePairingConnectionMode = options.hostMobilePairingConnectionMode @@ -98,28 +101,31 @@ export class DesktopRelayService { } start(): void { + this.fenced = false this.refreshDemand() } // Safe to call from any wake signal (power resume, network change). ensureLive(): void { - if (!this.stopped) { + if (!this.stopped && !this.fenced) { this.coordinator.ensureLive() } } authMutated(): void { + this.fenced = false this.refreshDemand() } + // The re-armable fence: sign-out and relaunch, which want the next auth + // mutation to bring Relay back. Quit wants stop() instead — it is terminal. 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. - if (this.livenessTimer) { - clearInterval(this.livenessTimer) - this.livenessTimer = null - } + // Why a latch rather than just clearing the timers: clearing only covered + // the liveness tick, and everything else outliving the fence lands in + // refreshDemand and re-arms it — a settling mint's finally, a pending + // invite-expiry wake, a power-resume ensureLive. + this.fenced = true + this.clearTimers() this.coordinator.fenceAndCloseNow(hostCloseReason) } @@ -221,8 +227,14 @@ export class DesktopRelayService { this.refreshDemand({ skipLinger: true }) } + // Terminal: no door re-arms this, which is what quit needs. stop(): void { this.stopped = true + this.clearTimers() + this.coordinator.stop() + } + + private clearTimers(): void { if (this.demandExpiryTimer) { clearTimeout(this.demandExpiryTimer) this.demandExpiryTimer = null @@ -231,7 +243,6 @@ export class DesktopRelayService { clearInterval(this.livenessTimer) this.livenessTimer = null } - this.coordinator.stop() } private requireMobileDevice(deviceId: string): void { @@ -308,7 +319,7 @@ export class DesktopRelayService { } private refreshDemand(options?: { skipLinger?: boolean }): void { - if (this.stopped) { + if (this.stopped || this.fenced) { return } if (!this.livenessTimer) { diff --git a/src/main/runtime/relay/relay-lifecycle-quit-fence.test.ts b/src/main/runtime/relay/relay-lifecycle-quit-fence.test.ts new file mode 100644 index 000000000000..c8e517cacb4e --- /dev/null +++ b/src/main/runtime/relay/relay-lifecycle-quit-fence.test.ts @@ -0,0 +1,251 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { OrcaCloudAuthConfig } from '../../orca-profiles/profile-cloud-auth-config' +import type { OrcaRuntimeRpcServer } from '../runtime-rpc' + +// fenceAndCloseNow is what app quit and pre-sign-out both call (stop() is never +// reached in production). Its own comment says the fence must be hard, yet every +// path that survives it lands in refreshDemand, which re-arms the interval the +// fence just cleared and starts a fresh reconcile. +const fakes = vi.hoisted(() => ({ + readRelayAuthContext: vi.fn(), + connect: vi.fn(), + brokers: [] as { closeNow: ReturnType }[] +})) + +vi.mock('./relay-auth-context', () => ({ readRelayAuthContext: fakes.readRelayAuthContext })) + +vi.mock('./relay-session-broker', () => { + class RelaySessionBroker { + readonly hostId: string = hostId + readonly ownerIdentityKey = 'user-1\0profile-1\0org-1' + readonly endpoint = { v: 1 as const, relayHostId: hostId } + readonly closeNow = vi.fn() + createPairingRelay = vi.fn(async (relayDeviceId: string) => ({ + v: 1, + relayHostId: this.hostId, + relayDeviceId, + inviteExpiresAt: 0 + })) + isLive(): boolean { + return this.closeNow.mock.calls.length === 0 + } + static connect = fakes.connect + } + return { RelaySessionBroker } +}) + +import { DesktopRelayService } from './desktop-relay-service' +import { deriveRelayHostId } from './relay-http-client' +import { RelaySessionBroker } from './relay-session-broker' + +const publicKey = new Uint8Array(32).fill(7) +const hostId = deriveRelayHostId(publicKey) + +function deferred() { + let resolve!: (value: T) => void + const promise = new Promise((resolvePromise) => { + resolve = resolvePromise + }) + return { promise, resolve } +} + +type FakeBroker = { + readonly hostId: string + readonly closeNow: ReturnType + createPairingRelay: (relayDeviceId: string) => Promise +} + +function newBroker(): FakeBroker { + const broker = new (RelaySessionBroker as unknown as new () => FakeBroker)() + fakes.brokers.push(broker) + return broker +} + +// A paired phone: standing demand, plus the pending-expiry wake timer +// refreshDemand arms while an invite is still unexpired. +function pairedDevice(inviteExpiresAt?: number) { + return { + deviceId: 'device-1', + scope: 'mobile' as const, + relayBinding: { + relayHostId: hostId, + relayDeviceId: 'device-1', + ownerIdentityKey: 'user-1\0profile-1\0org-1', + ...(inviteExpiresAt === undefined ? {} : { inviteExpiresAt }) + } + } +} + +function service(device: ReturnType): DesktopRelayService { + fakes.readRelayAuthContext.mockResolvedValue({ + identity: { userId: 'user-1', profileId: 'profile-1', organizationId: 'org-1' }, + accessToken: 'access-1', + relayEntitled: true + }) + const runtimeRpc = { + getE2EEKeypair: () => ({ + publicKey, + secretKey: new Uint8Array(32).fill(9), + publicKeyB64: 'x' + }), + getMobileSocketWiring: () => ({ attachTransport: () => () => {} }), + getRelayRevokeOutbox: () => ({ pendingFor: () => [], remove: vi.fn() }), + getDeviceRegistry: () => ({ + listDevices: () => [device], + getDevice: () => device, + getMobilePairingConnectionMode: () => 'automatic' + }) + } as unknown as OrcaRuntimeRpcServer + return new DesktopRelayService({ + authConfig: { + relayDirectorUrl: 'https://relay.example.test', + relayTokenEndpoint: 'https://login.example.test/relay-token' + } as OrcaCloudAuthConfig, + userDataPath: '/tmp/orca-relay-quit-fence', + appVersion: '1.4.188', + runtimeRpc, + onStatus: () => {} + }) +} + +beforeEach(() => { + vi.useFakeTimers() + fakes.brokers.length = 0 + fakes.connect.mockReset() +}) + +afterEach(() => { + vi.useRealTimers() +}) + +describe('DesktopRelayService quit fence is hard', () => { + it('does not let a pending invite-expiry wake resurrect the broker after the fence', async () => { + const device = pairedDevice(Date.now() + 60_000) + fakes.connect.mockImplementation(async () => newBroker()) + const relayService = service(device) + try { + relayService.start() + await vi.advanceTimersByTimeAsync(0) + expect(fakes.connect).toHaveBeenCalledTimes(1) + + relayService.fenceAndCloseNow() + expect(fakes.brokers[0]!.closeNow).toHaveBeenCalled() + // The fence cleared the liveness interval; nothing else may stay armed. + expect(vi.getTimerCount()).toBe(0) + + await vi.advanceTimersByTimeAsync(120_000) + expect(fakes.connect).toHaveBeenCalledTimes(1) + } finally { + relayService.stop() + } + }) + + it('does not let a mint settling after the fence re-arm the liveness tick', async () => { + const device = pairedDevice() + const broker = newBroker() + const mintCall = deferred() + broker.createPairingRelay = vi.fn(async () => { + await mintCall.promise + throw new Error('relay_control_closed') + }) + fakes.connect.mockResolvedValue(broker) + const relayService = service(device) + try { + relayService.start() + await vi.advanceTimersByTimeAsync(0) + const mint = relayService.createPairingRelay('device-1') + const rejection = expect(mint).rejects.toThrow() + await vi.advanceTimersByTimeAsync(0) + expect(broker.createPairingRelay).toHaveBeenCalled() + + relayService.fenceAndCloseNow() + expect(vi.getTimerCount()).toBe(0) + + mintCall.resolve() + await rejection + await vi.advanceTimersByTimeAsync(0) + + // The settling mint's finally must not bring the fenced service back. + expect(vi.getTimerCount()).toBe(0) + expect(fakes.connect).toHaveBeenCalledTimes(1) + + await vi.advanceTimersByTimeAsync(10 * 60_000) + expect(fakes.connect).toHaveBeenCalledTimes(1) + } finally { + relayService.stop() + } + }) + + it('does not let a power-resume ensureLive resurrect the broker after the fence', async () => { + const device = pairedDevice() + fakes.connect.mockImplementation(async () => newBroker()) + const relayService = service(device) + try { + relayService.start() + await vi.advanceTimersByTimeAsync(0) + expect(fakes.connect).toHaveBeenCalledTimes(1) + + relayService.fenceAndCloseNow() + relayService.ensureLive() + await vi.advanceTimersByTimeAsync(0) + expect(fakes.connect).toHaveBeenCalledTimes(1) + } finally { + relayService.stop() + } + }) + + it('still re-arms on the next auth mutation, which is the documented recovery', async () => { + const device = pairedDevice() + fakes.connect.mockImplementation(async () => newBroker()) + const relayService = service(device) + try { + relayService.start() + await vi.advanceTimersByTimeAsync(0) + relayService.fenceAndCloseNow() + + relayService.authMutated() + await vi.advanceTimersByTimeAsync(0) + expect(fakes.connect).toHaveBeenCalledTimes(2) + } finally { + relayService.stop() + } + }) + + it('re-arms on an explicit start, the other door out of the fence', async () => { + const device = pairedDevice() + fakes.connect.mockImplementation(async () => newBroker()) + const relayService = service(device) + try { + relayService.start() + await vi.advanceTimersByTimeAsync(0) + relayService.fenceAndCloseNow() + + relayService.start() + await vi.advanceTimersByTimeAsync(0) + expect(fakes.connect).toHaveBeenCalledTimes(2) + } finally { + relayService.stop() + } + }) + + it('stays fenced after stop, and a second stop changes nothing', async () => { + const device = pairedDevice(Date.now() + 60_000) + fakes.connect.mockImplementation(async () => newBroker()) + const relayService = service(device) + relayService.start() + await vi.advanceTimersByTimeAsync(0) + + relayService.stop() + expect(vi.getTimerCount()).toBe(0) + relayService.stop() + expect(vi.getTimerCount()).toBe(0) + + // stop() is terminal: unlike a fence, no door re-arms it. + relayService.authMutated() + relayService.start() + relayService.ensureLive() + await vi.advanceTimersByTimeAsync(10 * 60_000) + expect(fakes.connect).toHaveBeenCalledTimes(1) + expect(vi.getTimerCount()).toBe(0) + }) +}) diff --git a/src/main/runtime/relay/relay-revoke-outbox-flush.ts b/src/main/runtime/relay/relay-revoke-outbox-flush.ts index 1bab4e732312..404f3cf45d30 100644 --- a/src/main/runtime/relay/relay-revoke-outbox-flush.ts +++ b/src/main/runtime/relay/relay-revoke-outbox-flush.ts @@ -8,6 +8,8 @@ type RevokingBroker = { type RelayRevokeOutboxFlushOptions = { outbox: RelayRevokeOutbox + /** True once the service is fenced or stopped; re-read between items. */ + isHalted: () => boolean /** Removing an item can retire the demand holding the broker open. */ onDrained: () => void } @@ -25,6 +27,12 @@ export class RelayRevokeOutboxFlusher { async flushAll(broker: RevokingBroker): Promise { for (const item of this.options.outbox.pendingFor(broker.ownerIdentityKey, broker.hostId)) { + // Re-checked per item: a fence can land between two revokes. The outbox is + // durable, so deferring to the next launch beats pushing control frames + // through a fence that was supposed to end this session. + if (this.options.isHalted()) { + return + } await this.flushItem(broker, item) } } diff --git a/src/main/startup/main-process-quit.ts b/src/main/startup/main-process-quit.ts index a136981c873b..aff121366212 100644 --- a/src/main/startup/main-process-quit.ts +++ b/src/main/startup/main-process-quit.ts @@ -71,7 +71,11 @@ function installBeforeQuitHandler(): void { }) } state.isQuitting = true - state.desktopRelayService?.fenceAndCloseNow() + // Why stop() and not the fence: the fence is re-armable by design (sign-out + // waits for the next auth mutation), so at quit a settling mint, an invite + // expiry or a power-resume could still reopen a broker. Quit is terminal, + // and the provider null below already ends pairing even on a vetoed quit. + state.desktopRelayService?.stop() state.runtimeRpc?.setMobileRelayPairingProvider(null) state.unsubscribeAgentAwakeStatusChanges?.() state.unsubscribeAgentAwakeStatusChanges = null From 3ec7b8a634edbcd09a74dfe94dd65269251b450b Mon Sep 17 00:00:00 2001 From: Neil Date: Thu, 10 Sep 2026 18:35:01 -0700 Subject: [PATCH 3/9] fix(relay): drain the revoke outbox on one broker, not one open per item MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit flushAll runs unawaited from inside the coordinator's openBroker, so the broker it is draining is not registered yet. Refreshing demand after every removal therefore reconciled against a null ownership: each reconcile opened a fresh director assignment and abandoned the broker still working through the loop. Measured with three queued revocations and a real RelayRevokeOutbox on disk: 3 items produced 3 RelaySessionBroker.connect calls and 2 abandoned brokers, against a relay that rate-limits assignment. After: 1 connect, 3 revokes, one `connecting` transition, the draining broker never closed, outbox empty. Reconcile once after the loop instead, and only when something was actually removed. flushItem keeps its own refresh because onDeviceRevokeQueued calls it directly on an already-registered broker, where retiring the last demand is exactly what should reach standby. Tests use the durable file as the assertion rather than a mock, and cover the fence landing before the open resolves, the fence landing between two items, the multi-item drain, the last-demand revoke reaching standby, and the same reqId queued twice (dedup is the control layer's job — duplicate_relay_request_id — so the service must only settle both without stranding the item). Mutation note: moving onDrained one await later, inside the loop after flushItem returns, survives. The extra microtask lets the coordinator register the broker first, so no extra open occurs and the variant is genuinely inert here; the assertion does catch the real pre-fix placement, synchronous inside the revoke after remove. --- ...y-lifecycle-revoke-outbox-crossing.test.ts | 286 ++++++++++++++++++ .../relay/relay-revoke-outbox-flush.ts | 30 +- 2 files changed, 311 insertions(+), 5 deletions(-) create mode 100644 src/main/runtime/relay/relay-lifecycle-revoke-outbox-crossing.test.ts diff --git a/src/main/runtime/relay/relay-lifecycle-revoke-outbox-crossing.test.ts b/src/main/runtime/relay/relay-lifecycle-revoke-outbox-crossing.test.ts new file mode 100644 index 000000000000..3660083fb0f7 --- /dev/null +++ b/src/main/runtime/relay/relay-lifecycle-revoke-outbox-crossing.test.ts @@ -0,0 +1,286 @@ +import { mkdtempSync, readFileSync, rmSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { OrcaCloudAuthConfig } from '../../orca-profiles/profile-cloud-auth-config' +import type { OrcaRuntimeRpcServer } from '../runtime-rpc' + +// The outbox flush is launched unawaited from inside the coordinator's open, so +// it crosses every boundary the open does: a fence, a reconnect that replaced +// the broker it was handed, and a second revoke queued for the same reqId. +const fakes = vi.hoisted(() => ({ + readRelayAuthContext: vi.fn(), + connect: vi.fn() +})) + +vi.mock('./relay-auth-context', () => ({ readRelayAuthContext: fakes.readRelayAuthContext })) + +vi.mock('./relay-session-broker', () => { + class RelaySessionBroker { + readonly hostId: string = hostId + readonly ownerIdentityKey = ownerIdentityKey + readonly endpoint = { v: 1 as const, relayHostId: hostId } + readonly closeNow = vi.fn() + revokeDevice = vi.fn(async () => {}) + isLive(): boolean { + return this.closeNow.mock.calls.length === 0 + } + static connect = fakes.connect + } + return { RelaySessionBroker } +}) + +import { DesktopRelayService } from './desktop-relay-service' +import { deriveRelayHostId } from './relay-http-client' +import { RelayRevokeOutbox, type RelayRevokeOutboxItem } from './relay-revoke-outbox' +import { RelaySessionBroker } from './relay-session-broker' + +const publicKey = new Uint8Array(32).fill(7) +const hostId = deriveRelayHostId(publicKey) +const ownerIdentityKey = 'user-1\0profile-1\0org-1' + +function deferred() { + let resolve!: (value: T) => void + let reject!: (error: Error) => void + const promise = new Promise((resolvePromise, rejectPromise) => { + resolve = resolvePromise + reject = rejectPromise + }) + return { promise, resolve, reject } +} + +type FakeBroker = { + readonly hostId: string + readonly closeNow: ReturnType + revokeDevice: ReturnType +} + +function newBroker(): FakeBroker { + return new (RelaySessionBroker as unknown as new () => FakeBroker)() +} + +let userDataPath: string + +function outboxOnDisk(): RelayRevokeOutboxItem[] { + return JSON.parse( + readFileSync(join(userDataPath, 'mobile-relay-revoke-outbox.json'), 'utf-8') + ) as RelayRevokeOutboxItem[] +} + +// A paired phone, so standing demand survives the outbox draining to empty and +// the broker stays registered for the next queued revoke. +const pairedDevice = { + deviceId: 'device-paired', + scope: 'mobile' as const, + relayBinding: { relayHostId: hostId, relayDeviceId: 'device-paired', ownerIdentityKey } +} + +function service( + outbox: RelayRevokeOutbox, + onStatus: (status: string) => void = () => {}, + devices: (typeof pairedDevice)[] = [] +): DesktopRelayService { + fakes.readRelayAuthContext.mockResolvedValue({ + identity: { userId: 'user-1', profileId: 'profile-1', organizationId: 'org-1' }, + accessToken: 'access-1', + relayEntitled: true + }) + const runtimeRpc = { + getE2EEKeypair: () => ({ + publicKey, + secretKey: new Uint8Array(32).fill(9), + publicKeyB64: 'x' + }), + getMobileSocketWiring: () => ({ attachTransport: () => () => {} }), + getRelayRevokeOutbox: () => outbox, + getDeviceRegistry: () => ({ + listDevices: () => devices, + getDevice: () => ({ deviceId: 'device-1', scope: 'mobile' }), + getMobilePairingConnectionMode: () => 'automatic' + }) + } as unknown as OrcaRuntimeRpcServer + return new DesktopRelayService({ + authConfig: { + relayDirectorUrl: 'https://relay.example.test', + relayTokenEndpoint: 'https://login.example.test/relay-token' + } as OrcaCloudAuthConfig, + userDataPath, + appVersion: '1.4.188', + runtimeRpc, + onStatus + }) +} + +function queued(outbox: RelayRevokeOutbox, relayDeviceId: string): RelayRevokeOutboxItem { + return outbox.enqueue({ relayHostId: hostId, relayDeviceId, ownerIdentityKey }) +} + +beforeEach(() => { + vi.useFakeTimers() + fakes.connect.mockReset() + userDataPath = mkdtempSync(join(tmpdir(), 'orca-relay-outbox-')) +}) + +afterEach(() => { + vi.useRealTimers() + rmSync(userDataPath, { recursive: true, force: true }) +}) + +describe('revoke-outbox flush crossing a fence', () => { + it('does not issue control revokes through a broker the fence already discarded', async () => { + const outbox = new RelayRevokeOutbox(userDataPath) + queued(outbox, 'device-1') + const open = deferred() + fakes.connect.mockImplementation(() => open.promise) + const relayService = service(outbox) + try { + relayService.start() + await vi.advanceTimersByTimeAsync(0) + expect(fakes.connect).toHaveBeenCalledTimes(1) + + relayService.fenceAndCloseNow() + const broker = newBroker() + open.resolve(broker) + await vi.advanceTimersByTimeAsync(0) + + expect(broker.revokeDevice).not.toHaveBeenCalled() + expect(broker.closeNow).toHaveBeenCalled() + // Durable, so deferring to the next launch loses nothing. + expect(outboxOnDisk()).toHaveLength(1) + } finally { + relayService.stop() + } + }) + + it('stops mid-flush when the fence lands between two items', async () => { + const outbox = new RelayRevokeOutbox(userDataPath) + queued(outbox, 'device-1') + queued(outbox, 'device-2') + const broker = newBroker() + const firstRevoke = deferred() + broker.revokeDevice = vi.fn(async () => await firstRevoke.promise) + fakes.connect.mockResolvedValue(broker) + const relayService = service(outbox) + try { + relayService.start() + await vi.advanceTimersByTimeAsync(0) + expect(broker.revokeDevice).toHaveBeenCalledTimes(1) + + relayService.fenceAndCloseNow() + firstRevoke.resolve() + await vi.advanceTimersByTimeAsync(0) + + // device-1 really was revoked, so its item is gone; device-2 never went out. + expect(broker.revokeDevice).toHaveBeenCalledTimes(1) + expect(outboxOnDisk().map((item) => item.relayDeviceId)).toEqual(['device-2']) + } finally { + relayService.stop() + } + }) +}) + +describe('revoke-outbox flush driving the coordinator', () => { + it('drains a multi-item outbox on one broker instead of one open per item', async () => { + // The flush is launched from inside openBroker, so its per-item demand + // refresh reconciled against an ownership that was not registered yet: each + // removal opened a fresh director assignment and abandoned the broker still + // working through the loop. Measured before the fix: 3 opens for 3 items. + const outbox = new RelayRevokeOutbox(userDataPath) + queued(outbox, 'device-1') + queued(outbox, 'device-2') + queued(outbox, 'device-3') + const brokers: FakeBroker[] = [] + const statuses: string[] = [] + fakes.connect.mockImplementation(async () => { + const broker = newBroker() + brokers.push(broker) + return broker + }) + const relayService = service(outbox, (status) => statuses.push(status)) + try { + relayService.start() + await vi.advanceTimersByTimeAsync(0) + + expect(fakes.connect).toHaveBeenCalledTimes(1) + expect(brokers[0]!.revokeDevice).toHaveBeenCalledTimes(3) + // One assignment: each extra `connecting` is a director open the drain + // caused and then abandoned. + expect(statuses.filter((status) => status === 'connecting')).toHaveLength(1) + expect(brokers[0]!.closeNow).not.toHaveBeenCalled() + expect(outboxOnDisk()).toEqual([]) + } finally { + relayService.stop() + } + }) + + it('still reconciles once the drain is done so the relay can reach standby', async () => { + const outbox = new RelayRevokeOutbox(userDataPath) + queued(outbox, 'device-1') + const statuses: string[] = [] + const broker = newBroker() + fakes.connect.mockResolvedValue(broker) + const relayService = service(outbox, (status) => statuses.push(status)) + try { + relayService.start() + await vi.advanceTimersByTimeAsync(0) + expect(outboxOnDisk()).toEqual([]) + // The last demand is gone, so the drained broker must not stay registered. + expect(statuses.at(-1)).toBe('standby') + } finally { + relayService.stop() + } + }) + + it('reaches standby when a queued revoke removes the last standing demand', async () => { + // The runtime drops the device from the registry and then queues the relay + // revoke, so this flush is what retires the demand holding the control open. + const outbox = new RelayRevokeOutbox(userDataPath) + const devices = [pairedDevice] + const statuses: string[] = [] + const broker = newBroker() + fakes.connect.mockResolvedValue(broker) + const relayService = service(outbox, (status) => statuses.push(status), devices) + try { + relayService.start() + await vi.advanceTimersByTimeAsync(0) + expect(statuses.at(-1)).toBe('registered') + + devices.length = 0 + relayService.onDeviceRevokeQueued(queued(outbox, pairedDevice.deviceId)) + await vi.advanceTimersByTimeAsync(0) + + expect(broker.revokeDevice).toHaveBeenCalledTimes(1) + expect(statuses.at(-1)).toBe('standby') + } finally { + relayService.stop() + } + }) + + it('settles both flushes when the same revoke is queued twice', async () => { + const outbox = new RelayRevokeOutbox(userDataPath) + const broker = newBroker() + fakes.connect.mockResolvedValue(broker) + const relayService = service(outbox, () => {}, [pairedDevice]) + try { + relayService.start() + await vi.advanceTimersByTimeAsync(0) + + // enqueue returns the existing item for a repeat, so both carry one reqId. + const item = queued(outbox, 'device-1') + expect(queued(outbox, 'device-1').reqId).toBe(item.reqId) + relayService.onDeviceRevokeQueued(item) + relayService.onDeviceRevokeQueued(item) + await vi.advanceTimersByTimeAsync(0) + + // Dedup is the control layer's job (duplicate_relay_request_id); the + // service must simply settle both without stranding the item. + expect(broker.revokeDevice.mock.calls.map((call) => call[1])).toEqual([ + item.reqId, + item.reqId + ]) + expect(outboxOnDisk()).toEqual([]) + } finally { + relayService.stop() + } + }) +}) diff --git a/src/main/runtime/relay/relay-revoke-outbox-flush.ts b/src/main/runtime/relay/relay-revoke-outbox-flush.ts index 404f3cf45d30..1d825ea925cb 100644 --- a/src/main/runtime/relay/relay-revoke-outbox-flush.ts +++ b/src/main/runtime/relay/relay-revoke-outbox-flush.ts @@ -10,13 +10,18 @@ type RelayRevokeOutboxFlushOptions = { outbox: RelayRevokeOutbox /** True once the service is fenced or stopped; re-read between items. */ isHalted: () => boolean - /** Removing an item can retire the demand holding the broker open. */ + /** Called once per flush that emptied at least one item, never on a no-op. */ onDrained: () => void } /** * Drains queued device revocations over a live relay control. Runs unawaited from - * inside the coordinator's broker open, so it outlives its own caller. + * inside the coordinator's broker open, so it outlives its own caller and has to + * re-check the halt latch itself. + * + * Shared shape with the post-quit re-arm this landed beside: an operation that + * reconciles against state it assumes is registered, from a context where it is + * not — here the broker is not registered yet, there the service is already closed. */ export class RelayRevokeOutboxFlusher { private readonly options: RelayRevokeOutboxFlushOptions @@ -26,25 +31,40 @@ export class RelayRevokeOutboxFlusher { } async flushAll(broker: RevokingBroker): Promise { + let drained = false for (const item of this.options.outbox.pendingFor(broker.ownerIdentityKey, broker.hostId)) { // Re-checked per item: a fence can land between two revokes. The outbox is // durable, so deferring to the next launch beats pushing control frames // through a fence that was supposed to end this session. if (this.options.isHalted()) { - return + break } - await this.flushItem(broker, item) + drained = (await this.revoke(broker, item)) || drained + } + // Why once, after the loop: this runs from inside the coordinator's open, so + // `broker` is not registered yet. Reconciling per item read a null ownership + // and opened a fresh director assignment for every remaining revoke, each one + // abandoning the broker still working through the loop. + if (drained) { + this.options.onDrained() } } async flushItem(broker: RevokingBroker, item: RelayRevokeOutboxItem): Promise { + if (await this.revoke(broker, item)) { + this.options.onDrained() + } + } + + private async revoke(broker: RevokingBroker, item: RelayRevokeOutboxItem): Promise { try { await broker.revokeDevice(item.relayDeviceId, item.reqId) this.options.outbox.remove(item.reqId) - this.options.onDrained() + return true } catch { // Why: the durable item is the source of truth; reconnecting the same // account/control retries this stable reqId without delaying local revoke. + return false } } } From 0ba31b2145b3adea2408906eb309045058374ac7 Mon Sep 17 00:00:00 2001 From: Neil Date: Thu, 10 Sep 2026 18:35:23 -0700 Subject: [PATCH 4/9] fix(relay): stop a mid-read profile switch reporting as a sign-out MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Same taxonomy error 8972d744 fixed for the session file, in the other null exit. readRelayAuthContext re-reads the active profile after the refresh, because refresh and org selection can rewrite cloud linkage in flight. It then collapsed two unrelated outcomes into null: if (!cloud || refreshed.profile.id !== active.profile.id) return null A profile switch landing inside that window is not "the cloud session is gone", which is the contract the coordinator's own comment says null carries. Measured before: offlineReason "signed-out" — the terminal wire reason every paired phone latches as "sign in on your desktop to reconnect", arming no retry — for a race the switch's own authMutated() re-reads moments later. After: auth_unavailable, which is retryable. Split the condition: throw for the id mismatch, keep null for a profile that genuinely has no cloud linkage. Still unfixed, and not reachable from here: ensureActiveOrcaProfile treats an unreadable profile index the same as a corrupt one — readProfileIndexFile catches every error including EACCES/EMFILE, index and .bak fail together under descriptor exhaustion, and it fabricates a default local profile and writes it over the unreadable file. readRelayAuthContext then sees no cloud and reports a sign-out honestly, because by that point the index really has been replaced. That one belongs in profile-index-store.ts and changes app-wide sign-in semantics, the same caveat 8972d744 raised for decrypt-failed. --- src/main/runtime/relay/relay-auth-context.ts | 9 +- ...fecycle-auth-context-identity-race.test.ts | 92 +++++++++++++++++++ 2 files changed, 100 insertions(+), 1 deletion(-) create mode 100644 src/main/runtime/relay/relay-lifecycle-auth-context-identity-race.test.ts diff --git a/src/main/runtime/relay/relay-auth-context.ts b/src/main/runtime/relay/relay-auth-context.ts index 53f156e8915b..090a1f79975e 100644 --- a/src/main/runtime/relay/relay-auth-context.ts +++ b/src/main/runtime/relay/relay-auth-context.ts @@ -24,8 +24,15 @@ export async function readRelayAuthContext( // Why: refresh and org-selection can rewrite cloud linkage while the request // is in flight; identity must come from the post-refresh profile state. const refreshed = ensureActiveOrcaProfile(userDataPath) + // Why throw rather than return null, same argument as the unreadable session above: + // a profile switch landing inside this read is not a sign-out, and null spends the + // terminal SIGNED_OUT — latched by every paired phone, arming no retry — on a race + // the switch's own auth mutation re-reads moments later. + if (refreshed.profile.id !== active.profile.id) { + throw new Error('orca_profile_switched_during_read') + } const cloud = refreshed.profile.cloud - if (!cloud || refreshed.profile.id !== active.profile.id) { + if (!cloud) { return null } return { diff --git a/src/main/runtime/relay/relay-lifecycle-auth-context-identity-race.test.ts b/src/main/runtime/relay/relay-lifecycle-auth-context-identity-race.test.ts new file mode 100644 index 000000000000..7e2f9a0b7379 --- /dev/null +++ b/src/main/runtime/relay/relay-lifecycle-auth-context-identity-race.test.ts @@ -0,0 +1,92 @@ +import { describe, expect, it, vi } from 'vitest' +import { RELAY_HOST_CLOSE_REASON } from '../../../shared/relay-host-close-reason' + +// Same taxonomy question 8972d744 answered for the session file, asked of the +// other null exit: readRelayAuthContext re-reads the profile after the refresh, +// and a profile switch landing in that window is not a sign-out. +const fakes = vi.hoisted(() => ({ + ensureActiveOrcaProfile: vi.fn(), + readFreshOrcaCloudSession: vi.fn() +})) + +vi.mock('../../orca-profiles/profile-index-store', () => ({ + ensureActiveOrcaProfile: fakes.ensureActiveOrcaProfile +})) +vi.mock('../../orca-profiles/profile-cloud-session-refresh', () => ({ + readFreshOrcaCloudSession: fakes.readFreshOrcaCloudSession +})) + +import { readRelayAuthContext } from './relay-auth-context' +import { RelayAuthCoordinator } from './relay-auth-coordinator' + +const authConfig = {} as never + +function profile(id: string, cloud: object | null) { + return { profile: { id, ...(cloud ? { cloud } : {}) } } +} + +const signedIn = { userId: 'u1', cloudProfileId: 'cp1', activeOrgId: 'org-1' } + +function foundSession() { + return { + status: 'found', + session: { accessToken: 'access-1', capabilities: { flags: { 'relay.use': true } } } + } +} + +describe('readRelayAuthContext profile-switch race', () => { + it('refuses to call a mid-read profile switch a sign-out', async () => { + // ensureActiveOrcaProfile is called twice — once before the refresh and once + // after, because refresh and org selection can rewrite cloud linkage. A + // switch between them means "re-read", not "the cloud session is gone". + fakes.ensureActiveOrcaProfile + .mockReturnValueOnce(profile('profile-1', signedIn)) + .mockReturnValueOnce(profile('profile-2', signedIn)) + fakes.readFreshOrcaCloudSession.mockResolvedValue(foundSession()) + + await expect(readRelayAuthContext(authConfig, '/tmp/x')).rejects.toThrow( + 'orca_profile_switched_during_read' + ) + }) + + it('classifies the switch as auth_unavailable, never signed_out', async () => { + fakes.ensureActiveOrcaProfile + .mockReturnValueOnce(profile('profile-1', signedIn)) + .mockReturnValueOnce(profile('profile-2', signedIn)) + fakes.readFreshOrcaCloudSession.mockResolvedValue(foundSession()) + const broker = { closeNow: vi.fn() } + const coordinator = new RelayAuthCoordinator({ + readContext: () => readRelayAuthContext(authConfig, '/tmp/x'), + openBroker: async () => broker, + onStatus: vi.fn() + }) + coordinator.reconcile() + + const result = await coordinator.waitForLiveBrokerResult(0) + expect(result).toEqual({ broker: null, offlineReason: 'auth_unavailable' }) + // SIGNED_OUT is terminal on the wire — the phone latches it and arms no retry. + expect(broker.closeNow).not.toHaveBeenCalledWith(RELAY_HOST_CLOSE_REASON.SIGNED_OUT) + }) + + it('still reports a profile that genuinely has no cloud linkage as gone', async () => { + fakes.ensureActiveOrcaProfile + .mockReturnValueOnce(profile('profile-1', signedIn)) + .mockReturnValueOnce(profile('profile-1', null)) + fakes.readFreshOrcaCloudSession.mockResolvedValue(foundSession()) + + await expect(readRelayAuthContext(authConfig, '/tmp/x')).resolves.toBeNull() + }) + + it('returns the post-refresh identity when the profile held still', async () => { + fakes.ensureActiveOrcaProfile + .mockReturnValueOnce(profile('profile-1', signedIn)) + .mockReturnValueOnce(profile('profile-1', { ...signedIn, activeOrgId: 'org-2' })) + fakes.readFreshOrcaCloudSession.mockResolvedValue(foundSession()) + + await expect(readRelayAuthContext(authConfig, '/tmp/x')).resolves.toEqual({ + identity: { userId: 'u1', profileId: 'cp1', organizationId: 'org-2' }, + accessToken: 'access-1', + relayEntitled: true + }) + }) +}) From 67d950c1c6c907819deda5b5fdcf0733bf7c0093 Mon Sep 17 00:00:00 2001 From: Neil Date: Thu, 10 Sep 2026 18:35:37 -0700 Subject: [PATCH 5/9] test(relay): pin RelayControlClient teardown before, and twice after, connect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Characterisation only — no defect found, which is the result worth recording, because the same audit found the service-level fence was not hard. Covers closeNow() at 'idle' before any socket exists, connect() refused after teardown, a control request issued before connect, and closeNow() twice from both 'opening' and 'active'. Asserts the specific fields rather than absence of errors: socket handle dropped so the second pass cannot re-terminate, onClose not re-fired, pendingRequestCount back to 0, and vi.getTimerCount() 0 so neither the connect deadline nor the silence watchdog outlives the close. Mutation-verified: dropping the idle guard in connect(), `this.socket = null`, requests.rejectAll, or silenceWatchdog.stop() each fails one of these. --- .../relay-lifecycle-control-teardown.test.ts | 144 ++++++++++++++++++ 1 file changed, 144 insertions(+) create mode 100644 src/main/runtime/relay/relay-lifecycle-control-teardown.test.ts diff --git a/src/main/runtime/relay/relay-lifecycle-control-teardown.test.ts b/src/main/runtime/relay/relay-lifecycle-control-teardown.test.ts new file mode 100644 index 000000000000..1f7685540a19 --- /dev/null +++ b/src/main/runtime/relay/relay-lifecycle-control-teardown.test.ts @@ -0,0 +1,144 @@ +import { EventEmitter } from 'node:events' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import nacl from 'tweetnacl' +import type WebSocket from 'ws' +import { RelayControlClient } from './relay-control-client' + +// Teardown that runs before — or twice after — the first successful connect. +// Every field closeNow touches has to be initialised at 'idle', and a second +// pass must not re-fire onClose or leave the connect deadline armed. +class FakeSocket extends EventEmitter { + readonly OPEN = 1 + readyState = 0 + readonly sent: string[] = [] + readonly terminate = vi.fn() + readonly close = vi.fn() + + send(payload: string): void { + this.sent.push(payload) + } + + open(): void { + this.readyState = this.OPEN + this.emit('open') + } + + deliver(message: object): void { + this.emit('message', Buffer.from(JSON.stringify(message)), false) + } +} + +const keys = nacl.box.keyPair() + +function client(onClose = vi.fn()): { control: RelayControlClient; sockets: FakeSocket[] } { + const sockets: FakeSocket[] = [] + const control = new RelayControlClient({ + cellUrl: 'http://127.0.0.1:1/', + relayJwt: 'scoped-token', + relayHostId: 'AbCdEf0123_-xyZ9', + assignmentEpoch: 1, + identity: { userId: 'user-1', profileId: 'profile-1', organizationId: 'org-1' }, + keypair: { ...keys, publicKeyB64: Buffer.from(keys.publicKey).toString('base64') }, + appVersion: '1.4.188', + onConnectionOpen: vi.fn(), + onDrain: vi.fn(), + onClose, + createSocket: () => { + const socket = new FakeSocket() + sockets.push(socket) + return socket as unknown as WebSocket + } + }) + return { control, sockets } +} + +beforeEach(() => vi.useFakeTimers()) +afterEach(() => vi.useRealTimers()) + +describe('RelayControlClient teardown before the first connect', () => { + it('closes cleanly at idle, when no socket or watchdog was ever created', () => { + const onClose = vi.fn() + const { control, sockets } = client(onClose) + + expect(control.isLive()).toBe(false) + expect(() => control.closeNow()).not.toThrow() + + expect(sockets).toHaveLength(0) + expect(onClose).not.toHaveBeenCalled() + expect(control.pendingRequestCount).toBe(0) + expect(vi.getTimerCount()).toBe(0) + }) + + it('refuses a connect after teardown rather than reopening into a closed client', async () => { + const { control, sockets } = client() + control.closeNow() + + await expect(control.connect()).rejects.toThrow('relay_control_already_started') + expect(sockets).toHaveLength(0) + expect(vi.getTimerCount()).toBe(0) + }) + + it('rejects a control request issued before connect without stranding a deadline', async () => { + const { control } = client() + + await expect(control.revokeDevice('device-1', 'req-1')).rejects.toThrow( + 'relay_control_not_active' + ) + expect(control.pendingRequestCount).toBe(0) + expect(vi.getTimerCount()).toBe(0) + }) +}) + +describe('RelayControlClient teardown twice', () => { + it('settles the connect once and disarms the deadline on the first close', async () => { + const onClose = vi.fn() + const { control, sockets } = client(onClose) + const connecting = control.connect() + expect(vi.getTimerCount()).toBe(1) + + control.closeNow() + await expect(connecting).rejects.toThrow('relay_control_closed') + expect(vi.getTimerCount()).toBe(0) + expect(sockets[0]!.terminate).toHaveBeenCalledTimes(1) + + control.closeNow() + // The socket handle is dropped on the first pass, so the second is inert. + expect(sockets[0]!.terminate).toHaveBeenCalledTimes(1) + expect(onClose).not.toHaveBeenCalled() + expect(vi.getTimerCount()).toBe(0) + }) + + it('leaves nothing armed when an active control is closed twice', async () => { + const { control, sockets } = client() + const connecting = control.connect() + const socket = sockets[0]! + socket.open() + socket.deliver({ + type: 'host-hello-ack', + v: 1, + generation: 1, + controlResumeSecret: 'A'.repeat(43), + leaseExpiresAt: Date.now() + 60_000, + activeConnIds: [], + pendingConns: [] + }) + await connecting + expect(control.isLive()).toBe(true) + // Only the silence watchdog: activation already disarmed the connect deadline. + expect(vi.getTimerCount()).toBe(1) + + const pending = control.createInvite('device-1', 'req-1') + const rejection = expect(pending).rejects.toThrow('relay_control_closed') + expect(control.pendingRequestCount).toBe(1) + + control.closeNow() + await rejection + expect(control.isLive()).toBe(false) + expect(control.pendingRequestCount).toBe(0) + expect(vi.getTimerCount()).toBe(0) + + control.closeNow() + expect(vi.getTimerCount()).toBe(0) + expect(socket.terminate).toHaveBeenCalledTimes(1) + }) +}) From 9dd6f92c0e9b28f556771608c5906b9c7927dbe8 Mon Sep 17 00:00:00 2001 From: Neil Date: Thu, 10 Sep 2026 18:38:53 -0700 Subject: [PATCH 6/9] docs(relay): cross-reference the terminal stop() and the re-armable fence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit main-process-quit.ts has no test harness — single export, ~20 heavy imports including electron — so the quit call site's choice between these two is pinned only by the service-level contract. The risk is not that the contract is untested; it is that someone editing the quit handler has no way to find it. Each method now names the other, says which lifecycle events call it, and says what the other one is for. Grep from either end lands on the counterpart, which is the only protection available when one side cannot be tested. --- src/main/runtime/relay/desktop-relay-service.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/main/runtime/relay/desktop-relay-service.ts b/src/main/runtime/relay/desktop-relay-service.ts index 331ef4851bc6..fad1985e7a44 100644 --- a/src/main/runtime/relay/desktop-relay-service.ts +++ b/src/main/runtime/relay/desktop-relay-service.ts @@ -117,8 +117,10 @@ export class DesktopRelayService { this.refreshDemand() } - // The re-armable fence: sign-out and relaunch, which want the next auth - // mutation to bring Relay back. Quit wants stop() instead — it is terminal. + // The re-armable fence, by design: sign-out and relaunch call this + // (main-window-core-services.ts onBeforeOrcaProfileSignOut / onBeforeRelaunch) + // and want the next authMutated to bring Relay back. Quit deliberately does + // NOT use this — it calls stop(), which is terminal. Keep the two apart. fenceAndCloseNow(hostCloseReason?: RelayHostCloseReason): void { // Why a latch rather than just clearing the timers: clearing only covered // the liveness tick, and everything else outliving the fence lands in @@ -227,7 +229,9 @@ export class DesktopRelayService { this.refreshDemand({ skipLinger: true }) } - // Terminal: no door re-arms this, which is what quit needs. + // Terminal: no door re-arms this. Quit calls it (main-process-quit.ts + // before-quit). The re-armable counterpart is fenceAndCloseNow, which quit + // must not use — a settling mint or an invite expiry re-arms that one. stop(): void { this.stopped = true this.clearTimers() From a63e57aa8ddb114c2a582b48afa5c62f78795639 Mon Sep 17 00:00:00 2001 From: Neil Date: Thu, 10 Sep 2026 18:42:18 -0700 Subject: [PATCH 7/9] test(relay): ratchet which teardown each lifecycle event calls MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the gap the cross-reference comments could not: a reader arriving at stop() or fenceAndCloseNow() now finds the other, but nothing failed if someone edited main-process-quit.ts back to the fence, and that is the edit that reintroduces the defect. No harness needed — this reads the source text, like the other ratchets in src/main. Both methods take an optional argument and return void, so swapping them compiles clean and passes every unit test; the difference is that stop() is terminal and the fence is re-armable, and it only surfaces in timer state minutes later. This is the compile error that swap cannot produce. Both directions are asserted, because collapsing them either way destroys the distinction the defect came from: quit must not regain the re-armable fence, and sign-out/relaunch must not lose it. The regexes are qualified by receiver on purpose. The quit handler stops five unrelated services, so a bare `.stop()` would match rateLimits, starNag and the rest. Verified by mutation, both directions and a control: reverting quit to fenceAndCloseNow fails the first test only, collapsing sign-out onto stop() fails the second only, and renaming an unrelated service's stop() fires neither. --- ...p-relay-teardown-call-site-ratchet.test.ts | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) create mode 100644 src/main/startup/desktop-relay-teardown-call-site-ratchet.test.ts diff --git a/src/main/startup/desktop-relay-teardown-call-site-ratchet.test.ts b/src/main/startup/desktop-relay-teardown-call-site-ratchet.test.ts new file mode 100644 index 000000000000..e091b4bcba47 --- /dev/null +++ b/src/main/startup/desktop-relay-teardown-call-site-ratchet.test.ts @@ -0,0 +1,47 @@ +import { readFileSync } from 'node:fs' +import { join } from 'node:path' +import { describe, expect, it } from 'vitest' + +/** + * Guard which DesktopRelayService teardown each lifecycle event calls. + * + * `stop()` is terminal; `fenceAndCloseNow()` is re-armable by design, so sign-out comes back on the + * next auth mutation. Nothing in the types says so — both take an optional argument and return + * void, so swapping one for the other compiles clean and passes every unit test. Quit used the + * fence, and a settling mint, a pending invite expiry or a power-resume `ensureLive` each reopened + * a broker after quit; the damage only shows up in timer state, minutes later. + * + * This test is the compile error that swap cannot produce. Both directions matter: quit must not + * regain the re-armable fence, and sign-out/relaunch must not lose it — collapsing them either way + * destroys the distinction the defect came from. + */ +const STARTUP_DIR = __dirname +const QUIT_MODULE = 'main-process-quit.ts' +const AUTH_LIFECYCLE_MODULE = 'main-window-core-services.ts' + +// Qualified by the receiver on purpose: the quit handler stops five unrelated services, so a bare +// `.stop()` would match `rateLimits`, `starNag`, `automations` and the rest. Literal enough that +// hoisting the call behind a local also trips it — a refactor that hides which teardown runs at +// quit is exactly what should get a second look. +const RELAY_TERMINAL_STOP = /desktopRelayService\?\.stop\(\)/ +const RELAY_REARMABLE_FENCE = /desktopRelayService\?\.fenceAndCloseNow\(/ + +function startupSource(module: string): string { + return readFileSync(join(STARTUP_DIR, module), 'utf8') +} + +describe('desktop relay teardown call sites', () => { + it('ends the relay terminally at quit, never through the re-armable fence', () => { + const source = startupSource(QUIT_MODULE) + + expect(source).toMatch(RELAY_TERMINAL_STOP) + expect(source).not.toMatch(RELAY_REARMABLE_FENCE) + }) + + it('keeps sign-out and relaunch on the fence, so the next auth mutation re-arms', () => { + const source = startupSource(AUTH_LIFECYCLE_MODULE) + + expect(source).toMatch(RELAY_REARMABLE_FENCE) + expect(source).not.toMatch(RELAY_TERMINAL_STOP) + }) +}) From c0907f78c8b41238a843a027613c9413e3aac590 Mon Sep 17 00:00:00 2001 From: Neil Date: Thu, 10 Sep 2026 18:44:49 -0700 Subject: [PATCH 8/9] docs(relay): address the teardown ratchet's docstring to whoever it goes red on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reader who decides this test's fate is not the reviewer — it is someone hitting a red ratchet during an unrelated refactor, weighing whether to relax the pattern. Say the trade to them directly: a ratchet that fails loudly on a benign change beats one that passes silently on a harmful one, and the literal match is deliberate because a refactor that obscures which teardown runs at quit is exactly what should get a second look. --- .../startup/desktop-relay-teardown-call-site-ratchet.test.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/main/startup/desktop-relay-teardown-call-site-ratchet.test.ts b/src/main/startup/desktop-relay-teardown-call-site-ratchet.test.ts index e091b4bcba47..bd30447ae5bb 100644 --- a/src/main/startup/desktop-relay-teardown-call-site-ratchet.test.ts +++ b/src/main/startup/desktop-relay-teardown-call-site-ratchet.test.ts @@ -14,6 +14,10 @@ import { describe, expect, it } from 'vitest' * This test is the compile error that swap cannot produce. Both directions matter: quit must not * regain the re-armable fence, and sign-out/relaunch must not lose it — collapsing them either way * destroys the distinction the defect came from. + * + * If you are here because this went red on a refactor that looks harmless, that is the intended + * trade: a ratchet that fails loudly on a benign change beats one that passes silently on a + * harmful one. Re-read which teardown your call site now runs before relaxing the pattern. */ const STARTUP_DIR = __dirname const QUIT_MODULE = 'main-process-quit.ts' From 77d8b28aa2b22ca1dca4e3dea8161dec1a2cff11 Mon Sep 17 00:00:00 2001 From: Neil Date: Wed, 16 Sep 2026 23:41:07 -0700 Subject: [PATCH 9/9] fix(relay): make the quit teardown terminal on the committed handler, not the vetoable one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Moving quit off the re-armable fence was right; putting the terminal stop() in `before-quit` was not. That handler is vetoable — a renderer beforeunload can cancel the quit — and nothing anywhere clears `stopped`. `refreshDemand` returns early on it, so a quit the user backed out of left Relay dead for the rest of the session, with no liveness tick, no reconcile and no door back. The comment three lines below already warns about exactly this, and `desktopPushService` in the same file is split across the two handlers for exactly this reason. `before-quit` now fences: it closes the broker just as promptly and latches `fenced`, which `refreshDemand` also refuses, so the settling mint, the invite-expiry wake and the power-resume `ensureLive` still cannot reopen one in the window before `will-quit`. On a veto the next auth mutation clears `fenced` and Relay comes back. `will-quit` — the committed path — calls stop() and makes it terminal. The original goal is intact; only the irrecoverable case is gone. The prior justification ("the provider null below already ends pairing even on a vetoed quit") does not hold up: that is a second defect on the vetoed-quit path, not a licence to add a third. The call-site ratchet guarded the file, which could not see this — the quit module held both a latch and a veto. It now resolves each handler body and guards them separately: the vetoable one must fence and must not latch, the committed one the reverse. Mutation-verified — restoring the `before-quit` stop() fails both new cases. --- ...p-relay-teardown-call-site-ratchet.test.ts | 38 +++++++++++++++---- src/main/startup/main-process-quit.ts | 17 ++++++--- 2 files changed, 43 insertions(+), 12 deletions(-) diff --git a/src/main/startup/desktop-relay-teardown-call-site-ratchet.test.ts b/src/main/startup/desktop-relay-teardown-call-site-ratchet.test.ts index bd30447ae5bb..2971786165bc 100644 --- a/src/main/startup/desktop-relay-teardown-call-site-ratchet.test.ts +++ b/src/main/startup/desktop-relay-teardown-call-site-ratchet.test.ts @@ -11,9 +11,14 @@ import { describe, expect, it } from 'vitest' * fence, and a settling mint, a pending invite expiry or a power-resume `ensureLive` each reopened * a broker after quit; the damage only shows up in timer state, minutes later. * - * This test is the compile error that swap cannot produce. Both directions matter: quit must not - * regain the re-armable fence, and sign-out/relaunch must not lose it — collapsing them either way - * destroys the distinction the defect came from. + * The quit *module* is not the unit — the handler is. `before-quit` is vetoable (a renderer + * beforeunload can cancel it) and nothing clears `stopped`, so a terminal stop() there kills Relay + * for the rest of the session on a quit the user backed out of. `will-quit` is the committed path. + * The file already splits `desktopPushService` exactly this way, for exactly this reason. + * + * This test is the compile error those swaps cannot produce. Every direction matters: the vetoable + * handler must not latch, the committed handler must, and sign-out/relaunch must keep the fence — + * collapsing any pair destroys the distinction the defect came from. * * If you are here because this went red on a refactor that looks harmless, that is the intended * trade: a ratchet that fails loudly on a benign change beats one that passes silently on a @@ -34,12 +39,31 @@ function startupSource(module: string): string { return readFileSync(join(STARTUP_DIR, module), 'utf8') } +/** Body of one `function installHandler()`, up to the next top-level `function`. */ +function handlerBody(source: string, installer: string): string { + const start = source.indexOf(`function ${installer}(`) + expect(start, `${installer} not found — the ratchet cannot see what it guards`).toBeGreaterThan( + -1 + ) + const rest = source.slice(start) + const end = rest.indexOf('\nfunction ', 1) + return end === -1 ? rest : rest.slice(0, end) +} + describe('desktop relay teardown call sites', () => { - it('ends the relay terminally at quit, never through the re-armable fence', () => { - const source = startupSource(QUIT_MODULE) + it('fences the relay on the vetoable quit handler, and never latches it there', () => { + const beforeQuit = handlerBody(startupSource(QUIT_MODULE), 'installBeforeQuitHandler') + + expect(beforeQuit).toMatch(RELAY_REARMABLE_FENCE) + // The latch has nothing that clears it, so a vetoed quit would end Relay for the session. + expect(beforeQuit).not.toMatch(RELAY_TERMINAL_STOP) + }) + + it('ends the relay terminally on the committed quit handler', () => { + const willQuit = handlerBody(startupSource(QUIT_MODULE), 'installWillQuitHandler') - expect(source).toMatch(RELAY_TERMINAL_STOP) - expect(source).not.toMatch(RELAY_REARMABLE_FENCE) + expect(willQuit).toMatch(RELAY_TERMINAL_STOP) + expect(willQuit).not.toMatch(RELAY_REARMABLE_FENCE) }) it('keeps sign-out and relaunch on the fence, so the next auth mutation re-arms', () => { diff --git a/src/main/startup/main-process-quit.ts b/src/main/startup/main-process-quit.ts index aff121366212..bea3355dd816 100644 --- a/src/main/startup/main-process-quit.ts +++ b/src/main/startup/main-process-quit.ts @@ -71,11 +71,14 @@ function installBeforeQuitHandler(): void { }) } state.isQuitting = true - // Why stop() and not the fence: the fence is re-armable by design (sign-out - // waits for the next auth mutation), so at quit a settling mint, an invite - // expiry or a power-resume could still reopen a broker. Quit is terminal, - // and the provider null below already ends pairing even on a vetoed quit. - state.desktopRelayService?.stop() + // Why the fence here and the terminal stop() in will-quit: this handler is vetoable, and + // stop() latches `stopped` with nothing to clear it — a renderer beforeunload that cancelled + // the quit would leave the relay dead for the rest of the session. The fence closes the broker + // just as promptly and latches `fenced`, which refreshDemand also refuses, so a settling mint, + // an invite expiry or a power-resume cannot reopen one in the window before will-quit either. + // On a veto the next auth mutation clears `fenced` and Relay comes back; on the committed path + // will-quit makes it terminal. Same reasoning as desktopPushService, one handler down. + state.desktopRelayService?.fenceAndCloseNow() state.runtimeRpc?.setMobileRelayPairingProvider(null) state.unsubscribeAgentAwakeStatusChanges?.() state.unsubscribeAgentAwakeStatusChanges = null @@ -111,6 +114,10 @@ function installWillQuitHandler(): void { } // A renderer can veto before-quit; push must survive until quit is committed. state.desktopPushService?.stop() + // Same reason, and the one the fence in before-quit defers to: stop() is terminal and nothing + // clears `stopped`, so it only belongs on the committed path. before-quit already fenced the + // broker closed; this is what stops it coming back. + state.desktopRelayService?.stop() state.unsubscribeSystemResumeBroadcast?.() state.unsubscribeSystemResumeBroadcast = null // Why: renderer guards can still cancel before this committed phase; `log stream` must survive those vetoes.