Merge branch...

Merge branch '357422-activerecord-querycanceled-pg-querycanceled-error-canceling-statement-due-to-statement' into 'master'

Resolve "ActiveRecord::QueryCanceled: PG::QueryCanceled: ERROR:  canceling statement due to statement timeout"

See merge request gitlab-org/gitlab!84283

ee/app/services/security/findings/cleanup_service.rb

@@ -4,7 +4,8 @@ module Security
   module Findings
     class CleanupService
       MAX_STALE_SCANS_SIZE = 50_000
-      BATCH_DELETE_SIZE = 10_000
+      SCAN_BATCH_SIZE = 100
+      BATCH_DELETE_SIZE = 100
 
       class << self
         def delete_stale_records
@@ -25,7 +26,7 @@ module Security
       end
 
       def execute
-        security_scans.in_batches(&method(:purge)) # rubocop:disable Cop/InBatches (`each_batch` does not let us set a global limit of records to be batched)
+        security_scans.in_batches(of: SCAN_BATCH_SIZE, &method(:purge)) # rubocop:disable Cop/InBatches (`each_batch` does not let us set a global limit of records to be batched)
       end
 
       private
@@ -33,14 +34,16 @@ module Security
       attr_reader :security_scans
 
       def purge(scan_batch)
-        delete_findings_for(scan_batch)
+        delete_findings_for(scan_batch.where_values_hash['id'])
 
-        scan_batch.update_all(status: :purged)
+        scan_batch.unscope(where: :build_id).update_all(status: :purged) # rubocop:disable CodeReuse/ActiveRecord unlikely that a `unscope where build_id` scope would be used elsewhere
       end
 
       def delete_findings_for(scan_batch)
-        Security::Finding.by_scan(scan_batch).each_batch(of: BATCH_DELETE_SIZE) do |finding_batch|
-          finding_batch.delete_all
+        findings_relation = Security::Finding.by_scan(scan_batch).limit(BATCH_DELETE_SIZE)
+
+        loop do
+          break if findings_relation.delete_all < BATCH_DELETE_SIZE
         end
       end
     end

ee/spec/services/security/findings/cleanup_service_spec.rb

@@ -47,19 +47,39 @@ RSpec.describe Security::Findings::CleanupService do
   end
 
   describe '#execute' do
-    let(:security_scan) { create(:security_scan) }
-    let(:relation) { Security::Scan.where(id: security_scan.id) }
+    let(:security_scans) { create_list(:security_scan, 2) }
+    let(:relation) { Security::Scan.where(id: security_scans.map(&:id)) }
     let(:service_object) { described_class.new(relation) }
 
     subject(:cleanup_findings) { service_object.execute }
 
     before do
-      create_list(:security_finding, 2, scan: security_scan)
+      expect(relation).to receive(:unscope).with(where: :build_id).and_call_original
+      create_list(:security_finding, 2, scan: security_scans.first)
+      create_list(:security_finding, 2, scan: security_scans.last)
     end
 
     it 'deletes the findings of the given security scan object and marks the scan as purged' do
-      expect { cleanup_findings }.to change { security_scan.findings.count }.from(2).to(0)
-                                 .and change { security_scan.reload.status }.to('purged')
+      expect { cleanup_findings }.to change { security_scans.map(&:reload).sum(&:findings).count }.from(4).to(0)
+                                 .and change { Security::Scan.purged.count }.from(0).to(2)
+    end
+
+    context 'when iterating through security findings' do
+      let(:finding_scope) { instance_double(ActiveRecord::Relation) }
+
+      before do
+        allow(Security::Finding).to receive(:by_scan).and_return(finding_scope)
+        allow(finding_scope).to receive(:limit).with(100).and_return(finding_scope)
+        allow(finding_scope).to receive(:delete_all).and_return(2, 0)
+
+        cleanup_findings
+      end
+
+      it 'deletes findings in batches of 100' do
+        expect(Security::Finding).to have_received(:by_scan).with(relation.map(&:id))
+        expect(finding_scope).to have_received(:limit).with(100)
+        expect(finding_scope).to have_received(:delete_all)
+      end
     end
   end
 end