fix(state): recover from malformed legacy FTS index during rebuild

[Christopher-Schulze]

What does this PR do?

When the SQLite FTS trigram index is malformed (e.g. from a legacy schema), the rebuild previously failed. This PR:

1. Detects malformed legacy FTS indexes during rebuild and recovers by dropping+recreating
2. Narrows recovery to malformed-index errors only (classified by SQLite result code)
3. Heals on open via an integrity-check probe in _init_fts (once per SQLite engine)
4. Persists fts_stale breadcrumb and detaches FTS on failed recovery
5. Adds capability stamp to engine stamp

Related Issue

Fixes #86027

Type of Change

• 🐛 Bug fix

Changes Made

• hermes_state_schema.py: Malformed FTS index recovery, narrowed error classification, engine-scoped integrity probe, stale breadcrumb, capability stamp.
• tests/state/test_legacy_fts_malformed_rebuild.py: Tests updated to use HEAD's _rebuild_fts_indexes API.

How to Test

1. scripts/run_tests.sh tests/state/test_legacy_fts_malformed_rebuild.py — FTS recovery tests pass.
2. Full project checker passes.

Checklist

• Code follows the project's style and conventions
• Self-review completed
• Tests added/updated and passing

[Christopher-Schulze]

Thanks for the detailed review — all four points addressed:\n\n1. Recovery now runs on a trigger-complete corrupt install: a non-mutating FTS5 'integrity-check' probe on open detects a malformed shadow table even when all six triggers are present, so SessionDB(...) self-heals instead of skipping the rebuild gate.\n2. Narrowed exception: _is_malformed_fts_index_error matches only the corrupt-index class; lock/busy/disk-IO DatabaseErrors propagate to the open retry path untouched.\n3. Atomicity: the drop-triggers + drop-vtable + recreate-legacy-schema + backfill now runs inside one BEGIN IMMEDIATE ... COMMIT, matching _recover_stale_fts; a failed recovery can no longer leave a committed half-drop.\n4. Production-shaped tests: a corrupt trigger-complete legacy DB opened via SessionDB is asserted to heal (quick_check == ok, MATCH counts match messages, and _db_has_legacy_inline_fts stays true rather than demoting to v23).

[strzhao]

Nice turnaround on the review round — v2's narrow classifier and atomic BEGIN IMMEDIATE recovery are the right calls, and both were real gaps in my parallel attempt (#86183) too; I've adopted them there with credit (8f226234c6).

One delta worth weighing for whichever branch maintainers pick as home: v2's _legacy_fts_index_corrupt probe runs on every writable open of a legacy DB. On the installs reporting in #86027/#69672 (1.5–2 GB trigram indexes, ~30k messages) that's a recurring full-index integrity-check on every open, indefinitely until the user opts into hermes sessions optimize-storage. #86183 gates the same probe behind an fts_integrity_engine state_meta marker: one pass per engine change, then a single meta read per open — plus v23/cjk layout coverage and an fts_stale breadcrumb so a failed recovery can't freeze an empty index.

No urgency from my side on which one wins — happy to fold ideas either way.

[Enough1122]

AI code review — automated review for reference, author can ignore or act on any point.

fix(state): recover from malformed legacy FTS index during rebuild

