Merge PR #6493: fix(coding-agent): bypass extension guards during shutdown (@roboomp)
This commit is contained in:
@@ -116,6 +116,7 @@
|
||||
- Fixed legacy Pi extensions failing validation when importing the upstream `keyText` keybinding helper ([#6470](https://github.com/can1357/oh-my-pi/issues/6470)).
|
||||
- Fixed Escape waiting for an in-flight `session_stop` extension handler to exhaust its timeout; abort now cancels the active stop pass without reporting a false timeout or applying stale continuation context ([#6489](https://github.com/can1357/oh-my-pi/issues/6489)).
|
||||
- Fixed the agent not resuming after re-answering a past `ask` from the session tree. Committing a new answer via `/tree` branched a fresh sibling `toolResult` and rebuilt context, but nothing ever continued the agent — unlike a live `ask`, whose continuation is intrinsic to the streaming run loop — so the model never consumed the new answer and the session sat idle until a manual prompt. `navigateTree` now reports the commit (`askReanswerCommitted`) and the interactive `/tree` handler resumes the agent via `resumeAfterAskReanswer()` *after* its transcript rebuild, so the resumed turn never renders against the stale pre-rebuild UI. Plain leaf moves and the read-only `reopenAsk` probe stay idle ([#6483](https://github.com/can1357/oh-my-pi/issues/6483)).
|
||||
- Fixed Ctrl+C and fatal shutdown entering an `ExtensionExitError` rejection loop while an extension or hook was still loading ([#6488](https://github.com/can1357/oh-my-pi/issues/6488)).
|
||||
|
||||
## [17.1.0] - 2026-07-24
|
||||
|
||||
|
||||
@@ -34,6 +34,52 @@ describe("extension/hook loader process.exit guard (#3680)", () => {
|
||||
return filePath;
|
||||
};
|
||||
|
||||
const runProbe = async (probe: string) => {
|
||||
const proc = Bun.spawn([process.execPath, "-e", probe], {
|
||||
cwd: path.resolve(import.meta.dir, "../../.."),
|
||||
stdin: "pipe",
|
||||
stdout: "pipe",
|
||||
stderr: "pipe",
|
||||
});
|
||||
// Real process signals cannot use fake timers; this only bounds a wedged child.
|
||||
const watchdog = setTimeout(() => {
|
||||
try {
|
||||
proc.kill("SIGKILL");
|
||||
} catch {}
|
||||
}, 2000);
|
||||
try {
|
||||
const [exitCode, stdout, stderr] = await Promise.all([
|
||||
proc.exited,
|
||||
new Response(proc.stdout).text(),
|
||||
new Response(proc.stderr).text(),
|
||||
]);
|
||||
return { exitCode, stdout, stderr };
|
||||
} finally {
|
||||
clearTimeout(watchdog);
|
||||
}
|
||||
};
|
||||
|
||||
const runGuardedShutdownProbe = (trigger: "sigint" | "fatal") => {
|
||||
const action =
|
||||
trigger === "sigint"
|
||||
? 'process.kill(process.pid, "SIGINT");'
|
||||
: 'void Promise.reject(new Error("probe fatal"));';
|
||||
return runProbe(`
|
||||
import { postmortem } from "@oh-my-pi/pi-utils";
|
||||
import { withHostGuard } from "@oh-my-pi/pi-coding-agent/extensibility/utils";
|
||||
|
||||
postmortem.register("probe-cleanup", reason => {
|
||||
process.stdout.write(\`cleanup:\${reason}\\n\`);
|
||||
});
|
||||
void withHostGuard(async () => {
|
||||
process.stdout.write("guard-active\\n");
|
||||
${action}
|
||||
// Keep the real child event loop alive so the platform can deliver SIGINT.
|
||||
await Bun.sleep(10_000);
|
||||
});
|
||||
`);
|
||||
};
|
||||
|
||||
it("converts a top-level process.exit in an extension into a load error", async () => {
|
||||
const ext = writeModule("rogue-extension.ts", "process.exit(0)\n");
|
||||
const cwd = project!.path();
|
||||
@@ -127,6 +173,42 @@ describe("extension/hook loader process.exit guard (#3680)", () => {
|
||||
expect(process.exit).toBe(originalExit);
|
||||
});
|
||||
|
||||
it("keeps postmortem.quit behind the extension exit guard", async () => {
|
||||
const { exitCode, stdout, stderr } = await runProbe(`
|
||||
import { postmortem } from "@oh-my-pi/pi-utils";
|
||||
import { withHostGuard } from "@oh-my-pi/pi-coding-agent/extensibility/utils";
|
||||
|
||||
try {
|
||||
await withHostGuard(() => postmortem.quit(37));
|
||||
} catch (err) {
|
||||
process.stdout.write(\`\${err instanceof Error ? err.name : "UnknownError"}:\${String(err)}\\n\`);
|
||||
}
|
||||
`);
|
||||
|
||||
expect(exitCode).toBe(0);
|
||||
expect(stdout).toContain("ExtensionExitError:ExtensionExitError: Module called process.exit(37)");
|
||||
expect(stderr).toBe("");
|
||||
});
|
||||
|
||||
it("lets host SIGINT exit once while a guarded callback remains pending", async () => {
|
||||
const { exitCode, stdout, stderr } = await runGuardedShutdownProbe("sigint");
|
||||
|
||||
expect(exitCode).toBe(130);
|
||||
expect(stdout).toBe("guard-active\ncleanup:sigint\n");
|
||||
expect(stderr).not.toContain("[Unhandled Rejection]");
|
||||
expect(stderr).not.toContain("ExtensionExitError");
|
||||
});
|
||||
|
||||
it("lets fatal cleanup exit once while a guarded callback remains pending", async () => {
|
||||
const { exitCode, stdout, stderr } = await runGuardedShutdownProbe("fatal");
|
||||
|
||||
expect(exitCode).toBe(1);
|
||||
expect(stdout).toBe("guard-active\ncleanup:unhandled_rejection\n");
|
||||
expect(stderr.match(/\[Unhandled Rejection\]/g)).toHaveLength(1);
|
||||
expect(stderr).toContain("Error: probe fatal");
|
||||
expect(stderr).not.toContain("ExtensionExitError");
|
||||
});
|
||||
|
||||
it("only the outermost guard restores process.exit when guards nest", async () => {
|
||||
const originalExit = process.exit;
|
||||
|
||||
|
||||
@@ -2,6 +2,10 @@
|
||||
|
||||
## [Unreleased]
|
||||
|
||||
### Fixed
|
||||
|
||||
- Fixed postmortem signal and fatal shutdown exits being intercepted by temporary `process.exit` guards during extension startup ([#6488](https://github.com/can1357/oh-my-pi/issues/6488)).
|
||||
|
||||
## [17.0.9] - 2026-07-23
|
||||
|
||||
### Breaking Changes
|
||||
|
||||
@@ -29,6 +29,8 @@ const callbackList: ((reason: Reason) => Promise<void> | void)[] = [];
|
||||
// Tracks cleanup run state (to prevent recursion/reentry issues)
|
||||
let cleanupStage: "idle" | "running" | "complete" = "idle";
|
||||
const CLEANUP_DEADLINE_MS = 10_000;
|
||||
const exitProcess =
|
||||
typeof process.reallyExit === "function" ? process.reallyExit.bind(process) : process.exit.bind(process);
|
||||
let cleanupPromise: Promise<void> | undefined;
|
||||
let stdioDisconnectRegistrations = 0;
|
||||
|
||||
@@ -171,7 +173,7 @@ function formatFatalError(label: string, err: Error): string {
|
||||
}
|
||||
|
||||
async function exitAfterFatal(label: string, logMessage: string, err: Error, reason: Reason): Promise<void> {
|
||||
const forcedExit = setTimeout(() => process.exit(1), CLEANUP_DEADLINE_MS);
|
||||
const forcedExit = setTimeout(() => exitProcess(1), CLEANUP_DEADLINE_MS);
|
||||
try {
|
||||
restoreTerminalStderr();
|
||||
// A revoked terminal can make stream writes raise another fatal error. Use
|
||||
@@ -183,7 +185,7 @@ async function exitAfterFatal(label: string, logMessage: string, err: Error, rea
|
||||
await runCleanup(reason);
|
||||
} finally {
|
||||
clearTimeout(forcedExit);
|
||||
process.exit(1);
|
||||
exitProcess(1);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -191,7 +193,7 @@ if (isMainThread) {
|
||||
process
|
||||
.on("SIGINT", async () => {
|
||||
await runCleanup(Reason.SIGINT);
|
||||
process.exit(130); // 128 + SIGINT (2)
|
||||
exitProcess(130); // 128 + SIGINT (2)
|
||||
})
|
||||
.on("SIGUSR1", () => {
|
||||
if (inspectorOpened) return;
|
||||
@@ -225,7 +227,7 @@ if (isMainThread) {
|
||||
}
|
||||
if (brokenPipeSource === "stdio-write" && stdioDisconnectRegistrations > 0) {
|
||||
logger.warn("Stdio peer disconnected; shutting down gracefully", { err });
|
||||
await quit(0);
|
||||
await runQuit(0, "native");
|
||||
return;
|
||||
}
|
||||
if (isExpectedCleanupError(reason)) {
|
||||
@@ -248,11 +250,11 @@ if (isMainThread) {
|
||||
})
|
||||
.on("SIGTERM", async () => {
|
||||
await runCleanup(Reason.SIGTERM);
|
||||
process.exit(143); // 128 + SIGTERM (15)
|
||||
exitProcess(143); // 128 + SIGTERM (15)
|
||||
})
|
||||
.on("SIGHUP", async () => {
|
||||
await runCleanup(Reason.SIGHUP);
|
||||
process.exit(129); // 128 + SIGHUP (1)
|
||||
exitProcess(129); // 128 + SIGHUP (1)
|
||||
});
|
||||
} else {
|
||||
// Worker thread: only register exit handler for cleanup.
|
||||
@@ -317,13 +319,7 @@ export function cleanup(): Promise<void> {
|
||||
return runCleanup(Reason.MANUAL);
|
||||
}
|
||||
|
||||
/**
|
||||
* Runs all cleanup callbacks and exits.
|
||||
*
|
||||
* In main thread: waits for stdout drain, then calls process.exit().
|
||||
* In workers: runs cleanup only (process.exit would kill entire process).
|
||||
*/
|
||||
export async function quit(code: number = 0): Promise<void> {
|
||||
async function runQuit(code: number, exitMode: "guarded" | "native"): Promise<void> {
|
||||
await runCleanup(Reason.MANUAL);
|
||||
|
||||
if (!isMainThread) {
|
||||
@@ -335,5 +331,21 @@ export async function quit(code: number = 0): Promise<void> {
|
||||
process.stdout.once("drain", resolve);
|
||||
await Promise.race([promise, Bun.sleep(5000)]);
|
||||
}
|
||||
process.exit(code);
|
||||
|
||||
switch (exitMode) {
|
||||
case "guarded":
|
||||
return process.exit(code);
|
||||
case "native":
|
||||
return exitProcess(code);
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Runs all cleanup callbacks and exits through the current `process.exit`.
|
||||
*
|
||||
* In main thread: waits for stdout drain, then calls `process.exit()`.
|
||||
* In workers: runs cleanup only (process.exit would kill entire process).
|
||||
*/
|
||||
export function quit(code: number = 0): Promise<void> {
|
||||
return runQuit(code, "guarded");
|
||||
}
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
import { describe, expect, it } from "bun:test";
|
||||
import * as fs from "node:fs";
|
||||
import { postmortem } from "@oh-my-pi/pi-utils";
|
||||
|
||||
const childFlag = "--stdio-epipe-child";
|
||||
@@ -21,15 +22,9 @@ if (childFlagIndex >= 0) {
|
||||
const marker = process.argv[process.argv.indexOf(raceChildFlag) + 1];
|
||||
if (!marker) throw new Error("Missing cleanup marker path");
|
||||
let cleanupComplete = false;
|
||||
let exitAttempted = false;
|
||||
const exit = process.exit;
|
||||
process.exit = ((code?: number) => {
|
||||
if (!exitAttempted) {
|
||||
exitAttempted = true;
|
||||
void Bun.write(marker, cleanupComplete ? "after cleanup" : "before cleanup").then(() => exit(code));
|
||||
}
|
||||
return undefined as never;
|
||||
}) as typeof process.exit;
|
||||
process.on("exit", () => {
|
||||
fs.writeFileSync(marker, cleanupComplete ? "after cleanup" : "before cleanup");
|
||||
});
|
||||
postmortem.registerStdioDisconnectHandling();
|
||||
postmortem.register("stdio-epipe-race-test", async () => {
|
||||
process.stderr.write("cleanup started\n");
|
||||
|
||||
Reference in New Issue
Block a user