fix(serve): Bound ACP HTTP pre-attach buffers by bytes - #9007
Conversation
🩺 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 |
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. |
|
Re-run after review rounds R8–R12 and the rebase onto Template ✓ (complete, unchanged). Problem: settled long ago. The count-only 256-frame pre-attach cap with silent eviction is a tracked item of the daemon memory workstream (#8091), and a maintainer's real-daemon, no-mock A/B on the predecessor head observed the failure directly: base silently dropped 44 of 300 JSON-RPC replies (callers left waiting forever) and retained +613 MB RSS after 128 × ~1 MiB replies, while this PR's head failed closed at the documented cap and settled back to baseline. "Does the problem exist" is not a question on this PR. Direction: aligned — one checklist line of #8091, design doc in-repo, continuation of already-merged increments (#8245, #8423, #8462, #8508). Size: 27 files, +3,785/−492. Breakdown: 1,919 production logic lines, 2,295 test, 63 docs/E2E-plan. Production grew +259 since the last pass — the fixes rounds R8–R12 shook out. It spans three packages ( Approach: unchanged and still right — serialize once, charge serialized UTF-8 bytes at admission, tiered stream/connection/process-global budgets, refusal retires the exact owner instead of evicting an older frame, ownership grants provisional until local delivery. The delta since the last gate pass is review-shaken fix commits (delivery-failure escalation to the caller, teardown-safe logging) plus a rebase whose only tail commit ( Risk: no high-risk revert-correlated paths matched. The two items carried out of round 12 are assessed in the Stage 2 comment: the SSE split-write finding (R12-1) is successfully disputed in my independent reading, and the settle-time requeue asymmetry (R9-1) is a genuine trade-off that needs a maintainer call. Moving on to code review. 🔍 Maintainer note unchanged: per the core-module two-tier rule, a cross-package PR of this size needs a human sign-off before merge regardless of how clean the automated review comes back. 中文说明R8–R12 审查轮次及 rebase 到 模板完整 ✓（未变化）。 问题：早已确立。只计帧数的 256 帧 pre-attach 上限加静默淘汰是 daemon 内存工作流（#8091）的跟踪项；维护者已在先驱 head 上以真实 daemon 无 mock A/B 直接观测到失败形态：base 静默丢弃 300 条 JSON-RPC 回复中的 44 条（调用方永久挂起），128 × ~1 MiB 回复后 RSS 滞留 +613 MB，而本 PR 的 head 在文档化上限处 fail closed 并回落基线。"问题是否存在"在本 PR 上不是疑问。 方向：对齐——#8091 清单一行，设计文档在仓库内，是已合并增量（#8245、#8423、#8462、#8508）的延续。 规模：27 个文件，+3,785/−492。构成：1,919 行生产逻辑、2,295 行测试、63 行文档/E2E 计划。生产行较上次过门增加 259 行——即 R8–R12 震出的修复。跨三个包（ 方案：未变且仍然正确——一次性序列化、按序列化后的 UTF-8 字节在准入时计费、stream/connection/进程全局三级预算、拒绝时精确关闭当事 owner 而非淘汰旧帧、ownership 在本地投递前保持 provisional。上次过门之后的增量是审查震出的修复 commit（投递失败向调用方升级、teardown 安全日志）外加一次 rebase——其唯一尾 commit（ 风险：未命中 revert 相关高风险路径。第 12 轮带出的两项在 Stage 2 评论中评估：SSE 拆分写入发现（R12-1）经我独立判读成立地被驳回；settle 时重入队的不对称（R9-1）是真实的取舍，需要维护者拍板。 进入代码审查 🔍 给维护者的说明不变：按核心模块双层规则，这种跨包规模的 PR 合并前需要人工签核，与自动审查多干净无关。 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewRe-run focus: a full re-read of the diff at the rebased head No new Critical findings from this side. The settlement machinery holds up under re-read: admission is checked twice around serialization (before and after, so a concurrent admission can't sneak past an await), the lease/ownership accounting pairs one-to-one on every commit/detach/rollback path I traced, The two open items from round 12:
Everything else round 12 carried (R12-2/3/4, R11-2) stays deferred under that same rule — visible in-thread, not dropped. sequenceDiagram
participant P1 as Dispatcher
participant P2 as AcpConnection
participant P3 as PreAttachBudget
participant P4 as Stream writer
P1->>P2: reply with ownership receipt
P2->>P2: serialize once to UTF-8 Buffer
alt live stream and empty buffer
P2->>P4: sendSerialized, no lease needed
else must buffer
P2->>P3: admission - stream frames, connection frames and bytes, global lease
P3-->>P2: lease, or refuse and close exact owner with 1013
P2->>P2: buffer frame and charge owner
P4-->>P2: attach
P2->>P4: sendSerialized and markPendingDelivery
end
P4-->>P2: write callback settles delivered, unknown, or discarded
P2->>P3: release lease
P2->>P1: delivered commits ownership, unknown re-checks identity, discarded rolls back
Files changed (17 of 27 shown)
Testing evidence (CI run — no PR code executed locally)Unattended re-run; nothing was built or run here — the evidence is the PR's own CI on the reviewed commit, fetched via API. All five CI results for
One row per check name (latest run) on the reviewed commit. / 每个检查名一行（取该 commit 最新一次运行）。 The behavioural evidence stack: a maintainer's real-daemon, no-mock A/B on predecessor head Not verified here: current-head daemon behaviour has not been independently re-run by this review — the maintainer A/B predates the R8–R12 fixes and the rebase, and the author's focused-test counts (46/46, 442/442, 14/14) are the author's claim, not evidence this pass re-executed. Sandboxed verification would settle the remainder: 中文说明代码审查：本次 re-run 在 rebase 后的 head 本侧未发现新的 Critical。结算机制复读后依然成立：准入在序列化前后各检查一次（并发准入无法借 await 插队）；我所走查的 commit/detach/rollback 各路径上 lease/ownership 记账一一配对； 第 12 轮的两项未决：
第 12 轮携带的其余项（R12-2/3/4、R11-2）按同一规则保持延期——在帖中可见，未被丢弃。 测试证据（CI 运行，本地未执行 PR 代码）：无人值守 re-run，本地未构建未运行——证据为所审 commit 上 PR 自身 CI 经 API 拉取。 行为证据链：维护者在先驱 head 本处未验证：当前 head 的 daemon 级行为本审查未独立复跑——维护者 A/B 早于 R8–R12 修复与 rebase；作者的聚焦测试计数（46/46、442/442、14/14）是作者声明，非本次复跑的证据。沙箱验证将收口其余部分： — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 3/5 — clean review; the cap is policy plus one open trade-off, not doubt about the code. A ~1,900-production-line change crossing three packages is core infrastructure under the two-tier rule, which keeps the bot's approval off and hands the merge decision to a maintainer — and round 12 left exactly one question only a maintainer can answer. Stepping back over the whole arc: twelve review rounds, and the pattern is still the one you want to see — every round's Critical was fixed in the next commit with regression coverage attached, and the round-12 close-out is a disputed finding plus a trade-off, not a defect. I re-read the full diff at the rebased head and found nothing new to block on; the settlement machinery (generation-counted ownership identity, honest On the two round-12 residuals, my independent read:
The evidence stack is as complete as this repo gets: CI fully green on the reviewed head; a maintainer's real-daemon A/B on the predecessor head that watched base silently drop 44 replies while this PR's head failed closed at the exact documented cap; and the sandboxed What keeps this from an approval is standing policy and the open item, not a code finding:
Verdict: defer to maintainer — nothing blocks from this side beyond the open trade-off. @wenshao, this is yours when the 中文说明置信度：3/5 —— 审查干净；封顶来自政策加一项未决取舍，而非对代码的疑虑。约 1,900 行生产逻辑跨三个包，按双层规则属核心基础设施，机器人不投批准票，合并决定交由维护者——而第 12 轮恰好留下一个只有维护者能回答的问题。 退一步看整条弧线：十二轮审查，形态依然是最理想的那种——每一轮的 Critical 都在下一个 commit 修复并附回归覆盖，第 12 轮的收尾是一项被成功驳回的发现加一项取舍，而不是缺陷。我在 rebase 后的 head 上完整重读了 diff，没有新的可阻塞项；结算机制（带代数的 ownership 身份、如实的 对第 12 轮的两项残留，我的独立判读：
就本仓库而言，证据链已近乎完整：所审 head 上 CI 全绿；维护者在先驱 head 上的真实 daemon A/B 亲见 base 静默丢弃 44 条回复而本 PR 的 head 在文档上限处精确 fail closed；针对当前 head 的沙箱 挡住批准的只有既定政策与未决项，不是任何代码发现：
结论：转交维护者——除未决取舍外，本侧已无阻塞。@wenshao，待 — Qwen Code · qwen3.8-max Reviewed at |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Reviewed the full diff (26 files) with focus on the byte-bounding, overflow semantics, and lease/receipt lifecycle. Approving.
Byte-bound correctness
- Limits are enforced on real serialized sizes: frames are frozen via
JSON.stringify→BufferatprepareAndBuffertime, and budget checks run againstpayload.byteLength(plus a synchronous re-validation of stream/binding identity after serialization — no TOCTOU since it's all within one tick). - Three tiers all enforced: per-stream 256 frames, per-connection 1024 frames / 64 MB, daemon-global 4096 frames / 256 MB via shared
AcpPreAttachBudget(one instance shared across workspace mounts, covered by tests). Subtraction-form checks (bytes > maxBytes - usedBytes) avoid overflow. The oldpushCappedframe-count-only path is fully removed — no unbounded pre-attach growth path remains.
Overflow/discard semantics
- Overflow changed from silent drop-oldest to explicit retirement: session stream closed or connection retired with WS close 1013 "Resource limit", stderr log, and daemon-status counters (
preAttachGuardFailures, high-water marks, used frames/bytes). Observable, not silent. DeliveryReceipt+ ownership-commit-on-delivery is a real improvement:session/new|load|resume|forknow only commitclientId/ownership after the success reply is actually delivered (canCommitSessionOwnershipre-validates binding identity,closingSessions, destroyed state); undelivered replies roll the session back (detach / orphan removal) instead of leaking owned-but-unreachable sessions. Notification forms of these methods are now rejected, closing a session-leak path.
Transport interaction
- Both transports settle in-flight
sendSerializedpromises on close (activeSendClosers/activeWriteClosers), so leases backing pending deliveries are always released and accounting unwinds during teardown — covered by the "keeps a lease until an in-flight delivery settles after teardown" tests. - SSE
doWritenow waits for both the write callback and drain and settles false on close/finish/error; WSenqueueSendkeeps the serialized write chain and settles on socket close. - Bridge-side
estimatePreparedResponseBytesswitched to escape-awareestimateJsonStringBytes, fixing the previous undercount for escaped strings; property-tested againstJSON.stringifyfor all UTF-16 code unit classes.
Test coverage: byte-exact boundary ("accepts the exact byte limit and rejects the next byte"), cross-mount budget sharing, re-entry/serialization races, receipt-callback throws, teardown-vs-delivery races, notification rejection, and bootstrap status shape. Solid.
Nits (non-blocking)
- P2: an attached-but-stalled SSE client (open socket, never reads) can pin its flushed backlog's lease until the socket dies, since
doWritewaits on drain indefinitely; bounded by the budget but up to the global cap can be held, and later connections then fail pre-attach buffering. Bounded + observable is strictly better than the pre-PR unbounded state; a write-side stall timeout on live streams would be a good follow-up. - P3:
SseStream.close()ignores theTransportCloseReason(WS surfaces 1013); resource-limit closes on SSE are indistinguishable from a normal stream end client-side. - P3:
sendSerializedissues up to 4res.writecalls per SSE frame (id line /data:/ payload /\n\n); a single coalesced write would cut syscalls under throughput.
CI: Test (ubuntu-latest, Node 22.x) is red on DaemonSessionProvider.test.tsx ("keeps the current attachment live while a same-session load fails") — a fully mocked webui React timing test. Not related to this PR: this branch's two earlier runs today (e64e891, a76a5af) were fully green, the last commit only touches daemon-status/run-qwen-serve/SDK types, and today's main run is red on the same job. Flake, not gating.
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 558 passed · 0 failed · 558 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：558 通过 · 0 失败 · 558 总计 Verification reportPR #9007 Deep Verification —
|
| Cell | Scenario | HEAD (this PR) | BASE (50097c15) |
|---|---|---|---|
| C1 | 257 id-bearing frames, one session, no stream attached | #257 refused (failed); owning session closed; connection survives; guard logs ACP pre-attach frame limit |
all 257 accepted (void API); session stays open; buffer capped at 256 by silently evicting the oldest frame (buffer[0].id = 2, id 1 lost; id 257 admitted) |
| C2 | 257 id-less replies | #257 refused; session closed | buffer grows to 257 > "256 cap" (id-less frames exempt from eviction up to the 1024 hard cap — unbounded-growth path) |
| C3 | 3 × ~120 KiB frames, 300 KB per-connection byte limit (constructor seam) | frames 1–2 buffered; #3 refused; connection retired (registry.size 1→0); guard logs connection budget |
all 3 retained (~360 KB charged nowhere — no byte bound exists); connection alive |
| C4 | two connections, shared 6-frame global budget | A:3 + B:3 = 6 charged; B's 4th refused; B destroyed, bystander A intact with all 3 frames (no cross-connection eviction); guardFailures counted exactly once |
no global budget — both connections retain everything (B holds 4) |
| C5 | buffered pre-attach frames flushed on attach, real SSE wire | 3 frames reach the wire byte-equal, in order; all delivery promises resolve delivered |
same delivery parity (3/3 byte-equal) — normal path unchanged |
| C6 | 3 × 256 KiB buffered, attach with a stalled writer | charged at buffering (786,582 B); at the delivery seam pendingDeliveryFrames=3, budget usedFrames=3 still charged; after client close all counters return to 0/0 |
n/a (no lease accounting on base) |
| C7 | delivery-receipt lifecycle (commit/rollback signal) | teardown before attach → receipt discarded; while buffered → unsettled; wire delivery → delivered |
n/a (base sendConn has no receipt parameter — ownership was granted pre-delivery by construction) |
Head arm: 28/28; base arm: 12/12 (every base cell asserts the predicted base behaviour, including the silent-eviction oracle). C6's mid-flight snapshot is taken synchronously at the delivery seam (settlement needs ≥1 async hop); loopback kernel buffering otherwise drains a JS-level "stalled" client, so the seam snapshot is the deterministic observation of lease retention — the same seam a truly TCP-stalled writer holds open longer.
Secondary claim 1 (transactional ownership). Registry half proven in C7 (receipt discarded on teardown / delivered only on wire delivery). Dispatch half (ownershipReceipt wiring into session/new|load|resume|fork, notification rejection) exercised by the green transport suite (309 tests) and mutations M3/M3b below; a base-side dispatch A/B would require the full bridge and is listed under Not covered.
Secondary claim 2 (estimator). Wire oracle = Buffer.byteLength(JSON.stringify(·)) itself: exact equality on all 65,536 single UTF-16 code units, 2,000 seeded random strings, adversarial surrogate/escape combos; early-exit returns exactly min(actual, limit+1) across a full limit sweep; 2 MB mixed payload in 17 ms. Replaying the base formula (mechanical reproduction of the reverted hunk) against the same oracle shows systematic under-charging on every escape-requiring shape (700×U+0001: actual 4202, base charged 702 — under by 3500; object shape {content:…}: 4214 vs 714), while head is exact. Witness: 03-estimator-oracle.png.
Targeted gates (pristine tree)
| Suite | Result |
|---|---|
acp-bridge spawnChannel.test.ts + json-string-bytes.test.ts |
2 files, 46/46 |
cli pre-attach-budget + connection-registry + sse-stream + ws-stream + transport + daemon-status |
6 files, 443/443 (transport 309, connection-registry 50) |
sdk-typescript daemon-public-surface.test.ts |
1 file, 14/14 |
cli dispatch-error.test.ts + dispatch.sandbox.test.ts (extra: dispatch.ts is heavily changed) |
2 files, 7/7 |
Matches the PR's claimed local counts (46 / 442±1 / 14). All green on Linux, the OS the PR's test table marks "not tested".
Mutation matrix (vacuity)
Witness: 04-mutation-matrix-and-gates.png. Raw logs in logs/mut-*.log.
| Mutant | Change | Suite | Result | First failing assertion |
|---|---|---|---|---|
| M2 | spawnChannel.ts: revert estimator hunks to the exact base formula (Buffer.byteLength+2/+3) |
acp-bridge focused | KILLED — 1 red / 45 green | expected [Function] to throw an error (the new admission test; behavioural, not compile) |
| M1 | registry: disable per-connection + global refusal (if (false)) |
connection-registry + pre-attach-budget | KILLED — 6 red / 47 green | expected { …(2) } to be undefined (tryReserve must refuse) |
| M3 | dispatch: remove the canCommitSessionOwnership gate in delivered() |
transport.test.ts | SURVIVED — 0 red / 309 green | — |
| M3b | dispatch: disable rollback-on-discard (positive control) | transport.test.ts | KILLED — 7 red / 302 green | expected [] to include '550e8400-…' (rolled-back session id must vanish) |
M3b proves the suite can go red through the receipt mechanism in that exact file, so M3's survival is not harness failure. Classification of the M3 survivor: coverage gap, not dead code — the gate is live: it catches the close-race where a conn-stream write settles delivered while/after session/close runs for that id (closingSessions membership); without the gate ownSession() would re-add a session being torn down. No test drives that interleaving. The other half of the transaction (rollback-on-discard) is pinned by 7 tests. Note also the deliberate double per-stream frame check around serialization in prepareAndBuffer is re-entry defence (a toJSON re-entering the registry mid-JSON.stringify), pinned by the suite's serialization-reentry test — not dead duplication.
Findings
- Suggestion — ownership commit gate is unpinned (coverage gap, M3 survivor). The
delivered()path ofownershipReceiptis the only part of the new transaction no test asserts: removing thecanCommitSessionOwnershipcheck leaves all 309 transport tests green, while the symmetric rollback path is pinned by 7 tests. The guard is real (close-race on a live connection stream), so a regression here would be silent. A fixture that would pin it:session/newreply in flight on a live connection stream,session/closefor that id landing before the write settles, then assert the session is not re-owned. Not merge-blocking — the guard exists and is correct as written; this is completeness reporting.
Not covered
- Real-daemon E2E (committed plan steps 1–9: 128 × 1 MiB distinct-payload RSS, dynamic-workspace mounts competing on the real daemon,
GET /daemon/status?detail=fullon a liveqwen serve): not executed — the PR itself states this scenario was not run locally either. The A/B above proves the same mechanisms at the registry/stream level over real HTTP/SSE; this reproduces the handling (bounds, refusal attribution, lease accounting), not the end-to-end RSS trigger. - Per-commit attribution: the metadata lists 4 commits; the depth-2 merge-ref checkout makes only the aggregate reachable (
git rev-list HEAD^1..HEAD^2at a shallow boundary returns a grafted 1). Verified the aggregateHEAD^1..HEADdiff. - Dispatch-level A/B against base for ownership transactionality (would need the full bridge runtime); covered instead by C7 + gates + M3/M3b.
- Ordinary live SSE/WS new-frame queues and transient single-frame
JSON.stringifyamplification (PR-declared non-goals); Windows; repo-wide lint/typecheck (PR's own CI ran them; the base-controlnpm run buildand a cold HEAD-core typecheck ran clean here as part of A/B setup).
Methodology
Environment: node:22-bookworm CI container, Node v22.23.2, tree at refs/pull/9007/merge (depth 2), npm ci + npm run build pre-run at HEAD. A/B harness (ab-pre-attach.mjs) imports each arm's compiled packages/cli/dist/src/serve/acp-http/*.js — closure verified node-builtin-only via per-file import scan and node --check, so the control cannot silently load head code; base control built in git worktree tmp/base-tree at HEAD^1 with npm run build -w packages/cli (final build EXIT=0, 0 TS errors, after wiring the worktree to the root node_modules plus per-package node_modules directories, which the worktree lacks — earlier build attempts failing on 222 TS errors were this environmental gap plus a missing generated git-commit.ts, not a base-source problem; packages/core is untouched by the PR, confirmed by diff, and a cold tsc --noEmit of HEAD core exits 0). Estimator oracle (estimator-oracle.mjs) compares against JSON.stringify bytes directly and replays the base formula mechanically from the reverted hunk. Gates ran via npx vitest run <exact files> inside each package. Mutations applied in place with file backups, each restored and verified by empty git diff afterwards; raw per-cell logs in logs/. One pre-existing environmental artifact: the PR's own E2E-plan file was absent from the worktree (root-owned read-only .qwen/ directory prevented restore); it was read from git show HEAD: and does not affect build or tests.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
Maintainer verification — real daemon, no mocksI built two full bundles from source ( The lever for ~1 MiB replies is config-only, so the payloads are genuine dispatcher output rather than injected frames: the fixture registers 245 models under This closes the evidence gap the PR itself names ("that real daemon RSS scenario was not executed in this local pass"). The byte bound, the per-owner refusal, the status surface, the shared global budget and the ownership transactionality all behave as described. Two things are worth a maintainer's attention before merge — they are noted in §6 and §7, and neither is a regression. 1. The pre-attach byte bound holds against a real daemonOne connection, session stream never attached, 128 × ~1 MiB replies.
The refusal lands exactly where the constant says it should: 65 frames × 1,025,198 B = 66,637,870 B fits, the 66th would cross 64 MiB, and that admission closes the connection. Reproduced identically across two independent runs. One honest caveat on the RSS column: at equal frame counts head's RSS is not lower than base's (transient 2. Silent eviction becomes fail-closed — this is the strongest argument for the breaking change300 ×
On base, all 300 POSTs are accepted with 3. Refusal closes only the admitting ownerThe daemon stayed healthy on both arms after the guard fired: a fresh 4. Status and SDK surface
All five limits are exposed with the documented values, the runtime counters track and release correctly, and per-mount attribution is populated. The same endpoint on base has none of these fields. 5. Ownership is transactional with local delivery
6. One global budget across mounts — verified, but not to exhaustion
With I did not drive this to the 4,096-frame / 256 MiB global ceiling — reaching it needs four-plus connections each parked just under their own 64 MiB cap, i.e. ~250 MiB of live buffers. The sharing is proven; the global refusal edge is still only covered by unit tests. 7. Scope note: an attached-but-unread WebSocket is still unboundedSame fixture, WS transport, client stops reading its socket after
These frames take the live fast path into 8. Independent check of the JSON byte estimatorI wrote my own differential oracle (not the PR's test) against the built package: every UTF-16 code unit standalone, every code unit in escaping-sensitive context, astral code points across all planes, 40,000 deterministic random strings, and 5,000 randomized early-exit limit cases. 9. Test suites reproduce on head
(Counts are higher than the PR body's 46/442 because I ran the whole files rather than a filtered subset.) VerdictEverything the PR claims about the pre-attach byte bound, per-owner refusal, delivery-tied ownership and the status surface reproduces on a real daemon, and the base-vs-head contrast is decisive — most of all the 44 silently dropped JSON-RPC replies on base. From a verification standpoint this is good to merge. §6 and §7 are follow-up scope for the tracking issue, not blockers: the global ceiling is proven-shared but not driven to exhaustion, and live WS/SSE queues remain unbounded by design after this change. Not covered: Windows and Linux, the WS 中文版本维护者本地验证 —— 真实 daemon，无 mock我从源码构建了两份完整 bundle（ 产生 ~1 MiB 回复的杠杆完全来自配置，因此 payload 是真实的 dispatcher 输出而不是注入帧：fixture 在 这补上了 PR 自己声明的证据缺口（"该真实 daemon RSS 场景本次本地未执行"）。字节边界、按 owner 精确拒绝、status surface、跨 mount 共享全局预算，以及 ownership 事务性，行为都与描述一致。合并前有两点值得维护者注意，见 §6 和 §7，两者都不是回归。 1. 真实 daemon 上字节边界成立单连接，session stream 始终不 attach，128 × ~1 MiB 回复。
拒绝点与常量完全吻合：65 × 1,025,198 = 66,637,870 B 可容纳，第 66 帧会越过 64 MiB，于是这次 admission 关闭连接。两次独立运行结果一致。 RSS 一栏有一个如实的说明：在相同帧数下 head 的 RSS 并不低于 base（ 2. 静默淘汰变为 fail closed —— 这是支持该 breaking change 最有力的证据在未 attach stream 的连接上发 300 次 base：300 次 POST 全部 3. 拒绝只关闭发起 admission 的 owner两条腿在 guard 触发后 daemon 都保持健康：新的 4. status 与 SDK surface五个 limits 都以文档值暴露，runtime 计数正确累计与释放，per-mount 归因有值。base 上同一端点完全没有这些字段。 5. ownership 与本地投递构成事务
6. 跨 mount 共享同一预算 —— 已验证，但未跑到耗尽使用 我没有把它推到 4,096 帧 / 256 MiB 的全局上限：要触达需要四个以上连接各自停在自己 64 MiB 上限之下，即约 250 MiB 的活缓冲。共享已被证明，全局拒绝的边界目前仍只有单测覆盖。 7. 范围说明：已 attach 但不读取的 WebSocket 仍然无界同一 fixture，WS transport，客户端在
这些帧走 live 快路径进入 8. JSON 字节估算器的独立校验我针对已构建的包写了自己的差分 oracle（不是 PR 自带的测试）：每个 UTF-16 code unit 单独、每个 code unit 处于转义敏感上下文、覆盖全部平面的 astral code point、40,000 个确定性随机字符串，以及 5,000 个随机化的提前退出 limit 用例。 9. head 上测试套件复现
（数量高于 PR 正文的 46/442，因为我跑的是整文件而不是过滤后的子集。） 结论PR 关于 pre-attach 字节边界、按 owner 拒绝、ownership 绑定投递以及 status surface 的全部声明，都在真实 daemon 上复现，base 与 head 的对比结论明确 —— 尤其是 base 上 44 条被静默丢弃的 JSON-RPC 回复。从验证角度可以合并。§6 与 §7 属于 tracking issue 的后续范围，不是阻塞项：全局上限已证明共享但未跑到耗尽，live WS/SSE 队列在本次改动后按设计仍然无界。 未覆盖：Windows 与 Linux、WS |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): PR #9007 bounds the daemon's ACP HTTP pre-attach bufferin...: none — all checks above completed within the tool budget.; You are review agent reverse-audit — Reverse audit agen...: none — all checks I started were completed within budget.; chunk 3: I could not execute the test file (the review worktree has no node_modules). All assertions were manually traced against the implementation instead.; chunk 3: could not execute connection-registry.test.ts to confirm it passes (no node_modules in the review worktree); all assertions were verified by manual trace ag…; You are review agent reverse-audit — Reverse audit agen...: none — all checks I started were completed within budget., and 9 more.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
中文说明
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：PR #9007 bounds the daemon's ACP HTTP pre-attach bufferin...：none — all checks above completed within the tool budget.；You are review agent reverse-audit — Reverse audit agen...：none — all checks I started were completed within budget.；chunk 3：I could not execute the test file (the review worktree has no node_modules). All assertions were manually traced against the implementation instead.；chunk 3：could not execute connection-registry.test.ts to confirm it passes (no node_modules in the review worktree); all assertions were verified by manual trace ag…；You are review agent reverse-audit — Reverse audit agen...：none — all checks I started were completed within budget.，另有 9 条。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
— qwen3.8-max via Qwen Code /review (v0.21.10)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Addressed the latest review round in
Verification: explicit TypeScript compilation of the test file (0 diagnostics), negative mutation checks for all four type-pin groups, SDK public-surface tests 14/14, SDK typecheck, root ESLint, Prettier, and |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): PR #9007 bounds daemon ACP HTTP pre-attach buffering by f...: none — all planned checks completed within budget.; PR #9007 bounds daemon ACP HTTP pre-attach buffering by f...: none (~17 of ~53 tool calls used; all planned checks completed).; PR #9007 bounds daemon ACP HTTP pre-attach buffering by f...: none — all checks I started were completed within budget.; PR #9007 bounds daemon ACP HTTP pre-attach buffering by f...: none — all planned checks (baseline frame-write comparison, replay exclusivity, fake-vs-real attached shapes, receipt result mapping, onFatalConnection wiri…; PR #9007 bounds daemon ACP HTTP pre-attach buffering by f...: none — all planned checks completed within budget., and 11 more.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
中文说明
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：PR #9007 bounds daemon ACP HTTP pre-attach buffering by f...：none — all planned checks completed within budget.；PR #9007 bounds daemon ACP HTTP pre-attach buffering by f...：none (~17 of ~53 tool calls used; all planned checks completed).；PR #9007 bounds daemon ACP HTTP pre-attach buffering by f...：none — all checks I started were completed within budget.；PR #9007 bounds daemon ACP HTTP pre-attach buffering by f...：none — all planned checks (baseline frame-write comparison, replay exclusivity, fake-vs-real attached shapes, receipt result mapping, onFatalConnection wiri…；PR #9007 bounds daemon ACP HTTP pre-attach buffering by f...：none — all planned checks completed within budget.，另有 11 条。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
— qwen3.8-max via Qwen Code /review (v0.21.10)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Addressed the current Critical review batch in
Local verification: CLI focused tests 100/100, SDK public-surface tests 14/14, CLI and SDK typecheck, focused ESLint, repository build, full lint, and |
4425139 to
e7ea3b2
Compare
|
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）为单个提交。 |
|
Rebased this PR onto
The repository-wide local build/typecheck remains blocked in unchanged areas by the current workspace's stale/unbuilt package declarations and the installed |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Unresolved, please confirm:
- [Critical] R9-1 (connection-registry.ts:1254): the connection-scoped settle-time requeue in settlePreparedAttempt fires only for outcome === 'closed' while the sibling session-scoped branch requeues any outcome !== 'delivered'; a conn-scoped frame settling 'outcome_unknown' is discarded rather than re-buffered. Author declined twice (comments 3787351742, 3788206580): outcome_unknown may have delivered the complete frame, so replay can duplicate a completed JSON-RPC response. The mechanism is present at the reviewed commit; whether drop-on-settle or duplicate-on-replay is the correct tradeoff cannot be settled from the code alone. The concrete unanswered-request residuals reachable in this territory are filed as new findings R12-2 and R12-4.
Not reviewed: reverse audit — stopped at the 5-round cap without two consecutive dry rounds (round 5 reported one finding, verified post-cap).
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — webui suite did not run (whole-call budget) and the web-shell suite timed out (infrastructure); focused changed-workspace suites and dependents ran green apart from failures measured pre-existing on the merge base (test-delta netNew: []).
中文说明
仅完成部分审查，审查缺口已披露。
未决，请确认：共 1 条（原文未翻译，列表见上方英文部分）。
未审查：reverse audit — stopped at the 5-round cap without two consecutive dry rounds (round 5 reported one finding, verified post-cap)。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：build-and-test — webui suite did not run (whole-call budget) and the web-shell suite timed out (infrastructure); focused changed-workspace suites and dependents ran green apart from failures measured pre-existing on the merge base (test-delta netNew: [])。
— qwen3.8-max via Qwen Code /review (v0.21.12)
|
Review round 12 follow-up (HEAD e7ea3b2):
No branch changes were made. All five review threads were answered without duplicate replies. |
|
@qwen-code /triage |
1 similar comment
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 947 passed · 0 failed · 947 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：947 通过 · 0 失败 · 947 总计 Verification reportPR #9007 Deep Verification (follow-up round) —
|
| # | Finding (previous round, head 23f1af33) |
Severity | Status at e7ea3b21 |
|---|---|---|---|
| 1 | Ownership commit gate (canCommitSessionOwnership consulted in ownershipReceipt.delivered()) unpinned by any dispatch-level test — M3 survivor |
Suggestion (coverage gap) | stands — re-measured: with the gate removed (M3), transport + dispatch trio run 372/372 green (suite grew from 364 last round). Positive control M3b (disable rollback-on-discard) now kills 16 tests (was 9) — the rollback half got more pinned, the commit half remains unpinned. The predicate itself stays unit-pinned at registry level (rejects an ownership commit while session/close is in flight, connection-registry.test.ts:968), which is why M3 leaves every suite green: M3 removes the caller, not the predicate. |
All carried-forward measurements were re-run at the new head and new base — the input closure changed (rebase onto #8817 + fixture commit), so nothing was carried by shortcut. The previous round's harnesses were rebuilt from the dist API (artifact dir from run 31760035992 does not persist across runs).
Central claim + A/B
Central claim: daemon-owned ACP HTTP pre-attach buffering is bounded by frame count and serialized bytes at per-stream / per-connection / process-global scope, and refusal fails closed on the admitting owner instead of silently evicting an older frame.
Each arm's compiled dist is driven directly over real loopback HTTP (real SseStream, real fetch client). Head arm = the workflow-built packages/cli/dist; base arm = the six HEAD^1 modules transpiled with esbuild into a parallel tree (tmp/base-dist/). The import closure of every module both arms load is node-builtins + relative paths only — scripted check in harness/matrix-check.mjs asserts no package imports and that the base eviction marker (MAX_BUFFERED_FRAMES = 256) exists only in the base module — so no workspace symlink can contaminate either arm. package.json/package-lock.json are untouched by the PR (verified), so there is no dependency confound. Witnesses: 01-ab-head-arm-bounds-fail-closed.png, 02-ab-base-arm-silent-eviction.png.
| Cell | Scenario | HEAD (e7ea3b21) |
BASE (9f8f65dd) |
|---|---|---|---|
| C1 | 257 id-bearing frames, one session, no stream attached | #257 refused (failed); owning session binding closed+removed; connection survives; guard callback + guardFailures fire exactly once; teardown releases all 256 leases (no budget leak) |
all 257 accepted (void API); buffer capped at 256 by silently evicting the oldest (buffer[0].id 1→2; stderr dropped frame id 1) |
| C2 | 257 id-less replies | #257 refused; session closed; connection survives | buffer grows to 257 > 256 (id-less "irreplaceable", exempt to the 1024 hard cap) |
| C3 | 3 × ~120 KiB frames vs 300 KB per-connection cap (constructor seam) | frames 1–2 buffered, bytes charged exactly (245,850 B); #3 refused; connection destroyed; guard logs connection budget |
all 3 retained; no byte bound; connection alive |
| C4 | two connections, shared 6-frame global budget | A:3+B:3 charged; B's 4th refused; B destroyed; bystander A intact with all 3; guardFailures counted exactly once |
no global budget exists — B retains all 4; registry ctor has no budget seam; AcpPreAttachBudget export absent |
| C5 | buffered frames flushed on attach, real SSE wire | 3 frames byte-equal, in order; all 3 send promises resolve delivered |
identical wire parity (void API) |
| C6 | lease accounting while delivery is pending (deterministic hanging stream seam) | bytes/frames charged exactly at buffering; pendingDeliveryFrames=3 during stall; all counters 0 after teardown; high-water retained |
no lease accounting — pendingDeliveryFrames/preAttachOwned* fields absent from diagnostic |
| C7 | delivery-receipt lifecycle | unsettled while buffered; teardown → discarded; wire delivery → delivered (frame byte-intact) |
sendConn accepts no receipt (arity 1); ownership granted pre-delivery by construction |
Head arm 33/33; base arm 19/19 (every base assertion encodes the predicted broken behaviour; C5 checks are shared parity cells). Note: C3/C4 use constructor-seam caps (300 KB / 6 frames) to make triggers affordable; production defaults (64 MiB / 1024 conn, 256 MiB / 4096 global) are the same code paths, and the exact-cap boundary tests live in the committed suites (M1 red list below).
Delta probes (rebase onto #8817 + fixture commit)
Fork surface (the #8817 intersection). #8817 added checkpoint-anchored branching (sourceType/sourceId/atRecordId on BridgeBranchSessionRequest, "crash-safe ownership and referenced backups"). Interaction checks:
- Dispatch forwards only
name— ACPsession/forknever passesatRecordId, sobranchSessionalways takes the restore path and returns the fullBridgeBranchedSessionshape; the persisted-onlyBridgePersistedBranchedSessionvariant (which lacksclientId/attached) is only produced whenatRecordId !== undefined, i.e. the REST/web-shell path ACP dispatch never uses. Theas BridgeBranchedSessioncast in dispatch is therefore safe. (Static trace ofbridge.tsbranchSession +dispatch.tsfork case.) - Aligned fixtures are faithful: the mock
branchSessionintransport.test.tsreturnssessionId/workspaceCwd/attached/clientId/state/displayName/forkedFrom— exactly restored-shape ∪forkedFromfrom the post-feat: support fork from any conversation #8817bridgeTypes.ts. - Aligned fixtures still pin: fork-filtered transport run 5/5 green; and the M3b positive control's red list contains all four fork-rollback tests —
removes an undelivered persistent fork when the connection closes,'detach'/'kill' a restored fork when the connection closes before the reply, androlls back session/fork through the bridge generation that created it(workspace-qualified). The fixture alignment did not remove the teeth. - Fork rollback flows into the pre-existing shared
deleteDaemonSessionIfOrphan(unchanged by this PR, fromserver/session-archive.ts), so feat: support fork from any conversation #8817's persisted-fork cleanup semantics are what the rollback inherits — the PR only decides when it fires.
Ambiguous WebSocket deliveries (re-measured with real ws 8.21.0 server/client over loopback, paused receiver, ws-ambiguity.mjs; witness 03-ws-ambiguous-delivery-preserved.png): healthy send → delivered + byte-equal frame; accepted-but-unconfirmed send whose socket terminates mid-flight → outcome_unknown; buffered frame flushed onto a WS stream that dies mid-delivery → receipt settles exactly outcomeUnknown, never discarded (ownership is not rolled back for a delivery that may have happened). 5/5 at the new head.
Estimator (wire oracle = Buffer.byteLength(JSON.stringify(·)) itself, estimator-oracle.mjs; witness 04-estimator-oracle-exact.png): exact equality on all 65,536 single UTF-16 code units, 2,000 seeded random strings, 31 adversarial surrogate/escape shapes; early-exit returns exactly min(actual, limit+1) across a 363-point limit sweep; 2 MB mixed payload exact in 30 ms. Base formula replay (Buffer.byteLength(s)+2) under-charges 18/31 shapes — 700×U+0001: actual 4202 vs base 702, matching the PR's own test plan number.
Mutation matrix (re-run live at the new head)
Witness: 05-mutation-matrix-at-new-head.png. Raw logs logs/mut-*.log; scripted verdicts in harness/matrix-check.mjs (10/10). All mutants reverted afterwards; git diff on the four mutated files clean.
| Mutant | Change | Suite | Result | First failing assertion |
|---|---|---|---|---|
| M1 | registry: disable per-connection byte + global lease refusal | connection-registry + pre-attach-budget | KILLED — 4 red / 62 green | bounds retained serialized payload bytes before stream attachment (+ accepts the exact byte limit and rejects the next byte, + 2 cross-workspace budget tests) |
| M2 | spawnChannel: revert estimator to base formula | acp-bridge spawnChannel | KILLED — 1 red / 41 green | charges JSON string escaping before admitting prepared responses (expected [Function] to throw an error — the intended behavioral assertion) |
| M3 | dispatch: remove canCommitSessionOwnership gate in delivered() |
transport + dispatch trio | SURVIVED — 0 red / 372 green | — |
| M3b | dispatch: disable rollback-on-discard (positive control) | transport + dispatch trio | KILLED — 16 red / 356 green | rolls back an undelivered fresh session when the connection closes (+15, incl. all 4 fork-rollback tests) |
| M4 | dispatch: delete failed-delivery error-frame retry in replyConn |
transport + dispatch trio | KILLED — 2 red / 370 green | the two session/load … cannot be serialized (live+buffered) tests |
| M5 | ws-stream: flatten outcome_unknown → delivered |
ws-stream | KILLED — 2 red / 20 green | close() marks an active accepted send outcome unknown (+ peer-loss-race test) |
M3b/M4/M5 prove the harness can make every relevant suite red through the receipt/ambiguity/retry mechanisms, so M3's survival is a coverage statement, not harness failure. M3 escalation note: the finer half of the same receipt (rollback) dies with 16 red, and the commit gate's predicate is pinned at registry level — the gap is precisely "no dispatch-level test drives a close/generation race through delivered()".
Findings
- Suggestion (carried forward, stands) — ownership commit gate unpinned at dispatch level. Reproduce:
python3 harness/mut-apply.py m3(removes thecanCommitSessionOwnershipconsultation fromownershipReceipt.delivered()indispatch.ts), thencd packages/cli && npx vitest run src/serve/acp-http/transport.test.ts src/serve/acp-http/dispatch-error.test.ts src/serve/acp-http/dispatch.sandbox.test.ts src/serve/acp-http/workspace-qualified-acp.test.ts→ 372/372 green;mut-apply.py m3b→ 16 red. The gate is live, not dead code:closeSessionStreambumps the ownership generation andsession/closeinterposes, so a reply whose write settles after a close race is exactly the interleaving the gate decides. A fixture that would pin it:session/new(orsession/fork) reply in flight on a live connection stream,session/closefor that id landing before the write settles, then assert the session is not re-owned. Not merge-blocking — the guard exists and is correct as written; completeness reporting only.
No new findings this round.
Not covered
- Real-daemon E2E (committed plan: 128 × 1 MiB distinct-payload RSS, dynamic-workspace mounts, stalled-writer inspection, live
GET /daemon/status?detail=full): not executed — the PR states it was not run locally either. The plan file is intact in git (git show HEAD:.qwen/e2e-tests/2026-08-12-daemon-acp-http-pre-attach-bounds.md, 31 lines) but cannot be materialized in this worktree:.qwen/is root-owned read-only (dr-xr-xr-x root root) — environmental, identical to the previous round. The A/B above proves the same mechanisms at registry/stream level over real HTTP/SSE/WS; that is the handling, not the end-to-end RSS trigger. - Per-commit attribution: the depth-2 merge-ref checkout is grafted (
git rev-list HEAD^1..HEAD^2returns 1 vs the snapshot's 16 commits), so only the aggregateHEAD^1..HEADdiff was verified. The fixture commite7ea3b21's own parent is unreachable; its effect was scoped by fixture-shape validation + fork-filtered run + M3b red list instead of an isolated diff. - Dispatch-level A/B against base for ownership transactionality (needs the full bridge runtime): covered by C7 + gates + M3/M3b/M4 + WS probe instead.
- Ordinary live SSE/WS new-frame queues and transient single-frame
JSON.stringifyamplification (PR-declared non-goals); Windows (PR tested macOS only); repo-wide lint/typecheck (PR's own CI ran them; the workflow pre-rannpm ci+npm run buildat HEAD, which compiled every touched file clean). - The WS close-code 1013 claim beyond code inspection:
retireConnectionpasses{ code: 1013, reason: 'Resource limit' }(read in dist; exercised through the committed overflow tests in the gate), but my harness asserteddestroyed=truerather than the wire code on a shared WS.
Methodology
Environment: node:22-bookworm CI container, Node v22.23.2, tree at refs/pull/9007/merge (depth 2, grafted), npm ci + npm run build pre-run at HEAD. A/B harness (harness/ab-pre-attach.mjs) imports each arm's compiled modules by direct relative path — head from the workflow-built packages/cli/dist, base from tmp/base-dist/ (six HEAD^1 modules transpiled with esbuild, format=esm target=node22); a scratch worktree at HEAD^1 supplied the sources and was git worktree remove --forced after capture. Arm isolation is scripted (no package imports in either closure; base eviction marker present only in base). WS probe (harness/ws-ambiguity.mjs) uses the real ws 8.21.0 package with a paused receiver to keep writes genuinely in flight. Estimator oracle (harness/estimator-oracle.mjs) compares against JSON.stringify bytes directly and replays the base formula mechanically. Gates ran via npx vitest run <exact files> per package (CLI 812, acp-bridge 47, SDK 14). Mutations applied by harness/mut-apply.py with backups under mut-backup/, each restored and verified by empty git diff. Evidence captures via scripts/verify-capture.mjs; raw per-cell logs in logs/.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
|
Triage re-run completed without a new review. The bot already has a review of its own on 机器人在 The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
Maintainer re-verification at
|
|
Thanks for the full real-daemon re-verification. I independently checked both non-blocking observations against this exact head. §6a is accurate: step 5 of the committed E2E plan says counters return immediately to baseline, while the shipped reconnect-safe behavior requeues known-undelivered replies and keeps them charged until reconnect or grace expiry. I am deferring that documentation-only correction under the repository post-five-review-round Critical-only gate. §6b matches the PR explicit live-queue non-goal and requires no branch change. No code changes were made. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — stopped at the 5-round cap without two consecutive dry rounds (round 5's candidates were all ruled already-discussed or deferred ledger items).
Not explored to full depth (tool budget reached): chunk 10: could not run pre-attach-budget.test.ts (no node_modules in the worktree or the parent checkout; installing the full dependency tree was too costly for the re…; chunk 10: could not execute pre-attach-budget.test.ts (no node_modules in the worktree or parent checkout; a full install exceeded the remaining budget) — assertions we….
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：reverse audit — stopped at the 5-round cap without two consecutive dry rounds (round 5's candidates were all ruled already-discussed or deferred ledger items)。
未探索到全部深度（达到工具调用预算）：chunk 10：could not run pre-attach-budget.test.ts (no node_modules in the worktree or the parent checkout; installing the full dependency tree was too costly for the re…；chunk 10：could not execute pre-attach-budget.test.ts (no node_modules in the worktree or parent checkout; a full install exceeded the remaining budget) — assertions we…。
— qwen3.8-max via Qwen Code /review (v0.21.12)
|
Round 13 review handling at exact HEAD
Verification: real transport and registry probes passed 9/9; focused |
Re-verified after round 13 — no code changed, so I tested the two declined Criticals insteadFirst, the boring part: there is no new code to verify. The head is still What did change is that round 13 raised two Criticals and both were declined on empirical grounds, with the reviewer citing probes and the author citing counter-probes. That is the only open question left on this PR, so I re-ran both against a real The two findings came out differently.
R13-1 — reproduced, deterministicallyOne connection-scoped reply, the connection SSE stream torn down mid-write, then a reconnect well inside the 10-second grace window. Two runs per arm, identical results:
Three things this settles:
But it is not a regression. Base never answers any of these cases, including the one this PR fixes. So R13-1 is an unfixed neighbour of a real improvement, not something the PR breaks — which is a follow-up argument, not a merge blocker. R13-4 — I could not reproduce it48 real-transport trials across four shapes, each driving a session-scoped reply while a concurrent
The last shape is the one that matters: with ~400 registered models the The source-level asymmetry is real and plain to read. On this evidence the reachable failure is not, which is consistent with the author's position. I am not claiming it is impossible — only that I could not get there from a client, and neither shape I could build produced an unanswered id. One methodology note against myself: an earlier version of this harness reported R13-4 as reproduced. That was my bug — the stalled reader was never resumed, so I was judging delivery on a socket I had refused to read. Every number above comes from the corrected harness, which drains the socket before deciding. Where this leaves the merge decisionUnchanged from my previous report: good to merge. Everything the PR claims still reproduces on a real daemon at this head. R13-1 is real and worth a follow-up issue — the one-line widening is verified to work — but base is no better, so holding this PR for it would keep the 2.6 GB unbounded connection and the 44 silently dropped replies in 中文版本Round 13 后的复验 —— 没有代码变更，所以我改为验证两条被拒绝的 Critical先说结论中平淡的部分：没有新代码需要验证。 head 仍是 真正变化的是：round 13 提出了两条 Critical，两条都以实测为由被拒绝 —— reviewer 附了探针证据，作者附了反向探针证据。这是本 PR 目前唯一悬而未决的问题，所以我针对真实 两条结论并不相同。 R13-1 —— 可确定性复现单条 connection 作用域回复，connection SSE 流在写入中途被拆除，然后在 10 秒 grace 窗口内重连。每条腿两轮，结果一致：
这解决了三件事：
但这不是回归。 base 上这些情形一个都不应答，包括本 PR 修好的那一个。所以 R13-1 是一个真实改进旁边尚未修复的邻居，而不是本 PR 引入的问题 —— 这是后续项的论据，不是合并阻塞项。 R13-4 —— 我无法复现48 次真实传输试验，覆盖四种形态，每次都在并发
最后一种形态才是关键：注册约 400 个模型后， 源码层面的不对称是真实存在且一目了然的。但就这些证据而言，可触达的失败并不存在，这与作者的立场一致。我并不主张它不可能发生 —— 只是我无法从客户端到达那里，我能构造的两种形态都没有产生未被应答的 id。 一条针对我自己的方法学说明：本 harness 的早期版本曾报告 R13-4「已复现」。那是我的错误 —— 阻塞的读取端始终没有恢复读取，我是在一个自己拒绝读取的 socket 上判断投递结果。上面所有数字均来自修正后的 harness，它会在判定前先把 socket 读空。 这对合并决策意味着什么与上一份报告一致：可以合并。 PR 的全部声明在该 head 的真实 daemon 上依旧复现。R13-1 是真实存在的，值得开一个后续 issue —— 那一行放宽已被验证有效 —— 但 base 并不更好，为它扣住本 PR，等于为了一个 |
|
Round 13 maintainer re-verification addendum
Current state: 0 unresolved threads, 22 passing checks, mergeable without conflicts. GitHub still reports the earlier bot |
|
@Qwen-Coder /triage |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM — +1 per maintainer request. CI green at e7ea3b2.
already have 2 approves，3ks
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): chunk 16: completing vitest run test/unit/daemon-public-surface.test.ts in packages/sdk-typescript — one attempt timed out after 5 minutes with no output; since the add….
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：chunk 16：completing vitest run test/unit/daemon-public-surface.test.ts in packages/sdk-typescript — one attempt timed out after 5 minutes with no output; since the add…。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| settled = true; | ||
| if (attached) { | ||
| try { | ||
| void runtime.bridge.detachClient(sessionId, clientId).catch(() => {}); |
There was a problem hiding this comment.
[Suggestion] ownershipReceipt's attached rollback arm swallows async detach failures silently: detachClient(...).catch(() => {}) leaves no log, while the non-attached arm (removeOrphanSession) logs qwen serve: /acp orphan killSession(...) failed: ..., and the pre-PR fork-race cleanup logged both arms. detachClient itself has no internal failure logging. — Concrete cost: if a rollback detach fails (bridge error, session state racing), the daemon-issued clientId stays registered in the bridge's voter/known-client sets and there is no log line anywhere for an operator to correlate the phantom participant with; the detach path strictly lost observability versus the removed code.
| void runtime.bridge.detachClient(sessionId, clientId).catch(() => {}); | |
| void runtime.bridge.detachClient(sessionId, clientId).catch((err) => { | |
| writeStderrLineSafe( | |
| `qwen serve: /acp orphan detachClient(${logSafe(sessionId)}) failed: ${logSafe(errMsg(err))}`, | |
| ); | |
| }); |
中文说明
[Suggestion] ownershipReceipt 的 attached 回滚分支会静默吞掉异步 detach 失败：detachClient(...).catch(() => {}) 不打任何日志，而非 attached 分支（removeOrphanSession）会打印 qwen serve: /acp orphan killSession(...) failed: ...，且 PR 之前的 fork-race 清理对两个分支都有日志。detachClient 自身也没有失败日志。— 具体代价：如果回滚 detach 失败（bridge 错误、session 状态竞态），daemon 签发的 clientId 会残留在 bridge 的 voter/known-client 集合中，运维没有任何日志可用来关联这个幽灵参与者；相对被删除的代码，detach 路径严格失去了可观测性。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| if ((closeRaced || replayArmRaced) && !conn.destroyed) { | ||
| conn.sendConn( | ||
| error( | ||
| id, | ||
| RPC.INTERNAL_ERROR, | ||
| `session ${sessionId} was closed during load; retry`, | ||
| ), | ||
| ); |
There was a problem hiding this comment.
[Suggestion] The retry error sent when replayArmRaced fires without closeRaced misstates the cause — the session was not closed; the load lost an ownership-generation race (armInitialReplay() returned false because canCommitSessionOwnership failed on a replaced binding). — Failure scenario: a session/load for an already-owned session races a generation replacement while configOptionsFor awaits; the client receives session … was closed during load; retry even though no close is in flight — this PR's own test does not arm initial replay on a replacement session generation asserts this -32603 while conn.ownsSession('sess-1') is true immediately afterwards. The same message also fires when the actual closer was a resource-guard retirement (failOwner → closeSessionStream), where no session/close ever happened. An operator correlating this error hunts a close that never happened.
| if ((closeRaced || replayArmRaced) && !conn.destroyed) { | |
| conn.sendConn( | |
| error( | |
| id, | |
| RPC.INTERNAL_ERROR, | |
| `session ${sessionId} was closed during load; retry`, | |
| ), | |
| ); | |
| if ((closeRaced || replayArmRaced) && !conn.destroyed) { | |
| conn.sendConn( | |
| error( | |
| id, | |
| RPC.INTERNAL_ERROR, | |
| replayArmRaced && !closeRaced | |
| ? `session ${sessionId} load lost an ownership race; retry` | |
| : `session ${sessionId} was closed during load; retry`, | |
| ), | |
| ); |
中文说明
[Suggestion] 当 replayArmRaced 触发而 closeRaced 未触发时，发出的重试错误措辞与真实原因不符——session 并未被关闭，而是 load 输掉了一次 ownership generation 竞态（armInitialReplay() 因 canCommitSessionOwnership 在已被替换的 binding 上失败而返回 false）。— 失败场景：对已拥有 session 的 session/load 在 configOptionsFor await 期间遭遇 generation 替换；客户端收到 session … was closed during load; retry，但此刻并没有任何 close 在进行——本 PR 自带的测试 does not arm initial replay on a replacement session generation 正是在断言这个 -32603 的同时 conn.ownsSession('sess-1') 为 true。同样的措辞也会在资源守卫触发退役（failOwner → closeSessionStream）时出现，而那里从未发生过 session/close。运维按此错误排查时，会去寻找一个从未发生的 close。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| pendingDeliveryFrames += binding.buffer.filter( | ||
| (prepared) => | ||
| prepared.lease !== undefined && | ||
| prepared.deliveryAttempt !== undefined, | ||
| ).length; |
There was a problem hiding this comment.
[Suggestion] Per-connection pendingDeliveryFrames in getDiagnostic scans session buffers plus pendingDeliveries, but never connBuffer — a conn reply requeued by settlePreparedAttempt's conn arm lands in connBuffer with a lease that stays markPendingDelivery()-ed, so the global budget keeps counting it while this diagnostic does not. — Failure scenario (probe-verified at this commit): a conn reply delivered live when the conn stream closes with 'closed' is requeued into connBuffer with a marked lease; GET /daemon/status then shows top-level pendingDeliveryFrames: 1 while the owning connection's diagnostic shows pendingDeliveryFrames: 0 (the frame appears only in bufferedConnectionFrames) — an operator summing per-connection counters to reconcile against the global figure sees a permanent unexplained discrepancy until the frame is redelivered or discarded. Suggested fix — count the same lease && deliveryAttempt shape over connBuffer after the loop:
pendingDeliveryFrames += this.connBuffer.filter(
(prepared) =>
prepared.lease !== undefined &&
prepared.deliveryAttempt !== undefined,
).length;中文说明
[Suggestion] getDiagnostic 中的单连接 pendingDeliveryFrames 只统计 session buffer 和 pendingDeliveries，从不统计 connBuffer —— 被 settlePreparedAttempt conn 分支重入队的连接回复会带着仍处于 markPendingDelivery() 状态的 lease 落入 connBuffer，全局预算继续计数，而该连接诊断却不计。— 失败场景（已在被审 commit 上用探针验证）：conn 流以 'closed' 关闭时，一条已 live 投递的连接回复被重入队进 connBuffer 且 lease 已标记；GET /daemon/status 顶层显示 pendingDeliveryFrames: 1，而所属连接的诊断显示 pendingDeliveryFrames: 0（该帧只出现在 bufferedConnectionFrames 中）——运维用单连接计数对账全局数字时，会看到一个直到该帧被重投或丢弃之前都 permanent 无法解释的差值。建议修复：在循环之后对 connBuffer 按同样的 lease && deliveryAttempt 条件计数。
— qwen3.8-max via Qwen Code /review (v0.21.11)
|
Released in v0.21.13. |
* fix(acp): Account for JSON string escaping in response budgets Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(serve): Bound ACP HTTP pre-attach buffers by bytes Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(serve): Preserve ACP pre-attach stream scope Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(serve): Expose ACP guard failures by workspace Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (QwenLM#9007) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (QwenLM#9007) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (QwenLM#9007) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(serve): Preserve ambiguous WebSocket deliveries Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(serve): Report failed ACP response delivery Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (QwenLM#9007) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (QwenLM#9007) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (QwenLM#9007) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (QwenLM#9007) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (QwenLM#9007) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): make websocket teardown logging safe Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * test(serve): align ACP fork fixtures after rebase Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
4 participants
What this PR does
This PR makes daemon-owned ACP HTTP pre-attach buffering bounded by both frame count and serialized payload bytes. It serializes buffered replies once into UTF-8 buffers, applies per-stream, per-connection, and process-global limits shared by primary and dynamic workspace mounts, retains budget ownership through stalled SSE or WebSocket delivery, and closes the exact session or logical connection on refusal instead of silently evicting an older frame.
It also makes session ownership grants transactional with local transport delivery.
session/new,session/load,session/resume, andsession/forkremain provisional until their JSON-RPC reply is locally delivered; teardown, overflow, serialization failure, or delivery failure rolls back fresh sessions, persisted forks, and new client attachments without revoking ownership that already existed. Notification forms of these ownership-granting methods no longer mutate state.The daemon status and TypeScript SDK surfaces now expose the effective pre-attach limits, current and high-water frame/byte usage, pending delivery ownership, guard failures, per-mount attribution, and per-connection owned frames/bytes. The accompanying capacity and resumable-stream documentation records the compatibility changes and the remaining non-goals.
The first commit also corrects prepared-response estimation for JSON string escaping, UTF-8, and surrogate handling so admission matches the bytes that
JSON.stringifyactually emits.Why it's needed
The existing 256-frame pre-attach cap controlled only item count and could silently drop an older frame. A small number of very large replies, multiple connections across dynamic workspaces, or an attached but stalled writer could still retain large buffers without a daemon-global byte boundary. Silent eviction was especially unsafe for id-less JSON-RPC replies because the client could wait forever for a response that had been discarded.
Session creation and attach operations were also recorded as owned before their reply was delivered. If a connection closed or overflowed before the client learned the session ID, the daemon could leave an unreachable fresh session, persistent fork, or bridge client attachment behind. Tying ownership to the local delivery result closes that lifecycle gap.
Reviewer Test Plan
How to verify
Run the focused ACP bridge tests and confirm the JSON string estimator matches
JSON.stringifyfor every UTF-16 code unit, mixed escaping, surrogate pairs, and deterministic random strings.Run the focused CLI transport tests and confirm the N+1 frame/byte admission closes only the correct owner, primary and dynamic workspace registries compete for one global budget, stalled SSE/WS delivery retains leases until callback/close, serialization re-entry cannot revive stale stream identities, replay-deferred replies preserve order, and every provisional ownership receipt is either committed after delivery or rolled back on teardown/failure.
Run the daemon status and SDK surface tests and confirm the new limits and runtime counters are stable when ACP HTTP is disabled and populated when enabled.
The committed E2E plan describes the real 128 × 1 MiB distinct-payload reproduction, cross-workspace global-budget competition, stalled writer inspection, and workspace disposal baseline check. That real daemon RSS scenario was not executed in this local pass.
Validation completed locally: ACP bridge focused tests 46/46, CLI focused tests 442/442, SDK public-surface tests 14/14,
npm run build,npm run typecheck,npm run lint, andgit diff --check.Evidence (Before & After)
N/A — this is daemon transport, lifecycle, status, SDK type, documentation, and test coverage with no TUI change.
Tested on
Environment (optional)
macOS, Node.js v24.12.0, repository npm workspaces and package-local Vitest configurations. The repository requires Node.js >=22; no Docker or sandbox runtime was used for the focused tests.
Risk & Scope
JSON.stringifyamplification, remote exactly-once receipts, and the real 128 × 1 MiB daemon RSS E2E remain outside this PR or were not run locally.Linked Issues
Tracking: #8091
中文说明
本 PR 做了什么
本 PR 为 daemon 自有的 ACP HTTP pre-attach 缓冲增加帧数和序列化 payload 字节数双重边界。缓冲回复只序列化一次并保存为 UTF-8 Buffer，同时应用单 stream、单 connection 和 primary/动态 workspace mount 共享的进程级全局限制；即使 SSE 或 WebSocket writer 已 attach 但阻塞，预算所有权也会保留到本地投递完成或明确失败。资源拒绝时不再静默淘汰旧帧，而是关闭精确 session 或逻辑 connection。
本 PR 还把 session ownership grant 与本地 transport delivery 做成事务闭环。
session/new、session/load、session/resume和session/fork在 JSON-RPC 回复本地投递完成前保持 provisional；teardown、overflow、序列化失败或投递失败会回滚 fresh session、持久化 fork 和新增 client attach，同时不会撤销原本已经存在的 ownership。这些会授予 ownership 的方法以 notification 形式调用时不再执行 mutation。daemon status 和 TypeScript SDK 现在会暴露有效的 pre-attach limits、当前和高水位帧数/字节数、pending delivery ownership、guard failure、每个 mount 的归因，以及每个 connection 自有的帧数/字节数。配套的 capacity 与 resumable-stream 文档记录了兼容性变化和剩余非目标。
第一笔 commit 还修复了 prepared-response 对 JSON 字符串转义、UTF-8 和 surrogate 的计费，使 admission 与
JSON.stringify实际输出字节一致。为什么需要
现有 256 帧 pre-attach 限制只控制条目数，并且可能静默丢弃旧帧。少量超大回复、多个动态 workspace connection，或已 attach 但阻塞的 writer，仍可能在没有 daemon 全局字节边界的情况下保留大量 buffer。对于无 id 的 JSON-RPC 回复，静默淘汰尤其危险，因为客户端可能永远等待一个已经被丢弃的响应。
session create/attach 操作之前也会在回复投递前就记录为 owned。如果 connection 在客户端得知 session ID 之前关闭或 overflow，daemon 可能遗留客户端无法访问的 fresh session、持久化 fork 或 bridge client attachment。把 ownership 与本地 delivery result 绑定可以闭合这一生命周期缺口。
Reviewer 测试计划
如何验证
运行聚焦 ACP bridge 测试，确认 JSON 字符串 estimator 对全部 UTF-16 code unit、混合转义、surrogate pair 和确定性随机字符串都与
JSON.stringify一致。运行聚焦 CLI transport 测试，确认第 N+1 个 frame/byte admission 只关闭正确 owner，primary 与动态 workspace registry 竞争同一全局预算，阻塞的 SSE/WS delivery 在 callback/close 前持续持有 lease，序列化重入不能复活 stale stream identity，replay deferred reply 保持顺序，并且每个 provisional ownership receipt 都只会在 delivery 后提交或在 teardown/failure 时回滚。
运行 daemon status 与 SDK surface 测试，确认 ACP HTTP 关闭时新 limits/runtime 字段保持稳定，开启时正确填充。
已提交的 E2E 计划描述了真实 128 × 1 MiB distinct payload 复现、跨 workspace 全局预算竞争、阻塞 writer 观察和 workspace dispose 后回到基线的验证流程。本次本地验证未实际执行真实 daemon RSS 场景。
本地已完成验证：ACP bridge 聚焦测试 46/46、CLI 聚焦测试 442/442、SDK public-surface 测试 14/14、
npm run build、npm run typecheck、npm run lint和git diff --check。证据（Before & After）
N/A —— 本 PR 修改 daemon transport、生命周期、status、SDK 类型、文档和测试，不涉及 TUI 变化。
测试平台
环境（可选）
macOS，Node.js v24.12.0，仓库 npm workspaces 和包级 Vitest 配置。仓库要求 Node.js >=22；聚焦测试未使用 Docker 或 sandbox runtime。
风险与范围
JSON.stringify瞬时放大、远端 exactly-once receipt 不在本 PR 范围内；真实 128 × 1 MiB daemon RSS E2E 本地未执行。关联 Issue
Tracking: #8091