From 93508c2e58e95f4489a643526ab6a47127d1ccd1 Mon Sep 17 00:00:00 2001 From: SAMurai-16 Date: Wed, 28 Jan 2026 15:05:07 +0000 Subject: [PATCH 1/2] [connection] Fix update_config task incorrectly detecting itself as duplicate #1204 The _is_update_in_progress function was incorrectly returning True when the only active task was the current task itself, causing the worker to skip execution thinking another worker was handling it. Fixed by: - Adding current_task_id parameter to _is_update_in_progress() - Using bind=True on update_config task to access self.request.id - Excluding the current task when checking for active duplicates - Added unit tests to verify the fix Fixes #1204 --- openwisp_controller/connection/tasks.py | 11 +- .../connection/tests/test_tasks.py | 101 ++++++++++++++++++ 2 files changed, 108 insertions(+), 4 deletions(-) diff --git a/openwisp_controller/connection/tasks.py b/openwisp_controller/connection/tasks.py index d75bbde20..fb9f94fa2 100644 --- a/openwisp_controller/connection/tasks.py +++ b/openwisp_controller/connection/tasks.py @@ -16,20 +16,23 @@ _TASK_NAME = "openwisp_controller.connection.tasks.update_config" -def _is_update_in_progress(device_id): +def _is_update_in_progress(device_id, current_task_id=None): active = current_app.control.inspect().active() if not active: return False # check if there's any other running task before adding it for task_list in active.values(): for task in task_list: + # skip the current task itself + if current_task_id and task["id"] == current_task_id: + continue if task["name"] == _TASK_NAME and str(device_id) in task["args"]: return True return False -@shared_task -def update_config(device_id): +@shared_task(bind=True) +def update_config(self, device_id): """ Launches the ``update_config()`` operation of a specific device in the background @@ -48,7 +51,7 @@ def update_config(device_id): except ObjectDoesNotExist as e: logger.warning(f'update_config("{device_id}") failed: {e}') return - if _is_update_in_progress(device_id): + if _is_update_in_progress(device_id, current_task_id=self.request.id): return try: device_conn = DeviceConnection.get_working_connection(device) diff --git a/openwisp_controller/connection/tests/test_tasks.py b/openwisp_controller/connection/tests/test_tasks.py index 700dbbf32..a8bc7d22b 100644 --- a/openwisp_controller/connection/tests/test_tasks.py +++ b/openwisp_controller/connection/tests/test_tasks.py @@ -9,6 +9,7 @@ from ...config.tests.test_controller import TestRegistrationMixin from .. import tasks +from ..tasks import _TASK_NAME, _is_update_in_progress from .utils import CreateConnectionsMixin Command = load_model("connection", "Command") @@ -89,6 +90,106 @@ def test_launch_command_exception(self, *args): self.assertEqual(command.output, "Internal system error: test error\n") +class TestIsUpdateInProgress(CreateConnectionsMixin, TestCase): + + def _get_mocked_active_tasks(self, device_id, task_id="task-123"): + return { + "celery@worker1": [ + { + "id": task_id, + "name": _TASK_NAME, + "args": f"('{device_id}',)", + } + ] + } + + @mock.patch("openwisp_controller.connection.tasks.current_app") + def test_is_update_in_progress_without_current_task_id(self, mock_app): + + device_id = uuid.uuid4() + current_task_id = "task-123" + + mock_app.control.inspect.return_value.active.return_value = ( + self._get_mocked_active_tasks(device_id, task_id=current_task_id) + ) + + # BUG: Without passing current_task_id, the function returns True + # even though the only active task IS the current task + result = _is_update_in_progress(device_id) + self.assertTrue( + result, + ) + + @mock.patch("openwisp_controller.connection.tasks.current_app") + def test_is_update_in_progress_with_current_task_id_excluded(self, mock_app): + + device_id = uuid.uuid4() + current_task_id = "task-123" + + mock_app.control.inspect.return_value.active.return_value = ( + self._get_mocked_active_tasks(device_id, task_id=current_task_id) + ) + + # FIX: With current_task_id provided, the function correctly returns False + result = _is_update_in_progress(device_id, current_task_id=current_task_id) + self.assertFalse( + result, + ) + + @mock.patch("openwisp_controller.connection.tasks.current_app") + def test_is_update_in_progress_detects_another_task(self, mock_app): + + device_id = uuid.uuid4() + current_task_id = "task-123" + another_task_id = "task-456" + + # Mock active tasks with both current task and another task + mock_app.control.inspect.return_value.active.return_value = { + "celery@worker1": [ + { + "id": current_task_id, + "name": _TASK_NAME, + "args": f"('{device_id}',)", + }, + { + "id": another_task_id, + "name": _TASK_NAME, + "args": f"('{device_id}',)", + }, + ] + } + + # Should return True because another task IS running + result = _is_update_in_progress(device_id, current_task_id=current_task_id) + self.assertTrue( + result, + ) + + @mock.patch("openwisp_controller.connection.tasks.current_app") + def test_is_update_in_progress_no_active_tasks(self, mock_app): + + device_id = uuid.uuid4() + mock_app.control.inspect.return_value.active.return_value = None + + result = _is_update_in_progress(device_id, current_task_id="task-123") + self.assertFalse(result) + + @mock.patch("openwisp_controller.connection.tasks.current_app") + def test_is_update_in_progress_different_device(self, mock_app): + + device_id = uuid.uuid4() + other_device_id = uuid.uuid4() + + mock_app.control.inspect.return_value.active.return_value = ( + self._get_mocked_active_tasks(other_device_id, task_id="task-456") + ) + + result = _is_update_in_progress(device_id, current_task_id="task-123") + self.assertFalse( + result, + ) + + class TestTransactionTasks( TestRegistrationMixin, CreateConnectionsMixin, TransactionTestCase ): From 010ba62da2cc7d787b8847e45214f26c74cf1531 Mon Sep 17 00:00:00 2001 From: SAMurai-16 Date: Wed, 28 Jan 2026 15:28:42 +0000 Subject: [PATCH 2/2] [connection] Fix update_config task incorrectly detecting itself as duplicate #1204 The _is_update_in_progress function was incorrectly returning True when the only active task was the current task itself, causing the worker to skip execution thinking another worker was handling it. Fixed by: - Adding current_task_id parameter to _is_update_in_progress() - Using bind=True on update_config task to access self.request.id - Excluding the current task when checking for active duplicates - Added unit tests to verify the fix Fixes #1204 --- openwisp_controller/connection/tasks.py | 6 ++++-- .../connection/tests/test_tasks.py | 16 ++++------------ 2 files changed, 8 insertions(+), 14 deletions(-) diff --git a/openwisp_controller/connection/tasks.py b/openwisp_controller/connection/tasks.py index fb9f94fa2..886bfd9a4 100644 --- a/openwisp_controller/connection/tasks.py +++ b/openwisp_controller/connection/tasks.py @@ -24,9 +24,11 @@ def _is_update_in_progress(device_id, current_task_id=None): for task_list in active.values(): for task in task_list: # skip the current task itself - if current_task_id and task["id"] == current_task_id: + if current_task_id and task.get("id") == current_task_id: continue - if task["name"] == _TASK_NAME and str(device_id) in task["args"]: + if task.get("name") == _TASK_NAME and str(device_id) in task.get( + "args", "" + ): return True return False diff --git a/openwisp_controller/connection/tests/test_tasks.py b/openwisp_controller/connection/tests/test_tasks.py index a8bc7d22b..0bda66c4b 100644 --- a/openwisp_controller/connection/tests/test_tasks.py +++ b/openwisp_controller/connection/tests/test_tasks.py @@ -116,9 +116,7 @@ def test_is_update_in_progress_without_current_task_id(self, mock_app): # BUG: Without passing current_task_id, the function returns True # even though the only active task IS the current task result = _is_update_in_progress(device_id) - self.assertTrue( - result, - ) + self.assertTrue(result) @mock.patch("openwisp_controller.connection.tasks.current_app") def test_is_update_in_progress_with_current_task_id_excluded(self, mock_app): @@ -132,9 +130,7 @@ def test_is_update_in_progress_with_current_task_id_excluded(self, mock_app): # FIX: With current_task_id provided, the function correctly returns False result = _is_update_in_progress(device_id, current_task_id=current_task_id) - self.assertFalse( - result, - ) + self.assertFalse(result) @mock.patch("openwisp_controller.connection.tasks.current_app") def test_is_update_in_progress_detects_another_task(self, mock_app): @@ -161,9 +157,7 @@ def test_is_update_in_progress_detects_another_task(self, mock_app): # Should return True because another task IS running result = _is_update_in_progress(device_id, current_task_id=current_task_id) - self.assertTrue( - result, - ) + self.assertTrue(result) @mock.patch("openwisp_controller.connection.tasks.current_app") def test_is_update_in_progress_no_active_tasks(self, mock_app): @@ -185,9 +179,7 @@ def test_is_update_in_progress_different_device(self, mock_app): ) result = _is_update_in_progress(device_id, current_task_id="task-123") - self.assertFalse( - result, - ) + self.assertFalse(result) class TestTransactionTasks(