Shorten the approval rule finalization transaction on merge

What does this MR do and why?

ApprovalRules::FinalizeService runs one transaction after a merge request is merged, and it grows with the size of the approver group. This MR moves the CODEOWNERS lookup (a Gitaly call) out of the transaction and replaces the per-member INSERT loop with one insert_all per rule. Behind a feature flag, off by default. On GDK with a 150 member group and two rules the transaction went from 0.21s / 185 statements / 2 Gitaly calls inside to 0.03s / 28 statements / none.

Detailed context for AI agents

Root cause

Found by tracing an inline MergeWorker run on GDK with hooks on sql.active_record BEGIN/COMMIT and Gitlab::GitalyClient.call.

ApprovalRules::FinalizeService#execute runs after mark_as_merged and wraps all of its work in a single ApplicationRecord.transaction. Inside that transaction:

  • ApprovalWrappedCodeOwnerRule#finalize! calls approvals_required_pre_merge -> branch_requires_code_owner_approval? -> section_optional? -> Gitlab::CodeOwners.optional_section?, which loads the CODEOWNERS blob from Gitaly (GetBlobs RPC). The database session sits idle in transaction while waiting on Gitaly.
  • In the overwritten-rules path, merge_group_members_into_users does rule.users |= rule.group_users, a habtm assignment. This issues one INSERT into approval_merge_request_rules_users per group member per rule, so both the statement count and the transaction duration grow linearly with approver group size.

Measured on GDK with a 150 member group and two rules: transaction 0.21s, 185 statements, 150 row inserts per rule (300 total), 2 Gitaly calls inside the transaction.

What changed and why

All changes are in EE, behind the feature flag approval_rules_finalize_shorter_transaction (type gitlab_com_derisk, default off, actor target project).

  • ApprovalWrappedCodeOwnerRule#approvals_required_pre_merge is made public. It was already strong-memoized, so FinalizeService can call it ahead of time without changing its behavior.
  • FinalizeService memoizes wrapped_code_owner_rules and calls approvals_required_pre_merge on each before opening the transaction, so the CODEOWNERS read and the memoized result happen outside it. update_code_owner_rules reuses the same wrapped objects, so the Gitaly read is not repeated.
  • merge_group_members_into_users now computes rule.group_user_ids - rule.user_ids and performs a single ApprovalMergeRequestRulesUser.insert_all(rows, unique_by: [:approval_merge_request_rule_id, :user_id]) per rule, with project_id set on each row, followed by rule.users.reset.
  • When the flag is off, the original code path is unchanged.

Safety notes

  • The habtm users association has after_add: :audit_add, but ApprovalMergeRequestRule#audit_add is a no-op (only project-level rules audit). Bypassing the association loses no audit behavior.
  • The unique index index_approval_merge_request_rules_users_1 on (approval_merge_request_rule_id, user_id) makes the insert idempotent, matching unique_by:.
  • Memoizing the wrapped code owner rules before merge_group_members_into_users runs is safe because ApprovalRuleLike#approvers already includes group users, so the approver set used by approvals_required_pre_merge is the same before and after the merge into users.

Out of scope

  • copy_project_rules (the non-overwritten path) still builds MR rules with users: assigned through the association, one insert per approver.
  • sync_approved_approvers still assigns approved_approver_ids row by row inside the transaction.

Measurements

Same GDK setup, flag on: transaction 0.03s, 28 statements, 2 inserts (one per rule), no Gitaly call inside, all 150 users still attached to the rule. Flag off reproduces the original numbers.

Verification

bundle exec rspec ee/spec/services/approval_rules/finalize_service_spec.rb: 12 examples, 0 failures. New specs cover:

  • insert_all is called once per rule and the resulting users are correct.
  • Flag-off context asserts insert_all is not called and users and approved approvers are still correct.
  • CODEOWNERS is read (Gitlab::CodeOwners.optional_section?) before ApplicationRecord.transaction opens, using ordered expectations.
  • Flag-off context still updates approvals_required on the code owner rule.

Files changed

  • ee/app/services/approval_rules/finalize_service.rb
  • ee/app/models/approval_wrapped_code_owner_rule.rb
  • ee/spec/services/approval_rules/finalize_service_spec.rb
  • ee/config/feature_flags/gitlab_com_derisk/approval_rules_finalize_shorter_transaction.yml

References

Screenshots or screen recordings

Not applicable, backend only.

How to set up and validate locally

  1. Make sure an EE license is active so approval rules are available.
  2. In a Rails console enable the flag for a project: Feature.enable(:approval_rules_finalize_shorter_transaction, project).
  3. Create an MR-level approval rule on a merge request in that project and add a group as approver.
  4. Merge the merge request.
  5. Check that merge_request.approval_rules.first.users includes all of the group's members.

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Merge request reports

Loading
Loading