diff --git a/github-ci-exporter/src/collector.rs b/github-ci-exporter/src/collector.rs index 35ce5da..574ef76 100644 --- a/github-ci-exporter/src/collector.rs +++ b/github-ci-exporter/src/collector.rs @@ -591,7 +591,35 @@ fn record_runs( }) .unwrap_or_default(); + // 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) + }; + 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 + // 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 +631,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 { @@ -659,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 { @@ -1080,6 +1110,132 @@ 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 { + 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::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( + 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!( + !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 + .lines() + .any(|l| l.contains(r#"workflow="Lint""#) && l.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!(mentions_workflow(&rendered, "Deploy")); + assert!( + 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] 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..2c87fb0 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 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 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. ///