diff --git a/cmd/bd/autoflush.go b/cmd/bd/autoflush.go index 2f7eba50..fddd5225 100644 --- a/cmd/bd/autoflush.go +++ b/cmd/bd/autoflush.go @@ -283,94 +283,36 @@ func autoImportIfNewer() { // Thread-safe: Safe to call from multiple goroutines (no shared mutable state). // No-op if auto-flush is disabled via --no-auto-flush flag. func markDirtyAndScheduleFlush() { - // Use FlushManager if available (new path, fixes bd-52) + // Use FlushManager if available + // No FlushManager means sandbox mode or test without flush setup - no-op is correct if flushManager != nil { flushManager.MarkDirty(false) // Incremental export - return } - - // Legacy path for backward compatibility with tests - if !autoFlushEnabled { - return - } - - flushMutex.Lock() - defer flushMutex.Unlock() - - isDirty = true - - // Cancel existing timer if any - if flushTimer != nil { - flushTimer.Stop() - flushTimer = nil - } - - // Schedule new flush - flushTimer = time.AfterFunc(getDebounceDuration(), func() { - flushToJSONL() - }) } // markDirtyAndScheduleFullExport marks DB as needing a full export (for ID-changing operations) func markDirtyAndScheduleFullExport() { - // Use FlushManager if available (new path, fixes bd-52) + // Use FlushManager if available + // No FlushManager means sandbox mode or test without flush setup - no-op is correct if flushManager != nil { flushManager.MarkDirty(true) // Full export - return } - - // Legacy path for backward compatibility with tests - if !autoFlushEnabled { - return - } - - flushMutex.Lock() - defer flushMutex.Unlock() - - isDirty = true - needsFullExport = true // Force full export, not incremental - - // Cancel existing timer if any - if flushTimer != nil { - flushTimer.Stop() - flushTimer = nil - } - - // Schedule new flush - flushTimer = time.AfterFunc(getDebounceDuration(), func() { - flushToJSONL() - }) } // clearAutoFlushState cancels pending flush and marks DB as clean (after manual export) func clearAutoFlushState() { - // With FlushManager, clearing state is unnecessary (new path) + // With FlushManager, clearing state is unnecessary // If a flush is pending and fires after manual export, flushToJSONLWithState() // will detect nothing is dirty and skip the flush. This is harmless. - if flushManager != nil { - return - } - - // Legacy path for backward compatibility with tests + // Reset failure counters on manual export success flushMutex.Lock() - defer flushMutex.Unlock() - - // Cancel pending timer - if flushTimer != nil { - flushTimer.Stop() - flushTimer = nil - } - - // Clear dirty flag - isDirty = false - - // Reset failure counter (manual export succeeded) flushFailureCount = 0 lastFlushError = nil + flushMutex.Unlock() } // writeJSONLAtomic writes issues to a JSONL file atomically using temp file + rename. -// This is the common implementation used by both flushToJSONL (SQLite mode) and +// This is the common implementation used by flushToJSONLWithState (SQLite mode) and // writeIssuesToJSONL (--no-db mode). // // Atomic write pattern: @@ -807,38 +749,6 @@ func flushToJSONLWithState(state flushState) { } } - // Success! Don't clear global flags here - let the caller manage its own state. - // FlushManager manages its local state in run() goroutine. - // Legacy path clears global state in flushToJSONL() wrapper. + // Success! FlushManager manages its local state in run() goroutine. recordSuccess() } - -// flushToJSONL is a backward-compatible wrapper that reads global state. -// New code should use FlushManager instead of calling this directly. -// -// Reads global isDirty and needsFullExport flags, then calls flushToJSONLWithState. -// Invoked by the debounce timer or immediately on command exit (for legacy code). -func flushToJSONL() { - // Read current state and failure count - flushMutex.Lock() - forceDirty := isDirty - forceFullExport := needsFullExport - beforeFailCount := flushFailureCount - flushMutex.Unlock() - - // Call new implementation - flushToJSONLWithState(flushState{ - forceDirty: forceDirty, - forceFullExport: forceFullExport, - }) - - // Clear global state only if flush succeeded (legacy path only) - // Success is indicated by failure count not increasing - flushMutex.Lock() - if flushFailureCount == beforeFailCount { - // Flush succeeded - clear dirty flags - isDirty = false - needsFullExport = false - } - flushMutex.Unlock() -} diff --git a/cmd/bd/cli_fast_test.go b/cmd/bd/cli_fast_test.go index 623a7c93..5a92261b 100644 --- a/cmd/bd/cli_fast_test.go +++ b/cmd/bd/cli_fast_test.go @@ -129,14 +129,13 @@ func runBDInProcess(t *testing.T, dir string, args ...string) string { sandboxMode = false noDb = false autoFlushEnabled = true - isDirty = false - needsFullExport = false storeActive = false flushFailureCount = 0 lastFlushError = nil - if flushTimer != nil { - flushTimer.Stop() - flushTimer = nil + // Shutdown any existing FlushManager + if flushManager != nil { + _ = flushManager.Shutdown() + flushManager = nil } // Reset context state rootCtx = nil diff --git a/cmd/bd/direct_mode_test.go b/cmd/bd/direct_mode_test.go index b7fa3c1b..e4a47662 100644 --- a/cmd/bd/direct_mode_test.go +++ b/cmd/bd/direct_mode_test.go @@ -28,17 +28,15 @@ func TestFallbackToDirectModeEnablesFlush(t *testing.T) { origDBPath := dbPath origAutoImport := autoImportEnabled origAutoFlush := autoFlushEnabled - origIsDirty := isDirty - origNeedsFull := needsFullExport origFlushFailures := flushFailureCount origLastFlushErr := lastFlushError + origFlushManager := flushManager - flushMutex.Lock() - if flushTimer != nil { - flushTimer.Stop() - flushTimer = nil + // Shutdown any existing FlushManager + if flushManager != nil { + _ = flushManager.Shutdown() + flushManager = nil } - flushMutex.Unlock() defer func() { if store != nil && store != origStore { @@ -54,17 +52,14 @@ func TestFallbackToDirectModeEnablesFlush(t *testing.T) { dbPath = origDBPath autoImportEnabled = origAutoImport autoFlushEnabled = origAutoFlush - isDirty = origIsDirty - needsFullExport = origNeedsFull flushFailureCount = origFlushFailures lastFlushError = origLastFlushErr - flushMutex.Lock() - if flushTimer != nil { - flushTimer.Stop() - flushTimer = nil + // Restore FlushManager + if flushManager != nil { + _ = flushManager.Shutdown() } - flushMutex.Unlock() + flushManager = origFlushManager }() tmpDir := t.TempDir() @@ -112,8 +107,6 @@ func TestFallbackToDirectModeEnablesFlush(t *testing.T) { daemonStatus = DaemonStatus{} autoImportEnabled = false autoFlushEnabled = true - isDirty = false - needsFullExport = false if err := fallbackToDirectMode("test fallback"); err != nil { t.Fatalf("fallbackToDirectMode failed: %v", err) @@ -131,14 +124,7 @@ func TestFallbackToDirectModeEnablesFlush(t *testing.T) { } // Force a full export and flush synchronously - markDirtyAndScheduleFullExport() - flushMutex.Lock() - if flushTimer != nil { - flushTimer.Stop() - flushTimer = nil - } - flushMutex.Unlock() - flushToJSONL() + flushToJSONLWithState(flushState{forceDirty: true, forceFullExport: true}) jsonlPath := findJSONLPath() data, err := os.ReadFile(jsonlPath) diff --git a/cmd/bd/flush_manager_test.go b/cmd/bd/flush_manager_test.go index 7877cf2e..0292d7b1 100644 --- a/cmd/bd/flush_manager_test.go +++ b/cmd/bd/flush_manager_test.go @@ -402,8 +402,6 @@ func TestFlushManagerIdempotentShutdown(t *testing.T) { func setupTestEnvironment(t *testing.T) { autoFlushEnabled = true storeActive = true - isDirty = false - needsFullExport = false } // teardownTestEnvironment cleans up test environment diff --git a/cmd/bd/import.go b/cmd/bd/import.go index 634272b2..286c41b8 100644 --- a/cmd/bd/import.go +++ b/cmd/bd/import.go @@ -375,7 +375,7 @@ NOTE: Import requires direct database access and does not work with daemon mode. // Without this, daemon FileWatcher won't detect the import for up to 30s // Only flush if there were actual changes to avoid unnecessary I/O if result.Created > 0 || result.Updated > 0 || len(result.IDMapping) > 0 { - flushToJSONL() + flushToJSONLWithState(flushState{forceDirty: true}) } // Update jsonl_content_hash metadata to enable content-based staleness detection (bd-khnb fix) diff --git a/cmd/bd/main.go b/cmd/bd/main.go index 0f7b08cd..ee2ba2af 100644 --- a/cmd/bd/main.go +++ b/cmd/bd/main.go @@ -74,17 +74,14 @@ var ( rootCancel context.CancelFunc // Auto-flush state - autoFlushEnabled = true // Can be disabled with --no-auto-flush - isDirty = false // Tracks if DB has changes needing export (used by legacy code) - needsFullExport = false // Set to true when IDs change (used by legacy code) + autoFlushEnabled = true // Can be disabled with --no-auto-flush flushMutex sync.Mutex - flushTimer *time.Timer // DEPRECATED: Use flushManager instead - storeMutex sync.Mutex // Protects store access from background goroutine - storeActive = false // Tracks if store is available - flushFailureCount = 0 // Consecutive flush failures - lastFlushError error // Last flush error for debugging + storeMutex sync.Mutex // Protects store access from background goroutine + storeActive = false // Tracks if store is available + flushFailureCount = 0 // Consecutive flush failures + lastFlushError error // Last flush error for debugging - // Auto-flush manager (replaces timer-based approach to fix bd-52) + // Auto-flush manager (event-driven, fixes bd-52 race condition) flushManager *FlushManager // Hook runner for extensibility (bd-kwro.8) diff --git a/cmd/bd/main_test.go b/cmd/bd/main_test.go index d762fc13..b94291b0 100644 --- a/cmd/bd/main_test.go +++ b/cmd/bd/main_test.go @@ -16,9 +16,9 @@ import ( "github.com/steveyegge/beads/internal/types" ) -// TestAutoFlushOnExit tests that PersistentPostRun performs final flush before exit +// TestAutoFlushOnExit tests that FlushManager.Shutdown() performs final flush before exit func TestAutoFlushOnExit(t *testing.T) { - // FIX: Initialize rootCtx for flush operations + // Initialize rootCtx for flush operations ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() @@ -48,15 +48,16 @@ func TestAutoFlushOnExit(t *testing.T) { storeActive = true storeMutex.Unlock() - // Reset auto-flush state + // Initialize FlushManager for this test (short debounce for testing) autoFlushEnabled = true - isDirty = false - if flushTimer != nil { - flushTimer.Stop() - flushTimer = nil - } - - // ctx already declared above for rootCtx initialization + oldFlushManager := flushManager + flushManager = NewFlushManager(true, 50*time.Millisecond) + defer func() { + if flushManager != nil { + _ = flushManager.Shutdown() + } + flushManager = oldFlushManager + }() // Create test issue issue := &types.Issue{ @@ -75,50 +76,12 @@ func TestAutoFlushOnExit(t *testing.T) { // Mark dirty (simulating CRUD operation) markDirtyAndScheduleFlush() - // Simulate PersistentPostRun (exit behavior) - storeMutex.Lock() - storeActive = false - storeMutex.Unlock() - - flushMutex.Lock() - needsFlush := isDirty && autoFlushEnabled - if needsFlush { - if flushTimer != nil { - flushTimer.Stop() - flushTimer = nil - } - isDirty = false - } - flushMutex.Unlock() - - if needsFlush { - // Manually perform flush logic (simulating PersistentPostRun) - storeMutex.Lock() - storeActive = true // Temporarily re-enable for this test - storeMutex.Unlock() - - issues, err := testStore.SearchIssues(ctx, "", types.IssueFilter{}) - if err == nil { - allDeps, _ := testStore.GetAllDependencyRecords(ctx) - for _, iss := range issues { - iss.Dependencies = allDeps[iss.ID] - } - tempPath := jsonlPath + ".tmp" - f, err := os.Create(tempPath) - if err == nil { - encoder := json.NewEncoder(f) - for _, iss := range issues { - encoder.Encode(iss) - } - f.Close() - os.Rename(tempPath, jsonlPath) - } - } - - storeMutex.Lock() - storeActive = false - storeMutex.Unlock() + // Simulate PersistentPostRun exit behavior - shutdown FlushManager + // This performs the final flush before exit + if err := flushManager.Shutdown(); err != nil { + t.Fatalf("FlushManager shutdown failed: %v", err) } + flushManager = nil // Prevent double shutdown in defer testStore.Close() @@ -223,12 +186,8 @@ func TestAutoFlushJSONLContent(t *testing.T) { } } - // Mark dirty and flush immediately - flushMutex.Lock() - isDirty = true - flushMutex.Unlock() - - flushToJSONL() + // Flush immediately (forces export) + flushToJSONLWithState(flushState{forceDirty: true}) // Verify JSONL file exists if _, err := os.Stat(expectedJSONLPath); os.IsNotExist(err) {