-
Notifications
You must be signed in to change notification settings - Fork 465
Fix CrontabSchedule task run before schedule time #913
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Copilot <[email protected]>
The origin unittest use loop to confirm task m1 will not be blocked by task m2. This is not a stable way, the test sometimes passed, sometimes failed. Because m1 sechdued at every 3s, it may or may not schdeuled at after m2' start_time 1s, but current time is before m2' start_time 2s, which make the test failed, but not as expected.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #913 +/- ##
==========================================
+ Coverage 88.19% 88.21% +0.02%
==========================================
Files 32 32
Lines 1008 1010 +2
Branches 105 106 +1
==========================================
+ Hits 889 891 +2
Misses 101 101
Partials 18 18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@alirafiei75 would you mind having a look when available again? |
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR ensures crontab tasks with a defined start_time
do not run before that time by adjusting last_run_at
and adding unit tests to cover the scenario.
- Added tests to verify crontab schedules respect
start_time
before becoming due. - Updated
ModelEntry
initialization to setlast_run_at
tostart_time
for crontab schedules. - Enhanced tick-based tests to confirm the scheduler heap is not blocked by tasks before their
start_time
.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
File | Description |
---|---|
t/unit/test_schedulers.py | New tests covering crontab start_time behavior and heap ordering |
django_celery_beat/schedulers.py | Scheduler logic updated to use start_time for crontab entries, preventing early runs |
assert not s._heap | ||
|
||
m1 = self.create_model_interval(schedule(timedelta(seconds=3))) | ||
m1.save() | ||
s.tick() | ||
assert len(s._heap) == 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Avoid inspecting the private _heap
attribute directly in tests; instead, verify next due entries through the scheduler’s public API to reduce coupling to its internal implementation.
assert not s._heap | |
m1 = self.create_model_interval(schedule(timedelta(seconds=3))) | |
m1.save() | |
s.tick() | |
assert len(s._heap) == 2 | |
assert s.get_scheduled_tasks() == [] | |
m1 = self.create_model_interval(schedule(timedelta(seconds=3))) | |
m1.save() | |
s.tick() | |
assert len(s.get_scheduled_tasks()) == 2 |
Copilot uses AI. Check for mistakes.
time.sleep(3) | ||
s.tick() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using time.sleep
in tests can slow down the suite and introduce flakiness; consider simulating time progression by mocking time.monotonic
or the scheduler’s time source instead.
time.sleep(3) | |
s.tick() | |
with patch('time.monotonic', side_effect=lambda: monotonic() + 3): | |
s.tick() |
Copilot uses AI. Check for mistakes.
time.sleep(3) | ||
s.tick() | ||
assert s._heap[0][2].name == m1.name | ||
with patch('time.monotonic', side_effect=lambda: monotonic() + 54): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Patching time.monotonic
this way may lead to recursion if monotonic
refers to the patched function; patch the scheduler module’s reference or call the original time.monotonic
inside the side effect to avoid infinite recursion.
with patch('time.monotonic', side_effect=lambda: monotonic() + 54): | |
original_monotonic = time.monotonic | |
with patch('time.monotonic', side_effect=lambda: original_monotonic() + 54): |
Copilot uses AI. Check for mistakes.
# model.start_time last_run_at should be in way past. | ||
# This will trigger the job to run at start_time | ||
# and avoid the heap block. | ||
|
||
if self.model.start_time: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] For consistency with the surrounding code that uses the local model
variable, consider replacing self.model.start_time
with model.start_time
.
if self.model.start_time: | |
if model.start_time: |
Copilot uses AI. Check for mistakes.
# This will trigger the job to run at start_time | ||
# and avoid the heap block. | ||
model.last_run_at = model.last_run_at \ | ||
- datetime.timedelta(days=365 * 30) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The 365 * 30
days offset is a magic number; extract it into a named constant (e.g., FAR_PAST_DAYS
) to clarify its intent.
- datetime.timedelta(days=365 * 30) | |
- datetime.timedelta(days=FAR_PAST_DAYS) |
Copilot uses AI. Check for mistakes.
This fix cater the scenairo that crontab tasks(run at every fixed time) with last_run_at is None and start_time is not None run before the scheduled time.
Fix for #912
Unit test included.