From 8a8e62e5606d6b3a30de37468739ba7a4d9da879 Mon Sep 17 00:00:00 2001 From: Neil Date: Thu, 10 Sep 2026 17:45:37 -0700 Subject: [PATCH] fix(relay): stop a revoke that can never land from pinning the relay online MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reproduced first: enqueue a revoke, reject it 50 times the way a deleted device is rejected, and `hasDemand` is still true. It stays true for the life of the install. Two deliberate properties combine into the bug. `flushRevoke` removes the outbox item only on a SUCCESSFUL flush and swallows every failure in a bare `catch {}` with no attempt cap and no expiry, so a permanently-rejected revoke is pending forever. And the revoke arm of `hasDemand` is deliberately NOT filtered by the host's pairing-connection policy, because a credential must be killed on the relay whatever the user has since chosen. Forever-pending plus policy-exempt means the relay is held online permanently and a `local-only` pick is defeated outright — the regression path for what #19870 closes, previously with no test at all. THE CHOICE, stated because the convenient one is wrong. The revoke is NOT abandoned. Failing to revoke leaves a live credential on the relay, so discarding the intent to tidy up demand would trade a visible stuck relay for an invisible live credential. The item stays in the outbox and keeps retrying on every future connection, unchanged. What expires is only its CLAIM ON DEMAND, via `demandingFor` alongside the unchanged `pendingFor`: after 24h an unlanded revoke stops being a reason to force the relay online BY ITSELF. The reasoning is that holding the relay open is not what makes the revoke succeed — it has had a day of attempts — so holding it open buys nothing and costs the user the setting they chose. A revoke that can still land is unaffected inside the window, and one that lands is removed as before. Uses the existing `createdAt`, so nothing new is persisted and the window survives restart. Mutation-tested: putting `pendingFor` back in `hasDemand` fails the new test at the post-window assertion. The success path is pinned separately so the fix cannot be mistaken for aging out an item that should have been removed. Audited the rest of the file for the same shape, since a bare `catch {}` on a queue is rarely alone: `flushRevoke` is the only one in desktop-relay-service.ts. --- src/main/runtime/relay/relay-demand-ledger.ts | 9 +- src/main/runtime/relay/relay-revoke-outbox.ts | 29 +++++ .../relay-revoke-permanent-failure.test.ts | 101 ++++++++++++++++++ 3 files changed, 137 insertions(+), 2 deletions(-) create mode 100644 src/main/runtime/relay/relay-revoke-permanent-failure.test.ts diff --git a/src/main/runtime/relay/relay-demand-ledger.ts b/src/main/runtime/relay/relay-demand-ledger.ts index 8bcc1a6a2df..a43e08d00d9 100644 --- a/src/main/runtime/relay/relay-demand-ledger.ts +++ b/src/main/runtime/relay/relay-demand-ledger.ts @@ -37,10 +37,15 @@ export class RelayDemandLedger { if (this.transientRefs.size > 0) { 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 = device.relayBinding if ( 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) + }) +})