Closes 20817 fix datasource sync broken when cron is set #20837

ifoughal · 2025-11-19T19:07:58Z

Fixes: #20817

Datasources sync was broken when the user sets a sync interval.

The fix introduces 2 new states:

Ready
Scheduled

Ready state is set when the data-source object is not new. ie: either updated or changed state recently.
Scheduled is set when the user sets a sync interval.

This PR includes the following logic enhancements:

by removing DataSourceStatusChoices.QUEUED from the ready_for_sync method, we let the user sync the data-source anytime even when a daily interval is set, as long as the data-source is enabled.
Include the 2 new states (Scheduled, Ready) in the clean & save properties of data-source form model, to set the status correctly depending on the user inputs/
- If the user creates a new data-source object, then the state is new.
- if the user update the object with a defined interval, then the states changes to scheduled
- if the user removed the sync interval, then the state changes to Ready.
- if a sync is completed either successfully or with a failure, then the status will get update with the job's status as before without impact.

This reverts commit b4160ad.

jnovinger · 2025-11-20T07:29:53Z

@ifoughal, CI appears to be failing with a test failure.

ifoughal · 2025-11-20T09:09:18Z

@jnovinger

Initially I thought there was an issue with the view rendering class, but after some further TS, and checking the actual value of status on the DB entry, it seems the status field got overwritten somehow.

 id |            created            |         last_updated         | custom_field_data | description | comments | name | type |                           source_url                            | status | enabled | ignore_rules |                   parameters                   | last_synced | sync_interval 
----+-------------------------------+------------------------------+-------------------+-------------+----------+------+------+-----------------------------------------------------------------+--------+---------+--------------+------------------------------------------------+-------------+---------------
 10 | 2025-11-20 10:02:12.763951+01 | 2025-11-20 10:02:19.22684+01 | {}                |             |          | test | git  | https://gitlab.pic.services.prod/orion/IaC/config-templates.git | queued | t       |              | {"branch": "", "password": "", "username": ""} |             |           720

To circumvent this, I run the instance.save() method in the save override of dataSourcesForm. but this generates 2 commits and breaks the test. I'll continue looking into a fix.

ifoughal · 2025-11-20T09:46:40Z

@jnovinger

The issue was coming from :

@classmethod
    def enqueue(cls, *args, **kwargs):
        job = super().enqueue(*args, **kwargs)

        # Update the DataSource's synchronization status to queued
        if datasource := job.object:
            datasource.status = DataSourceStatusChoices.QUEUED
            DataSource.objects.filter(pk=datasource.pk).update(status=datasource.status)

        return job

The enqueue method was overriding the state, I have removed datasource.status = DataSourceStatusChoices.QUEUED, the state should then be updated on the clean() method of the post of the datasource form.

arthanson

@ifoughal Is QUEUED status used anymore? I don't see it ever getting set anywhere.

The QUEUED state was that it was submitted to be run (so pending being run). I think this is should still be a valid status, i.e. get set when it is pending being run (enqueued) and cleared once it is run.

The description for the PR says that two new states are added, but only one is added here, is the description old?

netbox/core/jobs.py

netbox/core/models/data.py

…-sync-broken-when-cron-is-set

ifoughal · 2025-11-26T07:58:11Z

@ifoughal Is QUEUED status used anymore? I don't see it ever getting set anywhere.

The QUEUED state was that it was submitted to be run (so pending being run). I think this is should still be a valid status, i.e. get set when it is pending being run (enqueued) and cleared once it is run.

The description for the PR says that two new states are added, but only one is added here, is the description old?

I will be reverting the removed of queued as it was indeed needed for when the rq workers as busy. It should then reflect the queued state.

The two new states are READY & SCHEDULED.
Ready is used when the datasource object is not NEW, meaning if the user removes the schedule, so when it changes from interval to None, then the state becomes Ready (ready for sync).
as for scheduled, as its name suggests, is used when the user sets a schedule for the datasource object.

…t' of https://github.com/ifoughal/netbox into closes-20817-Fix-datasource-sync-broken-when-cron-is-set

