[connection] Fix update_config task incorrectly detecting itself as d… #1206

SAMurai-16 · 2026-01-28T15:09:16Z

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

Checklist

I have read the OpenWISP Contributing Guidelines.
I have manually tested the changes proposed in this pull request.
I have written new test cases for new code and/or updated existing tests for changes to existing code.
I have updated the documentation.

Reference to Existing Issue

Closes #1204 .

…uplicate openwisp#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 openwisp#1204

coderabbitai · 2026-01-28T15:09:35Z

📝 Walkthrough

Walkthrough

The PR fixes a Celery task self-detection bug in update_config. _is_update_in_progress now accepts an optional current_task_id to exclude the caller when scanning active Celery tasks. update_config is converted to a bound task (bind=True) so it passes self.request.id to the checker and avoids false-positive detection of itself. Tests were added to validate _is_update_in_progress behavior across scenarios (excluding current task, detecting other tasks for the same device, different-device cases, and no active tasks).

Sequence Diagram(s)

sequenceDiagram
    participant API as Client/API
    participant Broker as Celery Broker
    participant Worker as Celery Worker
    participant Inspector as current_app.control.inspect
    participant DB as Database

    API->>Broker: enqueue update_config(device_id)
    Broker->>Worker: deliver task
    Worker->>Inspector: active() -> list of active tasks
    Inspector-->>Worker: active tasks list
    Worker->>Worker: _is_update_in_progress(device_id, current_task_id=self.request.id)
    alt another worker running same device
        Worker-->>Worker: returns True (skip execution)
    else no other worker running same device
        Worker->>DB: apply configuration update
        DB-->>Worker: ack
    end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The pull request title partially describes a real change—fixing update_config task incorrectly detecting itself—but is incomplete/truncated (ends with 'd…'), making it vague about what is being detected and difficult to understand without additional context.	Complete the truncated title to clearly describe the specific issue, e.g., 'Fix update_config task incorrectly detecting itself as a duplicate task'.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description check	✅ Passed	The description adequately explains the bug, lists implemented fixes, references the linked issue, and confirms tests were added; the checklist is mostly complete with only documentation marked incomplete.
Linked Issues check	✅ Passed	The code changes directly address issue `#1204` by excluding the current task from duplicate detection via current_task_id parameter and bind=True binding, and unit tests verify the fix works correctly.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to fixing the reported issue: modifications to _is_update_in_progress and update_config task logic, plus comprehensive unit tests validating the fix.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

📝 Generate docstrings

🧹 Recent nitpick comments

openwisp_controller/connection/tests/test_tasks.py (1)
93-93: Consider removing unused mixin inheritance.

TestIsUpdateInProgress inherits from CreateConnectionsMixin but doesn't use any of its methods—all tests use uuid.uuid4() for device IDs and mock Celery internals. Inheriting directly from TestCase would simplify the test class and avoid unnecessary setup overhead from the mixin.
Suggested change
-class TestIsUpdateInProgress(CreateConnectionsMixin, TestCase):
+class TestIsUpdateInProgress(TestCase):

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cca1aa2 and 010ba62.

📒 Files selected for processing (2)

openwisp_controller/connection/tasks.py
openwisp_controller/connection/tests/test_tasks.py

🚧 Files skipped from review as they are similar to previous changes (1)

openwisp_controller/connection/tasks.py

🧰 Additional context used

🧠 Learnings (2)

📚 Learning: 2026-01-15T15:05:49.557Z

Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.

Applied to files:

openwisp_controller/connection/tests/test_tasks.py

📚 Learning: 2026-01-15T15:07:17.354Z

Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.

Applied to files:

openwisp_controller/connection/tests/test_tasks.py

🧬 Code graph analysis (1)

openwisp_controller/connection/tests/test_tasks.py (2)

openwisp_controller/connection/tasks.py (1)

_is_update_in_progress (19-33)

openwisp_controller/connection/tests/utils.py (1)

CreateConnectionsMixin (31-119)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)

GitHub Check: Python==3.10 | django~=5.1.0
GitHub Check: Python==3.13 | django~=5.2.0
GitHub Check: Python==3.11 | django~=4.2.0
GitHub Check: Python==3.12 | django~=5.2.0
GitHub Check: Python==3.11 | django~=5.2.0
GitHub Check: Python==3.10 | django~=5.2.0
GitHub Check: Python==3.13 | django~=5.1.0
GitHub Check: Python==3.12 | django~=5.1.0
GitHub Check: Python==3.11 | django~=5.1.0
GitHub Check: Python==3.12 | django~=4.2.0
GitHub Check: Python==3.10 | django~=4.2.0

🔇 Additional comments (3)

openwisp_controller/connection/tests/test_tasks.py (3)

12-12: LGTM!

The imports are correctly added to access the internal function and constant needed for testing the fix.

95-104: LGTM!

The helper method cleanly encapsulates the mock task structure, matching the format returned by Celery's inspect().active().

106-182: Well-structured test coverage for the fix.

The tests comprehensively cover the key scenarios:

The original bug behavior (without current_task_id)

The fix behavior (excluding current task)

Detection of genuinely concurrent tasks

