From c5a2b1c2441f7598172a15b7e0997b8c08df6c9e Mon Sep 17 00:00:00 2001 From: J0nd1sk <99093160+J0nd1sk@users.noreply.github.com> Date: Tue, 30 Jun 2026 00:21:25 -0700 Subject: [PATCH] fix(fts): scope memories_fts update triggers to indexed columns `memories_fts` is external-content FTS5 maintained by two `AFTER UPDATE ON memories` triggers with no column scope. Every metadata-only write fired the delete+reinsert pair, so each `cmd_memory_search` (via `_retrieval_practice_boost` bumping `confidence`/`recalled_count` on every hit) eroded the inverted index. Searches progressively lost rows; `docsize` bled out. `q_value`, `trust_score`, `ewc_importance` and other metadata writes did the same. Scope both update triggers to `AFTER UPDATE OF content, category, tags, indexed, retired_at`. Metadata churn no longer touches FTS; `indexed` and `retired_at` stay in scope so promotion and retire still maintain the index. - `init_schema.sql`: scoped triggers - migration `083`: drop and recreate scoped on existing DBs, plus a one-time rebuild to repair indexes already corrupted in the field - `mcp_server` startup: rescope legacy triggers on installs that never run migrations (pip wheels do not ship `db/migrations/`) - `_ensure_fts_index_consistent` now returns `False` on a healthy index instead of `None`, matching its docstring (surfaced by the new tests) The search-quality bench was itself degraded by this bug; the fix raises overall `p_at_1` from 0.60 to 0.80 and `recall@10` from 0.51 to 0.85, so the baseline is refreshed. --- db/migrations/083_fts_trigger_scope.sql | 31 ++++ src/agentmemory/db/init_schema.sql | 14 +- src/agentmemory/mcp_server.py | 51 +++++- tests/bench/baselines/search_quality.json | 70 ++++---- tests/test_memories_fts_trigger_scope.py | 186 ++++++++++++++++++++++ 5 files changed, 307 insertions(+), 45 deletions(-) create mode 100644 db/migrations/083_fts_trigger_scope.sql create mode 100644 tests/test_memories_fts_trigger_scope.py diff --git a/db/migrations/083_fts_trigger_scope.sql b/db/migrations/083_fts_trigger_scope.sql new file mode 100644 index 0000000..6a96fe2 --- /dev/null +++ b/db/migrations/083_fts_trigger_scope.sql @@ -0,0 +1,31 @@ +-- Migration 083: scope memories_fts update triggers (issue #152) +-- +-- The unscoped AFTER UPDATE triggers fired on every metadata write, so each +-- memory_search (via _retrieval_practice_boost) eroded the external-content +-- FTS5 index. Scope them to the indexed columns. The one-time rebuild +-- repairs indexes already corrupted in the field. +-- +-- Rollback: +-- restore the unscoped triggers from migration 052 +-- DELETE FROM schema_version WHERE version = 83; +-- +-- IDEMPOTENT. + +DROP TRIGGER IF EXISTS memories_fts_update_delete; +DROP TRIGGER IF EXISTS memories_fts_update_insert; + +CREATE TRIGGER memories_fts_update_delete AFTER UPDATE OF content, category, tags, indexed, retired_at ON memories WHEN old.indexed = 1 BEGIN + INSERT INTO memories_fts(memories_fts, rowid, content, category, tags) + VALUES ('delete', old.id, old.content, old.category, old.tags); +END; + +CREATE TRIGGER memories_fts_update_insert AFTER UPDATE OF content, category, tags, indexed, retired_at ON memories WHEN new.indexed = 1 AND new.retired_at IS NULL BEGIN + INSERT INTO memories_fts(rowid, content, category, tags) + VALUES (new.id, new.content, new.category, new.tags); +END; + +INSERT INTO memories_fts(memories_fts) VALUES('rebuild'); + +INSERT OR IGNORE INTO schema_version (version, description, applied_at) +VALUES (83, 'scope memories_fts update triggers (issue #152)', + strftime('%Y-%m-%dT%H:%M:%S', 'now')); diff --git a/src/agentmemory/db/init_schema.sql b/src/agentmemory/db/init_schema.sql index 2dc37ff..255db9a 100644 --- a/src/agentmemory/db/init_schema.sql +++ b/src/agentmemory/db/init_schema.sql @@ -132,19 +132,15 @@ CREATE TRIGGER memories_fts_insert AFTER INSERT ON memories WHEN new.indexed = 1 INSERT INTO memories_fts(rowid, content, category, tags) VALUES (new.id, new.content, new.category, new.tags); END; --- Split into two triggers so 0→1 promotion correctly adds to FTS without double-delete. --- Added `NEW.retired_at IS NULL` guard on the INSERT leg so retire UPDATEs --- (retired_at NULL → non-NULL) do not re-insert the row. The companion --- trg_memories_fts_purge_on_retire trigger near the end of this file does --- the actual DELETE at the retire transition; without this guard, the --- 'delete' command issued there is silently no-op'd by FTS5 statement-level --- batching against the pending INSERT. -CREATE TRIGGER memories_fts_update_delete AFTER UPDATE ON memories WHEN old.indexed = 1 BEGIN +-- Update triggers scoped to FTS columns (plus indexed, retired_at) so +-- metadata-only updates don't churn the index (issue #152). The +-- retired_at IS NULL guard drops the re-insert at the retire transition. +CREATE TRIGGER memories_fts_update_delete AFTER UPDATE OF content, category, tags, indexed, retired_at ON memories WHEN old.indexed = 1 BEGIN INSERT INTO memories_fts(memories_fts, rowid, content, category, tags) VALUES ('delete', old.id, old.content, old.category, old.tags); END; -CREATE TRIGGER memories_fts_update_insert AFTER UPDATE ON memories WHEN new.indexed = 1 AND new.retired_at IS NULL BEGIN +CREATE TRIGGER memories_fts_update_insert AFTER UPDATE OF content, category, tags, indexed, retired_at ON memories WHEN new.indexed = 1 AND new.retired_at IS NULL BEGIN INSERT INTO memories_fts(rowid, content, category, tags) VALUES (new.id, new.content, new.category, new.tags); END; diff --git a/src/agentmemory/mcp_server.py b/src/agentmemory/mcp_server.py index e16cf13..408c4b2 100755 --- a/src/agentmemory/mcp_server.py +++ b/src/agentmemory/mcp_server.py @@ -346,10 +346,44 @@ def _ensure_fts_index_consistent(conn) -> bool: return False except sqlite3.Error: return False - return False +_SCOPED_FTS_UPDATE_TRIGGERS = """ +DROP TRIGGER IF EXISTS memories_fts_update_delete; +DROP TRIGGER IF EXISTS memories_fts_update_insert; +CREATE TRIGGER memories_fts_update_delete AFTER UPDATE OF content, category, tags, indexed, retired_at ON memories WHEN old.indexed = 1 BEGIN + INSERT INTO memories_fts(memories_fts, rowid, content, category, tags) + VALUES ('delete', old.id, old.content, old.category, old.tags); +END; +CREATE TRIGGER memories_fts_update_insert AFTER UPDATE OF content, category, tags, indexed, retired_at ON memories WHEN new.indexed = 1 AND new.retired_at IS NULL BEGIN + INSERT INTO memories_fts(rowid, content, category, tags) + VALUES (new.id, new.content, new.category, new.tags); +END; +""" + + +def _ensure_fts_triggers_scoped(conn) -> bool: + """Replace legacy unscoped memories_fts update triggers with column-scoped + ones (issue #152). Idempotent; returns True only if it rewrote them.""" + try: + rows = conn.execute( + "SELECT sql FROM sqlite_master WHERE type='trigger' " + "AND name IN ('memories_fts_update_delete', 'memories_fts_update_insert')" + ).fetchall() + except sqlite3.Error: + return False + if not rows or all(r[0] and "AFTER UPDATE OF" in r[0].upper() for r in rows): + return False + try: + conn.executescript(_SCOPED_FTS_UPDATE_TRIGGERS) + conn.execute("INSERT INTO memories_fts(memories_fts) VALUES('rebuild')") + conn.commit() + return True + except sqlite3.Error: + return False + + def ensure_agent(conn, agent_id: str) -> None: if not agent_id: return @@ -3640,6 +3674,21 @@ def _ensure_db_initialized() -> None: file=sys.stderr, ) + # Heal legacy unscoped memories_fts update triggers (issue #152) on + # installs that predate migration 083 or never run migrations. + try: + conn = sqlite3.connect(str(db_path), timeout=10) + try: + if _ensure_fts_triggers_scoped(conn): + print( + f"brainctl-mcp: rescoped memories_fts update triggers for {db_path}", + file=sys.stderr, + ) + finally: + conn.close() + except sqlite3.Error: + pass + def run(): """Synchronous entry point for pyproject.toml console_scripts.""" diff --git a/tests/bench/baselines/search_quality.json b/tests/bench/baselines/search_quality.json index 7ddb407..246ef1d 100644 --- a/tests/bench/baselines/search_quality.json +++ b/tests/bench/baselines/search_quality.json @@ -2,27 +2,27 @@ "by_category": { "ambiguous": { "count": 1, - "mrr": 0.0, - "ndcg_at_5": 0.0, - "p_at_1": 0.0, - "p_at_5": 0.0, - "recall_at_5": 0.0 + "mrr": 1.0, + "ndcg_at_5": 1.0, + "p_at_1": 1.0, + "p_at_5": 0.6, + "recall_at_5": 1.0 }, "decision": { "count": 3, - "mrr": 0.3333, - "ndcg_at_5": 0.3186, - "p_at_1": 0.3333, - "p_at_5": 0.1333, - "recall_at_5": 0.3333 + "mrr": 1.0, + "ndcg_at_5": 1.0, + "p_at_1": 1.0, + "p_at_5": 0.3333, + "recall_at_5": 1.0 }, "entity": { "count": 7, - "mrr": 0.7143, - "ndcg_at_5": 0.6099, - "p_at_1": 0.7143, - "p_at_5": 0.2, - "recall_at_5": 0.4524 + "mrr": 1.0, + "ndcg_at_5": 0.8474, + "p_at_1": 1.0, + "p_at_5": 0.2857, + "recall_at_5": 0.6429 }, "negative": { "count": 1, @@ -35,37 +35,37 @@ "procedural": { "count": 4, "mrr": 0.875, - "ndcg_at_5": 0.6333, + "ndcg_at_5": 0.8032, "p_at_1": 0.75, - "p_at_5": 0.25, - "recall_at_5": 0.75 + "p_at_5": 0.3, + "recall_at_5": 0.875 }, "temporal": { "count": 2, - "mrr": 1.0, - "ndcg_at_5": 0.8936, - "p_at_1": 1.0, - "p_at_5": 0.3, - "recall_at_5": 0.75 + "mrr": 0.4167, + "ndcg_at_5": 0.5335, + "p_at_1": 0.0, + "p_at_5": 0.4, + "recall_at_5": 1.0 }, "troubleshooting": { "count": 2, - "mrr": 0.5, - "ndcg_at_5": 0.3066, - "p_at_1": 0.5, - "p_at_5": 0.1, - "recall_at_5": 0.25 + "mrr": 1.0, + "ndcg_at_5": 1.0, + "p_at_1": 1.0, + "p_at_5": 0.3, + "recall_at_5": 1.0 } }, "k": 10, "overall": { - "mrr": 0.625, + "mrr": 0.8667, "n_queries": 20, - "ndcg_at_10": 0.5579, - "ndcg_at_5": 0.5579, - "p_at_1": 0.6, - "p_at_5": 0.18, - "recall_at_10": 0.5083, - "recall_at_5": 0.5083 + "ndcg_at_10": 0.8606, + "ndcg_at_5": 0.8606, + "p_at_1": 0.8, + "p_at_5": 0.31, + "recall_at_10": 0.85, + "recall_at_5": 0.85 } } \ No newline at end of file diff --git a/tests/test_memories_fts_trigger_scope.py b/tests/test_memories_fts_trigger_scope.py new file mode 100644 index 0000000..1ba0786 --- /dev/null +++ b/tests/test_memories_fts_trigger_scope.py @@ -0,0 +1,186 @@ +"""Regression tests for FTS index maintenance on the ``memories`` table. + +Issue #152 (FTS index corruption on memory_search): the ``memories_fts`` +external-content FTS5 index is maintained by two ``AFTER UPDATE ON memories`` +triggers that are *unscoped* (they fire on every column update). A +metadata-only UPDATE -- e.g. ``_retrieval_practice_boost`` bumping +``confidence`` / ``recalled_count`` on each search hit -- therefore fires the +delete+reinsert pair even though no FTS-indexed column (content/category/tags) +changed. The net effect erodes the inverted index, so repeated searches +progressively lose rows from ``memory_search``. + +Invariant under test: a metadata-only UPDATE must NOT change the FTS index, +while genuine content/category/tags edits, retire, and delete MUST still +maintain it. Exercised against the full production schema (``cli_db`` fixture) +so the real, effective triggers (init_schema + migrations) are what runs. +""" +from __future__ import annotations + +import sqlite3 +import sys +from pathlib import Path + +import pytest + +SRC = Path(__file__).resolve().parent.parent / "src" +if str(SRC) not in sys.path: + sys.path.insert(0, str(SRC)) + +_impl = pytest.importorskip("agentmemory._impl") + + +def _add(conn, content, category="note", tags="", agent="tester"): + cur = conn.execute( + "INSERT INTO memories (agent_id, category, content, tags, indexed, " + "created_at, updated_at) VALUES (?, ?, ?, ?, 1, " + "strftime('%Y-%m-%dT%H:%M:%S','now'), strftime('%Y-%m-%dT%H:%M:%S','now'))", + (agent, category, content, tags), + ) + return cur.lastrowid + + +def _match(conn, token): + return conn.execute( + "SELECT count(*) FROM memories_fts WHERE memories_fts MATCH ?", (token,) + ).fetchone()[0] + + +def _docsize(conn): + return conn.execute("SELECT count(*) FROM memories_fts_docsize").fetchone()[0] + + +def _rebuild(conn): + conn.execute("INSERT INTO memories_fts(memories_fts) VALUES('rebuild')") + conn.commit() + + +def _integrity_ok(conn): + """True if the FTS5 internal integrity-check passes (no exception).""" + try: + conn.execute("INSERT INTO memories_fts(memories_fts) VALUES('integrity-check')") + return True + except sqlite3.DatabaseError: + return False + + +@pytest.fixture +def conn(cli_db): + c = sqlite3.connect(str(cli_db)) + c.row_factory = sqlite3.Row + yield c + c.close() + + +def test_metadata_update_preserves_fts_index(conn): + """A single metadata-only UPDATE must not evict the row from FTS (#152).""" + mid = _add(conn, "kelly village infrastructure") + conn.commit() + _rebuild(conn) + assert _match(conn, "kelly") == 1 + base = _docsize(conn) + + # The exact column set _retrieval_practice_boost touches -- no + # content/category/tags among them. + conn.execute( + "UPDATE memories SET confidence = MIN(1.0, confidence + 0.01), " + "recalled_count = recalled_count + 1, " + "last_recalled_at = strftime('%Y-%m-%dT%H:%M:%S','now'), " + "labile_until = strftime('%Y-%m-%dT%H:%M:%S','now','+2 hours') " + "WHERE id = ?", + (mid,), + ) + conn.commit() + + assert _match(conn, "kelly") == 1, "metadata update evicted the row from FTS" + assert _docsize(conn) == base, "metadata update changed FTS docsize" + assert _integrity_ok(conn), "metadata update left FTS index inconsistent" + + +def test_repeated_metadata_updates_do_not_erode_index(conn): + """Repeated metadata UPDATEs reproduce the docsize-bleed symptom (#152).""" + ids = [ + _add(conn, "alpha kelly village"), + _add(conn, "beta howler routine"), + _add(conn, "gamma api ratelimit"), + ] + conn.commit() + _rebuild(conn) + base = _docsize(conn) + + for _ in range(5): + for mid in ids: + conn.execute( + "UPDATE memories SET confidence = MIN(1.0, confidence + 0.01), " + "recalled_count = recalled_count + 1 WHERE id = ?", + (mid,), + ) + conn.commit() + + assert _docsize(conn) == base, "repeated metadata updates eroded the FTS index" + assert _match(conn, "kelly") == 1 + assert _match(conn, "howler") == 1 + assert _match(conn, "ratelimit") == 1 + + +def test_real_retrieval_practice_preserves_index(conn): + """The real driver: _retrieval_practice_boost must not erode FTS (#152).""" + ids = [ + _add(conn, "alpha kelly village"), + _add(conn, "beta howler routine"), + _add(conn, "gamma api ratelimit"), + ] + conn.commit() + _rebuild(conn) + base = _docsize(conn) + + for _ in range(5): + for mid in ids: + _impl._retrieval_practice_boost(conn, mid) + conn.commit() + + assert _docsize(conn) == base, "_retrieval_practice_boost eroded the FTS index" + assert _match(conn, "kelly") == 1 + assert _match(conn, "howler") == 1 + assert _match(conn, "ratelimit") == 1 + + +# --- regression guards: must stay GREEN through the trigger-scoping fix --- + +def test_content_edit_reindexes(conn): + mid = _add(conn, "alpha zebra") + conn.commit() + _rebuild(conn) + assert _match(conn, "zebra") == 1 + + conn.execute( + "UPDATE memories SET content = ?, version = version + 1 WHERE id = ?", + ("beta giraffe", mid), + ) + conn.commit() + assert _match(conn, "zebra") == 0, "stale content still indexed after edit" + assert _match(conn, "giraffe") == 1, "new content not indexed after edit" + + +def test_retire_removes_from_fts(conn): + mid = _add(conn, "kelly village retire") + conn.commit() + _rebuild(conn) + assert _match(conn, "kelly") == 1 + + conn.execute( + "UPDATE memories SET retired_at = strftime('%Y-%m-%dT%H:%M:%S','now') WHERE id = ?", + (mid,), + ) + conn.commit() + assert _match(conn, "kelly") == 0, "retired memory still in FTS index" + + +def test_delete_removes_from_fts(conn): + mid = _add(conn, "kelly village delete") + conn.commit() + _rebuild(conn) + assert _match(conn, "kelly") == 1 + + conn.execute("DELETE FROM memories WHERE id = ?", (mid,)) + conn.commit() + assert _match(conn, "kelly") == 0, "deleted memory still in FTS index"