fix(core): tolerate HTTP 404 on optional Streamable HTTP GET SSE stream - #8785
fix(core): tolerate HTTP 404 on optional Streamable HTTP GET SSE stream#8785kenshin1986 wants to merge 2 commits into
Conversation
PR QwenLM#4521 taught the Streamable HTTP compatibility fetch to treat a 400 response to the optional standalone GET/SSE notification stream as "unsupported" (Spring AI's behavior). Spec-compliant servers commonly use 404 instead — confirmed against mcp.context7.com/mcp and api.githubcopilot.com/mcp/, both third-party servers unrelated to any one implementation — and that status isn't covered by the existing Set, so the GET SSE failure propagates as a fatal SseError and the whole MCP server connection is torn down instead of continuing with POST-only communication. Extends STREAMABLE_HTTP_GET_SSE_FALLBACK_STATUSES to include 404, following the same pattern as QwenLM#4521. Deliberately does not add 405: it's already the transport's own native "unsupported" sentinel handled elsewhere, and an existing test (`does not rewrite the SDK-native GET SSE unsupported sentinel`) asserts a raw 405 response passes through unmodified with its original body — adding it to this Set would rewrite that response instead.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Thanks for the PR, @kenshin1986 — the underlying fix looks well motivated (issue #8784 carries a solid reproduction), but the PR description doesn't follow the PR template, so it can't go through review as-is.
What's missing:
## Reviewer Test Planwith itsHow to verify/Evidence (Before & After)/Tested onsubsections — your test-plan checkboxes already carry the right content, just move them into this structure## Risk & Scope## Linked Issues— theFixes #8784line belongs here- The Chinese translation in the
<details>block
The content is already there; it just needs the template's shape. Please edit the PR description to fill in those sections, and triage will pick it up again on the next pass.
中文说明
感谢提交 PR,@kenshin1986——这个修复本身动机充分(issue #8784 提供了完整的复现),但 PR 描述没有遵循 PR 模板,因此暂时无法进入评审流程。
缺少的部分:
## Reviewer Test Plan及其How to verify/Evidence (Before & After)/Tested on子章节——你现有的测试计划内容已经足够,搬进这个结构即可## Risk & Scope## Linked Issues——Fixes #8784应放在这一节<details>块中的中文翻译
内容你已经写好了,只需要按模板重新组织。请编辑 PR 描述补全这些章节,下一轮 triage 会继续处理。
— Qwen Code · qwen3.8-max
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — the 'verify' CI check was skipped at this commit and its deep-verification suite did not run locally.
Not reviewed: reverse audit — stopped before round 4 by the review time budget.
Test Plan (not a blocker): src/tools/mcp-client.test.ts — no such file or directory; 113 passing — this review observed 19547, 1124, 18604, 1470, 481, 2941, 454 passed.
— qwen3.8-max via Qwen Code /review (v0.21.8)
| const STREAMABLE_HTTP_GET_SSE_FALLBACK_STATUSES = new Set([400, 404]); | ||
| const STREAMABLE_HTTP_GET_SSE_ERROR_BODY_LIMIT = 512; |
There was a problem hiding this comment.
[Critical] Adding 404 to this set routes 404 response bodies through readResponseBodyExcerpt, which awaits reader.read() with no timer or AbortSignal, on a fetch path whose MCP undici dispatcher runs with headersTimeout: 0 and bodyTimeout: 0 (getOrCreateMcpDispatcher in runtimeFetchOptions.ts), and nothing times transport.start()/client.connect() — a 404 whose body never completes now hangs MCP connect() indefinitely. Pre-diff, the SDK called response.body?.cancel() without reading and threw immediately. — Failure scenario: a server or intervening proxy answers the optional GET SSE request with 404 headers and a chunked body that never sends a chunk, holding the connection open → post-diff the wrapper matches 404 and awaits up to 512 bytes forever: connect() hangs indefinitely, the server stays CONNECTING, and the socket plus pending connect promise leak with no error ever surfaced (the configured MCP timeout never applies because initialize never starts). Confirmed by live probe: the PR arm hung the full observation window, a one-line revert arm resolved in 5 ms, and a bounded-excerpt arm resolved in ~2 s while still returning the intended synthetic 405. Caveat measured the same way: a naive outer Promise.race around readResponseBodyExcerpt does NOT fix it — after the race times out, the wrapper's subsequent response.body?.cancel() blocks, because cancelling one branch of the tee'd (.clone()d) body waits on the branch still held by the excerpt reader. The fix must cancel the excerpt's own reader or plumb an AbortSignal.timeout(...) through the read, e.g. inside the fallback branch:
const excerptReader = response.body?.getReader();
try {
responseBody = await Promise.race([
readResponseBodyExcerptFrom(excerptReader),
new Promise<undefined>((resolve) =>
setTimeout(() => resolve(undefined), 5_000).unref(),
),
]);
} finally {
await excerptReader?.cancel().catch(() => {});
}(The 400 path shares the latent hole; this diff newly exposes 404 bodies to it — which this PR's own test comment identifies as the common rejection shape.)
— qwen3.8-max via Qwen Code /review (v0.21.8)
| } | ||
|
|
||
| const STREAMABLE_HTTP_GET_SSE_FALLBACK_STATUSES = new Set([400]); | ||
| const STREAMABLE_HTTP_GET_SSE_FALLBACK_STATUSES = new Set([400, 404]); |
There was a problem hiding this comment.
[Suggestion] The JSDoc on createStreamableHttpCompatibilityFetch (lines 221–223) still says the wrapper exists "to normalize Spring AI-style 400 responses to the SDK's unsupported sentinel", but this diff extends the fallback set to also include 404, so the stated rationale now describes only half the behavior — and that same docblock carries the SDK-lockstep maintenance instruction. — Concrete cost: during the SDK-lockstep update the doc comment itself anticipates, a maintainer reads a rationale enumerating only 400/Spring AI and may treat the undocumented 404 entry as vestigial and drop it (or bolt a new special case on at a transport-construction site instead of extending the set), silently re-breaking every server that rejects the optional GET route with 404. Suggested fix: widen the sentence, e.g.
* Wraps fetch to preserve OAuth challenges before the SDK discards response
* metadata and to normalize known non-conformant rejections of the optional
* Streamable HTTP GET SSE request (Spring AI's 400, and 404 from servers with
* no GET route) to the SDK's unsupported sentinel.— qwen3.8-max via Qwen Code /review (v0.21.8)
| // https://mcp.context7.com/mcp and api.githubcopilot.com/mcp/ both | ||
| // do this, so this isn't specific to any one server implementation. |
There was a problem hiding this comment.
[Suggestion] This comment attributes the 404-on-GET behavior to mcp.context7.com and api.githubcopilot.com, but the linked issue's own record retracts both attributions — the issue's curl evidence shows context7 returning 405, and the reporter's correction comment calls the context7/GitHub Copilot repro "a false positive from my own machine's misconfiguration" (context7's raw 405 is tolerated natively by the SDK). A maintainer has already flagged this alignment nit in the issue thread. — Concrete cost: a future maintainer auditing this fallback probes context7, observes 405, and wrongly concludes the 404 path is dead code — the committed comment enshrines a claim the issue record withdrew.
| // https://mcp.context7.com/mcp and api.githubcopilot.com/mcp/ both | |
| // do this, so this isn't specific to any one server implementation. | |
| // Servers built on the official TypeScript SDK's stateless | |
| // StreamableHTTPServerTransport example pattern (no GET /mcp route | |
| // registered, e.g. Express's default fall-through) commonly do this, | |
| // so this isn't specific to any one server implementation. |
— qwen3.8-max via Qwen Code /review (v0.21.8)
| // do this, so this isn't specific to any one server implementation. | ||
| const fetchFn = vi | ||
| .fn<typeof fetch>() | ||
| .mockResolvedValue(new Response('not found', { status: 404 })); |
There was a problem hiding this comment.
[Suggestion] The new 404 fallback status has a positive test only — no negative pass-through test pins that a 404 on a non-GET-SSE request (POST tool call, plain GET) is left un-rewritten. All existing guard tests here ("does not rewrite POST responses", "does not rewrite non-SSE GET responses", "does not rewrite resumable GET SSE errors with Last-Event-ID") use status 400. — Failure scenario: a future refactor of the wrapper condition that evaluates the status before the request shape, or special-cases 404, would rewrite every 404 — including a real MCP POST response (e.g. a server returning 404 for a missing resource) — into an empty synthetic 405, discarding the server body and changing how the SDK classifies the error. Measured by mutation probe: an always-rewrite-404 mutant survives the current 113-test suite but fails against a POST+404 negative probe. Suggested addition, mirroring the existing 400 guard tests:
it('does not rewrite POST 404 responses', async () => {
const fetchFn = vi
.fn<typeof fetch>()
.mockResolvedValue(new Response('not found', { status: 404 }));
const fetchWithFallback = createStreamableHttpCompatibilityFetch(
'post-404',
fetchFn,
);
const response = await fetchWithFallback('http://test-server/mcp', {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
});
expect(response.status).toBe(404);
});— qwen3.8-max via Qwen Code /review (v0.21.8)
…probe (QwenLM#10091) After the mandatory POST-based MCP handshake succeeds, the SDK also probes an optional standalone GET <mcp-url> (Accept: text/event-stream) for a server-push notification stream. A server that rejects that optional probe with 404 instead of 400 (very common: it's what Express's default fallthrough gives an unhandled GET route, including the official SDK's own documented stateless StreamableHTTPServerTransport pattern) currently tears down the whole MCP connection, even though the mandatory POST path works fine. STREAMABLE_HTTP_GET_SSE_FALLBACK_STATUSES already tolerates 400 for this same reason (QwenLM#4521, Spring AI). Extend it to 404, deliberately leaving 405 out (the SDK's _startOrAuthSse already short-circuits on a raw 405) and 401 out (the OAuth challenge-capture path depends on observing it). Bound readResponseBodyExcerpt's body read with the file's existing runWithTimeout idiom (already used by disconnect()'s terminateSession call for the same headersTimeout:0/bodyTimeout:0 dispatcher reason): a server that sends 404 headers and then never completes the body would otherwise park the diagnostics read forever, since nothing above this wrapper ever times out the connection attempt. On timeout the reader is cancelled so the abandoned read settles instead of leaking; this also hardens the pre-existing, previously-untimed 400 path. Supersedes stalled PR QwenLM#8785 (kenshin1986), which implemented the same Set change but left the body read unbounded -- the repo's own review bot caught that as a Critical and the PR went unaddressed for 17 days. Credit to QwenLM#8785 for the Set change and initial test; the timeout fix, doc correction, and two of the four tests here are new. Also corrects a factual claim in the original PR's test comment (attributing the 404 behavior to two public servers) that the issue thread itself retracted as a reporter-side misconfiguration. Refs QwenLM#8784, QwenLM#8785 Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
What
Extends
STREAMABLE_HTTP_GET_SSE_FALLBACK_STATUSESinpackages/core/src/tools/mcp-client.tsto also tolerate404, not just400.Why
Fixes #8784.
#4521 taught the Streamable HTTP compatibility fetch to treat a
400response to the optional standalone GET/SSE notification stream as "unsupported" (Spring AI's specific behavior)._startOrAuthSseon its own already natively tolerates a raw405(if (status === 405) return;), so servers that reject the optional GET that way (e.g.mcp.context7.com) work fine as-is, noSetinvolvement needed. But a server that instead returns404for that same optional GET isn't covered by either path, so the rejection propagates as a fatalSseError/StreamableHTTPErrorand the whole MCP connection is torn down — even though the mandatory POST-based communication works fine and the server is fully spec-compliant.404here isn't exotic — it's what you get from the official SDK's own documented statelessStreamableHTTPServerTransportpattern (a plainapp.post('/mcp', ...)handler, no explicitGET/DELETEhandlers) the moment a client probesGET /mcp: Express's own unhandled-route fallthrough. Confirmed locally with exactly that server shape (before I added an explicitGEThandler to it as a workaround) — repro'd throughStreamableHTTPClientTransportin isolation (tracedcreateTransportto confirm the Streamable HTTP transport, not the legacy SSE one, was selected).Deliberately does not add
405to the set: it's already the transport's own native "unsupported" sentinel (handled directly in_startOrAuthSse, unrelated to this fetch wrapper), and an existing test —does not rewrite the SDK-native GET SSE unsupported sentinel— asserts a raw405response passes through unmodified with its original body. Adding405to this set would rewrite that response instead and break that guarantee.(Edit: earlier revisions of this PR/the linked issue also claimed
405from context7/GitHub Copilot's MCP servers hits this same failure — that was a false positive from a local~/.qwen/settings.jsonmisconfiguration on my end forcing those connections through the legacySSEClientTransportinstead ofStreamableHTTPClientTransport, unrelated to this code path. See the correction on #8784. The404case below is the real, isolated repro.)Test plan
npx vitest run src/tools/mcp-client.test.ts— 113/113 passing (112 existing + 1 new:treats 404 from optional GET SSE stream as unsupported, modeled on the existingtreats 400 ...test)npx tsc --noEmit -p packages/core/tsconfig.jsoncleannpx eslint packages/core/src/tools/mcp-client.ts packages/core/src/tools/mcp-client.test.tscleanQWEN_HOME(only thehttpUrl-based config, no settings.json override) against a local Streamable-HTTP-only server returning404for the optional GET