Fix policy approval rule desync detection and resync failures

Sashi Kumar Kumaresanrequested to merge
debug-retarget-policy-worker-failure into master

What does this MR do and why?

The security_policy_target_branch_desync_detection feature flag was rolled out to 10% and 25% on GitLab.com on 2026-09-01 and rolled back the same day. It caused two problems: the resync worker raised ActiveRecord::RecordInvalid on "sourceless" MR rules (rules whose approval_merge_request_rule_source row was lost to a cascade delete or a failed sync), leaving affected MRs permanently stuck in checking; and the detection ran on merged and closed MRs too, because mergeabilityChecks executes all checks regardless of state, so it logged security_policy_approval_rules_desync_detected and enqueued no-op resyncs for nearly every merged or closed MR in a policy-covered project.

This MR fixes both. The detection now only runs against open MRs and ignores legacy project rules with no approval_policy_rule_id (policy sync never copies those to MRs, so they were flagged forever). The rule sync now looks for a sourceless MR rule matching the uniqueness key before creating a new one, so it heals and relinks the existing rule instead of hitting the name uniqueness validation. The worker also rescues ActiveRecord::RecordInvalid and reports it via Gitlab::ErrorTracking instead of retrying, since the check re-enqueues the worker anyway.

  • check_security_policy_violations_service.rb: approval_rules_out_of_sync? returns false unless the MR is open.
  • ee/merge_request.rb: missing_policy_approval_rules? excludes project rules with a nil approval_policy_rule_id.
  • approval_project_rule.rb: merge_request_report_approver_rule adopts a matching sourceless MR rule before building a new one, so update! relinks the source instead of duplicating the rule. Shared by every sync entry point (MR create, retarget, reopen, policy sync), not just the worker.
  • resync_merge_request_rules_worker.rb: rescues ActiveRecord::RecordInvalid, tracks it via Gitlab::ErrorTracking.track_exception with merge_request_id, and still calls schedule_policy_synchronization.

References

Screenshots or screen recordings

Not applicable. This is a backend fix with no UI changes.

How to set up and validate locally

This requires Ultimate, a project with a protected main branch, and a merge request approval policy requiring approvals on protected branches. Enable the flag for the project:

Feature.enable(:security_policy_target_branch_desync_detection, Project.find(<id>))

  1. Sourceless rule repro. Open an MR targeting main so it picks up policy rules, then delete its source rows:

    mr.approval_rules.report_approver.each { |r| r.approval_merge_request_rule_source&.delete }

    Before this MR:

    MergeRequests::Mergeability::CheckSecurityPolicyViolationsService.new(merge_request: mr, params: {}).execute.status
# => :checking
Security::ScanResultPolicies::ResyncMergeRequestRulesWorker.new.perform(mr.id)
# raises ActiveRecord::RecordInvalid

    After this MR, the worker succeeds, mr.approval_rules.report_approver.map(&:approval_project_rule) are all present with the rule count unchanged, and the check no longer returns :checking for the desync reason.

  2. Merged MR repro. Take a merged MR in the project and run the check service on it. Before this MR it returns :checking and logs a security_policy_approval_rules_desync_detected line. After this MR it returns :inactive with no log.

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.

  • No database changes or migrations.
  • No documentation changes.
  • No UI changes.
  • Commit carries Changelog: fixed with EE: true, so no separate changelog entry is needed.
  • Tests added: check service spec (merged/closed MRs return inactive, no log, no enqueue), merge request spec (legacy nil-policy-rule rule alone does not trigger detection; a synced rule that lost its source still does), approval project rule spec (sourceless rule is adopted and relinked, no duplicate), worker spec (RecordInvalid is tracked and does not raise, sync still scheduled). 76 examples pass, RuboCop clean.

Merge request reports