Skip to content

feat(core): adopt spec#2907 error-code renumber; conformance pin to pkg.pr.new alpha.5; streamableHttp store-first fix#2335

Open
felixweinberger wants to merge 3 commits into
fweinberger/on-e2efrom
fweinberger/on-renumber
Open

feat(core): adopt spec#2907 error-code renumber; conformance pin to pkg.pr.new alpha.5; streamableHttp store-first fix#2335
felixweinberger wants to merge 3 commits into
fweinberger/on-e2efrom
fweinberger/on-renumber

Conversation

@felixweinberger

Copy link
Copy Markdown
Contributor

Adopts the modelcontextprotocol/modelcontextprotocol#2907 / modelcontextprotocol/conformance#353 draft error-code renumber, swaps the conformance pin to the pkg.pr.new alpha.5 build, and fixes a baseline gap in streamableHttp request-related event storage. Stacked on fweinberger/on-e2e.

Motivation and Context

spec#2907 error-code renumber

HeaderMismatch −32001 → −32020, MissingRequiredClientCapability −32003 → −32021, UnsupportedProtocolVersion −32004 → −32022.

  • ProtocolErrorCode enum, HEADER_MISMATCH_ERROR_CODE, and the spec.types.2026-07-28 anchor constants renumbered
  • emission sites (createMcpHandler, serveStdio, inboundClassification, mcpParamHeaders, LADDER_ERROR_HTTP_STATUS, the multi-round-trip capability gate) and client recognition (probeClassifier UNSUPPORTED_PROTOCOL_VERSION = −32022; NOT_PROBE_RECOGNIZED keeps the deployed −32001 Session-not-found overload and adds −32020/−32021; ProtocolError.fromError typed-class reconstruction; SEP-2243 one-refresh-on-miss keys on −32020) follow via the enum
  • the −32000 literal is verified untouched (streamableHttp / createMcpHandler / originValidation / hostHeaderValidation / inboundClassification); the encodeErrorCode pass-through pin still holds for −32000, list updated to −32020/−32021/−32022
  • 2025-era emission untouched: Session-not-found −32001 (streamableHttp, examples, e2e, conformance fixtures) and v1 RequestTimeout/ConnectionClosed history in migration.md left as-is
  • vendored schema-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)
  • every test/e2e assertion pinning the old codes flipped (28 test files); 8 pre-existing alpha changesets retro-aligned + 1 new changeset; both expected-failures*.yaml burned (the renumber-pending entries removed); header NOTE rewritten to "aligned"; migration.md / migration-SKILL.md alpha-to-alpha note added

Conformance pin → pkg.pr.new alpha.5

@modelcontextprotocol/conformance repointed from the vendored file:./vendor/...d70d7ad.tgz to https://pkg.pr.new/@modelcontextprotocol/conformance@357 (commit 65fcd39 = the 0.2.0-alpha.5 version bump on top of main@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.5 once published on npm). expected-failures.yaml header pin updated. Referee content-identical to d70d7ad — no baseline reconcile needed.

streamableHttp store-first for request-related events

