From 4676e182c2ade8e6156bce88fb4abd115650d6ee Mon Sep 17 00:00:00 2001 From: Ron Leizrowice Date: Fri, 28 Aug 2026 14:09:26 -0400 Subject: [PATCH 1/2] fix(cli): let srt exit after the command when --control-fd is a pipe srt --control-fd read the control fd with fs.createReadStream, which parks a threadpool thread in a blocking read(2); process.exit() on the wrapped command's exit then waited for that thread, so srt stayed alive until the parent closed the fd or killed it. A pipe or socket fd is now read through a net.Socket, driven by the event loop and unref'd so it never keeps srt alive; a regular file keeps the fs stream. The --control-fd tests waited out that hang with a 2 s timeout and a SIGKILL, and the next test's spawn could then race Bun's asynchronous teardown of the fd-3 socket ("Failed to connect"), a flake seen on Linux CI. They now wait for srt to exit on its own and assert exit code 0, so the hang is a failing test rather than a timeout. --- src/cli.ts | 22 +++++++++-- test/control-fd.test.ts | 82 ++++++++++++++++++++++------------------- 2 files changed, 62 insertions(+), 42 deletions(-) diff --git a/src/cli.ts b/src/cli.ts index 5d65450a0..348c67a9b 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -8,6 +8,7 @@ import { logForDebugging } from './utils/debug.js' import { loadConfig, loadConfigFromString } from './utils/config-loader.js' import * as readline from 'readline' import * as fs from 'fs' +import * as net from 'net' import * as path from 'path' import * as os from 'os' @@ -36,6 +37,22 @@ function getDefaultConfig(): SandboxRuntimeConfig { } } +/** + * A readable stream over the control fd. A pipe or socket is read through a + * libuv stream handle, driven by the event loop; fs.createReadStream would + * park a threadpool thread in a blocking read(2) that process.exit() then + * waits for, so srt would outlive the wrapped command until the parent + * closed the fd. A regular file has no such wait and keeps the fs stream. + * Either way the fd never keeps srt alive on its own. + */ +function openControlFd(fd: number): NodeJS.ReadableStream { + const stat = fs.fstatSync(fd) + if (stat.isFIFO() || stat.isSocket()) { + return new net.Socket({ fd, readable: true, writable: false }).unref() + } + return fs.createReadStream('', { fd }) +} + async function main(): Promise { const program = new Command() @@ -215,11 +232,8 @@ async function main(): Promise { let controlReader: readline.Interface | null = null if (options.controlFd !== undefined) { try { - const controlStream = fs.createReadStream('', { - fd: options.controlFd, - }) controlReader = readline.createInterface({ - input: controlStream, + input: openControlFd(options.controlFd), crlfDelay: Infinity, }) diff --git a/test/control-fd.test.ts b/test/control-fd.test.ts index 00e45aca8..1ef949ad6 100644 --- a/test/control-fd.test.ts +++ b/test/control-fd.test.ts @@ -8,6 +8,27 @@ import { type Writable } from 'stream' // Get the path to the built CLI const CLI_PATH = path.join(process.cwd(), 'dist', 'cli.js') +// srt is expected to exit on its own shortly after the wrapped command +// (which runs for well under a second) finishes; a hang is a failure, not +// something to wait out. +function waitForExit(child: ChildProcess): Promise { + return new Promise((resolve, reject) => { + const timer = setTimeout( + () => + reject(new Error('srt did not exit within 2s of the wrapped command')), + 2000, + ) + child.on('exit', code => { + clearTimeout(timer) + resolve(code) + }) + child.on('error', err => { + clearTimeout(timer) + reject(err) + }) + }) +} + describe('--control-fd', () => { let tmpDir: string let child: ChildProcess | null = null @@ -21,14 +42,12 @@ describe('--control-fd', () => { child.kill('SIGKILL') } fs.rmSync(tmpDir, { recursive: true, force: true }) - // Bun's node:child_process shim implements extra stdio 'pipe' entries - // (fd 3 here) via a unix socket, and tears that socket down - // asynchronously after the child exits. The tests above all hit the - // 2000ms safety timeout and SIGKILL their child, so on a fast runner - // the next test's spawn can race that teardown and Bun's - // #createStdioObject throws `Failed to connect` (connect ENOENT) — - // observed on linux/arm64. Yield briefly so the prior child's stdio - // cleanup settles before the next spawn. + // Every test waits for srt to exit on its own; the SIGKILL above only + // runs when one has already failed. Bun's node:child_process shim + // implements extra stdio 'pipe' entries (fd 3 here) via a unix socket + // torn down asynchronously after the child exits, and a spawn that + // races that teardown throws `Failed to connect` (connect ENOENT), so + // yield briefly before the next test's spawn either way. await new Promise(r => setTimeout(r, 50)) }) @@ -71,12 +90,10 @@ describe('--control-fd', () => { }) controlFd.write(configUpdate + '\n') - // Wait for process to complete - await new Promise((resolve, reject) => { - child!.on('exit', () => resolve()) - child!.on('error', reject) - setTimeout(() => resolve(), 2000) // Timeout safety - }) + // srt must exit by itself once the wrapped command finishes, with the + // control fd still open on our side. + const exitCode = await waitForExit(child) + expect(exitCode).toBe(0) // Check that config was updated - look for debug output const allStderr = stderr.join('') @@ -110,12 +127,10 @@ describe('--control-fd', () => { const controlFd = child.stdio[3] as Writable controlFd.write('{ invalid json }\n') - // Wait for process to complete - await new Promise((resolve, reject) => { - child!.on('exit', () => resolve()) - child!.on('error', reject) - setTimeout(() => resolve(), 2000) // Timeout safety - }) + // srt must exit by itself once the wrapped command finishes, with the + // control fd still open on our side. + const exitCode = await waitForExit(child) + expect(exitCode).toBe(0) // Process should still complete successfully const allStdout = stdout.join('') @@ -146,12 +161,10 @@ describe('--control-fd', () => { controlFd.write(' \n') controlFd.write('\t\n') - // Wait for process to complete - await new Promise((resolve, reject) => { - child!.on('exit', () => resolve()) - child!.on('error', reject) - setTimeout(() => resolve(), 2000) // Timeout safety - }) + // srt must exit by itself once the wrapped command finishes, with the + // control fd still open on our side. + const exitCode = await waitForExit(child) + expect(exitCode).toBe(0) // Process should still complete successfully const allStdout = stdout.join('') @@ -175,12 +188,7 @@ describe('--control-fd', () => { stdout.push(data.toString()) }) - // Wait for process to complete - const exitCode = await new Promise((resolve, reject) => { - child!.on('exit', code => resolve(code)) - child!.on('error', reject) - setTimeout(() => resolve(null), 2000) // Timeout safety - }) + const exitCode = await waitForExit(child) expect(exitCode).toBe(0) const allStdout = stdout.join('') @@ -211,12 +219,10 @@ describe('--control-fd', () => { const stdin = child.stdin as Writable stdin.write('hello from stdin\n') - // Wait for process to complete - await new Promise((resolve, reject) => { - child!.on('exit', () => resolve()) - child!.on('error', reject) - setTimeout(() => resolve(), 2000) // Timeout safety - }) + // srt must exit by itself once the wrapped command finishes, with the + // control fd still open on our side. + const exitCode = await waitForExit(child) + expect(exitCode).toBe(0) const allStdout = stdout.join('') expect(allStdout).toContain('GOT: hello from stdin') From 1a0db8cccac5af0fde96a42c6cf4962f66ddce6f Mon Sep 17 00:00:00 2001 From: Ron Leizrowice Date: Sat, 29 Aug 2026 02:35:54 -0400 Subject: [PATCH 2/2] fix(cli): keep the fs stream for the control fd under Bun and for datagram sockets net.Socket({ fd }) reads nothing under Bun, whose fs stream does not hold exit, and throws for a socket libuv cannot adopt as a stream; both keep the fs stream. The tests subscribe to srt's exit before the settle sleep, cap the wait at 4.5 s, assert the update was applied rather than echoed, and add FIFO, regular-file and Bun-runtime cases. --- src/cli.ts | 26 +++- test/control-fd.test.ts | 330 ++++++++++++++++++++++------------------ 2 files changed, 205 insertions(+), 151 deletions(-) diff --git a/src/cli.ts b/src/cli.ts index 348c67a9b..f617cacb3 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -39,16 +39,32 @@ function getDefaultConfig(): SandboxRuntimeConfig { /** * A readable stream over the control fd. A pipe or socket is read through a - * libuv stream handle, driven by the event loop; fs.createReadStream would + * libuv stream handle, driven by the event loop: fs.createReadStream would * park a threadpool thread in a blocking read(2) that process.exit() then * waits for, so srt would outlive the wrapped command until the parent - * closed the fd. A regular file has no such wait and keeps the fs stream. - * Either way the fd never keeps srt alive on its own. + * closed the fd. Anything else keeps the fs stream, which is right for a + * regular file (its reads never block) and leaves a tty with the old wait. + * Under Bun, net.Socket({ fd }) reads nothing, and Bun's fs stream does not + * hold exit, so the fs stream serves every fd kind there. + * + * A libuv handle switches the fd's open file description to non-blocking + * mode, from the moment srt opens it and for good (Node restores only fds + * 0-2 at exit). A parent that shares that description (a shell + * `exec 3 { return new Promise((resolve, reject) => { + if (child.exitCode !== null || child.signalCode !== null) { + resolve(child.exitCode) + return + } const timer = setTimeout( () => - reject(new Error('srt did not exit within 2s of the wrapped command')), - 2000, + reject( + new Error( + `srt did not exit within ${EXIT_TIMEOUT_MS}ms of the wrapped command`, + ), + ), + EXIT_TIMEOUT_MS, ) child.on('exit', code => { clearTimeout(timer) @@ -29,202 +41,228 @@ function waitForExit(child: ChildProcess): Promise { }) } +// srt's own runtime is node (the bin shebang); under `bun test` the same +// dist is exercised through bun as well, whose net.Socket({ fd }) reads +// nothing and whose fs stream does not hold exit — the reason the CLI +// gates on the runtime. +const RUNTIMES: Array<{ name: string; bin: string }> = [ + { name: 'node', bin: 'node' }, + ...(process.versions.bun ? [{ name: 'bun', bin: process.execPath }] : []), +] + +// One config update, with a domain the debug log will echo back. +const CONFIG_UPDATE = JSON.stringify({ + network: { allowedDomains: ['updated-domain.com'], deniedDomains: [] }, + filesystem: { denyRead: [], allowWrite: [], denyWrite: [] }, +}) + describe('--control-fd', () => { let tmpDir: string let child: ChildProcess | null = null - - beforeEach(() => { - tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'control-fd-test-')) - }) - - afterEach(async () => { - if (child && !child.killed) { - child.kill('SIGKILL') - } - fs.rmSync(tmpDir, { recursive: true, force: true }) - // Every test waits for srt to exit on its own; the SIGKILL above only - // runs when one has already failed. Bun's node:child_process shim - // implements extra stdio 'pipe' entries (fd 3 here) via a unix socket - // torn down asynchronously after the child exits, and a spawn that - // races that teardown throws `Failed to connect` (connect ENOENT), so - // yield briefly before the next test's spawn either way. - await new Promise(r => setTimeout(r, 50)) - }) - - it('should update config when receiving valid JSON on control fd', async () => { - // Create a test script that outputs the current network config - // We'll use the debug output to verify config was updated - const testScript = path.join(tmpDir, 'test.sh') - fs.writeFileSync(testScript, '#!/bin/bash\nsleep 0.3\necho "DONE"\n', { - mode: 0o755, - }) - - // Spawn srt with --control-fd 3, passing fd 3 as a pipe - child = spawn( - 'node', - [CLI_PATH, '--debug', '--control-fd', '3', '--', testScript], - { - stdio: ['inherit', 'pipe', 'pipe', 'pipe'], - env: { ...process.env, SRT_DEBUG: 'true' }, - }, - ) - + // Subscribed right after spawn so an srt that dies during the settle + // sleep below is reported by its real exit, not as a hang. + let exited: Promise | null = null + // fds this side keeps open for the child (a FIFO writer, a file); closed + // after each test. + let heldFds: number[] = [] + + // Spawn srt and start watching for its exit before anything else happens. + function spawnSrt( + args: string[], + stdio: Array<'inherit' | 'pipe' | number>, + env?: NodeJS.ProcessEnv, + runtime = 'node', + ): { stdout: string[]; stderr: string[] } { + child = spawn(runtime, [CLI_PATH, ...args], { stdio, env }) + exited = waitForExit(child) + // Attached later; a rejection before then must not surface as unhandled. + exited.catch(() => {}) const stdout: string[] = [] const stderr: string[] = [] - child.stdout?.on('data', (data: Buffer) => { stdout.push(data.toString()) }) - child.stderr?.on('data', (data: Buffer) => { stderr.push(data.toString()) }) + return { stdout, stderr } + } - // Wait a bit for srt to initialize, then send a config update - await new Promise(r => setTimeout(r, 100)) + function writeScript(body: string): string { + const testScript = path.join(tmpDir, 'test.sh') + fs.writeFileSync(testScript, `#!/bin/bash\n${body}\n`, { mode: 0o755 }) + return testScript + } - const controlFd = child.stdio[3] as Writable - const configUpdate = JSON.stringify({ - network: { allowedDomains: ['updated-domain.com'], deniedDomains: [] }, - filesystem: { denyRead: [], allowWrite: [], denyWrite: [] }, - }) - controlFd.write(configUpdate + '\n') + // Let srt initialize before the control channel is used. + const settle = () => new Promise(r => setTimeout(r, 100)) - // srt must exit by itself once the wrapped command finishes, with the - // control fd still open on our side. - const exitCode = await waitForExit(child) - expect(exitCode).toBe(0) + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'control-fd-test-')) + }) - // Check that config was updated - look for debug output - const allStderr = stderr.join('') - expect(allStderr).toContain('updated-domain.com') + afterEach(async () => { + // A no-op once srt has exited on its own, which every test waits for; + // it only bites when a test already failed with srt still running. + if (child && child.exitCode === null && child.signalCode === null) { + child.kill('SIGKILL') + } + for (const fd of heldFds) fs.closeSync(fd) + heldFds = [] + fs.rmSync(tmpDir, { recursive: true, force: true }) + // Bun's node:child_process shim implements extra stdio 'pipe' entries + // (fd 3 here) via a unix socket torn down asynchronously after the + // child exits, and a spawn that races that teardown throws `Failed to + // connect` (connect ENOENT), so yield briefly before the next test's + // spawn either way. + await new Promise(r => setTimeout(r, 50)) }) - it('should ignore invalid JSON on control fd and continue running', async () => { - const testScript = path.join(tmpDir, 'test.sh') - fs.writeFileSync(testScript, '#!/bin/bash\nsleep 0.3\necho "COMPLETED"\n', { - mode: 0o755, + for (const runtime of RUNTIMES) { + it(`should update config when receiving valid JSON on control fd (${runtime.name})`, async () => { + // Verify through the debug output that the update was applied. + const testScript = writeScript('sleep 0.3\necho "DONE"') + + // Spawn srt with --control-fd 3, passing fd 3 as a pipe + const { stderr } = spawnSrt( + ['--debug', '--control-fd', '3', '--', testScript], + ['inherit', 'pipe', 'pipe', 'pipe'], + { ...process.env, SRT_DEBUG: 'true' }, + runtime.bin, + ) + + await settle() + + const controlFd = child!.stdio[3] as Writable + controlFd.write(CONFIG_UPDATE + '\n') + + // srt must exit by itself once the wrapped command finishes, with the + // control fd still open on our side. + expect(await exited).toBe(0) + + // Applied, not rejected: the rejection path logs the raw line too, so + // the domain alone would not tell the two apart. + const allStderr = stderr.join('') + expect(allStderr).toContain('Config updated from control fd') + expect(allStderr).toContain('updated-domain.com') + expect(allStderr).not.toContain('Invalid config on control fd') }) + } - child = spawn( - 'node', - [CLI_PATH, '--debug', '--control-fd', '3', '--', testScript], - { - stdio: ['inherit', 'pipe', 'pipe', 'pipe'], - env: { ...process.env, SRT_DEBUG: 'true' }, - }, - ) - - const stdout: string[] = [] + it('should ignore invalid JSON on control fd and continue running', async () => { + const testScript = writeScript('sleep 0.3\necho "COMPLETED"') - child.stdout?.on('data', (data: Buffer) => { - stdout.push(data.toString()) - }) + const { stdout } = spawnSrt( + ['--debug', '--control-fd', '3', '--', testScript], + ['inherit', 'pipe', 'pipe', 'pipe'], + { ...process.env, SRT_DEBUG: 'true' }, + ) - // Wait a bit for srt to initialize, then send invalid JSON - await new Promise(r => setTimeout(r, 100)) + await settle() - const controlFd = child.stdio[3] as Writable + const controlFd = child!.stdio[3] as Writable controlFd.write('{ invalid json }\n') - // srt must exit by itself once the wrapped command finishes, with the - // control fd still open on our side. - const exitCode = await waitForExit(child) - expect(exitCode).toBe(0) + expect(await exited).toBe(0) // Process should still complete successfully - const allStdout = stdout.join('') - expect(allStdout).toContain('COMPLETED') + expect(stdout.join('')).toContain('COMPLETED') }) it('should ignore empty lines on control fd', async () => { - const testScript = path.join(tmpDir, 'test.sh') - fs.writeFileSync(testScript, '#!/bin/bash\nsleep 0.3\necho "DONE"\n', { - mode: 0o755, - }) - - child = spawn('node', [CLI_PATH, '--control-fd', '3', '--', testScript], { - stdio: ['inherit', 'pipe', 'pipe', 'pipe'], - }) - - const stdout: string[] = [] + const testScript = writeScript('sleep 0.3\necho "DONE"') - child.stdout?.on('data', (data: Buffer) => { - stdout.push(data.toString()) - }) + const { stdout } = spawnSrt( + ['--control-fd', '3', '--', testScript], + ['inherit', 'pipe', 'pipe', 'pipe'], + ) - // Wait a bit for srt to initialize, then send empty lines - await new Promise(r => setTimeout(r, 100)) + await settle() - const controlFd = child.stdio[3] as Writable + const controlFd = child!.stdio[3] as Writable controlFd.write('\n') controlFd.write(' \n') controlFd.write('\t\n') - // srt must exit by itself once the wrapped command finishes, with the - // control fd still open on our side. - const exitCode = await waitForExit(child) - expect(exitCode).toBe(0) + expect(await exited).toBe(0) // Process should still complete successfully - const allStdout = stdout.join('') - expect(allStdout).toContain('DONE') + expect(stdout.join('')).toContain('DONE') }) - it('should work without --control-fd (backward compat)', async () => { - const testScript = path.join(tmpDir, 'test.sh') - fs.writeFileSync(testScript, '#!/bin/bash\necho "NO_CONTROL_FD"\n', { - mode: 0o755, - }) + it('should exit with a FIFO control fd the parent keeps open', async () => { + // stdio 'pipe' hands srt a unix socket; a named pipe is what pipe(2), + // mkfifo and Python's pass_fds embedders hand it, and the fd kind the + // hang was reported against. The write end stays open for the whole + // test, so srt only exits if the fd does not keep it alive. + const fifo = path.join(tmpDir, 'control.fifo') + execFileSync('mkfifo', [fifo]) + // O_RDWR: opens without a reader and never delivers EOF to srt. + const writer = fs.openSync(fifo, fs.constants.O_RDWR) + heldFds.push(writer) + const readEnd = fs.openSync(fifo, fs.constants.O_RDONLY) + + const testScript = writeScript('sleep 0.3\necho "FIFO_DONE"') + const { stdout, stderr } = spawnSrt( + ['--debug', '--control-fd', '3', '--', testScript], + ['inherit', 'pipe', 'pipe', readEnd], + { ...process.env, SRT_DEBUG: 'true' }, + ) + fs.closeSync(readEnd) - // Spawn without --control-fd - child = spawn('node', [CLI_PATH, '--', testScript], { - stdio: ['inherit', 'pipe', 'pipe'], - }) + await settle() + fs.writeSync(writer, CONFIG_UPDATE + '\n') - const stdout: string[] = [] + expect(await exited).toBe(0) + expect(stdout.join('')).toContain('FIFO_DONE') + expect(stderr.join('')).toContain('Config updated from control fd') + }) - child.stdout?.on('data', (data: Buffer) => { - stdout.push(data.toString()) - }) + it('should read a regular file on the control fd', async () => { + // Not a pipe or socket: the fs stream path. The update is read to EOF + // and applied before the command finishes. + const configFile = path.join(tmpDir, 'control.json') + fs.writeFileSync(configFile, CONFIG_UPDATE + '\n') + const fileFd = fs.openSync(configFile, fs.constants.O_RDONLY) + + const testScript = writeScript('sleep 0.3\necho "FILE_DONE"') + const { stdout, stderr } = spawnSrt( + ['--debug', '--control-fd', '3', '--', testScript], + ['inherit', 'pipe', 'pipe', fileFd], + { ...process.env, SRT_DEBUG: 'true' }, + ) + fs.closeSync(fileFd) + + expect(await exited).toBe(0) + expect(stdout.join('')).toContain('FILE_DONE') + expect(stderr.join('')).toContain('Config updated from control fd') + }) - const exitCode = await waitForExit(child) + it('should work without --control-fd (backward compat)', async () => { + const testScript = writeScript('echo "NO_CONTROL_FD"') - expect(exitCode).toBe(0) - const allStdout = stdout.join('') - expect(allStdout).toContain('NO_CONTROL_FD') + // Spawn without --control-fd + const { stdout } = spawnSrt(['--', testScript], ['inherit', 'pipe', 'pipe']) + + expect(await exited).toBe(0) + expect(stdout.join('')).toContain('NO_CONTROL_FD') }) it('should allow stdin to pass through to child process', async () => { // Create a script that reads from stdin - const testScript = path.join(tmpDir, 'test.sh') - fs.writeFileSync( - testScript, - '#!/bin/bash\nread line\necho "GOT: $line"\n', - { mode: 0o755 }, - ) + const testScript = writeScript('read line\necho "GOT: $line"') // Spawn with stdin as pipe (not inherit) so we can write to it - child = spawn('node', [CLI_PATH, '--control-fd', '3', '--', testScript], { - stdio: ['pipe', 'pipe', 'pipe', 'pipe'], - }) - - const stdout: string[] = [] - - child.stdout?.on('data', (data: Buffer) => { - stdout.push(data.toString()) - }) + const { stdout } = spawnSrt( + ['--control-fd', '3', '--', testScript], + ['pipe', 'pipe', 'pipe', 'pipe'], + ) // Write to stdin (fd 0) - const stdin = child.stdin as Writable + const stdin = child!.stdin as Writable stdin.write('hello from stdin\n') - // srt must exit by itself once the wrapped command finishes, with the - // control fd still open on our side. - const exitCode = await waitForExit(child) - expect(exitCode).toBe(0) - - const allStdout = stdout.join('') - expect(allStdout).toContain('GOT: hello from stdin') + expect(await exited).toBe(0) + expect(stdout.join('')).toContain('GOT: hello from stdin') }) })