test(core): rescale the shell-safety CPU budget to the metric it measures - #10735
Conversation
…ures `process.cpuUsage()` is process-wide: it sums user + system across every thread for the window, so V8's background GC and JIT threads and the parser runtime's own threads are charged to it alongside the work under test. Measured against the built package on an idle machine, that reports 2.1-3.1x the wall time of the same six commands -- 2.1x even when they run sequentially, so the inflation is the accounting, not the `Promise.all` fan-out. When the assertions moved from wall clock to CPU time the 1000 ms literal came along unchanged, which left the budget below the floor of what a healthy run reports: ~270 ms in vitest on an idle machine, but 1232-1361 ms on GitHub-hosted runners, where it now fails deterministically. Per ci.yml's routing that lane is exactly where outside-contributor fork PRs land, so the failure reads to them as "your PR broke something". Rescale to 4000 ms and record how the number was derived. These tests guard against catastrophic backtracking on 10k-repetition adversarial inputs, which costs orders of magnitude rather than a small multiple, so the headroom does not blunt them. Fixes QwenLM#10734
github-actions
Bot
added
the
review/self-reported
|
|
|
Thanks for the PR! Template: Complete ✓ — every required section is present, and the description carries measured before/after data inline. (The Problem: Observed and deterministic, not theoretical. #10734 cites five Direction: In scope. This is a deterministic CI red, and the Size: Touches Approach: Minimal, and the right call. Rescale the literal to the metric it now measures, document the derivation, and explicitly defer the two larger alternatives (calibration, invariant-based assertion) to #10734. 4000 ms is ~2.9× the slowest observed healthy run (1361 ms); these guards catch order-of-magnitude regressions, so the headroom does not blunt them. Risk: No elevated risk signals — the changed file is a Moving on to code review. 🔍 中文说明感谢贡献！ 模板：完整 ✓——所有必需小节齐全，描述中直接内联了 before/after 实测数据。（ 问题：已观测到且是确定性的，不是理论问题。#10734 引用了五个托管 runner 上的 方向：在使命范围内。这是一个确定性 CI 红，且经 规模：触及 方案：最小且正确。把字面量重标定到它现在度量的量纲，记录推导过程，并把两个更大的替代方案（相对标定、断言不变量）明确留给 #10734。4000ms 约为最慢健康运行（1361ms）的 2.9 倍；这些防线针对的是数量级回归，余量不会钝化它们。 风险：无升级风险信号——改动文件是 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewIndependent baseline first: given "#10648 changed the metric from wall clock to process-wide CPU time but kept the 1000 literal, and hosted runners measure 1232–1361 ms", the minimal fix is to rescale the constant comfortably above the slowest observed run and document the derivation — leaving recalibration or invariant-based assertions for a follow-up. That is exactly what this PR does, so there is no simpler path it missed. The diff is one constant (1000 → 4000) plus an accurate derivation comment: process-wide accounting charges V8's GC/JIT threads, hence the 2.1–3.1× inflation, and the sequential control run shows it is the accounting rather than the CI evidenceCI on the reviewed commit is still running, and one caveat matters: I verified on Two facts frame the signal:
The oracle for the fix is this PR's own hosted-runner Test job once the unrelated transform error is out of the way. A sandboxed Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Real-scenario testingN/A — test-only change with nothing user-visible. Per the static-review rule the PR's code is never executed; test evidence comes from the PR's own CI above. 中文说明代码审查：先写独立方案——已知"#10648 把度量从墙钟改为进程级 CPU 时间、却保留了 1000 字面量，托管 runner 实测 1232–1361ms"，最小修法就是把常量重标定到最慢观测值之上并记录推导，标定/不变量方案留作后续。本 PR 正是这么做的，没有遗漏更简路径。 diff 就是一个常量（1000 → 4000）加一段准确的推导注释：进程级计费把 V8 的 GC/JIT 线程也算了进去，故有 2.1–3.1 倍膨胀；sequential 对照表明膨胀来自计费口径而非 CI 证据：被审提交的 CI 仍在运行，且有一个重要前提：已在 两个事实框定了信号：本提交的 修复的 oracle 是本 PR 自己的托管 runner Test job（待无关 transform 错误清除后）。沙箱 真实场景测试：N/A——纯测试改动，无用户可见行为。按静态审查规则不执行 PR 代码，测试证据来自上述 PR 自身 CI。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 5/5 — textbook small fix: a measured bug with public CI evidence, the minimum possible diff, and a derivation comment that saves the next reader the archaeology. Stepping back: the independent proposal I wrote before reading the diff — rescale the constant above the slowest observed run, document how it was chosen, defer the fancier alternatives — is exactly what landed. Every line in the diff is needed for the stated goal: one constant and the comment explaining it. This solves a real problem that lands disproportionately on first-time contributors, whose fork PRs run on GitHub-hosted runners where the wall-clock literal made Two process notes, neither a reflection on the code: this PR's 中文说明置信度：5/5——教科书式的小修复：问题有实测数据与公开 CI 证据，diff 取最小可能，推导注释让后来者不必再做考古。 退一步看：我在读 diff 之前写下的独立方案——把常量重标定到最慢观测值之上、记录取值依据、把更复杂的替代方案延后——与本 PR 完全一致。diff 中每一行都是目标所必需：一个常量加一段解释它的注释。这个问题真实存在，且不成比例地砸在首次贡献者头上——他们的 fork PR 跑在托管 runner 上，墙钟字面量让 两点流程说明，均与代码质量无关：在 #10729 合入之前，本 PR 的 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: reverse audit — stopped before round 1 by the review time budget.
Test Plan (not a blocker): src/utils/shellAstParser.test.ts — no such file or directory; 548 passed — this review observed 22978, 1898, 27712, 256, 1753, 504, 5468, 94 passed.
中文说明
未审查：反向审计——评审时间预算不足，未能开始第 1 轮。
Test Plan（非阻断）：src/utils/shellAstParser.test.ts — no such file or directory; 548 passed — this review observed 22978, 1898, 27712, 256, 1753, 504, 5468, 94 passed。
— qwen3.8-max via Qwen Code /review (v0.22.3)
|
Verification report at head What this is: one constant — Static check — the budget still bites: both call sites assert Local execution at head (scratch tree, Node 24.18.1, full Mutation check (assertion liveness): with the constant set to 100 ms in a scratch copy, CI at head: the PR's target suite is green in the Test(ubuntu) run ( Not approving: no ci-bot/maintainer APPROVED on record at 中文位于 — qqqys periodic review round · verified at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: reverse audit — stopped before round 1 by the review time budget.
Test Plan (not a blocker): src/utils/shellAstParser.test.ts — no such file or directory; 548 passed — this review observed 22988, 1898, 27821, 256, 1755, 504, 5628, 94 passed.
中文说明
未审查：反向审计——评审时间预算不足，未能开始第 1 轮。
Test Plan（非阻断）：src/utils/shellAstParser.test.ts — no such file or directory; 548 passed — this review observed 22988, 1898, 27821, 256, 1755, 504, 5628, 94 passed。
— qwen3.8-max via Qwen Code /review (v0.22.3)
|
CI's green now, hosted runner included. @yiliang114 @wenshao @chiga0 mind giving this a look when you have a sec? Test-only, one constant — process.cpuUsage() stays, just rescaling the literal to match |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Review passed.
The rescale is justified and well-documented: process.cpuUsage() is process-wide (user+system across all threads, so V8 GC/JIT and the parser runtime's own threads are charged alongside the work under test), and the measured 2.1–3.1x inflation over wall time — 2.1x even when the commands run sequentially — shows the old 1000 was a wall-clock number carried over unchanged when the metric changed, sitting below the floor of a healthy run (1232–1361 ms on GitHub-hosted runners). 4000 restores headroom without blunting the guard: these tests exist to catch catastrophic backtracking on 10k-repetition adversarial inputs, which costs orders of magnitude rather than a small multiple. CI green on this head.
qwen-code-dev-bot
left a comment
qwen-code-dev-bot
left a comment
There was a problem hiding this comment.
Reviewed at head 1a1711de.
- Correct diagnosis, minimally fixed: #10648 migrated the budget assertion from wall-clock to
process.cpuUsage(), which is process-wide (user+system across V8 GC/JIT and runtime threads), so the old1000was a wall-clock number applied to a 2–3x-inflated CPU metric — the author's sequential-vs-parallel measurements show the inflation is the accounting, not thePromise.allfan-out, so GH-hosted healthy runs (1232–1361ms) were already past the old ceiling. - Raising the ceiling to 4000ms keeps the guard's purpose intact: it defends against catastrophic backtracking on adversarial inputs, which costs orders of magnitude more than the added headroom, and the wall-clock cost stays well under vitest's timeout. Comment documents the derivation; no production code touched.
- No open threads, CI has no failures; per the channel convention the call is on the review itself.
|
Post-merge review — merged at Tier: Scan — test-only, one constant + comment, no production code. Scope: No blocking findings. What I checked:
Not run: rung 1 or higher (no working tree available; classified as not required — test-only change, no new algorithm, no production behavior changed). Cross-check against existing reviews: Reviewed with AI assistance. |
6 participants
What this PR does
Raises
maxClassificationCpuMsinpackages/core/src/utils/shellAstParser.test.tsfrom1000to4000, and records in a comment how the number was derived.Why it's needed
#10648moved the two budget assertions in that file from wall clock toprocess.cpuUsage(). That is a change of unit, not just of clock source, but the1000literal came along unchanged — so a wall-clock budget is now being applied to a CPU-time measurement.process.cpuUsage()is process-wide: it sumsuser + systemacross every thread in the process for the window, so V8's background GC and JIT compiler threads and the parser runtime's own threads are charged to it alongside the work under test.Measured directly against the built
packages/core/diston an idle 10-core M-series Mac (Node v22.22.1), running exactly the six adversarial commands from the failing test:The decisive rows are the sequential ones: the 2–3× inflation is not the
Promise.allfan-out over six commands — replacing it with an awaited loop still reports 2.1×. It is process-wide accounting of concurrent runtime threads. So the effect of#10648was to tighten the budget by 2–3× at the same moment the number being measured got noisier.That left
1000below the floor of what a healthy run reports:ecs-qwen-*)Hosted runners cluster at 1232–1361 ms — 1.23–1.36× the budget, with no overlap with it at all. This is not flake; it is the wrong constant for the metric. Per
ci.yml:155-159, in-repo PRs and fork PRs from OWNER/MEMBER/COLLABORATOR authors route to ECS and everything else stays on GitHub-hosted runners, so the failure lands squarely and only on outside-contributor fork PRs — the population least able to diagnose it, and for whom a redTestjob reads as "your PR broke something".Why 4000, and why it does not blunt the guard
4000 ms is ~2.9× the slowest healthy run observed anywhere (1361 ms) and ~15× what this machine reports inside vitest.
These two tests exist to catch catastrophic backtracking on 10 000-repetition adversarial inputs. That class of regression costs orders of magnitude, not a small multiple — it would blow past 1000, 4000, or 40000 alike. Widening from 1.3× under the healthy floor to ~3× above it removes the false positives without weakening what the assertions actually detect.
Alternatives considered
Recorded in #10734 and deliberately not done here, since either is a larger change that deserves its own discussion:
Reverting to wall clock is not proposed:
#10648's motivation was sound. Wall clock inflates under contention on shared runners because it counts descheduled time (see #10490,expected 1762 to be less than 1000on this very test), and CPU time correctly does not. The metric was the right call; only the constant was left behind.Reviewer Test Plan
How to verify
548 passed locally. To see the numbers for yourself, temporarily log
(cpuUsage.user + cpuUsage.system) / 1000next to each assertion and run with--silent=false: the adversarial-inputs test reports ~270 ms on an idle machine, the nested-substitution test ~4 ms.To confirm the guard still bites, drop the budget back to a value under the floor (e.g.
200) and watch the adversarial-inputs test go red.Tested on
prettier --checkandeslintclean on the changed file. Windows and Linux rely on CI.Risk & Scope
maincurrently carries a duplicatelanguage = 'en',binding inpackages/web-shell/client/components/ChatEditor.test.tsx(introduced byc3d54f12c8, feat(web-shell): add an experimental session workflow cockpit #8583), which is a hard syntax error and fails the wholeTestjob at transform time. That is orthogonal to this change.Linked Issues
Fixes #10734
中文说明
这个 PR 做了什么
把
packages/core/src/utils/shellAstParser.test.ts中的maxClassificationCpuMs从1000提到4000，并用注释记录这个数字是怎么定出来的。为什么需要
#10648把该文件的两条预算断言从墙钟时间改为process.cpuUsage()。这不只是换了计时源，而是换了量纲，但1000这个字面量原封不动地留了下来——于是一个墙钟预算被用来约束 CPU 时间的测量值。process.cpuUsage()是进程级的：它把窗口期内进程中所有线程的user + system累加，因此 V8 的后台 GC 与 JIT 编译线程、解析器运行时自身的线程，都和被测代码一起被计入。直接针对构建产物
packages/core/dist实测（空载 10 核 M 系列 Mac，Node v22.22.1，跑的正是失败测试里那六条对抗性命令），数据见上方英文部分。决定性的是 sequential 两行：2–3 倍的膨胀并非来自对六条命令的
Promise.all并发——改成串行 await 后依然是 2.1 倍。它来自对并发运行时线程的进程级计费。因此#10648的实际效果，是在被测数值变得更嘈杂的同时把预算收紧了 2–3 倍。这使得
1000落到了健康运行的地板以下：本机在 vitest 中报 267.6ms（通过），托管 runner 报 1232.8 / 1322.3 / 1328.3 / 1340.3 / 1360.8ms（全部失败），ECS 通过。托管 runner 集中在 1232–1361ms，是预算的 1.23–1.36 倍，与预算区间完全不重叠——这不是抖动，而是这个常量对新口径就是错的。按
ci.yml:155-159的路由，仓库内 PR 与 OWNER/MEMBER/COLLABORATOR 作者的 fork PR 跑 ECS，其余留在托管 runner，所以这个失败只砸在外部贡献者的 fork PR 上——恰恰是最难自行诊断的一群人，而红掉的Testjob 对他们读起来就是"你的 PR 弄坏了什么"。为什么是 4000，以及为什么不削弱这条防线
4000ms 约为已观测到的最慢健康运行（1361ms）的 2.9 倍，约为本机 vitest 内数值的 15 倍。
这两条测试的存在意义是捕获一万次重复对抗性输入引发的灾难性回溯。那一类回归的代价是数量级级别的，而不是小倍数——它会同样轻易地突破 1000、4000 或 40000。把边界从"低于健康地板 1.3 倍"改成"高于地板约 3 倍"，消除了误报，却不改变这些断言真正能检测到的东西。
考虑过但未采用的方案
记录在 #10734，本 PR 刻意不做，因为两者都是更大的改动、值得单独讨论：改为相对标定；以及断言不变量而非时长。
不提议改回墙钟计时：
#10648的动机是成立的。墙钟时间在共享 runner 上会因计入被调度出去的时间而在争用下膨胀（见 #10490，同一条测试上的expected 1762 to be less than 1000），而 CPU 时间正确地不计这部分。口径选对了，只是常量没跟上。复核测试方案
从
packages/core运行npx vitest run src/utils/shellAstParser.test.ts，本地 548 条全部通过。若想自行看数值，在两条断言旁临时打印(cpuUsage.user + cpuUsage.system) / 1000并加--silent=false运行：空载机器上对抗性输入那条约 270ms，嵌套替换那条约 4ms。若想确认防线仍然有效，把预算调回地板以下（例如200），对抗性输入那条会变红。prettier --check与eslint在改动文件上均干净。Windows 与 Linux 依赖 CI。风险与范围
main上packages/web-shell/client/components/ChatEditor.test.tsx存在重复的language = 'en',绑定（由c3d54f12c8、feat(web-shell): add an experimental session workflow cockpit #8583 引入），这是硬语法错误，会在 transform 阶段直接让整个Testjob 失败。与本改动正交。关联 Issue
Fixes #10734