Fix : Address McpRequestTracker issue #33168
Open
+116
−100
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Azquelt
requested changes
Oct 20, 2025
Comment on lines
2114
to
2151
| @Test | ||
| public void testDuplicateRequestIdReturnsError() throws Exception { | ||
|
|
||
| String requestTemplate = """ | ||
| { | ||
| "jsonrpc": "2.0", | ||
| "id": "%s", | ||
| "method": "tools/call", | ||
| "params": { | ||
| "name": "echo", | ||
| "arguments": { | ||
| "input": "Hello" | ||
| } | ||
| } | ||
| } | ||
| """; | ||
|
|
||
| // First request call | ||
| new HttpRequest(server, "/toolTest/mcp").requestProp("Accept", ACCEPT_HEADER) | ||
| .requestProp(MCP_PROTOCOL_HEADER, MCP_PROTOCOL_VERSION) | ||
| .requestProp("Mcp-Session-Id", client.getSessionId()) | ||
| .jsonBody(requestTemplate) | ||
| .method("POST") | ||
| .expectCode(200) | ||
| .run(String.class); | ||
|
|
||
| // Second request - same ID while the first is likely still active | ||
| String duplicateResponse = new HttpRequest(server, "/toolTest/mcp").requestProp("Accept", ACCEPT_HEADER) | ||
| .requestProp(MCP_PROTOCOL_HEADER, MCP_PROTOCOL_VERSION) | ||
| .requestProp("Mcp-Session-Id", client.getSessionId()) | ||
| .jsonBody(requestTemplate) | ||
| .method("POST") | ||
| .expectCode(200) | ||
| .run(String.class); | ||
|
|
||
| assertThat("Expected error for duplicate request ID", duplicateResponse, containsString("\"error\"")); | ||
| assertThat("Expected specific error code for duplicate request ID", duplicateResponse, containsString("CWMCM0009E")); | ||
| } |
There was a problem hiding this comment.
A few things confuse me about this test:
requestTemplatelooks like it should have an id substituted in place of the%sbut it doesn't. This doesn't actually break the test since"%s"is a valid identifier and it is used in both requests.HttpRequest.runwaits for the response and returns it, so the first request should always be completed before the second one runs. Given that, I'm not sure how this test passes. It suggests we may not be removing the completed request.- It does mean that this test (non-concurrent requests with the same ID) is probably a good one to have, since it should catch if we're not cleaning up correctly, we just also need a test which actually does concurrent requests.
- To test concurrent requests, you will need to do something like the cancellation tests.
- I'm not sure why this test doesn't use
McpClient.callMcp- it doesn't appear to be doing anything special? - When checking the for the error code, please also check for the id which should be included in the message.
dev/io.openliberty.mcp.internal/src/io/openliberty/mcp/internal/McpServlet.java
Outdated
Show resolved
Hide resolved
dev/io.openliberty.mcp.internal/src/io/openliberty/mcp/internal/McpServlet.java
Outdated
Show resolved
Hide resolved
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
release buglabel if applicable: https://github.com/OpenLiberty/open-liberty/wiki/Open-Liberty-Conventions).McpConnectionTrackerissues #33101".