diff --git a/cmd/bd/import.go b/cmd/bd/import.go index 92f46b94..ee12b37a 100644 --- a/cmd/bd/import.go +++ b/cmd/bd/import.go @@ -240,9 +240,11 @@ Behavior: // Phase 5: Sync ID counters after importing issues with explicit IDs // This prevents ID collisions with subsequently auto-generated issues + // CRITICAL: If this fails, subsequent auto-generated IDs WILL collide with imported issues if err := sqliteStore.SyncAllCounters(ctx); err != nil { - fmt.Fprintf(os.Stderr, "Warning: failed to sync ID counters: %v\n", err) - // Don't exit - this is not fatal, just a warning + fmt.Fprintf(os.Stderr, "Error: failed to sync ID counters: %v\n", err) + fmt.Fprintf(os.Stderr, "Cannot proceed - auto-generated IDs would collide with imported issues.\n") + os.Exit(1) } // Phase 6: Process dependencies diff --git a/internal/storage/sqlite/lazy_init_test.go b/internal/storage/sqlite/lazy_init_test.go new file mode 100644 index 00000000..7698d110 --- /dev/null +++ b/internal/storage/sqlite/lazy_init_test.go @@ -0,0 +1,217 @@ +package sqlite + +import ( + "context" + "os" + "path/filepath" + "testing" + + "github.com/steveyegge/beads/internal/types" +) + +// TestLazyCounterInitialization verifies that counters are initialized lazily +// on first use, not by scanning the entire database on every CreateIssue +func TestLazyCounterInitialization(t *testing.T) { + // Create temporary directory + tmpDir, err := os.MkdirTemp("", "beads-lazy-init-test-*") + if err != nil { + t.Fatalf("failed to create temp dir: %v", err) + } + defer os.RemoveAll(tmpDir) + + dbPath := filepath.Join(tmpDir, "test.db") + + // Initialize database + store, err := New(dbPath) + if err != nil { + t.Fatalf("failed to create storage: %v", err) + } + defer store.Close() + + ctx := context.Background() + + // Create some issues with explicit IDs (simulating import) + existingIssues := []string{"bd-5", "bd-10", "bd-15"} + for _, id := range existingIssues { + issue := &types.Issue{ + ID: id, + Title: "Existing issue", + Status: types.StatusOpen, + Priority: 2, + IssueType: types.TypeTask, + } + err := store.CreateIssue(ctx, issue, "test-user") + if err != nil { + t.Fatalf("CreateIssue with explicit ID failed: %v", err) + } + } + + // Verify no counter exists yet (lazy init hasn't happened) + var count int + err = store.db.QueryRow(`SELECT COUNT(*) FROM issue_counters WHERE prefix = 'bd'`).Scan(&count) + if err != nil { + t.Fatalf("Failed to query counters: %v", err) + } + + if count != 0 { + t.Errorf("Expected no counter yet, but found %d", count) + } + + // Now create an issue with auto-generated ID + // This should trigger lazy initialization + autoIssue := &types.Issue{ + Title: "Auto-generated ID", + Status: types.StatusOpen, + Priority: 2, + IssueType: types.TypeTask, + } + + err = store.CreateIssue(ctx, autoIssue, "test-user") + if err != nil { + t.Fatalf("CreateIssue with auto ID failed: %v", err) + } + + // Verify the ID is correct (should be bd-16, after bd-15) + if autoIssue.ID != "bd-16" { + t.Errorf("Expected bd-16, got %s", autoIssue.ID) + } + + // Verify counter was initialized + var lastID int + err = store.db.QueryRow(`SELECT last_id FROM issue_counters WHERE prefix = 'bd'`).Scan(&lastID) + if err != nil { + t.Fatalf("Failed to query counter: %v", err) + } + + if lastID != 16 { + t.Errorf("Expected counter at 16, got %d", lastID) + } + + // Create another issue - should NOT re-scan, just increment + anotherIssue := &types.Issue{ + Title: "Another auto-generated ID", + Status: types.StatusOpen, + Priority: 2, + IssueType: types.TypeTask, + } + + err = store.CreateIssue(ctx, anotherIssue, "test-user") + if err != nil { + t.Fatalf("CreateIssue failed: %v", err) + } + + if anotherIssue.ID != "bd-17" { + t.Errorf("Expected bd-17, got %s", anotherIssue.ID) + } +} + +// TestLazyCounterInitializationMultiplePrefix tests lazy init with multiple prefixes +func TestLazyCounterInitializationMultiplePrefix(t *testing.T) { + store, cleanup := setupTestDB(t) + defer cleanup() + + ctx := context.Background() + + // Set a custom prefix + err := store.SetConfig(ctx, "issue_prefix", "custom") + if err != nil { + t.Fatalf("SetConfig failed: %v", err) + } + + // Create issue with default prefix first + err = store.SetConfig(ctx, "issue_prefix", "bd") + if err != nil { + t.Fatalf("SetConfig failed: %v", err) + } + + bdIssue := &types.Issue{ + Title: "BD issue", + Status: types.StatusOpen, + Priority: 2, + IssueType: types.TypeTask, + } + + err = store.CreateIssue(ctx, bdIssue, "test-user") + if err != nil { + t.Fatalf("CreateIssue failed: %v", err) + } + + if bdIssue.ID != "bd-1" { + t.Errorf("Expected bd-1, got %s", bdIssue.ID) + } + + // Now switch to custom prefix + err = store.SetConfig(ctx, "issue_prefix", "custom") + if err != nil { + t.Fatalf("SetConfig failed: %v", err) + } + + customIssue := &types.Issue{ + Title: "Custom issue", + Status: types.StatusOpen, + Priority: 2, + IssueType: types.TypeTask, + } + + err = store.CreateIssue(ctx, customIssue, "test-user") + if err != nil { + t.Fatalf("CreateIssue failed: %v", err) + } + + if customIssue.ID != "custom-1" { + t.Errorf("Expected custom-1, got %s", customIssue.ID) + } + + // Verify both counters exist + var count int + err = store.db.QueryRow(`SELECT COUNT(*) FROM issue_counters`).Scan(&count) + if err != nil { + t.Fatalf("Failed to query counters: %v", err) + } + + if count != 2 { + t.Errorf("Expected 2 counters, got %d", count) + } +} + +// TestCounterInitializationFromExisting tests that the counter +// correctly initializes from the max ID of existing issues +func TestCounterInitializationFromExisting(t *testing.T) { + store, cleanup := setupTestDB(t) + defer cleanup() + + ctx := context.Background() + + // Create issues with explicit IDs, out of order + explicitIDs := []string{"bd-5", "bd-100", "bd-42", "bd-7"} + for _, id := range explicitIDs { + issue := &types.Issue{ + ID: id, + Title: "Explicit ID", + Status: types.StatusOpen, + Priority: 2, + IssueType: types.TypeTask, + } + err := store.CreateIssue(ctx, issue, "test-user") + if err != nil { + t.Fatalf("CreateIssue failed: %v", err) + } + } + + // Now auto-generate - should start at 101 (max is bd-100) + autoIssue := &types.Issue{ + Title: "Auto ID", + Status: types.StatusOpen, + Priority: 2, + IssueType: types.TypeTask, + } + + err := store.CreateIssue(ctx, autoIssue, "test-user") + if err != nil { + t.Fatalf("CreateIssue failed: %v", err) + } + + if autoIssue.ID != "bd-101" { + t.Errorf("Expected bd-101 (max was bd-100), got %s", autoIssue.ID) + } +} diff --git a/internal/storage/sqlite/migration_test.go b/internal/storage/sqlite/migration_test.go new file mode 100644 index 00000000..50a064e5 --- /dev/null +++ b/internal/storage/sqlite/migration_test.go @@ -0,0 +1,285 @@ +package sqlite + +import ( + "context" + "database/sql" + "os" + "path/filepath" + "testing" + + _ "github.com/mattn/go-sqlite3" + "github.com/steveyegge/beads/internal/types" +) + +// TestMigrateIssueCountersTable tests that the migration properly creates +// the issue_counters table and syncs it from existing issues +func TestMigrateIssueCountersTable(t *testing.T) { + // Create temporary directory + tmpDir, err := os.MkdirTemp("", "beads-migration-test-*") + if err != nil { + t.Fatalf("failed to create temp dir: %v", err) + } + defer os.RemoveAll(tmpDir) + + dbPath := filepath.Join(tmpDir, "test.db") + + // Step 1: Create database with old schema (no issue_counters table) + db, err := sql.Open("sqlite3", dbPath+"?_journal_mode=WAL&_foreign_keys=ON") + if err != nil { + t.Fatalf("failed to open database: %v", err) + } + + // Create minimal schema (issues table only, no issue_counters) + _, err = db.Exec(` + CREATE TABLE IF NOT EXISTS issues ( + id TEXT PRIMARY KEY, + title TEXT NOT NULL, + description TEXT NOT NULL DEFAULT '', + design TEXT NOT NULL DEFAULT '', + acceptance_criteria TEXT NOT NULL DEFAULT '', + notes TEXT NOT NULL DEFAULT '', + status TEXT NOT NULL DEFAULT 'open', + priority INTEGER NOT NULL DEFAULT 2, + issue_type TEXT NOT NULL DEFAULT 'task', + assignee TEXT, + estimated_minutes INTEGER, + created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, + updated_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, + closed_at DATETIME + ); + + CREATE TABLE IF NOT EXISTS config ( + key TEXT PRIMARY KEY, + value TEXT NOT NULL + ); + `) + if err != nil { + t.Fatalf("failed to create old schema: %v", err) + } + + // Insert some existing issues with IDs + _, err = db.Exec(` + INSERT INTO issues (id, title, status, priority, issue_type) + VALUES + ('bd-5', 'Issue 5', 'open', 2, 'task'), + ('bd-10', 'Issue 10', 'open', 2, 'task'), + ('bd-15', 'Issue 15', 'open', 2, 'task'), + ('custom-3', 'Custom 3', 'open', 2, 'task'), + ('custom-7', 'Custom 7', 'open', 2, 'task') + `) + if err != nil { + t.Fatalf("failed to insert test issues: %v", err) + } + + // Verify issue_counters table doesn't exist yet + var tableName string + err = db.QueryRow(` + SELECT name FROM sqlite_master + WHERE type='table' AND name='issue_counters' + `).Scan(&tableName) + if err != sql.ErrNoRows { + t.Fatalf("Expected issue_counters table to not exist, but it does") + } + + db.Close() + + // Step 2: Open database with New() which should trigger migration + store, err := New(dbPath) + if err != nil { + t.Fatalf("failed to create storage (migration failed): %v", err) + } + defer store.Close() + + // Step 3: Verify issue_counters table now exists + err = store.db.QueryRow(` + SELECT name FROM sqlite_master + WHERE type='table' AND name='issue_counters' + `).Scan(&tableName) + if err != nil { + t.Fatalf("Expected issue_counters table to exist after migration: %v", err) + } + + // Step 4: Verify counters were synced correctly + ctx := context.Background() + + // Check bd prefix counter (max is bd-15) + var bdCounter int + err = store.db.QueryRowContext(ctx, + `SELECT last_id FROM issue_counters WHERE prefix = 'bd'`).Scan(&bdCounter) + if err != nil { + t.Fatalf("Failed to query bd counter: %v", err) + } + if bdCounter != 15 { + t.Errorf("Expected bd counter to be 15, got %d", bdCounter) + } + + // Check custom prefix counter (max is custom-7) + var customCounter int + err = store.db.QueryRowContext(ctx, + `SELECT last_id FROM issue_counters WHERE prefix = 'custom'`).Scan(&customCounter) + if err != nil { + t.Fatalf("Failed to query custom counter: %v", err) + } + if customCounter != 7 { + t.Errorf("Expected custom counter to be 7, got %d", customCounter) + } + + // Step 5: Verify next auto-generated IDs are correct + // Set prefix to bd + err = store.SetConfig(ctx, "issue_prefix", "bd") + if err != nil { + t.Fatalf("Failed to set config: %v", err) + } + + issue := &types.Issue{ + Title: "New issue", + Status: types.StatusOpen, + Priority: 2, + IssueType: types.TypeTask, + } + + err = store.CreateIssue(ctx, issue, "test-user") + if err != nil { + t.Fatalf("CreateIssue failed: %v", err) + } + + // Should be bd-16 (after bd-15) + if issue.ID != "bd-16" { + t.Errorf("Expected bd-16, got %s", issue.ID) + } +} + +// TestMigrateIssueCountersTableEmptyDB tests migration on a fresh database +func TestMigrateIssueCountersTableEmptyDB(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "beads-migration-empty-*") + if err != nil { + t.Fatalf("failed to create temp dir: %v", err) + } + defer os.RemoveAll(tmpDir) + + dbPath := filepath.Join(tmpDir, "test.db") + + // Create a fresh database with New() - should create table with no issues + store, err := New(dbPath) + if err != nil { + t.Fatalf("failed to create storage: %v", err) + } + defer store.Close() + + // Verify table exists + var tableName string + err = store.db.QueryRow(` + SELECT name FROM sqlite_master + WHERE type='table' AND name='issue_counters' + `).Scan(&tableName) + if err != nil { + t.Fatalf("Expected issue_counters table to exist: %v", err) + } + + // Verify no counters exist (since no issues) + var count int + err = store.db.QueryRow(`SELECT COUNT(*) FROM issue_counters`).Scan(&count) + if err != nil { + t.Fatalf("Failed to query counters: %v", err) + } + if count != 0 { + t.Errorf("Expected 0 counters in empty DB, got %d", count) + } + + // Create first issue - should work fine + ctx := context.Background() + issue := &types.Issue{ + Title: "First issue", + Status: types.StatusOpen, + Priority: 2, + IssueType: types.TypeTask, + } + + err = store.CreateIssue(ctx, issue, "test-user") + if err != nil { + t.Fatalf("CreateIssue failed: %v", err) + } + + // Should be bd-1 + if issue.ID != "bd-1" { + t.Errorf("Expected bd-1, got %s", issue.ID) + } +} + +// TestMigrateIssueCountersTableIdempotent verifies that running migration +// multiple times is safe and doesn't corrupt data +func TestMigrateIssueCountersTableIdempotent(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "beads-migration-idempotent-*") + if err != nil { + t.Fatalf("failed to create temp dir: %v", err) + } + defer os.RemoveAll(tmpDir) + + dbPath := filepath.Join(tmpDir, "test.db") + + // Create database and migrate + store1, err := New(dbPath) + if err != nil { + t.Fatalf("failed to create storage: %v", err) + } + + // Create some issues + ctx := context.Background() + issue := &types.Issue{ + Title: "Test issue", + Status: types.StatusOpen, + Priority: 2, + IssueType: types.TypeTask, + } + err = store1.CreateIssue(ctx, issue, "test-user") + if err != nil { + t.Fatalf("CreateIssue failed: %v", err) + } + + firstID := issue.ID // Should be bd-1 + store1.Close() + + // Re-open database (triggers migration again) + store2, err := New(dbPath) + if err != nil { + t.Fatalf("failed to re-open storage: %v", err) + } + defer store2.Close() + + // Verify counter is still correct + var bdCounter int + err = store2.db.QueryRowContext(ctx, + `SELECT last_id FROM issue_counters WHERE prefix = 'bd'`).Scan(&bdCounter) + if err != nil { + t.Fatalf("Failed to query bd counter: %v", err) + } + if bdCounter != 1 { + t.Errorf("Expected bd counter to be 1 after idempotent migration, got %d", bdCounter) + } + + // Create another issue + issue2 := &types.Issue{ + Title: "Second issue", + Status: types.StatusOpen, + Priority: 2, + IssueType: types.TypeTask, + } + err = store2.CreateIssue(ctx, issue2, "test-user") + if err != nil { + t.Fatalf("CreateIssue failed: %v", err) + } + + // Should be bd-2 (not bd-1 again) + if issue2.ID != "bd-2" { + t.Errorf("Expected bd-2, got %s", issue2.ID) + } + + // Verify first issue still exists + firstIssue, err := store2.GetIssue(ctx, firstID) + if err != nil { + t.Fatalf("Failed to get first issue: %v", err) + } + if firstIssue == nil { + t.Errorf("First issue was lost after re-opening database") + } +} diff --git a/internal/storage/sqlite/sqlite.go b/internal/storage/sqlite/sqlite.go index a964a09b..637928c3 100644 --- a/internal/storage/sqlite/sqlite.go +++ b/internal/storage/sqlite/sqlite.go @@ -50,6 +50,11 @@ func New(path string) (*SQLiteStorage, error) { return nil, fmt.Errorf("failed to migrate dirty_issues table: %w", err) } + // Migrate existing databases to add issue_counters table if missing + if err := migrateIssueCountersTable(db); err != nil { + return nil, fmt.Errorf("failed to migrate issue_counters table: %w", err) + } + return &SQLiteStorage{ db: db, }, nil @@ -90,6 +95,66 @@ func migrateDirtyIssuesTable(db *sql.DB) error { return nil } +// migrateIssueCountersTable checks if the issue_counters table needs initialization. +// This ensures existing databases created before the atomic counter feature get migrated automatically. +// The table may already exist (created by schema), but be empty - in that case we still need to sync. +func migrateIssueCountersTable(db *sql.DB) error { + // Check if the table exists (it should, created by schema) + var tableName string + err := db.QueryRow(` + SELECT name FROM sqlite_master + WHERE type='table' AND name='issue_counters' + `).Scan(&tableName) + + tableExists := err == nil + + if !tableExists { + if err != sql.ErrNoRows { + return fmt.Errorf("failed to check for issue_counters table: %w", err) + } + // Table doesn't exist, create it (shouldn't happen with schema, but handle it) + _, err := db.Exec(` + CREATE TABLE issue_counters ( + prefix TEXT PRIMARY KEY, + last_id INTEGER NOT NULL DEFAULT 0 + ) + `) + if err != nil { + return fmt.Errorf("failed to create issue_counters table: %w", err) + } + } + + // Check if table is empty - if so, we need to sync from existing issues + var count int + err = db.QueryRow(`SELECT COUNT(*) FROM issue_counters`).Scan(&count) + if err != nil { + return fmt.Errorf("failed to count issue_counters: %w", err) + } + + if count == 0 { + // Table is empty, sync counters from existing issues to prevent ID collisions + // This is safe to do during migration since it's a one-time operation + _, err = db.Exec(` + INSERT INTO issue_counters (prefix, last_id) + SELECT + substr(id, 1, instr(id, '-') - 1) as prefix, + MAX(CAST(substr(id, instr(id, '-') + 1) AS INTEGER)) as max_id + FROM issues + WHERE instr(id, '-') > 0 + AND substr(id, instr(id, '-') + 1) GLOB '[0-9]*' + GROUP BY prefix + ON CONFLICT(prefix) DO UPDATE SET + last_id = MAX(last_id, excluded.last_id) + `) + if err != nil { + return fmt.Errorf("failed to sync counters during migration: %w", err) + } + } + + // Table exists and is initialized (either was already populated, or we just synced it) + return nil +} + // getNextIDForPrefix atomically generates the next ID for a given prefix // Uses the issue_counters table for atomic, cross-process ID generation func (s *SQLiteStorage) getNextIDForPrefix(ctx context.Context, prefix string) (int, error) { @@ -107,6 +172,43 @@ func (s *SQLiteStorage) getNextIDForPrefix(ctx context.Context, prefix string) ( return nextID, nil } +// ensureCounterInitialized checks if a counter exists for the given prefix, +// and initializes it from existing issues if needed. This is lazy initialization +// to avoid scanning the entire issues table on every CreateIssue call. +func (s *SQLiteStorage) ensureCounterInitialized(ctx context.Context, prefix string) error { + // Check if counter already exists for this prefix + var exists int + err := s.db.QueryRowContext(ctx, + `SELECT 1 FROM issue_counters WHERE prefix = ?`, prefix).Scan(&exists) + + if err == nil { + // Counter exists, we're good + return nil + } + + if err != sql.ErrNoRows { + // Unexpected error + return fmt.Errorf("failed to check counter existence: %w", err) + } + + // Counter doesn't exist, initialize it from existing issues with this prefix + _, err = s.db.ExecContext(ctx, ` + INSERT INTO issue_counters (prefix, last_id) + SELECT ?, COALESCE(MAX(CAST(substr(id, LENGTH(?) + 2) AS INTEGER)), 0) + FROM issues + WHERE id LIKE ? || '-%' + AND substr(id, LENGTH(?) + 2) GLOB '[0-9]*' + ON CONFLICT(prefix) DO UPDATE SET + last_id = MAX(last_id, excluded.last_id) + `, prefix, prefix, prefix, prefix) + + if err != nil { + return fmt.Errorf("failed to initialize counter for prefix %s: %w", prefix, err) + } + + return nil +} + // SyncAllCounters synchronizes all ID counters based on existing issues in the database // This scans all issues and updates counters to prevent ID collisions with auto-generated IDs func (s *SQLiteStorage) SyncAllCounters(ctx context.Context) error { @@ -137,19 +239,18 @@ func (s *SQLiteStorage) CreateIssue(ctx context.Context, issue *types.Issue, act // Generate ID if not set (using atomic counter table) if issue.ID == "" { - // Sync all counters first to ensure we don't collide with existing issues - // This handles the case where the database was created before this fix - // or issues were imported without syncing counters - if err := s.SyncAllCounters(ctx); err != nil { - return fmt.Errorf("failed to sync counters: %w", err) - } - // Get prefix from config, default to "bd" prefix, err := s.GetConfig(ctx, "issue_prefix") if err != nil || prefix == "" { prefix = "bd" } + // Ensure counter is initialized for this prefix (lazy initialization) + // Only scans issues with this prefix on first use, not the entire table + if err := s.ensureCounterInitialized(ctx, prefix); err != nil { + return fmt.Errorf("failed to initialize counter: %w", err) + } + // Atomically get next ID from counter table nextID, err := s.getNextIDForPrefix(ctx, prefix) if err != nil { diff --git a/internal/storage/sqlite/sqlite_test.go b/internal/storage/sqlite/sqlite_test.go index 4805527f..38cf2a08 100644 --- a/internal/storage/sqlite/sqlite_test.go +++ b/internal/storage/sqlite/sqlite_test.go @@ -460,9 +460,9 @@ func TestMultiProcessIDGeneration(t *testing.T) { ids[res.id] = true } - // With the bug, we expect UNIQUE constraint errors + // After the fix (atomic counter), all operations should succeed without errors if len(errors) > 0 { - t.Logf("Got %d errors (expected with current implementation):", len(errors)) + t.Errorf("Expected no errors with atomic counter fix, got %d:", len(errors)) for _, err := range errors { t.Logf(" - %v", err) } @@ -470,13 +470,9 @@ func TestMultiProcessIDGeneration(t *testing.T) { t.Logf("Successfully created %d unique issues out of %d attempts", len(ids), numProcesses) - // After the fix, all should succeed + // All issues should be created successfully with unique IDs if len(ids) != numProcesses { t.Errorf("Expected %d unique IDs, got %d", numProcesses, len(ids)) } - - if len(errors) > 0 { - t.Errorf("Expected no errors, got %d", len(errors)) - } }