Add unique constraints to ClockedSchedule and CrontabSchedule models #981
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds unique constraints to the IntervalSchedule, ClockedSchedule, and CrontabSchedule models to prevent race conditions and duplicate entries when using get_or_create under concurrent workers. The changes bring these models in line with the existing SolarSchedule model, which already uses unique_together. The PR addresses issues #980 and #701 related to duplicate schedule entries.
Key changes:
- Added
UniqueConstraintoneveryandperiodfields forIntervalSchedule - Added
unique=Trueto theclocked_timefield forClockedSchedule - Added
UniqueConstrainton all scheduling fields (minute, hour, day_of_month, month_of_year, day_of_week, timezone) forCrontabSchedule
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| django_celery_beat/models.py | Added unique constraints to IntervalSchedule, ClockedSchedule, and CrontabSchedule models to prevent duplicate schedule entries |
| django_celery_beat/migrations/0020_alter_clockedschedule_clocked_time_and_more.py | Database migration to apply the unique constraints and alter the ClockedSchedule.clocked_time field |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| constraints = [ | ||
| models.UniqueConstraint(fields=["every", "period"], name="unique_interval") | ||
| ] |
There was a problem hiding this comment.
The unique constraints added by this PR will prevent the creation of duplicate schedules, but the existing test _test_duplicate_schedules in t/unit/test_models.py expects to be able to create duplicates (line 77 creates a second instance with the same kwargs, and line 78 expects 2 instances to exist). These tests will now fail with IntegrityError when trying to create the second duplicate instance. The tests need to be updated to either:
- Test that duplicates are prevented (expect IntegrityError)
- Use different kwargs for the second instance
- Test that
from_scheduleproperly handles uniqueness withget_or_create
| help_text=_('Run the task at clocked time'), | ||
| verbose_name=_("Clock Time"), | ||
| help_text=_("Run the task at clocked time"), | ||
| unique=True, |
There was a problem hiding this comment.
The unique constraint on clocked_time will prevent the creation of duplicate schedules, but the existing test test_duplicate_schedules in ClockedScheduleTestCase (t/unit/test_models.py, line 148-150) expects to be able to create duplicates. This test will now fail with IntegrityError. The test needs to be updated to verify that the unique constraint works correctly.
| constraints = [ | ||
| models.UniqueConstraint( | ||
| fields=[ | ||
| "minute", | ||
| "hour", | ||
| "day_of_month", | ||
| "month_of_year", | ||
| "day_of_week", | ||
| "timezone", | ||
| ], | ||
| name="unique_crontab", | ||
| ) | ||
| ] |
There was a problem hiding this comment.
The unique constraint on CrontabSchedule will prevent the creation of duplicate schedules, but the existing test test_duplicate_schedules in CrontabScheduleTestCase (t/unit/test_models.py, line 99-109) expects to be able to create duplicates. This test will now fail with IntegrityError. The test needs to be updated to verify that the unique constraint works correctly.
auvipy
left a comment
auvipy
left a comment
There was a problem hiding this comment.
can you please check the comments and test failures in the CI
|
Thanks for the feedback. Before adjusting the tests, I think it would be useful to have a broader discussion about the implications of adding these uniqueness constraints. At the moment, the test suite explicitly allows the creation of duplicate schedule instances, which suggests that the current behavior is either intentional or at least tolerated for backward-compatibility reasons. Introducing strict uniqueness rules would change that contract, and may impact existing installations that rely on the current behavior—especially where duplicate rows may already exist in production databases. My PR was meant as an indicative proposal to address the race-condition issues reported in #980 and schedule uniqueness requested in #701, not as a final decision. As @sevdog noted in #701, the tuple of fields in CrontabSchedule (and similarly in the other schedule models) is conceptually meant to identify a single schedule instance. If that tuple is not unique, then multiple identical rows can coexist and the table becomes more of a loose validation helper rather than a true normalized representation of schedules. This aligns with the idea that—if duplication is intentional or required—it should ideally be configurable rather than the default behavior. The changes themselves make sense from a concurrency and data-integrity standpoint, but they should be evaluated in light of: Once there is agreement on the intended behavior moving forward, I’d be happy to contribute. In any case, I suggest we make a collective decision on this point and update the documentation so that users clearly understand the expected behavior and the reasoning behind it. Let me know how you’d like to proceed. |
|
we need time to decide on that. |
2 participants
Implement unique constraints for the ClockedSchedule and CrontabSchedule models to prevent race conditions and duplicate entries when using get_or_create under concurrent workers. This change ensures that only one instance of each schedule can be created, addressing the issue of duplicated schedule entries.
Fixes #980 and #701.