Fix three memU write paths that damage store integrity - #254
Closed
oranjeai wants to merge 7 commits into
Closed
Conversation
nerve's memory_update and memory_delete tools, and the equivalent web-UI
routes, all reach three defects in memu-py 1.4.0. All were reproduced against
the code on main and measured on a live 139,187-item store.
1. A content-only memory_update unlinks EVERY category of the item. memU has
one sentinel for two meanings: _patch_update_memory_item maps a missing
`categories` argument to [] (_map_category_names_to_ids returns [] for a
falsy list), so cats_to_remove becomes the item's entire current set. 154 of
the 154 items ever updated without a categories argument now hold zero
category links, about 47 percent of all 326 orphaned items. Worse, each
unlink records (old_content, None), which the summary-patch step renders as
"This memory content is discarded", so the LLM rewrites the category summary
to drop the item too. Fix 8: when `categories` is None, rewrite the payload
with the names of the item's current links, so memU's own diff removes
nothing and records (old, new) instead. An explicit list still replaces and
an explicit [] still clears.
2. update_item never refreshes extra.content_hash, which create_item_reinforce
dedups on, so an updated item keeps its old text's hash forever: 149 of 149
updated rows are hash-stale against a 400/400 fresh never-updated baseline.
The consequence is measurable, not theoretical - correct an item's wording,
re-memorize the old wording, and the corrected row is reinforced under its
new text. Fix 9: recompute the hash from the DB row when summary or
memory_type changes. Reading the ROW rather than get_item() is load-bearing:
read paths build MemoryItem without extra=, so a cached item has extra == {}
and a cache-based version would refresh nothing in any long-lived process
while still passing a create-path test. Only a hash that already exists is
refreshed, so an item created without one is not newly enrolled into dedup.
3. delete_item deletes the item row and leaves its memu_category_items rows
behind - there is no FK and no ON DELETE CASCADE, and no layer owns the
dependent rows. 6,455 dangling relations, and every one of the 5,611
distinct dangling item_ids is in the item_deleted audit log. They also
inflate memory_expand_category's reported total, which counts relations
while listing through a JOIN, by 3.5 to 4.4 percent on every category.
Fix 10: delete relations and the item in ONE transaction, so a failure can
never leave a surviving item stripped of its links. Installed before Fix 3
so its vector-index hook still wraps it.
The same investigation found the cause of a fourth, related population: Fix 7's
semantic-dedup writeback built `extra` from the item CACHE and assigned it over
the row's whole extra. Since read paths omit extra=, a reinforce through a cold
cache replaced the row's extra with just its own two salience keys, deleting
content_hash and any key another writer had added. All 6,473 rows with no
content_hash carry reinforcement_count > 1, none carry rc == 1, and the store
holds zero items of type "tool" - the only create path that legitimately writes
no hash - so that whole population is wipe damage. The writeback now seeds
`extra` from the row inside its existing transaction and refreshes the cache
from what was written, which is correct for every writer rather than only the
ones we know about: a raw-SQL sweep in this file adds extra.mentioned_at without
touching the cache, and that key is now preserved too.
All four patches live in _patch_sqlite_bugs(), the established seam for this
class of memu-py defect, which already carries seven numbered fixes.
Scope. This fixes the write paths only; it repairs no existing damage, and the
154 lost category memberships are not recoverable (the audit log records only
categories_changed: false, never the ids). Named but deliberately not fixed:
the read paths still omit extra= (no longer destructive now that the writeback
reads the row, so hydration is a separate change); clear_items has the same
dangling-relation shape but zero nerve callers; the in-memory and postgres repo
siblings and the in-memory reinforce arm are unreachable because the provider
is hardcoded sqlite; memu.app.patch.PatchMixin's duplicate handlers are dead
code. Tests pin each of those facts so an exemption cannot rot silently.
Behaviour changes worth noting: a reinforce now preserves more of extra,
including ref_id, which list_items_by_ref_ids filters on; hash-dedup now
matches rows whose hash used to be wiped, so some memorizations reinforce
instead of duplicating (which is the configured intent); and Fix 8 raises
rather than unlinking if an item's category ids do not round-trip through
ctx.category_name_to_id, which cannot happen through a supported path because
nerve rebuilds that map from every DB category on init.
Tests: 31 new tests in tests/test_memu_bridge.py. Each defect has a control arm
driving memU's own unpatched function against an identically-built fixture, so
a test that passes without the fix is a failed test rather than a passing fix.
A 13-mutant matrix over the four patches kills a test for every mutant, with a
no-op control that stays green. Full suite 2964 passed, with the pre-existing
failure set unchanged by name.
nerve's memory_update and memory_delete tools, and the equivalent web-UI
routes, all reach three defects in memu-py 1.4.0. All were reproduced against
the code on main and measured on a live 139,187-item store.
1. A content-only memory_update unlinks EVERY category of the item. memU has
one sentinel for two meanings: _patch_update_memory_item maps a missing
`categories` argument to [] (_map_category_names_to_ids returns [] for a
falsy list), so cats_to_remove becomes the item's entire current set. 154 of
the 154 items ever updated without a categories argument now hold zero
category links, about 47 percent of all 326 orphaned items. Worse, each
unlink records (old_content, None), which the summary-patch step renders as
"This memory content is discarded", so the LLM rewrites the category summary
to drop the item too. Fix 8: when `categories` is None, rewrite the payload
with the names of the item's current links, so memU's own diff removes
nothing and records (old, new) instead. An explicit list still replaces and
an explicit [] still clears. Preservation does not depend on `content` being
supplied: a type-only update reaches the same diff and is covered.
2. update_item never refreshes extra.content_hash, which create_item_reinforce
dedups on, so an updated item keeps its old text's hash forever: 149 of 149
updated rows are hash-stale against a 400/400 fresh never-updated baseline.
The consequence is measurable, not theoretical - correct an item's wording,
re-memorize the old wording, and the corrected row is reinforced under its
new text. Fix 9: recompute the hash from the DB row when summary or
memory_type changes. Reading the ROW rather than get_item() is load-bearing:
read paths build MemoryItem without extra=, so a cached item has extra == {}
and a cache-based version would refresh nothing in any long-lived process
while still passing a create-path test. Only a hash that already exists is
refreshed, so an item created without one is not newly enrolled into dedup.
3. delete_item deletes the item row and leaves its memu_category_items rows
behind - there is no FK and no ON DELETE CASCADE, and no layer owns the
dependent rows. 6,455 dangling relations, and every one of the 5,611
distinct dangling item_ids is in the item_deleted audit log. They also
inflate memory_expand_category's reported total, which counts relations
while listing through a JOIN, by 3.5 to 4.4 percent on every category.
Fix 10: delete relations and the item in ONE transaction, so a failure can
never leave a surviving item stripped of its links. Installed before Fix 3
so its vector-index hook still wraps it. The cache and DatabaseState.relations
are evicted on BOTH paths, as memU's own delete_item does: returning early
for an already-deleted row would leave the id in self.items, and Fix 5 serves
that cache unfiltered, so list_items() would keep returning a deleted item.
No DB write is attempted when the row is absent.
The same investigation found the cause of a fourth, related population: Fix 7's
semantic-dedup writeback built `extra` from the item CACHE and assigned it over
the row's whole extra. Since read paths omit extra=, a reinforce through a cold
cache replaced the row's extra with just its own two salience keys, deleting
content_hash and any key another writer had added. All 6,473 rows with no
content_hash carry reinforcement_count > 1, none carry rc == 1, and the store
holds zero items of type "tool" - the only create path that legitimately writes
no hash - so that whole population is wipe damage. The writeback now seeds
`extra` from the row inside its existing transaction and refreshes the cache
from what was written, which is correct for every writer rather than only the
ones we know about: a raw-SQL sweep in this file adds extra.mentioned_at without
touching the cache, and that key is now preserved too.
All four patches live in _patch_sqlite_bugs(), the established seam for this
class of memu-py defect, which already carries seven numbered fixes.
Scope. This fixes the write paths only; it repairs no existing damage, and the
154 lost category memberships are not recoverable (the audit log records only
categories_changed: false, never the ids). Named but deliberately not fixed:
the read paths still omit extra= (no longer destructive now that the writeback
reads the row, so hydration is a separate change); clear_items has the same
dangling-relation shape but zero nerve callers; the in-memory and postgres repo
siblings and the in-memory reinforce arm are unreachable because the provider
is hardcoded sqlite; memu.app.patch.PatchMixin's duplicate handlers are dead
code. Relations left behind by an item another process already deleted are also
out of scope: a relations-only DELETE for a vanished item is a separate concern.
Tests pin each of those facts so an exemption cannot rot silently.
Behaviour changes worth noting: a reinforce now preserves more of extra,
including ref_id, which list_items_by_ref_ids filters on; hash-dedup now
matches rows whose hash used to be wiped, so some memorizations reinforce
instead of duplicating (which is the configured intent); and Fix 8 raises
rather than unlinking if an item's category ids do not round-trip through
ctx.category_name_to_id, which cannot happen through a supported path because
nerve rebuilds that map from every DB category on init.
Tests: 34 new tests in tests/test_memu_bridge.py. Each defect has a control arm
driving memU's own unpatched function against an identically-built fixture, so
a test that passes without the fix is a failed test rather than a passing fix.
A 16-mutant matrix over the four patches kills a test for every mutant, with a
no-op control that stays green. Two of those mutants pin properties an earlier
revision of these tests could not see: an item-first split of Fix 10's deletes
(the existing atomicity case forces the ITEM delete, which raises first in both
orderings, so a second case forces only the RELATIONS delete), and Fix 8 gated
on `content` (which a type-only update exposes). Full suite 2967 passed, with
the pre-existing failure set unchanged by name.
This message supersedes the test counts of the previous revision of this commit
(31 new tests, 13 mutants, 2964 passed), which predate the three cases added
here.
Three defects in the memu-py SQLite backend, each measured on the live store: 1. A content-only memory_update unlinks EVERY category. Omitting `categories` passes None, which memU maps to [] and diffs as "remove all". 154 of 154 records updated without a categories argument now hold zero links - about 47% of every orphan in the store. 2. memory_update never recomputes extra.content_hash, which create_item_reinforce dedups on, so an updated record can never again be recognised as a duplicate: 149 of 149 updated items are hash-stale against a 400/400 fresh baseline. 3. memory_delete orphans category relations: there is no FK and no ON DELETE CASCADE, and no layer owns the dependent rows. 6,455 dangling relations, every one of the 5,611 distinct item_ids present in the item_deleted log. A fourth defect surfaced while measuring the second: the semantic-dedup writeback seeded `extra` from the ITEM CACHE and assigned it over the row's whole `extra`. Read paths build MemoryItem without extra=, so a reinforce through a cold cache deleted content_hash and every key another writer had added. This explains the hashless population (all rc > 1, none rc == 1, and the store holds 0 items of the one type that legitimately writes no hash). All four are fixed in _patch_sqlite_bugs(), the established seam for this class of memu-py defect - same tool and same layer as ClickHouse#119. The hash refresh reads the row memU's write LEFT BEHIND rather than a pre-read snapshot, and writes back through a single conditional UPDATE that is a no-op unless summary and memory_type still hold what was just written. A snapshot taken before the delegation closes its session before memU's write commits, so a writer outside the memU loop thread (the date sweep runs on _blocking_pool with its own connection; `nerve memory` is a second process) could otherwise leave a hash of text the row does not hold. When the conditional write declines, the cache is left alone rather than claiming a refresh that never landed. Scope: write paths only. No existing data is repaired, and the 154 lost memberships are not recoverable - the audit log records only `categories_changed: false`, never the ids. An absent-row delete issues no DB write at all, so a vanished item's own relation rows stay behind; repairing those is out of scope here. This revision supersedes the previous revision's counts: 39 new tests (not 34), a 27-mutant matrix (not 16), full suite 2972 passed (not 2967). It also corrects a claim that revision made: Fix 8's fail-closed raise is NOT unreachable through a supported path. get_or_create_category filters on an exact name with no unique index, while nerve's init rebuild keys on name.lower(), so two categories differing only in case leave the displaced id absent from that map and an omitted-category update on an item linked to it raises. The behaviour is still correct - base silently unlinks the item where this raises and changes nothing - but it is reachable, not unreachable. Every claim above is backed by a control arm that drives memU's own unpatched function, so a test that passes without the fix is a failed test rather than a passing fix. Each mutant in the matrix kills at least one test, with a green no-op control and zero vacuous mutations.
Three defects in the memu-py SQLite backend, each measured on the live store: 1. A content-only memory_update unlinks EVERY category. Omitting `categories` passes None, which memU maps to [] and diffs as "remove all". 154 of 154 records updated without a categories argument now hold zero links - about 47% of every orphan in the store. 2. memory_update never recomputes extra.content_hash, which create_item_reinforce dedups on, so an updated record can never again be recognised as a duplicate: 149 of 149 updated items are hash-stale against a 400/400 fresh baseline. 3. memory_delete orphans category relations: there is no FK and no ON DELETE CASCADE, and no layer owns the dependent rows. 6,455 dangling relations, every one of the 5,611 distinct item_ids present in the item_deleted log. A fourth defect surfaced while measuring the second: the semantic-dedup writeback seeded `extra` from the ITEM CACHE and assigned it over the row's whole `extra`. Read paths build MemoryItem without extra=, so a reinforce through a cold cache deleted content_hash and every key another writer had added. This explains the hashless population (all rc > 1, none rc == 1, and the store holds 0 items of the one type that legitimately writes no hash). All four are fixed in _patch_sqlite_bugs(), the established seam for this class of memu-py defect - same tool and same layer as ClickHouse#119. The hash refresh reads the row memU's write LEFT BEHIND rather than a pre-read snapshot, and writes back through a single conditional UPDATE that is a no-op unless summary and memory_type still hold what was just written. A snapshot taken before the delegation closes its session before memU's write commits, so a writer outside the memU loop thread (the date sweep runs on _blocking_pool with its own connection; `nerve memory` is a second process) could otherwise leave a hash of text the row does not hold. When the conditional write declines, the cache is left alone rather than claiming a refresh that never landed. Scope: write paths only. No existing data is repaired, and the 154 lost memberships are not recoverable - the audit log records only `categories_changed: false`, never the ids. An absent-row delete issues no DB write at all, so a vanished item's own relation rows stay behind; repairing those is out of scope here. This revision supersedes the previous revision's counts: 44 new tests (not 39), a 29-mutant matrix (not 27), full suite 2977 passed (not 2972). It also corrects a claim that revision made: Fix 8's fail-closed raise is NOT unreachable through a supported path. get_or_create_category filters on an exact name with no unique index, while nerve's init rebuild keys on name.lower(), so two categories differing only in case leave the displaced id absent from that map and an omitted-category update on an item linked to it raises. The behaviour is still correct - base silently unlinks the item where this raises and changes nothing - but it is reachable, not unreachable. The hash refresh is derived work and is best-effort: it runs after memU's content write has committed, so a failure there (a lock, say) leaves the row with a stale hash and a WARNING in the log rather than failing the update. That is base's unconditional behaviour, whereas letting the exception escape would report failure for an update that had already landed - with the categories undiffed, since the handler diffs them after update_item returns, and the vector index left on the old embedding. memU's own update call stays outside that guard, so a genuine update failure still propagates. One residual window is known and pinned by a test rather than claimed closed: category preservation works by rewriting the payload from the links it reads, and memU's handler re-reads them to build its diff, so a link created between those two reads is still removed. Closing it would mean reimplementing that diff, which would bypass category_updates and therefore the category-summary step. Base loses a link in the same race and loses every link when there is no race. Every claim above is backed by a control arm that drives memU's own unpatched function, so a test that passes without the fix is a failed test rather than a passing fix. Each mutant in the matrix kills at least one test, with a green no-op control and zero vacuous mutations.
This revision SUPERSEDES the figures stated in the previous commit message (39 new tests, 26 mutant arms, 2977 passed): the counts below replace them. memU's write paths damage the store in three measured ways. A content-only memory_update unlinks EVERY category (154 of 154 items ever updated without a categories argument held zero links), because memU maps a missing `categories` to [] and so diffs the item's entire current set into cats_to_remove. update_item never recomputes extra.content_hash, so 149 of 149 updated rows carried a hash for text they no longer hold, which silently breaks the hash-dedup create_item_reinforce relies on. delete_item removes the item row and leaves its category relations behind (6,455 dangling). Fix 8 no longer synthesizes a categories payload. An omitted `categories` now performs no membership mutation at all: link/unlink are neutralised on the repo instance for the delegated call and restored in a finally, and the (old, new) pairs the LLM summary step consumes are rebuilt from the links that actually survive. That closes the residual race the earlier snapshot-rewrite design carried (a link inserted between memU's two reads was diffed away; it is now preserved, pinned by a raw-SQL race) and deletes the fail-closed ValueError along with the ctx.category_name_to_id inversion, so an incomplete name map is harmless by construction rather than merely detected. An explicit list still replaces and an explicit [] still clears, assertions byte-unchanged. Fix 9 refreshes the hash under a CAS bound to summary/memory_type, taking the new extra from RETURNING so a concurrent writer's key is never reverted in the cache. The whole phase after memU's content write stays best-effort, since it runs post-commit and must not turn a landed update into a raising call; the swallowed failure is logged, and that log is now asserted rather than assumed. A decode failure of the RETURNING payload no longer returns early: the CAS has already committed by then, so skipping the cache assignments left the cache and the returned item on the old hash while the row held the new one. Only the decode is guarded, and its fallback is the row's content on that path because the CAS bound summary/memory_type and set content_hash alone. Fix 10 deletes the relations and the item in one transaction and prunes both caches, so a failure on either side rolls the other back. Validation: 48 targeted tests over the five write-path classes, 44 of them new; full suite 2981 passed with the 7 failures that reproduce at a clean origin/main export (6 TestResolveEventDatesSync + 1 test_telegram_sessions), failure sets compared by name against the pre-edit baseline. Mutation matrix re-anchored from scratch for this revision and re-run whole (nothing carried, because Fix 8 was rewritten and the Fix 9 decode moved): 36 arms, 34 killed each naming at least one failing test, 0 vacuous, and the only survivors are the two no-op controls, green at both ends. Two Fix 8 arms lost their subject in the rewrite (MI_fix8_fail_open has no raise to open, MG_fix8_mutates_ctx has no ctx read) and are recorded as retired with the reason; equivalents were substituted for the properties that remain, including an arm restoring the old snapshot shape, which the retargeted residual test kills. link_item_category is measurably unreachable on the omitted-categories path today, so stubbing it changes no outcome; it is stubbed anyway so that "no membership mutation" holds by construction, and a test observes that where it is made rather than by outcome. ruff over both changed files is back to the 2 findings present at origin/main, measured at every revision of this branch in one session.
This revision SUPERSEDES the figures stated in the previous commit message (48 targeted tests, 44 new, 36 mutant arms, 2981 passed): the counts below replace them, and Fix 8's mechanism is replaced for the second time. memU's write paths damage the store in three measured ways. A content-only memory_update unlinks EVERY category (154 of 154 items ever updated without a categories argument held zero links), because memU maps a missing `categories` to [] and so diffs the item's entire current set into cats_to_remove. update_item never recomputes extra.content_hash, so 149 of 149 updated rows carried a hash for text they no longer hold, which silently breaks the hash-dedup create_item_reinforce relies on. delete_item removes the item row and leaves its category relations behind (6,455 dangling). Fix 8 neutralises membership for ONE CALL, on a per-call proxy, instead of stubbing the repo instance. The previous revision assigned no-op link/unlink onto store.category_item_repo and restored them in a finally. That repo is a process-wide singleton, the delegated handler awaits an embedding call inside the window, and nothing serializes memU calls onto its single loop, so the no-ops were in force for every other coroutine for the duration of the await: a concurrent membership write from any other pipeline was silently swallowed. The restore was also not reentrant, so two overlapping updates could leak both no-ops permanently. Now a relation-repo proxy whose two mutators are no-ops is built inside the call and reached only through the `store` that call passes down; the real store is handed back in the returned mapping so the proxy cannot escape into persist_index or build_response. Nothing shared is written, so there is no restore to get wrong. The rebuilt category_updates now keeps only category ids the response step can resolve. memU's _patch_build_response subscripts memory_category_repo.categories unguarded and runs AFTER the content write commits, so an id it cannot resolve raised KeyError post-commit and made bridge.update_item report False for an update that had fully applied. memU could not reach that state (it derived ids through _map_category_names_to_ids); rebuilding from raw relation rows can, and this PR's own Defect 3 is that such rows accumulate. Fix 9 refreshes the hash under a CAS bound to summary/memory_type, taking the new extra from RETURNING so a concurrent writer's key is never reverted in the cache. The whole phase after memU's content write stays best-effort, since it runs post-commit and must not turn a landed update into a raising call; the swallowed failure is logged, and that log is asserted rather than assumed. A decode failure of the RETURNING payload does not return early: the CAS has already committed by then. Only the decode is guarded, and its fallback is the row's content on that path because the CAS bound summary/memory_type and set content_hash alone. Fix 10 deletes the relations and the item in one transaction and prunes both caches, so a failure on either side rolls the other back. The relation DELETE is no longer conditional on the item row still existing: when the row is already gone (another process removed it) its relation rows ARE the dangling rows this fix exists to prevent. Deleting the item stays conditional, and an unknown id remains a silent no-op because the relation DELETE then matches nothing. Validation: 51 targeted tests over the five write-path classes, all 51 new (none of those five classes exists at origin/main; counted three ways -- added test defs per class, pytest's own selection count, and the classes' absence at the base -- all agreeing). The previous revision's "44 of them new" understated this. Full suite 2984 passed with the 7 failures that reproduce at a clean origin/main export (6 TestResolveEventDatesSync + 1 test_telegram_sessions), failure sets compared by name against the pre-edit baseline and identical. The +3 selection delta is exactly 5 new cases minus the 2 whose subject (instance stubs) no longer exists; those two are deleted, not skipped. Mutation matrix re-anchored from scratch and re-run whole, nothing carried: 38 arms, 35 killed each naming at least one failing test, 0 vacuous, the two no-op controls green at both ends. Three arms lost their subject with the stubbing and are recorded as retired with a named successor each. Two arms survive and both are unobservable by construction, measured rather than assumed: MN6_proxy_only_unlink drops the proxy's link override, and link_item_category is unreachable on this path (with categories omitted mapped_new_cat_ids is [], so cats_to_add is empty); MN10_known_read_off_proxy reads the categories mapping off the proxy, which forwards it to the real store by identity (memory_category_repo is the real object, only category_item_repo is overridden). Two inherited mutant arms were found to be MIS-SITED against this tree and are re-anchored. Both anchors began with whitespace and were applied by substring replacement, so a hit count of 1 did not prove where they landed: MY_residual_window_closed matched an indented explicit-list early return instead of the delegation, splicing its payload into the wrong branch, which means its kill in the previous revision was vacuous; MD_fix9_substitute_extra matched a more deeply indented line inside a try (same statement and same effect, so it still killed, but the siting was not proof). The mutator now asserts that every anchor match begins at a line boundary, and that assertion catches both original anchors while passing all 38 current arms. ruff over both changed files is back to the 2 findings present at origin/main, measured at both revisions in one session.
This revision SUPERSEDES the figures stated in the previous commit message
(53 targeted tests / 53 new, 41 mutant arms, 38 killed): the counts below
replace them. It also REVERTS that revision's Fix 7 ghost-eviction change --
see "Reverted in this revision" below. Fixes 8, 9 and 10 are unchanged.
memU's write paths damage the store in three measured ways. A content-only
memory_update unlinks EVERY category (154 of 154 items ever updated without a
categories argument held zero links), because memU maps a missing `categories`
to [] and so diffs the item's entire current set into cats_to_remove.
update_item never recomputes extra.content_hash, so 149 of 149 updated rows
carried a hash for text they no longer hold, which silently breaks the
hash-dedup create_item_reinforce relies on. delete_item removes the item row
and leaves its category relations behind (6,455 dangling).
Fix 8 neutralises membership for ONE CALL, on a per-call proxy, instead of
stubbing the repo instance. That repo is a process-wide singleton, the delegated
handler awaits an embedding call inside the window, and nothing serializes memU
calls onto its single loop, so instance-level no-ops were in force for every
other coroutine for the duration of the await. Now a relation-repo proxy whose
two mutators are no-ops is built inside the call and reached only through the
`store` that call passes down; the real store is handed back in the returned
mapping so the proxy cannot escape into persist_index or build_response. The
rebuilt category_updates keeps only category ids the response step can resolve,
because memU's _patch_build_response subscripts memory_category_repo.categories
unguarded AFTER the content write commits.
Fix 9 refreshes the hash under a CAS bound to summary/memory_type, taking the
new extra from RETURNING so a concurrent writer's key is never reverted in the
cache. The whole phase after memU's content write stays best-effort, since it
runs post-commit and must not turn a landed update into a raising call; the
swallowed failure is logged, and that log is asserted rather than assumed.
Fix 10 deletes the relations and the item in one transaction and prunes both
caches, so a failure on either side rolls the other back. The relation DELETE is
no longer conditional on the item row still existing: when the row is already
gone (another process removed it) its relation rows ARE the dangling rows this
fix exists to prevent.
Fix 7's semantic-dedup writeback seeds `extra` from the ROW, not from the cache.
Read paths build MemoryItem without extra=, so a cached extra is {} in any
process that did not itself write the item, and writing that back would drop
content_hash and every key another writer added.
Reverted in this revision: the previous revision also made that writeback evict
the cache and vector-index entry when the matched row was gone, instead of
completing a phantom reinforce. That remedy is already open, and semantically
identical, as PR ClickHouse#248 ("memory: do not report a semantic reinforce as successful
when the row is gone", created 2026-08-03T12:49:42Z, over three hours before the
review round that asked for it here). Both pop the id from self.items, remove it
from the index, resync seen_items_len, and fall through to the real create path
with a WARNING. Carrying it in two of our own open PRs means whichever merges
second must be resolved by hand, so this half is reverted and ClickHouse#248 carries the
fix and its five tests. `_semantic_sqlite_reinforce` and its in-memory sibling
are now byte-identical to the previous revision's parent (verified by AST
extraction: 57 and 36 lines, both exact; the same comparison against the
previous revision reports 57 vs 76, so it discriminates). No fix is lost.
Item 8's docstring summary also described the deleted payload-rewrite design
("rewrite the payload with the names of the item's current links"), which is the
race-prone mechanism an earlier revision removed; it now describes the per-call
proxy. A file-wide grep confirms no other prose describes the deleted mechanism.
Validation: 52 targeted tests over the five write-path classes, all 52 new (none
of those five classes exists at origin/main -- grepped, 0 hits each). Counted
three ways, all agreeing: 14 + 19 + 6 + 7 + 6 test defs per class; 128 collected
here against 76 at a clean origin/main export; 52 added `def test_` lines in the
diff. This revision removes one case (the ghost-match assertion, which tests
ClickHouse#248's contract) and keeps the case pinning, rather than closing, the window in
which a writer landing between memU's content write and Fix 9's post-read leaves
the cache entry and returned item carrying the row's hash over the caller's
summary. That window is harmless because the only SQLite-path consumer of
extra.content_hash queries the DB column, which the test asserts by re-deduping
on the row's own text; closing it means making the two writes atomic, which was
built in an earlier round and measured to store a WRONG hash on two correctness
paths.
Full suite 2985 passed with the 7 failures that reproduce at a clean origin/main
export (6 TestResolveEventDatesSync + 1 test_telegram_sessions); failure sets
compared BY NAME against this revision's own pre-edit baseline through the same
runner, diff empty. The 2986 -> 2985 delta is exactly the removed case. The kept
test also passes ALONE, the order-independence control that matters for a suite
whose fixtures patch process-global state.
Mutation matrix: 38 live arms = 35 killed (each naming at least one failing
test) + the 2 disclosed survivors below + the no-op control. 42 invocations: the
38 live arms, the 3 retired-with-cause arms, and the control repeated at the end.
0 vacuous, both no-op control arms green at both ends (52 passed), tree restored
byte-exactly after every arm (42 TREE_RESTORED_OK lines). Nothing was carried
blindly: the arms are imported from the r5 mutator (one source, not a retyped
copy) and every anchor is asserted to resolve exactly once against this tree
before the run (38/38, bad=0). The three arms the previous revision added for the
eviction branch are RETIRED WITH CAUSE, printed by name at run time rather than
dropped silently: their subject no longer exists in this PR, and their coverage
now lives in ClickHouse#248, whose tests assert both the cache and the index eviction. Two
arms the previous revision had re-anchored are de-re-anchored: the revert
restores their block byte-identically, so the r5 anchors are used verbatim. The
harness additionally asserts NEGATIVE markers -- the reverted text must be ABSENT
from every export -- so no arm can silently re-test the pre-revert code.
The two survivors are unchanged and were RE-MEASURED on this tree rather than
carried. MN6_proxy_only_unlink drops the proxy's link override, and
link_item_category is unreachable on this path: instrumented live,
_map_category_names_to_ids is called once with None and returns [], so
cats_to_add is empty and memU's link loop never runs.
MN10_known_read_off_proxy reads the categories mapping off the proxy, which
forwards it by identity: observed from inside the delegated call,
memory_category_repo is the real repo and .categories is the same object, while
category_item_repo is the one override.
Known residual, unchanged by this PR: this branch still conflicts with ClickHouse#248 in
both files, and did so before the reverted change existed (measured at every
commit on the branch: clean at the merge base, conflicting from the first
commit onward). The overlap is the surrounding Fix 7 `extra`-seeding edit, not
the reverted eviction. Not resolved here, because ClickHouse#248 is unmerged and stacking
on an unmerged prerequisite is its own hazard.
ruff over both changed files reports exactly the 2 findings present at
origin/main, measured at both revisions in one session. Non-ASCII in this
revision's added lines: 0.
Contributor
Author
Internal second-model review - 9 Gate B rounds, 55 findings adjudicatedEvery change here was reviewed by a second model with no access to my reasoning, then Blockers found and fixed
Disagreed, with evidence
Reversals and corrections I made to my own work
Validation posture52 targeted tests over five write-path classes, none of which exists at the base. A 38-arm Known residual, disclosed rather than resolved: this branch conflicts with #248 in both Session id: cron:clickhouse-review-slot-11:20260803-181700 |
Contributor
Author
Pre-PR validation gatea-i self-check, with the measurement behind each answer
Also checked: the vector-index hook stays outermost and still fires; repeating |
|
|
Contributor
Author
|
cc @pufit @serxa — could you review this? It patches three memU write paths in |
This was referenced Aug 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Symptom
Measured on a live 139,187-item store.
memory_update,memory_deleteand the web-UIroutes all reach these.
categoriesargument now hold zero categorylinks - about 47% of all 326 orphaned items.
content_hash(400/400 never-updateditems are fresh), so re-memorizing an item's pre-correction wording reinforces the corrected
row rather than creating a new one.
memu_category_itemsrows, all 5,611 distinctitem_ids present in theitem_deletedaudit log; they inflatememory_expand_category'stotalby 3.5-4.6% percategory (relations counted, items listed through a JOIN).
Root cause
categoriesbecomesNone, and_map_category_names_to_ids(None, ctx)returns[], so_patch_update_memory_item'scats_to_removeis the item's entire current set, each unlink recording(old_content, None)- read downstream as "discarded", so the LLM drops the item from thecategory summary too.
content_hashis a derived field with no invalidation:update_itemrewritessummary/memory_typeand never touches it, whilecreate_item_reinforcededups on it.delete_itemdeletes only the item row; there is noFK or cascade.
The change
Four patches in
_patch_sqlite_bugs(), the established seam for this class of memu-py defect(precedent: #119).
categoriesisNone, neutralise membership for that one call: thedelegated handler gets a per-call proxy whose
link/unlinkare no-ops, so its diff removesnothing, and
(old, new)is rebuilt from the surviving links. Nothing process-wide ismutated. Explicit list still replaces; explicit
[]still clears.content_hashfrom the DB row whensummary/memory_typechanges. Only an existing hash is refreshed.
leave a surviving item stripped of its links. Installed before Fix 3 so its vector-index hook
still wraps it.
extraover the row's, andread paths omit
extra=, so a reinforce through a cold cache deletedcontent_hashand anykey another writer had added. It now seeds
extrafrom the row.Validation
52 new tests. Each defect has a control arm driving memU's own unpatched function against an
identically-built fixture, so a test passing without the fix is a failed test, not a passing
fix. A 38-arm mutation matrix kills 35, each naming a failing test, no-op controls green at both
ends; the 2 survivors are unreachable by construction, measured. Full suite 2985 passed,
pre-existing failure set unchanged by name.
Scope, unfixed siblings, behaviour changes
Write paths only - no data is repaired. The store still holds 6,455 dangling relations, 326
orphans, 149 stale hashes and 6,473 hashless rows; the 154 lost memberships are not
recoverable (the audit log records
categories_changed: false, not the ids).The Fix 7 wipe explains that population: all 6,473 have
rc > 1and none is atoolitem, theonly create path that legitimately writes no hash.
Both fixes read the row, not the cache: a cached item has
extra == {}, so a cache-based refreshrefreshes nothing in a long-lived process.
Pinned, not closed. Fix 9's CAS binds
summary/memory_type, so any hash it writes is therow's own, but the refresh is best-effort: if skipped (a lock) the row keeps its old hash, as
base always does - logged and asserted. A writer landing between memU's write and this read can
likewise leave the cache entry on that writer's hash, unread (dedup reads the DB). Fix 7 keeps
memU's read-modify-write shape, so a key written inside it is still lost (base loses every key).
All pinned by tests; a CAS is tracked apart.
Named but deliberately unfixed. Test-pinned so the exemption cannot rot: read paths still
omit
extra=(no longer destructive, so hydration is its own change);clear_itemshas the samedangling shape but no nerve callers;
PatchMixinis dead code. Unreachable and untested:the in-memory/postgres siblings (provider hardcoded
sqlite).Behaviour changes. A reinforce preserves more of
extra, includingref_id, whichlist_items_by_ref_idsfilters on. Hash-dedup now matches rows whose hash used to be wiped, sosome memorizations reinforce rather than duplicate (the intent). Recomputing the 149 stale
hashes collides with none of the 132,714 hashed rows; updating an item's text to exactly match
another's now yields two rows sharing a hash, handled as the dedup lookup handles two
identical
memorizecalls.