fix: saving, plugin sandboxing and honest failures

crates/app/src/workspace.rs

@@ -836,9 +836,21 @@ impl Workspace {
             cx.background_executor()
                 .timer(std::time::Duration::from_secs(AUTOSAVE_SECS))
                 .await;
-            if this.update(cx, |ws, _| ws.autosave()).is_err() {
+            // Only the snapshot is taken on the main thread -- a handful
+            // of Arc bumps. The encode itself is a full PSD
+            // serialization, composite included, of every dirty tab; it
+            // used to run inside `Entity::update`, which is a hard hitch
+            // every thirty seconds on any large document, seconds of
+            // frozen ui mid-brushstroke.
+            let Ok(jobs) = this.update(cx, |ws, _| ws.autosave_jobs()) else {
                 break;
+            };
+            if jobs.is_empty() {
+                continue;
             }
+            cx.background_executor()
+                .spawn(async move { Workspace::write_autosave_jobs(jobs) })
+                .await;
         })
         .detach();
         // March the selection ants. Eight steps a second is what
@@ -1429,9 +1441,7 @@ impl Workspace {
         let bytes = codec.export(doc)?;
         // Write to a sibling temp file and rename, so an interrupted save
         // can't truncate the user's existing file.
-        let tmp = path.with_extension("schist-tmp");
-        std::fs::write(&tmp, bytes)?;
-        std::fs::rename(&tmp, path)?;
+        schist_core::write_atomically(path, &bytes)?;
         Ok(())
     }
 
