Uh oh!
There was an error while loading. Please reload this page.
fix: async onclose, stdin EOF detection, SIGTERM in examples - #1814
fix: async onclose, stdin EOF detection, SIGTERM in examples#1814MayCXC wants to merge 2 commits into
Conversation
🦋 Changeset detectedLatest commit: 59d67bf The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
8cff2e5 to
5559b99Compare@modelcontextprotocol/client@modelcontextprotocol/server@modelcontextprotocol/express@modelcontextprotocol/fastify@modelcontextprotocol/hono@modelcontextprotocol/nodecommit: |
5559b99 to
3d3234bCompareThree related improvements to server lifecycle handling: 1. Allow async onclose callbacks on Transport and Protocol. MCP servers that hold external resources (browser sessions, database connections) need to await cleanup before the process exits. The onclose signature changes from `() => void` to `() => void | Promise<void>`, matching the existing pattern used by onsessionclosed in StreamableHTTPServerTransport. All transports and Protocol._onclose now await the callback. 2. Close StdioServerTransport when stdin ends. The transport listened for data and error but not EOF. When the MCP client disconnects, the transport stays open and onclose never fires. This is especially visible with containerized servers using docker run with automatic removal: without onclose the server never exits and the container accumulates. 3. Add SIGTERM handlers alongside SIGINT in all examples. MCP servers run as background processes spawned by clients, not interactively. SIGTERM is what container runtimes and process managers send to stop a process.
3d3234b to
3f70c00Compare
felixweinberger
left a comment
There was a problem hiding this comment.
What prompted you to open this PR, are any of these issues things you're running into?
MayCXC
commented
Mar 30, 2026
yes I ran into them all, and that is what prompted me to open the PR. it upstreams fixes that I have added to individual MCPs separately, for example https://github.com/mozilla/firefox-devtools-mcp/pull/50/changes#diff-a2a171449d862fe29692ce031981047d7ab755ae7f84c707aef80701b3ea0c80R365 |
km-anthropic
commented
Mar 31, 2026
@claude review |
felixweinberger
commented
Apr 9, 2026
@claude review |
| private async _onclose(): Promise<void> { | ||
| const responseHandlers = this._responseHandlers; | ||
| this._responseHandlers = new Map(); | ||
| this._progressHandlers.clear(); | ||
| this._taskManager.onClose(); | ||
| this._pendingDebouncedNotifications.clear(); | ||
| this._transport = undefined; | ||
| await this.onclose?.(); | ||
| for (const info of this._timeoutInfo.values()) { | ||
| clearTimeout(info.timeoutId); |
There was a problem hiding this comment.
🔴 The refactored _onclose() in protocol.ts removed the try/finally block that previously guaranteed cleanup of in-flight requests even if the onclose callback threw. Since this PR introduces async onclose callbacks as its primary feature, any rejection from await this.onclose?.() will now skip notifying pending response handlers with ConnectionClosed, clearing timeout info, and aborting request handler AbortControllers — causing in-flight requests to hang until their individual timeouts expire instead of failing immediately. Fix: wrap the await this.onclose?.() call in a try { ... } finally { /* cleanup */ } block.
Extended reasoning...
The Bug
The _onclose() method was refactored from synchronous to async to support the new async onclose callback. In the original code, the onclose callback was called inside a try { } finally { } block that unconditionally ran cleanup: notifying response handlers with a ConnectionClosed error, clearing timeout handles, and aborting AbortControllers for in-flight server-side request handlers. The new code calls await this.onclose?.() with no try/finally wrapping the subsequent cleanup loops.
The Code Path
In packages/core/src/shared/protocol.ts, _onclose() (starting around line 490 in the modified file):
responseHandlersis captured fromthis._responseHandlers(line ~492) and the map is cleared.await this.onclose?.()is called (line ~501) — notry/finally.- The cleanup loops —
clearTimeoutfor all_timeoutInfoentries,handler(error)for each capturedresponseHandler, andcontroller.abort(error)for each capturedAbortController— only execute if theawaitresolves successfully.
Why Existing Code Doesn't Prevent It
The outer connect() wrapper does use try/finally to ensure this._onclose() is called even if the old transport's onclose throws. But that only ensures _onclose() runs — it does not protect the code inside_onclose() from a rejection thrown by the user's onclose callback. Once await this.onclose?.() rejects, the async function propagates the rejection and the remaining cleanup statements are skipped.
Impact
If a user supplies an async onclose callback (which is now the advertised feature of this PR) that rejects:
- Pending in-flight requests whose handlers are stored in the captured
responseHandlersmap will never receive theConnectionClosederror — they will hang until their individual timeouts expire (default 60 seconds). clearTimeoutcalls will be skipped, leakingsetTimeouthandles.- Server-side
AbortControllers for active request handlers will not be aborted, so long-running server-side tool handlers will not be cancelled promptly.
Step-by-Step Proof
- Client connects and sends a request;
protocol._responseHandlersnow contains one entry with the pending handler. - User sets
protocol.onclose = async () => { await doAsyncCleanup(); /* throws */ }. - Connection drops; transport fires its
onclosecallback. connect()'s wrapper callsawait this._onclose().- Inside
_onclose:responseHandlersis captured (step 1 entry is snapshotted), thenawait this.onclose?.()rejects. _onclose()propagates the rejection — thefor (const handler of responseHandlers.values()) { handler(error); }loop never executes.- The pending request from step 1 sits unresolved until its 60-second timeout fires, rather than immediately receiving
ConnectionClosed.
Fix
Wrap the await this.onclose?.() call in a try/finally inside _onclose():
try{awaitthis.onclose?.();}finally{for(constinfoofthis._timeoutInfo.values()){clearTimeout(info.timeoutId);}this._timeoutInfo.clear();constrequestHandlerAbortControllers=this._requestHandlerAbortControllers;this._requestHandlerAbortControllers=newMap();consterror=newSdkError(SdkErrorCode.ConnectionClosed,'Connection closed');for(consthandlerofresponseHandlers.values()){handler(error);}for(constcontrollerofrequestHandlerAbortControllers.values()){controller.abort(error);}}There was a problem hiding this comment.
Fixed in 59d67bf. Wrapped await this.onclose?.() in try/finally so the clearTimeout loop, response-handler ConnectionClosed notification, and AbortController.abort calls all run even if the user's async onclose rejects. The thrown rejection still propagates up to connect()'s wrapper.
| this._stdin.on('end', () => { | ||
| this.close(); | ||
| }); |
There was a problem hiding this comment.
🔴 Two async event handlers introduced in this PR lack error handling, creating unhandled Promise rejection risks. In StdioServerTransport.start(), the new stdin end handler calls this.close() without .catch(); in StdioClientTransport, the close event handler was made async but Node.js EventEmitter does not await async listeners, so a rejecting onclose callback escapes silently in both cases. Fix both by using .catch(err => this.onerror?.(err)) instead of await inside the EventEmitter callbacks.
Extended reasoning...
Bug 1 — StdioServerTransport stdin 'end' handler (packages/server/src/server/stdio.ts, lines 62–64):
The PR adds EOF detection to close the server when stdin ends, which is one of its primary features. However, the handler calls this.close() without attaching a .catch():
this._stdin.on('end',()=>{this.close();// Promise return value discarded});Since close() is async and now awaits this.onclose?.() (whose type was widened to () => void | Promise<void>), any rejection from an async onclose callback propagates out of close() and becomes an unhandled Promise rejection. In Node.js v15+, unhandled rejections terminate the process with a non-zero exit code.
The correct pattern is demonstrated in the very same file by _onstdouterror:
this.close().catch(()=>{// Ignore errors during close — we're already in an error path});For the end handler, errors should be routed through onerror rather than silently swallowed:
this._stdin.on('end',()=>{this.close().catch(error=>this.onerror?.(error));});Bug 2 — StdioClientTransport process 'close' handler (packages/client/src/client/stdio.ts, ~line 147):
The handler was changed to async:
this._process.on('close',async_code=>{this._process=undefined;awaitthis.onclose?.();});Node.js EventEmitter never awaits the return value of event listeners. When the async listener function rejects (because this.onclose?.() rejects), the returned Promise is silently dropped by the EventEmitter. In Node.js v15+ this surfaces as an unhandledRejection event, which by default terminates the process.
The correct approach avoids async/await inside the listener and instead chains .catch():
this._process.on('close',_code=>{this._process=undefined;this.onclose?.()?.catch(error=>this.onerror?.(error));});Why this matters specifically for this PR:
The central motivation of the PR is enabling async onclose callbacks for MCP servers that hold external resources (browser sessions, database connections). These are exactly the callbacks most likely to perform fallible async operations (network calls, I/O cleanup). Both bugs mean that the very feature being added — async onclose support — silently fails to propagate errors in the two most common stdio transport paths.
Step-by-step proof for Bug 1:
- Server starts with
StdioServerTransport. - User sets
server.onclose = async () => { await db.close(); }wheredb.close()can throw. - MCP client disconnects; stdin receives EOF.
- The
'endhandler fires and callsthis.close()(no.catch()). close()callsawait this.onclose?.(); the async callback throws.close()rejects; the rejection propagates out of the'endhandler with no catch.- Node emits
unhandledRejection; process exits in v15+.
Step-by-step proof for Bug 2:
- Client spawns a stdio server process.
- User sets
transport.onclose = async () => { await session.destroy(); }wheresession.destroy()rejects. - The spawned process exits; the
'closeevent fires. - The
asynclistener runs and callsawait this.onclose?.(); the callback throws. - The
asynclistener's returned Promise rejects. - EventEmitter discards it; unhandled rejection in v15+.
There was a problem hiding this comment.
Fixed in 59d67bf, both handlers using .catch() to route async rejections to onerror, matching the existing _onstdouterror pattern in the same server file.
Server stdio 'end' handler:
this._stdin.on('end',()=>{this.close().catch(error=>this.onerror?.(errorasError));});Client stdio 'close' handler (changed back from async listener):
this._process.on('close',_code=>{this._process=undefined;this.onclose?.()?.catch(error=>this.onerror?.(errorasError));});Sync throws from a user-supplied onclose will surface as the existing convention here — async functions wrap throws in their returned Promise (caught by .catch), and a sync () => void that throws is treated as a programming error worth crashing on, consistent with how the rest of this file handles user callbacks.
| process.on('SIGTERM', async () => { | ||
| console.log('\nReceived SIGINT. Cleaning up...'); | ||
| await cleanup(); | ||
| }); |
There was a problem hiding this comment.
🟡 The SIGTERM handler in elicitationUrlExample.ts (line 817) and simpleStreamableHttp.ts logs '\nReceived SIGINT. Cleaning up...' instead of '\nReceived SIGTERM. Cleaning up...'. This copy-paste error means operators monitoring logs will see misleading signal names when the process is stopped via SIGTERM (e.g., by container runtimes or process managers).
Extended reasoning...
What the bug is: In both examples/client/src/elicitationUrlExample.ts and examples/client/src/simpleStreamableHttp.ts, the newly-added SIGTERM handler was copy-pasted from the SIGINT handler but the log string was not updated. It still reads 'Received SIGINT. Cleaning up...' inside process.on('SIGTERM', ...).
There was a problem hiding this comment.
Fixed in 59d67bf — both examples/client/elicitationUrlExample.ts and examples/client/simpleStreamableHttp.ts SIGTERM handlers now log '\nReceived SIGTERM. Cleaning up...'.
Server examples already use a generic 'Shutting down server...' log, so no copy-paste leak there.
Resolves three review comments from modelcontextprotocol#1814: 1. protocol.ts: wrap `await this.onclose?.()` in try/finally so in-flight request cleanup (clearTimeout, ConnectionClosed handler notification, AbortController.abort) runs even when an async onclose callback rejects. 2. stdio EventEmitter handlers: avoid bare `await onclose()` and bare `this.close()` inside listener callbacks where rejections would surface as unhandledRejection (process exit in Node 15+). Use `.catch()` to route errors to onerror, matching the existing `_onstdouterror` pattern in the same file. 3. examples: fix copy-paste in elicitationUrlExample.ts and simpleStreamableHttp.ts SIGTERM handlers, which logged "Received SIGINT" instead of "Received SIGTERM". Merge conflicts: - transport.ts onclose: combine our async return type with main's explicit `| undefined` (exactOptionalPropertyTypes). - streamableHttp.ts close(): keep main's restructured try/finally scope; apply our `await` to onclose call. - websocket.ts: deleted in main (transport removed), our changes dropped along with the file.
MayCXC
commented
Apr 20, 2026
@felixweinberger requested changes are addressed |
Summary
Three related improvements to server lifecycle handling.
1. Allow async
onclosecallbacksMCP servers that hold external resources (browser sessions, database connections) need to await cleanup before the process exits.
oncloseis the only transport/protocol callback called from an awaitable context (transport.close()is async, awaited byserver.close()). The other callbacks (onmessage,onerror) fire from event emitters that cannot await.The
onclosesignature changes from() => voidto() => void | Promise<void>, matching the existing pattern used byonsessionclosedinStreamableHTTPServerTransport. All transports andProtocol._onclosenow await the callback.Changed files:
Transportinterface,Protocol,StdioServerTransport,StreamableHTTPServerTransport,StdioClientTransport,WebSocketClientTransport,StreamableHTTPClientTransport,SSEClientTransport,InMemoryTransport, and mock transports in tests.2. Close
StdioServerTransportwhen stdin endsThe transport listened for
dataanderroron stdin but not EOF. When the MCP client disconnects (closing stdin), the transport stays open andonclosenever fires. This prevents servers from cleaning up resources.This is especially visible with containerized MCP servers using
docker run --rm: withoutonclose, the server process never exits, the container never stops, and containers accumulate on each client reconnect.3. Add SIGTERM handlers in examples
All 10 examples only handle SIGINT (Ctrl+C). MCP servers run as background processes spawned by clients, not interactively. SIGTERM is what container runtimes and process managers send to stop a process. Added SIGTERM handlers alongside SIGINT in all examples.
Test plan
should close when stdin ends(push null to stdin, verifyonclosefires)should await async onclose callback(async cleanup completes beforeclose()resolves)