Reorder approval-policy rule deletion for atomicity

Alan (Maciej) Paruszewskirequested to merge
fix/atomic-approval-policy-rule-deletion-order-master into master

Problem

Security::Policy#delete_approval_policy_rules_for_project deleted approval_project_rules first and scan_result_policy_reads last, across five separate non-transactional each_batch passes. Anything that interrupted that sequence between the two (job timeout, deploy, statement timeout) left a scan_result_policy_read orphaned: its owning rule already gone, but the read row itself still there with no approval_project_rule pointing at it.

Separately, BaseProjectPolicyService#unlink_policy called security_policy.unlink_project! first, then deleted the project's approval policy rules afterward. Between the two, a report_approver rule existed with no matching ApprovalPolicyRuleProjectLink for however long the batched rule deletion took, and EE::MergeRequest#linked_report_approver_project_rules treats that as "unlinked" and silently skips syncing it onto merge requests.

Security::ScanResultPolicies::ApprovalRules::CreateService (!252506 (closed)) now self-heals from an orphaned scan_result_policy_read on the next resync, but neither gap here stopped a new orphan from forming, or a rule from briefly looking unlinked while it still exists.

What changed

  • delete_approval_policy_rules_for_project: move the approval_project_rules deletion to run immediately before scan_result_policy_reads instead of first, shrinking the window from three unbounded batched passes to a single statement boundary. The other passes (approval_merge_request_rules, scan_result_policy_violations, software_license_policies) carry no such invariant and are left where they are.
  • unlink_policy: delete the approval policy rules before unlinking the project instead of after. The rule can only ever be gone before its link is now, never the other way around, so there's no window where a live rule looks unlinked. The skip_delete_worker link-existence check still runs after unlink_project!, since it depends on this project's own link already being gone to correctly tell whether any other project still uses the rule.

Testing

TDD throughout: each new test confirmed failing against the original ordering for the specific reason it targets (an "already gone" assertion failing because the old order deleted the rule before the raise, an .ordered call-sequence expectation failing against the old call order), then passing after the reorder. An end-state-only assertion wouldn't discriminate between the two orders, so plain before/after counts weren't enough on their own — the new tests specifically interrupt each pass with a stubbed raise and assert what survives.

Full regression: policy_spec.rb + base_project_policy_service_spec.rb — 293 examples, 0 failures. RuboCop clean on both changed files.

Note: this branch does not depend on !252506 (closed) — verified against current master, both reorders apply to the code as it exists there today, independent of !252506 (closed)'s merge status.

References

Merge request reports