Merge pull request #1540 from HakanSeven12/fix/sentinel-reads-crash-report

fix(gpu): a crash the device caused still costs the backend, however late

src/gpu_backend.rs

@@ -319,17 +319,46 @@ pub fn crashed_backends() -> Vec<String> {
         return Vec::new();
     };
     let text = std::fs::read_to_string(p).unwrap_or_default();
+    let named = || -> Vec<String> {
+        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::<u32>().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.

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<String> {
+        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<String> {
+        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]