fix(state): add sessions.trigram_fts flag to drop the trigram FTS index - #111
fix(state): add sessions.trigram_fts flag to drop the trigram FTS index#111exiao wants to merge 7 commits into
Conversation
The secondary trigram FTS5 index (messages_fts_trigram) stores its own full copy of every message plus a 3-gram index (3-5x the raw text) and fires on every message INSERT. On heavy gateway+cron users it dominates state.db size and lengthens WAL write-lock holds enough to exhaust the 15-retry write-lock budget, surfacing as 'state.db routing save failed: database is locked' and stalling the gateway on restart. Add sessions.trigram_fts (default True = current behavior). When false, schema init drops the trigram index + its triggers and never recreates them; _trigram_available is forced False so search_messages uses the existing base-FTS/LIKE fallback. The base messages_fts word index (used by recall and session search) is unaffected. Setting it back to true recreates the index on next open. Patch note: ~/.hermes/plans/hermes-patches/2026-07-14-trigram-fts-config-flag.md
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a configuration option trigram_fts to allow users to disable the secondary trigram FTS5 index on message content, which helps reduce database size and write latency on heavy usage. When disabled, the trigram index and its triggers are dropped, and search falls back to a LIKE scan. The feedback highlights two key improvements: first, a bug in the trigger repair logic where transitioning the configuration could leave missing base triggers unrepaired; second, an opportunity to check the return value of _drop_trigram_fts to suggest running a VACUUM command to reclaim disk space.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d6099754c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Addresses two P2s from independent review of the sessions.trigram_fts flag: 1. Base-trigger repair could be masked when trigram is disabled. Comparing the total FTS trigger count against a flag-adjusted expectation let a DB with 0 base + 3 stale trigram triggers tie the threshold and skip the base FTS rebuild, leaving rows written during the gap unsearchable. Count base triggers on their own (_base_fts_trigger_count / _BASE_FTS_TRIGGERS) so base health is judged independently of trigram presence. 2. The v11 migration built and backfilled messages_fts_trigram before the runtime block dropped it, so a legacy v10 user opting out still paid the full trigram rebuild + WAL churn once. Gate the v11 trigram build on _trigram_fts_enabled() and drop any pre-existing copy instead. Regression test: test_trigram_disabled_still_repairs_missing_base_triggers. Suite: 75 passed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 776d466640
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex review P2: the CJK LIKE fallback in search_messages omitted the (m.active = 1 OR m.compacted = 1) predicate that the base and trigram FTS paths apply, so it returned messages the user rewound (active=0, compacted=0). Pre-existing, but this PR widens exposure because sessions.trigram_fts=false now routes 3+ char CJK queries into that fallback by config, not just short-CJK ones. Add the filter, gated on include_inactive to match the other paths. Regression test test_cjk_like_fallback_excludes_rewound_rows (fail-before/pass-after verified). Suite: 77 passed. Not fixed here: the same fallback drops AND/NOT boolean operators (long-standing, predates this PR, separate change).
…K LIKE fallback
Builds on the active-row filter added in the prior commit. Codex P2: when
sessions.trigram_fts is false, all 3+ char CJK searches route through the
LIKE fallback, which flattened AND / OR / NOT to a single OR of every term.
So '大别山项目 NOT 桂林项目' returned rows containing the excluded term and
'A AND B' returned rows matching only A.
- Walk the tokens honoring AND / OR / NOT (adjacent terms default to AND per
FTS5). NOT attaches as 'AND NOT (...)'.
- COALESCE the LIKE columns to '' so a NULL tool_name/tool_calls yields FALSE,
not NULL — otherwise 'AND NOT (FALSE OR NULL)' collapses to NULL and drops
every row.
- Log a VACUUM hint when the trigram index is actually dropped (it can be
multiple GB; SQLite won't shrink the file automatically).
Tests: test_cjk_like_fallback_preserves_{not,and}_operator (fail-before/
pass-after). tests/test_hermes_state.py: 344 passed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8905d9acc5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex re-review P2: _drop_trigram_fts swallowed a broad OperationalError around DROP TABLE and still returned existed=True, so a 'database is locked' during startup (gateway + cron) would set _trigram_available=False and log the new 'dropped, run VACUUM' hint while the table + triggers actually survived. Re-check sqlite_master after the DROP attempt and return True only when the table that existed is genuinely gone; return False on any lock/undroppable state so the caller's VACUUM hint never misleads. Test: test_drop_trigram_fts_returns_false_when_drop_thwarted.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83f841a60d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try: | ||
| cursor.execute(f"DROP TRIGGER IF EXISTS {trigger}") | ||
| except sqlite3.OperationalError: | ||
| pass |
There was a problem hiding this comment.
When sessions.trigram_fts is false and a transient database is locked hits one of these DROP TRIGGER statements but clears before the later DROP TABLE, this suppresses the failure and can still remove messages_fts_trigram. SQLite does not remove triggers that are defined on messages, so any surviving messages_fts_trigram_* trigger will fire on the next message insert/update and fail with no such table: main.messages_fts_trigram, breaking message persistence for that DB until the stale trigger is removed.
Useful? React with 👍 / 👎.
Local code review (review bots note:
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9401caf9f3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if SessionDB._is_fts5_unavailable_error(exc): | ||
| return False |
There was a problem hiding this comment.
When sessions.trigram_fts is false for an existing DB that already has messages_fts_trigram, but the current SQLite build lacks the trigram tokenizer, the DROP TABLE can raise the same no such tokenizer: trigram error this branch suppresses. Because the handler rolls back before returning here, the trigger drops above are undone while init reports success; later message INSERTs can still fire messages_fts_trigram_insert against the unusable vtable and fail instead of degrading to the base FTS/LIKE path.
Useful? React with 👍 / 👎.
When sessions.trigram_fts is false on a DB that has messages_fts_trigram but the SQLite build lacks the trigram tokenizer, DROP TABLE raises "no such tokenizer: trigram" and the rollback undoes the tokenizer-free trigger drops. The stale triggers then fire against the unusable vtable on every message INSERT and crash. Drop the triggers in a standalone committed step on the FTS-unavailable path so writes degrade to the base FTS/LIKE path. Addresses reviewer P2.
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Superseded. #110 (merged) shipped the same trigram-disable feature under the config key |
…il-closed drops (#114) * fix(state): honor boolean operators in trigram-off LIKE fallback + fail-closed drops Salvages the two genuine improvements from #111 onto the merged sessions.fts_trigram gate (#110), which shipped the same trigram-disable feature under a different config key. #111 is superseded and closed. 1. CJK/substring LIKE fallback now preserves query boolean structure: it walks tokens left-to-right honoring AND (intersection) and NOT (exclusion) instead of OR-joining every term. Once trigram is disabled this fallback is the only CJK path, so "A NOT B" no longer returns rows with B and "A AND B" no longer returns rows matching only A. COALESCE guards the AND NOT branch against NULL tool columns dropping every row. 2. _drop_trigram_schema now fails closed: the trigger + table drops run in one BEGIN IMMEDIATE transaction so a "database is locked" propagates instead of leaving a stale trigger pointing at a missing table. On a build missing the trigram tokenizer (DROP TABLE raises), the triggers are re-dropped in a standalone committed step so message INSERTs degrade to the base FTS/LIKE path instead of crashing on the unusable vtable. Regression tests: red-before/green-after for the boolean fallback and the locked-drop propagation; tokenizer-missing trigger cleanup. * fix(state): fail closed on locked tokenizer-fallback trigger drop + support OR NOT Addresses codex review on #114: (1) the tokenizer-missing trigger-drop recovery now propagates a lock instead of swallowing it (a swallowed lock left stale triggers crashing INSERTs); (2) the LIKE-fallback boolean parser now keeps the OR connector on 'A OR NOT B' instead of collapsing it to 'A AND NOT B'. Adds regressions for both.
1 participant
Summary
Adds
sessions.trigram_ftsconfig flag (defaulttrue= current behavior) todisable the secondary trigram FTS5 index over message content.
The trigram index (
messages_fts_trigram) stores its own full copy of everymessage plus a 3-character-gram index, making it 3-5x the raw message text and
by far the largest object in
state.db. It fires on every message INSERT, whichon a heavy gateway+cron user lengthens WAL write-lock holds enough to exhaust the
15-retry app-level write-lock budget in
_execute_write, surfacing as:and stalling the gateway on restart (observed on a 9.4 GB
state.db).The trigram index only serves substring / CJK (3+ char) search. English word
search,
recall, andhermes sessions searchall run on the basemessages_ftsword index, which this change leaves untouched.
Behavior
sessions.trigram_fts: false:messages_fts_trigram+ its 3 triggers and does notrecreate them (previously recreated unconditionally every startup);
_trigram_availableis forcedFalse, sosearch_messagestransparently usesthe base-FTS / LIKE fallback that already existed;
fire (and needlessly rebuild base FTS) every startup.
Setting it back to
truerecreates the index on next open. Default preservescurrent behavior, so this is a no-op for existing installs until opted in.
Changes
hermes_cli/config.py— defaultsessions.trigram_fts: True+ explanatory comment.hermes_state.py—_TRIGRAM_FTS_TRIGGERS,_drop_trigram_fts(),_trigram_fts_enabled()(lazy best-effort config read), and the gated runtimeFTS block in
_init_schema.tests/test_hermes_state.py—test_trigram_fts_config_flag_false_drops_index.Test Plan
fresh-false (no trigram, 3 base triggers), migrate-off drops the index,
migrate-on recreates it.
Note
Separate, rarer issue seen in the same incident (out of scope here): a
/newreset whose resource cleanup exceeded 30s left an orphaned worker thread holding
the write lock. A smaller DB makes it benign.