diff --git a/src/main/runtime/relay/relay-control-client.test.ts b/src/main/runtime/relay/relay-control-client.test.ts index 431d26574cf..6896b1064e7 100644 --- a/src/main/runtime/relay/relay-control-client.test.ts +++ b/src/main/runtime/relay/relay-control-client.test.ts @@ -1,5 +1,6 @@ import { createHash, createHmac, randomBytes } from 'node:crypto' import { EventEmitter } from 'node:events' +import { createServer, type Server, type Socket } from 'node:net' import { afterEach, describe, expect, it, vi } from 'vitest' import nacl from 'tweetnacl' import { WebSocketServer, type WebSocket } from 'ws' @@ -84,11 +85,25 @@ function nextJson(ws: WebSocket): Promise> { describe('RelayControlClient', () => { const servers: WebSocketServer[] = [] const clients: RelayControlClient[] = [] + /** Raw TCP listeners that accept but never upgrade; they have no WebSocketServer to close. */ + const silentServers: Server[] = [] + const silentSockets: Socket[] = [] afterEach(async () => { for (const client of clients.splice(0)) { client.closeNow() } + for (const socket of silentSockets.splice(0)) { + socket.destroy() + } + await Promise.all( + silentServers.splice(0).map( + (server) => + new Promise((resolve) => { + server.close(() => resolve()) + }) + ) + ) await Promise.all( servers.splice(0).map( (server) => @@ -133,6 +148,38 @@ describe('RelayControlClient', () => { await expect(client.connect()).rejects.toThrow('relay_control_connect_timeout') }) + // Why: the connect deadline is armed in the same tick as the socket and expires from + // 'opening' too, so it already bounds a connect that never opens. Without this a reader + // concludes the phase is uncovered and adds a second, transport-level bound for it. + it('expires a connect whose upgrade is never answered', async () => { + const server = createServer((socket) => { + silentSockets.push(socket) + }) + silentServers.push(server) + await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)) + const address = server.address() + if (!address || typeof address === 'string') { + throw new Error('expected TCP relay test server') + } + const keypair = nacl.box.keyPair() + const client = new RelayControlClient({ + cellUrl: `http://127.0.0.1:${address.port}`, + relayJwt: 'scoped-token', + relayHostId: createHash('sha256').update(keypair.publicKey).digest('base64url').slice(0, 16), + assignmentEpoch: 1, + identity: { userId: 'user-1', profileId: 'profile-1', organizationId: 'org-1' }, + keypair: { ...keypair, publicKeyB64: Buffer.from(keypair.publicKey).toString('base64') }, + appVersion: '1.2.3', + onConnectionOpen: vi.fn(), + onDrain: vi.fn(), + onClose: vi.fn(), + connectDeadlineMs: 150 + }) + clients.push(client) + + await expect(client.connect()).rejects.toThrow('relay_control_connect_timeout') + }) + it('settles an opening control immediately when ownership closes', async () => { const server = new WebSocketServer({ host: '127.0.0.1', port: 0, perMessageDeflate: false }) servers.push(server) diff --git a/src/main/runtime/relay/relay-control-client.ts b/src/main/runtime/relay/relay-control-client.ts index 8321bf8caf2..e60c4a58de8 100644 --- a/src/main/runtime/relay/relay-control-client.ts +++ b/src/main/runtime/relay/relay-control-client.ts @@ -86,6 +86,9 @@ export class RelayControlClient { }) socket.once('close', (code) => this.handleClose(code)) // Recovery cannot advance while an upgrade/proof promise remains pending forever. + // Armed in the same tick as the socket and expiring from 'opening' as well as + // 'proving', so it also bounds a black-holed connect that never opens; a + // transport-level handshakeTimeout here would be a second bound on that phase. this.connectTimer = setTimeout( () => this.expireConnect(), this.options.connectDeadlineMs ?? RELAY_CONTROL_CONNECT_DEADLINE_MS diff --git a/src/shared/remote-runtime-connect-bound.test.ts b/src/shared/remote-runtime-connect-bound.test.ts new file mode 100644 index 00000000000..df70cd13e11 --- /dev/null +++ b/src/shared/remote-runtime-connect-bound.test.ts @@ -0,0 +1,230 @@ +import { readFileSync, readdirSync } from 'node:fs' +import { createServer, type Server, type Socket } from 'node:net' +import { basename, join } from 'node:path' +import { afterEach, describe, expect, it, vi } from 'vitest' +import { generateKeyPair, publicKeyToBase64 } from './e2ee-crypto' +import type { RemoteRuntimeClientError } from './remote-runtime-client-error' +import { isRecoverableRemoteRuntimeConnectionError } from './remote-runtime-client-error-classification' +import { + REMOTE_RUNTIME_CONNECT_TIMEOUT_MS, + WS_HANDSHAKE_TIMEOUT_MESSAGE, + isRemoteRuntimeConnectTimeout, + remoteRuntimeConnectFailureMessage, + remoteRuntimeConnectOptions +} from './remote-runtime-connect-bound' +import { openRemoteRuntimeWebSocket } from './remote-runtime-request-websocket' +import { withRemoteRuntimeTailscaleHint } from './remote-runtime-tailscale-hint' + +const servers = new Set() +const sockets = new Set() + +const handshakeTimeoutError = (): Error => new Error(WS_HANDSHAKE_TIMEOUT_MESSAGE) + +/** + * Files whose WebSocket construction must carry the connect bound: the shared + * remote-runtime transports, swept by prefix. Other WebSocket sites (relay + * control and data transports, emulator control) carry their own bounds and are + * deliberately not covered here. + */ +function coveredSocketSources(): string[] { + return readdirSync(__dirname) + .filter( + (name) => + name.startsWith('remote-runtime-') && name.endsWith('.ts') && !name.includes('.test.') + ) + .map((name) => join(__dirname, name)) +} + +afterEach(async () => { + for (const socket of sockets) { + socket.destroy() + } + sockets.clear() + await Promise.all( + [...servers].map( + (server) => + new Promise((resolve) => { + server.close(() => resolve()) + }) + ) + ) + servers.clear() +}) + +/** + * Accepts TCP but never answers the HTTP upgrade, which is the same silent + * stall a black-holed host produces and is bounded by the same `ws` timer. + */ +async function listenSilentUpgradeServer(): Promise { + const server = createServer((socket) => { + sockets.add(socket) + socket.once('close', () => sockets.delete(socket)) + }) + servers.add(server) + await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)) + const address = server.address() + if (address === null || typeof address === 'string') { + throw new Error('expected a TCP address') + } + return `ws://127.0.0.1:${address.port}` +} + +describe('remote runtime connect bound', () => { + it('bounds the production connect with a finite handshake timeout', () => { + const options = remoteRuntimeConnectOptions({ maxPayload: 1024 }) + expect(Number.isFinite(options.handshakeTimeout)).toBe(true) + expect(options.handshakeTimeout).toBe(REMOTE_RUNTIME_CONNECT_TIMEOUT_MS) + expect(options.maxPayload).toBe(1024) + }) + + // Why: the bound only helps if every Node-side remote-runtime socket carries + // it; a new transport that calls `new WebSocket` directly reintroduces #18191. + it('routes every covered WebSocket construction through the bounded options', () => { + const offenders: string[] = [] + let scannedConstructions = 0 + for (const path of coveredSocketSources()) { + const source = readFileSync(path, 'utf8') + const constructions = source.split('new WebSocket(').length - 1 + const bounded = source.split('remoteRuntimeConnectOptions(').length - 1 + scannedConstructions += constructions + if (constructions > bounded) { + offenders.push(`${basename(path)}: ${constructions} WebSocket(s), ${bounded} bounded`) + } + } + expect(offenders).toEqual([]) + // Guards against the scan silently matching nothing and passing vacuously. + expect(scannedConstructions).toBeGreaterThan(0) + }) + + it('reports an unanswered host as unreachable rather than as an empty result', async () => { + const endpoint = await listenSilentUpgradeServer() + const keyPair = generateKeyPair() + const onError = vi.fn() + const onTextFrame = vi.fn() + + const opened = openRemoteRuntimeWebSocket( + { + v: 2, + endpoint, + deviceToken: 'device-token', + publicKeyB64: publicKeyToBase64(keyPair.publicKey) + }, + { onClose: vi.fn(), onError, onTextFrame }, + 150 + ) + if (!opened.ok) { + throw opened.error + } + + await vi.waitFor(() => expect(onError).toHaveBeenCalledTimes(1), { + timeout: 5_000 + }) + + // The bounded path was taken: a connect failure, not a silent empty answer. + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: vi mock call args are untyped; this subscription's onError is only ever invoked with a RemoteRuntimeClientError. + const error = onError.mock.calls[0][1] as RemoteRuntimeClientError + expect(error.code).toBe('remote_runtime_unavailable') + expect(error.message).toContain(endpoint) + expect(error.message).toContain('unverifiable') + expect(onTextFrame).not.toHaveBeenCalled() + + // Loss of contact is never evidence the host's work stopped. + expect(error.message).not.toMatch(/\b(exited|gone|stopped|empty|no terminals)\b/i) + + // The subscribe IPC boundary drops `code`, so the renderer classifies this + // message alone. Fatal there means `recovery.cancel()` and a dead-ended pane + // instead of a retry, so the real produced message must still read recoverable. + expect(isRecoverableRemoteRuntimeConnectionError({ message: error.message })).toBe(true) + // ...and must still earn the Tailscale remedy, which is gated on the same phrase. + expect(withRemoteRuntimeTailscaleHint(error.message, endpoint)).not.toBe(error.message) + + opened.socket.cleanup() + opened.socket.ws.terminate() + }) + + it('only calls an elapsed handshake a connect timeout', () => { + expect(isRemoteRuntimeConnectTimeout(handshakeTimeoutError())).toBe(true) + expect(isRemoteRuntimeConnectTimeout(new Error('connect ECONNREFUSED'))).toBe(false) + expect(remoteRuntimeConnectFailureMessage(new Error('connect ECONNREFUSED'), 'ws://h')).toBe( + 'Could not connect to the remote Orca runtime.' + ) + }) + + // Why: the message is the only carrier on the code-less paths (subscribe IPC, + // web, mobile). Both gates below match a phrase, so a rewording silently turns + // a retrying pane into a dead-ended one and drops the only actionable remedy. + it('keeps the unreachable-host message inside both message gates', () => { + const message = remoteRuntimeConnectFailureMessage( + handshakeTimeoutError(), + 'ws://desk.example.com:6768' + ) + expect(isRecoverableRemoteRuntimeConnectionError({ message })).toBe(true) + // The shape Electron produces for a rejected ipcMain.handle, which keeps no code. + expect( + isRecoverableRemoteRuntimeConnectionError({ + message: `Error invoking remote method 'runtimeEnvironments:subscribe': Error: ${message}` + }) + ).toBe(true) + expect(withRemoteRuntimeTailscaleHint(message, 'ws://192.168.1.10:6768')).toContain( + 'connect both devices to Tailscale' + ) + expect( + withRemoteRuntimeTailscaleHint( + remoteRuntimeConnectFailureMessage(handshakeTimeoutError(), 'wss://desk.tail1234.ts.net'), + 'wss://desk.tail1234.ts.net' + ) + ).toContain('tailnet') + }) + + // Why: the hint's idempotency check used to key on the word "tailscale" anywhere in the + // message. Now that the message carries the endpoint, such a host would suppress the very + // remedy it needs. + it('still earns the hint when the endpoint itself contains the vendor name', () => { + const endpoint = 'wss://tailscale-box.example.com:6768' + const message = remoteRuntimeConnectFailureMessage(handshakeTimeoutError(), endpoint) + expect(message).toContain('tailscale-box') + expect(withRemoteRuntimeTailscaleHint(message, endpoint)).toContain( + 'connect both devices to Tailscale' + ) + }) + + // Why: the endpoint arrives from a pasted pairing code, which is only length-capped. + it('shows the endpoint origin only, never pasted credentials', () => { + const message = remoteRuntimeConnectFailureMessage( + handshakeTimeoutError(), + 'wss://user:s3cret@desk.example.com:6768/path?token=abc' + ) + expect(message).toContain('wss://desk.example.com:6768') + expect(message).not.toContain('s3cret') + expect(message).not.toContain('token=abc') + }) + + // Why: `isRemoteTerminalGoneMessage` in the pty transport substring-matches these tokens and + // runs BEFORE the recoverable gate, and WHATWG URL accepts `_` in a host. An endpoint could + // otherwise turn loss of contact into a terminal-gone verdict. + it('never lets the endpoint smuggle a terminal-gone token into the message', () => { + for (const host of ['terminal_gone.example', 'terminal_exited.example', 'no_connected_pty']) { + const message = remoteRuntimeConnectFailureMessage( + handshakeTimeoutError(), + `ws://${host}:6768` + ) + expect(message).not.toMatch(/terminal_exited|terminal_gone|no_connected_pty/) + // Still reads as a recoverable connect failure, so the pane keeps retrying. + expect(isRecoverableRemoteRuntimeConnectionError({ message })).toBe(true) + } + // A well-formed host is still shown, so the redaction is not blanket. + expect( + remoteRuntimeConnectFailureMessage(handshakeTimeoutError(), 'ws://[fd7a:115c:a1e0::1]:6768') + ).toContain('[fd7a:115c:a1e0::1]:6768') + }) + + // Why: `ws` and `net` gate on a truthy timeout, so 0 would leave the connect unbounded. + it('refuses a non-positive or non-finite bound and keeps the production default', () => { + for (const value of [0, -1, Number.NaN, Number.POSITIVE_INFINITY]) { + expect(remoteRuntimeConnectOptions(undefined, value).handshakeTimeout).toBe( + REMOTE_RUNTIME_CONNECT_TIMEOUT_MS + ) + } + expect(remoteRuntimeConnectOptions(undefined, 150).handshakeTimeout).toBe(150) + }) +}) diff --git a/src/shared/remote-runtime-connect-bound.ts b/src/shared/remote-runtime-connect-bound.ts new file mode 100644 index 00000000000..40df0927ce4 --- /dev/null +++ b/src/shared/remote-runtime-connect-bound.ts @@ -0,0 +1,103 @@ +import type { ClientOptions } from 'ws' + +/** + * Connect-phase bound for the Node-side remote-runtime WebSocket transports. + * + * Why: a host that is powered off or firewalled black-holes the TCP SYN, so the + * socket neither opens nor errors. Without this the only bound is the caller's + * whole-request timeout (60s in the CLI), which reads to the user as a frozen + * terminal. + * + * `ws` maps `handshakeTimeout` onto the `http.request` `timeout`, which Node + * implements as a socket *inactivity* timer: armed before DNS/connect and reset + * by connect completion and by every response chunk. So this is "12s with no + * bytes at all", not a 12s wall-clock budget — a slow-but-answering host is not + * cut off, while a silent one fails promptly. + * + * The value matches `CONNECT_TIMEOUT_MS` in + * `src/renderer/src/web/web-runtime-connection-transport.ts`, which already + * bounded the browser transport (a wall-clock budget there). + * + * Why this is not the duplicate bound that was removed from the relay control + * socket: there, both timers were 15s and the class one was wall-clock from + * construction, so the transport timer could never win and covered nothing. + * Here the whole-request timer is 15s (60s in the CLI) and measures the RPC, + * not the connect, and this one is inactivity-based and strictly tighter — so + * it is the bound that actually reports an unanswered host, with the specific + * message the recovery classifiers need, rather than a generic RPC timeout. + */ +export const REMOTE_RUNTIME_CONNECT_TIMEOUT_MS = 12_000 + +/** The `ws` message for an elapsed `handshakeTimeout`; matched, never thrown by us. */ +export const WS_HANDSHAKE_TIMEOUT_MESSAGE = 'Opening handshake has timed out' + +/** + * Every connect failure starts with this phrase. It is load-bearing, not copy: + * `RECOVERABLE_MESSAGE_FRAGMENTS` and `REMOTE_RUNTIME_UNREACHABLE_RE` both key + * on it, and the subscribe IPC boundary drops the error `code`, so on that path + * the phrase is the only thing keeping the terminal pane retrying instead of + * dead-ending. Reword it and both gates go silent. + */ +export const REMOTE_RUNTIME_CONNECT_FAILURE_PHRASE = 'Could not connect to the remote Orca runtime' + +export function remoteRuntimeConnectOptions( + options?: TOptions, + connectTimeoutMs?: number +): TOptions & { handshakeTimeout: number } { + return { + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the empty default stands in for an absent TOptions; every property it could carry is optional, and the spread below is the only use. + ...(options ?? ({} as TOptions)), + // Why: `ws` and `net` both gate on a truthy timeout, so 0 (or a non-finite value) + // would silently leave the connect unbounded — the defect this module exists to fix. + handshakeTimeout: + typeof connectTimeoutMs === 'number' && + Number.isFinite(connectTimeoutMs) && + connectTimeoutMs > 0 + ? connectTimeoutMs + : REMOTE_RUNTIME_CONNECT_TIMEOUT_MS + } +} + +/** + * A hostname or IP literal, optionally with a port. Deliberately excludes `_` and anything + * else WHATWG URL tolerates in a host: consumers still substring-match error messages for + * tokens such as `terminal_gone`, so an endpoint carrying one would turn loss of contact into + * a terminal-gone verdict — the one conclusion `ssh-execution-boundary.md` forbids. + */ +const DISPLAYABLE_ENDPOINT_HOST_RE = /^(?:\[[0-9a-f:.]+\]|[a-z0-9.-]+)(?::\d{1,5})?$/i + +/** + * Why: the endpoint comes from a pasted pairing code, which is only length-capped and can + * carry userinfo. Show scheme and host and nothing else, and only when the host cannot smuggle + * a token another consumer reads as a verdict. + */ +function endpointForDisplay(endpoint: string): string { + try { + const { protocol, host } = new URL(endpoint) + return DISPLAYABLE_ENDPOINT_HOST_RE.test(host) ? `${protocol}//${host}` : 'the paired endpoint' + } catch { + return 'the paired endpoint' + } +} + +export function isRemoteRuntimeConnectTimeout(error: unknown): boolean { + return error instanceof Error && error.message === WS_HANDSHAKE_TIMEOUT_MESSAGE +} + +/** + * Why: per `docs/reference/ssh-execution-boundary.md`, loss of contact is never + * evidence that remote work stopped. This message says the host did not answer + * and stops there — it must not imply the host's terminals are gone. + */ +export function remoteRuntimeConnectFailureMessage(error: unknown, endpoint: string): string { + if (!isRemoteRuntimeConnectTimeout(error)) { + return `${REMOTE_RUNTIME_CONNECT_FAILURE_PHRASE}.` + } + // Why no elapsed time: handshakeTimeout is an inactivity timer, so a `wss://` host that + // completes TCP and then goes silent re-arms it once and fails at ~2x the bound. Naming a + // number here would be wrong in that case; the endpoint is the actionable part anyway. + return ( + `${REMOTE_RUNTIME_CONNECT_FAILURE_PHRASE} at ${endpointForDisplay(endpoint)}: the host ` + + 'did not answer, so anything running on it is unverifiable.' + ) +} diff --git a/src/shared/remote-runtime-request-socket.ts b/src/shared/remote-runtime-request-socket.ts index 2ace6b4816f..cb0a04c2898 100644 --- a/src/shared/remote-runtime-request-socket.ts +++ b/src/shared/remote-runtime-request-socket.ts @@ -16,6 +16,10 @@ import { ignoreSettledRemoteRuntimeSocketError } from './remote-runtime-client-handshake' import { RemoteRuntimeClientError } from './remote-runtime-client-error' +import { + remoteRuntimeConnectFailureMessage, + remoteRuntimeConnectOptions +} from './remote-runtime-connect-bound' import { REMOTE_RUNTIME_MAX_WEBSOCKET_FRAME_BYTES, serializeRemoteRuntimePayload, @@ -183,7 +187,10 @@ export async function sendRemoteRuntimeRequestOnSocket( } try { - ws = new WebSocket(pairing.endpoint, { maxPayload: REMOTE_RUNTIME_MAX_WEBSOCKET_FRAME_BYTES }) + const connectOptions = remoteRuntimeConnectOptions({ + maxPayload: REMOTE_RUNTIME_MAX_WEBSOCKET_FRAME_BYTES + }) + ws = new WebSocket(pairing.endpoint, connectOptions) } catch (error) { const message = error instanceof Error ? error.message : String(error) finishError( @@ -201,11 +208,11 @@ export async function sendRemoteRuntimeRequestOnSocket( ) } - function onError(): void { + function onError(error: Error): void { finishError( new RemoteRuntimeClientError( 'remote_runtime_unavailable', - 'Could not connect to the remote Orca runtime.', + remoteRuntimeConnectFailureMessage(error, pairing.endpoint), { pairingStage: router.pairingStage } ) ) diff --git a/src/shared/remote-runtime-request-websocket.ts b/src/shared/remote-runtime-request-websocket.ts index e5b0ad8212d..ca2ddfec60d 100644 --- a/src/shared/remote-runtime-request-websocket.ts +++ b/src/shared/remote-runtime-request-websocket.ts @@ -7,6 +7,10 @@ import { publicKeyToBase64 } from './e2ee-crypto' import { RemoteRuntimeClientError } from './remote-runtime-client' +import { + remoteRuntimeConnectFailureMessage, + remoteRuntimeConnectOptions +} from './remote-runtime-connect-bound' import { invalidRemoteRuntimeResponseError, remoteRuntimeUnavailableError @@ -30,9 +34,12 @@ export type RemoteRuntimeWebSocketCallbacks = { export function openRemoteRuntimeWebSocket( pairing: PairingOffer, - callbacks: RemoteRuntimeWebSocketCallbacks + callbacks: RemoteRuntimeWebSocketCallbacks, + // Why: overridable so the connect-bound regression test can pin the behaviour + // without spending the production budget of wall-clock time. + connectTimeoutMs?: number ): { ok: true; socket: RemoteRuntimeWebSocket } | { ok: false; error: RemoteRuntimeClientError } { - const opened = createSocket(pairing) + const opened = createSocket(pairing, connectTimeoutMs) if (!opened.ok) { return opened } @@ -49,10 +56,10 @@ export function openRemoteRuntimeWebSocket( }) ) } - const onError = (): void => { + const onError = (error: Error): void => { callbacks.onError( ws, - remoteRuntimeUnavailableError('Could not connect to the remote Orca runtime.') + remoteRuntimeUnavailableError(remoteRuntimeConnectFailureMessage(error, pairing.endpoint)) ) } const onClose = (code: number, reason: Buffer): void => callbacks.onClose(ws, code, reason) @@ -100,7 +107,8 @@ export function openRemoteRuntimeWebSocket( function ignoreLateSocketError(): void {} function createSocket( - pairing: PairingOffer + pairing: PairingOffer, + connectTimeoutMs?: number ): | { ok: true; ws: WebSocket; keyPair: ReturnType } | { ok: false; error: RemoteRuntimeClientError } { @@ -119,7 +127,11 @@ function createSocket( } } try { - return { ok: true, ws: new WebSocket(pairing.endpoint), keyPair } + return { + ok: true, + ws: new WebSocket(pairing.endpoint, remoteRuntimeConnectOptions(undefined, connectTimeoutMs)), + keyPair + } } catch (error) { const message = error instanceof Error ? error.message : String(error) return { diff --git a/src/shared/remote-runtime-subscription-connect-bound.test.ts b/src/shared/remote-runtime-subscription-connect-bound.test.ts new file mode 100644 index 00000000000..2304da92cc4 --- /dev/null +++ b/src/shared/remote-runtime-subscription-connect-bound.test.ts @@ -0,0 +1,97 @@ +/** + * The subscribe path is the one that regressed: its connect failure rejects the subscribe + * promise rather than reaching `onError`, and that rejection crosses `ipcMain.handle`, which + * keeps only the message. So this file pins two things together — that the connect bound, not + * the subscription-start timer, is what fires against a silent host, and that the message it + * produces still classifies as recoverable once the code is gone. Split them and a future + * rewording passes both halves while dead-ending the terminal pane. + */ +import { createServer, type Server, type Socket } from 'node:net' +import { afterEach, describe, expect, it, vi } from 'vitest' +import { generateKeyPair, publicKeyToBase64 } from './e2ee-crypto' +import { RemoteRuntimeClientError } from './remote-runtime-client-error' +import { + isRecoverableRemoteRuntimeConnectionError, + toRemoteRuntimeClientErrorLike +} from './remote-runtime-client-error-classification' +import { subscribeRemoteRuntimeTransport } from './remote-runtime-subscription-transport' +import { withRemoteRuntimeTailscaleHint } from './remote-runtime-tailscale-hint' + +const servers = new Set() +const sockets = new Set() + +afterEach(async () => { + for (const socket of sockets) { + socket.destroy() + } + sockets.clear() + await Promise.all( + [...servers].map( + (server) => + new Promise((resolve) => { + server.close(() => resolve()) + }) + ) + ) + servers.clear() +}) + +/** Accepts TCP but never answers the upgrade — the same silence a black-holed host produces. */ +async function listenSilentUpgradeServer(): Promise { + const server = createServer((socket) => { + sockets.add(socket) + socket.once('close', () => sockets.delete(socket)) + }) + servers.add(server) + await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)) + const address = server.address() + if (address === null || typeof address === 'string') { + throw new Error('expected a TCP address') + } + return `ws://127.0.0.1:${address.port}` +} + +describe('remote runtime subscription connect bound', () => { + it('fails an unanswered subscribe on the connect bound, with a message that survives the code strip', async () => { + const endpoint = await listenSilentUpgradeServer() + const keyPair = generateKeyPair() + + const rejection = await subscribeRemoteRuntimeTransport( + { + v: 2, + endpoint, + deviceToken: 'device-token', + publicKeyB64: publicKeyToBase64(keyPair.publicKey) + }, + 'terminal.multiplex', + {}, + // Why: comfortably longer than the connect bound, reproducing production's + // 12s < 15s ordering. If the bound stops firing, the start timer wins and + // the code/message assertions below change. + 2_000, + { onResponse: vi.fn(), onError: vi.fn(), onClose: vi.fn() }, + { connectTimeoutMs: 150 } + ).then( + () => null, + (reason: unknown) => reason + ) + + expect(rejection).toBeInstanceOf(RemoteRuntimeClientError) + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: re-proved by the assertion above. + const error = rejection as RemoteRuntimeClientError + // The connect bound, not the subscription-start timer. + expect(error.code).toBe('remote_runtime_unavailable') + expect(error.code).not.toBe('runtime_timeout') + expect(error.message).toContain(endpoint) + + // Loss of contact is never evidence the host's work stopped. + expect(error.message).toContain('unverifiable') + expect(error.message).not.toMatch(/\b(exited|gone|stopped|empty|no terminals)\b/i) + + // What the renderer actually sees: ipcMain.handle forwards the message only. + const stripped = toRemoteRuntimeClientErrorLike(new Error(error.message)) + expect(stripped.code).toBeUndefined() + expect(isRecoverableRemoteRuntimeConnectionError(stripped)).toBe(true) + expect(withRemoteRuntimeTailscaleHint(error.message, endpoint)).not.toBe(error.message) + }) +}) diff --git a/src/shared/remote-runtime-subscription-contract.ts b/src/shared/remote-runtime-subscription-contract.ts new file mode 100644 index 00000000000..ce37d5ec4d3 --- /dev/null +++ b/src/shared/remote-runtime-subscription-contract.ts @@ -0,0 +1,36 @@ +import type { RemoteRuntimeClientError } from './remote-runtime-client-error' +import type { RuntimeCapability } from './protocol-version' +import type { RuntimeRpcResponse } from './runtime-rpc-envelope' +import type { RemoteRuntimeSocketLivenessOptions } from './remote-runtime-socket-liveness' +import type { + RemoteRuntimeOutboundMemoryBudget, + RemoteRuntimeOutboundQueueOptions +} from './remote-runtime-subscription-outbound' + +export type RemoteRuntimeTransportSubscription = { + requestId: string + close: () => void + sendBinary: (bytes: Uint8Array) => boolean + sendRequest?: ( + method: string, + params: unknown, + timeoutMs: number + ) => Promise> +} + +export type RemoteRuntimeTransportSubscriptionCallbacks = { + onResponse: (response: RuntimeRpcResponse) => void + onBinary?: (bytes: Uint8Array) => void + onError: (error: RemoteRuntimeClientError) => void + onClose?: () => void +} + +export type RemoteRuntimeSubscriptionOptions = RemoteRuntimeSocketLivenessOptions & { + clientCapabilities?: readonly RuntimeCapability[] + perMessageDeflate?: boolean + outboundQueue?: RemoteRuntimeOutboundQueueOptions + outboundMemoryBudget?: RemoteRuntimeOutboundMemoryBudget + // Why: overridable so the connect-bound regression test can pin the ordering against the + // subscription-start timer without spending production wall-clock. + connectTimeoutMs?: number +} diff --git a/src/shared/remote-runtime-subscription-transport.ts b/src/shared/remote-runtime-subscription-transport.ts index 9376cbbb9a5..275fa6f5a8e 100644 --- a/src/shared/remote-runtime-subscription-transport.ts +++ b/src/shared/remote-runtime-subscription-transport.ts @@ -8,12 +8,15 @@ import { publicKeyFromBase64, publicKeyToBase64 } from './e2ee-crypto' -import type { RuntimeCapability } from './protocol-version' import { formatRemoteRuntimeCloseMessage, ignoreSettledRemoteRuntimeSocketError } from './remote-runtime-client-handshake' import { RemoteRuntimeClientError } from './remote-runtime-client-error' +import { + remoteRuntimeConnectFailureMessage, + remoteRuntimeConnectOptions +} from './remote-runtime-connect-bound' import { isRemoteRuntimeBinaryFrameWithinLimit, REMOTE_RUNTIME_MAX_WEBSOCKET_FRAME_BYTES, @@ -21,49 +24,29 @@ import { serializeRemoteRuntimeRpcRequest } from './remote-runtime-memory-limits' import { remoteRuntimeClientCapabilities } from './remote-runtime-client-capabilities' -import type { RuntimeRpcResponse } from './runtime-rpc-envelope' import { RemoteRuntimeSubscriptionFrameRouter } from './remote-runtime-subscription-frame-router' -import { - RemoteRuntimeSubscriptionOutbound, - type RemoteRuntimeOutboundMemoryBudget, - type RemoteRuntimeOutboundQueueOptions -} from './remote-runtime-subscription-outbound' +import { RemoteRuntimeSubscriptionOutbound } from './remote-runtime-subscription-outbound' import { RemoteRuntimeSubscriptionRequestChannel } from './remote-runtime-subscription-request-channel' import { startRemoteRuntimeSocketLiveness, - type RemoteRuntimeSocketLivenessMonitor, - type RemoteRuntimeSocketLivenessOptions + type RemoteRuntimeSocketLivenessMonitor } from './remote-runtime-socket-liveness' +import type { + RemoteRuntimeSubscriptionOptions, + RemoteRuntimeTransportSubscription, + RemoteRuntimeTransportSubscriptionCallbacks +} from './remote-runtime-subscription-contract' export type { RemoteRuntimeOutboundMemoryBudget, RemoteRuntimeOutboundSocketMemory } from './remote-runtime-subscription-outbound' -export type RemoteRuntimeTransportSubscription = { - requestId: string - close: () => void - sendBinary: (bytes: Uint8Array) => boolean - sendRequest?: ( - method: string, - params: unknown, - timeoutMs: number - ) => Promise> -} - -export type RemoteRuntimeTransportSubscriptionCallbacks = { - onResponse: (response: RuntimeRpcResponse) => void - onBinary?: (bytes: Uint8Array) => void - onError: (error: RemoteRuntimeClientError) => void - onClose?: () => void -} - -export type RemoteRuntimeSubscriptionOptions = RemoteRuntimeSocketLivenessOptions & { - clientCapabilities?: readonly RuntimeCapability[] - perMessageDeflate?: boolean - outboundQueue?: RemoteRuntimeOutboundQueueOptions - outboundMemoryBudget?: RemoteRuntimeOutboundMemoryBudget -} +export type { + RemoteRuntimeSubscriptionOptions, + RemoteRuntimeTransportSubscription, + RemoteRuntimeTransportSubscriptionCallbacks +} from './remote-runtime-subscription-contract' export async function subscribeRemoteRuntimeTransport( pairing: PairingOffer, @@ -225,11 +208,15 @@ export async function subscribeRemoteRuntimeTransport( callbacks.onClose?.() } - try { - ws = new WebSocket(pairing.endpoint, { + const connectOptions = remoteRuntimeConnectOptions( + { maxPayload: REMOTE_RUNTIME_MAX_WEBSOCKET_FRAME_BYTES, ...(options?.perMessageDeflate === false ? { perMessageDeflate: false } : {}) - }) + }, + options?.connectTimeoutMs + ) + try { + ws = new WebSocket(pairing.endpoint, connectOptions) } catch (error) { const message = error instanceof Error ? error.message : String(error) fail(new RemoteRuntimeClientError('invalid_argument', `Invalid remote endpoint: ${message}`)) @@ -242,11 +229,11 @@ export async function subscribeRemoteRuntimeTransport( ) } - function onError(): void { + function onError(error: Error): void { fail( new RemoteRuntimeClientError( 'remote_runtime_unavailable', - 'Could not connect to the remote Orca runtime.' + remoteRuntimeConnectFailureMessage(error, pairing.endpoint) ) ) } diff --git a/src/shared/remote-runtime-tailscale-hint.ts b/src/shared/remote-runtime-tailscale-hint.ts index c04d3da5e9b..6dcccbb9e12 100644 --- a/src/shared/remote-runtime-tailscale-hint.ts +++ b/src/shared/remote-runtime-tailscale-hint.ts @@ -57,6 +57,16 @@ export function isTailscaleEndpoint(endpoint: string | null | undefined): boolea ) } +/** + * Why: a server already reached over Tailscale fails for tailnet-specific reasons, so "use + * Tailscale" would be useless — point at the real causes. Already-paired devices keep their + * saved token across server restarts, so re-pairing only matters when adding a new device. + */ +const TAILNET_ENDPOINT_HINT = + "The server may be offline on your tailnet, or its Tailscale Funnel reverted to tailnet-only. Confirm it's reachable; re-pair only when adding a new device, since already-paired devices reconnect with their saved token." + +const OTHER_NETWORK_HINT = `If the server is on another network, connect both devices to Tailscale and pair using its Tailscale address (100.x or a *.ts.net name). See ${TAILSCALE_DOWNLOAD_URL}.` + export function withRemoteRuntimeTailscaleHint( message: string, endpoint: string | null | undefined @@ -64,17 +74,11 @@ export function withRemoteRuntimeTailscaleHint( if (!REMOTE_RUNTIME_UNREACHABLE_RE.test(message)) { return message } - // Why: keep the hint idempotent so a message routed through this helper twice - // (e.g. re-wrapped error response) isn't suffixed with duplicate guidance. - if (/tailscale/i.test(message)) { + // Why: keep the hint idempotent so a message routed through this helper twice (e.g. a + // re-wrapped error response) isn't suffixed with duplicate guidance. Keyed on the hints + // themselves, not on the word — messages now carry an endpoint whose host can contain it. + if (message.endsWith(TAILNET_ENDPOINT_HINT) || message.endsWith(OTHER_NETWORK_HINT)) { return message } - if (isTailscaleEndpoint(endpoint)) { - // Why: a server already reached over Tailscale fails for tailnet-specific - // reasons, so "use Tailscale" would be useless — point at the real causes. - // Already-paired devices keep their saved token across server restarts, so - // re-pairing only matters when adding a new device. - return `${message} The server may be offline on your tailnet, or its Tailscale Funnel reverted to tailnet-only. Confirm it's reachable; re-pair only when adding a new device, since already-paired devices reconnect with their saved token.` - } - return `${message} If the server is on another network, connect both devices to Tailscale and pair using its Tailscale address (100.x or a *.ts.net name). See ${TAILSCALE_DOWNLOAD_URL}.` + return `${message} ${isTailscaleEndpoint(endpoint) ? TAILNET_ENDPOINT_HINT : OTHER_NETWORK_HINT}` } diff --git a/src/shared/remote-runtime-transport-error-agreement.test.ts b/src/shared/remote-runtime-transport-error-agreement.test.ts index 479666a917e..5dd3f074185 100644 --- a/src/shared/remote-runtime-transport-error-agreement.test.ts +++ b/src/shared/remote-runtime-transport-error-agreement.test.ts @@ -21,6 +21,7 @@ */ import { Buffer } from 'node:buffer' import { describe, expect, it } from 'vitest' +import { RemoteRuntimeClientError } from './remote-runtime-client-error' import { RECOVERABLE_CODES, RECOVERABLE_MESSAGE_FRAGMENTS, @@ -29,6 +30,10 @@ import { toRemoteRuntimeClientErrorLike, type RemoteRuntimeClientErrorLike } from './remote-runtime-client-error-classification' +import { + WS_HANDSHAKE_TIMEOUT_MESSAGE, + remoteRuntimeConnectFailureMessage +} from './remote-runtime-connect-bound' import { invalidRemoteRuntimeResponseError, parseAuthenticatedFrame, @@ -59,6 +64,12 @@ function frameError(producer: string, frame: string): TransportErrorPair { const CLOSE_REASON = Buffer.from('server restarting') const EMPTY_CLOSE_REASON = Buffer.from('') +// Why: the connect bound's message is built from the real helper so a rewording updates the +// corpus with it, and the code-less entry below then fails instead of the user. +const CONNECT_BOUND_ENDPOINT = 'ws://desk.example.com:6768' +const connectBoundMessage = (endpoint = CONNECT_BOUND_ENDPOINT): string => + remoteRuntimeConnectFailureMessage(new Error(WS_HANDSHAKE_TIMEOUT_MESSAGE), endpoint) + const REQUEST_TRANSPORT_ERRORS: TransportErrorPair[] = [ { producer: 'remote-runtime-client.ts:120', @@ -80,6 +91,10 @@ const REQUEST_TRANSPORT_ERRORS: TransportErrorPair[] = [ code: 'remote_runtime_unavailable', message: 'Could not connect to the remote Orca runtime.' }, + producedPair( + 'remote-runtime-connect-bound.ts (elapsed handshakeTimeout; request-socket, request-websocket and subscription-transport onError)', + new RemoteRuntimeClientError('remote_runtime_unavailable', connectBoundMessage()) + ), { producer: 'remote-runtime-client.ts:270 / :667 (formatRemoteRuntimeCloseMessage, 1006)', code: 'remote_runtime_unavailable', @@ -340,6 +355,11 @@ const TAILSCALE_HINTED_TRANSPORT_ERRORS: TransportErrorPair[] = [ 'Remote Orca runtime closed the connection.', 'https://desk.tail1234.ts.net' ) + }, + { + producer: 'main/ipc/runtime-environment-transport-routing.ts:153 (connect bound elapsed)', + code: 'remote_runtime_unavailable', + message: withRemoteRuntimeTailscaleHint(connectBoundMessage(), 'https://desk.example.com') } ] @@ -392,6 +412,15 @@ const CODELESS_TRANSPORT_ERRORS: (TransportErrorPair & { recoverable: boolean }) "Error invoking remote method 'runtimeEnvironments:call': RuntimeRpcCallQueueOverloadError: Remote runtime call queue is full; retry after current calls finish.", recoverable: true }, + { + // Why: the subscribe handler rethrows, and Electron keeps only the message. This is the + // exact pair that dead-ended a terminal pane when the connect bound's wording drifted + // outside RECOVERABLE_MESSAGE_FRAGMENTS. + producer: + "ipcMain.handle('runtimeEnvironments:subscribe') rejection after the connect bound elapsed (code stripped)", + message: `Error invoking remote method 'runtimeEnvironments:subscribe': Error: ${withRemoteRuntimeTailscaleHint(connectBoundMessage(), CONNECT_BOUND_ENDPOINT)}`, + recoverable: true + }, { producer: 'untyped host rejection with no connection wording', message: 'Worktree is missing on the remote host.', @@ -412,7 +441,7 @@ describe('transport error code/message classification agreement', () => { it('enumerates every reachable coded producer', () => { // Floor, not an exact count: the corpus should only grow. Lower it deliberately when a // producer is genuinely deleted. (#12667's review enumerated 34 of these by hand.) - expect(CODED_TRANSPORT_ERRORS.length).toBeGreaterThanOrEqual(57) + expect(CODED_TRANSPORT_ERRORS.length).toBeGreaterThanOrEqual(59) expect(CODED_TRANSPORT_ERRORS.every((pair) => typeof pair.code === 'string')).toBe(true) })