Stop sendAndWait from emitting an unhandled rejection - #2206
Conversation
`sendAndWait` creates `idlePromise` and registers the event listener that
can reject it, but the first consumer is only attached by the
`Promise.race` further down -- after `await this.send(options)`, a full
JSON-RPC round trip to the CLI.
A `session.error` arriving in that window therefore rejects a promise
that has no handler yet. Node's rejection tracker runs at the following
checkpoint, well before the `session.send` response lands, and classifies
it as unhandled, which terminates the process under the default
`--unhandled-rejections=throw`.
The window is reachable from ordinary, non-fatal traffic. `session.log`
with `{ level: "error" }` emits a `session.error` carrying
`errorType: "notification"` (asserted in test/e2e/session.e2e.test.ts),
so a joined client or extension writing an error log line while another
caller is mid-`sendAndWait` is enough. MCP servers failing to start and
sub-agent errors do the same. A caller cannot defend against this: the
rejection is on the internal promise, not on the one `sendAndWait`
returns, so even correct `.catch`/`try` handling around the call does not
prevent the crash.
Attaching a no-op `catch` marks the promise handled without consuming the
rejection, so the `Promise.race` still observes it and `sendAndWait`
rejects with the original error exactly as before.
The added test drives a session whose `session.send` RPC is held open,
dispatches a `session.error` into the window, and asserts both that no
`unhandledRejection` fires and that `sendAndWait` still rejects. It fails
on the unfixed code with the error captured by the process-level listener.
There was a problem hiding this comment.
Pull request overview
Prevents sendAndWait from triggering an unhandled rejection when session.error arrives during an in-flight send.
Changes:
- Marks the internal idle promise as handled immediately.
- Adds a regression test preserving the original rejection behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
nodejs/src/session.ts |
Prevents transient unhandled rejections. |
nodejs/test/session-send-and-wait.test.ts |
Tests the mid-send error race. |
|
Correction to the verification section above: the "6 failed / 371 passed → 5 failed / 372 passed" numbers were from an incomplete local setup — I had not run With both done, the full Node.js suite passes on this branch: The rest of the gate from
So there are no outstanding failures — the earlier numbers reflected my environment, not this change. Apologies for the confusion. Observed on darwin / Node v26.5.0, against |
Fixes #2205
What's wrong
sendAndWaitcreatesidlePromiseand registers the event listener that can reject it, but the first consumer is only attached by thePromise.racefurther down — afterawait this.send(options), a full JSON-RPC round trip to the CLI.A
session.errorarriving in that window therefore rejects a promise with no handler. Node's rejection tracker runs at the following checkpoint, well before thesession.sendresponse lands, and classifies it as unhandled — which terminates the process under the default--unhandled-rejections=throw.The existing comment above the listener shows the inverse race (subscribing too late) was considered; this is the opposite ordering.
Why it's reachable in normal use
session.erroris not reserved for turn-fatal failures.session.log(msg, { level: "error" })emits one witherrorType: "notification"— asserted intest/e2e/session.e2e.test.ts. So a joined client or extension writing an error log line while another caller is mid-sendAndWaitis enough; MCP servers failing to start and sub-agent errors do the same.A caller cannot defend against this: the rejection is on the internal promise, not on the one
sendAndWaitreturns, so even correcttry/.catchhandling around the call does not prevent the crash.The fix
A no-op
catchmarks the promise handled without consuming the rejection, so thePromise.racestill observes it andsendAndWaitrejects with the original error exactly as before. No behaviour change on any other path.Verification
Added
test/session-send-and-wait.test.ts, which holds thesession.sendRPC open, dispatches asession.errorinto the window, and asserts both that nounhandledRejectionfires and thatsendAndWaitstill rejects with the original message.It fails on the unfixed code:
and passes with the change. Full local run:
npx vitest run test/ --exclude "test/e2e/**" --exclude "test/cjs-compat.test.ts"→ 14 files, 359 passednpx tsc --noEmit→ cleannpm run lint→ 0 errors (3 pre-existing warnings, all intest/shared-codegen.test.ts, none in the files touched here)On the full
npm testmy environment goes from 6 failed / 371 passed on unmodifiedmainto 5 failed / 372 passed with this change — the delta is exactly the new test. The remaining failures aretest/cjs-compat.test.ts(needs a builtdist/, which I didn't produce) and thetest/e2e/**suites (need the realcopilotCLI); I verified both fail identically on unmodifiedmain.Scope
One line of behaviour change plus a test. Note the other SDKs are already structurally immune — Go (
go/session.go:502-505) uses a bufferederrChand Python (python/copilot/session.py:1729-1731) sets a variable plusidle_event.set()— so this brings Node in line rather than introducing a new pattern.