From 608a56ed7f50926ecfae1db39447b280a0bc4d1e Mon Sep 17 00:00:00 2001 From: briandevans <252620095+briandevans@users.noreply.github.com> Date: Sun, 9 Aug 2026 18:39:14 -0700 Subject: [PATCH] fix(state): stop rebuilding the whole FTS index on every open when the trigram tokenizer is missing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_init_schema` decided whether the FTS triggers needed repair by comparing the live trigger count against `len(_FTS_TRIGGERS)`, the full six-name set. Three of those six are the `messages_fts_trigram_*` triggers, and they are declared only inside `FTS_TRIGRAM_SQL` / `LEGACY_FTS_TRIGRAM_SQL`, whose `CREATE VIRTUAL TABLE ... tokenize='trigram'` needs a tokenizer SQLite only gained in 3.34. On an older build `_ensure_fts_schema` soft-fails that DDL by design (via `_is_trigram_unavailable_error`) and returns False, so those three triggers can never be created. The count is therefore pinned at 3, `3 < 6` is permanently true, and the repair path ran on every single `SessionDB` open, forever, while holding the SQLite write lock. It never converged: every `hermes` command, gateway start, dashboard request and cron tick paid a full re-index of the message corpus. That is ordinary LTS territory — Ubuntu 20.04 ships 3.31, RHEL/CentOS 8 and Alibaba Cloud Linux ship 3.26, and Hermes has no minimum-SQLite gate precisely because it is supposed to degrade gracefully here. The v23 repair also ends by clearing `fts_rebuild_high_water` and `fts_rebuild_progress`, which is correct after a genuine full rebuild but means an interrupted `hermes sessions optimize-storage` silently lost its resume point on the next open, restarting the chunked backfill from zero every time. Fix: keep `_FTS_TRIGGERS` as the single source of truth and derive two subsets from it, then measure each half against the DDL that can actually create it. `_fts_trigger_count` takes an optional `names` sequence (defaulting to the full set, so no caller changes), and both branches gate on `base_triggers_missing or (trigram_enabled and trigram_triggers_missing)`. The counts are still taken before the DDL runs so they describe the pre-repair state, while `trigram_enabled` is only known afterwards — hence the combination at the `if` rather than at the assignment. Behaviour is unchanged wherever the tokenizer exists: a genuinely missing trigram trigger on a capable host still triggers the rebuild. Only the permanently unsatisfiable comparison changes. --- hermes_state_schema.py | 65 ++++++-- tests/test_hermes_state.py | 310 +++++++++++++++++++++++++++++++++++++ 2 files changed, 365 insertions(+), 10 deletions(-) diff --git a/hermes_state_schema.py b/hermes_state_schema.py index 4a8ce5ca63..d234364f77 100644 --- a/hermes_state_schema.py +++ b/hermes_state_schema.py @@ -11,7 +11,7 @@ module-level constants live in hermes_state_common. import logging import json import sqlite3 -from typing import Dict, Optional +from typing import Dict, Optional, Sequence from hermes_constants import get_hermes_home from hermes_state_common import ( @@ -39,6 +39,19 @@ logger = logging.getLogger("hermes_state") # in-memory SQLite database, so derive the statements once per process. _READ_PROBE_STATEMENTS: Optional[tuple] = None +# _FTS_TRIGGERS is the full canonical set, but its two halves have different +# availability: the trigram triggers are declared ONLY by FTS_TRIGRAM_SQL / +# LEGACY_FTS_TRIGRAM_SQL, whose CREATE VIRTUAL TABLE needs the trigram +# tokenizer (SQLite >= 3.34). On a build without it, _ensure_fts_schema +# soft-fails that DDL, so those three triggers can never exist and any check +# for "all six are present" is permanently unsatisfiable. Split the set so a +# trigger's absence is only ever measured against the DDL that can create it. +# The two subsets are exhaustive and disjoint by construction (base is the +# complement of trigram); test_fts_trigger_subsets_match_the_ddl pins them +# against the DDL those triggers actually come from. +_FTS_TRIGRAM_TRIGGERS = tuple(n for n in _FTS_TRIGGERS if "_trigram_" in n) +_FTS_BASE_TRIGGERS = tuple(n for n in _FTS_TRIGGERS if n not in _FTS_TRIGRAM_TRIGGERS) + def schema_read_probe_statements() -> tuple: """SELECT statements that fail iff a live store is behind SCHEMA_SQL. @@ -147,12 +160,25 @@ class SessionSchemaMixin: pass @staticmethod - def _fts_trigger_count(cursor: sqlite3.Cursor) -> int: - placeholders = ",".join("?" for _ in _FTS_TRIGGERS) + def _fts_trigger_count( + cursor: sqlite3.Cursor, + names: Sequence[str] = _FTS_TRIGGERS, + ) -> int: + """Count how many of *names* currently exist as triggers. + + Defaults to the full canonical set so existing callers are unchanged; + callers that need to know whether one HALF of the set is intact pass + _FTS_BASE_TRIGGERS or _FTS_TRIGRAM_TRIGGERS. + """ + if not names: + # "name IN ()" is a syntax error in SQLite, and nothing can be + # missing from an empty set anyway. + return 0 + placeholders = ",".join("?" for _ in names) row = cursor.execute( f"SELECT COUNT(*) FROM sqlite_master " f"WHERE type = 'trigger' AND name IN ({placeholders})", - _FTS_TRIGGERS, + tuple(names), ).fetchone() return int(row[0] if not isinstance(row, sqlite3.Row) else row[0]) @@ -1271,8 +1297,17 @@ class SessionSchemaMixin: self._trigram_available = False self._fts_cjk_available = False elif legacy_fts: - triggers_need_repair = ( - self._fts_trigger_count(cursor) < len(_FTS_TRIGGERS) + # Measure BEFORE the DDL below runs, so these describe the + # pre-repair state. Whether the trigram half is even + # creatable is only known AFTER _ensure_fts_schema, which is + # why the two halves are combined at the `if`, not here. + base_triggers_missing = ( + self._fts_trigger_count(cursor, _FTS_BASE_TRIGGERS) + < len(_FTS_BASE_TRIGGERS) + ) + trigram_triggers_missing = ( + self._fts_trigger_count(cursor, _FTS_TRIGRAM_TRIGGERS) + < len(_FTS_TRIGRAM_TRIGGERS) ) self._fts_enabled = self._ensure_fts_schema( cursor, "messages_fts", LEGACY_FTS_SQL @@ -1282,7 +1317,9 @@ class SessionSchemaMixin: cursor, "messages_fts_trigram", LEGACY_FTS_TRIGRAM_SQL ) self._trigram_available = trigram_enabled - if triggers_need_repair: + if base_triggers_missing or ( + trigram_enabled and trigram_triggers_missing + ): self._run_admitted_startup_rebuild( cursor, lambda: self._rebuild_legacy_fts_indexes( @@ -1290,8 +1327,14 @@ class SessionSchemaMixin: ), ) else: - triggers_need_repair = ( - self._fts_trigger_count(cursor) < len(_FTS_TRIGGERS) + # Same split as the legacy branch above, same reason. + base_triggers_missing = ( + self._fts_trigger_count(cursor, _FTS_BASE_TRIGGERS) + < len(_FTS_BASE_TRIGGERS) + ) + trigram_triggers_missing = ( + self._fts_trigger_count(cursor, _FTS_TRIGRAM_TRIGGERS) + < len(_FTS_TRIGRAM_TRIGGERS) ) self._fts_enabled = self._ensure_fts_schema( cursor, "messages_fts", FTS_SQL @@ -1305,7 +1348,9 @@ class SessionSchemaMixin: cursor, "messages_fts_trigram", FTS_TRIGRAM_SQL ) self._trigram_available = trigram_enabled - if triggers_need_repair: + if base_triggers_missing or ( + trigram_enabled and trigram_triggers_missing + ): self._run_admitted_startup_rebuild( cursor, lambda: self._rebuild_fts_indexes( diff --git a/tests/test_hermes_state.py b/tests/test_hermes_state.py index 6f03a945b6..28480c1d15 100644 --- a/tests/test_hermes_state.py +++ b/tests/test_hermes_state.py @@ -1877,6 +1877,316 @@ class TestReconcileColumnsErrorHandling: assert "last_read_at" in cols +class TestFtsRebuildLoopWithoutTrigram: + """A trigram-less SQLite build must not re-index the store on every open. + + The three ``messages_fts_trigram_*`` triggers are declared only by the + trigram DDL, whose ``CREATE VIRTUAL TABLE ... tokenize='trigram'`` needs a + tokenizer SQLite only gained in 3.34 — Ubuntu 20.04 (3.31), RHEL/CentOS 8 + (3.26) and Amazon Linux 2 all ship older. ``_ensure_fts_schema`` + soft-fails that DDL there by design, so those three triggers can never + exist, and startup's "are all six canonical triggers present?" check was + therefore permanently unsatisfiable: the full FTS repair ran on every + single ``SessionDB`` open, holding the write lock, and never converged. + + The v23 repair also clears the deferred-rebuild resume markers, so an + interrupted ``hermes sessions optimize-storage`` silently lost its place + every time the store was reopened. + """ + + @staticmethod + def _trace(monkeypatch, statements, *, trigram): + """Record every statement SessionDB executes during an open. + + ``trigram=False`` additionally routes connections through the + module's existing ``_NoTrigramConnection``, which raises + ``no such tokenizer: trigram`` for the trigram DDL exactly as an + older SQLite does. + """ + real_connect = sqlite3.connect + + def connect(*args, **kwargs): + if not trigram: + kwargs["factory"] = _NoTrigramConnection + conn = real_connect(*args, **kwargs) + conn.set_trace_callback(statements.append) + return conn + + monkeypatch.setattr("hermes_state.sqlite3.connect", connect) + + @staticmethod + def _rebuilds(statements): + """External-content 'rebuild' commands seen in *statements*.""" + return [sql for sql in statements if "VALUES('rebuild')" in "".join(sql.split())] + + @staticmethod + def _legacy_wipes(statements): + """Legacy inline repair wipes the index before reinserting every row.""" + return [ + sql for sql in statements + if "".join(sql.split()).upper().startswith("DELETEFROMMESSAGES_FTS") + ] + + @staticmethod + def _seed(db_path): + db = SessionDB(db_path=db_path) + try: + db.create_session(session_id="s1", source="cli") + for i in range(5): + db.append_message("s1", role="user", content=f"payload {i} zebra") + finally: + db.close() + + @staticmethod + def _build_legacy_inline_db(db_path): + """A v22 store as it exists on a host that never had the tokenizer. + + Only the three base inline triggers were ever creatable there, so — + unlike the migration fixtures elsewhere in this file — this build + deliberately has no trigram table and no trigram triggers. + """ + conn = sqlite3.connect(str(db_path)) + try: + conn.executescript(SCHEMA_SQL) + conn.executescript(""" + DROP TABLE IF EXISTS messages_fts; + DROP TABLE IF EXISTS messages_fts_trigram; + DROP VIEW IF EXISTS messages_fts_trigram_src; + + CREATE VIRTUAL TABLE messages_fts USING fts5(content); + + CREATE TRIGGER messages_fts_insert AFTER INSERT ON messages BEGIN + INSERT INTO messages_fts(rowid, content) VALUES ( + new.id, + COALESCE(new.content, '') || ' ' || COALESCE(new.tool_name, '') + || ' ' || COALESCE(new.tool_calls, '') + ); + END; + + CREATE TRIGGER messages_fts_delete AFTER DELETE ON messages BEGIN + DELETE FROM messages_fts WHERE rowid = old.id; + END; + + CREATE TRIGGER messages_fts_update + AFTER UPDATE OF content, tool_name, tool_calls ON messages BEGIN + DELETE FROM messages_fts WHERE rowid = old.id; + INSERT INTO messages_fts(rowid, content) VALUES ( + new.id, + COALESCE(new.content, '') || ' ' || COALESCE(new.tool_name, '') + || ' ' || COALESCE(new.tool_calls, '') + ); + END; + """) + conn.execute("DELETE FROM schema_version") + conn.execute("INSERT INTO schema_version (version) VALUES (22)") + conn.execute( + "INSERT INTO sessions (id, source, started_at) VALUES ('s1', 'cli', ?)", + (time.time(),), + ) + for i in range(5): + conn.execute( + "INSERT INTO messages (session_id, timestamp, role, content) " + "VALUES ('s1', ?, 'user', ?)", + (time.time(), f"legacy payload {i} zebra"), + ) + conn.commit() + finally: + conn.close() + + def test_fts_trigger_subsets_match_the_ddl(self): + """The split must track the DDL each trigger actually comes from. + + The gate is only correct while every trigger classified as "trigram" + is one the trigram DDL creates, and every other one is created by DDL + that always works. Renaming a trigger without updating its DDL would + otherwise silently reintroduce an unsatisfiable check. + """ + from hermes_state_common import ( + FTS_SQL, + FTS_TRIGRAM_SQL, + LEGACY_FTS_SQL, + LEGACY_FTS_TRIGRAM_SQL, + _FTS_TRIGGERS, + ) + from hermes_state_schema import _FTS_BASE_TRIGGERS, _FTS_TRIGRAM_TRIGGERS + + # Exhaustive and disjoint: nothing may fall out of the classification. + assert set(_FTS_BASE_TRIGGERS) | set(_FTS_TRIGRAM_TRIGGERS) == set(_FTS_TRIGGERS) + assert not set(_FTS_BASE_TRIGGERS) & set(_FTS_TRIGRAM_TRIGGERS) + + for name in _FTS_TRIGRAM_TRIGGERS: + assert name in FTS_TRIGRAM_SQL and name in LEGACY_FTS_TRIGRAM_SQL, ( + f"{name} is classified as trigram-only but the trigram DDL " + f"does not create it" + ) + for name in _FTS_BASE_TRIGGERS: + assert name in FTS_SQL and name in LEGACY_FTS_SQL, ( + f"{name} is classified as always-creatable but the base DDL " + f"does not create it" + ) + assert name not in FTS_TRIGRAM_SQL + + def test_missing_trigram_tokenizer_does_not_rebuild_fts_on_every_open( + self, tmp_path, monkeypatch + ): + """v23 branch: the repair must converge instead of firing forever.""" + db_path = tmp_path / "state.db" + statements = [] + self._trace(monkeypatch, statements, trigram=False) + + self._seed(db_path) + + # Second and third opens of an already-initialised store. The trigram + # triggers are still absent and always will be, but nothing is + # actually broken, so there is nothing to repair. + for _ in range(2): + statements.clear() + db = SessionDB(db_path=db_path) + try: + assert db._trigram_available is False + assert self._rebuilds(statements) == [] + # The narrowed gate must not have cost us a working index. + assert len(db.search_messages("zebra")) == 5 + finally: + db.close() + + def test_legacy_inline_fts_without_trigram_does_not_rebuild_on_every_open( + self, tmp_path, monkeypatch + ): + """Legacy (pre-v23) branch: same gate, same permanent repair loop. + + This path is the more expensive of the two — inline tables have no + external-content 'rebuild' source, so the repair deletes the index and + reinserts a concatenation of every row in ``messages``. + """ + db_path = tmp_path / "legacy.db" + self._build_legacy_inline_db(db_path) + + statements = [] + self._trace(monkeypatch, statements, trigram=False) + + for _ in range(2): + statements.clear() + db = SessionDB(db_path=db_path) + try: + assert db._db_has_legacy_inline_fts(db._conn.cursor()) is True + assert db._trigram_available is False + assert self._legacy_wipes(statements) == [] + assert len(db.search_messages("zebra")) == 5 + finally: + db.close() + + def test_pending_fts_rebuild_markers_survive_a_trigramless_open( + self, tmp_path, monkeypatch + ): + """An interrupted optimize-storage must keep its resume point. + + ``_rebuild_fts_indexes`` clears both markers because a full rebuild + genuinely does cover every row. Running it unconditionally on a + trigram-less host therefore threw away the progress of a chunked, + throttled backfill on the very next open. + """ + db_path = tmp_path / "state.db" + statements = [] + self._trace(monkeypatch, statements, trigram=False) + + self._seed(db_path) + + db = SessionDB(db_path=db_path) + try: + for key, value in ( + ("fts_rebuild_high_water", "30"), + ("fts_rebuild_progress", "10"), + ): + db._conn.execute( + "INSERT INTO state_meta (key, value) VALUES (?, ?) " + "ON CONFLICT(key) DO UPDATE SET value = excluded.value", + (key, value), + ) + db._conn.commit() + finally: + db.close() + + db = SessionDB(db_path=db_path) + try: + assert db.get_meta("fts_rebuild_high_water") == "30" + assert db.get_meta("fts_rebuild_progress") == "10" + finally: + db.close() + + def test_missing_base_trigger_still_repairs_once(self, tmp_path, monkeypatch): + """Control: narrowing the gate must not disable genuine repair. + + A base trigger really can go missing (an earlier no-FTS5 runtime drops + them to keep writes alive), and rows written meanwhile are absent from + the index. That still has to be repaired — once, and then converge. + """ + db_path = tmp_path / "state.db" + statements = [] + self._trace(monkeypatch, statements, trigram=False) + + self._seed(db_path) + + db = SessionDB(db_path=db_path) + try: + db._conn.execute("DROP TRIGGER messages_fts_insert") + db._conn.commit() + finally: + db.close() + + statements.clear() + db = SessionDB(db_path=db_path) + try: + assert len(self._rebuilds(statements)) == 1 + assert db._conn.execute( + "SELECT COUNT(*) FROM sqlite_master " + "WHERE type = 'trigger' AND name = 'messages_fts_insert'" + ).fetchone()[0] == 1 + finally: + db.close() + + # …and having repaired it, the next open is quiet again. + statements.clear() + db = SessionDB(db_path=db_path) + try: + assert self._rebuilds(statements) == [] + finally: + db.close() + + def test_missing_trigram_trigger_still_repairs_where_the_tokenizer_exists( + self, tmp_path, monkeypatch + ): + """Control: on a capable host a missing trigram trigger is real damage. + + Only the permanently-unsatisfiable case changes. Where the trigram DDL + can run, a gap in those triggers means the index missed rows and must + still be rebuilt. + """ + db_path = tmp_path / "state.db" + statements = [] + self._trace(monkeypatch, statements, trigram=True) + + self._seed(db_path) + + db = SessionDB(db_path=db_path) + trigram_available = db._trigram_available + try: + if not trigram_available: + pytest.skip("this SQLite build has no trigram tokenizer") + db._conn.execute("DROP TRIGGER messages_fts_trigram_insert") + db._conn.commit() + finally: + db.close() + + statements.clear() + db = SessionDB(db_path=db_path) + try: + assert db._trigram_available is True + assert len(self._rebuilds(statements)) > 0 + finally: + db.close() + + class TestTitleUniqueness: """Tests for unique title enforcement and title-based lookups."""