1. Error-message classification is SQLite-version-fragile: _is_malformed_fts_index_error (hermes_state_schema.py line 302-317) matches message substrings ("malformed inverted index", "database disk image is malformed", "malformed database schema"). The motivating case is SQLite 3.53+'s new wording; older/newer versions could phrase it differently and the corrupt index would fall through to a re-raise → the DB stays broken. A behavioral probe (e.g. run the integrity-check first, as _legacy_fts_index_corrupt does) is more robust than string matching — note the two paths currently disagree: the pre-check classifies via the same strings, so both share the fragility.
2. Integrity-check cost on every open: _init_schema now runs _legacy_fts_index_corrupt (line 172-174) on every SessionDB open for legacy-DB installs (when triggers don't need repair). The FTS5 integrity-check command walks the index — O(index size), not O(1). For a large messages table this adds startup latency on every open; consider caching the verdict (e.g. once per process) or gating on a cheap signal first.
3. Dropping both indexes when only one is corrupt: the rebuild path drops and recreates messages_fts AND messages_fts_trigram even if only the trigram index was malformed (line 121-147) — heavier than necessary but atomic and safe; fine, just noting the blast radius.
4. _recover_stale_fts DELETE removal: the diff removes DELETE FROM messages_fts_trigram; from the rebuild SQL (line 471 region) — verify that path still drops/recreates the trigram table before the INSERT (otherwise the removal leaves duplicate rows on re-run). The v23 path drops tables in its own SQL, so it's likely redundant — a test pinning double-rebuild idempotency would settle it.
5. Tests are good: both corruption targets heal on a production-shaped open, PRAGMA quick_check passes, and the classification test distinguishes lock/busy/IO from the malformed class.

[Christopher-Schulze]

Thanks — adopted the one-pass probe. After a clean integrity-check (or a successful rebuild) we persist state_meta.fts_integrity_engine = fts5:<sqlite_version> and skip the full-index command on later opens. A later SQLite upgrade rematches the marker and probes again. That removes the 1.5–2 GB per-open tax you called out, while first-open heal on a trigger-complete corrupt install still works.

[Christopher-Schulze]

Thanks both — the branch is rebased onto current main and the probe-cost concern is fully adopted.

Rebase + conflict resolution. The rebase kept upstream's newer semantics intact: the split trigger-subset repair condition (base_triggers_missing / trigram_enabled and trigram_triggers_missing) from 608a56ed7f and the cross-process _run_admitted_startup_rebuild admission from 9d0727d49b. This PR's on-open corrupt-index detection now composes with both: the admitted rebuild fires when triggers are missing or the once-per-engine integrity probe finds corruption.

Probe cost (your point, @strzhao, same as @Enough1122's #2). Adopted your fts_integrity_engine marker idea with credit in-thread earlier: one 'integrity-check' pass per SQLite engine version, persisted in state_meta, then a single meta read per open. First-open heal on a trigger-complete corrupt install still works; the 1.5–2 GB per-open tax is gone until a SQLite upgrade rematches the marker.

Still material vs. #86183: this branch also carries the narrow malformed-only classifier (lock/busy/IO re-raised), the atomic BEGIN IMMEDIATE drop+recreate+backfill recovery, and production-shaped tests where a plain SessionDB(...) open heals a trigger-complete corrupt install. Happy to consolidate whichever way maintainers prefer.

[Enough1122]

Re-verified at rebased head edfe0c6231. Both threads from the last round are closed:

• Probe cost: _legacy_fts_integrity_probe_needed gates the FTS integrity-check behind state_meta.fts_integrity_engine = fts5:<sqlite_version> — one full-index probe per SQLite engine, re-probed only after an engine upgrade. The recurring 1.5–2 GB per-open tax is gone, and test_integrity_probe_runs_once_per_sqlite_engine pins the marker behavior (first open probes + records, second open skips).
• Composition with upstream: the split base_triggers_missing / trigram_enabled and trigram_triggers_missing condition is intact, the corrupt probe rides as a third disjunct through _run_admitted_startup_rebuild, and the short-circuit order is actually load-bearing: a trigger-missing rebuild records the marker without probing, which is correct since a rebuild leaves clean indexes either way. Dropping DELETE FROM messages_fts_trigram from _recover_stale_fts_locked's script is also right and necessary — on a corrupt index the DELETE itself raises the malformed class.
• Recovery shape: drop-triggers + vtable + recreate + backfill under a single BEGIN IMMEDIATE…COMMIT, rollback + re-raise on failure, and the narrow classifier keeps lock/busy/IO on the outer open-retry path. Matches _recover_stale_fts as advertised.

One latent nit (non-blocking): in _legacy_fts_index_corrupt the second handler is unreachable —

except sqlite3.DatabaseError as exc:
    ...
except sqlite3.OperationalError as exc:   # dead: OperationalError IS a DatabaseError

sqlite3.OperationalError subclasses sqlite3.DatabaseError and Python picks the first matching clause, so a genuinely missing table/tokenizer raises from the first branch instead of continue-ing as the comment intends. Unobservable today (the trigram probe creates the table before this runs), but worth swapping the clauses — or folding the "no such table"/"no such module" check into the first branch — so the intended tolerance survives future call sites.

Nothing further from me otherwise — tight rebase.

[strzhao]

Thanks for carrying the marker gate through the rebase — with that adopted, the two branches are functionally converged, so I'm stepping #86183 down as a competing PR. Rather than leaving our remaining deltas as review suggestions (and risking another idea-only adoption), I've put them on top of your rebased head as one ready-to-cherry-pick commit, authorship-preserving:

• branch: strzhao:converge/fts-integrity-hardening (based on your edfe0c6231, clean apply)
• commit: 1d0e71e822 — fix(state): class-based corruption split + stale breadcrumb + capability stamp

What it adds, each with the failure it closes:

1. Class-based corruption split. Corruption (SQLITE_CORRUPT, the FTS5 corrupt-structure class) surfaces as plain sqlite3.DatabaseError; lock/busy/disk-IO/readonly surface as sqlite3.OperationalError. The isinstance split classifies both directions with no dependence on SQLite's wording — the string list currently misses fts5: corrupt structure record, which a really-corrupt index produced in our Tier-1.5 verification (against a 3.46.1-written NUL index), and that variant would re-raise on every open instead of triggering the rebuild. Your existing transient test passes unchanged — it already constructs errors by class.

2. Stale breadcrumb on failed fallback. A drop/recreate script that dies after CREATE can leave the table empty-but-valid; a later integrity-check passes it and stamps the engine marker, freezing a silently-dead index. The failed-fallback path now persists fts_stale, drops triggers, and detaches FTS — same ordering contract as your deferred _run_admitted_startup_rebuild branch — and the next open routes through _recover_stale_fts for the full recovery. Regression test covers failed-fallback → breadcrumb → next-open full recovery → breadcrumb cleared → 10/10 matches restored.

3. Capability signature in the engine stamp (|missing=<tables> when a probe returns None). A host without the trigram/cjk tokenizer sweeps once per engine+capability pair instead of every open, while a capable host reading its stamp sees the mismatch and re-verifies — an incapable host never vouches for indexes it could not check.

4. Small ones: the optional messages_fts_cjk index joins the corruption probe (probed first, absent installs untouched), and the separate except sqlite3.OperationalError arm in _legacy_fts_index_corrupt was unreachable (the DatabaseError arm catches the subclass) — its no-such-table/no-such-module conditions moved into the single arm.

Verified: 7/7 in test_legacy_fts_malformed_rebuild.py (4 pre-existing + 3 new), full tests/state 96 passed, ruff clean.

If you'd rather review than cherry-pick, everything above is also described in #86183's thread with the underlying verification data. Either way works — the goal is one merged fix, not two PRs.

[Christopher-Schulze]

Convergence follow-up is now on 85283ad57f.

• Imported @strzhao’s 1d0e71e822 content on top of this branch: failed-fallback fts_stale recovery, capability-aware engine stamp, optional CJK probe, and the unreachable handler fix. Zhao remains the commit author; only the private author email was normalized to his verified GitHub noreply identity for this repository’s attribution gate.
• Tightened the proposed class-only classifier after live verification: corrupt base/trigram indexes report SQLite code 267 (SQLITE_CORRUPT_VTAB). The final code matches the SQLITE_CORRUPT primary result code and uses the four known messages only when an exception has no code. This prevents unrelated DatabaseError, IntegrityError, or ProgrammingError instances from triggering destructive recovery.
• Added negative classification coverage and made the forced fallback test restore its class patch through pytest monkeypatch even if construction fails.

Evidence on current main: 7/7 focused recovery tests, 276/276 adjacent state tests, all blocking scripts/check.sh gates passed.

[Christopher-Schulze]

Maintenance update

Rebased onto current origin/main (089bb32886) and resolved conflicts from the hermes_state_schema.py decomposition:

• HEAD refactored _rebuild_legacy_fts_indexes into unified _rebuild_fts_indexes(cursor, *, legacy=False, include_trigram=True) with loop-based approach.
• Applied all 5 commits' recovery logic onto HEAD's refactored code. Updated test references to use _rebuild_fts_indexes with legacy=True.

Head: 9eb4419e8c. All blocking gates pass.

[teknium1]

Closing: main already fails open on DEADBEEF trigram corruption and rebuilds on the next open (hermes sessions repair heals it); a +453 LOC on-open integrity probe is disproportionate. Thanks @Christopher-Schulze.