Skip to content

fix(ci): give the coverage matrix arm its own job timeout - #7517

Closed
SebastianYuSun wants to merge 1 commit into
kirodotdev:mainfrom
SebastianYuSun:fix/ci-coverage-arm-shard-timeout
Closed

fix(ci): give the coverage matrix arm its own job timeout#7517
SebastianYuSun wants to merge 1 commit into
kirodotdev:mainfrom
SebastianYuSun:fix/ci-coverage-arm-shard-timeout

Conversation

@SebastianYuSun

Copy link
Copy Markdown
Contributor

Problem / Motivation

backend-test runs the same four shards twice, on 3.10 with --no-cov and on 3.12 with --cov, and timeout-minutes is a job level key shared by all eight matrix cells. Coverage roughly doubles wall time, so the 30 minute cap that the cheap arm uses less than half of is the cap the coverage arm hits. A shard cancelled at the wall makes Coverage Gate fail closed with backend-test=cancelled -- failing closed.

Measured as completed_at - started_at from /commits/<sha>/check-runs:

head 3.10 (--no-cov) 3.12 (+cov) max ratio
d94950d39 7.68 to 13.87 min 27.33 to 30.25 CANCELLED 2.18x
760124a75 10.00 to 13.60 min 22.22 to 30.27 CANCELLED 2.23x
92c3b5fff 12.02 to 13.83 min 18.32 to 27.13 min 1.96x

Why it matters

It is red on main, not only on pull requests. Seven consecutive pushes to main sampled on 2026-09-01:

main head 3.12 shard times cancelled
dd9e002bf 21.7, 30.3, 30.3, 21.3 2
bb01943c1 15.1, 28.9, 28.2, 26.5 0
fd38eff08 27.6, 28.3, 27.1, 27.2 0
15b062558 27.9, 30.3, 27.7, 28.0 1
da28c697f 26.9, 28.3, 26.5, 24.4 0
7eea2bea1 26.2, 28.3, 27.9, 27.8 0
78cc85a83 15.3, 27.4, 28.4, 18.4 0

Three of seven lost a shard at the wall. The runs that survived finished at 94 to 96% of the cap, so ordinary variance is enough to flip the rest. Every red costs a maintainer a re-run and teaches contributors to read a real Coverage Gate failure as noise.

What changed (motivation → approach → change)

Symptom: a lone 3.12 shard reports cancelled while every sibling passes, and Coverage Gate fails closed on it.

Root cause: timeout-minutes is one scalar for a matrix whose two arms do measurably different amounts of work. Coverage is enabled on 3.12 only, and tracing costs about 2x wall time here, so a cap sized for the --no-cov arm is 46% occupied on that arm and 101% occupied on the other.

Change: resolve the cap per arm.

timeout-minutes: ${{ matrix.python-version == '3.12' && 45 || 30 }}

Two things I checked rather than assumed, because guessing at CI semantics is the same class of error as the defect:

  • jobs.<job_id>.timeout-minutes accepts the matrix context. GitHub's own context availability table lists github, needs, strategy, matrix, vars, inputs for that key. This is the first expression valued timeout-minutes in this repository, so it is worth a maintainer's eye on the shape even though the syntax is documented.
  • 45 comes from the measurement, not from taste. The slowest uncancelled coverage shard was 28.93 minutes, which is 64% of 45. The cancelled shards are censored at the 30 minute wall, so their real length is unknown, which is why I did not try to derive a tighter number from them. For comparison backend-test-windows already carries 40 minutes for its slowest observed shard of 18.03, so this leaves the coverage arm proportionally tighter than the Windows arm, not looser.

Deliberately not in this PR:

Tests

test/test_backend_test_timeout_contract.py, modelled on the existing test/test_xdist_worker_restart_contract.py workflow contract test.

  • test_ci_still_has_a_coverage_conditional_backend_matrix is the anti-vacuity guard. Every other assertion reads the job, matrix key and coverage-bearing value discovered here, so a job rename or a move of coverage into its own job fails loudly instead of turning the file into a no-op.
  • test_the_coverage_arm_is_not_capped_like_the_cheap_arm resolves timeout-minutes for each matrix arm and asserts the coverage arm gets strictly more. A plain scalar resolves to itself for every arm, which is exactly the unfixed state, so this fails on unfixed ci.yml.
  • test_the_coverage_arm_gets_at_least_the_windows_arms_budget pins the floor at the Windows arm's 40, since the coverage arm's measured maximum already exceeds the Windows arm's.

It is deliberately one directional. It does not pin the cheap arm's number, does not forbid raising both, and does not care which expression shape encodes the split, so a future change that gives the coverage arm even more room stays green.

Load bearing, verified by reverting the workflow with git show HEAD~1:.github/workflows/ci.yml:

2 failed, 1 passed     (unfixed ci.yml)
3 passed               (with the fix)

Manual verification

