memory: do not report a semantic reinforce as successful when the row is gone

[oranjeai]

Problem

MemUBridge._patch_sqlite_bugs()'s _semantic_sqlite_reinforce decides from the
in-memory cache but writes to the database, and its return matched sat
outside the if row: write guard. If another process deleted the matched row,
the lookup returned None, the write was correctly skipped, and the cached object's
reinforcement_count was still bumped and returned.

memU reads that as "already persisted" (memu/app/memorize.py:614: if reinforce and item.extra.get("reinforcement_count", 1) > 1: continue) and skips creation and
category linking, so the memory is silently dropped, nothing raised or logged,
and the un-evicted stale entry stayed a dedup magnet swallowing every later similar
memorize.

Measured at main (94406ea), reinforcing similar text after deleting the row over
a second connection: returned id == the deleted id, count 2, rows in DB
0, dead id still served by repo.items, the index and list_items().

Fix

An absent row is a cache-invalidation signal, not an authoritative match. Track
whether the write committed; on the stale path evict from self.items and the vector
index (resyncing seen_items_len, the two operations _indexed_delete_item performs)
and fall through to the real create_item_reinforce, which creates and returns a
persisted row. The success early-return now sits inside the row-present branch, so it
fires only when something was written. The index handle is bound once for search
and remove, since _vec_index_for rebuilds on cache-size drift and re-fetching it
after the pop would force an O(n) rebuild on a write path.

_semantic_inmemory_reinforce has the same shape but is unreachable (provider
hardcoded to sqlite), so it is unchanged. The dangling memu_category_items row is
the external deleter's, out of scope here.

Tests

TestSemanticReinforceStaleCacheHit in tests/test_memu_bridge.py (5 cases): a
stale hit creates a new persisted row; cache and index entries are evicted; the
returned item is always SELECT-able; the new item stays visible to later dedup;
plus a row-present control, byte-identical on both trees.

The 4 stale-path tests fail at main, pass with the fix. Whole repo suite
7 failed / 2938 passed vs 7 / 2933 at main: failure sets identical by name
(all pre-existing), passes +5 = exactly the new tests. Six mutants are each killed
by a new test; two initially survived and the tests were strengthened. Both ruff
findings here are byte-identical at main; memu_bridge.py is ruff-clean.

Third commit, test hygiene only. The helper called _patch_sqlite_bugs() per test,
and 8 of the 17 attributes it reassigns wrap the value they replace, none guarded, so
the create_item_reinforce chain reached 6 deep for the rest of the pytest
process. Moving the call into the existing _models_cache memo takes it to 2,
patch still installed. No production guard is added: :1028 asserts unconditional
re-wrapping.

[CLAassistant]

All committers have signed the CLA.

[oranjeai]

Pre-PR validation gate (click to expand)

