Skip to content

Commit 6929314

Browse files
corylanoucorylanou
committed
fix(vfs): remove PRAGMAs per review, fix config safety issues
Address Ben's review feedback: PRAGMAs are overkill for config that should be set at initialization time. Also fix issues found during code review. Changes: - Remove all new PRAGMAs (poll_interval, cache_size, hydration_enabled, log_level, replica_url) - Return defensive copies from SetVFSConfig/GetVFSConfig to prevent concurrent mutation of shared config - Add nil client check in openMainDB to fail early with clear error - Close per-connection client on f.Open() failure to prevent leaks - Revert unnecessary mutex addition in monitorReplicaClient - Add TestVFSConfig_CopyOnSetAndGet and TestVFS_NilClientReturnsError
1 parent 1957952 commit 6929314

File tree

3 files changed

+34
-246
lines changed

3 files changed

+34
-246
lines changed

‎vfs.go‎

Lines changed: 1 addition & 78 deletions
Original file line numberDiff line numberDiff line change
@@ -2500,80 +2500,6 @@ func (f *VFSFile) FileControl(op int, pragmaName string, pragmaValue *string) (*
25002500
return nil, fmt.Errorf("invalid value for litestream_write_enabled: %s (use 0 or 1)", *pragmaValue)
25012501
}
25022502

2503-
case "litestream_poll_interval":
2504-
f.mu.Lock()
2505-
if pragmaValue == nil {
2506-
result := f.PollInterval.String()
2507-
f.mu.Unlock()
2508-
return &result, nil
2509-
}
2510-
d, err := time.ParseDuration(*pragmaValue)
2511-
if err != nil {
2512-
f.mu.Unlock()
2513-
return nil, fmt.Errorf("invalid poll_interval: %w", err)
2514-
}
2515-
f.PollInterval = d
2516-
f.mu.Unlock()
2517-
return nil, nil
2518-
2519-
case "litestream_cache_size":
2520-
f.mu.Lock()
2521-
if pragmaValue == nil {
2522-
result := strconv.Itoa(f.CacheSize)
2523-
f.mu.Unlock()
2524-
return &result, nil
2525-
}
2526-
n, err := strconv.Atoi(*pragmaValue)
2527-
if err != nil {
2528-
f.mu.Unlock()
2529-
return nil, fmt.Errorf("invalid cache_size: %w", err)
2530-
}
2531-
f.CacheSize = n
2532-
f.mu.Unlock()
2533-
return nil, nil
2534-
2535-
case "litestream_hydration_enabled":
2536-
if pragmaValue != nil {
2537-
return nil, fmt.Errorf("litestream_hydration_enabled is read-only")
2538-
}
2539-
if f.hydrator != nil {
2540-
result := "1"
2541-
return &result, nil
2542-
}
2543-
result := "0"
2544-
return &result, nil
2545-
2546-
case "litestream_log_level":
2547-
if pragmaValue == nil {
2548-
if f.logger.Enabled(context.Background(), slog.LevelDebug) {
2549-
result := "debug"
2550-
return &result, nil
2551-
}
2552-
result := "info"
2553-
return &result, nil
2554-
}
2555-
switch strings.ToLower(*pragmaValue) {
2556-
case "debug":
2557-
f.logger = slog.New(slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{Level: slog.LevelDebug})).With("name", f.name)
2558-
case "info":
2559-
f.logger = slog.New(slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{Level: slog.LevelInfo})).With("name", f.name)
2560-
default:
2561-
return nil, fmt.Errorf("invalid log_level: %s (use \"debug\" or \"info\")", *pragmaValue)
2562-
}
2563-
return nil, nil
2564-
2565-
case "litestream_replica_url":
2566-
if pragmaValue != nil {
2567-
return nil, fmt.Errorf("litestream_replica_url is read-only")
2568-
}
2569-
cfg := GetVFSConfig(f.name)
2570-
if cfg != nil && cfg.ReplicaURL != "" {
2571-
result := cfg.ReplicaURL
2572-
return &result, nil
2573-
}
2574-
result := ""
2575-
return &result, nil
2576-
25772503
default:
25782504
return nil, sqlite3vfs.NotFoundError
25792505
}
@@ -2611,10 +2537,7 @@ func isRetryablePageError(err error) bool {
26112537
}
26122538

