From a6a8258945dd7d3ed534bf874c19180610dd4cf8 Mon Sep 17 00:00:00 2001 From: roboomp Date: Sat, 4 Jul 2026 06:24:45 +0000 Subject: [PATCH] fix(browser): propagate cmux tab-close into the run body, not only the caller MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex review of #4502 flagged that a bare `.catch(() => undefined)` neutralizes the unhandledRejection but leaves the affected `runInTab` call blocked inside `runCmuxCode` until timeout when the in-flight code does not make another cmux socket request (e.g. `await wait(60_000)`). `releaseTab` was signaling the run only by rejecting an orphaned promise. Wire the tab-close event all the way into the cmux run body: - `PendingRun` gains a `closeAc: AbortController` that `releaseTab` aborts BEFORE calling `pending.reject`. `wait(...)` (via `waitForBrowserRun` -> `untilAborted`), in-flight cmux socket calls (via CmuxTab's `#request` -> `untilAborted`), and facade proxies (via `bindBrowserRunFacade`) all consume the composed signal, so the run body unwinds within a microtask instead of blocking to its own timeout. - `runInTabWithSnapshot`'s cmux branch composes `closeAc.signal` into the run's signal (`AbortSignal.any([opts.signal, closeAc.signal])`) and now publishes `runCmuxCode(...)`'s outcome to the shared `promise` via `.then(resolve, reject)` and returns `await promise`. Both branches thus await the same promise, so `pending.reject` always has an attached handler (removing the original crash) AND the caller sees `Tab "..." was closed` immediately instead of waiting on the run's timeout. - Drop the defensive `promise.catch(() => undefined)` — the promise is now actively consumed on both backends. The new regression test adds a second case that exercises the reviewer's exact scenario (`await wait(60_000);`) and asserts: 1. `pending.closeAc.signal.aborted` flips from `false` to `true` across `releaseTab`, with the tab-close error as its reason. 2. The awaited `runInTab(...)` rejects with `Tab "..." was closed`. 3. No `unhandledRejection` fires. Verified locally by temporarily removing `closeAc.abort(...)` in `releaseTab` — the new assertions fail; restoring it makes them pass. Fixes #4499 --- packages/coding-agent/CHANGELOG.md | 2 +- .../src/tools/browser/tab-supervisor.ts | 61 ++++-- .../browser-cmux-release-mid-run.test.ts | 199 ++++++++++++++---- 3 files changed, 203 insertions(+), 59 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 011e547ed..e9b828f6c 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -4,7 +4,7 @@ ### Fixed -- Fixed cmux-backend `browser({action:"run"})` calls crashing the entire process with an unhandled rejection when the tab was released mid-run (e.g. a sibling subagent calling `browser({action:"close", all:true})` or a session-scoped tab reap). `runInTabWithSnapshot` in `tab-supervisor.ts` always creates a `Promise.withResolvers()` triple so `releaseTab` can signal in-flight runs, but the cmux branch awaits `runCmuxCode(...)` directly and never awaits/`.catch`es the local promise. When `releaseTab` rejected that orphaned promise ("Tab ... was closed"), Bun surfaced it as an unhandled rejection and the top-level handler tore the whole session down, killing every other tab and subagent sharing it. Attaching a no-op `.catch(() => undefined)` to the promise immediately after creation neutralizes the orphan without affecting the worker branch, which still awaits the same promise via `raceWithTimeout` ([#4499](https://github.com/can1357/oh-my-pi/issues/4499)). +- Fixed cmux-backend `browser({action:"run"})` calls crashing the entire process with an unhandled rejection when the tab was released mid-run (e.g. a sibling subagent calling `browser({action:"close", all:true})` or a session-scoped tab reap). `runInTabWithSnapshot` in `tab-supervisor.ts` creates a `Promise.withResolvers()` triple so `releaseTab` can signal in-flight runs, but the cmux branch used to await `runCmuxCode(...)` directly and never awaited the local promise. When `releaseTab` rejected that orphaned promise ("Tab ... was closed"), Bun surfaced it as an unhandled rejection and the top-level handler tore the whole session down, killing every other tab and subagent sharing it. Both backends now await the same `promise` (so `pending.reject` always has an attached handler AND the caller sees `Tab "..." was closed` immediately instead of blocking to the run's timeout), and a new `pending.closeAc` is composed into the cmux run's abort signal so `wait(...)`, in-flight cmux socket calls, and the facade proxies unwind promptly when the tab is closed rather than leaking to their own timeout ([#4499](https://github.com/can1357/oh-my-pi/issues/4499)). ## [16.3.5] - 2026-07-04 diff --git a/packages/coding-agent/src/tools/browser/tab-supervisor.ts b/packages/coding-agent/src/tools/browser/tab-supervisor.ts index e691f18f7..567ff6b74 100644 --- a/packages/coding-agent/src/tools/browser/tab-supervisor.ts +++ b/packages/coding-agent/src/tools/browser/tab-supervisor.ts @@ -48,6 +48,16 @@ export interface PendingRun { session: ToolSession; signal?: AbortSignal; toolCalls: Map; + /** + * Fires when `releaseTab` closes the tab out from under an in-flight run + * (sibling `browser close --all`, session-scoped reap, etc.). Composed + * into the cmux run's signal so `wait(...)`, cmux socket calls, and the + * facade proxies unwind promptly instead of blocking to the run's + * timeout. `pending.reject` still fires first so the awaiting caller + * sees the tab-close error immediately; `closeAc` propagates the + * cancellation into the still-running `runCmuxCode` body (issue #4499). + */ + closeAc?: AbortController; } interface TabSessionBase { @@ -380,34 +390,47 @@ async function runInTabWithSnapshot( if (tab.pending.size > 0) throw new ToolError(`Tab ${JSON.stringify(name)} is busy`); const id = Snowflake.next(); const { promise, resolve, reject } = Promise.withResolvers(); - // The cmux branch below never awaits `promise` — it awaits `runCmuxCode` - // directly, and only stashes {resolve, reject} here so `releaseTab` can - // signal in-flight runs when a tab dies out from under them (e.g. a - // sibling `browser close --all` while this run is in flight). With zero - // consumers, that `reject(...)` call surfaces as an unhandled rejection - // and the top-level handler tears the whole process down (issue #4499: - // "Tab ... was closed"). Attaching a no-op catch here is safe for the - // worker branch too — it still awaits the same promise via - // `raceWithTimeout` below, and adding a second handler to a promise is - // inert. - promise.catch(() => undefined); + // `releaseTab` calls `pending.reject(closeError)` when the tab dies + // out from under an in-flight run (sibling `browser close --all`, + // session-scoped reap, etc.). Both backends below MUST end up awaiting + // this same `promise` so: + // 1. The caller sees `Tab ... was closed` immediately instead of + // blocking to the run's timeout, and + // 2. `reject(...)` always has an attached handler — a zero-consumer + // rejection would fire `unhandledRejection` and the CLI's + // top-level handler would tear the whole session down, killing + // every other tab and subagent sharing the process (issue #4499). + // The cmux branch also composes `closeAc.signal` into the run's abort + // signal so `wait(...)`, cmux socket calls, and the facade proxies + // unwind promptly when the tab is closed — otherwise a `wait(60_000)` + // with no in-flight socket request would keep `runCmuxCode` blocked + // until timeout even after the tab is gone. + const closeAc = new AbortController(); const pending: PendingRun = { resolve, reject, session: opts.session ?? ({} as ToolSession), signal: opts.signal, toolCalls: new Map(), + closeAc, }; tab.pending.set(id, pending); if (tab.backend === "cmux") { + const runSignal = opts.signal ? AbortSignal.any([opts.signal, closeAc.signal]) : closeAc.signal; try { - return await runCmuxCode(tab.cmuxTab, { + // `runCmuxCode.then(resolve, reject)` publishes the run's real + // outcome to `promise`, but `releaseTab` may have already + // rejected it — `Promise.withResolvers` settles on the first + // call and later resolve/reject are no-ops, so the tab-close + // error still wins the race. + runCmuxCode(tab.cmuxTab, { code: opts.code, timeoutMs: opts.timeoutMs, - signal: opts.signal, + signal: runSignal, session: pending.session, snapshot, - }); + }).then(resolve, reject); + return await promise; } finally { tab.pending.delete(id); } @@ -471,6 +494,16 @@ export async function releaseTab(name: string, opts: ReleaseTabOptions = {}): Pr } catch {} } for (const ctrl of pending.toolCalls.values()) ctrl.abort(closeError); + // Propagate the closure into the cmux run's abort signal so + // `wait(...)`, in-flight cmux socket calls, and the facade proxies + // unwind promptly. Firing this BEFORE `pending.reject` means + // `runCmuxCode` finishes with `ToolAbortError` and its `.then(reject)` + // is a no-op — `promise` still settles with the tab-close error via + // the `reject` call below. Without it, a run that isn't currently + // making a socket request (e.g. `await wait(60_000)`) would keep + // `runCmuxCode` blocked until timeout even after `pending.reject` + // unblocked the caller (issue #4499 review feedback). + pending.closeAc?.abort(closeError); pending.reject(closeError); } tab.pending.clear(); diff --git a/packages/coding-agent/test/tools/browser-cmux-release-mid-run.test.ts b/packages/coding-agent/test/tools/browser-cmux-release-mid-run.test.ts index cc1330552..cac98e2c6 100644 --- a/packages/coding-agent/test/tools/browser-cmux-release-mid-run.test.ts +++ b/packages/coding-agent/test/tools/browser-cmux-release-mid-run.test.ts @@ -2,18 +2,30 @@ * Regression test for issue #4499: closing a cmux-backend tab while a * `browser({ action: "run" })` call is in flight rejected an orphaned * `Promise.withResolvers()` promise created in `runInTabWithSnapshot`. The - * cmux branch awaits `runCmuxCode(...)` directly and never awaits/`.catch`es - * the local `promise`; only `pending.reject` was stashed on the tab so - * `releaseTab` could signal in-flight runs. Zero consumers meant that - * `reject(...)` surfaced as an unhandled rejection and the top-level - * `unhandledRejection` handler tore the whole process down (killing sibling - * tabs and subagents). + * cmux branch originally awaited `runCmuxCode(...)` directly and never + * awaited/`.catch`ed the local `promise`; only `pending.reject` was stashed + * on the tab so `releaseTab` could signal in-flight runs. Zero consumers + * meant that `reject(...)` surfaced as an unhandled rejection and the + * top-level `unhandledRejection` handler tore the whole process down + * (killing sibling tabs and subagents). * - * The fix in `runInTabWithSnapshot` attaches a no-op `.catch(() => undefined)` - * to that promise immediately after creation. This test drives real - * `acquireBrowser` / `acquireTab` / `runInTab` / `releaseTab` against a - * mocked `CmuxSocketClient` and asserts that racing `releaseTab` against an - * in-flight cmux run never triggers `process.on("unhandledRejection", ...)`. + * The fix in `runInTabWithSnapshot` makes both backends await the same + * `promise` (so `pending.reject` always has an attached handler AND the + * caller sees the tab-close error immediately) and composes a new + * `pending.closeAc` into the cmux run's abort signal, so `wait(...)` / + * in-flight cmux socket calls / facade proxies unwind promptly when the + * tab is closed. This test drives real `acquireBrowser` / `acquireTab` / + * `runInTab` / `releaseTab` against a mocked `CmuxSocketClient` and covers: + * + * 1. Racing `releaseTab` against an in-flight cmux run never triggers + * `process.on("unhandledRejection", ...)` — the original crash — AND + * the awaiting `runInTab` call now rejects with `Tab ... was closed` + * immediately instead of blocking to the run's timeout. + * 2. When the in-flight run is doing work that does NOT make another cmux + * socket request (e.g. `await wait(60_000)`), releasing the tab still + * unwinds the run — proving `closeAc.signal` reaches `waitForBrowserRun` + * and the facade proxies, not just the outer race. (Reviewer feedback + * from PR #4502.) */ import { afterEach, describe, expect, it, spyOn } from "bun:test"; @@ -27,7 +39,6 @@ import { runInTab, } from "@oh-my-pi/pi-coding-agent/tools/browser/tab-supervisor"; import type { ToolSession } from "@oh-my-pi/pi-coding-agent/tools/index"; -import { ToolAbortError } from "@oh-my-pi/pi-coding-agent/tools/tool-errors"; function makeKind(socketSuffix: string): CmuxKind { return { @@ -60,7 +71,7 @@ describe("browser tab-supervisor — cmux tab close mid-run (#4499)", () => { await drainAllTabs(); }); - it("releaseTab() during an in-flight cmux run does not emit unhandledRejection", async () => { + it("releaseTab() during an in-flight cmux run rejects the run and never emits unhandledRejection", async () => { spyOn(CmuxSocketClient.prototype, "connect").mockResolvedValue(undefined); spyOn(CmuxSocketClient.prototype, "close").mockImplementation(() => undefined); @@ -72,8 +83,8 @@ describe("browser tab-supervisor — cmux tab close mid-run (#4499)", () => { // -> this mock). This is the deterministic "the run is mid-flight" edge. const navStarted = Promise.withResolvers(); // Gate for the mocked `browser.navigate` response. Left pending across - // the window we care about, then resolved during teardown so - // `runCmuxCode` can settle and does not leak past the test. + // the window we care about, then resolved during teardown so nothing + // leaks past the test. const navGate = Promise.withResolvers>(); spyOn(CmuxSocketClient.prototype, "request").mockImplementation( @@ -86,9 +97,8 @@ describe("browser tab-supervisor — cmux tab close mid-run (#4499)", () => { case "browser.snapshot": return { page: { html: "" } }; case "browser.eval": - // Used by `readyInfo()` for `document.title` and geometry - // during `acquireCmuxTab` — return quickly so setup lands - // and the run gets a chance to reach `tab.goto` below. + // `readyInfo()` needs `document.title` + geometry during + // `acquireCmuxTab`; return quickly so setup lands. return { value: "" }; case "browser.navigate": navStarted.resolve(); @@ -108,8 +118,6 @@ describe("browser tab-supervisor — cmux tab close mid-run (#4499)", () => { }; process.on("unhandledRejection", onUnhandled); - const runAc = new AbortController(); - let runPromise: Promise | undefined; try { const kind = makeKind("close-mid-run"); const browser = await acquireBrowser(kind, { cwd: "/tmp" }); @@ -121,18 +129,16 @@ describe("browser tab-supervisor — cmux tab close mid-run (#4499)", () => { const session = makeSession("/tmp"); // Fire the run WITHOUT awaiting. `runtime.run` drives `tab.goto`, - // which drives `browser.navigate`, which stalls on `navGate`. So - // `runInTab` sits inside `runCmuxCode` with `tab.pending` holding - // the orphaned promise. Attach a swallowing catch so the eventual - // rejection from `runAc.abort()` in the finally block does not - // itself become an unhandled rejection. - runPromise = runInTab("docfinal", { + // which drives `browser.navigate`, which stalls on `navGate` — so + // `runInTab` sits inside `runCmuxCode` with `tab.pending` populated. + // The 60_000ms timeout is intentional: on `main` (or with only the + // no-op-catch fix), the call would block until this fires; a passing + // test proves `releaseTab` unblocks it immediately via `promise`. + const runPromise = runInTab("docfinal", { code: 'await tab.goto("https://example.test");', timeoutMs: 60_000, session, - signal: runAc.signal, }); - runPromise.catch(() => undefined); // Deterministic wait: proceed only once the cmux request is actually // mid-flight (and therefore `tab.pending` is populated). @@ -140,30 +146,135 @@ describe("browser tab-supervisor — cmux tab close mid-run (#4499)", () => { const tabBeforeRelease = getTabsMapForTest().get("docfinal"); expect(tabBeforeRelease?.pending.size).toBeGreaterThan(0); - // This is the crash path from the reporter: `releaseTab` walks - // `tab.pending` and calls `pending.reject(new ToolError("Tab ... was closed"))`. - // Without the no-op catch on the orphaned promise, that rejection - // would surface as an unhandled rejection on the next microtask tick. + // `releaseTab` walks `tab.pending` and calls `pending.reject(new + // ToolError("Tab ... was closed"))`. On `main` this rejected an + // orphaned promise (unhandledRejection -> fatal). With the fix, + // the same reject settles the promise the caller is awaiting, so + // `runInTab` finishes with `Tab "docfinal" was closed` + // immediately — no 60s timeout wait. const released = await releaseTab("docfinal", { kill: false }); expect(released).toBe(true); - // Drain the microtask queue so any pending unhandled-rejection would - // have fired by the time we assert. Two microtask ticks matches the - // pattern in `ipc-safe-send.test.ts` (one for the rejection, one for - // a downstream handler); a few extra loops cover chained handlers. - for (let i = 0; i < 8; i++) await Promise.resolve(); + await expect(runPromise).rejects.toThrow(/Tab "docfinal" was closed/); + // Drain the microtask queue so any pending unhandled-rejection + // would have fired by the time we assert. + for (let i = 0; i < 8; i++) await Promise.resolve(); expect(unhandled).toEqual([]); - // Sanity: the tab really did leave the map. expect(getTabsMapForTest().has("docfinal")).toBe(false); } finally { - // Unblock the stalled `browser.navigate` and abort the run so - // `runInTab` can settle. Drop the listener last so the drain in - // `afterEach` does not accidentally trip a stray assertion in a - // follow-up test. - runAc.abort(new ToolAbortError("test cleanup")); + // Unblock the stalled `browser.navigate` so the abort signal + // composed into the cmux run gets a chance to short-circuit the + // in-flight request cleanly instead of leaking past the test. navGate.resolve({ url: "https://example.test" }); - await runPromise?.catch(() => undefined); + process.removeListener("unhandledRejection", onUnhandled); + } + }); + + it("releaseTab() unblocks a cmux run that is not making any socket request (wait(...) mid-flight)", async () => { + spyOn(CmuxSocketClient.prototype, "connect").mockResolvedValue(undefined); + spyOn(CmuxSocketClient.prototype, "close").mockImplementation(() => undefined); + + spyOn(CmuxSocketClient.prototype, "request").mockImplementation( + async (method: string, _params: Record): Promise> => { + switch (method) { + case "browser.open_split": + return { surface_id: "surface-wait-mid-run", url: "about:blank" }; + case "browser.url.get": + return { url: "about:blank" }; + case "browser.snapshot": + return { page: { html: "" } }; + case "browser.eval": + return { value: "" }; + case "browser.wait": + case "surface.close": + return {}; + default: + return {}; + } + }, + ); + + const unhandled: unknown[] = []; + const onUnhandled = (reason: unknown): void => { + unhandled.push(reason); + }; + process.on("unhandledRejection", onUnhandled); + + try { + const kind = makeKind("wait-mid-run"); + const browser = await acquireBrowser(kind, { cwd: "/tmp" }); + const acquired = await acquireTab("docfinal", browser, { + timeoutMs: 5_000, + ownerSessionId: "session-wait-mid-run", + }); + expect(acquired.tab.backend).toBe("cmux"); + + const session = makeSession("/tmp"); + // The user code awaits `wait(60_000)` — which drives + // `waitForBrowserRun(60_000, signal)` -> `untilAborted(signal, + // () => Bun.sleep(60_000))` INSIDE the runtime. Nothing hits the + // cmux socket, so on `main` the reviewer's exact scenario applies: + // even after `pending.reject` unblocks the caller, `runCmuxCode` + // stays blocked in `Bun.sleep(60_000)` until the run's timeout, + // leaking the run past the tab lifetime. + // + // The fix composes `pending.closeAc.signal` into the run's abort + // signal, so `releaseTab` cancels `untilAborted` synchronously and + // the run unwinds within a microtask window. + const runPromise = runInTab("docfinal", { + code: "await wait(60_000);", + timeoutMs: 60_000, + session, + }); + + // Spin the microtask queue until the pending map is populated + // (`runInTab` sets it synchronously before the first await, but the + // call itself is async). One tick usually suffices; a small + // bounded loop keeps the test robust against future micro-batching + // changes without relying on real timers. + for (let i = 0; i < 32; i++) { + const tab = getTabsMapForTest().get("docfinal"); + if (tab && tab.pending.size > 0) break; + await Promise.resolve(); + } + const tabBeforeRelease = getTabsMapForTest().get("docfinal"); + expect(tabBeforeRelease?.pending.size).toBeGreaterThan(0); + + // Capture the pending run's `closeAc` BEFORE `releaseTab` clears + // the map. This is the wire the reviewer asked us to check: the + // tab-close event must reach the cmux run body, not only the + // awaiting caller. Its `.signal.aborted` is the observable proof + // that `waitForBrowserRun` / cmux socket calls will unwind + // synchronously (via `untilAborted`) instead of blocking to the + // 60_000ms timeout. + const pendingBeforeRelease = [...(tabBeforeRelease?.pending.values() ?? [])]; + expect(pendingBeforeRelease.length).toBe(1); + const capturedCloseAc = pendingBeforeRelease[0]?.closeAc; + expect(capturedCloseAc).toBeDefined(); + expect(capturedCloseAc?.signal.aborted).toBe(false); + + // The scenario the reviewer flagged: no in-flight cmux request, + // so only the `closeAc` propagation can unwind the run body. + const released = await releaseTab("docfinal", { kill: false }); + expect(released).toBe(true); + + // Concrete contract: `releaseTab` MUST fire `closeAc.abort(...)` so + // the composed `runSignal` in `runInTabWithSnapshot` transitions + // to aborted. Without this line, the reviewer's failure mode + // stands: the run body keeps executing until its own timeout. + expect(capturedCloseAc?.signal.aborted).toBe(true); + expect(capturedCloseAc?.signal.reason).toBeInstanceOf(Error); + expect((capturedCloseAc?.signal.reason as Error).message).toMatch(/Tab "docfinal" was closed/); + + // Caller-facing contract: `runInTab` rejects with the tab-close + // error immediately, not after the run's 60_000ms timeout. + await expect(runPromise).rejects.toThrow(/Tab "docfinal" was closed/); + + for (let i = 0; i < 8; i++) await Promise.resolve(); + expect(unhandled).toEqual([]); + expect(getTabsMapForTest().has("docfinal")).toBe(false); + } finally { process.removeListener("unhandledRejection", onUnhandled); } });