Skip to content

Commit 168069e

Browse files
mrmaxsteelclaude
andcommitted
fix(remotecache): defer store close, add Push test, add freshness TTL
- Replace defer-in-loop with eager close in repo sync (defer inside a for loop leaks connections until function return) - Add FreshFor TTL to Cache (default 30s) — Ensure() skips pull when last pull is within the window, FreshFor=0 preserves always-pull - Add TestPush: full round-trip integration test (clone → insert → push → re-clone → verify data) - Add TestEnsureFreshFor: validates TTL skip and FreshFor=0 bypass - Add concurrency doc comment to OpenStore Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 488482d commit 168069e

3 files changed

Lines changed: 129 additions & 6 deletions

File tree

cmd/bd/repo.go

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -274,9 +274,7 @@ Also triggers Dolt push/pull if a remote is configured.`,
274274
}
275275

276276
issues, err := remoteStore.SearchIssues(ctx, "", types.IssueFilter{})
277-
if closeErr := remoteStore.Close(); closeErr != nil {
278-
fmt.Fprintf(os.Stderr, "Warning: failed to close remote store for %s: %v\n", repoPath, closeErr)
279-
}
277+
_ = remoteStore.Close() // close eagerly — defer in a loop would leak connections
280278
if err != nil {
281279
fmt.Fprintf(os.Stderr, "Warning: failed to read issues from %s: %v\n", repoPath, err)
282280
continue

internal/remotecache/cache.go

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,8 @@ type StoreOpener func(ctx context.Context, beadsDir string) (storage.DoltStorage
2626
// Cache manages local clones of remote Dolt databases.
2727
// Each remote URL maps to a directory under Dir named by CacheKey(url).
2828
type Cache struct {
29-
Dir string // e.g., ~/.cache/beads/remotes
29+
Dir string // e.g., ~/.cache/beads/remotes
30+
FreshFor time.Duration // skip pull if last pull was within this duration; 0 means always pull
3031
}
3132

3233
// CacheMeta stores metadata about a cached remote clone.
@@ -36,14 +37,18 @@ type CacheMeta struct {
3637
LastPush int64 `json:"last_push_ns"`
3738
}
3839

40+
// defaultFreshFor is the default TTL for cached clones. Ensure() skips
41+
// pulling when the last pull was within this duration.
42+
const defaultFreshFor = 30 * time.Second
43+
3944
// DefaultCache returns a Cache using the XDG-conventional cache directory.
4045
func DefaultCache() (*Cache, error) {
4146
cacheDir, err := os.UserCacheDir()
4247
if err != nil {
4348
return nil, fmt.Errorf("failed to determine cache directory: %w", err)
4449
}
4550
dir := filepath.Join(cacheDir, "beads", "remotes")
46-
return &Cache{Dir: dir}, nil
51+
return &Cache{Dir: dir, FreshFor: defaultFreshFor}, nil
4752
}
4853

4954
// entryDir returns the cache entry directory for a remote URL.
@@ -93,7 +98,16 @@ func (c *Cache) Ensure(ctx context.Context, remoteURL string) (string, error) {
9398

9499
target := c.cloneTarget(remoteURL)
95100
if c.doltExists(target) {
96-
// Warm start: pull
101+
// Warm start: skip pull if the cache is still fresh
102+
if c.FreshFor > 0 {
103+
meta := c.readMeta(remoteURL)
104+
age := time.Since(time.Unix(0, meta.LastPull))
105+
if age < c.FreshFor {
106+
debug.Logf("remotecache: skipping pull for %s (%.1fs old, fresh for %.0fs)\n",
107+
remoteURL, age.Seconds(), c.FreshFor.Seconds())
108+
return entry, nil
109+
}
110+
}
97111
if err := c.doltPull(ctx, target); err != nil {
98112
return "", fmt.Errorf("dolt pull failed for %s: %w", remoteURL, err)
99113
}
@@ -144,6 +158,11 @@ func (c *Cache) Push(ctx context.Context, remoteURL string) error {
144158
// OpenStore opens a DoltStorage from the cached clone using the provided
145159
// StoreOpener. The cache entry directory is used as the beads directory.
146160
// The caller is responsible for calling Close() on the returned store.
161+
//
162+
// Note: OpenStore does not acquire a cache lock. The caller must ensure
163+
// no concurrent Ensure() or Push() is running against the same remoteURL,
164+
// as those modify the underlying dolt database. This is safe for single-
165+
// process CLI use but not for concurrent multi-process access.
147166
func (c *Cache) OpenStore(ctx context.Context, remoteURL string, opener StoreOpener) (storage.DoltStorage, error) {
148167
entry := c.entryDir(remoteURL)
149168
if !c.doltExists(c.cloneTarget(remoteURL)) {

internal/remotecache/cache_test.go

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,9 @@ import (
55
"os"
66
"os/exec"
77
"path/filepath"
8+
"strings"
89
"testing"
10+
"time"
911
)
1012

1113
// skipIfNoDolt skips the test if the dolt CLI is not installed.
@@ -166,6 +168,110 @@ func TestEvict(t *testing.T) {
166168
}
167169
}
168170

171+
func TestPush(t *testing.T) {
172+
skipIfNoDolt(t)
173+
ctx := context.Background()
174+
175+
tmpDir := t.TempDir()
176+
remoteURL := initDoltRemote(t, filepath.Join(tmpDir, "remote"))
177+
178+
cache := &Cache{Dir: filepath.Join(tmpDir, "cache")}
179+
180+
// Clone the remote
181+
if _, err := cache.Ensure(ctx, remoteURL); err != nil {
182+
t.Fatalf("Ensure failed: %v", err)
183+
}
184+
185+
// Make a local change in the cached clone
186+
target := cache.cloneTarget(remoteURL)
187+
cmd := exec.Command("dolt", "sql", "-q", "INSERT INTO test_table VALUES (1, 'pushed')")
188+
cmd.Dir = target
189+
if out, err := cmd.CombinedOutput(); err != nil {
190+
t.Fatalf("insert failed: %v\n%s", err, out)
191+
}
192+
193+
cmd = exec.Command("dolt", "add", ".")
194+
cmd.Dir = target
195+
if out, err := cmd.CombinedOutput(); err != nil {
196+
t.Fatalf("dolt add failed: %v\n%s", err, out)
197+
}
198+
199+
cmd = exec.Command("dolt", "commit", "-m", "add row")
200+
cmd.Dir = target
201+
if out, err := cmd.CombinedOutput(); err != nil {
202+
t.Fatalf("dolt commit failed: %v\n%s", err, out)
203+
}
204+
205+
// Push back to remote
206+
if err := cache.Push(ctx, remoteURL); err != nil {
207+
t.Fatalf("Push failed: %v", err)
208+
}
209+
210+
// Verify push timestamp was recorded
211+
meta := cache.readMeta(remoteURL)
212+
if meta.LastPush == 0 {
213+
t.Error("meta.LastPush should be set after Push")
214+
}
215+
216+
// Verify the data made it to the remote by cloning into a fresh dir
217+
verifyDir := filepath.Join(tmpDir, "verify")
218+
cmd = exec.Command("dolt", "clone", remoteURL, verifyDir)
219+
if out, err := cmd.CombinedOutput(); err != nil {
220+
t.Fatalf("verification clone failed: %v\n%s", err, out)
221+
}
222+
223+
cmd = exec.Command("dolt", "sql", "-q", "SELECT name FROM test_table WHERE id = 1", "-r", "csv")
224+
cmd.Dir = verifyDir
225+
out, err := cmd.Output()
226+
if err != nil {
227+
t.Fatalf("verification query failed: %v", err)
228+
}
229+
if !strings.Contains(string(out), "pushed") {
230+
t.Errorf("expected 'pushed' in verification output, got: %s", out)
231+
}
232+
}
233+
234+
func TestEnsureFreshFor(t *testing.T) {
235+
skipIfNoDolt(t)
236+
ctx := context.Background()
237+
238+
tmpDir := t.TempDir()
239+
remoteURL := initDoltRemote(t, filepath.Join(tmpDir, "remote"))
240+
241+
cache := &Cache{
242+
Dir: filepath.Join(tmpDir, "cache"),
243+
FreshFor: 1 * time.Hour, // very long TTL so second call skips pull
244+
}
245+
246+
// Cold start (always clones)
247+
if _, err := cache.Ensure(ctx, remoteURL); err != nil {
248+
t.Fatalf("Ensure (cold) failed: %v", err)
249+
}
250+
251+
firstMeta := cache.readMeta(remoteURL)
252+
253+
// Second call should skip pull because of FreshFor
254+
if _, err := cache.Ensure(ctx, remoteURL); err != nil {
255+
t.Fatalf("Ensure (warm, fresh) failed: %v", err)
256+
}
257+
258+
secondMeta := cache.readMeta(remoteURL)
259+
if secondMeta.LastPull != firstMeta.LastPull {
260+
t.Error("LastPull should NOT update when cache is still fresh")
261+
}
262+
263+
// With FreshFor=0, should always pull
264+
cache.FreshFor = 0
265+
if _, err := cache.Ensure(ctx, remoteURL); err != nil {
266+
t.Fatalf("Ensure (warm, FreshFor=0) failed: %v", err)
267+
}
268+
269+
thirdMeta := cache.readMeta(remoteURL)
270+
if thirdMeta.LastPull <= firstMeta.LastPull {
271+
t.Error("LastPull should update when FreshFor=0")
272+
}
273+
}
274+
169275
func TestDefaultCache(t *testing.T) {
170276
cache, err := DefaultCache()
171277
if err != nil {

0 commit comments

Comments
 (0)