mirror of
https://github.com/pbakaus/impeccable.git
synced 2026-09-12 06:06:37 +03:00
Fix quarantine of comparison evidence after cleanup errors
Retain unremovable crops under explicit invalid-evidence names in the same parent. Record quarantine paths and filesystem failures while keeping the gate closed. AI assistance: implemented and validated with OpenAI Codex under maintainer direction.
This commit is contained in:
@@ -1110,16 +1110,67 @@ fn atomic_report(path: &Path, report: &Value) -> Result<(), String> {
|
||||
}
|
||||
|
||||
fn unavailable_report(io: &Io, out_dir: &str, gate: &Gate, phase: &str) -> Result<(), String> {
|
||||
let report = json!({ "interpretation": format!("{phase}-gate"), "measurementsAvailable": false,
|
||||
let mut report = json!({ "interpretation": format!("{phase}-gate"), "measurementsAvailable": false,
|
||||
"regions": [], "gate": { "ok": false, "reasons": gate.reasons,
|
||||
"advisories": gate.advisories, "unscopedReasons": gate.reasons } });
|
||||
// Invalidate the report before touching images; also clean partial writes on failure.
|
||||
// Attempt cleanup even when the report itself cannot be replaced.
|
||||
let report_write = atomic_report(&abs(io, &format!("{out_dir}/report.json")), &report);
|
||||
let cleanup = clear_comparison_artifacts(&abs(io, out_dir));
|
||||
if let Err(error) = &cleanup {
|
||||
// A read-only regions directory may prevent unlinking its children even
|
||||
// when its parent permits moving the directory. Preserve those bytes as
|
||||
// explicitly invalid evidence instead of leaving them at current paths.
|
||||
report["artifactCleanup"] = json!({"status":"failed", "error":error,
|
||||
"invalidArtifacts":["raw-report.json", "side-by-side.png", "heatmap.png", "regions/"]});
|
||||
match quarantine_comparison_artifacts(&abs(io, out_dir)) {
|
||||
Ok(record) => report["artifactCleanup"]["quarantine"] = record,
|
||||
Err(e) => report["artifactCleanup"]["quarantineError"] = json!(e),
|
||||
}
|
||||
// If the filesystem also refuses quarantine, the report explicitly marks
|
||||
// every potentially remaining artifact invalid. The gate still fails.
|
||||
atomic_report(&abs(io, &format!("{out_dir}/report.json")), &report)?;
|
||||
}
|
||||
report_write.and(cleanup)
|
||||
}
|
||||
|
||||
fn quarantine_comparison_artifacts(out_dir: &Path) -> Result<Value, String> {
|
||||
static NEXT: std::sync::atomic::AtomicUsize = std::sync::atomic::AtomicUsize::new(0);
|
||||
let quarantine = loop {
|
||||
let candidate = out_dir.join(format!("invalid-comparison-{}-{}", std::process::id(), NEXT.fetch_add(1, std::sync::atomic::Ordering::Relaxed)));
|
||||
match std::fs::create_dir(&candidate) {
|
||||
Ok(()) => break candidate,
|
||||
Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => continue,
|
||||
Err(e) => return Err(e.to_string()),
|
||||
}
|
||||
};
|
||||
let mut errors = Vec::new();
|
||||
let mut artifacts = Map::new();
|
||||
let prefix = quarantine.file_name().unwrap().to_string_lossy();
|
||||
for name in ["raw-report.json", "side-by-side.png", "heatmap.png", "regions"] {
|
||||
let path = out_dir.join(name);
|
||||
// Keep the same parent: moving a read-only directory into another parent
|
||||
// can require write permission on that directory (to change its `..`).
|
||||
let target = out_dir.join(format!("{prefix}-{name}"));
|
||||
let move_artifact = (|| -> std::io::Result<()> {
|
||||
match std::fs::symlink_metadata(&target) {
|
||||
Ok(_) => return Err(std::io::Error::new(std::io::ErrorKind::AlreadyExists, "quarantine target exists")),
|
||||
Err(e) if e.kind() == std::io::ErrorKind::NotFound => (),
|
||||
Err(e) => return Err(e),
|
||||
}
|
||||
std::fs::rename(&path, &target)
|
||||
})();
|
||||
match move_artifact {
|
||||
Ok(()) => { artifacts.insert(name.into(), json!(target.to_string_lossy().replace('\\', "/"))); },
|
||||
Err(e) if e.kind() == std::io::ErrorKind::NotFound => (),
|
||||
Err(e) => errors.push(format!("{name}: {e}")),
|
||||
}
|
||||
}
|
||||
let record = json!({"invalid":true, "artifacts":artifacts, "errors":errors});
|
||||
atomic_report(&quarantine.join("manifest.json"), &record)?;
|
||||
Ok(record)
|
||||
}
|
||||
|
||||
fn clear_comparison_artifacts(out_dir: &Path) -> Result<(), String> {
|
||||
// Only remove generated files. Never recursively delete a caller's output directory
|
||||
// or follow a regions symlink into another directory. Unexpected directories at
|
||||
|
||||
@@ -507,10 +507,15 @@ fn artifact_cleanup_failure_blocks_the_gate() {
|
||||
let dir = ws.path.join("diff/regions");
|
||||
std::fs::set_permissions(&dir, std::fs::Permissions::from_mode(0o555)).unwrap();
|
||||
let gate = gate_hero(&ws.io(), &mut state, "comp.png", HERO_MIN, "diff", Some("index.html"), &no_organic_scan);
|
||||
std::fs::set_permissions(&dir, std::fs::Permissions::from_mode(0o755)).unwrap();
|
||||
if dir.exists() { std::fs::set_permissions(&dir, std::fs::Permissions::from_mode(0o755)).unwrap(); }
|
||||
assert!(!gate.ok);
|
||||
assert!(gate.reasons.iter().any(|r| r.contains("cannot clear comparison artifacts")), "{:?}", gate.reasons);
|
||||
let report: Value = serde_json::from_slice(&std::fs::read(ws.path.join("diff/report.json")).unwrap()).unwrap();
|
||||
assert_eq!(report["measurementsAvailable"], false);
|
||||
assert_eq!(report["gate"]["ok"], false);
|
||||
assert_no_current_measurements(&ws);
|
||||
let quarantine = ws.path.join(report["artifactCleanup"]["quarantine"]["artifacts"]["regions"].as_str().expect("cleanup failure must identify quarantined evidence"));
|
||||
assert!(quarantine.join("retired.png").is_file());
|
||||
assert_eq!(report["artifactCleanup"]["quarantine"]["errors"], json!([]));
|
||||
std::fs::set_permissions(quarantine, std::fs::Permissions::from_mode(0o755)).unwrap();
|
||||
}
|
||||
|
||||
@@ -40,6 +40,13 @@ Each attempt clears the previous raw report, whole-frame PNGs, and region PNGs;
|
||||
failed attempts also clear any partial evidence they wrote. Removed regions cannot
|
||||
leave old crops in a successful comparison. Cleanup is limited to generated files,
|
||||
does not follow region-directory symlinks, and blocks the gate if it fails.
|
||||
When removal fails, the remaining generated set is renamed beside its current paths under a unique
|
||||
`invalid-comparison-*` prefix. `artifactCleanup` and a quarantine manifest record
|
||||
the moved paths. Keeping the same parent avoids requiring write access to the
|
||||
read-only directory itself.
|
||||
If filesystem permissions also prevent quarantine, that field records the error
|
||||
and explicitly lists the invalid artifact paths; no evidence is certified and the
|
||||
gate remains closed.
|
||||
Write failures block the gate
|
||||
and, when the report location is writable, publish an unavailable-evidence report.
|
||||
|
||||
|
||||
Reference in New Issue
Block a user