ci: give minimum Node qualification runner headroom - #4836
Conversation
Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
huangruiteng
left a comment
There was a problem hiding this comment.
动机
这个 PR 修的是一个有直接证据的 CI 预算问题:#4833 的 node-minimum-compatibility 在完整 TypeScript suite 已经执行到后段时,被 10 分钟 job timeout 取消,随后 required merge gate 失败。这里的目标不是让 CI “更宽松”,而是让同一套最小 Node 兼容性资格验证由断言/进程结果决定成败,而不是被过紧的外层时钟随机截断。
改动思路
改动只调整现有 node-minimum-compatibility job 的 timeout-minutes,从 10 增到 15;不改变 job 条件、Node 版本、测试发现逻辑、排除规则、Node invocation、required-check 关系或 merge gate。同时在已有 workflow contract test 中固定三件事:仍遍历全部 tests/control_plane_ts/*.test.ts,仍使用原来的完整 Node 命令,并且不能通过 --test-name-pattern 或 shard 偷偷缩减覆盖来换取时长。
具体改动
.github/workflows/python-tests.yml:只把 minimum-Node lane 的外层预算从 10 分钟改为 15 分钟,并解释当前 8–11 分钟的实测区间与 runner variance。tests/test_python_ci_workflow.py:新增一个 job-block contract test,绑定新的预算与完整覆盖形状。- 基线/当前 head 的真实 runner 证据一致:#4833 run
35542808078的 minimum-Node job 在 10m16s 被取消;本 PR exact-head run35544194850使用相同完整选择,在 10m31s 成功。这个差异说明 10 分钟确实低于健康执行时长,而 15 分钟提供了约 4.5 分钟的抖动余量。 - 本地验证:workflow focused pytest 44 passed,Ruff passed,YAML parse passed,
git diff --checkpassed;远端 exact-head workflow 和 merge-gate 均成功。
对主干的风险
主要代价是:真正 deadlock/hang 的 minimum-Node job 最坏会多占用 5 分钟才被 GitHub 取消。这个风险是有界且局部的,只影响 core-tests 启用时的一个 job;普通断言失败仍会立即以非零退出,lane 仍然 required,merge gate 也没有弱化。
我特别检查了 budget change 的合理性:这里没有通过删测试、改扫描根、过滤名称或注册无关阈值来“把失败改绿”;对比的是同一 job、同一完整测试选择下的真实 wall-clock 结果。若未来稳定时长继续接近 15 分钟,下一步应该 profile/压缩或做明确保持覆盖的并行化,而不是机械继续抬高上限。
我的整体评价
这是一个范围很窄、证据充分的回归预算调整。预算 owner、完整覆盖和 merge obligation 一起被保留并增加了回归测试;当前 head 也在真实 GitHub runner 上证明 15 分钟足够、10 分钟不足。未发现 blocking finding,结论是 APPROVE。
未来向前看:此处无需再抽象新的 CI budget 框架;把测量证据留在 PR review、把 durable invariant 留在现有 workflow test,已经是更容易回滚和维护的边界。
Reviewed exact head: 1892d7f5ebea4a70e23c3e9d994f3d948eed5c7f
English verdict: APPROVE — the five-minute job-local increase is evidence-backed by equal full-suite runner observations, preserves the required minimum-Node coverage and merge gate, and bounds the only material downside to five additional minutes on a true hang.
huangruiteng
left a comment
There was a problem hiding this comment.
Approval conclusion (author-owned PR; GitHub blocks formal self-approval)
动机
本次审阅绑定 exact head 1892d7f5ebea4a70e23c3e9d994f3d948eed5c7f。这个 PR 修的是 evidence-backed CI regression budget:minimum-Node lane 的完整 TypeScript qualification 已经进入后段,却会被 10 分钟 job timeout 取消,导致 required merge gate 因外层时钟而不是测试结论失败。目标不是“让 CI 更宽松”,而是让同一套完整资格验证由断言/进程结果决定成败。
改动思路
改动只调整现有 node-minimum-compatibility job 的外层 timeout-minutes,从 10 提到 15;不改变触发条件、Node 版本、test discovery、SQLite exclusions、Node invocation、required-check 关系或 merge gate。已有 workflow contract test 被扩展为同时固定预算和覆盖形状,明确禁止通过 --test-name-pattern 或 sharding 偷偷缩短运行时间。
这是合理的 budget decision:被保护的 invariant 是“minimum supported Node 上完整执行适用的 control-plane TypeScript suite”,而不是“必须在一个历史偶然值 10 分钟内结束”。同 workload 的既有 runner evidence 表明健康执行可能超过 10 分钟;15 分钟提供 bounded variance headroom,同时保留 assertion failure 的即时失败语义。
具体改动
关键代码讲解
.github/workflows/python-tests.yml::jobs.node-minimum-compatibility.timeout-minutes从 10 改为 15;作用域只限一个 required job。test_minimum_node_lane_keeps_full_coverage_with_runner_headroom固定 15 分钟预算、tests/control_plane_ts/*.test.ts完整遍历和原有 Node command。- 同一测试明确拒绝
--test-name-pattern与shard,防止以后把预算问题“修”成 coverage reduction。
本地验证:workflow contract suite 44 passed,Ruff 通过,YAML parse 通过,git diff --check 通过;current-main merge-tree 成功。
对主干的风险
最强反例是一个真实 deadlock/hang:外层 timeout 现在最多晚 5 分钟才取消,增加 runner 占用和反馈延迟。这个代价有界且局部,只影响 core-tests 启用时的 minimum-Node job;普通断言失败和进程非零退出仍立即失败,lane 仍是 required,merge gate 没有被绕过。
本改动没有用删测试、缩 scan root、改 exclusions、增加 unrelated threshold 或把 required lane 变 optional 来隐藏失败。若以后健康时长继续逼近 15 分钟,应 profile/compact,或做明确保持覆盖的并行化;不应机械继续抬高预算。
语义与 CI 对齐
这里调整的是 evidence-backed regression budget,不是 hard correctness limit,也没有改变测试语义。workflow owner 与 contract test 一起更新,budget 和所保护的完整 coverage 保持对齐。
我的整体评价
未发现 blocking finding,结论是 APPROVE。这是一个范围窄、可回滚、维护成本低的 CI 预算修复;它保留了完整 minimum-Node qualification,并把“不能靠过滤或 sharding 换绿”固化成 durable regression contract。
未来向检查结论:无需为一个 job-local scalar 新增通用 CI budget framework。把 measured trade-off 留在审阅证据、把 durable invariant 留在现有 workflow test,是更清晰的边界。
English verdict: APPROVE — exact head 1892d7f5ebea4a70e23c3e9d994f3d948eed5c7f makes an evidence-backed five-minute increase to one required minimum-Node job, preserves the complete test selection and merge gate, and bounds the only material downside to five extra minutes on a true hang.
…-timeout Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
huangruiteng
left a comment
There was a problem hiding this comment.
Approval conclusion (author-owned PR; GitHub blocks formal self-approval)
Reviewed exact head: 584497801666bd304fa18eeca4af80cec89682ea (codex/minimum-node-ci-timeout, re-review after the branch was merged with main 92185803b).
动机
#4833 上必须通过的最小 Node 资格 lane 在 job 的时钟上限处被取消,而它的 TypeScript 用例当时已经跑过一千多个;近期成功的运行也贴着上限完成。也就是说,一次健康的完整资格验证会被 runner 抖动变成时钟失败。更糟的是它教给评审者的习惯:把这种取消当成噪声重跑。而最省事的"修法"——给 lane 加 --test-name-pattern 或切开 shard——会真正缩小兼容性覆盖,而且当时没有任何测试会反对。
改动思路
给 lane 留出余量,同时用一条 workflow 契约测试把"不许靠缩减覆盖换绿"钉住。测试断言的是契约形状而不是某次事故的文本:lane 必须保留 glob 驱动的完整选择、必须是一次 node … --test "${tests[@]}" 调用、不得出现 --test-name-pattern、不得出现 shard。这样预算调整可以重复使用,而不会成为下一轮缩覆盖的入口。
本 head 相对上次评审的唯一变化是与 main 的合并,而这次合并暴露出一件必须说清楚的事:main 已经通过它自己的提交把同一 lane 的 10 分钟改成了 15 分钟(83ab0c0eb "ci: preserve minimum Node conformance coverage",带一条等价注释)。冲突因此只落在 timeout-minutes: 15 上方那段解释性注释上。我保留了 main 的注释、放弃本分支措辞重复的那份,于是这个 PR 相对当前 main 的真实增量只有那条测试(git diff origin/main HEAD = tests/test_python_ci_workflow.py +12)。
具体改动
相对当前 main:1 个文件、+12/-0。
tests/test_python_ci_workflow.py:新增test_minimum_node_lane_keeps_full_coverage_with_runner_headroom,从 workflow 文本里切出node-minimum-compatibility块,断言timeout-minutes: 15、for test in tests/control_plane_ts/*.test.ts、单次node --no-warnings --experimental-sqlite --experimental-strip-types --test "${tests[@]}"、以及--test-name-pattern/shard均不出现。.github/workflows/python-tests.yml:本条分支原有的注释改动已被 main 的等价注释取代,因此在本 head 上该文件与 main 逐字节一致;lane 的 15 分钟预算本身在 main 上已经生效。
关键内容讲解
tests/test_python_ci_workflow.py:65:切块方式(在node-minimum-compatibility:与node-forward-compatibility:之间取文本)保证断言只作用于这一条 lane,不会因为别的 lane 加了过滤就误报,也不会因为别的 lane 的变化而漏报。- 四条内容断言各挡一种"缩覆盖换绿"的路径:换掉 glob 循环、把一次调用拆成多次、加
--test-name-pattern过滤、切 shard。只有第一条timeout-minutes: 15是关于预算的,其余三条是关于覆盖的——这正是它比"把 YAML 抄一遍"更有价值的地方。 - 把测试放进既有的
tests/test_python_ci_workflow.py(同文件已有 checks 聚合、stage2c 门禁、shard 覆盖等 workflow 契约),而不是新建文件,符合仓库"同一变更理由放同一处"的惯例。
我复核的关键点(都是自己跑的):
pytest -q tests/test_python_ci_workflow.py→ 44 passed;ruff check→ all checks passed;workflow YAML 解析通过(14 个 job,node-minimum-compatibility.timeout-minutes == 15)。- 变异验证(关键):我把
--test-name-pattern=smoke注入 lane 的 node 调用后重跑,test_minimum_node_lane_keeps_full_coverage_with_runner_headroom失败(1 failed / 43 passed);恢复文件后 44 passed、git status干净。这说明这条断言是承重的,不是装饰。 - 重复度扫描:
rg -ln 'timeout-minutes' tests/ examples/只有三个文件,其中没有任何一个此前断言过这条 lane 的预算;rg -ln 'node-minimum-compatibility'里除本测试还有examples/github-actions-runtime-smoke.py,而后者只断言 merge gate 的needs列表。也就是说这条覆盖边界此前无人看守,不是重复建设。 loopx canary premerge --from-git-diff:status passed、self_merge_allowed: true、manual_holds: 0;git diff --check干净。
对主干的风险
风险面很窄:本 head 不触碰 loopx/**、apps/**、packages/**,不改权限、持久化状态、quota/scheduler/todo 规则,也不改必需检查集合;它只增加一条针对 CI 契约的测试。真正需要防的回归是"以后有人缩减 lane 覆盖"——这正是变异验证证明被挡住的那条路径。两个诚实的边界:(1) 测试钉的是配置,不是真实耗时,15 分钟这个数仍是经验判断;(2) 它无法证明每个 runner 上都能跑完,只有 CI 自己能给这个答案。回退成本一个 commit。
我的整体评价
结论 APPROVE。这条 12 行的契约测试保护的是"必需 lane 的覆盖不许因为时钟被削掉"这一边界,此前确实无人看守,且我通过注入过滤实测它会失败。需要明确记在案的是:本 PR 声称的"把 10 分钟改成 15 分钟"在当前 main 上已经由另一提交完成,因此这个 head 的实际贡献是那条守卫,而不是分钟数;这也让这次合并变成一次纯粹的测试增量,风险与回退面都很小。
English verdict: APPROVE - exact head 5844978 (re-review after the branch merged main 9218580). The conflict was only the explanatory comment above timeout-minutes: 15, and main already carried an equivalent comment from its own commit, so the resolution keeps main's wording and the delivered delta against current main is the +12-line workflow contract test alone - the timeout raise itself is already in main. The test asserts the minimum-Node lane keeps its glob-driven full selection in a single node --test invocation with no --test-name-pattern filter and no sharding, plus the headroom value, so the cheap way to make a cancelled lane green (narrowing coverage) now fails the suite. I verified that this is load-bearing rather than decorative by injecting --test-name-pattern=smoke into the lane in a scratch copy: the guard failed (1 failed, 43 passed), and after restoring the file the module is 44 passed with a clean git status. A coverage scan found no prior assertion of this lane's budget or selection (rg -ln 'timeout-minutes' tests/ examples/ returns three files, none of them about this lane), so the guard is not duplicative; the author's open queue is the maintainer's ordinary multi-topic backlog rather than a same-shape smoke batch. ruff passes, the workflow parses, git diff --check is clean, and canary premerge reports passed with self_merge_allowed true and manual_holds 0. No blocking finding; residual limits are that the guard pins configuration rather than runtime duration, so the 15-minute budget stays an empirical judgement that only a real CI run can confirm.
Problem
The required minimum-Node qualification on PR #4833 was cancelled at the 10-minute job limit after its TypeScript tests had already passed more than 1,000 cases. Recent successful runs also finish close to that ceiling, so runner variance can turn a healthy full qualification into a clock failure.
Change
node-minimum-compatibilityfrom 10 to 15 minutesValidation
python -m pytest -q tests/test_python_ci_workflow.py— 44 passedpython -m ruff check tests/test_python_ci_workflow.py— passedgit diff --check origin/main...HEAD— passedThis is an evidence-backed job budget adjustment; it does not weaken minimum-runtime coverage or make the lane optional.