diff --git a/src/gpu_backend.rs b/src/gpu_backend.rs index a11f211e8..ad16e13ab 100644 --- a/src/gpu_backend.rs +++ b/src/gpu_backend.rs @@ -319,17 +319,46 @@ pub fn crashed_backends() -> Vec { return Vec::new(); }; let text = std::fs::read_to_string(p).unwrap_or_default(); + let named = || -> Vec { + text.lines() + .find_map(|l| l.strip_prefix("backend=")) + .unwrap_or("") + .split(',') + .map(str::trim) + .filter(|s| !s.is_empty()) + .map(str::to_string) + .collect() + }; if text.lines().any(|l| l.trim() == SURVIVED_MARKER) { - return Vec::new(); + // Surviving the proof delay is good evidence, not proof. A backend + // can carry the interface for minutes and still be unable to hold a + // large drawing: D3D12 on an integrated GPU runs out of room only + // once the whole scene uploads, well past the mark, and a run killed + // then used to be excused. The crash report says what actually + // happened, so ask it before excusing anything. + return match crash_report_blames_the_device(&text) { + true => named(), + false => Vec::new(), + }; } - text.lines() - .find_map(|l| l.strip_prefix("backend=")) - .unwrap_or("") - .split(',') - .map(str::trim) - .filter(|s| !s.is_empty()) - .map(str::to_string) - .collect() + named() +} + +/// Whether the run this sentinel belongs to died inside the graphics device. +/// +/// The sentinel records the process id it armed for, and crash reports are +/// named for the process that wrote them, so the two join exactly: this asks +/// about that run and not whichever report happens to be newest. +fn crash_report_blames_the_device(sentinel: &str) -> bool { + let Some(pid) = sentinel + .lines() + .find_map(|line| line.strip_prefix("pid=")) + .and_then(|value| value.trim().parse::().ok()) + else { + return false; + }; + crate::sys::crash_log::report_for_pid(pid) + .is_some_and(|report| crate::sys::crash_log::is_device_failure(&report)) } /// Record that the armed backend has kept the app alive long enough to be @@ -1086,6 +1115,67 @@ mod tests { let _ = std::fs::remove_dir_all(&dir); } + /// Surviving the proof delay excuses a kill, but not a crash the + /// graphics device itself caused. D3D12 on an integrated GPU carries the + /// interface for minutes and only fails once a large drawing uploads, so + /// a run that died then has to keep costing the backend. + #[test] + fn a_device_crash_still_blames_the_backend_however_late_it_came() { + let _guard = SENTINEL_TESTS.lock().unwrap_or_else(|e| e.into_inner()); + let dir = std::env::temp_dir().join(format!("ocs-gpu-late-{}", std::process::id())); + let _ = std::fs::create_dir_all(&dir); + // SAFETY: single-threaded test mutating a process-local test hook. + unsafe { std::env::set_var(SENTINEL_DIR_ENV, &dir) }; + + let reports = crate::sys::crash_log::directory().expect("a config directory"); + let _ = std::fs::create_dir_all(&reports); + let report = reports.join(format!("crash-1-{}.log", std::process::id())); + + arm_sentinel("dx12"); + mark_sentinel_survived(); + let _ = std::fs::remove_file(&report); + assert!( + crashed_backends().is_empty(), + "with no crash report, a survived run blames nobody" + ); + + // The run left a report, and it is the device's own failure. + std::fs::write( + &report, + crate::sys::crash_log::report_from( + "Error in Buffer::get_mapped_range: Validation Error", + "wgpu-29.0.4/src/backend/wgpu_core.rs:2253:18", + 1, + ), + ) + .unwrap(); + assert_eq!( + crashed_backends(), + vec!["dx12".to_string()], + "a device failure must cost the backend even past the proof delay" + ); + + // A crash that has nothing to do with graphics still costs nothing. + std::fs::write( + &report, + crate::sys::crash_log::report_from( + "index out of bounds: the len is 3 but the index is 7", + "src/app/commands/draw.rs:120:5", + 1, + ), + ) + .unwrap(); + assert!( + crashed_backends().is_empty(), + "an unrelated panic must not cost the user a working backend" + ); + + let _ = std::fs::remove_file(&report); + disarm_sentinel(); + unsafe { std::env::remove_var(SENTINEL_DIR_ENV) }; + let _ = std::fs::remove_dir_all(&dir); + } + #[test] fn force_hooks_drive_probes_without_spawning() { // SAFETY: single-threaded test mutating process-local test hooks. diff --git a/src/sys.rs b/src/sys.rs index ff3b2e40f..52d27561e 100644 --- a/src/sys.rs +++ b/src/sys.rs @@ -429,6 +429,62 @@ pub mod crash_log { crate::config::config_dir().map(|path| path.join("crash_logs")) } + /// The report a given process left behind, if it left one. + /// + /// Reports are named for the process that wrote them, so a caller holding + /// a process id from somewhere else — the GPU crash sentinel does — can + /// ask what killed that exact run instead of guessing from the newest + /// file, which on a busy machine may belong to a different one. + pub fn report_for_pid(pid: u32) -> Option { + let directory = directory()?; + let suffix = format!("-{pid}.log"); + let entry = std::fs::read_dir(directory) + .ok()? + .flatten() + .find(|entry| { + entry + .file_name() + .to_string_lossy() + .ends_with(suffix.as_str()) + })?; + std::fs::read_to_string(entry.path()).ok() + } + + /// Whether a report describes the graphics device giving up. + /// + /// Deliberately narrow. Anything that reaches wgpu — an exhausted device, + /// a lost one, a buffer that was never created — is the backend's + /// business; a panic in the drawing code that happens to run on a frame + /// is not, and must not cost the user a working backend. + pub fn is_device_failure(report: &str) -> bool { + let mut panic_line = ""; + let mut at_line = ""; + for line in report.lines() { + if let Some(rest) = line.strip_prefix("panic: ") { + panic_line = rest; + } else if let Some(rest) = line.strip_prefix("at: ") { + at_line = rest; + } + } + // The message wgpu panics with names the call, and the location names + // wgpu itself; either alone is enough, since a stripped build can + // leave the message terse. + let haystack = format!("{panic_line} {at_line}").to_ascii_lowercase(); + ["wgpu", "out of memory", "buffer", "surface", "device lost"] + .iter() + .any(|needle| haystack.contains(needle)) + } + + /// The backend a report says was live, as written by [`report_from`]. + pub fn backend_in_report(report: &str) -> Option { + report + .lines() + .find_map(|line| line.strip_prefix("gpu backend: ")) + .map(str::trim) + .filter(|backend| !backend.is_empty() && *backend != "(default)") + .map(str::to_string) + } + /// Chain a report writer onto the current panic hook. /// /// Call once, early: a panic before this runs is still silent. @@ -562,6 +618,46 @@ pub mod crash_log { ); } + /// Only the device's own failures may cost a backend. A panic in + /// drawing code that merely happened to run on a frame must not. + #[test] + fn a_device_failure_is_told_apart_from_an_ordinary_panic() { + let device = super::report_from( + "Error in Buffer::get_mapped_range: Validation Error", + "wgpu-29.0.4/src/backend/wgpu_core.rs:2253:18", + 1, + ); + assert!(super::is_device_failure(&device), "{device}"); + + let oom = super::report_from("Out of Memory", "src/scene/pipeline/mod.rs:10:1", 1); + assert!(super::is_device_failure(&oom), "{oom}"); + + let ordinary = super::report_from( + "index out of bounds: the len is 3 but the index is 7", + "src/app/commands/draw.rs:120:5", + 1, + ); + assert!( + !super::is_device_failure(&ordinary), + "an indexing bug is not the graphics device's fault: +{ordinary}" + ); + } + + /// The backend line is what the sentinel joins on, so it has to come + /// back out — and "(default)" names nothing to blame. + #[test] + fn the_backend_is_read_back_out_of_a_report() { + // SAFETY: single-threaded test mutating a process-local variable. + unsafe { std::env::set_var("WGPU_BACKEND", "dx12") }; + let named = super::report_from("boom", "x.rs:1:1", 0); + assert_eq!(super::backend_in_report(&named).as_deref(), Some("dx12")); + + unsafe { std::env::remove_var("WGPU_BACKEND") }; + let unnamed = super::report_from("boom", "x.rs:1:1", 0); + assert_eq!(super::backend_in_report(&unnamed), None); + } + /// A panic with no message still has to produce a filed report rather /// than nothing at all. #[test]