Merge refs/tmp/p1577

apps/photocraft/src/services.rs

@@ -158,11 +158,12 @@ fn with_store<R>(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<PathBuf>) -> 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<Document>, 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 {

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<Mutex<Vec<SaveOutcome>>>;
+
 /// 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<Sender<Job>>,
     handle: Option<JoinHandle<()>>,
     last: Arc<Mutex<Option<Result<SaveStats>>>>,
+    /// 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::<Job>();
         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<Document>, revision: u64, original_path: Option<String>, 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<Document>, revision: u64, original_path: Option<String>, 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<Job>, bundle: PathBuf, sidecar: PathBuf, last: Arc<Mutex<Option<Result<SaveStats>>>>) {
+fn worker(rx: Receiver<Job>, bundle: PathBuf, sidecar: PathBuf, last: Arc<Mutex<Option<Result<SaveStats>>>>, 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<Job>, bundle: PathBuf, sidecar: PathBuf, last: Arc<Mutex<
             write_atomic(&sidecar, &serde_json::to_vec_pretty(&job.info)?)?;
             Ok(stats)
         })();
+        if let Ok(mut results) = completed.lock() {
+            results.push((job.info.revision, r.as_ref().map(|_| ()).map_err(ToString::to_string)));
+        }
         if let Ok(mut g) = last.lock() {
             *g = Some(r);
         }
@@ -231,12 +256,22 @@ impl RecoveryStore {
 
     /// Queue an autosave of `doc` (returns immediately; see [`Autosaver::request`]).
     pub fn autosave(&mut self, doc: &Arc<Document>, revision: u64, original_path: Option<String>) {
+        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<Document>, revision: u64, original_path: Option<String>) -> 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

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");

crates/ui-egui/src/lib.rs

@@ -210,6 +210,8 @@ pub type LoadTextFn = Box<dyn FnMut() -> Option<String>>;
 pub type SaveTextFn = Box<dyn FnMut(&str) -> Result<(), String>>;
 /// Autosave a document snapshot for crash recovery: (snapshot, revision, original path).
 pub type AutosaveFn = Box<dyn FnMut(&std::sync::Arc<Document>, u64, Option<&str>) -> Result<(), String>>;
+/// Poll successful or failed background writes: (document id, revision, result).
+pub type AutosaveResultsFn = Box<dyn FnMut() -> 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<dyn FnMut(u64)>;
 /// 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<String>,
     /// Crash-recovery autosave (Preferences › File Handling) and recovery at launch.
     pub autosave: Option<AutosaveFn>,
+    /// None for services that report synchronous success through `autosave`.
+    pub autosave_results: Option<AutosaveResultsFn>,
     pub discard_autosave: Option<DiscardAutosaveFn>,
     pub recover: Option<RecoverFn>,
     pub adopt_autosave: Option<AdoptAutosaveFn>,

crates/ui-egui/src/prefs_ui.rs

@@ -30,7 +30,10 @@ pub struct Runtime {
     save_retry: SaveRetry,
     theme_pref: Option<Theme>,
     next_autosave_ms: f64,
+    /// Last revision whose recovery file was actually written successfully.
     autosaved: HashMap<DocId, u64>,
+    /// Queued writes must not masquerade as durable snapshots.
+    autosave_pending: HashMap<DocId, u64>,
     log_len: usize,
     /// Snapping state of the drag in progress (see `snap_ui`).
     pub(crate) snap: Option<crate::snap_ui::ActiveSnap>,
@@ -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<DocId, bool> = app.session.documents().iter().map(|d| (d.doc.id, d.is_dirty())).collect();
-    let stale: Vec<DocId> = app.prefs_rt.autosaved.keys().filter(|id| live.get(id) != Some(&true)).copied().collect();
+    let mut stale: Vec<DocId> = 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}"),
             }
         }
     }