Merge pull request #15602 from opf/fix/limit_schedule_reminder_mail_jobs

Fix/limit schedule reminder mail jobs

app/models/users/scopes/having_reminder_mail_to_send.rb

@@ -31,13 +31,13 @@ module HavingReminderMailToSend
     extend ActiveSupport::Concern
 
     class_methods do
-      # Returns all users for which a reminder mails should be sent now. A user
+      # Returns all users for which a reminder mails should be sent. A user
       # will be included if:
       #
-      # * That user has an unread notification
+      # * That user has an unread notification that is not older than the latest_time value.
       # * The user hasn't been informed about the unread notification before
       # * The user has configured reminder mails to be within the time frame
-      #   between the provided time and now.
+      #   between the provided earliest_time and latest_time.
       #
       # This assumes that users only have full hours specified for the times
       # they desire to receive a reminder mail at.
@@ -48,9 +48,14 @@ module HavingReminderMailToSend
       #   Only the time part is used which is moved forward to the next quarter
       #   hour (e.g. 2021-05-03 10:34:12+02:00 -> 08:45:00). This is done
       #   because time zones always have a mod(15) == 0 minutes offset. Needs to
-      #   be before the current time.
-      def having_reminder_mail_to_send(earliest_time)
-        local_times = local_times_from(earliest_time)
+      #   be before the latest_time.
+      # @param [DateTime] latest_time The latest time to consider as a matching
+      #  slot.
+      #
+      #  Only the time part is used which is moved back to the last quarter hour
+      #  less than the latest_time value.
+      def having_reminder_mail_to_send(earliest_time, latest_time)
+        local_times = local_times_from(earliest_time, latest_time)
 
         return none if local_times.empty?
 
@@ -62,13 +67,15 @@ def having_reminder_mail_to_send(earliest_time)
                                  .joins(local_time_join(local_times))
 
         subscriber_ids = Notification
-                           .unsent_reminders_before(recipient: recipient_candidates, time: Time.current)
+                           .unsent_reminders_before(recipient: recipient_candidates, time: latest_time)
                            .group(:recipient_id)
                            .select(:recipient_id)
 
         where(id: subscriber_ids)
       end
 
+      private
+
       def local_time_join(local_times)
         # Joins the times local to the user preferences and then checks whether:
         #
@@ -129,8 +136,8 @@ def local_time_join(local_times)
         SQL
       end
 
-      def local_times_from(earliest_time)
-        times = quarters_between_earliest_and_now(earliest_time)
+      def local_times_from(earliest_time, latest_time)
+        times = quarters_between_earliest_and_latest(earliest_time, latest_time)
 
         times_for_zones(times)
       end
@@ -167,20 +174,20 @@ def build_local_times(times, zone)
         end
       end
 
-      def quarters_between_earliest_and_now(earliest_time)
-        latest_time = Time.current
+      def quarters_between_earliest_and_latest(earliest_time, latest_time) # rubocop:disable Metrics/AbcSize
         raise ArgumentError if latest_time < earliest_time || (latest_time - earliest_time) > 1.day
 
-        quarters = ((latest_time - earliest_time) / 60 / 15).floor
+        # The first quarter is equal or greater to the earliest time
+        first_quarter = earliest_time.change(min: (earliest_time.min.to_f / 15).ceil * 15)
+        # The last quarter is the one smaller than the latest time. But needs to be at least equal to the first quarter.
+        last_quarter = [first_quarter, latest_time.change(min: latest_time.min / 15 * 15)].max
 
-        (1..quarters).each_with_object([next_quarter_hour(earliest_time)]) do |_, times|
-          times << (times.last + 15.minutes)
+        (first_quarter.to_i..last_quarter.to_i)
+          .step(15.minutes)
+          .map do |time|
+          Time.zone.at(time)
         end
       end
-
-      def next_quarter_hour(time)
-        (time + (time.min % 15 == 0 ? 0.minutes : (15 - (time.min % 15)).minutes))
-      end
     end
   end
 end

app/workers/cron/quarter_hour_schedule_job.rb

@@ -0,0 +1,75 @@
+# -- copyright
+# OpenProject is an open source project management software.
+# Copyright (C) 2024 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.
+# ++
+
+module Cron::QuarterHourScheduleJob
+  extend ActiveSupport::Concern
+
+  included do
+    include GoodJob::ActiveJobExtensions::Concurrency
+
+    # With good_job and the limit of only allowing a single job to be enqueued and
+    # also a single job being performed at the same time we end up having
+    # up to two jobs that are not yet finished.
+
+    good_job_control_concurrency_with(
+      enqueue_limit: 1,
+      perform_limit: 1
+    )
+
+    # The job is scheduled to run every 15 minutes. If the job before takes longer
+    # than expected we retry two more times (at cron_at + 5 and cron_at + 15). Then the job is discarded.
+    # Once the job is discarded, the next job will be scheduled to run at the next quarter hour.
+    retry_on GoodJob::ActiveJobExtensions::Concurrency::ConcurrencyExceededError,
+             wait: 5.minutes,
+             attempts: 3
+  end
+
+  private
+
+  def upper_boundary
+    @upper_boundary ||= GoodJob::Job
+                          .find(job_id)
+                          .cron_at
+  end
+
+  def lower_boundary
+    @lower_boundary ||= begin
+      predecessor = GoodJob::Job
+                      .succeeded
+                      .where(job_class: self.class.name)
+                      .order(cron_at: :desc)
+                      .first
+
+      if predecessor
+        predecessor.cron_at + 15.minutes
+      else
+        upper_boundary
+      end
+    end
+  end
+end

app/workers/notifications/schedule_date_alerts_notifications_job.rb

@@ -30,24 +30,15 @@ module Notifications
   # Creates date alert jobs for users whose local time is 1:00 am.
   # The job is scheduled to run every 15 minutes.
   class ScheduleDateAlertsNotificationsJob < ApplicationJob
-    include GoodJob::ActiveJobExtensions::Concurrency
+    include Cron::QuarterHourScheduleJob
 
     def perform
       return unless EnterpriseToken.allows_to?(:date_alerts)
 
       Service.new(every_quater_hour_between_predecessor_cron_at_and_own_cron_at).call
     end
 
-    # With good_job and the limit of only allowing a single job to be enqueued and
-    # also a single job being performed at the same time we end up having
-    # up to two jobs that are not yet finished.
-
-    good_job_control_concurrency_with(
-      enqueue_limit: 1,
-      perform_limit: 1
-    )
-
-    # What cannot be controlled however is the time at which a job is actually performed.
+    # What cannot be controlled is the time at which a job is actually performed.
     # A high load on the system can lead to a job being performed later than expected.
     #
     # It might also happen that due to some outage of the background workers, cron
@@ -86,25 +77,5 @@ def every_quater_hour_between_predecessor_cron_at_and_own_cron_at
         Time.zone.at(time)
       end
     end
-
-    def upper_boundary
-      GoodJob::Job
-        .find(job_id)
-        .cron_at
-    end
-
-    def lower_boundary
-      predecessor = GoodJob::Job
-                    .succeeded
-                    .where(job_class: self.class.name)
-                    .order(cron_at: :desc)
-                    .first
-
-      if predecessor
-        predecessor.cron_at + 15.minutes
-      else
-        upper_boundary
-      end
-    end
   end
 end

app/workers/notifications/schedule_reminder_mails_job.rb

@@ -28,8 +28,12 @@
 
 module Notifications
   class ScheduleReminderMailsJob < ApplicationJob
+    include Cron::QuarterHourScheduleJob
+
     def perform
-      User.having_reminder_mail_to_send(job_scheduled_at)
+      return unless lower_boundary.present? && upper_boundary.present?
+
+      User.having_reminder_mail_to_send(lower_boundary, upper_boundary)
           .pluck(:id)
           .each do |user_id|
         Mails::ReminderJob.perform_later(user_id)

spec/features/notifications/reminder_mail_spec.rb

@@ -1,25 +1,34 @@
 require "spec_helper"
 require_relative "../users/notifications/shared_examples"
 
-RSpec.describe "Reminder email sending", :js, :with_cuprite do
+RSpec.describe "Reminder email sending", js: false do
   let!(:project) { create(:project, members: { current_user => role }) }
   let!(:mute_project) { create(:project, members: { current_user => role }) }
   let(:role) { create(:project_role, permissions: %i[view_work_packages]) }
   let(:other_user) { create(:user) }
   let(:work_package) { create(:work_package, project:) }
   let(:watched_work_package) { create(:work_package, project:, watcher_users: [current_user]) }
   let(:involved_work_package) { create(:work_package, project:, assigned_to: current_user) }
-  # ApplicationJob#job_scheduled_at is used for scheduling the reminder mails
-  # needs to be within a time frame eligible for sending out mails for the chose
-  # time zone. For the time zone Hawaii (UTC-10) this means between 8:00:00 and 8:14:59 UTC.
-  # The job is scheduled to run every 15 min so the scheduled_at will in production always move between the quarters of an hour.
-  # The current time can be way behind that.
-  let(:current_utc_time) { ActiveSupport::TimeZone["Pacific/Honolulu"].parse("2021-09-30T08:34:10").utc }
+  # GoodJob::Job#cron_at is used for scheduling the reminder mails.
+  # It needs to be within a time frame eligible for sending out mails for the chosen
+  # time zone. For the time zone Hawaii (UTC-10) this means 8:00:00 as the job has a cron tab to be run every 15 min.
   let(:job_run_at) { ActiveSupport::TimeZone["Pacific/Honolulu"].parse("2021-09-30T08:00:00").utc }
 
-  # Fix the time of the specs to ensure a consistent run
+  let(:scheduled_job) do
+    Notifications::ScheduleReminderMailsJob.perform_later.tap do |job|
+      GoodJob::Job
+        .where(id: job.job_id)
+        .update_all(cron_at: job_run_at)
+    end
+  end
+
+  # The reminder mail is sent out after notifications have been created which might have happened way earlier.
+  # This spec will be fixed to this time ensure a consistent run and to mimic the time that typically has passed
+  # between the changes to a work package and the reminder mail being sent out.
+  let(:work_package_update_time) { ActiveSupport::TimeZone["Pacific/Honolulu"].parse("2021-09-30T01:50:34").utc }
+
   around do |example|
-    Timecop.travel(current_utc_time) do
+    Timecop.travel(work_package_update_time) do
       example.run
     end
   end
@@ -31,7 +40,7 @@
         time_zone: "Pacific/Honolulu",
         daily_reminders: {
           enabled: true,
-          times: [hitting_reminder_slot_for("Pacific/Honolulu", current_utc_time)]
+          times: [hitting_reminder_slot_for("Pacific/Honolulu", job_run_at)]
         },
         immediate_reminders: {
           mentioned: false
@@ -59,8 +68,7 @@
 
     ActiveJob::Base.disable_test_adapter
 
-    scheduled_job = Notifications::ScheduleReminderMailsJob.perform_later
-    GoodJob::Job.where(id: scheduled_job.job_id).update_all(scheduled_at: job_run_at)
+    scheduled_job
   end
 
   it "sends a digest mail based on the configuration", with_settings: { journal_aggregation_time_minutes: 0 } do