fix(serve): Keep restore request shapes distinct - #8933
Merged
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
This was referenced Aug 11, 2026
Collaborator
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterℹ️ No screenshot changed against the PR base — but this PR edits 1 render-shaping file:
Either the change has no visual effect (logic, plumbing, a state the scenarios never reach), or no scenario renders this UI — in which case the preview cannot see it, and an empty result is a coverage gap rather than a clean bill of health. To make it visible, add a scenario to Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Collaborator
Author
|
Broad audit follow-up: the WebUI coordinator and ACP bridge diff were rechecked against adversarial same-shape, different-shape, A→B→A, ordinary-timeout retry, explicit lifecycle-cancellation, warm-session, coalesced attachment, and abandoned-restore paths. No further actionable code issue was found after d47eba4. Focused WebUI, bridge, and real-daemon tests; Prettier, focused ESLint, build, typecheck, and diff checks pass. The Ubuntu Java 17 dependency-resolution failure was confirmed unrelated to this TypeScript-only diff and its failed job rerun passed. One general Ubuntu Node CI job is still running; the PR remains Draft. |
doudouOUC
marked this pull request as ready for review
August 11, 2026 12:24
doudouOUC
enabled auto-merge
August 11, 2026 12:24
Collaborator
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. |
Collaborator
|
Third pass over this PR — still no new commits since the original review (
Moving on to code review. 🔍 中文说明本 PR 的第三轮审查——自首次审查（
进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Code review (re-verified on this pass)I re-formed my independent proposal before re-reading the diff: put the normalized request shape (action + page) into the in-flight restore identity at both coordinator boundaries, reject/serialize non-equivalent requests through the existing conflict path, snapshot the effective page at intent admission so config changes can't alter queued attempts, and fence superseded results one-way while letting same-lifecycle timeout retries still adopt a late same-shape result. The PR's approach matches this essentially line for line — I did not find a simpler path it missed. What I verified against the checked-out head this pass (not just the description):
No critical blockers and no convention violations found — this matches @yiliang114's approving review. The production diff is 81 lines; the other ~843 lines are unit and real-daemon regression tests that pin every scenario in the test plan (same-page coalescing, invalid/mismatched pages, stream/resume page-ignoring, inherited-history policy, page snapshot across retries, A→B→A supersede, lifecycle-cancellation fencing, no-pagination normalization). One non-blocking nit: the new admission validation throws a bare One sequence for the key path, since the settle-time fencing is the subtle part: sequenceDiagram
participant P1 as Caller action
participant P2 as Coordinator
participant P3 as SDK restore call
participant P4 as ACP bridge
P1->>P2: loadSession or resumeSession
P2->>P2: key = target plus replay shape
alt exact-shape intent already desired
P2-->>P1: coalesce onto same promise
else different shape
P2->>P2: latch raw result as superseded
P2-->>P1: reject prior intent with AbortError
P2->>P2: queue the new intent
end
P2->>P3: run raw with snapshotted page
P3->>P4: session load or resume
P4-->>P3: restored session
P3->>P2: settle
alt superseded, lifecycle or generation changed
P2->>P4: detach stale attachment once
P2->>P3: run the queued intent fresh
else all guards match
P2->>P2: commit the target session
end
Test evidence — the PR's own CI at
|
| Check | Conclusion |
|---|---|
Test (ubuntu-latest, Node 22.x) |
❌ failure |
Capture web-shell visuals (ubuntu-latest, Node 22.x) |
✅ success |
Classify PR |
✅ success |
Desktop Shell (ubuntu-22.04) |
✅ success |
Desktop Shell (windows-2022) |
✅ success |
Post Coverage Comment (ubuntu-latest, 22.x) |
✅ success |
Real daemon E2E / Java 11 |
✅ success |
macos-latest / Java 21 |
✅ success |
ubuntu-latest / Java 11 |
✅ success |
ubuntu-latest / Java 17 |
✅ success |
ubuntu-latest / Java 21 |
✅ success |
web-shell E2E Smoke (ubuntu-latest, Node 22.x) |
✅ success |
windows-latest / Java 21 |
✅ success |
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。
Classification of the one red check — environmental on all three attempts, with the evidence for that call:
- Attempt 1: only
shell-ast-parser-lazy.test.tsfailed (web-tree-sitter WASM-init 15 s timeout under load) — zero overlap with this PR's diff. - Attempt 2: every test suite passed; the job died on a heap OOM in web-shell's coverage processing, minutes after web-shell's last test passed.
- Attempt 3 (job
93849425200, 16:27–16:46 UTC, verified from the job log on this pass): all lint gates,tsc --noEmit, andtsc --buildpassed first. Then every suite that completed was green — including the two packages this PR touches:@qwen-code/acp-bridge26 files / 1233 tests passed and@qwen-code/webui34 files / 521 tests passed, plus the root workspace 798 files / 19056 passed. The step died when@qwen-code/web-shell'stest:civitest worker hitFATAL ERROR: Reached heap limit Allocation failed - JavaScript heap out of memoryat ~3.1 GB (16:44:57); the downstreampackages/web-shell/junit.xmlreporter error is noise from that death. Web-shell's own suite was cut short by the OOM this time, but the web-shell E2E Smoke and visuals checks — which also exercise it — passed on this head. - This PR touches zero
packages/web-shellfiles. Three attempts, three distinct failure modes (WASM-init timeout, coverage heap exhaustion, vitest-worker heap exhaustion), all in load/infra territory on the self-hosted runner, all with the PR's own suites green. - Cross-PR corroboration that this is runner load, not repo breakage: the same check went green for other PRs the same day — e.g.
fix/sharp-0.35-bumpfailed at 16:33 and passed at 16:47 after one re-run — so a re-run can land in a good load window.
Net effect: the matrix is still incomplete on this head — Test (macos-latest / windows-latest, Node 22.x) and the real-daemon Integration Tests (CLI, No Sandbox) job (which carries this PR's new fetch-intercepted serialization test) remain skipped behind the red ubuntu job.
Sandboxed verification — the lane has fired, and it is no longer a gap:
- Verify run 1 (workflow run 31511434799): ✅ passed — merge-ready (agent verdict), 25/25 scripted assertions, A/B against the base build in an isolated token-free container on exactly this head (
d47eba4f41). This is the real-daemon evidence the skipped integration job never produced here. Advisory evidence for human reviewers — not a review, an approval, or a CI check. - Verify run 2: a second independent run is in flight from this trigger and will post its own report in this thread when it completes.
Real-scenario tmux testing is not applicable here — this is an unattended CI run; live-behaviour signal comes from the isolated verify lane, and the agent env never executes PR code.
中文说明
代码审查（本轮重新核实）：重读 diff 前我先独立写下方案——在两个协调边界的 in-flight 恢复标识中加入规范化请求形状（action + page），非等价请求走现有冲突路径串行化，intent 接纳时快照有效 page，superseded 结果单向 fence、同一 lifecycle 内的超时重试仍可采用迟到的同形状结果。PR 的实现与这个方案基本逐点对应，没有找到被遗漏的更简路径。
本轮在 checkout 的 head 上逐项核实（而非只看描述）：crossSessionKey() 只有一个调用点且已更新；rawTransitionRef 直接持有 intent 对象，接纳时的 resultSuperseded = true 能传到 settle 守卫读取的对象上，闩锁按设计单向（A→B→A 测试固化"被替代结果永不重新采纳"）；新增的 settle 守卫沿用现有 lifecycleRef 计数器模式——显式取消时计数器自增，恰好 fence 更早的同形状原始结果，而同一 lifecycle 内的普通超时重试仍可采纳迟到结果，CI 在本 head 上先于测试步骤跑绿了 tsc --noEmit 与 tsc --build，收窄无误；bridge 侧复用 core 中 transcript reader 现有的 SESSION_TRANSCRIPT_MAX_LIMIT（= 500）做准入校验，page 只附加到 load+response 形状，冲突类型仍为 RestoreInProgressError；其余直接读 historyPageSizeRef 的位置是重连/首连路径和 live session 分页，按当前值读取正确；bridgeTypes.ts 注释修正是纠正过期注释，非行为变化。
未发现阻塞性问题，也无项目规范违规——与 @yiliang114 的批准性审查结论一致。一个非阻塞小点：新的准入校验抛出裸 Error，与同函数内既有 preflight 风格一致，不必改。生产改动 81 行，其余约 843 行为测试，逐项固化测试计划中的全部场景。上方时序图给出关键路径（settle 时的 fencing 是最微妙的部分）。
测试证据：该 head 上唯一的红灯 Test (ubuntu-latest, Node 22.x) 三次尝试均为环境问题，证据如下。第一次：仅 shell-ast-parser-lazy.test.ts 的 WASM 加载超时（与本 PR diff 零重叠）。第二次：所有套件通过后死于 web-shell 覆盖率处理的堆溢出。第三次（job 93849425200，16:27–16:46 UTC，本轮直接核对日志）：全部 lint 门禁与 tsc --noEmit、tsc --build 先行通过；所有完成的套件全绿——包括本 PR 触碰的两个包：acp-bridge 26 文件 / 1233 通过、webui 34 文件 / 521 通过，主工作区 798 文件 / 19056 通过；步骤死于 @qwen-code/web-shell 的 test:ci vitest worker 在约 3.1 GB 处堆耗尽（16:44:57），下游 junit.xml 报错是该崩溃的噪音。web-shell 自身套件这次被 OOM 中断，但同样行使它的 web-shell E2E Smoke 与 visuals 检查在本 head 上通过。本 PR 未触碰任何 web-shell 文件；三次尝试、三种不同失败模式，均在自托管 runner 的负载/基础设施层面。同一天其他 PR 的交叉佐证：同一检查当天对其他 PR 为绿——如 fix/sharp-0.35-bump 16:33 失败、重跑一次后 16:47 通过——说明重跑可能撞上负载较低的窗口。
后果：该 head 的矩阵仍不完整——macos/windows 测试与真实 daemon Integration Tests (CLI, No Sandbox) job（承载本 PR 新增的 fetch 拦截串行化测试）因 ubuntu job 红灯继续被跳过。
沙箱验证——该通道已经启动，不再是缺口：verify 第一次运行（run 31511434799）✅ 通过——merge-ready（agent 判定），25/25 脚本断言，在隔离无凭证容器中与 base 构建 A/B 对照，且正是针对本 head（d47eba4f41）。这是被跳过的集成 job 未能在此 head 上产出的真实 daemon 证据（仅作为评审参考，不构成评审、批准或 CI 检查）。verify 第二次运行：本次触发带起的第二个独立运行正在进行，完成后会在本线程发布报告。真实场景 tmux 测试在此不适用——这是无人值守的 CI 运行，agent 环境绝不运行 PR 代码。
— Qwen Code · qwen3.8-max
Reviewed at d47eba4f41a3f254bee13add58ceb6d965bc0483 · re-run with @qwen-code /triage
Collaborator
|
Confidence: 4/5 — the code is clean across independent reads and the behavioural claim is now pinned by sandboxed A/B verification plus a maintainer approval; the withheld point is the required CI check, red for the third time on this head — and for the third time in a package this PR doesn't touch. What changed since my last pass, and what I made of it:
Why I'm converting the defer into an approval: the defer never had a code objection — it withheld the vote pending two things, that the red check be proven not-the-PR's and that the behaviour hold without the skipped integration job. Both are now settled with evidence rather than attribution-by-assertion. Approving while the check is red would normally be wrong; here the red is a heap ceiling in an untouched package, corroborated across attempts and across other PRs, and approval is the code vote — pinned to the exact commit below, auto-dismissed by any push, and it does not unlock merge: branch protection keeps this PR blocked until the required check goes green. ⏸️ The remaining decision is an infra one for @wenshao @yiliang114, unchanged from before: re-run the ubuntu job until a load window clears (that worked for other PRs today), or track the web-shell heap ceiling on the self-hosted runner separately — nothing in this PR can or should fix it. Re-running is the faster path to merge now that both approvals stand. 中文说明置信度：4/5 —— 代码经独立审查确认干净，行为声明已有沙箱 A/B 验证与维护者批准双重固化；扣掉的一分是该 head 上第三次为红的必需 CI 检查——且第三次红在本 PR 未触碰的包上。 自上轮以来的变化及判断：第三次尝试已执行并再次失败——第三种不同的环境性模式。直接核对 job 日志：全部 lint 门禁、 defer 等待的证据已经到位：沙箱 verify 运行 ✅ merge-ready——25/25 脚本断言、与 base 构建 A/B 对照、正是针对本 head，即被跳过的集成 job 未能在此 head 上产出的真实 daemon 串行化证据；第二个独立 verify 运行正在进行，稍后发布。@yiliang114 的批准也在同一 commit 上。代码本身本轮再次通读：形状感知标识、接纳时 page 快照、单向 supersede 闩锁、lifecycle fencing——81 行生产代码，无遗漏的更简路径，无夹带改动。 为什么把 defer 转为批准：defer 从来不是对代码有异议——它是在两项证据到位前扣住票数：红灯被证明非 PR 所致、行为在没有集成 job 的情况下依然成立。两者现在都有证据而非断言支撑。红灯时批准通常是错的；此处的红灯是未触碰包的堆上限，经多次尝试与多个 PR 交叉佐证，且批准只是代码层面的投票——精确钉在下方 commit 上、任何推送即作废，也不会解锁合入：分支保护仍会阻塞本 PR 直到必需检查转绿。 ⏸️ 剩余的是一个基础设施决定，转交 @wenshao @yiliang114，与上轮相同：重跑 ubuntu job 直到撞上空闲负载窗口（今天对其他 PR 有效），或单独跟踪自托管 runner 上 web-shell 的堆上限问题——本 PR 无法也不应修复它。在两票批准都已就位的情况下，重跑是更快的合入路径。 — Qwen Code · qwen3.8-max Reviewed at |
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.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite is outside the scoped workspace test run (this review ran it manually against the PR bundle: 3/3 passed).
Not explored to full depth (tool budget reached): PR #8933 makes restore-request coalescing shape-aware at ...: none — all planned checks completed within budget.; PR #8933 makes restore-request coalescing shape-aware at ...: none — all checks I started were completed (~25 tool calls).; PR #8933 makes restore-request coalescing shape-aware at ...: none — all planned checks completed within budget.; PR #8933 makes restore-request coalescing shape-aware at ...: none — the end-to-end daemon path (SDK body → route validation → bridge) was verified by reading DaemonClient.loadSession body serialization, parseHistoryPag….
中文说明
已审查。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite is outside the scoped workspace test run (this review ran it manually against the PR bundle: 3/3 passed)。
未探索到全部深度（达到工具调用预算）：PR #8933 makes restore-request coalescing shape-aware at ...：none — all planned checks completed within budget.；PR #8933 makes restore-request coalescing shape-aware at ...：none — all checks I started were completed (~25 tool calls).；PR #8933 makes restore-request coalescing shape-aware at ...：none — all planned checks completed within budget.；PR #8933 makes restore-request coalescing shape-aware at ...：none — the end-to-end daemon path (SDK body → route validation → bridge) was verified by reading DaemonClient.loadSession body serialization, parseHistoryPag…。
— qwen3.8-max via Qwen Code /review (v0.21.9)
yiliang114
approved these changes
Aug 11, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
LGTM on the code. The shape-aware identity (sessionId \0 normalized workspace \0 resume:none|load:all|load:recent:N) is mutually unambiguous with the page snapshotted at intent admission so mid-flight config changes can't alter shape; the supersede fence is monotonic and correctly handles A->B->A (never adopt the stale A result even when the latest intent is shape-A again); lifecycleRef bumps only on explicit cancellation/unmount so a same-lifecycle timeout retry can still adopt a late same-shape raw result while any cancellation fences it — the intended #8882 interplay, preserved; bridge page validation (safe integer 1..500 inclusive, matching the transcript reader's own bound and the route's 400-level parse) precedes warm admission and cold-restore coalescing so no legitimate restore is rejected; serialization is single-raw-slot + latest-queued-only with per-intent deadlines and time-bounded raw requests, and non-equivalent bridge requests reuse the existing 409 restore_in_progress machinery. Adversarial interleavings traced with no superseded-commit, double-attach, lost-wakeup, or deadlock found; #8882 arbitration untouched.
CI on head is red only on two out-of-scope flakes (core shell-ast-parser tree-sitter WASM 15s timeout on a severely loaded runner, and the web-shell vitest runner heap OOM after App.test.tsx — this diff is TypeScript-only serve/webui/bridge and its own suites passed 34/34 webui, 26/26 acp-bridge, 798/798 cli); a green rerun or flake annotation is the merge gate, not code changes. Note for merge: the new real-daemon integration test has no CI enforcement on this fork PR (integration job skipped; manual 3/3 only). Non-blocking follow-ups: one-line fence-condition debug log for oncall, reconcile #8743's keepalive-migration scope text (expect more surfaced 409s during rehydration windows until daemon-internal callers migrate), typed bridge page-validation error, and a page=500 inclusive-boundary test. Ship it after the rerun.
Collaborator
Author
|
@qwen-code /triage |
Collaborator
|
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: 25 passed · 0 failed · 25 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：25 通过 · 0 失败 · 25 总计 Verification reportPR 8933 — fix(serve): Keep restore request shapes distinctVerdict: 中文摘要
Central claim + A/BCentral claim: concurrent cold restores only coalesce when their effective replay shapes match ( Method: head's test files (bridge tests purely additive; provider file carries its own updated scaffolding) run against base source in a Witnesses: Bridge boundary (
|
| Scenario (same file both arms) | base (86a474ba) |
head (d47eba4f) |
|---|---|---|
| rejects invalid page 0 / 501 / 1.5 before restore (×3) | red — promise resolved (no validation, agent call issued) | green |
| rejects coalescing with different explicit page / omitted page (×2) | red — second caller coalesced instead of RestoreInProgressError |
green |
| same-page coalescing control (1 ACP call, independent attachments) | green | green |
| streamed loads / resumes ignore page (×2 controls) | green | green |
| different inherited-history policy rejected (control) | green | green |
| rest of suite | 548/553 pass | 553/553 pass |
WebUI boundary (packages/webui/.../DaemonSessionProvider.tsx)
| Scenario | base | head |
|---|---|---|
| serializes load and resume for same target | red (held load swallowed the resume; stall) | green |
| snapshots effective page for queued load | red (stall) | green |
| does not reuse superseded result when latest shape matches it | red (stall) | green |
| does not reuse raw result across lifecycle cancellation | red — expected 1 to be 2 (base adopted the fenced result) |
green |
| retries a switch using the page captured at intent time (strengthened pre-existing) | red — historyPageSize: 500 received where 100 expected |
green |
| standalone resume control | green | green |
| same-shape coalescing control | green | green |
| configured pages normalize to load/all without pagination (control) | green | green |
The base arm also surfaced 3 unhandled AbortError: Session runner restarted rejections (old coalescing settling intents at unmount); the head run has zero — the fence turns those silent leaks into explicit terminal states.
Head gates
| Gate | Result |
|---|---|
acp-bridge bridge.test.ts |
553/553 |
webui DaemonSessionProvider.test.tsx |
237/237 |
| acp-bridge full workspace | 26 files, 1233/1233 |
| webui full workspace | 34 files, 521/521 |
| typecheck (both packages) | exit 0 |
real-daemon integration (qwen-serve-webui-session-switching, mock ACP child, real HTTP) |
3/3, incl. new serialize scenario: 1 held /load, 0 premature /resume, source stays live, stale attachment detached once, then /resume commits (05-integration-real-daemon.png) |
Mutation matrix (head, each mutant restored after; 04-mutation-matrix.png)
| Mutant | Guard removed | Killed (exactly) |
|---|---|---|
| M1 provider | shape in crossSessionKey |
serializes load+resume; snapshots effective page; does-not-reuse-superseded |
| M2 provider | intent.resultSuperseded === true commit guard |
does-not-reuse-superseded |
| M3 provider | latest.lifecycle !== intent.lifecycle commit guard |
does-not-reuse-across-lifecycle-cancellation |
| M4 bridge | historyPageSize !== inFlight.historyPageSize |
both mismatched-page rejects |
| M5 bridge | page-size validation throw | all 3 invalid-page rejects |
| M6 bridge (positive control, pre-PR guard) | historyReplay !== inFlight.historyReplay |
pre-existing "incompatible replay modes" test |
No survivors: every guard the PR introduces is pinned by its own test and kills nothing else. Sibling sweep of the coalesce key: clientId (per-caller attachments, asserted distinct), approvalMode (applied per-caller at attach, not replay content), and lineage fields (parentSessionId/sourceType/sourceId, identical for one session) are correctly excluded; no shape-relevant field is missing.
Findings
- (Nit, non-blocking) The provider test name "coalesces transactional loads only when their replay shapes match" overpromises: its fixture issues two identical loads (same page) and asserts one SDK call — the positive half only. The negative half ("only when") is pinned by the sibling tests above (M1/M2 rows), so coverage is complete by composition; only the name misleads a reader skimming the suite. No action required; a rename would suffice.
Not covered
- Per-commit attribution: the PR's first commit
223be6dcais unreachable at the depth-2 merge checkout (git rev-list HEAD^1..HEAD^2returns 1 against the snapshot's 2 commits). Verified the aggregateHEAD^1..HEADdiff; per-commit split (serve-side vs webui-side) not individually exercised. - Integration base arm: the real-daemon scenario ran only at head; building a base CLI dist for the control arm was out of budget. The unit-level A/B covers the same claim at both boundaries.
- Repo-wide gates: other packages' suites and repo-level lint/format were not run; both affected workspaces are fully green and typechecked.
- Keepalive scheduler: the 409-on-mismatch behavior is proven at the bridge level (
RestoreInProgressError); the scheduled keepalive path itself was not driven end-to-end. - Platforms: Linux container only (author tested macOS).
Methodology
Environment: node:22 Linux container, refs/pull/8933/merge at depth 2, npm ci + npm run build pre-run at head. A/B: git worktree add tmp/base-tree HEAD^1, head test files copied in, vitest run per package from the base worktree (webui needed its package-local node_modules symlinked — third-party only, zero symlinks inside). Mutation matrix: harness/mutation-matrix.sh (unique-anchor node replacements, targeted vitest, git checkout -- restore, tree verified clean). Assertions: harness/verify-assertions.mjs parses the raw logs in logs/ and runs live typecheck/control checks — 25/25. Evidence PNGs via scripts/verify-capture.mjs. Harnesses and logs are in this artifact dir for rerun.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
Collaborator
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
Collaborator
Author
|
@qwen-code /triage |
Collaborator
|
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: 29 passed · 0 failed · 29 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：29 通过 · 0 失败 · 29 总计 Verification report<!-- qwen-triage:verify --> PR 8933 — fix(serve): Keep restore request shapes distinct (follow-up round)Verdict: 中文 — 判定：✅ 通过 · 可合入（agent 判定）· 复核轮
Previous-finding status table (follow-up round)
No new findings this round. Central claim + A/B (re-measured live this round)Central claim: concurrent cold restores only coalesce when their effective replay shapes match ( Method: head's test files run against base source in a Witnesses: Bridge boundary (
|
| Scenario (same file both arms) | base (86a474ba) |
head (d47eba4f) |
|---|---|---|
| rejects invalid page 0 / 501 / 1.5 before restore (×3) | red — promise resolved ("instead of rejecting"; no validation, agent call issued) | green |
| rejects coalescing with different explicit page / omitted page (×2) | red — second caller coalesced instead of RestoreInProgressError |
green |
| same-page coalescing control (1 ACP call, independent attachments) | green | green |
| streamed loads / resumes ignore page (×2 controls) | green | green |
| different inherited-history policy rejected (control) | green | green |
| rest of suite | 548/553 pass | 553/553 pass |
WebUI boundary (packages/webui/.../DaemonSessionProvider.tsx)
| Scenario | base | head |
|---|---|---|
| retries a switch using the page captured at intent time (strengthened pre-existing) | red — expected { …(3) } to match object { historyPageSize: 100 } (base sent the changed configured page 500) |
green |
| serializes load and resume for same target | red (base coalesced; stall) | green |
| snapshots effective page for queued load | red (stall) | green |
| does not reuse superseded result when latest shape matches it | red (stall) | green |
| does not reuse raw result across lifecycle cancellation | red — expected 1 to be 2 (base adopted the fenced result) |
green |
| standalone resume / same-shape coalescing / no-pagination normalization (×3 controls) | green | green |
Unhandled AbortError: Session runner restarted rejections: base = 3, head = 0 — reproduced exactly; the fence turns those silent leaks into explicit terminal states.
Head gates (live this round)
| Gate | Result |
|---|---|
acp-bridge bridge.test.ts |
553/553 |
webui DaemonSessionProvider.test.tsx |
237/237 |
| acp-bridge full workspace | 26 files, 1233/1233 |
| webui full workspace | 34 files, 521/521 |
| typecheck (both packages) | exit 0 |
real-daemon integration (qwen-serve-webui-session-switching, mock ACP child, real HTTP) |
3/3, incl. serializes non-equivalent load and resume requests for one target |
Mutation matrix (6/6 re-measured live; each mutant applied to HEAD then restored; 04-mutation-matrix.png)
| Mutant | Guard removed | Killed (exactly) |
|---|---|---|
| M1 provider | shape in crossSessionKey return |
serializes load+resume; snapshots effective page (2 red, 235 skipped) |
| M2 provider | intent.resultSuperseded === true commit guard |
does-not-reuse-superseded (1 red) |
| M3 provider | latest.lifecycle !== intent.lifecycle commit guard |
does-not-reuse-across-lifecycle-cancellation (1 red) |
| M4 bridge | historyPageSize !== inFlight.historyPageSize |
both mismatched-page rejects (2 red) |
| M5 bridge | page-size validation throw | all 3 invalid-page rejects (3 red) |
| M6 bridge (positive control, pre-PR guard) | historyReplay !== inFlight.historyReplay |
pre-existing "rejects coalescing load requests with incompatible replay modes" (1 red) |
No survivors: every guard the PR introduces is pinned by exactly its own test(s) and kills nothing else. Post-run worktree clean (all mutants restored, verified by git status --porcelain).
Findings
- (Nit, non-blocking, carried from previous round — stands) The provider test name "coalesces transactional loads only when their replay shapes match" overpromises: its fixture issues two identical loads (same page) and asserts one SDK call — the positive half only. The negative half ("only when") is pinned by the sibling tests above (M1/M2 rows re-verified this round), so coverage is complete by composition; only the name misleads a reader skimming the suite. No action required; a rename would suffice.
Not covered
- Per-commit attribution: the PR's first commit
223be6dcais unreachable at the depth-2 merge checkout (git rev-list HEAD^1..HEAD^2returns 1 against the snapshot's 2 commits). Verified the aggregateHEAD^1..HEADdiff; per-commit split not individually exercised. - Integration base arm: the real-daemon scenario ran only at head; building a base CLI dist for the control arm was out of budget. The unit-level A/B covers the same claim at both boundaries.
- Repo-wide gates: other packages' suites and repo-level lint/format were not run; both affected workspaces are fully green and typechecked.
- Keepalive scheduler: the 409-on-mismatch behavior is proven at the bridge level (
RestoreInProgressError); the scheduled keepalive path itself was not driven end-to-end. - Coalesce-key sibling sweep (clientId / approvalMode / lineage fields correctly excluded): carried forward from the previous round under the proven-identical input closure (A1–A4); not re-derived this round.
- Platforms: Linux container only (author tested macOS).
Methodology
Environment: node:22 Linux container, refs/pull/8933/merge at depth 2, npm ci + npm run build pre-run at head. This is a follow-up round whose head and base OIDs are identical to the previously verified round, so the merged working tree (and therefore every source file, lockfile, config, and fixture consumed by prior measurements) is byte-identical by git content addressing — cited per the input-closure shortcut, and compared above (A1–A4). All core measurements were still re-executed live: A/B via git worktree add tmp/base-tree HEAD^1 with head test files copied in and vitest run per package from the base worktree; mutation matrix via harness/mutation-matrix.mjs (unique-anchor line removals, targeted -t vitest runs, byte-exact restore); integration via QWEN_SANDBOX=false npx vitest run cli/qwen-serve-webui-session-switching.test.ts against packages/cli/dist/index.js (real daemon, real HTTP, mock ACP child). Assertions: harness/verify-assertions.mjs parses the raw logs in logs/ — 29/29. Transparency note: the first two mutant runs of this round died at vitest CLI parsing (a harness bug passing multiple -t values); those runs were discarded, the harness fixed, and the mutants re-run — no PR-side measurement was affected. Evidence PNGs via scripts/verify-capture.mjs. Harnesses and logs are in this artifact dir for rerun.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
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. ✅
This was referenced Aug 11, 2026
wenshao
added a commit
that referenced
this pull request
Aug 12, 2026
…8905) The merge of main pulled in #8933, which gates historyPageSize on historyReplay === 'response'. The 'transport failure marks the channel dying before process exit' test (from #8947) passes historyPageSize with the default stream replay, so the paged transcript fetch it waits on is never issued and the test times out — a cross-PR interaction between two main commits, failing deterministically on main. Pin the response replay mode the paged fetch requires.
BenGuanRan
pushed a commit
to BenGuanRan/qwen-code
that referenced
this pull request
Aug 12, 2026
…re test (QwenLM#8984) 'transport failure marks the channel dying before process exit' (added in QwenLM#8947) fails deterministically on merged main: it calls loadSession with historyPageSize but without historyReplay, and since QwenLM#8933 historyPageSize only takes effect when historyReplay is 'response' — the request silently degrades to stream replay, no sessionTranscript page is ever requested, and the extMethodCalls vi.waitFor times out. This was a logical merge conflict, not a flake: QwenLM#8947 branched before QwenLM#8933 landed, so both PRs were green on their own branches and red only once combined. Passing historyReplay: 'response' restores the intended path — the transcript page request hangs, the transport failure rejects it, and the second assertAttachableSessionEntry maps the torn-down session to SessionNotFoundError exactly as the test asserts. Co-authored-by: verify <verify@local>
Collaborator
|
Released in v0.21.11. |
pull Bot
pushed a commit
to edisplay/qwen-code
that referenced
this pull request
Aug 13, 2026
…turn replay (QwenLM#8905) * feat(serve): adaptively grow live-journal caps before truncating mid-turn replay A single turn fanning out many concurrent subagents (e.g. a /review run) can emit hundreds of thousands of source events, far past the per-session live-journal baseline caps (10 000 entries / 8 MiB), so a mid-turn (re)load silently shows a truncated replay until the turn finishes. Before evicting, the engine now asks a growth advisor: caps double (entries scaled proportionally) while the growth granted across the bridge's live sessions fits in a pool derived from the daemon memory budget (5%, clamped to [32, 1024] MB), never past a per-session hard cap of 256 MiB. Growth is on demand, throttled after a refusal, and accounted statelessly from the current caps of all live sessions, so granted headroom dies with its session. An operator-pinned --max-journal-events/--max-journal-bytes disables growth; without a pool the fixed-cap eviction behavior is unchanged. * fix(serve): address adaptive live-journal growth review feedback (QwenLM#8905) * fix(serve): account in-flight restores in the journal growth pool (QwenLM#8905) Concurrent restores hold their buses in pendingRestoreEvents rather than byId, so each advisor ask only saw its own caps and concurrent restores could each draw a full doubling from the same pool. Sum the current caps of every in-flight restore bus into allSessionLimitBytes. Also skip the growth ask when the breaching append is a turn boundary — compactCurrentTurn discards the journal immediately afterwards, so the grant would be charged to the pool while buying zero eviction. Pin the previously untested contracts with tests: restore-window accounting, concurrent-restore accounting, headroom release on session close, the hard-cap clamp term, partial-grant eviction, requester discrimination in the policy fixtures, the maxEvents safe-integer conjunct, and the dynamic-workspace bridge pool wiring. Fix the docs: add the missing journal-flag rows to the daemon configuration and operations pages, and correct the effective-budget definition. * test(serve): request 'response' replay in the transport-failure test (QwenLM#8905) The merge of main pulled in QwenLM#8933, which gates historyPageSize on historyReplay === 'response'. The 'transport failure marks the channel dying before process exit' test (from QwenLM#8947) passes historyPageSize with the default stream replay, so the paged transcript fetch it waits on is never issued and the test times out — a cross-PR interaction between two main commits, failing deterministically on main. Pin the response replay mode the paged fetch requires. * fix(serve): share one daemon-wide journal growth pool (QwenLM#8905) Address the automated review of adaptive live-journal growth: - The growth pool is now one daemon-wide aggregate shared by every workspace bridge instead of a full pool per bridge, and growth is disabled when the budget is insufficient or leaves no headroom after the root reserve. - Grants that cannot retain any additional journal entries (an oversized event survives as the sole entry either way) are refused so the pool is never charged for growth that preserves no replay. - The refusal throttle defaults to a monotonic clock and treats a backward clock jump as an elapsed window. - The proportional event hard cap is clamped to MAX_SAFE_INTEGER so a valid-but-extreme baseline cannot poison every grant. - /daemon/status reports the growth semantics: limits.memory.journalGrowth (pool size, hard cap, baselines), per-session effective caps in full diagnostics, and enforced:false scoped to the child-heap model. - Validation-boundary tests for the growth-pool normalizer and doc fixes (positive safe integer types; growth toward double, limited by pool headroom). * fix(serve): align growth-pool docs and harden growth tests (QwenLM#8905) * fix(serve): account growth per session baseline and walk intermediate grants (QwenLM#8905) * fix(serve): harden growth-pool tests and derive help figures from constants (QwenLM#8905) * fix(serve): reject valueless journal cap flags and harden growth tests (QwenLM#8905) --------- Co-authored-by: qwen-code-ci-bot <qwen-code-ci@service.alibaba.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>
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 makes restore request coalescing shape-aware at both coordinator boundaries. The WebUI now treats the normalized target plus
resume/none,load/all, orload/recent(N)as the request identity, snapshots the effective page when the intent is admitted, and serializes non-equivalent requests without allowing a superseded raw result to commit. Explicit lifecycle cancellation also fences an older same-shape raw result, while an ordinary same-lifecycle timeout retry may still adopt it. The ACP bridge now validates response page sizes before restore admission, includes the effective page in cold-restore coalescing, and ignores page values that have no meaning for streamed loads or resumes.The change preserves exact-shape coalescing, same-shape retries after a timeout, transactional source-session availability, per-caller attachment registration, approval-mode handling, and existing restore error types. It also updates the transactional switching design and adds focused unit and real-daemon regression coverage.
Why it's needed
After #8882, concurrent restore intents for the same session and workspace could still be treated as equivalent even when they requested different replay semantics. A pending
loadcould satisfy a laterresume, or arecent(100)request could share the result ofrecent(500)or unpagedall, returning replay selected for another caller. This is a correctness prerequisite for the selective restore design in #8743.Reviewer Test Plan
How to verify
restore_in_progressconflict without changing the first request's attachment reservation.load, requestresumefor the same target, and confirm the load caller receivesAbortError, the source remains usable, no resume starts until the raw load settles, the stale attachment is released once, and the resume result commits.load/alland still coalesce.recent(100) → recent(500) → recent(100)and confirm the first result remains superseded rather than being adopted by the last intent. Restart the provider lifecycle and confirm an older same-shape raw result is fenced, while an ordinary timeout retry that stays in the same lifecycle may still adopt its late result.Evidence (Before & After)
Before: the existing WebUI unit contract explicitly coalesced
loadSession()andresumeSession()for one target, so a held/loadfollowed byresumeSession()sent no/resumerequest and resolved through the load result. The bridge in-flight key likewise omitted the effective response page.After: 237 WebUI tests, 553 ACP bridge tests, and all 3 focused real-daemon switching scenarios pass. The real-daemon regression observes one held
/load, zero premature/resumerequests, continued source-session events, one cleanup of the stale response's attachment, one subsequent/resume, and a final target commit with a live client attachment.Tested on
Environment (optional)
macOS 26.4.1, Node.js 22.22.3, npm 10.9.8. Unit tests ran with package-specific Vitest configs; the real-daemon test ran with
QWEN_SANDBOX=false. Prettier, focused ESLint, the repository build, and the repository typecheck passed.Risk & Scope
response/allrestore racing a WebUIrecent(N)cold restore now receives the existing HTTP 409restore_in_progressresponse and should honor its existing Retry-After guidance instead of incorrectly sharing another request's replay.Linked Issues
Refs #8678
Design prerequisite for #8743.
中文说明
本 PR 做了什么
本 PR 在两个协调边界上让恢复请求合并感知完整请求形状。WebUI 现在将规范化后的目标与
resume/none、load/all或load/recent(N)一起作为请求标识，在 intent 接纳时快照有效 page，并串行执行非等价请求，且不允许已被替代的原始结果提交。显式 lifecycle cancellation 也会 fence 更早的同形状 raw result，而同一 lifecycle 内的普通 timeout retry 仍可采用它。ACP bridge 现在会在恢复准入前验证 response page size，将有效 page 纳入冷恢复合并，并忽略对 streamed load 或 resume 没有语义的 page 值。该变更保留了完全相同形状的合并、超时后同形状重试、事务切换期间 source session 的可用性、每个调用方独立的 attachment 注册、approval mode 处理以及现有恢复错误类型。同时更新了 transactional switching 设计，并增加了聚焦的单元测试和真实 daemon 回归覆盖。
为什么需要
#8882 合入后，同一 session 与 workspace 的并发恢复 intent 即使请求了不同 replay 语义，仍可能被视为等价。pending
load可能满足后续resume，recent(100)也可能共享recent(500)或未分页all的结果，从而返回为另一个调用方选择的 replay。本 PR 是 #8743 selective restore 设计的 correctness 前置条件。Reviewer 测试计划
如何验证
restore_in_progress冲突，且不会改变第一个请求的 attachment reservation。load，再对同一目标请求resume，确认 load 调用方收到AbortError、source 保持可用、原始 load settle 前不会启动 resume、stale attachment 只释放一次，最后提交 resume 结果。load/all并继续合并。recent(100) → recent(500) → recent(100)，确认第一个结果始终保持 superseded，不会被最后一个 intent 重新采纳。重启 provider lifecycle，确认更早的同形状 raw result 被 fence；同一 lifecycle 内的普通 timeout retry 则仍可采用其 late result。证据（修复前后）
修复前：现有 WebUI 单元测试契约明确将同一目标的
loadSession()与resumeSession()合并，因此暂停的/load后调用resumeSession()不会发送/resume，而是通过 load 结果完成。Bridge 的 in-flight key 同样遗漏了有效 response page。修复后：237 个 WebUI 测试、553 个 ACP bridge 测试和全部 3 个聚焦 real-daemon switching 场景通过。Real-daemon 回归观察到一个被暂停的
/load、零个提前/resume请求、source session 事件持续传递、stale response attachment 被清理一次、随后发送一个/resume，并最终以存活的 client attachment 提交目标 session。测试平台
环境（可选）
macOS 26.4.1、Node.js 22.22.3、npm 10.9.8。单元测试使用各 package 的 Vitest 配置；real-daemon 测试使用
QWEN_SANDBOX=false。Prettier、聚焦 ESLint、仓库 build 和仓库 typecheck 均通过。风险与范围
response/all恢复与 WebUIrecent(N)冷恢复竞争时，现在会收到现有 HTTP 409restore_in_progress响应，并应遵循现有 Retry-After 指引，而不是错误共享另一个请求的 replay。关联 Issue
Refs #8678
#8743 的设计前置条件。