memory: read each memU snapshot in one transaction so its parts agree

[oranjeai]

Description

Python sqlite3 with the default isolation_level="" issues no BEGIN for
reads, so every SELECT is its own snapshot. Three memU reads compose one logical
result from several statements, and nothing constrains those statements to agree,
so a concurrent writer can make the parts disagree.

Reproduced by committing a write between the statements of each read path:

• expand_category (3 statements) returned total=3 with 2 items, rendered as
  showing 2 of 3 items for an unpaged category;
• _read_memu_snapshot_sync (4 scans) exported a category_items link absent
  from the same payload's items, and left every items[].resource_id pointing at
  a deleted resources[] row;
• _get_db_stats_sync (5 statements) reported total_items=3 while its type
  distribution summed to 4.

The fix gives each read an explicit transaction: BEGIN after connect,
rollback() in the existing finally (read-only, so never commit). At
_get_db_stats_sync, db.close() also moves into a finally so the exception
path releases too. No query is rewritten, and no result shape changes.

A writer now waits for a read, bounded by that read's duration. On the live
5.2 GiB store the gateway snapshot holds 702-717 ms, against 671-736 ms without it,
closing before json.dumps. That store runs journal_mode=wal, where a reader never
blocks a writer. In delete mode a reader does block writers, but not because of this
change: at live row counts the item scan alone is ~2.3 s of a ~2.4 s hold, so a 5 s
writer fares the same either way.

Validation: 24 new test cases interleave a committed write at an exact statement
boundary via a trace callback, most parametrized over wal and delete. Unfixed,
19 fail with the values above; with the fix all pass, and a mutation matrix
confirms each BEGIN, its span, and the close() move are load-bearing.
Full suite 2957 passed, same 7 pre-existing failures as main.

Enumeration of every sqlite3.connect read in nerve/

All 12 call sites, classified by whether they compose more than one SELECT
into a single result:

site	SELECTs per result	disposition
memu_bridge.py expand_category	3	fixed
routes/memory.py _read_memu_snapshot_sync	4	fixed
memu_bridge.py _get_db_stats_sync	5	fixed
handlers/memory.py _fetch_history_rows	1	safe by construction
handlers/memory.py _fetch_records_rows	1 per branch	safe by construction
memu_bridge.py event-date sweep	read-modify-write	out of scope, write-path
memu_bridge.py datetime sanitize	startup, write	out of scope
memu_bridge.py WAL pragma, WAL checkpoint	startup, sole connection	safe
cli.py x2	CLI, single process	out of scope
backup.py	Connection.backup, atomic	safe

isolation_level and BEGIN had 0 occurrences in nerve/ before this change,
so all three sites are pre-existing and no existing convention is contradicted.

[oranjeai]

Internal second-model review (3 rounds, 21 findings adjudicated)

Before this PR was opened it went through three independent review rounds: a cold
code review plus a separate reviewing model, with each finding adjudicated against
the code rather than accepted or dismissed. The source fix is byte-identical to
round 0; every round after that moved tests and prose only.

Verdict summary

round	findings	agreed + fixed	disagreed with evidence
0	7	6	1
1	8	6	2
2	6	3	3

Agreed and fixed

#	finding	resolution
1	The WAL claim "a reader never blocks a writer" had no test behind it; every arm ended ok, so the suite could not tell BEGIN from BEGIN EXCLUSIVE.	Each interleaving test now asserts whether the writer committed while the transaction was open. Measured: BEGIN EXCLUSIVE makes the WAL writer wait out the whole read and still succeed, so only this assertion catches it.
2	Transaction SPAN was untested: statements with no interleaving needle meant a BEGIN moved later would pass.	Needles added until every statement of all three reads is either the first or has a write firing at it. Verified by exhaustive displacement: moving the stats BEGIN past statement 1, 2 or 3 kills 2, 4 and 6 tests respectively, and the same mutation kills 2 tests at each of the other two sites.
3	The cleanup oracle depended on the garbage collector: a forced collection made a wall-clock assertion pass on unfixed code.	Both release tests now assert on the retained connection instead of the clock, and hold it so no collection can hide a leak.
4	close() moved into finally was unguarded: putting it back left a connection open forever and no test failed.	A closure assertion was added alongside the release one. They pin different properties and trip on different assertions, verified by reading which one each mutant fails.
5	"Every memU writer sets busy_timeout=30000" was published as the safety argument and is false for two of five writer channels (a startup sanitizer and a CLI backfill use the 5000 ms default).	Claim corrected to a per-writer statement on every surface.
6	The delete-mode safety sentence claimed no writer fails because all budgets exceed the hold. False at live scale, in the unsafe direction.	Re-measured at live row counts: the item scan alone is ~2.3 s of a ~2.4 s hold, and a 5 s writer fares the same with and without the transaction. The change is exonerated; the sentence was rewritten to say so.
7	Coverage was stated as complete on three surfaces while one site still had an uncovered boundary, and one row of this comment disclosed the gap while another asserted the unqualified version.	Gap closed first, then every surface restated.
8	Test figures drifted as rounds added cases (20 new/14 fail/2947 passed).	Re-derived from clean exports rather than relayed: 24 new cases, 19 pre-fix failures, 2957 passed.

