fix(webui): Close same-session refresh race gaps - #8990
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
E2E report: |
|
Thanks for the PR!
Moving on to code review. 🔍 中文说明感谢贡献！
进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewMy independent proposal for the five gaps — snapshot and return the epoch from bounded replay loads, centralize terminal candidate/capture cleanup, decouple the observed-turn latch from local shell activity, hold-and-retry controlled targets instead of failing them, and gate the prop-mirror write to legacy daemons — is exactly what this PR does, gap for gap. Re-tracing every fix against the head code on this re-run confirms the tricky parts:
No critical blockers found, and no convention violations — the diff is net-negative in production complexity (160 production lines vs 314 test lines), tests are colocated, and the design doc moves in lockstep with the semantics. CI test evidenceCI on the reviewed commit has settled. Every suite is green except one check that failed on runner disk exhaustion, unrelated to this PR — the Ubuntu unit job's only failures are 38 tests inside Classifying this as pre-existing infrastructure noise, with evidence: (1) the error is literally
Not verified by me directly: this is an unattended CI run, so I do not build or run PR code here — the evidence above is the PR's own CI, quoted from the API. The local integration-suite pass reported in the thread is the author's claim (macOS, Node v22.22.3); the Real daemon E2E check covers the same path in CI and is green. Sandboxed verification would settle the remaining gap: 中文说明代码审查我对这五个缺口的独立方案——有界 replay load 快照并返回 epoch、把终止路径的候选/capture 清理集中化、把观察到的 turn 标志与本地 shell 活动解耦、受控目标挂起重试而不是直接失败、prop 镜像写入仅限 legacy daemon——与本 PR 的做法逐条一致。本轮重新在 head 代码上逐条核对，确认了关键细节：
未发现关键阻塞问题，也无规范违规——生产代码净复杂度下降（160 行生产逻辑、314 行测试），测试与源码同目录，设计文档与语义同步。 CI 测试证据被审 commit 的 CI 已结束。除一项与 PR 无关的 runner 磁盘耗尽失败外全部为绿：Ubuntu 单测 job 的唯一失败是 判定为既有基础设施噪音，证据：（1）报错是宿主机 我未直接验证：无人值守 CI 环境不构建、不运行 PR 代码，以上证据为通过 API 引用的该 PR 自身 CI 结果。线程中报告的本地集成套件通过是作者自述（macOS、Node v22.22.3）；CI 的 Real daemon E2E 覆盖同一路径且为绿。 沙箱验证可以补齐剩余缺口： — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean, tightly scoped hardening that matches what I would have written, with dense regression coverage; the single red CI check is proven runner disk exhaustion, not this PR. Stepping back: this is what a good follow-up PR looks like. Five concrete gaps, five targeted fixes, a dedicated regression test per gap, and the design doc updated in lockstep — and the change removes duplication (four inline cleanup blocks → one helper) rather than adding machinery. On this re-run I re-traced each fix end-to-end against the head code: epoch snapshot consistency in the bridge, single-retire semantics on every terminal path, decoupled turn/shell gating for the refresh pump (including the On the red check: the earlier run deferred approval until CI landed green, and the one failure that blocked it is environmental — 38 tests in an untouched CLI review-manifest suite, all dying in Standing reservations, stated plainly: this is a complex state machine, and static review cannot exclude interleavings beyond the ones under test — but coverage is dense (+314 test lines pinning each race) and the held-response replay-plus-tail path runs green in CI. The author tested locally on macOS only; the OS matrix and CLI integration suites pick this up in the merge queue, which is this repo's design for those checks. The Approving now, pinned to the reviewed commit. 中文说明置信度：4/5 —— 干净、范围收敛的加固，与我会写出的方案一致，回归覆盖密集；唯一的红色 CI 检查已证实是 runner 磁盘耗尽，与本 PR 无关。 整体来看：这是一个高质量后续 PR 的样子。五个具体缺口、五处定向修复、每个缺口一个专门的回归测试，设计文档同步更新——而且改动是减少重复（四处内联清理块收敛为一个辅助函数），不是堆叠机制。本轮重新逐条在 head 代码上追到底：bridge 中 epoch 快照的一致性、所有终止路径的恰好一次退役、刷新泵 turn/shell 门控解耦（包括 shell 结束后 关于红色检查：上一轮把批准推迟到 CI 全绿，而挡住它的那个失败是环境问题——一个本 PR 未触及的 CLI review-manifest 套件中 38 个用例全部死于 坦率说明保留意见：这是一个复杂状态机，静态审查无法排除测试覆盖之外的交错——但覆盖密集（+314 行测试钉住每个竞态），held-response 的 replay+tail 合并路径在 CI 中为绿。作者仅在 macOS 本地测试；OS 矩阵与 CLI 集成套件按本仓库设计在 merge queue 阶段运行。审查评论中的 现在批准，绑定被审 commit。 — Qwen Code · qwen3.8-max Reviewed at |
🖼️ 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 |
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. |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Reviewed the race-gap closures against the merged #8939 coordinator design; all gaps are real and the fixes hold. Approving.
Verified fixes
runnerActiveTurnlatch: previously recomputed fromhasSessionActivePrompt()(which includes the:shellentry) at turn-terminal/stream-start, so a shell running when a remote turn completed latchedactiveTurntrue with nothing to clear it — a queued same-session refresh never started. NowrunnerActiveTurntracks only the runner event-stream turn lifecycle; orthogonal shell/prompt gating stays on the livehasCurrentSessionActivePromptRef()check in the pump/commit gates (3631/3838), and shell completion pumps viasetPromptStatus('idle'). Fail-closed direction holds; new test pins observed-turn + shell ordering.- Controlled target during source-bound ops: replaced the terminal failed-transition with pending + retry-on-settle (
controlledRetryPendingRef+ nonce re-run). Latest-target-wins, re-blocks if a new source-bound op starts before the microtask, mount-guarded, and the counter decrements on both settle paths of the raw op (create/branch.then(dec, dec)), so the retry fires exactly when the final raw op settles even after an outer action timeout. - Capture lifecycle unification:
cleanupTransitionArtifactswithpreserveInFlightCapture: rawTransitionRef.current === intentapplied consistently across deadline, pre-restore deadline, cancel, abort, supersede, and unmount paths. With a raw restore in flight the capture stays attached until.finally()releases it, so an exact-shape retry (adoptingRaw, same key) adopts the in-flight result with a gap-free tail; unadopted captures are released in.finally()— no lingering event capture. Removing the explicitretireAttachmentcalls is safe becauseexposeCrossSessionFailureis guarded bydesiredTransitionRef.current === intentand the pump reaches those checks synchronously, so exactly one path retires. New test pins retire-once + post-failure capture release. - Unmount leak: unmount now routes through
cancelTransitionRef, retiring the candidate and releasing the capture (previously only rejected the intent). - Committed clientId as recovery identity: the render-time prop mirror is now restricted to legacy daemons (no
client_identityfeature), so a committed rebind survives re-renders and ring/epoch recovery resumes with the attachment that owns the runner. No pre-capabilities gap — the ref is initialized to the same stable value. - Bridge bounded-load epoch:
refreshedReplayFieldsForsnapshots epoch + watermark together and requires both (plus same entry, no active prompt) unchanged through the persisted-page read; fail-closed fallback retries once then uses the in-memory replay. Bounded responses now carryeventEpochlike full responses, so the provider's same-epoch commit CAS works withhistoryPageSize(previouslycandidate.eventEpoch === undefinedfailed the completeness check). Pinned in bridge.test.ts and the integration test.
CI note: Test (ubuntu-latest, Node 22.x) is red from ENOSPC: no space left on device in packages/cli/src/commands/review/lib/manifest-repository-context.test.ts (38/60) — runner disk exhaustion in a file this PR doesn't touch; unrelated infra. This PR's files are green: bridge.test.ts 568 ✓, DaemonSessionProvider.test.tsx 265 ✓.
Nit (P3, non-blocking): legacyClientIdDependency still computes initialClientIdDependencyRef.current for modern daemons where it is now unused — harmless, could be simplified later.
|
@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: 1670 passed · 0 failed · 1670 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：1670 通过 · 0 失败 · 1670 总计 Verification reportPR 8990 — fix(webui): Close same-session refresh race gapsVerdict: 中文摘要
Central claim and A/BThe PR exists to close five deterministic race gaps left by #8939. The central one: the default bounded-history same-session refresh could never pass the provider's epoch integrity check, because the bridge's bounded (persisted-page) load response omitted Method: the HEAD test files were run against a
Five of five gap cells flip broken→fixed; zero collateral failures on either side. Mechanism notes verified in source: #3's base deadlock is a stale Reviewer Test Plan walkthrough
All six steps are executable and pass at head; no step is structurally unreachable. Vacuity and mutation matrixRunning the HEAD test files against base source is the revert-based vacuity check: all four new/changed provider tests go red on the unfixed source at their intended behavioral assertion (three clean expected-vs-actual diffs; the liveness one as a 5 s hang, the honest shape of "queued forever"), and the bridge test red on the missing
M1 is a coverage gap, not dead code: the clause guards a daemon restart (bus rebuild, new epoch) landing inside a bounded page read; without it the bridge would return a page snapshotted against a dead epoch and the provider would commit a replay anchored to it. The fallback (retry once, then FindingsF1 — Suggestion (non-blocking): the bridge epoch freshness clause is unpinned. Deleting Suggested fixture (sketch, unmeasured — ship together with the test)In a sibling of Not covered
MethodologyEnvironment: CI verify container ( Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
|
Released in v0.21.11. |
3 participants
What this PR does
This follow-up hardens transactional same-session refresh after #8939. It returns a stable event epoch from bounded replay loads, releases prepared candidates and source-tail captures on every terminal failure path, resumes controlled targets after source-bound operations settle, unblocks queued refreshes after both an observed turn and a shell command finish, and preserves the committed modern client identity for later recovery.
Why it's needed
Post-merge review found five deterministic race gaps: the default bounded-history refresh could fail its epoch integrity check, an event-driven timeout could leave a provisional attachment and capture alive, a turn terminal followed by shell completion could leave refresh queued forever, a controlled target could be silently dropped during create or branch work, and a committed client-ID rebind could be overwritten by the initial prop before a later resync. These fixes preserve the current session while closing those failure and liveness gaps.
Reviewer Test Plan
How to verify
Verify that a bounded same-session load includes the live event epoch and commits successfully; that a prepared candidate waiting for source catch-up is detached and its capture stops after an event-driven deadline; that refresh starts only after both an observed turn and an overlapping shell command finish; that a controlled target requested during session creation opens after creation settles; and that a committed client-ID rebind is used by a later ring-resync recovery request. The real-daemon integration should also complete the held-response replay-plus-live-tail path without loss or duplication.
Evidence (Before & After)
N/A — this is state-machine and daemon-response correctness hardening with no visual UI change.
Tested on
Environment (optional)
macOS, Node.js v22.22.3, no sandbox for the focused real-daemon integration.
Risk & Scope
Linked Issues
Refs #8678
Follow-up to #8939.
中文说明
本 PR 做了什么
这是 #8939 之后对同会话事务刷新进行加固的后续修复。它让有界 replay load 返回稳定的事件 epoch，在所有终止失败路径中释放已准备的候选 attachment 和 source-tail capture，在 source-bound 操作结束后继续处理受控目标，在观察到的 turn 与 shell 命令都结束后推进排队的刷新，并确保后续恢复继续使用现代事务提交后的 client identity。
为什么需要
合入后的 review 发现了五个可确定复现的竞态缺口：默认有界历史刷新可能无法通过 epoch 完整性校验；事件驱动的超时可能遗留 provisional attachment 和 capture；turn 结束后 shell 再结束可能让刷新永久停在 queued；create 或 branch 工作期间的受控目标可能被静默丢弃；已经提交的 client-ID rebind 可能在后续 resync 前被初始 prop 覆盖。这些修复在保留当前会话的同时关闭上述失败与活性缺口。
Reviewer 测试计划
如何验证
验证有界同会话 load 会携带 live event epoch 并成功提交；等待 source 追赶的 prepared candidate 在事件驱动 deadline 后会被 detach，且 capture 停止继续收集；刷新只会在观察到的 turn 和重叠 shell 命令都结束后启动；会话创建期间请求的受控目标会在创建结束后打开；已经提交的 client-ID rebind 会用于后续 ring-resync 恢复请求。真实 daemon integration 还应完成 held-response 的 replay 与 live tail 合并路径，且不丢失、不重复事件。
证据（Before & After）
N/A —— 这是状态机与 daemon 响应正确性加固，没有可视 UI 变化。
测试平台
环境（可选）
macOS、Node.js v22.22.3；focused 真实 daemon integration 使用 no-sandbox。
风险与范围
关联 Issue
Refs #8678
#8939 的后续修复。