From 2f32092b38dce4c3ade0c48897c6fab0edecb893 Mon Sep 17 00:00:00 2001 From: Kyzcreig <9063726+Kyzcreig@users.noreply.github.com> Date: Sun, 26 Jul 2026 04:34:58 -0700 Subject: [PATCH] `hermes sessions optimize-storage` aborts with MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ``` Error: optimization failed: no such table: messages_fts_trigram No data was lost. Re-run to resume. ``` on any install where the trigram FTS index is legitimately absent. The failure is deterministic — re-running can never make progress, because the crash happens at the same point every time — so the database is permanently stuck on the legacy high-footprint FTS layout with no supported way forward. Observed on a 5.4 GB production `state.db`. After the fix the same database optimized successfully and shrank to 3.3 GB. The trigram index is absent whenever the runtime cannot maintain it. On a SQLite build without the `trigram` tokenizer, `_ensure_fts_schema()` returns `False`, so `__init__` leaves `self._trigram_available = False` and no `messages_fts_trigram` table on disk. This is a **supported degraded runtime**, not damage — CJK/substring search falls back to `LIKE` and everything else works normally. `_is_fts5_unavailable_error()` and `_warn_trigram_unavailable()` exist specifically to make this path graceful. Two code paths write the boundary sweep for the deferred FTS rebuild, and only one of them respects that flag: | Function | Trigram `INSERT` guarded? | |---|---| | `fts_rebuild_step()` | ✅ `if include_trigram:` where `include_trigram = self._trigram_available` | | `_fts_rebuild_finish()` | ❌ unconditional | `_fts_rebuild_finish()` runs the boundary sweep at the *end* of the backfill. Its unguarded `INSERT INTO messages_fts_trigram …` raises `OperationalError`, which propagates out of `optimize_fts_storage()` and aborts the entire optimization — *after* the backfill has already completed. Hence the characteristic output showing 100% progress immediately before the error: ``` Rebuilding index: 100% (909,671/909,671) Error: optimization failed: no such table: messages_fts_trigram ``` There is a second, quieter consequence. The teardown phase that reclaims the demoted `fts_v22_trash_*` shadow tables runs *after* the backfill phase in `optimize_fts_storage()`. Because the crash happens before teardown is ever reached, those tables are never emptied or dropped — so the space the migration was supposed to reclaim stays allocated indefinitely, and the leftover trash tables look (misleadingly) like evidence of a half-finished migration. Build a populated v23 database, set the deferred-rebuild markers, then reopen it on a runtime where `_ensure_fts_schema('messages_fts_trigram', …)` returns `False` (exactly what a SQLite build without the trigram tokenizer produces) and call `optimize_fts_storage()`: ``` [precondition] trigram absent, _trigram_available=False, rebuild pending ✓ RED ✗ optimize_fts_storage raised OperationalError: no such table: messages_fts_trigram ``` With this patch applied, unchanged harness: ``` optimize_fts_storage returned {'ok': True, 'vacuumed': None} GREEN ✓ optimize ok; markers cleared; base FTS 'zebra' -> 200 hits ``` Full harness and transcripts in `TEST-EVIDENCE.md`. Gate the sweep on `self._trigram_available`, exactly as `fts_rebuild_step()` already does: ```python include_trigram = self._trigram_available def _do(conn): ... if include_trigram: conn.execute("INSERT INTO messages_fts_trigram(...) ...") ``` The base `messages_fts` sweep and the marker cleanup are untouched, so the rebuild still finalizes correctly and the index remains complete for every row it is responsible for. The fix does not disable or weaken search to dodge the error — the regression tests assert that base FTS still returns results afterwards. `TestFtsRebuildFinishWithoutTrigram` in `tests/test_hermes_state.py`: - `test_rebuild_finish_skips_trigram_when_unavailable` — drives `_fts_rebuild_finish()` directly on a trigram-less runtime; asserts it completes, clears both rebuild markers, and leaves base FTS searchable. - `test_optimize_fts_storage_succeeds_without_trigram` — end-to-end through the public `optimize_fts_storage()` entry point; asserts `ok=True`, markers cleared, search intact. Both use the existing `_NoTrigramConnection` helper already in the file. Both fail on `main` with `no such table: messages_fts_trigram` and pass with this patch. `tests/test_hermes_state.py` passes in full (463 tests → 465 with these two). `ruff` clean. This PR is the crash only. A companion PR narrows `_db_opens_cleanly()` so that `hermes sessions repair --check-only` stops reporting a write-broken FTS schema as healthy — the gap that makes this class of problem hard to diagnose in the first place. The two are independent and can land in either order. --- hermes_state_search.py | 26 +++++--- tests/test_hermes_state.py | 123 +++++++++++++++++++++++++++++++++++++ 2 files changed, 141 insertions(+), 8 deletions(-) diff --git a/hermes_state_search.py b/hermes_state_search.py index bcf92e28c52ee..756884b3f29f1 100644 --- a/hermes_state_search.py +++ b/hermes_state_search.py @@ -115,7 +115,16 @@ class SessionSearchMixin: trigger activation): re-index any row near the boundary that the index is missing. docsize has one row per indexed doc, so the anti-join is exact and runs on a narrow id range. + + The trigram half of the sweep is gated on ``self._trigram_available`` + for the same reason ``fts_rebuild_step()`` gates its backfill INSERT: + when the SQLite build has no trigram tokenizer (or the table was + never created), an unconditional INSERT raises ``no such table`` + and aborts the whole rebuild — taking ``optimize_fts_storage()`` + down with it. """ + include_trigram = self._trigram_available + def _do(conn): hw_row = conn.execute( "SELECT value FROM state_meta WHERE key = 'fts_rebuild_high_water'" @@ -132,14 +141,15 @@ class SessionSearchMixin: "AND NOT EXISTS (SELECT 1 FROM messages_fts_docsize d WHERE d.id = m.id)", (lo, hi), ) - conn.execute( - "INSERT INTO messages_fts_trigram(rowid, content, tool_name, tool_calls) " - "SELECT m.id, m.content, m.tool_name, m.tool_calls " - "FROM messages m " - "WHERE m.id > ? AND m.id <= ? AND m.role <> 'tool' " - "AND NOT EXISTS (SELECT 1 FROM messages_fts_trigram_docsize d WHERE d.id = m.id)", - (lo, hi), - ) + if include_trigram: + conn.execute( + "INSERT INTO messages_fts_trigram(rowid, content, tool_name, tool_calls) " + "SELECT m.id, m.content, m.tool_name, m.tool_calls " + "FROM messages m " + "WHERE m.id > ? AND m.id <= ? AND m.role <> 'tool' " + "AND NOT EXISTS (SELECT 1 FROM messages_fts_trigram_docsize d WHERE d.id = m.id)", + (lo, hi), + ) conn.execute( "DELETE FROM state_meta WHERE key IN " "('fts_rebuild_high_water', 'fts_rebuild_progress')" diff --git a/tests/test_hermes_state.py b/tests/test_hermes_state.py index 67cad709cbf5c..cf7c813338b82 100644 --- a/tests/test_hermes_state.py +++ b/tests/test_hermes_state.py @@ -3894,3 +3894,126 @@ class TestInsightsToolCallIndex: assert "WHERE" in sql assert "role = 'assistant'" in sql assert "tool_calls IS NOT NULL" in sql +class TestFtsRebuildFinishWithoutTrigram: + """An FTS index that the runtime cannot maintain must not wedge the store. + + Two independent failure sites shared one root shape: code that writes to + ``messages_fts_trigram`` without first checking the table is actually + present. It is legitimately absent whenever the trigram index is + unavailable (SQLite build without the tokenizer), and it can also be left + absent by an interrupted migration or a partially-applied schema change. + """ + + @staticmethod + def _seed(db_path, n=60): + seeded = SessionDB(db_path=db_path) + try: + seeded.create_session(session_id="s1", source="cli") + for i in range(n): + seeded.append_message( + "s1", + role=("user" if i % 3 == 0 + else "assistant" if i % 3 == 1 else "tool"), + content=f"sentinel payload {i} zebra", + ) + high_water = seeded._conn.execute( + "SELECT COALESCE(MAX(id), 0) FROM messages" + ).fetchone()[0] + finally: + seeded.close() + return high_water + + def test_rebuild_finish_skips_trigram_when_unavailable( + self, tmp_path, monkeypatch + ): + """optimize_fts_storage() completes when the trigram index is absent. + + ``fts_rebuild_step()`` already guards its backfill INSERT on + ``_trigram_available``; ``_fts_rebuild_finish()``'s boundary sweep did + not, so finishing a deferred rebuild on a trigram-less runtime raised + ``no such table: messages_fts_trigram`` and aborted the whole + optimization. The base index must still be swept and the markers + cleared. + """ + db_path = tmp_path / "state.db" + high_water = self._seed(db_path) + + real_connect = sqlite3.connect + + def connect_without_trigram(*args, **kwargs): + kwargs["factory"] = _NoTrigramConnection + return real_connect(*args, **kwargs) + + monkeypatch.setattr( + "hermes_state.sqlite3.connect", connect_without_trigram + ) + db = SessionDB(db_path=db_path) + try: + assert db._trigram_available is False + # A trigram-less runtime leaves no trigram index on disk. + db._conn.execute("DROP TABLE IF EXISTS messages_fts_trigram") + db._conn.commit() + assert db._fts_table_exists("messages_fts_trigram") is False + + # Put the DB in the pending-deferred-rebuild state. + for key, value in ( + ("fts_rebuild_high_water", str(high_water)), + ("fts_rebuild_progress", str(high_water)), + ): + db._conn.execute( + "INSERT INTO state_meta (key, value) VALUES (?, ?) " + "ON CONFLICT(key) DO UPDATE SET value = excluded.value", + (key, value), + ) + db._conn.commit() + + # Pre-fix this raised OperationalError("no such table: ..."). + db._fts_rebuild_finish() + + # The sweep ran to completion: markers cleared… + assert db.get_meta("fts_rebuild_high_water") is None + assert db.get_meta("fts_rebuild_progress") is None + # …and the base index is still usable (the fix must not disable + # real search to dodge the error). + assert db.search_messages("zebra") + finally: + db.close() + + def test_optimize_fts_storage_succeeds_without_trigram( + self, tmp_path, monkeypatch + ): + """End-to-end: the public optimize entry point returns ok=True.""" + db_path = tmp_path / "state.db" + high_water = self._seed(db_path) + + real_connect = sqlite3.connect + + def connect_without_trigram(*args, **kwargs): + kwargs["factory"] = _NoTrigramConnection + return real_connect(*args, **kwargs) + + monkeypatch.setattr( + "hermes_state.sqlite3.connect", connect_without_trigram + ) + db = SessionDB(db_path=db_path) + try: + db._conn.execute("DROP TABLE IF EXISTS messages_fts_trigram") + db._conn.commit() + assert db._trigram_available is False + for key, value in ( + ("fts_rebuild_high_water", str(high_water)), + ("fts_rebuild_progress", "0"), + ): + db._conn.execute( + "INSERT INTO state_meta (key, value) VALUES (?, ?) " + "ON CONFLICT(key) DO UPDATE SET value = excluded.value", + (key, value), + ) + db._conn.commit() + + result = db.optimize_fts_storage(vacuum=False) + assert result["ok"] is True + assert db.get_meta("fts_rebuild_high_water") is None + assert db.search_messages("zebra") + finally: + db.close()