diff --git a/internal/storage/sqlite/external_deps.go b/internal/storage/sqlite/external_deps.go index b3fed373..9a1dbeeb 100644 --- a/internal/storage/sqlite/external_deps.go +++ b/internal/storage/sqlite/external_deps.go @@ -120,16 +120,172 @@ func CheckExternalDep(ctx context.Context, ref string) *ExternalDepStatus { return status } -// CheckExternalDeps checks multiple external dependencies. +// CheckExternalDeps checks multiple external dependencies with batching optimization. +// Groups refs by project and opens each external DB only once, checking all +// capabilities for that project in a single query. This avoids O(N) DB opens +// when multiple issues depend on the same external project. // Returns a map of ref -> status. func CheckExternalDeps(ctx context.Context, refs []string) map[string]*ExternalDepStatus { results := make(map[string]*ExternalDepStatus) + + // Parse and group refs by project + // Key: project name, Value: map of capability -> list of original refs + // (multiple refs might have same project:capability, we dedupe) + projectCaps := make(map[string]map[string][]string) + for _, ref := range refs { - results[ref] = CheckExternalDep(ctx, ref) + parsed := parseExternalRef(ref) + if parsed == nil { + results[ref] = &ExternalDepStatus{ + Ref: ref, + Satisfied: false, + Reason: "invalid external reference format", + } + continue + } + + if projectCaps[parsed.project] == nil { + projectCaps[parsed.project] = make(map[string][]string) + } + projectCaps[parsed.project][parsed.capability] = append( + projectCaps[parsed.project][parsed.capability], ref) } + + // Check each project's capabilities in batch + for project, caps := range projectCaps { + capList := make([]string, 0, len(caps)) + for cap := range caps { + capList = append(capList, cap) + } + + // Check all capabilities for this project in one DB open + satisfied := checkProjectCapabilities(ctx, project, capList) + + // Map results back to original refs + for cap, refList := range caps { + isSatisfied := satisfied[cap] + reason := "capability shipped" + if !isSatisfied { + reason = "capability not shipped (no closed issue with provides:" + cap + " label)" + } + + for _, ref := range refList { + results[ref] = &ExternalDepStatus{ + Ref: ref, + Project: project, + Capability: cap, + Satisfied: isSatisfied, + Reason: reason, + } + } + } + } + return results } +// parsedRef holds parsed components of an external reference +type parsedRef struct { + project string + capability string +} + +// parseExternalRef parses "external:project:capability" into components. +// Returns nil if the format is invalid. +func parseExternalRef(ref string) *parsedRef { + if !strings.HasPrefix(ref, "external:") { + return nil + } + parts := strings.SplitN(ref, ":", 3) + if len(parts) != 3 || parts[1] == "" || parts[2] == "" { + return nil + } + return &parsedRef{project: parts[1], capability: parts[2]} +} + +// checkProjectCapabilities opens a project's beads DB once and checks +// multiple capabilities in a single query. Returns map of capability -> satisfied. +func checkProjectCapabilities(ctx context.Context, project string, capabilities []string) map[string]bool { + result := make(map[string]bool) + for _, cap := range capabilities { + result[cap] = false // default to unsatisfied + } + + if len(capabilities) == 0 { + return result + } + + // Look up project path from config + projectPath := config.ResolveExternalProjectPath(project) + if projectPath == "" { + return result // all unsatisfied - project not configured + } + + // Find the beads database in the project + beadsDir := filepath.Join(projectPath, ".beads") + cfg, err := configfile.Load(beadsDir) + if err != nil || cfg == nil { + return result // all unsatisfied - no beads database + } + + dbPath := cfg.DatabasePath(beadsDir) + + // Verify database file exists + if _, err := os.Stat(dbPath); err != nil { + return result // all unsatisfied - database not found + } + + // Open the external database once for all capability checks + db, err := sql.Open("sqlite3", dbPath) + if err != nil { + return result // all unsatisfied - cannot open + } + defer func() { _ = db.Close() }() + + if err := db.Ping(); err != nil { + return result // all unsatisfied - cannot connect + } + + // Build query to check all capabilities at once + // SELECT label FROM labels WHERE label IN ('provides:cap1', 'provides:cap2', ...) + // AND EXISTS closed issue with that label + placeholders := make([]string, len(capabilities)) + args := make([]interface{}, len(capabilities)) + for i, cap := range capabilities { + placeholders[i] = "?" + args[i] = "provides:" + cap + } + + // Query returns which provides: labels exist on closed issues + query := ` + SELECT DISTINCT l.label FROM labels l + JOIN issues i ON l.issue_id = i.id + WHERE i.status = 'closed' + AND l.label IN (` + strings.Join(placeholders, ",") + `) + ` + + rows, err := db.QueryContext(ctx, query, args...) + if err != nil { + return result // all unsatisfied - query failed + } + defer func() { _ = rows.Close() }() + + // Mark satisfied capabilities + for rows.Next() { + var label string + if err := rows.Scan(&label); err != nil { + continue + } + // Extract capability from "provides:capability" + if strings.HasPrefix(label, "provides:") { + cap := strings.TrimPrefix(label, "provides:") + result[cap] = true + } + } + + return result +} + // GetUnsatisfiedExternalDeps returns external dependencies that are not satisfied. func GetUnsatisfiedExternalDeps(ctx context.Context, refs []string) []string { var unsatisfied []string diff --git a/internal/storage/sqlite/ready.go b/internal/storage/sqlite/ready.go index 4a75105e..45358376 100644 --- a/internal/storage/sqlite/ready.go +++ b/internal/storage/sqlite/ready.go @@ -191,6 +191,10 @@ func (s *SQLiteStorage) GetReadyWork(ctx context.Context, filter types.WorkFilte // filterByExternalDeps removes issues that have unsatisfied external dependencies. // External deps have format: external:: // They are satisfied when the target project has a closed issue with provides: label. +// +// Optimization: Collects all unique external refs across all issues, then checks +// them in batch (one DB open per external project) rather than checking each +// ref individually. This avoids O(N) DB opens when issues share external deps. func (s *SQLiteStorage) filterByExternalDeps(ctx context.Context, issues []*types.Issue) ([]*types.Issue, error) { if len(issues) == 0 { return issues, nil @@ -213,12 +217,26 @@ func (s *SQLiteStorage) filterByExternalDeps(ctx context.Context, issues []*type return issues, nil } - // Check each external dep and build set of blocked issue IDs + // Collect all unique external refs across all issues + uniqueRefs := make(map[string]bool) + for _, deps := range externalDeps { + for _, dep := range deps { + uniqueRefs[dep] = true + } + } + + // Check all refs in batch (grouped by project internally) + refList := make([]string, 0, len(uniqueRefs)) + for ref := range uniqueRefs { + refList = append(refList, ref) + } + statuses := CheckExternalDeps(ctx, refList) + + // Build set of blocked issue IDs using batch results blockedIssues := make(map[string]bool) for issueID, deps := range externalDeps { for _, dep := range deps { - status := CheckExternalDep(ctx, dep) - if !status.Satisfied { + if status, ok := statuses[dep]; ok && !status.Satisfied { blockedIssues[issueID] = true break // One unsatisfied dep is enough to block } diff --git a/internal/storage/sqlite/ready_test.go b/internal/storage/sqlite/ready_test.go index 453d8fdb..a07fa982 100644 --- a/internal/storage/sqlite/ready_test.go +++ b/internal/storage/sqlite/ready_test.go @@ -1513,6 +1513,140 @@ func TestCheckExternalDepInvalidFormats(t *testing.T) { } } +// TestCheckExternalDepsBatching verifies that CheckExternalDeps correctly +// batches multiple refs to the same project and deduplicates refs (bd-687v). +func TestCheckExternalDepsBatching(t *testing.T) { + ctx := context.Background() + + // Initialize config (required for config.Set to work) + if err := config.Initialize(); err != nil { + t.Fatalf("failed to initialize config: %v", err) + } + + // Create external project directory with beads database + externalDir, err := os.MkdirTemp("", "beads-batch-test-*") + if err != nil { + t.Fatalf("failed to create external temp dir: %v", err) + } + defer os.RemoveAll(externalDir) + + // Create .beads directory and config + beadsDir := filepath.Join(externalDir, ".beads") + if err := os.MkdirAll(beadsDir, 0755); err != nil { + t.Fatalf("failed to create beads dir: %v", err) + } + cfg := configfile.DefaultConfig() + if err := cfg.Save(beadsDir); err != nil { + t.Fatalf("failed to save config: %v", err) + } + + // Create external database + externalDBPath := filepath.Join(beadsDir, "beads.db") + externalStore, err := New(ctx, externalDBPath) + if err != nil { + t.Fatalf("failed to create external store: %v", err) + } + + if err := externalStore.SetConfig(ctx, "issue_prefix", "ext"); err != nil { + t.Fatalf("failed to set issue_prefix: %v", err) + } + + // Ship capability "cap1" (closed issue with provides:cap1 label) + cap1Issue := &types.Issue{ + ID: "ext-cap1", + Title: "Capability 1", + Status: types.StatusOpen, + Priority: 1, + IssueType: types.TypeFeature, + } + if err := externalStore.CreateIssue(ctx, cap1Issue, "test-user"); err != nil { + t.Fatalf("failed to create cap1 issue: %v", err) + } + if err := externalStore.AddLabel(ctx, cap1Issue.ID, "provides:cap1", "test-user"); err != nil { + t.Fatalf("failed to add provides:cap1 label: %v", err) + } + if err := externalStore.CloseIssue(ctx, cap1Issue.ID, "Shipped", "test-user", ""); err != nil { + t.Fatalf("failed to close cap1 issue: %v", err) + } + + // Ship capability "cap2" + cap2Issue := &types.Issue{ + ID: "ext-cap2", + Title: "Capability 2", + Status: types.StatusOpen, + Priority: 1, + IssueType: types.TypeFeature, + } + if err := externalStore.CreateIssue(ctx, cap2Issue, "test-user"); err != nil { + t.Fatalf("failed to create cap2 issue: %v", err) + } + if err := externalStore.AddLabel(ctx, cap2Issue.ID, "provides:cap2", "test-user"); err != nil { + t.Fatalf("failed to add provides:cap2 label: %v", err) + } + if err := externalStore.CloseIssue(ctx, cap2Issue.ID, "Shipped", "test-user", ""); err != nil { + t.Fatalf("failed to close cap2 issue: %v", err) + } + + // Close to checkpoint WAL before read-only access + externalStore.Close() + + // Configure external_projects + oldProjects := config.GetExternalProjects() + t.Cleanup(func() { + if oldProjects != nil { + config.Set("external_projects", oldProjects) + } else { + config.Set("external_projects", map[string]string{}) + } + }) + config.Set("external_projects", map[string]string{ + "batch-test": externalDir, + }) + + // Test: Check multiple refs including duplicates and mixed satisfied/unsatisfied + refs := []string{ + "external:batch-test:cap1", // satisfied + "external:batch-test:cap2", // satisfied + "external:batch-test:cap3", // NOT satisfied + "external:batch-test:cap1", // duplicate - should still work + "external:unconfigured-project:cap1", // unconfigured project + "invalid-ref", // invalid format + } + + statuses := CheckExternalDeps(ctx, refs) + + // Verify we got results for all unique refs (5 unique, since cap1 appears twice) + expectedUnique := 5 + if len(statuses) != expectedUnique { + t.Errorf("Expected %d unique statuses, got %d", expectedUnique, len(statuses)) + } + + // cap1 should be satisfied + if s := statuses["external:batch-test:cap1"]; s == nil || !s.Satisfied { + t.Error("Expected external:batch-test:cap1 to be satisfied") + } + + // cap2 should be satisfied + if s := statuses["external:batch-test:cap2"]; s == nil || !s.Satisfied { + t.Error("Expected external:batch-test:cap2 to be satisfied") + } + + // cap3 should NOT be satisfied + if s := statuses["external:batch-test:cap3"]; s == nil || s.Satisfied { + t.Error("Expected external:batch-test:cap3 to be unsatisfied") + } + + // unconfigured project should NOT be satisfied + if s := statuses["external:unconfigured-project:cap1"]; s == nil || s.Satisfied { + t.Error("Expected external:unconfigured-project:cap1 to be unsatisfied") + } + + // invalid ref should NOT be satisfied + if s := statuses["invalid-ref"]; s == nil || s.Satisfied { + t.Error("Expected invalid-ref to be unsatisfied") + } +} + // TestGetNewlyUnblockedByClose tests the --suggest-next functionality (GH#679) func TestGetNewlyUnblockedByClose(t *testing.T) { env := newTestEnv(t)