[fix] Fixed command/connection resurrection and cache interference in CI#1392
Conversation
manage_vpn_clients created the VpnClient only for the templates present in the current m2m_changed cycle's pk_set. A single template change fires several m2m cycles: SortedManyToManyField clears then re-adds the templates, plus the group-template remove/add performed in ConfigForm.save. The new VpnClient was therefore created only in the one cycle whose pk_set contained the template, so a missed or mis-ordered cycle could leave the config with the old VPN client. This surfaced as the flaky test_vpn_template_switch_checksum_db failure, where switching a device template from vpn1 to vpn2 intermittently kept vpn1 (<Vpn: vpn1> != <Vpn: vpn2>). The deletion side already reconciled against the full current template set. Make creation symmetric by iterating the currently linked VPN templates, guarded by the existing existence check, so the handler becomes an idempotent, self-healing reconcile across all the m2m cycles a single change triggers.
DeviceConnection.connect() records the connection attempt with save(). When a background command (launch_command) runs against a connection whose row was already deleted by a concurrent deletion or test teardown, Django's save() found no row to UPDATE and fell back to an INSERT, resurrecting the connection with a dangling device foreign key. That raised "FOREIGN KEY constraint failed"; launch_command caught it, but its recovery command.save() then failed too and the second error escaped. This surfaced as the flaky test_update_config_hostname_changed_on_reregister error, and as the noisy caught tracebacks seen even in green runs. Record the attempt only when the connection still exists, so a stale in-memory object can no longer resurrect a deleted row. Added a regression test that deletes the connection row before calling connect().
test_multiple_connections_receive_updates_with_redis triggered real command execution without mocking paramiko.SSHClient.connect, unlike the sibling test_new_command_created. Command execution runs eagerly when the command is created, so an un-mocked SSH connection attempt to the test device can block until its timeout, pushing the whole test past the 5s pytest-timeout and producing the flaky "Timeout (>5.0s) from pytest-timeout" failures seen in CI. Mock paramiko.SSHClient.connect so command execution is deterministic and fast, matching the sibling test and keeping real SSH out of the unit test.
API tests authenticate with force_login, which stores the session in the cache because SESSION_ENGINE is the cache backend. The session used the "default" cache, which the whois tests flush with cache.clear() and which parallel test workers share. A cache.clear() in one place therefore wiped the authenticated session of a test running concurrently, so a later request returned 401 instead of the expected status. This produced the flaky "401 != 400" failures in test_multiple_vpn_client_templates_same_vpn. Keep sessions in a dedicated "sessions" cache so clearing the default cache can no longer drop them. Added a regression test that clears the default cache between two authenticated requests.
|
Caution Review failedThe pull request is closed. 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:
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR prevents DeviceConnection/Command rows from being resurrected by concurrent save() calls by adding a resurrection-safe helper and using it in connect()/execute(); isolates test session storage to a dedicated Redis cache and updates tests; makes VPN client auto-cert creation reconcile against the instance’s VPN templates; and warms OrganizationGeoSettings cache in geo tests to stabilize query-count assertions. Sequence Diagram(s)sequenceDiagram
participant Caller
participant AbstractDeviceConnection
participant ParamikoSSHClient
participant Database
Caller->>AbstractDeviceConnection: dc.connect()
AbstractDeviceConnection->>ParamikoSSHClient: SSHClient.connect(...)
ParamikoSSHClient-->>AbstractDeviceConnection: success or exception
alt new instance (_state.adding)
AbstractDeviceConnection->>Database: save() (INSERT)
else existing instance
AbstractDeviceConnection->>Database: save(force_update=True) (UPDATE)
Database-->>AbstractDeviceConnection: success or DatabaseError
alt DatabaseError and row missing
Database-->>AbstractDeviceConnection: row not found
AbstractDeviceConnection-->>Caller: ignore (no resurrection)
else DatabaseError and row exists
AbstractDeviceConnection-->>Caller: re-raise DatabaseError
end
end
AbstractDeviceConnection-->>Caller: return / exception
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 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: 2
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)
381-385:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid N existence queries while reconciling full VPN template set.
This now scans the full VPN template queryset and runs one
.exists()query per template, which is repeated DB I/O in a signal hot path. Prefetch existing template IDs once, then create only missing clients.Proposed optimization
- for template in instance.templates.filter(type="vpn"): - # Create VPN client if needed - if not vpn_client_model.objects.filter( - config=instance, vpn=template.vpn, template=template - ).exists(): - client = vpn_client_model( - config=instance, - vpn=template.vpn, - template=template, - auto_cert=template.auto_cert, - ) - client.full_clean() - client.save() + vpn_templates = instance.templates.filter(type="vpn").select_related("vpn") + existing_template_ids = set( + vpn_client_model.objects.filter(config=instance).values_list( + "template_id", flat=True + ) + ) + for template in vpn_templates: + if template.pk in existing_template_ids: + continue + client = vpn_client_model( + config=instance, + vpn=template.vpn, + template=template, + auto_cert=template.auto_cert, + ) + client.full_clean() + client.save()As per coding guidelines, "Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries."
Also applies to: 386-393
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openwisp_controller/config/base/config.py` around lines 381 - 385, The loop over instance.templates.filter(type="vpn") does an .exists() DB query per template; instead, fetch all VPN templates and prefetch existing vpn_client_model entries in one query: build template_ids = list(instance.templates.filter(type="vpn").values_list('id', flat=True')), get existing_template_ids = set(vpn_client_model.objects.filter(config=instance, template_id__in=template_ids).values_list('template_id', flat=True')), then iterate the templates and only create vpn_client_model objects for templates whose id is not in existing_template_ids; apply the same pattern for the other block (lines 386-393) to avoid N additional queries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openwisp_controller/connection/base/models.py`:
- Around line 357-365: The current exists() pre-check is TOCTOU-prone because
Django may fall back to INSERT; replace the conditional self.save() call with a
save(force_update=True) to prevent INSERT fallback, and wrap it in a try/except
that catches Django's NotUpdated exception in addition to DatabaseError (or the
DB-specific exception your Django version raises) to handle the no-row-updated
case gracefully; adjust the block around _state.adding and
type(self)._base_manager.filter(pk=self.pk).exists() so you still skip when
adding, but when not adding call save(force_update=True) and handle
NotUpdated/DatabaseError accordingly.
In `@openwisp_controller/connection/tests/pytest.py`:
- Around line 81-84: The patched async test functions decorated with
`@mock.patch`("paramiko.SSHClient.connect") are missing the injected mock
parameter in their signatures, which causes a positional-args TypeError; update
the signatures of test_multiple_connections_receive_updates_with_redis and
test_new_command_created to accept the injected mock (e.g., add a parameter like
mock_connect as the first argument after self) so the patched mock is received
and the tests run correctly.
---
Outside diff comments:
In `@openwisp_controller/config/base/config.py`:
- Around line 381-385: The loop over instance.templates.filter(type="vpn") does
an .exists() DB query per template; instead, fetch all VPN templates and
prefetch existing vpn_client_model entries in one query: build template_ids =
list(instance.templates.filter(type="vpn").values_list('id', flat=True')), get
existing_template_ids = set(vpn_client_model.objects.filter(config=instance,
template_id__in=template_ids).values_list('template_id', flat=True')), then
iterate the templates and only create vpn_client_model objects for templates
whose id is not in existing_template_ids; apply the same pattern for the other
block (lines 386-393) to avoid N additional queries.
🪄 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: a905a9bb-6dab-47c9-9195-26d23f2378a5
📒 Files selected for processing (7)
openwisp_controller/config/base/config.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/connection/base/models.pyopenwisp_controller/connection/tests/pytest.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/geo/estimated_location/tests/tests.pytests/openwisp2/settings.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.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.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~=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.10 | django~=5.2.0
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/connection/base/models.pyopenwisp_controller/config/tests/test_api.pytests/openwisp2/settings.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/base/config.pyopenwisp_controller/connection/tests/pytest.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/connection/base/models.pyopenwisp_controller/config/tests/test_api.pytests/openwisp2/settings.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/base/config.pyopenwisp_controller/connection/tests/pytest.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/connection/base/models.pyopenwisp_controller/config/tests/test_api.pytests/openwisp2/settings.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/base/config.pyopenwisp_controller/connection/tests/pytest.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/connection/base/models.pyopenwisp_controller/config/tests/test_api.pytests/openwisp2/settings.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/base/config.pyopenwisp_controller/connection/tests/pytest.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
Avoid unnecessary blank lines inside function and method bodies
Be careful with authentication, authorization, queryset filtering, serializers, admin behavior, cache invalidation, signals, Celery tasks, and websocket updates in Django code
Preserve validation around templates, VPN/PKI material, SSH credentials, device commands, uploaded files, URLs, and subnet/IP data
Write comments and docstrings only when they explain why code is shaped a certain way, placing them before the relevant code block instead of scattering them inside it
Files:
openwisp_controller/connection/base/models.pyopenwisp_controller/config/tests/test_api.pytests/openwisp2/settings.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/base/config.pyopenwisp_controller/connection/tests/pytest.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/connection/base/models.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/base/config.pyopenwisp_controller/connection/tests/pytest.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/connection/base/models.pyopenwisp_controller/config/tests/test_api.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/base/config.pyopenwisp_controller/connection/tests/pytest.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/connection/base/models.pyopenwisp_controller/config/tests/test_api.pytests/openwisp2/settings.pyopenwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/geo/estimated_location/tests/tests.pyopenwisp_controller/config/base/config.pyopenwisp_controller/connection/tests/pytest.py
📚 Learning: 2026-03-27T20:50:26.240Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1315
File: openwisp_controller/geo/estimated_location/service.py:70-76
Timestamp: 2026-03-27T20:50:26.240Z
Learning: In openwisp-controller’s WHOIS and estimated-location services (openwisp_controller/config/whois/ and openwisp_controller/geo/estimated_location/), these components only process public IP addresses. When reviewing logs/error/debug messages in this area, treat logging the IP address as acceptable and do not flag it as a privacy/security concern—unless the logged value can originate from non-public/private IPs in that specific code path.
Applied to files:
openwisp_controller/geo/estimated_location/tests/tests.py
🔇 Additional comments (4)
tests/openwisp2/settings.py (1)
169-180: LGTM!Also applies to: 191-191
openwisp_controller/config/tests/test_api.py (1)
1345-1355: LGTM!openwisp_controller/connection/tests/test_tasks.py (1)
185-196: LGTM!openwisp_controller/geo/estimated_location/tests/tests.py (1)
243-251: LGTM!Also applies to: 459-460, 484-485, 509-510
…ce_update The previous exists() pre-check was time-of-check-to-time-of-use racy: between the check and save() the row could still be deleted, and save() would then fall back to an INSERT and resurrect the connection. Use save(force_update=True) so Django performs an UPDATE only and never an INSERT, and ignore the DatabaseError raised when there is no row to update. New connections (self._state.adding) still use a normal save().
@mock.patch injects the mock as a positional argument, which was binding to admin_user instead of being captured explicitly. It was harmless only because admin_user was unused, but it left the wrong value in that parameter. Add an explicit mocked_connect parameter so the fixtures admin_user, admin_client and settings receive their real values.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openwisp_controller/connection/base/models.py`:
- Around line 365-368: In AbstractDeviceConnection.connect(), don’t swallow all
DatabaseError around the self.save(force_update=True) call; instead catch
DatabaseError as e, detect the specific deleted/missing-row case by checking
self.__class__.objects.filter(pk=self.pk).exists(), handle that case as before
(graceful return) but for any other DatabaseError log at error level (use module
logger = logging.getLogger(__name__)) with context (e and object id) and
re-raise the exception so real DB failures surface; ensure logging import is
present and only the missing-row branch is treated as non-fatal.
🪄 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: 254ef9e3-2826-4d16-8c3b-0c2aded29415
📒 Files selected for processing (2)
openwisp_controller/connection/base/models.pyopenwisp_controller/connection/tests/pytest.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). (12)
- 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~=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.10 | 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.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/connection/base/models.pyopenwisp_controller/connection/tests/pytest.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/connection/base/models.pyopenwisp_controller/connection/tests/pytest.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/connection/base/models.pyopenwisp_controller/connection/tests/pytest.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/connection/base/models.pyopenwisp_controller/connection/tests/pytest.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
Avoid unnecessary blank lines inside function and method bodies
Be careful with authentication, authorization, queryset filtering, serializers, admin behavior, cache invalidation, signals, Celery tasks, and websocket updates in Django code
Preserve validation around templates, VPN/PKI material, SSH credentials, device commands, uploaded files, URLs, and subnet/IP data
Write comments and docstrings only when they explain why code is shaped a certain way, placing them before the relevant code block instead of scattering them inside it
Files:
openwisp_controller/connection/base/models.pyopenwisp_controller/connection/tests/pytest.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/connection/base/models.pyopenwisp_controller/connection/tests/pytest.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/connection/base/models.pyopenwisp_controller/connection/tests/pytest.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/connection/base/models.pyopenwisp_controller/connection/tests/pytest.py
🔇 Additional comments (1)
openwisp_controller/connection/tests/pytest.py (1)
71-72: LGTM!Also applies to: 81-83
The previous "except DatabaseError: pass" swallowed every database error, hiding real persistence failures while connect() still returned based on the in-memory state. Only the deleted-row race should be ignored: re-raise (after logging) when the connection row still exists, and ignore the error only when the row is gone. Added a regression test asserting that a genuine database error is propagated.
Removed an em dash from a comment in manage_vpn_clients. Moved a cache import to the module top and dropped a blank line inside a test body, per the project style. Corrected the cache-eviction explanation in the estimated-location test helper: the shared cache is cleared by other tests (e.g. whois), it is not LocMemCache culling.
The earlier connect() fix only covered DeviceConnection resurrection. A background launch_command also resurrects a deleted Command through the final save() in execute(), and that FOREIGN KEY IntegrityError fires inside the selenium suite (in the live-server thread), where on SQLite it can leave the shared test database in a broken or locked state and make the live server hang, producing the flaky selenium timeouts that the retry mechanism cannot recover from. Extract the resurrection-safe save into a shared helper and use it in both connect() and execute(), so neither path can fall back to a resurrecting INSERT. Added a regression test for the command path.
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/connection/base/models.py (1)
549-576:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDeleted commands can still be sent to the device.
launch_command()fetches theCommandand then callsexecute(). If that row is deleted after the fetch but before this method starts,_exec_command()still runs and can perform the remote action; the new helper only prevents the trailingsave()from recreating the row. Please re-check that the command still exists before_exec_command()and bail out when it has already been deleted.As per coding guidelines, "Be careful with authentication, authorization, queryset filtering, serializers, admin behavior, cache invalidation, signals, Celery tasks, and websocket updates in Django code".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c258e49c-9962-4a89-880d-0c1c45750428
📒 Files selected for processing (2)
openwisp_controller/connection/base/models.pyopenwisp_controller/connection/tests/test_tasks.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: Python==3.11 | 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.10 | django~=5.1.0
- GitHub Check: Python==3.11 | 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.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/base/models.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/base/models.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/base/models.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/base/models.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
Avoid unnecessary blank lines inside function and method bodies
Be careful with authentication, authorization, queryset filtering, serializers, admin behavior, cache invalidation, signals, Celery tasks, and websocket updates in Django code
Preserve validation around templates, VPN/PKI material, SSH credentials, device commands, uploaded files, URLs, and subnet/IP data
Write comments and docstrings only when they explain why code is shaped a certain way, placing them before the relevant code block instead of scattering them inside it
Files:
openwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/base/models.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/connection/tests/test_tasks.pyopenwisp_controller/connection/base/models.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/base/models.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/connection/tests/test_tasks.pyopenwisp_controller/connection/base/models.py
github-project-automation
Bot
moved this to In progress
in OpenWISP Priorities for next releases
launch_command fetches the command and calls execute(); if the command was deleted after being scheduled (e.g. by a user or a cascading delete) while the background task was queued, _exec_command() still performed the remote action on the device. Bail out of execute() when the command no longer exists, before performing the remote action. The resurrection-safe save remains as defense in depth for a deletion that races the execution.
…mand launch_command's three exception handlers persisted the failed status with a plain command.save(). If the command was deleted while execute() was running (a long SSH run, a soft time limit, or a command timeout) and execute() then raised, that save fell back to an INSERT and resurrected the deleted command: the same FOREIGN KEY error that can corrupt the live-server DB on SQLite. Route those saves through the _save_without_resurrecting helper (imported lazily to avoid the models and tasks circular import) so the handlers update the row only when it still exists. Added a regression test.
Added a _save_without_resurrecting method on DeviceConnection and Command that delegates to the module-level helper, so launch_command can call it without importing it (the lazy import is gone). Also used instance._meta.model.objects instead of type(instance)._base_manager for the existence check (more readable and consistent, and these models have no custom manager), and converted the _warm_org_geo_settings_cache comment to a docstring.
github-project-automation
Bot
moved this from In progress
to Reviewer approved
in OpenWISP Priorities for next releases
nemesifier
changed the title
|
@coderabbitai full review, please resolve if no new issues are found |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Read all changed files
echo "=== config/base/config.py (around manage_vpn_clients) ==="
grep -n "manage_vpn_clients\|post_add\|pk_set\|vpn\|templates" openwisp_controller/config/base/config.py | head -80
echo ""
echo "=== Full manage_vpn_clients function ==="
grep -n "" openwisp_controller/config/base/config.py | grep -A 80 "def manage_vpn_clients"
echo ""
echo "=== connection/base/models.py - new helper and changes ==="
grep -n "" openwisp_controller/connection/base/models.py | grep -B5 -A 50 "_save_without_resurrecting\|DatabaseError"
echo ""
echo "=== connection/tests/test_tasks.py - new tests ==="
grep -n "" openwisp_controller/connection/tests/test_tasks.py | tail -80
echo ""
echo "=== connection/tests/pytest.py ==="
cat openwisp_controller/connection/tests/pytest.py
echo ""
echo "=== config/tests/test_api.py - new test ==="
grep -n "test_session_survives\|cache\|SESSION" openwisp_controller/config/tests/test_api.py | head -30
echo ""
echo "=== geo/estimated_location/tests/tests.py ==="
grep -n "_warm_org_geo_settings_cache\|assertNumQueries\|check_estimated_location_enabled" openwisp_controller/geo/estimated_location/tests/tests.py | head -30
echo ""
echo "=== tests/openwisp2/settings.py - sessions cache ==="
grep -n "CACHES\|SESSION\|sessions\|redis" tests/openwisp2/settings.pyLength of output: 33499 🏁 Script executed: #!/bin/bash
# Read the full relevant sections of each changed file
echo "=== manage_vpn_clients full function ==="
python3 - <<'EOF'
import re
with open("openwisp_controller/config/base/config.py") as f:
content = f.read()
# Find manage_vpn_clients
idx = content.find("def manage_vpn_clients")
if idx >= 0:
# Print 100 lines from there
lines = content[idx:idx+4000]
print(lines)
EOF
echo ""
echo "=== connection/base/models.py full _save_without_resurrecting ==="
python3 - <<'EOF'
with open("openwisp_controller/connection/base/models.py") as f:
content = f.read()
idx = content.find("_save_without_resurrecting")
if idx >= 0:
# Find the function start
start = content.rfind("\ndef ", 0, idx)
if start < 0:
start = idx
print(content[start:start+2000])
EOF
echo ""
echo "=== connection/base/models.py connect() ==="
python3 - <<'EOF'
with open("openwisp_controller/connection/base/models.py") as f:
content = f.read()
idx = content.find("def connect(")
if idx >= 0:
print(content[idx:idx+2000])
EOF
echo ""
echo "=== connection/base/models.py execute() ==="
python3 - <<'EOF'
with open("openwisp_controller/connection/base/models.py") as f:
content = f.read()
idx = content.find("def execute(")
if idx >= 0:
print(content[idx:idx+2000])
EOFLength of output: 11556 🏁 Script executed: #!/bin/bash
echo "=== Full test_tasks.py new regression tests ==="
grep -n "" openwisp_controller/connection/tests/test_tasks.py | grep -A 200 "resurrect"
echo ""
echo "=== Full settings.py ==="
cat tests/openwisp2/settings.py | grep -A 50 "CACHES"
echo ""
echo "=== config/tests/test_api.py new test ==="
grep -n "" openwisp_controller/config/tests/test_api.py | grep -A 20 "test_session_survives"
echo ""
echo "=== geo tests warmup helper ==="
grep -n "" openwisp_controller/geo/estimated_location/tests/tests.py | grep -B2 -A 30 "_warm_org_geo_settings_cache"Length of output: 12712
One very minor observation: the existence check uses
No new issues found. Resolving all open comments. [resolve] |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get the PR diff and changed files
gh pr view 1392 --repo openwisp/openwisp-controller --json files,commits,body | jq '.files[].path'Length of output: 564 🏁 Script executed: #!/bin/bash
# Read changed files
cat openwisp_controller/config/base/config.py | grep -n "manage_vpn_clients" -A 50 | head -100Length of output: 2669 🏁 Script executed: #!/bin/bash
# Read the connection models file
cat openwisp_controller/connection/base/models.py | grep -n "_save_without_resurrecting\|DatabaseError\|def connect\|def execute" -A 30 | head -200Length of output: 7481 🏁 Script executed: #!/bin/bash
# Read test_tasks.py for new tests
cat openwisp_controller/connection/tests/test_tasks.py | grep -n "resurrect\|does_not_resurrect\|reraises" -A 30 | head -150Length of output: 3847 🏁 Script executed: #!/bin/bash
# Read settings.py for sessions cache changes
cat tests/openwisp2/settings.pyLength of output: 11008 🏁 Script executed: #!/bin/bash
# Read the pytest.py changes
cat openwisp_controller/connection/tests/pytest.py | grep -n "paramiko\|mocked_connect\|test_new_command\|test_multiple_connections" -A 15 | head -100Length of output: 2346 🏁 Script executed: #!/bin/bash
# Read geo tests
cat openwisp_controller/geo/estimated_location/tests/tests.py | grep -n "_warm_org_geo_settings_cache\|test_estimated_location_creation_and_update" -A 20 | head -100Length of output: 5171 🏁 Script executed: #!/bin/bash
# Read api test
cat openwisp_controller/config/tests/test_api.py | grep -n "test_session_survives\|cache.clear\|import cache" -A 15 | head -60Length of output: 2252 🏁 Script executed: #!/bin/bash
# Read tasks.py - especially the launch_command function
cat openwisp_controller/connection/tasks.pyLength of output: 3851 🏁 Script executed: #!/bin/bash
# Check the full _save_without_resurrecting implementation and nearby context
cat openwisp_controller/connection/base/models.py | sed -n '38,60p'Length of output: 938 🏁 Script executed: #!/bin/bash
# Check the full execute() method
cat openwisp_controller/connection/base/models.py | grep -n "def execute" -A 60 | head -70Length of output: 2790 🏁 Script executed: #!/bin/bash
# Check the manage_vpn_clients post_add section more fully
cat openwisp_controller/config/base/config.py | sed -n '360,400p'Length of output: 2107
All regression tests — well-structured, covering both the resurrection-prevention path and the genuine-error re-raise path ( No new issues found. Resolving all comments. ✅ Action performedFull review finished. |
github-project-automation
Bot
moved this from Reviewer approved
to Done
in OpenWISP Priorities for next releases
github-project-automation
Bot
moved this from Ready for review/testing
to Done
in 26.06 Release
|
/backport 1.2 |
2 participants
Checklist
Reference to Existing Issue
There is no dedicated open issue. This addresses flaky failures observed across recent CI builds (failures by org members that passed on retry, plus a couple of master builds).
Description of Changes
Fixes the most frequent flaky tests found by analyzing failed CI builds that later passed on retry:
VpnClient not switching on template change (
subnet_division ... test_vpn_template_switch_checksum_db):manage_vpn_clientscreated the VpnClient only for the templates in the currentm2m_changedcycle'spk_set. A single template change fires several m2m cycles (SortedManyToManyField clears then re-adds, plus the group-template remove/add inConfigForm.save), so a missed or mis-ordered cycle could leave the old VPN client. Creation now reconciles against the full current set of VPN templates, guarded by the existing existence check, so it is idempotent and symmetric with the deletion side.FOREIGN KEY constraint failedfrom background commands (connection ... test_update_config_hostname_changed_on_reregister): a backgroundlaunch_commandcould runconnect()against aDeviceConnectionwhose row had already been deleted;save()then found no row to update and fell back to an INSERT, resurrecting the connection with a dangling device foreign key.connect()now records the attempt only if the connection still exists. This also removes the noisy caught tracebacks that appeared even in green runs.Redis websocket consumer timeout (
connection ... test_multiple_connections_receive_updates_with_redis): the test did not mockparamiko.SSHClient.connect(unlike the siblingtest_new_command_created), so eager command execution could block on a real SSH attempt and push the test past the 5s pytest-timeout. The SSH connect is now mocked.Flaky
401responses in API tests (config ... test_multiple_vpn_client_templates_same_vpn): sessions were stored in the default cache, which the whois tests flush withcache.clear()and which parallel workers share, so a clear in one place wiped the authenticated session of a test running concurrently. Sessions now use a dedicated cache.Flaky
assertNumQueries(geo.estimated_location ... test_estimated_location_creation_and_update): an extraOrganizationGeoSettingsquery appeared when the cached entry was dropped between subtests. The relevant cache is now warmed before the assertion.Regression tests were added where the failure could be reproduced deterministically.
Screenshot
N/A