Skip to content

perf: pre-compute query phrases once per rankAnswerEvidence call - #99

Merged
BigSimmo merged 1 commit into
mainfrom
copilot/task-87357024-1243497866-2b1ccdf0-c938-4268-b3ae-7d15785bf550
Jun 29, 2026
Merged

perf: pre-compute query phrases once per rankAnswerEvidence call#99
BigSimmo merged 1 commit into
mainfrom
copilot/task-87357024-1243497866-2b1ccdf0-c938-4268-b3ae-7d15785bf550

Conversation

Copilot AI commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Summary

  • phraseScore rebuilt the same token n-gram array for every scored result. With 30–50+ results per query, phrase construction was O(N × tokens²); this moves it to O(tokens²) by computing once and passing down.

Before:

// Called N times — phrases rebuilt from same tokens each time
function phraseScore(tokens: string[], haystack: string) {
  const phrases: string[] = [];
  for (let size = Math.min(4, tokens.length); size >= 2; size -= 1) { ... }
  ...
}

After:

// Built once in rankAnswerEvidence, passed to each answerEvidenceScore call
function buildQueryPhrases(tokens: string[]): string[] { ... }
function phraseScore(phrases: string[], haystack: string) { ... }

const phrases = buildQueryPhrases(tokens); // once per query
results.map(result => answerEvidenceScore(tokens, phrases, result, queryClass));

Verification

  • npm run verify:cheap
  • npm run verify:ui when UI, routing, styling, browser behavior, reduced-motion, or forced-colors behavior changed
  • npm run verify:release before release or handoff confidence claims
  • npm run format:check
  • npm run check:production-readiness when clinical workflow, privacy, environment, Supabase, source governance, or deployment behavior changed

Clinical Governance Preflight

Complete this section when the change touches ingestion, answer generation, search/ranking, source rendering, document access, privacy, production env, or clinical output.

  • Source-backed claims still require linked source verification before clinical use
  • No patient-identifiable document workflow was introduced or expanded without explicit governance approval
  • Supabase target remains Clinical KB Database (sjrfecxgysukkwxsowpy)
  • Service-role keys and private document access remain server-only
  • Demo/synthetic content remains clearly separated from real clinical sources
  • Source metadata, review status, and outdated/unknown-source behavior remain conservative
  • Deployment classification/TGA SaMD impact was checked when clinical decision-support behavior changed

Notes

  • No change in ranking behaviour — identical outputs, less work per query.
  • Remaining 9 identified improvements: Array.from(queryTerms) × 4 allocations per result in buildMatchExplanation; linear Array.includes membership checks in evidence.ts; DELETE+INSERT vs upsert in replaceSharedCacheRow; structuredClone on every cache read; redundant combined renormalization in resultTexts; eager buildClinicalTextSearchQuery in normalizedCacheQuery; no explicit response compression config; imageMatchScore re-splits source on each token; linear cache-key scan in invalidateRagCachesForOwner.

Extract buildQueryPhrases() from phraseScore() so the n-gram phrase
array is built once per query rather than once per scored result.
phraseScore() now accepts a pre-computed phrases[] instead of rebuilding
it from tokens on every call.

Before: rankAnswerEvidence scored N results, each calling
phraseScore(tokens, haystack) which rebuilt the same phrase array each
time — O(N × tokens²) redundant work.

After: phrases are computed once in rankAnswerEvidence and passed to
answerEvidenceScore → phraseScore — O(tokens²) total.

Typical search scores 30–50+ results, making this a meaningful
reduction in hot-path work for every search query.
Copilot AI requested a review from BigSimmo June 29, 2026 01:30
@BigSimmo
BigSimmo marked this pull request as ready for review June 29, 2026 01:57
@BigSimmo
BigSimmo merged commit eaf3c03 into main Jun 29, 2026
6 checks passed
@BigSimmo
BigSimmo deleted the copilot/task-87357024-1243497866-2b1ccdf0-c938-4268-b3ae-7d15785bf550 branch June 29, 2026 04:29
BigSimmo pushed a commit that referenced this pull request Jul 29, 2026
Codex review raised this as P1 on PR #1377 and it is correct. The L1-2 change
started resolveSearchScope concurrently with the rate-limit RPC and aborted it
on deny, on the claim that a throttled caller "still costs nothing". That claim
is false.

resolveSearchScope only returns without touching the database when there are no
filters and no explicit document ids (search-scope.ts:242,253). With either
present it enters the paginated `documents` loop at :269 plus the nested label
loop. An AbortSignal cancels the client request; it does not un-execute a
statement Postgres has already begun. `filters` is caller-controlled, so a
throttled caller could keep spending database capacity while collecting 429s —
the opposite of what admission control is for, and the wrong direction against
capacity-review.md:106-113, which names Postgres CPU under concurrency the
first soft failure.

Scope now sits behind admission again. Two parts of the original change are
kept because they are independent of the overlap and unambiguously correct:

- `signal: request.signal` is threaded into resolveSearchScope, so a client
  disconnect finally cancels its paginated queries. search-scope.ts:200,328
  always supported .abortSignal(...); this route never passed one.
- the `scope` stage is still reported in Server-Timing.

tests/answer-route-preamble.test.ts is inverted to the guard the reviewer
asked for: no scope query may begin before the limiter admits, and a denied
request (sent with filters, the shape that reaches the paginated loop)
dispatches none at all. Both cases fail against the overlapping shape.

The audit's L1-2 section and ledger #99 record the refutation so a later
latency pass does not rediscover the overlap; re-attempting it requires a
non-database admission gate ahead of the durable limiter.

Verification: verify:cheap exit 0 on the merged tree (423 files, 4278 passed /
4 skipped).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01361jh3eYVjJCzXWjAhdZiF
BigSimmo pushed a commit that referenced this pull request Jul 29, 2026
Confirms the squash merge landed by content: the diff between squash
commit f4a3f50 and branch tip bebc6c0 is empty, so nothing was orphaned
by the auto-merge race despite four concurrent pushes during the merge
window. Also records that the coverage follow-ups were renumbered
#98/#99 -> #106/#107 (main claimed #98-#105 concurrently) with both
rows verified intact, and that the three red `PR required` results were
concurrency cancellations rather than defects.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012YRCXgX4AWZ579bKN6sk6b
BigSimmo pushed a commit that referenced this pull request Jul 29, 2026
…iew batch

Eight review findings (1 Codex P2, 7 CodeRabbit), each verified against the
code before acting.

MOST CONSEQUENTIAL — the L2-3 "recall is byte-identical" claim was wrong, and
it was load-bearing. fetchDocumentTitleAliasRows (rag-candidate-sources.ts:482)
applies .limit(12) with no ORDER BY, so which twelve documents return is
plan-dependent; a new index can change the title-alias set feeding candidate
assembly. "No query text changes" is true, but recall does not follow from it.
That claim was the argument for keeping L2-3 out of canary territory, so the
gating is revised: the documents-list and (status,id) uses stay ordering-safe,
the RAG-path index is canary-gated unless the unordered .limit(12) is made
deterministic first — the cheaper fix, since an unordered LIMIT is latent
nondeterminism regardless of this work.

Operator SQL alone never reaches staging, DR, or local replay: migrations/ is
the source of truth and schema.sql only a mirror, so hand-run statements hit
the live database and nothing else, and a required_indexes registration would
fail on every replayed environment. Authoring the migration is now a required
part of #102, following the 20260717170000 idempotent pattern. This PR still
ships no migration (the #1312 objection), but the runbook no longer implies
the operator sequence is sufficient.

Test guard hardened: the ordering case anchored on a fixed 5 ms sleep, which
can expire before the handler reaches the limiter. It now waits for
consumeSubjectApiRateLimit to be entered, then asserts scope is untouched — the
same guarantee without the timing fragility.

Ledger: the 78e2beb record still described the reverted scope-vs-ratelimit
overlap. The ledger is append-only, so this appends a superseding record via
ledger:append --supersede rather than editing the row, per the contract.

Status wording: #102 is "runbook prepared", not done, while the operator steps
are pending; #105 separates shipped implementation from pending browser
verification; #98/#99/#102/#103/#105 restored to the execution queue with
their remaining actions. Fixed the MD038 malformed RAG-impact code span.

Verification: verify:cheap exit 0 (427 files, 4386 passed / 4 skipped);
check:branch-review-ledger pass (1232 records, no duplicates).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01361jh3eYVjJCzXWjAhdZiF
BigSimmo added a commit that referenced this pull request Aug 24, 2026
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.

2 participants