From 7f5c27d5027f7d49678a599025a6292a48f6a94b Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Tue, 8 Sep 2026 12:53:15 -0400 Subject: [PATCH 1/4] fix: cancel host process trees and scope daemon cleanup --- docs/architecture.md | 7 + docs/env.md | 9 ++ src/commands/daemon.ts | 2 + src/commands/env.ts | 1 + src/daemon/process.ts | 72 +++++++++- src/lib/shell.ts | 67 +++++++++- tests/daemon-command.test.ts | 59 ++++++++- tests/daemon-orphan.test.ts | 45 ++++++- tests/host-exec-lifetime.test.ts | 219 +++++++++++++++++++++++++++++++ 9 files changed, 474 insertions(+), 7 deletions(-) create mode 100644 tests/host-exec-lifetime.test.ts diff --git a/docs/architecture.md b/docs/architecture.md index 74c8cf81..f879f89e 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -215,6 +215,13 @@ perform full inspection so missed events and mutable network data remain eventua If the daemon is not running (or version-mismatched), the CLI falls back to direct Docker calls. +Daemon startup and `hack daemon clear` only terminate untracked daemon processes +when `lsof` confirms they hold a socket in the target daemon state directory. +Matching a process name is insufficient: a different `HOME` or `HACK_HOME` may +own another daemon. If socket ownership cannot be established, automatic orphan +termination is skipped. Scripted/noninteractive commands do not autostart the +daemon; after an explicit stop, use `hack daemon start` to restore cached queries. + Runtime health: - The daemon treats the container runtime as ephemeral; it fingerprints the engine (socket + engine id) and detects resets. diff --git a/docs/env.md b/docs/env.md index b89dfb79..7bc2f34f 100644 --- a/docs/env.md +++ b/docs/env.md @@ -242,6 +242,15 @@ hack host exec --env qa --scope api -- bun db:migrate hack host exec --env qa --scope api --target compose -- bun test ``` +Cancelling `hack host exec` or `hack env exec` with SIGINT or SIGTERM forwards +the signal to the command. Noninteractive commands run in an owned process group, +so cancellation also stops their descendants, escalating to SIGKILL after two +seconds if necessary. Hack returns 130 for SIGINT and 143 for SIGTERM. Interactive +commands retain their terminal process group so stdin and terminal job control +continue to work. SIGKILL cannot be forwarded; supervisors must terminate the +whole owned process tree when force-killing a wrapper. Commands have no implicit +time limit, and normal completion preserves the command's exit status. + When you want to inspect an injected value, avoid `hack host exec -- echo $VAR`. Your current shell expands `$VAR` before Hack starts the child process, so the command often sees an empty string. diff --git a/src/commands/daemon.ts b/src/commands/daemon.ts index 095b3d65..f25ed48f 100644 --- a/src/commands/daemon.ts +++ b/src/commands/daemon.ts @@ -252,6 +252,7 @@ async function handleDaemonStart({ // make every freshly spawned daemon exit cleanly. Sweep them first. const orphans = await findOrphanDaemonProcesses({ trackedPid: status.pid, + daemonRoot: paths.root, }); if (orphans.length > 0) { logger.warn({ @@ -603,6 +604,7 @@ async function handleDaemonClear({ const orphans = await findOrphanDaemonProcesses({ trackedPid: status.pid, + daemonRoot: paths.root, }); if (orphans.length > 0) { logger.warn({ diff --git a/src/commands/env.ts b/src/commands/env.ts index 1eaaab47..404963fb 100644 --- a/src/commands/env.ts +++ b/src/commands/env.ts @@ -1925,6 +1925,7 @@ async function runHostCommandWithInjectedEnv(input: { cwd: project.projectRoot, env: envState.env, stdin: "inherit", + forwardSignals: true, } ); } diff --git a/src/daemon/process.ts b/src/daemon/process.ts index 4050009d..847e1745 100644 --- a/src/daemon/process.ts +++ b/src/daemon/process.ts @@ -1,4 +1,8 @@ +import { realpath } from "node:fs/promises"; +import { resolve } from "node:path"; + import { readTextFile, writeTextFile } from "../lib/fs.ts"; +import { exec, findExecutableInPath } from "../lib/shell.ts"; export async function readDaemonPid({ pidPath, @@ -79,12 +83,18 @@ const WHITESPACE_PATTERN = /\s+/; * pid file — the root cause of "daemon starts then exits" contradictions: * the orphan holds the API while every newly spawned daemon exits. * + * Only processes holding a socket in the target daemon directory are eligible. + * A command-name match alone cannot distinguish another HOME/HACK_HOME daemon. + * Missing ownership evidence must never authorize termination. + * * @param opts.trackedPid - pid currently recorded in the pid file, if any. * @param opts.psLines - injectable `ps -axo pid=,command=` lines for tests. */ export async function findOrphanDaemonProcesses(opts: { readonly trackedPid: number | null; + readonly daemonRoot: string; readonly psLines?: readonly string[]; + readonly lsofLines?: readonly string[]; }): Promise { const lines = opts.psLines ?? (await listProcessTable()); const orphans: number[] = []; @@ -110,7 +120,67 @@ export async function findOrphanDaemonProcesses(opts: { } orphans.push(pid); } - return orphans; + if (orphans.length === 0) { + return []; + } + const owners = await findDaemonSocketOwners({ + daemonRoot: opts.daemonRoot, + lines: opts.lsofLines ?? (await listOpenUnixSockets({ pids: orphans })), + }); + return orphans.filter((orphan) => owners.has(orphan)); +} + +async function findDaemonSocketOwners(opts: { + readonly daemonRoot: string; + readonly lines: readonly string[]; +}): Promise> { + const roots = new Set([resolve(opts.daemonRoot)]); + try { + roots.add(await realpath(opts.daemonRoot)); + } catch { + // A deleted directory can still appear in an open socket's pathname. + } + const socketPaths = new Set( + [...roots].flatMap((root) => + ["hackd.sock", "hackd.internal.sock", "gateway.internal.sock"].map( + (name) => resolve(root, name) + ) + ) + ); + const owners = new Set(); + let pid: number | null = null; + for (const line of opts.lines) { + if (line.startsWith("p")) { + pid = Number.parseInt(line.slice(1), 10); + } else if ( + pid !== null && + line.startsWith("n") && + socketPaths.has(line.slice(1)) + ) { + owners.add(pid); + } + } + return owners; +} + +async function listOpenUnixSockets(opts: { + readonly pids: readonly number[]; +}): Promise { + const lsof = findExecutableInPath("lsof"); + if (!lsof) { + return []; + } + try { + const result = await exec( + // Socket names do not need filesystem stat/readlink calls, which can + // block on unrelated mounted filesystems during daemon recovery. + [lsof, "-nP", "-b", "-w", "-a", "-U", "-p", opts.pids.join(","), "-Fpn"], + { stdin: "ignore", timeoutMs: 3000 } + ); + return result.exitCode === 0 ? result.stdout.split("\n") : []; + } catch { + return []; + } } /** diff --git a/src/lib/shell.ts b/src/lib/shell.ts index b64f0aa2..342e8fc2 100644 --- a/src/lib/shell.ts +++ b/src/lib/shell.ts @@ -73,27 +73,86 @@ export interface RunOptions { */ readonly stdout?: "inherit" | "stderr"; readonly timeoutMs?: number; + /** Forward cancellation to the child, owning its process group when stdin is not a TTY. */ + readonly forwardSignals?: boolean; } export async function run( cmd: readonly string[], opts: RunOptions = {} ): Promise { + const ownsProcessGroup = + opts.timeoutMs !== undefined || + (opts.forwardSignals === true && !process.stdin.isTTY); const proc = Bun.spawn([...cmd], { cwd: opts.cwd, env: buildSpawnEnv(opts.env), stdin: opts.stdin ?? "inherit", stdout: opts.stdout === "stderr" ? 2 : "inherit", stderr: "inherit", - detached: opts.timeoutMs !== undefined, + detached: ownsProcessGroup, }); const timeout = installSubprocessTimeout({ pid: proc.pid, timeoutMs: opts.timeoutMs, }); - const exitCode = await proc.exited; - timeout.dispose(); - return timeout.didTimeout() ? 124 : exitCode; + const cancellation = opts.forwardSignals + ? installSubprocessSignalForwarding({ pid: proc.pid, ownsProcessGroup }) + : null; + try { + const exitCode = await proc.exited; + return cancellation?.exitCode() ?? (timeout.didTimeout() ? 124 : exitCode); + } finally { + timeout.dispose(); + cancellation?.dispose(); + } +} + +/** + * Noninteractive commands own a separate group so cancelling just the wrapper + * also stops descendants. TTY children keep their foreground group for stdin + * and terminal job control; the terminal delivers group signals itself. + */ +function installSubprocessSignalForwarding(opts: { + readonly pid: number; + readonly ownsProcessGroup: boolean; +}): { readonly dispose: () => void; readonly exitCode: () => number | null } { + let exitCode: number | null = null; + let forceKillTimer: ReturnType | null = null; + const send = (signal: NodeJS.Signals): void => { + try { + process.kill(opts.ownsProcessGroup ? -opts.pid : opts.pid, signal); + } catch { + // The owned process/group may already have exited. + } + }; + const cancel = (signal: "SIGINT" | "SIGTERM"): void => { + if (exitCode !== null) { + send("SIGKILL"); + return; + } + exitCode = signal === "SIGINT" ? 130 : 143; + send(signal); + forceKillTimer = setTimeout(() => send("SIGKILL"), 2000); + }; + const onInterrupt = (): void => cancel("SIGINT"); + const onTerminate = (): void => cancel("SIGTERM"); + process.on("SIGINT", onInterrupt); + process.on("SIGTERM", onTerminate); + return { + exitCode: () => exitCode, + dispose: () => { + process.off("SIGINT", onInterrupt); + process.off("SIGTERM", onTerminate); + if (forceKillTimer) { + clearTimeout(forceKillTimer); + } + if (exitCode !== null && opts.ownsProcessGroup) { + // A cooperative child can exit before its stubborn descendants. + send("SIGKILL"); + } + }, + }; } function installSubprocessTimeout(opts: { diff --git a/tests/daemon-command.test.ts b/tests/daemon-command.test.ts index edd1bd2c..a746edaa 100644 --- a/tests/daemon-command.test.ts +++ b/tests/daemon-command.test.ts @@ -1,13 +1,15 @@ import { afterEach, beforeEach, expect, test } from "bun:test"; -import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises"; +import { mkdir, mkdtemp, rm, symlink, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { resolveDaemonPaths } from "../src/daemon/paths.ts"; +import { findOrphanDaemonProcesses } from "../src/daemon/process.ts"; let tempDir: string | null = null; let originalHome: string | undefined; let originalLogger: string | undefined; +let foreignDaemon: ReturnType | null = null; beforeEach(async () => { originalHome = process.env.HOME; @@ -18,6 +20,11 @@ beforeEach(async () => { }); afterEach(async () => { + if (foreignDaemon) { + foreignDaemon.kill("SIGKILL"); + await foreignDaemon.exited; + foreignDaemon = null; + } if (tempDir) { await rm(tempDir, { recursive: true, force: true }); tempDir = null; @@ -26,6 +33,56 @@ afterEach(async () => { process.env.HACK_LOGGER = originalLogger; }); +test.skipIf(!Bun.which("lsof"))( + "daemon clear preserves a real daemon socket owned by another state directory", + async () => { + if (!tempDir) { + throw new Error("Fixture not initialized"); + } + const foreignRoot = join(tempDir, "foreign-daemon"); + await mkdir(foreignRoot); + const binary = join(tempDir, "hack-foreign-fixture"); + await symlink(process.execPath, binary); + const socket = join(foreignRoot, "hackd.sock"); + const ready = join(foreignRoot, "ready"); + foreignDaemon = Bun.spawn( + [ + binary, + "-e", + `Bun.serve({ unix: ${JSON.stringify(socket)}, fetch() { return new Response("fixture"); } }); await Bun.write(${JSON.stringify(ready)}, "ready");`, + "--", + "daemon", + "start", + "--foreground", + ], + { stdin: "ignore", stdout: "ignore", stderr: "ignore" } + ); + for (let attempt = 0; attempt < 100; attempt++) { + if (await Bun.file(ready).exists()) { + break; + } + await Bun.sleep(20); + } + expect(await Bun.file(ready).exists()).toBe(true); + expect( + await findOrphanDaemonProcesses({ + trackedPid: null, + daemonRoot: foreignRoot, + }) + ).toContain(foreignDaemon.pid); + + const paths = resolveDaemonPaths({}); + await mkdir(paths.root, { recursive: true }); + await writeFile(paths.pidPath, "999999\n"); + const { runCli } = await import("../src/cli/run.ts"); + expect(await runCli(["daemon", "clear"])).toBe(0); + await Bun.sleep(100); + expect(foreignDaemon.exitCode).toBeNull(); + expect(process.kill(foreignDaemon.pid, 0)).toBe(true); + }, + 10_000 +); + test("daemon clear removes stale pid and socket files", async () => { const paths = resolveDaemonPaths({}); await mkdir(paths.root, { recursive: true }); diff --git a/tests/daemon-orphan.test.ts b/tests/daemon-orphan.test.ts index fed98e6a..797eac13 100644 --- a/tests/daemon-orphan.test.ts +++ b/tests/daemon-orphan.test.ts @@ -8,19 +8,30 @@ const PS_LINES = [ " 789 vim src/commands/daemon.ts", " 999 hack daemon status", ]; +const DAEMON_ROOT = "/tmp/hack-home/.hack/daemon"; +const LSOF_LINES = [ + "p123", + `n${DAEMON_ROOT}/hackd.sock`, + "p456", + `n${DAEMON_ROOT}/hackd.internal.sock`, +]; test("finds daemon processes not tracked by the pid file", async () => { const orphans = await findOrphanDaemonProcesses({ trackedPid: 123, + daemonRoot: DAEMON_ROOT, psLines: PS_LINES, + lsofLines: LSOF_LINES, }); expect(orphans).toEqual([456]); }); -test("all daemon processes are orphans when no pid is tracked", async () => { +test("only socket-owned daemon processes are orphans when no pid is tracked", async () => { const orphans = await findOrphanDaemonProcesses({ trackedPid: null, + daemonRoot: DAEMON_ROOT, psLines: PS_LINES, + lsofLines: LSOF_LINES, }); expect(orphans).toEqual([123, 456]); }); @@ -28,6 +39,7 @@ test("all daemon processes are orphans when no pid is tracked", async () => { test("ignores its own pid, non-hack executables, and near-miss commands", async () => { const orphans = await findOrphanDaemonProcesses({ trackedPid: null, + daemonRoot: DAEMON_ROOT, psLines: [ ` ${process.pid} hack daemon start --foreground`, " 789 tail -f daemon-start-foreground.log", @@ -35,10 +47,41 @@ test("ignores its own pid, non-hack executables, and near-miss commands", async " 791 /tmp/hack-repo/bin/hack-dev daemon start --foreground", " 792 bun /tmp/hack-repo/index.ts daemon start --foreground", ], + lsofLines: [ + "p791", + `n${DAEMON_ROOT}/hackd.sock`, + "p792", + `n${DAEMON_ROOT}/hackd.internal.sock`, + ], }); expect(orphans).toEqual([791, 792]); }); +test("preserves daemons belonging to another state directory", async () => { + const orphans = await findOrphanDaemonProcesses({ + trackedPid: null, + daemonRoot: DAEMON_ROOT, + psLines: PS_LINES, + lsofLines: [ + "p123", + "n/tmp/other-home/daemon/hackd.sock", + "p456", + `n${DAEMON_ROOT}/hackd.sock.backup`, + ], + }); + expect(orphans).toEqual([]); +}); + +test("does not authorize cleanup without socket ownership evidence", async () => { + const orphans = await findOrphanDaemonProcesses({ + trackedPid: null, + daemonRoot: DAEMON_ROOT, + psLines: PS_LINES, + lsofLines: [], + }); + expect(orphans).toEqual([]); +}); + import { extractLaunchdProgramPath, isVirtualExecutablePath, diff --git a/tests/host-exec-lifetime.test.ts b/tests/host-exec-lifetime.test.ts new file mode 100644 index 00000000..7c63ce2e --- /dev/null +++ b/tests/host-exec-lifetime.test.ts @@ -0,0 +1,219 @@ +import { afterEach, expect, test } from "bun:test"; +import { mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { resolve } from "node:path"; + +const fixtures: string[] = []; +const wrappers: ReturnType[] = []; +const entrypoint = resolve(import.meta.dir, "../index.ts"); + +afterEach(async () => { + for (const root of fixtures) { + for (const name of ["grandchild.pid", "child.pid"]) { + const pid = await readPid(resolve(root, name)); + if (pid !== null) { + signalPid(pid, "SIGKILL"); + } + } + } + for (const wrapper of wrappers.splice(0)) { + if (wrapper.exitCode === null) { + wrapper.kill("SIGKILL"); + } + await wrapper.exited; + } + await Promise.all( + fixtures.splice(0).map((root) => rm(root, { recursive: true, force: true })) + ); +}); + +for (const signal of ["SIGTERM", "SIGINT"] as const) { + test(`host exec forwards wrapper-only ${signal} and removes stubborn descendants`, async () => { + const root = await createFixture({ childIgnoresSignals: false }); + const wrapper = startHostCommand({ root }); + const child = await waitForPid(resolve(root, "child.pid")); + const grandchild = await waitForPid(resolve(root, "grandchild.pid")); + + signalPid(wrapper.pid, signal); + + expect(await wrapper.exited).toBe(signal === "SIGTERM" ? 143 : 130); + await expectStopped(child); + await expectStopped(grandchild); + }); +} + +test("host exec bounds cancellation when the child ignores SIGTERM", async () => { + const root = await createFixture({ childIgnoresSignals: true }); + const wrapper = startHostCommand({ root }); + const child = await waitForPid(resolve(root, "child.pid")); + const grandchild = await waitForPid(resolve(root, "grandchild.pid")); + const started = performance.now(); + + signalPid(wrapper.pid, "SIGTERM"); + + expect(await wrapper.exited).toBe(143); + expect(performance.now() - started).toBeLessThan(4000); + await expectStopped(child); + await expectStopped(grandchild); +}); + +test("host exec forwards cancellation received by the wrapper's process group", async () => { + const root = await createFixture({ childIgnoresSignals: false }); + const wrapper = startHostCommand({ root }); + const child = await waitForPid(resolve(root, "child.pid")); + const grandchild = await waitForPid(resolve(root, "grandchild.pid")); + + process.kill(-wrapper.pid, "SIGTERM"); + + expect(await wrapper.exited).toBe(143); + await expectStopped(child); + await expectStopped(grandchild); +}); + +test("host exec preserves piped stdin and a normal nonzero exit status", async () => { + const root = await createFixture({ childIgnoresSignals: false }); + await writeFile( + resolve(root, "child.ts"), + "console.log(`received:${await Bun.stdin.text()}`); process.exitCode = 7;" + ); + const wrapper = Bun.spawn(hostCommand({ root }), { + cwd: root, + env: fixtureEnv(root), + stdin: "pipe", + stdout: "pipe", + stderr: "pipe", + }); + wrappers.push(wrapper); + wrapper.stdin.write("fixture-input"); + wrapper.stdin.end(); + const output = new Response(wrapper.stdout).text(); + + expect(await wrapper.exited).toBe(7); + expect(await output).toBe("received:fixture-input\n"); +}); + +function startHostCommand({ root }: { readonly root: string }) { + const wrapper = Bun.spawn(hostCommand({ root }), { + cwd: root, + env: fixtureEnv(root), + stdin: "ignore", + stdout: "ignore", + stderr: "ignore", + detached: true, + }); + wrappers.push(wrapper); + return wrapper; +} + +function hostCommand({ root }: { readonly root: string }): string[] { + return [ + process.execPath, + entrypoint, + "host", + "exec", + "--path", + root, + "--no-interactive", + "--", + process.execPath, + resolve(root, "child.ts"), + ]; +} + +function fixtureEnv(root: string): Record { + return { + PATH: process.env.PATH ?? "/usr/bin:/bin", + HACK_HOME: resolve(root, "hack-home"), + HACK_NO_INTERACTIVE: "1", + }; +} + +async function createFixture(opts: { + readonly childIgnoresSignals: boolean; +}): Promise { + const root = await mkdtemp(resolve(tmpdir(), "hack-host-lifetime-")); + fixtures.push(root); + await mkdir(resolve(root, ".hack")); + await writeFile( + resolve(root, ".hack/hack.config.json"), + JSON.stringify({ name: "lifetime-test", dev_host: "lifetime-test.hack" }) + ); + await writeFile( + resolve(root, ".hack/docker-compose.yml"), + "services:\n noop:\n image: alpine:3.20\n" + ); + await writeFile( + resolve(root, ".hack/hack.env.default.yaml"), + "version: 1\nenvironment: default\nsecretsprovider: project_key\nvalues:\n global: {}\n" + ); + await writeFile( + resolve(root, "grandchild.ts"), + [ + 'process.on("SIGTERM", () => {});', + 'process.on("SIGINT", () => {});', + 'await Bun.write("grandchild.pid", String(process.pid));', + "setInterval(() => {}, 1000);", + ].join("\n") + ); + await writeFile( + resolve(root, "child.ts"), + [ + `const stop = () => { ${opts.childIgnoresSignals ? "" : "process.exit(0);"} };`, + 'process.on("SIGTERM", stop);', + 'process.on("SIGINT", stop);', + 'await Bun.write("child.pid", String(process.pid));', + 'Bun.spawn([process.execPath, "grandchild.ts"], { stdin: "ignore", stdout: "ignore", stderr: "ignore" });', + "setInterval(() => {}, 1000);", + ].join("\n") + ); + return root; +} + +async function readPid(path: string): Promise { + try { + const pid = Number((await readFile(path, "utf8")).trim()); + return Number.isSafeInteger(pid) && pid > 0 ? pid : null; + } catch { + return null; + } +} + +async function waitForPid(path: string): Promise { + for (let attempt = 0; attempt < 100; attempt++) { + const pid = await readPid(path); + if (pid !== null) { + return pid; + } + await Bun.sleep(20); + } + throw new Error(`Child did not become ready: ${path}`); +} + +async function expectStopped(pid: number): Promise { + for (let attempt = 0; attempt < 100; attempt++) { + try { + process.kill(pid, 0); + } catch { + return; + } + await Bun.sleep(10); + } + expect(isAlive(pid)).toBe(false); +} + +function isAlive(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch { + return false; + } +} + +function signalPid(pid: number, signal: NodeJS.Signals): void { + try { + process.kill(pid, signal); + } catch { + // Test-owned processes may already have exited. + } +} From bd1c285fc86dbfe7a3f8db8b8f08f084de1c45fd Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Tue, 8 Sep 2026 13:44:24 -0400 Subject: [PATCH 2/4] fix: cancel verified TTY command descendants --- docs/env.md | 4 ++ src/lib/process-tree.ts | 84 ++++++++++++++++++++++++++++++ src/lib/shell.ts | 61 +++++++++++++++++++--- tests/fixtures/tty-cancellation.py | 77 +++++++++++++++++++++++++++ tests/host-exec-tty.test.ts | 40 ++++++++++++++ 5 files changed, 259 insertions(+), 7 deletions(-) create mode 100644 src/lib/process-tree.ts create mode 100644 tests/fixtures/tty-cancellation.py create mode 100644 tests/host-exec-tty.test.ts diff --git a/docs/env.md b/docs/env.md index 7bc2f34f..0210d161 100644 --- a/docs/env.md +++ b/docs/env.md @@ -404,3 +404,7 @@ If you are writing new docs or new project setup flows, document the YAML overla - [Sessions](sessions.md) - [CLI reference](cli.md) - [Pulumi-style env config design](plans/2026-03-27-pulumi-style-env-config-design.md) + +For terminal commands cancelled through the wrapper PID, Hack captures the child +process tree and revalidates process start times before signalling surviving +descendants. It never signals the shared terminal process group. diff --git a/src/lib/process-tree.ts b/src/lib/process-tree.ts new file mode 100644 index 00000000..c354fc61 --- /dev/null +++ b/src/lib/process-tree.ts @@ -0,0 +1,84 @@ +export type ProcessIdentityRow = { + readonly pid: number; + readonly parentPid: number; + readonly birth: string; +}; +const IDENTITY_ROW = /^\s*(\d+)\s+(\d+)\s+(.+?)\s*$/; + +/** Capture only process identity and lineage; never command arguments or environment. */ +export async function readProcessIdentities(): Promise { + try { + const proc = Bun.spawn(["ps", "-A", "-o", "pid=,ppid=,lstart="], { + env: { ...process.env, LC_ALL: "C" }, + stdin: "ignore", + stdout: "pipe", + stderr: "ignore", + timeout: 2000, + }); + const output = await new Response(proc.stdout).text(); + if ((await proc.exited) !== 0) { + return []; + } + return output.split("\n").flatMap((line) => { + const match = IDENTITY_ROW.exec(line); + return match + ? [ + { + pid: Number(match[1]), + parentPid: Number(match[2]), + birth: (match[3] ?? "").replace(/\s+/g, " "), + }, + ] + : []; + }); + } catch { + return []; + } +} + +export function selectProcessTree( + rows: readonly ProcessIdentityRow[], + rootPid: number +): ProcessIdentityRow[] { + const children = new Map(); + for (const row of rows) { + const entries = children.get(row.parentPid) ?? []; + entries.push(row); + children.set(row.parentPid, entries); + } + const root = rows.find((row) => row.pid === rootPid); + const tree = root ? [root] : []; + const seen = new Set([rootPid]); + for (const parent of tree) { + for (const child of children.get(parent.pid) ?? []) { + if (!seen.has(child.pid)) { + seen.add(child.pid); + tree.push(child); + } + } + } + return tree; +} + +/** Revalidate start times before signalling a captured tree, including reparented descendants. */ +export async function signalVerifiedProcessTree( + tree: readonly ProcessIdentityRow[], + signal: NodeJS.Signals +): Promise { + if (!tree.length) { + return; + } + const live = new Map( + (await readProcessIdentities()).map((row) => [row.pid, row.birth]) + ); + for (const row of [...tree].reverse()) { + if (live.get(row.pid) !== row.birth) { + continue; + } + try { + process.kill(row.pid, signal); + } catch { + /* Process already exited. */ + } + } +} diff --git a/src/lib/shell.ts b/src/lib/shell.ts index 342e8fc2..01e75ad4 100644 --- a/src/lib/shell.ts +++ b/src/lib/shell.ts @@ -1,3 +1,10 @@ +import { + type ProcessIdentityRow, + readProcessIdentities, + selectProcessTree, + signalVerifiedProcessTree, +} from "./process-tree.ts"; + export interface ExecResult { readonly exitCode: number; readonly stdout: string; @@ -97,14 +104,18 @@ export async function run( timeoutMs: opts.timeoutMs, }); const cancellation = opts.forwardSignals - ? installSubprocessSignalForwarding({ pid: proc.pid, ownsProcessGroup }) + ? installSubprocessSignalForwarding({ + pid: proc.pid, + ownsProcessGroup, + childRunning: () => proc.exitCode === null, + }) : null; try { const exitCode = await proc.exited; return cancellation?.exitCode() ?? (timeout.didTimeout() ? 124 : exitCode); } finally { timeout.dispose(); - cancellation?.dispose(); + await cancellation?.dispose(); } } @@ -116,24 +127,55 @@ export async function run( function installSubprocessSignalForwarding(opts: { readonly pid: number; readonly ownsProcessGroup: boolean; -}): { readonly dispose: () => void; readonly exitCode: () => number | null } { + readonly childRunning: () => boolean; +}): { + readonly dispose: () => Promise; + readonly exitCode: () => number | null; +} { let exitCode: number | null = null; let forceKillTimer: ReturnType | null = null; const send = (signal: NodeJS.Signals): void => { + if (!(opts.ownsProcessGroup || opts.childRunning())) { + return; + } try { process.kill(opts.ownsProcessGroup ? -opts.pid : opts.pid, signal); } catch { // The owned process/group may already have exited. } }; + let ttyTree: ProcessIdentityRow[] = []; + let ttyCancellation: Promise = Promise.resolve(); + let ttyEscalation: Promise = Promise.resolve(); + const cancelTty = async (signal: NodeJS.Signals): Promise => { + ttyTree = selectProcessTree(await readProcessIdentities(), opts.pid); + await signalVerifiedProcessTree(ttyTree, signal); + if (!ttyTree.some((row) => row.pid === opts.pid)) { + send(signal); + } + forceKillTimer = setTimeout(() => { + ttyEscalation = signalVerifiedProcessTree(ttyTree, "SIGKILL"); + send("SIGKILL"); + }, 2000); + }; const cancel = (signal: "SIGINT" | "SIGTERM"): void => { if (exitCode !== null) { - send("SIGKILL"); + if (opts.ownsProcessGroup) { + send("SIGKILL"); + } else { + ttyCancellation = ttyCancellation.then(() => + signalVerifiedProcessTree(ttyTree, "SIGKILL") + ); + } return; } exitCode = signal === "SIGINT" ? 130 : 143; - send(signal); - forceKillTimer = setTimeout(() => send("SIGKILL"), 2000); + if (opts.ownsProcessGroup) { + send(signal); + forceKillTimer = setTimeout(() => send("SIGKILL"), 2000); + } else { + ttyCancellation = cancelTty(signal); + } }; const onInterrupt = (): void => cancel("SIGINT"); const onTerminate = (): void => cancel("SIGTERM"); @@ -141,7 +183,8 @@ function installSubprocessSignalForwarding(opts: { process.on("SIGTERM", onTerminate); return { exitCode: () => exitCode, - dispose: () => { + dispose: async () => { + await ttyCancellation; process.off("SIGINT", onInterrupt); process.off("SIGTERM", onTerminate); if (forceKillTimer) { @@ -151,6 +194,10 @@ function installSubprocessSignalForwarding(opts: { // A cooperative child can exit before its stubborn descendants. send("SIGKILL"); } + await ttyEscalation; + if (exitCode !== null && !opts.ownsProcessGroup) { + await signalVerifiedProcessTree(ttyTree, "SIGKILL"); + } }, }; } diff --git a/tests/fixtures/tty-cancellation.py b/tests/fixtures/tty-cancellation.py new file mode 100644 index 00000000..175bdb43 --- /dev/null +++ b/tests/fixtures/tty-cancellation.py @@ -0,0 +1,77 @@ +import json +import os +import pathlib +import pty +import signal +import subprocess +import sys +import tempfile +import time + +if sys.argv[1] in ["worker", "grandchild"]: + signal.signal(signal.SIGHUP, signal.SIG_IGN) + +if sys.argv[1] == "worker": + root = pathlib.Path(sys.argv[2]) + if sys.argv[3] == "ignore": + signal.signal(signal.SIGTERM, signal.SIG_IGN) + signal.signal(signal.SIGINT, signal.SIG_IGN) + else: + signal.signal(signal.SIGTERM, lambda *_: sys.exit(0)) + signal.signal(signal.SIGINT, lambda *_: sys.exit(0)) + child = subprocess.Popen([sys.executable, __file__, "grandchild", str(root)]) + (root / "child.pid").write_text(str(os.getpid())) + while True: + time.sleep(1) +elif sys.argv[1] == "grandchild": + signal.signal(signal.SIGTERM, signal.SIG_IGN) + signal.signal(signal.SIGINT, signal.SIG_IGN) + (pathlib.Path(sys.argv[2]) / "grandchild.pid").write_text(str(os.getpid())) + while True: + time.sleep(1) +else: + bun, entrypoint, requested_signal, behavior = sys.argv[2:] + with tempfile.TemporaryDirectory(prefix="hack-tty-") as tmp: + root = pathlib.Path(tmp) + config = root / ".hack" + config.mkdir() + (config / "hack.config.json").write_text('{"name":"tty-regression"}') + (config / "docker-compose.yml").write_text("services:\n noop:\n image: alpine:3.20\n") + (config / "hack.env.default.yaml").write_text("version: 1\nenvironment: default\nsecretsprovider: project_key\nvalues:\n global: {}\n") + wrapper, fd = pty.fork() + if wrapper == 0: + sibling = subprocess.Popen([sys.executable, "-c", "import signal,time; signal.signal(signal.SIGHUP, signal.SIG_IGN); time.sleep(30)"]) + (root / "sibling.pid").write_text(str(sibling.pid)) + env = dict(os.environ, HACK_HOME=str(root / "state")) + os.execve(bun, [bun, entrypoint, "host", "exec", "--path", str(root), "--no-interactive", "--", sys.executable, __file__, "worker", str(root), behavior], env) + status = None + def alive(pid): + result = subprocess.run(["ps", "-p", str(pid), "-o", "stat="], capture_output=True, text=True) + return result.returncode == 0 and not result.stdout.strip().startswith("Z") + try: + deadline = time.monotonic() + 10 + while not (root / "grandchild.pid").exists() and time.monotonic() < deadline: + time.sleep(.025) + child = int((root / "child.pid").read_text()) + grandchild = int((root / "grandchild.pid").read_text()) + sibling = int((root / "sibling.pid").read_text()) + os.kill(wrapper, getattr(signal, requested_signal)) + deadline = time.monotonic() + 8 + while time.monotonic() < deadline: + got, result = os.waitpid(wrapper, os.WNOHANG) + if got: + status = result + break + time.sleep(.025) + print(json.dumps({"exit": os.waitstatus_to_exitcode(status) if status is not None else None, "childAlive": alive(child), "grandchildAlive": alive(grandchild), "siblingAlive": alive(sibling)})) + finally: + for name in ["child.pid", "grandchild.pid", "sibling.pid"]: + if (root / name).exists(): + try: + os.kill(int((root / name).read_text()), signal.SIGKILL) + except ProcessLookupError: + pass + if status is None: + os.kill(wrapper, signal.SIGKILL) + os.waitpid(wrapper, 0) + os.close(fd) diff --git a/tests/host-exec-tty.test.ts b/tests/host-exec-tty.test.ts new file mode 100644 index 00000000..55aed5ce --- /dev/null +++ b/tests/host-exec-tty.test.ts @@ -0,0 +1,40 @@ +import { expect, test } from "bun:test"; +import { resolve } from "node:path"; + +for (const [signal, behavior, code] of [ + ["SIGTERM", "exit", 143], + ["SIGINT", "exit", 130], + ["SIGTERM", "ignore", 143], +] as const) { + test.skipIf(!Bun.which("python3"))( + `TTY wrapper-only ${signal} cleans descendants (${behavior}) without signalling a sibling`, + async () => { + const proc = Bun.spawn( + [ + "python3", + resolve(import.meta.dir, "fixtures/tty-cancellation.py"), + "probe", + process.execPath, + resolve(import.meta.dir, "../index.ts"), + signal, + behavior, + ], + { stdin: "ignore", stdout: "pipe", stderr: "pipe" } + ); + const [stdout, stderr, exitCode] = await Promise.all([ + new Response(proc.stdout).text(), + new Response(proc.stderr).text(), + proc.exited, + ]); + expect(stderr).toBe(""); + expect(exitCode).toBe(0); + expect(JSON.parse(stdout)).toEqual({ + exit: code, + childAlive: false, + grandchildAlive: false, + siblingAlive: true, + }); + }, + 20_000 + ); +} From 1cc238d71da48b8e1dd0e0f63ccf07a07266dd3b Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Tue, 8 Sep 2026 14:12:11 -0400 Subject: [PATCH 3/4] fix: own terminal command groups before forwarding cancellation --- docs/env.md | 11 +- index.ts | 12 +- src/lib/process-tree.ts | 84 ----------- src/lib/shell.ts | 91 ++++-------- src/lib/tty-process-group.ts | 58 ++++++++ src/lib/tty-run.ts | 153 ++++++++++++++++++++ src/lib/tty-supervisor.ts | 75 ++++++++++ tests/fixtures/tty-cancellation.py | 223 ++++++++++++++++++++++------- tests/host-exec-tty.test.ts | 163 +++++++++++++++++++-- 9 files changed, 648 insertions(+), 222 deletions(-) delete mode 100644 src/lib/process-tree.ts create mode 100644 src/lib/tty-process-group.ts create mode 100644 src/lib/tty-run.ts create mode 100644 src/lib/tty-supervisor.ts diff --git a/docs/env.md b/docs/env.md index 0210d161..74e837c9 100644 --- a/docs/env.md +++ b/docs/env.md @@ -243,11 +243,12 @@ hack host exec --env qa --scope api --target compose -- bun test ``` Cancelling `hack host exec` or `hack env exec` with SIGINT or SIGTERM forwards -the signal to the command. Noninteractive commands run in an owned process group, +the signal to the command. Commands run in an owned process group, so cancellation also stops their descendants, escalating to SIGKILL after two seconds if necessary. Hack returns 130 for SIGINT and 143 for SIGTERM. Interactive -commands retain their terminal process group so stdin and terminal job control -continue to work. SIGKILL cannot be forwarded; supervisors must terminate the +commands use a supervisor in their own foreground group on the same terminal, +preserving stdin, separate output streams, and Ctrl-Z/foreground resume. The +supervisor holds group ownership until cancellation cleanup finishes. SIGKILL cannot be forwarded; supervisors must terminate the whole owned process tree when force-killing a wrapper. Commands have no implicit time limit, and normal completion preserves the command's exit status. @@ -404,7 +405,3 @@ If you are writing new docs or new project setup flows, document the YAML overla - [Sessions](sessions.md) - [CLI reference](cli.md) - [Pulumi-style env config design](plans/2026-03-27-pulumi-style-env-config-design.md) - -For terminal commands cancelled through the wrapper PID, Hack captures the child -process tree and revalidates process start times before signalling surviving -descendants. It never signals the shared terminal process group. diff --git a/index.ts b/index.ts index a9c9a80e..43dbfa7e 100644 --- a/index.ts +++ b/index.ts @@ -1,6 +1,16 @@ #!/usr/bin/env bun import { runCli } from "./packages/cli/index.ts"; +import { + runTtySupervisor, + TTY_SUPERVISOR_ARGUMENT, +} from "./src/lib/tty-supervisor.ts"; -const exitCode = await runCli(Bun.argv.slice(2)); +const exitCode = + Bun.argv[2] === TTY_SUPERVISOR_ARGUMENT && process.send + ? await runTtySupervisor() + : await runCli(Bun.argv.slice(2)); +if (Bun.argv[2] === TTY_SUPERVISOR_ARGUMENT && process.send) { + process.exit(exitCode); +} process.exitCode = exitCode; diff --git a/src/lib/process-tree.ts b/src/lib/process-tree.ts deleted file mode 100644 index c354fc61..00000000 --- a/src/lib/process-tree.ts +++ /dev/null @@ -1,84 +0,0 @@ -export type ProcessIdentityRow = { - readonly pid: number; - readonly parentPid: number; - readonly birth: string; -}; -const IDENTITY_ROW = /^\s*(\d+)\s+(\d+)\s+(.+?)\s*$/; - -/** Capture only process identity and lineage; never command arguments or environment. */ -export async function readProcessIdentities(): Promise { - try { - const proc = Bun.spawn(["ps", "-A", "-o", "pid=,ppid=,lstart="], { - env: { ...process.env, LC_ALL: "C" }, - stdin: "ignore", - stdout: "pipe", - stderr: "ignore", - timeout: 2000, - }); - const output = await new Response(proc.stdout).text(); - if ((await proc.exited) !== 0) { - return []; - } - return output.split("\n").flatMap((line) => { - const match = IDENTITY_ROW.exec(line); - return match - ? [ - { - pid: Number(match[1]), - parentPid: Number(match[2]), - birth: (match[3] ?? "").replace(/\s+/g, " "), - }, - ] - : []; - }); - } catch { - return []; - } -} - -export function selectProcessTree( - rows: readonly ProcessIdentityRow[], - rootPid: number -): ProcessIdentityRow[] { - const children = new Map(); - for (const row of rows) { - const entries = children.get(row.parentPid) ?? []; - entries.push(row); - children.set(row.parentPid, entries); - } - const root = rows.find((row) => row.pid === rootPid); - const tree = root ? [root] : []; - const seen = new Set([rootPid]); - for (const parent of tree) { - for (const child of children.get(parent.pid) ?? []) { - if (!seen.has(child.pid)) { - seen.add(child.pid); - tree.push(child); - } - } - } - return tree; -} - -/** Revalidate start times before signalling a captured tree, including reparented descendants. */ -export async function signalVerifiedProcessTree( - tree: readonly ProcessIdentityRow[], - signal: NodeJS.Signals -): Promise { - if (!tree.length) { - return; - } - const live = new Map( - (await readProcessIdentities()).map((row) => [row.pid, row.birth]) - ); - for (const row of [...tree].reverse()) { - if (live.get(row.pid) !== row.birth) { - continue; - } - try { - process.kill(row.pid, signal); - } catch { - /* Process already exited. */ - } - } -} diff --git a/src/lib/shell.ts b/src/lib/shell.ts index 01e75ad4..7cdd2847 100644 --- a/src/lib/shell.ts +++ b/src/lib/shell.ts @@ -1,10 +1,3 @@ -import { - type ProcessIdentityRow, - readProcessIdentities, - selectProcessTree, - signalVerifiedProcessTree, -} from "./process-tree.ts"; - export interface ExecResult { readonly exitCode: number; readonly stdout: string; @@ -80,7 +73,7 @@ export interface RunOptions { */ readonly stdout?: "inherit" | "stderr"; readonly timeoutMs?: number; - /** Forward cancellation to the child, owning its process group when stdin is not a TTY. */ + /** Forward cancellation to an owned command process group, preserving TTY input. */ readonly forwardSignals?: boolean; } @@ -88,9 +81,22 @@ export async function run( cmd: readonly string[], opts: RunOptions = {} ): Promise { + if ( + opts.forwardSignals && + process.stdin.isTTY && + (opts.stdin ?? "inherit") === "inherit" + ) { + const { runWithTerminalGroup } = await import("./tty-run.ts"); + return await runWithTerminalGroup({ + command: cmd, + cwd: opts.cwd, + env: buildSpawnEnv(opts.env), + stdout: opts.stdout, + timeoutMs: opts.timeoutMs, + }); + } const ownsProcessGroup = - opts.timeoutMs !== undefined || - (opts.forwardSignals === true && !process.stdin.isTTY); + opts.timeoutMs !== undefined || opts.forwardSignals === true; const proc = Bun.spawn([...cmd], { cwd: opts.cwd, env: buildSpawnEnv(opts.env), @@ -106,8 +112,6 @@ export async function run( const cancellation = opts.forwardSignals ? installSubprocessSignalForwarding({ pid: proc.pid, - ownsProcessGroup, - childRunning: () => proc.exitCode === null, }) : null; try { @@ -115,67 +119,32 @@ export async function run( return cancellation?.exitCode() ?? (timeout.didTimeout() ? 124 : exitCode); } finally { timeout.dispose(); - await cancellation?.dispose(); + cancellation?.dispose(); } } -/** - * Noninteractive commands own a separate group so cancelling just the wrapper - * also stops descendants. TTY children keep their foreground group for stdin - * and terminal job control; the terminal delivers group signals itself. - */ -function installSubprocessSignalForwarding(opts: { - readonly pid: number; - readonly ownsProcessGroup: boolean; - readonly childRunning: () => boolean; -}): { - readonly dispose: () => Promise; +/** Detached noninteractive children keep cancellation scoped to their group. */ +function installSubprocessSignalForwarding(opts: { readonly pid: number }): { + readonly dispose: () => void; readonly exitCode: () => number | null; } { let exitCode: number | null = null; let forceKillTimer: ReturnType | null = null; const send = (signal: NodeJS.Signals): void => { - if (!(opts.ownsProcessGroup || opts.childRunning())) { - return; - } try { - process.kill(opts.ownsProcessGroup ? -opts.pid : opts.pid, signal); + process.kill(-opts.pid, signal); } catch { - // The owned process/group may already have exited. + // The owned group may already have exited. } }; - let ttyTree: ProcessIdentityRow[] = []; - let ttyCancellation: Promise = Promise.resolve(); - let ttyEscalation: Promise = Promise.resolve(); - const cancelTty = async (signal: NodeJS.Signals): Promise => { - ttyTree = selectProcessTree(await readProcessIdentities(), opts.pid); - await signalVerifiedProcessTree(ttyTree, signal); - if (!ttyTree.some((row) => row.pid === opts.pid)) { - send(signal); - } - forceKillTimer = setTimeout(() => { - ttyEscalation = signalVerifiedProcessTree(ttyTree, "SIGKILL"); - send("SIGKILL"); - }, 2000); - }; const cancel = (signal: "SIGINT" | "SIGTERM"): void => { if (exitCode !== null) { - if (opts.ownsProcessGroup) { - send("SIGKILL"); - } else { - ttyCancellation = ttyCancellation.then(() => - signalVerifiedProcessTree(ttyTree, "SIGKILL") - ); - } + send("SIGKILL"); return; } exitCode = signal === "SIGINT" ? 130 : 143; - if (opts.ownsProcessGroup) { - send(signal); - forceKillTimer = setTimeout(() => send("SIGKILL"), 2000); - } else { - ttyCancellation = cancelTty(signal); - } + send(signal); + forceKillTimer = setTimeout(() => send("SIGKILL"), 2000); }; const onInterrupt = (): void => cancel("SIGINT"); const onTerminate = (): void => cancel("SIGTERM"); @@ -183,21 +152,15 @@ function installSubprocessSignalForwarding(opts: { process.on("SIGTERM", onTerminate); return { exitCode: () => exitCode, - dispose: async () => { - await ttyCancellation; + dispose: () => { process.off("SIGINT", onInterrupt); process.off("SIGTERM", onTerminate); if (forceKillTimer) { clearTimeout(forceKillTimer); } - if (exitCode !== null && opts.ownsProcessGroup) { - // A cooperative child can exit before its stubborn descendants. + if (exitCode !== null) { send("SIGKILL"); } - await ttyEscalation; - if (exitCode !== null && !opts.ownsProcessGroup) { - await signalVerifiedProcessTree(ttyTree, "SIGKILL"); - } }, }; } diff --git a/src/lib/tty-process-group.ts b/src/lib/tty-process-group.ts new file mode 100644 index 00000000..c44d45b9 --- /dev/null +++ b/src/lib/tty-process-group.ts @@ -0,0 +1,58 @@ +import { dlopen, FFIType, type Pointer } from "bun:ffi"; +import { constants } from "node:os"; + +/** POSIX job control without a proxy PTY: all three command streams stay intact. */ +export function openTerminalControl() { + const symbols = { + setpgid: { args: [FFIType.i32, FFIType.i32], returns: FFIType.i32 }, + getpgrp: { args: [], returns: FFIType.i32 }, + tcgetpgrp: { args: [FFIType.i32], returns: FFIType.i32 }, + tcsetpgrp: { args: [FFIType.i32, FFIType.i32], returns: FFIType.i32 }, + tcgetattr: { args: [FFIType.i32, FFIType.ptr], returns: FFIType.i32 }, + tcsetattr: { + args: [FFIType.i32, FFIType.i32, FFIType.ptr], + returns: FFIType.i32, + }, + signal: { args: [FFIType.i32, FFIType.ptr], returns: FFIType.ptr }, + } as const; + const library = + process.platform === "darwin" ? "/usr/lib/libSystem.B.dylib" : "libc.so.6"; + const libc = dlopen(library, symbols); + function withoutBackgroundStop(operation: () => T): T { + const previous = libc.symbols.signal( + constants.signals.SIGTTOU, + 1 as Pointer + ); + try { + return operation(); + } finally { + libc.symbols.signal(constants.signals.SIGTTOU, previous); + } + } + return { + createGroup: () => libc.symbols.setpgid(0, 0) === 0, + group: () => libc.symbols.getpgrp(), + foreground: () => libc.symbols.tcgetpgrp(0), + setForeground: (group: number) => + withoutBackgroundStop(() => libc.symbols.tcsetpgrp(0, group) === 0), + attributes: () => { + // Opaque termios storage, larger than both Darwin and Linux structures. + const value = new Uint8Array(256); + return libc.symbols.tcgetattr(0, value) === 0 ? value : null; + }, + restoreAttributes: (value: Uint8Array | null) => { + if (value) { + withoutBackgroundStop(() => libc.symbols.tcsetattr(0, 0, value)); + } + }, + close: () => libc.close(), + }; +} + +export function signalOwnedGroup(pid: number, signal: NodeJS.Signals): void { + try { + process.kill(-pid, signal); + } catch { + // The owned group may already have exited. + } +} diff --git a/src/lib/tty-run.ts b/src/lib/tty-run.ts new file mode 100644 index 00000000..509ede26 --- /dev/null +++ b/src/lib/tty-run.ts @@ -0,0 +1,153 @@ +import { fileURLToPath } from "node:url"; +import { openTerminalControl, signalOwnedGroup } from "./tty-process-group.ts"; +import { TTY_SUPERVISOR_ARGUMENT } from "./tty-supervisor.ts"; + +/** Own descendants before starting the command while preserving its real TTY. */ +export async function runWithTerminalGroup(opts: { + readonly command: readonly string[]; + readonly cwd?: string; + readonly env: Record; + readonly stdout?: "inherit" | "stderr"; + readonly timeoutMs?: number; +}): Promise { + const terminal = openTerminalControl(); + const parentGroup = terminal.group(); + const originalAttributes = terminal.attributes(); + let suspendedAttributes: Uint8Array | null = null; + let ready = false; + let stopping = false; + let acknowledged = false; + let cancellationCode: number | null = null; + let escalation: ReturnType | undefined; + let timeout: ReturnType | undefined; + const entrypoint = fileURLToPath(new URL("../../index.ts", import.meta.url)); + const invocation = Bun.main.startsWith("/$bunfs/") + ? [process.execPath] + : [process.execPath, entrypoint]; + const child = Bun.spawn([...invocation, TTY_SUPERVISOR_ARGUMENT], { + cwd: opts.cwd, + env: opts.env, + stdin: "inherit", + stdout: opts.stdout === "stderr" ? 2 : "inherit", + stderr: "inherit", + ipc(message: unknown) { + if ( + typeof message !== "object" || + message === null || + !("kind" in message) + ) { + return; + } + if (message.kind === "ready") { + startCommand(); + } else if ( + message.kind === "cancel" && + "signal" in message && + (message.signal === "SIGINT" || message.signal === "SIGTERM") + ) { + cancel(message.signal, message.signal === "SIGINT" ? 130 : 143, false); + } else if (message.kind === "done") { + if (cancellationCode !== null) { + signalOwnedGroup(child.pid, "SIGKILL"); + } else { + acknowledged = true; + child.send("ack"); + } + } else if (message.kind === "stop") { + suspend(); + } + }, + }); + function startCommand(): void { + ready = true; + if (cancellationCode !== null) { + signalOwnedGroup(child.pid, "SIGKILL"); + return; + } + if ( + terminal.foreground() === parentGroup && + !terminal.setForeground(child.pid) + ) { + cancel("SIGTERM", 1, true); + return; + } + child.send({ kind: "start", command: [...opts.command] }); + } + function cancel( + signal: "SIGINT" | "SIGTERM", + code: number, + forward: boolean + ): void { + if (cancellationCode !== null) { + if (forward && ready) { + signalOwnedGroup(child.pid, "SIGKILL"); + } + return; + } + cancellationCode = code; + if (ready && forward) { + signalOwnedGroup(child.pid, signal); + } + escalation = setTimeout(() => { + if (ready) { + signalOwnedGroup(child.pid, "SIGKILL"); + } else { + child.kill("SIGKILL"); + } + }, 2000); + } + function restoreTerminal(): void { + if (terminal.foreground() === child.pid) { + terminal.setForeground(parentGroup); + } + } + function suspend(): void { + if (stopping || cancellationCode !== null) { + return; + } + stopping = true; + signalOwnedGroup(child.pid, "SIGSTOP"); + suspendedAttributes = terminal.attributes(); + restoreTerminal(); + terminal.restoreAttributes(originalAttributes); + process.kill(process.pid, "SIGSTOP"); + } + function resume(): void { + stopping = false; + if (ready) { + if (terminal.foreground() === parentGroup) { + terminal.setForeground(child.pid); + terminal.restoreAttributes(suspendedAttributes); + } + signalOwnedGroup(child.pid, "SIGCONT"); + } + } + const interrupt = () => cancel("SIGINT", 130, true); + const terminate = () => cancel("SIGTERM", 143, true); + process.on("SIGINT", interrupt); + process.on("SIGTERM", terminate); + process.on("SIGTSTP", suspend); + process.on("SIGCONT", resume); + if (opts.timeoutMs !== undefined) { + timeout = setTimeout(() => cancel("SIGTERM", 124, true), opts.timeoutMs); + } + try { + const code = await child.exited; + return cancellationCode ?? code; + } finally { + clearTimeout(timeout); + clearTimeout(escalation); + process.off("SIGINT", interrupt); + process.off("SIGTERM", terminate); + process.off("SIGTSTP", suspend); + process.off("SIGCONT", resume); + if (cancellationCode !== null || !acknowledged) { + if (ready) { + signalOwnedGroup(child.pid, "SIGKILL"); + } + terminal.restoreAttributes(originalAttributes); + } + restoreTerminal(); + terminal.close(); + } +} diff --git a/src/lib/tty-supervisor.ts b/src/lib/tty-supervisor.ts new file mode 100644 index 00000000..69441d67 --- /dev/null +++ b/src/lib/tty-supervisor.ts @@ -0,0 +1,75 @@ +import { openTerminalControl } from "./tty-process-group.ts"; + +export const TTY_SUPERVISOR_ARGUMENT = "--internal-tty-supervisor"; + +type StartMessage = { readonly kind: "start"; readonly command: string[] }; +function isStartMessage(value: unknown): value is StartMessage { + return ( + typeof value === "object" && + value !== null && + "kind" in value && + value.kind === "start" && + "command" in value && + Array.isArray(value.command) && + value.command.length > 0 && + value.command.every((part: unknown) => typeof part === "string") + ); +} + +/** Remain the group leader until the wrapper acknowledges completion. */ +export async function runTtySupervisor(): Promise { + if (!process.send) { + return 1; + } + const terminal = openTerminalControl(); + if (!terminal.createGroup()) { + throw new Error("Unable to create the command process group"); + } + let cancelled = false; + let completed: number | null = null; + let started = false; + const result = Promise.withResolvers(); + for (const signal of ["SIGINT", "SIGTERM"] as const) { + process.on(signal, () => { + cancelled = true; + process.send?.({ kind: "cancel", signal }); + }); + } + for (const signal of ["SIGTSTP", "SIGTTIN", "SIGTTOU"] as const) { + process.on(signal, () => process.send?.({ kind: "stop" })); + } + process.on("disconnect", () => { + // Losing the wrapper must not leave the command running unattended. + process.kill(-process.pid, "SIGKILL"); + }); + process.on("message", (message: unknown) => { + if (message === "ack" && completed !== null && !cancelled) { + result.resolve(completed); + } + if (!started && isStartMessage(message)) { + started = true; + try { + const child = Bun.spawn(message.command, { + stdin: "inherit", + stdout: "inherit", + stderr: "inherit", + env: process.env, + }); + child.exited.then((code) => { + completed = code; + process.send?.({ kind: "done" }); + }); + } catch (error) { + process.stderr.write( + `${error instanceof Error ? error.message : String(error)}\n` + ); + completed = 1; + process.send?.({ kind: "done" }); + } + } + }); + process.send({ kind: "ready" }); + const code = await result.promise; + terminal.close(); + return code; +} diff --git a/tests/fixtures/tty-cancellation.py b/tests/fixtures/tty-cancellation.py index 175bdb43..6a471cdf 100644 --- a/tests/fixtures/tty-cancellation.py +++ b/tests/fixtures/tty-cancellation.py @@ -8,70 +8,185 @@ import tempfile import time -if sys.argv[1] in ["worker", "grandchild"]: - signal.signal(signal.SIGHUP, signal.SIG_IGN) -if sys.argv[1] == "worker": - root = pathlib.Path(sys.argv[2]) - if sys.argv[3] == "ignore": - signal.signal(signal.SIGTERM, signal.SIG_IGN) - signal.signal(signal.SIGINT, signal.SIG_IGN) - else: - signal.signal(signal.SIGTERM, lambda *_: sys.exit(0)) - signal.signal(signal.SIGINT, lambda *_: sys.exit(0)) - child = subprocess.Popen([sys.executable, __file__, "grandchild", str(root)]) - (root / "child.pid").write_text(str(os.getpid())) +def write_json(path, value): + temporary = path.with_suffix('.tmp') + temporary.write_text(json.dumps(value)) + temporary.replace(path) + + +def worker(root, behavior): + signal.signal(signal.SIGHUP, signal.SIG_IGN) + signal.signal(signal.SIGTSTP, signal.SIG_DFL) + signal.signal(signal.SIGCONT, lambda *_: (root / 'continued').write_text('yes')) + for sig in [signal.SIGINT, signal.SIGTERM]: + signal.signal(sig, signal.SIG_IGN if behavior == 'ignore' else lambda *_: sys.exit(0)) + with open('/dev/tty') as tty: + write_json(root / 'tty.json', { + 'stdin': os.isatty(0), 'stdout': os.isatty(1), 'stderr': os.isatty(2), + 'devTty': os.isatty(tty.fileno()), + 'foreground': os.tcgetpgrp(0) == os.getpgrp(), + }) + # Publish the parent before spawning the grandchild: its readiness can never race this file. + (root / 'child.pid').write_text(str(os.getpid())) + if behavior != 'normal': + subprocess.Popen([sys.executable, __file__, 'grandchild', str(root)]) + if behavior in ['normal', 'io']: + (root / 'input.txt').write_text(sys.stdin.readline()) + print('child stdout', flush=True) + print('child stderr', file=sys.stderr, flush=True) + if behavior == 'normal': + sys.exit(7) while True: time.sleep(1) -elif sys.argv[1] == "grandchild": - signal.signal(signal.SIGTERM, signal.SIG_IGN) - signal.signal(signal.SIGINT, signal.SIG_IGN) - (pathlib.Path(sys.argv[2]) / "grandchild.pid").write_text(str(os.getpid())) + + +def launcher(root, bun, entrypoint, behavior): + # This shell stand-in survives terminal signals and observes foreground restoration. + for sig in [signal.SIGHUP, signal.SIGINT, signal.SIGTERM, signal.SIGTSTP]: + signal.signal(sig, signal.SIG_IGN) + original_group = os.tcgetpgrp(0) + sibling = subprocess.Popen([sys.executable, '-c', 'import time; time.sleep(60)']) + (root / 'sibling.pid').write_text(str(sibling.pid)) + wrapper = os.fork() + if wrapper == 0: + for sig in [signal.SIGHUP, signal.SIGINT, signal.SIGTERM, signal.SIGTSTP]: + signal.signal(sig, signal.SIG_DFL) + if behavior in ['normal', 'io']: + for number, name in [(1, 'stdout.txt'), (2, 'stderr.txt')]: + output = os.open(str(root / name), os.O_CREAT | os.O_TRUNC | os.O_WRONLY, 0o600) + os.dup2(output, number) + os.close(output) + env = dict(os.environ, HACK_HOME=str(root / 'state')) + os.execve(bun, [bun, *([entrypoint] if entrypoint else []), 'host', 'exec', '--path', str(root), '--no-interactive', '--', sys.executable, __file__, 'worker', str(root), behavior], env) + (root / 'wrapper.pid').write_text(str(wrapper)) while True: - time.sleep(1) -else: - bun, entrypoint, requested_signal, behavior = sys.argv[2:] - with tempfile.TemporaryDirectory(prefix="hack-tty-") as tmp: - root = pathlib.Path(tmp) - config = root / ".hack" + got, status = os.waitpid(wrapper, os.WNOHANG | os.WUNTRACED | os.WCONTINUED) + if got and os.WIFSTOPPED(status): + write_json(root / 'stopped.json', {'foregroundRestored': os.tcgetpgrp(0) == original_group}) + if (root / 'resume.request').exists(): + (root / 'resume.request').unlink() + os.tcsetpgrp(0, original_group) + os.killpg(os.getpgid(wrapper), signal.SIGCONT) + if got and (os.WIFEXITED(status) or os.WIFSIGNALED(status)): + write_json(root / 'completion.json', { + 'exit': os.waitstatus_to_exitcode(status), + 'foregroundRestored': os.tcgetpgrp(0) == original_group, + }) + # Keep the terminal alive until the outer harness verifies the result. + while True: + time.sleep(1) + time.sleep(.01) + + +def process_alive(pid): + result = subprocess.run(['ps', '-p', str(pid), '-o', 'stat='], capture_output=True, text=True) + return result.returncode == 0 and not result.stdout.strip().startswith('Z') + + +def probe(bun, entrypoint, requested_signal, behavior, mode): + with tempfile.TemporaryDirectory(prefix='hack-tty-') as temporary: + root = pathlib.Path(temporary) + config = root / '.hack' config.mkdir() - (config / "hack.config.json").write_text('{"name":"tty-regression"}') - (config / "docker-compose.yml").write_text("services:\n noop:\n image: alpine:3.20\n") - (config / "hack.env.default.yaml").write_text("version: 1\nenvironment: default\nsecretsprovider: project_key\nvalues:\n global: {}\n") - wrapper, fd = pty.fork() - if wrapper == 0: - sibling = subprocess.Popen([sys.executable, "-c", "import signal,time; signal.signal(signal.SIGHUP, signal.SIG_IGN); time.sleep(30)"]) - (root / "sibling.pid").write_text(str(sibling.pid)) - env = dict(os.environ, HACK_HOME=str(root / "state")) - os.execve(bun, [bun, entrypoint, "host", "exec", "--path", str(root), "--no-interactive", "--", sys.executable, __file__, "worker", str(root), behavior], env) - status = None - def alive(pid): - result = subprocess.run(["ps", "-p", str(pid), "-o", "stat="], capture_output=True, text=True) - return result.returncode == 0 and not result.stdout.strip().startswith("Z") - try: - deadline = time.monotonic() + 10 - while not (root / "grandchild.pid").exists() and time.monotonic() < deadline: - time.sleep(.025) - child = int((root / "child.pid").read_text()) - grandchild = int((root / "grandchild.pid").read_text()) - sibling = int((root / "sibling.pid").read_text()) - os.kill(wrapper, getattr(signal, requested_signal)) - deadline = time.monotonic() + 8 + (config / 'hack.config.json').write_text('{"name":"tty-regression"}') + (config / 'docker-compose.yml').write_text('services:\n noop:\n image: alpine:3.20\n') + (config / 'hack.env.default.yaml').write_text('version: 1\nenvironment: default\nsecretsprovider: project_key\nvalues:\n global: {}\n') + shell, fd = pty.fork() + if shell == 0: + launcher(root, bun, entrypoint, behavior) + os._exit(0) + os.set_blocking(fd, False) + terminal_output = bytearray() + + def drain(): + while True: + try: + data = os.read(fd, 65536) + if not data: + return + terminal_output.extend(data) + except (BlockingIOError, OSError): + return + + def wait_for(names, seconds=8): + deadline = time.monotonic() + seconds while time.monotonic() < deadline: - got, result = os.waitpid(wrapper, os.WNOHANG) - if got: - status = result - break - time.sleep(.025) - print(json.dumps({"exit": os.waitstatus_to_exitcode(status) if status is not None else None, "childAlive": alive(child), "grandchildAlive": alive(grandchild), "siblingAlive": alive(sibling)})) + drain() + if all((root / name).exists() for name in names): + return True + time.sleep(.01) + return False + + try: + ready = ['child.pid', 'sibling.pid', 'wrapper.pid', 'tty.json'] + if behavior != 'normal': + ready.append('grandchild.pid') + if not wait_for(ready, 10): + raise RuntimeError('TTY fixture did not start: ' + terminal_output.decode(errors='replace')) + wrapper = int((root / 'wrapper.pid').read_text()) + stopped = None + if mode == 'resume': + os.write(fd, b'\x1a') + if not wait_for(['stopped.json']): + raise RuntimeError('Wrapper did not stop after foreground Ctrl-Z') + stopped = json.loads((root / 'stopped.json').read_text()) + (root / 'resume.request').write_text('resume') + if not wait_for(['continued']): + raise RuntimeError('Command did not resume after fg') + if behavior in ['normal', 'io']: + os.write(fd, b'hello tty\n') + if not wait_for(['input.txt']): + raise RuntimeError('Command did not read terminal stdin') + if behavior != 'normal': + if mode == 'paused': + os.kill(wrapper, signal.SIGSTOP) + if mode in ['foreground', 'paused', 'resume']: + os.write(fd, b'\x03') + else: + os.kill(wrapper, getattr(signal, requested_signal)) + if mode == 'paused': + time.sleep(.15) + os.kill(wrapper, signal.SIGCONT) + completed = wait_for(['completion.json']) + drain() + completion = json.loads((root / 'completion.json').read_text()) if completed else {'exit': None, 'foregroundRestored': False} + output = { + **completion, + 'childAlive': process_alive(int((root / 'child.pid').read_text())), + 'grandchildAlive': process_alive(int((root / 'grandchild.pid').read_text())) if (root / 'grandchild.pid').exists() else False, + 'siblingAlive': process_alive(int((root / 'sibling.pid').read_text())), + 'tty': json.loads((root / 'tty.json').read_text()), + 'stopped': stopped, + } + if behavior in ['normal', 'io']: + output.update({name: (root / path).read_text() for name, path in [('input', 'input.txt'), ('stdout', 'stdout.txt'), ('stderr', 'stderr.txt')]}) + print(json.dumps(output)) finally: - for name in ["child.pid", "grandchild.pid", "sibling.pid"]: + # Every PID below was created by this fixture. Never signal the shared outer group. + for name in ['child.pid', 'grandchild.pid', 'wrapper.pid', 'sibling.pid']: if (root / name).exists(): try: os.kill(int((root / name).read_text()), signal.SIGKILL) except ProcessLookupError: pass - if status is None: - os.kill(wrapper, signal.SIGKILL) - os.waitpid(wrapper, 0) + drain() os.close(fd) + try: + os.kill(shell, signal.SIGKILL) + except ProcessLookupError: + pass + os.waitpid(shell, 0) + + +if sys.argv[1] == 'worker': + worker(pathlib.Path(sys.argv[2]), sys.argv[3]) +elif sys.argv[1] == 'grandchild': + for sig in [signal.SIGHUP, signal.SIGINT, signal.SIGTERM]: + signal.signal(sig, signal.SIG_IGN) + signal.signal(signal.SIGTSTP, signal.SIG_DFL) + (pathlib.Path(sys.argv[2]) / 'grandchild.pid').write_text(str(os.getpid())) + while True: + time.sleep(1) +else: + probe(*sys.argv[2:6], sys.argv[6] if len(sys.argv) > 6 else 'wrapper') diff --git a/tests/host-exec-tty.test.ts b/tests/host-exec-tty.test.ts index 55aed5ce..728c8871 100644 --- a/tests/host-exec-tty.test.ts +++ b/tests/host-exec-tty.test.ts @@ -1,13 +1,70 @@ import { expect, test } from "bun:test"; -import { resolve } from "node:path"; +import { mkdtemp, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join, resolve } from "node:path"; -for (const [signal, behavior, code] of [ - ["SIGTERM", "exit", 143], - ["SIGINT", "exit", 130], - ["SIGTERM", "ignore", 143], -] as const) { +const cases = [ + { + name: "wrapper-only TERM", + signal: "SIGTERM", + behavior: "exit", + mode: "wrapper", + code: 143, + }, + { + name: "wrapper-only INT", + signal: "SIGINT", + behavior: "exit", + mode: "wrapper", + code: 130, + }, + { + name: "stubborn wrapper-only TERM", + signal: "SIGTERM", + behavior: "ignore", + mode: "wrapper", + code: 143, + }, + { + name: "immediate foreground Ctrl-C", + signal: "SIGINT", + behavior: "exit", + mode: "foreground", + code: 130, + }, + { + name: "foreground Ctrl-C while wrapper paused", + signal: "SIGINT", + behavior: "exit", + mode: "paused", + code: 130, + }, + { + name: "terminal input with separate output", + signal: "SIGTERM", + behavior: "io", + mode: "wrapper", + code: 143, + }, + { + name: "normal command return", + signal: "SIGTERM", + behavior: "normal", + mode: "wrapper", + code: 7, + }, + { + name: "Ctrl-Z and fg resume", + signal: "SIGINT", + behavior: "exit", + mode: "resume", + code: 130, + }, +] as const; + +for (const scenario of cases) { test.skipIf(!Bun.which("python3"))( - `TTY wrapper-only ${signal} cleans descendants (${behavior}) without signalling a sibling`, + `TTY ${scenario.name} preserves terminal ownership and cleans only owned processes`, async () => { const proc = Bun.spawn( [ @@ -16,8 +73,9 @@ for (const [signal, behavior, code] of [ "probe", process.execPath, resolve(import.meta.dir, "../index.ts"), - signal, - behavior, + scenario.signal, + scenario.behavior, + scenario.mode, ], { stdin: "ignore", stdout: "pipe", stderr: "pipe" } ); @@ -28,13 +86,94 @@ for (const [signal, behavior, code] of [ ]); expect(stderr).toBe(""); expect(exitCode).toBe(0); - expect(JSON.parse(stdout)).toEqual({ - exit: code, + const outcome = JSON.parse(stdout); + expect(outcome).toMatchObject({ + exit: scenario.code, + foregroundRestored: true, childAlive: false, grandchildAlive: false, siblingAlive: true, + tty: { stdin: true, devTty: true, foreground: true }, }); + if (scenario.behavior === "normal" || scenario.behavior === "io") { + expect(outcome).toMatchObject({ + input: "hello tty\n", + stdout: "child stdout\n", + stderr: "child stderr\n", + tty: { stdout: false, stderr: false }, + }); + } + if (scenario.mode === "resume") { + expect(outcome.stopped).toEqual({ foregroundRestored: true }); + } }, - 20_000 + 25_000 ); } + +test.skipIf(!Bun.which("python3"))( + "compiled TTY supervisor survives foreground Ctrl-C while its wrapper is paused", + async () => { + const directory = await mkdtemp(join(tmpdir(), "hack-compiled-tty-")); + try { + const binary = join(directory, "hack"); + const build = Bun.spawn( + [ + process.execPath, + "build", + "index.ts", + "--compile", + "--outfile", + binary, + ], + { + cwd: resolve(import.meta.dir, ".."), + stdin: "ignore", + stdout: "pipe", + stderr: "pipe", + } + ); + const [buildStdout, buildStderr, buildExit] = await Promise.all([ + new Response(build.stdout).text(), + new Response(build.stderr).text(), + build.exited, + ]); + expect({ + exit: buildExit, + stdout: buildStdout, + stderr: buildStderr, + }).toMatchObject({ exit: 0 }); + const probe = Bun.spawn( + [ + "python3", + resolve(import.meta.dir, "fixtures/tty-cancellation.py"), + "probe", + binary, + "", + "SIGINT", + "exit", + "paused", + ], + { stdin: "ignore", stdout: "pipe", stderr: "pipe" } + ); + const [stdout, stderr, exit] = await Promise.all([ + new Response(probe.stdout).text(), + new Response(probe.stderr).text(), + probe.exited, + ]); + expect(stderr).toBe(""); + expect(exit).toBe(0); + expect(JSON.parse(stdout)).toMatchObject({ + exit: 130, + foregroundRestored: true, + childAlive: false, + grandchildAlive: false, + siblingAlive: true, + tty: { stdin: true, devTty: true, foreground: true }, + }); + } finally { + await rm(directory, { recursive: true, force: true }); + } + }, + 45_000 +); From 8091e3a5adadaa5d37f56adafb9119c9925f4c84 Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Tue, 8 Sep 2026 14:18:33 -0400 Subject: [PATCH 4/4] fix: skip CLI initialization in terminal supervisor --- index.ts | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/index.ts b/index.ts index 43dbfa7e..ab105c0e 100644 --- a/index.ts +++ b/index.ts @@ -1,16 +1,12 @@ #!/usr/bin/env bun -import { runCli } from "./packages/cli/index.ts"; import { runTtySupervisor, TTY_SUPERVISOR_ARGUMENT, } from "./src/lib/tty-supervisor.ts"; -const exitCode = - Bun.argv[2] === TTY_SUPERVISOR_ARGUMENT && process.send - ? await runTtySupervisor() - : await runCli(Bun.argv.slice(2)); if (Bun.argv[2] === TTY_SUPERVISOR_ARGUMENT && process.send) { - process.exit(exitCode); + process.exit(await runTtySupervisor()); } -process.exitCode = exitCode; +const { runCli } = await import("./packages/cli/index.ts"); +process.exitCode = await runCli(Bun.argv.slice(2));