doctor: open the state DB read-only so inspection never mutates it
`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) <noreply@anthropic.com>
This commit is contained in:
parent
129475be20
commit
2685bc73f8
3 changed files with 141 additions and 1 deletions
|
|
@ -24,7 +24,12 @@ func Doctor(ctx context.Context) (system.Report, error) {
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return system.Report{}, err
|
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{
|
d := &Daemon{
|
||||||
layout: layout,
|
layout: layout,
|
||||||
config: cfg,
|
config: cfg,
|
||||||
|
|
|
||||||
|
|
@ -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) {
|
func TestRunMigrationsRejectsDuplicateID(t *testing.T) {
|
||||||
db := openRawDB(t)
|
db := openRawDB(t)
|
||||||
orig := migrations
|
orig := migrations
|
||||||
|
|
|
||||||
|
|
@ -38,6 +38,35 @@ func Open(path string) (*Store, error) {
|
||||||
return store, nil
|
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 {
|
func (s *Store) Close() error {
|
||||||
return s.db.Close()
|
return s.db.Close()
|
||||||
}
|
}
|
||||||
|
|
@ -66,6 +95,28 @@ func sqliteDSN(path string) (string, error) {
|
||||||
}).String(), nil
|
}).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 {
|
func (s *Store) UpsertImage(ctx context.Context, image model.Image) error {
|
||||||
s.writeMu.Lock()
|
s.writeMu.Lock()
|
||||||
defer s.writeMu.Unlock()
|
defer s.writeMu.Unlock()
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue