Skip to content

Commit 58fe95b

Browse files
radimsemclaude
andauthored
fix(rescan): retain modTimes when purge emit fails (#210)
## Summary Fixes #207 — a rescan tick that detected a file is gone deleted its entry from `r.modTimes` **before** `reconcileDeleted` actually emitted the purge snapshot. If `emitter.Emit` then failed (SQLITE_BUSY exhausting `Tx` retry budget, context cancellation racing the emit, transient I/O), the in-memory tracking was already cleared but the DB rows remained. The deletion was never retried, leaving zombie nodes that surfaced via `MemorySearch`, snapshot replays, etc. — until a full re-compile. **Fix (issue option a — defer the delete):** mirror the discipline already present in the compile path at `rescan.go:334` (`maps.Copy(r.modTimes, pending)` runs *after* `compiler.Compile` returns no error). Collect `(absPath, relPath)` pairs without mutating `r.modTimes` in the loop; clear `r.modTimes` only when `reconcileDeleted` returns `ok=true`. - `reconcileDeleted` signature: `[]PurgedFile` → `([]PurgedFile, bool)`. `ok=true` for the three no-op success branches (empty deleted, no matching nodes, happy path); `ok=false` for `GetNodesByFiles` and `Emit` errors. - `notifyChange()` gated on `ok && len(deleted) > 0` — failed purges don't fire change events. - Bool over `error` per `.claude/rules/go-concise.md` §5 ("handle or return, never both"): caller doesn't discriminate failure modes, and `reconcileDeleted` already logs the underlying error internally. ## Test plan - [x] `TestRescanLoop_RetainsModTimesOnPurgeEmitFailure` — new; mirrors sibling `TestRescanLoop_SkipsPurgeOnWalkError`. Injects `r.walkFn` to wrap the real walk + cancel ctx after the walk returns, so `emitter.Emit`'s `BeginTx(ctx, ...)` fails. Asserts modTimes preserved + DB nodes intact + no new snapshot row. Then resets `walkFn` and re-scans to verify the retry path completes the purge. - [x] `TestRescanLoop_ReconcilesDeletedFiles`, `TestRescanLoop_PublishesStatus`, `TestRescanLoop_SkipsPurgeOnWalkError` — still green (no regression in happy / walk-error paths). - [x] `make test`, `make fmt lint tidy` — all green (0 lint issues). Reviewed by `go-style-reviewer` (project subagent) + Codex (generic, via `/forge ... codex`). Loop converged on first pass — zero actionable findings. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent a005281 commit 58fe95b

5 files changed

Lines changed: 102 additions & 11 deletions

File tree

cmd/remindb/serve_test.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -171,6 +171,7 @@ func TestNewServeLogger_JsonFileOutput(t *testing.T) {
171171
}
172172
if file == nil {
173173
t.Fatal("output_path set, file handle should be returned for cleanup")
174+
return
174175
}
175176
defer func() { _ = file.Close() }()
176177

@@ -204,6 +205,7 @@ func TestNewServeLogger_ConfiguredBufferCaptures(t *testing.T) {
204205
}
205206
if buf == nil {
206207
t.Fatal("buffer should be returned for the logs resource")
208+
return
207209
}
208210

209211
lg.Info("a")

internal/pathmatch/ignore_test.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ func TestLoadIgnore_EmptyFile(t *testing.T) {
4141
}
4242
if m == nil {
4343
t.Fatal("expected non-nil matcher for empty file")
44+
return
4445
}
4546
if m.Match("anything.md", false) {
4647
t.Error("empty matcher should not match")
@@ -57,6 +58,7 @@ func TestLoadIgnore_CommentsAndBlanks(t *testing.T) {
5758
}
5859
if m == nil {
5960
t.Fatal("expected non-nil matcher")
61+
return
6062
}
6163
if len(m.patterns) != 0 {
6264
t.Errorf("expected 0 patterns, got %d", len(m.patterns))

mcp_integration_test.go

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -944,6 +944,7 @@ func TestMcp_OverviewResource(t *testing.T) {
944944
}
945945
if overview == nil {
946946
t.Fatalf("resources/list missing remindb://overview, got %d resources", len(listed.Resources))
947+
return
947948
}
948949
if overview.MIMEType != "application/json" {
949950
t.Errorf("overview MIME type = %q, want application/json", overview.MIMEType)
@@ -1032,6 +1033,7 @@ func TestMcp_FilesResource(t *testing.T) {
10321033
}
10331034
if files == nil {
10341035
t.Fatalf("resources/list missing remindb://files, got %d resources", len(listed.Resources))
1036+
return
10351037
}
10361038
if files.MIMEType != "application/json" {
10371039
t.Errorf("files MIME type = %q, want application/json", files.MIMEType)
@@ -1140,6 +1142,7 @@ func TestMcp_TreeResource(t *testing.T) {
11401142
}
11411143
if tree == nil {
11421144
t.Fatalf("resources/list missing remindb://tree, got %d resources", len(listed.Resources))
1145+
return
11431146
}
11441147
if tree.MIMEType != "application/json" {
11451148
t.Errorf("tree MIME type = %q, want application/json", tree.MIMEType)
@@ -1189,6 +1192,7 @@ func TestMcp_TreeResource(t *testing.T) {
11891192
}
11901193
if pivot == nil {
11911194
t.Fatalf("no node with a grandchild found; cannot assert depth bounding")
1195+
return
11921196
}
11931197

11941198
// Shape: every node carries the full field set.
@@ -1288,6 +1292,7 @@ func TestMcp_SnapshotsResource(t *testing.T) {
12881292
}
12891293
if snapshots == nil {
12901294
t.Fatalf("resources/list missing remindb://snapshots, got %d resources", len(listed.Resources))
1295+
return
12911296
}
12921297
if snapshots.MIMEType != "application/json" {
12931298
t.Errorf("snapshots MIME type = %q, want application/json", snapshots.MIMEType)
@@ -1488,6 +1493,7 @@ func TestMcp_TemperatureResource(t *testing.T) {
14881493
}
14891494
if heat == nil {
14901495
t.Fatalf("resources/list missing remindb://temperature, got %d resources", len(listed.Resources))
1496+
return
14911497
}
14921498
if heat.MIMEType != "application/json" {
14931499
t.Errorf("temperature MIME type = %q, want application/json", heat.MIMEType)
@@ -1576,6 +1582,7 @@ func TestMcp_DoctorResource(t *testing.T) {
15761582
}
15771583
if doctor == nil {
15781584
t.Fatalf("resources/list missing remindb://doctor, got %d resources", len(listed.Resources))
1585+
return
15791586
}
15801587
if doctor.MIMEType != "application/json" {
15811588
t.Errorf("doctor MIME type = %q, want application/json", doctor.MIMEType)
@@ -1646,6 +1653,7 @@ func TestMcp_LogsResource(t *testing.T) {
16461653
}
16471654
if logs == nil {
16481655
t.Fatalf("resources/list missing remindb://logs, got %d resources", len(listed.Resources))
1656+
return
16491657
}
16501658
if logs.MIMEType != "application/json" {
16511659
t.Errorf("logs MIME type = %q, want application/json", logs.MIMEType)
@@ -2100,6 +2108,7 @@ func TestMcp_RescanResource(t *testing.T) {
21002108
}
21012109
if rescan == nil {
21022110
t.Fatalf("resources/list missing remindb://rescan, got %d resources", len(listed.Resources))
2111+
return
21032112
}
21042113
if rescan.MIMEType != "application/json" {
21052114
t.Errorf("rescan MIME type = %q, want application/json", rescan.MIMEType)

pkg/mcp/rescan/rescan.go

Lines changed: 18 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -285,27 +285,34 @@ func (r *Loop) scan(ctx context.Context) {
285285
return
286286
}
287287

288-
var deleted []string
288+
var deletedAbs []string
289+
var deletedRel []string
289290
for path := range r.modTimes {
290291
if seen[path] {
291292
continue
292293
}
293-
delete(r.modTimes, path)
294294

295295
rel, err := filepath.Rel(r.dir, path)
296296
if err != nil {
297297
rel = path
298298
}
299-
deleted = append(deleted, rel)
299+
deletedAbs = append(deletedAbs, path)
300+
deletedRel = append(deletedRel, rel)
300301
}
301302

302303
r.store.OpMu.Lock()
303304
defer r.store.OpMu.Unlock()
304305

305-
snap.PurgedFiles = r.reconcileDeleted(ctx, deleted)
306+
purged, ok := r.reconcileDeleted(ctx, deletedRel)
307+
snap.PurgedFiles = purged
308+
if ok {
309+
for _, abs := range deletedAbs {
310+
delete(r.modTimes, abs)
311+
}
312+
}
306313

307314
if len(changed) == 0 {
308-
if len(deleted) > 0 {
315+
if ok && len(deletedRel) > 0 {
309316
r.notifyChange()
310317
}
311318

@@ -344,18 +351,18 @@ func (r *Loop) scan(ctx context.Context) {
344351
r.notifyChange()
345352
}
346353

347-
func (r *Loop) reconcileDeleted(ctx context.Context, deleted []string) []rescanstat.PurgedFile {
354+
func (r *Loop) reconcileDeleted(ctx context.Context, deleted []string) ([]rescanstat.PurgedFile, bool) {
348355
if len(deleted) == 0 {
349-
return nil
356+
return nil, true
350357
}
351358

352359
nodes, err := r.store.GetNodesByFiles(ctx, deleted)
353360
if err != nil {
354361
r.logger.Error("rescan: load deleted nodes failed", "err", err)
355-
return nil
362+
return nil, false
356363
}
357364
if len(nodes) == 0 {
358-
return nil
365+
return nil, true
359366
}
360367

361368
deltas := make([]diff.Delta, 0, len(nodes))
@@ -380,7 +387,7 @@ func (r *Loop) reconcileDeleted(ctx context.Context, deleted []string) []rescans
380387
emitter.WithMessage(msg),
381388
); err != nil {
382389
r.logger.Error("rescan: purge emit failed", "err", err)
383-
return nil
390+
return nil, false
384391
}
385392

386393
r.logger.Info("rescan: purged deleted files",
@@ -395,5 +402,5 @@ func (r *Loop) reconcileDeleted(ctx context.Context, deleted []string) []rescans
395402
}
396403

397404
sort.Slice(purged, func(i, j int) bool { return purged[i].Path < purged[j].Path })
398-
return purged
405+
return purged, true
399406
}

pkg/mcp/rescan/rescan_test.go

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -438,6 +438,77 @@ func TestRescanLoop_SkipsPurgeOnWalkError(t *testing.T) {
438438
}
439439
}
440440

441+
func TestRescanLoop_RetainsModTimesOnPurgeEmitFailure(t *testing.T) {
442+
dir := t.TempDir()
443+
writeFile(t, dir, "keep.md", "# Keep\n")
444+
writeFile(t, dir, "gone.md", "# Gone\n\nBody.\n")
445+
446+
st := testutil.OpenTestDB(t)
447+
r := mustRescan(t, st, dir, time.Minute, nil)
448+
r.now = func() time.Time { return time.Now().Add(time.Hour) }
449+
450+
ctx := context.Background()
451+
r.scan(ctx)
452+
453+
before, err := st.GetNodesByFile(ctx, "gone.md")
454+
if err != nil {
455+
t.Fatalf("GetNodesByFile: %v", err)
456+
}
457+
if len(before) == 0 {
458+
t.Fatal("expected gone.md nodes after initial scan")
459+
}
460+
461+
if err := os.Remove(filepath.Join(dir, "gone.md")); err != nil {
462+
t.Fatal(err)
463+
}
464+
465+
failCtx, cancel := context.WithCancel(ctx)
466+
originalWalk := r.walkFn
467+
r.walkFn = func(root string, fn fs.WalkDirFunc) error {
468+
if err := originalWalk(root, fn); err != nil {
469+
return err
470+
}
471+
cancel()
472+
return nil
473+
}
474+
475+
snapsBefore, _ := st.ListSnapshots(ctx, 10)
476+
r.scan(failCtx)
477+
478+
if _, ok := r.modTimes[filepath.Join(dir, "gone.md")]; !ok {
479+
t.Error("modTimes entry for gone.md dropped despite failed purge emit")
480+
}
481+
482+
stillThere, err := st.GetNodesByFile(ctx, "gone.md")
483+
if err != nil {
484+
t.Fatalf("GetNodesByFile (after failed emit): %v", err)
485+
}
486+
if len(stillThere) == 0 {
487+
t.Error("gone.md nodes purged from DB even though emit failed")
488+
}
489+
490+
snapsAfter, _ := st.ListSnapshots(ctx, 10)
491+
if len(snapsAfter) != len(snapsBefore) {
492+
t.Errorf("snapshots changed: before=%d after=%d (emit failed, must not commit purge snapshot)",
493+
len(snapsBefore), len(snapsAfter))
494+
}
495+
496+
r.walkFn = originalWalk
497+
r.scan(ctx)
498+
499+
if _, ok := r.modTimes[filepath.Join(dir, "gone.md")]; ok {
500+
t.Error("modTimes entry for gone.md should be cleared after successful retry")
501+
}
502+
503+
after, err := st.GetNodesByFile(ctx, "gone.md")
504+
if err != nil {
505+
t.Fatalf("GetNodesByFile (retry): %v", err)
506+
}
507+
if len(after) != 0 {
508+
t.Errorf("orphan nodes after successful retry = %d, want 0", len(after))
509+
}
510+
}
511+
441512
func TestRescanLoop_NewFile(t *testing.T) {
442513
dir := t.TempDir()
443514

0 commit comments

Comments
 (0)