Fix integration tests for roots/list

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>
This commit is contained in:
Daniel Garnier-Moiroux 2026-08-31 18:24:12 +02:00 • committed by Daniel Garnier-Moiroux
commit 59316252c4
3 changed files with 41 additions and 6 deletions

View file

@ -82,6 +82,13 @@ public abstract class AbstractMcpClientServerIntegrationTests {
abstract protected McpServer.SyncSpecification<?> prepareSyncServerBuilder(); abstract protected McpServer.SyncSpecification<?> prepareSyncServerBuilder();
// There is, for Streamable HTTP, a race condition between establishing the SSE stream
// and the server sending notifications. This breaks some `roots/list` tests (and
// could in theory break sampling and elicitation tests). This utility method allows
// delaying the test until the stream is opened.
protected void awaitClientStreamEstablished() {
}
@ParameterizedTest(name = "{0} : {displayName} ") @ParameterizedTest(name = "{0} : {displayName} ")
@MethodSource("clientsForTesting") @MethodSource("clientsForTesting")
void simple(String clientType) { void simple(String clientType) {
@ -1082,6 +1089,7 @@ public abstract class AbstractMcpClientServerIntegrationTests {
InitializeResult initResult = mcpClient.initialize(); InitializeResult initResult = mcpClient.initialize();
assertThat(initResult).isNotNull(); assertThat(initResult).isNotNull();
awaitClientStreamEstablished();
assertThat(rootsRef.get()).isNull(); assertThat(rootsRef.get()).isNull();
@ -1168,7 +1176,7 @@ public abstract class AbstractMcpClientServerIntegrationTests {
InitializeResult initResult = mcpClient.initialize(); InitializeResult initResult = mcpClient.initialize();
assertThat(initResult).isNotNull(); assertThat(initResult).isNotNull();
awaitClientStreamEstablished();
mcpClient.rootsListChangedNotification(); mcpClient.rootsListChangedNotification();
await().atMost(Duration.ofSeconds(5)).untilAsserted(() -> { await().atMost(Duration.ofSeconds(5)).untilAsserted(() -> {
@ -1201,7 +1209,7 @@ public abstract class AbstractMcpClientServerIntegrationTests {
.build()) { .build()) {
assertThat(mcpClient.initialize()).isNotNull(); assertThat(mcpClient.initialize()).isNotNull();
awaitClientStreamEstablished();
mcpClient.rootsListChangedNotification(); mcpClient.rootsListChangedNotification();
await().atMost(Duration.ofSeconds(5)).untilAsserted(() -> { await().atMost(Duration.ofSeconds(5)).untilAsserted(() -> {
@ -1234,7 +1242,7 @@ public abstract class AbstractMcpClientServerIntegrationTests {
InitializeResult initResult = mcpClient.initialize(); InitializeResult initResult = mcpClient.initialize();
assertThat(initResult).isNotNull(); assertThat(initResult).isNotNull();
awaitClientStreamEstablished();
mcpClient.rootsListChangedNotification(); mcpClient.rootsListChangedNotification();
await().atMost(Duration.ofSeconds(5)).untilAsserted(() -> { await().atMost(Duration.ofSeconds(5)).untilAsserted(() -> {

View file

@ -30,7 +30,6 @@ import jakarta.servlet.http.HttpServletResponse;
import org.apache.catalina.LifecycleException; import org.apache.catalina.LifecycleException;
import org.apache.catalina.LifecycleState; import org.apache.catalina.LifecycleState;
import org.apache.catalina.startup.Tomcat; import org.apache.catalina.startup.Tomcat;
import org.awaitility.Awaitility;
import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.BeforeAll;
@ -42,6 +41,7 @@ import reactor.core.publisher.Mono;
import reactor.test.StepVerifier; import reactor.test.StepVerifier;
import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThat;
import static org.awaitility.Awaitility.await;
@Timeout(15) @Timeout(15)
class HttpServletStreamableIntegrationTests extends AbstractMcpClientServerIntegrationTests { class HttpServletStreamableIntegrationTests extends AbstractMcpClientServerIntegrationTests {
@ -57,6 +57,16 @@ class HttpServletStreamableIntegrationTests extends AbstractMcpClientServerInteg
private HttpServletStreamableServerTransportProvider mcpServerTransportProvider; private HttpServletStreamableServerTransportProvider mcpServerTransportProvider;
@Override
protected void awaitClientStreamEstablished() {
var timeout = Duration.ofSeconds(5);
await().atMost(timeout).untilAsserted(() -> {
assertThat(MCP_SERVLET.isStreamEstablished())
.withFailMessage("[Failed to observe MCP Client connection within %s]", timeout)
.isTrue();
});
}
static Stream<Arguments> clientsForTesting() { static Stream<Arguments> clientsForTesting() {
return Stream.of(Arguments.of("httpclient")); return Stream.of(Arguments.of("httpclient"));
} }
@ -151,7 +161,7 @@ class HttpServletStreamableIntegrationTests extends AbstractMcpClientServerInteg
.verifyComplete(); .verifyComplete();
// Wait until we've received the response // Wait until we've received the response
Awaitility.await().atMost(Duration.ofSeconds(1)).until(() -> response.get() != null); await().atMost(Duration.ofSeconds(1)).until(() -> response.get() != null);
assertThat(response.get().error().code()).isEqualTo(McpSchema.ErrorCodes.METHOD_NOT_FOUND); assertThat(response.get().error().code()).isEqualTo(McpSchema.ErrorCodes.METHOD_NOT_FOUND);
assertThat(response.get().error().message()).isEqualTo("Method not found: foo/bar"); assertThat(response.get().error().message()).isEqualTo("Method not found: foo/bar");

View file

@ -7,6 +7,7 @@ package io.modelcontextprotocol.server.transport;
import java.io.IOException; import java.io.IOException;
import java.net.InetSocketAddress; import java.net.InetSocketAddress;
import java.net.ServerSocket; import java.net.ServerSocket;
import java.util.concurrent.atomic.AtomicBoolean;
import jakarta.servlet.Filter; import jakarta.servlet.Filter;
import jakarta.servlet.Servlet; import jakarta.servlet.Servlet;
@ -14,7 +15,9 @@ import jakarta.servlet.ServletConfig;
import jakarta.servlet.ServletException; import jakarta.servlet.ServletException;
import jakarta.servlet.ServletRequest; import jakarta.servlet.ServletRequest;
import jakarta.servlet.ServletResponse; import jakarta.servlet.ServletResponse;
import jakarta.servlet.http.HttpServletRequest;
import org.apache.catalina.Context; import org.apache.catalina.Context;
import org.apache.catalina.Wrapper;
import org.apache.catalina.startup.Tomcat; import org.apache.catalina.startup.Tomcat;
import org.apache.tomcat.util.descriptor.web.FilterDef; import org.apache.tomcat.util.descriptor.web.FilterDef;
import org.apache.tomcat.util.descriptor.web.FilterMap; import org.apache.tomcat.util.descriptor.web.FilterMap;
@ -41,7 +44,7 @@ public class TomcatTestUtil {
Context context = tomcat.addContext(contextPath, baseDir); Context context = tomcat.addContext(contextPath, baseDir);
// Add transport servlet to Tomcat // Add transport servlet to Tomcat
org.apache.catalina.Wrapper wrapper = context.createWrapper(); Wrapper wrapper = context.createWrapper();
wrapper.setName("mcpServlet"); wrapper.setName("mcpServlet");
wrapper.setServlet(servlet); wrapper.setServlet(servlet);
wrapper.setLoadOnStartup(1); wrapper.setLoadOnStartup(1);
@ -78,12 +81,19 @@ public class TomcatTestUtil {
private volatile Servlet delegate; private volatile Servlet delegate;
// Crude way of tracking whether a GET SSE stream has been established, to ensure
// a Streamable HTTP MCP Client is connected.
// This is an approximation: it switches to "true" whenever the handler is done
// servicing a GET request - even if the request is rejected or if it's a replay.
private final AtomicBoolean sseStreamEstablished = new AtomicBoolean(false);
/** /**
* Sets the servlet handling subsequent requests. The delegate is not * Sets the servlet handling subsequent requests. The delegate is not
* {@link Servlet#init(ServletConfig) initialized}, since the MCP servlet * {@link Servlet#init(ServletConfig) initialized}, since the MCP servlet
* transports do not rely on their {@link ServletConfig}. * transports do not rely on their {@link ServletConfig}.
*/ */
public void setDelegate(Servlet delegate) { public void setDelegate(Servlet delegate) {
this.sseStreamEstablished.set(false);
this.delegate = delegate; this.delegate = delegate;
} }
@ -104,6 +114,9 @@ public class TomcatTestUtil {
throw new IllegalStateException("No delegate servlet has been set"); throw new IllegalStateException("No delegate servlet has been set");
} }
current.service(request, response); current.service(request, response);
if (request instanceof HttpServletRequest req && req.getMethod().equals("GET")) {
sseStreamEstablished.set(true);
}
} }
@Override @Override
@ -116,6 +129,10 @@ public class TomcatTestUtil {
this.delegate = null; this.delegate = null;
} }
public boolean isStreamEstablished() {
return this.sseStreamEstablished.get();
}
} }
/** /**