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!callsapprovals_required_pre_merge->branch_requires_code_owner_approval?->section_optional?->Gitlab::CodeOwners.optional_section?, which loads the CODEOWNERS blob from Gitaly (GetBlobsRPC). The database session sits idle in transaction while waiting on Gitaly.- In the overwritten-rules path,
merge_group_members_into_usersdoesrule.users |= rule.group_users, a habtm assignment. This issues one INSERT intoapproval_merge_request_rules_usersper 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_mergeis made public. It was already strong-memoized, soFinalizeServicecan call it ahead of time without changing its behavior.FinalizeServicememoizeswrapped_code_owner_rulesand callsapprovals_required_pre_mergeon each before opening the transaction, so the CODEOWNERS read and the memoized result happen outside it.update_code_owner_rulesreuses the same wrapped objects, so the Gitaly read is not repeated.merge_group_members_into_usersnow computesrule.group_user_ids - rule.user_idsand performs a singleApprovalMergeRequestRulesUser.insert_all(rows, unique_by: [:approval_merge_request_rule_id, :user_id])per rule, withproject_idset on each row, followed byrule.users.reset.- When the flag is off, the original code path is unchanged.
Safety notes
- The habtm
usersassociation hasafter_add: :audit_add, butApprovalMergeRequestRule#audit_addis a no-op (only project-level rules audit). Bypassing the association loses no audit behavior. - The unique index
index_approval_merge_request_rules_users_1on(approval_merge_request_rule_id, user_id)makes the insert idempotent, matchingunique_by:. - Memoizing the wrapped code owner rules before
merge_group_members_into_usersruns is safe becauseApprovalRuleLike#approversalready includes group users, so the approver set used byapprovals_required_pre_mergeis the same before and after the merge intousers.
Out of scope
copy_project_rules(the non-overwritten path) still builds MR rules withusers:assigned through the association, one insert per approver.sync_approved_approversstill assignsapproved_approver_idsrow 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_allis called once per rule and the resulting users are correct.- Flag-off context asserts
insert_allis not called and users and approved approvers are still correct. - CODEOWNERS is read (
Gitlab::CodeOwners.optional_section?) beforeApplicationRecord.transactionopens, using ordered expectations. - Flag-off context still updates
approvals_requiredon the code owner rule.
Files changed
ee/app/services/approval_rules/finalize_service.rbee/app/models/approval_wrapped_code_owner_rule.rbee/spec/services/approval_rules/finalize_service_spec.rbee/config/feature_flags/gitlab_com_derisk/approval_rules_finalize_shorter_transaction.yml
References
- Work item: #628868 (closed)
- Sibling MR for the larger occurrence (metrics inside the post-merge event transaction): !255421 (merged)
transaction_timeoutproduction rollout: gitlab-com/gl-infra/production-engineering#25884
Screenshots or screen recordings
Not applicable, backend only.
How to set up and validate locally
- Make sure an EE license is active so approval rules are available.
- In a Rails console enable the flag for a project:
Feature.enable(:approval_rules_finalize_shorter_transaction, project). - Create an MR-level approval rule on a merge request in that project and add a group as approver.
- Merge the merge request.
- Check that
merge_request.approval_rules.first.usersincludes 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.