From 16ae0dc850ff8bf1b0a1024cee0fb7799e81b86d Mon Sep 17 00:00:00 2001 From: Hugh Brown Date: Thu, 5 Feb 2026 01:06:10 -0700 Subject: [PATCH] Fix silent error swallowing in repair_encoding.go Row scanning errors in repairMessageFields, repairDisplayNames, and repairOtherStrings were silently ignored with bare continue statements. Log skipped rows to stderr with error details and track count in repairStats so users are informed of data issues during encoding repair. Co-Authored-By: Claude Opus 4.5 --- cmd/msgvault/cmd/repair_encoding.go | 11 +++ cmd/msgvault/cmd/repair_encoding_test.go | 115 +++++++++++++++++++++++ 2 files changed, 126 insertions(+) create mode 100644 cmd/msgvault/cmd/repair_encoding_test.go diff --git a/cmd/msgvault/cmd/repair_encoding.go b/cmd/msgvault/cmd/repair_encoding.go index 10bf789b..1d9ecaec 100644 --- a/cmd/msgvault/cmd/repair_encoding.go +++ b/cmd/msgvault/cmd/repair_encoding.go @@ -5,6 +5,7 @@ import ( "database/sql" "fmt" "io" + "os" "strings" "unicode/utf8" @@ -55,6 +56,7 @@ type repairStats struct { labels int filenames int convTitles int + skippedRows int } func repairEncoding(s *store.Store) error { @@ -108,6 +110,9 @@ func repairEncoding(s *store.Store) error { if stats.convTitles > 0 { fmt.Printf(" Conv titles: %d\n", stats.convTitles) } + if stats.skippedRows > 0 { + fmt.Printf(" Skipped rows: %d (scan errors)\n", stats.skippedRows) + } fmt.Printf(" Total fields: %d\n", total) fmt.Println("\nRun 'msgvault build-cache --full-rebuild' to update the analytics cache.") return nil @@ -211,6 +216,8 @@ func repairMessageFields(s *store.Store, stats *repairStats) error { var compression sql.NullString if err := rows.Scan(&id, &subject, &bodyText, &bodyHTML, &snippet, &rawData, &compression); err != nil { + fmt.Fprintf(os.Stderr, "warning: skipping message row: scan error: %v\n", err) + stats.skippedRows++ continue } @@ -380,6 +387,8 @@ func repairDisplayNames(s *store.Store, stats *repairStats) error { var id int64 var name string if err := rows.Scan(&id, &name); err != nil { + fmt.Fprintf(os.Stderr, "warning: skipping %s row: scan error: %v\n", table.name, err) + stats.skippedRows++ continue } @@ -511,6 +520,8 @@ func repairOtherStrings(s *store.Store, stats *repairStats) error { var id int64 var value string if err := rows.Scan(&id, &value); err != nil { + fmt.Fprintf(os.Stderr, "warning: skipping %s.%s row: scan error: %v\n", table.name, table.column, err) + stats.skippedRows++ continue } diff --git a/cmd/msgvault/cmd/repair_encoding_test.go b/cmd/msgvault/cmd/repair_encoding_test.go new file mode 100644 index 00000000..1e771579 --- /dev/null +++ b/cmd/msgvault/cmd/repair_encoding_test.go @@ -0,0 +1,115 @@ +package cmd + +import ( + "testing" + + "github.com/wesm/msgvault/internal/testutil" +) + +// TestRepairOtherStrings_LogsScanErrors verifies that scan errors during +// repairOtherStrings are counted in stats.skippedRows rather than silently +// swallowed. We trigger scan errors by recreating the labels table with a +// TEXT id column and inserting a non-numeric id that can't be scanned into int64. +func TestRepairOtherStrings_LogsScanErrors(t *testing.T) { + st := testutil.NewTestStore(t) + db := st.DB() + + // Disable foreign keys so we can recreate the labels table + if _, err := db.Exec("PRAGMA foreign_keys = OFF"); err != nil { + t.Fatalf("disable foreign keys: %v", err) + } + + // Drop labels and recreate with TEXT id (not INTEGER PRIMARY KEY). + // Scanning a non-numeric TEXT value into int64 triggers a scan error. + if _, err := db.Exec("DROP TABLE IF EXISTS labels"); err != nil { + t.Fatalf("drop labels: %v", err) + } + if _, err := db.Exec(`CREATE TABLE labels ( + id TEXT, source_id INTEGER, source_label_id TEXT, + name TEXT NOT NULL, label_type TEXT, color TEXT + )`); err != nil { + t.Fatalf("create labels: %v", err) + } + + // Insert a row with non-numeric id → Scan into int64 will fail + if _, err := db.Exec(`INSERT INTO labels (id, source_id, source_label_id, name, label_type) + VALUES ('not-a-number', 1, 'lbl1', 'Test Label', 'user')`); err != nil { + t.Fatalf("insert bad label: %v", err) + } + + stats := &repairStats{} + err := repairOtherStrings(st, stats) + if err != nil { + t.Fatalf("repairOtherStrings returned error: %v", err) + } + + // Before fix: skippedRows == 0 (scan error silently swallowed) + // After fix: skippedRows == 1 (scan error counted) + if stats.skippedRows != 1 { + t.Errorf("skippedRows = %d, want 1", stats.skippedRows) + } +} + +// TestRepairDisplayNames_LogsScanErrors verifies that scan errors during +// repairDisplayNames are counted in stats.skippedRows. We trigger scan errors +// by recreating the participants table with a TEXT id column. +func TestRepairDisplayNames_LogsScanErrors(t *testing.T) { + st := testutil.NewTestStore(t) + db := st.DB() + + if _, err := db.Exec("PRAGMA foreign_keys = OFF"); err != nil { + t.Fatalf("disable foreign keys: %v", err) + } + + // Drop participants and recreate with TEXT id + if _, err := db.Exec("DROP TABLE IF EXISTS participants"); err != nil { + t.Fatalf("drop participants: %v", err) + } + if _, err := db.Exec(`CREATE TABLE participants ( + id TEXT, email_address TEXT, phone_number TEXT, + display_name TEXT, domain TEXT, canonical_id TEXT, + created_at DATETIME, updated_at DATETIME + )`); err != nil { + t.Fatalf("create participants: %v", err) + } + + // Insert a row with non-numeric id → Scan into int64 will fail + if _, err := db.Exec(`INSERT INTO participants (id, email_address, display_name, domain) + VALUES ('bad-id', 'test@example.com', 'Test User', 'example.com')`); err != nil { + t.Fatalf("insert bad participant: %v", err) + } + + stats := &repairStats{} + err := repairDisplayNames(st, stats) + if err != nil { + t.Fatalf("repairDisplayNames returned error: %v", err) + } + + // Before fix: skippedRows == 0 (scan error silently swallowed) + // After fix: skippedRows == 1 (scan error counted) + if stats.skippedRows != 1 { + t.Errorf("skippedRows = %d, want 1", stats.skippedRows) + } +} + +// TestRepairEncoding_NoScanErrors verifies that normal data produces +// zero skipped rows. +func TestRepairEncoding_NoScanErrors(t *testing.T) { + st := testutil.NewTestStore(t) + + stats := &repairStats{} + + if err := repairMessageFields(st, stats); err != nil { + t.Fatalf("repairMessageFields: %v", err) + } + if err := repairDisplayNames(st, stats); err != nil { + t.Fatalf("repairDisplayNames: %v", err) + } + if err := repairOtherStrings(st, stats); err != nil { + t.Fatalf("repairOtherStrings: %v", err) + } + + if stats.skippedRows != 0 { + t.Errorf("skippedRows = %d, want 0 for valid data", stats.skippedRows) + } +}