From 5869adadf267d70f9b1bd69f00582aefa167085e Mon Sep 17 00:00:00 2001 From: Steve Yegge Date: Wed, 17 Dec 2025 19:10:13 -0800 Subject: [PATCH] fix: Remove unsafe ClearDirtyIssues() method (bd-b6xo) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove ClearDirtyIssues() which had a race condition that could lose dirty issues if export failed partway through. All callers now use ClearDirtyIssuesByID() which only clears specific exported issues. - Remove from Storage interface - Remove from SQLite and Memory implementations - Update 6 test call sites to use ClearDirtyIssuesByID() 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- internal/storage/memory/memory.go | 8 -------- internal/storage/memory/memory_test.go | 4 ++-- internal/storage/sqlite/dirty.go | 13 ------------- internal/storage/sqlite/dirty_test.go | 8 ++++---- internal/storage/sqlite/events_test.go | 4 ++-- internal/storage/sqlite/labels_test.go | 8 ++++---- internal/storage/storage.go | 1 - 7 files changed, 12 insertions(+), 34 deletions(-) diff --git a/internal/storage/memory/memory.go b/internal/storage/memory/memory.go index b73e3fe2..ccf53d3d 100644 --- a/internal/storage/memory/memory.go +++ b/internal/storage/memory/memory.go @@ -1313,14 +1313,6 @@ func (m *MemoryStorage) GetDirtyIssues(ctx context.Context) ([]string, error) { return dirtyIDs, nil } -func (m *MemoryStorage) ClearDirtyIssues(ctx context.Context) error { - m.mu.Lock() - defer m.mu.Unlock() - - m.dirty = make(map[string]bool) - return nil -} - func (m *MemoryStorage) ClearDirtyIssuesByID(ctx context.Context, issueIDs []string) error { m.mu.Lock() defer m.mu.Unlock() diff --git a/internal/storage/memory/memory_test.go b/internal/storage/memory/memory_test.go index ecbe1063..b613656c 100644 --- a/internal/storage/memory/memory_test.go +++ b/internal/storage/memory/memory_test.go @@ -698,8 +698,8 @@ func TestDirtyTracking(t *testing.T) { } // Clear dirty - if err := store.ClearDirtyIssues(ctx); err != nil { - t.Fatalf("ClearDirtyIssues failed: %v", err) + if err := store.ClearDirtyIssuesByID(ctx, dirty); err != nil { + t.Fatalf("ClearDirtyIssuesByID failed: %v", err) } dirty, err = store.GetDirtyIssues(ctx) diff --git a/internal/storage/sqlite/dirty.go b/internal/storage/sqlite/dirty.go index b1414021..9eb5e0a0 100644 --- a/internal/storage/sqlite/dirty.go +++ b/internal/storage/sqlite/dirty.go @@ -95,19 +95,6 @@ func (s *SQLiteStorage) GetDirtyIssueHash(ctx context.Context, issueID string) ( return hash.String, nil } -// ClearDirtyIssues removes all entries from the dirty_issues table -// This should be called after a successful JSONL export -// -// WARNING: This has a race condition (bd-52). Use ClearDirtyIssuesByID instead -// to only clear specific issues that were actually exported. -func (s *SQLiteStorage) ClearDirtyIssues(ctx context.Context) error { - _, err := s.db.ExecContext(ctx, `DELETE FROM dirty_issues`) - if err != nil { - return fmt.Errorf("failed to clear dirty issues: %w", err) - } - return nil -} - // ClearDirtyIssuesByID removes specific issue IDs from the dirty_issues table // This avoids race conditions by only clearing issues that were actually exported func (s *SQLiteStorage) ClearDirtyIssuesByID(ctx context.Context, issueIDs []string) error { diff --git a/internal/storage/sqlite/dirty_test.go b/internal/storage/sqlite/dirty_test.go index fadc415a..d8863e56 100644 --- a/internal/storage/sqlite/dirty_test.go +++ b/internal/storage/sqlite/dirty_test.go @@ -39,9 +39,9 @@ func TestMarkIssueDirty(t *testing.T) { } // Clear dirty issues - err = store.ClearDirtyIssues(ctx) + err = store.ClearDirtyIssuesByID(ctx, []string{issue.ID}) if err != nil { - t.Fatalf("ClearDirtyIssues failed: %v", err) + t.Fatalf("ClearDirtyIssuesByID failed: %v", err) } // Verify cleared @@ -92,9 +92,9 @@ func TestMarkIssuesDirty(t *testing.T) { } // Clear all dirty issues - err := store.ClearDirtyIssues(ctx) + err := store.ClearDirtyIssuesByID(ctx, issueIDs) if err != nil { - t.Fatalf("ClearDirtyIssues failed: %v", err) + t.Fatalf("ClearDirtyIssuesByID failed: %v", err) } // Mark multiple issues dirty at once diff --git a/internal/storage/sqlite/events_test.go b/internal/storage/sqlite/events_test.go index af772b90..cda9f273 100644 --- a/internal/storage/sqlite/events_test.go +++ b/internal/storage/sqlite/events_test.go @@ -212,9 +212,9 @@ func TestAddCommentMarksDirty(t *testing.T) { } // Clear dirty issues - err = store.ClearDirtyIssues(ctx) + err = store.ClearDirtyIssuesByID(ctx, []string{issue.ID}) if err != nil { - t.Fatalf("ClearDirtyIssues failed: %v", err) + t.Fatalf("ClearDirtyIssuesByID failed: %v", err) } // Add comment - should mark issue dirty diff --git a/internal/storage/sqlite/labels_test.go b/internal/storage/sqlite/labels_test.go index 614192b3..1582377a 100644 --- a/internal/storage/sqlite/labels_test.go +++ b/internal/storage/sqlite/labels_test.go @@ -351,9 +351,9 @@ func TestLabelMarksDirty(t *testing.T) { } // Clear dirty issues - err = store.ClearDirtyIssues(ctx) + err = store.ClearDirtyIssuesByID(ctx, []string{issue.ID}) if err != nil { - t.Fatalf("ClearDirtyIssues failed: %v", err) + t.Fatalf("ClearDirtyIssuesByID failed: %v", err) } // Add label - should mark issue dirty @@ -372,9 +372,9 @@ func TestLabelMarksDirty(t *testing.T) { } // Clear dirty again - err = store.ClearDirtyIssues(ctx) + err = store.ClearDirtyIssuesByID(ctx, []string{issue.ID}) if err != nil { - t.Fatalf("ClearDirtyIssues failed: %v", err) + t.Fatalf("ClearDirtyIssuesByID failed: %v", err) } // Remove label - should mark issue dirty diff --git a/internal/storage/storage.go b/internal/storage/storage.go index 5df227c9..a4ff736f 100644 --- a/internal/storage/storage.go +++ b/internal/storage/storage.go @@ -126,7 +126,6 @@ type Storage interface { // Dirty tracking (for incremental JSONL export) GetDirtyIssues(ctx context.Context) ([]string, error) GetDirtyIssueHash(ctx context.Context, issueID string) (string, error) // For timestamp-only dedup (bd-164) - ClearDirtyIssues(ctx context.Context) error // WARNING: Race condition (bd-52), use ClearDirtyIssuesByID ClearDirtyIssuesByID(ctx context.Context, issueIDs []string) error // Export hash tracking (for timestamp-only dedup, bd-164)