From e09eda00cd1f3f4c106b881afd80013d5fe88dfc Mon Sep 17 00:00:00 2001 From: roboomp Date: Fri, 24 Jul 2026 07:25:47 +0000 Subject: [PATCH 1/2] fix(coding-agent): bypassed extension guard for shutdown - Captured the native hard-exit function for postmortem signal, fatal, and manual exits. - Added bounded child-process coverage for SIGINT and fatal cleanup during pending guarded loads. - Preserved extension and hook exit isolation and made the EPIPE race assertion observe the real exit event. Fixes #6488 --- packages/ai/src/auth-storage.ts | 2 +- .../auth-storage-oauth-account-select.test.ts | 2 +- packages/coding-agent/CHANGELOG.md | 1 + packages/coding-agent/src/live/transport.ts | 17 ++--- .../modes/controllers/selector-controller.ts | 2 +- .../coding-agent/src/session/auth-storage.ts | 2 +- .../extension-loader-process-exit.test.ts | 62 +++++++++++++++++++ packages/utils/CHANGELOG.md | 4 ++ packages/utils/src/postmortem.ts | 14 +++-- packages/utils/test/postmortem-epipe.test.ts | 13 ++-- 10 files changed, 87 insertions(+), 32 deletions(-) diff --git a/packages/ai/src/auth-storage.ts b/packages/ai/src/auth-storage.ts index c01c3220e..dba774da4 100644 --- a/packages/ai/src/auth-storage.ts +++ b/packages/ai/src/auth-storage.ts @@ -5245,7 +5245,7 @@ export class AuthStorage { const stored = this.#getStoredCredentials(provider); const index = stored.findIndex(entry => entry.id === credentialId); const target = stored[index]; - if (!target || target.credential.type !== "oauth") return false; + if (target?.credential.type !== "oauth") return false; this.#recordSessionCredential(provider, sessionId, "oauth", index); return true; } diff --git a/packages/ai/test/auth-storage-oauth-account-select.test.ts b/packages/ai/test/auth-storage-oauth-account-select.test.ts index 2e3034281..1f9abde53 100644 --- a/packages/ai/test/auth-storage-oauth-account-select.test.ts +++ b/packages/ai/test/auth-storage-oauth-account-select.test.ts @@ -2,8 +2,8 @@ import { afterEach, beforeEach, describe, expect, test, vi } from "bun:test"; import * as fs from "node:fs/promises"; import * as os from "node:os"; import * as path from "node:path"; -import { type AuthCredentialStore, AuthStorage, SqliteAuthCredentialStore } from "@oh-my-pi/pi-ai/auth-storage"; import { withOAuthAccess } from "@oh-my-pi/pi-ai/auth-retry"; +import { type AuthCredentialStore, AuthStorage, SqliteAuthCredentialStore } from "@oh-my-pi/pi-ai/auth-storage"; import * as oauthUtils from "@oh-my-pi/pi-ai/registry/oauth"; const PROVIDER = "unit-oauth-select"; diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 183f28f04..c368cd468 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -19,6 +19,7 @@ - Fixed spilled tool-output artifact descriptors leaking on error/abort paths. `OutputSink.dump()` was the only path that closed the spill `Bun.FileSink`, but the bash and Python executors re-throw on failure and their `finally` blocks never closed the sink, so a large-output command that errored leaked the artifact descriptor until an unrelated read (e.g. a `SKILL.md` load) hit `EMFILE`. `OutputSink` now exposes an idempotent `dispose()` that closes the sink exactly once, wired into every executor's `finally` ([#6463](https://github.com/can1357/oh-my-pi/issues/6463)). - Fixed the first submitted prompt stalling while the local tiny-title worker started: the interactive submit handler now paints the pending user row before starting title generation, and startup prewarms an idle, unref'd worker so the first submit reuses a live subprocess instead of paying spawn latency ahead of the first frame ([#6462](https://github.com/can1357/oh-my-pi/issues/6462)). - Fixed legacy Pi extensions failing validation when importing the upstream `keyText` keybinding helper ([#6470](https://github.com/can1357/oh-my-pi/issues/6470)). +- 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 diff --git a/packages/coding-agent/src/live/transport.ts b/packages/coding-agent/src/live/transport.ts index 9f0ffae3d..eea6c88c5 100644 --- a/packages/coding-agent/src/live/transport.ts +++ b/packages/coding-agent/src/live/transport.ts @@ -25,7 +25,6 @@ const LIVE_CALL_ID_PATTERN = /^rtc_[\w-]+$/; type Lifecycle = "idle" | "connecting" | "connected" | "closing" | "closed"; - interface LiveSignalingResult { answer: string; callId: string; @@ -115,7 +114,6 @@ function abortReason(signal: AbortSignal | undefined): Error { return new DOMException("Live connection aborted", "AbortError"); } - /** Native WebRTC transport for a Codex Frameless Bidi live session. */ export class CodexLiveTransport { readonly #options: LiveTransportOptions; @@ -226,7 +224,8 @@ export class CodexLiveTransport { throw new LiveSignalingError(response.status, `Codex live signaling failed (${response.status}): ${detail}`); } const answer = responseBody; - if (!answer.trim()) throw new LiveSignalingError(response.status, "Codex live signaling returned an empty SDP answer"); + if (!answer.trim()) + throw new LiveSignalingError(response.status, "Codex live signaling returned an empty SDP answer"); const callId = parseLiveCallId(response.headers.get("location")); if (!callId) { throw new LiveSignalingError(response.status, "Codex live signaling returned no valid call ID"); @@ -234,11 +233,7 @@ export class CodexLiveTransport { return { answer, callId, access, attestation }; } - async #connectSideband( - callId: string, - access: OAuthAccess, - attestation: string | undefined, - ): Promise { + async #connectSideband(callId: string, access: OAuthAccess, attestation: string | undefined): Promise { let failure = new Error("Codex live sideband connection failed"); for (let attempt = 0; attempt < SIDEBAND_CONNECT_ATTEMPTS; attempt++) { try { @@ -253,11 +248,7 @@ export class CodexLiveTransport { throw failure; } - async #openSideband( - callId: string, - access: OAuthAccess, - attestation: string | undefined, - ): Promise { + async #openSideband(callId: string, access: OAuthAccess, attestation: string | undefined): Promise { const url = buildLiveSidebandUrl(callId); const options = { headers: liveSessionHeaders(access, this.#options.sessionId, this.#realtimeSessionId, attestation), diff --git a/packages/coding-agent/src/modes/controllers/selector-controller.ts b/packages/coding-agent/src/modes/controllers/selector-controller.ts index 8a1dee626..396aea078 100644 --- a/packages/coding-agent/src/modes/controllers/selector-controller.ts +++ b/packages/coding-agent/src/modes/controllers/selector-controller.ts @@ -40,8 +40,8 @@ import { theme, } from "../../modes/theme/theme"; import type { InteractiveModeContext } from "../../modes/types"; -import type { ResetCreditAccountStatus, ResetCreditRedeemOutcome } from "../../session/auth-storage"; import type { SessionOAuthAccountList } from "../../session/agent-session-types"; +import type { ResetCreditAccountStatus, ResetCreditRedeemOutcome } from "../../session/auth-storage"; import type { SessionInfo } from "../../session/session-listing"; import { SessionManager } from "../../session/session-manager"; import { FileSessionStorage } from "../../session/session-storage"; diff --git a/packages/coding-agent/src/session/auth-storage.ts b/packages/coding-agent/src/session/auth-storage.ts index 0b58ed30a..8b40029f2 100644 --- a/packages/coding-agent/src/session/auth-storage.ts +++ b/packages/coding-agent/src/session/auth-storage.ts @@ -12,8 +12,8 @@ export type { AuthStorageOptions, CredentialOrigin, CredentialOriginKind, - OAuthAccountSummary, OAuthAccountIdentity, + OAuthAccountSummary, OAuthCredential, ResetCreditAccountStatus, ResetCreditRedeemOutcome, diff --git a/packages/coding-agent/test/extension-loader-process-exit.test.ts b/packages/coding-agent/test/extension-loader-process-exit.test.ts index ce21f503d..d7693682e 100644 --- a/packages/coding-agent/test/extension-loader-process-exit.test.ts +++ b/packages/coding-agent/test/extension-loader-process-exit.test.ts @@ -34,6 +34,49 @@ describe("extension/hook loader process.exit guard (#3680)", () => { return filePath; }; + const runGuardedShutdownProbe = async (trigger: "sigint" | "fatal") => { + const action = + trigger === "sigint" + ? 'process.kill(process.pid, "SIGINT");' + : 'void Promise.reject(new Error("probe fatal"));'; + const probe = ` +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); +}); +`; + 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 is only a hard failure bound. + 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); + } + }; + 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 +170,25 @@ describe("extension/hook loader process.exit guard (#3680)", () => { expect(process.exit).toBe(originalExit); }); + 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; diff --git a/packages/utils/CHANGELOG.md b/packages/utils/CHANGELOG.md index e0f8ad6be..36966503d 100644 --- a/packages/utils/CHANGELOG.md +++ b/packages/utils/CHANGELOG.md @@ -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 diff --git a/packages/utils/src/postmortem.ts b/packages/utils/src/postmortem.ts index 9377e9cbd..554f6987e 100644 --- a/packages/utils/src/postmortem.ts +++ b/packages/utils/src/postmortem.ts @@ -29,6 +29,8 @@ const callbackList: ((reason: Reason) => Promise | 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 | undefined; let stdioDisconnectRegistrations = 0; @@ -177,7 +179,7 @@ function formatFatalError(label: string, err: Error): string { } async function exitAfterFatal(label: string, logMessage: string, err: Error, reason: Reason): Promise { - 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 @@ -189,7 +191,7 @@ async function exitAfterFatal(label: string, logMessage: string, err: Error, rea await runCleanup(reason); } finally { clearTimeout(forcedExit); - process.exit(1); + exitProcess(1); } } @@ -197,7 +199,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; @@ -254,11 +256,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. @@ -341,5 +343,5 @@ export async function quit(code: number = 0): Promise { process.stdout.once("drain", resolve); await Promise.race([promise, Bun.sleep(5000)]); } - process.exit(code); + exitProcess(code); } diff --git a/packages/utils/test/postmortem-epipe.test.ts b/packages/utils/test/postmortem-epipe.test.ts index 71e7417d4..373eb47e2 100644 --- a/packages/utils/test/postmortem-epipe.test.ts +++ b/packages/utils/test/postmortem-epipe.test.ts @@ -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"); From c9363e2884b23347a762b436f33613015d87d442 Mon Sep 17 00:00:00 2001 From: roboomp Date: Fri, 24 Jul 2026 07:40:22 +0000 Subject: [PATCH 2/2] fix(utils): kept public quit behind host guard Routed postmortem.quit through the mutable process.exit path while retaining captured native exits for host-owned shutdown. Added child-process coverage proving guarded postmortem.quit calls surface ExtensionExitError without terminating the host. Fixes #6488 --- .../extension-loader-process-exit.test.ts | 60 ++++++++++++------- packages/utils/src/postmortem.ts | 28 ++++++--- 2 files changed, 59 insertions(+), 29 deletions(-) diff --git a/packages/coding-agent/test/extension-loader-process-exit.test.ts b/packages/coding-agent/test/extension-loader-process-exit.test.ts index d7693682e..8d3a20527 100644 --- a/packages/coding-agent/test/extension-loader-process-exit.test.ts +++ b/packages/coding-agent/test/extension-loader-process-exit.test.ts @@ -34,32 +34,14 @@ describe("extension/hook loader process.exit guard (#3680)", () => { return filePath; }; - const runGuardedShutdownProbe = async (trigger: "sigint" | "fatal") => { - const action = - trigger === "sigint" - ? 'process.kill(process.pid, "SIGINT");' - : 'void Promise.reject(new Error("probe fatal"));'; - const probe = ` -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); -}); -`; + 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 is only a hard failure bound. + // Real process signals cannot use fake timers; this only bounds a wedged child. const watchdog = setTimeout(() => { try { proc.kill("SIGKILL"); @@ -77,6 +59,27 @@ void withHostGuard(async () => { } }; + 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(); @@ -170,6 +173,23 @@ void withHostGuard(async () => { 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"); diff --git a/packages/utils/src/postmortem.ts b/packages/utils/src/postmortem.ts index 554f6987e..2613a78c1 100644 --- a/packages/utils/src/postmortem.ts +++ b/packages/utils/src/postmortem.ts @@ -233,7 +233,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)) { @@ -325,13 +325,7 @@ export function cleanup(): Promise { 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 { +async function runQuit(code: number, exitMode: "guarded" | "native"): Promise { await runCleanup(Reason.MANUAL); if (!isMainThread) { @@ -343,5 +337,21 @@ export async function quit(code: number = 0): Promise { process.stdout.once("drain", resolve); await Promise.race([promise, Bun.sleep(5000)]); } - exitProcess(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 { + return runQuit(code, "guarded"); }