From 9088988edd155628b7f68d64a07ca64e7188c36a Mon Sep 17 00:00:00 2001 From: Steve Yegge Date: Thu, 20 Nov 2025 20:45:09 -0500 Subject: [PATCH] Improve staleness check error handling and optimization (bd-n4td, bd-o4qy, bd-c4rq) This commit implements three related improvements to database staleness checking: **bd-n4td (P2): Add warning when staleness check errors** - Added stderr warnings when CheckStaleness fails in ensureDatabaseFresh - Users now see "Warning: could not check database staleness: " - Provides visibility into staleness check failures while allowing operations to proceed **bd-o4qy (P2): Improve CheckStaleness error handling** - Updated CheckStaleness to distinguish between expected and abnormal conditions: * Returns (false, nil) for expected "no data yet" scenarios (missing metadata, missing JSONL) * Returns (false, err) for abnormal errors (glob failures, permission errors) - Updated RPC server (2 locations) to log staleness errors but allow requests to proceed - Prevents blocking operations due to transient staleness check issues - Added comprehensive function documentation **bd-c4rq (P3): Refactor staleness check to avoid function call overhead** - Moved daemon check from inside ensureDatabaseFresh to all 8 call sites - Avoids unnecessary function call when running in daemon mode - Updated: list.go, info.go, status.go, show.go, stale.go, duplicates.go, ready.go, validate.go - Extracted staleness functions to new staleness.go for better organization **Code review fixes:** - Removed dead code in CheckStaleness (unreachable jsonlPath == "" check) - Removed unused ensureDatabaseFreshQuiet function **Files changed:** - New: cmd/bd/staleness.go (extracted staleness checking functions) - Modified: 8 command files (added daemon check before staleness calls) - Modified: internal/autoimport/autoimport.go (improved error handling) - Modified: internal/rpc/server_export_import_auto.go (handle errors gracefully) --- cmd/bd/duplicates.go | 10 +++++ cmd/bd/info.go | 11 +++++ cmd/bd/list.go | 12 +++++- cmd/bd/ready.go | 10 +++++ cmd/bd/show.go | 9 ++++ cmd/bd/stale.go | 10 +++++ cmd/bd/staleness.go | 51 +++++++++++++++++++++++ cmd/bd/status.go | 10 +++++ cmd/bd/validate.go | 10 +++++ internal/autoimport/autoimport.go | 33 ++++++++++----- internal/rpc/server_export_import_auto.go | 18 ++++++-- 11 files changed, 169 insertions(+), 15 deletions(-) create mode 100644 cmd/bd/staleness.go diff --git a/cmd/bd/duplicates.go b/cmd/bd/duplicates.go index 2ec067c2..3a36f516 100644 --- a/cmd/bd/duplicates.go +++ b/cmd/bd/duplicates.go @@ -33,6 +33,16 @@ Example: dryRun, _ := cmd.Flags().GetBool("dry-run") // Use global jsonOutput set by PersistentPreRun ctx := context.Background() + + // Check database freshness before reading (bd-2q6d, bd-c4rq) + // Skip check when using daemon (daemon auto-imports on staleness) + if daemonClient == nil { + if err := ensureDatabaseFresh(ctx); err != nil { + fmt.Fprintf(os.Stderr, "Error: %v\n", err) + os.Exit(1) + } + } + // Get all issues allIssues, err := store.SearchIssues(ctx, "", types.IssueFilter{}) if err != nil { diff --git a/cmd/bd/info.go b/cmd/bd/info.go index 2e1a0435..5942b34a 100644 --- a/cmd/bd/info.go +++ b/cmd/bd/info.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "os" "path/filepath" "strings" @@ -88,6 +89,16 @@ Examples: // Get issue count from direct store if store != nil { ctx := context.Background() + + // Check database freshness before reading (bd-2q6d, bd-c4rq) + // Skip check when using daemon (daemon auto-imports on staleness) + if daemonClient == nil { + if err := ensureDatabaseFresh(ctx); err != nil { + fmt.Fprintf(os.Stderr, "Error: %v\n", err) + os.Exit(1) + } + } + filter := types.IssueFilter{} issues, err := store.SearchIssues(ctx, "", filter) if err == nil { diff --git a/cmd/bd/list.go b/cmd/bd/list.go index 62718549..4df0d0e2 100644 --- a/cmd/bd/list.go +++ b/cmd/bd/list.go @@ -193,6 +193,16 @@ var listCmd = &cobra.Command{ filter.PriorityMax = &priorityMax } + // Check database freshness before reading (bd-2q6d, bd-c4rq) + // Skip check when using daemon (daemon auto-imports on staleness) + ctx := context.Background() + if daemonClient == nil { + if err := ensureDatabaseFresh(ctx); err != nil { + fmt.Fprintf(os.Stderr, "Error: %v\n", err) + os.Exit(1) + } + } + // If daemon is running, use RPC if daemonClient != nil { listArgs := &rpc.ListArgs{ @@ -310,7 +320,7 @@ var listCmd = &cobra.Command{ } // Direct mode - ctx := context.Background() + // ctx already created above for staleness check issues, err := store.SearchIssues(ctx, "", filter) if err != nil { fmt.Fprintf(os.Stderr, "Error: %v\n", err) diff --git a/cmd/bd/ready.go b/cmd/bd/ready.go index d9321182..976c380d 100644 --- a/cmd/bd/ready.go +++ b/cmd/bd/ready.go @@ -98,6 +98,16 @@ var readyCmd = &cobra.Command{ } // Direct mode ctx := context.Background() + + // Check database freshness before reading (bd-2q6d, bd-c4rq) + // Skip check when using daemon (daemon auto-imports on staleness) + if daemonClient == nil { + if err := ensureDatabaseFresh(ctx); err != nil { + fmt.Fprintf(os.Stderr, "Error: %v\n", err) + os.Exit(1) + } + } + issues, err := store.GetReadyWork(ctx, filter) if err != nil { fmt.Fprintf(os.Stderr, "Error: %v\n", err) diff --git a/cmd/bd/show.go b/cmd/bd/show.go index 0beb8be5..16e561ac 100644 --- a/cmd/bd/show.go +++ b/cmd/bd/show.go @@ -24,6 +24,15 @@ var showCmd = &cobra.Command{ jsonOutput, _ := cmd.Flags().GetBool("json") ctx := context.Background() + // Check database freshness before reading (bd-2q6d, bd-c4rq) + // Skip check when using daemon (daemon auto-imports on staleness) + if daemonClient == nil { + if err := ensureDatabaseFresh(ctx); err != nil { + fmt.Fprintf(os.Stderr, "Error: %v\n", err) + os.Exit(1) + } + } + // Resolve partial IDs first var resolvedIDs []string if daemonClient != nil { diff --git a/cmd/bd/stale.go b/cmd/bd/stale.go index 65282dde..cd232764 100644 --- a/cmd/bd/stale.go +++ b/cmd/bd/stale.go @@ -62,6 +62,16 @@ This helps identify: } // Direct mode ctx := context.Background() + + // Check database freshness before reading (bd-2q6d, bd-c4rq) + // Skip check when using daemon (daemon auto-imports on staleness) + if daemonClient == nil { + if err := ensureDatabaseFresh(ctx); err != nil { + fmt.Fprintf(os.Stderr, "Error: %v\n", err) + os.Exit(1) + } + } + issues, err := store.GetStaleIssues(ctx, filter) if err != nil { fmt.Fprintf(os.Stderr, "Error: %v\n", err) diff --git a/cmd/bd/staleness.go b/cmd/bd/staleness.go new file mode 100644 index 00000000..73497184 --- /dev/null +++ b/cmd/bd/staleness.go @@ -0,0 +1,51 @@ +package main + +import ( + "context" + "fmt" + "os" + + "github.com/steveyegge/beads/internal/autoimport" +) + +// ensureDatabaseFresh checks if the database is in sync with JSONL before read operations. +// If JSONL is newer than database, refuses to operate with an error message. +// This prevents users from making decisions based on stale/incomplete data. +// +// NOTE: Callers must check if daemonClient != nil and skip calling this function +// when running in daemon mode (daemon auto-imports on staleness). +// +// Implements bd-2q6d: All read operations should validate database freshness. +// Implements bd-c4rq: Daemon check moved to call sites to avoid function call overhead. +func ensureDatabaseFresh(ctx context.Context) error { + // Skip check if no storage available (shouldn't happen in practice) + if store == nil { + return nil + } + + // Check if database is stale + isStale, err := autoimport.CheckStaleness(ctx, store, dbPath) + if err != nil { + // If we can't determine staleness, allow operation to proceed + // (better to show potentially stale data than block user) + fmt.Fprintf(os.Stderr, "Warning: could not check database staleness: %v\n", err) + return nil + } + + if !isStale { + // Database is fresh, proceed + return nil + } + + // Database is stale - refuse to operate + return fmt.Errorf( + "Database out of sync with JSONL. Run 'bd import' first.\n\n"+ + "The JSONL file has been updated (e.g., after 'git pull') but the database\n"+ + "hasn't been imported yet. This would cause you to see stale/incomplete data.\n\n"+ + "To fix:\n"+ + " bd import # Import JSONL updates to database\n\n"+ + "Or use daemon mode (auto-imports on every operation):\n"+ + " bd daemon start\n"+ + " bd # Will auto-import before executing", + ) +} diff --git a/cmd/bd/status.go b/cmd/bd/status.go index 4ac77284..466be073 100644 --- a/cmd/bd/status.go +++ b/cmd/bd/status.go @@ -77,6 +77,16 @@ Examples: var stats *types.Statistics var err error + // Check database freshness before reading (bd-2q6d, bd-c4rq) + // Skip check when using daemon (daemon auto-imports on staleness) + ctx := context.Background() + if daemonClient == nil { + if err := ensureDatabaseFresh(ctx); err != nil { + fmt.Fprintf(os.Stderr, "Error: %v\n", err) + os.Exit(1) + } + } + // If daemon is running, use RPC if daemonClient != nil { resp, rpcErr := daemonClient.Stats() diff --git a/cmd/bd/validate.go b/cmd/bd/validate.go index e74eee50..e80086c2 100644 --- a/cmd/bd/validate.go +++ b/cmd/bd/validate.go @@ -34,6 +34,16 @@ Example: checksFlag, _ := cmd.Flags().GetString("checks") jsonOut, _ := cmd.Flags().GetBool("json") ctx := context.Background() + + // Check database freshness before reading (bd-2q6d, bd-c4rq) + // Skip check when using daemon (daemon auto-imports on staleness) + if daemonClient == nil { + if err := ensureDatabaseFresh(ctx); err != nil { + fmt.Fprintf(os.Stderr, "Error: %v\n", err) + os.Exit(1) + } + } + // Parse and normalize checks checks, err := parseChecks(checksFlag) if err != nil { diff --git a/internal/autoimport/autoimport.go b/internal/autoimport/autoimport.go index e0479b73..15d7d389 100644 --- a/internal/autoimport/autoimport.go +++ b/internal/autoimport/autoimport.go @@ -250,36 +250,49 @@ func parseJSONL(jsonlData []byte, _ Notifier) ([]*types.Issue, error) { // CheckStaleness checks if JSONL is newer than last import // dbPath is the full path to the database file +// +// Returns: +// - (true, nil) if JSONL is newer than last import (database is stale) +// - (false, nil) if database is fresh or no JSONL exists yet +// - (false, err) if an abnormal error occurred (file system issues, permissions, etc.) func CheckStaleness(ctx context.Context, store storage.Storage, dbPath string) (bool, error) { lastImportStr, err := store.GetMetadata(ctx, "last_import_time") if err != nil { + // No metadata yet - expected for first run return false, nil } - + lastImportTime, err := time.Parse(time.RFC3339, lastImportStr) if err != nil { + // Corrupted metadata - not critical, assume not stale return false, nil } - + // Find JSONL using database directory dbDir := filepath.Dir(dbPath) pattern := filepath.Join(dbDir, "*.jsonl") matches, err := filepath.Glob(pattern) + if err != nil { + // Glob failed - this is abnormal + return false, fmt.Errorf("failed to find JSONL file: %w", err) + } + var jsonlPath string - if err == nil && len(matches) > 0 { + if len(matches) > 0 { jsonlPath = matches[0] } else { jsonlPath = filepath.Join(dbDir, "issues.jsonl") } - - if jsonlPath == "" { - return false, nil - } - + stat, err := os.Stat(jsonlPath) if err != nil { - return false, nil + if os.IsNotExist(err) { + // JSONL doesn't exist - expected for new repo + return false, nil + } + // Other stat error (permissions, etc.) - abnormal + return false, fmt.Errorf("failed to stat JSONL file %s: %w", jsonlPath, err) } - + return stat.ModTime().After(lastImportTime), nil } diff --git a/internal/rpc/server_export_import_auto.go b/internal/rpc/server_export_import_auto.go index 68bada96..016cbc5a 100644 --- a/internal/rpc/server_export_import_auto.go +++ b/internal/rpc/server_export_import_auto.go @@ -184,8 +184,13 @@ func (s *Server) checkAndAutoImportIfStale(req *Request) error { // Fast path: Check if JSONL is stale using cheap mtime check // This avoids reading/hashing JSONL on every request isStale, err := autoimport.CheckStaleness(ctx, store, dbPath) - if err != nil || !isStale { - return err + if err != nil { + // Log error but allow request to proceed (don't block on staleness check failure) + fmt.Fprintf(os.Stderr, "Warning: failed to check staleness: %v\n", err) + return nil + } + if !isStale { + return nil } // Single-flight guard: Only allow one import at a time @@ -220,8 +225,13 @@ func (s *Server) checkAndAutoImportIfStale(req *Request) error { // Double-check staleness after acquiring lock (another goroutine may have imported) isStale, err = autoimport.CheckStaleness(ctx, store, dbPath) - if err != nil || !isStale { - return err + if err != nil { + // Log error but allow request to proceed (don't block on staleness check failure) + fmt.Fprintf(os.Stderr, "Warning: failed to check staleness: %v\n", err) + return nil + } + if !isStale { + return nil } // Create timeout context for import operation (bd-8931, bd-1048)