fix(trusty-common): persist the HNSW vector-id allocator so two live stores cannot alias drawers (#5005) - #5013
Conversation
Verdict: WARNAdversarial review, RUNG 5 (persistence / concurrency / data integrity). Independent worktree off The core fix holds. Allocation is genuinely serialisable, the migration is genuinely idempotent, the bound is genuinely safe, and the test suite is honest — I re-broke it and it went red where the PR says it does. One HIGH: the new detection surface fails open, which is the same defect shape #5005 exists to close. What I verified rather than accepted1. Serialisability — CONFIRMED
I searched for an escape hatch. Empirically, break #1 — restore the pre-fix process-local That is the defect, reproduced, and the persisted counter is what removes it. 2. Migration under rolling upgrade — CONFIRMED, with one caveat belowThe
Breaks #3 (remove the seed) and #4 (change 3. The bound — CONFIRMED SAFE, no degradation
4. Test honesty — CONFIRMED, 5 of 6 breaks reproducedBaseline
Tree restored, 38 passed; 0 failed again, Break #3 fails one more test than the PR table claims (it also reds the rolling-upgrade test). That is a superset, not a shortfall. 5. Detection surface — arithmetic is sound, the error branch is not
The Err branch is the problem. See the finding below. 6.
|
| Severity | File | Line | Issue | Fix | Disposition |
|---|---|---|---|---|---|
| HIGH | crates/trusty-common/src/memory_core/retrieval/embed_repair.rs |
177-184 | A failed alias audit is swallowed into (0, 0, Vec::new()), which makes is_healthy() true and palace_reembed report aliased: 0 — a false all-clear on the one signal #5005 exists to provide |
Carry the failure: add alias_audit_failed: bool to EmbedHealth, set it in the Err arm, and make is_healthy() return false when it is set. Surface it in palace_ops.rs's JSON so a deletion gate can see "unknown" instead of "clean" |
Fix here |
| MEDIUM | crates/trusty-common/src/memory_core/store/hnsw_store.rs |
191, 220-225 | The uniqueness backstop probes VECTORS only. high_water does too, so the collision jump can land on an id that VECTOR_KEYS already claims — re-creating an alias through the exact door this PR closes |
On the (rare) collision path, take the high-water across both tables: scan VECTOR_KEYS for its max value and .max() it into the candidate. Matches what the open-time seed at :397-409 already does |
Fix here |
| LOW | crates/trusty-common/src/memory_core/store/kg_store.rs |
616-634 | table_definitions_have_distinct_names was not extended with VECTOR_ID_SEQ, so the new table name is outside the collision guard |
Add VECTOR_ID_SEQ.name() to the names array |
Fix here |
On the HIGH
This is not hypothetical severity. Break #5 substituted (0, 0, Vec::new()) for the audit — the exact value the Err arm produces — and alias_audit_surfaces_a_collision went red, including its assert!(!health.is_healthy()). The codebase's own test declares that state broken. The Err arm ships it as healthy.
The comment at :174-176 says the zeros "read as unknown rather than clean because the row count is zero too", but nothing consumes vector_key_rows == 0 that way. is_healthy() (:78-80) does not. The palace_reembed JSON does not. The PR body tells operators to "gate a deletion-bearing workflow on aliased" — and on a scan failure that gate passes.
One related line to fix at the same time: UsearchStore::alias_audit (vector.rs:426-432) drops unparseable uuids, so aliased_drawer_ids can be empty while key_rows > distinct_vector_ids. Making is_healthy() also require vector_key_rows == distinct_vector_ids closes that one in the same edit.
On the MEDIUM's reachability
The precondition is a VECTOR_KEYS row whose id has no VECTORS row. The seed at :397-409 already assumes that state is reachable ("in case VECTORS was cleared but the mapping survived (defensive)"), and there is a concrete producer: HnswStore::compact_orphans (:787-845) reads live_ids in one read txn, computes orphan_ids in a second, and removes in a third. An upsert that commits between the first and second — the in-process concurrency this PR argues is intended and load-bearing — has its brand-new VECTORS row classified as an orphan and deleted while its VECTOR_KEYS row survives.
That race is pre-existing and unchanged here, so I am not filing it against this PR. But it is what makes the MEDIUM worth the one-line widening rather than dismissing: the counter alone would still protect (it is seeded above the VECTOR_KEYS high-water), and the probe is the backstop for when the counter does not.
Required Changes
embed_repair.rs:177-184— stop reporting a failed alias audit as a clean one.is_healthy()must be false when the audit could not run, andpalace_reembedmust expose that so a deletion gate can distinguish "0 aliased" from "could not tell".
Notes
- Out of scope, correctly, but worth stating for the follow-up:
upsertreusesexistingid without an alias check (:504-506), so on the owner's live palace an ordinary re-write of one member of the 5-uuid group atvector_id988 still clobbers the other four. Unchanged behaviour, and the new detection surface now makes it visible, but the repair genuinely is still owed. - In-memory graph divergence between two live stores is real and correctly triaged as separate — redb holds every row and the next open rebuilds. Not a data-loss class.
- Both changelog fragments are present, single-category, and correctly placed.
- I ran nothing against the owner's palaces, bounced no daemon, and installed nothing. Worktree left clean at
cc0ae392.
🤖🤖🤖 Generated with trusty-mpm — https://github.com/bobmatnyc/trusty-tools
…bound allocation by both vector tables (#5005) 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
Verdict: APPROVERe-review scoped to the delta All three findings cleared. Every claim below was broken and observed red rather than read and believed. Finding 1 (HIGH) — CLEAREDThe The test is the second version, not the vacuous one. Red at line 883 — The test also carries its own control: the No other path reaches a zero-valued audit that reads as clean. I checked every route into the type:
The enum is the right shape for this: "could not tell" is unrepresentable as a number, which is what makes the class of defect structurally gone rather than patched. Finding 2 (MEDIUM) — CLEARED, and the O(n) claim is proven, not asserted
I did not take the "only on the correction path" claim on trust. I installed a Exactly the two correction-path tests. Everything else passed without ever entering So a healthy upsert costs one Finding 3 (LOW) — CLEARED, and the reasoning is soundThe author is right that deleting the Correct choice of break. Gate coverage — the reasoning holds, and I closed it rather than accepting itYou are right that an enum replacing a triple in a type crossing into Rather than stop there I ran the two gates the author skipped: The author's judgement was correct and is now evidenced. Findings
This does not gate the APPROVE. The state word at Notes
🤖🤖🤖 Generated with trusty-mpm — https://github.com/bobmatnyc/trusty-tools |
…bound allocation by both vector tables (#5005) 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
5738e5d to
7bb2ac1
Compare
…unalias tool (#5005) `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
Code Critic — Verdict: WARNReviewed at head What I verified rather than readAll four mutation proofs reproduce exactly as the table claims (worktree reverted clean after each):
Gates at this head — the
A first run of Allocator atomicity holds.
Flaky-test claim verified. Findings
HIGH — the detector can still report clean over a real collision// vector.rs:446
.filter_map(|s| match Uuid::parse_str(s) {
Ok(u) => Some(u),
Err(e) => { tracing::warn!(...); None } // <- group silently shrinks
})I built a palace with a genuine collision — two
Not CRITICAL: no supported write path can produce a non-uuid Concretely: // embed_repair.rs:94
pub fn is_clean(&self) -> bool {
matches!(
self,
Self::Measured { key_rows, distinct_vector_ids, aliased_drawer_ids }
if aliased_drawer_ids.is_empty() && key_rows == distinct_vector_ids
)
}plus the same shortfall check on the For the record, the two sibling MEDIUM — the untested branch that matters is the second one, not the firstI accept the reasoning for dropping The doc comment marks the pre-repair half as unproven. The half worth marking is the post-repair one at 429–441: it runs after keys have already been deleted, and "wrote, then couldn't verify" is a materially worse state than "refused to write". A fault-injection seam is not worth it, but a pure decision function is nearly free and covers both: fn classify(before: &AliasAudit, after: &AliasAudit, freed: &UnaliasOutcome, expected: &[Uuid])
-> AliasRepairOutcomeCallable directly with a hand-built Does landing this unblock #4834?Not on its own, and the honest answer is "run the dry run to find out."
The next step is Required changes
Everything else is sound: the persisted allocator is the right mechanism, the 🤖🤖🤖 Generated with trusty-mpm — https://github.com/bobmatnyc/trusty-tools |
…bound allocation by both vector tables (#5005) 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
26f980f to
4f53bd3
Compare
…unalias tool (#5005) `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
code-critic — round 3 @
|
| # | Break applied | Result |
|---|---|---|
| 0 | none (baseline) | test result: ok. 507 passed; 0 failed; 3 ignored; 411 filtered out |
| 1 | is_clean back to aliased_drawer_ids.is_empty() alone |
FAILED. 506 passed; 1 failed — a_collision_whose_keys_do_not_parse_is_never_clean panics at embed_repair_tests.rs:1381, "a collision the audit cannot name is still a collision" |
| 2 | repair gate back to expected.is_empty() |
FAILED. 506 passed; 1 failed — same test, embed_repair_tests.rs:1395, left: "clean" / right: "partial" |
| 3 | classify_repair's Unavailable arm falls through |
FAILED. 506 passed; 1 failed — classify_refuses_to_call_an_unverified_write_repaired, left: "partial" / right: "unavailable" |
| 4 | all restored | git diff --stat empty; test result: ok. 507 passed; 0 failed; 3 ignored |
One correction to the PR's own framing on mutation 3: with the Unavailable
arm removed the outcome degrades to Partial, not Repaired, because
after.is_clean() is a second term in the Repaired guard. The test still
goes red, and is_success() stays false either way. The guard is doubly
covered, which is better than claimed, not worse.
The counts claim, checked against the query
Verified at crates/trusty-common/src/memory_core/store/hnsw_store.rs:704-734,
not the doc comment. key_rows increments once per VECTOR_KEYS row inside
the iterator; distinct_vector_ids is by_id.len() keyed on the raw u64
value. No parse, no filter, no filter_map anywhere on that path. A mid-scan
redb error propagates through entry? and becomes Unavailable. The two
counts are copied verbatim through UsearchStore::alias_audit
(vector.rs:479-483) and AliasAudit::from_scan (embed_repair.rs:85-90).
The claim holds.
Is the fix reachable?
Partly proven, and the gap is worth naming.
- The library success path is observed producing a non-zero result:
repair_aliases_frees_the_group_and_verifies_itfrees 3 real ids and ends
repaired, andrepair_aliases_then_reembed_makes_a_lost_drawer_retrievable
drives repair → backfill →retrieve_l2and asserts previously-unretrievable
drawers answer their own queries. - The daemon path is proven only for the empty case. The single dispatch
test runs against a palace with nothing aliased, so it proves routing, arg
parsing and palace resolution work — the "completely inert at argv" failure
mode is ruled out — butoutcome: "repaired"with a non-emptyfreed_ids
has never been produced throughdispatch_tool. See MEDIUM 2.
The MockEmbedder boundary
MockEmbedder::hash_to_vec (crates/trusty-common/src/embedder/mock.rs:32-42)
is a deterministic content-dependent hash, so the three seeded drawers get
three distinct vectors and each retrieves itself by its own text. That is
enough to prove the thing this PR actually fixes: the drawer-uuid ↔ vector-id
mapping, the HNSW round trip, and that freeing a group plus re-embedding
restores retrievability. What it does not prove is anything about a real
ONNX embedder's output quality, which is not what #5005 is about. The
engineer's caveat is accurate and does not undercut the coverage.
Fail-open sweep
Every failure branch the diff adds or touches fails closed:
- scan error →
Unavailable, never zeros (AliasAudit::from_scan), and
is_clean()/is_healthy()are both false for it - pre-repair audit unreadable → refuses to write, returns
Unavailable
(embed_repair.rs:481-492) - post-repair audit unreadable →
Unavailableeven though the delete
succeeded (classify_repair) - allocator cannot find a free id →
IdAllocationFailed, upsert fails rather
than overwriting a live vector all_ids()redb failure → empty set → every drawer reads as missing →
unhealthy (fail-closed direction; pre-existing)
No arm advances state, downgrades to a default, or reports success on failure.
Partial semantics
AliasRepairOutcome::is_success() is matches!(self, Clean | Repaired) —
Partial and Unavailable are false by construction. The only production
consumer in the tree is handle_palace_unalias
(crates/trusty-memory/src/tools/palace_ops.rs:337-397), which emits
"success": false and populates still_aliased_ids / not_freed_ids /
unparsed_keys. Nothing else reads AliasRepairReport. A Partial cannot be
read as success anywhere in-tree.
Findings
| Severity | File | Line | Issue | Fix | Disposition |
|---|---|---|---|---|---|
| MEDIUM | crates/trusty-memory/src/tools/definitions.rs | 299 | palace_reembed's tool description still reads "report drawers that have no vector … Defaults to a dry run." The PR adds alias_audit, vector_key_rows, distinct_vector_ids and aliased to that payload, and the source comment at palace_ops.rs:303-308 tells callers to "Gate deletions on alias_audit == \"clean\" as well as missing == 0" — but the description an MCP caller actually reads never says so. The guard is correct and the caller is never told to reach it. This is the surface #4834's deletion gate will read. |
Extend the description: "missing: 0 is NOT sufficient — an aliased drawer HAS a vector key and is still unretrievable. Also require alias_audit == \"clean\"; \"unavailable\" means the scan failed and is a block, not a pass. Repair with palace_unalias." |
Fix here |
| MEDIUM | crates/trusty-memory/src/tools/tests.rs | 253-280 | dispatch_palace_unalias_dry_run_names_ids_and_writes_nothing runs against a freshly created palace, so it only ever reaches outcome: "clean" with freed_ids: []. Through dispatch_tool the destructive branch has never produced a non-zero result — including the is_read_only() check inside repair_aliases, which a dry run skips entirely. If the daemon opens the palace read-only, every real palace_unalias run errors and the dry-run test still passes. |
Add a dispatch test that seeds a collision into the test palace's idx.usearch.redb (the seed_aliased_vector_file pattern already exists in embed_repair_tests.rs:752), calls palace_unalias with dry_run: false, and asserts outcome == "repaired", non-empty freed_ids, and reembed_required == true. |
Fix here |
| MEDIUM | crates/trusty-common/src/memory_core/store/hnsw_store.rs | 738-739 | The 🔴 doc block on HnswStore::unalias says "Not wired to any CLI or MCP surface, and never run against a live palace in the PR that added it (#5005)". This PR wires it — palace_unalias → repair_aliases → UsearchStore::unalias → here. Half the warning is now false, and it is the half a reader uses to judge whether the repair is reachable. |
Replace with: "🔴 Never run against a live palace. Reached through palace_unalias → PalaceHandle::repair_aliases, which adds the dry run and the post-repair verification; call those, not this." |
Fix here |
| LOW | crates/trusty-common/src/memory_core/retrieval/embed_repair.rs | 481-492 | The pre-repair Unavailable arm — refuse to write when the audit could not run — has no test through the real path. an_unavailable_or_partial_repair_is_never_a_success asserts the enum contract only, and classify_repair covers the post-repair audit. The same extract-a-pure-function move that made the post branch reachable was not applied here. Fails closed, so the risk is a silent regression rather than data loss. |
Either extract the pre-repair gate the way classify_repair was extracted, or accept it and say so in the doc comment's coverage note, as an_unavailable_or_partial_repair_is_never_a_success already does for its own branch. |
Parent |
| LOW | crates/trusty-memory/src/tools/palace_ops.rs | 369-372 | "error" is populated only for Unavailable. A Partial run carries error: null alongside success: false, so a caller that checks error == null reads an incomplete destructive repair as fine. outcome and success are both correct; this is the third field disagreeing in tone. |
Populate error on Partial too, e.g. "freed the group but the worklist is incomplete: N unnameable keys, M still aliased". |
Parent |
Notes
- Not flagged, below 80 %:
allocate_vector_id(hnsw_store.rs:192-195)
probesVECTORSonly on the first attempt, whilehigh_watercorrectly
consults bothVECTORSandVECTOR_KEYS. AVECTOR_KEYSrow can outlive
itsVECTORSrow — the PR documents that state itself and tests it in
upsert_refuses_an_id_that_only_vector_keys_still_claims, but only with the
counter row absent. Reaching it with the counter present but stale needs a
hand-edited file or a pre-trusty-memory: HnswStore vector-id allocator aliases across processes — upsert has no uniqueness check, silently overwrites drawers #5005 binary writing between two opens, and
open-time seeding raises the counter tomax(VECTORS, VECTOR_KEYS) + 1
while redb's exclusive lock bars two live writer processes. I can't assert
it is reachable, so it is a note, not a finding. - No tests were deleted. Every test function present on
origin/mainin
vector.rsandhnsw_store.rsis still present after the split into
vector/tests.rsandhnsw_store/tests.rs(verified bycommover sorted
function lists). The −331 / −193 in the diffstat is relocation, not removal. cold_restart_recalls_beyond_l1_snapshotis pre-existing onorigin/main
— do not hold this PR for it. It is#[ignore]d ("requires real ONNX
embedder (issue consolidate trusty-mpm to a single binary (6 [[bin]] → 1) #850)") and only surfaces under--include-ignored. I ran
it on both trees. Branch4f53bd32:FAILED. 0 passed; 1 failed, hits at
0.32576936, 0.32574186, 0.32571387, ….origin/maine97e40e6f:
FAILED. 0 passed; 1 failed, hits at0.32576936, 0.32574186, 0.32571387, …— bit-identical scores. The failure is the mock hash embedder standing in
for ONNX, unrelated to this branch.
Gates run
cargo test -p trusty-common --features memory-core --lib memory_core
→ test result: ok. 507 passed; 0 failed; 3 ignored; 0 measured; 411 filtered out
cargo test -p trusty-memory
→ test result: ok. 564 passed; 0 failed; 4 ignored (lib)
→ all 21 further test binaries ok, 0 failed
( bash scripts/check_line_cap.sh
&& cargo fmt --check
&& cargo clippy -p trusty-common --features memory-core --all-targets -- -D warnings
&& cargo clippy -p trusty-memory --all-targets -- -D warnings )
→ EXIT=0
Test ladder: rung 4 (cross-crate — trusty-common library plus its
trusty-memory consumer). Changelog fragments present for both crates.
Zero CRITICAL, zero HIGH → APPROVE. The three MEDIUMs are all one-edit
fixes; MEDIUM 1 should land before #4834 builds a deletion gate on
palace_reembed.
🤖🤖🤖 Generated with trusty-mpm — https://github.com/bobmatnyc/trusty-tools
…stores cannot alias drawers (#5005) `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<Database>` 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
… fields 🤖🤖🤖 Generated with trusty-mpm — https://github.com/bobmatnyc/trusty-tools
…bound allocation by both vector tables (#5005) 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
…udit (#5005) `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
…unalias tool (#5005) `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
…vived (#5005) `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
…l had (#5005) 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
… the write path (#5005) 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
f8d7eaa to
1c409c0
Compare
Rebased onto
|
2 participants
Closes #5005. Owning crate is
trusty-common—hnsw_store.rslives in itsmemory-corefeature, not intrusty-memory, which matters for the gates (see below).Is overlapping
HnswStoreopen over one file intended?Yes, in-process — and that is where the aliasing comes from. This is the question the fix hinges on, so the evidence:
flock(LOCK_EX)per file. A writer that hits a held lock retries a bounded window and then fails loud —open_writer_with_handoff_retry,concurrent_open.rs:331, deliberately refusing to degrade (bug(trusty-memory): stdio bridge auto-spawns unmanagedserve --foregroundthat squats the production redb write-lock #1152/bug(trusty-memory): HTTP daemon opens palace redb as ReadOnlyClient — a 2nd instance silently degrades to read-only and rejects ALL writes #1487). A read-only client gets a copy under$TMPDIRand every write method returnsReadOnly. So two writing processes over one file cannot exist, and preventing "the second open" buys nothing here.open_or_get_cached_db(vector.rs:101) caches anArc<VectorDbState>per canonical path precisely so the same palace can be opened twice without tripping redb's lock — its own doc says so. EveryUsearchStore::newfor that palace then builds a freshHnswStoreover the sharedArc<Database>(vector.rs:293). Shared database, privateAtomicU64counter each.That is the trigger the issue's follow-up comment narrowed to, and it explains the measured negative result: a single writer allocating 15 ids from empty tables was never going to fail.
So the fix is the first branch the brief named — persisted, atomically-reserved allocation plus a uniqueness guard — not blocking the second open.
The fix
Allocation moved into redb. New single-row table
vector_id_seq(kg_store.rs).HnswStoreno longer carries a counter at all;allocate_vector_idreads and bumps the persisted one inside the same write transaction that writes the row claiming it. redb permits one write transaction at a time per database, so allocation is now serialisable against every other writer on that file, and the id and its row commit or roll back together.Uniqueness guard on insert.
allocate_vector_idwill not return an id that already has aVECTORSrow. It jumps past the highest occupied id and retries (bounded,MAX_ALLOC_PROBES = 8), then fails withHnswStoreError::IdAllocationFailed. It never overwrites. Both branches log at WARN, so a counter that ever falls behind is visible rather than inferred.Single-store id assignment is unchanged: a fresh palace still issues 1, 2, 3 in order, exactly as the old
max_seen + 1seed did.Migration, and the rolling-upgrade case
Every palace on disk predates
vector_id_seq. Every read-write open raises the counter tomax(VECTORS, VECTOR_KEYS) + 1— as a max, never a set-if-missing. That one choice covers both cases the brief asks about:The old binary is never concurrent with the new one — redb's flock forbids two live writers — so the interleaving is strictly sequential and the max is sufficient. Read-only/snapshot opens skip the seed entirely; every write path there returns
ReadOnlybefore it could allocate.Detection (#5000 requirement, and how I verified the fix)
AliasAuditcomparesVECTOR_KEYSrow count against distinct mappedvector_idcount — arithmetic that never leaves that one table, so nothing about key presence can fool it. Wired where embed health already lives:PalaceHandle::embed_healthcarries anAliasAudit—Measured { key_rows, distinct_vector_ids, aliased_drawer_ids }orUnavailable { reason }.is_healthy()requires no missing drawers and an audit that ran and came back clean; key presence alone was never the health condition, and neither is an audit that could not run.palace_reembedreturnsalias_audit(clean|aliased|unavailable),alias_audit_error,vector_key_rows,distinct_vector_ids,aliased,aliased_ids. Gate a deletion-bearing workflow onalias_audit == "clean"as well asmissing == 0.unavailablewith null counts, never zeros. The Result-to-outcome mapping isAliasAudit::from_scan, a named function with its own test, soembed_healthhas no error branch of its own to get wrong.What I did NOT do
trusty-toolsdrawers are untouched. Not repaired, not re-embedded, not read beyond what the issue already recorded.unalias()is code only and was never run.HnswStore::unalias/UsearchStore::unaliasfree every uuid in a collision group and tombstone the shared id, so the group reads as ordinary "missing" and the existing backfill repairs it. It frees the whole group, including the reachable member:VECTORSholds whichever vector was written last and search resolves the id to whichever uuid sorts last, and those two are unrelated — so the reachable drawer's content is not reliably its own either. It is tested but not wired to any CLI or MCP surface. A follow-up needs one flag onpalace_reembed(or a smallpalace_unaliastool) plus an operator run; that is a deliberate stop, so nobody can repair the owner's palace by accident.acannot findb's newest writes until the file is reopened. Redb has every row and the next open rebuilds, so nothing is lost — unlike the id collision, which destroyed content. Separate defect, not filed; say the word and I will.Tests — and what each one goes red on
Every test below was broken deliberately and confirmed red, one mechanism at a time. Raw output for each break is in the gate section.
two_live_stores_over_one_file_never_alias_idsAtomicU64seeded at openupsert_refuses_to_reuse_an_id_already_present_in_vectorsvectors.get(candidate)?.is_none()probeold_palace_without_a_seq_row_is_seeded_on_openreopen_raises_a_counter_an_old_binary_left_behindaudit_detects_two_uuids_mapped_to_one_idembed_healthbreak)alias_audit_surfaces_a_collisionembed_healthreportingmissing: 0and naming all three aliasedembed_healthdrop the alias auditunalias_frees_every_uuid_in_a_collision_group,unalias_marks_the_whole_group_for_reembedalias_audit_failure_is_never_reported_as_clean(review finding 1)AliasAudit::from_scan'sErrarmupsert_refuses_an_id_that_only_vector_keys_still_claims(review finding 2)VECTOR_KEYSrow surviving itsVECTORSrow does not get its id re-issuedhigh_watertovectors.last()? + 1table_definitions_have_distinct_names(review finding 3)VECTOR_ID_SEQ's table name is unique"vectors"One note on discipline: the first attempt at finding 1's test built an
AliasAudit::Unavailablevalue by hand, and the break did not go red — the test never reached the mapping. It was rewritten to driveAliasAudit::from_scan(Err(..)), the branchembed_healthactually takes, and then reproduced red. The vacuous version is not what shipped.Each break failed only the tests naming it — the other 26 stayed green, so none of these assertions is being satisfied by a second mechanism.
Reproducing the defect is expressible as a test:
two_live_stores_over_one_file_never_alias_idsopens twoHnswStores over oneArc<Database>, exactly whatopen_or_get_cached_dbdoes in production. On the pre-fix allocator it fails at the first pair — both stores seed to 1, so storeb's first upsert takes the id storeajust issued. It asserts on a rehydrated index rather than on either live store, because each live store'shnsw_rsgraph only holds its own inserts; redb is the authority and the rebuild is what proves the persisted state is complete and unaliased.Fixtures that the fixed public API can no longer produce — an aliased
VECTOR_KEYSmapping, a rewound counter — are written at the redb level.SLOC split
hnsw_store.rsreached 780 SLOC andvector.rs526 with the new tests (the 500 cap counts inline#[cfg(test)]modules). Both inline test modules moved to childtests.rsfiles — child, not sibling, because the tests reach the store's privatedbhandle. No production code moved;check_line_cap.shis clean.Gates — rung 5,
trusty-common🔴 Gate trap, and it applies here.
hnsw_store.rsis intrusty-common'smemory-corefeature, and that crate'sdefault = []. A barecargo test -p trusty-commoncompiles none of this and still exits 0. Every command below passes--features memory-core,embedder-test-support.--include-ignored: 2 failures, both proven pre-existingNeither was made green by ignoring, cfg-gating or excluding anything. Both are ONNX-embedder-dependent tests reachable only via
--include-ignored:timeout_fires_on_embedder_init_with_tiny_limit— passes in isolation (1 passed; 0 failed). It assertsshared_embedder()returnsErron a tiny timeout and gotOk; with a warm model cache and a shared embedder already initialised by an earlier test in the same process, init returns instantly. Cross-test ordering artifact. Nothing in this diff touches embedder init.cold_restart_recalls_beyond_l1_snapshot— fails identically on cleanorigin/main. Checked out7045aa18in a throwaway worktree and ran the single test:test result: FAILED. 0 passed; 1 failed; 902 filtered out. Same panic, same line (tests.rs:631). It is the HuggingFace-model-dependent flake class of #852; every returned hit scores ~0.3257, i.e. the embeddings are degenerate because the real model is not loaded on this host.Change-specific gates pass; those two are blocked by the environment, not by this branch.
cargo check --workspacewithout the three exclusions fails intrusty-mpm-gui/trusty-code-guiwithfrontendDist ... ui/dist doesn't exist— a missing pnpm build, unrelated to this diff. The exclusion list is the one CI itself uses (ci.yml:811).LOC
Added 1403 / Removed 525 / Net +878 — of which 866 lines are the two test modules being moved out of their parent files, not new code.
🤖🤖🤖 Generated with trusty-mpm — https://github.com/bobmatnyc/trusty-tools
Follow-up in this PR: the repair is now runnable (#5005)
The section above says
unalias()is "code only… not wired to any CLI or MCPsurface". That was the gap that made this PR insufficient: it stops NEW aliasing
and makes existing aliasing visible, but leaves the three already-destroyed
drawers in the live
trusty-toolspalace durable-but-unretrievable — which iswhat blocks #4834.
grep -rn "unalias" crates/confirmed zero non-test call sites.Two new pieces:
PalaceHandle::repair_aliases(the operator primitive, inembed_repair.rsbesidebackfill_missing_vectors) andpalace_unalias(theMCP tool over it). Tool count 44 → 45.
Dry run by default, and it names ids
palace_unaliasmirrorspalace_reembed:dry_rundefaults to true, and thedefault run reports what it would free while writing nothing. More is at stake
here — this one deletes
VECTOR_KEYSrows, not just embeddings.The result carries
freed_ids, the drawer id set. Not a count. #5005 was acount (
missing: 0) reporting all-clear over four destroyed drawers; a repairanswering "3 repaired" without saying which three is the same defect one layer
up. Those ids are also the operator's re-embed worklist, so the caller needs
them by name regardless.
It cannot report success over a partial repair
AliasRepairOutcomehas five variants andRepairedis reachable throughexactly one path: the post-repair audit ran, came back clean, and
accounted for every id the pre-repair audit named. Anything else is its own
variant, and
is_success()is false for all of them:is_success()CleanPlannedRepairedPartialUnavailableTwo guards worth calling out:
Unavailableis checkedbefore the dry-run branch and before the read-only guard, so a failed scan
never reaches
unalias. Deleting vector keys with no idea which (or whetherany) are aliased, then reporting a clean palace, is the exact shape this
ticket exists to remove.
UsearchStore::unaliasno longer drops keys it cannot parse. It returnedVec<Uuid>built withfilter_map(…ok()), so a freed key that would notparse vanished from the worklist — a drawer left with no vector and nobody
knowing to repair it, reported inside a success. It now returns
UnaliasOutcome { freed, unparsed_keys }, and a non-emptyunparsed_keysforces
Partial.Idempotent: the second run finds no group, reports
clean, frees nothing.reembed_requiredsays outright when apalace_reembedrun is still owed —freeing a group turns an invisible drawer into an ordinary missing one, and only
the backfill makes it findable again.
Not run against any live palace. That stays an operator action, with a
backup taken immediately beforehand.
Tests — each one broken and confirmed red
repair_aliases_dry_run_names_the_group_and_changes_nothingDefaultreturnsdry_run: falserepair_aliases_never_reports_success_over_a_partial_repairunparsed_keysterm from theRepairedguardrepair_aliases_frees_the_group_and_verifies_it+ 2 othersunaliasan_unavailable_or_partial_repair_is_never_a_successis_success→!matches!(self, Self::Planned)repair_aliases_never_reports_success_over_a_partial_repairdispatch_palace_unalias_dry_run_names_ids_and_writes_nothingrepair_aliases_then_reembed_makes_a_lost_drawer_retrievableis the end-to-endclaim, and it asserts on recall rather than on table arithmetic: it seeds three
drawers with distinct content sharing one
vector_id, discovers whichmembers their own content cannot retrieve (the collision collapses the group
onto one reachable uuid), then runs repair → backfill → search and asserts each
previously-unretrievable drawer now answers its own query. Fail-before is
asserted in the same test, including that
embed_healthreports zero missingwhile they are lost.
One test was dropped rather than faked
repair_aliases_refuses_to_run_on_an_unreadable_auditis not in the diff. TheUnavailablebranch fires whenUsearchStore::alias_auditerrors, which needsa redb read failure — and every fixture that breaks that read also breaks
UsearchStore::new, so the store cannot be constructed in the state the testneeds. Corrupting the file gets it silently recreated; removing the
vector_keystable fails at open (Table 'vector_keys' does not exist).The branch is kept — a redb read genuinely can fail on a real disk error — but
its trigger is not reproducible in-process.
an_unavailable_or_partial_repair_is_never_a_successcovers the contract the MCP layer actually depends on, and its doc comment says
which half is unproven. A test built by hand-constructing
AliasAudit::Unavailablewould have been vacuous in exactly the way review finding 1 already caught once
in this PR, so it was not written.
Gates — rung 5, rebased onto
b8746f59--include-ignoredstill reports the same 2 pre-existing failures, unchanged bythe rebase and by this work:
git diff --name-only origin/main...HEADtouches neither test's file.timeout_fires_…passes in isolation (1 passed; 0 failed) — a cross-testordering artifact.
cold_restart_…fails in isolation with the signature thisPR documented before the rebase: every hit scoring ~0.3257, i.e. degenerate
embeddings because the real model is not loaded on this host
(#852). Change-specific
gates pass; those two are blocked by the environment.
Rebase note on #5039
#5039 landed a clippy fix
in
embed_repair.rs— the only file both this branch and the last 12 commits ofmain touch. It rewrote the drawer-liveness filter at what is now line 256; this
branch's hunks are at 46 / 162 / 268 / 373. No textual overlap, and both changes
are present on the rebased head: the De Morgan form (
!d.is_expired_at(now) || d.is_tier_c()) and theAliasAuditsurface. Neither side was taken blindly.🤖🤖🤖 Generated with trusty-mpm — https://github.com/bobmatnyc/trusty-tools
Review round 2 — the HIGH is fixed
The critic found the fail-open shape I killed in
UsearchStore::unaliashadsurvived one layer up, in the detector. Reproduced before touching anything:
Two
VECTOR_KEYSrows on onevector_idis a real collision. It reportedcleanand did nothing — this PR's own defect, on the machine-readable fieldthe PR tells callers to branch on, with #4834's deletion gate as that caller.
Fix.
alias_auditcarries the unnameable keys instead of dropping them, andis_clean()now consultskey_rowsvsdistinct_vector_ids. Those counts comestraight off the table — no parse, no filter can shrink them — which is what
makes them the signal that cannot be fooled.
repair_aliasesgates onis_clean()rather thanexpected.is_empty(), so an all-unnameable group fallsthrough to
unaliasand ends asPartial: freed, with the worklist honestlyincomplete.
palace_unaliasreportsunnameable_keyson the wire.The MEDIUM, taken as suggested.
classify_repairis now a pure function over(&AliasAudit, &UnaliasOutcome, &[Uuid]). It takes the audit as a parameter, sothe post-repair
Unavailablebranch — the one that fires after keys aredeleted — is reachable with a hand-built value and no fault-injection seam. No
indirection added to the production path: the function tested is the function
called. It also subsumes what the dropped pre-repair test was reaching for.
Three new mutation proofs, each red, each restored:
is_cleanback to the id list alonea_collision_whose_keys_do_not_parse_is_never_cleanFAILEDrepair_aliasesgate back toexpected.is_empty()classify_repair'sUnavailablearm falls throughclassify_refuses_to_call_an_unverified_write_repairedFAILEDGates, rebased onto
e97e40e6The critic's vacuous-green warning is worth keeping:
cargo test -p trusty-commonwithout
--features memory-corecompiles none of this module and still exits0. Every command above carries the flag.
Does this unblock #4834? Unconfirmed — and the check is read-only
palace_reembedreportingmissing: 0over unretrievable drawers is consistentwith aliasing and with #852 degenerate embeddings. The e2e test uses
MockEmbedder, so it proves the key/vector plumbing, not that the real embedderreturns a retrievable vector for those three drawers. Nothing here was run
against the live palace.
The two causes are cleanly separable from read-only output already on the wire:
vector_key_rowsvsdistinct_vector_idspalace_unaliasdry runaliased_before_idsclean, frees nothingSo: run
palace_unaliaswith the default dry run (read-only, deletes nothing)and read
vector_key_rows/distinct_vector_idsoffpalace_reembeddirectly. If the counts are equal and the dry run says
clean, aliasing is notthe cause and #4834 needs #852, not this PR. This PR is still correct and worth
landing either way — it closes a real data-destroying defect — but it should not
be described as unblocking #4834 until that dry run has named the three drawers.
🤖🤖🤖 Generated with trusty-mpm — https://github.com/bobmatnyc/trusty-tools
Review round 3 — the three MEDIUMs, fixed here
Correction to my round-2 table first, from the critic's independent re-run:
mutation 3 degrades to
Partial, notRepaired, becauseafter.is_clean()isa second term in the guard. Red either way — doubly covered, not less.
1.
palace_reembed's description omitted the guard the payload gained.The text an MCP caller actually reads still promised only "drawers that have no
vector". A correct guard nobody is told to reach had moved the capstone shape
out of the code and into the tool contract, with #4834's deletion gate as the
reader. It now says
missing: 0is not a complete account of what isretrievable, and to act only on
alias_audit.is_clean.2. The daemon success path had never produced a non-zero result. Every
dispatch_tooltest ran against an empty palace, sooutcome: "repaired"wasproven at the store layer and assumed through the tool — including the
is_read_only()routing a dry run skips. New test seeds two uuids onto onevector_idin the palace's ownindex.usearch.redband drivesdry_run: falsethrough
dispatch_tool. Forcingdry_run = truein the handler fails it:That payload also shows the seed is a real collision the audit detects, not a
fixture the assertion merely walks past.
3. Stale 🔴 doc block on
HnswStore::unaliassaid it was wired to no MCPsurface. This PR wires it. Corrected to name the
palace_unaliaspath and keeponly the claim still true — no live-palace run.
postcardjoins trusty-memory's dev-dependencies: the seeded vector must encodeat the real 384 dimensions or
HnswStorerejects the file. One line inCargo.lock, no refresh.Gates at
f8d7eaa0Every trusty-common command carries
--features memory-core; a bare runfilters out all 294 memory-core tests and still exits 0.
Scope note:
cold_restart_recalls_beyond_l1_snapshotis pre-existing onorigin/main— bit-identical scores on both trees,#[ignore]d pending a realONNX embedder. Not touched, not chased.
🤖🤖🤖 Generated with trusty-mpm — https://github.com/bobmatnyc/trusty-tools