Merge #297: Count a recovery revision as copied only after its write succeeds (@Silvertree2010)

# Conflicts:
#	contributors/people.toml

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<u64, (u64, Option<JoinHandle<()>>)>,
+    /// What was copied of each document, and the write in flight.
+    written: HashMap<u64, Copy>,
     /// 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<Waiting>,
 }
 
+/// 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<u64>,
+    /// The write in flight: the revision it copies, and whether it succeeded once it ends.
+    flight: Option<(u64, JoinHandle<bool>)>,
+}
+
+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<f64> {
         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(&note, text),
                     None => std::fs::remove_file(&note),
                 };
+                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<u64> = 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);

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);
+}

contributors/people.toml

@@ -23,3 +23,7 @@ display_name = "tylerjenningsw"
 
 [people.VanTan666]
 display_name = "VanTan"
+
+[people.Silvertree2010]
+display_name = "Netter"
+sync_github_name = false