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: <error>" - 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)
This commit is contained in:
@@ -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 {
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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)
|
||||
|
||||
51
cmd/bd/staleness.go
Normal file
51
cmd/bd/staleness.go
Normal file
@@ -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 <command> # Will auto-import before executing",
|
||||
)
|
||||
}
|
||||
@@ -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()
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user