Skip to content

Commit

Permalink
Refactor cron job time calculations
Browse files Browse the repository at this point in the history
When a job is scheduled exactly between next and previous runs then round down to the previous run time.
Updated specs to reflect this.
  • Loading branch information
afrase authored and marcelolx committed May 16, 2024
1 parent 2deef88 commit 9da5f13
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 12 deletions.
11 changes: 3 additions & 8 deletions lib/sidekiq-scheduler/utils.rb
Original file line number Diff line number Diff line change
Expand Up @@ -164,14 +164,9 @@ def self.calc_cron_run_time(cron, time)
previous_diff = time - previous_t

if next_diff == previous_diff
# `next_diff` and `previous_diff` can be equal in two scenarios.
# 1. The `time` is exactly when the job was scheduled to run.
# 2. The `time` is exactly between `next_t` and `previous_t`, down to the exact second.
#
# The odds of 2 happening are basically zero but it is possible.
# So in scenario 2 it's possible to return the wrong time. For example if the job runs every minute and `time`
# is 15:25.30 then the returned time will be 15:25.30 instead of 15:25.0 (if rounding down).
time
# In the event `time` is exactly between `previous_t` and `next_t` the diff will not be equal to
# `cron.rough_frequency`. In that case we round down.
cron.rough_frequency == next_diff ? time : previous_t
elsif next_diff > previous_diff
# We are closer to the previous run time so return that.
previous_t
Expand Down
8 changes: 4 additions & 4 deletions spec/sidekiq-scheduler/utils_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -399,8 +399,8 @@
# every minute would mean 30 seconds is between next and previous run
let(:time) { Time.utc(2024, 1, 1, 9, 0, 30) }

it 'returns the time' do
expect(described_class.calc_cron_run_time(cron, time)).to eq(time)
it 'returns the previous time' do
expect(described_class.calc_cron_run_time(cron, time)).to eq(cron.previous_time(time).utc)
end
end

Expand Down Expand Up @@ -444,8 +444,8 @@
# every hour would mean 30 minutes is between next and previous run
let(:time) { Time.utc(2024, 1, 1, 9, 30) }

it 'returns the time' do
expect(described_class.calc_cron_run_time(cron, time)).to eq(time)
it 'returns the previous time' do
expect(described_class.calc_cron_run_time(cron, time)).to eq(cron.previous_time(time).utc)
end
end

Expand Down

0 comments on commit 9da5f13

Please sign in to comment.