#	Question	Answer
a	Deterministic repro?	Yes, 100% deterministic, no sampling. A harness seeds one item, deletes its row over a second sqlite3 connection, then reinforces near-identical text (cosine ~0.999, threshold 0.85). At main 94406ea: returned id == the deleted id, count 2, item rows in DB 0. Also as 5 pytest cases: pytest tests/test_memu_bridge.py -k SemanticReinforceStale. Temp DBs only; the live store is never opened.
b	Root cause explained?	Yes. _semantic_sqlite_reinforce treats the in-memory cache as authoritative for the dedup decision but the DB for the write, and never reconciles a disagreement. The if row: guard correctly declines to write when another connection deleted the row, but return matched sits outside it, so only the cached object is mutated (count 1 -> 2) and success is reported. memu/app/memorize.py:614 reads count > 1 as "already persisted" and skips create + category linking, so the memory is dropped. The stale entry is never evicted, so it stays a permanent dedup magnet.
c	Fix matches root cause?	Yes. An absent row is treated as a cache-invalidation signal: evict from self.items and the vector index, then fall through to the real create_item_reinforce so the memory is actually stored; the success return moves inside the row-present branch. Not a band-aid: nothing is re-inserted, nothing raises, no bound is widened, no path disabled. Four alternatives (re-INSERT the deleted row, return None, raise, retry loop) were considered and rejected with reasons.
d	Test intent preserved / new tests added?	Yes. No existing test modified or weakened: the test-file diff is +219/-0, append-only. 5 new regression tests in TestSemanticReinforceStaleCacheHit, all mutation-verified.
e	Both directions demonstrated?	Yes. Base arm is a git archive origin/main export (sha-verified against the blob). Base: id == deleted id, count 2, 0 rows, dead id still in cache/index/list_items(). Fixed: different id, count 1, 1 row, all three residues gone. The 4 stale-path tests fail at base and pass with the fix. The row-present control is byte-identical on both trees (same id, count 2, 1 row, same DB extra).
f	Fix is general across code paths?	Yes, enumerated. _semantic_sqlite_reinforce fixed. Sibling _semantic_inmemory_reinforce has the same shape but is unreachable (provider hardcoded to sqlite, no config knob), so it is stated rather than edited. Cache and vector index both evicted; list_items() (which serves the cache unfiltered) is fixed transitively and asserted. _indexed_delete_item was already correct and is the eviction idiom reused. memu/app/memorize.py:614 is third-party and correct given an honest count, so it is not patched: the fix makes its premise true. The vanished item's dangling memu_category_items row belongs to the external deleter and is out of scope.
g	Fix generalizes across inputs (params/datatypes/wrappers)?	Yes. The change is control-flow only, so it is type- and value-agnostic: it adds no parsing, arithmetic or size assumption, and applies to any memory_type and any embedding above the threshold. Boundaries checked: an empty cache short-circuits earlier, and no-hit / below-threshold cases bypass the block unchanged. self.items.pop(..., None) and _VectorIndex.remove are both idempotent and safe when the id is absent, so a repeated eviction cannot raise.
h	Backward compatible? (maintainer-approved exception only)	Yes. No schema change, no serialization-format change, no new setting, no migration (this repo has no SettingsChangesHistory analogue). Behaviour changes only on the stale-row branch, which previously lost data silently.
i	Invariants and contracts preserved?	Yes. Establishes a returned reinforced item always corresponds to a row that exists and was written, asserted directly by one of the new tests. The return contract is unchanged (still a MemoryItem, never None, never raising), so the create_item caller is unaffected. Cache/index coherence is restored rather than broken: the seen_items_len resync is load-bearing, proven by a mutant showing that without it no index rebuild ever fires and the newly created item becomes invisible to dedup, inserting a duplicate row. The DB session is a plain SQLModel Session, so returning from inside its context manager leaves no transaction open, and the write path itself is behaviourally byte-identical (control arm).

Regression, compared by failure name rather than count: whole repo suite 7 failed / 2938 passed vs 7 failed / 2933 passed at main; failure sets identical by name (all pre-existing), passes +5 = exactly the new tests. Both ruff findings are byte-identical at main (same file, lines 11 and 646); the changed source file is ruff-clean.

Mutation matrix, since a green suite is not coverage: 6 mutants (cache-evict removed, index remove removed, seen_items_len resync removed, return matched moved back outside the guard, stale path returning matched, row_written forced true), all killed, 0 vacuous, with an unmutated no-op control green at both ends of the run and the tree verified restored. Two mutants initially survived and the tests were strengthened rather than the mutants weakened: reading the index through _vec_index_for masked a missing remove because it rebuilds on cache-size drift (the assertion now uses the no-build accessor), and the seen_items_len mutant needed a new test asserting that a later similar reinforce still dedups onto the created item.

50/50 randomized runs: not applicable, this is a Python unit-test repo with no --random-settings runner.

Session id: cron:clickhouse-impl-slot-43:20260803-115000

[oranjeai]

Internal second-model review (click to expand)

Before opening this PR I reviewed the resulting code cold, then had an independent model
(codex) re-review the full diff against the PR description as a contract. Findings from both
passes, with my adjudication.

Outcome: 0 blockers, 0 majors, 3 nits. Nothing was found that changes the fix. The three
nits are listed below rather than silently dropped; two are real and are being addressed
separately, one I disagreed with and say why.

#	Finding	Severity	Verdict
1	Class docstring of the new test class repeats the PR description's problem narrative	💡 nit	AGREE, noted not blocking
2	The new test helper calls _patch_sqlite_bugs() per test without the snapshot/restore this file uses elsewhere	💡 nit	AGREE, noted not blocking
3	matched = self.items[match_id] is an unguarded dict subscript	💡 nit	DISAGREE (pre-existing and not widened here)

1 (from the independent pass). Correct and measured: the docstring is 15 lines / 122
words, the longest of the file's 17 test classes (9 are one line, 4 have none, the next
largest is 10 lines / 71 words), and it restates the description above. It is being condensed
in a follow-up rather than here: the round found no blocker or major, so bouncing the branch
to rewrite a docstring would cost a full re-review of unchanged logic. No assertion, test name
or oracle is affected.

2 (from my own pass). _semantic_reinforce_store() calls MemUBridge._patch_sqlite_bugs()
once per test, and that function has no idempotence guard for the reinforce and list_items
patches, so each call captures the already-patched method and re-wraps it. The existing test at
tests/test_memu_bridge.py:1016-1045 snapshots exactly those method names and restores them in
a finally for this reason, so the new class deviates from the file's own convention. Measured
blast radius is nil today: no other test module touches these symbols, that
restore-convention test sits earlier in the file, whole-repo failure sets are identical by
name, and the nesting is behaviourally inert because once the outer level evicts the inner
level finds no hit and falls through. Same follow-up as (1).

