diff --git a/src/main/runtime/relay/desktop-relay-service.ts b/src/main/runtime/relay/desktop-relay-service.ts index f108fda29689..fad1985e7a44 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,13 +42,15 @@ 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 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,11 +59,16 @@ 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, + isHalted: () => this.stopped || this.fenced, + 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 +93,7 @@ export class DesktopRelayService { onAssignedCellActive: regionPreference.noteAssignedCell, onStatus: options.onStatus }) - void this.flushRevokeOutbox(broker) + void this.revokeFlusher.flushAll(broker) return broker }, onStatus: options.onStatus @@ -97,28 +101,33 @@ 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, 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 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) } @@ -148,7 +157,7 @@ export class DesktopRelayService { broker.hostId === item.relayHostId && broker.ownerIdentityKey === item.ownerIdentityKey ) { - void this.flushRevoke(broker, item) + void this.revokeFlusher.flushItem(broker, item) } } @@ -220,8 +229,16 @@ export class DesktopRelayService { this.refreshDemand({ skipLinger: true }) } + // 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() + this.coordinator.stop() + } + + private clearTimers(): void { if (this.demandExpiryTimer) { clearTimeout(this.demandExpiryTimer) this.demandExpiryTimer = null @@ -230,13 +247,6 @@ export class DesktopRelayService { clearInterval(this.livenessTimer) this.livenessTimer = null } - 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 { @@ -257,20 +267,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 { @@ -327,7 +323,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-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 + }) + }) +}) 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) + }) +}) 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-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 new file mode 100644 index 000000000000..1d825ea925cb --- /dev/null +++ b/src/main/runtime/relay/relay-revoke-outbox-flush.ts @@ -0,0 +1,70 @@ +import type { RelayRevokeOutbox, RelayRevokeOutboxItem } from './relay-revoke-outbox' + +type RevokingBroker = { + hostId: string + ownerIdentityKey: string + revokeDevice(relayDeviceId: string, reqId: string): Promise +} + +type RelayRevokeOutboxFlushOptions = { + outbox: RelayRevokeOutbox + /** True once the service is fenced or stopped; re-read between items. */ + isHalted: () => boolean + /** 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 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 + + constructor(options: RelayRevokeOutboxFlushOptions) { + this.options = options + } + + 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()) { + break + } + 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) + 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 + } + } +} 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..2971786165bc --- /dev/null +++ b/src/main/startup/desktop-relay-teardown-call-site-ratchet.test.ts @@ -0,0 +1,75 @@ +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. + * + * 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 + * 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' +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') +} + +/** 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('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(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', () => { + const source = startupSource(AUTH_LIFECYCLE_MODULE) + + expect(source).toMatch(RELAY_REARMABLE_FENCE) + expect(source).not.toMatch(RELAY_TERMINAL_STOP) + }) +}) diff --git a/src/main/startup/main-process-quit.ts b/src/main/startup/main-process-quit.ts index a136981c873b..bea3355dd816 100644 --- a/src/main/startup/main-process-quit.ts +++ b/src/main/startup/main-process-quit.ts @@ -71,6 +71,13 @@ function installBeforeQuitHandler(): void { }) } state.isQuitting = true + // 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?.() @@ -107,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.