From 812d5e36fcb3151a33b000bf059187bba2fce9e1 Mon Sep 17 00:00:00 2001 From: Keegan Carruthers-Smith Date: Thu, 21 Nov 2024 12:17:58 +0200 Subject: [PATCH] shards: handle failed cached List for selectRepoSet If we failed to List the repositories when loading a shard we would never search it due to selectRepoSet optimization. In practice this feels very rare to happen (only for logic error or disk corruption). However, in those cases we should surface these crashes searches by attempting to search the shard. Additionally I add logging so we can notice when this happens. I didn't add a metric since this is the sort of thing that I think is so rare we would never think to check the metric (but may notice logs). Note: I used the slightly tricky invariant that repos being nil means error. If the shard is actually empty (eg all repos tombstoned) then we still correctly apply the optimization. In practice having an empty shard shouldn't really happen so I'm open to just treating empty repos list as something we have to search. --- shards/shards.go | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/shards/shards.go b/shards/shards.go index 309ac17e..271c20e1 100644 --- a/shards/shards.go +++ b/shards/shards.go @@ -180,6 +180,8 @@ type rankedShard struct { // We have out of band ranking on compound shards which can change even if // the shard file does not. So we compute a rank in getShards. We store // repos here to avoid the cost of List in the search request path. + // + // repos is nil only if that call failed. repos []*zoekt.Repository } @@ -447,7 +449,13 @@ func doSelectRepoSet(shards []*rankedShard, and *query.And) ([]*rankedShard, que filteredAll := true for _, s := range shards { - if any, all := hasRepos(s.repos); any { + if s.repos == nil { + // repos is nil if we failed to List the shard. This shouldn't + // happen, but if it does we don't know what is in it and must search + // it without simplifying the query. + filtered = append(filtered, s) + filteredAll = false + } else if any, all := hasRepos(s.repos); any { filtered = append(filtered, s) filteredAll = filteredAll && all } @@ -1066,9 +1074,7 @@ func mkRankedShard(s zoekt.Searcher) *rankedShard { q := query.Const{Value: true} result, err := s.List(context.Background(), &q, nil) if err != nil { - return &rankedShard{Searcher: s} - } - if len(result.Repos) == 0 { + log.Printf("mkRankedShard(%s): failed to cache repository list: %v", s, err) return &rankedShard{Searcher: s} }