diff --git a/src/index.ts b/src/index.ts index 0ce70559e..35106d3d6 100644 --- a/src/index.ts +++ b/src/index.ts @@ -2,6 +2,7 @@ import { getLogger } from "@intx/log"; import { LOG_NAMESPACE_ROOT } from "./branding.js"; import { primeCrashReporting, writeCrashReport, type CrashKind } from "./crash/report.js"; import { getActiveRun, markCrashed } from "./session/active-run.js"; +import { getActiveDisposeHost } from "./session/active-host.js"; import { saveCrashState } from "./session/state.js"; import { loadConfig } from "./config/index.js"; import { ensureTelemetrySettings, globalSettingsPath } from "./config/settings.js"; @@ -115,9 +116,30 @@ export async function main(argv: readonly string[]): Promise { }); } +// Shared by handleFatal and the signal handlers below so a signal arriving +// mid-crash-unwind (or a crash surfacing while a signal is already tearing +// the process down) can't re-enter either path a second time. +let terminating = false; + // Exported so an integration test can register these process-level handlers // and inject a crash without spawning the full TUI stack. export async function handleFatal(kind: CrashKind, error: unknown): Promise { + if (terminating) return; + terminating = true; + // OpenTUI's own uncaughtException/unhandledRejection listener only logs + // (see installCrashHandlers' comment below) — it never tears down the + // terminal the way its signal listener does. Without this, a throw that + // escapes runTUI's own try/catch (e.g. inside a fire-and-forget `void` + // call) leaves the alternate screen and raw mode stuck. disposeHost is + // idempotent, so this is safe even if runTUI's own catch block already + // ran it moments earlier. + try { + getActiveDisposeHost()?.(); + } catch (disposeErr: unknown) { + process.stderr.write( + `host dispose failed during fatal handling: ${disposeErr instanceof Error ? disposeErr.message : String(disposeErr)}\n`, + ); + } // Flip this before any awaits below so any snapshot write still queued // behind another one in state.ts's per-session chain sees it and steps // aside the moment it's next in line, rather than racing saveCrashState's @@ -184,8 +206,80 @@ export function installCrashHandlers(): void { }); } +// Mirrors finalizeActiveRunOnCrash but is not itself a crash — a signal is a +// clean, externally-requested termination (operator, shell, orchestrator), +// so the run is left "failed" (interrupted) rather than "crashed", and no +// crash report is written for it. +async function finalizeActiveRunOnSignal(signal: NodeJS.Signals): Promise { + const run = getActiveRun(); + if (run === null || !run.active) return; + try { + await saveCrashState(run.cwd, run.sessionId, { + status: "failed", + turnsUsed: 0, + task: run.task, + startedAt: run.startedAt, + finishedAt: Date.now(), + error: `terminated by ${signal}`, + ...(run.model !== undefined ? { model: run.model } : {}), + }); + } catch (saveErr: unknown) { + process.stderr.write( + `failed to finalize run state after ${signal}: ${saveErr instanceof Error ? saveErr.message : String(saveErr)}\n`, + ); + } +} + +const SIGNAL_EXIT_NUMBER: Record<"SIGINT" | "SIGTERM" | "SIGHUP", number> = { + SIGHUP: 1, + SIGINT: 2, + SIGTERM: 15, +}; + +// Bun's tty raw mode (which the TUI runs under for its whole session) clears +// ISIG, so a real terminal's Ctrl+C never reaches this handler while a +// session is interactive — confirmed empirically (see the raw-mode SIGINT +// regression test) rather than assumed. The in-session double-tap-to-quit +// gesture (shell.ts, CTRL_C_EXIT_WINDOW_MS) is therefore untouched by this +// handler; it owns Ctrl+C exclusively for the interactive case. This handler +// exists for the signal actually reaching the process: external +// orchestration (kill, systemd, docker stop), or a terminal that never +// entered raw mode at all (exec mode has no TUI host and no raw stdin, so +// its Ctrl+C is a real SIGINT today with no listener at all — Bun's default +// disposition kills it immediately without a chance to close out run.json). +// +// Terminal restore is done directly here, the same way handleFatal does it, +// rather than left to OpenTUI's own same-signal listener (registered later, +// at host-mount time, once a TUI is actually running): relying on a +// vendored listener's registration order and internal behavior to already +// cover teardown would make correctness depend on undocumented @opentui +// internals that could change on any version bump, with terminal-left-wedged +// as the silent failure mode. disposeHost is idempotent, so calling it here +// even when OpenTUI's own listener also runs is harmless. +// Exported so an integration test can register these process-level handlers +// and send a real signal without spawning the full TUI stack. +export function installSignalHandlers(): void { + for (const signal of ["SIGINT", "SIGTERM", "SIGHUP"] as const) { + process.on(signal, () => { + if (terminating) return; + terminating = true; + try { + getActiveDisposeHost()?.(); + } catch (disposeErr: unknown) { + process.stderr.write( + `host dispose failed handling ${signal}: ${disposeErr instanceof Error ? disposeErr.message : String(disposeErr)}\n`, + ); + } + void finalizeActiveRunOnSignal(signal).finally(() => { + process.exit(128 + SIGNAL_EXIT_NUMBER[signal]); + }); + }); + } +} + if (import.meta.main) { installCrashHandlers(); + installSignalHandlers(); let code: number; try { diff --git a/src/session/active-host.test.ts b/src/session/active-host.test.ts new file mode 100644 index 000000000..39abe7268 --- /dev/null +++ b/src/session/active-host.test.ts @@ -0,0 +1,32 @@ +import { afterEach, describe, expect, test } from "bun:test"; + +import { clearActiveDisposeHost, getActiveDisposeHost, setActiveDisposeHost } from "./active-host.js"; + +describe("active-host", () => { + afterEach(() => { + clearActiveDisposeHost(); + }); + + test("starts with no active dispose handle", () => { + expect(getActiveDisposeHost()).toBeNull(); + }); + + test("returns the handle set by setActiveDisposeHost", () => { + const disposeHost = () => {}; + setActiveDisposeHost(disposeHost); + expect(getActiveDisposeHost()).toBe(disposeHost); + }); + + test("clearActiveDisposeHost removes the handle", () => { + setActiveDisposeHost(() => {}); + clearActiveDisposeHost(); + expect(getActiveDisposeHost()).toBeNull(); + }); + + test("setActiveDisposeHost overwrites a previously set handle", () => { + setActiveDisposeHost(() => {}); + const second = () => {}; + setActiveDisposeHost(second); + expect(getActiveDisposeHost()).toBe(second); + }); +}); diff --git a/src/session/active-host.ts b/src/session/active-host.ts new file mode 100644 index 000000000..046d7f63d --- /dev/null +++ b/src/session/active-host.ts @@ -0,0 +1,20 @@ +// A module-level slot mirroring active-run.ts's pattern: the top-level +// process handlers in src/index.ts (a detached-throw handler today, a signal +// handler alongside it) need to reach runTUI's terminal-restore routine even +// though it is a closure local to runTUI, bound only once the OpenTUI host +// has mounted. Cleared the moment runTUI itself finalizes (normally or via +// its own crash path) so a signal arriving after teardown has nothing left +// to call. +let activeDisposeHost: (() => void) | null = null; + +export function setActiveDisposeHost(disposeHost: () => void): void { + activeDisposeHost = disposeHost; +} + +export function clearActiveDisposeHost(): void { + activeDisposeHost = null; +} + +export function getActiveDisposeHost(): (() => void) | null { + return activeDisposeHost; +} diff --git a/src/tui-opentui/product-host.ts b/src/tui-opentui/product-host.ts index 376ff255b..fb07d70aa 100644 --- a/src/tui-opentui/product-host.ts +++ b/src/tui-opentui/product-host.ts @@ -296,6 +296,10 @@ export async function mountProductHost( const renderer = config.createRenderer ? await config.createRenderer() : await createCliRenderer({ + // Leaves Ctrl+C entirely to shell.ts's own double-tap-to-quit + // gesture (CTRL_C_EXIT_WINDOW_MS). index.ts's SIGINT handler also + // depends on this staying false: Ctrl+C only reaches it as a real + // OS signal when nothing already consumed it as a keypress. exitOnCtrlC: false, targetFps: 30, // Mouse reporting on by default: without it, wheel/trackpad scroll diff --git a/src/tui/runner.ts b/src/tui/runner.ts index 49a5d0ed8..50711d637 100644 --- a/src/tui/runner.ts +++ b/src/tui/runner.ts @@ -167,6 +167,7 @@ import { generateSessionId, initSessionDir, renameSession, sessionContextDir, se import { resolveSessionLabel, truncateSessionLabel } from "../session/session-label.js"; import { loadState, saveState, type ConnectedMcpServer, type RunState } from "../session/state.js"; import { setActiveRun, clearActiveRun, type RunStateHandle } from "../session/active-run.js"; +import { setActiveDisposeHost, clearActiveDisposeHost } from "../session/active-host.js"; import { openInBrowser } from "../auth/oauth/browser.js"; import { pickSession } from "./pick-session.js"; import { RESUME_TRANSCRIPT_BLOCK_LIMIT, turnsToContentBlocks } from "./turns-to-blocks.js"; @@ -529,6 +530,7 @@ export async function runTUI(initialConfig: Config): Promise { finalized = true; activeRunHandle.active = false; clearActiveRun(); + clearActiveDisposeHost(); await flushPartialOnCrash().catch((flushErr: unknown) => { // Best-effort only — still attempt saveState below. Log so a flush // failure is not invisible when diagnosing a crash exit. @@ -2192,6 +2194,7 @@ export async function runTUI(initialConfig: Config): Promise { }); disposeHost = host.dispose; + setActiveDisposeHost(disposeHost); setMentionSuggestionSource(host.shell, (prefix) => listPathSuggestions(prefix, config.cwd)); @@ -2296,6 +2299,7 @@ export async function runTUI(initialConfig: Config): Promise { finalized = true; activeRunHandle.active = false; clearActiveRun(); + clearActiveDisposeHost(); await writeRunSnapshot(persistedStatus, { finishedAt, ...(sinkError !== undefined ? { error: sinkError } : {}), diff --git a/tests/fixtures/crash-run/simulate-signal.ts b/tests/fixtures/crash-run/simulate-signal.ts new file mode 100644 index 000000000..ce660292e --- /dev/null +++ b/tests/fixtures/crash-run/simulate-signal.ts @@ -0,0 +1,36 @@ +// Spawned as a subprocess by tests/integration/signal-finalize.test.ts. +// Mimics what runTUI does at startup (register the active run, write the +// initial "running" run.json) and what index.ts does at process entry +// (install the signal handlers), then waits to receive a real signal sent by +// the test from outside the process. +import { installSignalHandlers } from "../../../src/index.js"; +import { setActiveRun } from "../../../src/session/active-run.js"; +import { sessionDir } from "../../../src/session/index.js"; +import { saveState } from "../../../src/session/state.js"; + +const cwd = process.cwd(); +const sessionId = process.env["SIGNAL_TEST_SESSION_ID"]; +if (sessionId === undefined) { + throw new Error("SIGNAL_TEST_SESSION_ID must be set"); +} + +const startedAt = Date.now(); +const task = "simulated signal task"; +const model = "test-provider:test-model"; + +await saveState(cwd, sessionId, { + status: "running", + turnsUsed: 3, + task, + startedAt, + model, +}); + +setActiveRun({ sessionId, cwd, active: true, task, startedAt, model }); +installSignalHandlers(); + +process.stdout.write(`${sessionDir(cwd, sessionId)}\n`); +process.stdout.write("ready\n"); + +// Keep the event loop alive until the test sends a signal. +setInterval(() => {}, 60_000); diff --git a/tests/fixtures/rawmode-sigint/probe.ts b/tests/fixtures/rawmode-sigint/probe.ts new file mode 100644 index 000000000..609cf4d99 --- /dev/null +++ b/tests/fixtures/rawmode-sigint/probe.ts @@ -0,0 +1,18 @@ +// Fixture for tests/integration/rawmode-sigint.test.ts. Proves, on the real +// Bun runtime rather than by assumption, whether a Ctrl+C keypress (0x03) +// generates a SIGINT deliverable to process.on("SIGINT") while stdin is in +// raw mode — the empirical claim src/index.ts's installSignalHandlers +// depends on to leave the in-session double-tap-to-quit gesture untouched. +process.stdin.setRawMode(true); +process.on("SIGINT", () => { + process.stdout.write("GOT_SIGINT\n"); + process.exit(0); +}); +process.stdin.resume(); +process.stdin.on("data", (chunk: Buffer) => { + if (chunk.includes(0x03)) process.stdout.write("GOT_CTRL_C_BYTE\n"); +}); +setTimeout(() => { + process.stdout.write("NO_SIGINT_ON_CTRL_C\n"); + process.exit(0); +}, 3000); diff --git a/tests/fixtures/rawmode-sigint/pty_probe.py b/tests/fixtures/rawmode-sigint/pty_probe.py new file mode 100644 index 000000000..3c3912003 --- /dev/null +++ b/tests/fixtures/rawmode-sigint/pty_probe.py @@ -0,0 +1,48 @@ +#!/usr/bin/env python3 +# Drives tests/fixtures/rawmode-sigint/probe.ts inside a real forked pty +# (stdlib `pty`/`os`/`select`, no third-party deps) so the fixture's stdin +# is a genuine tty rather than a pipe -- `setRawMode` only has the raw-mode +# vs. cooked-mode distinction this test cares about on a real tty. +import os +import pty +import select +import sys +import time + +def main() -> int: + probe_path = sys.argv[1] + pid, fd = pty.fork() + if pid == 0: + os.execvp("bun", ["bun", "run", probe_path]) + os._exit(127) + + time.sleep(1) + os.write(fd, b"\x03") + + out = b"" + deadline = time.time() + 4 + while time.time() < deadline: + ready, _, _ = select.select([fd], [], [], 0.5) + if fd not in ready: + continue + try: + chunk = os.read(fd, 4096) + except OSError: + break + if not chunk: + break + out += chunk + if b"NO_SIGINT_ON_CTRL_C" in out or b"GOT_SIGINT" in out: + break + + try: + os.kill(pid, 9) + except OSError: + pass + os.waitpid(pid, 0) + + sys.stdout.write(out.decode(errors="replace")) + return 0 + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/tests/integration/rawmode-sigint.test.ts b/tests/integration/rawmode-sigint.test.ts new file mode 100644 index 000000000..59085f1b2 --- /dev/null +++ b/tests/integration/rawmode-sigint.test.ts @@ -0,0 +1,30 @@ +import { describe, expect, test } from "bun:test"; + +// src/index.ts's installSignalHandlers relies on an empirical claim: Bun's +// stdin.setRawMode(true) clears ISIG on this platform, so a real Ctrl+C +// keypress never reaches process.on("SIGINT") during an interactive TUI +// session -- only out-of-band kill(2) signals do. If a future Bun upgrade +// changes that, the in-session double-tap-to-quit gesture (shell.ts, +// CTRL_C_EXIT_WINDOW_MS) would silently start racing a process-level exit +// on the very first Ctrl+C. This test pins the assumption against a real +// forked pty rather than trusting it to hold forever. +describe("integration — raw-mode stdin and SIGINT", () => { + test("Ctrl+C is delivered as a stdin byte, not as SIGINT, while raw mode is active", async () => { + const probe = new URL("../fixtures/rawmode-sigint/probe.ts", import.meta.url).pathname; + const driver = new URL("../fixtures/rawmode-sigint/pty_probe.py", import.meta.url).pathname; + + const proc = Bun.spawn(["python3", driver, probe], { + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, exitCode] = await Promise.all([ + new Response(proc.stdout).text(), + proc.exited, + ]); + + expect(exitCode).toBe(0); + expect(stdout).toContain("GOT_CTRL_C_BYTE"); + expect(stdout).toContain("NO_SIGINT_ON_CTRL_C"); + expect(stdout).not.toContain("GOT_SIGINT"); + }, 15000); +}); diff --git a/tests/integration/signal-finalize.test.ts b/tests/integration/signal-finalize.test.ts new file mode 100644 index 000000000..270707c19 --- /dev/null +++ b/tests/integration/signal-finalize.test.ts @@ -0,0 +1,67 @@ +import { mkdtempSync, readFileSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { describe, expect, test } from "bun:test"; + +import { generateSessionId } from "../../src/session/index.js"; +import type { RunState } from "../../src/session/state.js"; + +const FIXTURE = join(import.meta.dirname, "../fixtures/crash-run/simulate-signal.ts"); + +async function readLine(stream: ReadableStream): Promise { + const reader = stream.getReader(); + const decoder = new TextDecoder(); + let buffer = ""; + while (!buffer.includes("\n")) { + const { value, done } = await reader.read(); + if (done) break; + buffer += decoder.decode(value, { stream: true }); + } + reader.releaseLock(); + return buffer; +} + +describe("integration — signal finalizes run.json", () => { + test.each([ + ["SIGINT", 130], + ["SIGTERM", 143], + ["SIGHUP", 129], + ] as const)("%s writes status: failed and exits with %i", async (signal, expectedExitCode) => { + const cwd = mkdtempSync(join(tmpdir(), "corbits-signal-cwd-")); + const home = mkdtempSync(join(tmpdir(), "corbits-signal-home-")); + const sessionId = generateSessionId(); + + try { + const proc = Bun.spawn(["bun", "run", FIXTURE], { + cwd, + env: { ...process.env, HOME: home, SIGNAL_TEST_SESSION_ID: sessionId }, + stdout: "pipe", + stderr: "pipe", + }); + + const output = await readLine(proc.stdout); + const [runDir] = output.split("\n"); + if (runDir === undefined || runDir.length === 0) { + throw new Error(`fixture did not report a run directory: ${JSON.stringify(output)}`); + } + + proc.kill(signal); + const exitCode = await proc.exited; + + expect(exitCode).toBe(expectedExitCode); + + const runJsonPath = join(runDir, "run.json"); + const raw = readFileSync(runJsonPath, "utf8"); + const state = JSON.parse(raw) as RunState; + + expect(state.status).toBe("failed"); + expect(state.finishedAt).toBeGreaterThan(0); + expect(state.error).toBe(`terminated by ${signal}`); + expect(state.task).toBe("simulated signal task"); + } finally { + rmSync(cwd, { recursive: true, force: true }); + rmSync(home, { recursive: true, force: true }); + } + }, 15_000); +});