diff --git a/.changeset/quiet-terminals-exit.md b/.changeset/quiet-terminals-exit.md new file mode 100644 index 00000000..2068b36b --- /dev/null +++ b/.changeset/quiet-terminals-exit.md @@ -0,0 +1,5 @@ +--- +"hunkdiff": patch +--- + +Exit cleanly when the terminal hosting a review disconnects instead of leaving an unreachable Hunk process behind. diff --git a/src/core/terminal.test.ts b/src/core/terminal.test.ts index 67a3e728..a20f20e8 100644 --- a/src/core/terminal.test.ts +++ b/src/core/terminal.test.ts @@ -1,6 +1,7 @@ import { describe, expect, test } from "bun:test"; import type { CliInput } from "./types"; import { + installTerminalDisconnectSupport, openControllingTerminal, resolveRuntimeCliInput, shouldUseMouseForApp, @@ -8,6 +9,36 @@ import { usesPipedPatchInput, } from "./terminal"; +function createTestTerminalInputEvents( + state: { isTTY?: boolean; destroyed?: boolean; readableEnded?: boolean } = {}, +) { + const listeners = new Map void>>(); + + return { + isTTY: true, + ...state, + emit(event: "close" | "end" | "error") { + for (const listener of listeners.get(event) ?? []) { + listener(); + } + }, + listenerCount(event: "close" | "end" | "error") { + return listeners.get(event)?.size ?? 0; + }, + on(event: "close" | "end" | "error", listener: (...args: unknown[]) => void) { + let eventListeners = listeners.get(event); + if (!eventListeners) { + eventListeners = new Set(); + listeners.set(event, eventListeners); + } + eventListeners.add(listener); + }, + off(event: "close" | "end" | "error", listener: (...args: unknown[]) => void) { + listeners.get(event)?.delete(listener); + }, + }; +} + function createPatchInput(file?: string, pager = false): CliInput { return { kind: "patch", @@ -114,3 +145,69 @@ describe("controlling terminal attachment", () => { expect(controllingTerminal).toBeNull(); }); }); + +describe("terminal disconnect support", () => { + test.each(["close", "end", "error"] as const)("shuts down once on %s", (event) => { + const input = createTestTerminalInputEvents(); + let disconnectCalls = 0; + installTerminalDisconnectSupport(input, () => { + disconnectCalls += 1; + }); + + input.emit(event); + input.emit(event); + + expect(disconnectCalls).toBe(1); + }); + + test("dispose removes every input listener", () => { + const input = createTestTerminalInputEvents(); + let disconnectCalls = 0; + const support = installTerminalDisconnectSupport(input, () => { + disconnectCalls += 1; + }); + + support.dispose(); + + expect(input.listenerCount("close")).toBe(0); + expect(input.listenerCount("end")).toBe(0); + expect(input.listenerCount("error")).toBe(0); + input.emit("close"); + expect(disconnectCalls).toBe(0); + }); + + test.each(["destroyed", "readableEnded"] as const)( + "shuts down when input is already %s", + async (state) => { + const input = createTestTerminalInputEvents({ [state]: true }); + let disconnectCalls = 0; + installTerminalDisconnectSupport(input, () => { + disconnectCalls += 1; + }); + + await Promise.resolve(); + + expect(disconnectCalls).toBe(1); + }, + ); + + // Non-interactive input ends the moment the renderer resumes it. + test.each([{ isTTY: false }, { isTTY: undefined }])( + "ignores non-terminal input (%o)", + async (state) => { + const input = createTestTerminalInputEvents({ ...state, readableEnded: true }); + let disconnectCalls = 0; + installTerminalDisconnectSupport(input, () => { + disconnectCalls += 1; + }); + + await Promise.resolve(); + input.emit("end"); + input.emit("close"); + input.emit("error"); + + expect(input.listenerCount("end")).toBe(0); + expect(disconnectCalls).toBe(0); + }, + ); +}); diff --git a/src/core/terminal.ts b/src/core/terminal.ts index ccfd28d3..dcd46606 100644 --- a/src/core/terminal.ts +++ b/src/core/terminal.ts @@ -44,6 +44,59 @@ export interface ControllingTerminal { close: () => void; } +type TerminalDisconnectEvent = "close" | "end" | "error"; + +export interface TerminalInputEvents { + isTTY?: boolean; + destroyed?: boolean; + readableEnded?: boolean; + on: (event: TerminalDisconnectEvent, listener: (...args: unknown[]) => void) => unknown; + off: (event: TerminalDisconnectEvent, listener: (...args: unknown[]) => void) => unknown; +} + +export interface TerminalDisconnectSupport { + dispose: () => void; +} + +/** Shut the app down when its renderer input is closed or revoked by the terminal host. */ +export function installTerminalDisconnectSupport( + input: TerminalInputEvents, + onDisconnect: () => void, +): TerminalDisconnectSupport { + if (input.isTTY !== true) { + return { dispose: () => undefined }; + } + + const events: TerminalDisconnectEvent[] = ["close", "end", "error"]; + let disposed = false; + + const disconnect = () => { + if (disposed) { + return; + } + disposed = true; + onDisconnect(); + }; + + for (const event of events) { + input.on(event, disconnect); + } + + // Stream ended before listeners handle disconnect + if (input.destroyed || input.readableEnded) { + queueMicrotask(disconnect); + } + + return { + dispose: () => { + disposed = true; + for (const event of events) { + input.off(event, disconnect); + } + }, + }; +} + /** Minimal terminal construction hooks so tests can cover `/dev/tty` attach behavior. */ export interface ControllingTerminalDeps { openSync: typeof fs.openSync; diff --git a/src/ui/runInteractiveApp.tsx b/src/ui/runInteractiveApp.tsx index 8ffd4180..44077b4d 100644 --- a/src/ui/runInteractiveApp.tsx +++ b/src/ui/runInteractiveApp.tsx @@ -7,7 +7,12 @@ import { type JobControlSuspendSupport, } from "../core/jobControl"; import { shutdownSession } from "../core/shutdown"; -import { shouldUseMouseForApp, type ControllingTerminal } from "../core/terminal"; +import { + installTerminalDisconnectSupport, + shouldUseMouseForApp, + type ControllingTerminal, + type TerminalDisconnectSupport, +} from "../core/terminal"; import type { AppBootstrap } from "../core/types"; import { resolveStartupUpdateNotice } from "../core/updateNotice"; import { ReviewProducer } from "../app/review/producer"; @@ -29,6 +34,12 @@ export interface InteractiveAppInput { controllingTerminal: ControllingTerminal | null; } +// Leave fatal process faults to their default OS disposition. +const APP_SHUTDOWN_SIGNALS: NodeJS.Signals[] = + process.platform === "win32" + ? ["SIGINT", "SIGTERM", "SIGBREAK"] + : ["SIGINT", "SIGTERM", "SIGHUP", "SIGQUIT", "SIGPIPE"]; + /** Load and run the OpenTUI review app after startup has selected an interactive plan. */ export async function runInteractiveApp({ bootstrap, @@ -54,25 +65,28 @@ export async function runInteractiveApp({ hostClient.start(); // Keep OpenTUI's platform-safe threading default (enabled on macOS, disabled on Linux). + const rendererStdin = controllingTerminal?.stdin ?? process.stdin; const renderer = await createCliRenderer({ - stdin: controllingTerminal?.stdin, + stdin: rendererStdin, stdout: process.stdout, useMouse: shouldUseMouseForApp({ hasControllingTerminal: Boolean(controllingTerminal), }), screenMode: "alternate-screen", exitOnCtrlC: false, + // OpenTUI's destroy-only handlers can strand sessions with active broker handles. + exitSignals: [], openConsoleOnError: true, onDestroy: () => controllingTerminal?.close(), }); const appRenderer = renderer; const root = createRoot(appRenderer); - const shutdownSignals: NodeJS.Signals[] = ["SIGINT", "SIGTERM"]; const externalQuitController = new AbortController(); let shuttingDown = false; let jobControlSuspendSupport: JobControlSuspendSupport = { dispose: () => undefined }; let jobControlInterruptSupport: JobControlInterruptSupport = { dispose: () => undefined }; + let terminalDisconnectSupport: TerminalDisconnectSupport = { dispose: () => undefined }; /** Ask AppHost to retire extension authority before tearing down the terminal. */ function requestQuit() { @@ -86,18 +100,21 @@ export async function runInteractiveApp({ } shuttingDown = true; - for (const signal of shutdownSignals) { + for (const signal of APP_SHUTDOWN_SIGNALS) { process.off(signal, requestQuit); } jobControlInterruptSupport.dispose(); jobControlSuspendSupport.dispose(); + terminalDisconnectSupport.dispose(); hostClient.stop(); shutdownSession({ root, renderer: appRenderer }); } - for (const signal of shutdownSignals) { + for (const signal of APP_SHUTDOWN_SIGNALS) { process.once(signal, requestQuit); } + // Install after the renderer so a disconnect closes the live session instead of racing startup. + terminalDisconnectSupport = installTerminalDisconnectSupport(rendererStdin, requestQuit); jobControlInterruptSupport = installJobControlInterruptSupport(appRenderer, requestQuit); jobControlSuspendSupport = installJobControlSuspendSupport(appRenderer); diff --git a/test/cli/non-interactive-stdin.test.ts b/test/cli/non-interactive-stdin.test.ts new file mode 100644 index 00000000..4b620bec --- /dev/null +++ b/test/cli/non-interactive-stdin.test.ts @@ -0,0 +1,72 @@ +import { describe, expect, test } from "bun:test"; +import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +const MINIMUM_RENDERED_BYTES = 1_000; + +async function readUntilRendered( + stream: ReadableStream, + minimumBytes: number, + timeoutMs: number, +) { + const reader = stream.getReader(); + const deadline = Date.now() + timeoutMs; + let bytes = 0; + + try { + while (bytes < minimumBytes && Date.now() < deadline) { + const next = await Promise.race([ + reader.read(), + Bun.sleep(Math.max(0, deadline - Date.now())).then(() => "timeout" as const), + ]); + if (next === "timeout" || next.done) { + break; + } + bytes += next.value.length; + } + } finally { + reader.releaseLock(); + } + + return bytes; +} + +describe("non-interactive stdin contracts", () => { + test("renders the review and stays alive when stdin is not a terminal", async () => { + const dir = mkdtempSync(join(tmpdir(), "hunk-non-tty-stdin-")); + const before = join(dir, "before.ts"); + const after = join(dir, "after.ts"); + writeFileSync(before, "export const value = 1;\n"); + writeFileSync(after, "export const value = 2;\n"); + + const proc = Bun.spawn(["bun", "run", "src/main.tsx", "--", "diff", before, after], { + cwd: process.cwd(), + stdin: "ignore", + stdout: "pipe", + stderr: "pipe", + env: { + ...process.env, + TERM: "xterm-256color", + HUNK_MCP_DISABLE: "1", + HUNK_DISABLE_UPDATE_NOTICE: "1", + XDG_CONFIG_HOME: dir, + }, + }); + + try { + const bytes = await readUntilRendered(proc.stdout, MINIMUM_RENDERED_BYTES, 15_000); + expect(bytes).toBeGreaterThanOrEqual(MINIMUM_RENDERED_BYTES); + await expect( + Promise.race([ + proc.exited.then((code) => ({ exited: true, code })), + Bun.sleep(250).then(() => ({ exited: false })), + ]), + ).resolves.toEqual({ exited: false }); + } finally { + proc.kill(); + await proc.exited; + rmSync(dir, { recursive: true, force: true }); + } + }, 30_000); +}); diff --git a/test/pty/lifecycle.test.ts b/test/pty/lifecycle.test.ts new file mode 100644 index 00000000..d260f6e5 --- /dev/null +++ b/test/pty/lifecycle.test.ts @@ -0,0 +1,319 @@ +import { afterEach, describe, expect, setDefaultTimeout, test } from "bun:test"; +import { dlopen, FFIType, ptr, type Library } from "bun:ffi"; +import { spawn, type ChildProcess } from "node:child_process"; +import { closeSync, existsSync, read, readdirSync, readFileSync } from "node:fs"; +import { join } from "node:path"; +import { createPtyHarness } from "./harness"; + +const harness = createPtyHarness(); + +setDefaultTimeout(30_000); + +afterEach(() => { + harness.cleanup(); +}); + +interface ChildExit { + code: number | null; + signal: NodeJS.Signals | null; +} + +/** Wait for a child to exit and preserve whether it returned or died from a signal. */ +function waitForChildExit(child: ChildProcess, timeoutMs = 2_000): Promise { + if (child.exitCode !== null || child.signalCode !== null) { + return Promise.resolve({ code: child.exitCode, signal: child.signalCode }); + } + + return new Promise((resolve, reject) => { + const onExit = (code: number | null, signal: NodeJS.Signals | null) => { + clearTimeout(timer); + resolve({ code, signal }); + }; + const timer = setTimeout(() => { + child.off("exit", onExit); + reject(new Error(`Timed out waiting for process ${child.pid} to exit.`)); + }, timeoutMs); + child.once("exit", onExit); + }); +} + +/** Kill a child left behind by a failed assertion and wait for the process to be terminated. */ +async function stopChild(child: ChildProcess) { + if (child.exitCode === null && child.signalCode === null) { + child.kill("SIGKILL"); + } + await waitForChildExit(child).catch(() => undefined); +} + +/** Wait for a shell supervisor to record the reviewed process's exit code. */ +async function waitForExitCode(path: string, timeoutMs = 2_000) { + const deadline = Date.now() + timeoutMs; + while (Date.now() < deadline) { + if (existsSync(path)) { + return Number.parseInt(readFileSync(path, "utf8"), 10); + } + await Bun.sleep(25); + } + throw new Error(`Timed out waiting for an exit code in ${path}.`); +} + +async function stopDaemonsUnder(runtimeDir: string) { + const daemonDir = join(runtimeDir, "hunk-mcp"); + const deadline = Date.now() + 2_000; + + while (Date.now() < deadline) { + if (existsSync(daemonDir)) { + const metadataFiles = readdirSync(daemonDir).filter( + (entry) => entry.startsWith("daemon-") && entry.endsWith(".json"), + ); + if (metadataFiles.length > 0) { + for (const entry of metadataFiles) { + try { + const { pid } = JSON.parse(readFileSync(join(daemonDir, entry), "utf8")) as { + pid?: number; + }; + if (pid && pid > 0) { + process.kill(pid, "SIGTERM"); + } + } catch { + // Partially written metadata, or a daemon that already exited. + } + } + return; + } + } + await Bun.sleep(25); + } +} + +function revokeTerminal(path: string) { + const libc = dlopen("/usr/lib/libSystem.B.dylib", { + revoke: { + args: [FFIType.cstring], + returns: FFIType.i32, + }, + }); + try { + return libc.symbols.revoke(ptr(Buffer.from(`${path}\0`))); + } finally { + libc.close(); + } +} + +const OPENPTY_LIBRARIES = + process.platform === "darwin" + ? ["/usr/lib/libSystem.B.dylib"] + : ["libutil.so.1", "libc.so.6", "libc.so"]; + +const OPENPTY_SYMBOLS = { + openpty: { + args: [FFIType.ptr, FFIType.ptr, FFIType.ptr, FFIType.ptr, FFIType.ptr], + returns: FFIType.i32, + }, +} as const; + +/** + * Allocate a PTY pair so test can drop the master while child keeps slave + */ +function openPtyPair({ rows = 24, columns = 140 } = {}) { + let libc: Library | undefined; + for (const candidate of OPENPTY_LIBRARIES) { + try { + libc = dlopen(candidate, OPENPTY_SYMBOLS); + break; + } catch { + // Try the next platform candidate. + } + } + if (!libc) { + throw new Error(`No libc with openpty found (tried ${OPENPTY_LIBRARIES.join(", ")}).`); + } + + try { + const fds = new Int32Array(2); + const winsize = new Uint16Array([rows, columns, 0, 0]); + const result = libc.symbols.openpty( + ptr(fds), + ptr(fds, Int32Array.BYTES_PER_ELEMENT), + null, + null, + ptr(winsize), + ); + if (result !== 0) { + throw new Error(`openpty failed with ${result}.`); + } + + return { master: fds[0]!, slave: fds[1]! }; + } finally { + libc.close(); + } +} + +/** Read the PTY master until the app has rendered so disconnect lands on live session. */ +async function waitForPtyOutput(fd: number, pattern: RegExp, timeoutMs = 20_000) { + const deadline = Date.now() + timeoutMs; + const buffer = Buffer.alloc(64 * 1024); + let text = ""; + + while (Date.now() < deadline) { + const bytes = await new Promise((resolve, reject) => { + read(fd, buffer, 0, buffer.length, null, (error, bytesRead) => { + if (error) { + reject(error); + return; + } + resolve(bytesRead); + }); + }); + if (bytes === 0) { + break; + } + + text += buffer.subarray(0, bytes).toString("utf8"); + if (pattern.test(text)) { + return text; + } + } + + throw new Error(`Timed out waiting for ${pattern} on the PTY. Saw:\n${text}`); +} + +/** Read child output until the app paints, then leave the stream flowing for teardown. */ +function waitForStreamOutput(stream: NodeJS.ReadableStream, pattern: RegExp, timeoutMs = 20_000) { + return new Promise((resolve, reject) => { + let text = ""; + const cleanup = () => { + clearTimeout(timer); + stream.off("data", onData); + stream.off("end", onEnd); + }; + const onData = (chunk: Buffer | string) => { + text += Buffer.isBuffer(chunk) ? chunk.toString("utf8") : chunk; + if (pattern.test(text)) { + cleanup(); + stream.resume(); + resolve(); + } + }; + const onEnd = () => { + cleanup(); + reject(new Error(`Child output ended before ${pattern}. Saw:\n${text}`)); + }; + const timer = setTimeout(() => { + cleanup(); + reject(new Error(`Timed out waiting for ${pattern}. Saw:\n${text}`)); + }, timeoutMs); + + stream.on("data", onData); + stream.on("end", onEnd); + }); +} + +describe("PTY lifecycle", () => { + for (const signal of ["SIGHUP", "SIGQUIT", "SIGPIPE"] as const) { + test.skipIf(process.platform === "win32")(`exits cleanly on ${signal}`, async () => { + const fixture = harness.createLongWrapFilePair(); + const runtimeDir = harness.createIsolatedConfigHome(); + const hunkCommand = harness.buildHunkCommand(["diff", fixture.before, fixture.after]); + const child = spawn("/bin/sh", ["-c", `exec ${hunkCommand}`], { + cwd: fixture.dir, + stdio: ["pipe", "pipe", "pipe"], + env: { + ...process.env, + TERM: "xterm-256color", + XDG_CONFIG_HOME: harness.createIsolatedConfigHome(), + XDG_RUNTIME_DIR: runtimeDir, + // Brokering exposes an issue: disabled, passes on unfixed code. + HUNK_MCP_DISABLE: "0", + HUNK_DISABLE_UPDATE_NOTICE: "1", + }, + }); + child.stderr?.resume(); + + try { + await waitForStreamOutput(child.stdout!, /this is a very long wrapped line/); + process.kill(child.pid!, signal); + + await expect(waitForChildExit(child)).resolves.toEqual({ code: 0, signal: null }); + } finally { + await stopChild(child); + await stopDaemonsUnder(runtimeDir); + } + }); + } + + // Windows has no PTY slave to strand, and the disconnect signal there is not a stream event. + test.skipIf(process.platform === "win32")( + "exits when the host closes the PTY master", + async () => { + const fixture = harness.createLongWrapFilePair(); + const { master, slave } = openPtyPair(); + const hunkCommand = harness.buildHunkCommand(["diff", fixture.before, fixture.after]); + // `exec` to keep the pid pointing at Hunk + const child = spawn("/bin/sh", ["-c", `exec ${hunkCommand}`], { + cwd: fixture.dir, + stdio: [slave, slave, slave], + env: { + ...process.env, + TERM: "xterm-256color", + XDG_CONFIG_HOME: harness.createIsolatedConfigHome(), + HUNK_MCP_DISABLE: "1", + HUNK_DISABLE_UPDATE_NOTICE: "1", + }, + }); + + closeSync(slave); + expect(child.pid).toBeGreaterThan(0); + + let masterClosed = false; + const closeMaster = () => { + if (!masterClosed) { + masterClosed = true; + closeSync(master); + } + }; + + try { + await waitForPtyOutput(master, /this is a very long wrapped line/); + + // Some hosts drop the master between commands without killing the child. + closeMaster(); + await expect(waitForChildExit(child, 3_000)).resolves.toEqual({ + code: 0, + signal: null, + }); + } finally { + closeMaster(); + await stopChild(child); + } + }, + ); + + test.skipIf(process.platform !== "darwin")( + "exits when macOS revokes the controlling terminal", + async () => { + const fixture = harness.createLongWrapFilePair(); + const pidFile = join(fixture.dir, "hunk.pid"); + const exitFile = join(fixture.dir, "hunk.exit"); + const ttyFile = join(fixture.dir, "hunk.tty"); + const hunkCommand = harness.buildHunkCommand(["diff", fixture.before, fixture.after]); + const session = await harness.launchShellCommand({ + command: `trap '' HUP; tty_path="$(tty)"; printf '%s' "$tty_path" > ${harness.shellQuote(ttyFile)}; ${hunkCommand} < "$tty_path" & hunk_pid=$!; printf '%s' "$hunk_pid" > ${harness.shellQuote(pidFile)}; wait "$hunk_pid"; printf '%s' "$?" > ${harness.shellQuote(exitFile)}`, + cwd: fixture.dir, + }); + + try { + await session.waitForText(/this is a very long wrapped line/, { timeout: 15_000 }); + const pid = Number.parseInt(readFileSync(pidFile, "utf8"), 10); + const ttyPath = readFileSync(ttyFile, "utf8").trim(); + expect(pid).toBeGreaterThan(0); + expect(ttyPath).toStartWith("/dev/tty"); + + expect(revokeTerminal(ttyPath)).toBe(0); + expect(await waitForExitCode(exitFile)).toBe(0); + } finally { + session.close(); + } + }, + ); +});