feat: Implement cross-type cycle prevention in dependencies
- Remove type filter from cycle detection to check ALL dependency types - Extract maxDependencyDepth=100 constant shared across AddDependency and DetectCycles - Move cycle check before INSERT to avoid unnecessary write on failure - Add comprehensive tests: self-dependency, related cycles, cross-type cycles - Verify idx_dependencies_issue index exists for performance Fixes bd-312. Prevents cross-type cycles (e.g., A blocks B, B parent-child A) that previously hid work from ready list. Addresses oracle feedback for proper implementation.
This commit is contained in:
File diff suppressed because one or more lines are too long
@@ -11,6 +11,12 @@ import (
|
|||||||
"github.com/steveyegge/beads/internal/types"
|
"github.com/steveyegge/beads/internal/types"
|
||||||
)
|
)
|
||||||
|
|
||||||
|
const (
|
||||||
|
// maxDependencyDepth is the maximum depth for recursive dependency traversal
|
||||||
|
// to prevent infinite loops and limit query complexity
|
||||||
|
maxDependencyDepth = 100
|
||||||
|
)
|
||||||
|
|
||||||
// AddDependency adds a dependency between issues with cycle prevention
|
// AddDependency adds a dependency between issues with cycle prevention
|
||||||
func (s *SQLiteStorage) AddDependency(ctx context.Context, dep *types.Dependency, actor string) error {
|
func (s *SQLiteStorage) AddDependency(ctx context.Context, dep *types.Dependency, actor string) error {
|
||||||
// Validate dependency type
|
// Validate dependency type
|
||||||
@@ -49,6 +55,47 @@ func (s *SQLiteStorage) AddDependency(ctx context.Context, dep *types.Dependency
|
|||||||
}
|
}
|
||||||
defer tx.Rollback()
|
defer tx.Rollback()
|
||||||
|
|
||||||
|
// Check if this would create a cycle across ALL dependency types
|
||||||
|
// We check before inserting to avoid unnecessary write on failure
|
||||||
|
// We traverse all dependency types to detect cross-type cycles
|
||||||
|
// (e.g., A blocks B, B parent-child A would create a cycle)
|
||||||
|
// We need to check if we can reach IssueID from DependsOnID
|
||||||
|
// If yes, adding "IssueID depends on DependsOnID" would create a cycle
|
||||||
|
var cycleExists bool
|
||||||
|
err = tx.QueryRowContext(ctx, `
|
||||||
|
WITH RECURSIVE paths AS (
|
||||||
|
SELECT
|
||||||
|
issue_id,
|
||||||
|
depends_on_id,
|
||||||
|
1 as depth
|
||||||
|
FROM dependencies
|
||||||
|
WHERE issue_id = ?
|
||||||
|
|
||||||
|
UNION ALL
|
||||||
|
|
||||||
|
SELECT
|
||||||
|
d.issue_id,
|
||||||
|
d.depends_on_id,
|
||||||
|
p.depth + 1
|
||||||
|
FROM dependencies d
|
||||||
|
JOIN paths p ON d.issue_id = p.depends_on_id
|
||||||
|
WHERE p.depth < ?
|
||||||
|
)
|
||||||
|
SELECT EXISTS(
|
||||||
|
SELECT 1 FROM paths
|
||||||
|
WHERE depends_on_id = ?
|
||||||
|
)
|
||||||
|
`, dep.DependsOnID, maxDependencyDepth, dep.IssueID).Scan(&cycleExists)
|
||||||
|
|
||||||
|
if err != nil {
|
||||||
|
return fmt.Errorf("failed to check for cycles: %w", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
if cycleExists {
|
||||||
|
return fmt.Errorf("cannot add dependency: would create a cycle (%s → %s → ... → %s)",
|
||||||
|
dep.IssueID, dep.DependsOnID, dep.IssueID)
|
||||||
|
}
|
||||||
|
|
||||||
// Insert dependency
|
// Insert dependency
|
||||||
_, err = tx.ExecContext(ctx, `
|
_, err = tx.ExecContext(ctx, `
|
||||||
INSERT INTO dependencies (issue_id, depends_on_id, type, created_at, created_by)
|
INSERT INTO dependencies (issue_id, depends_on_id, type, created_at, created_by)
|
||||||
@@ -58,48 +105,6 @@ func (s *SQLiteStorage) AddDependency(ctx context.Context, dep *types.Dependency
|
|||||||
return fmt.Errorf("failed to add dependency: %w", err)
|
return fmt.Errorf("failed to add dependency: %w", err)
|
||||||
}
|
}
|
||||||
|
|
||||||
// Check if this creates a cycle (only for 'blocks' type dependencies)
|
|
||||||
// We need to check if we can reach IssueID from DependsOnID
|
|
||||||
// If yes, adding "IssueID depends on DependsOnID" would create a cycle
|
|
||||||
if dep.Type == types.DepBlocks {
|
|
||||||
var cycleExists bool
|
|
||||||
err = tx.QueryRowContext(ctx, `
|
|
||||||
WITH RECURSIVE paths AS (
|
|
||||||
SELECT
|
|
||||||
issue_id,
|
|
||||||
depends_on_id,
|
|
||||||
1 as depth
|
|
||||||
FROM dependencies
|
|
||||||
WHERE type = 'blocks'
|
|
||||||
AND issue_id = ?
|
|
||||||
|
|
||||||
UNION ALL
|
|
||||||
|
|
||||||
SELECT
|
|
||||||
d.issue_id,
|
|
||||||
d.depends_on_id,
|
|
||||||
p.depth + 1
|
|
||||||
FROM dependencies d
|
|
||||||
JOIN paths p ON d.issue_id = p.depends_on_id
|
|
||||||
WHERE d.type = 'blocks'
|
|
||||||
AND p.depth < 100
|
|
||||||
)
|
|
||||||
SELECT EXISTS(
|
|
||||||
SELECT 1 FROM paths
|
|
||||||
WHERE depends_on_id = ?
|
|
||||||
)
|
|
||||||
`, dep.DependsOnID, dep.IssueID).Scan(&cycleExists)
|
|
||||||
|
|
||||||
if err != nil {
|
|
||||||
return fmt.Errorf("failed to check for cycles: %w", err)
|
|
||||||
}
|
|
||||||
|
|
||||||
if cycleExists {
|
|
||||||
return fmt.Errorf("cannot add dependency: would create a cycle (%s → %s → ... → %s)",
|
|
||||||
dep.IssueID, dep.DependsOnID, dep.IssueID)
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
// Record event
|
// Record event
|
||||||
_, err = tx.ExecContext(ctx, `
|
_, err = tx.ExecContext(ctx, `
|
||||||
INSERT INTO events (issue_id, event_type, actor, comment)
|
INSERT INTO events (issue_id, event_type, actor, comment)
|
||||||
@@ -387,14 +392,14 @@ func (s *SQLiteStorage) DetectCycles(ctx context.Context) ([][]*types.Issue, err
|
|||||||
p.depth + 1
|
p.depth + 1
|
||||||
FROM dependencies d
|
FROM dependencies d
|
||||||
JOIN paths p ON d.issue_id = p.depends_on_id
|
JOIN paths p ON d.issue_id = p.depends_on_id
|
||||||
WHERE p.depth < 100
|
WHERE p.depth < ?
|
||||||
AND p.path NOT LIKE '%' || d.depends_on_id || '→%'
|
AND p.path NOT LIKE '%' || d.depends_on_id || '→%'
|
||||||
)
|
)
|
||||||
SELECT DISTINCT path || '→' || start_id as cycle_path
|
SELECT DISTINCT path || '→' || start_id as cycle_path
|
||||||
FROM paths
|
FROM paths
|
||||||
WHERE depends_on_id = start_id
|
WHERE depends_on_id = start_id
|
||||||
ORDER BY cycle_path
|
ORDER BY cycle_path
|
||||||
`)
|
`, maxDependencyDepth)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return nil, fmt.Errorf("failed to detect cycles: %w", err)
|
return nil, fmt.Errorf("failed to detect cycles: %w", err)
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -2,6 +2,7 @@ package sqlite
|
|||||||
|
|
||||||
import (
|
import (
|
||||||
"context"
|
"context"
|
||||||
|
"strings"
|
||||||
"testing"
|
"testing"
|
||||||
|
|
||||||
"github.com/steveyegge/beads/internal/types"
|
"github.com/steveyegge/beads/internal/types"
|
||||||
@@ -249,3 +250,228 @@ func TestNoCyclesDetected(t *testing.T) {
|
|||||||
t.Errorf("Expected no cycles, but found %d", len(cycles))
|
t.Errorf("Expected no cycles, but found %d", len(cycles))
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func TestCrossTypeCyclePrevention(t *testing.T) {
|
||||||
|
store, cleanup := setupTestDB(t)
|
||||||
|
defer cleanup()
|
||||||
|
|
||||||
|
ctx := context.Background()
|
||||||
|
|
||||||
|
// Create issues for cross-type cycle test
|
||||||
|
issue1 := &types.Issue{Title: "Task A", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeTask}
|
||||||
|
issue2 := &types.Issue{Title: "Task B", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeTask}
|
||||||
|
|
||||||
|
store.CreateIssue(ctx, issue1, "test-user")
|
||||||
|
store.CreateIssue(ctx, issue2, "test-user")
|
||||||
|
|
||||||
|
// Add: issue1 blocks issue2
|
||||||
|
err := store.AddDependency(ctx, &types.Dependency{
|
||||||
|
IssueID: issue1.ID,
|
||||||
|
DependsOnID: issue2.ID,
|
||||||
|
Type: types.DepBlocks,
|
||||||
|
}, "test-user")
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("First dependency (blocks) failed: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Try to add: issue2 parent-child issue1 (this would create a cross-type cycle)
|
||||||
|
err = store.AddDependency(ctx, &types.Dependency{
|
||||||
|
IssueID: issue2.ID,
|
||||||
|
DependsOnID: issue1.ID,
|
||||||
|
Type: types.DepParentChild,
|
||||||
|
}, "test-user")
|
||||||
|
if err == nil {
|
||||||
|
t.Fatal("Expected error when creating cross-type cycle, but got none")
|
||||||
|
}
|
||||||
|
|
||||||
|
// Verify no cycles exist
|
||||||
|
cycles, err := store.DetectCycles(ctx)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("DetectCycles failed: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
if len(cycles) != 0 {
|
||||||
|
t.Errorf("Expected no cycles after prevention, but found %d", len(cycles))
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestCrossTypeCyclePreventionDiscoveredFrom(t *testing.T) {
|
||||||
|
store, cleanup := setupTestDB(t)
|
||||||
|
defer cleanup()
|
||||||
|
|
||||||
|
ctx := context.Background()
|
||||||
|
|
||||||
|
// Create issues
|
||||||
|
issue1 := &types.Issue{Title: "Parent Task", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeTask}
|
||||||
|
issue2 := &types.Issue{Title: "Bug Found", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeBug}
|
||||||
|
|
||||||
|
store.CreateIssue(ctx, issue1, "test-user")
|
||||||
|
store.CreateIssue(ctx, issue2, "test-user")
|
||||||
|
|
||||||
|
// Add: issue2 discovered-from issue1
|
||||||
|
err := store.AddDependency(ctx, &types.Dependency{
|
||||||
|
IssueID: issue2.ID,
|
||||||
|
DependsOnID: issue1.ID,
|
||||||
|
Type: types.DepDiscoveredFrom,
|
||||||
|
}, "test-user")
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("First dependency (discovered-from) failed: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Try to add: issue1 blocks issue2 (this would create a cross-type cycle)
|
||||||
|
err = store.AddDependency(ctx, &types.Dependency{
|
||||||
|
IssueID: issue1.ID,
|
||||||
|
DependsOnID: issue2.ID,
|
||||||
|
Type: types.DepBlocks,
|
||||||
|
}, "test-user")
|
||||||
|
if err == nil {
|
||||||
|
t.Fatal("Expected error when creating cross-type cycle with discovered-from, but got none")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestSelfDependencyPrevention(t *testing.T) {
|
||||||
|
store, cleanup := setupTestDB(t)
|
||||||
|
defer cleanup()
|
||||||
|
|
||||||
|
ctx := context.Background()
|
||||||
|
|
||||||
|
issue := &types.Issue{Title: "Task", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeTask}
|
||||||
|
store.CreateIssue(ctx, issue, "test-user")
|
||||||
|
|
||||||
|
// Try to create self-dependency (issue depends on itself)
|
||||||
|
err := store.AddDependency(ctx, &types.Dependency{
|
||||||
|
IssueID: issue.ID,
|
||||||
|
DependsOnID: issue.ID,
|
||||||
|
Type: types.DepBlocks,
|
||||||
|
}, "test-user")
|
||||||
|
|
||||||
|
if err == nil {
|
||||||
|
t.Fatal("Expected error when creating self-dependency, but got none")
|
||||||
|
}
|
||||||
|
|
||||||
|
if !strings.Contains(err.Error(), "cannot depend on itself") {
|
||||||
|
t.Errorf("Expected self-dependency error message, got: %v", err)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestRelatedTypeCyclePrevention(t *testing.T) {
|
||||||
|
store, cleanup := setupTestDB(t)
|
||||||
|
defer cleanup()
|
||||||
|
|
||||||
|
ctx := context.Background()
|
||||||
|
|
||||||
|
issue1 := &types.Issue{Title: "Task A", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeTask}
|
||||||
|
issue2 := &types.Issue{Title: "Task B", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeTask}
|
||||||
|
|
||||||
|
store.CreateIssue(ctx, issue1, "test-user")
|
||||||
|
store.CreateIssue(ctx, issue2, "test-user")
|
||||||
|
|
||||||
|
// Add: issue1 related issue2
|
||||||
|
err := store.AddDependency(ctx, &types.Dependency{
|
||||||
|
IssueID: issue1.ID,
|
||||||
|
DependsOnID: issue2.ID,
|
||||||
|
Type: types.DepRelated,
|
||||||
|
}, "test-user")
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("First dependency (related) failed: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Try to add: issue2 related issue1 (this creates a 2-node cycle with related type)
|
||||||
|
err = store.AddDependency(ctx, &types.Dependency{
|
||||||
|
IssueID: issue2.ID,
|
||||||
|
DependsOnID: issue1.ID,
|
||||||
|
Type: types.DepRelated,
|
||||||
|
}, "test-user")
|
||||||
|
if err == nil {
|
||||||
|
t.Fatal("Expected error when creating related-type cycle, but got none")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestMixedTypeRelatedCyclePrevention(t *testing.T) {
|
||||||
|
store, cleanup := setupTestDB(t)
|
||||||
|
defer cleanup()
|
||||||
|
|
||||||
|
ctx := context.Background()
|
||||||
|
|
||||||
|
issue1 := &types.Issue{Title: "Task A", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeTask}
|
||||||
|
issue2 := &types.Issue{Title: "Task B", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeTask}
|
||||||
|
|
||||||
|
store.CreateIssue(ctx, issue1, "test-user")
|
||||||
|
store.CreateIssue(ctx, issue2, "test-user")
|
||||||
|
|
||||||
|
// Add: issue1 blocks issue2
|
||||||
|
err := store.AddDependency(ctx, &types.Dependency{
|
||||||
|
IssueID: issue1.ID,
|
||||||
|
DependsOnID: issue2.ID,
|
||||||
|
Type: types.DepBlocks,
|
||||||
|
}, "test-user")
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("First dependency (blocks) failed: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Try to add: issue2 related issue1 (this creates a cross-type cycle)
|
||||||
|
err = store.AddDependency(ctx, &types.Dependency{
|
||||||
|
IssueID: issue2.ID,
|
||||||
|
DependsOnID: issue1.ID,
|
||||||
|
Type: types.DepRelated,
|
||||||
|
}, "test-user")
|
||||||
|
if err == nil {
|
||||||
|
t.Fatal("Expected error when creating blocks+related cycle, but got none")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestCrossTypeCyclePreventionThreeIssues(t *testing.T) {
|
||||||
|
store, cleanup := setupTestDB(t)
|
||||||
|
defer cleanup()
|
||||||
|
|
||||||
|
ctx := context.Background()
|
||||||
|
|
||||||
|
// Create issues for 3-node cross-type cycle test
|
||||||
|
issue1 := &types.Issue{Title: "Task A", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeTask}
|
||||||
|
issue2 := &types.Issue{Title: "Task B", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeTask}
|
||||||
|
issue3 := &types.Issue{Title: "Task C", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeTask}
|
||||||
|
|
||||||
|
store.CreateIssue(ctx, issue1, "test-user")
|
||||||
|
store.CreateIssue(ctx, issue2, "test-user")
|
||||||
|
store.CreateIssue(ctx, issue3, "test-user")
|
||||||
|
|
||||||
|
// Add: issue1 blocks issue2
|
||||||
|
err := store.AddDependency(ctx, &types.Dependency{
|
||||||
|
IssueID: issue1.ID,
|
||||||
|
DependsOnID: issue2.ID,
|
||||||
|
Type: types.DepBlocks,
|
||||||
|
}, "test-user")
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("First dependency failed: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Add: issue2 parent-child issue3
|
||||||
|
err = store.AddDependency(ctx, &types.Dependency{
|
||||||
|
IssueID: issue2.ID,
|
||||||
|
DependsOnID: issue3.ID,
|
||||||
|
Type: types.DepParentChild,
|
||||||
|
}, "test-user")
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("Second dependency failed: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Try to add: issue3 discovered-from issue1 (this would create a 3-node cross-type cycle)
|
||||||
|
err = store.AddDependency(ctx, &types.Dependency{
|
||||||
|
IssueID: issue3.ID,
|
||||||
|
DependsOnID: issue1.ID,
|
||||||
|
Type: types.DepDiscoveredFrom,
|
||||||
|
}, "test-user")
|
||||||
|
if err == nil {
|
||||||
|
t.Fatal("Expected error when creating 3-node cross-type cycle, but got none")
|
||||||
|
}
|
||||||
|
|
||||||
|
// Verify no cycles exist
|
||||||
|
cycles, err := store.DetectCycles(ctx)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("DetectCycles failed: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
if len(cycles) != 0 {
|
||||||
|
t.Errorf("Expected no cycles after prevention, but found %d", len(cycles))
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user