Test the guard, since the bug was a branch nobody ran

The catalog clobber existed because `if let Ok(bytes)` had a failure arm
that was never exercised. Fixing it without covering that arm leaves the
next person free to collapse it back.

Three tests against a backend whose read fails in a chosen way, counting
writes — because what went wrong was not a wrong value but a write that
should not have happened at all:

- a dehydrated snapshot uploads nothing
- an unreadable one uploads nothing either, since "refused" is no more
  "absent" than "not downloaded" is
- and a genuine first sync still uploads, which is the half that keeps
  `NotFound` distinct from `NotMaterialised` rather than merely cautious

Checked against the original shape: the first two fail on it and the
third passes. A guard test that cannot tell the bug from the fix is
decoration.

`async-trait` joins dev-dependencies to stand the double up behind
`dyn RemoteBackend`.

ui/dr-ui/Cargo.toml

@@ -120,3 +120,6 @@ live-style = ["dep:serde_norway"]
 # The face_index batch job wants a log level from the environment; the library
 # itself only ever calls `log`, and picks up whatever the application installs.
 env_logger.workspace = true
+# Standing in a test backend behind `dyn RemoteBackend`, which is an
+# `#[async_trait]` trait — implementing one needs the same attribute.
+async-trait.workspace = true

ui/dr-ui/src/derived_sync.rs

@@ -753,3 +753,165 @@ mod tests {
         .did_anything());
     }
 }
