[fix] Fixed update_config duplicate task detection #1204#1292
[fix] Fixed update_config duplicate task detection #1204#1292nemesifier merged 1 commit intomasterfrom
Conversation
github-project-automation
bot
moved this to To do (general)
in OpenWISP Contributor's Board
📝 WalkthroughWalkthroughThe PR makes update_config a bound Celery task (bind=True) so it can access self.request.id, and changes _is_update_in_progress to accept an optional current_task_id to exclude the running task from the in-progress check. Calls to _is_update_in_progress now pass the current Celery task ID. Tests were added/updated to cover same-task exclusion, different-task detection, no active tasks, and different-device cases. Sequence Diagram(s)sequenceDiagram
participant Worker as Celery Worker
participant Task as update_config(self, device_id)
participant Inspect as Celery Inspect.active
participant DB as Config / Device models
participant Connector as get_working_connection / connector.update_config
Worker->>Task: start (self.request.id)
Task->>Inspect: Inspect.active()
Inspect-->>Task: active tasks per worker
Task->>Task: _is_update_in_progress(device_id, current_task_id=self.request.id)
alt no conflicting external task
Task->>DB: query pending Config for device
DB-->>Task: pending config entries
Task->>Connector: get_working_connection(device_id)
Connector-->>Task: connection
Task->>Connector: update_config(connection, config)
Connector-->>Task: result
else another worker's active task for same device
Task-->>Worker: skip execution (return)
end
Task-->>Worker: finish
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip Migrating from UI to YAML configuration.Use the |
The tasks._is_update_in_progress function will exclude the current task from the check. Fixes #1204 Co-authored-by: Piyush Bafna <130243298+piyushdev04@users.noreply.github.com>
pandafy
force-pushed
the
issues/1024-update-config-self-detection
branch
from
48075ce to
309ca0e
Compare
pandafy
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_controller/connection/tasks.py`:
- Around line 27-31: The membership check str(device_id) in task["args"] can
miss matches because inspect().active() may return typed args (UUID/int) not
strings; in the block that filters active tasks (using _TASK_NAME and
current_task_id), normalize task["args"] before comparing by converting each
element to string (e.g., build args_as_str = [str(a) for a in task["args"]] or
use any(str(device_id) == str(a) for a in task["args"])), then use that
normalized collection for the containment check to reliably detect duplicates.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4afb628a-b454-485e-bff8-38f7a057fe55
📒 Files selected for processing (3)
openwisp_controller/connection/tasks.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/connection/tests/test_tasks.py
📜 Review details
⏰ 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). (9)
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/tests/test_models.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/tests/test_models.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/tests/test_models.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/tests/test_models.py
🧠 Learnings (5)
📚 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.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/tests/test_models.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/tests/test_models.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.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/tests/test_models.py
📚 Learning: 2026-02-05T20:06:01.589Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1147
File: openwisp_controller/config/utils.py:222-222
Timestamp: 2026-02-05T20:06:01.589Z
Learning: In `openwisp_controller/config/utils.py`, the `send_api_task_notification` function is called from Celery background tasks. Do not use sleep() or blocking operations in this function as it blocks the Celery worker and prevents notifications from being sent.
Applied to files:
openwisp_controller/connection/tasks.py
📚 Learning: 2026-01-12T22:27:48.342Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
Applied to files:
openwisp_controller/connection/tests/test_models.py
🧬 Code graph analysis (2)
openwisp_controller/connection/tests/test_tasks.py (1)
openwisp_controller/connection/tasks.py (1)
_is_update_in_progress(19-33)
openwisp_controller/connection/tasks.py (3)
openwisp_controller/connection/base/models.py (1)
update_config(365-373)openwisp_controller/connection/connectors/ssh.py (1)
update_config(234-235)openwisp_controller/connection/connectors/openwrt/ssh.py (2)
update_config(11-25)update_config(60-61)
🔇 Additional comments (3)
openwisp_controller/connection/tests/test_tasks.py (1)
24-73: Good regression coverage for task de-duplication paths.The added tests correctly exercise same-task exclusion, different-task detection, empty active list, and different-device behavior for
_is_update_in_progress.openwisp_controller/connection/tasks.py (1)
36-37: Bound task +self.request.idwiring looks correct.Using
bind=Trueand passingcurrent_task_id=self.request.idis the right direction for excluding the currently runningupdate_configtask from duplicate detection.Also applies to: 56-56
openwisp_controller/connection/tests/test_models.py (1)
1029-1108: Strong behavioral coverage for in-progress detection after task binding.These test updates validate both critical branches: skipping when another task is active and proceeding when only the current task is present, plus the different-device non-blocking case.
github-project-automation
bot
moved this from To do (general)
to In progress
in OpenWISP Contributor's Board
|
@coderabbitai review! |
|
✅ Actions performedReview triggered.
|
|
@coderabbitai any outstanding issues? Please review. |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_controller/connection/tasks.py`:
- Around line 56-57: Before the early return in the _is_update_in_progress
check, add an info-level log stating that the update was skipped because another
update is in progress; include the device_id and current_task_id
(self.request.id) in the message for observability. Use the module logger (or
create one with logging.getLogger(__name__) if none exists) and call
logger.info("Skipping update for device %s: update already in progress
(task_id=%s)", device_id, self.request.id) immediately before the return in the
_is_update_in_progress(device_id, current_task_id=self.request.id) branch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 314a3845-cda1-42e4-989b-2bcb4fd00add
📒 Files selected for processing (3)
openwisp_controller/connection/tasks.pyopenwisp_controller/connection/tests/test_models.pyopenwisp_controller/connection/tests/test_tasks.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/tests/test_models.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/tests/test_models.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/tests/test_models.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/tests/test_models.py
🧠 Learnings (7)
📓 Common learnings
Learnt from: pandafy
Repo: openwisp/openwisp-controller PR: 1292
File: openwisp_controller/connection/tasks.py:27-31
Timestamp: 2026-03-17T09:20:04.547Z
Learning: In `openwisp_controller/connection/tasks.py`, the `update_config` Celery task only accepts one argument `device_id`, which is always passed as a string (via `str(device.pk)`) from the call site in `openwisp_controller/connection/apps.py`. Do not flag `str(device_id) in task["args"]` as unreliable due to typed args — the args are always strings.
📚 Learning: 2026-03-17T09:20:04.547Z
Learnt from: pandafy
Repo: openwisp/openwisp-controller PR: 1292
File: openwisp_controller/connection/tasks.py:27-31
Timestamp: 2026-03-17T09:20:04.547Z
Learning: In `openwisp_controller/connection/tasks.py`, the `update_config` Celery task only accepts one argument `device_id`, which is always passed as a string (via `str(device.pk)`) from the call site in `openwisp_controller/connection/apps.py`. Do not flag `str(device_id) in task["args"]` as unreliable due to typed args — the args are always strings.
Applied to files:
openwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/tests/test_models.py
📚 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.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/tests/test_models.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/tests/test_models.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.pyopenwisp_controller/connection/tasks.pyopenwisp_controller/connection/tests/test_models.py
📚 Learning: 2026-02-05T20:06:01.589Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1147
File: openwisp_controller/config/utils.py:222-222
Timestamp: 2026-02-05T20:06:01.589Z
Learning: In `openwisp_controller/config/utils.py`, the `send_api_task_notification` function is called from Celery background tasks. Do not use sleep() or blocking operations in this function as it blocks the Celery worker and prevents notifications from being sent.
Applied to files:
openwisp_controller/connection/tasks.py
📚 Learning: 2026-01-12T22:27:48.342Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
Applied to files:
openwisp_controller/connection/tests/test_models.py
🧬 Code graph analysis (2)
openwisp_controller/connection/tests/test_tasks.py (1)
openwisp_controller/connection/tasks.py (1)
_is_update_in_progress(19-33)
openwisp_controller/connection/tasks.py (3)
openwisp_controller/connection/base/models.py (1)
update_config(365-373)openwisp_controller/connection/connectors/ssh.py (1)
update_config(234-235)openwisp_controller/connection/connectors/openwrt/ssh.py (2)
update_config(11-25)update_config(60-61)
🔇 Additional comments (4)
openwisp_controller/connection/tasks.py (1)
19-31: Self-task exclusion fix looks correct.The
current_task_idexclusion in Line 30 addresses the false-positive self-detection path cleanly.openwisp_controller/connection/tests/test_tasks.py (1)
24-73: Good test coverage for current-task exclusion behavior.These cases validate the updated duplicate-detection semantics well (same task vs other task vs no tasks vs other device).
openwisp_controller/connection/tests/test_models.py (2)
1030-1080: Integration test update is solid.The two subtests now clearly validate both branches of the in-progress logic after introducing
current_task_id.
1093-1101: Different-device negative case is correctly modeled.This mock confirms that unrelated active tasks do not block updates for the current device.
| if _is_update_in_progress(device_id, current_task_id=self.request.id): | ||
| return |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add an info log before skipping duplicate updates.
Line 57 returns silently when another update is in progress. Add an info-level log so this background action is observable during incident/debug analysis.
Suggested patch
- if _is_update_in_progress(device_id, current_task_id=self.request.id):
+ if _is_update_in_progress(device_id, current_task_id=self.request.id):
+ logger.info(
+ f'Skipping update_config("{device_id}") because another update task is already running'
+ )
returnAs per coding guidelines "New code must handle errors properly ... log important background actions with info level".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_controller/connection/tasks.py` around lines 56 - 57, Before the
early return in the _is_update_in_progress check, add an info-level log stating
that the update was skipped because another update is in progress; include the
device_id and current_task_id (self.request.id) in the message for
observability. Use the module logger (or create one with
logging.getLogger(__name__) if none exists) and call logger.info("Skipping
update for device %s: update already in progress (task_id=%s)", device_id,
self.request.id) immediately before the return in the
_is_update_in_progress(device_id, current_task_id=self.request.id) branch.
There was a problem hiding this comment.
Solid fix. The bug was a classic self-detection problem, the task was inspecting active workers, finding itself in the list, and bailing out thinking someone else had it covered. Nobody was actually doing the work.
The approach is sound:
bind=Trueon the shared task gives access toself.request.id— the right way to get the current task's identity in Celery.- Comparing
task["id"] != current_task_idis the minimal, correct filter to exclude the running task from the duplicate check. - No unnecessary complexity added.
Tests: Good coverage: unit tests on _is_update_in_progress for same task, different task, no tasks, and different device. The integration-level test_device_update_config_in_progress now uses subTest to verify both the "should skip" and "should proceed" paths, which is a nice improvement over the old test that only checked one side of the coin.
One minor observation (non-blocking): _is_update_in_progress still does current_app.control.inspect().active() which broadcasts to all workers and waits for replies: it's a relatively expensive call on every config update. Not introduced by this PR though, so not something to hold this up for.
LGTM.
github-project-automation
bot
moved this from In progress
to Done
in OpenWISP Contributor's Board
3 participants
Checklist
Reference to Existing Issue
Fixes #1204
Description of Changes
The tasks._is_update_in_progress function will exclude the current task from the check.