From 7d4e8e2db94b5e65b194e60b659fa9150bd426fd Mon Sep 17 00:00:00 2001 From: Steve Yegge Date: Tue, 30 Dec 2025 16:47:38 -0800 Subject: [PATCH] fix: read operations no longer modify database file (GH#804) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add SQLite read-only mode for commands that only query data (list, ready, show, stats, blocked, count, search, graph, duplicates, comments, export). Changes: - Add NewReadOnly() and NewReadOnlyWithTimeout() to sqlite package - Opens with mode=ro to prevent any file writes - Skips WAL pragma, schema init, and migrations - Skips WAL checkpoint on Close() - Update main.go to detect read-only commands and use appropriate opener - Skip auto-migrate, FlushManager, and auto-import for read-only commands - Add tests verifying file mtime is unchanged after read operations This fixes the issue where file watchers (like beads-ui) would go into infinite loops because bd list/show/ready modified the database file. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- cmd/bd/main.go | 53 ++++++- internal/storage/sqlite/readonly_test.go | 189 +++++++++++++++++++++++ internal/storage/sqlite/store.go | 81 +++++++++- 3 files changed, 315 insertions(+), 8 deletions(-) create mode 100644 internal/storage/sqlite/readonly_test.go diff --git a/cmd/bd/main.go b/cmd/bd/main.go index 90238f4d..cf1d241e 100644 --- a/cmd/bd/main.go +++ b/cmd/bd/main.go @@ -82,6 +82,30 @@ var ( quietFlag bool // Suppress non-essential output ) +// readOnlyCommands lists commands that only read from the database. +// These commands open SQLite in read-only mode to avoid modifying the +// database file (which breaks file watchers). See GH#804. +var readOnlyCommands = map[string]bool{ + "list": true, + "ready": true, + "show": true, + "stats": true, + "blocked": true, + "count": true, + "search": true, + "graph": true, + "duplicates": true, + "comments": true, // list comments (not add) + "export": true, // export only reads +} + +// isReadOnlyCommand returns true if the command only reads from the database. +// This is used to open SQLite in read-only mode, preventing file modifications +// that would trigger file watchers. See GH#804. +func isReadOnlyCommand(cmdName string) bool { + return readOnlyCommands[cmdName] +} + func init() { // Initialize viper configuration if err := config.Initialize(); err != nil { @@ -656,14 +680,33 @@ var rootCmd = &cobra.Command{ debug.Logf("using direct mode (reason: %s)", daemonStatus.FallbackReason) } + // Check if this is a read-only command (GH#804) + // Read-only commands open SQLite in read-only mode to avoid modifying + // the database file (which breaks file watchers). + useReadOnly := isReadOnlyCommand(cmd.Name()) + // Auto-migrate database on version bump + // Skip for read-only commands - they can't write anyway // Do this AFTER daemon check but BEFORE opening database for main operation // This ensures: 1) no daemon has DB open, 2) we don't open DB twice - autoMigrateOnVersionBump(dbPath) + if !useReadOnly { + autoMigrateOnVersionBump(dbPath) + } // Fall back to direct storage access var err error - store, err = sqlite.NewWithTimeout(rootCtx, dbPath, lockTimeout) + if useReadOnly { + // Read-only mode: prevents file modifications (GH#804) + store, err = sqlite.NewReadOnlyWithTimeout(rootCtx, dbPath, lockTimeout) + if err != nil { + // If read-only fails (e.g., DB doesn't exist), fall back to read-write + // This handles the case where user runs "bd list" before "bd init" + debug.Logf("read-only open failed, falling back to read-write: %v", err) + store, err = sqlite.NewWithTimeout(rootCtx, dbPath, lockTimeout) + } + } else { + store, err = sqlite.NewWithTimeout(rootCtx, dbPath, lockTimeout) + } if err != nil { // Check for fresh clone scenario beadsDir := filepath.Dir(dbPath) @@ -682,10 +725,11 @@ var rootCmd = &cobra.Command{ // Initialize flush manager (fixes race condition in auto-flush) // Skip FlushManager creation in sandbox mode - no background goroutines needed // (improves Windows exit behavior and container scenarios) + // Skip for read-only commands - they don't write anything (GH#804) // For in-process test scenarios where commands run multiple times, // we create a new manager each time. Shutdown() is idempotent so // PostRun can safely shutdown whichever manager is active. - if !sandboxMode { + if !sandboxMode && !useReadOnly { flushManager = NewFlushManager(autoFlushEnabled, getDebounceDuration()) } @@ -703,7 +747,8 @@ var rootCmd = &cobra.Command{ // Skip for import command itself to avoid recursion // Skip for delete command to prevent resurrection of deleted issues // Skip if sync --dry-run to avoid modifying DB in dry-run mode - if cmd.Name() != "import" && cmd.Name() != "delete" && autoImportEnabled { + // Skip for read-only commands - they can't write anyway (GH#804) + if cmd.Name() != "import" && cmd.Name() != "delete" && autoImportEnabled && !useReadOnly { // Check if this is sync command with --dry-run flag if cmd.Name() == "sync" { if dryRun, _ := cmd.Flags().GetBool("dry-run"); dryRun { diff --git a/internal/storage/sqlite/readonly_test.go b/internal/storage/sqlite/readonly_test.go new file mode 100644 index 00000000..b8a0ad1f --- /dev/null +++ b/internal/storage/sqlite/readonly_test.go @@ -0,0 +1,189 @@ +package sqlite + +import ( + "context" + "os" + "path/filepath" + "testing" + "time" + + "github.com/steveyegge/beads/internal/types" +) + +// TestReadOnlyDoesNotModifyFile verifies that opening a database in read-only mode +// and performing read operations does not modify the database file. +// This is the fix for GH#804. +func TestReadOnlyDoesNotModifyFile(t *testing.T) { + // Create a temporary directory for the test + tmpDir, err := os.MkdirTemp("", "beads-readonly-test-*") + if err != nil { + t.Fatalf("failed to create temp dir: %v", err) + } + defer os.RemoveAll(tmpDir) + + dbPath := filepath.Join(tmpDir, "test.db") + ctx := context.Background() + + // Step 1: Create and initialize the database with some data + store, err := New(ctx, dbPath) + if err != nil { + t.Fatalf("failed to create storage: %v", err) + } + + // Set prefix and create a test issue + if err := store.SetConfig(ctx, "issue_prefix", "test"); err != nil { + store.Close() + t.Fatalf("failed to set issue_prefix: %v", err) + } + + issue := &types.Issue{ + Title: "Test issue", + Status: types.StatusOpen, + Priority: 1, + IssueType: types.TypeTask, + } + if err := store.CreateIssue(ctx, issue, "test-user"); err != nil { + store.Close() + t.Fatalf("failed to create issue: %v", err) + } + + // Close the store to flush all changes + if err := store.Close(); err != nil { + t.Fatalf("failed to close store: %v", err) + } + + // Step 2: Get the file's modification time after initial write + // Wait a moment to ensure mtime granularity + time.Sleep(100 * time.Millisecond) + + info1, err := os.Stat(dbPath) + if err != nil { + t.Fatalf("failed to stat database: %v", err) + } + mtime1 := info1.ModTime() + + // Step 3: Open in read-only mode and perform read operations + time.Sleep(100 * time.Millisecond) // Ensure time has passed + + roStore, err := NewReadOnly(ctx, dbPath) + if err != nil { + t.Fatalf("failed to open read-only: %v", err) + } + + // Perform various read operations using SearchIssues + issues, err := roStore.SearchIssues(ctx, "", types.IssueFilter{}) + if err != nil { + roStore.Close() + t.Fatalf("failed to search issues: %v", err) + } + if len(issues) != 1 { + roStore.Close() + t.Fatalf("expected 1 issue, got %d", len(issues)) + } + + // Read the issue by ID + _, err = roStore.GetIssue(ctx, issues[0].ID) + if err != nil { + roStore.Close() + t.Fatalf("failed to get issue: %v", err) + } + + // Get config + _, err = roStore.GetConfig(ctx, "issue_prefix") + if err != nil { + roStore.Close() + t.Fatalf("failed to get config: %v", err) + } + + // Close the read-only store + if err := roStore.Close(); err != nil { + t.Fatalf("failed to close read-only store: %v", err) + } + + // Step 4: Verify the file was NOT modified + info2, err := os.Stat(dbPath) + if err != nil { + t.Fatalf("failed to stat database after read-only operations: %v", err) + } + mtime2 := info2.ModTime() + + if !mtime1.Equal(mtime2) { + t.Errorf("database file was modified during read-only operations!\n"+ + " before: %v\n after: %v\n"+ + "This breaks file watchers (GH#804)", + mtime1, mtime2) + } +} + +// TestReadOnlyRejectsWrites verifies that write operations fail on read-only connections. +func TestReadOnlyRejectsWrites(t *testing.T) { + // Create a temporary directory for the test + tmpDir, err := os.MkdirTemp("", "beads-readonly-write-test-*") + if err != nil { + t.Fatalf("failed to create temp dir: %v", err) + } + defer os.RemoveAll(tmpDir) + + dbPath := filepath.Join(tmpDir, "test.db") + ctx := context.Background() + + // Create and initialize the database + store, err := New(ctx, dbPath) + if err != nil { + t.Fatalf("failed to create storage: %v", err) + } + if err := store.SetConfig(ctx, "issue_prefix", "test"); err != nil { + store.Close() + t.Fatalf("failed to set issue_prefix: %v", err) + } + if err := store.Close(); err != nil { + t.Fatalf("failed to close store: %v", err) + } + + // Open in read-only mode + roStore, err := NewReadOnly(ctx, dbPath) + if err != nil { + t.Fatalf("failed to open read-only: %v", err) + } + defer roStore.Close() + + // Attempt to create an issue - should fail + issue := &types.Issue{ + Title: "Should fail", + Status: types.StatusOpen, + Priority: 1, + IssueType: types.TypeTask, + } + err = roStore.CreateIssue(ctx, issue, "test-user") + if err == nil { + t.Error("expected write to fail on read-only database, but it succeeded") + } +} + +// TestReadOnlyFailsOnNonexistentDB verifies that NewReadOnly returns an error +// when the database file doesn't exist. +func TestReadOnlyFailsOnNonexistentDB(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "beads-readonly-noexist-*") + if err != nil { + t.Fatalf("failed to create temp dir: %v", err) + } + defer os.RemoveAll(tmpDir) + + dbPath := filepath.Join(tmpDir, "nonexistent.db") + ctx := context.Background() + + _, err = NewReadOnly(ctx, dbPath) + if err == nil { + t.Error("expected error when opening nonexistent database in read-only mode") + } +} + +// TestReadOnlyRejectsInMemory verifies that NewReadOnly rejects in-memory databases. +func TestReadOnlyRejectsInMemory(t *testing.T) { + ctx := context.Background() + + _, err := NewReadOnly(ctx, ":memory:") + if err == nil { + t.Error("expected error when opening in-memory database in read-only mode") + } +} diff --git a/internal/storage/sqlite/store.go b/internal/storage/sqlite/store.go index 2407129b..adaa1f8c 100644 --- a/internal/storage/sqlite/store.go +++ b/internal/storage/sqlite/store.go @@ -27,6 +27,7 @@ type SQLiteStorage struct { closed atomic.Bool // Tracks whether Close() has been called connStr string // Connection string for reconnection busyTimeout time.Duration + readOnly bool // True if opened in read-only mode (GH#804) freshness *FreshnessChecker // Optional freshness checker for daemon mode reconnectMu sync.RWMutex // Protects reconnection and db access (GH#607) } @@ -203,16 +204,88 @@ func NewWithTimeout(ctx context.Context, path string, busyTimeout time.Duration) return storage, nil } +// NewReadOnly opens an existing database in read-only mode. +// This prevents any modification to the database file, including: +// - WAL journal mode changes +// - Schema/migration updates +// - WAL checkpointing on close +// +// Use this for read-only commands (list, ready, show, stats, etc.) to avoid +// triggering file watchers. See GH#804. +// +// Returns an error if the database doesn't exist (unlike New which creates it). +func NewReadOnly(ctx context.Context, path string) (*SQLiteStorage, error) { + return NewReadOnlyWithTimeout(ctx, path, 30*time.Second) +} + +// NewReadOnlyWithTimeout opens an existing database in read-only mode with configurable timeout. +func NewReadOnlyWithTimeout(ctx context.Context, path string, busyTimeout time.Duration) (*SQLiteStorage, error) { + // Read-only mode doesn't make sense for in-memory databases + if path == ":memory:" || (strings.HasPrefix(path, "file:") && strings.Contains(path, "mode=memory")) { + return nil, fmt.Errorf("read-only mode not supported for in-memory databases") + } + + // Check that the database file exists + if _, err := os.Stat(path); os.IsNotExist(err) { + return nil, fmt.Errorf("database does not exist: %s", path) + } + + // Convert timeout to milliseconds for SQLite pragma + timeoutMs := int64(busyTimeout / time.Millisecond) + + // Build read-only connection string with mode=ro + // This prevents any writes to the database file + connStr := fmt.Sprintf("file:%s?mode=ro&_pragma=foreign_keys(ON)&_pragma=busy_timeout(%d)&_time_format=sqlite", path, timeoutMs) + + db, err := sql.Open("sqlite3", connStr) + if err != nil { + return nil, fmt.Errorf("failed to open database read-only: %w", err) + } + + // Read-only connections don't need a large pool + db.SetMaxOpenConns(2) + db.SetMaxIdleConns(1) + db.SetConnMaxLifetime(0) + + // Test connection + if err := db.Ping(); err != nil { + return nil, fmt.Errorf("failed to ping database: %w", err) + } + + // Skip schema initialization and migrations - we're read-only + // The database must already be properly initialized + + // Convert to absolute path for consistency + absPath, err := filepath.Abs(path) + if err != nil { + return nil, fmt.Errorf("failed to get absolute path: %w", err) + } + + return &SQLiteStorage{ + db: db, + dbPath: absPath, + connStr: connStr, + busyTimeout: busyTimeout, + readOnly: true, + }, nil +} + // Close closes the database connection. -// It checkpoints the WAL to ensure all writes are flushed to the main database file. +// For read-write connections, it checkpoints the WAL to ensure all writes +// are flushed to the main database file. +// For read-only connections (GH#804), it skips checkpointing to avoid file modifications. func (s *SQLiteStorage) Close() error { s.closed.Store(true) // Acquire write lock to prevent racing with reconnect() (GH#607) s.reconnectMu.Lock() defer s.reconnectMu.Unlock() - // Checkpoint WAL to ensure all writes are persisted to the main database file. - // Without this, writes may be stranded in the WAL and lost between CLI invocations. - _, _ = s.db.Exec("PRAGMA wal_checkpoint(TRUNCATE)") + // Only checkpoint for read-write connections (GH#804) + // Read-only connections should not modify the database file at all. + if !s.readOnly { + // Checkpoint WAL to ensure all writes are persisted to the main database file. + // Without this, writes may be stranded in the WAL and lost between CLI invocations. + _, _ = s.db.Exec("PRAGMA wal_checkpoint(TRUNCATE)") + } return s.db.Close() }