Merge branch 'fix-pipeline-schedule-edge-case' into 'master'

FIX: Scheduled pipelines scheduled to run at the same time as pipeline_schedule_worker gets pushed to the next run

Closes #63469

See merge request gitlab-org/gitlab-ce!29848

app/models/ci/pipeline_schedule.rb

@@ -55,15 +55,20 @@ module Ci
     # This way, a schedule like `*/1 * * * *` won't be triggered in a short interval
     # when PipelineScheduleWorker runs irregularly by Sidekiq Memory Killer.
     def set_next_run_at
-      self.next_run_at = Gitlab::Ci::CronParser.new(Settings.cron_jobs['pipeline_schedule_worker']['cron'],
-                                                    Time.zone.name)
-                                               .next_time_from(ideal_next_run_at)
+      now = Time.zone.now
+      ideal_next_run = ideal_next_run_from(now)
+
+      self.next_run_at = if ideal_next_run == cron_worker_next_run_from(now)
+                           ideal_next_run
+                         else
+                           cron_worker_next_run_from(ideal_next_run)
+                         end
     end
 
     def schedule_next_run!
       save! # with set_next_run_at
     rescue ActiveRecord::RecordInvalid
-      update_attribute(:next_run_at, nil) # update without validation
+      update_column(:next_run_at, nil) # update without validation
     end
 
     def job_variables
@@ -72,9 +77,15 @@ module Ci
 
     private
 
-    def ideal_next_run_at
+    def ideal_next_run_from(start_time)
       Gitlab::Ci::CronParser.new(cron, cron_timezone)
-        .next_time_from(Time.zone.now)
+                            .next_time_from(start_time)
+    end
+
+    def cron_worker_next_run_from(start_time)
+      Gitlab::Ci::CronParser.new(Settings.cron_jobs['pipeline_schedule_worker']['cron'],
+                                Time.zone.name)
+                            .next_time_from(start_time)
     end
   end
 end

changelogs/unreleased/fix-pipeline-schedule-edge-case.yml

+---
+title: Fix pipeline schedule does not run correctly when it's scheduled at the same
+  time with the cron worker
+merge_request: 29848
+author:
+type: fixed

spec/models/ci/pipeline_schedule_spec.rb

@@ -88,23 +88,8 @@ describe Ci::PipelineSchedule do
 
   describe '#set_next_run_at' do
     let(:pipeline_schedule) { create(:ci_pipeline_schedule, :nightly) }
-    let(:ideal_next_run_at) { pipeline_schedule.send(:ideal_next_run_at) }
-
-    let(:expected_next_run_at) do
-      Gitlab::Ci::CronParser.new(Settings.cron_jobs['pipeline_schedule_worker']['cron'], Time.zone.name)
-        .next_time_from(ideal_next_run_at)
-    end
-
-    let(:cron_worker_next_run_at) do
-      Gitlab::Ci::CronParser.new(Settings.cron_jobs['pipeline_schedule_worker']['cron'], Time.zone.name)
-        .next_time_from(Time.zone.now)
-    end
-
-    context 'when creates new pipeline schedule' do
-      it 'updates next_run_at automatically' do
-        expect(pipeline_schedule.next_run_at).to eq(expected_next_run_at)
-      end
-    end
+    let(:ideal_next_run_at) { pipeline_schedule.send(:ideal_next_run_from, Time.zone.now) }
+    let(:cron_worker_next_run_at) { pipeline_schedule.send(:cron_worker_next_run_from, Time.zone.now) }
 
     context 'when PipelineScheduleWorker runs at a specific interval' do
       before do
@@ -129,7 +114,7 @@ describe Ci::PipelineSchedule do
       let(:pipeline_schedule) { create(:ci_pipeline_schedule, :every_minute) }
 
       it "updates next_run_at to the sidekiq worker's execution time" do
-        Timecop.freeze(2019, 06, 19, 12, 00) do
+        Timecop.freeze(Time.parse("2019-06-01 12:18:00+0000")) do
           expect(pipeline_schedule.next_run_at).to eq(cron_worker_next_run_at)
         end
       end
@@ -157,9 +142,8 @@ describe Ci::PipelineSchedule do
       let(:new_cron) { '0 0 1 1 *' }
 
       it 'updates next_run_at automatically' do
-        pipeline_schedule.update!(cron: new_cron)
-
-        expect(pipeline_schedule.next_run_at).to eq(expected_next_run_at)
+        expect { pipeline_schedule.update!(cron: new_cron) }
+          .to change { pipeline_schedule.next_run_at }
       end
     end
   end
@@ -167,21 +151,24 @@ describe Ci::PipelineSchedule do
   describe '#schedule_next_run!' do
     let!(:pipeline_schedule) { create(:ci_pipeline_schedule, :nightly) }
 
-    context 'when reschedules after 10 days from now' do
-      let(:future_time) { 10.days.from_now }
-      let(:ideal_next_run_at) { pipeline_schedule.send(:ideal_next_run_at) }
+    before do
+      pipeline_schedule.update_column(:next_run_at, nil)
+    end
 
-      let(:expected_next_run_at) do
-        Gitlab::Ci::CronParser.new(Settings.cron_jobs['pipeline_schedule_worker']['cron'], Time.zone.name)
-          .next_time_from(ideal_next_run_at)
+    it 'updates next_run_at' do
+      expect { pipeline_schedule.schedule_next_run! }
+        .to change { pipeline_schedule.next_run_at }
     end
 
-      it 'points to proper next_run_at' do
-        Timecop.freeze(future_time) do
+    context 'when record is invalid' do
+      before do
+        allow(pipeline_schedule).to receive(:save!) { raise ActiveRecord::RecordInvalid.new(pipeline_schedule) }
+      end
+
+      it 'nullifies the next run at' do
         pipeline_schedule.schedule_next_run!
 
-          expect(pipeline_schedule.next_run_at).to eq(expected_next_run_at)
-        end
+        expect(pipeline_schedule.next_run_at).to be_nil
       end
     end
   end