Merge PR #5370: fix(gpu): catch validation errors in watched compositor work

crates/gpu/src/health.rs

@@ -99,11 +99,33 @@ 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).
+    ///
+    /// 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.poll(device);
+        #[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.
@@ -192,6 +214,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();

crates/gpu/src/lib.rs

@@ -619,8 +619,8 @@ fn lru_victims<K>(mut pages: Vec<(u64, K, u64)>, total: u64, incoming: u64, budg
 }
 
 /// 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<T>(device: &wgpu::Device, f: impl FnOnce() -> T) -> (T, Option<wgpu::Error>) {
     let validation = device.push_error_scope(wgpu::ErrorFilter::Validation);
     let internal = device.push_error_scope(wgpu::ErrorFilter::Internal);
@@ -637,17 +637,30 @@ fn first_error<T>(device: &wgpu::Device, f: impl FnOnce() -> T) -> (T, Option<wg
     (out, error)
 }
 
-/// Unwatched native callers need GPU errors to become `Unsupported` instead of reaching wgpu's
-/// default panic handler. Watched callers keep their fault callbacks, which stop work promptly
-/// and update the health flag shared with the canvas.
-fn scoped_work<T>(device: &wgpu::Device, watched: bool, f: impl FnOnce() -> Result<T, Unsupported>) -> Result<T, Unsupported> {
+/// 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<T>(device: &wgpu::Device, health: Option<DeviceHealth>, f: impl FnOnce() -> Result<T, Unsupported>) -> Result<T, Unsupported> {
     // WebGPU errors arrive asynchronously; its callers still use the health callbacks.
-    if watched || cfg!(target_arch = "wasm32") {
+    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,
     }
 }
@@ -1094,7 +1107,7 @@ impl Compositor {
         region: Rect,
         mut sink: impl FnMut(&mut wgpu::CommandEncoder, ChunkOut<'_>),
     ) -> Result<Stats, Unsupported> {
-        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") });
             if self.timer.as_ref().is_some_and(timing::GpuTimer::busy) {
                 // The readback map completes on a poll. The app polls every frame, but a
@@ -1155,7 +1168,7 @@ impl Compositor {
         region: Rect,
         sink: &mut dyn FnMut(&mut wgpu::CommandEncoder, ChunkOut<'_>),
     ) -> Result<Stats, Unsupported> {
-        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();
         }

crates/gpu/tests/it/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};
@@ -184,6 +185,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 };