diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 6afe9ad9..e697d433 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -8,6 +8,7 @@ ### Fixed +- Fixed SIGTERM/SIGHUP interactive shutdown to keep signal handlers installed until terminal cleanup completes, preventing `signal-exit` from re-sending the signal and leaving the terminal in raw/Kitty keyboard mode ([#5724](https://github.com/earendil-works/pi/issues/5724)). - Fixed extensions documentation to clarify that `pi.getActiveTools()` returns active tool names while `pi.getAllTools()` returns tool metadata ([#5729](https://github.com/earendil-works/pi/issues/5729)). - Fixed package commands such as `pi list`, `pi install`, and `pi update` to terminate after completing even if an extension leaves background handles open ([#5687](https://github.com/earendil-works/pi/issues/5687)). - Fixed `pi update` for pnpm global installs whose configured `global-bin-dir` no longer matches the active pnpm home ([#5689](https://github.com/earendil-works/pi/issues/5689)). diff --git a/packages/coding-agent/src/modes/interactive/interactive-mode.ts b/packages/coding-agent/src/modes/interactive/interactive-mode.ts index b3424a15..56ef9098 100644 --- a/packages/coding-agent/src/modes/interactive/interactive-mode.ts +++ b/packages/coding-agent/src/modes/interactive/interactive-mode.ts @@ -3362,7 +3362,9 @@ export class InteractiveMode { private async shutdown(options?: { fromSignal?: boolean }): Promise { if (this.isShuttingDown) return; this.isShuttingDown = true; - this.unregisterSignalHandlers(); + // Keep signal handlers registered until terminal cleanup has completed. + // `signal-exit` checks the listener list during the same SIGTERM/SIGHUP + // dispatch and re-sends the signal if only its own listeners remain. if (options?.fromSignal) { // Signal-triggered shutdown (SIGTERM/SIGHUP). Emit extension cleanup @@ -5742,7 +5744,6 @@ export class InteractiveMode { } stop(): void { - this.unregisterSignalHandlers(); if (this.settingsManager.getShowTerminalProgress()) { this.ui.terminal.setProgress(false); } @@ -5760,5 +5761,6 @@ export class InteractiveMode { this.ui.stop(); this.isInitialized = false; } + this.unregisterSignalHandlers(); } } 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 index 30810fe8..33a960e2 100644 --- 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 @@ -116,7 +116,6 @@ describe("InteractiveMode.shutdown ordering (#5080)", () => { 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 () => { diff --git a/packages/coding-agent/test/suite/regressions/5724-sigterm-signal-exit.test.ts b/packages/coding-agent/test/suite/regressions/5724-sigterm-signal-exit.test.ts new file mode 100644 index 00000000..d170f1cf --- /dev/null +++ b/packages/coding-agent/test/suite/regressions/5724-sigterm-signal-exit.test.ts @@ -0,0 +1,92 @@ +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/5724 +// +// `proper-lockfile` installs `signal-exit`, whose signal listener re-sends +// SIGTERM/SIGHUP when it observes no other process listeners during the same +// signal dispatch. InteractiveMode must therefore keep its signal handlers +// registered until async terminal cleanup has completed. + +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 deferred(): { promise: Promise; resolve: () => void } { + let resolve: (() => void) | undefined; + const promise = new Promise((res) => { + resolve = res; + }); + return { + promise, + resolve: () => resolve?.(), + }; +} + +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 SIGTERM shutdown with signal-exit (#5724)", () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + test("keeps signal handlers registered while signal-triggered cleanup is pending", async () => { + vi.spyOn(process, "exit").mockImplementation((() => { + throw new ProcessExitError(); + }) as typeof process.exit); + + const order: string[] = []; + const dispose = deferred(); + const context: ShutdownThis = { + isShuttingDown: false, + unregisterSignalHandlers: vi.fn(() => { + order.push("unregister"); + }), + runtimeHost: { + dispose: vi.fn(() => { + order.push("dispose"); + return dispose.promise; + }), + }, + ui: { + terminal: { + drainInput: vi.fn(async () => { + order.push("drainInput"); + }), + }, + }, + stop: vi.fn(() => { + order.push("stop"); + }), + }; + + const shutdownPromise = callShutdown(context, { fromSignal: true }); + await Promise.resolve(); + + expect(order).toEqual(["dispose"]); + expect(context.unregisterSignalHandlers).not.toHaveBeenCalled(); + + dispose.resolve(); + await shutdownPromise; + + expect(order).toEqual(["dispose", "drainInput", "stop"]); + }); +});