Do not push a catalog over one we could not read
`sync_catalog` is a read-modify-write over a file another device also
writes: take theirs, merge, push the union. It was shaped
if let Ok(bytes) = backend.get(&RemoteId::Path(target), None).await {
which folds *every* failure into "there is no remote catalog" and carries
straight on to the upload. On a placeholder library the snapshot in
`.darkroom-derived/` is dehydrated like anything else, so the read failed
every time and each sync pushed our catalog over theirs unmerged —
taking the other device's collections and their members with it.
The same shape as the sidecar bug, and the same fix: a read that fails
for anything other than `NotFound` stops the upload and says why. An
unreadable or unopenable snapshot stops it too — "will not parse" is not
"is not there". This is what `NotFound` and `NotMaterialised` being
separate errors is *for*: one means ours is the whole truth, the other
means do not dare.
Shard downloads go through the same fetch-on-demand read. They logged
and skipped before, which on a library the client keeps dehydrated is
every shard, every pass, and a peer's thumbnails and faces silently
never arriving.
And `put` over a placeholder now replaces it rather than refusing.
Refusing was over-cautious of me: derived state lives inside the library
folder, so a folder the client had dehydrated could never be written to
again. An unconditional write replaces the whole file, so there is
nothing in the stub to keep — content first, then the placeholder, since
in a synced tree an absence is a deletion that propagates. `IfMatch`
still refuses, because a stub's validator describes the stub; `IfAbsent`
fails, because the file is there and only its content is not.
This commit is contained in:
@@ -34,7 +34,7 @@
|
||||
|
||||
use std::path::{Path, PathBuf};
|
||||
|
||||
use dr_sync::{Connection, RemoteBackend, RemoteId, RemotePath};
|
||||
use dr_sync::{Connection, RemoteBackend, RemoteError, RemoteId, RemotePath};
|
||||
|
||||
use dr_thumbs::ThumbStore;
|
||||
|
||||
@@ -281,7 +281,10 @@ async fn sync_shards(
|
||||
}
|
||||
|
||||
let source = RemotePath::new(format!("{}/{name}", base.as_str()));
|
||||
let bytes = match backend.get(&RemoteId::Path(source), None).await {
|
||||
// Fetched where it is only a placeholder: a shard that will not open
|
||||
// is a peer's thumbnails never merging, and on a library the client
|
||||
// keeps dehydrated that would be every shard, every pass, silently.
|
||||
let bytes = match read_derived(backend, &source).await {
|
||||
Ok(b) => b,
|
||||
Err(e) => {
|
||||
log::warn!("downloading {name}: {e}");
|
||||
@@ -464,7 +467,9 @@ async fn sync_face_shards(
|
||||
)));
|
||||
|
||||
let source = RemotePath::new(format!("{}/{name}", face_base.as_str()));
|
||||
let bytes = match backend.get(&RemoteId::Path(source), None).await {
|
||||
// Fetched where it is only a placeholder, for the reason the thumbnail
|
||||
// shards are: otherwise a peer's faces never arrive and nothing says so.
|
||||
let bytes = match read_derived(backend, &source).await {
|
||||
Ok(b) => b,
|
||||
Err(e) => {
|
||||
log::warn!("downloading face shard {name}: {e}");
|
||||
@@ -547,7 +552,30 @@ async fn sync_catalog(
|
||||
// Merging before uploading means our upload carries the union rather than
|
||||
// only our own half, so a third device syncing next gets everything in one
|
||||
// fetch.
|
||||
if let Ok(bytes) = backend.get(&RemoteId::Path(target.clone()), None).await {
|
||||
// TRACES: FR-NC-9 | FR-NC-6c
|
||||
// A read that fails for any reason other than "there is not one yet" must
|
||||
// stop the upload below. This is a read-modify-write over a file another
|
||||
// device also writes, so skipping the read does not merely lose an
|
||||
// optimisation — it turns the write into a clobber, and the other device's
|
||||
// collections and their members go with it.
|
||||
//
|
||||
// The shape was previously `if let Ok(bytes) = ...`, which swallowed every
|
||||
// failure into "no remote catalog" and carried straight on to the upload.
|
||||
let theirs = match read_derived(backend, &target).await {
|
||||
Ok(bytes) => Some(bytes),
|
||||
// Genuinely the first sync of this library. Nothing to merge, and
|
||||
// ours is the whole truth.
|
||||
Err(RemoteError::NotFound(_)) => None,
|
||||
Err(e) => {
|
||||
log::warn!(
|
||||
"not pushing the catalog: the copy on the server could not be read ({e}); \
|
||||
uploading over it would discard whatever another device put there"
|
||||
);
|
||||
return Ok(());
|
||||
}
|
||||
};
|
||||
|
||||
if let Some(bytes) = theirs {
|
||||
let downloaded = scratch.join("catalog-remote.sqlite");
|
||||
if std::fs::write(&downloaded, &bytes).is_ok() {
|
||||
match dr_catalog::Catalog::open(catalog_path) {
|
||||
@@ -557,9 +585,19 @@ async fn sync_catalog(
|
||||
report.collections_gained = merge.inserted + merge.updated;
|
||||
report.members_gained = merge.members_added;
|
||||
}
|
||||
Err(e) => log::warn!("merging remote catalog: {e}"),
|
||||
// Unreadable is not the same as absent: it may be a newer
|
||||
// format, or a torn upload. Ours must not go over it.
|
||||
Err(e) => {
|
||||
log::warn!("not pushing the catalog: merging the server's copy: {e}");
|
||||
let _ = std::fs::remove_file(&downloaded);
|
||||
return Ok(());
|
||||
}
|
||||
},
|
||||
Err(e) => log::warn!("opening catalog to merge: {e}"),
|
||||
Err(e) => {
|
||||
log::warn!("not pushing the catalog: opening ours to merge: {e}");
|
||||
let _ = std::fs::remove_file(&downloaded);
|
||||
return Ok(());
|
||||
}
|
||||
}
|
||||
let _ = std::fs::remove_file(&downloaded);
|
||||
}
|
||||
@@ -586,6 +624,34 @@ async fn sync_catalog(
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// TRACES: FR-NC-6c
|
||||
/// Read a derived file, fetching its content first if only a placeholder is
|
||||
/// here.
|
||||
///
|
||||
/// Derived state lives *inside the library folder*, so on a placeholder
|
||||
/// library a sync client dehydrates a shard or a catalog snapshot exactly as
|
||||
/// it dehydrates a photograph. Unlike a photograph, these are ours, and none of
|
||||
/// them can be skipped: a shard that will not open is face data that never
|
||||
/// merges, and a catalog snapshot that will not open is the other device's
|
||||
/// collections.
|
||||
///
|
||||
/// So this fetches rather than giving up — and where it cannot, it says so
|
||||
/// with the error rather than an empty result, because the callers below treat
|
||||
/// "nothing there" as licence to write their own copy (ARCH §9.0a).
|
||||
async fn read_derived(
|
||||
backend: &dyn RemoteBackend,
|
||||
path: &RemotePath,
|
||||
) -> Result<Vec<u8>, RemoteError> {
|
||||
let id = RemoteId::Path(path.clone());
|
||||
match backend.get(&id, None).await {
|
||||
Err(RemoteError::NotMaterialised(_)) => {
|
||||
backend.materialise(&id).await?;
|
||||
backend.get(&id, None).await
|
||||
}
|
||||
other => other,
|
||||
}
|
||||
}
|
||||
|
||||
fn shard_name(client: &str, id: u32) -> String {
|
||||
format!("shard-{client}-{id:04}.sqlite")
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user