feat(daemon): name the saturated NDJSON budget in channel-teardown diagnostics - #10727
Merged
Conversation
Collaborator
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: observed, not theoretical. Per the PR description, repeated Direction: the goal is aligned — naming which of the five bounded budgets fired, with required/available/cap bytes, is exactly what that incident needed. One thing to flag, though: this change adds a new attribute to the existing Size: not applicable — no core paths touched ( Approach: the scope looks right from the outside — all five budget throw sites get the tag, the bridge derives a bounded Risk: no elevated risk signals — none of the changed files match the revert-correlated path list. The stderr format change (one extra comma-separated field before the session count) is honestly flagged in the PR description. Stopping here per the telemetry escalation: no code-review verdict or approval from this gate. A maintainer needs to make the call on the telemetry/log-line surface before this moves forward. 中文说明感谢贡献！ 模板完整 ✓ 问题： 已观测到，不是理论问题。按 PR 描述，生产 daemon（v0.22.2）上反复出现的 方向： 目标对齐——点名五个有界预算中哪一个触发、并附上 required/available/cap 字节数，正是该事故所需要的。但要说明一点：本改动给既有的 规模： 不适用——未触及核心路径（仅 方案： 从外部看范围合理——五个预算抛错点全部打标，bridge 派生有界的 风险： 无升级风险信号——改动文件均未命中与 revert 相关的高风险路径。stderr 格式变化（session 数之前多一个逗号分隔字段）在 PR 描述中已如实说明。 按遥测升级策略在此停止：本门禁不给出代码审查结论或批准。遥测/日志行层面的取舍需要维护者定夺后才能继续。 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
⏸️ Deferring to a maintainer — this PR adds a telemetry attribute to the |
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. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
中文说明
已审查。 建议见行内评论。
— qwen3.8-max via Qwen Code /review (v0.22.3)
Collaborator
Review + E2E report (head
|
Collaborator
Author
|
Follow-up a155229: adds the bridge-level regression test for the transport_detail wiring (real teardown path: pending prompt, guard fires with a budget-tagged queue-limit error, channel.exited telemetry carries the bounded budget string) and falls back to budget=unknown when a queue-limit error lacks the typed field. Also supersedes and closes #10722, which targeted the same log/telemetry lines with a generic field-extraction shape that could not name which of the five budgets fired; its test is ported here. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-2 budget literals at the five throw sites unpinned — still stands, already reported (comment 3906226161)
Not reviewed: reverse audit — stopped before round 2 by the review time budget.
Not explored to full depth (tool budget reached): "agent 6a": running the new test in packages/acp-bridge/src/bridge.test.ts ('records which budget fired on a transport-guard channel exit') — the worktree has no node_modul….
Not reviewed: reverse audit — stopped before round 1 by the review time budget.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：reverse audit — stopped before round 2 by the review time budget。
未探索到全部深度（达到工具调用预算）："agent 6a"：running the new test in packages/acp-bridge/src/bridge.test.ts ('records which budget fired on a transport-guard channel exit') — the worktree has no node_modul…。
未审查：反向审计——评审时间预算不足，未能开始第 1 轮。
— qwen3.8-max via Qwen Code /review (v0.22.3)
…agnostics Queue-saturation teardowns (ndjson_queue_limit_exceeded) previously logged only the transport code, so operators could not tell which of the five bounded budgets fired, nor the required/available bytes or direction. Tag each NdJsonQueueLimitError with its budget and surface a bounded budget:required/available/cap detail in the channel-exited log line and telemetry, per issue #10162.
Adds the bridge-level regression test for the transport_detail wiring (real teardown path: pending prompt, guard fires, channel.exited carries the bounded budget string) and falls back to budget=unknown when a queue-limit error lacks the typed budget field, so pre-budget errors still get required/available/cap context.
yiliang114
force-pushed
the
fix/ndjson-queue-diagnostics
branch
from
September 2, 2026 05:28
a155229 to
9413da9
Compare
Contributor
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
Collaborator
Author
|
Rebased onto main after #10731 (backpressure instead of immediate teardown) merged. The two are complementary: #10731 converts transient saturation into backpressure, but the fail-closed path still runs when the consumer stays stalled past the grace window, and the other four budgets (inbound/outbound request ledgers, prepared response, outbound operation) were never backpressured. This PR names whichever budget fired in the channel-exited stderr line and telemetry, so a post-#10731 teardown is diagnosable instead of anonymous. The rebase keeps #10731's ensureQueueSpace callback and threads the 'decoded' budget through its queueLimitError factory. |
yiliang114
enabled auto-merge
September 2, 2026 05:59
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Collaborator
Author
|
Addressed the bounded review feedback in 504fe4f:
The count-cap / duplicate-id observation is valid, but appending only Focused verification: 974 tests passed across |
qwen-code-dev-bot
approved these changes
Sep 2, 2026
qwen-code-dev-bot
left a comment
qwen-code-dev-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed at head a863643f.
- Diagnostic-only refinement, correctly bounded: each
NdJsonQueueLimitErrornow carries which of the five budgets fired, andsafeTransportFailureDetailreconstructs the teardown annotation exclusively from typed fields with a strict^[a-z0-9_.-]{1,64}$budget whitelist and finite-clamped numbers — the raw error message never reaches the diagnostic surface, preserving the redaction convention the adjacent code documents. - The constructor-signature change is compiler-enforced at every throw site (typecheck green), and bridge/spawnChannel/ndJsonStream tests pin the new detail format and the malformed-field fallback to
unknown. - 7/7 threads resolved, no prior review blockers; CI has no failures (Test and review lanes still running); per the channel convention the call is on the review itself.
yiliang114
commented
Sep 2, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
Author
There was a problem hiding this comment.
Review findings (self-PR, cannot self-approve):
- Every NdJsonQueueLimitError throw site now carries its budget (decoded, inbound_request, outbound_request, prepared_response, outbound_operation) with the message naming it — the five sites and their tests are consistent, so a channel teardown now tells which queue actually saturated instead of the generic decoded-queue text.
- safeTransportFailureDetail is properly paranoid: only fires on the ndjson_queue_limit_exceeded code, budget is whitelisted by /^[a-z0-9_.-]{1,64}$/ (anything else becomes unknown — the hostile-fixture test feeds INVALID budget <script> plus fractional, negative and NaN numbers and gets unknown:required=0:available=?:cap=1), numbers are clamped floor(max(0, v)) with ? for non-finite, and the raw error message never reaches the detail — matching the existing safe-code discipline for telemetry and the stderr line.
- The detail rides on channel.exited attributes and the stderr summary only when present; frame_too_large exits are asserted to carry no detail attribute, so the two transport codes stay distinguishable.
- No behavior change to the backpressure mechanics themselves (that is #10731); this is labeling + diagnostics on top.
CI on this head still running; no blocking issues found.
Collaborator
|
Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with |
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
qqqys
approved these changes
Sep 2, 2026
qqqys
left a comment
qqqys
left a comment
Collaborator
There was a problem hiding this comment.
Approving at head b9e91d305987876b59d8511db68c93c646378f72 (Critical-only scan).
- No historical blockers: this PR never received CHANGES_REQUESTED; all prior-round findings were Suggestions (teardown-diagnostic test coverage, budget-label pinning, count-cap detail, code-literal duplication) addressed by the follow-up commits.
- Verified at head: exactly five
NdJsonQueueLimitErrorconstruction sites (three inndJsonStream.ts, two inspawnChannel.ts), each carrying its budget label; the new required constructor parameter is compiler-enforced, and no other construction sites or consumers of the old message text exist. safeTransportFailureDetailderives the boundedbudget:required=…:available=…:cap=…detail from typed fields only: budget whitelist/^[a-z0-9_.-]{1,64}$/(elseunknown), non-finite values →?, floor + clamp at zero; the raw error message never reaches the stderr line or telemetry. The detail is appended only when present, sondjson_frame_too_largeexits stay distinguishable. Errorcodeand all existing fields are unchanged; no behavior change to the backpressure mechanics.- Tests pin the five budget labels on their real overflow paths, the telemetry attribute and stderr
transport_detail=segment, the malformed-field fallback (unknown:required=0:available=?:cap=1), and the absence of the detail attribute on frame-too-large exits. - This head only merges main (#10764, #10770) on top of the previously reviewed
594a7cd9; the PR diff against main is byte-identical to the reviewed version and the merged changes touch nopackages/acp-bridgefiles. - CI on this head: no failures; several checks still running, which does not block under this channel's convention.
中文说明
在 head b9e91d30 上批准（Critical-only 扫描）。历史无阻塞问题：本 PR 从未收到 CHANGES_REQUESTED，各轮发现均为 Suggestion 且已由后续提交处理。当前 head 已验证：五个 NdJsonQueueLimitError 构造点各携带预算标签（新增必填构造参数由编译器强制，无遗漏点、无旧消息文本消费者）；safeTransportFailureDetail 仅从类型化字段派生有界详情串（预算白名单正则、非有限值回退 ?、向下取整并钳制到 0），原始错误消息不会进入 stderr 或遥测；详情仅在存在时追加，ndjson_frame_too_large 退出仍可区分；错误 code 与既有字段不变，背压机制无行为变化。测试固定了五个预算标签、遥测与 stderr transport_detail= 格式、畸形字段回退及 frame-too-large 无详情。该 head 仅在先前已评审的 594a7cd9 上合并 main（#10764、#10770），PR 对 main 的 diff 与已评审版本逐字节一致，合入内容不涉及 packages/acp-bridge。CI 无失败，仍在运行的检查不构成本 Channel 的卡点。
Collaborator
Author
Before/After evidence — run on
|
| Field | Before (main) | After (this PR) |
|---|---|---|
message |
NDJSON decoded queue is full (required 2049 bytes, available 37 bytes) |
NDJSON prepared_response queue limit exceeded (required 2049 bytes, available 37 bytes) |
budget |
(undefined) | prepared_response |
code |
ndjson_queue_limit_exceeded |
ndjson_queue_limit_exceeded |
2. Bridge channel exited stderr line
Before:
qwen serve: channel exited (code=none, signal=SIGTERM, transport=ndjson_queue_limit_exceeded, 1 session(s) torn down)
After:
qwen serve: channel exited (code=none, signal=SIGTERM, transport=ndjson_queue_limit_exceeded, transport_detail=prepared_response:required=262144:available=0:cap=67108864, 1 session(s) torn down)
3. channel.exited telemetry attributes
Before — no transport_error_detail:
{
"qwen-code.daemon.channel.transport_error_code": "ndjson_queue_limit_exceeded"
}After — adds transport_error_detail:
{
"qwen-code.daemon.channel.transport_error_code": "ndjson_queue_limit_exceeded",
"qwen-code.daemon.channel.transport_error_detail": "prepared_response:required=262144:available=0:cap=67108864"
}中文
前后对比证据 — 在 origin/main 与 PR 分支上运行
在 origin/main 和本 PR 分支各运行了一次复现。通过 createSpawnChannelFactory（极小管道限制：1 条消息、4096 字节）触发 prepared_response 预算超限，并模拟 bridge 传输失败，捕获了实际的错误消息、stderr 行和遥测属性。
1. reservePreparedResponse 抛出的 NdJsonQueueLimitError
| 字段 | Before (main) | After (本 PR) |
|---|---|---|
message |
NDJSON decoded queue is full (required 2049 bytes, available 37 bytes) |
NDJSON prepared_response queue limit exceeded (required 2049 bytes, available 37 bytes) |
budget |
(undefined) | prepared_response |
code |
ndjson_queue_limit_exceeded |
ndjson_queue_limit_exceeded |
2. Bridge channel exited stderr 行
Before:
qwen serve: channel exited (code=none, signal=SIGTERM, transport=ndjson_queue_limit_exceeded, 1 session(s) torn down)
After:
qwen serve: channel exited (code=none, signal=SIGTERM, transport=ndjson_queue_limit_exceeded, transport_detail=prepared_response:required=262144:available=0:cap=67108864, 1 session(s) torn down)
3. channel.exited 遥测属性
Before — 无 transport_error_detail：
{
"qwen-code.daemon.channel.transport_error_code": "ndjson_queue_limit_exceeded"
}After — 新增 transport_error_detail：
{
"qwen-code.daemon.channel.transport_error_code": "ndjson_queue_limit_exceeded",
"qwen-code.daemon.channel.transport_error_detail": "prepared_response:required=262144:available=0:cap=67108864"
}
Collaborator
|
Post-merge review — PR merged at head Tier: Standard. Diagnostics/telemetry improvement. Risk Q3 (counterpart not in diff: the ScopeReviewed: all 6 changed files in NOT reviewed: any files outside the diff that might parse the Ran: none — working tree unavailable; static analysis only. Rung 2 (mutation/non-vacuity probe on the new tests) was not run; disclosure below. FindingsR1-3 · Minor — detail renders bytes only; message-count-triggered fires may show non-zero
|
| Prior finding | Outcome |
|---|---|
| R1-1 (ci-bot): teardown diagnostics untested | Fixed — author added tests in follow-up commits; the bridge-level regression test is present in this diff. |
| R1-2 (ci-bot): detail bytes-only, message-count may mislead | Confirmed, still present — included as my R1-3 above. |
| R2-1 (ci-bot): stderr format change | Confirmed — my R1-2. |
| R2-2 (ci-bot): defensive branches untested | Refuted — bridge.test.ts explicitly tests adversarial inputs (NaN, negative, non-finite, injection attempt) and asserts the expected sanitized output. |
| R2-3 (ci-bot): code literal duplication | Confirmed, still present — mentioned in "What checked out" above; minor. |
Approval blockers
None. (Post-merge; no approval action taken regardless.)
Unreviewed dimensions
- Rung 2 (mutation probe on new tests) not run — working tree unavailable. The test suite includes explicit adversarial inputs that provide meaningful though not mutation-complete coverage.
- No full-repo search for
channel exitedformat parsers; that surface remains an unreviewed dimension.
Reviewed with AI assistance.
Collaborator
|
Released in v0.23.0. |
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
5 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
When the daemon's bounded NDJSON transport guard tears a channel down, the stderr line and the
channel.exitedtelemetry event now say which of the five bounded budgets fired — the decoded-frame queue, the inbound-request ledger, the outbound-request ledger, the prepared-response ledger, or the outbound-operation ledger — together with the bytes required, the bytes available, and the configured byte cap. The queue-limit error carries this as typed fields, and the bridge derives a boundedbudget:required=…:available=…:cap=…detail string from those typed fields only (never from the raw error message), appending it astransport_detail=to theqwen serve: channel exited (…)line and as aqwen-code.daemon.channel.transport_error_detailtelemetry attribute.Why it's needed
On a production daemon (v0.22.2, one busy workspace), repeated
channel exited (…, transport=ndjson_queue_limit_exceeded, N session(s) torn down)events tore down every session on the channel, and the log could not distinguish a stalled consumer (ACP child event-loop wedged for tens of seconds) from a genuine flood. Root-causing it took log archaeology plus an external event-loop monitor instead of one log line. This is the small diagnostics follow-up that issue #10162 identifies as the step that unblocks diagnosing the fail-closed blast radius.Reviewer Test Plan
How to verify
Run the acp-bridge unit tests for the stream, the spawn channel, and the bridge. To see the new detail end to end, saturate the prepared-response budget the way the spawn-channel tests do: the thrown error message now names
prepared_response, and a channel-exited line produced while that budget is saturated carriestransport_detail=prepared_response:required=…:available=…:cap=…before the session count.Evidence (Before & After)
N/A — log/telemetry-only change; no user-visible UI.
Tested on
Environment
Local unit tests via
npx vitest runinpackages/acp-bridge; fullnpm run typecheckgreen.Risk & Scope
channel exitedstderr line gains one comma-separated field before the session count; anything parsing the exact old format would need to tolerate it. The telemetry attribute is purely additive.code(ndjson_queue_limit_exceeded) and all existing fields are unchanged.Linked Issues
Refs #10162 (does not close it; the fail-closed semantics themselves are the product question there).
中文说明
这个 PR 做了什么
当 daemon 的有界 NDJSON 传输守卫拆除 channel 时，stderr 日志行和
channel.exited遥测事件现在会说明五个有界预算中是哪一个触发了拆除——解码帧队列、入站请求账本、出站请求账本、预置响应账本、出站操作账本——并附上所需字节数、可用字节数和配置的字节上限。队列超限错误以类型化字段携带这些信息，bridge 仅从这些类型化字段（绝不从原始错误消息）派生一个有界的budget:required=…:available=…:cap=…详情串，作为transport_detail=追加到qwen serve: channel exited (…)行，并作为qwen-code.daemon.channel.transport_error_detail遥测属性。为什么需要
在一个生产 daemon（v0.22.2，单个繁忙 workspace）上，反复出现的
channel exited (…, transport=ndjson_queue_limit_exceeded, N session(s) torn down)事件把 channel 上所有 session 一起拆掉，而日志无法区分"消费方卡死"（ACP 子进程事件循环卡死几十秒）与"真实洪泛"。定位根因只能靠翻日志考古加外部事件循环监控，而不是一行日志。这正是 issue #10162 指出的、能解锁诊断 fail-closed 爆炸半径的小型诊断增强。评审测试计划
如何验证
运行 acp-bridge 的 stream、spawn channel、bridge 单元测试。要端到端看到新详情，可以像 spawn-channel 测试那样把预置响应预算打满：抛出的错误消息现在会点名
prepared_response，该预算饱和时产生的 channel-exited 行会在 session 数之前携带transport_detail=prepared_response:required=…:available=…:cap=…。证据（前后对比）
N/A —— 仅日志/遥测变更，无用户可见 UI。
测试环境
macOS ✅；Windows / Linux 由 CI 覆盖。
风险与范围
channel exitedstderr 行在 session 数之前多了一个逗号分隔字段；按旧格式精确解析的工具需要容忍它。遥测属性纯增量。code（ndjson_queue_limit_exceeded）与所有既有字段不变。关联 Issue
Refs #10162（不关闭；fail-closed 语义本身是该 issue 的产品问题）。