Skip to content

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions django_celery_beat/schedulers.py
Original file line number Diff line number Diff line change
Expand Up @@ -94,13 +94,19 @@ def __init__(self, model, app=None):

if not model.last_run_at:
model.last_run_at = model.date_changed or self._default_now()
# if last_run_at is not set and
# 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:
Copy link
Preview

Copilot AI Jul 12, 2025

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.

Suggested change
if self.model.start_time:
if model.start_time:

Copilot uses AI. Check for mistakes.

model.last_run_at = model.last_run_at \
- datetime.timedelta(days=365 * 30)
if isinstance(model.schedule, schedules.schedule) \
and not isinstance(model.schedule, schedules.crontab):
# if last_run_at is not set and
# 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.
model.last_run_at = model.last_run_at \
- datetime.timedelta(days=365 * 30)
Copy link
Preview

Copilot AI Jul 12, 2025

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.

Suggested change
- datetime.timedelta(days=365 * 30)
- datetime.timedelta(days=FAR_PAST_DAYS)

Copilot uses AI. Check for mistakes.

else:
# last_run_at should be the time the task started.
model.last_run_at = model.start_time

self.last_run_at = model.last_run_at

Expand Down
41 changes: 35 additions & 6 deletions t/unit/test_schedulers.py
Original file line number Diff line number Diff line change
Expand Up @@ -1035,6 +1035,24 @@ def test_crontab_with_start_time_after_crontab(self, app):
assert not is_due
assert next_check == pytest.approx(expected_delay, abs=60)

def test_crontab_with_start_time_before_crontab(self, app):
now = app.now()
delay_minutes = 2
test_start_time = now - timedelta(minutes=delay_minutes)
crontab_time = now + timedelta(minutes=delay_minutes)

# start_time(now - 2min) < now < crontab_time(now + 2min)
task = self.create_model_crontab(
crontab(minute=f'{crontab_time.minute}'),
start_time=test_start_time)

entry = EntryTrackSave(task, app=app)
is_due, next_check = entry.is_due()

expected_delay = delay_minutes * 60
assert not is_due
assert next_check < expected_delay

def test_crontab_with_start_time_different_time_zone(self, app):
now = app.now()

Expand Down Expand Up @@ -1083,31 +1101,42 @@ def test_crontab_with_start_time_different_time_zone(self, app):
assert next_check == pytest.approx(expected_delay, abs=60)

def test_crontab_with_start_time_tick(self, app):
# Ensure the heapq does not block by new task with start_time
PeriodicTask.objects.all().delete()
s = self.Scheduler(app=self.app)
assert not s._heap

m1 = self.create_model_interval(schedule(timedelta(seconds=3)))
m1.save()
s.tick()
assert len(s._heap) == 2
Comment on lines 1107 to +1112
Copy link
Preview

Copilot AI Jul 12, 2025

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.

Suggested change
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.


now = timezone.now()
start_time = now + timedelta(minutes=1)
crontab_trigger_time = now + timedelta(minutes=2)

# now < start_time(now + 1min) < crontab_time(now + 2min)
m2 = self.create_model_crontab(
crontab(minute=f'{crontab_trigger_time.minute}'),
start_time=start_time)
m2.save()
s.tick()
assert len(s._heap) == 3
assert s._heap[0][2].name == m1.name

e2 = EntryTrackSave(m2, app=self.app)
is_due, _ = e2.is_due()
is_due, delay = e2.is_due()
assert not is_due
assert 60 < delay < 120

max_iterations = 1000
iterations = 0
while (not is_due and iterations < max_iterations):
# tick twice to make sure the heap is not blocked by m2
# before it reaches its start_time
time.sleep(3)
Copy link
Preview

Copilot AI Jun 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using real time.sleep calls makes this test slow and brittle. Consider mocking or freezing time (e.g., with freezegun or a custom clock) to simulate time progression instantly instead of waiting in real time.

Copilot uses AI. Check for mistakes.

s.tick()
Comment on lines +1134 to +1135
Copy link
Preview

Copilot AI Jul 12, 2025

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.

Suggested change
time.sleep(3)
s.tick()
with patch('time.monotonic', side_effect=lambda: monotonic() + 3):
s.tick()

Copilot uses AI. Check for mistakes.

assert s._heap[0][2].name == m1.name
with patch('time.monotonic', side_effect=lambda: monotonic() + 54):
Copy link
Preview

Copilot AI Jul 12, 2025

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.

Suggested change
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.

s.tick()
assert s._heap[0][2].name != m2.name
is_due, _ = e2.is_due()
assert s._heap[0][2].name == m1.name

@pytest.mark.django_db
def test_crontab_exclusion_logic_basic(self):
Expand Down