From d492353b8307b1644626b55fe9008130f038e41b Mon Sep 17 00:00:00 2001 From: Babken Egoian <101829110+green2grey@users.noreply.github.com> Date: Sun, 5 Jul 2026 22:28:02 -0700 Subject: [PATCH 1/4] feat: wire up on-disk OCR cache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cache OCR results as JSON under ~/.cache/quickview/ocr/, keyed by blake3(path + lang + mtime + size) so edited files are simply misses. Revises ADR-0009's in-memory-first decision: Quick Preview spawns a fresh process per invocation, so only a persistent cache helps it. - cache.rs: load_ocr/store_ocr taking an explicit cache root (testable without ProjectDirs); atomic temp-file + rename writes since concurrent QuickView processes are a designed use case - start_ocr: cache check/write inside the existing worker-thread closure — main thread never touches disk, and hits flow through the same channel so the OCR job-id guard applies unchanged - empty results are cached (text-free images don't re-run tesseract); failures are not, so transient errors retry on next open - no eviction in v1 (entries are a few KB); documented in ADR-0009 implementation notes along with the future settings-in-key rule - tests: key sensitivity (lang/path/size/mtime), round-trip, corrupt and absent entries as misses --- .claude/CLAUDE.md | 3 +- Cargo.lock | 2 + adrs/ADR-0009-Caching.md | 25 ++++ crates/quickview-core/Cargo.toml | 4 + crates/quickview-core/src/cache.rs | 159 +++++++++++++++++++++- crates/quickview-ui/src/windows/shared.rs | 19 ++- docs/DECISIONS.md | 14 +- docs/DEPENDENCIES.md | 1 + docs/PHASED_PLAN.md | 6 +- 9 files changed, 221 insertions(+), 12 deletions(-) diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md index d5cd21a..601ecb8 100644 --- a/.claude/CLAUDE.md +++ b/.claude/CLAUDE.md @@ -51,13 +51,14 @@ The scaffold is functional with image display, async OCR pipeline, drag-select o - Full Viewer window (headerbar, arrow key navigation) - File info in the headerbar (filename, dimensions, file size) - Async OCR (Tesseract TSV → word bounding boxes) +- On-disk OCR cache (`~/.cache/quickview/ocr/`, keyed by path+lang+mtime+size; + no eviction in v1 — see ADR-0009 implementation notes) - Drag-select overlay with word highlighting - Ctrl+C clipboard copy and right-click context menu (Copy / Copy All Text) - Stale decode and OCR result cancellation via monotonic job IDs - Zoom & pan (Ctrl+scroll, pinch, +/- keys, middle-drag pan) via custom `ZoomableCanvas` widget ### What's not implemented yet: -- OCR caching (cache module exists but is not wired up) - Quick Preview click-outside-to-close and single-instance toggle - Performance benchmarks diff --git a/Cargo.lock b/Cargo.lock index a40f77c..ed305e2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1653,6 +1653,8 @@ dependencies = [ "csv", "directories", "serde", + "serde_json", + "tempfile", "thiserror 2.0.18", ] diff --git a/adrs/ADR-0009-Caching.md b/adrs/ADR-0009-Caching.md index 2755141..6839ac1 100644 --- a/adrs/ADR-0009-Caching.md +++ b/adrs/ADR-0009-Caching.md @@ -35,3 +35,28 @@ Cache key should include: - **Persistent on-disk cache from day one** — more complex, needs eviction policy and cleanup; deferred to a later phase - **No cache** — simplest but OCR re-runs on every open, noticeably slow for repeat views +## Implementation notes (2026-07-05) + +The implementation went **straight to on-disk**, revising the decision above: + +- Quick Preview spawns a fresh process per invocation, so an in-memory cache + buys the flagship workflow nothing; on-disk also covers in-session revisits + (re-reading a few-KB JSON is effectively instant). +- `cache.rs` key derivation already existed: blake3 of + `path + lang + mtime + size` → `/ocr/.json` (Linux: + `~/.cache/quickview/ocr/`). Staleness needs no invalidation logic — an + edited file produces a new key and is simply a miss. The path is derived + from the lowercased app name, so the pending app-ID rename does not move it + on Linux. +- Tesseract is currently invoked with no psm/oem flags, so `lang` is the only + setting and it is in the key. **When OCR settings become configurable + (Phase 7 hardening: psm/oem, tessdata_fast/best), they must join the key.** +- Writes are atomic (temp file + rename in the same directory): concurrent + QuickView processes are a designed use case. +- Empty results are cached (text-free images shouldn't re-run tesseract); + failures are not cached, so transient errors retry on the next open. A + failed cache write is a warning, never a failed OCR. +- **No eviction in v1.** Entries are a few KB; the directory grows unbounded + but slowly. Manual cleanup: `rm -rf ~/.cache/quickview/ocr`. Real eviction + arrives with the Phase 8 persistent SQLite cache that replaces this scheme. + diff --git a/crates/quickview-core/Cargo.toml b/crates/quickview-core/Cargo.toml index 8a68486..98d4f13 100644 --- a/crates/quickview-core/Cargo.toml +++ b/crates/quickview-core/Cargo.toml @@ -11,6 +11,10 @@ anyhow.workspace = true csv = "1" serde = { version = "1", features = ["derive"] } +serde_json = "1" directories = "5" blake3 = "1" +[dev-dependencies] +tempfile = "3" + diff --git a/crates/quickview-core/src/cache.rs b/crates/quickview-core/src/cache.rs index 0688ee4..f9c7d02 100644 --- a/crates/quickview-core/src/cache.rs +++ b/crates/quickview-core/src/cache.rs @@ -1,7 +1,17 @@ +//! On-disk OCR result cache. +//! +//! Entries are JSON files under `/ocr/`, keyed by a blake3 hash of +//! the image path, OCR language, and file mtime+size — so an edited file is +//! simply a cache miss (no invalidation logic needed). There is no eviction in +//! v1: entries are a few KB each, and users can clear the directory manually. +//! Phase 8's persistent SQLite cache is the planned successor (ADR-0009). + use std::path::{Path, PathBuf}; use directories::ProjectDirs; +use crate::ocr::models::OcrResult; + /// Return the XDG cache directory for QuickView. /// /// On Linux this is typically: `~/.cache/quickview/`. @@ -11,9 +21,7 @@ pub fn cache_dir() -> Option { Some(proj.cache_dir().to_path_buf()) } -pub fn ocr_cache_path(file: &Path, lang: &str) -> Option { - let dir = cache_dir()?; - +pub fn ocr_cache_path(cache_root: &Path, file: &Path, lang: &str) -> PathBuf { // Include file metadata to avoid stale caches. let meta = std::fs::metadata(file).ok(); let mtime = meta @@ -33,5 +41,148 @@ pub fn ocr_cache_path(file: &Path, lang: &str) -> Option { hasher.update(&size.to_le_bytes()); let key = hasher.finalize().to_hex().to_string(); - Some(dir.join("ocr").join(format!("{key}.json"))) + cache_root.join("ocr").join(format!("{key}.json")) +} + +/// Load a cached OCR result for `file`, or `None` on a miss. +/// +/// Any failure (entry absent, unreadable, corrupt, or written by an +/// incompatible older schema) is treated as a miss: OCR re-runs and the entry +/// is overwritten. +pub fn load_ocr(cache_root: &Path, file: &Path, lang: &str) -> Option { + let path = ocr_cache_path(cache_root, file, lang); + let bytes = std::fs::read(path).ok()?; + serde_json::from_slice(&bytes).ok() +} + +/// Store an OCR result for `file`. +/// +/// The write is atomic (temp file + rename in the same directory): concurrent +/// QuickView processes are a designed use case (Quick Preview spawns one per +/// invocation), so a torn write must never be readable. +pub fn store_ocr( + cache_root: &Path, + file: &Path, + lang: &str, + result: &OcrResult, +) -> anyhow::Result<()> { + let path = ocr_cache_path(cache_root, file, lang); + let dir = path + .parent() + .ok_or_else(|| anyhow::anyhow!("cache path has no parent: {}", path.display()))?; + std::fs::create_dir_all(dir)?; + + let json = serde_json::to_vec(result)?; + let tmp = path.with_extension(format!("json.tmp-{}", std::process::id())); + std::fs::write(&tmp, &json)?; + if let Err(err) = std::fs::rename(&tmp, &path) { + let _ = std::fs::remove_file(&tmp); + return Err(err.into()); + } + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::geometry::Rect; + use crate::ocr::models::OcrWord; + + fn sample_result() -> OcrResult { + OcrResult { + words: vec![OcrWord { + text: "hello".into(), + confidence: 96.5, + bbox: Rect { + x: 10.0, + y: 20.0, + w: 30.0, + h: 40.0, + }, + order: 0, + }], + } + } + + #[test] + fn key_changes_with_lang_path_and_metadata() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + + let img = dir.path().join("a.png"); + std::fs::write(&img, b"xx").unwrap(); + let base = ocr_cache_path(root, &img, "eng"); + + // Same inputs -> same key. + assert_eq!(base, ocr_cache_path(root, &img, "eng")); + + // Different language -> different key. + assert_ne!(base, ocr_cache_path(root, &img, "deu")); + + // Different path -> different key. + let img2 = dir.path().join("b.png"); + std::fs::write(&img2, b"xx").unwrap(); + assert_ne!(base, ocr_cache_path(root, &img2, "eng")); + + // Different size -> different key. + std::fs::write(&img, b"xxxx").unwrap(); + assert_ne!(base, ocr_cache_path(root, &img, "eng")); + + // Different mtime (same size) -> different key. + std::fs::write(&img, b"xx").unwrap(); + let before = ocr_cache_path(root, &img, "eng"); + let old = std::time::SystemTime::UNIX_EPOCH + std::time::Duration::from_secs(1_000_000); + std::fs::File::open(&img) + .unwrap() + .set_modified(old) + .unwrap(); + assert_ne!(before, ocr_cache_path(root, &img, "eng")); + } + + #[test] + fn store_then_load_round_trips() { + let dir = tempfile::tempdir().unwrap(); + let img = dir.path().join("a.png"); + std::fs::write(&img, b"xx").unwrap(); + + let result = sample_result(); + store_ocr(dir.path(), &img, "eng", &result).unwrap(); + + let loaded = load_ocr(dir.path(), &img, "eng").expect("cache hit"); + assert_eq!(loaded.words.len(), 1); + assert_eq!(loaded.words[0].text, "hello"); + assert_eq!(loaded.words[0].bbox, result.words[0].bbox); + assert_eq!(loaded.words[0].order, 0); + + // No stray temp file left behind. + let ocr_dir = dir.path().join("ocr"); + let leftovers: Vec<_> = std::fs::read_dir(&ocr_dir) + .unwrap() + .filter_map(|e| e.ok()) + .filter(|e| e.path().extension().is_none_or(|ext| ext != "json")) + .collect(); + assert!(leftovers.is_empty(), "temp files left: {leftovers:?}"); + } + + #[test] + fn absent_entry_is_a_miss() { + let dir = tempfile::tempdir().unwrap(); + let img = dir.path().join("a.png"); + std::fs::write(&img, b"xx").unwrap(); + + assert!(load_ocr(dir.path(), &img, "eng").is_none()); + } + + #[test] + fn corrupt_entry_is_a_miss() { + let dir = tempfile::tempdir().unwrap(); + let img = dir.path().join("a.png"); + std::fs::write(&img, b"xx").unwrap(); + + let path = ocr_cache_path(dir.path(), &img, "eng"); + std::fs::create_dir_all(path.parent().unwrap()).unwrap(); + std::fs::write(&path, b"{not json").unwrap(); + + assert!(load_ocr(dir.path(), &img, "eng").is_none()); + } } diff --git a/crates/quickview-ui/src/windows/shared.rs b/crates/quickview-ui/src/windows/shared.rs index caa7fd7..c27883b 100644 --- a/crates/quickview-ui/src/windows/shared.rs +++ b/crates/quickview-ui/src/windows/shared.rs @@ -8,7 +8,7 @@ use gtk::prelude::*; use gtk4 as gtk; use quickview_core::{ - fs, + cache, fs, ocr::{tesseract, tsv}, }; @@ -224,10 +224,27 @@ impl ViewerController { let new_id = self.ocr_job_id.get().wrapping_add(1); self.ocr_job_id.set(new_id); + // All cache I/O stays on the worker thread; hits flow through the same + // channel as fresh results, so the job-id guard applies unchanged. + let cache_root = cache::cache_dir(); std::thread::spawn(move || { let r = (|| { + if let Some(root) = &cache_root { + if let Some(cached) = cache::load_ocr(root, &path, &lang) { + tracing::debug!("OCR cache hit for {}", path.display()); + return Ok(cached); + } + } let tsv_out = tesseract::run_tesseract_tsv(&path, &lang)?; let parsed = tsv::parse_tesseract_tsv(&tsv_out)?; + // Empty results are cached too (text-free images shouldn't + // re-run tesseract); failures are not, so transient errors + // retry on the next open. + if let Some(root) = &cache_root { + if let Err(err) = cache::store_ocr(root, &path, &lang, &parsed) { + tracing::warn!("failed to write OCR cache: {err:#}"); + } + } Ok(parsed) })(); diff --git a/docs/DECISIONS.md b/docs/DECISIONS.md index b85ba03..bc19408 100644 --- a/docs/DECISIONS.md +++ b/docs/DECISIONS.md @@ -29,7 +29,7 @@ For more detail, see the ADRs in `adrs/`. ### UX + data - **Selection:** word boxes + drag selection; render highlight overlay - **Clipboard:** GTK native clipboard API; optional `wl-copy` fallback -- **Caching:** start in-memory; add persistent cache when needed +- **Caching:** on-disk OCR cache keyed by path+lang+mtime+size (implemented; revised from "in-memory first" — see ADR-0009 implementation notes) --- @@ -129,11 +129,15 @@ Recommendation: implement TSV first; add hOCR later as debug/export. ### 7) Caching -**Recommended** -- In-memory cache keyed by `{path, mtime, size, lang, settings}`. +**Implemented (revised from the original in-memory recommendation)** +- On-disk OCR cache: blake3 key of `{path, lang, mtime, size}` → JSON under + `~/.cache/quickview/ocr/`. Chosen because Quick Preview spawns a fresh + process per invocation, so only a persistent cache helps it. No eviction in + v1; when OCR settings (psm/oem, tessdata variant) become configurable they + must join the key. See ADR-0009 implementation notes. -**Alternative** -- Persistent cache (SQLite) when repeated OCR is common. +**Future** +- Persistent cache (SQLite) with eviction — Phase 8. --- diff --git a/docs/DEPENDENCIES.md b/docs/DEPENDENCIES.md index 453c1d7..9424b20 100644 --- a/docs/DEPENDENCIES.md +++ b/docs/DEPENDENCIES.md @@ -34,6 +34,7 @@ Optional: - `csv` (TSV parsing) - `blake3` (cache key hashing) - `serde` (OCR model serialization) +- `serde_json` (on-disk OCR cache entries) - `directories` (platform cache/config paths) ## Updating crates diff --git a/docs/PHASED_PLAN.md b/docs/PHASED_PLAN.md index f048721..1129a67 100644 --- a/docs/PHASED_PLAN.md +++ b/docs/PHASED_PLAN.md @@ -157,7 +157,11 @@ If priorities change, you can reshuffle phases, but try to keep the “render fi - previous image stays visible (under the busy spinner) until decode completes ✅ - stale-result guard (monotonic job ID, same pattern as OCR) so fast arrow-key navigation cannot race decodes ✅ -- Add cache (in-memory first; `cache.rs` on-disk key derivation exists but is unwired) +- Add cache ✅ — on-disk OCR cache (revising ADR-0009's "in-memory first"; see + its implementation notes): blake3 key of path+lang+mtime+size → JSON under + `~/.cache/quickview/ocr/`, checked/written on the OCR worker thread, atomic + writes, no eviction in v1. Survives restarts, so Quick Preview's + process-per-invocation benefits too. - Add basic benchmarking hooks (decode + OCR timing) - Improve OCR accuracy options: - language selection via config file / env var (CLI `--lang` already exists) From f886e19dbff48e0563eccaf7888c753bd06269fb Mon Sep 17 00:00:00 2001 From: Babken Egoian <101829110+green2grey@users.noreply.github.com> Date: Sun, 5 Jul 2026 22:50:41 -0700 Subject: [PATCH 2/4] fix: harden OCR cache key and temp-file handling (PR review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - hash nanosecond mtime: whole seconds aliased a same-second rewrite of the same path with an unchanged byte length (Codex) - snapshot the cache key before tesseract runs: a file edited mid-OCR now stores its stale result under the old key, which the edited file correctly misses, instead of under the new metadata's key (Codex); load_ocr/store_ocr now take the entry path derived once up front - unique temp file per store (pid + per-process counter) so concurrent same-key writes cannot clobber each other's temp file (CodeRabbit) - remove the temp file when the write itself fails, not just the rename — v1 has no eviction pass to sweep orphans (CodeRabbit) - test: subsecond mtime change (same second, same size) changes the key --- adrs/ADR-0009-Caching.md | 10 ++- crates/quickview-core/src/cache.rs | 79 ++++++++++++++--------- crates/quickview-ui/src/windows/shared.rs | 18 ++++-- 3 files changed, 68 insertions(+), 39 deletions(-) diff --git a/adrs/ADR-0009-Caching.md b/adrs/ADR-0009-Caching.md index 6839ac1..04ed311 100644 --- a/adrs/ADR-0009-Caching.md +++ b/adrs/ADR-0009-Caching.md @@ -45,9 +45,13 @@ The implementation went **straight to on-disk**, revising the decision above: - `cache.rs` key derivation already existed: blake3 of `path + lang + mtime + size` → `/ocr/.json` (Linux: `~/.cache/quickview/ocr/`). Staleness needs no invalidation logic — an - edited file produces a new key and is simply a miss. The path is derived - from the lowercased app name, so the pending app-ID rename does not move it - on Linux. + edited file produces a new key and is simply a miss. The mtime is hashed at + full nanosecond precision so a same-second rewrite with an unchanged byte + length still misses, and the key is snapshotted *before* tesseract runs so + a file edited mid-OCR stores its stale result under the old key (which the + edited file then correctly misses) rather than the new one. The path is + derived from the lowercased app name, so the pending app-ID rename does not + move it on Linux. - Tesseract is currently invoked with no psm/oem flags, so `lang` is the only setting and it is in the key. **When OCR settings become configurable (Phase 7 hardening: psm/oem, tessdata_fast/best), they must join the key.** diff --git a/crates/quickview-core/src/cache.rs b/crates/quickview-core/src/cache.rs index f9c7d02..fd1a38f 100644 --- a/crates/quickview-core/src/cache.rs +++ b/crates/quickview-core/src/cache.rs @@ -22,13 +22,15 @@ pub fn cache_dir() -> Option { } pub fn ocr_cache_path(cache_root: &Path, file: &Path, lang: &str) -> PathBuf { - // Include file metadata to avoid stale caches. + // Include file metadata to avoid stale caches. Full nanosecond mtime: + // whole seconds would alias a same-second rewrite of the same path with + // an unchanged byte length (rapid screenshot/editor saves). let meta = std::fs::metadata(file).ok(); let mtime = meta .as_ref() .and_then(|m| m.modified().ok()) .and_then(|t| t.duration_since(std::time::UNIX_EPOCH).ok()) - .map(|d| d.as_secs()) + .map(|d| d.as_nanos()) .unwrap_or(0); let size = meta.as_ref().map(|m| m.len()).unwrap_or(0); @@ -44,38 +46,45 @@ pub fn ocr_cache_path(cache_root: &Path, file: &Path, lang: &str) -> PathBuf { cache_root.join("ocr").join(format!("{key}.json")) } -/// Load a cached OCR result for `file`, or `None` on a miss. +/// Load the cached OCR result at `entry` (from [`ocr_cache_path`]), or `None` +/// on a miss. /// /// Any failure (entry absent, unreadable, corrupt, or written by an /// incompatible older schema) is treated as a miss: OCR re-runs and the entry /// is overwritten. -pub fn load_ocr(cache_root: &Path, file: &Path, lang: &str) -> Option { - let path = ocr_cache_path(cache_root, file, lang); - let bytes = std::fs::read(path).ok()?; +pub fn load_ocr(entry: &Path) -> Option { + let bytes = std::fs::read(entry).ok()?; serde_json::from_slice(&bytes).ok() } -/// Store an OCR result for `file`. +/// Store an OCR result at `entry` (from [`ocr_cache_path`]). /// -/// The write is atomic (temp file + rename in the same directory): concurrent -/// QuickView processes are a designed use case (Quick Preview spawns one per -/// invocation), so a torn write must never be readable. -pub fn store_ocr( - cache_root: &Path, - file: &Path, - lang: &str, - result: &OcrResult, -) -> anyhow::Result<()> { - let path = ocr_cache_path(cache_root, file, lang); - let dir = path +/// Callers must derive `entry` *before* running OCR, so that a file edited +/// mid-OCR stores its (now stale) result under the old key — which the edited +/// file then correctly misses — rather than under the new metadata's key. +/// +/// The write is atomic (unique temp file + rename in the same directory): +/// concurrent QuickView processes are a designed use case (Quick Preview +/// spawns one per invocation), so a torn write must never be readable. +pub fn store_ocr(entry: &Path, result: &OcrResult) -> anyhow::Result<()> { + let dir = entry .parent() - .ok_or_else(|| anyhow::anyhow!("cache path has no parent: {}", path.display()))?; + .ok_or_else(|| anyhow::anyhow!("cache path has no parent: {}", entry.display()))?; std::fs::create_dir_all(dir)?; let json = serde_json::to_vec(result)?; - let tmp = path.with_extension(format!("json.tmp-{}", std::process::id())); - std::fs::write(&tmp, &json)?; - if let Err(err) = std::fs::rename(&tmp, &path) { + // pid + per-process counter: unique even across concurrent same-key + // writes from this process and from other QuickView processes. + static WRITE_SEQ: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); + let seq = WRITE_SEQ.fetch_add(1, std::sync::atomic::Ordering::Relaxed); + let tmp = entry.with_extension(format!("json.tmp-{}-{seq}", std::process::id())); + if let Err(err) = std::fs::write(&tmp, &json) { + // A partial write (e.g. disk full) may have created the file; there + // is no eviction pass in v1 to sweep it up later. + let _ = std::fs::remove_file(&tmp); + return Err(err.into()); + } + if let Err(err) = std::fs::rename(&tmp, entry) { let _ = std::fs::remove_file(&tmp); return Err(err.into()); } @@ -137,6 +146,16 @@ mod tests { .set_modified(old) .unwrap(); assert_ne!(before, ocr_cache_path(root, &img, "eng")); + + // Subsecond mtime change (same second, same size) -> different key. + let with_key = |t| { + std::fs::File::open(&img).unwrap().set_modified(t).unwrap(); + ocr_cache_path(root, &img, "eng") + }; + assert_ne!( + with_key(old + std::time::Duration::from_nanos(1)), + with_key(old) + ); } #[test] @@ -144,11 +163,12 @@ mod tests { let dir = tempfile::tempdir().unwrap(); let img = dir.path().join("a.png"); std::fs::write(&img, b"xx").unwrap(); + let entry = ocr_cache_path(dir.path(), &img, "eng"); let result = sample_result(); - store_ocr(dir.path(), &img, "eng", &result).unwrap(); + store_ocr(&entry, &result).unwrap(); - let loaded = load_ocr(dir.path(), &img, "eng").expect("cache hit"); + let loaded = load_ocr(&entry).expect("cache hit"); assert_eq!(loaded.words.len(), 1); assert_eq!(loaded.words[0].text, "hello"); assert_eq!(loaded.words[0].bbox, result.words[0].bbox); @@ -170,7 +190,8 @@ mod tests { let img = dir.path().join("a.png"); std::fs::write(&img, b"xx").unwrap(); - assert!(load_ocr(dir.path(), &img, "eng").is_none()); + let entry = ocr_cache_path(dir.path(), &img, "eng"); + assert!(load_ocr(&entry).is_none()); } #[test] @@ -179,10 +200,10 @@ mod tests { let img = dir.path().join("a.png"); std::fs::write(&img, b"xx").unwrap(); - let path = ocr_cache_path(dir.path(), &img, "eng"); - std::fs::create_dir_all(path.parent().unwrap()).unwrap(); - std::fs::write(&path, b"{not json").unwrap(); + let entry = ocr_cache_path(dir.path(), &img, "eng"); + std::fs::create_dir_all(entry.parent().unwrap()).unwrap(); + std::fs::write(&entry, b"{not json").unwrap(); - assert!(load_ocr(dir.path(), &img, "eng").is_none()); + assert!(load_ocr(&entry).is_none()); } } diff --git a/crates/quickview-ui/src/windows/shared.rs b/crates/quickview-ui/src/windows/shared.rs index c27883b..ce62ab5 100644 --- a/crates/quickview-ui/src/windows/shared.rs +++ b/crates/quickview-ui/src/windows/shared.rs @@ -229,19 +229,23 @@ impl ViewerController { let cache_root = cache::cache_dir(); std::thread::spawn(move || { let r = (|| { - if let Some(root) = &cache_root { - if let Some(cached) = cache::load_ocr(root, &path, &lang) { - tracing::debug!("OCR cache hit for {}", path.display()); - return Ok(cached); - } + // Snapshot the cache key before OCR runs: if the file is + // edited mid-OCR, the stale result lands under the old key, + // which the edited file then correctly misses. + let entry = cache_root + .as_deref() + .map(|root| cache::ocr_cache_path(root, &path, &lang)); + if let Some(cached) = entry.as_deref().and_then(cache::load_ocr) { + tracing::debug!("OCR cache hit for {}", path.display()); + return Ok(cached); } let tsv_out = tesseract::run_tesseract_tsv(&path, &lang)?; let parsed = tsv::parse_tesseract_tsv(&tsv_out)?; // Empty results are cached too (text-free images shouldn't // re-run tesseract); failures are not, so transient errors // retry on the next open. - if let Some(root) = &cache_root { - if let Err(err) = cache::store_ocr(root, &path, &lang, &parsed) { + if let Some(entry) = &entry { + if let Err(err) = cache::store_ocr(entry, &parsed) { tracing::warn!("failed to write OCR cache: {err:#}"); } } From b4c327d67301efdb9d0fd0242891c8f08ebc33b3 Mon Sep 17 00:00:00 2001 From: Babken Egoian <101829110+green2grey@users.noreply.github.com> Date: Sun, 5 Jul 2026 23:03:55 -0700 Subject: [PATCH 3/4] fix: canonicalize input path at the CLI boundary The OCR cache key hashes the literal path string, so a relative invocation ('quickview image.png') keyed on the cwd-dependent spelling: two different files could collide on one entry (given equal lang, size, and mtime), and one file opened via different spellings fragmented into several. Relative paths also broke directory navigation, since "image.png".parent() is empty. Canonicalize once in resolve_input_path so every downstream consumer sees one absolute, symlink-resolved form. Missing files fall through unchanged, preserving the graceful load-failed state. --- Cargo.lock | 1 + crates/quickview/Cargo.toml | 2 ++ crates/quickview/src/main.rs | 38 +++++++++++++++++++++++++++++++++--- 3 files changed, 38 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ed305e2..7891942 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1640,6 +1640,7 @@ dependencies = [ "anyhow", "clap", "quickview-ui", + "tempfile", "tracing", "tracing-subscriber", ] diff --git a/crates/quickview/Cargo.toml b/crates/quickview/Cargo.toml index 5df807a..c0155c9 100644 --- a/crates/quickview/Cargo.toml +++ b/crates/quickview/Cargo.toml @@ -18,3 +18,5 @@ clap = { version = "4", features = ["derive"] } quickview-ui = { path = "../quickview-ui" } +[dev-dependencies] +tempfile = "3" diff --git a/crates/quickview/src/main.rs b/crates/quickview/src/main.rs index d2298d4..be0f96e 100644 --- a/crates/quickview/src/main.rs +++ b/crates/quickview/src/main.rs @@ -53,7 +53,7 @@ fn main() -> Result<()> { } fn resolve_input_path(arg: Option) -> Result { - match arg.as_deref() { + let path = match arg.as_deref() { None | Some("-") => { let mut buf = String::new(); io::stdin().read_to_string(&mut buf)?; @@ -61,8 +61,40 @@ fn resolve_input_path(arg: Option) -> Result { if path.is_empty() { return Err(anyhow!("No input path provided (stdin was empty).")); } - Ok(PathBuf::from(path)) + PathBuf::from(path) } - Some(p) => Ok(PathBuf::from(p)), + Some(p) => PathBuf::from(p), + }; + // Canonicalize at the app boundary so every downstream consumer sees one + // absolute, symlink-resolved spelling: the OCR cache key must not depend + // on the invocation cwd (a relative path hashes as its literal string), + // and directory navigation needs a real parent dir ("image.png" has an + // empty one). A missing file falls through unchanged so the viewer can + // show its load-failed state instead of erroring here. + Ok(std::fs::canonicalize(&path).unwrap_or(path)) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn resolve_canonicalizes_existing_paths() { + let dir = tempfile::tempdir().unwrap(); + std::fs::create_dir(dir.path().join("sub")).unwrap(); + let img = dir.path().join("a.png"); + std::fs::write(&img, b"x").unwrap(); + + // A dot-and-dotdot spelling of the same file resolves to one form. + let indirect = dir.path().join(".").join("sub").join("..").join("a.png"); + let resolved = resolve_input_path(Some(indirect.display().to_string())).unwrap(); + assert_eq!(resolved, std::fs::canonicalize(&img).unwrap()); + assert!(resolved.is_absolute()); + } + + #[test] + fn resolve_passes_missing_paths_through() { + let p = resolve_input_path(Some("does-not-exist.png".into())).unwrap(); + assert_eq!(p, PathBuf::from("does-not-exist.png")); } } From 4d22e87e0a1f48f34835e808735fe3e2bcf91450 Mon Sep 17 00:00:00 2001 From: Babken Egoian <101829110+green2grey@users.noreply.github.com> Date: Sun, 5 Jul 2026 23:20:57 -0700 Subject: [PATCH 4/4] fix: create OCR cache entries 0600 in 0700 dirs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cache entries hold recognized text from the user's images (screenshots often contain emails, tokens, passwords) and are the first place QuickView persists OCR text; with umask 022 they landed world-readable (0644 in 0755 dirs), relying on the home directory for privacy. Create the temp file with mode 0600 (rename preserves it) and cache directories with mode 0700. Creation-time only; pre-existing entries keep their modes — documented in ADR-0009 with the cache-clear reset. --- adrs/ADR-0009-Caching.md | 5 +++++ crates/quickview-core/src/cache.rs | 28 ++++++++++++++++++++++++++-- 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/adrs/ADR-0009-Caching.md b/adrs/ADR-0009-Caching.md index 04ed311..ca47e04 100644 --- a/adrs/ADR-0009-Caching.md +++ b/adrs/ADR-0009-Caching.md @@ -57,6 +57,11 @@ The implementation went **straight to on-disk**, revising the decision above: (Phase 7 hardening: psm/oem, tessdata_fast/best), they must join the key.** - Writes are atomic (temp file + rename in the same directory): concurrent QuickView processes are a designed use case. +- Entries are created `0600` in `0700` directories — they hold recognized + text from the user's images, so they must not rely on the home directory + for privacy. Modes apply at creation only: entries written by builds + predating this (or a pre-existing lax cache dir) keep their old modes; + clearing `~/.cache/quickview/ocr` resets everything. - Empty results are cached (text-free images shouldn't re-run tesseract); failures are not cached, so transient errors retry on the next open. A failed cache write is a warning, never a failed OCR. diff --git a/crates/quickview-core/src/cache.rs b/crates/quickview-core/src/cache.rs index fd1a38f..611f857 100644 --- a/crates/quickview-core/src/cache.rs +++ b/crates/quickview-core/src/cache.rs @@ -66,11 +66,22 @@ pub fn load_ocr(entry: &Path) -> Option { /// The write is atomic (unique temp file + rename in the same directory): /// concurrent QuickView processes are a designed use case (Quick Preview /// spawns one per invocation), so a torn write must never be readable. +/// +/// Entries are created 0600 in 0700 directories: they hold recognized text +/// from the user's images (screenshots often contain emails, tokens, +/// passwords), so they must not rely on the home directory for privacy. +/// Modes apply at creation only; pre-existing entries/dirs keep theirs. pub fn store_ocr(entry: &Path, result: &OcrResult) -> anyhow::Result<()> { + use std::io::Write; + use std::os::unix::fs::{DirBuilderExt, OpenOptionsExt}; + let dir = entry .parent() .ok_or_else(|| anyhow::anyhow!("cache path has no parent: {}", entry.display()))?; - std::fs::create_dir_all(dir)?; + std::fs::DirBuilder::new() + .recursive(true) + .mode(0o700) + .create(dir)?; let json = serde_json::to_vec(result)?; // pid + per-process counter: unique even across concurrent same-key @@ -78,7 +89,14 @@ pub fn store_ocr(entry: &Path, result: &OcrResult) -> anyhow::Result<()> { static WRITE_SEQ: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); let seq = WRITE_SEQ.fetch_add(1, std::sync::atomic::Ordering::Relaxed); let tmp = entry.with_extension(format!("json.tmp-{}-{seq}", std::process::id())); - if let Err(err) = std::fs::write(&tmp, &json) { + // The rename preserves the temp file's 0600 mode on the final entry. + let write_result = std::fs::OpenOptions::new() + .write(true) + .create_new(true) + .mode(0o600) + .open(&tmp) + .and_then(|mut f| f.write_all(&json)); + if let Err(err) = write_result { // A partial write (e.g. disk full) may have created the file; there // is no eviction pass in v1 to sweep it up later. let _ = std::fs::remove_file(&tmp); @@ -174,6 +192,12 @@ mod tests { assert_eq!(loaded.words[0].bbox, result.words[0].bbox); assert_eq!(loaded.words[0].order, 0); + // Entry and its directory are private (0600 / 0700). + use std::os::unix::fs::PermissionsExt; + let mode = |p: &Path| std::fs::metadata(p).unwrap().permissions().mode() & 0o777; + assert_eq!(mode(&entry), 0o600); + assert_eq!(mode(entry.parent().unwrap()), 0o700); + // No stray temp file left behind. let ocr_dir = dir.path().join("ocr"); let leftovers: Vec<_> = std::fs::read_dir(&ocr_dir)