Skip to content

feat: add rebuild-fts command for FTS5 shadow-table corruption (#287)#288

Merged
wesm merged 1 commit intomainfrom
fix/issue-287
Apr 22, 2026
Merged

feat: add rebuild-fts command for FTS5 shadow-table corruption (#287)#288
wesm merged 1 commit intomainfrom
fix/issue-287

Conversation

@wesm
Copy link
Copy Markdown
Owner

@wesm wesm commented Apr 22, 2026

Closes #287.

What this adds

  • msgvault rebuild-fts — drops and recreates messages_fts, then repopulates it from messages / message_bodies / message_recipients / participants via the existing batched backfill. Recovery path for malformed inverted index for FTS5 table main.messages_fts in verify output — the case where SQLite's own rebuild pragma and delete-all don't help on a contentful FTS5 table. Wraps SQLITE_BUSY/SQLITE_LOCKED as "stop msgvault serve / MCP and retry."
  • verify ordering — integrity check now runs before OAuth setup, so corrupt databases surface the repair hint even with an expired token.
  • verify hints — split between FTS-only corruption (points at rebuild-fts) and core-table corruption (points at .recover).
  • docs/recovery.md — covers both paths and the contentful-FTS5 caveat.

What this does not cover

Usage

msgvault rebuild-fts

Stop msgvault serve and MCP clients first — needs an exclusive write lock. Peak extra disk ≈ size of the FTS5 shadow tables (a few percent of the DB).

msgvault verify surfaces "malformed inverted index for FTS5 table
main.messages_fts" on long-lived archives. SQLite's own rebuild pragma
reads from the same corrupt shadow tables and cannot clear the state,
and delete-all is rejected on contentful FTS5 tables.

rebuild-fts drops messages_fts outright (discarding the shadow tables)
and repopulates it from messages / message_bodies / message_recipients
/ participants via the existing batched backfill path.

RebuildFTS intentionally bypasses the cached fts5Available flag — the
probe that sets that flag is the same one that fails on corrupt shadow
tables, so the repair command cannot consult it. SQLITE_BUSY / LOCKED
errors surface as an actionable "stop serve / MCP and retry" message.

verify now runs the integrity check before OAuth setup so corruption
hints reach users with expired tokens, and splits the recovery hint
between derived-index corruption (rebuild-fts) and core-table
corruption (.recover). docs/recovery.md covers both paths.
@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented Apr 22, 2026

roborev: Combined Review (70afca3)

High-risk compile break: the new Dialect methods are only implemented for SQLite.

High

  • internal/store/dialect.go:93
    The Dialect interface now requires FTSRebuildSchema and IsBusyError, but the diff only adds SQLite implementations. Any existing non-SQLite dialect implementation will no longer satisfy the interface, causing a compile failure.
    Fix: Add implementations for every dialect type, even if PostgreSQL initially returns an explicit unsupported error for FTSRebuildSchema and IsBusyError returns false or checks driver-specific busy errors.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@wesm
Copy link
Copy Markdown
Owner Author

wesm commented Apr 22, 2026

The compile-break finding is a false positive. Verified:

  • `ls internal/store/dialect*.go` returns only `dialect.go` (the interface) and `dialect_sqlite.go` (the sole implementation). No other type implements `Dialect`.
  • `go build ./...` and `go build -tags fts5 ./...` both pass locally.
  • All CI build/test jobs are green on this diff: test (Linux), test-windows, nix-build, validate (Docker). A missing interface method would fail every one of them at compile time.

The PostgreSQL dialect referenced in the new method's docstring (`PostgreSQL: TODO (REINDEX / recompute tsvector column)`) is forward-looking documentation for the in-progress PG refactor (see #276 "PostgreSQL dialect refactor - PR1 of 4 - Dialect"). No PG dialect type exists yet. When it lands, whoever writes it will implement `FTSRebuildSchema` and `IsBusyError` as part of satisfying the interface — which is exactly how interface evolution is supposed to work.

Not making changes based on this finding.

@wesm wesm merged commit b7698ce into main Apr 22, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

FTS5 + B-tree corruption on contentful index: recovery requires manual DROP+CREATE+backfill

1 participant