Repository navigation
Recover incomplete full-text highlight batches - #3247
kylebernhardy wants to merge 3 commits into
Conversation
Release cherry-pick
|
There was a problem hiding this comment.
Code Review
This pull request implements a retry mechanism for full-text search highlight tracing when a multi-record native trace exceeds its span budget. Incomplete multi-record traces are now discarded and retried in sequential halves under the original query deadline and cancellation, up to a maximum of 256 additional trace calls per search request. The PR also updates the design and user documentation, and adds comprehensive unit, integration, and lifecycle tests to verify this recovery behavior. There are no review comments, and I have no feedback to provide.
kriszyp
left a comment
There was a problem hiding this comment.
My review mentions that:
Halving is a sound Harper-side mitigation, but it discards every completed record in a failed batch and can spend up to 257 calls where the native side could report the completed-record prefix (or a per-record truncated flag) and let Harper retry only the remainder in one extra call. Worth filing against HarperFast/fulltext as the long-term fix; the singleton case (one record with > 1024 matches, e.g. a long text/plain blob and a frequent term) is unrecoverable from Harper at all and will keep 500-ing until that lands.
Is this worth addressing in the native side?
🤖 Reviewed with Claude
| if (!traced.complete) { | ||
| if (batch.length < 2) throw new ServerError('Full-text index returned incomplete highlights', 500); | ||
| if (remainingAdditionalTraceCalls < 2) | ||
| throw new ServerError( |
There was a problem hiding this comment.
Status-code consistency: this failure is a deterministic function of the request (result limit × match density), and the message tells the client how to change the request. That is the shape of a 4xx, and it is exactly how the sibling oversize-source check is classified (ClientError(..., 400) in traceEntryBatches/traceSourceBatches). As a 500 it will trip client/proxy retry policies for a request that can never succeed unchanged, and it will show up in error-rate alerts as a server fault.
The DESIGN note says 500 is retained deliberately; if the reason is "class name is the public error code and the singleton case was already ServerError", consider at least making this new branch a ClientError (400 or 422) since it is a new message anyway and nobody is coding against it yet. If there is a reason it must stay 500, a one-line rationale in the DESIGN paragraph would stop the next reviewer from asking the same question.
| }); | ||
| assertSearchActive(); | ||
| if (!traced.complete) { | ||
| if (batch.length < 2) throw new ServerError('Full-text index returned incomplete highlights', 500); |
There was a problem hiding this comment.
After this change this is the only unrecoverable branch, and it fires for a legitimate, deterministic input: a single record whose selected highlight fields hold more than 1024 matches of the query terms (easy with a text/plain blob and a common word, since maxTraceSourceBytes is far larger than 1024 tokens). The message gives the client nothing to act on, unlike the budget message two lines down, and it cannot be told apart from a native fault by an operator.
Suggest two small things: (1) append the same advice ("omit $highlights or narrow the query"), and (2) log once with the record id/field so an operator can see which record is un-highlightable. Longer term this needs a native-side fix (per-record truncation flag or caller-supplied span cap) — worth filing against the Fulltext package, since nothing Harper does with batching can recover a singleton.
| deadline: number, | ||
| assertSearchActive: () => void | ||
| ): Promise<void> { | ||
| let remainingAdditionalTraceCalls = MAX_ADDITIONAL_HIGHLIGHT_TRACE_CALLS; |
There was a problem hiding this comment.
Nit: this is declared before the two early returns (!highlighting, highlightFields.size === 0). Moving it below them keeps the no-highlight path allocation-free and reads as "budget belongs to the tracing phase". Trivial, flagging only because this runs per request.
⊙ Problem
Highlighted full-text searches can fail with
500 Full-text index returned incomplete highlightseven when each record fits the native limits. The trace span limit applies across a batch: 60 records with 17 matches each fit its 1,024 spans, while adding one match per record exceeds it. The affected adapter is already shipped in v5.3.2.💡 Solution
Discard incomplete multi-record trace results and retry sequential halves. The 60 × 18 example now returns all 60 records with complete highlights. Each child retains the original reader lease, execution deadline, cancellation and transaction checks.
$highlights⚖️ Alternatives
🔧 Changes
The private highlight path in resources/indexes/fullTextQueryIndex.ts retains the existing record/source-byte batching and allocates a bounded work stack only after an incomplete trace; normally complete traces allocate no recovery stack:
fullTextQueryIndex.ts:773
resources/indexes/DESIGN.md records these invariants. resources/FULL_TEXT.md explains the bounded recovery and the two failure messages. No schema, native binary, wrapper dependency, or harper-pro pointer changes are included. No companion public documentation PR is proposed: this is an internal recovery fix with unchanged search API and limits; the existing core full-text reference documents its failure behavior.
The shared allowance spans every leaf and source batch; the liveness callback centralizes cancellation and transaction checks before and after native work.
✅ Verification
maxTraceRecords: 100,maxTraceSourceBytes: 1048576andmaxTraceSpans: 1024; the real-native premise assertions passed in the executed suites.Comment generated by kAIle (GPT-6); posted via @kylebernhardy.
Release target: v5.3 — repairs the adapter already shipped in v5.3.2; proposed next patch milestone.
Related PRs: none found (partial scan: two large candidates could not be fully matched)
Complexity: complicated
Review-Coverage: authored=codex; ran=gemini,claude; adjudicated=domain; declined=cursor-grok,cursor-composer,cursor-kimi,cursor-muse; rounds=4; full=2 @ 357150c
Review-Attention: study ~20m (critical: fullTextQueryIndex.ts; decisions: recovery-budget-shape, incomplete-highlight-semantics, do-less-alternative) @ 357150c