direct long running tool progress notifications to correct stream#2672
direct long running tool progress notifications to correct stream#2672
Conversation
There was a problem hiding this comment.
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.
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. b) I verified in the Typescript SDK that the send method of 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. |
|
Hi Cliff, see video. There are some GET calls interspersed due to the random log messages. GET_POST_share.mp4 |

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
Checklist