From 9b6896579d84ed2202320415f5917e0da042883a Mon Sep 17 00:00:00 2001 From: Bruce Bolt Date: Wed, 18 Sep 2024 08:56:00 +0100 Subject: [PATCH 1/6] Upgrade to Sidekiq 7 (by bumping `govuk_sidekiq` to version 9.0.0) --- Gemfile.lock | 35 +++++++++++++++-------------------- 1 file changed, 15 insertions(+), 20 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index abca5eff..080591b3 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -97,9 +97,6 @@ GEM msgpack (~> 1.2) brakeman (6.2.1) racc - brpoplpush-redis_script (0.1.3) - concurrent-ruby (~> 1.0, >= 1.0.5) - redis (>= 1.0, < 6) bson (5.0.1) builder (3.3.0) byebug (11.1.3) @@ -195,12 +192,11 @@ GEM sentry-rails (~> 5.3) sentry-ruby (~> 5.3) statsd-ruby (~> 1.5) - govuk_sidekiq (8.0.1) + govuk_sidekiq (9.0.0) gds-api-adapters (>= 19.1.0) govuk_app_config (>= 1.1) - redis (< 5) - redis-namespace (~> 1.6) - sidekiq (~> 6.5, >= 6.5.12) + redis-client (>= 0.22.2) + sidekiq (~> 7.0, < 8) hashdiff (1.1.1) hashie (5.0.0) http-accept (1.7.0) @@ -596,9 +592,8 @@ GEM ffi (~> 1.0) rdoc (6.7.0) psych (>= 4.0.0) - redis (4.8.1) - redis-namespace (1.11.0) - redis (>= 4) + redis-client (0.22.2) + connection_pool regexp_parser (2.9.2) reline (0.5.10) io-console (~> 0.5) @@ -682,16 +677,16 @@ GEM sentry-sidekiq (5.19.0) sentry-ruby (~> 5.19.0) sidekiq (>= 3.0) - sidekiq (6.5.12) - connection_pool (>= 2.2.5, < 3) - rack (~> 2.0) - redis (>= 4.5.0, < 5) - sidekiq-unique-jobs (7.1.33) - brpoplpush-redis_script (> 0.1.1, <= 2.0.0) + sidekiq (7.3.2) + concurrent-ruby (< 2) + connection_pool (>= 2.3.0) + logger + rack (>= 2.2.4) + redis-client (>= 0.22.2) + sidekiq-unique-jobs (8.0.10) concurrent-ruby (~> 1.0, >= 1.0.5) - redis (< 5.0) - sidekiq (>= 5.0, < 7.0) - thor (>= 0.20, < 3.0) + sidekiq (>= 7.0.0, < 8.0.0) + thor (>= 1.0, < 3.0) simplecov (0.22.0) docile (~> 1.1) simplecov-html (~> 0.11) @@ -741,7 +736,7 @@ GEM addressable (>= 2.8.0) crack (>= 0.3.2) hashdiff (>= 0.4.0, < 2.0.0) - webrick (1.8.1) + webrick (1.8.2) websocket-driver (0.7.6) websocket-extensions (>= 0.1.0) websocket-extensions (0.1.5) From 8e2b0ed222899c0d986dc0a39a749b6db13331d6 Mon Sep 17 00:00:00 2001 From: Bruce Bolt Date: Mon, 23 Sep 2024 10:40:45 +0100 Subject: [PATCH 2/6] Remove Sidekiq strict arguments We have now upgraded to Sidekiq 7, so don't need to require strict arguments in preparation for switching, as this version includes that requirement by default. --- config/initializers/sidekiq.rb | 3 --- 1 file changed, 3 deletions(-) diff --git a/config/initializers/sidekiq.rb b/config/initializers/sidekiq.rb index 605a1f0d..af25e713 100644 --- a/config/initializers/sidekiq.rb +++ b/config/initializers/sidekiq.rb @@ -21,6 +21,3 @@ 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! From 79edf5160fdbdd12d609c12605aa7d409f52347a Mon Sep 17 00:00:00 2001 From: Bruce Bolt Date: Mon, 23 Sep 2024 11:48:23 +0100 Subject: [PATCH 3/6] Rename `VirusScanWorker` `Sidekiq::Worker` has been deprecated in Sidekiq 7, so we need to replace it with `Sidekiq::Job`, and then rename the workers to be jobs. --- app/models/asset.rb | 2 +- .../virus_scan_worker.rb => sidekiq/virus_scan_job.rb} | 6 +++--- spec/models/asset_spec.rb | 10 +++++----- spec/requests/asset_requests_spec.rb | 2 +- spec/requests/virus_scanning_spec.rb | 2 +- .../virus_scan_job_spec.rb} | 2 +- 6 files changed, 12 insertions(+), 12 deletions(-) rename app/{workers/virus_scan_worker.rb => sidekiq/virus_scan_job.rb} (76%) rename spec/{workers/virus_scan_worker_spec.rb => sidekiq/virus_scan_job_spec.rb} (98%) diff --git a/app/models/asset.rb b/app/models/asset.rb index f8348045..f53c6147 100644 --- a/app/models/asset.rb +++ b/app/models/asset.rb @@ -243,7 +243,7 @@ def reset_state end def schedule_virus_scan - VirusScanWorker.perform_async(id.to_s) if unscanned? && redirect_url.blank? + VirusScanJob.perform_async(id.to_s) if unscanned? && redirect_url.blank? end def file_exists? diff --git a/app/workers/virus_scan_worker.rb b/app/sidekiq/virus_scan_job.rb similarity index 76% rename from app/workers/virus_scan_worker.rb rename to app/sidekiq/virus_scan_job.rb index 01abf553..6be9b368 100644 --- a/app/workers/virus_scan_worker.rb +++ b/app/sidekiq/virus_scan_job.rb @@ -1,7 +1,7 @@ require "services" -class VirusScanWorker - include Sidekiq::Worker +class VirusScanJob + include Sidekiq::Job sidekiq_options lock: :until_and_while_executing @@ -9,7 +9,7 @@ def perform(asset_id) asset = Asset.find(asset_id) if asset.unscanned? begin - Rails.logger.info("#{asset_id} - VirusScanWorker#perform - Virus scan started") + Rails.logger.info("#{asset_id} - VirusScanJob#perform - Virus scan started") Services.virus_scanner.scan(asset.file.path) asset.scanned_clean! rescue VirusScanner::InfectedFile => e diff --git a/spec/models/asset_spec.rb b/spec/models/asset_spec.rb index 17db8ae2..d62bcf11 100644 --- a/spec/models/asset_spec.rb +++ b/spec/models/asset_spec.rb @@ -456,7 +456,7 @@ it "schedules a scan after create" do a = described_class.new(file: load_fixture_file("asset.png")) - expect(VirusScanWorker).to receive(:perform_async).with(a.id) + expect(VirusScanJob).to receive(:perform_async).with(a.id) a.save! end @@ -465,7 +465,7 @@ a = FactoryBot.create(:clean_asset) a.file = load_fixture_file("lorem.txt") - expect(VirusScanWorker).to receive(:perform_async).with(a.id) + expect(VirusScanJob).to receive(:perform_async).with(a.id) a.save! end @@ -475,7 +475,7 @@ original_filename = a.file.send(:original_filename) a.file = load_fixture_file("lorem.txt", named: original_filename) - expect(VirusScanWorker).to receive(:perform_async).with(a.id) + expect(VirusScanJob).to receive(:perform_async).with(a.id) a.save! end @@ -484,7 +484,7 @@ a = FactoryBot.create(:clean_asset) a.created_at = 5.days.ago - expect(VirusScanWorker).not_to receive(:perform_async) + expect(VirusScanJob).not_to receive(:perform_async) a.save! end @@ -492,7 +492,7 @@ it "does not schedule a scan if a redirect url is present" do a = FactoryBot.create(:asset, redirect_url: "/some-redirect") - expect(VirusScanWorker).not_to receive(:perform_async) + expect(VirusScanJob).not_to receive(:perform_async) a.save! end diff --git a/spec/requests/asset_requests_spec.rb b/spec/requests/asset_requests_spec.rb index 50727eec..6a621d4c 100644 --- a/spec/requests/asset_requests_spec.rb +++ b/spec/requests/asset_requests_spec.rb @@ -152,7 +152,7 @@ it "does not result in an invalid transition error when a redirect is received in short succession after a create" do # use threads to simulate multiple sidekiq workers threads = [] - allow(VirusScanWorker).to receive(:perform_async) do |asset_id| + allow(VirusScanJob).to receive(:perform_async) do |asset_id| threads << Thread.new do sleep(0.5) asset = Asset.find(asset_id) diff --git a/spec/requests/virus_scanning_spec.rb b/spec/requests/virus_scanning_spec.rb index a76ccec0..a40df07c 100644 --- a/spec/requests/virus_scanning_spec.rb +++ b/spec/requests/virus_scanning_spec.rb @@ -22,7 +22,7 @@ expect(response).to have_http_status(:not_found) allow(Services.virus_scanner).to receive(:scan) - VirusScanWorker.drain + VirusScanJob.drain get download_media_path(id: asset, filename: "lorem.txt") expect(response).to have_http_status(:not_found) diff --git a/spec/workers/virus_scan_worker_spec.rb b/spec/sidekiq/virus_scan_job_spec.rb similarity index 98% rename from spec/workers/virus_scan_worker_spec.rb rename to spec/sidekiq/virus_scan_job_spec.rb index f6a7831b..17a8c76c 100644 --- a/spec/workers/virus_scan_worker_spec.rb +++ b/spec/sidekiq/virus_scan_job_spec.rb @@ -2,7 +2,7 @@ require "services" require "sidekiq_unique_jobs/testing" -RSpec.describe VirusScanWorker do +RSpec.describe VirusScanJob do let(:worker) { described_class.new } let(:asset) { FactoryBot.create(:asset) } let(:scanner) { instance_double(VirusScanner) } From a1ead6a827f80dbf0c4c030626edac8ebda62c1d Mon Sep 17 00:00:00 2001 From: Bruce Bolt Date: Mon, 23 Sep 2024 11:49:47 +0100 Subject: [PATCH 4/6] Rename `DeleteAssetFileFromNfsWorker` `Sidekiq::Worker` has been deprecated in Sidekiq 7, so we need to replace it with `Sidekiq::Job`, and then rename the workers to be jobs. --- app/models/asset.rb | 2 +- .../delete_asset_file_from_nfs_job.rb} | 6 +++--- config/brakeman.ignore | 2 +- lib/tasks/govuk_assets.rake | 2 +- spec/models/asset_spec.rb | 2 +- .../delete_asset_file_from_nfs_job_spec.rb} | 2 +- 6 files changed, 8 insertions(+), 8 deletions(-) rename app/{workers/delete_asset_file_from_nfs_worker.rb => sidekiq/delete_asset_file_from_nfs_job.rb} (56%) rename spec/{workers/delete_asset_file_from_nfs_worker_spec.rb => sidekiq/delete_asset_file_from_nfs_job_spec.rb} (93%) diff --git a/app/models/asset.rb b/app/models/asset.rb index f53c6147..f0c0b983 100644 --- a/app/models/asset.rb +++ b/app/models/asset.rb @@ -108,7 +108,7 @@ class Asset after_transition to: :uploaded do |asset, _| asset.save! - DeleteAssetFileFromNfsWorker.perform_in(5.minutes, asset.id.to_s) + DeleteAssetFileFromNfsJob.perform_in(5.minutes, asset.id.to_s) end end diff --git a/app/workers/delete_asset_file_from_nfs_worker.rb b/app/sidekiq/delete_asset_file_from_nfs_job.rb similarity index 56% rename from app/workers/delete_asset_file_from_nfs_worker.rb rename to app/sidekiq/delete_asset_file_from_nfs_job.rb index d87f04f5..103080d1 100644 --- a/app/workers/delete_asset_file_from_nfs_worker.rb +++ b/app/sidekiq/delete_asset_file_from_nfs_job.rb @@ -1,12 +1,12 @@ -class DeleteAssetFileFromNfsWorker - include Sidekiq::Worker +class DeleteAssetFileFromNfsJob + include Sidekiq::Job sidekiq_options queue: "low_priority" def perform(asset_id) asset = Asset.find(asset_id) if asset.uploaded? FileUtils.rm_rf(File.dirname(asset.file.path)) - Rails.logger.info("#{asset.id} - DeleteAssetFileFromNfsWorker - File removed") + Rails.logger.info("#{asset.id} - DeleteAssetFileFromNfsJob - File removed") end end end diff --git a/config/brakeman.ignore b/config/brakeman.ignore index a71ec30b..fb1efd5b 100644 --- a/config/brakeman.ignore +++ b/config/brakeman.ignore @@ -13,7 +13,7 @@ "render_path": null, "location": { "type": "method", - "class": "DeleteAssetFileFromNfsWorker", + "class": "DeleteAssetFileFromNfsJob", "method": "perform" }, "user_input": "Asset.find(asset_id).file", diff --git a/lib/tasks/govuk_assets.rake b/lib/tasks/govuk_assets.rake index 8b6addfb..38c4fa7d 100644 --- a/lib/tasks/govuk_assets.rake +++ b/lib/tasks/govuk_assets.rake @@ -5,7 +5,7 @@ namespace :govuk_assets do task delete_file_from_nfs_for_assets_uploaded_to_s3: :environment do processor = AssetProcessor.new(scope: Asset.where(state: "uploaded")) processor.process_all_assets_with do |asset_id| - DeleteAssetFileFromNfsWorker.perform_async(asset_id.to_s) + DeleteAssetFileFromNfsJob.perform_async(asset_id.to_s) end end diff --git a/spec/models/asset_spec.rb b/spec/models/asset_spec.rb index d62bcf11..4970541b 100644 --- a/spec/models/asset_spec.rb +++ b/spec/models/asset_spec.rb @@ -1153,7 +1153,7 @@ end it "triggers the delete asset file worker" do - expect(DeleteAssetFileFromNfsWorker).to receive(:perform_in) + expect(DeleteAssetFileFromNfsJob).to receive(:perform_in) asset.upload_success! end end diff --git a/spec/workers/delete_asset_file_from_nfs_worker_spec.rb b/spec/sidekiq/delete_asset_file_from_nfs_job_spec.rb similarity index 93% rename from spec/workers/delete_asset_file_from_nfs_worker_spec.rb rename to spec/sidekiq/delete_asset_file_from_nfs_job_spec.rb index 8b6840af..95eaa42f 100644 --- a/spec/workers/delete_asset_file_from_nfs_worker_spec.rb +++ b/spec/sidekiq/delete_asset_file_from_nfs_job_spec.rb @@ -1,6 +1,6 @@ require "rails_helper" -RSpec.describe DeleteAssetFileFromNfsWorker, type: :worker do +RSpec.describe DeleteAssetFileFromNfsJob, type: :worker do subject(:worker) { described_class.new } let(:asset) { FactoryBot.create(:asset, state:) } From a791ec06eca284452f3cfa9a518b10a637142c50 Mon Sep 17 00:00:00 2001 From: Bruce Bolt Date: Mon, 23 Sep 2024 11:50:42 +0100 Subject: [PATCH 5/6] Rename `SaveToCloudStorageWorker` `Sidekiq::Worker` has been deprecated in Sidekiq 7, so we need to replace it with `Sidekiq::Job`, and then rename the workers to be jobs. --- app/models/asset.rb | 2 +- .../save_to_cloud_storage_job.rb} | 4 ++-- spec/models/asset_spec.rb | 6 +++--- spec/requests/virus_scanning_spec.rb | 2 +- .../save_to_cloud_storage_job_spec.rb} | 2 +- 5 files changed, 8 insertions(+), 8 deletions(-) rename app/{workers/save_to_cloud_storage_worker.rb => sidekiq/save_to_cloud_storage_job.rb} (78%) rename spec/{workers/save_to_cloud_storage_worker_spec.rb => sidekiq/save_to_cloud_storage_job_spec.rb} (96%) diff --git a/app/models/asset.rb b/app/models/asset.rb index f0c0b983..78f9dab1 100644 --- a/app/models/asset.rb +++ b/app/models/asset.rb @@ -95,7 +95,7 @@ class Asset end after_transition to: :clean do |asset, _| - SaveToCloudStorageWorker.perform_async(asset.id.to_s) + SaveToCloudStorageJob.perform_async(asset.id.to_s) end event :scanned_infected do diff --git a/app/workers/save_to_cloud_storage_worker.rb b/app/sidekiq/save_to_cloud_storage_job.rb similarity index 78% rename from app/workers/save_to_cloud_storage_worker.rb rename to app/sidekiq/save_to_cloud_storage_job.rb index ef10f758..fffae7a4 100644 --- a/app/workers/save_to_cloud_storage_worker.rb +++ b/app/sidekiq/save_to_cloud_storage_job.rb @@ -1,7 +1,7 @@ require "services" -class SaveToCloudStorageWorker - include Sidekiq::Worker +class SaveToCloudStorageJob + include Sidekiq::Job def perform(asset_id) asset = Asset.undeleted.find(asset_id) diff --git a/spec/models/asset_spec.rb b/spec/models/asset_spec.rb index 4970541b..9136b9e8 100644 --- a/spec/models/asset_spec.rb +++ b/spec/models/asset_spec.rb @@ -503,7 +503,7 @@ let(:asset) { FactoryBot.build(:asset, state:) } before do - allow(SaveToCloudStorageWorker).to receive(:perform_async) + allow(SaveToCloudStorageJob).to receive(:perform_async) end it "sets the asset state to clean" do @@ -513,7 +513,7 @@ end it "schedules saving the asset to cloud storage" do - expect(SaveToCloudStorageWorker).to receive(:perform_async).with(asset.id) + expect(SaveToCloudStorageJob).to receive(:perform_async).with(asset.id) asset.scanned_clean! end @@ -551,7 +551,7 @@ let(:asset) { FactoryBot.build(:asset, state:) } it "does not schedule saving the asset to cloud storage" do - expect(SaveToCloudStorageWorker).not_to receive(:perform_async).with(asset.id) + expect(SaveToCloudStorageJob).not_to receive(:perform_async).with(asset.id) asset.scanned_infected! end diff --git a/spec/requests/virus_scanning_spec.rb b/spec/requests/virus_scanning_spec.rb index a40df07c..55730125 100644 --- a/spec/requests/virus_scanning_spec.rb +++ b/spec/requests/virus_scanning_spec.rb @@ -27,7 +27,7 @@ get download_media_path(id: asset, filename: "lorem.txt") expect(response).to have_http_status(:not_found) - SaveToCloudStorageWorker.drain + SaveToCloudStorageJob.drain get download_media_path(id: asset, filename: "lorem.txt") expect(response).to have_http_status(:found) diff --git a/spec/workers/save_to_cloud_storage_worker_spec.rb b/spec/sidekiq/save_to_cloud_storage_job_spec.rb similarity index 96% rename from spec/workers/save_to_cloud_storage_worker_spec.rb rename to spec/sidekiq/save_to_cloud_storage_job_spec.rb index b676a8a3..ba90f3b1 100644 --- a/spec/workers/save_to_cloud_storage_worker_spec.rb +++ b/spec/sidekiq/save_to_cloud_storage_job_spec.rb @@ -1,6 +1,6 @@ require "rails_helper" -RSpec.describe SaveToCloudStorageWorker, type: :worker do +RSpec.describe SaveToCloudStorageJob, type: :worker do let(:worker) { described_class.new } describe "#perform" do From ad280c674affbe8a2639ffd982750e4e14fd3547 Mon Sep 17 00:00:00 2001 From: Bruce Bolt Date: Mon, 23 Sep 2024 13:26:51 +0100 Subject: [PATCH 6/6] Pin `sidekiq-unique-jobs` to version 8.0.7 or earlier A [bug in version 8.0.8](https://github.com/mhenrixon/sidekiq-unique-jobs/issues/846) means jobs never get processed when using `until_and_while_executing` lock type. Therefore pinning to use an earlier version that does not have this issue. --- Gemfile | 2 +- Gemfile.lock | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Gemfile b/Gemfile index 83cafa4d..0a523a36 100644 --- a/Gemfile +++ b/Gemfile @@ -20,7 +20,7 @@ gem "plek" gem "rack_strip_client_ip" gem "rails-controller-testing" gem "sentry-sidekiq" -gem "sidekiq-unique-jobs" +gem "sidekiq-unique-jobs", "< 8.0.8" gem "sprockets-rails" gem "state_machines-mongoid" diff --git a/Gemfile.lock b/Gemfile.lock index 080591b3..4fec8634 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -683,7 +683,7 @@ GEM logger rack (>= 2.2.4) redis-client (>= 0.22.2) - sidekiq-unique-jobs (8.0.10) + sidekiq-unique-jobs (8.0.7) concurrent-ruby (~> 1.0, >= 1.0.5) sidekiq (>= 7.0.0, < 8.0.0) thor (>= 1.0, < 3.0) @@ -779,7 +779,7 @@ DEPENDENCIES rspec-sidekiq rubocop-govuk sentry-sidekiq - sidekiq-unique-jobs + sidekiq-unique-jobs (< 8.0.8) simplecov sprockets-rails state_machines-mongoid