From 475bf0bd2dfa6ad215248138acfb0d2899bf97e7 Mon Sep 17 00:00:00 2001 From: kara Diaby Date: Thu, 4 Nov 2021 11:27:23 +0100 Subject: [PATCH 1/3] add lock version to blobs --- ...0211104102349_add_lock_version_to_active_storage_blobs.rb | 5 +++++ db/schema.rb | 3 ++- 2 files changed, 7 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20211104102349_add_lock_version_to_active_storage_blobs.rb diff --git a/db/migrate/20211104102349_add_lock_version_to_active_storage_blobs.rb b/db/migrate/20211104102349_add_lock_version_to_active_storage_blobs.rb new file mode 100644 index 000000000..ef2e6e8b6 --- /dev/null +++ b/db/migrate/20211104102349_add_lock_version_to_active_storage_blobs.rb @@ -0,0 +1,5 @@ +class AddLockVersionToActiveStorageBlobs < ActiveRecord::Migration[6.1] + def change + add_column :active_storage_blobs, :lock_version, :integer + end +end diff --git a/db/schema.rb b/db/schema.rb index c992f0ae8..85850cb93 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2021_10_26_131800) do +ActiveRecord::Schema.define(version: 2021_11_04_102349) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -45,6 +45,7 @@ ActiveRecord::Schema.define(version: 2021_10_26_131800) do t.string "checksum", null: false t.datetime "created_at", null: false t.string "service_name", null: false + t.integer "lock_version" t.index ["key"], name: "index_active_storage_blobs_on_key", unique: true end From 9ee9389ba1f0209ccf3bd52d96634f359a49892c Mon Sep 17 00:00:00 2001 From: kara Diaby Date: Thu, 4 Nov 2021 12:01:00 +0100 Subject: [PATCH 2/3] add the good retry_on StaleObjectError --- app/jobs/virus_scanner_job.rb | 8 -------- app/lib/active_job/retry_on_transient_errors.rb | 3 ++- 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/app/jobs/virus_scanner_job.rb b/app/jobs/virus_scanner_job.rb index a76ee8979..45bea9f61 100644 --- a/app/jobs/virus_scanner_job.rb +++ b/app/jobs/virus_scanner_job.rb @@ -1,15 +1,8 @@ class VirusScannerJob < ApplicationJob - class FileNotAnalyzedYetError < StandardError - end - - queue_as :active_storage_analysis - # If by the time the job runs the blob has been deleted, ignore the error discard_on ActiveRecord::RecordNotFound # If the file is deleted during the scan, ignore the error discard_on ActiveStorage::FileNotFoundError - # If the file is not analyzed yet, retry later (to avoid clobbering metadata) - retry_on FileNotAnalyzedYetError, wait: :exponentially_longer, attempts: 10 # If for some reason the file appears invalid, retry for a while retry_on(ActiveStorage::IntegrityError, attempts: 5, wait: 5.seconds) do |job, _error| blob = job.arguments.first @@ -23,7 +16,6 @@ class VirusScannerJob < ApplicationJob end def perform(blob) - if !blob.analyzed? then raise FileNotAnalyzedYetError end if blob.virus_scanner.done? then return end metadata = extract_metadata_via_virus_scanner(blob) diff --git a/app/lib/active_job/retry_on_transient_errors.rb b/app/lib/active_job/retry_on_transient_errors.rb index cf714174a..38a704a52 100644 --- a/app/lib/active_job/retry_on_transient_errors.rb +++ b/app/lib/active_job/retry_on_transient_errors.rb @@ -5,7 +5,8 @@ module ActiveJob::RetryOnTransientErrors Excon::Error::InternalServerError, Excon::Error::GatewayTimeout, Excon::Error::BadRequest, - Excon::Error::Socket + Excon::Error::Socket, + ActiveRecord::StaleObjectError ] included do From 1b27ab5aff13ac7116487633c13b423bd07db290 Mon Sep 17 00:00:00 2001 From: kara Diaby Date: Thu, 4 Nov 2021 12:01:16 +0100 Subject: [PATCH 3/3] tests --- spec/jobs/virus_scanner_job_spec.rb | 88 ++++++++++++++++------------- 1 file changed, 49 insertions(+), 39 deletions(-) diff --git a/spec/jobs/virus_scanner_job_spec.rb b/spec/jobs/virus_scanner_job_spec.rb index ce663cdac..2ea97bff6 100644 --- a/spec/jobs/virus_scanner_job_spec.rb +++ b/spec/jobs/virus_scanner_job_spec.rb @@ -7,57 +7,67 @@ describe VirusScannerJob, type: :job do VirusScannerJob.perform_now(blob) end - context "when the blob is not analyzed yet" do - it "retries the job later" do - expect { subject }.to have_enqueued_job(VirusScannerJob) + context "when the virus scan launch before rails analyze" do + before do + allow(ClamavService).to receive(:safe_file?).and_return(true) + subject + blob.analyze + end + it { expect(blob.virus_scanner.safe?).to be_truthy } + it { expect(blob.analyzed?).to be_truthy } + it { expect(blob.lock_version).to eq(2) } + end + + context "should raise ActiveRecord::StaleObjectError" do + let(:blob_2) { ActiveStorage::Blob.find(blob.id) } + before do + blob_2.metadata[:virus_scan_result] = "infected" + blob.metadata[:virus_scan_result] = "safe" + blob.save + end + + it { expect { blob_2.save }.to raise_error(ActiveRecord::StaleObjectError) } + end + + context "when there is an integrity error" do + before do + blob.update_column('checksum', 'integrity error') + + assert_performed_jobs(5) do + VirusScannerJob.perform_later(blob) + end + end + + it do + expect(blob.reload.virus_scanner.corrupt?).to be_truthy end end - context "when the blob has been analyzed" do + context "when no virus is found" do before do - blob.analyze + allow(ClamavService).to receive(:safe_file?).and_return(true) + subject end - context "when there is an integrity error" do - before do - blob.update_column('checksum', 'integrity error') + it { expect(blob.virus_scanner.safe?).to be_truthy } + end - assert_performed_jobs(5) do - VirusScannerJob.perform_later(blob) - end - end - - it do - expect(blob.reload.virus_scanner.corrupt?).to be_truthy - end + context "when a virus is found" do + before do + allow(ClamavService).to receive(:safe_file?).and_return(false) + subject end - context "when no virus is found" do - before do - allow(ClamavService).to receive(:safe_file?).and_return(true) - subject - end + it { expect(blob.virus_scanner.infected?).to be_truthy } + end - it { expect(blob.virus_scanner.safe?).to be_truthy } + context "when the blob has been deleted" do + before do + ActiveStorage::Blob.find(blob.id).purge end - context "when a virus is found" do - before do - allow(ClamavService).to receive(:safe_file?).and_return(false) - subject - end - - it { expect(blob.virus_scanner.infected?).to be_truthy } - end - - context "when the blob has been deleted" do - before do - ActiveStorage::Blob.find(blob.id).purge - end - - it "ignores the error" do - expect { subject }.not_to raise_error - end + it "ignores the error" do + expect { subject }.not_to raise_error end end end