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:
- Orphaned
scan_result_policy_readblocks 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 staleapproval_policy_rule_id. The existing "already exists?" guard keys offapproval_policy_rule_id, so it never finds the stale row, andcreate!raisesActiveRecord::RecordInvalidon the DB uniqueness validation instead. - Non-atomic unlink.
Security::Policy#unlink_project!deleted theApprovalPolicyRuleProjectLinkjoin row in its own transaction; the matchingApprovalProjectRulerow was deleted separately, later. Between the two, the rule row existed with no join row, andEE::MergeRequest#linked_report_approver_project_rulestreats that as "unlinked" and silently skips syncing it onto merge requests. - Silent discard.
SyncProjectPolicyWorker'ssidekiq_retry_indiscards a job that raisedRecordInvalid— 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
Security::ScanResultPolicies::ApprovalRules::CreateService#create_scan_result_policylooks 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 (itsapproval_policy_rule_idisNULL, or noSecurity::ApprovalPolicyRuleProjectLinkexists for its rule and this project), the service deletes the row and creates a fresh one; its dependentapproval_project_rules,scan_result_policy_violations, andsoftware_license_policiesgo with it through cascading foreign keys. If the row still belongs to a rule linked to the project, the service leaves it alone andcreate!raises as before. This matters because a policy or rule reorder briefly leaves two policies claiming oneorchestration_policy_idx(update_rearranged_policiesreassignspolicy_index); overwriting a live row would make itsApprovalProjectRuleenforce another policy's content with nothing to resync it back. Raising there is the self-healing path: the worker converts it into a resync.Security::Policy#unlink_project!now deletes the project'sapproval_project_rulesfor this policy in the same transaction as theSecurity::ApprovalPolicyRuleProjectLinkrow, so areport_approverrule can no longer exist without its link row.EE::MergeRequest#linked_report_approver_project_rulestreats that state as unlinked and skips the rule when syncing a merge request.Security::SyncProjectPolicyWorkerlogs project, policy, event type, and exception message before discarding anActiveRecord::RecordInvalidjob, 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 1delete_approval_project_rules_for_projectkeeps itseach_batchand now runs inside theunlink_project!transaction. The row count is bounded: the policy schema caps a policy atmaxItems: 5rules, 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 throughSecurity::ScanResultPolicies::ApprovalRules::DeleteService. The second is idempotent and load-bearing:sync_approval_policy_diffcallsDeleteServicewithout going throughunlink_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_readcan still raiseRecordInvalidon the same uniqueness scope. It has no equivalent guard.CreateServicestill returns early when ascan_result_policy_readexists for the correctapproval_policy_rule_idbut itsApprovalProjectRuleis missing, so a resync does not recreate that shape.ActiveRecord::RecordInvalidis rare in production: one occurrence in seven days ofgitlab.comlogs, withcreate_service.rbincreate_scan_result_policyin the backtrace. That confirms the path this MR fixes. Over the same window, the dominantSecurity::SyncProjectPolicyWorkerfailure isActiveRecord::RecordNotUnique: 453 occurrences, 137 distinct jobs across 29 projects, retrying up to 18 times before dying. It is aPG::UniqueViolationonindex_approval_project_rules_groups_1, key(approval_project_rule_id, group_id), raised fromee/app/services/concerns/approval_rules/updater.rb:102. Thesidekiq_retry_inhandler this MR touches only matchesRecordInvalid, 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)