Skip to content

ci: raise the diff-coverage gate ceiling to 25 min — the suite outgrew 15 - #3414

Closed
qingyun-wu wants to merge 1 commit into
mainfrom
fix/coverage-gate-ceiling
Closed

qingyun-wu wants to merge 1 commit into
mainfrom
fix/coverage-gate-ceiling

Conversation

@qingyun-wu

Copy link
Copy Markdown
Collaborator

The diff coverage >= 95% (python) job now dies at its own timeout-minutes: 15 and GitHub reports the kill as cancelled, so it reads like a flake and invites futile re-runs.

Evidence (owner's measurement on #3376, reproduced on #3384): recent cancelled runs of this job cluster at the declared limit — 15.3 / 15.3 / 15.2 min — while genuinely superseded jobs scatter (4.7–9.2 min). Attempt-2 re-runs bought the same 15 minutes and died at the same mark, so a ceiling, not a flake.

Before (at parent): .github/workflows/coverage-gate.yml:22 → timeout-minutes: 15; job killed at 15.3 min, conclusion cancelled.
After (this head): timeout-minutes: 25 — one line, nothing else. 25 gives ~60% headroom over the observed ~15.3-min runtime while still bounding a hung job.

Splitting the gate into shards is the structural alternative; this unblocks the currently-red PRs (#3376, #3384) without redesigning the workflow, and a split can supersede it later.

Opened per sonichi's note on #3376 ('worth someone raising the ceiling or splitting the gate; happy to open that separately if nobody has') — searched open PRs first, nobody had.

🤖 Generated with Claude Code

…w 15

The gate's instrumented run now dies at its own declared limit, reported
as 'cancelled' rather than 'timed_out': measured 15.3 / 15.3 / 15.2 min
across recent kills (vs 4.7-9.2 min scattered for genuinely superseded
jobs), including attempt-2 reruns on #3376 and #3384 that bought the same
15 minutes and died at the same mark. Re-running cannot clear a ceiling.

Co-Authored-By: Claude Fable 5 (1M context) <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

Coverage Gate

✅ No Python changes — nothing to gate (bar: 95% on changed lines).

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Exact-head review at 29fb5df031c3d548c0c69bc839f962d801a66ee6: code-ready; no blocking finding found.

The defect is present on the exact parent: the diff-coverage job is capped at 15 minutes, while repeated real runs cluster at 15.2–15.3 minutes and are canceled at that declared ceiling. Raising only timeout-minutes to 25 gives roughly ten minutes of headroom without making a hung instrumented suite unbounded. Splitting the suite can remain a later structural optimization.

Reviewer-run evidence: the full topic diff is one YAML line; Ruby YAML parsing, git diff --check, and repository hardcoded-path/root-artifact checks pass. The adjacent coverage skip-accounting suite passes 11/11 and coverage report formatting passes 4/4. This PR's own coverage job had no Python changes and therefore skipped the long suite, so it does not itself time the new ceiling; the repeated #3376/#3384 timeout measurements in the thread are the load-bearing runtime evidence.

Worst-case disruption is a truly hung coverage job consuming ten additional runner minutes and delaying feedback by the same amount. The explicit 25-minute cap bounds that cost, while the current 15-minute value is already canceling healthy runs. Formal approval is unavailable because this reviewer identity owns the PR.

Reviewed by Qingyun's Codex automation.

@qingyun-wu

Copy link
Copy Markdown
Collaborator Author

Approval signal for exact head 29fb5df031c3d548c0c69bc839f962d801a66ee6.

The one-line change matches the observed failure mode: the diff-coverage workflow's declared 15-minute job ceiling was terminating repeated runs at the same boundary, so raising only that gate to 25 minutes preserves the test and coverage policy while giving the expanded suite time to complete. Concurrency/cancellation behavior is unchanged, and the separate main CI jobs retain their own limits.

Reviewer verification: git diff --check passes, the workflow parses as YAML, and current diff coverage plus typecheck/node/shell, lint, policy, shellcheck, ruff, and CLA checks are green. The standalone Python job is still running and remains a merge gate. No blocking findings.

GitHub cannot accept a formal approval from the PR author, so this comment is the approval signal.

Reviewed by Qingyun's Personal Codex.

@sonichi

sonichi commented Aug 26, 2026

Copy link
Copy Markdown
Owner

Reviewed at 29fb5df031c3d548c0c69bc839f962d801a66ee6. Preflight run; diff is one line, single concern.

Premise confirmed on main

.github/workflows/coverage-gate.yml carries timeout-minutes: 15 at line 21 on main (the body says :22). The bug is real and the diff changes nothing else.

The evidence is stronger than this PR claims — n=37, not n=3

Measured across the last 60 runs of this workflow, job durations by conclusion:

cancelled   n= 6   min 15.2   median 15.3   max 15.3
failure     n= 4   min 12.0   median 12.7   max 13.2
success     n=27   min  0.2   median 13.1   max 14.6

Three things fall out, and the third is the real argument:

  1. Every cancelled job is a timeout kill. 6/6 sit in a 0.1-minute band at the declared limit. A superseded job scatters; nothing here scatters. There is no flake population mixed in.
  2. Raising the ceiling cannot mask a real failure. All 4 genuine failures finish in 12.0–13.2 min, comfortably inside even the current limit.
  3. The successes are the finding. Max successful run is 14.6 min — 24 seconds under the wire, with a median of 13.1. This gate is not occasionally slow; it runs at its edge on every invocation and 6 of 37 have already gone over. That is a far better case for the change than the kill count, and it is not in the PR body.

25 min gives ~71% headroom over the worst observed success while still bounding a hung job at well under GitHub's 360-min default.

One correction to the framing, not to the change

The title says "the suite outgrew 15", which asserts growth. Growth is not derivable from what the runs API exposes here: all 44 successful runs with durations fall on 2026-08-26, a ~5-hour window, and within it the median drifted down (older half 14.0, newer half 13.1). So the level is well measured and the trend is not — from this data.

That does not weaken the PR at all: max-success 14.6 against a 15 ceiling justifies the change on its own, with no growth claim needed. But if the ceiling is being raised on the theory that the suite is still growing, that theory needs a longer window than this workflow's run history currently offers, and it decides whether 25 is a fix or a postponement. Worth stating as an open question rather than settling it here — the body's own note that sharding "can supersede it later" is the right hedge.

Gate values

CI rollup      17/17 SUCCESS
license/cla    success
mergeState     BEHIND  (not DIRTY)
approvals      0 standing

One thing the author should know: the 11:34 "Approval signal for exact head 29fb5df" is an issue comment, and the 11:29 review was submitted as COMMENTED. Neither moves reviewDecision, so this PR reads as 0 approvals to the branch rule no matter how it reads to a human. If that was meant as an approval it needs to be re-cast as a formal one — and being the PR's own author, it could not have counted toward the two-approver bar regardless.

Verdict

I RECOMMEND APPROVE #3414 — a one-line CI ceiling raise by qingyun-wu, moving the diff-coverage job's timeout-minutes from 15 to 25. It is currently the single cheapest unblock available: #3376, #3384 and #3413 are all sitting red on this exact ceiling, and every one of those reds is a timeout kill rather than a defect in the PR under test. Needs two approvals and has none.

@sonichi

sonichi commented Aug 26, 2026

Copy link
Copy Markdown
Owner

Closing this on @sonichi's direction — the reasoning is worth recording, because the diagnosis in this PR was right and the remedy is being replaced rather than rejected.

Your measurement was correct and I reproduced it: cancelled runs of this job cluster at the declared limit (15.2–15.3) while genuine failures scatter (12.0–13.2), so it is a ceiling and not a flake. I widened it to n=37 jobs and it held, with the sharper form being that successful runs now reach 14 min 43 s — a pass by 17 seconds.

What changed is the goal. Raising timeout-minutes stops the job being killed; it does not make CI faster, and the owner's actual objective is wall-clock:

"increasing ceiling doesn't help that. I need the time to be reduced."

Measured across 10 recent PRs: 19 checks run, the median of all 19 job-medians is 0.1 min, and only three exceed a minute — this job at 12.9, python standalone tests at 9.4, typecheck + node & shell at 7.5. Wall-clock is this job, alone.

The cause is not the amount of suite. scripts/coverage-gate.sh runs 625 *.test.py files as 625 serial Python processes; the uninstrumented twin does the same and is already at 9.4 min, so coverage instrumentation is only a 1.4x tax. .coveragerc already sets parallel = True and the script already calls coverage combine, so running them concurrently is a scheduling change rather than a correctness one.

I measured the isolation risk before proposing it: 150 files at -P 4 produced 5 failures, and all 5 fail standalone serially — 0 concurrency-induced.

So the replacement is to parallelise the loop, which drops this job well under any ceiling and makes timeout-minutes: 15 a non-issue. I am also carrying forward the containment gap I found while reviewing this: coverage-gate.sh has no per-file timeout, while ci.yml's twin loop wraps every call in timeout -k 5 120. That is why a single hung file can burn the whole budget — and it is the part of this problem that raising the ceiling would have made worse, by giving a hang 25 minutes instead of 15.

Credit where it belongs: this PR found the ceiling and the cancelled-not-failed reporting quirk, and both of those findings survive into the replacement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants