Skip to content

memory: count only resolvable items in memU category totals - #252

Closed
oranjeai wants to merge 1 commit into
ClickHouse:mainfrom
oranjeai:oranjeai/memu-category-count-join
Closed

memory: count only resolvable items in memU category totals#252
oranjeai wants to merge 1 commit into
ClickHouse:mainfrom
oranjeai:oranjeai/memu-category-count-join

Conversation

@oranjeai

@oranjeai oranjeai commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Symptom

memory_expand_category advertises more items than it can return:

Category 'procedures' - showing 20 of 68069 items (most recent):

Only 65655 of those are reachable, so paging past the reachable end returns
nothing. The web memory sidebar's per-category badge is inflated identically.

Measured on a live 146030-item store: 6455 of 175114 membership rows are
unreturnable, a 3.69% overstatement affecting all 8 categories (3.29% to
4.29%).

Root cause

memu_category_items is a link table with no foreign key to
memu_memory_items, so delete-side writers strand membership rows whose item
is gone. Two read paths counted those rows directly:

  • MemUBridge.expand_category computed total with an unjoined count(*),
    while the item listing three lines below already joined memu_memory_items.
    So total and items were computed over two different row sets in the same
    function, connection and transaction.
  • _read_memu_snapshot_sync exported unjoined (category_id, item_id) pairs
    as category_items, which the sidebar counts via ids.length.

The stranded rows are permanent, not in-flight: they were created between
2026-06-25 and 2026-08-02, while the newest relation in the store is newer
still.

Fix

Add the listing's own join predicate at both sites, so the count and the
listing agree by construction rather than by coincidence. Chosen over an
equivalent EXISTS because it is textually the same join the function already
performs, leaving no room for the two spellings to drift.

Both queries stay fully index-covered (SCAN ci USING COVERING INDEX idx_sqlite_category_items_unique + SEARCH i USING COVERING INDEX ...).
Measured cost: 72.2 -> 102.8 ms for the once-per-page-load snapshot (already
run in a worker thread) and 9.4 -> 12.9 ms for a per-category total.

Server-side SQL only. No frontend change is needed: the sidebar badge is
repaired by the gateway fix, and the other five categoryItems consumers
already drop unresolvable ids, so they are no-ops under it.

Reported counts decrease for existing users. That is the point - the new
numbers are what the store can actually return.

Two pre-existing, distinct mismatches are deliberately not addressed here:
total ignores the query filter, and the sidebar badge counts all memory
types while the Facts tab renders five of them.

Tests

Two tests in tests/test_recall_breadcrumbs.py, one per site, both failing on
main (assert 5 == 3) and passing here. The bridge test pins
total == len(items) when unpaged - the actual invariant - rather than a magic
number, and the gateway test asserts cross-site agreement between the badge
count and the agent's total.

Target suite 16 passed (14 before). Full suite 2935 passed; the 7 failures are
pre-existing and timezone-related, with a byte-identical FAILED set before and
after this change.

Generated by Nerve

## Symptom

`memory_expand_category` advertises more items than it can return, e.g.
"Category 'procedures' - showing 20 of 68069 items" when only 65655 are
reachable; paging past the reachable end yields nothing. The web memory
sidebar badge is inflated the same way. Measured on a live 146030-item
store: 6455 of 175114 membership rows are unreturnable, a 3.69% overstatement
present in all 8 categories (3.29% to 4.29%).

## Root cause

`memu_category_items` is a link table with no foreign key to
`memu_memory_items`, so delete-side writers strand membership rows whose item
is gone. Two read paths counted those rows directly:

- `MemUBridge.expand_category` computed `total` with an unjoined
  `count(*)`, while the item listing three lines below already joined
  `memu_memory_items`. So `total` and `items` were computed over two
  different row sets in the same function and connection.
- `_read_memu_snapshot_sync` exported unjoined `(category_id, item_id)`
  pairs as `category_items`, which the web sidebar counts via `ids.length`.

The stranded rows are permanent, not in-flight: they were created between
2026-06-25 and 2026-08-02.

## The fix

Add the listing's own join predicate at both sites, so the count and the
listing agree by construction rather than by coincidence. Chosen over an
equivalent `EXISTS` because it is textually the same join the function
already performs, leaving no room for the two to drift.

