diff --git a/internal/mcp/edit_serialization.go b/internal/mcp/edit_serialization.go index c6664ccea..089d73230 100644 --- a/internal/mcp/edit_serialization.go +++ b/internal/mcp/edit_serialization.go @@ -171,6 +171,9 @@ func (s *Server) trackMutationTicket(ticket *indexer.MutationTicket) *mutationRe receipt.result = result receipt.completed = true receipt.mu.Unlock() + if result.Err == nil && result.Reindexed { + s.resolveSupersededFailedReceipts(receipt.path, receipt.generation, result) + } close(receipt.done) time.AfterFunc(mutationReceiptRetention, func() { s.mutationReceipts.Delete(receipt.id) @@ -179,6 +182,44 @@ func (s *Server) trackMutationTicket(ticket *indexer.MutationTicket) *mutationRe return receipt } +// resolveSupersededFailedReceipts resolves terminally failed receipts for a +// path once a later generation of the same path has been applied +// successfully. The graph then reflects newer bytes than the failed +// generation ever wrote, so the stale failure no longer describes a real +// freshness gap — keeping it would only fail freshness barriers that waiting +// cannot heal, because a terminal error never completes differently. +// +// The failed receipt is resolved in place rather than deleted: the +// mutation-commit ledger refreshes its graph half through +// mutationReceiptState, and a deleted receipt would leave that record +// reading "pending" forever. Stamping the superseding apply mirrors how +// completeMutationWaiters resolves earlier waiters with the later apply's +// result. Pending receipts and failures at or above the succeeded +// generation are left untouched. The succeeded result is passed by value so +// the sweep holds no lock besides the receipt it is stamping. +func (s *Server) resolveSupersededFailedReceipts(succeededPath string, succeededGeneration uint64, applied indexer.MutationResult) { + cleanPath := filepath.Clean(succeededPath) + s.mutationReceipts.Range(func(_, value any) bool { + other, ok := value.(*mutationReceipt) + if !ok { + return true + } + if other.generation >= succeededGeneration || filepath.Clean(other.path) != cleanPath { + return true + } + other.mu.Lock() + if other.completed && (other.result.Err != nil || !other.result.Reindexed) { + other.result = indexer.MutationResult{ + RequestedGeneration: other.generation, + AppliedGeneration: applied.AppliedGeneration, + Reindexed: true, + } + } + other.mu.Unlock() + return true + }) +} + func (r *mutationReceipt) outcome(pending bool) mutationReindexOutcome { r.mu.RLock() defer r.mu.RUnlock() @@ -313,16 +354,19 @@ waitLoop: } issues := make([]string, 0, len(receipts)) + hasTerminalFailure := false for _, receipt := range receipts { select { case <-receipt.done: outcome := receipt.outcome(false) switch { case outcome.Err != nil: + hasTerminalFailure = true issues = append(issues, fmt.Sprintf( "failed receipt=%s repo=%q path=%q generation=%d error=%q", receipt.id, receipt.repo, receipt.path, receipt.generation, outcome.Err.Error())) case !outcome.Reindexed: + hasTerminalFailure = true issues = append(issues, fmt.Sprintf( "failed receipt=%s repo=%q path=%q generation=%d error=%q", receipt.id, receipt.repo, receipt.path, receipt.generation, "reindex not confirmed")) @@ -352,6 +396,11 @@ waitLoop: } message += issue } + if hasTerminalFailure { + message += "; terminally failed generations do not recover by waiting — " + + "they clear when a later mutation of the same path succeeds or when " + + "the receipt retention lapses" + } return fmt.Errorf("%s", message) } diff --git a/internal/mcp/mutation_freshness_test.go b/internal/mcp/mutation_freshness_test.go index e632fe555..397cf85ff 100644 --- a/internal/mcp/mutation_freshness_test.go +++ b/internal/mcp/mutation_freshness_test.go @@ -104,6 +104,122 @@ func TestMutationFreshnessTerminalFailureFailsClosed(t *testing.T) { } } +func TestMutationFreshnessSuccessResolvesSupersededFailures(t *testing.T) { + s := &Server{mutationSafetyWait: time.Millisecond} + stale := pendingFreshnessReceipt(s, "receipt-stale-failed", "repo-a", "/repo-a/file.go", 6) + completeFreshnessReceipt(stale, indexer.MutationResult{ + RequestedGeneration: 6, + Err: errors.New("context deadline exceeded"), + }) + otherPath := pendingFreshnessReceipt(s, "receipt-other-path", "repo-a", "/repo-a/other.go", 7) + completeFreshnessReceipt(otherPath, indexer.MutationResult{ + RequestedGeneration: 7, + Err: errors.New("unrelated failure"), + }) + newer := pendingFreshnessReceipt(s, "receipt-newer-failed", "repo-a", "/repo-a/file.go", 12) + completeFreshnessReceipt(newer, indexer.MutationResult{ + RequestedGeneration: 12, + Err: errors.New("later failure"), + }) + + succeeded := pendingFreshnessReceipt(s, "receipt-success", "repo-a", "/repo-a/file.go", 9) + appliedResult := indexer.MutationResult{ + RequestedGeneration: 9, + AppliedGeneration: 9, + Reindexed: true, + } + completeFreshnessReceipt(succeeded, appliedResult) + s.resolveSupersededFailedReceipts("/repo-a/file.go", 9, appliedResult) + + if _, loaded := s.mutationReceipts.Load("receipt-stale-failed"); !loaded { + t.Fatal("superseded receipt was deleted; it must stay queryable for the mutation-commit ledger") + } + resolved := stale.outcome(false) + if resolved.Err != nil || !resolved.Reindexed || resolved.AppliedGeneration != 9 { + t.Fatalf("superseded failure was not resolved in place: %+v", resolved) + } + if outcome := otherPath.outcome(false); outcome.Err == nil { + t.Fatal("failure on an unrelated path was resolved") + } + if outcome := newer.outcome(false); outcome.Err == nil { + t.Fatal("failure newer than the succeeded generation was resolved") + } + + err := s.awaitMutationFreshnessForRepos(context.Background(), "repo-a") + if err == nil { + t.Fatal("remaining failures did not fail closed") + } + message := err.Error() + if strings.Contains(message, "receipt-stale-failed") { + t.Fatalf("freshness error still reports the superseded receipt: %s", message) + } + for _, want := range []string{ + "receipt-other-path", + "receipt-newer-failed", + "do not recover by waiting", + } { + if !strings.Contains(message, want) { + t.Fatalf("freshness error %q does not contain %q", message, want) + } + } +} + +func TestTrackMutationTicketResolvesSupersededFailures(t *testing.T) { + s := &Server{} + stale := pendingFreshnessReceipt(s, "receipt-stale-failed", "", "/repo/file.go", 3) + completeFreshnessReceipt(stale, indexer.MutationResult{ + RequestedGeneration: 3, + Err: errors.New("context deadline exceeded"), + }) + + done := make(chan indexer.MutationResult, 1) + ticket := &indexer.MutationTicket{Path: "/repo/file.go", Generation: 5, Done: done} + receipt := s.trackMutationTicket(ticket) + done <- indexer.MutationResult{ + RequestedGeneration: 5, + AppliedGeneration: 5, + Reindexed: true, + } + close(done) + <-receipt.done + + if _, loaded := s.mutationReceipts.Load("receipt-stale-failed"); !loaded { + t.Fatal("stale receipt was deleted; it must stay queryable for the mutation-commit ledger") + } + resolved := stale.outcome(false) + if resolved.Err != nil || !resolved.Reindexed || resolved.AppliedGeneration != 5 { + t.Fatalf("stale failed receipt not resolved after a successful ticket: %+v", resolved) + } + if _, loaded := s.mutationReceipts.Load(receipt.id); !loaded { + t.Fatal("successful receipt itself was dropped before retention") + } +} + +func TestMutationFreshnessSuccessKeepsPendingSamePathReceipts(t *testing.T) { + s := &Server{mutationSafetyWait: time.Millisecond} + inflight := pendingFreshnessReceipt(s, "receipt-inflight", "repo-a", "/repo-a/file.go", 4) + + appliedResult := indexer.MutationResult{ + RequestedGeneration: 9, + AppliedGeneration: 9, + Reindexed: true, + } + succeeded := pendingFreshnessReceipt(s, "receipt-success", "repo-a", "/repo-a/file.go", 9) + completeFreshnessReceipt(succeeded, appliedResult) + s.resolveSupersededFailedReceipts("/repo-a/file.go", 9, appliedResult) + + if _, loaded := s.mutationReceipts.Load("receipt-inflight"); !loaded { + t.Fatal("a still-pending receipt for the same path was dropped by the resolve") + } + if outcome := inflight.outcome(true); outcome.Pending != true { + t.Fatalf("a still-pending receipt was marked completed by the resolve: %+v", outcome) + } + err := s.awaitMutationFreshnessForRepos(context.Background(), "repo-a") + if err == nil || !strings.Contains(err.Error(), "receipt-inflight") { + t.Fatalf("barrier no longer reports the in-flight receipt: %v", err) + } +} + func TestMutationReposForSymbolIDsUnresolvedWidensBarrier(t *testing.T) { g := graph.New() g.AddNode(&graph.Node{