Clarify the search in-flight ceiling's dual role after live-search coalescing - #6065
Clarify the search in-flight ceiling's dual role after live-search coalescing#6065lukemelia wants to merge 2 commits into
Conversation
The admission gate admits before the body is parsed, so it can't tell a cheap request (a live-search-cache coalesced join or TTL hit) from an expensive fresh compute. That makes SERVER_MAX_IN_FLIGHT_SEARCHES two bounds in one: it caps request concurrency, and it is still the heap bound for distinct searches — the worst case is that many concurrent multi-MB result documents. The live-search cache collapses identical bursts to ~one document but does nothing for distinct concurrent queries, so raising this ceiling to be friendlier to identical bursts would raise the distinct-query worst case and re-expose the heap exhaustion the bound exists to prevent. Document that, so the number isn't mistaken for a pure heap figure and raised. Comment-only; the default stays 30. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014GsYGsuCqHJz9GAti4jheG
Host Test Results 1 files 1 suites 2h 40m 52s ⏱️ Results for commit 046a7a3. Realm Server Test Results 1 files ± 0 210 suites +3 1h 14m 46s ⏱️ - 2m 36s Results for commit 046a7a3. ± Comparison against earlier commit 2df2a3d. |
There was a problem hiding this comment.
🟡 Changes recommended
Address the two outstanding nit findings in the search-bound comments.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This comment-only PR clarifies the dual concurrency and heap-protection role of SERVER_MAX_IN_FLIGHT_SEARCHES while preserving the default of 30 and runtime behavior.
Changes:
- Documents distinct-query heap protection.
- Explains interaction with live-search coalescing.
- Clarifies why identical bursts should not raise the ceiling.
File summaries
| File | Summary and review findings |
|---|---|
packages/runtime-common/search-bounds.ts |
Updates search-bound rationale comments. Two outstanding nits: qualify cache scope to live /_federated-search traffic (3 votes), and describe retries as only possibly becoming cache hits when retained (3 votes). |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // at once, across every caller. Each distinct in-flight search holds tens | ||
| // of MB of heap while its result set is assembled, so this is the number | ||
| // that decides whether a burst of distinct searches exhausts the heap | ||
| // (identical ones share one document via the live-search cache). Enforced | ||
| // at admission in the realm-server's request middleware; arrivals above the | ||
| // ceiling wait briefly for a slot and are then shed with 429 + Retry-After. |
There was a problem hiding this comment.
[Claude Code 🤖] Good catch — fixed in 046a7a3. The parenthetical now reads "identical live /_federated-search requests can share one document via the live-search cache, which is wired into that handler only; per-realm /_search calls, also gated here, assemble independently," so the heap rationale stays correct for the non-federated gated path.
| // exhaustion this bound exists to prevent — identical-burst overflow is instead | ||
| // shed and safely retried into a cache hit. Tune per environment against the | ||
| // distinct-query heap cost, never against identical-burst volume. |
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 046a7a3. Reworded to a possible hit: overflow "is instead shed and retried, which lands as a cache hit only while the first response is still retained (a non-zero LIVE_SEARCH_CACHE_TTL_MS, body within LIVE_SEARCH_CACHE_MAX_BYTES); otherwise the retry re-assembles the document" — so it no longer overstates the default protection.
The in-flight ceiling's doc comments implied the live-search cache benefits every gated search and that a shed request always retries into a cache hit. Neither holds: the gate admits both `/_search` and `/_federated-search`, but `LiveSearchCache` is wired only into the federated handler, so per-realm `/_search` calls assemble independently; and a retry is a hit only while the first response is still retained (non-zero LIVE_SEARCH_CACHE_TTL_MS, body within LIVE_SEARCH_CACHE_MAX_BYTES), otherwise it re-assembles the document. Qualify both comments accordingly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014GsYGsuCqHJz9GAti4jheG
Fixes CS-12916.
What
Comment-only change to
SERVER_MAX_IN_FLIGHT_SEARCHESinpackages/runtime-common/search-bounds.ts. The default stays 30; no behavior changes.Why
Two heap-protection changes recently landed on
_federated-search:SERVER_MAX_IN_FLIGHT_SEARCHES, default 30) — caps concurrent searches, shedding the excess with 429 + Retry-After before the body is parsed so a shed is cheap;join) or a short-TTL body (hit).Read together, the gate's original rationale ("each in-flight search holds tens of MB, so cap at ~30") invites a wrong conclusion: "identical bursts are now cheap, so raise the ceiling." That's unsafe. Because the gate admits before body-parse, it cannot tell a cheap
join/hitfrom an expensive distinctmiss, so30is doing two jobs at once:The live-search cache collapses identical bursts to ~one document, but does nothing for distinct concurrent queries. So raising the ceiling to be friendlier to identical bursts would also raise the distinct-query worst case and re-expose the OOM the gate exists to prevent. Identical-burst overflow is instead safely shed and retried into a cache hit (~1s added latency on the overflow, no heap risk).
This PR spells that out in the comment so the number isn't mistaken for a pure heap figure and bumped.
Not in scope
Actually admitting more identical-burst traffic would require not charging a heap slot for a
join/hit— i.e. knowing the cache-key outcome before consuming a slot, which conflicts with the gate's before-body-parse cheapness. Tracked as a design option in CS-12916 only if identical-burst 429s show up as painful in telemetry.Test plan
Comment-only; no runtime surface. Typecheck/lint on the changed file only.
🤖 Generated with Claude Code