Cap search page depth so from + size cannot exceed max_result_window

🤖 AI-authored change.

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 BadRequest in ee/lib/gitlab/search/client.rb would regress three existing rescue BadRequest call 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 page to the last reachable page makes the headers lie: OffsetPagination#needs_pagination? (lib/gitlab/pagination/offset_pagination.rb) re-pages from params[:page], so page=1000 came back with X-Page: 1000 and X-Next-Page: 1001 over 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_window on 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_window finds no production override in the repo, so the Elasticsearch default applies.
  • app/services/search_service.rb — #search_objects returns early via results_beyond_result_window when the guard fires, so the paged Elasticsearch query is never built.
  • app/services/search_service.rb — #page_beyond_result_window? is search_type == 'advanced' && page > max_page. The search_type guard is what keeps basic search (plain SQL OFFSET, lib/gitlab/search_results.rb) and Zoekt serving deep pages.
  • app/services/search_service.rb — #results_beyond_result_window returns Kaminari.paginate_array([], total_count: MAX_RESULT_WINDOW, limit: per_page, offset: per_page * (page - 1)). The offset is the requested one, which is what keeps X-Page honest; total_count sits at Kaminari's MAX_COUNT_LIMIT so OffsetPagination omits X-Total rather than publishing a fabricated one.
  • app/services/search_service.rb — #max_page is [MAX_RESULT_WINDOW / per_page, 1].max. Integer division, so from + per_page <= MAX_RESULT_WINDOW holds for non-dividing per_page too.
  • app/services/search_service.rb — #page keeps 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 SearchService itself; the sibling-query behaviour above comes from a source read plus a rails runner probe.
  • The change is behind no feature flag.

🤖 Automated change. Mention @johnmason for feedback, or reply #human to escalate to John.

Edited by John Mason

Merge request reports

Loading
Loading