Repository navigation
fix(api): answer 404 when a clear-scroll frees nothing - #2914
Open
amartya1523 wants to merge 3 commits into
Open
amartya1523 wants to merge 3 commits into
amartya1523 wants to merge 3 commits into
Conversation
Motivation:
Clearing a scroll id that was already cleared or never existed answered
200. ES keeps the body (`{"succeeded": true, "num_freed": 0}`) but sets
the status from it: ClearScrollResponse#status is NOT_FOUND when
`num_freed` is 0. Clients (and `scroll/11_clear.yml`, which clears the
same id twice and expects `catch: missing`) use that status to tell a
freed context from a stale id. Three catch-lenient lines are this case.
Root cause:
`clear_scroll` and `clear_scroll_path` always wrapped the body in a
plain `Json(..)`, i.e. 200.
Fix:
Both handlers return `clear_scroll_response(num_freed)`, which keeps
the body and answers 404 when nothing was freed. That covers the body
id, body id list, `?scroll_id=` list, `_all` and the
`/_search/scroll/{id}` path form. Fetching a cleared scroll already
answered 404 (`search_context_missing_exception`) and is unchanged.
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 three `scroll/11_clear.yml` lines are
deleted.
A second clear of the same id, the path form on a freed id and `_all` with nothing open answer 404 with `num_freed: 0`; a clear that frees at least one context (including a body list mixed with an unknown id, which wins over the query string) answers 200. On main the first test fails at the second clear: left: 200 right: 404
7 tasks done
…ear-scroll-404 # Conflicts: # engine/tests/es-compat-yaml/catch-lenient.txt
This branch has not been deployed
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
DELETE /_search/scrollanswered 200 when it freed nothing: the same id cleared twice, an expired id, or_allwith no open scrolls. ES keeps the body ({"succeeded": true, "num_freed": 0}) but sets the status from it.ClearScrollResponse#statusisNOT_FOUNDwhennum_freed == 0. Clients use that status to tell a freed context from a stale id.scroll/11_clear.ymlclears an id and then expectscatch: missingon clearing it again, and threecatch-lenient.txtlines are that step.Fix:
clear_scrollandclear_scroll_pathboth returnclear_scroll_response(num_freed). It keeps the body and answers 404 when nothing was freed. That covers every selector: body id, body id list,?scroll_id=list,_alland/_search/scroll/{id}. Fetching a cleared scroll already answered 404 (search_context_missing_exception) and is unchanged.Evidence
Wire, node built from main
b78004832vs this branch:New test module
clear_scroll_status_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 and the scroll-validation PR).scroll/11_clear.ymlcases now enforce it; their catch-lenient lines are deleted.quick): everyxerj-api/testsfile that opens or clears a scroll passes (scroll_, collapse_scroll_, search_scroll_*, rerank/judge stage HTTP, among 31 run).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 the sibling scroll-validation PR 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._allwith nothing open is also a 404 in ES. That follows from the samenum_freed == 0rule; no YAML case pins it.