Merge pull request #15573 from opf/fix/limit_schedule_date_alert_jobs

Fix/limit schedule date alert jobs

app/workers/notifications/schedule_date_alerts_notifications_job.rb

@@ -28,32 +28,83 @@
 
 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
+
     def perform
       return unless EnterpriseToken.allows_to?(:date_alerts)
 
-      Service.new(times_from_scheduled_to_execution).call
+      Service.new(every_quater_hour_between_predecessor_cron_at_and_own_cron_at).call
     end
 
-    # Returns times from scheduled execution time to current time in 15 minutes
-    # steps.
+    # 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.
+    # 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
+    # jobs are not enqueued as they should be.
+    #
+    # But we want to achieve even under these circumstances that date alerts are sent out
+    # for all the users even if we cannot guarantee that they are sent out at the time where we want it to happen
+    # which would be 1:00 am. At the same time we want to prevent date alerts being sent out multiple times.
     #
-    # As scheduled execution time can be different from current time by more
-    # than 15 minutes when workers are busy, all times at 15 minutes interval
-    # between scheduled time and current time need to be considered to match
-    # with 1:00am in a time zone.
-    def times_from_scheduled_to_execution
-      time = scheduled_time
-      times = []
-      begin
-        times << time
-        time += 15.minutes
-      end while time < Time.current
-      times
+    # There are three scenarios to consider which mostly circle around the predecessor
+    # of the job that is currently run:
+    # * The predecessor ran successfully within the 15 minutes interval between it being scheduled and the current job
+    #   having to run. If this is the case, then the current job will only have to handle only the point in time of
+    #   its cron_at value.
+    # * The predecessor took longer to run than 15 minutes or was scheduled to run at a later time (because its)
+    #   predecessor was delayed. This will potentially lead to the current job also being delayed. If this is the case,
+    #   then the current job will have to handle all the 15 minute slots between its cron_at value and the cron_at value
+    #   of its predecessor + 15 minutes.
+    # * There is no predecessor since it is the first job ever to run or for some reasons the GoodJob::Execution where
+    #   salvaged. In this case there is no certainty as to what is the right choice which is why the cron_at value of
+    #   the current job is again used as the sole point in time to consider.
+    #
+    # In other words, the current job will take the
+    # * cron_at value of the current job as the maximum time to consider.
+    # * Take the cron_at of the previous job + 15 minutes as the minimum time to consider.
+    # If no previous job exists, then the cron_at of the current job is the minimum.
+    #
+    # Using this pattern, between all the instances of this job, every time slot where there is the potential
+    # for 1:00 am local time are covered. The sole exception would be the case where previously finished jobs
+    # have been removed from the database and the current job took longer to start executing. This is for now
+    # an accepted shortcoming as the likelihood of this happening is considered to be very low.
+    def every_quater_hour_between_predecessor_cron_at_and_own_cron_at
+      (lower_boundary.to_i..upper_boundary.to_i)
+        .step(15.minutes)
+        .map do |time|
+        Time.zone.at(time)
+      end
     end
 
-    def scheduled_time
-      job_scheduled_at.then { |t| t.change(min: t.min / 15 * 15) }
+    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

spec/workers/notifications/schedule_date_alerts_notifications_job_spec.rb

@@ -48,14 +48,32 @@
   end
 
   let(:scheduled_job) { described_class.perform_later }
+  let(:scheduled_predecessor_job) do
+    described_class.perform_later.tap do
+      GoodJob.perform_inline
+    end
+  end
 
   before do
     ActiveJob::Base.disable_test_adapter
-    scheduled_job
   end
 
-  def set_scheduled_time(scheduled_at)
-    GoodJob::Job.where(id: scheduled_job.job_id).update_all(scheduled_at:)
+  def set_predecessor_cron_time(cron_at)
+    GoodJob::Job
+      .where(id: scheduled_predecessor_job.job_id)
+      .update_all(cron_at:)
+  end
+
+  def set_cron_time(cron_at)
+    GoodJob::Job
+      .where(id: scheduled_job.job_id)
+      .update_all(cron_at:)
+  end
+
+  def set_scheduled_at_time(scheduled_at)
+    GoodJob::Job
+      .where(id: scheduled_job.job_id)
+      .update_all(scheduled_at:)
   end
 
   # Converts "hh:mm" into { hour: h, min: m }
@@ -67,11 +85,12 @@ def timezone_time(time, timezone)
     timezone.now.change(time_hash(time))
   end
 
