From 10ec0b52ccc74ba1238265e0d1b3835a980ce5ed Mon Sep 17 00:00:00 2001 From: Greg Pomerantz Date: Mon, 2 Nov 2026 12:37:42 -0500 Subject: [PATCH] gtauri: fix phantom rows, dead deletion detection, 5-min launch stall Root causes found by reproducing each on the emulator (2.8k-file fixture with mixed jpg/mp4): 1. walk_dir_nostats (incremental rescan of >2000-file dirs) had NO image-extension filter: it re-enumerated every file, injecting all ~1,727 videos into the DB. This is what made the count swing 8,439 -> 10,166 and the grid full of blank cells (404 rows). Now filters through is_image_path() like every other walk. 2. walk_dir aborted the ENTIRE walk on any per-file stat failure (file deleted mid-walk), so an error during the walk meant the walk was never applied and deletions were never detected. Per-file stat errors are now skipped; only readdir/root errors fail the walk (an errored walk is still not applied). 3. dir_sig stat'ed every entry of every root dir every watch tick: 10k+ stats/second on the phone = 10s+ of FUSE I/O per second = the long delays. On Android the MediaStore URI payloads already carry the exact change signal, so the mtime-diff machinery now runs desktop-only. The Android watch tick is a single read_dir of our own app dir (no-op when empty): 1s backstop, 30s-throttled fallback full rescan for events with no payload, 5-min full pass. 4. TOCTOU: a delete consumed while a full rescan was mid-walk lost to the walk's stale snapshot (walk re-added the row). RESCANNING gate parks payloads during the walk; they apply after it, so mid-walk deletions always win. Reproduced deterministically (churn during the startup walk), fixed, re-verified. Emulator stress (all passing): full-walk convergence, add +1s, delete +1s, video add/delete = zero rows, 60-op churn converges to exact disk count, 3x relaunch idempotency, startup-walk race converges (1s normally, 5-min backstop when the OS scanner drops burst events), no-blank-cells visual check, 0.00% dropped frames. Phone verified: 10,166 -> 8,439 on first launch (all video phantoms purged), add appears ~2s after the OS indexes it, delete same, DB returns to 8,439 exactly. ContentObserver registration + onChange logging added (it fired reliably once the stale instance was replaced). --- .../main/java/wow/gmp/gallery/MainActivity.kt | 3 + harnesses/gtauri/src/lib.rs | 139 +++++++++++++----- harnesses/gtauri/src/scan.rs | 38 +++-- 3 files changed, 132 insertions(+), 48 deletions(-) diff --git a/harnesses/gtauri/gen/android/app/src/main/java/wow/gmp/gallery/MainActivity.kt b/harnesses/gtauri/gen/android/app/src/main/java/wow/gmp/gallery/MainActivity.kt index 79a640c..c804fba 100644 --- a/harnesses/gtauri/gen/android/app/src/main/java/wow/gmp/gallery/MainActivity.kt +++ b/harnesses/gtauri/gen/android/app/src/main/java/wow/gmp/gallery/MainActivity.kt @@ -164,6 +164,7 @@ class MainActivity : TauriActivity() { // Event-driven: the OS passes the changed URI(s) directly. val obs = object : ContentObserver(mainHandler) { override fun onChange(selfChange: Boolean, uri: Uri?, flags: Int) { + android.util.Log.i("gallery-ms", "onChange uri=$uri self=$selfChange flags=$flags") if (selfChange) return fsExecutor.execute { handleUri(uri) } } @@ -174,6 +175,7 @@ class MainActivity : TauriActivity() { contentResolver.registerContentObserver( MediaStore.Images.Media.EXTERNAL_CONTENT_URI, true, obs ) + android.util.Log.i("gallery-ms", "observer registered in pid " + android.os.Process.myPid()) // Startup catch-up: if the app was killed after a photo was taken, // the observer won't replay — push whatever was indexed recently. mainHandler.postDelayed({ fsExecutor.execute { windowQuery() } }, 1500) @@ -181,6 +183,7 @@ class MainActivity : TauriActivity() { } override fun onDestroy() { + android.util.Log.i("gallery-ms", "onDestroy: unregistering observer") observer?.let { contentResolver.unregisterContentObserver(it) } fsExecutor.shutdownNow() super.onDestroy() diff --git a/harnesses/gtauri/src/lib.rs b/harnesses/gtauri/src/lib.rs index e8cb01a..d953911 100644 --- a/harnesses/gtauri/src/lib.rs +++ b/harnesses/gtauri/src/lib.rs @@ -728,12 +728,34 @@ fn walk_once(app: &tauri::AppHandle) -> std::io::Result> { }) } +/// Set while a full rescan's walk→apply is in flight. The watch loop +/// must NOT consume MediaStore payloads meanwhile: a delete consumed +/// mid-walk would race the walk's in-memory snapshot (the walk would +/// re-add the row the delete already processed). Payloads simply wait +/// as files and are drained by the rescan's post-apply drain. +static RESCANNING: std::sync::atomic::AtomicBool = + std::sync::atomic::AtomicBool::new(false); + +struct RescanGuard; +impl RescanGuard { + fn new() -> Self { + RESCANNING.store(true, std::sync::atomic::Ordering::Relaxed); + Self + } +} +impl Drop for RescanGuard { + fn drop(&mut self) { + RESCANNING.store(false, std::sync::atomic::Ordering::Relaxed); + } +} + /// Full rescan: walk the roots, diff against the DB (insert new / /// update changed / delete removed), refresh META, emit the list. /// Used by the startup scan AND the 5-minute reconciliation pass. /// If the walk fails, nothing is applied — an error is not "those /// files are gone"; the next pass retries. fn rescan(app: &tauri::AppHandle) { + let _guard = RescanGuard::new(); let entries = match walk_once(app) { Ok(e) => e, Err(e) => { @@ -759,6 +781,11 @@ fn rescan(app: &tauri::AppHandle) { } emit_files(app, false, entries.len()); log::info!("rescan: {} files", entries.len()); + // Draining happens in the watch loop's next tick: the RESCANNING + // flag lifted at drop time guarantees any payload consumed from now + // on is applied AFTER this walk's apply, so mid-walk deletions + // always win over the stale snapshot. (The 5-min pass catches any + // straggler in the worst-case millisecond window.) } fn spawn_scan(app: tauri::AppHandle) { @@ -774,9 +801,12 @@ fn mtime_ns(m: &std::fs::Metadata) -> i64 { } /// Tree signature: (dir, mtime) for every directory under the roots. -/// Verified on the phone: Android's FUSE DOES update a directory's mtime -/// when files are added/removed inside it (stat before/after a copy -/// showed the change). Stat-only cost: a few dozen syscalls per tick. +/// Verified on the phone: Android's FUSE DOES update a directory's +/// mtime when files are added/removed inside it (stat before/after a +/// copy showed the change). Cost per tick: one stat per DIRECTORY and +/// one readdir per directory — no per-FILE stat. (The old version +/// called is_dir() on every entry: 10k stats per second on a 10k-file +/// Camera dir, which put a 10 s I/O floor under every event.) fn dir_sig(roots: &[PathBuf]) -> HashMap { let mut out = HashMap::new(); let mut stack: Vec = roots.iter().filter(|r| r.is_dir()).cloned().collect(); @@ -787,7 +817,10 @@ fn dir_sig(roots: &[PathBuf]) -> HashMap { Ok(rd) => { for ent in rd.flatten() { let p = ent.path(); - if p.is_symlink() || !p.is_dir() { + // file_type() uses the d_type from getdents (no + // syscall); is_dir() would stat the entry. + let Ok(ft) = ent.file_type() else { continue }; + if ft.is_symlink() || !ft.is_dir() { continue; } if p.file_name().map_or(false, |n| n.to_string_lossy().starts_with('.')) { @@ -902,11 +935,14 @@ fn rescan_subtrees(app: &tauri::AppHandle, changed: &[PathBuf]) { // A||| row exists → upsert // D| row gone → delete by id // --------------------------------------------------------------------- +/// Consume pending event files. Returns true when at least one change +/// line was applied (the watch loop uses this to decide whether the +/// OS event was fully handled by the payload path). #[cfg(target_os = "android")] -fn apply_mediasotre_payload(app: &tauri::AppHandle) { +fn apply_mediasotre_payload(app: &tauri::AppHandle) -> bool { // Consume every pending .ms_recent.* event file (oldest first). let dir = external_files_dir(); - let Ok(rd) = std::fs::read_dir(&dir) else { return }; + let Ok(rd) = std::fs::read_dir(&dir) else { return false }; let mut files: Vec = rd .flatten() .filter(|e| { @@ -917,9 +953,10 @@ fn apply_mediasotre_payload(app: &tauri::AppHandle) { .map(|e| e.path()) .collect(); if files.is_empty() { - return; + return false; } files.sort(); + let mut applied = false; for f in files { let name = f.file_name().map(|s| s.to_string_lossy().into_owned()).unwrap_or_default(); let Ok(text) = std::fs::read_to_string(&f) else { continue }; @@ -928,20 +965,23 @@ fn apply_mediasotre_payload(app: &tauri::AppHandle) { continue; } tr(&format!("mediastore event {name}: {} lines", text.lines().count())); - mediastore_apply(app, &text); + applied |= mediastore_apply(app, &text); } + applied } #[cfg(not(target_os = "android"))] -fn apply_mediasotre_payload(_app: &tauri::AppHandle) {} +fn apply_mediasotre_payload(_app: &tauri::AppHandle) -> bool { + false +} #[cfg(target_os = "android")] -fn mediastore_apply(app: &tauri::AppHandle, text: &str) { +fn mediastore_apply(app: &tauri::AppHandle, text: &str) -> bool { let mut conn = match db::open(&db::db_path()) { Ok(c) => c, Err(e) => { log::warn!("mediastore db: {e}"); - return; + return false; } }; let mut changed = 0usize; @@ -1038,6 +1078,7 @@ fn mediastore_apply(app: &tauri::AppHandle, text: &str) { } emit_files(app, false, META.read().unwrap().len()); } + changed + removed > 0 } /// Watcher wake events. @@ -1140,7 +1181,13 @@ fn watch_files(app: tauri::AppHandle) { }); #[cfg(not(target_os = "android"))] spawn_fs_watcher(tx); + // Desktop only: the mtime-diff machinery. On Android the MediaStore + // URI payloads ARE the change signal (exact paths, no scanning), + // and diffing would cost a 10k-entry readdir every second on the + // phone — pure overhead for what the payloads already deliver. + #[cfg(not(target_os = "android"))] let mut sig: HashMap = HashMap::new(); + #[cfg(not(target_os = "android"))] let mut baseline = true; let mut last_full = std::time::Instant::now(); loop { @@ -1156,42 +1203,56 @@ fn watch_files(app: tauri::AppHandle) { std::thread::sleep(std::time::Duration::from_millis(600)); while rx.try_recv().is_ok() {} } - // MediaStore event path: MainActivity drops a one-line payload - // per changed URI and wakes us via nativeFsChange. Consume on - // EVERY tick — the check is a single read_dir of our own app - // dir (no-op when empty), so a lost JNI wake self-heals within - // the 1 s backstop instead of waiting for the 5-min pass. - apply_mediasotre_payload(&app); - let next = dir_sig(&scan::roots()); - let changed = if baseline { - baseline = false; - Vec::new() - } else { - changed_dirs(&sig, &next) - }; - sig = next; if do_full { tr("watch: full rescan (5min)"); rescan(&app); last_full = std::time::Instant::now(); continue; } - if changed.is_empty() { - if woke.is_some() { - // The OS reported a change but no dir mtime moved - // (in-place edit / event for an untracked file): - // full rescan, throttled to one per minute. - if last_full.elapsed() >= std::time::Duration::from_secs(60) { - tr("watch: full rescan (event, no dir delta)"); - rescan(&app); - last_full = std::time::Instant::now(); - } - } - // Timeout with no dir delta: nothing to do (~0.05 ms spent). + // MediaStore event path (Android): MainActivity drops a one-line + // payload per changed URI and wakes us via nativeFsChange. + // Consume on EVERY tick (single read_dir of our own app dir — + // no-op when empty, so a lost JNI wake self-heals within the 1 s + // backstop) — but NOT while a full rescan is mid-walk: those + // payloads wait as files and apply after the walk's apply, so + // mid-walk deletions win over the walk's stale snapshot. + let applied_ms = if RESCANNING.load(std::sync::atomic::Ordering::Relaxed) { + false + } else { + apply_mediasotre_payload(&app) + }; + // Android fallback: the OS said "media changed" but no payload + // arrived (permission missing, scanner gap, or the payload was + // parked behind a rescan). Full rescan, throttled — the + // degraded path, not the common one. Parking behind a rescan + // also counts as "not applied": the rescan's next 1 s tick + // re-evaluates and either drains the payload or rescans. + #[cfg(target_os = "android")] + if matches!(woke, Some(WEvent::Fs)) && !applied_ms + && last_full.elapsed() >= std::time::Duration::from_secs(30) + { + tr("watch: full rescan (event, no payload)"); + rescan(&app); + last_full = std::time::Instant::now(); continue; } - tr(&format!("watch: {} changed dir(s)", changed.len())); - rescan_subtrees(&app, &changed); + // Desktop: diff the dir-mtime signature, rescan changed subtrees. + #[cfg(not(target_os = "android"))] + { + let next = dir_sig(&scan::roots()); + let changed = if baseline { + baseline = false; + Vec::new() + } else { + changed_dirs(&sig, &next) + }; + sig = next; + if changed.is_empty() { + continue; + } + tr(&format!("watch: {} changed dir(s)", changed.len())); + rescan_subtrees(&app, &changed); + } } } diff --git a/harnesses/gtauri/src/scan.rs b/harnesses/gtauri/src/scan.rs index 604f4f4..b982bfb 100644 --- a/harnesses/gtauri/src/scan.rs +++ b/harnesses/gtauri/src/scan.rs @@ -3,10 +3,18 @@ //! Plain recursive scan of the roots (Pictures + DCIM on Android). //! FUSE is a reliable infrastructure point: a directory reads either //! returns its full listing or returns an error. There is no silent -//! truncation. Therefore the one invariant here is simple and strict: -//! **any read_dir or stat error fails the walk** — a failed walk must -//! never be applied to the DB, because an error is not "those files -//! are gone." +//! truncation. Error semantics: +//! - a **read_dir error fails the walk** — the result would be +//! incomplete and must never be applied to the DB (an error is not +//! "those files are gone"); +//! - a **per-file failure is skipped** — a file vanishing between +//! readdir and stat is a normal race on an actively used +//! directory (the user deleting while we walk), not a walk +//! failure. Aborting on this killed deletion detection: every +//! incremental rescan of a busy 10k-file directory aborted. +//! Both walks (with-stat and readdir-only) filter to image +//! extensions: library directories hold thousands of videos, and +//! enumerating them would inject non-image rows into the DB. use std::io; use std::path::{Path, PathBuf}; @@ -108,8 +116,13 @@ pub fn walk_dir_into(dir: &Path, out: &mut Vec) -> io::Result<()> { /// Readdir-only walk (NO per-file stat) of ONE directory subtree — for /// huge FUSE directories where stat'ing every file costs ~30 s; the /// caller supplies metadata for new files and keeps DB metadata for -/// known ones. A read_dir error fails the walk; a per-file type -/// failure (file vanishing mid-walk — a normal race) is skipped. +/// known ones. +/// +/// FILTERS TO IMAGE EXTENSIONS (critical: a library directory holds +/// thousands of videos too — enumerating them would inject non-image +/// rows into the DB: phantom blank grid cells and count swings). +/// A read_dir error fails the walk; per-file failures (file vanishing +/// mid-walk — a normal race) are skipped. pub fn walk_dir_nostats(dir: &Path, cb: &mut dyn FnMut(&Path)) -> io::Result<()> { if !dir.is_dir() { return Ok(()); @@ -129,13 +142,20 @@ pub fn walk_dir_nostats(dir: &Path, cb: &mut dyn FnMut(&Path)) -> io::Result<()> } stack.push(p); } else if ft.is_file() { - cb(&p); + let ps = p.to_string_lossy(); + if is_image_path(&ps) { + cb(&p); + } } } } Ok(()) } +/// Full stat walk. Error semantics: a read_dir error fails the walk +/// (the result would be incomplete — must never be applied); per-file +/// failures (a file vanishing between readdir and stat — a normal race +/// on an actively used directory) are SKIPPED, not propagated. fn walk_dir( dir: &Path, out: &mut Vec, @@ -144,7 +164,7 @@ fn walk_dir( for ent in std::fs::read_dir(dir)? { let ent = ent?; let p = ent.path(); - let ft = ent.file_type()?; + let Ok(ft) = ent.file_type() else { continue }; if ft.is_dir() { // Skip hidden dirs (.thumbnails, .trash, …) — metadata, not photos. if p.file_name().and_then(|n| n.to_str()).is_some_and(|n| n.starts_with('.')) { @@ -164,7 +184,7 @@ fn walk_dir( if !is_image_file(name, ext) { continue; } - let (mtime, size) = file_metadata(&p)?; + let Ok((mtime, size)) = file_metadata(&p) else { continue }; out.push(Entry { path: p.to_string_lossy().into_owned(), mtime,