Repository navigation
fix(serve): load persisted MCP config after ACP preheat at startup - #11145
Conversation
Reconcile workspace MCP configuration once the primary runtime bridge finishes preheating so persisted mcpServers settings are available without a manual reload. Fixes QwenLM#7771
|
@qwen-code /triage — PR description updated to the full template; Linked Issues now uses Relates to #7771 (no auto-close). Please re-run from the description gate. |
Use the non-throwing primary runtime lookup and coordinator.ensure() so startup discovery runs on the workspace Config without re-enabling bootstrap MCP spawn.
|
Thanks for the detailed Stage 1b notes — addressed on
Local: Fork CI is still likely |
|
Verified the mechanism chain on current The placement is also correct — One suggestion (non-blocking): |
|
Thanks @now-ing — glad the Agree on the catch logging nit; I will add a debug log for best-effort warm-start failures so a stuck |
Boot discovery now prepares the already-live runtime without a second client-route keep-alive preheat, so an idle daemon is not forced to a 10-minute channel window. Warm-start failures are debug-logged.
b4b51e1 to
87e2575
Compare
|
Addressed the boot keep-alive issue on After a successful ACP preheat the runtime is already live, so the boot path now calls Also added debug logging in the boot catch for warm-start failures, and tests that pin: no post-startup |
|
@qwen-code /review --timeout=360 |
Document EnsureOptions.skipKeepAlivePreheat and the ensure overload so object-form live-runtime skip is no longer implicit. Log the two boot early returns, and use skip-path wording when a live snapshot dies before status().
Avoid polling daemon status after clearing the primary generation so the skip-no-runtime debug assertion does not race a broken status read.
|
Addressed Stage 2 suggestions 1–3 on
Local: coordinator 48 passed; |
Boot ensure({}) skipped keep-alive, so default channelIdleTimeoutMs=0
reaped the warm child before initializeWorkspaceMcp. Arm the 10-minute
window and load createDebugLogger from the debugLogger subpath so the
serve fast-path bundle stays closed.
|
Addressed Critical R1-1 and R2-1 on
Also: skip boot discovery when Local: coordinator 50 passed; |
|
@qwen-code /resolve |
main's QwenLM#10957 and QwenLM#10917 added the same @qwen-code/qwen-code-core/debugLogger subpath aliases this PR added, so the union of the two sides is main's version. Auto-merge could not see that and kept both copies in core's exports map and in the integration-tests paths block; drop the duplicates so each map names the subpath once. cli's vitest alias map moved to an ordered array on main, with the named core subpaths matched ahead of the core wildcard. debugLogger is one of those named subpaths and has to stay above the wildcard: the real file is core/src/utils/debugLogger.ts, which the wildcard cannot derive.
|
Qwen Code resolved the merge conflicts and pushed the branch update. Root cause. Base commits Semantic, not adjacent. Two files conflicted ( Load-bearing. Alias order is precedence in '@qwen-code/qwen-code-core/debugLogger': '../core/src/utils/debugLogger.ts',
{ find: /^@qwen-code\/qwen-code-core\/(.*)$/, replacement: `${CORE_SRC}/$1` },The wildcard derives Not verified. No build, typecheck, lint, or tests were run.
中文说明根因。 基线提交 属于语义冲突。 两个文件文本冲突（ 关键约束。 别名顺序即优先级，具名条目须在通配规则之上（代码见上）。通配规则会推导出并不存在的 未能验证。 未运行构建、类型检查、lint 或测试。
|
Extend the preheat success guard to skip discovery when runtime startup already failed, route boot diagnostics through daemonLog (including cause on failure), and add tests that go red if discovery runs on preheat failure, targets non-primary workspaces, or races shutdown/startup failure.
|
Addressed round-3 Suggestions on
Local: |
Annotate the channel-worker snapshot mock for typecheck, split shutdown vs startup-failure skip guards, unwrap ensure failure causes for daemon logs, skip warn on WorkspaceDrainingError, rename logs to workspace runtime ensure, and add tests for cause-less failures and the runtimeStartupError guard arm.
|
Addressed round-4 on d8e5428: Critical ChannelWorkerSnapshot mock typing; cause-less log test; runtimeStartupError guard test; deepestErrorMessage unwrap; skip warn on WorkspaceDrainingError; rename logs to workspace runtime ensure. Risk & Scope updated (tee to stderr). tsc/build green; startup observability 18 passed. |
Remove the forbidden bridgeErrors static import and classify ensure .catch failures via shuttingDown/runtimeStartupError. Check startup failure before shutdown when skipping post-preheat ensure, gate primary runtime on active state, and add tests for deepestErrorMessage, inactive primary, and the runtimeStartupError guard.
|
Addressed round-5 on :
Local: fast-path import-boundary green; / green; → 21 passed. |
Maintainer verification — real
|
| after boot, no session | BASE (main) |
PR #11145 |
|---|---|---|
capabilities.mcp.state |
not_started (whole run) |
ready at t+2.8 s |
GET /workspace/runtime/mcp → discoveryState |
not_started |
completed |
| server entry | witness-mcp status: error, mcpStatus: disconnected |
status: ok, mcpStatus: connected |
| MCP server process | never spawned (server log empty) | spawned; answered initialize, tools/list |
The description's clarification is accurate: on main the server is still listed through the bootstrap-Config fallback — what is missing is discovery and connection state.
2. The cost, measured
Both daemons started with --channel-idle-timeout-ms 5000, zero sessions, 11.7 min:
- BASE kills the channel at +10.8 s (
idle timeout (5000ms) expired) and never discovers MCP. - PR holds the ACP child to +603.9 s — the configured 5 s reclaim does not apply inside the boot keep-alive window — then kills it;
capabilities.mcp.stateflips tostale. - Held during the window: ACP child ≈ 227 MB plus one process per configured MCP server (≈ 50 MB for this trivial stdio one).
- Fairness note: the 10-minute window is not new.
POST /workspace/runtime/ensurearms the sameENSURE_KEEP_ALIVE_MSonmain, and I reproduced that on the BASE bundle. What is new is that every boot arms it unconditionally, including boots that serve nothing.
One thing this made visible that is worth knowing either way: on main, a single GET /workspace/runtime/mcp reaps the boot-preheated child (default idle timeout 0 → immediate kill when the runtime operation releases). So on main the boot warm-up is discarded the first time anything asks for status. Preventing that is the substantive part of what the boot keep-alive buys.
3. What a client actually gains
First POST /workspace/runtime/ensure at boot+25 s, 3 fresh daemons per arm: BASE 268 / 273 / 272 ms → PR 6 / 7 / 8 ms. BASE spawns the MCP server inside that call; a trivial local stdio server is the cheap case, an npx- or network-backed one moves this into seconds.
Worth setting expectations on, though: the Web Shell plugins/MCP page calls ensureRuntime() itself on mount, so its MCP panel ends up showing connected on both arms — I confirmed that in a real browser against both daemons. This PR changes who pays for discovery and when, not what that page eventually shows. The readers that genuinely see the gap are the non-ensuring ones: GET /workspace/runtime/{status,mcp}, daemon status, and anything polling before the first session.
4. Scope
With --workspace A --workspace B, at boot+30 s the secondary is still state: cold, mcp: not_started, discoveryState: not_started, and no MCP process was spawned for it. Primary-only, as described and as the unit test asserts.
5. Coverage of the new guards — 12 mutants, 10 killed
Each mutant is one semantic edit to the PR's own production code, run against run-qwen-serve.test.ts + workspace-runtime-coordinator.test.ts (451 tests). Pinned at this head: discovery only after a successful preheat (M4), primary-only via the state === 'active' gate (M3), both skip guards (M1/M2), the boot keepAliveMs (M5), the deepestErrorMessage unwrap loop (M7), and all three terms of the new skip condition (M8/M9/M11). Several standing review threads about exactly these being unpinned no longer reproduce at 736c673.
Two survivors, both minor:
- M6 — deleting
if (shuttingDown || runtimeStartupError !== undefined) return;from the failure.catchleaves all 451 tests green. It only gates a log line, so the exposure is a spurious warn during an orderly shutdown. - M10 — making
preheatignore the caller'skeepAliveMsand always use the constant is also invisible to the suites, because boot is the only caller and it passes exactly that constant. That is the concrete cost ofEnsureOptionsbeing wider than its use:timeoutMsandskipKeepAlivePreheathave no production caller at all, and the HTTP route still takes the numeric path (coordinator.ensure()), so the object-form skip branch is unreachable in production today.
I also checked the operator-visibility question: daemonLog tees every INFO/WARN to stderr and the daemon log file, and I see those lines in my captured daemon stderr — so the round-5 diagnostics do reach an operator (unlike the earlier debugLogger rounds). Reaching the skip branches in a live daemon is another matter: in shutdown races the preheat itself fails first (so nothing is scheduled) and the daemon exits ~100 ms after SIGTERM. Those branches are defensive, not observable in practice.
6. Gates
npm run buildon the merged tree: pass — this compiles the test files too, so the earlierChannelWorkerSnapshotmock typing Critical is gone at this head.fast-path.test.tsincluding "keeps the runQwenServe static source graph free of ACP runtime modules": 95 pass — the forbidden-import Critical is resolved.run-qwen-serve.test.ts401 pass,workspace-runtime-coordinator.test.ts50 pass, eslint + prettier on the 5 changed files: clean.- The red
Lint & Staticis not this PR. The job, evaluated from the merge ref, runs.github/scripts/check-lint-gate-freshness.mjs, whichmaingained in ci: fail Lint & Static when the base moved the lint gate #11383 and this branch does not have →MODULE_NOT_FOUNDafter 14 s. Mergingorigin/maininto the branch clears it — please merge, not rebase, so the inline review threads stay attached. - One caveat on the bot's
serve daemon A/B"no response changes": its scenario set is/health,/capabilitiesand/session/*— it never reads/workspace/runtime/statusor/workspace/runtime/mcp, which is precisely where this PR's difference lives. It is not evidence of no behaviour change.
Verdict
Behaviour matches the description on every claim I could drive, the scope is honest, and the trade is disclosed in Risk & Scope. From my side this is mergeable; the open question is a product call, not a correctness one. Non-blocking follow-ups, in the order I'd rank them:
- Size the keep-alive to the work. Release it when the boot
ensure()resolves, or pass a window sized to the preparation, so a daemon that serves nothing does not hold an ACP child plus every configured MCP server for a flat 10 minutes and does not override a configured--channel-idle-timeout-ms. - Trim
EnsureOptionsto what boot uses (or add the second caller that justifies the shape); ifkeepAliveMsstays a parameter, pin the?? ENSURE_KEEP_ALIVE_MSfallback — M10 shows nothing tests it. - Optional: a test for the failure-
.catchguard (M6).
中文说明
结论
以维护者身份在本地用真实 qwen serve daemon 做了同树 A/B 验证：main 上的问题确实复现，本 PR 确实修好了，代价与描述里 Risk & Scope 写的完全一致。没有发现阻塞性缺陷；唯一的红 CI 是分支落后 main 导致的，与本 PR 无关。
装置
PR head 736c673 与 origin/main 2e21214 合并（无冲突）后，用同一棵树打两个包：PR 臂原样；BASE 臂只把 run-qwen-serve.ts 和 workspace-runtime-coordinator.ts（外加三个测试文件，否则 tsc 过不了）回退到 origin/main。臂身份在产物里核过：workspace runtime ensure after preheat 在 PR 包出现 5 次、BASE 包 0 次。隔离 HOME/QWEN_HOME，持久化一个会把每次被拉起和每条 JSON-RPC 都记流水账的 stdio MCP server，这样"discovery 到底有没有真连上"由服务端来判，而不是看 daemon 自己的状态字段。全程不建 session、不开 Web Shell。
1. 缺口复现 & 修复生效（图 1）
BASE：mcp.state 全程 not_started、discoveryState: not_started、server 是 error/disconnected、MCP 进程根本没被拉起。PR：t+2.8 s 就 ready / completed / connected，MCP server 被拉起并回了 initialize、tools/list。描述里那句澄清是准确的——main 上 server 仍会经 bootstrap 回落列出来，缺的是 discovery/连接状态。
2. 代价（图 2）
两臂都带 --channel-idle-timeout-ms 5000、零 session 跑 11.7 分钟：BASE 在 +10.8 s 就把 channel 杀了且从未 discovery；PR 把 ACP 子进程一直留到 +603.9 s（配置的 5 秒回收在 keep-alive 窗口内不生效），之后被杀、mcp.state 变 stale。窗口期内常驻：ACP 子进程约 227 MB，外加每个配置的 MCP server 一个进程（这个极简的约 50 MB）。
需要公平地说明：10 分钟窗口本身不是本 PR 引入的——POST /workspace/runtime/ensure 在 main 上就 arm 同一个 ENSURE_KEEP_ALIVE_MS，我在 BASE 包上复现了。新的是每次启动都无条件 arm 它，包括一个 session 都不服务的启动。
另外一个顺带查明、无论如何都值得知道的事实：在 main 上，只要有人 GET /workspace/runtime/mcp 一次，启动时预热的子进程就会被回收（默认 idle timeout 为 0，运行时操作一释放就立刻杀）。也就是说 main 的启动预热在"第一次有人问状态"时就作废了——挡住这件事才是这个 keep-alive 真正买到的东西。
3. 客户端实际得到什么（图 3）
启动后 25 秒发第一次 POST /workspace/runtime/ensure，每臂 3 次全新 daemon：BASE 268 / 273 / 272 ms → PR 6 / 7 / 8 ms。BASE 是在这次调用里才拉起 MCP server；本地极简 stdio 是最便宜的情况，npx 或走网络的 server 会把这个数量级抬到秒。
但期望值要说清楚：Web Shell 的插件/MCP 页面自己会在挂载时调 ensureRuntime()，所以两臂的 MCP 面板最终都显示"已连接"（我用真浏览器对两个 daemon 都看过）。本 PR 改的是谁在什么时候为 discovery 买单，不是那个页面最终显示什么。真正看得见缺口的是不会主动 ensure 的读取方：GET /workspace/runtime/{status,mcp}、daemon status，以及第一次 prompt 之前的各种轮询。
4. 作用域
--workspace A --workspace B 时，boot+30 s 的 secondary 仍是 cold / not_started / not_started，也没为它拉起 MCP 进程。只覆盖 primary，与描述和单测一致。
5. 新守卫的覆盖度 —— 12 个变异体杀 10
已被钉住的：只在 preheat 成功后才 discovery（M4）、state === 'active' 的 primary 门（M3）、两个 skip 守卫（M1/M2）、boot 的 keepAliveMs（M5）、deepestErrorMessage 的展开循环（M7），以及新 skip 条件的全部三个项（M8/M9/M11）。若干条说这些"没有测试钉住"的历史评审线程，在 736c673 上已经复现不出来了。
两个存活的都不严重：
- M6 —— 把失败
.catch里的if (shuttingDown || runtimeStartupError !== undefined) return;删掉，451 个测试全绿。它只挡一条日志，风险是有序关停时多打一条 warn。 - M10 —— 让
preheat忽略调用方传的keepAliveMs、永远用常量，同样全绿：因为 boot 是唯一调用方且传的正是那个常量。这正是EnsureOptions比实际用法更宽的代价：timeoutMs和skipKeepAlivePreheat没有任何生产调用方，HTTP 路由仍走数字重载（coordinator.ensure()），所以对象形态的 skip 分支在生产里目前不可达。
关于"诊断能不能被运维看到"：daemonLog 的每条 INFO/WARN 都会同时 tee 到 stderr 和日志文件，我抓到的 daemon stderr 里确实有这些行——第 5 轮的诊断是能到达运维的（早几轮的 debugLogger 则不能）。但这些 skip 分支在真 daemon 里几乎跑不到：关停竞态下先失败的是 preheat 本身（于是根本不会调度），而且 daemon 收到 SIGTERM 后约 100 ms 就退出了。它们是防御性的，不是可观测的。
6. 门禁
合并树 npm run build 通过（会一起编译测试文件，所以上一轮那条 ChannelWorkerSnapshot mock 类型 Critical 在这个 head 上已经不存在）；fast-path.test.ts 的 import 边界用例 95 个全过（禁止导入那条 Critical 已解决）；run-qwen-serve.test.ts 401 过、workspace-runtime-coordinator.test.ts 50 过；改动的 5 个文件 eslint + prettier 干净。
红着的 Lint & Static 不是本 PR 的问题：该 job 从 merge ref 求值，要跑 .github/scripts/check-lint-gate-freshness.mjs，这个脚本是 main 在 #11383 才加的、本分支没有，于是 14 秒就 MODULE_NOT_FOUND。把 origin/main merge（不要 rebase，否则行内评审线程会失效）进分支即可清掉。
还有一点提醒：机器人那条 serve daemon A/B 的"no response changes"不能当作"没有行为变化"的证据——它的场景集是 /health、/capabilities 和 /session/*，从不读 /workspace/runtime/status 或 /workspace/runtime/mcp，而差异恰恰全在那里。
建议（都不阻塞合并）
- 让 keep-alive 与实际工作等长：boot 的
ensure()一 resolve 就释放，或传一个与准备工作等长的窗口；避免"零 session 的 daemon 白白占着 ACP 子进程 + 所有 MCP server 十分钟"，也避免盖过运维配置的--channel-idle-timeout-ms。 - 把
EnsureOptions收敛到 boot 真正用到的形状（或补上能证明这个形状的第二个调用方）；若keepAliveMs保留为参数，就把?? ENSURE_KEEP_ALIVE_MS这条回退钉住——M10 说明目前没有任何测试覆盖它。 - 可选：给失败
.catch的守卫补一个测试（M6）。
|
Thanks @wenshao for the thorough real-daemon A/B verification — really helpful measurements on the keep-alive cost and the readers that actually see the gap. I will merge into this branch (not rebase) so the Lint & Static freshness gate clears and the inline review threads stay attached. Happy to follow up separately on sizing the boot keep-alive to the ensure work and trimming unused fields if you want those as a follow-up PR. |
|
Correction: I will merge |
|
Merged into this branch at (merge, not rebase) so the Lint & Static freshness script from #11383 is present and inline review threads stay attached. |
Maintainer verification, second pass — current head
|
| after boot, no session | base a0f0d38 |
PR 186a2431 |
|---|---|---|
discoveryState |
not_started for the whole 25 s |
completed at t≈3.0 s |
| server entry | status: error, mcpStatus: disconnected |
status: ok, mcpStatus: connected |
capabilities.mcp.state |
not_started |
ready |
| MCP server process | never spawned (log file empty) | spawned; answered initialize, prompts/list, resources/list, tools/list |
Same result as pass 1, on a different OS and Node major. processToListenMs is unchanged (702 ms base / 750 ms PR) — the boot ensure is fire-and-forget after preheat and does not sit in the listen path.
2. New — with a session running, base's workspace-scope status is still not_started
Pass 1 only covered the no-session window. Repeating with POST /session {cwd, sessionScope:"thread"} fired immediately after listen separates two symptoms:
- On base, the session Config does spawn and connect the server —
GET /workspace/mcp/<name>/toolsreturns["mcp__pr11145-witness__pr11145_ping"]. But the workspace-scopediscoveryStateis stillnot_started, because the dedicated discovery Config was never created. - On PR, the same request returns the same tool, and
discoveryStateiscompleted.
This sharpens the "who actually sees the gap" point from pass 1: on main the field is not merely late, it is wrong for as long as no HTTP ensure/reconcile path runs — even while MCP tools are demonstrably working inside sessions. Creating a session at boot is otherwise unaffected by the change (200 on both arms).
3. Re-measured cost, this time from the MCP server's own clock
Both arms started with --channel-idle-timeout-ms 3000, zero sessions, and zero HTTP requests — polling /workspace/mcp is itself channel activity and resets the idle timer, which silently masks the reclaim behaviour, so liveness is read off the process table instead.
- base: ACP child alive at t=8 s, gone by t=13 s — reclaimed inside the configured 3 s window. MCP server never spawned.
- PR: the MCP server process records its own lifetime on exit — spawned
09:42:53.452Z,SIGTERM09:52:53.413Z,uptimeMs: 599961. Held for 10 min 00 s, i.e. 200× the configured idle timeout, on a daemon that served nothing.
That corroborates pass 1's +603.9 s ACP-child figure from the other side of the pipe, and confirms the hold covers every configured MCP server process, not just the ACP child. The trade is the one Risk & Scope already discloses.
4. New — a broken MCP server does not stall boot or take the daemon down
Swapped the witness for a stdio server that connects and then never answers initialize — the case that would expose boot ensure's 60 s budget as an operator-visible failure:
GET /healthstays200throughout; the daemon process never exits.discoveryStatesits atin_progressfor ~35 s, then settles tocompletedwith the serverdisconnected.- No
workspace runtime ensure after preheat failed/skippedline is emitted — boot ensure resolves inside its budget rather than logging a spurious warning.
The same figure re-confirms scope: with --workspace A --workspace B, at boot+12 s the secondary is state: cold, mcp: not_started, skills: not_started, and exactly 1 MCP process exists daemon-wide. Primary-only, as described.
5. Test A/B and two mutations pass 1 did not run
Reverting only packages/cli/src/serve/run-qwen-serve.ts to a0f0d38 (all test files and the coordinator change stay at PR head): 10 of the 12 new run-qwen-serve.test.ts cases go red. The two that stay green are the negative guards (does not schedule … when ACP preheat fails, … when shutting down during preheat) — expected, they cannot fail in the absence of the feature. Forcing skipKeepAlivePreheat = false in the coordinator reddens 4 of the 6 new coordinator cases.
Two mutations aimed at the design decisions in this diff, both pinned:
- Duplicated constant.
run-qwen-serve.tsredeclaresENSURE_KEEP_ALIVE_MS = 10 * 60_000locally rather than importing it, with a "must match" comment. Changing the local literal to9 * 60_000failsstarts workspace MCP discovery after ACP preheat succeedswithexpected "ensure" to be called with [ { keepAliveMs: 600000 } ]— because the test imports the coordinator's exported constant. Drift between the two is caught. - Import boundary. Adding a static
import { ENSURE_KEEP_ALIVE_MS } from './workspace-runtime-coordinator.js'torun-qwen-serve.tsfailsfast-path.test.ts > keeps the runQwenServe static source graph free of ACP runtime modules. The reason the constant is duplicated is enforced by a test, not just by a comment.
Local gates on this head: tsc --noEmit -p packages/cli exit 0; eslint on the 5 changed files exit 0; workspace-runtime-coordinator.test.ts 50 pass; run-qwen-serve.test.ts -t 'runQwenServe startup observability' 21 pass (the PR body's Environment block still says 18 — worth refreshing).
6. The red lane — different cause than last time
Last pass, Lint & Static died with MODULE_NOT_FOUND because the branch predated #11383. That cause is gone — .github/scripts/check-lint-gate-freshness.mjs is present at 186a2431, so the step can no longer fail on a missing module. The job's step list confirms steps 1–7 pass and step 8 Check lint gate freshness is the only failure. (The workflow run was still in progress, so its log is not retrievable yet.) Running the script's own containment check locally identifies the stale file — main moved a gate-defining file after the branch's last sync:
NO .github/workflows/ci.yml 17990c330d fix(desktop): realign the release test with the new signing step (#11522)
All six other gate files are contained in the branch. Nothing in this diff. Re-merging main clears it — but note this will keep re-reddening on a repo merging at this rate; it is worth landing this soon after a sync rather than treating the red as actionable. (Test (ubuntu-latest, Node 22.x) was still in progress when I wrote this.)
Standing follow-ups — still open, still non-blocking
Re-confirmed at this head: there are exactly two coordinator.ensure() call sites in production — routes/workspace-runtime.ts:64 (ensure(), numeric path) and the new boot call (ensure({ keepAliveMs })). Neither can reach the object-form skip branch, so EnsureOptions.skipKeepAlivePreheat and timeoutMs remain unreachable in the shipped daemon, exactly as pass 1's M10 found. My ranking is unchanged:
- Size the keep-alive to the work — release it when the boot
ensure()resolves, so a daemon that serves nothing does not hold an ACP child plus one process per configured MCP server for a flat 10 minutes, and does not override a configured--channel-idle-timeout-ms. - Trim
EnsureOptionsto what boot actually uses, or add the second caller that justifies the shape. - Optional: a test for the failure-
.catchguard (pass 1's surviving M6).
Verdict
Every claim in the description that I could drive holds on Linux/Node 22 as it did on macOS/Node 24, the new evidence in §2 makes the case slightly stronger than the description does, and §4 closes the "what if an MCP server misbehaves at boot" question. Mergeable from my side; the keep-alive sizing is a product call, not a correctness one.
中文版（合并参考）
维护者验证 · 第二轮 —— 当前 head 186a2431，Linux / Node 22
接上一轮（macOS 15.6 / Node 24，head 736c673）。本 PR 对这五个文件的实际改动在两个 head 之间逐字节相同（各自对自己的 merge-base 取 diff，两份补丁完全一致）——186a2431 只是 merge 了 main——所以这一轮是跨平台复核 + 上一轮没跑的四项新检查。结论不变：merge-base 上问题确实复现，本 PR 确实修好，没有阻塞缺陷，唯一红的是分支落后。
装置（与上一轮的差异）
- 两个独立 worktree，各自真实
npm ci：base = 本分支自己的 merge-basea0f0d38，PR = head186a2431；都用packages/cli/dist/index.js起 daemon。臂身份在产物里核过：workspace runtime ensure after preheat在 PRdist出现 5 次、base 0 次。 - Linux 6.12 / Node 22.22.2（上一轮是 macOS / Node 24）。
- 持久化一个会把每次被拉起、每条 JSON-RPC 都记流水账的 stdio MCP server，"discovery 到底连上没有"由服务端进程判定，而不是看 daemon 自己的状态字段。
- 一条值得记下的装置坑：我最初把 server 放在 workspace 的
.qwen/settings.json。这个 scope 在 Add project-scoped .mcp.json support with pending approval semantics #4615 的审批门后面，于是两臂都返回approvalState: "pending"/mcpStatus: "disconnected"，进程根本不会被拉起——这会把本 PR 的效果整个掩盖成假阴性。必须放在用户级（不受门控），连接才能成为可观测量。
1. 复核：缺口与修复（图 1）
启动后每秒拉一次 GET /workspace/mcp，不建 session、不开 Web Shell：base 全程 discoveryState: not_started、server 是 error/disconnected、capabilities.mcp.state: not_started、MCP 进程从未被拉起；PR 在 t≈3.0 s 变为 completed / connected / ready，MCP 进程被拉起并完整回了 initialize、prompts/list、resources/list、tools/list。与上一轮在不同 OS 和 Node 大版本上结果一致。processToListenMs 无变化（base 702 ms / PR 750 ms）——boot ensure 是 preheat 之后 fire-and-forget，不在 listen 路径上。
2. 新增：即使有 session 在跑，base 的 workspace 级状态依然是 not_started（图 5）
上一轮只覆盖了"无 session"窗口。这次在 daemon listen 后立刻 POST /session {cwd, sessionScope:"thread"}，把两个症状分开了：
- base：session Config 确实会把 server 拉起并连上——
GET /workspace/mcp/<name>/tools返回["mcp__pr11145-witness__pr11145_ping"]。但 workspace 级discoveryState仍是not_started，因为专用 discovery Config 从未被创建。 - PR：同一个请求返回同一个工具，且
discoveryState为completed。
这把上一轮"谁真正看得见缺口"的结论收得更紧：在 main 上这个字段不只是"晚"，而是只要没有 HTTP ensure/reconcile 跑过就一直是错的——哪怕 session 里的 MCP 工具明明在正常工作。除此之外，启动即建 session 不受本改动影响（两臂都 200）。
3. 代价复测，这次从 MCP server 自己的时钟读（图 3）
两臂都带 --channel-idle-timeout-ms 3000、零 session、零 HTTP 请求——轮询 /workspace/mcp 本身就是 channel 活动、会重置 idle 计时器，从而悄悄掩盖回收行为，所以改用进程表读存活。
- base：ACP 子进程 t=8 s 在、t=13 s 已消失——在配置的 3 秒窗口内被回收；MCP server 从未被拉起。
- PR：MCP server 进程在退出时记下自己的寿命——
09:42:53.452Z拉起，09:52:53.413Z收到SIGTERM，uptimeMs: 599961。整整 10 分 00 秒，是配置 idle 超时的 200 倍，而这个 daemon 一个 session 都没服务。
这从管道另一端印证了上一轮 +603.9 s 的 ACP 子进程数字，并确认这个 hold 覆盖的是每一个配置的 MCP server 进程，不只是 ACP 子进程。这个取舍 Risk & Scope 已经如实披露。
4. 新增：坏掉的 MCP server 不会卡住启动，也不会拖垮 daemon（图 4）
把 witness 换成一个"连上之后永不回应 initialize"的 stdio server——这正是最可能把 boot ensure 的 60 秒预算暴露成运维可见故障的场景：
GET /health全程200，daemon 进程从未退出。discoveryState在in_progress停留约 35 s，随后落到completed，server 标记disconnected。- 没有任何
workspace runtime ensure after preheat failed/skipped日志——boot ensure 在预算内正常返回，不会打出吓人的告警。
同一张图复核了作用域：--workspace A --workspace B 时，boot+12 s 的 secondary 仍是 cold / not_started / not_started，全 daemon 只有 1 个 MCP 进程。只覆盖 primary，与描述一致。
5. 测试 A/B，以及上一轮没做的两个变异（图 2）
只把 packages/cli/src/serve/run-qwen-serve.ts 回退到 a0f0d38（所有测试文件和 coordinator 改动都留在 PR head）：12 个新增用例里 10 个变红。保持绿的两个是否定式守卫（does not schedule … when ACP preheat fails、… when shutting down during preheat）——符合预期，没有该特性时它们本来就不可能失败。把 coordinator 的 skipKeepAlivePreheat 强制为 false，6 个新增 coordinator 用例红 4 个。
针对本 diff 两个设计决策的变异，都被钉住了：
- 重复常量：
run-qwen-serve.ts本地重新声明了ENSURE_KEEP_ALIVE_MS = 10 * 60_000（带 "must match" 注释）而不是 import。把本地字面量改成9 * 60_000，starts workspace MCP discovery after ACP preheat succeeds会以expected "ensure" to be called with [ { keepAliveMs: 600000 } ]失败——因为测试 import 的是 coordinator 导出的那个常量。两处漂移能被抓到。 - 导入边界：在
run-qwen-serve.ts里加一条静态import { ENSURE_KEEP_ALIVE_MS } from './workspace-runtime-coordinator.js'，fast-path.test.ts > keeps the runQwenServe static source graph free of ACP runtime modules会失败。"为什么要复制常量而不是 import"是被测试强制的，不只是注释。
本地门禁：tsc --noEmit -p packages/cli exit 0；改动的 5 个文件 eslint exit 0；workspace-runtime-coordinator.test.ts 50 过；run-qwen-serve.test.ts -t 'runQwenServe startup observability' 21 过（PR 描述的 Environment 段还写着 18，可以顺手更新）。
6. 红着的那条 lane —— 与上次原因不同
上一轮 Lint & Static 是因为分支早于 #11383 而 MODULE_NOT_FOUND。这个原因已经消失——.github/scripts/check-lint-gate-freshness.mjs 在 186a2431 上已存在，该步骤不可能再因缺模块而失败。job 的 step 列表显示第 1–7 步全过、第 8 步 Check lint gate freshness 是唯一失败项。（该 workflow run 当时仍在进行中，日志还取不到。）在本地跑一遍该脚本的包含性判定，可以定位到落后的那个文件——分支上次同步之后 main 又动了一个 gate 文件：
NO .github/workflows/ci.yml 17990c330d fix(desktop): realign the release test with the new signing step (#11522)
其余六个 gate 文件分支都已包含。与本 diff 无关。再 merge 一次 main 即可清掉——但请注意在这个合并速率下它会反复变红，与其把这条红当作待办，不如同步完就尽快合。（写这条评论时 Test (ubuntu-latest, Node 22.x) 仍在跑。）
仍然开着的后续项（都不阻塞）
在这个 head 上复核过：生产里 coordinator.ensure() 只有两个调用点——routes/workspace-runtime.ts:64（ensure()，走数字重载）和新增的 boot 调用（ensure({ keepAliveMs })）。两者都到不了对象形态的 skip 分支，所以 EnsureOptions.skipKeepAlivePreheat 和 timeoutMs 在已发布的 daemon 里不可达，与上一轮 M10 的结论一致。排序不变：
- 让 keep-alive 与实际工作等长——boot 的
ensure()一 resolve 就释放，避免零 session 的 daemon 白占一个 ACP 子进程外加每个配置的 MCP server 一个进程整整十分钟，也避免盖过运维配置的--channel-idle-timeout-ms。 - 把
EnsureOptions收敛到 boot 真正用到的形状，或补上能证明这个形状的第二个调用方。 - 可选：给失败
.catch的守卫补测试（上一轮存活的 M6）。
结论
描述里我能驱动到的每一条，在 Linux/Node 22 上与 macOS/Node 24 表现一致；§2 的新证据让理由比描述本身还更强一点；§4 关掉了"启动时 MCP server 行为异常怎么办"这个问题。从我这边可以合并；keep-alive 的窗口大小是产品取舍，不是正确性问题。
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed at d515b9a. This is an approval, so I want to show the work rather than just assert it — there are four Criticals still marked unresolved on this PR, and I checked each one against the code as it actually stands at this commit.
All four are fixed; three of them are stale anchors rather than live findings.
- The
createDebugLoggermisuse and thebridgeErrorshandling are both simply gone fromrun-qwen-serve.tsat this SHA — I grepped the whole file, neither symbol appears. WorkspaceDrainingErroris likewise absent. Note that this also makes the corresponding claim in the PR description stale; worth trimming the body so a future reader is not sent looking for it.- The typecheck failure in
run-qwen-serve.test.tsis fixed: thesnapshotmock is now annotatedvi.fn((): ChannelWorkerSnapshot => ({ enabled: true, ... })).
Both security concerns raised earlier provably do not occur.
The one that mattered most was whether workspace MCP discovery could mutate or leak into the bootstrap Config. It cannot: createWorkspaceMcpDiscoveryConfig in acpAgent.ts builds a separate Config via loadCliConfig, attaches the transport pool to that instance, initializes it with skipMcpDiscovery: true, and runs incremental discovery against it. The bootstrap Config is never touched.
The trust gate also holds at boot — run-qwen-serve.ts clears shouldPreheat when the workspace is untrusted, before anything preheats, and the discovery helper throws rather than proceeding when discovery does not start ("The workspace may not be trusted.").
The reordering itself is coherent. Boot passes an explicit keepAliveMs into .ensure(...), and the skipKeepAlivePreheat computation in workspace-runtime-coordinator.ts only takes the skip branch when keepAliveMs === undefined, so the boot path still arms preheat as intended.
One Suggestion, non-blocking: EnsureOptions.skipKeepAlivePreheat has no importer anywhere outside this file — every caller reaches it through the ?? typeof timeoutMsOrOptions !== 'number' default. It is a dead switch today. Benign because the default reproduces the previous behavior exactly, but if nothing is going to set it, dropping the option would be simpler than keeping a knob nobody turns.
On the core gate: it does not apply. All five changed files are under packages/cli/src/serve/** plus the ACP integration in the same package — no packages/core and no cross-package surface.
CI is green on d515b9a. Note on method: I could not run tests locally (the worktree lacks node_modules/packages/cli's built dependencies, and the vitest globalSetup guard stops runs until npm run build from the repo root), so the above is read-verified against this exact commit plus CI on the same SHA.
LGTM — approving. Please squash the stale WorkspaceDrainingError sentence out of the description before merge.
Independent e2e verification at
|
head d515b9a (this PR) |
base 424e40cc1f (revert control) |
|
|---|---|---|
| source in tree, witnessed before start | matches head, new symbol ×2 | matches base, new symbol ×0 |
| process executing the PR tree | yes | yes |
/health 200 |
2.0 s | 2.0 s |
runtimeLive at window end |
true | true |
| probe MCP server spawned inside the window | 10.7 s | never |
| probe log lines at window end | 12 — initialize → notifications/initialized → prompts/list → resources/list → tools/list |
0 |
capabilities.mcp.state at window end |
ready | not_started |
the same, after an explicit POST /workspace/runtime/ensure |
ready (unchanged) | ready, probe then spawns with the identical 12-message handshake |
What this establishes:
- The fix executes on the real boot path. MCP discovery completes ~10 s after the daemon starts serving, with no client asking for it. The spawn is downstream of
scheduleWorkspaceMcpDiscoveryAfterPreheat, which is only reachable from preheat's.then()— so it is positive evidence that preheat succeeded, not merely an absence of theACP preheat failedline. - The control excludes the two alternative explanations. A live runtime is not sufficient (control:
runtimeLive=true,mcp=not_started, zero probe traffic), and my status polling is not the trigger (identical polling, identical unchanged route code, no discovery in the control). - The control's empty window is not a broken MCP config. The same config loads on demand in the control and produces a byte-comparable handshake, so the measured difference is when discovery happens — precisely what this PR claims to change. A control that could not differ would have proved nothing; this one differed on the axis under test and on no other.
- No skip branch fired: zero
workspace runtime ensure after preheat skippedlines and zero… ensure after preheat failedlines in the head arm.
Code review at this head — no Critical found
Production coverage is file-complete (2 of 2 production files, read from head bytes, all hunks accounted for against the API's own +913/-15). The three test files are not covered by that claim beyond the roles named below.
- The new object-form default cannot silently change an existing caller. Every
.ensure(site was enumerated: the only coordinator caller isroutes/workspace-runtime.ts:64,await coordinator.ensure()— no argument, sotypeof timeoutMsOrOptions === 'number'holds and the skip branch is unreachable, i.e. its behavior is unchanged. This PR's boot call is the sole object-form caller and it passeskeepAliveMs, which also forces the preheat. This independently corroborates the dead-switch observation already filed here as a non-blocking Suggestion:skipKeepAlivePreheathas no setter and its default reproduces prior behavior exactly, so it is inert rather than wrong, and I am not raising it. - The ordering the fix depends on holds.
workspaceRegistryis constructed insidebuildRuntime()from the already-built runtime list (run-qwen-serve.ts:6425), andcreateEntrysetsstate: 'active'together withcurrent.runtimeat creation (workspace-registry.ts:281), so when preheat resolves theprimaryEntry.state === 'active'read at:9007does find a runtime. The access pattern matches the pre-existing one at:6506. - Preheat failure leaves MCP undiscovered at boot, which is the pre-existing behavior, is pinned by
does not schedule workspace MCP discovery when ACP preheat fails, and still tells the user it retries on first session. Not a regression. deepestErrorMessageonly fails to terminate on a self-referentialcausechain, which nothing on this path constructs; the three shapes that do occur (multi-hop, non-Error, cause-less) are each pinned by a test.- The duplicated
ENSURE_KEEP_ALIVE_MS(re-declared in serve with a comment naming the static-import boundary, now exported from the coordinator) is a divergence risk for a future edit, not a defect at this head.
Scope and CI
Exercised: serve-mode boot, trusted single primary workspace, one stdio MCP server, both arms in the same environment. Not exercised: untrusted workspaces (shouldPreheat is cleared at :4768 before anything preheats, so the code under test is unreachable there by design), secondary/multi-workspace runtimes, HTTP-transport MCP servers, the four skip branches and the keep-alive/reap interaction (each is unit-tested instead, including reaps a live child before MCP init unless ensure arms a keep-alive hold). CI at this head: 19 product lanes success, 0 failure, 0 unfinished, with Test (macos-latest, Node 22.x), Test (windows-latest, Node 22.x) and Integration Tests (CLI, No Sandbox) skipped by event type.
Verdict, as of the state read immediately before posting: at d515b9a I found no Critical, the change demonstrably does what it says on a live daemon, and CI is green — from my side this is merge-ready. This comment carries no approval; the approval at this head is doudouOUC's, and any approval of my own would be a separate later act.
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Independent verification — approval at d515b9a.
My full report is in the comment above (posted 2026-09-10T14:30:51Z at this same head). It concluded merge-ready and deliberately carried no approval; this row is that separate later act.
Re-verified from primary sources immediately before submitting, at head d515b9a7103c5316c2967c58eae8ade0f0025c3c (unmoved since the report):
- Lifecycle:
state=open,merged=false,draft=false. - CI at this head, measured in this same read: 31 check runs — 23 green, 0 failure, 0 unfinished, 8 skipped by event type (
skippedcounted as completed, not pending). Green product lanes includeTest (ubuntu-latest, Node 22.x),Lint & Static (ubuntu-latest, Node 22.x),Integration Tests (no-AK, No Sandbox),Serve A/B,web-shell E2E Smoke,TUI parity snapshots (ink vs opentui),OpenTUI no-flicker gateand bothDesktop Shelllanes. (My report 46 minutes earlier quoted a product-lane subset with a different denominator; the polarity is identical — nothing failing, nothing still running.) doudouOUC's approval is live at this head: id5167178644, submitted2026-09-10T12:40:50Z, which is after the head commit's own committer date2026-09-10T10:07:50Z— so it is genuinely anchored tod515b9aand not re-anchored onto it by a server-side base merge. That approval is read-verified against this SHA and walks all four previously-unresolved Criticals.qwen-code-ci-botround 8 (id5168585669,COMMENTED,2026-09-10T14:47:05Z, landed after my report) carries zero Criticals — its own machine ledger reads{"round":8,"findings":[],"posted":0,"floor":"c","fresh":0}, and its prose states "Advisory only: this does not affect the verdict, and nothing here is a blocker." The six findings it lists are all Suggestion-level and all already reported on this PR.
What my own report contributed beyond the existing approvals: an executed two-arm daemon A/B on a live qwen serve (fix arm vs. revert control under identical polling), which is what established that persisted MCP config is actually discovered at boot rather than only on first session — the behaviour this PR exists to change.
Scope of this approval, stated so it does not over-claim: it covers the code at d515b9a and the paths my report exercised (serve-mode boot, a trusted single primary workspace, one stdio MCP server). It does not cover untrusted workspaces (unreachable by design — shouldPreheat is cleared before anything preheats), secondary/multi-workspace runtimes, HTTP-transport MCP servers, or the four skip branches, each of which is unit-tested instead.
The bot's non-blocking Suggestions and deferred items (the dead EnsureOptions.skipKeepAlivePreheat switch, the discarded ensure() status, the one skip string flattening five WorkspaceEntryState cases, and the stale WorkspaceDrainingError sentence in the PR description) are all real and all below a merge blocker; I agree with doudouOUC that they can land as follow-ups.
7 participants
What this PR does
After ACP preheat in
qwen serve, fire-and-forgetcoordinator.ensure({ keepAliveMs: ENSURE_KEEP_ALIVE_MS })on the primary workspace runtime so workspace-scope MCP discovery can start at daemon boot viaprepareMcpRevision()/initializeWorkspaceMcp(), on the separate workspace discovery Config.Primary runtime is resolved with the non-throwing
registry?.primaryEntry.current?.runtimelookup so a closed generation cannot flip a successful preheat tofailed.Why it's needed
On
main, nothing starts workspace-scope MCP discovery at daemon boot. The ACP bootstrap Config is initialized withskipMcpDiscovery: true, and the dedicated workspace discovery Config is only created from HTTP control paths /prepareMcpRevision(). Right after preheat,discoveryStatestaysnot_started.Clarification vs an earlier draft of this description: persisted MCP servers are still listed after preheat, because workspace status falls back to the bootstrap Config's
getMcpServers(). What is missing is discovery and connection state (discoveryState: 'not_started'), not an empty server list.This is not the Qwen Desktop bug in #7771 (Electron IPC /
settings.json). It only addresses the analogous boot gap in the CLIqwen servedaemon.Reviewer Test Plan
How to verify
mcpServersentry the serve daemon should know about.qwen serveand, before creating a session or opening Web Shell, fetch workspace MCP status.discoveryStateremainsnot_started(no workspace discovery Config yet).ensure({ keepAliveMs: ENSURE_KEEP_ALIVE_MS })drives workspace discovery initialization on the separate discovery Config (bootstrap Config stays on the W119 skip-discovery path).Unit coverage:
packages/cli/src/serve/run-qwen-serve.test.tsassertsensure({ keepAliveMs: ENSURE_KEEP_ALIVE_MS })after successful preheat, and goes red if discovery runs on preheat failure, non-primary workspaces, shutdown, or runtime startup failure.Evidence (Before & After)
Before: successful ACP preheat left workspace MCP discovery at
not_starteduntil an HTTP ensure/reconcile path ran.After:
ensure({ keepAliveMs: ENSURE_KEEP_ALIVE_MS })is scheduled after preheat using a non-throwing primary lookup; unit test asserts that call. No live daemon JSON capture in this pass (fork CI still oftenaction_requiredfor first-time contributors). N/A for TUI screenshots.Tested on
Environment (optional)
(18 passed in that describe block)
Risk & Scope
Boot calls
ensure({ keepAliveMs: ENSURE_KEEP_ALIVE_MS })after a successful ACP preheat. That deliberately arms the same daemon-wide 10-minute keep-alive window the HTTPPOST /workspace/runtime/ensureroute uses, so the already-warm ACP child stays held through Skills/MCP preparation under the defaultchannelIdleTimeoutMs=0policy. A boot that serves zero sessions therefore reaps the warm child and its discovery Config at boot+10min unless a session or another keep-alive extends it; the configured--channel-idle-timeout-msreclaim does not apply inside that window becausekeepAliveUntilonly extends the effective idle timeout.Boot
ensure()also runsprepareSkills()— not MCP-only. Skip paths (shutting down,runtime startup failed, no primary runtime, unsupported lifecycle) log viadaemonLog.infoand are also teed to stderr for operator visibility. Preparation failures log viadaemonLog.warn(with deepesterr.causewhen present) and are teed to stderr. OrderlyWorkspaceDrainingErroris not treated as a discovery failure.EnsureOptions.skipKeepAlivePreheatremains for callers that pass the object form withoutkeepAliveMs; boot always passeskeepAliveMsso the skip path is not taken at startup.Linked Issues
Relates to #7771 (Desktop issue; this PR does not close it — serve-daemon analogue only)
中文说明
这个 PR 做了什么
在
qwen serve的 ACP preheat 成功后，对 primary workspace runtime fire-and-forget 调用coordinator.ensure({ keepAliveMs: ENSURE_KEEP_ALIVE_MS })，通过prepareMcpRevision()/initializeWorkspaceMcp()在独立的 workspace discovery Config 上启动 discovery。primary 用不会抛错的
registry?.primaryEntry.current?.runtime解析，避免 generation 已关闭时把成功的 preheat 改写成failed。为什么需要
main上守护进程启动时不会启动 workspace 级 MCP discovery：bootstrap Config 带skipMcpDiscovery: true，专用 discovery Config 只从 HTTP /prepareMcpRevision()创建。preheat 之后discoveryState仍是not_started。相对更早一版描述的澄清：持久化的 MCP server 仍会经 bootstrap Config 回落出现在列表里；缺的是 discovery/连接状态，不是空列表。
这与 #7771（Qwen Desktop）不是同一条产品路径；本 PR 只覆盖 CLI
qwen serve。评审人验证计划
如何验证
mcpServers。qwen serve，在建 session / 开 Web Shell 前拉 workspace MCP 状态。discoveryState为not_started。ensure({ keepAliveMs: ENSURE_KEEP_ALIVE_MS })推动 workspace discovery 初始化；bootstrap 仍走 W119 跳过 discovery。单测断言 preheat 后调用
ensure()，mock 仅挂在本用例。证据（前后对比）
前：preheat 后 discovery 停在
not_started。后：ensure()+ 安全 primary 查找；本轮为本地单测，无真实守护进程 JSON。非 TUI。测试平台
Linux ✅；macOS/Windows N/A。
风险与范围
ensure({ keepAliveMs: ENSURE_KEEP_ALIVE_MS })，有意挂上与 HTTP ensure 相同的守护进程级 10 分钟 keep-alive。ensure()也会跑prepareSkills()，不只是 MCP。跳过路径（关闭中、runtime 启动已失败、无 primary、不支持 lifecycle）经daemonLog.info记录，并 tee 到 stderr；准备失败经daemonLog.warn（展开最深层 cause）并 tee 到 stderr。有序WorkspaceDrainingError不当作 discovery 失败告警。EnsureOptions.skipKeepAlivePreheat保留给未传keepAliveMs的对象形式调用方；boot 始终传keepAliveMs。关联 Issue
Relates to #7771（不自动关闭 Desktop issue）