From 3a0f987c15fa1006655b1f7abde7760f7434dc1b Mon Sep 17 00:00:00 2001 From: Bob Matsuoka Date: Thu, 6 Aug 2026 10:54:23 -0400 Subject: [PATCH 1/8] fix(trusty-common): persist the HNSW vector-id allocator so two live stores cannot alias drawers (#5005) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `HnswStore` allocated vector ids from a process-local `AtomicU64` seeded at open from `max(VECTORS, VECTOR_KEYS) + 1`, and `upsert` inserted at the allocated id with no uniqueness check. Two live stores over one palace file — a configuration the in-process vector-db cache deliberately supports, since it hands the same `Arc` to every `UsearchStore` opened for a palace — each seeded a private counter from the same high-water mark and then issued the same ids. `VECTOR_KEYS` aliased several drawers onto one `vector_id` and `VECTORS` overwrote in place, silently: on the live `trusty-tools` palace, `vector_id` 988 is shared by 5 uuids and 4 of them are embedded nowhere. The counter now lives in redb (`vector_id_seq`) and is reserved inside the same write transaction that writes the row claiming it, so every writer on the file serialises against it. `upsert` additionally refuses an id that already has a `VECTORS` row — it allocates past it, or fails with `IdAllocationFailed`, but never overwrites. Existing palaces have no counter row. Every read-write open raises it to `max(VECTORS, VECTOR_KEYS) + 1`, as a max rather than a set-if-missing, so the seed is idempotent and a rolling upgrade that interleaves a pre-fix binary cannot leave the counter behind the tables. Detection, which #5000 also needs: `embed_health` and `palace_reembed` now report `vector_key_rows`, `distinct_vector_ids`, and the aliased drawer ids. Key presence — the only thing they checked before — reported a false all-clear for this class, and `is_healthy()` is now false when any drawer is aliased. Closes #5005 🤖🤖🤖 Generated with trusty-mpm — https://github.com/bobmatnyc/trusty-tools --- .../changelog.d/5005-hnsw-id-aliasing.md | 6 + .../src/memory_core/retrieval/embed_repair.rs | 40 +- .../retrieval/embed_repair_tests.rs | 126 +++++ .../src/memory_core/store/hnsw_store.rs | 471 +++++++++------- .../src/memory_core/store/hnsw_store/tests.rs | 527 ++++++++++++++++++ .../src/memory_core/store/kg_store.rs | 22 + .../src/memory_core/store/vector.rs | 382 ++----------- .../src/memory_core/store/vector/tests.rs | 339 +++++++++++ crates/trusty-memory/src/tools/palace_ops.rs | 10 + 9 files changed, 1398 insertions(+), 525 deletions(-) create mode 100644 crates/trusty-common/changelog.d/5005-hnsw-id-aliasing.md create mode 100644 crates/trusty-common/src/memory_core/store/hnsw_store/tests.rs create mode 100644 crates/trusty-common/src/memory_core/store/vector/tests.rs diff --git a/crates/trusty-common/changelog.d/5005-hnsw-id-aliasing.md b/crates/trusty-common/changelog.d/5005-hnsw-id-aliasing.md new file mode 100644 index 000000000..bc0ea7d85 --- /dev/null +++ b/crates/trusty-common/changelog.d/5005-hnsw-id-aliasing.md @@ -0,0 +1,6 @@ +Fixed + +- `HnswStore` no longer aliases vector ids across two live stores over one palace file, which silently overwrote one drawer's embedding with another's (closes [#5005](https://github.com/bobmatnyc/trusty-tools/issues/5005)) + - the vector-id counter now lives in redb (`vector_id_seq`) and is reserved inside the same write transaction as the insert, so every writer on the file serialises against it; an existing palace has its counter seeded to the file's high-water mark on open, and re-raised on every subsequent open so a rolling upgrade cannot leave it behind + - `upsert` refuses an id that already has a `VECTORS` row: it allocates past it, or fails with `IdAllocationFailed` — it never overwrites + - `PalaceHandle::embed_health` and `palace_reembed` now report `vector_key_rows`, `distinct_vector_ids`, and the aliased drawer ids; key presence alone reported a false all-clear for this class, and `is_healthy()` is now false when any drawer is aliased diff --git a/crates/trusty-common/src/memory_core/retrieval/embed_repair.rs b/crates/trusty-common/src/memory_core/retrieval/embed_repair.rs index 2cc7c8428..b5862e600 100644 --- a/crates/trusty-common/src/memory_core/retrieval/embed_repair.rs +++ b/crates/trusty-common/src/memory_core/retrieval/embed_repair.rs @@ -58,12 +58,25 @@ pub struct EmbedHealth { pub recorded_failures: Vec, /// Whether a shared embedder has initialised in this process. pub embedder_ready: bool, + /// Rows in the `VECTOR_KEYS` table (#5005). + pub vector_key_rows: usize, + /// Distinct vector ids those rows point at (#5005). Below + /// `vector_key_rows` exactly when drawers share an id. + pub distinct_vector_ids: usize, + /// Drawers whose vector was overwritten by another drawer's (#5005). These + /// have a key, so they are NOT in `missing_vector_ids` — that is precisely + /// why the count gap missed them — but their content is embedded nowhere. + pub aliased_drawer_ids: Vec, } impl EmbedHealth { /// Whether every live drawer is vector-searchable. + /// + /// #5005: an aliased drawer has a vector key and still resolves to nothing, + /// so key presence alone is not the health condition. Both sets must be + /// empty. pub fn is_healthy(&self) -> bool { - self.missing_vector_ids.is_empty() + self.missing_vector_ids.is_empty() && self.aliased_drawer_ids.is_empty() } } @@ -117,6 +130,14 @@ pub struct VectorBackfillReport { /// Ids still without a vector after this run (including any skipped by /// `limit`), so a caller can act on the remainder. pub still_missing_ids: Vec, + /// Drawers sharing a vector id with another drawer (#5005). This run never + /// repairs them — a re-embed alone would not, since they already have a + /// key. A non-zero value means the palace is NOT clean however small + /// `missing` is, and it is the number a deletion-bearing workflow must gate + /// on alongside `missing` (#5000 resolution item 3). + pub aliased: usize, + /// The ids behind `aliased`. + pub aliased_ids: Vec, } impl PalaceHandle { @@ -149,6 +170,18 @@ impl PalaceHandle { .as_ref() .map(|d| embed_ledger::load(d)) .unwrap_or_default(); + // #5005: an aliased drawer has a key, so it is invisible to the set + // difference above. A redb scan failure must not be reported as "no + // aliasing" — log it and leave the counts at zero, which reads as + // unknown rather than clean because the row count is zero too. + let (vector_key_rows, distinct_vector_ids, aliased_drawer_ids) = + match self.vector_store.alias_audit() { + Ok(triple) => triple, + Err(e) => { + tracing::warn!(palace = %self.id, "#5005: alias audit failed: {e:#}"); + (0, 0, Vec::new()) + } + }; EmbedHealth { palace_id: self.id.as_str().to_string(), drawer_count: live.len(), @@ -156,6 +189,9 @@ impl PalaceHandle { missing_vector_ids, recorded_failures, embedder_ready: shared_embedder_initialized(), + vector_key_rows, + distinct_vector_ids, + aliased_drawer_ids, } } @@ -191,6 +227,8 @@ impl PalaceHandle { repaired: 0, still_failing: 0, still_missing_ids: health.missing_vector_ids.clone(), + aliased: health.aliased_drawer_ids.len(), + aliased_ids: health.aliased_drawer_ids.clone(), }; // A healthy palace short-circuits BEFORE touching the embedder, so the diff --git a/crates/trusty-common/src/memory_core/retrieval/embed_repair_tests.rs b/crates/trusty-common/src/memory_core/retrieval/embed_repair_tests.rs index 561316485..517483381 100644 --- a/crates/trusty-common/src/memory_core/retrieval/embed_repair_tests.rs +++ b/crates/trusty-common/src/memory_core/retrieval/embed_repair_tests.rs @@ -734,3 +734,129 @@ fn concurrent_clear_and_record_do_not_lose_each_other() { ); } } + +// ── 6. #5005 — id aliasing is detectable and repairable ───────────────────── + +/// Build the palace's vector redb file with `uuids` all mapped onto one +/// `vector_id`, the state a pre-#5005 double-open left behind. +/// +/// Why: after the allocator fix this state can no longer be produced through +/// `upsert`, so the fixture has to be written at the redb level. The handle is +/// dropped before returning, releasing the file lock so `make_handle` can open +/// it normally. +/// What: creates `/idx.usearch.redb`, writes one 384-d vector at +/// `shared_id` and a `VECTOR_KEYS` row per uuid pointing at it. +/// Test: used by `alias_audit_surfaces_a_collision`. +fn seed_aliased_vector_file(dir: &std::path::Path, shared_id: u64, uuids: &[Uuid]) { + use crate::memory_core::store::kg_store::{VECTOR_KEYS, VECTORS}; + use redb::Database; + let db = Database::create(dir.join("idx.usearch.redb")).expect("create vector redb"); + let encoded = postcard::to_allocvec(&vec![0.05_f32; 384]).expect("encode"); + let wtx = db.begin_write().expect("begin"); + { + let mut vectors = wtx.open_table(VECTORS).expect("vectors"); + let mut keys = wtx.open_table(VECTOR_KEYS).expect("keys"); + vectors.insert(shared_id, encoded.as_slice()).expect("vec"); + for u in uuids { + keys.insert(u.to_string().as_str(), shared_id).expect("key"); + } + } + wtx.commit().expect("commit"); + drop(db); // release the flock so the palace can open the file +} + +/// Why (#5005): `palace_reembed` reported `missing: 0` for a palace with four +/// unretrievable drawers, because an aliased drawer HAS a vector key. Health +/// that only set-differences drawer ids against vector keys cannot see it, and +/// a deletion-bearing workflow gating on `missing` would have read that as a +/// clean bill of health. +/// What: seeds three drawers sharing one `vector_id`, opens the palace over +/// that file, and asserts `embed_health` reports zero missing (the false +/// all-clear, unchanged) AND names all three as aliased, with the key-row / +/// distinct-id arithmetic that detects it. `is_healthy` must be false. +/// Test: itself. Making `embed_health` drop the alias audit leaves +/// `aliased_drawer_ids` empty and `is_healthy()` true — both assertions fail. +#[test] +fn alias_audit_surfaces_a_collision() { + let dir = tempfile::tempdir().unwrap(); + // Build the drawers first: `Drawer::new`'s first argument is the ROOM id and + // the drawer id is generated, so the vector file has to be seeded with the + // ids the drawers actually carry. + let room = Uuid::new_v4(); + let drawers: Vec = (0..3) + .map(|_| Drawer::new(room, "aliased content")) + .collect(); + let mut ids: Vec = drawers.iter().map(|d| d.id).collect(); + ids.sort(); + seed_aliased_vector_file(dir.path(), 988, &ids); + + let handle = make_handle(dir.path()); + for d in drawers { + handle.add_drawer(d); + } + + let health = handle.embed_health(); + assert!( + health.missing_vector_ids.is_empty(), + "the false all-clear this ticket is about: every aliased drawer has a key" + ); + assert_eq!(health.vector_key_rows, 3); + assert_eq!( + health.distinct_vector_ids, 1, + "three keys, one id — the gap is the detector" + ); + let mut aliased = health.aliased_drawer_ids.clone(); + aliased.sort(); + assert_eq!(aliased, ids, "every member of the group must be named"); + assert!( + !health.is_healthy(), + "a palace with aliased drawers is not healthy however small `missing` is" + ); +} + +/// Why (#5005 repair): freeing the group is what turns an invisible alias into +/// an ordinary missing drawer the existing backfill can repair. Code only — +/// this was never run against a live palace in the PR that added it. +/// What: seeds the same three-way collision, calls `UsearchStore::unalias`, and +/// asserts health flips from "0 missing, 3 aliased" to "3 missing, 0 aliased". +/// Test: itself. A repair that spared the reachable member frees 2 and leaves +/// `missing` at 2, failing both counts. +#[test] +fn unalias_marks_the_whole_group_for_reembed() { + let dir = tempfile::tempdir().unwrap(); + // Build the drawers first: `Drawer::new`'s first argument is the ROOM id and + // the drawer id is generated, so the vector file has to be seeded with the + // ids the drawers actually carry. + let room = Uuid::new_v4(); + let drawers: Vec = (0..3) + .map(|_| Drawer::new(room, "aliased content")) + .collect(); + let mut ids: Vec = drawers.iter().map(|d| d.id).collect(); + ids.sort(); + seed_aliased_vector_file(dir.path(), 988, &ids); + + let handle = make_handle(dir.path()); + for d in drawers { + handle.add_drawer(d); + } + + let freed = handle.vector_store.unalias().expect("unalias"); + assert_eq!( + freed.len(), + 3, + "the whole group is freed, not just the losers" + ); + + let after = handle.embed_health(); + assert!( + after.aliased_drawer_ids.is_empty(), + "no group survives the repair" + ); + let mut missing = after.missing_vector_ids.clone(); + missing.sort(); + assert_eq!( + missing, ids, + "the freed drawers must now read as ordinary missing, which the backfill repairs" + ); + assert!(!after.is_healthy(), "they still need a re-embed"); +} diff --git a/crates/trusty-common/src/memory_core/store/hnsw_store.rs b/crates/trusty-common/src/memory_core/store/hnsw_store.rs index cf4c75096..bcd006a71 100644 --- a/crates/trusty-common/src/memory_core/store/hnsw_store.rs +++ b/crates/trusty-common/src/memory_core/store/hnsw_store.rs @@ -20,14 +20,15 @@ use std::collections::HashMap; use std::sync::Arc; -use std::sync::atomic::{AtomicU64, Ordering}; use hnsw_rs::prelude::{DistCosine, Hnsw}; use parking_lot::RwLock; -use redb::{Database, ReadableDatabase, ReadableTable, ReadableTableMetadata}; +use redb::{Database, ReadableDatabase, ReadableTable, ReadableTableMetadata, Table}; use thiserror::Error; -use crate::memory_core::store::kg_store::{DELETED_VECTORS, VECTOR_KEYS, VECTORS}; +use crate::memory_core::store::kg_store::{ + DELETED_VECTORS, NEXT_VECTOR_ID, VECTOR_ID_SEQ, VECTOR_KEYS, VECTORS, +}; /// Default HNSW connectivity. Maps to `max_nb_connection` in `hnsw_rs`. /// @@ -58,6 +59,20 @@ const HNSW_INITIAL_CAPACITY: usize = 1024; /// stable for small `top_k`. const HNSW_DEFAULT_EF_SEARCH: usize = 64; +/// How many candidate ids the allocator may probe before it gives up. +/// +/// Why (#5005): the persisted counter is the mechanism that keeps ids unique; +/// the occupancy probe in [`allocate_vector_id`] is the backstop for a counter +/// that is somehow behind the table (a palace written by a pre-#5005 binary +/// between two opens, a hand-edited file). Each probe jumps past the highest +/// occupied id, so one iteration resolves any real collision — a budget this +/// small only runs out if the table is being mutated underneath a write +/// transaction, which redb does not permit. Bounded so a corrupt file makes +/// `upsert` fail loudly instead of spinning. +/// What: 8 attempts, then [`HnswStoreError::IdAllocationFailed`]. +/// Test: `upsert_refuses_to_reuse_an_id_already_present_in_vectors`. +const MAX_ALLOC_PROBES: u8 = 8; + /// Why: A structured error type lets callers distinguish redb failures /// from postcard decode failures and from UUID parse failures without /// pattern-matching on stringly-typed `anyhow::Error` payloads. @@ -86,6 +101,15 @@ pub enum HnswStoreError { InvalidUuid(#[from] uuid::Error), #[error("vector dimension mismatch: expected {expected}, got {got}")] DimensionMismatch { expected: usize, got: usize }, + /// The allocator could not find a free `vector_id` within + /// [`MAX_ALLOC_PROBES`]. Fails the upsert rather than overwriting a live + /// vector (#5005). + #[error( + "could not allocate a free vector_id after {probes} attempts \ + (last candidate {last_candidate} is already present in VECTORS); \ + refusing to overwrite a live vector — the palace index needs a rebuild" + )] + IdAllocationFailed { probes: u8, last_candidate: u64 }, /// Returned by every write method when the store is in snapshot /// (read-only) mode. Callers should surface this verbatim — the /// message is the canonical guidance for issue #59. @@ -128,6 +152,78 @@ impl From for HnswStoreError { } } +/// Reserve the next free `vector_id`, inside the caller's write transaction. +/// +/// Why (#5005): the old allocator was a process-local `AtomicU64` seeded at +/// open. Two live `HnswStore`s over one database file — which the vector-db +/// cache deliberately allows, so that a palace opened twice in one process +/// shares a single redb handle — each seeded from the same high-water mark and +/// then issued the same ids. Because redb permits one write transaction at a +/// time per database, moving the reservation into the transaction that does the +/// insert makes it serialisable with every other writer on that file: the id +/// and the row that claims it commit or roll back together. +/// What: reads `VECTOR_ID_SEQ`, takes the first candidate with no `VECTORS` +/// row, writes `candidate + 1` back, and returns the candidate. When the +/// candidate IS occupied — a counter left behind by a pre-#5005 binary, or a +/// hand-edited file — it jumps past the highest occupied id rather than +/// overwriting, logs the correction, and retries; after [`MAX_ALLOC_PROBES`] +/// it fails with [`HnswStoreError::IdAllocationFailed`] instead of aliasing. +/// A missing counter row (only reachable if the store skipped open-time +/// seeding) falls back to the same high-water mark the seed would have used. +/// Test: `upsert_refuses_to_reuse_an_id_already_present_in_vectors`, +/// `two_live_stores_over_one_file_never_alias_ids`. +fn allocate_vector_id( + seq: &mut Table<'_, &'static str, u64>, + vectors: &Table<'_, u64, &'static [u8]>, +) -> Result { + let mut candidate = match seq.get(NEXT_VECTOR_ID)? { + Some(g) => g.value(), + None => { + tracing::warn!( + "#5005: VECTOR_ID_SEQ has no counter row at allocation time; \ + falling back to the VECTORS high-water mark" + ); + high_water(vectors)? + } + }; + + for probe in 0..MAX_ALLOC_PROBES { + if vectors.get(candidate)?.is_none() { + seq.insert(NEXT_VECTOR_ID, candidate.saturating_add(1))?; + return Ok(candidate); + } + tracing::warn!( + candidate, + probe, + "#5005: persisted vector_id counter is behind the VECTORS table; \ + skipping the occupied id instead of overwriting it" + ); + // Jump past the highest occupied id so one correction is enough. + candidate = high_water(vectors)?.max(candidate.saturating_add(1)); + } + + Err(HnswStoreError::IdAllocationFailed { + probes: MAX_ALLOC_PROBES, + last_candidate: candidate, + }) +} + +/// One past the highest `vector_id` present in `VECTORS` (1 when empty). +/// +/// Why: both the allocator's collision jump and its missing-counter fallback +/// need the same "no id at or above this is taken" bound. `VECTORS` is a +/// B-tree keyed by `u64`, so `last()` is O(log n) — cheap enough to call on the +/// rare correction path. +/// What: `last()? + 1`, or 1 for an empty table (id 0 is never issued, matching +/// the pre-#5005 seed of `max_seen + 1`). +/// Test: exercised by `upsert_refuses_to_reuse_an_id_already_present_in_vectors`. +fn high_water(vectors: &Table<'_, u64, &'static [u8]>) -> Result { + Ok(match vectors.last()? { + Some((k, _)) => k.value().saturating_add(1), + None => 1, + }) +} + /// Public result alias to keep call-site signatures concise. /// /// Why: Most call sites only need `Result` and @@ -136,6 +232,44 @@ impl From for HnswStoreError { /// Test: Used by every public method below. pub type Result = std::result::Result; +/// How many drawer keys point at a `vector_id` that another key also points at. +/// +/// Why (#5005 / #5000): every count-based health signal this palace had could +/// be zero while four drawers were unretrievable — `drawer_count` matched, +/// `palace_reembed` reported nothing missing, and recall could still hit +/// lexically. `key_rows` versus `distinct_vector_ids` is the one comparison +/// that cannot be fooled by it, because it never leaves `VECTOR_KEYS`. +/// What: the two counts plus the offending groups, each `(vector_id, uuids)`. +/// Test: `audit_detects_two_uuids_mapped_to_one_id`. +#[derive(Debug, Clone, Default)] +pub struct AliasAudit { + /// Rows in `VECTOR_KEYS` — one per drawer that has ever been embedded. + pub key_rows: usize, + /// Distinct `vector_id`s those rows point at. Equal to `key_rows` iff no + /// drawer's vector has been overwritten by another drawer's. + pub distinct_vector_ids: usize, + /// Every `vector_id` claimed by more than one uuid, with its uuids sorted. + pub aliased: Vec<(u64, Vec)>, +} + +impl AliasAudit { + /// Drawer keys caught in a collision — the count an operator acts on. + /// + /// Why: `aliased.len()` counts groups, not drawers, and the number that + /// matters is how many drawers are affected. Equals + /// `key_rows - distinct_vector_ids` on a consistent audit. + /// What: sums the group sizes. + /// Test: `audit_detects_two_uuids_mapped_to_one_id`. + pub fn aliased_key_count(&self) -> usize { + self.aliased.iter().map(|(_, uuids)| uuids.len()).sum() + } + + /// Whether no drawer's vector is shared with another drawer. + pub fn is_clean(&self) -> bool { + self.aliased.is_empty() + } +} + /// Pure-Rust HNSW store backed by redb for persistence (issue #50). /// /// Why: We need durable HNSW search without a C++ FFI dependency. @@ -145,15 +279,15 @@ pub type Result = std::result::Result; /// minor versions. The cost is a one-time O(N) re-insertion per open; /// real-world palaces are bounded to ~10⁵ vectors so this is acceptable. /// What: Holds `Arc` for persistence, the in-memory `Hnsw` wrapped in `Arc>` for concurrent reads, the -/// embedding dimension (for validation), and an `AtomicU64` monotonic -/// vector_id counter (seeded from `max(VECTOR_KEYS) + 1` on open). +/// DistCosine>` wrapped in `Arc>` for concurrent reads, and the +/// embedding dimension (for validation). The vector_id counter is NOT held +/// here — it lives in redb (`VECTOR_ID_SEQ`), because a per-store counter is +/// exactly the #5005 aliasing bug. /// Test: See `tests::upsert_and_search_round_trips` and friends. pub struct HnswStore { db: Arc, index: Arc>>, dim: usize, - next_id: AtomicU64, /// When true, every write method short-circuits with a "read-only" /// error before touching redb. /// @@ -174,12 +308,12 @@ impl HnswStore { /// drawer/triple writes and vector upserts can be coordinated. Passing /// in `Arc` (rather than a path) lets the caller own the /// connection cache and reuse the same handle as `KgStoreRedb`. - /// What: Touches `VECTORS` / `VECTOR_KEYS` / `DELETED_VECTORS` to - /// create them if missing, then reads every `(vector_id, vec)` row from - /// `VECTORS` (skipping tombstoned ids) and replays them into a fresh - /// in-memory `Hnsw` index. Seeds `next_id` from - /// `max(VECTOR_KEYS.value()) + 1` so subsequent upserts never collide - /// with an existing id. + /// What: Touches `VECTORS` / `VECTOR_KEYS` / `DELETED_VECTORS` / + /// `VECTOR_ID_SEQ` to create them if missing, then reads every + /// `(vector_id, vec)` row from `VECTORS` (skipping tombstoned ids) and + /// replays them into a fresh in-memory `Hnsw` index. + /// Raises the persisted `VECTOR_ID_SEQ` counter to at least + /// `max(VECTORS, VECTOR_KEYS) + 1` (#5005). /// Test: `hydration_restores_index`. pub fn open(db: Arc, dim: usize) -> Result { Self::open_with_mode(db, dim, false) @@ -208,6 +342,7 @@ impl HnswStore { let _ = wtx.open_table(VECTORS)?; let _ = wtx.open_table(VECTOR_KEYS)?; let _ = wtx.open_table(DELETED_VECTORS)?; + let _ = wtx.open_table(VECTOR_ID_SEQ)?; } wtx.commit()?; } @@ -273,11 +408,34 @@ impl HnswStore { } } + // #5005: raise the persisted allocator to the file's high-water mark. + // + // This is both the migration for a palace written before `VECTOR_ID_SEQ` + // existed (no row → seeded here) and the repair for one that a + // pre-#5005 binary wrote to between two opens (row present but stale → + // raised here). Written as a max, never a plain set, so it is + // idempotent and can only move forward: a rolling upgrade that + // interleaves an old binary can leave the counter behind the tables, + // and the next open of a fixed binary pulls it back up before any id is + // issued. Skipped in read-only/snapshot mode, where every write path + // returns `ReadOnly` before it could allocate anything. + if !read_only { + let wtx = db.begin_write()?; + { + let mut seq = wtx.open_table(VECTOR_ID_SEQ)?; + let current = seq.get(NEXT_VECTOR_ID)?.map(|g| g.value()).unwrap_or(0); + let floor = max_seen.saturating_add(1); + if current < floor { + seq.insert(NEXT_VECTOR_ID, floor)?; + } + } + wtx.commit()?; + } + Ok(Self { db, index: Arc::new(RwLock::new(index)), dim, - next_id: AtomicU64::new(max_seen.saturating_add(1)), read_only, }) } @@ -311,7 +469,15 @@ impl HnswStore { /// `VECTORS`, writes the UUID→id mapping to `VECTOR_KEYS`, and removes /// any prior tombstone for this id. Then inserts the vector into the /// in-memory graph. - /// Test: `upsert_and_search_round_trips`. + /// + /// #5005: allocation for a NEW uuid reads and bumps the persisted + /// `VECTOR_ID_SEQ` counter inside this same write transaction, so two live + /// stores over one file serialise against each other instead of each + /// handing out ids from a private counter. [`allocate_vector_id`] also + /// refuses to return an id that already has a `VECTORS` row, so an upsert + /// can never silently overwrite another drawer's vector. + /// Test: `upsert_and_search_round_trips`, + /// `two_live_stores_over_one_file_never_alias_ids`. pub fn upsert(&self, uuid: &str, vector: &[f32]) -> Result { if self.read_only { return Err(HnswStoreError::ReadOnly); @@ -330,6 +496,7 @@ impl HnswStore { let mut vectors = wtx.open_table(VECTORS)?; let mut keys = wtx.open_table(VECTOR_KEYS)?; let mut tombstones = wtx.open_table(DELETED_VECTORS)?; + let mut seq = wtx.open_table(VECTOR_ID_SEQ)?; // Resolve the existing id in a scoped block so the AccessGuard // (immutable borrow of `keys`) is dropped before we re-borrow @@ -338,7 +505,8 @@ impl HnswStore { vector_id = match existing { Some(id) => id, None => { - let id = self.next_id.fetch_add(1, Ordering::SeqCst); + // #5005: allocate from redb, inside this txn. + let id = allocate_vector_id(&mut seq, &vectors)?; keys.insert(uuid, id)?; id } @@ -501,6 +669,99 @@ impl HnswStore { Ok(out) } + /// Count how many drawer keys share a `vector_id` with another key. + /// + /// Why (#5005, and requirement 1 of #5000): key PRESENCE is what + /// `palace_reembed` tests, and an aliased drawer has a key — so the palace + /// that lost four drawers to id collisions reported zero missing. The + /// signal that does catch it is arithmetic on `VECTOR_KEYS` alone: one row + /// per drawer, one distinct id per row, so any shortfall between the two is + /// exactly the number of drawers whose vector belongs to someone else. + /// What: one pass over `VECTOR_KEYS` building id → uuids. Returns the row + /// count, the distinct-id count, and every group with more than one uuid. + /// Cheap: `VECTOR_KEYS` is one small row per drawer and holds no vectors. + /// Test: `audit_detects_two_uuids_mapped_to_one_id`. + pub fn audit_aliases(&self) -> Result { + let mut by_id: HashMap> = HashMap::new(); + let mut key_rows = 0usize; + { + let rtx = self.db.begin_read()?; + let keys = rtx.open_table(VECTOR_KEYS)?; + for entry in keys.iter()? { + let (k, v) = entry?; + key_rows += 1; + by_id + .entry(v.value()) + .or_default() + .push(k.value().to_string()); + } + } + let distinct_vector_ids = by_id.len(); + let mut aliased: Vec<(u64, Vec)> = by_id + .into_iter() + .filter(|(_, uuids)| uuids.len() > 1) + .collect(); + // Deterministic order so callers, logs, and tests all agree. + aliased.sort_by_key(|(id, _)| *id); + for (_, uuids) in &mut aliased { + uuids.sort(); + } + Ok(AliasAudit { + key_rows, + distinct_vector_ids, + aliased, + }) + } + + /// Unmap every drawer caught in an id collision so a re-embed can repair it. + /// + /// 🔴 Not wired to any CLI or MCP surface, and never run against a live + /// palace in the PR that added it (#5005) — see that PR's body. + /// + /// Why: when N uuids alias onto one `vector_id`, the single surviving + /// `VECTORS` row holds whichever vector was written LAST, and search + /// resolves that id to whichever uuid `VECTOR_KEYS` yields last (ascending + /// by uuid). Those two "last"s are unrelated, so the reachable drawer's + /// content is not reliably its own either — every member of the group is + /// suspect, not just the unreachable ones. The only sound repair is to drop + /// the whole group's mapping and re-embed all of them. + /// What: for each aliased group, removes every `VECTOR_KEYS` row in it and + /// tombstones the shared id, in one write transaction. Returns the freed + /// uuids, which then read as ordinary "missing" drawers to + /// `PalaceHandle::embed_health` and are repaired by the normal backfill. + /// Idempotent: a second run finds no groups and frees nothing. + /// Test: `unalias_frees_every_uuid_in_a_collision_group`. + pub fn unalias(&self) -> Result> { + if self.read_only { + return Err(HnswStoreError::ReadOnly); + } + let audit = self.audit_aliases()?; + if audit.aliased.is_empty() { + return Ok(Vec::new()); + } + let mut freed: Vec = Vec::new(); + let wtx = self.db.begin_write()?; + { + let mut keys = wtx.open_table(VECTOR_KEYS)?; + let mut tombstones = wtx.open_table(DELETED_VECTORS)?; + for (id, uuids) in &audit.aliased { + for uuid in uuids { + let _ = keys.remove(uuid.as_str())?; + freed.push(uuid.clone()); + } + tombstones.insert(*id, [].as_slice())?; + } + } + wtx.commit()?; + tracing::warn!( + groups = audit.aliased.len(), + freed = freed.len(), + "#5005: unmapped every drawer in an aliased vector_id group; they now \ + read as missing and need a re-embed" + ); + Ok(freed) + } + /// Remove `VECTORS` rows whose `vector_id` no longer has a matching /// `VECTOR_KEYS` entry (i.e. dangling vectors). Also clears tombstoned /// rows from `VECTORS` and `DELETED_VECTORS` in the same pass. @@ -587,180 +848,4 @@ impl HnswStore { } #[cfg(test)] -mod tests { - use super::*; - use redb::Database; - use tempfile::tempdir; - use uuid::Uuid; - - /// Build a deterministic dim-D unit vector. Different seeds produce - /// numerically distinct vectors, which keeps the HNSW graph from - /// short-circuiting any "exact match" paths during search. - fn unit_vec(dim: usize, seed: u32) -> Vec { - let raw: Vec = (0..dim).map(|i| ((i as u32 + seed) as f32) + 1.0).collect(); - let norm: f32 = raw.iter().map(|x| x * x).sum::().sqrt(); - raw.into_iter().map(|x| x / norm).collect() - } - - fn open_store(dim: usize) -> (tempfile::TempDir, HnswStore) { - let dir = tempdir().expect("tempdir"); - let path = dir.path().join("hnsw.redb"); - let db = Arc::new(Database::create(&path).expect("create db")); - let store = HnswStore::open(db, dim).expect("open store"); - (dir, store) - } - - /// Why: The end-to-end contract is "upsert a vector under a UUID, - /// then search for that vector and get the UUID back at rank 0". - /// What: Insert three vectors, query with one of them, assert the - /// matching UUID is the top hit with near-zero distance. - /// Test: This test itself is the verification. - #[test] - fn upsert_and_search_round_trips() { - let (_dir, store) = open_store(8); - let u1 = Uuid::new_v4().to_string(); - let u2 = Uuid::new_v4().to_string(); - let u3 = Uuid::new_v4().to_string(); - let v1 = unit_vec(8, 1); - let v2 = unit_vec(8, 100); - let v3 = unit_vec(8, 200); - - store.upsert(&u1, &v1).unwrap(); - store.upsert(&u2, &v2).unwrap(); - store.upsert(&u3, &v3).unwrap(); - - let hits = store.search(&v2, 1).unwrap(); - assert_eq!(hits.len(), 1, "expected one hit"); - assert_eq!(hits[0].0, u2, "top hit must be the queried vector's uuid"); - assert!( - hits[0].1 < 1e-3, - "distance should be ~0 for exact match, got {}", - hits[0].1 - ); - } - - /// Why: `delete` must hide the vector from subsequent searches even - /// though `hnsw_rs` cannot physically remove it from the graph. - /// What: Insert three vectors, delete one, search using the deleted - /// vector, assert the deleted UUID is NOT in the results. - /// Test: This test itself is the verification. - #[test] - fn delete_filters_results() { - let (_dir, store) = open_store(8); - let u1 = Uuid::new_v4().to_string(); - let u2 = Uuid::new_v4().to_string(); - let u3 = Uuid::new_v4().to_string(); - let v1 = unit_vec(8, 11); - let v2 = unit_vec(8, 22); - let v3 = unit_vec(8, 33); - - store.upsert(&u1, &v1).unwrap(); - store.upsert(&u2, &v2).unwrap(); - store.upsert(&u3, &v3).unwrap(); - - assert!(store.delete(&u2).unwrap(), "delete should report removed"); - // Second delete is a no-op and returns false. - assert!(!store.delete(&u2).unwrap()); - - let hits = store.search(&v2, 3).unwrap(); - assert!( - !hits.iter().any(|(uuid, _)| uuid == &u2), - "deleted uuid must not appear in results: {hits:?}" - ); - assert_eq!(store.len().unwrap(), 2, "len should account for tombstone"); - } - - /// Why: A fresh `HnswStore::open` against the same redb file must - /// rehydrate the in-memory graph from `VECTORS`, so searches return - /// the same UUIDs as before reopen. - /// What: Upsert via one store instance, drop it, reopen at the same - /// path, run the same search, assert the same UUID comes back. - /// Test: This test itself is the verification. - #[test] - fn hydration_restores_index() { - let dir = tempdir().expect("tempdir"); - let path = dir.path().join("hnsw.redb"); - let u1 = Uuid::new_v4().to_string(); - let v1 = unit_vec(8, 42); - - { - let db = Arc::new(Database::create(&path).expect("create")); - let store = HnswStore::open(db, 8).unwrap(); - store.upsert(&u1, &v1).unwrap(); - assert_eq!(store.len().unwrap(), 1); - } - - // Reopen — the in-memory graph must rebuild from redb. - let db = Arc::new(Database::create(&path).expect("reopen")); - let store = HnswStore::open(db, 8).unwrap(); - assert_eq!(store.len().unwrap(), 1, "len survives reopen"); - - let hits = store.search(&v1, 1).unwrap(); - assert_eq!(hits.len(), 1); - assert_eq!(hits[0].0, u1, "uuid must round-trip across reopen"); - } - - /// Why: `compact_orphans` is the maintenance hook that reclaims - /// `VECTORS` rows whose `VECTOR_KEYS` mapping has been removed. - /// What: Manually insert a `VECTORS` row without a corresponding - /// `VECTOR_KEYS` entry (simulating an old orphan), upsert a real one, - /// run compaction, assert the orphan was removed and the real one - /// survived. - /// Test: This test itself is the verification. - #[test] - fn compact_orphans_removes_dangling() { - let (_dir, store) = open_store(8); - let u1 = Uuid::new_v4().to_string(); - let v1 = unit_vec(8, 7); - - // Real upsert — creates one (VECTORS, VECTOR_KEYS) pair. - store.upsert(&u1, &v1).unwrap(); - assert_eq!(store.len().unwrap(), 1); - - // Manually inject an orphan: write to VECTORS without writing to - // VECTOR_KEYS. Use a vector_id that the store has not allocated. - let orphan_id: u64 = 999_999; - let orphan_vec: Vec = unit_vec(8, 99); - let encoded = postcard::to_allocvec(&orphan_vec).unwrap(); - { - let wtx = store.db.begin_write().unwrap(); - { - let mut vectors = wtx.open_table(VECTORS).unwrap(); - vectors.insert(orphan_id, encoded.as_slice()).unwrap(); - } - wtx.commit().unwrap(); - } - - // Now `len()` sees two VECTORS rows minus zero tombstones = 2. - assert_eq!(store.len().unwrap(), 2); - - let removed = store.compact_orphans().unwrap(); - assert_eq!(removed, 1, "should remove exactly the orphan"); - assert_eq!(store.len().unwrap(), 1, "live vector survives"); - - // The real upsert should still resolve via search. - let hits = store.search(&v1, 1).unwrap(); - assert_eq!(hits.len(), 1); - assert_eq!(hits[0].0, u1); - } - - /// Why: Dimension mismatches are programmer errors that must surface - /// loudly (not corrupt the index silently). - /// What: Open a dim=8 store, attempt to upsert a 4-d vector, assert - /// the call returns `DimensionMismatch`. - /// Test: This test itself is the verification. - #[test] - fn dimension_mismatch_is_rejected() { - let (_dir, store) = open_store(8); - let u1 = Uuid::new_v4().to_string(); - let too_small = vec![0.1_f32; 4]; - let err = store.upsert(&u1, &too_small).unwrap_err(); - match err { - HnswStoreError::DimensionMismatch { - expected: 8, - got: 4, - } => {} - other => panic!("wrong error variant: {other:?}"), - } - } -} +mod tests; diff --git a/crates/trusty-common/src/memory_core/store/hnsw_store/tests.rs b/crates/trusty-common/src/memory_core/store/hnsw_store/tests.rs new file mode 100644 index 000000000..a0c6766fd --- /dev/null +++ b/crates/trusty-common/src/memory_core/store/hnsw_store/tests.rs @@ -0,0 +1,527 @@ +//! Unit tests for the redb-backed HNSW store. +//! +//! Why: split out of `hnsw_store.rs` so the 500-SLOC production cap measures +//! production code rather than fixtures (#610's dual cap; the #5005 tests +//! pushed the combined file to 780). Kept a CHILD module, not a sibling, +//! because these tests reach the store's private `db` handle to build states +//! the public API can no longer produce — an aliased `VECTOR_KEYS` mapping, a +//! rewound allocator counter. +//! What: everything that used to live in `#[cfg(test)] mod tests` inline. +//! Test: this file IS the tests. + +use super::*; +use redb::Database; +use tempfile::tempdir; +use uuid::Uuid; + +/// Build a deterministic dim-D unit vector. Different seeds produce +/// numerically distinct vectors, which keeps the HNSW graph from +/// short-circuiting any "exact match" paths during search. +fn unit_vec(dim: usize, seed: u32) -> Vec { + let raw: Vec = (0..dim).map(|i| ((i as u32 + seed) as f32) + 1.0).collect(); + let norm: f32 = raw.iter().map(|x| x * x).sum::().sqrt(); + raw.into_iter().map(|x| x / norm).collect() +} + +fn open_store(dim: usize) -> (tempfile::TempDir, HnswStore) { + let dir = tempdir().expect("tempdir"); + let path = dir.path().join("hnsw.redb"); + let db = Arc::new(Database::create(&path).expect("create db")); + let store = HnswStore::open(db, dim).expect("open store"); + (dir, store) +} + +/// Why: The end-to-end contract is "upsert a vector under a UUID, +/// then search for that vector and get the UUID back at rank 0". +/// What: Insert three vectors, query with one of them, assert the +/// matching UUID is the top hit with near-zero distance. +/// Test: This test itself is the verification. +#[test] +fn upsert_and_search_round_trips() { + let (_dir, store) = open_store(8); + let u1 = Uuid::new_v4().to_string(); + let u2 = Uuid::new_v4().to_string(); + let u3 = Uuid::new_v4().to_string(); + let v1 = unit_vec(8, 1); + let v2 = unit_vec(8, 100); + let v3 = unit_vec(8, 200); + + store.upsert(&u1, &v1).unwrap(); + store.upsert(&u2, &v2).unwrap(); + store.upsert(&u3, &v3).unwrap(); + + let hits = store.search(&v2, 1).unwrap(); + assert_eq!(hits.len(), 1, "expected one hit"); + assert_eq!(hits[0].0, u2, "top hit must be the queried vector's uuid"); + assert!( + hits[0].1 < 1e-3, + "distance should be ~0 for exact match, got {}", + hits[0].1 + ); +} + +/// Why: `delete` must hide the vector from subsequent searches even +/// though `hnsw_rs` cannot physically remove it from the graph. +/// What: Insert three vectors, delete one, search using the deleted +/// vector, assert the deleted UUID is NOT in the results. +/// Test: This test itself is the verification. +#[test] +fn delete_filters_results() { + let (_dir, store) = open_store(8); + let u1 = Uuid::new_v4().to_string(); + let u2 = Uuid::new_v4().to_string(); + let u3 = Uuid::new_v4().to_string(); + let v1 = unit_vec(8, 11); + let v2 = unit_vec(8, 22); + let v3 = unit_vec(8, 33); + + store.upsert(&u1, &v1).unwrap(); + store.upsert(&u2, &v2).unwrap(); + store.upsert(&u3, &v3).unwrap(); + + assert!(store.delete(&u2).unwrap(), "delete should report removed"); + // Second delete is a no-op and returns false. + assert!(!store.delete(&u2).unwrap()); + + let hits = store.search(&v2, 3).unwrap(); + assert!( + !hits.iter().any(|(uuid, _)| uuid == &u2), + "deleted uuid must not appear in results: {hits:?}" + ); + assert_eq!(store.len().unwrap(), 2, "len should account for tombstone"); +} + +/// Why: A fresh `HnswStore::open` against the same redb file must +/// rehydrate the in-memory graph from `VECTORS`, so searches return +/// the same UUIDs as before reopen. +/// What: Upsert via one store instance, drop it, reopen at the same +/// path, run the same search, assert the same UUID comes back. +/// Test: This test itself is the verification. +#[test] +fn hydration_restores_index() { + let dir = tempdir().expect("tempdir"); + let path = dir.path().join("hnsw.redb"); + let u1 = Uuid::new_v4().to_string(); + let v1 = unit_vec(8, 42); + + { + let db = Arc::new(Database::create(&path).expect("create")); + let store = HnswStore::open(db, 8).unwrap(); + store.upsert(&u1, &v1).unwrap(); + assert_eq!(store.len().unwrap(), 1); + } + + // Reopen — the in-memory graph must rebuild from redb. + let db = Arc::new(Database::create(&path).expect("reopen")); + let store = HnswStore::open(db, 8).unwrap(); + assert_eq!(store.len().unwrap(), 1, "len survives reopen"); + + let hits = store.search(&v1, 1).unwrap(); + assert_eq!(hits.len(), 1); + assert_eq!(hits[0].0, u1, "uuid must round-trip across reopen"); +} + +/// Why: `compact_orphans` is the maintenance hook that reclaims +/// `VECTORS` rows whose `VECTOR_KEYS` mapping has been removed. +/// What: Manually insert a `VECTORS` row without a corresponding +/// `VECTOR_KEYS` entry (simulating an old orphan), upsert a real one, +/// run compaction, assert the orphan was removed and the real one +/// survived. +/// Test: This test itself is the verification. +#[test] +fn compact_orphans_removes_dangling() { + let (_dir, store) = open_store(8); + let u1 = Uuid::new_v4().to_string(); + let v1 = unit_vec(8, 7); + + // Real upsert — creates one (VECTORS, VECTOR_KEYS) pair. + store.upsert(&u1, &v1).unwrap(); + assert_eq!(store.len().unwrap(), 1); + + // Manually inject an orphan: write to VECTORS without writing to + // VECTOR_KEYS. Use a vector_id that the store has not allocated. + let orphan_id: u64 = 999_999; + let orphan_vec: Vec = unit_vec(8, 99); + let encoded = postcard::to_allocvec(&orphan_vec).unwrap(); + { + let wtx = store.db.begin_write().unwrap(); + { + let mut vectors = wtx.open_table(VECTORS).unwrap(); + vectors.insert(orphan_id, encoded.as_slice()).unwrap(); + } + wtx.commit().unwrap(); + } + + // Now `len()` sees two VECTORS rows minus zero tombstones = 2. + assert_eq!(store.len().unwrap(), 2); + + let removed = store.compact_orphans().unwrap(); + assert_eq!(removed, 1, "should remove exactly the orphan"); + assert_eq!(store.len().unwrap(), 1, "live vector survives"); + + // The real upsert should still resolve via search. + let hits = store.search(&v1, 1).unwrap(); + assert_eq!(hits.len(), 1); + assert_eq!(hits[0].0, u1); +} + +/// Write `(uuid → id)` and `(id → vector)` straight into redb, bypassing +/// `upsert` and its allocator. Models a palace written by a pre-#5005 +/// binary — including one whose ids already collide. +fn raw_map(store: &HnswStore, uuid: &str, id: u64, seed: u32, dim: usize) { + let encoded = postcard::to_allocvec(&unit_vec(dim, seed)).unwrap(); + let wtx = store.db.begin_write().unwrap(); + { + let mut vectors = wtx.open_table(VECTORS).unwrap(); + let mut keys = wtx.open_table(VECTOR_KEYS).unwrap(); + vectors.insert(id, encoded.as_slice()).unwrap(); + keys.insert(uuid, id).unwrap(); + } + wtx.commit().unwrap(); +} + +/// Read the persisted allocator counter, or `None` when the row is absent. +fn read_seq(store: &HnswStore) -> Option { + let rtx = store.db.begin_read().unwrap(); + let seq = rtx.open_table(VECTOR_ID_SEQ).unwrap(); + seq.get(NEXT_VECTOR_ID).unwrap().map(|g| g.value()) +} + +/// Overwrite the persisted counter. Models the exact state a pre-#5005 +/// binary leaves behind: rows written, counter untouched. +fn set_seq(store: &HnswStore, value: u64) { + let wtx = store.db.begin_write().unwrap(); + { + let mut seq = wtx.open_table(VECTOR_ID_SEQ).unwrap(); + seq.insert(NEXT_VECTOR_ID, value).unwrap(); + } + wtx.commit().unwrap(); +} + +/// Why (#5005): this is the defect. Two live `HnswStore`s over one +/// database file each seeded a private `AtomicU64` from the same +/// high-water mark and then issued the same ids, so `VECTOR_KEYS` aliased +/// several drawers onto one `vector_id` and `VECTORS` overwrote in place — +/// silently, with a present key and no error. That second live store is a +/// supported configuration, not a misuse: `vector::open_or_get_cached_db` +/// hands the same `Arc` to every `UsearchStore` opened for a +/// palace in this process, precisely so the second open does not trip +/// redb's exclusive lock. +/// What: opens two stores over one `Arc` and interleaves upserts +/// through both. Asserts every returned id is distinct, that the redb +/// tables agree (`key_rows == distinct_vector_ids`, no aliased group), and +/// that a THIRD store hydrated from that redb file retrieves every drawer +/// by its own vector — the property the four `trusty-tools` drawers lost. +/// +/// The hydrated store is the right vantage point, not an evasion: each live +/// store's in-memory `hnsw_rs` graph only holds the points it inserted +/// itself, so `a` cannot find `b`'s newest writes until something reopens +/// the file. That staleness is a separate limitation of running two live +/// stores and costs nothing — redb has every row — whereas the id collision +/// this test names destroyed content permanently. +/// Test: this test itself is the verification. It fails on the pre-fix +/// code at the very first pair: both stores seed `next_id` to 1, so `b`'s +/// first upsert takes the id `a` just issued. +#[test] +fn two_live_stores_over_one_file_never_alias_ids() { + let dir = tempdir().expect("tempdir"); + let path = dir.path().join("hnsw.redb"); + let db = Arc::new(Database::create(&path).expect("create db")); + + let a = HnswStore::open(db.clone(), 8).expect("open a"); + let b = HnswStore::open(db.clone(), 8).expect("open b"); + + let mut issued: Vec = Vec::new(); + let mut drawers: Vec<(String, Vec)> = Vec::new(); + for i in 0..10u32 { + let ua = Uuid::new_v4().to_string(); + let va = unit_vec(8, i * 2 + 1); + issued.push(a.upsert(&ua, &va).expect("upsert via a")); + drawers.push((ua, va)); + + let ub = Uuid::new_v4().to_string(); + let vb = unit_vec(8, i * 2 + 2); + issued.push(b.upsert(&ub, &vb).expect("upsert via b")); + drawers.push((ub, vb)); + } + + let distinct: std::collections::HashSet = issued.iter().copied().collect(); + assert_eq!( + distinct.len(), + issued.len(), + "two live stores must never issue the same vector_id twice: {issued:?}" + ); + + let audit = a.audit_aliases().expect("audit"); + assert_eq!(audit.key_rows, 20, "one key row per drawer"); + assert_eq!( + audit.distinct_vector_ids, audit.key_rows, + "every key must own its own vector_id; aliased groups: {:?}", + audit.aliased + ); + assert!(audit.is_clean(), "no drawer may share an id: {audit:?}"); + + // The point of the ids being distinct: every drawer stays findable + // once the graph is rebuilt from the (shared, authoritative) redb file. + drop(a); + drop(b); + let hydrated = HnswStore::open(db, 8).expect("hydrate"); + for (uuid, vec) in &drawers { + let hits = hydrated.search(vec, 1).expect("search"); + assert_eq!( + hits.first().map(|(u, _)| u.as_str()), + Some(uuid.as_str()), + "drawer {uuid} must retrieve itself" + ); + } +} + +/// Why (#5005): the persisted counter is the mechanism, but it is only as +/// good as its last writer. A palace touched by a pre-#5005 binary between +/// two opens of a fixed one has rows the counter does not know about, and +/// an allocator that trusts the counter blindly would hand out an id that +/// is already in use and overwrite a live vector. `upsert` must refuse. +/// What: upserts a drawer, rewinds the counter to that drawer's id (the +/// stale-counter state), then upserts a second drawer. The second must get +/// a DIFFERENT id, and the first drawer must still resolve to itself. +/// Test: this test itself is the verification. Deleting the +/// `vectors.get(candidate)?.is_none()` probe in `allocate_vector_id` makes +/// the second upsert return the first drawer's id and this test fail on the +/// `assert_ne!`. +#[test] +fn upsert_refuses_to_reuse_an_id_already_present_in_vectors() { + let (_dir, store) = open_store(8); + let u1 = Uuid::new_v4().to_string(); + let v1 = unit_vec(8, 5); + let first = store.upsert(&u1, &v1).expect("first upsert"); + + // Rewind the counter so the next allocation targets an occupied id. + set_seq(&store, first); + + let u2 = Uuid::new_v4().to_string(); + let v2 = unit_vec(8, 500); + let second = store.upsert(&u2, &v2).expect("second upsert must not fail"); + + assert_ne!( + second, first, + "a stale counter must not hand out an id that already has a VECTORS row" + ); + let audit = store.audit_aliases().expect("audit"); + assert!( + audit.is_clean(), + "stale counter aliased two drawers: {audit:?}" + ); + let hits = store.search(&v1, 1).expect("search"); + assert_eq!( + hits.first().map(|(u, _)| u.as_str()), + Some(u1.as_str()), + "the first drawer's vector must survive the second upsert" + ); +} + +/// Why (#5005 migration): every palace on disk predates `VECTOR_ID_SEQ`. +/// Opening one must seed the counter above everything already written, or +/// the first upsert after the upgrade collides with an existing row. +/// What: writes `VECTORS`/`VECTOR_KEYS` rows for ids 1..=5 directly, with +/// no counter row, then opens the store. Asserts the counter is seeded to +/// 6, that a fresh upsert takes 6, and that re-opening does not disturb it. +/// Test: this test itself is the verification. Removing the seeding block +/// from `open_with_mode` leaves `read_seq` at `None` and fails the first +/// assertion. +#[test] +fn old_palace_without_a_seq_row_is_seeded_on_open() { + let dir = tempdir().expect("tempdir"); + let path = dir.path().join("hnsw.redb"); + let db = Arc::new(Database::create(&path).expect("create db")); + + { + // A pre-#5005 palace: rows, no counter. + let seeder = HnswStore::open(db.clone(), 8).expect("open seeder"); + for id in 1..=5u64 { + raw_map(&seeder, &Uuid::new_v4().to_string(), id, id as u32 * 7, 8); + } + // Clear the counter this open wrote so the file is genuinely + // pre-#5005 for the open under test. + let wtx = db.begin_write().unwrap(); + { + let mut seq = wtx.open_table(VECTOR_ID_SEQ).unwrap(); + seq.remove(NEXT_VECTOR_ID).unwrap(); + } + wtx.commit().unwrap(); + } + + let store = HnswStore::open(db.clone(), 8).expect("open migrated"); + assert_eq!( + read_seq(&store), + Some(6), + "opening a pre-#5005 palace must seed the counter above every existing id" + ); + + let fresh = Uuid::new_v4().to_string(); + assert_eq!( + store.upsert(&fresh, &unit_vec(8, 900)).expect("upsert"), + 6, + "the first post-migration upsert must take the next free id" + ); + + drop(store); + let reopened = HnswStore::open(db, 8).expect("reopen"); + assert_eq!( + read_seq(&reopened), + Some(7), + "re-opening must not rewind or re-seed an already-correct counter" + ); +} + +/// Why (#5005 rolling upgrade): a fixed binary and an old one can take +/// turns on the same palace — the old one writes rows and leaves the +/// counter where it found it. Seeding only when the row is ABSENT would +/// let the fixed binary re-issue every id the old one wrote. The seed has +/// to be a max, not a set-if-missing. +/// What: seeds a counter, writes rows past it the way an old binary would, +/// reopens, and asserts the counter was raised to clear them. +/// Test: this test itself is the verification. Changing the seed to +/// "insert only when the row is absent" leaves the counter at 3 and fails +/// both assertions. +#[test] +fn reopen_raises_a_counter_an_old_binary_left_behind() { + let dir = tempdir().expect("tempdir"); + let path = dir.path().join("hnsw.redb"); + let db = Arc::new(Database::create(&path).expect("create db")); + + { + let store = HnswStore::open(db.clone(), 8).expect("open"); + set_seq(&store, 3); + // The "old binary" leg: rows at 3..=8, counter untouched. + for id in 3..=8u64 { + raw_map(&store, &Uuid::new_v4().to_string(), id, id as u32 * 3, 8); + } + assert_eq!(read_seq(&store), Some(3), "precondition: counter is stale"); + } + + let store = HnswStore::open(db, 8).expect("reopen"); + assert_eq!( + read_seq(&store), + Some(9), + "reopen must raise the counter past rows an old binary wrote" + ); + let fresh = Uuid::new_v4().to_string(); + assert_eq!( + store.upsert(&fresh, &unit_vec(8, 77)).expect("upsert"), + 9, + "no id an old binary already used may be re-issued" + ); +} + +/// Why (#5005 / #5000): `palace_reembed` reported 0 missing for a palace +/// with four unretrievable drawers, because it tests key PRESENCE. The +/// detector has to compare key rows against distinct ids instead. +/// What: writes three uuids onto one `vector_id` directly, then audits. +/// Test: this test itself is the verification. Making `audit_aliases` +/// return an empty `aliased` vec fails the group assertions; dropping the +/// distinct-id count fails the arithmetic one. +#[test] +fn audit_detects_two_uuids_mapped_to_one_id() { + let (_dir, store) = open_store(8); + // One honest drawer through the real path. + let solo = Uuid::new_v4().to_string(); + store.upsert(&solo, &unit_vec(8, 3)).expect("upsert"); + + let mut aliased_uuids = vec![ + "11111111-1111-4111-8111-111111111111".to_string(), + "22222222-2222-4222-8222-222222222222".to_string(), + "33333333-3333-4333-8333-333333333333".to_string(), + ]; + aliased_uuids.sort(); + for u in &aliased_uuids { + raw_map(&store, u, 988, 12, 8); + } + + let audit = store.audit_aliases().expect("audit"); + assert_eq!(audit.key_rows, 4, "one row per uuid, aliased or not"); + assert_eq!( + audit.distinct_vector_ids, 2, + "three uuids share id 988, so only two ids are distinct" + ); + assert_eq!( + audit.key_rows - audit.distinct_vector_ids, + 2, + "the arithmetic #5000 asks for: rows minus distinct ids" + ); + assert!(!audit.is_clean()); + assert_eq!(audit.aliased.len(), 1, "one offending group"); + assert_eq!(audit.aliased[0].0, 988); + assert_eq!(audit.aliased[0].1, aliased_uuids); + assert_eq!( + audit.aliased_key_count(), + 3, + "all three drawers are affected" + ); +} + +/// Why (#5005 repair): the reachable member of a collision group is no +/// safer than the unreachable ones — `VECTORS` holds whichever vector was +/// written last, and search resolves the id to whichever uuid sorts last, +/// and those are unrelated. So the repair must free the WHOLE group, not +/// just the losers. +/// What: builds a three-uuid collision, runs `unalias`, and asserts all +/// three keys are gone, the audit is clean, the untouched drawer is +/// untouched, and a second run is a no-op. +/// Test: this test itself is the verification. Keeping the +/// lexicographically-last uuid mapped (the tempting "preserve the +/// reachable one" shortcut) leaves 2 freed and fails the count. +#[test] +fn unalias_frees_every_uuid_in_a_collision_group() { + let (_dir, store) = open_store(8); + let solo = Uuid::new_v4().to_string(); + let solo_vec = unit_vec(8, 3); + store.upsert(&solo, &solo_vec).expect("upsert"); + + for u in [ + "11111111-1111-4111-8111-111111111111", + "22222222-2222-4222-8222-222222222222", + "33333333-3333-4333-8333-333333333333", + ] { + raw_map(&store, u, 988, 12, 8); + } + + let freed = store.unalias().expect("unalias"); + assert_eq!(freed.len(), 3, "every member of the group must be freed"); + + let audit = store.audit_aliases().expect("audit after repair"); + assert!( + audit.is_clean(), + "repair must leave no aliased group: {audit:?}" + ); + assert_eq!(audit.key_rows, 1, "only the untouched drawer keeps its key"); + assert_eq!(audit.distinct_vector_ids, 1); + + // The healthy drawer is unaffected and still retrievable. + let hits = store.search(&solo_vec, 1).expect("search"); + assert_eq!(hits.first().map(|(u, _)| u.as_str()), Some(solo.as_str())); + + assert!( + store.unalias().expect("second unalias").is_empty(), + "repair must be idempotent" + ); +} + +/// Why: Dimension mismatches are programmer errors that must surface +/// loudly (not corrupt the index silently). +/// What: Open a dim=8 store, attempt to upsert a 4-d vector, assert +/// the call returns `DimensionMismatch`. +/// Test: This test itself is the verification. +#[test] +fn dimension_mismatch_is_rejected() { + let (_dir, store) = open_store(8); + let u1 = Uuid::new_v4().to_string(); + let too_small = vec![0.1_f32; 4]; + let err = store.upsert(&u1, &too_small).unwrap_err(); + match err { + HnswStoreError::DimensionMismatch { + expected: 8, + got: 4, + } => {} + other => panic!("wrong error variant: {other:?}"), + } +} diff --git a/crates/trusty-common/src/memory_core/store/kg_store.rs b/crates/trusty-common/src/memory_core/store/kg_store.rs index 6b71a939b..f4a6f3bed 100644 --- a/crates/trusty-common/src/memory_core/store/kg_store.rs +++ b/crates/trusty-common/src/memory_core/store/kg_store.rs @@ -181,6 +181,28 @@ pub const VECTOR_KEYS: TableDefinition<&str, u64> = TableDefinition::new("vector /// (`delete_filters_results`). pub const DELETED_VECTORS: TableDefinition = TableDefinition::new("deleted_vectors"); +/// Persisted vector-id allocator (issue #5005). +/// +/// Why: `HnswStore` used to allocate vector ids from a process-local +/// `AtomicU64` seeded at open from `max(VECTORS, VECTOR_KEYS) + 1`. Two +/// live stores over the same database — which the in-process vector-db +/// cache deliberately supports, see +/// `crate::memory_core::store::vector::open_or_get_cached_db` — each +/// seeded their own counter from the same high-water mark and then handed +/// out the same ids, so `VECTOR_KEYS` aliased several drawers onto one +/// `vector_id` and `VECTORS` overwrote in place. Keeping the reservation +/// in redb and bumping it inside the same write transaction as the insert +/// makes allocation serialisable with every other writer on the file. +/// What: Single-row table. Key = [`NEXT_VECTOR_ID`], value = the next +/// unissued `u64` vector_id. +/// Test: `crate::memory_core::store::hnsw_store::tests:: +/// two_live_stores_over_one_file_never_alias_ids` and +/// `old_palace_without_a_seq_row_is_seeded_on_open`. +pub const VECTOR_ID_SEQ: TableDefinition<&str, u64> = TableDefinition::new("vector_id_seq"); + +/// The only key stored in [`VECTOR_ID_SEQ`]. +pub const NEXT_VECTOR_ID: &str = "next_vector_id"; + /// Chat-session store (for the trusty-memory web UI's chat panel). /// /// Why: Each chat session is keyed by a UUID string and carries a small diff --git a/crates/trusty-common/src/memory_core/store/vector.rs b/crates/trusty-common/src/memory_core/store/vector.rs index 08d685b2e..0818ca75c 100644 --- a/crates/trusty-common/src/memory_core/store/vector.rs +++ b/crates/trusty-common/src/memory_core/store/vector.rs @@ -404,6 +404,56 @@ impl UsearchStore { } } + /// Drawer ids whose vector has been overwritten by another drawer's. + /// + /// Why (#5005): key presence — what `embed_health` and `palace_reembed` + /// test — cannot see an id collision, so a palace with four unretrievable + /// drawers reported a clean bill of health. This is the comparison that + /// does see it. + /// What: delegates to `HnswStore::audit_aliases` and parses the uuids back + /// into `Uuid`s, dropping (and logging) any row that will not parse so one + /// bad key cannot hide the rest. Returns the two counts alongside the ids. + /// Test: `alias_audit_surfaces_a_collision` in `embed_repair_tests`. + pub fn alias_audit(&self) -> Result<(usize, usize, Vec)> { + let audit = self + .inner + .audit_aliases() + .context("alias_audit: scan vector keys")?; + let ids = audit + .aliased + .iter() + .flat_map(|(_, uuids)| uuids.iter()) + .filter_map(|s| match Uuid::parse_str(s) { + Ok(u) => Some(u), + Err(e) => { + tracing::warn!(key = %s, "alias_audit: skipping unparseable uuid: {e}"); + None + } + }) + .collect(); + Ok((audit.key_rows, audit.distinct_vector_ids, ids)) + } + + /// Unmap every drawer caught in an id collision so a re-embed repairs it. + /// + /// 🔴 Not wired to any CLI or MCP surface, and never run against a live + /// palace in the PR that added it (#5005). + /// + /// Why: see [`HnswStore::unalias`] — the reachable member of a collision + /// group is no more trustworthy than the unreachable ones, so the repair + /// has to free the whole group. + /// What: delegates to `HnswStore::unalias` and returns the freed drawer + /// ids, which then read as ordinary "missing" to `embed_health`. + /// Test: `unalias_marks_the_whole_group_for_reembed` in + /// `embed_repair_tests`. + pub fn unalias(&self) -> Result> { + let freed = self.inner.unalias().context("unalias: free aliased keys")?; + Ok(freed + .iter() + .filter_map(|s| Uuid::parse_str(s).ok()) + .collect()) + } + /// Remove vector entries whose drawer IDs are not in `valid_ids`. /// /// Why: Issue #49 — over a palace's lifetime, vectors get orphaned by @@ -580,334 +630,4 @@ fn migrate_legacy_usearch_if_present( } #[cfg(test)] -mod tests { - use super::*; - use tempfile::tempdir; - - fn unit_vec(dim: usize, seed: u32) -> Vec { - let raw: Vec = (0..dim).map(|i| ((i as u32 + seed) as f32) + 1.0).collect(); - let norm: f32 = raw.iter().map(|x| x * x).sum::().sqrt(); - raw.into_iter().map(|x| x / norm).collect() - } - - #[tokio::test] - async fn upsert_then_search_returns_same_vector_at_rank_0() { - let dir = tempdir().unwrap(); - let store = UsearchStore::new(dir.path().join("test.usearch"), 384).unwrap(); - let id = Uuid::new_v4(); - let v = unit_vec(384, 0); - - store.upsert(id, v.clone()).await.unwrap(); - let hits = store.search(&v, 1).await.unwrap(); - assert_eq!(hits.len(), 1); - assert_eq!(hits[0].drawer_id, id); - assert!(hits[0].score >= 0.99, "score was {}", hits[0].score); - } - - #[tokio::test] - async fn remove_clears_vector() { - let dir = tempdir().unwrap(); - let store = UsearchStore::new(dir.path().join("test.usearch"), 384).unwrap(); - let id = Uuid::new_v4(); - let v = unit_vec(384, 7); - store.upsert(id, v.clone()).await.unwrap(); - store.remove(id).await.unwrap(); - - let hits = store.search(&v, 5).await.unwrap(); - assert!( - !hits.iter().any(|h| h.drawer_id == id), - "removed id still present in results" - ); - } - - #[tokio::test] - async fn persist_and_reload() { - let dir = tempdir().unwrap(); - let path = dir.path().join("test.usearch"); - let id = Uuid::new_v4(); - let v = unit_vec(384, 13); - { - let store = UsearchStore::new(path.clone(), 384).unwrap(); - store.upsert(id, v.clone()).await.unwrap(); - } - let store2 = UsearchStore::new(path, 384).unwrap(); - let hits = store2.search(&v, 1).await.unwrap(); - assert_eq!(hits.len(), 1); - assert_eq!(hits[0].drawer_id, id); - assert!(hits[0].score >= 0.99, "score was {}", hits[0].score); - } - - /// Why: Issue #51 — `compact_orphans` must remove only the vectors - /// whose drawer UUIDs are absent from the supplied valid set, and must - /// persist the change so a subsequent reload doesn't resurrect the - /// orphans. - /// What: Insert three vectors, mark one as valid, run compaction, - /// then assert (a) total_checked counts all three, (b) two were - /// removed, and (c) reopening the store from disk shows only the - /// kept vector. - /// Test: This test itself is the verification. - #[tokio::test] - async fn compact_orphans_removes_only_missing_ids() { - let dir = tempdir().unwrap(); - let path = dir.path().join("test.usearch"); - let store = UsearchStore::new(path.clone(), 384).unwrap(); - - let keep = Uuid::new_v4(); - let drop_a = Uuid::new_v4(); - let drop_b = Uuid::new_v4(); - store.upsert(keep, unit_vec(384, 1)).await.unwrap(); - store.upsert(drop_a, unit_vec(384, 2)).await.unwrap(); - store.upsert(drop_b, unit_vec(384, 3)).await.unwrap(); - - let mut valid = HashSet::new(); - valid.insert(keep); - let res = store.compact_orphans(&valid).unwrap(); - assert_eq!(res.total_checked, 3); - assert_eq!(res.orphans_removed, 2); - assert_eq!(res.index_size_before, 3); - assert_eq!(res.index_size_after, 1); - - // Reopen from disk — the compacted state must survive. - drop(store); - let reopened = UsearchStore::new(path, 384).unwrap(); - let ids = reopened.all_ids(); - assert_eq!(ids.len(), 1); - assert_eq!(ids[0], keep); - } - - /// Why: Search results must round-trip the full UUID (not a truncated - /// or zero-padded form), so dedup across L1/L2 doesn't silently fail. - /// What: Upsert a vector under a fresh `Uuid::new_v4`, search for it, - /// and assert the returned `drawer_id` matches the input bit-for-bit. - /// Test: This test itself is the verification. - #[tokio::test] - async fn upsert_then_l1_l2_no_duplicate() { - let dir = tempdir().unwrap(); - let store = UsearchStore::new(dir.path().join("test.usearch"), 384).unwrap(); - let id = Uuid::new_v4(); - let v = unit_vec(384, 42); - - store.upsert(id, v.clone()).await.unwrap(); - let hits = store.search(&v, 1).await.unwrap(); - assert_eq!(hits.len(), 1); - assert_eq!( - hits[0].drawer_id, id, - "search must return the full original UUID" - ); - } - - /// Why: `reset` must wipe the index so the next search returns - /// nothing — the dream cycle relies on this to safely rebuild from - /// drawers. - /// What: Insert two vectors, reset, then search; expect an empty - /// result. - /// Test: This test itself is the verification. - #[tokio::test] - async fn reset_clears_index() { - let dir = tempdir().unwrap(); - let store = UsearchStore::new(dir.path().join("test.usearch"), 384).unwrap(); - store - .upsert(Uuid::new_v4(), unit_vec(384, 1)) - .await - .unwrap(); - store - .upsert(Uuid::new_v4(), unit_vec(384, 2)) - .await - .unwrap(); - assert!(store.index_size() >= 2); - - store.reset().unwrap(); - assert_eq!(store.index_size(), 0); - - let hits = store.search(&unit_vec(384, 1), 5).await.unwrap(); - assert!(hits.is_empty(), "search after reset should be empty"); - } - - // -- Issue #59 / #1152: cross-process lock + snapshot fallback ------------- - // `UsearchStore::new` uses `OpenIntent::ReadOnlyClient` so that when another - // process holds the redb exclusive lock, we fall back to a read-only snapshot - // (issue #59 behaviour). Writes against that snapshot are rejected via - // `READ_ONLY_ERROR_MSG`. The issue #1152 guard is enforced at the daemon - // level (`single_instance_check` in main.rs), not at the storage layer. - - /// Why (issue #59 / #1152): `UsearchStore::new` uses - /// `OpenIntent::ReadOnlyClient` — when a cross-process lock conflict occurs - /// (another daemon holds the file), the caller gets a read-only snapshot - /// handle rather than an error. Writes are rejected via `READ_ONLY_ERROR_MSG` - /// so silent divergence is impossible. - /// What: Seeds the vector file, drops the store so the cache expires, holds - /// the redb file lock with a raw handle, then asserts the second - /// `UsearchStore::new` SUCCEEDS in snapshot (read-only) mode. - /// Test: this test. - #[tokio::test] - async fn vector_open_on_locked_file_returns_snapshot_handle() { - let dir = tempdir().unwrap(); - let logical = dir.path().join("test.usearch"); - - // Populate and drop so the cache entry expires. - { - let primary = UsearchStore::new(logical.clone(), 384).unwrap(); - primary - .upsert(Uuid::new_v4(), unit_vec(384, 1)) - .await - .unwrap(); - } - - // Hold the redb file lock with a raw `Database::create`. - let redb_path = redb_path_for(&logical); - let _live = redb::Database::create(&redb_path).expect("lock vector redb"); - - // ReadOnlyClient open must succeed via snapshot fallback. - let result = UsearchStore::new(logical.clone(), 384); - assert!( - result.is_ok(), - "ReadOnlyClient open on locked vector redb must succeed via snapshot fallback" - ); - let snap = result.expect("should be Ok"); - assert!( - snap.is_read_only(), - "snapshot store must report is_read_only()" - ); - } - - /// Why (issue #1487): the HTTP daemon opens the vector store with - /// `OpenIntent::Writer`. When a second live instance already holds the - /// redb write lock, the Writer open MUST fail loud (after the bounded - /// handoff window) and MUST NOT return a read-only snapshot handle — - /// otherwise every `upsert`/`remove` would be silently rejected for the - /// daemon's lifetime (the original bug). - /// What: Seeds the vector file, drops the store so the cache expires, - /// holds the redb file lock with a raw handle, then calls - /// `UsearchStore::new_with_intent(.., Writer)`. The call must return `Err` - /// naming the lock conflict — never an `Ok` snapshot handle. - /// Test: this test. - #[tokio::test] - async fn writer_intent_open_fails_loud_on_locked_vector_file() { - let dir = tempdir().unwrap(); - let logical = dir.path().join("test.usearch"); - - // Populate and drop so the cache entry expires. - { - let primary = UsearchStore::new(logical.clone(), 384).unwrap(); - primary - .upsert(Uuid::new_v4(), unit_vec(384, 1)) - .await - .unwrap(); - } - - // Hold the redb file lock with a raw `Database::create`. - let redb_path = redb_path_for(&logical); - let _live = redb::Database::create(&redb_path).expect("lock vector redb"); - - // Writer open must fail loud, never snapshot. - let result = UsearchStore::new_with_intent(logical.clone(), 384, OpenIntent::Writer); - // Match rather than `unwrap_err()` so we don't require UsearchStore: Debug. - let err = match result { - Ok(_) => panic!( - "Writer open on a locked vector redb must fail loud, not return a snapshot handle" - ), - Err(e) => e, - }; - // Use the alternate `{:#}` form so the full anyhow context chain - // (the `open_or_get_cached_db` wrapper + the root lock message) is - // rendered, not just the outermost context line. - let msg = format!("{err:#}"); - assert!( - msg.contains("still locked") || msg.contains("write access"), - "Writer error must name the lock conflict; got: {msg}" - ); - } - - /// Why (issue #59): `upsert` and `remove` on a snapshot handle must - /// return an error that includes the read-only sentinel text so callers - /// see actionable guidance. This tests the storage-layer write guard - /// independently of the daemon-level `single_instance_check`. - /// What: Seeds a vector file, drops the store so the cache expires, - /// holds the lock, opens a snapshot handle, then asserts both write - /// methods Err with the expected message. - /// Test: this test — `vector_writes_rejected_on_snapshot`. - #[tokio::test] - async fn vector_writes_rejected_on_snapshot() { - let dir = tempdir().unwrap(); - let logical = dir.path().join("test.usearch"); - - // Populate and drop so the cache entry expires. - { - let primary = UsearchStore::new(logical.clone(), 384).unwrap(); - primary - .upsert(Uuid::new_v4(), unit_vec(384, 1)) - .await - .unwrap(); - } - - // Hold the lock so the next open takes the snapshot path. - let redb_path = redb_path_for(&logical); - let _live = redb::Database::create(&redb_path).expect("lock vector redb"); - - let snap = UsearchStore::new(logical.clone(), 384).expect("snapshot open must succeed"); - assert!(snap.is_read_only()); - - // upsert must fail with read-only guidance. - let err = snap - .upsert(Uuid::new_v4(), unit_vec(384, 99)) - .await - .unwrap_err(); - let msg = err.to_string(); - assert!( - msg.contains("read-only"), - "upsert on snapshot must mention read-only, got: {msg}" - ); - - // remove must fail with read-only guidance. - let err = snap.remove(Uuid::new_v4()).await.unwrap_err(); - let msg = err.to_string(); - assert!( - msg.contains("read-only"), - "remove on snapshot must mention read-only, got: {msg}" - ); - } - - /// Why (issue #59): reads must succeed on a snapshot handle — the - /// snapshot is a point-in-time copy of the live file and must be - /// searchable. - /// What: Seeds one vector, drops, acquires lock, opens snapshot, - /// searches, and asserts the seeded id is returned at rank 0. - /// Test: this test — `vector_remove_rejected_on_snapshot` (search - /// path — the symmetric read-succeeds counterpart). - #[tokio::test] - async fn vector_remove_rejected_on_snapshot() { - let dir = tempdir().unwrap(); - let logical = dir.path().join("test.usearch"); - let id = Uuid::new_v4(); - let v = unit_vec(384, 5); - - // Seed then drop so cache expires. - { - let primary = UsearchStore::new(logical.clone(), 384).unwrap(); - primary.upsert(id, v.clone()).await.unwrap(); - } - - // Hold the lock. - let redb_path = redb_path_for(&logical); - let _live = redb::Database::create(&redb_path).expect("lock vector redb"); - - let snap = UsearchStore::new(logical.clone(), 384).expect("snapshot open must succeed"); - assert!(snap.is_read_only()); - - // Search (read) must succeed and return the seeded vector. - let hits = snap.search(&v, 1).await.unwrap(); - assert_eq!( - hits.len(), - 1, - "search on snapshot must return seeded vector" - ); - assert_eq!(hits[0].drawer_id, id); - - // remove must be rejected. - let err = snap.remove(id).await.unwrap_err(); - assert!( - err.to_string().contains("read-only"), - "remove on snapshot must be rejected" - ); - } -} +mod tests; diff --git a/crates/trusty-common/src/memory_core/store/vector/tests.rs b/crates/trusty-common/src/memory_core/store/vector/tests.rs new file mode 100644 index 000000000..0126c8c54 --- /dev/null +++ b/crates/trusty-common/src/memory_core/store/vector/tests.rs @@ -0,0 +1,339 @@ +//! Unit tests for the `UsearchStore` / `VectorStore` surface. +//! +//! Why: split out of `vector.rs` for the same reason as +//! `hnsw_store/tests.rs` — the 500-SLOC production cap (#610) counts inline +//! test modules, and `vector.rs` reached 526 with the #5005 audit delegates +//! added. A child module keeps access to the module-private helpers the tests +//! already used. +//! What: everything that used to live in `#[cfg(test)] mod tests` inline. +//! Test: this file IS the tests. + +use super::*; +use tempfile::tempdir; + +fn unit_vec(dim: usize, seed: u32) -> Vec { + let raw: Vec = (0..dim).map(|i| ((i as u32 + seed) as f32) + 1.0).collect(); + let norm: f32 = raw.iter().map(|x| x * x).sum::().sqrt(); + raw.into_iter().map(|x| x / norm).collect() +} + +#[tokio::test] +async fn upsert_then_search_returns_same_vector_at_rank_0() { + let dir = tempdir().unwrap(); + let store = UsearchStore::new(dir.path().join("test.usearch"), 384).unwrap(); + let id = Uuid::new_v4(); + let v = unit_vec(384, 0); + + store.upsert(id, v.clone()).await.unwrap(); + let hits = store.search(&v, 1).await.unwrap(); + assert_eq!(hits.len(), 1); + assert_eq!(hits[0].drawer_id, id); + assert!(hits[0].score >= 0.99, "score was {}", hits[0].score); +} + +#[tokio::test] +async fn remove_clears_vector() { + let dir = tempdir().unwrap(); + let store = UsearchStore::new(dir.path().join("test.usearch"), 384).unwrap(); + let id = Uuid::new_v4(); + let v = unit_vec(384, 7); + store.upsert(id, v.clone()).await.unwrap(); + store.remove(id).await.unwrap(); + + let hits = store.search(&v, 5).await.unwrap(); + assert!( + !hits.iter().any(|h| h.drawer_id == id), + "removed id still present in results" + ); +} + +#[tokio::test] +async fn persist_and_reload() { + let dir = tempdir().unwrap(); + let path = dir.path().join("test.usearch"); + let id = Uuid::new_v4(); + let v = unit_vec(384, 13); + { + let store = UsearchStore::new(path.clone(), 384).unwrap(); + store.upsert(id, v.clone()).await.unwrap(); + } + let store2 = UsearchStore::new(path, 384).unwrap(); + let hits = store2.search(&v, 1).await.unwrap(); + assert_eq!(hits.len(), 1); + assert_eq!(hits[0].drawer_id, id); + assert!(hits[0].score >= 0.99, "score was {}", hits[0].score); +} + +/// Why: Issue #51 — `compact_orphans` must remove only the vectors +/// whose drawer UUIDs are absent from the supplied valid set, and must +/// persist the change so a subsequent reload doesn't resurrect the +/// orphans. +/// What: Insert three vectors, mark one as valid, run compaction, +/// then assert (a) total_checked counts all three, (b) two were +/// removed, and (c) reopening the store from disk shows only the +/// kept vector. +/// Test: This test itself is the verification. +#[tokio::test] +async fn compact_orphans_removes_only_missing_ids() { + let dir = tempdir().unwrap(); + let path = dir.path().join("test.usearch"); + let store = UsearchStore::new(path.clone(), 384).unwrap(); + + let keep = Uuid::new_v4(); + let drop_a = Uuid::new_v4(); + let drop_b = Uuid::new_v4(); + store.upsert(keep, unit_vec(384, 1)).await.unwrap(); + store.upsert(drop_a, unit_vec(384, 2)).await.unwrap(); + store.upsert(drop_b, unit_vec(384, 3)).await.unwrap(); + + let mut valid = HashSet::new(); + valid.insert(keep); + let res = store.compact_orphans(&valid).unwrap(); + assert_eq!(res.total_checked, 3); + assert_eq!(res.orphans_removed, 2); + assert_eq!(res.index_size_before, 3); + assert_eq!(res.index_size_after, 1); + + // Reopen from disk — the compacted state must survive. + drop(store); + let reopened = UsearchStore::new(path, 384).unwrap(); + let ids = reopened.all_ids(); + assert_eq!(ids.len(), 1); + assert_eq!(ids[0], keep); +} + +/// Why: Search results must round-trip the full UUID (not a truncated +/// or zero-padded form), so dedup across L1/L2 doesn't silently fail. +/// What: Upsert a vector under a fresh `Uuid::new_v4`, search for it, +/// and assert the returned `drawer_id` matches the input bit-for-bit. +/// Test: This test itself is the verification. +#[tokio::test] +async fn upsert_then_l1_l2_no_duplicate() { + let dir = tempdir().unwrap(); + let store = UsearchStore::new(dir.path().join("test.usearch"), 384).unwrap(); + let id = Uuid::new_v4(); + let v = unit_vec(384, 42); + + store.upsert(id, v.clone()).await.unwrap(); + let hits = store.search(&v, 1).await.unwrap(); + assert_eq!(hits.len(), 1); + assert_eq!( + hits[0].drawer_id, id, + "search must return the full original UUID" + ); +} + +/// Why: `reset` must wipe the index so the next search returns +/// nothing — the dream cycle relies on this to safely rebuild from +/// drawers. +/// What: Insert two vectors, reset, then search; expect an empty +/// result. +/// Test: This test itself is the verification. +#[tokio::test] +async fn reset_clears_index() { + let dir = tempdir().unwrap(); + let store = UsearchStore::new(dir.path().join("test.usearch"), 384).unwrap(); + store + .upsert(Uuid::new_v4(), unit_vec(384, 1)) + .await + .unwrap(); + store + .upsert(Uuid::new_v4(), unit_vec(384, 2)) + .await + .unwrap(); + assert!(store.index_size() >= 2); + + store.reset().unwrap(); + assert_eq!(store.index_size(), 0); + + let hits = store.search(&unit_vec(384, 1), 5).await.unwrap(); + assert!(hits.is_empty(), "search after reset should be empty"); +} + +// -- Issue #59 / #1152: cross-process lock + snapshot fallback ------------- +// `UsearchStore::new` uses `OpenIntent::ReadOnlyClient` so that when another +// process holds the redb exclusive lock, we fall back to a read-only snapshot +// (issue #59 behaviour). Writes against that snapshot are rejected via +// `READ_ONLY_ERROR_MSG`. The issue #1152 guard is enforced at the daemon +// level (`single_instance_check` in main.rs), not at the storage layer. + +/// Why (issue #59 / #1152): `UsearchStore::new` uses +/// `OpenIntent::ReadOnlyClient` — when a cross-process lock conflict occurs +/// (another daemon holds the file), the caller gets a read-only snapshot +/// handle rather than an error. Writes are rejected via `READ_ONLY_ERROR_MSG` +/// so silent divergence is impossible. +/// What: Seeds the vector file, drops the store so the cache expires, holds +/// the redb file lock with a raw handle, then asserts the second +/// `UsearchStore::new` SUCCEEDS in snapshot (read-only) mode. +/// Test: this test. +#[tokio::test] +async fn vector_open_on_locked_file_returns_snapshot_handle() { + let dir = tempdir().unwrap(); + let logical = dir.path().join("test.usearch"); + + // Populate and drop so the cache entry expires. + { + let primary = UsearchStore::new(logical.clone(), 384).unwrap(); + primary + .upsert(Uuid::new_v4(), unit_vec(384, 1)) + .await + .unwrap(); + } + + // Hold the redb file lock with a raw `Database::create`. + let redb_path = redb_path_for(&logical); + let _live = redb::Database::create(&redb_path).expect("lock vector redb"); + + // ReadOnlyClient open must succeed via snapshot fallback. + let result = UsearchStore::new(logical.clone(), 384); + assert!( + result.is_ok(), + "ReadOnlyClient open on locked vector redb must succeed via snapshot fallback" + ); + let snap = result.expect("should be Ok"); + assert!( + snap.is_read_only(), + "snapshot store must report is_read_only()" + ); +} + +/// Why (issue #1487): the HTTP daemon opens the vector store with +/// `OpenIntent::Writer`. When a second live instance already holds the +/// redb write lock, the Writer open MUST fail loud (after the bounded +/// handoff window) and MUST NOT return a read-only snapshot handle — +/// otherwise every `upsert`/`remove` would be silently rejected for the +/// daemon's lifetime (the original bug). +/// What: Seeds the vector file, drops the store so the cache expires, +/// holds the redb file lock with a raw handle, then calls +/// `UsearchStore::new_with_intent(.., Writer)`. The call must return `Err` +/// naming the lock conflict — never an `Ok` snapshot handle. +/// Test: this test. +#[tokio::test] +async fn writer_intent_open_fails_loud_on_locked_vector_file() { + let dir = tempdir().unwrap(); + let logical = dir.path().join("test.usearch"); + + // Populate and drop so the cache entry expires. + { + let primary = UsearchStore::new(logical.clone(), 384).unwrap(); + primary + .upsert(Uuid::new_v4(), unit_vec(384, 1)) + .await + .unwrap(); + } + + // Hold the redb file lock with a raw `Database::create`. + let redb_path = redb_path_for(&logical); + let _live = redb::Database::create(&redb_path).expect("lock vector redb"); + + // Writer open must fail loud, never snapshot. + let result = UsearchStore::new_with_intent(logical.clone(), 384, OpenIntent::Writer); + // Match rather than `unwrap_err()` so we don't require UsearchStore: Debug. + let err = match result { + Ok(_) => panic!( + "Writer open on a locked vector redb must fail loud, not return a snapshot handle" + ), + Err(e) => e, + }; + // Use the alternate `{:#}` form so the full anyhow context chain + // (the `open_or_get_cached_db` wrapper + the root lock message) is + // rendered, not just the outermost context line. + let msg = format!("{err:#}"); + assert!( + msg.contains("still locked") || msg.contains("write access"), + "Writer error must name the lock conflict; got: {msg}" + ); +} + +/// Why (issue #59): `upsert` and `remove` on a snapshot handle must +/// return an error that includes the read-only sentinel text so callers +/// see actionable guidance. This tests the storage-layer write guard +/// independently of the daemon-level `single_instance_check`. +/// What: Seeds a vector file, drops the store so the cache expires, +/// holds the lock, opens a snapshot handle, then asserts both write +/// methods Err with the expected message. +/// Test: this test — `vector_writes_rejected_on_snapshot`. +#[tokio::test] +async fn vector_writes_rejected_on_snapshot() { + let dir = tempdir().unwrap(); + let logical = dir.path().join("test.usearch"); + + // Populate and drop so the cache entry expires. + { + let primary = UsearchStore::new(logical.clone(), 384).unwrap(); + primary + .upsert(Uuid::new_v4(), unit_vec(384, 1)) + .await + .unwrap(); + } + + // Hold the lock so the next open takes the snapshot path. + let redb_path = redb_path_for(&logical); + let _live = redb::Database::create(&redb_path).expect("lock vector redb"); + + let snap = UsearchStore::new(logical.clone(), 384).expect("snapshot open must succeed"); + assert!(snap.is_read_only()); + + // upsert must fail with read-only guidance. + let err = snap + .upsert(Uuid::new_v4(), unit_vec(384, 99)) + .await + .unwrap_err(); + let msg = err.to_string(); + assert!( + msg.contains("read-only"), + "upsert on snapshot must mention read-only, got: {msg}" + ); + + // remove must fail with read-only guidance. + let err = snap.remove(Uuid::new_v4()).await.unwrap_err(); + let msg = err.to_string(); + assert!( + msg.contains("read-only"), + "remove on snapshot must mention read-only, got: {msg}" + ); +} + +/// Why (issue #59): reads must succeed on a snapshot handle — the +/// snapshot is a point-in-time copy of the live file and must be +/// searchable. +/// What: Seeds one vector, drops, acquires lock, opens snapshot, +/// searches, and asserts the seeded id is returned at rank 0. +/// Test: this test — `vector_remove_rejected_on_snapshot` (search +/// path — the symmetric read-succeeds counterpart). +#[tokio::test] +async fn vector_remove_rejected_on_snapshot() { + let dir = tempdir().unwrap(); + let logical = dir.path().join("test.usearch"); + let id = Uuid::new_v4(); + let v = unit_vec(384, 5); + + // Seed then drop so cache expires. + { + let primary = UsearchStore::new(logical.clone(), 384).unwrap(); + primary.upsert(id, v.clone()).await.unwrap(); + } + + // Hold the lock. + let redb_path = redb_path_for(&logical); + let _live = redb::Database::create(&redb_path).expect("lock vector redb"); + + let snap = UsearchStore::new(logical.clone(), 384).expect("snapshot open must succeed"); + assert!(snap.is_read_only()); + + // Search (read) must succeed and return the seeded vector. + let hits = snap.search(&v, 1).await.unwrap(); + assert_eq!( + hits.len(), + 1, + "search on snapshot must return seeded vector" + ); + assert_eq!(hits[0].drawer_id, id); + + // remove must be rejected. + let err = snap.remove(id).await.unwrap_err(); + assert!( + err.to_string().contains("read-only"), + "remove on snapshot must be rejected" + ); +} diff --git a/crates/trusty-memory/src/tools/palace_ops.rs b/crates/trusty-memory/src/tools/palace_ops.rs index ab26f0c06..ae99a3456 100644 --- a/crates/trusty-memory/src/tools/palace_ops.rs +++ b/crates/trusty-memory/src/tools/palace_ops.rs @@ -299,6 +299,16 @@ pub(crate) async fn handle_palace_reembed(state: &AppState, args: Value) -> Resu // reads identically to "the embedder is dropping writes". "embedder_ready": health.embedder_ready, "recorded_failures": health.recorded_failures.len(), + // #5005 / #5000: `missing` counts drawers with no vector key. An + // aliased drawer HAS a key and is still unretrievable, so `missing: 0` + // was a false all-clear on the palace that lost four of them. These + // three fields are the signal that catches it; gate deletions on + // `aliased` as well as `missing`. + "vector_key_rows": health.vector_key_rows, + "distinct_vector_ids": health.distinct_vector_ids, + "aliased": report.aliased, + "aliased_ids": report.aliased_ids + .iter().map(|i| i.to_string()).collect::>(), })) } From 5a5bfe7aaffc5050be5213f7897c9f8ba0832636 Mon Sep 17 00:00:00 2001 From: Bob Matsuoka Date: Thu, 6 Aug 2026 10:54:42 -0400 Subject: [PATCH 2/8] docs(trusty-memory): changelog fragment for the #5005 alias-detection fields MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 🤖🤖🤖 Generated with trusty-mpm — https://github.com/bobmatnyc/trusty-tools --- crates/trusty-memory/changelog.d/5005-hnsw-id-aliasing.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 crates/trusty-memory/changelog.d/5005-hnsw-id-aliasing.md diff --git a/crates/trusty-memory/changelog.d/5005-hnsw-id-aliasing.md b/crates/trusty-memory/changelog.d/5005-hnsw-id-aliasing.md new file mode 100644 index 000000000..df4265b63 --- /dev/null +++ b/crates/trusty-memory/changelog.d/5005-hnsw-id-aliasing.md @@ -0,0 +1,5 @@ +Fixed + +- `palace_reembed` no longer gives a false all-clear for drawers whose vector was overwritten by another drawer's (closes [#5005](https://github.com/bobmatnyc/trusty-tools/issues/5005)) + - it now returns `vector_key_rows`, `distinct_vector_ids`, `aliased`, and `aliased_ids` alongside `missing`; `missing` counts drawers with no vector key, and an aliased drawer HAS a key, so `missing: 0` was reported for a palace with four unretrievable drawers + - a deletion-bearing workflow must gate on `aliased` as well as `missing` ([#5000](https://github.com/bobmatnyc/trusty-tools/issues/5000) resolution item 3) From 23fb528984727c04027eb61616539d7949326d94 Mon Sep 17 00:00:00 2001 From: Bob Matsuoka Date: Thu, 6 Aug 2026 11:30:55 -0400 Subject: [PATCH 3/8] fix(trusty-common): never report an unread alias audit as clean, and bound allocation by both vector tables (#5005) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review findings on PR #5013. 1. HIGH — `embed_health` swallowed a failed alias audit into `(0, 0, [])`, which made `is_healthy()` true and `palace_reembed` report `aliased: 0` on the exact signal #5005 exists to provide. A failure branch leaving state that looks successful is the defect shape this PR was written to remove. `AliasAudit` is now an enum: `Measured { .. }` or `Unavailable { reason }`. `is_healthy()` is false for `Unavailable`, `palace_reembed` names the state (`clean` / `aliased` / `unavailable`) and reports null counts rather than zeros, and the Result-to-outcome mapping lives in `AliasAudit::from_scan` so `embed_health` has no error branch of its own left to get wrong. 2. MEDIUM — the allocator's `high_water` bound read `VECTORS` only, but a `VECTOR_KEYS` row can outlive its `VECTORS` row: `compact_orphans` splits the live-id read, the orphan computation, and the delete across three transactions, so an `upsert` landing between the first and the third has its brand-new id classed as an orphan and its vector row removed while the key survives. `high_water` now clears the highest id either table knows about, so the correction path cannot hand an id back to a surviving key. 3. LOW — `VECTOR_ID_SEQ` was missing from the table-name collision test. Each fix has a test that fails without it, confirmed by breaking the named mechanism: restoring the zeros in `from_scan`'s error arm, reverting `high_water` to `vectors.last()? + 1`, and giving `VECTOR_ID_SEQ` a colliding name each turn exactly one test red. Refs #5005 🤖🤖🤖 Generated with trusty-mpm — https://github.com/bobmatnyc/trusty-tools --- .../changelog.d/5005-hnsw-id-aliasing.md | 3 +- .../src/memory_core/retrieval/embed_repair.rs | 154 ++++++++++++++---- .../retrieval/embed_repair_tests.rs | 77 ++++++++- .../src/memory_core/retrieval/mod.rs | 2 +- .../src/memory_core/store/hnsw_store.rs | 56 +++++-- .../src/memory_core/store/hnsw_store/tests.rs | 63 +++++++ .../src/memory_core/store/kg_store.rs | 1 + .../changelog.d/5005-hnsw-id-aliasing.md | 4 +- crates/trusty-memory/src/tools/palace_ops.rs | 35 +++- 9 files changed, 322 insertions(+), 73 deletions(-) diff --git a/crates/trusty-common/changelog.d/5005-hnsw-id-aliasing.md b/crates/trusty-common/changelog.d/5005-hnsw-id-aliasing.md index bc0ea7d85..a2242e95c 100644 --- a/crates/trusty-common/changelog.d/5005-hnsw-id-aliasing.md +++ b/crates/trusty-common/changelog.d/5005-hnsw-id-aliasing.md @@ -3,4 +3,5 @@ Fixed - `HnswStore` no longer aliases vector ids across two live stores over one palace file, which silently overwrote one drawer's embedding with another's (closes [#5005](https://github.com/bobmatnyc/trusty-tools/issues/5005)) - the vector-id counter now lives in redb (`vector_id_seq`) and is reserved inside the same write transaction as the insert, so every writer on the file serialises against it; an existing palace has its counter seeded to the file's high-water mark on open, and re-raised on every subsequent open so a rolling upgrade cannot leave it behind - `upsert` refuses an id that already has a `VECTORS` row: it allocates past it, or fails with `IdAllocationFailed` — it never overwrites - - `PalaceHandle::embed_health` and `palace_reembed` now report `vector_key_rows`, `distinct_vector_ids`, and the aliased drawer ids; key presence alone reported a false all-clear for this class, and `is_healthy()` is now false when any drawer is aliased + - `PalaceHandle::embed_health` and `palace_reembed` now carry an `AliasAudit`: key presence alone reported a false all-clear for this class, and `is_healthy()` is now false when any drawer is aliased + - an alias audit that could not run is `AliasAudit::Unavailable`, not zeros; `is_healthy()` is false for it, so a failed scan can never be read as a clean palace diff --git a/crates/trusty-common/src/memory_core/retrieval/embed_repair.rs b/crates/trusty-common/src/memory_core/retrieval/embed_repair.rs index b5862e600..c077d31a0 100644 --- a/crates/trusty-common/src/memory_core/retrieval/embed_repair.rs +++ b/crates/trusty-common/src/memory_core/retrieval/embed_repair.rs @@ -32,6 +32,103 @@ use anyhow::{Context, Result}; use std::collections::HashSet; use uuid::Uuid; +/// Whether the vector-id alias audit actually ran (#5005). +/// +/// Why: the audit's whole job is to answer "is any drawer's vector owned by a +/// different drawer". A failed scan has NO answer, and reporting it as zeros +/// would be the exact defect this ticket exists to fix, one level up — a +/// failure branch leaving state that looks successful. So "could not tell" is a +/// state of its own, not a number: nothing can read it as clean by accident. +/// What: `Measured` carries the two counts and the aliased ids; `Unavailable` +/// carries why. [`EmbedHealth::is_healthy`] is false for `Unavailable`. +/// Test: `alias_audit_failure_is_never_reported_as_clean`. +#[derive(Debug, Clone)] +pub enum AliasAudit { + /// The audit ran; these numbers are authoritative. + Measured { + /// Rows in the `VECTOR_KEYS` table. + key_rows: usize, + /// Distinct vector ids those rows point at. Below `key_rows` exactly + /// when drawers share an id. + distinct_vector_ids: usize, + /// Drawers whose vector was overwritten by another drawer's. These have + /// a key, so they are NOT in `missing_vector_ids` — that is precisely + /// why the count gap missed them — but their content is embedded + /// nowhere. + aliased_drawer_ids: Vec, + }, + /// The audit could not run. Nothing is known about aliasing in this palace. + Unavailable { + /// The scan error, for the operator. + reason: String, + }, +} + +impl AliasAudit { + /// Turn a scan result into an outcome — the ONLY way this type is built + /// from a fallible read. + /// + /// Why: the mapping is where a failed scan could be laundered into a + /// clean-looking zero, so it is a named function with its own test rather + /// than an inline `match` arm inside `embed_health`. `embed_health` now has + /// no error branch of its own to get wrong. + /// What: `Ok` → `Measured`; `Err` → `Unavailable` carrying the rendered + /// error. Never returns zeros for a failure. + /// Test: `alias_audit_failure_is_never_reported_as_clean`. + pub fn from_scan(scan: anyhow::Result<(usize, usize, Vec)>) -> Self { + match scan { + Ok((key_rows, distinct_vector_ids, aliased_drawer_ids)) => Self::Measured { + key_rows, + distinct_vector_ids, + aliased_drawer_ids, + }, + Err(e) => Self::Unavailable { + reason: format!("{e:#}"), + }, + } + } + + /// Whether the audit ran AND found no drawer sharing an id. + /// + /// `Unavailable` is false: an unread palace is not a clean one. + pub fn is_clean(&self) -> bool { + matches!(self, Self::Measured { aliased_drawer_ids, .. } if aliased_drawer_ids.is_empty()) + } + + /// Drawers caught in a collision; empty when the audit did not run. + /// + /// Callers that branch on emptiness MUST check [`Self::is_clean`] instead — + /// empty here means "none found OR none looked for". + pub fn aliased_drawer_ids(&self) -> &[Uuid] { + match self { + Self::Measured { + aliased_drawer_ids, .. + } => aliased_drawer_ids, + Self::Unavailable { .. } => &[], + } + } + + /// `(key_rows, distinct_vector_ids)`, or `None` when the audit did not run. + pub fn counts(&self) -> Option<(usize, usize)> { + match self { + Self::Measured { + key_rows, + distinct_vector_ids, + .. + } => Some((*key_rows, *distinct_vector_ids)), + Self::Unavailable { .. } => None, + } + } + + /// Why the audit could not run, when it could not. + pub fn unavailable_reason(&self) -> Option<&str> { + match self { + Self::Unavailable { reason } => Some(reason), + Self::Measured { .. } => None, + } + } +} + /// Vector-coverage snapshot for one palace. /// /// Why: the question "is this palace's memory actually findable?" had no answer @@ -58,25 +155,19 @@ pub struct EmbedHealth { pub recorded_failures: Vec, /// Whether a shared embedder has initialised in this process. pub embedder_ready: bool, - /// Rows in the `VECTOR_KEYS` table (#5005). - pub vector_key_rows: usize, - /// Distinct vector ids those rows point at (#5005). Below - /// `vector_key_rows` exactly when drawers share an id. - pub distinct_vector_ids: usize, - /// Drawers whose vector was overwritten by another drawer's (#5005). These - /// have a key, so they are NOT in `missing_vector_ids` — that is precisely - /// why the count gap missed them — but their content is embedded nowhere. - pub aliased_drawer_ids: Vec, + /// Vector-id alias audit (#5005), including whether it ran at all. + pub alias_audit: AliasAudit, } impl EmbedHealth { /// Whether every live drawer is vector-searchable. /// /// #5005: an aliased drawer has a vector key and still resolves to nothing, - /// so key presence alone is not the health condition. Both sets must be - /// empty. + /// so key presence alone is not the health condition — and an alias audit + /// that could not run is not a passing one. Healthy requires no missing + /// drawers AND an audit that ran and came back clean. pub fn is_healthy(&self) -> bool { - self.missing_vector_ids.is_empty() && self.aliased_drawer_ids.is_empty() + self.missing_vector_ids.is_empty() && self.alias_audit.is_clean() } } @@ -130,14 +221,12 @@ pub struct VectorBackfillReport { /// Ids still without a vector after this run (including any skipped by /// `limit`), so a caller can act on the remainder. pub still_missing_ids: Vec, - /// Drawers sharing a vector id with another drawer (#5005). This run never - /// repairs them — a re-embed alone would not, since they already have a - /// key. A non-zero value means the palace is NOT clean however small - /// `missing` is, and it is the number a deletion-bearing workflow must gate - /// on alongside `missing` (#5000 resolution item 3). - pub aliased: usize, - /// The ids behind `aliased`. - pub aliased_ids: Vec, + /// The alias audit for this palace (#5005). This run never repairs an + /// aliased drawer — a re-embed alone would not, since it already has a key. + /// A deletion-bearing workflow must require `alias_audit.is_clean()` as well + /// as `missing == 0` (#5000 resolution item 3); an `Unavailable` audit + /// blocks exactly as a non-empty one does. + pub alias_audit: AliasAudit, } impl PalaceHandle { @@ -171,17 +260,13 @@ impl PalaceHandle { .map(|d| embed_ledger::load(d)) .unwrap_or_default(); // #5005: an aliased drawer has a key, so it is invisible to the set - // difference above. A redb scan failure must not be reported as "no - // aliasing" — log it and leave the counts at zero, which reads as - // unknown rather than clean because the row count is zero too. - let (vector_key_rows, distinct_vector_ids, aliased_drawer_ids) = - match self.vector_store.alias_audit() { - Ok(triple) => triple, - Err(e) => { - tracing::warn!(palace = %self.id, "#5005: alias audit failed: {e:#}"); - (0, 0, Vec::new()) - } - }; + // difference above. A failed scan becomes `Unavailable`, never zeros — + // a numeric zero standing in for "I could not tell" is the same + // false-all-clear shape this ticket exists to remove. + let alias_audit = AliasAudit::from_scan(self.vector_store.alias_audit()); + if let Some(reason) = alias_audit.unavailable_reason() { + tracing::error!(palace = %self.id, "#5005: alias audit failed: {reason}"); + } EmbedHealth { palace_id: self.id.as_str().to_string(), drawer_count: live.len(), @@ -189,9 +274,7 @@ impl PalaceHandle { missing_vector_ids, recorded_failures, embedder_ready: shared_embedder_initialized(), - vector_key_rows, - distinct_vector_ids, - aliased_drawer_ids, + alias_audit, } } @@ -227,8 +310,7 @@ impl PalaceHandle { repaired: 0, still_failing: 0, still_missing_ids: health.missing_vector_ids.clone(), - aliased: health.aliased_drawer_ids.len(), - aliased_ids: health.aliased_drawer_ids.clone(), + alias_audit: health.alias_audit.clone(), }; // A healthy palace short-circuits BEFORE touching the embedder, so the diff --git a/crates/trusty-common/src/memory_core/retrieval/embed_repair_tests.rs b/crates/trusty-common/src/memory_core/retrieval/embed_repair_tests.rs index 517483381..451a3376d 100644 --- a/crates/trusty-common/src/memory_core/retrieval/embed_repair_tests.rs +++ b/crates/trusty-common/src/memory_core/retrieval/embed_repair_tests.rs @@ -16,7 +16,7 @@ use super::deferred_embed::{ EmbedLoss, RetryPolicy, embed_and_store, embed_store_or_record, record_loss, }; -use super::embed_repair::VectorBackfillOptions; +use super::embed_repair::{AliasAudit, EmbedHealth, VectorBackfillOptions}; use super::embedder::seed_shared_embedder_with_mock; use super::handle::PalaceHandle; use crate::embedder::MockEmbedder; @@ -800,12 +800,12 @@ fn alias_audit_surfaces_a_collision() { health.missing_vector_ids.is_empty(), "the false all-clear this ticket is about: every aliased drawer has a key" ); - assert_eq!(health.vector_key_rows, 3); assert_eq!( - health.distinct_vector_ids, 1, + health.alias_audit.counts(), + Some((3, 1)), "three keys, one id — the gap is the detector" ); - let mut aliased = health.aliased_drawer_ids.clone(); + let mut aliased = health.alias_audit.aliased_drawer_ids().to_vec(); aliased.sort(); assert_eq!(aliased, ids, "every member of the group must be named"); assert!( @@ -848,10 +848,7 @@ fn unalias_marks_the_whole_group_for_reembed() { ); let after = handle.embed_health(); - assert!( - after.aliased_drawer_ids.is_empty(), - "no group survives the repair" - ); + assert!(after.alias_audit.is_clean(), "no group survives the repair"); let mut missing = after.missing_vector_ids.clone(); missing.sort(); assert_eq!( @@ -860,3 +857,67 @@ fn unalias_marks_the_whole_group_for_reembed() { ); assert!(!after.is_healthy(), "they still need a re-embed"); } + +/// Why (#5005, review finding): the alias audit is the ONLY signal that catches +/// an overwritten drawer, and the PR that added it tells operators to gate +/// deletions on it. A scan that fails and reports `(0, 0, [])` therefore ships +/// the exact defect this ticket exists to remove, one level up: a failure +/// branch leaving state that looks successful. `is_healthy()` would return true +/// and `palace_reembed` would say `aliased: 0` while nothing had been read. +/// What: takes a palace with a real three-way collision — a state +/// `alias_audit_surfaces_a_collision` proves is reported as unhealthy — and +/// asserts the `Unavailable` outcome is ALSO unhealthy, is not clean, names its +/// reason, and reports no counts rather than zeros. +/// Test: itself. Making `AliasAudit::from_scan`'s `Err` arm return +/// `Measured { key_rows: 0, distinct_vector_ids: 0, aliased_drawer_ids: vec![] }` +/// — the code this finding removed — makes `is_clean()` true and fails every +/// assertion here. +#[test] +fn alias_audit_failure_is_never_reported_as_clean() { + // Drive the REAL mapping with a failed scan, not a hand-built enum value: + // this is the branch `embed_health` takes, and the only place a failure + // could be laundered into a clean-looking zero. + let unavailable = AliasAudit::from_scan(Err(anyhow::anyhow!( + "redb storage error: simulated scan failure" + ))); + assert!( + !unavailable.is_clean(), + "an audit that could not run must never read as clean" + ); + assert_eq!( + unavailable.counts(), + None, + "no counts at all — a zero would be misread as 'measured, nothing found'" + ); + assert!( + unavailable.unavailable_reason().is_some(), + "the reason must survive to the operator" + ); + + // The health verdict a caller actually gates on. + let health = EmbedHealth { + palace_id: "unreadable".to_string(), + drawer_count: 3, + vector_count: 3, + missing_vector_ids: Vec::new(), + recorded_failures: Vec::new(), + embedder_ready: true, + alias_audit: unavailable, + }; + assert!( + !health.is_healthy(), + "zero missing drawers plus an unreadable alias audit is NOT healthy" + ); + + // Contrast: the same shape with a measured-clean audit IS healthy, so the + // assertion above is about the unknown state and not about something else. + let measured = EmbedHealth { + alias_audit: AliasAudit::Measured { + key_rows: 3, + distinct_vector_ids: 3, + aliased_drawer_ids: Vec::new(), + }, + ..health + }; + assert!(measured.is_healthy(), "a measured-clean palace is healthy"); +} diff --git a/crates/trusty-common/src/memory_core/retrieval/mod.rs b/crates/trusty-common/src/memory_core/retrieval/mod.rs index 66cf20c7e..12ef6c499 100644 --- a/crates/trusty-common/src/memory_core/retrieval/mod.rs +++ b/crates/trusty-common/src/memory_core/retrieval/mod.rs @@ -52,7 +52,7 @@ pub use handle::PalaceHandle; // public because a caller choosing to run a longer policy than the write-path // default is a legitimate operator decision. pub use deferred_embed::RetryPolicy; -pub use embed_repair::{EmbedHealth, VectorBackfillOptions, VectorBackfillReport}; +pub use embed_repair::{AliasAudit, EmbedHealth, VectorBackfillOptions, VectorBackfillReport}; // Recall scoping (ADR-0027 T9) pub use scope::{RecallScope, list_drawers_in_wing, scope_admits}; diff --git a/crates/trusty-common/src/memory_core/store/hnsw_store.rs b/crates/trusty-common/src/memory_core/store/hnsw_store.rs index bcd006a71..9d2bc31c1 100644 --- a/crates/trusty-common/src/memory_core/store/hnsw_store.rs +++ b/crates/trusty-common/src/memory_core/store/hnsw_store.rs @@ -165,9 +165,10 @@ impl From for HnswStoreError { /// What: reads `VECTOR_ID_SEQ`, takes the first candidate with no `VECTORS` /// row, writes `candidate + 1` back, and returns the candidate. When the /// candidate IS occupied — a counter left behind by a pre-#5005 binary, or a -/// hand-edited file — it jumps past the highest occupied id rather than -/// overwriting, logs the correction, and retries; after [`MAX_ALLOC_PROBES`] -/// it fails with [`HnswStoreError::IdAllocationFailed`] instead of aliasing. +/// hand-edited file — it jumps past the highest id EITHER vector table knows +/// about (see [`high_water`]) rather than overwriting, logs the correction, and +/// retries; after [`MAX_ALLOC_PROBES`] it fails with +/// [`HnswStoreError::IdAllocationFailed`] instead of aliasing. /// A missing counter row (only reachable if the store skipped open-time /// seeding) falls back to the same high-water mark the seed would have used. /// Test: `upsert_refuses_to_reuse_an_id_already_present_in_vectors`, @@ -175,6 +176,7 @@ impl From for HnswStoreError { fn allocate_vector_id( seq: &mut Table<'_, &'static str, u64>, vectors: &Table<'_, u64, &'static [u8]>, + keys: &Table<'_, &'static str, u64>, ) -> Result { let mut candidate = match seq.get(NEXT_VECTOR_ID)? { Some(g) => g.value(), @@ -183,7 +185,7 @@ fn allocate_vector_id( "#5005: VECTOR_ID_SEQ has no counter row at allocation time; \ falling back to the VECTORS high-water mark" ); - high_water(vectors)? + high_water(vectors, keys)? } }; @@ -199,7 +201,7 @@ fn allocate_vector_id( skipping the occupied id instead of overwriting it" ); // Jump past the highest occupied id so one correction is enough. - candidate = high_water(vectors)?.max(candidate.saturating_add(1)); + candidate = high_water(vectors, keys)?.max(candidate.saturating_add(1)); } Err(HnswStoreError::IdAllocationFailed { @@ -208,20 +210,38 @@ fn allocate_vector_id( }) } -/// One past the highest `vector_id` present in `VECTORS` (1 when empty). +/// One past the highest `vector_id` either vector table knows about. /// /// Why: both the allocator's collision jump and its missing-counter fallback -/// need the same "no id at or above this is taken" bound. `VECTORS` is a -/// B-tree keyed by `u64`, so `last()` is O(log n) — cheap enough to call on the -/// rare correction path. -/// What: `last()? + 1`, or 1 for an empty table (id 0 is never issued, matching -/// the pre-#5005 seed of `max_seen + 1`). -/// Test: exercised by `upsert_refuses_to_reuse_an_id_already_present_in_vectors`. -fn high_water(vectors: &Table<'_, u64, &'static [u8]>) -> Result { - Ok(match vectors.last()? { - Some((k, _)) => k.value().saturating_add(1), - None => 1, - }) +/// need a bound above which NO id is taken. `VECTORS` alone is not that bound. +/// A `VECTOR_KEYS` row can outlive its `VECTORS` row: `compact_orphans` reads +/// the live-id set, computes orphans, and deletes them in three separate +/// transactions, so an `upsert` that lands between the first and the third has +/// its brand-new id treated as an orphan and its `VECTORS` row removed while +/// the key survives. Under the in-process concurrency this store supports that +/// is a reachable state, and a `VECTORS`-only bound would hand the id straight +/// back out — the same aliasing this module exists to prevent, arrived at from +/// the other table. +/// What: `max(last VECTORS key, largest mapped VECTOR_KEYS value) + 1`, or 1 +/// when both are empty (id 0 is never issued, matching the pre-#5005 seed of +/// `max_seen + 1`). `VECTORS.last()` is O(log n); the `VECTOR_KEYS` sweep is +/// O(n) but runs only on the rare correction path — never on a healthy upsert, +/// where the counter's own invariant already bounds the candidate. +/// Test: `upsert_refuses_to_reuse_an_id_already_present_in_vectors`, +/// `upsert_refuses_an_id_that_only_vector_keys_still_claims`. +fn high_water( + vectors: &Table<'_, u64, &'static [u8]>, + keys: &Table<'_, &'static str, u64>, +) -> Result { + let mut max_seen = match vectors.last()? { + Some((k, _)) => k.value(), + None => 0, + }; + for entry in keys.iter()? { + let (_, v) = entry?; + max_seen = max_seen.max(v.value()); + } + Ok(max_seen.saturating_add(1)) } /// Public result alias to keep call-site signatures concise. @@ -506,7 +526,7 @@ impl HnswStore { Some(id) => id, None => { // #5005: allocate from redb, inside this txn. - let id = allocate_vector_id(&mut seq, &vectors)?; + let id = allocate_vector_id(&mut seq, &vectors, &keys)?; keys.insert(uuid, id)?; id } diff --git a/crates/trusty-common/src/memory_core/store/hnsw_store/tests.rs b/crates/trusty-common/src/memory_core/store/hnsw_store/tests.rs index a0c6766fd..5ed1c8a36 100644 --- a/crates/trusty-common/src/memory_core/store/hnsw_store/tests.rs +++ b/crates/trusty-common/src/memory_core/store/hnsw_store/tests.rs @@ -187,6 +187,17 @@ fn read_seq(store: &HnswStore) -> Option { seq.get(NEXT_VECTOR_ID).unwrap().map(|g| g.value()) } +/// Delete the persisted counter, forcing the allocator onto its high-water +/// fallback. Models a palace whose counter row never existed. +fn set_seq_absent(store: &HnswStore) { + let wtx = store.db.begin_write().unwrap(); + { + let mut seq = wtx.open_table(VECTOR_ID_SEQ).unwrap(); + seq.remove(NEXT_VECTOR_ID).unwrap(); + } + wtx.commit().unwrap(); +} + /// Overwrite the persisted counter. Models the exact state a pre-#5005 /// binary leaves behind: rows written, counter untouched. fn set_seq(store: &HnswStore, value: u64) { @@ -525,3 +536,55 @@ fn dimension_mismatch_is_rejected() { other => panic!("wrong error variant: {other:?}"), } } + +/// Why (#5005, review finding): the occupancy probe reads `VECTORS`, but a +/// `VECTOR_KEYS` row can outlive its `VECTORS` row. `compact_orphans` snapshots +/// the live-id set, computes orphans, and deletes them in three separate +/// transactions, so an `upsert` landing between the first and the third has its +/// brand-new id classed as an orphan and its `VECTORS` row removed while the +/// key survives — reachable under exactly the in-process concurrency this store +/// supports. A `VECTORS`-only bound would then hand that id straight back out +/// and alias the surviving key, arriving at the same defect from the other +/// table. +/// What: builds that state directly — a `VECTOR_KEYS` row claiming id 7 with no +/// `VECTORS` row anywhere — clears the counter so allocation must fall back to +/// the high-water mark, then upserts. The new drawer must not take 7. +/// Test: this test itself is the verification. Reverting `high_water` to +/// `vectors.last()? + 1` makes the fallback return 1, the probe sees `VECTORS` +/// empty, and the upsert takes an id `VECTOR_KEYS` already claims — failing the +/// `assert_ne!` and the audit. +#[test] +fn upsert_refuses_an_id_that_only_vector_keys_still_claims() { + let (_dir, store) = open_store(8); + let stranded = Uuid::new_v4().to_string(); + + // The post-`compact_orphans`-race state: key present, vector row gone. + { + let wtx = store.db.begin_write().unwrap(); + { + let mut keys = wtx.open_table(VECTOR_KEYS).unwrap(); + keys.insert(stranded.as_str(), 7u64).unwrap(); + } + wtx.commit().unwrap(); + } + set_seq_absent(&store); + + let fresh = Uuid::new_v4().to_string(); + let id = store.upsert(&fresh, &unit_vec(8, 21)).expect("upsert"); + assert_ne!( + id, 7, + "an id still claimed by a VECTOR_KEYS row must not be re-issued, even with no VECTORS row" + ); + assert!( + id > 7, + "the fallback must clear every id either table knows about, got {id}" + ); + + let audit = store.audit_aliases().expect("audit"); + assert!( + audit.is_clean(), + "a VECTOR_KEYS-only id must not become an alias: {audit:?}" + ); + assert_eq!(audit.key_rows, 2); + assert_eq!(audit.distinct_vector_ids, 2); +} diff --git a/crates/trusty-common/src/memory_core/store/kg_store.rs b/crates/trusty-common/src/memory_core/store/kg_store.rs index f4a6f3bed..12e4ff5b6 100644 --- a/crates/trusty-common/src/memory_core/store/kg_store.rs +++ b/crates/trusty-common/src/memory_core/store/kg_store.rs @@ -633,6 +633,7 @@ mod tests { VECTORS.name(), VECTOR_KEYS.name(), DELETED_VECTORS.name(), + VECTOR_ID_SEQ.name(), ]; for i in 0..names.len() { for j in (i + 1)..names.len() { diff --git a/crates/trusty-memory/changelog.d/5005-hnsw-id-aliasing.md b/crates/trusty-memory/changelog.d/5005-hnsw-id-aliasing.md index df4265b63..8a010a39a 100644 --- a/crates/trusty-memory/changelog.d/5005-hnsw-id-aliasing.md +++ b/crates/trusty-memory/changelog.d/5005-hnsw-id-aliasing.md @@ -1,5 +1,5 @@ Fixed - `palace_reembed` no longer gives a false all-clear for drawers whose vector was overwritten by another drawer's (closes [#5005](https://github.com/bobmatnyc/trusty-tools/issues/5005)) - - it now returns `vector_key_rows`, `distinct_vector_ids`, `aliased`, and `aliased_ids` alongside `missing`; `missing` counts drawers with no vector key, and an aliased drawer HAS a key, so `missing: 0` was reported for a palace with four unretrievable drawers - - a deletion-bearing workflow must gate on `aliased` as well as `missing` ([#5000](https://github.com/bobmatnyc/trusty-tools/issues/5000) resolution item 3) + - it now returns `alias_audit` (`clean` | `aliased` | `unavailable`), `alias_audit_error`, `vector_key_rows`, `distinct_vector_ids`, `aliased`, and `aliased_ids` alongside `missing`; `missing` counts drawers with no vector key, and an aliased drawer HAS a key, so `missing: 0` was reported for a palace with four unretrievable drawers + - a failed audit reports `unavailable` with null counts, never zeros — a deletion-bearing workflow must require `alias_audit == "clean"` as well as `missing == 0` ([#5000](https://github.com/bobmatnyc/trusty-tools/issues/5000) resolution item 3) diff --git a/crates/trusty-memory/src/tools/palace_ops.rs b/crates/trusty-memory/src/tools/palace_ops.rs index ae99a3456..9e5e82fd1 100644 --- a/crates/trusty-memory/src/tools/palace_ops.rs +++ b/crates/trusty-memory/src/tools/palace_ops.rs @@ -301,17 +301,38 @@ pub(crate) async fn handle_palace_reembed(state: &AppState, args: Value) -> Resu "recorded_failures": health.recorded_failures.len(), // #5005 / #5000: `missing` counts drawers with no vector key. An // aliased drawer HAS a key and is still unretrievable, so `missing: 0` - // was a false all-clear on the palace that lost four of them. These - // three fields are the signal that catches it; gate deletions on - // `aliased` as well as `missing`. - "vector_key_rows": health.vector_key_rows, - "distinct_vector_ids": health.distinct_vector_ids, - "aliased": report.aliased, - "aliased_ids": report.aliased_ids + // was a false all-clear on the palace that lost four of them. Gate + // deletions on `alias_audit == "clean"` as well as `missing == 0`: + // `"unavailable"` means the scan failed and nothing is known, which is + // a block, not a pass. `vector_key_rows` / `distinct_vector_ids` are + // null in that case rather than 0, so no zero can be misread as clean. + "alias_audit": alias_audit_state(&report.alias_audit), + "alias_audit_error": report.alias_audit.unavailable_reason(), + "vector_key_rows": report.alias_audit.counts().map(|(rows, _)| rows), + "distinct_vector_ids": report.alias_audit.counts().map(|(_, ids)| ids), + "aliased": report.alias_audit.aliased_drawer_ids().len(), + "aliased_ids": report.alias_audit.aliased_drawer_ids() .iter().map(|i| i.to_string()).collect::>(), })) } +/// One word for how the #5005 alias audit went, for the `palace_reembed` payload. +/// +/// Why: a caller has to be able to tell "no drawer is aliased" from "the scan +/// failed and nothing is known" without inspecting counts — the second must +/// never read as the first. +/// What: `"clean"`, `"aliased"`, or `"unavailable"`. +/// Test: `dispatch_palace_reembed_dry_run_reports_counts` in `tools::tests`. +fn alias_audit_state(audit: &trusty_common::memory_core::retrieval::AliasAudit) -> &'static str { + if audit.unavailable_reason().is_some() { + "unavailable" + } else if audit.is_clean() { + "clean" + } else { + "aliased" + } +} + pub(crate) async fn handle_palace_compact(state: &AppState, args: Value) -> Result { let palace = resolve_palace(state, &args, "palace_compact")?; let handle = open_palace_handle(state, &palace)?; From a3ee2aa2693258876e18d172b9fb4e2cb8818cb2 Mon Sep 17 00:00:00 2001 From: Bob Matsuoka Date: Thu, 6 Aug 2026 11:52:21 -0400 Subject: [PATCH 4/8] fix(trusty-memory): report a null aliased count for an unread alias audit (#5005) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `palace_reembed` reported `"aliased": 0` when the alias audit could not run, while the two fields beside it correctly reported null. Nothing is misled today — four adjacent signals in the same object say `unavailable` — but a zero standing in for "I could not tell" is the exact defect class this PR exists to eliminate, and a consumer reading that one field in isolation is the failure mode #5005 documents. Fixed at the source rather than at the consumer: `AliasAudit::aliased_drawer_ids` returned `&[]` for `Unavailable`, so a caller could reach a length without ever deciding what to do about the unknown. It now returns `Option<&[Uuid]>`, which makes the zero unrepresentable — `Some(&[])` means "looked, found nothing", the only state a zero legitimately describes. `aliased` and `aliased_ids` are both null when the audit did not run. Systematic sweep of the rest of the payload and its siblings found no second instance of this failure mode. Two other swallow sites exist and are deliberately unchanged: `UsearchStore::all_ids` returns an empty vec on a redb scan failure, which makes every live drawer read as missing — fail-CLOSED, it over-reports and blocks, so it cannot produce a false all-clear; and `embed_ledger::load` treats an unreadable ledger as empty, which is #4906's tested, deliberate behaviour and feeds context rather than a gate. Refs #5005 🤖🤖🤖 Generated with trusty-mpm — https://github.com/bobmatnyc/trusty-tools --- .../src/memory_core/retrieval/embed_repair.rs | 19 +++++++++++++------ .../retrieval/embed_repair_tests.rs | 18 +++++++++++++++++- .../changelog.d/5005-hnsw-id-aliasing.md | 2 +- crates/trusty-memory/src/tools/palace_ops.rs | 8 ++++++-- 4 files changed, 37 insertions(+), 10 deletions(-) diff --git a/crates/trusty-common/src/memory_core/retrieval/embed_repair.rs b/crates/trusty-common/src/memory_core/retrieval/embed_repair.rs index c077d31a0..3e5a597b4 100644 --- a/crates/trusty-common/src/memory_core/retrieval/embed_repair.rs +++ b/crates/trusty-common/src/memory_core/retrieval/embed_repair.rs @@ -95,16 +95,23 @@ impl AliasAudit { matches!(self, Self::Measured { aliased_drawer_ids, .. } if aliased_drawer_ids.is_empty()) } - /// Drawers caught in a collision; empty when the audit did not run. + /// Drawers caught in a collision, or `None` when the audit did not run. /// - /// Callers that branch on emptiness MUST check [`Self::is_clean`] instead — - /// empty here means "none found OR none looked for". - pub fn aliased_drawer_ids(&self) -> &[Uuid] { + /// Why: this returned `&[]` for `Unavailable` in the first cut, and the + /// `palace_reembed` payload then reported `aliased: 0` for a palace nobody + /// had read — the same zero-standing-in-for-unknown this ticket exists to + /// remove, one field short of the two beside it. An `Option` makes that + /// unrepresentable: a caller cannot reach a length without first deciding + /// what to do about the `None`. + /// What: `Some(ids)` only when the audit ran. `Some(&[])` means "looked, + /// found nothing" — the only state a zero legitimately describes. + /// Test: `alias_audit_failure_is_never_reported_as_clean`. + pub fn aliased_drawer_ids(&self) -> Option<&[Uuid]> { match self { Self::Measured { aliased_drawer_ids, .. - } => aliased_drawer_ids, - Self::Unavailable { .. } => &[], + } => Some(aliased_drawer_ids), + Self::Unavailable { .. } => None, } } diff --git a/crates/trusty-common/src/memory_core/retrieval/embed_repair_tests.rs b/crates/trusty-common/src/memory_core/retrieval/embed_repair_tests.rs index 451a3376d..79ca513de 100644 --- a/crates/trusty-common/src/memory_core/retrieval/embed_repair_tests.rs +++ b/crates/trusty-common/src/memory_core/retrieval/embed_repair_tests.rs @@ -805,7 +805,11 @@ fn alias_audit_surfaces_a_collision() { Some((3, 1)), "three keys, one id — the gap is the detector" ); - let mut aliased = health.alias_audit.aliased_drawer_ids().to_vec(); + let mut aliased = health + .alias_audit + .aliased_drawer_ids() + .expect("a measured audit must expose its ids") + .to_vec(); aliased.sort(); assert_eq!(aliased, ids, "every member of the group must be named"); assert!( @@ -893,6 +897,13 @@ fn alias_audit_failure_is_never_reported_as_clean() { unavailable.unavailable_reason().is_some(), "the reason must survive to the operator" ); + // Review follow-up: this accessor used to return `&[]` here, and the + // `palace_reembed` payload reported `aliased: 0` for a palace nobody had + // read — the same zero-for-unknown, one field short of the two beside it. + assert!( + unavailable.aliased_drawer_ids().is_none(), + "an unread audit must expose NO id list — an empty one reads as 'looked, found nothing'" + ); // The health verdict a caller actually gates on. let health = EmbedHealth { @@ -920,4 +931,9 @@ fn alias_audit_failure_is_never_reported_as_clean() { ..health }; assert!(measured.is_healthy(), "a measured-clean palace is healthy"); + assert_eq!( + measured.alias_audit.aliased_drawer_ids(), + Some(&[][..]), + "a measured-clean audit exposes an EMPTY list — what a zero legitimately means" + ); } diff --git a/crates/trusty-memory/changelog.d/5005-hnsw-id-aliasing.md b/crates/trusty-memory/changelog.d/5005-hnsw-id-aliasing.md index 8a010a39a..d2ef83e87 100644 --- a/crates/trusty-memory/changelog.d/5005-hnsw-id-aliasing.md +++ b/crates/trusty-memory/changelog.d/5005-hnsw-id-aliasing.md @@ -2,4 +2,4 @@ Fixed - `palace_reembed` no longer gives a false all-clear for drawers whose vector was overwritten by another drawer's (closes [#5005](https://github.com/bobmatnyc/trusty-tools/issues/5005)) - it now returns `alias_audit` (`clean` | `aliased` | `unavailable`), `alias_audit_error`, `vector_key_rows`, `distinct_vector_ids`, `aliased`, and `aliased_ids` alongside `missing`; `missing` counts drawers with no vector key, and an aliased drawer HAS a key, so `missing: 0` was reported for a palace with four unretrievable drawers - - a failed audit reports `unavailable` with null counts, never zeros — a deletion-bearing workflow must require `alias_audit == "clean"` as well as `missing == 0` ([#5000](https://github.com/bobmatnyc/trusty-tools/issues/5000) resolution item 3) + - a failed audit reports `unavailable` with null in EVERY count-shaped field — `aliased` and `aliased_ids` included, never zeros or empty arrays — a deletion-bearing workflow must require `alias_audit == "clean"` as well as `missing == 0` ([#5000](https://github.com/bobmatnyc/trusty-tools/issues/5000) resolution item 3) diff --git a/crates/trusty-memory/src/tools/palace_ops.rs b/crates/trusty-memory/src/tools/palace_ops.rs index 9e5e82fd1..b727fa340 100644 --- a/crates/trusty-memory/src/tools/palace_ops.rs +++ b/crates/trusty-memory/src/tools/palace_ops.rs @@ -310,9 +310,13 @@ pub(crate) async fn handle_palace_reembed(state: &AppState, args: Value) -> Resu "alias_audit_error": report.alias_audit.unavailable_reason(), "vector_key_rows": report.alias_audit.counts().map(|(rows, _)| rows), "distinct_vector_ids": report.alias_audit.counts().map(|(_, ids)| ids), - "aliased": report.alias_audit.aliased_drawer_ids().len(), + // `aliased` reported 0 for an unreadable audit in the first cut, while + // the two fields above it correctly reported null. Every count-shaped + // field in this object is now absent rather than zero when nothing was + // read — a lone zero is exactly the misreading #5005 documents. + "aliased": report.alias_audit.aliased_drawer_ids().map(<[Uuid]>::len), "aliased_ids": report.alias_audit.aliased_drawer_ids() - .iter().map(|i| i.to_string()).collect::>(), + .map(|ids| ids.iter().map(|i| i.to_string()).collect::>()), })) } From 246266392127578230872e8d21e1f73a963acd3b Mon Sep 17 00:00:00 2001 From: bobmatnyc Date: Fri, 7 Aug 2026 00:00:18 -0400 Subject: [PATCH 5/8] feat(trusty-memory): wire the HNSW alias repair to a callable palace_unalias tool (#5005) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `unalias()` had zero call sites. #5013 stops new aliasing and makes existing aliasing visible through `palace_reembed`'s audit, but the repair itself was code an operator could not run — so the three aliased drawers in the live trusty-tools palace stayed durable-but-unretrievable, which is what blocks #4834. Adds `PalaceHandle::repair_aliases` and the `palace_unalias` MCP tool over it. Dry-run by default, mirroring `palace_reembed` — with more at stake, since this one deletes `VECTOR_KEYS` rows. The result names the drawer id SET, never a count: #5005 was a count (`missing: 0`) reporting all-clear over real loss, and a repair answering "3 repaired" without saying which three is that same defect one layer up. Those ids are also the operator's re-embed worklist. It cannot fail open. `Repaired` is reachable only after a post-repair audit ran, came back clean, and accounted for every id the pre-repair audit named; every other ending is its own variant (`Partial`, `Unavailable`), and neither reads as success. An unreadable audit refuses to write at all rather than deleting keys blind. `UsearchStore::unalias` now returns `UnaliasOutcome` and carries the keys it freed but could not parse into a drawer id — it used to drop them, which put a freed drawer nobody knew to repair inside a reported success. Idempotent: a second run finds no group, reports `clean`, and writes nothing. Not run against any live palace — that stays an operator action, with a backup taken immediately beforehand. Closes #5005 🤖🤖🤖 Generated with trusty-mpm — https://github.com/bobmatnyc/trusty-tools --- .../changelog.d/5005-hnsw-id-aliasing.md | 2 + .../src/memory_core/retrieval/embed_repair.rs | 237 +++++++++++ .../retrieval/embed_repair_tests.rs | 394 +++++++++++++++++- .../src/memory_core/retrieval/mod.rs | 5 +- .../src/memory_core/store/vector.rs | 57 ++- .../changelog.d/5005-palace-unalias.md | 4 + crates/trusty-memory/src/lib_tests.rs | 4 +- crates/trusty-memory/src/mcp_service.rs | 8 +- crates/trusty-memory/src/openrpc.rs | 2 + crates/trusty-memory/src/tools/definitions.rs | 12 + crates/trusty-memory/src/tools/mod.rs | 4 +- crates/trusty-memory/src/tools/palace_ops.rs | 73 ++++ crates/trusty-memory/src/tools/tests.rs | 45 +- crates/trusty-memory/src/transport/rpc.rs | 1 + 14 files changed, 825 insertions(+), 23 deletions(-) create mode 100644 crates/trusty-memory/changelog.d/5005-palace-unalias.md diff --git a/crates/trusty-common/changelog.d/5005-hnsw-id-aliasing.md b/crates/trusty-common/changelog.d/5005-hnsw-id-aliasing.md index a2242e95c..49fc0a504 100644 --- a/crates/trusty-common/changelog.d/5005-hnsw-id-aliasing.md +++ b/crates/trusty-common/changelog.d/5005-hnsw-id-aliasing.md @@ -5,3 +5,5 @@ Fixed - `upsert` refuses an id that already has a `VECTORS` row: it allocates past it, or fails with `IdAllocationFailed` — it never overwrites - `PalaceHandle::embed_health` and `palace_reembed` now carry an `AliasAudit`: key presence alone reported a false all-clear for this class, and `is_healthy()` is now false when any drawer is aliased - an alias audit that could not run is `AliasAudit::Unavailable`, not zeros; `is_healthy()` is false for it, so a failed scan can never be read as a clean palace + - new `PalaceHandle::repair_aliases`: the operator surface for the repair, which had no caller at all. Dry-run by default; a real run frees the whole collision group and then re-audits, and reports `Repaired` only when that verification ran and came back clean. `Partial` and `Unavailable` are distinct outcomes and neither is a success + - `UsearchStore::unalias` now returns `UnaliasOutcome`, carrying the keys it freed but could not parse back into a drawer id instead of dropping them — those drawers would otherwise be missing from the operator's re-embed worklist inside a reported success diff --git a/crates/trusty-common/src/memory_core/retrieval/embed_repair.rs b/crates/trusty-common/src/memory_core/retrieval/embed_repair.rs index 3e5a597b4..edc014e42 100644 --- a/crates/trusty-common/src/memory_core/retrieval/embed_repair.rs +++ b/crates/trusty-common/src/memory_core/retrieval/embed_repair.rs @@ -236,7 +236,244 @@ pub struct VectorBackfillReport { pub alias_audit: AliasAudit, } +/// How an alias repair run should behave. +/// +/// Why: mirrors [`VectorBackfillOptions`] on purpose — the operator learns one +/// convention, and the destructive half is opt-in on both surfaces. This run +/// deletes `VECTOR_KEYS` rows, so seeing the exact id list before anything +/// changes matters more here than it does for a re-embed. +/// What: `dry_run` reports the ids it would free and writes nothing. +/// Test: `repair_aliases_dry_run_names_the_group_and_changes_nothing`. +#[derive(Debug, Clone, Copy)] +pub struct AliasRepairOptions { + pub dry_run: bool, +} + +impl Default for AliasRepairOptions { + fn default() -> Self { + Self { dry_run: true } + } +} + +/// How an alias repair run ended. +/// +/// Why (#5005): the defect being repaired was a success-shaped report over real +/// loss, so the repair must not be able to produce one. `Repaired` is reachable +/// ONLY after a post-repair audit ran and came back clean — every other ending, +/// including "the verification could not run", is its own variant. A caller +/// cannot reach a success by reading a count. +/// What: `Clean` (nothing aliased), `Planned` (dry run), `Repaired` (freed and +/// verified), `Partial` (freed, but verification still finds a problem), and +/// `Unavailable` (an audit could not run, so nothing is known). +/// Test: `repair_aliases_never_reports_success_over_a_partial_repair`, +/// `repair_aliases_refuses_to_run_on_an_unreadable_audit`. +#[derive(Debug, Clone)] +pub enum AliasRepairOutcome { + /// The audit ran and found no collision. Nothing was written. + Clean, + /// Dry run. `freed_ids` is what a real run WOULD free; nothing changed. + Planned, + /// Freed, and the post-repair audit confirms no collision remains. + Repaired, + /// Something was written, but the palace is not provably clean afterwards. + /// Never report this as success. + Partial { + /// Drawers a collision still covers after the repair. + still_aliased: Vec, + /// Ids the pre-repair audit named that this run did not free. + not_freed: Vec, + /// Keys freed that could not be named, so they are missing from the + /// re-embed worklist. + unparsed_keys: Vec, + }, + /// An audit could not run, before or after. Nothing is known about this + /// palace's alias state; treat it as a block, never as a pass. + Unavailable { reason: String }, +} + +impl AliasRepairOutcome { + /// One word for a wire payload or a log line. + pub fn as_str(&self) -> &'static str { + match self { + Self::Clean => "clean", + Self::Planned => "planned", + Self::Repaired => "repaired", + Self::Partial { .. } => "partial", + Self::Unavailable { .. } => "unavailable", + } + } + + /// Whether the palace is provably free of aliasing after this run. + /// + /// `Planned` is false: a dry run repaired nothing. `Partial` and + /// `Unavailable` are false by construction — that is the whole point. + pub fn is_success(&self) -> bool { + matches!(self, Self::Clean | Self::Repaired) + } +} + +/// Outcome of one alias repair run. +/// +/// Why: `freed_ids` is a SET, not a count. #5005 was a count (`missing: 0`) +/// reporting all-clear over four destroyed drawers; a repair that answered +/// "3 repaired" without naming which three would be the same defect one layer +/// up, and the ids are also the operator's re-embed worklist. +/// What: the audit before, the exact ids freed, the audit after (absent on a +/// dry run, which reads nothing twice), and the outcome. +/// Test: `repair_aliases_frees_the_group_and_verifies_it`. +#[derive(Debug, Clone)] +pub struct AliasRepairReport { + pub palace_id: String, + pub dry_run: bool, + /// The alias audit taken before anything was written. + pub before: AliasAudit, + /// Exact drawer ids freed — or, on a dry run, that would be freed. These + /// now have no vector and need a `backfill_missing_vectors` run. + pub freed_ids: Vec, + /// The verification audit. `None` on a dry run and when nothing was + /// aliased, because neither wrote anything to verify. + pub after: Option, + pub outcome: AliasRepairOutcome, +} + +impl AliasRepairReport { + /// Whether the freed drawers still need a re-embed to become findable. + /// + /// Freeing an aliased group turns an invisible drawer into an ordinary + /// missing one; only the backfill makes it retrievable again. + pub fn reembed_required(&self) -> bool { + !self.dry_run && !self.freed_ids.is_empty() + } +} + impl PalaceHandle { + /// Free every drawer caught in a vector-id collision so a re-embed can + /// repair it — the operator surface for [`UsearchStore::unalias`]. + /// + /// Why (#5005): stopping new aliasing does not repair the drawers already + /// destroyed by it, and those are what block #4834. `unalias` existed but + /// had no caller, so an operator had no way to run the repair. This adds + /// the three things a destructive repair owes: a dry run that names what it + /// would touch, a result that names ids rather than counting them, and a + /// verification pass that makes a partial repair impossible to mistake for + /// a complete one. + /// What: audits, and refuses to write on an unreadable audit. A dry run + /// (the default) returns the ids and stops. A real run frees the whole + /// group, then re-audits: `Repaired` requires that second audit to have run + /// AND come back clean AND account for every id the first one named. + /// Idempotent — the second run finds no group and reports `Clean`. + /// + /// The freed drawers are left needing a re-embed on purpose: this call must + /// not depend on an embedder, so it stays runnable on a host with no model + /// and the two halves fail independently. Follow it with + /// [`PalaceHandle::backfill_missing_vectors`]. + /// Test: `repair_aliases_frees_the_group_and_verifies_it`, + /// `repair_aliases_dry_run_names_the_group_and_changes_nothing`, + /// `repair_aliases_never_reports_success_over_a_partial_repair`, + /// `repair_aliases_refuses_to_run_on_an_unreadable_audit`, + /// `repair_aliases_then_reembed_makes_a_lost_drawer_retrievable`. + pub fn repair_aliases(&self, opts: AliasRepairOptions) -> Result { + let before = AliasAudit::from_scan(self.vector_store.alias_audit()); + let mut report = AliasRepairReport { + palace_id: self.id.as_str().to_string(), + dry_run: opts.dry_run, + before: before.clone(), + freed_ids: Vec::new(), + after: None, + outcome: AliasRepairOutcome::Clean, + }; + + // An unreadable audit is not an empty one. Writing here would be + // deleting vector keys with no idea which, or whether any, are aliased. + let Some(aliased) = before.aliased_drawer_ids() else { + let reason = before + .unavailable_reason() + .unwrap_or("alias audit unavailable") + .to_string(); + tracing::error!( + palace = %self.id, + "#5005: refusing to repair aliases — the audit could not run: {reason}" + ); + report.outcome = AliasRepairOutcome::Unavailable { reason }; + return Ok(report); + }; + + let mut expected: Vec = aliased.to_vec(); + expected.sort(); + if expected.is_empty() { + return Ok(report); + } + + if opts.dry_run { + report.freed_ids = expected; + report.outcome = AliasRepairOutcome::Planned; + return Ok(report); + } + + if self.is_read_only() { + anyhow::bail!( + "palace '{}' is read-only: the HTTP daemon holds the write lock — run the \ + alias repair through the daemon (`palace_unalias`) or stop it first", + self.id + ); + } + + let freed = self.vector_store.unalias().context("repair_aliases")?; + report.freed_ids = freed.freed.clone(); + report.freed_ids.sort(); + + // Verification. This is the only path to `Repaired`, so a post-repair + // audit that fails downgrades the run rather than being ignored. + let after = AliasAudit::from_scan(self.vector_store.alias_audit()); + report.after = Some(after.clone()); + let Some(still) = after.aliased_drawer_ids() else { + let reason = after + .unavailable_reason() + .unwrap_or("post-repair alias audit unavailable") + .to_string(); + tracing::error!( + palace = %self.id, freed = report.freed_ids.len(), + "#5005: alias repair wrote, but the verification audit could not run — \ + the palace is NOT confirmed clean: {reason}" + ); + report.outcome = AliasRepairOutcome::Unavailable { reason }; + return Ok(report); + }; + + let freed_set: HashSet = report.freed_ids.iter().copied().collect(); + let not_freed: Vec = expected + .iter() + .copied() + .filter(|id| !freed_set.contains(id)) + .collect(); + let mut still_aliased = still.to_vec(); + still_aliased.sort(); + + if still_aliased.is_empty() && not_freed.is_empty() && freed.unparsed_keys.is_empty() { + tracing::warn!( + palace = %self.id, freed = report.freed_ids.len(), + "#5005: alias repair freed every aliased drawer and verified the palace \ + clean; those drawers now need a re-embed" + ); + report.outcome = AliasRepairOutcome::Repaired; + } else { + tracing::error!( + palace = %self.id, + freed = report.freed_ids.len(), + still_aliased = still_aliased.len(), + not_freed = not_freed.len(), + unparsed = freed.unparsed_keys.len(), + "#5005: alias repair is INCOMPLETE — do not treat this palace as repaired" + ); + report.outcome = AliasRepairOutcome::Partial { + still_aliased, + not_freed, + unparsed_keys: freed.unparsed_keys, + }; + } + Ok(report) + } + /// Vector-coverage snapshot for this palace. /// /// Why: see the module docs — this is the queryable form of "which drawers diff --git a/crates/trusty-common/src/memory_core/retrieval/embed_repair_tests.rs b/crates/trusty-common/src/memory_core/retrieval/embed_repair_tests.rs index 79ca513de..8c24a0275 100644 --- a/crates/trusty-common/src/memory_core/retrieval/embed_repair_tests.rs +++ b/crates/trusty-common/src/memory_core/retrieval/embed_repair_tests.rs @@ -16,7 +16,9 @@ use super::deferred_embed::{ EmbedLoss, RetryPolicy, embed_and_store, embed_store_or_record, record_loss, }; -use super::embed_repair::{AliasAudit, EmbedHealth, VectorBackfillOptions}; +use super::embed_repair::{ + AliasAudit, AliasRepairOptions, AliasRepairOutcome, EmbedHealth, VectorBackfillOptions, +}; use super::embedder::seed_shared_embedder_with_mock; use super::handle::PalaceHandle; use crate::embedder::MockEmbedder; @@ -846,10 +848,14 @@ fn unalias_marks_the_whole_group_for_reembed() { let freed = handle.vector_store.unalias().expect("unalias"); assert_eq!( - freed.len(), + freed.freed.len(), 3, "the whole group is freed, not just the losers" ); + assert!( + freed.unparsed_keys.is_empty(), + "every key in this fixture is a real uuid" + ); let after = handle.embed_health(); assert!(after.alias_audit.is_clean(), "no group survives the repair"); @@ -937,3 +943,387 @@ fn alias_audit_failure_is_never_reported_as_clean() { "a measured-clean audit exposes an EMPTY list — what a zero legitimately means" ); } + +// ── #5005 repair surface: `PalaceHandle::repair_aliases` ───────────────────── + +/// Seed a three-drawer collision with DISTINCT content and return the drawer +/// ids, sorted. +/// +/// Why: `seed_aliased_vector_file` above uses one content string for all three, +/// which is enough to test the audit arithmetic but cannot show that a repaired +/// drawer becomes retrievable BY ITS OWN CONTENT — with identical text every +/// drawer answers every query. The recall proof needs them distinguishable. +/// What: builds three drawers with unrelated content, writes the aliased redb +/// file, registers them on the handle, and returns `(handle, ids)`. +/// Test: used by `repair_aliases_*` and +/// `repair_aliases_then_reembed_makes_a_lost_drawer_retrievable`. +fn seed_distinct_alias_group(dir: &std::path::Path) -> (PalaceHandle, Vec, Vec) { + let room = Uuid::new_v4(); + let contents = [ + "Rust ownership and borrowing rules", + "Postgres autovacuum and index bloat", + "Kubernetes pod eviction under memory pressure", + ]; + let drawers: Vec = contents.iter().map(|c| Drawer::new(room, *c)).collect(); + let mut ids: Vec = drawers.iter().map(|d| d.id).collect(); + ids.sort(); + seed_aliased_vector_file(dir, 4242, &ids); + + let handle = make_handle(dir); + // Keep content aligned with the id it belongs to, so a recall assertion can + // ask "does THIS drawer answer ITS OWN query". + let mut by_id: Vec<(Uuid, String)> = + drawers.iter().map(|d| (d.id, d.content.clone())).collect(); + by_id.sort_by_key(|(id, _)| *id); + for d in drawers { + handle.add_drawer(d); + } + let ordered_content = by_id.into_iter().map(|(_, c)| c).collect(); + (handle, ids, ordered_content) +} + +/// Why (#5005): the repair deletes `VECTOR_KEYS` rows. An operator has to be +/// able to see exactly which drawers that will touch before it happens — the +/// same reason `palace_reembed` defaults to a dry run, with more at stake. +/// A dry run that silently repaired, or that reported a count instead of the +/// ids, would leave the operator unable to check the tool's work. +/// What: runs `repair_aliases` with the DEFAULT options, asserts the outcome is +/// `Planned`, that it names all three drawer ids, and that the palace is +/// byte-for-byte unrepaired afterwards (still 3 keys on 1 id, still 0 missing). +/// Test: itself. Making `Default` return `dry_run: false`, or having the +/// dry-run branch fall through to `unalias`, flips the palace to 3-missing and +/// fails the unchanged-state assertions. +#[test] +fn repair_aliases_dry_run_names_the_group_and_changes_nothing() { + let dir = tempfile::tempdir().unwrap(); + let (handle, ids, _) = seed_distinct_alias_group(dir.path()); + + let report = handle + .repair_aliases(AliasRepairOptions::default()) + .expect("dry run must not error"); + + assert_eq!(report.outcome.as_str(), "planned"); + assert!( + report.dry_run, + "the DEFAULT must be a dry run, not merely available as one" + ); + assert!( + !report.outcome.is_success(), + "a dry run repaired nothing, so it is not a success" + ); + assert_eq!( + report.freed_ids, ids, + "the plan must name every id it would free, not count them" + ); + assert!( + report.after.is_none(), + "a dry run wrote nothing, so there is nothing to verify" + ); + assert!( + !report.reembed_required(), + "nothing was freed, so nothing needs a re-embed" + ); + + // Nothing changed: the collision is still there, untouched. + let health = handle.embed_health(); + assert_eq!( + health.alias_audit.counts(), + Some((3, 1)), + "a dry run must leave all three keys on the one id" + ); + assert!( + health.missing_vector_ids.is_empty(), + "a dry run must not turn the aliased drawers into missing ones" + ); +} + +/// Why (#5005): this is the repair the PR was missing. Stopping new aliasing +/// leaves the already-destroyed drawers destroyed, and `unalias` had zero call +/// sites — an operator could see the damage and had no way to act on it. +/// What: frees a real three-way collision through the operator surface and +/// asserts the four things that make the result trustworthy: the outcome is +/// `Repaired`, the freed set is the exact id set (not a count), the palace +/// verifies clean AFTER the write, and the freed drawers are flagged as needing +/// a re-embed. Then runs it a second time to prove idempotence. +/// Test: itself. Making `repair_aliases` skip the `unalias` call leaves the +/// audit at (3, 1) and the outcome at `planned`; skipping the post-repair audit +/// leaves `after` at `None`. +#[test] +fn repair_aliases_frees_the_group_and_verifies_it() { + let dir = tempfile::tempdir().unwrap(); + let (handle, ids, _) = seed_distinct_alias_group(dir.path()); + + let report = handle + .repair_aliases(AliasRepairOptions { dry_run: false }) + .expect("repair must run"); + + assert_eq!(report.outcome.as_str(), "repaired"); + assert!(report.outcome.is_success()); + assert_eq!( + report.freed_ids, ids, + "every member of the group is freed and named" + ); + assert_eq!( + report.before.aliased_drawer_ids().map(<[Uuid]>::len), + Some(3), + "the before-audit is preserved so the operator can see what was found" + ); + let after = report + .after + .as_ref() + .expect("a real repair must be verified, not assumed"); + assert!( + after.is_clean(), + "`repaired` is only reachable through a clean verification audit" + ); + assert!( + report.reembed_required(), + "the freed drawers have no vector until a backfill runs" + ); + + // The palace state agrees with the report. + let health = handle.embed_health(); + assert!( + health.alias_audit.is_clean(), + "no group survives the repair" + ); + let mut missing = health.missing_vector_ids.clone(); + missing.sort(); + assert_eq!( + missing, ids, + "freed drawers read as ordinary missing, which the backfill repairs" + ); + + // Idempotent: a second run finds no group, frees nothing, writes nothing. + let again = handle + .repair_aliases(AliasRepairOptions { dry_run: false }) + .expect("second run must not error"); + assert_eq!(again.outcome.as_str(), "clean"); + assert!( + again.freed_ids.is_empty(), + "a second run must not double-repair" + ); + assert!( + !again.reembed_required(), + "a no-op run owes the operator no follow-up" + ); + let mut still_missing = handle.embed_health().missing_vector_ids; + still_missing.sort(); + assert_eq!( + still_missing, ids, + "the second run must not have freed or resurrected anything" + ); +} + +/// Why (#5005, and the whole reason this ticket exists): the defect was a +/// success-shaped report over real loss. A repair that wrote something and then +/// could not confirm the palace is clean must NOT report success — otherwise it +/// reproduces the original bug in the tool built to fix it. +/// What: frees a group whose keys are NOT all valid uuids. The redb write +/// succeeds and the post-audit comes back clean, so every count-based check +/// would read "done" — but one freed key has no drawer id, so the operator's +/// re-embed worklist is incomplete. Asserts the outcome is `partial`, is not a +/// success, and names the key it could not resolve. +/// Test: itself. Dropping `unparsed_keys` from the `Repaired` guard — i.e. +/// `still_aliased.is_empty() && not_freed.is_empty()` alone — makes the outcome +/// `repaired` and fails every assertion here. +#[test] +fn repair_aliases_never_reports_success_over_a_partial_repair() { + use crate::memory_core::store::kg_store::{VECTOR_KEYS, VECTORS}; + use redb::Database; + + let dir = tempfile::tempdir().unwrap(); + let room = Uuid::new_v4(); + let drawers: Vec = (0..2).map(|_| Drawer::new(room, "aliased")).collect(); + let mut ids: Vec = drawers.iter().map(|d| d.id).collect(); + ids.sort(); + + // A key that is not a uuid, sharing the id with two real drawers. Written + // at the redb level because no public API can produce it. + { + let db = Database::create(dir.path().join("idx.usearch.redb")).expect("create"); + let encoded = postcard::to_allocvec(&vec![0.05_f32; 384]).expect("encode"); + let wtx = db.begin_write().expect("begin"); + { + let mut vectors = wtx.open_table(VECTORS).expect("vectors"); + let mut keys = wtx.open_table(VECTOR_KEYS).expect("keys"); + vectors.insert(4242_u64, encoded.as_slice()).expect("vec"); + for u in &ids { + keys.insert(u.to_string().as_str(), 4242_u64).expect("key"); + } + keys.insert("not-a-uuid", 4242_u64).expect("bad key"); + } + wtx.commit().expect("commit"); + } + + let handle = make_handle(dir.path()); + for d in drawers { + handle.add_drawer(d); + } + + let report = handle + .repair_aliases(AliasRepairOptions { dry_run: false }) + .expect("repair must run"); + + assert_eq!( + report.outcome.as_str(), + "partial", + "a repair that cannot name everything it freed is not a success" + ); + assert!( + !report.outcome.is_success(), + "`is_success` is what a caller branches on; it must be false here" + ); + match &report.outcome { + AliasRepairOutcome::Partial { unparsed_keys, .. } => assert_eq!( + unparsed_keys, + &["not-a-uuid".to_string()], + "the unnameable key must be surfaced, not swallowed" + ), + other => panic!("expected Partial, got {other:?}"), + } + // The contrast that proves the assertion is about the unnameable key and + // not about leftover aliasing: the palace IS clean afterwards. + assert!( + report.after.as_ref().expect("verification ran").is_clean(), + "the redb write itself succeeded — `partial` here is about the worklist" + ); +} + +/// Why (#5005): the MCP layer branches on `is_success()` to decide whether the +/// palace may be treated as repaired, so an `Unavailable` outcome that answered +/// `true` there would ship the exact defect this ticket exists to remove — a +/// failure reported as a pass. `Unavailable` is what `repair_aliases` returns +/// when the alias scan cannot run, i.e. when nothing at all is known. +/// What: asserts the two outcomes that must NOT read as done — `Unavailable` +/// and `Partial` — are both `is_success() == false` and carry distinct words, +/// against the two that must (`Clean`, `Repaired`). +/// +/// Coverage note: this asserts the CONTRACT, not the I/O failure that triggers +/// it. `UsearchStore::alias_audit` can only fail on a redb read error, and +/// every fixture that breaks that read also breaks `UsearchStore::new` — the +/// store cannot be constructed in the state that would exercise the branch +/// end-to-end. `repair_aliases_never_reports_success_over_a_partial_repair` +/// covers the other non-success ending through the real code path. +/// Test: itself. Making `is_success` `!matches!(self, Self::Planned)` — the +/// natural wrong simplification — passes `Unavailable` and `Partial` and fails +/// here. +#[test] +fn an_unavailable_or_partial_repair_is_never_a_success() { + let unavailable = AliasRepairOutcome::Unavailable { + reason: "redb read failed".to_string(), + }; + assert_eq!(unavailable.as_str(), "unavailable"); + assert!( + !unavailable.is_success(), + "nothing is known about this palace, which is a block and not a pass" + ); + + let partial = AliasRepairOutcome::Partial { + still_aliased: vec![Uuid::new_v4()], + not_freed: Vec::new(), + unparsed_keys: Vec::new(), + }; + assert_eq!(partial.as_str(), "partial"); + assert!( + !partial.is_success(), + "a repair that left a collision behind is not done" + ); + + // The contrast that keeps the assertions above about failure specifically, + // and not about `is_success` being false for everything. + assert!(AliasRepairOutcome::Clean.is_success()); + assert!(AliasRepairOutcome::Repaired.is_success()); + assert!( + !AliasRepairOutcome::Planned.is_success(), + "a dry run repaired nothing" + ); +} + +/// Why (#5005 / #4834): this is the end-to-end claim the repair makes — a +/// drawer that was durable and permanently unretrievable becomes retrievable +/// again. Every other test here asserts on table arithmetic; a test that proves +/// `unalias` is CALLED proves nothing about whether recall works afterwards. +/// This one drives the operator's real sequence: repair, then re-embed, then +/// search, and asserts on what a user would actually experience. +/// What: seeds a three-way collision over drawers with distinct content, finds +/// the members that their OWN content cannot retrieve (the collision collapses +/// the group to one reachable uuid), runs `repair_aliases` then +/// `backfill_missing_vectors`, and asserts every previously-unretrievable +/// drawer now answers its own query. +/// Test: itself. Skipping the `repair_aliases` call leaves the drawers with +/// keys, so the backfill reports 0 missing and the final recall still misses +/// them — the fail-before state this asserts against. +#[tokio::test] +async fn repair_aliases_then_reembed_makes_a_lost_drawer_retrievable() { + seed_shared_embedder_with_mock(); + let dir = tempfile::tempdir().unwrap(); + let (handle, ids, contents) = seed_distinct_alias_group(dir.path()); + let embedder = super::embedder::shared_embedder().await.unwrap(); + + let finds_itself = |id: Uuid, query: &str| { + let handle = &handle; + let embedder = embedder.clone(); + let query = query.to_string(); + async move { + super::layers::retrieve_l2(handle, embedder.as_ref(), &query, None, 10) + .await + .unwrap() + .iter() + .any(|r| r.drawer.id == id) + } + }; + + // Fail-before: the collision collapses the group onto one reachable uuid, + // so at least two drawers cannot be found by their own content. + let mut lost: Vec<(Uuid, String)> = Vec::new(); + for (id, content) in ids.iter().zip(contents.iter()) { + if !finds_itself(*id, content).await { + lost.push((*id, content.clone())); + } + } + assert!( + lost.len() >= 2, + "an aliased group of 3 shares one vector row, so at most one member can \ + be reachable — got {} unretrievable", + lost.len() + ); + // And the health surface calls this palace fully covered, which is #5005. + assert!( + handle.embed_health().missing_vector_ids.is_empty(), + "the false all-clear: every lost drawer still HAS a vector key" + ); + + // Repair, then re-embed — the operator's documented sequence. + let repair = handle + .repair_aliases(AliasRepairOptions { dry_run: false }) + .expect("repair"); + assert_eq!(repair.outcome.as_str(), "repaired"); + assert!(repair.reembed_required(), "the repair says so itself"); + + let backfill = handle + .backfill_missing_vectors(VectorBackfillOptions { + dry_run: false, + limit: None, + retry: RetryPolicy::instant(2), + }) + .await + .expect("backfill"); + assert_eq!( + backfill.repaired, 3, + "all three need and get a fresh vector" + ); + assert!(backfill.still_missing_ids.is_empty()); + + // Pass-after: every previously-unretrievable drawer answers its own query. + for (id, content) in &lost { + assert!( + finds_itself(*id, content).await, + "drawer {id} was durable and unretrievable; after repair + re-embed \ + its own content must find it" + ); + } + // And the palace is healthy on both conditions, not just the count. + let health = handle.embed_health(); + assert!(health.is_healthy(), "clean audit AND no missing drawers"); +} diff --git a/crates/trusty-common/src/memory_core/retrieval/mod.rs b/crates/trusty-common/src/memory_core/retrieval/mod.rs index 12ef6c499..3cb1bde71 100644 --- a/crates/trusty-common/src/memory_core/retrieval/mod.rs +++ b/crates/trusty-common/src/memory_core/retrieval/mod.rs @@ -52,7 +52,10 @@ pub use handle::PalaceHandle; // public because a caller choosing to run a longer policy than the write-path // default is a legitimate operator decision. pub use deferred_embed::RetryPolicy; -pub use embed_repair::{AliasAudit, EmbedHealth, VectorBackfillOptions, VectorBackfillReport}; +pub use embed_repair::{ + AliasAudit, AliasRepairOptions, AliasRepairOutcome, AliasRepairReport, EmbedHealth, + VectorBackfillOptions, VectorBackfillReport, +}; // Recall scoping (ADR-0027 T9) pub use scope::{RecallScope, list_drawers_in_wing, scope_admits}; diff --git a/crates/trusty-common/src/memory_core/store/vector.rs b/crates/trusty-common/src/memory_core/store/vector.rs index 0818ca75c..500a19aff 100644 --- a/crates/trusty-common/src/memory_core/store/vector.rs +++ b/crates/trusty-common/src/memory_core/store/vector.rs @@ -174,6 +174,26 @@ pub struct VectorHit { pub score: f32, } +/// What one `unalias` run freed, including anything it could not name. +/// +/// Why (#5005): the freed ids ARE the operator's worklist — each one is a +/// drawer that now has no vector and needs a re-embed. A key removed from +/// `VECTOR_KEYS` but dropped from the returned list because it would not parse +/// is a drawer nobody knows to repair, reported inside a success. That is the +/// count-based all-clear this ticket exists to remove, one layer down, so the +/// unparseable keys are carried rather than discarded. +/// What: `freed` is the parseable drawer ids; `unparsed_keys` is every raw key +/// that was freed and could not be read back as a `Uuid`. A non-empty +/// `unparsed_keys` means the worklist is incomplete. +/// Test: `repair_aliases_never_reports_success_over_a_partial_repair`. +#[derive(Debug, Clone, Default)] +pub struct UnaliasOutcome { + /// Drawer ids freed by this run — the re-embed worklist. + pub freed: Vec, + /// Keys freed that are not valid uuids, so they have no drawer id. + pub unparsed_keys: Vec, +} + /// Result summary returned by `UsearchStore::compact_orphans`. /// /// Why: CLI / MCP callers need a structured report (not just a count) so they @@ -436,22 +456,33 @@ impl UsearchStore { /// Unmap every drawer caught in an id collision so a re-embed repairs it. /// - /// 🔴 Not wired to any CLI or MCP surface, and never run against a live - /// palace in the PR that added it (#5005). - /// /// Why: see [`HnswStore::unalias`] — the reachable member of a collision /// group is no more trustworthy than the unreachable ones, so the repair /// has to free the whole group. - /// What: delegates to `HnswStore::unalias` and returns the freed drawer - /// ids, which then read as ordinary "missing" to `embed_health`. - /// Test: `unalias_marks_the_whole_group_for_reembed` in - /// `embed_repair_tests`. - pub fn unalias(&self) -> Result> { - let freed = self.inner.unalias().context("unalias: free aliased keys")?; - Ok(freed - .iter() - .filter_map(|s| Uuid::parse_str(s).ok()) - .collect()) + /// What: delegates to `HnswStore::unalias` and splits the freed raw keys + /// into parseable drawer ids and unnameable leftovers. The freed drawers + /// then read as ordinary "missing" to `embed_health`. Callers should route + /// through [`PalaceHandle::repair_aliases`], which adds the dry run and the + /// post-repair verification; this is the raw primitive. + /// Test: `unalias_marks_the_whole_group_for_reembed`, + /// `repair_aliases_never_reports_success_over_a_partial_repair`. + pub fn unalias(&self) -> Result { + let raw = self.inner.unalias().context("unalias: free aliased keys")?; + let mut out = UnaliasOutcome::default(); + for key in raw { + match Uuid::parse_str(&key) { + Ok(u) => out.freed.push(u), + Err(e) => { + tracing::error!( + key = %key, + "#5005: unalias freed a key that is not a uuid, so no drawer id \ + can be reported for it: {e}" + ); + out.unparsed_keys.push(key); + } + } + } + Ok(out) } /// Remove vector entries whose drawer IDs are not in `valid_ids`. diff --git a/crates/trusty-memory/changelog.d/5005-palace-unalias.md b/crates/trusty-memory/changelog.d/5005-palace-unalias.md new file mode 100644 index 000000000..c7ead4775 --- /dev/null +++ b/crates/trusty-memory/changelog.d/5005-palace-unalias.md @@ -0,0 +1,4 @@ +Added +- `palace_unalias`: free drawers whose vector was destroyed by an id collision, so a `palace_reembed` run can make them findable again ([#5005](https://github.com/bobmatnyc/trusty-tools/issues/5005)) + - dry-run by default, like `palace_reembed`. It reports the drawer id SET (`freed_ids`), never a bare count — a count-based all-clear is the defect #5005 is about + - callers branch on `outcome` (`clean` | `planned` | `repaired` | `partial` | `unavailable`) or `success`; `partial` and `unavailable` carry ids and neither is a success. `reembed_required` says outright when a `palace_reembed` run is still owed diff --git a/crates/trusty-memory/src/lib_tests.rs b/crates/trusty-memory/src/lib_tests.rs index 72cef28c8..3e8ac5e68 100644 --- a/crates/trusty-memory/src/lib_tests.rs +++ b/crates/trusty-memory/src/lib_tests.rs @@ -98,8 +98,8 @@ async fn tools_list_returns_all_tools() { // issue #1722 adds `task_add`, `task_list`, `task_complete`; // ADR-0027 T6 (#4805) adds `room_list`, `room_create`, `room_rename`; // ADR-0027 T9 (#4809) adds `wing_list`, `wing_create`, `wing_rename`; - // #4906 adds `palace_reembed`. - assert_eq!(tools.len(), 44); + // #4906 adds `palace_reembed`; #5005 adds `palace_unalias`. + assert_eq!(tools.len(), 45); } #[tokio::test] diff --git a/crates/trusty-memory/src/mcp_service.rs b/crates/trusty-memory/src/mcp_service.rs index 9e11eabd4..a9a282212 100644 --- a/crates/trusty-memory/src/mcp_service.rs +++ b/crates/trusty-memory/src/mcp_service.rs @@ -87,8 +87,8 @@ mod tests { let tools = svc.tools(); assert_eq!( tools.len(), - 44, - "expected 44 memory tools (chat-session + dream-ops + palace_dream + the ADR-0027 room and wing surfaces + #4906 palace_reembed), got {}", + 45, + "expected 45 memory tools (chat-session + dream-ops + palace_dream + the ADR-0027 room and wing surfaces + #4906 palace_reembed + #5005 palace_unalias), got {}", tools.len() ); } @@ -111,8 +111,10 @@ mod tests { // so we must confirm dynamic dispatch resolves correctly here. let svc: Box = Box::new(MemoryMcpService); assert_eq!(svc.name(), "trusty-memory"); - assert_eq!(svc.tools().len(), 44); + assert_eq!(svc.tools().len(), 45); assert_eq!(svc.scopes_for("palace_create"), vec!["memory.write"]); + // #5005: the repair deletes vector keys, so it must classify as a write. + assert_eq!(svc.scopes_for("palace_unalias"), vec!["memory.write"]); assert_eq!(svc.scopes_for("palace_delete"), vec!["memory.write"]); assert_eq!(svc.scopes_for("palace_update"), vec!["memory.write"]); assert_eq!(svc.scopes_for("palace_list"), vec!["memory.read"]); diff --git a/crates/trusty-memory/src/openrpc.rs b/crates/trusty-memory/src/openrpc.rs index 430d8cc91..f3b409d21 100644 --- a/crates/trusty-memory/src/openrpc.rs +++ b/crates/trusty-memory/src/openrpc.rs @@ -78,6 +78,8 @@ pub fn scopes_for_tool(name: &str) -> Vec { | "palace_compact" // #4906: dry-run is read-only but the repair writes vectors. | "palace_reembed" + // #5005: dry-run is read-only but the repair deletes vector keys. + | "palace_unalias" | "kg_assert" | "add_alias" | "remove_prompt_fact" diff --git a/crates/trusty-memory/src/tools/definitions.rs b/crates/trusty-memory/src/tools/definitions.rs index 885e6e5c5..ab41e2d8c 100644 --- a/crates/trusty-memory/src/tools/definitions.rs +++ b/crates/trusty-memory/src/tools/definitions.rs @@ -307,6 +307,18 @@ pub fn tool_definitions_with(has_default: bool) -> Value { "required": palace_compact_required, } }, + { + "name": "palace_unalias", + "description": "#5005: free drawers whose vector was destroyed by an id collision (`palace_reembed` reports these as `aliased`), so a re-embed can repair them. Defaults to a dry run. Branch on `outcome` (clean/planned/repaired/partial/unavailable), never on the id counts — `partial` and `unavailable` are not successes. Run `palace_reembed` afterwards to make the freed drawers findable again.", + "inputSchema": { + "type": "object", + "properties": { + "palace": {"type": "string"}, + "dry_run": {"type": "boolean", "description": "Name the drawer ids that would be freed; delete nothing. Default true."} + }, + "required": palace_compact_required, + } + }, { "name": "add_alias", "description": "Add a short→full alias (e.g. tga → trusty-git-analytics) to the prompt-facts surface. Asserts the alias as a hot KG triple and refreshes the session-init prompt cache.", diff --git a/crates/trusty-memory/src/tools/mod.rs b/crates/trusty-memory/src/tools/mod.rs index da2f2d308..3b565e592 100644 --- a/crates/trusty-memory/src/tools/mod.rs +++ b/crates/trusty-memory/src/tools/mod.rs @@ -78,7 +78,7 @@ use memory_ops::{ }; use palace_ops::{ handle_palace_compact, handle_palace_create, handle_palace_delete, handle_palace_info, - handle_palace_list, handle_palace_reembed, handle_palace_update, + handle_palace_list, handle_palace_reembed, handle_palace_unalias, handle_palace_update, }; use room_ops::{handle_room_create, handle_room_list, handle_room_rename}; use task_ops::{handle_task_add, handle_task_complete, handle_task_list}; @@ -116,6 +116,8 @@ pub async fn dispatch_tool(state: &AppState, name: &str, args: Value) -> Result< "palace_compact" => handle_palace_compact(state, args).await, // #4906: report / repair drawers that have no vector. "palace_reembed" => handle_palace_reembed(state, args).await, + // #5005: free drawers destroyed by a vector-id collision. + "palace_unalias" => handle_palace_unalias(state, args).await, "kg_gaps" => handle_kg_gaps(state, args).await, "memory_recall_all" => handle_memory_recall_all(state, args).await, "get_prompt_context" => handle_get_prompt_context(state, args).await, diff --git a/crates/trusty-memory/src/tools/palace_ops.rs b/crates/trusty-memory/src/tools/palace_ops.rs index b727fa340..b83871c12 100644 --- a/crates/trusty-memory/src/tools/palace_ops.rs +++ b/crates/trusty-memory/src/tools/palace_ops.rs @@ -320,6 +320,79 @@ pub(crate) async fn handle_palace_reembed(state: &AppState, args: Value) -> Resu })) } +/// `palace_unalias` — free drawers destroyed by a vector-id collision so a +/// re-embed can repair them. +/// +/// Why (#5005): the allocator fix stops NEW aliasing and `palace_reembed` now +/// makes existing aliasing visible, but neither repairs it — `unalias` had no +/// caller at all, so an operator could see the damage and not act on it. The +/// three drawers still blocking #4834 need this surface. It runs inside the +/// daemon for the same reason `palace_reembed` does: the daemon holds the +/// palace's writer lock, so a CLI would only get a read-only snapshot. +/// What: `dry_run` (the default) names the exact drawer ids it would free and +/// writes nothing. `dry_run: false` frees the whole collision group, then +/// re-audits — `outcome` is `"repaired"` only when that verification ran and +/// came back clean. Idempotent: a second run reports `"clean"` and frees +/// nothing. The freed drawers still need a `palace_reembed` run to become +/// findable, which `reembed_required` says outright. +/// +/// 🔴 `outcome` is the field to branch on, never `freed_ids.len()`. `"partial"` +/// and `"unavailable"` both carry ids and neither is a success. +/// Test: `dispatch_palace_unalias_dry_run_names_ids_and_writes_nothing`. +pub(crate) async fn handle_palace_unalias(state: &AppState, args: Value) -> Result { + use trusty_common::memory_core::retrieval::{AliasRepairOptions, AliasRepairOutcome}; + let palace = resolve_palace(state, &args, "palace_unalias")?; + let handle = open_palace_handle(state, &palace)?; + // Defaults to a dry run for the same reason `palace_reembed` does, and with + // more at stake: this one deletes vector keys. + let dry_run = args + .get("dry_run") + .and_then(|v| v.as_bool()) + .unwrap_or(true); + let report = + tokio::task::spawn_blocking(move || handle.repair_aliases(AliasRepairOptions { dry_run })) + .await + .context("join palace_unalias")??; + + let ids = |v: &[Uuid]| v.iter().map(|i| i.to_string()).collect::>(); + let (still_aliased_ids, not_freed_ids, unparsed_keys) = match &report.outcome { + AliasRepairOutcome::Partial { + still_aliased, + not_freed, + unparsed_keys, + } => ( + Some(ids(still_aliased)), + Some(ids(not_freed)), + Some(unparsed_keys.clone()), + ), + _ => (None, None, None), + }; + let error = match &report.outcome { + AliasRepairOutcome::Unavailable { reason } => Some(reason.as_str()), + _ => None, + }; + Ok(json!({ + "palace": report.palace_id, + "dry_run": report.dry_run, + // Branch on this. `"clean"` and `"repaired"` are the only successes. + "outcome": report.outcome.as_str(), + "success": report.outcome.is_success(), + // The id SET, never a bare count: #5005 was a count reporting all-clear + // over real loss, and these ids are also the re-embed worklist. + "freed_ids": ids(&report.freed_ids), + "aliased_before_ids": report.before.aliased_drawer_ids().map(ids), + // Present only on a partial repair, which is exactly when a caller must + // not read the run as done. + "still_aliased_ids": still_aliased_ids, + "not_freed_ids": not_freed_ids, + "unparsed_keys": unparsed_keys, + "error": error, + // Freeing a group turns an invisible drawer into an ordinary missing + // one; only `palace_reembed` makes it retrievable again. + "reembed_required": report.reembed_required(), + })) +} + /// One word for how the #5005 alias audit went, for the `palace_reembed` payload. /// /// Why: a caller has to be able to tell "no drawer is aliased" from "the scan diff --git a/crates/trusty-memory/src/tools/tests.rs b/crates/trusty-memory/src/tools/tests.rs index 36223eaf2..62d2a4486 100644 --- a/crates/trusty-memory/src/tools/tests.rs +++ b/crates/trusty-memory/src/tools/tests.rs @@ -102,6 +102,7 @@ fn tool_definitions_drops_palace_required_when_default_set() { ("palace_info", true), ("palace_compact", true), ("palace_reembed", true), + ("palace_unalias", true), ("kg_assert", true), ("kg_query", true), // Issue #664: add_alias and discover_aliases now include `palace` @@ -139,7 +140,8 @@ fn tool_definitions_lists_all_tools() { // #1722) + 3 room tools (room_list, room_create, room_rename, ADR-0027 T6) // + 3 wing tools (wing_list, wing_create, wing_rename, ADR-0027 T9 / #4809) // + 1 repair tool (palace_reembed, #4906) - assert_eq!(tools.len(), 44); + // + 1 alias-repair tool (palace_unalias, #5005) + assert_eq!(tools.len(), 45); let names: Vec<&str> = tools .iter() .filter_map(|t| t.get("name").and_then(|n| n.as_str())) @@ -158,6 +160,7 @@ fn tool_definitions_lists_all_tools() { "palace_info", "palace_compact", "palace_reembed", + "palace_unalias", "kg_assert", "kg_query", "memory_recall_all", @@ -238,6 +241,46 @@ async fn dispatch_palace_reembed_dry_run_reports_counts() { assert!(out["vector_count"].is_number()); } +/// Why (#5005): `unalias` had zero call sites — the repair existed as code an +/// operator could not run. The claim this makes is that it is now reachable +/// through `dispatch_tool`, defaults to a dry run like `palace_reembed`, and +/// reports an id SET rather than a count (the count-based all-clear is the +/// defect the ticket is about). It also runs inside the daemon for the same +/// reason `palace_reembed` does: the daemon holds the writer lock. +/// What: creates a palace and calls `palace_unalias` with no arguments, +/// asserting the default is a dry run, the outcome word is `clean` on a palace +/// with nothing aliased, and the payload carries `freed_ids` as an array. +/// Test: itself. Removing the `palace_unalias` dispatch arm makes this an +/// unknown-tool error; defaulting `dry_run` to false fails the first assertion. +#[tokio::test] +async fn dispatch_palace_unalias_dry_run_names_ids_and_writes_nothing() { + let (state, _tmp) = test_state(); + dispatch_tool(&state, "palace_create", json!({"name": "unalias-test"})) + .await + .expect("palace_create"); + let out = dispatch_tool(&state, "palace_unalias", json!({"palace": "unalias-test"})) + .await + .expect("palace_unalias must be dispatchable"); + assert_eq!(out["dry_run"], true, "must default to a dry run: {out}"); + assert_eq!( + out["outcome"], "clean", + "a palace with no collision is clean, not repaired: {out}" + ); + assert_eq!(out["success"], true); + assert!( + out["freed_ids"] + .as_array() + .expect("freed_ids array") + .is_empty(), + "an id SET, empty here — never a bare count: {out}" + ); + assert_eq!( + out["reembed_required"], false, + "nothing was freed, so nothing is owed" + ); + assert!(out["error"].is_null(), "a clean run has no error: {out}"); +} + /// Why (issue #1714): `force=true` bypasses slug validation with no /// authorization check by default (single-tenant mode, unchanged /// behaviour) — confirm that default mode still lets `force=true` through diff --git a/crates/trusty-memory/src/transport/rpc.rs b/crates/trusty-memory/src/transport/rpc.rs index b433f1f20..bb7beacdf 100644 --- a/crates/trusty-memory/src/transport/rpc.rs +++ b/crates/trusty-memory/src/transport/rpc.rs @@ -205,6 +205,7 @@ const TOOL_METHODS: &[&str] = &[ "palace_info", "palace_list", "palace_reembed", + "palace_unalias", "remove_prompt_fact", ]; From 545d2106509cd98b7abbbd3409566d307771dced Mon Sep 17 00:00:00 2001 From: bobmatnyc Date: Fri, 7 Aug 2026 00:05:24 -0400 Subject: [PATCH 6/8] docs(trusty-common): repoint two Test: citations at the test that survived (#5005) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `repair_aliases_refuses_to_run_on_an_unreadable_audit` was dropped: the branch it targeted is only reachable on a redb read error, and every fixture that breaks that read also breaks `UsearchStore::new`, so the store cannot be built in the state the test needed. `an_unavailable_or_partial_repair_is_never_a_success` covers the contract instead. Two doc pointers still cited the removed name and `check_test_pointers.sh` flagged both. 🤖🤖🤖 Generated with trusty-mpm — https://github.com/bobmatnyc/trusty-tools --- .../trusty-common/src/memory_core/retrieval/embed_repair.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/trusty-common/src/memory_core/retrieval/embed_repair.rs b/crates/trusty-common/src/memory_core/retrieval/embed_repair.rs index edc014e42..4cd5bb0fc 100644 --- a/crates/trusty-common/src/memory_core/retrieval/embed_repair.rs +++ b/crates/trusty-common/src/memory_core/retrieval/embed_repair.rs @@ -266,7 +266,7 @@ impl Default for AliasRepairOptions { /// verified), `Partial` (freed, but verification still finds a problem), and /// `Unavailable` (an audit could not run, so nothing is known). /// Test: `repair_aliases_never_reports_success_over_a_partial_repair`, -/// `repair_aliases_refuses_to_run_on_an_unreadable_audit`. +/// `an_unavailable_or_partial_repair_is_never_a_success`. #[derive(Debug, Clone)] pub enum AliasRepairOutcome { /// The audit ran and found no collision. Nothing was written. @@ -370,7 +370,7 @@ impl PalaceHandle { /// Test: `repair_aliases_frees_the_group_and_verifies_it`, /// `repair_aliases_dry_run_names_the_group_and_changes_nothing`, /// `repair_aliases_never_reports_success_over_a_partial_repair`, - /// `repair_aliases_refuses_to_run_on_an_unreadable_audit`, + /// `an_unavailable_or_partial_repair_is_never_a_success`, /// `repair_aliases_then_reembed_makes_a_lost_drawer_retrievable`. pub fn repair_aliases(&self, opts: AliasRepairOptions) -> Result { let before = AliasAudit::from_scan(self.vector_store.alias_audit()); From 918493af2cd098084c793c3aeec3b51ad45652f1 Mon Sep 17 00:00:00 2001 From: bobmatnyc Date: Fri, 7 Aug 2026 00:31:55 -0400 Subject: [PATCH 7/8] fix(trusty-common): close the alias-audit fail-open the detector still had (#5005) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review HIGH. The `filter_map(…ok())` shape fixed in `UsearchStore::unalias` survived one layer up, in `alias_audit` — the detector rather than the repair. A collision group whose keys are not uuids shrank to nothing, and `is_clean()` tested only that id list. Reproduced before fixing: raw alias_audit -> key_rows=2 distinct=1 ids=[] is_clean=true is_healthy=true repair_aliases(dry_run:false) outcome=clean is_success=true freed_ids=[] after "repair" -> key_rows=2 distinct=1 <- collision untouched Two rows on one vector_id is a real collision, reported as clean and left in place — this PR's own defect, on the field it tells callers to branch on, with #4834's deletion gate as the caller. `alias_audit` now carries the unnameable keys instead of dropping them, and `is_clean()` consults `key_rows` vs `distinct_vector_ids`. Those counts come straight off `VECTOR_KEYS`; no parse can shrink them, which is what makes them the signal that cannot be fooled. `repair_aliases` gates on `is_clean()` rather than `expected.is_empty()`, so an all-unnameable group falls through to `unalias` and ends as `Partial` — freed, with the worklist honestly incomplete. Also extracts `classify_repair`, the post-repair decision, as a pure function over `(&AliasAudit, &UnaliasOutcome, &[Uuid])`. That branch fires after keys are already deleted, so "wrote, then could not verify" is the ending most worth proving, and taking the audit as a parameter makes it reachable without a fault-injection seam. No indirection added to the production path: the function tested is the function called. 🤖🤖🤖 Generated with trusty-mpm — https://github.com/bobmatnyc/trusty-tools --- .../changelog.d/5005-hnsw-id-aliasing.md | 1 + .../src/memory_core/retrieval/embed_repair.rs | 169 ++++++++++----- .../retrieval/embed_repair_tests.rs | 192 ++++++++++++++++++ .../src/memory_core/store/vector.rs | 75 +++++-- .../changelog.d/5005-palace-unalias.md | 1 + crates/trusty-memory/src/tools/palace_ops.rs | 6 + 6 files changed, 376 insertions(+), 68 deletions(-) diff --git a/crates/trusty-common/changelog.d/5005-hnsw-id-aliasing.md b/crates/trusty-common/changelog.d/5005-hnsw-id-aliasing.md index 49fc0a504..9f2813234 100644 --- a/crates/trusty-common/changelog.d/5005-hnsw-id-aliasing.md +++ b/crates/trusty-common/changelog.d/5005-hnsw-id-aliasing.md @@ -7,3 +7,4 @@ Fixed - an alias audit that could not run is `AliasAudit::Unavailable`, not zeros; `is_healthy()` is false for it, so a failed scan can never be read as a clean palace - new `PalaceHandle::repair_aliases`: the operator surface for the repair, which had no caller at all. Dry-run by default; a real run frees the whole collision group and then re-audits, and reports `Repaired` only when that verification ran and came back clean. `Partial` and `Unavailable` are distinct outcomes and neither is a success - `UsearchStore::unalias` now returns `UnaliasOutcome`, carrying the keys it freed but could not parse back into a drawer id instead of dropping them — those drawers would otherwise be missing from the operator's re-embed worklist inside a reported success + - `alias_audit` no longer drops collision-group keys that are not uuids, and `AliasAudit::is_clean` now consults `key_rows` vs `distinct_vector_ids` rather than the id list alone. Two `VECTOR_KEYS` rows on one `vector_id` with non-uuid keys previously reported `is_clean() == true`, `is_healthy() == true`, and a `clean` repair while leaving the collision in place — the counts come straight off the table and no parse can shrink them, so they are the signal that cannot be fooled diff --git a/crates/trusty-common/src/memory_core/retrieval/embed_repair.rs b/crates/trusty-common/src/memory_core/retrieval/embed_repair.rs index 4cd5bb0fc..9b1333041 100644 --- a/crates/trusty-common/src/memory_core/retrieval/embed_repair.rs +++ b/crates/trusty-common/src/memory_core/retrieval/embed_repair.rs @@ -28,6 +28,7 @@ use super::deferred_embed::{RetryPolicy, embed_and_store}; use super::embedder::{shared_embedder, shared_embedder_initialized}; use super::handle::PalaceHandle; use crate::memory_core::store::embed_ledger::{self, EmbedFailure}; +use crate::memory_core::store::vector::{AliasScan, UnaliasOutcome}; use anyhow::{Context, Result}; use std::collections::HashSet; use uuid::Uuid; @@ -54,8 +55,12 @@ pub enum AliasAudit { /// Drawers whose vector was overwritten by another drawer's. These have /// a key, so they are NOT in `missing_vector_ids` — that is precisely /// why the count gap missed them — but their content is embedded - /// nowhere. + /// nowhere. May be SHORT of the real group: a key that is not a uuid + /// names no drawer. Never read its emptiness as "no collision" — that + /// is what `key_rows` vs `distinct_vector_ids` is for. aliased_drawer_ids: Vec, + /// Keys in a collision group that could not be parsed into a drawer id. + unnameable_keys: Vec, }, /// The audit could not run. Nothing is known about aliasing in this palace. Unavailable { @@ -75,12 +80,13 @@ impl AliasAudit { /// What: `Ok` → `Measured`; `Err` → `Unavailable` carrying the rendered /// error. Never returns zeros for a failure. /// Test: `alias_audit_failure_is_never_reported_as_clean`. - pub fn from_scan(scan: anyhow::Result<(usize, usize, Vec)>) -> Self { + pub fn from_scan(scan: anyhow::Result) -> Self { match scan { - Ok((key_rows, distinct_vector_ids, aliased_drawer_ids)) => Self::Measured { - key_rows, - distinct_vector_ids, - aliased_drawer_ids, + Ok(s) => Self::Measured { + key_rows: s.key_rows, + distinct_vector_ids: s.distinct_vector_ids, + aliased_drawer_ids: s.aliased_drawer_ids, + unnameable_keys: s.unnameable_keys, }, Err(e) => Self::Unavailable { reason: format!("{e:#}"), @@ -88,11 +94,39 @@ impl AliasAudit { } } - /// Whether the audit ran AND found no drawer sharing an id. + /// Whether the audit ran AND no drawer shares a vector id. /// - /// `Unavailable` is false: an unread palace is not a clean one. + /// Why: this used to test `aliased_drawer_ids.is_empty()` alone, which a + /// collision whose keys are not uuids satisfies while the collision is + /// still there — the id list shrinks, the table does not. `key_rows` vs + /// `distinct_vector_ids` comes straight off `VECTOR_KEYS` and no parse can + /// affect it, so it is the signal that cannot be fooled. Both are checked: + /// if they ever disagree the answer is "not clean", which fails closed. + /// `Unavailable` is false — an unread palace is not a clean one. + /// Test: `a_collision_whose_keys_do_not_parse_is_never_clean`. pub fn is_clean(&self) -> bool { - matches!(self, Self::Measured { aliased_drawer_ids, .. } if aliased_drawer_ids.is_empty()) + matches!( + self, + Self::Measured { + key_rows, + distinct_vector_ids, + aliased_drawer_ids, + unnameable_keys, + } if aliased_drawer_ids.is_empty() + && unnameable_keys.is_empty() + && key_rows == distinct_vector_ids + ) + } + + /// Keys in a collision group that name no drawer, or `None` when the audit + /// did not run. Non-empty means [`Self::aliased_drawer_ids`] is short. + pub fn unnameable_keys(&self) -> Option<&[String]> { + match self { + Self::Measured { + unnameable_keys, .. + } => Some(unnameable_keys), + Self::Unavailable { .. } => None, + } } /// Drawers caught in a collision, or `None` when the audit did not run. @@ -330,6 +364,9 @@ pub struct AliasRepairReport { /// Exact drawer ids freed — or, on a dry run, that would be freed. These /// now have no vector and need a `backfill_missing_vectors` run. pub freed_ids: Vec, + /// Keys the pre-repair audit found in a collision group but could not name. + /// Non-empty means `freed_ids` cannot be the whole story. + pub unnameable_keys: Vec, /// The verification audit. `None` on a dry run and when nothing was /// aliased, because neither wrote anything to verify. pub after: Option, @@ -346,6 +383,61 @@ impl AliasRepairReport { } } +/// Decide how a repair run ended, from the post-repair audit alone. +/// +/// Why (#5005): this is the guard that makes a partial repair impossible to +/// report as a complete one, and it runs AFTER keys have already been deleted — +/// "wrote, then could not verify" is a worse state than "refused to write", so +/// it is the branch most worth testing. Pulling it out of `repair_aliases` +/// makes it reachable with a hand-built `after` value without a fault-injection +/// seam, and without adding any indirection to the production path: the +/// function tested IS the function called. +/// What: `Unavailable` when the verification audit could not run — that is not +/// a success, even though the write itself succeeded. Otherwise `Repaired` only +/// when nothing is still aliased, every id `expected` named was freed, and +/// every freed key could be named; anything else is `Partial` carrying all +/// three shortfalls. +/// Test: `classify_refuses_to_call_an_unverified_write_repaired`, +/// `classify_reports_partial_when_the_verification_still_finds_a_collision`, +/// and end-to-end through `repair_aliases_frees_the_group_and_verifies_it`. +pub(crate) fn classify_repair( + after: &AliasAudit, + freed: &UnaliasOutcome, + expected: &[Uuid], +) -> AliasRepairOutcome { + let Some(still) = after.aliased_drawer_ids() else { + return AliasRepairOutcome::Unavailable { + reason: after + .unavailable_reason() + .unwrap_or("post-repair alias audit unavailable") + .to_string(), + }; + }; + let freed_set: HashSet = freed.freed.iter().copied().collect(); + let not_freed: Vec = expected + .iter() + .copied() + .filter(|id| !freed_set.contains(id)) + .collect(); + let mut still_aliased = still.to_vec(); + still_aliased.sort(); + // A post-repair audit that is measured but NOT clean counts as still + // aliased even when it can name nobody — same arithmetic-over-ids rule as + // `is_clean`, so an all-unnameable group cannot verify as repaired. + if still_aliased.is_empty() + && not_freed.is_empty() + && freed.unparsed_keys.is_empty() + && after.is_clean() + { + return AliasRepairOutcome::Repaired; + } + AliasRepairOutcome::Partial { + still_aliased, + not_freed, + unparsed_keys: freed.unparsed_keys.clone(), + } +} + impl PalaceHandle { /// Free every drawer caught in a vector-id collision so a re-embed can /// repair it — the operator surface for [`UsearchStore::unalias`]. @@ -379,6 +471,7 @@ impl PalaceHandle { dry_run: opts.dry_run, before: before.clone(), freed_ids: Vec::new(), + unnameable_keys: before.unnameable_keys().unwrap_or_default().to_vec(), after: None, outcome: AliasRepairOutcome::Clean, }; @@ -400,7 +493,14 @@ impl PalaceHandle { let mut expected: Vec = aliased.to_vec(); expected.sort(); - if expected.is_empty() { + // Gate on the audit, NOT on `expected.is_empty()`. A collision whose + // keys are not uuids names no drawer, so the id list is empty while the + // collision is still in the table — returning `Clean` there reported a + // real collision as repaired. `is_clean()` consults the row-vs-distinct + // arithmetic, which no parse can shrink, so this now falls through to + // `unalias` and ends as `Partial`: the group IS freed, and the worklist + // genuinely cannot be named. + if before.is_clean() { return Ok(report); } @@ -422,54 +522,19 @@ impl PalaceHandle { report.freed_ids = freed.freed.clone(); report.freed_ids.sort(); - // Verification. This is the only path to `Repaired`, so a post-repair - // audit that fails downgrades the run rather than being ignored. let after = AliasAudit::from_scan(self.vector_store.alias_audit()); report.after = Some(after.clone()); - let Some(still) = after.aliased_drawer_ids() else { - let reason = after - .unavailable_reason() - .unwrap_or("post-repair alias audit unavailable") - .to_string(); - tracing::error!( - palace = %self.id, freed = report.freed_ids.len(), - "#5005: alias repair wrote, but the verification audit could not run — \ - the palace is NOT confirmed clean: {reason}" - ); - report.outcome = AliasRepairOutcome::Unavailable { reason }; - return Ok(report); - }; - - let freed_set: HashSet = report.freed_ids.iter().copied().collect(); - let not_freed: Vec = expected - .iter() - .copied() - .filter(|id| !freed_set.contains(id)) - .collect(); - let mut still_aliased = still.to_vec(); - still_aliased.sort(); - - if still_aliased.is_empty() && not_freed.is_empty() && freed.unparsed_keys.is_empty() { - tracing::warn!( + report.outcome = classify_repair(&after, &freed, &expected); + match &report.outcome { + AliasRepairOutcome::Repaired => tracing::warn!( palace = %self.id, freed = report.freed_ids.len(), "#5005: alias repair freed every aliased drawer and verified the palace \ clean; those drawers now need a re-embed" - ); - report.outcome = AliasRepairOutcome::Repaired; - } else { - tracing::error!( - palace = %self.id, - freed = report.freed_ids.len(), - still_aliased = still_aliased.len(), - not_freed = not_freed.len(), - unparsed = freed.unparsed_keys.len(), + ), + other => tracing::error!( + palace = %self.id, freed = report.freed_ids.len(), outcome = other.as_str(), "#5005: alias repair is INCOMPLETE — do not treat this palace as repaired" - ); - report.outcome = AliasRepairOutcome::Partial { - still_aliased, - not_freed, - unparsed_keys: freed.unparsed_keys, - }; + ), } Ok(report) } diff --git a/crates/trusty-common/src/memory_core/retrieval/embed_repair_tests.rs b/crates/trusty-common/src/memory_core/retrieval/embed_repair_tests.rs index 8c24a0275..1219c8474 100644 --- a/crates/trusty-common/src/memory_core/retrieval/embed_repair_tests.rs +++ b/crates/trusty-common/src/memory_core/retrieval/embed_repair_tests.rs @@ -933,6 +933,7 @@ fn alias_audit_failure_is_never_reported_as_clean() { key_rows: 3, distinct_vector_ids: 3, aliased_drawer_ids: Vec::new(), + unnameable_keys: Vec::new(), }, ..health }; @@ -1327,3 +1328,194 @@ async fn repair_aliases_then_reembed_makes_a_lost_drawer_retrievable() { let health = handle.embed_health(); assert!(health.is_healthy(), "clean audit AND no missing drawers"); } + +/// Why (#5005, review HIGH): the fail-open shape fixed in `UsearchStore::unalias` +/// survived one layer up, in the DETECTOR. `alias_audit` built its id list with +/// `filter_map(…ok())`, so a collision group whose keys are not uuids shrank to +/// nothing — and `is_clean()` tested only that list. A palace with two +/// `VECTOR_KEYS` rows on one `vector_id` therefore reported `is_clean() = true`, +/// `is_healthy() = true`, and `repair_aliases` returned `clean` / `is_success` +/// while leaving the collision exactly where it was. That is the defect this PR +/// exists to fix, reproduced inside the tool built to fix it, on the field the +/// PR tells callers (including #4834's deletion gate) to branch on. +/// What: builds that palace at the redb level and asserts the whole chain +/// refuses it — the audit is not clean, health is not healthy, the counts show +/// the shortfall, and the repair ends `partial` rather than `clean`, having +/// actually freed the group. +/// Test: itself. Reverting `is_clean` to `aliased_drawer_ids.is_empty()` alone, +/// or the `repair_aliases` gate to `expected.is_empty()`, makes the outcome +/// `clean` and fails here. +#[test] +fn a_collision_whose_keys_do_not_parse_is_never_clean() { + use crate::memory_core::store::kg_store::{VECTOR_KEYS, VECTORS}; + use redb::Database; + + let dir = tempfile::tempdir().unwrap(); + { + let db = Database::create(dir.path().join("idx.usearch.redb")).expect("create"); + let encoded = postcard::to_allocvec(&vec![0.05_f32; 384]).expect("encode"); + let wtx = db.begin_write().expect("begin"); + { + let mut vectors = wtx.open_table(VECTORS).expect("vectors"); + let mut keys = wtx.open_table(VECTOR_KEYS).expect("keys"); + vectors.insert(4242_u64, encoded.as_slice()).expect("vec"); + // A genuine collision: two rows, one id. Neither key names a drawer. + keys.insert("not-a-uuid-one", 4242_u64).expect("k1"); + keys.insert("not-a-uuid-two", 4242_u64).expect("k2"); + } + wtx.commit().expect("commit"); + } + let handle = make_handle(dir.path()); + + let health = handle.embed_health(); + assert_eq!( + health.alias_audit.counts(), + Some((2, 1)), + "two key rows on one id — the arithmetic no parse can shrink" + ); + assert_eq!( + health.alias_audit.aliased_drawer_ids(), + Some(&[][..]), + "the id list IS empty here — which is exactly why it must not be the signal" + ); + assert!( + !health.alias_audit.is_clean(), + "a collision the audit cannot name is still a collision" + ); + assert!(!health.is_healthy(), "and the palace is not healthy"); + assert_eq!( + health.alias_audit.unnameable_keys().map(<[String]>::len), + Some(2), + "the keys must be carried, not dropped" + ); + + let report = handle + .repair_aliases(AliasRepairOptions { dry_run: false }) + .expect("repair must run"); + assert_eq!( + report.outcome.as_str(), + "partial", + "freed, but the worklist cannot be named — never `clean`" + ); + assert!( + !report.outcome.is_success(), + "this is the field #4834's deletion gate branches on" + ); + assert_eq!( + report.unnameable_keys.len(), + 2, + "the operator needs the keys the run could not turn into drawer ids" + ); + + // The repair did real work: the collision is gone from the table, which is + // what separates this from the pre-fix behaviour of reporting clean and + // doing nothing. + let after = handle.vector_store.alias_audit().expect("post-repair scan"); + assert_eq!( + (after.key_rows, after.distinct_vector_ids), + (0, 0), + "the whole group is freed even though it could not be named" + ); +} + +/// Why (#5005, review MEDIUM): `classify_repair` runs AFTER keys have been +/// deleted. "Wrote, then could not verify" is a worse state than "refused to +/// write", so it is the branch most worth proving — and unlike the pre-repair +/// audit, it is reachable, because the function takes the audit as a parameter +/// instead of building its own. This is the real production classifier, not a +/// reimplementation of it. +/// What: hands it an `Unavailable` post-repair audit after a fully successful +/// free, and asserts it refuses `Repaired` — the freed ids are irrelevant when +/// nothing verified them. +/// Test: itself. Making the `Unavailable` arm fall through to the `Repaired` +/// check reports success over an unverified destructive write. +#[test] +fn classify_refuses_to_call_an_unverified_write_repaired() { + use crate::memory_core::store::vector::UnaliasOutcome; + let id = Uuid::new_v4(); + let freed = UnaliasOutcome { + freed: vec![id], + unparsed_keys: Vec::new(), + }; + let after = AliasAudit::from_scan(Err(anyhow::anyhow!("redb read failed mid-verify"))); + + let outcome = super::embed_repair::classify_repair(&after, &freed, &[id]); + + assert_eq!(outcome.as_str(), "unavailable"); + assert!( + !outcome.is_success(), + "the write landed but nothing confirmed it; that is not a repaired palace" + ); + + // Contrast: the identical inputs with a clean verification DO give + // `Repaired`, so the assertion above is about the unverified audit and not + // about `classify_repair` refusing everything. + let clean = AliasAudit::Measured { + key_rows: 1, + distinct_vector_ids: 1, + aliased_drawer_ids: Vec::new(), + unnameable_keys: Vec::new(), + }; + assert_eq!( + super::embed_repair::classify_repair(&clean, &freed, &[id]).as_str(), + "repaired" + ); +} + +/// Why (#5005): the other two ways a repair can be incomplete — the palace +/// still has a collision, or the run did not free something the pre-repair +/// audit named. Both must land on `Partial` with the shortfall named, because +/// the ids are what the operator acts on. +/// What: drives `classify_repair` over a verification audit that still reports +/// a collision, and separately over one that is clean but where an expected id +/// was never freed. +/// Test: itself. Dropping the `not_freed` term from the `Repaired` guard makes +/// the second case report `repaired`. +#[test] +fn classify_reports_partial_when_the_verification_still_finds_a_collision() { + use crate::memory_core::store::vector::UnaliasOutcome; + let a = Uuid::new_v4(); + let b = Uuid::new_v4(); + + // Case 1: the post-repair audit still sees a collision. + let freed = UnaliasOutcome { + freed: vec![a, b], + unparsed_keys: Vec::new(), + }; + let still_dirty = AliasAudit::Measured { + key_rows: 2, + distinct_vector_ids: 1, + aliased_drawer_ids: vec![a], + unnameable_keys: Vec::new(), + }; + let outcome = super::embed_repair::classify_repair(&still_dirty, &freed, &[a, b]); + assert_eq!(outcome.as_str(), "partial"); + match &outcome { + AliasRepairOutcome::Partial { still_aliased, .. } => { + assert_eq!(still_aliased, &[a], "the survivor must be named") + } + other => panic!("expected Partial, got {other:?}"), + } + + // Case 2: verification is clean, but the run never freed `b`. + let short = UnaliasOutcome { + freed: vec![a], + unparsed_keys: Vec::new(), + }; + let clean = AliasAudit::Measured { + key_rows: 1, + distinct_vector_ids: 1, + aliased_drawer_ids: Vec::new(), + unnameable_keys: Vec::new(), + }; + let outcome = super::embed_repair::classify_repair(&clean, &short, &[a, b]); + assert_eq!( + outcome.as_str(), + "partial", + "a clean audit does not excuse an id the run was supposed to free" + ); + match &outcome { + AliasRepairOutcome::Partial { not_freed, .. } => assert_eq!(not_freed, &[b]), + other => panic!("expected Partial, got {other:?}"), + } +} diff --git a/crates/trusty-common/src/memory_core/store/vector.rs b/crates/trusty-common/src/memory_core/store/vector.rs index 500a19aff..6a3c471cd 100644 --- a/crates/trusty-common/src/memory_core/store/vector.rs +++ b/crates/trusty-common/src/memory_core/store/vector.rs @@ -174,6 +174,32 @@ pub struct VectorHit { pub score: f32, } +/// One raw alias scan of a palace's `VECTOR_KEYS` table. +/// +/// Why (#5005): the counts and the id list answer different questions, and only +/// the counts are trustworthy. `key_rows` and `distinct_vector_ids` come +/// straight off the table — no parse, no filter, nothing that can shrink them — +/// so their difference is the number of drawers whose vector belongs to someone +/// else. The id list can be incomplete, because a key that is not a uuid names +/// no drawer. Reading "no ids" as "no collision" is what let a real collision +/// report clean. +/// What: the two authoritative counts, the drawer ids in collision groups, and +/// the keys in those groups that could not be parsed into one. +/// Test: `a_collision_whose_keys_do_not_parse_is_never_clean`. +#[derive(Debug, Clone, Default)] +pub struct AliasScan { + /// Rows in `VECTOR_KEYS` — one per drawer with a vector. + pub key_rows: usize, + /// Distinct vector ids those rows point at. Below `key_rows` exactly when + /// drawers share an id. + pub distinct_vector_ids: usize, + /// Drawers caught in a collision group. + pub aliased_drawer_ids: Vec, + /// Keys in a collision group that are not valid uuids, so no drawer id can + /// be reported for them. Non-empty means `aliased_drawer_ids` is short. + pub unnameable_keys: Vec, +} + /// What one `unalias` run freed, including anything it could not name. /// /// Why (#5005): the freed ids ARE the operator's worklist — each one is a @@ -430,28 +456,45 @@ impl UsearchStore { /// test — cannot see an id collision, so a palace with four unretrievable /// drawers reported a clean bill of health. This is the comparison that /// does see it. - /// What: delegates to `HnswStore::audit_aliases` and parses the uuids back - /// into `Uuid`s, dropping (and logging) any row that will not parse so one - /// bad key cannot hide the rest. Returns the two counts alongside the ids. - /// Test: `alias_audit_surfaces_a_collision` in `embed_repair_tests`. - pub fn alias_audit(&self) -> Result<(usize, usize, Vec)> { + /// + /// This used to build its id list with `filter_map(…ok())`, which was the + /// same fail-open shape as the one fixed in [`UsearchStore::unalias`], one + /// layer earlier and in the detector rather than the repair: a collision + /// group whose keys did not parse shrank to nothing, so `is_clean()` + /// answered true over a real collision and `repair_aliases` returned + /// `Clean` without touching it. The keys are carried now, and + /// [`AliasAudit::is_clean`] consults the row-vs-distinct arithmetic rather + /// than the id list alone. + /// What: delegates to `HnswStore::audit_aliases` and splits each collision + /// group's keys into drawer ids and unnameable leftovers, alongside the two + /// counts. The counts come straight from the table and no parse can affect + /// them, which is why they are the authoritative signal. + /// Test: `alias_audit_surfaces_a_collision`, + /// `a_collision_whose_keys_do_not_parse_is_never_clean`. + pub fn alias_audit(&self) -> Result { let audit = self .inner .audit_aliases() .context("alias_audit: scan vector keys")?; - let ids = audit - .aliased - .iter() - .flat_map(|(_, uuids)| uuids.iter()) - .filter_map(|s| match Uuid::parse_str(s) { - Ok(u) => Some(u), + let mut scan = AliasScan { + key_rows: audit.key_rows, + distinct_vector_ids: audit.distinct_vector_ids, + ..Default::default() + }; + for key in audit.aliased.iter().flat_map(|(_, uuids)| uuids.iter()) { + match Uuid::parse_str(key) { + Ok(u) => scan.aliased_drawer_ids.push(u), Err(e) => { - tracing::warn!(key = %s, "alias_audit: skipping unparseable uuid: {e}"); - None + tracing::error!( + key = %key, + "#5005: a key in a collision group is not a uuid, so the drawer it \ + covers cannot be named: {e}" + ); + scan.unnameable_keys.push(key.clone()); } - }) - .collect(); - Ok((audit.key_rows, audit.distinct_vector_ids, ids)) + } + } + Ok(scan) } /// Unmap every drawer caught in an id collision so a re-embed repairs it. diff --git a/crates/trusty-memory/changelog.d/5005-palace-unalias.md b/crates/trusty-memory/changelog.d/5005-palace-unalias.md index c7ead4775..0a6888498 100644 --- a/crates/trusty-memory/changelog.d/5005-palace-unalias.md +++ b/crates/trusty-memory/changelog.d/5005-palace-unalias.md @@ -2,3 +2,4 @@ Added - `palace_unalias`: free drawers whose vector was destroyed by an id collision, so a `palace_reembed` run can make them findable again ([#5005](https://github.com/bobmatnyc/trusty-tools/issues/5005)) - dry-run by default, like `palace_reembed`. It reports the drawer id SET (`freed_ids`), never a bare count — a count-based all-clear is the defect #5005 is about - callers branch on `outcome` (`clean` | `planned` | `repaired` | `partial` | `unavailable`) or `success`; `partial` and `unavailable` carry ids and neither is a success. `reembed_required` says outright when a `palace_reembed` run is still owed + - reports `unnameable_keys`: keys in a collision group that name no drawer, so `aliased_before_ids` can be empty over a real collision. Branch on `outcome`, never on the id counts diff --git a/crates/trusty-memory/src/tools/palace_ops.rs b/crates/trusty-memory/src/tools/palace_ops.rs index b83871c12..af5b84c0c 100644 --- a/crates/trusty-memory/src/tools/palace_ops.rs +++ b/crates/trusty-memory/src/tools/palace_ops.rs @@ -381,6 +381,12 @@ pub(crate) async fn handle_palace_unalias(state: &AppState, args: Value) -> Resu // over real loss, and these ids are also the re-embed worklist. "freed_ids": ids(&report.freed_ids), "aliased_before_ids": report.before.aliased_drawer_ids().map(ids), + // #5005 review HIGH: a collision group whose keys are not uuids names + // no drawer, so `aliased_before_ids` can be EMPTY over a real + // collision. Non-empty here means that id list is short — read + // `vector_key_rows` vs `distinct_vector_ids` off `palace_reembed`, and + // branch on `outcome`, never on the id counts. + "unnameable_keys": report.unnameable_keys.clone(), // Present only on a partial repair, which is exactly when a caller must // not read the run as done. "still_aliased_ids": still_aliased_ids, From 1c409c0066d9eab875ddbc75aaa49672bf6b2523 Mon Sep 17 00:00:00 2001 From: bobmatnyc Date: Fri, 7 Aug 2026 01:02:33 -0400 Subject: [PATCH 8/8] fix(trusty-memory): tell MCP callers about the alias guard, and prove the write path (#5005) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 2, three MEDIUMs, fixed in this PR. 1. `palace_reembed`'s tool description omitted the guard the payload gained. The text an MCP caller reads still promised only "drawers that have no vector", so a correct guard nobody is told to reach had moved the capstone shape from the code into the tool contract — with #4834's deletion gate as the reader. It now says `missing: 0` is not a complete account of what is retrievable, and to act only on `alias_audit.is_clean`. 2. The daemon success path had never produced a non-zero result: every `dispatch_tool` test ran against an empty palace, so `outcome: "repaired"` was proven at the store layer and assumed through the tool — including the `is_read_only()` routing a dry run skips. Adds a test that seeds two uuids onto one `vector_id` in the palace's own `index.usearch.redb` and drives `dry_run: false` through `dispatch_tool`, asserting both uuids in `freed_ids`, `reembed_required: true`, and `clean` on a second call. Forcing `dry_run = true` in the handler fails it (`outcome` "planned", `success` false), so the assertion is load-bearing. 3. Stale 🔴 block on `HnswStore::unalias` still said it was wired to no MCP surface; this PR wires it. Corrected to name the `palace_unalias` path and keep only the claim that is still true — no live-palace run. `postcard` joins trusty-memory's dev-dependencies so the seeded vector encodes at the real 384 dimensions; HnswStore rejects the file otherwise. One line in Cargo.lock, no refresh. 🤖🤖🤖 Generated with trusty-mpm — https://github.com/bobmatnyc/trusty-tools --- Cargo.lock | 1 + .../src/memory_core/store/hnsw_store.rs | 6 +- crates/trusty-memory/Cargo.toml | 3 + .../changelog.d/5005-palace-unalias.md | 1 + crates/trusty-memory/src/tools/definitions.rs | 2 +- crates/trusty-memory/src/tools/palace_ops.rs | 4 +- crates/trusty-memory/src/tools/tests.rs | 102 ++++++++++++++++++ 7 files changed, 115 insertions(+), 4 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 4a04cc7c8..57c18f463 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -11641,6 +11641,7 @@ dependencies = [ "mime_guess", "open", "parking_lot", + "postcard", "redb 4.1.0", "reqwest 0.12.28", "rust-embed", diff --git a/crates/trusty-common/src/memory_core/store/hnsw_store.rs b/crates/trusty-common/src/memory_core/store/hnsw_store.rs index 9d2bc31c1..f6717a27b 100644 --- a/crates/trusty-common/src/memory_core/store/hnsw_store.rs +++ b/crates/trusty-common/src/memory_core/store/hnsw_store.rs @@ -735,8 +735,10 @@ impl HnswStore { /// Unmap every drawer caught in an id collision so a re-embed can repair it. /// - /// 🔴 Not wired to any CLI or MCP surface, and never run against a live - /// palace in the PR that added it (#5005) — see that PR's body. + /// 🔴 Never run against a live palace in the PR that added it (#5005) — the + /// coverage below is all synthetic collisions. Reached from the + /// `palace_unalias` MCP tool via `PalaceHandle::repair_aliases`, which + /// defaults to a dry run. /// /// Why: when N uuids alias onto one `vector_id`, the single surviving /// `VECTORS` row holds whichever vector was written LAST, and search diff --git a/crates/trusty-memory/Cargo.toml b/crates/trusty-memory/Cargo.toml index 81e6c40a0..8f1d72032 100644 --- a/crates/trusty-memory/Cargo.toml +++ b/crates/trusty-memory/Cargo.toml @@ -169,6 +169,9 @@ open = "5" [dev-dependencies] tempfile = { workspace = true } +# #5005: encode a real 384-dim vector when seeding a synthetic id collision +# into a palace's index redb, so HnswStore's dimension check accepts the file. +postcard = { workspace = true } tower = { workspace = true } serial_test = { workspace = true } # Issue #1722: `task_mcp` integration tests drive `seed_shared_embedder_with_mock` diff --git a/crates/trusty-memory/changelog.d/5005-palace-unalias.md b/crates/trusty-memory/changelog.d/5005-palace-unalias.md index 0a6888498..454c54f71 100644 --- a/crates/trusty-memory/changelog.d/5005-palace-unalias.md +++ b/crates/trusty-memory/changelog.d/5005-palace-unalias.md @@ -3,3 +3,4 @@ Added - dry-run by default, like `palace_reembed`. It reports the drawer id SET (`freed_ids`), never a bare count — a count-based all-clear is the defect #5005 is about - callers branch on `outcome` (`clean` | `planned` | `repaired` | `partial` | `unavailable`) or `success`; `partial` and `unavailable` carry ids and neither is a success. `reembed_required` says outright when a `palace_reembed` run is still owed - reports `unnameable_keys`: keys in a collision group that name no drawer, so `aliased_before_ids` can be empty over a real collision. Branch on `outcome`, never on the id counts + - `palace_reembed`'s tool description now states the guard: `missing: 0` is not a complete account of what is retrievable, so read `alias_audit` — and act only on `is_clean: true` — before deleting a drawer on the strength of that report diff --git a/crates/trusty-memory/src/tools/definitions.rs b/crates/trusty-memory/src/tools/definitions.rs index ab41e2d8c..b618741d0 100644 --- a/crates/trusty-memory/src/tools/definitions.rs +++ b/crates/trusty-memory/src/tools/definitions.rs @@ -296,7 +296,7 @@ pub fn tool_definitions_with(has_default: bool) -> Value { }, { "name": "palace_reembed", - "description": "#4906: report drawers that have no vector (durable but unfindable), and optionally re-embed them. Defaults to a dry run.", + "description": "#4906: report drawers that have no vector (durable but unfindable), and optionally re-embed them. Defaults to a dry run. #5005: `missing: 0` does NOT mean every drawer is findable — a drawer lost to an id collision has a vector row and is still unreachable. Before treating this report as a complete account of what is retrievable — and ALWAYS before deleting a drawer on the strength of it — read `alias_audit`: act only on `is_clean: true`, and run `palace_unalias` first when it is false. Read `alias_audit.key_rows` vs `distinct_vector_ids` directly if you need the raw counts; they cannot be masked.", "inputSchema": { "type": "object", "properties": { diff --git a/crates/trusty-memory/src/tools/palace_ops.rs b/crates/trusty-memory/src/tools/palace_ops.rs index af5b84c0c..45910b7f5 100644 --- a/crates/trusty-memory/src/tools/palace_ops.rs +++ b/crates/trusty-memory/src/tools/palace_ops.rs @@ -338,7 +338,9 @@ pub(crate) async fn handle_palace_reembed(state: &AppState, args: Value) -> Resu /// /// 🔴 `outcome` is the field to branch on, never `freed_ids.len()`. `"partial"` /// and `"unavailable"` both carry ids and neither is a success. -/// Test: `dispatch_palace_unalias_dry_run_names_ids_and_writes_nothing`. +/// Test: `dispatch_palace_unalias_dry_run_names_ids_and_writes_nothing`, and +/// `dispatch_palace_unalias_frees_a_real_collision_and_is_idempotent` for the +/// write path (#5005 review: the success path had only ever run empty). pub(crate) async fn handle_palace_unalias(state: &AppState, args: Value) -> Result { use trusty_common::memory_core::retrieval::{AliasRepairOptions, AliasRepairOutcome}; let palace = resolve_palace(state, &args, "palace_unalias")?; diff --git a/crates/trusty-memory/src/tools/tests.rs b/crates/trusty-memory/src/tools/tests.rs index 62d2a4486..1d5200dad 100644 --- a/crates/trusty-memory/src/tools/tests.rs +++ b/crates/trusty-memory/src/tools/tests.rs @@ -3506,3 +3506,105 @@ async fn a_closed_index_queue_queues_the_palace_for_repair() { "a closed queue loses the write as completely as a full one and must queue repair" ); } + +/// Why (#5005 review): every `palace_unalias` test that reached `dispatch_tool` +/// ran against an empty palace, so the only daemon-level outcome ever observed +/// was `clean` — the branch that does nothing. The success path was proven at +/// the store layer and assumed through the tool: nothing had shown that a real +/// collision survives arg parsing, the `is_read_only()` routing a dry run skips, +/// and JSON serialization to arrive as a non-empty `freed_ids`. An unobserved +/// happy path is not a proven one, and this is the path #4834's deletion gate +/// will call. +/// What: seeds two drawer uuids onto one `vector_id` in the palace's own +/// `index.usearch.redb`, then drives `palace_unalias` with `dry_run: false` +/// through `dispatch_tool`, asserting `outcome: "repaired"`, both uuids named +/// in `freed_ids`, and `reembed_required: true`. A second call must then report +/// `clean` and free nothing — idempotence observed at the tool surface, not +/// just at the store. +/// Test: itself. Routing the write path through the read-only lock, or letting +/// `freed_ids` serialize as a count, fails it. +#[tokio::test] +async fn dispatch_palace_unalias_frees_a_real_collision_and_is_idempotent() { + use redb::{Database, TableDefinition}; + const VECTORS: TableDefinition = TableDefinition::new("vectors"); + const VECTOR_KEYS: TableDefinition<&str, u64> = TableDefinition::new("vector_keys"); + + let (state, tmp) = test_state(); + dispatch_tool(&state, "palace_create", json!({"name": "collide"})) + .await + .expect("palace_create"); + + // Two drawers, one shared vector id — the collision this PR repairs. + let a = uuid::Uuid::new_v4(); + let b = uuid::Uuid::new_v4(); + let shared_id: u64 = 7; + { + // Drop the cached handle so the palace releases its flock; the next + // dispatch reopens the file and sees the seeded collision. + state.registry.remove(&PalaceId::new("collide")); + let db = Database::create(tmp.path().join("collide/index.usearch.redb")) + .expect("open palace vector redb"); + let wtx = db.begin_write().expect("begin"); + { + let mut vectors = wtx.open_table(VECTORS).expect("vectors"); + let mut keys = wtx.open_table(VECTOR_KEYS).expect("keys"); + let encoded = postcard::to_allocvec(&vec![0.05_f32; 384]).expect("encode vector"); + vectors.insert(shared_id, encoded.as_slice()).expect("vec"); + keys.insert(a.to_string().as_str(), shared_id) + .expect("key a"); + keys.insert(b.to_string().as_str(), shared_id) + .expect("key b"); + } + wtx.commit().expect("commit"); + drop(db); // release the flock before the palace reopens the file + } + + let out = dispatch_tool( + &state, + "palace_unalias", + json!({"palace": "collide", "dry_run": false}), + ) + .await + .expect("palace_unalias must dispatch on the write path"); + + assert_eq!(out["dry_run"], false, "explicit write run: {out}"); + assert_eq!( + out["outcome"], "repaired", + "a real collision must repair, not report clean: {out}" + ); + assert_eq!(out["success"], true, "{out}"); + let freed: Vec = out["freed_ids"] + .as_array() + .expect("freed_ids array") + .iter() + .map(|v| v.as_str().expect("uuid string").to_string()) + .collect(); + assert_eq!(freed.len(), 2, "both members of the group: {out}"); + assert!(freed.contains(&a.to_string()), "{a} missing: {out}"); + assert!(freed.contains(&b.to_string()), "{b} missing: {out}"); + assert_eq!( + out["reembed_required"], true, + "freed drawers are owed a re-embed: {out}" + ); + assert!(out["error"].is_null(), "{out}"); + + // Idempotence at the tool surface: the collision is gone, not just reported. + let again = dispatch_tool( + &state, + "palace_unalias", + json!({"palace": "collide", "dry_run": false}), + ) + .await + .expect("second palace_unalias"); + assert_eq!( + again["outcome"], "clean", + "the repair must be durable, not repeatable: {again}" + ); + assert!( + again["freed_ids"] + .as_array() + .expect("freed_ids array") + .is_empty(), + "nothing left to free: {again}" + ); +}