…-sync-broken-when-cron-is-set

arthanson · 2025-12-04T19:27:21Z

netbox/core/choices.py

        (SYNCING, _('Syncing'), 'cyan'),
        (COMPLETED, _('Completed'), 'green'),
        (FAILED, _('Failed'), 'red'),
+        (READY, _('Ready'), 'green'),


You say SCHEDULED status is added but there is no scheduled in the status or code anywhere? Still confused as you state "The two new states are READY & SCHEDULED."

I ended up removing it, so there is only one state that has been added in the end, which is the READY state.

The reasoning behind was that scheduled would interfere with the post sync action trigger status (completed/failed/syncing). Therefore, I decided to keep the ready state only, which has the same effect as new, the only difference is that new is only applied when the object gets created, whereas ready is applied when there there is no effective sync interval.

netbox/core/forms/model_forms.py

arthanson · 2025-12-04T19:38:08Z

netbox/core/models/data.py

-            DataSourceStatusChoices.QUEUED,
-            DataSourceStatusChoices.SYNCING
-        )
+        return self.enabled and self.status != DataSourceStatusChoices.SYNCING


Question: If we are adding the READY state and other changes here do we still need to skip QUEUED check here? Just wondering if your other changes will allow it to be kept as I think if it is actually queued and hasn't finished we probably don't want to allow sync again?

that's why I had initially added the scheduled state initially, so that when it's queued, I would disable the syncing button, but allow it only when the state is scheduled. The issue was that scheduled was completely redundant with Queued. As they could be set only during the save action of the data-source object. They would also get overwritten when the job finishes. so queued, just as before reflects when the job is queued when the user hits sync, but also when the sync_interval is set.

The proper way to fix it would be to add a new field, for example last-status and use that for the latest job status.

keep the current status field, and use it only for the workers Q state (new, ready, queued) and maybe re-add the scheduled state as well.

what do you think?

…-sync-broken-when-cron-is-set

Copilot

Pull request overview

This PR attempts to fix an issue where DataSource synchronization was broken when a sync interval (cron schedule) was set. The proposed solution introduces a new READY status and modifies the ready_for_sync property to allow manual syncing even when a recurring sync job is queued.

Key Changes

Removes QUEUED status from the ready_for_sync check to allow manual sync triggers
Adds READY status to DataSourceStatusChoices
Implements form-level status management in DataSourceForm's clean() and save() methods

Reviewed changes

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

File	Description
netbox/core/models/data.py	Modified `ready_for_sync` property to exclude only SYNCING status instead of both QUEUED and SYNCING
netbox/core/forms/model_forms.py	Added status management logic in form's clean() and save() methods to set status based on instance state and sync_interval
netbox/core/choices.py	Added new READY status choice to DataSourceStatusChoices

Critical Issues Identified:

The implementation has several fundamental design flaws that need to be addressed:

Missing SCHEDULED Status: The PR description mentions introducing both "Ready" and "Scheduled" states, but only READY is implemented. The logic to set status to SCHEDULED when sync_interval is configured is missing.
Status Field Design Violation: The DataSource.status field is marked as editable=False, indicating it should be managed programmatically by the sync system, not by forms. The current implementation has the form directly manipulating this field, which conflicts with the intended API design.
Status Override Issue: The form unconditionally overwrites the DataSource status on every save, which will destroy sync operation results (COMPLETED/FAILED) set by the sync process. This breaks the ability to track sync history.
Race Condition Risk: Removing QUEUED from ready_for_sync could allow multiple simultaneous sync operations since QUEUED is set by SyncDataSourceJob.enqueue() before the job runs.
Signal Interference: The form's status changes conflict with the enqueue_sync_job signal handler that sets status to QUEUED when a sync job is enqueued.
Implementation Logic Errors: The form checks self.instance.pk which will always be truthy for updates (should check if pk is None), and uses raw self.data instead of self.cleaned_data.

