Fix rule index collision when tombstoning deleted policy rules

What this does

Fixes an off-by-one in the rule tombstone index calculation that can make UpdateSecurityPoliciesService write a rule_index value a previous deletion already used, causing ActiveRecord::RecordNotUnique and rolling back the entire policy-type sync for a security policy configuration.

Root cause

mark_rules_for_deletion computes the tombstone index for newly-deleted rules from policy.max_rule_index, the current maximum absolute rule_index across all rules of the policy (live and tombstoned):

def mark_rules_for_deletion(policy, deleted_rules)
  new_index = policy.max_rule_index || 1
  deleted_rules.each_with_index do |rule_diff, index|
    rule_record = rule_diff.from
    rule_record.update!(rule_index: -(new_index + index))
  end
end

The first deleted rule in a batch gets -max_rule_index, landing exactly on the boundary of the existing index range instead of past it. If a rule tombstoned in an earlier sync already occupies that value, the update violates the unique index on (security_policy_id, rule_index).

The model already has the correct pattern for policy-level deletion, in ee/app/models/security/policy.rb:

def self.next_deletion_index
  (maximum("ABS(policy_index)") || 0) + 1
end

mark_rules_for_deletion never picked up the + 1.

Because PersistPolicyService#execute syncs all policies of one type for a configuration inside a single transaction, one policy hitting this collision rolls back the whole batch, including unrelated policies with valid pending changes. No error reaches the UI; the only trace is a Sidekiq exception and a worker that retries and fails on the same collision indefinitely.

The fix

new_index = policy.max_rule_index + 1

The || 1 fallback was dead code: max_rule_index already coalesces to 0 when a policy has no rules. Every newly-tombstoned rule now gets an index strictly past any existing one, live or tombstoned.

This also unblocks configurations already stuck on a pre-existing collision. No manual data fix needed: max_rule_index + 1 recomputes fresh on every run, so it walks past an orphaned tombstone regardless of when that tombstone was created. The "rule from an earlier deletion still occupies the next negative index" spec below covers this exact case: an orphan at -2, a genuine pending deletion, the new rule lands at -3, the orphan stays untouched. The next sync after this deploys resolves any already-stuck configuration on its own.

Orphaned tombstone rows still accumulate forever, since nothing purges them, but they stop blocking anything once this ships. A cleanup job for those rows is a separate follow-up, not part of this fix.

Tests

Added to ee/spec/services/security/security_orchestration_policies/update_security_policies_service_spec.rb:

  • A policy with an orphaned tombstone at rule_index = -2 alongside live rules at 1 and 2, then a genuine rule deletion. Before this change the update raised ActiveRecord::RecordNotUnique. After it, the newly-tombstoned rule lands at -3 and the existing tombstone at -2 is untouched.
  • A policy with no rules still creates its first rule at index 0, covering the max_rule_index == 0 path.

Updated three existing expectations that hardcoded the old off-by-one indices: two in this spec file, one in persist_policy_service_spec.rb.

Full run of update_security_policies_service_spec.rb, persist_policy_service_spec.rb, ee/spec/models/security/policy_spec.rb, and ee/spec/workers/security/persist_security_policies_worker_spec.rb: 353 examples, 0 failures. RuboCop clean on the changed files.

Closes #623550

Edited by Alan (Maciej) Paruszewski

Merge request reports

Loading
Loading