From 04abf17d60380ccc9444ffe9bb3277e9ce466d2d Mon Sep 17 00:00:00 2001 From: Chris Wanstrath Date: Tue, 10 Mar 2026 09:32:17 -0700 Subject: [PATCH 1/2] Add tests and kill process group on cleanup --- src/run.test.ts | 82 +++++++++++++++++++++++++++++++++++++++++++++++++ src/run.ts | 26 +++++++++++----- 2 files changed, 100 insertions(+), 8 deletions(-) create mode 100644 src/run.test.ts diff --git a/src/run.test.ts b/src/run.test.ts new file mode 100644 index 0000000..37d0227 --- /dev/null +++ b/src/run.test.ts @@ -0,0 +1,82 @@ +import { describe, expect, test } from "bun:test" +import { readFile } from "node:fs/promises" + +import { runFile, cleanupTmpDir } from "./run.ts" +import type { ShoutFile } from "./parse.ts" + +function makeFile(commands: { command: string; expected?: string[] }[]): ShoutFile { + return { + path: "test.shout", + commands: commands.map((c, i) => ({ + line: i + 1, + raw: `$ ${c.command}`, + command: c.command, + expected: c.expected ?? [], + exitCode: null, + })), + } +} + +const defaultOpts = { + cleanEnv: false, + timeout: 5000, + verbose: false, +} + +async function isProcessRunning(pid: number): Promise { + try { + const stat = await readFile(`/proc/${pid}/stat`, "utf-8") + // State is the 3rd field: R=running, S=sleeping, Z=zombie, etc. + const state = stat.split(" ")[2] + return state !== "Z" && state !== "X" + } catch { + // /proc entry doesn't exist — process is fully gone + return false + } +} + +describe("runFile", () => { + test("basic command", async () => { + const file = makeFile([{ command: "echo hello" }]) + const result = await runFile(file, defaultOpts) + expect(result.results[0]?.actual).toEqual(["hello"]) + expect(result.results[0]?.exitCode).toBe(0) + await cleanupTmpDir(result.tmpDir) + }) + + test("cleans up backgrounded processes after exit", async () => { + const file = makeFile([ + { command: "sleep 300 >/dev/null 2>&1 & echo $!" }, + ]) + + const result = await runFile(file, defaultOpts) + const pid = parseInt(result.results[0]?.actual[0] ?? "", 10) + expect(pid).toBeGreaterThan(0) + + // Give the SIGTERM a moment to be delivered + await new Promise(r => setTimeout(r, 100)) + + expect(await isProcessRunning(pid)).toBe(false) + await cleanupTmpDir(result.tmpDir) + }) + + test("cleans up multiple backgrounded processes", async () => { + const file = makeFile([ + { command: "sleep 300 >/dev/null 2>&1 & P1=$!; sleep 300 >/dev/null 2>&1 & P2=$!; echo $P1 $P2" }, + ]) + + const result = await runFile(file, defaultOpts) + const pids = (result.results[0]?.actual[0] ?? "").split(" ").map(Number) + expect(pids).toHaveLength(2) + expect(pids[0]).toBeGreaterThan(0) + expect(pids[1]).toBeGreaterThan(0) + + await new Promise(r => setTimeout(r, 100)) + + for (const pid of pids) { + expect(await isProcessRunning(pid)).toBe(false) + } + + await cleanupTmpDir(result.tmpDir) + }) +}) diff --git a/src/run.ts b/src/run.ts index 001ac4d..e5754ac 100644 --- a/src/run.ts +++ b/src/run.ts @@ -132,15 +132,15 @@ export async function runFile( env["PATH"] = options.pathDirs.join(":") + ":" + (env["PATH"] ?? "") } - try { - const proc = Bun.spawn(["/bin/sh"], { - stdin: "pipe", - stdout: "pipe", - stderr: "pipe", - cwd: tmpDir, - env, - }) + const proc = Bun.spawn(["setsid", "/bin/sh"], { + stdin: "pipe", + stdout: "pipe", + stderr: "pipe", + cwd: tmpDir, + env, + }) + try { proc.stdin.write(script) proc.stdin.end() @@ -174,6 +174,16 @@ export async function runFile( tmpDir, error: err instanceof Error ? err.message : String(err), } + } finally { + // Kill the shell's process group to clean up backgrounded children + if (proc.pid) { + try { + process.kill(-proc.pid, "SIGTERM") + } catch {} + setTimeout(() => { + try { process.kill(-proc.pid, "SIGKILL") } catch {} + }, 500) + } } } From 22d61d906e5e5bae846f188f210c5ac565d51550 Mon Sep 17 00:00:00 2001 From: Chris Wanstrath Date: Tue, 10 Mar 2026 09:45:45 -0700 Subject: [PATCH 2/2] use setsid for process group cleanup --- CLAUDE.md | 2 +- src/run.test.ts | 54 ++++++++++++++++++++++++------------------------- src/run.ts | 12 ++++------- 3 files changed, 32 insertions(+), 36 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 9bf54dc..521c80e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -11,7 +11,7 @@ Transcript-based shell integration test runner. Bun + TypeScript. ## Architecture - `src/parse.ts` — parses `.shout` files into `ShoutFile` (list of `Command`) -- `src/run.ts` — executes commands via `Bun.spawn(["/bin/sh"])`, captures output with sentinels +- `src/run.ts` — executes commands via `Bun.spawn(["setsid", "/bin/sh"])`, captures output with sentinels - `src/match.ts` — wildcard-aware output matching and diff generation - `src/format.ts` — evaluates pass/fail, formats failures and summary - `src/update.ts` — rewrites `.shout` files with actual output (`--update` mode) diff --git a/src/run.test.ts b/src/run.test.ts index 37d0227..e2ab64f 100644 --- a/src/run.test.ts +++ b/src/run.test.ts @@ -26,11 +26,10 @@ const defaultOpts = { async function isProcessRunning(pid: number): Promise { try { const stat = await readFile(`/proc/${pid}/stat`, "utf-8") - // State is the 3rd field: R=running, S=sleeping, Z=zombie, etc. - const state = stat.split(" ")[2] + // Find state after the comm field (which is wrapped in parens and may contain spaces) + const state = stat.charAt(stat.lastIndexOf(")") + 2) return state !== "Z" && state !== "X" } catch { - // /proc entry doesn't exist — process is fully gone return false } } @@ -39,44 +38,45 @@ describe("runFile", () => { test("basic command", async () => { const file = makeFile([{ command: "echo hello" }]) const result = await runFile(file, defaultOpts) - expect(result.results[0]?.actual).toEqual(["hello"]) - expect(result.results[0]?.exitCode).toBe(0) - await cleanupTmpDir(result.tmpDir) + try { + expect(result.results[0]?.actual).toEqual(["hello"]) + expect(result.results[0]?.exitCode).toBe(0) + } finally { + await cleanupTmpDir(result.tmpDir) + } }) test("cleans up backgrounded processes after exit", async () => { const file = makeFile([ { command: "sleep 300 >/dev/null 2>&1 & echo $!" }, ]) - const result = await runFile(file, defaultOpts) - const pid = parseInt(result.results[0]?.actual[0] ?? "", 10) - expect(pid).toBeGreaterThan(0) - - // Give the SIGTERM a moment to be delivered - await new Promise(r => setTimeout(r, 100)) - - expect(await isProcessRunning(pid)).toBe(false) - await cleanupTmpDir(result.tmpDir) + try { + const pid = parseInt(result.results[0]?.actual[0] ?? "", 10) + expect(pid).toBeGreaterThan(0) + await new Promise(r => setTimeout(r, 100)) + expect(await isProcessRunning(pid)).toBe(false) + } finally { + await cleanupTmpDir(result.tmpDir) + } }) test("cleans up multiple backgrounded processes", async () => { const file = makeFile([ { command: "sleep 300 >/dev/null 2>&1 & P1=$!; sleep 300 >/dev/null 2>&1 & P2=$!; echo $P1 $P2" }, ]) - const result = await runFile(file, defaultOpts) - const pids = (result.results[0]?.actual[0] ?? "").split(" ").map(Number) - expect(pids).toHaveLength(2) - expect(pids[0]).toBeGreaterThan(0) - expect(pids[1]).toBeGreaterThan(0) - - await new Promise(r => setTimeout(r, 100)) - - for (const pid of pids) { - expect(await isProcessRunning(pid)).toBe(false) + try { + const pids = (result.results[0]?.actual[0] ?? "").split(" ").map(Number) + expect(pids).toHaveLength(2) + expect(pids[0]).toBeGreaterThan(0) + expect(pids[1]).toBeGreaterThan(0) + await new Promise(r => setTimeout(r, 100)) + for (const pid of pids) { + expect(await isProcessRunning(pid)).toBe(false) + } + } finally { + await cleanupTmpDir(result.tmpDir) } - - await cleanupTmpDir(result.tmpDir) }) }) diff --git a/src/run.ts b/src/run.ts index e5754ac..c8c9bb2 100644 --- a/src/run.ts +++ b/src/run.ts @@ -175,14 +175,8 @@ export async function runFile( error: err instanceof Error ? err.message : String(err), } } finally { - // Kill the shell's process group to clean up backgrounded children if (proc.pid) { - try { - process.kill(-proc.pid, "SIGTERM") - } catch {} - setTimeout(() => { - try { process.kill(-proc.pid, "SIGKILL") } catch {} - }, 500) + try { process.kill(-proc.pid, "SIGKILL") } catch {} } } } @@ -194,8 +188,9 @@ async function readWithTimeout( const reader = stream.getReader() const chunks: Uint8Array[] = [] + let timerId: ReturnType const timeout = new Promise((_, reject) => - setTimeout(() => reject(new Error("Timeout reading output")), timeoutMs), + timerId = setTimeout(() => reject(new Error("Timeout reading output")), timeoutMs), ) try { @@ -205,6 +200,7 @@ async function readWithTimeout( if (value) chunks.push(value) } } finally { + clearTimeout(timerId!) reader.releaseLock() }