From f6290b63736aaa6db05bac146fa084abd08ab997 Mon Sep 17 00:00:00 2001 From: MeshCore Bot Date: Tue, 19 May 2026 06:24:37 +0000 Subject: [PATCH] =?UTF-8?q?test(#1283):=20RED=20=E2=80=94=20server=20*DB?= =?UTF-8?q?=20has=20no=20write=20methods;=20ingestor=20owns=20PruneOldPack?= =?UTF-8?q?ets?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Enforces issue #1283 architecture: cmd/server is read-only, all write/maintenance ops live on the ingestor's *Store. Three new tests: - TestServerDBHasNoWriteMethods — reflect-asserts PruneOldPackets, PruneOldMetrics, RemoveStaleObservers are NOT methods on cmd/server *DB. Fails on master (all three currently exist + use cachedRW to bypass the server's read-only handle, racing ingestor INSERTs → SQLITE_BUSY). - TestServerDBConnIsReadOnly — opens via OpenDB, asserts INSERT fails. Today this passes via OpenDB(mode=ro), but pinning it as an invariant prevents future regressions if anyone ever drops the ro flag. - TestIngestorPruneOldPackets — exercises new Store.PruneOldPackets that the GREEN commit will implement. Stub returns 0; test asserts 2 rows pruned → fails (RED proof). Plus TestIngestorVacuumOnStartupMigratesNONEtoINCREMENTAL guarding the existing CheckAutoVacuum path so the GREEN commit's deletions in cmd/server cannot regress the vacuum migration. --- cmd/ingestor/issue1283_test.go | 98 +++++++++++++++++++++++++++ cmd/ingestor/maintenance.go | 10 +++ cmd/server/readonly_invariant_test.go | 77 +++++++++++++++++++++ 3 files changed, 185 insertions(+) create mode 100644 cmd/ingestor/issue1283_test.go create mode 100644 cmd/ingestor/maintenance.go create mode 100644 cmd/server/readonly_invariant_test.go diff --git a/cmd/ingestor/issue1283_test.go b/cmd/ingestor/issue1283_test.go new file mode 100644 index 00000000..f19936ba --- /dev/null +++ b/cmd/ingestor/issue1283_test.go @@ -0,0 +1,98 @@ +package main + +import ( + "database/sql" + "path/filepath" + "testing" + "time" + + _ "modernc.org/sqlite" +) + +// TestIngestorPruneOldPackets enforces #1283: the writer for +// transmissions retention lives on the ingestor's *Store. Before the fix, +// this lived on cmd/server/*DB and raced with ingestor INSERTs. After +// the fix, ingestor owns it and runs it on its own write-locked handle. +func TestIngestorPruneOldPackets(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "prune.db") + store, err := OpenStore(path) + if err != nil { + t.Fatalf("OpenStore: %v", err) + } + defer store.Close() + + old := time.Now().UTC().AddDate(0, 0, -10).Format(time.RFC3339) + new := time.Now().UTC().Format(time.RFC3339) + for i, ts := range []string{old, old, new} { + _, err := store.db.Exec( + `INSERT INTO transmissions (raw_hex, hash, first_seen, route_type, payload_type, payload_version, decoded_json) + VALUES (?, ?, ?, 0, 1, 1, '{}')`, + "AA", "h"+string(rune('a'+i)), ts, + ) + if err != nil { + t.Fatalf("seed tx: %v", err) + } + } + + n, err := store.PruneOldPackets(5) + if err != nil { + t.Fatalf("PruneOldPackets: %v", err) + } + if n != 2 { + t.Fatalf("expected 2 pruned, got %d", n) + } + + var remaining int + if err := store.db.QueryRow(`SELECT COUNT(*) FROM transmissions`).Scan(&remaining); err != nil { + t.Fatalf("count: %v", err) + } + if remaining != 1 { + t.Fatalf("expected 1 transmission remaining, got %d", remaining) + } +} + +// TestIngestorVacuumOnStartupMigratesNONEtoINCREMENTAL exercises the +// scenario that originally broke in #1283: a fresh DB with +// auto_vacuum=NONE, vacuumOnStartup=true, no contention from a server +// process. The ingestor must complete the VACUUM and flip auto_vacuum to +// INCREMENTAL. Before the fix, the migration ran inside cmd/server and +// hit SQLITE_BUSY because the ingestor (sharing the container) was +// already writing. +func TestIngestorVacuumOnStartupMigratesNONEtoINCREMENTAL(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "vac.db") + + // Create a NONE-auto_vacuum DB (simulates an older deployment). + seed, err := sql.Open("sqlite", path+"?_pragma=journal_mode(WAL)") + if err != nil { + t.Fatal(err) + } + seed.SetMaxOpenConns(1) + if _, err := seed.Exec(`CREATE TABLE dummy(id INTEGER PRIMARY KEY)`); err != nil { + t.Fatal(err) + } + var before int + seed.QueryRow("PRAGMA auto_vacuum").Scan(&before) + if before != 0 { + t.Fatalf("precondition: auto_vacuum=%d, want 0", before) + } + seed.Close() + + store, err := OpenStore(path) + if err != nil { + t.Fatalf("OpenStore: %v", err) + } + defer store.Close() + + cfg := &Config{DB: &DBConfig{VacuumOnStartup: true}} + store.CheckAutoVacuum(cfg) + + var after int + if err := store.db.QueryRow("PRAGMA auto_vacuum").Scan(&after); err != nil { + t.Fatal(err) + } + if after != 2 { + t.Fatalf("expected auto_vacuum=2 after ingestor VACUUM, got %d", after) + } +} diff --git a/cmd/ingestor/maintenance.go b/cmd/ingestor/maintenance.go new file mode 100644 index 00000000..055cda67 --- /dev/null +++ b/cmd/ingestor/maintenance.go @@ -0,0 +1,10 @@ +package main + +// PruneOldPackets deletes transmissions (and their observations) older +// than the given number of days. Returns count of transmissions deleted. +// Owned by the ingestor per #1283 (the writer process). +// +// Stub: real implementation lands in the GREEN commit. +func (s *Store) PruneOldPackets(days int) (int64, error) { + return 0, nil +} diff --git a/cmd/server/readonly_invariant_test.go b/cmd/server/readonly_invariant_test.go new file mode 100644 index 00000000..629009d8 --- /dev/null +++ b/cmd/server/readonly_invariant_test.go @@ -0,0 +1,77 @@ +package main + +import ( + "database/sql" + "fmt" + "reflect" + "testing" + + _ "modernc.org/sqlite" +) + +// TestServerDBHasNoWriteMethods enforces the architectural invariant from +// issue #1283: cmd/server is the read path. All write/maintenance methods +// (PruneOldPackets, PruneOldMetrics, RemoveStaleObservers) MUST live on +// the ingestor's *Store, not on the server's *DB. +// +// Before the fix, these methods existed on cmd/server/*DB and used +// cachedRW(db.path) to acquire a write lock, racing with the ingestor's +// concurrent INSERTs and producing SQLITE_BUSY (the bug in #1283). +// After the fix, this test passes because the methods are gone. +func TestServerDBHasNoWriteMethods(t *testing.T) { + forbidden := []string{ + "PruneOldPackets", + "PruneOldMetrics", + "RemoveStaleObservers", + } + typ := reflect.TypeOf((*DB)(nil)) + for _, name := range forbidden { + if _, ok := typ.MethodByName(name); ok { + t.Errorf("server *DB exposes forbidden write method %q — must be relocated to ingestor (#1283)", name) + } + } +} + +// TestServerDBConnIsReadOnly asserts that the *sql.DB the server opens +// cannot acquire a write lock. The server has always opened mode=ro, but +// before #1283 it routed around that by calling cachedRW(path) to get a +// second RW handle. After the fix, server-side writes are impossible +// because there is no helper to open a writable connection. +func TestServerDBConnIsReadOnly(t *testing.T) { + dir := t.TempDir() + path := dir + "/ro_invariant.db" + + // Bootstrap a minimal DB with the ingestor-style WAL opener so the + // server can attach in read-only mode. + if err := bootstrapMinimalDB(path); err != nil { + t.Fatalf("bootstrap: %v", err) + } + + d, err := OpenDB(path) + if err != nil { + t.Fatalf("OpenDB: %v", err) + } + defer d.conn.Close() + + _, err = d.conn.Exec(`INSERT INTO nodes (public_key, name) VALUES ('x','y')`) + if err == nil { + t.Fatalf("expected INSERT via server *DB to fail (read-only invariant)") + } +} + +// bootstrapMinimalDB creates a tiny DB with the columns these tests +// need, opened with WAL so the read-only opener in OpenDB can attach. +// Kept in *_test.go so it does NOT add any write capability to the +// production server binary. +func bootstrapMinimalDB(path string) error { + dsn := fmt.Sprintf("file:%s?_journal_mode=WAL&_busy_timeout=5000", path) + rw, err := sql.Open("sqlite", dsn) + if err != nil { + return err + } + defer rw.Close() + if _, err := rw.Exec(`CREATE TABLE IF NOT EXISTS nodes (public_key TEXT PRIMARY KEY, name TEXT)`); err != nil { + return err + } + return nil +}