Skip to content

Commit

Permalink
Merge pull request #1493 from alphagov/sidekiq-7
Browse files Browse the repository at this point in the history
Upgrade to Sidekiq 7
  • Loading branch information
brucebolt authored Sep 25, 2024
2 parents 585a50a + ad280c6 commit 405f8e3
Show file tree
Hide file tree
Showing 15 changed files with 44 additions and 52 deletions.
2 changes: 1 addition & 1 deletion Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down
35 changes: 15 additions & 20 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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.7)
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)
Expand Down Expand Up @@ -784,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
Expand Down
6 changes: 3 additions & 3 deletions app/models/asset.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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

Expand Down Expand Up @@ -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?
Expand Down
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -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)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
require "services"

class VirusScanWorker
include Sidekiq::Worker
class VirusScanJob
include Sidekiq::Job

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")
Rails.logger.info("#{asset_id} - VirusScanJob#perform - Virus scan started")
Services.virus_scanner.scan(asset.file.path)
asset.scanned_clean!
rescue VirusScanner::InfectedFile => e
Expand Down
2 changes: 1 addition & 1 deletion config/brakeman.ignore
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
"render_path": null,
"location": {
"type": "method",
"class": "DeleteAssetFileFromNfsWorker",
"class": "DeleteAssetFileFromNfsJob",
"method": "perform"
},
"user_input": "Asset.find(asset_id).file",
Expand Down
3 changes: 0 additions & 3 deletions config/initializers/sidekiq.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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!
2 changes: 1 addition & 1 deletion lib/tasks/govuk_assets.rake
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
18 changes: 9 additions & 9 deletions spec/models/asset_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -484,15 +484,15 @@
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

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
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion spec/requests/asset_requests_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions spec/requests/virus_scanning_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,12 @@
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)

SaveToCloudStorageWorker.drain
SaveToCloudStorageJob.drain

get download_media_path(id: asset, filename: "lorem.txt")
expect(response).to have_http_status(:found)
Expand Down
Original file line number Diff line number Diff line change
@@ -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:) }
Expand Down
Original file line number Diff line number Diff line change
@@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) }
Expand Down

0 comments on commit 405f8e3

Please sign in to comment.