From 4193a82dfbb0cbc1d28e93a7df6337dbd63d6e05 Mon Sep 17 00:00:00 2001 From: NetterSilver <133535268+Silvertree2010@users.noreply.github.com> Date: Wed, 14 Oct 2026 16:08:47 +0200 Subject: [PATCH] Count a recovery revision as copied only after its write succeeds Fixes #186 Co-Authored-By: Claude Haiku 5.5 Co-Authored-By: Claude Sonnet 5.5 --- apps/figcraft/src/recovery.rs | 63 ++++++++++++++++++++++------- apps/figcraft/src/recovery_tests.rs | 48 ++++++++++++++++++++++ contributors/people.toml | 4 ++ 3 files changed, 101 insertions(+), 14 deletions(-) diff --git a/apps/figcraft/src/recovery.rs b/apps/figcraft/src/recovery.rs index 93335394..0885c68e 100644 --- a/apps/figcraft/src/recovery.rs +++ b/apps/figcraft/src/recovery.rs @@ -41,14 +41,40 @@ pub struct Recovery { dir: PathBuf, /// Held while the app runs: a free lock means the run that wrote the folder is gone. _lock: File, - /// The revision each document was last copied at, and the write in flight. - written: HashMap>)>, + /// What was copied of each document, and the write in flight. + written: HashMap, /// When (UI clock) the next copies are due. next: f64, /// Copies of online files from earlier runs, kept until their outcome is known. waiting: Vec, } +/// The state of the copy of one document: only a write that succeeded counts as a copy. +#[derive(Default)] +struct Copy { + /// The revision whose copy was written. + done: Option, + /// The write in flight: the revision it copies, and whether it succeeded once it ends. + flight: Option<(u64, JoinHandle)>, +} + +impl Copy { + /// Take in the result of the write in flight once it ended; a failed one leaves its revision uncopied. + fn reap(&mut self) { + if self.flight.as_ref().is_some_and(|f| f.1.is_finished()) + && let Some((revision, h)) = self.flight.take() + && h.join().unwrap_or(false) + { + self.done = Some(revision); + } + } + + /// Whether `revision` has neither been copied nor is being copied. + fn pending(&self, revision: u64) -> bool { + self.done != Some(revision) && self.flight.as_ref().is_none_or(|f| f.0 != revision) + } +} + /// A copy of one of your online files whose file is being joined again as session `session`. pub struct Waiting { session: u64, @@ -86,15 +112,21 @@ impl Recovery { /// an idle window can sleep). pub fn tick(&mut self, s: &Session, now: f64) -> Option { self.settle(s); + self.written.values_mut().for_each(Copy::reap); // A join on its way settles within its timeouts: look again soon. let settling = (!self.waiting.is_empty()).then_some(0.5); - let pending = s.docs.iter().any(|d| needs_copy(d) && self.written.get(&d.uid).is_none_or(|w| w.0 != d.revision)); if now < self.next { - return pending.then_some(self.next - now).or(settling); + return self.pending(s).then_some(self.next - now).or(settling); } self.write(s, false); self.next = now + INTERVAL; - settling + // A copy that failed is tried again at the next turn, without another edit. + settling.or(self.pending(s).then_some(INTERVAL)) + } + + /// Whether a document with unsaved changes has a revision that is neither copied nor being copied. + fn pending(&self, s: &Session) -> bool { + s.docs.iter().any(|d| needs_copy(d) && self.written.get(&d.uid).is_none_or(|w| w.pending(d.revision))) } /// Remove earlier runs' copies whose outcome is known, once what they became is copied here. @@ -131,9 +163,10 @@ impl Recovery { } continue; } - let entry = self.written.entry(d.uid).or_insert((0, None)); - let busy = entry.1.as_ref().is_some_and(|h| !h.is_finished()); - if entry.0 == d.revision || busy { + let entry = self.written.entry(d.uid).or_default(); + entry.reap(); + // A write still in flight (for any revision) is waited for; a failed one is tried again. + if entry.done == Some(d.revision) || entry.flight.is_some() { continue; } let doc = d.doc.clone(); @@ -144,20 +177,22 @@ impl Recovery { let online = s.online_of(d.uid).map(|(server, file, at)| format!("{server}\n{file}\n{} {} {}", at.session, at.client_id, at.seq)); let job = move || { if !write_atomic(&path, &doc) { - return; + return false; } let _ = match online { Some(text) => std::fs::write(¬e, text), None => std::fs::remove_file(¬e), }; + true }; + // The revision counts as copied only once its write succeeded. if wait { - job(); - entry.1 = None; + if job() { + entry.done = Some(d.revision); + } } else { - entry.1 = std::thread::Builder::new().name("recovery".into()).spawn(job).ok(); + entry.flight = std::thread::Builder::new().name("recovery".into()).spawn(job).ok().map(|h| (d.revision, h)); } - entry.0 = d.revision; } // Closed documents. let open: Vec = s.docs.iter().map(|d| d.uid).collect(); @@ -177,7 +212,7 @@ impl Recovery { pub fn finish(self) { let Self { dir, _lock, written, .. } = self; // Windows removes a folder only once nothing in it is open: copies being written first. - for h in written.into_values().filter_map(|w| w.1) { + for (_, h) in written.into_values().filter_map(|w| w.flight) { let _ = h.join(); } drop(_lock); diff --git a/apps/figcraft/src/recovery_tests.rs b/apps/figcraft/src/recovery_tests.rs index 95560319..18d18f6a 100644 --- a/apps/figcraft/src/recovery_tests.rs +++ b/apps/figcraft/src/recovery_tests.rs @@ -62,3 +62,51 @@ fn copies_follow_saves_and_a_normal_quit_leaves_nothing() { assert!(!dir.exists(), "a normal quit removes the copies"); let _ = std::fs::remove_dir_all(&root); } + +/// The files named `*.figcraft` in the run folder under `root`. +fn copy_count(root: &std::path::Path) -> usize { + let Some(dir) = std::fs::read_dir(root).unwrap().flatten().next().map(|e| e.path()) else { return 0 }; + std::fs::read_dir(dir).unwrap().flatten().filter(|f| f.path().extension().is_some_and(|e| e == "figcraft")).count() +} + +#[test] +fn a_copy_that_failed_is_tried_again_without_another_edit() { + let root = root("retry"); + let s = edited(); + let mut r = Recovery::start(&root).unwrap(); + let dir = std::fs::read_dir(&root).unwrap().flatten().next().unwrap().path(); + let moved = root.join("moved"); + // The run folder is gone while the copy is written: it fails. + std::fs::rename(&dir, &moved).unwrap(); + r.write(&s, true); + std::fs::rename(&moved, &dir).unwrap(); + // Nothing was edited since: the same revision still has to be copied. + r.write(&s, true); + assert_eq!(copy_count(&root), 1, "the failed revision was copied again"); + r.finish(); + let _ = std::fs::remove_dir_all(&root); +} + +#[test] +fn a_background_copy_that_failed_is_tried_again_without_another_edit() { + let root = root("retry-bg"); + let s = edited(); + let mut r = Recovery::start(&root).unwrap(); + let dir = std::fs::read_dir(&root).unwrap().flatten().next().unwrap().path(); + let moved = root.join("moved"); + std::fs::rename(&dir, &moved).unwrap(); + r.write(&s, false); + // Let the failing write end before the folder comes back. + std::thread::sleep(std::time::Duration::from_millis(300)); + std::fs::rename(&moved, &dir).unwrap(); + for _ in 0..50 { + r.write(&s, false); + if copy_count(&root) == 1 { + break; + } + std::thread::sleep(std::time::Duration::from_millis(100)); + } + assert_eq!(copy_count(&root), 1, "the failed revision was copied again"); + r.finish(); + let _ = std::fs::remove_dir_all(&root); +} diff --git a/contributors/people.toml b/contributors/people.toml index e61cfd31..db1f2b54 100644 --- a/contributors/people.toml +++ b/contributors/people.toml @@ -16,3 +16,7 @@ # # To remove your names, delete your entry (or set fields empty) in a PR. The next # contributors.py run, and the next build, drop them. + +[people.Silvertree2010] +display_name = "Netter" +sync_github_name = false