diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 875dd7f4..392fd0bd 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -4,6 +4,7 @@ ### Fixed +- Fixed SIGTERM/SIGHUP exits to run extension `session_shutdown` cleanup and restore the terminal: signal-triggered shutdown now emits `session_shutdown` before any terminal writes, and SIGHUP no longer hard-exits, so extension resources (e.g. sockets) are released even when the terminal is gone ([#5080](https://github.com/earendil-works/pi/issues/5080)). - Fixed Windows startup crashes under MSYS2 ucrt64 Node.js by updating the native clipboard addon to napi-rs 3.x ([#5028](https://github.com/earendil-works/pi/issues/5028)). - Fixed API key and header config resolution to treat plain strings as literals, support `$ENV_VAR` / `${ENV_VAR}` interpolation and `$!` bang escaping, and require explicit env syntax for config files, avoiding Windows case-insensitive env matches corrupting literal keys ([#5095](https://github.com/earendil-works/pi/issues/5095)). - Fixed session disposal to abort in-flight agent, compaction, branch summary, retry, and bash work ([#5029](https://github.com/earendil-works/pi/pull/5029) by [@TerminallyChilI](https://github.com/TerminallyChilI)). diff --git a/packages/coding-agent/src/modes/interactive/interactive-mode.ts b/packages/coding-agent/src/modes/interactive/interactive-mode.ts index 3068c89c..b470070f 100644 --- a/packages/coding-agent/src/modes/interactive/interactive-mode.ts +++ b/packages/coding-agent/src/modes/interactive/interactive-mode.ts @@ -3248,11 +3248,28 @@ export class InteractiveMode { */ private isShuttingDown = false; - private async shutdown(): Promise { + private async shutdown(options?: { fromSignal?: boolean }): Promise { if (this.isShuttingDown) return; this.isShuttingDown = true; this.unregisterSignalHandlers(); + if (options?.fromSignal) { + // Signal-triggered shutdown (SIGTERM/SIGHUP). Emit extension cleanup + // (session_shutdown) BEFORE touching the terminal. Extension teardown + // such as removing sockets does not write to the tty, so it must not be + // skipped if a later terminal-restore write fails on a dead or stalled + // terminal. If the terminal is gone, the restore writes below emit EIO, + // which the stdout/stderr error handler turns into emergencyTerminalExit; + // the render loop is already idle, so this cannot hot-spin (see #4144). + await this.runtimeHost.dispose(); + await this.ui.terminal.drainInput(1000); + this.stop(); + process.exit(0); + } + + // Interactive quit (Ctrl+D, Ctrl+C, /quit, extension shutdown()). Stop the + // TUI before emitting shutdown events so extension UI cleanup cannot repaint + // the final frame while the process is exiting. // Drain any in-flight Kitty key release events before stopping. // This prevents escape sequences from leaking to the parent shell over slow SSH. await this.ui.terminal.drainInput(1000); @@ -3319,11 +3336,12 @@ export class InteractiveMode { for (const signal of signals) { const handler = () => { - if (signal === "SIGHUP") { - this.emergencyTerminalExit(); - } + // SIGHUP no longer hard-exits: graceful shutdown emits session_shutdown + // first, then attempts terminal restore. A genuinely dead terminal + // surfaces as an EIO on the restore writes, which the stdout/stderr + // error handler converts into emergencyTerminalExit (see #4144, #5080). killTrackedDetachedChildren(); - void this.shutdown(); + void this.shutdown({ fromSignal: true }); }; process.prependListener(signal, handler); this.signalCleanupHandlers.push(() => process.off(signal, handler)); diff --git a/packages/coding-agent/test/suite/regressions/5080-signal-shutdown-extension-cleanup.test.ts b/packages/coding-agent/test/suite/regressions/5080-signal-shutdown-extension-cleanup.test.ts new file mode 100644 index 00000000..d8bcd867 --- /dev/null +++ b/packages/coding-agent/test/suite/regressions/5080-signal-shutdown-extension-cleanup.test.ts @@ -0,0 +1,103 @@ +import { afterEach, describe, expect, test, vi } from "vitest"; +import { InteractiveMode } from "../../../src/modes/interactive/interactive-mode.ts"; + +// Regression for https://github.com/earendil-works/pi/issues/5080 +// +// On SIGTERM/SIGHUP the graceful shutdown must emit `session_shutdown` +// (runtimeHost.dispose) BEFORE touching the terminal. Extension teardown such +// as removing a socket does not write to the tty, so it must not be skipped if +// a later terminal-restore write fails on a dead or stalled terminal. The +// interactive quit path (Ctrl+D, /quit) keeps the opposite order to preserve +// the final TUI frame. + +type ShutdownThis = { + isShuttingDown: boolean; + unregisterSignalHandlers: () => void; + runtimeHost: { dispose: () => Promise }; + ui: { terminal: { drainInput: (ms: number) => Promise } }; + stop: () => void; +}; + +type InteractiveModePrototypeWithShutdown = { + shutdown(this: ShutdownThis, options?: { fromSignal?: boolean }): Promise; +}; + +const interactiveModePrototype = InteractiveMode.prototype as unknown; + +class ProcessExitError extends Error {} + +function createContext(order: string[]): ShutdownThis { + return { + isShuttingDown: false, + unregisterSignalHandlers: vi.fn(), + runtimeHost: { + dispose: vi.fn(async () => { + order.push("dispose"); + }), + }, + ui: { + terminal: { + drainInput: vi.fn(async () => { + order.push("drainInput"); + }), + }, + }, + stop: vi.fn(() => { + order.push("stop"); + }), + }; +} + +async function callShutdown(context: ShutdownThis, options?: { fromSignal?: boolean }): Promise { + try { + await (interactiveModePrototype as InteractiveModePrototypeWithShutdown).shutdown.call(context, options); + } catch (error) { + if (!(error instanceof ProcessExitError)) throw error; + } +} + +describe("InteractiveMode.shutdown ordering (#5080)", () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + test("signal-triggered shutdown emits session_shutdown before terminal writes", async () => { + vi.spyOn(process, "exit").mockImplementation((() => { + throw new ProcessExitError(); + }) as typeof process.exit); + const order: string[] = []; + const context = createContext(order); + + await callShutdown(context, { fromSignal: true }); + + expect(order).toEqual(["dispose", "drainInput", "stop"]); + expect(context.isShuttingDown).toBe(true); + expect(context.unregisterSignalHandlers).toHaveBeenCalledTimes(1); + }); + + test("interactive quit stops the TUI before emitting session_shutdown", async () => { + vi.spyOn(process, "exit").mockImplementation((() => { + throw new ProcessExitError(); + }) as typeof process.exit); + const order: string[] = []; + const context = createContext(order); + + await callShutdown(context); + + expect(order).toEqual(["drainInput", "stop", "dispose"]); + }); + + test("re-entrant shutdown is a no-op", async () => { + vi.spyOn(process, "exit").mockImplementation((() => { + throw new ProcessExitError(); + }) as typeof process.exit); + const order: string[] = []; + const context = createContext(order); + context.isShuttingDown = true; + + await callShutdown(context, { fromSignal: true }); + + expect(order).toEqual([]); + expect(context.runtimeHost.dispose).not.toHaveBeenCalled(); + }); +});