[fix] Ensure VpnClient post_delete signal fires on template removal#1277
[fix] Ensure VpnClient post_delete signal fires on template removal#1277mn-ram wants to merge 2 commits 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:
📝 WalkthroughWalkthroughThe changes convert two bulk deletion operations into iterative per-object deletions for VPN client management. In openwisp_controller/config/api/serializers.py and openwisp_controller/config/base/config.py, VPN client querysets are now iterated and each instance's delete() is called inside atomic blocks instead of using queryset.delete(). Filtering logic and conditions are unchanged; only the deletion mechanism was modified to ensure per-object signals and side-effects run. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (2 passed)
✨ 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.
Actionable comments posted: 1
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/config.py (1)
337-343: 🧹 Nitpick | 🔵 TrivialConsider wrapping the deletion loop in an explicit atomic block.
The per-instance deletion correctly ensures
post_deletesignals fire for eachVpnClient. However, unlike the similar change inserializers.py(which useswith transaction.atomic():), this loop lacks an explicit transaction wrapper. If an error occurs mid-loop, some VPN clients may be deleted while others remain, potentially leaving the system in an inconsistent state.♻️ Proposed fix to add atomic block
if action == "post_clear": if instance.is_deactivating_or_deactivated(): # If the device is deactivated or in the process of deactivating, then # delete all vpn clients and return. - for vpnclient in instance.vpnclient_set.all().iterator(): - vpnclient.delete() + with transaction.atomic(): + for vpnclient in instance.vpnclient_set.all().iterator(): + vpnclient.delete() return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/config/base/config.py` around lines 337 - 343, The deletion loop for VPN clients when action == "post_clear" isn't wrapped in a transaction, so partial failures can leave some VpnClient rows deleted and others intact; update the block in config.py (the branch checking action == "post_clear" and instance.is_deactivating_or_deactivated()) to wrap the iteration over instance.vpnclient_set.all().iterator() in a Django transaction.atomic() context (import transaction from django.db) so all vpnclient.delete() calls are executed atomically and still allow post_delete signals to fire, then return as before.
🤖 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/config.py`:
- Around line 374-377: The deletion loop over
instance.vpnclient_set.exclude(template_id__in=instance.templates.values_list("id",
flat=True)).iterator() is not wrapped in an atomic transaction, risking partial
deletions; wrap that loop in a transaction.atomic() block to ensure all
deletions are committed or rolled back together (mirror the pattern used in
serializers.py), i.e., import transaction and execute the for vpnclient ...
vpnclient.delete() inside transaction.atomic() to ensure consistency.
---
Outside diff comments:
In `@openwisp_controller/config/base/config.py`:
- Around line 337-343: The deletion loop for VPN clients when action ==
"post_clear" isn't wrapped in a transaction, so partial failures can leave some
VpnClient rows deleted and others intact; update the block in config.py (the
branch checking action == "post_clear" and
instance.is_deactivating_or_deactivated()) to wrap the iteration over
instance.vpnclient_set.all().iterator() in a Django transaction.atomic() context
(import transaction from django.db) so all vpnclient.delete() calls are executed
atomically and still allow post_delete signals to fire, then return as before.
🪄 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: 9f633166-df85-44be-b897-9b5b8405e473
📒 Files selected for processing (2)
openwisp_controller/config/api/serializers.pyopenwisp_controller/config/base/config.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.1.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~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.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/config.pyopenwisp_controller/config/api/serializers.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/config.pyopenwisp_controller/config/api/serializers.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/config.pyopenwisp_controller/config/api/serializers.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/config.pyopenwisp_controller/config/api/serializers.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/config.pyopenwisp_controller/config/api/serializers.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/config.pyopenwisp_controller/config/api/serializers.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/config.pyopenwisp_controller/config/api/serializers.py
🔇 Additional comments (1)
openwisp_controller/config/api/serializers.py (1)
205-210: LGTM! Per-instance deletion properly wrapped in atomic block.The change correctly ensures
post_deletesignals fire for eachVpnClient. The existingtransaction.atomic()block at line 202 guarantees that all deletions are rolled back together if any fails, and using.iterator()is appropriate for memory efficiency when processing potentially large querysets.
CI Failures in OpenWISP ControllerHello @prakash-kalwaniya, Failures & Remediation:
|
mn-ram
force-pushed
the
fix/1221-vpn-peer-cache-desync
branch
from
5713199 to
6fc7dec
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
openwisp_controller/config/base/config.py (1)
337-343:⚠️ Potential issue | 🟠 MajorWrap both per-instance deletion loops in a single atomic boundary per branch.
If one
vpnclient.delete()fails mid-loop, earlier deletions may already be committed, leaving partial cleanup state. Make each loop all-or-nothing withtransaction.atomic().♻️ Proposed fix
if action == "post_clear": if instance.is_deactivating_or_deactivated(): # If the device is deactivated or in the process of deactivating, then # delete all vpn clients and return. - for vpnclient in instance.vpnclient_set.all().iterator(): - vpnclient.delete() + with transaction.atomic(): + for vpnclient in instance.vpnclient_set.all().iterator(): + vpnclient.delete() return @@ if len(pk_set) != templates.filter(required=True).count(): @@ - for vpnclient in instance.vpnclient_set.exclude( - template_id__in=instance.templates.values_list("id", flat=True) - ).iterator(): - vpnclient.delete() + with transaction.atomic(): + for vpnclient in instance.vpnclient_set.exclude( + template_id__in=instance.templates.values_list("id", flat=True) + ).iterator(): + vpnclient.delete()Also applies to: 374-377
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/config/base/config.py` around lines 337 - 343, The deletion loops that remove vpnclient records when handling the "post_clear" branch (the loop iterating over instance.vpnclient_set.all() guarded by instance.is_deactivating_or_deactivated()) — and the similar loop later in the file — must be wrapped in a single transaction boundary so each branch is all-or-nothing; update the code to import and use transaction.atomic() around each per-instance deletion loop (i.e., wrap the for vpnclient in instance.vpnclient_set.all().iterator(): vpnclient.delete() block inside a with transaction.atomic():) to ensure partial commits don't leave the DB in a half-cleaned state while keeping the existing instance.is_deactivating_or_deactivated() guard and vpnclient.delete() calls unchanged.
🤖 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/config/base/config.py`:
- Around line 337-343: The deletion loops that remove vpnclient records when
handling the "post_clear" branch (the loop iterating over
instance.vpnclient_set.all() guarded by
instance.is_deactivating_or_deactivated()) — and the similar loop later in the
file — must be wrapped in a single transaction boundary so each branch is
all-or-nothing; update the code to import and use transaction.atomic() around
each per-instance deletion loop (i.e., wrap the for vpnclient in
instance.vpnclient_set.all().iterator(): vpnclient.delete() block inside a with
transaction.atomic():) to ensure partial commits don't leave the DB in a
half-cleaned state while keeping the existing
instance.is_deactivating_or_deactivated() guard and vpnclient.delete() calls
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ba385d3b-af23-4f0d-97e3-2e620fcfe0a5
📒 Files selected for processing (2)
openwisp_controller/config/api/serializers.pyopenwisp_controller/config/base/config.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.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | 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/config/api/serializers.pyopenwisp_controller/config/base/config.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/api/serializers.pyopenwisp_controller/config/base/config.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/api/serializers.pyopenwisp_controller/config/base/config.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/api/serializers.pyopenwisp_controller/config/base/config.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/api/serializers.pyopenwisp_controller/config/base/config.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/api/serializers.pyopenwisp_controller/config/base/config.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/api/serializers.pyopenwisp_controller/config/base/config.py
🔇 Additional comments (1)
openwisp_controller/config/api/serializers.py (1)
205-210: Good change: per-instance deletion is correctly preserved inside the existing transaction block.This keeps cleanup side-effects tied to each
VpnClientdelete while retaining atomic update flow.
CI Failure AnalysisHello @prakash-kalwaniya, Failures & Remediation
|
mn-ram
force-pushed
the
fix/1221-vpn-peer-cache-desync
branch
from
6fc7dec to
a6a2c65
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/api/serializers.py`:
- Around line 205-208: The per-instance deletes on config.vpnclient_set
currently cause N+1 queries because VpnClient.post_delete accesses FK/OneToOne
relations; change the queryset to prefetch those relations before iterating by
using select_related('vpn', 'cert', 'ip') on config.vpnclient_set (e.g.
config.vpnclient_set.exclude(vpn__in=vpn_list).select_related('vpn', 'cert',
'ip').iterator()) so each related object is fetched in the initial query and
vpnclient.delete() no longer triggers extra queries.
In `@openwisp_controller/config/base/config.py`:
- Around line 341-343: The delete loops iterate over
instance.vpnclient_set.all().iterator() and a second similar loop that triggers
VpnClient.post_delete, causing N+1 queries when post_delete accesses
instance.vpn, instance.cert, and instance.ip; update both iterations to preload
those relations by calling .select_related("vpn", "cert", "ip") on the queryset
before .iterator() (i.e., replace instance.vpnclient_set.all().iterator() with
instance.vpnclient_set.select_related("vpn","cert","ip").iterator() in both
places) so related objects are fetched in a single query.
🪄 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: 6287c082-c206-4597-92fe-fc8fdb58aae5
📒 Files selected for processing (2)
openwisp_controller/config/api/serializers.pyopenwisp_controller/config/base/config.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.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.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.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.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/config/api/serializers.pyopenwisp_controller/config/base/config.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/api/serializers.pyopenwisp_controller/config/base/config.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/api/serializers.pyopenwisp_controller/config/base/config.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/api/serializers.pyopenwisp_controller/config/base/config.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/api/serializers.pyopenwisp_controller/config/base/config.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/api/serializers.pyopenwisp_controller/config/base/config.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/api/serializers.pyopenwisp_controller/config/base/config.py
mn-ram
force-pushed
the
fix/1221-vpn-peer-cache-desync
branch
from
a6a2c65 to
156a756
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
openwisp_controller/config/base/config.py (1)
341-343:⚠️ Potential issue | 🟠 MajorApply
select_related("vpn", "cert", "ip")in both delete loops to prevent repeated DB hits.At Line 342 and Line 376, per-instance deletes invoke
post_delete, which dereferences related objects. Without preloading, both loops can degrade into N+1 relation queries.♻️ Proposed fix
- for vpnclient in instance.vpnclient_set.all().iterator(): + for vpnclient in ( + instance.vpnclient_set.all() + .select_related("vpn", "cert", "ip") + .iterator() + ): vpnclient.delete() @@ - for vpnclient in instance.vpnclient_set.exclude( - template_id__in=instance.templates.values_list("id", flat=True) - ).iterator(): + for vpnclient in ( + instance.vpnclient_set.exclude( + template_id__in=instance.templates.values_list("id", flat=True) + ) + .select_related("vpn", "cert", "ip") + .iterator() + ): vpnclient.delete()#!/bin/bash # Verify both loops and whether related fields are preloaded. rg -n -C3 'vpnclient_set\.all\(\)\.iterator|vpnclient_set\.exclude\(|vpnclient\.delete\(\)|select_related\("vpn", "cert", "ip"\)' openwisp_controller/config/base/config.py rg -n -C3 'def post_delete|instance\.vpn|instance\.cert|instance\.ip' openwisp_controller/config/base/vpn.pyAs per coding guidelines: "Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries".
Also applies to: 375-379
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/config/base/config.py` around lines 341 - 343, The delete loops currently iterate over instance.vpnclient_set.all().iterator() and delete each vpnclient causing N+1 queries when post_delete accesses related vpn/cert/ip; fix by changing both loops to preload related objects via select_related("vpn", "cert", "ip") before iterating (e.g., use instance.vpnclient_set.select_related("vpn","cert","ip").all().iterator()) inside the existing transaction.atomic() blocks so each vpnclient.delete() does not trigger extra DB hits.openwisp_controller/config/api/serializers.py (1)
205-208:⚠️ Potential issue | 🟠 MajorPreload related fields before per-instance deletion to avoid N+1 queries.
At Line 205, each
vpnclient.delete()triggerspost_delete; that handler accessesinstance.vpn,instance.cert, andinstance.ip, which can cause repeated per-row relation fetches. Addselect_related("vpn", "cert", "ip")before.iterator().♻️ Proposed fix
- for vpnclient in config.vpnclient_set.exclude( - vpn__in=vpn_list - ).iterator(): + for vpnclient in ( + config.vpnclient_set.exclude(vpn__in=vpn_list) + .select_related("vpn", "cert", "ip") + .iterator() + ): vpnclient.delete()#!/bin/bash # Verify current queryset and signal-side relation dereferences. rg -n -C3 'config\.vpnclient_set\.exclude|vpnclient\.delete\(\)|select_related\("vpn", "cert", "ip"\)' openwisp_controller/config/api/serializers.py rg -n -C3 'def post_delete|instance\.vpn|instance\.cert|instance\.ip' openwisp_controller/config/base/vpn.pyAs per coding guidelines: "Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/config/api/serializers.py` around lines 205 - 208, The loop calling vpnclient.delete() on config.vpnclient_set.exclude(vpn__in=vpn_list).iterator() causes N+1 queries because post_delete handlers access related fields (instance.vpn, instance.cert, instance.ip); fix by preloading those relations: replace the queryset before .iterator() with one that calls select_related("vpn", "cert", "ip") so related objects are fetched in bulk prior to per-instance deletion (keep using vpnclient.delete() to trigger signals).
🤖 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/config/api/serializers.py`:
- Around line 205-208: The loop calling vpnclient.delete() on
config.vpnclient_set.exclude(vpn__in=vpn_list).iterator() causes N+1 queries
because post_delete handlers access related fields (instance.vpn, instance.cert,
instance.ip); fix by preloading those relations: replace the queryset before
.iterator() with one that calls select_related("vpn", "cert", "ip") so related
objects are fetched in bulk prior to per-instance deletion (keep using
vpnclient.delete() to trigger signals).
In `@openwisp_controller/config/base/config.py`:
- Around line 341-343: The delete loops currently iterate over
instance.vpnclient_set.all().iterator() and delete each vpnclient causing N+1
queries when post_delete accesses related vpn/cert/ip; fix by changing both
loops to preload related objects via select_related("vpn", "cert", "ip") before
iterating (e.g., use
instance.vpnclient_set.select_related("vpn","cert","ip").all().iterator())
inside the existing transaction.atomic() blocks so each vpnclient.delete() does
not trigger extra DB hits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9d124f9c-7aa1-4ffc-b196-ea53cf78e19a
📒 Files selected for processing (2)
openwisp_controller/config/api/serializers.pyopenwisp_controller/config/base/config.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.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.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~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.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/config.pyopenwisp_controller/config/api/serializers.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/config.pyopenwisp_controller/config/api/serializers.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/config.pyopenwisp_controller/config/api/serializers.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/config.pyopenwisp_controller/config/api/serializers.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/config.pyopenwisp_controller/config/api/serializers.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/config.pyopenwisp_controller/config/api/serializers.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/config.pyopenwisp_controller/config/api/serializers.py
This comment was marked as spam.
This comment was marked as spam.
CI Failure AnalysisHello @prakash-kalwaniya, Failures & Remediation
|
mn-ram
force-pushed
the
fix/1221-vpn-peer-cache-desync
branch
from
c2f224d to
16320a2
Compare
This comment was marked as spam.
This comment was marked as spam.
|
✅ Actions performedComments resolved and changes approved. |
…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. Uses cache.add() for atomic lock acquisition with UUID token-based ownership to prevent one task from releasing another's lock. Includes 5-minute TTL safety net and guaranteed release via finally block. Closes openwisp#1277
…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.
…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
Test Failure:
|
mn-ram
force-pushed
the
fix/1221-vpn-peer-cache-desync
branch
2 times, most recently
from
5711b36 to
7b202aa
Compare
mn-ram
force-pushed
the
fix/1221-vpn-peer-cache-desync
branch
2 times, most recently
from
cfcfb84 to
4d06878
Compare
Multiple QA Failures DetectedHello @mn-ram,
|
mn-ram
force-pushed
the
fix/1221-vpn-peer-cache-desync
branch
from
4d06878 to
19cd174
Compare
CI Failures: Formatting and Documentation IssuesHello @mn-ram,
|
…wisp#1221 Replaced QuerySet.delete() with per-instance deletion in all three locations where VpnClient objects are bulk-deleted so that post_delete signals fire for every instance, ensuring: - VPN peer cache is invalidated on the affected VPN server - Client certificates are revoked (OpenVPN, auto_cert=True) - IP addresses are released (Wireguard) Also fixed the serializers.py cleanup logic: vpn_list was built from the old templates so .exclude(vpn__in=vpn_list) never matched the client being removed. Now uses new_vpn_ids from config_templates (the incoming new set) so PATCH with an empty templates list also works. Added select_related("vpn", "cert", "ip") to all deletion querysets to avoid N+1 queries triggered by the post_delete signal handler. Wrapped deletion loops in transaction.atomic() to prevent partial deletions on failure. Fixes openwisp#1221 [backport 1.2]
mn-ram
force-pushed
the
fix/1221-vpn-peer-cache-desync
branch
from
19cd174 to
3396d94
Compare
Test Query Count MismatchHello @mn-ram, The test To fix this, you need to investigate the extra query being executed and optimize the code to meet the expected query count. Review the captured queries in the failure log to identify the redundant query. |
… in CI Bumps the SQLite connection timeout from 10 s to 20 s so that TransactionTestCase teardown/flush operations that briefly hold the write lock on slow CI runners no longer cause spurious "database is locked" errors.
mn-ram
force-pushed
the
fix/1221-vpn-peer-cache-desync
branch
3 times, most recently
from
76d5eb0 to
2093d3a
Compare
2 participants
[fix] VPN peers not removed when VPN template is detached
Closes #1221
Problem
QuerySet.delete()performs a bulk delete which bypasses Django's per-instancepost_deletesignals. Because of this, theVpnClient.post_deletehandler isnot triggered when VPN clients are deleted through bulk operations.
Impact
When a VPN template is detached or a device is deactivated:
This allows revoked devices to remain authorized on the VPN server until the
peer cache expires (up to 7 days).
Root Cause
Bulk deletion of
VpnClientobjects skips the modeldelete()method andtherefore the
post_deletesignal used for cleanup.Solution
Replace bulk
.delete()calls with per-instance deletion using.iterator()so that Django fires the
post_deletesignal for everyVpnClient.Additionally,
select_related("vpn", "cert", "ip")was added to the querysetsto prevent potential N+1 queries triggered by the
post_deletesignalhandler when accessing related objects.
Changes
Updated the following locations:
config/base/config.py(device deactivation)config/base/config.py(template synchronization / template removal)config/api/serializers.py(API configuration update)Example
Checklist
select_related