The core issue appears to be that the fix tries to solve a coordination problem between recurring scheduled syncs and manual syncs by manipulating status at the form level, when this should be handled by the job scheduling system and sync operations themselves. A better approach would be to keep status management entirely within the sync process and job system, and use the existing QUEUED/SYNCING/COMPLETED/FAILED states appropriately.

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

Copilot · 2025-12-05T14:03:00Z

netbox/core/forms/model_forms.py

+        else:
+            if not self.data.get('sync_interval'):
+                self.cleaned_data['status'] = DataSourceStatusChoices.READY



The logic for setting status based on sync_interval is incomplete. According to the PR description, when a user sets a sync interval, the status should be set to "Scheduled". However, this code only handles the case when sync_interval is removed (setting to READY), but doesn't set the status to SCHEDULED when sync_interval is present. There's also no SCHEDULED state defined, so you should add an else branch to handle when sync_interval is set (e.g., set status to QUEUED or implement SCHEDULED).

Suggested change

else:

self.cleaned_data['status'] = DataSourceStatusChoices.QUEUED

Copilot · 2025-12-05T14:03:00Z

netbox/core/forms/model_forms.py

+        if not self.instance.pk:
+            self.cleaned_data['status'] = DataSourceStatusChoices.NEW
+        else:
+            if not self.data.get('sync_interval'):


Using self.data.get('sync_interval') directly accesses raw form data instead of the cleaned and validated data. This could lead to inconsistencies since self.data contains the raw POST/GET data (strings), while self.cleaned_data['sync_interval'] contains the properly validated and converted value. Use self.cleaned_data.get('sync_interval') instead for consistency and reliability.

Suggested change

if not self.data.get('sync_interval'):

if not self.cleaned_data.get('sync_interval'):

Copilot · 2025-12-05T14:03:01Z

netbox/core/forms/model_forms.py

+    def clean(self):
+        super().clean()
+        if not self.instance.pk:
+            self.cleaned_data['status'] = DataSourceStatusChoices.NEW
+        else:
+            if not self.data.get('sync_interval'):
+                self.cleaned_data['status'] = DataSourceStatusChoices.READY

+    def save(self, *args, **kwargs):
        parameters = {}
        for name in self.fields:
            if name.startswith('backend_'):
                parameters[name[8:]] = self.cleaned_data[name]
        self.instance.parameters = parameters

+        # update status
+        self.instance.status = self.cleaned_data.get('status', self.instance.status)
+
        return super().save(*args, **kwargs)


The status modification in clean() will interfere with the enqueue_sync_job signal handler in signals.py (line 259-275). When saving a DataSource with a sync_interval, the sequence will be: 1) Form sets status to READY (or NEW), 2) Form saves the instance, 3) The post_save signal fires and enqueues a job, 4) The SyncDataSourceJob.enqueue() sets status to QUEUED. This creates an inconsistent state where the form's status is immediately overwritten. The form should not manage status at all, or should coordinate with the signal/job system.

Copilot · 2025-12-05T14:03:01Z

netbox/core/forms/model_forms.py

+    def clean(self):
+        super().clean()
+        if not self.instance.pk:
+            self.cleaned_data['status'] = DataSourceStatusChoices.NEW
+        else:
+            if not self.data.get('sync_interval'):
+                self.cleaned_data['status'] = DataSourceStatusChoices.READY

+    def save(self, *args, **kwargs):
        parameters = {}
        for name in self.fields:
            if name.startswith('backend_'):
                parameters[name[8:]] = self.cleaned_data[name]
        self.instance.parameters = parameters

+        # update status
+        self.instance.status = self.cleaned_data.get('status', self.instance.status)
+
        return super().save(*args, **kwargs)


The DataSource.status field is marked as editable=False in the model (data.py line 56), indicating it's intended to be managed programmatically, not by user input. Having the form manipulate this field directly violates this design principle. The status should be managed exclusively by the sync process (DataSource.sync(), SyncDataSourceJob) and the signal handlers, not by the form. This creates a conflict between the intended API design and the form implementation.

Copilot · 2025-12-05T14:03:01Z

