perf(knowledge): overlap search candidate stages - #2312
Conversation
|
The dependency audit comes back clean, which is the thing that had to be true: the lexical leg never Two design choices deserve credit rather than silence. The one real behaviour change is contention that did not previously exist inside a single fn is_read_timeout(e: &RuntimeError) -> bool {
matches!(e, RuntimeError::Storage(khive_storage::StorageError::Timeout { .. }))
}( So under Tests cannot distinguish the two completion orders. One visibility note. The rename to |
ohdearquant
left a comment
ohdearquant
left a comment
There was a problem hiding this comment.
Automated review. Posted by this repository's automated pull-request review pipeline; this is not a human read and does not gate the merge by itself.
Verdict on head 8306c79: REQUEST-CHANGES, 1 blocking finding. Finding details are delivered to the review's recipients rather than posted here. Do not merge this head while blocking findings are outstanding; a pipeline comment on a newer head supersedes this one.
knowledge.search and knowledge.suggest run the ANN and lexical candidate stages concurrently under one shared request deadline. When the ANN leg was the one to exhaust that deadline, the joined result carried no record of it: the outcome defaulted to an empty, "healthy" state indistinguishable from an ANN leg that legitimately found nothing. If the lexical leg had already produced hits, the post-processing that follows treated the request as undegraded and re-touched the now-expired deadline anyway (a fresh embedding rerank in both verbs, plus a final deadline check in search), turning a successful lexical search into a verb-level timeout instead of returning the hits already in hand. The ANN candidate outcome now carries its own timeout flag, set only on the timeout arms of the ANN helper. Both verbs combine it with the lexical leg's timeout flag into one degraded condition that gates the rerank, the read-heavy post-processing, and the final deadline check — mirroring the fail-open path the lexical timeout already used — and report the ANN timeout alongside the existing lexical one in the response's degraded metadata. Also replaces a regression test that asserted on this file's literal source text (call-site spellings and occurrence counts) with a behavioural test: a fake embedder that returns different vectors for the query role versus the generic role, asserting on which candidate wins the fused ranking.
ohdearquant
left a comment
ohdearquant
left a comment
There was a problem hiding this comment.
Automated review. Posted by this repository's automated pull-request review pipeline; this is not a human read and does not gate the merge.
Verdict on current head: APPROVE, zero blocking findings. This is a comment, not an approval — a human reviewer decides whether to approve and merge.
Overlapping the vector and lexical candidate stages moved the vector search's read-deadline timeout onto a new arm that discarded any candidates it had already found, so a request whose deadline expired mid-search could return zero results even after real work completed. The vector candidate leg now carries forward whatever its most recently finished round produced when the deadline expires, matching the lexical leg's existing partial-result behavior, so a degraded response reports whatever either side actually found instead of failing open to nothing. Also tightens the stage-overlap regression test, which could pass for a lexical-first sequential handler due to a notification signal fired before anyone was listening for it, and extends the query-role coverage added for the overlap work to the third call site in section-scoring.
ohdearquant
left a comment
ohdearquant
left a comment
There was a problem hiding this comment.
Automated review. Posted by this repository's automated pull-request review pipeline; this is not a human read and does not gate the merge by itself.
Verdict on head 6c9e66d: REQUEST-CHANGES, 1 blocking finding. Finding details are delivered to the review's recipients rather than posted here. Do not merge this head while blocking findings are outstanding; a pipeline comment on a newer head supersedes this one.
…times out search_eligible_ann_with_refill widens the ANN candidate pool across rounds until enough eligible hits are hydrated or the request deadline expires. When the deadline expired right after a round's hydration call returned, the timeout branch returned that round's own hit set, even when its hydration had just failed (a missing row, or a reader-acquisition failure that strips every shell in the round) and an earlier round had already produced real, hydrated, eligible hits. That could hand back fewer hits than a prior round, or none at all, discarding completed work just because the deadline was noticed late. The deadline branch now unions the current round's hits with the previous round's, keeping the previous round's copy for any duplicate id, sorts by score, and truncates to the eligible target, so a hydration timeout on a later round can never erase a round that already completed. Adds a regression test that forces a first round to complete with two real, hydrated hits, fails the second round's hydration outright via a new test-only fault hook, and expires the deadline immediately after — asserting the two first-round hits survive with timed_out set.
ohdearquant
left a comment
ohdearquant
left a comment
There was a problem hiding this comment.
Automated review. Posted by this repository's automated pull-request review pipeline; this is not a human read and does not gate the merge by itself.
Verdict on head e3db0d3: REQUEST-CHANGES, 1 blocking finding. Finding details are delivered to the review's recipients rather than posted here. Do not merge this head while blocking findings are outstanding; a pipeline comment on a newer head supersedes this one.
… the hydrator at its seam The ANN refill loop's deadline branch only carried the immediately preceding round's hits forward. If an earlier round produced valid hydrated hits, a later round's hydration then failed for a reason unrelated to the deadline (clearing that round's hits), and a subsequent round hit the request read deadline, the earlier round's hits were lost even though they had already been safely hydrated. The loop now accumulates every hit that has hydrated successfully in any completed round, keyed by id, and the deadline branch returns from that accumulator instead of just the last round. A round that fails hydration for a non-deadline reason contributes no hits and therefore cannot evict an id an earlier round already placed in the accumulator. On a duplicate id the most recently hydrated round's entry wins, since ids are re-read from the canonical store every round and ANN scores are deterministic per id. The test-only hydration fault used to model this failure bypassed the hydrator entirely, clearing hit shells directly in the refill loop. It now lives inside the hydrator's own reader-acquisition seam, substituting a synthetic storage timeout so the failure count and the shell-stripping come from the hydrator's real error-handling branch rather than a duplicate of it. The deadline branch's fallback also sorted its full merged batch before truncating to the eligible target. It now selects the top entries with a bounded partition and sorts only the kept prefix, so the discarded tail no longer pays for a full sort.
ohdearquant
left a comment
ohdearquant
left a comment
There was a problem hiding this comment.
Automated review. Posted by this repository's automated pull-request review pipeline; this is not a human read and does not gate the merge by itself.
Verdict on head 979b745: REQUEST-CHANGES, 1 blocking finding. Finding details are delivered to the review's recipients rather than posted here. Do not merge this head while blocking findings are outstanding; a pipeline comment on a newer head supersedes this one.
1 participant
Summary
knowledge.searchandknowledge.suggestValidation
cargo test -p khive-pack-knowledge— 402 passed, 1 ignored; doctests passed separatelycargo check -p khive-pack-knowledge --all-targets --all-featurescargo clippy -p khive-pack-knowledge --all-targets --all-features -- -D warningscargo check --workspace --all-targetscargo clippy --workspace --all-targets -- -D warningscargo fmt --manifest-path crates/Cargo.toml --check --allCloses #1996