From 3cb2e790a9254798a3915b969b90423aaf19de95 Mon Sep 17 00:00:00 2001 From: Steve Yegge Date: Tue, 4 Nov 2025 17:00:37 -0800 Subject: [PATCH] Fix transaction conflict in TryResurrectParent (bd-58c0) Refactored resurrection functions to accept optional *sql.Conn parameter: - Added tryResurrectParentWithConn() internal function - Added tryResurrectParentChainWithConn() internal function - Updated CreateIssue to use conn-based resurrection - Updated EnsureIDs to use conn-based resurrection This eliminates 'database is locked' errors when resurrection happens inside an existing transaction. Fixes bd-58c0 --- internal/storage/sqlite/child_id_test.go | 6 ++-- internal/storage/sqlite/ids.go | 11 +++--- internal/storage/sqlite/resurrection.go | 43 +++++++++++++++++------- internal/storage/sqlite/sqlite.go | 11 +++--- 4 files changed, 47 insertions(+), 24 deletions(-) diff --git a/internal/storage/sqlite/child_id_test.go b/internal/storage/sqlite/child_id_test.go index 92f6973a..f5f9d047 100644 --- a/internal/storage/sqlite/child_id_test.go +++ b/internal/storage/sqlite/child_id_test.go @@ -197,7 +197,9 @@ func TestCreateIssue_HierarchicalID_ParentNotExists(t *testing.T) { if err == nil { t.Errorf("expected error for child without parent, got nil") } - if err != nil && err.Error() != "parent issue bd-nonexistent does not exist" { - t.Errorf("unexpected error message: %v", err) + // With resurrection feature, error message includes JSONL history check + expectedErr := "parent issue bd-nonexistent does not exist and could not be resurrected from JSONL history" + if err != nil && err.Error() != expectedErr { + t.Errorf("unexpected error message: got %q, want %q", err.Error(), expectedErr) } } diff --git a/internal/storage/sqlite/ids.go b/internal/storage/sqlite/ids.go index 0acbd871..60136cb2 100644 --- a/internal/storage/sqlite/ids.go +++ b/internal/storage/sqlite/ids.go @@ -189,17 +189,18 @@ func (s *SQLiteStorage) EnsureIDs(ctx context.Context, conn *sql.Conn, prefix st // For hierarchical IDs (bd-a3f8e9.1), ensure parent exists if strings.Contains(issues[i].ID, ".") { // Try to resurrect entire parent chain if any parents are missing - resurrected, err := s.TryResurrectParentChain(ctx, issues[i].ID) + // Use the conn-based version to participate in the same transaction + resurrected, err := s.tryResurrectParentChainWithConn(ctx, conn, issues[i].ID) if err != nil { return fmt.Errorf("failed to resurrect parent chain for %s: %w", issues[i].ID, err) } if !resurrected { - // Parent(s) not found in JSONL history - cannot proceed + // Parent(s) not found in JSONL history - cannot proceed lastDot := strings.LastIndex(issues[i].ID, ".") - parentID := issues[i].ID[:lastDot] + parentID := issues[i].ID[:lastDot] return fmt.Errorf("parent issue %s does not exist and could not be resurrected from JSONL history", parentID) - } - } + } + } usedIDs[issues[i].ID] = true } diff --git a/internal/storage/sqlite/resurrection.go b/internal/storage/sqlite/resurrection.go index 607d98b5..d05acfc0 100644 --- a/internal/storage/sqlite/resurrection.go +++ b/internal/storage/sqlite/resurrection.go @@ -3,6 +3,7 @@ package sqlite import ( "bufio" "context" + "database/sql" "encoding/json" "fmt" "os" @@ -24,9 +25,22 @@ import ( // - false if parent was not found in JSONL history // - error if resurrection failed for any other reason func (s *SQLiteStorage) TryResurrectParent(ctx context.Context, parentID string) (bool, error) { + // Get a connection for the entire resurrection operation + conn, err := s.db.Conn(ctx) + if err != nil { + return false, fmt.Errorf("failed to get database connection: %w", err) + } + defer conn.Close() + + return s.tryResurrectParentWithConn(ctx, conn, parentID) +} + +// tryResurrectParentWithConn is the internal version that accepts an existing connection. +// This allows resurrection to participate in an existing transaction. +func (s *SQLiteStorage) tryResurrectParentWithConn(ctx context.Context, conn *sql.Conn, parentID string) (bool, error) { // First check if parent already exists in database var count int - err := s.db.QueryRowContext(ctx, `SELECT COUNT(*) FROM issues WHERE id = ?`, parentID).Scan(&count) + err := conn.QueryRowContext(ctx, `SELECT COUNT(*) FROM issues WHERE id = ?`, parentID).Scan(&count) if err != nil { return false, fmt.Errorf("failed to check parent existence: %w", err) } @@ -63,14 +77,7 @@ func (s *SQLiteStorage) TryResurrectParent(ctx context.Context, parentID string) tombstone.Description = fmt.Sprintf("%s\n\nOriginal description:\n%s", tombstone.Description, parentIssue.Description) } - // Get a connection for the transaction - conn, err := s.db.Conn(ctx) - if err != nil { - return false, fmt.Errorf("failed to get database connection: %w", err) - } - defer conn.Close() - - // Insert tombstone into database + // Insert tombstone into database using the provided connection if err := insertIssue(ctx, conn, tombstone); err != nil { return false, fmt.Errorf("failed to create tombstone for parent %s: %w", parentID, err) } @@ -80,9 +87,9 @@ func (s *SQLiteStorage) TryResurrectParent(ctx context.Context, parentID string) for _, dep := range parentIssue.Dependencies { // Only resurrect dependencies if both source and target exist var targetCount int - err := s.db.QueryRowContext(ctx, `SELECT COUNT(*) FROM issues WHERE id = ?`, dep.DependsOnID).Scan(&targetCount) + err := conn.QueryRowContext(ctx, `SELECT COUNT(*) FROM issues WHERE id = ?`, dep.DependsOnID).Scan(&targetCount) if err == nil && targetCount > 0 { - _, err := s.db.ExecContext(ctx, ` + _, err := conn.ExecContext(ctx, ` INSERT OR IGNORE INTO dependencies (issue_id, depends_on_id, type, created_by) VALUES (?, ?, ?, ?) `, parentID, dep.DependsOnID, dep.Type, "resurrection") @@ -170,12 +177,24 @@ func (s *SQLiteStorage) findIssueInJSONL(issueID string) (*types.Issue, error) { // - false if any parent in the chain was not found in JSONL history // - error if resurrection failed for any other reason func (s *SQLiteStorage) TryResurrectParentChain(ctx context.Context, childID string) (bool, error) { + // Get a connection for the entire chain resurrection + conn, err := s.db.Conn(ctx) + if err != nil { + return false, fmt.Errorf("failed to get database connection: %w", err) + } + defer conn.Close() + + return s.tryResurrectParentChainWithConn(ctx, conn, childID) +} + +// tryResurrectParentChainWithConn is the internal version that accepts an existing connection. +func (s *SQLiteStorage) tryResurrectParentChainWithConn(ctx context.Context, conn *sql.Conn, childID string) (bool, error) { // Extract all parent IDs from the hierarchical chain parents := extractParentChain(childID) // Resurrect from root to leaf (shallower to deeper) for _, parentID := range parents { - resurrected, err := s.TryResurrectParent(ctx, parentID) + resurrected, err := s.tryResurrectParentWithConn(ctx, conn, parentID) if err != nil { return false, fmt.Errorf("failed to resurrect parent %s: %w", parentID, err) } diff --git a/internal/storage/sqlite/sqlite.go b/internal/storage/sqlite/sqlite.go index 7a581e26..b0dc90fb 100644 --- a/internal/storage/sqlite/sqlite.go +++ b/internal/storage/sqlite/sqlite.go @@ -182,17 +182,18 @@ func (s *SQLiteStorage) CreateIssue(ctx context.Context, issue *types.Issue, act // For hierarchical IDs (bd-a3f8e9.1), ensure parent exists if strings.Contains(issue.ID, ".") { // Try to resurrect entire parent chain if any parents are missing - resurrected, err := s.TryResurrectParentChain(ctx, issue.ID) + // Use the conn-based version to participate in the same transaction + resurrected, err := s.tryResurrectParentChainWithConn(ctx, conn, issue.ID) if err != nil { return fmt.Errorf("failed to resurrect parent chain for %s: %w", issue.ID, err) } if !resurrected { - // Parent(s) not found in JSONL history - cannot proceed + // Parent(s) not found in JSONL history - cannot proceed lastDot := strings.LastIndex(issue.ID, ".") - parentID := issue.ID[:lastDot] + parentID := issue.ID[:lastDot] return fmt.Errorf("parent issue %s does not exist and could not be resurrected from JSONL history", parentID) - } - } + } + } } // Insert issue