From b8707322580e99298dfde91cc577c3cd90f80f31 Mon Sep 17 00:00:00 2001 From: roboomp Date: Tue, 2 Jun 2026 12:28:07 +0000 Subject: [PATCH] fix(mcp-stdio): surfaced notify() write failures so handshake fails loudly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 ""`. 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. --- packages/coding-agent/CHANGELOG.md | 2 +- .../coding-agent/src/mcp/transports/stdio.ts | 11 +++-- .../test/mcp-stdio-transport.test.ts | 43 +++++++++++-------- 3 files changed, 33 insertions(+), 23 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 321a0a310..0481eb6d2 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -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 diff --git a/packages/coding-agent/src/mcp/transports/stdio.ts b/packages/coding-agent/src/mcp/transports/stdio.ts index 1d8859001..dc7358168 100644 --- a/packages/coding-agent/src/mcp/transports/stdio.ts +++ b/packages/coding-agent/src/mcp/transports/stdio.ts @@ -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}"`); } } diff --git a/packages/coding-agent/test/mcp-stdio-transport.test.ts b/packages/coding-agent/test/mcp-stdio-transport.test.ts index deea5514f..1e72b35d7 100644 --- a/packages/coding-agent/test/mcp-stdio-transport.test.ts +++ b/packages/coding-agent/test/mcp-stdio-transport.test.ts @@ -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(); }