Files
orca/src/shared/remote-runtime-connect-bound.ts
T
Neil 2531dc9d5a fix(runtime): bound the connect phase against an unreachable host, at the transport (#20053)
* fix(runtime): bound the remote-runtime connect against an unreachable host

A host that is powered off or firewalled black-holes the TCP SYN, so the
remote-runtime WebSocket neither opens nor errors. The Node-side transports
set no connect bound, leaving the caller's whole-request timeout as the only
one: every `orca <cmd> --environment <unreachable>` sat silent for 60s before
failing with a generic `runtime_timeout`.

Measured on an unreachable paired host (win-lowspec, SYNs dropped): terminal
list / worktree list / repo list / status each took 60.19-60.26s; the same
command against a reachable host answered in 0.24s. So this was the shared
transport, not one command.

Pass `handshakeTimeout` at the three shared remote-runtime WebSocket
construction sites, which `ws` applies across TCP connect and the HTTP
upgrade. The value matches the bound the browser transport already used.

The failure keeps code `remote_runtime_unavailable` so the existing
transport-loss classification in terminal-process-inspection still applies,
and the message names the endpoint and stops at "unverifiable" — per
docs/reference/ssh-execution-boundary.md, loss of contact is never evidence
that the host's work stopped.

* fix(relay): bound the control socket's connect phase at the transport

The relay control socket was constructed with no `handshakeTimeout`, the same
gap fixed for the remote-runtime transports. It was not a live defect: the
class-level `connectDeadlineMs` (15s) also covers a stalled connect, and that
deadline does fire — its `unref()` is safe because the pending TCP connect is
itself a ref'd libuv handle that holds the event loop open. Measured in a bare
Node process: unref'd timer with an empty loop never fires (exit at 0ms), but
the same timer alongside a black-holed connect fired at 2003ms.

It was a defect waiting on a refactor. The two bounds cover different phases,
and the class deadline covers the connect phase only incidentally.

DO NOT REMOVE EITHER BOUND AS REDUNDANT. They are not. Proven by mutation:

- Remove the transport bound -> a stalled *connect* falls through to the class
  deadline, rejecting with `relay_control_connect_timeout` after the full
  deadline instead of the transport error.
- Remove the class deadline -> a stall during the *proving* phase (socket open,
  host proof never answered) is unbounded; the incumbent test hangs 30s.
  `handshakeTimeout` cannot see that phase at all.

Reuses `remoteRuntimeConnectOptions` rather than forking a second helper, and
moves the construction into `relay-control-socket-factory.ts` so a caller that
needs a relay control socket gets the bound instead of re-deriving an unbounded
one. `handshakeTimeoutMs` is settable apart from `connectDeadlineMs` so a test
can stall the connect alone and assert which bound produced the rejection —
error identity, not elapsed time.

The connect-bound ratchet now covers the relay site and asserts the site still
resolves, so an allowlist that silently stopped matching cannot pass vacuously.

* fix(lint): carry SAFETY rationales for the connect-bound casts

main tightened typescript/consistent-type-assertions to assertionStyle:
never, which the rebase brings onto these added lines. Dropping the
generic default is not typeable, so each cast keeps its own rationale.

* fix(runtime): keep the bounded connect failure inside both message gates

The connect bound's new wording dropped out of the two gates that classify
remote-transport failures by message text, and those gates are the only ones
that run on the path the bound made reachable.

`subscribeRemoteRuntimeTransport` reports a connect failure by *rejecting* the
subscribe promise, and that rejection crosses `ipcMain.handle`, which keeps only
the message. The renderer then classifies it with `RECOVERABLE_MESSAGE_FRAGMENTS`.
`Could not reach the remote Orca runtime at …` matched no fragment, so it read as
fatal: `recovery.cancel()` and a red banner instead of a retry. Before the bound
existed this case reached the 15s subscription-start timer, whose message did
match a fragment, so introducing a 12s bound turned an auto-recovering pane into
a dead-ended one — the #12650 shape.

The same wording also fell outside `REMOTE_RUNTIME_UNREACHABLE_RE`, so the
Tailscale remedy was dropped for precisely the unreachable-host failure it
exists for.

Keep the canonical phrase both gates already recognise rather than teaching each
gate a second synonym for one condition, and pin it: the phrase is now a named
constant, the corpus in `remote-runtime-transport-error-agreement.test.ts` grows
the coded, hinted and code-stripped producers derived from the real helper, and
a new subscribe-path test proves the connect bound (not the start timer) is what
fires and that its message still classifies as recoverable once the code is
gone. Verdict wording is unchanged: `unverifiable`, never a synonym for exited.

Also states the bound in seconds, corrects the module comment (`handshakeTimeout`
is a socket inactivity timer, so a slow-but-answering host is not cut off), and
splits the subscription contract types out to stay under `max-lines`.

* fix(relay): drop the duplicate connect bound on the control socket

The claim that `connectDeadlineMs` cannot see a black-holed connect is false.
`RelayControlClient.connect()` constructs the socket and arms `connectTimer` in
the same synchronous call — `new WebSocket()` never blocks — and `expireConnect`
fires from `opening` as well as `proving`. The class deadline was already a
strict superset of a transport `handshakeTimeout` on that socket.

It was also inert. Production passes neither option, so the transport bound was
derived from `connectDeadlineMs` and both timers were 15_000, armed in the same
tick; the ws timer is an inactivity timer armed on the later `socket` event, so
it could not win. Its only reachable effect was changing which string a stalled
relay connect rejects with, and it narrowed an existing test's 20ms deadline
into a handshake bound it could race.

So this removes the factory, the test-only `handshakeTimeoutMs` option and the
source-grep test whose premise was wrong, and replaces them with a test that
holds the real ground: a connect whose upgrade is never answered expires on the
class deadline. Moving the timer arm after `open`, or narrowing `expireConnect`
to `proving`, both turn it red — which is what a future reader needs before
concluding the phase is uncovered and adding a second bound again.

No behaviour change for a reachable relay, and none for the verdict: a stalled
connect still rejects and still reaches `unverifiable`, never `exited`.

* fix(runtime): stop the endpoint in the failure message from undoing the fix

Putting the endpoint into the message created three problems the message itself
caused.

The Tailscale hint is idempotent by testing whether "tailscale" already appears
anywhere in the message. That held while the message was fixed copy. Now a host
called `tailscale-box` puts the word there itself, and the hint — the only
actionable remedy on an unreachable host — is suppressed for it. Key the guard
on the two hints instead of the word.

The endpoint comes from a pasted pairing code, which is only length-capped;
`normalizePairingUrl` rejects userinfo but nothing re-validates a stored offer.
Render scheme, host and port only, so a pasted `wss://user:secret@host` cannot
reach a surface the user reads.

And drop the elapsed time from the wording. `handshakeTimeout` is a socket
inactivity timer, so a `wss://` host that completes TCP and then goes silent
re-arms it once and fails at about twice the bound; measured at 2008ms against a
1000ms bound. "within 12s" would have been wrong there, and the endpoint is the
actionable part regardless.

Also refuse a non-positive or non-finite bound: `ws` and `net` both gate on a
truthy timeout, so `0` left the connect completely unbounded while still
satisfying the connect-bound ratchet.

* fix(runtime): keep the endpoint from smuggling a verdict into the message

`isRemoteTerminalGoneMessage` in the pty transport substring-matches
`terminal_gone` / `terminal_exited` / `no_connected_pty`, and it runs before the
recoverable-connection gate: a match retires the pane's terminal id and cancels
recovery. WHATWG URL accepts `_` in a special-scheme host, so once the failure
message carried the endpoint, `ws://terminal_gone.example:6768` turned loss of
contact into a terminal-gone verdict — the one conclusion
`docs/reference/ssh-execution-boundary.md` forbids.

Render the host only when it matches a hostname or IP-literal grammar that
cannot carry such a token, and fall back to naming no endpoint at all. A
well-formed host, including a bracketed IPv6 literal, is still shown.

* docs(runtime): say why this connect bound is not the relay's removed duplicate
2026-09-16 22:23:03 -07:00

104 lines
4.9 KiB
TypeScript

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<TOptions extends ClientOptions>(
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.'
)
}