N/A for the runtime behaviour, since this changes a workflow and not code that a test can execute. What was run locally on the pushed head:

  • every test file in the repository that reads .github/workflows/ci.yml, 24 files, 1899 passed
  • python3 scripts/check_black_formatting.py, isort --check-only src/kiro_crew test conftest.py xdist_budget.py, flake8 src/kiro_crew test conftest.py xdist_budget.py, mypy src/kiro_crew/, all clean
  • ./scripts/scrub-lint.sh --no-history, all checks passed

Screenshots / video

N/A, no user-visible UI change. This edits .github/workflows/ci.yml and adds a test that parses it, so there is no rendered surface to show.

Related Issues

Fixes #7516

Two independent PRs are sitting red on this today, #7414 and #7376, and neither diff can account for a 28 minute shard. Prior art on the split half is #4227, closed on its fail fast half by #5043 and #5267 with the durations question left open.

Pattern harvest

Rule candidate: review-prompt
Pattern: a job level resource budget shared by matrix arms that do measurably different amounts of work. timeout-minutes sits above strategy.matrix, so a cap written when every cell was equivalent silently becomes wrong the moment one arm gains work that the others do not have. Worth asking on any PR that adds a per arm conditional flag to a matrix step: does the cheap arm's budget still fit the arm you just made expensive?

Checklist

  • At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable)
  • No secrets, credentials, or internal references in the diff

Contribution License Agreement

backend-test runs four shards twice, on 3.10 with --no-cov and on 3.12 with
--cov, and timeout-minutes is a job level key shared by all eight matrix cells.
Coverage roughly doubles wall time, so one cap sized for the cheap arm cancels
the expensive one at the wall while its sibling finishes under half the budget.

Measured from completed_at minus started_at on three heads:

  head        3.10 (--no-cov)      3.12 (+cov)
  d94950d   7.68 to 13.87 min    27.33 to 30.25 CANCELLED
  760124a   10.00 to 13.60 min   22.22 to 30.27 CANCELLED
  92c3b5f   12.02 to 13.83 min   18.32 to 27.13 min

It is not confined to pull requests. Of seven consecutive pushes to main
sampled on 2026-09-01, three had a 3.12 shard cancelled at the wall and the
rest finished at 28.3 to 28.9 minutes, which is 94 to 96 percent of the cap. A
cancelled shard makes Coverage Gate fail closed, so the red lands on diffs that
cannot account for a 28 minute shard.

The same file already accepts the principle. backend-test-windows carries 40
minutes because those runners run the same shards measurably slower, and its
slowest observed shard used 18.03 of the 40. The coverage arm is closer to its
budget than Windows is to its, and still shares 30.

45 comes from the measurement rather than from taste: the slowest uncancelled
coverage shard was 28.93 minutes, which is 64 percent of 45. The cancelled
shards are censored at the wall, so their real length is unknown, and that is
why the split itself is the other half of this problem. Without a committed
.test_durations file pytest-split falls back to an even split by test count, so
which shard carries the heavy tests is arbitrary. Landing that file is a
maintainer action and is not part of this change.

Adds test/test_backend_test_timeout_contract.py, which resolves the job cap per
matrix arm and asserts the two arms are not capped identically. Two of its
three assertions fail on the unfixed workflow.
@SebastianYuSun

Copy link
Copy Markdown
Contributor Author

@bolichen97 @iamwhatever this one touches .github/workflows/ci.yml, so it needs the allow-fork-workflow-change label before fork CI can report anything. The code is final, so the label will not get stripped by a later push.

Context for why it is worth the round trip: main itself is losing 3.12 shards to the same wall. Of seven consecutive pushes sampled on 2026-09-01, three had a shard cancelled at 30.3 minutes and the rest finished at 28.3 to 28.9, which is 94 to 96 percent of the cap. Details and the measurements are in #7516.

The split half of the problem is not in this PR. chore/update-test-durations needs write access to land and #4227 already treated it as a CI policy decision, so I left it to you.

@github-actions github-actions Bot added the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Sep 1, 2026
bolichen97 added a commit that referenced this pull request Sep 2, 2026
Diagnostic report finding the 14.8% reading is a CI coverage-arm
truncation artifact (#7516/#7517 class), not an undertested file.
TestFleetProbe already covers fleet_probe.py; no tests added and no
baseline entry, per the gate's own wording.
chenmingwei23 pushed a commit that referenced this pull request Sep 2, 2026
…7613)