@@ -1445,35 +1455,52 @@ impl Workspace {
     /// One snapshot file per open document, so every dirty tab survives a
     /// crash, not just the frontmost one.
     fn recovery_path(&self, id: schist_core::DocumentId) -> Option<PathBuf> {
+        Self::recovery_path_for(id)
+    }
+
+    fn recovery_path_for(id: schist_core::DocumentId) -> Option<PathBuf> {
         Some(Self::recovery_dir()?.join(format!("session-{}-{}.psd", std::process::id(), id.0)))
     }
 
-    /// Write a recovery snapshot for every document with unsaved changes.
-    /// Returns true when at least one snapshot was written.
-    pub fn autosave(&mut self) -> bool {
+    /// What the next autosave has to write: one cheap snapshot per dirty
+    /// document, paired with where it goes.
+    ///
+    /// Taken on the main thread; encoded and written off it.
+    pub fn autosave_jobs(&mut self) -> Vec<(Document, PathBuf)> {
         let dirty: Vec<&Document> = self
             .doc
             .iter()
             .chain(self.background_tabs.iter().map(|t| &t.doc))
             .filter(|d| d.dirty)
             .collect();
         if dirty.is_empty() {
-            return false;
+            return Vec::new();
         }
         let Some(dir) = Self::recovery_dir() else {
-            return false;
+            return Vec::new();
         };
         if let Err(err) = std::fs::create_dir_all(&dir) {
             log::warn!("autosave: cannot create {dir:?}: {err}");
-            return false;
+            return Vec::new();
         }
+        dirty
+            .into_iter()
+            .filter_map(|doc| Some((doc.snapshot_for_export(), Self::recovery_path_for(doc.id)?)))
+            .collect()
+    }
+
+    /// Encode and write the snapshots. Runs on a background thread, so it
+    /// uses the codec directly rather than the registry the workspace
+    /// owns; recovery snapshots are always PSD.
+    pub fn write_autosave_jobs(jobs: Vec<(Document, PathBuf)>) -> bool {
         let mut wrote = false;
-        for doc in dirty {
-            let Some(path) = self.recovery_path(doc.id) else {
-                continue;
-            };
-            match self.write_doc_to(doc, &path) {
-                Ok(()) => {
+        for (doc, path) in jobs {
+            match schist_codec_psd::write_psd(&doc) {
+                Ok(bytes) => {
+                    if let Err(err) = schist_core::write_atomically(&path, &bytes) {
+                        log::warn!("autosave failed: {err:#}");
+                        continue;
+                    }
                     log::debug!("autosaved recovery snapshot to {path:?}");
                     wrote = true;
                 }
@@ -2523,7 +2550,9 @@ impl Workspace {
             let Some(filter) = self.registry.filters().find(|f| f.id() == entry.id) else {
                 continue;
             };
-            filter.apply(buf, w, h, &entry.values);
+            if let Err(err) = filter.try_apply(buf, w, h, &entry.values) {
+                log::warn!("gallery filter {} failed: {err}", entry.id);
+            }
         }
     }
 
@@ -4900,7 +4929,10 @@ impl Workspace {
             let Some(filter) = self.registry.filters().find(|f| f.id() == id) else {
                 return;
             };
-            filter.apply(&mut buf, w, h, values);
+            if let Err(err) = filter.try_apply(&mut buf, w, h, values) {
+                self.status = format!("{} failed: {err}", filter.name()).into();
+                return;
+            }
         }
         self.write_region(
             preview.layer,
@@ -4966,13 +4998,24 @@ impl Workspace {
             return;
         };
         let mut buf = original.clone();
-        let filter = self.registry.filters().find(|f| f.id() == id).unwrap();
-        filter.apply(
+        let Some(filter) = self.registry.filters().find(|f| f.id() == id) else {
+            self.status = "Filter went away".into();
+            return;
+        };
+        // A sandboxed filter can trap or run out of fuel. Recording the
+        // edit anyway put the filter's name in the status bar, marked the
+        // document unsaved and added a no-op history step for pixels that
+        // never changed.
+        if let Err(err) = filter.try_apply(
             &mut buf,
             region.width() as usize,
             region.height() as usize,
             values,
-        );
+        ) {
+            self.status = format!("{name} failed: {err}").into();
+            cx.notify();
+            return;
+        }
         self.write_region(layer_id, region, &original, &buf, &name, true);
         self.status = name.into();
         self.after_change(cx);
@@ -5178,15 +5221,23 @@ impl Workspace {
     /// Enable or disable a third-party plugin.
     pub fn set_plugin_enabled(&mut self, id: String, enabled: bool, cx: &mut Context<Self>) {
         let Some(dir) = schist_plugin_host_wasm::PluginManager::plugin_dir() else {
+            self.status = "No plugin directory to record the change in".into();
+            cx.notify();
             return;
         };
         self.plugins.set_enabled(&id, enabled, &dir);
-        self.status = format!(
-            "{} {} — restart to apply",
-            id,
-            if enabled { "enabled" } else { "disabled" }
-        )
-        .into();
+        // The write was a discarded `let _`, so a read-only or full config
+        // directory reported success and the choice was gone at the next
+        // launch.
+        self.status = match self.plugins.disabled_write_error() {
+            Some(err) => format!("{id}: could not record the change ({err})").into(),
+            None => format!(
+                "{} {} \u{2014} restart to apply",
+                id,
+                if enabled { "enabled" } else { "disabled" }
+            )
+            .into(),
+        };
         cx.notify();
     }
 

crates/core/src/document.rs

@@ -214,6 +214,50 @@ impl Document {
         }
     }
 
+    /// A copy of everything a codec reads, for encoding somewhere else.
+    ///
+    /// Tile maps are copy-on-write, so this is a handful of `Arc` bumps
+    /// rather than a pixel copy. History, the history-brush source and
+    /// the damage list are dropped: nothing exports them, and the first
+    /// is the one part of a document that is genuinely large.
+    ///
+    /// `Document` deliberately does not derive `Clone` -- an accidental
+    /// copy with a duplicate `DocumentId` would be a bad day -- so this
+    /// spells out what a snapshot is for.
+    pub fn snapshot_for_export(&self) -> Document {
+        Document {
+            id: self.id,
+            title: self.title.clone(),
+            path: self.path.clone(),
+            width: self.width,
+            height: self.height,
+            resolution_dpi: self.resolution_dpi,
+            mode: self.mode,
+            depth: self.depth,
+            icc_profile: self.icc_profile.clone(),
+            tree: self.tree.clone(),
+            selection: self.selection.clone(),
+            active_layer: self.active_layer,
+            selected: self.selected.clone(),
+            history: History::new(),
+            preserved_resources: self.preserved_resources.clone(),
+            revision: self.revision,
+            guides: self.guides.clone(),
+            last_selection: self.last_selection.clone(),
+            artboards: self.artboards.clone(),
+            slices: self.slices.clone(),
+            notes: self.notes.clone(),
+            counts: self.counts.clone(),
+            layer_comps: self.layer_comps.clone(),
+            paths: self.paths.clone(),
+            active_path: self.active_path,
+            history_source: Default::default(),
+            saved_selections: self.saved_selections.clone(),
+            damage: Vec::new(),
+            dirty: self.dirty,
+        }
+    }
+
     pub fn undo(&mut self) -> Option<String> {
         let edit = self.history.pop_undo()?;
         for op in edit.ops.iter().rev() {
@@ -1022,6 +1066,55 @@ impl StrokeEdit {
     }
 }
 
+/// The temp path to write beside `path` for an atomic save.
+///
+/// `path.with_extension("schist-tmp")` *replaces* the final extension, so
+/// saving `photo.psd` wrote and renamed `photo.schist-tmp`, destroying any
+/// pre-existing file of that name.
+pub fn temp_save_path(path: &std::path::Path) -> PathBuf {
+    let mut name = path.file_name().unwrap_or_default().to_os_string();
+    name.push(format!(".schist-tmp-{}", std::process::id()));
+    path.with_file_name(name)
+}
+
+/// Write `bytes` to a sibling temp file and rename it over `path`.
+///
+/// The one implementation of this: the interactive save, autosave and the
+/// MCP server each had their own, with two different temp-name schemes
+/// and only one of them flushing.
+///
+/// `rename` is atomic against a process crash but not against power
+/// loss, and it can otherwise reach the disk ahead of the data, so the
+/// file is synced first. The parent directory has to exist already:
+/// creating it meant a mistyped destination silently scattered empty
+/// trees instead of saying the path was wrong.
+pub fn write_atomically(path: &std::path::Path, bytes: &[u8]) -> std::io::Result<()> {
+    use std::io::Write as _;
+    if let Some(parent) = path.parent() {
+        if !parent.as_os_str().is_empty() && !parent.is_dir() {
+            return Err(std::io::Error::new(
+                std::io::ErrorKind::NotFound,
+                format!("{} is not a directory", parent.display()),
+            ));
+        }
+    }
+    let tmp = temp_save_path(path);
+    let write = (|| {
+        let mut file = std::fs::File::create(&tmp)?;
+        file.write_all(bytes)?;
+        file.sync_all()
+    })();
+    if let Err(err) = write {
+        let _ = std::fs::remove_file(&tmp);
+        return Err(err);
+    }
+    if let Err(err) = std::fs::rename(&tmp, path) {
+        let _ = std::fs::remove_file(&tmp);
+        return Err(err);
+    }
+    Ok(())
+}
+
 /// Fill a whole raster layer tilemap region from an RGBA8 buffer
 /// (importer/test convenience; not undoable).
 pub fn blit_rgba8(tiles: &mut TileMap, depth: Depth, rect: IntRect, rgba: &[u8]) {
@@ -1221,4 +1314,76 @@ mod tests {
         assert!(doc.revision > 0, "repaint was requested");
         assert!(!doc.dirty, "but nothing was actually changed");
     }
+
+    #[test]
+    fn the_temp_save_path_does_not_clobber_a_sibling() {
+        use std::path::Path;
+        // `with_extension` *replaces* the final extension, so saving
+        // photo.psd wrote and renamed photo.schist-tmp -- destroying any
+        // pre-existing file of that name.
+        let tmp = temp_save_path(Path::new("/tmp/photos/photo.psd"));
+        let name = tmp.file_name().unwrap().to_str().unwrap();
+        assert!(name.starts_with("photo.psd."), "{name}");
+        assert!(name.contains("schist-tmp"), "{name}");
+        assert_eq!(tmp.parent(), Path::new("/tmp/photos/photo.psd").parent());
+        // Two files that differ only in extension get different temps.
+        let other = temp_save_path(Path::new("/tmp/photos/photo.png"));
+        assert_ne!(tmp, other);
+        // A file with no extension still works.
+        let bare = temp_save_path(Path::new("/tmp/photos/photo"));
+        assert!(bare
+            .file_name()
+            .unwrap()
+            .to_str()
+            .unwrap()
+            .starts_with("photo."));
+    }
+
+    fn scratch(name: &str) -> std::path::PathBuf {
+        let dir = std::env::temp_dir().join(format!("schist-{name}-{}", std::process::id()));
+        let _ = std::fs::remove_dir_all(&dir);
+        std::fs::create_dir_all(&dir).unwrap();
+        dir
+    }
+
+    #[test]
+    fn an_atomic_write_replaces_the_file_and_leaves_nothing_behind() {
+        let dir = scratch("atomic-ok");
+        let target = dir.join("doc.psd");
+        std::fs::write(&target, b"old").unwrap();
+        write_atomically(&target, b"new").unwrap();
+        assert_eq!(std::fs::read(&target).unwrap(), b"new");
+        assert_eq!(std::fs::read_dir(&dir).unwrap().count(), 1, "temp left");
+        let _ = std::fs::remove_dir_all(&dir);
+    }
+
+    #[test]
+    fn a_failed_atomic_write_cleans_its_temp_file_up() {
+        // The rename cannot replace a directory, which is the one failure
+        // that is easy to induce. The half-written temp file must not be
+        // left sitting beside the user's work.
+        let dir = scratch("atomic-fail");
+        let target = dir.join("in-the-way");
+        std::fs::create_dir(&target).unwrap();
+        assert!(write_atomically(&target, b"x").is_err());
+        let leftovers: Vec<_> = std::fs::read_dir(&dir)
+            .unwrap()
+            .flatten()
+            .map(|e| e.file_name())
+            .filter(|n| n.to_string_lossy().contains("schist-tmp"))
+            .collect();
+        assert!(leftovers.is_empty(), "temp file left behind: {leftovers:?}");
+        let _ = std::fs::remove_dir_all(&dir);
+    }
+
+    #[test]
+    fn an_atomic_write_refuses_a_missing_directory() {
+        // Creating it meant a mistyped destination silently built the
+        // whole tree instead of saying the path was wrong.
+        let dir = scratch("atomic-missing");
+        let missing = dir.join("no/such/place/doc.psd");
+        assert!(write_atomically(&missing, b"x").is_err());
+        assert!(!dir.join("no").exists(), "nothing should have been created");
+        let _ = std::fs::remove_dir_all(&dir);
+    }
 }

crates/core/src/lib.rs

@@ -20,7 +20,8 @@ pub mod tile;
 pub use annotate::{Artboard, CountGroup, LayerComp, LayerCompState, Note, Slice};
 pub use blend::BlendMode;
 pub use document::{
-    blit_rgba8, Document, DocumentId, EditBuilder, Guide, PreservedResource, StrokeEdit,
+    blit_rgba8, temp_save_path, write_atomically, Document, DocumentId, EditBuilder, Guide,
+    PreservedResource, StrokeEdit,
 };
 pub use geom::IntRect;
 pub use history::{Edit, EditOp, History, LayerProps};