From 0777bb907c1a4f4e2a66e15db16cf0b5ae761f73 Mon Sep 17 00:00:00 2001 From: wolf Date: Fri, 9 Jan 2026 22:58:46 -0800 Subject: [PATCH] fix: prevent FK constraint errors on concurrent issue creation (GH#956) Root cause: CreateIssue used INSERT OR IGNORE which could silently skip the insert (e.g., on duplicate ID from hash collision), then fail with FOREIGN KEY constraint error when trying to record the creation event. Fix: Add insertIssueStrict() that uses plain INSERT (fails on duplicates) and use it for CreateIssue in both queries.go and transaction.go. The existing insertIssue() with INSERT OR IGNORE is preserved for import scenarios where duplicates are expected. Added test TestCreateIssueDuplicateID to verify duplicate IDs are properly rejected instead of silently ignored. Co-Authored-By: Claude Opus 4.5 --- internal/storage/sqlite/issues.go | 57 +++++++++++++++++++++++++- internal/storage/sqlite/queries.go | 6 ++- internal/storage/sqlite/sqlite_test.go | 43 +++++++++++++++++++ internal/storage/sqlite/transaction.go | 6 ++- 4 files changed, 107 insertions(+), 5 deletions(-) diff --git a/internal/storage/sqlite/issues.go b/internal/storage/sqlite/issues.go index f97164ea..92ef06de 100644 --- a/internal/storage/sqlite/issues.go +++ b/internal/storage/sqlite/issues.go @@ -20,7 +20,9 @@ func isUniqueConstraintError(err error) bool { strings.Contains(errMsg, "constraint failed: UNIQUE") } -// insertIssue inserts a single issue into the database +// insertIssue inserts a single issue into the database. +// Uses INSERT OR IGNORE for backward compatibility with imports. +// For fresh issue creation, use insertIssueStrict instead. func insertIssue(ctx context.Context, conn *sql.Conn, issue *types.Issue) error { sourceRepo := issue.SourceRepo if sourceRepo == "" { @@ -75,6 +77,59 @@ func insertIssue(ctx context.Context, conn *sql.Conn, issue *types.Issue) error return nil } +// insertIssueStrict inserts a single issue into the database, failing on duplicates. +// This is used for fresh issue creation (CreateIssue) where duplicates indicate a bug. +// For imports where duplicates are expected, use insertIssue instead. +// GH#956: Using plain INSERT prevents FK constraint errors from silent INSERT OR IGNORE failures. +func insertIssueStrict(ctx context.Context, conn *sql.Conn, issue *types.Issue) error { + sourceRepo := issue.SourceRepo + if sourceRepo == "" { + sourceRepo = "." // Default to primary repo + } + + wisp := 0 + if issue.Ephemeral { + wisp = 1 + } + pinned := 0 + if issue.Pinned { + pinned = 1 + } + isTemplate := 0 + if issue.IsTemplate { + isTemplate = 1 + } + + _, err := conn.ExecContext(ctx, ` + INSERT INTO issues ( + id, content_hash, title, description, design, acceptance_criteria, notes, + status, priority, issue_type, assignee, estimated_minutes, + created_at, created_by, updated_at, closed_at, external_ref, source_repo, close_reason, + deleted_at, deleted_by, delete_reason, original_type, + sender, ephemeral, pinned, is_template, + await_type, await_id, timeout_ns, waiters, mol_type, + event_kind, actor, target, payload, + due_at, defer_until + ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + `, + issue.ID, issue.ContentHash, issue.Title, issue.Description, issue.Design, + issue.AcceptanceCriteria, issue.Notes, issue.Status, + issue.Priority, issue.IssueType, issue.Assignee, + issue.EstimatedMinutes, issue.CreatedAt, issue.CreatedBy, issue.UpdatedAt, + issue.ClosedAt, issue.ExternalRef, sourceRepo, issue.CloseReason, + issue.DeletedAt, issue.DeletedBy, issue.DeleteReason, issue.OriginalType, + issue.Sender, wisp, pinned, isTemplate, + issue.AwaitType, issue.AwaitID, int64(issue.Timeout), formatJSONStringArray(issue.Waiters), + string(issue.MolType), + issue.EventKind, issue.Actor, issue.Target, issue.Payload, + issue.DueAt, issue.DeferUntil, + ) + if err != nil { + return fmt.Errorf("failed to insert issue: %w", err) + } + return nil +} + // insertIssues bulk inserts multiple issues using a prepared statement func insertIssues(ctx context.Context, conn *sql.Conn, issues []*types.Issue) error { stmt, err := conn.PrepareContext(ctx, ` diff --git a/internal/storage/sqlite/queries.go b/internal/storage/sqlite/queries.go index b665cd29..87dbf0c5 100644 --- a/internal/storage/sqlite/queries.go +++ b/internal/storage/sqlite/queries.go @@ -212,8 +212,10 @@ func (s *SQLiteStorage) CreateIssue(ctx context.Context, issue *types.Issue, act } } - // Insert issue - if err := insertIssue(ctx, conn, issue); err != nil { + // Insert issue using strict mode (fails on duplicates) + // GH#956: Use insertIssueStrict instead of insertIssue to prevent FK constraint errors + // from silent INSERT OR IGNORE failures under concurrent load. + if err := insertIssueStrict(ctx, conn, issue); err != nil { return wrapDBError("insert issue", err) } diff --git a/internal/storage/sqlite/sqlite_test.go b/internal/storage/sqlite/sqlite_test.go index b0bcdf9e..dd4642de 100644 --- a/internal/storage/sqlite/sqlite_test.go +++ b/internal/storage/sqlite/sqlite_test.go @@ -4,6 +4,7 @@ import ( "context" "os" "path/filepath" + "strings" "testing" "time" @@ -139,6 +140,48 @@ func TestCreateIssueValidation(t *testing.T) { } } +// TestCreateIssueDuplicateID verifies that CreateIssue properly rejects duplicate IDs. +// GH#956: This test ensures that insertIssueStrict (used by CreateIssue) properly fails +// on duplicate IDs instead of silently ignoring them (which would cause FK constraint +// errors when recording the creation event). +func TestCreateIssueDuplicateID(t *testing.T) { + store, cleanup := setupTestDB(t) + defer cleanup() + + ctx := context.Background() + + // Create first issue + issue1 := &types.Issue{ + Title: "First issue", + Status: types.StatusOpen, + Priority: 2, + IssueType: types.TypeTask, + } + err := store.CreateIssue(ctx, issue1, "test-user") + if err != nil { + t.Fatalf("CreateIssue failed for first issue: %v", err) + } + + // Try to create second issue with same explicit ID - should fail + issue2 := &types.Issue{ + ID: issue1.ID, // Use same ID as first issue + Title: "Second issue with duplicate ID", + Status: types.StatusOpen, + Priority: 2, + IssueType: types.TypeTask, + } + err = store.CreateIssue(ctx, issue2, "test-user") + if err == nil { + t.Error("CreateIssue should have failed for duplicate ID, but succeeded") + } + + // Verify the error mentions constraint or duplicate + errStr := err.Error() + if !strings.Contains(errStr, "UNIQUE constraint") && !strings.Contains(errStr, "already exists") { + t.Errorf("Expected error to mention constraint or duplicate, got: %v", err) + } +} + func TestGetIssue(t *testing.T) { store, cleanup := setupTestDB(t) defer cleanup() diff --git a/internal/storage/sqlite/transaction.go b/internal/storage/sqlite/transaction.go index 919dae39..9e058914 100644 --- a/internal/storage/sqlite/transaction.go +++ b/internal/storage/sqlite/transaction.go @@ -187,8 +187,10 @@ func (t *sqliteTxStorage) CreateIssue(ctx context.Context, issue *types.Issue, a } } - // Insert issue - if err := insertIssue(ctx, t.conn, issue); err != nil { + // Insert issue using strict mode (fails on duplicates) + // GH#956: Use insertIssueStrict instead of insertIssue to prevent FK constraint errors + // from silent INSERT OR IGNORE failures under concurrent load. + if err := insertIssueStrict(ctx, t.conn, issue); err != nil { return fmt.Errorf("failed to insert issue: %w", err) }