Constrain VirusScanWorker to one concurrent worker per asset

Gemfile

@@ -19,6 +19,7 @@ gem "plek"
 gem "rack_strip_client_ip"
 gem "rails-controller-testing"
 gem "sentry-sidekiq"
+gem "sidekiq-unique-jobs"
 gem "sprockets-rails"
 gem "state_machines-mongoid"
 
@@ -31,6 +32,7 @@ group :development, :test do
   gem "pact", require: false
   gem "pact_broker-client", require: false
   gem "rspec-rails"
+  gem "rspec-sidekiq"
   gem "rubocop-govuk"
   gem "simplecov"
   gem "webmock", require: false

Gemfile.lock

@@ -100,6 +100,9 @@ GEM
     bootsnap (1.17.0)
       msgpack (~> 1.2)
     brakeman (6.1.0)
+    brpoplpush-redis_script (0.1.3)
+      concurrent-ruby (~> 1.0, >= 1.0.5)
+      redis (>= 1.0, < 6)
     bson (4.15.0)
     builder (3.2.4)
     byebug (11.1.3)
@@ -601,6 +604,11 @@ GEM
       rspec-expectations (~> 3.12)
       rspec-mocks (~> 3.12)
       rspec-support (~> 3.12)
+    rspec-sidekiq (4.1.0)
+      rspec-core (~> 3.0)
+      rspec-expectations (~> 3.0)
+      rspec-mocks (~> 3.0)
+      sidekiq (>= 5, < 8)
     rspec-support (3.12.1)
     rubocop (1.55.0)
       json (~> 2.3)
@@ -651,6 +659,12 @@ GEM
       connection_pool (>= 2.2.5, < 3)
       rack (~> 2.0)
       redis (>= 4.5.0, < 5)
+    sidekiq-unique-jobs (7.1.31)
+      brpoplpush-redis_script (> 0.1.1, <= 2.0.0)
+      concurrent-ruby (~> 1.0, >= 1.0.5)
+      redis (< 5.0)
+      sidekiq (>= 5.0, < 7.0)
+      thor (>= 0.20, < 3.0)
     simplecov (0.22.0)
       docile (~> 1.1)
       simplecov-html (~> 0.11)
@@ -736,8 +750,10 @@ DEPENDENCIES
   rails (= 7.1.2)
   rails-controller-testing
   rspec-rails
+  rspec-sidekiq
   rubocop-govuk
   sentry-sidekiq
+  sidekiq-unique-jobs
   simplecov
   sprockets-rails
   state_machines-mongoid

app/workers/virus_scan_worker.rb

@@ -3,18 +3,15 @@
 class VirusScanWorker
   include Sidekiq::Worker
 
+  sidekiq_options lock: :until_and_while_executing
+
   def perform(asset_id)
     asset = Asset.find(asset_id)
     if asset.unscanned?
       begin
         Rails.logger.info("#{asset_id} - VirusScanWorker#perform - Virus scan started")
         Services.virus_scanner.scan(asset.file.path)
-        # 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?
+        asset.scanned_clean!
       rescue VirusScanner::InfectedFile => e
         GovukError.notify(e, extra: { id: asset.id, filename: asset.filename })
         asset.scanned_infected!

config/initializers/sidekiq.rb

@@ -1,2 +1,25 @@
+SidekiqUniqueJobs.configure do |config|
+  config.enabled = !Rails.env.test? # SidekiqUniqueJobs recommends not testing this behaviour https://github.com/mhenrixon/sidekiq-unique-jobs#uniqueness
+  config.lock_ttl = 1.hour
+end
+
+Sidekiq.configure_client do |config|
+  config.client_middleware do |chain|
+    chain.add SidekiqUniqueJobs::Middleware::Client
+  end
+end
+
+Sidekiq.configure_server do |config|
+  config.client_middleware do |chain|
+    chain.add SidekiqUniqueJobs::Middleware::Client
+  end
+
+  config.server_middleware do |chain|
+    chain.add SidekiqUniqueJobs::Middleware::Server
+  end
+
+  SidekiqUniqueJobs::Server.configure(config)
+end
+
 # Use Sidekiq strict args to force Sidekiq 6 deprecations to error ahead of upgrade to Sidekiq 7
 Sidekiq.strict_args!

spec/workers/virus_scan_worker_spec.rb

@@ -1,5 +1,6 @@
 require "rails_helper"
 require "services"
+require "sidekiq_unique_jobs/testing"
 
 RSpec.describe VirusScanWorker do
   let(:worker) { described_class.new }
@@ -10,6 +11,15 @@
     allow(Services).to receive(:virus_scanner).and_return(scanner)
   end
 
+  specify { expect(described_class).to have_valid_sidekiq_options }
+
+  it "does not permit multiple jobs to be enqueued for the same asset" do
+    SidekiqUniqueJobs.use_config(enabled: true) do
+      expect { described_class.perform_in(1.minute, asset.id.to_s) }.to enqueue_sidekiq_job(described_class)
+      expect { described_class.perform_in(1.minute, asset.id.to_s) }.not_to enqueue_sidekiq_job(described_class)
+    end
+  end
+
   it "calls out to the VirusScanner to scan the file" do
     expect(scanner).to receive(:scan).with(asset.file.path)
 
@@ -39,18 +49,6 @@
     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
-
-      worker.perform(asset.id)
-
-      expect(asset.reload).not_to be_clean
-    end
-  end
-
   context "when the asset is already marked as infected" do
     let(:asset) { FactoryBot.create(:infected_asset) }