From 319d988b7215a8fb45541477f226784543d2e11f Mon Sep 17 00:00:00 2001 From: Saoud Rizwan <7799382+saoudrizwan@users.noreply.github.com> Date: Wed, 29 Apr 2026 17:16:58 -0700 Subject: [PATCH] fix(core): observe public run promise during abort (#312) ## Problem Renee reported that after cancelling a request mid-run in `apps/cli`, the hub seemed to die. The next request failed with a WebSocket error like: ```text WebSocket connection to ws://127.0.0.1:xxxx/hub failed: Failed to connect ``` The short version: cancellation could make the hub daemon think an expected abort rejection was an unhandled crash, so the daemon exited. After that, the CLI tried to reconnect to the old hub port and got the WebSocket failure. ## What was happening The hub flow looks like this: ```text 1. CLI sends: start this run 2. Hub daemon starts the run and returns a promise for it 3. User hits cancel 4. CLI sends a separate: abort that run 5. Abort makes the run promise reject 6. If Node/Bun thinks nobody is handling that rejection, the daemon's unhandledRejection handler kills the process 7. The next CLI request tries to connect to the old hub port and fails ``` There was already defensive code in `SessionRuntime.abort()` for this: ```ts void this.activeRunPromise.catch(() => {}); ``` That catch is intentionally not application-level error handling. It just tells the process: this rejection can be expected during cancellation, so do not classify it as an unhandled crash. Awaiting callers should still receive the same rejection or result. The subtle bug was promise identity. Before this PR, `run()` and `continue()` were `async` wrappers. That means this shape: ```ts async run() { return this.executeRun(); } ``` Even if `executeRun()` returns promise A, `async run()` returns a different wrapper promise B that mirrors promise A. So the runtime was observing promise A with `activeRunPromise.catch(...)`, but the caller actually held promise B. During fast cancellation timing, promise B could reject before the caller awaited it, and the daemon could still see an unhandled rejection. ## Fix This PR makes the tracked promise and the returned promise be the same promise. `run()` and `continue()` now return directly instead of creating `async` wrapper promises: ```ts run() { return this.executeRun(...); } ``` And `executeRun()` stores the same promise it returns: ```text activeRunPromise === the promise returned to the caller ``` Now when abort attaches the existing catch observer to `activeRunPromise`, it is observing the exact promise that can reject during cancellation. That closes the timing gap without adding retry logic, hiding the abort, or changing what callers receive when they await the run. ## Regression test I added a focused test for the failure shape: 1. Start a run. 2. Wait until the fake runtime is active. 3. Abort the run. 4. Let a tick pass before awaiting the returned promise. 5. Assert no `unhandledRejection` was observed. 6. Assert the returned promise still rejects with the original cancellation error. That test failed before the fix because the public wrapper promise triggered `unhandledRejection`. It passes after this change. ## Why this belongs in core The CLI is where the user sees the broken behavior, but the promise lifecycle lives in the daemon-side core session runtime. Any hub-backed caller that aborts an in-flight run could hit the same timing issue, so the fix belongs in `packages/core` rather than in CLI retry or discovery code. I also avoided changing hub retry or stale discovery handling here. Restarting or rediscovering the hub might mask the symptom, but it would not address the daemon exit caused by cancellation. ## Verification Commands run: ```sh bunx vitest run src/runtime/orchestration/session-runtime-orchestrator.test.ts --config vitest.config.ts bunx vitest run src/hub/client/index.test.ts src/hub/daemon/index.test.ts src/hub/server/browser-websocket.test.ts --config vitest.config.ts bun run typecheck bun run test:unit ``` Additional commit hook verification also ran: ```sh bun run types bun biome check --no-errors-on-unmatched --files-ignore-unknown=true ``` The full core unit suite passed with 87 test files, 773 passing tests, and 3 skipped tests. The focused CLI abort/runtime tests also passed. ## Remaining risk I did not perform a live provider-backed manual cancellation test. The regression now covers the promise timing failure that can kill the hub daemon, but a live run would still be useful for confidence around provider stream behavior and user-facing CLI recovery. --- .../session-runtime-orchestrator.test.ts | 53 +++++++++++++++++++ .../session-runtime-orchestrator.ts | 21 ++++---- 2 files changed, 63 insertions(+), 11 deletions(-) diff --git a/sdk/packages/core/src/runtime/orchestration/session-runtime-orchestrator.test.ts b/sdk/packages/core/src/runtime/orchestration/session-runtime-orchestrator.test.ts index 194b4f53d1..314aa3d3c6 100644 --- a/sdk/packages/core/src/runtime/orchestration/session-runtime-orchestrator.test.ts +++ b/sdk/packages/core/src/runtime/orchestration/session-runtime-orchestrator.test.ts @@ -779,6 +779,59 @@ describe("SessionRuntime.abort", () => { expect(abortCalls).toEqual(["user cancelled"]); }); + it("observes an abort rejection before the caller awaits the run", async () => { + let rejectRun: ((error: Error) => void) | undefined; + const runGate = new Promise((_resolve, reject) => { + rejectRun = reject; + }); + let markRunStarted: (() => void) | undefined; + const runStarted = new Promise((resolve) => { + markRunStarted = resolve; + }); + const abortCalls: unknown[] = []; + const runtime = { + async run() { + markRunStarted?.(); + return await runGate; + }, + async continue() { + markRunStarted?.(); + return await runGate; + }, + abort(reason?: unknown) { + abortCalls.push(reason); + rejectRun?.(new Error(String(reason ?? "aborted"))); + }, + subscribe() { + return () => {}; + }, + snapshot() { + return makeSnapshot(); + }, + } as unknown as AgentRuntime; + const unhandledReasons: unknown[] = []; + const onUnhandledRejection = (reason: unknown): void => { + unhandledReasons.push(reason); + }; + + process.prependListener("unhandledRejection", onUnhandledRejection); + try { + const session = new SessionRuntime(makeAgentConfig(), { + createAgentRuntimeImpl: () => runtime, + }); + const runPromise = session.run("slow"); + await runStarted; + session.abort("user cancelled"); + await new Promise((resolve) => setTimeout(resolve, 0)); + + expect(unhandledReasons).toEqual([]); + await expect(runPromise).rejects.toThrow("user cancelled"); + expect(abortCalls).toEqual(["user cancelled"]); + } finally { + process.off("unhandledRejection", onUnhandledRejection); + } + }); + it("is a no-op when no run is active", () => { const { deps } = withFakeRuntime(); const session = new SessionRuntime(makeAgentConfig(), deps); diff --git a/sdk/packages/core/src/runtime/orchestration/session-runtime-orchestrator.ts b/sdk/packages/core/src/runtime/orchestration/session-runtime-orchestrator.ts index e47b25681d..90b6a186b3 100644 --- a/sdk/packages/core/src/runtime/orchestration/session-runtime-orchestrator.ts +++ b/sdk/packages/core/src/runtime/orchestration/session-runtime-orchestrator.ts @@ -194,7 +194,7 @@ export class SessionRuntime { private abortReason: string | undefined; /** Reference to the current run's `AgentRuntime` so `abort` can forward. */ private activeRuntime: AgentRuntime | null = null; - /** Promise for the current run so shutdown can await an aborted run's drain. */ + /** Promise returned from the current run so shutdown can await its drain. */ private activeRunPromise: Promise | null = null; /** Per-run `Agent → AgentEvent` adapter; `reset()` each run. */ private readonly eventAdapter = new RuntimeEventAdapter(); @@ -541,7 +541,7 @@ export class SessionRuntime { // Run / continue // ------------------------------------------------------------------- - async run( + run( userMessage: string, userImages?: string[], userFiles?: string[], @@ -556,7 +556,7 @@ export class SessionRuntime { }); } - async continue( + continue( userMessage?: string, userImages?: string[], userFiles?: string[], @@ -573,21 +573,20 @@ export class SessionRuntime { // Private implementation // ------------------------------------------------------------------- - private async executeRun(input: { + private executeRun(input: { userMessage?: string; userImages?: string[]; userFiles?: string[]; isContinue: boolean; }): Promise { - const runPromise = this.executeRunInternal(input); - this.activeRunPromise = runPromise; - try { - return await runPromise; - } finally { - if (this.activeRunPromise === runPromise) { + let activePromise!: Promise; + activePromise = this.executeRunInternal(input).finally(() => { + if (this.activeRunPromise === activePromise) { this.activeRunPromise = null; } - } + }); + this.activeRunPromise = activePromise; + return activePromise; } private async executeRunInternal(input: {