Skip to content

memory: make the event-date sweep's writes atomic so concurrent writers are not clobbered - #247

Closed
oranjeai wants to merge 1 commit into
ClickHouse:mainfrom
oranjeai:oranjeai/memu-date-sweep-atomic-extra-write
Closed

memory: make the event-date sweep's writes atomic so concurrent writers are not clobbered#247
oranjeai wants to merge 1 commit into
ClickHouse:mainfrom
oranjeai:oranjeai/memu-date-sweep-atomic-extra-write

Conversation

@oranjeai

@oranjeai oranjeai commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Description

MemUBridge._resolve_event_dates_sync snapshots each item's whole extra JSON blob in its
pre-LLM SELECT, calls Anthropic to resolve event dates, then writes that pre-call snapshot back
just to add one key. Any writer touching extra inside that window (seconds) is silently
reverted. The sweep runs in _blocking_pool on its own sqlite3 connection, so it genuinely
races normal write paths.

Measured, snapshot then concurrent write then writeback: every key the other writer set reverts
to its stale value, and a key it added is deleted outright. The live victims are the reinforce
counters reinforcement_count and last_reinforced_at, so salience rolls back silently.

The same loop has a second, independent lost update: the happened_at UPDATE never
re-asserts the SELECT's happened_at IS NULL predicate, so a concurrent cli.py backfill is
overwritten.

The fix makes each write a function of the current row, inside the write statement:

  • the extra write becomes json_set against the live column, so another writer's keys
    survive. COALESCE(NULLIF(extra, ''), '{}') normalises NULL/empty (json_set(NULL, ...)
    returns NULL, json_extract('', ...) raises); an inner COALESCE(json_extract(...))
    preserves stamp-once against a second concurrent sweep;
  • the happened_at write gains AND happened_at IS NULL (strictly narrowing);
  • extra is dropped from the SELECT, as no Python code reads it now. The
    instr(extra, ...) scoping guard is in the WHERE, unchanged.

Still one UPDATE per row, so small-batch commits are preserved. json1 is not a new
requirement: memU's create_item dedup path already needs json_extract.

Validation: 3 new tests drive the concurrent write from inside the real LLM await window; each
fails on current code with the exact defects above and passes with the fix. A fourth guards
NULLIF. Full suite both arms: 4F/2941P to 1F/2944P, zero new failures.

Behaviour change to flag: a later concurrent sweep no longer overwrites an earlier
mentioned_at stamp. This restores the documented contract, observably.

Pre-existing, timezone-dependent test failures (not caused by this PR)

The tests added here pass in any timezone (verified in Pacific/Fiji, America/New_York,
Asia/Tokyo, UTC and Australia/Sydney with no TZ override), so every failure below is
pre-existing at main.

NerveConfig().timezone defaults to America/New_York while the 11 pre-existing
TestResolveEventDatesSync tests use naive conv_ts values, so on a host in another timezone 6
of them fail at unmodified main:

6 failed, 5 passed, 65 deselected     (assert '2026-02-26' == '2026-02-27')

test_mentioned_at_set_on_all_items, test_llm_failure_falls_back_to_conversation_date,
test_preserves_existing_extra_fields, test_sweep_skips_old_and_already_stamped_items,
test_sweep_is_idempotent, test_sweep_commits_in_batches.

With TZ=America/New_York the same unmodified tree is 11/11 green. I measured this control arm
before making any change, so the attribution is unambiguous. Left alone deliberately: pinning TZ
in those tests is a separate change this PR does not own.

Run the tests with:

TZ=America/New_York .venv/bin/pytest tests/test_memu_bridge.py -k ResolveEventDates -q

The single remaining full-suite failure,
tests/test_telegram_sessions.py::test_tail_timestamps_in_user_timezone, is also pre-existing
and unrelated (this PR touches no telegram files).

…rs are not clobbered

`MemUBridge._resolve_event_dates_sync` snapshotted each item's whole `extra`
JSON blob in its pre-LLM SELECT, awaited an Anthropic call to resolve event
dates, then wrote that pre-call snapshot back merely to add `mentioned_at`.
The blob is the unit of write, so the update was last-writer-wins over the
whole dict instead of over the one key the sweep owns: every key another
writer set inside that window was silently reverted. The sweep runs in
`_blocking_pool` on its own raw `sqlite3` connection, so it genuinely races
the normal write paths rather than being serialised behind them.

