Fix approval-policy rules silently failing to resync

Problem

Three related gaps in approval-policy resync (link_policy/unlink_policy/resync) can leave a project without its MR approval-policy rules, with no error surfaced anywhere:

  1. Orphaned scan_result_policy_read blocks all future resyncs. A row can be left behind by an interrupted resync (partial failure, retry) holding the same (config, project, policy_idx, rule_idx, action_idx) uniqueness slot as the row a later resync tries to create, but with a stale approval_policy_rule_id. The existing "already exists?" guard keys off approval_policy_rule_id, so it never finds the stale row, and create! raises ActiveRecord::RecordInvalid on the DB uniqueness validation instead.
  2. Non-atomic unlink. Security::Policy#unlink_project! deleted the ApprovalPolicyRuleProjectLink join row in its own transaction; the matching ApprovalProjectRule row was deleted separately, later. Between the two, the rule row existed with no join row, and EE::MergeRequest#linked_report_approver_project_rules treats that as "unlinked" and silently skips syncing it onto merge requests.
  3. Silent discard. SyncProjectPolicyWorker's sidekiq_retry_in discards a job that raised RecordInvalid — for a resync event, permanently, with no retry — and never logged it. Combined with (1), a project can stop resyncing its approval rules indefinitely with zero trace in the application logs.

What changed

  1. Security::ScanResultPolicies::ApprovalRules::CreateService#create_scan_result_policy looks up the row occupying the DB uniqueness slot (security_orchestration_policy_configuration_id, project_id, orchestration_policy_idx, rule_idx, action_idx) before creating. If nothing owns that row any more (its approval_policy_rule_id is NULL, or no Security::ApprovalPolicyRuleProjectLink exists for its rule and this project), the service deletes the row and creates a fresh one; its dependent approval_project_rules, scan_result_policy_violations, and software_license_policies go with it through cascading foreign keys. If the row still belongs to a rule linked to the project, the service leaves it alone and create! raises as before. This matters because a policy or rule reorder briefly leaves two policies claiming one orchestration_policy_idx (update_rearranged_policies reassigns policy_index); overwriting a live row would make its ApprovalProjectRule enforce another policy's content with nothing to resync it back. Raising there is the self-healing path: the worker converts it into a resync.
  2. Security::Policy#unlink_project! now deletes the project's approval_project_rules for this policy in the same transaction as the Security::ApprovalPolicyRuleProjectLink row, so a report_approver rule can no longer exist without its link row. EE::MergeRequest#linked_report_approver_project_rules treats that state as unlinked and skips the rule when syncing a merge request.
  3. Security::SyncProjectPolicyWorker logs project, policy, event type, and exception message before discarding an ActiveRecord::RecordInvalid job, on both the resync-scheduling and terminal-discard branches. The exception already reaches Sentry and the Sidekiq structured log; what was missing was the decision to stop retrying, after which the project keeps stale rules until a new event arrives.

Testing

Each fix follows TDD (test written first, confirmed failing for the right reason against unfixed code, then fixed). Full regression sweep across every affected model/service/worker spec: 563 examples, 0 failures. RuboCop clean on all changed files. bundle exec keela clean (removed the now-stale for_rule_index unused-scope baseline entry, since this MR gives it a real caller).

The new scan_result_policy_reads lookup in (1) is fully covered by the existing unique index index_scan_result_policies_on_configuration_action_and_rule_idx — confirmed via EXPLAIN (ANALYZE, BUFFERS), sub-millisecond, no new index needed.

The ownership guard has a regression test for the reorder case (a live row of another linked policy must raise rather than be overwritten), confirmed failing when the guard is stubbed out. Latest sweep across the changed models, services and worker: 511 examples, 0 failures, 2 pending. RuboCop clean.

Database review notes

  • The new lookup is covered end-to-end by the existing unique index index_scan_result_policies_on_configuration_action_and_rule_idx:
SELECT "scan_result_policies".* FROM "scan_result_policies"
WHERE "scan_result_policies"."security_orchestration_policy_configuration_id" = $1
  AND "scan_result_policies"."project_id" = $2
  AND "scan_result_policies"."orchestration_policy_idx" = $3
  AND "scan_result_policies"."rule_idx" = $4
  AND "scan_result_policies"."action_idx" = $5
ORDER BY "scan_result_policies"."id" ASC
LIMIT 1
  • The ownership guard only runs when that lookup returns a row, so it costs nothing on the normal create path. It hits the unique index index_approval_policy_rule_on_project_and_rule:
SELECT 1 AS one FROM "approval_policy_rule_project_links"
WHERE "approval_policy_rule_project_links"."project_id" = $1
  AND "approval_policy_rule_project_links"."approval_policy_rule_id" = $2
LIMIT 1
  • delete_approval_project_rules_for_project keeps its each_batch and now runs inside the unlink_project! transaction. The row count is bounded: the policy schema caps a policy at maxItems: 5 rules, and rows are scoped to one project and one policy's rules, so this is a single batch in practice. Flagging it because batching inside a transaction reads oddly.
  • The project-rule delete now happens twice per unlink: once in unlink_project!, again through Security::ScanResultPolicies::ApprovalRules::DeleteService. The second is idempotent and load-bearing: sync_approval_policy_diff calls DeleteService without going through unlink_project!, so it still needs to delete project rules on its own.

Known adjacent gaps (not fixed here)

  • Security::ScanResultPolicies::ApprovalRules::UpdateService#update_scan_result_policy_read can still raise RecordInvalid on the same uniqueness scope. It has no equivalent guard.
  • CreateService still returns early when a scan_result_policy_read exists for the correct approval_policy_rule_id but its ApprovalProjectRule is missing, so a resync does not recreate that shape.
  • ActiveRecord::RecordInvalid is rare in production: one occurrence in seven days of gitlab.com logs, with create_service.rb in create_scan_result_policy in the backtrace. That confirms the path this MR fixes. Over the same window, the dominant Security::SyncProjectPolicyWorker failure is ActiveRecord::RecordNotUnique: 453 occurrences, 137 distinct jobs across 29 projects, retrying up to 18 times before dying. It is a PG::UniqueViolation on index_approval_project_rules_groups_1, key (approval_project_rule_id, group_id), raised from ee/app/services/concerns/approval_rules/updater.rb:102. The sidekiq_retry_in handler this MR touches only matches RecordInvalid, so those jobs get neither the log nor the resync. Reported separately in #625042; #596146 (closed) quotes the same constraint but is already closed.

References

  • Closes #625029 (closed)
  • #557737 — adjacent, acknowledged report of the same failure class (approval rules silently not created/updated on policy sync)
Edited by Alan (Maciej) Paruszewski

Merge request reports

Loading
Loading