From 64d5f20b90571d673e5a439cbfbb7c04b8d588c0 Mon Sep 17 00:00:00 2001 From: Steve Yegge Date: Mon, 29 Dec 2025 13:46:47 -0800 Subject: [PATCH] fix: Add pre-migration orphan cleanup to avoid chicken-and-egg failure (bd-eko4) When the database has orphaned foreign key references (dependencies or labels pointing to non-existent issues), the migration invariant check would fail, preventing the database from opening. This created a chicken-and-egg problem: 1. bd doctor --fix tries to open the database 2. Opening triggers migrations with invariant checks 3. Invariant check fails due to orphaned refs 4. Fix never runs because database won't open The fix adds CleanOrphanedRefs() that runs BEFORE captureSnapshot() in RunMigrations. This automatically cleans up orphaned dependencies and labels (preserving external:* refs), allowing the database to open normally. Added test coverage for the cleanup function. --- .../storage/sqlite/migration_invariants.go | 43 +++++++ .../sqlite/migration_invariants_test.go | 105 ++++++++++++++++++ internal/storage/sqlite/migrations.go | 7 ++ 3 files changed, 155 insertions(+) diff --git a/internal/storage/sqlite/migration_invariants.go b/internal/storage/sqlite/migration_invariants.go index b1e3b105..34baf56a 100644 --- a/internal/storage/sqlite/migration_invariants.go +++ b/internal/storage/sqlite/migration_invariants.go @@ -205,3 +205,46 @@ func GetInvariantNames() []string { sort.Strings(names) return names } + +// CleanOrphanedRefs removes orphaned dependencies and labels that reference non-existent issues. +// This runs BEFORE migrations to prevent the chicken-and-egg problem where: +// 1. bd doctor --fix tries to open the database +// 2. Opening triggers migrations with invariant checks +// 3. Invariant check fails due to orphaned refs from prior tombstone deletion +// 4. Fix never runs because database won't open +// +// Returns counts of cleaned items for logging. +func CleanOrphanedRefs(db *sql.DB) (deps int, labels int, err error) { + // Clean orphaned dependencies (issue_id not in issues) + result, err := db.Exec(` + DELETE FROM dependencies + WHERE NOT EXISTS (SELECT 1 FROM issues WHERE id = issue_id) + `) + if err != nil { + return 0, 0, fmt.Errorf("failed to clean orphaned dependencies (issue_id): %w", err) + } + depsIssue, _ := result.RowsAffected() + + // Clean orphaned dependencies (depends_on_id not in issues, excluding external refs) + result, err = db.Exec(` + DELETE FROM dependencies + WHERE NOT EXISTS (SELECT 1 FROM issues WHERE id = depends_on_id) + AND depends_on_id NOT LIKE 'external:%' + `) + if err != nil { + return 0, 0, fmt.Errorf("failed to clean orphaned dependencies (depends_on_id): %w", err) + } + depsDependsOn, _ := result.RowsAffected() + + // Clean orphaned labels (issue_id not in issues) + result, err = db.Exec(` + DELETE FROM labels + WHERE NOT EXISTS (SELECT 1 FROM issues WHERE id = issue_id) + `) + if err != nil { + return 0, 0, fmt.Errorf("failed to clean orphaned labels: %w", err) + } + labelsCount, _ := result.RowsAffected() + + return int(depsIssue + depsDependsOn), int(labelsCount), nil +} diff --git a/internal/storage/sqlite/migration_invariants_test.go b/internal/storage/sqlite/migration_invariants_test.go index bf600764..7b201146 100644 --- a/internal/storage/sqlite/migration_invariants_test.go +++ b/internal/storage/sqlite/migration_invariants_test.go @@ -248,6 +248,111 @@ func TestGetInvariantNames(t *testing.T) { } } +func TestCleanOrphanedRefs(t *testing.T) { + db := setupInvariantTestDB(t) + defer db.Close() + + // Create a valid issue + _, err := db.Exec(`INSERT INTO issues (id, title) VALUES ('test-1', 'Test Issue')`) + if err != nil { + t.Fatalf("failed to insert test issue: %v", err) + } + + // Create valid dependency and label + _, err = db.Exec(`INSERT INTO dependencies (issue_id, depends_on_id, created_by) VALUES ('test-1', 'test-1', 'test')`) + if err != nil { + t.Fatalf("failed to insert valid dependency: %v", err) + } + + _, err = db.Exec(`INSERT INTO labels (issue_id, label) VALUES ('test-1', 'valid-label')`) + if err != nil { + t.Fatalf("failed to insert valid label: %v", err) + } + + // Disable FK constraints to create orphaned refs + _, err = db.Exec(`PRAGMA foreign_keys = OFF`) + if err != nil { + t.Fatalf("failed to disable foreign keys: %v", err) + } + + // Create orphaned dependency (issue_id not in issues) + _, err = db.Exec(`INSERT INTO dependencies (issue_id, depends_on_id, created_by) VALUES ('orphan-1', 'test-1', 'test')`) + if err != nil { + t.Fatalf("failed to insert orphaned dependency (issue_id): %v", err) + } + + // Create orphaned dependency (depends_on_id not in issues) + _, err = db.Exec(`INSERT INTO dependencies (issue_id, depends_on_id, created_by) VALUES ('test-1', 'orphan-2', 'test')`) + if err != nil { + t.Fatalf("failed to insert orphaned dependency (depends_on_id): %v", err) + } + + // Create external dependency (should NOT be cleaned) + _, err = db.Exec(`INSERT INTO dependencies (issue_id, depends_on_id, created_by) VALUES ('test-1', 'external:proj:cap', 'test')`) + if err != nil { + t.Fatalf("failed to insert external dependency: %v", err) + } + + // Create orphaned label + _, err = db.Exec(`INSERT INTO labels (issue_id, label) VALUES ('orphan-3', 'orphan-label')`) + if err != nil { + t.Fatalf("failed to insert orphaned label: %v", err) + } + + _, err = db.Exec(`PRAGMA foreign_keys = ON`) + if err != nil { + t.Fatalf("failed to enable foreign keys: %v", err) + } + + // Verify we have orphaned refs (checkForeignKeys should fail) + err = checkForeignKeys(db, &Snapshot{}) + if err == nil { + t.Error("expected checkForeignKeys to fail with orphaned refs") + } + + // Run cleanup + deps, labels, err := CleanOrphanedRefs(db) + if err != nil { + t.Fatalf("CleanOrphanedRefs failed: %v", err) + } + + // Should have cleaned 2 orphaned deps (orphan-1 and orphan-2, not external) + if deps != 2 { + t.Errorf("expected 2 orphaned deps cleaned, got %d", deps) + } + + // Should have cleaned 1 orphaned label + if labels != 1 { + t.Errorf("expected 1 orphaned label cleaned, got %d", labels) + } + + // Verify checkForeignKeys now passes + err = checkForeignKeys(db, &Snapshot{}) + if err != nil { + t.Errorf("expected checkForeignKeys to pass after cleanup, got: %v", err) + } + + // Verify valid data is still present + var depCount, labelCount int + err = db.QueryRow(`SELECT COUNT(*) FROM dependencies`).Scan(&depCount) + if err != nil { + t.Fatalf("failed to count dependencies: %v", err) + } + // Should have 2: valid self-ref + external ref + if depCount != 2 { + t.Errorf("expected 2 dependencies remaining, got %d", depCount) + } + + err = db.QueryRow(`SELECT COUNT(*) FROM labels`).Scan(&labelCount) + if err != nil { + t.Fatalf("failed to count labels: %v", err) + } + // Should have 1: valid label + if labelCount != 1 { + t.Errorf("expected 1 label remaining, got %d", labelCount) + } +} + // setupInvariantTestDB creates an in-memory test database with schema func setupInvariantTestDB(t *testing.T) *sql.DB { t.Helper() diff --git a/internal/storage/sqlite/migrations.go b/internal/storage/sqlite/migrations.go index ceba5a79..bf2e13d6 100644 --- a/internal/storage/sqlite/migrations.go +++ b/internal/storage/sqlite/migrations.go @@ -142,6 +142,13 @@ func RunMigrations(db *sql.DB) error { } }() + // Pre-migration cleanup: remove orphaned refs that would fail invariant checks. + // This prevents the chicken-and-egg problem where the database can't open + // due to orphans left behind by tombstone deletion (see bd-eko4). + if _, _, err := CleanOrphanedRefs(db); err != nil { + return fmt.Errorf("pre-migration orphan cleanup failed: %w", err) + } + snapshot, err := captureSnapshot(db) if err != nil { return fmt.Errorf("failed to capture pre-migration snapshot: %w", err)