[fix] Invalidate controller views cache when org/group context variables change #1070#1308
Conversation
When organization context variables are updated, `bulk_invalidate_config_get_cached_checksum` was called to bust the Config checksum cache, but `invalidate_controller_views_cache` was never called. This left the DeviceChecksumView cache stale, so devices kept receiving the old checksum, never transitioned to `modified` status, and never pulled the updated configuration. Fix: also call `invalidate_controller_views_cache` whenever the org `context` field changes, mirroring the existing behaviour that already fires this task when an organisation is disabled/enabled. Fixes openwisp#1070
|
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:
📝 WalkthroughWalkthroughWhen an existing Organization or DeviceGroup instance is saved and its Sequence Diagram(s)sequenceDiagram
participant Admin as Admin (updates context)
participant Django as Django Model Save
participant Celery as Celery Broker/Worker
participant DB as Database (Config rows)
participant Cache as Controller Views Cache
Admin->>Django: save(Organization with changed context)
Django->>Celery: bulk_invalidate_config_get_cached_checksum.delay({"device__organization_id": org_id})
Django->>Celery: invalidate_controller_views_cache.delay(org_id)
Celery->>DB: locate affected Config rows and mark as "modified"
Celery->>Cache: invalidate DeviceChecksumView & GetVpnView for org_id
sequenceDiagram
participant Admin as Admin (updates context)
participant Django as Django Model Save
participant Celery as Celery Broker/Worker
participant DB as Database (Config rows)
participant Cache as Controller Views Cache
Admin->>Django: save(DeviceGroup with changed context)
Django->>Celery: bulk_invalidate_config_get_cached_checksum.delay({"device__group_id": group_id})
Django->>Celery: invalidate_controller_views_for_group.delay(group_id)
Celery->>DB: locate affected Config rows and mark as "modified"
Celery->>Cache: invalidate DeviceChecksumView for devices in group_id
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_controller/config/base/multitenancy.py (1)
99-103:⚠️ Potential issue | 🟠 MajorEnforce deterministic task order to avoid stale cache re-population.
At Line 100 and Line 103, dispatching two independent Celery
.delay()calls can run out of order. If controller-view cache invalidation finishes first, stale checksum data can be cached again before checksum invalidation completes.Proposed fix (chain the tasks in order)
+from celery import chain from ..tasks import bulk_invalidate_config_get_cached_checksum, invalidate_controller_views_cache @@ if context_changed: - bulk_invalidate_config_get_cached_checksum.delay( - {"device__organization_id": str(self.organization_id)} - ) - invalidate_controller_views_cache.delay(str(self.organization_id)) + chain( + bulk_invalidate_config_get_cached_checksum.si( + {"device__organization_id": str(self.organization_id)} + ), + invalidate_controller_views_cache.si(str(self.organization_id)), + ).delay()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/config/base/multitenancy.py` around lines 99 - 103, Two independent Celery .delay() calls (bulk_invalidate_config_get_cached_checksum.delay and invalidate_controller_views_cache.delay) can run out of order; replace them with a Celery chain so checksum invalidation runs first and then controller-view invalidation. Create task signatures (e.g., bulk_invalidate_config_get_cached_checksum.s({"device__organization_id": str(self.organization_id)}) and invalidate_controller_views_cache.s(str(self.organization_id))) and execute them with chain(...).apply_async() so the tasks execute deterministically in that order.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@openwisp_controller/config/base/multitenancy.py`:
- Around line 99-103: Two independent Celery .delay() calls
(bulk_invalidate_config_get_cached_checksum.delay and
invalidate_controller_views_cache.delay) can run out of order; replace them with
a Celery chain so checksum invalidation runs first and then controller-view
invalidation. Create task signatures (e.g.,
bulk_invalidate_config_get_cached_checksum.s({"device__organization_id":
str(self.organization_id)}) and
invalidate_controller_views_cache.s(str(self.organization_id))) and execute them
with chain(...).apply_async() so the tasks execute deterministically in that
order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7bbfb950-c36f-4654-be6f-cab7769c0ee2
📒 Files selected for processing (1)
openwisp_controller/config/base/multitenancy.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.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.13 | 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~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | 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/config/base/multitenancy.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/config/base/multitenancy.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/config/base/multitenancy.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/config/base/multitenancy.py
🧠 Learnings (3)
📚 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/config/base/multitenancy.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/config/base/multitenancy.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/config/base/multitenancy.py
🔇 Additional comments (1)
openwisp_controller/config/base/multitenancy.py (1)
15-15: Import update is correct and consistent.The new task import matches the usage in
save()and follows the established task call pattern.
…rg-context-change
Split the long import line into multi-line format to stay within the 88-character line length limit required by Black and isort.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_controller/config/base/multitenancy.py (1)
102-106:⚠️ Potential issue | 🟠 MajorLGTM! The fix correctly invalidates controller views cache on context change.
The implementation mirrors the existing pattern from
handlers.py(passingstr(organization_id)) and correctly positions the call aftersuper().save()within thecontext_changedblock.However:
AbstractDeviceGroup.save()indevice_group.py(lines 79-90) should also callinvalidate_controller_views_cachewhen itscontextchanges. Currently it only callsbulk_invalidate_config_get_cached_checksum, leaving device checksum view caches stale when device group variables change. This mirrors the pattern established in this PR for organization-level context changes and should be applied consistently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/config/base/multitenancy.py` around lines 102 - 106, AbstractDeviceGroup.save currently only calls bulk_invalidate_config_get_cached_checksum when context changes; update AbstractDeviceGroup.save to also call invalidate_controller_views_cache with the organization's id to avoid stale device checksum view caches. Locate the context_changed handling in AbstractDeviceGroup.save (method name: save, variable: context_changed) and add a call to invalidate_controller_views_cache.delay(str(self.organization_id)) in the same post-save block (after super().save()) mirroring the pattern used in multitenancy.py and handlers.py so the organization id is passed as a string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@openwisp_controller/config/base/multitenancy.py`:
- Around line 102-106: AbstractDeviceGroup.save currently only calls
bulk_invalidate_config_get_cached_checksum when context changes; update
AbstractDeviceGroup.save to also call invalidate_controller_views_cache with the
organization's id to avoid stale device checksum view caches. Locate the
context_changed handling in AbstractDeviceGroup.save (method name: save,
variable: context_changed) and add a call to
invalidate_controller_views_cache.delay(str(self.organization_id)) in the same
post-save block (after super().save()) mirroring the pattern used in
multitenancy.py and handlers.py so the organization id is passed as a string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 951a382e-02af-4524-b783-a0ab36bb1bd6
📒 Files selected for processing (1)
openwisp_controller/config/base/multitenancy.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.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | 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~=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/config/base/multitenancy.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/config/base/multitenancy.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/config/base/multitenancy.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/config/base/multitenancy.py
🧠 Learnings (3)
📚 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/config/base/multitenancy.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/config/base/multitenancy.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/config/base/multitenancy.py
🔇 Additional comments (1)
openwisp_controller/config/base/multitenancy.py (1)
15-18: LGTM!The import is correctly added alongside the existing
bulk_invalidate_config_get_cached_checksumimport, maintaining clean organization of task imports.
Fixes openwisp#1070 Add tests to verify that when organization or device group configuration variables change, the config status updates to "modified" only for devices whose rendered configuration is actually affected by the change.
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/config/base/device_group.py`:
- Line 94: Currently the DeviceGroup.save path calls
invalidate_controller_views_cache.delay(str(self.organization_id)) which
triggers an org-wide loop; replace this with a new, group-scoped task to avoid
invalidating unrelated devices and VPNs: add a task function in
openwisp_controller/config/tasks.py (e.g.,
invalidate_controller_views_for_group(group_id, organization_id=None)) that
queries devices filtered by group_id and only clears DeviceChecksumView (and any
other group-local views) for those device IDs, then call that new task from
DeviceGroup.context/save instead of invalidate_controller_views_cache; also
ensure the new task does not perform VPN invalidation and accepts group_id as
the unique identifier to limit scope.
In `@openwisp_controller/config/tests/test_device.py`:
- Around line 463-490: Add a mock.patch around the controller-view cache
invalidation task (patch invalidate_controller_views_cache.delay) in
test_status_update_on_group_variable_change so you assert the task is enqueued
when dg.context is changed: patch the task before assigning dg.context and
saving (after dg.full_clean()/dg.save()), assert the patched delay was called
(e.g., called_once or called_with expected args) to ensure
invalidate_controller_views_cache.delay is triggered when context changes while
keeping the existing assertions for c1.status and c2.status.
🪄 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: 3661edd0-e1f9-4a60-aa51-69420385e73f
📒 Files selected for processing (2)
openwisp_controller/config/base/device_group.pyopenwisp_controller/config/tests/test_device.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~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.13 | 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/config/base/device_group.pyopenwisp_controller/config/tests/test_device.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/config/base/device_group.pyopenwisp_controller/config/tests/test_device.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/config/base/device_group.pyopenwisp_controller/config/tests/test_device.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/config/base/device_group.pyopenwisp_controller/config/tests/test_device.py
🧠 Learnings (4)
📚 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/config/base/device_group.pyopenwisp_controller/config/tests/test_device.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/config/base/device_group.pyopenwisp_controller/config/tests/test_device.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/config/base/device_group.pyopenwisp_controller/config/tests/test_device.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/config/tests/test_device.py
🔇 Additional comments (2)
openwisp_controller/config/base/device_group.py (1)
18-21: Import update looks correct.Good addition of
invalidate_controller_views_cachein the task imports; this aligns with the new save-path behavior.openwisp_controller/config/tests/test_device.py (1)
436-462: Nice targeted regression coverage for selective status updates.The affected/unaffected assertions are clear and directly validate the intended org-context behavior.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_controller/config/base/device_group.py (1)
90-94:⚠️ Potential issue | 🟠 MajorEnforce deterministic ordering between checksum invalidation and view-cache invalidation.
Line 91 and Line 94 enqueue two independent async tasks. Their execution order is nondeterministic, so
DeviceChecksumViewcan be repopulated with stale data if view invalidation runs first.Suggested fix
+from celery import chain @@ if context_changed: - bulk_invalidate_config_get_cached_checksum.delay( - {"device__group_id": str(self.id)} - ) - invalidate_controller_views_for_group.delay(str(self.id)) + chain( + bulk_invalidate_config_get_cached_checksum.si( + {"device__group_id": str(self.id)} + ), + invalidate_controller_views_for_group.si(str(self.id)), + ).delay()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/config/base/device_group.py` around lines 90 - 94, Two async tasks (bulk_invalidate_config_get_cached_checksum.delay and invalidate_controller_views_for_group.delay) are enqueued in nondeterministic order, risking DeviceChecksumView repopulation with stale data; change the enqueueing so checksum invalidation always runs before view invalidation by chaining the tasks (e.g., use Celery chain or apply_async with link/chain) instead of calling .delay independently, referencing bulk_invalidate_config_get_cached_checksum and invalidate_controller_views_for_group and ensuring the chain guarantees completion of the checksum task before the view invalidation task is scheduled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@openwisp_controller/config/base/device_group.py`:
- Around line 90-94: Two async tasks
(bulk_invalidate_config_get_cached_checksum.delay and
invalidate_controller_views_for_group.delay) are enqueued in nondeterministic
order, risking DeviceChecksumView repopulation with stale data; change the
enqueueing so checksum invalidation always runs before view invalidation by
chaining the tasks (e.g., use Celery chain or apply_async with link/chain)
instead of calling .delay independently, referencing
bulk_invalidate_config_get_cached_checksum and
invalidate_controller_views_for_group and ensuring the chain guarantees
completion of the checksum task before the view invalidation task is scheduled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d5f34fc8-c8ee-44c4-9512-61ddd9d3e4f1
📒 Files selected for processing (3)
openwisp_controller/config/base/device_group.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/tests/test_device.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/config/base/device_group.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/tests/test_device.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/config/base/device_group.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/tests/test_device.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/config/base/device_group.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/tests/test_device.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/config/base/device_group.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/tests/test_device.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/config/base/device_group.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/tests/test_device.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/config/base/device_group.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/tests/test_device.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/config/base/device_group.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/tests/test_device.py
📚 Learning: 2026-03-17T09:20:10.456Z
Learnt from: pandafy
Repo: openwisp/openwisp-controller PR: 1292
File: openwisp_controller/connection/tasks.py:27-31
Timestamp: 2026-03-17T09:20:10.456Z
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/config/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/config/tests/test_device.py
🔇 Additional comments (3)
openwisp_controller/config/tasks.py (1)
222-237: Good scoped cache invalidation for group context updates.This narrows invalidation to devices in the changed group and avoids unnecessary VPN/org-wide cache churn.
openwisp_controller/config/tests/test_device.py (2)
436-462: Regression test for org-context impact looks solid.The affected/unaffected status assertions are clear and directly validate the intended behavior.
463-493: Nice addition of explicit enqueue assertion for group view-cache invalidation.This protects the root fix path by asserting
invalidate_controller_views_for_group.delay(...)is triggered when group context changes.
QA, Test, and Commit Message FailuresHello @mn-ram,
A correct commit message format looks like this:
|
mn-ram
force-pushed
the
fix/invalidate-controller-views-cache-on-org-context-change
branch
from
449af8f to
e46e816
Compare
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/config/tasks.py`:
- Around line 222-237: The task decorator for
invalidate_controller_views_for_group should include a soft_time_limit to match
other long-running iteration tasks; update the `@shared_task`(...) call on the
invalidate_controller_views_for_group function to pass soft_time_limit=7200
(preserving base=OpenwispCeleryTask) so the task will have a soft timeout
safeguard for large device groups.
In `@openwisp_controller/config/tests/test_device.py`:
- Around line 436-461: The test test_status_update_on_org_variable_change
verifies config status changes but omits asserting that the org-level cache
invalidation task was enqueued; update the test to patch/mocking
invalidate_controller_views_cache.delay (same approach used in
test_status_update_on_group_variable_change), set
OrganizationConfigSettings.context, save, then assert
invalidate_controller_views_cache.delay was called (and with appropriate args if
required) after saving OrganizationConfigSettings; reference
OrganizationConfigSettings and invalidate_controller_views_cache.delay to locate
where to add the mock and assertion.
🪄 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: 1c23ae23-770b-4200-98fe-3b10bc9ef0e4
📒 Files selected for processing (3)
openwisp_controller/config/base/device_group.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/tests/test_device.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/config/base/device_group.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/tests/test_device.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/config/base/device_group.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/tests/test_device.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/config/base/device_group.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/tests/test_device.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/config/base/device_group.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/tests/test_device.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/config/base/device_group.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/tests/test_device.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/config/base/device_group.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/tests/test_device.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/config/base/device_group.pyopenwisp_controller/config/tasks.pyopenwisp_controller/config/tests/test_device.py
📚 Learning: 2026-03-17T09:20:10.456Z
Learnt from: pandafy
Repo: openwisp/openwisp-controller PR: 1292
File: openwisp_controller/connection/tasks.py:27-31
Timestamp: 2026-03-17T09:20:10.456Z
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/config/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/config/tests/test_device.py
🔇 Additional comments (3)
openwisp_controller/config/base/device_group.py (1)
18-21: LGTM! Group-scoped cache invalidation correctly implemented.The new
invalidate_controller_views_for_grouptask is appropriately imported and invoked with the group ID (str(self.id)), ensuring only devices in the affected group have theirDeviceChecksumViewcache invalidated. This addresses the previous performance concern about org-wide invalidation.Also applies to: 94-94
openwisp_controller/config/tasks.py (1)
223-229: Good documentation of task scope.The docstring clearly explains the difference from
invalidate_controller_views_cacheand notes that VPN caches are not invalidated. This helps maintainers understand the intentional scoping.openwisp_controller/config/tests/test_device.py (1)
463-495: LGTM! Comprehensive test with proper mock verification.The test correctly:
- Sets up a device group with two devices where only one config uses the variable
- Mocks the task at the correct import path in
device_groupmodule- Verifies the async task is called with the correct group ID
- Uses
subTestto clearly separate the status assertionsThis addresses the previous review comment requesting explicit assertion for controller-view cache invalidation.
| @shared_task(base=OpenwispCeleryTask) | ||
| def invalidate_controller_views_for_group(group_id): | ||
| """ | ||
| Invalidates the DeviceChecksumView cache only for devices in the given group. | ||
|
|
||
| Unlike invalidate_controller_views_cache, this is scoped to a single device | ||
| group and does not invalidate VPN caches. | ||
| """ | ||
| from .controller.views import DeviceChecksumView | ||
|
|
||
| Device = load_model("config", "Device") | ||
|
|
||
| for device in ( | ||
| Device.objects.filter(group_id=group_id).only("id").iterator() | ||
| ): | ||
| DeviceChecksumView.invalidate_get_device_cache(device) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding soft_time_limit for consistency with similar tasks.
The new task lacks the soft_time_limit parameter that other iteration-based tasks in this file use (e.g., invalidate_controller_views_cache implicitly inherits defaults, but tasks like update_template_related_config_status explicitly set soft_time_limit=7200). For large device groups, this task could run longer than expected without a safeguard.
💡 Suggested fix
-@shared_task(base=OpenwispCeleryTask)
+@shared_task(base=OpenwispCeleryTask, soft_time_limit=7200)
def invalidate_controller_views_for_group(group_id):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_controller/config/tasks.py` around lines 222 - 237, The task
decorator for invalidate_controller_views_for_group should include a
soft_time_limit to match other long-running iteration tasks; update the
`@shared_task`(...) call on the invalidate_controller_views_for_group function to
pass soft_time_limit=7200 (preserving base=OpenwispCeleryTask) so the task will
have a soft timeout safeguard for large device groups.
| def test_status_update_on_org_variable_change(self): | ||
| org = self._get_org() | ||
| cs = OrganizationConfigSettings.objects.create(organization=org, context={}) | ||
| c1 = self._create_config(organization=org) | ||
| c1.templates.add( | ||
| self._create_template( | ||
| name="t-with-var", | ||
| config={"interfaces": [{"name": "{{ ssid }}", "type": "ethernet"}]}, | ||
| default_values={"ssid": "eth0"}, | ||
| ) | ||
| ) | ||
| c1.set_status_applied() | ||
| d2 = self._create_device( | ||
| organization=org, name="d2", mac_address="00:11:22:33:44:56" | ||
| ) | ||
| c2 = self._create_config(device=d2) | ||
| c2.set_status_applied() | ||
| cs.context = {"ssid": "OrgA"} | ||
| cs.full_clean() | ||
| cs.save() | ||
| c1.refresh_from_db() | ||
| c2.refresh_from_db() | ||
| with self.subTest("affected config changes to modified"): | ||
| self.assertEqual(c1.status, "modified") | ||
| with self.subTest("unaffected config remains applied"): | ||
| self.assertEqual(c2.status, "applied") |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding explicit mock assertion for org-level cache invalidation.
The organization variable change test validates the status transition correctly, but unlike test_status_update_on_group_variable_change, it doesn't explicitly verify that invalidate_controller_views_cache.delay is called when OrganizationConfigSettings.context changes. Adding a mock assertion would strengthen the regression test for the PR's core fix.
💡 Suggested addition
cs.context = {"ssid": "OrgA"}
cs.full_clean()
- cs.save()
+ patch_path = (
+ "openwisp_controller.config.base.multitenancy"
+ ".invalidate_controller_views_cache"
+ )
+ with mock.patch(patch_path) as mocked_task:
+ cs.save()
+ mocked_task.delay.assert_called_once_with(str(org.id))
c1.refresh_from_db()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_controller/config/tests/test_device.py` around lines 436 - 461, The
test test_status_update_on_org_variable_change verifies config status changes
but omits asserting that the org-level cache invalidation task was enqueued;
update the test to patch/mocking invalidate_controller_views_cache.delay (same
approach used in test_status_update_on_group_variable_change), set
OrganizationConfigSettings.context, save, then assert
invalidate_controller_views_cache.delay was called (and with appropriate args if
required) after saving OrganizationConfigSettings; reference
OrganizationConfigSettings and invalidate_controller_views_cache.delay to locate
where to add the mock and assertion.
e9b0da0 to
8538212
Compare
…xt change openwisp#1070 When device group context variables are updated, bulk_invalidate_config_get_cached_checksum was already called to mark affected configs as modified, but invalidate_controller_views_cache was never called, leaving the DeviceChecksumView cache stale for devices in the group. Replace the org-wide invalidate_controller_views_cache with a new group-scoped task, invalidate_controller_views_for_group, that only clears DeviceChecksumView entries for devices in the changed group, avoiding unnecessary invalidation of unrelated devices and VPNs.
mn-ram
force-pushed
the
fix/invalidate-controller-views-cache-on-org-context-change
branch
from
8538212 to
4aff88c
Compare
…rg-context-change
mn-ram
changed the title
|
Tip For best results, initiate chat on the files or code changes.
[approve] |
|
Tip For best results, initiate chat on the files or code changes.
[approve] |
|
Hi @nemesifier, the root cause of #1070 was that Fix: call Would appreciate your review whenever you get a chance. Thank you! |
…rg-context-change
2 participants
Checklist
Reference to Existing Issue
Closes #1070.
Description of Changes
When organization or device group configuration variables are updated, only
bulk_invalidate_config_get_cached_checksumwas called to bust the Configchecksum cache. However, the
DeviceChecksumViewcontroller endpoint maintainsa separate cache that was never invalidated on this code path.
As a result, devices kept receiving the old (stale) checksum when polling the
controller, so they never transitioned to
modifiedstatus and never pulledthe updated configuration.
Root cause: two separate cache layers exist for configuration checksums:
bulk_invalidate_config_get_cached_checksuminvalidate_controller_views_cacheOnly layer 1 was being cleared; layer 2 was left stale.
Fix:
openwisp_controller/config/base/multitenancy.py— callinvalidate_controller_views_cachewhen org context changes, mirroringthe existing behaviour already done in
handlers.pywhen an organisationis disabled or enabled.
openwisp_controller/config/base/device_group.py— add a new group-scopedtask
invalidate_controller_views_for_groupinstead of the org-wideinvalidate_controller_views_cache, so only the DeviceChecksumView entriesfor devices in the changed group are cleared (avoids unnecessary invalidation
of unrelated devices and VPN caches).
openwisp_controller/config/tasks.py— new taskinvalidate_controller_views_for_group(group_id)that clearsDeviceChecksumViewonly for devices filtered bygroup_id.openwisp_controller/config/tests/test_device.py— regression tests forboth org and group variable changes; verifies only configs whose rendered
output is actually affected transition to
modified, and asserts thegroup-scoped task is enqueued with the correct
group_id.