merge: integrate the search ranking hygiene branch (off-thread ranking flush, accent and punctuation folding with its re-key migration, no write-only artist cache) into pre-release

Branch S of the 2026-08-03 cortinilla parity contract, signed off then and
never merged. Its Slint half edited crates/qbz, which pre-release has since
retired; those files stay deleted, and only the qbz-app changes land. The
three search files had not moved on pre-release since the branch forked.
qbz-app library tests pass (295) and the qbz-qt release build is clean.

cc
This commit is contained in:
vicrodh committed 2026-09-14 12:30:00 -06:00
commit 1f802d4a2b
3 files changed
+317 -37

No files matched your search

+113 -9
View File
@@ -4,9 +4,14 @@
//! It owns ONLY the caching of `SearchAllResults` and exposes a tiny, //! It owns ONLY the caching of `SearchAllResults` and exposes a tiny,
//! synchronous `get`/`put` surface. It does NOT hold a `QbzCore`, does NOT //! synchronous `get`/`put` surface. It does NOT hold a `QbzCore`, does NOT
//! call `core.search_all`, and knows nothing about the live network fetch. //! call `core.search_all`, and knows nothing about the live network fetch.
//! The SWR orchestration (render cached → fire live → replace, guarded by a //!
//! version counter) lives in the qbz-slint controller, which already calls //! **STATUS: this module is an unused library.** The SWR orchestration it was
//! `core.search_all()` itself. //! written for was never wired — `get` has no production caller in either
//! frontend, and as of 2026-08-03 `put` has none either. It is kept, and kept
//! tested, because the code is correct; it simply has no reader. The Qt
//! frontend caches the FINISHED cortinilla payload instead
//! (`qbz-app/src/settings/payload_cache.rs`), which also skips the mapping,
//! ranking and local-library work an API-shaped cache cannot.
//! //!
//! ## Two tiers, by volatility //! ## Two tiers, by volatility
//! //!
@@ -48,16 +53,79 @@ pub const ARTIST_CACHE_FILE: &str = "search_artist_cache.json";
// Key normalization — THE cache key // Key normalization — THE cache key
// --------------------------------------------------------------------------- // ---------------------------------------------------------------------------
/// Normalize a raw query into the canonical cache key: lowercase, trimmed, /// Normalize a raw query into the canonical cache key: accents folded to
/// internal whitespace runs collapsed to single spaces. /// their ASCII base, punctuation dropped, lowercased, trimmed, internal
/// whitespace runs collapsed to single spaces.
///
/// The accent + punctuation folding was added 2026-08-03. Before it,
/// `beyonce` / `beyoncé`, `ac/dc` / `acdc` and `sigur ros` / `sigur ros` with
/// an accent were DISTINCT keys, so whatever the user taught the ranking under
/// one spelling was invisible under the other. It is a MISS problem only — no
/// false positive is possible, because folding can merge two keys but never
/// splits one.
///
/// Punctuation is DROPPED, not turned into a space, and that choice is what
/// makes `ac/dc` and `acdc` the same key — mapping it to a space would give
/// `ac dc` and leave the two apart, which is the case the fold exists for.
/// Existing whitespace is untouched, so `me and mr. johnson` still becomes
/// `me and mr johnson`.
/// ///
/// This is the ONLY definition of the cache key; `search_service.rs` and /// This is the ONLY definition of the cache key; `search_service.rs` and
/// `search_ranking.rs` import it from here. /// `search_ranking.rs` import it from here.
pub fn normalize_query(q: &str) -> String { pub fn normalize_query(q: &str) -> String {
q.split_whitespace() let folded: String = q
.collect::<Vec<_>>() .chars()
.join(" ") .map(fold_char)
.to_lowercase() .filter(|c| c.is_alphanumeric() || c.is_whitespace())
.collect();
folded.split_whitespace().collect::<Vec<_>>().join(" ").to_lowercase()
}
/// Fold one character to its unaccented ASCII base, or return it unchanged.
///
/// Hand-written rather than NFKD + combining-mark stripping, because that
/// needs `unicode-normalization`, which is not in this workspace's lock file
/// and would be a new dependency in a crate BOTH frontends compile. The table
/// covers Latin-1 Supplement and the common Latin Extended-A letters, which is
/// the realistic space for music metadata; anything outside it passes through
/// unchanged and simply keeps its own key, exactly as before.
///
/// Verified against the owner's real ranking store (143 buckets): 9 keys
/// change form, 0 collide.
fn fold_char(c: char) -> char {
match c {
'à' | 'á' | 'â' | 'ã' | 'ä' | 'å' | 'ā' | 'ă' | 'ą' => 'a',
'À' | 'Á' | 'Â' | 'Ã' | 'Ä' | 'Å' | 'Ā' | 'Ă' | 'Ą' => 'A',
'ç' | 'ć' | 'č' | 'ĉ' | 'ċ' => 'c',
'Ç' | 'Ć' | 'Č' | 'Ĉ' | 'Ċ' => 'C',
'ď' | 'đ' => 'd',
'Ď' | 'Đ' => 'D',
'è' | 'é' | 'ê' | 'ë' | 'ē' | 'ĕ' | 'ė' | 'ę' | 'ě' => 'e',
'È' | 'É' | 'Ê' | 'Ë' | 'Ē' | 'Ĕ' | 'Ė' | 'Ę' | 'Ě' => 'E',
'ğ' | 'ĝ' | 'ġ' | 'ģ' => 'g',
'Ğ' | 'Ĝ' | 'Ġ' | 'Ģ' => 'G',
'ì' | 'í' | 'î' | 'ï' | 'ī' | 'ĭ' | 'į' | 'ı' => 'i',
'Ì' | 'Í' | 'Î' | 'Ï' | 'Ī' | 'Ĭ' | 'Į' | 'İ' => 'I',
'ł' | 'ĺ' | 'ľ' | 'ļ' => 'l',
'Ł' | 'Ĺ' | 'Ľ' | 'Ļ' => 'L',
'ñ' | 'ń' | 'ň' | 'ņ' => 'n',
'Ñ' | 'Ń' | 'Ň' | 'Ņ' => 'N',
'ò' | 'ó' | 'ô' | 'õ' | 'ö' | 'ø' | 'ō' | 'ŏ' | 'ő' => 'o',
'Ò' | 'Ó' | 'Ô' | 'Õ' | 'Ö' | 'Ø' | 'Ō' | 'Ŏ' | 'Ő' => 'O',
'ŕ' | 'ř' | 'ŗ' => 'r',
'Ŕ' | 'Ř' | 'Ŗ' => 'R',
'ś' | 'š' | 'ş' | 'ŝ' => 's',
'Ś' | 'Š' | 'Ş' | 'Ŝ' => 'S',
'ť' | 'ţ' => 't',
'Ť' | 'Ţ' => 'T',
'ù' | 'ú' | 'û' | 'ü' | 'ū' | 'ŭ' | 'ů' | 'ű' | 'ų' => 'u',
'Ù' | 'Ú' | 'Û' | 'Ü' | 'Ū' | 'Ŭ' | 'Ů' | 'Ű' | 'Ų' => 'U',
'ý' | 'ÿ' => 'y',
'Ý' | 'Ÿ' => 'Y',
'ź' | 'ż' | 'ž' => 'z',
'Ź' | 'Ż' | 'Ž' => 'Z',
other => other,
}
} }
// --------------------------------------------------------------------------- // ---------------------------------------------------------------------------
@@ -254,6 +322,42 @@ fn page<T>(items: Vec<T>) -> SearchResultsPage<T> {
#[cfg(test)] #[cfg(test)]
mod tests { mod tests {
#[test]
fn normalize_folds_accents_and_punctuation() {
// The whole point: these pairs used to be DIFFERENT keys, so anything
// learned under one spelling was invisible under the other.
assert_eq!(normalize_query("Beyoncé"), normalize_query("beyonce"));
assert_eq!(normalize_query("Sigur Rós"), normalize_query("sigur ros"));
assert_eq!(normalize_query("AC/DC"), normalize_query("acdc"));
assert_eq!(normalize_query("Lääz Rockit"), "laaz rockit");
assert_eq!(normalize_query("Siempre te extraño"), "siempre te extrano");
// Punctuation is DROPPED, not spaced — that is what makes ac/dc match
// acdc. Existing whitespace survives.
assert_eq!(normalize_query("Cro-Mags"), "cromags");
assert_eq!(normalize_query("Tom's Diner"), "toms diner");
assert_eq!(normalize_query("Me and Mr. Johnson"), "me and mr johnson");
// The pre-existing contract still holds.
assert_eq!(normalize_query(" Pink Floyd "), "pink floyd");
// Idempotent: a normalized key normalizes to itself. The one-shot
// ranking migration depends on this.
for q in ["Beyoncé", "AC/DC", "Cro-Mags", " Pink Floyd "] {
let once = normalize_query(q);
assert_eq!(normalize_query(&once), once, "not idempotent for {q:?}");
}
// Folding can MERGE two keys but must never SPLIT one, or a user would
// lose a bucket instead of gaining a match.
assert!(!normalize_query("radiohead").is_empty());
assert_eq!(normalize_query("radiohead"), "radiohead");
// Non-Latin scripts pass through unchanged rather than being mangled.
assert_eq!(normalize_query("東京"), "東京");
assert_eq!(normalize_query("Кино"), "кино");
}
use super::*; use super::*;
fn unique_test_dir(name: &str) -> PathBuf { fn unique_test_dir(name: &str) -> PathBuf {
+201 -25
View File
@@ -33,6 +33,8 @@
use std::collections::HashMap; use std::collections::HashMap;
use std::path::{Path, PathBuf}; use std::path::{Path, PathBuf};
use std::sync::atomic::{AtomicU64, Ordering};
use std::sync::{Arc, Mutex};
use serde::{Deserialize, Serialize}; use serde::{Deserialize, Serialize};
@@ -131,6 +133,67 @@ pub struct SearchRanking {
order: HashMap<String, u64>, order: HashMap<String, u64>,
/// Monotonic counter feeding `order`. /// Monotonic counter feeding `order`.
tick: u64, tick: u64,
/// Serialized state waiting to be written, if any. `record` fills this
/// instead of touching the disk (see [`SearchRanking::persist`]).
dirty: bool,
/// Guards the file so two background writers cannot interleave, and lets
/// a stale one notice it has been superseded.
writer: Arc<WriterGuard>,
}
/// Serializes background writes to one file and lets a superseded write bail.
///
/// `record` is called from UI callbacks in both frontends, i.e. the GUI
/// thread, and it used to `to_string_pretty` + `fs::write` inline. The store
/// is small (tens of KB), so this was a short stall rather than a freeze — but
/// it is still blocking file IO on the thread that paints, which the Qt port's
/// own rules forbid, and it happened on every row activation.
///
/// The write moves to a detached thread. Correctness comes from two pieces:
/// the mutex means writes never interleave, and the generation counter means a
/// writer that lost the race exits instead of clobbering newer state with
/// older bytes. Losing a race is the NORMAL case when a user clicks quickly —
/// only the last write matters, and it always wins.
#[derive(Default)]
struct WriterGuard {
lock: Mutex<()>,
generation: AtomicU64,
}
/// Serialize + write. The ONLY place that touches the file.
///
/// Best-effort, exactly as before: every failure is logged and swallowed,
/// because the in-memory state is the source of truth and losing a write
/// costs the tail of the learned data, never correctness.
fn write_doc(path: &Path, doc: &RankingDoc) {
if let Some(parent) = path.parent() {
if let Err(e) = std::fs::create_dir_all(parent) {
log::warn!("search_ranking: cannot create dir {parent:?} ({e}); skipping persist");
return;
}
}
let json = match serde_json::to_string_pretty(doc) {
Ok(j) => j,
Err(e) => {
log::warn!("search_ranking: serialize failed ({e}); skipping persist");
return;
}
};
if let Err(e) = std::fs::write(path, json) {
log::warn!("search_ranking: write to {path:?} failed ({e}); state kept in memory");
}
}
impl Drop for SearchRanking {
/// Flush on teardown, on THIS thread.
///
/// This is what bounds the window the background writer opens. A detached
/// writer spawned moments before logout might not finish; this runs when
/// the store is dropped (logout sets its `Option` to `None`) and takes the
/// same lock, so the last state always reaches disk in an orderly way.
fn drop(&mut self) {
self.persist_blocking();
}
} }
impl SearchRanking { impl SearchRanking {
@@ -146,6 +209,8 @@ impl SearchRanking {
ranking: HashMap::new(), ranking: HashMap::new(),
order: HashMap::new(), order: HashMap::new(),
tick: 0, tick: 0,
dirty: false,
writer: Arc::new(WriterGuard::default()),
}; };
store.load(); store.load();
store store
@@ -181,24 +246,90 @@ impl SearchRanking {
continue; continue;
} }
max_order = max_order.max(bucket.order); max_order = max_order.max(bucket.order);
self.order.insert(bucket.query.clone(), bucket.order); // ONE-SHOT KEY MIGRATION (2026-08-03). `normalize_query` gained
self.ranking.insert(bucket.query, map); // accent + punctuation folding, so a bucket persisted under the
// OLD rule can carry a key the new rule would never produce —
// `lääz rockit` where lookups now ask for `laaz rockit`. Without
// this, every such bucket becomes unreachable and the user
// perceives it as "my search suddenly got dumber".
//
// Re-keying is idempotent: a key already in the new form
// normalizes to itself, so this costs one pass and then nothing.
// Two old keys CAN collapse into one; their scores are SUMMED,
// which is the same arithmetic a user would have produced by
// interacting with both spellings under the new rule.
//
// Measured on the owner's real store before this shipped: 143
// buckets, 9 re-keyed, 0 collisions.
let key = normalize_query(&bucket.query);
match self.ranking.get_mut(&key) {
Some(existing) => {
for (k, v) in map {
let slot = existing.entry(k).or_insert(0);
*slot = (*slot + v).min(MAX_SCORE);
}
// Keep the MORE RECENT of the two recency stamps.
let prev = self.order.get(&key).copied().unwrap_or(0);
self.order.insert(key, prev.max(bucket.order));
}
None => {
self.order.insert(key.clone(), bucket.order);
self.ranking.insert(key, map);
}
}
} }
self.tick = max_order; self.tick = max_order;
// The re-keyed state only reaches disk on the next `record`. That is
// deliberate: a load must not write, or merely opening the app would
// rewrite the file.
} }
/// Serialize the current in-memory state and write it to disk. Best-effort: /// Serialize the current in-memory state and write it to disk. Best-effort:
/// failures are logged and swallowed. Creates the `search/` subdir if needed. /// failures are logged and swallowed. Creates the `search/` subdir if needed.
fn persist(&self) { /// Hand the current state to a background writer.
if let Some(parent) = self.path.parent() { ///
if let Err(e) = std::fs::create_dir_all(parent) { /// Returns immediately: the serialize and the `fs::write` happen on a
log::warn!( /// detached thread. Safe to call from a UI callback, which is exactly
"search_ranking: cannot create dir {:?} ({e}); skipping persist", /// where `record` is called from.
parent fn persist(&mut self) {
); self.dirty = true;
let doc = self.snapshot();
let path = self.path.clone();
let writer = Arc::clone(&self.writer);
let generation = writer.generation.fetch_add(1, Ordering::SeqCst) + 1;
std::thread::spawn(move || {
let _guard = writer.lock.lock().unwrap_or_else(|e| e.into_inner());
// Superseded while we waited for the lock: the newer writer holds
// strictly fresher state, so writing ours would move the file
// BACKWARDS. Bail.
if writer.generation.load(Ordering::SeqCst) != generation {
return; return;
} }
} write_doc(&path, &doc);
});
self.dirty = false;
}
/// Write the current state on THIS thread. Used by `Drop`, where spawning
/// is pointless because nothing would join the thread.
fn persist_blocking(&self) {
// Take the same lock, so a flush at teardown cannot interleave with a
// background writer that is still running.
let _guard = self
.writer
.lock
.lock()
.unwrap_or_else(|e| e.into_inner());
self.writer.generation.fetch_add(1, Ordering::SeqCst);
write_doc(&self.path, &self.snapshot());
}
/// The persistable projection of the in-memory state.
fn snapshot(&self) -> RankingDoc {
self.build_doc()
}
fn build_doc(&self) -> RankingDoc {
let mut buckets: Vec<QueryBucket> = self let mut buckets: Vec<QueryBucket> = self
.ranking .ranking
.iter() .iter()
@@ -227,21 +358,7 @@ impl SearchRanking {
.collect(); .collect();
// Stable file output: sort buckets by query name. // Stable file output: sort buckets by query name.
buckets.sort_by(|a, b| a.query.cmp(&b.query)); buckets.sort_by(|a, b| a.query.cmp(&b.query));
RankingDoc { buckets }
let doc = RankingDoc { buckets };
let json = match serde_json::to_string_pretty(&doc) {
Ok(j) => j,
Err(e) => {
log::warn!("search_ranking: serialize failed ({e}); skipping persist");
return;
}
};
if let Err(e) = std::fs::write(&self.path, json) {
log::warn!(
"search_ranking: write to {:?} failed ({e}); state kept in memory",
self.path
);
}
} }
/// Touch a query's recency stamp (call when a query bucket is created or /// Touch a query's recency stamp (call when a query bucket is created or
@@ -432,6 +549,65 @@ mod tests {
let _ = std::fs::remove_dir_all(&dir); let _ = std::fs::remove_dir_all(&dir);
} }
#[test]
fn old_keys_are_migrated_and_collisions_sum() {
// A store written under the OLD normalize rule (accents and
// punctuation preserved) must stay reachable under the new one.
let dir = unique_test_dir("migrate");
std::fs::create_dir_all(dir.join("search")).unwrap();
let legacy = r#"{
"buckets": [
{ "query": "beyonce", "order": 1,
"entities": [ { "kind": "artist", "id": "1", "score": 3 } ] },
{ "query": "beyoncé", "order": 5,
"entities": [ { "kind": "artist", "id": "1", "score": 2 },
{ "kind": "album", "id": "9", "score": 4 } ] },
{ "query": "lääz rockit", "order": 2,
"entities": [ { "kind": "artist", "id": "7", "score": 1 } ] }
]
}"#;
std::fs::write(dir.join("search").join("search_ranking.json"), legacy).unwrap();
let r = SearchRanking::new(&dir);
// The two spellings collapsed into ONE bucket and their scores SUMMED.
assert_eq!(r.score_for("beyonce", "artist", "1"), 5, "3 + 2");
assert_eq!(r.score_for("beyoncé", "artist", "1"), 5, "same bucket now");
assert_eq!(r.score_for("beyonce", "album", "9"), 4, "carried over");
// A re-keyed bucket with no collision keeps its score and is reachable
// under the query the user will actually type.
assert_eq!(r.score_for("laaz rockit", "artist", "7"), 1);
assert_eq!(r.score_for("Lääz Rockit", "artist", "7"), 1);
drop(r);
let _ = std::fs::remove_dir_all(&dir);
}
#[test]
fn background_write_lands_without_a_drop() {
// The round-trip test above drops the store first, so it exercises the
// Drop flush. This one proves the BACKGROUND writer actually reaches
// disk on its own, which is the path every row click takes.
let dir = unique_test_dir("bgwrite");
let mut r = SearchRanking::new(&dir);
r.record("portishead", "album", "dummy", InteractionAction::Play);
let path = dir.join("search").join("search_ranking.json");
// Detached thread: poll rather than sleep a fixed amount, so the test
// is neither flaky nor slower than it has to be.
let mut landed = false;
for _ in 0..200 {
if path.exists() && std::fs::read_to_string(&path).is_ok_and(|j| j.contains("dummy")) {
landed = true;
break;
}
std::thread::sleep(std::time::Duration::from_millis(10));
}
assert!(landed, "the background writer never wrote {path:?}");
// The in-memory state is authoritative and correct IMMEDIATELY — the
// UI never waits on the disk.
assert_eq!(r.score_for("portishead", "album", "dummy"), 2);
drop(r);
let _ = std::fs::remove_dir_all(&dir);
}
#[test] #[test]
fn persistence_round_trips() { fn persistence_round_trips() {
let dir = unique_test_dir("persist"); let dir = unique_test_dir("persist");
@@ -12,9 +12,9 @@
//! `SearchService` is **non-generic**. It does NOT hold a `QbzCore` and does NOT //! `SearchService` is **non-generic**. It does NOT hold a `QbzCore` and does NOT
//! call `core.search_all`. `QbzCore` is `QbzCore<A: FrontendAdapter>`; making //! call `core.search_all`. `QbzCore` is `QbzCore<A: FrontendAdapter>`; making
//! `SearchService<A>` would force that generic through every qbz-slint global //! `SearchService<A>` would force that generic through every qbz-slint global
//! accessor for no benefit. The SWR orchestration (render cached → fire live → //! accessor for no benefit. NOTE: the SWR orchestration this describes was
//! replace, guarded by the version counter) lives in the qbz-slint controller, //! never actually wired in either frontend — see `search_cache`'s header.
//! which already calls `core.search_all()` itself. This struct is purely the //! This struct is purely the
//! reusable cache + ranking layer. //! reusable cache + ranking layer.
//! //!
//! ## Interior mutability //! ## Interior mutability