Edge cases (no active tasks, different device)

The inline comments documenting the bug/fix context are helpful for understanding the PR's purpose.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Copilot

Pull request overview

This PR fixes a critical bug where the update_config Celery task incorrectly detected itself as another running task, causing it to skip execution. The fix adds task binding and current task ID exclusion logic to properly differentiate between the current task and other running tasks.

Changes:

Modified _is_update_in_progress() to accept an optional current_task_id parameter and skip the current task when checking for active duplicate tasks
Updated update_config task to use bind=True decorator and pass self.request.id to _is_update_in_progress()
Added comprehensive unit tests covering the bug scenario, the fix, and various edge cases

Reviewed changes

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

File	Description
openwisp_controller/connection/tasks.py	Adds `bind=True` to task decorator, adds `current_task_id` parameter to `_is_update_in_progress()`, and implements logic to skip current task when checking for duplicates
openwisp_controller/connection/tests/test_tasks.py	Adds new test class `TestIsUpdateInProgress` with 5 comprehensive test cases covering the bug, fix, and edge cases

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

Copilot · 2026-01-28T15:13:45Z

openwisp_controller/connection/tests/test_tasks.py

+        self.assertTrue(
+            result,
+        )


The assertion has an unnecessary trailing comma that is inconsistent with the existing test style in this file. The existing assertions (e.g., lines 68-69) don't use trailing commas for single-argument method calls. Consider removing the trailing comma for consistency.

@copilot open a new pull request to apply changes based on this feedback

Copilot · 2026-01-28T15:13:45Z

openwisp_controller/connection/tests/test_tasks.py

+        self.assertFalse(
+            result,
+        )


The assertion has an unnecessary trailing comma that is inconsistent with the existing test style in this file. The existing assertions (e.g., lines 68-69) don't use trailing commas for single-argument method calls. Consider removing the trailing comma for consistency.

Copilot · 2026-01-28T15:13:45Z

openwisp_controller/connection/tests/test_tasks.py

+        self.assertTrue(
+            result,
+        )


The assertion has an unnecessary trailing comma that is inconsistent with the existing test style in this file. The existing assertions (e.g., lines 68-69) don't use trailing commas for single-argument method calls. Consider removing the trailing comma for consistency.

Copilot · 2026-01-28T15:13:46Z

openwisp_controller/connection/tests/test_tasks.py

+        self.assertFalse(
+            result,
+        )


The assertion has an unnecessary trailing comma that is inconsistent with the existing test style in this file. The existing assertions (e.g., lines 68-69) don't use trailing commas for single-argument method calls. Consider removing the trailing comma for consistency.

…uplicate openwisp#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 openwisp#1204

coveralls · 2026-01-28T16:55:41Z

coverage: 98.658% (+0.001%) from 98.657%
when pulling 010ba62 on SAMurai-16:issues/1204-celery-execution-skip
into 0c5c4a5 on openwisp:master.

Copilot AI review requested due to automatic review settings January 28, 2026 15:09

Copilot started reviewing on behalf of SAMurai-16 January 28, 2026 15:09 View session

Merge branch 'master' into issues/1204-celery-execution-skip

e881eba

Copilot AI reviewed Jan 28, 2026

View reviewed changes

coderabbitai bot previously approved these changes Jan 28, 2026

View reviewed changes

SAMurai-16 dismissed coderabbitai[bot]’s stale review via 736044c January 28, 2026 15:29

coderabbitai bot previously approved these changes Jan 28, 2026

View reviewed changes

SAMurai-16 force-pushed the issues/1204-celery-execution-skip branch from 736044c to cca1aa2 Compare January 28, 2026 16:20

SAMurai-16 dismissed coderabbitai[bot]’s stale review via 010ba62 January 28, 2026 16:42

SAMurai-16 force-pushed the issues/1204-celery-execution-skip branch from cca1aa2 to 010ba62 Compare January 28, 2026 16:42

coderabbitai bot approved these changes Jan 28, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[connection] Fix update_config task incorrectly detecting itself as d… #1206

[connection] Fix update_config task incorrectly detecting itself as d… #1206

SAMurai-16 commented Jan 28, 2026 •

edited

Loading

Uh oh!

coderabbitai bot commented Jan 28, 2026 •

edited

Loading

Walkthrough

Sequence Diagram(s)

Estimated code review effort

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 28, 2026

Uh oh!

SAMurai-16 Jan 28, 2026

Uh oh!

Copilot AI Jan 28, 2026

Uh oh!

Copilot AI Jan 28, 2026

Uh oh!

Copilot AI Jan 28, 2026

Uh oh!

coveralls commented Jan 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

[connection] Fix update_config task incorrectly detecting itself as d… #1206

Are you sure you want to change the base?

[connection] Fix update_config task incorrectly detecting itself as d… #1206

Conversation

SAMurai-16 commented Jan 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Reference to Existing Issue

Uh oh!

coderabbitai bot commented Jan 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Sequence Diagram(s)

Estimated code review effort

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

SAMurai-16 Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jan 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

SAMurai-16 commented Jan 28, 2026 •

edited

Loading

coderabbitai bot commented Jan 28, 2026 •

edited

Loading