Recovery: tests hold the background writer instead of sleeping

The timer test now holds the recovery writer at the start of its write
through a handshake (`recovery::testing`, behind the engine's `testing`
feature), checks that frames and edits go on while the write is held,
and fails when the write runs on the interface thread. The engine tests
wait for the writer to go idle instead of polling with sleeps.

`file.recovery.save` rejects a `background` that is not true or false.

crates/engine/Cargo.toml

@@ -8,6 +8,10 @@ description = "DesignCraft engine: session, documents, history, selection, comma
 [lints]
 workspace = true
 
+[features]
+# Recovery-writer handshakes for other crates' tests (`designcraft_engine::recovery::testing`).
+testing = []
+
 [dependencies]
 craft-print = { workspace = true }
 craft-docpath = { workspace = true }

crates/engine/src/cmd/file/registry.rs

@@ -70,8 +70,12 @@ pub fn specs() -> Vec<CommandSpec> {
         cmd!(noundo "file.recovery.save", "Save Recovery Data", [], None,
             "{dir?, background?} — write every unsaved document to the recovery folder (the app does this on a timer; `background` queues the writes and returns at once) → {saved} or {queued}",
             always, |s, p| {
+            let background = match p.get("background") {
+                None | Some(Value::Null) => false,
+                Some(v) => v.as_bool().ok_or_else(|| bad("file.recovery.save", "`background` must be true or false"))?,
+            };
             let dir = recovery_dir(s, p, FolderAccess::Write)?;
-            if p.get("background").and_then(Value::as_bool).unwrap_or(false) {
+            if background {
                 return Ok(json!({"queued": crate::recovery::start_tracked(s, &dir)?}));
             }
             Ok(json!({"saved": crate::recovery::save_tracked(s, &dir)?}))

crates/engine/src/recovery.rs

@@ -679,6 +679,8 @@ struct Written {
 /// document was saved, reverted or closed since the write started: its snapshot is then dropped
 /// instead of published, so a crash cannot offer back a copy older than the saved file.
 fn write_snapshot(entry: &Snapshot, cancelled: impl Fn() -> bool) -> Written {
+    #[cfg(any(test, feature = "testing"))]
+    testing::gate(&entry.dir);
     let (doc, meta) = files(&entry.dir, entry.uid);
     let published = publish_recovery_written(
         &doc,
@@ -812,7 +814,7 @@ pub(crate) fn finished(_s: &mut Session) -> Finished {
 mod worker {
     use std::collections::BTreeSet;
     use std::sync::mpsc::{Receiver, Sender};
-    use std::sync::{Mutex, OnceLock};
+    use std::sync::{Condvar, Mutex, OnceLock};
 
     use super::{Path, PathBuf, Result, Snapshot, write_snapshot};
 
@@ -834,6 +836,8 @@ mod worker {
         /// A document saved, reverted or closed while its write was queued or running: the
         /// snapshot it is producing must not be published.
         cancelled: Mutex<BTreeSet<(PathBuf, u64)>>,
+        /// Notified each time a write finishes and leaves `running`.
+        idle: Condvar,
     }
 
     static WRITER: OnceLock<Option<Writer>> = OnceLock::new();
@@ -851,6 +855,16 @@ mod worker {
             if let Ok(mut cancelled) = self.cancelled.lock() {
                 cancelled.remove(&key);
             }
+            self.idle.notify_all();
+        }
+
+        /// Block until no write is queued or running.
+        #[cfg(any(test, feature = "testing"))]
+        pub(super) fn wait_until_idle(&self) {
+            let mut running = self.running.lock().unwrap_or_else(std::sync::PoisonError::into_inner);
+            while !running.is_empty() {
+                running = self.idle.wait(running).unwrap_or_else(std::sync::PoisonError::into_inner);
+            }
         }
 
         /// Do not publish the snapshot being written for `dir`/`uid`: the document was saved,
@@ -898,6 +912,7 @@ mod worker {
                     done: Mutex::new(done_rx),
                     running: Mutex::new(BTreeSet::new()),
                     cancelled: Mutex::new(BTreeSet::new()),
+                    idle: Condvar::new(),
                 })
             })
             .as_ref()
@@ -959,6 +974,107 @@ mod worker {
     }
 }
 
+/// Handshakes for tests of the recovery writer, in this crate and, with the `testing` feature, in
+/// the app's: hold a write at its start, and wait for the writer to finish.
+#[cfg(any(test, feature = "testing"))]
+pub mod testing {
+    use std::path::{Path, PathBuf};
+    use std::sync::atomic::{AtomicBool, Ordering};
+    use std::sync::mpsc::{Receiver, Sender};
+    use std::sync::{Arc, Mutex, PoisonError};
+    use std::thread::ThreadId;
+
+    struct Gate {
+        dir: PathBuf,
+        holder: ThreadId,
+        entered: Sender<()>,
+        go: Receiver<()>,
+        ran_on_holder: Arc<AtomicBool>,
+    }
+
+    static GATE: Mutex<Option<Gate>> = Mutex::new(None);
+
+    /// The next recovery write into one folder, held at its start until this is released or
+    /// dropped.
+    pub struct HeldWrite {
+        entered: Receiver<()>,
+        go: Option<Sender<()>>,
+        ran_on_holder: Arc<AtomicBool>,
+    }
+
+    /// Hold the next recovery write into `dir`. A write on another thread waits before it writes
+    /// anything; a write on the calling thread does not wait, and [`HeldWrite::ran_here`] reports it.
+    pub fn hold_next_write(dir: &Path) -> HeldWrite {
+        let (entered_tx, entered) = std::sync::mpsc::channel();
+        let (go, go_rx) = std::sync::mpsc::channel();
+        let ran_on_holder = Arc::new(AtomicBool::new(false));
+        let gate = Gate {
+            dir: dir.to_path_buf(),
+            holder: std::thread::current().id(),
+            entered: entered_tx,
+            go: go_rx,
+            ran_on_holder: ran_on_holder.clone(),
+        };
+        *GATE.lock().unwrap_or_else(PoisonError::into_inner) = Some(gate);
+        HeldWrite { entered, go: Some(go), ran_on_holder }
+    }
+
+    impl HeldWrite {
+        /// The held write ran on the thread that called [`hold_next_write`].
+        pub fn ran_here(&self) -> bool {
+            self.ran_on_holder.load(Ordering::SeqCst)
+        }
+
+        /// Block until the write has reached the gate.
+        pub fn wait_until_held(&self) {
+            let _ = self.entered.recv();
+        }
+
+        /// Let the held write go on.
+        pub fn release(mut self) {
+            if let Some(go) = self.go.take() {
+                let _ = go.send(());
+            }
+        }
+    }
+
+    impl Drop for HeldWrite {
+        fn drop(&mut self) {
+            // A gate no write reached is removed, so it cannot hold a later test's write.
+            let mut slot = GATE.lock().unwrap_or_else(PoisonError::into_inner);
+            if slot.as_ref().is_some_and(|gate| Arc::ptr_eq(&gate.ran_on_holder, &self.ran_on_holder)) {
+                *slot = None;
+            }
+        }
+    }
+
+    /// Called at the start of every recovery write.
+    pub(super) fn gate(dir: &Path) {
+        let gate = {
+            let mut slot = GATE.lock().unwrap_or_else(PoisonError::into_inner);
+            if slot.as_ref().is_some_and(|gate| gate.dir == dir) { slot.take() } else { None }
+        };
+        let Some(gate) = gate else { return };
+        if std::thread::current().id() == gate.holder {
+            gate.ran_on_holder.store(true, Ordering::SeqCst);
+            let _ = gate.entered.send(());
+            return;
+        }
+        let _ = gate.entered.send(());
+        // A dropped `HeldWrite` closes the channel, which also lets the write go on.
+        let _ = gate.go.recv();
+    }
+
+    /// Block until the background writer has no write queued or running.
+    pub fn wait_until_idle() {
+        // Nothing is written in the background on the web.
+        #[cfg(not(target_arch = "wasm32"))]
+        if let Some(w) = super::worker::started() {
+            w.wait_until_idle();
+        }
+    }
+}
+
 #[cfg(not(target_arch = "wasm32"))]
 fn secure_recovery_dir(dir: &Path) -> Result<bool> {
     match std::fs::symlink_metadata(dir) {

crates/engine/src/recovery/tests.rs

@@ -135,20 +135,49 @@ fn background() -> std::sync::MutexGuard<'static, ()> {
     BACKGROUND.lock().unwrap_or_else(std::sync::PoisonError::into_inner)
 }
 
-/// Poll `file.recovery.progress` until the background writer has nothing left, as the app does at
-/// the end of a frame.
+/// Wait for the background writer to have nothing left, then read `file.recovery.progress` as the
+/// app does at the end of a frame.
 fn finished_writes(s: &mut Session) -> usize {
-    let mut written = 0;
-    for _ in 0..1200 {
-        let v = s.execute("file.recovery.progress", &json!({})).unwrap();
-        assert_eq!(v["errors"], json!([]), "{v}");
-        written += v["written"].as_u64().unwrap_or(0) as usize;
-        if !v["busy"].as_bool().unwrap_or(false) {
-            return written;
-        }
-        std::thread::sleep(std::time::Duration::from_millis(5));
-    }
-    panic!("the background writer did not finish");
+    testing::wait_until_idle();
+    let v = s.execute("file.recovery.progress", &json!({})).unwrap();
+    assert_eq!(v["errors"], json!([]), "{v}");
+    assert_eq!(v["busy"], false, "{v}");
+    v["written"].as_u64().unwrap_or(0) as usize
+}
+
+#[cfg(not(target_arch = "wasm32"))]
+#[test]
+fn a_background_save_returns_while_its_write_is_held() {
+    let _turn = background();
+    let dir = tempfile::tempdir().unwrap();
+    let mut s = dirty_session();
+    s.recovery_dir = Some(dir.path().to_path_buf());
+    let uid = s.docs[0].uid;
+    let (file, _) = files(dir.path(), uid);
+    let held = testing::hold_next_write(dir.path());
+    let queued = s.execute("file.recovery.save", &json!({"background": true})).unwrap();
+    assert!(!held.ran_here(), "the write ran on the calling thread");
+    assert_eq!(queued["queued"], 1, "{queued}");
+    // The writer thread is stopped at the start of the write, and the session goes on.
+    held.wait_until_held();
+    let v = s.execute("file.recovery.progress", &json!({})).unwrap();
+    assert_eq!(v["busy"], true, "{v}");
+    s.execute("frame.create", &json!({"rect": [20, 20, 60, 60]})).unwrap();
+    assert!(!file.exists(), "nothing is published while the write is held");
+    held.release();
+    assert_eq!(finished_writes(&mut s), 1);
+    assert!(file.exists());
+    discard(dir.path(), uid);
+}
+
+#[test]
+fn a_background_save_rejects_a_background_that_is_not_true_or_false() {
+    let dir = tempfile::tempdir().unwrap();
+    let mut s = dirty_session();
+    s.recovery_dir = Some(dir.path().to_path_buf());
+    let err = s.execute("file.recovery.save", &json!({"background": "yes"})).unwrap_err();
+    assert!(err.to_string().contains("`background` must be true or false"), "{err}");
+    assert!(list(dir.path()).is_empty(), "nothing was written");
 }
 
 #[test]
@@ -242,17 +271,10 @@ fn a_background_snapshot_whose_metadata_failed_is_still_the_documents_to_remove(
     std::fs::create_dir(&metadata).unwrap();
     let queued = s.execute("file.recovery.save", &json!({"background": true})).unwrap();
     assert_eq!(queued["queued"], 1, "{queued}");
-    let mut reported = None;
-    for _ in 0..1200 {
-        let v = s.execute("file.recovery.progress", &json!({})).unwrap();
-        if let Some(first) = v["errors"].as_array().and_then(|errors| errors.first()) {
-            reported = first.as_str().map(str::to_string);
-        }
-        if !v["busy"].as_bool().unwrap_or(false) {
-            break;
-        }
-        std::thread::sleep(std::time::Duration::from_millis(5));
-    }
+    testing::wait_until_idle();
+    let v = s.execute("file.recovery.progress", &json!({})).unwrap();
+    assert_eq!(v["busy"], false, "{v}");
+    let reported = v["errors"].as_array().and_then(|errors| errors.first()).and_then(Value::as_str).map(str::to_string);
     let reported = reported.expect("the metadata failure is reported");
     assert!(reported.contains(&metadata.display().to_string()), "{reported}");
     assert!(document.exists(), "the primary landed");

crates/ui-egui/Cargo.toml

@@ -44,6 +44,7 @@ zip = { workspace = true }
 # Headless (offscreen wgpu) rendering of the whole window for UI screenshots: `examples/ui_shot.rs`.
 egui_kittest = { workspace = true, features = ["wgpu", "snapshot"] }
 designcraft-fonts = { workspace = true, features = ["testing"] }
+designcraft-engine = { workspace = true, features = ["testing"] }
 
 [build-dependencies]
 serde_json = { workspace = true }

crates/ui-egui/src/tests.rs

@@ -151,18 +151,30 @@ fn the_recovery_timer_writes_off_the_interface_thread() {
     app.last_recovery = -1.0e9;
     let uid = app.session.documents()[0].uid;
     let entry = dir.path().join(format!("{uid}.designcraft"));
-    let mut written = false;
-    for _ in 0..600 {
+    // The writer stops at the start of the write until the test lets it go on, so a frame that
+    // waited for the write would never return.
+    let held = designcraft_engine::recovery::testing::hold_next_write(dir.path());
+    // egui discards a pass that needs another layout; the timer runs in the first pass it keeps.
+    for _ in 0..10 {
         shot_frame(&mut app, &ctx, egui::vec2(1400.0, 900.0));
-        // The timer fired and the write finished: the file is there and nothing is still queued.
-        if entry.exists() && !app.recovery_writing {
-            written = true;
+        if app.last_recovery > 0.0 {
             break;
         }
-        std::thread::sleep(std::time::Duration::from_millis(5));
     }
-    assert!(written, "the timer's background write published the document");
-    assert!(app.last_recovery > 0.0, "the timer restarted its interval");
+    assert!(app.last_recovery > 0.0, "the timer fired and restarted its interval");
+    assert!(!held.ran_here(), "the timer wrote the recovery data on the interface thread");
+    assert!(app.recovery_writing, "the timer queued the write");
+    held.wait_until_held();
+    // While the write is held, the interface keeps drawing frames and taking edits.
+    app.run("frame.create", json!({"rect": [20, 20, 80, 80]})).unwrap();
+    shot_frame(&mut app, &ctx, egui::vec2(1400.0, 900.0));
+    assert!(app.recovery_writing, "the write is still held");
+    assert!(!entry.exists(), "nothing is published while the write is held");
+    held.release();
+    designcraft_engine::recovery::testing::wait_until_idle();
+    shot_frame(&mut app, &ctx, egui::vec2(1400.0, 900.0));
+    assert!(!app.recovery_writing, "the frame after the write took its result");
+    assert!(entry.exists(), "the timer's background write published the document");
     // The checkpoint is the document's: saving it removes the recovery entry.
     let saved = dir.path().join("saved.designcraft");
     app.run("file.saveAs", json!({"path": saved.to_string_lossy()})).unwrap();