fix: Remove unsafe ClearDirtyIssues() method (bd-b6xo)
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 <noreply@anthropic.com>
This commit is contained in:
@@ -1313,14 +1313,6 @@ func (m *MemoryStorage) GetDirtyIssues(ctx context.Context) ([]string, error) {
|
|||||||
return dirtyIDs, nil
|
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 {
|
func (m *MemoryStorage) ClearDirtyIssuesByID(ctx context.Context, issueIDs []string) error {
|
||||||
m.mu.Lock()
|
m.mu.Lock()
|
||||||
defer m.mu.Unlock()
|
defer m.mu.Unlock()
|
||||||
|
|||||||
@@ -698,8 +698,8 @@ func TestDirtyTracking(t *testing.T) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
// Clear dirty
|
// Clear dirty
|
||||||
if err := store.ClearDirtyIssues(ctx); err != nil {
|
if err := store.ClearDirtyIssuesByID(ctx, dirty); err != nil {
|
||||||
t.Fatalf("ClearDirtyIssues failed: %v", err)
|
t.Fatalf("ClearDirtyIssuesByID failed: %v", err)
|
||||||
}
|
}
|
||||||
|
|
||||||
dirty, err = store.GetDirtyIssues(ctx)
|
dirty, err = store.GetDirtyIssues(ctx)
|
||||||
|
|||||||
@@ -95,19 +95,6 @@ func (s *SQLiteStorage) GetDirtyIssueHash(ctx context.Context, issueID string) (
|
|||||||
return hash.String, nil
|
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
|
// ClearDirtyIssuesByID removes specific issue IDs from the dirty_issues table
|
||||||
// This avoids race conditions by only clearing issues that were actually exported
|
// This avoids race conditions by only clearing issues that were actually exported
|
||||||
func (s *SQLiteStorage) ClearDirtyIssuesByID(ctx context.Context, issueIDs []string) error {
|
func (s *SQLiteStorage) ClearDirtyIssuesByID(ctx context.Context, issueIDs []string) error {
|
||||||
|
|||||||
@@ -39,9 +39,9 @@ func TestMarkIssueDirty(t *testing.T) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
// Clear dirty issues
|
// Clear dirty issues
|
||||||
err = store.ClearDirtyIssues(ctx)
|
err = store.ClearDirtyIssuesByID(ctx, []string{issue.ID})
|
||||||
if err != nil {
|
if err != nil {
|
||||||
t.Fatalf("ClearDirtyIssues failed: %v", err)
|
t.Fatalf("ClearDirtyIssuesByID failed: %v", err)
|
||||||
}
|
}
|
||||||
|
|
||||||
// Verify cleared
|
// Verify cleared
|
||||||
@@ -92,9 +92,9 @@ func TestMarkIssuesDirty(t *testing.T) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
// Clear all dirty issues
|
// Clear all dirty issues
|
||||||
err := store.ClearDirtyIssues(ctx)
|
err := store.ClearDirtyIssuesByID(ctx, issueIDs)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
t.Fatalf("ClearDirtyIssues failed: %v", err)
|
t.Fatalf("ClearDirtyIssuesByID failed: %v", err)
|
||||||
}
|
}
|
||||||
|
|
||||||
// Mark multiple issues dirty at once
|
// Mark multiple issues dirty at once
|
||||||
|
|||||||
@@ -212,9 +212,9 @@ func TestAddCommentMarksDirty(t *testing.T) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
// Clear dirty issues
|
// Clear dirty issues
|
||||||
err = store.ClearDirtyIssues(ctx)
|
err = store.ClearDirtyIssuesByID(ctx, []string{issue.ID})
|
||||||
if err != nil {
|
if err != nil {
|
||||||
t.Fatalf("ClearDirtyIssues failed: %v", err)
|
t.Fatalf("ClearDirtyIssuesByID failed: %v", err)
|
||||||
}
|
}
|
||||||
|
|
||||||
// Add comment - should mark issue dirty
|
// Add comment - should mark issue dirty
|
||||||
|
|||||||
@@ -351,9 +351,9 @@ func TestLabelMarksDirty(t *testing.T) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
// Clear dirty issues
|
// Clear dirty issues
|
||||||
err = store.ClearDirtyIssues(ctx)
|
err = store.ClearDirtyIssuesByID(ctx, []string{issue.ID})
|
||||||
if err != nil {
|
if err != nil {
|
||||||
t.Fatalf("ClearDirtyIssues failed: %v", err)
|
t.Fatalf("ClearDirtyIssuesByID failed: %v", err)
|
||||||
}
|
}
|
||||||
|
|
||||||
// Add label - should mark issue dirty
|
// Add label - should mark issue dirty
|
||||||
@@ -372,9 +372,9 @@ func TestLabelMarksDirty(t *testing.T) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
// Clear dirty again
|
// Clear dirty again
|
||||||
err = store.ClearDirtyIssues(ctx)
|
err = store.ClearDirtyIssuesByID(ctx, []string{issue.ID})
|
||||||
if err != nil {
|
if err != nil {
|
||||||
t.Fatalf("ClearDirtyIssues failed: %v", err)
|
t.Fatalf("ClearDirtyIssuesByID failed: %v", err)
|
||||||
}
|
}
|
||||||
|
|
||||||
// Remove label - should mark issue dirty
|
// Remove label - should mark issue dirty
|
||||||
|
|||||||
@@ -126,7 +126,6 @@ type Storage interface {
|
|||||||
// Dirty tracking (for incremental JSONL export)
|
// Dirty tracking (for incremental JSONL export)
|
||||||
GetDirtyIssues(ctx context.Context) ([]string, error)
|
GetDirtyIssues(ctx context.Context) ([]string, error)
|
||||||
GetDirtyIssueHash(ctx context.Context, issueID string) (string, error) // For timestamp-only dedup (bd-164)
|
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
|
ClearDirtyIssuesByID(ctx context.Context, issueIDs []string) error
|
||||||
|
|
||||||
// Export hash tracking (for timestamp-only dedup, bd-164)
|
// Export hash tracking (for timestamp-only dedup, bd-164)
|
||||||
|
|||||||
Reference in New Issue
Block a user