Disagreed, with evidence

A JOIN on the relation scan is not an isolation fix, so the test pinning that is kept.
It was twice proposed for deletion because it calls no production function. Its assertion is the
negation of the transaction property, so it goes red if anyone adds a BEGIN there, and it guards
against exactly the wrong fix a sibling open PR ships on that same scan. Its docstring already
states that it pins nothing in nerve/.

The writer-progress oracle's one-second sample is not timing-fragile.
The concern was that a slow writer thread could make a correct build fail. Measured over 60 runs:
the sampled quantity is the writer's whole connect-to-commit latency, 4.6-12.5 ms unloaded and
6.6-26.2 ms under 700-way CPU oversubscription (load average 118) - a 38x margin against the
1000 ms budget, with 30/30 verdicts correct in each mode, and the delete arm pinned to the
other side at 1034-1097 ms by its busy_timeout. The decisive half is direction: forcing the
sampled value to the wrong verdict fails 9 tests in one direction and 11 in the other, so a
timing slip can only produce a false failure, never a false pass. Removing the assertion would
also reopen finding 1 above, since it is the only thing distinguishing BEGIN from
BEGIN EXCLUSIVE. The corroborating wall-clock assertions are labelled as such in-file and run
at 0.05 s against a 4.0 s threshold.

rollback() is documentation, not behaviour, and stays.
close() alone releases the transaction, measured, so a mutant dropping rollback() survives by
construction. Three read paths that must never commit say so explicitly in one line each.

Observed, not blocking

• db.execute("BEGIN") sits between connect() and the try at all three sites, so a raise
  there would leak the connection. No reachable raise was found: a deferred BEGIN takes no lock
  and cannot fail on contention (it succeeds even against a held BEGIN EXCLUSIVE), a corrupt
  database file still lets it return, and a bad path raises at connect() instead.
• The commit message quotes the live-store hold as 595 ms against 577 ms. Re-measured on the now
  5.23 GiB store it is 702-717 ms against 671-736 ms; the store grew between rounds. The figure
  understates the hold and the conclusion - that the two arms are indistinguishable - is unchanged.
  The PR body carries the current numbers.
• Five of the 24 new cases pass against unfixed source by design: they pin boundary shapes
  (total staying unfiltered, all-links-dangling, unknown-id release) and the JOIN claim above,
  rather than discriminating the fix. The other 19 are the discriminating set.

Reviewed at three commits; 8 gate runs, $34.73.

[oranjeai]

Pre-PR validation gate (click to expand)

