From 374b317fb43a5603f246c3039b02565b98b52909 Mon Sep 17 00:00:00 2001 From: 631583 Date: Fri, 1 Dec 2023 12:04:35 -0800 Subject: [PATCH 1/3] Simplify efolder manifest_source logic to prevent stuck manifest --- app/jobs/v2/download_manifest_job.rb | 2 +- app/models/manifest_source.rb | 23 +++++++++-------------- app/services/manifest_fetcher.rb | 18 +++++++++--------- spec/models/manifest_source_spec.rb | 15 +++++++++++++-- spec/services/manifest_fetcher_spec.rb | 6 +++--- 5 files changed, 35 insertions(+), 29 deletions(-) diff --git a/app/jobs/v2/download_manifest_job.rb b/app/jobs/v2/download_manifest_job.rb index 5ab952b24..8b0c76635 100644 --- a/app/jobs/v2/download_manifest_job.rb +++ b/app/jobs/v2/download_manifest_job.rb @@ -2,7 +2,7 @@ class V2::DownloadManifestJob < ApplicationJob queue_as :high_priority def perform(manifest_source, user = nil) - return if manifest_source.current? + manifest_source.update(status: :pending) RequestStore.store[:current_user] = user if user Raven.extra_context(manifest_source: manifest_source.id) diff --git a/app/models/manifest_source.rb b/app/models/manifest_source.rb index ceff265f3..65c9cff5c 100644 --- a/app/models/manifest_source.rb +++ b/app/models/manifest_source.rb @@ -20,21 +20,12 @@ class ManifestSource < ApplicationRecord SECONDS_TO_AUTO_UNLOCK = 5 def start! - s = Redis::Semaphore.new("download_manifest_source_#{id}".to_s, - url: Rails.application.secrets.redis_url_cache, - expiration: SECONDS_TO_AUTO_UNLOCK) - s.lock(SECONDS_TO_AUTO_UNLOCK) do - return if current? || pending? - update(status: :pending) - end - - begin - V2::DownloadManifestJob.perform_later(self, RequestStore[:current_user]) - rescue StandardError - update(status: :initialized) + return if current? || processing? + V2::DownloadManifestJob.perform_later(self, RequestStore[:current_user]) + rescue StandardError + update(status: :initialized) - raise - end + raise end def service @@ -53,4 +44,8 @@ def expiry_hours def current? success? && fetched_at && fetched_at > expiry_hours.hours.ago end + + def processing? + pending? && fetched_at && fetched_at > 24.hours.ago + end end diff --git a/app/services/manifest_fetcher.rb b/app/services/manifest_fetcher.rb index e6eb93594..17fd47b74 100644 --- a/app/services/manifest_fetcher.rb +++ b/app/services/manifest_fetcher.rb @@ -6,15 +6,15 @@ class ManifestFetcher EXCEPTIONS = [VBMS::ClientError, VVA::ClientError].freeze def process - begin - DocumentCreator.new(manifest_source: manifest_source, external_documents: documents).create - manifest_source.update!(status: :success, fetched_at: Time.zone.now) - documents - rescue *EXCEPTIONS => e - manifest_source.update!(status: :failed) - ExceptionLogger.capture(e) - [] - end + DocumentCreator.new(manifest_source: manifest_source, external_documents: documents).create + manifest_source.update!(status: :success, fetched_at: Time.zone.now) + documents + rescue *EXCEPTIONS => e + manifest_source.update!(status: :failed) + ExceptionLogger.capture(e) + raise e + ensure + return documents end def documents diff --git a/spec/models/manifest_source_spec.rb b/spec/models/manifest_source_spec.rb index 88524e69d..2aac89446 100644 --- a/spec/models/manifest_source_spec.rb +++ b/spec/models/manifest_source_spec.rb @@ -82,9 +82,9 @@ end end - context "when manifest is pending" do + context "when manifest is pending less than 24 hours" do before do - source.update_attributes!(fetched_at: Time.zone.now - 7.hours, status: :pending) + source.update_attributes!(fetched_at: Time.zone.now - 3.hours, status: :pending) end it "does not start the manifest job" do @@ -93,6 +93,17 @@ end end + context "when manifest is pending more than 24 hours" do + before do + source.update_attributes!(fetched_at: Time.zone.now - 25.hours, status: :pending) + end + + it "starts the manifest job" do + subject + expect(V2::DownloadManifestJob).to have_received(:perform_later) + end + end + context "when fetched less than 3 hours ago" do before do source.update_attributes!(fetched_at: Time.zone.now - 2.hours, status: :success) diff --git a/spec/services/manifest_fetcher_spec.rb b/spec/services/manifest_fetcher_spec.rb index 8856eea01..d4ef97f8e 100644 --- a/spec/services/manifest_fetcher_spec.rb +++ b/spec/services/manifest_fetcher_spec.rb @@ -70,8 +70,8 @@ allow(VBMSService).to receive(:v2_fetch_documents_for).and_raise(VBMS::ClientError) end - it "saves manifest status as failed" do - expect(subject).to eq [] + it "saves manifest status as failed and propagates errors" do + expect{ subject }.to raise_error(VBMS::ClientError) expect(source.reload.status).to eq "failed" expect(source.reload.fetched_at).to be_nil end @@ -99,7 +99,7 @@ end it "saves manifest status as failed" do - expect(subject).to eq [] + expect { subject }.to raise_error(VVA::ClientError) expect(source.reload.status).to eq "failed" expect(source.reload.fetched_at).to be_nil end From cd034522f800ac667d5b7137ec34d0aed747ea92 Mon Sep 17 00:00:00 2001 From: 631583 Date: Tue, 5 Dec 2023 12:05:59 -0800 Subject: [PATCH 2/3] remove unused constant --- app/models/manifest_source.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/models/manifest_source.rb b/app/models/manifest_source.rb index 65c9cff5c..6a74f4be3 100644 --- a/app/models/manifest_source.rb +++ b/app/models/manifest_source.rb @@ -17,8 +17,6 @@ class ManifestSource < ApplicationRecord delegate :file_number, to: :manifest - SECONDS_TO_AUTO_UNLOCK = 5 - def start! return if current? || processing? V2::DownloadManifestJob.perform_later(self, RequestStore[:current_user]) From 6f00227e56dfc8e783dd0962f6e440614f36e7ad Mon Sep 17 00:00:00 2001 From: 631583 Date: Tue, 5 Dec 2023 12:12:46 -0800 Subject: [PATCH 3/3] refresh the manifest after 72 hours even if pending --- app/models/manifest.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/models/manifest.rb b/app/models/manifest.rb index 450724212..86f97247c 100644 --- a/app/models/manifest.rb +++ b/app/models/manifest.rb @@ -29,7 +29,7 @@ class Manifest < ApplicationRecord def start! # Reset stale manifests. - update!(fetched_files_status: :initialized) if stale? + update!(fetched_files_status: :initialized) if ready_for_refresh? vbms_source.start! vva_source.start! unless FeatureToggle.enabled?(:skip_vva) @@ -54,9 +54,9 @@ def download_and_package_files! end end - # completed? because it can be recent pending with stale fetched_files_at - def stale? - completed? && fetched_files_at && fetched_files_at < UI_HOURS_UNTIL_EXPIRY.hours.ago + # ready_for_refresh? we want to refresh the manifest after 72 hours regardless of the state + def ready_for_refresh? + !initialized? && fetched_files_at && fetched_files_at < UI_HOURS_UNTIL_EXPIRY.hours.ago end def completed?