Merge pull request #406 from corbitsdev/cl-5552-own-process-termination

Own process termination on detached throws and OS signals

Author: TheGreatAxios

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<number> {
   });
 }
 
+// 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<void> {
+  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<void> {
+  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 {

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);
+  });
+});

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;
+}

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

src/tui/runner.ts

@@ -176,6 +176,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";
@@ -538,6 +539,7 @@ export async function runTUI(initialConfig: Config): Promise<number> {
     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.
@@ -2263,6 +2265,7 @@ export async function runTUI(initialConfig: Config): Promise<number> {
   });
 
   disposeHost = host.dispose;
+  setActiveDisposeHost(disposeHost);
 
   setMentionSuggestionSource(host.shell, (prefix) => listPathSuggestions(prefix, config.cwd));
 
@@ -2381,6 +2384,7 @@ export async function runTUI(initialConfig: Config): Promise<number> {
   finalized = true;
   activeRunHandle.active = false;
   clearActiveRun();
+  clearActiveDisposeHost();
   await writeRunSnapshot(persistedStatus, {
     finishedAt,
     ...(sinkError !== undefined ? { error: sinkError } : {}),

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);

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);

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())

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);
+});