Skip to content

fix(relay): stop the relay reopening a broker after quit - #19963

Open
nwparker wants to merge 9 commits into
nwparker/relay-unreadable-session-taxonomyfrom
nwparker/relay-lifecycle-fences
Open

nwparker wants to merge 9 commits into
nwparker/relay-unreadable-session-taxonomyfrom
nwparker/relay-lifecycle-fences

Conversation

@nwparker

@nwparker nwparker commented Sep 11, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 5 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​848 0 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​848
Prod 4 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​125 $\color{#cf222e}{\Huge{\mathbf{−}}}$​41 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​84

What breaks

Orca can open a fresh Relay control session after the user has quit the app. Three separate things reach it, each measured against a fresh service:

door measured
a pending invite-expiry wake getTimerCount() is 1 immediately after quit; the timer fires and reopens the broker
a pairing mint settling after quit 0 timers at quit, 1 once the mint settles, and connect runs again
a power-resume ensureLive() connect called twice — a new control session opened post-quit

The same hole sits between the pre-sign-out fence and the profile wipe, which is the window that fence exists to close.

Why it survived

DesktopRelayService.stop() has zero production callers. Quit, relaunch and pre-sign-out all go through fenceAndCloseNow().

The two are not equivalent. stop() sets stopped and clears both timers. The fence cleared only livenessTimer, left demandExpiryTimer armed, and left stopped false — and its comment, "the next auth mutation re-arms via refreshDemand", is correct for sign-out and exactly wrong for quit.

So the path production takes was the one that permitted the resurrection, and the test suite was exercising the other one. stop() is genuinely hard; nobody was calling it. That asymmetry is the whole bug, and it is worth not re-litigating in review.

The fix: two intents, kept apart

Rather than unifying them:

  • fenceAndCloseNow() gains a fenced latch cleared only by start()/authMutated(). Sign-out and relaunch keep the re-arm the old comment protects; the dangerous window stays shut.
  • Quit calls stop(), which is terminal. Wire behaviour is unchanged — coordinator.stop() fences reasonlessly exactly as the old call did.

Collapsing them would destroy the distinction the defect came from.

Why a new file, and why it lands first

relay-revoke-outbox-flush.ts is an extraction, behaviour-preserving, no logic change — flushRevokeOutbox/flushRevoke moved out of DesktopRelayService verbatim.

It had to land first because desktop-relay-service.ts has no headroom under the 300-line cap: the fix alone puts it at 304 and the pre-commit hook rejects it. A max-lines disable is not an option in this repo.

On reuse — nothing existing drains the outbox. RelayRevokeOutbox is the durable store, and 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 only new code is an options type and a class shell.

Commits — each independently green

Typecheck, lint, and the full relay + startup suites were run at each commit, not just at the tip, so the stack is genuinely splittable.

b0d02c30022 refactor(relay): extract the revoke-outbox flush into its own module
9a8eef01372 fix(relay): make the quit fence hard, and route quit to the terminal stop()
74f9feb47c7 fix(relay): drain the revoke outbox on one broker, not one open per item
5f5aa6658cb fix(relay): stop a mid-read profile switch reporting as a sign-out
3a71739e3c6 test(relay): pin RelayControlClient teardown before, and twice after, connect
572055ae36a docs(relay): cross-reference the terminal stop() and the re-armable fence
33e3bd42778 test(relay): ratchet which teardown each lifecycle event calls
8cde428aaeb docs(relay): address the teardown ratchet's docstring to whoever it goes red on

The other two fixes in the stack

74f9feb47c7 — one director open per outbox item. flushAll runs unawaited from inside the coordinator's openBroker, so its broker is not registered yet; refreshing demand after every removal reconciled against a null ownership. Measured with a real outbox on disk: 3 queued revocations produced 3 connect calls and 2 abandoned brokers, against a relay that rate-limits assignment. After: 1 connect, 3 revokes, outbox empty.

5f5aa6658cb — a mid-read profile switch reported as a sign-out. Same taxonomy error 8972d744 fixed for the session file, in the other null exit. 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() resolves moments later. After: auth_unavailable, which is retryable.

Two decisions made, not asked

A vetoed quit leaves Relay down for the session. before-quit already nulls the mobile pairing provider unconditionally on the next line, so pairing is dead there regardless — a re-armed broker would be a zombie holding a control session nothing can reach. A user who cancels a quit can toggle Relay back; an unreachable broker is not recoverable by any user action.

fenced is not set by stop(). The two latches stay independent. Collapsing them would merge two guards that pin different properties, which is the shape that caused this.

Residual risk

The cross-reference comments (572055ae36a) protect a reader who arrives at either method. They do nothing about someone editing the quit handler back to the fence — which is the edit that reintroduces the defect.

33e3bd42778 closes that. It is a source-text ratchet, no harness, matching the existing runtime-file-target-connection-field-ratchet.test.ts: both methods take an optional argument and return void, so swapping them compiles clean and passes every unit test, and the difference only surfaces in timer state minutes later. Mutation-verified in three directions — reverting quit to the fence fails one test only, collapsing sign-out onto stop() fails the other only, and renaming an unrelated service's stop() fires neither. Regexes are receiver-qualified because the quit handler stops five unrelated services.

Known limit, chosen deliberately: the match is literal, so hoisting the call behind a local trips it as a false positive. A refactor that obscures which teardown runs at quit should get a second look.

Not fixed here: ensureActiveOrcaProfile treats an unreadable profile index the same as a corrupt one — it catches every error including EACCES/EMFILE, fabricates a default local profile, and writes it over the unreadable file. The relay then reports a sign-out honestly, because by that point the index really has been replaced. That belongs in profile-index-store.ts and changes app-wide sign-in semantics — the same caveat 8972d744 raised for decrypt-failed.

Verification

pnpm tc:node clean, oxlint clean on src/main/runtime/relay/ and src/main/startup/, pnpm test src/main/runtime src/main/startup → 8152 passed, 0 ELIFECYCLE.

Every assertion added in this stack was mutation-tested: the specific line it protects was mutated, only that assertion was confirmed to fail, and the mutation was reverted. One survivor is documented in 74f9feb47c7's message as genuinely inert.

Base — retargeted

This PR was based on nwparker/adv2-concurrency-fixes, an integration branch with no PR of its own. It could never have merged through that base, and a reviewer approving it there would have been approving a diff against a tree that is not going anywhere.

The real dependency set is two commits, not sixty. Rebasing this stack onto them reproduces the diff of this PR byte for byte — same nine files, same +943/−42, identical blob hashes — which is the proof that adv2-concurrency-fixes contributed nothing else:

#19870 fix/18211-lan-mode-applies-to-paired-devices supplies hostMobilePairingConnectionMode and isRelayAllowedForDevice, which desktop-relay-service.ts reads here. Without it the extraction commit conflicts on main.
#20055 nwparker/relay-unreadable-session-taxonomy the sibling taxonomy fix 8972d744, cited by name in the commit below and by the code comment it adds.

Base is now #20055, which is itself stacked on #19870. Merge order: #19870 → #20055 → this.

Commit SHAs moved

The rebase rewrote all eight SHAs. The old commits stay reachable via nwparker/adv2-concurrency-fixes, nwparker/adv3-map-composed and the nwparker/j4-* stacks, so any citation elsewhere still resolves with git show.

was now
cd175054c5f b0d02c30022
4733b21cdcb 9a8eef01372
f1eb8037c85 74f9feb47c7
ed35db892bc 5f5aa6658cb
83369d0ffce 3a71739e3c6
ae4c1304229 572055ae36a
762e1faa80d 33e3bd42778
d4f77000628 8cde428aaeb

The splittable property survived the rebase

Re-verified per commit on the new base, all nine including the base commit: tsc --noEmit -p config/tsconfig.node.json clean at each, and src/main/runtime/relay + src/main/startup green at each, with the test-file count rising monotonically 76 → 81 as the stack builds. No commit is green only because a later one repairs it.

(The one red in that range, src/main/startup/ensure-virtual-display.test.ts › reports failure when a stale socket blocks the Xvfb rebind, reproduces on the untouched base and is excluded as pre-existing.)

Not folded in, deliberately

6bd997b2cca fix(relay): clear the demand-expiry timeout on a fence, not only in stop() — superseded by this PR. Re-applying it is a regression, not a duplicate.

It attacks the same defect from the same starting observation (fenceAndCloseNow cleared livenessTimer and left demandExpiryTimer armed; stop() cleared both but has no production caller). It stops at clearing both timers, via a shared disarmTimers().

This PR already contains that dedup — clearTimers(), same shape, shared by the fence and stop(), relieving the same max-lines pressure — and strictly more: the fenced latch, cleared only by start()/authMutated() and wired into the coordinator as isHalted: () => this.stopped || this.fenced, plus routing quit to the terminal stop().

The latch is not decoration. This PR's own comment is a direct rebuttal of the timer-clearing approach: "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." Clearing both timers closes one of the three doors measured at the top of this PR; the settling-mint and power-resume doors reach refreshDemand without any timer.

So the hazard is a merge resolution, not a merge conflict. 6bd997b2cca is older and looks simpler, and it touches the same lines — anyone resolving toward it deletes the latch and reopens two of three doors. Patch-id does not catch this: the two differ, so it reads as a missing commit rather than a superseded one.

Its two genuinely unique observations are both already carried: the note on why the existing lifecycle test missed this (it stubs demandLedger: { nextPendingExpiry: () => null }, so the second timer is never armed) and the waitForLiveBrokerResult deadline-not-re-checked caveat, which is fixed by 0549bd19a65 in #20061.

ec0a2c19db9 "fix(relay): drop the outbox method the lifecycle merge orphaned" looks like a fix for this PR and is not one. flushRevokeOutbox does not exist on this branch — the extraction commit removed it. The orphan appears only in the union of this branch and adv3-map-composed, where the merge kept a method reading this.revokeOutbox/this.flushRevoke that this branch moved into the coordinator. It is an integration-only typecheck break and belongs to whoever composes those two, not here. Applying it to main would delete a method that still has a live caller at desktop-relay-service.ts:95.

Rebase record (2026-09-11)

Stack preserved: base stays nwparker/relay-unreadable-session-taxonomy (#20055), which itself stacks on #19870. Both were rebased onto main at 5fa62feda7c264c6eda99ba4788ffebab9aae26c first; this branch was replayed with git rebase --onto 0dc273fd9090210d41ea50af66c7edfdca32a57c aff7b40fff5112e9806a0a7f9c297f01cef77936.

SHA
old head 8cde428aaebfc692f6639eec337efd76a680cd7c
new head c0907f78c8b41238a843a027613c9413e3aac590
new base (#20055) 0dc273fd9090210d41ea50af66c7edfdca32a57c

No conflicts across all eight commits.

Gates on the rebased head: typecheck exit 0 (proven failable); vitest run --config config/vitest.config.ts src/main/runtime/relay src/main/startup/desktop-relay-startup.test.ts — 30 files / 226 tests passed. desktop-relay-service.ts is 347 physical lines and passes the 300-line max-lines cap (blank/comment-skipping); the revoke-outbox extraction on this branch is what provides that headroom and was kept, not re-inlined.

Fix: the terminal stop() belongs on the committed quit handler

77d8b28aa2b. Moving quit off the re-armable fence was right; putting stop() in before-quit was not.

before-quit 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: no liveness tick, no reconcile, no door back. The comment three lines below in the same handler warns about precisely this, and desktopPushService in the same file is already split across the two handlers for the same reason.

The original justification in this PR — "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.

Now:

  • before-quit fences. It closes the broker just as promptly and latches fenced, which refreshDemand also refuses — so the settling mint, the pending invite-expiry wake and the power-resume ensureLive still cannot reopen a broker in the window before will-quit. That was the whole point of this PR and it is intact.
  • will-quit — the committed path — calls stop() and makes it terminal.
  • On a veto, the next auth mutation clears fenced and Relay comes back.

The call-site ratchet guarded the module, which could not express this: the quit module legitimately holds both a latch and a fence. It now resolves each handler body and guards them separately — the vetoable handler must fence and must not latch, the committed handler the reverse. Mutation-verified: restoring the before-quit stop() fails both new cases.

Merge-order constraints

  • This PR is three deep: main ← #19870 ← #20055 ← #19963. It cannot be built or evaluated against main alone — it references hostMobilePairingConnectionMode, isRelayAllowedForDevice, relayOfflineReasonMintFailureCode and refreshDemand({ skipLinger }), none of which exist on main.
  • fix(relay): stop a revoke that can never land from pinning the relay online #20062 collides with this PR's refactor. Its test builds the service with Object.create(DesktopRelayService.prototype) and calls service.flushRevokeOutbox(...). This PR moves that method into RelayRevokeOutboxFlusher and takes revokeOutbox off the service, so whichever of the two lands second must update that test. No production-line conflict.

The #20062 collision, with a tested patch ready

#20062 is independent and lands first; this PR is three deep, so this PR carries the fix.

The collision is real and confirmed by execution, not predicted. Landing #20062's test file onto this tree gives:

FAIL  relay-revoke-permanent-failure.test.ts > stops pinning relay demand once its window passes, but is never abandoned
TypeError: service.flushRevokeOutbox is not a function
FAIL  relay-revoke-permanent-failure.test.ts > still lets a revoke that lands remove the item and release demand
TypeError: service.flushRevokeOutbox is not a function

That test builds a service with Object.create(DesktopRelayService.prototype) and calls the private flushRevokeOutbox. This PR moves that loop into RelayRevokeOutboxFlusher and takes revokeOutbox off the service, so the method is gone.

The fix makes the test simpler, because RelayRevokeOutboxFlusher is exported and directly constructible — the prototype fixture and all three of its type assertions disappear. Apply to src/main/runtime/relay/relay-revoke-permanent-failure.test.ts after rebasing past #20062:

-import { DesktopRelayService } from './desktop-relay-service'
+import { RelayRevokeOutboxFlusher } from './relay-revoke-outbox-flush'

-function permanentlyFailingBroker(revokeDevice: ReturnType<typeof vi.fn>) {
-  // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: ...
-  return { ownerIdentityKey, hostId: relayHostId, revokeDevice } as never
+function permanentlyFailingBroker(revokeDevice: (id: string, reqId: string) => Promise<void>) {
+  return { ownerIdentityKey, hostId: relayHostId, revokeDevice }
 }

-function serviceOver(revokeOutbox: RelayRevokeOutbox, ledger: RelayDemandLedger) {
-  // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: ...
-  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
-  })
-  // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: ...
-  return service as unknown as { flushRevokeOutbox: (broker: unknown) => Promise<void> }
+/** #19963 moved the drain loop off DesktopRelayService, so the flusher is driven directly
+ *  instead of faking a service through its prototype. */
+function flusherOver(revokeOutbox: RelayRevokeOutbox): RelayRevokeOutboxFlusher {
+  return new RelayRevokeOutboxFlusher({
+    outbox: revokeOutbox,
+    isHalted: () => false,
+    onDrained: () => {}
+  })
 }

and at both call sites:

-    const service = serviceOver(revokeOutbox, ledger)
+    const flusher = flusherOver(revokeOutbox)
-      await service.flushRevokeOutbox(permanentlyFailingBroker(revokeDevice))
+      await flusher.flushAll(permanentlyFailingBroker(revokeDevice))

Verified on this tree, not just written: both tests pass with the patch applied, and the guard keeps its teeth — reverting #20062's demand-window change still fails stops pinning relay demand once its window passes with expected true to be false, the same signature it produces on #20062's own branch. The second test survives that mutation there too; it is a deliberate happy-path control, not coverage.

Verification against the current base

Re-run after nwparker/relay-unreadable-session-taxonomy advanced to 287e850e537:

  • merges clean onto the new base
  • pnpm tc exit 0, zero error TS
  • src/main/runtime/relay + src/main/startup + src/main/orca-profiles: 114 files, 921 tests passed, 8 skipped, 0 failed

This PR has still never been built against main alone — it cannot be, at three deep. That remains an explicit gap, not a verdict.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

This is a careful, well-tested fix, but routing quit through the terminal stop() introduces a regression for vetoed quits that the PR's rationale underestimates. See the section below before merging.

Reviewed changes

  • fenced latch closes the re-arm windows. fenceAndCloseNow() now sets a fenced latch (cleared only by start()/authMutated()) and clears both timers via clearTimers(); refreshDemand() and ensureLive() bail while fenced, so a settling mint, a pending invite-expiry wake, and a power-resume ensureLive() can no longer resurrect a broker after the fence.
  • Quit routed to the terminal stop(). main-process-quit.ts now calls desktopRelayService?.stop() instead of the re-armable fence, matching the existing (previously uncalled) stop() which sets stopped and stops the coordinator.
  • Revoke-outbox flush extracted and fixed. flushRevoke/flushRevokeOutbox move into RelayRevokeOutboxFlusher, which re-checks a halt latch between items and calls onDrained once after the loop instead of per item, fixing the one-director-open-per-item behavior.
  • Profile-switch race reclassified. readRelayAuthContext now throws orca_profile_switched_during_read on a mid-read switch instead of returning null, which had spent the terminal SIGNED_OUT on a retryable race.
  • Teardown ratchet + pinning tests. A source-text ratchet asserts quit calls stop() (never the fence) and sign-out/relaunch call the fence (never stop()); new tests pin RelayControlClient teardown and the lifecycle/outbox behavior.

⚠️ A vetoed quit now leaves Relay terminally down, and the recovery you cite doesn't exist

The before-quit handler runs on every quit attempt, including ones the renderer later vetoes — this file itself references both the unsaved-work beforeunload veto and the macOS buffer-capture deferral that cancels a quit. Routing quit through stop() sets stopped = true on both DesktopRelayService and RelayAuthCoordinator, and nothing in the codebase ever resets either flag or reconstructs the service: startDesktopRelayService is called exactly once at launch. So after any vetoed quit, Relay stays down until a full app restart.

The PR body's mitigation — "A user who cancels a quit can toggle Relay back" — doesn't hold. Toggling the pairing mode reaches pairingPolicyChanged() → refreshDemand(), which returns early on this.stopped (and the coordinator's stopped is already latched). There is no stopped reset path anywhere. The previous fence was re-armable precisely so a vetoed quit recovered on the next authMutated()/resume; stop() forecloses that.

Technical details
# Vetoed quit terminates Relay with no in-session recovery

## Affected sites
- src/main/startup/main-process-quit.ts:78 — `state.desktopRelayService?.stop()` runs in `before-quit`, which renderers can veto.
- src/main/runtime/relay/desktop-relay-service.ts:51 — `private stopped` set true in `stop()`, never reset.
- src/main/runtime/relay/relay-auth-coordinator.ts:59 — `private stopped` set true in `stop()`, never reset.
- src/main/startup/desktop-relay-startup.ts:10-47 — `startDesktopRelayService` constructs the service once; no recreation path.

## Required outcome
- Decide whether terminal-on-veto is intended. If not, ensure a vetoed quit leaves Relay recoverable (or provide an in-session recovery path), and correct the PR body's "toggle Relay back" claim to match the code.

## Suggested approach (optional)
- Defer the terminal `stop()` to the `will-quit` (committed) handler, keeping `fenceAndCloseNow()` at `before-quit`; a vetoed quit then re-arms as before, while a real quit still terminates Relay.

## Open questions for the human (optional)
- Is the phone's loss of its control session after a vetoed quit acceptable? The `setMobileRelayPairingProvider(null)` on the next line kills the desktop's own pairing path, but not the broker/control session the phone connects to.

Pullfrog  | Fix it ➔ | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

@nwparker
nwparker force-pushed the nwparker/relay-lifecycle-fences branch from d4f7700 to 8cde428 Compare September 11, 2026 06:59
@nwparker
nwparker changed the base branch from nwparker/adv2-concurrency-fixes to nwparker/relay-unreadable-session-taxonomy September 11, 2026 06:59
@nwparker
nwparker force-pushed the nwparker/relay-unreadable-session-taxonomy branch from aff7b40 to 0dc273f Compare September 11, 2026 08:10
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.
…stop()

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.
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.
Same taxonomy error 8972d74 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 8972d74 raised for decrypt-failed.
… connect

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.
…ence

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.
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.
…oes red on

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.
@nwparker
nwparker force-pushed the nwparker/relay-lifecycle-fences branch from 8cde428 to c0907f7 Compare September 11, 2026 08:15
… not the vetoable one

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.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No critical issues — minor suggestions inline.

Reviewed changes

Since the prior pullfrog review, 77d8b28aa2 moved the terminal relay teardown off the vetoable quit handler:

  • Quit teardown split across the two quit handlers. before-quit now calls fenceAndCloseNow() instead of the terminal stop(); will-quit, the committed path, calls stop(). This resolves the prior review's concern that a renderer-vetoed quit latched stopped with nothing to clear it.
  • Ratchet resolves each handler body separately. The source-text ratchet now extracts installBeforeQuitHandler and installWillQuitHandler independently: the vetoable handler must fence and must not latch, the committed handler the reverse, and sign-out/relaunch keep the fence.

ℹ️ The PR body still describes the pre-77d8b28aa2 decision

The "Two decisions made, not asked" section still says a vetoed quit leaves Relay down for the session and that a user can toggle Relay back. 77d8b28aa2 replaced that design — before-quit now fences, so a vetoed quit re-arms on the next auth mutation — and toggling Relay still does not recover it: pairingPolicyChanged() → refreshDemand() returns early on fenced, as does power-resume ensureLive(). Only authMutated() and start() clear the latch.

Technical details
# PR body contradicts the new quit-teardown split

## Affected sites
- PR body, "Two decisions made, not asked" — "A vetoed quit leaves Relay down for the session."
- PR body, same paragraph — "A user who cancels a quit can toggle Relay back."
- src/main/runtime/relay/desktop-relay-service.ts:326 — `refreshDemand` returns early on `stopped || fenced`.
- src/main/runtime/relay/desktop-relay-service.ts:110 — `ensureLive` returns early on `fenced`.
- src/main/runtime/relay/desktop-relay-service.ts:115 — only `authMutated()` (and `start()`) clears `fenced`.

## Required outcome
- Align the body with `77d8b28aa2`: a vetoed quit is recoverable via `authMutated()`, not by toggling Relay or a power resume.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

Comment on lines +232 to +234
// 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The quit call-site cross-references are now stale. This comment says quit calls stop() from before-quit, and the sibling comment on fenceAndCloseNow (desktop-relay-service.ts:120) still says quit deliberately does not use the fence. 77d8b28aa2 did the opposite of both: before-quit is back on the fence and the terminal stop() moved to will-quit. These are the comments the PR leans on to keep the split legible, so they should describe the new arrangement.

Suggested change
// 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.
// Terminal: no door re-arms this. Quit's committed handler calls it
// (main-process-quit.ts will-quit); before-quit only fences. The re-armable
// counterpart is fenceAndCloseNow, which a settling mint or invite expiry re-arms.

Comment on lines +5 to +6
// 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This parenthetical is now false: 77d8b28aa2 reaches stop() in production, on will-quit. The rest of the header still holds, but that clause was the original observation that motivated the fix and now reads as if nothing ever calls stop().

Suggested change
// 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
// fenceAndCloseNow is what the vetoable quit handler and pre-sign-out call; the
// committed will-quit handler runs the terminal stop(). Its own comment says the
// fence must be hard, yet every

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant