feat(core): adopt spec#2907 error-code renumber; conformance pin to pkg.pr.new alpha.5; streamableHttp store-first fix#2335
Conversation
🦋 Changeset detectedLatest commit: b0afb16 The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 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 |
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
f61a634 to
308c912
Compare
d81cc51 to
5725817
Compare
308c912 to
fc38188
Compare
5725817 to
9fcdcc2
Compare
9fcdcc2 to
246c77f
Compare
fc38188 to
11a9ca1
Compare
246c77f to
601893a
Compare
11a9ca1 to
6e8bf73
Compare
601893a to
0f18a51
Compare
dd652fd to
56eeb2c
Compare
0f18a51 to
3c4a829
Compare
56eeb2c to
d5453fd
Compare
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
packages/server/src/server/streamableHttp.ts:1017-1040— After a request completes while its per-request SSE stream is closed (the new poll-and-replay success path), the client'sGET+Last-Event-IDreconnect replays the stored final response butreplayEvents()then registers a resumed live stream under the same streamId that nothing ever closes — no request maps to it anymore, the resumed stream's cancel callback is a no-op, and there is no after-replay close — so each completed-while-disconnected poll cycle leaks an open controller plus a permanent_streamMappingentry untiltransport.close(), and the server never terminates the SSE response carrying the replayed final response. Fix by closing (or not registering) the resumed stream after replay when no in-flight request still maps to the resolved streamId.Extended reasoning...
The bug. This PR's store-first change makes "response delivered via replay" the standard success path of the SEP-1699 poll-and-replay flow, but the replay side never terminates that path.
closeSSEStream()(packages/server/src/server/streamableHttp.ts:939-962) closes the controller and installs a controller-less placeholder under the per-request streamIdS. When the handler finishes before the client reconnects,send()for the final response stores it to the eventStore (the placeholder satisfies the newstream !== undefinedgate at line 1031), then the response branch finds the placeholder, callsstream.cleanup()at line 1069 (deleting_streamMapping[S]), and the cleanup loop at lines 1072-1075 deletes_requestResponseMap/_requestToStreamMappingfor the request. When the client then reconnects withGET+Last-Event-ID,replayEvents()(lines 502-582) passes the conflict check (the placeholder was just removed), replays the stored notification(s) and the final response, and unconditionally registers a resumed live stream underSat lines 564-575 before returning the 200 SSE Response.Why nothing ever closes it. In the pre-existing resume-before-response flow, a later
transport.send(response)targetsS, and its response branch runsstream.cleanup(), closing the controller and removing the entry. In the resume-after-response flow there will never be anothersend()forS:_requestToStreamMappingno longer maps any request to it (deleted in the step above, and new requests get fresh UUID streamIds).replayEvents()has no after-replay close, and the resumed stream'sReadableStreamcancel callback is deliberately empty ("Cleanup will be handled by the mapping", lines 544-547), so even a client-side disconnect leaves the entry. Onlytransport.close()clears it.Impact. Two consequences per completed-while-disconnected poll cycle on a long-lived sessionful transport: (a) the server holds an open
ReadableStreamcontroller plus a permanent_streamMappingentry — an unbounded accumulation for a workload that polls repeatedly; (b) the SSE response that delivered the replayed final JSON-RPC response is never terminated by the server, contrary to the Streamable HTTP guidance that the server SHOULD close the per-request SSE stream once all responses for the request have been sent — a spec-following client (or an intermediary doing idle-connection accounting) is left holding a hanging GET that never ends.Step-by-step proof. (1) Client POSTs a
tools/call(id 5) on a sessionful transport with an eventStore;_requestToStreamMapping[5] = S,_streamMapping[S]is live. (2) The handler callsctx.http.closeSSE()→closeSSEStream(5)closes the controller and installs the placeholder underS. (3) The handler finishes before the client's retry interval elapses;send(response)stores the response (line 1031-1032), the response branch runsstream.cleanup()(line 1069) deleting_streamMapping[S], and the cleanup loop deletes_requestToStreamMapping[5]. (4) The client reconnects withGET+Last-Event-ID;replayEvents()finds no conflict, replays the stored events including the final response, and registers a resumed stream underS(line 564). (5) From this point on, no code path can reach that entry:send()can never resolveSagain, the cancel callback is a no-op, and the SSE response stays open. The entry and its controller persist untiltransport.close()(lines 924-931).Why this is in the PR's scope.
replayEvents()itself is unchanged, but pre-PR this state was effectively unreachable as a designed flow: events and responses emitted while the per-request stream was disconnected were never stored, so aLast-Event-IDreconnect could never be the delivery vehicle for an already-settled response — any resumed per-request stream still had its live response coming throughsend(), which closed it. The store-first change plus the placeholder cleanup-on-response is exactly what creates the "response already delivered, nothing left to close the resumed stream" state, and the changeset advertises this as the new behavior, so the missing close is the natural completion of this PR's fix. It is also distinct from the previously-flagged issues: the 409-conflict comment covers reconnects attempted while the placeholder is still installed (mid-flight), and the cancelled-request placeholder leak covers the case where no response is ever sent — this finding is the success path after the response has been delivered via replay.How to fix. After
replayEventsAfter()completes inreplayEvents(), check whether any entry in_requestToStreamMappingstill maps to the resolvedreplayedStreamId; if none does (the replay-after-completion case), close the controller and skip (or immediately remove) the_streamMappingregistration instead of leaving the resumed stream open indefinitely. A regression test that performs thecloseSSE()→ complete-while-disconnected →Last-Event-IDGET sequence and asserts the replayed SSE stream is closed and_streamMappingdoes not retain the entry would lock this in — the PR's new vitest case stops at checking the eventStore contents and never issues the reconnect. -
🔴
packages/server/src/server/streamableHttp.ts:951-961— When a client cancels an in-flight request (notifications/cancelled) after the handler has calledcloseSSE(), the placeholder_streamMappingentry installed bycloseSSEStream()is never removed: the Protocol layer suppresses the final response on cancellation, and the response-branchstream.cleanup()is the placeholder's only normal removal path. The entry (plus the_requestToStreamMappingentry) leaks for the lifetime of the sessionful transport, and for eventStores implementinggetStreamIdForEventIdthe leaked placeholder makesreplayEvents()'s conflict check answer 409 for that streamId permanently. Pre-PR,closeSSEStream()deleted the entry, so cancellation left no residue — worth folding into the placeholder rework already requested in the other review comment: also clean up the placeholder (and request bookkeeping) when a request is torn down without a response, or key the intentional-close marker outside_streamMapping.Extended reasoning...
The bug.
closeSSEStream()(packages/server/src/server/streamableHttp.ts:951-961) now installs a controller-less placeholder under the per-requeststreamIdinstead of deleting the_streamMappingentry as the pre-PRstream.cleanup()did. The inline comment says the placeholder "is removed when the final response is sent (or overwritten when the client reconnects via Last-Event-ID)", but a final response is not guaranteed: inpackages/core/src/shared/protocol.ts,_onrequest's completion arms both return early when the request was cancelled (if (abortController.signal.aborted) { /* Request was cancelled */ return; }at lines 1023-1026 and 1053-1056), sotransport.send(response)never runs for a cancelled request and the response-branch cleanup loop — the only normal-operation path that calls the placeholder'scleanup()— never executes.\n\nThe code path. Entirely within the SEP-1699 polling flow this PR targets: (1) a sessionful 2025-era transport with aneventStore; (2) a long-running tool handler callsctx.http.closeSSE()→closeSSEStream(requestId)closes the controller and installs the placeholder; (3) the client cancels the in-flight request by POSTingnotifications/cancelled(the normal 2025-era cancellation path); (4) the Protocol layer aborts the request and suppresses the response, sosend(response)never runs for that request id; (5) nothing else removes the placeholder — the original per-requestReadableStream's cancel callback cannot fire becausecloseSSEalready closed the controller gracefully, andreplayEvents()only overwrites the entry if the cancelling client bothers to resume, which it has no reason to do. Only the transport-levelclose()clears it.\n\nImpact. The placeholder_streamMappingentry (plus the pre-existing_requestToStreamMappingentry) accumulates one per cancelled-after-closeSSErequest for the lifetime of the long-lived sessionful transport. The sharper consequence: for eventStores implementinggetStreamIdForEventId,replayEvents()'s conflict check at line 520 (this._streamMapping.get(streamId) !== undefined→ 409) treats the leaked placeholder as an active connection, so any laterLast-Event-IDresume keyed to events stored under that stream is permanently rejected with 409 instead of replaying the stored events.\n\nWhy this is a regression introduced by this PR. Pre-PR,closeSSEStream()calledstream.cleanup(), which deleted the_streamMappingentry — cancellation aftercloseSSEleft no_streamMappingresidue and no 409 (only the pre-existing_requestToStreamMappingleak). The new vitest cases only cover the closeSSE-then-respond and hard-disconnect paths, both of which end in a response or a deleted entry; nothing cancels a request aftercloseSSE(), so no test would catch this.\n\nWhy it's distinct from the existing placeholder comment (inline comment at line 961). That comment's primary 409 issue is transient — it resolves when the final response is sent and cleans up the placeholder. This bug is the path where the response is never sent, so the leak and the 409-blocked streamId are permanent. Notably, even the fixes that comment proposes (treat a controller-less placeholder as not-active in the conflict check, or guard the install) would still leave the cancelled request's marker leaking unless teardown-without-response cleanup is added — which is exactly this report's point.\n\nStep-by-step proof. (1) Client POSTstools/callid 5 on a sessionful transport with agetStreamIdForEventId-implementing eventStore:_requestToStreamMapping[5] = S,_streamMapping[S]is the live per-request stream. (2) The handler callsctx.http.closeSSE(): the controller is closed and_streamMapping[S]becomes the placeholder. (3) The client decides to give up and POSTsnotifications/cancelledfor id 5;_oncancelaborts the per-request AbortController. (4) The handler eventually settles; both completion arms in_onrequestseeabortController.signal.abortedand return without callingtransport.send. (5) The placeholder'scleanup()is never invoked;_streamMapping[S]and_requestToStreamMapping[5]persist until the session is closed. (6) Any later GET with aLast-Event-IDresolving to streamShits the line-520 conflict check and gets 409 forever.\n\nFix. Fold this into the placeholder rework the existing comment already requests: remove the placeholder and the per-request bookkeeping when a request is torn down without a response (e.g. on processingnotifications/cancelledfor a request whose stream entry is a controller-less placeholder), or key the intentional-close marker somewhere other than_streamMappingso neither the conflict check nor the lifetime of the map is affected. A regression test that cancels a request aftercloseSSE()and asserts the mappings are cleared (and a subsequentLast-Event-IDresume succeeds) would lock the behavior in.
3c4a829 to
deebc80
Compare
d5453fd to
0d4e24a
Compare
deebc80 to
35461e9
Compare
0d4e24a to
ff86e6b
Compare
… 2026-07-28
HeaderMismatch -32001 -> -32020, MissingRequiredClientCapability -32003 ->
-32021, UnsupportedProtocolVersion -32004 -> -32022. These codes are part of
the draft 2026-07-28 protocol revision only and have never appeared on a
2025-era wire — the 2025 serving paths and the SDK-conventional -32001
('Session not found') on the stateful Streamable HTTP transport are unchanged.
- ProtocolErrorCode enum members and HEADER_MISMATCH_ERROR_CODE constant
renumbered; spec.types.2026-07-28 anchor constants follow.
- Emission sites (createMcpHandler/serveStdio/inboundClassification/mcpParamHeaders),
the LADDER_ERROR_HTTP_STATUS table and the multi-round-trip capability gate
emit the renumbered codes via the renumbered enum members.
- Client recognition: the probe classifier recognizes -32022 for the
corrective continuation, NOT_PROBE_RECOGNIZED keeps -32001 (deployed
Session-not-found overload) and adds -32020/-32021; ProtocolError.fromError
reconstructs the typed classes from the renumbered codes; the SEP-2243
one-refresh-on-miss retry keys on -32020.
- encodeErrorCode pass-through pin: -32000 literal untouched; the
pass-through list updated to -32020/-32021/-32022.
- Conformance: request-metadata / server-stateless /
http-custom-header-server-validation / http-header-validation now pass; the
'renumber pending Felix ruling' entries are burned from both
expected-failures yamls and the header NOTE rewritten to reflect alignment.
- Migration docs note the alpha-to-alpha renumber (no v1.x->v2 impact).
The vendored schema-twins/2026-07-28.schema.json and
corpus/fixtures/2026-07-28/* are deliberately untouched — they are sha256-
locked upstream artifacts at spec commit 2fb207da (pre-#2907) and refresh
atomically with the next spec-anchor repin.
…341d58 Replace the file:./vendor/*.tgz pin (built locally from conformance@d70d7ad) with the pkg.pr.new build of conformance@3341d58 (main, post #347 + #353). Pinned by commit SHA rather than PR number so the resolved tarball is immutable. Content-equivalent to the previous referee; expected-failures baselines are unchanged. - test/conformance/package.json: @modelcontextprotocol/conformance → https://pkg.pr.new/@modelcontextprotocol/conformance@3341d58 - test/conformance/vendor/: deleted (tarball + .gitignore override) - README.md: drop local-build/tarball/sha256 instructions; note pkg.pr.new pin pending the next npm publish - expected-failures.yaml: header pin → pkg.pr.new @3341d58 - pnpm-lock.yaml: regenerated
35461e9 to
9c942e2
Compare
ff86e6b to
c5142d3
Compare
c5142d3 to
8f1d91c
Compare
… is disconnected The standalone-SSE path stores to eventStore first, then writes if connected. The request-related path only stored when the stream was live, so a notification sent after closeSSE() (SEP-1699 polling) was silently dropped instead of being persisted for replay on reconnect. Exposed by the ctx.mcpReq.log request-related change against the new sse-polling example story; the gap pre-exists on main for any request-related notification (progress, ctx.mcpReq.notify) emitted after closeSSE().
8f1d91c to
b0afb16
Compare
Adopts the modelcontextprotocol/modelcontextprotocol#2907 / modelcontextprotocol/conformance#353 draft error-code renumber, swaps the conformance pin to the
pkg.pr.newalpha.5 build, and fixes a baseline gap instreamableHttprequest-related event storage. Stacked onfweinberger/on-e2e.Motivation and Context
spec#2907 error-code renumber
HeaderMismatch−32001 → −32020,MissingRequiredClientCapability−32003 → −32021,UnsupportedProtocolVersion−32004 → −32022.ProtocolErrorCodeenum,HEADER_MISMATCH_ERROR_CODE, and thespec.types.2026-07-28anchor constants renumberedcreateMcpHandler,serveStdio,inboundClassification,mcpParamHeaders,LADDER_ERROR_HTTP_STATUS, the multi-round-trip capability gate) and client recognition (probeClassifierUNSUPPORTED_PROTOCOL_VERSION= −32022;NOT_PROBE_RECOGNIZEDkeeps the deployed −32001 Session-not-found overload and adds −32020/−32021;ProtocolError.fromErrortyped-class reconstruction; SEP-2243 one-refresh-on-miss keys on −32020) follow via the enumstreamableHttp/createMcpHandler/originValidation/hostHeaderValidation/inboundClassification); theencodeErrorCodepass-through pin still holds for −32000, list updated to −32020/−32021/−32022streamableHttp, examples, e2e, conformance fixtures) and v1RequestTimeout/ConnectionClosedhistory inmigration.mdleft as-isschema-twins/2026-07-28.schema.json+corpus/fixtures/2026-07-28/*deliberately not touched (sha256-locked at a pre-#2907 spec anchor; refresh atomically with the next repin)expected-failures*.yamlburned (the renumber-pending entries removed); header NOTE rewritten to "aligned";migration.md/migration-SKILL.mdalpha-to-alpha note addedConformance pin → pkg.pr.new alpha.5
@modelcontextprotocol/conformancerepointed from the vendoredfile:./vendor/...d70d7ad.tgztohttps://pkg.pr.new/@modelcontextprotocol/conformance@357(commit65fcd39= the0.2.0-alpha.5version bump on top ofmain@d70d7ad; carries #347 + #353).test/conformance/vendor/deleted; README local-pin block rewritten (drops the tarball sha256 / rebuild recipe; switch to^0.2.0-alpha.5once published on npm).expected-failures.yamlheader pin updated. Referee content-identical tod70d7ad— no baseline reconcile needed.streamableHttpstore-first for request-related eventsThe
sse-pollingexample (callctx.http.closeSSE()then log while disconnected) exposed a baseline gap:streamableHttp.tssend()only stored request-related events toeventStorewhen the stream controller was live, so events emitted aftercloseSSEwere dropped — unlike the standalone path which stores first. Fixed to store-first, then write-if-connected.How Has This Been Tested?
Full gates at tip: typecheck, lint, build:all, docs:check; core (1210), server (334), client (536), codemod (350), server-legacy (162), express (40), fastify (22), hono (12), node (88), examples-shared (2); integration (348); e2e (2490p/205xf);
pnpm run:examples24 stories / 63 legs / 0 failed; conformanceclient:all(348p/12xf baseline ok,request-metadatapasses),server(42/0),server:draft(81/0),client:2026(baseline ok,request-metadata7/0),server:2026(baseline ok,server-stateless26/0,http-header-validation13/0,http-custom-header-server-validation9/0).Breaking Changes
None at the API surface. Wire-level: the 2026-07-28 era now emits −32020/−32021/−32022 where it previously emitted −32001/−32003/−32004 (alpha-to-alpha; covered by changeset + migration note).
Types of changes
Checklist
Additional context
60 files, +258/−257 in the renumber commit. The
streamableHttpstore-first fix is a standalone tip commit so it can be cherry-picked independently if preferred.