From 21d599b420ba12060ec1c3774fa5918efcac953d Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Sat, 29 Aug 2026 10:40:38 +0200 Subject: [PATCH] Test the guard, since the bug was a branch nobody ran MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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`. --- Cargo.lock | 1 + ui/dr-ui/Cargo.toml | 3 + ui/dr-ui/src/derived_sync.rs | 162 +++++++++++++++++++++++++++++++++++ 3 files changed, 166 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index ee1f518..e630191 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1620,6 +1620,7 @@ name = "dr-ui" version = "0.8.0" dependencies = [ "anyhow", + "async-trait", "dr-catalog", "dr-decode", "dr-export", diff --git a/ui/dr-ui/Cargo.toml b/ui/dr-ui/Cargo.toml index 2baefce..77d30f2 100644 --- a/ui/dr-ui/Cargo.toml +++ b/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 diff --git a/ui/dr-ui/src/derived_sync.rs b/ui/dr-ui/src/derived_sync.rs index a41c202..2b38d7c 100644 --- a/ui/dr-ui/src/derived_sync.rs +++ b/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, + puts: Arc, + caps: dr_sync::Capabilities, + } + + impl Fussy { + fn reading(fail_with: Option) -> (Self, Arc) { + 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, RemoteError> { + Ok(Vec::new()) + } + async fn dir_validator( + &self, + _dir: &RemotePath, + ) -> Result { + Err(RemoteError::Unsupported("test")) + } + async fn delta( + &self, + _c: &dr_sync::Cursor, + ) -> Result<(Vec, dr_sync::Cursor), RemoteError> { + Err(RemoteError::Unsupported("test")) + } + async fn get( + &self, + _id: &RemoteId, + _r: Option>, + ) -> Result, 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, + _pc: Option, + ) -> Result { + self.puts.fetch_add(1, Ordering::SeqCst); + Ok(dr_sync::Validator::new("v")) + } + async fn delete( + &self, + _id: &RemoteId, + _pc: Option, + ) -> 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, 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); + } +}