26132539
func (f *VFSFile) monitorReplicaClient(ctx context.Context) {
2614-
f.mu.Lock()
2615-
pollInterval := f.PollInterval
2616-
f.mu.Unlock()
2617-
ticker := time.NewTicker(pollInterval)
2540+
ticker := time.NewTicker(f.PollInterval)
26182541
defer ticker.Stop()
26192542

26202543
for {

‎vfs_config.go‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,13 +28,19 @@ var (
2828
func SetVFSConfig(dbName string, cfg *VFSConfig) {
2929
vfsConfigsMu.Lock()
3030
defer vfsConfigsMu.Unlock()
31-
vfsConfigs[dbName] = cfg
31+
copied := *cfg
32+
vfsConfigs[dbName] = &copied
3233
}
3334

3435
func GetVFSConfig(dbName string) *VFSConfig {
3536
vfsConfigsMu.RLock()
3637
defer vfsConfigsMu.RUnlock()
37-
return vfsConfigs[dbName]
38+
orig := vfsConfigs[dbName]
39+
if orig == nil {
40+
return nil
41+
}
42+
copied := *orig
43+
return &copied
3844
}
3945

4046
func DeleteVFSConfig(dbName string) {

‎vfs_config_test.go‎

Lines changed: 25 additions & 166 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ package litestream
55
import (
66
"fmt"
77
"log/slog"
8-
"strconv"
98
"sync"
109
"testing"
1110
"time"
@@ -100,6 +99,26 @@ func TestVFSConfig_NilOptionalFields(t *testing.T) {
10099
}
101100
}
102101

102+
func TestVFSConfig_CopyOnSetAndGet(t *testing.T) {
103+
defer clearVFSConfigs()
104+
105+
cfg := &VFSConfig{ReplicaURL: "s3://bucket/original"}
106+
SetVFSConfig("copy.db", cfg)
107+
108+
cfg.ReplicaURL = "s3://bucket/mutated"
109+
110+
got := GetVFSConfig("copy.db")
111+
if got.ReplicaURL != "s3://bucket/original" {
112+
t.Fatalf("expected original url, got %q (SetVFSConfig did not copy)", got.ReplicaURL)
113+
}
114+
115+
got.ReplicaURL = "s3://bucket/mutated-via-get"
116+
got2 := GetVFSConfig("copy.db")
117+
if got2.ReplicaURL != "s3://bucket/original" {
118+
t.Fatalf("expected original url, got %q (GetVFSConfig did not copy)", got2.ReplicaURL)
119+
}
120+
}
121+
103122
func TestVFSConfig_PerConnectionOverrides(t *testing.T) {
104123
defer clearVFSConfigs()
105124

@@ -115,7 +134,7 @@ func TestVFSConfig_PerConnectionOverrides(t *testing.T) {
115134

116135
vfs := NewVFS(client, slog.Default())
117136

118-
f, _, err := vfs.openMainDB("config-override.db", 0x00000100) // OpenMainDB
137+
f, _, err := vfs.openMainDB("config-override.db", 0x00000100)
119138
if err != nil {
120139
t.Fatalf("open main db: %v", err)
121140
}
@@ -130,174 +149,14 @@ func TestVFSConfig_PerConnectionOverrides(t *testing.T) {
130149
}
131150
}
132151

133-
func TestVFSFile_PRAGMAPollInterval(t *testing.T) {
134-
client := newMockReplicaClient()
135-
client.addFixture(t, buildLTXFixture(t, 1, 'a'))
136-
137-
f := NewVFSFile(client, "test.db", slog.Default())
138-
if err := f.Open(); err != nil {
139-
t.Fatalf("open vfs file: %v", err)
140-
}
141-
defer f.Close()
142-
143-
const SQLITE_FCNTL_PRAGMA = 14
144-
145-
result, err := f.FileControl(SQLITE_FCNTL_PRAGMA, "litestream_poll_interval", nil)
146-
if err != nil {
147-
t.Fatalf("get poll_interval: %v", err)
148-
}
149-
if result == nil || *result != DefaultPollInterval.String() {
150-
t.Fatalf("expected default poll interval, got %v", result)
151-
}
152-
153-
newInterval := "5s"
154-
_, err = f.FileControl(SQLITE_FCNTL_PRAGMA, "litestream_poll_interval", &newInterval)
155-
if err != nil {
156-
t.Fatalf("set poll_interval: %v", err)
157-
}
158-
159-
result, err = f.FileControl(SQLITE_FCNTL_PRAGMA, "litestream_poll_interval", nil)
160-
if err != nil {
161-
t.Fatalf("get poll_interval after set: %v", err)
162-
}
163-
if result == nil || *result != "5s" {
164-
t.Fatalf("expected 5s, got %v", result)
165-
}
166-
}
167-
168-
func TestVFSFile_PRAGMACacheSize(t *testing.T) {
169-
client := newMockReplicaClient()
170-
client.addFixture(t, buildLTXFixture(t, 1, 'a'))
171-
172-
f := NewVFSFile(client, "test.db", slog.Default())
173-
if err := f.Open(); err != nil {
174-
t.Fatalf("open vfs file: %v", err)
175-
}
176-
defer f.Close()
177-
178-
const SQLITE_FCNTL_PRAGMA = 14
179-
180-
result, err := f.FileControl(SQLITE_FCNTL_PRAGMA, "litestream_cache_size", nil)
181-
if err != nil {
182-
t.Fatalf("get cache_size: %v", err)
183-
}
184-
if result == nil || *result != strconv.Itoa(DefaultCacheSize) {
185-
t.Fatalf("expected default cache size, got %v", result)
186-
}
187-
188-
newSize := "20971520"
189-
_, err = f.FileControl(SQLITE_FCNTL_PRAGMA, "litestream_cache_size", &newSize)
190-
if err != nil {
191-
t.Fatalf("set cache_size: %v", err)
192-
}
193-
194-
result, err = f.FileControl(SQLITE_FCNTL_PRAGMA, "litestream_cache_size", nil)
195-
if err != nil {
196-
t.Fatalf("get cache_size after set: %v", err)
197-
}
198-
if result == nil || *result != "20971520" {
199-
t.Fatalf("expected 20971520, got %v", result)
200-
}
201-
}
202-
203-
func TestVFSFile_PRAGMALogLevel(t *testing.T) {
204-
client := newMockReplicaClient()
205-
client.addFixture(t, buildLTXFixture(t, 1, 'a'))
206-
207-
f := NewVFSFile(client, "test.db", slog.Default())
208-
if err := f.Open(); err != nil {
209-
t.Fatalf("open vfs file: %v", err)
210-
}
211-
defer f.Close()
212-
213-
const SQLITE_FCNTL_PRAGMA = 14
214-
215-
debugLevel := "debug"
216-
_, err := f.FileControl(SQLITE_FCNTL_PRAGMA, "litestream_log_level", &debugLevel)
217-
if err != nil {
218-
t.Fatalf("set log_level to debug: %v", err)
219-
}
220-
221-
result, err := f.FileControl(SQLITE_FCNTL_PRAGMA, "litestream_log_level", nil)
222-
if err != nil {
223-
t.Fatalf("get log_level: %v", err)
224-
}
225-
if result == nil || *result != "debug" {
226-
t.Fatalf("expected debug, got %v", result)
227-
}
228-
229-
infoLevel := "info"
230-
_, err = f.FileControl(SQLITE_FCNTL_PRAGMA, "litestream_log_level", &infoLevel)
231-
if err != nil {
232-
t.Fatalf("set log_level to info: %v", err)
233-
}
234-
235-
result, err = f.FileControl(SQLITE_FCNTL_PRAGMA, "litestream_log_level", nil)
236-
if err != nil {
237-
t.Fatalf("get log_level: %v", err)
238-
}
239-
if result == nil || *result != "info" {
240-
t.Fatalf("expected info, got %v", result)
241-
}
242-
}
243-
244-
func TestVFSFile_PRAGMAHydrationEnabled(t *testing.T) {
245-
client := newMockReplicaClient()
246-
client.addFixture(t, buildLTXFixture(t, 1, 'a'))
247-
248-
f := NewVFSFile(client, "test.db", slog.Default())
249-
if err := f.Open(); err != nil {
250-
t.Fatalf("open vfs file: %v", err)
251-
}
252-
defer f.Close()
253-
254-
const SQLITE_FCNTL_PRAGMA = 14
255-
256-
result, err := f.FileControl(SQLITE_FCNTL_PRAGMA, "litestream_hydration_enabled", nil)
257-
if err != nil {
258-
t.Fatalf("get hydration_enabled: %v", err)
259-
}
260-
if result == nil || *result != "0" {
261-
t.Fatalf("expected 0, got %v", result)
262-
}
263-
264-
writeVal := "1"
265-
_, err = f.FileControl(SQLITE_FCNTL_PRAGMA, "litestream_hydration_enabled", &writeVal)
266-
if err == nil {
267-
t.Fatal("expected error for read-only pragma, got nil")
268-
}
269-
}
270-
271-
func TestVFSFile_PRAGMAReplicaURL(t *testing.T) {
152+
func TestVFS_NilClientReturnsError(t *testing.T) {
272153
defer clearVFSConfigs()
273154

274-
client := newMockReplicaClient()
275-
client.addFixture(t, buildLTXFixture(t, 1, 'a'))
276-
277-
SetVFSConfig("replica-url-test.db", &VFSConfig{
278-
ReplicaURL: "s3://my-bucket/db",
279-
})
280-
281-
f := NewVFSFile(client, "replica-url-test.db", slog.Default())
282-
if err := f.Open(); err != nil {
283-
t.Fatalf("open vfs file: %v", err)
284-
}
285-
defer f.Close()
286-
287-
const SQLITE_FCNTL_PRAGMA = 14
288-
289-
result, err := f.FileControl(SQLITE_FCNTL_PRAGMA, "litestream_replica_url", nil)
290-
if err != nil {
291-
t.Fatalf("get replica_url: %v", err)
292-
}
293-
if result == nil || *result != "s3://my-bucket/db" {
294-
t.Fatalf("expected s3://my-bucket/db, got %v", result)
295-
}
155+
vfs := NewVFS(nil, slog.Default())
296156

297-
writeVal := "s3://other/path"
298-
_, err = f.FileControl(SQLITE_FCNTL_PRAGMA, "litestream_replica_url", &writeVal)
157+
_, _, err := vfs.openMainDB("no-client.db", 0x00000100)
299158
if err == nil {
300-
t.Fatal("expected error for read-only pragma, got nil")
159+
t.Fatal("expected error when no client configured, got nil")
301160
}
302161
}
303162

0 commit comments

Comments
 (0)