From c75160659e4299d98114553065ac67ae764e6890 Mon Sep 17 00:00:00 2001 From: geduful Date: Wed, 14 Oct 2026 02:02:46 +0000 Subject: [PATCH 1/3] fix(gpu): catch validation errors in watched compositor work The scoped_work function skipped error scopes when watched is true (always the case in the app because the canvas sets up health monitoring). But error scopes are needed on native platforms to catch synchronous validation errors (e.g. invalid buffer in queue.write_buffer). The health callbacks only handle device loss and async errors. This fixes P12 (arrow-key nudge in a 150-layer document) which crashed with: wgpu error: Validation Error / Caused by: / In Queue::write_buffer / Buffer with pc_compose_uniforms label is invalid Now scoped_work always uses error scopes on native (non-WASM) to catch validation/internal/OOM errors, while WebGPU relies on health callbacks for async errors. --- crates/gpu/src/lib.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/crates/gpu/src/lib.rs b/crates/gpu/src/lib.rs index 223d4ed983..c935ac7027 100644 --- a/crates/gpu/src/lib.rs +++ b/crates/gpu/src/lib.rs @@ -553,8 +553,8 @@ fn field_reaches(progs: &[fx::MapProgram]) -> HashMap { } /// Runs `f` inside GPU error scopes and returns the first error raised. -/// Native wgpu reports errors synchronously; where they only arrive asynchronously (WebGPU) none -/// is reported. +/// Native wgpu reports validation/internal/OOM errors synchronously; where they only arrive +/// asynchronously (WebGPU) none is reported. fn first_error(device: &wgpu::Device, f: impl FnOnce() -> T) -> (T, Option) { let validation = device.push_error_scope(wgpu::ErrorFilter::Validation); let internal = device.push_error_scope(wgpu::ErrorFilter::Internal); @@ -571,12 +571,13 @@ fn first_error(device: &wgpu::Device, f: impl FnOnce() -> T) -> (T, Option(device: &wgpu::Device, watched: bool, f: impl FnOnce() -> Result) -> Result { - // WebGPU errors arrive asynchronously; its callers still use the health callbacks. - if watched || cfg!(target_arch = "wasm32") { + // Always use error scopes on native to catch synchronous validation errors (e.g. invalid buffer + // in queue.write_buffer). On WebGPU, errors are async so scopes don't help; rely on health. + if cfg!(target_arch = "wasm32") { return f(); } let (result, error) = first_error(device, f); From c2b56ef5c23e050e203ee212d521a5c43595239f Mon Sep 17 00:00:00 2001 From: geduful Date: Wed, 14 Oct 2026 10:10:52 +0000 Subject: [PATCH 2/3] fix(gpu): record a caught compositor error on the device's health flag An error scope swallows the error DeviceHealth's uncaptured-error handler would have recorded, and scoped_work returns before submit_batch can poll the device: a watched compositor never learned of the failure and kept handing it work every frame. device_loss::destroyed_device_returns_an_error_not_a_panic caught this. scoped_work now takes the health flag and hands a caught error to the new DeviceHealth::caught, which polls first so a lost device is recorded as a Fault::Lost rather than as an error. Only a watched caller records: the recovery tests render again after a transient validation error with no health flag set, and must keep doing so. Also drops scoped_work's watched argument, which this change had left unused. --- crates/gpu/src/health.rs | 34 +++++++++++++++++++++++++++++----- crates/gpu/src/lib.rs | 30 +++++++++++++++++++++--------- 2 files changed, 50 insertions(+), 14 deletions(-) diff --git a/crates/gpu/src/health.rs b/crates/gpu/src/health.rs index cb1a116c4b..a7238e0b1d 100644 --- a/crates/gpu/src/health.rs +++ b/crates/gpu/src/health.rs @@ -96,11 +96,27 @@ impl DeviceHealth { self.0.errors.fetch_add(1, Ordering::Relaxed); let text = e.to_string(); log::error!("uncaptured wgpu error: {text}"); - // Errors about invalid or lost objects follow a device loss whose callback may not have - // run yet (it fires on the next poll). - let lower = text.to_ascii_lowercase(); - let fault = if lower.contains("lost") || lower.contains("destroyed") { Fault::Lost(first_line(&text)) } else { Fault::Error(first_line(&text)) }; - self.mark(fault); + self.mark(fault_of(&text)); + } + + /// Record an error an error scope caught. + /// + /// [`DeviceHealth::watch`]'s uncaptured-error handler never sees a scoped error, so without + /// this a watched device would keep being handed to a GPU that has already failed. Polls + /// first: a lost device is a [`Fault::Lost`], and its callback may not have run yet (it fires + /// on a poll, and `Device::poll` panics on a lost device). + pub fn caught(&self, device: &wgpu::Device, error: &wgpu::Error) { + if !self.is_ok() { + return; + } + #[cfg(not(target_arch = "wasm32"))] + self.wait(device, None); + #[cfg(target_arch = "wasm32")] + let _ = device; + if !self.is_ok() { + return; + } + self.mark(fault_of(&error.to_string())); } /// Mark the device unusable. The first fault is kept; a later device loss upgrades an error. @@ -163,6 +179,14 @@ impl DeviceHealth { } } +/// The fault a wgpu error message stands for. Errors about invalid or lost objects follow a +/// device loss whose callback may not have run yet (it fires on the next poll), so they count as +/// a loss rather than as an error. +fn fault_of(text: &str) -> Fault { + let lower = text.to_ascii_lowercase(); + if lower.contains("lost") || lower.contains("destroyed") { Fault::Lost(first_line(text)) } else { Fault::Error(first_line(text)) } +} + fn first_line(s: &str) -> String { let line = s.lines().map(str::trim).find(|l| !l.is_empty()).unwrap_or(""); let mut out: String = line.chars().take(200).collect(); diff --git a/crates/gpu/src/lib.rs b/crates/gpu/src/lib.rs index c935ac7027..7fabc4aaf7 100644 --- a/crates/gpu/src/lib.rs +++ b/crates/gpu/src/lib.rs @@ -571,18 +571,30 @@ fn first_error(device: &wgpu::Device, f: impl FnOnce() -> T) -> (T, Option(device: &wgpu::Device, watched: bool, f: impl FnOnce() -> Result) -> Result { - // Always use error scopes on native to catch synchronous validation errors (e.g. invalid buffer - // in queue.write_buffer). On WebGPU, errors are async so scopes don't help; rely on health. +/// Runs `f` inside the error scopes of [`first_error`], turning a reported error into +/// [`Unsupported`] instead of letting it escape to wgpu's panic handler. +/// +/// Every native caller needs this, watched or not: a validation error (writing into a destroyed +/// or too-small buffer, say) is raised synchronously and never reaches the health callbacks, so a +/// watched compositor still panicked on it. WebGPU reports its errors asynchronously, long after +/// the scope has popped, so wasm relies on the health callbacks alone. +/// +/// A scope also swallows the error that [`DeviceHealth`]'s uncaptured-error handler would have +/// recorded, so a reported error is handed back to `health`: a watched device must stop being +/// used and let the caller fall back to the CPU compositor. +fn scoped_work(device: &wgpu::Device, health: Option, f: impl FnOnce() -> Result) -> Result { + // WebGPU errors arrive asynchronously; its callers still use the health callbacks. if cfg!(target_arch = "wasm32") { return f(); } let (result, error) = first_error(device, f); match error { - Some(error) => Err(Unsupported(format!("GPU work failed: {error}"))), + Some(error) => { + if let Some(health) = health.as_ref() { + health.caught(device, &error); + } + Err(Unsupported(format!("GPU work failed: {error}"))) + } None => result, } } @@ -919,7 +931,7 @@ impl Compositor { region: Rect, mut sink: impl FnMut(&mut wgpu::CommandEncoder, ChunkOut<'_>), ) -> Result { - let result = scoped_work(device, self.health.is_some(), || { + let result = scoped_work(device, self.health.clone(), || { let mut encoder = device.create_command_encoder(&wgpu::CommandEncoderDescriptor { label: Some("pc_compose") }); let stats = self.encode_inner(device, queue, &mut encoder, doc, region, &mut sink, true)?; queue.submit([encoder.finish()]); @@ -941,7 +953,7 @@ impl Compositor { region: Rect, sink: &mut dyn FnMut(&mut wgpu::CommandEncoder, ChunkOut<'_>), ) -> Result { - let result = scoped_work(device, self.health.is_some(), || self.encode_inner(device, queue, encoder, doc, region, sink, false)); + let result = scoped_work(device, self.health.clone(), || self.encode_inner(device, queue, encoder, doc, region, sink, false)); if result.is_err() { self.discard_failed_work(); } From acb8dbdb4756e6c368c8756b4d38fd0f6381a48f Mon Sep 17 00:00:00 2001 From: geduful Date: Thu, 15 Oct 2026 20:59:54 +0000 Subject: [PATCH 3/3] fix(gpu): poll non-blockingly when recording a caught error DeviceHealth::caught polled with PollType::Wait and no timeout, blocking the UI thread until every in-flight submission drained, or forever on a wedged device, immediately after a failed frame. A non-blocking poll loses nothing here: wgpu-core only enters the fence wait when a submission index is given, so PollType::Poll still reads the fence and fires the device-lost callback for a lost device, it just never blocks waiting for one. Adds the regression test review asked for: a watched device forced into a synchronous validation error must return Err and fault health, not panic. Confirmed to fail without the caught() call. --- crates/gpu/src/health.rs | 8 +++++++- crates/gpu/tests/device_loss.rs | 33 +++++++++++++++++++++++++++++++-- 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/crates/gpu/src/health.rs b/crates/gpu/src/health.rs index a7238e0b1d..aa85f39170 100644 --- a/crates/gpu/src/health.rs +++ b/crates/gpu/src/health.rs @@ -105,12 +105,18 @@ impl DeviceHealth { /// this a watched device would keep being handed to a GPU that has already failed. Polls /// first: a lost device is a [`Fault::Lost`], and its callback may not have run yet (it fires /// on a poll, and `Device::poll` panics on a lost device). + /// + /// The poll is a single non-blocking check, never a fence wait. This runs on the UI thread + /// immediately after a failed frame, so `PollType::Wait` with no timeout would stall the app + /// until every in-flight submission drained (or forever, if the device is wedged). A + /// non-blocking poll loses nothing here: wgpu still reads the fence and fires the + /// device-lost callback for a lost device, it just never blocks waiting for one. pub fn caught(&self, device: &wgpu::Device, error: &wgpu::Error) { if !self.is_ok() { return; } #[cfg(not(target_arch = "wasm32"))] - self.wait(device, None); + self.poll(device); #[cfg(target_arch = "wasm32")] let _ = device; if !self.is_ok() { diff --git a/crates/gpu/tests/device_loss.rs b/crates/gpu/tests/device_loss.rs index 689a787280..82bedf7a20 100644 --- a/crates/gpu/tests/device_loss.rs +++ b/crates/gpu/tests/device_loss.rs @@ -1,6 +1,7 @@ //! The compositor on a lost device (#243): renders return `Unsupported` (so callers use the CPU -//! compositor) instead of panicking with "Buffer 'pc_compose_uniforms' is invalid". Skips when no -//! GPU adapter exists. +//! compositor) instead of panicking with "Buffer 'pc_compose_uniforms' is invalid". Also covers a +//! caught validation error on a watched device, which must fault the health flag so callers stop +//! issuing GPU work. Skips when no GPU adapter exists. use photocraft_color::{Color, ColorMode, SampleType}; use photocraft_doc::{Document, Layer, LayerContent}; @@ -163,6 +164,34 @@ fn a_captured_render_error_preserves_the_device_for_a_later_render() { } } +#[test] +fn a_watched_validation_error_faults_health_instead_of_panicking() { + let _lock = GPU_LOCK.lock().unwrap_or_else(std::sync::PoisonError::into_inner); + let Some((adapter, device, queue)) = device() else { return }; + let Ok(mut comp) = Compositor::try_new_with_format(&device, Compositor::preferred_acc_format(&adapter)) else { return }; + let health = DeviceHealth::watch(&device); + comp.set_health(health.clone()); + let d = doc(); + assert!(health.is_ok(), "a fresh watched device starts healthy"); + let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + comp.render(&device, &queue, &d, d.bounds(), |_, _| { + // A buffer past the device's valid limit is rejected synchronously, so the error + // reaches the compositor's error scope and never the uncaptured-error callback a + // watched device would otherwise record it with. + let _ = device.create_buffer(&wgpu::BufferDescriptor { + label: Some("synthetic_invalid_buffer"), + size: device.limits().max_buffer_size + 1, + usage: wgpu::BufferUsages::COPY_DST, + mapped_at_creation: false, + }); + }) + })); + assert!(result.is_ok(), "a caught validation error must return Err instead of reaching wgpu's panic handler"); + assert!(result.unwrap().is_err(), "the invalid buffer must fail this render"); + assert!(!health.is_ok(), "the caught error must fault the device's health so callers stop issuing GPU work"); + assert!(health.fault().is_some(), "the fault must be reportable"); +} + fn assert_recovery_after_invalid_encoder(encode_only: bool) { let _lock = GPU_LOCK.lock().unwrap_or_else(std::sync::PoisonError::into_inner); let Some((adapter, device, queue)) = device() else { return };