diff --git a/src/hyperlight_ci/src/remote.rs b/src/hyperlight_ci/src/remote.rs index d5a44f7f6..4f8961331 100644 --- a/src/hyperlight_ci/src/remote.rs +++ b/src/hyperlight_ci/src/remote.rs @@ -58,6 +58,7 @@ struct Run { head_sha: String, #[serde(rename = "createdAt")] created_at: String, + conclusion: String, } /// Run `gh` and hand back its stdout. @@ -137,6 +138,11 @@ fn artifacts(repo: &str, run: u64) -> Result> { /// most recent runs out of the first replies. Taking one at its word picks a /// baseline months older than the one asked for, so ask until two replies /// agree on the newest run and keep everything either of them saw. +/// +/// Asking GitHub to filter is what leaves an answer cold. A listing narrowed +/// by status came back weeks stale in four of ten tries, and stably enough +/// that consecutive replies agreed on it, while the same listing unnarrowed +/// was current every time. Narrow the results here instead. fn runs(repo: &str, filter: &[&str]) -> Result> { let limit = RUNS_SEARCHED.to_string(); let mut seen: Vec = Vec::new(); @@ -151,7 +157,7 @@ fn runs(repo: &str, filter: &[&str]) -> Result> { "--limit", &limit, "--json", - "databaseId,headSha,createdAt", + "databaseId,headSha,createdAt,conclusion", ]; args.extend_from_slice(filter); @@ -227,14 +233,49 @@ pub(crate) fn run_commit(repo: &str, run: u64) -> Result { Ok(String::from_utf8_lossy(&sha).trim().to_string()) } -/// Whether `commit` is `ancestor` or was built on top of it. -fn descends_from(repo: &str, commit: &str, ancestor: &str) -> Result { +/// How many commits `commit` sits past `ancestor`, or `None` when it was not +/// built on top of it. +fn commits_between(repo: &str, commit: &str, ancestor: &str) -> Result> { + #[derive(Deserialize)] + struct Comparison { + status: String, + ahead_by: u64, + } + let path = format!("repos/{repo}/compare/{ancestor}...{commit}"); - let status = gh(&["api", &path, "--jq", ".status"])?; - Ok(matches!( - String::from_utf8_lossy(&status).trim(), - "identical" | "ahead" - )) + let comparison: Comparison = + serde_json::from_slice(&gh(&["api", &path])?).context("Failed to compare two commits")?; + + Ok(matches!(comparison.status.as_str(), "identical" | "ahead").then_some(comparison.ahead_by)) +} + +/// The newest run that measured something `commit` was built on top of, and +/// how many commits separate the two. +/// +/// `behind` says how far a run's commit sits before the one asked for, or +/// `None` when the run measured a different line of development. `kept` says +/// whether a run still has its artifacts. +fn baseline( + runs: &[Run], + behind: impl Fn(&str) -> Result>, + kept: impl Fn(u64) -> Result, +) -> Result> { + for run in runs { + // A cancelled run leaves some configurations unmeasured. + if run.conclusion != "success" { + continue; + } + + let Some(behind) = behind(&run.head_sha)? else { + continue; + }; + + if kept(run.id)? { + return Ok(Some((run, behind))); + } + } + + Ok(None) } /// The most recent benchmarks of the default branch taken at or before @@ -245,19 +286,25 @@ fn descends_from(repo: &str, commit: &str, ancestor: &str) -> Result { /// changes the commit never had. pub(crate) fn run_at(repo: &str, commit: &str) -> Result { let commit = commit_sha(repo, commit)?; - // A cancelled run leaves some configurations unmeasured. - let runs = runs( - repo, - &["--workflow", BASELINE_WORKFLOW, "--status", "success"], + let runs = runs(repo, &["--workflow", BASELINE_WORKFLOW])?; + + let found = baseline( + &runs, + |sha| commits_between(repo, &commit, sha), + |id| Ok(!artifacts(repo, id)?.is_empty()), )?; - for run in &runs { - if descends_from(repo, &commit, &run.head_sha)? && !artifacts(repo, run.id)?.is_empty() { - return Ok(run.id); - } - } + let Some((run, behind)) = found else { + bail!("No benchmarks taken at or before {commit} still have their artifacts") + }; - bail!("No benchmarks taken at or before {commit} still have their artifacts") + // How far back the branch point is says whether the baseline is the one + // meant, which a stale listing silently answers wrong. + eprintln!( + "Taking benchmarks of {}, {behind} commits back", + &run.head_sha[..12] + ); + Ok(run.id) } /// Where `pull_request` branched off the branch it targets. @@ -446,4 +493,63 @@ mod tests { "hyperlight-dev/hyperlight" ); } + + /// Newest first, as a listing arrives. + fn listing() -> Vec { + ["failure", "success", "success", "success", "success"] + .into_iter() + .enumerate() + .map(|(i, conclusion)| Run { + id: 50 - i as u64, + head_sha: format!("{:040x}", 50 - i), + created_at: format!("2026-10-{:02}T00:00:00Z", 10 - i), + conclusion: conclusion.to_string(), + }) + .collect() + } + + /// Every run measured the line of development asked about, one commit + /// further back each time, and kept its artifacts. + fn ancestral(sha: &str) -> Result> { + Ok(Some( + 50 - u64::from_str_radix(sha.trim_start_matches('0'), 16).unwrap(), + )) + } + + fn kept(_: u64) -> Result { + Ok(true) + } + + #[test] + fn passes_over_a_run_that_did_not_finish() { + let runs = listing(); + let (run, behind) = baseline(&runs, ancestral, kept).unwrap().unwrap(); + assert_eq!(run.id, 49); + assert_eq!(behind, 1); + } + + #[test] + fn passes_over_a_run_that_lost_its_artifacts() { + let runs = listing(); + let (run, _) = baseline(&runs, ancestral, |id| Ok(id < 48)) + .unwrap() + .unwrap(); + assert_eq!(run.id, 47); + } + + #[test] + fn passes_over_a_run_of_another_line_of_development() { + let runs = listing(); + let (run, behind) = baseline(&runs, |sha| Ok(ancestral(sha)?.filter(|&n| n > 2)), kept) + .unwrap() + .unwrap(); + assert_eq!(run.id, 47); + assert_eq!(behind, 3); + } + + #[test] + fn finds_nothing_to_compare_against() { + let runs = listing(); + assert!(baseline(&runs, |_| Ok(None), kept).unwrap().is_none()); + } }