fix(memory): don't take a write lock to backfill nothing on every open (#368)

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:

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"