smashimo/oh-my-pi
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(coding-agent): resolved browser teardown crashes by tracking expected abort errors

Browse Source 
- Added `markHandled` helper to prevent unhandled promise rejections in fire-and-forget browser tasks.
- Integrated `postmortem.markExpectedCleanupError` to distinguish between expected teardown aborts and actual runtime failures.
- Updated `runCmuxCode` and `WorkerCore` to propagate abort reasons via `ToolAbortError` cause chains.
- Modified `tab-protocol` and supervisor logic to signal expected cleanup states during tab release.
- Added test coverage for `ToolAbortError` wrapping and cause preservation.
This commit is contained in:
can1357
2026-07-06 08:07:24 +02:00
parent 45d95e7bf9
commit dd5c329bc3
7 changed files with 84 additions and 26 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/coding-agent/src/tools/browser/cmux/cmux-tab.ts
+7 -3
View File
@@ -1,7 +1,7 @@
import * as fs from "node:fs";
import * as os from "node:os";
import * as path from "node:path";
import { logger, Snowflake, untilAborted } from "@oh-my-pi/pi-utils";
import { logger, postmortem, Snowflake, untilAborted } from "@oh-my-pi/pi-utils";
import { JsRuntime, type RuntimeHooks } from "../../../eval/js/shared/runtime";
import type { JsDisplayOutput } from "../../../eval/js/shared/types";
import { callSessionTool } from "../../../eval/js/tool-bridge";
@@ -1306,7 +1306,11 @@ export async function runCmuxCode(tab: CmuxTab, opts: RunCmuxCodeOptions): Promi
if (timeoutSignal.aborted) {
reject(new ToolError(`Browser code execution timed out after ${opts.timeoutMs}ms`));
} else {
reject(new ToolAbortError());
reject(
signal.reason instanceof ToolAbortError
? signal.reason
: new ToolAbortError(undefined, { cause: signal.reason }),
);
}
};
if (signal.aborted) onAbort();
@@ -1336,7 +1340,7 @@ export async function runCmuxCode(tab: CmuxTab, opts: RunCmuxCodeOptions): Promi
return { displays, returnValue: cloneSafe(returnValue), screenshots };
} finally {
signal.removeEventListener("abort", onAbort);
runAc.abort(new ToolAbortError("Browser run ended"));
runAc.abort(postmortem.markExpectedCleanupError(new ToolAbortError("Browser run ended")));
tab.clearRunContext();
}
}
packages/coding-agent/src/tools/browser/run-cancellation.ts
+28 -8
View File
@@ -1,11 +1,29 @@
import { untilAborted } from "@oh-my-pi/pi-utils";
import { throwIfAborted } from "../tool-errors";
/**
* Marks a run-scoped promise as observed without changing its behavior for awaited callers.
*
* Browser run teardown aborts can reject promises created for evaluated code after user code
* has stopped observing them (for example fire-and-forget `wait()`/facade calls). In 16.3.0
* those zero-consumer rejections reached the process-level `unhandledRejection` handler and
* killed every subagent sharing the process (issues #4499/#4672). Attaching a no-op rejection
* handler at creation makes the promise observed while returning the original promise so callers
* that do await it still receive the rejection.
*/
export function markHandled<T>(promise: Promise<T>): Promise<T> {
void promise.catch(() => undefined);
return promise;
}
/** Sleeps inside evaluated browser code while honoring the owning run's cancellation signal. */
export async function waitForBrowserRun(ms: number, signal: AbortSignal): Promise<void> {
throwIfAborted(signal);
await untilAborted(signal, () => Bun.sleep(ms));
throwIfAborted(signal);
export function waitForBrowserRun(ms: number, signal: AbortSignal): Promise<void> {
const promise = (async (): Promise<void> => {
throwIfAborted(signal);
await untilAborted(signal, () => Bun.sleep(ms));
throwIfAborted(signal);
})();
return markHandled(promise);
}
/** Binds a long-lived browser facade to one evaluated run's abort signal. */
@@ -24,10 +42,12 @@ export function bindBrowserRunFacade<T extends object>(target: T, signal: AbortS
if (result && typeof result === "object") {
const then = Reflect.get(result, "then");
if (typeof then === "function") {
return Promise.resolve(result).then(resolved => {
throwIfAborted(signal);
return resolved;
});
return markHandled(
Promise.resolve(result).then(resolved => {
throwIfAborted(signal);
return resolved;
}),
);
}
}
throwIfAborted(signal);
packages/coding-agent/src/tools/browser/tab-protocol.ts
+1 -1
View File
@@ -66,7 +66,7 @@ export type ToolReply = { ok: true; value: unknown } | { ok: false; error: RunEr
export type WorkerInbound =
| { type: "init"; payload: WorkerInitPayload }
| { type: "run"; id: string; name: string; code: string; timeoutMs: number; session: SessionSnapshot }
| { type: "abort"; id: string }
| { type: "abort"; id: string; expectedCleanup?: boolean }
| { type: "tool-reply"; id: string; reply: ToolReply }
| { type: "close" };
packages/coding-agent/src/tools/browser/tab-supervisor.ts
+4 -4
View File
@@ -1,4 +1,4 @@
import { getPuppeteerDir, logger, Snowflake, workerHostEntry } from "@oh-my-pi/pi-utils";
import { getPuppeteerDir, logger, postmortem, Snowflake, workerHostEntry } from "@oh-my-pi/pi-utils";
import type { Page, Target } from "puppeteer-core";
import { callSessionTool } from "../../eval/js/tool-bridge";
import { webpExclusionForModel } from "../../utils/image-loading";
@@ -486,11 +486,11 @@ export async function releaseTab(name: string, opts: ReleaseTabOptions = {}): Pr
}
const wasAlive = tab.state === "alive";
tab.state = "dead";
const closeError = new ToolError(`Tab ${JSON.stringify(name)} was closed`);
const closeError = postmortem.markExpectedCleanupError(new ToolError(`Tab ${JSON.stringify(name)} was closed`));
for (const [id, pending] of tab.pending) {
if (tab.backend === "worker") {
try {
tab.worker.send({ type: "abort", id });
tab.worker.send({ type: "abort", id, expectedCleanup: true });
} catch {}
}
for (const ctrl of pending.toolCalls.values()) ctrl.abort(closeError);
@@ -744,7 +744,7 @@ async function forceKillTab(name: string, reason: string): Promise<void> {
const tab = tabs.get(name);
if (!tab) return;
tab.state = "dead";
const error = new ToolError(reason);
const error = postmortem.markExpectedCleanupError(new ToolError(reason));
for (const pending of tab.pending.values()) pending.reject(error);
tab.pending.clear();
if (tab.backend === "cmux") {
packages/coding-agent/src/tools/browser/tab-worker.ts
+19 -7
View File
@@ -2,7 +2,7 @@ import * as fs from "node:fs";
import * as os from "node:os";
import * as path from "node:path";
import { Snowflake, untilAborted } from "@oh-my-pi/pi-utils";
import { postmortem, Snowflake, untilAborted } from "@oh-my-pi/pi-utils";
import type { HTMLElement } from "linkedom";
import type {
Browser,
@@ -36,7 +36,7 @@ import {
loadPuppeteerInWorker,
} from "./launch";
import { extractReadableFromHtml, type ReadableFormat } from "./readable";
import { waitForBrowserRun } from "./run-cancellation";
import { markHandled, waitForBrowserRun } from "./run-cancellation";
import type {
Observation,
ObservationEntry,
@@ -593,7 +593,12 @@ export class WorkerCore {
await this.#run(msg);
return;
case "abort":
if (this.#active?.id === msg.id) this.#active.ac.abort(new ToolAbortError());
if (this.#active?.id === msg.id) {
const reason = msg.expectedCleanup
? postmortem.markExpectedCleanupError(new ToolAbortError())
: new ToolAbortError();
this.#active.ac.abort(reason);
}
return;
case "tool-reply":
this.#deliverToolReply(msg.id, msg.reply);
@@ -732,6 +737,10 @@ export class WorkerCore {
});
const { promise: cancelRejection, reject: rejectCancel } = Promise.withResolvers<never>();
const onCancel = (): void => {
const abortError =
signal.reason instanceof ToolAbortError
? signal.reason
: new ToolAbortError(undefined, { cause: signal.reason });
if (timeoutSignal.aborted) {
const stalled = describeInflight(active.inflight);
rejectCancel(
@@ -740,11 +749,14 @@ export class WorkerCore {
),
);
} else {
rejectCancel(new ToolAbortError());
rejectCancel(abortError);
}
// Cancel in-flight tool calls so user code's awaited proxies reject promptly.
const toolAbort = timeoutSignal.aborted
? postmortem.markExpectedCleanupError(new ToolAbortError(undefined, { cause: timeoutSignal.reason }))
: abortError;
for (const pending of active.pendingTools.values()) {
pending.reject(new ToolAbortError());
pending.reject(toolAbort);
}
active.pendingTools.clear();
};
@@ -771,7 +783,7 @@ export class WorkerCore {
this.#transport.send({ type: "result", id: msg.id, ok: false, error: errorPayload(error) });
} finally {
if (this.#active?.id === msg.id) this.#active = null;
runAc.abort(new ToolAbortError("Browser run ended"));
runAc.abort(postmortem.markExpectedCleanupError(new ToolAbortError("Browser run ended")));
}
}
@@ -889,7 +901,7 @@ export class WorkerCore {
const waitMs = (explicit?: number): number => resolveWaitTimeout(timeoutMs, explicit);
const INF = Number.POSITIVE_INFINITY;
const op = <T>(label: string, perOpMs: number, fn: (sig: AbortSignal) => Promise<T>): Promise<T> =>
this.#runOp(active, label, signal, perOpMs, fn);
markHandled(this.#runOp(active, label, signal, perOpMs, fn));
return {
name,
page,
packages/coding-agent/src/tools/tool-errors.ts
+3 -3
View File
@@ -30,8 +30,8 @@ export class ToolError extends Error {
export class ToolAbortError extends Error {
static readonly MESSAGE = "Operation aborted";
constructor(message: string = ToolAbortError.MESSAGE) {
super(message);
constructor(message: string = ToolAbortError.MESSAGE, options?: ErrorOptions) {
super(message, options);
this.name = "ToolAbortError";
}
}
@@ -43,7 +43,7 @@ export class ToolAbortError extends Error {
export function throwIfAborted(signal?: AbortSignal): void {
if (signal?.aborted) {
const reason = signal.reason instanceof Error ? signal.reason : undefined;
throw reason instanceof ToolAbortError ? reason : new ToolAbortError();
throw reason instanceof ToolAbortError ? reason : new ToolAbortError(undefined, { cause: signal.reason });
}
}
packages/coding-agent/test/tools/tool-errors.test.ts
+22
View File
@@ -0,0 +1,22 @@
import { describe, expect, it } from "bun:test";
import { postmortem } from "@oh-my-pi/pi-utils";
import { ToolAbortError, throwIfAborted } from "../../src/tools/tool-errors";
describe("tool abort errors", () => {
it("wraps non-ToolAbortError abort reasons as ToolAbortError while preserving marked cause chains", () => {
const controller = new AbortController();
const reason = postmortem.markExpectedCleanupError(new Error("browser run ended"));
controller.abort(reason);
let caught: unknown;
try {
throwIfAborted(controller.signal);
} catch (error) {
caught = error;
}
expect(caught).toBeInstanceOf(ToolAbortError);
expect((caught as Error).cause).toBe(reason);
expect(postmortem.isExpectedCleanupError(caught)).toBe(true);
});
});