From 5a537d5d39b69dfac601ac8f9c99784a5f398e98 Mon Sep 17 00:00:00 2001 From: hxw <530313@qq.com> Date: Sat, 10 Oct 2026 00:46:24 -0500 Subject: [PATCH] LR-BEHAV-GPU: no render is inside the GPU driver when the app quits (issue #620) A render still running on a worker thread while the process exits crashes in the driver (SIGSEGV in the NVIDIA Vulkan driver on Linux), where neither the panic hook nor catch_unwind helps: pool workers were detached, the devices are statics that are never dropped, and nothing on the way out waited for GPU work. - gpu: every entry into a device (render, device creation, pool trim, denoise runner and tiles) is counted as work in flight (exit.rs). begin_shutdown() lets nothing new in; wait_idle(timeout) waits for what is inside. - preview: JobPool keeps its workers. shutdown(timeout) and Drop discard the queued jobs and wait, bounded, for the running ones (2 s on drop); never for the calling thread, and not twice. - engine: a render job caught by the shutdown gives up instead of rendering on the CPU, so quitting doesn't wait for it. - app: on_exit ends with LightcraftApp::shutdown (2 s for everything), after settings and library are saved; the macOS menu-bar Quit goes through on_exit. Not changed: the export, merge, denoise and import threads are still detached; their GPU work goes through the gate, their CPU work is cut off at exit as before. Tests: pool drop waits / is bounded / survives a panicked job and its own job dropping it; the gate; GPU renders refused and the device not created after begin_shutdown; the app's shutdown leaves no render worker. The crash itself needs Linux + NVIDIA and was not reproduced here (Windows, DX12): 9 of 9 quits of the app mid-render exit with code 0 in under 0.8 s. Co-Authored-By: Claude Opus 5.5 --- apps/lightcraft/src/main.rs | 8 ++ crates/engine/src/media.rs | 9 +- crates/gpu/src/exit.rs | 132 ++++++++++++++++++++++++ crates/gpu/src/lib.rs | 53 +++++++++- crates/gpu/src/nn.rs | 11 ++ crates/gpu/tests/shutdown.rs | 58 +++++++++++ crates/gpu/tests/shutdown_first.rs | 25 +++++ crates/preview/src/pool.rs | 160 ++++++++++++++++++++++++++--- crates/preview/src/tests.rs | 113 ++++++++++++++++++++ crates/ui-egui/src/lib.rs | 17 +++ crates/ui-egui/src/render.rs | 13 +++ crates/ui-egui/tests/shutdown.rs | 40 ++++++++ docs/gpu-pipeline.md | 11 ++ docs/parity.md | 2 +- 14 files changed, 634 insertions(+), 18 deletions(-) create mode 100644 crates/gpu/src/exit.rs create mode 100644 crates/gpu/tests/shutdown.rs create mode 100644 crates/gpu/tests/shutdown_first.rs create mode 100644 crates/ui-egui/tests/shutdown.rs diff --git a/apps/lightcraft/src/main.rs b/apps/lightcraft/src/main.rs index 832b158b..8d33b881 100644 --- a/apps/lightcraft/src/main.rs +++ b/apps/lightcraft/src/main.rs @@ -78,9 +78,17 @@ impl eframe::App for App { if let Err(e) = self.0.session.close_library() { log::error!("saving the library failed: {e}"); } + // last, with everything saved: no render may be inside the GPU driver when the process ends + if !self.0.shutdown(EXIT_WAIT) { + log::warn!("quit: background work was still running after {EXIT_WAIT:?}"); + } } } +/// How long quitting waits for renders that are running (issue #620). A GPU render takes well under +/// a second; anything slower gives way to quitting. +const EXIT_WAIT: std::time::Duration = std::time::Duration::from_secs(2); + /// The window's renderer (egui-wgpu): eframe's defaults, with LightCraft's backend choice — DX12 /// alone on Windows unless `LIGHTCRAFT_GPU_BACKEND` / `WGPU_BACKEND` say otherwise (issue #136: /// with Vulkan in the set, wgpu loads the Vulkan driver even when it then picks DX12) — and DX12 diff --git a/crates/engine/src/media.rs b/crates/engine/src/media.rs index 1231221b..a67bdcaa 100644 --- a/crates/engine/src/media.rs +++ b/crates/engine/src/media.rs @@ -752,7 +752,14 @@ impl RenderJob { }; } let was_loaded = matches!(self.source, SourceRef::Loaded(_)); - match self.source.load_source() { + // The app is closing (issue #620): nobody is left to show this, and quitting waits for the + // jobs that are running. Without this, one that lost the GPU mid-way would render on the CPU. + let closing = || lightcraft_gpu::shutting_down().then(|| "LightCraft is closing".to_string()); + let source = match closing() { + Some(e) => Err(e), + None => self.source.load_source().and_then(|s| closing().map_or(Ok(s), Err)), + }; + match source { Ok(source) => { // the plain picture, or (Denoise amount above 0 and a denoised picture cached) the mix the amount asks for let src = &source.image_for(self.settings.denoise_amount()); diff --git a/crates/gpu/src/exit.rs b/crates/gpu/src/exit.rs new file mode 100644 index 00000000..79f114ef --- /dev/null +++ b/crates/gpu/src/exit.rs @@ -0,0 +1,132 @@ +//! Leaving: no GPU work may be in flight when the process ends (issue #620). +//! +//! The devices are statics that are never dropped, and the threads that render (preview workers, +//! export, denoise, the device warm-up) are not the one that ends the process. Exiting runs the +//! driver's own teardown while those threads keep running: a render that is inside the driver at +//! that moment crashes there (seen as a SIGSEGV in the NVIDIA Vulkan driver), where neither the +//! panic hook nor `catch_unwind` can help. +//! +//! So every entry into a device goes through [`enter`], and the host ends the process with +//! [`crate::begin_shutdown`] (nothing new enters) and [`crate::wait_idle`] (what is in flight +//! finishes, up to a deadline). + +use std::sync::{Condvar, Mutex, MutexGuard}; +use std::time::{Duration, Instant}; + +struct State { + closed: bool, + /// [`Work`] in flight, on any thread. + busy: usize, +} + +pub(crate) struct Gate { + state: Mutex, + idle: Condvar, +} + +/// GPU work in flight (until dropped). +pub(crate) struct Work<'a>(&'a Gate); + +static GATE: Gate = Gate::new(); + +/// Start GPU work, or `None` when the process is ending: the caller then does without the GPU. +/// Work may nest (a render that creates the device). +pub(crate) fn enter() -> Option> { + GATE.enter() +} + +pub(crate) fn begin_shutdown() { + GATE.close(); +} + +pub(crate) fn shutting_down() -> bool { + GATE.lock().closed +} + +pub(crate) fn wait_idle(timeout: Duration) -> bool { + GATE.wait_idle(timeout) +} + +impl Gate { + pub const fn new() -> Gate { + Gate { state: Mutex::new(State { closed: false, busy: 0 }), idle: Condvar::new() } + } + + fn lock(&self) -> MutexGuard<'_, State> { + self.state.lock().unwrap_or_else(|e| e.into_inner()) + } + + pub fn enter(&self) -> Option> { + let mut s = self.lock(); + if s.closed { + return None; + } + s.busy = s.busy.saturating_add(1); + Some(Work(self)) + } + + /// Nothing enters from now on. + pub fn close(&self) { + self.lock().closed = true; + } + + /// Wait up to `timeout` until no work is in flight; `false` when some still is. + pub fn wait_idle(&self, timeout: Duration) -> bool { + let t0 = Instant::now(); + let mut s = self.lock(); + while s.busy > 0 { + let Some(left) = timeout.checked_sub(t0.elapsed()).filter(|d| !d.is_zero()) else { return false }; + s = self.idle.wait_timeout(s, left).unwrap_or_else(|e| e.into_inner()).0; + } + true + } +} + +impl Drop for Work<'_> { + fn drop(&mut self) { + let mut s = self.0.lock(); + s.busy = s.busy.saturating_sub(1); + if s.busy == 0 { + drop(s); + self.0.idle.notify_all(); + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn closed_gate_lets_nothing_in_and_waits_for_what_is_inside() { + let gate = Gate::new(); + assert!(gate.wait_idle(Duration::ZERO), "idle: no wait"); + let outer = gate.enter().unwrap(); + let inner = gate.enter().unwrap(); + gate.close(); + assert!(gate.enter().is_none(), "nothing new once closed"); + drop(inner); + let t0 = Instant::now(); + assert!(!gate.wait_idle(Duration::from_millis(50)), "the outer work is still in flight"); + assert!(t0.elapsed() >= Duration::from_millis(50)); + std::thread::scope(|s| { + s.spawn(move || { + std::thread::sleep(Duration::from_millis(100)); + drop(outer); + }); + assert!(gate.wait_idle(Duration::from_secs(30)), "woken when the work ends"); + }); + assert!(gate.enter().is_none()); + } + + #[test] + fn work_that_panics_still_leaves() { + let gate = Gate::new(); + let r = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + let _work = gate.enter(); + panic!("a render panics (expected in this test)"); + })); + assert!(r.is_err()); + assert!(gate.wait_idle(Duration::ZERO)); + } +} diff --git a/crates/gpu/src/lib.rs b/crates/gpu/src/lib.rs index 9c234964..6c3a2ebe 100644 --- a/crates/gpu/src/lib.rs +++ b/crates/gpu/src/lib.rs @@ -15,6 +15,8 @@ //! and [`last_fallback`] say why (`ui.inspect` → `perf.gpuReason` / `perf.gpuFallback`). //! The browser build has no GPU path yet (WebGPU device creation is asynchronous): everything here //! compiles to the CPU fallback on wasm32. +//! +//! Before the process ends: [`begin_shutdown`], then [`wait_idle`] (issue #620, see `exit.rs`). #![forbid(unsafe_code)] #![deny(clippy::unwrap_used, clippy::expect_used, clippy::panic, clippy::unimplemented, clippy::todo, clippy::unreachable)] @@ -29,6 +31,8 @@ use lightcraft_raster::Rgb32f; pub mod backend; #[cfg(not(target_arch = "wasm32"))] mod ctx; +#[cfg(not(target_arch = "wasm32"))] +mod exit; #[cfg(all(feature = "denoise", not(target_arch = "wasm32")))] pub mod nn; #[cfg(not(target_arch = "wasm32"))] @@ -77,6 +81,9 @@ pub fn unavailable_reason() -> Option { if !ENABLED.load(Ordering::Relaxed) { return Some("disabled by the GPU rendering preference (app.gpu)".into()); } + if shutting_down() { + return Some("LightCraft is closing".into()); + } if BROKEN.load(Ordering::Relaxed) { let r = BROKEN_REASON.lock().unwrap_or_else(|e| e.into_inner()).clone().unwrap_or_else(|| "device error".into()); return Some(format!("stopped after a GPU failure: {r}")); @@ -147,6 +154,41 @@ pub fn enabled() -> bool { ENABLED.load(Ordering::Relaxed) && !BROKEN.load(Ordering::Relaxed) && !env_disabled() } +/// The process is about to end: no GPU work starts from now on, on any thread. [`render`] returns +/// `None` (callers render on the CPU), the devices are not created and denoise tiles fail over to +/// the CPU. There is no way back. Follow it with [`wait_idle`]. +pub fn begin_shutdown() { + #[cfg(not(target_arch = "wasm32"))] + exit::begin_shutdown(); +} + +/// Whether [`begin_shutdown`] was called. +pub fn shutting_down() -> bool { + #[cfg(not(target_arch = "wasm32"))] + { + exit::shutting_down() + } + #[cfg(target_arch = "wasm32")] + { + false + } +} + +/// Wait up to `timeout` for the GPU work in flight on every thread to end. `true`: none is left, +/// and after [`begin_shutdown`] none can start, so the process may end; `false`: the deadline +/// passed with a render (or a device being created) still inside the driver. +pub fn wait_idle(timeout: std::time::Duration) -> bool { + #[cfg(not(target_arch = "wasm32"))] + { + exit::wait_idle(timeout) + } + #[cfg(target_arch = "wasm32")] + { + let _ = timeout; + true + } +} + /// Why GPU compute that has a device of its own (denoise) is switched off by the user's settings, or `None`: the /// environment switches and the rendering preference, but not a render-device failure (the devices are separate). #[cfg(all(feature = "denoise", not(target_arch = "wasm32")))] @@ -177,6 +219,11 @@ pub(crate) fn device() -> Option<&'static ctx::Gpu> { if env_disabled() || !ENABLED.load(Ordering::Relaxed) { return existing_device(); } + if let Some(g) = GPU.get() { + return g.as_ref().ok(); + } + // creating the device is a long call into the driver: not once the process is ending + let _work = exit::enter()?; GPU.get_or_init(|| { let Some(backends) = backend::compute_backends() else { return Err("disabled by LIGHTCRAFT_GPU_BACKEND=off".into()) }; backend::with_init_marker(backends, || { @@ -281,7 +328,9 @@ pub fn set_pool_limit(bytes: u64) { /// Free recycled buffers until at most `keep` bytes stay pooled (e.g. when the app goes idle). pub fn trim_pool(keep: u64) { #[cfg(not(target_arch = "wasm32"))] - if let Some(g) = existing_device() { + if let Some(g) = existing_device() + && let Some(_work) = exit::enter() + { if keep == 0 { render::release_shared_source(); } @@ -340,6 +389,8 @@ pub fn render(src: &Arc, info: &SourceInfo, s: &DevelopSettings, req: &R if !enabled() || req.depth != lightcraft_pipeline::OutputDepth::U8 || req.proof.is_some() { return None; } + // in flight until this returns; `None` once the process is ending (issue #620) + let _work = exit::enter()?; let gpu = device()?; let ext = stages.map(|c| c.extension::()); let fault = take_fault(); diff --git a/crates/gpu/src/nn.rs b/crates/gpu/src/nn.rs index bb9d5eea..fb4a4f78 100644 --- a/crates/gpu/src/nn.rs +++ b/crates/gpu/src/nn.rs @@ -63,9 +63,17 @@ fn dev() -> Result<&'static Dev, String> { if let Some(why) = crate::switched_off() { return Err(why); } + // creating the device is GPU work too: not once the process is ending (issue #620) + let _work = match DEV.get() { + Some(_) => None, + None => Some(crate::exit::enter().ok_or(CLOSING)?), + }; DEV.get_or_init(create_device).as_ref().map_err(|e| e.clone()) } +/// Why nothing runs on the GPU once [`crate::begin_shutdown`] was called. +const CLOSING: &str = "LightCraft is closing"; + fn create_device() -> Result { let Some(backends) = crate::backend::compute_backends() else { return Err("disabled by LIGHTCRAFT_GPU_BACKEND=off".into()) }; crate::backend::with_init_marker(backends, || { @@ -654,6 +662,7 @@ pub struct NetRunner { /// Put `net` on the GPU for `tile × tile` input cells, or say why it cannot be (no GPU, a layer or a size the kernels /// do not do, the device refusing the buffers). Creates the device on first use and compiles the kernels. pub fn runner(net: &Net, tile: usize) -> Result { + let _work = crate::exit::enter().ok_or(CLOSING)?; let d = dev()?; let max_binding = d.limits.max_storage_buffer_binding_size.min(d.limits.max_buffer_size).min(MAX_BUFFER); let (layout, layers) = plan(net, tile, max_binding)?; @@ -840,6 +849,8 @@ impl TileRunner for NetRunner { if input.len() != self.shared.layout.in_floats { return Err(Error::Input("the tile is not the size the model wants".into())); } + // one tile is one piece of GPU work: a photo's worth would outlast any wait at exit + let Some(_work) = crate::exit::enter() else { return Err(Error::Runtime(CLOSING.into())) }; let w = self.take()?; let r = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| self.execute(&w, input))); match r { diff --git a/crates/gpu/tests/shutdown.rs b/crates/gpu/tests/shutdown.rs new file mode 100644 index 00000000..b827a071 --- /dev/null +++ b/crates/gpu/tests/shutdown.rs @@ -0,0 +1,58 @@ +//! Issue #620: a render still inside the GPU driver while the process exits crashes there (a +//! SIGSEGV no panic hook catches). Once `begin_shutdown` is called nothing starts on the GPU, and +//! `wait_idle` returns when the renders in flight on other threads have ended. +//! One test: the shutdown holds for the whole process. Without a GPU adapter only the CPU side of +//! the contract is checked. + +use std::sync::Arc; +use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; +use std::time::Duration; + +use lightcraft_develop::DevelopSettings; +use lightcraft_pipeline::{RenderRequest, SourceInfo}; + +#[test] +fn nothing_renders_on_the_gpu_once_the_process_is_ending() { + let (w, h) = (600, 400); + let src = Arc::new(lightcraft_scenes::demo_library()[0].render(w, h)); + let info = SourceInfo { raw: true, ..Default::default() }; + let mut s = DevelopSettings::default(); + s.light.exposure = 0.3; + let req = RenderRequest::fit(w, h); + let has_gpu = lightcraft_gpu::available(); + if !has_gpu { + eprintln!("no GPU adapter ({:?}): only the CPU side is checked", lightcraft_gpu::unavailable_reason()); + } + assert!(!lightcraft_gpu::shutting_down()); + assert!(lightcraft_gpu::wait_idle(Duration::ZERO), "nothing is in flight yet"); + + // a worker that renders until the GPU refuses, as the preview workers do while the app quits + let (rendered, refused) = (Arc::new(AtomicUsize::new(0)), Arc::new(AtomicBool::new(false))); + let worker = { + let (src, info, s) = (src.clone(), info.clone(), s.clone()); + let (rendered, refused) = (rendered.clone(), refused.clone()); + std::thread::spawn(move || { + while lightcraft_gpu::render(&src, &info, &s, &req, None).is_some() { + rendered.fetch_add(1, Ordering::SeqCst); + } + refused.store(true, Ordering::SeqCst); + }) + }; + if has_gpu { + while rendered.load(Ordering::SeqCst) == 0 && !refused.load(Ordering::SeqCst) { + std::thread::yield_now(); + } + assert!(rendered.load(Ordering::SeqCst) > 0, "the GPU renders before the shutdown ({:?})", lightcraft_gpu::last_fallback()); + } + + lightcraft_gpu::begin_shutdown(); + assert!(lightcraft_gpu::shutting_down()); + assert!(lightcraft_gpu::wait_idle(Duration::from_secs(60)), "the render in flight ends"); + worker.join().unwrap(); + assert!(refused.load(Ordering::SeqCst)); + assert!(lightcraft_gpu::render(&src, &info, &s, &req, None).is_none(), "no GPU render once the process is ending"); + assert!(lightcraft_gpu::wait_idle(Duration::ZERO)); + if has_gpu { + assert_eq!(lightcraft_gpu::unavailable_reason().as_deref(), Some("LightCraft is closing")); + } +} diff --git a/crates/gpu/tests/shutdown_first.rs b/crates/gpu/tests/shutdown_first.rs new file mode 100644 index 00000000..3810762e --- /dev/null +++ b/crates/gpu/tests/shutdown_first.rs @@ -0,0 +1,25 @@ +//! Issue #620: quitting right after the start must not leave the device being created (a long +//! call into the driver, on the `lc-gpu-init` thread) while the process exits. Once +//! `begin_shutdown` is called the device is not created at all. +//! One test in its own process: the shutdown comes before the first use of the GPU. + +use std::sync::Arc; +use std::time::Duration; + +use lightcraft_develop::DevelopSettings; +use lightcraft_pipeline::{RenderRequest, SourceInfo}; + +#[test] +fn the_device_is_not_created_once_the_process_is_ending() { + lightcraft_gpu::begin_shutdown(); + lightcraft_gpu::warm_up(); + assert!(!lightcraft_gpu::available()); + assert_eq!(lightcraft_gpu::adapter_name(), None); + let src = Arc::new(lightcraft_scenes::demo_library()[0].render(300, 200)); + let rendered = lightcraft_gpu::render(&src, &SourceInfo::default(), &DevelopSettings::default(), &RenderRequest::fit(300, 200), None); + assert!(rendered.is_none()); + assert!(lightcraft_gpu::wait_idle(Duration::from_secs(60)), "the warm-up thread found the gate closed"); + // `ready()` is "device creation finished" (or the GPU is switched off by the environment) + let switched_off = lightcraft_gpu::unavailable_reason().is_some_and(|r| r.starts_with("disabled by LIGHTCRAFT_GPU")); + assert!(switched_off || !lightcraft_gpu::ready(), "no device was created"); +} diff --git a/crates/preview/src/pool.rs b/crates/preview/src/pool.rs index a0ea185f..4dbd3566 100644 --- a/crates/preview/src/pool.rs +++ b/crates/preview/src/pool.rs @@ -7,11 +7,16 @@ //! //! Native: a fixed set of worker threads, started on first use. wasm32 (no threads): the host //! calls [`JobPool::run_inline`] once per frame. +//! +//! No worker outlives its pool by more than a deadline (issue #620: a render still inside the GPU +//! driver while the process exits crashes there): [`JobPool::shutdown`] and dropping the pool +//! discard the queued jobs and wait, bounded, for the running ones. use std::hash::Hash; use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::mpsc::{Receiver, Sender, channel}; use std::sync::{Arc, Condvar, Mutex}; +use std::time::Duration; pub type Job = Box R + Send>; @@ -27,8 +32,16 @@ struct Shared { queue: Mutex>>, cv: Condvar, shutdown: AtomicBool, + /// The worker threads that have not ended yet, and the signal that one did. + #[cfg(not(target_arch = "wasm32"))] + live: Mutex>, + #[cfg(not(target_arch = "wasm32"))] + ended: Condvar, } +/// How long dropping a pool waits for the jobs still running (see [`JobPool::shutdown`]). +const DROP_WAIT: Duration = Duration::from_secs(2); + /// A finished job. pub struct Done { pub slot: S, @@ -47,6 +60,10 @@ pub struct JobPool { threads: usize, started: bool, seq: u64, + #[cfg(not(target_arch = "wasm32"))] + workers: Vec>, + /// [`JobPool::shutdown`] ran: dropping the pool doesn't wait a second time. + stopped: bool, } impl JobPool { @@ -54,12 +71,23 @@ impl JobPool { pub fn new(threads: usize) -> Self { let (tx, rx) = channel(); JobPool { - shared: Arc::new(Shared { queue: Mutex::new(Vec::new()), cv: Condvar::new(), shutdown: AtomicBool::new(false) }), + shared: Arc::new(Shared { + queue: Mutex::new(Vec::new()), + cv: Condvar::new(), + shutdown: AtomicBool::new(false), + #[cfg(not(target_arch = "wasm32"))] + live: Mutex::new(Vec::new()), + #[cfg(not(target_arch = "wasm32"))] + ended: Condvar::new(), + }), tx, rx, threads, started: false, seq: 0, + #[cfg(not(target_arch = "wasm32"))] + workers: Vec::new(), + stopped: false, } } @@ -81,24 +109,69 @@ impl JobPool { } self.started = true; #[cfg(not(target_arch = "wasm32"))] - for i in 0..self.threads { - let shared = self.shared.clone(); - let tx = self.tx.clone(); - let _ = std::thread::Builder::new().name(format!("lc-job-{i}")).spawn(move || { - while let Some(e) = take(&shared, true) { - let t0 = std::time::Instant::now(); - let result = (e.job)(); - let done = Done { slot: e.slot, key: e.key, result, ms: t0.elapsed().as_secs_f64() * 1000.0 }; - if tx.send(done).is_err() { - break; + { + // held while spawning: a worker that ends at once still finds its id listed + let mut live = self.shared.live.lock().unwrap_or_else(|e| e.into_inner()); + for i in 0..self.threads { + let shared = self.shared.clone(); + let tx = self.tx.clone(); + let spawned = std::thread::Builder::new().name(format!("lc-job-{i}")).spawn(move || { + let _ended = Ended(&shared); + while let Some(e) = take(&shared, true) { + let t0 = std::time::Instant::now(); + let result = (e.job)(); + let done = Done { slot: e.slot, key: e.key, result, ms: t0.elapsed().as_secs_f64() * 1000.0 }; + if tx.send(done).is_err() { + break; + } } + }); + match spawned { + Ok(h) => { + live.push(h.thread().id()); + self.workers.push(h); + } + Err(e) => log::error!("job pool: worker {i} could not start: {e}"), } - }); + } + } + } + + /// Stop the pool: queued jobs are dropped, no new one starts, and the call waits up to + /// `timeout` for the jobs that are running. Returns whether every worker has ended; one that + /// hasn't is left to finish on its own. For owners that are about to end the process, where + /// dropping the pool may come too late or never (`std::process::exit`). + pub fn shutdown(&mut self, timeout: Duration) -> bool { + self.stopped = true; + #[cfg(not(target_arch = "wasm32"))] + { + stop(&self.shared, &mut self.workers, timeout) + } + #[cfg(target_arch = "wasm32")] + { + let _ = timeout; + close(&self.shared); + true + } + } + + /// Worker threads that have not ended (0 before the first job, and on wasm). + pub fn live_workers(&self) -> usize { + #[cfg(not(target_arch = "wasm32"))] + { + self.shared.live.lock().map(|l| l.len()).unwrap_or(0) + } + #[cfg(target_arch = "wasm32")] + { + 0 } } /// Queue `job` for `slot`, replacing a queued (not yet running) job for the same slot. pub fn submit(&mut self, slot: S, key: u64, priority: u32, job: Job) { + if self.stopped { + return; // nothing runs it any more + } self.start(); self.seq += 1; let (lock, cv) = (&self.shared.queue, &self.shared.cv); @@ -172,9 +245,66 @@ fn take(shared: &Shared, wait: bool) -> Option> { } } -impl Drop for JobPool { +/// Takes a worker off the live list when its thread ends (also when a job panicked). +#[cfg(not(target_arch = "wasm32"))] +struct Ended<'a, S, R>(&'a Shared); + +#[cfg(not(target_arch = "wasm32"))] +impl Drop for Ended<'_, S, R> { fn drop(&mut self) { - self.shared.shutdown.store(true, Ordering::Relaxed); - self.shared.cv.notify_all(); + let me = std::thread::current().id(); + self.0.live.lock().unwrap_or_else(|e| e.into_inner()).retain(|id| *id != me); + self.0.ended.notify_all(); + } +} + +/// Tell the workers to stop and take the queued jobs away from them. +fn close(shared: &Shared) { + // the flag changes under the queue lock: a worker is either before its check or waiting + let queued = { + let mut q = shared.queue.lock().unwrap_or_else(|e| e.into_inner()); + shared.shutdown.store(true, Ordering::Relaxed); + std::mem::take(&mut *q) + }; + shared.cv.notify_all(); + drop(queued); // outside the lock: a job may own a lot +} + +/// [`JobPool::shutdown`]: wait up to `timeout` for the workers to end, without holding the queue +/// lock, and never for the calling thread (a pool dropped by one of its own jobs). +#[cfg(not(target_arch = "wasm32"))] +fn stop(shared: &Shared, workers: &mut Vec>, timeout: Duration) -> bool { + close(shared); + let me = std::thread::current().id(); + let t0 = std::time::Instant::now(); + let mut live = shared.live.lock().unwrap_or_else(|e| e.into_inner()); + while live.iter().any(|id| *id != me) { + let Some(left) = timeout.checked_sub(t0.elapsed()).filter(|d| !d.is_zero()) else { break }; + live = shared.ended.wait_timeout(live, left).unwrap_or_else(|e| e.into_inner()).0; + } + let running: Vec = live.iter().copied().filter(|id| *id != me).collect(); + drop(live); + for h in workers.drain(..) { + let id = h.thread().id(); + // a worker off the list is past its last job: joining it only waits for the thread to unwind + if id != me && !running.contains(&id) { + let _ = h.join(); + } + } + if !running.is_empty() { + log::warn!("job pool: {} job(s) still running after {timeout:?}; not waiting for them", running.len()); + } + running.is_empty() +} + +impl Drop for JobPool { + fn drop(&mut self) { + if self.stopped { + return; + } + #[cfg(not(target_arch = "wasm32"))] + stop(&self.shared, &mut self.workers, DROP_WAIT); + #[cfg(target_arch = "wasm32")] + close(&self.shared); } } diff --git a/crates/preview/src/tests.rs b/crates/preview/src/tests.rs index 3d634003..d733ad82 100644 --- a/crates/preview/src/tests.rs +++ b/crates/preview/src/tests.rs @@ -201,6 +201,119 @@ fn pool_runs_inline() { assert_eq!(p.try_recv().map(|d| d.result), Some(1)); } +/// A pool with one worker that is inside a job until the returned sender is used or dropped. +fn pool_with_a_running_job() -> (JobPool, std::sync::mpsc::Sender<()>) { + let mut p: JobPool = JobPool::new(1); + let (started, has_started) = std::sync::mpsc::channel(); + let (release, released) = std::sync::mpsc::channel::<()>(); + p.submit( + 1, + 0, + 1, + Box::new(move || { + let _ = started.send(()); + let _ = released.recv_timeout(std::time::Duration::from_secs(60)); + }), + ); + has_started.recv().unwrap(); + (p, release) +} + +#[test] +fn pool_drop_waits_for_running_job() { + // issue #620: a job that outlives its pool can still be inside the GPU driver when the process exits + use std::sync::atomic::{AtomicBool, Ordering}; + let mut p: JobPool = JobPool::new(1); + let (started, has_started) = std::sync::mpsc::channel(); + let (finished, queued_ran) = (Arc::new(AtomicBool::new(false)), Arc::new(AtomicBool::new(false))); + let f = finished.clone(); + p.submit( + 1, + 0, + 9, + Box::new(move || { + let _ = started.send(()); + std::thread::sleep(std::time::Duration::from_millis(200)); + f.store(true, Ordering::SeqCst); + }), + ); + has_started.recv().unwrap(); + let q = queued_ran.clone(); + p.submit(2, 0, 1, Box::new(move || q.store(true, Ordering::SeqCst))); + drop(p); + assert!(finished.load(Ordering::SeqCst), "drop returned while the job was still running"); + assert!(!queued_ran.load(Ordering::SeqCst), "a job that had not started is discarded"); +} + +#[test] +fn pool_shutdown_reports_workers_and_drop_does_not_wait_again() { + let (mut p, release) = pool_with_a_running_job(); + assert_eq!(p.live_workers(), 1); + let t0 = std::time::Instant::now(); + assert!(!p.shutdown(std::time::Duration::from_millis(100)), "the job is still running"); + let waited = t0.elapsed(); + assert!(waited >= std::time::Duration::from_millis(100) && waited < std::time::Duration::from_secs(30), "{waited:?}"); + assert_eq!(p.live_workers(), 1); + // stopped: nothing new is queued, and the job that ends is noticed + p.submit(2, 0, 1, Box::new(|| ())); + assert_eq!(p.queued(), 0); + drop(release); + assert!(p.shutdown(std::time::Duration::from_secs(30))); + assert_eq!(p.live_workers(), 0); + + let (mut p, _release) = pool_with_a_running_job(); + assert!(!p.shutdown(std::time::Duration::ZERO)); + let t0 = std::time::Instant::now(); + drop(p); + assert!(t0.elapsed() < std::time::Duration::from_secs(1), "dropped after a shutdown: no second wait"); +} + +#[test] +fn pool_drop_is_bounded_when_a_job_never_ends() { + // a job blocked on something its owner holds must not hang the owner (it would hang quitting) + let (p, _release) = pool_with_a_running_job(); + let t0 = std::time::Instant::now(); + drop(p); + let waited = t0.elapsed(); + assert!(waited >= std::time::Duration::from_secs(1) && waited < std::time::Duration::from_secs(30), "{waited:?}"); +} + +#[test] +fn pool_stops_after_a_job_panicked_and_when_dropped_by_its_own_job() { + // a worker that unwound is not waited for + let mut p: JobPool = JobPool::new(1); + p.submit(1, 0, 1, Box::new(|| panic!("a job panics (expected in this test)"))); + while p.live_workers() > 0 { + std::thread::yield_now(); + } + assert!(p.shutdown(std::time::Duration::from_secs(30))); + + // the last owner of a pool is one of its jobs: the worker must not wait for (or join) itself + let slot: Arc>>> = Arc::new(std::sync::Mutex::new(None)); + let (dropped, has_dropped) = std::sync::mpsc::channel(); + let mut p: JobPool = JobPool::new(1); + let s = slot.clone(); + p.submit( + 1, + 0, + 1, + Box::new(move || { + let own = loop { + if let Some(p) = s.lock().unwrap().take() { + break p; + } + std::thread::yield_now(); + }; + let t0 = std::time::Instant::now(); + drop(own); + let _ = dropped.send(t0.elapsed()); + }), + ); + *slot.lock().unwrap() = Some(p); + let waited = has_dropped.recv_timeout(std::time::Duration::from_secs(30)).unwrap(); + assert!(waited < std::time::Duration::from_secs(1), "{waited:?}"); +} + #[test] fn disk_cache_never_touches_foreign_files() { // issue #98: a library opened on a folder that already has a `thumbs/` folder diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 70581891..3c55333c 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -685,6 +685,23 @@ impl LightcraftApp { } } + /// The process is about to end (settings and library are saved by then): make sure nothing is + /// inside the GPU driver when it does (issue #620: a render running on a worker thread while + /// the process exits crashes in the driver). No GPU work starts from here on; the render + /// workers get up to `timeout` to finish their jobs, and what is left of it goes to GPU work + /// on other threads (export, denoise, the device warm-up). `false`: the deadline passed first + /// (the host exits anyway). The app renders nothing afterwards. + pub fn shutdown(&mut self, timeout: std::time::Duration) -> bool { + lightcraft_engine::gpu::begin_shutdown(); + #[cfg(not(target_arch = "wasm32"))] + let t0 = std::time::Instant::now(); + let stopped = self.renderer.shutdown(timeout); + #[cfg(not(target_arch = "wasm32"))] + let timeout = timeout.saturating_sub(t0.elapsed()); + let idle = lightcraft_engine::gpu::wait_idle(timeout); + stopped && idle + } + /// Per-frame logic before layout (control channel, renders, shortcuts, drops). pub fn logic(&mut self, ctx: &egui::Context) { i18n::set_language(self.ui.language); diff --git a/crates/ui-egui/src/render.rs b/crates/ui-egui/src/render.rs index f4643694..5bbd4516 100644 --- a/crates/ui-egui/src/render.rs +++ b/crates/ui-egui/src/render.rs @@ -415,6 +415,19 @@ impl Renderer { self.pending.contains_key(&slot) } + /// The app is closing: drop the jobs that have not started and wait up to `timeout` for the + /// ones running on the worker threads. Returns whether all of them have ended. Nothing is + /// rendered afterwards. + pub fn shutdown(&mut self, timeout: std::time::Duration) -> bool { + self.queue.clear(); + self.pool.shutdown(timeout) + } + + /// Worker threads that are still alive (0 before the first render). + pub fn live_workers(&self) -> usize { + self.pool.live_workers() + } + /// The texture to show for a grid/filmstrip thumbnail: the rendered one, else its stand-in. pub fn thumb(&self, id: PhotoId) -> Option<&Tex> { self.textures.get(&Slot::Thumb(id)).or_else(|| self.textures.get(&Slot::ThumbQuick(id))) diff --git a/crates/ui-egui/tests/shutdown.rs b/crates/ui-egui/tests/shutdown.rs new file mode 100644 index 00000000..1a601bc7 --- /dev/null +++ b/crates/ui-egui/tests/shutdown.rs @@ -0,0 +1,40 @@ +//! Issue #620: quitting while previews render must not leave a render worker running (on Linux +//! with the NVIDIA driver, one inside the GPU when the process exits is a SIGSEGV). +//! `LightcraftApp::shutdown` stops the render workers and closes the GPU for the whole process, +//! hence one test in a binary of its own. + +use std::time::{Duration, Instant}; + +use lightcraft_ui_egui::headless::Headless; +use lightcraft_ui_egui::{LightcraftApp, Services}; + +#[test] +fn shutdown_leaves_no_render_worker_running() { + let services = Services { png: None, ..Default::default() }; + let mut h = Headless::new(LightcraftApp::new(lightcraft_engine::Session::with_demo(), services), [1200.0, 760.0], 1.0); + // frames until the grid's thumbnails are being rendered on the workers + let t0 = Instant::now(); + while h.app.renderer.live_workers() == 0 || h.app.renderer.in_flight() == 0 { + assert!(t0.elapsed() < Duration::from_secs(120), "no render started"); + h.step(); + } + assert!(!lightcraft_engine::gpu::shutting_down()); + + assert!(h.app.shutdown(Duration::from_secs(120)), "the jobs that were running ended in time"); + assert_eq!(h.app.renderer.live_workers(), 0); + assert_eq!(h.app.renderer.queued(), 0, "jobs that had not started are dropped"); + assert!(lightcraft_engine::gpu::shutting_down()); + assert!(lightcraft_engine::gpu::wait_idle(Duration::ZERO), "nothing is inside the GPU"); + + // frames after it (the window's last ones) ask for renders that are not started any more + for _ in 0..3 { + h.step(); + } + assert_eq!(h.app.renderer.live_workers(), 0); + assert_eq!(h.app.renderer.queued(), 0); + + // a job that was caught before its render gives up: quitting doesn't wait for a CPU render + let id = h.app.session.active().or_else(|| h.app.session.catalog.photos().next().map(|p| p.id)).expect("a demo photo"); + let job = h.app.session.render_job(id, 300, 200, false, true).expect("a render job"); + assert_eq!(job.run().rendered.err().as_deref(), Some("LightCraft is closing")); +} diff --git a/docs/gpu-pipeline.md b/docs/gpu-pipeline.md index c843d5a0..8d747235 100644 --- a/docs/gpu-pipeline.md +++ b/docs/gpu-pipeline.md @@ -60,6 +60,17 @@ up (`gpu::warm_up` from the first frame's settings), and never while GPU renderi Settings ▸ Performance ▸ *Use the GPU for rendering* unchecked (applied before the window opens), `LIGHTCRAFT_GPU=0` or `LIGHTCRAFT_GPU_BACKEND=off`, no GPU driver is loaded for rendering at all. +**Quitting.** The devices are never dropped, and renders run on worker threads: a render still inside +the driver while the process exits crashes there (a SIGSEGV in the NVIDIA Vulkan driver on Linux, +issue #620), which no panic hook catches. So every entry into a device counts as work in flight +(`crates/gpu/src/exit.rs`), and the desktop app ends with `LightcraftApp::shutdown` (from `on_exit`, +after settings and library are saved): `gpu::begin_shutdown()` — from then on `gpu::render` returns +`None`, no device is created, denoise tiles fail over and a render job gives up instead of rendering +on the CPU — then the render pool stops (queued jobs dropped, running ones waited for) and +`gpu::wait_idle` waits for GPU work on other threads (export, denoise, the device warm-up). One +deadline of 2 s covers it all; past it the app quits anyway. A job pool that is dropped (headless +hosts, tests) waits for its running jobs the same way (`JobPool::shutdown`, `crates/preview/src/pool.rs`). + **Crash sentinel.** The desktop app writes `gpu-init.marker` into its settings folder (next to `ui.json`: `%APPDATA%\LightCraft`, `~/Library/Application Support/LightCraft`, `~/.config/lightcraft`) just before the compute device is created and removes it as soon as creation diff --git a/docs/parity.md b/docs/parity.md index 4c27755b..4f64fdca 100644 --- a/docs/parity.md +++ b/docs/parity.md @@ -517,7 +517,7 @@ under the UI command that wraps it (a no-op error); the UI command now wins. | LR-BEHAV-BG-TASKS | Background tasks | P0 | ✅ | `crates/preview/src/lib.rs` (`JobPool`), `crates/ui-egui/src/export_task.rs`, `crates/ui-egui/src/import.rs` | renders off the UI thread; exports started from the UI run on a worker thread with a progress panel and Cancel (`ui.inspect` → `export`); imports show a progress window. No combined activity centre | | LR-BEHAV-OFFLINE | Offline editing | P1 | ✅ | | local-first: everything works offline | | LR-BEHAV-WEB-SAFETY | Library safety in the browser build | P1 | 🟡 | `cmd:file.backupLibrary`, `cmd:file.restoreLibrary`, `apps/lightcraft-web/src/safety.rs`, `apps/lightcraft-web/src/backup.rs`, `apps/lightcraft-web/src/files.rs` | web only (experimental): Back Up Library (zip of the catalog and every stored original) and a non-destructive Restore; failed saves show as unsaved and are retried; a photo that can't be stored isn't added; one tab per library (Web Locks); notices for storage that isn't persistent and for a library that can't open; `?reset` asks first; a panic shows a message. Missing: `?safe` start, deleting originals of removed photos, zip64 (backups > 4 GB) | -| LR-BEHAV-GPU | GPU acceleration | P0 | ✅ | `crates/gpu/src/render.rs`, `cmd:app.gpu`, `docs/gpu-pipeline.md` | CPU fallback per render on device limits, out of memory, device errors / loss, incomplete or black results (`perf.gpuReason`, `perf.gpuFallback`); short submissions for slow iGPUs; backend choice (DX12 only on Windows, `LIGHTCRAFT_GPU_BACKEND` / `WGPU_BACKEND`), device created after the window opens, crash sentinel (issue #136); no tiling beyond the buffer limit (CPU) | +| LR-BEHAV-GPU | GPU acceleration | P0 | ✅ | `crates/gpu/src/render.rs`, `cmd:app.gpu`, `docs/gpu-pipeline.md` | CPU fallback per render on device limits, out of memory, device errors / loss, incomplete or black results (`perf.gpuReason`, `perf.gpuFallback`); short submissions for slow iGPUs; backend choice (DX12 only on Windows, `LIGHTCRAFT_GPU_BACKEND` / `WGPU_BACKEND`), device created after the window opens, crash sentinel (issue #136); quitting waits (up to 2 s) for renders in flight and starts no new GPU work, so none is inside the driver when the process exits (`crates/gpu/src/exit.rs`, issue #620; seen crashing only on Linux + NVIDIA, other drivers unverified); no tiling beyond the buffer limit (CPU) | | LR-BEHAV-RENDER-FIDELITY | Rendering matches Lightroom | P1 | 🟡 | `crates/pipeline/src/lib.rs`, `crates/pipeline/src/tone.rs` | every slider exists and works, but the character of the result (tone curve shape, highlight recovery, texture / clarity / dehaze, noise reduction, sharpening, default look) is tuned by eye; there is no systematic side-by-side comparison against Lightroom on the same CC0 raws. Needs a fidelity suite: Lightroom reference renders kept only in the local `plan/` (never committed), compared per slider and preset with a perceptual metric | | LR-BEHAV-DRAGDROP | Drag and drop | P1 | ✅ | `crates/ui-egui/src/lib.rs`, `crates/ui-egui/src/panels/grid.rs` (`drag_feedback`), `crates/ui-egui/src/panels/left.rs` (`drop_target`, `album_drag_over`, `top_level_drop_target`, `drag_auto_scroll`) | files → app (import); grid photos → an album row (adds the selection, with a count badge while dragging); sidebar album / folder rows → a folder (moves it; its name follows the pointer); a drag of albums or grid photos near the top or bottom edge of the sidebar scrolls it (faster nearer the edge, only while the pointer is over the sidebar) | | LR-BEHAV-TOAST | Toast notifications | P1 | ✅ | `crates/ui-egui/src/panels/mod.rs` | |