From aa98edf35ab918f53e56424dfd7f39a00eb5138e Mon Sep 17 00:00:00 2001 From: Neil Date: Thu, 10 Sep 2026 17:40:33 -0700 Subject: [PATCH 1/9] fix(runtime): contain a parked resume replay that throws MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The handle-gap park releases its waiters from inside `useAppStore.subscribe`, and the replay it runs is `resumeSleepingAgentSessionsForWorktree` — a large synchronous sweep. Zustand notifies listeners in a plain loop, so a replay that threw escaped the `setState` that triggered it: measured, the exception left the store write, every listener registered after this module missed that write, and the sibling pane the same mirror frame made due was never released. The waiter's timer, map entry and subscription are already torn down before `run`, so containing the throw holds nothing back — it only stops one pane's failed recovery from taking the store notification and its siblings with it. --- ...rror-handle-gap-replay-containment.test.ts | 82 +++++++++++++++++++ .../src/lib/host-mirror-handle-gap-wait.ts | 10 ++- 2 files changed, 91 insertions(+), 1 deletion(-) create mode 100644 src/renderer/src/lib/host-mirror-handle-gap-replay-containment.test.ts diff --git a/src/renderer/src/lib/host-mirror-handle-gap-replay-containment.test.ts b/src/renderer/src/lib/host-mirror-handle-gap-replay-containment.test.ts new file mode 100644 index 00000000000..b11d80ba188 --- /dev/null +++ b/src/renderer/src/lib/host-mirror-handle-gap-replay-containment.test.ts @@ -0,0 +1,82 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { useAppStore } from '@/store' +import { clearRuntimeEnvironmentConnectionGenerationsForTests } from '@/store/slices/runtime-status' +import { + HOST_MIRROR_HANDLE_GAP_DEADLINE_MS, + countParkedHostMirrorHandleGapPanesForTests, + parkUntilHostMirrorHandleLands, + resetHostMirrorHandleGapWaitsForTests +} from './host-mirror-handle-gap-wait' + +// What this pins: the parked replay is `resumeSleepingAgentSessionsForWorktree`, a large +// synchronous sweep, and it is released from inside `useAppStore.subscribe`. Zustand notifies +// listeners in a plain loop, so a replay that throws escapes the `setState` that triggered it: +// the listeners registered after this module never see the write, and every sibling pane the +// same mirror frame made due is left parked. The waiter's own state is torn down before `run`, +// so containing the throw holds nothing back. + +const initialAppStoreState = useAppStore.getState() +const ENV_ID = 'env-gap-containment' + +function seedTwoMirroredPanes(): void { + useAppStore.setState({ + tabsByWorktree: { + wt: [ + { id: 'tab-a', title: 'a', ptyId: null }, + { id: 'tab-b', title: 'b', ptyId: null } + ] as never + }, + ptyIdsByTabId: {} + }) +} + +describe('host-mirror handle-gap replay containment', () => { + beforeEach(() => { + vi.useFakeTimers() + useAppStore.setState(initialAppStoreState, true) + }) + + afterEach(() => { + resetHostMirrorHandleGapWaitsForTests() + clearRuntimeEnvironmentConnectionGenerationsForTests() + useAppStore.setState(initialAppStoreState, true) + vi.useRealTimers() + vi.restoreAllMocks() + }) + + it('a replay that throws neither aborts the store write nor strands its sibling panes', () => { + vi.spyOn(console, 'error').mockImplementation(() => {}) + seedTwoMirroredPanes() + const siblingReplay = vi.fn() + parkUntilHostMirrorHandleLands(ENV_ID, 'wt', 'tab-a', () => { + throw new Error('replay blew up') + }) + parkUntilHostMirrorHandleLands(ENV_ID, 'wt', 'tab-b', siblingReplay) + expect(countParkedHostMirrorHandleGapPanesForTests()).toBe(2) + + // A store subscriber registered after this module's, so it is notified after the drain. + const laterSubscriber = vi.fn() + const unsubscribe = useAppStore.subscribe(laterSubscriber) + + // One mirror frame lands both handles, making both waiters due on a single store write. + expect(() => + useAppStore.setState({ ptyIdsByTabId: { 'tab-a': ['pty-a'], 'tab-b': ['pty-b'] } }) + ).not.toThrow() + unsubscribe() + + expect(siblingReplay).toHaveBeenCalledTimes(1) + expect(laterSubscriber).toHaveBeenCalledTimes(1) + expect(countParkedHostMirrorHandleGapPanesForTests()).toBe(0) + }) + + it('a replay that throws on the deadline path does not escape the timer', () => { + vi.spyOn(console, 'error').mockImplementation(() => {}) + seedTwoMirroredPanes() + parkUntilHostMirrorHandleLands(ENV_ID, 'wt', 'tab-a', () => { + throw new Error('replay blew up') + }) + + expect(() => vi.advanceTimersByTime(HOST_MIRROR_HANDLE_GAP_DEADLINE_MS)).not.toThrow() + expect(countParkedHostMirrorHandleGapPanesForTests()).toBe(0) + }) +}) diff --git a/src/renderer/src/lib/host-mirror-handle-gap-wait.ts b/src/renderer/src/lib/host-mirror-handle-gap-wait.ts index 9582dcc9d6b..37cc8b2ba02 100644 --- a/src/renderer/src/lib/host-mirror-handle-gap-wait.ts +++ b/src/renderer/src/lib/host-mirror-handle-gap-wait.ts @@ -82,7 +82,15 @@ function releaseWaiter(key: string): void { clearTimeout(waiter.deadline) waitersByPane.delete(key) stopStoreSubscriptionIfIdle() - waiter.run() + // Why contained: the release path runs inside useAppStore.subscribe, so a replay that throws + // escapes the setState that triggered it — aborting the listener loop, so every subscriber + // after this one misses the write, and stranding the sibling panes the same frame made due. + // The waiter's own state is already torn down above, so nothing is held by swallowing here. + try { + waiter.run() + } catch (error) { + console.error(`[host-mirror] parked resume replay failed for ${key}:`, error) + } } function waiterIsReleased(waiter: HandleGapWaiter, state: HandleGapStoreState): boolean { From 81dd2fe59005db5a21c7227971abca98532eff3f Mon Sep 17 00:00:00 2001 From: Neil Date: Thu, 10 Sep 2026 17:46:35 -0700 Subject: [PATCH 2/9] fix(worktrees): an unknown host home refuses the recursive delete MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `resolveWorktreeRemovalHome` answers `{ kind: 'executionHost', homePath: null }` whenever the relay session has left `activeSessions` or never resolved its host env — an ordinary disconnect, and the default resolver before the registration side effect runs at all. `resolveGuardHomePath` then correctly declines to substitute the client's home, but `isHomeDirectoryRemovalPath` fell through to path SHAPES only, and shapes do not know `/srv/homes/alice`, `/export/home/alice` or `D:\Profiles\bob`. Injected: the host-home fixture with the resolver answering null. Measured: `removeRuntimeUnregisteredWorktree` called `deletePath('/srv/homes/alice', true)` — a recursive delete of the host's own home, the exact path the two shipped tests pin as refused when the resolver does answer. "Could not ask the host where its home is" is unverifiable, so the two recursive-delete gates now fail closed on it. `isDangerousWorktreeRemovalPath` deliberately does not consult it: it also fences the registered `git worktree remove` path, which must stay usable mid-reconnect. The stated cost is pinned too: an ordinary orphan is also declined while the home is unknown. Declining is recoverable — the row survives and the next connected removal proceeds — and a recursive delete of the wrong directory is not. --- ...worktrees-orphan-directory-cleanup.test.ts | 10 ++++++- ...istered-worktree-removal-host-home.test.ts | 28 +++++++++++++++++++ src/main/worktree-removal-safety.ts | 22 +++++++++++++++ 3 files changed, 59 insertions(+), 1 deletion(-) diff --git a/src/main/ipc/worktrees-orphan-directory-cleanup.test.ts b/src/main/ipc/worktrees-orphan-directory-cleanup.test.ts index 954a994940d..8b8a89ca450 100644 --- a/src/main/ipc/worktrees-orphan-directory-cleanup.test.ts +++ b/src/main/ipc/worktrees-orphan-directory-cleanup.test.ts @@ -1,4 +1,4 @@ -import { beforeEach, describe, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { lstat, mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' @@ -15,6 +15,7 @@ import { import { handlers, mainWindow, setupWorktreeHandlers, store } from './worktrees-test-harness' import { makeWorktreeMeta, mockKnownFeatureWorktree } from './worktrees-test-fixtures' import type { WorktreeRuntimeStub } from './worktrees-test-runtime-stub' +import { setWorktreeRemovalSshHostHomeResolver } from '../worktree-removal-execution-host-route' vi.mock('electron', async () => (await import('./worktrees-test-module-mocks')).electronModuleMock() @@ -105,6 +106,10 @@ describe('registerWorktreeHandlers', () => { runtimeStub = setupWorktreeHandlers() }) + afterEach(() => { + setWorktreeRemovalSshHostHomeResolver(() => null) + }) + it('reports already-missing unregistered delete paths before teardown, hooks, or git removal', async () => { mockKnownFeatureWorktree('/workspace/real-feature') getEffectiveHooksMock.mockReturnValue({ @@ -421,7 +426,10 @@ describe('registerWorktreeHandlers', () => { } }) + // The recursive-delete gate now requires the execution host to have reported its `$HOME`, so + // this test has to establish it before it can reach the symlink check it is actually about. it('refuses SSH orphan cleanup when remote .git is a symlink', async () => { + setWorktreeRemovalSshHostHomeResolver(() => '/remote/home/alice') const repo = { id: 'repo-ssh-symlink-git', path: '/remote/repo', diff --git a/src/main/runtime/runtime-unregistered-worktree-removal-host-home.test.ts b/src/main/runtime/runtime-unregistered-worktree-removal-host-home.test.ts index 259209b9b04..3248680f023 100644 --- a/src/main/runtime/runtime-unregistered-worktree-removal-host-home.test.ts +++ b/src/main/runtime/runtime-unregistered-worktree-removal-host-home.test.ts @@ -77,6 +77,34 @@ describe('removeRuntimeUnregisteredWorktree against an SSH host home', () => { expect(fsProvider.deletePath).not.toHaveBeenCalled() }) + // The resolver answers null whenever the relay session left `activeSessions` or never resolved + // its host env — an ordinary disconnect. That skips the containment check entirely and leaves + // only path SHAPES, which do not know `/srv/homes/alice`. "Could not ask the host where its home + // is" is unverifiable, so the recursive delete has to fail closed. + it('refuses the recursive delete when the host never reported its home', async () => { + setWorktreeRemovalSshHostHomeResolver(() => null) + const fsProvider = provenOrphanFilesystem(HOST_HOME) + + await expect( + removeRuntimeUnregisteredWorktree(removalArgs(HOST_HOME, fsProvider)) + ).rejects.toThrow(`Refusing to delete unregistered worktree path: ${HOST_HOME}`) + expect(fsProvider.deletePath).not.toHaveBeenCalled() + }) + + // The stated cost of failing closed: an ordinary orphan is also declined until the host answers. + // Declining is recoverable — the row survives and the next connected removal proceeds — while a + // recursive delete of the wrong directory is not. + it('declines an ordinary orphan too while the home is unknown', async () => { + setWorktreeRemovalSshHostHomeResolver(() => null) + const worktreePath = `${HOST_HOME}/workspaces/leftover` + const fsProvider = provenOrphanFilesystem(worktreePath) + + await expect( + removeRuntimeUnregisteredWorktree(removalArgs(worktreePath, fsProvider)) + ).rejects.toThrow(`Refusing to delete unregistered worktree path: ${worktreePath}`) + expect(fsProvider.deletePath).not.toHaveBeenCalled() + }) + it('still deletes a proven orphan under that host home', async () => { setWorktreeRemovalSshHostHomeResolver(() => HOST_HOME) const worktreePath = `${HOST_HOME}/workspaces/leftover` diff --git a/src/main/worktree-removal-safety.ts b/src/main/worktree-removal-safety.ts index 129a44cfbef..8254c731661 100644 --- a/src/main/worktree-removal-safety.ts +++ b/src/main/worktree-removal-safety.ts @@ -128,6 +128,22 @@ export function assertWorktreeDoesNotContainRegisteredWorktree( } } +/** + * Whether the home guard was able to ask the machine that executes the removal. + * + * An execution host with no reported `$HOME` is `unknown`, not safe: the containment check is + * skipped entirely and only path SHAPES remain, and shapes do not know `/srv/homes/alice`, + * `/export/home/alice` or `D:\Profiles\bob`. The resolver answers `null` whenever the relay + * session is gone from `activeSessions` or never resolved its host env, which is an ordinary + * disconnect — and loss of contact is not permission to recursively delete + * (docs/reference/ssh-execution-boundary.md). Only the recursive-delete gates consult this; + * `isDangerousWorktreeRemovalPath` deliberately does not, because it also fences the registered + * `git worktree remove` path, which must stay usable while a session is mid-reconnect. + */ +function homeAuthorityAnswered(home: WorktreeRemovalHomeAuthority): boolean { + return home.kind !== 'executionHost' || Boolean(home.homePath) +} + export async function canSafelyRemoveOrphanedWorktreeDirectory( worktreePath: string, repoPath: string, @@ -135,6 +151,9 @@ export async function canSafelyRemoveOrphanedWorktreeDirectory( statPath: StatPath = lstat, readPath: ReadPath = (path) => readFile(path, 'utf8') ): Promise { + if (!homeAuthorityAnswered(home)) { + return false + } if (isDangerousWorktreeRemovalPath(worktreePath, repoPath, home)) { return false } @@ -185,6 +204,9 @@ export async function canCleanupUnregisteredOrcaLeftoverDirectory(args: { if (!hasCurrentOrcaCreationProvenance(args.meta) && !hasLegacyOrcaCreationEvidence(args.meta)) { return false } + if (!homeAuthorityAnswered(args.home)) { + return false + } if ( isDangerousWorktreeRemovalPath(args.worktreePath, args.repo.path, args.home) || From 58fac71ba0284cc0929c1624171a82c875aa5463 Mon Sep 17 00:00:00 2001 From: Neil Date: Thu, 10 Sep 2026 17:51:22 -0700 Subject: [PATCH 3/9] fix(runtime): a landed handle retires the handle-gap timeout verdict MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `expiredGenerationByPane` was cleared only by a later expiry on the same environment with a different connection generation. The module's escape hatch was "a reconnect bumps the connection generation and arms a fresh wait" — and the #19647 change in this same stack stops recording `status: null` for an unreachable host, so `connectionChanged` no longer fires across an outage on the same runtime. Measured: park, let the deadline fire, land the handle, republish rows ahead of handles on the same generation — `findUnhydratedHostMirrorForPane` returned null and the sweep resumed immediately with zero panes parked. That is #19735 with the bounded wait removed entirely rather than merely shortened. A published handle is positive host evidence and ends the gap episode the deadline was about, so it now retires the verdict. The store subscription outlives the waiter for exactly as long as an expiry needs watching. Also carries the worktree across a re-park: adopting an orphaned terminal re-keys `tabsByWorktree` without re-keying the record, so a live wait kept releasing on retraction evidence about the workspace it was no longer about. And contains the same throwing-replay shape in the sibling drain (`host-session-mirror-hydration`), which runs from the frame-apply path. --- .../lib/host-mirror-handle-gap-expiry.test.ts | 87 +++++++++++++++++++ .../src/lib/host-mirror-handle-gap-wait.ts | 35 +++++++- .../runtime/host-session-mirror-hydration.ts | 9 +- 3 files changed, 129 insertions(+), 2 deletions(-) create mode 100644 src/renderer/src/lib/host-mirror-handle-gap-expiry.test.ts diff --git a/src/renderer/src/lib/host-mirror-handle-gap-expiry.test.ts b/src/renderer/src/lib/host-mirror-handle-gap-expiry.test.ts new file mode 100644 index 00000000000..0c9a8f87727 --- /dev/null +++ b/src/renderer/src/lib/host-mirror-handle-gap-expiry.test.ts @@ -0,0 +1,87 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { useAppStore } from '@/store' +import { clearRuntimeEnvironmentConnectionGenerationsForTests } from '@/store/slices/runtime-status' +import { + HOST_MIRROR_HANDLE_GAP_DEADLINE_MS, + countParkedHostMirrorHandleGapPanesForTests, + hasHostMirrorHandleWaitExpired, + parkUntilHostMirrorHandleLands, + resetHostMirrorHandleGapWaitsForTests +} from './host-mirror-handle-gap-wait' + +// What this pins: the expiry verdict is per handle-gap EPISODE, not per connection generation. +// The module's stated escape was "a reconnect bumps the connection generation and arms a fresh +// wait", but #19647 (same stack) stops recording `status: null` for an unreachable host, so +// `connectionChanged` no longer fires across an outage on the same runtime. A verdict that +// outlives the gap it was about removes the bounded wait entirely for every later gap on that +// pane — the #19735 fork with no wait at all, which is worse than the shortened wait the +// deadline was designed to give. + +const initialAppStoreState = useAppStore.getState() +const ENV_ID = 'env-gap-expiry' +const TAB_ID = 'web-terminal-expiry-tab' + +function publishRowWithoutHandle(worktreeId = 'wt'): void { + useAppStore.setState({ + tabsByWorktree: { [worktreeId]: [{ id: TAB_ID, title: 't', ptyId: null }] as never }, + ptyIdsByTabId: {} + }) +} + +describe('host-mirror handle-gap expiry', () => { + beforeEach(() => { + vi.useFakeTimers() + useAppStore.setState(initialAppStoreState, true) + }) + + afterEach(() => { + resetHostMirrorHandleGapWaitsForTests() + clearRuntimeEnvironmentConnectionGenerationsForTests() + useAppStore.setState(initialAppStoreState, true) + vi.useRealTimers() + }) + + it('a landed handle retires the expiry so the next gap gets its own wait', () => { + publishRowWithoutHandle() + parkUntilHostMirrorHandleLands(ENV_ID, 'wt', TAB_ID, vi.fn()) + + vi.advanceTimersByTime(HOST_MIRROR_HANDLE_GAP_DEADLINE_MS) + expect(hasHostMirrorHandleWaitExpired(ENV_ID, TAB_ID)).toBe(true) + + // The handle the wait was about finally lands — positive host evidence, same connection. + useAppStore.setState({ ptyIdsByTabId: { [TAB_ID]: ['remote:env@@term_1'] } }) + expect(hasHostMirrorHandleWaitExpired(ENV_ID, TAB_ID)).toBe(false) + + // A later frame republishes the row ahead of its handle: a NEW gap, which must be waited on. + publishRowWithoutHandle() + expect(hasHostMirrorHandleWaitExpired(ENV_ID, TAB_ID)).toBe(false) + const secondRun = vi.fn() + parkUntilHostMirrorHandleLands(ENV_ID, 'wt', TAB_ID, secondRun) + expect(countParkedHostMirrorHandleGapPanesForTests()).toBe(1) + expect(secondRun).not.toHaveBeenCalled() + }) + + it('a wait re-parked under a new worktree is released by that worktree, not the old one', () => { + useAppStore.setState({ + tabsByWorktree: { + 'wt-old': [{ id: TAB_ID, title: 't', ptyId: null }] as never, + 'wt-new': [{ id: TAB_ID, title: 't', ptyId: null }] as never + }, + ptyIdsByTabId: {} + }) + parkUntilHostMirrorHandleLands(ENV_ID, 'wt-old', TAB_ID, vi.fn()) + const replayAfterAdoption = vi.fn() + parkUntilHostMirrorHandleLands(ENV_ID, 'wt-new', TAB_ID, replayAfterAdoption) + + // Only the OLD worktree's rows are retracted. That says nothing about the live wait. + useAppStore.setState({ + tabsByWorktree: { 'wt-new': [{ id: TAB_ID, title: 't', ptyId: null }] as never } + }) + expect(replayAfterAdoption).not.toHaveBeenCalled() + expect(countParkedHostMirrorHandleGapPanesForTests()).toBe(1) + + // Retracting the worktree the wait is actually about does release it. + useAppStore.setState({ tabsByWorktree: {} }) + expect(replayAfterAdoption).toHaveBeenCalledTimes(1) + }) +}) diff --git a/src/renderer/src/lib/host-mirror-handle-gap-wait.ts b/src/renderer/src/lib/host-mirror-handle-gap-wait.ts index 37cc8b2ba02..ab17f8fac7f 100644 --- a/src/renderer/src/lib/host-mirror-handle-gap-wait.ts +++ b/src/renderer/src/lib/host-mirror-handle-gap-wait.ts @@ -46,6 +46,10 @@ function paneWaitKey(environmentId: string, tabId: string): string { return `${environmentId}\0${tabId}` } +function tabIdFromPaneWaitKey(key: string): string { + return key.slice(key.indexOf('\0') + 1) +} + /** True once the deadline fired for this pane on the current connection. */ export function hasHostMirrorHandleWaitExpired(environmentId: string, tabId: string): boolean { return ( @@ -65,10 +69,33 @@ function recordExpiredWait(environmentId: string, key: string): void { } } expiredGenerationByPane.set(key, generation) + // Why the subscription outlives the waiter: the verdict below has to be retired when a handle + // lands, and by then the waiter is gone. + startStoreSubscription() +} + +/** + * A landed handle retires the timeout verdict for its pane. + * + * Why this is needed at all: the module's own premise was "a reconnect bumps the connection + * generation and arms a fresh wait", and the #19647 change in this same stack stops recording + * `status: null` for an unreachable host — so `connectionChanged` no longer fires across an + * outage on the same runtime. Without this the first timeout stuck for the rest of the + * generation, and the NEXT handle gap on that pane got no wait at all: straight back to the + * #19735 fork, with the bounded wait removed rather than merely shortened. A published handle is + * positive host evidence and ends the gap episode the deadline was about. + */ +function retireExpiredWaitsWithLandedHandles(state: HandleGapStoreState): void { + // Deleting the current entry mid-iteration is defined for Map, so no snapshot is needed. + for (const key of expiredGenerationByPane.keys()) { + if ((state.ptyIdsByTabId[tabIdFromPaneWaitKey(key)]?.length ?? 0) > 0) { + expiredGenerationByPane.delete(key) + } + } } function stopStoreSubscriptionIfIdle(): void { - if (waitersByPane.size === 0 && unsubscribeStore) { + if (waitersByPane.size === 0 && expiredGenerationByPane.size === 0 && unsubscribeStore) { unsubscribeStore() unsubscribeStore = null } @@ -130,7 +157,9 @@ function startStoreSubscription(): void { return } previous = state + retireExpiredWaitsWithLandedHandles(state) releaseDueWaiters(state) + stopStoreSubscriptionIfIdle() }) } @@ -149,6 +178,10 @@ export function parkUntilHostMirrorHandleLands( const existing = waitersByPane.get(key) if (existing) { existing.run = run + // Why the worktree moves with `run`: adopting an orphaned terminal re-keys `tabsByWorktree` + // without re-keying the record, so a live wait left on the old worktree released on evidence + // about a workspace it is no longer about. + existing.worktreeId = worktreeId return } const generation = getRuntimeEnvironmentConnectionGeneration(environmentId) diff --git a/src/renderer/src/runtime/host-session-mirror-hydration.ts b/src/renderer/src/runtime/host-session-mirror-hydration.ts index be21db6e08b..3650f3b5a9d 100644 --- a/src/renderer/src/runtime/host-session-mirror-hydration.ts +++ b/src/renderer/src/runtime/host-session-mirror-hydration.ts @@ -53,7 +53,14 @@ function drainParkedWaiters(matches: (waiter: ParkedMirrorWaiter) => boolean): v const waiter = parkedWaitersByWorktree.get(key) if (waiter) { parkedWaitersByWorktree.delete(key) - waiter.run() + // Why contained: `run` is the whole worktree resume sweep, and this drains from the + // frame-apply path. A throw would strand every sibling waiter this hydration settled and + // escape into the caller mid-frame. The waiter is already removed, so nothing is held. + try { + waiter.run() + } catch (error) { + console.error(`[host-mirror] parked hydration replay failed for ${key}:`, error) + } } } } From 1095e360c0ee1f906bdc1ad07cd68f790a95331c Mon Sep 17 00:00:00 2001 From: Neil Date: Thu, 10 Sep 2026 17:51:31 -0700 Subject: [PATCH 4/9] fix(runtime): release the bootstrap latch when the post-create read throws MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `tabsByWorktree` row read that decides between release and park sat outside the try. Measured with `useAppStore.getState` throwing once after a successful create: the dispatch rejects with the latch still in `creating`, and `releaseWebRuntimeInitialTerminalBootstrapOnMirrorFrame` only ever clears `awaiting-mirror` — so no mirror frame can rescue it and every later dispatch for that workspace returns false without creating, until environment teardown. --- ...nitial-terminal-bootstrap-dispatch.test.ts | 52 +++++++++++++++++++ ...ime-initial-terminal-bootstrap-dispatch.ts | 25 +++++---- 2 files changed, 66 insertions(+), 11 deletions(-) create mode 100644 src/renderer/src/runtime/web-runtime-initial-terminal-bootstrap-dispatch.test.ts diff --git a/src/renderer/src/runtime/web-runtime-initial-terminal-bootstrap-dispatch.test.ts b/src/renderer/src/runtime/web-runtime-initial-terminal-bootstrap-dispatch.test.ts new file mode 100644 index 00000000000..1f1ee5f5bd9 --- /dev/null +++ b/src/renderer/src/runtime/web-runtime-initial-terminal-bootstrap-dispatch.test.ts @@ -0,0 +1,52 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { useAppStore } from '@/store' +import { dispatchWebRuntimeInitialTerminalBootstrap } from './web-runtime-initial-terminal-bootstrap-dispatch' +import { + isWebRuntimeInitialTerminalBootstrapInFlight, + releaseWebRuntimeInitialTerminalBootstrapOnMirrorFrame, + resetWebRuntimeInitialTerminalBootstrapForTests +} from './web-runtime-initial-terminal-bootstrap' + +const createTerminal = vi.hoisted(() => vi.fn()) +vi.mock('./web-runtime-session', () => ({ createWebRuntimeSessionTerminal: createTerminal })) + +// What this pins: a throw AFTER the create resolves must not leave the latch in `creating`. +// `releaseWebRuntimeInitialTerminalBootstrapOnMirrorFrame` only ever clears `awaiting-mirror`, so +// a stranded `creating` claim survives every later mirror frame and makes the workspace refuse to +// bootstrap a terminal for the rest of the environment's life. + +const ENV_ID = 'env-bootstrap-dispatch' +const WORKTREE_ID = 'repo-1::/w/one' + +describe('dispatchWebRuntimeInitialTerminalBootstrap', () => { + beforeEach(() => { + resetWebRuntimeInitialTerminalBootstrapForTests() + createTerminal.mockReset() + }) + + afterEach(() => { + resetWebRuntimeInitialTerminalBootstrapForTests() + vi.restoreAllMocks() + }) + + it('releases the latch when the post-create store read throws', async () => { + createTerminal.mockResolvedValue({ status: 'created' }) + const getState = vi.spyOn(useAppStore, 'getState').mockImplementation(() => { + throw new Error('store read blew up') + }) + + await expect(dispatchWebRuntimeInitialTerminalBootstrap(ENV_ID, WORKTREE_ID)).rejects.toThrow( + 'store read blew up' + ) + getState.mockRestore() + + expect(isWebRuntimeInitialTerminalBootstrapInFlight(ENV_ID, WORKTREE_ID)).toBe(false) + // A mirror frame cannot rescue a stranded `creating` claim, so the latch had to release itself. + releaseWebRuntimeInitialTerminalBootstrapOnMirrorFrame(ENV_ID, WORKTREE_ID) + expect(isWebRuntimeInitialTerminalBootstrapInFlight(ENV_ID, WORKTREE_ID)).toBe(false) + + createTerminal.mockResolvedValue({ status: 'created' }) + await dispatchWebRuntimeInitialTerminalBootstrap(ENV_ID, WORKTREE_ID) + expect(createTerminal).toHaveBeenCalledTimes(2) + }) +}) diff --git a/src/renderer/src/runtime/web-runtime-initial-terminal-bootstrap-dispatch.ts b/src/renderer/src/runtime/web-runtime-initial-terminal-bootstrap-dispatch.ts index d4f097a2a68..ad554d1f1c6 100644 --- a/src/renderer/src/runtime/web-runtime-initial-terminal-bootstrap-dispatch.ts +++ b/src/renderer/src/runtime/web-runtime-initial-terminal-bootstrap-dispatch.ts @@ -31,22 +31,25 @@ export async function dispatchWebRuntimeInitialTerminalBootstrap( return false } let outcome: WebRuntimeTerminalCreateOutcome + // Why the row read is inside the try too: a throw between the create and the latch decision + // left `creating` held forever — the mirror-frame release only ever clears `awaiting-mirror`, + // so every later dispatch for that workspace refused without creating until teardown. try { outcome = await createWebRuntimeSessionTerminal({ worktreeId, environmentId, activate: true }) + // Why check the outcome: the create reports RPC and network failures as `{ status: 'failed' }` + // rather than throwing, so the catch below never sees them. Both arms report the same way. + if (outcome.status === 'failed') { + endWebRuntimeInitialTerminalBootstrap(environmentId, worktreeId) + return false + } + if (Object.hasOwn(useAppStore.getState().tabsByWorktree, worktreeId)) { + endWebRuntimeInitialTerminalBootstrap(environmentId, worktreeId) + } else { + markWebRuntimeInitialTerminalBootstrapAwaitingMirror(environmentId, worktreeId) + } } catch (error) { endWebRuntimeInitialTerminalBootstrap(environmentId, worktreeId) throw error } - // Why check the outcome: the create reports RPC and network failures as `{ status: 'failed' }` - // rather than throwing, so the catch above never sees them. Both arms report the same way. - if (outcome.status === 'failed') { - endWebRuntimeInitialTerminalBootstrap(environmentId, worktreeId) - return false - } - if (Object.hasOwn(useAppStore.getState().tabsByWorktree, worktreeId)) { - endWebRuntimeInitialTerminalBootstrap(environmentId, worktreeId) - } else { - markWebRuntimeInitialTerminalBootstrapAwaitingMirror(environmentId, worktreeId) - } return true } From 1bb7489a929bd25448b993026ab07607bf2efaa5 Mon Sep 17 00:00:00 2001 From: Neil Date: Thu, 10 Sep 2026 17:54:03 -0700 Subject: [PATCH 5/9] fix(relay): a throwing retry step must not kill the retry chain MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `RelayRetrySchedule` ran the caller's recovery step bare inside its timer and resolved `armed` only on the line after it. Injected a synchronous throw: the error escaped the timer as an uncaughtException, `armed.resolve()` never ran, and the schedule was left with no armed timer — so every waiter on `settled` parked on a promise nothing would ever settle and nothing re-entered the chain. The reachable trigger is the origin pool's drain recovery, which calls `onStatus('draining')` straight through to `webContents.send`. `state.mainWindow` is nulled only on `'closed'`, so between destroy and that event the send throws `Object has been destroyed` — every other `webContents.send` under `src/main/startup/` already guards with `isDestroyed()`. Guarded here too, so the trigger is removed as well as contained. --- .../relay/relay-retry-schedule.test.ts | 26 +++++++++++++++++++ .../runtime/relay/relay-retry-schedule.ts | 14 ++++++++-- src/main/startup/desktop-relay-startup.ts | 14 +++++++--- 3 files changed, 48 insertions(+), 6 deletions(-) diff --git a/src/main/runtime/relay/relay-retry-schedule.test.ts b/src/main/runtime/relay/relay-retry-schedule.test.ts index aff1ce6319a..db99e31382e 100644 --- a/src/main/runtime/relay/relay-retry-schedule.test.ts +++ b/src/main/runtime/relay/relay-retry-schedule.test.ts @@ -27,6 +27,32 @@ describe('RelayRetrySchedule', () => { expect(schedule.settled).toBeNull() }) + // Why this matters beyond the throw itself: the retry callback is the caller's whole recovery + // step (the origin pool's `handleDrain` reaches `onStatus` -> `webContents.send`). A throw used + // to escape the timer AND skip the resolve, parking every `settled` waiter on a promise nothing + // would settle while the schedule held no armed timer — dead with no re-entry. + it('settles and stays reschedulable when the retry throws', async () => { + vi.useFakeTimers() + const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {}) + const schedule = new RelayRetrySchedule(() => 0.5) + schedule.schedule(0, () => { + throw new Error('recovery step blew up') + }) + const settled = schedule.settled! + + await vi.advanceTimersByTimeAsync(500) + + await expect(settled).resolves.toBeUndefined() + expect(schedule.pending).toBe(false) + expect(consoleError).toHaveBeenCalled() + + const next = vi.fn() + schedule.schedule(0, next) + await vi.advanceTimersByTimeAsync(10_000) + expect(next).toHaveBeenCalledTimes(1) + consoleError.mockRestore() + }) + it('settles on cancel without running the retry', async () => { vi.useFakeTimers() const schedule = new RelayRetrySchedule(() => 0.5) diff --git a/src/main/runtime/relay/relay-retry-schedule.ts b/src/main/runtime/relay/relay-retry-schedule.ts index dcab1069e37..a3ef621d454 100644 --- a/src/main/runtime/relay/relay-retry-schedule.ts +++ b/src/main/runtime/relay/relay-retry-schedule.ts @@ -28,8 +28,18 @@ export class RelayRetrySchedule { this.timer = setTimeout(() => { this.timer = null this.armed = null - retry() - armed.resolve() + // Why contained: `retry` is the caller's whole recovery step, run bare inside a timer. A + // synchronous throw there escaped as an uncaughtException AND skipped the resolve, so every + // waiter on `settled` parked on a promise nothing would ever settle, while the schedule was + // left with no armed timer — the chain dead with no re-entry. Waking the waiters is right + // either way: the retry is over, however it ended. + try { + retry() + } catch (error) { + console.error('[relay] scheduled retry threw:', error) + } finally { + armed.resolve() + } }, delayMs) } diff --git a/src/main/startup/desktop-relay-startup.ts b/src/main/startup/desktop-relay-startup.ts index 1846c004193..d61701de381 100644 --- a/src/main/startup/desktop-relay-startup.ts +++ b/src/main/startup/desktop-relay-startup.ts @@ -23,10 +23,16 @@ export function startDesktopRelayService(runtimeRpc: OrcaRuntimeRpcServer): void onStatus: (status, cellUrl) => { state.desktopRelayStatus = status state.desktopRelayCellUrl = cellUrl - state.mainWindow?.webContents.send('mobile:relayStatusChanged', { - status, - ...(cellUrl === undefined ? {} : { cellUrl }) - } satisfies MobileRelayStatusDetail) + // Why isDestroyed and not just the optional chain: `state.mainWindow` is nulled on + // 'closed', so between destroy and that event `webContents.send` throws + // "Object has been destroyed". This callback runs from inside a bare `setTimeout` + // recovery step, where a throw killed the whole retry chain. + if (state.mainWindow && !state.mainWindow.isDestroyed()) { + state.mainWindow.webContents.send('mobile:relayStatusChanged', { + status, + ...(cellUrl === undefined ? {} : { cellUrl }) + } satisfies MobileRelayStatusDetail) + } } }) state.desktopRelayService = relayService From 25096b39d8ea312d0b0da8abe3b77a2eb205d07b Mon Sep 17 00:00:00 2001 From: Neil Date: Thu, 10 Sep 2026 17:56:17 -0700 Subject: [PATCH 6/9] fix(relay): the demand wake signal must not fail the grant it wakes for MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `withTransientDemand` called `refreshDemand()` bare on both sides of the operation. It reaches the device registry (`nextPendingExpiry` -> `listDevices`) and the settings store (`hasDemand` -> `isRelayAllowedForDevice`), so it can fail on its own — and measured, each call site failed the operation instead: - the pre-call threw with the transient ref already acquired, so the ref was never released and the operation never ran. Transient refs have no expiry, so that ref holds relay demand for the rest of the process. - the teardown call replaced the operation's own result in both directions: a named mint failure arrived as `listDevices exploded`, and a SUCCESSFUL mint arrived as a rejection. Both are wake signals; the liveness tick and the next refresh re-ask, so a lost signal is recoverable where a stranded ref is not. The containment lives beside the ledger that owns the ref, because desktop-relay-service.ts is at its max-lines ceiling and this must not cost it a line. Also carries the original error as the `cause` of the mid-operation policy flip rewrite, which was discarding it — the one place in a commit about naming causes that destroyed one. --- ...-service-transient-demand-teardown.test.ts | 113 ++++++++++++++++++ .../runtime/relay/desktop-relay-service.ts | 8 +- src/main/runtime/relay/relay-demand-ledger.ts | 19 +++ 3 files changed, 136 insertions(+), 4 deletions(-) create mode 100644 src/main/runtime/relay/desktop-relay-service-transient-demand-teardown.test.ts diff --git a/src/main/runtime/relay/desktop-relay-service-transient-demand-teardown.test.ts b/src/main/runtime/relay/desktop-relay-service-transient-demand-teardown.test.ts new file mode 100644 index 00000000000..8ac7e62549f --- /dev/null +++ b/src/main/runtime/relay/desktop-relay-service-transient-demand-teardown.test.ts @@ -0,0 +1,113 @@ +import { describe, expect, it, vi } from 'vitest' +import { DesktopRelayService } from './desktop-relay-service' +import type { MobilePairingConnectionMode } from '../../../shared/mobile-pairing-connection-mode' + +/** + * `withTransientDemand`'s teardown runs work that can fail on its own. + * + * `refreshDemand` reaches the device registry (`nextPendingExpiry` -> `listDevices`) and the + * settings store (`hasDemand` -> `isRelayAllowedForDevice`). Running it bare in the `finally` let + * its failure replace the operation's result in BOTH directions: a named mint failure arrived as + * an unrelated message, and a successful mint arrived as a rejection. Before the operation it was + * worse still: the throw landed after the transient ref was acquired, so the ref was never + * released, and transient refs have no expiry. + */ +type TransientDemandHost = { + withTransientDemand: ( + kind: string, + deviceId: string, + operation: () => Promise + ) => Promise +} + +function serviceWithTeardown(options: { + release?: () => void + refreshDemand?: () => void +}): (operation: () => Promise) => Promise { + const service = Object.create(DesktopRelayService.prototype) as DesktopRelayService + Object.assign(service, { + hostMobilePairingConnectionMode: () => 'automatic' as MobilePairingConnectionMode, + runtimeRpc: { + getDeviceRegistry: () => ({ getMobilePairingConnectionMode: () => 'automatic' }) + }, + demandLedger: { acquireTransient: () => options.release ?? ((): void => {}) }, + refreshDemand: options.refreshDemand ?? ((): void => {}) + }) + const host = service as unknown as TransientDemandHost + return (operation) => host.withTransientDemand.call(service, 'pairing', 'device-1', operation) +} + +describe('DesktopRelayService transient-demand teardown', () => { + it('keeps the operation failure when the demand refresh throws', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + const run = serviceWithTeardown({ + refreshDemand: () => { + throw new Error('listDevices exploded') + } + }) + + await expect( + run(async () => { + throw new Error('relay_broker_rejected') + }) + ).rejects.toThrow('relay_broker_rejected') + expect(warn).toHaveBeenCalled() + warn.mockRestore() + }) + + it('does not strand the demand ref when the pre-operation refresh throws', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + const release = vi.fn() + const operation = vi.fn(async () => 'minted') + const run = serviceWithTeardown({ + release, + refreshDemand: () => { + throw new Error('listDevices exploded') + } + }) + + // The ref has no expiry, so failing the wake signal must not take the release with it. + await expect(run(operation)).resolves.toBe('minted') + expect(operation).toHaveBeenCalledTimes(1) + expect(release).toHaveBeenCalledTimes(1) + warn.mockRestore() + }) + + it('keeps a successful mint successful when the demand refresh throws', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + const run = serviceWithTeardown({ + refreshDemand: () => { + throw new Error('listDevices exploded') + } + }) + + await expect(run(async () => 'minted')).resolves.toBe('minted') + warn.mockRestore() + }) + + it('carries the original failure as the cause of a mid-operation policy flip', async () => { + const mode = { current: 'automatic' as MobilePairingConnectionMode } + const service = Object.create(DesktopRelayService.prototype) as DesktopRelayService + Object.assign(service, { + hostMobilePairingConnectionMode: () => mode.current, + runtimeRpc: { + getDeviceRegistry: () => ({ getMobilePairingConnectionMode: () => 'automatic' }) + }, + demandLedger: { acquireTransient: () => (): void => {} }, + refreshDemand: () => {} + }) + const host = service as unknown as TransientDemandHost + + const rejection = await host.withTransientDemand + .call(service, 'pairing', 'device-1', async () => { + mode.current = 'local-only' + throw new Error('relay_token_exchange_failed_503') + }) + .catch((error: unknown) => error) + + expect((rejection as Error).message).toBe('relay_disabled_for_device') + expect(((rejection as Error).cause as Error | undefined)?.message).toBe( + 'relay_token_exchange_failed_503' + ) + }) +}) diff --git a/src/main/runtime/relay/desktop-relay-service.ts b/src/main/runtime/relay/desktop-relay-service.ts index f108fda2968..cba87beaa52 100644 --- a/src/main/runtime/relay/desktop-relay-service.ts +++ b/src/main/runtime/relay/desktop-relay-service.ts @@ -18,7 +18,7 @@ import type { RelayRevokeOutboxItem } from './relay-revoke-outbox' import { deriveRelayHostId } from './relay-http-client' -import { RelayDemandLedger } from './relay-demand-ledger' +import { RelayDemandLedger, refreshRelayDemandBestEffort } from './relay-demand-ledger' import { createRelayRegionPreferenceReader } from './relay-region-preference' import { pairingAuthorizationForContext } from './relay-pairing-authorization' import { buildPairingEndpointsResult } from './relay-pairing-endpoints-result' @@ -292,7 +292,7 @@ export class DesktopRelayService { throw new Error('relay_disabled_for_device') } const release = this.demandLedger.acquireTransient(`${kind}:${deviceId}`, deviceId) - this.refreshDemand() + refreshRelayDemandBestEffort(() => this.refreshDemand()) try { return await operation() } catch (error) { @@ -301,12 +301,12 @@ export class DesktopRelayService { // reaches `standby` and clears the offline reason. The wait then ends with no cause at all // — the generic `relay_control_not_active` — when the flip is exactly the cause. if (!this.isRelayAllowedForDevice(deviceId)) { - throw new Error('relay_disabled_for_device') + throw new Error('relay_disabled_for_device', { cause: error }) } throw error } finally { release() - this.refreshDemand() + refreshRelayDemandBestEffort(() => this.refreshDemand()) } } diff --git a/src/main/runtime/relay/relay-demand-ledger.ts b/src/main/runtime/relay/relay-demand-ledger.ts index 817fbc37e37..ea5ee4e14e4 100644 --- a/src/main/runtime/relay/relay-demand-ledger.ts +++ b/src/main/runtime/relay/relay-demand-ledger.ts @@ -13,6 +13,25 @@ type RelayDemandLedgerOptions = { type TransientRef = { deviceId: string; count: number } +/** + * Run a demand refresh as the wake signal it is. + * + * A refresh reaches the device registry (`nextPendingExpiry` -> `listDevices`) and the settings + * store (`hasDemand` -> the host pairing mode), so it can fail on its own. Bare, it failed the + * caller it was waking for: before an operation it threw with a transient ref already acquired — + * and those have no expiry, so the ref held relay demand for the rest of the process — and after + * one it replaced the operation's own result, turning a named mint failure into an unrelated + * message and a successful mint into a rejection. A lost wake signal is recoverable; the liveness + * tick and the next refresh both re-ask. + */ +export function refreshRelayDemandBestEffort(refresh: () => void): void { + try { + refresh() + } catch (error) { + console.warn('[relay] demand refresh failed:', error) + } +} + export class RelayDemandLedger { private readonly options: RelayDemandLedgerOptions private readonly transientRefs = new Map() From 2051f160088490cfafb6db3a8a138fb6d3440d90 Mon Sep 17 00:00:00 2001 From: Neil Date: Thu, 10 Sep 2026 17:57:43 -0700 Subject: [PATCH 7/9] fix(mobile): a relay status we could not read is not "offline" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two problems in the same newly-extracted collector, both in the artifact a user pastes into a bug report. `.catch(() => 'offline' as const)` reported the definite neighbour for a lookup that observed nothing, so a support engineer could not tell a host that reported offline from one that never answered. It now reports `unreadable`. And the collector could reject: `window.api.mobile.getRelayStatus()` throws synchronously when the bridge has no `mobile` — before `.catch` is attached — while both call sites fire this as `void copyRelayDiagnostics()` with the await sitting AHEAD of their try/catch. Measured: the click wrote no clipboard and showed no toast at all, where the pre-extraction inline payload build could not fail. The collector is total now, and the await moved inside the try so a future addition cannot silently kill the toast again. --- .../src/components/mobile/MobilePage.tsx | 10 ++-- .../mobile-relay-diagnostics-payload.test.ts | 55 ++++++++++++++++++- .../mobile-relay-diagnostics-payload.ts | 35 ++++++++++-- .../src/components/settings/MobilePane.tsx | 10 ++-- 4 files changed, 92 insertions(+), 18 deletions(-) diff --git a/src/renderer/src/components/mobile/MobilePage.tsx b/src/renderer/src/components/mobile/MobilePage.tsx index 4087e483efe..312d896746d 100644 --- a/src/renderer/src/components/mobile/MobilePage.tsx +++ b/src/renderer/src/components/mobile/MobilePage.tsx @@ -152,12 +152,12 @@ export default function MobilePage(): React.JSX.Element { if (relayMintFailure == null) { return } - // Why: users share this payload — an address (selected or relay cell) would leak a LAN/Tailscale IP or hostname. - const payload = await collectMobileRelayDiagnosticsPayload({ - connectionMode, - failure: relayMintFailure - }) try { + // Why: users share this payload — an address (selected or relay cell) would leak a LAN/Tailscale IP or hostname. + const payload = await collectMobileRelayDiagnosticsPayload({ + connectionMode, + failure: relayMintFailure + }) await window.api.ui.writeClipboardText(JSON.stringify(payload, null, 2)) if (mountedRef.current) { toast.success( diff --git a/src/renderer/src/components/mobile/mobile-relay-diagnostics-payload.test.ts b/src/renderer/src/components/mobile/mobile-relay-diagnostics-payload.test.ts index 3bd45ed3368..81c6857e013 100644 --- a/src/renderer/src/components/mobile/mobile-relay-diagnostics-payload.test.ts +++ b/src/renderer/src/components/mobile/mobile-relay-diagnostics-payload.test.ts @@ -1,5 +1,8 @@ -import { describe, expect, it } from 'vitest' -import { buildMobileRelayDiagnosticsPayload } from './mobile-relay-diagnostics-payload' +import { afterEach, describe, expect, it, vi } from 'vitest' +import { + buildMobileRelayDiagnosticsPayload, + collectMobileRelayDiagnosticsPayload +} from './mobile-relay-diagnostics-payload' import type { MobileRelayMintFailure } from '../../../../shared/mobile-relay-mint-failure' const failure: MobileRelayMintFailure = { @@ -47,3 +50,51 @@ describe('buildMobileRelayDiagnosticsPayload', () => { expect(payload).not.toHaveProperty('cellUrl') }) }) + +// Why this matters beyond the field value: the payload is what a user pastes into a bug report, +// and both Copy-diagnostics buttons fire the collector as `void copyRelayDiagnostics()` with the +// await sitting ahead of their try/catch — so a rejection here is a click that writes no +// clipboard and shows no toast at all. +describe('collectMobileRelayDiagnosticsPayload', () => { + afterEach(() => { + Reflect.deleteProperty(globalThis, 'window') + }) + + function stubWindow(mobile: unknown): void { + const api: Record = {} + if (mobile !== undefined) { + api.mobile = mobile + } + Object.defineProperty(globalThis, 'window', { configurable: true, value: { api } }) + } + + it("reports 'unreadable' rather than the definite 'offline' when the status lookup fails", async () => { + stubWindow({ getRelayStatus: vi.fn().mockRejectedValue(new Error('ipc down')) }) + + const payload = await collectMobileRelayDiagnosticsPayload({ + connectionMode: 'automatic', + failure + }) + + expect(payload.relayStatus).toBe('unreadable') + }) + + it('still resolves when the mobile bridge is missing entirely', async () => { + stubWindow(undefined) + + await expect( + collectMobileRelayDiagnosticsPayload({ connectionMode: 'automatic', failure }) + ).resolves.toMatchObject({ relayStatus: 'unreadable' }) + }) + + it('reports a host-answered offline as offline', async () => { + stubWindow({ getRelayStatus: vi.fn().mockResolvedValue({ status: 'offline' }) }) + + const payload = await collectMobileRelayDiagnosticsPayload({ + connectionMode: 'automatic', + failure + }) + + expect(payload.relayStatus).toBe('offline') + }) +}) diff --git a/src/renderer/src/components/mobile/mobile-relay-diagnostics-payload.ts b/src/renderer/src/components/mobile/mobile-relay-diagnostics-payload.ts index 8ccac2eedc8..07ea129fa05 100644 --- a/src/renderer/src/components/mobile/mobile-relay-diagnostics-payload.ts +++ b/src/renderer/src/components/mobile/mobile-relay-diagnostics-payload.ts @@ -3,11 +3,18 @@ import type { MobileRelayMintFailure } from '../../../../shared/mobile-relay-min import type { MobileRelayStatus } from '../../../../shared/mobile-relay-status' import { resolveClientEnvironmentInfo } from '@/lib/client-environment-info' +/** The status lookup's own failure, kept distinct from the host answering 'offline'. */ +export const MOBILE_RELAY_DIAGNOSTICS_STATUS_UNREADABLE = 'unreadable' + +export type MobileRelayDiagnosticsStatus = + | MobileRelayStatus + | typeof MOBILE_RELAY_DIAGNOSTICS_STATUS_UNREADABLE + export type MobileRelayDiagnosticsPayload = { kind: 'mobile_pairing_relay_failure' preferredConnectionMode: MobilePairingConnectionMode failure: MobileRelayMintFailure - relayStatus: MobileRelayStatus + relayStatus: MobileRelayDiagnosticsStatus appVersion: string at: string } @@ -17,7 +24,7 @@ export type MobileRelayDiagnosticsPayload = { export function buildMobileRelayDiagnosticsPayload(args: { connectionMode: MobilePairingConnectionMode failure: MobileRelayMintFailure - relayStatus: MobileRelayStatus + relayStatus: MobileRelayDiagnosticsStatus appVersion: string }): MobileRelayDiagnosticsPayload { return { @@ -30,6 +37,25 @@ export function buildMobileRelayDiagnosticsPayload(args: { } } +/** + * Why not 'offline' on failure: this payload is what a user pastes into a bug report, and + * 'offline' is a claim the broker was down. A status call this renderer could not complete + * observed nothing (docs/reference/ssh-execution-boundary.md), and reporting it as the definite + * neighbour points triage at the relay instead of at the lookup that actually failed. + * + * Why the whole call and not just a `.catch`: both Copy-diagnostics buttons fire this as + * `void copyRelayDiagnostics()`, and the await now sits ahead of their try/catch, so a bridge + * missing `mobile` — which throws where a rejected promise was expected — would leave the click + * with no clipboard write and no toast at all. + */ +async function readRelayStatusForDiagnostics(): Promise { + try { + return (await window.api.mobile.getRelayStatus()).status + } catch { + return MOBILE_RELAY_DIAGNOSTICS_STATUS_UNREADABLE + } +} + // Why here rather than at each call site: both "Copy diagnostics" buttons must // fetch the same two fields the same way, and MobilePane sits at the line ceiling. export async function collectMobileRelayDiagnosticsPayload(args: { @@ -37,10 +63,7 @@ export async function collectMobileRelayDiagnosticsPayload(args: { failure: MobileRelayMintFailure }): Promise { const [relayStatus, environment] = await Promise.all([ - window.api.mobile - .getRelayStatus() - .then((detail) => detail.status) - .catch(() => 'offline' as const), + readRelayStatusForDiagnostics(), resolveClientEnvironmentInfo() ]) return buildMobileRelayDiagnosticsPayload({ diff --git a/src/renderer/src/components/settings/MobilePane.tsx b/src/renderer/src/components/settings/MobilePane.tsx index 814f3beeaa1..78904d81222 100644 --- a/src/renderer/src/components/settings/MobilePane.tsx +++ b/src/renderer/src/components/settings/MobilePane.tsx @@ -306,12 +306,12 @@ export function MobilePane(): React.JSX.Element { if (relayMintFailure == null) { return } - // Why: users share this payload, so it carries no address (selected or relay cell). - const payload = await collectMobileRelayDiagnosticsPayload({ - connectionMode, - failure: relayMintFailure - }) try { + // Why: users share this payload, so it carries no address (selected or relay cell). + const payload = await collectMobileRelayDiagnosticsPayload({ + connectionMode, + failure: relayMintFailure + }) await window.api.ui.writeClipboardText(JSON.stringify(payload, null, 2)) if (mountedRef.current) { toast.success( From 794089794ac00db0217913a92a918e4628690598 Mon Sep 17 00:00:00 2001 From: Neil Date: Thu, 10 Sep 2026 17:58:43 -0700 Subject: [PATCH 8/9] fix(relay): keep the diagnostic the refusal path exists to produce MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two error paths that destroy their own evidence. `parseHandshakeMessage`'s unknown-type refusal interpolated `String(t)` on a peer-supplied value: `{"type":{"toString":1}}` makes String() throw "Cannot convert object to primitive value", so the refusal arrives without naming what was refused. `describeRelayProtocolVersion` guards this exact hazard two files away; the sibling was missed. `runRelayOrcaCliChannel`'s new `onDecodeError` wrote to stderr and then exited synchronously. stderr is async on a pipe transport, so the one line recording why the command died could be dropped — the reason relay-handshake.ts already exits inside its write callback. --- src/relay/protocol-handshake.test.ts | 7 +++++++ src/relay/protocol.ts | 5 ++++- src/relay/relay-orca-cli-channel.ts | 10 +++++++--- 3 files changed, 18 insertions(+), 4 deletions(-) diff --git a/src/relay/protocol-handshake.test.ts b/src/relay/protocol-handshake.test.ts index c50f8ae2d4b..68ea0178ef7 100644 --- a/src/relay/protocol-handshake.test.ts +++ b/src/relay/protocol-handshake.test.ts @@ -61,6 +61,13 @@ describe('handshake framing', () => { expect(() => parseHandshakeMessage(bogus)).toThrow(/Unknown handshake type/) }) + // `type` is peer-supplied, so it can be an object whose String() conversion throws — which + // replaced the one diagnostic this refusal exists to produce with a primitive-conversion error. + it('still names the refusal when the peer type cannot be stringified', () => { + const hostile = Buffer.from(JSON.stringify({ type: { toString: 1 } })) + expect(() => parseHandshakeMessage(hostile)).toThrow(/Unknown handshake type: object/) + }) + // The daemon logs the peer's version before any credential check, and `JSON.parse` can hand // back a value a template literal throws on. The parser is the one place every reader shares. it('rejects a version that is not a string on both arms that carry one', () => { diff --git a/src/relay/protocol.ts b/src/relay/protocol.ts index bc54a5f4f6d..1a2f7882930 100644 --- a/src/relay/protocol.ts +++ b/src/relay/protocol.ts @@ -107,7 +107,10 @@ export function parseHandshakeMessage(payload: Buffer): HandshakeMessage { ? HANDSHAKE_STRING_FIELDS[t as HandshakeMessage['type']] : null if (required === null) { - throw new Error(`Unknown handshake type: ${String(t)}`) + // Why typeof and not String(t): a peer-supplied `{ "type": { "toString": 1 } }` makes String() + // itself throw "Cannot convert object to primitive value", replacing the one diagnostic this + // line exists to produce. + throw new Error(`Unknown handshake type: ${typeof t === 'string' ? t : typeof t}`) } for (const field of required) { if (typeof msg[field] !== 'string') { diff --git a/src/relay/relay-orca-cli-channel.ts b/src/relay/relay-orca-cli-channel.ts index e174850c309..1d528c63280 100644 --- a/src/relay/relay-orca-cli-channel.ts +++ b/src/relay/relay-orca-cli-channel.ts @@ -154,9 +154,13 @@ export async function runRelayOrcaCliChannel( // Why an explicit error path: the decoder contains a throwing frame owner instead of letting // it escape, so a malformed relay reply must still end this one-shot command, not park it. const onDecodeError = (error: Error): void => { - process.stderr.write(`[orca-cli] Relay protocol error: ${error.message}\n`) - sock.destroy() - process.exit(1) + // Why exit inside the write callback: stderr is async on pipe transports, so exiting early + // drops the only evidence this failure ever produces — the same reason relay-handshake.ts + // writes its mismatch line this way. + process.stderr.write(`[orca-cli] Relay protocol error: ${error.message}\n`, () => { + sock.destroy() + process.exit(1) + }) } const decoder = new FrameDecoder((frame: DecodedFrame) => { if (frame.id > highestReceivedSeq) { From a5dfddf6fa2f396392f21faef7d48f6a3a9597bc Mon Sep 17 00:00:00 2001 From: Neil Date: Thu, 10 Sep 2026 18:16:35 -0700 Subject: [PATCH 9/9] test(runtime): supply the host home the SSH orphan-cleanup gate now requires The scenario registers an SSH filesystem provider directly. In production that provider is minted by the relay session that also reports the host's `$HOME`, so the test has to supply the other half rather than rely on the recursive-delete gate falling open on an unknown home. --- .../worktree-removal-and-reconciliation-part-03.spec.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/main/runtime/orca-runtime-tests/worktree-removal-and-reconciliation-part-03.spec.ts b/src/main/runtime/orca-runtime-tests/worktree-removal-and-reconciliation-part-03.spec.ts index 06982cf179e..9daec4aec70 100644 --- a/src/main/runtime/orca-runtime-tests/worktree-removal-and-reconciliation-part-03.spec.ts +++ b/src/main/runtime/orca-runtime-tests/worktree-removal-and-reconciliation-part-03.spec.ts @@ -35,6 +35,7 @@ import { store } from '../orca-runtime-test-fixtures.spec' import { createWorktreeRemovalRuntime } from '../orca-runtime-test-scenario-builders.spec' +import { setWorktreeRemovalSshHostHomeResolver } from '../../worktree-removal-execution-host-route' describe('OrcaRuntimeService', () => { it('force-deletes a preserved branch on the qualified host when repo ids collide', async () => { @@ -453,6 +454,10 @@ describe('OrcaRuntimeService', () => { } registerSshGitProvider(repo.connectionId, gitProvider as never) registerSshFilesystemProvider(repo.connectionId, fsProvider as never) + // The recursive-delete gate needs the execution host to have reported its `$HOME`. In + // production that comes from the same relay session that minted this fs provider; the test + // registers the provider directly, so it has to supply the other half. + setWorktreeRemovalSshHostHomeResolver(() => '/remote/home/alice') const runtime = new OrcaRuntimeService(runtimeStore as never, undefined, { getSshProvider: () => ptyProvider as never }) @@ -460,6 +465,7 @@ describe('OrcaRuntimeService', () => { try { await expect(runtime.removeManagedWorktree(`id:${worktreeId}`, true)).resolves.toEqual({}) } finally { + setWorktreeRemovalSshHostHomeResolver(() => null) unregisterSshGitProvider(repo.connectionId) unregisterSshFilesystemProvider(repo.connectionId) }