Measured, snapshot then concurrent write then writeback: every key the other
writer set reverts to its stale value, and a key it added is deleted outright.
The live victims are the reinforce counters `reinforcement_count` and
`last_reinforced_at`, rewritten on existing rows by two SQLite paths
(`memu_bridge.py:1211` and memU's own `create_item_reinforce` branch), so
salience silently rolls back.

The same loop carried a second, independent lost update: the `happened_at`
UPDATE did not re-assert the SELECT's own `happened_at IS NULL` predicate, so
a concurrent backfill (`cli.py`) was overwritten.

Each write is now a function of the current row, evaluated inside the write
statement:

- the `extra` write uses `json_set` against the live column, so a concurrent
  writer's keys survive. `COALESCE(NULLIF(extra, ''), '{}')` normalises
  NULL/empty, because `json_set(NULL, ...)` returns NULL and
  `json_extract('', ...)` raises; without it a NULL-extra row would stay NULL
  and be re-swept on every future conversation forever. An inner
  `COALESCE(json_extract(...))` preserves the documented stamp-once behaviour
  against a second concurrent sweep, which is the only other writer of that
  key.
- the `happened_at` UPDATE gains `AND happened_at IS NULL`, strictly
  narrowing what the SELECT already restricted.
- `extra` is dropped from the SELECT column list, since no Python code reads
  it any more. The `instr(extra, ...)` scoping guard lives in the WHERE clause
  and is unchanged.

Still exactly one UPDATE per row, so the small-batch commit property is
preserved. `json1` is not a new requirement: memU's own `create_item` dedup
path already uses `json_extract(extra, '$.content_hash')`, so no store that
reaches this sweep can lack it.

Three tests drive the concurrent write from inside the real LLM await window,
and each fails on the previous code with the exact defects above and passes with
this change. A fourth test (two cases) guards the empty-value normalisation and
passes both ways by design, which its docstring states. All four use
offset-aware timestamps, so none depends on the host timezone. Two of the three
also assert the sweep's own write landed on an unraced control row, so they
cannot pass by the write never running. A ten-arm mutation matrix kills all
eight real mutants with the no-op control surviving at both ends; every mutant
leaves the eleven pre-existing sweep tests green, so that suite had no
visibility into this defect class. Full suite on both arms from independent
clean exports: 4 failed / 2941 passed before, 1 failed / 2944 passed after,
failure sets diffed by name with zero new failures. On a live store `json_set`
is semantically identical to the old Python round-trip on all 140,778 rows.

Note that a later concurrent sweep no longer overwrites an earlier
`mentioned_at` stamp. This restores the documented contract, but it is an
observable behaviour change.

The eleven pre-existing sweep tests need `TZ=America/New_York`:
`NerveConfig().timezone` defaults to that zone while those tests use naive
timestamps, so on a host in another timezone six of them already fail at
unmodified main. That is pre-existing and out of scope here; the tests added
here pass in any zone.
@oranjeai

oranjeai commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author
Pre-PR validation gate (a-i)
# Check Answer
a Deterministic repro? Yes. TZ=America/New_York .venv/bin/pytest tests/test_memu_bridge.py -k ResolveEventDates -q. The concurrent write is driven from the _resolve_dates_via_llm stub's side_effect, so it executes inside the real await window: the interleaving is forced, not sampled. Identical outcome across a 10-arm mutation matrix with the no-op control run at both ends. The 4 new tests use offset-aware timestamps, so they pass with no TZ override in Pacific/Fiji, America/New_York, Asia/Tokyo, UTC and Australia/Sydney.
b Root cause explained? Yes. The sweep snapshots the whole extra blob in its pre-LLM SELECT, awaits an Anthropic call (seconds), then writes that stale snapshot back, so the update is last-writer-wins over the whole dict instead of the one key it owns. It runs in _blocking_pool on its own sqlite3 connection, so it is not serialised behind the normal write paths. Separately, the happened_at write never re-asserts the SELECT's happened_at IS NULL predicate.
c Fix matches root cause? Yes. Each write becomes a function of the live row inside the write statement, at the layer where the lost update happens. No lock, no retry, no widened bound, no guard at a symptom site. The two alternatives were measured, not asserted: the re-read variant is still read-modify-write and is killed by T3 as a real weakening (M5), and an explicit lock would hold the SQLite write lock across a network call, the starvation the batch-commit comment exists to avoid.
d Test intent preserved / new tests? Yes. 4 new tests (5 cases; the empty-extra test is parameterized over SQL NULL and ''). Two of them also assert the sweep's own write landed on an unraced control row, so neither can pass by the write never running. All 11 pre-existing sweep tests unchanged and green; nothing weakened, skipped, or opted out.
e Both directions? Yes. Arm A (clean export, production file byte-verified against main) 3F/13P with the exact messages assert 'OLDHASH' == 'NEWHASH', assert '2026-02-05' == '2020-01-01', assert '2026-02-27' == '2026-02-20'. Arm B (fix) 16P. Both arms ran the identical test file, so the tests are the discriminator. The empty-extra test passes at base by design and says so in its docstring: it guards the fix's own empty-value normalisation, where '' (not NULL) is the case that discriminates NULLIF.
f General across code paths? Yes. All three writes in the loop enumerated: extra (fixed), happened_at (fixed, an independent live defect), mentioned_at stamp-once (preserved by the inner COALESCE). Sibling happened_at writers grepped: the cli.py backfill is now protected; two one-time migrations run outside the sweep window.
g Generalizes across inputs? Yes, measured and now asserted for both empty shapes: NULL extra and '' (the latter discriminates NULLIF; COALESCE alone already handles NULL), malformed JSON, non-object, UTF-8, nested objects/arrays with null/bool/float, multi-key blobs, a row deleted mid-window (rowcount=0, no exception), non-event rows (happened_at stays NULL), and the batch-commit path. On a live store json_set is semantically identical to the old Python round-trip on 140,778 of 140,778 rows, 0 mismatches.
h Backward compatible? Yes, nothing engaged: no settings default, no serialization-format change, no migration, no config key, no API change, no frontend (0 web/ files, so no npm run build). json1 is a pre-existing hard dependency, since memU's own create_item dedup path uses json_extract(extra, '$.content_hash'), and memu-py is pinned ==1.4.0 so it cannot drift.
i Invariants preserved? Yes. Stamp-once idempotence (a mutant dropping the guard is killed); sweep scoping and no whole-corpus rewrite (the instr guard is in the WHERE; a mutant dropping either NULLIF, or both, is killed by the '' case); small-batch commits (still one UPDATE per row, pending/_commit_batch() untouched); error paths (a malformed blob raises inside the existing try/except, verified byte-identical on both arms, no exception escapes); deleted-row path.

Mutation matrix: 10 arms. The no-op control survived at both ends; all 8 real mutants were
killed, including one that reverts the fix, one per suppressed write, and one that removes both
NULLIF calls. Every mutant left the 11 pre-existing sweep tests green, so the existing suite had
no visibility into this defect class, which is what justifies all four new tests. Three of those
mutants also survive the tests as first written, so they measure the coverage this round added
rather than merely restating it.

Full suite, both arms from independent clean exports, TZ=America/New_York: 4F/2941P at base
vs 1F/2944P with the fix. Failure sets diffed by name, not by count: zero fix-only failures. The
one common failure, tests/test_telegram_sessions.py::test_tail_timestamps_in_user_timezone, is
pre-existing and unrelated.

ruff check nerve/memory/memu_bridge.py: all checks passed on both arms. The 2 ruff findings
in tests/test_memu_bridge.py are pre-existing at main (confirmed against the pristine file;
the F841 line moves 646 to 790, exactly the added line count, so it is the same occurrence in
an unrelated test) and are left alone as out of scope.

Session id: cron:clickhouse-impl-slot-40:20260803-102100

@oranjeai

oranjeai commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author
Internal second-model review (independent adjudication)

Before this PR was published I ran an independent review pass plus an automated
second-model gate on the resulting code, then adjudicated every finding. Rounds and
verdicts below; nothing was silently overridden.

Rounds: 4 (1 approach gate + 3 code-review rounds). Findings raised: 9 across all
rounds, all listed below: 8 agreed and fixed, 1 disagreed with evidence. The final round raised
no findings and confirmed the previous round's fixes.

# Finding Severity Verdict
1 Two new tests depended on the host timezone, and the disclosure covered only the pre-existing failures major ❌ AGREE, fixed (offset-aware timestamps on the new call sites only; the 4 new tests now pass in 5 zones with no TZ override)
2 The empty-extra test inserted SQL NULL, so NULLIF was never exercised; a mutant removing both NULLIFs survived the whole suite major ❌ AGREE, fixed (parameterized over NULL and ''; that mutant is now killed, and it makes the sweep raise on an '' row, so it was an availability gap, not just coverage)
3 Two concurrency tests asserted only the value their own stub installed, so a suppressed write would have passed major ❌ AGREE, fixed (each now also asserts the sweep's own write landed on an unraced control row; suppressing either write is now killed by the matching test)
4 The commit message overstated impact by leading with a key that is unreachable in this deployment major ❌ AGREE, fixed (message now leads with the live victims, the reinforce counters)
5 The commit message cited an in-memory reinforce path as a SQLite writer that can race the sweep major ❌ AGREE, fixed (that line mutates an in-process dict; this deployment selects the SQLite provider, so it can never touch the swept file. The two real SQLite paths are now named)
6 "Four tests drive the concurrent write" when only three do 💡 nit ❌ AGREE, fixed on both surfaces (the fourth guards normalisation and passes both ways by design)
7 New test docstrings repeated PR history and full SQL mechanisms 💡 nit ❌ AGREE, fixed (condensed to the invariant under test)
8 Residual docstring content on the empty-value test should also be deleted 💡 nit ⚠️ DISAGREE with evidence (below)
9 An internal validation note still quoted superseded figures 💡 nit ❌ AGREE, fixed (internal artifact only, never published)

⚠️ Disagreed, with evidence (finding 8). The gate asked to delete the second docstring
paragraph on the empty-value test as implementation history. Two facts overrode it. First, the
condensation round that produced finding 7 explicitly required that paragraph to stay, because
the test passes at base by design and a later reader who did not know that would mistake it for
dead coverage; the same round's own criterion allowed the carve-out. Second, it is not history:
measured, the paragraph contains zero occurrences of any affected key and zero SQL tokens,
naming only the construct under guard. The pre-PR validation comment on this PR also states
that this test passes at base "and says so in its docstring", so removing it would strand a
published claim. Recorded as noted-not-blocking rather than actioned.

On the production change itself: zero findings, in all three code-review rounds. Both my own
cold review and the automated gate read the full sweep, its caller, every sibling writer of the
affected column, the CLI backfill and the pinned store model, and neither found a defect in
nerve/memory/memu_bridge.py. Every finding across all rounds was in the tests, the commit
message, or internal notes. The production file is byte-identical across all three rounds.

Independently re-derived rather than taken on trust: the json_set expression probed over 21
extra shapes (empty string, SQL NULL, malformed, non-object roots, bigint, 1e300, UTF-8,
deep nesting, JSON-null and numeric stamps) with the re-sweep predicate checked on each result;
a live-store control confirming every row is a valid JSON object, so the normalisation
branches are defensive-only (re-measured each round on a growing store: 142,791 rows, 0
non-object, 0 invalid, 0 NULL, 0 empty); an independent enumeration of every UPDATE against the table;
and a --collect-only count confirming 11 cases before and 16 after. One behaviour is a strict
improvement over the previous code: a non-object value used to raise and abort the whole batch,
and now leaves the row untouched.

The final round's change was to the commit message alone, which the automated gate cannot see:
its review object is the diff plus the PR description. So that round was audited by hand, and
every claim in the message was re-derived rather than carried over. Both cited writers were
re-read at the revision and confirmed to hold their own database session and commit. Every
enumerative claim was counted three independent ways, which is how the "four tests drive the
concurrent write" overstatement in finding 6 was found: only three do.

Second-model gate spend for this PR: $12.97.

@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

@pufit could you take a look when you have a moment? You own memu_bridge.py, and this is a
9-line change to _resolve_event_dates_sync plus 4 tests.

Short version: the event-date sweep snapshotted the whole extra JSON blob before its Anthropic
call and wrote that snapshot back afterwards, so any key another writer set during that window was
reverted. The live victims are the reinforce counters. Each write now happens against the current
row inside the write statement (json_set for extra, a re-asserted happened_at IS NULL
predicate for the other), so nothing else in the blob is touched.

I do not have push access here, so this comes from my fork and I could not add you as a formal
reviewer. Two things worth flagging:

  • one deliberate behaviour change is called out in the description (a later concurrent sweep no
    longer overwrites an earlier mentioned_at stamp, which restores the documented contract);
  • the CLA check is pending and needs a human to resolve.

@pufit

pufit commented Aug 4, 2026

Copy link
Copy Markdown
Member

@oranjeai memU will be rewritten and sunset soon.

Please don't open PRs to Nerve unless:

  1. it's critical performance problem (e.g. lag)
  2. it is something that makes your work easier

All other fixes we will be handling ourself. Close all already opened PRs that don't fit these criteria above.

@pufit

pufit commented Aug 4, 2026

Copy link
Copy Markdown
Member

Please also update your MEMORY.md to remember this

@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 commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

Understood, and done on both counts.

Closed all 13 of my open Nerve PRs unmerged (#247-#253, #255-#260 — 11 memU, 2 plans), each with a
comment citing this directive. None of them fit the two criteria: they were correctness fixes, not
performance problems, and none was tooling for my own work.

The rule is now in my MEMORY.md, which is injected into every session, so it survives my session
amnesia: no Nerve PR unless it is a critical performance problem or it makes my own work easier, and
nerve/memory/** is off-limits entirely given the rewrite. If I hit a genuine Nerve defect outside
those bounds I will record it and move on rather than open a PR.

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.

3 participants