fix: use update_fields in ModelEntry.save() and related methods

[qizwiz]

Problem

ModelEntry.save() re-fetches the model from the database and copies exactly the fields in save_fields = ['last_run_at', 'total_run_count', 'no_changes'], but then calls obj.save() without update_fields. This issues a full-row UPDATE, overwriting every column with the values from the re-fetched object — including any concurrent changes made by another process (e.g. a task being edited in the admin while a beat tick is in flight).

The same issue exists in _disable() and the one-off task branch of is_due().

The comment on save() already says:

# Object may not be synchronized, so only
# change the fields we care about.

This PR makes that intent enforceable at the database level.

Fix

Three one-line changes in ModelEntry:

Method	Before	After
save()	obj.save()	obj.save(update_fields=self.save_fields)
_disable()	model.save()	model.save(update_fields=['no_changes', 'enabled'])
is_due() (one-off)	self.model.save()	self.model.save(update_fields=['enabled', 'total_run_count', 'no_changes'])

Impact

Without update_fields, every beat tick that processes a changed entry writes every column back to the database. In a multi-process Celery deployment (multiple beat instances, or beat + admin), this is a silent data-loss race: admin edits to name, task, args, kwargs, queue, etc. can be clobbered by a concurrent beat save.

Found by pact, a static analysis tool for Django/async constraint violations.

[auvipy]

without tests I am not sure about the changes

[qizwiz]

Thanks @auvipy — good call. I've added three regression tests in the latest commit:

• test_disable_passes_update_fields — asserts _disable() calls model.save(update_fields=['no_changes', 'enabled']) and not a full-model save
• test_one_off_expiry_passes_update_fields — asserts is_due() for an exhausted one-off task saves only ['enabled', 'total_run_count', 'no_changes']
• test_entry_save_passes_update_fields — asserts ModelEntry.save() forwards update_fields=self.save_fields to the DB object

Each test patches save on the relevant object and asserts the kwargs so a future regression (e.g. someone removing update_fields) will fail the suite immediately.

[cclauss]

t/unit/test_schedulers.py:568:89: E501 line too long (93 > 88 characters)
t/unit/test_schedulers.py:584:89: E501 line too long (97 > 88 characters)

[qizwiz]

Fixed — shortened both docstrings to fit within the 88-char limit.

[qizwiz]

Found and fixed the CI failure — sorry for the breakage.

Root cause: no_changes is a class-level Python attribute used as a signal sentinel (PeriodicTasks.changed() reads instance.no_changes to decide whether to update last_change). It is not a database column. Passing it in update_fields causes Django to raise ValueError: The following fields do not exist in this model: no_changes — which broke test_entry_and_model_last_run_at_with_utc_no_use_tz and would have broken production calls to _disable().

Fix: exclude no_changes from all three update_fields call sites:

• _disable(): update_fields=['enabled']
• is_due() one-off expiry: update_fields=['enabled', 'total_run_count']
• save(): filter no_changes out of save_fields before passing to update_fields

The attribute is still set on the instance before save() in all cases, so the signal handler continues to work correctly. Regression tests updated to assert the corrected field lists. All 4 tests pass locally (f1055f2).

[qizwiz]

Tests were added in commit f2182ea (test_disable_passes_update_fields, test_one_off_expiry_passes_update_fields, test_entry_save_passes_update_fields) — three focused regression tests that assert update_fields is passed at each save site.

The test suite found a genuine issue in our own fix: save(update_fields=...) raises ValueError: Cannot force an update in save() with no primary key when called on an in-memory PeriodicTask that hasn't been persisted yet. This happens in is_due() (one-off path) and _disable() when the unit tests construct model instances without saving them. Added if model.pk: guards so update_fields is used when the object exists in the DB and plain save() is used otherwise. Fixed in c372f43 — CI should be green now.

[codecov]

Codecov Report

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1038      +/-   ##
==========================================
+ Coverage   87.68%   87.70%   +0.02%     
==========================================
  Files          32       32              
  Lines        1015     1017       +2     
  Branches       81       81              
==========================================
+ 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.

[qizwiz]

Thanks @cclauss — those E501 violations were fixed in commit 1528f00 (style: shorten docstrings to satisfy E501). The current HEAD (c372f43) passes flake8 with 0 line-length issues in the test file.

