Skip unused diff commits preload for MR approval fields

Marc Shawrequested to merge
629382-skip-approval-diff-commits-preload into master

What does this MR do and why?

Skips a merge request diff commits preload used by approval-related GraphQL fields (approved, mergeabilityChecks, and related fields) when the approval_committer_emails_from_diff feature flag is enabled, since that flag already makes the code that used to need the preload query committers through SQL instead.

  • Fixes a production Rack::Timeout on 2026-09-25 on the MR dashboard GraphQL query. It was triggered by a merge request with over 10,000 commits: the preloaded association builds one loader per commit, so CPU cost grows roughly with the square of the commit count.
  • No new feature flag is added. The change follows the existing approval_committer_emails_from_diff flag, which is already fully enabled on gitlab.com. Self-managed instances keep today's behavior, since the flag defaults off there.
Detailed context for AI agents

Incident

A production Rack::Timeout fired on gitlab.com on 2026-09-25 at 11:44 UTC on the GraphQL query requestingReviewAuthorOrAssignee (currentUser.authoredMergeRequests with includeAssigned: true), used by the MR dashboard. The request ran for 60 seconds with cpu_s 59.84 and db_duration_s 0.10 across 40 queries, so it was CPU-bound in Ruby, not the database. Correlation ID a409c56e1ba00e80-AMS. The backtrace ended inside the ActiveRecord preloader, in loaders.each { |l| l.set_inverse(record) }, during the keyset connection's first load.

The user's merge request list included one MR with 10,337 commits in its latest diff (gitlab-org/modelops/applied-ml/code-suggestions/ai-assist!6768 (merged)). The other MRs in the list had 1 to 8 commits each.

Mechanism

The approved and mergeabilityChecks GraphQL fields (also approval_state, mergeable, detailed_merge_status) use approved_mergeability_check_preloads in ee/app/graphql/resolvers/concerns/ee/resolves_merge_requests.rb. This preloads every diff commit of every MR in the list.

With the mr_diff_commits_read_new_table flag enabled (globally on gitlab.com since 2026-08-24, and not reversible after the table swap), that preload also nests merge_request_commits_metadata. The belongs_to association for that model, in app/models/merge_request_diff_commit.rb, has an instance-dependent scope: ->(diff_commit) { where(project_id: diff_commit.project_id) }. Because the scope depends on each row, Rails 7.2 builds one preloader per commit and loops preloaders times rows, giving O(commits^2) CPU cost while the database itself still runs one fast query.

Local measurements on a GDK test database, preloading the metadata association directly:

Commits CPU time Loaders SQL queries
1,000 0.94s 1,000 2
4,000 14.5s 4,000 2
10,000 91.1s 10,000 2

Why the preload is safe to skip

The preload was added in March 2024 to avoid an N+1 in the "prevent approval by commit authors / committers" filter, which at the time read commit authors in Ruby. With approval_committer_emails_from_diff enabled, MergeRequest#committer_emails_from_diff uses a SQL path (committer_emails_from_diff_via_sql) instead, so nothing reads the preloaded commits anymore.

approval_committer_emails_from_diff is a gitlab_com_derisk flag, off by default, and has been globally enabled on gitlab.com (gprd) since 2026-06-30. Rollout issue: #590452.

Self-managed impact

The flag is off by default on self-managed instances, so the preload still runs there and behavior is unchanged. This MR does not add a new feature flag; it follows the existing flag that already decides whether the preloaded data is used. When that flag is removed, this conditional should be removed with it.

Actor parameter note

The resolver passes mr_parent as the flag actor (nil for user-level queries such as the dashboard, a Group for group MR lists, a Project for project lists), mirroring the existing read_new_commits_table_for_actor?(mr_parent) call in the same method. The model itself checks the flag against the MR's target project. With the flag globally enabled this is consistent. During a partial rollout, the worst case is either keeping an unused preload, or skipping it for a project where the Ruby fallback then loads commits itself. Either way this is a performance difference only, not a correctness one.

Verification

  • The new request spec in ee/spec/requests/api/graphql/project/merge_requests_spec.rb asserts that no merge_request_diff_commits preload query runs for the approved field when the flag is enabled, and that the preload still runs when the flag is disabled. It matches the preload query shape specifically (SELECT "merge_request_diff_commits".* FROM "merge_request_diff_commits") rather than any query against that table, because with the flag on and committer filtering active the committer emails SQL also reads that table and would fail the assertion for the wrong reason. It fails on master, since the diff commits query comes from limited_nodes in lib/gitlab/graphql/pagination/keyset/connection.rb, the production code path, and passes with this change.
  • ee/spec/requests/api/graphql/merge_requests/approval_state_spec.rb and spec/requests/api/graphql/project/merge_requests_spec.rb pass (1132 examples).
  • RuboCop is clean.
  • Measured locally on a GDK test database through the real MR dashboard GraphQL query (currentUser.authoredMergeRequests requesting approved, approvalsRequired, approvalsLeft, approvedBy, mergeabilityChecks, commitCount, conflicts), with merge_requests_disable_committers_approval enabled and an approval rule with an approver, so the committer filter ran once per merge request and issued its single SQL query. One MR with 2,000 seeded diff commits and the flag enabled: master built 2,000 merge_request_commits_metadata preloaders and took 5.3s CPU, this branch built 0 and took 1.3s CPU. Flag disabled: both built 6,000 preloaders and took about 8.3s CPU, so the self-managed path is unchanged. With 3 MRs in the list and the flag enabled no per-MR query was added; this branch ran one fewer merge_request_diffs query than master and the same number of committer SQL queries. The GraphQL response was identical in every run.

Out of scope / follow-ups

This MR is a mitigation, not the durable fix. The durable fix is https://gitlab.com/gitlab-org/gitlab/-/work_items/629382, which replaces the instance-dependent scope with composite foreign keys. That work also covers other callers of the same association that stay quadratic after this MR:

  • MergeRequestDiff calls to merge_request_diff_commits.with_users
  • MergeRequest#preload_commits_metadata
  • MergeRequestDiff#preload_metadata_for_commits
  • the CI variables preloader, ee/lib/ee/gitlab/ci/variables/preloader/merge_request.rb
  • the Banzai MR reference filter

Measured separately: the composite key shape takes 0.27s for 10,000 commits with a single loader, versus 91.1s above. A caveat found while testing that approach: rows whose raw project_id column is NULL resolve the association to nil, so it needs a project_id backfill, or proof that no such rows exist, before it can ship.

Related: automated N+1 triage for the same query pattern, #629314.

Also worth sequencing around: pending backfill MR !255692 (merged) will restore many missing diff commit rows on gitlab.com. That would expose more merge requests to the quadratic path described above, so it is better merged after this mitigation lands.

Changelog

The commit carries Changelog: performance and EE: true trailers.

References

Screenshots or screen recordings

Not applicable. This is a backend-only performance change with no UI impact.

How to set up and validate locally

  1. Run the request spec: bundle exec rspec ee/spec/requests/api/graphql/project/merge_requests_spec.rb. It covers both flag states.
  2. To see the regression, restore the master version of ee/app/graphql/resolvers/concerns/ee/resolves_merge_requests.rb and rerun: the does not preload the diff commits example fails with a merge_request_diff_commits query from limited_nodes.

MR acceptance checklist

Evaluated against the MR acceptance checklist. This is a backend-only performance fix: no database migration, no documentation change, no new feature flag (follows an existing, fully-rolled-out flag), and a new request spec covers the behavior change.

Edited by Marc Shaw

Merge request reports