Fix lock contention in PeriodicTasks change detection - #1061
Fix lock contention in PeriodicTasks change detection#1061alirafiei75 wants to merge 11 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1061 +/- ##
==========================================
+ Coverage 87.93% 88.34% +0.40%
==========================================
Files 32 33 +1
Lines 1028 1047 +19
Branches 85 90 +5
==========================================
+ Hits 904 925 +21
+ Misses 106 101 -5
- Partials 18 21 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…nge marker branches.
| for model, field in ( | ||
| (PeriodicTask, 'date_changed'), | ||
| (IntervalSchedule, 'updated_at'), | ||
| (CrontabSchedule, 'updated_at'), | ||
| (SolarSchedule, 'updated_at'), | ||
| (ClockedSchedule, 'updated_at'), | ||
| ): | ||
| val = model.objects.aggregate(m=Max(field))['m'] | ||
| if val: | ||
| stamps.append(val) |
There was a problem hiding this comment.
Call stamps.extend() once instead of repeated calls to stamps.append(). See: ruff rule FURB113
| for model, field in ( | |
| (PeriodicTask, 'date_changed'), | |
| (IntervalSchedule, 'updated_at'), | |
| (CrontabSchedule, 'updated_at'), | |
| (SolarSchedule, 'updated_at'), | |
| (ClockedSchedule, 'updated_at'), | |
| ): | |
| val = model.objects.aggregate(m=Max(field))['m'] | |
| if val: | |
| stamps.append(val) | |
| stamps.extend( | |
| val | |
| for model, field in ( | |
| (PeriodicTask, "date_changed"), | |
| (IntervalSchedule, "updated_at"), | |
| (CrontabSchedule, "updated_at"), | |
| (SolarSchedule, "updated_at"), | |
| (ClockedSchedule, "updated_at"), | |
| ) | |
| if (val := model.objects.aggregate(m=Max(field))["m"]) | |
| ) |
| marker = cls.objects.get(ident=1).last_change_marker | ||
| if marker: |
There was a problem hiding this comment.
| marker = cls.objects.get(ident=1).last_change_marker | |
| if marker: | |
| if marker := cls.objects.get(ident=1).last_change_marker: |
There was a problem hiding this comment.
Pull request overview
This PR refactors django-celery-beat’s schedule change detection to avoid lock contention on the singleton PeriodicTasks row by switching to pull-based change detection via MAX(date_changed/updated_at) and reserving the singleton marker for out-of-band changes (deletes, bulk updates), with marker bumps deferred to transaction.on_commit().
Changes:
- Implement pull-based change detection via
PeriodicTasks.last_change()aggregatingMAX()across task/schedule timestamp fields. - Add
updated_atfields (indexed) to schedule models and indexPeriodicTask.date_changed; repurposePeriodicTasks.last_update→last_change_marker. - Update scheduler housekeeping saves to avoid bumping
date_changed, and expand unit tests to cover the new change-detection behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
django_celery_beat/models.py |
Adds updated_at fields, indexes date_changed, changes PeriodicTasks marker semantics and implements last_change() aggregation logic. |
django_celery_beat/signals.py |
Removes hot-path save signals; retains delete hooks to bump the marker for deletions. |
django_celery_beat/schedulers.py |
Uses update_fields in housekeeping saves to avoid bumping date_changed and triggering reload loops. |
django_celery_beat/admin.py |
Ensures admin bulk actions that use queryset.update() also bump the change marker. |
django_celery_beat/migrations/0020_rename_last_update_periodictasks_last_change_marker_and_more.py |
Renames the marker field and adds timestamp/index schema needed for pull-based detection. |
t/unit/test_schedulers.py |
Adds/updates tests for pull-based detection, deletion marker bumps, and housekeeping behavior. |
t/unit/test_models.py |
Updates existing model tests to reflect last_change() semantics and on-commit marker bumping. |
Comments suppressed due to low confidence (1)
django_celery_beat/models.py:703
- PeriodicTask deletion now triggers PeriodicTasks.changed via the (newly adjusted) delete signal, so calling PeriodicTasks.changed() again in PeriodicTask.delete() schedules a duplicate on_commit bump and causes an extra write to the marker row for every instance.delete(). This is avoidable overhead (and partially reintroduces contention on delete-heavy workloads).
def delete(self, *args, **kwargs):
super().delete(*args, **kwargs)
PeriodicTasks.changed(self)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| signals.pre_delete.connect( | ||
| PeriodicTasks.update_changed, sender=IntervalSchedule | ||
| ) |
| migrations.AddField( | ||
| model_name="clockedschedule", | ||
| name="updated_at", | ||
| field=models.DateTimeField( | ||
| auto_now=True, db_index=True, null=True, verbose_name="Last Modified" | ||
| ), | ||
| ), |
There was a problem hiding this comment.
this is not necessary. the description was wrong and I will change it
|
|
||
| ident = models.SmallIntegerField(default=1, primary_key=True, unique=True) | ||
| last_update = models.DateTimeField(null=False) | ||
| last_change_marker = models.DateTimeField(null=False) |
There was a problem hiding this comment.
i think we can keep the attribute name as is, as there is no actual change here
There was a problem hiding this comment.
I just wanted this to be clear that the column has different usage now. I will revert it.
| PeriodicTasks.update_changed, sender=IntervalSchedule | ||
| ) | ||
|
|
||
| signals.post_save.connect( |
There was a problem hiding this comment.
changes here are breaking change. are they strictly necessary? can we figure out any backward compatible way?
There was a problem hiding this comment.
this is the main reason for PR. if I do not delete the post save signals, the lock duration stays the same under heavy load. with this change the beat just reads the save changes from another source.
| ) | ||
| updated_at = models.DateTimeField( | ||
| auto_now=True, | ||
| db_index=True, |
There was a problem hiding this comment.
for db_index should we consider index in meta option?
There was a problem hiding this comment.
yes meta is a better practice. I will change this and check the copilot comments too and will get back to you. thanks.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| @@ -0,0 +1,70 @@ | |||
| # Generated by Django 5.0.1 on 2026-07-05 07:08 | |||
There was a problem hiding this comment.
would you mind removing it and generate a new one with the newer changes? also it would be nice to use atleast django 5.2.x LTS
There was a problem hiding this comment.
this one has the new changes but I will now change it and generate it with lts
There was a problem hiding this comment.
Thank your for the effort @alirafiei75!
Overview
This PR replaces the singleton marker row (PeriodicTasks, ident=1, updated via update_or_create/lock) with pull-based detection: last_change() is now a MAX() over date_changed (PeriodicTask) and updated_at (new, on the 4 schedule models), plus the singleton marker, which is only touched for cases auto_now can't cover (deletes, bulk queryset.update()). Writes to the marker are deferred via transaction.on_commit(). This fixes the root cause of the contention (#1000): it removes the write from the save hot path entirely, rather than just swapping SELECT FOR UPDATE for a plain UPDATE (which would still hold the row lock until COMMIT).
I like this solution — I agree it's better than the fix I proposed in the original issue. This PR's pull-based approach eliminates the write from the hot path altogether.
Code quality & style
Well structured, comments land in the right places (the warning in ModelEntry.save() about not adding date_changed to update_fields is essential — without it, a future "fix" reintroduces the infinite reload loop). Migration is auto-generated and clean, indexes match the fields used in the MAX() queries.
Findings
-
Silent compatibility break —
PeriodicTasks.last_updateis a public model field. Any external code (dashboards, monitoring scripts, third-party libs) reading that field directly instead of callinglast_change()will now see a stale value, with no error and no warning.After this PR,
last_updateonly advances in 3 cases, all throughupdate_changed():- Delete —
post_deleteonPeriodicTaskor any of the 4 schedule models fireschanged()/update_changed()directly. - Admin bulk update —
enable_tasks,disable_tasks,toggle_tasks(admin.py) usequeryset.update(), which bypassesauto_now; that's why they callupdate_changed()manually right after. - First creation of the singleton row — if
ident=1doesn't exist yet,update_changed()creates it via a fallback (.create()withIntegrityErrorhandled for the concurrent-creation race).
A normal save of
PeriodicTaskor any schedule model no longer toucheslast_update— what advances instead isdate_changed/updated_at(auto_now) on those tables themselves, and onlylast_change()(theMAX()aggregating the marker plus those columns) reflects the real state. Before this PR,last_updatewas bumped on every save/delete, so this is a new, non-obvious behavior for anyone going by the field name alone. Worth an explicit breaking-change note in the changelog/docs — not just the class docstring — making clear thatlast_updatein isolation is no longer the source of truth, onlylast_change()is. - Delete —
-
Per-tick cost on Beat (suggestion, non-blocking) —
last_change()now runs up to 6 queries (1geton the marker + 5aggregate/MAX, one per model) every tick, versus 1 query before. Indexes make eachMAX()cheap in isolation, but that's 6 network/DB round-trips instead of 1 — adds up with a short tick interval (seconds) and many Beat instances running. The PR already acknowledges this trade-off.This could be collapsed into a single query via
UNION ALLover 6 subselects, avoiding the round-trips — but that steps outside plain Django ORM (would needRawSQL/.raw()or a manual cursor), which counts against readability and ORM-only familiarity for maintainers. Worth considering as an optimization if the overhead proves relevant in practice, not as a blocker — the current version is simpler to read and maintain, and the trade-off (6 lightweight indexed queries vs. 1 less-readable query) may not be worth it without measuring the actual cost first. -
Post-migration
NULLwindow (informational only) — theAddFieldforupdated_atisauto_now=True, null=Truewith nodefault, so existing schedule rows will haveupdated_at=NULLuntil their next save. This doesn't affect the correctness ofMAX()— SQL/Django aggregates ignoreNULL, so the computed value stays correct across the populated rows. A row that's never re-saved simply doesn't contribute tolast_change()until its first save after this deploy (expected, since nothing actually changed on it). The one event that would matter in that window — deleting a never-touched row — is already covered separately by the explicit marker bump viapost_delete, regardless of whetherupdated_atwasNULL. No correctness risk, just a behavioral detail worth knowing about post-deploy. -
no_changesflag is now dead for thePeriodicTasksave path —_disable()and theone_offbranch inis_due()still setself.model.no_changesbeforesave(), but sincePeriodicTasks.changed(self)was removed fromPeriodicTask.save()entirely (not just gated, removed), that flag no longer suppresses or triggers anything on that path — it only survives because_refresh_schedule()still copies it around in memory. A comment at those two call sites would prevent future confusion about what the flag actually does now. -
Minor —
admin.pycallsupdate_changed()unconditionally in all 3 actions (enable_tasks,disable_tasks,toggle_tasks), even whenqueryset.update()returns 0 affected rows:def enable_tasks(self, request, queryset): rows_updated = queryset.update(enabled=True) # queryset.update() bypasses auto_now on date_changed; bump marker. PeriodicTasks.update_changed() self.message_user(request, ...)
rows_updatedis computed but never used as a guard. In practice,rows_updated == 0only happens in a rare edge case (rows selected in the admin got deleted by another process between selection and clicking the action, or an empty queryset via direct URL manipulation) — but when it does, the code still firesupdate_changed()unnecessarily: it schedules anon_commitcallback, issues an unneeded UPDATE/CREATE against thePeriodicTaskstable, and forces Beat to reload the whole schedule on the next tick despite nothing having actually changed.Trivial fix, same pattern in all 3 actions:
if rows_updated: PeriodicTasks.update_changed()
Non-blocking — just a wasted write/reload in a rare edge case, doesn't cause inconsistency.
Test coverage
Good — covers save without marker bump, delete via marker, in-place edits for all 4 schedule types, the housekeeping-save guard, bulk update, and the IntegrityError fallback. Missing: a test that explicitly documents that PeriodicTasks.last_update (the raw field) goes stale after a normal save — locking in this behavior as intentional and preventing a future regression that "fixes" it back.
Recommendation
The design is correct and addresses the root cause, not just the symptom. Before approving I'd want: (1) an explicit breaking-change note in the PR/changelog for finding 1, (2) confirmation of the per-tick query cost for finding 2. auvipy already requested changes — worth checking whether they overlap with these points.
thanks for your detailed review. |
… and stale-marker test
|
Thanks for the thorough review @rodrigondec Finding 1 (compatibility break): Added an explicit breaking-change note in the Changelog and a test (test_normal_save_does_not_bump_last_update_field) that locks in this behavior. Finding 2 (per-tick queries): Agreed. keeping the ORM-only version for readability. Finding 3 (NULL window): Acknowledged, has no risk. Finding 4 (no_changes flag): Added clarifying comments at _disable(), the one-off branch, and next(). Finding 5 (admin guard): Wrapped all three actions with if rows_updated:. |
auvipy
left a comment
There was a problem hiding this comment.
I am reluctant on introducing yet another breaking change. any way to keep backward compatible?
@auvipy I hear the concern, I think the only way to keep backward compatibility and also keep the pr changes is to introduce a new config so people can change it in their settings and have these new optimizations, but I think a config adds complexity for an internal detail that shouldn't be user-facing. last_update was never a documented API — last_change() is the documented interface (and what Beat itself uses). The field still exists and still updates on deletes/bulk actions; it just isn't the sole source of truth anymore. |
yeah that would be acceptable |
Great! I'll update the PR to polish the upgrade notes/changelog to make the behavior change crystal clear |
|
please fix the newly raised merge conflicts |
Resolve conflict in ModelEntry.is_due() one-off disable by keeping upstream update_fields save (celery#1070) while preserving pull-based change-detection comments.
@auvipy I did resolve the conflicts. |
Summary
This PR refactors how django-celery-beat detects schedule changes for Celery Beat. The previous design updated a single singleton row (
PeriodicTasks,ident=1) on every insert, update, and delete via Django signals. Under high concurrency—especially withATOMIC_REQUESTS=Trueand long transactions—workers contended for an exclusive lock on that row, causing PostgreSQLlock_timeouterrors (issue #1000).The new design eliminates extra writes on the hot path (task/schedule saves) while keeping prompt delete detection and in-place schedule edit detection.
Root cause
PeriodicTasks.update_changed()usedupdate_or_create(), which in modern Django acquiresSELECT FOR UPDATEon the singleton row. Even replacing that with a plainUPDATEdoes not fully solve the problem: any row update holds a lock untilCOMMIT. When the signal runs inside a longatomic()block (e.g.ATOMIC_REQUESTS=True), all concurrent workers serialize on the same row.Solution
Pull-based detection for writes (no extra DB write)
Beat reads the latest change timestamp from existing
auto_nowfields:PeriodicTask.date_changed(already existed; now indexed)updated_atonIntervalSchedule,CrontabSchedule,SolarSchedule,ClockedSchedule(new, indexed)PeriodicTasks.last_change()returns:Inserts, updates, and in-place schedule edits advance one of the
MAX(...)values automatically—no singleton write.Singleton row repurposed for out-of-band changes only
It is bumped only when
auto_nowcannot help:MAXmay drop or stay unchanged)queryset.update()bypassesauto_now)Writes are deferred with
transaction.on_commit()so the lock is not held for the duration of the outer transaction.Signal wiring
PeriodicTasksavepre_save+ explicitsave()hookdate_changed)PeriodicTaskdeletepre_deletepost_deletepost_saveupdated_at)pre_delete/post_deleteCritical: beat housekeeping must not bump
date_changedModelEntry.save()now usesupdate_fields=['last_run_at', 'total_run_count']. A fullsave()would bumpdate_changedon every task run, andMAX(date_changed)would make Beat reload the schedule in an infinite loop. An inline comment documents this constraint; a regression test guards it.Migration
0020_clockedschedule_updated_at_and_more.py:updated_atto the four schedule modelsPeriodicTask.date_changedAlternatives considered
UPDATEwithoutSELECT FOR UPDATECOMMIT; does not fixATOMIC_REQUESTScontentiontransaction.on_commit()only (keep write on every save)COUNT(*)fingerprintO(n)on largePeriodicTasktables every Beat tickLISTEN/NOTIFYTrade-offs
last_change()runs several indexedMAX()queries per Beat tick (cheap vs. former lock storms);update_changed()is async viaon_commit(rolled-back transactions correctly skip the bump); admin bulk updates still require explicitupdate_changed()Test plan
last_change()viadate_changed(no marker bump)last_change()via marker (on_commitcapture)last_change()for all four schedule typesdate_changedor trigger reloadqueryset.update()+update_changed()advances markerdate_changedremains detectable (marker dominates)References