fix: read operations no longer modify database file (GH#804)
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 <noreply@anthropic.com>
This commit is contained in:
@@ -82,6 +82,30 @@ var (
|
|||||||
quietFlag bool // Suppress non-essential output
|
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() {
|
func init() {
|
||||||
// Initialize viper configuration
|
// Initialize viper configuration
|
||||||
if err := config.Initialize(); err != nil {
|
if err := config.Initialize(); err != nil {
|
||||||
@@ -656,14 +680,33 @@ var rootCmd = &cobra.Command{
|
|||||||
debug.Logf("using direct mode (reason: %s)", daemonStatus.FallbackReason)
|
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
|
// 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
|
// 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
|
// 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
|
// Fall back to direct storage access
|
||||||
var err error
|
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 {
|
if err != nil {
|
||||||
// Check for fresh clone scenario
|
// Check for fresh clone scenario
|
||||||
beadsDir := filepath.Dir(dbPath)
|
beadsDir := filepath.Dir(dbPath)
|
||||||
@@ -682,10 +725,11 @@ var rootCmd = &cobra.Command{
|
|||||||
// Initialize flush manager (fixes race condition in auto-flush)
|
// Initialize flush manager (fixes race condition in auto-flush)
|
||||||
// Skip FlushManager creation in sandbox mode - no background goroutines needed
|
// Skip FlushManager creation in sandbox mode - no background goroutines needed
|
||||||
// (improves Windows exit behavior and container scenarios)
|
// (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,
|
// For in-process test scenarios where commands run multiple times,
|
||||||
// we create a new manager each time. Shutdown() is idempotent so
|
// we create a new manager each time. Shutdown() is idempotent so
|
||||||
// PostRun can safely shutdown whichever manager is active.
|
// PostRun can safely shutdown whichever manager is active.
|
||||||
if !sandboxMode {
|
if !sandboxMode && !useReadOnly {
|
||||||
flushManager = NewFlushManager(autoFlushEnabled, getDebounceDuration())
|
flushManager = NewFlushManager(autoFlushEnabled, getDebounceDuration())
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -703,7 +747,8 @@ var rootCmd = &cobra.Command{
|
|||||||
// Skip for import command itself to avoid recursion
|
// Skip for import command itself to avoid recursion
|
||||||
// Skip for delete command to prevent resurrection of deleted issues
|
// Skip for delete command to prevent resurrection of deleted issues
|
||||||
// Skip if sync --dry-run to avoid modifying DB in dry-run mode
|
// 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
|
// Check if this is sync command with --dry-run flag
|
||||||
if cmd.Name() == "sync" {
|
if cmd.Name() == "sync" {
|
||||||
if dryRun, _ := cmd.Flags().GetBool("dry-run"); dryRun {
|
if dryRun, _ := cmd.Flags().GetBool("dry-run"); dryRun {
|
||||||
|
|||||||
189
internal/storage/sqlite/readonly_test.go
Normal file
189
internal/storage/sqlite/readonly_test.go
Normal file
@@ -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")
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -27,6 +27,7 @@ type SQLiteStorage struct {
|
|||||||
closed atomic.Bool // Tracks whether Close() has been called
|
closed atomic.Bool // Tracks whether Close() has been called
|
||||||
connStr string // Connection string for reconnection
|
connStr string // Connection string for reconnection
|
||||||
busyTimeout time.Duration
|
busyTimeout time.Duration
|
||||||
|
readOnly bool // True if opened in read-only mode (GH#804)
|
||||||
freshness *FreshnessChecker // Optional freshness checker for daemon mode
|
freshness *FreshnessChecker // Optional freshness checker for daemon mode
|
||||||
reconnectMu sync.RWMutex // Protects reconnection and db access (GH#607)
|
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
|
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.
|
// 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 {
|
func (s *SQLiteStorage) Close() error {
|
||||||
s.closed.Store(true)
|
s.closed.Store(true)
|
||||||
// Acquire write lock to prevent racing with reconnect() (GH#607)
|
// Acquire write lock to prevent racing with reconnect() (GH#607)
|
||||||
s.reconnectMu.Lock()
|
s.reconnectMu.Lock()
|
||||||
defer s.reconnectMu.Unlock()
|
defer s.reconnectMu.Unlock()
|
||||||
// Checkpoint WAL to ensure all writes are persisted to the main database file.
|
// Only checkpoint for read-write connections (GH#804)
|
||||||
// Without this, writes may be stranded in the WAL and lost between CLI invocations.
|
// Read-only connections should not modify the database file at all.
|
||||||
_, _ = s.db.Exec("PRAGMA wal_checkpoint(TRUNCATE)")
|
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()
|
return s.db.Close()
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user