#	Question	Answer
a	Deterministic repro?	Yes. A probe interleaves a committed write, from a separate thread with the busy_timeout=30000 every memU writer sets, between the statements of each real read path via sqlite3.set_trace_callback. All four invariants are violated on every run, in both wal and delete: total=3 len=2; links-not-in-items=['it-9']; items-with-missing-res=['it-3','it-2','it-1']; total_items=3 sum(dist)=4. Each arm reports that the interleaved write really committed. Not a rate-based repro.
b	Root cause explained?	Default isolation_level="" emits no BEGIN for reads, so every SELECT on a connection is its own snapshot. expand_category (3 statements), _read_memu_snapshot_sync (4 scans) and _get_db_stats_sync (5 statements) each compose one logical result from several statements, so nothing constrains any statement's output to agree with any other's. Every query is correct alone; the composition has no isolation boundary.
c	Fix matches root cause?	Yes: one explicit read transaction per read, i.e. the property itself. Measured alternatives rejected as band-aids: a python-side filter on the relation scan repairs the link invariant but leaves items[].resource_id dangling, and a SQL JOIN fixes neither (it runs in its own later snapshot and still returns the post-scan link). No bound widened, no query rewritten, no result shape changed.
d	Test intent preserved / new tests added?	24 new test cases in 14 new functions (base collected 14 in this file, now 38; counted by name against a clean git archive export, 0 removed). The 3 existing expand_category tests and their helpers are untouched and still green (repo convention prefers a new test over extending one). Boundary shapes covered: paged LIMIT, query filter matching one and none (asserting total stays unfiltered), all-links-dangling, unknown id, and both transaction-release paths. Every statement of all three reads is either the first statement or has an interleaved write firing at it, so each contract boundary is covered: 2 of 3 statements in expand_category, 3 of 4 gateway scans and 4 of 5 in _get_db_stats_sync carry a needle, the remainder being each read's first statement. Every site is mutant-proven individually: moving its BEGIN past the first read kills 2 tests at each of the three sites. Each test also asserts whether the interleaved writer committed while the transaction was open, the one observable that separates this BEGIN from a lock-taking one. Both release tests assert on the connection the code under test opened (closed, or no transaction in progress) instead of on a wall-clock threshold, so their verdict does not depend on the garbage collector or the scheduler.
e	Both directions demonstrated?	Yes. Base arm with only the source reverted and the new tests kept: 19 failed, 19 passed, with the values in (a). Fixed arm: 38 passed. One of the two release tests appears in that arm and the other correctly does not: the stats one fails pre-fix because its closure oracle sees the connection the skipped close() leaks, while the expand_category one discriminates only the close() move, so its arm is the mutant that puts close() back in the try body, which it kills even with a forced gc.collect(). Compared by failure NAME against a clean git archive export of the base, run in ONE process together with the other memory and db suites (these tests patch the global sqlite3.connect, so a single-file run cannot see contamination): base 25 failed, 262 passed, fixed 6 failed, 281 passed, with no failure unique to the fixed arm and the 19 base-only failures being exactly the discriminating tests; all 6 are pre-existing date-boundary failures unrelated to transactions. 50 sequential repetitions of the file: 50/50 pass (1,900 executions). A mutation matrix covers each BEGIN, the close() move, and each transaction SPAN: moving any BEGIN past the first read is killed, as is BEGIN EXCLUSIVE. Neutralizing the shared writer-progress assertion and re-running the span mutants shows each test's own invariant still kills its own mutant independently, and that BEGIN EXCLUSIVE is caught only by the progress assertion, so neither layer is redundant.
f	Fix is general across code paths?	All 12 sqlite3.connect( sites in nerve/ enumerated and classified; every multi-statement read is fixed. I read the two single-statement neighbours (_fetch_history_rows, _fetch_records_rows, both branches) rather than trusting a label. _get_db_stats_sync was found by that enumeration, not by the original report. Remaining sites are startup-only, CLI-only, the atomic Connection.backup, or a write-path read-modify-write that is a different defect.
g	Fix generalizes across inputs?	All ten interleaving tests are parametrized over wal and delete (20 cases; the repo sets WAL only best-effort, so delete is reachable and is where a read transaction genuinely blocks). The interleaved writer uses the real busy_timeout=30000. Boundary shapes per (d). Live-store cost re-measured at 5.23 GiB / 154,112 items: transaction held 702-717 ms, versus 671-736 ms without it. The writer-progress oracle's margin was measured under 700-way CPU oversubscription (load average 118): the sampled writer latency stays at 6.6-26.2 ms against its 1000 ms budget, 30/30 verdicts correct, and forcing either wrong verdict fails 9 and 11 tests respectively, so a timing slip can only produce a false failure, never a false pass.
h	Backward compatible?	Yes, nothing to break: no setting, serialization format, schema, migration, or public signature is touched, and every path returns the same shape (asserted by the untouched pre-existing tests). No deliberate break, so no maintainer exception is needed.
i	Invariants and contracts preserved?	The invariant introduced is "one logical read result comes from one snapshot", and it now holds on every path. The transaction is read-only (rollback(), never commit()), and every exit releases it, including expand_category's early return for an unknown category and _get_db_stats_sync's exception path, which is why its db.close() moved into a finally. Writer liveness is measured, not assumed: in delete mode the interleaved writer waits about a second and succeeds in every delete arm.

Session id: cron:clickhouse-impl-slot-6:20260803-191600

[oranjeai]

cc @alex-fedotyev — could you review this? Python sqlite3 emits no BEGIN for reads, so each SELECT is its own snapshot; three memU reads compose one result from 3, 4 and 5 statements, and a concurrent writer can make those parts disagree (expand_category reporting showing 2 of 3 for an unpaged category, the gateway snapshot exporting links and resource refs absent from the same payload, diagnostics reporting a type distribution that does not sum to total_items). Each read now runs in one explicit read transaction; no query is rewritten and no result shape changes.

[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.