From 038157ecf3e32d0a25b3053cb1a993eca3f31025 Mon Sep 17 00:00:00 2001 From: Fixer Date: Sun, 2 Aug 2026 13:35:46 +0200 Subject: [PATCH] fix(memory): don't take a write lock to backfill nothing on every open (#368) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _migrate_backfill_review_schedule ran its UPDATE unconditionally on every ReflectionStore open, so every component that constructs a store took a write transaction at daemon start even when no row needed scheduling — which is the steady state. Guard it with a read-only EXISTS probe and write only when there is something to schedule. Note this is deliberately NOT turned into a one-shot migration despite its name and old docstring: insert() never sets next_review_date, so this UPDATE is also what makes newly created memories reviewable. Running it once would silently stop the review system from picking up anything inserted later. The docstring now says so, and a test pins the behaviour. The row predicate moves into _NEEDS_REVIEW_DATE, shared by the guard and the UPDATE so they cannot drift: a narrower guard would skip real work, a wider one would write when there is nothing to do. --- src/pinky_memory/store.py | 43 +++++--- tests/test_memory_backfill_review_writes.py | 109 ++++++++++++++++++++ 2 files changed, 140 insertions(+), 12 deletions(-) create mode 100644 tests/test_memory_backfill_review_writes.py diff --git a/src/pinky_memory/store.py b/src/pinky_memory/store.py index 7f329aec..65a92577 100644 --- a/src/pinky_memory/store.py +++ b/src/pinky_memory/store.py @@ -134,6 +134,18 @@ class InvalidQueryEmbeddingError(ValueError): """ +# Rows that still need a review date. Shared by the guard and the UPDATE in +# _migrate_backfill_review_schedule so the two can never drift apart: a guard +# that is narrower than the write would skip work, a wider one would write when +# there is nothing to do. salience >= 4 is protected — those stay NULL and are +# never auto-reviewed. +_NEEDS_REVIEW_DATE = """active = 1 + AND next_review_date IS NULL + AND salience < 4 + AND type != 'continuation' + AND no_recall = 0""" + + def _now_iso() -> str: return datetime.now(timezone.utc).isoformat() @@ -230,19 +242,26 @@ def _migrate_add_column(self, column: str, definition: str) -> None: pass # column already exists def _migrate_backfill_review_schedule(self) -> None: - """Backfill next_review_date for existing active memories on first run.""" + """Give unscheduled active memories a review date, writing only if any exist. + + Despite the "migration" name this is not one-shot: insert() leaves + next_review_date NULL, so this is also what makes newly created + memories reviewable. It must keep running on every open — it just must + not take a write lock when there is nothing to schedule, which is the + steady state and used to cost one write transaction per store opened + at daemon start. #368 + """ with self._lock: - # Only backfill rows that have NULL next_review_date AND salience < 4 - # (salience >= 4 are protected and stay NULL = never auto-reviewed) - self._conn.execute(""" - UPDATE reflections - SET next_review_date = date(created_at, '+30 days') - WHERE active = 1 - AND next_review_date IS NULL - AND salience < 4 - AND type != 'continuation' - AND no_recall = 0 - """) + unscheduled = self._conn.execute( + f"SELECT 1 FROM reflections WHERE {_NEEDS_REVIEW_DATE} LIMIT 1" + ).fetchone() + if unscheduled is None: + return + self._conn.execute( + f"""UPDATE reflections + SET next_review_date = date(created_at, '+30 days') + WHERE {_NEEDS_REVIEW_DATE}""" + ) self._conn.commit() def _migrate_create_memory_events(self) -> None: diff --git a/tests/test_memory_backfill_review_writes.py b/tests/test_memory_backfill_review_writes.py new file mode 100644 index 00000000..4d691603 --- /dev/null +++ b/tests/test_memory_backfill_review_writes.py @@ -0,0 +1,109 @@ +"""Opening a ReflectionStore must not write when there is nothing to backfill (#368). + +_migrate_backfill_review_schedule ran an unconditional UPDATE on every open, so +every component that constructs a store took a write lock at daemon start even +on a database where every row was already scheduled. + +Careful: despite its "on first run" docstring, this UPDATE is load-bearing. +insert() never sets next_review_date, so this is also what schedules *new* +memories. Making it a one-time migration would silently stop the review system +from ever picking up anything inserted later — hence the tests below pin the +scheduling behaviour, not just the write. +""" +from __future__ import annotations + +import sqlite3 +from pathlib import Path + +from pinky_memory.store import ReflectionStore +from pinky_memory.types import Reflection, ReflectionType + + +def _trace_opens(monkeypatch) -> list[str]: + """Capture every SQL statement the next ReflectionStore(s) execute.""" + seen: list[str] = [] + real_connect = sqlite3.connect + + def traced(*args, **kwargs): + conn = real_connect(*args, **kwargs) + conn.set_trace_callback(seen.append) + return conn + + monkeypatch.setattr(sqlite3, "connect", traced) + return seen + + +def _backfill_writes(seen: list[str]) -> list[str]: + return [ + s + for s in seen + if s.strip().upper().startswith("UPDATE REFLECTIONS") and "next_review_date" in s + ] + + +def _scheduled(db: Path, reflection_id: str) -> str | None: + conn = sqlite3.connect(str(db)) + try: + row = conn.execute( + "SELECT next_review_date FROM reflections WHERE id = ?", (reflection_id,) + ).fetchone() + return row[0] + finally: + conn.close() + + +def _seed(db: Path, **kwargs) -> str: + store = ReflectionStore(str(db)) + try: + r = store.insert( + Reflection( + type=ReflectionType.fact, + content="il daemon riavvia il gateway", + **kwargs, + ) + ) + return r.id + finally: + store.close() + + +class TestBackfillOnlyWritesWhenNeeded: + def test_reopening_a_fully_scheduled_db_writes_nothing(self, tmp_path, monkeypatch): + db = tmp_path / "memory.db" + _seed(db) + ReflectionStore(str(db)).close() # this open does the backfill + + seen = _trace_opens(monkeypatch) + ReflectionStore(str(db)).close() + + writes = _backfill_writes(seen) + assert writes == [], f"reopen still takes a write lock to backfill nothing: {writes}" + + def test_an_empty_db_writes_nothing(self, tmp_path, monkeypatch): + db = tmp_path / "memory.db" + ReflectionStore(str(db)).close() + + seen = _trace_opens(monkeypatch) + ReflectionStore(str(db)).close() + + assert _backfill_writes(seen) == [] + + +class TestBackfillStillSchedules: + def test_a_memory_inserted_later_is_scheduled_on_the_next_open(self, tmp_path): + """Load-bearing: insert() leaves next_review_date NULL, this fills it in.""" + db = tmp_path / "memory.db" + rid = _seed(db) + assert _scheduled(db, rid) is None, "insert() unexpectedly schedules the review itself" + + ReflectionStore(str(db)).close() + + assert _scheduled(db, rid) is not None, "a new memory never became reviewable" + + def test_protected_high_salience_memories_stay_unscheduled(self, tmp_path): + db = tmp_path / "memory.db" + rid = _seed(db, salience=5) + + ReflectionStore(str(db)).close() + + assert _scheduled(db, rid) is None, "a protected memory was pulled into the review cycle"