diff --git a/src/main/runtime/relay/relay-demand-ledger.ts b/src/main/runtime/relay/relay-demand-ledger.ts index 4fe77a45b1c..7aad8959d45 100644 --- a/src/main/runtime/relay/relay-demand-ledger.ts +++ b/src/main/runtime/relay/relay-demand-ledger.ts @@ -89,10 +89,15 @@ export class RelayDemandLedger { return true } } - if (this.options.revokeOutbox.pendingFor(ownerIdentityKey, this.options.relayHostId).length) { + const now = (this.options.now ?? Date.now)() + // Why `demandingFor` and not `pendingFor`: a revoke the server rejects permanently is never + // removed, and this check is deliberately unfiltered by the host's pairing policy, so counting + // every pending item held the relay up forever. The item still retries; only its demand expires. + if ( + this.options.revokeOutbox.demandingFor(ownerIdentityKey, this.options.relayHostId, now).length + ) { return true } - const now = (this.options.now ?? Date.now)() return this.options.deviceRegistry.listDevices().some((device) => { const binding = this.demandCandidateBinding(device) if (!binding || binding.ownerIdentityKey !== ownerIdentityKey) { diff --git a/src/main/runtime/relay/relay-revoke-outbox.ts b/src/main/runtime/relay/relay-revoke-outbox.ts index 5989af0c283..342df3e39b2 100644 --- a/src/main/runtime/relay/relay-revoke-outbox.ts +++ b/src/main/runtime/relay/relay-revoke-outbox.ts @@ -21,6 +21,10 @@ export type RelayRevokeOutboxItem = RelayDeviceBinding & { const OUTBOX_FILENAME = 'mobile-relay-revoke-outbox.json' +/** How long an unlanded revoke keeps forcing the relay online on its own. It is retried forever + * regardless; this bounds only its claim on demand. */ +export const RELAY_REVOKE_DEMAND_WINDOW_MS = 24 * 60 * 60_000 + function isItem(value: unknown): value is RelayRevokeOutboxItem { if (!value || typeof value !== 'object') { return false @@ -72,6 +76,31 @@ export class RelayRevokeOutbox { ) } + /** + * The pending revokes that still justify forcing the relay online by themselves. + * + * Why this is narrower than `pendingFor`: an item is removed only on a SUCCESSFUL flush, so one + * the server rejects permanently stays pending forever, and demand counts pending revokes + * unfiltered by the host's pairing-connection policy. That combination pinned the relay up for + * the life of the install and defeated a `local-only` pick outright. + * + * What deliberately does NOT happen here is giving up on the revoke. The item stays in the + * outbox and keeps retrying on every connection, because failing to revoke leaves a live + * credential on the relay and silently discarding that intent is the worse hazard. Only its + * claim on demand expires: a revoke that has not landed in this long is not going to land + * because the relay is held open, so holding it open buys nothing and costs the user the + * setting they chose. + */ + demandingFor( + ownerIdentityKey: string, + relayHostId: string, + now: number + ): readonly RelayRevokeOutboxItem[] { + return this.pendingFor(ownerIdentityKey, relayHostId).filter( + (item) => now - item.createdAt < RELAY_REVOKE_DEMAND_WINDOW_MS + ) + } + remove(reqId: string): void { const next = this.items.filter((item) => item.reqId !== reqId) if (next.length === this.items.length) { diff --git a/src/main/runtime/relay/relay-revoke-permanent-failure.test.ts b/src/main/runtime/relay/relay-revoke-permanent-failure.test.ts new file mode 100644 index 00000000000..975e536a2bf --- /dev/null +++ b/src/main/runtime/relay/relay-revoke-permanent-failure.test.ts @@ -0,0 +1,101 @@ +import { mkdtempSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { describe, expect, it, vi } from 'vitest' +import { DeviceRegistry } from '../device-registry' +import { RelayDemandLedger } from './relay-demand-ledger' +import { + RELAY_REVOKE_DEMAND_WINDOW_MS, + RelayRevokeOutbox, + type RelayDeviceBinding +} from './relay-revoke-outbox' +import { DesktopRelayService } from './desktop-relay-service' + +// What this pins: a revoke that can never succeed must not hold relay demand forever. +// +// `hasDemand` counts a pending revoke deliberately unfiltered by the host's pairing-connection +// policy — the credential has to be killed on the relay whatever the user has since chosen. That +// is right for a revoke that can still land, and wrong for one that never will: the item is only +// removed on a SUCCESSFUL flush, and `flushRevoke` swallowed every failure with no attempt cap and +// no expiry. A permanently-rejected revoke therefore pinned demand true for the life of the +// install and defeated a `local-only` pick entirely (#19870's regression path). + +const ownerIdentityKey = 'user-1\0profile-1\0org-1' +const relayHostId = 'relay-host-1' + +function fixture(now: () => number) { + const userDataPath = mkdtempSync(join(tmpdir(), 'orca-relay-revoke-')) + const deviceRegistry = new DeviceRegistry(userDataPath) + const revokeOutbox = new RelayRevokeOutbox(userDataPath) + const ledger = new RelayDemandLedger({ deviceRegistry, revokeOutbox, relayHostId, now }) + return { deviceRegistry, revokeOutbox, ledger } +} + +function binding(relayDeviceId: string): RelayDeviceBinding { + return { relayDeviceId, relayHostId, ownerIdentityKey } +} + +/** A broker whose revoke is rejected permanently, the way a deleted device or a revoked + * credential is rejected: the server is reachable and says no, every time. */ +function permanentlyFailingBroker(revokeDevice: ReturnType) { + return { ownerIdentityKey, hostId: relayHostId, revokeDevice } as never +} + +function serviceOver(revokeOutbox: RelayRevokeOutbox, ledger: RelayDemandLedger) { + const service = Object.create(DesktopRelayService.prototype) as DesktopRelayService + Object.assign(service, { + revokeOutbox, + demandLedger: ledger, + coordinator: { reconcile: vi.fn(), ensureLive: vi.fn(), getActiveBroker: () => null }, + stopped: false, + livenessTimer: null, + demandExpiryTimer: null + }) + return service as unknown as { flushRevokeOutbox: (broker: unknown) => Promise } +} + +describe('a revoke that can never succeed', () => { + it('stops pinning relay demand once its window passes, but is never abandoned', async () => { + let now = Date.now() + const { revokeOutbox, ledger } = fixture(() => now) + revokeOutbox.enqueue(binding('device-1')) + expect(ledger.hasDemand(ownerIdentityKey)).toBe(true) + + const revokeDevice = vi.fn().mockRejectedValue(new Error('device_not_found')) + const service = serviceOver(revokeOutbox, ledger) + + // Every reconnect re-drives the outbox against the same stable reqId. + for (let attempt = 0; attempt < 50; attempt += 1) { + await service.flushRevokeOutbox(permanentlyFailingBroker(revokeDevice)) + } + // Inside the window the relay is still held up on purpose: the revoke may yet land. + expect(ledger.hasDemand(ownerIdentityKey)).toBe(true) + + now += RELAY_REVOKE_DEMAND_WINDOW_MS + 1 + expect(ledger.hasDemand(ownerIdentityKey)).toBe(false) + + // The intent is NOT discarded. Dropping it would leave a live credential on the relay with + // nothing left to kill it, so the item stays and keeps retrying on any future connection. + expect(revokeOutbox.pendingFor(ownerIdentityKey, relayHostId)).toHaveLength(1) + const callsBefore = revokeDevice.mock.calls.length + await service.flushRevokeOutbox(permanentlyFailingBroker(revokeDevice)) + expect(revokeDevice.mock.calls.length).toBe(callsBefore + 1) + }) + + it('still lets a revoke that lands remove the item and release demand', async () => { + let now = Date.now() + const { revokeOutbox, ledger } = fixture(() => now) + revokeOutbox.enqueue(binding('device-1')) + const revokeDevice = vi.fn().mockResolvedValue(undefined) + const service = serviceOver(revokeOutbox, ledger) + + await service.flushRevokeOutbox(permanentlyFailingBroker(revokeDevice)) + + expect(revokeDevice).toHaveBeenCalledTimes(1) + expect(revokeOutbox.pendingFor(ownerIdentityKey, relayHostId)).toHaveLength(0) + expect(ledger.hasDemand(ownerIdentityKey)).toBe(false) + // And it was removed on success, not merely aged out. + now += RELAY_REVOKE_DEMAND_WINDOW_MS + 1 + expect(ledger.hasDemand(ownerIdentityKey)).toBe(false) + }) +})