fix(mcp): use a dedicated undici fetch for Streamable HTTP transports - #7195
Conversation
Verification reportUnit + contract tests (packages/core):
Static checks: Stall reproduction matrix (real
None trigger the stall locally — the failure needs something the Fastmail endpoint does that a synthetic server doesn't (h2-related negotiation is the prime suspect; Node's fetch is h1-only but the connection setup differs from npm undici's). Therefore the decisive before/after evidence is @imrehg's bisection on the affected environment (comment): swapping only the fetch to npm undici fixes it; suppressing only the GET stream also fixes it; stock build times out at the SDK's 60 s. This PR lands the first (spec-preserving) direction. Stated plainly: the tests above pin the plumbing, not the stall itself. Review focus suggestions: (1) the wrapper must pass 中文版本单测 + 契约测试(packages/core): 静态检查:typecheck / eslint / prettier 全绿。 stall 复现矩阵(真实 SDK transport;上表 8 组合:Node 22.23/26.4/26.5 × HTTP/HTTPS × GET 流保持/头挂起/连接数 1)全部毫秒级完成 审阅要点:(1) 包装层除 |
|
Thanks for the PR — and for the thorough re-architecture across the review rounds. Template looks good ✓ Problem: observed bug, not theoretical. #7147 is a real failure against Fastmail's MCP endpoint — OAuth succeeds but Direction: aligned. Decoupling the MCP Streamable HTTP transport from Node's bundled fetch is the right fix for a stall that lives in that fetch's connection handling. CHANGELOG has no direct reference, but MCP transport reliability is squarely in scope. Size: touches core ( Approach: scope feels right. The re-architecture is an improvement over the original standalone- Moving on to code review. 🔍 中文说明感谢贡献,也感谢在多轮 review 中的彻底重构。 模板完整 ✓ 问题: 已观测到的 bug,非理论性问题。#7147 是针对 Fastmail MCP 端点的真实故障——OAuth 成功但 方向: 对齐。把 MCP Streamable HTTP transport 与 Node 内置 fetch 解耦,是针对该 fetch 连接处理中 stall 的正确修复。CHANGELOG 无直接引用,但 MCP transport 可靠性完全在范围内。 规模: 触及核心( 方案: 范围合理。相比最初的独立 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code reviewMy independent proposal for this bug — pin undici's own
No correctness, security, or convention blockers found. One theoretical, harmless note (not blocking): if an MCP connection completed before Checks (run in an isolated worktree at the reviewed commit): Real-scenario testing
The stall itself does not reproduce here — it needs the specific server × Node-26.4 combination from #7147, and this runner is Node 22.23.1 with a local server (the PR documents this exact limitation). So before == after on this environment; the decisive evidence for the stall remains the reporter's bisection plus the maintainer's real-environment verification. What I could verify is that the new dedicated-fetch transport works end to end in the real CLI. After (this PR —
|
|
Confidence: 5/5 — clean across every stage; the evidence chain is complete and the fix is exactly the minimal, well-integrated change this bug needs. Stepping back: this is a real, bisected bug with a complete evidence chain — mechanism (the reporter's two-way bisection pinning it to built-in fetch + the held SSE stream), plumbing (the contract test proves real requests flow through the dedicated dispatcher; my headless run shows discovery and a tool call working through it in the real CLI), and outcome (the reporter and a maintainer both confirmed the fix in the actual affected Fastmail environment). That's about as solid as a fix for an environment-specific stall gets. The implementation matches my independent proposal and reuses the existing dispatcher/cache/proxy/TLS infrastructure rather than adding a parallel one — the re-architecture across the review rounds (lazy undici load preserving #7264, shared cache, proxy-awareness) made it strictly better. Scope is tight: five files, every edit serving the fix, no drive-by churn. Tests, lint, prettier, and typecheck are all green. The one thing I can't do is reproduce the stall locally — but that's the bug's nature, not a gap in the PR, and it's documented honestly. I'm approving on the strength of the bisection plus two independent real-environment confirmations, not because I ran out of reasons to say no. LGTM, looks ready to ship. ✅ 中文说明置信度:5/5 —— 每个阶段都干净;证据链完整,修复正是此 bug 所需的最小且集成良好的改动。 退一步看:这是一个真实的、经二分的 bug,证据链完整——机制(报告者双向二分,钉在内置 fetch + 长驻 SSE 流上)、管线(契约测试证明真实请求经专用 dispatcher;我的 headless 运行显示真实 CLI 中经它完成发现与工具调用)、结果(报告者与维护者都在真实受影响的 Fastmail 环境确认修复)。对于一个环境特定的 stall,这已经是最扎实的证据。 实现与我的独立方案一致,并复用既有 dispatcher/缓存/代理/TLS 基础设施而非另起炉灶——多轮 review 中的重构(保留 #7264 的 undici 懒加载、共享缓存、代理感知)使其严格更优。范围紧凑:五个文件,每处改动都服务于修复,无顺手夹带。测试、lint、prettier、typecheck 全绿。 唯一无法做的是本地复现 stall——但那是 bug 的本质,不是 PR 的缺陷,且已如实记录。我基于二分加上两次独立的真实环境确认而批准,不是因为找不到反对理由。 LGTM,可以发布。✅ — Qwen Code · qwen3.8-max-preview Reviewed at |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
| const transportFetch = createStreamableHttpCompatibilityFetch( | ||
| 'undici-contract', | ||
| undefined, |
There was a problem hiding this comment.
[Suggestion] Dead transportFetch variable — created with createStreamableHttpCompatibilityFetch('undici-contract', undefined, ...) which defaults to globalThis.fetch (the exact path this PR replaces), but never invoked. The void transportFetch at line 135 is a lint-suppression no-op.
Failure scenario: A future maintainer reading this test sees transportFetch and may assume both the default-arg path and the dedicated undici path are exercised, when only the createTransport path actually is.
| const transportFetch = createStreamableHttpCompatibilityFetch( | |
| 'undici-contract', | |
| undefined, |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
| mcpFetchDispatcher = new Agent({ | ||
| headersTimeout: 0, | ||
| bodyTimeout: 0, | ||
| keepAliveTimeout: 60_000, | ||
| ...(isTlsVerificationDisabled() | ||
| ? { connect: { rejectUnauthorized: false } } | ||
| : {}), |
There was a problem hiding this comment.
[Suggestion] The isTlsVerificationDisabled() branch in the new MCP dispatcher is untested — no test exercises the path where NODE_TLS_REJECT_UNAUTHORIZED=0 or QWEN_TLS_INSECURE=1 is set when getMcpFetchDispatcher() is first called.
Failure scenario: If someone later refactors the conditional (e.g., renames the env var or swaps the spread logic), MCP connections to servers with self-signed certificates would silently fail with TLS verification errors while the rest of the application continues working.
Consider adding a test that exercises the TLS-disabled branch, possibly with a _resetMcpFetchDispatcherForTest function to reset the singleton between tests.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Added in daa1573: honors the TLS-insecure switch for self-signed MCP endpoints plus the suggested _resetMcpFetchDispatcherForTest seam. It does a real HTTPS round trip against a local self-signed server: default dispatcher → the request rejects; QWEN_TLS_INSECURE=1 + singleton reset → the same transport fetch connects (200). QWEN_TLS_INSECURE is used deliberately instead of NODE_TLS_REJECT_UNAUTHORIZED=0: Node's own TLS layer honors the latter too, so the positive phase would pass even without the dispatcher branch. Verified the test fails when the isTlsVerificationDisabled() spread is removed. 104/104.
中文:已在 daa1573 新增该测试与建议的 _resetMcpFetchDispatcherForTest 测试缝。真实自签名 HTTPS 往返:默认 dispatcher 拒绝;设 QWEN_TLS_INSECURE=1 并重置单例后同一 transport fetch 连接成功。刻意用 QWEN_TLS_INSECURE 而非 NODE_TLS_REJECT_UNAUTHORIZED=0——后者 Node 自身 TLS 层也会尊重,正向阶段没有该分支也会通过。已验证移除 isTlsVerificationDisabled() 分支后测试失败。104/104。
d657914 to
7bafbae
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push,因为这会使已有的评审评论失效。另外,供日后参考:作为集成流程的一部分,机器人始终会自动将所有改动压缩(squash)为单个提交。 |
|
Rebased onto latest main ( 中文:已 rebase 到最新 main(无代码变化,103/103 依旧全绿)。此前 Test job 失败是 #7161 引入的 NOTICES.txt 漂移守卫在本分支旧基线上触发(CI 检出 PR head,其提交的 NOTICES.txt 早于重生成版本),与本 PR 的两个改动文件无关。 |
|
FYI, I've tested out this change locally, and seems to address the issue originally reported, in the case of the fastmail MCP provider in particular. |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. 1 Suggestion-level finding(s) could not be anchored to the diff; see the terminal output.
— qwen3.7-max via Qwen Code /review
…patcher branch Review follow-up on QwenLM#7195. The contract test carried an unused transportFetch built over the default globalThis.fetch path — removed so the test no longer implies that path is exercised. The isTlsVerificationDisabled() branch of the dedicated dispatcher is now pinned by a real self-signed HTTPS round trip: with the default dispatcher the request rejects; after setting QWEN_TLS_INSECURE=1 and resetting the singleton via the new _resetMcpFetchDispatcherForTest seam, the same transport fetch connects. QWEN_TLS_INSECURE is used deliberately — NODE_TLS_REJECT_UNAUTHORIZED=0 is also honored by Node's own TLS layer and would make the positive phase pass without the dispatcher branch. Verified the test fails when the branch is removed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Both inline suggestions addressed in daa1573: dead 中文:两条行内建议均已在 daa1573 落地——契约测试删除死变量;TLS 分支由真实自签名 HTTPS 正反对照测试固定(默认拒绝 → |
|
Reporter validation received: @imrehg tested this change against the originally-reported Fastmail MCP setup and confirms it addresses the issue (comment above). With that, the evidence chain is complete — mechanism (bisection), plumbing (tests), outcome (real-environment confirmation). 中文:报告者验证已到位——@imrehg 在原始报告的 Fastmail 环境实测本改动并确认问题解决。至此证据链完整:机制(二分)、管线(测试)、结果(真机确认)。 |
Maintainer verification — local build + real-server E2E ✅ LGTMI built this PR from source and verified it end-to-end on Linux (not just unit tests). Posting as a merge reference. Environment: Linux · Node 1) Unit + contract suite —
|
Mutation applied to mcp-client.ts |
Expected | Observed |
|---|---|---|
Remove connect:{rejectUnauthorized:false} branch from the Agent |
TLS test fails | ✅ fails — TypeError: fetch failed (self‑signed rejected even with the flag) |
Revert transport fetch → globalThis.fetch.bind(globalThis) |
bypass harness + OAuth seam test fail | ✅ both fail — harness: globalThis.fetch must NOT be used; OAuth: fetch failed (3.9s real attempt) |
Both mutations restored afterward; suite green again.
4) Live E2E through the real bundled CLI — node dist/cli.js mcp list
qwen mcp list runs the exact affected path (createTransport → createMcpStreamableHttpFetch → dedicated undici fetch), connects, and pings a real @modelcontextprotocol/sdk Streamable HTTP server.
A) Plain HTTP — reproduces the #7147 request sequence and completes:
✓ pr7195-e2e: http://127.0.0.1:<port>/mcp (http) - Connected
server saw: POST initialize
POST notifications/initialized
GET /mcp -> standalone GET SSE stream OPENED and HELD OPEN <- the #7147 trigger
POST /mcp -> ping (subsequent same-origin POST behind the held stream) -> completes
This is exactly the "long‑lived GET SSE stream + subsequent same‑origin POST" shape that stalls on the reporter's Node 26.4; here it completes cleanly (as it does on both base and PR on Node 22 — confirming no regression).
B) HTTPS self‑signed — TLS‑insecure differential (decisive):
B1) no flag : ✗ pr7195-tls: https://127.0.0.1:<port>/mcp - Disconnected
B2) QWEN_TLS_INSECURE=1 : ✓ pr7195-tls: https://127.0.0.1:<port>/mcp - Connected
Why this is decisive: Node's own global TLS layer does not read QWEN_TLS_INSECURE (only NODE_TLS_REJECT_UNAUTHORIZED). So "Connected with the flag set" can only come from the PR's isTlsVerificationDisabled() → connect.rejectUnauthorized:false branch on the dedicated Agent. This proves, live through the shipped binary, that the dedicated undici dispatcher is the real code path — and that its TLS branch works.
Notes (non‑blocking)
- Consistency: the new
getMcpFetchDispatcher()Agent mirrors the establishedruntimeFetchOptions.tspattern already used for the OpenAI/Anthropic SDKs (headersTimeout:0, bodyTimeout:0, keepAliveTimeout:60_000, connect.rejectUnauthorized). Good — MCP now behaves like the rest of the app's outbound HTTP. - Scope is honestly documented: stall not reproduced in CI; stdio/legacy‑SSE untouched;
HTTP(S)_PROXYstill not honored for streamable HTTP (unchanged behavior). Agreed. - Unrelated cosmetic nit (pre‑existing, not this PR):
qwen mcp listprints the(http)label for anhttpUrleven when it'shttps://…— that'scommands/mcp/list.ts, independent of this change.
Recommendation: merge. The fix is minimal, mirrors existing infra, is well‑tested with genuinely non‑vacuous tests, and I confirmed the dedicated‑undici path + TLS knob work live through the real CLI binary.
中文版(点击展开)
维护者验证 — 本地构建 + 真实服务器端到端 ✅ 建议合并
我从源码构建了本 PR,并在 Linux 上做了端到端验证(不止单测)。作为合并参考。
环境: Linux · Node v22.22.2 · undici@7.27.2(npm 包)· head daa1573b · 现场构建的 bundle(node dist/cli.js)。
说明:#7147 中 Node‑26.4 的卡死是环境相关的,在此无法复现——这与 PR 自己如实的说明一致。因此我的目标是证明:(a) 管线确实按声明改变;(b) 无回归;(c) TLS 开关生效——全部走真实代码路径,无 mock。
1)单测 + 契约测试 — mcp-client.test.ts
Test Files 1 passed (1)
Tests 104 passed (104)
2 个真正新增的测试使用真实回环服务器(非 stub):真实 node:http 服务器 + 真实 node:https 自签名证书。2 个 OAuth challenge 测试已正确地从 spyOn(globalThis,'fetch') 迁移到新的 _setMcpFetchForTest 测试缝。改动的两个文件 Prettier / ESLint / tsc 全部通过。
2)独立验证 — globalThis.fetch 是否真的被绕过?
PR 自带的契约测试证明请求能真实发出,但即便仍走 globalThis.fetch 也会通过(服务器是真的)。于是我加了一个 harness:故意把 globalThis.fetch 弄成抛错,再经 createTransport 构建真实 transport 并调用 transport._fetch:
✓ globalThis.fetch 被破坏时,MCP transport fetch 仍成功
· 请求经专用 undici dispatcher 成功
· 被破坏的 globalThis.fetch 被 transport 调用 0 次
→ 专用 undici 路径确实生效。这正是本修复的决定性行为主张。
3)非平凡性(变异测试)— 测试是否真的“咬住”了修复?
对 mcp-client.ts 施加的变异 |
预期 | 实测 |
|---|---|---|
删除 Agent 的 connect:{rejectUnauthorized:false} 分支 |
TLS 测试失败 | ✅ 失败 — fetch failed(带开关也拒绝自签名) |
把 transport fetch 还原为 globalThis.fetch |
harness + OAuth 缝测试失败 | ✅ 均失败 — harness:must NOT be used;OAuth:fetch failed(真实尝试 3.9s) |
之后均已还原,套件重新全绿。
4)经真实 bundle CLI 的端到端 — node dist/cli.js mcp list
qwen mcp list 走的就是受影响路径(createTransport → createMcpStreamableHttpFetch → 专用 undici fetch),连接并 ping 一个真实的 @modelcontextprotocol/sdk Streamable HTTP 服务器。
A)纯 HTTP —— 复现 #7147 的请求时序并顺利完成:
✓ pr7195-e2e: http://127.0.0.1:<port>/mcp (http) - Connected
服务器收到: POST initialize → POST notifications/initialized
GET /mcp → 打开并【长驻保持】独立 GET SSE 流 ← #7147 的触发点
POST /mcp → ping(长驻流之后的同源 POST)→ 完成
这正是“长驻 GET SSE 流 + 后续同源 POST”的形态——在报告者的 Node 26.4 上会卡死;此处顺利完成(base 与 PR 在 Node 22 上均如此,证明无回归)。
B)HTTPS 自签名 —— TLS‑insecure 差分(决定性):
B1) 无开关 : ✗ ... - Disconnected
B2) QWEN_TLS_INSECURE=1 : ✓ ... - Connected
为何决定性:Node 自身的全局 TLS 层不读 QWEN_TLS_INSECURE(只认 NODE_TLS_REJECT_UNAUTHORIZED)。因此“带开关即 Connected”只可能来自本 PR 的 isTlsVerificationDisabled() → 专用 Agent 的 connect.rejectUnauthorized:false 分支。这在已发布的二进制上实证了:专用 undici dispatcher 确为真实路径,且其 TLS 分支生效。
备注(非阻塞)
- 一致性: 新的
getMcpFetchDispatcher()与既有runtimeFetchOptions.ts(OpenAI/Anthropic SDK 已用)的模式完全一致(headersTimeout:0, bodyTimeout:0, keepAliveTimeout:60_000, connect.rejectUnauthorized)。MCP 现与其余出站 HTTP 行为统一,很好。 - 范围如实说明: 卡死无法在 CI 复现;stdio/旧 SSE 未动;streamable HTTP 仍不走
HTTP(S)_PROXY(行为不变)。认同。 - 无关的表层小瑕疵(既有、非本 PR):
qwen mcp list对httpUrl一律打印(http)标签,即使是https://…;出自commands/mcp/list.ts,与本次改动无关。
结论:建议合并。 修复很小、复用既有基础设施、测试非平凡,并已在真实 CLI 二进制上实证专用 undici 路径与 TLS 开关均生效。
|
@qwen-code /triage |
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
|
@qwen-code /resolve |
|
Qwen Code resolved the merge conflicts and pushed the branch update. Merge resolution: PR #7195 into mainRoot causePR #7279 ( Textual or semanticPurely textual. The two sides added different imports from different modules with no semantic overlap. Resolution keeps both: import {
INVOCATION_CONTEXT_META_KEY,
runWithInvocationContext,
type InvocationContextV1,
} from '../utils/invocation-context.js';
import {
_resetMcpFetchDispatcherForTest,
_setMcpFetchForTest,
addMCPStatusChangeListener,
// …rest of mcp-client named imports
} from './mcp-client.js';The What is load-bearing
What I could not verifyNo build or tests were run. The auto-merged 中文说明合并解决:PR #7195 合入 main根因main 分支上的 PR #7279( 文本冲突还是语义冲突纯文本冲突。两侧添加的是来自不同模块的不同 import,无语义重叠。解决方式保留两者: 关键约束
未验证的内容未执行构建或测试。自动合并的 |
28712db
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
| ReadResourceResultSchema, | ||
| } from '@modelcontextprotocol/sdk/types.js'; | ||
| import { parse } from 'shell-quote'; | ||
| import { Agent, fetch as undiciFetch } from 'undici'; |
There was a problem hiding this comment.
[Suggestion] Static import of undici regresses startup performance — mcp-client.ts is eagerly loaded via index.ts → export * from './tools/mcp-client.js', so this static value import forces the entire undici package to evaluate on every CLI invocation (including qwen --help, qwen --version, non-MCP sessions). runtimeFetchOptions.ts deliberately uses loadUndici() (dynamic import('undici')) to keep it out of the eager startup closure (#7264), and every other production file follows that pattern — this is the sole exception.
Concrete cost: measurable startup-time regression for all users, undoing the #7264 optimization.
| import { Agent, fetch as undiciFetch } from 'undici'; | |
| // Use loadUndici() / requireUndici() from runtimeFetchOptions.ts instead | |
| // of a static import. The MCP connection path is already async. |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in e366644 — the static import is gone; the MCP fetch now awaits loadUndici()/preloadRuntimeFetchModule() at call time, matching the #7264 pattern everywhere else, and the built mcp-client.js carries no top-level undici import (verified by grep on dist). 中文:静态 import 已移除,MCP fetch 调用时经 loadUndici/preload 惰性加载(与 #7264 模式一致),编译产物顶层无 undici 导入(dist grep 验证)。
| let mcpFetchDispatcher: Agent | undefined; | ||
| function getMcpFetchDispatcher(): Agent { | ||
| if (!mcpFetchDispatcher) { | ||
| mcpFetchDispatcher = new Agent({ | ||
| headersTimeout: 0, | ||
| bodyTimeout: 0, | ||
| keepAliveTimeout: 60_000, |
There was a problem hiding this comment.
[Suggestion] Duplicated Agent factory — getMcpFetchDispatcher() creates an Agent({headersTimeout: 0, bodyTimeout: 0, keepAliveTimeout: 60_000, ...}) identical to buildFetchOptionsWithDispatcher() in runtimeFetchOptions.ts, which caches the same config under __no_proxy__. Two independent singletons = two connection pools, two maintenance points.
Concrete cost: a keepAliveTimeout change in one file must be mirrored in the other; a silent mismatch gives MCP and LLM connections different timeout behavior for no user-visible reason.
| let mcpFetchDispatcher: Agent | undefined; | |
| function getMcpFetchDispatcher(): Agent { | |
| if (!mcpFetchDispatcher) { | |
| mcpFetchDispatcher = new Agent({ | |
| headersTimeout: 0, | |
| bodyTimeout: 0, | |
| keepAliveTimeout: 60_000, | |
| // Extract into a shared factory in runtimeFetchOptions.ts | |
| // (e.g., getOrCreateNoProxyAgent()) that both paths call. |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in e366644 — the hand-rolled Agent is deleted; the dispatcher now comes from a new getOrCreateMcpDispatcher() in runtimeFetchOptions.ts backed by the same dispatcherCache the LLM path uses (explicit proxy → the very same getOrCreateSharedDispatcher entry; no proxy → a cached env-aware agent under __env_proxy__). One pool, one timeout policy, one maintenance point. 中文:手写 Agent 已删除,dispatcher 收敛到 runtimeFetchOptions 的共享缓存(显式代理复用同一 getOrCreateSharedDispatcher 条目;无代理走 env_proxy 缓存),连接池/超时策略/维护点合一。
| mcpFetchDispatcher = new Agent({ | ||
| headersTimeout: 0, | ||
| bodyTimeout: 0, | ||
| keepAliveTimeout: 60_000, | ||
| ...(isTlsVerificationDisabled() | ||
| ? { connect: { rejectUnauthorized: false } } | ||
| : {}), | ||
| }); |
There was a problem hiding this comment.
[Suggestion] Dedicated MCP Agent bypasses process-wide proxy — the plain Agent is passed as an explicit dispatcher to undiciFetch, which causes undici to ignore the global dispatcher. When config.ts sets setGlobalDispatcher(new EnvHttpProxyAgent(...)) for proxy support, the old globalThis.fetch path honored it; this new path does not.
Failure scenario: user behind a corporate proxy with --proxy configured connects to a remote streamable HTTP MCP endpoint → direct TCP connection fails at firewall → MCP server unreachable.
| mcpFetchDispatcher = new Agent({ | |
| headersTimeout: 0, | |
| bodyTimeout: 0, | |
| keepAliveTimeout: 60_000, | |
| ...(isTlsVerificationDisabled() | |
| ? { connect: { rejectUnauthorized: false } } | |
| : {}), | |
| }); | |
| // Use getOrCreateSharedDispatcher() from runtimeFetchOptions.ts | |
| // (already creates proxy-aware dispatchers with the same timeout | |
| // settings), or fall back to EnvHttpProxyAgent when getProxy() | |
| // returns a URL. |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in e366644 — MCP traffic is proxy-aware again, without losing the dedicated-dispatcher fix: when the config installs the global proxy dispatcher it now records the resolved URL via a new setResolvedProxyUrlForRuntimeFetch, and getOrCreateMcpDispatcher() reuses the SAME cached proxy-aware dispatcher as the LLM path for it; with no explicit proxy it falls back to a cached EnvHttpProxyAgent honoring HTTP(S)_PROXY/NO_PROXY — both with the disabled header/body timeouts the SSE stream needs. Net improvement over the old globalThis.fetch path: that one never honored env-only proxies (Node's fetch ignores proxy env) and carried undici's 300s defaults through the global dispatcher. 中文:MCP 流量恢复代理感知且不失去专用 dispatcher 修复——config 安装全局代理时登记解析后的 URL,MCP 复用与 LLM 相同的缓存代理 dispatcher;无显式代理则回退到尊重 HTTP(S)_PROXY/NO_PROXY 的缓存 EnvHttpProxyAgent,两者都带 SSE 需要的禁用超时。相比旧 globalThis.fetch 路径是净改进(Node fetch 从不读 env 代理、全局 dispatcher 还带 300s 默认超时)。
Against some MCP servers, Node's built-in fetch stalls same-origin POSTs (tools/resources/prompts discovery) behind the transport's long-lived standalone GET SSE stream until the SDK's 60s request timeout fires — leaving the server connected but with no tools (QwenLM#7147, reproduced by the reporter against Fastmail's MCP endpoint on Node 26.4.0; OAuth to a different origin succeeds while every discovery call times out at exactly 60s). Route MCP Streamable HTTP requests through undici's own fetch with a dedicated module-level Agent instead of globalThis.fetch: - the npm undici build decouples the transport from the Node-bundled undici version (the reporter bisected the stall to the built-in fetch: swapping only the fetch implementation resolved it, as did independently suppressing the GET stream); - the Agent disables headersTimeout/bodyTimeout (undici defaults both to 300s — a standalone SSE stream legitimately idles longer between events) and honors NODE_TLS_REJECT_UNAUTHORIZED via isTlsVerificationDisabled(); - binding the dispatcher to the same undici build avoids the "invalid onError method" mismatch between package and bundled versions. The stall only manifests with specific server/undici-version combinations — a local matrix (Node 22.23/26.4/26.5 x plain HTTP/TLS x GET flushed/hung headers, real SDK transport) does not trigger it — so tests pin the plumbing: a contract test performs real requests through the transport fetch against a local server, and the two OAuth tests that stubbed globalThis.fetch now use a _setMcpFetchForTest seam (following the _reset*ForTest convention). Based on the reproduction, patch and bisection by @imrehg on QwenLM#7147. Fixes QwenLM#7147 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…patcher branch Review follow-up on QwenLM#7195. The contract test carried an unused transportFetch built over the default globalThis.fetch path — removed so the test no longer implies that path is exercised. The isTlsVerificationDisabled() branch of the dedicated dispatcher is now pinned by a real self-signed HTTPS round trip: with the default dispatcher the request rejects; after setting QWEN_TLS_INSECURE=1 and resetting the singleton via the new _resetMcpFetchDispatcherForTest seam, the same transport fetch connects. QWEN_TLS_INSECURE is used deliberately — NODE_TLS_REJECT_UNAUTHORIZED=0 is also honored by Node's own TLS layer and would make the positive phase pass without the dispatcher branch. Verified the test fails when the branch is removed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review follow-ups on QwenLM#7195 (three findings): - The static `import 'undici'` in mcp-client.ts was the sole eager undici entry left (index.ts re-exports the module), undoing the QwenLM#7264 startup optimization for every CLI invocation. The MCP fetch now awaits loadUndici()/preloadRuntimeFetchModule() at call time, and the built module carries no top-level undici import. - The hand-rolled Agent duplicated buildFetchOptionsWithDispatcher's no-proxy configuration. The dispatcher now lives in runtimeFetchOptions' shared cache via a new getOrCreateMcpDispatcher(): one pool, one timeout policy, one maintenance point. - The plain Agent also bypassed the process-wide proxy the config installs via setGlobalDispatcher. getOrCreateMcpDispatcher() reuses the same cached proxy-aware dispatcher as the LLM path when an explicit --proxy/settings proxy was resolved (config records it via a new setResolvedProxyUrlForRuntimeFetch at install time), and falls back to a cached EnvHttpProxyAgent honoring HTTP(S)_PROXY/NO_PROXY — with the same disabled header/body timeouts either way. Net improvement over the old globalThis.fetch path, which never honored env-only proxies and carried undici's 300s defaults through the global dispatcher. _resetMcpFetchDispatcherForTest keeps its name and delegates to the shared resetDispatcherCache (which now also clears the recorded proxy URL). mcp-client 109/109, runtimeFetchOptions 66/66. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
3a45b74 to
e366644
Compare
|
All three findings addressed in e366644 (per-thread replies posted; branch also rebased onto latest main): lazy undici load restoring #7264, dispatcher unified into runtimeFetchOptions' shared cache, and proxy-awareness via the recorded explicit proxy (same cached dispatcher as the LLM path) with an env-aware fallback. mcp-client 109/109 · runtimeFetchOptions 66/66 · typecheck of touched packages clean (the two pre-existing cli UI type errors on main are unrelated). 中文:三条发现已在 e366644 全部落地(已逐 thread 回复;分支已 rebase 最新 main):惰性加载恢复 #7264、dispatcher 收敛共享缓存、经登记的显式代理 + env 回退恢复代理感知。109/109 + 66/66,所触包 typecheck 干净(main 上既有的两个 cli UI 类型错误与本 PR 无关)。 |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
| if (resolvedProxyUrlForRuntimeFetch) { | ||
| return getOrCreateSharedDispatcher( | ||
| resolvedProxyUrlForRuntimeFetch, | ||
| insecure, | ||
| ); | ||
| } |
There was a problem hiding this comment.
[Suggestion] The explicit proxy path — the main fix for #7195 — lacks a direct test. The contract tests verify the env-proxy and TLS-insecure branches, but not that MCP traffic routes through an explicitly configured proxy.
Failure scenario: a regression in setResolvedProxyUrlForRuntimeFetch or the delegation logic could silently break proxy routing for MCP traffic when --proxy or settings.proxy is set, while the contract tests continue passing.
Consider adding a unit test:
it('routes MCP dispatcher through explicit proxy when set', () => {
setResolvedProxyUrlForRuntimeFetch('http://proxy.example.com:8080');
const dispatcher = getOrCreateMcpDispatcher();
expect(dispatcher).toBe(
getOrCreateSharedDispatcher('http://proxy.example.com:8080', false),
);
resetDispatcherCache();
});— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Added in 9f93dfe — a new getOrCreateMcpDispatcher describe block with three direct tests: (1) after setResolvedProxyUrlForRuntimeFetch('http://proxy.example.com:8080'), getOrCreateMcpDispatcher(false) returns the exact shared dispatcher instance from getOrCreateSharedDispatcher(proxyUrl, false) (the suggested test, with the identity assertion); (2) with no registered proxy, repeated calls return the same cached env-aware dispatcher and it is not the explicit-proxy one; (3) resetDispatcherCache() drops the registration, so a stale explicit proxy can't leak across resets. runtimeFetchOptions 112/112 + mcp-client 66/66.
中文
已在 9f93dfe 补上——新增 getOrCreateMcpDispatcher describe,共三条直接测试:(1) 注册显式代理后返回与 getOrCreateSharedDispatcher(proxyUrl, false) 完全相同的实例(即建议中的测试,用恒等断言);(2) 未注册时重复调用返回同一个缓存的 env-aware dispatcher,且不等于显式代理实例;(3) resetDispatcherCache() 会清除注册,避免过期显式代理跨重置泄漏。runtimeFetchOptions 112/112 + mcp-client 66/66。
Adds the direct unit tests the review asked for: a registered resolved proxy URL routes getOrCreateMcpDispatcher() to the same shared dispatcher buildRuntimeFetchOptions uses, the env-aware fallback stays cached when no proxy is registered, and resetDispatcherCache() drops the registration. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Re-review follow-up: the single suggestion (direct test for the explicit-proxy path) is addressed in 9f93dfe — see the inline reply for details. No source changes; runtimeFetchOptions 112/112, mcp-client 66/66, eslint/prettier clean. 中文重审跟进:唯一一条建议(显式代理路径的直接测试)已在 9f93dfe 落实,详见行内回复。无源码改动;runtimeFetchOptions 112/112、mcp-client 66/66、eslint/prettier 全绿。 |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
| const { EnvHttpProxyAgent } = requireUndici(); | ||
| const dispatcher = new EnvHttpProxyAgent({ | ||
| headersTimeout: 0, | ||
| bodyTimeout: 0, | ||
| keepAliveTimeout: 60_000, |
There was a problem hiding this comment.
[Suggestion] The EnvHttpProxyAgent options block here is a near-exact copy of the one in getOrCreateSharedDispatcher (line ~263) — same three timeout fields (headersTimeout: 0, bodyTimeout: 0, keepAliveTimeout: 60_000), same insecure TLS spread (connect/requestTls/proxyTls). If a future edit adds an option to one site (e.g. connectTimeout) without mirroring it here, the MCP and LLM dispatchers silently diverge — the JSDoc claim that MCP uses "the same disabled header/body timeouts as the LLM path" loses its enforcement.
Concrete cost: the two blocks are 8 lines of identical undici agent options, differing only in the absent httpProxy/httpsProxy fields (correct by design for the env-proxy fallback).
Consider extracting a buildEnvProxyAgentOptions(insecure: boolean) helper that returns the common options object, called from both getOrCreateSharedDispatcher and here.
— qwen3.7-max via Qwen Code /review
Maintainer re-verification (round 2) — re-architected since my round‑1 LGTM ✅ still merge‑readyFollowing up on my round‑1 verification (head Environment: head What changed since round‑1, and how I re‑verified each
1) Suites at the new head + independent harnesses
2) Non‑vacuity (mutation testing) — the tests genuinely gate on the fixBoth mutations were applied to the PR head and reverted (suite green again afterward): disabling the explicit‑proxy branch fails both the author's unit test and my routing harness; reverting the transport fetch to 3) Live E2E through the shipped binary —
|
| 变化点 | 第 1 轮设计(daa1573b) |
新 head 9f93dfec2 |
已复验 |
|---|---|---|---|
| undici 加载 | 顶层静态 import { Agent, fetch } from 'undici' |
移除 → 懒加载 loadUndici() / preloadRuntimeFetchModule()——undici 不再进入 eager 启动闭包(#7264) |
✅ |
| dispatcher | mcp-client.ts 内独立的 getMcpFetchDispatcher() Agent |
统一进 runtimeFetchOptions.getOrCreateMcpDispatcher()——与 LLM 复用同一连接池 + 超时策略 |
✅ |
显式 --proxy |
不生效(内置 fetch 从不为 streamable HTTP 走代理) | 现已生效——config.ts 记录解析后的代理 → MCP 复用与 LLM 相同的代理感知 dispatcher |
✅ 新增 |
| 测试 | 104 | 109(mcp‑client)+ 69(runtimeFetchOptions),含 3 个显式代理路由的新用例 | ✅ |
1)新 head 的测试套件 + 独立 harness
mcp-client.test.ts 109/109、runtimeFetchOptions.test.ts 69/69。在作者测试之外,我针对真实回环服务器、且不 mock 被测代码运行了三个独立 harness,直接验证新主张(共 5 文件 184 测试,全绿,见截图一):
- Cold-start follow-ups: remaining lazy-loading candidates from the ACP eager-closure audit #7264 懒加载:全新导入模块图后 undici 仍未加载——
getOrCreateMcpDispatcher()在preloadRuntimeFetchModule()之前会抛 "undici is not loaded yet",之后才构建真实 Dispatcher。(源码 diff 证实静态import … from 'undici'已删除,仅剩import type与动态import('undici')。) - 代理路由 / 统一(本轮重点新行为):注册显式代理后,
getOrCreateMcpDispatcher(false)返回与 LLM 路径getOrCreateSharedDispatcher(proxyUrl, false)完全相同的池化实例,真实请求经一个记录型 CONNECT 代理隧道转发;无代理时直连(代理零触达);环境变量HTTP_PROXY经 fallback dispatcher 生效。 - 绕过 globalThis.fetch(在重构后 head 再次确认):把
globalThis.fetch改为抛错后,transport 仍经专用 undici 路径成功,且对全局 fetch 的调用为 0 次。
2)非平凡性(变异测试)——测试确实咬住修复
两处变异均施于 PR head 后还原(还原后套件复绿,见截图三):禁用显式代理分支会让作者单测与我的路由 harness 双双失败;把 transport fetch 还原为 globalThis.fetch 会让绕过 harness 与 OAuth 缝测试失败。
3)经已发布二进制的端到端 —— qwen mcp list
qwen mcp list 走的正是受影响路径(createTransport → mcpUndiciFetch → 懒加载 → getOrCreateMcpDispatcher),并 ping 一个真实 @modelcontextprotocol/sdk Streamable HTTP 服务器:成功连接,复现 #7147 的请求形态(长驻独立 GET SSE 流 + 其后同源 POST 顺利完成),TLS‑insecure 开关在真实二进制上生效(见截图二)。TLS 差分具有决定性:Node 全局 TLS 层不读 QWEN_TLS_INSECURE,故“带开关即 Connected”只可能来自重构后 dispatcher 的 connect/requestTls/proxyTls: { rejectUnauthorized: false } 分支。
结论
相比第 1 轮我已认可的版本,本次重构是净改进:MCP 现尊重显式 --proxy 并与 LLM 共享连接池与禁用超时策略;undici 改为懒加载,不再拖累 eager 启动闭包(#7264);与 runtimeFetchOptions 的一致性从“并行”升级为“结构性统一”。范围如实说明(卡死本身无法在 CI 复现;stdio / 旧 SSE transport 未改动)。无阻塞项。
结论:建议合并。
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅



What this PR does
Routes MCP Streamable HTTP requests through undici's own
fetchwith a dedicated module-levelAgentinstead ofglobalThis.fetch. The Agent disablesheadersTimeout/bodyTimeout(undici defaults both to 300 s — a standalone SSE stream legitimately idles longer between server-sent events), honorsNODE_TLS_REJECT_UNAUTHORIZEDvia the existingisTlsVerificationDisabled()helper, and is bound to the same undici build as the fetch implementation (mixing the npm package's dispatcher into Node's bundled fetch throwsinvalid onError method). The two OAuth unit tests that stubbedglobalThis.fetchnow stub the transport fetch through a_setMcpFetchForTestseam (following the repo's_reset*ForTestconvention), and a new contract test performs real requests through the transport fetch against a local HTTP server.Why it's needed
#7147: against Fastmail's MCP endpoint, OAuth succeeds but
tools/list/resources/list/prompts/listall time out at exactly the SDK's 60 s — the server shows connected with no tools. The transport opens a long-lived standalone GET SSE stream afternotifications/initialized(per the MCP spec), and on the reporter's environment (Node v26.4.0, no proxy, endpoint negotiating h2 with curl) Node's built-in fetch stalls subsequent same-origin POSTs behind that stream. The reporter bisected it both ways at my request: swapping only the fetch implementation to the npm undici build resolved it, and independently, keepingglobalThis.fetchbut suppressing the GET stream also resolved it — pinning the failure to the combination of the built-in fetch and the held stream. The same server works in Claude Code, LMStudio, and viacurl.Reviewer Test Plan
How to verify
tools/listcompletes normally. (@imrehg verified the equivalent patch in that environment.)npx vitest run src/tools/mcp-client.test.ts(packages/core) 103/103, including the new real-server contract test.NODE_TLS_REJECT_UNAUTHORIZED=0, self-signed MCP endpoints still connect (the Agent'sconnect.rejectUnauthorizedfollows the same helper other transports use).Evidence (Before & After)
The stall needs a specific server/undici-version combination that a local synthetic server does not trigger: a reproduction matrix with the real
@modelcontextprotocol/sdktransport (Node 22.23 / 26.4 / 26.5 × plain HTTP / TLS × GET-stream flushed / headers-hung /maxConnections=1) completedtools/listin milliseconds in all combinations. The decisive before/after evidence is therefore the reporter's bisection on the affected environment (#7147 comment): undici-fetch-only → fixed; GET-stream-suppression-only → fixed; unmodified → 60 s timeouts. Tests in this PR pin the plumbing (real requests flow through the dedicated dispatcher; OAuth-challenge capture still works through the wrapper) rather than the stall itself, and the commit message documents that limitation.Tested on
Environment (optional)
macOS (Darwin 24.6), Node v22.23.1 (matrix also run on 26.4.0/26.5.0 via nvm); vitest unit + contract tests.
Risk & Scope
globalThis.fetch(test stubs, APM monkey-patches). Unit tests use the new seam; the dedicated path is deliberate — decoupling from the bundled undici is the fix. Timeouts move from undici's 300 s defaults to "none" on this Agent; per-request abort signals from the SDK still apply.HTTP(S)_PROXY) matches the previous behavior of the bundled fetch (neither honored proxies for streamable HTTP) and is left as is.Linked Issues
Fixes #7147
中文说明
本 PR 做了什么
将 MCP Streamable HTTP 请求改走 undici 自带的
fetch+ 模块级专用Agent,不再使用globalThis.fetch。Agent 禁用headersTimeout/bodyTimeout(undici 默认均为 300 秒——独立 SSE 流在事件之间的空闲完全可能超过它)、经既有isTlsVerificationDisabled()尊重NODE_TLS_REJECT_UNAUTHORIZED,并与 fetch 实现绑定同一 undici 构建(把 npm 包的 dispatcher 混入 Node 内置 fetch 会抛invalid onError method)。两个曾 stubglobalThis.fetch的 OAuth 单测改用_setMcpFetchForTest测试缝(遵循仓库_reset*ForTest惯例),并新增契约测试:经 transport fetch 对本地真实 HTTP 服务器发起真实请求。为什么需要
#7147:对 Fastmail 的 MCP 端点,OAuth 成功但
tools/resources/prompts发现全部在 SDK 的 60 秒处超时——服务器显示已连接却没有工具。transport 在notifications/initialized后按规范打开长驻独立 GET SSE 流;在报告者环境(Node v26.4.0、无代理、端点与 curl 协商 h2)中,Node 内置 fetch 会让后续同源 POST 卡在该流之后。应我的请求,报告者完成了双向二分:仅把 fetch 换成 npm undici 构建即可修复;独立地,保留globalThis.fetch仅抑制 GET 流也可修复——把故障钉在「内置 fetch + 长驻流」的组合上。同一服务器在 Claude Code、LMStudio 与curl下均正常。审阅测试计划
如何验证
tools/list正常完成(@imrehg 已在该环境验证等效补丁);mcp-client.test.ts103/103,含新的真实服务器契约测试;NODE_TLS_REJECT_UNAUTHORIZED=0下自签名 MCP 端点仍可连接。证据(Before & After)
该 stall 依赖特定「服务器 × undici 版本」组合,本地合成服务器无法触发:用真实 SDK transport 的复现矩阵(Node 22.23/26.4/26.5 × plain HTTP/TLS × GET 流正常/头挂起/连接数 1)在全部组合中毫秒级完成
tools/list。因此决定性的 before/after 证据是报告者在受影响环境的二分(issue 评论):仅换 undici fetch → 修复;仅抑制 GET 流 → 修复;不改 → 60 秒超时。本 PR 的测试固定的是管线(真实请求经专用 dispatcher、OAuth challenge 捕获经包装层仍有效)而非 stall 本身,提交信息中已如实说明这一局限。测试平台
macOS 已本地验证(✅);Windows / Linux 依赖 CI(⚠️ )。
环境
macOS(Darwin 24.6)、Node v22.23.1(矩阵另经 nvm 覆盖 26.4.0/26.5.0);vitest 单测 + 契约测试。
风险与范围
globalThis.fetch(测试 stub、APM monkey-patch)。单测改用新测试缝;独立路径是有意为之——与内置 undici 解耦正是修复本身。该 Agent 上的超时从 undici 默认 300 秒变为无;SDK 的每请求 abort 信号仍然生效。HTTP(S)_PROXY)与此前内置 fetch 行为一致(两者都不为 streamable HTTP 走代理),维持现状。关联 Issue
Fixes #7147
🤖 Generated with Claude Code