feat(serve): bound daemon ACP NDJSON buffers - #8911
Merged
Merged
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Collaborator
Author
E2E test reportTested on macOS arm64 with Node.js 24.12.0 and npm 10.9.8.
Verification:
Known residuals: a legal 64 MiB frame still has transient decode/parse amplification, and the outbound limit is checked after stringify/encode. Both are documented and remain separate follow-up container work. |
11 tasks
Collaborator
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 4 scenario(s). — Qwen Code · serve A/B |
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
doudouOUC
marked this pull request as ready for review
August 11, 2026 13:46
doudouOUC
enabled auto-merge
August 11, 2026 13:46
Collaborator
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: real and already scheduled. The tracking issue #8091 explicitly lists "Bound ACP and transport containers" as the next increment of the daemon resource-protection series, and parent #8051 makes count + byte bounds on daemon-owned buffers a Phase 1 acceptance criterion. This isn't ad-hoc theoretical hardening — the PR also carries before/after memory measurements (author-run): a 96 MiB unterminated input canceled at ~65 MiB, and a 40,000-message stalled consumer cut to 256 admitted messages. Direction: aligned. #8051 Phase 1 names bounded NDJSON paths for Size: cross-package change ( Approach: scope feels right. The two unbounded buffers (frame accumulator, decoded-message queue) are genuinely distinct problems and both get bounded; using the platform Risk: no high-risk path matches (Stage 1e). This is daemon-critical transport plumbing, so review depth is full Stage 2 with CI evidence. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：真实存在且已排期。跟踪 issue #8091 明确将"Bound ACP and transport containers"列为 daemon 资源保护系列的下一个增量，父 issue #8051 也将"daemon 自有 buffer 的条数 + 字节上限"列为 Phase 1 验收标准。这不是临时起意的理论加固——PR 还附带了 before/after 内存测量（作者执行）：96 MiB 未终止输入在约 65 MiB 处被取消，40,000 条消息的慢消费者被限制为接纳 256 条。 方向：对齐。#8051 Phase 1 直接点名了 规模：跨包改动（ 方案：范围合理。两个无界 buffer（帧累积器、解码后消息队列）是真正不同的问题，都被约束了；用平台 风险：无高风险路径命中（Stage 1e）。这是 daemon 关键传输管道，因此按完整 Stage 2 + CI 证据的深度审查。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Code reviewMy independent proposal for this problem (before reading the diff): an opt-in limits parameter on the NDJSON decoder with a frame-accumulator cap and a decoded-queue cap, a typed transport error reported through a hook, close-not-reject on the decoded stream so the ACP SDK receive loop doesn't surface an unhandled rejection, and a fixed policy enabled only at the daemon spawn sites. The PR matches that baseline essentially point for point, and I didn't find a simpler path it missed. What I verified in the diff:
No critical findings. Minor, no action needed: a whitespace-only frame containing non-ASCII whitespace (e.g. a lone NBSP) is silently skipped on the legacy path but produces a redacted parse-error log on the bounded path — the outcome (message dropped, nothing enqueued) is the same, and the scenario is pathological. Test evidenceUnattended CI run — I did not build or execute PR code; the evidence below is the PR's own CI results on the reviewed commit, fetched via the API. Zero failing checks. macOS/Windows unit tests are skipped per this repo's PR CI configuration (the ubuntu matrix is what runs); Serve A/B and the SDK workflow's Real daemon E2E job are the serve-side coverage here, both green. The only in-progress jobs ( The before/after memory numbers in the PR description (96 MiB → cancel at 65 MiB; 40,000 queued → 256 admitted) are the author's measurements on macOS arm64 — attributed as the author's claim, not independently re-run here. The mechanism itself is pinned by the new unit tests, which CI ran green.
Sandboxed verification would settle the remaining behavioural gap: 中文说明代码审查我在看 diff 之前独立构想的方案是：给 NDJSON 解码器加一个可选 limits 参数，含帧累积上限和解码队列上限、通过 hook 报告的 typed transport error、对解码 stream 采用"关闭但不 reject"以避免 ACP SDK receive loop 出现未处理 rejection，以及只在 daemon spawn 点启用的固定策略。PR 与该基线基本逐点对齐，我也没找到它遗漏的更简路径。 在 diff 中核实的内容：
无阻塞性问题。次要、无需处理：仅含非 ASCII 空白（如单独一个 NBSP）的空白帧在 legacy 路径被静默跳过，而在有界路径会产生一条脱敏的 parse-error 日志——结果一致（消息被丢弃、不入队），场景本身是病态的。 测试证据无人值守 CI 运行——未构建或执行 PR 代码；以下证据为被审 commit 上 PR 自身 CI 结果，经 API 获取。无失败检查。macOS/Windows 单测按本仓库 PR CI 配置被跳过（PR 矩阵只跑 ubuntu）；Serve A/B 与 SDK workflow 的 Real daemon E2E 是本 PR 相关的 serve 侧覆盖，均为绿。仅有的进行中任务（ PR 描述中的 before/after 内存数字（96 MiB → 65 MiB 处取消；40,000 条排队 → 接纳 256 条）是作者在 macOS arm64 上的测量——作为作者声明引用，未在此独立复现。机制本身由新增单测固定，CI 已跑绿（见上方 CI 表格）。 沙箱验证可以补齐剩余的行为缺口： — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 4/5 — clean review across every stage; the one open item is verification coverage, not code. Stepping back: this is the NDJSON increment of the daemon resource-protection series, exactly as scheduled in #8091, and it lands the way the design doc says it should. The implementation matches the proposal I'd have written independently — opt-in limits on the decoder, typed transport errors, close-not-reject for the SDK receive loop, fixed frozen policy wired into all three daemon factories, legacy path untouched for everyone else. I looked for the simpler version of this PR and didn't find one: the accumulator and the decoded queue are genuinely different unbounded buffers, and using the platform The part I can't give a fifth point for is honest uncertainty, not a defect: the mechanism is pinned by new tests and green in CI (including a real Approving, pinned to the reviewed commit. 中文说明置信度：4/5 —— 各阶段审查都很干净；唯一的未决项是验证覆盖面，不是代码问题。 退一步看：这是 daemon 资源保护系列中的 NDJSON 增量，与 #8091 的排期完全一致，落地方式与设计文档所述一致。实现与我会独立写出的方案相符——解码器上的可选 limits、typed transport error、对 SDK receive loop 采用"关闭但不 reject"、固定冻结策略接入全部三个 daemon factory、其他所有消费者的 legacy 路径保持原样。我找过这个 PR 的更简版本，没找到：累积器和解码后队列确实是两个不同的无界 buffer，用平台 不给第五分的原因是诚实的不确定性，而非缺陷：机制由新增测试固定且 CI 全绿（包括真实 批准，固定在被审 commit 上。 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
approved these changes
Aug 11, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
yiliang114
approved these changes
Aug 11, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
LGTM. The security-critical properties hold under inspection: frame accumulation is bounded during streaming (append checks length+chunk > max per chunk, geometric growth capped), exactly-64MiB-incl-newline passes with +1 rejected in both directions, the decoded queue charges max(frameBytes, ceil(64MiB/256)) synchronously before enqueue and WHATWG streams deduct the exact charge on read, oversized/incomplete/saturated conditions report typed causes and terminate the exact tracked child (memoized ChildProcess SIGTERM, no group kill, no sibling/daemon exposure), all three serve spawn sites pass the limits while interactive/legacy paths are untouched, and bounded logs carry only errorKind/bytes/sha256 with a test asserting a secret string cannot appear.
One P2 design call to consciously accept, explicitly documented in the PR: queue saturation is fatal, not backpressure — a transient daemon event-loop stall coincident with a burst past 256 messages/64 MiB from a healthy child kills the session (deliberate, to avoid pipe-block hangs in a multiplexed daemon; 256x256KiB is a fair cushion). P3 follow-ups, not blocking: legal 64MiB frames still amplify ~5x transient memory and the outbound check is post-allocation; whitespace-only EOF tail is stricter than legacy; malformed-JSON path is unbounded in rate (SHA-256 over up to 64MiB per failure, still strictly better than legacy's content echo); a few boundary test gaps. CI green on head (22 success, skips are fork-PR gating). Ship it after signing off on the saturation-kills-child tradeoff.
This was referenced Aug 12, 2026
yiliang114
approved these changes
Aug 12, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
LGTM. The change is already merged and the existing reviews (from qwen-code-ci-bot and yiliang114) are thorough. My independent re-review confirms the key properties hold:
Correctness: Frame boundary enforcement is checked per chunk before accumulation (assertFrameSize in readBoundedChunk), the BoundedFrameBuffer geometric growth is capped at maxFrameBytes, queue admission is checked against desiredSize before enqueue with the conservative minimumQueueCharge strategy, and the cancel path correctly sets canceled = true synchronously before awaiting cancelReader so the pump loop sees it on the next reader.read() resolution.
Security: Parse-error logs on the bounded path carry only errorKind, bytes, sha256, and payloadOmitted — the test asserts a secret string cannot appear. The legacy path is untouched. Frame and queue limits are enforced in the transport layer before any consumer can observe the data.
Backward compatibility: ndJsonStream without limits retains the legacy behavior; only qwen serve (primary, secondary, and dynamic workspace runtimes) opts in through the pipeLimits parameter.
Design tradeoff (documented): Queue saturation is fatal, not backpressure — a transient event-loop stall coincident with a burst past 256 messages/64 MiB from a healthy child kills the session. This is deliberate to avoid pipe-block hangs in a multiplexed daemon and is explicitly documented in the PR body.
Tests: 57 focused ACP bridge tests, 243 daemon runtime tests, all passing. CI green on head. Typecheck and lint clean.
One P3 observation (not blocking): minimumQueueCharge at 64 MiB/256 = 256 KiB means small messages charge a disproportionate amount, which could prematurely reject a mixed queue before either independent limit is actually full. This is a deliberate conservative choice, already noted in the PR body and the prior review.
Collaborator
|
Released in v0.21.11. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
This PR adds opt-in frame and decoded-queue protection to newline-delimited ACP streams, then enables it for every ACP child created by
qwen serve: primary, startup secondary, and dynamically registered workspace runtimes. Inbound and outbound frames are limited to 64 MiB including the newline, while the decoded inbound queue is conservatively limited to 256 messages and 64 MiB of retained wire bytes.Inbound limit violations, queue saturation, and unterminated EOF report a typed transport cause, cancel the input, close the decoded stream without rejection, and terminate the exact tracked ACP child. Bounded parse failures log only byte length and a SHA-256 digest. Public streams remain unchanged unless limits are explicitly supplied, so in-memory channels, direct embeds, interactive CLI, and IDE companion behavior stay compatible.
Why it's needed
Workspace and session count limits do not bound the daemon root's memory. Before this change, one child could emit an unterminated frame that accumulated indefinitely, or a notification flood could fill the decoded
ReadableStreambehind a slow consumer. The existing large-frame observer ran only after decode, parse, and enqueue, so it could report the growth but could not prevent it.Reviewer Test Plan
How to verify
Feed a bounded stream a frame exactly at the configured limit and confirm it is delivered. Then exceed the limit across several chunks and confirm the transport hook receives one typed cause before parsing, the input is canceled, and the decoded stream closes normally. Repeat with an unterminated EOF and with a stalled consumer that reaches the count and byte queue watermarks.
Construct a real ACP SDK client connection over the bounded stream and confirm an inbound violation closes and aborts the connection without an unhandled rejection. Spawn a bounded child channel and confirm the exact tracked child receives termination. Boot primary and secondary workspaces, register another workspace dynamically, and confirm all three factories receive the same fixed policy.
Confirm calls that omit limits retain legacy behavior, including split UTF-8, CRLF, full parse-error logging, and silently dropping an unterminated final line.
Evidence (Before & After)
ArrayBuffergrew by 96.0 MiB; RSS grew by 93.5 MiBArrayBufferreturned to baseline after GCconsole.errorpayloadOmittedClientSideConnectionreachesclosed/abortedwith zero unhandled rejectionsTested on
Environment (optional)
macOS arm64, Node.js 24.12.0, npm 10.9.8. Verified with 57 focused ACP bridge tests, 243 daemon runtime tests, full build, full typecheck, full lint, and direct GC/memory probes.
Risk & Scope
qwen servechanges by default. A daemon ACP child that emits a frame over 64 MiB, exceeds the decoded queue boundary, or closes with an unterminated frame now has that workspace channel terminated instead of allowing unbounded retention or silently dropping EOF. Public and standalone stream defaults are unchanged.Linked Issues
Part of #8091. Related to #8051.
中文说明
本 PR 做了什么
本 PR 为 NDJSON ACP stream 增加可选的帧与解码队列保护，并在
qwen serve创建的所有 ACP child 上启用：primary、启动时 secondary 以及动态注册的 workspace runtime。入站和出站帧上限均为 64 MiB（包含换行符）；解码后的入站队列采用保守计费，最多保留 256 条消息和 64 MiB wire bytes。入站超限、队列饱和和未终止 EOF 会通过 transport hook 报告 typed cause、取消输入、以不 reject 的方式关闭解码 stream，并终止准确的 tracked ACP child。受保护路径的解析失败日志只保留字节长度和 SHA-256 digest。未显式传入 limits 的公共 stream 保持原行为，因此 in-memory channel、direct embed、交互式 CLI 和 IDE companion 不受影响。
为什么需要
workspace 和 session 的数量限制无法约束 daemon root 的内存。变更前，单个 child 可以持续输出无换行帧使 accumulator 无限增长，也可以通过 notification flood 在慢消费者后方填满解码
ReadableStream。现有 large-frame observer 只在 decode、parse 和 enqueue 完成后运行，只能观测增长，不能阻止增长。Reviewer 测试计划
如何验证
向 bounded stream 输入一个恰好达到配置上限的帧，确认能够投递；随后将超限帧拆成多个 chunk 输入，确认 transport hook 在解析前只收到一次 typed cause，输入被取消，并且解码 stream 正常关闭。对未终止 EOF，以及慢消费者触发 count/byte queue watermark 的场景重复验证。
使用真实 ACP SDK client connection 连接 bounded stream，确认入站违规会关闭并 abort connection，且不会产生 unhandled rejection。创建 bounded child channel，确认准确的 tracked child 收到终止信号。启动 primary 和 secondary workspace，再动态注册一个 workspace，确认三条 factory 路径都接入同一固定策略。
确认不传 limits 的调用保留 legacy 行为，包括拆分 UTF-8、CRLF、完整 parse-error 日志以及静默丢弃未终止尾行。
证据（Before & After）
ArrayBuffer增长 96.0 MiB；RSS 增长 93.5 MiBArrayBuffer回到基线console.errorpayloadOmittedClientSideConnection进入closed/aborted，unhandled rejection 为 0测试平台
环境（可选）
macOS arm64、Node.js 24.12.0、npm 10.9.8。已验证 57 个 ACP bridge 聚焦测试、243 个 daemon runtime 测试、全量 build、全量 typecheck、全量 lint，以及直接 GC/内存探测。
风险与范围
qwen serve默认行为变化。daemon ACP child 若输出超过 64 MiB 的帧、超过解码队列边界或以未终止帧关闭，现在会终止该 workspace channel，不再允许无限保留或静默丢弃 EOF。公共和 standalone stream 默认行为不变。关联 Issue
属于 #8091 的一部分，与 #8051 相关。