mirror of
https://github.com/stablyai/orca.git
synced 2026-10-14 00:02:42 +00:00
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 <help@stably.ai>
This commit is contained in:
@@ -1,11 +1,26 @@
|
||||
import { describe, expect, it, vi, beforeEach } from 'vitest'
|
||||
import { Socket } from 'net'
|
||||
|
||||
let eventHandlers: Map<string, (...args: unknown[]) => void>
|
||||
let connectBehavior: 'ready' | 'error' = 'ready'
|
||||
let connectErrorMessage = ''
|
||||
|
||||
vi.mock('ssh2', () => ({
|
||||
Client: class MockSshClient {
|
||||
type MockSshClient = {
|
||||
setNoDelay: ReturnType<typeof vi.fn>
|
||||
_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<void>
|
||||
}
|
||||
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 () => {
|
||||
|
||||
@@ -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()
|
||||
|
||||
Reference in New Issue
Block a user