fix(serve): Optimize daemon NDJSON stream handling - #6263
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Thanks for the PR! Template looks good ✓ On direction: daemon NDJSON parsing performance and baseline observability (event-loop lag, pipe byte histograms) are squarely within the project's core infrastructure needs. Claude Code's own CHANGELOG shows event-loop stall fixes and startup perf work — this area is clearly relevant. No direct reference for NDJSON streaming, but the problem (quadratic re-split on growing buffers) is a real and well-understood class of issue. On approach: the scope is well-contained. Three focused pieces — local incremental NDJSON stream, event-loop lag monitor via Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 方向：daemon NDJSON 解析性能和基础可观测性（event-loop lag、pipe byte 直方图）属于项目核心基础设施范畴。Claude Code 自己的 CHANGELOG 中也有 event-loop stall 修复和启动性能优化——这个方向明确相关。NDJSON 流式处理没有直接参考，但问题（对增长中的 buffer 重复 split 导致二次复杂度）是一类已知且真实的性能问题。 方案：范围收敛良好。三个聚焦的模块——本地增量 NDJSON stream、基于 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
There was a problem hiding this comment.
Pull request overview
This PR optimizes the daemon ↔ ACP-child NDJSON stdio transport by introducing a local incremental NDJSON scanner (to avoid repeated whole-buffer splits) and adds new observability for daemon/ACP event-loop lag and daemon-child pipe message sizes, surfacing a daemon-only perf snapshot via /daemon/status.
Changes:
- Add a local
ndJsonStreamimplementation (with byte-count hooks) and thread pipe hooks through spawned ACP channels. - Introduce event-loop lag monitoring + OpenTelemetry gauges for daemon and ACP roles, plus a daemon pipe payload-size histogram.
- Extend
/daemon/status(and docs) with an optional, daemon-onlyruntime.perfsnapshot.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core/src/telemetry/index.ts | Re-exports new event-loop lag utilities and daemon pipe message metric API. |
| packages/core/src/telemetry/event-loop-lag.ts | Adds monitorEventLoopDelay-based lag monitor with snapshot + stall callback. |
| packages/core/src/telemetry/event-loop-lag.test.ts | Tests event-loop lag monitor snapshot conversion and stall reporting behavior. |
| packages/core/src/telemetry/event-loop-lag-metrics.ts | Adds OTel observable gauges for daemon/ACP event-loop lag with stat attrs. |
| packages/core/src/telemetry/event-loop-lag-metrics.test.ts | Tests gauge registration and observation attributes/error swallowing. |
| packages/core/src/telemetry/daemon-metrics.ts | Adds daemon pipe message bytes histogram + recordDaemonPipeMessage. |
| packages/core/src/telemetry/daemon-metrics.test.ts | Tests daemon pipe message byte recording + metric initialization wiring. |
| packages/cli/vitest.config.ts | Adds CLI test alias for new @qwen-code/acp-bridge/ndJsonStream entrypoint. |
| packages/cli/src/serve/server.ts | Wires optional getPerfSnapshot through serve app deps for status route. |
| packages/cli/src/serve/run-qwen-serve.ts | Starts daemon event-loop monitor, registers gauge, records pipe stats, injects perf snapshot provider, disposes on shutdown. |
| packages/cli/src/serve/run-qwen-serve.test.ts | Adds coverage that daemon event-loop monitor is disposed on close. |
| packages/cli/src/serve/routes/daemon-status.ts | Threads getPerfSnapshot into daemon status response builder. |
| packages/cli/src/serve/daemon-status.ts | Defines DaemonPerfSnapshot and conditionally includes runtime.perf in status response. |
| packages/cli/src/serve/daemon-status.test.ts | Tests inclusion/omission of runtime.perf in /daemon/status response shape. |
| packages/cli/src/acp-integration/acpAgent.worktree.test.ts | Updates mocks to use local NDJSON stream and adds event-loop lag monitor mocks. |
| packages/cli/src/acp-integration/acpAgent.ts | Switches stdio NDJSON to local implementation and adds ACP event-loop lag monitoring + gauge registration with cleanup. |
| packages/cli/src/acp-integration/acpAgent.test.ts | Tests event-loop monitor registration/disposal and setup-failure cleanup. |
| packages/acp-bridge/src/spawnChannel.ts | Switches spawned child stdio NDJSON to local stream and adds optional pipe hooks. |
| packages/acp-bridge/src/spawnChannel.test.ts | Verifies NDJSON pipe hooks record sent/received payload byte sizes. |
| packages/acp-bridge/src/ndJsonStream.ts | New incremental NDJSON scanner stream with hookable sent/received payload byte counts. |
| packages/acp-bridge/src/ndJsonStream.test.ts | Tests framing, large split messages, UTF-8 boundaries, errors, EOF tail drop, hooks, and write-error propagation. |
| packages/acp-bridge/src/index.ts | Exports ndJsonStream from the acp-bridge package surface. |
| packages/acp-bridge/package.json | Adds package export entry for ./ndJsonStream. |
| docs/developers/qwen-serve-protocol.md | Documents optional runtime.perf shape on /daemon/status. |
| docs/developers/daemon/19-observability.md | Documents new OTel metrics and daemon-only perf snapshot behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } catch (err) { | ||
| // eslint-disable-next-line no-console -- match ACP SDK parse-error behavior | ||
| console.error('Failed to parse JSON message:', trimmedLine, err); | ||
| } |
| // Stdout is used to send messages to the client, so console.log/console.info | ||
| // messages to stderr so that they don't interfere with ACP. | ||
| console.log = console.error; | ||
| console.info = console.error; | ||
| console.debug = console.error; | ||
|
|
||
| const stream = ndJsonStream(stdout, stdin); | ||
| connection = new AgentSideConnection((conn) => { | ||
| acpConnection = conn; | ||
| agentInstance = new QwenAgent(config, settings, argv, conn); | ||
| return agentInstance; | ||
| }, stream); | ||
| } catch (err) { | ||
| eventLoopMonitor.dispose(); | ||
| throw err; |
Code ReviewThe implementation is clean and follows project conventions. Three new modules ( Key observations:
No critical blockers or AGENTS.md violations found. TestingBuild & typecheck: both pass cleanly. Unit tests (all green):
Before (installed v0.19.1)After (this PR, v0.19.5)The 中文说明代码审查实现干净，符合项目规范。三个新模块（ 关键观察：
未发现关键阻塞项或 AGENTS.md 违规。 测试构建 & 类型检查： 均通过。 单元测试（全部通过）：ndJsonStream 10/10，spawnChannel 28/28，event-loop-lag 5/5，event-loop-lag-metrics 4/4，daemon-metrics 19/19，daemon-status 11/11。 真实场景测试： 已安装版本 (v0.19.1) 的 — Qwen Code · qwen3.7-max |
|
This PR does exactly what it says, does it well, and doesn't do anything it shouldn't. The quadratic NDJSON re-split problem is real and the fix is minimal — a local incremental scanner that only touches the daemon hot path while leaving the SDK implementation untouched for other consumers. The event-loop lag and pipe byte observability fills a genuine gap (the docs previously listed "no metrics endpoint" as a known gap; this doesn't add Prometheus but it does add OTel gauges and a status-endpoint snapshot, which is the right incremental step). The implementation is straightforward: ~300 lines of new production code across three focused modules, each with thorough tests covering edge cases (UTF-8 splits, CRLF, hook errors, EOF tails, idempotent registration). The daemon wiring correctly disposes on both failure and normal shutdown. The before/after testing confirms the No concerns. Ships cleanly. 中文说明本 PR 准确实现了所述功能，没有多余内容。 NDJSON 二次 re-split 问题是真实的，修复也很精简——仅在 daemon 热路径上使用本地增量扫描器，其他消费方保持 SDK 实现不变。event-loop lag 和 pipe 字节可观测性填补了一个真实的空白（文档此前将"无指标端点"列为已知缺失；本 PR 没有加 Prometheus，但加了 OTel gauges 和 status 快照，是正确的增量步骤）。 实现简洁：三个聚焦模块共约 300 行生产代码，每个都有充分的边界测试（UTF-8 拆分、CRLF、hook 错误、EOF 尾巴、幂等注册）。daemon 接线在失败和正常关闭路径上都正确 dispose。Before/after 测试确认 无顾虑，可以合入。 — Qwen Code · qwen3.7-max |
| readChunk(value, pending, controller, textDecoder, hooks); | ||
| } | ||
| } finally { | ||
| reader.releaseLock(); |
There was a problem hiding this comment.
[Suggestion] Input stream errors are silently converted to EOF. The start() method uses try/finally with controller.close() but no catch block. If reader.read() throws (e.g., broken pipe), the finally block closes the controller before the async function rejects — and ReadableStream ignores the start() promise rejection. The consumer sees a clean EOF instead of an error, and any messages after the failure point are silently lost.
| reader.releaseLock(); | |
| } catch (err) { | |
| controller.error(err); | |
| return; | |
| } finally { | |
| reader.releaseLock(); | |
| controller.close(); | |
| } |
— qwen3.7-max via Qwen Code /review
| /* event loop monitoring must not break the process */ | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
[Suggestion] The stall notification is suppressed permanently after a single large stall. histogram.max is monotonically non-decreasing (the all-time peak, never reset), so once lastReportedMaxMs is set to a large value (e.g., 5000ms during startup), all subsequent stalls below that threshold — even recurring 2-3 second stalls that degrade user-facing latency — produce no log line or callback. Consider periodically resetting the histogram and lastReportedMaxMs (e.g., every 60 seconds) so the stall callback reports against recent behavior rather than all-time highs.
— qwen3.7-max via Qwen Code /review
| dispose(): void { | ||
| if (disposed) return; | ||
| disposed = true; | ||
| if (interval) { |
There was a problem hiding this comment.
[Suggestion] snapshot() returns lifetime statistics — meanMs converges toward a constant over the process lifetime, and maxMs is the all-time peak. Neither reflects recent event-loop behavior. Operators relying on p50Ms or meanMs via /daemon/status or the OTel gauge will see values that become increasingly stale and unresponsive to current conditions. Consider calling histogram.reset() after reading values (making each observation interval independent), or documenting the lifetime semantics prominently so consumers know to compute deltas between consecutive snapshots.
— 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.
No review findings. Downgraded from Approve to Comment: CI still running.
— qwen3.7-max via Qwen Code /review
|
@qwen-code /triage |
Maintainer verification — local build, benchmark & live E2E ✅I re-built this PR from head Gate results
1. The headline claim is real — and measuredThe old path used the SDK's I fed one large ACP message through both real parsers (SDK
2. Drop-in safe — contract is byte-identical to the SDK parserA faster parser is only safe if it frames identically. I ran 10 adversarial framing cases through both parsers and diffed the emitted messages and the 3. Live daemon E2E (real bundle, real spawned child, real pipe)Stood up the real daemon; it spawned a genuine
4. On the automated-review flags — verified, none are blockers
Bottom lineFrom a correctness and performance standpoint this PR is verified sound: build/typecheck/lint/375 tests green, the new parser is byte-identical to the SDK on every framing edge case, and the O(n²)→O(n) win is real and measured (up to 166× on the isolation benchmark). The new surface ( 中文说明（完整对应）Maintainer 验证 — 本地构建、基准测试与真实 E2E ✅我在干净的 worktree 里从 PR head 门禁结果
1. 头号声明是真的 — 而且被实测出来了旧路径用 SDK 的 我把一个大 ACP 消息拆成很多个 pipe 大小的小 chunk，喂给两个真实解析器（SDK
2. 可安全直接替换 — 契约与 SDK 解析器逐字节一致更快的解析器只有在 framing 完全一致时才安全。我用 10 个对抗性 framing 用例跑两个解析器，并 diff 了产出的消息和 3. 真实 daemon E2E（真实 bundle、真实 spawned child、真实 pipe）起了真实 daemon，它 spawn 了真正的
4. 关于自动 review 的标记 — 已核验，均非 blocker
结论从正确性与性能角度，本 PR 已验证可靠：build/typecheck/lint/375 测试全绿，新解析器在每个 framing 边界用例上与 SDK 逐字节一致，O(n²)→O(n) 的收益真实且实测（隔离基准上最高 166×）。新增表面（ |
|
Thanks for the direction check. I’m going to keep this PR intact and continue with the full PR1 scope. The reason is that this change is intentionally framed as “NDJSON O(n) receive + daemon performance baseline”, not just the parser swap. The parser fix removes the hot-path O(n²) receive behavior, while the daemon/child event-loop lag and daemon pipe-size metrics provide the baseline needed to validate and diagnose this path after rollout. The public surface is intentionally narrow: no ACP ext-method is added, child lag is not exposed through I agree that the public |
|
@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. ✅
|
Benchmark filling the "Before & After" evidence gap: one NDJSON-framed message fed through the receive side in 64 KiB chunks, timed until parsed (median of 7 runs, Node v24.12.0, Apple Silicon).
The SDK column grows ~quadratically with message size (5→10 MB: 2x size, ~3.8x time) because each incoming chunk re-scans and re-copies the whole buffered content; the replacement scans only the new chunk, so it stays linear. Also reported upstream: agentclientprotocol/typescript-sdk#206 🤖 Generated with Qwen Code |
…ation (QwenLM#6310) * perf(cli): cache LoadedSettings per workspace with stat-based invalidation The ACP child under `qwen serve` is long-lived and re-runs a full loadSettings() on the shared event loop for every session/new, session/load and session/resume: four settings files read, parsed, migration-checked and structuredClone'd, the .env tree walked, home .env re-read, ${VAR} references re-resolved, and all scopes merged. Same-cwd repeat sessions (the typical serve workload) pay full price every time. Add a process-level cache keyed by resolved workspace dir (LRU 64). Freshness is checked deterministically on every access via a fingerprint of every filesystem input: stat signatures (mtimeMs:size:ino) of the four settings files, the re-discovered .env file list with signatures, IDE trust, realpath(cwd) and realpath(homedir). Any change -> full reload; fingerprint errors fail open to a reload; loadSettings() throws propagate uncached. Only the three hot ACP session handlers switch to loadSettingsCached(); all other loadSettings() callers (ext-methods write paths etc.) keep their direct read semantics. Known accepted differences (documented in the module doc): direct process.env mutation without any file change does not re-bake ${VAR} references on a hit; a .env edit racing the miss-path load itself is the usual mtime-cache TOCTOU microsecond window; an in-place overwrite preserving mtime+size+ino is invisible (self-writes go through temp+rename, which changes the inode). Part of the qwen serve multi-session performance work (QwenLM#6263). 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * test(cli): add IDE trust flip invalidation test for settings cache Covers the ideTrust fingerprint component, which is the only trust input that can change within a live process (trustedFolders.json is a permanent singleton, folder-trust toggles live in the settings files). Addresses a Copilot review suggestion to guard against stale-cache trust regressions. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * refactor(cli): harden settings cache observability and fail-open coverage Addresses three review suggestions: - Warn comment on settingsFileSigs that the 4-scope path list must stay in sync with loadSettings() (unlike envFileSigs, it is enumerated separately). - Add a createDebugLogger('SETTINGS_CACHE'), matching the SETTINGS / SETTINGS_WATCHER / CONFIG convention in neighbouring config modules, and log hit/miss, each fail-open catch (with the swallowed error), and eviction. - Add a fault-injection test asserting the cache reloads (never throws) when the fingerprint check fails, then recovers once the fault clears. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
What this PR does
This PR replaces the daemon-critical NDJSON stdio path between qwen serve and qwen --acp with a local incremental scanner that preserves the SDK stream contract while avoiding repeated whole-buffer split work on growing pending input. It wires daemon-only pipe hooks into the spawned child channel, records daemon child-pipe message sizes, starts event-loop lag monitors for the daemon and ACP child, exposes daemon-only performance snapshots in /daemon/status, and documents the new observability surface.
Why it's needed
Large ACP messages that span chunks could make receive-side work grow quadratically because pending data was repeatedly split as new chunks arrived. The daemon also lacked baseline visibility into event-loop lag and daemon-child pipe payload size, which made overload diagnosis harder. This keeps protocol behavior stable while making the hot daemon path linear and observable without adding ACP ext-methods or exposing child lag in /daemon/status.
Reviewer Test Plan
How to verify
Reviewers should confirm the daemon/child ACP stdio path now uses the local incremental NDJSON stream while non-daemon paths keep the SDK stream implementation. The focused ACP bridge tests verify empty-line handling, invalid JSON continuation, EOF tail dropping, UTF-8 split boundaries, payload byte hooks, newline framing, and write-error propagation. The core telemetry tests verify daemon/ACP event-loop gauges and daemon pipe histograms register with the expected metric names and attributes. The CLI tests verify ACP agent monitor cleanup, daemon monitor cleanup, and the additive /daemon/status perf shape.
Evidence (Before & After)
N/A
Tested on
Environment (optional)
Node.js 22+ local development checkout on macOS. Validation used npm run build, npm run typecheck, focused Vitest suites for packages/acp-bridge, packages/core, and packages/cli, changed-file ESLint, and git diff --cached --check.
Risk & Scope
Linked Issues
N/A
中文说明
本 PR 做了什么
本 PR 将 qwen serve daemon 与 qwen --acp child 之间的关键 NDJSON stdio 通路替换为本地增量扫描实现，在保持 SDK stream 契约兼容的同时，避免对不断增长的 pending 输入重复执行整段 split。它为 daemon 侧 spawned child channel 接入 pipe hooks，记录 daemon-child pipe 消息大小，启动 daemon 和 ACP child 的 event-loop lag monitor，在 /daemon/status 中暴露仅 daemon 进程的性能快照，并补充相关观测文档。
为什么需要
跨 chunk 的大型 ACP 消息过去可能让接收端工作量呈二次增长，因为每来一个新 chunk 都会对 pending 数据重复 split。daemon 也缺少 event-loop lag 和 daemon-child pipe payload size 的基础可见性，导致过载诊断更困难。这个改动在不新增 ACP ext-method、也不把 child lag 暴露到 /daemon/status 的前提下，保持协议行为稳定，同时让 daemon 热路径变为线性并具备基础观测能力。
审阅测试计划
如何验证
审阅者应确认 daemon/child ACP stdio 通路现在使用本地增量 NDJSON stream，而非 daemon 路径仍继续使用 SDK stream 实现。ACP bridge focused tests 验证空行处理、非法 JSON 后继续、EOF 尾巴丢弃、UTF-8 跨 chunk 边界、payload byte hooks、newline framing 和写失败传播。core telemetry tests 验证 daemon/ACP event-loop gauges 和 daemon pipe histograms 使用预期的指标名与 attributes 注册。CLI tests 验证 ACP agent monitor cleanup、daemon monitor cleanup，以及 /daemon/status 中 additive perf 字段形状。
证据（Before & After）
N/A
已测试平台
环境（可选）
macOS 本地 Node.js 22+ 开发 checkout。验证使用了 npm run build、npm run typecheck、packages/acp-bridge、packages/core 和 packages/cli 的 focused Vitest suites、changed-file ESLint，以及 git diff --cached --check。
风险与范围
关联 Issues
N/A