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/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) } 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 ca961759e4e..29d1bc54983 100644 --- a/src/main/runtime/relay/desktop-relay-service.ts +++ b/src/main/runtime/relay/desktop-relay-service.ts @@ -15,7 +15,7 @@ import type { PairingRelay } from '../../../shared/mobile-relay-pairing-offer' import type { RelayDeviceBinding, RelayRevokeOutboxItem } from './relay-revoke-outbox' import { RelayRevokeOutboxFlusher } from './relay-revoke-outbox-flush' 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' @@ -290,7 +290,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) { @@ -299,12 +299,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 cc585351ae9..7aad8959d45 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() 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/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/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 diff --git a/src/main/worktree-removal-safety.ts b/src/main/worktree-removal-safety.ts index 0d84e3af354..7ca6c1a289c 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) || 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) { 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( 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-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/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 }