Cache the total count on the group merge requests API

Marc Shawrequested to merge
fix/mr-list-api-policy-rule-query-and-group-count into master

Behind a new gitlab_com_derisk flag cache_total_count_on_group_merge_requests_api (default off, group actor), GET /groups/:id/merge_requests caches the count behind X-Total for 15 minutes instead of running it on every page.

  • All pagination headers stay. A client walking every page pays for a count on the first and last page only.
  • This replaces the earlier version of this MR, which removed the headers and was ruled a breaking change.
Detailed context for AI agents

Problem

  • GET /groups/:id/merge_requests runs an exact COUNT (Kaminari total_count_with_limit, capped at LIMIT 10001) on every request to fill X-Total and X-Total-Pages.
  • The count is identical on every page of the same query.
  • On gitlab-org it took 1240ms of 2400ms DB time in one request.
  • Heavy callers walk every page: 5,400 open MRs at per_page=100 is 54 identical counts per walk.

History of this MR

  • The previous version skipped the count behind the flag skip_total_count_on_group_merge_requests_api and removed X-Total and X-Total-Pages.
  • API owners said removing them by default is a breaking change that needs VP approval: !257173 (comment 3961105985) and !257173 (comment 3983043278).
  • That flag is removed in this version.
  • A breaking change exception request was filed for the skip version: gitlab-com/Product#14583.
  • A variant that built the headers without a Kaminari patch was tried in !261177 (closed). It was closed in favour of this one because it had to reimplement Kaminari's next and prev page math.
  • After review, page 1 was made to always count: !257173 (comment 3995493014)

What changes

  • Flag: cache_total_count_on_group_merge_requests_api, type gitlab_com_derisk, default off. The actor is the requested group, not the root ancestor. Rollout issue: #630469.
  • With the flag on, all headers are still returned: X-Total, X-Total-Pages, X-Page, X-Per-Page, X-Next-Page, X-Prev-Page and the Link header (first/prev/next/last).
  • Project and global endpoints are unchanged. With the flag off the group endpoint is unchanged.

Implementation

  • config/initializers/kaminari_active_record_relation_methods_with_limit.rb: new preset_total_count(count) sets Kaminari's memoised @total_count. total_count_with_limit already returns early when @total_count is set, so X-Total, X-Total-Pages, next_page and prev_page all use the preset value and no COUNT runs.
  • lib/gitlab/pagination/offset_pagination.rb: new keyword count_cache_key: on paginate, with the constant COUNT_CACHE_EXPIRES_IN = 15.minutes. apply_cached_count reads Rails.cache. Page 1 always runs total_count_with_limit and writes the result to the cache. On later pages, if there is a cached count and the current page is before the last page that count implies (1 < current_page < ceil(count / per_page)), it presets it. Otherwise it counts and writes. It is skipped when without_count is true.
  • Cache key: [:offset_pagination_total_count, [:group_merge_requests, group id, user id], SHA256 of relation.except(:offset, :limit, :order).to_sql].
    • The count SQL covers every filter and admin mode, and ignores serialization-only params (like with_labels_details) and sort order.
    • The user id keeps counts per user even if visibility filtering ever moves out of the SQL.
    • An earlier cache iteration keyed on a hash of the request params. That missed admin mode and split the cache on serialization-only params.
  • lib/api/merge_requests.rb: find_merge_requests(count_cache_key: nil, **args). The group route passes group_merge_requests_count_cache_key, which returns nil when the flag is off. The signature uses **args because (args = {}, keyword: ...) makes find_merge_requests(project_id: ...) raise ArgumentError on Ruby 3.
  • Last-page recount: the page the cached count calls the last one (or any later page) always recounts and rewrites the cache. A stale low count can never drop the next link, so clients following next links never miss MRs.
  • Over 10,000 results: the totals are still omitted. A cached count above the limit is reused on every page after the first, also past the page it calls the last one, because the without_count path finds the next page from the loaded records and never shows the count.

Trade-offs and staleness

  • Page 1 is always exact: X-Total, X-Total-Pages and the Link last link are the same as today.
  • On pages after the first and before the last, X-Total and X-Total-Pages show the count from the latest page 1 or last page of the same query by the same user, at most 15 minutes old.
  • If more than per_page MRs close or merge during the TTL, a walking client can hit some empty pages before the cached last page, which recounts and ends the walk.
  • A client that reads X-Total-Pages on page 1 and fetches the rest in parallel is not affected, because page 1 is exact.
  • Permission changes inside the TTL: if a user loses access to a project, their X-Total can still include its MRs for up to 15 minutes. Only the number is affected, never the MRs.
  • Results that fit on one page always recount. Those counts are cheap.
  • Precedent: Groups::MergeRequestsCountService serves the group open MR count from a 24 hour cache once it is over 1,000, shared across users.

Open question for API owners

  • Is an X-Total up to 15 minutes old on pages after the first and before the last acceptable as "no change" for the API contract?

Not in scope

  • An opt-in without_count=true request param (approved in !257173 (comment 3961105985)). This is a separate change.
  • Per merge request mergeability check costs.
  • The per-MR policy rule query, which was split into !257613 (merged) (merged).

Verification done

  • spec/lib/gitlab/pagination/offset_pagination_spec.rb: 41 examples, 0 failures. New examples check:
    • counts on the first request and caches the result
    • counts again on the first page even with a cached count
    • reuses the cached count on later pages before the last page
    • reuses a cached count over the limit on the page it calls the last one
    • recounts on the page the cached count calls the last one
    • counts again for another query with the same cache key (this one fails if the SQL is left out of the key)
    • ignores the cache when without_count is true
  • spec/requests/api/merge_requests_spec.rb, context 'total count cache':
    • reuses the count from an earlier request on page 2
    • reuses it when only serialization params change (with_labels_details)
    • counts again on the first page
    • counts again on page 2 for another user or other filters
    • with the flag off every request counts
    • Group list block: 139 examples, 0 failures. Without the page 1 change, the two first-page examples fail.
  • RuboCop clean.

How to set up and validate locally

  1. In a Rails console on GDK, enable the flag for a group with a few merge requests: Feature.enable(:cache_total_count_on_group_merge_requests_api, Group.find_by_full_path('<group>')).
  2. Request page 1, then page 2, and compare X-Total: curl -sI --header "PRIVATE-TOKEN: <token>" "http://gdk.test:3000/api/v4/groups/<group_id>/merge_requests?per_page=1" (add &page=2).
  3. Run tail -f log/development.log while doing this. You should see SELECT COUNT(*) FROM (SELECT 1 AS one FROM "merge_requests" only on page 1 and on the last page.
  4. Add with_labels_details=true and check that no new count runs.
  5. Disable the flag with Feature.disable(:cache_total_count_on_group_merge_requests_api, Group.find_by_full_path('<group>')) and repeat. Every request should now run the count.

References

Edited by Marc Shaw

Merge request reports