3 (from my own pass, disagreed). memu_bridge.py:1207 subscripts self.items with an id
sourced from the vector index. It is byte-identical at main, so pre-existing -- but that alone
would not excuse it if this change touched the invariant, so I checked whether the fix widens
it. It does not: the eviction pops the entry and resyncs seen_items_len to the post-pop
length, so the subsequent create leaves seen_items_len != len(items) and the next lookup
rebuilds the index from self.items. The index therefore cannot end up holding an id the cache
lacks by way of this code. Out of scope for the invariant this PR establishes.

Independent-review cost for this PR: $3.67 over 1 round (Gate A + Gate B combined: $6.09).

[oranjeai]

Internal second-model review (third commit) (click to expand)

The third commit (8f382d0, test hygiene) went through the same two-pass review as the
fix: I reviewed the resulting code cold, then an independent model (codex) re-reviewed
the full diff against the description as a contract. Findings from both passes, with my
adjudication.

Outcome: 0 blockers, 0 majors. The independent pass returned 0 findings. My own
pass found 5 nits, all in the PR's own prose rather than in the code; 4 are already fixed
in this push and 1 is listed below unfixed.

#	Finding	Severity	Verdict
1	The nesting figure "8 of the 14 attributes" does not hold under any one partition	💡 nit	AGREE, fixed in the description
2	The refreshed description dropped four facts the previous one stated	💡 nit	AGREE, fixed
3	The description draft began with the PR title as body text	💡 nit	AGREE, fixed
4	Same wrong figure as (1) in the validation-gate comment	💡 nit	AGREE, fixed
5	The validation-gate comment inverted "2 directly, 3 via the helper"	💡 nit	AGREE, fixed
-	The commit message still carries the figure from (1)	💡 nit	AGREE, left unfixed, see below
-	matched = self.items[match_id] unguarded subscript	💡 nit	DISAGREE (pre-existing, not widened)

1 (and 4). The commit message and both comments said "8 of the 14 attributes
_patch_sqlite_bugs() reassigns install a wrapper that calls the value it replaced". I
re-derived that census by AST, following the same free-variable rule the depth harness
uses. There are 17 attribute-install statements at 17 distinct targets (15 distinct
attribute names), of which exactly 8 nest: 7 through an _original_* capture, plus
sqlite_models._merge_models, which nests through a re-imported free variable of the same
name. The denominator 14 counts class attributes only, which excludes the 3 module
attributes -- and the 8th nester is one of those 3, so under that partition the count is 7.
So the numerator is right and the pair is off by one member: it is 8-of-17, or 7-of-14,
never 8-of-14. The description and the validation-gate comment now say 8 of 17. No
measurement depends on the count: the depth figures (6 -> 2) come from walking the live
wrapper chain, not from counting symbols.

The commit message is left as-is, with "Eight of the fourteen". Amending it would move
the tree and invalidate the independent review that just passed on this exact content, for
a prose correction that is already right on both surfaces a reviewer reads. Flagged here so
nobody has to rediscover it.

2 and 3. Refreshing the description for the third commit had rewritten it rather than
extended it, which silently dropped four things the published version said: that the
if row: guard correctly declined to write; that the vanished item's dangling
memu_category_items row is the external deleter's and out of scope; the ruff
byte-identity statement; and that the row-present control is byte-identical on both trees.
It had also narrowed the stale-residue list to repo.items and the index, while the test
asserts list_items() too. The description was rebuilt from the published text with a
checklist asserting all 42 load-bearing facts, citations and figures survive; the
third-commit paragraph is funded by condensing prose, not by dropping facts. The draft also
started with the PR title as a body paragraph, which would have published a duplicate title.

5. Re-checking every remaining figure in the validation-gate comment (a wrong figure
means the others are unverified, not that they are fine) turned up one more: it said the 5
tests reach the helper "2 directly, 3 via _seed_then_delete_externally". It is the
reverse -- 4 tests go through the helper and exactly 1 calls it directly. The point being
made, that all 5 exercise the changed line, is unaffected. Every other figure re-derived
clean: the docstring metrics (13 non-blank lines / 122 words -> 2 / 24), the 17 unchanged
class docstrings by sha256, "5 of the 8 nesting symbols" covered by the existing
snapshot idiom, and the two failure-name sets being identical.

The unguarded subscript (memu_bridge.py:1207) keeps the verdict it got last round.
It is byte-identical at main, and I re-verified that this change cannot widen it: the
eviction pops the entry and resyncs seen_items_len to the post-pop length, so the
following create leaves seen_items_len != len(items) and the next lookup rebuilds the
index from self.items, meaning the index cannot end up holding an id the cache lacks by
way of this code.

