Skip to content

Conversation

@felixweinberger
Copy link
Contributor

@felixweinberger felixweinberger commented Nov 17, 2025

Summary

Implements SEP-1699 which enables servers to disconnect SSE connections at will by sending priming events and retry fields.

Motivation and Context

SEP-1699 introduces SSE polling behavior that allows servers to control client reconnection timing and close connections gracefully. This enables more efficient resource management on the server side while maintaining resumability.

We implement this on the POST SSE stream as implied by the SEP language linked above. I.e. when a server establishes an SSE stream:

  1. It's first message will be an event including no data, only an event ID.
  2. After that, it may call cancelSSEStream to close the stream while still gathering the events.
  3. The client can start "polling" the SSE stream based on the retryInterval supplied by the server before disconnection.

How Has This Been Tested?

Breaking Changes

None. Client falls back to exponential backoff if no retry field is provided.

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

@pkg-pr-new
Copy link

pkg-pr-new bot commented Nov 17, 2025

Open in StackBlitz

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

commit: 2516b88

toolResolve!();
});

it('should support POST SSE polling with client reconnection', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

probably the most interesting test

const primingEventId = primingIdMatch![1];

// Server closes the stream to trigger polling
transport.closeSSEStream(100);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this feels slightly weird. The whole point is for the server to be able to close SSE streams, so maybe this method should be exposed on server?

On the other hand, this disconnection only makes sense in the SHTTP transport - whereas the server is actually transport agnostic.

Maybe the correct expectation is to have the server call this method directly via its reference to the transport if necessary and actually available, so this might be fine actually.`

Copy link
Contributor

@mattzcarey mattzcarey Nov 18, 2025

Choose a reason for hiding this comment

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

I think this should be called by the user on the server. A bunch of frameworks dont give the user easy access to the transport but they do the server. Proxy by reference is good

Choose a reason for hiding this comment

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

To me it would feel very weird to have a closeSSEStream on the server that should have no knowledge of sessions let alone streams.

A bunch of frameworks dont give the user easy access to the transport but they do the server. Proxy by reference is good

This is a good point but maybe the right API is to expose the transport from the server?

Add support for SSE retry field to enable server-controlled client reconnection timing.

Client changes:
- Capture server-provided retry field from SSE events
- Use retry value for reconnection delay instead of exponential backoff
- Reconnect on graceful stream close with Last-Event-ID header

Server changes:
- Add retryInterval option to StreamableHTTPServerTransportOptions
- Send priming events with id/retry/empty-data when eventStore is configured
- Add closeSSEStream(requestId) API to close POST SSE streams for polling
- Priming events establish resumption capability before actual messages

Tests:
- Client: retry field capture, exponential backoff fallback, graceful reconnection
- Server: priming events, retry field, stream closure, POST SSE polling flow
@felixweinberger felixweinberger marked this pull request as ready for review November 18, 2025 11:29
@felixweinberger felixweinberger requested a review from a team as a code owner November 18, 2025 11:29
@felixweinberger
Copy link
Contributor Author

felixweinberger commented Nov 18, 2025

Hi @jonathanhefner would love feedback on whether this accurately captures the intent of your original SEP

@paoloricciuti in case you want to TAL as you had valuable input on modelcontextprotocol/conformance#35

Copy link

@paoloricciuti paoloricciuti left a comment

Choose a reason for hiding this comment

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

So we actually had a brief discussion about this on discord and I think the spec should be clearer here: from the discussion we had the reconnection should be different from the standalone stream. So if the client was doing two POST requests each POST would have it's own lastEventId.

On reconnect this should basically result in 3 new GET SSE streams, two to get the remaining notificiations/responses from the POST requests and one as a standalone stream for the new notifications (and to resume the notifications eventually sent during the disconnection period).

However right now the server still errors out if there's more that one SSE stream. Should this be fixed? Is this even the right interpretation of the spec?

const primingEventId = primingIdMatch![1];

// Server closes the stream to trigger polling
transport.closeSSEStream(100);

Choose a reason for hiding this comment

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

To me it would feel very weird to have a closeSSEStream on the server that should have no knowledge of sessions let alone streams.

A bunch of frameworks dont give the user easy access to the transport but they do the server. Proxy by reference is good

This is a good point but maybe the right API is to expose the transport from the server?

Copy link
Member

@jonathanhefner jonathanhefner left a comment

Choose a reason for hiding this comment

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

would love feedback on whether this accurately captures the intent of your original SEP

Yes, looks great! Thank you! 😃

toolResolve!();

// Give the tool time to complete and store the result
await new Promise(resolve => setTimeout(resolve, 50));
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if this is a concern, but this could lead to flaky tests. I'm not sure what we could await as an alternative though.

Addresses PR feedback from paoloricciuti requesting test coverage
for the scenario where multiple messages are sent while the SSE
client is disconnected. Uses a batch of tool calls to generate
multiple responses that get stored and replayed on reconnection.
@felixweinberger
Copy link
Contributor Author

felixweinberger commented Nov 19, 2025

So we actually had a brief discussion about this on discord and I think the spec should be clearer here: from the discussion we had the reconnection should be different from the standalone stream. So if the client was doing two POST requests each POST would have it's own lastEventId.

On reconnect this should basically result in 3 new GET SSE streams, two to get the remaining notificiations/responses from the POST requests and one as a standalone stream for the new notifications (and to resume the notifications eventually sent during the disconnection period).

However right now the server still errors out if there's more that one SSE stream. Should this be fixed? Is this even the right interpretation of the spec?

Ack, after the discusson on Discord seems clear we need to not error out on multiple streams. Still figuring out the right way to do that on this PR, will come back with an update soon, moving to draft for now.

@felixweinberger felixweinberger marked this pull request as draft November 19, 2025 00:22
- Fix replayEvents to use streamId from last-event-id header
- Add conflict check per streamId (not global)
- Add missing close handler to clean up stream mapping
- Add test demonstrating concurrent GET streams resuming different POST streams

This aligns with the spec: "The client MAY remain connected to
multiple SSE streams simultaneously."
@felixweinberger
Copy link
Contributor Author

Horrible timeout based tests - but looking for feedback on the implementation before figuring out prettier tests.

- Add closeStandaloneSSEStream() method to allow server to close the
  standalone GET notification stream, triggering client reconnection
- Send priming event with retry field on GET streams (when eventStore
  configured) for resumability
- Add tests for GET stream priming events and closeStandaloneSSEStream
- Fix flaky test timeout for POST SSE polling test
@felixweinberger felixweinberger marked this pull request as draft November 20, 2025 15:15
Keep PR focused on POST stream resumability only:
- Remove closeStandaloneSSEStream() method
- Remove priming event from GET notification stream
- Remove GET stream polling tests
- Update resumability tests to not expect GET priming events
The server's replayEvents now requires getStreamIdForEventId to resolve
the stream ID from an event ID. Without this, the fallback parsing
with '::' separator doesn't match InMemoryEventStore's '_' separator,
causing the resumability test to fail.
When getStreamIdForEventId is not implemented, fall back to using the
streamId returned by replayEventsAfter() instead of requiring a specific
event ID format. This preserves compatibility with existing EventStore
implementations that don't implement the new optional method.
The test was incorrectly calling client.request() with resumptionToken
expecting a POST response. Per the spec's "Resumability and Redelivery"
section, resumption uses GET with Last-Event-ID header:

> If the client wishes to resume after a broken connection, it SHOULD
> issue an HTTP GET to the MCP endpoint, and include the Last-Event-ID
> header to indicate the last event ID it received.

See: https://modelcontextprotocol.io/specification/2025-06-18/basic/transports#resumability-and-redelivery

When resumptionToken is provided, the client's send() method only
reconnects the GET SSE stream and returns early - it never sends the
POST request. Fix by using transport.send() with a notification (no
response expected) to properly trigger GET-based SSE reconnection.
// Per spec, resumption uses GET with Last-Event-ID header, not POST
// When resumptionToken is provided, send() only triggers GET reconnection and returns early
// We use a notification (no id) so we don't expect a response
await transport2.send(
Copy link
Contributor Author

@felixweinberger felixweinberger Nov 20, 2025

Choose a reason for hiding this comment

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

fixed this test to use GET for resumption as required by the spec: https://modelcontextprotocol.io/specification/2025-06-18/basic/transports#resumability-and-redelivery

cc: @mattzcarey as we chatted about this weirdness between POST & GET

Choose a reason for hiding this comment

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

It feels weird to have to send a notification to resume the stream...wouldn't it make sense to have a specific method to resume?

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.

Implement SEP-1699: Support SSE Polling via Server-Side Disconnect

5 participants