perf(state): merge FTS5 segments + add handoff_state index to curb write-lock contention - #54752
Conversation
…ntion
The message triggers append one FTS5 segment per insert into both the
porter and trigram indexes. Nothing ever called the existing
optimize_fts() maintenance helper, so on a long-lived state.db these
segments accumulate without bound (observed: ~34k trigram segments for
~27k messages). Every MATCH then has to scan all segments, and every
insert pays a growing automerge cost that lengthens the WAL write-lock
hold time. Because the gateway and cron agents are separate processes
sharing one state.db, those longer holds exhaust the 1s-timeout x 15-retry
budget in _execute_write and surface as repeated:
Session DB creation failed (will retry next turn): database is locked
Session DB append_message failed: database is locked
Wire optimize_fts() into the write path on a coarse cadence
(_OPTIMIZE_EVERY_N_WRITES = 1000), alongside the existing every-50-writes
checkpoint. 'optimize' is effectively free once the index is already
merged, so steady-state cost is negligible; only the first merge of a
neglected index is expensive. The call is best-effort and never fails the
surrounding write.
Tests: cadence fires on the write path; a failing optimize never breaks
the write.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
(cherry picked from commit 583647b)
The index references the handoff_state column which is added by _reconcile_columns() on legacy databases. Placing it in SCHEMA_SQL causes 'no such column' errors during schema migration tests because SCHEMA_SQL runs before reconciliation. Move to DEFERRED_INDEX_SQL which runs after _reconcile_columns() — matching the existing pattern used by idx_messages_session_active. Refs: NousResearch#43504, NousResearch#40695 (cherry picked from commit 40ecd61)
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: LGTM
Performance improvement: periodically merges fragmented FTS5 segments (every 1000 writes) to prevent tens-of-thousands of segments from lengthening write-lock hold time and starving competing writers. Adds idx_sessions_handoff_state index.
Looks Good
- Best-effort optimize never fails writes
- Coarse cadence amortizes merge cost far below checkpoint cadence
- optimize is a no-op once merged — idle DB pays almost nothing
- Index uses CREATE INDEX IF NOT EXISTS (safe for existing DBs)
- Tests cover cadence triggering and failure resilience
Reviewed by Hermes Agent
Related: salvages two open PRs into one reviewable change — #50124 (FTS5 segment-merge cadence, @isair) and #43504 (handoff_state index, @kenyonxu). All three are open; this is the combined superset. Flagging for a maintainer to pick the canonical PR (this salvage vs. the two originals). |
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: LGTM
Good performance fix to merge FTS5 segments and add a handoff_state index. The implementation is clean with proper error handling and comprehensive tests. The periodic merge cadence is well-documented and the failure handling is robust.
Looks Good
- Clean implementation with proper error handling
- Good test coverage (2 tests)
- Well-documented merge cadence and failure handling
- Adds useful index for handoff_state queries
- Follows existing patterns in the codebase
Reviewed by Hermes Agent
Summary
Salvages two verified, complementary
state.dbperformance fixes into onereviewable change. On long-lived databases shared by the gateway + cron
processes, write-lock hold times grew until competing writers hit
database is locked.Two orthogonal fixes, together:
hermes_state.py) — original work by @isair (fix(state): periodically merge FTS5 segments to curb write-lock contention #50124)messagestriggers append one FTS5 segment per insert; the existingoptimize_fts()helper was never called, so segments accumulatedunbounded (~34k trigram segments / 27k messages observed), lengthening
every
MATCHscan and every insert's automerge cost — and the WALwrite-lock hold.
optimize_fts()into_execute_writeon a coarse 1000-write cadencebeside the existing checkpoint;
_try_optimize_ftsis best-effort /never-raises, off-lock.
optimizeis a no-op once merged, so steady-statecost is negligible.
handoff_stateindex (hermes_state.py) — original work by @kenyonxu (fix(gateway): add handoff_state index to prevent heartbeat blocking #43504)idx_sessions_handoff_state ON sessions(handoff_state, started_at)sothe gateway heartbeat's handoff query stops doing a full table scan.
Credit
Cherry-picked preserving original authorship:
fix(state): periodically merge FTS5 segments to curb write-lock contention— @isair (fix(state): periodically merge FTS5 segments to curb write-lock contention #50124)fix(gateway): move handoff_state index to DEFERRED_INDEX_SQL— @kenyonxu (fix(gateway): add handoff_state index to prevent heartbeat blocking #43504, closes [Bug]: Discord gateway heartbeat can be blocked by synchronous handoff SQLite polling #40695)One trivial conflict resolved: the
DEFERRED_INDEX_SQLblock gained two indexeson
mainsince #43504 was authored; all three index definitions are kept.Verification
tests/test_hermes_state.py→ 295 passed (full suite, incl. fix(state): periodically merge FTS5 segments to curb write-lock contention #50124's twonew behavior-contract tests: cadence fires + search unaffected; failing
optimize never breaks the surrounding write).
Note
Deliberately NOT bundled here: #43701 (trigram index redesign, schema migration
v15→v16) changes trigram search behavior (drops
tool_name/tool_callsfromthe index) and interacts with this FTS path — it deserves its own review.