Lock the approval project rule on MR sync and delete policy links before rules

What does this MR do and why?

Sourceless merge request rules from a lost race

ApprovalProjectRule#apply_report_approver_rules_to copies a project-level policy approval rule onto a merge request. It looks for the existing merge request rule only through approval_merge_request_rule_sources.approval_project_rule_id. That source row is written through a has_one :through association without autosave, so if the project rule has already been deleted, the source insert fails validation silently while rule.update! still succeeds.

This creates a race. A policy sync can delete a project rule after another process has already loaded it in memory, for example on merge request create, push, retarget, in Security::SecurityOrchestrationPolicies::SyncMergeRequestsService, in Security::ScanResultPolicies::ResyncMergeRequestRulesWorker, or in the mergeability check that enqueues that worker. That process then writes an approval_merge_request_rules row with no source. Nothing that looks up merge request rules through the source can find it again, so it is never updated or removed, but approval evaluation still sees it. This is the mechanism behind leftover duplicate report_approver rules such as #617882.

The existing guard, policy_rule_linked_to_project?, takes a FOR SHARE lock on approval_policy_rule_project_links. It only helps when the link is removed before the rules. On the full-refresh path in BaseProjectPolicyService#sync_approval_policy_diff, used when the number of approval actions changes, the links stay in place while rules are deleted and recreated, so the guard passes and the race stays open.

This MR adds a private method, policy_rule_still_present?, called right after policy_rule_linked_to_project? in the same transaction. It runs SELECT 1 FROM approval_project_rules WHERE id = ? LIMIT 1 FOR SHARE (self.class.lock('FOR SHARE').exists?(id)) and returns early when the row is gone. Rules with a blank approval_policy_rule_id, which are legacy and not policy-managed, skip the check, matching the existing guard's behavior.

With this in place, a policy sync that deletes the project rule row, either directly or through the cascade from deleting scan_result_policies rows, waits for an in-flight merge request sync transaction to commit, then deletes both the rule and its source. A merge request sync that starts after the delete finds no row and writes nothing. No sourceless merge request rule can be produced by this path, regardless of the order in which the delete path removes links, reads, and rules.

The cost is one primary-key lookup per merge request and project rule pair, inside a transaction that already exists, plus one extra row lock held for the duration of that short transaction. The delete side already competes for the same row lock through the cascade.

This complements !253938 (merged), which fixes delete ordering, and !253939 (merged), which fixes read reuse, both on the scan_result_policies side. Those two close the delete and create paths; this MR closes the merge request side race that delete ordering alone cannot fix.

Deadlock from the new lock's order

The new policy_rule_still_present? lock makes the merge request sync transaction's lock order explicit: it locks the approval_policy_rule_project_links row (through policy_rule_linked_to_project?) and then the approval_project_rules row.

Two deletion paths ran a single transaction that touched the same two tables in the opposite order, so a concurrent run could deadlock instead of one side simply waiting for the other:

• Security::DeleteSecurityPolicyWorker calls Security::Policy#delete_approval_policy_rules, which deleted approval_project_rules first, then deleted approval_policy_rules, which cascades to approval_policy_rule_project_links through a foreign key. This worker is enqueued directly from the policy-deleted event, so the links still exist when it runs.
• Security::DeleteOrchestrationConfigurationWorker and Security::RecreateOrchestrationConfigurationWorker call delete_scan_result_policy_reads (the Security::ScanResultPolicy concern on Security::OrchestrationPolicyConfiguration), which deleted approval_project_rules first, then deleted the configuration, which cascades through security_policies and approval_policy_rules to the links.

Every per-project path (unlink_policy, sync_approval_policy_diff, Security::Policies::ProjectTransferWorker, Security::DeleteApprovalPolicyRulesWorker) already changes links in its own transaction or with autocommit statements, so none of those were affected.

This reverse order actually predates this MR: inserting the approval_merge_request_rule_sources row already takes a FOR KEY SHARE lock on the project rule through its foreign key, in the same transaction and after the link lock. What this MR's first commit changed is that the lock on the project rule now happens earlier, via an explicit query, and it now also applies to syncs that update an existing merge request rule rather than only to syncs that insert one. That widened an existing deadlock window.

To close it, this MR reorders both delete paths so links are removed before rules, matching the merge request sync's lock order:

• Security::Policy#delete_approval_policy_rules now calls a new delete_approval_policy_rule_project_links as its first step.
• delete_scan_result_policy_reads (the Security::ScanResultPolicy concern) does the same.

Deleting links first is safe in both cases: every caller of these two methods deletes the policies, or the whole configuration, right afterwards, which would cascade-delete the same links anyway. The configuration-level method is deliberately configuration-wide, since it already deletes reads and project rules for every project in the configuration, so deleting the links for every project in the same pass is consistent with that. The concern's version batches with each_batch directly instead of the concern's delete_in_batches helper, because that helper orders batches by updated_at, and the links table has no such column.

