diff --git a/crates/format/src/autosave.rs b/crates/format/src/autosave.rs index a2fbddb734..a2739dfc3a 100644 --- a/crates/format/src/autosave.rs +++ b/crates/format/src/autosave.rs @@ -299,13 +299,16 @@ impl RecoveryStore { /// Finish every queued autosave and wait for it, then return the outcomes as /// [`take_completed`](Self::take_completed) does. For a session that ends without asking, - /// e.g. because the window's graphics device was lost (#3438). The workers stop, so later - /// autosaves of the same documents report a stopped worker. + /// e.g. because the window's graphics device was lost (#3438). The workers stop; a later + /// autosave of the same document starts a new one on the same entry, so a failed write can be + /// tried again. pub fn flush(&mut self) -> Vec<(u64, u64, std::result::Result<(), String>)> { for saver in self.savers.values_mut() { saver.shutdown(); } - self.take_completed() + let done = self.take_completed(); + self.savers.clear(); + done } /// Document `doc_id` was saved or closed: remove its recovery data, both its own autosaves diff --git a/crates/format/tests/autosave.rs b/crates/format/tests/autosave.rs index fa078dab6c..4f5f1de29c 100644 --- a/crates/format/tests/autosave.rs +++ b/crates/format/tests/autosave.rs @@ -204,7 +204,13 @@ fn flush_writes_every_queued_autosave_before_returning() { let docs = recovered(&RecoveryStore::new(&dir)); assert_eq!(docs.len(), 2); assert!(docs.values().any(|d| *d == same(a.clone())) && docs.values().any(|d| *d == same(b.clone()))); - assert!(store.autosave_checked(&Arc::new(a), 5, None).is_err(), "the workers stopped"); + // A later autosave (trying a failed write again) starts a new worker on the same entry. + let newer = version(1, 4, ColorMode::Rgb); + store.autosave_checked(&Arc::new(newer.clone()), 5, None).unwrap(); + assert_eq!(store.flush(), vec![(1, 5, Ok(()))]); + let docs = recovered(&RecoveryStore::new(&dir)); + assert_eq!(docs.len(), 2, "the same entry, not a new one"); + assert!(docs.values().any(|d| *d == same(newer.clone())) && !docs.values().any(|d| *d == same(a.clone()))); drop(store); std::fs::remove_dir_all(dir).unwrap(); } diff --git a/crates/ui-egui/src/gpu_status.rs b/crates/ui-egui/src/gpu_status.rs index 008e52a6f1..401704a135 100644 --- a/crates/ui-egui/src/gpu_status.rs +++ b/crates/ui-egui/src/gpu_status.rs @@ -43,7 +43,9 @@ pub fn check(app: &mut PhotocraftApp, ctx: &egui::Context) { /// with shapes panics creating its staging buffers (#1215), and one without leaves a frozen window /// whose close button opens an unsaved-changes prompt nobody can see. So this frame and every later /// one draw nothing; the first writes every unsaved document to crash recovery, waits for the -/// writes, and closes the app without asking. The next launch reopens the documents. +/// writes, and closes the app without asking once all of them are saved. The next launch reopens +/// the documents. If a write fails (a full disk, say) the app stays open with the documents in +/// memory and tries again every [`RECOVERY_RETRY_SECS`]: closing would lose them. pub fn check_window(app: &mut PhotocraftApp, ctx: &egui::Context) { let Some((device, health)) = app.window_device.as_ref() else { return }; // A loss while the app sat idle or the PC slept is noticed here, before the next frame. @@ -52,36 +54,78 @@ pub fn check_window(app: &mut PhotocraftApp, ctx: &egui::Context) { return; } ctx.graphics_mut(|g| *g = Default::default()); - if app.window_lost { - return; + let now = ctx.input(|i| i.time); + if !app.window_lost { + app.window_lost = true; + let reason = health.fault().map_or_else(|| "GPU device lost".to_string(), |f| f.to_string()); + log::error!("{reason}: the window can't be drawn any more; saving unsaved documents for recovery and closing"); + } else { + match app.window_recovery_retry { + // Saved, and the close is on its way. + None => return, + Some(at) if now < at => { + ctx.request_repaint_after(std::time::Duration::from_secs_f64((at - now).max(0.0))); + return; + } + Some(_) => {} + } + } + match save_for_recovery(app) { + Ok(()) => { + app.window_recovery_retry = None; + app.allow_close = true; + ctx.send_viewport_cmd(egui::ViewportCommand::Close); + } + Err(e) => { + log::error!("{e}; keeping the documents open and trying again in {RECOVERY_RETRY_SECS} s"); + app.window_recovery_retry = Some(now + RECOVERY_RETRY_SECS); + ctx.request_repaint_after(std::time::Duration::from_secs_f64(RECOVERY_RETRY_SECS)); + } } - app.window_lost = true; - let reason = health.fault().map_or_else(|| "GPU device lost".to_string(), |f| f.to_string()); - log::error!("{reason}: the window can't be drawn any more; saving unsaved documents for recovery and closing"); - save_for_recovery(app); - app.allow_close = true; - ctx.send_viewport_cmd(egui::ViewportCommand::Close); } -/// Queue a crash-recovery autosave of every document with unsaved changes, whatever the autosave -/// timer and preference, and wait until the writes finished (see `Services::flush_autosaves`). -fn save_for_recovery(app: &mut PhotocraftApp) { +/// Seconds between attempts to save unsaved work for recovery after the window's device was lost. +pub const RECOVERY_RETRY_SECS: f64 = 2.0; + +/// Write every document with unsaved changes to crash recovery, whatever the autosave timer and +/// preference, and wait until the writes finished (see `Services::flush_autosaves`). `Ok` only when +/// each of them is known to be on disk. +fn save_for_recovery(app: &mut PhotocraftApp) -> Result<(), String> { let jobs: Vec<_> = app.session.documents().iter().filter(|d| d.is_dirty()).map(|d| (d.doc.clone(), d.revision, d.path.clone())).collect(); - if let Some(save) = app.services.autosave.as_mut() { - for (doc, revision, path) in &jobs { - if let Err(e) = save(doc, *revision, path.as_deref()) { - log::error!("couldn't save {} for recovery: {e}", doc.name); - } + if jobs.is_empty() { + return Ok(()); + } + // (document id, why) for each document not known to be saved. + let mut failed: Vec<(u64, String)> = Vec::new(); + let Some(save) = app.services.autosave.as_mut() else { return Err("no crash-recovery autosave".into()) }; + for (doc, revision, path) in &jobs { + if let Err(e) = save(doc, *revision, path.as_deref()) { + failed.push((doc.id.0, format!("{}: {e}", doc.name))); } } - if let Some(flush) = app.services.flush_autosaves.as_mut() { - for (id, revision, outcome) in flush() { - match outcome { - Ok(()) => log::info!("document {id} (revision {revision}) saved for recovery"), - Err(e) => log::error!("couldn't save document {id} for recovery: {e}"), - } + // Without a flush, only a service whose `autosave` reports the finished write can confirm it. + let outcomes = match app.services.flush_autosaves.as_mut() { + Some(flush) => flush(), + None if app.services.autosave_results.is_none() => Vec::new(), + None => return Err("crash-recovery autosaves can't be waited for".into()), + }; + let confirm = app.services.flush_autosaves.is_some(); + for (doc, revision, _) in &jobs { + if failed.iter().any(|(id, _)| *id == doc.id.0) { + continue; + } + // The writer skips to the newest queued snapshot, so a later revision confirms this one. + let mine: Vec<_> = outcomes.iter().filter(|(id, written, _)| *id == doc.id.0 && written >= revision).collect(); + if let Some((.., Err(e))) = mine.iter().find(|(.., outcome)| outcome.is_err()) { + failed.push((doc.id.0, format!("{}: {e}", doc.name))); + } else if confirm && mine.is_empty() { + failed.push((doc.id.0, format!("{}: the write wasn't confirmed", doc.name))); + } else { + log::info!("{} (revision {revision}) saved for recovery", doc.name); } } + let failed: Vec<_> = failed.into_iter().map(|(_, why)| why).collect(); + if failed.is_empty() { Ok(()) } else { Err(format!("couldn't save for recovery: {}", failed.join("; "))) } } /// Drop the GPU canvas after `fault`: free its resources, send every view through the CPU diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 7736de1171..10a078ed43 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -820,8 +820,11 @@ pub struct PhotocraftApp { /// The device the window itself draws with, and its health flag: the GPU canvas's, or one /// watched for the CPU canvas (see `gpu_status::check_window`). window_device: Option<(eframe::wgpu::Device, photocraft_gpu::DeviceHealth)>, - /// That device was lost: frames draw nothing and the app is closing (#3438). + /// That device was lost: frames draw nothing and the app closes once its work is saved (#3438). window_lost: bool, + /// When (in `egui::InputState::time`) to try saving unsaved work for recovery again after a + /// failed attempt; the app closes only once it succeeded. + window_recovery_retry: Option, /// Run once the first frames have rendered (see [`Self::on_started`]). started: Option, /// Frame and canvas-upload timings (exposed via `ui.inspect`). @@ -987,6 +990,7 @@ impl PhotocraftApp { gpu: None, window_device: None, window_lost: false, + window_recovery_retry: None, started: None, perf: Default::default(), stroke_trace: stroke_trace::StrokeTrace::from_env(), @@ -1062,8 +1066,8 @@ impl PhotocraftApp { self.window_device = Some((device.clone(), photocraft_gpu::DeviceHealth::watch(device))); } - /// Whether the window's graphics device was lost, so the app saved its documents for - /// recovery and is closing (#3438). + /// Whether the window's graphics device was lost, so the app saves its documents for + /// recovery and closes once they are saved (#3438). pub fn window_lost(&self) -> bool { self.window_lost } @@ -2069,6 +2073,7 @@ impl eframe::App for PhotocraftApp { } }); self.automation_input = false; + gpu_status::check_window(self, &ctx); return; } if screen_picker::busy(&ctx) { diff --git a/crates/ui-egui/tests/gpu_device_loss.rs b/crates/ui-egui/tests/gpu_device_loss.rs index 3a84b96cbb..37dd3db267 100644 --- a/crates/ui-egui/tests/gpu_device_loss.rs +++ b/crates/ui-egui/tests/gpu_device_loss.rs @@ -293,3 +293,70 @@ fn a_lost_window_device_saves_unsaved_work_for_recovery_and_closes() { assert_eq!(upload(&h), (0, true), "a frame with shapes panics inside egui-wgpu on a lost device"); assert_eq!(h.state().session.documents().len(), 1, "the document stays until the app closes"); } + +/// #6464 review: when the recovery save fails (a full disk, say), closing would lose the work, so +/// the app stays open with the document and tries again until the write succeeds, then closes. +#[test] +fn a_lost_window_device_closes_only_once_unsaved_work_is_saved() { + let _gpu = gpu_lock(); + let disk_full = std::sync::Arc::new(std::sync::atomic::AtomicBool::new(true)); + let attempts = std::sync::Arc::new(std::sync::atomic::AtomicUsize::new(0)); + let (full, tries) = (disk_full.clone(), attempts.clone()); + let pending = std::sync::Arc::new(std::sync::Mutex::new(Vec::<(u64, u64)>::new())); + let queued = pending.clone(); + let built = std::panic::catch_unwind(std::panic::AssertUnwindSafe(move || { + egui_kittest::Harness::builder().with_size(egui::vec2(1000.0, 700.0)).with_max_steps(16).wgpu().build_eframe(move |cc| { + PhotocraftApp::setup_context(&cc.egui_ctx, Default::default()); + let services = photocraft_ui_egui::Services { + autosave: Some(Box::new(move |doc: &std::sync::Arc, revision: u64, _: Option<&str>| { + queued.lock().unwrap().push((doc.id.0, revision)); + Ok(()) + })), + autosave_results: Some(Box::new(Vec::new)), + flush_autosaves: Some(Box::new(move || { + tries.fetch_add(1, std::sync::atomic::Ordering::SeqCst); + let outcome = if full.load(std::sync::atomic::Ordering::SeqCst) { Err("disk full".to_string()) } else { Ok(()) }; + pending.lock().unwrap().drain(..).map(|(id, revision)| (id, revision, outcome.clone())).collect() + })), + ..Default::default() + }; + let mut app = PhotocraftApp::new(photocraft_engine::Session::new(), services); + if let Some(rs) = cc.wgpu_render_state.as_ref() { + app.set_wgpu(rs.clone()); + } + app.run("file.new", json!({"width": 320, "height": 240})).unwrap(); + app.run("layer.new.layer", json!({})).unwrap(); + app + }) + })); + let Ok(mut h) = built else { + eprintln!("skipping: no GPU adapter"); + return; + }; + h.run_steps(4); + assert!(h.state_mut().destroy_window_device()); + for _ in 0..120 { + h.run_steps(1); + if h.state().window_lost() { + break; + } + std::thread::sleep(std::time::Duration::from_millis(250)); + } + assert!(h.state().window_lost(), "the loss was noticed"); + let tries = || attempts.load(std::sync::atomic::Ordering::SeqCst); + assert_eq!(tries(), 1); + assert!(h.state().has_unsaved_work(), "a failed recovery save doesn't let the app close"); + // Each step is 0.25 s: no new attempt before the retry interval. + h.run_steps(4); + assert_eq!(tries(), 1, "waits before trying again"); + h.run_steps(5); + assert_eq!(tries(), 2, "tries again"); + assert!(h.state().has_unsaved_work()); + disk_full.store(false, std::sync::atomic::Ordering::SeqCst); + h.run_steps(9); + assert_eq!(tries(), 3); + assert!(!h.state().has_unsaved_work(), "closes once the work is saved"); + h.run_steps(9); + assert_eq!(tries(), 3, "saved once, not again"); + assert_eq!(h.state().session.documents().len(), 1, "the document stays until the app closes"); +}