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.
This commit is contained in:
Saoud Rizwan
2026-04-29 17:16:58 -07:00
committed by GitHub
parent d6bdf39f80
commit 319d988b72
2 changed files with 63 additions and 11 deletions
@@ -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<AgentRunResult>((_resolve, reject) => {
rejectRun = reject;
});
let markRunStarted: (() => void) | undefined;
const runStarted = new Promise<void>((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);
@@ -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<AgentResult> | 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<AgentResult> {
const runPromise = this.executeRunInternal(input);
this.activeRunPromise = runPromise;
try {
return await runPromise;
} finally {
if (this.activeRunPromise === runPromise) {
let activePromise!: Promise<AgentResult>;
activePromise = this.executeRunInternal(input).finally(() => {
if (this.activeRunPromise === activePromise) {
this.activeRunPromise = null;
}
}
});
this.activeRunPromise = activePromise;
return activePromise;
}
private async executeRunInternal(input: {