Finally, apply_report_approver_rules_to now also retries on ActiveRecord::Deadlocked, alongside the existing RecordNotUnique retry (2 tries total). If Postgres aborts the merge request side of a remaining deadlock, the retry re-runs the transaction, and by then the link lookup sees the link is gone and writes nothing.

References

• Related: #625031 (closed)
• Related: #617882
• Related: !253938 (merged)
• Related: !253939 (merged)

Screenshots or screen recordings

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

How to set up and validate locally

1. In a Rails console, find a project with a merge request approval policy and an open merge request on a protected branch.
2. Load the project rule: rule = project.approval_rules.report_approver.last.
3. Delete the row directly to simulate a concurrent policy sync: ApprovalProjectRule.where(id: rule.id).delete_all.
4. Apply the rule to the merge request: rule.apply_report_approver_rules_to(merge_request).
5. Confirm merge_request.approval_rules.report_approver.count is unchanged, and that no approval_merge_request_rules row was created without a matching approval_merge_request_rule_sources row.
6. To check the lock-order fix, take a policy that is linked to a project (policy.approval_policy_rules should have at least one row with a matching approval_policy_rule_project_links row), then run either ActiveRecord::QueryRecorder.new { policy.delete_approval_policy_rules }.log or ActiveRecord::QueryRecorder.new { configuration.delete_scan_result_policy_reads }.log, and confirm the DELETE FROM "approval_policy_rule_project_links" statement appears before the DELETE FROM "approval_project_rules" statement in the log.

Test coverage is in ee/spec/models/approval_project_rule_spec.rb, under #apply_report_approver_rules_to. The context that deletes the project rule row after the instance has been loaded asserts that no merge request rule is created and that the row lock query is issued; this spec fails without the change, because a sourceless rule is written today. A second spec in the same file simulates a deadlock on the first attempt and asserts the retry succeeds on the second.

ee/spec/models/security/policy_spec.rb and ee/spec/models/security/orchestration_policy_configuration_spec.rb each add a spec confirming the project link row is deleted, plus a QueryRecorder-based spec asserting the links DELETE is issued before the project rules DELETE.

Database review notes

This adds the following queries. There is no schema change.

Existing query (from the first commit), a primary key lookup:

SELECT 1 AS one FROM "approval_project_rules" WHERE "approval_project_rules"."id" = $1 LIMIT 1 FOR SHARE

New queries from the lock-order fix. Security::Policy#delete_approval_policy_rules batches deletion of links by policy via each_batch on the links primary key:

SELECT "approval_policy_rule_project_links"."id" FROM "approval_policy_rule_project_links" WHERE "approval_policy_rule_project_links"."approval_policy_rule_id" IN (SELECT "approval_policy_rules"."id" FROM "approval_policy_rules" WHERE "approval_policy_rules"."security_policy_id" = $1) ORDER BY "approval_policy_rule_project_links"."id" ASC LIMIT 1

SELECT "approval_policy_rule_project_links"."id" FROM "approval_policy_rule_project_links" WHERE "approval_policy_rule_project_links"."approval_policy_rule_id" IN (SELECT "approval_policy_rules"."id" FROM "approval_policy_rules" WHERE "approval_policy_rules"."security_policy_id" = $1) AND "approval_policy_rule_project_links"."id" >= $2 ORDER BY "approval_policy_rule_project_links"."id" ASC LIMIT 1 OFFSET 1000

DELETE FROM "approval_policy_rule_project_links" WHERE "approval_policy_rule_project_links"."approval_policy_rule_id" IN (SELECT "approval_policy_rules"."id" FROM "approval_policy_rules" WHERE "approval_policy_rules"."security_policy_id" = $1) AND "approval_policy_rule_project_links"."id" >= $2

delete_scan_result_policy_reads (the Security::ScanResultPolicy concern) deletes links for every project in a configuration, so it has one more level of subquery, same batching shape:

SELECT "approval_policy_rule_project_links"."id" FROM "approval_policy_rule_project_links" WHERE "approval_policy_rule_project_links"."approval_policy_rule_id" IN (SELECT "approval_policy_rules"."id" FROM "approval_policy_rules" WHERE "approval_policy_rules"."security_policy_id" IN (SELECT "security_policies"."id" FROM "security_policies" WHERE "security_policies"."security_orchestration_policy_configuration_id" = $1)) ORDER BY "approval_policy_rule_project_links"."id" ASC LIMIT 1

The batch-boundary SELECT (with AND "approval_policy_rule_project_links"."id" >= $2 ... LIMIT 1 OFFSET 1000) and the DELETE (with the same >= $2 bound) follow the same pattern as the policy-level queries above, with the extra security_policies subquery in place of the direct security_policy_id = $1 filter.

approval_policy_rule_project_links has a unique index on (approval_policy_rule_id, project_id) and an index on (project_id, id). The approval_policy_rule_id filter is served by the unique index; the batch ordering by id is not. Query plans from postgres.ai are still to be added.

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.