The sse-polling example (call ctx.http.closeSSE() then log while disconnected) exposed a baseline gap: streamableHttp.ts send() only stored request-related events to eventStore when the stream controller was live, so events emitted after closeSSE were 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:examples 24 stories / 63 legs / 0 failed; conformance client:all (348p/12xf baseline ok, request-metadata passes), server (42/0), server:draft (81/0), client:2026 (baseline ok, request-metadata 7/0), server:2026 (baseline ok, server-stateless 26/0, http-header-validation 13/0, http-custom-header-server-validation 9/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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

60 files, +258/−257 in the renumber commit. The streamableHttp store-first fix is a standalone tip commit so it can be cherry-picked independently if preferred.

@felixweinberger felixweinberger requested a review from a team as a code owner June 19, 2026 14:18
@changeset-bot

changeset-bot Bot commented Jun 19, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: b0afb16

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages
Name Type
@modelcontextprotocol/client Minor
@modelcontextprotocol/core Minor
@modelcontextprotocol/server Minor
@modelcontextprotocol/codemod Patch
@modelcontextprotocol/express Major
@modelcontextprotocol/fastify Major
@modelcontextprotocol/hono Major
@modelcontextprotocol/node Major

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

@pkg-pr-new

pkg-pr-new Bot commented Jun 19, 2026

Copy link
Copy Markdown

Open in StackBlitz

@modelcontextprotocol/client

npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/client@2335

@modelcontextprotocol/codemod

npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/codemod@2335

@modelcontextprotocol/server

npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/server@2335

@modelcontextprotocol/server-legacy

npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/server-legacy@2335

@modelcontextprotocol/express

npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/express@2335

@modelcontextprotocol/fastify

npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/fastify@2335

@modelcontextprotocol/hono

npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/hono@2335

@modelcontextprotocol/node

npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/node@2335

commit: b0afb16

Comment thread packages/server/src/server/streamableHttp.ts
Comment thread packages/server/src/server/streamableHttp.ts
Comment thread .changeset/pin-modern-rejection-codes.md Outdated
Comment thread test/conformance/package.json Outdated
Comment thread .changeset/spec-anchor-repin-2fb207da.md Outdated
Comment thread packages/server/src/server/streamableHttp.ts
Comment thread packages/server/src/server/streamableHttp.ts
Comment thread docs/migration-SKILL.md
Comment thread packages/server/src/server/streamableHttp.ts Outdated
@felixweinberger felixweinberger force-pushed the fweinberger/on-renumber branch 2 times, most recently from dd652fd to 56eeb2c Compare June 22, 2026 20:15
@felixweinberger felixweinberger force-pushed the fweinberger/on-renumber branch from 56eeb2c to d5453fd Compare June 22, 2026 20:21

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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's GET + Last-Event-ID reconnect replays the stored final response but replayEvents() 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 _streamMapping entry until transport.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 streamId S. When the handler finishes before the client reconnects, send() for the final response stores it to the eventStore (the placeholder satisfies the new stream !== undefined gate at line 1031), then the response branch finds the placeholder, calls stream.cleanup() at line 1069 (deleting _streamMapping[S]), and the cleanup loop at lines 1072-1075 deletes _requestResponseMap / _requestToStreamMapping for the request. When the client then reconnects with GET + 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 under S at 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) targets S, and its response branch runs stream.cleanup(), closing the controller and removing the entry. In the resume-after-response flow there will never be another send() for S: _requestToStreamMapping no 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's ReadableStream cancel callback is deliberately empty ("Cleanup will be handled by the mapping", lines 544-547), so even a client-side disconnect leaves the entry. Only transport.close() clears it.

    Impact. Two consequences per completed-while-disconnected poll cycle on a long-lived sessionful transport: (a) the server holds an open ReadableStream controller plus a permanent _streamMapping entry — 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 calls ctx.http.closeSSE()closeSSEStream(5) closes the controller and installs the placeholder under S. (3) The handler finishes before the client's retry interval elapses; send(response) stores the response (line 1031-1032), the response branch runs stream.cleanup() (line 1069) deleting _streamMapping[S], and the cleanup loop deletes _requestToStreamMapping[5]. (4) The client reconnects with GET + Last-Event-ID; replayEvents() finds no conflict, replays the stored events including the final response, and registers a resumed stream under S (line 564). (5) From this point on, no code path can reach that entry: send() can never resolve S again, the cancel callback is a no-op, and the SSE response stays open. The entry and its controller persist until transport.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 a Last-Event-ID reconnect could never be the delivery vehicle for an already-settled response — any resumed per-request stream still had its live response coming through send(), 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 in replayEvents(), check whether any entry in _requestToStreamMapping still maps to the resolved replayedStreamId; if none does (the replay-after-completion case), close the controller and skip (or immediately remove) the _streamMapping registration instead of leaving the resumed stream open indefinitely. A regression test that performs the closeSSE() → complete-while-disconnected → Last-Event-ID GET sequence and asserts the replayed SSE stream is closed and _streamMapping does 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 called closeSSE(), the placeholder _streamMapping entry installed by closeSSEStream() is never removed: the Protocol layer suppresses the final response on cancellation, and the response-branch stream.cleanup() is the placeholder's only normal removal path. The entry (plus the _requestToStreamMapping entry) leaks for the lifetime of the sessionful transport, and for eventStores implementing getStreamIdForEventId the leaked placeholder makes replayEvents()'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-request streamId instead of deleting the _streamMapping entry as the pre-PR stream.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: in packages/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), so transport.send(response) never runs for a cancelled request and the response-branch cleanup loop — the only normal-operation path that calls the placeholder's cleanup() — never executes.\n\nThe code path. Entirely within the SEP-1699 polling flow this PR targets: (1) a sessionful 2025-era transport with an eventStore; (2) a long-running tool handler calls ctx.http.closeSSE()closeSSEStream(requestId) closes the controller and installs the placeholder; (3) the client cancels the in-flight request by POSTing notifications/cancelled (the normal 2025-era cancellation path); (4) the Protocol layer aborts the request and suppresses the response, so send(response) never runs for that request id; (5) nothing else removes the placeholder — the original per-request ReadableStream's cancel callback cannot fire because closeSSE already closed the controller gracefully, and replayEvents() only overwrites the entry if the cancelling client bothers to resume, which it has no reason to do. Only the transport-level close() clears it.\n\nImpact. The placeholder _streamMapping entry (plus the pre-existing _requestToStreamMapping entry) accumulates one per cancelled-after-closeSSE request for the lifetime of the long-lived sessionful transport. The sharper consequence: for eventStores implementing getStreamIdForEventId, replayEvents()'s conflict check at line 520 (this._streamMapping.get(streamId) !== undefined → 409) treats the leaked placeholder as an active connection, so any later Last-Event-ID resume 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() called stream.cleanup(), which deleted the _streamMapping entry — cancellation after closeSSE left no _streamMapping residue and no 409 (only the pre-existing _requestToStreamMapping leak). 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 after closeSSE(), 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 POSTs tools/call id 5 on a sessionful transport with a getStreamIdForEventId-implementing eventStore: _requestToStreamMapping[5] = S, _streamMapping[S] is the live per-request stream. (2) The handler calls ctx.http.closeSSE(): the controller is closed and _streamMapping[S] becomes the placeholder. (3) The client decides to give up and POSTs notifications/cancelled for id 5; _oncancel aborts the per-request AbortController. (4) The handler eventually settles; both completion arms in _onrequest see abortController.signal.aborted and return without calling transport.send. (5) The placeholder's cleanup() is never invoked; _streamMapping[S] and _requestToStreamMapping[5] persist until the session is closed. (6) Any later GET with a Last-Event-ID resolving to stream S hits 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 processing notifications/cancelled for a request whose stream entry is a controller-less placeholder), or key the intentional-close marker somewhere other than _streamMapping so neither the conflict check nor the lifetime of the map is affected. A regression test that cancels a request after closeSSE() and asserts the mappings are cleared (and a subsequent Last-Event-ID resume succeeds) would lock the behavior in.

Comment thread packages/core/src/types/spec.types.2026-07-28.ts
Comment thread packages/server/src/server/streamableHttp.ts
Comment thread packages/server/src/server/streamableHttp.ts
… 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
@felixweinberger felixweinberger force-pushed the fweinberger/on-renumber branch from ff86e6b to c5142d3 Compare June 22, 2026 21:45
Comment thread packages/server/src/server/streamableHttp.ts
@felixweinberger felixweinberger force-pushed the fweinberger/on-renumber branch from c5142d3 to 8f1d91c Compare June 22, 2026 22:04
Comment thread packages/server/src/server/streamableHttp.ts
Comment thread packages/server/src/server/streamableHttp.ts
Comment thread packages/server/src/server/streamableHttp.ts
… 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().
@felixweinberger felixweinberger force-pushed the fweinberger/on-renumber branch from 8f1d91c to b0afb16 Compare June 22, 2026 23:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant