fix(search): repair shards after recovered faults - #1125
Conversation
keegancsmith
left a comment
keegancsmith
left a comment
There was a problem hiding this comment.
Ok I haven't deeply reviewed this but have some feedback I'd be interested in before deeply reviewing. Also sharing this bit of review an agent did for me which seems legit.
Medium: recovered reload panics leak mappings
The outer recovery boundary in shard_repair.go:103-112 cannot close a replacement opened inside shards.go:1301-1323. A fault during NewSearcher or mkRankedShard.List leaks the mmap; repeated retries leak additional mappings.
Fix: make ownership transfer panic-safe with a deferred close until installation succeeds.
| metricListShardRunning.Dec() | ||
| if r := recover(); r != nil { | ||
| logShardCrash("list", s, q, r, debug.Stack()) | ||
| ss.shardRepairs.schedule(s) |
There was a problem hiding this comment.
we also panic for corrupt shards, not just due to memory faults. Reloading the whole shard doesn't really help in this case.
I think you can inspect r to check if it implements interface { Addr() uintptr } then only attempt repair if its a mmap fault.
There was a problem hiding this comment.
Agreed. Repair is now limited to recovered values that implement both runtime.Error and Addr() uintptr. Corrupt-shard panics and a real nil-pointer panic are still contained, but neither schedules a reopen; both cases have regression tests.
| } | ||
| } | ||
|
|
||
| func (q *shardRepairQueue) schedule(searcher zoekt.Searcher) bool { |
There was a problem hiding this comment.
A bit of a race with the watcher in this implementation due to solely deduplicating by path. eg if shard A is repairing, the watcher installs B, and B faults, schedule(B) is rejected because A still owns that path’s inFlight entry. When A finishes, the queue reports ready even though B remains faulted.
There was a problem hiding this comment.
Good catch. Single-flight state is now keyed by the exact loaded Searcher instance; the path is only the reload target. The regression test blocks A’s repair, installs B at the same path, and verifies B can schedule and finish before A is released.
| q.mu.Lock() | ||
| currentKey, stillRegistered := q.entries[request.faulted] | ||
| if resolved || !stillRegistered || currentKey != request.key { | ||
| delete(q.unresolved, request.key) | ||
| } else { | ||
| if q.unresolved == nil { | ||
| q.unresolved = make(map[string]struct{}) | ||
| } | ||
| q.unresolved[request.key] = struct{}{} | ||
| } | ||
| delete(q.inFlight, request.key) | ||
| q.running-- | ||
| q.startPendingLocked() | ||
| q.mu.Unlock() |
There was a problem hiding this comment.
It isn't clear to me we will retry a shard to remark the shard as ready. IE if we are marked unready => no traffic => never gets a retry. Althought I might be misunderstanding this.
There was a problem hiding this comment.
You were right. Repair state no longer feeds Ready(). A failed reopen releases its single-flight slot and stays observable in the error log; later traffic can retry it instead of readiness withdrawing the traffic needed to trigger that retry.
610578a to
ca174e2
Compare
|
Also fixed the mapping leak from the review summary. |
2 participants
Summary
SearchandListboundariesThis addresses the shard-repair portion of #1106.
Behavior
The query that encountered the fault remains incomplete and retains its crash count. It is not retried or rewritten as successful. A later query can use the reopened shard.
Only runtime memory faults carrying
Addr() uintptrschedule repair. Existing corruption, bounds, and nil-pointer panics remain contained but do not repeatedly reopen unchanged corrupt bytes.Repairs reuse
loadShardand the existing replacement path. At most four reopen operations run concurrently per sharded searcher. Single-flight state is keyed by the exact loaded shard instance, so a watcher-installed generation at the same path can schedule its own repair while an older generation is still finishing.Repair state does not feed
Ready(): a failed attempt releases its slot so later traffic can retry, rather than withdrawing the traffic needed to trigger that retry. Errors and recovered reload faults remain logged.Controlled before/after
The Linux regression builds a real 20 MB, 700-document shard, loads it through
shardedSearcher, truncates the mapped backing file, publishes the original bytes at a new inode, and searches again without runningDirectoryWatcher.SIGBUSwithfatal error: faultSetPanicOnFaultcontainment only, without repair: survives but still returnsFileCount=0, Crashes=1after 15 secondsFileCount=700, Crashes=0in the next bounded repair cycleThis validates fault containment and actual
loadShard/conditional-replacement healing independently. It does not claim to fix the publication-lifetime root cause in #1109.Test plan
go test ./...go test -race ./searchgo vet ./...go test ./search, including real inaccessible-mapping coverage and the real-shard no-watcher repair test