Stop reading dates at the first sign the server is unreachable
A window of cells whose thumbnails were cached but whose dates were not sent a header read per cell, and offline each one was three attempts at a 15 s connect timeout: `is_transient` counts a network error as worth retrying, and `read_metadata_only` returned a bare bool that could not say why a read failed. So the grid sat on "reading N dates" for minutes against a server that was not there, and no banner went up, because nothing in that loop ever reported the connection. `read_metadata_only` now returns a `DateRead`: reached, failed, or offline. An offline error is returned on the first attempt rather than retried — a dead server answers the second exactly as the first — while a 423 lock is still retried, which is what the retry was for. The grid's worker stops on it and sends `Offline`, as its fetch loop already did, so the banner goes up and the bar stops. The sweep's lanes stop on it too, one timeout each rather than one per image.
This commit is contained in:
File diff suppressed because one or more lines are too long
@@ -199,7 +199,7 @@ fn the_catalog_scan_reads_headers_through_the_trait() {
|
||||
&mut found,
|
||||
));
|
||||
|
||||
assert!(reached);
|
||||
assert!(matches!(reached, crate::library::DateRead::Reached));
|
||||
assert_eq!(found.len(), 1, "the stub's header was read");
|
||||
assert_eq!(found[0].image_id, 7);
|
||||
assert_eq!(found[0].camera.as_deref(), Some("Stubco One"));
|
||||
|
||||
@@ -83,7 +83,7 @@ pub(super) fn flush_metadata(
|
||||
/// shards synced from another device, which carry pixels but no metadata.
|
||||
/// Read a header for its date.
|
||||
///
|
||||
/// Returns whether the file was **reached**, which the caller needs and cannot
|
||||
/// Reports whether the file was **reached**, which the caller needs and cannot
|
||||
/// otherwise tell: a header that carried no EXIF and a fetch that never
|
||||
/// happened both leave `found` untouched, and recording the second as "this
|
||||
/// image has no date" would let one lock mark it dateless for good.
|
||||
@@ -92,7 +92,7 @@ pub(crate) async fn read_metadata_only(
|
||||
decoder: &dyn dr_decode::Decoder,
|
||||
req: &ThumbnailRequest,
|
||||
found: &mut Vec<MetadataFound>,
|
||||
) -> bool {
|
||||
) -> DateRead {
|
||||
let id = RemoteId::Path(RemotePath::new(&req.path));
|
||||
|
||||
// Retried, because one failure here is usually a lock rather than a
|
||||
@@ -108,7 +108,16 @@ pub(crate) async fn read_metadata_only(
|
||||
match backend.get(&id, Some(0..decoder.header_bytes())).await {
|
||||
Ok(header) => {
|
||||
collect_metadata(backend, decoder, &id, &header, req, found).await;
|
||||
return true;
|
||||
return DateRead::Reached;
|
||||
}
|
||||
// Not retried, unlike a lock. A dead server answers the second
|
||||
// attempt exactly as it answered the first, after the same 15 s
|
||||
// connect timeout — three of those per image turned a window of
|
||||
// cached-but-undated cells into minutes of "reading dates"
|
||||
// against nothing, with no banner, because nothing said why.
|
||||
Err(e) if e.indicates_offline() => {
|
||||
log::debug!("reading date for {}: {e}", req.path);
|
||||
return DateRead::Offline(e.to_string());
|
||||
}
|
||||
Err(e) if e.is_transient() && attempt < ATTEMPTS => {
|
||||
// Backing off at all matters more than the exact interval: the
|
||||
@@ -121,11 +130,23 @@ pub(crate) async fn read_metadata_only(
|
||||
// timeline rather than breaking anything, and the next sweep
|
||||
// retries it regardless.
|
||||
log::debug!("reading date for {} ({attempt} attempts): {e}", req.path);
|
||||
return false;
|
||||
return DateRead::Failed;
|
||||
}
|
||||
}
|
||||
}
|
||||
false
|
||||
DateRead::Failed
|
||||
}
|
||||
|
||||
/// What one header read for a date came to.
|
||||
#[derive(Debug, PartialEq, Eq)]
|
||||
pub(crate) enum DateRead {
|
||||
/// The header arrived. Whatever EXIF it held is in `found`.
|
||||
Reached,
|
||||
/// This file could not be read; the next pass tries it again.
|
||||
Failed,
|
||||
/// The server could not be reached, so no read after this one will be
|
||||
/// either. The caller stops rather than paying a timeout per image.
|
||||
Offline(String),
|
||||
}
|
||||
|
||||
/// Write capture metadata read during the thumbnail pass.
|
||||
@@ -381,8 +402,12 @@ pub fn spawn_sweep(conn: Connection, catalog_path: PathBuf) -> Receiver<SweepMes
|
||||
let mut found = Vec::new();
|
||||
let mut reached = Vec::new();
|
||||
for req in lane {
|
||||
if read_metadata_only(backend, decoder, req, &mut found).await {
|
||||
reached.push(req.image_id);
|
||||
match read_metadata_only(backend, decoder, req, &mut found).await {
|
||||
DateRead::Reached => reached.push(req.image_id),
|
||||
DateRead::Failed => {}
|
||||
// The other lanes find the same, each after
|
||||
// one timeout rather than one per image.
|
||||
DateRead::Offline(_) => break,
|
||||
}
|
||||
}
|
||||
(found, reached)
|
||||
@@ -901,6 +926,130 @@ pub(super) fn thumbnails_outstanding(
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
/// A backend whose every read fails the same way, counting the attempts.
|
||||
struct Refusing {
|
||||
error: fn() -> dr_sync::RemoteError,
|
||||
gets: std::sync::atomic::AtomicUsize,
|
||||
caps: dr_sync::Capabilities,
|
||||
}
|
||||
|
||||
#[async_trait::async_trait]
|
||||
impl RemoteBackend for Refusing {
|
||||
fn capabilities(&self) -> &dr_sync::Capabilities {
|
||||
&self.caps
|
||||
}
|
||||
fn name(&self) -> &str {
|
||||
"refusing"
|
||||
}
|
||||
async fn list(
|
||||
&self,
|
||||
_dir: &RemotePath,
|
||||
_since: Option<&dr_sync::Validator>,
|
||||
) -> Result<Vec<dr_sync::RemoteEntry>, dr_sync::RemoteError> {
|
||||
Err((self.error)())
|
||||
}
|
||||
async fn dir_validator(
|
||||
&self,
|
||||
_dir: &RemotePath,
|
||||
) -> Result<dr_sync::Validator, dr_sync::RemoteError> {
|
||||
Err((self.error)())
|
||||
}
|
||||
async fn delta(
|
||||
&self,
|
||||
_c: &dr_sync::Cursor,
|
||||
) -> Result<(Vec<dr_sync::RemoteChange>, dr_sync::Cursor), dr_sync::RemoteError> {
|
||||
Err((self.error)())
|
||||
}
|
||||
async fn get(
|
||||
&self,
|
||||
_id: &RemoteId,
|
||||
_r: Option<std::ops::Range<u64>>,
|
||||
) -> Result<Vec<u8>, dr_sync::RemoteError> {
|
||||
self.gets.fetch_add(1, std::sync::atomic::Ordering::SeqCst);
|
||||
Err((self.error)())
|
||||
}
|
||||
async fn put(
|
||||
&self,
|
||||
_p: &RemotePath,
|
||||
_b: Vec<u8>,
|
||||
_c: Option<dr_sync::Precondition>,
|
||||
) -> Result<dr_sync::Validator, dr_sync::RemoteError> {
|
||||
Err((self.error)())
|
||||
}
|
||||
async fn delete(
|
||||
&self,
|
||||
_id: &RemoteId,
|
||||
_c: Option<dr_sync::Precondition>,
|
||||
) -> Result<(), dr_sync::RemoteError> {
|
||||
Err((self.error)())
|
||||
}
|
||||
async fn move_to(
|
||||
&self,
|
||||
_f: &RemoteId,
|
||||
_t: &RemotePath,
|
||||
) -> Result<(), dr_sync::RemoteError> {
|
||||
Err((self.error)())
|
||||
}
|
||||
async fn create_dir(&self, _p: &RemotePath) -> Result<(), dr_sync::RemoteError> {
|
||||
Err((self.error)())
|
||||
}
|
||||
}
|
||||
|
||||
fn read_date_against(error: fn() -> dr_sync::RemoteError) -> (DateRead, usize) {
|
||||
let backend = Refusing {
|
||||
error,
|
||||
gets: Default::default(),
|
||||
caps: dr_sync::Capabilities::minimal(),
|
||||
};
|
||||
let req = ThumbnailRequest {
|
||||
row: 0,
|
||||
path: "a.CR2".into(),
|
||||
file_id: Some(1),
|
||||
size: 0,
|
||||
image_id: 1,
|
||||
thumb_size: dr_thumbs::ThumbSize::Grid,
|
||||
needs_metadata: true,
|
||||
full_resolution: false,
|
||||
};
|
||||
let rt = crate::net_runtime::build().unwrap();
|
||||
let outcome = rt.block_on(read_metadata_only(
|
||||
&backend,
|
||||
dr_decode::default(),
|
||||
&req,
|
||||
&mut Vec::new(),
|
||||
));
|
||||
(
|
||||
outcome,
|
||||
backend.gets.load(std::sync::atomic::Ordering::SeqCst),
|
||||
)
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_date_read_against_a_dead_server_asks_once_and_says_so() {
|
||||
// Each attempt waited out a 15 s connect timeout, three per image, for
|
||||
// every cached-but-undated cell in the window — minutes of "reading
|
||||
// dates" with no banner, because the caller could not tell a dead
|
||||
// server from a missing file.
|
||||
let (outcome, gets) =
|
||||
read_date_against(|| dr_sync::RemoteError::Network("connection refused".into()));
|
||||
|
||||
assert!(matches!(outcome, DateRead::Offline(_)), "{outcome:?}");
|
||||
assert_eq!(gets, 1, "retrying a dead server buys another timeout");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_date_read_that_hits_a_lock_still_retries() {
|
||||
// The reason the retry exists: Nextcloud answers a read with 423 under
|
||||
// our own concurrency, and the same range succeeds moments later.
|
||||
let (outcome, gets) = read_date_against(|| dr_sync::RemoteError::Server {
|
||||
status: 423,
|
||||
detail: "locked".into(),
|
||||
});
|
||||
|
||||
assert_eq!(outcome, DateRead::Failed);
|
||||
assert_eq!(gets, 3);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_header_gives_the_size_the_photograph_is_seen_at() {
|
||||
let mut md = dr_decode::Metadata {
|
||||
|
||||
@@ -9,7 +9,7 @@ use dr_thumbs::ThumbStore;
|
||||
use std::path::PathBuf;
|
||||
use std::sync::mpsc::Receiver;
|
||||
|
||||
use super::sweep::{flush_metadata, read_metadata_only, MetadataFound};
|
||||
use super::sweep::{flush_metadata, read_metadata_only, DateRead, MetadataFound};
|
||||
#[cfg(test)]
|
||||
use super::sweep::{thumbnails_outstanding, SWEEP_THUMB_SIZE};
|
||||
use super::thumbnails_fetch::{ThumbnailRequest, MAX_PREVIEW_BYTES};
|
||||
@@ -226,7 +226,14 @@ pub fn spawn_thumbnails(
|
||||
if tx.send(ThumbnailMessage::DateProgress).is_err() {
|
||||
break;
|
||||
}
|
||||
read_metadata_only(&*backend, decoder, &req, &mut found).await;
|
||||
if let DateRead::Offline(reason) =
|
||||
read_metadata_only(&*backend, decoder, &req, &mut found).await
|
||||
{
|
||||
// Said once, as the fetch loop does, so the banner goes
|
||||
// up and the bar stops rather than sweeping for ever.
|
||||
let _ = tx.send(ThumbnailMessage::Offline { reason });
|
||||
break;
|
||||
}
|
||||
|
||||
if found.len() >= FLUSH_EVERY {
|
||||
flush_metadata(&catalog_path, &mut found, &tx);
|
||||
|
||||
Reference in New Issue
Block a user