Skip to content

ci: hold every pull request lane to one 45-minute limit - #3948

Merged
Astro-Han merged 1 commit into
apache:mainfrom
Astro-Han:ci/windows-lanes-input-closure
Aug 27, 2026
Merged

ci: hold every pull request lane to one 45-minute limit#3948
Astro-Han merged 1 commit into
apache:mainfrom
Astro-Han:ci/windows-lanes-input-closure

Conversation

@Astro-Han

@Astro-Han Astro-Han commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Summary

Every pull-request-triggered lane now carries timeout-minutes: 45.

The old 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 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.mjs now 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 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.

Refs #3945

What this PR no longer does, and why

An earlier revision of this branch also narrowed the paths filters on windows-recovery and release-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-recovery runs six gates. Splitting the closure by package:

gate group files reached share of PRs matched
storage (6 test files) 102 15.4%
runtime (2 test files) 206 18.4%
runtime-host (3 test files) 660 38.1%
all 675 39.8%

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 of runtime, runtime-host, storage and core. 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. Meanwhile release-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.
  • Mutation-checked the new contract test; each of these turns it red: 4560; deleting the timeout-minutes line; adding a quoted-id job with no limit; putting the limit at the wrong indentation.
  • npm run format:check — clean.
  • Every pull-request-triggered workflow re-parsed with yaml to confirm all nine jobs report 45.
  • The hit-rate and closure figures above were measured by replaying each lane's paths list against the last 300 first-parent commits on main with GitHub's glob semantics; the closure figures were cross-checked against an independent oxc-parser walk.
  • Not run: the repository-wide suite. No workflow logic changed beyond the limits, so no lane needed a live run to verify.

AI use

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

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-by trailer. A human contributor reviews the final diff and owns the change.

Checklist

  • Tests cover the change and fail without it
  • Lint, format and the affected suite pass locally (no TypeScript changed, so typecheck has nothing to cover; CI runs it regardless)

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@Astro-Han
Astro-Han force-pushed the ci/windows-lanes-input-closure branch from 5c03994 to 5ec2195 Compare August 26, 2026 17:22
@Astro-Han Astro-Han changed the title ci: narrow the two most expensive non-Linux lanes to their real input closure ci: derive the Windows recovery filter from what its steps reach Aug 26, 2026
@Astro-Han
Astro-Han force-pushed the ci/windows-lanes-input-closure branch from 5ec2195 to 72e576b Compare August 26, 2026 18:51
@Astro-Han Astro-Han changed the title ci: derive the Windows recovery filter from what its steps reach ci: hold every pull request lane to one 45-minute limit Aug 26, 2026
@Astro-Han
Astro-Han marked this pull request as ready for review August 26, 2026 19:20
@Astro-Han
Astro-Han requested review from M4n5ter and jackwener August 26, 2026 19:21
@github-actions github-actions Bot added the effort/S Under 100 readable lines label Aug 27, 2026
@Astro-Han
Astro-Han force-pushed the ci/windows-lanes-input-closure branch from 72e576b to bbaf798 Compare August 27, 2026 04:21

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_target lane 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-minutes at all, which is what pulls gitoxide-helper-admission.yml into line rather than letting it inherit GitHub's 360-minute default.
  • Counting jobs by runs-on rather than by job id avoids both false positives the comment describes, and step-level timeout-minutes sits 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 YayoiNanoka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.mjs passed 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 M4n5ter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.mjs passed 45/45.
  • Biome and git diff --check passed.
  • 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);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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)
@Astro-Han
Astro-Han force-pushed the ci/windows-lanes-input-closure branch from bbaf798 to ed1e6d0 Compare August 27, 2026 14:42
@Astro-Han
Astro-Han merged commit b9b590c into apache:main Aug 27, 2026
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/S Under 100 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants