From 3e56fdd1d7d2bd6c207c801ee222c3e795980b4d Mon Sep 17 00:00:00 2001 From: Martin Date: Thu, 7 Jul 2022 18:15:48 +0200 Subject: [PATCH] fix(export): when it takes more than 3 hours, exports are purge before being generated. make it possible to have an export that takes more than 3 hours and share this behaviour with kind of same class archive --- .../export_component.html.haml | 2 +- .../administrateurs/exports_controller.rb | 4 +- .../instructeurs/procedures_controller.rb | 4 +- app/jobs/archive_creation_job.rb | 15 +++--- app/jobs/cron/purge_stale_archives_job.rb | 2 +- app/jobs/cron/purge_stale_exports_job.rb | 2 +- app/jobs/export_job.rb | 4 +- app/models/archive.rb | 31 +---------- ...sient_models_with_purgeable_job_concern.rb | 51 +++++++++++++++++++ app/models/export.rb | 9 ++-- app/views/shared/archives/_table.html.haml | 2 +- ...220707152632_cleanup_export_and_archive.rb | 7 +++ db/schema.rb | 3 +- .../exports_controller_spec.rb | 2 +- .../procedures_controller_spec.rb | 2 +- spec/factories/archive.rb | 4 +- spec/jobs/archive_creation_job_spec.rb | 2 +- spec/models/archive_spec.rb | 26 ++++++---- .../procedure_archive_service_spec.rb | 4 +- 19 files changed, 106 insertions(+), 70 deletions(-) create mode 100644 app/models/concerns/transient_models_with_purgeable_job_concern.rb create mode 100644 db/migrate/20220707152632_cleanup_export_and_archive.rb diff --git a/app/components/dossiers/export_component/export_component.html.haml b/app/components/dossiers/export_component/export_component.html.haml index 43960a3c7..1f9985c44 100644 --- a/app/components/dossiers/export_component/export_component.html.haml +++ b/app/components/dossiers/export_component/export_component.html.haml @@ -11,7 +11,7 @@ %li - if export.nil? = link_to t(".everything_#{item[:format]}_html"), download_export_path(export_format: item[:format]), data: { turbo_method: :post } - - elsif export.ready? + - elsif export.available? = link_to ready_link_label(export), export.file.service_url, target: "_blank", rel: "noopener" - if export.old? = button_to download_export_path(export_format: export.format, force_export: true), **refresh_button_options(export) do diff --git a/app/controllers/administrateurs/exports_controller.rb b/app/controllers/administrateurs/exports_controller.rb index 44c336b96..15810e6b6 100644 --- a/app/controllers/administrateurs/exports_controller.rb +++ b/app/controllers/administrateurs/exports_controller.rb @@ -5,12 +5,12 @@ module Administrateurs def download export = Export.find_or_create_export(export_format, all_groupe_instructeurs, **export_options) - if export.ready? && export.old? && force_export? + if export.available? && export.old? && force_export? export.destroy export = Export.find_or_create_export(export_format, all_groupe_instructeurs, **export_options) end - if export.ready? + if export.available? respond_to do |format| format.turbo_stream do @dossiers_count = export.count diff --git a/app/controllers/instructeurs/procedures_controller.rb b/app/controllers/instructeurs/procedures_controller.rb index 185bde472..9daf0c80e 100644 --- a/app/controllers/instructeurs/procedures_controller.rb +++ b/app/controllers/instructeurs/procedures_controller.rb @@ -149,12 +149,12 @@ module Instructeurs export = Export.find_or_create_export(export_format, groupe_instructeurs, **export_options) - if export.ready? && export.old? && force_export? + if export.available? && export.old? && force_export? export.destroy export = Export.find_or_create_export(export_format, groupe_instructeurs, **export_options) end - if export.ready? + if export.available? respond_to do |format| format.turbo_stream do @procedure = procedure diff --git a/app/jobs/archive_creation_job.rb b/app/jobs/archive_creation_job.rb index c3a4d0f6d..8fec78ef6 100644 --- a/app/jobs/archive_creation_job.rb +++ b/app/jobs/archive_creation_job.rb @@ -2,14 +2,11 @@ class ArchiveCreationJob < ApplicationJob queue_as :archives def perform(procedure, archive, administrateur_or_instructeur) - archive.restart! if archive.failed? # restart for AASM - ProcedureArchiveService - .new(procedure) - .make_and_upload_archive(archive) - archive.make_available! - UserMailer.send_archive(administrateur_or_instructeur, procedure, archive).deliver_later - rescue => e - archive.fail! # fail for observability - raise e # re-raise for retryable behaviour + archive.compute_with_safe_stale_for_purge do + ProcedureArchiveService + .new(procedure) + .make_and_upload_archive(archive) + UserMailer.send_archive(administrateur_or_instructeur, procedure, archive).deliver_later + end end end diff --git a/app/jobs/cron/purge_stale_archives_job.rb b/app/jobs/cron/purge_stale_archives_job.rb index 4ce48784a..124c7935e 100644 --- a/app/jobs/cron/purge_stale_archives_job.rb +++ b/app/jobs/cron/purge_stale_archives_job.rb @@ -2,6 +2,6 @@ class Cron::PurgeStaleArchivesJob < Cron::CronJob self.schedule_expression = "every 5 minutes" def perform - Archive.stale.destroy_all + Archive.stale(Archive::RETENTION_DURATION).destroy_all end end diff --git a/app/jobs/cron/purge_stale_exports_job.rb b/app/jobs/cron/purge_stale_exports_job.rb index 5e0fbcf6c..2945e26a6 100644 --- a/app/jobs/cron/purge_stale_exports_job.rb +++ b/app/jobs/cron/purge_stale_exports_job.rb @@ -2,6 +2,6 @@ class Cron::PurgeStaleExportsJob < Cron::CronJob self.schedule_expression = "every 5 minutes" def perform - Export.stale.destroy_all + Export.stale(Export::MAX_DUREE_CONSERVATION_EXPORT).destroy_all end end diff --git a/app/jobs/export_job.rb b/app/jobs/export_job.rb index a1ee18f14..bb34695fb 100644 --- a/app/jobs/export_job.rb +++ b/app/jobs/export_job.rb @@ -4,6 +4,8 @@ class ExportJob < ApplicationJob discard_on ActiveRecord::RecordNotFound def perform(export) - export.compute + export.compute_with_safe_stale_for_purge do + export.compute + end end end diff --git a/app/models/archive.rb b/app/models/archive.rb index 7ea337a1a..d4081d8f4 100644 --- a/app/models/archive.rb +++ b/app/models/archive.rb @@ -3,15 +3,15 @@ # Table name: archives # # id :bigint not null, primary key +# job_status :string not null # key :text not null # month :date -# status :string not null # time_span_type :string not null # created_at :datetime not null # updated_at :datetime not null # class Archive < ApplicationRecord - include AASM + include TransientModelsWithPurgeableJobConcern RETENTION_DURATION = 4.days MAX_SIZE = 100.gigabytes @@ -20,7 +20,6 @@ class Archive < ApplicationRecord has_one_attached :file - scope :stale, -> { where('updated_at < ?', (Time.zone.now - RETENTION_DURATION)) } scope :for_groupe_instructeur, -> (groupe_instructeur) { joins(:archives_groupe_instructeurs) .where( @@ -33,32 +32,6 @@ class Archive < ApplicationRecord monthly: 'monthly' } - enum status: { - pending: 'pending', - generated: 'generated', - failed: 'failed' - } - - aasm whiny_persistence: true, column: :status, enum: true do - state :pending, initial: true - state :generated - state :failed - - event :make_available do - transitions from: :pending, to: :generated - end - event :restart do - transitions from: :failed, to: :pending - end - event :fail do - transitions from: :pending, to: :failed - end - end - - def available? - status == 'generated' && file.attached? - end - def filename(procedure) if time_span_type == 'everything' "procedure-#{procedure.id}.zip" diff --git a/app/models/concerns/transient_models_with_purgeable_job_concern.rb b/app/models/concerns/transient_models_with_purgeable_job_concern.rb new file mode 100644 index 000000000..8edc63a80 --- /dev/null +++ b/app/models/concerns/transient_models_with_purgeable_job_concern.rb @@ -0,0 +1,51 @@ +# Archive and Export models are generated in background +# those models being are destroy after an expiration period +# but, it might take more time to process than the expiration period +# this module expose the shared behaviour to compute a job and purge instances +# based on a state machine +module TransientModelsWithPurgeableJobConcern + extend ActiveSupport::Concern + included do + include AASM + + enum job_status: { + pending: 'pending', + generated: 'generated', + failed: 'failed' + } + + aasm whiny_persistence: true, column: :job_status, enum: true do + state :pending, initial: true + state :generated + state :failed + + event :make_available do + transitions from: :pending, to: :generated + end + event :restart do + transitions from: :failed, to: :pending + end + event :fail do + transitions from: :pending, to: :failed + end + end + + scope :stale, lambda { |duration| + where(job_status: [job_statuses.fetch(:generated), job_statuses.fetch(:failed)]) + .where('updated_at < ?', (Time.zone.now - duration)) + } + + def available? + generated? + end + + def compute_with_safe_stale_for_purge(&block) + restart! if failed? # restart for AASM + yield + make_available! + rescue => e + fail! # fail for observability + raise e # re-raise for retryable behaviour + end + end +end diff --git a/app/models/export.rb b/app/models/export.rb index 3e444243c..be80e1647 100644 --- a/app/models/export.rb +++ b/app/models/export.rb @@ -4,6 +4,7 @@ # # id :bigint not null, primary key # format :string not null +# job_status :string # key :text not null # procedure_presentation_snapshot :jsonb # statut :string default("tous") @@ -13,7 +14,9 @@ # procedure_presentation_id :bigint # class Export < ApplicationRecord - MAX_DUREE_CONSERVATION_EXPORT = 3.hours + include TransientModelsWithPurgeableJobConcern + + MAX_DUREE_CONSERVATION_EXPORT = 16.hours enum format: { csv: 'csv', @@ -69,10 +72,6 @@ class Export < ApplicationRecord time_span_type == Export.time_span_types.fetch(:monthly) ? 30.days.ago : nil end - def ready? - file.attached? - end - def old? updated_at < 20.minutes.ago || filters_changed? end diff --git a/app/views/shared/archives/_table.html.haml b/app/views/shared/archives/_table.html.haml index 069cb661a..d27fee6a2 100644 --- a/app/views/shared/archives/_table.html.haml +++ b/app/views/shared/archives/_table.html.haml @@ -22,7 +22,7 @@ = number_to_human_size(weight) %td.center - if matching_archive.present? - - if matching_archive.status == 'generated' && matching_archive.file.attached? + - if matching_archive.available? = link_to url_for(matching_archive.file), class: 'button primary' do %span.icon.download-white = t(:archive_ready_html, scope: [:instructeurs, :procedure], generated_period: time_ago_in_words(matching_archive.updated_at)) diff --git a/db/migrate/20220707152632_cleanup_export_and_archive.rb b/db/migrate/20220707152632_cleanup_export_and_archive.rb new file mode 100644 index 000000000..f28464e46 --- /dev/null +++ b/db/migrate/20220707152632_cleanup_export_and_archive.rb @@ -0,0 +1,7 @@ +class CleanupExportAndArchive < ActiveRecord::Migration[6.1] + def change + safety_assured do + rename_column :archives, :status, :job_status + end + end +end diff --git a/db/schema.rb b/db/schema.rb index eeca800f2..54487b249 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -88,9 +88,9 @@ ActiveRecord::Schema.define(version: 2022_07_08_152039) do create_table "archives", force: :cascade do |t| t.datetime "created_at", precision: 6, null: false + t.string "job_status", null: false t.text "key", null: false t.date "month" - t.string "status", null: false t.string "time_span_type", null: false t.datetime "updated_at", precision: 6, null: false t.index ["key", "time_span_type", "month"], name: "index_archives_on_key_and_time_span_type_and_month", unique: true @@ -430,6 +430,7 @@ ActiveRecord::Schema.define(version: 2022_07_08_152039) do create_table "exports", force: :cascade do |t| t.datetime "created_at", null: false t.string "format", null: false + t.string "job_status" t.text "key", null: false t.bigint "procedure_presentation_id" t.jsonb "procedure_presentation_snapshot" diff --git a/spec/controllers/administrateurs/exports_controller_spec.rb b/spec/controllers/administrateurs/exports_controller_spec.rb index 80e9a4322..af8f4e5b2 100644 --- a/spec/controllers/administrateurs/exports_controller_spec.rb +++ b/spec/controllers/administrateurs/exports_controller_spec.rb @@ -38,7 +38,7 @@ describe Administrateurs::ExportsController, type: :controller do end context 'when the export is ready' do - let(:export) { create(:export, groupe_instructeurs: procedure.groupe_instructeurs) } + let(:export) { create(:export, job_status: :generated, groupe_instructeurs: procedure.groupe_instructeurs) } before do export.file.attach(io: StringIO.new('export'), filename: 'file.csv') diff --git a/spec/controllers/instructeurs/procedures_controller_spec.rb b/spec/controllers/instructeurs/procedures_controller_spec.rb index dc1c31eaa..ffd65d8e5 100644 --- a/spec/controllers/instructeurs/procedures_controller_spec.rb +++ b/spec/controllers/instructeurs/procedures_controller_spec.rb @@ -498,7 +498,7 @@ describe Instructeurs::ProceduresController, type: :controller do end context 'when the export is ready' do - let(:export) { create(:export, groupe_instructeurs: [gi_1]) } + let(:export) { create(:export, groupe_instructeurs: [gi_1], job_status: 'generated') } before do export.file.attach(io: StringIO.new('export'), filename: 'file.csv') diff --git a/spec/factories/archive.rb b/spec/factories/archive.rb index 41fbc2542..35529acbd 100644 --- a/spec/factories/archive.rb +++ b/spec/factories/archive.rb @@ -5,11 +5,11 @@ FactoryBot.define do key { 'unique-key' } trait :pending do - status { 'pending' } + job_status { 'pending' } end trait :generated do - status { 'generated' } + job_status { 'generated' } end end end diff --git a/spec/jobs/archive_creation_job_spec.rb b/spec/jobs/archive_creation_job_spec.rb index 073dbfebd..f21f985b6 100644 --- a/spec/jobs/archive_creation_job_spec.rb +++ b/spec/jobs/archive_creation_job_spec.rb @@ -1,6 +1,6 @@ describe ArchiveCreationJob, type: :job do describe 'perform' do - let(:archive) { create(:archive, status: status, groupe_instructeurs: [procedure.groupe_instructeurs.first]) } + let(:archive) { create(:archive, job_status: status, groupe_instructeurs: [procedure.groupe_instructeurs.first]) } let(:instructeur) { create(:instructeur) } let(:procedure) { create(:procedure, instructeurs: [instructeur]) } let(:job) { ArchiveCreationJob.new(procedure, archive, instructeur) } diff --git a/spec/models/archive_spec.rb b/spec/models/archive_spec.rb index 5753340d3..c69ecdc5b 100644 --- a/spec/models/archive_spec.rb +++ b/spec/models/archive_spec.rb @@ -1,32 +1,38 @@ -describe Dossier do +describe Archive do include ActiveJob::TestHelper before { Timecop.freeze(Time.zone.now) } after { Timecop.return } - let(:archive) { create(:archive) } + let(:archive) { create(:archive, job_status: :pending) } describe 'scopes' do describe 'staled' do - let(:recent_archive) { create(:archive) } - let(:staled_archive) { create(:archive, updated_at: (Archive::RETENTION_DURATION + 2).days.ago) } + let(:recent_archive) { create(:archive, job_status: :pending) } + let(:staled_archive_still_pending) { create(:archive, job_status: :pending, updated_at: (Archive::RETENTION_DURATION + 2).days.ago) } + let(:staled_archive_still_failed) { create(:archive, job_status: :failed, updated_at: (Archive::RETENTION_DURATION + 2).days.ago) } + let(:staled_archive_still_generated) { create(:archive, job_status: :generated, updated_at: (Archive::RETENTION_DURATION + 2).days.ago) } subject do - archive; recent_archive; staled_archive - Archive.stale + archive + recent_archive + staled_archive_still_pending + staled_archive_still_failed + staled_archive_still_generated + Archive.stale(Archive::RETENTION_DURATION) end - it { is_expected.to match_array([staled_archive]) } + it { is_expected.to match_array([staled_archive_still_failed, staled_archive_still_generated]) } end end - describe '.status' do - it { expect(archive.status).to eq('pending') } + describe '.job_status' do + it { expect(archive.job_status).to eq('pending') } end describe '#make_available!' do before { archive.make_available! } - it { expect(archive.status).to eq('generated') } + it { expect(archive.job_status).to eq('generated') } end describe '#available?' do diff --git a/spec/services/procedure_archive_service_spec.rb b/spec/services/procedure_archive_service_spec.rb index d7b79d816..60ce15e39 100644 --- a/spec/services/procedure_archive_service_spec.rb +++ b/spec/services/procedure_archive_service_spec.rb @@ -18,7 +18,7 @@ describe ProcedureArchiveService do after { Timecop.return } context 'for a specific month' do - let(:archive) { create(:archive, time_span_type: 'monthly', status: 'pending', month: date_month, groupe_instructeurs: groupe_instructeurs) } + let(:archive) { create(:archive, time_span_type: 'monthly', job_status: 'pending', month: date_month, groupe_instructeurs: groupe_instructeurs) } let(:year) { 2021 } it 'collects files with success' do @@ -120,7 +120,7 @@ describe ProcedureArchiveService do end context 'for all months' do - let(:archive) { create(:archive, time_span_type: 'everything', status: 'pending', groupe_instructeurs: groupe_instructeurs) } + let(:archive) { create(:archive, time_span_type: 'everything', job_status: 'pending', groupe_instructeurs: groupe_instructeurs) } it 'collect files' do allow_any_instance_of(ActiveStorage::Attachment).to receive(:url).and_return("https://opengraph.githubassets.com/5e61989aecb78e369c93674f877d7bf4ecde378850114a9563cdf8b6a2472536/typhoeus/typhoeus/issues/110")