Skip to content

fix: read check runs from the status rollup and page past the first 100 - #41

Open
mmalyska wants to merge 1 commit into
fix/per-name-required-check-coveragefrom
fix/paginate-check-data
Open

fix: read check runs from the status rollup and page past the first 100#41
mmalyska wants to merge 1 commit into
fix/per-name-required-check-coveragefrom
fix/paginate-check-data

Conversation

@mmalyska

Copy link
Copy Markdown

Stacked on #37 — review and merge that one first. The base of this PR is fix/per-name-required-check-coverage, so the diff here shows only the query layer. I'll retarget it to master once #37 lands.

This is the follow-up #37 asks for in its "Known limitation, deliberately not fixed here" section. #37 fixes how results are counted; this fixes what gets fetched. Identifiers are generic below — this repository is public.

What was wrong

checkSuites(first: 20) with a nested checkRuns(first: 25), both hardcoded:

  • a required check could sit outside that window and never be fetched, so "not satisfied" partly depended on where a suite happened to sort
  • because the connections were nested, the node count was commits × suites × runs

Measured: 27 check suites on a merge commit of a large monorepo, and 105–134 on the heaviest consumer — already past the 100-per-connection maximum, so widening alone cannot fix it even in principle.

What changed

Check runs come from statusCheckRollup.contexts, which is flat and returns the latest run per name — suite ordering stops deciding visibility, and re-runs arrive as one entry instead of one per attempt.

checkSuites is still selected, minus its nested runs. I checked whether the rollup could replace it outright (CheckRun.checkSuite.workflowRun.workflow.name costs 0 extra nodes, so it looked free): it cannot. On the 134-suite commit the rollup yields 4 of the 6 SUCCESS workflow names, the other two having been de-duplicated away by name collisions. A required name can also be a workflow whose jobs were all skipped, which contributes no run to the rollup at all.

It costs less than before, not more — which is where #33 and #35 got stuck:

query shape commits nodeCount charged cost
before 25 13,075 6
before 100 52,300 23
#33's shape (100 × 100) 25 252,575 26
#33's shape (100 × 100) 100 MAX_NODE_LIMIT_EXCEEDED
this PR 25 5,050 1
this PR 100 20,200 3

Two corrections to the record while we're here: #35's "~20× / 2,526 points" was nodeCount / 100, not the cost GitHub charges — the real increase was 6 → 26. And what should have blocked #33 outright is the last row: at the CLI's own default -c 100 that shape is rejected before it executes. It only ever worked because the reusable workflow passes -c 25.

Paging past 100

The caps are now --contexts-number and --check-suites-number, and the remainder is fetched by TopUpChecks. That runs from the evaluation loop rather than from hydration, because the loop already stops at the first commit that passes: no extra request in the usual case, at most one page-set per commit actually examined, and no commit judged on a partial view.

Proven by asymmetry rather than by assertion: at --check-suites-number 5 the first page of that commit contains no workflow runs at all, yet a name that can only resolve via a workflow still comes back ✔.

They default to 50, not 100, because rate limit is not the only ceiling. On the heaviest consumer a 100/100 first page makes GitHub exceed its own query execution time and return 504; at -c 50 or above it times out at every page size I tried, including 25/25 — and so does the pre-change query (502). So the limit is pre-existing rather than introduced, and both binaries succeed at -c 25, which is what the reusable workflow passes. GetCommits now names the knobs to turn when it happens.

Effect on #37's code

Evaluation reads from a different place, so collectResults takes the accumulated sources rather than one query edge, via a shared sourcesFromEdge adapter. The 42 existing cases are otherwise untouched and all still pass. Two of them set StatusCheckRollup wholesale, which now discards the runs, so they set State only — same intent, and arguably a sharper test since the red run it plants is no longer thrown away.

14 new cases cover truncation detection, a later page satisfying a name, a later page revealing a failure (so paging can't turn red into a promotion), a later suite page satisfying a workflow name, and page-size validation.

Verification

testdata/baseline/ is the harness for that comparison. Its repository list and recordings are gitignored: recorded output embeds commit messages, author names and branch names, and this repo is public.

Not in scope

Nothing here touches the acceptance rule — SUCCESS only, NEUTRAL never, SKIPPED per #37's flag. A skipped run still arrives with conclusion SKIPPED when read from the rollup, so that policy is unaffected.

Follow-up to the "Known limitation" in the parent change: the query itself, which
that change deliberately left alone.

github/types.go hardcoded checkSuites(first: 20) with a nested
checkRuns(first: 25), which had two consequences. A required check could sit
outside that window and never be fetched, and the count of what "not satisfied"
means then depended on where a suite happened to sort. And because the two
connections were nested, the query's node count was commits × suites × runs.

Check runs now come from statusCheckRollup.contexts. That connection is flat and
returns the latest run per name, so suite ordering stops deciding what is visible
and re-runs arrive as one entry. checkSuites is still selected, without its nested
runs, because a required name can be the name of a workflow whose jobs were all
skipped and which therefore contributes no run to the rollup at all. Measured on a
large-CI repository at -c 25, the query goes from 13,075 nodes to 5,050 and from 6
rate-limit points to 1 — cheaper than before the change, not more expensive, which
is what #33/#35 foundered on. Note that #35's ~20x figure was nodeCount/100 rather
than the charged cost; the real increase there was 6 -> 26, and what should have
blocked it was that its shape exceeds the 500,000-node limit outright at the CLI's
default -c 100.

Both connections cap at 100 per page and repositories exceed that, so the caps are
now variables (--contexts-number, --check-suites-number) and the remainder is
fetched by TopUpChecks. That runs from the evaluation loop rather than from
hydration, because the loop already stops at the first commit that passes: no
extra request in the usual case, at most one page-set per commit actually
examined, and no commit judged on a partial view.

The two flags default to 50 rather than 100 because rate limit is not the only
ceiling. On the heaviest repository a 100/100 first page makes GitHub exceed its
own query execution time and return 504; at -c 50 or above it times out at every
page size, as the pre-change query also does. So this is a pre-existing limit
rather than a new one, and GetCommits now names the knobs to turn when it is hit.

Where evaluation reads from moved, so collectResults takes the accumulated
sources rather than one query edge, and the test fixtures put check runs in the
rollup. The 42 existing cases are otherwise untouched and still pass; two of them
were setting StatusCheckRollup wholesale, which now discards the runs, so they set
State only.

Verified by recording the promotion verdict for all 11 consumer repositories with
the parent branch and with this one: identical across every commit examined, so
this changes how checks are fetched and nothing about how they are judged.
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.

1 participant