diff --git a/crates/pdfs-cli/src/main.rs b/crates/pdfs-cli/src/main.rs index 226329c..947e15f 100644 --- a/crates/pdfs-cli/src/main.rs +++ b/crates/pdfs-cli/src/main.rs @@ -310,6 +310,11 @@ enum Command { /// What to refresh: a folder path (inside the mountpoint or relative to /// it), `trash`, or `photos`. The mount root if omitted. target: Option, + /// With `photos`: read every photo's metadata again instead of only what + /// changed. Minutes on a large library; for a gallery that disagrees with + /// the account. + #[arg(long)] + full: bool, }, /// Manage the account's registered devices. @@ -683,7 +688,7 @@ fn main() -> Result<()> { Command::Restore { uids } => cmd_restore(uids), Command::DeleteForever { uids } => cmd_delete_forever(uids), Command::EmptyTrash => cmd_empty_trash(), - Command::Refresh { target } => cmd_refresh(target), + Command::Refresh { target, full } => cmd_refresh(target, full), Command::Devices { action } => cmd_devices(action), Command::Sync { action } => cmd_sync(action), Command::Share { @@ -2269,15 +2274,15 @@ fn cmd_empty_trash() -> Result<()> { /// Drop a cached listing. `trash` and `photos` name those two listings; anything /// else is read as a folder path, so a folder actually called "trash" is still /// reachable as `./trash`. -fn cmd_refresh(target: Option) -> Result<()> { +fn cmd_refresh(target: Option, full: bool) -> Result<()> { let scope = match target.as_deref() { Some("trash") => RefreshScope::Trash, - Some("photos") => RefreshScope::Photos, + Some("photos") => RefreshScope::Photos { full }, path => RefreshScope::Dir { path: path.unwrap_or("").to_string(), }, }; - let photos = scope == RefreshScope::Photos; + let photos = matches!(scope, RefreshScope::Photos { .. }); match control_request(CtlRequest::Refresh { scope })? { CtlResponse::Ok { message } => println!("{message}"), CtlResponse::Error { message, kind } => bail!("{}", cli_error(kind, &message)), diff --git a/crates/pdfs-core/src/control.rs b/crates/pdfs-core/src/control.rs index db60988..a79d08f 100644 --- a/crates/pdfs-core/src/control.rs +++ b/crates/pdfs-core/src/control.rs @@ -592,7 +592,15 @@ pub enum RefreshScope { /// *started*, not when it has finished: re-reading a large library takes /// minutes, which is far longer than a control reply may take. Poll /// [`Request::PhotosRefreshStatus`] to know when the new timeline is there. - Photos, + /// + /// A refresh normally only reads the photos the daemon has never read and + /// the ones a remote event marked changed. `full` drops that knowledge and + /// reads every photo's node again — the repair for a timeline that somehow + /// disagrees with the account, and minutes of work on a large library. + Photos { + #[serde(default)] + full: bool, + }, } /// A registered device in a [`Response::Devices`] listing. @@ -2077,6 +2085,16 @@ mod tests { assert_eq!(line, serde_json::to_string(&decoded).unwrap()); } + /// `full` defaults, so a front-end built against the older wire shape still + /// asks for the ordinary refresh rather than failing to parse. + #[test] + fn a_photos_refresh_scope_defaults_to_not_full() { + let scope: RefreshScope = serde_json::from_str(r#"{"Photos":{}}"#).unwrap(); + assert_eq!(scope, RefreshScope::Photos { full: false }); + let line = serde_json::to_string(&RefreshScope::Photos { full: true }).unwrap(); + assert_eq!(line, r#"{"Photos":{"full":true}}"#); + } + /// A photos refresh is followed, not awaited, so the request that reports /// its progress is part of the wire contract. #[test] @@ -2262,7 +2280,10 @@ mod tests { scope: RefreshScope::Trash, }, Request::Refresh { - scope: RefreshScope::Photos, + scope: RefreshScope::Photos { full: false }, + }, + Request::Refresh { + scope: RefreshScope::Photos { full: true }, }, ]; for req in reqs { diff --git a/crates/pdfs-core/src/db/migrations.rs b/crates/pdfs-core/src/db/migrations.rs index f01db7f..b649ec4 100644 --- a/crates/pdfs-core/src/db/migrations.rs +++ b/crates/pdfs-core/src/db/migrations.rs @@ -9,7 +9,7 @@ use super::Db; use crate::Result; /// Current schema version. Bump on every forward migration added below. -pub(super) const SCHEMA_VERSION: i64 = 30; +pub(super) const SCHEMA_VERSION: i64 = 31; impl Db { pub(super) fn migrate(&self) -> Result<()> { @@ -254,6 +254,22 @@ impl Db { tx.execute_batch(MIGRATION_V30)?; } } + if current < 31 { + // Same guards as V26-V30. + let has_column: bool = tx.query_row( + "SELECT COUNT(*) FROM pragma_table_info('photos') WHERE name = 'resolved_at'", + [], + |row| row.get::<_, i64>(0), + )? > 0; + let has_photos: bool = tx.query_row( + "SELECT COUNT(*) FROM sqlite_master WHERE type = 'table' AND name = 'photos'", + [], + |row| row.get::<_, i64>(0), + )? > 0; + if has_photos && !has_column { + tx.execute_batch(MIGRATION_V31)?; + } + } tx.execute( "INSERT INTO sync_state (key, value) VALUES ('schema_version', ?1) ON CONFLICT(key) DO UPDATE SET value = excluded.value", @@ -918,3 +934,21 @@ ALTER TABLE photos ADD COLUMN main_uid TEXT; ALTER TABLE photos ADD COLUMN group_key TEXT; CREATE INDEX IF NOT EXISTS idx_photos_group ON photos(group_key); "; + +/// Schema v31: which photos a refresh still has to read from the server. +/// +/// A refresh used to resolve every photo's node to learn its name, media type, +/// favourite tag, content hash and photo relation — minutes of round-trips for a +/// library of any size, repeated every time the timeline went stale. `resolved_at` +/// records that the work was already done, so a refresh only reads the photos it +/// has never read and the ones a remote event marked stale (`resolved_at = NULL`). +/// +/// The backfill treats an existing timeline as resolved, so an upgrade does not +/// re-read the whole library once. `WHERE name IS NOT NULL` keeps the photos +/// whose earlier resolve had failed eligible: a nameless row is one nothing was +/// ever learned about. +const MIGRATION_V31: &str = " +ALTER TABLE photos ADD COLUMN resolved_at INTEGER; +CREATE INDEX IF NOT EXISTS idx_photos_unresolved ON photos(resolved_at); +UPDATE photos SET resolved_at = strftime('%s','now') WHERE name IS NOT NULL; +"; diff --git a/crates/pdfs-core/src/db/photos.rs b/crates/pdfs-core/src/db/photos.rs index 09dfe8d..956f8b9 100644 --- a/crates/pdfs-core/src/db/photos.rs +++ b/crates/pdfs-core/src/db/photos.rs @@ -58,6 +58,11 @@ pub struct TimelineRow { /// The main photo this one is *related* to (a live-photo video, a burst /// sibling), when the server says it belongs to one. pub main_uid: Option, + /// When this refresh resolved the photo's node, if it did at all. `Some` + /// makes every other field authoritative — including a `None` that means + /// "the server no longer reports one" — while `None` means the refresh + /// skipped this photo and whatever is stored must be kept. + pub resolved_at: Option, } impl TimelineRow { @@ -74,12 +79,16 @@ impl TimelineRow { /// What a previous refresh learned about a photo and a new one must not lose. #[derive(Clone, Debug, Default)] struct Learned { + name: Option, ratio: Option, thumb_state: i64, media_type: Option, favorite: bool, content_hash: Option, main_uid: Option, + /// When this photo's node was last resolved, kept so a refresh that skips it + /// does not make it look unresolved again. + resolved_at: Option, } /// The local calendar day a capture time falls on, as a `YYYY-MM-DD` string. @@ -253,35 +262,39 @@ fn stored_photo(r: &rusqlite::Row<'_>) -> rusqlite::Result { } impl Db { - /// Replace the timeline wholesale. `favorite`, `media_type`, `content_hash` - /// and `main_uid` are `None` when the refresh could not resolve that photo's - /// node, in which case what is already stored is kept rather than silently - /// cleared. + /// Replace the timeline wholesale. A row whose `resolved_at` is `Some` was + /// read from the server in this pass, so its fields are taken as they are — + /// a `None` there means the server no longer reports that value. A row whose + /// `resolved_at` is `None` was skipped or failed to resolve, and keeps + /// whatever is already stored rather than having it silently cleared. pub fn photos_replace(&self, items: &[TimelineRow]) -> Result<()> { let mut conn = self.conn.lock(); let tx = conn.transaction()?; - // `media_type` is learned-and-kept like the ratio and thumb verdict: the - // timeline DTO carries only the uid and capture time, so the daemon may - // not know a photo's media type yet when it replaces the timeline. Keep - // any previously learned value so the Photos/Videos/Raw split survives a - // refresh instead of collapsing back to name-extension guesses. The photo - // relation is kept for the same reason: losing it would break a group up - // until the next successful resolve. + // `name` and `media_type` are learned-and-kept like the ratio and thumb + // verdict: the timeline DTO carries only the uid and capture time, so the + // daemon may not know them when it replaces the timeline — because the + // refresh skipped an already-resolved photo, or because resolving it + // failed. Keep any previously learned value so the tab split and the + // RAW+JPEG grouping survive a refresh instead of collapsing to nothing: + // both are derived from the name. The photo relation is kept for the same + // reason — losing it would break a group up until the next resolve. let learned: HashMap = { let mut stmt = tx.prepare( - "SELECT uid, ratio, thumb_state, media_type, favorite, content_hash, main_uid \ - FROM photos", + "SELECT uid, name, ratio, thumb_state, media_type, favorite, content_hash, \ + main_uid, resolved_at FROM photos", )?; let rows = stmt.query_map([], |r| { Ok(( r.get::<_, String>(0)?, Learned { - ratio: r.get(1)?, - thumb_state: r.get(2)?, - media_type: r.get(3)?, - favorite: r.get::<_, i64>(4)? != 0, - content_hash: r.get(5)?, - main_uid: r.get(6)?, + name: r.get(1)?, + ratio: r.get(2)?, + thumb_state: r.get(3)?, + media_type: r.get(4)?, + favorite: r.get::<_, i64>(5)? != 0, + content_hash: r.get(6)?, + main_uid: r.get(7)?, + resolved_at: r.get(8)?, }, )) })?; @@ -296,15 +309,29 @@ impl Db { thumb_state: THUMB_UNKNOWN, ..Learned::default() }); - carried.media_type = row.media_type.clone().or(carried.media_type); - carried.favorite = row.favorite.unwrap_or(carried.favorite); - carried.content_hash = row.content_hash.clone().or(carried.content_hash); - carried.main_uid = row.main_uid.clone().or(carried.main_uid); + if row.resolved_at.is_some() { + // This pass read the node itself, so what it says is the + // truth — including what it no longer says. A photo unlinked + // from its main on another device loses its `main_uid` here, + // which a carry could never express. + carried.name = row.name.clone(); + carried.media_type = row.media_type.clone(); + carried.favorite = row.favorite.unwrap_or(false); + carried.content_hash = row.content_hash.clone(); + carried.main_uid = row.main_uid.clone(); + } else { + carried.name = row.name.clone().or(carried.name); + carried.media_type = row.media_type.clone().or(carried.media_type); + carried.favorite = row.favorite.unwrap_or(carried.favorite); + carried.content_hash = row.content_hash.clone().or(carried.content_hash); + carried.main_uid = row.main_uid.clone().or(carried.main_uid); + } + carried.resolved_at = row.resolved_at.or(carried.resolved_at); // The tab this photo lands in is derived here, once, so a page or // count query is a plain indexed `WHERE kind = ?` rather than a // reclassification of every row. let kind = crate::control::PhotoKind::classify( - row.name.as_deref(), + carried.name.as_deref(), carried.media_type.as_deref(), ); (row, carried, kind) @@ -318,7 +345,7 @@ impl Db { kind: *kind, main_uid: carried.main_uid.clone(), day: capture_day(&tx, row.capture_time), - stem: name_stem(row.name.as_deref()), + stem: name_stem(carried.name.as_deref()), }) .collect(); let group_keys = group_photos(&entries); @@ -328,8 +355,8 @@ impl Db { let mut stmt = tx.prepare( "INSERT INTO photos (uid, capture_time, name, ratio, thumb_state, seq, media_type, kind, favorite, - content_hash, main_uid, group_key) - VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10, ?11, ?12)", + content_hash, main_uid, group_key, resolved_at) + VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10, ?11, ?12, ?13)", )?; for (seq, ((row, carried, kind), group_key)) in resolved.iter().zip(group_keys.iter()).enumerate() @@ -337,7 +364,7 @@ impl Db { stmt.execute(params![ row.uid, row.capture_time, - row.name, + carried.name, carried.ratio, carried.thumb_state, seq as i64, @@ -347,6 +374,7 @@ impl Db { carried.content_hash, carried.main_uid, group_key, + carried.resolved_at, ])?; } } @@ -354,6 +382,50 @@ impl Db { Ok(()) } + /// The uids whose node metadata is already resolved, so a refresh can skip + /// reading their nodes again. Everything else — a photo never seen before, or + /// one a remote event marked stale — is resolved. That is what keeps a + /// 12k-photo library off 63 round-trips of `enumerate_nodes` every refresh. + pub fn photos_resolved(&self) -> Result> { + let conn = self.read(); + let mut stmt = conn.prepare("SELECT uid FROM photos WHERE resolved_at IS NOT NULL")?; + let rows = stmt.query_map([], |r| r.get::<_, String>(0))?; + let mut uids = Vec::new(); + for row in rows { + uids.push(row?); + } + Ok(uids) + } + + /// Mark photos as needing a fresh metadata resolve, and report how many rows + /// that matched. This is how a remote change is recorded: the event feed says + /// *which* node changed but never *what* changed about it, so the uid is + /// written down and the next refresh reads the node. + pub fn photos_mark_unresolved(&self, uids: &[String]) -> Result { + if uids.is_empty() { + return Ok(0); + } + let mut conn = self.conn.lock(); + let tx = conn.transaction()?; + let mut marked = 0; + { + let mut stmt = tx.prepare("UPDATE photos SET resolved_at = NULL WHERE uid = ?1")?; + for uid in uids { + marked += stmt.execute(params![uid])?; + } + } + tx.commit()?; + Ok(marked) + } + + /// The same for the whole timeline: every photo is resolved again on the next + /// refresh. For a continuity loss, where the daemon cannot know what it + /// missed, and for an explicit `refresh photos --full`. + pub fn photos_mark_all_unresolved(&self) -> Result { + let conn = self.conn.lock(); + Ok(conn.execute("UPDATE photos SET resolved_at = NULL", [])?) + } + /// Forget photos that have been trashed, so the gallery does not show them /// until the next timeline refresh would have caught up. /// diff --git a/crates/pdfs-core/src/db/tests.rs b/crates/pdfs-core/src/db/tests.rs index 68e80b0..f885e6c 100644 --- a/crates/pdfs-core/src/db/tests.rs +++ b/crates/pdfs-core/src/db/tests.rs @@ -4217,3 +4217,215 @@ fn migration_v30_leaves_a_v29_timeline_intact() { let _ = std::fs::remove_file(&path); let _ = std::fs::remove_file(format!("{}.lock", path.display())); } + +/// A refresh that skips an already-resolved photo sends a row carrying only the +/// uid and capture time. The name must survive that, because the tab split and +/// the grouping are both derived from it. +#[test] +fn photos_replace_keeps_a_skipped_photos_name() { + let db = Db::open_in_memory().unwrap(); + db.photos_replace(&[ + TimelineRow { + name: Some("IMG_1234.JPG".into()), + media_type: Some("image/jpeg".into()), + resolved_at: Some(1), + ..TimelineRow::new("jpeg", 300) + }, + TimelineRow { + name: Some("IMG_1234.CR2".into()), + media_type: Some("application/octet-stream".into()), + resolved_at: Some(1), + ..TimelineRow::new("raw", 300) + }, + ]) + .unwrap(); + let page = db.photos_page(0, 10, None, None, false).unwrap(); + assert_eq!(page.len(), 1, "one shot, two files"); + assert!(page[0].has_raw); + + // The next refresh resolves neither, because both are already resolved. + db.photos_replace(&[TimelineRow::new("jpeg", 300), TimelineRow::new("raw", 300)]) + .unwrap(); + let page = db.photos_page(0, 10, None, None, false).unwrap(); + assert_eq!(page.len(), 1, "the group survives a skipped refresh"); + assert_eq!(page[0].name.as_deref(), Some("IMG_1234.JPG")); + assert!(page[0].has_raw, "the raw file is still a raw file"); + assert_eq!(page[0].group_size, 2); +} + +/// Only the photos a refresh actually read are marked resolved; the rest stay +/// eligible, so the next refresh tries them again. +#[test] +fn photos_replace_marks_only_the_rows_it_resolved() { + let db = Db::open_in_memory().unwrap(); + db.photos_replace(&[ + TimelineRow { + name: Some("ok.jpg".into()), + resolved_at: Some(1), + ..TimelineRow::new("ok", 300) + }, + // The chunk holding this one failed. + TimelineRow::new("failed", 200), + ]) + .unwrap(); + assert_eq!(db.photos_resolved().unwrap(), ["ok"]); + + // A resolved photo stays resolved across a refresh that skips it. + db.photos_replace(&[ + TimelineRow::new("ok", 300), + TimelineRow { + name: Some("late.jpg".into()), + resolved_at: Some(2), + ..TimelineRow::new("failed", 200) + }, + ]) + .unwrap(); + let mut resolved = db.photos_resolved().unwrap(); + resolved.sort(); + assert_eq!(resolved, ["failed", "ok"]); +} + +/// What a refresh read is the truth, including what it no longer reports: a +/// photo unlinked from its main on another device leaves the group. +#[test] +fn a_resolved_replace_clears_a_relation_the_server_dropped() { + let db = Db::open_in_memory().unwrap(); + db.photos_replace(&[ + TimelineRow { + name: Some("IMG_0001.HEIC".into()), + resolved_at: Some(1), + ..TimelineRow::new("main", 300) + }, + TimelineRow { + name: Some("clip.mov".into()), + main_uid: Some("main".into()), + resolved_at: Some(1), + ..TimelineRow::new("related", 299) + }, + ]) + .unwrap(); + assert_eq!(db.photos_group("related").unwrap().len(), 2); + + // Both are read again and the server no longer reports the relation. + db.photos_replace(&[ + TimelineRow { + name: Some("IMG_0001.HEIC".into()), + resolved_at: Some(2), + ..TimelineRow::new("main", 300) + }, + TimelineRow { + name: Some("clip.mov".into()), + resolved_at: Some(2), + ..TimelineRow::new("related", 299) + }, + ]) + .unwrap(); + assert_eq!( + db.photos_group("related").unwrap().len(), + 1, + "an unlinked photo stands on its own" + ); +} + +/// A refresh that did *not* read the nodes must not break the group up, which is +/// the older behaviour and still the one a failed resolve gets. +#[test] +fn an_unresolved_replace_keeps_a_relation() { + let db = Db::open_in_memory().unwrap(); + db.photos_replace(&[ + TimelineRow { + name: Some("IMG_0001.HEIC".into()), + resolved_at: Some(1), + ..TimelineRow::new("main", 300) + }, + TimelineRow { + name: Some("clip.mov".into()), + main_uid: Some("main".into()), + resolved_at: Some(1), + ..TimelineRow::new("related", 299) + }, + ]) + .unwrap(); + + db.photos_replace(&[ + TimelineRow::new("main", 300), + TimelineRow::new("related", 299), + ]) + .unwrap(); + assert_eq!(db.photos_group("related").unwrap().len(), 2); +} + +/// A remote event names the photo it changed, and that is all it takes to have +/// the next refresh read that one node again. +#[test] +fn marking_a_photo_unresolved_makes_it_eligible_again() { + let db = Db::open_in_memory().unwrap(); + db.photos_replace(&[ + TimelineRow { + name: Some("a.jpg".into()), + resolved_at: Some(1), + ..TimelineRow::new("a", 300) + }, + TimelineRow { + name: Some("b.jpg".into()), + resolved_at: Some(1), + ..TimelineRow::new("b", 200) + }, + ]) + .unwrap(); + + assert_eq!(db.photos_mark_unresolved(&["a".to_string()]).unwrap(), 1); + assert_eq!(db.photos_resolved().unwrap(), ["b"]); + // A uid the timeline does not hold changes nothing. + assert_eq!(db.photos_mark_unresolved(&["gone".to_string()]).unwrap(), 0); + + assert_eq!(db.photos_mark_all_unresolved().unwrap(), 2); + assert!(db.photos_resolved().unwrap().is_empty()); +} + +/// An existing timeline opens as resolved, so upgrading does not re-read the +/// whole library once — except for the photos nothing was ever learned about. +#[test] +fn migration_v31_treats_an_existing_timeline_as_resolved() { + let path = std::env::temp_dir().join(format!( + "pdfs-db-v30-fixture-{}-{}.db", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + { + let db = Db::open(&path).unwrap(); + db.photos_replace(&[ + TimelineRow { + name: Some("IMG_1234.JPG".into()), + resolved_at: Some(1), + ..TimelineRow::new("named", 300) + }, + // A photo whose resolve had failed: nothing was ever learned. + TimelineRow::new("nameless", 200), + ]) + .unwrap(); + } + { + // Put the file back in the state a released V30 database was in. + let conn = rusqlite::Connection::open(&path).unwrap(); + conn.execute_batch( + "DROP INDEX idx_photos_unresolved; + ALTER TABLE photos DROP COLUMN resolved_at; + UPDATE sync_state SET value = '30' WHERE key = 'schema_version';", + ) + .unwrap(); + } + + let db = Db::open(&path).unwrap(); + assert_eq!( + db.photos_resolved().unwrap(), + ["named"], + "the stored timeline is trusted, a nameless row is not" + ); + + let _ = std::fs::remove_file(&path); + let _ = std::fs::remove_file(format!("{}.lock", path.display())); +} diff --git a/crates/pdfs-fuse/src/background.rs b/crates/pdfs-fuse/src/background.rs index 7d4ddfe..1509076 100644 --- a/crates/pdfs-fuse/src/background.rs +++ b/crates/pdfs-fuse/src/background.rs @@ -489,36 +489,73 @@ pub(super) async fn run_photos_event_sync(core: Core) { } } -/// Apply one batch of photos-volume events to the gallery's tables. -fn apply_photo_events(core: &Core, events: &[DriveEvent]) { - let mut gone: Vec = Vec::new(); - let mut refresh = false; +/// What one batch of photos-volume events asks the gallery's tables to do. +#[derive(Debug, Default, PartialEq)] +pub(super) struct PhotoEventPlan { + /// Photos that left the volume, by delete or by trash. + pub(super) gone: Vec, + /// Photos whose node changed, so their stored metadata is no longer trusted. + pub(super) touched: Vec, + /// Whether the whole timeline has to be read again. + pub(super) full: bool, +} + +impl PhotoEventPlan { + fn is_empty(&self) -> bool { + self.gone.is_empty() && self.touched.is_empty() && !self.full + } +} + +/// Read one batch of photos-volume events. Pure, so the classification is +/// testable without a volume behind it. +/// +/// `is_echo` claims an event as this daemon's own write. The feed replays our +/// writes back at us and a `NodeUpdated` carries no revision id, so without this +/// every favourite the user toggles here would mark its own photo stale and buy +/// a resolve of a node we just wrote. +fn classify_photo_events( + events: &[DriveEvent], + mut is_echo: impl FnMut(&NodeUid) -> bool, +) -> PhotoEventPlan { + let mut plan = PhotoEventPlan::default(); for event in events { match event { - DriveEvent::NodeDeleted { node_uid, .. } => gone.push(node_uid.to_string()), + DriveEvent::NodeDeleted { node_uid, .. } => plan.gone.push(node_uid.to_string()), DriveEvent::NodeUpdated { node_uid, is_trashed, .. } => { if *is_trashed { - gone.push(node_uid.to_string()); - } else { - // An untrash, a rename, a new upload from the phone: all of - // them need the timeline itself, which only a refresh has. - refresh = true; + plan.gone.push(node_uid.to_string()); + } else if !is_echo(node_uid) { + // An untrash, a rename, a favourite toggled on the phone: the + // event says which node changed but never what about it, so + // the uid is marked stale and the next refresh reads it. + plan.touched.push(node_uid.to_string()); } } - // Nothing here can be applied event by event, so the whole timeline - // is re-read instead. + // The daemon cannot know what it missed here, so everything it + // thinks it knows about every photo is dropped. DriveEvent::ContinuityLost { .. } | DriveEvent::ScopeAccessLost { .. } => { - refresh = true; + plan.full = true; } DriveEvent::CursorAdvanced { .. } | DriveEvent::SharedWithMeUpdated { .. } => {} } } + plan +} + +/// Apply one batch of photos-volume events to the gallery's tables. +fn apply_photo_events(core: &Core, events: &[DriveEvent]) { + let plan = classify_photo_events(events, |uid| core.take_self_change(uid)); + let PhotoEventPlan { + gone, + touched, + full, + } = &plan; if !gone.is_empty() { - match core.db.photos_delete(&gone) { + match core.db.photos_delete(gone) { Ok(removed) if removed > 0 => { debug!(removed, "photos removed by a remote event"); } @@ -526,7 +563,22 @@ fn apply_photo_events(core: &Core, events: &[DriveEvent]) { Err(error) => warn!(%error, "removing remotely deleted photos failed"), } } - if refresh || !gone.is_empty() { + // A stale mark is what makes the next refresh cheap: it reads these photos' + // nodes and carries every other photo's metadata as it stands. + if !touched.is_empty() { + match core.db.photos_mark_unresolved(touched) { + Ok(marked) if marked > 0 => debug!(marked, "photos changed by a remote event"), + Ok(_) => {} + Err(error) => warn!(%error, "marking remotely changed photos failed"), + } + } + if *full { + match core.db.photos_mark_all_unresolved() { + Ok(marked) => debug!(marked, "the photo event stream lost continuity"), + Err(error) => warn!(%error, "marking the whole timeline unresolved failed"), + } + } + if !plan.is_empty() { core.invalidate_photos(); core.invalidate_albums(); } @@ -901,4 +953,96 @@ mod tests { assert!(acknowledge_applied_event(&db, &event, applied).is_err()); assert!(db.get_event_cursor().unwrap().is_none()); } + + /// A photo trashed or deleted elsewhere leaves the gallery; one that only + /// changed is marked stale, so the next refresh reads that node and no other. + #[test] + fn a_photo_event_names_what_it_changed() { + let plan = classify_photo_events( + &[ + DriveEvent::NodeDeleted { + id: DriveEventId::from("e1"), + node_uid: uid("deleted"), + parent_node_uid: None, + }, + DriveEvent::NodeUpdated { + id: DriveEventId::from("e2"), + node_uid: uid("trashed"), + parent_node_uid: None, + is_trashed: true, + is_shared: false, + }, + DriveEvent::NodeUpdated { + id: DriveEventId::from("e3"), + node_uid: uid("favourited"), + parent_node_uid: None, + is_trashed: false, + is_shared: false, + }, + ], + |_| false, + ); + assert_eq!( + plan, + PhotoEventPlan { + gone: vec![uid("deleted").to_string(), uid("trashed").to_string()], + touched: vec![uid("favourited").to_string()], + full: false, + } + ); + } + + /// The feed replays this daemon's own writes. Applying one would cost a + /// resolve of a node whose changed field the daemon just wrote itself. + #[test] + fn our_own_photo_write_is_not_a_remote_change() { + let plan = classify_photo_events( + &[DriveEvent::NodeUpdated { + id: DriveEventId::from("e1"), + node_uid: uid("ours"), + parent_node_uid: None, + is_trashed: false, + is_shared: false, + }], + |claimed| *claimed == uid("ours"), + ); + assert_eq!(plan, PhotoEventPlan::default()); + } + + /// A gap in the stream means the daemon cannot know what it missed, so it + /// keeps nothing. + #[test] + fn a_lost_photo_event_stream_asks_for_everything() { + for event in [ + DriveEvent::ContinuityLost { + id: DriveEventId::from("e1"), + }, + DriveEvent::ScopeAccessLost { + id: DriveEventId::from("e1"), + }, + ] { + let plan = classify_photo_events(std::slice::from_ref(&event), |_| false); + assert!(plan.full, "{event:?}"); + assert!(plan.gone.is_empty()); + assert!(plan.touched.is_empty()); + } + } + + /// Bookkeeping events change nothing about the gallery, and must not buy a + /// refresh of it. + #[test] + fn photo_bookkeeping_events_ask_for_nothing() { + let plan = classify_photo_events( + &[ + DriveEvent::CursorAdvanced { + id: DriveEventId::from("e1"), + }, + DriveEvent::SharedWithMeUpdated { + id: DriveEventId::from("e2"), + }, + ], + |_| false, + ); + assert_eq!(plan, PhotoEventPlan::default()); + } } diff --git a/crates/pdfs-fuse/src/control.rs b/crates/pdfs-fuse/src/control.rs index 4fb02ee..cd687f6 100644 --- a/crates/pdfs-fuse/src/control.rs +++ b/crates/pdfs-fuse/src/control.rs @@ -255,8 +255,13 @@ fn handle_control_conn(core: &Core, username: &str, mountpoint: &Path, stream: U core.invalidate_trash(); Ok(()) } - RefreshScope::Photos => { + RefreshScope::Photos { full } => { core.invalidate_photos(); + // A full refresh is "forget what you know", not a second code + // path: the ordinary refresh then finds every photo stale. + if let Some(Err(e)) = full.then(|| core.db.photos_mark_all_unresolved()) { + warn!(error = %e, "marking the whole timeline unresolved failed"); + } // Whatever changed the timeline can just as easily have // changed an album; one scope covers the whole photos view. core.invalidate_albums(); @@ -274,7 +279,7 @@ fn handle_control_conn(core: &Core, username: &str, mountpoint: &Path, stream: U match result { Ok(()) => CtlResponse::Ok { message: match scope { - RefreshScope::Photos => "refreshing".to_string(), + RefreshScope::Photos { .. } => "refreshing".to_string(), _ => "refreshed".to_string(), }, }, diff --git a/crates/pdfs-fuse/src/lib.rs b/crates/pdfs-fuse/src/lib.rs index d7a48ae..7cd201a 100644 --- a/crates/pdfs-fuse/src/lib.rs +++ b/crates/pdfs-fuse/src/lib.rs @@ -213,7 +213,10 @@ const TIMELINE_TTL: Duration = Duration::from_secs(60); /// Photos / Videos / Raw split). Batched so a large library is a handful of /// round-trips rather than one request per photo, and bounded so a single call /// never asks the server to decrypt the whole library at once. -const TIMELINE_ENRICH_CHUNK: usize = 200; +/// +/// 150 because that is the SDK's own `MAX_BATCH_COUNT`: a larger chunk is split +/// there anyway, into one full request and a ragged tail. +const TIMELINE_ENRICH_CHUNK: usize = 150; /// The same, for the persisted trash listing. Shorter, because the trash is the /// one listing a user changes and then immediately looks at — though our own /// mutations also invalidate it outright, so this only covers other clients. diff --git a/crates/pdfs-fuse/src/photos.rs b/crates/pdfs-fuse/src/photos.rs index 9ad453a..c5689f2 100644 --- a/crates/pdfs-fuse/src/photos.rs +++ b/crates/pdfs-fuse/src/photos.rs @@ -499,6 +499,11 @@ impl Core { .map_err(|e| CoreError::from_api(&e, "update photo tags"))?; pdfs_core::batch::into_unit(outcomes) .map_err(|e| CoreError::from_api(&e, "update photo tags"))?; + // The photo event feed replays this write back at us as a bare + // `NodeUpdated`, which is indistinguishable from the same photo being + // favourited on a phone. Claim the echo, or the gallery pays for a + // resolve of a node whose one changed field is already stored below. + self.note_self_change(uid); self.db .photos_set_favorite(&uid.to_string(), favorite) .map_err(CoreError::from)?; @@ -1416,13 +1421,30 @@ impl Core { .await .map_err(|e| CoreError::from_api(&e, "timeline"))?; - // The timeline DTO carries only a uid and capture time, but the Photos - // page has to split into Photos / Videos / Raw — which needs each photo's - // name and media type. Resolve those in batches off the request path. - // Best-effort: a photo whose node we fail to resolve keeps whatever was - // learned before (or classifies from nothing, i.e. a still photo), so a - // partial resolve never blanks the timeline. - let uids: Vec = items.iter().map(|it| it.uid.clone()).collect(); + // The timeline DTO carries only a uid and capture time, but the gallery + // has to split into its tabs — which needs each photo's name and media + // type. Resolve those in batches off the request path. Best-effort: a + // photo whose node we fail to resolve keeps whatever was learned before + // (or classifies from nothing, i.e. a still photo), so a partial resolve + // never blanks the timeline. + // + // Only photos the daemon has never read, and ones a remote event marked + // stale, are read. Resolving all of them every time is what made a + // 12k-photo refresh take minutes (`docs/BUGS.md` B92): this metadata does + // not change on its own, and when it does change the event feed names the + // uid that changed (`Db::photos_mark_unresolved`). + let known: HashSet = self + .db + .photos_resolved() + .map_err(CoreError::from)? + .into_iter() + .collect(); + let uids: Vec = items + .iter() + .filter(|it| !known.contains(&it.uid.to_string())) + .map(|it| it.uid.clone()) + .collect(); + let resolved_at = now_ms(); let mut meta: HashMap = HashMap::new(); // A main photo names its related photos rather than the other way round, // so the relation is recorded from whichever end resolves — the two ends @@ -1482,6 +1504,7 @@ impl Core { favorite: Some(m.favorite), content_hash: m.content_hash, main_uid, + resolved_at: Some(resolved_at), ..db::TimelineRow::new(key, it.capture_time) }, None => db::TimelineRow { @@ -1498,6 +1521,8 @@ impl Core { // like it never happened, and the chunk count is what it scales with. info!( photos = rows.len(), + resolved = uids.len(), + skipped = rows.len().saturating_sub(uids.len()), chunks = uids.len().div_ceil(TIMELINE_ENRICH_CHUNK), elapsed_ms = started.elapsed().as_millis() as u64, "photos timeline refreshed" diff --git a/crates/pdfs-fuse/src/redate.rs b/crates/pdfs-fuse/src/redate.rs index 20d4ff6..b102b46 100644 --- a/crates/pdfs-fuse/src/redate.rs +++ b/crates/pdfs-fuse/src/redate.rs @@ -222,8 +222,13 @@ impl Core { tags_to_add: vec![PhotoTag::Favorite], tags_to_remove: Vec::new(), }; - if let Err(e) = self.rt.block_on(photos.update_photos(&[update])) { - warn!(name = candidate.name, error = %e, "cannot re-favourite re-dated photo"); + match self.rt.block_on(photos.update_photos(&[update])) { + // As in `set_photo_favorite`: claim our own echo, so the tag we + // just set does not come back as a remote change. + Ok(_) => self.note_self_change(&new_uid), + Err(e) => { + warn!(name = candidate.name, error = %e, "cannot re-favourite re-dated photo") + } } } for album in &candidate.albums { diff --git a/crates/pdfs-gui/src/app/main.rs b/crates/pdfs-gui/src/app/main.rs index 880338f..391fee7 100644 --- a/crates/pdfs-gui/src/app/main.rs +++ b/crates/pdfs-gui/src/app/main.rs @@ -848,7 +848,7 @@ fn reload_current_page(ui: &Rc) { } // One scope covers the whole photos view, so Refresh reloads whichever of // the two — album grid or timeline/album — is actually on screen. - Some("gallery") => refresh_then(ui, RefreshScope::Photos, |ui| { + Some("gallery") => refresh_then(ui, RefreshScope::Photos { full: false }, |ui| { if ui.gallery.content.visible_child_name().as_deref() == Some("albums") { load_albums(ui); } else { @@ -886,7 +886,7 @@ fn refresh_then(ui: &Rc, scope: RefreshScope, load: fn(&Rc)) { // A photos refresh answers as soon as it has started, because re-reading a // large library takes minutes. The page is therefore loaded twice: once from // what the daemon already holds, and again when the refresh has landed. - let follow = scope == RefreshScope::Photos; + let follow = matches!(scope, RefreshScope::Photos { .. }); let rx = spawn_request(ui.dirs.control_socket(), Request::Refresh { scope }); let ui = ui.clone(); glib::spawn_future_local(async move { diff --git a/docs/BUGS.md b/docs/BUGS.md index 90eafbb..1788e20 100644 --- a/docs/BUGS.md +++ b/docs/BUGS.md @@ -12,6 +12,32 @@ Conventions: --- +## B93 — A photo whose node would not resolve lost its name, and with it its tab and its group + +**Status:** Fixed (unverified) — found and fixed while making the timeline refresh incremental +(B92); no library with an undecryptable photo in it has been driven against the fix. +**Found:** 2026-09-19, by reading `photos_replace` before making a refresh skip photos it has +already resolved. Skipping one means writing a `TimelineRow` that carries only the uid and the +capture time, which is exactly the row a failed resolve already produced. + +**Where:** `crates/pdfs-core/src/db/photos.rs` (`Learned`, `photos_replace`). + +**Cause.** `photos_replace` keeps what a refresh could not learn — ratio, thumbnail verdict, media +type, favourite flag, content hash, photo relation all survive a `None`. The `name` did not: +`Learned` never held it, the SELECT never read it, and the INSERT bound `row.name` straight +through. So a photo the SDK could not decrypt (it logs and skips the node) had its stored name +overwritten with `NULL` on every refresh. That is not a cosmetic loss — both the tab split and the +grouping are derived from the name. `PhotoKind::classify` is extension-first, so a nameless RAW +file classifies as a still photo and leaves the Raw tab; `name_stem` returns `None`, so the RAW and +the JPEG of one shot stop being one shot; and `redate` skips nameless photos outright. + +**What changed.** `Learned` carries the name like everything else, and the kind and the group stem +are computed from the carried name rather than the incoming row. A row that *was* resolved is now +authoritative instead: its fields are taken as they are, so a photo unlinked from its main on +another device finally loses its `main_uid` — a carry could never express "the server says none". + +--- + ## B92 — Refreshing the gallery reported "Resource temporarily unavailable" **Status:** Fixed (unverified) — the daemon carrying the change has not been driven against a real @@ -43,11 +69,24 @@ the gallery polls it every 2 s and reloads the page when it clears, and `pdfs re polls in the same way so the command still means "the timeline is up to date when I return". `refresh_timeline` now logs `photos`, `chunks` and `elapsed_ms`. -**Still open.** Every refresh re-resolves every photo, whatever changed. The metadata the resolve -learns (name, media type, content hash, photo relation) is already kept across refreshes, so the -pass could be limited to uids that have none — but the favourite tag is read from the same node, -and remote favourite changes arrive as `NodeUpdated` events that only ask for "a refresh". Making -the refresh incremental means deciding where favourites come from first. +**What changed, second pass (2026-09-19).** The refresh is incremental. `photos.resolved_at` +(schema v31) records that a photo's node was read, and `refresh_timeline` resolves only the photos +that have no such mark — the ones it has never read, and the ones a remote event marked stale. +A library that has already been read costs one `enumerate_timeline` and no `enumerate_nodes` call +at all. + +The favourite question resolved by moving it off the refresh and onto the event feed: +`classify_photo_events` turns a `NodeUpdated` into `photos_mark_unresolved(uid)` instead of a +blanket "re-read everything", so a favourite toggled on a phone costs one node read rather than +63. A local toggle is claimed as our own echo (`note_self_change`), because the feed replays it and +a `NodeUpdated` carries nothing that says what changed. `ContinuityLost` / `ScopeAccessLost` and +`pdfs refresh photos --full` drop every mark, which is the one way back to a whole-library read. +`TIMELINE_ENRICH_CHUNK` is 150, matching the SDK's own `MAX_BATCH_COUNT` — a chunk of 200 was split +there into a full request and a 50-uid tail. + +**Still open.** `invalidate_albums()` drives the same pattern per album +(`crates/pdfs-fuse/src/albums.rs:96` and `:191`). It is bounded by album size rather than library +size, so it is nowhere near as expensive, but the technique that fixed this would apply there too. ---