Reconcile stuck "modified" config status on device checksum request#1330
Reconcile stuck "modified" config status on device checksum request#1330MichaelUray wants to merge 1 commit intoopenwisp:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a reconciliation step to DeviceChecksumView: after emitting the Sequence Diagram(s)sequenceDiagram
participant Client
participant DeviceChecksumView
participant CachedConfig
participant Database
Client->>DeviceChecksumView: GET /controller/device_checksum
activate DeviceChecksumView
DeviceChecksumView->>DeviceChecksumView: emit checksum_requested
DeviceChecksumView->>CachedConfig: check device.config (cached)
alt no config or status != "modified"
DeviceChecksumView-->>Client: return checksum response
else status == "modified"
DeviceChecksumView->>DeviceChecksumView: if now - cached.modified < 300s -> return
alt within grace
DeviceChecksumView-->>Client: return checksum response
else grace exceeded
DeviceChecksumView->>Database: transaction + select_for_update() refetch Config
activate Database
Database-->>DeviceChecksumView: latest config (status, modified)
deactivate Database
alt still status == "modified" and grace exceeded
DeviceChecksumView->>Database: call set_status_applied() (UPDATE)
Database-->>DeviceChecksumView: persist success
DeviceChecksumView->>DeviceChecksumView: log info
else
Note over DeviceChecksumView: no change (already applied)
end
DeviceChecksumView-->>Client: return checksum response
end
end
deactivate DeviceChecksumView
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Changes ReviewedThis is the same major refactor reviewed previously - no new changes since last review. The PR moves estimated location functionality from config to geo module with clean separation of concerns:
Architecture: Clean separation between WHOIS lookup (config) and estimated location (geo), signal-based communication, proper cache invalidation. All Issues Resolved
Files Reviewed (44 files)Key files:
Reviewed by kimi-k2.5-0127 · 235,789 tokens |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_controller/config/controller/views.py`:
- Line 191: Move the inline import of timezone out of the function and into the
module-level imports: add "from django.utils import timezone" to the top import
block of this module and remove the in-body "from django.utils import timezone"
statement currently inside the view method (where the inline import appears) so
the view functions use the module-level timezone symbol instead.
- Around line 193-201: The cached device.config may be stale; before checking
status and calling config.set_status_applied() inside
DeviceChecksumView._reconcile_modified_status (or where get_device() is used),
reload the related Config from the DB to get the current status: e.g., fetch
Config.objects.select_for_update()/get(pk=device.config.pk) or call
device.config.refresh_from_db() (or re-query via Config model) and then
re-evaluate config.status and grace logic, ensuring you operate on the fresh
record before calling config.set_status_applied().
In `@openwisp_controller/config/tests/test_controller.py`:
- Line 279: The import PropertyMock in
openwisp_controller/config/tests/test_controller.py is unused; remove
PropertyMock from the import statement (the line that currently reads "from
unittest.mock import PropertyMock") so only used mocks remain, ensuring there
are no lint/unused-import failures related to PropertyMock in the test module.
🪄 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: 82c81550-bb41-4990-bcea-5aa10c631f7e
📒 Files selected for processing (2)
openwisp_controller/config/controller/views.pyopenwisp_controller/config/tests/test_controller.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~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.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~=4.2.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/controller/views.pyopenwisp_controller/config/tests/test_controller.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/controller/views.pyopenwisp_controller/config/tests/test_controller.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/controller/views.pyopenwisp_controller/config/tests/test_controller.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/controller/views.pyopenwisp_controller/config/tests/test_controller.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/controller/views.pyopenwisp_controller/config/tests/test_controller.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/controller/views.pyopenwisp_controller/config/tests/test_controller.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/controller/views.pyopenwisp_controller/config/tests/test_controller.py
🔇 Additional comments (6)
openwisp_controller/config/tests/test_controller.py (2)
273-305: Test logic is well-structured and comprehensive.The test correctly verifies:
- Initial request within grace period keeps status as "modified"
- After backdating beyond the grace period (600s > 300s), status is reconciled to "applied"
Using
Config.objects.filter().update()to backdate the timestamp is an appropriate approach for testing time-dependent behavior.
307-327: Edge case tests are correctly implemented.Both tests properly verify the negative cases:
test_device_checksum_no_reconcile_for_applied: Confirms "applied" status remains unchangedtest_device_checksum_no_reconcile_within_grace_period: Confirms no premature reconciliationopenwisp_controller/config/controller/views.py (4)
200-208: Signal emission is appropriate for status reconciliation.Calling
set_status_applied()will emit theconfig_status_changedsignal, which is the correct behavior. This ensures downstream systems (notifications, Celery tasks, etc.) are notified when the config status is reconciled, maintaining consistency with the normalreport_statusflow.The
logger.infomessage provides good observability for operators to track reconciliation events.
209-213: Exception handling is appropriately defensive.The broad
Exceptioncatch withlogger.exception()ensures:
- Reconciliation failures don't break the checksum response (device continues operating normally)
- Full tracebacks are logged for debugging
This is the correct approach for a "best effort" reconciliation feature.
147-152: Grace period constant is well-documented.The comment clearly explains the rationale for the 5-minute grace period. The underscore prefix appropriately marks it as private.
For future flexibility, consider making this configurable via
app_settings, but the hardcoded default is acceptable for the initial implementation.
163-169: Integration point for reconciliation is appropriate.Placing the reconciliation call after
checksum_requestedsignal and before returning ensures:
- The checksum response is always returned (reconciliation failures are caught internally)
- The device is provably online (checksum request proves active polling)
- Signal handlers can observe the original state before reconciliation
Code Style and Commit Message FailuresHello @MichaelUray, There are several code style and formatting issues detected by
Fix:
For example: |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_controller/config/controller/views.py`:
- Around line 201-210: Wrap the pre-checks that currently call
Config.objects.get(device=device) and compute elapsed = (timezone.now() -
config.modified).total_seconds() inside a try/except that catches broad
runtime/DB exceptions, logs the error at error level with the exception details,
and returns gracefully so checksum responses are not turned into 500s;
specifically protect the Config.objects.get call and the use of config.modified
(referencing Config.objects.get and
DeviceChecksumView._STATUS_RECONCILE_GRACE_SECONDS/config.modified) and apply
the same change to the similar block around the later lines (the other pre-check
at 220-224) so both reconciliation pre-check failures are logged and
short-circuited without raising.
- Around line 195-203: The local import "from ..models import Config" inside the
view bypasses the module-level swapped Config loaded via swapper; remove that
local import and use the existing module-level Config variable instead (the same
Config referenced at module top, loaded via swapper) in the code block where you
fetch the config (the try/except that calls Config.objects.get(device=device));
ensure no new local Config import remains so swapped models are respected.
In `@openwisp_controller/config/tests/test_controller.py`:
- Around line 297-299: Tests set a hardcoded 600s grace window when updating
Config.modified; replace the magic number with the production view grace
constant (e.g. VIEW_GRACE_SECONDS) to keep tests robust: change
timedelta(seconds=600) to timedelta(seconds=VIEW_GRACE_SECONDS) in the
Config.objects.filter(...).update(...) calls (the occurrence around Config in
test_controller.py and the second occurrence at the noted lines), and add the
corresponding import for VIEW_GRACE_SECONDS from the module that defines the
view constant so the tests use the same value as the view.
🪄 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: c0848535-acad-4c03-803b-ee9a29188839
📒 Files selected for processing (2)
openwisp_controller/config/controller/views.pyopenwisp_controller/config/tests/test_controller.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.2.0
- GitHub Check: Python==3.10 | 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.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Kilo Code Review
🧰 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/tests/test_controller.pyopenwisp_controller/config/controller/views.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/tests/test_controller.pyopenwisp_controller/config/controller/views.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/tests/test_controller.pyopenwisp_controller/config/controller/views.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/tests/test_controller.pyopenwisp_controller/config/controller/views.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/tests/test_controller.pyopenwisp_controller/config/controller/views.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/tests/test_controller.pyopenwisp_controller/config/controller/views.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/tests/test_controller.pyopenwisp_controller/config/controller/views.py
🔇 Additional comments (1)
openwisp_controller/config/tests/test_controller.py (1)
329-357: Good cache-bypass coverage for the reconciliation path.This test validates the exact stale-cache failure mode and confirms DB-first reconciliation behavior.
| Config.objects.filter(pk=c.pk).update( | ||
| modified=timezone.now() - timedelta(seconds=600) | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Replace hardcoded 600 with the view grace constant to keep tests future-proof.
Using the production constant avoids brittle tests if the grace window changes.
♻️ Proposed fix
Config.objects.filter(pk=c.pk).update(
- modified=timezone.now() - timedelta(seconds=600)
+ modified=timezone.now()
+ - timedelta(
+ seconds=DeviceChecksumView._STATUS_RECONCILE_GRACE_SECONDS + 1
+ )
)
...
Config.objects.filter(pk=c.pk).update(
- modified=timezone.now() - timedelta(seconds=600)
+ modified=timezone.now()
+ - timedelta(
+ seconds=DeviceChecksumView._STATUS_RECONCILE_GRACE_SECONDS + 1
+ )
)Also applies to: 348-350
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_controller/config/tests/test_controller.py` around lines 297 - 299,
Tests set a hardcoded 600s grace window when updating Config.modified; replace
the magic number with the production view grace constant (e.g.
VIEW_GRACE_SECONDS) to keep tests robust: change timedelta(seconds=600) to
timedelta(seconds=VIEW_GRACE_SECONDS) in the
Config.objects.filter(...).update(...) calls (the occurrence around Config in
test_controller.py and the second occurrence at the noted lines), and add the
corresponding import for VIEW_GRACE_SECONDS from the module that defines the
view constant so the tests use the same value as the view.
Code Style and Commit Message FailuresHello @MichaelUray, There are several issues with the code style and commit message:
|
…enwisp#1330 When a device applies a new configuration but fails to report its status back (e.g. due to a transient HTTP 502 from the controller), the config remains in "modified" state on the server forever. The device's agent will compare its local checksum with the remote checksum on the next polling cycle and find them identical, so it will not re-download or re-report. Without manual intervention the status stays "modified" indefinitely. Detect this condition on the DeviceChecksumView: if the config has been in "modified" state longer than a 5 minute grace period and the device is actively polling (proven by the very checksum request we are handling), set the status to "applied". Implementation notes: - Use the cached device object's config status as a fast path: if the cached status is not "modified" we return immediately with zero extra database queries, preserving the existing zero-query guarantee of the cached checksum path. - Only when the cached status says "modified" do we re-query Config fresh from the database. This covers the edge case where the cache has been populated with a "modified" status that was already reconciled a moment ago by a concurrent request. - The re-query uses .only() on the fields we need to keep it cheap. - A 5 minute grace period avoids fighting an in-flight apply that has not had time to report yet. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
MichaelUray
force-pushed
the
fix/reconcile-config-status-on-checksum-request
branch
from
5270e09 to
2ecf5a1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
openwisp_controller/config/tests/test_controller.py (1)
297-299:⚠️ Potential issue | 🟡 MinorUse the view grace constant instead of
600.This test is coupling itself to the current grace window. If
_STATUS_RECONCILE_GRACE_SECONDSchanges, it will fail even though the behavior is still correct.♻️ Proposed fix
Config.objects.filter(pk=c.pk).update( - modified=timezone.now() - timedelta(seconds=600) + modified=timezone.now() + - timedelta( + seconds=DeviceChecksumView._STATUS_RECONCILE_GRACE_SECONDS + 1 + ) )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/config/tests/test_controller.py` around lines 297 - 299, Replace the hardcoded 600 seconds in the test with the view grace constant _STATUS_RECONCILE_GRACE_SECONDS: import _STATUS_RECONCILE_GRACE_SECONDS from the module where it's defined, then set modified=timezone.now() - timedelta(seconds=_STATUS_RECONCILE_GRACE_SECONDS) in the Config.objects.filter(...).update(...) call so the test tracks the configured grace window instead of a magic number.openwisp_controller/config/controller/views.py (1)
213-220:⚠️ Potential issue | 🟠 MajorDon't let best-effort reconciliation fail the checksum endpoint.
The
Config.objects.get(...)/ elapsed-time pre-check can still raise before you reach the protectedset_status_applied()block. A transient DB/runtime failure here turns a hot polling endpoint into a 500 even though reconciliation is optional.♻️ Proposed fix
- try: - config = Config.objects.only("id", "status", "modified").get(device=device) - except Config.DoesNotExist: - return - if config.status != "modified": - return - grace = DeviceChecksumView._STATUS_RECONCILE_GRACE_SECONDS - elapsed = (timezone.now() - config.modified).total_seconds() - if elapsed < grace: - return try: + config = Config.objects.only("id", "status", "modified").get( + device=device + ) + if config.status != "modified": + return + grace = DeviceChecksumView._STATUS_RECONCILE_GRACE_SECONDS + elapsed = (timezone.now() - config.modified).total_seconds() + if elapsed < grace: + return + except Config.DoesNotExist: + return + except Exception: + logger.exception( + "Failed to reconcile config status pre-check for device %s", + device, + ) + return + try: config.set_status_applied()As per coding guidelines, "New code must handle errors properly: log errors that cannot be resolved by the user with error level..."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/config/controller/views.py` around lines 213 - 220, The DB lookup and elapsed-time check for reconciliation can raise and must not bubble up; wrap the Config.objects.only(...).get(device=device) and the elapsed computation in a broad try/except that catches exceptions from the ORM/timezone operations, log the exception at error level (including context like device and that reconciliation is best-effort) and return early so the checksum endpoint remains healthy; keep the existing path that calls DeviceChecksumView.set_status_applied() when status is "modified" and elapsed exceeds DeviceChecksumView._STATUS_RECONCILE_GRACE_SECONDS.
🤖 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/controller/views.py`:
- Around line 213-224: The read-check-write race in DeviceChecksumView allows
two requests to observe config.status == "modified" and both call
Config.set_status_applied(); make the transition atomic by performing a
conditional update inside a transaction (or using the ORM filter/update) so only
one request succeeds: load the Config
(Config.objects.only("id","status","modified").get(device=device)), compute the
grace check, then perform Config.objects.filter(id=config.id,
status="modified").update(status="applied") (or wrap a select_for_update() in
transaction.atomic and call set_status_applied() only if the database row still
matches) and only emit config_status_changed when the update affected 1 row; use
Config.set_status_applied() or its logic only after confirming the conditional
update succeeded.
---
Duplicate comments:
In `@openwisp_controller/config/controller/views.py`:
- Around line 213-220: The DB lookup and elapsed-time check for reconciliation
can raise and must not bubble up; wrap the
Config.objects.only(...).get(device=device) and the elapsed computation in a
broad try/except that catches exceptions from the ORM/timezone operations, log
the exception at error level (including context like device and that
reconciliation is best-effort) and return early so the checksum endpoint remains
healthy; keep the existing path that calls
DeviceChecksumView.set_status_applied() when status is "modified" and elapsed
exceeds DeviceChecksumView._STATUS_RECONCILE_GRACE_SECONDS.
In `@openwisp_controller/config/tests/test_controller.py`:
- Around line 297-299: Replace the hardcoded 600 seconds in the test with the
view grace constant _STATUS_RECONCILE_GRACE_SECONDS: import
_STATUS_RECONCILE_GRACE_SECONDS from the module where it's defined, then set
modified=timezone.now() - timedelta(seconds=_STATUS_RECONCILE_GRACE_SECONDS) in
the Config.objects.filter(...).update(...) call so the test tracks the
configured grace window instead of a magic number.
🪄 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: b21b8852-13e8-423c-9bdb-bb532646bd81
📒 Files selected for processing (2)
openwisp_controller/config/controller/views.pyopenwisp_controller/config/tests/test_controller.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.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | 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.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.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/tests/test_controller.pyopenwisp_controller/config/controller/views.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/tests/test_controller.pyopenwisp_controller/config/controller/views.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/tests/test_controller.pyopenwisp_controller/config/controller/views.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/tests/test_controller.pyopenwisp_controller/config/controller/views.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/tests/test_controller.pyopenwisp_controller/config/controller/views.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/tests/test_controller.pyopenwisp_controller/config/controller/views.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/tests/test_controller.pyopenwisp_controller/config/controller/views.py
🔇 Additional comments (3)
openwisp_controller/config/tests/test_controller.py (2)
283-293: This regression test hits the stale-cache path well.The first checksum request warms
get_device(), and the second request verifies that reconciliation still works once the database state has moved past the grace window.Also applies to: 303-306
308-329: The no-op coverage is useful.These two tests pin the intended exits for already
"applied"configs and for requests that arrive before the grace window expires.openwisp_controller/config/controller/views.py (1)
147-166: The reconciliation hook is placed sensibly.Running it during checksum GET keeps the fix server-side and transparent to existing agents.
Commit Message FailureHello @MichaelUray, (Analysis for commit 2ecf5a1) Your commit message is missing a closing keyword and issue number in the body. Here's an example of the correct format: |
…enwisp#1330 When a device applies a new configuration but fails to report its status back (e.g. due to a transient HTTP 502 from the controller), the config remains in "modified" state on the server forever. The device's agent will compare its local checksum with the remote checksum on the next polling cycle and find them identical, so it will not re-download or re-report. Without manual intervention the status stays "modified" indefinitely. Detect this condition on the DeviceChecksumView: if the config has been in "modified" state longer than a 5 minute grace period and the device is actively polling (proven by the very checksum request we are handling), set the status to "applied". Implementation notes: - Use the cached device object's config status as a fast path: if the cached status is not "modified" we return immediately with zero extra database queries, preserving the existing zero-query guarantee of the cached checksum path. - Only when the cached status says "modified" do we re-query Config fresh from the database. This covers the edge case where the cache has been populated with a "modified" status that was already reconciled a moment ago by a concurrent request. - The re-query uses .only() on the fields we need to keep it cheap. - A 5 minute grace period avoids fighting an in-flight apply that has not had time to report yet. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
MichaelUray
force-pushed
the
fix/reconcile-config-status-on-checksum-request
branch
from
2ecf5a1 to
5bc5967
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
openwisp_controller/config/controller/views.py (2)
224-235: 🧹 Nitpick | 🔵 TrivialConsider atomic update to prevent duplicate signal emissions.
Two concurrent checksum requests past the grace window could both read
status="modified"and both callset_status_applied(), causing double signal emission. While the end state is correct, downstream signal handlers would run twice.A conditional update could ensure only one request wins:
♻️ Suggested atomic approach
# Instead of: config.set_status_applied() # Use conditional update: from django.db import transaction with transaction.atomic(): updated = Config.objects.filter( pk=config.pk, status="modified" ).update(status="applied") if updated: # Only emit signal if we actually changed the status config.status = "applied" config._send_config_status_changed_signal()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/config/controller/views.py` around lines 224 - 235, Race condition: two concurrent handlers can both read Config with status="modified" and each call set_status_applied(), causing duplicate signals; fix by performing an atomic conditional update and only emitting the signal when the update actually changed a row. Wrap the change in a transaction.atomic(), run Config.objects.filter(pk=config.pk, status="modified").update(status="applied"), check the returned count (updated > 0), and only then set config.status = "applied" and call the internal signal emitter (e.g. _send_config_status_changed_signal()) instead of unconditionally calling set_status_applied().
197-197: 🧹 Nitpick | 🔵 TrivialMove
timezoneimport to module level for consistency.The inline import could be moved to the module-level imports for consistency with the rest of the codebase.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/config/controller/views.py` at line 197, Move the inline "from django.utils import timezone" import up to the module-level imports in openwisp_controller/config/controller/views.py and remove the inline import at its current location; ensure the module-level imports include "from django.utils import timezone" so functions that reference timezone (e.g., the view containing the original inline import) use the top-level symbol consistently with the rest of the file.
🤖 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/tests/test_controller.py`:
- Around line 273-306: The test backdates Config.modified via
Config.objects.update(), which bypasses signals and leaves the cached device
used by get_device() stale so _reconcile_modified_status() takes the fast path;
after the DB update and before the second client.get() you must invalidate the
device cache so get_device() loads fresh state — update the
test_device_checksum_reconciles_modified_status test to clear the same cache key
get_device() uses (or call the model helper that invalidates cache, e.g.,
Device.invalidate_cache(d.pk) if available) after
Config.objects.filter(pk=c.pk).update(...), then perform the second request and
assert status == "applied".
---
Duplicate comments:
In `@openwisp_controller/config/controller/views.py`:
- Around line 224-235: Race condition: two concurrent handlers can both read
Config with status="modified" and each call set_status_applied(), causing
duplicate signals; fix by performing an atomic conditional update and only
emitting the signal when the update actually changed a row. Wrap the change in a
transaction.atomic(), run Config.objects.filter(pk=config.pk,
status="modified").update(status="applied"), check the returned count (updated >
0), and only then set config.status = "applied" and call the internal signal
emitter (e.g. _send_config_status_changed_signal()) instead of unconditionally
calling set_status_applied().
- Line 197: Move the inline "from django.utils import timezone" import up to the
module-level imports in openwisp_controller/config/controller/views.py and
remove the inline import at its current location; ensure the module-level
imports include "from django.utils import timezone" so functions that reference
timezone (e.g., the view containing the original inline import) use the
top-level symbol consistently with the rest of the file.
🪄 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: 568c9196-7a6e-49c3-9d88-15c7cf97d3e9
📒 Files selected for processing (2)
openwisp_controller/config/controller/views.pyopenwisp_controller/config/tests/test_controller.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.11 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.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.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.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/tests/test_controller.pyopenwisp_controller/config/controller/views.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/tests/test_controller.pyopenwisp_controller/config/controller/views.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/tests/test_controller.pyopenwisp_controller/config/controller/views.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/tests/test_controller.pyopenwisp_controller/config/controller/views.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/tests/test_controller.pyopenwisp_controller/config/controller/views.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/tests/test_controller.pyopenwisp_controller/config/controller/views.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/tests/test_controller.pyopenwisp_controller/config/controller/views.py
🔇 Additional comments (5)
openwisp_controller/config/tests/test_controller.py (2)
308-317: LGTM!Test correctly verifies that configs already in
"applied"status are not modified by the reconciliation logic.
319-328: LGTM!Test correctly verifies that configs within the grace period are not prematurely reconciled.
openwisp_controller/config/controller/views.py (3)
147-152: LGTM!Clear documentation of the grace period constant and its purpose. The 5-minute window is a reasonable default to avoid interfering with normal apply/report cycles.
166-169: LGTM!Good placement of the reconciliation call after the signal emission.
199-247: Well-structured best-effort reconciliation with proper error handling.The implementation correctly:
- Uses fast paths to minimize DB queries for the common case
- Re-reads from DB to bypass long-TTL cache before reconciliation
- Wraps everything in try/except to ensure checksum endpoint never fails due to reconciliation
- Logs both success and failure appropriately
Code Style and Commit Message FailuresHello @MichaelUray, (Analysis for commit 5bc5967) There are two main issues with your commit:
|
MichaelUray
force-pushed
the
fix/reconcile-config-status-on-checksum-request
branch
from
5bc5967 to
2ecf5a1
Compare
Commit Message Convention FailureHello @MichaelUray, (Analysis for commit 2ecf5a1) The commit message is missing a reference to the issue it resolves in the body. Fix: Add a line in the commit message body that references the issue number. Example of a correct commit message: |
When a device applies a new configuration but fails to report its status back (e.g. due to a transient HTTP 502 from the controller), the config remains in "modified" state on the server forever. The device's agent will compare its local checksum with the remote checksum on the next polling cycle and find them identical, so it will not re-download or re-report. Without manual intervention the status stays "modified" indefinitely. Detect this condition on the DeviceChecksumView: if the config has been in "modified" state longer than a 5 minute grace period and the device is actively polling (proven by the very checksum request we are handling), set the status to "applied". Implementation notes: - Use the cached device object's config status as a fast path: if the cached status is not "modified" we return immediately with zero extra database queries, preserving the existing zero-query guarantee of the cached checksum path. - A second fast path checks the cached "modified" timestamp: if the grace period has not yet elapsed based on the cached timestamp alone, we skip the DB query entirely. - Only when both fast paths pass do we re-query Config fresh from the database inside a transaction with select_for_update to avoid race conditions with concurrent requests. - The re-query uses .only() on the fields we need to keep it cheap. - A 5 minute grace period avoids fighting an in-flight apply that has not had time to report yet. - The entire method is wrapped in try/except to ensure reconciliation never fails the checksum endpoint. Closes openwisp#1330
MichaelUray
force-pushed
the
fix/reconcile-config-status-on-checksum-request
branch
from
2ecf5a1 to
963ecb1
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
openwisp_controller/config/tests/test_controller.py (1)
295-309:⚠️ Potential issue | 🟠 MajorTest will fail: cache not invalidated after backdating timestamp.
The
Config.objects.filter().update()at line 297 modifies the database directly, bypassing the Django ORM and cache invalidation. The second request at line 306 reuses the cached device object (from the first request), which still hasconfig.modified ≈ now. In_reconcile_modified_status(), the fast path 2 computescached_elapsedas the seconds elapsed since the first request—a small number—which is less than the 300-second grace period, causing an early return without hitting the slow path that performs reconciliation.Invalidate the device cache after backdating so the second request fetches a fresh device object with the updated timestamp:
Proposed fix
Config.objects.filter(pk=c.pk).update( modified=timezone.now() - timedelta( seconds=DeviceChecksumView._STATUS_RECONCILE_GRACE_SECONDS + 300 ) ) + DeviceChecksumView.invalidate_get_device_cache(instance=d) # Second request after grace period: status should be reconciled # to "applied"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/config/tests/test_controller.py` around lines 295 - 309, The test fails because Config.objects.filter(...).update(...) bypasses the ORM cache so the subsequent request still uses a cached device with the old modified timestamp and _reconcile_modified_status() fast-path returns early; after backdating the Config.modified you should invalidate the device cache before making the second GET: call Django's cache.clear() or delete the specific cache entry used by the view (ensure you import django.core.cache.cache and invoke cache.clear() or cache.delete(<device-cache-key>)) immediately after the Config.objects.filter(...).update(...) so DeviceChecksumView will load a fresh device and respect DeviceChecksumView._STATUS_RECONCILE_GRACE_SECONDS during reconciliation.
🤖 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/tests/test_controller.py`:
- Around line 295-309: The test fails because
Config.objects.filter(...).update(...) bypasses the ORM cache so the subsequent
request still uses a cached device with the old modified timestamp and
_reconcile_modified_status() fast-path returns early; after backdating the
Config.modified you should invalidate the device cache before making the second
GET: call Django's cache.clear() or delete the specific cache entry used by the
view (ensure you import django.core.cache.cache and invoke cache.clear() or
cache.delete(<device-cache-key>)) immediately after the
Config.objects.filter(...).update(...) so DeviceChecksumView will load a fresh
device and respect DeviceChecksumView._STATUS_RECONCILE_GRACE_SECONDS during
reconciliation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6fb3ffdf-87b3-4792-b6af-1581653a4529
📒 Files selected for processing (2)
openwisp_controller/config/controller/views.pyopenwisp_controller/config/tests/test_controller.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.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.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | 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/tests/test_controller.pyopenwisp_controller/config/controller/views.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/tests/test_controller.pyopenwisp_controller/config/controller/views.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/tests/test_controller.pyopenwisp_controller/config/controller/views.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/tests/test_controller.pyopenwisp_controller/config/controller/views.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/tests/test_controller.pyopenwisp_controller/config/controller/views.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/tests/test_controller.pyopenwisp_controller/config/controller/views.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/tests/test_controller.pyopenwisp_controller/config/controller/views.py
🔇 Additional comments (2)
openwisp_controller/config/controller/views.py (1)
148-251: Well-designed reconciliation mechanism with proper safeguards.The implementation correctly addresses all concerns from previous reviews:
- Module-level
timezoneimport and use of swapper-loadedConfig- Atomicity via
select_for_update()insidetransaction.atomic()- Best-effort exception handling to never break the checksum endpoint
- Fast paths to minimize DB queries for the common case (
status != "modified"or within grace period)The
only("id", "status", "modified")clause is safe here sinceset_status_applied()only accessesstatusand saves withupdate_fields=["status"]when transitioning from "modified" to "applied".openwisp_controller/config/tests/test_controller.py (1)
311-331: LGTM!These two tests correctly validate the non-reconciliation paths:
test_device_checksum_no_reconcile_for_appliedverifies fast path 1 (status already "applied")test_device_checksum_no_reconcile_within_grace_periodverifies fast path 2 (within grace period)Neither test is affected by caching since they don't modify the DB after the initial request.
Code Style and Commit Message FailuresHello @MichaelUray,
Here's an example of a correct commit message: |
…enwisp#1330 When a device applies a new configuration but fails to report its status back (e.g. due to a transient HTTP 502 from the controller), the config remains in "modified" state on the server forever. The device's agent will compare its local checksum with the remote checksum on the next polling cycle and find them identical, so it will not re-download or re-report. Without manual intervention the status stays "modified" indefinitely. Detect this condition on the DeviceChecksumView: if the config has been in "modified" state longer than a 5 minute grace period and the device is actively polling (proven by the very checksum request we are handling), set the status to "applied". Implementation notes: - Use the cached device object's config status as a fast path: if the cached status is not "modified" we return immediately with zero extra database queries, preserving the existing zero-query guarantee of the cached checksum path. - A second fast path checks the cached "modified" timestamp: if the grace period has not yet elapsed based on the cached timestamp alone, we skip the DB query entirely. - Only when both fast paths pass do we re-query Config fresh from the database inside a transaction with select_for_update to avoid race conditions with concurrent requests. - The re-query uses .only() on the fields we need to keep it cheap. - A 5 minute grace period avoids fighting an in-flight apply that has not had time to report yet. - The entire method is wrapped in try/except to ensure reconciliation never fails the checksum endpoint. Closes openwisp#1330
MichaelUray
force-pushed
the
fix/reconcile-config-status-on-checksum-request
branch
from
963ecb1 to
abbe513
Compare
CI Failure: Code Style (Black)Hello @MichaelUray, The CI build failed due to a Fix: |
…enwisp#1330 When a device applies a new configuration but fails to report its status back (e.g. due to a transient HTTP 502 from the controller), the config remains in "modified" state on the server forever. The device's agent will compare its local checksum with the remote checksum on the next polling cycle and find them identical, so it will not re-download or re-report. Without manual intervention the status stays "modified" indefinitely. Detect this condition on the DeviceChecksumView: if the config has been in "modified" state longer than a 5 minute grace period and the device is actively polling (proven by the very checksum request we are handling), set the status to "applied". Implementation notes: - Use the cached device object's config status as a fast path: if the cached status is not "modified" we return immediately with zero extra database queries, preserving the existing zero-query guarantee of the cached checksum path. - A second fast path checks the cached "modified" timestamp: if the grace period has not yet elapsed based on the cached timestamp alone, we skip the DB query entirely. - Only when both fast paths pass do we re-query Config fresh from the database inside a transaction with select_for_update to avoid race conditions with concurrent requests. - The re-query uses .only() on the fields we need to keep it cheap. - A 5 minute grace period avoids fighting an in-flight apply that has not had time to report yet. - The entire method is wrapped in try/except to ensure reconciliation never fails the checksum endpoint. Closes openwisp#1330
MichaelUray
force-pushed
the
fix/reconcile-config-status-on-checksum-request
branch
from
abbe513 to
df87f47
Compare
Test Failures in OpenWISP ControllerHello @MichaelUray,
|
…enwisp#1330 When a device applies a new configuration but fails to report its status back (e.g. due to a transient HTTP 502 from the controller), the config remains in "modified" state on the server forever. The device's agent will compare its local checksum with the remote checksum on the next polling cycle and find them identical, so it will not re-download or re-report. Without manual intervention the status stays "modified" indefinitely. Detect this condition on the DeviceChecksumView: if the config has been in "modified" state longer than a 5 minute grace period and the device is actively polling (proven by the very checksum request we are handling), set the status to "applied". Implementation notes: - Use the cached device object's config status as a fast path: if the cached status is not "modified" we return immediately with zero extra database queries, preserving the existing zero-query guarantee of the cached checksum path. - A second fast path checks the cached "modified" timestamp: if the grace period has not yet elapsed based on the cached timestamp alone, we skip the DB query entirely. - Only when both fast paths pass do we re-query Config fresh from the database inside a transaction with select_for_update to avoid race conditions with concurrent requests. - The re-query uses .only() on the fields we need to keep it cheap. - A 5 minute grace period avoids fighting an in-flight apply that has not had time to report yet. - The entire method is wrapped in try/except to ensure reconciliation never fails the checksum endpoint. Closes openwisp#1330
MichaelUray
force-pushed
the
fix/reconcile-config-status-on-checksum-request
branch
from
df87f47 to
3b12254
Compare
Test Failure in
|
1 participant
Summary
When a device applies a new configuration but fails to report its status back to the controller (e.g., due to a transient HTTP 502 error during server maintenance), the config status remains
modifiedindefinitely.Root Cause
The device agent (openwisp-config) compares its local checksum with the remote checksum on subsequent polling cycles. Since the checksums match (the device already has the current config), the agent skips re-downloading and never calls
report_status. The controller keeps showingmodifiedeven though the device is up to date.While the agent does delete checksums when
report_statusfails (triggering a re-download on the next cycle), this self-healing mechanism can fail when:Fix
In
DeviceChecksumView.get(), after returning the checksum, check if the device's config has been inmodifiedstate for longer than a grace period (5 minutes). If so, and the device is actively polling (proven by this checksum request), reconcile the status toapplied.The grace period ensures we don't prematurely reconcile during the normal apply cycle (download → apply → report).
Backward Compatibility
This fix is entirely server-side and requires no changes to the openwisp-config agent. It works with all existing agent versions.
Tests
Added 3 unit tests:
test_device_checksum_reconciles_modified_status: Verifies reconciliation after grace periodtest_device_checksum_no_reconcile_for_applied: Verifies no false reconciliationtest_device_checksum_no_reconcile_within_grace_period: Verifies grace period is respected