memory: merge the content-hash reinforce's extra write into the live row - #255
memory: merge the content-hash reinforce's extra write into the live row#255oranjeai wants to merge 2 commits into
Conversation
memu-py 1.4.0's SQLiteMemoryItemRepo.create_item_reinforce reinforces an existing item by reading `extra`, mutating a dict copy and flushing the whole JSON column back. A plain SELECT takes no SQLite write reservation, so every key another connection commits between the read and the flush is silently replaced. The event-date sweep's `mentioned_at` is such a key, and 145,995 of 149,964 rows in a live store carry it while 17,935 have reinforcement_count > 1, so the two writers genuinely co-occur. Compute the merge server-side with json_set and read it back inside the same still-open write transaction. json_extract on the same column in the same statement expresses count = count + 1 without a snapshot, and the UPDATE promotes the session to a write transaction so the read-back cannot observe an interleaved write. Two details are load-bearing rather than defensive: - content_hash has no uniqueness index (the only indexes on the table are the id PK autoindex and ix_memu_memory_items_id), so a bare hash-filtered UPDATE would bump every duplicate where upstream's .first() reinforces exactly one. The arm resolves one target id with limit(1) and updates by id. Measured on a 3-row duplicate set: 1 row bumped, and it is the row .first() picks. - NULLIF is reachable, not paranoia: a writer can blank the target's `extra` between the id resolve and the UPDATE, and coalesce alone would then feed '' to json_set, which raises "malformed JSON". The wrapper is installed before the existing semantic-dedup patch binds the original, so the order stays semantic -> this arm -> upstream create. A hash-first arm would change dedup precedence: with a fixture holding the query's text under an orthogonal embedding and unrelated text under the query's embedding, semantic-first reinforces the semantic match while hash-first reinforces the other row. `update_item` carries the same defective shape and is deliberately left alone: its only extra-writing caller is gated behind enable_item_references, which defaults False and has no environment or nerve config source, nerve's own memory_update path passes extra=None so the column is never written at all, and 0 of 149,964 live rows carry any key that path would write. Measured both directions with the concurrent write injected inside the window: unpatched loses `mentioned_at`, patched keeps it, and both reach reinforcement_count 2. Nine new tests in tests/test_memu_bridge.py cover the window (row, returned item and cache), the NULLIF window, an increment from a seeded count of 7, a row deleted inside the window, dedup precedence, the duplicate-hash set, the genuine-miss create, the absence of double counting on a semantic hit, and the patch order.
…the window
Review round 1 on the previous commit. Two defects, both in the same two places.
1. The json_set chain set only $.reinforcement_count and $.last_reinforced_at,
so when a writer blanks the already-chosen target's `extra` between the
limit(1) id resolve and the UPDATE, `live` degrades to '{}' and the row is
rewritten WITHOUT content_hash. Measured consequence: the row permanently
stops satisfying the arm's own hash filter, so the next reinforce of the same
text creates a duplicate (rows 1 -> 2). This is introduced by the previous
commit, not pre-existing: upstream rewrote content_hash as a side effect of
the whole-column flush that commit removes, so the value has to be asserted
explicitly. Fixed by one more innermost json_set. The increment still reads
the ORIGINAL live column, so it stays live-valued rather than reading the
rewritten expression.
2. test_preserves_a_concurrent_extra_write injected from a patched Repo._now().
That is correct for upstream, which calls _now() after its entity SELECT, but
this arm calls _now() before it opens its session and before the id resolve,
so the injected write committed BEFORE the arm's first statement. The test
therefore only proved the arm does not clobber a key written before it
started, which a plain Python read-modify-write also satisfies: a mutant that
keeps the id resolve, the rowcount fall-through and the read-back but merges
in Python left the suite green. The fixed arm now injects immediately before
its own UPDATE. The base arm keeps its _now() hook, because upstream flushes
via session.add/commit and never issues an explicit session.execute("UPDATE"),
so the pre-UPDATE hook cannot fire there. That asymmetry is the finding, so it
is stated in the test rather than papered over by asserting one hook for both.
The in-test comment that asserted the wrong arm's ordering is replaced with
the measured one.
The load-bearing details are now three, not the two the previous commit's
message enumerates: the content_hash carry joins limit(1) and NULLIF.
Test count is unchanged at nine: the content_hash assertion and its
reachability check (a third cold reinforce must still dedup, rows == 1) extend
the existing NULLIF window test rather than adding a near-duplicate.
The mutation matrix grows from six mutants to eight. M7 removes only the inner
content_hash json_set; M8 is the Python read-modify-write above. Both are
killed, M1-M6 stay killed, and the no-op control still survives at both ends.
|
|
Pre-PR validation gate (a-i)
An 8-mutant matrix backs rows (d)/(i): all 8 killed, with a no-op control surviving at both ends of the run and an exact tree restore asserted after every arm. Five mutants have survived a pass at some point on this branch; each was a real coverage gap and each is now closed by a named test. The two newest are attributable rather than assumed: M7 (removes only the inner |
Internal second-model review (2 rounds, 9 findings, $11.63)An independent model reviewed this change cold against the frozen PR description, and I
Findings 1-4 changed the code. Finding 1 was worse than reported: losing Findings 5 and 6 I first disagreed with, and that was the wrong call to leave standing. Finding 9 stays open as a note. Gate spend: $11.63 over 2 rounds ( |
|
Closing per @pufit's directive on #247: memU is being rewritten and sunset, and Nerve fixes |
2 participants
Description
memu-py1.4.0'sSQLiteMemoryItemRepo.create_item_reinforcereinforces an item by readingextra, mutating a dict copy and flushing the whole JSON column back(
memory_item_repo.py:320-333). The plainSELECTtakes no write reservation, so any key anotherconnection commits in between is silently replaced -- such as the event-date sweep's
mentioned_at, on 97% of 152k live rows. Injecting a write there: unpatched loses it, patchedkeeps it.
The fix merges server-side with
json_setand reads back inside the same still-open writetransaction:
json_extracton the same column increments without a snapshot, and theUPDATEpromotes the session to a write transaction. No
RETURNING, avoiding an unchecked SQLite >= 3.35requirement;
rowcountseparates hit from miss.Three details are reachable, not defensive.
content_hashhas no unique index, so a barehash-filtered
UPDATEwould bump every duplicate where.first()reinforces one; the arm resolvesone id with
limit(1). A writer can blank that target inside the window, wherecoalescealonefeeds
''tojson_setand raisesmalformed JSON, henceNULLIF. Blanking also dropscontent_hash-- upstream rewrote it only as a side effect of the flush this arm removes -- so aninnermost
json_setre-asserts it, else the row stops matching the arm's filter and the nextreinforce duplicates it.
The wrapper installs before the semantic-dedup patch binds the original, so the order stays
semantic -> this arm -> upstream create; hash-first would change which row a memorize reinforces,
and a test pins that.
update_itemhas that shape too and is untouched: its onlyextra-writing caller is gated behindenable_item_references(defaultFalse, unreachable from nerve), and 0 live rows carry its keys.Validation: 9 new tests; an 8-mutant matrix kills all 8, with a no-op control surviving at both
ends. The whole-suite FAILED-test-name set is byte-identical to clean
main; passed 2933 -> 2942,exactly the new tests.