Merge pull request #6608 from betagouv/feat/6547

FIX PJ - Lancer les jobs d'antivirus et analyse rails indépendamment

app/jobs/virus_scanner_job.rb

@@ -1,15 +1,8 @@
 class VirusScannerJob < ApplicationJob
-  class FileNotAnalyzedYetError < StandardError
-  end
-
-  queue_as :active_storage_analysis
-
   # If by the time the job runs the blob has been deleted, ignore the error
   discard_on ActiveRecord::RecordNotFound
   # If the file is deleted during the scan, ignore the error
   discard_on ActiveStorage::FileNotFoundError
-  # If the file is not analyzed yet, retry later (to avoid clobbering metadata)
-  retry_on FileNotAnalyzedYetError, wait: :exponentially_longer, attempts: 10
   # If for some reason the file appears invalid, retry for a while
   retry_on(ActiveStorage::IntegrityError, attempts: 5, wait: 5.seconds) do |job, _error|
     blob = job.arguments.first
@@ -23,7 +16,6 @@ class VirusScannerJob < ApplicationJob
   end
 
   def perform(blob)
-    if !blob.analyzed? then raise FileNotAnalyzedYetError end
     if blob.virus_scanner.done? then return end
 
     metadata = extract_metadata_via_virus_scanner(blob)

app/lib/active_job/retry_on_transient_errors.rb

@@ -5,7 +5,8 @@ module ActiveJob::RetryOnTransientErrors
     Excon::Error::InternalServerError,
     Excon::Error::GatewayTimeout,
     Excon::Error::BadRequest,
-    Excon::Error::Socket
+    Excon::Error::Socket,
+    ActiveRecord::StaleObjectError
   ]
 
   included do

db/migrate/20211104102349_add_lock_version_to_active_storage_blobs.rb

@@ -0,0 +1,5 @@
+class AddLockVersionToActiveStorageBlobs < ActiveRecord::Migration[6.1]
+  def change
+    add_column :active_storage_blobs, :lock_version, :integer
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2021_10_26_131800) do
+ActiveRecord::Schema.define(version: 2021_11_04_102349) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -45,6 +45,7 @@ ActiveRecord::Schema.define(version: 2021_10_26_131800) do
     t.string "checksum", null: false
     t.datetime "created_at", null: false
     t.string "service_name", null: false
+    t.integer "lock_version"
     t.index ["key"], name: "index_active_storage_blobs_on_key", unique: true
   end
 

spec/jobs/virus_scanner_job_spec.rb

@@ -7,57 +7,67 @@ describe VirusScannerJob, type: :job do
     VirusScannerJob.perform_now(blob)
   end
 
-  context "when the blob is not analyzed yet" do
-    it "retries the job later" do
-      expect { subject }.to have_enqueued_job(VirusScannerJob)
+  context "when the virus scan launch before rails analyze" do
+    before do
+      allow(ClamavService).to receive(:safe_file?).and_return(true)
+      subject
+      blob.analyze
+    end
+    it { expect(blob.virus_scanner.safe?).to be_truthy }
+    it { expect(blob.analyzed?).to be_truthy }
+    it { expect(blob.lock_version).to eq(2) }
+  end
+
+  context "should raise ActiveRecord::StaleObjectError" do
+    let(:blob_2) { ActiveStorage::Blob.find(blob.id) }
+    before do
+      blob_2.metadata[:virus_scan_result] = "infected"
+      blob.metadata[:virus_scan_result] = "safe"
+      blob.save
+    end
+
+    it { expect { blob_2.save }.to raise_error(ActiveRecord::StaleObjectError) }
+  end
+
+  context "when there is an integrity error" do
+    before do
+      blob.update_column('checksum', 'integrity error')
+
+      assert_performed_jobs(5) do
+        VirusScannerJob.perform_later(blob)
+      end
+    end
+
+    it do
+      expect(blob.reload.virus_scanner.corrupt?).to be_truthy
     end
   end
 
-  context "when the blob has been analyzed" do
+  context "when no virus is found" do
     before do
-      blob.analyze
+      allow(ClamavService).to receive(:safe_file?).and_return(true)
+      subject
     end
 
-    context "when there is an integrity error" do
-      before do
-        blob.update_column('checksum', 'integrity error')
+    it { expect(blob.virus_scanner.safe?).to be_truthy }
+  end
 
-        assert_performed_jobs(5) do
-          VirusScannerJob.perform_later(blob)
-        end
-      end
-
-      it do
-        expect(blob.reload.virus_scanner.corrupt?).to be_truthy
-      end
+  context "when a virus is found" do
+    before do
+      allow(ClamavService).to receive(:safe_file?).and_return(false)
+      subject
     end
 
-    context "when no virus is found" do
-      before do
-        allow(ClamavService).to receive(:safe_file?).and_return(true)
-        subject
-      end
+    it { expect(blob.virus_scanner.infected?).to be_truthy }
+  end
 
-      it { expect(blob.virus_scanner.safe?).to be_truthy }
+  context "when the blob has been deleted" do
+    before do
+      ActiveStorage::Blob.find(blob.id).purge
     end
 
-    context "when a virus is found" do
-      before do
-        allow(ClamavService).to receive(:safe_file?).and_return(false)
-        subject
-      end
-
-      it { expect(blob.virus_scanner.infected?).to be_truthy }
-    end
-
-    context "when the blob has been deleted" do
-      before do
-        ActiveStorage::Blob.find(blob.id).purge
-      end
-
-      it "ignores the error" do
-        expect { subject }.not_to raise_error
-      end
+    it "ignores the error" do
+      expect { subject }.not_to raise_error
     end
   end
 end