Repository navigation
fix(api): enforce ES's per-index search request limits with ES's messages - #1859
Conversation
…ages Motivation: All eight `search/30_limits.yml` cases in `catch-lenient.txt` (xerj-org#1583) expect a 400 that XERJ did not return, or returned with a different message: a negative `from` (URL or body), `from + size` over `index.max_result_window`, a scroll batch over it, a rescore window over `index.max_rescore_window`, more `docvalue_fields` than `index.max_docvalue_fields_search`, and a `regexp` / `prefix` / `/regex/` query_string longer than `index.max_regex_length`. Root cause: - `?from=-1` failed the `usize` parse and got the generic "Failed to parse int parameter"; `{"from": -1}` failed serde with a type error. - The result window was only checked inside the engine, with its own message; a scroll request had its size rewritten to the snapshot cap first, so it was never checked at all. - Rescore window, docvalue_fields count and regex/prefix length were never checked (HTTP 200). Fix (es_compat.rs): - `?from` that parses as a negative integer -> ES's `illegal_argument_exception` `[from] parameter cannot be negative but was [-1]`; the body's `from` uses a deserializer that refuses a negative value with the same reason. - `find_index_request_limit_violation`, run per participating index next to the existing `max_script_fields` check, using the caller's `from`/`size` captured before the scroll rewrite, with ES 8.13's messages for: result window / scroll batch size, rescore window (defaulting to the result window, as ES does), docvalue_fields count (default 100), and regex / prefix length (default 1000, walking the whole query). Settings are read by a new `index_int_setting`, the generalisation of `index_max_result_window`'s three-spelling lookup. The engine's own window check is unchanged and stays as a backstop. Evidence: - Full ES-YAML suite against a node built from this commit: 0 failed, and the runner reported exactly the eight 30_limits entries stale; with them deleted and enforced: `1402 passed · 0 failed · 10 skipped`, catch-lenient 151 -> 143.
|
Thanks for this contribution — we'd like to merge it. Before we can, we need a signed Contributor License Agreement from: @amartya1523 It is a one-time thing per contributor, not per pull request. Read CLA.md, then open a small pull request adding your GitHub username to Once it is merged, comment |
|
@cla-bot check |
|
Re-checking the CLA status for every commit author on this pull request… |
The non-negative `from` deserializer read an i64, so from=u64::MAX failed as a JSON type error instead of ES's 'Result window is too large' (rerank_stage_http every_refusal_is_a_400_that_names_the_problem_and_sends_nothing). Accept u64 too, saturating to usize::MAX; negatives still get ES's message.
|
Pushed a small fix (7d5434b) on top: the new non-negative |
|
Xerj Merger made the fix: reviewed the full diff: ES's per-index search limits (result window and scroll batch, rescore window, docvalue_fields, regex and prefix length, negative |
AI-assisted change, run and reviewed by @amartya1523.
Burns down all 8
search/30_limits.ymllines of thecatch-lenient.txtratchet from #1583 (151 → 143). Independent of #1824 and #1854: different code (search_impland the search-body struct, not the aggregation validators), but the samecatch-lenient.txt, so the second to merge needs a trivial rebase there.What this changes, and why
Each case expects the 400 Elasticsearch 8.13 returns before running a search. XERJ either answered 200 or refused with a different message:
?from=-1Failed to parse int parameter [from] with value [-1][from] parameter cannot be negative but was [-1](illegal_argument_exception){"from": -1}expected usize)?from=10000(size 10)Result window is too large, from + size must be less than or equal to: [10000] but was [10010]. See the scroll api …?scroll=5m&size=10010Batch size is too large, size must be less than or equal to: [10000] but was [10010]. …window_size: 10001Rescore window [10001] is too large. It must be less than [10000]. …docvalue_fieldsvsmax_docvalue_fields_search: 2Trying to retrieve too many docvalue_fields. Must be less than or equal to: [2] but was [3]. …regexp//…/query_string of 1110 charsThe length of regex [1110] used in the Regexp Query request has exceeded the allowed maximum of [1000]. …prefixof 1110 charsThe length of prefix [1110] used in the Prefix Query request has exceeded …Fix (
es_compat.rs):from: a URLfromthat parses as a negative integer gets ES'sillegal_argument_exception. The body'sfromusesdeserialize_non_negative_from, which refuses a negative value with the same reason.find_index_request_limit_violation:max_script_fieldscheck, so it stays after theids-vs-window check that630_ids_query.ymlrelies on.from/size, captured before the scroll rewrite.index_int_setting, a generalisation ofindex_max_result_window's three-spelling lookup. The defaults are ES's: rescore window = result window, 100 docvalue_fields, 1000-character regex.es_compat::searchcaller (graph, memory) either uses small sizes or would already be refused by that engine check, so only the message changes for them.Evidence
Full ES-YAML suite against a node built from this commit. The runner reported exactly the 8
30_limits.ymlentries as stale, and 0 failed. With them deleted and enforced:The other limit cases in that file (
Script_fields size limit) and630_ids_query.ymlstill pass.Checks
cargo fmt --all -- --check(exit 0)cargo build --release -p xerj-server -p es-yaml-runner[limits] disk_flood_stage_percent = 0because this machine's disk is above the 95% flood stage.30_limits.ymlcases, now strictNot run:
cargo test -p xerj-api(release/fat-LTO test binaries do not fit this machine's free disk; CI runs them) and clippy.Scope note: these checks are in the
_searchpath (search_impl)._msearchand the template endpoints build their requests separately and are not covered here. No YAML case exercises them, and they keep the engine's window check.Provenance
query_stringregex case. The YAML only pinsThe length of regex [1110], and I reuse the Regexp-query sentence.