Both queries stay fully index-covered (`SCAN ci USING COVERING INDEX
idx_sqlite_category_items_unique` + `SEARCH i USING COVERING INDEX ...`);
measured cost is 72.2 -> 102.8 ms for the once-per-page-load snapshot
(already in a worker thread) and 9.4 -> 12.9 ms for a per-category total.

Reported counts decrease for existing users. That is the point: the new
numbers are what the store can actually return.

Two pre-existing and distinct mismatches are deliberately NOT addressed
here: `total` ignores the `query` filter, and the sidebar badge counts all
memory types while the Facts tab renders five of them.
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@oranjeai

oranjeai commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author
Internal second-model review (2 findings adjudicated, plus 2 I raised myself)

Before opening this PR I ran an independent cold review of the resulting code, then an independent
second model (codex, gpt-5.6-sol, xhigh) over the same diff and PR contract. Findings and my
verdicts:

# Finding Severity Verdict
1 The PR description quoted a fenced "verbatim" tool output containing (all types) ⚠️ major AGREE - fixed before publishing
2 expand_category: count and listing are separate read snapshots, so a concurrent write can make an unpaged total disagree with items ⚠️ major DISAGREE on attribution (mechanism real, pre-existing; see below)
3 Gateway snapshot: item scan and relation scan are separate read snapshots, so an id can be exported that the payload's item list omits ⚠️ major DISAGREE on attribution (mechanism real, pre-existing; and the stated consequence is falsified)
4 A category whose links are all dangling now loses its category_items key entirely 💡 nit AGREE - noted, not blocking

1 - fixed. The handler can only ever render (most recent) or (matching '<q>')
(nerve/agent/tools/handlers/memory.py:170); all types appears nowhere in the repo at any
revision. My own review caught it and I corrected the description before publishing, so no
maintainer had to grep for a string that never existed.

2 and 3 - the mechanism is real and I reproduced it, but it is not this change's defect.
Python's sqlite3 default isolation_level="" issues no BEGIN, so each SELECT is its own
snapshot (measured: count(*) returns 3 then 4 on one connection across an interleaved commit).
That structure is unchanged by this PR: both functions already ran their statements on one
no-BEGIN connection at main (0 hits for BEGIN/isolation_level in either function at base,
and 0 in this diff). The decisive control is a base arm - same fixture, same interleaved DELETE -
which produces an identical discrepancy with and without the join (total=3 len(items)=2 both
ways), so no measurement distinguishes the two arms. This PR changes which rows the count
matches, never how many snapshots are taken.

For finding 3 the stated consequence is specifically falsified. Running the reviewer's own INSERT
interleaving, the exported-id-not-in-items outcome appears identically before and after
(links-not-in-items=['it-9'] both ways), so the join cannot be what "transiently recreates an
inflated sidebar badge". The one direction the join does change is the opposite and beneficial:
under an interleaved delete the joined export drops the dead id, making the badge transiently
conservative, never inflated.

I checked this against the rule that pre-existing behaviour never excuses a PR's own contract. The
changelog here promises the count matches what the store can return - a single-snapshot integrity
property. Cross-statement atomicity under a concurrent writer is a separate isolation property whose
remedy is a transaction, not a join.

I also measured the suggested remedy rather than assuming it, and it is not safe to apply here.
At the bridge site an explicit BEGIN does fix the invariant in both journal modes, but in
journal_mode=delete it blocks the writer (database is locked) - and WAL is only best-effort in
this codebase (memu_bridge.py:1344 warns when it cannot be set). At the gateway site the
transaction would have to span the whole snapshot, which I timed on a live store at 11.1 s
(the resource scan alone is 10.5 s); memu_bridge.py:1356-1360 documents in-repo that exactly such
long-lived readers starve WAL checkpoints until the WAL grows to gigabytes and every commit slows.
Applying it would trade a benign one-page-load transient for a documented store-wide hazard. Both
findings, the reproductions, the timings, and a prototyped single-statement alternative (a scalar
subquery, verified across five boundary shapes and atomic under the interleave) are recorded on a
separate follow-up task so the real defect is owned rather than dropped.

