From 1d5e89b9bb9d0cc66a1bbcceb1b41117583ecee8 Mon Sep 17 00:00:00 2001 From: Steve Yegge Date: Fri, 24 Oct 2025 12:22:22 -0700 Subject: [PATCH] Fix :memory: database handling with shared cache and proper URL construction - Convert :memory: to file::memory:?cache=shared for shared in-memory databases - Skip directory creation for memory databases - Properly append URL params with & when ? already exists in path - Add tests for in-memory database and shared cache behavior Amp-Thread-ID: https://ampcode.com/threads/T-c3d60758-fa92-472f-9239-6dab9b6a25c2 Co-authored-by: Amp --- internal/storage/sqlite/sqlite.go | 28 ++++-- internal/storage/sqlite/sqlite_test.go | 116 +++++++++++++++++++++++++ 2 files changed, 139 insertions(+), 5 deletions(-) diff --git a/internal/storage/sqlite/sqlite.go b/internal/storage/sqlite/sqlite.go index 003c94cb..a0a0a864 100644 --- a/internal/storage/sqlite/sqlite.go +++ b/internal/storage/sqlite/sqlite.go @@ -26,10 +26,20 @@ type SQLiteStorage struct { // New creates a new SQLite storage backend func New(path string) (*SQLiteStorage, error) { - // Ensure directory exists - dir := filepath.Dir(path) - if err := os.MkdirAll(dir, 0o755); err != nil { - return nil, fmt.Errorf("failed to create directory: %w", err) + // Convert :memory: to shared memory URL for consistent behavior across connections + // SQLite creates separate in-memory databases for each connection to ":memory:", + // but "file::memory:?cache=shared" creates a shared in-memory database. + dbPath := path + if path == ":memory:" { + dbPath = "file::memory:?cache=shared" + } + + // Ensure directory exists (skip for memory databases) + if !strings.Contains(dbPath, ":memory:") { + dir := filepath.Dir(dbPath) + if err := os.MkdirAll(dir, 0o755); err != nil { + return nil, fmt.Errorf("failed to create directory: %w", err) + } } // Open database with WAL mode for better concurrency and busy timeout for parallel writes @@ -38,7 +48,15 @@ func New(path string) (*SQLiteStorage, error) { // _pragma=foreign_keys(ON) enforces foreign key constraints // _pragma=busy_timeout(30000) means wait up to 30 seconds for locks instead of failing immediately // _time_format=sqlite enables automatic parsing of DATETIME columns to time.Time - db, err := sql.Open("sqlite", path+"?_pragma=journal_mode(WAL)&_pragma=foreign_keys(ON)&_pragma=busy_timeout(30000)&_time_format=sqlite") + // Note: For shared memory URLs, additional params need to be added with & not ? + connStr := dbPath + if strings.Contains(dbPath, "?") { + connStr += "&_pragma=journal_mode(WAL)&_pragma=foreign_keys(ON)&_pragma=busy_timeout(30000)&_time_format=sqlite" + } else { + connStr += "?_pragma=journal_mode(WAL)&_pragma=foreign_keys(ON)&_pragma=busy_timeout(30000)&_time_format=sqlite" + } + + db, err := sql.Open("sqlite", connStr) if err != nil { return nil, fmt.Errorf("failed to open database: %w", err) } diff --git a/internal/storage/sqlite/sqlite_test.go b/internal/storage/sqlite/sqlite_test.go index 445be42e..0a96d834 100644 --- a/internal/storage/sqlite/sqlite_test.go +++ b/internal/storage/sqlite/sqlite_test.go @@ -1293,3 +1293,119 @@ func TestMultipleStorageDistinctPaths(t *testing.T) { t.Errorf("Both paths should be absolute: path1=%s, path2=%s", path1, path2) } } + +func TestInMemoryDatabase(t *testing.T) { + ctx := context.Background() + + // Test that :memory: database works + store, err := New(":memory:") + if err != nil { + t.Fatalf("failed to create in-memory storage: %v", err) + } + defer store.Close() + + // Verify we can create and retrieve an issue + issue := &types.Issue{ + Title: "Test in-memory issue", + Description: "Testing :memory: database", + Status: types.StatusOpen, + Priority: 1, + IssueType: types.TypeTask, + } + + err = store.CreateIssue(ctx, issue, "test-user") + if err != nil { + t.Fatalf("CreateIssue failed in memory database: %v", err) + } + + // Retrieve the issue + retrieved, err := store.GetIssue(ctx, issue.ID) + if err != nil { + t.Fatalf("GetIssue failed in memory database: %v", err) + } + + if retrieved == nil { + t.Fatal("GetIssue returned nil for in-memory issue") + } + + if retrieved.Title != issue.Title { + t.Errorf("Title mismatch: got %v, want %v", retrieved.Title, issue.Title) + } + + if retrieved.Description != issue.Description { + t.Errorf("Description mismatch: got %v, want %v", retrieved.Description, issue.Description) + } +} + +func TestInMemorySharedCache(t *testing.T) { + ctx := context.Background() + + // Create first connection + store1, err := New(":memory:") + if err != nil { + t.Fatalf("failed to create first in-memory storage: %v", err) + } + defer store1.Close() + + // Create an issue in the first connection + issue := &types.Issue{ + Title: "Shared memory test", + Description: "Testing shared cache behavior", + Status: types.StatusOpen, + Priority: 2, + IssueType: types.TypeBug, + } + + err = store1.CreateIssue(ctx, issue, "test-user") + if err != nil { + t.Fatalf("CreateIssue failed: %v", err) + } + + // Create second connection - should share the same database due to file::memory:?cache=shared + store2, err := New(":memory:") + if err != nil { + t.Fatalf("failed to create second in-memory storage: %v", err) + } + defer store2.Close() + + // Retrieve the issue from the second connection + retrieved, err := store2.GetIssue(ctx, issue.ID) + if err != nil { + t.Fatalf("GetIssue failed from second connection: %v", err) + } + + if retrieved == nil { + t.Fatal("Shared memory cache not working: second connection can't see first connection's data") + } + + if retrieved.Title != issue.Title { + t.Errorf("Title mismatch: got %v, want %v", retrieved.Title, issue.Title) + } + + // Verify both connections can see each other's changes + issue2 := &types.Issue{ + Title: "Second issue", + Status: types.StatusOpen, + Priority: 1, + IssueType: types.TypeTask, + } + + err = store2.CreateIssue(ctx, issue2, "test-user") + if err != nil { + t.Fatalf("CreateIssue failed in second connection: %v", err) + } + + // First connection should see the issue created by second connection + retrieved2, err := store1.GetIssue(ctx, issue2.ID) + if err != nil { + t.Fatalf("GetIssue failed from first connection: %v", err) + } + + if retrieved2 == nil { + t.Fatal("First connection can't see second connection's data") + } + + if retrieved2.Title != issue2.Title { + t.Errorf("Title mismatch: got %v, want %v", retrieved2.Title, issue2.Title) + } +}