Merge pull request #1331 from onlymoein/codex/fix-1303-benchmark-evidence

Reject corrupt benchmark results and incomplete regression evidence

docs/testing.md

@@ -70,3 +70,9 @@ date, load average, scenario, settings, metrics); `--compare OLD.jsonl` prints t
 the latest matching runs there, `--compare OLD --with NEW` compares two files without rendering.
 The table notes the 1-minute load average: on a busy machine wall time is noise, so compare
 peak memory and instructions retired, and rerun when the load differs a lot.
+
+Results files allow blank lines; corrupt records or invalid comparison fields report the file
+and line instead of being discarded. `--fail-over PCT` requires the same run keys on both sides,
+successful latest runs, and at least one comparable resource metric per run. Failed latest runs,
+missing coverage, empty results and wall-time-only measurements cannot pass the gate. Omit
+`--fail-over` for an exploratory partial comparison; failed latest runs remain visible.

xtask/src/bench.rs

@@ -32,7 +32,8 @@ Run it under `nice -n 10` on a busy machine; the table notes the load average of
   --with NEW.jsonl      with --compare: compare two result files, without rendering
   --fail-over PCT       with --compare: exit 1 when a matched run got more than PCT % worse in
                         CPU time, peak memory, max RSS or instructions retired (wall time is
-                        left out: it follows the machine's load)
+                        left out: it follows the machine's load). Requires matching successful
+                        latest runs and comparable resource measurements for every key
 
 Scenarios: demo (the built-in demo comp), layers (1080p, 100 solid/text/shape layers with Gaussian
 Blur, Glow, Levels, Curves, Drop Shadow and keyframes), precomps (static and animated precomps
@@ -744,7 +745,49 @@ fn append_line(path: &Path, v: &Value) -> Result<(), String> {
 
 fn read_jsonl(path: &Path) -> Result<Vec<Value>, String> {
     let text = std::fs::read_to_string(path).map_err(|e| format!("{}: {e}", path.display()))?;
-    Ok(text.lines().filter(|l| !l.trim().is_empty()).filter_map(|l| serde_json::from_str(l).ok()).collect())
+    text.lines()
+        .enumerate()
+        .filter(|(_, line)| !line.trim().is_empty())
+        .map(|(index, line)| {
+            let context = |error| format!("{}:{}: {error}", path.display(), index.saturating_add(1));
+            let row: Value = serde_json::from_str(line).map_err(|error| context(format!("invalid JSON: {error}")))?;
+            validate_row(&row).map_err(context)?;
+            Ok(row)
+        })
+        .collect()
+}
+
+/// Validate comparison keys and measurements; unavailable platform metrics may be null/absent.
+fn validate_row(row: &Value) -> Result<(), String> {
+    for (field, value) in [
+        ("scenario", &row["scenario"]),
+        ("run", &row["run"]),
+        ("settings.resolution", &row["settings"]["resolution"]),
+        ("settings.format", &row["settings"]["format"]),
+    ] {
+        if value.as_str().is_none_or(|text| text.trim().is_empty()) {
+            return Err(format!("`{field}` must be a nonempty string"));
+        }
+    }
+    if row["settings"]["frames"].as_u64().is_none() {
+        return Err("`settings.frames` must be a nonnegative integer".into());
+    }
+    if row["ok"].as_bool().is_none() {
+        return Err("`ok` must be a boolean".into());
+    }
+    if !row["metrics"].is_object() {
+        return Err("`metrics` must be an object".into());
+    }
+    if row["metrics"]["wall_s"].is_null() {
+        return Err("`metrics.wall_s` is required".into());
+    }
+    for (field, _) in COMPARED {
+        let value = &row["metrics"][*field];
+        if !value.is_null() && value.as_f64().is_none_or(|number| !number.is_finite() || number < 0.0) {
+            return Err(format!("`metrics.{field}` must be a finite nonnegative number or null"));
+        }
+    }
+    Ok(())
 }
 
 // ---------------------------------------------------------------------------------------- output
@@ -816,10 +859,10 @@ fn key_of(v: &Value) -> String {
     )
 }
 
-/// The last successful result per key (a results file accumulates runs; the latest one counts).
+/// The latest result per key, including failures that supersede older successful runs.
 fn latest_per_key(rows: &[Value]) -> BTreeMap<String, Value> {
     let mut map = BTreeMap::new();
-    for r in rows.iter().filter(|r| r["ok"] == true) {
+    for r in rows {
         map.insert(key_of(r), r.clone());
     }
     map
@@ -857,6 +900,31 @@ fn regressions(old: &BTreeMap<String, Value>, new: &BTreeMap<String, Value>, lim
 
 fn check_regressions(old: &BTreeMap<String, Value>, new: &BTreeMap<String, Value>, limit: Option<f64>) -> Result<(), String> {
     let Some(limit) = limit else { return Ok(()) };
+    if old.is_empty() || new.is_empty() {
+        return Err("insufficient benchmark evidence: baseline and candidate must contain runs".into());
+    }
+    let mut missing = vec![];
+    for (key, baseline) in old {
+        match new.get(key) {
+            None => missing.push(format!("missing candidate run: {key}")),
+            Some(candidate) => {
+                for (label, row) in [("baseline", baseline), ("candidate", candidate)] {
+                    if row["ok"] != true {
+                        missing.push(format!("failed latest {label} run: {key}"));
+                    }
+                }
+                if !GATED.iter().any(|field| pct(baseline["metrics"][*field].as_f64(), candidate["metrics"][*field].as_f64()).is_some()) {
+                    missing.push(format!("no comparable resource measurements: {key}"));
+                }
+            }
+        }
+    }
+    for key in new.keys().filter(|key| !old.contains_key(*key)) {
+        missing.push(format!("missing baseline run: {key}"));
+    }
+    if !missing.is_empty() {
+        return Err(format!("insufficient benchmark evidence:\n  {}", missing.join("\n  ")));
+    }
     let bad = regressions(old, new, limit);
     if bad.is_empty() {
         println!("no run got more than {limit}% worse");
@@ -883,6 +951,10 @@ fn compare_table(old: &BTreeMap<String, Value>, new: &BTreeMap<String, Value>) -
             n["run"].as_str().unwrap_or(""),
             n["settings"]["frames"].as_u64().unwrap_or(0)
         ));
+        if o["ok"] != true || n["ok"] != true {
+            s.push_str(&format!(" (failed latest {} run)\n", if n["ok"] != true { "candidate" } else { "baseline" }));
+            continue;
+        }
         for (field, _) in COMPARED {
             s.push_str(&format!(" {:>8}", fmt_pct(pct(o["metrics"][field].as_f64(), n["metrics"][field].as_f64()))));
         }
@@ -963,11 +1035,11 @@ mod tests {
         assert_eq!(fmt_pct(Some(25.0)), "+25.0%");
         assert_eq!(fmt_pct(None), "-");
 
-        // The latest successful run per key is the baseline; failed runs don't count.
+        // A successful run supersedes an earlier failure for the same key.
         let old = latest_per_key(&[
+            row("layers", "full", 1.0, None, false),
             row("layers", "full", 20.0, Some(1000), true),
             row("layers", "full", 10.0, Some(2000), true),
-            row("layers", "full", 1.0, None, false),
         ]);
         assert_eq!(old.len(), 1);
         let new = latest_per_key(&[row("layers", "full", 8.0, Some(1500), true), row("demo", "half", 1.0, None, true)]);

xtask/tests/bench.rs

@@ -0,0 +1,134 @@
+//! Regression gates must fail when benchmark evidence is corrupt or incomplete.
+use serde_json::{Value, json};
+use std::path::PathBuf;
+use std::process::{Command, Output};
+
+struct ResultsDir(PathBuf);
+impl ResultsDir {
+    fn new() -> Self {
+        use std::sync::atomic::{AtomicU64, Ordering};
+        static NEXT: AtomicU64 = AtomicU64::new(0);
+        let dir = std::env::temp_dir().join(format!("effectcraft-bench-{}-{}", std::process::id(), NEXT.fetch_add(1, Ordering::Relaxed)));
+        std::fs::create_dir_all(&dir).unwrap();
+        Self(dir)
+    }
+    fn compare(&self, old: &str, new: &str, gate: bool) -> Output {
+        let baseline = self.0.join("baseline.jsonl");
+        let candidate = self.0.join("candidate.jsonl");
+        std::fs::write(&baseline, old).unwrap();
+        std::fs::write(&candidate, new).unwrap();
+        let mut command = Command::new(env!("CARGO_BIN_EXE_xtask"));
+        command.args(["bench", "--compare"]).arg(baseline).arg("--with").arg(candidate);
+        if gate {
+            command.args(["--fail-over", "5"]);
+        }
+        command.output().unwrap()
+    }
+}
+impl Drop for ResultsDir {
+    fn drop(&mut self) {
+        let _ = std::fs::remove_dir_all(&self.0);
+    }
+}
+
+fn row(scenario: &str, ok: bool) -> Value {
+    json!({"scenario": scenario, "run": "full", "ok": ok,
+        "settings": {"resolution": "full", "frames": 60, "format": "png"},
+        "metrics": {"wall_s": 1.0, "cpu_s": 2.0, "peak_footprint_bytes": 1000, "max_rss_bytes": null, "instructions": null}})
+}
+fn diagnostic(output: &Output) -> String {
+    format!("{}{}", String::from_utf8_lossy(&output.stdout), String::from_utf8_lossy(&output.stderr))
+}
+
+#[test]
+fn corrupt_records_report_the_input_file_and_line() {
+    let files = ResultsDir::new();
+    let valid = row("demo", true);
+    let mut missing = valid.clone();
+    missing["settings"].as_object_mut().unwrap().remove("frames");
+    let mut missing_wall = valid.clone();
+    missing_wall["metrics"].as_object_mut().unwrap().remove("wall_s");
+    let mut bad_metric = valid.clone();
+    bad_metric["metrics"]["cpu_s"] = json!("not a measurement");
+    let mut negative_metric = valid.clone();
+    negative_metric["metrics"]["cpu_s"] = json!(-1);
+    for bad in [
+        "{".into(),
+        "null".into(),
+        "[]".into(),
+        "{}".into(),
+        missing.to_string(),
+        missing_wall.to_string(),
+        bad_metric.to_string(),
+        negative_metric.to_string(),
+    ] {
+        let output = files.compare(&format!("{valid}\n"), &format!("{valid}\n\n \n{bad}\n"), true);
+        let message = diagnostic(&output);
+        assert!(!output.status.success(), "corrupt input passed: {bad}\n{message}");
+        assert!(message.contains("candidate.jsonl:4:"), "{message}");
+    }
+    let output = files.compare("{\n", &format!("{valid}\n"), false);
+    assert!(!output.status.success());
+    assert!(diagnostic(&output).contains("baseline.jsonl:1:"));
+}
+
+#[test]
+fn gates_reject_failed_latest_runs_and_missing_coverage() {
+    let files = ResultsDir::new();
+    let good = row("demo", true);
+    let failed = row("demo", false);
+    let other = row("layers", true);
+    for (old, new) in [
+        (good.to_string(), String::new()),
+        (String::new(), String::new()),
+        (good.to_string(), failed.to_string()),
+        (good.to_string(), format!("{good}\n{failed}")),
+        (format!("{good}\n{failed}"), good.to_string()),
+        (good.to_string(), other.to_string()),
+        (format!("{good}\n{other}"), good.to_string()),
+        (good.to_string(), format!("{good}\n{other}")),
+    ] {
+        let output = files.compare(&old, &new, true);
+        assert!(!output.status.success(), "incomplete gate passed: {old} -> {new}\n{}", diagnostic(&output));
+    }
+}
+
+#[test]
+fn gates_require_a_comparable_resource_metric_for_every_run() {
+    let files = ResultsDir::new();
+    let good = row("demo", true);
+    let mut wall_only = good.clone();
+    wall_only["metrics"]["cpu_s"] = Value::Null;
+    wall_only["metrics"]["peak_footprint_bytes"] = Value::Null;
+    let output = files.compare(&good.to_string(), &wall_only.to_string(), true);
+    assert!(!output.status.success(), "{}", diagnostic(&output));
+    assert!(diagnostic(&output).contains("comparable"));
+    let mut zero = good.clone();
+    zero["metrics"]["cpu_s"] = json!(0);
+    zero["metrics"]["peak_footprint_bytes"] = json!(0);
+    assert!(!files.compare(&zero.to_string(), &good.to_string(), true).status.success());
+    let mut mixed = row("layers", true);
+    mixed["metrics"] = wall_only["metrics"].clone();
+    assert!(!files.compare(&format!("{good}\n{mixed}"), &format!("{good}\n{mixed}"), true).status.success());
+}
+
+#[test]
+fn valid_results_keep_comparison_and_exploratory_mode() {
+    let files = ResultsDir::new();
+    let good = row("demo", true);
+    let failed = row("demo", false);
+    let output = files.compare(&format!("\n{failed}\n{good}\n"), &format!("{good}\n \n"), true);
+    assert!(output.status.success(), "{}", diagnostic(&output));
+    assert!(diagnostic(&output).contains("+0.0%"));
+    let mut open = good.clone();
+    open["run"] = json!("open");
+    open["settings"]["resolution"] = json!("-");
+    open["settings"]["frames"] = json!(0);
+    assert!(files.compare(&open.to_string(), &open.to_string(), true).status.success(), "project-open runs have zero rendered frames");
+    let mut regression = good.clone();
+    regression["metrics"]["cpu_s"] = json!(3);
+    assert!(!files.compare(&good.to_string(), &regression.to_string(), true).status.success());
+    let output = files.compare(&good.to_string(), &format!("{good}\n{failed}"), false);
+    assert!(output.status.success(), "exploratory comparison stays available");
+    assert!(diagnostic(&output).contains("failed"), "latest failure is visible: {}", diagnostic(&output));
+}