fix(coding-agent): run extension cleanup and restore terminal on signal exits
Signal-triggered shutdown (SIGTERM/SIGHUP) now emits session_shutdown before any terminal writes, and SIGHUP no longer hard-exits, so extension resources (e.g. sockets) are released and the terminal is restored even when the terminal is gone. A genuinely dead terminal still exits without hot-spinning via the EIO error path. closes #5080
This commit is contained in:
@@ -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)).
|
||||
|
||||
@@ -3248,11 +3248,28 @@ export class InteractiveMode {
|
||||
*/
|
||||
private isShuttingDown = false;
|
||||
|
||||
private async shutdown(): Promise<void> {
|
||||
private async shutdown(options?: { fromSignal?: boolean }): Promise<void> {
|
||||
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));
|
||||
|
||||
@@ -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<void> };
|
||||
ui: { terminal: { drainInput: (ms: number) => Promise<void> } };
|
||||
stop: () => void;
|
||||
};
|
||||
|
||||
type InteractiveModePrototypeWithShutdown = {
|
||||
shutdown(this: ShutdownThis, options?: { fromSignal?: boolean }): Promise<void>;
|
||||
};
|
||||
|
||||
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<void> {
|
||||
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();
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user