fix(cli): Keep serve health responsive before runtime load - #6013
Conversation
E2E / startup validation reportBranch: Validation commands run in
Bundled
Expected behavior confirmed: the bootstrap app can return the first healthy |
|
Thanks for the PR! Template looks good ✓ — all required sections present with bilingual translation. On direction: this is a well-motivated performance optimization that directly follows up on PR #5995. Keeping the serve health endpoint responsive before the heavy runtime graph loads is a clear win for orchestrators and health-check-based deployments. The approach — defer runtime until the first On approach: the scope feels right for the stated goal. Each piece serves a clear purpose: the Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ — 所有必需章节齐全，含双语翻译。 方向：这是一个动机明确的性能优化，直接跟进 PR #5995。在重型 runtime graph 加载前保持 serve health endpoint 可响应，对编排器和基于健康检查的部署是明确的收益。延迟 runtime 直到首次 方案：范围与目标匹配。每个组件都有明确用途： 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewIndependent proposal before reading the diff: I'd have added a flag to The PR's approach matches this and exceeds it — the delegating serve app now also handles request-triggered runtime starts for non-bootstrap routes (with auth gating), which I didn't initially consider but is clearly needed for clients that skip health probes. No correctness bugs, security holes, or regressions found. The Test coverage is comprehensive: 81 tests in Real-Scenario TestingBundled Key observations:
Unit test run (worktree, after build): 中文说明代码审查独立方案：我会给 PR 的方案匹配并超越了此方案——delegating serve app 还处理了非 bootstrap 路由触发的 runtime 启动（带 auth 验证），这是我最初没考虑到的，但对跳过 health 探活的客户端来说显然必要。 未发现正确性 bug、安全漏洞或回归。 实际场景测试用 PR 代码 bundle 的
单元测试：4 个文件 152 个测试全部通过。 — Qwen Code · qwen3.7-max |
|
This PR delivers exactly what it promises: the serve fast path stays responsive for the first health probe by deferring the heavy runtime graph, with a clean fallback timer for no-probe scenarios and proper shutdown cancellation. Going back to my independent proposal — the PR's approach matches and exceeds it. The addition of request-triggered runtime starts (with auth gating) for non-bootstrap routes is a thoughtful touch that covers clients who skip health probes entirely. The The implementation is straightforward despite the complexity of the problem space. Timer lifecycle is well-managed, edge cases (shutdown before health, shutdown after health, duplicate probes, auth failures, startup failures with no retry) are all covered by both code and tests. 152 unit tests pass. Real-scenario testing confirms the deferred runtime pattern works as designed — fallback timer fires at ~1s, health responds in single-digit milliseconds, clean SIGTERM handling. The diff is focused: every change serves the stated goal. No drive-by refactors, no speculative features. The ~1190 additions are dominated by test code (~900 lines of new tests), which is the right ratio for a state-machine change like this. Approving. ✅ 中文说明这个 PR 完全兑现了承诺：通过延迟重型 runtime graph，serve fast path 在首次 health 探活时保持可响应，同时为无探活场景提供干净的 fallback timer 和正确的 shutdown 取消逻辑。 回顾我的独立方案——PR 的方案匹配并超越了它。为非 bootstrap 路由添加请求触发的 runtime 启动（带 auth 验证）是一个周到的设计，覆盖了跳过 health 探活的客户端。 尽管问题空间复杂，实现直截了当。Timer 生命周期管理良好，边界情况（health 前关闭、health 后关闭、重复探活、auth 失败、启动失败不重试）都由代码和测试覆盖。152 个单元测试通过。实际场景测试确认 deferred runtime 模式按设计工作。 Diff 聚焦：每个改动都服务于目标。无无关重构，无投机性功能。~1190 行新增中约 900 行是测试代码——对于状态机变更来说这是正确的比例。 批准合并 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
@qwen-code /review |
| _Qwen Code review request accepted. Review is queued in [workflow run](https://github.com/QwenLM/qwen-code/actions/runs/28375932842)._ |
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No critical issues found. The deferred-runtime startup mechanism is correctly implemented with proper timer lifecycle management and idempotency guards. Two non-blocking suggestions below.
— qwen3.7-max via Qwen Code /review
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Qwen Code review did not complete successfully. Qwen review aborted with an API error before posting comments. See workflow logs. |
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No critical issues found. The deferred-runtime startup mechanism is correctly implemented with proper timer lifecycle management and idempotency guards. All 147 tests pass and no new type errors were introduced. Three suggestions below for consideration.
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
✅ Local real-daemon verification (maintainer)Built a real bundled Environment: macOS (Darwin 25.5), Node v22.22.2 · isolated A/B method: Build A = PR as-is ( 1. First-
|
| Build | listen → first /health 200 (isolates the effect) |
spawn → first /health 200 |
defer markers |
|---|---|---|---|
| A — defer ON (this PR) | median 3 ms (2–4) | median 299 ms | YES ✓ |
| B — defer OFF (baseline) | median 237 ms (233–261) | median 529 ms | NO ✓ |
→ ~79× / −234 ms on the isolated listen→/health metric; the spawn→/health delta (−230 ms) corroborates (node cold-start is constant across A/B). In the baseline the first /health is blocked behind the runtime-graph mount (core module eval + bridge creation) that runs right after listen; this PR defers that until after the health response flushes. Direction & mechanism match the PR's claim (the absolute "before" is OS-dependent — larger on the Linux host cited in the PR).
2. Functional behavior — real daemon, tmux, daemon debug-log markers
| Scenario | Result |
|---|---|
| Health-triggered start | first /health → 200 {"status":"ok"} in 4 ms; log: scheduling fallback → health served, scheduling start in 50ms → health timer fired, starting (+51 ms ≈ FAST_PATH_RUNTIME_START_AFTER_HEALTH_MS); fallback did not fire ✓ |
| Fallback (no probe) | scheduling fallback start in 1000ms → fallback timer fired, starting (+1001 ms ≈ FAST_PATH_RUNTIME_START_FALLBACK_MS); runtime starts with no probe ✓ |
| Close before first health | SIGTERM → received SIGTERM, draining → deferred runtime: cancelled, server closed before startup; no runtime-start timer fired; clean drain ✓ |
| First runtime route | GET /workspace/agents → 200 (221 ms) with real data — not the bootstrap daemon_runtime_starting 503; it triggered the deferred start, awaited mount, and the runtime served it; runtime reached READY (session reaper started + /acp WebSocket transport enabled) ✓ |
3. Static checks & tests
- Import-boundary guard
npm run check:serve-fast-path-bundle→Serve fast-path bundle closure check passed(the fast path still does not pull core into the pre-listen closure). - Unit tests (vitest, real build): 147 passed across
run-qwen-serve.test.ts(76),fast-path.test.ts,fast-path-open.test.ts,headlessSafetyWarnings.test.ts— incl. the 3 new deferred-runtime tests (keeps health responsive…,…fallback…,…close before first health). - Typecheck:
cliexit 0,coreexit 0. - Headless YOLO warning: still emitted on the fast path (observed live). The
headlessSafetyWarningsrefactor is sound — confirmedApprovalMode.YOLO === 'yolo'in core, and the helper no longer imports core at runtime (guard + regex test cover it).
4. One non-blocking observation (LOW — optional follow-up)
isBootstrapServeRoute() matches the path exactly (req.path === '/health'), but Express non-strict routing still routes a trailing-slash GET /health/ to the health handler. Empirically on build A:
GET /health→ 200 in 6 ms (full deferred sequence, fast path)GET /health/→ 200{"status":"ok"}but in 723 ms — classified as a non-bootstrap route, so it eagerly started the runtime and waited for the full mount before responding.
Same applies to /capabilities/ and /daemon/status/. The response is still correct, but the latency optimization is silently lost for the trailing-slash variant. Since the whole PR targets health-probe latency, consider normalizing the trailing slash in isBootstrapServeRoute. Not a merge blocker — canonical probes use /health.
Verdict
The PR does what it claims. The mechanism is correct and well-guarded (single runtimeStarting master guard against double-start; all three timers unref()'d and cleared on close; cancel-on-close path proven), the A/B confirms the first-/health improvement on a real daemon, and the new tests pass. Recommend merge, with the trailing-slash item as an optional follow-up.
🇨🇳 中文版（完整对应）
✅ 本地真实 daemon 验证（维护者）
在隔离 worktree 里基于 PR head（a55a21740，其 base main 已含 #5995）构建了真实打包的 dist/cli.js，并在 tmux 下驱动真实 qwen serve daemon。权威证据是 daemon 自身的调试日志 marker + 真实二进制上的 A/B 基准，而非客户端打印。
环境： macOS（Darwin 25.5）、Node v22.22.2 · 独立 npm ci worktree · 代码分割 bundle（serve fast path 落在独立 chunk）· loopback 绑定（--no-web，免 token）· QWEN_DEBUG_LOG_FILE=1。
A/B 方法： 构建 A = PR 原样（deferRuntimeUntilFirstHealth: !parsed.open → 开）。构建 B = 仅改一行为 deferRuntimeUntilFirstHealth: false（PR 前基线），重新打包成不同 chunk 哈希。其余完全一致。
1. 首个 /health 延迟 —— 真实二进制 A/B（各 12 次，丢弃 2 次预热）
| 构建 | listen → 首个 /health 200（隔离效应） |
spawn → 首个 /health 200 |
defer marker |
|---|---|---|---|
| A — defer 开（本 PR） | 中位 3 ms（2–4） | 中位 299 ms | YES ✓ |
| B — defer 关（基线） | 中位 237 ms（233–261） | 中位 529 ms | NO ✓ |
→ 隔离指标 listen→/health 约 79×、减少 234 ms；spawn→/health 的 delta（−230 ms）相互印证（node 冷启动对 A/B 恒定）。基线下首个 /health 被 listen 后立即执行的 runtime graph mount（core 模块求值 + bridge 创建）阻塞；本 PR 把这部分推迟到 health 响应 flush 之后。方向与机制与 PR 主张一致（"before" 的绝对值随 OS 不同——PR 引用的 Linux 机更大）。
2. 功能行为 —— 真实 daemon、tmux、daemon 调试日志 marker
| 场景 | 结果 |
|---|---|
| health 触发启动 | 首个 /health → 200 {"status":"ok"}，4 ms；日志：scheduling fallback → health served, scheduling start in 50ms → health timer fired, starting（+51 ms ≈ FAST_PATH_RUNTIME_START_AFTER_HEALTH_MS）；fallback 未触发 ✓ |
| fallback（无探活） | scheduling fallback start in 1000ms → fallback timer fired, starting（+1001 ms ≈ FAST_PATH_RUNTIME_START_FALLBACK_MS）；无探活也启动 runtime ✓ |
| 首个 health 前关闭 | SIGTERM → received SIGTERM, draining → deferred runtime: cancelled, server closed before startup；无任何 runtime-start 定时器触发；优雅退出 ✓ |
| 首个 runtime 路由 | GET /workspace/agents → 200（221 ms） 返回真实数据——非 bootstrap daemon_runtime_starting 503；它触发了 deferred 启动、等待 mount，由 runtime 服务；runtime 达到 READY（session reaper started + /acp WebSocket transport enabled）✓ |
3. 静态检查与测试
- import 边界 guard
npm run check:serve-fast-path-bundle→Serve fast-path bundle closure check passed（fast path 仍未把 core 拉进 pre-listen 闭包）。 - 单元测试（vitest，真实构建）：
run-qwen-serve.test.ts(76)、fast-path.test.ts、fast-path-open.test.ts、headlessSafetyWarnings.test.ts共 147 通过——含 3 个新增 deferred-runtime 用例。 - Typecheck：
cliexit 0、coreexit 0。 - headless YOLO 警告：fast path 上仍正常输出（实测）。
headlessSafetyWarnings重构无误——已确认 core 中ApprovalMode.YOLO === 'yolo'，且 helper 运行时不再 import core（guard + 正则测试覆盖）。
4. 一个不阻塞合并的观察（LOW —— 可选后续）
isBootstrapServeRoute() 用精确匹配（req.path === '/health'），但 Express 非严格路由仍会把尾斜杠 GET /health/ 路由到 health handler。构建 A 上实测：
GET /health→ 6 ms 返回 200（完整 deferred 序列，走 fast path）GET /health/→ 200{"status":"ok"}但耗时 723 ms——被当作非 bootstrap 路由，于是提前启动 runtime 并等待整个 mount 完成才响应。
/capabilities/、/daemon/status/ 同理。响应仍正确，但尾斜杠变体悄悄丢掉了延迟优化。鉴于整个 PR 就是冲着 health 探活延迟去的，建议在 isBootstrapServeRoute 里归一化尾斜杠。非合并阻塞——规范探活用 /health。
结论
PR 行为与主张一致。机制正确且守卫完备（runtimeStarting 单一主闸防重复启动；三个定时器均 unref() 且在 close 时清理；close-before-health 取消路径已证）；A/B 在真实 daemon 上证实了首个 /health 的改善；新增测试全过。建议合并，尾斜杠一项作为可选后续。
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI failing (Post Coverage Comment).
The deferred-runtime startup mechanism is correctly implemented with proper timer lifecycle management, idempotency guards, and comprehensive test coverage (147/147 tests pass). Deterministic analysis (tsc + eslint) is clean.
— qwen3.7-max via Qwen Code /review
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Thanks for the update. I reviewed the latest deferred-startup flow and the added coverage around health/bootstrap handling, auth, CORS preflight, cancellation, and --open behavior; no blocking issues from my side.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No critical or suggestion issues found beyond what prior reviews already covered. Downgraded from Approve to Comment: CI still running. The deferred-runtime startup mechanism is well-engineered with proper timer lifecycle management, idempotency guards, auth gating for deferred routes, CORS preflight bypass, and comprehensive test coverage (152 tests passing). Three low-confidence observations for human review: (1) deferred-mode blocks non-bootstrap requests for up to 120s without per-request timeout — clients preferring fast-fail get no 503 until the global startup timeout, (2) no test exercises concurrent health probe + runtime route arrival, (3) the health-trigger callback is registered on every 200 health response for the server's lifetime even after runtime has started.
— qwen3.7-max via Qwen Code /review
What this PR does
This PR keeps the serve fast path responsive for the first
/healthprobe by deferring the heavier runtime graph until after a successful bootstrap health response has been flushed. It also gives the deferred path a fallback timer so the runtime still starts when no health probe arrives, and keeps the headless YOLO startup warning on the fast path without dynamically importing the core runtime before health can respond.Why it's needed
PR #5995 removes accidental pre-listen runtime closure from the serve bundle, but end-to-end
/healthcan still be delayed by work that runs immediately after the listener is ready. The runtime import/evaluation path and the headless warning's core import can monopolize the event loop before the first health response is observed, so clients can still wait around the old startup duration even thoughprocessToListenMsis low.Reviewer Test Plan
How to verify
Start from a build that includes PR #5995 and run
qwen serve --hostname 127.0.0.1 --port <free-port> --workspace /tmp --no-web, then pollGET /healthimmediately after process start. The first successful response should come from the bootstrap app with{"status":"ok"}before runtime routes are mounted, while the runtime should still mount shortly afterward via the health-triggered start or the fallback timer. Reviewers can also confirm that no health probe still starts runtime after the fallback delay, and that closing a handle before the first health response does not start runtime during shutdown.Evidence (Before & After)
Before: on the remote Linux verification host, PR #5995 lowered
processToListenMsto roughly 200-300ms but/healthstill took roughly 1.6s median because post-listen runtime work and the headless warning import could block the event loop before the first health response. After: with this follow-up applied and bundled, the remote benchmark returned first/healthresponses in 220, 244, 266, 282, and 287ms fornode dist/cli.js; the final local bundled benchmark returned 234, 122, 128, 135, and 126ms with a 128ms median.Tested on
Environment (optional)
macOS local PR worktree with Node v26.0.0 and npm 11.12.1. Validation included
npm install(which ran the repository prepare/build/bundle flow),npm run check:serve-fast-path-bundle,npm run typecheck,cd packages/cli && npx vitest run src/serve/run-qwen-serve.test.ts -t "runQwenServe runtime startup failures",cd packages/cli && npx vitest run src/serve/fast-path.test.ts src/utils/headlessSafetyWarnings.test.ts, and a bundleddist/cli.js/healthfirst-response benchmark. Linux validation was performed on the remote startup-performance host during the investigation using the same bundled benchmark shape on top of PR #5995.Risk & Scope
/healthis intentionally available before the full runtime graph is mounted, so clients that immediately call non-health runtime routes can still observe the existing bootstrap "runtime starting" response until runtime mounting completes.Linked Issues
Follow-up to #5995.
中文说明
What this PR does
这个 PR 让 serve fast path 在首次
/health探活时保持可响应：成功的 bootstrap health 响应 flush 之后，才启动较重的 runtime graph。它同时保留 fallback timer，确保没有 health 探活时 runtime 仍会启动，并让 headless YOLO 启动警告留在 fast path 上，但不再为了这个警告在 health 可响应前动态导入 core runtime。Why it's needed
PR #5995 移除了 serve bundle 中意外的 pre-listen runtime 闭包，但端到端
/health仍可能被 listener ready 之后立即执行的工作拖慢。runtime import/evaluation 路径以及 headless warning 的 core import 都可能在首次 health 响应被客户端观察到前占住事件循环，所以即使processToListenMs已经很低，客户端仍可能等待接近旧启动耗时。Reviewer Test Plan
How to verify
基于包含 PR #5995 的构建启动
qwen serve --hostname 127.0.0.1 --port <free-port> --workspace /tmp --no-web，然后在进程启动后立即轮询GET /health。首次成功响应应来自 bootstrap app，返回{"status":"ok"}，且发生在 runtime routes mount 之前；之后 runtime 应通过 health 触发启动或 fallback timer 正常 mount。Reviewer 也可以确认没有 health 探活时 runtime 会在 fallback 延迟后启动，并确认 handle 在首次 health 前关闭时不会在 shutdown 期间再启动 runtime。Evidence (Before & After)
Before：在远程 Linux 验证机器上，PR #5995 将
processToListenMs降到约 200-300ms，但/healthmedian 仍约 1.6s，因为 listen 后的 runtime work 和 headless warning import 仍可能在首次 health 响应前阻塞事件循环。After：叠加这个 follow-up 并打包后，远程 benchmark 中node dist/cli.js的首次/health响应为 220、244、266、282、287ms；最终本地 bundled benchmark 的结果为 234、122、128、135、126ms，中位数 128ms。Tested on
Environment (optional)
macOS 本地 PR worktree，Node v26.0.0，npm 11.12.1。验证包括
npm install（触发仓库 prepare/build/bundle 流程）、npm run check:serve-fast-path-bundle、npm run typecheck、cd packages/cli && npx vitest run src/serve/run-qwen-serve.test.ts -t "runQwenServe runtime startup failures"、cd packages/cli && npx vitest run src/serve/fast-path.test.ts src/utils/headlessSafetyWarnings.test.ts，以及 bundleddist/cli.js的/health首响 benchmark。Linux 验证是在远程启动性能机器上，调查过程中基于 PR #5995 用同样的 bundled benchmark 形态完成的。Risk & Scope
/health会在完整 runtime graph mount 前可用；如果客户端立刻调用非 health runtime 路由，仍会看到现有 bootstrap 的 "runtime starting" 响应，直到 runtime mount 完成。Linked Issues
#5995 的 follow-up。