Repository navigation
fix(api): validate scroll searches the way SearchRequest#validate does - #2915
Merged
xerj-org merged 2 commits intoNov 4, 2026
Merged
Conversation
Motivation: ES refuses these scroll searches with one 400 action_request_validation_exception before any shard runs: - `size: 0` -> [size] cannot be [0] in a scroll context - `?request_cache=true` -> [request_cache] cannot be used in a scroll context - `from > 0` -> using [from] is not allowed in a scroll context - a `rescore` -> using [rescore] is not allowed in a scroll context - a `search_after` -> [search_after] cannot be used in a scroll context XERJ ran each one and opened a scroll context. `size: 0` opened a context that can never page; `from: 1` returned the very document it asked to skip, because the scroll snapshot resets `from` to 0. Two catch-lenient lines are this case (`scroll/10_basic.yml · Scroll cannot used the request cache`, `· Scroll with size 0`). Root cause: The scroll setup in `search_impl` overwrites `size`/`from` for the snapshot without first checking what the caller sent. Fix: `scroll_request_validation_errors` collects the ES messages in SearchRequest#validate order, and `search_impl` answers them before the open-scroll cap check, so a refused request never leaves a context behind. Several errors are numbered together (`Validation Failed: 1: ...;2: ...;`), as ES does. The existing api-key helper that renders this envelope is renamed `action_request_validation_error`, since nothing in it is api-key specific. Not mirrored on purpose: `track_total_hits: false` (XERJ forces exact scroll totals instead), and `collapse` in a scroll, which XERJ supports and renders on continuation pages (xerj-org#621-xerj-org#624); its catch-lenient line (`search/110_field_collapsing.yml · field collapsing and scroll`) stays. Evidence: - ES-YAML suite against a node built from this change (with its two sibling fixes): `1453 passed · 0 failed · 10 skipped`, no stale catch-lenient entries; the two `scroll/10_basic.yml` lines are deleted. - The collapse_scroll_* and scroll_* integration tests still pass.
Each of size 0 (body and `?size=0`), `?request_cache=true`, `from: 1`, a rescore and a search_after answers 400 with ES's message, size 0 plus request_cache numbers both errors, and no refused request leaves a scroll context behind. The same options without `scroll`, a scroll with `request_cache=false`, and a collapsed scroll still answer 200. On main the first test fails at the first case: left: 200 right: 400
Owner
|
Xerj Merger made the fix: validates scroll searches the way ES's SearchRequest#validate does, so invalid scroll requests are refused before a context opens. Diff reviewed, Build + Test green on ab7c697 (cargo-audit was red only from RUSTSEC-2026-0436, fixed on main by #2912). Thanks @amartya1523! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
AI-assisted change, run and reviewed by @amartya1523.
What this changes, and why
ES refuses these scroll searches in
SearchRequest#validate, before any shard runs, with one 400action_request_validation_exception:?scroll=size: 0(body or?size=0)[size] cannot be [0] in a scroll context?request_cache=true[request_cache] cannot be used in a scroll contextfrom > 0using [from] is not allowed in a scroll contextrescoreusing [rescore] is not allowed in a scroll contextsearch_after[search_after] cannot be used in a scroll contextXERJ ran each one and opened a scroll context.
size: 0opened a context that can never page.from: 1returned the very document it asked to skip, because the snapshot setup insearch_implresetsfromto 0 without first looking at what the caller sent. Twocatch-lenient.txtlines are this case (scroll/10_basic.yml · Scroll cannot used the request cache,· Scroll with size 0).Fix:
scroll_request_validation_errorscollects the messages in ES's order.search_implanswers them before the open-scroll cap check, so a refused request never leaves a context behind.Validation Failed: 1: …;2: …;), as ES does.api_key_validation_error, one caller) is renamedaction_request_validation_error, since nothing in it is api-key specific.Not mirrored, on purpose:
collapsein a scroll: XERJ supports it and renders continuation pages for it (collapse + scroll: continuation pages leak __xy_collapse_group__/__xy_collapse_spec__ into _source and never render inner_hits #621–collapse + scroll: continuation renders no inner_hits under _source: false (deferred from #622) #624,collapse_scroll_*tests). Its catch-lenient line (search/110_field_collapsing.yml · field collapsing and scroll) stays. If you'd rather match ES there, it's a one-line addition, but it would remove a feature you built.track_total_hits: false: XERJ forces exact totals for every scroll instead of rejecting it.Evidence
Wire, node built from main
b78004832vs this branch (one doc int):New test module
scroll_request_validation_tests(es_compat.rs), run against main first:Checks
cargo fmt --allquickprofile (cargo build --profile quick -p xerj-server, per docs/FAST_BUILDS.md). Not--release; see Not run.cargo test --profile quick -p xerj-api --lib: 417 passed, 0 failed (all three sibling fixes applied)1453 passed · 0 failed · 10 skipped · 1463 total, 65 cases catch-lenient (was 74), no stale-entry notes. Run withes-yaml-runner --dir tests/es-compat-yaml/yamlagainst aquicknode built from mainb78004832plus this PR and its two siblings (fix(api): reject avg/sum/min/max/stats on a non-numeric field like ES #2913, fix(api): answer 404 when a clear-scroll frees nothing #2914).scroll/10_basic.ymlcases now enforce it; their catch-lenient lines are deleted.quick):collapse_scroll_page_parity,collapse_scroll_source_false_repro,collapse_scroll_snapshot_version_torn_read,scroll_snapshot_cap_is_documented,scroll_source_suppression_parity,scroll_seq_no_torn_read,scroll_multi_index_identity,scroll_alias_multi_index_cap,search_scroll_*,slice_spec_is_validatedand the rest of the 31: all pass, except the replica-timing flake incluster_metadata_replicates, which does not scroll.Not run:
--releasebuilds and localclippy, for disk reasons (the machine is at 99%). Thequickprofile from FAST_BUILDS.md was used instead.Merge note: this and #2914 both delete adjacent lines in
catch-lenient.txt. Whichever lands second needs a trivial rebase, and I'll do it.Provenance
quickprofile, based on mainb78004832./{index}/_search_scroll?scroll=route opens its scroll in a separate handler and does not run this check. It sits insearch_implonly, the_search?scroll=path the YAML suite uses. I can extend it there if you want both routes to agree.