Fix non-transactional incremental update delete protocol

updateGraphWithoutTx (the path taken by the MCP build_or_update_graph /
parse_project tools, whose injected syncer is not transactional) passed
the full existing-file set to the first normal batch. Because
SyncWithExisting deletes existing files absent from the batch, a
multi-record spool deleted files belonging to later batches and re-added
them (churning node IDs, annotations, and stats); it also double-counted
deletions against the explicit delete pass, and skipped deletions
entirely when no normal batch ran. Mirror the transactional path: normal
batches carry no existing files and deletions run once, unconditionally.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Author: tae2089

internal/mcp/handlers_test.go

@@ -973,14 +973,12 @@ func TestBuildOrUpdateGraph_IncrementalIncludePaths_DefaultsToReplace(t *testing
 	mockSync := &mockIncrementalSyncer{result: &incremental.SyncStats{}}
 	deps.Incremental = mockSync
 
+	// Only handler.go is on disk; the out-of-scope other.go is missing. Under default replace
+	// semantics the delete pass must consider all namespace files, so other.go is deletable.
 	dir := t.TempDir()
 	os.MkdirAll(filepath.Join(dir, "src", "api"), 0o755)
-	os.MkdirAll(filepath.Join(dir, "src", "other"), 0o755)
 	writeGoFile(t, filepath.Join(dir, "src", "api"), "handler.go", `package api
 func Handler() {}
-`)
-	writeGoFile(t, filepath.Join(dir, "src", "other"), "other.go", `package other
-func Other() {}
 `)
 
 	callTool(t, deps, "build_or_update_graph", map[string]any{
@@ -994,16 +992,24 @@ func Other() {}
 	if !mockSync.syncWithExisting {
 		t.Fatal("expected Incremental.SyncWithExisting to be called")
 	}
-	if len(mockSync.existingCalls) == 0 {
-		t.Fatal("expected at least one SyncWithExisting call")
+	deleteExisting := deletePassExistingFiles(mockSync)
+	if deleteExisting == nil {
+		t.Fatalf("expected a delete pass for the missing out-of-scope file, calls=%v", mockSync.existingCalls)
 	}
-	firstExisting := mockSync.existingCalls[0]
-	if len(firstExisting) != 2 {
-		t.Fatalf("expected default replace semantics to pass all namespace files, got %v", firstExisting)
+	if !containsStringInSlice(deleteExisting, filepath.Join("src", "other", "other.go")) {
+		t.Fatalf("default replace must consider the out-of-scope file for deletion, got %v", deleteExisting)
 	}
-	if !containsStringInSlice(firstExisting, filepath.Join("src", "other", "other.go")) {
-		t.Fatalf("expected existingFiles to include out-of-scope file under default replace semantics, got %v", firstExisting)
+}
+
+// deletePassExistingFiles returns the existingFiles of the delete pass (the SyncWithExisting
+// call that received no files), or nil when no delete pass ran.
+func deletePassExistingFiles(m *mockIncrementalSyncer) []string {
+	for i := range m.filesCalls {
+		if len(m.filesCalls[i]) == 0 && len(m.existingCalls[i]) > 0 {
+			return m.existingCalls[i]
+		}
 	}
+	return nil
 }
 
 func TestBuildOrUpdateGraph_IncrementalIncludePaths_ReplaceFalsePreservesOutOfScopeFiles(t *testing.T) {
@@ -1020,14 +1026,13 @@ func TestBuildOrUpdateGraph_IncrementalIncludePaths_ReplaceFalsePreservesOutOfSc
 	mockSync := &mockIncrementalSyncer{result: &incremental.SyncStats{}}
 	deps.Incremental = mockSync
 
+	// The seeded handler.go and other.go are both missing from disk; a new in-scope file is
+	// added. With replace=false + include_paths, only the in-scope handler.go may be deleted,
+	// and the out-of-scope other.go must be scoped out of the existing-file set entirely.
 	dir := t.TempDir()
 	os.MkdirAll(filepath.Join(dir, "src", "api"), 0o755)
-	os.MkdirAll(filepath.Join(dir, "src", "other"), 0o755)
-	writeGoFile(t, filepath.Join(dir, "src", "api"), "handler.go", `package api
-func Handler() {}
-`)
-	writeGoFile(t, filepath.Join(dir, "src", "other"), "other.go", `package other
-func Other() {}
+	writeGoFile(t, filepath.Join(dir, "src", "api"), "new.go", `package api
+func New() {}
 `)
 
 	callTool(t, deps, "build_or_update_graph", map[string]any{
@@ -1042,15 +1047,15 @@ func Other() {}
 	if !mockSync.syncWithExisting {
 		t.Fatal("expected Incremental.SyncWithExisting to be called")
 	}
-	if len(mockSync.existingCalls) == 0 {
-		t.Fatal("expected at least one SyncWithExisting call")
+	deleteExisting := deletePassExistingFiles(mockSync)
+	if deleteExisting == nil {
+		t.Fatalf("expected a delete pass for the missing in-scope file, calls=%v", mockSync.existingCalls)
 	}
-	firstExisting := mockSync.existingCalls[0]
-	if containsStringInSlice(firstExisting, filepath.Join("src", "other", "other.go")) {
-		t.Fatalf("expected replace=false to exclude out-of-scope file from existingFiles, got %v", firstExisting)
+	if containsStringInSlice(deleteExisting, filepath.Join("src", "other", "other.go")) {
+		t.Fatalf("replace=false must exclude the out-of-scope file from deletions, got %v", deleteExisting)
 	}
-	if !containsStringInSlice(firstExisting, filepath.Join("src", "api", "handler.go")) {
-		t.Fatalf("expected replace=false to keep in-scope file, got %v", firstExisting)
+	if !containsStringInSlice(deleteExisting, filepath.Join("src", "api", "handler.go")) {
+		t.Fatalf("replace=false must still delete the missing in-scope file, got %v", deleteExisting)
 	}
 }
 

internal/service/indexer.go

@@ -1204,8 +1204,10 @@ func (s *GraphService) updateGraphWithoutTx(ctx context.Context, absDir string,
 	}
 
 	stats := &incremental.SyncStats{}
-	normalExistingFiles := existingFilesExcluding(existingFiles, forceFiles)
-	passedExistingFiles := false
+	// Normal batches must never carry existingFiles: SyncWithExisting deletes existing files
+	// absent from the batch, so a multi-record spool would delete files belonging to later
+	// batches (then re-add them, churning node IDs and stats). Deletions are handled once,
+	// explicitly, below — mirroring the transactional path (applyUpdateSpoolInTx).
 	for _, path := range spool.records {
 		record, err := spool.readRecord(path)
 		if err != nil {
@@ -1215,19 +1217,14 @@ func (s *GraphService) updateGraphWithoutTx(ctx context.Context, absDir string,
 		if len(normalFiles) == 0 {
 			continue
 		}
-		batchExistingFiles := []string(nil)
-		if !passedExistingFiles {
-			batchExistingFiles = normalExistingFiles
-			passedExistingFiles = true
-		}
-		batchStats, err := opts.Syncer.SyncWithExisting(ctx, normalFiles, batchExistingFiles)
+		batchStats, err := opts.Syncer.SyncWithExisting(ctx, normalFiles, nil)
 		if err != nil {
 			return nil, trace.Wrap(err, "incremental sync")
 		}
 		addSyncStats(stats, batchStats)
 	}
 	deletedFiles := existingFilesMissingFromSet(spool.currentFiles, existingFiles)
-	if len(deletedFiles) > 0 && passedExistingFiles {
+	if len(deletedFiles) > 0 {
 		batchStats, err := opts.Syncer.SyncWithExisting(ctx, nil, deletedFiles)
 		if err != nil {
 			return nil, trace.Wrap(err, "incremental delete sync")
@@ -1295,25 +1292,6 @@ func existingFilesMissingFromSet(currentFiles map[string]struct{}, existingFiles
 	return deleted
 }
 
-// existingFilesExcluding filters out paths excluded from the current sync pass.
-// @intent keep incremental sync from revisiting files the caller already ruled out.
-func existingFilesExcluding(existingFiles []string, exclude map[string]struct{}) []string {
-	if len(existingFiles) == 0 {
-		return nil
-	}
-	if len(exclude) == 0 {
-		return append([]string(nil), existingFiles...)
-	}
-	filtered := make([]string, 0, len(existingFiles))
-	for _, fp := range existingFiles {
-		if _, ok := exclude[fp]; ok {
-			continue
-		}
-		filtered = append(filtered, fp)
-	}
-	return filtered
-}
-
 // affectedNodeIDsForUpdate collects node IDs whose search documents must be refreshed for a given change set.
 // @intent merge previously stored node IDs with newly created ones so the search index sees both removals and additions.
 func affectedNodeIDsForUpdate(ctx context.Context, db *gorm.DB, existingNodesByFile map[string][]model.Node, changedFiles, deletedFiles []string) ([]uint, error) {

internal/service/indexer_test.go

@@ -2701,6 +2701,40 @@ func TestUpdate_MaxTotalParsedBytesRejectsBeforeSync(t *testing.T) {
 	}
 }
 
+func TestUpdateGraphWithoutTx_DeletesEvenWithNoNormalBatches(t *testing.T) {
+	db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{Logger: gormlogger.Discard})
+	if err != nil {
+		t.Fatalf("open db: %v", err)
+	}
+	if err := db.AutoMigrate(&model.Node{}); err != nil {
+		t.Fatalf("migrate nodes: %v", err)
+	}
+	if err := db.Create(&model.Node{Namespace: ctxns.DefaultNamespace, FilePath: "gone.go"}).Error; err != nil {
+		t.Fatalf("seed node: %v", err)
+	}
+
+	svc := &GraphService{
+		DB:      db,
+		Walkers: map[string]*treesitter.Walker{".go": treesitter.NewWalker(treesitter.GoSpec)},
+		Logger:  slog.Default(),
+	}
+
+	// Empty working dir: nothing to parse, so there are no normal batches. The previously
+	// stored gone.go must still be deleted (regression: the delete pass was gated on a normal
+	// batch having run).
+	tmpDir := t.TempDir()
+	syncer := &recordingIncrementalSyncer{result: &incremental.SyncStats{}}
+	if _, err := svc.Update(context.Background(), UpdateOptions{BuildOptions: BuildOptions{Dir: tmpDir, SkipSearchRebuild: true}, Syncer: syncer}); err != nil {
+		t.Fatalf("Update: %v", err)
+	}
+	if len(syncer.calls) != 1 {
+		t.Fatalf("expected a single delete pass, got %d calls: %v", len(syncer.calls), syncer.calls)
+	}
+	if got, want := syncer.calls[0].existingFiles, []string{"gone.go"}; !reflect.DeepEqual(got, want) {
+		t.Fatalf("delete pass existingFiles = %v, want %v", got, want)
+	}
+}
+
 func TestUpdate_IncludePaths_FiltersExistingFilesWhenReplaceFalse(t *testing.T) {
 	db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{Logger: gormlogger.Discard})
 	if err != nil {
@@ -2709,6 +2743,8 @@ func TestUpdate_IncludePaths_FiltersExistingFilesWhenReplaceFalse(t *testing.T)
 	if err := db.AutoMigrate(&model.Node{}); err != nil {
 		t.Fatalf("migrate nodes: %v", err)
 	}
+	// Both seeded files are missing from disk. handler.go is inside the include path and must be
+	// deleted; helper.go is outside it and must be scoped out of the existing-file set entirely.
 	if err := db.Create(&model.Node{Namespace: ctxns.DefaultNamespace, FilePath: filepath.Join("src", "api", "handler.go")}).Error; err != nil {
 		t.Fatalf("seed api node: %v", err)
 	}
@@ -2727,17 +2763,29 @@ func TestUpdate_IncludePaths_FiltersExistingFilesWhenReplaceFalse(t *testing.T)
 	if err := os.MkdirAll(apiDir, 0o755); err != nil {
 		t.Fatalf("mkdir api: %v", err)
 	}
-	if err := os.WriteFile(filepath.Join(apiDir, "handler.go"), []byte("package api\n\nfunc Handler() {}\n"), 0o644); err != nil {
-		t.Fatalf("write handler: %v", err)
+	// A new file under the include path so the update has something to parse; the seeded
+	// handler.go is absent from disk and should be detected as a deletion.
+	if err := os.WriteFile(filepath.Join(apiDir, "new.go"), []byte("package api\n\nfunc New() {}\n"), 0o644); err != nil {
+		t.Fatalf("write new: %v", err)
 	}
 
 	syncer := &recordingIncrementalSyncer{result: &incremental.SyncStats{}}
 	_, err = svc.Update(context.Background(), UpdateOptions{BuildOptions: BuildOptions{Dir: tmpDir, IncludePaths: []string{filepath.Join("src", "api")}}, Syncer: syncer, Replace: false})
 	if err != nil {
 		t.Fatalf("Update: %v", err)
 	}
-	if got, want := syncer.existingFiles, []string{filepath.Join("src", "api", "handler.go")}; !reflect.DeepEqual(got, want) {
-		t.Fatalf("existingFiles mismatch: got=%v want=%v", got, want)
+
+	var deleteCall *recordingSyncCall
+	for i := range syncer.calls {
+		if len(syncer.calls[i].files) == 0 && len(syncer.calls[i].existingFiles) > 0 {
+			deleteCall = &syncer.calls[i]
+		}
+	}
+	if deleteCall == nil {
+		t.Fatalf("expected a delete pass for the missing include-path file, calls=%v", syncer.calls)
+	}
+	if got, want := deleteCall.existingFiles, []string{filepath.Join("src", "api", "handler.go")}; !reflect.DeepEqual(got, want) {
+		t.Fatalf("delete pass existingFiles mismatch: got=%v want=%v (helper.go outside include path must be scoped out)", got, want)
 	}
 }
 
@@ -3045,11 +3093,10 @@ func TestUpdateGraphWithoutTx_DoesNotDeleteForcedFilesDuringNormalSync(t *testin
 	if _, ok := first.files["target.go"]; !ok {
 		t.Fatalf("expected changed target.go in normal sync files, got %v", sortedIncrementalFileKeys(first.files))
 	}
-	if slices.Contains(first.existingFiles, "source.go") {
-		t.Fatalf("expected forced source.go to be excluded from normal sync existingFiles, got %v", first.existingFiles)
-	}
-	if !slices.Contains(first.existingFiles, "deleted.go") {
-		t.Fatalf("expected deleted.go to remain in normal sync existingFiles, got %v", first.existingFiles)
+	// Normal sync passes no existingFiles: deletions are handled by the separate delete pass,
+	// so a normal batch can never delete files that belong to other batches (mirrors the tx path).
+	if len(first.existingFiles) != 0 {
+		t.Fatalf("expected normal sync to pass no existingFiles, got %v", first.existingFiles)
 	}
 	second := syncer.calls[1]
 	if len(second.files) != 0 {