From 990548fd750df342c595c0a212308663f27c73ff Mon Sep 17 00:00:00 2001 From: Fred Clausen <43556888+fredclausen@users.noreply.github.com> Date: Sun, 9 Aug 2026 21:12:27 -0600 Subject: [PATCH 1/3] fix: report only genuinely current CI state Naively taking the newest run per workflow produced 38 "failures" across the two organisations, of which fewer than half described anything actionable. Four distinct causes, each verified against the live API: - Orphaned runs. GitHub retains run history after a workflow file is deleted, so a long-removed "Update pre-commit hooks" reported a permanent failure in 10 repositories. Runs are now intersected with the repository's live workflow list. - Path-named runs. A run created before its workflow gained a `name:` reports the file path in the name field, splitting one workflow into two series. Workflow identity is now the file path, with the display name resolved from the live list. - Pull-request runs. The API's `branch=` filter matches a pull request's HEAD branch, not its base, so PR runs leaked into what is meant to be default-branch state. A merged PR's last pre-merge failure was reported as the branch's CI state indefinitely. Only push, schedule, and workflow_dispatch now count. - Stale runs. Workflows that fire only on pull_request leave a branch-state run months old; `pre-commit-checks` Lint last ran on a push in December 2025. Past 90 days a run reports conclusion="stale" and sets `workflow_run_stale`, rather than an unclearable failure. Disabled workflows are no longer filtered out. Excluding them hid a failing `update-flakes` in fred-cal that GitHub had auto-disabled for repository inactivity, which is precisely the condition worth alerting on. `workflow_enabled` now carries the state, distinguishing an inactivity auto-disable from a deliberate manual one. Net effect: 209 workflow series to 126, and 38 failures to 16 real plus 7 aged out. --- README.md | 15 + github-ci-exporter/src/collector.rs | 147 ++++++--- github-ci-exporter/src/github/rest.rs | 455 ++++++++++++++++++++++++-- github-ci-exporter/src/metrics.rs | 28 ++ 4 files changed, 580 insertions(+), 65 deletions(-) diff --git a/README.md b/README.md index 7a60b2f..65c986c 100644 --- a/README.md +++ b/README.md @@ -89,6 +89,8 @@ flag alone. | `github_repo_pulls_draft` | gauge | `org`, `repo`, `author_kind` | | `github_pull_created_timestamp_seconds` | gauge | `org`, `repo`, `number`, `author`, `author_kind`, `draft` | | `github_workflow_run_status` | gauge | `org`, `repo`, `workflow`, `event`, `conclusion` | +| `github_workflow_run_stale` | gauge | `org`, `repo`, `workflow` | +| `github_workflow_enabled` | gauge | `org`, `repo`, `workflow`, `state` | | `github_workflow_run_timestamp_seconds` | gauge | `org`, `repo`, `workflow` | | `github_workflow_last_success_timestamp_seconds` | gauge | `org`, `repo`, `workflow` | | `github_workflow_expected_interval_seconds` | gauge | `org`, `repo`, `workflow` | @@ -114,6 +116,19 @@ activity for visibility while alert rules select only `author_kind="human"`. workflow. An in-flight run reports `conclusion="running"` and does not clear a previous failure. +## What counts as the current CI state + +Getting this right matters more than it sounds. Naively taking the newest run +per workflow produced 38 "failures", of which fewer than half were real: + +| Filter | Why | +| ----------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| Workflow must still exist | GitHub keeps run history after a workflow file is deleted. A removed `Update pre-commit hooks` reported a permanent failure across 10 repositories. | +| Identity is the file path | Runs predating a workflow's `name:` report the path instead, splitting one workflow into two series. | +| Branch-state events only | The API's `branch=` filter matches a pull request's *head* branch, so PR runs leak in. A merged PR's last pre-merge failure would otherwise be the branch's CI state forever. | +| Runs age out after 90 days | Some workflows only fire on `pull_request`, leaving a branch-state run many months old. Those report `conclusion="stale"` and set `github_workflow_run_stale`, rather than an unclearable failure. | +| Disabled workflows are kept | A workflow auto-disabled by GitHub after 60 days of inactivity has stopped running silently. That is the fault worth alerting on, so it is reported rather than filtered out. | + ## Configuration TOML file, with `GHCI_`-prefixed environment overrides: diff --git a/github-ci-exporter/src/collector.rs b/github-ci-exporter/src/collector.rs index 6a01911..5b722dc 100644 --- a/github-ci-exporter/src/collector.rs +++ b/github-ci-exporter/src/collector.rs @@ -9,8 +9,8 @@ use crate::{ config::Config, github::{Client, client::RateLimitResource, graphql, rest}, metrics::{ - AuthorLabels, Metrics, PullLabels, RepoLabels, ResourceLabels, SkipLabels, WorkflowLabels, - WorkflowStateLabels, author_label, + AuthorLabels, Metrics, PullLabels, RepoLabels, ResourceLabels, SkipLabels, + WorkflowEnabledLabels, WorkflowLabels, WorkflowStateLabels, author_label, }, model::{Repo, SkipReason}, }; @@ -45,8 +45,12 @@ const fn estimate_core_requests(monitored: u64) -> u64 { pub struct WorkflowCache { /// `owner/name` -> workflow name -> expected interval in seconds. intervals: HashMap>, - /// `owner/name` -> whether the repo has any workflow files at all. - has_workflows: HashMap, + /// `owner/name` -> the workflow files currently present in the repo. + /// + /// Retained so run history can be intersected against it: GitHub keeps + /// runs of deleted workflows forever, and reporting them shows failures + /// for CI that no longer exists. + workflows: HashMap>, /// Repositories monitored by the previous cycle, used to size the /// budget pre-flight check. monitored_count: u64, @@ -139,36 +143,7 @@ pub async fn collect( let (candidates, mut skipped) = graphql::partition_repos(discovered, &|name| config.is_denylisted(name), max_age, now); - // Workflow presence. Repos with no workflow files are content-hosting - // repos with no CI signal; they are dropped so the dashboard is not - // padded with permanently-empty rows. - let mut monitored = Vec::with_capacity(candidates.len()); - for repo in candidates { - let key = repo.full_name(); - let workflows = match rest::list_workflows(client, &repo).await { - Ok(workflows) => workflows, - Err(error) => { - warn!(repo = %repo, %error, "failed to list workflows; keeping repository"); - monitored.push(repo); - continue; - } - }; - - let has_workflows = !workflows.is_empty(); - cache.has_workflows.insert(key.clone(), has_workflows); - - if !has_workflows && config.skip_repos_without_workflows { - skipped.push((repo, SkipReason::NoWorkflows)); - continue; - } - - // Resolve cron schedules once per workflow set; they change rarely. - if let std::collections::hash_map::Entry::Vacant(entry) = cache.intervals.entry(key) { - entry.insert(resolve_cron_intervals(client, &repo, &workflows).await); - } - - monitored.push(repo); - } + let monitored = resolve_monitored(client, config, cache, candidates, &mut skipped).await; info!( monitored = monitored.len(), @@ -189,8 +164,14 @@ pub async fn collect( // Actions runs, one request per repository. for repo in &monitored { - match rest::fetch_runs(client, repo).await { - Ok(runs) => record_runs(metrics, repo, &runs, cache), + let live = cache + .workflows + .get(&repo.full_name()) + .cloned() + .unwrap_or_default(); + record_workflow_states(metrics, repo, &live); + match rest::fetch_runs(client, repo, &live).await { + Ok(runs) => record_runs(metrics, repo, &runs, cache, now), Err(error) => warn!(repo = %repo, %error, "failed to fetch workflow runs"), } } @@ -281,6 +262,70 @@ fn record_repo_inventory(metrics: &Metrics, monitored: &[Repo], skipped: &[(Repo } } +/// Determines which candidates actually have CI, caching their workflow sets. +/// +/// Repositories with no workflow files are content-hosting repos with no CI +/// signal; they are dropped so the dashboard is not padded with permanently +/// empty rows. +async fn resolve_monitored( + client: &Client, + config: &Config, + cache: &mut WorkflowCache, + candidates: Vec, + skipped: &mut Vec<(Repo, SkipReason)>, +) -> Vec { + let mut monitored = Vec::with_capacity(candidates.len()); + + for repo in candidates { + let key = repo.full_name(); + let workflows = match rest::list_workflows(client, &repo).await { + Ok(workflows) => workflows, + Err(error) => { + // A listing failure is not evidence of absent CI, so the + // repository is kept rather than silently dropped. + warn!(repo = %repo, %error, "failed to list workflows; keeping repository"); + monitored.push(repo); + continue; + } + }; + + if workflows.is_empty() && config.skip_repos_without_workflows { + cache.workflows.remove(&key); + skipped.push((repo, SkipReason::NoWorkflows)); + continue; + } + cache.workflows.insert(key.clone(), workflows.clone()); + + // Cron schedules change rarely, so they are resolved once per set. + if let std::collections::hash_map::Entry::Vacant(entry) = cache.intervals.entry(key) { + entry.insert(resolve_cron_intervals(client, &repo, &workflows).await); + } + + monitored.push(repo); + } + + monitored +} + +/// Publishes whether GitHub will actually run each workflow. +/// +/// A workflow auto-disabled for inactivity has silently stopped running; the +/// `state` label lets an alert distinguish that from a deliberate manual +/// disable. +fn record_workflow_states(metrics: &Metrics, repo: &Repo, workflows: &[rest::Workflow]) { + for workflow in workflows { + metrics + .workflow_enabled + .get_or_create(&WorkflowEnabledLabels { + org: repo.owner.clone(), + repo: repo.name.clone(), + workflow: workflow.name.clone(), + state: workflow.state.as_str().to_owned(), + }) + .set(i64::from(workflow.state == rest::WorkflowState::Active)); + } +} + /// Resolves each workflow's expected run interval from its cron schedule. async fn resolve_cron_intervals( client: &Client, @@ -360,10 +405,36 @@ fn record_activity( } } -fn record_runs(metrics: &Metrics, repo: &Repo, runs: &rest::RepoRuns, cache: &WorkflowCache) { +fn record_runs( + metrics: &Metrics, + repo: &Repo, + runs: &rest::RepoRuns, + cache: &WorkflowCache, + now: DateTime, +) { let intervals = cache.intervals.get(&repo.full_name()); for run in &runs.latest { + // 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 + // cleared without an artificial push. `workflow_run_stale` carries the + // fact separately so it stays visible on the dashboard. + let stale = run.is_stale(now); + metrics + .workflow_run_stale + .get_or_create(&WorkflowLabels { + org: repo.owner.clone(), + repo: repo.name.clone(), + workflow: run.workflow.clone(), + }) + .set(i64::from(stale)); + + let conclusion = if stale { + "stale" + } else { + run.conclusion.as_str() + }; metrics .workflow_run_status .get_or_create(&WorkflowStateLabels { @@ -371,7 +442,7 @@ fn record_runs(metrics: &Metrics, repo: &Repo, runs: &rest::RepoRuns, cache: &Wo repo: repo.name.clone(), workflow: run.workflow.clone(), event: run.event.clone(), - conclusion: run.conclusion.as_str().to_owned(), + conclusion: conclusion.to_owned(), }) .set(1); diff --git a/github-ci-exporter/src/github/rest.rs b/github-ci-exporter/src/github/rest.rs index 2d50687..0484e1a 100644 --- a/github-ci-exporter/src/github/rest.rs +++ b/github-ci-exporter/src/github/rest.rs @@ -23,6 +23,15 @@ use crate::model::{Repo, RunConclusion}; /// the most recent run of every workflow for these repositories. const RUNS_PER_PAGE: usize = 100; +/// Age beyond which a run no longer describes the current code. +/// +/// Some workflows only fire on `pull_request`, so their newest branch-state +/// run can be many months old while the workflow itself is healthy. Reporting +/// that ancient conclusion produces an alert nobody can clear without an +/// artificial push. Past this age the run is marked stale and stops +/// contributing a conclusion. +pub const STALE_RUN_AGE: chrono::TimeDelta = chrono::TimeDelta::days(90); + #[derive(Debug, Deserialize)] struct WorkflowsResponse { workflows: Vec, @@ -35,6 +44,40 @@ struct WorkflowEntry { state: String, } +/// Whether GitHub will currently run a workflow. +/// +/// The distinction between the two disabled states is the point: GitHub +/// automatically disables scheduled workflows in a repository with no activity +/// for 60 days, which silently stops CI. That is a fault worth alerting on, +/// whereas a manually disabled workflow is a deliberate choice. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +pub enum WorkflowState { + Active, + /// Auto-disabled by GitHub after 60 days of repository inactivity. + DisabledInactivity, + /// Switched off by a human, or disabled because the repo is a fork. + DisabledManually, +} + +impl WorkflowState { + fn from_api(state: &str) -> Self { + match state { + "active" => Self::Active, + "disabled_inactivity" => Self::DisabledInactivity, + _ => Self::DisabledManually, + } + } + + #[must_use] + pub const fn as_str(self) -> &'static str { + match self { + Self::Active => "active", + Self::DisabledInactivity => "disabled_inactivity", + Self::DisabledManually => "disabled_manually", + } + } +} + /// A workflow definition that exists in the repository. /// /// Serialisable because this is what gets stored in the `ETag` cache, rather @@ -43,9 +86,15 @@ struct WorkflowEntry { pub struct Workflow { pub name: String, pub path: String, + pub state: WorkflowState, } -/// Lists active workflows defined by files in `.github/workflows`. +/// Lists workflows defined by files in `.github/workflows`. +/// +/// Disabled workflows are **included**, with their state, because a workflow +/// auto-disabled for inactivity still has meaningful run history and its +/// disablement is itself worth reporting. Filtering them out here caused a +/// failing `update-flakes` to disappear from the metrics entirely. /// /// GitHub also reports "dynamic" workflows (Dependabot updates, Copilot /// reviewers) that have no file in the repository. Those are excluded: they @@ -64,9 +113,10 @@ pub async fn list_workflows(client: &Client, repo: &Repo) -> Result>() @@ -83,7 +133,9 @@ struct RunsResponse { #[derive(Debug, Deserialize)] struct RunEntry { - name: Option, + // The display name is deliberately not read: it is unreliable (older runs + // report the file path instead) and the authoritative name comes from the + // live workflow list, keyed by `path`. #[serde(default)] path: Option, status: String, @@ -103,6 +155,14 @@ pub struct LatestRun { pub html_url: String, } +impl LatestRun { + /// Whether this run is too old to describe the current code. + #[must_use] + pub fn is_stale(&self, now: DateTime) -> bool { + now - self.created_at > STALE_RUN_AGE + } +} + /// Most recent run per workflow, plus the most recent *successful* run. /// /// This reduced form is what the `ETag` cache stores. The raw runs listing is @@ -118,47 +178,89 @@ pub struct RepoRuns { /// Fetches recent runs on the default branch and reduces them to the latest /// run per workflow. /// +/// `live` is the current workflow set from [`list_workflows`]; runs belonging +/// to workflows absent from it are discarded as orphaned history. +/// /// # Errors /// Returns [`ClientError`] if the request fails. -pub async fn fetch_runs(client: &Client, repo: &Repo) -> Result { +pub async fn fetch_runs( + client: &Client, + repo: &Repo, + live: &[Workflow], +) -> Result { let path = format!( "/repos/{}/{}/actions/runs?per_page={RUNS_PER_PAGE}&branch={}", repo.owner, repo.name, repo.default_branch ); let (runs, outcome) = client .get_cached(&path, |response: RunsResponse| { - reduce_runs(response.workflow_runs) + reduce_runs(response.workflow_runs, live) }) .await?; debug!(repo = %repo, ?outcome, workflows = runs.latest.len(), "fetched runs"); Ok(runs) } -/// Reduces a run list to the newest run per workflow. +/// Whether a run's trigger reflects the state of the default branch. +/// +/// Only these events describe "is the branch healthy right now": +/// +/// * `push` -- code landed on the branch. +/// * `schedule` / `workflow_dispatch` -- ran against the branch as it stands. +/// +/// `pull_request` runs are excluded even when the API's `branch=` filter +/// matches them, because that filter matches the PR's *head* branch. A merged +/// PR's last pre-merge failure would otherwise be reported as the branch's +/// current CI state forever, which was observed on both +/// `fredsystems/pre-commit-checks` and `sdr-enthusiasts/docker-planefence`. +/// Post-merge health is covered by the `push` run that merging produces. +/// +/// `dynamic` is Dependabot's generated security-update runs, each with a +/// unique name; keeping them would make cardinality unbounded. +fn is_branch_state_event(event: &str) -> bool { + matches!(event, "push" | "schedule" | "workflow_dispatch") +} + +/// Reduces a run list to the newest run per workflow, keeping only workflows +/// that still exist in the repository. /// -/// Dependabot's security-update runs are excluded: each one has a unique -/// generated name (`npm_and_yarn in /. for ...`), so keeping them would -/// produce unbounded metric cardinality. -fn reduce_runs(runs: Vec) -> RepoRuns { +/// Two classes of stale data must be discarded, both observed in the wild: +/// +/// * **Orphaned runs.** Run history outlives the workflow file. A deleted +/// `Update pre-commit hooks` workflow kept reporting its final failure +/// indefinitely -- 18 of 38 observed failures were this. Runs are therefore +/// intersected with the live workflow list. +/// * **Path-named runs.** Runs created before a workflow gained a `name:` +/// field report the file path where the name should be, so `CI` and +/// `.github/workflows/ci.yml` appear as two distinct workflows. Keying on +/// `path` and resolving the display name from the live workflow list +/// collapses them. +/// +/// Dependabot's security-update runs are excluded: each has a unique generated +/// name, which would make cardinality unbounded. +fn reduce_runs(runs: Vec, live: &[Workflow]) -> RepoRuns { + // Workflow identity is the file path, not the display name: a run created + // before the workflow gained a `name:` reports the path in the name field, + // and a renamed workflow would otherwise split into two series. + let live_by_path: HashMap<&str, &Workflow> = + live.iter().map(|w| (w.path.as_str(), w)).collect(); + let mut latest: HashMap = HashMap::new(); let mut last_success: HashMap> = HashMap::new(); for run in runs { - if run.event == "dynamic" { + if !is_branch_state_event(&run.event) { continue; } - // A run whose workflow file was deleted still appears in history; - // without a path there is no stable identity to key on. - let Some(name) = run.name.filter(|n| !n.is_empty()) else { + // Runs of a since-deleted workflow linger in history forever. Only + // workflows still present in the repository are reported. + let Some(path) = run.path.as_deref() else { continue; }; - if run - .path - .as_ref() - .is_some_and(|p| !p.starts_with(".github/workflows")) - { + let Some(workflow) = live_by_path.get(path) else { continue; - } + }; + let name = workflow.name.clone(); let conclusion = RunConclusion::from_api(&run.status, run.conclusion.as_deref()); @@ -305,16 +407,34 @@ fn parse_crons(yaml: &str) -> Vec { mod tests { use super::*; + /// A run of a workflow whose file is `.github/workflows/.yml`. fn run( name: &str, status: &str, conclusion: Option<&str>, event: &str, created: &str, + ) -> RunEntry { + run_at( + name, + &format!(".github/workflows/{name}.yml"), + status, + conclusion, + event, + created, + ) + } + + fn run_at( + name: &str, + path: &str, + status: &str, + conclusion: Option<&str>, + event: &str, + created: &str, ) -> RunEntry { RunEntry { - name: Some(name.to_owned()), - path: Some(".github/workflows/x.yaml".to_owned()), + path: Some(path.to_owned()), status: status.to_owned(), conclusion: conclusion.map(str::to_owned), event: event.to_owned(), @@ -323,6 +443,18 @@ mod tests { } } + /// Declares workflows as currently present in the repository. + fn live(names: &[&str]) -> Vec { + names + .iter() + .map(|n| Workflow { + name: (*n).to_owned(), + path: format!(".github/workflows/{n}.yml"), + state: WorkflowState::Active, + }) + .collect() + } + #[test] fn keeps_only_the_newest_run_per_workflow() { let runs = vec![ @@ -348,7 +480,7 @@ mod tests { "2026-08-05T00:00:00Z", ), ]; - let reduced = reduce_runs(runs); + let reduced = reduce_runs(runs, &live(&["CI", "Deploy"])); assert_eq!(reduced.latest.len(), 2); let ci = reduced @@ -382,7 +514,7 @@ mod tests { "2026-08-01T00:00:00Z", ), ]; - let reduced = reduce_runs(runs); + let reduced = reduce_runs(runs, &live(&["CI", "Deploy"])); assert_eq!(reduced.latest[0].conclusion, RunConclusion::Success); } @@ -404,7 +536,7 @@ mod tests { "2026-08-09T00:00:00Z", ), ]; - let reduced = reduce_runs(runs); + let reduced = reduce_runs(runs, &live(&["CI", "Deploy"])); assert_eq!(reduced.latest[0].conclusion, RunConclusion::Failure); assert_eq!( @@ -414,6 +546,63 @@ mod tests { ); } + #[test] + fn excludes_pull_request_runs() { + // Regression guard: the API's `branch=` filter matches a PR's HEAD + // branch, so PR runs leak through. A merged PR's failing pre-merge run + // was being reported as the default branch's current CI state. + let runs = vec![ + run( + "Lint", + "completed", + Some("failure"), + "pull_request", + "2026-08-03T16:51:03Z", + ), + run( + "Lint", + "completed", + Some("success"), + "push", + "2026-08-01T00:00:00Z", + ), + ]; + let reduced = reduce_runs(runs, &live(&["Lint"])); + + assert_eq!(reduced.latest.len(), 1); + assert_eq!( + reduced.latest[0].conclusion, + RunConclusion::Success, + "a newer pull_request run must not override the branch's push state" + ); + } + + #[test] + fn keeps_schedule_and_dispatch_runs() { + for event in ["push", "schedule", "workflow_dispatch"] { + let runs = vec![run( + "CI", + "completed", + Some("failure"), + event, + "2026-08-09T00:00:00Z", + )]; + let reduced = reduce_runs(runs, &live(&["CI"])); + assert_eq!(reduced.latest.len(), 1, "{event} must be kept"); + } + for event in ["pull_request", "dynamic", "pull_request_target"] { + let runs = vec![run( + "CI", + "completed", + Some("failure"), + event, + "2026-08-09T00:00:00Z", + )]; + let reduced = reduce_runs(runs, &live(&["CI"])); + assert!(reduced.latest.is_empty(), "{event} must be excluded"); + } + } + #[test] fn excludes_dependabot_dynamic_runs() { // These have unique generated names and would explode cardinality. @@ -433,7 +622,7 @@ mod tests { "2026-08-09T00:00:00Z", ), ]; - let reduced = reduce_runs(runs); + let reduced = reduce_runs(runs, &live(&["CI", "Deploy"])); assert_eq!(reduced.latest.len(), 1); assert_eq!(reduced.latest[0].workflow, "CI"); @@ -448,11 +637,223 @@ mod tests { "push", "2026-08-09T00:00:00Z", )]; - let reduced = reduce_runs(runs); + let reduced = reduce_runs(runs, &live(&["CI", "Deploy"])); assert_eq!(reduced.latest[0].conclusion, RunConclusion::Running); assert!(!reduced.latest[0].conclusion.is_failure()); } + #[test] + fn discards_runs_of_deleted_workflows() { + // Regression guard: GitHub keeps run history after a workflow file is + // removed. Observed in the wild as a long-deleted "Update pre-commit + // hooks" reporting a permanent failure -- 18 of 38 failures were this. + let runs = vec![ + run( + "CI", + "completed", + Some("success"), + "push", + "2026-08-09T00:00:00Z", + ), + run( + "Update pre-commit hooks", + "completed", + Some("failure"), + "schedule", + "2025-12-14T00:54:24Z", + ), + ]; + let reduced = reduce_runs(runs, &live(&["CI"])); + + assert_eq!(reduced.latest.len(), 1); + assert_eq!(reduced.latest[0].workflow, "CI"); + assert!( + !reduced.latest.iter().any(|r| r.conclusion.is_failure()), + "a deleted workflow must not report a failure" + ); + } + + #[test] + fn collapses_path_named_runs_onto_the_workflow_name() { + // Runs created before a workflow gained a `name:` report the file path + // in the name field. Keying on path prevents "CI" and + // ".github/workflows/ci.yml" becoming two series for one workflow. + let runs = vec![ + run_at( + ".github/workflows/CI.yml", + ".github/workflows/CI.yml", + "completed", + Some("failure"), + "push", + "2026-07-08T03:36:04Z", + ), + run( + "CI", + "completed", + Some("success"), + "push", + "2026-08-09T00:00:00Z", + ), + ]; + let reduced = reduce_runs(runs, &live(&["CI"])); + + assert_eq!(reduced.latest.len(), 1, "one workflow, one series"); + assert_eq!(reduced.latest[0].workflow, "CI"); + assert_eq!( + reduced.latest[0].conclusion, + RunConclusion::Success, + "newest run wins after collapsing" + ); + } + + #[test] + fn keeps_workflows_regardless_of_yml_or_yaml_extension() { + // Both spellings are in active use across the fleet. + let workflows = vec![ + Workflow { + name: "Deploy".to_owned(), + path: ".github/workflows/deploy.yml".to_owned(), + state: WorkflowState::Active, + }, + Workflow { + name: "Lint".to_owned(), + path: ".github/workflows/lint.yaml".to_owned(), + state: WorkflowState::Active, + }, + ]; + let runs = vec![ + run_at( + "Deploy", + ".github/workflows/deploy.yml", + "completed", + Some("success"), + "push", + "2026-08-09T00:00:00Z", + ), + run_at( + "Lint", + ".github/workflows/lint.yaml", + "completed", + Some("failure"), + "push", + "2026-08-09T00:00:00Z", + ), + ]; + let reduced = reduce_runs(runs, &workflows); + + assert_eq!(reduced.latest.len(), 2, "both extensions must be kept"); + } + + #[test] + fn disabled_workflows_still_report_their_runs() { + // Regression guard: filtering to state=="active" made a failing + // `update-flakes` vanish from the metrics after GitHub auto-disabled + // it for repository inactivity -- the exact condition worth alerting + // on was the one being hidden. + let workflows = vec![Workflow { + name: "update-flakes".to_owned(), + path: ".github/workflows/update-flakes.yaml".to_owned(), + state: WorkflowState::DisabledInactivity, + }]; + let runs = vec![run_at( + "update-flakes", + ".github/workflows/update-flakes.yaml", + "completed", + Some("failure"), + "schedule", + "2026-08-01T00:47:26Z", + )]; + let reduced = reduce_runs(runs, &workflows); + + assert_eq!(reduced.latest.len(), 1); + assert!(reduced.latest[0].conclusion.is_failure()); + } + + #[test] + fn workflow_state_maps_the_two_disabled_kinds_apart() { + assert_eq!(WorkflowState::from_api("active"), WorkflowState::Active); + assert_eq!( + WorkflowState::from_api("disabled_inactivity"), + WorkflowState::DisabledInactivity + ); + assert_eq!( + WorkflowState::from_api("disabled_manually"), + WorkflowState::DisabledManually + ); + // Unknown future states are treated as a deliberate disable rather + // than as an inactivity fault, so they cannot cause a false page. + assert_eq!( + WorkflowState::from_api("something_new"), + WorkflowState::DisabledManually + ); + } + + #[test] + fn ancient_runs_are_marked_stale() { + // Regression guard: `pre-commit-checks` Lint last ran on a push to + // main in Dec 2025 and failed; every run since has been a passing + // pull_request. Reporting that failure forever is not actionable. + let run = run( + "Lint", + "completed", + Some("failure"), + "push", + "2025-12-13T13:42:49Z", + ); + let reduced = reduce_runs(vec![run], &live(&["Lint"])); + let latest = reduced.latest.first().expect("run retained"); + + let now: DateTime = "2026-08-10T00:00:00Z".parse().expect("timestamp"); + assert!(latest.is_stale(now), "an 8-month-old run must be stale"); + } + + #[test] + fn recent_runs_are_not_stale() { + let run = run( + "CI", + "completed", + Some("failure"), + "push", + "2026-08-01T00:00:00Z", + ); + let reduced = reduce_runs(vec![run], &live(&["CI"])); + let latest = reduced.latest.first().expect("run retained"); + + let now: DateTime = "2026-08-10T00:00:00Z".parse().expect("timestamp"); + assert!(!latest.is_stale(now), "a 9-day-old failure is actionable"); + } + + #[test] + fn staleness_boundary_is_exclusive() { + let run = run( + "CI", + "completed", + Some("failure"), + "push", + "2026-05-12T00:00:00Z", + ); + let reduced = reduce_runs(vec![run], &live(&["CI"])); + let latest = reduced.latest.first().expect("run retained"); + + // Exactly 90 days is not yet stale; a second past it is. + let at_horizon = latest.created_at + STALE_RUN_AGE; + assert!(!latest.is_stale(at_horizon)); + assert!(latest.is_stale(at_horizon + chrono::TimeDelta::seconds(1))); + } + + #[test] + fn run_without_a_path_is_discarded() { + let mut orphan = run( + "CI", + "completed", + Some("failure"), + "push", + "2026-08-09T00:00:00Z", + ); + orphan.path = None; + assert!(reduce_runs(vec![orphan], &live(&["CI"])).latest.is_empty()); + } + #[test] fn parses_cron_from_workflow_yaml() { let yaml = r#" diff --git a/github-ci-exporter/src/metrics.rs b/github-ci-exporter/src/metrics.rs index 35d27db..d1ff43a 100644 --- a/github-ci-exporter/src/metrics.rs +++ b/github-ci-exporter/src/metrics.rs @@ -46,6 +46,14 @@ pub struct WorkflowLabels { pub workflow: String, } +#[derive(Clone, Debug, Hash, PartialEq, Eq, EncodeLabelSet)] +pub struct WorkflowEnabledLabels { + pub org: String, + pub repo: String, + pub workflow: String, + pub state: String, +} + #[derive(Clone, Debug, Hash, PartialEq, Eq, EncodeLabelSet)] pub struct WorkflowStateLabels { pub org: String, @@ -76,6 +84,8 @@ pub struct Metrics { pub workflow_run_timestamp: Family, pub workflow_last_success_timestamp: Family, pub workflow_expected_interval: Family, + pub workflow_enabled: Family, + pub workflow_run_stale: Family, pub repo_monitored: Family, pub repos_skipped: Family, pub rate_limit_remaining: Family, @@ -155,6 +165,20 @@ impl Metrics { workflow_expected_interval.clone(), ); + let workflow_enabled = Family::::default(); + registry.register( + "workflow_enabled", + "1 if GitHub will run this workflow; state distinguishes an inactivity auto-disable from a manual one", + workflow_enabled.clone(), + ); + + let workflow_run_stale = Family::::default(); + registry.register( + "workflow_run_stale", + "1 if the latest branch-state run is too old to describe current code", + workflow_run_stale.clone(), + ); + let repo_monitored = Family::::default(); registry.register( "repo_monitored", @@ -263,6 +287,8 @@ impl Metrics { workflow_run_timestamp, workflow_last_success_timestamp, workflow_expected_interval, + workflow_enabled, + workflow_run_stale, repo_monitored, repos_skipped, rate_limit_remaining, @@ -295,6 +321,8 @@ impl Metrics { self.workflow_run_timestamp.clear(); self.workflow_last_success_timestamp.clear(); self.workflow_expected_interval.clear(); + self.workflow_enabled.clear(); + self.workflow_run_stale.clear(); self.repo_monitored.clear(); self.repos_skipped.clear(); } From 9634586dc6051da41097ec6b402708e44c0c6e4e Mon Sep 17 00:00:00 2001 From: Fred Clausen <43556888+fredclausen@users.noreply.github.com> Date: Sun, 9 Aug 2026 21:30:07 -0600 Subject: [PATCH 2/3] fix: address review findings on CI-state accuracy Three issues raised in review, all confirmed against the code: - The projection cache could revive the orphaned-run bug. `get_cached` keys on the request URL and replays the stored reduction on 304, but the reduction also depends on the live workflow list. Deleting or renaming a workflow leaves the runs listing unchanged, so the request answers 304 and the stale reduction is served indefinitely. Cache entries for runs now include an order-independent fingerprint of the workflow set's paths and names, via a new `get_cached_as` that decouples the cache key from the request path. Warm-cache reuse is unaffected: 310 of 379 requests still answer 304. - An empty workflow set silently emptied a repository. When `list_workflows` fails and nothing is cached, every run is discarded as orphaned, so the repository publishes no series at all while still spending a request. That is the "CI vanished" appearance the budget pre-flight exists to avoid. The run fetch is now skipped with a warning. - Documented that lookup is by path while output is keyed by display name, so two workflow files sharing a `name:` collapse into one series. Accepted deliberately: it does not occur in the monitored organisations, and keying metrics by file path would make dashboards and alerts harder to read. --- github-ci-exporter/src/collector.rs | 11 ++++ github-ci-exporter/src/github/client.rs | 74 +++++++++++++++++++++- github-ci-exporter/src/github/rest.rs | 81 ++++++++++++++++++++++++- 3 files changed, 162 insertions(+), 4 deletions(-) diff --git a/github-ci-exporter/src/collector.rs b/github-ci-exporter/src/collector.rs index 5b722dc..d06d58f 100644 --- a/github-ci-exporter/src/collector.rs +++ b/github-ci-exporter/src/collector.rs @@ -169,6 +169,17 @@ pub async fn collect( .get(&repo.full_name()) .cloned() .unwrap_or_default(); + // Without a workflow set every run would be discarded as orphaned, + // publishing no series at all and making a listing failure look like + // "this repository's CI vanished". Skip the fetch rather than spend a + // request that cannot produce a result. + if live.is_empty() { + warn!( + repo = %repo, + "no workflow set available; skipping run fetch for this cycle" + ); + continue; + } record_workflow_states(metrics, repo, &live); match rest::fetch_runs(client, repo, &live).await { Ok(runs) => record_runs(metrics, repo, &runs, cache, now), diff --git a/github-ci-exporter/src/github/client.rs b/github-ci-exporter/src/github/client.rs index 264d2fe..c4070f0 100644 --- a/github-ci-exporter/src/github/client.rs +++ b/github-ci-exporter/src/github/client.rs @@ -370,6 +370,30 @@ impl Client { path: &str, project: F, ) -> Result<(R, CacheOutcome), ClientError> + where + T: DeserializeOwned, + R: Serialize + DeserializeOwned, + F: FnOnce(T) -> R, + { + self.get_cached_as(path, path, project).await + } + + /// As [`Self::get_cached`], but with the cache key decoupled from the + /// request path. + /// + /// Needed when the projection depends on inputs beyond the response body: + /// the cached value must be invalidated when those inputs change, even + /// though the request itself is unchanged. + /// + /// # Errors + /// Returns [`ClientError`] on transport failure, a non-success status, or + /// a body that does not match `T`. + pub async fn get_cached_as( + &self, + cache_key: &str, + path: &str, + project: F, + ) -> Result<(R, CacheOutcome), ClientError> where T: DeserializeOwned, R: Serialize + DeserializeOwned, @@ -380,12 +404,17 @@ impl Client { } else { format!("{}{}", self.api_url, path) }; + let cache_key = if cache_key == path { + url.clone() + } else { + cache_key.to_owned() + }; let cached_etag = self .cache .lock() .ok() - .and_then(|cache| cache.get(&url).map(|entry| entry.etag.clone())); + .and_then(|cache| cache.get(&cache_key).map(|entry| entry.etag.clone())); // A conditional request answered 304 is free, but that is only known // after the fact; the budget must be checked as though it will cost. @@ -409,7 +438,7 @@ impl Client { .cache .lock() .ok() - .and_then(|cache| cache.get(&url).map(|entry| entry.body.clone())); + .and_then(|cache| cache.get(&cache_key).map(|entry| entry.body.clone())); if let Some(cached) = cached { let value = serde_json::from_str(&cached).map_err(|source| ClientError::Decode { @@ -423,7 +452,7 @@ impl Client { // retry unconditionally. warn!(%url, "304 with no cached body; refetching unconditionally"); if let Ok(mut cache) = self.cache.lock() { - cache.remove(&url); + cache.remove(&cache_key); } let response = self.http.get(&url).send().await?; self.requests_total.fetch_add(1, Ordering::Relaxed); @@ -726,6 +755,45 @@ mod tests { ); } + #[tokio::test] + async fn distinct_cache_keys_do_not_share_an_entry() { + // A projection that depends on more than the response body must be + // invalidated when those inputs change, even though the URL is + // identical and the server would answer 304. + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/runs")) + .respond_with( + ResponseTemplate::new(200) + .insert_header("etag", "\"same\"") + .set_body_json(json!({"n": 1})), + ) + .mount(&server) + .await; + + let client = client_for(&server); + let (first, _): (String, _) = client + .get_cached_as("/runs#v1", "/runs", |v: serde_json::Value| { + format!("v1:{}", v["n"]) + }) + .await + .expect("first"); + assert_eq!(first, "v1:1"); + + // Same URL, different key: must re-project rather than replay. + let (second, outcome): (String, _) = client + .get_cached_as("/runs#v2", "/runs", |v: serde_json::Value| { + format!("v2:{}", v["n"]) + }) + .await + .expect("second"); + assert_eq!( + second, "v2:1", + "a new cache key must not replay the old projection" + ); + assert_eq!(outcome, CacheOutcome::Modified); + } + #[tokio::test] async fn records_rate_limit_headers() { let server = MockServer::start().await; diff --git a/github-ci-exporter/src/github/rest.rs b/github-ci-exporter/src/github/rest.rs index 0484e1a..4a3f933 100644 --- a/github-ci-exporter/src/github/rest.rs +++ b/github-ci-exporter/src/github/rest.rs @@ -192,8 +192,16 @@ pub async fn fetch_runs( "/repos/{}/{}/actions/runs?per_page={RUNS_PER_PAGE}&branch={}", repo.owner, repo.name, repo.default_branch ); + // The cached value is a *reduction*, and the reduction depends on `live` + // as well as on the response. Deleting or renaming a workflow does not + // change the runs listing, so the request would answer 304 and replay a + // reduction computed against the previous workflow set -- reviving the + // orphaned-run bug this reduction exists to prevent. Folding a fingerprint + // of `live` into the cache key invalidates the entry whenever the workflow + // set changes, at the cost of one uncached fetch after such a change. + let cache_key = format!("{path}#wf={}", fingerprint_workflows(live)); let (runs, outcome) = client - .get_cached(&path, |response: RunsResponse| { + .get_cached_as(&cache_key, &path, |response: RunsResponse| { reduce_runs(response.workflow_runs, live) }) .await?; @@ -201,6 +209,28 @@ pub async fn fetch_runs( Ok(runs) } +/// A stable fingerprint of the workflow set's identities and display names. +/// +/// Order-independent so an API reordering cannot spuriously invalidate the +/// cache, and it covers names as well as paths because a rename must change +/// the reduction's output labels. +fn fingerprint_workflows(live: &[Workflow]) -> u64 { + use std::hash::{Hash as _, Hasher as _}; + + let mut pairs: Vec<(&str, &str)> = live + .iter() + .map(|w| (w.path.as_str(), w.name.as_str())) + .collect(); + pairs.sort_unstable(); + + let mut hasher = std::collections::hash_map::DefaultHasher::new(); + for (path, name) in pairs { + path.hash(&mut hasher); + name.hash(&mut hasher); + } + hasher.finish() +} + /// Whether a run's trigger reflects the state of the default branch. /// /// Only these events describe "is the branch healthy right now": @@ -238,6 +268,12 @@ fn is_branch_state_event(event: &str) -> bool { /// /// Dependabot's security-update runs are excluded: each has a unique generated /// name, which would make cardinality unbounded. +/// +/// Note that while *lookup* is by path, the output is keyed by display name, +/// because that is the label operators recognise on a dashboard. Two workflow +/// files declaring the same `name:` therefore collapse into one series. That +/// is accepted: it does not occur in the monitored organisations, and keying +/// metrics by file path would make every dashboard and alert harder to read. fn reduce_runs(runs: Vec, live: &[Workflow]) -> RepoRuns { // Workflow identity is the file path, not the display name: a run created // before the workflow gained a `name:` reports the path in the name field, @@ -841,6 +877,49 @@ mod tests { assert!(latest.is_stale(at_horizon + chrono::TimeDelta::seconds(1))); } + #[test] + fn fingerprint_changes_when_a_workflow_is_deleted() { + // Regression guard: deleting a workflow does not change the runs + // listing, so the request answers 304. Without the fingerprint in the + // cache key the stale reduction is replayed and the deleted + // workflow's runs come back. + let before = live(&["CI", "Deploy"]); + let after = live(&["CI"]); + assert_ne!( + fingerprint_workflows(&before), + fingerprint_workflows(&after) + ); + } + + #[test] + fn fingerprint_changes_when_a_workflow_is_renamed() { + let before = vec![Workflow { + name: "CI".to_owned(), + path: ".github/workflows/ci.yml".to_owned(), + state: WorkflowState::Active, + }]; + let after = vec![Workflow { + name: "Build".to_owned(), + path: ".github/workflows/ci.yml".to_owned(), + state: WorkflowState::Active, + }]; + assert_ne!( + fingerprint_workflows(&before), + fingerprint_workflows(&after), + "a rename changes the output labels and must invalidate the cache" + ); + } + + #[test] + fn fingerprint_is_order_independent() { + // An API reordering must not spuriously invalidate the cache and + // force a full uncached sweep. + let a = live(&["CI", "Deploy", "Lint"]); + let mut b = a.clone(); + b.reverse(); + assert_eq!(fingerprint_workflows(&a), fingerprint_workflows(&b)); + } + #[test] fn run_without_a_path_is_discarded() { let mut orphan = run( From 3c406d6d0a11ec9b234f19d150798d7a10b1f2de Mon Sep 17 00:00:00 2001 From: Fred Clausen <43556888+fredclausen@users.noreply.github.com> Date: Sun, 9 Aug 2026 21:42:12 -0600 Subject: [PATCH 3/3] fix: write cache entries under the cache key, not the URL Entries were read by `cache_key` but written by `url`, so every `get_cached_as` caller stored under a key it would never look up. Since `fetch_runs` always uses a workflow-fingerprint key, the runs endpoint -- the largest consumer of the rate-limit budget -- refetched and reprojected on every cycle instead of revalidating. The regression was masked in testing: the warm-cache run still showed 310 of 379 requests answered 304, because the workflow-list and file-contents lookups were cached normally. Only the 62 run fetches were affected. With the write corrected, warm cycles answer 372 of 379 from cache. Adds a test asserting that a repeated custom key revalidates and yields `CacheOutcome::NotModified`, which the previous test did not cover: it checked only that two *different* keys stay separate. --- github-ci-exporter/src/github/client.rs | 53 ++++++++++++++++++++++++- 1 file changed, 52 insertions(+), 1 deletion(-) diff --git a/github-ci-exporter/src/github/client.rs b/github-ci-exporter/src/github/client.rs index c4070f0..7ec4280 100644 --- a/github-ci-exporter/src/github/client.rs +++ b/github-ci-exporter/src/github/client.rs @@ -493,7 +493,7 @@ impl Client { && let Ok(mut cache) = self.cache.lock() { cache.insert( - url, + cache_key, CacheEntry { etag, body: encoded, @@ -794,6 +794,57 @@ mod tests { assert_eq!(outcome, CacheOutcome::Modified); } + #[tokio::test] + async fn custom_cache_key_is_reused_on_repeat() { + // Regression guard: entries were read by cache key but written by URL, + // so every `get_cached_as` caller refetched and reprojected. That + // silently disabled ETag revalidation for the runs endpoint, the + // single largest consumer of the rate-limit budget. + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/runs")) + .respond_with( + ResponseTemplate::new(200) + .insert_header("etag", "\"e1\"") + .set_body_json(json!({"n": 7})), + ) + .up_to_n_times(1) + .mount(&server) + .await; + + let client = client_for(&server); + let (first, outcome): (i64, _) = client + .get_cached_as("/runs#fp=abc", "/runs", |v: serde_json::Value| { + v["n"].as_i64().unwrap_or_default() + }) + .await + .expect("first"); + assert_eq!(first, 7); + assert_eq!(outcome, CacheOutcome::Modified); + + // The same key must now revalidate and be answered from cache. + Mock::given(method("GET")) + .and(path("/runs")) + .and(header("if-none-match", "\"e1\"")) + .respond_with(ResponseTemplate::new(304)) + .expect(1) + .mount(&server) + .await; + + let (second, outcome): (i64, _) = client + .get_cached_as("/runs#fp=abc", "/runs", |v: serde_json::Value| { + v["n"].as_i64().unwrap_or_default() + }) + .await + .expect("second"); + assert_eq!(second, 7, "cached projection must be replayed"); + assert_eq!( + outcome, + CacheOutcome::NotModified, + "a repeated custom key must revalidate rather than refetch" + ); + } + #[tokio::test] async fn records_rate_limit_headers() { let server = MockServer::start().await;