Repository navigation
Key checks by content, not by push — a base update re-runs everything it cannot affect #15967
Description
Activity
Sharpening one acceptance criterion, from a case found in the queue rather than in this issue's own scope.
repo_tests/requirements_pin_parity_test.pycarries three hard-coded version literals of identical shape::138— a defect. The test's docstring states its subject is reach ("must be inside its reach, not merely near it"), and the literal over-specifies that into an exact version. Every protobuf bump failed it reporting"version mismatch", which reads as the guard working, so three dependabot PRs sat red with the cause invisible. Filed bug(test): a reach check pins a protobuf version literal, so every bump fails it #15981, fixed in fix(test): assert protobuf parity, not a version literal, in the reach check (#15981) #15982.:152and:156— deliberate. A tripwire couplingopentelemetry-proto's pin to the protobuf constraint, because the two drifted apart historically and the rationale went stale twice. That test is meant to fail on a bump.
Same shape, opposite correct verdict. The only thing separating them is a docstring recording a decision — and a sweep staged an edit to all three, stopped only by reading it.
This is a different failure from the one this issue currently describes, and stronger. Everything else logged today is output that fails to distinguish success from failure. This is code whose shape does not distinguish a defect from a deliberate constraint, and the record of the decision is the only thing that does.
That matters here specifically, because content-keyed reuse decides not to run something and then reports a verdict it did not compute. A skip that says "unchanged, not run" is exactly the artifact above: correct and indistinguishable from a skip that should have run.
Amended acceptance criterion — replacing the weaker "a republished context states that it was reused and names the run whose conclusion it carries":
- A reused or skipped verdict cites the decision that made it safe, not merely the fact that it was skipped — the filter entry or recorded rationale that establishes this check is a pure function of those paths. A reader must be able to tell a sound reuse from an unsound one without re-deriving the reasoning.
And an addition:
- Where a path set is deliberately narrow or deliberately broad, that decision is recorded next to it, so a later sweep "simplifying" it fails loudly rather than silently widening the reuse window.
Credit: found and diagnosed by
Open issues scope and solution orderwhile clearing the shard-8 failures; the near-miss on:152/:156is the half that makes the case.Sharpening the value argument, from a cost model that came out of an unrelated docs placement tonight.
This issue has been justified in suites avoided. That is the wrong unit, and the right one changes what the issue is worth.
The cost of a push is not one suite — it is whatever verdict the push destroys.
- A push that restarts a run which had not finished: cost zero. There was no verdict; nothing was lost. The compute was already going to be spent or cancelled.
- A push that restarts a run which had finished: cost one verdict. Something knowable was discarded, and the next run has to re-derive it.
Two consequences that were not obvious before:
- Mid-run is the cheapest moment to push, not the worst. Both the framing on Auto-update starves long-running PRs: every merge restarts their CI, so the busiest nights are the ones nothing old lands #16015 and my own reading here assumed the opposite — that landing something into a running suite was the expensive case. It is the free one. The expensive case is pushing onto a PR that has just gone green.
- It reframes what this issue buys. Content-keyed reuse does not avoid suites; suites will run regardless. It preserves verdicts across pushes that cannot have changed the answer. A base merge that touches none of a check's paths currently destroys a completed verdict and pays to re-derive an identical one — that is the whole loss, and it is measurable in verdicts rather than in minutes.
The measured backdrop from #16015 stands and reads differently under this unit: #15972 took 9 auto-update merges across 10 commits, #15977 8 of 9, #15978 7 of 8 — 28 restarts, ~670 check runs, zero completed suites. Zero completed is the point. Those 28 restarts destroyed almost no verdicts, because almost none had been reached; the harm was that no verdict was ever reachable, which is a starvation problem rather than a waste problem. They are different failures and need different fixes — this issue addresses the second, #15937 addresses the first.
Suggested addition to the acceptance criteria:
- The value of a reuse is stated in verdicts preserved, not suites avoided, and the measurement distinguishes a restart that destroyed a completed verdict from one that destroyed nothing
Credit: the verdict-versus-suite distinction is 46's, arrived at while deciding when to place a docs file on a mid-run PR.
Problem
A check re-runs when a push happens, not when its inputs change. Three cases pay for this today:
auto-update-pr-branchesmerges base in)The first is the measurable one. Over 24 hours: 29
auto-update-pr-branchesruns, and base-merge commits actually landed on the open PRs — #15954: 3, #15944: 2, #15959: 1, #15939: 1, #15962: 0. Seven refreshes, each restarting the applicable suite, at an 8.9-minute critical path — roughly an hour of critical path, and far more in parallel job time, for pushes that changed none of the PR's own code.Six of seven open PRs are behind base as of filing, so this is the queue's steady state, not an edge case.
Why the existing filters do not cover it
dorny/paths-filteron apull_requestevent compares the PR head against its base, so it selects on the PR diff. A base merge does not change the PR diff — it adds base's commits to the tree. The filter therefore selects exactly the same checks as before and every one of them re-runs.Nothing currently looks at the base delta (
old_base..new_base), which is the only thing that changed.Scope
Key each check by the content it depends on, rather than by the event that triggered it:
An unchanged key means an unchanged verdict, and the prior conclusion is republished instead of recomputed. This subsumes all three cases in the table above with one mechanism.
Both halves already exist and should be reused rather than rebuilt:
.github/filters/*.ymlalready define, per check, the path set it depends on. They are documented as the single source of truth for exactly this question — see the header ofcode-quality-paths.yml.*-required-context.ymlshims already publish a required context without running its job. Republishing a retained verdict is the same operation with a different reason string.The limit this trades against — decide with it in view
Path-disjointness is not semantic independence. A PR changes
f; base changes a caller offunder a different filter. The two are textually disjoint and behaviourally coupled, and a reused green is then wrong.This hole already exists — every path filter in the repo assumes a check is a pure function of its filtered paths. Content-keying does not create the assumption, it leans on it harder, and base moving 14-40 commits/day makes the coupling likelier to bite than it is on the PR side. That is a deliberate trade and belongs in the decision, not in the small print.
A reused verdict must say so
A retained green and a fresh green must not look identical. This repository has five separate mechanisms whose failure mode is a verdict indistinguishable from its opposite — #15956, #15961, #15965, #15966 and the vacuous test in #15964 — and shipping a sixth would be a poor trade for saved minutes. The republished context must state that it was reused and name the run it came from, so "checked" and "assumed unchanged" are distinguishable in the artifact a human reads.
Relationship to #15937
The merge queue removes the base-update case entirely: PRs stop being refreshed speculatively, and one suite runs at merge against the base the change actually lands on. Those seven refreshes become zero. Do #15937 first; this issue keeps earning afterwards because the docs-only and incremental-push cases survive it.
Acceptance criteria
.github/filters/*.yml, read at runtime — not a second inlined copy of the path setImplementation Order
Wave: 3
Depends on: #15937
Umbrella: #15932