diff --git a/Gemfile b/Gemfile index a8ca33f5..6615340b 100644 --- a/Gemfile +++ b/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 diff --git a/Gemfile.lock b/Gemfile.lock index 26a2b3fa..c6d8b29a 100644 --- a/Gemfile.lock +++ b/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 diff --git a/app/workers/virus_scan_worker.rb b/app/workers/virus_scan_worker.rb index 46268f4d..01abf553 100644 --- a/app/workers/virus_scan_worker.rb +++ b/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! diff --git a/config/initializers/sidekiq.rb b/config/initializers/sidekiq.rb index 98860c61..17ef0bf5 100644 --- a/config/initializers/sidekiq.rb +++ b/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! diff --git a/spec/workers/virus_scan_worker_spec.rb b/spec/workers/virus_scan_worker_spec.rb index bffd9af8..d9e7ac6b 100644 --- a/spec/workers/virus_scan_worker_spec.rb +++ b/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) }