fix(mcp): notify onclose when standalone SSE stream aborts - #2075
Conversation
MCP clients often drop the GET SSE stream without DELETE. Call onclose on standalone stream abort so apps can detect disconnects, without double-firing when close() also runs. Fixes honojs#1762
🦋 Changeset detectedLatest commit: 67ebe5c The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2075 +/- ##
==========================================
+ Coverage 92.23% 92.25% +0.02%
==========================================
Files 116 116
Lines 4122 4134 +12
Branches 1077 1082 +5
==========================================
+ Hits 3802 3814 +12
Misses 285 285
Partials 35 35
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Await stopTestServer and prefer ?? for session ids so @hono/mcp lint passes in CI.
|
Hello @MathurAditya724 ! Can you review this? |
| this.onclose?.() | ||
| // Standalone SSE abort may already have notified; still notify when there | ||
| // was no GET stream (e.g. DELETE after JSON-only traffic). | ||
| this.#notifyClose() |
There was a problem hiding this comment.
Thanks for working on this — exposing a dropped standalone SSE connection would be useful.
I’m not sure onclose is the right hook here, though. In the MCP Streamable HTTP model, a disconnected SSE stream can be resumed, and the session itself remains valid until the client sends DELETE or the server terminates it. The official SDK keeps that distinction: an aborted GET stream only cleans up the stream mapping, while onclose is reserved for transport.close().
With this change, a temporary disconnect fires onclose, but a later reconnect is still allowed. The #closedNotified guard then suppresses the callback when the session is actually closed.
Could we add a separate callback for the standalone SSE stream disconnect instead, and keep onclose for terminal transport shutdown? It would also be great to add an abort → reconnect → DELETE test, especially for the resumable/event-store path.
There was a problem hiding this comment.
Implemented in fd6b5fd. The transport now exposes onsessiondisconnected(sessionId) for standalone SSE aborts while keeping onclose reserved for terminal close() / DELETE shutdown. Added coverage for abort → callback → resumable reconnect; with the source fix stashed, the test fails (0 callback calls), and with it restored the focused test passes. Package typecheck and all @hono/mcp tests pass (196/196).
Expose onsessiondisconnected for standalone SSE aborts while keeping onclose reserved for terminal transport shutdown. Add abort and resumability coverage.
Remove new non-null assertions from the resumable SSE callback path and cover errors raised by the session disconnect hook.
|
CI follow-up pushed as
The earlier red |
|
@edenbuilds @MathurAditya724 Thanks! |
4 participants
Summary
onclosewhen the standalone GET SSE stream (_GET_stream) aborts, so apps can detect client disconnects without relying on DELETE.close()after an abort does not fireonclosetwice.Fixes #1762
Test plan
oncloseonce; subsequentclose()does not double-fireonsessionclosedDELETE tests still passMade with Cursor