netbox/core/choices.py

        (SYNCING, _('Syncing'), 'cyan'),
        (COMPLETED, _('Completed'), 'green'),
        (FAILED, _('Failed'), 'red'),
+        (READY, _('Ready'), 'green'),


The READY status uses the same color ('green') as COMPLETED. This could be confusing for users since these statuses represent different states: COMPLETED means a sync operation finished successfully, while READY means the DataSource is ready to be synced but hasn't completed a sync yet. Consider using a different color like 'blue' or 'gray' for READY to better differentiate it visually from COMPLETED.

Suggested change

(READY, _('Ready'), 'green'),

(READY, _('Ready'), 'gray'),

Copilot · 2025-12-05T14:03:02Z

netbox/core/forms/model_forms.py

        self.instance.parameters = parameters

+        # update status
+        self.instance.status = self.cleaned_data.get('status', self.instance.status)


This logic unconditionally overwrites the DataSource status from the form's cleaned_data. This will break the intended behavior where sync operations set the status to SYNCING, COMPLETED, or FAILED (as seen in DataSource.sync() method and SyncDataSourceJob). After a sync completes and sets status to COMPLETED or FAILED, if a user edits the DataSource, this form will reset the status to READY or NEW, losing the sync result information. The form should only update the status when explicitly changing sync_interval settings, not on every save.

Copilot · 2025-12-05T14:03:02Z

netbox/core/models/data.py

-            DataSourceStatusChoices.QUEUED,
-            DataSourceStatusChoices.SYNCING
-        )
+        return self.enabled and self.status != DataSourceStatusChoices.SYNCING


While removing QUEUED from the ready_for_sync check allows users to manually trigger syncs even when a recurring job is queued, this change could allow multiple simultaneous sync operations for the same DataSource. The SyncDataSourceJob.enqueue() method sets status to QUEUED before the job runs (in jobs.py line 34). If a user manually triggers a sync while a job is queued but not yet running, both could execute. Consider whether QUEUED should be excluded or if additional logic is needed to prevent race conditions.

Suggested change

return self.enabled and self.status != DataSourceStatusChoices.SYNCING

return self.enabled and self.status not in (

DataSourceStatusChoices.SYNCING,

DataSourceStatusChoices.QUEUED,

)

Copilot · 2025-12-05T14:03:02Z

netbox/core/models/data.py

-            DataSourceStatusChoices.QUEUED,
-            DataSourceStatusChoices.SYNCING
-        )
+        return self.enabled and self.status != DataSourceStatusChoices.SYNCING


The modified ready_for_sync logic lacks test coverage. Given that this property is critical for determining when a DataSource can be synced and is a key part of the bug fix, tests should be added to verify the behavior with different status values (NEW, QUEUED, READY, COMPLETED, FAILED) and the enabled flag. Consider adding tests to netbox/core/tests/test_models.py to cover this property.

Copilot · 2025-12-05T14:03:03Z

netbox/core/forms/model_forms.py

+    def clean(self):
+        super().clean()
+        if not self.instance.pk:
+            self.cleaned_data['status'] = DataSourceStatusChoices.NEW
+        else:
+            if not self.data.get('sync_interval'):
+                self.cleaned_data['status'] = DataSourceStatusChoices.READY

+    def save(self, *args, **kwargs):
        parameters = {}
        for name in self.fields:
            if name.startswith('backend_'):
                parameters[name[8:]] = self.cleaned_data[name]
        self.instance.parameters = parameters

+        # update status
+        self.instance.status = self.cleaned_data.get('status', self.instance.status)
+
        return super().save(*args, **kwargs)


The new status transition logic in the form's clean() and save() methods lacks test coverage. Tests should be added to verify that: 1) New DataSources get status=NEW, 2) Updating an existing DataSource with sync_interval removed sets status=READY, 3) Updating an existing DataSource with sync_interval set behaves correctly, and 4) Status from completed sync operations (COMPLETED/FAILED) is preserved appropriately. Consider adding tests to verify this form behavior.

Copilot · 2025-12-05T14:03:03Z