One thing worth stating plainly about the code itself: reverting the moved call leaves the
whole suite green, so no test guards this commit. That is deliberate -- asserting a
closure depth would pin _patch_sqlite_bugs's wrapper style -- and it is why the recorded
oracle is the measured chain depth rather than a passing suite. A production idempotence
guard stays out of scope because the test at :1028 asserts that repatching does re-wrap.

Independent-review cost for the third commit: $9.41 over 1 round (this PR total, Gate A +
Gate B: $18.20).

[oranjeai]

Pre-PR validation gate (third commit: test hygiene)

#	check	answer
a	Deterministic repro?	Yes. python depth.py <clone> instruments _patch_sqlite_bugs, runs the 5-test class in-process, then walks the wrapper chain of SQLiteMemoryItemRepo.create_item_reinforce through every function-valued free variable: PATCH_CALLS=5, DEPTH=6 before, reproduced identically on 4 independent arms. Not statistical.
b	Root cause explained?	The helper called _patch_sqlite_bugs() on every invocation, i.e. once per test. 8 of the 17 attributes it reassigns install a wrapper closing over the value it replaced, and none of the 8 has an idempotence guard (the only guard, _nerve_numpy_wrapped, protects create_item inside _initialize_impl), so each extra call added a layer and the nesting persisted for the pytest process.
c	Fix matches the root cause?	Yes -- it removes the repeat invocation at the only site that repeats, reusing the helper's existing _models_cache memo. The model build is itself one of the patched symbols and the shipped code already ordered patch-then-build, so "models cached" already implied "patch applied". Nothing widened, disabled, tagged or guarded away.
d	Test intent preserved / tests added?	Preserved: no assertion, oracle, EMB_* constant or test name touched. _vec_index_note deliberately kept -- _vec_index_for rebuilds on cache-size drift, which would make the eviction assert vacuous. No new test: asserting closure depth would pin _patch_sqlite_bugs's wrapper style.
e	Both directions?	PATCH_CALLS 5 -> 1, DEPTH 6 -> 2, INSTALLED True both ways, class 5 passed both ways. INSTALLED=True is load-bearing: a memo skipping the first call would give depth 1 and silently unpatch everything. Two mutants: M1 (revert the move) -> depth 6; M2 (drop the call) -> depth 1, INSTALLED=False, 5 failed. Unmutated controls green at both ends of the matrix, tree hash restored after every arm. M1 leaves the suite green, so the depth measurement -- not a green suite -- is the oracle.
f	General across code paths?	All three _patch_sqlite_bugs() call sites enumerated by grep: the convention test at :1028 (byte-unchanged -- it asserts unconditional re-wrapping), the helper (changed), and _initialize_impl in memu_bridge.py (unchanged, no production file touched). All 8 nesting symbols stop nesting because the repeat invocation is what is removed, not a per-symbol guard. Non-restorable mutations (model_fields.pop, the embedding property) are not method reassignments and cannot nest.
g	Generalizes across inputs?	No input surface: the change is the position of one existing no-arg call plus prose -- no signature, parameter or type-wrapper matrix exists. All 5 tests in the class reach the helper (1 directly, 4 via _seed_then_delete_externally), so all 5 exercise it.
h	Backward compatible?	Nothing to be compatible with: test-only, no setting, no serialization format, no migration.
i	Invariants preserved?	The helper's contract is "return a live store whose repos are patched". INSTALLED=True plus 5/5 green prove the patch is applied on the first call and still in force on every later one. The :1016 snapshot/restore idiom is deliberately not reused inside the helper: it returns a live store to the caller, so a finally restore would unpatch the very repo under test, and it covers only 5 of the 8 nesting symbols.

Suites by name, not count: tests/test_memu_bridge.py 6 failed / 75 passed and whole repo 7 failed / 2938 passed, failure sets identical before and after (diff empty; all pre-existing timezone artifacts). Pass count unchanged at 2938 -- no test added or removed. Docstring re-measured by AST: 13 non-blank lines / 122 words -> 2 / 24, no longer the file's longest, and the other 17 class docstrings byte-identical by sha256. 0 non-ASCII in the added lines; the 2 pre-existing ruff findings and the file's pre-existing non-ASCII lines are byte-identical to main and untouched.

[oranjeai]

Closing per @pufit's directive on #247: memU is being rewritten and sunset, and Nerve fixes
outside "critical performance problem" or "makes my work easier" are handled by the Nerve team.
This PR is a correctness fix in neither category, so it is closed unmerged. The analysis stays in
the description and comments if it is useful during the rewrite. No further action needed from me.