Skip to content

Commit 7b7eab8

Browse files
committed
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>
1 parent 24fbb11 commit 7b7eab8

3 files changed

Lines changed: 40 additions & 6 deletions

File tree

mcp-test/src/main/java/io/modelcontextprotocol/AbstractMcpClientServerIntegrationTests.java

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,13 @@ public abstract class AbstractMcpClientServerIntegrationTests {
8282

8383
abstract protected McpServer.SyncSpecification<?> prepareSyncServerBuilder();
8484

85+
// There is, for Streamable HTTP, a race condition between establishing the SSE stream
86+
// and the server sending notifications. This breaks some `roots/list` tests (and
87+
// could in theory break sampling and elicitation tests). This utility method allows
88+
// delaying the test until the stream is opened.
89+
protected void awaitClientStreamEstablished() {
90+
}
91+
8592
@ParameterizedTest(name = "{0} : {displayName} ")
8693
@MethodSource("clientsForTesting")
8794
void simple(String clientType) {
@@ -1082,6 +1089,7 @@ void testRootsSuccess(String clientType) {
10821089

10831090
InitializeResult initResult = mcpClient.initialize();
10841091
assertThat(initResult).isNotNull();
1092+
awaitClientStreamEstablished();
10851093

10861094
assertThat(rootsRef.get()).isNull();
10871095

@@ -1168,7 +1176,7 @@ void testRootsNotificationWithEmptyRootsList(String clientType) {
11681176

11691177
InitializeResult initResult = mcpClient.initialize();
11701178
assertThat(initResult).isNotNull();
1171-
1179+
awaitClientStreamEstablished();
11721180
mcpClient.rootsListChangedNotification();
11731181

11741182
await().atMost(Duration.ofSeconds(5)).untilAsserted(() -> {
@@ -1201,7 +1209,7 @@ void testRootsWithMultipleHandlers(String clientType) {
12011209
.build()) {
12021210

12031211
assertThat(mcpClient.initialize()).isNotNull();
1204-
1212+
awaitClientStreamEstablished();
12051213
mcpClient.rootsListChangedNotification();
12061214

12071215
await().atMost(Duration.ofSeconds(5)).untilAsserted(() -> {
@@ -1234,7 +1242,7 @@ void testRootsServerCloseWithActiveSubscription(String clientType) {
12341242

12351243
InitializeResult initResult = mcpClient.initialize();
12361244
assertThat(initResult).isNotNull();
1237-
1245+
awaitClientStreamEstablished();
12381246
mcpClient.rootsListChangedNotification();
12391247

12401248
await().atMost(Duration.ofSeconds(5)).untilAsserted(() -> {

mcp-test/src/test/java/io/modelcontextprotocol/server/HttpServletStreamableIntegrationTests.java

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,6 @@
3030
import org.apache.catalina.LifecycleException;
3131
import org.apache.catalina.LifecycleState;
3232
import org.apache.catalina.startup.Tomcat;
33-
import org.awaitility.Awaitility;
3433
import org.junit.jupiter.api.AfterAll;
3534
import org.junit.jupiter.api.AfterEach;
3635
import org.junit.jupiter.api.BeforeAll;
@@ -42,6 +41,7 @@
4241
import reactor.test.StepVerifier;
4342

4443
import static org.assertj.core.api.Assertions.assertThat;
44+
import static org.awaitility.Awaitility.await;
4545

4646
@Timeout(15)
4747
class HttpServletStreamableIntegrationTests extends AbstractMcpClientServerIntegrationTests {
@@ -57,6 +57,16 @@ class HttpServletStreamableIntegrationTests extends AbstractMcpClientServerInteg
5757

5858
private HttpServletStreamableServerTransportProvider mcpServerTransportProvider;
5959

60+
@Override
61+
protected void awaitClientStreamEstablished() {
62+
var timeout = Duration.ofSeconds(1);
63+
await().atMost(timeout).untilAsserted(() -> {
64+
assertThat(MCP_SERVLET.isStreamEstablished())
65+
.withFailMessage("[Failed to observe MCP Client connection within %s]", timeout)
66+
.isTrue();
67+
});
68+
}
69+
6070
static Stream<Arguments> clientsForTesting() {
6171
return Stream.of(Arguments.of("httpclient"));
6272
}
@@ -151,7 +161,7 @@ void testMissingHandlerReturnsMethodNotFoundError() {
151161
.verifyComplete();
152162

153163
// Wait until we've received the response
154-
Awaitility.await().atMost(Duration.ofSeconds(1)).until(() -> response.get() != null);
164+
await().atMost(Duration.ofSeconds(1)).until(() -> response.get() != null);
155165

156166
assertThat(response.get().error().code()).isEqualTo(McpSchema.ErrorCodes.METHOD_NOT_FOUND);
157167
assertThat(response.get().error().message()).isEqualTo("Method not found: foo/bar");

mcp-test/src/test/java/io/modelcontextprotocol/server/transport/TomcatTestUtil.java

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,14 +7,17 @@
77
import java.io.IOException;
88
import java.net.InetSocketAddress;
99
import java.net.ServerSocket;
10+
import java.util.concurrent.atomic.AtomicBoolean;
1011

1112
import jakarta.servlet.Filter;
1213
import jakarta.servlet.Servlet;
1314
import jakarta.servlet.ServletConfig;
1415
import jakarta.servlet.ServletException;
1516
import jakarta.servlet.ServletRequest;
1617
import jakarta.servlet.ServletResponse;
18+
import jakarta.servlet.http.HttpServletRequest;
1719
import org.apache.catalina.Context;
20+
import org.apache.catalina.Wrapper;
1821
import org.apache.catalina.startup.Tomcat;
1922
import org.apache.tomcat.util.descriptor.web.FilterDef;
2023
import org.apache.tomcat.util.descriptor.web.FilterMap;
@@ -41,7 +44,7 @@ public static Tomcat createTomcatServer(String contextPath, int port, Servlet se
4144
Context context = tomcat.addContext(contextPath, baseDir);
4245

4346
// Add transport servlet to Tomcat
44-
org.apache.catalina.Wrapper wrapper = context.createWrapper();
47+
Wrapper wrapper = context.createWrapper();
4548
wrapper.setName("mcpServlet");
4649
wrapper.setServlet(servlet);
4750
wrapper.setLoadOnStartup(1);
@@ -78,12 +81,18 @@ public static class DelegatingServlet implements Servlet {
7881

7982
private volatile Servlet delegate;
8083

84+
// Crude way of tracking whether a GET SSE stream has been
85+
// established, to ensure a Streamable HTTP MCP Client is
86+
// connected.
87+
private final AtomicBoolean sseStreamEstablished = new AtomicBoolean(false);
88+
8189
/**
8290
* Sets the servlet handling subsequent requests. The delegate is not
8391
* {@link Servlet#init(ServletConfig) initialized}, since the MCP servlet
8492
* transports do not rely on their {@link ServletConfig}.
8593
*/
8694
public void setDelegate(Servlet delegate) {
95+
this.sseStreamEstablished.set(false);
8796
this.delegate = delegate;
8897
}
8998

@@ -104,6 +113,9 @@ public void service(ServletRequest request, ServletResponse response) throws Ser
104113
throw new IllegalStateException("No delegate servlet has been set");
105114
}
106115
current.service(request, response);
116+
if (request instanceof HttpServletRequest req && req.getMethod().equals("GET")) {
117+
sseStreamEstablished.set(true);
118+
}
107119
}
108120

109121
@Override
@@ -116,6 +128,10 @@ public void destroy() {
116128
this.delegate = null;
117129
}
118130

131+
public boolean isStreamEstablished() {
132+
return this.sseStreamEstablished.get();
133+
}
134+
119135
}
120136

121137
/**

0 commit comments

Comments
 (0)