From 9a39b1345a426042f075dcfec82b8708f9b2c282 Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Mon, 11 May 2026 17:28:59 -0400 Subject: [PATCH] fix(ssh): enable TCP_NODELAY on ssh2 client to eliminate per-keystroke typing lag (#1660) (#1679) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(ssh): enable TCP_NODELAY on ssh2 client to eliminate per-keystroke typing lag (#1660) ssh2 leaves Nagle's algorithm on by default. For single-byte keystrokes through a remote PTY, Nagle interacts with the kernel's delayed-ACK timer and adds up to ~40 ms per keystroke — visible as the typing lag reported in #1660. OpenSSH's `ssh` client sets TCP_NODELAY whenever a PTY is allocated; this change mirrors that on the ssh2 client right after the `ready` event in doSsh2Connect, covering both initial connect and auto-reconnect. Proxy-command / proxy-jump connections (where ssh2's underlying socket is a custom Duplex over a child-process pipe) are a no-op by design, gated by the public Client.setNoDelay()'s own type guard. A discriminating log line records which path each connect took. Tests cover initial connect and a full reconnect cycle to guard against the regression class "Nagle is re-enabled because someone refactored only the initial connect path." Co-authored-by: Orca * fix(ssh): bound relay-lost reconnect with exponential backoff When the relay exec channel keeps dying (e.g. a remote-side bug closes every fresh --connect channel right after handshake, or a stale bridge keeps being replaced), the unguarded _onRelayLost handler reconnects as fast as the network allows — spawning relay deploy attempts in a tight loop until the user force-quits. Each iteration spawns a fresh ssh2 exec channel, hammers sshd's MaxSessions counter, and floods the renderer with state churn. Add per-target exponential backoff (500ms → 15s, capped at 6 attempts) so the loop terminates instead of running forever. After the cap the session goes to 'error' state with a 'Relay channel kept dropping. Please reconnect.' message — visible in the renderer instead of an invisible failure where typing in remote terminals just stops working. Successful 'ready' resets the attempt counter only if the session stabilized for >= 5s; faster flaps preserve the counter so a flaky remote backs off rather than retrying indefinitely on every brief ready→lost cycle. Backoff state is cleared on explicit disconnect, on session replacement during reconnect, and on connect failures, so a real reconnect attempt after backoff exhaustion always starts from zero. Co-authored-by: Orca * fix(ssh): detect stale relay daemons via running-version marker The on-disk relay version check compares local .version against the remote .version file in the relay dir. A daemon launched by an earlier deploy keeps running its in-memory copy of the OLD relay code, so when the client later rewrites relay.js + .version on disk and bridges in via --connect, the new bridge process drives a stale daemon. Protocol or behavior changes between the two versions then tear down the channel in a tight reconnect loop (observed against PR #1672 on a daemon predating that change). The daemon now writes its running version into a .running-version sidecar at startup, anchored to the relay-script directory rather than process.cwd() so test spawns cannot pollute the repo root. Before attaching to an existing socket, the client probes that marker and, on mismatch with the locally-deployed .version, kills the stale daemon (TERM only, never KILL) and falls through to a fresh launch. Conservative defaults: when either marker is unreadable, attach so older builds keep their live PTYs. Co-authored-by: Orca * Revert "fix(ssh): detect stale relay daemons via running-version marker" This reverts commit e58acf07c04db5178bd9db62ff02b38aaee86db0. * fix(ssh): isolate relay versions via per-version install dirs and wire handshake The relay's previous single-dir layout (~/.orca-remote/relay-v0.1.0/) let the deploy step rewrite relay.js in place while a daemon was still loaded in memory at the previous version. New clients then drove that stale daemon, surfacing as a reconnect loop (issue #1660 follow-up) and the field failure observed against an 8-day-old daemon on openclaw. Switch to a VS Code-style versioned layout where each (RELAY_VERSION + content-hash) bundle installs into its own directory and is never mutated after install. A v2 client's --connect socket path is rooted in relay-${v2-hash}/ and structurally cannot reach a v1 daemon's socket. Defense-in-depth: the daemon now reads exactly one Handshake-typed frame on each newly-accepted Unix socket before attaching the JSON-RPC dispatcher (mirrors VS Code's remoteExtensionHostAgentServer.ts:340). Mismatch closes the socket; the bridge exits with code 42; client maps that to a typed RelayVersionMismatchError and skips the relay-lost backoff loop instead of retrying through 6 attempts. Other deploy hardening: - atomic mkdir-based install lock with stale-lock recovery serialises concurrent first-installs of the same version - .install-complete sentinel distinguishes a finished install from a crashed-mid-install partial that should be retried - gcOldRelayVersions removes unreferenced sibling dirs (allowlist regex, skips locked or incomplete dirs, skips dirs with a live socket) - readLocalFullVersion fails fast on a missing/empty local .version rather than silently falling back to a path where a daemon from a different code generation may already be running Includes a cross-version isolation test that fails any future refactor which collapses the per-version layout back to a shared dir. Co-authored-by: Orca * fix(ssh): harden relay versioning per review feedback Address must-fix and should-fix findings from the parallel triple review of 26d1666e: - Surface RelayVersionMismatchError to ssh.ts on initial establish() (not just reconnect), so the user sees the typed terminal error instead of silent retry on first connect (#13). - Give the sentinel timeout a 500ms grace window for the close handler to deliver exit-42, so a slow remote does not misclassify a wire-handshake mismatch as a generic timeout (#D11). - Drain the handshake decoder's residue at the handshake -> dispatcher transition on both daemon and --connect sides; pipelined frames that were coalesced with the handshake are now forwarded into the dispatcher / stdout instead of silently dropped (#A1, #A2). - Reset the install-lock acquire timer after a stale-lock recovery so a single post-recovery race does not immediately exhaust the budget (#E14). - Treat a stale install-lock as recoverable in the GC pass when .install-complete is present (covers an interrupted finalize where the rm-lock failed) (#E15). - GC legacy relay-v\d+\.\d+\.\d+ install dirs whose daemons have died, now that .install-complete is no longer required for them (#12). - Resolve symlinks in readLaunchVersion() so a daemon launched via a symlinked entry script still reads .version next to the real file (#G21). - Flush stderr before exit-42 in --connect handshake mismatch path so the diagnostic line reaches the client before the process tears down (#C8). Tests: - Round-trip handshake over a real Socket pair: matching version, mismatch exit-42, leftover bytes preserved on both sides when frames are coalesced with the handshake. - waitForSentinel exit-42 -> RelayVersionMismatchError, exit-1 -> generic. - SshRelaySession terminal-error callback fires on both establish() and reconnect() when deployAndLaunchRelay throws RelayVersionMismatchError. - acquireInstallLock concurrent BUSY -> OK polling, stale-lock recovery with reset timeout window, and fresh-lock timeout failure path. - gcOldRelayVersions stale-lock-with-complete branch, legacy-dead path, legacy-alive path; existing locked-test asserts fresh-lock now keeps. - Cross-version isolation test now asserts a blanket invariant that every v1-referencing command from a v2 deploy is a read-only liveness probe. Lint and typecheck clean across all 3 tsconfigs; 426 SSH/relay tests pass. Co-authored-by: Orca * fix(ssh): bypass npm init for content-hashed relay dirs and harden install probe The versioned-install dirs land at `relay-${version}+${hash}/` (e.g. `relay-0.1.0+07994a7870e1`). npm 11 / Node 26 reject the `+` in derived package names and `npm init -y` exits 1 — silently, since both stderr and the failure landed inside the `2>/dev/null && ...` chain. The catch swallowed the throw, `.install-complete` was written anyway, and every reconnect surfaced 'node-pty is not available' at first pty.spawn. Sidestep `npm init` entirely: SFTP-write a hardcoded minimal package.json (`name: orca-relay`, `type: commonjs`) and run `npm install node-pty` directly. `type: commonjs` pins the module system against future Node default flips or remote-side .npmrc overrides. Also harden the install path against the same class of silent failure: - npm install errors now propagate (no more `.install-complete` on hard fail; future reconnects retry instead of stranding the user) - Replace the weak `test -d node-pty` post-install probe with `node -e 'require("node-pty")'` so built-but-unloadable installs (missing prebuild, wrong arch, broken native binding) surface clearly - Add a session-level error handler on the SFTP write so a torn-down session rejects the promise instead of hanging until enclosing timeout Separate fix: add `for-each-ref` to the relay's git subcommand allowlist. Client code (`src/main/git/repo.ts` ref-search and worktree-listing) calls `git for-each-ref` over SSH; the relay was rejecting it. The `--shell`/`--python`/`--perl`/`--tcl` format flags only control output quoting (no eval) and the relay invokes git via execFileAsync (no shell), so the read-only allowlist treatment matches `rev-parse`, `log`, etc. Co-authored-by: Orca * comment(ssh-relay): TODO link to #1693 for VS Code-style pre-bundled node-pty Co-authored-by: Orca * fix(ssh): harden node-pty install probe and tighten review-fix tests Round-3 review fixes on top of 963f56d7. deploy.ts: - Replace endsWith('OK') with includes('ORCA-NPTY-PROBE-OK'). Node can emit deprecation/experimental warnings to stderr after our stdout 'OK' write, and 2>&1 would push them past 'OK' producing false NPTY-MISSING warnings. A unique sentinel survives any trailing stderr noise. - Switch sftpPkg/ws .on -> .once for error/close. A late session 'error' after the promise had already settled would otherwise become an unhandled EventEmitter error and crash main. - Trim per-block comments to 1-2 lines per AGENTS.md (was 7-9). Tests: - Pin the BEFORE-ordering contract: SftpWriteCapture now records the count of execCommand calls observed at the moment ws.end() ran for each path, and the test asserts that count <= the index of npm install. Catches a future Promise.all-style refactor that would still pass final-state checks. - Strengthen the SSH-channel-failure test: assert the rejection actually came from the probe call (not an earlier exec) by finding the probe invocation in mock.calls. Also assert NPTY-INSTALL-FAIL is NOT logged (channel failure must not be conflated with install failure) and that abandonInstall was called so the lock is released. - Fix misleading clearAllMocks comment: it claims to wipe mockReturnValue, but actually clearAllMocks only resets .mock.calls. Re-priming was defense-in-depth, not a correctness requirement. Validator: - Add for-each-ref negative cases (--git-dir, --output, --work-tree) to the global-denied-flags it.each. The first round of for-each-ref enablement trusted that the post-subcommand GLOBAL_DENIED_FLAGS check applied; this pins it so a future allowlist refactor that bypasses the global check fails loudly. Co-authored-by: Orca * fix(ssh): split node-pty probe into test-d guard + load-test Round-4 review fixes for the install probe in installNativeDeps: (1) test -d guard runs before the load-test. If the install dir vanished between npm install and probe (concurrent rm, fs unmount, permission flip), the deploy now throws and the next reconnect retries fresh — previously the cd failure flowed into '|| echo MISSING' and we'd write .install-complete, stranding the user in degraded mode. (2) Load-test discards stderr (2>/dev/null) so customized .bashrc output (NVM init, conda greetings, etc.) can't pollute the sentinel match. The shell-level '|| echo MISSING' is preserved so SSH-channel rejections still propagate as exec errors, distinct from require failures which exit the node process nonzero. (3) PROBE_OK is passed via process.argv[1] so the JS literal stays trivial regardless of future sentinel characters. Test changes: - New 'dir-gone' probe mode in makeExecResponses - New test pinning that vanished-dir throws (not silent MISSING) - SSH-channel test now asserts probeCallIdx > npmInstallIdx - cross-version-isolation feeds an extra '' for the test -d slot Co-authored-by: Orca * fix(ssh): simplify node-pty probe and harden test ordering pins Round-5 review fixes for installNativeDeps: Production: - Drop redundant test -d guard. `cd ${dir} && (...)` short-circuits on cd-failure (dir-vanished) and propagates as exec reject already; the separate guard added a round trip without preventing anything. - Capture probe stderr to a per-deploy file rather than 2>&1 or 2>/dev/null. .bashrc noise can't pollute the sentinel match, but the require() error message is preserved in the [NPTY-MISSING] log breadcrumb so bug reports point at the real cause (e.g. GLIBC version mismatch). - Mirror the install command's PATH (export PATH=${binDir}:$PATH) so any future require-time child_process call resolves the same node binary used during install. - Add platform tuple to [NPTY-MISSING] and [NPTY-INSTALL-FAIL] logs for triageable bug reports without asking users to dig out their arch. - Trim probe comment per AGENTS.md (why-only, no mechanism narration). Tests: - Pin full installNativeDeps ordering: npm install < chmod prebuilds < probe. Catches refactors that probe before install or move chmod after. - Pressure-test .includes(PROBE_OK) survives bashrc/MOTD noise prefixed to probe stdout (corporate banner / NVM init / conda greeting case). - Pressure-test MISSING detection survives Node deprecation warnings prepended to the MISSING token. - Pin platform tuple appears in [NPTY-MISSING] log. - Pin finalizeInstall called exactly once + abandonInstall not called on happy paths; reverse on failure paths. - Strengthen dir-gone test: assert probeIdx > npmInstallIdx so a refactor that swaps order doesn't silently let the test pass on its own injected error string. - New probeStdoutOverride option in makeExecResponses for shell-noise injection tests. Cross-version-isolation: dropped obsolete test -d slot, added rm-stderr cleanup slot to match the new probe shape. eslint-disable max-lines on both files with rationale (pattern used widely in this repo for cohesive single-responsibility modules). 441/441 tests pass; lint clean; typecheck clean. Probe shape verified end-to-end on real remote. Co-authored-by: Orca --------- Co-authored-by: Orca --- src/main/ipc/ssh.ts | 135 ++++- src/main/ssh/ssh-connection.test.ts | 57 ++- src/main/ssh/ssh-connection.ts | 17 + .../ssh-relay-cross-version-isolation.test.ts | 166 +++++++ src/main/ssh/ssh-relay-deploy-helpers.test.ts | 49 ++ src/main/ssh/ssh-relay-deploy-helpers.ts | 93 +++- src/main/ssh/ssh-relay-deploy.test.ts | 50 +- src/main/ssh/ssh-relay-deploy.ts | 237 +++++---- .../ssh/ssh-relay-native-deps-install.test.ts | 462 ++++++++++++++++++ .../ssh-relay-session-terminal-error.test.ts | 163 ++++++ src/main/ssh/ssh-relay-session.ts | 39 ++ .../ssh/ssh-relay-version-mismatch-error.ts | 34 ++ .../ssh/ssh-relay-versioned-install.test.ts | 324 ++++++++++++ src/main/ssh/ssh-relay-versioned-install.ts | 340 +++++++++++++ src/relay/git-exec-validator.test.ts | 19 +- src/relay/git-exec-validator.ts | 1 + src/relay/protocol-handshake.test.ts | 68 +++ src/relay/protocol.ts | 38 ++ src/relay/relay-handshake-roundtrip.test.ts | 227 +++++++++ src/relay/relay-handshake.ts | 259 ++++++++++ src/relay/relay.ts | 140 +++--- 21 files changed, 2741 insertions(+), 177 deletions(-) create mode 100644 src/main/ssh/ssh-relay-cross-version-isolation.test.ts create mode 100644 src/main/ssh/ssh-relay-native-deps-install.test.ts create mode 100644 src/main/ssh/ssh-relay-session-terminal-error.test.ts create mode 100644 src/main/ssh/ssh-relay-version-mismatch-error.ts create mode 100644 src/main/ssh/ssh-relay-versioned-install.test.ts create mode 100644 src/main/ssh/ssh-relay-versioned-install.ts create mode 100644 src/relay/protocol-handshake.test.ts create mode 100644 src/relay/relay-handshake-roundtrip.test.ts create mode 100644 src/relay/relay-handshake.ts diff --git a/src/main/ipc/ssh.ts b/src/main/ipc/ssh.ts index edb556697..fd820e241 100644 --- a/src/main/ipc/ssh.ts +++ b/src/main/ipc/ssh.ts @@ -41,6 +41,39 @@ const connectInFlight = new Map>() // avoids that visual glitch. const testingTargets = new Set() +// Why: when a relay channel keeps dying (e.g. a stale --connect bridge keeps +// being replaced, a remote-side bug closes the channel right after handshake, +// or a mismatched relay binary refuses every handshake), the unguarded +// _onRelayLost handler reconnects as fast as the network allows, hammering +// both the local main process and the remote sshd in a tight loop. Track +// per-target reconnect attempts and apply exponential backoff so the loop +// terminates with a recoverable error instead of running forever. Successful +// `ready` resets the attempt counter for the next genuine drop. +type RelayLostBackoffState = { + attempts: number + lastAttemptStartedAt: number + pendingTimer: ReturnType | null +} +const relayLostBackoff = new Map() +const RELAY_LOST_MAX_ATTEMPTS = 6 +const RELAY_LOST_BASE_DELAY_MS = 500 +const RELAY_LOST_MAX_DELAY_MS = 15_000 +// Why: if a fresh reconnect's mux dies within this window, the new session +// never stabilized (a flap, not a real recovery). Without this clamp the +// attempt counter would be reset prematurely by a mux that "reached ready" +// only on paper. 5 seconds covers normal post-deploy provider registration +// and PTY reattach without being so generous that a real long-lived session +// looks like a flap. +const RELAY_LOST_STABILIZED_MS = 5_000 + +function clearRelayLostBackoff(targetId: string): void { + const state = relayLostBackoff.get(targetId) + if (state?.pendingTimer) { + clearTimeout(state.pendingTimer) + } + relayLostBackoff.delete(targetId) +} + function broadcastPortForwards(getMainWindow: () => BrowserWindow | null, targetId: string): void { const win = getMainWindow() if (!win || win.isDestroyed()) { @@ -291,6 +324,7 @@ export function registerSshHandlers( await portForwardManager!.removeAllForwards(targetId) existingSession.dispose() activeSessions.delete(targetId) + clearRelayLostBackoff(targetId) } // Why: create the session early so onStateChange sees it in 'deploying' @@ -322,6 +356,7 @@ export function registerSshHandlers( // needs a passphrase. credentialRequestedForTarget.delete(targetId) activeSessions.delete(targetId) + clearRelayLostBackoff(targetId) const win = getMainWindow() if (win && !win.isDestroyed()) { win.webContents.send('ssh:state-changed', { @@ -353,22 +388,112 @@ export function registerSshHandlers( // triggers session.reconnect() using the live SSH connection. // Set before establish() so the callback is in place if the relay // dies during the deploy/connect sequence. + // Why: a wire-handshake mismatch (typed RelayVersionMismatchError) means + // the local client and remote daemon are at different code versions — + // no amount of backoff will reconcile them. Skip the relay-lost loop + // entirely and surface a terminal "please reconnect manually" error. + session.setOnTerminalRelayError((tid, err) => { + clearRelayLostBackoff(tid) + console.warn( + `[ssh] Terminal relay error for ${tid}: ${err.message}; skipping reconnect backoff.` + ) + const win = getMainWindow() + if (win && !win.isDestroyed()) { + win.webContents.send('ssh:state-changed', { + targetId: tid, + state: { + targetId: tid, + status: 'error', + error: err.message, + reconnectAttempt: 0 + } + }) + } + }) + session.setOnRelayLost((tid) => { const s = activeSessions.get(tid) if (!s) { return } const c = connectionManager?.getConnection(tid) - const t = sshStore?.getTarget(tid) - if (c) { - void s.reconnect(c, t?.relayGracePeriodSeconds) + if (!c) { + return } + const t = sshStore?.getTarget(tid) + + // Why: bounded exponential backoff. Without this, a remote-side bug + // that closes every fresh --connect channel turns into an infinite + // tight loop spawning relay deploys until the user force-quits. + const state = relayLostBackoff.get(tid) ?? { + attempts: 0, + lastAttemptStartedAt: 0, + pendingTimer: null + } + if (state.pendingTimer) { + // A retry is already scheduled — coalesce this burst. + return + } + if (state.attempts >= RELAY_LOST_MAX_ATTEMPTS) { + console.warn( + `[ssh] Relay channel for ${tid} kept dying across ${state.attempts} attempts; giving up. User must reconnect manually.` + ) + relayLostBackoff.delete(tid) + // Why: surface the failure so the renderer can prompt the user. + // A still-live SSH connection with a dead relay is otherwise an + // invisible failure — typing in remote terminals just stops working. + const win = getMainWindow() + if (win && !win.isDestroyed()) { + win.webContents.send('ssh:state-changed', { + targetId: tid, + state: { + targetId: tid, + status: 'error', + error: 'Relay channel kept dropping. Please reconnect.', + reconnectAttempt: 0 + } + }) + } + return + } + const delay = Math.min( + RELAY_LOST_BASE_DELAY_MS * 2 ** state.attempts, + RELAY_LOST_MAX_DELAY_MS + ) + state.attempts += 1 + state.pendingTimer = setTimeout(() => { + state.pendingTimer = null + state.lastAttemptStartedAt = Date.now() + relayLostBackoff.set(tid, state) + const liveConn = connectionManager?.getConnection(tid) + if (!liveConn || !activeSessions.has(tid)) { + return + } + void s.reconnect(liveConn, t?.relayGracePeriodSeconds) + }, delay) + relayLostBackoff.set(tid, state) + console.warn( + `[ssh] Relay channel for ${tid} lost; reconnect attempt ${state.attempts}/${RELAY_LOST_MAX_ATTEMPTS} in ${delay}ms` + ) }) // Why: fires after both establish() and reconnect() reach 'ready'. // Re-creates persisted port forwards so they survive app restarts - // and network blips without manual re-configuration. + // and network blips without manual re-configuration. We also clear + // the relay-lost backoff state so a subsequent genuine drop starts + // from a fresh attempt counter — but only if the session had a chance + // to stabilize, otherwise rapid `ready → lost → ready → lost` flaps + // would silently keep retrying forever. session.setOnReady((tid) => { + const state = relayLostBackoff.get(tid) + if (state) { + const stabilized = + state.lastAttemptStartedAt === 0 || + Date.now() - state.lastAttemptStartedAt >= RELAY_LOST_STABILIZED_MS + if (stabilized) { + relayLostBackoff.delete(tid) + } + } void restorePortForwards(tid, getMainWindow) }) @@ -393,6 +518,7 @@ export function registerSshHandlers( } catch (err) { // Relay deployment failed — disconnect SSH activeSessions.delete(targetId) + clearRelayLostBackoff(targetId) await connectionManager!.disconnect(targetId) throw err } @@ -417,6 +543,7 @@ export function registerSshHandlers( await portForwardManager!.removeAllForwards(args.targetId) session.dispose() activeSessions.delete(args.targetId) + clearRelayLostBackoff(args.targetId) } await connectionManager!.disconnect(args.targetId) }) diff --git a/src/main/ssh/ssh-connection.test.ts b/src/main/ssh/ssh-connection.test.ts index 552e6d66a..29d89f071 100644 --- a/src/main/ssh/ssh-connection.test.ts +++ b/src/main/ssh/ssh-connection.test.ts @@ -1,11 +1,26 @@ import { describe, expect, it, vi, beforeEach } from 'vitest' +import { Socket } from 'net' let eventHandlers: Map void> let connectBehavior: 'ready' | 'error' = 'ready' let connectErrorMessage = '' -vi.mock('ssh2', () => ({ - Client: class MockSshClient { +type MockSshClient = { + setNoDelay: ReturnType + _sock: Socket | undefined +} +let clientInstances: MockSshClient[] = [] + +vi.mock('ssh2', () => { + class MockSshClient { + setNoDelay = vi.fn() + // Why: production code reads `client._sock` and checks `instanceof net.Socket` + // to decide which log line to emit. A real Socket instance lets the test + // exercise the "enabled" branch instead of the "skipped (proxy socket)" branch. + _sock: Socket | undefined = new Socket() + constructor() { + clientInstances.push(this) + } on(event: string, handler: (...args: unknown[]) => void) { eventHandlers?.set(event, handler) } @@ -23,7 +38,8 @@ vi.mock('ssh2', () => ({ exec() {} sftp() {} } -})) + return { Client: MockSshClient } +}) vi.mock('./ssh-system-fallback', () => ({ spawnSystemSsh: vi.fn().mockReturnValue({ @@ -67,6 +83,7 @@ describe('SshConnection', () => { eventHandlers = new Map() connectBehavior = 'ready' connectErrorMessage = '' + clientInstances = [] }) it('transitions to connected on successful connect', async () => { @@ -82,6 +99,39 @@ describe('SshConnection', () => { ) }) + it('enables TCP_NODELAY on the ssh2 client after ready', async () => { + const conn = new SshConnection(createTarget(), createCallbacks()) + await conn.connect() + + expect(clientInstances).toHaveLength(1) + expect(clientInstances[0].setNoDelay).toHaveBeenCalledWith(true) + }) + + it('enables TCP_NODELAY on the new ssh2 client after a reconnect cycle', async () => { + // Why: guards the "Nagle is re-enabled because someone refactored only + // the initial connect path" regression class. attemptConnect bumps + // connectGeneration on every call, and both the initial connect and the + // explicit reconnect path go through doSsh2Connect → client.on('ready'). + // The new client must also receive setNoDelay(true). + const conn = new SshConnection(createTarget(), createCallbacks()) + await conn.connect() + expect(clientInstances).toHaveLength(1) + expect(clientInstances[0].setNoDelay).toHaveBeenCalledWith(true) + + // Simulate the reconnect path: a fresh attemptConnect run via the + // internal helper that scheduleReconnect uses. Easiest from the public + // API is to call connect() again — disposed/connected guard rejects, so + // we exercise the path via a private call. Use the bracket-access + // form to keep the test free of `any` casts. + const privateConn = conn as unknown as { + attemptConnect: () => Promise + } + await privateConn.attemptConnect() + + expect(clientInstances).toHaveLength(2) + expect(clientInstances[1].setNoDelay).toHaveBeenCalledWith(true) + }) + it('transitions through connecting → connected states', async () => { const states: string[] = [] const callbacks = createCallbacks({ @@ -162,6 +212,7 @@ describe('SshConnectionManager', () => { eventHandlers = new Map() connectBehavior = 'ready' connectErrorMessage = '' + clientInstances = [] }) it('connect creates and stores a connection', async () => { diff --git a/src/main/ssh/ssh-connection.ts b/src/main/ssh/ssh-connection.ts index fc19c622b..4e8e88a42 100644 --- a/src/main/ssh/ssh-connection.ts +++ b/src/main/ssh/ssh-connection.ts @@ -1,4 +1,5 @@ /* eslint-disable max-lines -- Why: SSH connection lifecycle, credential retries, reconnect policy, and transport fallback are intentionally co-located so state transitions stay auditable in one file. */ +import * as net from 'net' import { Client as SshClient } from 'ssh2' import type { ChildProcess } from 'child_process' import type { ClientChannel, ConnectConfig, SFTPWrapper } from 'ssh2' @@ -220,6 +221,22 @@ export class SshConnection { settled = true this.client = client this.proxyProcess = null + // Why: ssh2 leaves Nagle's algorithm on by default. For single-byte + // keystrokes through a remote PTY this stacks with the kernel's + // delayed-ACK timer and adds up to ~40 ms per keystroke. OpenSSH's + // `ssh` sets TCP_NODELAY whenever a PTY is allocated; we mirror that + // because every channel we open over this connection (PTY data, + // JSON-RPC requests, port-scan probes) is latency-sensitive. No-op + // for proxy-command / proxy-jump connections where _sock is a custom + // Duplex; that case relies on the proxy program's own TCP behavior, + // same as native ssh. + const sock = (client as unknown as { _sock?: { setNoDelay?: unknown } })._sock + if (sock instanceof net.Socket) { + console.warn(`[ssh] TCP_NODELAY enabled for ${this.target.label}`) + } else { + console.warn(`[ssh] TCP_NODELAY skipped for ${this.target.label} (proxy socket)`) + } + client.setNoDelay(true) this.setState('connected') this.setupDisconnectHandler(client) resolve() diff --git a/src/main/ssh/ssh-relay-cross-version-isolation.test.ts b/src/main/ssh/ssh-relay-cross-version-isolation.test.ts new file mode 100644 index 000000000..d86232677 --- /dev/null +++ b/src/main/ssh/ssh-relay-cross-version-isolation.test.ts @@ -0,0 +1,166 @@ +// Cross-version isolation guard. +// +// Why: this test is the executable form of the "Pattern Note" in +// docs/ssh-relay-versioned-install-dirs.md — it asserts that a v2 deploy +// targeting a remote where a v1 daemon is already running NEVER touches +// v1's install dir or socket. Without this test a future refactor that +// collapses to a shared dir passes every other unit test and re-introduces +// the original "stale daemon serves new client" bug. + +import { beforeEach, describe, expect, it, vi } from 'vitest' + +vi.mock('electron', () => ({ + app: { getAppPath: () => '/mock/app' } +})) + +vi.mock('fs', () => ({ + existsSync: vi.fn().mockReturnValue(true), + readFileSync: vi.fn().mockReturnValue('0.1.0+v2hash') +})) + +vi.mock('./relay-protocol', () => ({ + RELAY_VERSION: '0.1.0', + RELAY_REMOTE_DIR: '.orca-remote', + parseUnameToRelayPlatform: vi.fn().mockReturnValue('linux-x64'), + RELAY_SENTINEL: 'ORCA-RELAY v0.1.0 READY\n', + RELAY_SENTINEL_TIMEOUT_MS: 10_000 +})) + +vi.mock('./ssh-relay-deploy-helpers', () => ({ + uploadDirectory: vi.fn().mockResolvedValue(undefined), + waitForSentinel: vi.fn().mockResolvedValue({ + write: vi.fn(), + onData: vi.fn(), + onClose: vi.fn() + }), + execCommand: vi.fn(), + resolveRemoteNodePath: vi.fn().mockResolvedValue('/usr/bin/node') +})) + +vi.mock('./ssh-connection-utils', () => ({ + shellEscape: (s: string) => `'${s}'` +})) + +import { deployAndLaunchRelay } from './ssh-relay-deploy' +import { execCommand } from './ssh-relay-deploy-helpers' +import type { SshConnection } from './ssh-connection' + +function makeMockConnection(): SshConnection { + return { + exec: vi.fn().mockResolvedValue({ + on: vi.fn(), + stderr: { on: vi.fn() }, + stdin: {}, + stdout: { on: vi.fn() }, + close: vi.fn() + }), + sftp: vi.fn().mockResolvedValue({ + mkdir: vi.fn((_p: string, cb: (err: Error | null) => void) => cb(null)), + on: vi.fn(), + once: vi.fn(), + createWriteStream: vi.fn().mockReturnValue({ + on: vi.fn((_event: string, cb: () => void) => { + if (_event === 'close') { + setTimeout(cb, 0) + } + }), + once: vi.fn((_event: string, cb: () => void) => { + if (_event === 'close') { + setTimeout(cb, 0) + } + }), + end: vi.fn() + }), + end: vi.fn() + }) + } as unknown as SshConnection +} + +describe('cross-version isolation', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it('a v2 deploy never references the v1 install dir or v1 socket path', async () => { + const conn = makeMockConnection() + const mockExec = vi.mocked(execCommand) + + // Simulated remote where: + // v1 dir = ~/.orca-remote/relay-0.1.0+v1hash/ (live daemon, listening) + // v2 dir = ~/.orca-remote/relay-0.1.0+v2hash/ (does not yet exist) + // The v2 client has fullVersion='0.1.0+v2hash' (from the fs mock above). + // + // We feed enough exec results to walk through the deploy: platform, + // $HOME, isRelayAlreadyInstalled probe, lock acquire, upload (no exec), + // npm install, finalize, socket probe, socket poll, then GC scan. + const responses: string[] = [ + 'Linux x86_64', // uname -sm + '/home/u', // echo $HOME + 'MISSING', // isRelayAlreadyInstalled (v2 dir doesn't exist) + '', // mkdir -p remoteRelayDir (v2) + 'OK', // mkdir lock OK + 'MISSING', // re-probe after lock → still missing → proceed with install + '', // mkdir remoteDir (uploadRelay) + '', // chmod +x node + '', // npm install + '', // chmod prebuilds + 'ORCA-NPTY-PROBE-OK\n', // node -e require() load-test (post-install verify) + '', // rm -f probe-stderr (best-effort cleanup after probe resolved) + '', // touch .install-complete (finalizeInstall) + '', // rm -rf .install-lock + 'DEAD', // launch socket probe + 'READY', // socket poll + // GC scan begins here + 'relay-0.1.0+v1hash\nrelay-0.1.0+v2hash\n', // ls listing + 'OPEN', // v1 lock probe (siblings only — current dir is v2) + 'COMPLETE', // v1 .install-complete probe + 'ALIVE' // v1 socket probe → live → SKIP (don't GC v1) + ] + for (const r of responses) { + mockExec.mockResolvedValueOnce(r) + } + + await deployAndLaunchRelay(conn) + + const allCmds = [ + ...mockExec.mock.calls.map(([, c]) => c), + ...vi.mocked(conn.exec).mock.calls.map(([c]) => c as string) + ] + + // (a) the v2 deploy creates dirs/files under relay-0.1.0+v2hash + expect(allCmds.some((c) => c.includes('relay-0.1.0+v2hash'))).toBe(true) + + // (b) the v2 launch and connect socket paths are rooted in v2 dir, never v1 + const launchAndConnectCmds = vi + .mocked(conn.exec) + .mock.calls.map(([c]) => c as string) + .filter((c) => c.includes('--sock-path')) + expect(launchAndConnectCmds.length).toBeGreaterThan(0) + for (const cmd of launchAndConnectCmds) { + expect(cmd).toContain('relay-0.1.0+v2hash') + expect(cmd).not.toContain('relay-0.1.0+v1hash') + } + + // (c) GC observes v1 has a live socket and never issues an rm -rf for it + const v1RemoveCmds = allCmds.filter( + (c) => c.includes('rm -rf') && c.includes('relay-0.1.0+v1hash') + ) + expect(v1RemoveCmds).toHaveLength(0) + + // (d) blanket isolation: every command that mentions v1hash MUST be a + // GC liveness probe (`ls`, `test -d`, `test -f`, or `for f in .../*.sock`) + // — never a write, mkdir, chmod, touch, rm, node launch, or socket poll. + // This prevents a future refactor that accidentally writes to the v1 dir + // (e.g. shared install-complete, upload over symlink) from passing. + const v1Refs = allCmds.filter((c) => c.includes('relay-0.1.0+v1hash')) + for (const cmd of v1Refs) { + const isReadOnlyProbe = + /^\s*ls\b/.test(cmd) || + /\btest -d\b/.test(cmd) || + /\btest -f\b/.test(cmd) || + /\btest -S\b/.test(cmd) || + /\bfor f in .*\.sock\b/.test(cmd) + expect(isReadOnlyProbe, `unexpected v1 reference: ${cmd}`).toBe(true) + } + }) +}) diff --git a/src/main/ssh/ssh-relay-deploy-helpers.test.ts b/src/main/ssh/ssh-relay-deploy-helpers.test.ts index fd66ecb1f..3c30d2331 100644 --- a/src/main/ssh/ssh-relay-deploy-helpers.test.ts +++ b/src/main/ssh/ssh-relay-deploy-helpers.test.ts @@ -3,6 +3,10 @@ import { describe, expect, it, vi } from 'vitest' import type { ClientChannel } from 'ssh2' import { execCommand, waitForSentinel } from './ssh-relay-deploy-helpers' import { RELAY_SENTINEL } from './relay-protocol' +import { + RelayVersionMismatchError, + RELAY_EXIT_CODE_VERSION_MISMATCH +} from './ssh-relay-version-mismatch-error' function createMockChannel(): ClientChannel { return Object.assign(new EventEmitter(), { @@ -60,6 +64,51 @@ describe('waitForSentinel', () => { expect(() => channel.emit('error', new Error('remote host rebooted'))).not.toThrow() expect(onClose).toHaveBeenCalledTimes(1) }) + + it('translates a pre-sentinel exit-42 + close into RelayVersionMismatchError', async () => { + const channel = createMockChannel() + const transportPromise = waitForSentinel(channel) + + channel.stderr.emit( + 'data', + Buffer.from( + '[relay-connect] Handshake mismatch: expected=0.1.0+aaa, daemon=0.1.0+bbb; exiting 42\n' + ) + ) + channel.emit('exit', RELAY_EXIT_CODE_VERSION_MISMATCH) + channel.emit('close') + + await expect(transportPromise).rejects.toBeInstanceOf(RelayVersionMismatchError) + await transportPromise.catch((err: RelayVersionMismatchError) => { + expect(err.expected).toBe('0.1.0+aaa') + expect(err.got).toBe('0.1.0+bbb') + }) + }) + + it('translates a pre-sentinel exit-42 even when the version detail is missing', async () => { + const channel = createMockChannel() + const transportPromise = waitForSentinel(channel) + + channel.stderr.emit('data', Buffer.from('boom\n')) + channel.emit('exit', RELAY_EXIT_CODE_VERSION_MISMATCH) + channel.emit('close') + + await expect(transportPromise).rejects.toBeInstanceOf(RelayVersionMismatchError) + }) + + it('rejects with a generic error (not RelayVersionMismatchError) on a non-42 exit code', async () => { + const channel = createMockChannel() + const transportPromise = waitForSentinel(channel) + + channel.stderr.emit('data', Buffer.from('node: bad bytecode\n')) + channel.emit('exit', 1) + channel.emit('close') + + await expect(transportPromise).rejects.toThrow(/Relay process exited before ready/) + await transportPromise.catch((err: unknown) => { + expect(err).not.toBeInstanceOf(RelayVersionMismatchError) + }) + }) }) describe('execCommand', () => { diff --git a/src/main/ssh/ssh-relay-deploy-helpers.ts b/src/main/ssh/ssh-relay-deploy-helpers.ts index 747731171..4b3146a0c 100644 --- a/src/main/ssh/ssh-relay-deploy-helpers.ts +++ b/src/main/ssh/ssh-relay-deploy-helpers.ts @@ -2,6 +2,10 @@ import type { ClientChannel } from 'ssh2' import type { SshConnection } from './ssh-connection' import { RELAY_SENTINEL, RELAY_SENTINEL_TIMEOUT_MS } from './relay-protocol' import type { MultiplexerTransport } from './ssh-channel-multiplexer' +import { + RelayVersionMismatchError, + RELAY_EXIT_CODE_VERSION_MISMATCH +} from './ssh-relay-version-mismatch-error' export { uploadFile, uploadDirectory, mkdirSftp } from './sftp-upload' @@ -14,19 +18,53 @@ export function waitForSentinel(channel: ClientChannel): Promise | null = null const timeout = setTimeout(() => { - if (!settled) { - settled = true - channel.close() - reject( - new Error( - `Relay failed to start within ${RELAY_SENTINEL_TIMEOUT_MS / 1000}s.${stderrOutput ? ` stderr: ${stderrOutput.trim()}` : ''}` + timeoutFired = true + channel.close() + timeoutGraceTimer = setTimeout(() => { + if (!settled) { + settled = true + reject( + new Error( + `Relay failed to start within ${RELAY_SENTINEL_TIMEOUT_MS / 1000}s.${stderrOutput ? ` stderr: ${stderrOutput.trim()}` : ''}` + ) ) - ) - } + } + }, TIMEOUT_GRACE_MS) }, RELAY_SENTINEL_TIMEOUT_MS) + const cancelTimers = (): void => { + clearTimeout(timeout) + if (timeoutGraceTimer) { + clearTimeout(timeoutGraceTimer) + timeoutGraceTimer = null + } + } + + channel.on('exit', (code: number | null) => { + if (typeof code === 'number') { + lastExitCode = code + } + }) + const MAX_BUFFER_CAP = 64 * 1024 channel.stderr.on('data', (data: Buffer) => { stderrOutput += data.toString('utf-8') @@ -49,7 +87,7 @@ export function waitForSentinel(channel: ClientChannel): Promise { - clearTimeout(timeout) + cancelTimers() if (!sentinelReceived) { if (!settled) { settled = true @@ -68,12 +106,28 @@ export function waitForSentinel(channel: ClientChannel): Promise { if (!sentinelReceived) { - clearTimeout(timeout) + cancelTimers() if (!settled) { settled = true + // Why: a wire-handshake mismatch on the daemon side closes the + // socket; --connect prints the mismatch detail to stderr and exits + // with code 42 BEFORE writing the sentinel. Translate that into a + // typed RelayVersionMismatchError so the retry loop in ssh.ts can + // distinguish a recoverable transport drop from this terminal + // condition and skip backoff. The check still wins over a fired + // timeout because the timeout handler defers settling for a small + // grace window so the close handler can deliver the exit code. + if (lastExitCode === RELAY_EXIT_CODE_VERSION_MISMATCH) { + const { expected, got } = parseHandshakeMismatchStderr(stderrOutput) + reject(new RelayVersionMismatchError(expected, got, stderrOutput.trim())) + return + } + const timeoutSuffix = timeoutFired + ? ` (after ${RELAY_SENTINEL_TIMEOUT_MS / 1000}s sentinel timeout)` + : '' reject( new Error( - `Relay process exited before ready.${stderrOutput ? ` stderr: ${stderrOutput.trim()}` : ''}` + `Relay process exited before ready${timeoutSuffix}.${stderrOutput ? ` stderr: ${stderrOutput.trim()}` : ''}` ) ) } @@ -109,7 +163,7 @@ export function waitForSentinel(channel: ClientChannel): Promise, daemon=" so the typed error carries +// actionable detail. Best-effort: returns undefined fields if the regex +// doesn't match, preserving the raw stderr verbatim for diagnostics. +function parseHandshakeMismatchStderr(stderr: string): { + expected: string | undefined + got: string | undefined +} { + const match = /expected=([^,\s]+),\s*daemon=([^\s;]+)/.exec(stderr) + if (!match) { + return { expected: undefined, got: undefined } + } + return { expected: match[1], got: match[2] } +} diff --git a/src/main/ssh/ssh-relay-deploy.test.ts b/src/main/ssh/ssh-relay-deploy.test.ts index 145cfa6cd..a6b0ca8d5 100644 --- a/src/main/ssh/ssh-relay-deploy.test.ts +++ b/src/main/ssh/ssh-relay-deploy.test.ts @@ -4,9 +4,13 @@ vi.mock('electron', () => ({ app: { getAppPath: () => '/mock/app' } })) +// Why: deployAndLaunchRelay now reads `${localRelayDir}/.version` upfront +// (per docs/ssh-relay-versioned-install-dirs.md). The fs mock must report +// the local relay package as existing AND return a content-hashed version +// string so readLocalFullVersion succeeds. vi.mock('fs', () => ({ - existsSync: vi.fn().mockReturnValue(false), - readFileSync: vi.fn() + existsSync: vi.fn().mockReturnValue(true), + readFileSync: vi.fn().mockReturnValue('0.1.0+abcdef012345') })) vi.mock('./relay-protocol', () => ({ @@ -28,6 +32,19 @@ vi.mock('./ssh-relay-deploy-helpers', () => ({ resolveRemoteNodePath: vi.fn().mockResolvedValue('/usr/bin/node') })) +// Why: the versioned-install module shells out to the remote for install +// state, lock acquisition, and GC. Tests stub these to no-ops so the deploy +// happy-path is exercised without a real SSH connection. +vi.mock('./ssh-relay-versioned-install', () => ({ + readLocalFullVersion: vi.fn().mockReturnValue('0.1.0+abcdef012345'), + computeRemoteRelayDir: (home: string, v: string) => `${home}/.orca-remote/relay-${v}`, + isRelayAlreadyInstalled: vi.fn().mockResolvedValue(true), + acquireInstallLock: vi.fn().mockResolvedValue(undefined), + finalizeInstall: vi.fn().mockResolvedValue(undefined), + abandonInstall: vi.fn().mockResolvedValue(undefined), + gcOldRelayVersions: vi.fn().mockResolvedValue(undefined) +})) + vi.mock('./ssh-connection-utils', () => ({ shellEscape: (s: string) => `'${s}'` })) @@ -70,8 +87,6 @@ describe('deployAndLaunchRelay', () => { const mockExecCommand = vi.mocked(execCommand) mockExecCommand.mockResolvedValueOnce('Linux x86_64') // uname -sm mockExecCommand.mockResolvedValueOnce('/home/user') // echo $HOME - mockExecCommand.mockResolvedValueOnce('OK') // check relay exists - mockExecCommand.mockResolvedValueOnce('0.1.0') // version check mockExecCommand.mockResolvedValueOnce('DEAD') // socket probe mockExecCommand.mockResolvedValueOnce('READY') // socket poll @@ -85,8 +100,6 @@ describe('deployAndLaunchRelay', () => { const mockExecCommand = vi.mocked(execCommand) mockExecCommand.mockResolvedValueOnce('Linux x86_64') mockExecCommand.mockResolvedValueOnce('/home/user') - mockExecCommand.mockResolvedValueOnce('OK') - mockExecCommand.mockResolvedValueOnce('0.1.0') mockExecCommand.mockResolvedValueOnce('DEAD') // socket probe mockExecCommand.mockResolvedValueOnce('READY') // socket poll @@ -97,6 +110,27 @@ describe('deployAndLaunchRelay', () => { expect(progress).toContain('Starting relay...') }) + it('uses a content-hashed versioned remote install directory', async () => { + const conn = makeMockConnection() + const mockExecCommand = vi.mocked(execCommand) + mockExecCommand.mockResolvedValueOnce('Linux x86_64') + mockExecCommand.mockResolvedValueOnce('/home/user') + mockExecCommand.mockResolvedValueOnce('DEAD') + mockExecCommand.mockResolvedValueOnce('READY') + + await deployAndLaunchRelay(conn) + + // The launch + connect commands include the versioned dir path. + const execArgs = vi.mocked(conn.exec).mock.calls.map(([cmd]) => cmd as string) + const allCmds = [...execArgs, ...mockExecCommand.mock.calls.map(([, cmd]) => cmd)] + const sawVersionedDir = allCmds.some((cmd) => + cmd.includes('/.orca-remote/relay-0.1.0+abcdef012345') + ) + expect(sawVersionedDir).toBe(true) + const sawLegacyDir = allCmds.some((cmd) => cmd.includes('relay-v0.1.0')) + expect(sawLegacyDir).toBe(false) + }) + it('has a 120-second overall timeout', async () => { const conn = makeMockConnection() const mockExecCommand = vi.mocked(execCommand) @@ -125,14 +159,10 @@ describe('deployAndLaunchRelay', () => { mockExecCommand .mockResolvedValueOnce('Linux x86_64') // uname A .mockResolvedValueOnce('/home/user') // $HOME A - .mockResolvedValueOnce('OK') // exists A - .mockResolvedValueOnce('0.1.0') // version A .mockResolvedValueOnce('DEAD') // probe A .mockResolvedValueOnce('READY') // poll A .mockResolvedValueOnce('Linux x86_64') // uname B .mockResolvedValueOnce('/home/user') // $HOME B - .mockResolvedValueOnce('OK') // exists B - .mockResolvedValueOnce('0.1.0') // version B .mockResolvedValueOnce('DEAD') // probe B .mockResolvedValueOnce('READY') // poll B diff --git a/src/main/ssh/ssh-relay-deploy.ts b/src/main/ssh/ssh-relay-deploy.ts index 8251b244c..a166d2d7b 100644 --- a/src/main/ssh/ssh-relay-deploy.ts +++ b/src/main/ssh/ssh-relay-deploy.ts @@ -1,14 +1,13 @@ import { join } from 'path' +/* eslint-disable max-lines -- Why: the relay-deploy module owns one cohesive + contract — version detection, install-locked deploy, native-deps probe, + relay launch, and GC — and splitting risks drift between the install + sequence and the GC's live-socket invariant. */ import { existsSync } from 'fs' import { app } from 'electron' import { createHash } from 'crypto' import type { SshConnection } from './ssh-connection' -import { - RELAY_VERSION, - RELAY_REMOTE_DIR, - parseUnameToRelayPlatform, - type RelayPlatform -} from './relay-protocol' +import { parseUnameToRelayPlatform, type RelayPlatform } from './relay-protocol' import type { MultiplexerTransport } from './ssh-channel-multiplexer' import { uploadDirectory, @@ -16,6 +15,15 @@ import { execCommand, resolveRemoteNodePath } from './ssh-relay-deploy-helpers' +import { + readLocalFullVersion, + computeRemoteRelayDir, + isRelayAlreadyInstalled, + acquireInstallLock, + finalizeInstall, + abandonInstall, + gcOldRelayVersions +} from './ssh-relay-versioned-install' import { shellEscape } from './ssh-connection-utils' export type RelayDeployResult = { @@ -79,6 +87,19 @@ async function deployAndLaunchRelayInner( } console.log(`[ssh-relay] Platform: ${platform}`) + const localRelayDir = getLocalRelayPath(platform) + if (!localRelayDir) { + throw new Error( + `Relay package for ${platform} not found locally. ` + + `This may be a packaging issue — try reinstalling Orca.` + ) + } + // Why: read the content-hashed full version from the local build's .version + // file. Used as both the remote dir name and the wire-handshake version. + // Throws on missing/empty rather than silently falling back — see + // docs/ssh-relay-versioned-install-dirs.md "Data Flow: Upstream Error". + const fullVersion = readLocalFullVersion(localRelayDir) + // Why: SFTP does not expand `~`, so we must resolve the remote home directory // explicitly. `echo $HOME` over exec gives us the absolute path. const remoteHome = (await execCommand(conn, 'echo $HOME')).trim() @@ -89,24 +110,46 @@ async function deployAndLaunchRelayInner( if (!remoteHome || !remoteHome.startsWith('/') || /[\u0000\r\n]/.test(remoteHome)) { throw new Error(`Remote $HOME is not a valid path: ${remoteHome.slice(0, 100)}`) } - const remoteRelayDir = `${remoteHome}/${RELAY_REMOTE_DIR}/relay-v${RELAY_VERSION}` + const remoteRelayDir = computeRemoteRelayDir(remoteHome, fullVersion) console.log(`[ssh-relay] Remote dir: ${remoteRelayDir}`) onProgress?.('Checking existing relay...') - const localRelayDir = getLocalRelayPath(platform) - const alreadyDeployed = await checkRelayExists(conn, remoteRelayDir, localRelayDir) - console.log(`[ssh-relay] Already deployed: ${alreadyDeployed}`) + const alreadyInstalled = await isRelayAlreadyInstalled(conn, remoteRelayDir) + console.log(`[ssh-relay] Already installed at ${fullVersion}: ${alreadyInstalled}`) - if (!alreadyDeployed) { - onProgress?.('Uploading relay...') - console.log('[ssh-relay] Uploading relay...') - await uploadRelay(conn, platform, remoteRelayDir) - console.log('[ssh-relay] Upload complete') + if (!alreadyInstalled) { + // Why: serialize concurrent first-installs of the same version against + // each other via an atomic mkdir lock. The losing caller polls and either + // re-checks `alreadyInstalled` (now true) or steals a stale lock. + await acquireInstallLock(conn, remoteRelayDir) + try { + // Re-probe after acquiring the lock — a sibling installer may have + // finished while we were waiting. + if (!(await isRelayAlreadyInstalled(conn, remoteRelayDir))) { + onProgress?.('Uploading relay...') + console.log('[ssh-relay] Uploading relay...') + await uploadRelay(conn, platform, remoteRelayDir, fullVersion) + console.log('[ssh-relay] Upload complete') - onProgress?.('Installing native dependencies...') - console.log('[ssh-relay] Installing node-pty...') - await installNativeDeps(conn, remoteRelayDir) - console.log('[ssh-relay] Native deps installed') + onProgress?.('Installing native dependencies...') + console.log('[ssh-relay] Installing node-pty...') + await installNativeDeps(conn, remoteRelayDir, platform) + console.log('[ssh-relay] Native deps installed') + + // Why: write `.install-complete` BEFORE releasing the lock so a + // sibling never observes the dir as "complete but locked", which + // would lead GC to skip a recoverable dir indefinitely. + await finalizeInstall(conn, remoteRelayDir) + } else { + await abandonInstall(conn, remoteRelayDir) + } + } catch (err) { + // Why: leave a partial install dir in place (no `.install-complete`) + // so the next deploy detects the partial and re-runs upload + install. + // Just release the lock so a concurrent caller can retry. + await abandonInstall(conn, remoteRelayDir) + throw err + } } onProgress?.('Starting relay...') @@ -114,6 +157,11 @@ async function deployAndLaunchRelayInner( const transport = await launchRelay(conn, remoteRelayDir, graceTimeSeconds, relayInstanceId) console.log('[ssh-relay] Relay started successfully') + // Why: best-effort cleanup of unreferenced sibling version dirs. Errors + // are logged inside gcOldRelayVersions and never propagate, so a GC failure + // can never block the user from connecting. + void gcOldRelayVersions(conn, remoteHome, remoteRelayDir).catch(() => {}) + return { transport, platform } } @@ -126,47 +174,11 @@ async function detectRemotePlatform(conn: SshConnection): Promise { - try { - const output = await execCommand( - conn, - `test -f ${shellEscape(`${remoteDir}/relay.js`)} && echo OK || echo MISSING` - ) - if (output.trim() !== 'OK') { - return false - } - - // Why: compare against the local .version file content (which includes a - // content hash) so any code change triggers re-deploy, even without bumping - // RELAY_VERSION. Falls back to the bare RELAY_VERSION for safety. - let expectedVersion = RELAY_VERSION - if (localRelayDir) { - try { - const { readFileSync } = await import('fs') - expectedVersion = readFileSync(join(localRelayDir, '.version'), 'utf-8').trim() - } catch { - /* fall back to RELAY_VERSION */ - } - } - - const versionOutput = await execCommand( - conn, - `cat ${shellEscape(`${remoteDir}/.version`)} 2>/dev/null || echo MISSING` - ) - return versionOutput.trim() === expectedVersion - } catch { - return false - } -} - async function uploadRelay( conn: SshConnection, platform: RelayPlatform, - remoteDir: string + remoteDir: string, + fullVersion: string ): Promise { const localRelayDir = getLocalRelayPath(platform) if (!localRelayDir || !existsSync(localRelayDir)) { @@ -191,25 +203,16 @@ async function uploadRelay( // Make the node binary executable await execCommand(conn, `chmod +x ${shellEscape(`${remoteDir}/node`)} 2>/dev/null; true`) - // Why: version marker includes a content hash so code changes trigger - // re-deploy even without bumping RELAY_VERSION. Read from the local build - // output so the remote marker matches exactly what checkRelayExists expects. - // Why: we write the version file via SFTP instead of a shell command to - // avoid shell injection — the version string could contain characters - // that break or escape single-quoted shell interpolation. - let versionString = RELAY_VERSION - const localVersionFile = join(localRelayDir, '.version') - if (existsSync(localVersionFile)) { - const { readFileSync } = await import('fs') - versionString = readFileSync(localVersionFile, 'utf-8').trim() - } + // Why: write `.version` via SFTP rather than shell to avoid quoting issues + // with content-hashed version strings. The remote daemon reads this same + // file on startup so the wire-handshake validates against it. const versionSftp = await conn.sftp() try { await new Promise((resolve, reject) => { const ws = versionSftp.createWriteStream(`${remoteDir}/.version`) ws.on('close', resolve) ws.on('error', reject) - ws.end(versionString) + ws.end(fullVersion) }) } finally { versionSftp.end() @@ -217,10 +220,19 @@ async function uploadRelay( } // Why: node-pty is a native addon that can't be bundled by esbuild. It must -// be compiled on the remote host against its Node.js version and OS. We run -// `npm init -y && npm install node-pty` in the relay directory so -// `require('node-pty')` resolves to the local node_modules. -async function installNativeDeps(conn: SshConnection, remoteDir: string): Promise { +// be compiled on the remote host against its Node.js version and OS. We +// write a minimal package.json + run `npm install node-pty` in the relay +// directory so `require('node-pty')` resolves to the local node_modules. +// +// TODO(#1693): VS Code ships per-platform tarballs with node-pty pre-built +// from CI and skips `npm install` on the remote entirely. That approach +// eliminates the whole class of bugs around npm/compiler/network failures +// on the remote. Worth doing once we're past the immediate fix. +async function installNativeDeps( + conn: SshConnection, + remoteDir: string, + platform: RelayPlatform +): Promise { const nodePath = await resolveRemoteNodePath(conn) // Why: node's bin directory must be in PATH for npm's child processes. // npm install runs node-pty's prebuild script (`node scripts/prebuild.js`) @@ -229,25 +241,78 @@ async function installNativeDeps(conn: SshConnection, remoteDir: string): Promis const nodeBinDir = nodePath.replace(/\/node$/, '') const escapedDir = shellEscape(remoteDir) const escapedBinDir = shellEscape(nodeBinDir) + const escapedNode = shellEscape(nodePath) + + // npm init -y rejects '+' in derived package names (content-hashed dir + // names like relay-0.1.0+abc123). Bypass it with a fixed minimal + // package.json. type:commonjs pins module resolution against Node default + // flips or a remote ~/.npmrc setting type=module. + const pkgJson = `${JSON.stringify({ + name: 'orca-relay', + version: '1.0.0', + private: true, + type: 'commonjs' + })}\n` + const sftpPkg = await conn.sftp() + try { + await new Promise((resolve, reject) => { + const ws = sftpPkg.createWriteStream(`${remoteDir}/package.json`) + // .once: a session 'error' arriving after we've already resolved/rejected + // would otherwise become an unhandled error and crash main. + sftpPkg.once('error', reject) + ws.once('close', resolve) + ws.once('error', reject) + ws.end(pkgJson) + }) + } finally { + sftpPkg.end() + } try { await execCommand( conn, - `export PATH=${escapedBinDir}:$PATH && cd ${escapedDir} && npm init -y --silent 2>/dev/null && npm install node-pty 2>&1` - ) - // Why: SFTP uploads preserve file content but not Unix execute bits. - // node-pty ships a prebuilt `spawn-helper` binary that must be executable - // for posix_spawnp to fork the PTY process. - await execCommand( - conn, - `find ${shellEscape(`${remoteDir}/node_modules/node-pty/prebuilds`)} -name spawn-helper -exec chmod +x {} + 2>/dev/null; true` + `export PATH=${escapedBinDir}:$PATH && cd ${escapedDir} && npm install node-pty 2>&1` ) } catch (err) { - // Why: node-pty install can fail if build tools (python, make, g++) are - // missing on the remote. Log the error but don't block relay startup — - // the relay will degrade gracefully (pty.spawn returns an error). - console.warn('[ssh-relay] Failed to install node-pty:', (err as Error).message) + // Don't write .install-complete on hard fail; reconnect retries on a + // partial install. Greppable token so user bug reports paste something + // searchable. + const msg = (err as Error).message + console.warn( + `[ssh-relay][NPTY-INSTALL-FAIL] npm install node-pty failed at ${remoteDir} (${platform}): ${msg}` + ) + throw err } + + // SFTP doesn't preserve execute bits; node-pty's spawn-helper prebuild + // must be +x for posix_spawnp. + await execCommand( + conn, + `find ${shellEscape(`${remoteDir}/node_modules/node-pty/prebuilds`)} -name spawn-helper -exec chmod +x {} + 2>/dev/null; true` + ) + + // node -e require() catches unloadable installs (wrong arch, missing + // prebuild, broken native binding) that test -d cannot. Stderr → file + // so .bashrc noise can't pollute the sentinel match; preserved for the + // [NPTY-MISSING] breadcrumb. MISSING is non-fatal by design — see + // docs/ssh-relay-versioned-install-dirs.md (relay still serves + // fs/git/preflight; only pty.spawn fails at runtime). + const PROBE_OK = 'ORCA-NPTY-PROBE-OK' + const stderrFile = `${remoteDir}/.npty-probe.stderr` + const escapedStderr = shellEscape(stderrFile) + const probeOutput = await execCommand( + conn, + `export PATH=${escapedBinDir}:$PATH && cd ${escapedDir} && (${escapedNode} -e 'require("node-pty"); console.log(process.argv[1])' ${shellEscape(PROBE_OK)} 2>${escapedStderr} || echo MISSING)` + ) + if (!probeOutput.includes(PROBE_OK)) { + const remoteStderr = await execCommand(conn, `cat ${escapedStderr} 2>/dev/null; true`).catch( + () => '' + ) + console.warn( + `[ssh-relay][NPTY-MISSING] node-pty installed but require() failed at ${remoteDir} (${platform}). stdout=${probeOutput.trim().slice(-200)} stderr=${remoteStderr.trim().slice(-500)}` + ) + } + await execCommand(conn, `rm -f ${escapedStderr} 2>/dev/null; true`).catch(() => {}) } function getLocalRelayPath(platform: RelayPlatform): string | null { diff --git a/src/main/ssh/ssh-relay-native-deps-install.test.ts b/src/main/ssh/ssh-relay-native-deps-install.test.ts new file mode 100644 index 000000000..a5d381f9a --- /dev/null +++ b/src/main/ssh/ssh-relay-native-deps-install.test.ts @@ -0,0 +1,462 @@ +/* eslint-disable max-lines -- Why: pinning every layer that should have + caught the original "node-pty not available" bug (chained shell, package + ordering, probe shape, channel-failure surfacing, .bashrc-noise immunity, + platform-tagged logs) requires keeping these scenarios in one file so the + shared mock connection and exec-response fixture stay aligned. */ +// Why: regression coverage for the install-probe contract. The original +// "node-pty is not available" bug shipped because every layer that should +// have caught it (chained shell, swallowing catch, dir-only probe) was +// silent. Tests below pin the parts that, individually, would have caught +// it. + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +vi.mock('electron', () => ({ + app: { getAppPath: () => '/mock/app' } +})) + +vi.mock('fs', () => ({ + existsSync: vi.fn().mockReturnValue(true), + readFileSync: vi.fn().mockReturnValue('0.1.0+testhash') +})) + +vi.mock('./relay-protocol', () => ({ + RELAY_VERSION: '0.1.0', + RELAY_REMOTE_DIR: '.orca-remote', + parseUnameToRelayPlatform: vi.fn().mockReturnValue('linux-x64'), + RELAY_SENTINEL: 'ORCA-RELAY v0.1.0 READY\n', + RELAY_SENTINEL_TIMEOUT_MS: 10_000 +})) + +vi.mock('./ssh-relay-deploy-helpers', () => ({ + uploadDirectory: vi.fn().mockResolvedValue(undefined), + waitForSentinel: vi.fn().mockResolvedValue({ + write: vi.fn(), + onData: vi.fn(), + onClose: vi.fn() + }), + execCommand: vi.fn(), + resolveRemoteNodePath: vi.fn().mockResolvedValue('/usr/bin/node') +})) + +vi.mock('./ssh-relay-versioned-install', () => ({ + readLocalFullVersion: vi.fn().mockReturnValue('0.1.0+testhash'), + computeRemoteRelayDir: (home: string, v: string) => `${home}/.orca-remote/relay-${v}`, + isRelayAlreadyInstalled: vi.fn().mockResolvedValue(false), + acquireInstallLock: vi.fn().mockResolvedValue(undefined), + finalizeInstall: vi.fn().mockResolvedValue(undefined), + abandonInstall: vi.fn().mockResolvedValue(undefined), + gcOldRelayVersions: vi.fn().mockResolvedValue(undefined) +})) + +vi.mock('./ssh-connection-utils', () => ({ + shellEscape: (s: string) => `'${s}'` +})) + +import { deployAndLaunchRelay } from './ssh-relay-deploy' +import { execCommand } from './ssh-relay-deploy-helpers' +import { parseUnameToRelayPlatform } from './relay-protocol' +import { + abandonInstall, + finalizeInstall, + isRelayAlreadyInstalled +} from './ssh-relay-versioned-install' +import type { SshConnection } from './ssh-connection' + +type SftpWriteCapture = { + paths: string[] + contents: Record + // Number of execCommand calls observed at the moment ws.end() ran for each + // captured path. Used to pin "package.json was written before npm install". + execCallCountAtWrite: Record +} + +function makeMockConnection(capture: SftpWriteCapture): SshConnection { + const sftpCreate = (): unknown => ({ + mkdir: vi.fn((_p: string, cb: (err: Error | null) => void) => cb(null)), + on: vi.fn(), + once: vi.fn(), + createWriteStream: vi.fn().mockImplementation((path: string) => { + capture.paths.push(path) + let buf = '' + let closeCb: (() => void) | undefined + const stub = { + on: vi.fn((event: string, cb: () => void) => { + if (event === 'close') { + closeCb = cb + } + }), + end: vi.fn((data?: string) => { + if (typeof data === 'string') { + buf += data + } + capture.contents[path] = buf + capture.execCallCountAtWrite[path] = vi.mocked(execCommand).mock.calls.length + if (closeCb) { + setTimeout(closeCb, 0) + } + }) + } + // Why: production code uses ws.once('close', ...). The 'once' wrapper + // delegates to the same handler-table as 'on' for the test mock. + return Object.assign(stub, { once: stub.on }) + }), + end: vi.fn() + }) + return { + exec: vi.fn().mockResolvedValue({ + on: vi.fn(), + stderr: { on: vi.fn() }, + stdin: {}, + stdout: { on: vi.fn() }, + close: vi.fn() + }), + sftp: vi.fn().mockImplementation(() => Promise.resolve(sftpCreate())) + } as unknown as SshConnection +} + +type ExecResponse = string | { reject: string } + +// Exec call order under our mocks (deploy happy path): +// 1: uname 2: $HOME 3: mkdir remoteDir (uploadRelay) +// 4: chmod +x node 5: npm install 6: chmod prebuilds +// 7: probe (cd && node -e require) +// [8: cat stderr — only when probe stdout is MISSING (graceful path)] +// 8 or 9: rm probe-stderr (best-effort cleanup; runs whenever probe resolved) +// next: socket DEAD next: socket READY +// +// When the probe rejects (SSH channel close or cd-failure when the install +// dir vanished), the catch path skips both stderr-capture and the rm. +function makeExecResponses(opts: { + npmInstall: 'ok' | { reject: string } + // 'ok' : probe resolves with the sentinel; rm runs once + // 'missing' : probe resolves with 'MISSING'; cat stderr + rm both run + // 'dir-gone': probe rejects (cd-failure), exec rejects directly + // { reject }: probe rejects with custom error (e.g. SSH channel) + probe: 'ok' | 'missing' | 'dir-gone' | { reject: string } + // Override probe stdout for shell-noise pressure tests. If set, replaces + // the load-test stdout entirely (useful for testing pollution prefixes). + probeStdoutOverride?: string +}): ExecResponse[] { + const probeSlot: ExecResponse = + opts.probeStdoutOverride !== undefined + ? opts.probeStdoutOverride + : opts.probe === 'ok' + ? 'ORCA-NPTY-PROBE-OK\n' + : opts.probe === 'missing' + ? 'MISSING\n' // shell-level `|| echo MISSING` after require throw + : opts.probe === 'dir-gone' + ? { reject: 'cd: no such file or directory' } + : opts.probe + const slots: ExecResponse[] = [ + 'Linux x86_64', + '/home/u', + '', // mkdir remoteDir (uploadRelay) + '', // chmod +x node + opts.npmInstall === 'ok' ? '' : opts.npmInstall, + '', // chmod prebuilds + probeSlot + ] + // Cleanup execs only run when the probe resolved (not when it rejected). + const probeResolved = typeof probeSlot === 'string' + if (probeResolved) { + const probeOk = probeSlot.includes('ORCA-NPTY-PROBE-OK') + if (!probeOk) { + slots.push('') // cat stderr (graceful failure path captures detail) + } + slots.push('') // rm -f stderr (best-effort cleanup) + } + slots.push('DEAD', 'READY') + return slots +} + +describe('installNativeDeps (via deployAndLaunchRelay)', () => { + let warnSpy: ReturnType + const sftpCapture: SftpWriteCapture = { + paths: [], + contents: {}, + execCallCountAtWrite: {} + } + + beforeEach(() => { + vi.clearAllMocks() + // Tests that throw mid-deploy leave unconsumed `mockResolvedValueOnce` + // entries queued. Without resetting, the next test's first await consumes + // a leaked response. clearAllMocks doesn't drop the queue (it only clears + // .mock.calls), so we explicitly mockReset. + vi.mocked(execCommand).mockReset() + sftpCapture.paths.length = 0 + for (const k of Object.keys(sftpCapture.contents)) { + delete sftpCapture.contents[k] + } + for (const k of Object.keys(sftpCapture.execCallCountAtWrite)) { + delete sftpCapture.execCallCountAtWrite[k] + } + // Re-prime: factory mockReturnValue / mockResolvedValue survive + // clearAllMocks, so this is just defense-in-depth in case a test does its + // own resetAllMocks. + vi.mocked(parseUnameToRelayPlatform).mockReturnValue('linux-x64') + vi.mocked(isRelayAlreadyInstalled).mockResolvedValue(false) + warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) + }) + + afterEach(() => { + warnSpy.mockRestore() + }) + + function feed(execResponses: ExecResponse[]): void { + const mockExec = vi.mocked(execCommand) + for (const r of execResponses) { + if (typeof r === 'string') { + mockExec.mockResolvedValueOnce(r) + } else { + mockExec.mockRejectedValueOnce(new Error(r.reject)) + } + } + } + + it('writes a hardcoded package.json BEFORE running npm install', async () => { + const conn = makeMockConnection(sftpCapture) + feed(makeExecResponses({ npmInstall: 'ok', probe: 'ok' })) + + await deployAndLaunchRelay(conn) + + const pkgPath = sftpCapture.paths.find((p) => p.endsWith('/package.json')) + expect(pkgPath, 'package.json must be written via SFTP').toBeTruthy() + + const written = sftpCapture.contents[pkgPath as string] + expect(written).toBeTruthy() + const parsed = JSON.parse(written) as Record + expect(parsed.name).toBe('orca-relay') + expect(parsed.version).toBe('1.0.0') + expect(parsed.private).toBe(true) + // Why: pin commonjs so a future Node default flip doesn't silently + // break `require('node-pty')`. + expect(parsed.type).toBe('commonjs') + + const execCalls = vi.mocked(execCommand).mock.calls.map(([, c]) => c) + const npmInstallIdx = execCalls.findIndex((c) => c.includes('npm install node-pty')) + expect(npmInstallIdx).toBeGreaterThanOrEqual(0) + // Pin actual ordering: number of execCommand calls observed at the moment + // ws.end() ran for package.json must be < the index of `npm install`. + // Catches a future refactor that fires SFTP-write and npm install via + // Promise.all (where the final-state assertions above would still pass). + const writeObservedAt = sftpCapture.execCallCountAtWrite[pkgPath as string] + expect(writeObservedAt).toBeLessThanOrEqual(npmInstallIdx) + }) + + it('propagates a hard `npm install` failure so the deploy aborts before finalizeInstall', async () => { + const conn = makeMockConnection(sftpCapture) + feed( + makeExecResponses({ + npmInstall: { reject: 'npm ERR! E404 Not Found node-pty' }, + probe: 'ok' + }) + ) + + await expect(deployAndLaunchRelay(conn)).rejects.toThrow(/npm ERR/) + + // The crucial regression: `.install-complete` must NOT have been written. + // Previously the catch swallowed the throw and finalizeInstall ran anyway. + expect(vi.mocked(finalizeInstall)).not.toHaveBeenCalled() + + const warnMessages = warnSpy.mock.calls.map((args) => String(args[0] ?? '')) + expect(warnMessages.some((m) => m.includes('[ssh-relay][NPTY-INSTALL-FAIL]'))).toBe(true) + }) + + it('warns clearly when node-pty installs but require() fails (built-but-unloadable)', async () => { + const conn = makeMockConnection(sftpCapture) + feed(makeExecResponses({ npmInstall: 'ok', probe: 'missing' })) + + await deployAndLaunchRelay(conn) + + // Probe failure is non-fatal by design (see docs/ssh-relay-versioned- + // install-dirs.md): relay still serves fs/git/preflight, only pty.spawn + // fails at runtime. Throwing here would loop reconnects forever on + // hosts where node-pty truly cannot build (Alpine without compiler, + // glibc too old). + const warnMessages = warnSpy.mock.calls.map((args) => String(args[0] ?? '')) + expect(warnMessages.some((m) => m.includes('[ssh-relay][NPTY-MISSING]'))).toBe(true) + + expect(vi.mocked(finalizeInstall)).toHaveBeenCalledTimes(1) + expect(vi.mocked(abandonInstall)).not.toHaveBeenCalled() + }) + + it('lets a probe SSH-channel failure bubble up rather than silently mapping to MISSING', async () => { + const conn = makeMockConnection(sftpCapture) + feed( + makeExecResponses({ + npmInstall: 'ok', + probe: { reject: 'SSH channel closed unexpectedly' } + }) + ) + + await expect(deployAndLaunchRelay(conn)).rejects.toThrow(/SSH channel/) + + // Pin that the rejection actually came from the PROBE call (not some + // earlier/later exec). Drift in slot ordering would otherwise let this + // test pass while exercising a different failure path. + const execCalls = vi.mocked(execCommand).mock.calls.map(([, c]) => c) + const probeCallIdx = execCalls.findIndex((c) => c.includes('require("node-pty")')) + const npmInstallIdx = execCalls.findIndex((c) => c.includes('npm install node-pty')) + expect(probeCallIdx, 'probe must have been invoked').toBeGreaterThanOrEqual(0) + // Probe must come strictly AFTER `npm install` — otherwise we'd be + // probing into an empty install dir and this whole failure mode + // wouldn't represent the real-world race. + expect(probeCallIdx).toBeGreaterThan(npmInstallIdx) + + const warnMessages = warnSpy.mock.calls.map((args) => String(args[0] ?? '')) + // Channel failure must NOT be conflated with "node-pty missing" or with + // "npm install failed". + expect(warnMessages.some((m) => m.includes('[ssh-relay][NPTY-MISSING]'))).toBe(false) + expect(warnMessages.some((m) => m.includes('[ssh-relay][NPTY-INSTALL-FAIL]'))).toBe(false) + + expect(vi.mocked(finalizeInstall)).not.toHaveBeenCalled() + // Lock must be released so a future reconnect can retry. + expect(vi.mocked(abandonInstall)).toHaveBeenCalledTimes(1) + }) + + it('throws (rather than warns MISSING) when the install dir vanishes between npm install and probe', async () => { + const conn = makeMockConnection(sftpCapture) + feed(makeExecResponses({ npmInstall: 'ok', probe: 'dir-gone' })) + + // The probe shape `cd ${dir} && (node -e ... || echo MISSING)` short- + // circuits on cd-failure (`&&`), so the whole exec rejects rather than + // resolving with the MISSING sentinel. Conflating "dir vanished" with + // "node-pty missing" would mark the version installed and strand the + // user in degraded mode forever. + await expect(deployAndLaunchRelay(conn)).rejects.toThrow(/cd:/) + + // Pin that the rejection came from the probe slot specifically, not + // some earlier exec — otherwise a future refactor could move probe + // before npm install and this test would still pass for the wrong + // reason. + const execCalls = vi.mocked(execCommand).mock.calls.map(([, c]) => c) + const probeIdx = execCalls.findIndex((c) => c.includes('require("node-pty")')) + const npmInstallIdx = execCalls.findIndex((c) => c.includes('npm install node-pty')) + expect(probeIdx).toBeGreaterThan(npmInstallIdx) + + const warnMessages = warnSpy.mock.calls.map((args) => String(args[0] ?? '')) + expect(warnMessages.some((m) => m.includes('[ssh-relay][NPTY-MISSING]'))).toBe(false) + + expect(vi.mocked(finalizeInstall)).not.toHaveBeenCalled() + expect(vi.mocked(abandonInstall)).toHaveBeenCalledTimes(1) + }) + + it('uses `node -e require()` rather than `test -d` so unloadable installs are caught', async () => { + const conn = makeMockConnection(sftpCapture) + feed(makeExecResponses({ npmInstall: 'ok', probe: 'ok' })) + + await deployAndLaunchRelay(conn) + + const probeCmds = vi + .mocked(execCommand) + .mock.calls.map(([, c]) => c) + .filter((c) => c.includes(`require("node-pty")`)) + + // Why: the probe shape must invoke the deployed node binary against + // require('node-pty'). A weaker probe (test -d) could pass even when + // the native binding load is broken. + expect(probeCmds.length).toBeGreaterThan(0) + expect(probeCmds[0]).toMatch(/node['"]?\s+-e/) + + // Pin the full installNativeDeps exec sequence: npm install → chmod + // prebuilds → probe. A refactor that moves chmod-prebuilds after the + // probe would silently break spawn-helper bits; one that probes before + // npm install would test an empty dir. + const all = vi.mocked(execCommand).mock.calls.map(([, c]) => c) + const npmIdx = all.findIndex((c) => c.includes('npm install node-pty')) + const chmodPrebuildsIdx = all.findIndex( + (c) => c.includes('spawn-helper') && c.includes('chmod +x') + ) + const probeIdx = all.findIndex((c) => c.includes('require("node-pty")')) + expect(npmIdx).toBeGreaterThanOrEqual(0) + expect(chmodPrebuildsIdx).toBeGreaterThan(npmIdx) + expect(probeIdx).toBeGreaterThan(chmodPrebuildsIdx) + + // Happy path: finalize exactly once, abandon never. + expect(vi.mocked(finalizeInstall)).toHaveBeenCalledTimes(1) + expect(vi.mocked(abandonInstall)).not.toHaveBeenCalled() + }) + + it('matches the sentinel even with bashrc/MOTD noise prefixed to probe stdout', async () => { + const conn = makeMockConnection(sftpCapture) + // Some remotes have customized .bashrc that prints to stdout on every + // non-interactive shell exec (corporate MOTD, NVM/conda init banners). + // Production uses .includes(PROBE_OK) with stderr redirected to a file, + // so noise on stdout BEFORE the sentinel must still resolve to OK. + feed( + makeExecResponses({ + npmInstall: 'ok', + probe: 'ok', + probeStdoutOverride: 'Welcome to Acme Corp\nLast login: ...\nORCA-NPTY-PROBE-OK\n' + }) + ) + + await deployAndLaunchRelay(conn) + + const warnMessages = warnSpy.mock.calls.map((args) => String(args[0] ?? '')) + expect(warnMessages.some((m) => m.includes('[ssh-relay][NPTY-MISSING]'))).toBe(false) + expect(vi.mocked(finalizeInstall)).toHaveBeenCalledTimes(1) + }) + + it('detects MISSING even when the shell prepends noise before the MISSING token', async () => { + const conn = makeMockConnection(sftpCapture) + feed( + makeExecResponses({ + npmInstall: 'ok', + probe: 'missing', + probeStdoutOverride: '(node:1234) [DEP0040] DeprecationWarning: ...\nMISSING\n' + }) + ) + + await deployAndLaunchRelay(conn) + + // Absence of PROBE_OK is what triggers the warn, regardless of what + // appears around it. finalize still runs (degraded-mode by design). + const warnMessages = warnSpy.mock.calls.map((args) => String(args[0] ?? '')) + expect(warnMessages.some((m) => m.includes('[ssh-relay][NPTY-MISSING]'))).toBe(true) + expect(vi.mocked(finalizeInstall)).toHaveBeenCalledTimes(1) + expect(vi.mocked(abandonInstall)).not.toHaveBeenCalled() + }) + + it('includes the platform tuple in NPTY-MISSING and NPTY-INSTALL-FAIL logs', async () => { + // Platform tuple lets bug reports be triaged for prebuild availability + // without asking the user to dig out their arch. + const conn = makeMockConnection(sftpCapture) + feed(makeExecResponses({ npmInstall: 'ok', probe: 'missing' })) + await deployAndLaunchRelay(conn) + const missingMsgs = warnSpy.mock.calls + .map((args) => String(args[0] ?? '')) + .filter((m) => m.includes('[ssh-relay][NPTY-MISSING]')) + expect(missingMsgs.length).toBeGreaterThan(0) + expect(missingMsgs[0]).toContain('linux-x64') + }) + + it('writes an idempotent package.json (same bytes on every install)', async () => { + // First install run. + const conn1 = makeMockConnection(sftpCapture) + feed(makeExecResponses({ npmInstall: 'ok', probe: 'ok' })) + await deployAndLaunchRelay(conn1) + const firstPath = sftpCapture.paths.find((p) => p.endsWith('/package.json')) as string + const first = sftpCapture.contents[firstPath] + + // Reset capture, run again as if it were a fresh install of the same dir. + sftpCapture.paths.length = 0 + for (const k of Object.keys(sftpCapture.contents)) { + delete sftpCapture.contents[k] + } + for (const k of Object.keys(sftpCapture.execCallCountAtWrite)) { + delete sftpCapture.execCallCountAtWrite[k] + } + vi.mocked(execCommand).mockReset() + + const conn2 = makeMockConnection(sftpCapture) + feed(makeExecResponses({ npmInstall: 'ok', probe: 'ok' })) + await deployAndLaunchRelay(conn2) + const secondPath = sftpCapture.paths.find((p) => p.endsWith('/package.json')) as string + const second = sftpCapture.contents[secondPath] + + expect(second).toBe(first) + }) +}) diff --git a/src/main/ssh/ssh-relay-session-terminal-error.test.ts b/src/main/ssh/ssh-relay-session-terminal-error.test.ts new file mode 100644 index 000000000..0b56e275a --- /dev/null +++ b/src/main/ssh/ssh-relay-session-terminal-error.test.ts @@ -0,0 +1,163 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +import { SshRelaySession } from './ssh-relay-session' +import type { SshConnection } from './ssh-connection' +import type { Store } from '../persistence' +import type { SshPortForwardManager } from './ssh-port-forward' +import { RelayVersionMismatchError } from './ssh-relay-version-mismatch-error' +import type { BrowserWindow } from 'electron' + +vi.mock('./ssh-relay-deploy', () => ({ + deployAndLaunchRelay: vi.fn() +})) + +vi.mock('./ssh-channel-multiplexer', () => { + return { + SshChannelMultiplexer: class MockSshChannelMultiplexer { + notify = vi.fn() + request = vi.fn().mockResolvedValue([]) + onNotification = vi.fn().mockReturnValue(() => {}) + onDispose = vi.fn().mockReturnValue(() => {}) + dispose = vi.fn() + isDisposed = vi.fn().mockReturnValue(false) + } + } +}) + +vi.mock('../providers/ssh-pty-provider', () => ({ + SshPtyProvider: class MockSshPtyProvider { + onData = vi.fn().mockReturnValue(() => {}) + onReplay = vi.fn().mockReturnValue(() => {}) + onExit = vi.fn().mockReturnValue(() => {}) + attach = vi.fn().mockResolvedValue(undefined) + dispose = vi.fn() + } +})) + +vi.mock('../providers/ssh-filesystem-provider', () => ({ + SshFilesystemProvider: class MockSshFilesystemProvider { + dispose = vi.fn() + } +})) + +vi.mock('../providers/ssh-git-provider', () => ({ + SshGitProvider: class MockSshGitProvider {} +})) + +vi.mock('../ipc/pty', () => ({ + registerSshPtyProvider: vi.fn(), + unregisterSshPtyProvider: vi.fn(), + getSshPtyProvider: vi.fn().mockReturnValue({ + dispose: vi.fn(), + attach: vi.fn().mockResolvedValue(undefined) + }), + getPtyIdsForConnection: vi.fn().mockReturnValue([]), + clearPtyOwnershipForConnection: vi.fn(), + clearProviderPtyState: vi.fn(), + deletePtyOwnership: vi.fn() +})) + +vi.mock('../providers/ssh-filesystem-dispatch', () => ({ + registerSshFilesystemProvider: vi.fn(), + unregisterSshFilesystemProvider: vi.fn(), + getSshFilesystemProvider: vi.fn().mockReturnValue({ dispose: vi.fn() }) +})) + +vi.mock('../providers/ssh-git-dispatch', () => ({ + registerSshGitProvider: vi.fn(), + unregisterSshGitProvider: vi.fn() +})) + +const { deployAndLaunchRelay } = await import('./ssh-relay-deploy') + +function createMockDeps(): { + mockConn: SshConnection + mockStore: Store + mockPortForward: SshPortForwardManager + getMainWindow: () => BrowserWindow | null +} { + const mockConn = {} as SshConnection + const mockStore = { + getRepos: vi.fn().mockReturnValue([]) + } as unknown as Store + const mockPortForward = { + removeAllForwards: vi.fn() + } as unknown as SshPortForwardManager + const mockWindow = { + isDestroyed: (): boolean => false, + webContents: { send: vi.fn() } + } as unknown as BrowserWindow + const getMainWindow = vi.fn().mockReturnValue(mockWindow) as unknown as () => BrowserWindow | null + return { mockConn, mockStore, mockPortForward, getMainWindow } +} + +function mockDeploySuccess(): void { + const mockTransport = { + write: vi.fn(), + onData: vi.fn(), + onClose: vi.fn() + } + vi.mocked(deployAndLaunchRelay).mockResolvedValue({ + transport: mockTransport, + platform: 'linux-x64' + }) +} + +describe('SshRelaySession terminal relay error (RelayVersionMismatchError)', () => { + beforeEach(() => { + vi.clearAllMocks() + mockDeploySuccess() + }) + + it('fires onTerminalRelayError on initial establish() and rethrows', async () => { + const { mockConn, mockStore, mockPortForward, getMainWindow } = createMockDeps() + const session = new SshRelaySession('target-1', getMainWindow, mockStore, mockPortForward) + const onTerminal = vi.fn() + const onLost = vi.fn() + session.setOnTerminalRelayError(onTerminal) + session.setOnRelayLost(onLost) + + const mismatchErr = new RelayVersionMismatchError( + '0.1.0+aaa', + '0.1.0+bbb', + '[relay-connect] Handshake mismatch...' + ) + vi.mocked(deployAndLaunchRelay).mockRejectedValueOnce(mismatchErr) + + await expect(session.establish(mockConn)).rejects.toBe(mismatchErr) + expect(onTerminal).toHaveBeenCalledTimes(1) + expect(onTerminal).toHaveBeenCalledWith('target-1', mismatchErr) + expect(onLost).not.toHaveBeenCalled() + expect(session.getState()).toBe('idle') + }) + + it('does NOT fire onTerminalRelayError on a generic establish failure', async () => { + const { mockConn, mockStore, mockPortForward, getMainWindow } = createMockDeps() + const session = new SshRelaySession('target-1', getMainWindow, mockStore, mockPortForward) + const onTerminal = vi.fn() + session.setOnTerminalRelayError(onTerminal) + + vi.mocked(deployAndLaunchRelay).mockRejectedValueOnce(new Error('boom')) + + await expect(session.establish(mockConn)).rejects.toThrow('boom') + expect(onTerminal).not.toHaveBeenCalled() + expect(session.getState()).toBe('idle') + }) + + it('fires onTerminalRelayError on reconnect() when deploy throws RelayVersionMismatchError', async () => { + const { mockConn, mockStore, mockPortForward, getMainWindow } = createMockDeps() + const session = new SshRelaySession('target-1', getMainWindow, mockStore, mockPortForward) + const onTerminal = vi.fn() + session.setOnTerminalRelayError(onTerminal) + + await session.establish(mockConn) + expect(session.getState()).toBe('ready') + + const mismatchErr = new RelayVersionMismatchError('0.1.0+old', '0.1.0+new', '') + vi.mocked(deployAndLaunchRelay).mockRejectedValueOnce(mismatchErr) + + await session.reconnect(mockConn) + expect(onTerminal).toHaveBeenCalledTimes(1) + expect(onTerminal).toHaveBeenCalledWith('target-1', mismatchErr) + }) +}) diff --git a/src/main/ssh/ssh-relay-session.ts b/src/main/ssh/ssh-relay-session.ts index f0f8cc2b0..4401053f7 100644 --- a/src/main/ssh/ssh-relay-session.ts +++ b/src/main/ssh/ssh-relay-session.ts @@ -10,6 +10,8 @@ import type { BrowserWindow } from 'electron' import { deployAndLaunchRelay } from './ssh-relay-deploy' +import { isRelayVersionMismatchError } from './ssh-relay-version-mismatch-error' +import type { RelayVersionMismatchError } from './ssh-relay-version-mismatch-error' import { SshChannelMultiplexer } from './ssh-channel-multiplexer' import { SshPtyProvider } from '../providers/ssh-pty-provider' import { SshFilesystemProvider } from '../providers/ssh-filesystem-provider' @@ -47,6 +49,14 @@ export class SshRelaySession { // up, the onStateChange reconnect path never fires. This callback lets // ssh.ts wire up relay-level reconnect from outside the session. private _onRelayLost: ((targetId: string) => void) | null = null + // Why: a wire-handshake mismatch is terminal — the daemon and client are at + // different versions, no amount of backoff retry will reconcile them. This + // separate callback lets ssh.ts surface the failure to the user and skip + // the relay-lost backoff loop entirely. Distinct from _onRelayLost because + // _onRelayLost expects a recoverable transport drop. + private _onTerminalRelayError: + | ((targetId: string, err: RelayVersionMismatchError) => void) + | null = null private _onReady: ((targetId: string) => void) | null = null private portScanner: PortScanner | null = null @@ -67,6 +77,10 @@ export class SshRelaySession { this._onRelayLost = cb } + setOnTerminalRelayError(cb: (targetId: string, err: RelayVersionMismatchError) => void): void { + this._onTerminalRelayError = cb + } + setOnReady(cb: (targetId: string) => void): void { this._onReady = cb } @@ -159,6 +173,20 @@ export class SshRelaySession { this.teardownProviders('shutdown') this._state = 'idle' } + // Why: a wire-handshake mismatch on the FIRST connect is also terminal + // — the deployed relay binary on disk does not match a still-running + // daemon (typically because a legacy daemon from before the + // versioned-dir change is still alive). Notify the terminal-error + // callback so ssh.ts surfaces an actionable message and the caller's + // catch path doesn't conflate this with a transient deploy failure. + // We still rethrow so doConnect's existing failure path runs (clean up + // the SSH connection); ssh.ts's handler is idempotent. + if (isRelayVersionMismatchError(err)) { + console.warn( + `[ssh-relay-session] Terminal relay version mismatch on initial connect for ${this.targetId}: ${err.message}` + ) + this._onTerminalRelayError?.(this.targetId, err) + } throw err } } @@ -294,6 +322,17 @@ export class SshRelaySession { if (this.abortController === abortController && !this.isDisposed()) { this.teardownProviders('connection_lost') } + // Why: a version-mismatch is terminal. Fire the typed callback so + // ssh.ts can surface a "please reconnect manually" notice and skip the + // relay-lost backoff loop entirely. We do NOT keep state at + // 'reconnecting' — there's no transient drop to recover from. + if (isRelayVersionMismatchError(err)) { + console.warn( + `[ssh-relay-session] Terminal relay version mismatch for ${this.targetId}: ${err.message}` + ) + this._onTerminalRelayError?.(this.targetId, err) + return + } // Why: stay in 'reconnecting' rather than reverting to 'ready', because // the provider stack is already torn down. The SSH connection manager // will fire another onStateChange when it reconnects again. diff --git a/src/main/ssh/ssh-relay-version-mismatch-error.ts b/src/main/ssh/ssh-relay-version-mismatch-error.ts new file mode 100644 index 000000000..66bc90c28 --- /dev/null +++ b/src/main/ssh/ssh-relay-version-mismatch-error.ts @@ -0,0 +1,34 @@ +// Why: a unique error class so callers (in particular the relay-lost retry +// loop in src/main/ipc/ssh.ts) can branch on `instanceof +// RelayVersionMismatchError` and treat the failure as terminal — i.e. skip +// the exponential-backoff retry and surface a user-visible "please reconnect +// manually" error. Any other transport failure remains transiently retryable. +// +// Trigger: the remote `--connect` process exits with code 42 after the +// daemon's wire-level handshake reports a version mismatch. See +// docs/ssh-relay-versioned-install-dirs.md. + +export class RelayVersionMismatchError extends Error { + readonly name = 'RelayVersionMismatchError' + + constructor( + readonly expected: string | undefined, + readonly got: string | undefined, + readonly stderr?: string + ) { + super( + `Remote relay version mismatch — expected=${expected ?? 'unknown'}, ` + + `daemon=${got ?? 'unknown'}. The remote daemon was launched against a different ` + + `relay binary than the local client expects. Please reconnect manually.` + ) + } +} + +export function isRelayVersionMismatchError(err: unknown): err is RelayVersionMismatchError { + return err instanceof RelayVersionMismatchError +} + +// Why: the remote --connect process uses this exit code to signal the wire +// handshake failed because of a version mismatch. The mapping daemon ⇄ exit +// code 42 lives in src/relay/relay-handshake.ts (EXIT_CODE_VERSION_MISMATCH). +export const RELAY_EXIT_CODE_VERSION_MISMATCH = 42 diff --git a/src/main/ssh/ssh-relay-versioned-install.test.ts b/src/main/ssh/ssh-relay-versioned-install.test.ts new file mode 100644 index 000000000..a7bae19e9 --- /dev/null +++ b/src/main/ssh/ssh-relay-versioned-install.test.ts @@ -0,0 +1,324 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +vi.mock('fs', () => ({ + existsSync: vi.fn(), + readFileSync: vi.fn() +})) + +vi.mock('./ssh-relay-deploy-helpers', () => ({ + execCommand: vi.fn() +})) + +vi.mock('./ssh-connection-utils', () => ({ + shellEscape: (s: string) => `'${s}'` +})) + +import { existsSync, readFileSync } from 'fs' +import { + readLocalFullVersion, + computeRemoteRelayDir, + isRelayAlreadyInstalled, + acquireInstallLock, + finalizeInstall, + abandonInstall, + gcOldRelayVersions +} from './ssh-relay-versioned-install' +import { execCommand } from './ssh-relay-deploy-helpers' +import type { SshConnection } from './ssh-connection' + +const conn = {} as SshConnection +const mockExec = vi.mocked(execCommand) +const mockExists = vi.mocked(existsSync) +const mockRead = vi.mocked(readFileSync) + +describe('readLocalFullVersion', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it('returns trimmed contents of the .version file', () => { + mockExists.mockReturnValue(true) + mockRead.mockReturnValue('0.1.0+deadbeef\n') + expect(readLocalFullVersion('/local/relay')).toBe('0.1.0+deadbeef') + }) + + it('throws an actionable error when the .version file is missing', () => { + mockExists.mockReturnValue(false) + expect(() => readLocalFullVersion('/local/relay')).toThrow(/missing its version marker/) + }) + + it('throws when the .version file is empty', () => { + mockExists.mockReturnValue(true) + mockRead.mockReturnValue(' \n') + expect(() => readLocalFullVersion('/local/relay')).toThrow(/is empty/) + }) +}) + +describe('computeRemoteRelayDir', () => { + it('joins remoteHome with .orca-remote and the version-keyed dir name', () => { + expect(computeRemoteRelayDir('/home/u', '0.1.0+abc')).toBe( + '/home/u/.orca-remote/relay-0.1.0+abc' + ) + }) +}) + +describe('isRelayAlreadyInstalled', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it('returns true only when the OK probe succeeds', async () => { + mockExec.mockResolvedValueOnce('OK') + expect(await isRelayAlreadyInstalled(conn, '/r')).toBe(true) + }) + + it('returns false when the probe reports MISSING', async () => { + mockExec.mockResolvedValueOnce('MISSING') + expect(await isRelayAlreadyInstalled(conn, '/r')).toBe(false) + }) + + it('returns false on exec error', async () => { + mockExec.mockRejectedValueOnce(new Error('boom')) + expect(await isRelayAlreadyInstalled(conn, '/r')).toBe(false) + }) + + it('checks for relay.js AND .install-complete in addition to the dir', async () => { + mockExec.mockResolvedValueOnce('OK') + await isRelayAlreadyInstalled(conn, '/r') + const cmd = mockExec.mock.calls.at(-1)?.[1] ?? '' + expect(cmd).toContain('relay.js') + expect(cmd).toContain('.install-complete') + }) +}) + +describe('acquireInstallLock', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it('returns when mkdir reports OK', async () => { + // 1st call: mkdir -p remoteRelayDir + // 2nd call: mkdir lockDir → OK + mockExec.mockResolvedValueOnce('').mockResolvedValueOnce('OK') + await acquireInstallLock(conn, '/r') + expect(mockExec).toHaveBeenCalledTimes(2) + }) + + it('polls until the lock becomes available (concurrent installer wins, then we acquire)', async () => { + vi.useFakeTimers() + try { + // Sequence: + // 1. mkdir -p (parent dir prep) + // 2. mkdir lockDir → BUSY (someone else holds it) + // 3. mkdir lockDir → BUSY again + // 4. mkdir lockDir → OK (concurrent installer released) + mockExec + .mockResolvedValueOnce('') + .mockResolvedValueOnce('BUSY') + .mockResolvedValueOnce('BUSY') + .mockResolvedValueOnce('OK') + + const promise = acquireInstallLock(conn, '/r') + // Drive the polling loop: each iteration awaits a 1s timer. + for (let i = 0; i < 5; i++) { + await vi.advanceTimersByTimeAsync(1_000) + } + await promise + const cmds = mockExec.mock.calls.map(([, c]) => c) + const mkdirAttempts = cmds.filter((c) => c.includes('mkdir') && c.includes('.install-lock')) + expect(mkdirAttempts.length).toBeGreaterThanOrEqual(3) + } finally { + vi.useRealTimers() + } + }) + + it('steals a stale lock and retries with a reset timeout window', async () => { + vi.useFakeTimers({ now: 1_700_000_000_000 }) + try { + let mkdirCalls = 0 + mockExec.mockImplementation(async (_conn: unknown, cmd: string) => { + if (cmd.startsWith('mkdir -p')) { + return '' + } + if (cmd.includes('mkdir') && cmd.includes('.install-lock')) { + mkdirCalls++ + return mkdirCalls > 200 ? 'OK' : 'BUSY' + } + if (cmd.includes('stat')) { + return `${Math.floor((Date.now() - 10 * 60 * 1000) / 1000)}\n` + } + if (cmd.startsWith('rm -rf')) { + mkdirCalls = 1000 + return '' + } + return '' + }) + + const promise = acquireInstallLock(conn, '/r') + // Drive through the full timeout (120s) so the stale-recovery branch + // fires, then drive a few more seconds for the post-recovery retry. + await vi.advanceTimersByTimeAsync(125_000) + await promise + + const cmds = mockExec.mock.calls.map(([, c]) => c) + expect(cmds.some((c) => c.includes('rm -rf') && c.includes('.install-lock'))).toBe(true) + } finally { + vi.useRealTimers() + } + }) + + it('throws if the timeout elapses and the lock is fresh', async () => { + vi.useFakeTimers({ now: 1_700_000_000_000 }) + try { + mockExec.mockImplementation(async (_conn: unknown, cmd: string) => { + if (cmd.startsWith('mkdir -p')) { + return '' + } + if (cmd.includes('mkdir') && cmd.includes('.install-lock')) { + return 'BUSY' + } + if (cmd.includes('stat')) { + return `${Math.floor(Date.now() / 1000)}\n` + } + return '' + }) + + const rejection = expect(acquireInstallLock(conn, '/r')).rejects.toThrow(/not yet stale/i) + await vi.advanceTimersByTimeAsync(125_000) + await rejection + } finally { + vi.useRealTimers() + } + }) + + it('finalizeInstall writes .install-complete then removes the lock', async () => { + mockExec.mockResolvedValueOnce('').mockResolvedValueOnce('') + await finalizeInstall(conn, '/r') + const cmds = mockExec.mock.calls.map(([, c]) => c) + expect(cmds[0]).toContain('touch') + expect(cmds[0]).toContain('.install-complete') + expect(cmds[1]).toContain('rm -rf') + expect(cmds[1]).toContain('.install-lock') + }) + + it('abandonInstall removes the lock without writing the sentinel', async () => { + mockExec.mockResolvedValueOnce('') + await abandonInstall(conn, '/r') + const cmd = mockExec.mock.calls[0]?.[1] ?? '' + expect(cmd).toContain('rm -rf') + expect(cmd).toContain('.install-lock') + expect(cmd).not.toContain('.install-complete') + }) +}) + +describe('gcOldRelayVersions', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it('removes a sibling that is complete, unlocked, and has no live socket', async () => { + // ls listing + mockExec.mockResolvedValueOnce('relay-0.1.0+aaa\nrelay-0.1.0+bbb\n') + // For sibling "aaa": LOCKED probe → OPEN, COMPLETE probe → COMPLETE, sock probe → empty (no ALIVE), then rm -rf + mockExec + .mockResolvedValueOnce('OPEN') + .mockResolvedValueOnce('COMPLETE') + .mockResolvedValueOnce('') + .mockResolvedValueOnce('') + + await gcOldRelayVersions(conn, '/home/u', '/home/u/.orca-remote/relay-0.1.0+bbb') + + const lastCmd = mockExec.mock.calls.at(-1)?.[1] ?? '' + expect(lastCmd).toContain('rm -rf') + expect(lastCmd).toContain('relay-0.1.0+aaa') + }) + + it('skips siblings that are missing .install-complete (mid-install or partial)', async () => { + mockExec.mockResolvedValueOnce('relay-0.1.0+aaa\n') + mockExec + .mockResolvedValueOnce('OPEN') // not locked + .mockResolvedValueOnce('PARTIAL') // missing .install-complete + await gcOldRelayVersions(conn, '/home/u', '/home/u/.orca-remote/relay-0.1.0+bbb') + const cmds = mockExec.mock.calls.map(([, c]) => c) + expect(cmds.some((c) => c.includes('rm -rf'))).toBe(false) + }) + + it('skips siblings whose .install-lock is held and fresh', async () => { + mockExec.mockResolvedValueOnce('relay-0.1.0+aaa\n') + mockExec.mockResolvedValueOnce('LOCKED') + // isLockStale: mtime ~now → not stale. + mockExec.mockResolvedValueOnce(`${Math.floor(Date.now() / 1000)}\n`) + await gcOldRelayVersions(conn, '/home/u', '/home/u/.orca-remote/relay-0.1.0+bbb') + const cmds = mockExec.mock.calls.map(([, c]) => c) + expect(cmds.some((c) => c.includes('rm -rf'))).toBe(false) + }) + + it('removes a sibling with a stale lock + .install-complete (rm-lock failed mid-finalize)', async () => { + mockExec.mockResolvedValueOnce('relay-0.1.0+aaa\n') + mockExec.mockResolvedValueOnce('LOCKED') + // isLockStale: mtime well in the past → stale. + const staleSec = Math.floor((Date.now() - 10 * 60 * 1000) / 1000) + mockExec.mockResolvedValueOnce(`${staleSec}\n`) + mockExec.mockResolvedValueOnce('COMPLETE') // .install-complete present + mockExec.mockResolvedValueOnce('') // socket probe → no ALIVE + mockExec.mockResolvedValueOnce('') // rm -rf + await gcOldRelayVersions(conn, '/home/u', '/home/u/.orca-remote/relay-0.1.0+bbb') + const lastCmd = mockExec.mock.calls.at(-1)?.[1] ?? '' + expect(lastCmd).toContain('rm -rf') + expect(lastCmd).toContain('relay-0.1.0+aaa') + }) + + it('GCs a legacy relay-v0.1.0 dir whose daemon is dead (no .install-complete required)', async () => { + mockExec.mockResolvedValueOnce('relay-v0.1.0\n') + mockExec.mockResolvedValueOnce('OPEN') // not locked + mockExec.mockResolvedValueOnce('') // socket probe → no ALIVE (no completeProbe — legacy) + mockExec.mockResolvedValueOnce('') // rm -rf + await gcOldRelayVersions(conn, '/home/u', '/home/u/.orca-remote/relay-0.1.0+bbb') + const cmds = mockExec.mock.calls.map(([, c]) => c) + expect(cmds.some((c) => c.includes('rm -rf') && c.includes('relay-v0.1.0'))).toBe(true) + // critically: no .install-complete probe on legacy dirs + expect(cmds.some((c) => c.includes('.install-complete'))).toBe(false) + }) + + it('keeps a legacy relay-v0.1.0 dir whose daemon is still serving', async () => { + mockExec.mockResolvedValueOnce('relay-v0.1.0\n') + mockExec.mockResolvedValueOnce('OPEN') + mockExec.mockResolvedValueOnce('ALIVE') // socket alive → keep + await gcOldRelayVersions(conn, '/home/u', '/home/u/.orca-remote/relay-0.1.0+bbb') + const cmds = mockExec.mock.calls.map(([, c]) => c) + expect(cmds.some((c) => c.includes('rm -rf'))).toBe(false) + }) + + it('skips siblings with a live relay-*.sock', async () => { + mockExec.mockResolvedValueOnce('relay-0.1.0+aaa\n') + mockExec + .mockResolvedValueOnce('OPEN') + .mockResolvedValueOnce('COMPLETE') + .mockResolvedValueOnce('ALIVE') + await gcOldRelayVersions(conn, '/home/u', '/home/u/.orca-remote/relay-0.1.0+bbb') + const cmds = mockExec.mock.calls.map(([, c]) => c) + expect(cmds.some((c) => c.includes('rm -rf'))).toBe(false) + }) + + it('does not consider the current dir as a GC candidate', async () => { + mockExec.mockResolvedValueOnce('relay-0.1.0+aaa\n') + await gcOldRelayVersions(conn, '/home/u', '/home/u/.orca-remote/relay-0.1.0+aaa') + expect(mockExec.mock.calls.length).toBe(1) // only the listing + }) + + it('ignores entries that do not match the relay version dir regex (allowlist)', async () => { + mockExec.mockResolvedValueOnce('logs\nbackup\nrelay-0.1.0+aaa\n') + mockExec + .mockResolvedValueOnce('OPEN') + .mockResolvedValueOnce('COMPLETE') + .mockResolvedValueOnce('') + .mockResolvedValueOnce('') + await gcOldRelayVersions(conn, '/home/u', '/home/u/.orca-remote/relay-0.1.0+bbb') + const cmds = mockExec.mock.calls.map(([, c]) => c) + const rmCmds = cmds.filter((c) => c.includes('rm -rf')) + expect(rmCmds).toHaveLength(1) + expect(rmCmds[0]).toContain('relay-0.1.0+aaa') + expect(rmCmds[0]).not.toContain('logs') + expect(rmCmds[0]).not.toContain('backup') + }) +}) diff --git a/src/main/ssh/ssh-relay-versioned-install.ts b/src/main/ssh/ssh-relay-versioned-install.ts new file mode 100644 index 000000000..07aaa28c2 --- /dev/null +++ b/src/main/ssh/ssh-relay-versioned-install.ts @@ -0,0 +1,340 @@ +// Versioned-install plumbing for the remote relay. +// +// Why this exists: the relay used to install into a single shared directory +// (~/.orca-remote/relay-v0.1.0) which the deploy step would overwrite in place +// on every cross-version push. A daemon already loaded into memory then served +// new clients off rewritten on-disk code, producing protocol drift and a +// reconnect loop. We now install each (RELAY_VERSION + content-hash) bundle +// into its own directory and never mutate it after the install finishes, +// matching VS Code's `~/.vscode-server/bin//` layout. +// +// See: docs/ssh-relay-versioned-install-dirs.md + +import { join } from 'path' +import { existsSync, readFileSync } from 'fs' +import type { SshConnection } from './ssh-connection' +import { RELAY_REMOTE_DIR } from './relay-protocol' +import { execCommand } from './ssh-relay-deploy-helpers' +import { shellEscape } from './ssh-connection-utils' + +// Why: the GC pass and the version-dir parser must agree on what counts as a +// relay install dir. Single source of truth for both. The pattern matches the +// new layout `relay-${RELAY_VERSION}+${hash}` and the legacy `relay-v${VERSION}` +// so the GC eventually drains the old layout once its daemons idle out. +const RELAY_VERSION_DIR_REGEX = /^relay-(v?\d+\.\d+\.\d+(\+[0-9a-f]+)?)$/ + +// Why: legacy dirs from before `.install-complete` was introduced (i.e. the +// `relay-v0.1.0` shape with no content-hash suffix). They are missing the +// install-complete sentinel by definition and need a separate liveness-only +// GC check so they actually drain after the legacy daemon dies, instead of +// living on remote disks forever. +const LEGACY_RELAY_DIR_REGEX = /^relay-v\d+\.\d+\.\d+$/ + +const INSTALL_LOCK_NAME = '.install-lock' +const INSTALL_COMPLETE_NAME = '.install-complete' + +const INSTALL_LOCK_POLL_MS = 1_000 +const INSTALL_LOCK_TIMEOUT_MS = 120_000 +// Why: a stale lock dir from a crashed installer must be recoverable without +// user intervention. After the timeout we check the lock's mtime; if it's +// older than this window the previous installer is assumed dead and we steal +// the lock. 2 minutes is well above a normal `npm install node-pty` runtime +// (10–60s on slow hosts) so a slow concurrent installer is not falsely +// declared dead. +const INSTALL_LOCK_STALE_MS = 120_000 + +/** + * Read the local relay's content-hashed version (e.g. "0.1.0+0a5fe134d020") + * from `${localRelayDir}/.version`. Throws on missing/empty so the caller + * never silently falls back to a path where a daemon from a different code + * generation may already be running — that fallback is the failure mode the + * versioned-install design exists to prevent. + */ +export function readLocalFullVersion(localRelayDir: string): string { + const versionFile = join(localRelayDir, '.version') + if (!existsSync(versionFile)) { + throw new Error( + `Orca's local relay build is missing its version marker at ${versionFile}. ` + + `This usually indicates a packaging or build problem; reinstall Orca.` + ) + } + const v = readFileSync(versionFile, 'utf-8').trim() + if (!v) { + throw new Error( + `Orca's local relay version marker at ${versionFile} is empty. ` + + `This usually indicates a packaging or build problem; reinstall Orca.` + ) + } + return v +} + +/** + * Compute the absolute remote install directory for a given content-hashed + * version. The format is `${remoteHome}/${RELAY_REMOTE_DIR}/relay-${fullVersion}`. + */ +export function computeRemoteRelayDir(remoteHome: string, fullVersion: string): string { + return `${remoteHome}/${RELAY_REMOTE_DIR}/relay-${fullVersion}` +} + +/** + * Probe whether a fully-installed relay already exists at remoteRelayDir. + * + * "Fully installed" means: the directory exists, contains relay.js, AND + * contains the .install-complete sentinel written at the end of a successful + * install. A directory missing .install-complete is either mid-install (lock + * held) or a crashed-install partial — either way we re-run the deploy. + */ +export async function isRelayAlreadyInstalled( + conn: SshConnection, + remoteRelayDir: string +): Promise { + try { + const probe = await execCommand( + conn, + `test -d ${shellEscape(remoteRelayDir)} ` + + `&& test -f ${shellEscape(`${remoteRelayDir}/relay.js`)} ` + + `&& test -f ${shellEscape(`${remoteRelayDir}/${INSTALL_COMPLETE_NAME}`)} ` + + `&& echo OK || echo MISSING` + ) + return probe.trim() === 'OK' + } catch { + return false + } +} + +/** + * Acquire the per-version install lock via atomic `mkdir`. Returns when the + * caller owns the lock; throws if the lock could not be acquired within + * INSTALL_LOCK_TIMEOUT_MS even after one stale-lock recovery attempt. + * + * Why mkdir: POSIX `mkdir` is atomic and fails with EEXIST if the dir already + * exists, giving us a free mutex. A second concurrent caller polls and + * eventually either acquires the lock or steals it after the stale window. + */ +export async function acquireInstallLock( + conn: SshConnection, + remoteRelayDir: string +): Promise { + const lockDir = `${remoteRelayDir}/${INSTALL_LOCK_NAME}` + // Why: the parent dir may not exist yet on a first install. mkdir -p is + // safe to run multiple times — it's a no-op if the dir already exists. + await execCommand(conn, `mkdir -p ${shellEscape(remoteRelayDir)}`) + + let start = Date.now() + let recoveredOnce = false + while (true) { + try { + const result = await execCommand( + conn, + `mkdir ${shellEscape(lockDir)} 2>&1 && echo OK || echo BUSY` + ) + if (result.trim().endsWith('OK')) { + return + } + } catch { + /* mkdir failed with non-zero — fall through to BUSY treatment */ + } + if (Date.now() - start >= INSTALL_LOCK_TIMEOUT_MS) { + if (recoveredOnce) { + throw new Error( + `Could not acquire relay install lock at ${lockDir} after ${ + INSTALL_LOCK_TIMEOUT_MS / 1000 + }s; another install is in progress or the lock is wedged.` + ) + } + // Stale-lock recovery: if the lock dir's mtime is older than the stale + // window, the previous installer crashed. Steal it and retry once, + // resetting the timeout window so a single post-recovery race doesn't + // immediately exhaust the budget. + const ageOk = await isLockStale(conn, lockDir) + if (ageOk) { + console.warn(`[ssh-relay] Stealing stale install lock at ${lockDir}`) + await execCommand(conn, `rm -rf ${shellEscape(lockDir)}`).catch(() => {}) + recoveredOnce = true + start = Date.now() + continue + } + throw new Error( + `Could not acquire relay install lock at ${lockDir} after ${ + INSTALL_LOCK_TIMEOUT_MS / 1000 + }s and the lock is not yet stale.` + ) + } + await new Promise((r) => setTimeout(r, INSTALL_LOCK_POLL_MS)) + } +} + +async function isLockStale(conn: SshConnection, lockDir: string): Promise { + try { + // Why: `stat` flags differ between GNU coreutils (Linux) and BSD (macOS). + // We try GNU first, then BSD; both produce a Unix epoch in seconds on + // stdout. If both fail we conservatively treat the lock as not stale. + const out = await execCommand( + conn, + `stat -c %Y ${shellEscape(lockDir)} 2>/dev/null || stat -f %m ${shellEscape(lockDir)} 2>/dev/null || echo` + ) + const mtimeSec = parseInt(out.trim(), 10) + if (!Number.isFinite(mtimeSec)) { + return false + } + const ageMs = Date.now() - mtimeSec * 1000 + return ageMs > INSTALL_LOCK_STALE_MS + } catch { + return false + } +} + +/** + * Mark the install as complete and release the lock. Sentinel ordering is: + * write `.install-complete` FIRST, then remove `.install-lock`. This ensures + * a sibling dir is never observed by GC as "complete but locked", which + * would lead GC to skip a recoverable dir indefinitely. + */ +export async function finalizeInstall(conn: SshConnection, remoteRelayDir: string): Promise { + const sentinel = `${remoteRelayDir}/${INSTALL_COMPLETE_NAME}` + const lock = `${remoteRelayDir}/${INSTALL_LOCK_NAME}` + await execCommand(conn, `touch ${shellEscape(sentinel)}`) + await execCommand(conn, `rm -rf ${shellEscape(lock)}`).catch(() => {}) +} + +/** + * Release the install lock without writing the completion sentinel. Called + * from the failure path so the dir remains a recoverable partial that the + * next deploy detects (alreadyInstalled=false) and re-runs upload+install. + */ +export async function abandonInstall(conn: SshConnection, remoteRelayDir: string): Promise { + const lock = `${remoteRelayDir}/${INSTALL_LOCK_NAME}` + await execCommand(conn, `rm -rf ${shellEscape(lock)}`).catch(() => {}) +} + +/** + * Garbage-collect old version directories. Removes a sibling dir under + * `${remoteHome}/${RELAY_REMOTE_DIR}/` only if ALL of: + * + * - it matches the relay-version-dir regex (allowlist) + * - it is NOT the current version dir + * - it has no live `relay-*.sock` (pgrep + connectability probe) + * - it contains `.install-complete` (a fully-installed dir, not a partial) + * - it does NOT contain `.install-lock` (no in-progress install) + * + * Best-effort: any error is logged and swallowed; GC must never block the + * user from connecting. + */ +export async function gcOldRelayVersions( + conn: SshConnection, + remoteHome: string, + currentDirAbsPath: string +): Promise { + const baseDir = `${remoteHome}/${RELAY_REMOTE_DIR}` + const currentDirName = currentDirAbsPath.split('/').filter(Boolean).pop() ?? '' + let listing: string + try { + listing = await execCommand(conn, `ls -1 ${shellEscape(baseDir)} 2>/dev/null || true`) + } catch { + return + } + const candidates = listing + .split('\n') + .map((s) => s.trim()) + .filter(Boolean) + .filter((name) => RELAY_VERSION_DIR_REGEX.test(name)) + .filter((name) => name !== currentDirName) + + if (candidates.length === 0) { + return + } + + const removed: string[] = [] + const kept: string[] = [] + for (const name of candidates) { + const dir = `${baseDir}/${name}` + try { + const safe = await isCandidateSafeToRemove(conn, dir, name) + if (!safe) { + kept.push(name) + continue + } + await execCommand(conn, `rm -rf ${shellEscape(dir)}`) + removed.push(name) + } catch (err) { + console.warn( + `[ssh-relay] GC failed for ${dir}: ${err instanceof Error ? err.message : String(err)}` + ) + kept.push(name) + } + } + + if (removed.length > 0) { + const keptSuffix = kept.length > 0 ? ` (kept: ${kept.join(', ')})` : '' + console.log( + `[ssh-relay] GC: removed ${removed.length} stale version dir(s): ${removed.join(', ')}${keptSuffix}` + ) + } +} + +async function isCandidateSafeToRemove( + conn: SshConnection, + dir: string, + name: string +): Promise { + const isLegacy = LEGACY_RELAY_DIR_REGEX.test(name) + + const lockProbe = await execCommand( + conn, + `test -d ${shellEscape(`${dir}/${INSTALL_LOCK_NAME}`)} && echo LOCKED || echo OPEN` + ).catch(() => 'OPEN') + const locked = lockProbe.trim() === 'LOCKED' + + if (locked) { + // Why: a locked dir is normally unsafe to remove — but a STALE lock + // (mtime older than INSTALL_LOCK_STALE_MS) means the previous installer + // crashed and is never coming back. If the dir also has the + // .install-complete sentinel (touch succeeded but the rm-lock at the + // end of finalizeInstall failed), removing the dir is safe — no + // installer is racing us, and the daemon (if any) keeps running off + // its already-loaded code regardless of disk state. + const lockDir = `${dir}/${INSTALL_LOCK_NAME}` + if (!(await isLockStale(conn, lockDir))) { + return false + } + process.stderr.write?.(`[ssh-relay] GC: lock at ${lockDir} is stale; treating as recoverable\n`) + } + + // Legacy dirs (relay-v0.1.0) predate .install-complete. Skip the sentinel + // check for them and rely solely on the live-socket probe — that's the + // only signal we have that a legacy daemon is still serving clients. + if (!isLegacy) { + const completeProbe = await execCommand( + conn, + `test -f ${shellEscape(`${dir}/${INSTALL_COMPLETE_NAME}`)} && echo COMPLETE || echo PARTIAL` + ).catch(() => 'PARTIAL') + if (completeProbe.trim() !== 'COMPLETE') { + // Crashed-install partial; leave for the next deploy to recover. + return false + } + } + + const sockAlive = await hasLiveRelaySocket(conn, dir) + if (sockAlive) { + return false + } + return true +} + +async function hasLiveRelaySocket(conn: SshConnection, dir: string): Promise { + try { + // Why: `ls -1 dir/relay-*.sock 2>/dev/null` lists socket files. For each, + // we test -S to confirm it's a socket inode. We do NOT attempt to open + // the socket here — `test -S` is sufficient for the GC decision and a + // connect-and-close probe would race with a daemon that's about to idle. + const out = await execCommand( + conn, + `for f in ${shellEscape(dir)}/relay-*.sock ${shellEscape(dir)}/relay.sock; do ` + + `[ -S "$f" ] && echo ALIVE && break; ` + + `done; true` + ) + return out.includes('ALIVE') + } catch { + return false + } +} diff --git a/src/relay/git-exec-validator.test.ts b/src/relay/git-exec-validator.test.ts index ce168eede..54039ebd7 100644 --- a/src/relay/git-exec-validator.test.ts +++ b/src/relay/git-exec-validator.test.ts @@ -28,7 +28,16 @@ describe('validateGitExecArgs', () => { [['config', '--get-all', 'remote.origin.url']], [['config', '--list']], [['config', '-l']], - [['config', '--get-regexp', 'user']] + [['config', '--get-regexp', 'user']], + [['for-each-ref', '--format=%(refname)', 'refs/remotes']], + [ + [ + 'for-each-ref', + '--format=%(refname)%00%(refname:short)', + '--sort=-committerdate', + 'refs/heads/*foo*' + ] + ] ])('allows %j', (args) => { expectAllowed(args) }) @@ -76,7 +85,13 @@ describe('validateGitExecArgs', () => { [['log', '-o', '/tmp/leak']], [['rev-parse', '--exec-path=/evil']], [['log', '--work-tree=/other']], - [['log', '--git-dir=/other/.git']] + [['log', '--git-dir=/other/.git']], + // Pin global-deny coverage on for-each-ref so a future allowlist + // refactor that bypassed GLOBAL_DENIED_FLAGS for this subcommand fails + // loudly. The first round of for-each-ref enablement omitted these. + [['for-each-ref', '--git-dir=/other/.git', '--format=%(refname)']], + [['for-each-ref', '--output=/tmp/leak', '--format=%(refname)']], + [['for-each-ref', '--work-tree=/other']] ])('rejects %j', (args) => { expectBlocked(args, 'Dangerous git flags are not allowed') }) diff --git a/src/relay/git-exec-validator.ts b/src/relay/git-exec-validator.ts index fa22356c5..b881fbb6a 100644 --- a/src/relay/git-exec-validator.ts +++ b/src/relay/git-exec-validator.ts @@ -17,6 +17,7 @@ const ALLOWED_GIT_SUBCOMMANDS = new Set([ 'symbolic-ref', 'merge-base', 'ls-files', + 'for-each-ref', 'config' ]) const CONFIG_READ_ONLY_FLAGS = new Set(['--get', '--get-all', '--list', '--get-regexp', '-l']) diff --git a/src/relay/protocol-handshake.test.ts b/src/relay/protocol-handshake.test.ts new file mode 100644 index 000000000..822fea3e2 --- /dev/null +++ b/src/relay/protocol-handshake.test.ts @@ -0,0 +1,68 @@ +import { describe, expect, it } from 'vitest' +import { + MessageType, + HEADER_LENGTH, + FrameDecoder, + encodeHandshakeFrame, + parseHandshakeMessage, + type DecodedFrame +} from './protocol' + +describe('handshake framing', () => { + it('round-trips an orca-relay-handshake envelope through the existing framing', () => { + const sent = encodeHandshakeFrame({ + type: 'orca-relay-handshake', + version: '0.1.0+deadbeef' + }) + expect(sent[0]).toBe(MessageType.Handshake) + expect(sent.length).toBeGreaterThan(HEADER_LENGTH) + + const frames: DecodedFrame[] = [] + const decoder = new FrameDecoder((f) => frames.push(f)) + decoder.feed(sent) + + expect(frames).toHaveLength(1) + expect(frames[0].type).toBe(MessageType.Handshake) + const msg = parseHandshakeMessage(frames[0].payload) + expect(msg).toEqual({ type: 'orca-relay-handshake', version: '0.1.0+deadbeef' }) + }) + + it('round-trips an orca-relay-handshake-ok reply', () => { + const sent = encodeHandshakeFrame({ + type: 'orca-relay-handshake-ok', + version: '0.1.0+deadbeef' + }) + const frames: DecodedFrame[] = [] + const decoder = new FrameDecoder((f) => frames.push(f)) + decoder.feed(sent) + const msg = parseHandshakeMessage(frames[0].payload) + expect(msg).toEqual({ type: 'orca-relay-handshake-ok', version: '0.1.0+deadbeef' }) + }) + + it('round-trips an orca-relay-handshake-mismatch reply', () => { + const sent = encodeHandshakeFrame({ + type: 'orca-relay-handshake-mismatch', + expected: '0.1.0+aaa', + got: '0.1.0+bbb' + }) + const frames: DecodedFrame[] = [] + const decoder = new FrameDecoder((f) => frames.push(f)) + decoder.feed(sent) + const msg = parseHandshakeMessage(frames[0].payload) + expect(msg).toEqual({ + type: 'orca-relay-handshake-mismatch', + expected: '0.1.0+aaa', + got: '0.1.0+bbb' + }) + }) + + it('rejects payloads with unknown type', () => { + const bogus = Buffer.from(JSON.stringify({ type: 'orca-something-else', version: 'x' })) + expect(() => parseHandshakeMessage(bogus)).toThrow(/Unknown handshake type/) + }) + + it('handshake frames use a distinct MessageType from Regular and KeepAlive', () => { + expect(MessageType.Handshake).not.toBe(MessageType.Regular) + expect(MessageType.Handshake).not.toBe(MessageType.KeepAlive) + }) +}) diff --git a/src/relay/protocol.ts b/src/relay/protocol.ts index dfe9bda73..e555289bc 100644 --- a/src/relay/protocol.ts +++ b/src/relay/protocol.ts @@ -9,9 +9,37 @@ export const MAX_MESSAGE_SIZE = 16 * 1024 * 1024 export const MessageType = { Regular: 1, + Handshake: 2, KeepAlive: 9 } as const +// Why: a pre-dispatcher envelope on a freshly-accepted Unix socket. The daemon +// reads exactly one Handshake frame before attaching the JSON-RPC dispatcher, +// to refuse mismatched-version --connect bridges that would otherwise drive a +// stale daemon. +export type HandshakeMessage = + | { type: 'orca-relay-handshake'; version: string } + | { type: 'orca-relay-handshake-ok'; version: string } + | { type: 'orca-relay-handshake-mismatch'; expected: string; got: string } + +export function encodeHandshakeFrame(msg: HandshakeMessage): Buffer { + const payload = Buffer.from(JSON.stringify(msg), 'utf-8') + return encodeFrame(MessageType.Handshake, 0, 0, payload) +} + +export function parseHandshakeMessage(payload: Buffer): HandshakeMessage { + const msg = JSON.parse(payload.toString('utf-8')) as HandshakeMessage + const t = (msg as { type?: string }).type + if ( + t !== 'orca-relay-handshake' && + t !== 'orca-relay-handshake-ok' && + t !== 'orca-relay-handshake-mismatch' + ) { + throw new Error(`Unknown handshake type: ${t}`) + } + return msg +} + export const KEEPALIVE_SEND_MS = 5_000 export const TIMEOUT_MS = 20_000 @@ -125,6 +153,16 @@ export class FrameDecoder { reset(): void { this.buffer = Buffer.alloc(0) } + + // Why: at the handshake → dispatcher transition, the next consumer must + // pick up any bytes that arrived in the same TCP chunk as the handshake + // frame. This returns and clears the decoder's internal residue so the + // caller can hand it to the dispatcher (or stdout pipe) without loss. + drain(): Buffer { + const out = this.buffer + this.buffer = Buffer.alloc(0) + return out + } } export function parseJsonRpcMessage(payload: Buffer): JsonRpcMessage { diff --git a/src/relay/relay-handshake-roundtrip.test.ts b/src/relay/relay-handshake-roundtrip.test.ts new file mode 100644 index 000000000..28d884940 --- /dev/null +++ b/src/relay/relay-handshake-roundtrip.test.ts @@ -0,0 +1,227 @@ +import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest' +import { createServer, connect, type Server, type Socket } from 'net' +import { mkdtempSync, rmSync } from 'fs' +import { tmpdir } from 'os' +import { join } from 'path' + +import { + setupDaemonHandshake, + runConnectHandshake, + EXIT_CODE_VERSION_MISMATCH +} from './relay-handshake' +import { + encodeHandshakeFrame, + encodeJsonRpcFrame, + FrameDecoder, + type DecodedFrame, + MessageType +} from './protocol' + +// Why: --connect normally calls process.exit on mismatch / fatal handshake +// errors. Stub it for tests so the harness sees a thrown sentinel error +// rather than tearing down the test runner. +class ExitCalled extends Error { + code: number + constructor(code: number) { + super(`process.exit(${code})`) + this.code = code + } +} + +describe('handshake round-trip over a real Socket pair', () => { + let server: Server + let sockPath: string + let tmpDir: string + let exitSpy: ReturnType + + let uncaughtHandler: (err: Error) => void + + beforeEach(() => { + tmpDir = mkdtempSync(join(tmpdir(), 'orca-handshake-test-')) + sockPath = join(tmpDir, 'relay.sock') + exitSpy = vi.spyOn(process, 'exit').mockImplementation(((code?: number) => { + throw new ExitCalled(code ?? 0) + }) as never) + // Why: process.exit is called from inside async callbacks + // (process.stderr.write flush callback) which would otherwise surface + // as an uncaughtException after the test resolves and tear down the + // runner. We swallow ExitCalled — exitSpy still records the call so + // assertions hold. + uncaughtHandler = (err: Error): void => { + if (err instanceof ExitCalled) { + return + } + throw err + } + process.on('uncaughtException', uncaughtHandler) + }) + + afterEach(async () => { + process.off('uncaughtException', uncaughtHandler) + exitSpy.mockRestore() + for (const s of liveServerSockets) { + s.destroy() + } + liveServerSockets.length = 0 + if (server) { + await new Promise((r) => server.close(() => r())) + } + rmSync(tmpDir, { recursive: true, force: true }) + }) + + const liveServerSockets: Socket[] = [] + function trackServerSocket(s: Socket): Socket { + liveServerSockets.push(s) + return s + } + + function startDaemon(version: string): Promise<{ + accepted: Promise<{ sock: Socket; leftover: Buffer }> + }> { + return new Promise((resolve) => { + const acceptedDeferred: { + promise: Promise<{ sock: Socket; leftover: Buffer }> + resolve: (v: { sock: Socket; leftover: Buffer }) => void + } = (() => { + let _resolve: (v: { sock: Socket; leftover: Buffer }) => void = () => {} + const promise = new Promise<{ sock: Socket; leftover: Buffer }>((r) => { + _resolve = r + }) + return { promise, resolve: _resolve } + })() + + server = createServer((sock) => { + trackServerSocket(sock) + setupDaemonHandshake(sock, { + launchVersion: version, + onAccepted: (s, leftover) => acceptedDeferred.resolve({ sock: s, leftover }) + }) + }) + server.listen(sockPath, () => resolve({ accepted: acceptedDeferred.promise })) + }) + } + + it('accepts a matching version and delivers no leftover when the bridge sent only the handshake', async () => { + const { accepted } = await startDaemon('0.1.0+match') + + const bridgeSock = connect(sockPath) + await new Promise((r) => bridgeSock.once('connect', () => r())) + + const acceptedCb = vi.fn<(leftover: Buffer) => void>() + runConnectHandshake(bridgeSock, '0.1.0+match', { onAccepted: acceptedCb }) + + const { leftover } = await accepted + expect(leftover.length).toBe(0) + + await vi.waitFor(() => expect(acceptedCb).toHaveBeenCalledTimes(1)) + expect(acceptedCb.mock.calls[0][0].length).toBe(0) + + bridgeSock.destroy() + }) + + it('preserves leftover bytes on the daemon side when an extra frame is coalesced after the handshake', async () => { + // Why: simulate an aggressive client that pipelines a frame immediately + // after the handshake. We bypass runConnectHandshake here and write the + // raw bytes directly so we control the coalescing behaviour. + const { accepted } = await startDaemon('0.1.0+match') + + const bridgeSock = connect(sockPath) + await new Promise((r) => bridgeSock.once('connect', () => r())) + + const handshakeFrame = encodeHandshakeFrame({ + type: 'orca-relay-handshake', + version: '0.1.0+match' + }) + const trailingPayload = encodeJsonRpcFrame({ jsonrpc: '2.0', method: 'noop', params: {} }, 1, 0) + bridgeSock.write(Buffer.concat([handshakeFrame, trailingPayload])) + + const { leftover } = await accepted + + const seen: DecodedFrame[] = [] + const dec = new FrameDecoder((f) => seen.push(f)) + dec.feed(leftover) + expect(seen).toHaveLength(1) + expect(seen[0].type).toBe(MessageType.Regular) + + bridgeSock.destroy() + }) + + it('preserves leftover bytes on the bridge side when the daemon coalesces handshake-ok + a JSON-RPC frame', async () => { + let serverHandshakeSeen = false + server = createServer((sock) => { + trackServerSocket(sock) + const decoder = new FrameDecoder((frame) => { + if (frame.type !== MessageType.Handshake || serverHandshakeSeen) { + return + } + serverHandshakeSeen = true + const ok = encodeHandshakeFrame({ + type: 'orca-relay-handshake-ok', + version: '0.1.0+match' + }) + const trailing = encodeJsonRpcFrame( + { jsonrpc: '2.0', method: 'pty.event', params: { evt: 'data' } }, + 7, + 1 + ) + sock.write(Buffer.concat([ok, trailing])) + }) + sock.on('data', (chunk: Buffer) => decoder.feed(chunk)) + }) + await new Promise((r) => server.listen(sockPath, () => r())) + + const bridgeSock = connect(sockPath) + await new Promise((r) => bridgeSock.once('connect', () => r())) + + const acceptedCb = vi.fn<(leftover: Buffer) => void>() + runConnectHandshake(bridgeSock, '0.1.0+match', { onAccepted: acceptedCb }) + + await vi.waitFor(() => expect(acceptedCb).toHaveBeenCalledTimes(1)) + const leftover = acceptedCb.mock.calls[0][0] + + const seen: DecodedFrame[] = [] + const dec = new FrameDecoder((f) => seen.push(f)) + dec.feed(leftover) + expect(seen).toHaveLength(1) + expect(seen[0].type).toBe(MessageType.Regular) + + bridgeSock.destroy() + }) + + it('exits with EXIT_CODE_VERSION_MISMATCH when the daemon reports a mismatch', async () => { + await startDaemon('0.1.0+server-version') + + const bridgeSock = connect(sockPath) + await new Promise((r) => bridgeSock.once('connect', () => r())) + + const acceptedCb = vi.fn() + runConnectHandshake(bridgeSock, '0.1.0+different', { onAccepted: acceptedCb }) + + await vi.waitFor(() => expect(exitSpy).toHaveBeenCalled()) + expect(exitSpy).toHaveBeenCalledWith(EXIT_CODE_VERSION_MISMATCH) + expect(acceptedCb).not.toHaveBeenCalled() + + bridgeSock.destroy() + }) + + it('does not call onAccepted before any handshake-ok frame arrives', async () => { + // Why: silent server that never replies. acceptedCb must stay + // un-invoked even though the bridge has flushed its handshake frame. + server = createServer((sock) => { + trackServerSocket(sock) + /* swallow */ + }) + await new Promise((r) => server.listen(sockPath, () => r())) + + const bridgeSock = connect(sockPath) + await new Promise((r) => bridgeSock.once('connect', () => r())) + + const acceptedCb = vi.fn() + runConnectHandshake(bridgeSock, '0.1.0+match', { onAccepted: acceptedCb }) + + await new Promise((r) => setTimeout(r, 100)) + expect(acceptedCb).not.toHaveBeenCalled() + + bridgeSock.destroy() + }) +}) diff --git a/src/relay/relay-handshake.ts b/src/relay/relay-handshake.ts new file mode 100644 index 000000000..653e03ddb --- /dev/null +++ b/src/relay/relay-handshake.ts @@ -0,0 +1,259 @@ +// Wire-level handshake helpers for the Orca relay. +// +// Why this lives in its own module: oxlint enforces a 300-line limit (with +// blanks/comments stripped) on .ts files, and relay.ts already runs near that +// limit. Splitting the version-handshake plumbing into a sibling module keeps +// relay.ts focused on the daemon-lifecycle wiring and makes the handshake +// independently unit-testable. + +import { dirname, join } from 'path' +import { existsSync, readFileSync, realpathSync } from 'fs' +import type { Socket } from 'net' +import { + RELAY_VERSION, + MessageType, + FrameDecoder, + encodeHandshakeFrame, + parseHandshakeMessage, + type DecodedFrame +} from './protocol' + +// Why: a unique exit code reserved for the wire-level version-mismatch terminal +// condition. The client (waitForSentinel + ssh.ts) maps this exit code to a +// non-retryable RelayVersionMismatchError so _onRelayLost skips the backoff +// loop. Any other non-zero exit is treated as a transient transport error. +export const EXIT_CODE_VERSION_MISMATCH = 42 + +// Why: the deploy step writes a content-hashed version marker (e.g. +// "0.1.0+0a5fe134d020") into ${remoteDir}/.version next to relay.js. Read it +// from the directory the running script lives in (NOT process.cwd()) so test +// spawns from arbitrary working dirs still report a coherent version. We +// resolve symlinks via realpathSync so a daemon launched indirectly (e.g. +// `node /tmp/symlink-to-relay.js`) still finds .version next to the real +// script. Falls back to bare RELAY_VERSION only if the file truly cannot be +// read; the wire handshake then refuses a fresh content-hashed client and +// the user gets a clean typed error rather than a silent stale-daemon loop. +export function readLaunchVersion(): string { + try { + const entry = process.argv[1] + let dir: string + if (entry) { + let resolved = entry + try { + resolved = realpathSync(entry) + } catch { + /* fall back to the unresolved path */ + } + dir = dirname(resolved) + } else { + dir = process.cwd() + } + const versionFile = join(dir, '.version') + if (existsSync(versionFile)) { + const v = readFileSync(versionFile, 'utf-8').trim() + if (v) { + return v + } + } + } catch { + /* fall through */ + } + return RELAY_VERSION +} + +// ── Daemon side ───────────────────────────────────────────────────── + +export type DaemonHandshakeCallbacks = { + // Why: leftover is any bytes the FrameDecoder buffered AFTER the handshake + // frame (e.g. the bridge wrote handshake + a JSON-RPC frame in the same + // TCP send). The caller MUST feed leftover into the dispatcher before + // attaching the new 'data' listener, otherwise those bytes are silently + // lost. + onAccepted: (sock: Socket, leftover: Buffer) => void + launchVersion: string +} + +// Why: pre-dispatcher version handshake. The daemon reads exactly one +// Handshake-typed frame off this freshly-accepted socket BEFORE the JSON-RPC +// dispatcher pipe is attached. Mismatch means the connecting bridge was +// launched against a different relay.js version than the daemon was; we close +// the socket so the bridge exits 42 and the client surfaces a typed error +// instead of looping over the dispatcher. +export function setupDaemonHandshake(sock: Socket, cb: DaemonHandshakeCallbacks): void { + let handshakeResolved = false + const decoder: FrameDecoder = new FrameDecoder( + (frame: DecodedFrame) => { + if (handshakeResolved) { + return + } + const accepted = handleDaemonHandshakeFrame(sock, frame, cb.launchVersion) + if (accepted) { + handshakeResolved = true + const leftover = decoder.drain() + detachHandshakeListener(sock) + cb.onAccepted(sock, leftover) + } + }, + (err) => { + process.stderr.write(`[relay] Handshake decode error: ${err.message}\n`) + sock.destroy() + } + ) + + const onHandshakeData = (chunk: Buffer): void => { + decoder.feed(chunk) + } + sock.on('data', onHandshakeData) + ;(sock as Socket & { __orcaOnHandshake?: typeof onHandshakeData }).__orcaOnHandshake = + onHandshakeData +} + +export function detachHandshakeListener(sock: Socket): void { + const tagged = sock as Socket & { __orcaOnHandshake?: (chunk: Buffer) => void } + if (tagged.__orcaOnHandshake) { + sock.removeListener('data', tagged.__orcaOnHandshake) + delete tagged.__orcaOnHandshake + } +} + +function handleDaemonHandshakeFrame( + sock: Socket, + frame: DecodedFrame, + launchVersion: string +): boolean { + if (frame.type !== MessageType.Handshake) { + process.stderr.write( + `[relay] Protocol violation pre-handshake: type=${frame.type}; closing socket\n` + ) + sock.destroy() + return false + } + let msg: ReturnType + try { + msg = parseHandshakeMessage(frame.payload) + } catch (err) { + process.stderr.write( + `[relay] Could not parse handshake: ${(err as Error).message}; closing socket\n` + ) + sock.destroy() + return false + } + if (msg.type !== 'orca-relay-handshake') { + process.stderr.write( + `[relay] Unexpected handshake type from client: ${msg.type}; closing socket\n` + ) + sock.destroy() + return false + } + if (msg.version !== launchVersion) { + process.stderr.write( + `[relay] Handshake mismatch: own=${launchVersion}, client=${msg.version}; closing socket\n` + ) + try { + sock.write( + encodeHandshakeFrame({ + type: 'orca-relay-handshake-mismatch', + expected: launchVersion, + got: msg.version + }) + ) + } catch { + /* best-effort — close+exit-42 still wins */ + } + sock.end() + return false + } + process.stderr.write(`[relay] Handshake OK from version=${msg.version}\n`) + sock.write(encodeHandshakeFrame({ type: 'orca-relay-handshake-ok', version: launchVersion })) + return true +} + +// ── --connect side ────────────────────────────────────────────────── + +export type ConnectHandshakeCallbacks = { + // Why: leftover is any bytes the FrameDecoder buffered AFTER the + // handshake-ok frame. The caller MUST forward leftover to process.stdout + // (the SSH stdout pipe) before attaching the raw bridge, otherwise daemon + // bytes coalesced into the same TCP send as handshake-ok are silently + // dropped. + onAccepted: (leftover: Buffer) => void +} + +// Why: the wire-level version handshake from the bridge side. Before we attach +// the bidirectional pipe (and before we write RELAY_SENTINEL to stdout to +// unblock the client), we send a Handshake-typed frame carrying our version +// and wait for the daemon's Handshake response. This is defense-in-depth on +// top of the versioned-install-dir layout: a corrupt/missing .version, hash +// collision, or legacy-fallback path would otherwise let a v2 bridge drive a +// v1 daemon. VS Code's remoteExtensionHostAgentServer.ts:340 does the same +// check. +export function runConnectHandshake( + sock: Socket, + myVersion: string, + cb: ConnectHandshakeCallbacks +): void { + let handshakeDone = false + + const decoder: FrameDecoder = new FrameDecoder( + (frame: DecodedFrame) => { + if (handshakeDone) { + return + } + if (frame.type !== MessageType.Handshake) { + process.stderr.write( + `[relay-connect] Protocol violation: expected Handshake frame, got type=${frame.type}\n` + ) + sock.destroy() + process.exit(1) + } + let msg: ReturnType + try { + msg = parseHandshakeMessage(frame.payload) + } catch (err) { + process.stderr.write( + `[relay-connect] Could not parse handshake reply: ${(err as Error).message}\n` + ) + sock.destroy() + process.exit(1) + } + if (msg.type === 'orca-relay-handshake-ok') { + process.stderr.write(`[relay-connect] Handshake OK at version=${msg.version}\n`) + handshakeDone = true + const leftover = decoder.drain() + sock.removeAllListeners('data') + cb.onAccepted(leftover) + return + } + if (msg.type === 'orca-relay-handshake-mismatch') { + // Why: explicit stderr flush + exit so the diagnostic line is + // delivered to the client BEFORE the process exits. Without this, + // process.stderr writes can be buffered/async on pipe transports + // and parseHandshakeMismatchStderr loses the version detail. + process.stderr.write( + `[relay-connect] Handshake mismatch: expected=${msg.expected}, daemon=${msg.got}; exiting ${EXIT_CODE_VERSION_MISMATCH}\n`, + () => { + sock.destroy() + process.exit(EXIT_CODE_VERSION_MISMATCH) + } + ) + return + } + process.stderr.write(`[relay-connect] Unexpected handshake type: ${msg.type}\n`) + sock.destroy() + process.exit(1) + }, + (err) => { + process.stderr.write(`[relay-connect] Handshake decode error: ${err.message}\n`) + sock.destroy() + process.exit(1) + } + ) + + sock.on('data', (chunk: Buffer) => { + if (!handshakeDone) { + decoder.feed(chunk) + } + }) + + sock.write(encodeHandshakeFrame({ type: 'orca-relay-handshake', version: myVersion })) +} diff --git a/src/relay/relay.ts b/src/relay/relay.ts index ca306d2c9..3df1555ee 100644 --- a/src/relay/relay.ts +++ b/src/relay/relay.ts @@ -15,6 +15,7 @@ import { homedir } from 'os' import { resolve, join } from 'path' import { unlinkSync, existsSync } from 'fs' import { RELAY_SENTINEL } from './protocol' +import { readLaunchVersion, runConnectHandshake, setupDaemonHandshake } from './relay-handshake' import { RelayDispatcher } from './dispatcher' import { RelayContext } from './context' import { PtyHandler } from './pty-handler' @@ -68,6 +69,7 @@ function parseArgs(argv: string[]): { // that owns the PTY sessions. function runConnectMode(sockPath: string): void { + const myVersion = readLaunchVersion() const sock = createConnection({ path: sockPath }) const connectTimeout = setTimeout(() => { @@ -78,13 +80,27 @@ function runConnectMode(sockPath: string): void { sock.on('connect', () => { clearTimeout(connectTimeout) - // Why: the client-side waitForSentinel expects this exact string - // before it starts sending framed data. Emitting it here lets the - // deploy code use the same sentinel-detection path for both fresh - // launches and reconnects. - process.stdout.write(RELAY_SENTINEL) - process.stdin.pipe(sock) - sock.pipe(process.stdout) + runConnectHandshake(sock, myVersion, { + onAccepted: (leftover: Buffer) => { + // Why: RELAY_SENTINEL must be written AFTER the handshake passes; if it + // were written earlier, waitForSentinel on the client would resolve + // and start sending JSON-RPC over a socket the daemon was about to + // close on mismatch — surfacing as a generic channel drop and + // re-entering the backoff loop. Sequencing it post-handshake makes + // mismatch a clean exit-42 path with no false-positive sentinel. + process.stdout.write(RELAY_SENTINEL) + // Why: bytes that arrived in the same TCP send as the handshake-ok + // frame were buffered inside the handshake's FrameDecoder. Forward + // them to stdout BEFORE attaching sock.pipe(process.stdout), so the + // multiplexer downstream sees them in order and no daemon frames + // are silently dropped at the transition. + if (leftover.length > 0) { + process.stdout.write(leftover) + } + process.stdin.pipe(sock) + sock.pipe(process.stdout) + } + }) }) // Why: when the SSH channel closes, stdout becomes a broken pipe. @@ -210,77 +226,81 @@ function main(): void { let activeSocket: Socket | null = null let socketServer: Server | null = null + const launchVersion = readLaunchVersion() + + function attachAcceptedSocket(sock: Socket, leftover: Buffer): void { + // Why: only one client at a time. If a second reconnect arrives (e.g. + // user restarts again quickly), close the stale bridge so the new one + // takes over cleanly. We null activeSocket BEFORE destroying so the old + // socket's close handler sees it's been replaced and skips starting the + // grace timer. + if (activeSocket) { + process.stderr.write('[relay] Replacing existing socket client with new connection\n') + const replaced = activeSocket + activeSocket = null + replaced.destroy() + } + activeSocket = sock + + // Why: stdin's data listener is still registered from the initial + // connection. If the old SSH channel hasn't fully closed yet (TCP FIN + // delayed), buffered stdin data would interleave with the new socket + // client's frames, corrupting the frame decoder. + process.stdin.pause() + process.stdin.removeAllListeners('data') + + ptyHandler.cancelGraceTimer() + + dispatcher.setWrite((data) => { + if (!sock.destroyed) { + sock.write(data) + } + }) + + // Why: bytes that arrived in the same TCP send as the handshake frame + // were buffered inside the handshake's FrameDecoder. Feed them into the + // dispatcher BEFORE wiring sock.on('data'), so frame ordering is + // preserved and no client data is silently dropped at the transition. + if (leftover.length > 0) { + dispatcher.feed(leftover) + } + + sock.on('data', (chunk: Buffer) => { + if (activeSocket !== sock) { + return + } + ptyHandler.cancelGraceTimer() + dispatcher.feed(chunk) + }) + } function startSocketServer(): Server { cleanupSocket(sockPath) const server = createServer((sock) => { - // Why: only one client at a time. If a second reconnect arrives - // (e.g. user restarts again quickly), close the stale bridge so the - // new one takes over cleanly. We null activeSocket BEFORE destroying - // so the old socket's close handler sees it's been replaced and - // skips starting the grace timer. - if (activeSocket) { - process.stderr.write('[relay] Replacing existing socket client with new connection\n') - const replaced = activeSocket - activeSocket = null - replaced.destroy() - } - activeSocket = sock + // Why: pre-dispatcher version handshake — see relay-handshake.ts. + setupDaemonHandshake(sock, { launchVersion, onAccepted: attachAcceptedSocket }) - // Why: stdin's data listener is still registered from the initial - // connection. If the old SSH channel hasn't fully closed yet (TCP - // FIN delayed), buffered stdin data would interleave with the new - // socket client's frames, corrupting the frame decoder. - process.stdin.pause() - process.stdin.removeAllListeners('data') - - ptyHandler.cancelGraceTimer() - - dispatcher.setWrite((data) => { - if (!sock.destroyed) { - sock.write(data) - } - }) - - sock.on('data', (chunk: Buffer) => { - if (activeSocket !== sock) { - return - } - ptyHandler.cancelGraceTimer() - dispatcher.feed(chunk) - }) - - // Why: when the --connect bridge's SSH channel dies, stdin.pipe(sock) - // calls sock.end(), sending FIN to the relay. Without this handler - // the relay-side socket stays half-open — the relay keeps writing - // pty.data frames that the bridge can no longer forward, silently - // dropping output until the next --connect replaces the socket. - // Destroying on 'end' ensures the 'close' handler fires promptly. + // Why: when --connect's SSH channel dies, stdin.pipe(sock) calls + // sock.end(), sending FIN to the relay. Destroying on 'end' ensures + // the 'close' handler fires promptly so the daemon can enter grace. sock.on('end', () => { if (!sock.destroyed) { sock.destroy() } }) + sock.on('error', () => { + // Why: Node emits 'error' then 'close'. The close handler owns + // activeSocket cleanup and grace startup. + }) + sock.on('close', () => { - // Why: only start the grace timer if THIS socket is still the - // active one. If it was replaced by a newer connection (see - // above), activeSocket was already nulled and reassigned — starting - // the grace timer here would incorrectly begin shutdown while a - // live client is connected. if (activeSocket === sock) { activeSocket = null dispatcher.invalidateClient() startGrace() } }) - - sock.on('error', () => { - // Why: Node emits 'error' then 'close'. The close handler owns - // activeSocket cleanup and grace startup; clearing activeSocket here - // would make close skip the grace timer and leave the relay alive - // indefinitely with no client. - }) }) // Why: setting umask to 0o177 BEFORE listen ensures the socket is