From c4b5a36330fb32db54a17cd4316951fbf3813c0b Mon Sep 17 00:00:00 2001 From: xarmian Date: Mon, 27 Apr 2026 13:34:10 -0400 Subject: [PATCH] feat: warn at startup when shipped FTS triggers are missing (TASK-824) (#265) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(store): warn at startup when shipped FTS triggers are missing (TASK-824) Defensive follow-up to BUG-822, where the documents_* triggers had silently drifted off some production DBs and search was broken until a user noticed. The migration runner had no notion that the triggers should exist — the only invariant was "this migration ran without erroring," which is too weak when SQLite's table-rebuild path can leave auxiliary objects in a different state than the migration intended. Add a hardcoded list of expected FTS5 triggers (one row per trigger, naming the table it's attached to) and a one-shot validateFTSInvariants step at the end of Store.migrate(). Each missing trigger emits a structured slog.Warn that points the operator at the recovery migration (046). Choices: - SQLite-only. Postgres uses tsvector update functions in pgmigrations with a different invariant model. - Logging-only, no auto-repair. Auto-creating triggers here would mask legitimate future removals and obscure the source of truth (the migrations directory). The recovery path is a targeted migration like 046_restore_documents_fts_triggers.sql. - Non-fatal. A missing trigger doesn't block startup; the operator may have intentionally removed one and just not updated the list yet, and we'd rather warn loudly than refuse to boot. Tests: - TestStartupInvariants_AllFTSTriggersExist — fresh DB has all 9 expected triggers (forward-looking guard against future migrations that break one). - TestStartupInvariants_LogsOnMissingTrigger — drop a trigger, run validator, capture slog records, assert a warning naming the missing trigger was emitted. Manual verification on the production DB: - Clean DB (after migration 046): no warnings on startup. - After manually `DROP TRIGGER documents_ai`: server logs `level=WARN msg="FTS trigger missing — ..." trigger=documents_ai table=documents` immediately on startup. * test(store): address Codex review on TASK-824 — bidirectional drift + Record.Clone Two LOW findings from Codex's first pass: 1. recordCapturingHandler.Handle stored slog.Record values without cloning. Records have internal shared state; the documented pattern for retaining them is r.Clone() first. Test passed today only because nothing mutated the record after Handle, but the helper was relying on slog internals. 2. TestStartupInvariants_AllFTSTriggersExist only proved every entry in expectedFTSTriggers exists. It didn't catch the inverse: a future migration adding a new FTS-style trigger on items/comments/documents without also adding it to expectedFTSTriggers, leaving the new trigger off the invariant check forever. Add TestExpectedFTSTriggers_MatchesActual which queries sqlite_master for every trigger on items/comments/documents and asserts each is in the expected list. A new trigger that isn't tracked fails this test with a clear "update the list in store.go" message. If a future trigger on these tables is legitimately not FTS-related, the test failure points the developer at this guard and they can either add it to expectedFTSTriggers or extend the exclusion. --- internal/store/items_test.go | 160 +++++++++++++++++++++++++++++++++++ internal/store/store.go | 76 +++++++++++++++++ 2 files changed, 236 insertions(+) diff --git a/internal/store/items_test.go b/internal/store/items_test.go index 5173d7b4..698fe6ec 100644 --- a/internal/store/items_test.go +++ b/internal/store/items_test.go @@ -1,7 +1,9 @@ package store import ( + "context" "fmt" + "log/slog" "testing" "github.com/xarmian/pad/internal/models" @@ -1356,6 +1358,164 @@ func TestListDocuments_HyphenatedQuery(t *testing.T) { } } +// TestStartupInvariants_AllFTSTriggersExist asserts every trigger in the +// canonical expectedFTSTriggers list is present after migrations run on a +// fresh DB. This is a forward-looking guard — any future migration that +// inadvertently breaks one of these will fail this test, before it can +// silently drift on production DBs the way BUG-822 did. +func TestStartupInvariants_AllFTSTriggersExist(t *testing.T) { + s := testStore(t) + + if s.dialect.Driver() != DriverSQLite { + t.Skip("FTS triggers are SQLite-specific; Postgres uses a different model") + } + + for _, want := range expectedFTSTriggers { + var name string + err := s.db.QueryRow( + `SELECT name FROM sqlite_master WHERE type='trigger' AND tbl_name=? AND name=?`, + want.table, want.name, + ).Scan(&name) + if err != nil { + t.Errorf("expected trigger %q on table %q to exist after migrations, got error: %v", + want.name, want.table, err) + } + } +} + +// TestExpectedFTSTriggers_MatchesActual catches drift in the *opposite* +// direction from TestStartupInvariants_AllFTSTriggersExist: if a future +// migration adds a new trigger on items / comments / documents and the +// author forgets to add it to expectedFTSTriggers, the invariant check +// won't know to monitor it. This test compares the actual set of triggers +// on those tables against expectedFTSTriggers and fails if anything is +// missing from the list. +// +// If a new non-FTS trigger is legitimately added to one of these tables, +// either add it to expectedFTSTriggers (if it serves an FTS-like role) or +// extend the exclusion below. +func TestExpectedFTSTriggers_MatchesActual(t *testing.T) { + s := testStore(t) + + if s.dialect.Driver() != DriverSQLite { + t.Skip("FTS triggers are SQLite-specific") + } + + expected := map[string]bool{} + for _, e := range expectedFTSTriggers { + expected[e.table+"/"+e.name] = true + } + + rows, err := s.db.Query(` + SELECT name, tbl_name FROM sqlite_master + WHERE type='trigger' AND tbl_name IN ('items', 'comments', 'documents') + `) + if err != nil { + t.Fatalf("query triggers: %v", err) + } + defer rows.Close() + + for rows.Next() { + var name, table string + if err := rows.Scan(&name, &table); err != nil { + t.Fatalf("scan: %v", err) + } + key := table + "/" + name + if !expected[key] { + t.Errorf("found trigger %q on table %q that's not in expectedFTSTriggers — "+ + "update the list in store.go (or exclude in this test if it's not an FTS-style trigger)", + name, table) + } + } +} + +// TestStartupInvariants_LogsOnMissingTrigger verifies the alarm actually +// fires: drop a trigger, run validateFTSInvariants, capture the slog +// output, assert a warning was emitted naming the missing trigger. +// +// Without this test, the validator could regress (e.g. a typo in the +// SELECT, a dialect check that always returns early) and the BUG-822 +// class of drift would go undetected again. +func TestStartupInvariants_LogsOnMissingTrigger(t *testing.T) { + s := testStore(t) + + if s.dialect.Driver() != DriverSQLite { + t.Skip("FTS trigger invariant check runs only on SQLite") + } + + // Drop a known trigger to simulate the BUG-822 broken state. + target := "documents_ai" + if _, err := s.db.Exec("DROP TRIGGER " + target); err != nil { + t.Fatalf("DROP TRIGGER %s: %v", target, err) + } + + // Capture slog output via a custom handler that records records into + // a slice we can inspect after. + var captured []slog.Record + prev := slog.Default() + t.Cleanup(func() { slog.SetDefault(prev) }) + slog.SetDefault(slog.New(&recordCapturingHandler{records: &captured})) + + s.validateFTSInvariants() + + // Look for a warning record that mentions the trigger name we dropped. + found := false + for _, r := range captured { + if r.Level != slog.LevelWarn { + continue + } + mentioned := false + r.Attrs(func(a slog.Attr) bool { + if a.Key == "trigger" && a.Value.String() == target { + mentioned = true + return false + } + return true + }) + if mentioned { + found = true + break + } + } + if !found { + t.Errorf("expected a slog.Warn record naming trigger=%q after dropping it, got %d records", target, len(captured)) + for _, r := range captured { + t.Logf(" %s: %s", r.Level, r.Message) + } + } +} + +// recordCapturingHandler is a minimal slog.Handler used by +// TestStartupInvariants_LogsOnMissingTrigger to capture records without +// emitting them to stderr. Not safe for concurrent use; tests are +// single-threaded. +type recordCapturingHandler struct { + records *[]slog.Record + attrs []slog.Attr +} + +func (h *recordCapturingHandler) Enabled(_ context.Context, _ slog.Level) bool { + return true +} +func (h *recordCapturingHandler) Handle(_ context.Context, r slog.Record) error { + // slog.Record has internal shared state; clone before retaining so we + // don't depend on the caller refraining from mutating it after Handle. + r = r.Clone() + for _, a := range h.attrs { + r.AddAttrs(a) + } + *h.records = append(*h.records, r) + return nil +} +func (h *recordCapturingHandler) WithAttrs(attrs []slog.Attr) slog.Handler { + clone := *h + clone.attrs = append(append([]slog.Attr{}, h.attrs...), attrs...) + return &clone +} +func (h *recordCapturingHandler) WithGroup(_ string) slog.Handler { + return h +} + // TestMigration046_DocumentsFTSTriggersExist verifies that after all // migrations run on a fresh DB, the three documents_* triggers are present. // This regression-protects BUG-822 — production DBs ended up missing these diff --git a/internal/store/store.go b/internal/store/store.go index 66932bcc..bc03b345 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -5,6 +5,7 @@ import ( "embed" "fmt" "io/fs" + "log/slog" "sort" "strings" "time" @@ -229,9 +230,84 @@ func (s *Store) migrate() error { } } + // Defensive: log a warning for any FTS trigger that should exist but + // doesn't. Catches the BUG-822 class of regression — a table-rebuild + // migration that recorded as applied but silently failed to recreate + // its triggers, leaving search broken until someone notices. + s.validateFTSInvariants() + return nil } +// expectedFTSTriggers is the canonical list of FTS5 triggers that ship with +// the SQLite migrations. Each entry is (trigger name, table the trigger is +// attached to). The migrations file at the right of each row defines them: +// +// - items_fts_* — internal/store/migrations/001_initial.sql +// - comments_fts_* — internal/store/migrations/007_comments.sql +// - documents_* — internal/store/migrations/001_initial.sql, +// restored by 046 after BUG-822 drift. +// +// If a future migration intentionally renames or removes any of these, +// update this list in the same commit. +var expectedFTSTriggers = []struct { + name string + table string +}{ + {"items_fts_insert", "items"}, + {"items_fts_update", "items"}, + {"items_fts_delete", "items"}, + {"comments_fts_insert", "comments"}, + {"comments_fts_update", "comments"}, + {"comments_fts_delete", "comments"}, + {"documents_ai", "documents"}, + {"documents_au", "documents"}, + {"documents_ad", "documents"}, +} + +// validateFTSInvariants checks that every trigger in expectedFTSTriggers +// exists. Missing triggers are logged as structured warnings; this function +// is intentionally non-fatal and non-repairing. +// +// Non-fatal: a missing trigger doesn't prevent server startup — the +// operator may have legitimately removed one and just not updated this +// list yet, and we'd rather warn loudly than refuse to boot. +// +// Non-repairing: auto-creating triggers here would mask future legitimate +// removals and obscure the source of truth (the migrations directory). +// The recovery path is a targeted migration like 046_restore_documents_fts_triggers.sql. +// +// SQLite-only: Postgres uses a different trigger model and has its own +// search_vector update functions in pgmigrations. +func (s *Store) validateFTSInvariants() { + if s.dialect.Driver() != DriverSQLite { + return + } + for _, t := range expectedFTSTriggers { + var name string + err := s.db.QueryRow( + `SELECT name FROM sqlite_master WHERE type='trigger' AND tbl_name=? AND name=?`, + t.table, t.name, + ).Scan(&name) + if err == sql.ErrNoRows { + slog.Warn( + "FTS trigger missing — search may silently miss new rows; consider a restoration migration like 046_restore_documents_fts_triggers.sql", + "trigger", t.name, + "table", t.table, + ) + continue + } + if err != nil { + slog.Warn( + "FTS trigger check errored", + "trigger", t.name, + "table", t.table, + "err", err, + ) + } + } +} + // migratePostgres applies PostgreSQL migrations. // PostgreSQL supports multi-statement execution natively, so we don't need execMulti. func (s *Store) migratePostgres() error {