fix(server): close a listen stream that has honored nothing - #2651
fix(server): close a listen stream that has honored nothing#2651sushantkumar23 wants to merge 3 commits into
Conversation
🦋 Changeset detectedLatest commit: f847f46 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 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/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
|
Flagging that the second commit touches an existing e2e test, since that deserves a look rather than being taken on trust.
The test is about If you would rather the guard keep testing the empty-filter case specifically, the alternative is to narrow the fix so it only closes when a client asked for something the server cannot honor, leaving an explicitly-empty filter holding the stream. I did not take that route because it seems the wrong way round — a client that asks for nothing has less claim on an open stream, not more — but I am happy to switch if you disagree. Full e2e suite passes locally with the change (44 files, 2637 tests). |
claude
Bot
added
the
v2
`serve()` computes `honoredSubset(filter, capabilities)` and then opens the
stream, acks, subscribes to the bus and arms a keepalive without consulting
it. When a server declares no `listChanged` and no `resources.subscribe`,
`honored` is `{}` — `listenFilterAccepts({}, event)` is false for every
event kind, so the subscription is provably a no-op and nothing can follow
the ack. Nothing closes the stream either: `teardown` runs only on client
disconnect or abort.
Close gracefully once the ack is out, reusing the same `teardown(true)`
path `closeAll()` uses, so the client still learns exactly what was
honored and still receives the `resultType: "complete"` result. Streams
that honor at least one type are untouched.
The transport binding for 2026-07-28 ends the listen stream "until the
client or server closes the stream", so a server-side close is in spec.
This is most visible on request-scoped runtimes: an invocation held for a
subscription that can never deliver runs until the platform kills it, and
the client immediately reopens. Observed in production as a continuous
reconnect cycle, one held invocation per connected client, independent of
the configured function timeout.
Refs modelcontextprotocol#2650
`subscriptions:listen:capacity-guard` opened both subscriptions with an empty filter. That is the one case the preceding commit now closes: an empty filter honors nothing, so the first subscription acks, completes and releases its slot before the second arrives — which then gets a stream instead of the `-32603` the test expects. The empty filter was incidental. What the test is about is `maxSubscriptions`, and `makeServer()` registers a tool, so `toolsListChanged` is advertised and honored: the subscription stays open and occupies the slot the guard is there to defend. The assertions are unchanged, and the guard is now exercised by a subscription that a server would really hold.
adcd29b to
f847f46
Compare
|
Ready for review whenever someone has a moment — rebased onto current @felixweinberger @mattzcarey — tagging you two specifically because of the code rather than to cast a wide net: Felix wrote The linked issue #2650 is labelled Two things worth a reviewer's eye:
|
1 participant
Closes #2650.
The bug
listenRouter.serve()computeshonoredSubset(filter, capabilities)— the set it will actually deliver — and then opens the SSE stream, acks, subscribes to the bus and arms a keepalive without ever consulting it.When a server declares no
listChangedand noresources.subscribe,honoredis{}.listenFilterAccepts({}, event)returns false for every event kind, so the bus subscription is provably a no-op and nothing can follow the ack. Nothing closes the stream either —teardownruns only on client disconnect or abort. The connection is held open indefinitely to deliver a set the server has already told the client is empty.The change
Once the ack is enqueued, if
honoredhas no entries, close via the existingteardown(true)— the same graceful pathcloseAll()uses. The client still learns exactly what was honored, and still receives theresultType: "complete"result, so it can distinguish this from a transport drop.Streams honoring at least one type are untouched.
Why this is in spec
The 2026-07-28 transport binding ends the listen stream "until the client or server closes the stream", so a server-side close is explicitly permitted. The ack-first MUST is preserved — the acknowledged notification is still the first frame.
Impact
Most visible on request-scoped runtimes. An invocation held for a subscription that can never deliver runs until the platform kills it, and the client immediately reopens:
Each connected client permanently occupies one invocation; the timeout only changes how the same wall-clock time is sliced. #2650 has production logs showing the handler resolving in 44 ms and the invocation then dying at the ceiling, plus a before/after where refusing the method at the route removes the timeouts entirely with tool calls unaffected.
Tests
Two added to
createMcpHandlerListen.test.ts:capabilities: {}acks withnotifications: {}and the stream ends on its own, final frame being thecompleteresulttoolsstill holds the stream, so the narrowing does not close streams that can still deliverpackages/server: 470 tests pass, prettier and typecheck clean.