diff --git a/apps/photocraft/src/services.rs b/apps/photocraft/src/services.rs index 48bc1ad5bb..363059ad57 100644 --- a/apps/photocraft/src/services.rs +++ b/apps/photocraft/src/services.rs @@ -158,11 +158,12 @@ fn with_store(store: &SharedRecovery, f: impl FnOnce(&mut RecoveryStore) -> R /// until a newer autosave replaces them or they're saved or closed (see [`RecoveryStore`]). fn recovery_services(dir: Option) -> Services { let store: SharedRecovery = Rc::new(RefCell::new(dir.map(RecoveryStore::new))); - let (s1, s2, s3) = (store.clone(), store.clone(), store.clone()); + let (s1, s2, s3, s4) = (store.clone(), store.clone(), store.clone(), store.clone()); Services { autosave: Some(Box::new(move |doc: &Arc, revision: u64, path: Option<&str>| { - with_store(&s1, |s| s.autosave(doc, revision, path.map(str::to_string))) + with_store(&s1, |s| s.autosave_checked(doc, revision, path.map(str::to_string)))? })), + autosave_results: Some(Box::new(move || with_store(&s4, |s| s.take_completed()).unwrap_or_default())), discard_autosave: Some(Box::new(move |id: u64| { let _ = with_store(&s2, |s| s.discard(id)); })), @@ -575,6 +576,54 @@ mod tests { let _ = std::fs::remove_dir_all(&dir); } + #[test] + fn failed_background_autosave_is_reported_and_retried_without_another_edit() { + let dir = temp("autosave-retry"); + let recovery = dir.join("Recovery"); + // A regular file prevents creation of the recovery bundle directory. + std::fs::write(&recovery, b"blocked").unwrap(); + let ctx = egui::Context::default(); + let mut app = launch(&recovery); + new_doc(&mut app, "#ff0000"); + let revision = app.session.active().unwrap().revision; + autosave(&mut app, &ctx); + + let mut failed = false; + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(30); + while std::time::Instant::now() < deadline { + prefs_ui::tick(&mut app, &ctx); + if app.ui.status.starts_with("Autosave failed:") { + failed = true; + break; + } + std::thread::sleep(std::time::Duration::from_millis(10)); + } + assert!(failed, "background write failures must reach the UI"); + assert!(app.ui.status_error); + assert!(list_recovery(&recovery).is_empty()); + + // No edit occurs: retrying this very same revision must still work. + std::fs::remove_file(&recovery).unwrap(); + std::fs::create_dir(&recovery).unwrap(); + autosave(&mut app, &ctx); + let mut recovered = Vec::new(); + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(30); + while std::time::Instant::now() < deadline { + prefs_ui::tick(&mut app, &ctx); + recovered = list_recovery(&recovery); + if !recovered.is_empty() { + break; + } + std::thread::sleep(std::time::Duration::from_millis(10)); + } + assert_eq!(recovered.len(), 1, "unchanged revision should retry after failure"); + assert_eq!(recovered[0].info.revision, revision); + let restored = photocraft_format::recover(&recovered[0]).unwrap(); + assert_eq!(photocraft_compose::flatten(&restored).px.first().copied(), Some(RED)); + drop(app); + std::fs::remove_dir_all(dir).unwrap(); + } + use std::sync::atomic::{AtomicUsize, Ordering}; fn temp(tag: &str) -> PathBuf { diff --git a/crates/format/src/autosave.rs b/crates/format/src/autosave.rs index 367af9722c..114cdf2c5a 100644 --- a/crates/format/src/autosave.rs +++ b/crates/format/src/autosave.rs @@ -48,6 +48,10 @@ struct Job { opts: SaveOptions, } +/// One completed revision's disk-write outcome (distinct from enqueue success). +type SaveOutcome = (u64, std::result::Result<(), String>); +type CompletedWrites = Arc>>; + /// Background autosaver for one document. Requests are coalesced: if saves /// arrive faster than they complete, only the newest snapshot is written. pub struct Autosaver { @@ -56,6 +60,8 @@ pub struct Autosaver { tx: Option>, handle: Option>, last: Arc>>>, + /// Per-request write outcomes, delivered to the desktop without blocking its frame. + completed: CompletedWrites, } fn sanitize(key: &str) -> String { @@ -70,12 +76,14 @@ impl Autosaver { let key = sanitize(key); let (tx, rx) = mpsc::channel::(); let last = Arc::new(Mutex::new(None)); + let completed = Arc::new(Mutex::new(Vec::new())); let bundle = dir.join(format!("{key}.pcraft")); let sidecar = dir.join(format!("{key}.json")); let last2 = last.clone(); + let completed2 = completed.clone(); // A thread that never started must not look like a working autosave: record the // failure where `last_result`/`flush` report it, so recovery health checks see it. - let spawned = std::thread::Builder::new().name(format!("autosave-{key}")).spawn(move || worker(rx, bundle, sidecar, last2)); + let spawned = std::thread::Builder::new().name(format!("autosave-{key}")).spawn(move || worker(rx, bundle, sidecar, last2, completed2)); let handle = match spawned { Ok(h) => Some(h), Err(e) => { @@ -83,15 +91,22 @@ impl Autosaver { None } }; - Autosaver { dir, key, tx: Some(tx), handle, last } + Autosaver { dir, key, tx: Some(tx), handle, last, completed } } pub fn bundle_path(&self) -> PathBuf { self.dir.join(format!("{}.pcraft", self.key)) } - /// Queue a snapshot for saving (returns immediately). + /// Queue a snapshot for saving (returns immediately). Existing callers that only + /// need best-effort autosaving can use this; the app uses `request_checked`. pub fn request(&self, snapshot: Arc, revision: u64, original_path: Option, opts: SaveOptions) { + let _ = self.request_checked(snapshot, revision, original_path, opts); + } + + /// Queue a snapshot, reporting a worker that could not start or has stopped. + /// Success here means queued, not written: use `take_completed` for write outcomes. + pub fn request_checked(&self, snapshot: Arc, revision: u64, original_path: Option, opts: SaveOptions) -> std::result::Result<(), String> { let info = RecoveryInfo { key: self.key.clone(), document_name: snapshot.name.clone(), @@ -99,9 +114,16 @@ impl Autosaver { saved_at: SystemTime::now().duration_since(UNIX_EPOCH).map(|d| d.as_secs()).unwrap_or(0), revision, }; - if let Some(tx) = &self.tx { - let _ = tx.send(Job { snapshot, info, opts }); - } + self.tx + .as_ref() + .ok_or_else(|| "autosave worker is stopped".to_string())? + .send(Job { snapshot, info, opts }) + .map_err(|_| self.last_result().and_then(std::result::Result::err).unwrap_or_else(|| "autosave worker is unavailable".to_string())) + } + + /// Drain completed write results without waiting for the worker. + pub fn take_completed(&self) -> Vec<(u64, std::result::Result<(), String>)> { + self.completed.lock().map(|mut results| std::mem::take(&mut *results)).unwrap_or_default() } /// Result of the most recent completed save. @@ -136,7 +158,7 @@ impl Drop for Autosaver { } } -fn worker(rx: Receiver, bundle: PathBuf, sidecar: PathBuf, last: Arc>>>) { +fn worker(rx: Receiver, bundle: PathBuf, sidecar: PathBuf, last: Arc>>>, completed: CompletedWrites) { let mut writer = PcraftWriter::new(); while let Ok(mut job) = rx.recv() { // Coalesce: skip to the newest queued snapshot. @@ -149,6 +171,9 @@ fn worker(rx: Receiver, bundle: PathBuf, sidecar: PathBuf, last: Arc, revision: u64, original_path: Option) { + let _ = self.autosave_checked(doc, revision, original_path); + } + + /// Queue a snapshot, reporting enqueue failures to the application. + pub fn autosave_checked(&mut self, doc: &Arc, revision: u64, original_path: Option) -> std::result::Result<(), String> { let id = doc.id.0; let saver = self.savers.entry(id).or_insert_with(|| { let key = self.adopted.get(&id).cloned().unwrap_or_else(|| format!("doc-{}-{id}", self.session)); Autosaver::new(&self.dir, &key) }); - saver.request(doc.clone(), revision, original_path, SaveOptions::default()); + saver.request_checked(doc.clone(), revision, original_path, SaveOptions::default()) + } + + /// Return completed write outcomes as (document id, revision, outcome), without blocking. + pub fn take_completed(&mut self) -> Vec<(u64, u64, std::result::Result<(), String>)> { + self.savers.iter().flat_map(|(&id, saver)| saver.take_completed().into_iter().map(move |(revision, result)| (id, revision, result))).collect() } /// 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 bf601fb85d..95e4e9daf1 100644 --- a/crates/format/tests/autosave.rs +++ b/crates/format/tests/autosave.rs @@ -28,6 +28,42 @@ fn autosave_then_recover() { std::fs::remove_dir_all(dir).unwrap(); } +/// A queued snapshot can fail later; callers must observe and retry that revision. +#[test] +fn background_save_outcomes_report_failure_then_recovery() { + let root = temp_dir("retry-failed-write"); + let blocked = root.join("recovery"); + std::fs::write(&blocked, b"not a directory").unwrap(); + let saver = Autosaver::new(&blocked, "doc"); + let doc = Arc::new(rich_doc(ColorMode::Rgb, SampleType::U8)); + saver.request_checked(doc.clone(), 7, None, SaveOptions::default()).unwrap(); + + let receive = |saver: &Autosaver| { + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(30); + while std::time::Instant::now() < deadline { + if let Some(outcome) = saver.take_completed().into_iter().next() { + return outcome; + } + std::thread::sleep(std::time::Duration::from_millis(10)); + } + panic!("autosave worker did not report its completed write within 30 seconds"); + }; + let (revision, result) = receive(&saver); + assert_eq!(revision, 7); + assert!(result.is_err(), "the recovery location is not a directory"); + + std::fs::remove_file(&blocked).unwrap(); + std::fs::create_dir(&blocked).unwrap(); + saver.request_checked(doc.clone(), 7, None, SaveOptions::default()).unwrap(); + let (revision, result) = receive(&saver); + assert_eq!(revision, 7); + result.unwrap(); + assert_eq!(list_recovery(&blocked).len(), 1); + assert_eq!(recover(&list_recovery(&blocked)[0]).unwrap(), *doc); + drop(saver); + std::fs::remove_dir_all(root).unwrap(); +} + #[test] fn repeated_autosaves_are_incremental_and_coalesced() { let dir = temp_dir("coalesce"); diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index fe439ec477..19c54c305c 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -210,6 +210,8 @@ pub type LoadTextFn = Box Option>; pub type SaveTextFn = Box Result<(), String>>; /// Autosave a document snapshot for crash recovery: (snapshot, revision, original path). pub type AutosaveFn = Box, u64, Option<&str>) -> Result<(), String>>; +/// Poll successful or failed background writes: (document id, revision, result). +pub type AutosaveResultsFn = Box Vec<(u64, u64, Result<(), String>)>>; /// Drop the recovery data of a document (by `DocId` value) once it is saved or closed. pub type DiscardAutosaveFn = Box; /// Load recoverable documents left by a previous session. Their recovery data stays until the @@ -277,6 +279,8 @@ pub struct Services { pub xwayland_command: Option, /// Crash-recovery autosave (Preferences › File Handling) and recovery at launch. pub autosave: Option, + /// None for services that report synchronous success through `autosave`. + pub autosave_results: Option, pub discard_autosave: Option, pub recover: Option, pub adopt_autosave: Option, diff --git a/crates/ui-egui/src/prefs_ui.rs b/crates/ui-egui/src/prefs_ui.rs index 503d6985b5..913df38d77 100644 --- a/crates/ui-egui/src/prefs_ui.rs +++ b/crates/ui-egui/src/prefs_ui.rs @@ -30,7 +30,10 @@ pub struct Runtime { save_retry: SaveRetry, theme_pref: Option, next_autosave_ms: f64, + /// Last revision whose recovery file was actually written successfully. autosaved: HashMap, + /// Queued writes must not masquerade as durable snapshots. + autosave_pending: HashMap, log_len: usize, /// Snapping state of the drag in progress (see `snap_ui`). pub(crate) snap: Option, @@ -368,12 +371,34 @@ fn autosave(app: &mut PhotocraftApp) { if app.services.autosave.is_none() { return; } + // A successful queue operation is not a successful disk write. Poll each frame, + // including frames before the next autosave interval, so failures are visible. + if let Some(results) = app.services.autosave_results.as_mut() { + for (raw_id, revision, result) in results() { + let id = DocId(raw_id); + if app.prefs_rt.autosave_pending.get(&id) != Some(&revision) { + continue; + } + app.prefs_rt.autosave_pending.remove(&id); + match result { + Ok(()) => { + app.prefs_rt.autosaved.insert(id, revision); + } + Err(e) => { + app.ui.status = format!("Autosave failed: {e}"); + app.ui.status_error = true; + } + } + } + } let now = crate::gpu_canvas::now_ms(); // Saved or closed documents drop their recovery data. let live: HashMap = app.session.documents().iter().map(|d| (d.doc.id, d.is_dirty())).collect(); - let stale: Vec = app.prefs_rt.autosaved.keys().filter(|id| live.get(id) != Some(&true)).copied().collect(); + let mut stale: Vec = app.prefs_rt.autosaved.keys().filter(|id| live.get(id) != Some(&true)).copied().collect(); + stale.extend(app.prefs_rt.autosave_pending.keys().filter(|id| live.get(*id) != Some(&true) && !app.prefs_rt.autosaved.contains_key(*id)).copied()); for id in stale { app.prefs_rt.autosaved.remove(&id); + app.prefs_rt.autosave_pending.remove(&id); if let Some(d) = app.services.discard_autosave.as_mut() { d(id.0); } @@ -394,16 +419,24 @@ fn autosave(app: &mut PhotocraftApp) { .session .documents() .iter() - .filter(|d| d.is_dirty() && app.prefs_rt.autosaved.get(&d.doc.id) != Some(&d.revision)) + .filter(|d| d.is_dirty() && app.prefs_rt.autosaved.get(&d.doc.id) != Some(&d.revision) && !app.prefs_rt.autosave_pending.contains_key(&d.doc.id)) .map(|d| (d.doc.clone(), d.revision, d.path.clone())) .collect(); for (doc, rev, path) in jobs { if let Some(save) = app.services.autosave.as_mut() { match save(&doc, rev, path.as_deref()) { Ok(()) => { - app.prefs_rt.autosaved.insert(doc.id, rev); + if app.services.autosave_results.is_some() { + app.prefs_rt.autosave_pending.insert(doc.id, rev); + } else { + // Synchronous or mocked services report completion on return. + app.prefs_rt.autosaved.insert(doc.id, rev); + } + } + Err(e) => { + app.ui.status = format!("Autosave failed: {e}"); + app.ui.status_error = true; } - Err(e) => app.ui.status = format!("Autosave failed: {e}"), } } }