From 1c7dbfea929c0a39de41b8d50b73439faf068165 Mon Sep 17 00:00:00 2001 From: Fred Clausen <43556888+fredclausen@users.noreply.github.com> Date: Tue, 11 Aug 2026 17:14:48 -0600 Subject: [PATCH 1/2] fix: discard run reductions computed before the trigger filter Reached production. `sdr-e-base-repo-setup`'s `Lint` -- the PR-only workflow the trigger filter was written to drop -- came back as `conclusion="failure"` and put GitHubCIFailingDefaultBranch into pending. Worse than before the filter existed, because it previously showed as the harmless `stale`. `fetch_runs` caches a *reduction*, keyed by a fingerprint of each workflow's path, name, and triggers. Teaching `reduce_runs` to drop workflows with no default-branch state changed what it produces without changing any of those, so the key was unchanged, the persisted file's version still matched, and every entry still deserialised cleanly. GitHub answered 304 and the exporter replayed a pre-filter reduction containing the run it would now discard. `record_runs` then published it, and since such a workflow is no longer masked as stale, published it as an outright failure. Both existing safeguards missed it because both key on decodability: the version check and the per-entry decode fallback catch a projection whose *shape* changed, and this one's shape was byte-identical while its meaning was not. `CACHE_FORMAT_VERSION` is bumped to 2 and its documentation now says to bump it for a change in meaning as well as in shape, with a version history, since that distinction is what made this invisible. The filter is also enforced in `record_runs`, not only in `reduce_runs`. Reaching that point means the reduction did not come from the current code, and a cached projection is the way that happens. Checking at publication too makes the rule hold regardless of a reduction's provenance, which is the difference between a stale cache costing one sweep of accuracy and it publishing a fossil failure that pages. Note that clearing the cache is sufficient to recover a running deployment; verified that the released binary filters the workflow correctly on a cold cache, and that the pre-filter binary is what writes the poisoned entry. The publication guard is mutation-tested: removed, the new test fails on the exact production symptom. Its companion asserts a cadenced workflow's ancient failure is still published, masked as stale, so the guard stays surgical. --- github-ci-exporter/src/collector.rs | 127 ++++++++++++++++++++++-- github-ci-exporter/src/github/client.rs | 22 +++- 2 files changed, 142 insertions(+), 7 deletions(-) diff --git a/github-ci-exporter/src/collector.rs b/github-ci-exporter/src/collector.rs index 35ce5da..96837ad 100644 --- a/github-ci-exporter/src/collector.rs +++ b/github-ci-exporter/src/collector.rs @@ -592,6 +592,27 @@ fn record_runs( .unwrap_or_default(); for run in &runs.latest { + let signal = signal_for_name + .get(run.workflow.as_str()) + .copied() + .unwrap_or(rest::DefaultBranchSignal::Cadenced); + + // `reduce_runs` already drops these, so reaching here means the + // reduction did not come from the current code -- in practice, a + // cached projection replayed on a `304`. Enforcing the rule at the + // point of publication as well makes it hold regardless of a + // reduction's provenance, which is the difference between a stale + // cache costing one sweep of accuracy and it publishing a fossil + // failure that pages. + if signal == rest::DefaultBranchSignal::None { + debug!( + repo = %repo, + workflow = run.workflow, + "discarding a run for a workflow with no default-branch state" + ); + continue; + } + // A run older than the staleness horizon says nothing about the // current code. Reporting `stale` instead of its original conclusion // keeps an ancient failure from producing an alert that cannot be @@ -603,12 +624,7 @@ fn record_runs( // fault and must not mask what that run actually concluded -- that is // what reported every `Deploy` the fleet had not released in three // months as though its CI had gone quiet. - let cadenced = signal_for_name - .get(run.workflow.as_str()) - .copied() - .unwrap_or(rest::DefaultBranchSignal::Cadenced) - == rest::DefaultBranchSignal::Cadenced; - let stale = cadenced && run.is_stale(now); + let stale = signal == rest::DefaultBranchSignal::Cadenced && run.is_stale(now); metrics .workflow_run_stale .get_or_create(&WorkflowLabels { @@ -1080,6 +1096,105 @@ mod tests { ); } + /// A cache entry from before `reduce_runs` learned to drop PR-only + /// workflows: the run is present in the reduction even though the current + /// reducer would never emit it. + fn replayed_reduction_containing(workflow: &str) -> rest::RepoRuns { + rest::RepoRuns { + latest: vec![rest::LatestRun { + workflow: workflow.to_owned(), + conclusion: crate::model::RunConclusion::Failure, + event: "workflow_dispatch".to_owned(), + created_at: "2025-12-13T15:55:22Z".parse().expect("timestamp"), + html_url: "https://github.com/o/r/actions/runs/1".to_owned(), + }], + last_success: HashMap::new(), + } + } + + fn cache_with_workflow(repo: &Repo, name: &str, triggers: &[&str]) -> WorkflowCache { + let mut cache = WorkflowCache::default(); + cache.workflows.insert( + repo.full_name(), + vec![rest::Workflow { + name: name.to_owned(), + path: format!(".github/workflows/{name}.yml"), + state: rest::WorkflowState::Active, + triggers: triggers.iter().map(|t| (*t).to_owned()).collect(), + }], + ); + cache + } + + #[test] + fn a_replayed_reduction_cannot_resurrect_a_pr_only_workflow() { + // The regression that reached production. The runs cache stores a + // *reduction*, keyed by a fingerprint of path, name, and triggers -- + // none of which changed when the reducer learned to drop workflows + // with no default-branch state. So `304` replayed a pre-filter + // reduction, the fossil run came back, and because such a workflow is + // no longer masked as stale it published as an outright `failure`. + // Bumping the cache version fixes the cause; this asserts the rule + // holds at publication regardless of a reduction's provenance. + let (metrics, registry) = Metrics::new(); + let repo = Repo { + owner: "sdr-enthusiasts".to_owned(), + name: "sdr-e-base-repo-setup".to_owned(), + default_branch: "main".to_owned(), + }; + let cache = cache_with_workflow( + &repo, + "Lint", + &["merge_group", "pull_request", "workflow_dispatch"], + ); + + record_runs( + &metrics, + &repo, + &replayed_reduction_containing("Lint"), + &cache, + Utc::now(), + ); + + let rendered = Publisher::new(metrics, registry).render(); + assert!( + !rendered.contains(r#"workflow="Lint""#), + "a PR-only workflow must publish nothing even from a stale cache:\n{rendered}" + ); + assert!( + !rendered.contains(r#"conclusion="failure""#), + "and must certainly not publish a pageable failure:\n{rendered}" + ); + } + + #[test] + fn a_replayed_reduction_still_publishes_a_cadenced_workflow() { + // The guard must be surgical: a cadenced workflow's old failure is + // real history and still belongs in the output, masked as stale. + let (metrics, registry) = Metrics::new(); + let repo = Repo { + owner: "sdr-enthusiasts".to_owned(), + name: "docker-jaero".to_owned(), + default_branch: "main".to_owned(), + }; + let cache = cache_with_workflow(&repo, "Deploy", &["push"]); + + record_runs( + &metrics, + &repo, + &replayed_reduction_containing("Deploy"), + &cache, + Utc::now(), + ); + + let rendered = Publisher::new(metrics, registry).render(); + assert!(rendered.contains(r#"workflow="Deploy""#)); + assert!( + rendered.contains(r#"conclusion="stale""#), + "an ancient cadenced failure is masked, not dropped:\n{rendered}" + ); + } + #[test] fn a_failure_clears_a_stale_budget_bypass_flag() { // Only non-budget errors reach record_failure -- a bypassed cycle diff --git a/github-ci-exporter/src/github/client.rs b/github-ci-exporter/src/github/client.rs index 5dbfb46..45a5f0a 100644 --- a/github-ci-exporter/src/github/client.rs +++ b/github-ci-exporter/src/github/client.rs @@ -150,7 +150,27 @@ struct CacheEntry { /// Bump this whenever any cached projection's serialised shape changes. The /// per-entry decode fallback in [`Client::get_cached_as`] recovers from a /// missed bump, at the cost of one uncached fetch per affected entry. -const CACHE_FORMAT_VERSION: u32 = 1; +/// +/// **Bump it when the projection's _meaning_ changes too, not only its +/// shape.** Both existing safeguards key on decodability, so a reduction that +/// still deserialises cleanly but is now computed differently sails through +/// them: the version matches, every entry decodes, and `304` replays a value +/// the current code would never have produced. +/// +/// That is also not hypothetical. Version 1 held run reductions computed +/// before `reduce_runs` learned to discard workflows with no default-branch +/// trigger. The shape was byte-identical, so the file was accepted and the +/// filtered-out runs came straight back -- resurrecting the exact fossil run +/// the filter existed to remove, and, because such a workflow is no longer +/// masked as stale, publishing it as an outright `failure` that would page. +/// Worse than before the filter was added. +/// +/// Version history: +/// +/// * 1 -- initial versioned format. +/// * 2 -- `reduce_runs` drops workflows whose triggers give them no +/// default-branch state. Same shape, different contents. +const CACHE_FORMAT_VERSION: u32 = 2; /// Versioned envelope for the persisted cache, as read from disk. /// From dc8882dee4d34b81072bb1ecbae6a2d3c5ffd578 Mon Sep 17 00:00:00 2001 From: Fred Clausen <43556888+fredclausen@users.noreply.github.com> Date: Tue, 11 Aug 2026 17:22:23 -0600 Subject: [PATCH 2/2] fix: gate last_success by default-branch signal too Review findings, all three valid. The publication guard covered `runs.latest` but not `runs.last_success`, which a separate loop publishes. A replayed pre-filter reduction populates both, so a PR-only workflow could still emit `workflow_last_success_timestamp` -- asserting that it last passed on the default branch, which is precisely the claim being retracted. No alert or dashboard reads that series today, so the practical effect was nil, but the guard's whole premise is that the rule holds regardless of a reduction's provenance, and it did not. The signal lookup is now a single closure used by both loops, so the two cannot drift apart again. The replay fixture carries a `last_success` entry, without which the new path was untested; confirmed by mutation that removing only that guard fails the test. The PR-only assertion now checks line-wise that no sample mentions the workflow at all, rather than searching the whole document for `conclusion="failure"`, which would have been brittle against any unrelated series carrying that label. The cadenced companion gained an assertion that its last-success timestamp is still published, so the new gate cannot silently over-reach. Version-history wording now names `DefaultBranchSignal::None` rather than "no default-branch trigger", matching the type it describes. --- github-ci-exporter/src/collector.rs | 61 +++++++++++++++++++++---- github-ci-exporter/src/github/client.rs | 16 +++---- 2 files changed, 59 insertions(+), 18 deletions(-) diff --git a/github-ci-exporter/src/collector.rs b/github-ci-exporter/src/collector.rs index 96837ad..574ef76 100644 --- a/github-ci-exporter/src/collector.rs +++ b/github-ci-exporter/src/collector.rs @@ -591,11 +591,18 @@ fn record_runs( }) .unwrap_or_default(); - for run in &runs.latest { - let signal = signal_for_name - .get(run.workflow.as_str()) + // Unknown workflows default to cadenced, matching how an unresolved + // trigger list is treated everywhere else: permissive, so a failed + // definition lookup cannot blank a repository. + let signal_of = |workflow: &str| { + signal_for_name + .get(workflow) .copied() - .unwrap_or(rest::DefaultBranchSignal::Cadenced); + .unwrap_or(rest::DefaultBranchSignal::Cadenced) + }; + + for run in &runs.latest { + let signal = signal_of(&run.workflow); // `reduce_runs` already drops these, so reaching here means the // reduction did not come from the current code -- in practice, a @@ -675,6 +682,13 @@ fn record_runs( } for (workflow, at) in &runs.last_success { + // Same gate as above, and for the same reason: a replayed pre-filter + // reduction carries `last_success` entries too, and publishing "this + // workflow last passed on the default branch" for one that never runs + // against the default branch is exactly the claim being retracted. + if signal_of(workflow) == rest::DefaultBranchSignal::None { + continue; + } metrics .workflow_last_success_timestamp .get_or_create(&WorkflowLabels { @@ -1099,6 +1113,9 @@ mod tests { /// A cache entry from before `reduce_runs` learned to drop PR-only /// workflows: the run is present in the reduction even though the current /// reducer would never emit it. + /// + /// Carries a `last_success` entry as well, because that map is published + /// by a separate loop and a replayed reduction populates both. fn replayed_reduction_containing(workflow: &str) -> rest::RepoRuns { rest::RepoRuns { latest: vec![rest::LatestRun { @@ -1108,10 +1125,24 @@ mod tests { created_at: "2025-12-13T15:55:22Z".parse().expect("timestamp"), html_url: "https://github.com/o/r/actions/runs/1".to_owned(), }], - last_success: HashMap::new(), + last_success: HashMap::from([( + workflow.to_owned(), + "2025-12-01T00:00:00Z".parse().expect("timestamp"), + )]), } } + /// Whether any rendered sample mentions `workflow`. + /// + /// Line-wise rather than a substring search over the whole document, so an + /// unrelated series carrying the same conclusion cannot make an assertion + /// pass or fail by accident. + fn mentions_workflow(rendered: &str, workflow: &str) -> bool { + rendered + .lines() + .any(|line| line.contains(&format!(r#"workflow="{workflow}""#))) + } + fn cache_with_workflow(repo: &Repo, name: &str, triggers: &[&str]) -> WorkflowCache { let mut cache = WorkflowCache::default(); cache.workflows.insert( @@ -1158,11 +1189,15 @@ mod tests { let rendered = Publisher::new(metrics, registry).render(); assert!( - !rendered.contains(r#"workflow="Lint""#), - "a PR-only workflow must publish nothing even from a stale cache:\n{rendered}" + !mentions_workflow(&rendered, "Lint"), + "a PR-only workflow must publish nothing from a stale cache -- not a \ + run status, not a stale flag, and not a last-success timestamp:\n{rendered}" ); + // Named explicitly, because this is the series that paged. assert!( - !rendered.contains(r#"conclusion="failure""#), + !rendered + .lines() + .any(|l| l.contains(r#"workflow="Lint""#) && l.contains(r#"conclusion="failure""#)), "and must certainly not publish a pageable failure:\n{rendered}" ); } @@ -1188,11 +1223,17 @@ mod tests { ); let rendered = Publisher::new(metrics, registry).render(); - assert!(rendered.contains(r#"workflow="Deploy""#)); + assert!(mentions_workflow(&rendered, "Deploy")); assert!( - rendered.contains(r#"conclusion="stale""#), + rendered + .lines() + .any(|l| l.contains(r#"workflow="Deploy""#) && l.contains(r#"conclusion="stale""#)), "an ancient cadenced failure is masked, not dropped:\n{rendered}" ); + assert!( + rendered.contains("github_workflow_last_success_timestamp_seconds"), + "and its last-success timestamp is still published:\n{rendered}" + ); } #[test] diff --git a/github-ci-exporter/src/github/client.rs b/github-ci-exporter/src/github/client.rs index 45a5f0a..2c87fb0 100644 --- a/github-ci-exporter/src/github/client.rs +++ b/github-ci-exporter/src/github/client.rs @@ -158,18 +158,18 @@ struct CacheEntry { /// the current code would never have produced. /// /// That is also not hypothetical. Version 1 held run reductions computed -/// before `reduce_runs` learned to discard workflows with no default-branch -/// trigger. The shape was byte-identical, so the file was accepted and the -/// filtered-out runs came straight back -- resurrecting the exact fossil run -/// the filter existed to remove, and, because such a workflow is no longer -/// masked as stale, publishing it as an outright `failure` that would page. -/// Worse than before the filter was added. +/// before `reduce_runs` learned to discard workflows classified +/// `DefaultBranchSignal::None`. The shape was byte-identical, so the file was +/// accepted and the filtered-out runs came straight back -- resurrecting the +/// exact fossil run the filter existed to remove, and, because such a workflow +/// is no longer masked as stale, publishing it as an outright `failure` that +/// would page. Worse than before the filter was added. /// /// Version history: /// /// * 1 -- initial versioned format. -/// * 2 -- `reduce_runs` drops workflows whose triggers give them no -/// default-branch state. Same shape, different contents. +/// * 2 -- `reduce_runs` drops workflows whose declared triggers make them +/// `DefaultBranchSignal::None`. Same shape, different contents. const CACHE_FORMAT_VERSION: u32 = 2; /// Versioned envelope for the persisted cache, as read from disk.