fix(#1901): detectSchema fails loud instead of caching wrong schema mode (#1909)

Fixes #1901.

Thanks to @MarekWo for the exceptionally thorough report — root cause,
repro, and a prioritised fix checklist in one. This implements it.

## Problem

`detectSchema()` swallowed any probe-query error with a bare `return`,
so a single transient failure of the first `PRAGMA
table_info(observations)` at startup left `isV3` (and the feature flags)
at their zero value **for the entire process lifetime**. The server then
ran v2 SQL against a v3 DB: Packets page empty,
`/api/channels/<name>/messages` → 500, logs full of `no such column:
o.observer_id`, while the database was perfectly healthy. Nothing
re-checked the flag, so only a manual restart recovered it.

## Fix

Works through the report's checklist:

- **Don't swallow the error.** `detectSchema` now returns `error` and
`OpenDB` aborts on it. `main.go` already `log.Fatalf`s on an `OpenDB`
failure, so the supervisord/Docker restart policy retries and a
transient cause clears on the next attempt — strictly better than
serving a broken read API.
- **Log the mode unconditionally** — `[db] schema mode: v3
(observer_idx)` / `v2 (observer_id)`. A clean startup log is now
positive evidence detection ran, not just an absence of errors.
- **Run detection on a single pinned connection** (`conn.Conn(ctx)`)
rather than an arbitrary pooled one, so the startup race in the report's
hypothesis can't quietly hand detection a fresh, not-yet-openable handle
— if the connection can't be acquired, we fail loud.
- **`Close()` no longer checkpoints the read-only handle.** `PRAGMA
wal_checkpoint(TRUNCATE)` on a `mode=ro` connection always failed with
`disk I/O error (778)` and looked like a storage fault on every shutdown
(the report's aside). The ingestor (the writer) owns WAL checkpointing.

The three near-identical PRAGMA scan loops are consolidated into one
`schemaColumns()` helper that returns errors instead of ignoring `Scan`
failures.

### On the "single source of truth" item

The report suggests deriving `isV3` from `dbschema.TableHasColumn(...)`.
I kept the PRAGMA-scan structure here because `detectSchema` sets six
flags from three tables in a single pass; swapping to `TableHasColumn`
would mean six separate probe calls and wouldn't actually be cleaner.
The goal it was aimed at — never cache a false negative — is met by
making the existing scan fail loud. Happy to switch to the
single-probe-per-column shape if you'd prefer it.

### Honest note on the connection

`conn.Conn(ctx)` pins *a* single connection for all four probes and
fails loud if it can't be acquired; it does not guarantee the literal
connection `Ping()` validated (`database/sql` doesn't expose that). The
fail-fast is what actually closes the bug — a mis-detected schema aborts
startup instead of persisting for the process lifetime.

## Tests

- `TestDetectSchemaFailsLoudOnProbeError` — injects a probe failure
through a `rowQuerier` and asserts the error propagates and `isV3` stays
unset (the invariant the old bare-`return` violated).
- `TestDetectSchemaV3AndV2` — covers both schema shapes through
`OpenDB`.

`go vet ./cmd/server` and `go build` are clean; targeted `go test -run
'DetectSchema|OpenDB'` is green.

Heads-up on the full `go test ./cmd/server` run: a handful of
`TestHandleNodePaths_*` / `TestHandleAnalytics*` tests return `503 index
loading`, plus one intentional panic test — these fail identically on
pristine `master` (`a06ac8ac`) with this branch stashed, i.e. they're
pre-existing/timing-related and untouched by this change.

Out of scope (per the issue): frontend behaviour when the API 500s.

🤖 Authored with [Claude](https://claude.com) · Co-Authored-By trailer on
the commit.

Co-authored-by: SaarMesh-Bot <300107934+SaarMesh-Bot@users.noreply.github.com>
Co-authored-by: Claude <noreply@anthropic.com>

cmd/server/coverage_test.go

@@ -2,6 +2,7 @@ package main
 
 import (
 	"bytes"
+	"context"
 	"database/sql"
 	"encoding/json"
 	"fmt"
@@ -112,7 +113,9 @@ func TestDetectSchemaV3(t *testing.T) {
 func TestDetectSchemaV2(t *testing.T) {
 	db := setupTestDBv2(t)
 	defer db.Close()
-	db.detectSchema()
+	if err := db.detectSchema(context.Background(), db.conn); err != nil {
+		t.Fatalf("detectSchema: %v", err)
+	}
 	if db.isV3 {
 		t.Error("expected v2 schema (observer_id), got v3")
 	}

cmd/server/db.go

@@ -1,6 +1,7 @@
 package main
 
 import (
+	"context"
 	"database/sql"
 	"encoding/json"
 	"fmt"
@@ -63,87 +64,105 @@ func OpenDB(path string) (*DB, error) {
 		return nil, fmt.Errorf("ping failed: %w", err)
 	}
 	d := &DB{conn: conn, path: path}
-	d.detectSchema()
+	// Detect the on-disk schema on a single pinned connection and fail loudly if
+	// it cannot be probed. A swallowed detection failure would silently cache the
+	// wrong schema mode (isV3=false against a v3 DB) for the entire process
+	// lifetime, breaking every read path until a manual restart (#1901). Aborting
+	// here lets the supervisor restart us, which clears any transient cause (e.g.
+	// a WAL recovery racing the read-only open).
+	ctx := context.Background()
+	sc, err := conn.Conn(ctx)
+	if err != nil {
+		conn.Close()
+		return nil, fmt.Errorf("schema detection: acquire connection: %w", err)
+	}
+	derr := d.detectSchema(ctx, sc)
+	_ = sc.Close()
+	if derr != nil {
+		conn.Close()
+		return nil, fmt.Errorf("schema detection failed: %w", derr)
+	}
 	return d, nil
 }
 
 func (db *DB) Close() error {
-	// Checkpoint WAL before closing to release lock cleanly for new processes
-	if _, err := db.conn.Exec("PRAGMA wal_checkpoint(TRUNCATE)"); err != nil {
-		log.Printf("[db] WAL checkpoint error: %v", err)
-	} else {
-		log.Println("[db] WAL checkpoint complete")
-	}
+	// The connection is opened read-only (mode=ro in OpenDB), so it cannot
+	// checkpoint the WAL: PRAGMA wal_checkpoint(TRUNCATE) always fails with
+	// "disk I/O error (778)" on a read-only handle. Attempting it emitted a
+	// misleading storage-fault line on every shutdown that wasted incident
+	// investigation time (#1901); the ingestor (the writer) owns checkpointing.
 	return db.conn.Close()
 }
 
-// detectSchema checks if the observations table uses v3 schema (observer_idx).
-func (db *DB) detectSchema() {
-	rows, err := db.conn.Query("PRAGMA table_info(observations)")
+// detectSchema probes the on-disk schema and sets the capability flags.
+//
+// It returns an error if any probe query fails. Previously these failures were
+// swallowed with a bare return, leaving isV3 (and the feature flags) at their
+// zero value for the whole process lifetime: a single transient failure of the
+// first PRAGMA silently ran v2 SQL against a v3 database until the process was
+// restarted (#1901). Detection is now all-or-nothing — on any probe error the
+// caller aborts startup so the supervisor can retry.
+func (db *DB) detectSchema(ctx context.Context, q rowQuerier) error {
+	obs, err := schemaColumns(ctx, q, "observations")
 	if err != nil {
-		return
+		return fmt.Errorf("probe observations: %w", err)
+	}
+	db.isV3 = obs["observer_idx"]
+	db.hasResolvedPath = obs["resolved_path"]
+	db.hasObsRawHex = obs["raw_hex"]
+
+	tx, err := schemaColumns(ctx, q, "transmissions")
+	if err != nil {
+		return fmt.Errorf("probe transmissions: %w", err)
+	}
+	db.hasScopeName = tx["scope_name"]
+	db.hasLastSeen = tx["last_seen"]
+
+	nodes, err := schemaColumns(ctx, q, "nodes")
+	if err != nil {
+		return fmt.Errorf("probe nodes: %w", err)
+	}
+	db.hasDefaultScope = nodes["default_scope"]
+	db.hasMultibyteSupCols = nodes["multibyte_sup"]
+
+	if db.isV3 {
+		log.Printf("[db] schema mode: v3 (observer_idx)")
+	} else {
+		log.Printf("[db] schema mode: v2 (observer_id)")
+	}
+	return nil
+}
+
+// rowQuerier is satisfied by both *sql.DB and *sql.Conn. detectSchema takes it
+// so it can run against a single pinned connection (see OpenDB) and be
+// unit-tested with an injected probe failure (#1901).
+type rowQuerier interface {
+	QueryContext(ctx context.Context, query string, args ...any) (*sql.Rows, error)
+}
+
+// schemaColumns returns the set of column names present on the given table via
+// PRAGMA table_info. Unlike the previous inline scans, a query or scan error is
+// returned rather than swallowed (#1901). The table name is a trusted literal
+// supplied by the caller, not user input.
+func schemaColumns(ctx context.Context, q rowQuerier, table string) (map[string]bool, error) {
+	rows, err := q.QueryContext(ctx, fmt.Sprintf("PRAGMA table_info(%s)", table))
+	if err != nil {
+		return nil, err
 	}
 	defer rows.Close()
+	cols := make(map[string]bool)
 	for rows.Next() {
 		var cid int
-		var colName string
-		var colType sql.NullString
-		var notNull, pk int
+		var name string
+		var ctype sql.NullString
+		var notnull, pk int
 		var dflt sql.NullString
-		if rows.Scan(&cid, &colName, &colType, &notNull, &dflt, &pk) == nil {
-			if colName == "observer_idx" {
-				db.isV3 = true
-			}
-			if colName == "resolved_path" {
-				db.hasResolvedPath = true
-			}
-			if colName == "raw_hex" {
-				db.hasObsRawHex = true
-			}
-		}
-	}
-
-	txRows, err := db.conn.Query("PRAGMA table_info(transmissions)")
-	if err != nil {
-		return
-	}
-	defer txRows.Close()
-	for txRows.Next() {
-		var cid int
-		var colName string
-		var colType sql.NullString
-		var notNull, pk int
-		var dflt sql.NullString
-		if txRows.Scan(&cid, &colName, &colType, &notNull, &dflt, &pk) == nil {
-			if colName == "scope_name" {
-				db.hasScopeName = true
-			}
-			if colName == "last_seen" {
-				db.hasLastSeen = true
-			}
-		}
-	}
-
-	nodeRows, err := db.conn.Query("PRAGMA table_info(nodes)")
-	if err != nil {
-		return
-	}
-	defer nodeRows.Close()
-	for nodeRows.Next() {
-		var cid int
-		var colName string
-		var colType sql.NullString
-		var notNull, pk int
-		var dflt sql.NullString
-		if nodeRows.Scan(&cid, &colName, &colType, &notNull, &dflt, &pk) == nil {
-			switch colName {
-			case "default_scope":
-				db.hasDefaultScope = true
-			case "multibyte_sup":
-				db.hasMultibyteSupCols = true
-			}
+		if err := rows.Scan(&cid, &name, &ctype, &notnull, &dflt, &pk); err != nil {
+			return nil, err
 		}
+		cols[name] = true
 	}
+	return cols, rows.Err()
 }
 
 // nodeSelectCols returns the SELECT column list for nodes queries.

cmd/server/db_test.go

@@ -1,9 +1,12 @@
 package main
 
 import (
+	"context"
 	"database/sql"
+	"errors"
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"
 	"time"
 
@@ -2327,3 +2330,98 @@ func TestLoadIndexesRelayHopsFromResolvedPath(t *testing.T) {
 		t.Errorf("relay byNode entry has wrong hash: %s", store.byNode[relayPubkey][0].Hash)
 	}
 }
+
+// failingQuerier wraps a real *sql.DB but forces the PRAGMA probe for a chosen
+// table to fail, so the #1901 fail-loud path can be exercised deterministically.
+type failingQuerier struct {
+	real   *sql.DB
+	failOn string // if the query text contains this substring, return err
+	err    error
+}
+
+func (f *failingQuerier) QueryContext(ctx context.Context, query string, args ...any) (*sql.Rows, error) {
+	if f.failOn != "" && strings.Contains(query, f.failOn) {
+		return nil, f.err
+	}
+	return f.real.QueryContext(ctx, query, args...)
+}
+
+// TestDetectSchemaFailsLoudOnProbeError is the regression guard for #1901: a
+// probe-query failure must surface as an error so OpenDB can abort startup,
+// instead of being silently swallowed and leaving the server in v2 mode against
+// a v3 database for the whole process lifetime.
+func TestDetectSchemaFailsLoudOnProbeError(t *testing.T) {
+	dbPath := filepath.Join(t.TempDir(), "v3.db")
+	conn, err := sql.Open("sqlite", dbPath)
+	if err != nil {
+		t.Fatal(err)
+	}
+	conn.SetMaxOpenConns(1)
+	// v3-shaped: observations carries observer_idx.
+	if _, err := conn.Exec(`CREATE TABLE observations (id INTEGER PRIMARY KEY, observer_idx INTEGER)`); err != nil {
+		conn.Close()
+		t.Fatal(err)
+	}
+	conn.Exec(`CREATE TABLE transmissions (id INTEGER PRIMARY KEY)`)
+	conn.Exec(`CREATE TABLE nodes (public_key TEXT PRIMARY KEY)`)
+	conn.Close()
+
+	real, err := sql.Open("sqlite", "file:"+dbPath+"?mode=ro")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer real.Close()
+
+	q := &failingQuerier{real: real, failOn: "table_info(observations)", err: errors.New("injected probe failure")}
+	db := &DB{}
+	if err := db.detectSchema(context.Background(), q); err == nil {
+		t.Fatal("detectSchema must return an error when a schema probe fails (#1901)")
+	}
+	if db.isV3 {
+		t.Error("isV3 must not be set when detection failed")
+	}
+}
+
+// TestDetectSchemaV3AndV2 verifies detection sets isV3 correctly for both schema
+// shapes via the real OpenDB path, and that both open cleanly.
+func TestDetectSchemaV3AndV2(t *testing.T) {
+	dir := t.TempDir()
+
+	v3 := filepath.Join(dir, "v3.db")
+	c, err := sql.Open("sqlite", v3)
+	if err != nil {
+		t.Fatal(err)
+	}
+	c.SetMaxOpenConns(1)
+	c.Exec(`CREATE TABLE observations (id INTEGER PRIMARY KEY, observer_idx INTEGER)`)
+	c.Exec(`CREATE TABLE transmissions (id INTEGER PRIMARY KEY, hash TEXT)`)
+	c.Exec(`CREATE TABLE nodes (public_key TEXT PRIMARY KEY)`)
+	c.Close()
+	db, err := OpenDB(v3)
+	if err != nil {
+		t.Fatalf("OpenDB v3: %v", err)
+	}
+	if !db.isV3 {
+		t.Error("isV3 should be true when observations.observer_idx exists")
+	}
+	db.Close()
+
+	v2 := filepath.Join(dir, "v2.db")
+	c2, err := sql.Open("sqlite", v2)
+	if err != nil {
+		t.Fatal(err)
+	}
+	c2.SetMaxOpenConns(1)
+	c2.Exec(`CREATE TABLE observations (id INTEGER PRIMARY KEY, observer_id INTEGER)`)
+	c2.Exec(`CREATE TABLE transmissions (id INTEGER PRIMARY KEY, hash TEXT)`)
+	c2.Exec(`CREATE TABLE nodes (public_key TEXT PRIMARY KEY)`)
+	c2.Close()
+	db2, err := OpenDB(v2)
+	if err != nil {
+		t.Fatalf("OpenDB v2: %v", err)
+	}
+	if db2.isV3 {
+		t.Error("isV3 should be false when observations has no observer_idx")
+	}
+	db2.Close()
+}