From dd5c329bc3428cd2df8073289d8256937bc1a6d4 Mon Sep 17 00:00:00 2001 From: can1357 Date: Mon, 6 Jul 2026 08:07:24 +0200 Subject: [PATCH] fix(coding-agent): resolved browser teardown crashes by tracking expected abort errors - 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. --- .../src/tools/browser/cmux/cmux-tab.ts | 10 ++++-- .../src/tools/browser/run-cancellation.ts | 36 ++++++++++++++----- .../src/tools/browser/tab-protocol.ts | 2 +- .../src/tools/browser/tab-supervisor.ts | 8 ++--- .../src/tools/browser/tab-worker.ts | 26 ++++++++++---- .../coding-agent/src/tools/tool-errors.ts | 6 ++-- .../test/tools/tool-errors.test.ts | 22 ++++++++++++ 7 files changed, 84 insertions(+), 26 deletions(-) create mode 100644 packages/coding-agent/test/tools/tool-errors.test.ts diff --git a/packages/coding-agent/src/tools/browser/cmux/cmux-tab.ts b/packages/coding-agent/src/tools/browser/cmux/cmux-tab.ts index acb8bc602..0616812fc 100644 --- a/packages/coding-agent/src/tools/browser/cmux/cmux-tab.ts +++ b/packages/coding-agent/src/tools/browser/cmux/cmux-tab.ts @@ -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(); } } diff --git a/packages/coding-agent/src/tools/browser/run-cancellation.ts b/packages/coding-agent/src/tools/browser/run-cancellation.ts index 4680dd683..3d5057752 100644 --- a/packages/coding-agent/src/tools/browser/run-cancellation.ts +++ b/packages/coding-agent/src/tools/browser/run-cancellation.ts @@ -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(promise: Promise): Promise { + 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 { - throwIfAborted(signal); - await untilAborted(signal, () => Bun.sleep(ms)); - throwIfAborted(signal); +export function waitForBrowserRun(ms: number, signal: AbortSignal): Promise { + const promise = (async (): Promise => { + 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(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); diff --git a/packages/coding-agent/src/tools/browser/tab-protocol.ts b/packages/coding-agent/src/tools/browser/tab-protocol.ts index e7d50a8ee..d2f5d51bd 100644 --- a/packages/coding-agent/src/tools/browser/tab-protocol.ts +++ b/packages/coding-agent/src/tools/browser/tab-protocol.ts @@ -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" }; diff --git a/packages/coding-agent/src/tools/browser/tab-supervisor.ts b/packages/coding-agent/src/tools/browser/tab-supervisor.ts index 567ff6b74..921fb23ff 100644 --- a/packages/coding-agent/src/tools/browser/tab-supervisor.ts +++ b/packages/coding-agent/src/tools/browser/tab-supervisor.ts @@ -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 { 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") { diff --git a/packages/coding-agent/src/tools/browser/tab-worker.ts b/packages/coding-agent/src/tools/browser/tab-worker.ts index c09a5d827..677db7a6f 100644 --- a/packages/coding-agent/src/tools/browser/tab-worker.ts +++ b/packages/coding-agent/src/tools/browser/tab-worker.ts @@ -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(); 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 = (label: string, perOpMs: number, fn: (sig: AbortSignal) => Promise): Promise => - this.#runOp(active, label, signal, perOpMs, fn); + markHandled(this.#runOp(active, label, signal, perOpMs, fn)); return { name, page, diff --git a/packages/coding-agent/src/tools/tool-errors.ts b/packages/coding-agent/src/tools/tool-errors.ts index 546c73c23..f4a38e0b7 100644 --- a/packages/coding-agent/src/tools/tool-errors.ts +++ b/packages/coding-agent/src/tools/tool-errors.ts @@ -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 }); } } diff --git a/packages/coding-agent/test/tools/tool-errors.test.ts b/packages/coding-agent/test/tools/tool-errors.test.ts new file mode 100644 index 000000000..3fc95f4a3 --- /dev/null +++ b/packages/coding-agent/test/tools/tool-errors.test.ts @@ -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); + }); +});