diff --git a/crates/engine/Cargo.toml b/crates/engine/Cargo.toml index c1c0f1d33..6b6a43a87 100644 --- a/crates/engine/Cargo.toml +++ b/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 } diff --git a/crates/engine/src/cmd/file/registry.rs b/crates/engine/src/cmd/file/registry.rs index a9d14892c..df8709f10 100644 --- a/crates/engine/src/cmd/file/registry.rs +++ b/crates/engine/src/cmd/file/registry.rs @@ -70,8 +70,12 @@ pub fn specs() -> Vec { 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)?})) diff --git a/crates/engine/src/recovery.rs b/crates/engine/src/recovery.rs index a84034fb7..420d1795f 100644 --- a/crates/engine/src/recovery.rs +++ b/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>, + /// Notified each time a write finishes and leaves `running`. + idle: Condvar, } static WRITER: OnceLock> = 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, + } + + static GATE: Mutex> = 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>, + ran_on_holder: Arc, + } + + /// 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 { match std::fs::symlink_metadata(dir) { diff --git a/crates/engine/src/recovery/tests.rs b/crates/engine/src/recovery/tests.rs index c1a742a31..3b7c2303d 100644 --- a/crates/engine/src/recovery/tests.rs +++ b/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"); diff --git a/crates/ui-egui/Cargo.toml b/crates/ui-egui/Cargo.toml index 6b08420a9..f7fe65d8e 100644 --- a/crates/ui-egui/Cargo.toml +++ b/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 } diff --git a/crates/ui-egui/src/tests.rs b/crates/ui-egui/src/tests.rs index 17cc20d28..8f2eef43d 100644 --- a/crates/ui-egui/src/tests.rs +++ b/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();