From 6cc8f72bab76331bbebcfd4b78395298ad4c84ad Mon Sep 17 00:00:00 2001 From: Nils Pukropp Date: Sat, 19 Sep 2026 17:39:49 +0200 Subject: [PATCH] fix(core,fuse,gui,cli): read only the photos a refresh has not read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A timeline refresh resolved every photo's node to learn its name, media type, favourite tag, content hash and photo relation — 63 round-trips for a 12,445-photo library, minutes of work, repeated every time the timeline went stale sixty seconds later. None of that metadata changes on its own, and when it does change the event feed names the node it changed. `photos.resolved_at` (schema v31) records that a photo's node was read. The refresh resolves the photos without that mark: the ones it has never read, and the ones a remote event marked stale. An existing timeline is backfilled as resolved, so upgrading does not re-read the library once. Remote changes now name what they changed instead of asking for everything. `classify_photo_events` turns a `NodeUpdated` into `photos_mark_unresolved(uid)`, so a favourite toggled on a phone costs one node read rather than the whole library; a local toggle is claimed as our own echo, because the feed replays it and a `NodeUpdated` carries nothing that says what changed. A continuity loss and `refresh photos --full` drop every mark, which is the way back to a whole-library read. Along the way: `photos_replace` never kept a photo's name, so a photo the SDK could not decrypt was renamed to nothing on every refresh — and the tab split and the RAW+JPEG grouping are both derived from the name. It is kept now, and a row that *was* resolved is authoritative instead, so a photo unlinked from its main elsewhere finally loses its `main_uid`. `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. --- crates/pdfs-cli/src/main.rs | 13 +- crates/pdfs-core/src/control.rs | 25 ++- crates/pdfs-core/src/db/migrations.rs | 36 ++++- crates/pdfs-core/src/db/photos.rs | 128 ++++++++++++---- crates/pdfs-core/src/db/tests.rs | 212 ++++++++++++++++++++++++++ crates/pdfs-fuse/src/background.rs | 174 +++++++++++++++++++-- crates/pdfs-fuse/src/control.rs | 9 +- crates/pdfs-fuse/src/lib.rs | 5 +- crates/pdfs-fuse/src/photos.rs | 39 ++++- crates/pdfs-fuse/src/redate.rs | 9 +- crates/pdfs-gui/src/app/main.rs | 4 +- docs/BUGS.md | 49 +++++- 12 files changed, 634 insertions(+), 69 deletions(-) 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. ---