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"