Follow-up from CodeRabbit's review of #135 (thread). The refactor extracted this loop verbatim (search::resolve_hits), so the semantics predate it; changing them was out of that PR's scope.
Current behavior
resolve_hits (crates/b2-core/src/search.rs) takes the top limit entries of the fused RRF ranking first, then drops any whose note_for_chunk lookup misses. A miss can only happen when the chunk row vanished between the FTS/vector scan and resolution — i.e. a concurrent reindex replaced the note's chunks mid-query. In that window, a dead chunk occupying a top-limit slot means a live, lower-ranked candidate is never considered, and search returns fewer than limit hits than it could.
This race is legitimate, not theoretical: C1 (invariants.md; index-engine.md §3) promises readers are never refused while writers rebuild — b2 search during a b2 reindex &, or the desktop searching while the fs-watch pulse re-projects, is the documented posture.
Proposed change
Iterate the full fused ranking and stop once hits.len() == limit, so unresolved chunks are skipped over rather than counted against the budget:
for (chunk_id, score) in fused {
if let Some(note_b2id) = db::note_for_chunk(conn, chunk_id)? {
hits.push(Hit { chunk_id, note_b2id, score });
if hits.len() == limit { break; }
}
}
Steady-state results are identical (every top-limit chunk resolves); only the torn-index window changes, and per-hit resolution stays bounded — the loop still stops at limit, it just doesn't charge misses against it.
Scope note
The same drop-don't-backfill shape exists one level up: Vault::search_chunks skips a hit whose path/detail lookup misses (documented there as preferring a dropped hit over a half-resolved one), and Vault::search's note-dedup loop does the same. If the answer here is "backfill", those loops deserve the same one-pass treatment in the same PR; if it's "under-filling during a torn read is fine", resolve_hits' doc should say so the way search_chunks' already does.
(A limit == 0 early return, also suggested in the review, is a no-op — take(0)/an immediate break both yield nothing — so it's style, not behavior.)
Follow-up from CodeRabbit's review of #135 (thread). The refactor extracted this loop verbatim (
search::resolve_hits), so the semantics predate it; changing them was out of that PR's scope.Current behavior
resolve_hits(crates/b2-core/src/search.rs) takes the toplimitentries of the fused RRF ranking first, then drops any whosenote_for_chunklookup misses. A miss can only happen when the chunk row vanished between the FTS/vector scan and resolution — i.e. a concurrent reindex replaced the note's chunks mid-query. In that window, a dead chunk occupying a top-limitslot means a live, lower-ranked candidate is never considered, andsearchreturns fewer thanlimithits than it could.This race is legitimate, not theoretical: C1 (invariants.md; index-engine.md §3) promises readers are never refused while writers rebuild —
b2 searchduring ab2 reindex &, or the desktop searching while the fs-watch pulse re-projects, is the documented posture.Proposed change
Iterate the full fused ranking and stop once
hits.len() == limit, so unresolved chunks are skipped over rather than counted against the budget:Steady-state results are identical (every top-
limitchunk resolves); only the torn-index window changes, and per-hit resolution stays bounded — the loop still stops atlimit, it just doesn't charge misses against it.Scope note
The same drop-don't-backfill shape exists one level up:
Vault::search_chunksskips a hit whose path/detail lookup misses (documented there as preferring a dropped hit over a half-resolved one), andVault::search's note-dedup loop does the same. If the answer here is "backfill", those loops deserve the same one-pass treatment in the same PR; if it's "under-filling during a torn read is fine",resolve_hits' doc should say so the waysearch_chunks' already does.(A
limit == 0early return, also suggested in the review, is a no-op —take(0)/an immediatebreakboth yield nothing — so it's style, not behavior.)