mirror of
https://github.com/badlogic/pi-mono.git
synced 2026-11-05 23:37:25 +00:00
fix(coding-agent): let a started RPC or print shutdown finish; only a repeated signal forces exit (#11752)
Follow-up to #11746 for the case @fcatuhe reported on #11599: a second shutdown trigger cut off cleanup. Problem: Paseo stops pi by closing stdin and sending SIGTERM in the same tick (SIGKILL 2 s later); systemd sends SIGTERM and stdin closes after. In RPC mode: - stdin EOF, then SIGTERM: `shutdown()` removed pi's SIGTERM handler when it started, so the signal killed pi (exit 143) mid-`dispose()`. - SIGTERM, then stdin EOF: the second `shutdown()` call ran `process.exit()`. A pending MCP token refresh was lost: the server had rotated the refresh token, the next pi sent the old one, and the server revoked the grant. Rule after this change: once a shutdown has started, a stdin EOF or a single signal does not interrupt it; a second signal exits at once. SIGKILL still always works. - **RPC mode** (`rpc-mode.ts`): signal handlers stay registered until exit; `shutdown()` runs once and later calls do nothing; the signal handler counts signals and exits only on a repeated one. - **Print mode** (`print-mode.ts`): the final `finally` removed the signal handlers before `disposeRuntime()`, so a signal during the final disposal killed pi. Handlers are now removed after it, `disposeRuntime()` returns the shared in-flight disposal (a signal then waits for it), and a repeated signal exits at once, as before. - **Interactive mode**: unchanged. It already lets a started shutdown finish. Docs (`docs/rpc.md`) and the CHANGELOG are updated. ## Testing - `test/rpc-prompt-response-semantics.test.ts`: EOF then SIGTERM, SIGTERM then EOF (both wait for a slow dispose, exit once with 0 and 143), and EOF then two SIGTERMs (exits at once with 143). All three fail on main. - `test/print-mode.test.ts`: SIGTERM during the final disposal waits for it, a second SIGTERM exits with 143, handlers are removed afterwards. Fails on main. - Real process, `pi --mode rpc` with a `session_shutdown` handler that takes 1 s and real signals: - EOF then SIGTERM: cleanup done, exit 0 (main: no cleanup, exit 143) - SIGTERM then EOF: cleanup done, exit 143 - EOF then two SIGTERMs: exit 143 at once - `npm run check` passes; all `test/rpc*` and `test/print-mode` tests pass.
This commit is contained in:
parent
efb45b51d4
commit
ace560c302
6 changed files with 167 additions and 26 deletions
|
|
@ -152,4 +152,37 @@ describe("runPrintMode", () => {
|
|||
expect(exitCode).toBe(1);
|
||||
expect(errorSpy).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
// A signal during the final disposal used to hit no handler and kill pi mid-dispose.
|
||||
it("waits for the final disposal when SIGTERM arrives during it, and exits on a repeated SIGTERM", async () => {
|
||||
const exitSpy = vi.spyOn(process, "exit").mockImplementation((() => undefined) as never);
|
||||
const runtimeHost = createRuntimeHost(createAssistantMessage({ text: "done" }));
|
||||
let finishDispose = () => {};
|
||||
runtimeHost.dispose.mockImplementation(
|
||||
() =>
|
||||
new Promise<void>((resolve) => {
|
||||
finishDispose = resolve;
|
||||
}),
|
||||
);
|
||||
const listenersBefore = process.listenerCount("SIGTERM");
|
||||
|
||||
const run = runPrintMode(runtimeHost as unknown as Parameters<typeof runPrintMode>[0], {
|
||||
mode: "text",
|
||||
initialMessage: "Say done",
|
||||
});
|
||||
await vi.waitFor(() => expect(runtimeHost.dispose).toHaveBeenCalledTimes(1));
|
||||
expect(process.listenerCount("SIGTERM")).toBe(listenersBefore + 1);
|
||||
|
||||
process.emit("SIGTERM");
|
||||
await new Promise((resolve) => setTimeout(resolve, 10));
|
||||
expect(exitSpy).not.toHaveBeenCalled();
|
||||
expect(runtimeHost.dispose).toHaveBeenCalledTimes(1);
|
||||
|
||||
process.emit("SIGTERM");
|
||||
expect(exitSpy).toHaveBeenCalledWith(143);
|
||||
|
||||
finishDispose();
|
||||
await run;
|
||||
expect(process.listenerCount("SIGTERM")).toBe(listenersBefore);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -8,7 +8,7 @@ import {
|
|||
getModel,
|
||||
type Model,
|
||||
} from "@earendil-works/pi-ai/compat";
|
||||
import { afterEach, describe, expect, it, vi } from "vitest";
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import { AgentSession } from "../src/core/agent-session.ts";
|
||||
import type { AgentSessionRuntime } from "../src/core/agent-session-runtime.ts";
|
||||
import { AuthStorage } from "../src/core/auth-storage.ts";
|
||||
|
|
@ -162,6 +162,7 @@ async function createRuntimeHost(options: {
|
|||
|
||||
async function startRpcMode(options: Parameters<typeof createRuntimeHost>[0]): Promise<{
|
||||
lineHandler: (line: string) => void;
|
||||
runtimeHost: AgentSessionRuntime;
|
||||
cleanup: () => Promise<void>;
|
||||
}> {
|
||||
rpcIo.outputLines = [];
|
||||
|
|
@ -171,7 +172,7 @@ async function startRpcMode(options: Parameters<typeof createRuntimeHost>[0]): P
|
|||
void runRpcMode(runtimeHost);
|
||||
await vi.waitFor(() => expect(rpcIo.lineHandler).toBeDefined());
|
||||
|
||||
return { lineHandler: rpcIo.lineHandler!, cleanup };
|
||||
return { lineHandler: rpcIo.lineHandler!, runtimeHost, cleanup };
|
||||
}
|
||||
|
||||
describe("RPC prompt response semantics", () => {
|
||||
|
|
@ -550,3 +551,94 @@ describe("RPC extension shutdown requests", () => {
|
|||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe("RPC shutdown triggers", () => {
|
||||
// Earlier tests leave RPC instances listening (process.exit never runs). Detach every signal and
|
||||
// stdin "end" listener so emitted triggers reach only this test's instance, then restore them.
|
||||
const signals: NodeJS.Signals[] = ["SIGTERM", "SIGHUP"];
|
||||
let savedSignalListeners: Array<[NodeJS.Signals, NodeJS.SignalsListener[]]> = [];
|
||||
let savedEndListeners: Array<() => void> = [];
|
||||
|
||||
beforeEach(() => {
|
||||
savedSignalListeners = signals.map((signal) => [signal, process.listeners(signal)]);
|
||||
savedEndListeners = process.stdin.listeners("end") as Array<() => void>;
|
||||
for (const signal of signals) process.removeAllListeners(signal);
|
||||
process.stdin.removeAllListeners("end");
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
for (const signal of signals) process.removeAllListeners(signal);
|
||||
process.stdin.removeAllListeners("end");
|
||||
for (const [signal, listeners] of savedSignalListeners) {
|
||||
for (const listener of listeners) process.on(signal, listener);
|
||||
}
|
||||
for (const listener of savedEndListeners) process.stdin.on("end", listener);
|
||||
rpcIo.outputLines = [];
|
||||
rpcIo.lineHandler = undefined;
|
||||
vi.restoreAllMocks();
|
||||
});
|
||||
|
||||
async function startWithSlowDispose() {
|
||||
const exitSpy = vi.spyOn(process, "exit").mockImplementation((() => undefined) as never);
|
||||
vi.spyOn(process.stdin, "pause").mockImplementation(() => process.stdin);
|
||||
const started = await startRpcMode({ withAuth: false, responseDelayMs: 0 });
|
||||
let finishDispose = () => {};
|
||||
const disposeFinished = new Promise<void>((resolve) => {
|
||||
finishDispose = resolve;
|
||||
});
|
||||
vi.mocked(started.runtimeHost.dispose).mockImplementation(() => disposeFinished);
|
||||
return { ...started, exitSpy, finishDispose };
|
||||
}
|
||||
|
||||
// fcatuhe's report on #11599: Paseo closes stdin and sends SIGTERM together.
|
||||
it("lets a shutdown started by stdin EOF finish when one SIGTERM follows", async () => {
|
||||
const { runtimeHost, exitSpy, finishDispose, cleanup } = await startWithSlowDispose();
|
||||
try {
|
||||
process.stdin.emit("end");
|
||||
await vi.waitFor(() => expect(runtimeHost.dispose).toHaveBeenCalledTimes(1));
|
||||
// Without a listener, a real SIGTERM would kill the process mid-dispose.
|
||||
expect(process.listenerCount("SIGTERM")).toBe(1);
|
||||
process.emit("SIGTERM");
|
||||
await sleep(10);
|
||||
expect(exitSpy).not.toHaveBeenCalled();
|
||||
|
||||
finishDispose();
|
||||
await vi.waitFor(() => expect(exitSpy).toHaveBeenCalledWith(0));
|
||||
expect(exitSpy).toHaveBeenCalledTimes(1);
|
||||
} finally {
|
||||
await cleanup();
|
||||
}
|
||||
});
|
||||
|
||||
it("lets a shutdown started by SIGTERM finish when stdin EOF follows", async () => {
|
||||
const { runtimeHost, exitSpy, finishDispose, cleanup } = await startWithSlowDispose();
|
||||
try {
|
||||
process.emit("SIGTERM");
|
||||
await vi.waitFor(() => expect(runtimeHost.dispose).toHaveBeenCalledTimes(1));
|
||||
process.stdin.emit("end");
|
||||
await sleep(10);
|
||||
expect(exitSpy).not.toHaveBeenCalled();
|
||||
|
||||
finishDispose();
|
||||
await vi.waitFor(() => expect(exitSpy).toHaveBeenCalledWith(143));
|
||||
expect(exitSpy).toHaveBeenCalledTimes(1);
|
||||
expect(runtimeHost.dispose).toHaveBeenCalledTimes(1);
|
||||
} finally {
|
||||
await cleanup();
|
||||
}
|
||||
});
|
||||
|
||||
it("exits at once on a repeated signal during shutdown", async () => {
|
||||
const { runtimeHost, exitSpy, cleanup } = await startWithSlowDispose();
|
||||
try {
|
||||
process.stdin.emit("end");
|
||||
await vi.waitFor(() => expect(runtimeHost.dispose).toHaveBeenCalledTimes(1));
|
||||
process.emit("SIGTERM");
|
||||
process.emit("SIGTERM");
|
||||
expect(exitSpy).toHaveBeenCalledTimes(1);
|
||||
expect(exitSpy).toHaveBeenCalledWith(143);
|
||||
} finally {
|
||||
await cleanup();
|
||||
}
|
||||
});
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue