Delete approval policy reads before their project rules

Sashi Kumar Kumaresanrequested to merge
625031-atomic-approval-policy-rule-deletion into master

What does this MR do and why?

Security::Policy#delete_approval_policy_rules_for_project (ee/app/models/security/policy.rb) removes the rows a merge request approval policy created for one project. It does this in five separate, non-transactional passes: approval_project_rules, approval_merge_request_rules (unmerged MRs only), scan_result_policy_violations, software_license_policies, and finally the scan_result_policies rows (model Security::ScanResultPolicyRead, "reads").

If the job stops between the first and last pass (Sidekiq interruption during a deploy, a statement timeout, sidekiq_retry_in discarding the job on ActiveRecord::RecordInvalid, retries exhausted), a read can be left behind whose approval_project_rule is already gone. On the next sync, Security::ScanResultPolicies::ApprovalRules::CreateService#create_rule finds that read for the same approval_policy_rule_id and returns early. The approval rule is never recreated, and nothing is logged. The project silently loses that policy's approval rule until the policy is edited. Security::Policies::ProjectTransferWorker#delete_rules_for_configurations has the same pattern.

This MR reorders the passes so the rule and its read are removed together instead of in separate statements far apart in the sequence. The five passes now run as: violations, software license policies, reads, unmerged MR rules, then a final approval_project_rules sweep.

Deleting a read now cascades its approval_project_rules row in the same statement, through the existing foreign key fk_e1372c912e (approval_project_rules.scan_result_policy_id ON DELETE CASCADE). CreateService always sets scan_result_policy_id on rules it creates, so the pair is removed atomically. At every statement boundary, a read either still has its rule or neither exists.

The final approval_project_rules pass stays as a sweep for rows without scan_result_policy_id, and to cover the future where reads stop being written under the deprecate_scan_result_policies feature flag.

Violations and license policies stay ahead of the reads pass so the cascade from deleting reads remains small (at most rules times approval actions rows). Deleting reads first would make the violations cascade one unbatched statement, one row per merge request per read.

The unmerged MR-rules pass runs after reads. It is keyed on approval_policy_rule_id, so the ON DELETE SET NULL on approval_merge_request_rules.scan_result_policy_id does not affect it. Running it after the rules are gone also means a concurrent MR sync started after the delete finds no project rules to copy from.

ProjectTransferWorker#delete_rules_for_configurations gets the same reorder: license policies, violations, reads, MR rules, project rules. There, license policies and violations are resolved through the reads, so they must stay before the reads pass.

We did not wrap all five passes in one transaction. The MR-rules pass iterates the whole configuration's approval_merge_request_rules in batches of 5000. Holding locks across that batch loop is the pattern the database guidelines advise against. The cascade already gives atomicity for the pair that matters (rule and read).

References

Screenshots or screen recordings

Not applicable. This is a backend-only change to deletion ordering in Security::Policy and Security::Policies::ProjectTransferWorker. There is no UI change.

How to set up and validate locally

  1. In a Rails console, find a project linked to a merge request approval policy.
  2. Take one Security::ScanResultPolicyRead for that project and note its approval_project_rules row.
  3. Stub Security::Policy#delete_approval_project_rules_for_project to raise an error.
  4. Run: 
    Security::ScanResultPolicies::ApprovalRules::DeleteService.new(
  project: project,
  security_policy: security_policy,
  approval_policy_rules: security_policy.approval_policy_rules
).execute
    and rescue the raised error.
  5. Confirm both the read and its rule are gone.
  6. Run a policy resync and confirm the approval rule is recreated.

Automated coverage:

  • ee/spec/models/security/policy_spec.rb: the existing #delete_approval_policy_rules_for_project examples now also cover reads. New examples stub each pass in turn to raise, then assert no read for the project is left without its approval_project_rule. Two direct checks: when the reads pass raises, the rule and read both still exist; when the final rules pass raises, the rule and read are both gone through the cascade. Six of these examples fail against the old pass order and pass with the new one.
  • ee/spec/workers/security/policies/project_transfer_worker_spec.rb: the same two direct checks for the worker.

Database review notes: no new SQL is introduced, only a reorder of existing statements. .where(project_id:) on the project-rules pass became the existing for_project scope. The reads DELETE now cascades at most rules times actions approval_project_rules rows (index idx_approval_project_rules_on_scan_result_policy_id) plus their join rows, work the first pass already did before this change, just earlier in the sequence. The SET NULL on merged MRs' approval_merge_request_rules rows (index idx_approval_merge_request_rules_on_scan_result_policy_id) is pre-existing behavior of the last pass and is unchanged.

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