4 - noted. Harmless because every consumer is guard-indexed
(categoryItems[cat.id] || [], catCounts[cat.id] || 0), and the shape was probed during
development - but no shipped test pins it. Carried on the same follow-up rather than bouncing a
round for an unshipped test of an unreachable breakage.

Independently re-derived every published figure rather than carrying them: 6455 dangling relations
(exact match), eight categories at 3.27-4.28%, dangling created_at spanning 2026-06-25 to
2026-08-02 against a newer newest relation, and EXPLAIN confirming both fixed queries stay fully
index-covered. Also verified the join cannot fan out (memu_memory_items.id is a primary key,
0 duplicate ids) and that SQL join resolution agrees exactly with set-membership resolution
(169513 == 169513), so the corrected count cannot diverge from how the UI resolves ids.

Gate spend: $8.30 over 2 gate runs (plan approach review + this code review).

@oranjeai

oranjeai commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author
Pre-PR validation gate (a-i)
# Check Result
a Deterministic repro Yes. pytest tests/test_recall_breadcrumbs.py::test_expand_category_total_excludes_dangling_relations fails 100% on main with assert 5 == 3; the gateway twin fails with assert 5 == 3 on ['it-1','it-2','it-3','it-gone-1','it-gone-2']. Also directly observable on any live store: unjoined 175114 rows vs joined 168659.
b Root cause memu_category_items has no foreign key to memu_memory_items, so delete-side writers strand membership rows. expand_category computed total with an unjoined count(*) while its own item listing three lines below joined memu_memory_items; _read_memu_snapshot_sync exported unjoined pairs. Both therefore counted rows neither could return. Orphans are permanent (created 2026-06-25 to 2026-08-02), not in-flight.
c Fix matches root cause Yes. The falsehood is produced by the missing join predicate, and the join is added exactly there. Not a band-aid: no filtering downstream, no data mutation, no threshold widening, no defensive guard masking the writers.
d Test intent preserved / new tests All 14 pre-existing tests unchanged and still passing. The shared fixture gained resource_id, happened_at and memu_resources (additive only, required by the gateway reader). Two new tests, one per site.
e Both directions Yes, with the tests as sole discriminator: reverting only the two source files (tests kept) gives 2 failed / 14 passed; restored gives 16 passed. Re-run after the final tree change.
f General across code paths Carrier set closed by grep: exactly 5 non-test memu_category_items hits - 2 unjoined counters (both fixed), 1 already-joined listing (the model), 1 docstring, 1 table registration. No third server-side counter; xmemory_bridge.py has no category count. All 6 frontend categoryItems consumers read individually: :644 is the repaired badge, the other 5 already drop unresolvable ids, so web/ needs no edit.
g Generalizes across inputs Probed 6 shapes against both sites, all agreeing: empty category (0/0/0), all links dangling (0/0/0), single live link, NULL item_id, empty-string item_id, and a duplicated (category_id, item_id) pair. The last is the one case a JOIN could fan out: it returns 2, matching the listing, because memu_memory_items.id is a primary key.
h Backward compatible No persisted format, setting, migration or API shape changes. total stays an int, category_items stays dict[str, list[str]]. Counts decrease, which is the intended correction and is stated plainly in the PR body.
i Invariants and contracts The invariant reported count == what the store can return now holds by construction. All four expand_category early returns (unavailable bridge, empty id, unknown category, exception) return before the changed query and are unaffected; the gateway statement stays inside the existing try/finally: db.close(). Read-only change: no locks, no writes, no concurrency contract touched.

Cost measured, not assumed: both fixed queries are fully index-covered
(SCAN ci USING COVERING INDEX idx_sqlite_category_items_unique + SEARCH i USING COVERING INDEX sqlite_autoindex_memu_memory_items_1 (id=?));
snapshot 72.2 -> 102.8 ms, per-category total 9.4 -> 12.9 ms.

Full suite: 2935 passed. The 7 failures are pre-existing and timezone-related
(box TZ is +1200); the sorted FAILED name set is byte-identical before and after.

@oranjeai

oranjeai commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

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.

@oranjeai oranjeai closed this Aug 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants