From 46120b6cd0835ab0001906c4bc398cc91d96526d Mon Sep 17 00:00:00 2001 From: Christophe Bliard Date: Tue, 1 Oct 2024 14:58:17 +0200 Subject: [PATCH 1/3] Make our `MailerJob` match Rails' `MailDeliveryJob` We want `MailerJob` to behave like both our `ApplicationJob` (for reloading the mailer configuration and resetting the request store automatically) and `ActionMailer::MailDeliveryJob` (for the basic functionalities of sending emails). As we can only inherit from one of these two classes, we inherit from `ApplicationJob` and copy the relevant parts from `ActionMailer::MailDeliveryJob`. This copy was done some time ago, and the `ActionMailer::MailDeliveryJob` class has evolved since then. This commit updates our copy to match the current `ActionMailer::MailDeliveryJob` class. --- app/workers/mails/mailer_job.rb | 37 ++++++++++++++++++++++----------- 1 file changed, 25 insertions(+), 12 deletions(-) diff --git a/app/workers/mails/mailer_job.rb b/app/workers/mails/mailer_job.rb index beaed7c77fae..c5c2dfe24381 100644 --- a/app/workers/mails/mailer_job.rb +++ b/app/workers/mails/mailer_job.rb @@ -26,29 +26,42 @@ # See COPYRIGHT and LICENSE files for more details. #++ -## -# This job gets called when internally using +# OpenProject is configured to use this job when sending emails like this: # # ``` # UserMailer.some_mail("some param").deliver_later # ``` # -# because we want to have the sending of the email run in an `ApplicationJob` -# as opposed to using `ActiveJob::QueueAdapters::DelayedJobAdapter::JobWrapper`. -# We want it to run in an `ApplicationJob` because of the shared setup required -# such as reloading the mailer configuration and resetting the request store. +# This job is used because all our `XxxMailer` classes inherit from +# `ApplicationMailer`, and `ApplicationMailer.delivery_job` is set to +# `::Mails::MailerJob`. +# +# The `delivery_job` is customized because we want to have the sending of the +# email run in an instance inheriting from `ApplicationJob` instead of a +# `ActionMailer::MailDeliveryJob` (Rails default delivery job). Indeed +# `ApplicationJob` contains all the shared setup required for OpenProject such +# as reloading the mailer configuration and resetting the request store. class Mails::MailerJob < ApplicationJob - queue_as { ActionMailer::Base.deliver_later_queue_name } - # Retry mailing jobs three times with polinomial backoff retry_on StandardError, wait: :polynomially_longer, attempts: 3 - # If exception is handled in mail handler - # retry_on will be ignored + # The following lines are copied from ActionMailer::MailDeliveryJob + queue_as do + mailer_class = arguments.first.constantize + mailer_class.deliver_later_queue_name + end + rescue_from StandardError, with: :handle_exception_with_mailer_class - def perform(mailer, mail_method, delivery, args:) - mailer.constantize.public_send(mail_method, *args).send(delivery) + def perform(mailer, mail_method, delivery_method, args:, kwargs: nil, params: nil) + mailer_class = params ? mailer.constantize.with(params) : mailer.constantize + message = + if kwargs + mailer_class.public_send(mail_method, *args, **kwargs) + else + mailer_class.public_send(mail_method, *args) + end + message.send(delivery_method) end private From 49a74827ffdd26bd7f8d75c00862ecd2231704a4 Mon Sep 17 00:00:00 2001 From: Christophe Bliard Date: Wed, 2 Oct 2024 12:17:18 +0200 Subject: [PATCH 2/3] [57932] Keep retrying mailer jobs for 1.5 days https://community.openproject.org/wp/57932 Retrying 14 times with polynomial backoff means it will retry for roughly 1.5 days. Previously the `retry_on StandardError` defined in the `Mails::MailerJob` class was ignored because there is also a `rescue_from StandardError` declared after it. As `retry_on` is implemented using `rescue_from`, and the handlers are evaluated in reverse order, the last declared `rescue_from` would be picked up and the retry logic would not be triggered. That's why mail jobs were discarded instead of being retried. This commit fixes the issue by inheriting directly from `ActionMailer::MailDeliveryJob` instead of redefining its methods. This was the `rescue_from` is declared first in the parent class, and then `retry_on` is declared second in the child class, meaning it will take precedence and be picked up. The needed shared logic from `ApplicationJob` is extracted to a new `SharedJobSetup` module and included in both `ApplicationJob` and `Mails::MailerJob`. --- app/workers/application_job.rb | 46 +---------------- app/workers/mails/mailer_job.rb | 73 +++++++++++--------------- app/workers/shared_job_setup.rb | 92 +++++++++++++++++++++++++++++++++ 3 files changed, 122 insertions(+), 89 deletions(-) create mode 100644 app/workers/shared_job_setup.rb diff --git a/app/workers/application_job.rb b/app/workers/application_job.rb index 44c81ab93723..c2197a5f15bd 100644 --- a/app/workers/application_job.rb +++ b/app/workers/application_job.rb @@ -30,13 +30,7 @@ class ApplicationJob < ActiveJob::Base include ::JobStatus::ApplicationJobWithStatus - - ## - # By default, do not log the arguments of a background job - # to avoid leaking sensitive information to logs - self.log_arguments = false - - around_perform :prepare_job_context + include SharedJobSetup ## # Return a priority number on the given payload @@ -65,45 +59,7 @@ def self.queue_with_priority(value = :default) end end - # Resets the thread local request store. - # This should be done, because normal application code expects the RequestStore to be - # invalidated between multiple requests and does usually not care whether it is executed - # from a request or from a delayed job. - # For a delayed job, each job execution is the thing that comes closest to - # the concept of a new request. - def with_clean_request_store - store = RequestStore.store - - begin - RequestStore.clear! - yield - ensure - # Reset to previous value - RequestStore.clear! - RequestStore.store.merge! store - end - end - - # Reloads the thread local ActionMailer configuration. - # Since the email configuration is now done in the web app, we need to - # make sure that any changes to the configuration is correctly picked up - # by the background jobs at runtime. - def reload_mailer_settings! - Setting.reload_mailer_settings! - end - def job_scheduled_at GoodJob::Job.where(id: job_id).pick(:scheduled_at) end - - private - - def prepare_job_context - with_clean_request_store do - ::OpenProject::Appsignal.tag_request - reload_mailer_settings! - - yield - end - end end diff --git a/app/workers/mails/mailer_job.rb b/app/workers/mails/mailer_job.rb index c5c2dfe24381..696daa79ec4a 100644 --- a/app/workers/mails/mailer_job.rb +++ b/app/workers/mails/mailer_job.rb @@ -36,49 +36,34 @@ # `ApplicationMailer`, and `ApplicationMailer.delivery_job` is set to # `::Mails::MailerJob`. # -# The `delivery_job` is customized because we want to have the sending of the -# email run in an instance inheriting from `ApplicationJob` instead of a -# `ActionMailer::MailDeliveryJob` (Rails default delivery job). Indeed -# `ApplicationJob` contains all the shared setup required for OpenProject such -# as reloading the mailer configuration and resetting the request store. -class Mails::MailerJob < ApplicationJob - # Retry mailing jobs three times with polinomial backoff - retry_on StandardError, wait: :polynomially_longer, attempts: 3 - - # The following lines are copied from ActionMailer::MailDeliveryJob - queue_as do - mailer_class = arguments.first.constantize - mailer_class.deliver_later_queue_name - end - - rescue_from StandardError, with: :handle_exception_with_mailer_class - - def perform(mailer, mail_method, delivery_method, args:, kwargs: nil, params: nil) - mailer_class = params ? mailer.constantize.with(params) : mailer.constantize - message = - if kwargs - mailer_class.public_send(mail_method, *args, **kwargs) - else - mailer_class.public_send(mail_method, *args) - end - message.send(delivery_method) - end - - private - - # "Deserialize" the mailer class name by hand in case another argument - # (like a Global ID reference) raised DeserializationError. - def mailer_class - if mailer = Array(@serialized_arguments).first || Array(arguments).first - mailer.constantize - end - end +# The `delivery_job` is customized to add the shared job setup required for +# OpenProject such as reloading the mailer configuration and resetting the +# request store on each job execution. +# +# It also adds retry logic to the job. +class Mails::MailerJob < ActionMailer::MailDeliveryJob + include SharedJobSetup - def handle_exception_with_mailer_class(exception) - if klass = mailer_class - klass.handle_exception exception - else - raise exception - end - end + # Retry mailing jobs 14 times with polynomial backoff (retries for ~ 1.5 days). + # + # with polynomial backoff, the formula to get wait_duration is: + # + # ((executions**4) + (Kernel.rand * (executions**4) * jitter)) + 2 + # + # as the default jitter is 0.0, the formula becomes: + # + # ((executions**4) + 2) + # + # To get the numbers, run this: + # + # (1..20).reduce(0) do |total_wait, i| + # wait = (i**4) + 2 + # total_wait += wait + # puts "Execution #{i} waits #{wait} secs. Total wait: #{total_wait} secs" + # total_wait + # end + # + # We set attemps to 14 to have it retry for 127715 seconds which is more than + # 1 day (~= 1 day 11 hours 30 min) + retry_on StandardError, wait: :polynomially_longer, attempts: 14 end diff --git a/app/workers/shared_job_setup.rb b/app/workers/shared_job_setup.rb new file mode 100644 index 000000000000..21019b136b1d --- /dev/null +++ b/app/workers/shared_job_setup.rb @@ -0,0 +1,92 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +# Shared setup for jobs. +# +# This module is included in `ApplicationJob` and `Mails::MailerJob` and does +# the following: +# +# - disable logging of arguments +# - before each job execution: +# - reloads the mailer settings +# - resets the request store +# - tags the request for AppSignal +module SharedJobSetup + extend ActiveSupport::Concern + + included do + # By default, do not log the arguments of a background job + # to avoid leaking sensitive information to logs + self.log_arguments = false + + around_perform :prepare_job_context + end + + # Prepare the job execution by cleaning the request store, reloading the + # mailer settings and tagging the request + def prepare_job_context + with_clean_request_store do + ::OpenProject::Appsignal.tag_request + reload_mailer_settings! + + yield + end + end + + # Resets the thread local request store. + # + # This should be done, because normal application code expects the + # RequestStore to be invalidated between multiple requests and does usually + # not care whether it is executed from a request or from a job. + # + # For a job, each job execution is the thing that comes closest to the concept + # of a new request. + def with_clean_request_store + store = RequestStore.store + + begin + RequestStore.clear! + yield + ensure + # Reset to previous value + RequestStore.clear! + RequestStore.store.merge! store + end + end + + # Reloads the thread local ActionMailer configuration. + # + # Since the email configuration is done in the web app, it makes sure that any + # changes to the configuration is correctly picked up by the background jobs + # at runtime. + def reload_mailer_settings! + Setting.reload_mailer_settings! + end +end From 8567cedb0cc40c75a77af3f88188d2d8494b41c3 Mon Sep 17 00:00:00 2001 From: Christophe Bliard Date: Wed, 2 Oct 2024 16:02:27 +0200 Subject: [PATCH 3/3] [57932] Add spec for resending email logic --- spec/workers/mails/mailer_job_spec.rb | 101 ++++++++++++++++++++++++++ 1 file changed, 101 insertions(+) create mode 100644 spec/workers/mails/mailer_job_spec.rb diff --git a/spec/workers/mails/mailer_job_spec.rb b/spec/workers/mails/mailer_job_spec.rb new file mode 100644 index 000000000000..d8697d8376e5 --- /dev/null +++ b/spec/workers/mails/mailer_job_spec.rb @@ -0,0 +1,101 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +require "spec_helper" + +class MyRSpecExampleMailer < ApplicationMailer + default from: "openproject@example.com", + subject: "Welcome to OpenProject" + + def welcome_email + user = params[:user] + mail(to: user) do |format| + format.text { render plain: "Welcome!" } + format.html { render html: "

Welcome!

".html_safe } + end + end + + def welcome2(user) + mail(to: user) do |format| + format.text { render plain: "Welcome!" } + format.html { render html: "

Welcome!

".html_safe } + end + end +end + +RSpec.describe Mails::MailerJob do + subject { described_class.new } + + it "is used to send emails when calling .deliver_later on a mailer" do + user = create(:user, mail: "user@example.com") + job = MyRSpecExampleMailer.with(user:).welcome_email.deliver_later + expect(job).to be_an_instance_of(described_class) + expect(enqueued_jobs).to contain_exactly(a_hash_including("job_class" => described_class.name)) + enqueued_job = enqueued_jobs.first + + perform_enqueued_jobs + # job has been performed + expect(performed_jobs).to contain_exactly(enqueued_job) + # there are no more jobs + expect(enqueued_jobs).to be_empty + end + + it "retries sending email on StandardError" do + user = "Will raise ArgumentError because ApplicationMailer expect a User instance as recipient" + MyRSpecExampleMailer.with(user:).welcome_email.deliver_later + expect(enqueued_jobs).to contain_exactly(a_hash_including("job_class" => "Mails::MailerJob", + "executions" => 0, + "exception_executions" => {})) + + # let's execute the mailer job + job1 = enqueued_jobs.first + perform_enqueued_jobs + + # job has been performed, but has encountered an error + expect(performed_jobs).to contain_exactly(job1) + expect(job1).to include("exception_executions" => { "[StandardError]" => 1 }) + + # and it is being retried: another identical job is queued with an increased execution count + expect(enqueued_jobs).to contain_exactly(a_hash_including("job_class" => "Mails::MailerJob", + "executions" => 1, + "exception_executions" => { "[StandardError]" => 1 })) + + # we can run this retried job, it will be performed, fail again, and enqueue another retry job again + job2 = enqueued_jobs.first + perform_enqueued_jobs + expect(performed_jobs).to contain_exactly(job1, job2) + expect(job2).to include("exception_executions" => { "[StandardError]" => 2 }) + expect(enqueued_jobs).to contain_exactly(a_hash_including("job_class" => "Mails::MailerJob", + "executions" => 2, + "exception_executions" => { "[StandardError]" => 2 })) + + # and so on... + end +end