Copilot commented on code in PR #7046:
URL: https://github.com/apache/shenyu/pull/7046#discussion_r4032641542
##########
shenyu-plugin/shenyu-plugin-mcp-server/src/main/java/org/apache/shenyu/plugin/mcp/server/transport/ShenyuStreamableHttpServerTransportProvider.java:
##########
@@ -542,11 +542,11 @@ private Mono<MessageHandlingResult>
processWithExistingSession(final McpServerSe
.doOnSuccess(result -> LOGGER.debug("Successfully processed
message for session: {}", sessionId))
.then(waitForTransportResponse(transport, sessionId,
messageId))
.doOnNext(result -> {
- // Clear the captured response after each completed
message so that a
- // subsequent message on this session cannot observe a
stale response
- // from a previous request.
+ // Clear the response captured for this specific message
id after it has
+ // been delivered, so that a subsequent message on this
session cannot
+ // observe a stale response from a previous request.
if (Objects.nonNull(transport)) {
Review Comment:
`resetCapturedMessage(messageId)` is executed in `doOnNext`, which only runs
on success. If `session.handle(message)` errors after the transport captured a
response, the per-id entry can leak and be incorrectly returned by a later
request that reuses the same JSON-RPC id. Move the cleanup to `doFinally` (or
`doOnTerminate`) so it runs for both success and error signals.
##########
shenyu-plugin/shenyu-plugin-mcp-server/src/main/java/org/apache/shenyu/plugin/mcp/server/transport/ShenyuStreamableHttpServerTransportProvider.java:
##########
@@ -1074,6 +1081,20 @@ public McpSchema.JSONRPCMessage getLastSentMessage() {
return lastSentMessage;
}
+ /**
+ * Gets the response message captured for the given message id,
falling back
+ * to the last sent message when no id-based correlation is available.
+ *
+ * @param messageId the JSON-RPC message id to look up
+ * @return the correlated response, or null if none has been captured
+ */
+ public McpSchema.JSONRPCMessage getLastSentMessage(final Object
messageId) {
+ if (Objects.nonNull(messageId)) {
+ return messageResponses.get(String.valueOf(messageId));
+ }
+ return lastSentMessage;
Review Comment:
`messageResponses` uses `String.valueOf(messageId)` as the correlation key.
This can conflate distinct JSON-RPC ids like numeric `1` vs string `"1"`,
allowing concurrent in-flight calls to overwrite each other again. Consider
keying by `(type, value)` (e.g., prefix with the id type) or using an `Object`
key map and consistently applying the same keying in put/get/remove.
##########
shenyu-plugin/shenyu-plugin-mcp-server/src/main/java/org/apache/shenyu/plugin/mcp/server/transport/ShenyuStreamableHttpServerTransportProvider.java:
##########
@@ -824,7 +824,12 @@ private Mono<MessageHandlingResult>
waitForTransportResponse(final StreamableHtt
final String
sessionId,
final Object
messageId) {
return Mono.fromCallable(() -> {
- if (Objects.nonNull(transport) && transport.isResponseReady() &&
Objects.nonNull(transport.getLastSentMessage())) {
+ final McpSchema.JSONRPCMessage correlatedResponse =
Objects.nonNull(transport)
+ ? transport.getLastSentMessage(messageId) : null;
+ if (Objects.nonNull(messageId) &&
Objects.nonNull(correlatedResponse)) {
+ LOGGER.debug("Retrieved correlated response for message id {}
on session: {}", messageId, sessionId);
+ return new MessageHandlingResult(200, correlatedResponse,
sessionId);
+ } else if (Objects.nonNull(transport) &&
transport.isResponseReady() && Objects.nonNull(transport.getLastSentMessage()))
{
Review Comment:
The new per-message-id correlation logic is not exercised by existing tests;
`ShenyuStreamableHttpServerTransportProviderTest` currently covers notification
handling but not concurrent (or interleaved) in-flight requests on the same
session. Please add a regression test that issues multiple parallel `tools/*`
requests with distinct ids and asserts each HTTP response contains the matching
id/body and that no cross-delivery occurs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
