Fix scroll-back-to-top shimmer: prepend-on-submit job queue

Root cause: the worker job queue was FIFO (mpsc). Scrolling down filled it
with slow cold decodes of far cells; scrolling back up re-submitted the top
cells as fast disk-cache hits that then waited behind the whole backlog —
seconds of shimmer. Deterministic repro measured 6.3 s behind a 180-cold
backlog (emulator UI showed the same while 220 cold jobs waited 0.5-0.9 s).

Fix: job queue is now Mutex<VecDeque> + Condvar; Engine::submit prepends
each new batch so the most-recent intent (the viewport, or what you scrolled
back to) is decoded before stale far-away prefetch. Cold backlog keeps
warming the disk cache in the background (total work unchanged).

- regression: thumb::tests::scroll_back_cache_hit_not_starved (6.3 s -> 130 ms)
- dispatcher exits via a closed flag set in Engine::drop (replaces the
  mpsc channel-close shutdown)
- HANDOFF: M3 fix note, prepend rationale, emulator realtest_extra dataset

HANDOFF.md

@@ -10,10 +10,13 @@ rediscovered.
 instant list paint, M2 thumbnail engine (turbojpeg DCT-shrink decode, worker
 pool, disk cache, cancellation), M3 grid UI (3-column ListView grid, shimmer
 placeholders, scroll-driven prefetch, memory-bound cell retention) all
-verified: 13 unit tests, 5k synthetic engine test, on the emulator (150
-real JPEGs: first paint 0.6 s cold, warm relaunch from disk cache, 0 ANRs
-through 16 fast swipes), and on a real Pixel 9 Pro (8,577 photos: grid
-renders, scroll = 60 fps with 0 hitches). Next: M4 viewer.
+verified: 14 unit tests (incl. the scroll-back FIFO-starvation regression),
+5k synthetic engine test, on the emulator (real JPEGs: first paint 0.6 s
+cold, warm relaunch from disk cache, 0 ANRs through 16 fast swipes), and on
+a real Pixel 9 Pro (8,577 photos: grid renders, scroll = 60 fps with 0
+hitches). The reported scroll-back-to-top shimmer was root-caused (FIFO job
+queue starvation) and fixed (prepend-on-submit) + regression-tested. Next:
+M4 viewer.
 
 ---
 
@@ -93,9 +96,12 @@ cargo run                     # desktop (Linux needs libfontconfig1+pkg-config)
   `/Pictures/realtest/` (150, generated with PIL). They were `touch`-ed to a
   future mtime so they sort to the TOP of the grid — when reasoning about
   "which cells should have images", remember only cells 0–149 are real;
-  everything below is stubs and correctly shows shimmer. `adb push` of a
-  directory nests a subfolder (push files individually or `mv` up);
-  `adb shell touch -d '…'` works for setting mtimes. The thumb cache is at
+  everything below is stubs and correctly shows shimmer. **`/Pictures/realtest_extra/`
+  holds 700 more real, decodable JPEGs (PIL, 1200–4032 px, mtime 2031 so they
+  sort to the very top, above the 150 in `realtest`) — added for the
+  scroll-back backlog repro; the real region is now ~850 files (283 rows).
+  `adb push` of a directory nests a subfolder (push files individually or
+  `mv` up); `adb shell touch -d '…'` works for setting mtimes. The thumb cache is at
   `/sdcard/Android/data/wow.gmp.gallery/files/thumbs` (wipe it to force
   re-decode).
 - Perf measurement: `adb shell perfetto` / `adb shell dumpsys gfxinfo wow.gmp.gallery`,
@@ -407,6 +413,19 @@ pool threads), and `logcat` pool timing logs behind a `debug_timing` flag.
   (`apply_row_batch`) and are capped at 16 rows/tick to avoid ANR on fast
   scroll; `on_scroll` only re-anchors the active range + submits prefetch
   (no model mutation), all mutation is in the 10 Hz drain timer.
