fix(state): WAL watchdog + v20 view-backed FTS + trigram config gate - #110
Conversation
state.db was 9.3GB with a 3.6GB -wal live. Two root causes, both addressed: 1. WAL checkpoint starvation — a pinned reader lets the -wal grow unbounded (giant WAL + hard shutdown = malformed-image corruption). New SessionDB.wal_watchdog(max_mb): RESTART-checkpoint an oversized -wal, then TRUNCATE to reclaim when unpinned; on a busy checkpoint log a WARNING naming the PIDs holding the DB. Gateway calls it at startup + hourly, gated on sessions.wal_watchdog / sessions.wal_max_mb (config.yaml, no env vars). 2. FTS duplication bloat — v11 made messages_fts/_trigram INLINE (no content=), storing a full text copy per table (~2.8GB). Schema v20 rebuilds both as external-content backed by a new messages_search_v VIEW (content||tool_name||tool_calls), preserving the v11 intent (tool-call search + snippet()) while dropping the copies. External-content triggers use the fts5 'delete' command form. Migration is idempotent, VACUUMs with checkpoint before/after to bound WAL, bumps the version only on success, and honors the FTS5-unavailable pattern. sessions.fts_trigram (default true): when false, drop the trigram table+triggers and fall back to the existing CJK LIKE path — reclaims the ~5GB trigram index. Docs: session-storage.md FTS section (was stale, showed content=messages), migration table (v16/v18/v20), and the new config knobs. Tests: invariant-only — search finds tool_calls pre/post migration, snippet markers present, migration idempotent, trigram-off falls back to LIKE, WAL watchdog shrinks an unpinned WAL and no-ops below threshold. Patch note: plans/hermes-patches/state-db-hardening.md
|
Claude encountered an error after 3s —— 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 updates the database schema to version 20, transitioning the full-text search (FTS) tables to view-backed external-content storage to eliminate duplicate message body storage and reclaim disk space. It also introduces a configuration-gated trigram index and a WAL watchdog to prevent unbounded WAL file growth. The reviewer's feedback highlights several important improvements for robustness and correctness: implementing an explicit transaction rollback on migration failure to prevent database corruption, avoiding a TOCTOU race condition when checking WAL size, resolving potential path matching and AttributeError issues when identifying PIDs holding the database, catching the broader sqlite3.Error exception during checkpoints, and correctly using monkeypatch in tests to suppress periodic checkpoints.
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.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — reviewed the WAL watchdog, the v20 view-backed external-content FTS migration/triggers, and the trigram config gate.
Confirmed correct: external-content FTS wiring (view + content=/content_rowid, 'delete'-command triggers, 'rebuild'), byte-identical concatenation across view/insert-trigger/delete-payload, unbroken search read-path (search_messages returns m.content via JOIN and snippet() still works under external content), consistent trigram gate with the 3-vs-6 trigger-repair threshold, and a WAL watchdog that keys on the RESTART busy flag and never raises. No env vars, no new model tool, and no prompt-cache/role-alternation surface touched. Tests are invariant-based.
One minor, non-blocking note (posted on the PR): flipping sessions.fts_trigram=false on an already-v20 DB drops the trigram table but doesn't VACUUM in the open path, so the ~5 GB isn't returned to the OS until a later vacuum runs — the initial v19→v20 migration does VACUUM, so cutover is fine.
0 blocking, 1 minor.
![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: 22c97f7f85
ℹ️ 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".
Code review — PR #110 (state.db WAL watchdog + v20 view-backed FTS + trigram gate)Local reviewer pass, ran against live HEAD Verdict: REQUEST CHANGES — one CI-blocking P2 (event-loop-blocking DB call). The FTS/migration/trigram core is solid and verified.P2 — hourly WAL watchdog blocks the gateway event loop (CI is RED on this)
self._session_db._db.wal_watchdog(max_mb=int(_sess_cfg.get("wal_max_mb", 64)))The comment says "Runs sync — the checkpoint is fast and this watcher tolerates a brief block", but the watchdog only fires when the WAL is already oversized, and its whole job is The repo already guards this class of bug. Reproduced locally (fail) and it is the red check in CI ("Python tests / Run tests slice 2/8"). Fix:
Net: the guard should end at 4 escapes (construction ×2 + run_sync ×2... currently 3235/19485/20316 = 3, +3245 = 4), and the hourly path routes through What I verified as PASS (evidence)
The Fix the P2 (offload the hourly call + bump the guard allowance for the startup site) and this is good to merge. Local Codex/Claude-persona reviewer pass — ran the repo's own guard suite and hand-built E2E probes against live HEAD |
- Move the hourly WAL watchdog checkpoint off the gateway event loop via asyncio.to_thread (a pinned reader can make TRUNCATE block). This also drops the on-loop sync escape; bump _ALLOWED_SYNC_DB_ESCAPES to 4 for the legitimate off-loop startup watchdog and document all four sites. - Roll back the v20 FTS migration when it can't complete so already-dropped inline FTS tables aren't committed with the DB stuck at v19 (data loss). - _wal_size_bytes: drop redundant exists() check, rely on stat()/OSError. - _pids_holding_db: os.path.abspath the target and guard f.path is None. - WAL checkpoint: catch sqlite3.Error, not just OperationalError. - test: patch _CHECKPOINT_EVERY_N_WRITES via monkeypatch on the class.
There was a problem hiding this comment.
Review posted in the Claude comment: #110 (comment)
1 blocking, 1 minor. P2 (confirmed, matches Codex): SessionDB._read_fts_trigram_config() resolves sessions.fts_trigram from the process HERMES_HOME, but tui_gateway/server.py:1687 and :1813 open other profiles' DBs without binding their home — so once fts_trigram is flipped false in any profile (this PR's live plan), opening a different profile's v20 DB drops that profile's trigram table + triggers. Scope the read to the DB owning self.db_path, or bind the home override at those two sites like :1353 does. The earlier loop-blocking hourly-watchdog P2 is resolved (asyncio.to_thread + guard bumped to 4), and Gemini's four suggestions are already in HEAD.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a3e59755c
ℹ️ 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".
The trigram gate drives destructive DDL (_drop_trigram_schema). A cross-profile open (e.g. the TUI resuming another profile's session via SessionDB(db_path=other_home/state.db) without overriding HERMES_HOME) read the launch profile's sessions.fts_trigram, so a launch profile with fts_trigram: false could silently drop a different profile's trigram index. Read the setting from the profile that owns self.db_path: use the fast cached loader for the same-profile hot path, else read the target profile's config.yaml directly. Fails open (keeps trigram) on any error. Adds a cross-profile regression test; updates the two gate-patching tests to the instance-method signature.
The connection runs with isolation_level=None (autocommit), so the v20 DROP statements committed immediately and the previous rollback() was a no-op — a failed external-content rebuild left the DB at v19 with the FTS tables gone (the exact data-loss this fallback must prevent). Wrap the destructive drop+recreate in an explicit BEGIN and route the recreate DDL through a no-commit executor (_exec_ddl_no_commit splits on sqlite3.complete_statement so executescript's implicit COMMIT can't defeat the rollback). Commit before the VACUUM/TRUNCATE-checkpoint (both need no open transaction). On failure the rollback now restores the pre-migration schema and leaves the DB at v19; the next open retries. Adds test_failed_v20_migration_rolls_back_and_stays_v19 (fails without the fix — table gone, asserts it survives). Extends _NoTrigramCursor to also simulate the missing tokenizer on execute() since the recreate now runs via execute inside the transaction.
Babysit summary — merge-ready (pending stale review dismissal)Head Fixes pushed this pass:
On the The 1 remaining "minor" (non-blocking, deferred): flipping |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30a9bcc093
ℹ️ 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".
When sessions.fts_trigram is flipped false on an already-v20 DB, the open path dropped messages_fts_trigram but only moved its pages to the freelist and committed — the on-disk file never shrank, so the knob's advertised multi-GB recovery never happened (the v20 migration path already VACUUMs for the same reason). _drop_trigram_schema now returns whether a real table was dropped; the open path checkpoint→VACUUM→checkpoints only when there is space to reclaim (never on every steady-state open with trigram disabled). Regression: test_trigram_gate_flip_off_vacuums_and_reclaims_disk (fails without the VACUUM — file doesn't shrink; passes now, freelist empty). Addresses Codex/claude P2 on hermes_state.py:1962.
|
Merge-ready at head
Note: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 94fbcc0003
ℹ️ 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".
| _expected_triggers = ( | ||
| len(_FTS_TRIGGERS) if self._fts_trigram_enabled else 3 | ||
| ) | ||
| triggers_need_repair = self._fts_trigger_count(cursor) < _expected_triggers |
There was a problem hiding this comment.
When sessions.fts_trigram is false, this check still counts the trigram triggers that are about to be dropped. On a DB where any base messages_fts_* trigger was missing while the trigram triggers still existed, the count can be at least 3, so triggers_need_repair stays false; _ensure_fts_schema recreates the missing base trigger, but the base FTS index is never rebuilt, leaving messages written during the missing-trigger window unsearchable once the trigram table is dropped. Count the base triggers separately for the trigram-disabled path, or rebuild whenever any base trigger was absent.
Useful? React with 👍 / 👎.
The trigram-disabled open path counted the full six-trigger set against a hardcoded expected count of 3. Three surviving trigram triggers from a prior trigram-enabled run could mask a dropped base trigger (2 base + 3 trigram = 5 >= 3), so triggers_need_repair stayed False and the base FTS was never rebuilt. Messages written during the missing-base-trigger window then stayed permanently unsearchable once the trigram table was dropped. Split _FTS_TRIGGERS into _BASE_FTS_TRIGGERS + _TRIGRAM_FTS_TRIGGERS and give _fts_trigger_count an explicit names arg. The open path now counts base triggers alone when trigram is off, all six when on. Regression test fails on the old logic (gap message unsearchable, 0==1) and passes now. Addresses Codex P2 (hermes_state.py:1967) on PR #110.
|
Fixed the Codex P2 (hermes_state.py:1967) in c066d9f. Root cause confirmed: the trigram-disabled open path counted the full six-trigger Fix: split Verified: new regression test |
Review: PR #110 — fix(state): WAL watchdog + v20 view-backed FTS + trigram config gateReviewed at live HEAD Verdict: APPROVE. No blocking issues. The change is well-engineered and the risky paths are correct under test. What I verified (fail-hard reproductions, not reasoning)
Non-blocking notes (P3)
Rubric
0 P0, 0 P1, 0 P2, 3 P3 — No blocking issues. Top note: — Local Codex/Claude-persona review (review bots budget-exhausted; ran the repo's AGENTS.md rubric locally and reproduced every finding against live HEAD c066d9f). Not a formal approval gate — Eric merges. |
Local independent review — PR #110 (fix/state-db-hardening)Local Codex/Claude-persona review ran the repo's rubric against live HEAD Verdict: APPROVE (comment). 0 P0, 0 P1, 0 P2, 2 P3. The one outstanding blocker is stale, not liveThe That review was posted against commit I verified this is genuinely fixed and genuinely guarded:
Verification (all run locally against HEAD c066d9f)
Correctness spot-checks (confirmed)
Non-blocking (P3)
Rubric
Recommend the Local rubric review; review bots' verdicts predate the fix commits. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! 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". |
1 participant
Summary
state.db concurrency + reliability hardening. Live on 2026-07-13 the DB was 9.3 GB with a 3.6 GB
-wal; ~70% of the file was FTS duplication. This addresses both root causes in one PR (implementsplans/state-db-concurrency-reliability.md).Base branch:
live-config. Patch note:plans/hermes-patches/state-db-hardening.md.Changes
1. WAL watchdog (
hermes_state.py+gateway/run.py)A long-lived reader pins the WAL so the every-50-writes TRUNCATE checkpoint silently no-ops and the
-walgrows without bound — and a giant WAL + hard shutdown is the malformed-image corruption scenario. NewSessionDB.wal_watchdog(max_mb): when-walexceeds the threshold, runwal_checkpoint(RESTART)(drains the log, restarts at offset 0), thenTRUNCATEto reclaim disk when no reader is pinning it. The "reader is pinning it" decision keys on the RESTART return'sbusyflag (not the post-checkpoint file size — RESTART never truncates); onbusyit logs a WARNING naming the PIDs holding the DB (psutil, best-effort). The gateway calls it at startup and hourly.2. Schema v20 — view-backed external-content FTS
v11 switched the FTS tables to INLINE mode (no
content=), storing a full copy of every message body in each shadow table (~2.8 GB). v20 introduces amessages_search_v(id, content)VIEW =content || tool_name || tool_callsand rebuildsmessages_fts+messages_fts_trigramas external-content (content=messages_search_v, content_rowid=id). This preserves the v11 intent (tool_name/tool_calls searchable,snippet()works) while dropping the duplicate copies. External-content DELETE/UPDATE triggers use the fts5'delete'command form. The migration is idempotent, VACUUMs (checkpoint before/after to bound WAL), bumps the version only on success, and honors the FTS5-unavailable pattern.3.
sessions.fts_trigramconfig gate (defaulttrue)When
false, the trigram table + its triggers are dropped and_trigram_available=False, so the existing CJKLIKEfallback engages — reclaiming the ~5 GB trigram index. Set false in the live config after merge.4. Docs —
session-storage.mdFTS section (was stale, still showedcontent=messages), migration table (v16/v18/v20 rows), and the new config knobs.New config (
config.yaml, no env vars):sessions.wal_watchdog: true,sessions.wal_max_mb: 64,sessions.fts_trigram: true.Verification
web_search/kittens/ tool_calls searchable, snippet markers present, incremental insert indexes, idempotent reopen._trigram_available=False, CJK query falls back to LIKE and still hits. WAL watchdog: 4.7 MB → 0 when unpinned; no-op below threshold.scripts/run_tests.sh tests/test_hermes_state.py→ 345 passed (6 new, invariant-only — no change-detector tests). Config + session_search + gateway startup suites green.Not in this PR (deliberate)
sessions.fts_trigram: falsein the live config is a post-merge op.mainuntouched.