Cap search page depth so from + size cannot exceed max_result_window
A deep advanced-search page returned a 500; it now returns an empty page. ?page=1000&per_page=200 built an Elasticsearch body with from: 199800, size: 200, which exceeds index.max_result_window (default 10,000) — Elasticsearch answers HTTP 400, and because that error is unrescued on the user-facing search path it surfaced as a 500.
| Before | After |
|---|---|
| Advanced search past 10,000 results: HTTP 500 | Empty page at the requested offset, X-Page = the requested page, no rel="next" |
Page depth bounded from below only (page = [1, params[:page].to_i].max) |
Also bounded above, for advanced search only |
| Basic search / exact code search | Unchanged — neither has a result window, both still serve deep pages |
- Fixed at the source, not in the search client. Rescuing
BadRequestinee/lib/gitlab/search/client.rbwould regress three existingrescue BadRequestcall sites that need the raw error. No out-of-window body is ever built now, for any backend. - An empty page, not a clamp. Clamping
pageto the last reachable page makes the headers lie:OffsetPagination#needs_pagination?(lib/gitlab/pagination/offset_pagination.rb) re-pages fromparams[:page], sopage=1000came back withX-Page: 1000andX-Next-Page: 1001over page 500's rows, and a client that walks until it sees an empty response collected thousands of duplicates instead of stopping. - API docs state the limit and say that raising
index.max_result_windowon your own cluster does not lift it, because GitLab never reads that setting (doc/api/search.md).
Reviewer focus: the early return in #search_objects leaves the search_results memo unset, so #search_highlight and the REST #failed? check each run one extra default-paged query (page: 1, per_page: 20) whose result is discarded. Those are at from: 0 so they cannot 400, and they are cheaper than the pre-MR behaviour where the objects query itself hit Elasticsearch and failed — but if you would rather short-circuit them in this MR than in a follow-up, say so.
Mechanism and citations
Mechanism
Search::Elastic::Formats.page (ee/lib/search/elastic/formats.rb) computes from = per_page * (page - 1) with size = per_page and no ceiling. MAX_PER_PAGE bounded per_page at 200; nothing bounded page. The window bound is inclusive: from + size must be <= index.max_result_window.
app/services/search_service.rb—MAX_RESULT_WINDOW = 10_000. Not read back from the cluster;grep -rn max_result_windowfinds no production override in the repo, so the Elasticsearch default applies.app/services/search_service.rb—#search_objectsreturns early viaresults_beyond_result_windowwhen the guard fires, so the paged Elasticsearch query is never built.app/services/search_service.rb—#page_beyond_result_window?issearch_type == 'advanced' && page > max_page. Thesearch_typeguard is what keeps basic search (plain SQLOFFSET,lib/gitlab/search_results.rb) and Zoekt serving deep pages.app/services/search_service.rb—#results_beyond_result_windowreturnsKaminari.paginate_array([], total_count: MAX_RESULT_WINDOW, limit: per_page, offset: per_page * (page - 1)). The offset is the requested one, which is what keepsX-Pagehonest;total_countsits at Kaminari'sMAX_COUNT_LIMITsoOffsetPaginationomitsX-Totalrather than publishing a fabricated one.app/services/search_service.rb—#max_pageis[MAX_RESULT_WINDOW / per_page, 1].max. Integer division, sofrom + per_page <= MAX_RESULT_WINDOWholds for non-dividingper_pagetoo.app/services/search_service.rb—#pagekeeps its original lower bound only.
Call sites deliberately untouched: ee/lib/search/elastic/composite_pagination/paginator.rb, ee/lib/search/elastic/indexer.rb, ee/app/services/search/elastic/cluster_reindexing_service.rb.
The two sibling queries named under Reviewer focus are #search_highlight → Gitlab::Elastic::SearchResults#highlight_map (ee/lib/gitlab/elastic/search_results.rb) and, on the REST path, #failed? (lib/api/search.rb, ee/lib/gitlab/elastic/search_results.rb).
New coverage: spec/services/search_service_spec.rb (result window boundary, parameterised over per_page 1/3/20/199/200 on both sides), spec/requests/api/search_spec.rb (header assertions) and spec/requests/api/search_spec.rb (lowered window, with a basic-search control).
Evidence — the Elasticsearch probes, the header measurements and the mutation runs: $6053830
What I did not verify
- CI had not run at the time of writing.
- No end-to-end Elasticsearch round trip through
SearchServiceitself; the sibling-query behaviour above comes from a source read plus arails runnerprobe. - The change is behind no feature flag.
@johnmason for feedback, or reply #human to escalate to John.