fix(web-shell): make the 30 s overview-poll e2e deterministic - #10934
Conversation
The "polls an expanded workspace once per 30 s tick and not faster" case has been red in every web-shell Browser Regression run since it landed in QwenLM#10407. The `settled` baseline raced the real-time startup bursts (StrictMode double mount, composer skill loader, connection-settle re-fetch), and the fake clock kept advancing in real time while the spec waited for startup to settle, so the mount-anchored 30 s interval tick landed inside the "no new requests" runFor window (Expected 11 / Received 16 in CI). Pause the fake clock across startup, let the real-time bursts land and stabilise, then flush up to and including the first poll tick so the baseline sits on a known interval phase before asserting the cadence. Tag the case @smoke so the pre-merge web-shell leg covers it. Fixes QwenLM#10903 Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-issue-patrol/jmtljezz3fw
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the PR! Template looks good ✓ — every section filled in, real before/after output, and a deliberate answer on the Problem: observed, not theoretical. #10903 carries job-level history (green 08-28/29/30, red from 08-31 onward), two named failing runs with job ids, and one stable assertion signature ( Direction: aligned. A standing red nightly is precisely the condition under which a real red gets waved through, and the 30 s polling contract #10407 introduced currently has no working automated proof in either direction. The issue's acceptance criteria explicitly permit the deterministic-equivalent path this PR takes, and it changes no product code. Size: not applicable. One changed file, Approach: the scope feels right and is genuinely minimal — one local helper plus a re-sequenced test body, the rest of the spec untouched, no drive-by edits or formatting churn. I checked the phase math against the product code rather than taking the description's word for it: the facet poll is a mount-anchored The one thing I'd ask you to think about, not a blocker: the 2 s quiet window is a wall-clock heuristic. It's generous against a local mock daemon where startup bursts are milliseconds apart, but it is still a real-time bound, so a pathologically slow runner could land a burst after it — which narrows the old race rather than removing it in every environment. Given the 60 s test timeout, the two CI retries, and the fact that a flake now surfaces pre-merge instead of only at 3 AM, that tradeoff looks acceptable to me; just naming it so it isn't a surprise later. The Risk: no elevated risk signals. The revert-history path screen doesn't match a spec file. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ —— 每一节都填了，有真实的 before/after 输出，并且对 问题： 是已观测到的 bug，不是理论性加固。#10903 提供了 job 级别的历史（08-28/29/30 绿，08-31 起每次红）、两个带 job id 的失败 run，以及一个稳定的失败签名（ 方向： 对齐。nightly 长期红正是"真正的红被放行"的那种环境；#10407 引入的 30 秒轮询约定目前两个方向都没有可用的自动化证明。issue 的验收标准明确允许本 PR 走的"确定性等价物"这条路，而且它不改任何产品代码。 规模： 不适用。只改了一个文件 方案： 范围合理，而且是真正最小化的——一个本地 helper 加上重排过的用例主体，spec 其余部分不动，没有顺手重构或格式化噪音。我没有直接采信描述，而是对着产品代码核了相位关系：facet 轮询是挂载时锚定的 有一个想请你考虑的点，不是阻塞项：2 秒静默窗口是一个真实时间的启发式判断。对着本地 mock daemon（启动突发之间只差毫秒）它很宽裕，但它终究是一个真实时间上界，所以极端慢的 runner 仍可能在窗口之后才落下一次突发——这是把旧的竞争收窄了，而不是在所有环境下彻底消除。考虑到 60 秒用例超时、CI 的两次重试，以及一旦抖动现在会在合入前暴露而不是只在凌晨三点，我认为这个取舍可以接受；只是先说清楚，免得以后意外。
风险： 无升级风险信号。回滚历史的路径筛查不会命中 spec 文件。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewI wrote down what I'd do before reading the diff, from the issue's two races alone: pin the phase with Playwright's documented install-then-pause idiom so every startup timer shares one fake instant, flush past exactly one interval to land on a known phase, take the baseline there, and wait out the startup bursts in real time with a Node-side timer rather than The subtlest part of the change is right, and it's worth saying why: I checked the timing claims against the product code instead of taking the description's word:
Conventions are clean: no No blockers. Two observations, neither a request to change this PR:
TestingWhat this section carries: the PR's own CI check results, read through the API for the reviewed commit. Nothing was built or executed here — this is an unattended CI run, so the review is static and no PR-derived code was run. No tmux capture (that path is local-invocation only). Not verified: the leg that actually executes the changed case has not run yet. Nothing red on this commit as of the fetch. Four real jobs still in flight ( Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 The author's local numbers (5 consecutive passes, full spec file green, ~18 s per run) are quoted in the description; they are the author's claim, not evidence I re-ran, and I'm not presenting them as verification. Sandboxed verification would settle the part CI cannot: 中文说明代码审查在读 diff 之前，我先只根据 issue 里那两个竞争写下了自己的做法：用 Playwright 官方文档里的 install-then-pause 惯用法把相位钉住，让所有启动定时器共享同一个假时刻；冲刷过恰好一个 interval 落到已知相位；在那里取基线；并且用 Node 侧的定时器（而不是 这个改动里最微妙的一点是对的，值得说明为什么： 我没有采信描述，而是对着产品代码核了这些时序论断：
约定方面是干净的：没有 没有阻塞项。两点观察，都不是要求本 PR 修改：
测试本节承载的是：通过 API 读到的、针对被审 commit 的 PR 自身 CI 检查结果。这里没有构建或执行任何东西——这是无人值守的 CI 运行，审查是静态的，没有运行任何 PR 派生代码。没有 tmux 抓屏（那条路径只适用于本地调用）。 未验证：真正执行被改用例的那条 leg 还没跑。 截至抓取，该 commit 上没有红的。四个真实 job 仍在进行（ubuntu （上方表格区域带机器可读标记，CI 落定后由 finalize 任务原地重写。） 作者本地的数字（连续 5 次通过、整个 spec 文件全绿、每次约 18 秒）引自描述；那是作者的说法，不是我重跑出来的证据，我不把它当作验证呈现。 沙箱验证可以补上 CI 补不了的那部分： — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 4/5 — solid and genuinely minimal; the two things I'd name are a pre-existing sibling assertion this PR correctly leaves alone, and the fact that the one CI leg able to substantiate "deterministic" hasn't run yet. Stepping back: this is the right fix for the right problem, and it answers the question the issue actually asked. #10903 isn't complaining that a test is red — it's complaining that a browser e2e case merged with zero execution evidence and then stayed red for days where only a schedule could see it. The PR fixes the case and closes that hole by tagging it The implementation doesn't feel like it's trying too hard. Twenty-odd lines of helper, a re-sequenced test body, nothing else touched. I went looking for the shortcut it missed and didn't find one: the startup bursts are React-effect and network driven, not timer driven, so no amount of fake-clock stepping can wait them out — a real-time settle is unavoidable, and doing it on Node's timer rather than The thing that most raised my confidence is that this test fails loudly rather than silently. If the phase pinning were wrong — say the clock resumed real time after the flush — the second advance would straddle the 60 s deadline and the quiet-window assertion would fail. There's no path where a broken pin produces a green run that measures nothing, which is the failure mode I'd normally worry about in a clock-mocking test. Combined with the interval being mount-anchored at a documented 30 s, the reload token never being bumped by a timer, and none of the shorter polls hitting a counted route, the arithmetic checks out from the product side and not just from the description. What I'd want before merging, and can't have in this run: the smoke job's actual result on this commit, and ideally a look at its Playwright report to confirm the case passed on the first attempt rather than on one of the two configured retries. A green run with Six months from now I'd thank whoever wrote this, because the comments explain the fake-clock reasoning that would otherwise read as arbitrary Verdict: approve — deferred until CI lands green. Two 中文说明信心度：4/5 —— 扎实且真正最小化；我要点名的两件事，一是本 PR 正确地没有去动的、同文件里一个早已存在的兄弟断言，二是唯一能证明"确定性"的那条 CI leg 还没跑。 退一步看：这是对的问题的对的修法，而且回答了 issue 真正在问的问题。#10903 抱怨的不是"某个测试红了"——它抱怨的是一个浏览器 e2e 用例在零执行证据的情况下合入，然后连续红了好几天，而只有 schedule 能看见它。本 PR 既修了用例，又通过打上 实现没有"用力过猛"的感觉。二十来行 helper、一段重排过的用例主体，其它什么都没动。我去找它漏掉的捷径，没找到：启动突发是 React effect 和网络驱动的，不是定时器驱动的，所以再多的假时钟步进也等不完它们——真实时间里的稳定等待无法避免；而用 Node 的定时器而不是 最让我提高信心的是：这个测试是响亮地失败，而不是静默地失败。如果相位钉定错了——比如冲刷之后时钟恢复走真实时间——那么第二次推进就会跨过 60 秒截止点，静默窗口的断言会失败。不存在"钉定坏了却跑绿、但什么也没测"的路径，而那正是我在一个 mock 时钟的测试里通常会担心的失效模式。再加上 interval 是挂载时锚定的、有文档记录的 30 秒，reload token 绝不由定时器自增，更短的那些轮询没有一个打在被计数的路由上——这套算术从产品侧就能核对，而不只是从描述里读到。 在合入前我会想要、但这一次拿不到的东西：这个 commit 上 smoke job 的真实结果，最好还能看它的 Playwright 报告，确认用例是第一次尝试就通过、而不是靠配置的两次重试之一通过的。在 六个月后我会感谢写这段代码的人，因为注释解释了假时钟的推理，否则那些 结论：approve —— 延后到 CI 变绿。 该 commit 上还有两个 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterFull-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
qwen-code-dev-bot
left a comment
qwen-code-dev-bot
left a comment
There was a problem hiding this comment.
Reviewed at head c1a6fe0a.
- One Playwright spec, no product code. The phase math checks out independent of the description: after
runFor(30_500)the mount-anchored 30 s interval sits 0.5 s past a tick with the next deadline 30 s away, so the quiet window at +29 s (59.5 s) stays pre-deadline and the assertion window at +2 s (61.5 s) straddles exactly one round — the case now measures "not faster than 30 s" instead of the old mount-to-runFor phase offset. A wrong pin fails loudly (the second advance would straddle two deadlines), which is the right shape for a clock-mocked cadence test. - The subtleties are each handled: the stability helper polls on Node timers, not
page.waitForTimeout, so it cannot deadlock against the paused fake clock; its 15 s budget throws rather than returning a moving baseline; both bursts that made the old snapshot stale (StrictMode re-mount, skill loader, connection-settle round) land inside the real-time quiet window before the flush; and re-takingsettledafter the flush anchors on the post-tick count. - Tagging the case
@smokeis the durable half of the fix: the pre-merge leg will execute it from now on, which is what the red-for-days nightly was actually complaining about. - No reviews or threads to reconcile; the author's before/after evidence reproduces the exact CI signature and five consecutive local greens, which is the claim under test. Local re-execution of the Playwright case was not possible on this machine (no browser binaries installed here) — disclosed rather than implied.
- CI facts: 15 checks pass, zero fail, three lanes still in flight on this head (the smoke leg that executes this case queues behind Test); per the channel convention the call is on the review itself.
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Scope: 1 file changed, test-only: packages/web-shell/client/e2e/web-shell.workspace-overview.spec.ts. Not reviewed: no production code touched; no CI configuration consulted. Working tree unavailable — execution rung not run (test-only spec file; no production behavior changed, so this does not block approval).
No blocking findings.
Approval blockers: none.
Findings
None.
What was checked
Class 1 — contract asymmetry: Not applicable — no persisted or transmitted format touched.
Class 2 — API compatibility: Not applicable — no exported symbols changed.
Class 3 — error handling: waitForStableOverviewCount propagates a throw at its 15s deadline and has no silent-swallow path. Correct.
Class 5 — test validity (primary concern for a test-only PR):
Can waitForStableOverviewCount pass while covering nothing? The function uses Node.js setTimeout and Date.now() in the test process, not page.waitForTimeout, which is coupled to the fake browser clock. This distinction is load-bearing: using page.waitForTimeout would advance the fake clock during the settle wait and undo the phase pinning that pauseAt established. The ordering clock.install() then clock.pauseAt(Date.now()) then gotoSession(...) is correct and necessary.
Phase math: The facet poll is a window.setInterval(..., 30_000) created at mount, gated on document.visibilityState === "visible". Freezing the clock across startup pins every instance to one fake instant; runFor(30_500) consumes exactly one tick; next deadline is 60 s; quiet window ends at 59.5 s; final advance reaches 61.5 s — exactly settled + 5 (one round of 5 facets). The assertion is tight in both directions: a 20 s polling interval fires during the 29 s short window (assertion fails); a 60 s interval does not fire during the remaining 31.5 s window (assertion fails). The test detects both too-fast and too-slow polling.
reloadToken re-anchor risk: workspaceSessionsReloadToken is bumped only by workspace removal and user actions, never by a timer. No re-anchor can fire inside the quiet window from a timer.
Other app timers during the flush: Shorter timers (live-state 2 s, session list 3 s, status 10 s, background tasks 3 s, git 60 s) all fire during runFor(30_500), but none match the overviewRequests filter /(mcp|skills|extensions|channels|memory|hooks)$/. They cannot inflate settled or the +5 delta.
@smoke tag: Adds the test to the pre-merge smoke job, closing the gap that allowed the original non-deterministic case to merge with zero execution evidence.
Cross-check against existing reviews: The qwen-code-ci-bot independently traced the same phase math, identified the same Node-vs-fake-clock distinction as the key mechanism, and noted the same pre-existing sibling assertion in the hover test. No finding in the bot review that I cannot confirm; no finding I hold that the bot refuted. The sibling assertion in the hover test was correctly left untouched by this PR.
Unreviewed dimensions
- Execution rung not run: no working tree available. The changed file is a browser e2e spec; the inability to execute it does not create a coverage gap that blocks approval of a test-only fix.
retries: 2in playwright.config.ts means a single green smoke job does not distinguish "deterministic" from "passed on retry". Pre-existing CI configuration, outside this diff, disclosed here only.
Reviewed with AI assistance.
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 — CI landed green after the review. ✅
4 participants
What this PR does
Makes the web-shell e2e case "polls an expanded workspace once per 30 s tick and not faster" deterministic and tags it
@smoke. The spec now pauses the fake clock across startup, waits for the real-time startup request bursts to settle, then flushes the fake clock up to and including the first poll tick so the baseline sits on a known interval phase, and only then asserts the cadence (no new requests just short of the next tick, exactly one round past it). No product code changes; the rest of the spec file is untouched.Why it's needed
The nightly
web-shell Browser Regressionjob has been red in every run since #10407 introduced this case (e.g. runs 33590493339 and 33702961416), always failing withExpected 11 / Received 16. Two intertwined races, both confirmed by local instrumentation:settledbaseline snapshot was taken as soon as the five distinct facets first appeared, while the startup bursts — the StrictMode double mount (two facet rounds), the composer skill loader's strayGET .../skills(counted because it shares the workspace route), and one more facet round when the connection settles — kept landing in real time afterwards, so the count kept climbing after the snapshot.page.clock.install()keeps advancing fake time in lockstep with real time while the spec waits ~2 s for startup to settle. The overview interval is anchored at mount, so its first tick deadline lies ~28 s into the fake 29 s "quiet" window and always fires inside it.So the case never actually measured "polling faster than 30 s"; it measured the phase offset between mount and the first
runFor, which is never zero in a real browser. The 30 s timer itself behaves correctly. This PR is the deterministic-equivalent path of the issue's acceptance criteria.Reviewer Test Plan
How to verify
From
packages/web-shell(full config, not the@smokesubset):npm run test:e2e -- --grep "30 s tick"Before: fails reliably with the exact CI signature (
Expected 11 / Received 16at the "just short of a tick" assertion). After: 5 consecutive local runs pass, and the whole spec file stays green:Why it is deterministic: with the clock paused, every startup timer is pinned to one fake instant; the flush then consumes exactly one interval tick (the following deadline is a full 30 s away), so the 29 s quiet window and the 2 s tick window keep a fixed phase relationship to the interval regardless of real startup duration.
Evidence (Before & After)
N/A (e2e spec-only change; the failing/passing output is inline above)
Tested on
Environment (optional)
Local vite dev server + Playwright chromium (headless) via the package's
test:e2escript;tsc -p tsconfig.json --noEmit,eslintandprettier --checkall clean for the package.Risk & Scope
@smokedecision (asked by the issue): added. This job only runs on schedule/workflow_dispatch, and the case merged without the pre-merge leg ever executing it — which is exactly how it stayed red unnoticed. The pre-mergeweb-shell browser smokejob already boots the same vite + mock-daemon harness for other@smokecases, so the marginal cost is this case's ~18 s runtime.Linked Issues
Fixes #10903
中文说明
这个 PR 做了什么
让 web-shell e2e 用例 "polls an expanded workspace once per 30 s tick and not faster" 变为确定性通过，并给它加上
@smoke标记。现在 spec 会在启动阶段暂停假时钟，等真实时间里的启动请求突发全部落定，再把假时钟快进“冲刷”到（含）第一次轮询 tick 为止，使基线落在已知的区间相位上，然后才断言节奏（下一个 tick 到来前没有新请求，过 tick 后恰好一轮）。不改任何产品代码，spec 文件其余部分不动。为什么需要
自 #10407 引入该用例后，nightly 的
web-shell Browser Regression任务每次运行都红（例如 run 33590493339 和 33702961416），失败签名始终是Expected 11 / Received 16。这是两个交织的竞争，均已通过本地埋点确认：settled基线快照在五个 facet 首次全部出现时就被取走，而启动突发——StrictMode 双挂载（两轮 facet 请求）、composer 技能加载器的单独GET .../skills（因为同路由被计入）、连接收敛后的又一轮 facet——之后仍在真实时间里陆续到达，计数在快照之后持续上涨。page.clock.install()在 spec 等待启动的约 2 秒真实时间里仍让假时钟与真实时间同步前进。overview 轮询定时器锚定在挂载时刻，其首个 tick 截止点恰好落在假时钟 29 秒“静默”窗口内（约第 28 秒），因此必然在窗口内触发。所以该用例实际测的从来不是“轮询快于 30 秒”，而是挂载时刻与首次
runFor之间的相位差——在真实浏览器里这个差值永远不为零。30 秒定时器本身行为正确。本 PR 走 issue 验收标准里“确定性等价物”这条路径。审阅者测试计划
如何验证
在
packages/web-shell下（全量 config，不是@smoke子集）：npm run test:e2e -- --grep "30 s tick"修复前：稳定复现 CI 签名（“差一点到一个 tick”断言处
Expected 11 / Received 16）。修复后：本地连续 5 次通过，整个 spec 文件保持全绿：为什么是确定性的：时钟暂停期间，所有启动定时器都钉在同一个假时刻；随后的冲刷恰好消耗掉一次区间 tick（下一个截止点在整整 30 秒之后），因此 29 秒静默窗口和 2 秒过 tick 窗口与区间保持固定相位关系，与真实启动耗时无关。
前后证据
N/A（仅 e2e spec 改动；失败/通过输出已在上文贴出）
测试环境
环境（可选）
本地 vite dev server + Playwright chromium（headless），走包内
test:e2e脚本；tsc -p tsconfig.json --noEmit、eslint、prettier --check对该包均干净。风险与范围
@smoke决定（issue 点名要回答）：加。该任务只在 schedule/workflow_dispatch 运行，用例合入时合入前 leg 从未执行过它——这正是它一直红却无人发现的原因。合入前的web-shell browser smoke任务本来就会为其它@smoke用例启动同一套 vite + mock-daemon 基建，边际成本只是该用例约 18 秒的运行时间。关联 Issue
Fixes #10903