Diagnostic report finding the 14.8% reading is a CI coverage-arm
truncation artifact (#7516/#7517 class), not an undertested file.
TestFleetProbe already covers fleet_probe.py; no tests added and no
baseline entry, per the gate's own wording.
@bolichen97

Copy link
Copy Markdown
Collaborator

Closing — no longer applicable: #7780 merged and #7552 already raised the cap

Verified relationship: superseded by work already on main``

#7780 merged as 5119816 and its content is on origin/main 1a765b8: pyproject.toml requires-python = ">=3.12", dep_sync.python_floor_breach wired at dep_sync.py:955 (the reinstall branch), and backend-test's matrix collapsed to python-version: ["3.12"]. #7517's entire production change is one line that no longer applies: I isolated its ci.yml hunk and ran git apply --check against main's ci.yml in a scratch dir, and it fails, because the timeout-minutes: 30 line it deletes was already replaced by timeout-minutes: 40 in landed 7827b03 (#7527/#7552) under a comment that states #7517's exact failure mode verbatim ("a 30-minute cap kills the job at the coverage step with every test green and Coverage Gate then reds on the missing shard artifact"). Its 164-line test file is not merely stale: I executed all three tests against main's ci.yml and each raises _coverage_arm()'s own AssertionError -- "the arms no longer share a cap and this file should be deleted" -- because #7780 removed the only matrix ternary in the file (a grep for any ${{ matrix.X == 'v' && .. || .. }} on main's ci.yml returns nothing). Rebased, #7517's expression resolves to a constant 45 for the single remaining arm, so it differentiates nothing, and the residue is a 40 -> 45 nudge with no measurement behind it: its own data gives a slowest uncancelled coverage shard of 28.93 min and censored maxima >= 30.3, both comfortably inside 40. So this is not co-equal work owing a consolidation decision -- the still-open side has no mergeable artifact left, only an idea worth ~15 lines. the first adjudication's functional overlap with survivor #7780 was mechanically wrong (#7780 never touches timeout-minutes) but landed on the right disposition; with #7780 merged, the accurate label is supersession by the landed tree, done jointly by #7780 (killed the mechanism and the test's precondition) and #7552 (delivered the outcome).

Carry this over first

This closure is about redundancy, and these items are the exception: they are not on main and not in the surviving PR, so they need a home before the topic is finished. Please don't let them go with the branch.

From test/test_backend_test_timeout_contract.py (#7517) carry over exactly one assertion, rewritten for a single-arm matrix: the constant WINDOWS_PRECEDENT_MINUTES = 40 plus the body of test_the_coverage_arm_gets_at_least_the_windows_arms_budget, re-expressed as a scalar floor -- yaml-load .github/workflows/ci.yml and assert jobs['backend-test']['timeout-minutes'] >= 40, the budget backend-test-windows already carries. Nothing in test/ pins that value today and it regressed once already (closed issue #7527), so this is the one real gap #7517 leaves behind. Drop _TERNARY, _coverage_arm(), _cap_for() and test_the_coverage_arm_is_not_capped_like_the_cheap_arm entirely: they require the two-arm python-version axis and the --cov/--no-cov ternary that #7780 removed, and all three tests currently raise AssertionError against main. Do NOT carry the 40 -> 45 value change -- it is unmeasured against 40 and second-guesses landed 7827b03. Housekeeping, not code: close issue #7516 as resolved by 7827b03 + 5119816 (its rationale comment is already in ci.yml on main) rather than leaving it open as a reason to revive the branch.

Current state

#7780 has MERGED (5119816; requires-python >=3.12, dep_sync.python_floor_breach on the reinstall branch, and python-version: ["3.12"] all verified on main). By itself it only PARTIALLY covers #7517 -- it never touches timeout-minutes (zero +/- timeout lines in its whole 29-file patch), so it does not deliver #7517's outcome. What it does is delete the two-arm python-version: ["3.10", "3.12"] axis and the --cov/--no-cov ternary, which is the sole premise of #7517's expression-valued cap and the precondition every one of its three tests reads, making them permanently red. #7517's actual outcome was delivered by a different landed commit, 7827b03 (#7527/#7552), which set timeout-minutes: 40 under a comment naming #7517's exact failure mode. Between the two landed changes the remainder is: a scalar guard test on backend-test's cap (which exists nowhere on main) and an unevidenced 40 -> 45 nudge.


From a repository-wide duplicate/overlap audit of every pull request open against main (2026-09-02, 330 PRs, one reviewer per PR). Each PR was read as its full merge-base diff plus its description and every comment and review, then compared against each candidate PR's own diff and against origin/main at 1a765b88ceb7. Findings that implied a closure were re-adjudicated independently, including an adversarial pass whose only job was to refute them; the reasoning above is what survived. If it is wrong, reopening costs nothing — please say so, and treat the reasoning rather than the outcome as the thing to correct.

@bolichen97 bolichen97 closed this Sep 2, 2026
@github-actions github-actions Bot removed the readiness: action required A blocking check or review needs attention label Sep 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fork Pull request from a fork (external contributor) merge conflict Branch has merge conflicts with its base — author must resolve before merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Backend Tests: the coverage matrix arm shares the no-cov arm 30-minute cap, so a 3.12 shard is cancelled at the wall and Coverage Gate fails closed

2 participants