netbox/core/choices.py

    SYNCING = 'syncing'
    COMPLETED = 'completed'
    FAILED = 'failed'
+    READY = 'ready'


The PR description mentions introducing two new states: "Ready" and "Scheduled". However, only the READY state is added here. The SCHEDULED state is missing from the DataSourceStatusChoices. This means the logic described in the PR description for setting status to "Scheduled" when a user sets a sync interval cannot be implemented.

…et' of https://github.com/ifoughal/netbox into closes-20817-Fix-datasource-sync-broken-when-cron-is-set

ifoughal added 6 commits November 19, 2025 18:49

Feat: set status as editable field

b4160ad

Feat: added clean method to set data-source state to Ready or scheduled

111aca1

Feat: added status update during save method of DataSourceForm

bfeba36

Feat: added 2 states for DataSourceStatusChoices

2e0ff04

Feat: removed QUEUED from ready for sync condition

a49869a

Revert "Feat: set status as editable field"

5b5b5c8

This reverts commit b4160ad.

ifoughal added 3 commits November 20, 2025 10:50

Fix: removed status update from the enqueue method

e11508d

Feat: removed SCHEDULED choice due to redundency with sync interval

71f707b

Feat: reworked status update logic

da4c669

jeremystretch requested review from a team and arthanson and removed request for a team November 20, 2025 14:24

arthanson requested changes Nov 25, 2025

View reviewed changes

netbox/core/jobs.py Show resolved Hide resolved

netbox/core/models/data.py Outdated Show resolved Hide resolved

ifoughal added 2 commits November 26, 2025 08:05

style: use != instead of not in for single SYNCING check

57b47dc

Merge branch 'netbox-community:main' into closes-20817-Fix-datasource…

3016b1d

…-sync-broken-when-cron-is-set

ifoughal added 3 commits November 26, 2025 09:00

revert: re-added queued status set for datasource object

e4b6140

Merge branch 'closes-20817-Fix-datasource-sync-broken-when-cron-is-se…

929d024

…t' of https://github.com/ifoughal/netbox into closes-20817-Fix-datasource-sync-broken-when-cron-is-set

Merge branch 'netbox-community:main' into closes-20817-Fix-datasource…

93e5f91

…-sync-broken-when-cron-is-set

jeremystretch requested a review from arthanson December 3, 2025 18:00

arthanson requested changes Dec 4, 2025

View reviewed changes

Merge branch 'netbox-community:main' into closes-20817-Fix-datasource…

09d1049

…-sync-broken-when-cron-is-set

Copilot AI review requested due to automatic review settings December 5, 2025 13:51

Copilot started reviewing on behalf of ifoughal December 5, 2025 13:52 View session

Copilot finished reviewing on behalf of ifoughal December 5, 2025 14:01

Copilot AI reviewed Dec 5, 2025

View reviewed changes

refactor: moved status update logic from clean() to save() method

77ee6ba

ifoughal added 2 commits December 5, 2025 15:23

XMerge branch 'closes-20817-Fix-datasource-sync-broken-when-cron-is-s…

544c97d

…et' of https://github.com/ifoughal/netbox into closes-20817-Fix-datasource-sync-broken-when-cron-is-set

Style: removed comment

cf16a29


	else:
	self.cleaned_data['status'] = DataSourceStatusChoices.QUEUED

	if not self.data.get('sync_interval'):
	if not self.cleaned_data.get('sync_interval'):

Closes 20817 fix datasource sync broken when cron is set #20837

Are you sure you want to change the base?

Closes 20817 fix datasource sync broken when cron is set #20837

Conversation

ifoughal commented Nov 19, 2025

Fixes: #20817

Uh oh!

jnovinger commented Nov 20, 2025

Uh oh!

ifoughal commented Nov 20, 2025

Uh oh!

ifoughal commented Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arthanson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ifoughal commented Nov 26, 2025

Uh oh!

arthanson Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

ifoughal Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

arthanson Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

ifoughal Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Key Changes

Reviewed changes

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ifoughal commented Nov 20, 2025 •

edited

Loading