From b1ca6594239032d6ebb1d187bd551d87c05482f0 Mon Sep 17 00:00:00 2001 From: Audrey Tang Date: Wed, 12 Aug 2026 10:23:55 +0800 Subject: [PATCH] fix(store): let the edge table rebuild run, and survive orphaned edges Two defects in migrateRemoveNarrativeEdges, both caused by foreign key enforcement being on while the migration works. The migration never runs. It probes for the old schema by inserting a sentinel edge whose endpoints do not exist: INSERT INTO edges VALUES ('__test','__test','narrative',...) The driver opens the database with foreign_keys(1), so that insert fails on the foreign key long before the CHECK constraint is consulted, and the error is read as "the schema already rejects 'narrative'". Every database opened through the driver therefore skips the migration and keeps the old constraint indefinitely. If it did run, it could still abort. The rebuild copies every row into a fresh table, and that copy is rejected by any pre-existing dangling edge. Orphans are not exotic: a `.recover` after corruption, or any write made while enforcement was off, leaves edges pointing at rows that are gone. One live store was found holding 51. Because the migration runs on every open, one such row strands the database permanently. Follow SQLite's documented table-rebuild procedure and disable enforcement across both the probe and the rebuild, restoring it after. The pragma is a no-op inside a transaction, so it is toggled around one; the pool is capped at a single connection, so it applies to the connection doing the work. Orphans are copied verbatim rather than filtered out. Dropping them would be a silent data deletion justified only by the convenience of this migration, and their presence is evidence worth keeping. The added test reproduces both defects: without the change it fails because the narrative type is still accepted afterwards. --- internal/memory/store/db.go | 51 ++++++++- internal/memory/store/store_test.go | 165 ++++++++++++++++++++++++++++ 2 files changed, 213 insertions(+), 3 deletions(-) diff --git a/internal/memory/store/db.go b/internal/memory/store/db.go index 3880a076..74b20f4d 100644 --- a/internal/memory/store/db.go +++ b/internal/memory/store/db.go @@ -437,8 +437,52 @@ func looksLikeLegacyVector(v []float64) bool { // migrateRemoveNarrativeEdges recreates the edges table without the 'narrative' type // if the old CHECK constraint still allows it. func (db *DB) migrateRemoveNarrativeEdges() error { - // Probe whether the old schema allows 'narrative' - _, testErr := db.conn.Exec(`INSERT INTO edges VALUES ('__test','__test','narrative',0,'{}',datetime('now'))`) + // Foreign key enforcement is disabled for both the probe and the rebuild. + // + // The probe needs it because it inserts a sentinel edge whose endpoints do + // not exist. The driver opens the database with foreign_keys(1), so the + // insert fails on the foreign key long before the CHECK constraint is + // consulted -- and the error was being read as "the schema already rejects + // 'narrative'". The migration therefore never ran on any database opened + // through the driver, and legacy stores still carry the old constraint. + // + // The rebuild needs it because copying every row into a fresh table is + // rejected by any pre-existing dangling edge, and real stores accumulate + // them: a `.recover` after corruption, or any write made while + // enforcement was off, leaves edges pointing at rows that are gone. One + // such row would abort the migration on every open. Orphans are copied + // verbatim rather than filtered out -- dropping them would be a silent + // data deletion justified only by the convenience of this migration. + // + // PRAGMA foreign_keys is a no-op inside a transaction, so it is toggled + // around one. The pool is capped at a single connection, so it applies to + // the connection performing the work. + if _, err := db.conn.Exec(`PRAGMA foreign_keys=OFF`); err != nil { + return fmt.Errorf("disable foreign keys for edge rebuild: %w", err) + } + defer func() { _, _ = db.conn.Exec(`PRAGMA foreign_keys=ON`) }() + + // The probe is rolled back, always. Written in autocommit -- which is what + // it was, and what disabling enforcement above now lets succeed -- the + // sentinel row outlives a rebuild that fails or a process that dies + // mid-migration. The next open's probe then collides with the leftover on + // the primary key, and this function reads any error as "the schema + // already rejects 'narrative'", so the migration is skipped from then on + // and the database is stranded on the old schema permanently: the same + // failure being repaired here, arriving through a different door. + // + // The id is random rather than a fixed '__test' because insight ids are + // caller-supplied. A fixed sentinel can collide with a real row, and a + // cleanup keyed on it can take real edges with it. + probeTx, err := db.conn.Begin() + if err != nil { + return fmt.Errorf("begin probe: %w", err) + } + _, testErr := probeTx.Exec( + `INSERT INTO edges VALUES ('__probe_'||hex(randomblob(8)),'__probe_'||hex(randomblob(8)),'narrative',0,'{}',datetime('now'))`) + if err := probeTx.Rollback(); err != nil { + return fmt.Errorf("roll back probe: %w", err) + } if testErr != nil { return nil // current schema already rejects 'narrative', nothing to do } @@ -451,7 +495,8 @@ func (db *DB) migrateRemoveNarrativeEdges() error { defer tx.Rollback() steps := []string{ - `DELETE FROM edges WHERE source_id = '__test'`, + // A sentinel left behind by the older autocommit probe is a + // 'narrative' row, so the next step removes it along with the rest. `DELETE FROM edges WHERE edge_type = 'narrative'`, `ALTER TABLE edges RENAME TO edges_old`, `CREATE TABLE edges ( diff --git a/internal/memory/store/store_test.go b/internal/memory/store/store_test.go index 21067abf..999ac85c 100644 --- a/internal/memory/store/store_test.go +++ b/internal/memory/store/store_test.go @@ -452,6 +452,171 @@ func TestBaseWeight(t *testing.T) { } } +// --- edge table rebuild --- + +// allowNarrativeEdges restores the historical edges schema, which still admits +// the 'narrative' edge type, by rewriting the stored CHECK constraint in place. +func allowNarrativeEdges(t *testing.T, db *DB) { + t.Helper() + for _, s := range []string{ + `PRAGMA writable_schema=ON`, + `UPDATE sqlite_master SET sql = replace(sql, "'entity'", "'entity','narrative'") WHERE name = 'edges'`, + `PRAGMA writable_schema=OFF`, + } { + if _, err := db.conn.Exec(s); err != nil { + t.Fatalf("%s: %v", s, err) + } + } +} + +// Removing the 'narrative' edge type rebuilds the edges table, copying every +// row into a fresh one. The driver enables foreign key enforcement, so a +// single pre-existing dangling edge aborts that copy -- and because the +// migration runs on every open, the database can never get past it. +// +// Orphans are not exotic. A `.recover` rebuild after corruption, or any write +// made while enforcement was off, produces them; one live store was found +// holding 51. They must survive the rebuild verbatim rather than being +// silently dropped. +func TestMigrateRemoveNarrativeEdges_ToleratesOrphanedEdges(t *testing.T) { + dir := t.TempDir() + + db, err := Open(dir) + if err != nil { + t.Fatalf("open: %v", err) + } + if err := db.InsertInsight(makeInsight("narr-src", "content", 3)); err != nil { + t.Fatalf("insert insight: %v", err) + } + + // Restore the historical schema that still admits 'narrative', and plant + // an orphan the way real damage arrives: with enforcement off. + if _, err := db.conn.Exec(`PRAGMA foreign_keys=OFF`); err != nil { + t.Fatalf("disable fk: %v", err) + } + allowNarrativeEdges(t, db) + if _, err := db.conn.Exec( + `INSERT INTO edges VALUES ('narr-src','vanished','semantic',0.5,'{}','2026-01-01T00:00:00Z')`); err != nil { + t.Fatalf("insert orphaned edge: %v", err) + } + db.Close() + + // Reopening runs migrate(); the rebuild must not be blocked by the orphan. + reopened, err := Open(dir) + if err != nil { + t.Fatalf("migration must survive an orphaned edge, got: %v", err) + } + defer reopened.Close() + + var orphans int + if err := reopened.conn.QueryRow( + `SELECT COUNT(*) FROM edges WHERE target_id = 'vanished'`).Scan(&orphans); err != nil { + t.Fatalf("count orphan: %v", err) + } + if orphans != 1 { + t.Errorf("orphaned edge must survive the rebuild verbatim, got %d", orphans) + } + + // And the migration actually did its job. + if _, err := reopened.conn.Exec( + `INSERT INTO edges VALUES ('narr-src','narr-src','narrative',1,'{}','2026-01-01T00:00:00Z')`); err == nil { + t.Error("narrative edge type must be rejected after migration") + } +} + +// The probe writes a sentinel edge to learn whether the CHECK still admits +// 'narrative'. Written outside a transaction it outlives a rebuild that fails +// or a process that dies mid-migration, and the next probe then collides with +// the leftover on the primary key. This function reads any error as "the +// schema already rejects narrative", so the rebuild is skipped from then on +// and the database is stranded on the old schema permanently. +func TestMigrateRemoveNarrativeEdges_ProbeSurvivesNothing(t *testing.T) { + dir := t.TempDir() + + // A second handle, kept open, is the only way to inspect and repair the + // file between two failed opens: Open closes its connection when migrate + // fails and hands back nothing to inspect with. + side, err := Open(dir) + if err != nil { + t.Fatalf("open: %v", err) + } + defer side.Close() + if err := side.InsertInsight(makeInsight("probe-src", "content", 3)); err != nil { + t.Fatalf("insert insight: %v", err) + } + allowNarrativeEdges(t, side) + + // Fail the rebuild the way a crash would: after the probe has run. An + // existing `edges_old` makes the rename step fail deterministically. + if _, err := side.conn.Exec(`CREATE TABLE edges_old (x)`); err != nil { + t.Fatalf("block rebuild: %v", err) + } + if _, err := Open(dir); err == nil { + t.Fatal("blocked rebuild must be reported, not silently skipped") + } + + var left int + if err := side.conn.QueryRow( + `SELECT COUNT(*) FROM edges WHERE edge_type = 'narrative'`).Scan(&left); err != nil { + t.Fatalf("count leftovers: %v", err) + } + if left != 0 { + t.Errorf("probe row outlived a failed migration: %d left behind", left) + } + + // Unblocked, the migration must still run. This is the assertion that + // matters: a surviving probe row makes every later attempt collide, and + // the database keeps the old constraint for good. + if _, err := side.conn.Exec(`DROP TABLE edges_old`); err != nil { + t.Fatalf("unblock rebuild: %v", err) + } + reopened, err := Open(dir) + if err != nil { + t.Fatalf("reopen after unblocking: %v", err) + } + defer reopened.Close() + if _, err := reopened.conn.Exec( + `INSERT INTO edges VALUES ('probe-src','probe-src','narrative',1,'{}','2026-01-01T00:00:00Z')`); err == nil { + t.Error("narrative edge type must be rejected after migration") + } +} + +// Insight ids are caller-supplied, so the fixed '__test' sentinel could name a +// real insight -- and the migration deleted edges by that id to clean up after +// itself. +func TestMigrateRemoveNarrativeEdges_KeepsRealEdgesNamedLikeTheSentinel(t *testing.T) { + dir := t.TempDir() + + db, err := Open(dir) + if err != nil { + t.Fatalf("open: %v", err) + } + if err := db.InsertInsight(makeInsight("__test", "content", 3)); err != nil { + t.Fatalf("insert insight: %v", err) + } + allowNarrativeEdges(t, db) + if _, err := db.conn.Exec( + `INSERT INTO edges VALUES ('__test','__test','semantic',0.5,'{}','2026-01-01T00:00:00Z')`); err != nil { + t.Fatalf("insert edge: %v", err) + } + db.Close() + + reopened, err := Open(dir) + if err != nil { + t.Fatalf("reopen: %v", err) + } + defer reopened.Close() + + var kept int + if err := reopened.conn.QueryRow( + `SELECT COUNT(*) FROM edges WHERE source_id = '__test' AND edge_type = 'semantic'`).Scan(&kept); err != nil { + t.Fatalf("count: %v", err) + } + if kept != 1 { + t.Errorf("a real edge was deleted because its id matched the probe sentinel, got %d", kept) + } +} + // --- AutoPrune --- func TestAutoPrune_PrunesLowestEI(t *testing.T) {