Only change the asset state after virus scan completes, if the asset …

…is unscanned

In production it looks like there are concurrency issues and the VirusScannerWorker
executes several times for the same file. When several processes attempt to change
the asset state at the same time, it can causes other issues. Specifically any attempt
to read or remove the local file after the state transition can fail.

app/workers/virus_scan_worker.rb

@@ -9,7 +9,12 @@ def perform(asset_id)
       begin
         Rails.logger.info("#{asset_id} - VirusScanWorker#perform - Virus scan started")
         Services.virus_scanner.scan(asset.file.path)
-        asset.scanned_clean!
+        # This is to deal with a concurrency issue in production
+        # It looks like sometimes VirusScannerWorker can be scheduled several times for one file
+        # And the other process might change the state already of as the virus scan take time
+        # This if condition is to avoid unnecessary state transitions and therefore other
+        # concurrency issues
+        asset.scanned_clean! if Asset.find(asset_id).unscanned?
       rescue VirusScanner::InfectedFile => e
         GovukError.notify(e, extra: { id: asset.id, filename: asset.filename })
         asset.scanned_infected!

spec/workers/virus_scan_worker_spec.rb

@@ -39,6 +39,18 @@
     end
   end
 
+  context "when the asset becomes marked clean by another process" do
+    it "does change the asset state" do
+      allow(scanner).to receive(:scan) do
+        asset.scanned_infected!
+      end
+
+      expect_any_instance_of(Asset).not_to receive(:scanned_clean!)
+
+      worker.perform(asset.id)
+    end
+  end
+
   context "when the asset is already marked as infected" do
     let(:asset) { FactoryBot.create(:infected_asset) }