From e8a4474788138b43d67d1c373b91b53cf7f6f849 Mon Sep 17 00:00:00 2001 From: garnet Date: Mon, 12 Jan 2026 19:41:34 -0800 Subject: [PATCH] fix(storage): use strict INSERT for batch issue creation (GH#956) Add insertIssuesStrict function that uses plain INSERT instead of INSERT OR IGNORE. Update bulkInsertIssues and transactional CreateIssues to use the strict variant. This fixes a race condition where INSERT OR IGNORE could silently skip duplicate insertions, but the code would still attempt to record events for those "inserted" issues, causing FOREIGN KEY constraint failures. The strict INSERT will now fail explicitly if a duplicate is encountered, which should never happen since checkForExistingIDs runs first within the same transaction. Co-Authored-By: Claude Opus 4.5 --- internal/storage/sqlite/batch_ops.go | 5 +- internal/storage/sqlite/issues.go | 65 ++++++++++++++++++++++++++ internal/storage/sqlite/transaction.go | 9 ++-- 3 files changed, 73 insertions(+), 6 deletions(-) diff --git a/internal/storage/sqlite/batch_ops.go b/internal/storage/sqlite/batch_ops.go index a3e13631..2505638c 100644 --- a/internal/storage/sqlite/batch_ops.go +++ b/internal/storage/sqlite/batch_ops.go @@ -87,9 +87,10 @@ func (s *SQLiteStorage) generateBatchIDs(ctx context.Context, conn *sql.Conn, is return nil } -// bulkInsertIssues delegates to insertIssues helper +// bulkInsertIssues delegates to insertIssuesStrict helper for fresh issue creation. +// GH#956: Using strict insert prevents FK constraint errors from silent INSERT OR IGNORE failures. func bulkInsertIssues(ctx context.Context, conn *sql.Conn, issues []*types.Issue) error { - return insertIssues(ctx, conn, issues) + return insertIssuesStrict(ctx, conn, issues) } // bulkRecordEvents delegates to recordCreatedEvents helper diff --git a/internal/storage/sqlite/issues.go b/internal/storage/sqlite/issues.go index c59fe8d4..473870bc 100644 --- a/internal/storage/sqlite/issues.go +++ b/internal/storage/sqlite/issues.go @@ -204,3 +204,68 @@ func insertIssues(ctx context.Context, conn *sql.Conn, issues []*types.Issue) er } return nil } + +// insertIssuesStrict bulk inserts multiple issues using plain INSERT (no OR IGNORE). +// This is used for fresh batch issue creation (CreateIssues) where duplicates indicate a bug. +// For imports where duplicates are expected, use insertIssues instead. +// GH#956: Using plain INSERT prevents FK constraint errors from silent INSERT OR IGNORE failures. +func insertIssuesStrict(ctx context.Context, conn *sql.Conn, issues []*types.Issue) error { + stmt, err := conn.PrepareContext(ctx, ` + INSERT INTO issues ( + id, content_hash, title, description, design, acceptance_criteria, notes, + status, priority, issue_type, assignee, estimated_minutes, + created_at, created_by, owner, updated_at, closed_at, external_ref, source_repo, close_reason, + deleted_at, deleted_by, delete_reason, original_type, + sender, ephemeral, pinned, is_template, crystallizes, + await_type, await_id, timeout_ns, waiters, mol_type, + event_kind, actor, target, payload, + due_at, defer_until + ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + `) + if err != nil { + return fmt.Errorf("failed to prepare statement: %w", err) + } + defer func() { _ = stmt.Close() }() + + for _, issue := range issues { + 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 + } + crystallizes := 0 + if issue.Crystallizes { + crystallizes = 1 + } + + _, err = stmt.ExecContext(ctx, + 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.Owner, issue.UpdatedAt, + issue.ClosedAt, issue.ExternalRef, sourceRepo, issue.CloseReason, + issue.DeletedAt, issue.DeletedBy, issue.DeleteReason, issue.OriginalType, + issue.Sender, wisp, pinned, isTemplate, crystallizes, + 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 %s: %w", issue.ID, err) + } + } + return nil +} diff --git a/internal/storage/sqlite/transaction.go b/internal/storage/sqlite/transaction.go index 7474e860..31b30c71 100644 --- a/internal/storage/sqlite/transaction.go +++ b/internal/storage/sqlite/transaction.go @@ -297,14 +297,15 @@ func (t *sqliteTxStorage) CreateIssues(ctx context.Context, issues []*types.Issu } // GH#956: Check for conflicts with existing IDs in database before inserting. - // This prevents INSERT OR IGNORE from silently skipping duplicates, which would - // cause FK constraint failures when recording events for non-inserted issues. + // This prevents duplicates from causing FK constraint failures when recording events. if err := checkForExistingIDs(ctx, t.conn, issues); err != nil { return err } - // Insert all issues - if err := insertIssues(ctx, t.conn, issues); err != nil { + // Insert all issues using strict mode (fails on duplicates) + // GH#956: Use insertIssuesStrict instead of insertIssues to prevent FK constraint errors + // from silent INSERT OR IGNORE failures under concurrent load. + if err := insertIssuesStrict(ctx, t.conn, issues); err != nil { return fmt.Errorf("failed to insert issues: %w", err) }