perf: batch external dep checks by project (bd-687v)
Optimize CheckExternalDeps to group refs by project and open each external DB only once, checking all capabilities in a single query. Before: If 10 issues depend on external:gastown:cap1, we opened gastown's DB 10 times. After: We open each external project's DB once, query for all capabilities needed from that project, then close. Also added deduplication in filterByExternalDeps to collect all unique refs before checking, avoiding redundant checks for the same ref across multiple issues. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
committed by
Steve Yegge
parent
939f222c35
commit
32a4a9e060
@@ -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
|
||||
|
||||
@@ -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:<project>:<capability>
|
||||
// They are satisfied when the target project has a closed issue with provides:<capability> 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
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user