[konlue]

Fixes #8045.

Summary

With a custom message endpoint (e.g. /rpc), the SSE connect creates the transport under the canonical server path and advertises /rpc?sessionId=... — but the subsequent POST of that URI was looked up by its full request URI. normalizeServerPath only strips /message, /sse and /streamablehttp, so /mcp/rpc created a second transport whose session map cannot know the SSE session: the client gets 404 Session not found, and an extra shared server stays registered.

Modifications

• handleSseRequest resolves the transport by the cached selector's canonical server path; the message endpoint is used only to decide SSE-vs-message dispatch. A blank server path falls back to the request URI.

Verifying this change

• Real-manager end-to-end: SSE session established on /mcp with endpoint /rpc, then POST /mcp/rpc?sessionId=... — no shared server is created under /mcp/rpc and the request is not answered 404. Fails on the previous code.

[Aias00]

Review (Aias00)

Verdict: REQUEST CHANGES. The production fix is correct and I would merge it as-is — but the test edit silently disables an existing test, and that has to be fixed before this lands.

Blocker: testStreamableHttpNotificationReturnsEmptyBodyViaPluginPath no longer runs

In McpServerPluginTest, the new @Test was inserted above the existing method instead of above the new one, so the annotation now belongs to testCustomMessageEndpointKeepsSseSessionOnTheSameServer and the pre-existing testStreamableHttpNotificationReturnsEmptyBodyViaPluginPath is left as a plain void method:

    @Test
    void testCustomMessageEndpointKeepsSseSessionOnTheSameServer() {
        ...
    }

    void testStreamableHttpNotificationReturnsEmptyBodyViaPluginPath() {   // <- no @Test any more

The javadoc for the streamable-http test is also stranded: it now sits directly above the #8045 javadoc, so the two blocks stack on top of the new test.

This is invisible in CI because the total is still 12 either way. Confirmed on the PR head (307adda7): the surefire report lists testCustomMessageEndpointKeepsSseSessionOnTheSameServer but no testStreamableHttpNotificationReturnsEmptyBodyViaPluginPath, while master runs the latter. So the PR trades one regression guard for another and the check count does not move.

Fix (verified locally on top of 307adda7):

    /**
     * #8045: with a custom message endpoint, the SSE connect and the subsequent
     * JSON-RPC POST must share the same server/session, and no extra shared
     * server may be created for the endpoint path.
     */
    @Test
    void testCustomMessageEndpointKeepsSseSessionOnTheSameServer() { ... }

    /**
     * A JSON-RPC notification routed through the real plugin path must be
     * acknowledged with HTTP 202 and an empty body, not a serialized null
     * response body.
     */
    @Test
    void testStreamableHttpNotificationReturnsEmptyBodyViaPluginPath() { ... }

With that, mvn -pl shenyu-plugin/shenyu-plugin-mcp-server -am test -Dtest=McpServerPluginTest runs 13 tests, 0 failures (12 + the new one), so both guards are live.

The production change itself is right

• Root cause confirmed: ShenyuMcpServerManager.normalizeServerPath only strips the hard-coded /message, /sse and /streamablehttp suffixes (lines 516-518), so a custom endpoint such as /rpc survives normalization. A POST to /mcp/rpc therefore used to key the lookup on /mcp/rpc, create a second shared server with an empty session map, and answer 404 for the SSE session established under /mcp. Issue #8045 is exactly that.
• Keying on server.getPath() is the right identity: it is stable across the SSE connect and the message POST, both of which flow through this same handleSseRequest call site, and it still normalizes identically for the common cases (/mcp/** → /mcp, /mcp/sse → /mcp), so no existing deployment changes bucket.
• StringUtils.isNotBlank(...) ? server.getPath() : uri is a sensible fallback for servers configured without a path.

Test strength (once the annotation is restored)

I reverted only the production hunk and re-ran the module: testCustomMessageEndpointKeepsSseSessionOnTheSameServer fails with

AssertionFailedError: no extra shared server may be created under the message endpoint path
  ==> expected: <false> but was: <true>

so the new test genuinely pins the fix rather than passing vacuously. Good.

Nits (non-blocking)

1. assertNotEquals(HttpStatus.NOT_FOUND, messageExchange.getResponse().getStatusCode()) is the weaker of the two assertions: on the success path the status is null and assertNotEquals passes for null too, so a request that produced no response at all would still satisfy it. The hasMcpServer assertion is doing the real work; consider either dropping this one or tightening it to assertNull(...) / an explicit "session handle was invoked" check, so the comment and the assertion say the same thing.
2. Import placement is not alphabetical in either file — org.apache.commons.lang3.StringUtils lands between two org.apache.shenyu.common.* imports in McpServerPlugin, and in the test org.springframework.web.reactive.function.server.ServerRequest / ShenyuSseServerTransportProvider / reactor.core.Disposable are appended after the existing block. checkstyle:check passes (no ImportOrder rule), so this is cosmetic only.
3. org.awaitility.Awaitility.await() is used fully qualified while Disposable, Objects, Duration are imported. Prefer a static import or a normal import for consistency. awaitility comes in via the root pom.xml <dependencies>, so no module-level declaration is needed.
4. commons-lang3 is not declared in shenyu-plugin-mcp-server/pom.xml either, but three other classes in this module already import it, so this PR is not making that situation worse — worth a separate cleanup, not a blocker here.

CI

All checks green on 307adda7: ci / build / build (shenyu-integrated-test-http) / build (shenyu-integrated-test-k8s-gateway-api-http) / CodeQL Analyze (java) / pr_build / rat_check / check-license-header / registry_dependencies all pass. Green CI is exactly why the missing @Test slipped through — it changes no counts.

[konlue]

Fixed — you were right, and the annotation misplacement was entirely my slip: the streamable-http test had lost its @Test and was silently skipped while the count stayed at 12.

The javadoc blocks are back with their own tests, and mvn -Dtest=McpServerPluginTest now runs 13 tests, 0 failures — both guards live.

Also took the nits:

• The weak assertNotEquals is now assertNull on the status (matching the comment about the SSE-stream answer).
• Imports are alphabetical in both files.
• Awaitility is properly imported instead of fully qualified.

Thanks for verifying the fix reverts to a genuine failure — the new test does pin the behavior.

[Aias00]

The displaced @Test is back: testStreamableHttpNotificationReturnsEmptyBodyViaPluginPath carries its annotation again at line 307, so that case runs once more. That was my only blocker.

On the fix itself — keying the transport by the selector's canonical path instead of the request URI is the right identity. normalizeServerPath cannot strip a custom endpoint such as /rpc, so getOrCreateMcpServerTransport(uri, ...) was creating a second provider for the same logical server, and its session map had never seen the SSE session. Using server.getPath() also makes the lookup agree with the key that McpServerPluginDataHandler uses when it registers and removes a server, which is what makes #8086's teardown correct as well. The isNotBlank fallback to uri keeps the old behaviour for a server without a configured path.

The new test is the right shape too: it drives a real SSE request, reads the endpoint out of the stream and then posts to it, rather than asserting on the manager call. That is the only way this regression would have been caught.