test(mcp): cover last-event-id 404 guard; remove dead replay code - #3253
test(mcp): cover last-event-id 404 guard; remove dead replay code#3253arimu1 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Adjusts MCP SSE GET handling to explicitly reject unsupported SSE resume via last-event-id, preventing hanging connections and ensuring clients re-initialize properly.
Changes:
- Return 404 immediately when
last-event-idis present on GET SSE requests. - Remove the unreachable/hang-prone replay branch in GET handling.
- Add EmbeddedChannel regression tests covering
last-event-idandLast-Event-IDheader behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| arthas-mcp-server/src/main/java/com/taobao/arthas/mcp/server/protocol/server/handler/McpStreamableHttpRequestHandler.java | Rejects unsupported last-event-id resume up-front and simplifies GET SSE handling by removing replay logic. |
| arthas-mcp-server/src/test/java/com/taobao/arthas/mcp/server/protocol/server/handler/McpStreamableHttpRequestHandlerTest.java | Adds regression tests to ensure last-event-id triggers immediate 404 and the connection doesn’t hang. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| private static EmbeddedChannel newChannel(McpStreamableHttpRequestHandler handler) { | ||
| return new EmbeddedChannel(new SimpleChannelInboundHandler<FullHttpRequest>(false) { | ||
| @Override | ||
| protected void channelRead0(ChannelHandlerContext ctx, FullHttpRequest request) throws Exception { | ||
| handler.handle(ctx, request); | ||
| } | ||
| }); | ||
| } |
| // Must not leave a hanging SSE stream open for the client to time out on. | ||
| assertThat((Object) channel.readOutbound()).isNull(); | ||
| assertThat(channel.isActive()).isFalse(); | ||
| channel.finishAndReleaseAll(); |
| * Handles GET requests to establish SSE listening streams. | ||
| * <p> | ||
| * Resume via {@code last-event-id} is not supported. Clients such as Cherry Studio may | ||
| * reconnect with that header after the server closes a completed streamable response; | ||
| * answering immediately with 404 lets them re-initialize instead of hanging until timeout | ||
| * (see #3118). |
hengyunabc
left a comment
hengyunabc
left a comment
There was a problem hiding this comment.
这里的回归测试在 PR 父提交 a960592c7 上也会通过，因此目前无法证明这个 PR 修复了 #3118。
handleGetRequest 中 last-event-id 直接返回 404 的 guard 已由 c8c585d0d 引入，并且在本 PR 的 merge-base 中已经存在；本 PR 的生产代码实际只删除了该 guard 之后不可达的 replay 分支。两个新增测试也都只验证这个既有 guard。
如果 #3118 目前仍可复现，请先补充一个在父提交上失败、覆盖真实 Netty 请求入口的回归测试，再修复实际未返回 404 的请求链路。如果问题已经由 c8c585d0d 修复，请将本 PR 明确调整为既有修复的测试补充和死代码清理（例如 test/refactor(mcp)），不要再声明由此 PR 修复 #3118。
另外，目标分支位于 session 查找之前，测试可以直接使用任意 session ID，无需搭建完整初始化流程和 StubCommandExecutor。
本地在最新 master 合并结果上执行 ./mvnw -pl arthas-mcp-server test，15 个测试全部通过。
arimu1
changed the title
16226c0 to
1f78131
Compare
|
Thanks for the review — agreed. I confirmed the Updated accordingly:
Please take another look when you have time. |
The immediate 404 for unsupported last-event-id resume was already added in c8c585d (alibaba#3118). This change adds EmbeddedChannel regression tests for that guard and removes the unreachable replay branch after it. Relates-to alibaba#3118
1f78131 to
85f1c52
Compare
Summary
The immediate 404 when
last-event-idis present on GET SSE requests was already fixed inc8c585d0d(#3118). This PR does not re-fix that behavior.This change:
last-event-id/Last-Event-IDguard (runs before session lookup, so tests use an arbitrary session id)Relates-to #3118
Verification
./mvnw -pl arthas-mcp-server testResult: 15 tests passed (including 2 new handler tests).