Fix integration tests for roots/list - #1119
Conversation
There is a race condition where the GET /mcp SSE stream in Streamable HTTP is established _after_ the server tries to send notifications. This surfaces errors from the MCP client sending notifications and breaks some integration tests. We add a utility showing to ensure the stream is established before sending server notifications. Fixes #1114 Signed-off-by: Daniel Garnier-Moiroux <git@garnier.wf>
There was a problem hiding this comment.
Verified the chain behind the barrier: DelegatingServlet.service() sets the flag only after the delegate returns, and a non-replay GET /mcp registers the listening stream synchronously inside doGet (McpStreamableServerSession#listeningStream), so a true flag implies the stream is registered in the session. setDelegate() resets the flag per test, and the await placement covers all four roots seams. LGTM, two minor nits below.
One thought on framing: since the server-side race intentionally stays as-is (consistent with the C#, TypeScript and Python SDKs), "Stabilizes tests for #1114" with the issue closed as not-planned would keep that decision discoverable, rather than "Fixes #1114".
This review was created with AI assistance.
|
|
||
| @Override | ||
| protected void awaitClientStreamEstablished() { | ||
| var timeout = Duration.ofSeconds(1); |
There was a problem hiding this comment.
Minor: 1s is tight next to the 5s awaits used elsewhere in this suite — on a loaded CI runner this could trade the -32603 flake for a "Failed to observe MCP Client connection" timeout. Would you consider aligning with the 5s convention (or a shared constant)? Non-blocking either way since the failure message would at least be clearer.
| } | ||
| current.service(request, response); | ||
| if (request instanceof HttpServletRequest req && req.getMethod().equals("GET")) { | ||
| sseStreamEstablished.set(true); |
There was a problem hiding this comment.
Might be worth one extra line in the utility's comment: the observed signal is "a GET completed through service()", which equals "listening stream registered" only because registration is synchronous inside doGet today. A replay GET (Last-Event-ID) or a rejected one (e.g. 400/404 from Accept/session checks, or protocol-version validation as proposed in #1112) would set the flag without registering a stream. Current tests hit neither, so fine as-is; just so the hook isn't over-trusted if doGet ever changes.
2 participants
There is a race condition where the GET /mcp SSE stream in Streamable HTTP is established after the server tries to send notifications. This surfaces errors from the MCP client sending notifications and breaks some integration tests.
We add a utility showing to ensure the stream is established before sending server notifications.
Fixes #1114
Supersedes #1115
@slachiewicz would you like to review this fix of #1114?