fix(pr-monitor): dedupe same-name check runs so a cancelled duplicate cannot flap the checklist - #1985
Conversation
… cannot flap the checklist A head whose workflow was re-triggered under `concurrency` carries two runs of every check: the live `completed/success` run and the cancelled duplicate (whose gate job reports a genuine `failure`). The rollup listed both, so the checklist tallied each name twice and the monitor diff reported `passed -> failed` on every poll with identical forge data. - `RollupCheck` gains `started_at` (GraphQL `CheckRun.startedAt`), selected by both merge-requirements queries and mapped by `map_rollup_context`. - `merge_requirements` collapses same-name runs onto the live one: the latest start wins; when starts are equal or unreported a state rank decides (pending > failure > success > neutral > cancelled), so a cancelled twin never shadows the run that completed. The REST check-runs fallback dedupes the same way; `required` is the OR across twins. Regression tests (RED before the fix): - `pr_ops::tests::merge_requirements_dedupes_same_name_checks_by_live_outcome` - `pr_monitor::tests::a_concurrency_cancelled_duplicate_run_does_not_flap_the_checks` Fixes intent-hq/intent#5372
panghy
left a comment
There was a problem hiding this comment.
One correctness issue found in the deterministic duplicate-run reduction; no approval submitted.
…EST started_at Review follow-ups on #1985: - The pairwise comparator switched criteria on missing start times, so it was not transitive: two timed runs plus an untimed legacy status under one name reduced to different answers depending on rollup order. Replace it with a single lexicographic `live_key` — (in-flight, started_at, state rank) — so the per-name maximum, and the checklist, is order-independent. An untimed run sorts below every timed run; a pending run wins outright. - The REST `/check-runs` fallback dropped `started_at`, so the older duplicate's failure still beat the newer success there. `CheckRun` now carries it as a `#[serde(skip)]` internal field (the documented wire shape is unchanged) and the fallback feeds it into the same reduction. - Tests: all six permutations of the mixed timed/untimed triple (completed and in-flight variants), timed REST fallback in both orders, and the monitor regression now reverses the forge order between polls on the same monitor and checks each stored snapshot.
panghy
left a comment
There was a problem hiding this comment.
Re-review: the permutation regression is fixed (36 checks passed), but the completed legacy-status policy still hides a required failure. No approval submitted.
… run Review follow-up on #1985: collapsing every same-name rollup node onto one live run let a timed successful CheckRun hide a failed StatusContext posted under the same context name. A legacy commit status is not an older attempt of the check run — GitHub requires both to pass when their shared name is required — so the reduction now keeps the two apart. - `RollupCheck` carries a daemon-internal `kind` (CheckRun | StatusContext), set by `map_rollup_context`; the REST `/check-runs` fallback is always CheckRun. `MergeRequirementSignals` is not part of the documented wire shape, and the field is serde-defaulted. - `dedupe_checks` dedupes CheckRun attempts by `live_key` as before, holds the same-name status separately, and reports the worse of the two independent outcomes (failed over pending over passed; the run on a tie so it supplies the link). The cancelled-duplicate and mixed-order behaviour is unchanged. - Tests: timed success + failing status (and the converse, pending status, in-flight run + failed status, both green) in both orders; the cancelled duplicate beside a green and a red status in all six orders; the monitor regression now forces a full fetch on every reordered poll (asserting the rollup was re-read, so the reorder assertions are not vacuous) and adds the legacy status turning red: `passed → failed` reported exactly once, in either twin order.
panghy
left a comment
There was a problem hiding this comment.
Re-review of 9ef0f5d: no remaining code findings.
Both prior reproductions pass with source-kind separation. Independently compiled the production reduction in a lightweight harness: 98 cases passed, including the prior 36 permutations, all independent run/status state pairs in both orders, required-flag OR, link selection, and superseded-run triples with green/red legacy statuses.
Also ran the existing prebuilt repository test binaries directly: all 3 focused intent-services regressions passed (dedupe, independent legacy status, actual monitor polling), and both intent-sourcecontrol mapping tests passed. The monitor test now asserts a fresh merge-requirements fetch on reordered polls rather than reusing the cheap-poll cache, and verifies the real legacy success-to-failure transition. Wire shape remains unchanged; no migrations. git diff --check passed.
Full gates were not independently rerun (host load exceeds the spec limit); current-head CI was still running at review time. This is a comment-only review, not GitHub approval or permission to merge.
🤖 Augment PR SummarySummary: This PR addresses checklist flapping caused by concurrency-cancelled duplicate CI runs. Changes:
Technical Notes: The serialized merge-requirements response remains unchanged; timestamps are used only while building its checklist. 🤖 Was this summary useful? React with 👍 or 👎 |
…heck run Review follow-up on #1985: a successful StatusContext never covers for a CheckRun of the same name whose only attempt was cancelled — the run stays the name's live run and reports failed (both orders).
Fixes intent-hq/intent#5372
Problem
A head whose workflow was re-triggered under
concurrencycarries two runs of every check instatusCheckRollup.contexts: the livecompleted/successrun and the cancelled duplicate — whose gate job (CI Gate) reports a genuinefailure, notcancelled.merge_requirementslisted both nodes, so the checklist tallied each name twice (passed: 2, failed: 1for one real check), andpr_monitordiffedpassed → failedon every poll against identical forge data — a pending-change burst and a wake per poll with nothing actually changing.Fix
RollupCheckgainsstarted_at(GraphQLCheckRun.startedAt); both merge-requirements queries select it andmap_rollup_contextmaps it (Nonefor legacyStatusContext).merge_requirementscollapses same-name runs onto the live one viadedupe_checks/supersedes:started_atwins (a re-run, or the run that superseded the cancelled duplicate);pending > failure > success > neutral > cancelled— so a cancelled twin never shadows the run that completed, and a genuine failure still beats a same-name success;requiredis the OR across twins; the first occurrence keeps its checklist position.checks_known == false) dedupes the same way.RollupCheckis not on the wire (MergeRequirementCheckis what serializes), so the new field is internal; the wire shape is unchanged.Tests (RED before the fix)
pr_ops::tests::merge_requirements_dedupes_same_name_checks_by_live_outcome— both rollup orders, timestamped supersession, untimed state-rank fallback, and the REST fallback.pr_monitor::tests::a_concurrency_cancelled_duplicate_run_does_not_flap_the_checks— registers on a duplicated rollup, polls twice: checklist reports onepassedper name, nothing pending, nopassed → failed, no wake.github::tests::maps_rollup_contexts_from_both_variantsextended forstartedAt.Gates
cargo fmt --check+cargo clippy --workspace --all-targets -- -D warnings: clean.cargo nextestonpr_ops::,pr_monitor::,intent-sourcecontrol,e2e_wss_pr_monitor: 317 passed.scripts/changed-tests.sh(changed crates vsorigin/main): see the PR Context note / follow-up comment.