Skip to content

fix: cap is_due delay at start_time for one-off crontab tasks (#1044) - #1045

Open
yanlong-pan wants to merge 1 commit into
celery:mainfrom
yanlong-pan:fix/1044-oneoff-crontab-start-time
Open

fix: cap is_due delay at start_time for one-off crontab tasks (#1044)#1045
yanlong-pan wants to merge 1 commit into
celery:mainfrom
yanlong-pan:fix/1044-oneoff-crontab-start-time

Conversation

@yanlong-pan

Copy link
Copy Markdown

For a one-off PeriodicTask whose CrontabSchedule pins day_of_month + month_of_year (and often day_of_week) to specific values, CrontabSchedule.due_start_time() returns the next matching calendar date -- typically one year in the future. ModelEntry.is_due() then returned this years-long delay, causing the task to fire ~5 minutes late (when the forced sync interval expires) instead of at start_time.

Cap the start_time used for delay computation by min()-ing with the user-specified model.start_time when model.one_off is True. Recurring crontabs are unaffected -- day_of_month='*' makes the next match at most 24h away, and the post-#844 wake-at-next-match behavior is desirable for them.

Fixes #1044

…#1044)

For a one-off PeriodicTask whose CrontabSchedule pins day_of_month +
month_of_year (and often day_of_week) to specific values,
CrontabSchedule.due_start_time() returns the *next* matching calendar
date -- typically one year in the future. ModelEntry.is_due() then
returned this years-long delay, causing the task to fire ~5 minutes
late (when the forced sync interval expires) instead of at start_time.

Cap the start_time used for delay computation by min()-ing with the
user-specified model.start_time when model.one_off is True. Recurring
crontabs are unaffected -- day_of_month='*' makes the next match at
most 24h away, and the post-celery#844 wake-at-next-match behavior is
desirable for them.

Fixes celery#1044
@codecov

codecov Bot commented May 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.70%. Comparing base (cf85723) to head (e21b055).
⚠️ Report is 13 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1045      +/-   ##
==========================================
+ Coverage   87.68%   87.70%   +0.02%     
==========================================
  Files          32       32              
  Lines        1015     1017       +2     
  Branches       81       82       +1     
==========================================
+ Hits          890      892       +2     
  Misses        107      107              
  Partials       18       18              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown

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 fixes a scheduling regression for one-off crontab-backed PeriodicTasks with future start_time, ensuring they wake at start_time rather than the next matching fixed-date crontab occurrence.

Changes:

  • Caps one-off task delay calculation at model.start_time in ModelEntry.is_due().
  • Adds regression coverage for fixed-date one-off crontab tasks.
  • Adds coverage confirming recurring crontab behavior remains unchanged.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
django_celery_beat/schedulers.py Caps computed future start delay for one-off tasks so they do not sleep past start_time.
t/unit/test_schedulers.py Adds tests for the one-off fixed-date crontab regression and recurring crontab behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@auvipy auvipy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

so we are OK with partial changes for the regression?

@JinRiYao2001

JinRiYao2001 commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Author of #844 here, just wanted to share some notes from digging into this. The core question I keep coming back to is whether start_time >= cron match time should be considered a valid configuration — most of the rest follows from that.

The repro

run_at = timezone.now() + timedelta(minutes=2)  # e.g. 05:36:53.747
cron = CrontabSchedule(
    minute=str(run_at.minute),   # "36"
    hour=str(run_at.hour),       # "5"
    day_of_month=str(run_at.day),
    month_of_year=str(run_at.month),
    day_of_week=str(run_at.isoweekday() % 7),
)
task = PeriodicTask(crontab=cron, start_time=run_at, one_off=True)

The cron pattern matches at minute boundary (e.g. 05:36:00). start_time is set to run_at, which falls in the same minute as the cron tick (05:36:53 here, but the seconds don't matter — the boundary is the minute).

Whenever start_time lands in the same minute as the cron tick (or later) for a fixed-date crontab, CrontabSchedule.due_start_time() returns the next matching calendar date — typically years out. I verified this on the celery side: crontab.remaining_delta() uses strict < on last_run_at.minute < max(self.minute), so once start_time.minute == cron.minute, that occurrence is treated as already passed regardless of seconds. Only start_time strictly before the cron-tick minute (e.g. 05:35:59) is handled correctly.

In 2.7.0 this didn't surface because the delay was just start_time - now with no crontab lookup at all.

What #1045 actually does

The patch does min(due_start_time, model.start_time) for one-off tasks. Since due_start_time is by definition >= model.start_time, the min() is unconditionally model.start_time. So for one-off crontab tasks, the patch reverts the delay calculation back to pre-#844's start_time - now. Recurring crontab keeps #844's behavior. In that sense it's closer to a scoped revert of #844 on the one-off path than a scoped fix.

A question worth raising

The original issue #843 that #844 was addressing wasn't about the task failing to fire — it was about heap-top blocking. In celery's scheduler tick():

event = H[0]                      # peek at top, don't pop
is_due, _ = self.is_due(entry)
if is_due:
    heappop(H)                    # only pop when due
    ...
# if not due, the top stays, and entries below it are never evaluated this tick

Pre-#844, a crontab task with future start_time had heap priority equal to start_time (absolute). If that was the smallest priority in the heap, the entry sat at the top with is_due=False for the entire wait, and other entries weren't evaluated until it cleared. #844 pushed the priority forward to the next cron tick so the task moves down the heap and other entries can be processed normally during the wait.

With #1045, one-off crontab tasks with future start_time go back to having heap priority = start_time. Would this reintroduce the same heap-top blocking shape from #843 for one-off tasks with future start_time? Worth a closer look before merging.

Two ways to look at the policy call

  • If start_time falling in the same minute as the cron tick (or later) is treated as an invalid config, this isn't really a bug — users should set start_time strictly before the cron tick, or use ClockedSchedule for absolute single fires (which the issue itself already lists as the workaround, and which PeriodicTask already validates as one_off=True). Since celery crontab is minute-resolution (no seconds field), the cron tick is always at :00 of some minute, so start_time < cron match time naturally implies start_time lives in an earlier minute and the same-minute case is avoided by construction.
  • The 2.7.0 backward-compat angle is worth weighing against the actual impact surface. The justification really only holds if a meaningful number of users are relying on this shape in production — but from what's visible here, it looks like a single reporter's case rather than a broader pattern (the issue has been silent from other users, and ClockedSchedule already covers the one-off absolute-time use case cleanly). If you do want to land a fix anyway, it'd be worth the PR author taking another pass on (a) whether the heap-top blocking from Celery Task with start_time Set in the Future Causes Blocking of Other Tasks #843 comes back on the one-off path, and (b) the same minute-boundary misalignment exists for non-one-off fixed-date crontabs too, which the current patch doesn't cover.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

4 participants