-  def run_job(scheduled_at:, local_time:, timezone:)
-    set_scheduled_time(timezone_time(scheduled_at, timezone))
+  def run_job(cron_at:, local_time:, timezone:, scheduled_at: cron_at)
+    set_cron_time(timezone_time(cron_at, timezone))
+    set_scheduled_at_time(timezone_time(scheduled_at, timezone))
+
     travel_to(timezone_time(local_time, timezone)) do
       GoodJob.perform_inline
-      # scheduled_job.reload.invoke_job
 
       yield if block_given?
     end
@@ -94,9 +113,16 @@ def expect_job(job, *arguments)
   shared_examples_for "job execution creates date alerts creation job" do
     let(:job_class) { Notifications::CreateDateAlertsNotificationsJob.name }
 
+    before do
+      set_predecessor_cron_time(timezone_time(predecessor_cron_at, timezone)) if defined?(predecessor_cron_at)
+    end
+
     it "creates the job for the user" do
       expect do
-        run_job(timezone:, scheduled_at:, local_time:) do
+        run_job(timezone:,
+                cron_at:,
+                scheduled_at: defined?(scheduled_at) ? scheduled_at : cron_at,
+                local_time:) do
           j = GoodJob::Job.where(job_class:)
                           .order(created_at: :desc)
                           .last
@@ -107,18 +133,33 @@ def expect_job(job, *arguments)
   end
 
   shared_examples_for "job execution creates no date alerts creation job" do
+    before do
+      set_predecessor_cron_time(timezone_time(predecessor_cron_at, timezone)) if defined?(predecessor_cron_at)
+    end
+
     it "creates no job" do
       expect do
-        run_job(timezone:, scheduled_at:, local_time:)
-      end.not_to change(GoodJob::Job, :count)
+        run_job(timezone:,
+                cron_at:,
+                scheduled_at: defined?(scheduled_at) ? scheduled_at : cron_at,
+                local_time:)
+      end.not_to change { GoodJob::Job.where(job_class: Notifications::CreateDateAlertsNotificationsJob.name).count }
+    end
+  end
+
+  describe "#perform_later" do
+    it "only one can be scheduled at a time" do
+      scheduled_job
+
+      expect(described_class.perform_later).to be_falsey
     end
   end
 
   describe "#perform" do
     context "for users whose local time is 1:00 am (UTC+1) when the job is executed" do
       it_behaves_like "job execution creates date alerts creation job" do
         let(:timezone) { timezone_paris }
-        let(:scheduled_at) { "1:00" }
+        let(:cron_at) { "1:00" }
         let(:local_time) { "1:04" }
         let(:user) { user_paris }
       end
@@ -127,7 +168,7 @@ def expect_job(job, *arguments)
     context "for users whose local time is 1:00 am (UTC+05:45) when the job is executed" do
       it_behaves_like "job execution creates date alerts creation job" do
         let(:timezone) { timezone_kathmandu }
-        let(:scheduled_at) { "1:00" }
+        let(:cron_at) { "1:00" }
         let(:local_time) { "1:04" }
         let(:user) { user_kathmandu }
       end
@@ -136,46 +177,89 @@ def expect_job(job, *arguments)
     context "without enterprise token", with_ee: false do
       it_behaves_like "job execution creates no date alerts creation job" do
         let(:timezone) { timezone_paris }
-        let(:scheduled_at) { "1:00" }
+        let(:cron_at) { "1:00" }
         let(:local_time) { "1:04" }
       end
     end
 
-    context "when scheduled and executed at 01:00 am local time" do
+    context "when cron-ed, scheduled and executed at 01:00 am local time" do
       it_behaves_like "job execution creates date alerts creation job" do
         let(:timezone) { timezone_paris }
-        let(:scheduled_at) { "1:00" }
+        let(:cron_at) { "1:00" }
         let(:local_time) { "1:00" }
         let(:user) { user_paris }
       end
     end
 
-    context "when scheduled and executed at 01:14 am local time" do
+    context "when cron-ed at 1:00 am but scheduled and executed at 01:14 am local time" do
       it_behaves_like "job execution creates date alerts creation job" do
         let(:timezone) { timezone_paris }
+        let(:cron_at) { "1:00" }
         let(:scheduled_at) { "1:14" }
         let(:local_time) { "1:14" }
         let(:user) { user_paris }
       end
     end
 
-    context "when scheduled and executed at 01:15 am local time" do
+    context "when cron-ed, scheduled and executed at 01:15 am local time" do
       it_behaves_like "job execution creates no date alerts creation job" do
         let(:timezone) { timezone_paris }
+        let(:cron_at) { "1:15" }
         let(:scheduled_at) { "1:15" }
         let(:local_time) { "1:15" }
       end
     end
 
-    context "when scheduled at 01:00 am local time and executed at 01:37 am local time" do
+    context "when cron-ed and scheduled at 01:00 am local time and executed at 01:37 am local time" do
       it_behaves_like "job execution creates date alerts creation job" do
         let(:timezone) { timezone_paris }
+        let(:cron_at) { "1:00" }
         let(:scheduled_at) { "1:00" }
         let(:local_time) { "1:37" }
         let(:user) { user_paris }
       end
     end
 
+    context "when cron-ed at 01:00 am but rescheduled to 1:15 and executed at 01:37 am local time" do
+      it_behaves_like "job execution creates date alerts creation job" do
+        let(:timezone) { timezone_paris }
+        let(:cron_at) { "1:00" }
+        let(:scheduled_at) { "1:15" }
+        let(:local_time) { "1:37" }
+        let(:user) { user_paris }
+      end
+    end
+
+    context "when cron-ed at 00:45 am but rescheduled to 1:15 and executed at 01:37 am local time" do
+      it_behaves_like "job execution creates no date alerts creation job" do
+        let(:timezone) { timezone_paris }
+        let(:cron_at) { "0:45" }
+        let(:scheduled_at) { "1:15" }
+        let(:local_time) { "1:37" }
+      end
+    end
+
+    context "when cron-ed, scheduled and executed at 01:15 am and the predecessor being cron-ed to 0:30 am" do
+      it_behaves_like "job execution creates date alerts creation job" do
+        let(:timezone) { timezone_paris }
+        let(:cron_at) { "1:15" }
+        let(:predecessor_cron_at) { "0:30" }
+        let(:scheduled_at) { "1:15" }
+        let(:local_time) { "1:15" }
+        let(:user) { user_paris }
+      end
+    end
+
+    context "when cron-ed, scheduled and executed at 01:15 am and the predecessor being cron-ed to 1:00 am" do
+      it_behaves_like "job execution creates no date alerts creation job" do
+        let(:timezone) { timezone_paris }
+        let(:cron_at) { "1:15" }
+        let(:predecessor_cron_at) { "1:00" }
+        let(:scheduled_at) { "1:15" }
+        let(:local_time) { "1:15" }
+      end
+    end
+
     context "with a user having only due_date active in notification settings" do
       before do
         NotificationSetting
@@ -187,7 +271,7 @@ def expect_job(job, *arguments)
 
       it_behaves_like "job execution creates date alerts creation job" do
         let(:timezone) { timezone_paris }
-        let(:scheduled_at) { "1:00" }
+        let(:cron_at) { "1:00" }
         let(:local_time) { "1:00" }
         let(:user) { user_paris }
       end
@@ -204,7 +288,7 @@ def expect_job(job, *arguments)
 
       it_behaves_like "job execution creates date alerts creation job" do
         let(:timezone) { timezone_paris }
-        let(:scheduled_at) { "1:00" }
+        let(:cron_at) { "1:00" }
         let(:local_time) { "1:00" }
         let(:user) { user_paris }
       end
@@ -221,7 +305,7 @@ def expect_job(job, *arguments)
 
       it_behaves_like "job execution creates date alerts creation job" do
         let(:timezone) { timezone_paris }
-        let(:scheduled_at) { "1:00" }
+        let(:cron_at) { "1:00" }
         let(:local_time) { "1:00" }
         let(:user) { user_paris }
       end
@@ -238,7 +322,7 @@ def expect_job(job, *arguments)
 
       it_behaves_like "job execution creates no date alerts creation job" do
         let(:timezone) { timezone_paris }
-        let(:scheduled_at) { "1:00" }
+        let(:cron_at) { "1:00" }
         let(:local_time) { "1:00" }
       end
     end
@@ -261,7 +345,7 @@ def expect_job(job, *arguments)
 
       it_behaves_like "job execution creates date alerts creation job" do
         let(:timezone) { timezone_paris }
-        let(:scheduled_at) { "1:00" }
+        let(:cron_at) { "1:00" }
         let(:local_time) { "1:00" }
         let(:user) { user_paris }
       end
@@ -274,7 +358,7 @@ def expect_job(job, *arguments)
 
       it_behaves_like "job execution creates no date alerts creation job" do
         let(:timezone) { timezone_paris }
-        let(:scheduled_at) { "1:00" }
+        let(:cron_at) { "1:00" }
         let(:local_time) { "1:00" }
       end
     end