fix(state): stop rebuilding the whole FTS index on every open when the trigram tokenizer is missing
`_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.
This commit is contained in:
briandevans
+55
-10
@@ -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(
|
||||
|
||||
@@ -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."""
|
||||
|
||||
|
||||
Reference in New Issue
Block a user