Use set lookups for dropped identifier resolution

🤖 AI wrote this code from my instructions.

Unreviewed AI output often lands on reviewers and costs them time.
This MR is human verified: I read the full diff and ran the tests myself, because your time is as valuable as mine.

What does this MR do and why?

ScheduleMarkDroppedAsResolvedService decides which vulnerabilities a scan no longer reports, so they can be auto-resolved. Two predicates run once per existing identifier, and each rescanned the whole primary_identifiers array. primary_identifier_types was not memoized either, so it rebuilt its array on every call.

That is O(n*m) comparisons for n reported and m existing identifiers, plus m array allocations of size n. Both numbers reach the thousands on a project that has accumulated vulnerabilities.

Build the two lookups once as memoized sets. to_set also drops the now redundant uniq. Behaviour is unchanged, since primary_identifiers is never mutated inside the service.

One gap this MR does not close: the type_unresovable? branch has no spec coverage. It had none before this change either. A follow-up MR adds it.

References

None.

How to set up and validate locally

Specs:

bundle exec rspec ee/spec/services/security/ingestion/schedule_mark_dropped_as_resolved_service_spec.rb

Measured with benchmark-swap(note that, this is not a microbenchmark. This measures the overall performance improvement for the entire service.), which swaps the method bodies in place and runs both sides through benchmark-ips. original is this branch, swapped is master, so a slower line on swapped is the improvement.

Existing vulnerabilities	Scanned identifiers	original	swapped
50	200	206.1 i/s	132.4 i/s	1.56x
200	1000	98.4 i/s	14.9 i/s	6.62x
500	2000	43.2 i/s	3.2 i/s	13.64x

At 500 and 2000 that is 23 ms against 313 ms per execute. The gain grows with the number of vulnerabilities a project has accumulated. On a small project it is close to noise, because the database work is the same on both sides and dominates.

Reproduction script

Save as ee/spec/services/security/ingestion/bench_dropped_identifiers_spec.rb, run it, then delete it. It has to sit under a recognised spec path or Quality::TestLevel refuses to load the file.

VULNS=500 SCANNED=2000 bundle exec rspec ee/spec/services/security/ingestion/bench_dropped_identifiers_spec.rb

The identifiers the scan reports never match the existing ones, so present_in_scan? never short-circuits. That is the worst case. A rescan where most identifiers still match shows less. Sidekiq enqueueing is stubbed on both sides so it is not measured. Benchmark.swap verifies both sides return the same result before measuring, and reported no mismatch at any size.

# frozen_string_literal: true

require 'spec_helper'
require 'benchmark/swap'

RSpec.describe Security::Ingestion::ScheduleMarkDroppedAsResolvedService,
  feature_category: :static_application_security_testing do
  VULNS = Integer(ENV.fetch('VULNS', 300))
  SCANNED = Integer(ENV.fetch('SCANNED', 1000))

  let_it_be(:project) { create(:project) }
  let_it_be(:pipeline) { create(:ci_pipeline, project: project) }

  let_it_be(:existing) do
    Array.new(VULNS) do |i|
      identifier = create(:vulnerabilities_identifier,
        project: project,
        external_type: 'find_sec_bugs_type',
        external_id: "EXISTING_#{i}")

      finding = create(:vulnerabilities_finding,
        project_id: project.id,
        pipeline: pipeline,
        primary_identifier_id: identifier.id,
        identifiers: [identifier])

      create(:vulnerability, :detected,
        report_type: :sast,
        resolved_on_default_branch: true,
        project: project,
        vulnerability_finding: finding,
        findings: [finding]).tap { |vuln| finding.update!(vulnerability_id: vuln.id) }
    end
  end

  let(:scanned_identifiers) do
    Array.new(SCANNED) do |i|
      build(:ci_reports_security_identifier,
        external_type: 'find_sec_bugs_type',
        external_id: "SCANNED_#{i}")
    end
  end

  before do
    allow(::Vulnerabilities::MarkDroppedAsResolvedWorker).to receive(:perform_in)

    # master's implementations. benchmark-swap finds twins as :"#{name}#{suffix}",
    # so a predicate twin is `type_unresovable?_perf`, which needs define_method.
    described_class.class_eval do
      define_method(:"type_unresovable?_perf") do |existing_identifier|
        primary_identifiers.map(&:external_type).uniq.exclude?(existing_identifier.external_type)
      end
      private :"type_unresovable?_perf"

      define_method(:"present_in_scan?_perf") do |existing_identifier|
        primary_identifiers.any? do |incoming_identifier|
          incoming_identifier.external_type == existing_identifier.external_type &&
            incoming_identifier.external_id == existing_identifier.external_id
        end
      end
      private :"present_in_scan?_perf"
    end
  end

  after do
    described_class.send(:remove_method, :"type_unresovable?_perf")
    described_class.send(:remove_method, :"present_in_scan?_perf")
  end

  it 'compares the whole service' do
    existing

    puts "\nexisting vulnerabilities = #{VULNS}, scanned identifiers = #{SCANNED}"

    Benchmark.swap(time: 5, warmup: 2) do
      described_class.new(project.id, 'sast', scanned_identifiers).execute
    end
  end
end

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.