fix(mcp-stdio): surfaced notify() write failures so handshake fails loudly
Per review on #1711: when the write inside notify() fails (FileSink EPIPE on Windows during the initialize/notifications-initialized race), silently closing the transport while still resolving the notify promise let initializeConnection() return a 'connected' handle wrapping a dead transport. The manager only wires its reconnect onClose handler after connectToServer() resolves, so a swallowed handshake failure would neither reconnect nor fail the connection — it would just leak. notify() now still calls #handleClose() on write failure (so any wired onClose runs) but additionally throws `Transport closed while sending notification "<method>"`. The single in-tree notify caller is initializeConnection() at client.ts:122; the rejection propagates into connectToServer()'s catch (which closes the transport and rethrows) and on into the manager's pending-connection error path. #sendResponse() is unchanged — silent on failure, since a dead subprocess has no use for the response. Test coverage updated to document the surfaced-rejection contract and also assert transport.connected flips to false.
This commit is contained in:
@@ -4,7 +4,7 @@
|
||||
|
||||
### Fixed
|
||||
|
||||
- Fixed an unhandled `EPIPE` rejection when an MCP stdio server exits between returning the `initialize` response and the client's `notifications/initialized` send. `StdioTransport.notify()` and `#sendResponse()` route stdin writes through a shared helper that swallows synchronous sink failures; `notify()` additionally tears the transport down so the reconnect machinery engages instead of leaking a rejection ([#1710](https://github.com/can1357/oh-my-pi/issues/1710)).
|
||||
- Fixed an unhandled `EPIPE` rejection when an MCP stdio server exits between returning the `initialize` response and the client's `notifications/initialized` send. `StdioTransport.notify()` and `#sendResponse()` now route stdin writes through a shared helper that catches synchronous sink failures: `notify()` tears the transport down (firing `onClose`) and surfaces a `Transport closed while sending notification` rejection so `connectToServer()` treats the handshake as a failed connection instead of returning a "connected" handle wrapping a dead transport; `#sendResponse()` stays silent because a dead subprocess has no use for the response ([#1710](https://github.com/can1357/oh-my-pi/issues/1710)).
|
||||
|
||||
## [15.8.0] - 2026-06-02
|
||||
|
||||
|
||||
@@ -314,11 +314,16 @@ export class StdioTransport implements MCPTransport {
|
||||
// Bun's FileSink can throw EPIPE synchronously on Windows when the
|
||||
// subprocess has exited between the last read-loop tick and this
|
||||
// write (e.g. an MCP server that dies after returning `initialize`
|
||||
// but before `notifications/initialized` is delivered). Treat any
|
||||
// such failure as transport closure so the reconnect machinery
|
||||
// engages instead of leaking an unhandled rejection — see #1710.
|
||||
// but before `notifications/initialized` is delivered). Tear the
|
||||
// transport down so any wired `onClose` (and reconnect machinery)
|
||||
// engages, then surface the failure to the caller so a write that
|
||||
// dropped on the floor is never silently treated as delivered —
|
||||
// `initializeConnection()` runs before the manager installs its
|
||||
// `onClose` handler, so a swallowed failure there would yield a
|
||||
// "connected" handle wrapping a dead transport. See #1710.
|
||||
if (!writeFrame(this.#process.stdin, `${JSON.stringify(notification)}\n`)) {
|
||||
this.#handleClose();
|
||||
throw new Error(`Transport closed while sending notification "${method}"`);
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -2,9 +2,9 @@ import { afterEach, describe, expect, it } from "bun:test";
|
||||
import { StdioTransport, writeFrame } from "../src/mcp/transports/stdio";
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// writeFrame — the seam that swallows synchronous FileSink failures so the
|
||||
// async `notify` / `#sendResponse` paths can never leak unhandled rejections
|
||||
// when an MCP subprocess exits between read-loop ticks. See issue #1710.
|
||||
// writeFrame — the seam that catches synchronous FileSink failures so the
|
||||
// async `notify` / `#sendResponse` paths can decide whether to swallow or
|
||||
// surface the error. See issue #1710.
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
describe("writeFrame", () => {
|
||||
@@ -68,12 +68,19 @@ describe("writeFrame", () => {
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// StdioTransport.notify — guards and end-to-end behavior with a real
|
||||
// subprocess that exits between the `initialize` response and the
|
||||
// `notifications/initialized` send. The harness can't directly reproduce the
|
||||
// Windows EPIPE on Linux (Bun's FileSink absorbs it), but the contract we
|
||||
// defend is platform-independent: no unhandled rejection ever escapes
|
||||
// notify(), even when the read loop hasn't yet flipped #connected.
|
||||
// StdioTransport.notify — end-to-end behavior against a real subprocess that
|
||||
// exits between the `initialize` response and the `notifications/initialized`
|
||||
// send. Contract defended here:
|
||||
//
|
||||
// 1. notify() always settles — no unhandled rejection ever escapes when
|
||||
// the underlying FileSink throws synchronously.
|
||||
// 2. A failed write tears the transport down (`onClose` fires) AND surfaces
|
||||
// a rejection to the caller so `initializeConnection()` doesn't return a
|
||||
// "connected" handle wrapping a dead transport.
|
||||
//
|
||||
// On Linux, Bun's FileSink absorbs the EPIPE so the only failure surfaced is
|
||||
// the "Transport not connected" guard on subsequent calls; on Windows the
|
||||
// write actually throws. Either way the tracker must stay empty.
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
function trackUnhandled(): { release: () => unknown[]; capture: () => unknown[] } {
|
||||
@@ -152,24 +159,22 @@ describe("StdioTransport.notify", () => {
|
||||
try {
|
||||
await transport.connect();
|
||||
await transport.request("initialize", {});
|
||||
|
||||
// Fire several notifies — covers both the "subprocess just exited"
|
||||
// race and the "already torn down" guard path. None may yield an
|
||||
// unhandled rejection.
|
||||
// race (write may fail) and the "already torn down" guard path
|
||||
// (subsequent calls reject with `Transport not connected`). Every
|
||||
// rejection is handled here; the contract under test is that none
|
||||
// of them leak as an unhandled rejection.
|
||||
for (let i = 0; i < 5; i++) {
|
||||
await transport.notify("notifications/initialized").catch(err => {
|
||||
// Re-throwing "Transport not connected" is fine (handled).
|
||||
if (!(err instanceof Error) || err.message !== "Transport not connected") {
|
||||
throw err;
|
||||
}
|
||||
});
|
||||
await transport.notify("notifications/initialized").catch(() => {});
|
||||
}
|
||||
|
||||
// Let any deferred microtasks settle.
|
||||
// Let any deferred microtasks settle so an escaped rejection has
|
||||
// a chance to fire `unhandledRejection` before we assert.
|
||||
await Bun.sleep(50);
|
||||
|
||||
expect(tracker.capture()).toEqual([]);
|
||||
expect(closed).toBe(true);
|
||||
expect(transport.connected).toBe(false);
|
||||
} finally {
|
||||
tracker.release();
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user