Skip to content

Conversation

@evalstate
Copy link
Member

Specifies related Request ID to allow routing to POST response SSE stream for long running tool notifications.

Description

Fixes issue modelcontextprotocol/modelcontextprotocol#1394

Server Details

Everything Server reference implementation.

Motivation and Context

As per issue 1394.

How Has This Been Tested?

Tested locally.

Breaking Changes

None.

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 Protocol Documentation
  • My changes follows MCP security best practices
  • I have updated the server's README accordingly
  • I have tested this with an LLM client
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have documented all environment variables and configuration options

@evalstate evalstate added bug Something isn't working server-everything Reference implementation for the Everything MCP server - src/everything labels Sep 3, 2025
Copy link
Member

@cliffhall cliffhall left a comment

Choose a reason for hiding this comment

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

Approving because I agree that this change is correct and necessary. It ensures that the protocol will not debounce the messages, and that's always what we want for progress notifications.

However, I was not able to verify that the issue that this PR was raised in response to
a) actually exists
b) was solved by this

a) That issue claims that the progress notifications are sent from the server to the client on the stream opened by a GET request. That is actually what the specification calls for on the StreamableHttp transport.

b) In my tests using the Inspector to connect to the everything server and invoke the longRunningOperation tool both with and without this change, I see all activity happening on the POST, not on the GET as the spec would indicate it should be doing. I believe this is the result of the proxy being between the client and server, muddying the waters and that the server is likely sending these progress notifications on the GET stream as expected.

@cliffhall
Copy link
Member

However, I was not able to verify that the issue that this PR was raised in response to a) actually exists b) was solved by this

Ok, I now have more information and see that

a) it is a problem (any SSE message associated with a live request should go out on the stream dedicated to that request, not the standalone stream for notifications such as list or resource changes.
image

b) I verified in the Typescript SDK that the send method of ‎StreamableHTTPServerTransport will in fact check for relatedRequestId and direct messages to the appropriate stream (standalone if it exists and there is no relatedRequestId, or the dedicated stream for the request if relatedRequestId is present).

Therefore, what I was seeing when I tested with the inspector (videos posted to the issue that this PR was raised in response to) was in fact the Inspector proxy muddying the waters. There, I observed that both before and after this change, the messages appeared on the POST opened by the client to the proxy. The hidden behavior was the stream the server was sending to the proxy on.

@evalstate has confirmed that his locally instrumented version of the Python SDK showed that the notification was previously going out on the standalone (GET) stream, and this change causes it to go out on the POST stream for the request. He will post before/after screenshots to this PR as soon as he's back at his machine.

@cliffhall cliffhall merged commit 8ba0ff5 into main Sep 5, 2025
23 checks passed
@cliffhall cliffhall deleted the fix/notification-stream branch September 5, 2025 16:51
@evalstate
Copy link
Member Author

Hi Cliff, see video. There are some GET calls interspersed due to the random log messages.

GET_POST_share.mp4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working server-everything Reference implementation for the Everything MCP server - src/everything

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants