forked from 1b-agent/opencadstudio-upstream
fix(gpu): a crash the device caused still costs the backend, however late
#1519 stopped the sentinel blaming a backend for a run that had already drawn for SENTINEL_PROOF_DELAY, on the reasoning that a backend which kept the application alive that long had proved itself. That reasoning does not hold for the failure this repository sees most on integrated hardware. Measured here on an Intel Iris Xe: with `--backend dx12` a large drawing aborts the process 110 s in, long past the proof delay, so the sentinel carried `survived=1` and the next launch went straight back to dx12 and crashed again. The same binary on `--backend vulkan` opens the same drawings with no out-of-memory report and no uncaptured errors at all, so the backend really is the difference and moving off it really is the cure. D3D12 is fine while the interface draws and only fails once the whole scene uploads, which is why the clock is the wrong question. The crash report from #1521 answers the right one. It records the backend that was live and what the panic was, and it is named for the process that wrote it, so it joins exactly to the `pid` the sentinel already stores — no guessing from whichever report is newest, and no new state to keep. A sentinel marked survived is now excused only when that run left no report, or left one that has nothing to do with the graphics device. The test that matters is the pair, and both were run end to end: killed from Task Manager after 40 s, no report -> dx12 kept device crash after 110 s, report names wgpu -> dx12 skipped, Vulkan `is_device_failure` is deliberately narrow: an indexing bug in drawing code that happens to run on a frame must not cost the user a working backend. Co-Authored-By: Claude Opus 5 <[email protected]>
This commit is contained in:
1 parent
07f25f4bca
commit
6cf170d253
2 files changed
+195
-9
No files matched your search
+99
-9
@@ -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.
|
||||
|
||||
+96
@@ -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]
|
||||
|
||||
Reference in new issue
Block a user