From 68a2030614404b8c59d4bc93243e72346b3f9a3d Mon Sep 17 00:00:00 2001 From: francesco fantoni Date: Tue, 3 Nov 2026 23:30:53 +0100 Subject: [PATCH 01/14] fix(engine): write automatic recovery off the interface thread MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 30 s timer ran file.recovery.save on the UI thread, and designcraft_format::save() deflated the entire package — every linked placed file included — into a Vec in RAM: 3.6 GB and 30-100 s of zlib inside DesignApp::ui() per autosave on a 190-page catalogue with 4.2 GB of links. - file.recovery.save {background: true} snapshots the dirty documents (Arc clones; no bytes copied) and queues them on one "designcraft-recovery" thread, skipping a document whose previous write is still in flight. file.recovery.progress reports {written, busy, errors}; the app's timer uses it and polls from ui_frame_end while a write is outstanding. The synchronous command is unchanged for MCP, scripts and tests, and the web build still writes inline (it has one thread). - designcraft_format gains write(), streaming the package to any Write + Seek, so a large save needs no second copy of the archive. - A snapshot still queued or running when its document is saved, reverted or closed is cancelled before its atomic rename; a write that lands after the poll is discarded, so a crash cannot offer back a copy older than the saved file. A panicking write is the job's error instead of ending the thread and leaving the document stuck as running. Tests: the_recovery_timer_writes_off_the_interface_thread; recovery::a_background_save_writes_recovery_data_the_app_can_recover, a_saved_document_is_not_written_in_the_background, a_snapshot_whose_metadata_failed_is_still_the_documents_to_remove, a_background_snapshot_whose_metadata_failed_is_still_the_documents_to_remove, a_write_cancelled_midway_publishes_nothing, only_a_queued_write_is_cancelled, a_panicking_recovery_write_is_an_error_and_the_writer_keeps_going; format::writing_to_a_stream_makes_the_same_file_as_saving_in_memory. Gates: cargo fmt --all --check; clippy -p designcraft-engine -p designcraft-format -p designcraft-ui-egui --all-targets -D warnings; cargo test -p designcraft-engine -p designcraft-format. Part of #1923, split for review: leaving linked placed files out of snapshots is a separate pull request on top of this one. --- ROADMAP.md | 6 + crates/engine/src/cmd/file/registry.rs | 12 +- crates/engine/src/recovery.rs | 424 +++++++++++++++++++++++-- crates/engine/src/recovery/tests.rs | 188 +++++++++++ crates/format/src/lib.rs | 22 +- crates/format/src/tests.rs | 18 ++ crates/ui-egui/src/app.rs | 1 + crates/ui-egui/src/app/frame.rs | 40 ++- crates/ui-egui/src/lib.rs | 3 + crates/ui-egui/src/tests.rs | 29 ++ docs/architecture.md | 7 +- docs/gaps.md | 8 +- 12 files changed, 711 insertions(+), 47 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index 963b8b855..34ce00129 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -13,6 +13,12 @@ DesignCraft aims at full Adobe InDesign parity — and to be better: faster, open (documented JSON format + IDML), scriptable by agents (MCP), and available on the web. - **2026-10-30** · Document Fonts lookup treats a bare document filename's empty parent as the current directory (#2014), preserving case-insensitive discovery and automation read-root checks. No readiness percentage change. +- **2026-10-27** · Automatic recovery writes off the interface thread, straight to disk: on a + 190-page catalogue with 4.2 GB of placed files an autosave was 3.6 GB deflated in RAM on the + interface thread, which stopped the window answering for 30-100 s after an edit. It is now a + background write (`file.recovery.save {background}`, `file.recovery.progress`), the caller + blocked for 109 ms instead, and Save, Close and Revert cancel a snapshot still being written. + No readiness percentage change. - **2026-10-28** · Inline table rows leave room for column and spanning footnotes, continuing in the next column or frame when needed (#1901). Cell-local footnotes contribute to row height and sit inside the cell rather than outside its measuring frame. Vertical justification keeps diff --git a/crates/engine/src/cmd/file/registry.rs b/crates/engine/src/cmd/file/registry.rs index c1f522956..e3a21213e 100644 --- a/crates/engine/src/cmd/file/registry.rs +++ b/crates/engine/src/cmd/file/registry.rs @@ -67,10 +67,20 @@ pub fn specs() -> Vec { let dir = recovery_dir(s, p, FolderAccess::Read)?; Ok(Value::Array(crate::recovery::list(&dir).into_iter().map(|(uid, mut m)| { m["uid"] = json!(uid); m }).collect())) }), - cmd!(noundo "file.recovery.save", "Save Recovery Data", [], None, "{dir?} — write every unsaved document to the recovery folder (the app does this on a timer)", always, |s, p| { + 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 dir = recovery_dir(s, p, FolderAccess::Write)?; + if p.get("background").and_then(Value::as_bool).unwrap_or(false) { + return Ok(json!({"queued": crate::recovery::start_tracked(s, &dir)?})); + } Ok(json!({"saved": crate::recovery::save_tracked(s, &dir)?})) }), + cmd!(query "file.recovery.progress", "Recovery Progress", [], None, + "{} → {written, busy, errors} recovery data queued in the background has been written", always, |s, _| { + let done = crate::recovery::finished(s); + Ok(json!({"written": done.written, "busy": done.busy, "errors": done.errors})) + }), cmd!(noundo "file.recovery.open", "Recover Documents", [], None, "{dir?} — reopen documents left in the recovery folder (unsaved)", always, |s, p| { // Reopening removes the entries it read. let dir = recovery_dir(s, p, FolderAccess::ReadWrite)?; diff --git a/crates/engine/src/recovery.rs b/crates/engine/src/recovery.rs index eb6d99886..4598ae397 100644 --- a/crates/engine/src/recovery.rs +++ b/crates/engine/src/recovery.rs @@ -477,6 +477,9 @@ pub(crate) fn discard_document(s: &mut Session, index: usize, default_dir: Optio if !s.docs.iter().enumerate().any(|(other_index, owner)| { other_index != index && owner.recovery_entries.iter().any(|(entry_dir, entry_uid)| *entry_uid == uid && same_directory(entry_dir, &dir)) }) { + // A write of it still running must not publish its snapshot after this point. + #[cfg(not(target_arch = "wasm32"))] + worker::cancel(&dir, uid); discard(&dir, uid); } } @@ -528,7 +531,10 @@ pub fn save(s: &Session, dir: &Path) -> Result { discard(dir, d.uid); continue; } - write_entry(&access, dir, d)?; + // The caller asked for this write now: nothing has cancelled it. + if let Some(e) = write_snapshot(&Snapshot::of(&access, dir, d), || false).error { + return Err(e); + } n += 1; } Ok(n) @@ -537,14 +543,7 @@ pub fn save(s: &Session, dir: &Path) -> Result { /// Command-path checkpointing also records explicitly selected directories, without configuring /// automatic recovery. The immutable public helper remains available for fixtures and callers. pub(crate) fn save_tracked(s: &mut Session, dir: &Path) -> Result { - // Reject the complete batch before writing rather than silently losing old ownership records. - for document in &s.docs { - if document.is_dirty() && !has_checkpoint(document, dir) && document.recovery_entries.len() >= MAX_RECOVERY_ENTRIES { - return Err(EngineError::Other( - "too many recovery checkpoints; save or close the document before choosing another recovery folder".into(), - )); - } - } + reject_overfull_checkpoints(s, dir)?; let access = prepare_save(s, dir)?; let mut n = 0; let preview = s.new_document_preview(); @@ -557,34 +556,361 @@ pub(crate) fn save_tracked(s: &mut Session, dir: &Path) -> Result { discard_document(s, index, Some(dir)); continue; } - let Some(document) = s.docs.get_mut(index) else { continue }; - let (doc, meta) = files(dir, document.uid); - let bytes = designcraft_format::save(document.preview_stash.as_deref().unwrap_or(document.doc.as_ref())) - .map_err(|e| EngineError::Other(e.to_string()))?; - // An entry this run did not write is never replaced, even one made since it checked. - let replace = access.owns(document.uid); - publish_recovery_file(&doc, &bytes, replace)?; - access.record(document.uid); - // The primary is useful even if optional metadata fails next. Track it immediately. - if !has_checkpoint(document, dir) { + let Some(document) = s.docs.get(index) else { continue }; + let entry = Snapshot::of(&access, dir, document); + let written = write_snapshot(&entry, || false); + // The primary is the recovery data; its metadata is optional. Track the primary before + // returning a metadata error, so Save and Close remove the snapshot this run wrote. + if written.published + && let Some(document) = s.docs.get_mut(index) + && !has_checkpoint(document, dir) + { document.recovery_entries.push((dir.to_path_buf(), document.uid)); } - let metadata = json!({"path": document.path, "title": document.title(), "saved": designcraft_doc::vars::now()}); - publish_recovery_file(&meta, metadata.to_string().as_bytes(), replace)?; + if let Some(e) = written.error { + return Err(e); + } n += 1; } Ok(n) } -/// Publish a document's recovery entry before its optional metadata. -fn write_entry(access: &Access, dir: &Path, d: &DocState) -> Result<()> { - let (doc, meta) = files(dir, d.uid); - let bytes = designcraft_format::save(d.preview_stash.as_deref().unwrap_or(d.doc.as_ref())).map_err(|e| EngineError::Other(e.to_string()))?; - let replace = access.owns(d.uid); - publish_recovery_file(&doc, &bytes, replace)?; - access.record(d.uid); - let metadata = json!({"path": d.path, "title": d.title(), "saved": designcraft_doc::vars::now()}); - publish_recovery_file(&meta, metadata.to_string().as_bytes(), replace) +/// Reject a complete batch before writing rather than silently losing old ownership records. +fn reject_overfull_checkpoints(s: &Session, dir: &Path) -> Result<()> { + for document in &s.docs { + if document.is_dirty() && !has_checkpoint(document, dir) && document.recovery_entries.len() >= MAX_RECOVERY_ENTRIES { + return Err(EngineError::Other( + "too many recovery checkpoints; save or close the document before choosing another recovery folder".into(), + )); + } + } + Ok(()) +} + +/// A document's recovery write, copied so a background thread can write it while the interface +/// stays responsive: no deflate, no copy of the placed files, happens on the interface thread. +struct Snapshot { + dir: PathBuf, + uid: u64, + /// The document as it was when the write started (data-merge previews save the unfilled one). + doc: std::sync::Arc, + path: Option, + title: String, + /// Whether this run may replace an entry already in the folder. + own: bool, + /// The folder's canonical key, for recording that a shared entry is this run's now. + key: PathBuf, +} + +impl Snapshot { + fn of(access: &Access, dir: &Path, d: &DocState) -> Self { + Snapshot { + dir: dir.to_path_buf(), + uid: d.uid, + doc: d.preview_stash.clone().unwrap_or_else(|| d.doc.clone()), + path: d.path.clone(), + title: d.title(), + // An entry this run did not write is never replaced, even one made since it checked. + own: access.owns(d.uid), + key: access.key.clone(), + } + } +} + +/// What one recovery write left in the folder: whether the primary document was published, and +/// the error that stopped it or the optional metadata written after it. The primary is the +/// recovery data (see the module docs: metadata may be missing or stale), so a metadata failure +/// must not lose the checkpoint that lets Save and Close remove what this run wrote. +struct Written { + published: bool, + error: Option, +} + +/// Publish a document's recovery entry before its optional metadata. `cancelled` says whether the +/// 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 { + let (doc, meta) = files(&entry.dir, entry.uid); + let published = publish_recovery_written( + &doc, + entry.own, + |out| designcraft_format::write(out, &entry.doc, None).map_err(|e| EngineError::Other(e.to_string())), + &cancelled, + ); + match published { + Ok(true) => {} + // Cancelled while writing: no snapshot and no metadata for it. + Ok(false) => return Written { published: false, error: None }, + Err(e) => return Written { published: false, error: Some(e) }, + } + // Written: in a folder that is not this run's own, the entry is now this run's to replace and + // remove (Save and Close must clean up the snapshot they wrote). + if !entry.own { + WRITTEN.lock().unwrap_or_else(std::sync::PoisonError::into_inner).insert((entry.key.clone(), entry.uid)); + } + let metadata = json!({"path": entry.path, "title": entry.title, "saved": designcraft_doc::vars::now()}); + Written { published: true, error: publish_recovery_file(&meta, metadata.to_string().as_bytes(), entry.own).err() } +} + +/// Write recovery entries for dirty documents on a background thread and drop clean checkpoints +/// now. The interface thread only snapshots what to write: `designcraft_format::write` deflates +/// and copies placed files, which takes minutes on a catalogue with gigabytes of links. Returns +/// how many documents were queued. +pub(crate) fn start_tracked(s: &mut Session, dir: &Path) -> Result { + reject_overfull_checkpoints(s, dir)?; + let access = prepare_save(s, dir)?; + // Nothing runs on the web's single thread: it would freeze the page just the same. + #[cfg(target_arch = "wasm32")] + { + let _ = access; + return save_tracked(s, dir); + } + #[cfg(not(target_arch = "wasm32"))] + { + let mut queued = 0; + let preview = s.new_document_preview(); + for index in 0..s.docs.len() { + if s.docs.get(index).is_some_and(|document| preview == Some(document.uid)) { + continue; + } + if s.docs.get(index).is_some_and(|document| !document.is_dirty()) { + discard_document(s, index, Some(dir)); + continue; + } + let Some(document) = s.docs.get(index) else { continue }; + let entry = Snapshot::of(&access, dir, document); + // A write still running for this document already holds a snapshot of it; the next + // timer tick catches up with the edits made since. + match worker::submit(entry) { + Some(true) => queued += 1, + Some(false) => {} + None => { + // No thread: write this one here rather than lose the recovery data. + let written = write_snapshot(&Snapshot::of(&access, dir, document), || false); + if written.published { + record_checkpoint(s, dir.to_path_buf(), document.uid); + } + if let Some(e) = written.error { + return Err(e); + } + queued += 1; + } + } + } + Ok(queued) + } +} + +/// Apply one finished background write: record the checkpoint of a document still open and +/// unsaved, or drop the snapshot of one saved, reverted or closed while its write ran. Returns +/// whether a checkpoint was recorded. +#[cfg(not(target_arch = "wasm32"))] +fn record_checkpoint(s: &mut Session, dir: PathBuf, uid: u64) -> bool { + match s.docs.iter_mut().find(|doc| doc.uid == uid) { + Some(doc) if doc.is_dirty() => { + if !has_checkpoint(doc, &dir) { + doc.recovery_entries.push((dir, uid)); + } + true + } + // Saved, reverted or closed while being written: its snapshot must not come back after a + // crash, and the timer writes only when a document is dirty, so nothing else would + // remove it. + Some(_) | None => { + discard(&dir, uid); + false + } + } +} + +/// Apply finished background writes. Returns what was written and what failed. +pub(crate) struct Finished { + pub written: usize, + pub errors: Vec, + pub busy: bool, +} + +#[cfg(not(target_arch = "wasm32"))] +pub(crate) fn finished(s: &mut Session) -> Finished { + let mut out = Finished { written: 0, errors: Vec::new(), busy: false }; + let Some(w) = worker::started() else { return out }; + let Ok(done) = w.done.lock() else { + out.busy = w.busy(); + return out; + }; + while let Ok(d) = done.try_recv() { + // A primary that landed belongs to this run even if its optional metadata failed: record + // it, so Save and Close remove the snapshot instead of leaving it in the folder for ever. + if d.published && record_checkpoint(s, d.dir, d.uid) { + out.written += 1; + } + if let Some(e) = d.error { + out.errors.push(e); + } + } + // After draining: the worker delivers a result before it clears the document as running, so + // "not busy" means every result is already in the channel, not on its way. + out.busy = w.busy(); + out +} + +#[cfg(target_arch = "wasm32")] +pub(crate) fn finished(_s: &mut Session) -> Finished { + // Nothing is written in the background on the web. + Finished { written: 0, errors: Vec::new(), busy: false } +} + +/// Automatic recovery's writer: one thread with its own queue, so an unsaved document with +/// gigabytes of placed files is written without stopping the interface. +#[cfg(not(target_arch = "wasm32"))] +mod worker { + use std::collections::BTreeSet; + use std::sync::mpsc::{Receiver, Sender}; + use std::sync::{Mutex, OnceLock}; + + use super::{Path, PathBuf, Result, Snapshot, write_snapshot}; + + /// One document written, for the interface thread to record. + pub(super) struct Done { + pub(super) dir: PathBuf, + pub(super) uid: u64, + pub(super) error: Option, + /// The primary document reached the folder (its optional metadata may have failed): the + /// checkpoint must still be recorded, or Save and Close could not remove it. + pub(super) published: bool, + } + + pub(super) struct Writer { + jobs: Sender, + pub(super) done: Mutex>, + /// Documents queued or being written: a second write of one still holds nothing newer. + running: Mutex>, + /// A document saved, reverted or closed while its write was queued or running: the + /// snapshot it is producing must not be published. + cancelled: Mutex>, + } + + static WRITER: OnceLock> = OnceLock::new(); + + impl Writer { + pub(super) fn busy(&self) -> bool { + self.running.lock().is_ok_and(|running| !running.is_empty()) + } + + fn finish(&self, dir: &Path, uid: u64) { + let key = (dir.to_path_buf(), uid); + if let Ok(mut running) = self.running.lock() { + running.remove(&key); + } + if let Ok(mut cancelled) = self.cancelled.lock() { + cancelled.remove(&key); + } + } + + /// Do not publish the snapshot being written for `dir`/`uid`: the document was saved, + /// reverted or closed. Only a write that is queued or running is marked, so the set stays + /// as small as the work in flight. + fn cancel(&self, dir: &Path, uid: u64) { + let key = (dir.to_path_buf(), uid); + if !self.running.lock().is_ok_and(|running| running.contains(&key)) { + return; + } + if let Ok(mut cancelled) = self.cancelled.lock() { + cancelled.insert(key); + } + } + + fn cancelled(&self, dir: &Path, uid: u64) -> bool { + self.cancelled.lock().is_ok_and(|cancelled| cancelled.contains(&(dir.to_path_buf(), uid))) + } + } + + /// The writer, started on first use (a thread that can't start leaves recovery synchronous). + fn writer() -> Option<&'static Writer> { + WRITER + .get_or_init(|| { + let (jobs, job_rx) = std::sync::mpsc::channel::(); + let (done_tx, done_rx) = std::sync::mpsc::channel::(); + let spawned = std::thread::Builder::new().name("designcraft-recovery".into()).spawn(move || { + while let Ok(job) = job_rx.recv() { + let (published, error) = guarded_write(&job); + let (dir, uid) = (job.dir, job.uid); + // The result before the document reads as idle: a writer that is not busy + // has already delivered it, so the interface never stops polling with an + // answer still on its way. + let sent = done_tx.send(Done { dir: dir.clone(), uid, error, published }).is_ok(); + if let Some(w) = WRITER.get().and_then(Option::as_ref) { + w.finish(&dir, uid); + } + if !sent { + break; + } + } + }); + spawned.ok().map(|_| Writer { + jobs, + done: Mutex::new(done_rx), + running: Mutex::new(BTreeSet::new()), + cancelled: Mutex::new(BTreeSet::new()), + }) + }) + .as_ref() + } + + /// The writer if it has been started. + pub(super) fn started() -> Option<&'static Writer> { + WRITER.get().and_then(Option::as_ref) + } + + /// Do not publish the snapshot being written for `dir`/`uid` (see [`Writer::cancel`]). + pub(super) fn cancel(dir: &Path, uid: u64) { + if let Some(w) = started() { + w.cancel(dir, uid); + } + } + + /// Whether that write is already marked as cancelled (see [`Writer::cancelled`]). + pub(super) fn cancelled(dir: &Path, uid: u64) -> bool { + started().is_some_and(|w| w.cancelled(dir, uid)) + } + + /// Write one snapshot, turning a panic into its error: a panic that ended this thread would + /// leave the document marked as running for ever (so it is never written again) and the + /// interface asking for frames for ever. A safety net: a writer still must not panic. + fn guarded_write(job: &Snapshot) -> (bool, Option) { + match guarded(|| Ok(write_snapshot(job, || cancelled(&job.dir, job.uid)))) { + // A primary that landed is this run's to remove, even if its metadata failed. + Ok(written) => (written.published, written.error.map(|e| e.to_string())), + Err(e) => (false, Some(e.to_string())), + } + } + + /// [`guarded_write`]'s guard, for any write on this thread. + pub(super) fn guarded(write: impl FnOnce() -> Result) -> Result { + std::panic::catch_unwind(std::panic::AssertUnwindSafe(write)).unwrap_or_else(|payload| { + Err(crate::EngineError::Other(format!("recovery writer panicked: {}", crate::guard::panic_message(payload.as_ref())))) + }) + } + + /// Queue a document's write. `None`: no writer thread (the caller writes it itself); + /// `Some(false)`: one for this document is still queued or running. + pub(super) fn submit(entry: Snapshot) -> Option { + let w = writer()?; + let mut running = w.running.lock().ok()?; + let key = (entry.dir.clone(), entry.uid); + if !running.insert(key.clone()) { + return Some(false); + } + // A write is wanted again: whatever cancelled the last one does not apply to this one. + if let Ok(mut cancelled) = w.cancelled.lock() { + cancelled.remove(&key); + } + if w.jobs.send(entry).is_err() { + running.remove(&key); + return None; + } + Some(true) + } } #[cfg(not(target_arch = "wasm32"))] @@ -632,6 +958,21 @@ fn secure_recovery_dir(_dir: &Path) -> Result { #[cfg(not(target_arch = "wasm32"))] fn publish_recovery_file(path: &Path, bytes: &[u8], replace: bool) -> Result<()> { + publish_recovery_written(path, replace, |out| out.write_all(bytes).map_err(|e| EngineError::Other(format!("{}: {e}", path.display()))), || false) + .map(|_| ()) +} + +/// [`publish_recovery_file`], with `write` filling the temporary file: a large document is written +/// straight to disk instead of being built in memory first. Returns whether it was published; +/// `cancelled` is asked after the bytes are written and before the rename, so a document saved or +/// closed meanwhile leaves nothing behind. +#[cfg(not(target_arch = "wasm32"))] +fn publish_recovery_written( + path: &Path, + replace: bool, + write: impl FnOnce(&mut std::io::BufWriter<&mut std::fs::File>) -> Result<()>, + cancelled: impl Fn() -> bool, +) -> Result { let Some(dir) = path.parent() else { return Err(EngineError::Other(format!("{} has no parent folder", path.display()))); }; @@ -646,15 +987,24 @@ fn publish_recovery_file(path: &Path, bytes: &[u8], replace: bool) -> Result<()> use std::os::unix::fs::PermissionsExt; tmp.as_file().set_permissions(std::fs::Permissions::from_mode(0o600)).map_err(|e| EngineError::Other(format!("{}: {e}", path.display())))?; } - tmp.as_file_mut().write_all(bytes).map_err(|e| EngineError::Other(format!("{}: {e}", path.display())))?; + { + let mut out = std::io::BufWriter::new(tmp.as_file_mut()); + write(&mut out)?; + out.flush().map_err(|e| EngineError::Other(format!("{}: {e}", path.display())))?; + } tmp.as_file_mut().sync_all().map_err(|e| EngineError::Other(format!("{}: {e}", path.display())))?; + // Last moment: the document was saved, reverted or closed while the bytes were written. The + // temporary file is dropped, so nothing of the stale snapshot stays. + if cancelled() { + return Ok(false); + } // Validate the non-followed leaf again immediately before renaming the randomized temporary // file to its fixed sibling destination. validate_recovery_dir(dir)?; let published = if replace { tmp.persist(path) } else { tmp.persist_noclobber(path) }; published.map_err(|e| EngineError::Other(format!("{}: {}", path.display(), e.error)))?; sync_recovery_dir(dir)?; - Ok(()) + Ok(true) } #[cfg(not(target_arch = "wasm32"))] @@ -712,6 +1062,16 @@ fn publish_recovery_file(_path: &Path, _bytes: &[u8], _replace: bool) -> Result< Err(EngineError::Other("recovery files are unavailable on the web".into())) } +#[cfg(target_arch = "wasm32")] +fn publish_recovery_written( + _path: &Path, + _replace: bool, + _write: impl FnOnce(&mut std::io::BufWriter<&mut std::fs::File>) -> Result<()>, + _cancelled: impl Fn() -> bool, +) -> Result { + Err(EngineError::Other("recovery files are unavailable on the web".into())) +} + /// Canonical keys are only for alias comparison; operational paths stay unchanged so run /// ownership checks still find the directory registered by the native profile. fn directory_key(dir: &Path) -> Result { diff --git a/crates/engine/src/recovery/tests.rs b/crates/engine/src/recovery/tests.rs index 395e8385b..89feb2873 100644 --- a/crates/engine/src/recovery/tests.rs +++ b/crates/engine/src/recovery/tests.rs @@ -126,6 +126,194 @@ fn recovery_data_is_private_to_the_user() { } } +/// The recovery writer is one thread for the whole process, and its results go to whichever +/// session polls it: tests that submit to it take turns, so one test's write or error is never +/// drained by another. +static BACKGROUND: std::sync::Mutex<()> = std::sync::Mutex::new(()); + +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. +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"); +} + +#[test] +fn a_background_save_writes_recovery_data_the_app_can_recover() { + let _turn = background(); + let dir = tempfile::tempdir().unwrap(); + let mut s = dirty_session(); + s.recovery_dir = Some(dir.path().to_path_buf()); + // The timer's call: the interface thread does not wait for the deflate or the file writes. + let queued = s.execute("file.recovery.save", &json!({"background": true})).unwrap(); + assert_eq!(queued["queued"], 1, "{queued}"); + assert_eq!(finished_writes(&mut s), 1); + assert_eq!(list(dir.path()).len(), 1); + assert!(s.docs[0].recovery_entries.iter().any(|(d, _)| same_directory(d, dir.path())), "the checkpoint is recorded"); + // Recovering it gives the unsaved edits back. + let mut reopened = Session::new(); + assert_eq!(open(&mut reopened, dir.path()).unwrap().len(), 1); + assert_eq!(reopened.doc().unwrap().doc.spreads[0].items.len(), 1); + assert!(reopened.doc().unwrap().is_dirty()); + discard(dir.path(), reopened.doc().unwrap().uid); +} + +#[test] +fn a_snapshot_that_finishes_after_the_save_is_dropped() { + // A write of a large document runs for seconds: the user can save while it does. The saved + // document's checkpoint is consumed then, and the write must not put a stale snapshot back. + 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; + // The snapshot the writer took before the save. + let entry = Snapshot::of(&prepare_save(&s, dir.path()).unwrap(), dir.path(), &s.docs[0]); + let saved = dir.path().join("saved.designcraft"); + s.execute("file.saveAs", &json!({"path": saved.to_string_lossy()})).unwrap(); + assert!(!s.docs[0].is_dirty()); + // The write lands after the save. + let written = write_snapshot(&entry, || false); + assert!(written.published && written.error.is_none(), "the snapshot landed"); + let (file, _) = files(dir.path(), uid); + assert!(file.exists()); + assert!(!record_checkpoint(&mut s, dir.path().to_path_buf(), uid), "a saved document records no checkpoint"); + assert!(!file.exists(), "its snapshot is not left to be recovered"); + assert!(list(dir.path()).is_empty()); + // A document still unsaved does record one. + s.execute("frame.create", &json!({"rect": [10, 10, 50, 50]})).unwrap(); + let entry = Snapshot::of(&prepare_save(&s, dir.path()).unwrap(), dir.path(), &s.docs[0]); + let written = write_snapshot(&entry, || false); + assert!(written.published && written.error.is_none(), "the second snapshot landed"); + assert!(record_checkpoint(&mut s, dir.path().to_path_buf(), uid)); + assert!(s.docs[0].recovery_entries.iter().any(|(d, u)| *u == uid && d == dir.path())); +} + +#[cfg(not(target_arch = "wasm32"))] +#[test] +fn a_snapshot_whose_metadata_failed_is_still_the_documents_to_remove() { + // The primary document is the recovery data; its metadata is optional. A metadata failure + // must not leave the published snapshot untracked, or Save and Close would not remove it and + // it would stay in the folder for ever. + let root = tempfile::tempdir().unwrap(); + let dir = run_dir(root.path()).unwrap(); + let mut s = dirty_session(); + s.recovery_dir = Some(dir.clone()); + let uid = s.docs[0].uid; + let (document, metadata) = files(&dir, uid); + // The folder is this run's own, so the entry is its own; a folder takes the metadata's name, + // which makes the optional write fail after the primary landed. + std::fs::create_dir(&metadata).unwrap(); + let err = save_tracked(&mut s, &dir).unwrap_err(); + assert!(err.to_string().contains(&metadata.display().to_string()), "{err}"); + assert!(document.exists(), "the primary landed"); + assert!(s.docs[0].recovery_entries.iter().any(|(d, u)| *u == uid && same_directory(d, &dir)), "and is tracked"); + // Saving the document removes the snapshot this run wrote. + let saved = root.path().join("saved.designcraft"); + s.execute("file.saveAs", &json!({"path": saved.to_string_lossy()})).unwrap(); + assert!(!document.exists(), "Save removed the snapshot"); + assert!(s.docs[0].recovery_entries.is_empty()); +} + +#[cfg(not(target_arch = "wasm32"))] +#[test] +fn a_background_snapshot_whose_metadata_failed_is_still_the_documents_to_remove() { + // The background writer's result must be applied the same way: a published primary is the + // document's snapshot even when its optional metadata fails. + let _turn = background(); + let root = tempfile::tempdir().unwrap(); + let dir = run_dir(root.path()).unwrap(); + let mut s = dirty_session(); + s.recovery_dir = Some(dir.clone()); + let uid = s.docs[0].uid; + let (document, metadata) = files(&dir, uid); + 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)); + } + let reported = reported.expect("the metadata failure is reported"); + assert!(reported.contains(&metadata.display().to_string()), "{reported}"); + assert!(document.exists(), "the primary landed"); + assert!(s.docs[0].recovery_entries.iter().any(|(d, u)| *u == uid && same_directory(d, &dir)), "and is tracked"); + // Saving the document removes the snapshot this run wrote. + let saved = root.path().join("saved.designcraft"); + s.execute("file.saveAs", &json!({"path": saved.to_string_lossy()})).unwrap(); + assert!(!document.exists(), "Save removed the snapshot"); +} + +#[test] +fn a_write_cancelled_midway_publishes_nothing() { + // Save/Revert/Close during a write marks it cancelled; the snapshot must not land after it, + // even if the app crashes before the next interface frame discards it. + let dir = tempfile::tempdir().unwrap(); + let s = dirty_session(); + let entry = Snapshot::of(&prepare_save(&s, dir.path()).unwrap(), dir.path(), &s.docs[0]); + let written = write_snapshot(&entry, || true); + assert!(!written.published && written.error.is_none(), "a cancelled write is not an error"); + assert!(list(dir.path()).is_empty(), "a cancelled write leaves no snapshot"); + assert!(!files(dir.path(), entry.uid).0.exists(), "nor its document"); + assert!(!files(dir.path(), entry.uid).1.exists(), "nor its metadata"); + // The temporary file it was written to is dropped, not left behind. + assert!( + std::fs::read_dir(dir.path()).unwrap().all(|e| e.is_ok_and(|e| !e.file_name().to_string_lossy().starts_with(".designcraft-recovery-"))), + "no temporary file is left" + ); +} + +#[cfg(not(target_arch = "wasm32"))] +#[test] +fn only_a_queued_write_is_cancelled() { + // A mark for a document with no write in flight would stay in the writer's set for ever. + let dir = tempfile::tempdir().unwrap(); + worker::cancel(dir.path(), 7); + assert!(!worker::cancelled(dir.path(), 7), "nothing was queued for it"); +} + +#[cfg(not(target_arch = "wasm32"))] +#[test] +fn a_panicking_recovery_write_is_an_error_and_the_writer_keeps_going() { + let err = worker::guarded(|| -> Result<()> { panic!("boom") }).unwrap_err(); + assert!(err.to_string().contains("panicked") && err.to_string().contains("boom"), "{err}"); + assert_eq!(worker::guarded(|| Ok(7)).unwrap(), 7, "the next write still runs"); +} + +#[test] +fn a_saved_document_is_not_written_in_the_background() { + // The autosave writes unsaved documents only. + let _turn = background(); + let dir = tempfile::tempdir().unwrap(); + let mut s = dirty_session(); + s.recovery_dir = Some(dir.path().to_path_buf()); + let saved = dir.path().join("saved.designcraft"); + s.execute("file.saveAs", &json!({"path": saved.to_string_lossy()})).unwrap(); + let queued = s.execute("file.recovery.save", &json!({"background": true})).unwrap(); + assert_eq!(queued["queued"], 0, "{queued}"); + assert_eq!(finished_writes(&mut s), 0); + assert!(list(dir.path()).is_empty()); +} + #[test] fn existing_recovery_directory_is_used() { let root = tempfile::tempdir().unwrap(); diff --git a/crates/format/src/lib.rs b/crates/format/src/lib.rs index 09bdfe334..126c9470a 100644 --- a/crates/format/src/lib.rs +++ b/crates/format/src/lib.rs @@ -86,6 +86,23 @@ fn required_version(doc: &Document) -> u32 { } fn save_with_large_entries_from(doc: &Document, preview: Option<&[u8]>, large_entry: usize) -> Result, FormatError> { + let mut buf = Cursor::new(Vec::new()); + write_with_large_entries_from(&mut buf, doc, preview, large_entry)?; + Ok(buf.into_inner()) +} + +/// Write the package to `out` instead of returning it in memory: a document with gigabytes of +/// placed files needs no second copy in RAM (recovery snapshots, large saves). +pub fn write(out: &mut W, doc: &Document, preview: Option<&[u8]>) -> Result<(), FormatError> { + write_with_large_entries_from(out, doc, preview, LARGE_ENTRY) +} + +fn write_with_large_entries_from( + out: &mut W, + doc: &Document, + preview: Option<&[u8]>, + large_entry: usize, +) -> Result<(), FormatError> { // JSON writes infinity and NaN as null, which no load accepts: the file would not open. if let Some(id) = doc.non_finite_item() { return Err(FormatError::Unsaveable(format!("object {} has a position or size that is not a finite number", id.0))); @@ -94,9 +111,8 @@ fn save_with_large_entries_from(doc: &Document, preview: Option<&[u8]>, large_en compact.compact_assets(); let doc = &compact; let io = |e: zip::result::ZipError| FormatError::Io(e.to_string()); - let mut buf = Cursor::new(Vec::new()); { - let mut z = zip::ZipWriter::new(&mut buf); + let mut z = zip::ZipWriter::new(out); let stored = zip::write::SimpleFileOptions::default().compression_method(zip::CompressionMethod::Stored); let deflate = zip::write::SimpleFileOptions::default().compression_method(zip::CompressionMethod::Deflated); z.start_file("mimetype", stored).map_err(io)?; @@ -124,7 +140,7 @@ fn save_with_large_entries_from(doc: &Document, preview: Option<&[u8]>, large_en } z.finish().map_err(io)?; } - Ok(buf.into_inner()) + Ok(()) } /// The file name extension of a DesignCraft document. diff --git a/crates/format/src/tests.rs b/crates/format/src/tests.rs index a95bee160..8820eabda 100644 --- a/crates/format/src/tests.rs +++ b/crates/format/src/tests.rs @@ -577,6 +577,24 @@ fn saved_tab_lists_load_sanitized() { assert_eq!(tabs.last().map(|t| t.position), Some(3960.0), "the first stops by position"); } +#[test] +fn writing_to_a_stream_makes_the_same_file_as_saving_in_memory() { + // A save that streams needs no copy of the archive in RAM; it must write the same package. + let mut d = Document::new(&NewDocument::default()); + let a = png_asset(&mut d, b"streamed bytes"); + place(&mut d, a, 0.0); + let png = b"\x89PNG streamed".to_vec(); + let saved = save_with_preview(&d, Some(&png)).unwrap(); + let mut out = Cursor::new(Vec::new()); + write(&mut out, &d, Some(&png)).unwrap(); + let streamed = out.into_inner(); + assert_eq!(saved.len(), streamed.len()); + assert_eq!(asset_entries(&streamed), 1); + let back = load(&streamed).unwrap(); + assert_eq!(back.assets.len(), 1); + assert_eq!(read_preview(Cursor::new(&streamed)), Some(png)); +} + #[test] fn the_preview_is_read_back_without_the_document() { let d = Document::new(&NewDocument::default()); diff --git a/crates/ui-egui/src/app.rs b/crates/ui-egui/src/app.rs index b2b433b71..43071f366 100644 --- a/crates/ui-egui/src/app.rs +++ b/crates/ui-egui/src/app.rs @@ -108,6 +108,7 @@ impl DesignApp { shortcut_answers: Default::default(), last_time: now, last_recovery: now, + recovery_writing: false, egui_ctx: None, in_control: false, object_clip_text: None, diff --git a/crates/ui-egui/src/app/frame.rs b/crates/ui-egui/src/app/frame.rs index ceeb28743..985ede0f5 100644 --- a/crates/ui-egui/src/app/frame.rs +++ b/crates/ui-egui/src/app/frame.rs @@ -83,14 +83,28 @@ impl DesignApp { /// uses the monotonic clock, so a wall-clock/NTP jump cannot postpone or advance it. pub(super) fn ui_frame_end(&mut self, ctx: &egui::Context) { let now = now_ms() / 1000.0; - // Crash recovery: unsaved documents are written to the recovery folder every 30 s. + // Crash recovery: unsaved documents are written to the recovery folder every 30 s, on a + // background thread — writing a catalogue with gigabytes of placed files took minutes, and + // on the interface thread the window stopped answering for all of it. #[cfg(not(target_arch = "wasm32"))] - if self.session.recovery_dir.is_some() && now - self.last_recovery > (self.session.prefs.recovery_minutes * 60.0).max(5.0) { - self.last_recovery = now; - if self.session.documents().iter().any(|d| d.is_dirty()) - && let Err(e) = self.session.execute("file.recovery.save", &json!({})) - { - self.status(crate::i18n::tr(&self.ui.language, "Couldn't write recovery data: {error}").replace("{error}", &e.to_string())); + { + if self.session.recovery_dir.is_some() && now - self.last_recovery > (self.session.prefs.recovery_minutes * 60.0).max(5.0) { + self.last_recovery = now; + if self.session.documents().iter().any(|d| d.is_dirty()) { + match self.session.execute("file.recovery.save", &json!({"background": true})) { + Ok(v) if v["queued"].as_u64().unwrap_or(0) > 0 => self.recovery_writing = true, + Ok(_) => {} + Err(e) => self + .status(crate::i18n::tr(&self.ui.language, "Couldn't write recovery data: {error}").replace("{error}", &e.to_string())), + } + } + } + if self.recovery_writing { + self.poll_recovery(); + // Keep the frames coming while the writer works; it has no interface to ask. + if self.recovery_writing { + ctx.request_repaint_after(std::time::Duration::from_millis(150)); + } } } let dt = now - self.last_time; @@ -101,6 +115,18 @@ impl DesignApp { self.issue_screenshots(ctx); } + /// Take what the background recovery writer finished: its checkpoints, and what failed. + #[cfg(not(target_arch = "wasm32"))] + fn poll_recovery(&mut self) { + let Ok(v) = self.session.execute("file.recovery.progress", &json!({})) else { return }; + self.recovery_writing = v["busy"].as_bool().unwrap_or(false); + let errors: Vec = v["errors"].as_array().into_iter().flatten().filter_map(|e| e.as_str().map(str::to_string)).collect(); + for e in errors { + let message = crate::i18n::tr(&self.ui.language, "Couldn't write recovery data: {error}").replace("{error}", &e); + self.status(message); + } + } + /// The document area: the start screen, or the document tabs over one or two canvas panes. pub(crate) fn canvas_area(&mut self, ui: &mut egui::Ui, presenting: bool) { let t = theme::Tokens::get(ui.ctx()); diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 6775c7755..751f8ccca 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -242,6 +242,9 @@ pub struct DesignApp { last_time: f64, /// When recovery data was last written (seconds on the monotonic clock; see [`now_ms`]). pub last_recovery: f64, + /// Recovery data is being written in the background: poll for it, and keep repainting. + #[cfg_attr(target_arch = "wasm32", allow(dead_code))] + pub(crate) recovery_writing: bool, /// The egui context (set by the first UI pass): copied text goes to the system clipboard through it. egui_ctx: Option, /// A control-channel request is being handled: commands it runs never read or write the diff --git a/crates/ui-egui/src/tests.rs b/crates/ui-egui/src/tests.rs index 5b3871bbf..08b709505 100644 --- a/crates/ui-egui/src/tests.rs +++ b/crates/ui-egui/src/tests.rs @@ -140,6 +140,35 @@ fn app_with_unsaved_document() -> DesignApp { app } +/// The recovery timer wrote the whole document on the interface thread: on a catalogue with +/// gigabytes of linked files the window stopped answering for minutes after every edit. +#[test] +fn the_recovery_timer_writes_off_the_interface_thread() { + let ctx = egui::Context::default(); + let mut app = app_with_unsaved_document(); + let dir = tempfile::tempdir().unwrap(); + app.session.recovery_dir = Some(dir.path().to_path_buf()); + 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 { + 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; + 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"); + // 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(); + assert!(!dir.path().join(format!("{uid}.designcraft")).exists()); +} + /// File ▸ Close (and its shortcut) closed a document with unsaved changes without asking, and /// closing discards the document's recovery data too. #[test] diff --git a/docs/architecture.md b/docs/architecture.md index 7b86388a9..c0b7ac7a5 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -1,6 +1,6 @@ # DesignCraft architecture -> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-12 · **Change:** minor (shared PDF writing) · **Target:** Adobe InDesign 2026 +> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-27 · **Change:** minor (recovery writes on a background thread, streamed to disk) · **Target:** Adobe InDesign 2026 How DesignCraft is built today. The design notes behind it are in `plan/architecture.md` (local, gitignored); this file describes what the code on `main` does. Agents: AGENTS.md has the @@ -58,7 +58,9 @@ panels, zoom, dialogs) are `UI_COMMANDS` in `crates/ui-egui/src/menus/ui_command Tool gestures emit Begin/Preview/Commit commands so a drag is one undo step. Every journaled command is replayable as a script. Escaped panics during a command or an import/export become an error (`engine/src/guard.rs`), never a lost document; native recovery is written periodically -into each running app's locked folder. Orderly native shutdown finalizes complete, current quit +into each running app's locked folder, on a background thread and straight to disk, so an +autosave of a catalogue with gigabytes of placed files no longer stops the window answering. +Orderly native shutdown finalizes complete, current quit approvals and writes the remaining documents' latest recovery state, including clean-document cleanup after Undo. Unavailable or failed recovery is reported. The desktop opts into a pinned winit 0.30 patch that cancels AppKit termination and queues ordinary window close requests, @@ -110,6 +112,7 @@ builds (on manifest changes), packaging lint and the release workflow; `xtask ci | Date | Change | Summary | |---|---|---| +| 2026-10-27 | minor | Recovery writes off the interface thread, streamed to disk; a snapshot still being written when its document is saved, reverted or closed is cancelled | | 2026-10-12 | minor | Documented cancelable Dock Quit through the native event-loop boundary | | 2026-10-12 | minor | Documented orderly native exit recovery and the remaining cancelable Dock Quit gap | | 2026-10-10 | major | Created from the code on main (92649ff) and `plan/architecture.md` | diff --git a/docs/gaps.md b/docs/gaps.md index d74de3b04..b83dbdb2b 100644 --- a/docs/gaps.md +++ b/docs/gaps.md @@ -1,6 +1,6 @@ # Where DesignCraft falls short of InDesign -> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-24 · **Change:** minor (placed graphic layer-option preservation and explicit rendering warnings; gap 16 shrunk: File › File Info; gap 16: Convert Variable to Text; gap 28 added: placed-PDF font repair remains after visible unembedded font detection, #909; gap 26 shrunk: autoflow fills the following pages before it adds pages at the end; gap 6 shrunk: Type › Reload Fonts reads fonts installed, edited or removed while running (#328); gap 3: Package links are relative, #340; gap 27 closed: the loaded text cursor threads into a frame inside a group (#1075); .indd and .indt open through indd-utils; gap 0 shrunk: text threads by hand; threading gaps added; Optical kerning from outlines (#181); gap 6: macOS lists fonts that font services and managers activate; shared print spooler; image clipping paths from IDML; Tabs panel listed as present (#343); book chapter update safety; gap 16: Based on Parent leftovers; bounded control click/drag input; swatches on placed greyscale and bitmap images) · **Target:** Adobe InDesign 2026 (21.6) +> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-27 · **Change:** minor (gap 20: automatic recovery moved off the interface thread; placed graphic layer-option preservation and explicit rendering warnings; gap 16 shrunk: File › File Info; gap 16: Convert Variable to Text; gap 28 added: placed-PDF font repair remains after visible unembedded font detection, #909; gap 26 shrunk: autoflow fills the following pages before it adds pages at the end; gap 6 shrunk: Type › Reload Fonts reads fonts installed, edited or removed while running (#328); gap 3: Package links are relative, #340; gap 27 closed: the loaded text cursor threads into a frame inside a group (#1075); .indd and .indt open through indd-utils; gap 0 shrunk: text threads by hand; threading gaps added; Optical kerning from outlines (#181); gap 6: macOS lists fonts that font services and managers activate; shared print spooler; image clipping paths from IDML; Tabs panel listed as present (#343); book chapter update safety; gap 16: Based on Parent leftovers; bounded control click/drag input; swatches on placed greyscale and bitmap images) · **Target:** Adobe InDesign 2026 (21.6) Every known shortfall against InDesign 2026, one entry each, ranked by how much it stops an InDesign professional from doing real work. This is the work list: agents pick from the top. @@ -234,7 +234,11 @@ Kind: **F** feature · **U** UI/UX · **T** typography · **FF** file format · - **Missing:** incremental composition (recompose from the edited paragraph; 8 ms keystroke budget), benchmarks against InDesign on long books and image-heavy catalogues. - **Have:** compose 300k characters ~38 ms warm, damage-region repaint (< 1 ms while typing), - parallel story composition. + parallel story composition. Automatic recovery writes on a background thread and streams to + disk, so an autosave of that catalogue no longer blocks the interface thread for 30-100 s + (`file.recovery.save {background}`, `file.recovery.progress`). A snapshot still being written + when its document is saved, reverted or closed is cancelled before it is published, so a crash + cannot offer back a copy older than the saved file. - **Estimate:** 40–80 h. ### 26. Autoflow flows into the primary text frame of the pages that follow (F) From 6444c76202d1d5b325cd208899850d3493c29af6 Mon Sep 17 00:00:00 2001 From: aiongg <67543387+aiongg@users.noreply.github.com> Date: Thu, 5 Nov 2026 21:04:52 +0000 Subject: [PATCH 02/14] Apply review fixes - crates/engine/src/cmd/file/registry.rs - crates/engine/src/recovery.rs --- crates/engine/src/cmd/file/registry.rs | 2 +- crates/engine/src/recovery.rs | 17 ++++++++++------- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/crates/engine/src/cmd/file/registry.rs b/crates/engine/src/cmd/file/registry.rs index e3a21213e..a9d14892c 100644 --- a/crates/engine/src/cmd/file/registry.rs +++ b/crates/engine/src/cmd/file/registry.rs @@ -68,7 +68,7 @@ pub fn specs() -> Vec { Ok(Value::Array(crate::recovery::list(&dir).into_iter().map(|(uid, mut m)| { m["uid"] = json!(uid); m }).collect())) }), 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}", + "{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 dir = recovery_dir(s, p, FolderAccess::Write)?; if p.get("background").and_then(Value::as_bool).unwrap_or(false) { diff --git a/crates/engine/src/recovery.rs b/crates/engine/src/recovery.rs index 4598ae397..877759f8f 100644 --- a/crates/engine/src/recovery.rs +++ b/crates/engine/src/recovery.rs @@ -532,6 +532,9 @@ pub fn save(s: &Session, dir: &Path) -> Result { continue; } // The caller asked for this write now: nothing has cancelled it. + // A background write of an older state must not land over this one. + #[cfg(not(target_arch = "wasm32"))] + worker::cancel(dir, d.uid); if let Some(e) = write_snapshot(&Snapshot::of(&access, dir, d), || false).error { return Err(e); } @@ -557,6 +560,9 @@ pub(crate) fn save_tracked(s: &mut Session, dir: &Path) -> Result { continue; } let Some(document) = s.docs.get(index) else { continue }; + // A background write of an older state must not land over this one. + #[cfg(not(target_arch = "wasm32"))] + worker::cancel(dir, document.uid); let entry = Snapshot::of(&access, dir, document); let written = write_snapshot(&entry, || false); // The primary is the recovery data; its metadata is optional. Track the primary before @@ -734,10 +740,9 @@ pub(crate) struct Finished { pub(crate) fn finished(s: &mut Session) -> Finished { let mut out = Finished { written: 0, errors: Vec::new(), busy: false }; let Some(w) = worker::started() else { return out }; - let Ok(done) = w.done.lock() else { - out.busy = w.busy(); - return out; - }; + // Before draining: the worker sends a result before it clears the document as running, so a writer idle now has every result in the channel already. + let busy = w.busy(); + let done = w.done.lock().unwrap_or_else(std::sync::PoisonError::into_inner); while let Ok(d) = done.try_recv() { // A primary that landed belongs to this run even if its optional metadata failed: record // it, so Save and Close remove the snapshot instead of leaving it in the folder for ever. @@ -748,9 +753,7 @@ pub(crate) fn finished(s: &mut Session) -> Finished { out.errors.push(e); } } - // After draining: the worker delivers a result before it clears the document as running, so - // "not busy" means every result is already in the channel, not on its way. - out.busy = w.busy(); + out.busy = busy; out } From 9f99420e32bbde7baab20d040344c3d99ac82785 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Wed, 11 Nov 2026 19:40:54 -0800 Subject: [PATCH 03/14] 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 | 4 + crates/engine/src/cmd/file/registry.rs | 6 +- crates/engine/src/recovery.rs | 118 ++++++++++++++++++++++++- crates/engine/src/recovery/tests.rs | 70 ++++++++++----- crates/ui-egui/Cargo.toml | 1 + crates/ui-egui/src/tests.rs | 28 ++++-- 6 files changed, 193 insertions(+), 34 deletions(-) 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(); From 43088a87c0214ecb475c8b31fe1c30c2502563cb Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Wed, 11 Nov 2026 19:51:54 -0800 Subject: [PATCH 04/14] Recovery: a cancel cannot race the rename, and results go to their session A save, revert or close that cancels a background write now waits for a rename already under way, and a later rename sees the cancel, so a stale snapshot or its metadata cannot be published after the cleanup. The writer's results go back to the session that queued them: another session's poll no longer removes their snapshots, and results for a session that has been dropped are not kept. --- crates/engine/src/lib.rs | 4 ++ crates/engine/src/recovery.rs | 97 ++++++++++++++++++++++++----- crates/engine/src/recovery/tests.rs | 59 ++++++++++++++++++ 3 files changed, 145 insertions(+), 15 deletions(-) diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 3d43a80e0..bb67875b4 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -412,6 +412,9 @@ pub struct Session { pub clipboard_origin: f64, /// Crash-recovery folder (the app sets it; saving or closing a document removes its entry). pub recovery_dir: Option, + /// This session's number for the background recovery writer, which returns each result to + /// the session that queued it. + pub(crate) recovery_owner: recovery::Owner, /// Copied text with its formatting (a frameless story slice), its plain text, and what pasting /// it needs from its document: a copy holding only the stories its anchored text frames show /// (and theirs) and the graphics the text and those stories place. @@ -477,6 +480,7 @@ impl Session { clipboard: None, clipboard_origin: 0.0, recovery_dir: None, + recovery_owner: recovery::Owner::new(), text_clipboard: None, link_checks: Default::default(), ui_requests: vec![], diff --git a/crates/engine/src/recovery.rs b/crates/engine/src/recovery.rs index 420d1795f..dc7d63ed7 100644 --- a/crates/engine/src/recovery.rs +++ b/crates/engine/src/recovery.rs @@ -649,6 +649,32 @@ struct Snapshot { own: bool, /// The folder's canonical key, for recording that a shared entry is this run's now. key: PathBuf, + /// The session that queued a background write ([`Session::recovery_owner`]), which takes its + /// result. + #[cfg_attr(target_arch = "wasm32", allow(dead_code))] + owner: u64, +} + +/// A number for each session, so background results go back to the session that queued them. +/// Dropping it retires the number: results still queued for it are then dropped. +pub(crate) struct Owner(pub(crate) u64); + +/// The numbers of the sessions that exist. +static LIVE_OWNERS: std::sync::Mutex> = std::sync::Mutex::new(std::collections::BTreeSet::new()); + +impl Owner { + pub(crate) fn new() -> Self { + static NEXT_OWNER: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(1); + let id = NEXT_OWNER.fetch_add(1, std::sync::atomic::Ordering::Relaxed); + LIVE_OWNERS.lock().unwrap_or_else(std::sync::PoisonError::into_inner).insert(id); + Owner(id) + } +} + +impl Drop for Owner { + fn drop(&mut self) { + LIVE_OWNERS.lock().unwrap_or_else(std::sync::PoisonError::into_inner).remove(&self.0); + } } impl Snapshot { @@ -662,6 +688,7 @@ impl Snapshot { // An entry this run did not write is never replaced, even one made since it checked. own: access.owns(d.uid), key: access.key.clone(), + owner: 0, } } } @@ -700,7 +727,16 @@ fn write_snapshot(entry: &Snapshot, cancelled: impl Fn() -> bool) -> Written { WRITTEN.lock().unwrap_or_else(std::sync::PoisonError::into_inner).insert((entry.key.clone(), entry.uid)); } let metadata = json!({"path": entry.path, "title": entry.title, "saved": designcraft_doc::vars::now()}); - Written { published: true, error: publish_recovery_file(&meta, metadata.to_string().as_bytes(), entry.own).err() } + // A cancel after the primary landed removes it; the metadata must not be left without it. + let bytes = metadata.to_string(); + let error = publish_recovery_written( + &meta, + entry.own, + |out| out.write_all(bytes.as_bytes()).map_err(|e| EngineError::Other(format!("{}: {e}", meta.display()))), + &cancelled, + ) + .err(); + Written { published: true, error } } /// Write recovery entries for dirty documents on a background thread and drop clean checkpoints @@ -729,7 +765,8 @@ pub(crate) fn start_tracked(s: &mut Session, dir: &Path) -> Result { continue; } let Some(document) = s.docs.get(index) else { continue }; - let entry = Snapshot::of(&access, dir, document); + let mut entry = Snapshot::of(&access, dir, document); + entry.owner = s.recovery_owner.0; // A write still running for this document already holds a snapshot of it; the next // timer tick catches up with the edits made since. match worker::submit(entry) { @@ -787,8 +824,20 @@ pub(crate) fn finished(s: &mut Session) -> Finished { let Some(w) = worker::started() else { return out }; // Before draining: the worker sends a result before it clears the document as running, so a writer idle now has every result in the channel already. let busy = w.busy(); - let done = w.done.lock().unwrap_or_else(std::sync::PoisonError::into_inner); - while let Ok(d) = done.try_recv() { + // Results go to the session that queued them: another session has none of those documents, + // and would remove their snapshots as if they had been closed. + let mine: Vec = { + let done = w.done.lock().unwrap_or_else(std::sync::PoisonError::into_inner); + let mut others = w.undelivered.lock().unwrap_or_else(std::sync::PoisonError::into_inner); + let mut all = std::mem::take(&mut *others); + all.extend(done.try_iter()); + let (mine, rest): (Vec<_>, Vec<_>) = all.into_iter().partition(|d| d.owner == s.recovery_owner.0); + // A session that is gone takes nothing: its results are dropped rather than kept for ever. + let live = LIVE_OWNERS.lock().unwrap_or_else(std::sync::PoisonError::into_inner); + *others = rest.into_iter().filter(|d| live.contains(&d.owner)).collect(); + mine + }; + for d in mine { // A primary that landed belongs to this run even if its optional metadata failed: record // it, so Save and Close remove the snapshot instead of leaving it in the folder for ever. if d.published && record_checkpoint(s, d.dir, d.uid) { @@ -826,11 +875,15 @@ mod worker { /// The primary document reached the folder (its optional metadata may have failed): the /// checkpoint must still be recorded, or Save and Close could not remove it. pub(super) published: bool, + /// The session that queued the write. + pub(super) owner: u64, } pub(super) struct Writer { jobs: Sender, pub(super) done: Mutex>, + /// Results taken from `done` by a session that did not queue them, kept for their own. + pub(super) undelivered: Mutex>, /// Documents queued or being written: a second write of one still holds nothing newer. running: Mutex>, /// A document saved, reverted or closed while its write was queued or running: the @@ -872,6 +925,9 @@ mod worker { /// as small as the work in flight. fn cancel(&self, dir: &Path, uid: u64) { let key = (dir.to_path_buf(), uid); + // A rename under way finishes first, so the caller's cleanup comes after it; a later + // one sees the mark. + let _publishing = super::PUBLISH.lock().unwrap_or_else(std::sync::PoisonError::into_inner); if !self.running.lock().is_ok_and(|running| running.contains(&key)) { return; } @@ -894,11 +950,11 @@ mod worker { let spawned = std::thread::Builder::new().name("designcraft-recovery".into()).spawn(move || { while let Ok(job) = job_rx.recv() { let (published, error) = guarded_write(&job); - let (dir, uid) = (job.dir, job.uid); + let (dir, uid, owner) = (job.dir, job.uid, job.owner); // The result before the document reads as idle: a writer that is not busy // has already delivered it, so the interface never stops polling with an // answer still on its way. - let sent = done_tx.send(Done { dir: dir.clone(), uid, error, published }).is_ok(); + let sent = done_tx.send(Done { dir: dir.clone(), uid, error, published, owner }).is_ok(); if let Some(w) = WRITER.get().and_then(Option::as_ref) { w.finish(&dir, uid); } @@ -910,6 +966,7 @@ mod worker { spawned.ok().map(|_| Writer { jobs, done: Mutex::new(done_rx), + undelivered: Mutex::new(Vec::new()), running: Mutex::new(BTreeSet::new()), cancelled: Mutex::new(BTreeSet::new()), idle: Condvar::new(), @@ -1124,6 +1181,11 @@ fn publish_recovery_file(path: &Path, bytes: &[u8], replace: bool) -> Result<()> .map(|_| ()) } +/// Serializes the last check for a cancelled write with the rename that publishes it, and with +/// [`worker::cancel`]. +#[cfg(not(target_arch = "wasm32"))] +static PUBLISH: std::sync::Mutex<()> = std::sync::Mutex::new(()); + /// [`publish_recovery_file`], with `write` filling the temporary file: a large document is written /// straight to disk instead of being built in memory first. Returns whether it was published; /// `cancelled` is asked after the bytes are written and before the rename, so a document saved or @@ -1155,16 +1217,21 @@ fn publish_recovery_written( out.flush().map_err(|e| EngineError::Other(format!("{}: {e}", path.display())))?; } tmp.as_file_mut().sync_all().map_err(|e| EngineError::Other(format!("{}: {e}", path.display())))?; - // Last moment: the document was saved, reverted or closed while the bytes were written. The - // temporary file is dropped, so nothing of the stale snapshot stays. - if cancelled() { - return Ok(false); + { + // Held from the check to the rename: a cancel waits for a rename already under way (the + // cleanup after it then removes the file), and a rename after a cancel sees it. + let _publishing = PUBLISH.lock().unwrap_or_else(std::sync::PoisonError::into_inner); + // Last moment: the document was saved, reverted or closed while the bytes were written. + // The temporary file is dropped, so nothing of the stale snapshot stays. + if cancelled() { + return Ok(false); + } + // Validate the non-followed leaf again immediately before renaming the randomized + // temporary file to its fixed sibling destination. + validate_recovery_dir(dir)?; + let published = if replace { tmp.persist(path) } else { tmp.persist_noclobber(path) }; + published.map_err(|e| EngineError::Other(format!("{}: {}", path.display(), e.error)))?; } - // Validate the non-followed leaf again immediately before renaming the randomized temporary - // file to its fixed sibling destination. - validate_recovery_dir(dir)?; - let published = if replace { tmp.persist(path) } else { tmp.persist_noclobber(path) }; - published.map_err(|e| EngineError::Other(format!("{}: {}", path.display(), e.error)))?; sync_recovery_dir(dir)?; Ok(true) } diff --git a/crates/engine/src/recovery/tests.rs b/crates/engine/src/recovery/tests.rs index 3b7c2303d..79644ca3d 100644 --- a/crates/engine/src/recovery/tests.rs +++ b/crates/engine/src/recovery/tests.rs @@ -170,6 +170,65 @@ fn a_background_save_returns_while_its_write_is_held() { discard(dir.path(), uid); } +#[cfg(not(target_arch = "wasm32"))] +#[test] +fn a_document_saved_while_its_write_is_held_leaves_no_snapshot() { + 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 held = testing::hold_next_write(dir.path()); + s.execute("file.recovery.save", &json!({"background": true})).unwrap(); + held.wait_until_held(); + let saved = dir.path().join("saved.designcraft"); + s.execute("file.saveAs", &json!({"path": saved.to_string_lossy()})).unwrap(); + held.release(); + assert_eq!(finished_writes(&mut s), 0); + assert!(!files(dir.path(), uid).0.exists(), "the stale snapshot was not published"); + assert!(!files(dir.path(), uid).1.exists(), "nor its metadata"); + assert!(list(dir.path()).is_empty()); +} + +#[cfg(not(target_arch = "wasm32"))] +#[test] +fn a_background_result_goes_to_the_session_that_queued_it() { + let _turn = background(); + let dir = tempfile::tempdir().unwrap(); + let mut queued_by = dirty_session(); + queued_by.recovery_dir = Some(dir.path().to_path_buf()); + let uid = queued_by.docs[0].uid; + let mut other = dirty_session(); + queued_by.execute("file.recovery.save", &json!({"background": true})).unwrap(); + testing::wait_until_idle(); + // Another session has none of these documents: it must neither take the result nor remove + // the snapshot as if its document had been closed. + let v = other.execute("file.recovery.progress", &json!({})).unwrap(); + assert_eq!(v["written"], 0, "{v}"); + assert!(files(dir.path(), uid).0.exists(), "the snapshot is still there"); + assert_eq!(finished_writes(&mut queued_by), 1); + assert!(queued_by.docs[0].recovery_entries.iter().any(|(d, u)| *u == uid && same_directory(d, dir.path())), "the checkpoint is recorded"); + discard(dir.path(), uid); +} + +#[cfg(not(target_arch = "wasm32"))] +#[test] +fn results_for_a_session_that_is_gone_are_not_kept() { + let _turn = background(); + let dir = tempfile::tempdir().unwrap(); + let mut gone = dirty_session(); + gone.recovery_dir = Some(dir.path().to_path_buf()); + let owner = gone.recovery_owner.0; + gone.execute("file.recovery.save", &json!({"background": true})).unwrap(); + testing::wait_until_idle(); + drop(gone); + let mut other = dirty_session(); + other.execute("file.recovery.progress", &json!({})).unwrap(); + let writer = worker::started().unwrap(); + let kept = writer.undelivered.lock().unwrap_or_else(std::sync::PoisonError::into_inner); + assert!(!kept.iter().any(|d| d.owner == owner), "a result nobody can take was kept"); +} + #[test] fn a_background_save_rejects_a_background_that_is_not_true_or_false() { let dir = tempfile::tempdir().unwrap(); From 7b91cad608f2c47f626a2d4818710a98ab9f5f6c Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Wed, 11 Nov 2026 20:24:55 -0800 Subject: [PATCH 05/14] Recovery: Save cancels a document's writes to any folder Save, Revert and Close now cancel the document's background writes to every folder, including one named with `file.recovery.save {dir}` that is not in its entries yet, and remove a snapshot such a write already published even when its result has not been taken. A cancel that comes after the write published retires its result, so the removed snapshot is not recorded as a checkpoint after a later edit. Cancels apply only to writes the same session queued. The test that closes a run's lock and expects it free no longer runs while a test starts its child process, which held a copy of the lock until the child ran its program. --- crates/engine/src/recovery.rs | 64 ++++++++++++++++--- crates/engine/src/recovery/tests.rs | 96 ++++++++++++++++++++++++++--- 2 files changed, 146 insertions(+), 14 deletions(-) diff --git a/crates/engine/src/recovery.rs b/crates/engine/src/recovery.rs index dc7d63ed7..6d2eac44d 100644 --- a/crates/engine/src/recovery.rs +++ b/crates/engine/src/recovery.rs @@ -514,6 +514,15 @@ pub(crate) fn discard_document(s: &mut Session, index: usize, default_dir: Optio { entries.push((dir.to_path_buf(), uid)); } + // Background writes still queued or running for it, in any folder (one named by + // `file.recovery.save {dir}` is not in its entries until the write's result is taken): none + // may publish after this point, and what one already published is removed below. + #[cfg(not(target_arch = "wasm32"))] + for dir in worker::cancel_document(uid, s.recovery_owner.0) { + if !entries.iter().any(|(entry_dir, entry_uid)| *entry_uid == uid && same_directory(entry_dir, &dir)) { + entries.push((dir, uid)); + } + } for (dir, uid) in entries { // Session ownership governs deletion as well as writes: never consume another live // document's only source merely because its old uid matches this unrelated document. @@ -861,7 +870,7 @@ pub(crate) fn finished(_s: &mut Session) -> Finished { /// gigabytes of placed files is written without stopping the interface. #[cfg(not(target_arch = "wasm32"))] mod worker { - use std::collections::BTreeSet; + use std::collections::{BTreeMap, BTreeSet}; use std::sync::mpsc::{Receiver, Sender}; use std::sync::{Condvar, Mutex, OnceLock}; @@ -884,8 +893,9 @@ mod worker { pub(super) done: Mutex>, /// Results taken from `done` by a session that did not queue them, kept for their own. pub(super) undelivered: Mutex>, - /// Documents queued or being written: a second write of one still holds nothing newer. - running: Mutex>, + /// Documents queued or being written, with the session that queued each: a second write + /// of one still holds nothing newer. + running: Mutex>, /// A document saved, reverted or closed while its write was queued or running: the /// snapshot it is producing must not be published. cancelled: Mutex>, @@ -928,7 +938,7 @@ mod worker { // A rename under way finishes first, so the caller's cleanup comes after it; a later // one sees the mark. let _publishing = super::PUBLISH.lock().unwrap_or_else(std::sync::PoisonError::into_inner); - if !self.running.lock().is_ok_and(|running| running.contains(&key)) { + if !self.running.lock().is_ok_and(|running| running.contains_key(&key)) { return; } if let Ok(mut cancelled) = self.cancelled.lock() { @@ -936,6 +946,35 @@ mod worker { } } + /// [`Writer::cancel`] for every folder a write of `uid` queued by session `owner` is queued + /// or running for. Returns those folders, and the folders of its finished writes whose + /// results nobody has taken. + fn cancel_document(&self, uid: u64, owner: u64) -> Vec { + // Held while results are read: the writer checks for a cancel and sends its result + // under it, so a result sent after this sees the cancel. + let _publishing = super::PUBLISH.lock().unwrap_or_else(std::sync::PoisonError::into_inner); + let keys: Vec<(PathBuf, u64)> = self + .running + .lock() + .map(|running| running.iter().filter(|((_, u), o)| *u == uid && **o == owner).map(|(key, _)| key.clone()).collect()) + .unwrap_or_default(); + if let Ok(mut cancelled) = self.cancelled.lock() { + cancelled.extend(keys.iter().cloned()); + } + let mut dirs: Vec = keys.into_iter().map(|(dir, _)| dir).collect(); + // Results stay queued for their session; only their folders are read here. + let done = self.done.lock().unwrap_or_else(std::sync::PoisonError::into_inner); + let mut undelivered = self.undelivered.lock().unwrap_or_else(std::sync::PoisonError::into_inner); + undelivered.extend(done.try_iter()); + for d in undelivered.iter_mut().filter(|d| d.uid == uid && d.owner == owner && d.published) { + dirs.push(d.dir.clone()); + // The caller removes that snapshot: its result must not record it as a checkpoint + // once the document has changed again. + d.published = false; + } + dirs + } + fn cancelled(&self, dir: &Path, uid: u64) -> bool { self.cancelled.lock().is_ok_and(|cancelled| cancelled.contains(&(dir.to_path_buf(), uid))) } @@ -954,7 +993,12 @@ mod worker { // The result before the document reads as idle: a writer that is not busy // has already delivered it, so the interface never stops polling with an // answer still on its way. - let sent = done_tx.send(Done { dir: dir.clone(), uid, error, published, owner }).is_ok(); + let sent = { + let _publishing = super::PUBLISH.lock().unwrap_or_else(std::sync::PoisonError::into_inner); + // Cancelled after it was published: the canceller removes the snapshot. + let published = published && !cancelled(&dir, uid); + done_tx.send(Done { dir: dir.clone(), uid, error, published, owner }).is_ok() + }; if let Some(w) = WRITER.get().and_then(Option::as_ref) { w.finish(&dir, uid); } @@ -967,7 +1011,7 @@ mod worker { jobs, done: Mutex::new(done_rx), undelivered: Mutex::new(Vec::new()), - running: Mutex::new(BTreeSet::new()), + running: Mutex::new(BTreeMap::new()), cancelled: Mutex::new(BTreeSet::new()), idle: Condvar::new(), }) @@ -987,6 +1031,11 @@ mod worker { } } + /// Do not publish any write of `uid` in flight (see [`Writer::cancel_document`]). + pub(super) fn cancel_document(uid: u64, owner: u64) -> Vec { + started().map(|w| w.cancel_document(uid, owner)).unwrap_or_default() + } + /// Whether that write is already marked as cancelled (see [`Writer::cancelled`]). pub(super) fn cancelled(dir: &Path, uid: u64) -> bool { started().is_some_and(|w| w.cancelled(dir, uid)) @@ -1016,9 +1065,10 @@ mod worker { let w = writer()?; let mut running = w.running.lock().ok()?; let key = (entry.dir.clone(), entry.uid); - if !running.insert(key.clone()) { + if running.contains_key(&key) { return Some(false); } + running.insert(key.clone(), entry.owner); // A write is wanted again: whatever cancelled the last one does not apply to this one. if let Ok(mut cancelled) = w.cancelled.lock() { cancelled.remove(&key); diff --git a/crates/engine/src/recovery/tests.rs b/crates/engine/src/recovery/tests.rs index 79644ca3d..2d6cc8861 100644 --- a/crates/engine/src/recovery/tests.rs +++ b/crates/engine/src/recovery/tests.rs @@ -184,12 +184,57 @@ fn a_document_saved_while_its_write_is_held_leaves_no_snapshot() { let saved = dir.path().join("saved.designcraft"); s.execute("file.saveAs", &json!({"path": saved.to_string_lossy()})).unwrap(); held.release(); - assert_eq!(finished_writes(&mut s), 0); + // Before the session takes the result: a crash now must not find the stale snapshot. + testing::wait_until_idle(); assert!(!files(dir.path(), uid).0.exists(), "the stale snapshot was not published"); assert!(!files(dir.path(), uid).1.exists(), "nor its metadata"); + assert_eq!(finished_writes(&mut s), 0); assert!(list(dir.path()).is_empty()); } +#[cfg(not(target_arch = "wasm32"))] +#[test] +fn a_document_saved_while_its_write_to_a_named_folder_is_held_leaves_no_snapshot() { + // A folder given to `file.recovery.save` is not the session's, and is not in the document's + // entries until the write's result is taken. + let _turn = background(); + let named = tempfile::tempdir().unwrap(); + let mut s = dirty_session(); + let uid = s.docs[0].uid; + let held = testing::hold_next_write(named.path()); + let queued = s.execute("file.recovery.save", &json!({"background": true, "dir": named.path().to_string_lossy()})).unwrap(); + assert_eq!(queued["queued"], 1, "{queued}"); + held.wait_until_held(); + let saved = named.path().join("saved.designcraft"); + s.execute("file.saveAs", &json!({"path": saved.to_string_lossy()})).unwrap(); + held.release(); + // Before the session takes the result: a crash now must not find the stale snapshot. + testing::wait_until_idle(); + assert!(!files(named.path(), uid).0.exists(), "the stale snapshot was not published"); + assert_eq!(finished_writes(&mut s), 0); + assert!(list(named.path()).is_empty()); +} + +#[cfg(not(target_arch = "wasm32"))] +#[test] +fn saving_removes_a_finished_write_to_a_named_folder_whose_result_was_not_taken() { + let _turn = background(); + let named = tempfile::tempdir().unwrap(); + let mut s = dirty_session(); + let uid = s.docs[0].uid; + s.execute("file.recovery.save", &json!({"background": true, "dir": named.path().to_string_lossy()})).unwrap(); + testing::wait_until_idle(); + assert!(files(named.path(), uid).0.exists(), "the write finished"); + let saved = named.path().join("saved.designcraft"); + s.execute("file.saveAs", &json!({"path": saved.to_string_lossy()})).unwrap(); + assert!(!files(named.path(), uid).0.exists(), "Save removed the snapshot before its result was taken"); + // Edited again before the result is taken: the removed snapshot is not its checkpoint. + s.execute("frame.create", &json!({"rect": [20, 20, 60, 60]})).unwrap(); + assert_eq!(finished_writes(&mut s), 0); + assert!(!s.docs[0].recovery_entries.iter().any(|(d, _)| same_directory(d, named.path())), "no checkpoint for a removed snapshot"); + assert!(list(named.path()).is_empty()); +} + #[cfg(not(target_arch = "wasm32"))] #[test] fn a_background_result_goes_to_the_session_that_queued_it() { @@ -211,6 +256,28 @@ fn a_background_result_goes_to_the_session_that_queued_it() { discard(dir.path(), uid); } +#[cfg(not(target_arch = "wasm32"))] +#[test] +fn saving_in_one_session_leaves_another_sessions_write_alone() { + let _turn = background(); + let dir = tempfile::tempdir().unwrap(); + let mut queued_by = dirty_session(); + queued_by.recovery_dir = Some(dir.path().to_path_buf()); + let uid = queued_by.docs[0].uid; + // The same uid in another session (recovered documents keep the uids of their entries). + let mut other = dirty_session(); + other.docs[0].uid = uid; + let held = testing::hold_next_write(dir.path()); + queued_by.execute("file.recovery.save", &json!({"background": true})).unwrap(); + held.wait_until_held(); + let elsewhere = tempfile::tempdir().unwrap(); + other.execute("file.saveAs", &json!({"path": elsewhere.path().join("other.designcraft").to_string_lossy()})).unwrap(); + held.release(); + assert_eq!(finished_writes(&mut queued_by), 1); + assert!(files(dir.path(), uid).0.exists(), "the other session's save did not cancel this write"); + discard(dir.path(), uid); +} + #[cfg(not(target_arch = "wasm32"))] #[test] fn results_for_a_session_that_is_gone_are_not_kept() { @@ -709,6 +776,11 @@ fn restart_recovery_child() { } } +/// A child process starts with a copy of every open file of this one until it runs its program, so +/// a lock file closed meanwhile stays locked: a test that closes a lock and expects it free does +/// not run while a child is started. +static SPAWN: std::sync::Mutex<()> = std::sync::Mutex::new(()); + fn run_restart_case(mode: &str) { let source = restart_fixture(); if mode == "max_uid" { @@ -717,12 +789,20 @@ fn run_restart_case(mode: &str) { std::fs::rename(doc, max_doc).unwrap(); std::fs::rename(meta, max_meta).unwrap(); } - let output = std::process::Command::new(std::env::current_exe().unwrap()) - .args(["--exact", "recovery::tests::restart_recovery_child", "--ignored", "--nocapture"]) - .env("DESIGNCRAFT_TEST_RECOVERY_SOURCE", source.path()) - .env("DESIGNCRAFT_TEST_RECOVERY_MODE", mode) - .output() - .unwrap(); + // `spawn` returns once the child runs its program, and its copies of this process's files + // are closed by then. + let child = { + let _spawning = SPAWN.lock().unwrap_or_else(std::sync::PoisonError::into_inner); + std::process::Command::new(std::env::current_exe().unwrap()) + .args(["--exact", "recovery::tests::restart_recovery_child", "--ignored", "--nocapture"]) + .env("DESIGNCRAFT_TEST_RECOVERY_SOURCE", source.path()) + .env("DESIGNCRAFT_TEST_RECOVERY_MODE", mode) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) + .spawn() + .unwrap() + }; + let output = child.wait_with_output().unwrap(); assert!(output.status.success(), "fresh child failed: {}{}", String::from_utf8_lossy(&output.stdout), String::from_utf8_lossy(&output.stderr)); let child_stdout = String::from_utf8_lossy(&output.stdout); assert!(child_stdout.contains("test result: ok. 1 passed; 0 failed;"), "the fresh-process helper did not run exactly once: {child_stdout}"); @@ -953,6 +1033,8 @@ fn documents_of_two_apps_that_ended_are_both_recovered() { fn a_running_apps_recovery_data_is_left_alone() { let root = tempfile::tempdir().unwrap(); let running = run_left(root.path(), "run-7-0", "still open", 1); + // No child starts while the lock is open, so closing it frees it (see `SPAWN`). + let _no_spawn = SPAWN.lock().unwrap_or_else(std::sync::PoisonError::into_inner); let lock = std::fs::File::open(running.join(LOCK)).unwrap(); lock.try_lock().unwrap(); let own = run_dir(root.path()).unwrap(); From 04fac58d9b9b229805e0fb165763b2fe0463afcc Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Wed, 11 Nov 2026 20:43:12 -0800 Subject: [PATCH 06/14] Recovery: writes are keyed by the folder's canonical path A synchronous recovery write now cancels a background write to the same folder named another way, and a cancel also matches the folder as the session named it. Background writes whose results have not been taken count toward the per-document checkpoint limit, cancelled ones excluded. Save, Revert and Close clean up a cancelled write's folder under the name the session used for it. --- crates/engine/src/recovery.rs | 103 ++++++++++++++++++++++------ crates/engine/src/recovery/tests.rs | 57 +++++++++++++++ 2 files changed, 138 insertions(+), 22 deletions(-) diff --git a/crates/engine/src/recovery.rs b/crates/engine/src/recovery.rs index 6d2eac44d..f5bafd316 100644 --- a/crates/engine/src/recovery.rs +++ b/crates/engine/src/recovery.rs @@ -636,7 +636,21 @@ pub(crate) fn save_tracked(s: &mut Session, dir: &Path) -> Result { /// Reject a complete batch before writing rather than silently losing old ownership records. fn reject_overfull_checkpoints(s: &Session, dir: &Path) -> Result<()> { for document in &s.docs { - if document.is_dirty() && !has_checkpoint(document, dir) && document.recovery_entries.len() >= MAX_RECOVERY_ENTRIES { + if !document.is_dirty() || has_checkpoint(document, dir) { + continue; + } + // Background writes whose results are not taken yet hold checkpoints the entries do not. + #[cfg(not(target_arch = "wasm32"))] + let pending = worker::pending_folders(document.uid, s.recovery_owner.0) + .into_iter() + .map(|pending| directory_key(&pending).unwrap_or(pending)) + .collect::>() + .iter() + .filter(|pending| !same_directory(pending, dir) && !has_checkpoint(document, pending)) + .count(); + #[cfg(target_arch = "wasm32")] + let pending = 0; + if document.recovery_entries.len() + pending >= MAX_RECOVERY_ENTRIES { return Err(EngineError::Other( "too many recovery checkpoints; save or close the document before choosing another recovery folder".into(), )); @@ -895,7 +909,7 @@ mod worker { pub(super) undelivered: Mutex>, /// Documents queued or being written, with the session that queued each: a second write /// of one still holds nothing newer. - running: Mutex>, + running: Mutex>, /// A document saved, reverted or closed while its write was queued or running: the /// snapshot it is producing must not be published. cancelled: Mutex>, @@ -903,15 +917,27 @@ mod worker { idle: Condvar, } + /// A write in flight: the session that queued it, and its folder as that session named it. + struct Queued { + owner: u64, + dir: PathBuf, + } + static WRITER: OnceLock> = OnceLock::new(); + /// A write's key: the folder's canonical path, so two spellings of one folder are one write. + fn key(dir: &Path, uid: u64) -> (PathBuf, u64) { + (super::directory_key(dir).unwrap_or_else(|_| dir.to_path_buf()), uid) + } + impl Writer { pub(super) fn busy(&self) -> bool { self.running.lock().is_ok_and(|running| !running.is_empty()) } - fn finish(&self, dir: &Path, uid: u64) { - let key = (dir.to_path_buf(), uid); + /// `key` is the job's ([`Snapshot::key`], uid). + fn finish(&self, key: &(PathBuf, u64)) { + let key = key.clone(); if let Ok(mut running) = self.running.lock() { running.remove(&key); } @@ -934,15 +960,19 @@ mod worker { /// reverted or closed. Only a write that is queued or running is marked, so the set stays /// as small as the work in flight. fn cancel(&self, dir: &Path, uid: u64) { - let key = (dir.to_path_buf(), uid); + let canonical = key(dir, uid); // A rename under way finishes first, so the caller's cleanup comes after it; a later // one sees the mark. let _publishing = super::PUBLISH.lock().unwrap_or_else(std::sync::PoisonError::into_inner); - if !self.running.lock().is_ok_and(|running| running.contains_key(&key)) { - return; - } + // The folder's canonical path, or its name as given: a link in the path may lead + // elsewhere now than when the write was queued. + let keys: Vec<(PathBuf, u64)> = self + .running + .lock() + .map(|running| running.iter().filter(|(k, q)| **k == canonical || (k.1 == uid && q.dir == dir)).map(|(k, _)| k.clone()).collect()) + .unwrap_or_default(); if let Ok(mut cancelled) = self.cancelled.lock() { - cancelled.insert(key); + cancelled.extend(keys); } } @@ -953,15 +983,18 @@ mod worker { // Held while results are read: the writer checks for a cancel and sends its result // under it, so a result sent after this sees the cancel. let _publishing = super::PUBLISH.lock().unwrap_or_else(std::sync::PoisonError::into_inner); - let keys: Vec<(PathBuf, u64)> = self + // The canonical keys to mark, and the folders as the session named them, which is how + // the cleanup finds this run's folder. + let (keys, mut dirs): (Vec<(PathBuf, u64)>, Vec) = self .running .lock() - .map(|running| running.iter().filter(|((_, u), o)| *u == uid && **o == owner).map(|(key, _)| key.clone()).collect()) + .map(|running| { + running.iter().filter(|((_, u), q)| *u == uid && q.owner == owner).map(|(key, q)| (key.clone(), q.dir.clone())).unzip() + }) .unwrap_or_default(); if let Ok(mut cancelled) = self.cancelled.lock() { - cancelled.extend(keys.iter().cloned()); + cancelled.extend(keys); } - let mut dirs: Vec = keys.into_iter().map(|(dir, _)| dir).collect(); // Results stay queued for their session; only their folders are read here. let done = self.done.lock().unwrap_or_else(std::sync::PoisonError::into_inner); let mut undelivered = self.undelivered.lock().unwrap_or_else(std::sync::PoisonError::into_inner); @@ -975,8 +1008,26 @@ mod worker { dirs } - fn cancelled(&self, dir: &Path, uid: u64) -> bool { - self.cancelled.lock().is_ok_and(|cancelled| cancelled.contains(&(dir.to_path_buf(), uid))) + /// The folders of writes of `uid` queued by session `owner` that are in flight, or finished + /// and published with their results not taken yet: checkpoints its entries do not hold yet. + fn pending_folders(&self, uid: u64, owner: u64) -> Vec { + let keys: Vec<(PathBuf, u64)> = self + .running + .lock() + .map(|running| running.iter().filter(|((_, u), q)| *u == uid && q.owner == owner).map(|(key, _)| key.clone()).collect()) + .unwrap_or_default(); + // A cancelled write publishes nothing. + let mut dirs: Vec = keys.into_iter().filter(|key| !self.cancelled(key)).map(|(dir, _)| dir).collect(); + let done = self.done.lock().unwrap_or_else(std::sync::PoisonError::into_inner); + let mut undelivered = self.undelivered.lock().unwrap_or_else(std::sync::PoisonError::into_inner); + undelivered.extend(done.try_iter()); + dirs.extend(undelivered.iter().filter(|d| d.uid == uid && d.owner == owner && d.published).map(|d| d.dir.clone())); + dirs + } + + /// `key` as in [`Writer::finish`]. + fn cancelled(&self, key: &(PathBuf, u64)) -> bool { + self.cancelled.lock().is_ok_and(|cancelled| cancelled.contains(key)) } } @@ -989,6 +1040,7 @@ mod worker { let spawned = std::thread::Builder::new().name("designcraft-recovery".into()).spawn(move || { while let Ok(job) = job_rx.recv() { let (published, error) = guarded_write(&job); + let key = (job.key.clone(), job.uid); let (dir, uid, owner) = (job.dir, job.uid, job.owner); // The result before the document reads as idle: a writer that is not busy // has already delivered it, so the interface never stops polling with an @@ -996,11 +1048,11 @@ mod worker { let sent = { let _publishing = super::PUBLISH.lock().unwrap_or_else(std::sync::PoisonError::into_inner); // Cancelled after it was published: the canceller removes the snapshot. - let published = published && !cancelled(&dir, uid); - done_tx.send(Done { dir: dir.clone(), uid, error, published, owner }).is_ok() + let published = published && !started().is_some_and(|w| w.cancelled(&key)); + done_tx.send(Done { dir, uid, error, published, owner }).is_ok() }; if let Some(w) = WRITER.get().and_then(Option::as_ref) { - w.finish(&dir, uid); + w.finish(&key); } if !sent { break; @@ -1031,21 +1083,28 @@ mod worker { } } + /// See [`Writer::pending_folders`]. + pub(super) fn pending_folders(uid: u64, owner: u64) -> Vec { + started().map(|w| w.pending_folders(uid, owner)).unwrap_or_default() + } + /// Do not publish any write of `uid` in flight (see [`Writer::cancel_document`]). pub(super) fn cancel_document(uid: u64, owner: u64) -> Vec { started().map(|w| w.cancel_document(uid, owner)).unwrap_or_default() } /// Whether that write is already marked as cancelled (see [`Writer::cancelled`]). + #[cfg(test)] pub(super) fn cancelled(dir: &Path, uid: u64) -> bool { - started().is_some_and(|w| w.cancelled(dir, uid)) + started().is_some_and(|w| w.cancelled(&key(dir, uid))) } /// Write one snapshot, turning a panic into its error: a panic that ended this thread would /// leave the document marked as running for ever (so it is never written again) and the /// interface asking for frames for ever. A safety net: a writer still must not panic. fn guarded_write(job: &Snapshot) -> (bool, Option) { - match guarded(|| Ok(write_snapshot(job, || cancelled(&job.dir, job.uid)))) { + let key = (job.key.clone(), job.uid); + match guarded(|| Ok(write_snapshot(job, || started().is_some_and(|w| w.cancelled(&key))))) { // A primary that landed is this run's to remove, even if its metadata failed. Ok(written) => (written.published, written.error.map(|e| e.to_string())), Err(e) => (false, Some(e.to_string())), @@ -1064,11 +1123,11 @@ mod worker { pub(super) fn submit(entry: Snapshot) -> Option { let w = writer()?; let mut running = w.running.lock().ok()?; - let key = (entry.dir.clone(), entry.uid); + let key = (entry.key.clone(), entry.uid); if running.contains_key(&key) { return Some(false); } - running.insert(key.clone(), entry.owner); + running.insert(key.clone(), Queued { owner: entry.owner, dir: entry.dir.clone() }); // A write is wanted again: whatever cancelled the last one does not apply to this one. if let Ok(mut cancelled) = w.cancelled.lock() { cancelled.remove(&key); diff --git a/crates/engine/src/recovery/tests.rs b/crates/engine/src/recovery/tests.rs index 2d6cc8861..2353e476d 100644 --- a/crates/engine/src/recovery/tests.rs +++ b/crates/engine/src/recovery/tests.rs @@ -278,6 +278,63 @@ fn saving_in_one_session_leaves_another_sessions_write_alone() { discard(dir.path(), uid); } +#[cfg(not(target_arch = "wasm32"))] +#[test] +fn a_write_now_cancels_a_background_write_to_the_same_folder_spelled_another_way() { + let _turn = background(); + let dir = tempfile::tempdir().unwrap(); + std::fs::create_dir(dir.path().join("sub")).unwrap(); + let spelled = dir.path().join("sub").join(".."); + let mut s = dirty_session(); + s.recovery_dir = Some(spelled.clone()); + let uid = s.docs[0].uid; + let held = testing::hold_next_write(&spelled); + s.execute("file.recovery.save", &json!({"background": true})).unwrap(); + held.wait_until_held(); + // The newer state, written now under the folder's plain name. + s.execute("frame.create", &json!({"rect": [20, 20, 60, 60]})).unwrap(); + s.execute("file.recovery.save", &json!({"dir": dir.path().to_string_lossy()})).unwrap(); + assert!(worker::cancelled(&spelled, uid), "the older write is cancelled"); + held.release(); + testing::wait_until_idle(); + let _ = s.execute("file.recovery.progress", &json!({})).unwrap(); + let mut reopened = Session::new(); + assert_eq!(open(&mut reopened, dir.path()).unwrap().len(), 1); + assert_eq!(reopened.doc().unwrap().doc.spreads[0].items.len(), 2, "the newer snapshot was kept"); + discard(dir.path(), reopened.doc().unwrap().uid); + discard(dir.path(), uid); +} + +#[cfg(not(target_arch = "wasm32"))] +#[test] +fn queued_background_writes_count_toward_the_checkpoint_limit() { + let _turn = background(); + let mut s = dirty_session(); + let folders: Vec = (0..=MAX_RECOVERY_ENTRIES).map(|_| tempfile::tempdir().unwrap()).collect(); + // The first write is held, so every later one is still queued when the next is asked for. + let held = testing::hold_next_write(folders[0].path()); + for folder in &folders[..MAX_RECOVERY_ENTRIES] { + let queued = s.execute("file.recovery.save", &json!({"background": true, "dir": folder.path().to_string_lossy()})).unwrap(); + assert_eq!(queued["queued"], 1, "{queued}"); + } + let over = folders[MAX_RECOVERY_ENTRIES].path(); + let err = s.execute("file.recovery.save", &json!({"background": true, "dir": over.to_string_lossy()})).unwrap_err(); + assert!(err.to_string().contains("too many recovery checkpoints"), "{err}"); + // Saving cancels the queued writes: they no longer count once the document changes again. + let saved = folders[0].path().join("saved.designcraft"); + s.execute("file.saveAs", &json!({"path": saved.to_string_lossy()})).unwrap(); + s.execute("frame.create", &json!({"rect": [20, 20, 60, 60]})).unwrap(); + let queued = s.execute("file.recovery.save", &json!({"background": true, "dir": over.to_string_lossy()})).unwrap(); + assert_eq!(queued["queued"], 1, "{queued}"); + held.release(); + assert_eq!(finished_writes(&mut s), 1, "only the write queued after the save is a checkpoint"); + assert_eq!(list(over).len(), 1); + let uid = s.docs[0].uid; + for folder in &folders { + discard(folder.path(), uid); + } +} + #[cfg(not(target_arch = "wasm32"))] #[test] fn results_for_a_session_that_is_gone_are_not_kept() { From eff8865f86c42218f8aba795c722b5607b16e802 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Wed, 11 Nov 2026 20:50:31 -0800 Subject: [PATCH 07/14] Recovery: an edit after a save is written once the cancelled write stops A recovery write queued while a cancelled write of the same document and folder is still running now follows it instead of being refused until the next timer tick. A write in a folder that is not the run's own is recorded as the run's in the same step as its rename, so a save right after it can remove it. --- crates/engine/src/recovery.rs | 161 ++++++++++++++++++---------- crates/engine/src/recovery/tests.rs | 27 +++++ 2 files changed, 133 insertions(+), 55 deletions(-) diff --git a/crates/engine/src/recovery.rs b/crates/engine/src/recovery.rs index f5bafd316..e2e09a42b 100644 --- a/crates/engine/src/recovery.rs +++ b/crates/engine/src/recovery.rs @@ -737,6 +737,14 @@ fn write_snapshot(entry: &Snapshot, cancelled: impl Fn() -> bool) -> Written { entry.own, |out| designcraft_format::write(out, &entry.doc, None).map_err(|e| EngineError::Other(e.to_string())), &cancelled, + // Written: in a folder that is not this run's own, the entry is now this run's to replace + // and remove (Save and Close must clean up the snapshot they wrote). Recorded with the + // rename, so a cleanup after it can remove the entry. + || { + if !entry.own { + WRITTEN.lock().unwrap_or_else(std::sync::PoisonError::into_inner).insert((entry.key.clone(), entry.uid)); + } + }, ); match published { Ok(true) => {} @@ -744,11 +752,6 @@ fn write_snapshot(entry: &Snapshot, cancelled: impl Fn() -> bool) -> Written { Ok(false) => return Written { published: false, error: None }, Err(e) => return Written { published: false, error: Some(e) }, } - // Written: in a folder that is not this run's own, the entry is now this run's to replace and - // remove (Save and Close must clean up the snapshot they wrote). - if !entry.own { - WRITTEN.lock().unwrap_or_else(std::sync::PoisonError::into_inner).insert((entry.key.clone(), entry.uid)); - } let metadata = json!({"path": entry.path, "title": entry.title, "saved": designcraft_doc::vars::now()}); // A cancel after the primary landed removes it; the metadata must not be left without it. let bytes = metadata.to_string(); @@ -757,6 +760,7 @@ fn write_snapshot(entry: &Snapshot, cancelled: impl Fn() -> bool) -> Written { entry.own, |out| out.write_all(bytes.as_bytes()).map_err(|e| EngineError::Other(format!("{}: {e}", meta.display()))), &cancelled, + || {}, ) .err(); Written { published: true, error } @@ -921,6 +925,9 @@ mod worker { struct Queued { owner: u64, dir: PathBuf, + /// A newer write of the same document and folder, queued after this one was cancelled: + /// it is written as soon as this one stops. + next: Option, } static WRITER: OnceLock> = OnceLock::new(); @@ -935,16 +942,30 @@ mod worker { self.running.lock().is_ok_and(|running| !running.is_empty()) } - /// `key` is the job's ([`Snapshot::key`], uid). - fn finish(&self, key: &(PathBuf, u64)) { - let key = key.clone(); - if let Ok(mut running) = self.running.lock() { - running.remove(&key); - } - if let Ok(mut cancelled) = self.cancelled.lock() { - cancelled.remove(&key); - } + /// `key` is the job's ([`Snapshot::key`], uid). Returns the write queued to follow it. + fn finish(&self, key: &(PathBuf, u64)) -> Option { + let next = match self.running.lock() { + Ok(mut running) => { + let next = running.get_mut(key).and_then(|q| q.next.take()); + match (&next, running.get_mut(key)) { + (Some(n), Some(q)) => { + q.owner = n.owner; + q.dir = n.dir.clone(); + } + _ => { + running.remove(key); + } + } + // The mark was for the write that stopped, not the one that follows it. + if let Ok(mut cancelled) = self.cancelled.lock() { + cancelled.remove(key); + } + next + } + Err(_) => None, + }; self.idle.notify_all(); + next } /// Block until no write is queued or running. @@ -966,11 +987,15 @@ mod worker { let _publishing = super::PUBLISH.lock().unwrap_or_else(std::sync::PoisonError::into_inner); // The folder's canonical path, or its name as given: a link in the path may lead // elsewhere now than when the write was queued. - let keys: Vec<(PathBuf, u64)> = self - .running - .lock() - .map(|running| running.iter().filter(|(k, q)| **k == canonical || (k.1 == uid && q.dir == dir)).map(|(k, _)| k.clone()).collect()) - .unwrap_or_default(); + let Ok(mut running) = self.running.lock() else { return }; + let keys: Vec<(PathBuf, u64)> = + running.iter().filter(|(k, q)| **k == canonical || (k.1 == uid && q.dir == dir)).map(|(k, _)| k.clone()).collect(); + // A write queued to follow it is older than what the caller has now, too. + for k in &keys { + if let Some(q) = running.get_mut(k) { + q.next = None; + } + } if let Ok(mut cancelled) = self.cancelled.lock() { cancelled.extend(keys); } @@ -985,15 +1010,17 @@ mod worker { let _publishing = super::PUBLISH.lock().unwrap_or_else(std::sync::PoisonError::into_inner); // The canonical keys to mark, and the folders as the session named them, which is how // the cleanup finds this run's folder. - let (keys, mut dirs): (Vec<(PathBuf, u64)>, Vec) = self - .running - .lock() - .map(|running| { - running.iter().filter(|((_, u), q)| *u == uid && q.owner == owner).map(|(key, q)| (key.clone(), q.dir.clone())).unzip() - }) - .unwrap_or_default(); - if let Ok(mut cancelled) = self.cancelled.lock() { - cancelled.extend(keys); + let mut dirs = Vec::new(); + if let Ok(mut running) = self.running.lock() { + let mut keys = Vec::new(); + for (key, q) in running.iter_mut().filter(|((_, u), q)| *u == uid && q.owner == owner) { + keys.push(key.clone()); + dirs.push(q.dir.clone()); + q.next = None; + } + if let Ok(mut cancelled) = self.cancelled.lock() { + cancelled.extend(keys); + } } // Results stay queued for their session; only their folders are read here. let done = self.done.lock().unwrap_or_else(std::sync::PoisonError::into_inner); @@ -1011,13 +1038,20 @@ mod worker { /// The folders of writes of `uid` queued by session `owner` that are in flight, or finished /// and published with their results not taken yet: checkpoints its entries do not hold yet. fn pending_folders(&self, uid: u64, owner: u64) -> Vec { - let keys: Vec<(PathBuf, u64)> = self + // A cancelled write publishes nothing; a write queued to follow it does. + let mut dirs: Vec = self .running .lock() - .map(|running| running.iter().filter(|((_, u), q)| *u == uid && q.owner == owner).map(|(key, _)| key.clone()).collect()) + .map(|running| { + running + .iter() + .filter(|(key, q)| { + key.1 == uid && ((q.owner == owner && !self.cancelled(key)) || q.next.as_ref().is_some_and(|n| n.owner == owner)) + }) + .map(|((dir, _), _)| dir.clone()) + .collect() + }) .unwrap_or_default(); - // A cancelled write publishes nothing. - let mut dirs: Vec = keys.into_iter().filter(|key| !self.cancelled(key)).map(|(dir, _)| dir).collect(); let done = self.done.lock().unwrap_or_else(std::sync::PoisonError::into_inner); let mut undelivered = self.undelivered.lock().unwrap_or_else(std::sync::PoisonError::into_inner); undelivered.extend(done.try_iter()); @@ -1038,24 +1072,25 @@ mod worker { let (jobs, job_rx) = std::sync::mpsc::channel::(); let (done_tx, done_rx) = std::sync::mpsc::channel::(); let spawned = std::thread::Builder::new().name("designcraft-recovery".into()).spawn(move || { - while let Ok(job) = job_rx.recv() { - let (published, error) = guarded_write(&job); - let key = (job.key.clone(), job.uid); - let (dir, uid, owner) = (job.dir, job.uid, job.owner); - // The result before the document reads as idle: a writer that is not busy - // has already delivered it, so the interface never stops polling with an - // answer still on its way. - let sent = { - let _publishing = super::PUBLISH.lock().unwrap_or_else(std::sync::PoisonError::into_inner); - // Cancelled after it was published: the canceller removes the snapshot. - let published = published && !started().is_some_and(|w| w.cancelled(&key)); - done_tx.send(Done { dir, uid, error, published, owner }).is_ok() - }; - if let Some(w) = WRITER.get().and_then(Option::as_ref) { - w.finish(&key); - } - if !sent { - break; + while let Ok(first) = job_rx.recv() { + let mut queued = Some(first); + while let Some(job) = queued.take() { + let (published, error) = guarded_write(&job); + let key = (job.key.clone(), job.uid); + let (dir, uid, owner) = (job.dir, job.uid, job.owner); + // The result before the document reads as idle: a writer that is not + // busy has already delivered it, so the interface never stops polling + // with an answer still on its way. + let sent = { + let _publishing = super::PUBLISH.lock().unwrap_or_else(std::sync::PoisonError::into_inner); + // Cancelled after it was published: the canceller removes the snapshot. + let published = published && !started().is_some_and(|w| w.cancelled(&key)); + done_tx.send(Done { dir, uid, error, published, owner }).is_ok() + }; + queued = WRITER.get().and_then(Option::as_ref).and_then(|w| w.finish(&key)); + if !sent { + return; + } } } }); @@ -1124,10 +1159,16 @@ mod worker { let w = writer()?; let mut running = w.running.lock().ok()?; let key = (entry.key.clone(), entry.uid); - if running.contains_key(&key) { + if let Some(q) = running.get_mut(&key) { + // The write in flight was cancelled (the document was saved since), so it holds + // nothing to recover: this one follows it, in place of any that was waiting. + if w.cancelled.lock().is_ok_and(|cancelled| cancelled.contains(&key)) { + q.next = Some(entry); + return Some(true); + } return Some(false); } - running.insert(key.clone(), Queued { owner: entry.owner, dir: entry.dir.clone() }); + running.insert(key.clone(), Queued { owner: entry.owner, dir: entry.dir.clone(), next: None }); // A write is wanted again: whatever cancelled the last one does not apply to this one. if let Ok(mut cancelled) = w.cancelled.lock() { cancelled.remove(&key); @@ -1286,8 +1327,14 @@ fn secure_recovery_dir(_dir: &Path) -> Result { #[cfg(not(target_arch = "wasm32"))] fn publish_recovery_file(path: &Path, bytes: &[u8], replace: bool) -> Result<()> { - publish_recovery_written(path, replace, |out| out.write_all(bytes).map_err(|e| EngineError::Other(format!("{}: {e}", path.display()))), || false) - .map(|_| ()) + publish_recovery_written( + path, + replace, + |out| out.write_all(bytes).map_err(|e| EngineError::Other(format!("{}: {e}", path.display()))), + || false, + || {}, + ) + .map(|_| ()) } /// Serializes the last check for a cancelled write with the rename that publishes it, and with @@ -1298,13 +1345,15 @@ static PUBLISH: std::sync::Mutex<()> = std::sync::Mutex::new(()); /// [`publish_recovery_file`], with `write` filling the temporary file: a large document is written /// straight to disk instead of being built in memory first. Returns whether it was published; /// `cancelled` is asked after the bytes are written and before the rename, so a document saved or -/// closed meanwhile leaves nothing behind. +/// closed meanwhile leaves nothing behind. `on_publish` runs right after the rename, before a +/// cancel can. #[cfg(not(target_arch = "wasm32"))] fn publish_recovery_written( path: &Path, replace: bool, write: impl FnOnce(&mut std::io::BufWriter<&mut std::fs::File>) -> Result<()>, cancelled: impl Fn() -> bool, + on_publish: impl FnOnce(), ) -> Result { let Some(dir) = path.parent() else { return Err(EngineError::Other(format!("{} has no parent folder", path.display()))); @@ -1340,6 +1389,7 @@ fn publish_recovery_written( validate_recovery_dir(dir)?; let published = if replace { tmp.persist(path) } else { tmp.persist_noclobber(path) }; published.map_err(|e| EngineError::Other(format!("{}: {}", path.display(), e.error)))?; + on_publish(); } sync_recovery_dir(dir)?; Ok(true) @@ -1406,6 +1456,7 @@ fn publish_recovery_written( _replace: bool, _write: impl FnOnce(&mut std::io::BufWriter<&mut std::fs::File>) -> Result<()>, _cancelled: impl Fn() -> bool, + _on_publish: impl FnOnce(), ) -> Result { Err(EngineError::Other("recovery files are unavailable on the web".into())) } diff --git a/crates/engine/src/recovery/tests.rs b/crates/engine/src/recovery/tests.rs index 2353e476d..c59c5ee76 100644 --- a/crates/engine/src/recovery/tests.rs +++ b/crates/engine/src/recovery/tests.rs @@ -235,6 +235,33 @@ fn saving_removes_a_finished_write_to_a_named_folder_whose_result_was_not_taken( assert!(list(named.path()).is_empty()); } +#[cfg(not(target_arch = "wasm32"))] +#[test] +fn an_edit_after_a_save_is_written_after_the_cancelled_write_stops() { + 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 held = testing::hold_next_write(dir.path()); + s.execute("file.recovery.save", &json!({"background": true})).unwrap(); + held.wait_until_held(); + let saved = tempfile::tempdir().unwrap(); + s.execute("file.saveAs", &json!({"path": saved.path().join("saved.designcraft").to_string_lossy()})).unwrap(); + s.execute("frame.create", &json!({"rect": [20, 20, 60, 60]})).unwrap(); + // The held write was cancelled: the new edit is queued behind it instead of waiting for the + // next timer tick. + let queued = s.execute("file.recovery.save", &json!({"background": true})).unwrap(); + assert_eq!(queued["queued"], 1, "{queued}"); + held.release(); + assert_eq!(finished_writes(&mut s), 1); + let mut reopened = Session::new(); + assert_eq!(open(&mut reopened, dir.path()).unwrap().len(), 1); + assert_eq!(reopened.doc().unwrap().doc.spreads[0].items.len(), 2, "the snapshot after the edit was written"); + discard(dir.path(), reopened.doc().unwrap().uid); + discard(dir.path(), uid); +} + #[cfg(not(target_arch = "wasm32"))] #[test] fn a_background_result_goes_to_the_session_that_queued_it() { From 966c8a587cce0d2853f1977cb73889ea4d8446d1 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Wed, 11 Nov 2026 21:01:47 -0800 Subject: [PATCH 08/14] Recovery: a cancel stops a background write at its next chunk A save, revert or close during a background recovery write now stops the write at its next piece of output (at most 1 MiB) instead of after the whole package, and a stopped write is not reported as a failure. --- crates/engine/src/lib.rs | 1 + crates/engine/src/recovery.rs | 41 ++++++++++++++++++++++++++--- crates/engine/src/recovery/tests.rs | 20 ++++++++++++++ 3 files changed, 59 insertions(+), 3 deletions(-) diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index bb67875b4..363f4e9cd 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -414,6 +414,7 @@ pub struct Session { pub recovery_dir: Option, /// This session's number for the background recovery writer, which returns each result to /// the session that queued it. + #[cfg_attr(target_arch = "wasm32", allow(dead_code))] pub(crate) recovery_owner: recovery::Owner, /// Copied text with its formatting (a frameless story slice), its plain text, and what pasting /// it needs from its document: a copy holding only the stories its anchored text frames show diff --git a/crates/engine/src/recovery.rs b/crates/engine/src/recovery.rs index e2e09a42b..3b4060763 100644 --- a/crates/engine/src/recovery.rs +++ b/crates/engine/src/recovery.rs @@ -725,6 +725,36 @@ struct Written { error: Option, } +/// A writer that fails once `cancelled` says so, checked before each write of at most +/// [`STOP_CHECK_BYTES`]. +struct StopWhen<'a, W, C> { + out: &'a mut W, + cancelled: &'a C, +} + +/// The most [`StopWhen`] writes between two checks for a cancel. +const STOP_CHECK_BYTES: usize = 1 << 20; + +impl bool> std::io::Write for StopWhen<'_, W, C> { + fn write(&mut self, buf: &[u8]) -> std::io::Result { + if (self.cancelled)() { + return Err(std::io::Error::other("recovery write cancelled")); + } + // A large placed file is written in pieces, so a cancel does not wait for all of it. + self.out.write(&buf[..buf.len().min(STOP_CHECK_BYTES)]) + } + + fn flush(&mut self) -> std::io::Result<()> { + self.out.flush() + } +} + +impl std::io::Seek for StopWhen<'_, W, C> { + fn seek(&mut self, pos: std::io::SeekFrom) -> std::io::Result { + self.out.seek(pos) + } +} + /// Publish a document's recovery entry before its optional metadata. `cancelled` says whether the /// 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. @@ -735,7 +765,10 @@ fn write_snapshot(entry: &Snapshot, cancelled: impl Fn() -> bool) -> Written { let published = publish_recovery_written( &doc, entry.own, - |out| designcraft_format::write(out, &entry.doc, None).map_err(|e| EngineError::Other(e.to_string())), + // A cancel stops the write at its next chunk instead of after the whole package. + |out| { + designcraft_format::write(&mut StopWhen { out, cancelled: &cancelled }, &entry.doc, None).map_err(|e| EngineError::Other(e.to_string())) + }, &cancelled, // Written: in a folder that is not this run's own, the entry is now this run's to replace // and remove (Save and Close must clean up the snapshot they wrote). Recorded with the @@ -748,8 +781,10 @@ fn write_snapshot(entry: &Snapshot, cancelled: impl Fn() -> bool) -> Written { ); match published { Ok(true) => {} - // Cancelled while writing: no snapshot and no metadata for it. + // Cancelled while writing: no snapshot and no metadata for it. A write stopped part way + // reports an error, which is the cancel's and not a failure. Ok(false) => return Written { published: false, error: None }, + Err(_) if cancelled() => return Written { published: false, error: None }, Err(e) => return Written { published: false, error: Some(e) }, } let metadata = json!({"path": entry.path, "title": entry.title, "saved": designcraft_doc::vars::now()}); @@ -758,7 +793,7 @@ fn write_snapshot(entry: &Snapshot, cancelled: impl Fn() -> bool) -> Written { let error = publish_recovery_written( &meta, entry.own, - |out| out.write_all(bytes.as_bytes()).map_err(|e| EngineError::Other(format!("{}: {e}", meta.display()))), + |out| std::io::Write::write_all(out, bytes.as_bytes()).map_err(|e| EngineError::Other(format!("{}: {e}", meta.display()))), &cancelled, || {}, ) diff --git a/crates/engine/src/recovery/tests.rs b/crates/engine/src/recovery/tests.rs index c59c5ee76..4534c3c37 100644 --- a/crates/engine/src/recovery/tests.rs +++ b/crates/engine/src/recovery/tests.rs @@ -514,6 +514,26 @@ fn a_write_cancelled_midway_publishes_nothing() { ); } +#[test] +fn a_cancel_stops_a_write_before_the_package_is_finished() { + let dir = tempfile::tempdir().unwrap(); + let s = dirty_session(); + let entry = Snapshot::of(&prepare_save(&s, dir.path()).unwrap(), dir.path(), &s.docs[0]); + // Cancelled once the write has started: the first check passes, every later one fails. + let checks = std::cell::Cell::new(0); + let written = write_snapshot(&entry, || { + checks.set(checks.get() + 1); + checks.get() > 1 + }); + assert!(!written.published && written.error.is_none(), "a stopped write is not an error"); + assert!(checks.get() >= 2, "the write checked again before it finished"); + assert!(!files(dir.path(), entry.uid).0.exists(), "nothing was published"); + assert!( + std::fs::read_dir(dir.path()).unwrap().all(|e| e.is_ok_and(|e| !e.file_name().to_string_lossy().starts_with(".designcraft-recovery-"))), + "no temporary file is left" + ); +} + #[cfg(not(target_arch = "wasm32"))] #[test] fn only_a_queued_write_is_cancelled() { From 1eda04663b84f2aa87a4f72953b99efa9ed97476 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Wed, 11 Nov 2026 22:07:30 -0800 Subject: [PATCH 09/14] Recovery: a clean document stops its background writes; a failed folder sync keeps the snapshot tracked - recovery::save cancels the background writes the session queued for a clean document and removes what a finished one left, so an older snapshot is not published after Undo returns the document to its saved state. - Cancelling a document's writes leaves a write another session queued to follow in place. - When the folder sync fails after the rename, the snapshot is reported as published with the error, so it is tracked and Save or Close removes it. --- crates/engine/src/recovery.rs | 45 ++++++++++- crates/engine/src/recovery/tests.rs | 115 ++++++++++++++++++++++++++++ 2 files changed, 156 insertions(+), 4 deletions(-) diff --git a/crates/engine/src/recovery.rs b/crates/engine/src/recovery.rs index 3b4060763..d6e434a4a 100644 --- a/crates/engine/src/recovery.rs +++ b/crates/engine/src/recovery.rs @@ -580,6 +580,12 @@ pub fn save(s: &Session, dir: &Path) -> Result { for d in &s.docs { // New Document's preview is not a document until Create makes it one. if !d.is_dirty() || s.new_document_preview() == Some(d.uid) { + // A background write this session queued of an older state must not publish after + // this discard, and what one already wrote in another folder is removed too. + #[cfg(not(target_arch = "wasm32"))] + for other in worker::cancel_document(d.uid, s.recovery_owner.0) { + discard(&other, d.uid); + } discard(dir, d.uid); continue; } @@ -762,6 +768,8 @@ 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); + // Set at the rename: an error after it (syncing the folder) leaves the entry on disk. + let landed = std::cell::Cell::new(false); let published = publish_recovery_written( &doc, entry.own, @@ -774,6 +782,7 @@ fn write_snapshot(entry: &Snapshot, cancelled: impl Fn() -> bool) -> Written { // and remove (Save and Close must clean up the snapshot they wrote). Recorded with the // rename, so a cleanup after it can remove the entry. || { + landed.set(true); if !entry.own { WRITTEN.lock().unwrap_or_else(std::sync::PoisonError::into_inner).insert((entry.key.clone(), entry.uid)); } @@ -785,6 +794,8 @@ fn write_snapshot(entry: &Snapshot, cancelled: impl Fn() -> bool) -> Written { // reports an error, which is the cancel's and not a failure. Ok(false) => return Written { published: false, error: None }, Err(_) if cancelled() => return Written { published: false, error: None }, + // Renamed into place, then the folder sync failed: the entry is there to track and clean up. + Err(e) if landed.get() => return Written { published: true, error: Some(e) }, Err(e) => return Written { published: false, error: Some(e) }, } let metadata = json!({"path": entry.path, "title": entry.title, "saved": designcraft_doc::vars::now()}); @@ -1048,10 +1059,15 @@ mod worker { let mut dirs = Vec::new(); if let Ok(mut running) = self.running.lock() { let mut keys = Vec::new(); - for (key, q) in running.iter_mut().filter(|((_, u), q)| *u == uid && q.owner == owner) { - keys.push(key.clone()); - dirs.push(q.dir.clone()); - q.next = None; + for (key, q) in running.iter_mut().filter(|((_, u), _)| *u == uid) { + // A write another session queued to follow stays queued. + if q.next.as_ref().is_some_and(|n| n.owner == owner) { + q.next = None; + } + if q.owner == owner { + keys.push(key.clone()); + dirs.push(q.dir.clone()); + } } if let Ok(mut cancelled) = self.cancelled.lock() { cancelled.extend(keys); @@ -1315,6 +1331,23 @@ pub mod testing { w.wait_until_idle(); } } + + static FAIL_SYNC: Mutex> = Mutex::new(None); + + /// Make the next folder sync after a recovery file is renamed into `dir` fail. + pub fn fail_next_sync(dir: &Path) { + *FAIL_SYNC.lock().unwrap_or_else(PoisonError::into_inner) = Some(dir.to_path_buf()); + } + + /// Whether this sync of `dir` is the one [`fail_next_sync`] asked to fail. + pub(super) fn sync_fails(dir: &Path) -> bool { + let mut slot = FAIL_SYNC.lock().unwrap_or_else(PoisonError::into_inner); + if slot.as_deref() == Some(dir) { + *slot = None; + return true; + } + false + } } #[cfg(not(target_arch = "wasm32"))] @@ -1460,6 +1493,10 @@ fn is_link_like(metadata: &std::fs::Metadata) -> bool { #[cfg(not(target_arch = "wasm32"))] fn sync_recovery_dir(dir: &Path) -> Result<()> { validate_recovery_dir(dir)?; + #[cfg(any(test, feature = "testing"))] + if testing::sync_fails(dir) { + return Err(EngineError::Other(format!("{}: sync failed", dir.display()))); + } #[cfg(unix)] { diff --git a/crates/engine/src/recovery/tests.rs b/crates/engine/src/recovery/tests.rs index 4534c3c37..d58f31fd6 100644 --- a/crates/engine/src/recovery/tests.rs +++ b/crates/engine/src/recovery/tests.rs @@ -466,6 +466,121 @@ fn a_snapshot_whose_metadata_failed_is_still_the_documents_to_remove() { assert!(s.docs[0].recovery_entries.is_empty()); } +#[cfg(not(target_arch = "wasm32"))] +#[test] +fn a_snapshot_whose_folder_sync_failed_is_still_the_documents_to_remove() { + // The rename put the snapshot in place before the folder sync failed: it is tracked, so Save + // removes it. + let root = tempfile::tempdir().unwrap(); + let dir = run_dir(root.path()).unwrap(); + let mut s = dirty_session(); + s.recovery_dir = Some(dir.clone()); + let uid = s.docs[0].uid; + let (document, _) = files(&dir, uid); + testing::fail_next_sync(&dir); + let err = save_tracked(&mut s, &dir).unwrap_err(); + assert!(err.to_string().contains("sync failed"), "{err}"); + assert!(document.exists(), "the snapshot landed"); + assert!(s.docs[0].recovery_entries.iter().any(|(d, u)| *u == uid && same_directory(d, &dir)), "and is tracked"); + let saved = root.path().join("saved.designcraft"); + s.execute("file.saveAs", &json!({"path": saved.to_string_lossy()})).unwrap(); + assert!(!document.exists(), "Save removed the snapshot"); +} + +#[cfg(not(target_arch = "wasm32"))] +#[test] +fn a_document_undone_to_its_saved_state_while_its_write_is_held_leaves_no_snapshot() { + 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 held = testing::hold_next_write(dir.path()); + s.execute("file.recovery.save", &json!({"background": true})).unwrap(); + held.wait_until_held(); + s.execute("edit.undo", &json!({})).unwrap(); + assert!(!s.docs[0].is_dirty(), "back to the state it had"); + // A save of recovery data now drops the clean document's checkpoint, and the held write of + // the older state must not publish after it. + save(&s, dir.path()).unwrap(); + held.release(); + testing::wait_until_idle(); + assert!(!files(dir.path(), uid).0.exists(), "the older snapshot was not published"); + let _ = s.execute("file.recovery.progress", &json!({})); + assert!(list(dir.path()).is_empty()); +} + +#[cfg(not(target_arch = "wasm32"))] +#[test] +fn a_document_undone_to_its_saved_state_loses_the_snapshot_a_finished_write_left_in_another_folder() { + let _turn = background(); + let dir = tempfile::tempdir().unwrap(); + let other = tempfile::tempdir().unwrap(); + let mut s = dirty_session(); + s.recovery_dir = Some(other.path().to_path_buf()); + let uid = s.docs[0].uid; + s.execute("file.recovery.save", &json!({"background": true})).unwrap(); + // Written, but its result not yet taken by the session. + testing::wait_until_idle(); + assert!(files(other.path(), uid).0.exists()); + s.execute("edit.undo", &json!({})).unwrap(); + save(&s, dir.path()).unwrap(); + assert!(!files(other.path(), uid).0.exists(), "the snapshot of the older state is removed"); + let _ = s.execute("file.recovery.progress", &json!({})); +} + +#[cfg(not(target_arch = "wasm32"))] +#[test] +fn a_clean_document_leaves_another_sessions_write_queued_behind_its_own() { + let _turn = background(); + let dir = tempfile::tempdir().unwrap(); + let mut a = dirty_session(); + a.recovery_dir = Some(dir.path().to_path_buf()); + let uid = a.docs[0].uid; + let mut b = dirty_session(); + b.recovery_dir = Some(dir.path().to_path_buf()); + b.docs[0].uid = uid; + b.execute("frame.create", &json!({"rect": [20, 20, 60, 60]})).unwrap(); + let held = testing::hold_next_write(dir.path()); + a.execute("file.recovery.save", &json!({"background": true})).unwrap(); + held.wait_until_held(); + // A write now cancels the held one; the other session's write is queued to follow it. + a.execute("file.recovery.save", &json!({"dir": dir.path().to_string_lossy()})).unwrap(); + b.execute("file.recovery.save", &json!({"background": true})).unwrap(); + a.execute("edit.undo", &json!({})).unwrap(); + assert!(!a.docs[0].is_dirty()); + save(&a, dir.path()).unwrap(); + held.release(); + assert_eq!(finished_writes(&mut b), 1, "the other session's write ran"); + let mut reopened = Session::new(); + assert_eq!(open(&mut reopened, dir.path()).unwrap().len(), 1); + assert_eq!(reopened.doc().unwrap().doc.spreads[0].items.len(), 2, "its snapshot was kept"); + discard(dir.path(), uid); +} + +#[cfg(not(target_arch = "wasm32"))] +#[test] +fn a_clean_document_in_another_session_leaves_this_sessions_write_alone() { + let _turn = background(); + let dir = tempfile::tempdir().unwrap(); + let mut queued_by = dirty_session(); + queued_by.recovery_dir = Some(dir.path().to_path_buf()); + let uid = queued_by.docs[0].uid; + // The same uid, clean, in another session. + let mut other = Session::new(); + other.execute("file.new", &json!({})).unwrap(); + other.docs[0].uid = uid; + assert!(!other.docs[0].is_dirty()); + let held = testing::hold_next_write(dir.path()); + queued_by.execute("file.recovery.save", &json!({"background": true})).unwrap(); + held.wait_until_held(); + save(&other, dir.path()).unwrap(); + held.release(); + assert_eq!(finished_writes(&mut queued_by), 1); + assert!(files(dir.path(), uid).0.exists(), "the other session's save did not cancel this write"); + discard(dir.path(), uid); +} + #[cfg(not(target_arch = "wasm32"))] #[test] fn a_background_snapshot_whose_metadata_failed_is_still_the_documents_to_remove() { From c4b2d730627ac1e8e1bc8b1f8c14411a66afa663 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Wed, 11 Nov 2026 23:02:32 -0800 Subject: [PATCH 10/14] Recovery: a shared folder's entry is removed or replaced only by the session that wrote it Entries written or recovered in a recovery folder that is not the run's own were tracked per folder and uid only, so another session in the same process with a document of that uid could replace or remove them; a recovered entry keeps the uid it was written under. Each entry now carries the session that wrote or recovered it, and only that session may replace or remove it. A write queued while its entry was the session's is recorded again when it lands, so a cleanup that dropped the record in between no longer leaves the snapshot on disk after Save. --- crates/engine/src/recovery.rs | 77 ++++++++++++++++++----------- crates/engine/src/recovery/tests.rs | 59 +++++++++++++++++++++- 2 files changed, 104 insertions(+), 32 deletions(-) diff --git a/crates/engine/src/recovery.rs b/crates/engine/src/recovery.rs index d6e434a4a..f7f4dcda2 100644 --- a/crates/engine/src/recovery.rs +++ b/crates/engine/src/recovery.rs @@ -272,30 +272,39 @@ fn relock(dir: &Path, created: bool) -> Result { } /// Entries this process wrote or recovered in folders that are not its run's own: (canonical -/// folder, uid). Document uids start at 1 in every process, so a shared folder's entry under -/// one of this run's uids may be another run's. -static WRITTEN: std::sync::Mutex> = std::sync::Mutex::new(std::collections::BTreeSet::new()); +/// folder, uid), with the session that wrote or recovered each ([`Session::recovery_owner`]). +/// Document uids start at 1 in every process, so a shared folder's entry under one of this run's +/// uids may be another run's; and a recovered entry keeps the uid it was written under, which a +/// document of another session can have. +static WRITTEN: std::sync::Mutex> = std::sync::Mutex::new(std::collections::BTreeMap::new()); -/// What this run may do in a recovery folder. +/// What a session may do in a recovery folder. struct Access { /// This run's own folder (`run_dir`): every entry in it is this run's. own: bool, key: PathBuf, + /// The session asking ([`Session::recovery_owner`]). + owner: u64, } impl Access { fn owns(&self, uid: u64) -> bool { - self.own || WRITTEN.lock().unwrap_or_else(std::sync::PoisonError::into_inner).contains(&(self.key.clone(), uid)) + self.own || WRITTEN.lock().unwrap_or_else(std::sync::PoisonError::into_inner).get(&(self.key.clone(), uid)) == Some(&self.owner) } fn record(&self, uid: u64) { if !self.own { - WRITTEN.lock().unwrap_or_else(std::sync::PoisonError::into_inner).insert((self.key.clone(), uid)); + WRITTEN.lock().unwrap_or_else(std::sync::PoisonError::into_inner).insert((self.key.clone(), uid), self.owner); } } + /// Drop the record of an entry this session wrote or recovered; another session's stays. fn forget(&self, uid: u64) { - WRITTEN.lock().unwrap_or_else(std::sync::PoisonError::into_inner).remove(&(self.key.clone(), uid)); + let mut written = WRITTEN.lock().unwrap_or_else(std::sync::PoisonError::into_inner); + let key = (self.key.clone(), uid); + if written.get(&key) == Some(&self.owner) { + written.remove(&key); + } } /// Refuse an entry of another run before anything is written: this run never replaces it. @@ -319,9 +328,9 @@ impl Access { } } -/// Check what this run may do in `dir`. A folder that is not this run's own may be the folder -/// of another running app, whose lock that app holds: this run leaves it alone. -fn access(dir: &Path, created: bool) -> Result { +/// Check what session `owner` may do in `dir`. A folder that is not this run's own may be the +/// folder of another running app, whose lock that app holds: this run leaves it alone. +fn access(dir: &Path, created: bool, owner: u64) -> Result { #[cfg(not(target_arch = "wasm32"))] let own = relock(dir, created)?; #[cfg(not(target_arch = "wasm32"))] @@ -333,7 +342,7 @@ fn access(dir: &Path, created: bool) -> Result { let _ = created; false }; - Ok(Access { own, key: directory_key(dir)? }) + Ok(Access { own, key: directory_key(dir)?, owner }) } #[cfg(not(target_arch = "wasm32"))] @@ -485,13 +494,13 @@ fn read_entry_file(path: &Path) -> std::io::Result> { } /// Remove a document's recovery entry (it was saved or closed). In a folder that is not this -/// run's own, only an entry this run wrote or recovered is removed. -pub fn discard(dir: &Path, uid: u64) { +/// run's own, only an entry session `owner` wrote or recovered is removed. +pub fn discard(dir: &Path, uid: u64, owner: u64) { #[cfg(not(target_arch = "wasm32"))] if validate_recovery_dir(dir).is_err() { return; } - let Ok(access) = access(dir, false) else { return }; + let Ok(access) = access(dir, false, owner) else { return }; if !access.owns(uid) { return; } @@ -504,6 +513,7 @@ pub fn discard(dir: &Path, uid: u64) { /// Consume this document's checkpoint provenance after Save/Close/Revert. A default-directory /// uid may also be another live recovered document's retained source after a failed republish. pub(crate) fn discard_document(s: &mut Session, index: usize, default_dir: Option<&Path>) { + let owner = s.recovery_owner.0; let Some(doc) = s.docs.get_mut(index) else { return }; let uid = doc.uid; // A completed Save/Close/Revert consumes the provenance, even when removal fails. A stale @@ -518,7 +528,7 @@ pub(crate) fn discard_document(s: &mut Session, index: usize, default_dir: Optio // `file.recovery.save {dir}` is not in its entries until the write's result is taken): none // may publish after this point, and what one already published is removed below. #[cfg(not(target_arch = "wasm32"))] - for dir in worker::cancel_document(uid, s.recovery_owner.0) { + for dir in worker::cancel_document(uid, owner) { if !entries.iter().any(|(entry_dir, entry_uid)| *entry_uid == uid && same_directory(entry_dir, &dir)) { entries.push((dir, uid)); } @@ -532,7 +542,7 @@ pub(crate) fn discard_document(s: &mut Session, index: usize, default_dir: Optio // A write of it still running must not publish its snapshot after this point. #[cfg(not(target_arch = "wasm32"))] worker::cancel(&dir, uid); - discard(&dir, uid); + discard(&dir, uid, owner); } } } @@ -563,7 +573,7 @@ fn prepare_save(s: &Session, dir: &Path) -> Result { // coincides with an unrelated live document in a manually configured recovery directory. protect_retained_sources(s, dir)?; let created = secure_recovery_dir(dir)?; - let access = access(dir, created)?; + let access = access(dir, created, s.recovery_owner.0)?; // New Document's preview is never written. let preview = s.new_document_preview(); for document in s.docs.iter().filter(|d| d.is_dirty() && preview != Some(d.uid)) { @@ -584,9 +594,9 @@ pub fn save(s: &Session, dir: &Path) -> Result { // this discard, and what one already wrote in another folder is removed too. #[cfg(not(target_arch = "wasm32"))] for other in worker::cancel_document(d.uid, s.recovery_owner.0) { - discard(&other, d.uid); + discard(&other, d.uid, s.recovery_owner.0); } - discard(dir, d.uid); + discard(dir, d.uid, s.recovery_owner.0); continue; } // The caller asked for this write now: nothing has cancelled it. @@ -676,7 +686,9 @@ struct Snapshot { title: String, /// Whether this run may replace an entry already in the folder. own: bool, - /// The folder's canonical key, for recording that a shared entry is this run's now. + /// The folder is not this run's own: the entry, once written, is recorded as the session's. + shared: bool, + /// The folder's canonical key, for recording that a shared entry is this session's now. key: PathBuf, /// The session that queued a background write ([`Session::recovery_owner`]), which takes its /// result. @@ -716,8 +728,9 @@ impl Snapshot { title: d.title(), // An entry this run did not write is never replaced, even one made since it checked. own: access.owns(d.uid), + shared: !access.own, key: access.key.clone(), - owner: 0, + owner: access.owner, } } } @@ -778,13 +791,15 @@ fn write_snapshot(entry: &Snapshot, cancelled: impl Fn() -> bool) -> Written { designcraft_format::write(&mut StopWhen { out, cancelled: &cancelled }, &entry.doc, None).map_err(|e| EngineError::Other(e.to_string())) }, &cancelled, - // Written: in a folder that is not this run's own, the entry is now this run's to replace - // and remove (Save and Close must clean up the snapshot they wrote). Recorded with the - // rename, so a cleanup after it can remove the entry. + // Written: in a folder that is not this run's own, the entry is now this session's to + // replace and remove (Save and Close must clean up the snapshot they wrote). Recorded with + // the rename, so a cleanup after it can remove the entry, and recorded even when the + // entry was this session's as the write was queued: a cleanup since may have dropped that + // record. || { landed.set(true); - if !entry.own { - WRITTEN.lock().unwrap_or_else(std::sync::PoisonError::into_inner).insert((entry.key.clone(), entry.uid)); + if entry.shared { + WRITTEN.lock().unwrap_or_else(std::sync::PoisonError::into_inner).insert((entry.key.clone(), entry.uid), entry.owner); } }, ); @@ -867,6 +882,7 @@ pub(crate) fn start_tracked(s: &mut Session, dir: &Path) -> Result { /// whether a checkpoint was recorded. #[cfg(not(target_arch = "wasm32"))] fn record_checkpoint(s: &mut Session, dir: PathBuf, uid: u64) -> bool { + let owner = s.recovery_owner.0; match s.docs.iter_mut().find(|doc| doc.uid == uid) { Some(doc) if doc.is_dirty() => { if !has_checkpoint(doc, &dir) { @@ -878,7 +894,7 @@ fn record_checkpoint(s: &mut Session, dir: PathBuf, uid: u64) -> bool { // crash, and the timer writes only when a document is dirty, so nothing else would // remove it. Some(_) | None => { - discard(&dir, uid); + discard(&dir, uid, owner); false } } @@ -1604,10 +1620,11 @@ pub fn open(s: &mut Session, dir: &Path) -> Result> { fn open_with_publisher(s: &mut Session, dir: &Path, mut publish: impl FnMut(&Path, &[u8]) -> Result<()>) -> Result> { #[cfg(not(target_arch = "wasm32"))] validate_recovery_dir(dir)?; - let source = access(dir, false)?; + let owner = s.recovery_owner.0; + let source = access(dir, false, owner)?; let destination = s.recovery_dir.clone().unwrap_or_else(|| dir.to_path_buf()); let created = secure_recovery_dir(&destination)?; - let target = access(&destination, created)?; + let target = access(&destination, created, owner)?; let source_key = source.key.clone(); let destination_key = target.key.clone(); let mut opened = Vec::new(); @@ -1665,7 +1682,7 @@ fn open_with_publisher(s: &mut Session, dir: &Path, mut publish: impl FnMut(&Pat let meta = json!({"path": state.path, "title": state.title(), "saved": designcraft_doc::vars::now()}); match publish(&metadata, meta.to_string().as_bytes()) { Ok(()) => { - discard(dir, uid); + discard(dir, uid, owner); state.recovery_entries = vec![(destination.clone(), state.uid)]; } Err(e) => log::warn!("recovery entry {uid} kept: {e}"), diff --git a/crates/engine/src/recovery/tests.rs b/crates/engine/src/recovery/tests.rs index d58f31fd6..1a4881a1d 100644 --- a/crates/engine/src/recovery/tests.rs +++ b/crates/engine/src/recovery/tests.rs @@ -1,5 +1,12 @@ use super::*; +/// [`super::discard`] as the session that wrote or recovered the entry, if one did. +fn discard(dir: &Path, uid: u64) { + let key = directory_key(dir).ok().map(|key| (key, uid)); + let owner = key.and_then(|key| WRITTEN.lock().unwrap_or_else(std::sync::PoisonError::into_inner).get(&key).copied()); + super::discard(dir, uid, owner.unwrap_or(0)); +} + fn dirty_session() -> Session { let mut session = Session::new(); session.execute("file.new", &json!({})).unwrap(); @@ -440,6 +447,51 @@ fn a_snapshot_that_finishes_after_the_save_is_dropped() { assert!(s.docs[0].recovery_entries.iter().any(|(d, u)| *u == uid && d == dir.path())); } +#[test] +fn a_shared_folders_entry_is_only_its_writing_sessions() { + // A recovered entry keeps the uid it was written under, which a document of another session + // in the same process can have. + let dir = tempfile::tempdir().unwrap(); + let writer = dirty_session(); + let uid = writer.docs[0].uid; + assert_eq!(save(&writer, dir.path()).unwrap(), 1); + let (file, _) = files(dir.path(), uid); + let snapshot = std::fs::read(&file).unwrap(); + // Another session's clean document with that uid: its discard leaves the entry alone. + let mut clean = Session::new(); + clean.execute("file.new", &json!({})).unwrap(); + clean.docs[0].uid = uid; + assert_eq!(save(&clean, dir.path()).unwrap(), 0); + assert!(file.exists(), "another session's discard removed the entry"); + // Another session's edited document with that uid: it does not write over the entry. + let mut edited = dirty_session(); + edited.docs[0].uid = uid; + assert!(save(&edited, dir.path()).is_err()); + assert_eq!(std::fs::read(&file).unwrap(), snapshot, "another session wrote over the entry"); + super::discard(dir.path(), uid, writer.recovery_owner.0); + assert!(!file.exists(), "the writing session removes its entry"); +} + +#[test] +fn a_snapshot_queued_as_the_sessions_own_is_recorded_when_it_lands() { + // A write taken while the shared folder's entry was this session's, published after a cleanup + // dropped that record: Save and Close must still remove what it put there. + let dir = tempfile::tempdir().unwrap(); + let s = dirty_session(); + let uid = s.docs[0].uid; + let owner = s.recovery_owner.0; + assert_eq!(save(&s, dir.path()).unwrap(), 1); + let entry = Snapshot::of(&prepare_save(&s, dir.path()).unwrap(), dir.path(), &s.docs[0]); + assert!(entry.own); + super::discard(dir.path(), uid, owner); + let (file, _) = files(dir.path(), uid); + assert!(!file.exists()); + let written = write_snapshot(&entry, || false); + assert!(written.published && written.error.is_none(), "the snapshot landed"); + super::discard(dir.path(), uid, owner); + assert!(!file.exists(), "the snapshot it wrote is the session's to remove"); +} + #[cfg(not(target_arch = "wasm32"))] #[test] fn a_snapshot_whose_metadata_failed_is_still_the_documents_to_remove() { @@ -544,12 +596,15 @@ fn a_clean_document_leaves_another_sessions_write_queued_behind_its_own() { let held = testing::hold_next_write(dir.path()); a.execute("file.recovery.save", &json!({"background": true})).unwrap(); held.wait_until_held(); - // A write now cancels the held one; the other session's write is queued to follow it. + // A write now cancels the held one. The other session does not write over the entry it left. a.execute("file.recovery.save", &json!({"dir": dir.path().to_string_lossy()})).unwrap(); - b.execute("file.recovery.save", &json!({"background": true})).unwrap(); + let err = b.execute("file.recovery.save", &json!({"background": true})).unwrap_err(); + assert!(err.to_string().contains("did not write"), "{err}"); a.execute("edit.undo", &json!({})).unwrap(); assert!(!a.docs[0].is_dirty()); save(&a, dir.path()).unwrap(); + // Once that entry is gone, the other session's write is queued to follow the held one. + b.execute("file.recovery.save", &json!({"background": true})).unwrap(); held.release(); assert_eq!(finished_writes(&mut b), 1, "the other session's write ran"); let mut reopened = Session::new(); From b9cba43f89cdaa574a1a1b40248bb3b67225eaa9 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Wed, 11 Nov 2026 23:09:49 -0800 Subject: [PATCH 11/14] Recovery: closing or saving a document cancels only its own session's background writes Discarding a document's recovery entries cancelled every background write of that uid in the folder, so another session's write of a document with the same uid was dropped without a replacement. The cancel now covers only writes the session itself queued. --- crates/engine/src/recovery.rs | 32 ++++++++++++++++++++--------- crates/engine/src/recovery/tests.rs | 24 ++++++++++++++++++++++ 2 files changed, 46 insertions(+), 10 deletions(-) diff --git a/crates/engine/src/recovery.rs b/crates/engine/src/recovery.rs index f7f4dcda2..2a0422417 100644 --- a/crates/engine/src/recovery.rs +++ b/crates/engine/src/recovery.rs @@ -539,9 +539,10 @@ pub(crate) fn discard_document(s: &mut Session, index: usize, default_dir: Optio if !s.docs.iter().enumerate().any(|(other_index, owner)| { other_index != index && owner.recovery_entries.iter().any(|(entry_dir, entry_uid)| *entry_uid == uid && same_directory(entry_dir, &dir)) }) { - // A write of it still running must not publish its snapshot after this point. + // A write of it this session still runs must not publish its snapshot after this + // point; another session's write of the same uid is its own. #[cfg(not(target_arch = "wasm32"))] - worker::cancel(&dir, uid); + worker::cancel_owned(&dir, uid, owner); discard(&dir, uid, owner); } } @@ -1041,22 +1042,26 @@ mod worker { /// Do not publish the snapshot being written for `dir`/`uid`: the document was saved, /// reverted or closed. Only a write that is queued or running is marked, so the set stays - /// as small as the work in flight. - fn cancel(&self, dir: &Path, uid: u64) { + /// as small as the work in flight. With `owner`, only writes that session queued are + /// marked: another session's write of the same uid stays. + fn cancel(&self, dir: &Path, uid: u64, owner: Option) { let canonical = key(dir, uid); + let mine = |queued_by: u64| owner.is_none_or(|owner| owner == queued_by); // A rename under way finishes first, so the caller's cleanup comes after it; a later // one sees the mark. let _publishing = super::PUBLISH.lock().unwrap_or_else(std::sync::PoisonError::into_inner); // The folder's canonical path, or its name as given: a link in the path may lead // elsewhere now than when the write was queued. let Ok(mut running) = self.running.lock() else { return }; - let keys: Vec<(PathBuf, u64)> = - running.iter().filter(|(k, q)| **k == canonical || (k.1 == uid && q.dir == dir)).map(|(k, _)| k.clone()).collect(); - // A write queued to follow it is older than what the caller has now, too. - for k in &keys { - if let Some(q) = running.get_mut(k) { + let mut keys = Vec::new(); + for (k, q) in running.iter_mut().filter(|(k, q)| **k == canonical || (k.1 == uid && q.dir == dir)) { + // A write queued to follow it is older than what the caller has now, too. + if q.next.as_ref().is_some_and(|n| mine(n.owner)) { q.next = None; } + if mine(q.owner) { + keys.push(k.clone()); + } } if let Ok(mut cancelled) = self.cancelled.lock() { cancelled.extend(keys); @@ -1181,7 +1186,14 @@ mod worker { /// Do not publish the snapshot being written for `dir`/`uid` (see [`Writer::cancel`]). pub(super) fn cancel(dir: &Path, uid: u64) { if let Some(w) = started() { - w.cancel(dir, uid); + w.cancel(dir, uid, None); + } + } + + /// [`cancel`] for the writes session `owner` queued. + pub(super) fn cancel_owned(dir: &Path, uid: u64, owner: u64) { + if let Some(w) = started() { + w.cancel(dir, uid, Some(owner)); } } diff --git a/crates/engine/src/recovery/tests.rs b/crates/engine/src/recovery/tests.rs index 1a4881a1d..d1d03f162 100644 --- a/crates/engine/src/recovery/tests.rs +++ b/crates/engine/src/recovery/tests.rs @@ -312,6 +312,30 @@ fn saving_in_one_session_leaves_another_sessions_write_alone() { discard(dir.path(), uid); } +#[cfg(not(target_arch = "wasm32"))] +#[test] +fn saving_in_one_session_leaves_another_sessions_write_to_the_same_folder_alone() { + let _turn = background(); + let dir = tempfile::tempdir().unwrap(); + let mut queued_by = dirty_session(); + queued_by.recovery_dir = Some(dir.path().to_path_buf()); + let uid = queued_by.docs[0].uid; + // The same uid and recovery folder in another session. + let mut other = dirty_session(); + other.recovery_dir = Some(dir.path().to_path_buf()); + other.docs[0].uid = uid; + let held = testing::hold_next_write(dir.path()); + queued_by.execute("file.recovery.save", &json!({"background": true})).unwrap(); + held.wait_until_held(); + let elsewhere = tempfile::tempdir().unwrap(); + other.execute("file.saveAs", &json!({"path": elsewhere.path().join("other.designcraft").to_string_lossy()})).unwrap(); + assert!(!worker::cancelled(dir.path(), uid), "the other session's save cancelled this write"); + held.release(); + assert_eq!(finished_writes(&mut queued_by), 1); + assert!(files(dir.path(), uid).0.exists(), "this session's snapshot was kept"); + discard(dir.path(), uid); +} + #[cfg(not(target_arch = "wasm32"))] #[test] fn a_write_now_cancels_a_background_write_to_the_same_folder_spelled_another_way() { From 8b7fea42fba13445b8f96d9693172f5b8a64bb6d Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Wed, 11 Nov 2026 23:39:39 -0800 Subject: [PATCH 12/14] Recovery: a write now leaves another session's background write of the same uid alone `recovery::save` and the tracked save cancelled every background write of the document's folder and uid, including one another session in the same process had queued. Both now cancel only their own session's writes, refuse while another session's write of that uid is queued or running, and keep other sessions' background writes of it from queuing until the write now is done. --- crates/engine/src/recovery.rs | 122 +++++++++++++++++++++++++--- crates/engine/src/recovery/tests.rs | 52 +++++++++++- 2 files changed, 161 insertions(+), 13 deletions(-) diff --git a/crates/engine/src/recovery.rs b/crates/engine/src/recovery.rs index 2a0422417..4f774b8d3 100644 --- a/crates/engine/src/recovery.rs +++ b/crates/engine/src/recovery.rs @@ -583,10 +583,33 @@ fn prepare_save(s: &Session, dir: &Path) -> Result { Ok(access) } +/// Refuse a write now while another session's background write of one of the dirty documents' +/// uids to `dir` is queued or running. A write now cancels only this session's writes. +#[cfg(not(target_arch = "wasm32"))] +fn check_no_other_write(s: &Session, dir: &Path) -> Result<()> { + let preview = s.new_document_preview(); + for document in s.docs.iter().filter(|d| d.is_dirty() && preview != Some(d.uid)) { + if worker::queued_by_other(dir, document.uid, s.recovery_owner.0) { + return Err(still_being_written(dir)); + } + } + Ok(()) +} + +#[cfg(not(target_arch = "wasm32"))] +fn still_being_written(dir: &Path) -> EngineError { + EngineError::Other(format!( + "{}: another document's recovery data is still being written here; try again or choose another recovery folder", + dir.display() + )) +} + /// Write recovery entries for dirty documents and drop clean checkpoints. Returns how many /// were written. Command callers use the tracked variant without configuring automatic recovery. pub fn save(s: &Session, dir: &Path) -> Result { let access = prepare_save(s, dir)?; + #[cfg(not(target_arch = "wasm32"))] + check_no_other_write(s, dir)?; let mut n = 0; for d in &s.docs { // New Document's preview is not a document until Create makes it one. @@ -601,9 +624,10 @@ pub fn save(s: &Session, dir: &Path) -> Result { continue; } // The caller asked for this write now: nothing has cancelled it. - // A background write of an older state must not land over this one. + // A background write of an older state this session queued must not land over this one, + // and no other session's write of the uid is queued until it is done. #[cfg(not(target_arch = "wasm32"))] - worker::cancel(dir, d.uid); + let _now = worker::write_now(dir, d.uid, s.recovery_owner.0).ok_or_else(|| still_being_written(dir))?; if let Some(e) = write_snapshot(&Snapshot::of(&access, dir, d), || false).error { return Err(e); } @@ -617,6 +641,8 @@ pub fn save(s: &Session, dir: &Path) -> Result { pub(crate) fn save_tracked(s: &mut Session, dir: &Path) -> Result { reject_overfull_checkpoints(s, dir)?; let access = prepare_save(s, dir)?; + #[cfg(not(target_arch = "wasm32"))] + check_no_other_write(s, dir)?; let mut n = 0; let preview = s.new_document_preview(); for index in 0..s.docs.len() { @@ -629,9 +655,10 @@ pub(crate) fn save_tracked(s: &mut Session, dir: &Path) -> Result { continue; } let Some(document) = s.docs.get(index) else { continue }; - // A background write of an older state must not land over this one. + // A background write of an older state this session queued must not land over this one, + // and no other session's write of the uid is queued until it is done. #[cfg(not(target_arch = "wasm32"))] - worker::cancel(dir, document.uid); + let _now = worker::write_now(dir, document.uid, s.recovery_owner.0).ok_or_else(|| still_being_written(dir))?; let entry = Snapshot::of(&access, dir, document); let written = write_snapshot(&entry, || false); // The primary is the recovery data; its metadata is optional. Track the primary before @@ -980,6 +1007,9 @@ 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>, + /// Documents a session is writing now on its own thread, with that session: no background + /// write of one is queued until it is done (see [`write_now`]). + writing_now: Mutex>, /// Notified each time a write finishes and leaves `running`. idle: Condvar, } @@ -1131,6 +1161,48 @@ mod worker { dirs } + /// Whether a session other than `owner` has a write of `dir`/`uid` queued or running that + /// is not cancelled, or queued to follow one. + fn queued_by_other(&self, dir: &Path, uid: u64, owner: u64) -> bool { + let running = self.running.lock().unwrap_or_else(std::sync::PoisonError::into_inner); + self.other_in_flight(&running, dir, uid, owner) + } + + /// [`Writer::queued_by_other`] with `running` held, counting writes now as well. + fn other_in_flight(&self, running: &BTreeMap<(PathBuf, u64), Queued>, dir: &Path, uid: u64, owner: u64) -> bool { + let canonical = key(dir, uid); + running + .iter() + .filter(|(k, q)| **k == canonical || (k.1 == uid && q.dir == dir)) + .any(|(k, q)| (q.owner != owner && !self.cancelled(k)) || q.next.as_ref().is_some_and(|n| n.owner != owner)) + || self.writing_now.lock().is_ok_and(|now| now.get(&canonical).is_some_and(|by| *by != owner)) + } + + /// Start a write now of `dir`/`uid` by session `owner`. Returns `None` while another + /// session's write of it is in flight. Otherwise cancels this session's writes of it (as + /// [`Writer::cancel`]) and records the write now under the returned key, all under one + /// hold of `running`. + fn write_now(&self, dir: &Path, uid: u64, owner: u64) -> Option<(PathBuf, u64)> { + let canonical = key(dir, uid); + let _publishing = super::PUBLISH.lock().unwrap_or_else(std::sync::PoisonError::into_inner); + let mut running = self.running.lock().unwrap_or_else(std::sync::PoisonError::into_inner); + if self.other_in_flight(&running, dir, uid, owner) { + return None; + } + let mut keys = Vec::new(); + for (k, q) in running.iter_mut().filter(|(k, q)| **k == canonical || (k.1 == uid && q.dir == dir)) { + q.next = None; + keys.push(k.clone()); + } + if let Ok(mut cancelled) = self.cancelled.lock() { + cancelled.extend(keys); + } + if let Ok(mut now) = self.writing_now.lock() { + now.insert(canonical.clone(), owner); + } + Some(canonical) + } + /// `key` as in [`Writer::finish`]. fn cancelled(&self, key: &(PathBuf, u64)) -> bool { self.cancelled.lock().is_ok_and(|cancelled| cancelled.contains(key)) @@ -1172,6 +1244,7 @@ mod worker { undelivered: Mutex::new(Vec::new()), running: Mutex::new(BTreeMap::new()), cancelled: Mutex::new(BTreeSet::new()), + writing_now: Mutex::new(BTreeMap::new()), idle: Condvar::new(), }) }) @@ -1183,20 +1256,41 @@ mod worker { WRITER.get().and_then(Option::as_ref) } - /// Do not publish the snapshot being written for `dir`/`uid` (see [`Writer::cancel`]). - pub(super) fn cancel(dir: &Path, uid: u64) { - if let Some(w) = started() { - w.cancel(dir, uid, None); - } - } - - /// [`cancel`] for the writes session `owner` queued. + /// Do not publish the snapshot session `owner` is having written for `dir`/`uid` (see + /// [`Writer::cancel`]). pub(super) fn cancel_owned(dir: &Path, uid: u64, owner: u64) { if let Some(w) = started() { w.cancel(dir, uid, Some(owner)); } } + /// A write now in progress; dropping it lets background writes of its document queue again. + pub(super) struct WriteNow(Option<(PathBuf, u64)>); + + impl Drop for WriteNow { + fn drop(&mut self) { + if let (Some(key), Some(w)) = (self.0.take(), started()) + && let Ok(mut now) = w.writing_now.lock() + { + now.remove(&key); + } + } + } + + /// See [`Writer::write_now`]. `None` while another session's write of `dir`/`uid` is in + /// flight. Without a writer thread there are no background writes to wait for. + pub(super) fn write_now(dir: &Path, uid: u64, owner: u64) -> Option { + match writer() { + Some(w) => w.write_now(dir, uid, owner).map(|key| WriteNow(Some(key))), + None => Some(WriteNow(None)), + } + } + + /// See [`Writer::queued_by_other`]. + pub(super) fn queued_by_other(dir: &Path, uid: u64, owner: u64) -> bool { + started().is_some_and(|w| w.queued_by_other(dir, uid, owner)) + } + /// See [`Writer::pending_folders`]. pub(super) fn pending_folders(uid: u64, owner: u64) -> Vec { started().map(|w| w.pending_folders(uid, owner)).unwrap_or_default() @@ -1238,6 +1332,10 @@ mod worker { let w = writer()?; let mut running = w.running.lock().ok()?; let key = (entry.key.clone(), entry.uid); + // A session is writing this document now: this write waits for a later turn. + if w.writing_now.lock().is_ok_and(|now| now.contains_key(&key)) { + return Some(false); + } if let Some(q) = running.get_mut(&key) { // The write in flight was cancelled (the document was saved since), so it holds // nothing to recover: this one follows it, in place of any that was waiting. diff --git a/crates/engine/src/recovery/tests.rs b/crates/engine/src/recovery/tests.rs index d1d03f162..a45cea1ce 100644 --- a/crates/engine/src/recovery/tests.rs +++ b/crates/engine/src/recovery/tests.rs @@ -336,6 +336,56 @@ fn saving_in_one_session_leaves_another_sessions_write_to_the_same_folder_alone( discard(dir.path(), uid); } +#[cfg(not(target_arch = "wasm32"))] +#[test] +fn a_write_now_in_one_session_leaves_another_sessions_write_of_the_same_uid_alone() { + let _turn = background(); + let dir = tempfile::tempdir().unwrap(); + let mut queued_by = dirty_session(); + queued_by.recovery_dir = Some(dir.path().to_path_buf()); + let uid = queued_by.docs[0].uid; + // The same uid in another session, with two objects instead of one. + let mut other = dirty_session(); + other.docs[0].uid = uid; + other.execute("frame.create", &json!({"rect": [20, 20, 60, 60]})).unwrap(); + let held = testing::hold_next_write(dir.path()); + queued_by.execute("file.recovery.save", &json!({"background": true})).unwrap(); + held.wait_until_held(); + let err = save(&other, dir.path()).unwrap_err(); + assert!(err.to_string().contains("still being written"), "{err}"); + let err = other.execute("file.recovery.save", &json!({"dir": dir.path().to_string_lossy()})).unwrap_err(); + assert!(err.to_string().contains("still being written"), "{err}"); + assert!(!worker::cancelled(dir.path(), uid), "the other session's write now cancelled this write"); + held.release(); + assert_eq!(finished_writes(&mut queued_by), 1); + let mut reopened = Session::new(); + assert_eq!(open(&mut reopened, dir.path()).unwrap().len(), 1); + assert_eq!(reopened.doc().unwrap().doc.spreads[0].items.len(), 1, "the queued session's snapshot was kept"); + discard(dir.path(), reopened.doc().unwrap().uid); + discard(dir.path(), uid); +} + +#[cfg(not(target_arch = "wasm32"))] +#[test] +fn no_other_sessions_write_of_the_uid_is_queued_during_a_write_now() { + let _turn = background(); + let dir = tempfile::tempdir().unwrap(); + let writing = dirty_session(); + let uid = writing.docs[0].uid; + let mut other = dirty_session(); + other.recovery_dir = Some(dir.path().to_path_buf()); + other.docs[0].uid = uid; + let now = worker::write_now(dir.path(), uid, writing.recovery_owner.0).expect("nothing else is in flight"); + let queued = other.execute("file.recovery.save", &json!({"background": true})).unwrap(); + assert_eq!(queued["queued"], 0, "another session's write was queued during the write now: {queued}"); + assert!(worker::write_now(dir.path(), uid, other.recovery_owner.0).is_none(), "a second write now of the uid started"); + drop(now); + let queued = other.execute("file.recovery.save", &json!({"background": true})).unwrap(); + assert_eq!(queued["queued"], 1, "{queued}"); + assert_eq!(finished_writes(&mut other), 1); + discard(dir.path(), uid); +} + #[cfg(not(target_arch = "wasm32"))] #[test] fn a_write_now_cancels_a_background_write_to_the_same_folder_spelled_another_way() { @@ -733,7 +783,7 @@ fn a_cancel_stops_a_write_before_the_package_is_finished() { fn only_a_queued_write_is_cancelled() { // A mark for a document with no write in flight would stay in the writer's set for ever. let dir = tempfile::tempdir().unwrap(); - worker::cancel(dir.path(), 7); + worker::cancel_owned(dir.path(), 7, 1); assert!(!worker::cancelled(dir.path(), 7), "nothing was queued for it"); } From 3efde54ae9354f02adf61de979697931a49d1d24 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Thu, 12 Nov 2026 00:08:50 -0800 Subject: [PATCH 13/14] Recovery: opening the recovery folder leaves this session's unfinished background writes alone A background write that is running, or that finished before the session took its result, is not in the document's entries yet. Opening recovery from that folder reopened it as a second copy of the live document. It is now skipped. The PUBLISH doc comment names the functions that take the lock. --- crates/engine/src/recovery.rs | 8 +++++++- crates/engine/src/recovery/tests.rs | 18 ++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/crates/engine/src/recovery.rs b/crates/engine/src/recovery.rs index 4f774b8d3..d045b2415 100644 --- a/crates/engine/src/recovery.rs +++ b/crates/engine/src/recovery.rs @@ -1532,7 +1532,7 @@ fn publish_recovery_file(path: &Path, bytes: &[u8], replace: bool) -> Result<()> } /// Serializes the last check for a cancelled write with the rename that publishes it, and with -/// [`worker::cancel`]. +/// [`worker::cancel_owned`] and [`worker::cancel_document`]. #[cfg(not(target_arch = "wasm32"))] static PUBLISH: std::sync::Mutex<()> = std::sync::Mutex::new(()); @@ -1755,6 +1755,12 @@ fn open_with_publisher(s: &mut Session, dir: &Path, mut publish: impl FnMut(&Pat }) { continue; } + // A background write of this session that is running, or finished with its result not + // taken yet, is a live document's checkpoint, not an entry to recover. + #[cfg(not(target_arch = "wasm32"))] + if worker::pending_folders(uid, owner).iter().any(|pending| directory_key(pending).is_ok_and(|key| key == source_key)) { + continue; + } #[cfg(not(target_arch = "wasm32"))] validate_recovery_dir(dir)?; let (doc, _) = files(dir, uid); diff --git a/crates/engine/src/recovery/tests.rs b/crates/engine/src/recovery/tests.rs index a45cea1ce..f66d28879 100644 --- a/crates/engine/src/recovery/tests.rs +++ b/crates/engine/src/recovery/tests.rs @@ -242,6 +242,24 @@ fn saving_removes_a_finished_write_to_a_named_folder_whose_result_was_not_taken( assert!(list(named.path()).is_empty()); } +#[cfg(not(target_arch = "wasm32"))] +#[test] +fn opening_recovery_leaves_a_finished_background_write_whose_result_was_not_taken() { + 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; + s.execute("file.recovery.save", &json!({"background": true})).unwrap(); + testing::wait_until_idle(); + assert!(files(dir.path(), uid).0.exists(), "the write finished"); + assert!(open(&mut s, dir.path()).unwrap().is_empty(), "the live document's own snapshot is not recovered"); + assert_eq!(s.docs.len(), 1); + assert_eq!(finished_writes(&mut s), 1); + assert_eq!(list(dir.path()).len(), 1); + discard(dir.path(), uid); +} + #[cfg(not(target_arch = "wasm32"))] #[test] fn an_edit_after_a_save_is_written_after_the_cancelled_write_stops() { From 213f3b75a309b0ad45e087f975800011ee5b494f Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Thu, 12 Nov 2026 15:43:43 -0500 Subject: [PATCH 14/14] Report recovery writes owned by another session --- crates/engine/src/recovery.rs | 37 ++++++++++++++++++++--------- crates/engine/src/recovery/tests.rs | 26 ++++++++++++++++++++ 2 files changed, 52 insertions(+), 11 deletions(-) diff --git a/crates/engine/src/recovery.rs b/crates/engine/src/recovery.rs index d045b2415..16b1e0ee2 100644 --- a/crates/engine/src/recovery.rs +++ b/crates/engine/src/recovery.rs @@ -886,8 +886,14 @@ pub(crate) fn start_tracked(s: &mut Session, dir: &Path) -> Result { // A write still running for this document already holds a snapshot of it; the next // timer tick catches up with the edits made since. match worker::submit(entry) { - Some(true) => queued += 1, - Some(false) => {} + Some(worker::Submit::Queued) => queued += 1, + Some(worker::Submit::AlreadyQueued) => {} + Some(worker::Submit::Conflict) => { + return Err(crate::EngineError::Other(format!( + "recovery data for document {} is already being written by another session", + document.uid + ))); + } None => { // No thread: write this one here rather than lose the recovery data. let written = write_snapshot(&Snapshot::of(&access, dir, document), || false); @@ -1326,24 +1332,33 @@ mod worker { }) } - /// Queue a document's write. `None`: no writer thread (the caller writes it itself); - /// `Some(false)`: one for this document is still queued or running. - pub(super) fn submit(entry: Snapshot) -> Option { + pub(super) enum Submit { + Queued, + AlreadyQueued, + Conflict, + } + + /// Queue a document's write. `None`: no writer thread (the caller writes it itself). + pub(super) fn submit(entry: Snapshot) -> Option { let w = writer()?; let mut running = w.running.lock().ok()?; let key = (entry.key.clone(), entry.uid); - // A session is writing this document now: this write waits for a later turn. - if w.writing_now.lock().is_ok_and(|now| now.contains_key(&key)) { - return Some(false); + // Another session's save or recovery write owns this destination. Check ownership while + // holding the same reservation locks used to queue or publish writes. + if let Some(owner) = w.writing_now.lock().ok().and_then(|now| now.get(&key).copied()) { + return Some(if owner == entry.owner { Submit::AlreadyQueued } else { Submit::Conflict }); } if let Some(q) = running.get_mut(&key) { // The write in flight was cancelled (the document was saved since), so it holds // nothing to recover: this one follows it, in place of any that was waiting. if w.cancelled.lock().is_ok_and(|cancelled| cancelled.contains(&key)) { + if q.next.as_ref().is_some_and(|next| next.owner != entry.owner) { + return Some(Submit::Conflict); + } q.next = Some(entry); - return Some(true); + return Some(Submit::Queued); } - return Some(false); + return Some(if q.owner == entry.owner { Submit::AlreadyQueued } else { Submit::Conflict }); } running.insert(key.clone(), Queued { owner: entry.owner, dir: entry.dir.clone(), next: None }); // A write is wanted again: whatever cancelled the last one does not apply to this one. @@ -1354,7 +1369,7 @@ mod worker { running.remove(&key); return None; } - Some(true) + Some(Submit::Queued) } } diff --git a/crates/engine/src/recovery/tests.rs b/crates/engine/src/recovery/tests.rs index f66d28879..8d28dc8a9 100644 --- a/crates/engine/src/recovery/tests.rs +++ b/crates/engine/src/recovery/tests.rs @@ -705,6 +705,32 @@ fn a_clean_document_leaves_another_sessions_write_queued_behind_its_own() { discard(dir.path(), uid); } +#[cfg(not(target_arch = "wasm32"))] +#[test] +fn a_background_recovery_write_reports_another_sessions_held_write() { + let _turn = background(); + let dir = tempfile::tempdir().unwrap(); + let mut first = dirty_session(); + first.recovery_dir = Some(dir.path().to_path_buf()); + let uid = first.docs[0].uid; + let mut second = dirty_session(); + second.recovery_dir = Some(dir.path().to_path_buf()); + second.docs[0].uid = uid; + second.execute("frame.create", &json!({"rect": [20, 20, 60, 60]})).unwrap(); + + let held = testing::hold_next_write(dir.path()); + first.execute("file.recovery.save", &json!({"background": true})).unwrap(); + held.wait_until_held(); + + let err = second.execute("file.recovery.save", &json!({"background": true})).unwrap_err(); + assert!(err.to_string().contains("another session"), "{err}"); + + held.release(); + assert_eq!(finished_writes(&mut first), 1); + assert!(files(dir.path(), uid).0.exists(), "the first session's recovery file remains intact"); + discard(dir.path(), uid); +} + #[cfg(not(target_arch = "wasm32"))] #[test] fn a_clean_document_in_another_session_leaves_this_sessions_write_alone() {