+
+  **Scroll-back-to-top shimmer (fixed 2026-10-27, reported from the phone):**
+  scrolling down fills the worker queue with slow cold decodes of far cells;
+  scrolling back up re-submits the top cells as fast disk-cache HITS, but on
+  the old FIFO job queue they waited behind the whole cold backlog — seconds
+  of shimmer (deterministic repro: 6.3 s behind a 180-cold backlog). Fix: the
+  job queue is now a `Mutex<VecDeque>` + `Condvar` and `Engine::submit`
+  **prepends** each new batch (most-recent intent first; stale far prefetch
+  can no longer starve the viewport). Regression test
+  `thumb::tests::scroll_back_cache_hit_not_starved` (6.3 s → 130 ms). Note
+  `mpsc` can't do this — it's FIFO-only; a priority queue needs the
+  mutex+condvar (dispatcher exits via a `closed` flag set in `Engine::drop`).
+
   *Verified (emulator, 150 real JPEGs + 5k stubs): first paint 0.6 s cold;
   warm relaunch from disk cache; 0 ANRs through 16 fast swipes; backlog
   drains to 0 when idle. Verified on Pixel 9 Pro (release aarch64, 8,577

src/thumb.rs

@@ -15,12 +15,19 @@
 //! valid) but drop the UI delivery. The queue is unbounded on purpose —
 //! backlog is visible in the timing logs (acceptance: it must not grow while
 //! scrolling).
+//!
+//! Queue ordering is PREPEND (not FIFO): `submit` puts each new batch at the
+//! FRONT of the queue. The most recently requested content is what the user
+//! is looking at (or scrolling back to); letting a backlog of stale far-away
+//! cold decodes sit ahead of it starved fast disk-cache-hit re-submissions —
+//! the scroll-back-to-top shimmer bug. Regression: `scroll_back_cache_hit_not_starved`.
 
+use std::collections::VecDeque;
 use std::fs;
 use std::path::{Path, PathBuf};
-use std::sync::atomic::{AtomicU64, AtomicUsize, Ordering};
+use std::sync::atomic::{AtomicBool, AtomicU64, AtomicUsize, Ordering};
 use std::sync::mpsc;
-use std::sync::{Arc, Mutex};
+use std::sync::{Arc, Condvar, Mutex};
 use std::time::Instant;
 
 use sha1::{Digest, Sha1};
@@ -60,7 +67,15 @@ pub struct JobResult {
 }
 
 pub struct Engine {
-    tx: mpsc::Sender<Job>,
+    /// Job queue: Mutex<VecDeque> + Condvar (not mpsc — mpsc is FIFO-only).
+    /// NEW submissions are PREPENDED: the most recently requested content is
+    /// what the user is looking at (or scrolling back to); stale far-away
+    /// prefetch must not starve it. Concretely fixes the scroll-back-to-top
+    /// case where fast disk-cache-hit jobs waited seconds behind a backlog of
+    /// slow cold decodes (regression: `scroll_back_cache_hit_not_starved`).
+    jobs: Arc<Mutex<VecDeque<Job>>>,
+    job_cv: Arc<Condvar>,
+    closed: Arc<AtomicBool>,
     /// Only the UI thread drains; the Mutex just makes Engine Sync so it can
     /// live in a process-wide OnceLock.
     rx: Mutex<mpsc::Receiver<JobResult>>,
@@ -73,6 +88,13 @@ pub struct Engine {
     target: u32,
 }
 
+impl Drop for Engine {
+    fn drop(&mut self) {
+        self.closed.store(true, Ordering::SeqCst);
+        self.job_cv.notify_all();
+    }
+}
+
 /// Cache file for a job: `thumbs/{sha1(path|mtime|size|target)}.jpg` — the
 /// target is in the key so different render sizes coexist (M2 list 64 px,
 /// M3 grid 500 px).
@@ -94,7 +116,9 @@ impl Engine {
     /// `target` px (longest side).
     pub fn new(cache_dir: PathBuf, workers: usize, target: u32) -> Result<Self, String> {
         fs::create_dir_all(&cache_dir).map_err(|e| format!("mkdir {}: {e}", cache_dir.display()))?;
-        let (job_tx, job_rx) = mpsc::channel::<Job>();
+        let jobs = Arc::new(Mutex::new(VecDeque::<Job>::new()));
+        let job_cv = Arc::new(Condvar::new());
+        let closed = Arc::new(AtomicBool::new(false));
         let (res_tx, res_rx) = mpsc::channel::<JobResult>();
         let gen = Arc::new(AtomicU64::new(1));
         let in_flight = Arc::new(AtomicUsize::new(0));
@@ -120,12 +144,25 @@ impl Engine {
         }
         drop(res_tx);
 
-        // Dispatcher: single receiver for std::mpsc, round-robins to workers.
+        // Dispatcher: pops the job queue (front = most recent intent) and
+        // round-robins to workers. Exits when the engine is dropped and the
+        // queue has drained.
+        let (d_jobs, d_cv, d_closed) = (jobs.clone(), job_cv.clone(), closed.clone());
         std::thread::Builder::new()
             .name("thumb-dispatch".into())
             .spawn(move || {
                 let mut next = 0usize;
-                for job in job_rx {
+                loop {
+                    let job = {
+                        let mut dq = d_jobs.lock().unwrap();
+                        while dq.is_empty() {
+                            if d_closed.load(Ordering::SeqCst) {
+                                return; // engine dropped, queue drained
+                            }
+                            dq = d_cv.wait(dq).unwrap();
+                        }
+                        dq.pop_front().unwrap()
+                    };
                     for _ in 0..workers_tx.len() {
                         if workers_tx[next % workers_tx.len()].send(job.clone()).is_ok() {
                             next += 1;
@@ -138,7 +175,9 @@ impl Engine {
             .map_err(|e| e.to_string())?;
 
         Ok(Self {
-            tx: job_tx,
+            jobs,
+            job_cv,
+            closed,
             rx: Mutex::new(res_rx),
             gen,
             cache_dir,
@@ -167,14 +206,21 @@ impl Engine {
         self.gen.fetch_add(1, Ordering::Relaxed) + 1
     }
 
+    /// Submit jobs, PREPENDED to the queue: fresh submissions express the
+    /// user's current (or return) intent and must be decoded before stale
+    /// far-away prefetch. Within a batch the original order is preserved.
     pub fn submit(&self, jobs: &[Job]) {
-        for j in jobs {
-            self.in_flight.fetch_add(1, Ordering::Relaxed);
-            if self.tx.send(j.clone()).is_err() {
-                self.in_flight.fetch_sub(1, Ordering::Relaxed);
-                break; // engine gone
+        if jobs.is_empty() {
+            return;
+        }
+        {
+            let mut dq = self.jobs.lock().unwrap();
+            for j in jobs.iter().rev() {
+                dq.push_front(j.clone());
             }
         }
+        self.in_flight.fetch_add(jobs.len(), Ordering::Relaxed);
+        self.job_cv.notify_one();
     }
 
     /// Non-blocking drain (UI thread). Stale-generation results are dropped.
@@ -443,6 +489,92 @@ mod tests {
         let _ = p;
     }
 
+    /// Scroll-back bug (regression): a user scrolls down (queue fills with
+    /// slow cold decodes of far cells), then back to the top. The top cells'
+    /// jobs are fast disk-cache HITS but, on a FIFO queue, they wait behind
+    /// the entire cold backlog — the user sees shimmer for seconds.
+    /// Re-submitted (recently requested) work must be decoded first.
+    #[test]
+    fn scroll_back_cache_hit_not_starved() {
+        let dir = tempfile::tempdir().unwrap();
+        let cache = dir.path().join("thumbs");
+        std::env::set_var("GALLERY_DB_PATH", dir.path().join("gallery.db"));
+
+        // Detailed (noise) JPEGs: non-trivial cold decodes, still small files.
+        let mk = |name: &str| -> Entry {
+            let (w, h) = (2048u32, 1536u32);
+            let mut rgb = Vec::with_capacity(3 * (w * h) as usize);
+            let mut x: u64 = 0x9e3779b97f4a7c15;
+            for _ in 0..(w * h) {
+                x = x
+                    .wrapping_mul(6364136223846793005)
+                    .wrapping_add(1442695040888963407);
+                rgb.extend_from_slice(&[(x >> 33) as u8, (x >> 17) as u8, (x >> 1) as u8]);
+            }
+            let p = dir.path().join(name);
+            std::fs::write(&p, images::encode_jpeg(&rgb, w, h, 80).unwrap()).unwrap();
+            Entry {
+                path: p.to_str().unwrap().into(),
+                mtime: 1,
+                size: std::fs::metadata(&p).unwrap().len() as i64,
+            }
+        };
+        let top: Vec<Entry> = (0..60).map(|i| mk(&format!("top{i:03}.jpg"))).collect();
+        let far: Vec<Entry> = (0..180).map(|i| mk(&format!("far{i:03}.jpg"))).collect();
+
+        let eng = Engine::new(cache.clone(), 4, GRID_TARGET).unwrap();
+        let gen = eng.generation();
+        let jobs = |es: &[Entry], base: usize| {
+            es.iter()
+                .enumerate()
+                .map(|(i, e)| Job {
+                    id: base + i,
+                    path: e.path.clone(),
+                    mtime: e.mtime,
+                    size: e.size,
+                    gen,
+                })
+                .collect::<Vec<_>>()
+        };
+
+        // Warm the top 60 into the disk cache (they were painted earlier).
+        eng.submit(&jobs(&top, 0));
+        let mut got = 0;
+        while got < 60 {
+            std::thread::sleep(std::time::Duration::from_millis(5));
+            got += eng.drain().len();
+        }
+
+        // "Scroll down": 180 cold far jobs. "Scroll back up": 60 warm tops.
+        let t0 = Instant::now();
+        eng.submit(&jobs(&far, 1000));
+        eng.submit(&jobs(&top, 0));
+        let mut tops = 0;
+        let mut total = 0;
+        let mut t_tops = None;
+        while total < 240 {
+            std::thread::sleep(std::time::Duration::from_millis(2));
+            for r in eng.drain() {
+                total += 1;
+                if r.id < 60 && t_tops.is_none() {
+                    tops += 1;
+                    if tops == 60 {
+                        t_tops = Some(t0.elapsed());
+                    }
+                }
+            }
+        }
+        let t_tops = t_tops.unwrap();
+        eprintln!(
+            "scroll-back: all 60 cache-hit tops in {t_tops:?} behind a 180-cold-job backlog (total {:?})",
+            t0.elapsed()
+        );
+        assert!(
+            t_tops.as_secs_f64() < 0.75,
+            "cache-hit re-entry waited {t_tops:?} behind the cold backlog — FIFO starvation"
+        );
+    }
+
     #[test]
     fn cache_key_stable_and_distinct() {
         let d = Path::new("/tmp/x");