Skip to content

fix(knowledge): bound distinct FTS terms per request, not per candidate fetch - #2382

Draft
ohdearquant wants to merge 1 commit into
knowledge/search-score-provenancefrom
knowledge/search-term-fan-out-bound
Draft

fix(knowledge): bound distinct FTS terms per request, not per candidate fetch#2382
ohdearquant wants to merge 1 commit into
knowledge/search-score-provenancefrom
knowledge/search-term-fan-out-bound

Conversation

@ohdearquant

Copy link
Copy Markdown
Owner

Bounds distinct FTS terms per request rather than per candidate fetch.

  • FTS_TERM_COUNT_LIMIT (32) is a request-wide budget carried on the search context, shared by the full query and both decomposed sub-queries, so a decomposed search never fans out to three times the limit.
  • The eligibility probe joins the same possibly-truncated term set the per-term loop searched, so a truncation-caused miss reports as no_match with terms_truncated: true, never as filtered.
  • Tests cover the shared budget across all three lexical legs and the filtered-versus-truncated distinction.

Stacked on the score-provenance change; targets that branch until it merges.

…te fetch

A knowledge.search query with many distinct terms turned one request into
one FTS MATCH statement per term, bounded only by the request read
deadline. Cap the number of distinct scoreable terms one request can spend
on full-text lookups, and share that budget across every lexical fetch a
single request makes: query decomposition can call the fetch up to three
times (the full query plus two sub-queries), and without sharing, each of
those would get its own allowance and triple the effective cap.

candidate_provenance.terms_truncated reports when a query exceeded the
budget. The raw existence probe that distinguishes a true miss from a
match removed by status/kind filtering now scopes itself to the same
(possibly truncated) term set the candidate fetch searched, so a
truncation-caused miss is never mislabeled as filtered.
@ohdearquant

Copy link
Copy Markdown
Owner Author

Measured against the current served build, before this lands as the fix for the reported
degraded.lexical_timeout behaviour.

Term count does not drive the flag on the search path. A nested-prefix ladder, holding the
topic constant so that every longer query contains every shorter one and only the term count
varies, produced no degraded key on any knowledge.search call: 18 runs from one to twelve
terms, across two interleaved passes. Carrier and result limit were excluded in the same way,
with the identical queries run over both the MCP and CLI paths at limits 2 and 3. Total request
wall time does not separate the two classes either; the slowest clean run was slower than most
flagged runs, which is what a stage-scoped allowance rather than a request-scoped one looks like
from outside.

The cap would not bind the reported workload. The queries in the incident expand to about 20
terms, below the 32-term cap this change introduces, and they are not decomposed, so their
lexical work would be unchanged by it.

What does vary is how much the stage reads. A single search returning three results was
measured making the serving process pull between 16 KB and 337 MB off disk, against a database of
roughly 9 GB. That four-order-of-magnitude spread, not the term count, is what tracks the flag.

None of this argues against the change on its own terms: bounding long-query fan-out is a real
and separate concern, and the per-candidate versus per-request distinction in the title is
correct. The narrow point is that merging it should not be recorded as resolving the reported
timeout, because two independent methods, a live measurement ladder and a source read of the
stage's own budget, agree that it would not.

@ohdearquant

Copy link
Copy Markdown
Owner Author

Correction to my previous comment, on one sentence in it.

I wrote that a single search returning three results was measured making the serving process pull
between 16 KB and 337 MB off disk. That sentence attributes to one request a counter that belongs
to a whole process. The measurement differenced a per-process disk-read counter across an interval
enclosing each request, on a daemon that serves every client on the machine. The defensible
statement is narrower: the daemon's disk-read counter rose by up to that amount during an interval
enclosing a request. Attribution to the request, to any particular database file, or to the lexical
stage specifically does not follow from it, and I stated the shared-process caveat in my own notes
and then wrote the attributed version anyway.

The rest of that comment stands. The term ladder, the exclusion of carrier and result limit, and
the observation that the queries in question expand to about 20 terms and so would not be bound by
a 32-term cap are all direct readings of the response envelope rather than inferences from a shared
counter.

The narrow conclusion is unchanged and does not depend on the retracted sentence: this change
should not be recorded as resolving the reported timeout.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant