fix(search): hybrid max via reduce + LRU doc-cache (#227)

[ranxianglei]

Fixes #227.

Two independent kernel-layer costs in the searchBlocks path. Both were reproduced locally on the current tree before changing anything.

Item 1 (P1): hybridAlgorithm.score hard-crashes at large doc counts

src/search/algorithms/hybrid.ts normalized the two channel scores with an argument spread:

const maxBm = Math.max(...bm.map((r) => r.score), 1e-9)

The spread throws RangeError: Maximum call stack size exceeded once the doc count exceeds V8's argument limit (~65K–125K depending on build). Reproduced: 160,170 docs → the search tool throws instead of degrading, before returning any result.

Fix: compute the max with reduce (O(n), no argument limit, no extra allocation):

const maxBm = bm.reduce((m, r) => (r.score > m ? r.score : m), 1e-9)

This was the only argument-spread hazard in the codebase (audited every Math.max/min(...spread) and array spread).

Item 2: docFeatures 8MB cap re-tokenizes large corpora on every call

The doc-features cache evicted in insertion order (FIFO), keyed by full doc text. A corpus larger than the 8MB cap re-tokenized (corpus − cap) on every call. Reproduced: 12.0MB corpus / 27,741 docs / 8MB cap → cold 7242ms, warm 7524ms (warm/cold = 104%); cache pinned at 8.0MB.

Fix (design call — chose LRU over per-scope):

• Eviction is now LRU: a cache hit re-inserts the doc at the tail, so the docs a host keeps re-querying are the ones retained (strictly better than FIFO, no interface change).
• setDocCacheCap(≥corpus) / Infinity is now documented + tested as the opt-in for a single-session host to cache the whole corpus once and make later calls O(docs × query-terms) — this is what the host needs to stop re-tokenizing, without a per-scope cache redesign.

Why not per-scope (key by doc ref): it would require threading a scope through the stateless public SearchAlgorithm.score(docs, query) contract — either a breaking change for custom algorithms, or a fragile non-async-safe module-level "active scope". That's a lot of surface for a multi-session-server use case that doesn't exist yet. The single-session host already gets full-corpus caching via the cap knob.

The hybrid double pass (BM25 + fuzzy both scan all docs, then applyRoleWeight/buildResults each build a full Map) is left as-is: it's fine once features are warm (~140ms at 20K docs), per the issue.

Tests

• hybrid: no RangeError at large doc counts — 160K docs (the crash scale) returns results instead of throwing.
• LRU: re-accessing a doc keeps it alive (FIFO would evict it) — pins the LRU-vs-FIFO difference via object identity.
• setDocCacheCap(Infinity): whole corpus cached, no eviction — pins the full-corpus opt-in.
• Renamed the eviction test to "evicts least-recently-used".

Full suite: 583 pass, 0 fail. Typecheck clean, build succeeds.

[github-actions]

📦 Built Package Artifact

Branch: 2026-09-09_search-hybrid-crash-doccache-lru (becded6)

Option A — Install from npm PR tag (recommended)

In your adapter project:

npm install acp-kernel@pr-229

Each push to this PR publishes a new version under the pr-229 npm tag.

Option B — Download artifact

1. Download the artifact from the Actions run
2. Extract the tarball and install:

tar xzf acp-kernel-pr229.tgz
npm install ./package

---

This comment is automatically updated on each push.

[ranxianglei]

🤖 Powered by ework · qwen3.8-27b

Reviewed PR #229 against the live tree — reproduced both bugs pre-change, then re-ran the full suite on the PR branch. Verdict: mergeable. Both fixes are correct; 583/583 tests pass, typecheck clean, build green. Two notes below: a framing clarification on P2, and a minor finding the "only hazard" claim misses.

P1 — hybrid Math.max(...spread) → reduce: verified.

• Pre-PR tree, 160K docs → RangeError: Maximum call stack size exceeded (search throws, returns nothing). Post-PR: returns 10 correctly-scored results in ~1.2s.
• The reduce is semantically identical to the old Math.max(...scores, 1e-9), including the empty-array → 1e-9 floor. No behavior change at small N.
• The new 160K-doc test is a valid crash-scale regression test (throws on old, passes on new).

P2 — LRU + cap knob: correct, but one framing point.

• LRU implementation is sound (hit → re-insert at tail; eviction takes the head); strictly ≥ FIFO.
• But LRU alone does not fix the reported symptom. I measured a 12MB corpus under the 8MB default cap: old FIFO warm/cold ≈ 132%, new LRU warm/cold ≈ 137% — no improvement. The host re-scores the same corpus in the same order every call, so with cap < corpus the 8MB window slides and every doc is a miss each pass, FIFO or LRU.
• The actual fix is the cap knob (setDocCacheCap(≥corpus) / Infinity): I measured warm/cold ≈ 14% (7× faster), whole corpus cached once. That's what the host needs, and the PR correctly documents + tests it.
• Net: the host must still set the cap ≥ corpus to get the win. LRU is a genuine robustness improvement for varied/subset access, but the "chose LRU" framing could lead a host to expect the upgrade alone to stop re-tokenization. Suggest one line in the host-facing note: "set the cap; LRU does not by itself stop same-order full-scan re-tokenization."

Minor — the "only argument-spread hazard" claim is inaccurate.

• src/search/tokenizer.ts:102: tokens.push(...cjkRunTokens(segs)) is the same class (spread into a function call). Verified push(...) throws RangeError at 150K args on this Node build (same as Math.max); array-literal spread [...] is safe.
• Trigger: a single doc with a very long contiguous CJK run — all-OOV run >~75KB (cjkRunTokens returns ~2L tokens) or a dictionary run >~300KB. Far rarer than the hybrid case (which only needed 65K+ small docs), so lower severity, but a real latent crash in the search path.
• Trivial follow-up: replace the push(...) with a for...of. The other function-call spreads (src/decompress.ts:74/82, src/compress.ts:347/361) are the same class but bounded by block/warning counts — very unlikely to trip, worth a glance in the same pass.

Merge cleanliness: PR is based on 04bd5ed; master is 32 commits ahead but none touch the 4 changed files, so it should merge cleanly (dry-run merge: no conflicts).

Happy to open a follow-up issue for the tokenizer push(...) if you want it tracked.