From 9fdc42014b15fbf062f2f3eae6dacbbfe94a99a4 Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Tue, 18 Aug 2026 02:24:25 -0400 Subject: [PATCH] test(ssh): stop the live-relay harness from leaking PTY children MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Killing a relay on its own reparents its PTY shells to init — the leak these tests are about, reproduced on the machine running them. Terminate the daemon first so it disposes its PTYs, then kill whatever survives, deepest first. Also waits for the replacement daemon to actually exit instead of sleeping, and consumes whole lines until the relay sentinel rather than assuming the first one. --- src/main/ssh/ssh-relay-live-daemon-harness.ts | 65 +++++++++++++++++-- ...ssh-relay-socket-owner.integration.test.ts | 14 ++-- 2 files changed, 68 insertions(+), 11 deletions(-) diff --git a/src/main/ssh/ssh-relay-live-daemon-harness.ts b/src/main/ssh/ssh-relay-live-daemon-harness.ts index dea87d8e2a1..b8e8df6da43 100644 --- a/src/main/ssh/ssh-relay-live-daemon-harness.ts +++ b/src/main/ssh/ssh-relay-live-daemon-harness.ts @@ -3,7 +3,7 @@ // // Needs `pnpm build:relay` — callers skip when relayBundleDirForHost() is null. -import { spawn, type ChildProcess } from 'node:child_process' +import { spawn, spawnSync, type ChildProcess } from 'node:child_process' import { randomBytes } from 'node:crypto' import { EventEmitter } from 'node:events' import { cpSync, existsSync, mkdirSync, symlinkSync, writeFileSync } from 'node:fs' @@ -32,6 +32,51 @@ export function isProcessAlive(pid: number): boolean { } } +/** + * SIGKILL a process and every descendant, deepest first. + * + * Why: a relay killed on its own leaves its PTY shells reparented to init — the very + * leak these tests are about, reproduced on the machine running them. + */ +export function killProcessTree(pid: number): void { + const listing = spawnSync('ps', ['-eo', 'pid=,ppid=']).stdout?.toString() ?? '' + const childrenByParent = new Map() + for (const line of listing.split('\n')) { + const [child, parent] = line.trim().split(/\s+/).map(Number) + if (Number.isInteger(child) && Number.isInteger(parent)) { + childrenByParent.set(parent, [...(childrenByParent.get(parent) ?? []), child]) + } + } + const ordered: number[] = [] + const visit = (current: number): void => { + for (const child of childrenByParent.get(current) ?? []) { + visit(child) + } + ordered.push(current) + } + visit(pid) + for (const target of ordered) { + try { + process.kill(target, 'SIGKILL') + } catch { + /* already gone */ + } + } +} + +export function waitForExit(child: ChildProcess, timeoutMs = 20_000): Promise { + if (child.exitCode !== null) { + return Promise.resolve(child.exitCode) + } + return new Promise((resolve, reject) => { + const timer = setTimeout(() => reject(new Error('relay daemon never exited')), timeoutMs) + child.once('exit', (code) => { + clearTimeout(timer) + resolve(code) + }) + }) +} + export class LiveRelayFixture { readonly sockPath: string readonly credentialFile: string @@ -112,7 +157,8 @@ export class LiveRelayFixture { return bridge } - dispose(): void { + /** Ask each daemon to shut down (which disposes its PTYs), then kill whatever is left. */ + async dispose(): Promise { for (const child of this.children) { try { child.kill('SIGKILL') @@ -122,11 +168,17 @@ export class LiveRelayFixture { } for (const pid of this.daemonPids) { try { - process.kill(pid, 'SIGKILL') + process.kill(pid, 'SIGTERM') } catch { /* already gone */ } } + for (let attempt = 0; attempt < 30 && [...this.daemonPids].some(isProcessAlive); attempt++) { + await delay(100) + } + for (const pid of this.daemonPids) { + killProcessTree(pid) + } } } @@ -160,13 +212,16 @@ export class RelayBridge { private ingest(chunk: Buffer): void { this.buffer = Buffer.concat([this.buffer, chunk]) - if (!this.sentinelSeen) { + while (!this.sentinelSeen) { const newline = this.buffer.indexOf('\n') if (newline === -1) { return } - this.sentinelSeen = true + // Why: consume whole lines until the sentinel; a stray pre-sentinel line would + // otherwise be mistaken for it and desynchronise every frame after it. + const line = this.buffer.subarray(0, newline).toString('utf-8') this.buffer = this.buffer.subarray(newline + 1) + this.sentinelSeen = line.includes('ORCA-RELAY') } while (this.buffer.length >= FRAME_HEADER_BYTES) { const length = this.buffer.readUInt32BE(9) diff --git a/src/main/ssh/ssh-relay-socket-owner.integration.test.ts b/src/main/ssh/ssh-relay-socket-owner.integration.test.ts index 3e518efe3d1..eef231ca3ca 100644 --- a/src/main/ssh/ssh-relay-socket-owner.integration.test.ts +++ b/src/main/ssh/ssh-relay-socket-owner.integration.test.ts @@ -21,7 +21,9 @@ import { createLocalShellConnection, delay, isProcessAlive, - relayBundleDirForHost + killProcessTree, + relayBundleDirForHost, + waitForExit } from './ssh-relay-live-daemon-harness' import { isRelaySocketOwnerLiveError, @@ -42,8 +44,8 @@ describe.skipIf(!BUNDLE_DIR || process.platform === 'win32')('relay socket owner let fixture: LiveRelayFixture | null = null let root: string | null = null - afterEach(() => { - fixture?.dispose() + afterEach(async () => { + await fixture?.dispose() fixture = null if (root) { rmSync(root, { recursive: true, force: true }) @@ -77,7 +79,7 @@ describe.skipIf(!BUNDLE_DIR || process.platform === 'win32')('relay socket owner await expect(probeRelaySocketOwner(conn, process.execPath, live.sockPath)).resolves.toBe('live') // A crashed relay leaves the inode behind; that, and only that, is a stale socket. - process.kill(pid, 'SIGKILL') + killProcessTree(pid) await delay(500) expect(existsSync(live.sockPath)).toBe(true) await expect(probeRelaySocketOwner(conn, process.execPath, live.sockPath)).resolves.toBe( @@ -98,8 +100,8 @@ describe.skipIf(!BUNDLE_DIR || process.platform === 'win32')('relay socket owner // The socket kept its owner, so a fresh daemon hits EADDRINUSE, finds a live // listener, and refuses to start — the interlock the unconditional rm -f defeated. - live.launchDaemon('replacement') - await delay(2_000) + const replacement = live.launchDaemon('replacement') + expect(await waitForExit(replacement)).not.toBe(0) const bridge = live.openBridge() await bridge.waitForSentinel() const status = await bridge.request('relay.status')