mirror of
https://github.com/stablyai/orca.git
synced 2026-09-28 16:02:45 +00:00
fix(relay): scope nextPendingExpiry the way hasDemand is scoped
`hasDemand` requires four things of a standing binding — mobile scope, matching
owner identity, matching relay host, and allowed by the live pairing policy.
`nextPendingExpiry` applied NONE of them: it took the minimum `inviteExpiresAt`
across every device in the registry. So it armed the `demandExpiryTimer` wake in
`refreshDemand` off state that provably cannot produce demand.
Three cases measured returning a wake time where `hasDemand` was already false:
a binding for a different relayHostId, a runtime-scope (non-mobile) device, and
a phone the live LAN policy excludes.
Low severity on its own — the fired timer just reconciles to no demand — but it
is spurious wakeup churn from the class that owns the correct predicate three
lines above, which is the kind of drift that stops being harmless later. The
shared clause is now `demandCandidateBinding`; the owner check stays in
`hasDemand` because `nextPendingExpiry` takes no identity. The `hasDemand` side
is a pure conjunction reorder, confirmed behaviour-preserving by mutation rather
than by eye.
Re-arming after a policy exclusion is safe: `pairingPolicyChanged()` calls
`refreshDemand()`, so a flip back to automatic restores the timer. That round
trip is asserted rather than assumed.
Also records two things next to the code that would otherwise be lost:
- Transient refs carry NO OWNER IDENTITY, so `hasDemand`'s transient loop answers
true for any signed-in identity while the two branches below it filter on
`ownerIdentityKey`. Nothing defends the current behaviour OR that regression:
scoping the loop by owner fails exactly one test across the whole relay suite,
the characterisation test added for it. Deliberately not fixed here, and the
comment says why the in-file version is unsafe —
`withTransientDemand('provision')` calls `setMobileRelayBinding` INSIDE the
operation, so during a re-pair the device still holds the old owner's binding
and an inferred filter would drop demand mid-provision, tearing the broker down
under the operation holding the ref. The real fix threads identity through
`acquireTransient`, whose call site is desktop-relay-service.ts.
- The `if (!current) return` guard in the release closure is unreachable today
and mutating it away breaks nothing. Kept and labelled INERT rather than
deleted: it goes load-bearing the moment the ledger grows a dispose()/clear(),
and nothing in the suite would catch that.
The ref-counting core itself came back clean under every hazard exercised:
policy flip between acquire and release (the release closure is policy-blind by
construction, and the demand filter is a live pull per call rather than an
acquire-time snapshot, so neither a permanent pin nor a dropped-but-needed
demand); double release; opposite-order release of two refs on one key; and
cross-device release.
This commit is contained in:
@@ -0,0 +1,248 @@
|
||||
import { mkdtempSync } from 'node:fs'
|
||||
import { tmpdir } from 'node:os'
|
||||
import { join } from 'node:path'
|
||||
import { afterEach, describe, expect, it, vi } from 'vitest'
|
||||
import { DeviceRegistry } from '../device-registry'
|
||||
import type { MobilePairingConnectionMode } from '../../../shared/mobile-pairing-connection-mode'
|
||||
import { isMobileRelayAllowed } from '../../../shared/mobile-relay-policy'
|
||||
import { RelayDemandLedger } from './relay-demand-ledger'
|
||||
import { RelayRevokeOutbox, type RelayDeviceBinding } from './relay-revoke-outbox'
|
||||
|
||||
const ownerIdentityKey = 'user-1\0profile-1\0org-1'
|
||||
const otherOwnerIdentityKey = 'user-2\0profile-2\0org-2'
|
||||
const relayHostId = 'relay-host-1'
|
||||
|
||||
/** Mirrors DesktopRelayService.isRelayAllowedForDevice: a live pull, not a snapshot. */
|
||||
function fixture(now = 1_000) {
|
||||
const userDataPath = mkdtempSync(join(tmpdir(), 'orca-relay-demand-lifecycle-'))
|
||||
const deviceRegistry = new DeviceRegistry(userDataPath)
|
||||
const revokeOutbox = new RelayRevokeOutbox(userDataPath)
|
||||
const host = { mode: 'automatic' as MobilePairingConnectionMode }
|
||||
const ledger = new RelayDemandLedger({
|
||||
deviceRegistry,
|
||||
revokeOutbox,
|
||||
relayHostId,
|
||||
now: () => now,
|
||||
isRelayAllowedForDevice: (deviceId) =>
|
||||
isMobileRelayAllowed({
|
||||
hostConnectionMode: host.mode,
|
||||
deviceConnectionMode: deviceRegistry.getMobilePairingConnectionMode(deviceId) ?? null
|
||||
})
|
||||
})
|
||||
return { userDataPath, deviceRegistry, revokeOutbox, ledger, host }
|
||||
}
|
||||
|
||||
function binding(relayDeviceId: string, inviteExpiresAt?: number): RelayDeviceBinding {
|
||||
return { relayDeviceId, relayHostId, ownerIdentityKey, inviteExpiresAt }
|
||||
}
|
||||
|
||||
function pairedPhone(fx: ReturnType<typeof fixture>, name = 'Phone') {
|
||||
const phone = fx.deviceRegistry.addDevice(name)
|
||||
fx.deviceRegistry.setMobilePairingConnectionMode(phone.deviceId, 'automatic')
|
||||
fx.deviceRegistry.setRelayBinding(phone.deviceId, binding(phone.deviceId))
|
||||
fx.deviceRegistry.updateLastSeen(phone.deviceId)
|
||||
return phone.deviceId
|
||||
}
|
||||
|
||||
describe('RelayDemandLedger transient-ref lifecycle', () => {
|
||||
afterEach(() => vi.useRealTimers())
|
||||
|
||||
// Hazard 1: the live policy flips between acquire and release.
|
||||
it('releases a ref acquired before a policy flip instead of leaking it', () => {
|
||||
const fx = fixture()
|
||||
const deviceId = pairedPhone(fx)
|
||||
const release = fx.ledger.acquireTransient(`endpoints:${deviceId}`, deviceId)
|
||||
expect(fx.ledger.hasDemand(ownerIdentityKey)).toBe(true)
|
||||
|
||||
fx.host.mode = 'local-only'
|
||||
expect(fx.ledger.hasDemand(ownerIdentityKey)).toBe(false)
|
||||
|
||||
// The release path must be policy-blind, or the ref is unreachable forever.
|
||||
release()
|
||||
expect(fx.ledger.transientRefsForTests().size).toBe(0)
|
||||
|
||||
fx.host.mode = 'automatic'
|
||||
// Standing binding demand returns; the released ref must not add a second pin.
|
||||
expect(fx.ledger.hasDemand(ownerIdentityKey)).toBe(true)
|
||||
fx.deviceRegistry.removeDevice(deviceId)
|
||||
expect(fx.ledger.hasDemand(ownerIdentityKey)).toBe(false)
|
||||
})
|
||||
|
||||
it('restores a held ref as demand when the policy flips back', () => {
|
||||
const fx = fixture()
|
||||
const deviceId = pairedPhone(fx)
|
||||
fx.deviceRegistry.removeDevice(deviceId)
|
||||
const release = fx.ledger.acquireTransient(`pairing:${deviceId}`, deviceId)
|
||||
|
||||
fx.host.mode = 'local-only'
|
||||
expect(fx.ledger.hasDemand(ownerIdentityKey)).toBe(false)
|
||||
// The filter must be re-evaluated per call, not baked in at acquire time.
|
||||
fx.host.mode = 'automatic'
|
||||
expect(fx.ledger.hasDemand(ownerIdentityKey)).toBe(true)
|
||||
release()
|
||||
expect(fx.ledger.hasDemand(ownerIdentityKey)).toBe(false)
|
||||
})
|
||||
|
||||
// Hazard 2: the same release closure invoked repeatedly.
|
||||
it('a repeated release does not decrement a count another holder still owns', () => {
|
||||
const fx = fixture()
|
||||
const deviceId = pairedPhone(fx)
|
||||
fx.deviceRegistry.removeDevice(deviceId)
|
||||
const key = `endpoints:${deviceId}`
|
||||
const releaseFirst = fx.ledger.acquireTransient(key, deviceId)
|
||||
const releaseSecond = fx.ledger.acquireTransient(key, deviceId)
|
||||
expect(fx.ledger.transientRefsForTests().get(key)?.count).toBe(2)
|
||||
|
||||
releaseFirst()
|
||||
releaseFirst()
|
||||
releaseFirst()
|
||||
expect(fx.ledger.transientRefsForTests().get(key)?.count).toBe(1)
|
||||
expect(fx.ledger.hasDemand(ownerIdentityKey)).toBe(true)
|
||||
|
||||
releaseSecond()
|
||||
expect(fx.ledger.transientRefsForTests().size).toBe(0)
|
||||
expect(fx.ledger.hasDemand(ownerIdentityKey)).toBe(false)
|
||||
})
|
||||
|
||||
it('a stale release from a finished ref does not cancel a later acquire of the same key', () => {
|
||||
const fx = fixture()
|
||||
const deviceId = pairedPhone(fx)
|
||||
fx.deviceRegistry.removeDevice(deviceId)
|
||||
const key = `provision:${deviceId}`
|
||||
const stale = fx.ledger.acquireTransient(key, deviceId)
|
||||
stale()
|
||||
expect(fx.ledger.transientRefsForTests().size).toBe(0)
|
||||
|
||||
const fresh = fx.ledger.acquireTransient(key, deviceId)
|
||||
stale()
|
||||
expect(fx.ledger.transientRefsForTests().get(key)?.count).toBe(1)
|
||||
expect(fx.ledger.hasDemand(ownerIdentityKey)).toBe(true)
|
||||
fresh()
|
||||
expect(fx.ledger.transientRefsForTests().size).toBe(0)
|
||||
})
|
||||
|
||||
// Hazard 3: an operation throws and its ref is never released.
|
||||
it('an unreleased ref pins demand and retains its map entry', () => {
|
||||
const fx = fixture()
|
||||
const deviceId = pairedPhone(fx)
|
||||
fx.deviceRegistry.removeDevice(deviceId)
|
||||
for (let index = 0; index < 5; index += 1) {
|
||||
fx.ledger.acquireTransient(`pairing:abandoned-${index}`, `abandoned-${index}`)
|
||||
}
|
||||
expect(fx.ledger.transientRefsForTests().size).toBe(5)
|
||||
expect(fx.ledger.hasDemand(ownerIdentityKey)).toBe(true)
|
||||
})
|
||||
|
||||
// Hazard 4: two refs for one device released in the opposite order.
|
||||
it('is release-order independent for refs sharing a key', () => {
|
||||
const fx = fixture()
|
||||
const deviceId = pairedPhone(fx)
|
||||
fx.deviceRegistry.removeDevice(deviceId)
|
||||
const key = `endpoints:${deviceId}`
|
||||
const outer = fx.ledger.acquireTransient(key, deviceId)
|
||||
const inner = fx.ledger.acquireTransient(key, deviceId)
|
||||
inner()
|
||||
expect(fx.ledger.hasDemand(ownerIdentityKey)).toBe(true)
|
||||
const middle = fx.ledger.acquireTransient(key, deviceId)
|
||||
outer()
|
||||
expect(fx.ledger.transientRefsForTests().get(key)?.count).toBe(1)
|
||||
middle()
|
||||
expect(fx.ledger.transientRefsForTests().size).toBe(0)
|
||||
})
|
||||
|
||||
// Hazard 5: a release for device B must not disturb device A's ref.
|
||||
it('keeps per-device refs independent across a concurrent release', () => {
|
||||
const fx = fixture()
|
||||
const deviceA = fx.deviceRegistry.addDevice('Phone A').deviceId
|
||||
const deviceB = fx.deviceRegistry.addDevice('Phone B').deviceId
|
||||
fx.deviceRegistry.setMobilePairingConnectionMode(deviceA, 'local-only')
|
||||
fx.deviceRegistry.setMobilePairingConnectionMode(deviceB, 'automatic')
|
||||
|
||||
const releaseA = fx.ledger.acquireTransient(`pairing:${deviceA}`, deviceA)
|
||||
const releaseB = fx.ledger.acquireTransient(`pairing:${deviceB}`, deviceB)
|
||||
// A is excluded by its own pair-time mode; B alone carries the demand.
|
||||
expect(fx.ledger.hasDemand(ownerIdentityKey)).toBe(true)
|
||||
|
||||
releaseB()
|
||||
expect(fx.ledger.transientRefsForTests().size).toBe(1)
|
||||
expect(fx.ledger.transientRefsForTests().get(`pairing:${deviceA}`)?.deviceId).toBe(deviceA)
|
||||
expect(fx.ledger.hasDemand(ownerIdentityKey)).toBe(false)
|
||||
|
||||
releaseA()
|
||||
expect(fx.ledger.transientRefsForTests().size).toBe(0)
|
||||
})
|
||||
|
||||
// Hazard 6: the ledger owns no timers, so teardown is the caller's map only.
|
||||
it('arms no timers of its own across acquire, query and release', () => {
|
||||
vi.useFakeTimers()
|
||||
const fx = fixture()
|
||||
const deviceId = pairedPhone(fx)
|
||||
fx.deviceRegistry.setRelayBinding(deviceId, binding(deviceId, 2_000))
|
||||
const release = fx.ledger.acquireTransient(`pairing:${deviceId}`, deviceId)
|
||||
expect(fx.ledger.hasDemand(ownerIdentityKey)).toBe(true)
|
||||
expect(fx.ledger.nextPendingExpiry()).toBe(2_000)
|
||||
release()
|
||||
expect(vi.getTimerCount()).toBe(0)
|
||||
})
|
||||
})
|
||||
|
||||
describe('RelayDemandLedger nextPendingExpiry scoping', () => {
|
||||
it('ignores a pending invite bound to a different relay host', () => {
|
||||
const fx = fixture()
|
||||
const phone = fx.deviceRegistry.addDevice('Foreign-host phone')
|
||||
fx.deviceRegistry.setRelayBinding(phone.deviceId, {
|
||||
...binding(phone.deviceId, 2_000),
|
||||
relayHostId: 'relay-host-2'
|
||||
})
|
||||
expect(fx.ledger.hasDemand(ownerIdentityKey)).toBe(false)
|
||||
expect(fx.ledger.nextPendingExpiry()).toBeNull()
|
||||
})
|
||||
|
||||
it('ignores a pending invite on a non-mobile device', () => {
|
||||
const fx = fixture()
|
||||
const runtime = fx.deviceRegistry.addDevice('Runtime', 'runtime')
|
||||
fx.deviceRegistry.setRelayBinding(runtime.deviceId, binding(runtime.deviceId, 2_000))
|
||||
expect(fx.ledger.hasDemand(ownerIdentityKey)).toBe(false)
|
||||
expect(fx.ledger.nextPendingExpiry()).toBeNull()
|
||||
})
|
||||
|
||||
it('ignores a pending invite the live policy excludes, and re-arms when it flips back', () => {
|
||||
const fx = fixture()
|
||||
const phone = fx.deviceRegistry.addDevice('LAN phone')
|
||||
fx.deviceRegistry.setMobilePairingConnectionMode(phone.deviceId, 'automatic')
|
||||
fx.deviceRegistry.setRelayBinding(phone.deviceId, binding(phone.deviceId, 2_000))
|
||||
expect(fx.ledger.nextPendingExpiry()).toBe(2_000)
|
||||
|
||||
fx.host.mode = 'local-only'
|
||||
expect(fx.ledger.hasDemand(ownerIdentityKey)).toBe(false)
|
||||
expect(fx.ledger.nextPendingExpiry()).toBeNull()
|
||||
|
||||
fx.host.mode = 'automatic'
|
||||
expect(fx.ledger.nextPendingExpiry()).toBe(2_000)
|
||||
})
|
||||
|
||||
it('still reports the nearest in-scope pending invite', () => {
|
||||
const fx = fixture()
|
||||
const near = fx.deviceRegistry.addDevice('Near phone')
|
||||
const far = fx.deviceRegistry.addDevice('Far phone')
|
||||
fx.deviceRegistry.setRelayBinding(far.deviceId, binding(far.deviceId, 9_000))
|
||||
fx.deviceRegistry.setRelayBinding(near.deviceId, binding(near.deviceId, 4_000))
|
||||
expect(fx.ledger.nextPendingExpiry()).toBe(4_000)
|
||||
})
|
||||
})
|
||||
|
||||
describe('RelayDemandLedger owner scoping of transient refs', () => {
|
||||
// Characterisation: transient refs carry no owner identity, so they answer
|
||||
// hasDemand for any signed-in identity. See the report for the risk window.
|
||||
it('counts a transient ref for an identity that did not request it', () => {
|
||||
const fx = fixture()
|
||||
const phone = fx.deviceRegistry.addDevice('Phone')
|
||||
fx.deviceRegistry.setMobilePairingConnectionMode(phone.deviceId, 'automatic')
|
||||
fx.deviceRegistry.setRelayBinding(phone.deviceId, binding(phone.deviceId))
|
||||
const release = fx.ledger.acquireTransient(`pairing:${phone.deviceId}`, phone.deviceId)
|
||||
expect(fx.ledger.hasDemand(ownerIdentityKey)).toBe(true)
|
||||
expect(fx.ledger.hasDemand(otherOwnerIdentityKey)).toBe(true)
|
||||
release()
|
||||
expect(fx.ledger.hasDemand(otherOwnerIdentityKey)).toBe(false)
|
||||
})
|
||||
})
|
||||
@@ -1,5 +1,5 @@
|
||||
import type { DeviceRegistry } from '../device-registry'
|
||||
import type { RelayRevokeOutbox } from './relay-revoke-outbox'
|
||||
import type { DeviceEntry, DeviceRegistry } from '../device-registry'
|
||||
import type { RelayDeviceBinding, RelayRevokeOutbox } from './relay-revoke-outbox'
|
||||
|
||||
type RelayDemandLedgerOptions = {
|
||||
deviceRegistry: DeviceRegistry
|
||||
@@ -35,6 +35,10 @@ export class RelayDemandLedger {
|
||||
}
|
||||
released = true
|
||||
const current = this.transientRefs.get(key)
|
||||
// Keep this guard even though nothing can reach it today: `released` makes the lookup happen
|
||||
// at most once per closure and no public path empties the map, so mutating it away survives
|
||||
// the suite. It goes load-bearing the moment the ledger grows a dispose()/clear(), and
|
||||
// nothing would catch that.
|
||||
if (!current) {
|
||||
return
|
||||
}
|
||||
@@ -47,6 +51,20 @@ export class RelayDemandLedger {
|
||||
}
|
||||
|
||||
hasDemand(ownerIdentityKey: string): boolean {
|
||||
// KNOWN GAP, deliberately not fixed here: a transient ref carries no owner identity, so this
|
||||
// loop answers true for ANY signed-in identity. The device-binding and revoke-outbox branches
|
||||
// below both filter on `ownerIdentityKey`; this one cannot. A profile/org switch mid-pairing
|
||||
// therefore keeps the coordinator holding a relay control session for an identity that never
|
||||
// asked for one.
|
||||
//
|
||||
// Nothing defends either the current behaviour or that regression: scoping this loop by owner
|
||||
// fails exactly one test across the whole relay suite, the characterisation test written for
|
||||
// it. The fix has to thread identity through `acquireTransient`, whose call site is
|
||||
// desktop-relay-service.ts. Inferring the owner here instead — skipping a ref whose device
|
||||
// carries a different owner's binding — is UNSAFE: `withTransientDemand('provision')` calls
|
||||
// `setMobileRelayBinding` inside the operation, so during a re-pair the device still holds the
|
||||
// old owner's binding and that filter would drop demand mid-provision, tearing the broker down
|
||||
// under the very operation holding the ref.
|
||||
for (const ref of this.transientRefs.values()) {
|
||||
if (this.isRelayAllowed(ref.deviceId)) {
|
||||
return true
|
||||
@@ -57,14 +75,8 @@ export class RelayDemandLedger {
|
||||
}
|
||||
const now = (this.options.now ?? Date.now)()
|
||||
return this.options.deviceRegistry.listDevices().some((device) => {
|
||||
const binding = device.relayBinding
|
||||
if (
|
||||
device.scope !== 'mobile' ||
|
||||
!binding ||
|
||||
binding.ownerIdentityKey !== ownerIdentityKey ||
|
||||
binding.relayHostId !== this.options.relayHostId ||
|
||||
!this.isRelayAllowed(device.deviceId)
|
||||
) {
|
||||
const binding = this.demandCandidateBinding(device)
|
||||
if (!binding || binding.ownerIdentityKey !== ownerIdentityKey) {
|
||||
return false
|
||||
}
|
||||
// Why: E2EE authentication marks a scanned DeviceEntry as seen before
|
||||
@@ -78,7 +90,9 @@ export class RelayDemandLedger {
|
||||
const now = (this.options.now ?? Date.now)()
|
||||
let next: number | null = null
|
||||
for (const device of this.options.deviceRegistry.listDevices()) {
|
||||
const expiresAt = device.relayBinding?.inviteExpiresAt
|
||||
// Why: an expiry that hasDemand would never look at still armed a wake
|
||||
// timer — another host's binding, a runtime device, a LAN-excluded phone.
|
||||
const expiresAt = this.demandCandidateBinding(device)?.inviteExpiresAt
|
||||
if (expiresAt && expiresAt > now && (next === null || expiresAt < next)) {
|
||||
next = expiresAt
|
||||
}
|
||||
@@ -86,6 +100,25 @@ export class RelayDemandLedger {
|
||||
return next
|
||||
}
|
||||
|
||||
/** Test-only view of the outstanding transient refs. */
|
||||
transientRefsForTests(): ReadonlyMap<string, Readonly<TransientRef>> {
|
||||
return this.transientRefs
|
||||
}
|
||||
|
||||
/** The binding when this device could stand as demand on this relay host, else null. */
|
||||
private demandCandidateBinding(device: DeviceEntry): RelayDeviceBinding | null {
|
||||
const binding = device.relayBinding
|
||||
if (
|
||||
device.scope !== 'mobile' ||
|
||||
!binding ||
|
||||
binding.relayHostId !== this.options.relayHostId ||
|
||||
!this.isRelayAllowed(device.deviceId)
|
||||
) {
|
||||
return null
|
||||
}
|
||||
return binding
|
||||
}
|
||||
|
||||
private isRelayAllowed(deviceId: string): boolean {
|
||||
return this.options.isRelayAllowedForDevice?.(deviceId) ?? true
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user