fix(browser): propagate cmux tab-close into the run body, not only the caller

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

@@ -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
 

packages/coding-agent/src/tools/browser/tab-supervisor.ts

@@ -48,6 +48,16 @@ export interface PendingRun {
 	session: ToolSession;
 	signal?: AbortSignal;
 	toolCalls: Map<string, AbortController>;
+	/**
+	 * 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<TBrowser extends BrowserHandle = BrowserHandle> {
@@ -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<RunResultOk>();
-	// 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();

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<void>();
 		// 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<Record<string, unknown>>();
 
 		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<unknown> | 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<string, unknown>): Promise<Record<string, unknown>> => {
+				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);
 		}
 	});