From 2685bc73f86e4fdd9e74d2f3881e8bbcb07da4c1 Mon Sep 17 00:00:00 2001 From: Thales Maciel Date: Wed, 22 Apr 2026 11:05:23 -0300 Subject: [PATCH] doctor: open the state DB read-only so inspection never mutates it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `banger doctor` used to call store.Open, which unconditionally runs migrations on the way up. Diagnostics mutating persistent state is a surprise — particularly now that migration 2 drops a column, so a plain `doctor` invocation against an old DB would silently schema- evolve it. Add store.OpenReadOnly: separate DSN builder with mode=ro and a minimal pragma set (foreign_keys, busy_timeout — no journal_mode=WAL, no wal_autocheckpoint), skips runMigrations, and pings on open so a missing DB fails up front rather than at first query. doctor.go now uses OpenReadOnly; the existing storeErr fallback path surfaces any failure as a failing check, unchanged. Tests pin two invariants: - OpenReadOnly against a DB whose migration 2 marker was removed and packages_path re-added must leave both alone (i.e. no drift is applied behind the user's back). - Any write attempted through the read-only handle is rejected at the driver layer (belt-and-braces for future refactors). Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/daemon/doctor.go | 7 ++- internal/store/migrations_test.go | 84 +++++++++++++++++++++++++++++++ internal/store/store.go | 51 +++++++++++++++++++ 3 files changed, 141 insertions(+), 1 deletion(-) diff --git a/internal/daemon/doctor.go b/internal/daemon/doctor.go index 4a3c910..ff4960f 100644 --- a/internal/daemon/doctor.go +++ b/internal/daemon/doctor.go @@ -24,7 +24,12 @@ func Doctor(ctx context.Context) (system.Report, error) { if err != nil { return system.Report{}, err } - db, storeErr := store.Open(layout.DBPath) + // Doctor must be read-only: running it should never mutate the + // state DB (no migrations, no WAL checkpoint, no pragma writes). + // If the DB is missing or unreadable the storeErr path surfaces + // it as a failing check rather than half-opening a writable + // handle. + db, storeErr := store.OpenReadOnly(layout.DBPath) d := &Daemon{ layout: layout, config: cfg, diff --git a/internal/store/migrations_test.go b/internal/store/migrations_test.go index bf1e209..fa0144b 100644 --- a/internal/store/migrations_test.go +++ b/internal/store/migrations_test.go @@ -215,6 +215,90 @@ func TestMigrateDropDeadImageColumns_AcrossInstallPaths(t *testing.T) { }) } +// TestOpenReadOnlyDoesNotRunMigrations pins the doctor contract: +// OpenReadOnly must not mutate the DB. We create a DB without the +// schema_migrations row for migration 2 present (simulating a +// daemon-not-yet-run state), open it read-only, and confirm no row +// was added and no column dropped. +func TestOpenReadOnlyDoesNotRunMigrations(t *testing.T) { + path := filepath.Join(t.TempDir(), "state.db") + // Seed the file by running full Open once, then roll migration 2 + // backwards manually so the DB is "behind" current code. + full, err := Open(path) + if err != nil { + t.Fatalf("Open: %v", err) + } + if _, err := full.db.Exec("ALTER TABLE images ADD COLUMN packages_path TEXT"); err != nil { + t.Fatalf("re-add packages_path: %v", err) + } + if _, err := full.db.Exec("DELETE FROM schema_migrations WHERE id = 2"); err != nil { + t.Fatalf("remove migration 2 marker: %v", err) + } + _ = full.Close() + + ro, err := OpenReadOnly(path) + if err != nil { + t.Fatalf("OpenReadOnly: %v", err) + } + defer ro.Close() + + // Migration 2 marker must still be absent; packages_path must + // still exist. + var migCount int + if err := ro.db.QueryRow("SELECT COUNT(*) FROM schema_migrations WHERE id = 2").Scan(&migCount); err != nil { + t.Fatalf("query schema_migrations: %v", err) + } + if migCount != 0 { + t.Fatal("OpenReadOnly recorded migration 2 — the open path mutated the DB") + } + rows, err := ro.db.Query("PRAGMA table_info(images)") + if err != nil { + t.Fatalf("PRAGMA table_info: %v", err) + } + defer rows.Close() + var sawColumn bool + for rows.Next() { + var ( + cid int + name string + valueType string + notNull int + defaultV sql.NullString + pk int + ) + if err := rows.Scan(&cid, &name, &valueType, ¬Null, &defaultV, &pk); err != nil { + t.Fatalf("scan: %v", err) + } + if name == "packages_path" { + sawColumn = true + } + } + if !sawColumn { + t.Fatal("packages_path disappeared — OpenReadOnly ran the drop migration") + } +} + +// TestOpenReadOnlyRefusesWrites confirms SQLite's mode=ro is in effect +// — no matter what a caller tries, writes are rejected at the driver +// level. Belt-and-braces guard against a future refactor that might +// plumb a write method through. +func TestOpenReadOnlyRefusesWrites(t *testing.T) { + path := filepath.Join(t.TempDir(), "state.db") + if s, err := Open(path); err != nil { + t.Fatalf("seed Open: %v", err) + } else { + _ = s.Close() + } + ro, err := OpenReadOnly(path) + if err != nil { + t.Fatalf("OpenReadOnly: %v", err) + } + defer ro.Close() + if _, err := ro.db.Exec("INSERT INTO schema_migrations (id, name, applied_at) VALUES (999, 'x', 'x')"); err == nil { + t.Fatal("write succeeded against a read-only store") + } +} + func TestRunMigrationsRejectsDuplicateID(t *testing.T) { db := openRawDB(t) orig := migrations diff --git a/internal/store/store.go b/internal/store/store.go index f87b559..ad995dc 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -38,6 +38,35 @@ func Open(path string) (*Store, error) { return store, nil } +// OpenReadOnly opens the state DB without running migrations and with +// SQLite's mode=ro flag so no write can slip through — the file and +// its WAL sidecar stay untouched. Used by `banger doctor`, which must +// be pure inspection: running it should never mutate user state, and +// it must not trigger a schema migration the user didn't ask for. +// +// Returns the usual sql.ErrNoRows-compatible errors from the read +// queries if the DB's schema is older than the current code expects; +// doctor surfaces those as failing checks rather than a hard crash. +func OpenReadOnly(path string) (*Store, error) { + dsn, err := sqliteReadOnlyDSN(path) + if err != nil { + return nil, err + } + db, err := sql.Open("sqlite", dsn) + if err != nil { + return nil, err + } + // Ping forces SQLite to actually open the file, so a missing or + // unreadable DB fails here rather than at first query. Match the + // existing Open contract: caller expects success to mean "ready + // to read." + if err := db.Ping(); err != nil { + _ = db.Close() + return nil, err + } + return &Store{db: db}, nil +} + func (s *Store) Close() error { return s.db.Close() } @@ -66,6 +95,28 @@ func sqliteDSN(path string) (string, error) { }).String(), nil } +// sqliteReadOnlyDSN builds a DSN that opens the DB in SQLite's +// read-only mode. Deliberately omits journal_mode=WAL and the other +// write-adjacent pragmas set by sqliteDSN — mode=ro refuses them +// anyway, and keeping the list minimal means the query never touches +// the file. foreign_keys and busy_timeout are the only pragmas worth +// keeping for read paths (semantics parity + lock backoff). +func sqliteReadOnlyDSN(path string) (string, error) { + absPath, err := filepath.Abs(path) + if err != nil { + return "", fmt.Errorf("resolve sqlite path: %w", err) + } + query := url.Values{} + query.Set("mode", "ro") + query.Add("_pragma", "foreign_keys(1)") + query.Add("_pragma", "busy_timeout(5000)") + return (&url.URL{ + Scheme: "file", + Path: filepath.ToSlash(absPath), + RawQuery: query.Encode(), + }).String(), nil +} + func (s *Store) UpsertImage(ctx context.Context, image model.Image) error { s.writeMu.Lock() defer s.writeMu.Unlock()