+
+#[cfg(test)]
+mod catalog_guard_tests {
+    //! What `sync_catalog` does when it cannot read the server's copy.
+    //!
+    //! The bug these exist for was a control-flow one — `if let Ok(bytes)`
+    //! folding every failure into "there is none yet" and falling through to
+    //! the upload — so the thing to assert is not a value but *whether a write
+    //! happened at all*.
+
+    use super::*;
+    use std::sync::atomic::{AtomicUsize, Ordering};
+    use std::sync::Arc;
+
+    /// A backend whose read fails in a chosen way, counting writes.
+    struct Fussy {
+        fail_with: Option<RemoteError>,
+        puts: Arc<AtomicUsize>,
+        caps: dr_sync::Capabilities,
+    }
+
+    impl Fussy {
+        fn reading(fail_with: Option<RemoteError>) -> (Self, Arc<AtomicUsize>) {
+            let puts = Arc::new(AtomicUsize::new(0));
+            (
+                Self {
+                    fail_with,
+                    puts: puts.clone(),
+                    caps: dr_sync::Capabilities::minimal(),
+                },
+                puts,
+            )
+        }
+    }
+
+    #[async_trait::async_trait]
+    impl RemoteBackend for Fussy {
+        fn capabilities(&self) -> &dr_sync::Capabilities {
+            &self.caps
+        }
+        fn name(&self) -> &str {
+            "fussy"
+        }
+        async fn list(
+            &self,
+            _dir: &RemotePath,
+            _since: Option<&dr_sync::Validator>,
+        ) -> Result<Vec<dr_sync::RemoteEntry>, RemoteError> {
+            Ok(Vec::new())
+        }
+        async fn dir_validator(
+            &self,
+            _dir: &RemotePath,
+        ) -> Result<dr_sync::Validator, RemoteError> {
+            Err(RemoteError::Unsupported("test"))
+        }
+        async fn delta(
+            &self,
+            _c: &dr_sync::Cursor,
+        ) -> Result<(Vec<dr_sync::RemoteChange>, dr_sync::Cursor), RemoteError> {
+            Err(RemoteError::Unsupported("test"))
+        }
+        async fn get(
+            &self,
+            _id: &RemoteId,
+            _r: Option<std::ops::Range<u64>>,
+        ) -> Result<Vec<u8>, RemoteError> {
+            match &self.fail_with {
+                Some(RemoteError::NotFound(s)) => Err(RemoteError::NotFound(s.clone())),
+                Some(RemoteError::NotMaterialised(s)) => {
+                    Err(RemoteError::NotMaterialised(s.clone()))
+                }
+                Some(_) => Err(RemoteError::PermissionDenied),
+                None => Ok(Vec::new()),
+            }
+        }
+        async fn put(
+            &self,
+            _p: &RemotePath,
+            _b: Vec<u8>,
+            _pc: Option<dr_sync::Precondition>,
+        ) -> Result<dr_sync::Validator, RemoteError> {
+            self.puts.fetch_add(1, Ordering::SeqCst);
+            Ok(dr_sync::Validator::new("v"))
+        }
+        async fn delete(
+            &self,
+            _id: &RemoteId,
+            _pc: Option<dr_sync::Precondition>,
+        ) -> Result<(), RemoteError> {
+            Ok(())
+        }
+        async fn move_to(&self, _f: &RemoteId, _t: &RemotePath) -> Result<(), RemoteError> {
+            Ok(())
+        }
+        async fn create_dir(&self, _p: &RemotePath) -> Result<(), RemoteError> {
+            Ok(())
+        }
+    }
+
+    /// A real catalog and a scratch directory, since `sync_catalog` snapshots
+    /// one before uploading.
+    fn fixture(name: &str) -> (std::path::PathBuf, std::path::PathBuf) {
+        let dir = std::env::temp_dir().join(format!("dr-catalog-guard-{name}"));
+        let _ = std::fs::remove_dir_all(&dir);
+        std::fs::create_dir_all(dir.join("scratch")).unwrap();
+        let catalog_path = dir.join("catalog.sqlite");
+        dr_catalog::Catalog::open(&catalog_path).unwrap();
+        (catalog_path, dir.join("scratch"))
+    }
+
+    async fn run_with(fail_with: Option<RemoteError>, name: &str) -> (usize, SyncReport) {
+        let (catalog_path, scratch) = fixture(name);
+        let (backend, puts) = Fussy::reading(fail_with);
+        let mut report = SyncReport::default();
+        sync_catalog(
+            &backend,
+            &RemotePath::new(".darkroom-derived"),
+            &catalog_path,
+            &scratch,
+            &mut report,
+        )
+        .await
+        .unwrap();
+        let _ = std::fs::remove_dir_all(catalog_path.parent().unwrap());
+        (puts.load(Ordering::SeqCst), report)
+    }
+
+    #[tokio::test]
+    async fn a_catalog_that_is_here_but_not_downloaded_is_never_written_over() {
+        // The bug. On a placeholder library the snapshot is dehydrated, the
+        // read fails, and the old code took that for "there is no remote
+        // catalog" and pushed ours — discarding the other device's
+        // collections and their members on every single sync.
+        let (puts, report) = run_with(
+            Some(RemoteError::NotMaterialised("catalog.sqlite".into())),
+            "notmaterialised",
+        )
+        .await;
+        assert_eq!(puts, 0, "must not upload over a catalog it could not read");
+        assert!(!report.catalog_uploaded);
+        assert!(!report.catalog_merged);
+    }
+
+    #[tokio::test]
+    async fn a_catalog_that_cannot_be_read_at_all_is_never_written_over() {
+        // Not only placeholders: a refused read, a dropped connection. Any
+        // failure that is not "there is none" leaves the server's copy alone.
+        let (puts, _) = run_with(Some(RemoteError::PermissionDenied), "denied").await;
+        assert_eq!(puts, 0);
+    }
+
+    #[tokio::test]
+    async fn the_first_sync_of_a_library_still_uploads() {
+        // The other half, and the reason `NotFound` had to stay distinct: with
+        // genuinely nothing on the server, ours *is* the whole truth and
+        // refusing to push it would mean the catalog never syncs at all.
+        let (puts, report) = run_with(Some(RemoteError::NotFound("nope".into())), "firstrun").await;
+        assert_eq!(puts, 1, "nothing to merge, so ours goes up");
+        assert!(report.catalog_uploaded);
+    }
+}