Bulk insert approvers when copying project rules on merge

Marc Shawrequested to merge
628868-copy-project-rules-bulk-insert into master

What does this MR do and why?

When a project has "Prevent editing approval rules in merge requests" enabled, merging copies each project approval rule onto the merge request. Building that copy with its approvers already attached makes ActiveRecord validate every approver user and insert their join rows one at a time, all inside the merge finalize transaction. Behind the approval_rules_finalize_bulk_copy_approvers flag (off by default), we save the rule first and then bulk insert the approver join rows in one query per rule. On GDK with 5 project rules of 600 approvers each, this takes the finalize transaction from 8.5s and 6152 statements down to 0.52s and 106 statements.

Detailed context for AI agents

Root cause. MergeWorker runs MergeRequests::PostMergeService, whose EE override runs ApprovalRules::FinalizeService#execute inside a single ApplicationRecord.transaction. When approval_state.approval_rules_overwritten? is false, which happens whenever the project prevents overriding approvers per merge request (common in enterprise setups), handling_of_rules calls copy_project_rules. For each project rule it builds a new ApprovalMergeRequestRule via merge_request.approval_rules.build(attributes.merge(users: project_rule.approvers, groups: ...)), then calls valid? and save!. Assigning users: on an unsaved habtm parent triggers ActiveRecord's autosave association validation, which calls valid? on every associated User inside the transaction. Each User validation runs before_validation :ensure_namespace_correct (app/models/user.rb:422, :2163), loading the user's namespace with one SELECT namespaces per approver, plus the User uniqueness validations. Then one INSERT per approver follows. Measured about 3ms per approver per rule on GDK, so a large approver group across several project rules can hold the finalize transaction for minutes. This matches the PatroniLongRunningTransactionDetected alerts for MergeWorker referenced in the work item; the planned 30 minute transaction_timeout rollout would start failing these merges.

How found. Ran MergeWorker#perform inline on GDK with a tracer subscribed to sql.active_record, recording every BEGIN/COMMIT with per-transaction statement counts, Gitaly RPCs, and idle gaps. Reproduced the N+1 in isolation: building a rule with 20 users produced 22 SELECT namespaces plus 20 inserts. The backtrace for the namespace query goes through User#ensure_namespace_correct <- ActiveSupport callbacks <- state_machines run_actions <- ActiveRecord autosave association validation.

What changed. EE only, ee/app/services/approval_rules/finalize_service.rb. Behind the flag, the new rule is built without users:/groups:, validated and saved, then copy_approvers inserts the approver join rows with a single ApprovalMergeRequestRulesUser.insert_all(rows, unique_by: [:approval_merge_request_rule_id, :user_id]) per rule, setting project_id on each row and calling rule.users.reset afterwards. rule.groups = groups is then assigned on the now-persisted rule; since groups are few and the owner is already persisted, this habtm assignment only writes join rows and does not re-save the Group records. Flag off runs the original code path, unchanged. The service also includes Gitlab::Utils::StrongMemoize to memoize the flag check.

Flag semantics. approval_rules_finalize_bulk_copy_approvers, type gitlab_com_derisk, default off, actor is merge_request.target_project, defined in ee/config/feature_flags/gitlab_com_derisk/approval_rules_finalize_bulk_copy_approvers.yml.

Safety. The unique index index_approval_merge_request_rules_users_1 on (approval_merge_request_rule_id, user_id) backs unique_by. approval_merge_request_rules_users.project_id has a NOT NULL check constraint. rule.project_id is nil on the Ruby side right after save!, because approval_merge_request_rules.project_id is filled by a BEFORE INSERT trigger and Rails only reads back id, so rows set project_id: merge_request.target_project_id explicitly rather than relying on the join table's own sharding-key trigger (trigger_9b944f36fdac) to fill it. The habtm users association has after_add: :audit_add, which is a no-op for ApprovalMergeRequestRule since only project rules audit, so bypassing the association loses no auditing. The User validations that no longer run were incidental; nothing in the copy relied on them. The invalid-rule branch (duplicate name leads to merge_request.approval_rules.delete(new_rule) plus a debug log) is unchanged.

Database query. One statement per copied rule that has approvers, on approval_merge_request_rules_users (unique index index_approval_merge_request_rules_users_1 on (approval_merge_request_rule_id, user_id)). Captured from a local run with 3 approvers:

INSERT INTO "approval_merge_request_rules_users" ("approval_merge_request_rule_id","user_id","project_id")
VALUES (44, 39, 32), (44, 40, 32), (44, 41, 32)
ON CONFLICT ("approval_merge_request_rule_id","user_id") DO NOTHING
RETURNING "id"

In production the VALUES list has one row per active approver of the project rule (direct users plus visible group members), so a few hundred rows for a large group.

Failure mode. If insert_all raises, the whole finalize transaction rolls back exactly as before when save! raised. The merge itself is already recorded by mark_as_merged, which happens earlier, outside this transaction.

Alternatives rejected.

  • Assigning rule.users = users after save! on the persisted rule avoids the User validations but still does one INSERT per approver: measured 56 statements for 50 users versus 10 with insert_all.
  • Moving finalization to a worker changes when approvers become visible on the merged MR and is a larger change.
  • Removing the transaction would leave rules half-finalized on failure.

Out of scope. sync_approved_approvers (ee/app/models/approval_merge_request_rule.rb:154) still assigns approved_approver_ids row by row inside the same transaction; this grows with approvals x rules and measured 88 statements / 0.36s for 6 rules x 600 members with the sibling flag on. The sibling MR !255430 (merged) handles the merge_group_members_into_users path and the CODEOWNERS Gitaly read; this MR does not touch those.

Measurements. GDK project with 5 project rules, each with a group of 600 members, disable_overriding_approvers_per_merge_request: true, a 41-commit MR, sibling flags irrelevant to this path. Flag off: finalize transaction 8544ms, 6152 statements (3000 INSERT into approval_merge_request_rules_users, 3015 SELECT namespaces), about 7.7s of that idle in transaction. Flag on: 518ms, 106 statements. An earlier run with both sibling MR flags on but this fix absent measured 10873ms, confirming the sibling MRs do not help this path.

Verification. bundle exec rspec ee/spec/services/approval_rules/finalize_service_spec.rb: 11 examples, 0 failures. Rubocop clean on changed files. In the "not overwritten" context, the existing "copies the expected rules with expected params" example became a shared example run with the flag on and off. A new example asserts ApprovalMergeRequestRulesUser.insert_all is called once, only for the rule with approvers, and that the copied rule has the expected users. The flag-off example asserts insert_all is not called and users still match.

Rollout. Rollout issue: #629137.

References

Screenshots or screen recordings

Not applicable, backend only.

How to set up and validate locally

  1. In a Rails console, enable the flag for a project: Feature.enable(:approval_rules_finalize_bulk_copy_approvers, project).
  2. In that project's merge request approval settings, turn on "Prevent editing approval rules in merge requests".
  3. Add a project approval rule whose approvers include a group.
  4. Create a merge request and merge it.
  5. Check that merge_request.approval_rules.regular.first.users includes the group members.
  6. Optionally, subscribe to sql.active_record around the merge and confirm there is no SELECT "namespaces" per approver.

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.

Edited by Marc Shaw

Merge request reports