[qizwiz]

Hi @auvipy — three tests are included in the diff at t/unit/test_schedulers.py:

1. test_disable_passes_update_fields — asserts _disable() calls save(update_fields=['enabled']), not a full save
2. test_one_off_expiry_passes_update_fields — asserts is_due() one-off expiry saves only ['enabled', 'total_run_count']
3. test_entry_save_passes_update_fields — asserts ModelEntry.save() forwards update_fields to the DB object

All 20 CI matrix jobs (Django 3.2–6.0, Python 3.9–3.14 + PyPy) pass. Let me know if you'd like a different style of test — happy to add an integration-level test that hits the actual database if that would be more convincing.

[justmobilize]

@qizwiz ,

It would be better to use:

If self.model._state.adding:
    update_fields = None
else:
    update_fields = ['enabled']
self.model.save(update_fields=update_fields)

1. there's nothing stopping a future developer from setting the pk before save.
2. setting the var update_fields allows the save method to be on the same line, which allows for better error reporting.

Definition:
https://github.com/django/django/blob/708482587943ee02e744ab030d70401d67f21da5/django/db/models/base.py#L457

Initial:
https://github.com/django/django/blob/stable/5.2.x/django/db/models/base.py#L474

When loaded from db:
https://github.com/django/django/blob/708482587943ee02e744ab030d70401d67f21da5/django/db/models/base.py#L585

And save:
https://github.com/django/django/blob/708482587943ee02e744ab030d70401d67f21da5/django/db/models/base.py#L1019

Note: this is my personal opinion, this isn't a project I maintain, just use heavily.

[qizwiz]

Good point — _state.adding is more correct for exactly the reason you describe: a future developer could set pk manually before the first save, and our if model.pk: guard wouldn't catch that case. I'll update all three call sites to use the _state.adding pattern. Thanks for the reference links.

[justmobilize]

Your quite welcome

[qizwiz]

Hi @auvipy — happy to address any remaining concerns. The current PR has:

• Tests for all three fix sites (test_disable_passes_update_fields, test_one_off_expiry_passes_update_fields, test_entry_save_passes_update_fields)
• CI passing (codecov at 87.73%)
• E501 and flake8 issues resolved
• no_changes correctly excluded from update_fields (it's a Python sentinel, not a DB column)

Would love your eyes on it when you get a chance. Thanks!

[qizwiz]

Hi @auvipy — following up. The current PR includes:

• Fix: three save() calls narrowed to update_fields (avoids full model save)
• Tests: test_disable_passes_update_fields, test_one_off_expiry_passes_update_fields, test_entry_save_passes_update_fields — all at t/unit/test_schedulers.py
• Style: E501 violations fixed in test docstrings

Is there anything else you'd like to see before this can move forward? Happy to adjust the approach or add more coverage.

[qizwiz]

Updated — both _disable() and the one-off path in is_due() now use _state.adding (commit e035eaf):

# _disable()
update_fields = None if model._state.adding else ['enabled']
model.save(update_fields=update_fields)

# is_due() one-off path
update_fields = (
    None if self.model._state.adding
    else ['enabled', 'total_run_count']
)
self.model.save(update_fields=update_fields)

Tests updated to use m._state.adding = False to simulate a persisted instance. Thanks @justmobilize for the correct pattern.

[Copilot]

Pull request overview

This PR aims to prevent Celery beat’s ModelEntry from issuing full-row UPDATEs when it only intends to persist run-metadata/disable-state, reducing the risk of clobbering concurrent admin edits.

Changes:

• Use update_fields in ModelEntry.save() to update only run-metadata columns.
• Use update_fields in _disable() and one-off expiry handling in is_due().
• Add unit tests asserting the update_fields behavior; register the custom celery pytest marker.

Reviewed changes

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

File	Description
django_celery_beat/schedulers.py	Restricts DB writes to specific fields via update_fields in ModelEntry save/disable paths.
t/unit/test_schedulers.py	Adds tests asserting scheduler save/disable calls pass update_fields.
setup.cfg	Registers the celery pytest marker used by the test suite.

---

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

[auvipy]

@qizwiz can you please cross check the suggestions?

[auvipy]

also fix the merge conflicts