[fix] Prevent update_config task from detecting itself as duplicate#1278
[fix] Prevent update_config task from detecting itself as duplicate#1278mn-ram wants to merge 1 commit intoopenwisp:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📜 Recent 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)
Files:
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}📄 CodeRabbit inference engine (Custom checks)
Files:
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}📄 CodeRabbit inference engine (Custom checks)
Files:
**/*.{py,html}📄 CodeRabbit inference engine (Custom checks)
Files:
🧠 Learnings (8)📓 Common learnings📚 Learning: 2026-03-17T09:20:04.547ZApplied to files:
📚 Learning: 2026-01-12T22:27:48.342ZApplied to files:
📚 Learning: 2026-02-05T20:06:01.589ZApplied to files:
📚 Learning: 2026-03-14T19:34:33.773ZApplied to files:
📚 Learning: 2026-01-15T15:05:49.557ZApplied to files:
📚 Learning: 2026-02-17T19:13:10.088ZApplied to files:
📚 Learning: 2026-01-15T15:07:17.354ZApplied to files:
🔇 Additional comments (9)
📝 WalkthroughWalkthroughReplaced in-flight Celery Inspect checks with a per-device lock stored in Django cache in openwisp_controller/connection/tasks.py. Added _acquire_update_config_lock(device_id) which returns a token when lock acquired (or None), and _release_update_config_lock(device_id, token) to release it. update_config now tries to acquire the lock, skips if unavailable, performs the existing device lookup and update when locked, and always releases the lock in a finally block. Tests were updated and new tests added for lock acquire/release semantics. Sequence Diagram(s)sequenceDiagram
participant Worker as "Celery Worker\n(update_config)"
participant Cache as "Django Cache\n(per-device lock)"
participant DB as "Database\n(Device)"
rect rgba(0,128,0,0.5)
Worker->>Cache: _acquire_update_config_lock(device_id)
end
alt lock acquired
Worker->>DB: fetch device & perform update_config(device_id)
DB-->>Worker: update result
rect rgba(0,0,255,0.5)
Worker->>Cache: _release_update_config_lock(device_id, token)
end
else lock not acquired
Worker->>Worker: log "skipping update - lock held"
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (3 errors, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 55-56: Add an info-level log when the task returns early due to an
in-progress update: inside the branch that calls
_is_update_in_progress(device_id, current_task_id=self.request.id) and returns,
emit logger.info with the device_id and self.request.id (e.g., "Skipping
firmware update for device=%s: update in progress (task_id=%s)"). If logger is
not already defined in this module, ensure you obtain one via
logging.getLogger(__name__) before using it.
In `@openwisp_controller/connection/tests/test_models.py`:
- Around line 1033-1042: Add a test case that verifies a task with the same id
as the current task is ignored: when mocking celery.app.control.Inspect.active
(mocked_active) return value, include a task dict with "name": _TASK_NAME and
"args": [str(conf.device.pk)] but set "id" to the current task id (simulate
current_task_id) and assert the code path that checks for in-progress tasks does
NOT treat it as in-progress; update the existing test(s) that currently use
"other-task-id" (the block using mocked_active around _TASK_NAME and
conf.device.pk) and add the new assertion so the self-id branch is covered (also
apply the same addition to the similar case at lines 1061-1066).
🪄 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: 4805ecff-4012-40ec-b375-1caf68853b02
📒 Files selected for processing (2)
openwisp_controller/connection/tasks.pyopenwisp_controller/connection/tests/test_models.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). (11)
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.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_models.pyopenwisp_controller/connection/tasks.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_models.pyopenwisp_controller/connection/tasks.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_models.pyopenwisp_controller/connection/tasks.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_models.pyopenwisp_controller/connection/tasks.py
🧠 Learnings (5)
📚 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
📚 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_models.pyopenwisp_controller/connection/tasks.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_models.pyopenwisp_controller/connection/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_models.pyopenwisp_controller/connection/tasks.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
🔇 Additional comments (1)
openwisp_controller/connection/tasks.py (1)
19-30: Self-detection fix looks correct.Binding the task and passing
self.request.idinto_is_update_in_progresscorrectly avoids treating the currently runningupdate_configtask as a duplicate.Also applies to: 35-36, 55-55
CI Failures DetectedHello @prakash-kalwaniya, There are Code Style/QA failures detected in the CI logs. Failures & Remediation
|
mn-ram
force-pushed
the
fix/update-config-self-detection
branch
from
77e7300 to
f3832c5
Compare
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 19-31: The current _is_update_in_progress(device_id,
current_task_id=None) uses a non-atomic snapshot of Celery active tasks and can
return True for both overlapping update_config tasks; replace this dedup logic
with an atomic per-device lock (e.g., a Redis/Django cache lock or database row
lock) that is acquired at task start and released at task end to prevent
concurrent updates for the same device. Update the task entrypoint
(update_config) to attempt to acquire a lock keyed by device_id before
proceeding, fail/exit if the lock cannot be acquired, and ensure the lock is
always released (or has a TTL) even on errors; you can keep _TASK_NAME and
current_task_id but remove reliance on inspect().active() and use the lock for
mutual exclusion.
🪄 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: c0792b9a-6c0c-4fbe-961d-e0c12d0707a6
📒 Files selected for processing (2)
openwisp_controller/connection/tasks.pyopenwisp_controller/connection/tests/test_models.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). (11)
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.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/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/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/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/tasks.pyopenwisp_controller/connection/tests/test_models.py
🧠 Learnings (6)
📚 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-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/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/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/tasks.pyopenwisp_controller/connection/tests/test_models.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
📚 Learning: 2026-03-14T19:34:33.761Z
Learnt from: CR
Repo: openwisp/openwisp-controller PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-14T19:34:33.761Z
Learning: Applies to **/*{.test,.spec,_test,test_}.{js,ts,tsx,jsx,py,java,go,cs,rb} : Tests must be updated to cover non-trivial changes and ensure proper validation of modified behavior
Applied to files:
openwisp_controller/connection/tests/test_models.py
🔇 Additional comments (1)
openwisp_controller/connection/tests/test_models.py (1)
1072-1107: Good regression coverage for the self-id fix.These tests cover both branches of the new
current_task_idhandling and would catch the original self-detection bug.
This comment was marked as spam.
This comment was marked as spam.
🧩 Analysis chainLet me fetch the current state of the code before reviewing. 🏁 Script executed: Length of output: 5662
✅ What's well done
🔍 One minor cleanup
# Before (current state — bind=True is now unused)
`@shared_task`(bind=True)
def update_config(self, device_id):
# After (cleaner)
`@shared_task`
def update_config(device_id):This is purely cosmetic but avoids confusing future readers into thinking 🤔 One edge case worth verifyingIf the cache backend is unavailable (e.g. Redis is down), Overall, this is a meaningful improvement over the ✅ Actions performedReview triggered.
|
mn-ram
force-pushed
the
fix/update-config-self-detection
branch
from
fbd3f5c to
09208c6
Compare
This comment was marked as spam.
This comment was marked as spam.
|
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 23-37: The current lock functions (_acquire_update_config_lock and
_release_update_config_lock) use a boolean value and unconditionally delete the
key, allowing one task to delete a lock another task owns; change to a tokenized
ownership lock: when acquiring in _acquire_update_config_lock generate a unique
token (UUID), store that token as the cache value via cache.add with the
configured timeout (ensure the timeout equals or exceeds the worst-case task
duration), and return the token to the caller; update callers to pass that token
into _release_update_config_lock; in _release_update_config_lock check the
current cache value and only delete the key if it matches the token (to avoid
deleting a newer lock owned by another task), otherwise leave the key intact and
return a failure/OK status accordingly.
- Around line 67-68: In the except NoWorkingDeviceConnectionError block add a
warning-level log before returning: call the module logger (use the existing
logger or logging.getLogger(__name__)) and emit logger.warning(...) with a short
message including the exception details and available context identifiers (e.g.,
device_id, connection_id or task id variables in this scope) so the silent
return is recorded; keep the return behavior unchanged.
In `@openwisp_controller/connection/tests/test_models.py`:
- Around line 1058-1069: The test currently patches
"_release_update_config_lock" but never asserts it was invoked; update the test
in test_models.py so after calling conf.full_clean() and conf.save() you assert
the release mock was called (e.g., assert_called_once()) to ensure the
finally/release path for _release_update_config_lock is exercised; reference the
patched symbol "_release_update_config_lock" (or capture it as a mocked_release
variable in the same mock.patch context) and add a single assertion after
mocked_update_config.assert_called_once() to validate the lock release.
🪄 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: bd1bc3ac-cf66-4921-94bc-004c6aa6f44b
📒 Files selected for processing (2)
openwisp_controller/connection/tasks.pyopenwisp_controller/connection/tests/test_models.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). (11)
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | 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.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.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/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/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/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/tasks.pyopenwisp_controller/connection/tests/test_models.py
🧠 Learnings (7)
📓 Common learnings
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.
📚 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-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/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/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/tasks.pyopenwisp_controller/connection/tests/test_models.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
📚 Learning: 2026-03-14T19:34:33.761Z
Learnt from: CR
Repo: openwisp/openwisp-controller PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-14T19:34:33.761Z
Learning: Applies to **/*{.test,.spec,_test,test_}.{js,ts,tsx,jsx,py,java,go,cs,rb} : Tests must be updated to cover non-trivial changes and ensure proper validation of modified behavior
Applied to files:
openwisp_controller/connection/tests/test_models.py
🔇 Additional comments (3)
openwisp_controller/connection/tasks.py (1)
59-63: Good observability on duplicate-skip path.The info log added when lock acquisition fails makes background task behavior traceable and debuggable.
openwisp_controller/connection/tests/test_models.py (2)
1037-1046: Good lock-contention regression coverage.This correctly simulates lock acquisition failure and verifies that connection lookup and config push are both skipped.
1070-1081: Nice direct coverage for lock acquire/release semantics.The new test validates first-acquire success, duplicate rejection, and reacquisition after release.
There was a problem hiding this comment.
♻️ Duplicate comments (3)
openwisp_controller/connection/tasks.py (2)
65-68:⚠️ Potential issue | 🟠 MajorLog the no-working-connection early exit at warning level.
This branch returns silently, which makes production diagnosis harder.
🪵 Proposed logging addition
- except NoWorkingDeviceConnectionError: + except NoWorkingDeviceConnectionError as exc: + logger.warning( + 'update_config("%s") skipped: no working connection (%s)', + device_id, + exc, + ) returnAs per coding guidelines: "New code must handle errors properly: log unusual conditions with warning level, 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 65 - 68, The code silently returns when DeviceConnection.get_working_connection(device) raises NoWorkingDeviceConnectionError; update the except block to log a warning before returning (e.g., call logger.warning or the module's logger) including identifying context such as device (id or name) and the exception message, then keep the early return; modify the except for NoWorkingDeviceConnectionError in tasks.py accordingly.
23-37:⚠️ Potential issue | 🔴 CriticalMake the cache lock ownership-aware to prevent deleting another task’s lock.
The current boolean lock + unconditional delete allows a stale task to remove a newer lock owner after TTL expiry, re-opening overlap.
🔐 Proposed fix (tokenized lock ownership)
+import uuid @@ def _acquire_update_config_lock(device_id): @@ - return cache.add(lock_key, True, timeout=_UPDATE_CONFIG_LOCK_TIMEOUT) + token = uuid.uuid4().hex + acquired = cache.add(lock_key, token, timeout=_UPDATE_CONFIG_LOCK_TIMEOUT) + return token if acquired else None @@ -def _release_update_config_lock(device_id): +def _release_update_config_lock(device_id, token): """Releases the per-device update_config lock.""" lock_key = _UPDATE_CONFIG_LOCK_KEY.format(device_id=device_id) - cache.delete(lock_key) + if cache.get(lock_key) == token: + cache.delete(lock_key) @@ - if not _acquire_update_config_lock(device_id): + lock_token = _acquire_update_config_lock(device_id) + if not lock_token: logger.info( f"update_config for device {device_id} is already in progress, skipping" ) return @@ - _release_update_config_lock(device_id) + _release_update_config_lock(device_id, lock_token)Also applies to: 72-73
🤖 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 23 - 37, Current boolean lock allows a caller to unconditionally delete a lock; change to tokenized ownership: in _acquire_update_config_lock(device_id) generate a unique token (uuid4), use cache.add(lock_key, token, timeout=_UPDATE_CONFIG_LOCK_TIMEOUT) and return the token on success (or False/None on failure), and change _release_update_config_lock(device_id, token) to fetch the current value (cache.get(lock_key)) and only call cache.delete(lock_key) if the stored token equals the provided token; update all call sites that call _acquire_update_config_lock and _release_update_config_lock (e.g., the update_config task) to accept and pass the returned token so only the lock owner can release it.openwisp_controller/connection/tests/test_models.py (1)
1058-1068:⚠️ Potential issue | 🟡 MinorAssert the mocked release call to protect the
finallycontract.
_release_update_config_lockis patched but never asserted; a regression in release would still pass.✅ Proposed assertion update
with mock.patch( "openwisp_controller.connection.tasks._acquire_update_config_lock", return_value=True, ), mock.patch( "openwisp_controller.connection.tasks._release_update_config_lock", - ): + ) as mocked_release: conf.config = {"general": {"timezone": "UTC"}} conf.full_clean() conf.save() mocked_get_working_connection.assert_called_once() mocked_update_config.assert_called_once() + mocked_release.assert_called_once()Based on learnings: "Tests must be updated to cover non-trivial changes and ensure proper validation of modified behavior".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/connection/tests/test_models.py` around lines 1058 - 1068, The test patches _acquire_update_config_lock and _release_update_config_lock but never asserts the release call; update the test to capture the patched release as a mock (e.g., mock.patch(..., autospec=True) as mocked_release) and add an assertion like mocked_release.assert_called_once() after the save block to ensure the finally contract is honored; keep the existing assertions for mocked_get_working_connection and mocked_update_config and place the new mocked_release.assert_called_once() alongside them.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@openwisp_controller/connection/tasks.py`:
- Around line 65-68: The code silently returns when
DeviceConnection.get_working_connection(device) raises
NoWorkingDeviceConnectionError; update the except block to log a warning before
returning (e.g., call logger.warning or the module's logger) including
identifying context such as device (id or name) and the exception message, then
keep the early return; modify the except for NoWorkingDeviceConnectionError in
tasks.py accordingly.
- Around line 23-37: Current boolean lock allows a caller to unconditionally
delete a lock; change to tokenized ownership: in
_acquire_update_config_lock(device_id) generate a unique token (uuid4), use
cache.add(lock_key, token, timeout=_UPDATE_CONFIG_LOCK_TIMEOUT) and return the
token on success (or False/None on failure), and change
_release_update_config_lock(device_id, token) to fetch the current value
(cache.get(lock_key)) and only call cache.delete(lock_key) if the stored token
equals the provided token; update all call sites that call
_acquire_update_config_lock and _release_update_config_lock (e.g., the
update_config task) to accept and pass the returned token so only the lock owner
can release it.
In `@openwisp_controller/connection/tests/test_models.py`:
- Around line 1058-1068: The test patches _acquire_update_config_lock and
_release_update_config_lock but never asserts the release call; update the test
to capture the patched release as a mock (e.g., mock.patch(..., autospec=True)
as mocked_release) and add an assertion like mocked_release.assert_called_once()
after the save block to ensure the finally contract is honored; keep the
existing assertions for mocked_get_working_connection and mocked_update_config
and place the new mocked_release.assert_called_once() alongside them.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b4dc6f12-d05a-44b5-ae09-e12c73030fff
📒 Files selected for processing (2)
openwisp_controller/connection/tasks.pyopenwisp_controller/connection/tests/test_models.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/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/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/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/tasks.pyopenwisp_controller/connection/tests/test_models.py
🧠 Learnings (7)
📓 Common learnings
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.
📚 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-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/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/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/tasks.pyopenwisp_controller/connection/tests/test_models.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
📚 Learning: 2026-03-14T19:34:33.761Z
Learnt from: CR
Repo: openwisp/openwisp-controller PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-14T19:34:33.761Z
Learning: Applies to **/*{.test,.spec,_test,test_}.{js,ts,tsx,jsx,py,java,go,cs,rb} : Tests must be updated to cover non-trivial changes and ensure proper validation of modified behavior
Applied to files:
openwisp_controller/connection/tests/test_models.py
🔇 Additional comments (1)
openwisp_controller/connection/tests/test_models.py (1)
1070-1081: Good lock behavior regression coverage.This test clearly validates acquire → block duplicate → release → reacquire, which matches the new synchronization model.
This comment was marked as spam.
This comment was marked as spam.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me pull the current state of the code before responding. 🏁 Script executed: Length of output: 8978
✅ What's well done
🔍 Two minor nits worth noting1. The current assertion only checks the call count, not the arguments. Since mocked_release.assert_called_once_with(str(device.pk), "fake-lock-token")This makes the test more precise and would catch a regression where the token is not passed through correctly. 2. Missing cleanup guard in lock tests In both def test_release_update_config_lock_wrong_token(self):
device_id = "test-device-id"
token = _acquire_update_config_lock(device_id)
self.addCleanup(_release_update_config_lock, device_id, token) # always runs
self.assertIsNotNone(token)
_release_update_config_lock(device_id, "wrong-token")
self.assertIsNone(_acquire_update_config_lock(device_id))Both nits are minor. The core logic — atomic acquire, UUID-gated release, TTL safety net, and |
This comment was marked as spam.
This comment was marked as spam.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3054
The implementation is solid end-to-end — atomic acquire, UUID-gated release, TTL safety net, |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
openwisp_controller/connection/tasks.py (1)
77-80:⚠️ Potential issue | 🟡 MinorAdd warning log before returning on missing working connection.
The
NoWorkingDeviceConnectionErrorpath silently returns, making failed background updates hard to trace. Per coding guidelines, unusual conditions should be logged at warning level.Suggested logging tweak
try: device_conn = DeviceConnection.get_working_connection(device) except NoWorkingDeviceConnectionError: + logger.warning( + 'update_config("%s") skipped: no working connection available', + device_id, + ) return🤖 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 77 - 80, The except branch for NoWorkingDeviceConnectionError currently returns silently in DeviceConnection.get_working_connection usage; add a warning log before returning to surface missing working connections (e.g., use the module logger to call logger.warning and include the device id/name and exception context). Update the except block that catches NoWorkingDeviceConnectionError in the task handling code in openwisp_controller/connection/tasks.py so it logs a descriptive warning mentioning DeviceConnection.get_working_connection and the device variable, then returns as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@openwisp_controller/connection/tasks.py`:
- Around line 77-80: The except branch for NoWorkingDeviceConnectionError
currently returns silently in DeviceConnection.get_working_connection usage; add
a warning log before returning to surface missing working connections (e.g., use
the module logger to call logger.warning and include the device id/name and
exception context). Update the except block that catches
NoWorkingDeviceConnectionError in the task handling code in
openwisp_controller/connection/tasks.py so it logs a descriptive warning
mentioning DeviceConnection.get_working_connection and the device variable, then
returns as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 35ad059b-b35a-4857-9239-b81329929e63
📒 Files selected for processing (2)
openwisp_controller/connection/tasks.pyopenwisp_controller/connection/tests/test_models.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/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/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/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/tasks.pyopenwisp_controller/connection/tests/test_models.py
🧠 Learnings (7)
📓 Common learnings
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.
📚 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-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/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/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/tasks.pyopenwisp_controller/connection/tests/test_models.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
📚 Learning: 2026-03-14T19:34:33.761Z
Learnt from: CR
Repo: openwisp/openwisp-controller PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-14T19:34:33.761Z
Learning: Applies to **/*{.test,.spec,_test,test_}.{js,ts,tsx,jsx,py,java,go,cs,rb} : Tests must be updated to cover non-trivial changes and ensure proper validation of modified behavior
Applied to files:
openwisp_controller/connection/tests/test_models.py
🔇 Additional comments (7)
openwisp_controller/connection/tasks.py (3)
18-21: LGTM on lock constants.The lock key pattern and 5-minute TTL as a safety net is appropriate. The comment clearly explains the timeout's purpose.
24-36: LGTM on lock acquisition.The use of
cache.add()provides atomic lock acquisition, and the UUID token enables ownership-aware release. The docstring clearly documents the contract.
39-47: LGTM on lock release with ownership check.The token comparison ensures only the lock owner can release. While there's a theoretical TOCTOU window between
cache.get()andcache.delete(), the window is very small and the trade-off was accepted per PR discussion.openwisp_controller/connection/tests/test_models.py (4)
24-28: LGTM on updated imports.The imports correctly reference the new lock functions for testing.
1032-1045: LGTM on in-progress test.The test correctly validates that when lock acquisition fails (returns
None), the update is skipped and neither connection nor config update is attempted.
1047-1069: LGTM on not-in-progress test.The test properly validates the happy path: lock acquired, connection obtained, update performed, and lock released. The
mocked_release.assert_called_once()assertion at line 1069 ensures thefinallyblock contract is tested.
1071-1096: LGTM on lock unit tests.
test_acquire_update_config_lockvalidates the full lifecycle: acquisition, duplicate prevention, release, and re-acquisition.test_release_update_config_lock_wrong_tokenconfirms ownership-aware release prevents unauthorized lock deletion. Both tests properly clean up state.
This comment was marked as spam.
This comment was marked as spam.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 4590
except NoWorkingDeviceConnectionError as e:
logger.warning(
f"update_config for device {device_id}: "
f"DeviceConnection.get_working_connection failed: {e}"
)
return
This is a clean, informative addition that will make these failures much easier to diagnose in production. The PR looks solid end-to-end — ready to merge. 🎉 |
|
Approving the PR now. 🎉 ✅ Actions performedComments resolved and changes approved. |
Test Failure in
|
mn-ram
force-pushed
the
fix/update-config-self-detection
branch
from
7e95edd to
bc41f8d
Compare
Test Failure in
|
mn-ram
force-pushed
the
fix/update-config-self-detection
branch
from
36ecca8 to
691541c
Compare
Commit Message Format FailureHello @prakash-kalwaniya, Failure: Commit Message Validation Failed Explanation: Remediation: Here is an example of the correct format: |
…penwisp#1277 The _is_update_in_progress() helper was detecting the currently executing task as a duplicate because Celery's inspect().active() snapshot included the running task itself. Replaced snapshot-based detection with an atomic per-device cache lock using Django's cache framework. Fixes openwisp#1277
mn-ram
force-pushed
the
fix/update-config-self-detection
branch
from
691541c to
0952f5b
Compare
This comment was marked as spam.
This comment was marked as spam.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 7661
The implementation is solid end-to-end. Approving now. 🎉 [approve] |
This comment was marked as spam.
This comment was marked as spam.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 7661
The implementation is solid end-to-end. Approving now. 🎉 [approve] |
|
✅ Actions performedComments resolved and changes approved. |
|
✅ Actions performedFull review triggered. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3054
The implementation is solid end-to-end — atomic acquire, UUID-gated release, TTL safety net, |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 8578
✅ What's well done
🔍 Two minor nits worth noting1. The current assertion only checks the call count, not the arguments. Since mocked_release.assert_called_once_with(str(device.pk), "fake-lock-token")This makes the test more precise and would catch a regression where the token is not passed through correctly. 2. Missing cleanup guard in lock tests In both def test_release_update_config_lock_wrong_token(self):
device_id = "test-device-id"
token = _acquire_update_config_lock(device_id)
self.addCleanup(_release_update_config_lock, device_id, token) # always runs
self.assertIsNotNone(token)
_release_update_config_lock(device_id, "wrong-token")
self.assertIsNone(_acquire_update_config_lock(device_id))Both nits are minor. The core logic — atomic acquire, UUID-gated release, TTL safety net, and |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 6863
✅ What's well done
🔍 One minor cleanup
# Before (current state — bind=True is now unused)
`@shared_task`(bind=True)
def update_config(self, device_id):
# After (cleaner)
`@shared_task`
def update_config(device_id):This is purely cosmetic but avoids confusing future readers into thinking 🤔 One edge case worth verifyingIf the cache backend is unavailable (e.g. Redis is down), Overall, this is a meaningful improvement over the |
|
We're already working on it: #1292 |
3 participants
Closes #1277
Fix self-detection in
_is_update_in_progresscausing skipped config pushesProblem
The
_is_update_in_progress()helper checks Celery'sinspect().active()to detect if anupdate_configtask is already running for the same device.However, this list includes the currently executing task itself. Since the function does not exclude the current task ID, it always detects the running task as a duplicate and returns
True.As a result, the
update_configtask exits early: