ci: hold every pull request lane to one 45-minute limit - #3948
Conversation
5c03994 to
5ec2195
Compare
5ec2195 to
72e576b
Compare
72e576b to
bbaf798
Compare
jackwener
left a comment
There was a problem hiding this comment.
Approving. No blocking issues. I measured the job durations behind this change rather than taking the stated numbers on trust, and the reductions all hold up.
The reductions are safe
I pulled recent successful runs from the Actions API and timed each job:
| Job | Old limit | New | Measured max | Samples |
|---|---|---|---|---|
ci.yml / test |
120 | 45 | 20.4 min (median 13.4) | 11 |
release-windows-check / package |
90 | 45 | 25.6 min (median 22.5) | 16 |
cli-package-validation / Build immutable tarball |
60 | 45 | 1.6 min | 8 |
Nothing came within 19 minutes of the new limit, so none of the three cuts should start timing out work that used to pass. The tightest ratio is release-windows-check at about 1.8x its worst observed run, which is still reasonable headroom for a cold cache.
One inconsistency I would like resolved
copilot-auto-review.yml goes from 5 minutes to 45, but that workflow triggers on pull_request_target only. The comment in the new test says exactly why that case is different:
a
pull_request_targetlane reads the pull request rather than gating it, so it is not competing for a runner the author is waiting on and keeps its own tighter limit
The test agrees — \bpull_request\b does not match pull_request_target, so this lane is excluded from enforcement. The change to it is therefore neither required by the new rule nor covered by it, and it contradicts the rule's own stated reasoning. It also happens to be the one lane that runs with repository secrets exposed to fork events, which is an argument for keeping it short rather than lengthening it ninefold. I would revert that one file to timeout-minutes: 5.
A tradeoff worth naming
The other direction of this change is that several already-tight lanes get looser: the Validate installed CLI matrix goes 15 to 45 while its slowest arm measures 9.2 minutes, dependency-audit goes 15 to 45, windows_sandbox_w0_protocol 25 to 45, and four more go 30 to 45. The stated motivation is to stop hung jobs squatting on scarce runners, and for these lanes the change moves the other way — a hung validate job now holds a runner three times as long as it used to.
I understand this is the deliberate cost of "one tier, not per-lane values", and a single number is genuinely easier to keep honest than eleven. I am not asking for it to change, only noting that the PR reads as purely a tightening and it is not.
Also checked
- The new test fails a lane that has no
timeout-minutesat all, which is what pullsgitoxide-helper-admission.ymlinto line rather than letting it inherit GitHub's 360-minute default. - Counting jobs by
runs-onrather than by job id avoids both false positives the comment describes, and step-leveltimeout-minutessits at deeper indentation so the four-space anchor does not pick it up. - All 22 checks are green on
bbaf798.
Posted by an automated review agent operated by @jackwener. This is not an
independent human review and does not satisfy the committer review required by
CONTRIBUTING.md. A human is accountable for this comment — please push back if
anything here is wrong.
简体中文
本条评论由 @jackwener 运行的自动化审查程序发出。它不构成 CONTRIBUTING.md
所要求的独立人类审查,也不能替代人类审查。有人类对本条评论负责,如有错误请直接指出。
YayoiNanoka
left a comment
There was a problem hiding this comment.
Approved — I found no reachable P0/P1 blocker at bbaf79897b82535b4f8f7ce58e27d293dad54605.
The workflow contract derives pull-request lanes from the actual trigger block, counts runner-consuming jobs through runs-on, and requires one 45-minute timeout per such job. The changed workflows consistently apply that bound, while skipped/non-PR behavior remains governed by the existing job conditions.
Verification on the PR head and on a clean merge with current main:
node --test --test-concurrency=1 scripts/ci-test-plan.test.mjspassed all 45 tests.- Hosted checks are green, including the 20-minute core test lane, CLI package validation, Windows recovery, and Windows sandbox evidence.
- No temporary test was committed or pushed; the original workspace was unchanged.
点击展开中文
已批准——在 bbaf79897b82535b4f8f7ce58e27d293dad54605 上未发现真实可达的 P0/P1 阻塞问题。
工作流契约从真实的 trigger block 推导 pull-request lanes,通过 runs-on 统计会占用 runner 的 job,并要求每个此类 job 恰好有一个 45 分钟超时。改动涉及的工作流一致应用了该限制,而跳过路径与非 PR 行为仍由既有 job 条件控制。
已在 PR head 和与当前 main 的干净合并结果上验证:
node --test --test-concurrency=1 scripts/ci-test-plan.test.mjs的 45 个测试全部通过。- 托管检查全部为绿色,包括耗时 20 分钟的核心测试 lane、CLI package validation、Windows recovery 与 Windows sandbox evidence。
- 没有提交或推送临时测试;原工作区未改动。
M4n5ter
left a comment
There was a problem hiding this comment.
Approved at exact head bbaf79897b82535b4f8f7ce58e27d293dad54605: I found no reachable P0/P1 blocker.
The ordinary pull-request jobs now have a finite 45-minute ceiling, including the previously unbounded Gitoxide matrix, and the contract test catches missing, misplaced, or drifting job-level limits. The single tier genuinely removes per-lane configuration, but it also has the non-blocking scope mismatch noted inline: it widens one excluded pull_request_target job and one non-PR job despite not governing them as pull-request lanes.
Verification on this head:
ci-test-plan.test.mjspassed 45/45.- Biome and
git diff --checkpassed. - All exact-head hosted checks are terminal green.
- The current-main synthetic merge was clean and preserved both this timeout change and main's newer direct-peer Windows packaging steps.
Posted by an automated review agent operated by @M4n5ter. This is not an
independent human review and does not satisfy the committer review required by
CONTRIBUTING.md. A human is accountable for this comment — please push back if
anything here is wrong.
简体中文
本条评论由 @M4n5ter 运行的自动化审查程序发出。它不构成 CONTRIBUTING.md
所要求的独立人类审查,也不能替代人类审查。有人类对本条评论负责,如有错误请直接指出。
| const workflow = readWorkflow(name).replaceAll(/^[ \t]*#.*$/gmu, ''); | ||
| const start = workflow.indexOf('\njobs:'); | ||
| assert.ok(start >= 0, `${name}: no jobs block`); | ||
| const jobs = workflow.slice(start); |
There was a problem hiding this comment.
[P2] The enforced scope is wider in one direction and narrower in the other than the rule described above. After selecting workflows with a literal pull_request trigger, this scans every runner job below jobs: without evaluating its if, so it forces the cli-package-validation Eval job from 30 to 45 minutes even though that job explicitly cannot run on pull_request. Conversely, copilot-auto-review.yml is deliberately excluded here because it uses pull_request_target, yet this PR still changes its secret-bearing API job from 5 to 45 minutes without a contract covering that exception. The workflow remains trusted-base-only and cancel-in-progress, so this is operational exposure rather than a P0/P1 blocker. Please keep that job at 5 minutes and make the contract enumerate jobs that can actually run on ordinary pull requests.
Posted by an automated review agent operated by @M4n5ter. This is not an
independent human review and does not satisfy the committer review required by
CONTRIBUTING.md. A human is accountable for this comment — please push back if
anything here is wrong.
简体中文
本条评论由 @M4n5ter 运行的自动化审查程序发出。它不构成 CONTRIBUTING.md
所要求的独立人类审查,也不能替代人类审查。有人类对本条评论负责,如有错误请直接指出。
The limits were inherited rather than chosen: 120 on `ci.yml` against a 19-minute worst observed successful run, 90 on `release-windows-check` against 20, then 30, 25, 15 and 5 elsewhere, and none at all on `gitoxide-helper-admission`, which left its Windows and macOS matrix on GitHub's 360-minute default. A hung job therefore held a scarce runner for hours after any real run would have finished. 45 is about 2.3x the worst observed run of the slowest lane, which keeps the cold-cache and flake-retry headroom a 30-minute limit would not. Per-lane tiers buy nothing here: the cheap lanes rarely run at all, so raising their worst case costs far less than the two expensive ones save. `ci-test-plan.test.mjs` now reads the limit off every pull-request-triggered workflow, counting jobs by `runs-on` rather than by job id — a quoted id escapes an id pattern, and a two-space line inside a `run: |` block satisfies one — so a new lane cannot land without a limit and an existing lane cannot drift back to its own value. Generated-by: Claude Opus 5 (Claude Code)
bbaf798 to
ed1e6d0
Compare
Summary
Every pull-request-triggered lane now carries
timeout-minutes: 45.The old limits were inherited rather than chosen: 120 on
ci.ymlagainst a 19-minute worst observed successful run, 90 onrelease-windows-checkagainst 20, then 30, 25, 15 and 5 elsewhere — and none at all ongitoxide-helper-admission, which left its Windows and macOS matrix on GitHub's 360-minute default. A hung job held a scarce non-Linux runner for hours after any real run would have finished.45 is about 2.3x the worst observed run of the slowest lane, which keeps the cold-cache and flake-retry headroom a 30-minute limit would not. Per-lane tiers were considered and rejected: the cheap lanes rarely run at all, so raising their worst case costs far less than the two expensive ones save.
scripts/ci-test-plan.test.mjsnow reads the limit off every pull-request-triggered workflow, so a new lane cannot land without one and an existing lane cannot drift back to its own value. It counts jobs byruns-onrather than by job id — a quoted id escapes an id pattern, and a two-space line inside arun: |block satisfies one.Refs #3945
What this PR no longer does, and why
An earlier revision of this branch also narrowed the
pathsfilters onwindows-recoveryandrelease-windows-check, per the first half of #3945. I dropped that work after measuring it. Three findings, in order of how much they change the issue:1. The filter cannot be narrower than what the lane runs.
windows-recoveryruns six gates. Splitting the closure by package:Two of those eleven test files — the owner-death recovery gates — account for 655 of the 675 files on their own, because they fork
__tests__/fixtures/execution-host.ts, which boots a real Host and therefore reaches most ofruntime,runtime-host,storageandcore. That is what the gate tests, not an accident of how it is written. A correctly derived filter lands at 46.2% against today's 54.8% — the trigger rate is set by the lane's subject matter, not by the filter's precision.2. The projected runner saving does not survive a correct closure. #3945 quotes a 435-file import closure. Following imports alone misses this repository's other edge kinds —
fork(new URL('./x.js', import.meta.url))child entry points,run('npm', ['run', ...])chains, triple-slash references — and the corrected closure is 675 files, not 435. Meanwhilerelease-windows-check's hand list turns out to be too narrow rather than too wide, so deriving its filter widens it. Netted out against the medians in the issue, the expected non-Linux runner time per pull request moved by roughly +2s. There is no saving to collect.3. Materialising a 675-file closure into the workflow costs more than it returns. 30.0% of the last 300 first-parent commits touch an
import/export … from/<reference>line in those four source trees, and 16.3% add or delete a file there. Each of those would make the contract test red until the author regenerated a 699-line YAML block — and two concurrent PRs regenerating it conflict. That is a recurring tax on roughly one PR in three, paid for a saving that measures as zero.I also checked whether the Windows re-runs are redundant with the Linux suite: all three workspaces run
node --test "dist/**/*.test.js", so these files do run on Linux. But only two gates are Windows-only by construction (skip: process.platform !== 'win32'— NTFS alternate streams and the named-pipe endpoint), and every one of the remaining tests kills processes and renames or deletes files, with Windows-specific accommodations already in the source (CRASH_HARNESS_TIMEOUT_MS = win32 ? 180_000 : 60_000,maxRetries: win32 ? 20 : 0,terminateChildProcessTree). Those accommodations are evidence the tests did behave differently on Windows, so I found no safe cut there either.The remaining lever is to change what the lane runs on a pull request rather than which pull requests it runs on — for example leaving the owner-death gates to the unfiltered
push: [main]and nightly runs the lane already has. That trades pre-merge detection for trigger rate and is a maintainer call, not something to fold into a timeout change. I'll write it up on #3945 rather than guess at it here.Verification
node --test --test-concurrency=1 scripts/ci-test-plan.test.mjs— 44 tests, all pass.45→60; deleting thetimeout-minutesline; adding a quoted-id job with no limit; putting the limit at the wrong indentation.npm run format:check— clean.yamlto confirm all nine jobs report45.pathslist against the last 300 first-parent commits onmainwith GitHub's glob semantics; the closure figures were cross-checked against an independentoxc-parserwalk.AI use
Tool(s) and scope: Claude Opus 5 (Claude Code) made the change and did the measurement behind the section above, including the adversarial review that found the import-only closure defect in the dropped revision. The commit carries a
Generated-bytrailer. A human contributor reviews the final diff and owns the change.Checklist
Does this PR entail a change in behavior?