diff --git a/src/main/ssh/ssh-connection.test.ts b/src/main/ssh/ssh-connection.test.ts index 552e6d66ace..29d89f071d8 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 fc19c622b1c..4e8e88a4239 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()