From 92759710de1a13b52748faadd4faed6a0ef6660f Mon Sep 17 00:00:00 2001 From: Steve Yegge Date: Tue, 14 Oct 2025 00:29:23 -0700 Subject: [PATCH] Fix race condition in dirty issue tracking (bd-52, bd-53) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix critical TOCTOU bug where concurrent operations could lose dirty issue tracking, causing data loss in incremental exports. Also fixes bug where export with filters would incorrectly clear all dirty issues. The Problem: 1. GetDirtyIssues() returns [bd-1, bd-2] 2. Concurrent CRUD marks bd-3 dirty 3. Export writes bd-1, bd-2 4. ClearDirtyIssues() deletes ALL (including bd-3) 5. Result: bd-3 never gets exported! The Fix: - Add ClearDirtyIssuesByID() that only clears specific issue IDs - Track which issues were actually exported - Clear only those specific IDs, not all dirty issues - Fixes both race condition and filter export bug Changes: - internal/storage/sqlite/dirty.go: * Add ClearDirtyIssuesByID() method * Add warning to ClearDirtyIssues() about race condition - internal/storage/storage.go: * Add ClearDirtyIssuesByID to interface - cmd/bd/main.go: * Update auto-flush to use ClearDirtyIssuesByID() - cmd/bd/export.go: * Track exported issue IDs * Use ClearDirtyIssuesByID() instead of ClearDirtyIssues() Testing: - Created test-1, test-2, test-3 (all dirty) - Updated test-2 to in_progress - Exported with --status open filter (exports only test-1, test-3) - Verified only test-2 remains dirty ✓ - All existing tests pass ✓ Impact: - Race condition eliminated - concurrent operations are safe - Export with filters now works correctly - No data loss from competing writes Closes bd-52, bd-53 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- cmd/bd/export.go | 6 ++++-- cmd/bd/main.go | 4 ++-- internal/storage/sqlite/dirty.go | 31 +++++++++++++++++++++++++++++++ internal/storage/storage.go | 3 ++- 4 files changed, 39 insertions(+), 5 deletions(-) diff --git a/cmd/bd/export.go b/cmd/bd/export.go index 302e345a..439b7cf0 100644 --- a/cmd/bd/export.go +++ b/cmd/bd/export.go @@ -76,18 +76,20 @@ Output to stdout by default, or use -o flag for file output.`, // Write JSONL encoder := json.NewEncoder(out) + exportedIDs := make([]string, 0, len(issues)) for _, issue := range issues { if err := encoder.Encode(issue); err != nil { fmt.Fprintf(os.Stderr, "Error encoding issue %s: %v\n", issue.ID, err) os.Exit(1) } + exportedIDs = append(exportedIDs, issue.ID) } // Only clear dirty issues and auto-flush state if exporting to the default JSONL path // This prevents clearing dirty flags when exporting to custom paths (e.g., bd export -o backup.jsonl) if output == "" || output == findJSONLPath() { - // Clear dirty issues since we just exported to the canonical JSONL file - if err := store.ClearDirtyIssues(ctx); err != nil { + // Clear only the issues that were actually exported (fixes bd-52 race condition) + if err := store.ClearDirtyIssuesByID(ctx, exportedIDs); err != nil { fmt.Fprintf(os.Stderr, "Warning: failed to clear dirty issues: %v\n", err) } diff --git a/cmd/bd/main.go b/cmd/bd/main.go index 8a0cdf06..4b1c4308 100644 --- a/cmd/bd/main.go +++ b/cmd/bd/main.go @@ -516,8 +516,8 @@ func flushToJSONL() { return } - // Clear dirty issues after successful export - if err := store.ClearDirtyIssues(ctx); err != nil { + // Clear only the dirty issues that were actually exported (fixes bd-52 race condition) + if err := store.ClearDirtyIssuesByID(ctx, dirtyIDs); err != nil { // Don't fail the whole flush for this, but warn fmt.Fprintf(os.Stderr, "Warning: failed to clear dirty issues: %v\n", err) } diff --git a/internal/storage/sqlite/dirty.go b/internal/storage/sqlite/dirty.go index 4f8e52ea..a612a89e 100644 --- a/internal/storage/sqlite/dirty.go +++ b/internal/storage/sqlite/dirty.go @@ -77,6 +77,9 @@ func (s *SQLiteStorage) GetDirtyIssues(ctx context.Context) ([]string, error) { // 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 { @@ -85,6 +88,34 @@ func (s *SQLiteStorage) ClearDirtyIssues(ctx context.Context) error { 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 { + if len(issueIDs) == 0 { + return nil + } + + tx, err := s.db.BeginTx(ctx, nil) + if err != nil { + return fmt.Errorf("failed to begin transaction: %w", err) + } + defer tx.Rollback() + + stmt, err := tx.PrepareContext(ctx, `DELETE FROM dirty_issues WHERE issue_id = ?`) + if err != nil { + return fmt.Errorf("failed to prepare statement: %w", err) + } + defer stmt.Close() + + for _, issueID := range issueIDs { + if _, err := stmt.ExecContext(ctx, issueID); err != nil { + return fmt.Errorf("failed to clear dirty issue %s: %w", issueID, err) + } + } + + return tx.Commit() +} + // GetDirtyIssueCount returns the count of dirty issues (for monitoring/debugging) func (s *SQLiteStorage) GetDirtyIssueCount(ctx context.Context) (int, error) { var count int diff --git a/internal/storage/storage.go b/internal/storage/storage.go index 0aab6450..1b3b4c96 100644 --- a/internal/storage/storage.go +++ b/internal/storage/storage.go @@ -45,7 +45,8 @@ type Storage interface { // Dirty tracking (for incremental JSONL export) GetDirtyIssues(ctx context.Context) ([]string, error) - ClearDirtyIssues(ctx context.Context) error + ClearDirtyIssues(ctx context.Context) error // WARNING: Race condition (bd-52), use ClearDirtyIssuesByID + ClearDirtyIssuesByID(ctx context.Context, issueIDs []string) error // Config SetConfig(ctx context.Context, key, value string) error