test(webui): drain the batched transcript dispatch with two timer hops - #9058
Conversation
|
⏳ Approval still deferred — 1 PR CI workflow run(s) still in progress for ⏳ 审批仍在延迟中 —— |
|
Thanks for the PR! Template looks good ✓ Problem: observed, not theoretical. The linked job (Quality Checks, job 94340231638 in Release run 31665796236) genuinely failed on Direction: aligned. A flaky test failing Release Quality Checks on Size: not applicable — one test file, +9/-0 (one code line + explanatory comment); no core paths touched ( Approach: scope is right and minimal. I verified the mechanics against the code: Risk: no elevated risk signals — no files match the revert-correlated high-risk paths, and the change cannot weaken any assertion (the extra hop only drains more). Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 问题:已观测到,不是理论问题。关联的 job(Release run 31665796236 中的 Quality Checks,job 94340231638)确实在 方向:对齐。flaky 测试在 规模:不适用——单个测试文件,+9/-0(一行代码 + 解释性注释);未触及核心路径( 方案:范围正确且最小。已对照代码核实机制: 风险:无升级风险信号——没有文件命中与 revert 相关的高风险路径,且该改动不可能弱化任何断言(多一跳只会排空得更彻底)。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewMy independent take on this flake before reading the diff: the dispatch is intentionally batched onto What I verified against the code:
No blockers. The only nit was already raised in Stage 1 ("10 call sites" vs the 9 I count). Test evidence — the PR's own CI on the reviewed commitThe unit suite that matters here ( CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 The loop numbers in the PR body (6/60 failing before, 0/100 after, on Node 24 and Node 22) are the author's own local runs, not independently re-run here — this run is unattended CI and never executes PR code. Sandboxed verification would settle this: Real-scenario testing: N/A — test-only change with nothing user-visible (and this unattended run does not drive the product regardless). 中文说明代码审查:在看 diff 之前,我对这个 flake 的独立判断是——派发被有意批量挂在 已对照代码核实: 测试证据:关键的单元测试( 真实场景测试:不适用——纯测试改动,无用户可见行为(且无人值守运行本身不驱动产品)。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 5/5 — clean at every stage: an observed flake with a verified root cause, and a minimal fix that matches what I'd have proposed independently. The problem is real — I checked the linked job record, and it failed on The one thing not independently confirmed is the 0/100 loop number — that's the author's local evidence, and Stage 2 names the lane ( Approval deferred until CI lands green on 中文说明置信度:5/5 —— 每个阶段都干净:已观测到的 flake、已核实的根因,以及一个与我独立设想完全一致的最小修复。 问题是真实的——我核查了关联 job 的记录,它今天确实在 唯一未独立确认的是 0/100 的循环数字——那是作者的本地证据,Stage 2 已给出可将其独立化的通道( 批准推迟到 CI 在 — 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 changes against the PR base. 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. |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
中文说明
未发现问题。LGTM!✅
— qwen3.8-max via Qwen Code /review (v0.21.11)
yiliang114
left a comment
There was a problem hiding this comment.
LGTM — correct structural fix for the known DaemonSessionProvider flake (the one that red-CI'd unrelated PRs, e.g. #9007).
- The ordering argument holds: the mock SSE consumer chain is microtask-driven up to enqueueTranscriptEvents, so the dispatch timer is always registered during the microtask drain preceding the first hop's callback; the second hop therefore fires after the dispatch regardless of registration order between the two chains.
- Empirical backing is solid (≈10% baseline failure → 0/100 clean; reproduced on both Node 22 and 24 pre-fix, ruling out the Node bump).
- All 10 call sites share this one helper, so the strengthened drain covers every user; absence assertions only get stricter. Test-only, provider batching untouched.
- CI on head green.
What this PR does
Fixes the flaky
DaemonSessionProvidertestkeeps the current attachment live while a same-session load fails, which failed the Release Quality Checks onmain(run 31665796236) withexpected [ 'A transcript' ] to deeply equal [ 'A transcript', ' still live' ]. The test helperflushTranscriptDispatchnow awaits two chainedsetTimeout(0)hops instead of one before test assertions read the transcript.Why it's needed
The provider batches transcript events onto a single
setTimeout(0)macrotask. The helper also waited exactly onesetTimeout(0)hop. Those two timers are registered from two concurrently-draining microtask chains — the event-consumer chain (mock generator resume →for await→enqueueTranscriptEvents) and the test chain (flushPromises()→flushTranscriptDispatch()) — so their registration order is unspecified. InstrumentingsetTimeoutin the failing runs shows the helper's timer registering before the dispatch timer; since Node drains the microtask queue between timer callbacks, the helper's single hop resumed the test and ran the assertion before the dispatch timer ever fired, so the streamed' still live'block was missing. Whether a given run passed depended on whetherawait act(...)'s exit path happened to cross one more macrotask (letting the dispatch timer slip in), which is why the failure was intermittent (~10% locally) rather than deterministic.Two chained hops close the window structurally: the dispatch timer is always registered during the microtask drain that precedes the first hop's callback, so the second hop — registered after that callback — is guaranteed to fire after the dispatch has run, regardless of which chain registered its timer first. This is a test-only change; the provider's macrotask batching is intentional (it coalesces event bursts into one reducer pass) and is untouched.
Reviewer Test Plan
How to verify
Repro is a plain repetition loop in
packages/webui(no load or stress needed); the fix is judged by the same loop going clean. Baseline vs fixed on the same machine, same code, with the 9-line helper change as the only variable:vitest run … -t 'keeps the current attachment live'loopDaemonSessionProvider.test.tsx, Node 24.18.1DaemonSessionProvider.test.tsx, Node 22.23.2 (CI's version)Every baseline failure is the exact CI assertion (
expected [ 'A transcript' ] to deeply equal [ 'A transcript', ' still live' ]). The Node 22 leg rules out the Node22.23.1 → 22.23.2bump between the passing and failing Release runs as the cause — both versions reproduce the flake before the fix.All 10 existing
flushTranscriptDispatch()call sites in the file want "drain the batched dispatch", so strengthening the drain is safe for all of them — including the two absence assertions (not.toContain('stale output'),not.toEqual(...)), which the extra hop only makes stricter. The full-file loops above cover them.Evidence (Before & After)
N/A — test-timing change; the before/after failure rates above are the evidence.
Tested on
Environment (optional)
Unit tests only:
vitest runinpackages/webui, Node 24.18.1 and Node 22.23.2 (the version the failing CI job used).Risk & Scope
Linked Issues
The flaky test was introduced with the same-session refresh coverage in #8939 (and extended in #8990); the failing run was Release run 31665796236 on
main.中文说明
本 PR 做了什么
修复
DaemonSessionProvider的 flaky 测试keeps the current attachment live while a same-session load fails。它在main的 Release Quality Checks(run 31665796236)中以expected [ 'A transcript' ] to deeply equal [ 'A transcript', ' still live' ]失败。测试助手flushTranscriptDispatch现在在断言读取 transcript 之前等待两跳链式setTimeout(0),而不是一跳。为什么需要
provider 把 transcript 事件批量派发挂在单个
setTimeout(0)宏任务上,而助手也只等恰好一跳setTimeout(0)。这两个计时器分别从两条并行清空的微任务链注册——事件消费链(mock 生成器恢复 →for await→enqueueTranscriptEvents)和测试链(flushPromises()→flushTranscriptDispatch())——注册顺序不受保证。对失败运行插桩setTimeout显示助手的计时器先于派发计时器注册;由于 Node 在相邻计时器回调之间会清空微任务队列,助手的单跳直接把测试推进到断言,派发计时器根本没来得及触发,流式增量块' still live'自然缺失。某次运行是否通过,取决于await act(...)的退出路径是否恰好多跨一个宏任务(让派发计时器插进来),因此故障呈间歇性(本地约 10%)而非确定性。两跳链式等待在结构上关闭了这个窗口:派发计时器必定在首跳回调之前的微任务清空阶段注册,因此第二跳——注册于首跳回调之后——无论两条链谁先注册计时器,都保证在派发完成之后才触发。这是纯测试改动;provider 的宏任务批量派发是有意设计(把事件突发合并为一次 reducer 执行),不做改动。
Reviewer Test Plan
如何验证
复现只需在
packages/webui里做普通重复循环(无需负载或压力);修复以同一循环跑干净为判据。同机同码 A/B,唯一变量是这 9 行助手改动:vitest run … -t 'keeps the current attachment live'循环DaemonSessionProvider.test.tsx,Node 24.18.1DaemonSessionProvider.test.tsx,Node 22.23.2(CI 版本)每次基线失败都是与 CI 完全相同的断言(
expected [ 'A transcript' ] to deeply equal [ 'A transcript', ' still live' ])。Node 22 一组排除了通过/失败两次 Release run 之间 Node22.23.1 → 22.23.2升级作为原因——修复前两个版本都能复现。文件内现有 10 处
flushTranscriptDispatch()调用点的意图都是"排空批量派发",加强排空对所有调用点都安全——包括两处"排除型"断言(not.toContain('stale output')、not.toEqual(...)),额外一跳只会让它们更严格。上面的整文件循环覆盖了它们。证据(Before & After)
N/A——测试时序改动;上面的修复前后失败率即证据。
测试环境
仅单元测试:
packages/webui下vitest run,Node 24.18.1 与 Node 22.23.2(失败 CI job 使用的版本)。风险与范围
关联 Issue
该 flaky 测试随 #8939 的同会话刷新覆盖引入(#8990 有扩展);失败运行为
main上的 Release run 31665796236。