From e3afecca372eac7fd67acfa8629ad06265e857dc Mon Sep 17 00:00:00 2001 From: Steve Yegge Date: Thu, 30 Oct 2025 21:51:39 -0700 Subject: [PATCH] Remove sequential ID code path (bd-aa744b) - Removed nextSequentialID() and getIDMode() functions - Removed issue_counters table from schema - Made SyncAllCounters() a no-op for backward compatibility - Simplified ID generation to hash-only (adaptive length) - Removed id_mode config setting - Removed sequential ID tests and migration code - Updated CONFIG.md and AGENTS.md to remove sequential ID references Follow-up bd-2a70 will remove obsolete test files and renumber command. --- AGENTS.md | 2 - CONFIG.md | 1 - beads_nway_test.go | 2 - beads_twoclone_test.go | 2 - cmd/bd/migrate.go | 16 - cmd/bd/renumber_test.go | 268 --------------- internal/storage/memory/memory.go | 20 +- internal/storage/memory/memory_test.go | 34 -- internal/storage/sqlite/adaptive_e2e_test.go | 8 +- .../storage/sqlite/adaptive_length_test.go | 5 - .../storage/sqlite/counter_import_test.go | 160 --------- internal/storage/sqlite/counter_sync_test.go | 232 ------------- internal/storage/sqlite/hash_id_test.go | 10 +- internal/storage/sqlite/lazy_init_test.go | 229 ------------- internal/storage/sqlite/migration_test.go | 299 ---------------- internal/storage/sqlite/schema.go | 6 - internal/storage/sqlite/sqlite.go | 318 +++++------------- 17 files changed, 85 insertions(+), 1527 deletions(-) delete mode 100644 cmd/bd/renumber_test.go delete mode 100644 internal/storage/sqlite/counter_import_test.go delete mode 100644 internal/storage/sqlite/counter_sync_test.go delete mode 100644 internal/storage/sqlite/lazy_init_test.go delete mode 100644 internal/storage/sqlite/migration_test.go diff --git a/AGENTS.md b/AGENTS.md index d05a9713..287c2db9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -210,8 +210,6 @@ bd merge bd-42 bd-43 --into bd-41 --dry-run # Preview merge bd migrate # Detect and migrate old databases bd migrate --dry-run # Preview migration bd migrate --cleanup --yes # Migrate and remove old files -bd migrate --to-hash-ids # Migrate sequential IDs to hash-based IDs -bd migrate --to-hash-ids --dry-run # Preview hash ID migration ``` ### Managing Daemons diff --git a/CONFIG.md b/CONFIG.md index 4cfc9e6e..60526eef 100644 --- a/CONFIG.md +++ b/CONFIG.md @@ -166,7 +166,6 @@ Configuration keys use dot-notation namespaces to organize settings: - `compact_*` - Compaction settings (see EXTENDING.md) - `issue_prefix` - Issue ID prefix (managed by `bd init`) -- `id_mode` - ID generation mode: `sequential` or `hash` (managed by `bd init`) - `max_collision_prob` - Maximum collision probability for adaptive hash IDs (default: 0.25) - `min_hash_length` - Minimum hash ID length (default: 4) - `max_hash_length` - Maximum hash ID length (default: 8) diff --git a/beads_nway_test.go b/beads_nway_test.go index 938b32d2..31cd55f4 100644 --- a/beads_nway_test.go +++ b/beads_nway_test.go @@ -138,7 +138,6 @@ func setupClone(t *testing.T, tmpDir, remoteDir, name, bdPath string) string { t.Logf("Initializing beads in clone %s", name) runCmd(t, cloneDir, "./bd", "init", "--quiet", "--prefix", "test") // Enable hash ID mode for collision-free IDs - runCmdWithEnv(t, cloneDir, map[string]string{"BEADS_NO_DAEMON": "1"}, "./bd", "config", "set", "id_mode", "hash") runCmd(t, cloneDir, "git", "add", ".beads") runCmd(t, cloneDir, "git", "commit", "-m", "Initialize beads") runCmd(t, cloneDir, "git", "push", "origin", "master") @@ -147,7 +146,6 @@ func setupClone(t *testing.T, tmpDir, remoteDir, name, bdPath string) string { runCmd(t, cloneDir, "git", "pull", "origin", "master") runCmd(t, cloneDir, "./bd", "init", "--quiet", "--prefix", "test") // Enable hash ID mode (same as clone A) - runCmdWithEnv(t, cloneDir, map[string]string{"BEADS_NO_DAEMON": "1"}, "./bd", "config", "set", "id_mode", "hash") } // Install git hooks diff --git a/beads_twoclone_test.go b/beads_twoclone_test.go index 5572ab0c..0578cad7 100644 --- a/beads_twoclone_test.go +++ b/beads_twoclone_test.go @@ -48,7 +48,6 @@ func TestTwoCloneCollision(t *testing.T) { t.Log("Initializing beads in clone A") runCmd(t, cloneA, "./bd", "init", "--quiet", "--prefix", "test") // Enable hash ID mode for collision-free IDs - runCmdWithEnv(t, cloneA, map[string]string{"BEADS_NO_DAEMON": "1"}, "./bd", "config", "set", "id_mode", "hash") // Configure git to use merge instead of rebase (sorted JSONL merges cleanly) runCmd(t, cloneA, "git", "config", "pull.rebase", "false") @@ -65,7 +64,6 @@ func TestTwoCloneCollision(t *testing.T) { t.Log("Initializing database in clone B") runCmd(t, cloneB, "./bd", "init", "--quiet", "--prefix", "test") // Enable hash ID mode (same as clone A) - runCmdWithEnv(t, cloneB, map[string]string{"BEADS_NO_DAEMON": "1"}, "./bd", "config", "set", "id_mode", "hash") // Configure git to use merge instead of rebase (sorted JSONL merges cleanly) runCmd(t, cloneB, "git", "config", "pull.rebase", "false") diff --git a/cmd/bd/migrate.go b/cmd/bd/migrate.go index 31284bb1..f581629d 100644 --- a/cmd/bd/migrate.go +++ b/cmd/bd/migrate.go @@ -357,22 +357,6 @@ This command: color.Green("✓ Migrated %d issues to hash-based IDs\n", len(mapping)) } } - - // Set id_mode=hash after successful migration (not in dry-run) - if !dryRun { - store, err := sqlite.New(targetPath) - if err == nil { - ctx := context.Background() - if err := store.SetConfig(ctx, "id_mode", "hash"); err != nil { - if !jsonOutput { - fmt.Fprintf(os.Stderr, "Warning: failed to set id_mode=hash: %v\n", err) - } - } else if !jsonOutput { - color.Green("✓ Switched database to hash ID mode\n") - } - store.Close() - } - } } else { store.Close() if !jsonOutput { diff --git a/cmd/bd/renumber_test.go b/cmd/bd/renumber_test.go deleted file mode 100644 index 11f2b97f..00000000 --- a/cmd/bd/renumber_test.go +++ /dev/null @@ -1,268 +0,0 @@ -package main - -import ( - "context" - "os" - "path/filepath" - "testing" - - "github.com/steveyegge/beads/internal/storage/sqlite" - "github.com/steveyegge/beads/internal/types" -) - -func TestRenumberWithGaps(t *testing.T) { - // Create a temporary directory for the test database - tmpDir := t.TempDir() - dbPath := filepath.Join(tmpDir, "test.db") - - // Initialize store - testStore, err := sqlite.New(dbPath) - if err != nil { - t.Fatalf("failed to create store: %v", err) - } - defer testStore.Close() - - ctx := context.Background() - - // Set up config - if err := testStore.SetConfig(ctx, "issue_prefix", "bd"); err != nil { - t.Fatalf("failed to set prefix: %v", err) - } - - // Create issues with large gaps in numbering (simulating compacted database) - // This reproduces the scenario from bd-345: 107 issues with IDs 1-344 - testIssues := []struct { - id string - title string - }{ - {"bd-1", "Issue 1"}, - {"bd-4", "Issue 4"}, // Gap here (2, 3 missing) - {"bd-100", "Issue 100"}, // Large gap - {"bd-200", "Issue 200"}, // Another large gap - {"bd-344", "Issue 344"}, // Final issue - } - - for _, tc := range testIssues { - issue := &types.Issue{ - ID: tc.id, // Set explicit ID to simulate gaps - Title: tc.title, - Description: "Test issue for renumbering", - Priority: 1, - IssueType: "task", - Status: "open", - } - if err := testStore.CreateIssue(ctx, issue, "test"); err != nil { - t.Fatalf("failed to create issue: %v", err) - } - } - - // Add a dependency to test that it gets updated - dep := &types.Dependency{ - IssueID: "bd-4", - DependsOnID: "bd-1", - Type: "blocks", - } - if err := testStore.AddDependency(ctx, dep, "test"); err != nil { - t.Fatalf("failed to add dependency: %v", err) - } - - // Get all issues before renumbering - issuesBefore, err := testStore.SearchIssues(ctx, "", types.IssueFilter{}) - if err != nil { - t.Fatalf("failed to list issues before: %v", err) - } - if len(issuesBefore) != 5 { - t.Fatalf("expected 5 issues, got %d", len(issuesBefore)) - } - - // Build the ID mapping (what renumber would create) - idMapping := map[string]string{ - "bd-1": "bd-1", - "bd-4": "bd-2", - "bd-100": "bd-3", - "bd-200": "bd-4", - "bd-344": "bd-5", - } - - // Temporarily set the global store for renumberIssuesInDB - oldStore := store - store = testStore - defer func() { store = oldStore }() - - // Run the renumbering - if err := renumberIssuesInDB(ctx, "bd", idMapping, issuesBefore); err != nil { - t.Fatalf("renumberIssuesInDB failed: %v", err) - } - - // Verify all issues were renumbered correctly - issuesAfter, err := testStore.SearchIssues(ctx, "", types.IssueFilter{}) - if err != nil { - t.Fatalf("failed to list issues after: %v", err) - } - - if len(issuesAfter) != 5 { - t.Fatalf("expected 5 issues after renumbering, got %d", len(issuesAfter)) - } - - // Check that new IDs are sequential 1-5 - expectedIDs := map[string]bool{ - "bd-1": true, - "bd-2": true, - "bd-3": true, - "bd-4": true, - "bd-5": true, - } - - for _, issue := range issuesAfter { - if !expectedIDs[issue.ID] { - t.Errorf("unexpected issue ID after renumbering: %s", issue.ID) - } - delete(expectedIDs, issue.ID) - } - - if len(expectedIDs) > 0 { - t.Errorf("missing expected IDs after renumbering: %v", expectedIDs) - } - - // Verify dependency was updated using GetAllDependencyRecords - finalDeps, err := testStore.GetAllDependencyRecords(ctx) - if err != nil { - t.Fatalf("failed to get dependencies: %v", err) - } - - foundDep := false - if deps, ok := finalDeps["bd-2"]; ok { - for _, dep := range deps { - if dep.DependsOnID == "bd-1" && dep.Type == "blocks" { - foundDep = true - break - } - } - } - - if !foundDep { - t.Errorf("dependency not updated correctly: expected bd-2 -> bd-1") - } -} - -func TestRenumberWithTextReferences(t *testing.T) { - tmpDir := t.TempDir() - dbPath := filepath.Join(tmpDir, "test.db") - - testStore, err := sqlite.New(dbPath) - if err != nil { - t.Fatalf("failed to create store: %v", err) - } - defer testStore.Close() - - ctx := context.Background() - - if err := testStore.SetConfig(ctx, "issue_prefix", "bd"); err != nil { - t.Fatalf("failed to set prefix: %v", err) - } - - // Create issues with text references to each other - issue1 := &types.Issue{ - Title: "First issue", - Description: "See bd-2 for details", - Priority: 1, - IssueType: "task", - Status: "open", - } - if err := testStore.CreateIssue(ctx, issue1, "test"); err != nil { - t.Fatalf("failed to create issue: %v", err) - } - // Manually set ID to bd-1 - oldID1 := issue1.ID - if err := testStore.UpdateIssueID(ctx, oldID1, "bd-1", issue1, "test"); err != nil { - t.Fatalf("failed to set issue ID: %v", err) - } - - issue2 := &types.Issue{ - Title: "Second issue", - Description: "Blocks bd-1", - Notes: "Also see bd-1 and bd-100", - Priority: 1, - IssueType: "task", - Status: "open", - } - if err := testStore.CreateIssue(ctx, issue2, "test"); err != nil { - t.Fatalf("failed to create issue: %v", err) - } - // Manually set ID to bd-100 - oldID2 := issue2.ID - if err := testStore.UpdateIssueID(ctx, oldID2, "bd-100", issue2, "test"); err != nil { - t.Fatalf("failed to set issue ID: %v", err) - } - - // Renumber: bd-1 -> bd-1, bd-100 -> bd-2 - issues, _ := testStore.SearchIssues(ctx, "", types.IssueFilter{}) - idMapping := map[string]string{ - "bd-1": "bd-1", - "bd-100": "bd-2", - } - - oldStore := store - store = testStore - defer func() { store = oldStore }() - - if err := renumberIssuesInDB(ctx, "bd", idMapping, issues); err != nil { - t.Fatalf("renumberIssuesInDB failed: %v", err) - } - - // Verify text references were updated - updated1, err := testStore.GetIssue(ctx, "bd-1") - if err != nil { - t.Fatalf("failed to get bd-1: %v", err) - } - if updated1.Description != "See bd-2 for details" { - t.Errorf("bd-1 description not updated: got %q, want %q", - updated1.Description, "See bd-2 for details") - } - - updated2, err := testStore.GetIssue(ctx, "bd-2") - if err != nil { - t.Fatalf("failed to get bd-2: %v", err) - } - if updated2.Description != "Blocks bd-1" { - t.Errorf("bd-2 description not updated: got %q, want %q", - updated2.Description, "Blocks bd-1") - } - if updated2.Notes != "Also see bd-1 and bd-2" { - t.Errorf("bd-2 notes not updated: got %q, want %q", - updated2.Notes, "Also see bd-1 and bd-2") - } -} - -func TestRenumberEmptyDatabase(t *testing.T) { - tmpDir := t.TempDir() - dbPath := filepath.Join(tmpDir, "test.db") - - testStore, err := sqlite.New(dbPath) - if err != nil { - t.Fatalf("failed to create store: %v", err) - } - defer testStore.Close() - - ctx := context.Background() - - if err := testStore.SetConfig(ctx, "issue_prefix", "bd"); err != nil { - t.Fatalf("failed to set prefix: %v", err) - } - - oldStore := store - store = testStore - defer func() { store = oldStore }() - - // Renumber should succeed with no issues - issues, _ := testStore.SearchIssues(ctx, "", types.IssueFilter{}) - if err := renumberIssuesInDB(ctx, "bd", map[string]string{}, issues); err != nil { - t.Fatalf("renumberIssuesInDB failed on empty database: %v", err) - } -} - -// Cleanup any test databases -func TestMain(m *testing.M) { - code := m.Run() - os.Exit(code) -} diff --git a/internal/storage/memory/memory.go b/internal/storage/memory/memory.go index 5267444c..6f3f4a85 100644 --- a/internal/storage/memory/memory.go +++ b/internal/storage/memory/memory.go @@ -942,24 +942,10 @@ func (m *MemoryStorage) UnderlyingConn(ctx context.Context) (*sql.Conn, error) { return nil, fmt.Errorf("UnderlyingConn not available in memory storage") } -// SyncAllCounters synchronizes ID counters based on existing issues +// SyncAllCounters is a no-op now that sequential IDs are removed (bd-aa744b). +// Kept for backward compatibility with existing code that calls it. func (m *MemoryStorage) SyncAllCounters(ctx context.Context) error { - m.mu.Lock() - defer m.mu.Unlock() - - // Reset counters - m.counters = make(map[string]int) - - // Recompute from issues - for _, issue := range m.issues { - prefix, num := extractPrefixAndNumber(issue.ID) - if prefix != "" && num > 0 { - if m.counters[prefix] < num { - m.counters[prefix] = num - } - } - } - + // No-op: hash IDs don't use counters return nil } diff --git a/internal/storage/memory/memory_test.go b/internal/storage/memory/memory_test.go index d5b2c454..94d564cd 100644 --- a/internal/storage/memory/memory_test.go +++ b/internal/storage/memory/memory_test.go @@ -825,40 +825,6 @@ func TestMetadataOperations(t *testing.T) { } } -func TestSyncAllCounters(t *testing.T) { - store := New("") - defer store.Close() - - ctx := context.Background() - - // Load issues with different prefixes - issues := []*types.Issue{ - {ID: "bd-5", Title: "Test 1", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeTask}, - {ID: "bd-10", Title: "Test 2", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeTask}, - {ID: "custom-3", Title: "Test 3", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeTask}, - } - - if err := store.LoadFromIssues(issues); err != nil { - t.Fatalf("LoadFromIssues failed: %v", err) - } - - // Manually corrupt counter - store.counters["bd"] = 1 - - // Sync counters - if err := store.SyncAllCounters(ctx); err != nil { - t.Fatalf("SyncAllCounters failed: %v", err) - } - - // Verify corrected - if store.counters["bd"] != 10 { - t.Errorf("Expected bd counter to be 10, got %d", store.counters["bd"]) - } - - if store.counters["custom"] != 3 { - t.Errorf("Expected custom counter to be 3, got %d", store.counters["custom"]) - } -} func TestThreadSafety(t *testing.T) { store := setupTestMemory(t) diff --git a/internal/storage/sqlite/adaptive_e2e_test.go b/internal/storage/sqlite/adaptive_e2e_test.go index 703a09c8..40805b7d 100644 --- a/internal/storage/sqlite/adaptive_e2e_test.go +++ b/internal/storage/sqlite/adaptive_e2e_test.go @@ -18,13 +18,10 @@ func TestAdaptiveIDLength_E2E(t *testing.T) { ctx := context.Background() - // Initialize with prefix and hash mode + // Initialize with prefix if err := db.SetConfig(ctx, "issue_prefix", "test"); err != nil { t.Fatalf("Failed to set prefix: %v", err) } - if err := db.SetConfig(ctx, "id_mode", "hash"); err != nil { - t.Fatalf("Failed to set id_mode: %v", err) - } // Helper to create issue and verify ID length createAndCheckLength := func(title string, expectedHashLen int) string { @@ -124,9 +121,6 @@ func TestAdaptiveIDLength_CustomConfig(t *testing.T) { if err := db.SetConfig(ctx, "issue_prefix", "test"); err != nil { t.Fatalf("Failed to set prefix: %v", err) } - if err := db.SetConfig(ctx, "id_mode", "hash"); err != nil { - t.Fatalf("Failed to set id_mode: %v", err) - } // Set stricter collision threshold (1%) and min length of 5 if err := db.SetConfig(ctx, "max_collision_prob", "0.01"); err != nil { diff --git a/internal/storage/sqlite/adaptive_length_test.go b/internal/storage/sqlite/adaptive_length_test.go index e9f4639c..fa7a4d71 100644 --- a/internal/storage/sqlite/adaptive_length_test.go +++ b/internal/storage/sqlite/adaptive_length_test.go @@ -152,11 +152,6 @@ func TestGetAdaptiveIDLength_Integration(t *testing.T) { t.Fatalf("Failed to set prefix: %v", err) } - // Set id_mode to hash - if err := db.SetConfig(ctx, "id_mode", "hash"); err != nil { - t.Fatalf("Failed to set id_mode: %v", err) - } - // Test default config (should use 4 chars for empty database) conn, err := db.db.Conn(ctx) if err != nil { diff --git a/internal/storage/sqlite/counter_import_test.go b/internal/storage/sqlite/counter_import_test.go deleted file mode 100644 index 306fc460..00000000 --- a/internal/storage/sqlite/counter_import_test.go +++ /dev/null @@ -1,160 +0,0 @@ -package sqlite - -import ( - "context" - "fmt" - "testing" - - "github.com/steveyegge/beads/internal/types" -) - -// TestCounterSyncAfterImport verifies that counters are properly synced after import -// This test reproduces the scenario from bd-50: -// - Start with a database that has stale counter (e.g., 4106) -// - Import issues with lower IDs (e.g., bd-1 to bd-49) -// - Verify counter syncs to actual max ID (49), not stuck at 4106 -func TestCounterSyncAfterImport(t *testing.T) { - store, cleanup := setupTestDB(t) - defer cleanup() - - ctx := context.Background() - - // Set the issue prefix to "bd" for this test - if err := store.SetConfig(ctx, "issue_prefix", "bd"); err != nil { - t.Fatalf("Failed to set issue_prefix: %v", err) - } - - // Simulate "test pollution" scenario: manually set counter to high value - // This simulates having had many issues before that were deleted - _, err := store.db.ExecContext(ctx, ` - INSERT INTO issue_counters (prefix, last_id) - VALUES ('bd', 4106) - ON CONFLICT(prefix) DO UPDATE SET last_id = 4106 - `) - if err != nil { - t.Fatalf("Failed to set stale counter: %v", err) - } - - // Verify counter is at 4106 - var counter int - err = store.db.QueryRow(`SELECT last_id FROM issue_counters WHERE prefix = 'bd'`).Scan(&counter) - if err != nil { - t.Fatalf("Failed to query counter: %v", err) - } - if counter != 4106 { - t.Errorf("Expected counter at 4106, got %d", counter) - } - - // Now import 49 issues (bd-1 through bd-49) - for i := 1; i <= 49; i++ { - issue := &types.Issue{ - ID: genID("bd", i), - Title: fmt.Sprintf("Imported issue %d", i), - Priority: 2, - IssueType: types.TypeTask, - Status: types.StatusOpen, - } - // Use CreateIssue which will be called during import - if err := store.CreateIssue(ctx, issue, "import"); err != nil { - t.Fatalf("Failed to import issue %d: %v", i, err) - } - } - - // After import, manually call SyncAllCounters (this is what import_shared.go does) - if err := store.SyncAllCounters(ctx); err != nil { - t.Fatalf("Failed to sync counters: %v", err) - } - - // Counter should now be synced to 49 (the actual max ID) - err = store.db.QueryRow(`SELECT last_id FROM issue_counters WHERE prefix = 'bd'`).Scan(&counter) - if err != nil { - t.Fatalf("Failed to query counter after import: %v", err) - } - if counter != 49 { - t.Errorf("Expected counter at 49 after import+sync, got %d (bug from bd-50!)", counter) - } - - // Create new issue with auto-generated ID - should be bd-50, not bd-4107 - newIssue := &types.Issue{ - Title: "New issue after import", - Priority: 2, - IssueType: types.TypeTask, - Status: types.StatusOpen, - } - if err := store.CreateIssue(ctx, newIssue, "test"); err != nil { - t.Fatalf("Failed to create new issue: %v", err) - } - if newIssue.ID != "bd-50" { - t.Errorf("Expected new issue to be bd-50, got %s (bug from bd-50!)", newIssue.ID) - } -} - -// TestCounterSyncWithoutExplicitSync verifies behavior when SyncAllCounters is NOT called -// This shows the bug that would happen if import didn't call SyncAllCounters -func TestCounterNotSyncedWithoutExplicitSync(t *testing.T) { - store, cleanup := setupTestDB(t) - defer cleanup() - - ctx := context.Background() - - // Set the issue prefix to "bd" for this test - if err := store.SetConfig(ctx, "issue_prefix", "bd"); err != nil { - t.Fatalf("Failed to set issue_prefix: %v", err) - } - - // Manually set counter to high value (stale counter) - _, err := store.db.ExecContext(ctx, ` - INSERT INTO issue_counters (prefix, last_id) - VALUES ('bd', 4106) - ON CONFLICT(prefix) DO UPDATE SET last_id = 4106 - `) - if err != nil { - t.Fatalf("Failed to set stale counter: %v", err) - } - - // Import 49 issues WITHOUT calling SyncAllCounters - for i := 1; i <= 49; i++ { - issue := &types.Issue{ - ID: genID("bd", i), - Title: fmt.Sprintf("Imported issue %d", i), - Priority: 2, - IssueType: types.TypeTask, - Status: types.StatusOpen, - } - if err := store.CreateIssue(ctx, issue, "import"); err != nil { - t.Fatalf("Failed to import issue %d: %v", i, err) - } - } - - // DO NOT call SyncAllCounters - simulate the bug - - // Counter should still be at 4106 (stale!) - var counter int - err = store.db.QueryRow(`SELECT last_id FROM issue_counters WHERE prefix = 'bd'`).Scan(&counter) - if err != nil { - t.Fatalf("Failed to query counter: %v", err) - } - if counter != 4106 { - t.Logf("Counter was at %d instead of 4106 - counter got updated somehow", counter) - } - - // Create new issue - without the sync fix, this would be bd-4107 - newIssue := &types.Issue{ - Title: "New issue after import (no sync)", - Priority: 2, - IssueType: types.TypeTask, - Status: types.StatusOpen, - } - if err := store.CreateIssue(ctx, newIssue, "test"); err != nil { - t.Fatalf("Failed to create new issue: %v", err) - } - - // Without SyncAllCounters, this would be bd-4107 (the bug!) - // With the fix in import_shared.go, it should be bd-50 - t.Logf("New issue ID: %s (expected bd-4107 without fix, bd-50 with fix)", newIssue.ID) - - // This test documents the bug behavior - if counter is stale, next ID is wrong - if newIssue.ID == "bd-4107" { - t.Logf("Bug confirmed: counter not synced, got wrong ID %s", newIssue.ID) - } -} diff --git a/internal/storage/sqlite/counter_sync_test.go b/internal/storage/sqlite/counter_sync_test.go deleted file mode 100644 index 758909c1..00000000 --- a/internal/storage/sqlite/counter_sync_test.go +++ /dev/null @@ -1,232 +0,0 @@ -package sqlite - -import ( - "context" - "fmt" - "testing" - - "github.com/steveyegge/beads/internal/types" -) - -// TestCounterSyncAfterDelete verifies that counters are synced after deletion (bd-49) -func TestCounterSyncAfterDelete(t *testing.T) { - store, cleanup := setupTestDB(t) - defer cleanup() - - ctx := context.Background() - - // Set the issue prefix to "bd" for this test - if err := store.SetConfig(ctx, "issue_prefix", "bd"); err != nil { - t.Fatalf("Failed to set issue_prefix: %v", err) - } - - // Create issues bd-1 through bd-5 - for i := 1; i <= 5; i++ { - issue := &types.Issue{ - ID: genID("bd", i), - Title: "Test issue", - Priority: 2, - IssueType: types.TypeTask, - Status: types.StatusOpen, - } - if err := store.CreateIssue(ctx, issue, "test"); err != nil { - t.Fatalf("Failed to create issue: %v", err) - } - } - - // Create one issue with auto-generated ID to initialize counter - autoIssue := &types.Issue{ - Title: "Auto issue", - Priority: 2, - IssueType: types.TypeTask, - Status: types.StatusOpen, - } - if err := store.CreateIssue(ctx, autoIssue, "test"); err != nil { - t.Fatalf("Failed to create auto issue: %v", err) - } - if autoIssue.ID != "bd-6" { - t.Fatalf("Expected auto issue to be bd-6, got %s", autoIssue.ID) - } - - // Verify counter is at 6 - var counter int - err := store.db.QueryRow(`SELECT last_id FROM issue_counters WHERE prefix = 'bd'`).Scan(&counter) - if err != nil { - t.Fatalf("Failed to query counter: %v", err) - } - if counter != 6 { - t.Errorf("Expected counter at 6, got %d", counter) - } - - // Delete bd-5 and bd-6 - if err := store.DeleteIssue(ctx, "bd-5"); err != nil { - t.Fatalf("Failed to delete bd-5: %v", err) - } - if err := store.DeleteIssue(ctx, "bd-6"); err != nil { - t.Fatalf("Failed to delete bd-6: %v", err) - } - - // Counter should now be synced to 4 (max remaining ID) - err = store.db.QueryRow(`SELECT last_id FROM issue_counters WHERE prefix = 'bd'`).Scan(&counter) - if err != nil { - t.Fatalf("Failed to query counter after delete: %v", err) - } - if counter != 4 { - t.Errorf("Expected counter at 4 after deletion, got %d", counter) - } - - // Create new issue - should be bd-5 (not bd-7) - newIssue := &types.Issue{ - Title: "New issue after deletion", - Priority: 2, - IssueType: types.TypeTask, - Status: types.StatusOpen, - } - if err := store.CreateIssue(ctx, newIssue, "test"); err != nil { - t.Fatalf("Failed to create new issue: %v", err) - } - if newIssue.ID != "bd-5" { - t.Errorf("Expected new issue to be bd-5, got %s", newIssue.ID) - } -} - -// TestCounterSyncAfterBatchDelete verifies that counters are synced after batch deletion (bd-49) -func TestCounterSyncAfterBatchDelete(t *testing.T) { - store, cleanup := setupTestDB(t) - defer cleanup() - - ctx := context.Background() - - // Set the issue prefix to "bd" for this test - if err := store.SetConfig(ctx, "issue_prefix", "bd"); err != nil { - t.Fatalf("Failed to set issue_prefix: %v", err) - } - - // Create issues bd-1 through bd-10 - for i := 1; i <= 10; i++ { - issue := &types.Issue{ - ID: genID("bd", i), - Title: "Test issue", - Priority: 2, - IssueType: types.TypeTask, - Status: types.StatusOpen, - } - if err := store.CreateIssue(ctx, issue, "test"); err != nil { - t.Fatalf("Failed to create issue: %v", err) - } - } - - // Create one issue with auto-generated ID to initialize counter - autoIssue := &types.Issue{ - Title: "Auto issue", - Priority: 2, - IssueType: types.TypeTask, - Status: types.StatusOpen, - } - if err := store.CreateIssue(ctx, autoIssue, "test"); err != nil { - t.Fatalf("Failed to create auto issue: %v", err) - } - if autoIssue.ID != "bd-11" { - t.Fatalf("Expected auto issue to be bd-11, got %s", autoIssue.ID) - } - - // Verify counter is at 11 - var counter int - err := store.db.QueryRow(`SELECT last_id FROM issue_counters WHERE prefix = 'bd'`).Scan(&counter) - if err != nil { - t.Fatalf("Failed to query counter: %v", err) - } - if counter != 11 { - t.Errorf("Expected counter at 11, got %d", counter) - } - - // Batch delete bd-6 through bd-11 - toDelete := []string{"bd-6", "bd-7", "bd-8", "bd-9", "bd-10", "bd-11"} - _, err = store.DeleteIssues(ctx, toDelete, false, false, false) - if err != nil { - t.Fatalf("Failed to batch delete: %v", err) - } - - // Counter should now be synced to 5 (max remaining ID) - err = store.db.QueryRow(`SELECT last_id FROM issue_counters WHERE prefix = 'bd'`).Scan(&counter) - if err != nil { - t.Fatalf("Failed to query counter after batch delete: %v", err) - } - if counter != 5 { - t.Errorf("Expected counter at 5 after batch deletion, got %d", counter) - } - - // Create new issue - should be bd-6 (not bd-11) - newIssue := &types.Issue{ - Title: "New issue after batch deletion", - Priority: 2, - IssueType: types.TypeTask, - Status: types.StatusOpen, - } - if err := store.CreateIssue(ctx, newIssue, "test"); err != nil { - t.Fatalf("Failed to create new issue: %v", err) - } - if newIssue.ID != "bd-6" { - t.Errorf("Expected new issue to be bd-6, got %s", newIssue.ID) - } -} - -// TestCounterSyncAfterDeleteAll verifies counter resets when all issues deleted (bd-49) -func TestCounterSyncAfterDeleteAll(t *testing.T) { - store, cleanup := setupTestDB(t) - defer cleanup() - - ctx := context.Background() - - // Set the issue prefix to "bd" for this test - if err := store.SetConfig(ctx, "issue_prefix", "bd"); err != nil { - t.Fatalf("Failed to set issue_prefix: %v", err) - } - - // Create issues bd-1 through bd-5 - for i := 1; i <= 5; i++ { - issue := &types.Issue{ - ID: genID("bd", i), - Title: "Test issue", - Priority: 2, - IssueType: types.TypeTask, - Status: types.StatusOpen, - } - if err := store.CreateIssue(ctx, issue, "test"); err != nil { - t.Fatalf("Failed to create issue: %v", err) - } - } - - // Delete all issues - toDelete := []string{"bd-1", "bd-2", "bd-3", "bd-4", "bd-5"} - _, err := store.DeleteIssues(ctx, toDelete, false, false, false) - if err != nil { - t.Fatalf("Failed to delete all issues: %v", err) - } - - // Counter should be deleted (no issues left with this prefix) - var counter int - err = store.db.QueryRow(`SELECT last_id FROM issue_counters WHERE prefix = 'bd'`).Scan(&counter) - if err == nil { - t.Errorf("Expected no counter row, but found counter at %d", counter) - } - - // Create new issue - should be bd-1 (fresh start) - newIssue := &types.Issue{ - Title: "First issue after deleting all", - Priority: 2, - IssueType: types.TypeTask, - Status: types.StatusOpen, - } - if err := store.CreateIssue(ctx, newIssue, "test"); err != nil { - t.Fatalf("Failed to create new issue: %v", err) - } - if newIssue.ID != testIssueBD1 { - t.Errorf("Expected new issue to be bd-1, got %s", newIssue.ID) - } -} - -// genID is a helper to generate issue IDs in tests -func genID(_ string, num int) string { - return fmt.Sprintf("bd-%d", num) -} diff --git a/internal/storage/sqlite/hash_id_test.go b/internal/storage/sqlite/hash_id_test.go index 1978b7e5..85507f2e 100644 --- a/internal/storage/sqlite/hash_id_test.go +++ b/internal/storage/sqlite/hash_id_test.go @@ -17,13 +17,10 @@ func TestHashIDGeneration(t *testing.T) { ctx := context.Background() - // Set up database with prefix and hash mode + // Set up database with prefix if err := store.SetConfig(ctx, "issue_prefix", "bd"); err != nil { t.Fatalf("Failed to set prefix: %v", err) } - if err := store.SetConfig(ctx, "id_mode", "hash"); err != nil { - t.Fatalf("Failed to set id_mode: %v", err) - } // Create an issue - should get a hash ID issue := &types.Issue{ @@ -143,13 +140,10 @@ func TestHashIDBatchCreation(t *testing.T) { ctx := context.Background() - // Set up database with prefix and hash mode + // Set up database with prefix if err := store.SetConfig(ctx, "issue_prefix", "bd"); err != nil { t.Fatalf("Failed to set prefix: %v", err) } - if err := store.SetConfig(ctx, "id_mode", "hash"); err != nil { - t.Fatalf("Failed to set id_mode: %v", err) - } // Create multiple issues with similar content issues := []*types.Issue{ diff --git a/internal/storage/sqlite/lazy_init_test.go b/internal/storage/sqlite/lazy_init_test.go deleted file mode 100644 index 3390565f..00000000 --- a/internal/storage/sqlite/lazy_init_test.go +++ /dev/null @@ -1,229 +0,0 @@ -package sqlite - -import ( - "context" - "os" - "path/filepath" - "testing" - - "github.com/steveyegge/beads/internal/types" -) - -const testIssueCustom1 = "custom-1" - -// 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() - - // Set the issue prefix to "bd" for this test - if err := store.SetConfig(ctx, "issue_prefix", "bd"); err != nil { - t.Fatalf("Failed to set issue_prefix: %v", err) - } - - // 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 != testIssueCustom1 { - 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() - - // Set the issue prefix to "bd" for this test - if err := store.SetConfig(ctx, "issue_prefix", "bd"); err != nil { - t.Fatalf("Failed to set issue_prefix: %v", err) - } - - // 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 deleted file mode 100644 index be877cd9..00000000 --- a/internal/storage/sqlite/migration_test.go +++ /dev/null @@ -1,299 +0,0 @@ -package sqlite - -import ( - "context" - "database/sql" - "os" - "path/filepath" - "testing" - - "github.com/steveyegge/beads/internal/types" - _ "modernc.org/sqlite" -) - -// 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("sqlite", 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() - - // Set the issue prefix to "bd" for this test - err = store.SetConfig(ctx, "issue_prefix", "bd") - if err != nil { - t.Fatalf("Failed to set issue_prefix: %v", err) - } - - 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() - - // Set the issue prefix to "bd" for this test - err = store1.SetConfig(ctx, "issue_prefix", "bd") - if err != nil { - t.Fatalf("Failed to set issue_prefix: %v", err) - } - - 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/schema.go b/internal/storage/sqlite/schema.go index 7bb7c19c..0b638dd5 100644 --- a/internal/storage/sqlite/schema.go +++ b/internal/storage/sqlite/schema.go @@ -129,12 +129,6 @@ CREATE TABLE IF NOT EXISTS export_hashes ( FOREIGN KEY (issue_id) REFERENCES issues(id) ON DELETE CASCADE ); --- Issue counters table (for atomic ID generation) -CREATE TABLE IF NOT EXISTS issue_counters ( - prefix TEXT PRIMARY KEY, - last_id INTEGER NOT NULL DEFAULT 0 -); - -- Child counters table (for hierarchical ID generation) -- Tracks sequential child numbers per parent issue CREATE TABLE IF NOT EXISTS child_counters ( diff --git a/internal/storage/sqlite/sqlite.go b/internal/storage/sqlite/sqlite.go index 6da95b44..4e82b46a 100644 --- a/internal/storage/sqlite/sqlite.go +++ b/internal/storage/sqlite/sqlite.go @@ -78,11 +78,6 @@ 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) - } - // Migrate existing databases to add external_ref column if missing if err := migrateExternalRefColumn(db); err != nil { return nil, fmt.Errorf("failed to migrate external_ref column: %w", err) @@ -193,66 +188,6 @@ 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 -} - // migrateExternalRefColumn checks if the external_ref column exists and adds it if missing. // This ensures existing databases created before the external reference feature get migrated automatically. func migrateExternalRefColumn(db *sql.DB) error { @@ -696,40 +631,10 @@ func (s *SQLiteStorage) GetNextChildID(ctx context.Context, parentID string) (st return childID, 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 -// Note: This unconditionally overwrites counter values, allowing them to decrease after deletions +// SyncAllCounters is a no-op now that sequential IDs are removed (bd-aa744b). +// Kept for backward compatibility with existing code that calls it. func (s *SQLiteStorage) SyncAllCounters(ctx context.Context) error { - // First, delete counters for prefixes that have no issues - _, err := s.db.ExecContext(ctx, ` - DELETE FROM issue_counters - WHERE prefix NOT IN ( - SELECT DISTINCT substr(id, 1, instr(id, '-') - 1) - FROM issues - WHERE instr(id, '-') > 0 - AND substr(id, instr(id, '-') + 1) GLOB '[0-9]*' - ) - `) - if err != nil { - return fmt.Errorf("failed to delete orphaned counters: %w", err) - } - - // Then, upsert counters for prefixes that have issues - _, err = s.db.ExecContext(ctx, ` - 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 = excluded.last_id - `) - if err != nil { - return fmt.Errorf("failed to sync counters: %w", err) - } + // No-op: hash IDs don't use counters return nil } @@ -737,43 +642,7 @@ func (s *SQLiteStorage) SyncAllCounters(ctx context.Context) error { // The database should ALWAYS have issue_prefix config set explicitly (by 'bd init' or auto-import) // Never derive prefix from filename - it leads to silent data corruption -// getIDMode returns the ID generation mode from config (sequential or hash). -// Defaults to "sequential" for backward compatibility if not set. -func getIDMode(ctx context.Context, conn *sql.Conn) string { - var mode string - err := conn.QueryRowContext(ctx, `SELECT value FROM config WHERE key = ?`, "id_mode").Scan(&mode) - if err != nil || mode == "" { - return "sequential" // Default to sequential for backward compatibility - } - return mode -} -// nextSequentialID atomically increments and returns the next sequential ID number. -// Must be called inside an IMMEDIATE transaction on the same connection. -// Implements lazy initialization: if counter doesn't exist, initializes from existing issues. -func nextSequentialID(ctx context.Context, conn *sql.Conn, prefix string) (int, error) { - var nextID int - - // The query handles three cases atomically: - // 1. Counter doesn't exist: initialize from MAX(existing IDs) or 1, then return that + 1 - // 2. Counter exists but lower than max ID: update to max and return max + 1 - // 3. Counter exists and correct: just increment and return next ID - err := conn.QueryRowContext(ctx, ` - INSERT INTO issue_counters (prefix, last_id) - SELECT ?, COALESCE(MAX(CAST(substr(id, LENGTH(?) + 2) AS INTEGER)), 0) + 1 - FROM issues - WHERE id LIKE ? || '-%' - AND substr(id, LENGTH(?) + 2) GLOB '[0-9]*' - AND instr(substr(id, LENGTH(?) + 2), '.') = 0 - ON CONFLICT(prefix) DO UPDATE SET - last_id = last_id + 1 - RETURNING last_id - `, prefix, prefix, prefix, prefix, prefix).Scan(&nextID) - if err != nil { - return 0, fmt.Errorf("failed to generate next sequential ID for prefix %s: %w", prefix, err) - } - return nextID, nil -} // generateHashID creates a hash-based ID for a top-level issue. // For child issues, use the parent ID with a numeric suffix (e.g., "bd-a3f8e9.1"). @@ -869,61 +738,49 @@ func (s *SQLiteStorage) CreateIssue(ctx context.Context, issue *types.Issue, act // Generate ID if not set (inside transaction to prevent race conditions) if issue.ID == "" { - // Check id_mode config to determine ID generation strategy - idMode := getIDMode(ctx, conn) + // Generate hash-based ID with adaptive length based on database size (bd-ea2a13) + // Start with length determined by database size, expand on collision + var err error - if idMode == "hash" { - // Generate hash-based ID with adaptive length based on database size (bd-ea2a13) - // Start with length determined by database size, expand on collision - var err error - - // Get adaptive base length based on current database size - baseLength, err := GetAdaptiveIDLength(ctx, conn, prefix) - if err != nil { - // Fallback to 6 on error - baseLength = 6 - } - - // Try baseLength, baseLength+1, baseLength+2, up to max of 8 - maxLength := 8 - if baseLength > maxLength { - baseLength = maxLength - } - - for length := baseLength; length <= maxLength; length++ { - // Try up to 10 nonces at each length - for nonce := 0; nonce < 10; nonce++ { - candidate := generateHashID(prefix, issue.Title, issue.Description, actor, issue.CreatedAt, length, nonce) - - // Check if this ID already exists - var count int - err = conn.QueryRowContext(ctx, `SELECT COUNT(*) FROM issues WHERE id = ?`, candidate).Scan(&count) - if err != nil { - return fmt.Errorf("failed to check for ID collision: %w", err) - } - - if count == 0 { - issue.ID = candidate - break - } + // Get adaptive base length based on current database size + baseLength, err := GetAdaptiveIDLength(ctx, conn, prefix) + if err != nil { + // Fallback to 6 on error + baseLength = 6 + } + + // Try baseLength, baseLength+1, baseLength+2, up to max of 8 + maxLength := 8 + if baseLength > maxLength { + baseLength = maxLength + } + + for length := baseLength; length <= maxLength; length++ { + // Try up to 10 nonces at each length + for nonce := 0; nonce < 10; nonce++ { + candidate := generateHashID(prefix, issue.Title, issue.Description, actor, issue.CreatedAt, length, nonce) + + // Check if this ID already exists + var count int + err = conn.QueryRowContext(ctx, `SELECT COUNT(*) FROM issues WHERE id = ?`, candidate).Scan(&count) + if err != nil { + return fmt.Errorf("failed to check for ID collision: %w", err) } - // If we found a unique ID, stop trying longer lengths - if issue.ID != "" { + if count == 0 { + issue.ID = candidate break } } - if issue.ID == "" { - return fmt.Errorf("failed to generate unique ID after trying lengths %d-%d with 10 nonces each", baseLength, maxLength) + // If we found a unique ID, stop trying longer lengths + if issue.ID != "" { + break } - } else { - // Default: generate sequential ID using counter - nextID, err := nextSequentialID(ctx, conn, prefix) - if err != nil { - return err - } - issue.ID = fmt.Sprintf("%s-%d", prefix, nextID) + } + + if issue.ID == "" { + return fmt.Errorf("failed to generate unique ID after trying lengths %d-%d with 10 nonces each", baseLength, maxLength) } } else { // Validate that explicitly provided ID matches the configured prefix (bd-177) @@ -1037,9 +894,6 @@ func generateBatchIDs(ctx context.Context, conn *sql.Conn, issues []*types.Issue return fmt.Errorf("failed to get config: %w", err) } - // Check id_mode config to determine ID generation strategy - idMode := getIDMode(ctx, conn) - // Validate explicitly provided IDs and generate IDs for those that need them expectedPrefix := prefix + "-" usedIDs := make(map[string]bool) @@ -1056,64 +910,50 @@ func generateBatchIDs(ctx context.Context, conn *sql.Conn, issues []*types.Issue } // Second pass: generate IDs for issues that need them - if idMode == "hash" { - // Hash mode: generate with adaptive length based on database size (bd-ea2a13) - // Get adaptive base length based on current database size - baseLength, err := GetAdaptiveIDLength(ctx, conn, prefix) - if err != nil { - // Fallback to 6 on error - baseLength = 6 - } - - // Try baseLength, baseLength+1, baseLength+2, up to max of 8 - maxLength := 8 - if baseLength > maxLength { - baseLength = maxLength - } - - for i := range issues { - if issues[i].ID == "" { - var generated bool - // Try lengths from baseLength to maxLength with progressive fallback - for length := baseLength; length <= maxLength && !generated; length++ { - for nonce := 0; nonce < 10; nonce++ { - candidate := generateHashID(prefix, issues[i].Title, issues[i].Description, actor, issues[i].CreatedAt, length, nonce) - - // Check if this ID is already used in this batch or in the database - if usedIDs[candidate] { - continue - } - - var count int - err := conn.QueryRowContext(ctx, `SELECT COUNT(*) FROM issues WHERE id = ?`, candidate).Scan(&count) - if err != nil { - return fmt.Errorf("failed to check for ID collision: %w", err) - } - - if count == 0 { - issues[i].ID = candidate - usedIDs[candidate] = true - generated = true - break - } + // Hash mode: generate with adaptive length based on database size (bd-ea2a13) + // Get adaptive base length based on current database size + baseLength, err := GetAdaptiveIDLength(ctx, conn, prefix) + if err != nil { + // Fallback to 6 on error + baseLength = 6 + } + + // Try baseLength, baseLength+1, baseLength+2, up to max of 8 + maxLength := 8 + if baseLength > maxLength { + baseLength = maxLength + } + + for i := range issues { + if issues[i].ID == "" { + var generated bool + // Try lengths from baseLength to maxLength with progressive fallback + for length := baseLength; length <= maxLength && !generated; length++ { + for nonce := 0; nonce < 10; nonce++ { + candidate := generateHashID(prefix, issues[i].Title, issues[i].Description, actor, issues[i].CreatedAt, length, nonce) + + // Check if this ID is already used in this batch or in the database + if usedIDs[candidate] { + continue + } + + var count int + err := conn.QueryRowContext(ctx, `SELECT COUNT(*) FROM issues WHERE id = ?`, candidate).Scan(&count) + if err != nil { + return fmt.Errorf("failed to check for ID collision: %w", err) + } + + if count == 0 { + issues[i].ID = candidate + usedIDs[candidate] = true + generated = true + break } } - - if !generated { - return fmt.Errorf("failed to generate unique ID for issue %d after trying lengths 6-8 with 10 nonces each", i) - } } - } - } else { - // Sequential mode: allocate sequential IDs for all issues that need them - for i := range issues { - if issues[i].ID == "" { - nextID, err := nextSequentialID(ctx, conn, prefix) - if err != nil { - return fmt.Errorf("failed to generate sequential ID for issue %d: %w", i, err) - } - issues[i].ID = fmt.Sprintf("%s-%d", prefix, nextID) - usedIDs[issues[i].ID] = true + + if !generated { + return fmt.Errorf("failed to generate unique ID for issue %d after trying lengths 6-8 with 10 nonces each", i) } } }