From 6e3d2c335e83017bc2f0812be0f5c68e4b316a52 Mon Sep 17 00:00:00 2001 From: Nils Pukropp Date: Wed, 12 Aug 2026 16:15:27 +0200 Subject: [PATCH] improved search prompt --- crates/pdfs-core/src/cache.rs | 19 ++++- crates/pdfs-core/src/db/local.rs | 40 ++++++--- crates/pdfs-core/src/db/migrations.rs | 58 ++++++++++++- crates/pdfs-core/src/db/mod.rs | 1 + crates/pdfs-core/src/db/nodes.rs | 61 ++++++++++---- crates/pdfs-core/src/db/pins.rs | 35 ++++++-- crates/pdfs-core/src/db/tests.rs | 89 +++++++++++++++++++- crates/pdfs-core/src/db/utils.rs | 19 ++++- crates/pdfs-core/src/localindex.rs | 49 ++++++++++- crates/pdfs-fuse/src/lib.rs | 32 ++++---- crates/pdfs-fuse/src/profile.rs | 8 +- crates/pdfs-gui/src/dmenu.rs | 2 +- crates/pdfs-gui/src/prompt.rs | 54 +++++++----- docs/BUGS.md | 113 ++++++++++++++++++++++++++ docs/plans/search-index.md | 110 +++++++++++++++++++++++++ 15 files changed, 605 insertions(+), 85 deletions(-) create mode 100644 docs/plans/search-index.md diff --git a/crates/pdfs-core/src/cache.rs b/crates/pdfs-core/src/cache.rs index ae77a7d..392377e 100644 --- a/crates/pdfs-core/src/cache.rs +++ b/crates/pdfs-core/src/cache.rs @@ -102,6 +102,16 @@ pub struct Pin { /// file. Defaulted so a legacy `pins.json` (file pins only) still parses. #[serde(default)] pub recursive: bool, + /// Whether the pinned node is a folder, when the node cache knows. + /// + /// `recursive` is pin *policy*, not node kind: a folder pinned + /// non-recursively has `recursive: false`, and a front-end reading it as + /// "this is a file" draws a file icon and sends an invalid `OpenFile`. This + /// is resolved from the `nodes` row at list time, so it is `None` for a pin + /// whose node is not cached — and absent from an older daemon's reply, + /// which is why it is defaulted. + #[serde(default)] + pub is_dir: Option, } /// The legacy JSON pin registry, kept only to import a pre-P5 `pins.json` into @@ -1348,10 +1358,11 @@ impl ContentCache { .pin_list() .unwrap_or_default() .into_iter() - .map(|(uid, path, recursive)| Pin { - uid, - path, - recursive, + .map(|row| Pin { + uid: row.uid, + path: row.path, + recursive: row.recursive, + is_dir: row.is_dir, }) .collect() } diff --git a/crates/pdfs-core/src/db/local.rs b/crates/pdfs-core/src/db/local.rs index 5ae40b6..fbbb6a0 100644 --- a/crates/pdfs-core/src/db/local.rs +++ b/crates/pdfs-core/src/db/local.rs @@ -182,16 +182,36 @@ impl Db { if !terms.is_empty() && let Some(first) = query.chars().next() { - let prefix = format!("{}%", like_escape(&first.to_string())); - let mut stmt = conn.prepare( - "SELECT path, name, is_dir, size, mtime FROM local_files - WHERE name COLLATE NOCASE LIKE ?1 ESCAPE '\\' - ORDER BY name COLLATE NOCASE LIMIT ?2", - )?; - let fallback = stmt - .query_map(params![prefix, lane_limit as i64], local_hit)? - .collect::, _>>()?; - for hit in fallback { + // Trigram rank says nothing about how well a name *starts* with the + // query, so on a home directory full of dependency trees the pool + // fills with vendored files before the obvious answer is reached + // (measured: 73% of the 500 best-ranked candidates for "test" came + // from one Go module cache). Give whole-query prefix matches their + // own lane, newest first, and keep the single-character lane — + // which recovers a typo that destroyed every trigram — behind it. + let mut lanes = vec![( + format!("{}%", like_escape(query)), + "ORDER BY mtime DESC LIMIT ?2", + )]; + lanes.push(( + format!("{}%", like_escape(&first.to_string())), + "ORDER BY name COLLATE NOCASE LIMIT ?2", + )); + let mut extra = Vec::new(); + for (pattern, order) in lanes { + let mut stmt = conn.prepare(&format!( + "SELECT path, name, is_dir, size, mtime FROM local_files + WHERE name COLLATE NOCASE LIKE ?1 ESCAPE '\\' {order}" + ))?; + extra.extend( + stmt.query_map(params![pattern, lane_limit as i64], local_hit)? + .collect::, _>>()?, + ); + } + // Prefix hits go in front: `rows` is truncated to `limit` below, and + // the lane exists precisely because trigram rank was burying them. + let trigram = std::mem::take(&mut rows); + for hit in extra.into_iter().chain(trigram) { if !rows.iter().any(|existing| existing.path == hit.path) { rows.push(hit); } diff --git a/crates/pdfs-core/src/db/migrations.rs b/crates/pdfs-core/src/db/migrations.rs index 44b9ce3..de7ec96 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 = 20; +pub(super) const SCHEMA_VERSION: i64 = 21; impl Db { pub(super) fn migrate(&self) -> Result<()> { @@ -101,6 +101,18 @@ impl Db { if current < 20 { tx.execute_batch(MIGRATION_V20)?; } + if current < 21 { + // A V17-and-older fixture may carry only the tables its own test + // cares about; there is nothing to reindex without `nodes`. + let has_nodes: bool = tx.query_row( + "SELECT COUNT(*) FROM sqlite_master WHERE type = 'table' AND name = 'nodes'", + [], + |row| row.get::<_, i64>(0), + )? > 0; + if has_nodes { + tx.execute_batch(MIGRATION_V21)?; + } + } tx.execute( "INSERT INTO sync_state (key, value) VALUES ('schema_version', ?1) ON CONFLICT(key) DO UPDATE SET value = excluded.value", @@ -527,3 +539,47 @@ const MIGRATION_V20: &str = " ALTER TABLE photos ADD COLUMN favorite INTEGER NOT NULL DEFAULT 0; CREATE INDEX IF NOT EXISTS idx_photos_favorite ON photos(favorite, seq); "; + +/// V21: rebuild the node search index after fixing what counts as indexable. +/// +/// The v16 backfill rooted its path walk at any node whose parent is null *or* +/// not cached, but the write path's `node_is_indexable_tx` demanded a +/// null-parent ancestor. Only the My Files root has one — a device folder's +/// root is stored in `device`/`sync_folder`, never in `nodes` — so every node +/// under a device folder was dropped from `nodes_fts` the first time it was +/// upserted after the backfill, and was unfindable from the prompt (2,705 of +/// 9,252 nodes on the account this was diagnosed against). +/// +/// The rule now matches the backfill, so this replays it: same recursive walk +/// as v16, over the current `nodes`. +/// Two details the v16 backfill did not carry, both cheap here: a node under a +/// trashed folder stays out (the write path has always excluded it, and +/// `tombstone_subtree_tx` only marks a subtree it was told about), and the walk +/// is depth-capped so a parent cycle in a corrupted database cannot spin the +/// migration forever. +const MIGRATION_V21: &str = " +DROP TABLE IF EXISTS nodes_fts; +CREATE VIRTUAL TABLE nodes_fts USING fts5( + name, path, tokenize='trigram' +); +INSERT INTO nodes_fts (rowid, name, path) +WITH RECURSIVE paths(rowid, uid, path, trashed, depth) AS ( + -- Only a true root (no parent at all) contributes nothing to the path: its + -- name is the mount. A row whose parent is merely uncached is an ordinary + -- folder and keeps its name, which is what `path_of` does at runtime. + SELECT n.rowid, n.uid, + CASE WHEN n.parent_uid IS NULL THEN '' ELSE n.name END, n.trashed, 0 + FROM nodes n + WHERE n.parent_uid IS NULL + OR NOT EXISTS (SELECT 1 FROM nodes p WHERE p.uid = n.parent_uid) + UNION ALL + SELECT n.rowid, n.uid, + CASE WHEN paths.path = '' THEN n.name ELSE paths.path || '/' || n.name END, + MAX(paths.trashed, n.trashed), paths.depth + 1 + FROM nodes n JOIN paths ON n.parent_uid = paths.uid + WHERE paths.depth < 256 +) +SELECT n.rowid, n.name, paths.path + FROM nodes n JOIN paths ON paths.rowid = n.rowid + WHERE paths.trashed = 0; +"; diff --git a/crates/pdfs-core/src/db/mod.rs b/crates/pdfs-core/src/db/mod.rs index 84a91d0..7ac02fb 100644 --- a/crates/pdfs-core/src/db/mod.rs +++ b/crates/pdfs-core/src/db/mod.rs @@ -50,6 +50,7 @@ pub use ops::{ PendingCounts, PendingOp, RenameMeta, op_supersedes, }; pub use photos::{StoredPhoto, THUMB_HAVE, THUMB_NONE, THUMB_UNKNOWN, TimelineRow}; +pub use pins::PinRow; pub use sync::{StoredSyncEntry, StoredSyncFolder}; pub use trash::StoredTrash; diff --git a/crates/pdfs-core/src/db/nodes.rs b/crates/pdfs-core/src/db/nodes.rs index b0c7377..a7fb182 100644 --- a/crates/pdfs-core/src/db/nodes.rs +++ b/crates/pdfs-core/src/db/nodes.rs @@ -403,22 +403,30 @@ impl Db { )?; collect_pairs(stmt.query_map(params![expression, lane_limit as i64], pair)?)? }; - // A short substitution can destroy every trigram (`vedio` vs `video`). - // Feed the scorer an additional indexed same-initial lane so it can - // recover those candidates without scanning the full node table. + // Trigram rank does not know that a name *starts* with the query, and a + // short substitution can destroy every trigram outright (`vedio` vs + // `video`). Two indexed lanes cover both without scanning the node + // table: whole-query prefix first, then same-initial. if !terms.is_empty() && let Some(first) = query.chars().next() { - let prefix = format!("{}%", like_escape(&first.to_string())); let mut stmt = conn.prepare( "SELECT node_json, uid FROM nodes WHERE name COLLATE NOCASE LIKE ?1 ESCAPE '\\' AND trashed = 0 AND node_json IS NOT NULL ORDER BY name COLLATE NOCASE LIMIT ?2", )?; - let fallback = - collect_pairs(stmt.query_map(params![prefix, lane_limit as i64], pair)?)?; - for row in fallback { + let mut extra = Vec::new(); + for pattern in [ + format!("{}%", like_escape(query)), + format!("{}%", like_escape(&first.to_string())), + ] { + extra.extend(collect_pairs( + stmt.query_map(params![pattern, lane_limit as i64], pair)?, + )?); + } + let trigram = std::mem::take(&mut rows); + for row in extra.into_iter().chain(trigram) { if !rows.iter().any(|(_, uid)| uid == &row.1) { rows.push(row); } @@ -556,12 +564,18 @@ fn upsert_node_tx(tx: &Transaction<'_>, node: &Node) -> Result<()> { )?; let mut affected = vec![rowid]; if node.is_folder() { + // The depth cap is a liveness guard, not a policy: `UNION ALL` over a + // parent cycle (a's parent is b, b's parent is a — corrupt data, but the + // API can hand it to us) never terminates, and this runs inside the + // write transaction that holds the daemon's only SQLite connection. + // Real Drive trees are nowhere near this deep. let mut stmt = tx.prepare( - "WITH RECURSIVE descendants(rowid, uid) AS ( - SELECT rowid, uid FROM nodes WHERE parent_uid = ?1 + "WITH RECURSIVE descendants(rowid, uid, depth) AS ( + SELECT rowid, uid, 0 FROM nodes WHERE parent_uid = ?1 UNION ALL - SELECT n.rowid, n.uid FROM nodes n + SELECT n.rowid, n.uid, d.depth + 1 FROM nodes n JOIN descendants d ON n.parent_uid = d.uid + WHERE d.depth < 256 ) SELECT rowid FROM descendants", )?; for descendant in stmt.query_map(params![uid], |row| row.get(0))? { @@ -603,13 +617,27 @@ fn direct_child_uids_tx(tx: &Transaction<'_>, parent: &str) -> Result, uid: &str) -> Result { let indexable: i64 = tx.query_row( - "WITH RECURSIVE ancestors(uid, parent_uid, trashed, path) AS ( - SELECT uid, parent_uid, trashed, char(31) || uid || char(31) + "WITH RECURSIVE ancestors(uid, parent_uid, trashed, depth, path) AS ( + SELECT uid, parent_uid, trashed, 0, char(31) || uid || char(31) FROM nodes WHERE uid = ?1 UNION ALL - SELECT n.uid, n.parent_uid, n.trashed, + SELECT n.uid, n.parent_uid, n.trashed, a.depth + 1, a.path || n.uid || char(31) FROM ancestors a JOIN nodes n ON n.uid = a.parent_uid WHERE instr(a.path, char(31) || n.uid || char(31)) = 0 @@ -617,7 +645,12 @@ fn node_is_indexable_tx(tx: &Transaction<'_>, uid: &str) -> Result { SELECT CASE WHEN COUNT(*) = 0 THEN 0 WHEN MAX(trashed) != 0 THEN 0 - WHEN MAX(CASE WHEN parent_uid IS NULL THEN 1 ELSE 0 END) = 0 THEN 0 + WHEN EXISTS ( + SELECT 1 FROM ancestors a + WHERE a.depth = (SELECT MAX(depth) FROM ancestors) + AND a.parent_uid IS NOT NULL + AND EXISTS (SELECT 1 FROM nodes p WHERE p.uid = a.parent_uid) + ) THEN 0 ELSE 1 END FROM ancestors", diff --git a/crates/pdfs-core/src/db/pins.rs b/crates/pdfs-core/src/db/pins.rs index d00f94b..2631af9 100644 --- a/crates/pdfs-core/src/db/pins.rs +++ b/crates/pdfs-core/src/db/pins.rs @@ -6,6 +6,18 @@ use rusqlite::{OptionalExtension, params}; use super::Db; use crate::Result; +/// One row of [`Db::pin_list`]. `recursive` is pin *policy* — whether the whole +/// subtree is kept — while `is_dir` is the node's kind as the node cache knows +/// it, or `None` for a pin whose node is not cached. Reading the former as the +/// latter is what made a non-recursively pinned folder render as a file. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct PinRow { + pub uid: String, + pub path: String, + pub recursive: bool, + pub is_dir: Option, +} + impl Db { pub fn pin_add(&self, uid: &str, path: &str, recursive: bool) -> Result<()> { let conn = self.conn.lock(); @@ -25,16 +37,23 @@ impl Db { Ok(n > 0) } - /// Every directly-pinned entry `(uid, path, recursive)`, ordered by uid. - pub fn pin_list(&self) -> Result> { + /// Every directly-pinned entry, ordered by uid. + pub fn pin_list(&self) -> Result> { let conn = self.conn.lock(); - let mut stmt = conn.prepare("SELECT uid, path, recursive FROM pins ORDER BY uid")?; + // `recursive` is pin policy; the node's kind comes from `nodes`, and is + // NULL for a pin whose node was never cached. + let mut stmt = conn.prepare( + "SELECT p.uid, p.path, p.recursive, n.is_dir + FROM pins p LEFT JOIN nodes n ON n.uid = p.uid + ORDER BY p.uid", + )?; let rows = stmt.query_map([], |r| { - Ok(( - r.get::<_, String>(0)?, - r.get::<_, String>(1)?, - r.get::<_, i64>(2)? != 0, - )) + Ok(PinRow { + uid: r.get(0)?, + path: r.get(1)?, + recursive: r.get::<_, i64>(2)? != 0, + is_dir: r.get::<_, Option>(3)?.map(|is_dir| is_dir != 0), + }) })?; let mut out = Vec::new(); for r in rows { diff --git a/crates/pdfs-core/src/db/tests.rs b/crates/pdfs-core/src/db/tests.rs index cf19498..de25d79 100644 --- a/crates/pdfs-core/src/db/tests.rs +++ b/crates/pdfs-core/src/db/tests.rs @@ -989,6 +989,62 @@ fn search_excludes_trashed_and_respects_limit() { assert_eq!(db.search("alph", 1).unwrap().len(), 1); } +/// A device folder's root is stored in `device`/`sync_folder`, never in +/// `nodes`, so its children's `parent_uid` points at a row that does not exist. +/// Requiring the ancestor walk to reach a `parent_uid IS NULL` row therefore +/// excluded every device-folder subtree from search: 2,705 of 9,252 nodes on +/// the account this was found on, including everything under `~/Downloads`. +#[test] +fn search_finds_nodes_below_an_uncached_parent() { + let db = Db::open_in_memory().unwrap(); + // "device-root" is deliberately never upserted. + db.upsert_node(&folder("downloads", Some("device-root"), "Downloads")) + .unwrap(); + db.upsert_node(&file("t", "downloads", "tickets.pdf", 1)) + .unwrap(); + + let hits = db.search("tickets", 10).unwrap(); + assert_eq!(hits.len(), 1, "a device folder's files must be searchable"); + assert_eq!(hits[0].path, "Downloads/tickets.pdf"); +} + +/// The relaxed rule must not also start indexing trash: a node whose *parent* +/// is trashed stays out even though the walk now terminates happily. +#[test] +fn search_excludes_a_node_under_a_trashed_ancestor() { + let db = Db::open_in_memory().unwrap(); + db.upsert_node(&folder("root", None, "My Files")).unwrap(); + let mut folder_gone = folder("boxed", Some("root"), "Boxed"); + folder_gone.trashed = true; + db.upsert_node(&folder_gone).unwrap(); + db.upsert_node(&file("f1", "boxed", "buried.txt", 1)) + .unwrap(); + + assert!(db.search("buried", 10).unwrap().is_empty()); +} + +/// A parent cycle is corrupt data the API can still hand us. It must stay out +/// of the index, and — because the short-query `LIKE` lane reads `nodes` +/// directly and never consults the index — resolving one's path must terminate +/// rather than spin inside the daemon's only SQLite connection. +#[test] +fn a_parent_cycle_is_unindexed_and_never_hangs_a_search() { + let db = Db::open_in_memory().unwrap(); + db.upsert_node(&folder("a", Some("b"), "Alphaville")) + .unwrap(); + db.upsert_node(&folder("b", Some("a"), "Betamax")).unwrap(); + + // Index lane: the cycle is not indexed, so it cannot be found. + assert!(db.search("alphaville", 10).unwrap().is_empty()); + assert!(db.search("betamax", 10).unwrap().is_empty()); + + // LIKE lane (below the trigram minimum): the rows are reachable, and what + // matters is that resolving their paths returns at all. + let hits = db.search("al", 10).unwrap(); + assert_eq!(hits.len(), 1); + assert!(hits[0].path.ends_with("Alphaville")); +} + #[test] fn search_short_query_like_fallback() { let db = Db::open_in_memory().unwrap(); @@ -2472,14 +2528,41 @@ fn pin_add_list_remove_roundtrip() { db.pin_add("vol~d", "docs", true).unwrap(); let list = db.pin_list().unwrap(); assert_eq!(list.len(), 2); - assert_eq!(list[0], ("vol~a".into(), "docs/a.txt".into(), false)); - assert_eq!(list[1], ("vol~d".into(), "docs".into(), true)); + // No `nodes` row for either pin, so the kind is unknown rather than guessed + // from the recursive flag. + assert_eq!( + list[0], + PinRow { + uid: "vol~a".into(), + path: "docs/a.txt".into(), + recursive: false, + is_dir: None, + } + ); + assert_eq!( + list[1], + PinRow { + uid: "vol~d".into(), + path: "docs".into(), + recursive: true, + is_dir: None, + } + ); + + // Once the node is cached, the pin reports the real kind — a folder pinned + // non-recursively must not look like a file. + db.upsert_node(&folder("root", None, "My Files")).unwrap(); + db.upsert_node(&folder("d", Some("root"), "docs")).unwrap(); + db.pin_add("vol~d", "docs", false).unwrap(); + let list = db.pin_list().unwrap(); + assert!(!list[1].recursive, "pin policy stays non-recursive"); + assert_eq!(list[1].is_dir, Some(true), "but the node is still a folder"); // Re-pin refreshes path/flag, not a duplicate row. db.pin_add("vol~a", "moved/a.txt", false).unwrap(); let list = db.pin_list().unwrap(); assert_eq!(list.len(), 2); - assert_eq!(list[0].1, "moved/a.txt"); + assert_eq!(list[0].path, "moved/a.txt"); assert!(db.pin_remove("vol~a").unwrap()); assert!(!db.pin_remove("vol~a").unwrap()); diff --git a/crates/pdfs-core/src/db/utils.rs b/crates/pdfs-core/src/db/utils.rs index a60462c..15a6125 100644 --- a/crates/pdfs-core/src/db/utils.rs +++ b/crates/pdfs-core/src/db/utils.rs @@ -31,19 +31,32 @@ pub(super) fn like_escape(s: &str) -> String { .replace('_', "\\_") } +/// How deep the parent walk may go before it gives up. Real Drive trees are +/// nowhere near this; a parent *cycle* is unbounded, and this query is on the +/// path of every search result, including the `LIKE` lane that reads `nodes` +/// directly and so never passed through the index's cycle check. +const MAX_PATH_DEPTH: usize = 256; + /// Resolve a node's mountpoint-relative path by walking `parent_uid` to the /// root via a recursive CTE. The root (the node with no parent) is excluded, so /// a top-level file `report.pdf` yields `"report.pdf"`, not `"My Files/report.pdf"`. +/// +/// A cycle in `parent_uid` is corrupt data the API can still hand us, and +/// `UNION ALL` over one never terminates — the walk is therefore depth-capped +/// and returns the truncated path rather than hanging the caller. That caller is +/// usually a search serving a keystroke, holding the daemon's only SQLite +/// connection. pub(super) fn path_of(conn: &Connection, uid: &str) -> Result { - let mut stmt = conn.prepare( + let mut stmt = conn.prepare(&format!( "WITH RECURSIVE anc(uid, parent_uid, name, depth) AS ( SELECT uid, parent_uid, name, 0 FROM nodes WHERE uid = ?1 UNION ALL SELECT n.uid, n.parent_uid, n.name, anc.depth + 1 FROM nodes n JOIN anc ON n.uid = anc.parent_uid + WHERE anc.depth < {MAX_PATH_DEPTH} ) - SELECT name, parent_uid FROM anc ORDER BY depth DESC", - )?; + SELECT name, parent_uid FROM anc ORDER BY depth DESC" + ))?; let rows = stmt.query_map(params![uid], |r| { Ok((r.get::<_, String>(0)?, r.get::<_, Option>(1)?)) })?; diff --git a/crates/pdfs-core/src/localindex.rs b/crates/pdfs-core/src/localindex.rs index 13324b1..80c477f 100644 --- a/crates/pdfs-core/src/localindex.rs +++ b/crates/pdfs-core/src/localindex.rs @@ -26,10 +26,20 @@ const SKIP_DIRS: &[&str] = &[ "venv", ".venv", "vendor", + "dist", "dist-newstyle", + "build", "Trash", ]; +/// Directory *paths* skipped when they end with one of these, for caches whose +/// own name is too generic to blacklist (`mod`). The Go module cache was 63,399 +/// of the 110,187 entries indexed on the machine this was measured on — 58% of +/// the index, crowding real files out of the bounded candidate pool the fuzzy +/// scorer draws from. The other big dependency caches (`.cargo`, `.rustup`, +/// `.gradle`, `.m2`) are already skipped by the hidden-file filter. +const SKIP_PATH_SUFFIXES: &[&str] = &["go/pkg/mod"]; + /// Hard cap on indexed entries, so a pathological home directory cannot grow the /// database without bound. Reaching it stops the walk early. const MAX_ENTRIES: usize = 400_000; @@ -90,7 +100,10 @@ pub fn scan( if excludes.iter().any(|e| path.starts_with(e)) { return false; } - !matches!(entry.file_name().to_str(), Some(name) if SKIP_DIRS.contains(&name)) + if matches!(entry.file_name().to_str(), Some(name) if SKIP_DIRS.contains(&name)) { + return false; + } + !is_skipped_path(path) }); let state = parking_lot::Mutex::new(SinkState { @@ -150,6 +163,17 @@ impl)> SinkState { } } +/// Whether `path` is (or is inside) one of the [`SKIP_PATH_SUFFIXES`] caches. +/// Matched on the path's own components so a directory merely *named* `mod` +/// keeps being indexed. +fn is_skipped_path(path: &Path) -> bool { + let path = path.to_string_lossy(); + SKIP_PATH_SUFFIXES.iter().any(|suffix| { + path.strip_suffix(suffix) + .is_some_and(|head| head.is_empty() || head.ends_with('/')) + }) +} + /// Convert a walker entry into a [`LocalEntry`], reusing the metadata the walker /// already fetched. Entries with non-UTF-8 paths are dropped: the index (and the /// JSON wire format) is UTF-8 only. @@ -221,4 +245,27 @@ mod tests { std::fs::remove_dir_all(&tmp).unwrap(); } + + /// The Go module cache is the single largest source of index noise on a + /// developer's machine (58% of the entries on the one this was measured + /// on), and it cannot be pruned by directory name: `mod` is far too + /// common a name to blacklist outright. + #[test] + fn the_go_module_cache_is_pruned_but_a_plain_mod_dir_is_not() { + let tmp = std::env::temp_dir().join(format!("pdfs-scan-gomod-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&tmp); + std::fs::create_dir_all(tmp.join("go/pkg/mod/example.com/lib")).unwrap(); + std::fs::create_dir_all(tmp.join("kernel/mod")).unwrap(); + std::fs::write(tmp.join("go/pkg/mod/example.com/lib/vendored.go"), b"x").unwrap(); + std::fs::write(tmp.join("kernel/mod/mine.conf"), b"x").unwrap(); + + let mut got = Vec::new(); + scan(std::slice::from_ref(&tmp), &[], |batch| got.extend(batch)); + + let names: Vec<&str> = got.iter().map(|e| e.name.as_str()).collect(); + assert!(!names.contains(&"vendored.go"), "{names:?}"); + assert!(names.contains(&"mine.conf"), "{names:?}"); + + std::fs::remove_dir_all(&tmp).unwrap(); + } } diff --git a/crates/pdfs-fuse/src/lib.rs b/crates/pdfs-fuse/src/lib.rs index ca78e47..7411fa7 100644 --- a/crates/pdfs-fuse/src/lib.rs +++ b/crates/pdfs-fuse/src/lib.rs @@ -5252,14 +5252,17 @@ mod tests { ); descendant.parent_uid = Some(shared_with_me_uid()); state.db.upsert_node(&descendant).unwrap(); - assert!( - state - .db - .search("AtomicRollbackFindable", 10) - .unwrap() - .is_empty(), - "a descendant with no synthetic ancestor starts outside FTS" - ); + // A node whose parent row is not cached — the synthetic root is not one + // until this publication commits — is indexed under the path it can + // actually be resolved to, which is its bare name. Publication reindexes + // the subtree beneath the synthetic ancestor, so that path is what the + // rollback has to restore. + let path_before = { + let hits = state.db.search("AtomicRollbackFindable", 10).unwrap(); + assert_eq!(hits.len(), 1, "the descendant starts out searchable"); + hits[0].path.clone() + }; + assert_eq!(path_before, "AtomicRollbackFindable"); // The pin is the transaction's final statement. Failing it proves the // earlier access, node, and descendant FTS work rolls back with it. @@ -5321,13 +5324,12 @@ mod tests { .is_none() ); assert_eq!(state.db.share_access(&shared_with_me_uid()).unwrap(), None); - assert!( - state - .db - .search("AtomicRollbackFindable", 10) - .unwrap() - .is_empty(), - "descendant FTS visibility must roll back with the failed pin" + let hits = state.db.search("AtomicRollbackFindable", 10).unwrap(); + assert_eq!(hits.len(), 1); + assert_eq!( + hits[0].path, path_before, + "the descendant's indexed path must roll back with the failed pin, \ + not keep the synthetic ancestor the publication tried to add" ); } diff --git a/crates/pdfs-fuse/src/profile.rs b/crates/pdfs-fuse/src/profile.rs index be3fc08..0e32eb0 100644 --- a/crates/pdfs-fuse/src/profile.rs +++ b/crates/pdfs-fuse/src/profile.rs @@ -149,10 +149,10 @@ impl Core { .pin_list() .map_err(|e| CoreError::internal(format!("db: {e:?}")))? .into_iter() - .map(|(uid, path, recursive)| ProfilePin { - uid, - path, - recursive, + .map(|row| ProfilePin { + uid: row.uid, + path: row.path, + recursive: row.recursive, }) .collect(); let cfg = pdfs_core::config::AppDirs::new() diff --git a/crates/pdfs-gui/src/dmenu.rs b/crates/pdfs-gui/src/dmenu.rs index 8efa6b7..d5df609 100644 --- a/crates/pdfs-gui/src/dmenu.rs +++ b/crates/pdfs-gui/src/dmenu.rs @@ -130,7 +130,7 @@ fn pins(socket: &Path) -> Result, String> { Hit::Drive(SearchHit { name: file_name(&pin.path), path: pin.path, - is_dir: pin.recursive, + is_dir: pin.is_dir.unwrap_or(pin.recursive), size: 0, modified: 0, pinned: true, diff --git a/crates/pdfs-gui/src/prompt.rs b/crates/pdfs-gui/src/prompt.rs index 792ba54..f4d7640 100644 --- a/crates/pdfs-gui/src/prompt.rs +++ b/crates/pdfs-gui/src/prompt.rs @@ -147,29 +147,26 @@ impl Hit { } /// The dimmed second line: the containing folder, as the user thinks of it. + /// + /// A Drive hit is only "My files" when it really sits in the primary mount. + /// A node inside a device folder resolves to a path on this machine + /// (`~/Downloads/…`) — the daemon has already worked that out in + /// `mounted_path`, and saying "My files / Downloads" for it names a folder + /// the user cannot find and does not match where activating the row opens. fn location(&self) -> String { match self { - Hit::Drive(h) => { - let parent = parent_of(&h.path); - if parent.is_empty() { - "My files".to_string() - } else { - format!("My files / {}", parent.replace('/', " / ")) + Hit::Drive(h) => match h.mounted_path.as_deref() { + Some(mounted) => home_relative(&parent_of(mounted)), + None => { + let parent = parent_of(&h.path); + if parent.is_empty() { + "My files".to_string() + } else { + format!("My files / {}", parent.replace('/', " / ")) + } } - } - Hit::Local(h) => { - let parent = parent_of(&h.path); - match dirs_home().and_then(|home| { - Path::new(&parent) - .strip_prefix(&home) - .ok() - .map(|rel| rel.display().to_string()) - }) { - Some(rel) if rel.is_empty() => "Home".to_string(), - Some(rel) => format!("Home / {}", rel.replace('/', " / ")), - None => parent, - } - } + }, + Hit::Local(h) => home_relative(&parent_of(&h.path)), } } @@ -869,7 +866,7 @@ impl Ui { .map(|pin| SearchHit { name: file_name(&pin.path), path: pin.path, - is_dir: pin.recursive, + is_dir: pin.is_dir.unwrap_or(pin.recursive), size: 0, modified: 0, pinned: true, @@ -1358,6 +1355,21 @@ fn file_name(path: &str) -> String { .to_string() } +/// An absolute directory path as "Home / …", falling back to the path itself +/// for anything outside the home directory. +fn home_relative(parent: &str) -> String { + match dirs_home().and_then(|home| { + Path::new(parent) + .strip_prefix(&home) + .ok() + .map(|rel| rel.display().to_string()) + }) { + Some(rel) if rel.is_empty() => "Home".to_string(), + Some(rel) => format!("Home / {}", rel.replace('/', " / ")), + None => parent.to_string(), + } +} + fn parent_of(path: &str) -> String { Path::new(path) .parent() diff --git a/docs/BUGS.md b/docs/BUGS.md index abfa9bf..ddb2126 100644 --- a/docs/BUGS.md +++ b/docs/BUGS.md @@ -12,6 +12,119 @@ Conventions: --- +## B82 — A `parent_uid` cycle hangs the daemon on any short search + +**Status:** Fixed (unverified) 2026-08-12 — unit-tested, not driven against a +real corrupted database (nobody has one). +**Found:** 2026-08-12, writing a test for B80. The test built a two-node cycle +and called `search`; `cargo test -p pdfs-core` then sat at +`search_excludes_a_parent_cycle` for 29 minutes with **zero CPU** before it was +killed. Two earlier runs had already been abandoned to the same stall without +the cause being understood — it looks exactly like a cargo lock contention +problem, which is what it was first blamed on. +**Where:** `path_of` (`crates/pdfs-core/src/db/utils.rs`), `upsert_node_tx`'s +`descendants` CTE (`db/nodes.rs`). + +**Root cause.** Both walks are recursive CTEs using `UNION ALL`, which does not +deduplicate, so `a.parent = b, b.parent = a` never terminates. The index side +was never exposed — `node_is_indexable_tx` rejects a cycle — but a query shorter +than `TRIGRAM_MIN` (3 chars) takes the `LIKE` lane, and **that lane reads +`nodes` directly and never consults the index**. Every row it returns is then +handed to `path_of`. So typing two characters into the prompt was enough, and +the spin happens while holding the daemon's only SQLite connection: the mount, +the drain, and every control request stop with it. + +The API is what would produce such a cycle (a rename/move race, a malformed +event), so this is not a "corrupt your own DB" scenario only. + +**Fix.** `path_of` is depth-capped at 256 and returns the truncated path rather +than hanging; `path_relative_to` already carried a 1024 cap, which is the +precedent. The `descendants` CTE in `upsert_node_tx` got the same guard for the +write side. Cycles also stay out of the index (see B80). + +**Still open:** `pin_is_pinned`'s ancestor walk (`db/pins.rs`) has the same +shape and no guard. Not reached from search, so it was left alone. + +## B81 — Prompt labels every Drive hit "My files", including device folders + +**Status:** Fixed (unverified) 2026-08-12 — needs a GUI/dmenu run. +**Found:** 2026-08-12, reading the prompt while fixing B80: with B80 fixed, a +device-folder hit would render as `My files / Downloads`, a folder that does not +exist under the primary mount, while activating the same row opens +`~/Downloads/…`. Label and action disagreed. +**Where:** `Hit::location()` (`crates/pdfs-gui/src/prompt.rs`), shared by the +GTK prompt and the dmenu front-end. + +The label was built from `SearchHit::path` with a hardcoded `My files /` prefix. +The daemon already resolves a device/sync-folder hit to a real local path in +`mounted_path` (`Core::mounted_search_path`), which is also what activation +uses; the label now derives from that when it is set, and falls back to +`My files` only for the primary mount. + +**Second defect, same area.** Pin rows set `is_dir` from `Pin::recursive` +(`prompt.rs`, `dmenu.rs`). That flag is pin *policy* — whether the subtree is +kept on disk — not node kind, so a folder pinned non-recursively drew a file +icon and sent an invalid `OpenFile`. `Pin` now carries an `is_dir: Option` +resolved from the `nodes` row by `pin_list`, serde-defaulted so an older daemon +(which omits it) still parses; front-ends fall back to `recursive` only when the +node is not cached. + +## B80 — 29% of the account is absent from the search index + +**Status:** Fixed (unverified) 2026-08-12 — migration replayed against a +read-only copy of the live DB; not yet run by the daemon itself. +**Found:** 2026-08-12, auditing `cache.db` after a user report that search felt +unreliable. `pdfs search tickets` returned only the My Files folder +`Documents/Tickets`, never the `tickets.pdf` sitting in the device folder +`Downloads`. Counting the tables explained it: + +``` +nodes 9252 +nodes_fts 6547 ← 2705 rows never indexed +nodes in orphan subtrees 2705 ← exactly the missing set +``` + +**Where:** `node_is_indexable_tx` (`crates/pdfs-core/src/db/nodes.rs`). + +**Root cause — the same one as B79.** A node was indexed only if walking +`parent_uid` reached a row with `parent_uid IS NULL`. Exactly one row is stored +that way (the My Files root). A device folder's root lives in `device` / +`sync_folder` and is **never persisted as a `nodes` row**, so `Downloads`, +`Pictures`, `Documents`, `Music` and `Videos` walk up to a parent that does not +exist and were judged unindexable, along with everything beneath them. Stale +`pdfs-acceptance-*` roots hit the same path. + +The v16 backfill did *not* share the bug — it rooted its walk at any node whose +parent is null **or absent**. So the index was correct immediately after +migrating and then lost each subtree the first time one of its nodes was +rewritten. That divergence between backfill and write path is why this survived: +a fresh install looks fine. + +**Fix.** Indexability now requires only that the walk terminates with nothing +trashed on the way; a cycle is still rejected explicitly (B82). `MIGRATION_V21` +(`SCHEMA_VERSION` → 21) rebuilds `nodes_fts` so existing installs recover +without waiting for each node to be rewritten, and fixes two gaps in the v16 +walk it replays: descendants of a trashed folder stay out, and the walk is +depth-capped. + +Replayed against a `.backup` of the live DB: **9276 non-trashed nodes → 9276 +indexed** (from 6547), 300 rows under `Downloads`, `tickets.pdf` at +`Downloads/tickets.pdf`. + +**Watch for.** That path is the second thing this uncovered: the backfill first +seeded an uncached-parent root with an empty path, so the migration indexed +`tickets.pdf` while a later upsert indexed `Downloads/tickets.pdf` — the same +node with two paths depending on which code wrote it. Any future rebuild of this +index has to match `path_of`, not invent its own root rule. + +**Related, not a defect.** The local half of the prompt indexes 110,187 files, +63,399 of them (58%) the Go module cache. Candidates are capped at 1000 and +scored *after* SQLite, so for a common term the pool filled with vendored files +before a real one was considered — for "test", 367 of the 500 best-ranked +candidates came from `go/pkg/mod`. `SKIP_DIRS` gained `dist`/`build`, a new +`SKIP_PATH_SUFFIXES` covers `go/pkg/mod`, and both candidate functions gained a +whole-query prefix lane ahead of the existing single-character one. + ## B79 — Every write in an on-demand device-folder mount fails EACCES **Status:** Fixed (verified live 2026-07-31, running in a locally rebuilt 1.2.1 diff --git a/docs/plans/search-index.md b/docs/plans/search-index.md new file mode 100644 index 0000000..f951dbe --- /dev/null +++ b/docs/plans/search-index.md @@ -0,0 +1,110 @@ +# Search index + prompt display fixes + +Working plan. Findings come from an audit of a real `cache.db` +(9,252 Drive nodes, 110,187 local files) on 2026-08-12. + +## Evidence + +``` +nodes 9252 +nodes_fts 6547 ← 2705 rows never indexed +nodes in orphan subtrees 2705 ← exactly the missing set +local_files 110187 + /home/narl/go/pkg/mod 63399 (58% of the local index) +query "test" 46405 local matches, candidate pool capped at 1000 + top 500 by fts rank 367 (73%) from go/pkg/mod +``` + +`pdfs search tickets` returns only `Documents/Tickets` (My Files); the +`tickets.pdf` sitting in the device folder `Downloads` is invisible. + +## P1 — 29% of Drive is unsearchable + +`node_is_indexable_tx` (`crates/pdfs-core/src/db/nodes.rs:606`) indexes a node +only when the ancestor walk terminates at a row with `parent_uid IS NULL`. The +DB holds exactly one such row (the My Files root). Device roots live in +`device`/`sync_folder`, never in `nodes`, so every device-folder subtree +(`Downloads`, `Pictures`, `Documents`, `Music`, `Videos` under device +*nthinkpad*) walks up to a `parent_uid` with no row and is judged unindexable. +Stale `pdfs-acceptance-*` roots hit the same path. + +- [x] Relax the rule to "the walk terminates and no ancestor is trashed", + keeping the existing cycle guard. A subtree whose top row's parent is + simply not cached is still real, reachable data. +- [x] `MIGRATION_V21`: rebuild `nodes_fts` under the new rule, so existing + installs recover the missing rows instead of waiting for each node to be + upserted again. +- [x] Tests: a node under an uncached parent is indexed; a trashed ancestor + still excludes; the migration backfills. + +### Found while testing: a parent cycle hangs the daemon + +Writing the cycle test surfaced a live defect, not a test artefact. A query +below the trigram minimum takes the `LIKE` lane, which reads `nodes` directly +and never consults the index, and every row it returns goes through `path_of` — +whose parent walk is an unguarded `UNION ALL`. On a `parent_uid` cycle it never +terminates, while holding the daemon's only SQLite connection. + +- [x] Depth-cap `path_of` (`db/utils.rs`), matching the 1024 cap + `path_relative_to` already carried. +- [x] Depth-cap the `descendants` CTE in `upsert_node_tx`, which has the same + exposure on the write side. +- [x] Keep cycles out of the index (part of the P1 rule above). + +Still unguarded, not touched here: `pin_is_pinned`'s ancestor walk. + +## P2 — local index noise crowds real files out of the candidate pool + +`SKIP_DIRS` (`crates/pdfs-core/src/localindex.rs:23`) misses the Go module +cache and build output dirs. `search_v2` clamps candidates to ≤1000 and scores +*after* SQLite, so for any common term the pool is exhausted by vendored files +before a real one is considered. + +- [x] Skip `go/pkg/mod` (path-relative, not a bare name), `dist`, `build`, + `.cache`-style artefact dirs. +- [x] Give `search_local_candidates` a name-prefix lane, mirroring what + `search_candidates` already does for Drive, so an exact-prefix match + cannot be starved by trigram rank. +- [x] Tests for both. + +## P3 — display lies about where a hit lives + +- [x] `Hit::location()` (`crates/pdfs-gui/src/prompt.rs:150`) hardcodes + `My files / …` for every Drive hit. Derive the label from + `mounted_path` when the daemon resolved one (device folder, sync folder) + and fall back to `My files` only for the primary mount. +- [x] Pin rows set `is_dir: pin.recursive` (`dmenu.rs:133`, `prompt.rs:872`) — + a non-recursively pinned folder renders and activates as a file. +- [ ] `pdfs locations` prints `Device folder 1..5` rather than the device and + folder name. (Cosmetic; deferred.) + +## Verification + +Done, 2026-08-12: + +``` +cargo fmt --all -- --check clean +cargo clippy --workspace --all-targets -- -D warnings clean +cargo test --workspace --locked 439 passed, 0 failed +``` + +The V21 backfill was also replayed against a read-only `.backup` of the live +`cache.db`: + +``` +live_nodes 9276 +indexed 9276 (was 6547) +downloads_subtree 300 +tickets.pdf -> Downloads/tickets.pdf +``` + +That run is what caught the last defect: the backfill originally seeded an +uncached-parent root with an empty path, so a node indexed by the migration +(`tickets.pdf`) and the same node after a later upsert (`Downloads/tickets.pdf`) +disagreed. The seed now keeps the name for any row that has a parent, matching +`path_of`. + +Still to do, on the live system: rebuild, restart `proton-drive.service`, and +confirm `pdfs search tickets` returns the device folder's `tickets.pdf`. The +schema move to 21 is one-way — an older `pdfs` refuses a v21 database. The +`local_files` shrink (~110k → ~47k) lands with the next background scan.