[change] Send VPN sync errors/recovery as generic_message notifications #1049#1147
[change] Send VPN sync errors/recovery as generic_message notifications #1049#1147nemesifier merged 21 commits intomasterfrom
Conversation
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
The test failure is due to openwisp/django-x509@d9b8913.
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
@stktyagi I cherry-picked your last commit to fix the CI build on top of master, can you please rebase on master and remove that commit from this branch?
Fixed all failing tests that were failing due to inconsistencies with recent updates in notification handling, logging, and retry mechanisms. Fixes #1049
stktyagi
force-pushed
the
issues/1049-send-generic-message
branch
from
624eb0c to
9f31948
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughNotification utilities (send_api_task_notification, handle_error_notification, handle_recovery_notification) were added and wired into VPN and ZeroTier task flows. External HTTP POST calls now use Sequence Diagram(s)sequenceDiagram
participant Task as API Task (VPN / ZeroTier)
participant HTTP as External HTTP Endpoint
participant Cache as Django Cache
participant Notify as openwisp_notifications
participant Log as Logger
Note over Task,HTTP: Build & send HTTP POST (timeout=30, verify=not DEBUG)
Task->>HTTP: POST (payload, timeout=30, verify=not DEBUG)
alt HTTP raises RequestException
HTTP-->>Task: RequestException
Task->>Log: warning (include resource id & error)
Task->>Cache: get(task_key)
Cache-->>Task: previous_state
alt previous_state != "error"
Task->>Cache: set(task_key, "error")
Task->>Notify: handle_error_notification(task_key, sleep_time?, exception=..., ...)
Notify-->>Task: error notification dispatched
else
Note over Task: duplicate notification suppressed by cache
end
else 2xx Response
HTTP-->>Task: Response (2xx)
Task->>Log: info (include resource id)
Task->>Cache: set(task_key, "success")
Task->>Notify: handle_recovery_notification(task_key, sleep_time?, ...)
Notify-->>Task: recovery notification dispatched
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (4)📓 Common learnings📚 Learning: 2026-02-05T20:06:01.589ZApplied to files:
📚 Learning: 2026-01-15T15:05:49.557ZApplied to files:
📚 Learning: 2026-01-15T15:07:17.354ZApplied to files:
🧬 Code graph analysis (1)openwisp_controller/config/tasks_zerotier.py (1)
⏰ 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). (6)
🔇 Additional comments (3)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
openwisp_controller/config/tasks.py (1)
143-154: Consider using f-string conversion flag for cleaner exception formatting.Using
{e!s}is more idiomatic thanstr(e)in f-strings.🔎 Proposed fix
except requests.RequestException as e: logger.warning( f"Failed to update VPN Server configuration. " - f"Error: {str(e)}, " + f"Error: {e!s}, " f"VPN Server UUID: {vpn_id}" )openwisp_controller/config/utils.py (1)
214-255: Guard against missingactionparameter to prevent AttributeError.If
kwargs.get("action")returnsNone, calling.replace("_", " ")will raise anAttributeError. While current callers always provideaction, defensive coding would prevent future regressions.🔎 Proposed fix
def send_api_task_notification(type, **kwargs): vpn = kwargs.get("instance") - action = kwargs.get("action").replace("_", " ") + action = kwargs.get("action", "").replace("_", " ") exception = kwargs.get("exception")openwisp_controller/config/tests/test_vpn.py (1)
731-736: Consider using method-level mock creation for better test isolation.Class-level
mock.Mock()objects are shared across all test methods in the class. While this works for read-only usage, if any test modifiesmock_responseproperties (e.g.,status_code), it could affect subsequent tests. Consider creating the mock insetUpor within individual test methods for better isolation.🔎 Example using setUp
class TestWireguardTransaction(BaseTestVpn, TestWireguardVpnMixin, TransactionTestCase): def setUp(self): super().setUp() self.mock_response = mock.Mock(spec=requests.Response) self.mock_response.status_code = 200 self.mock_response.raise_for_status = mock.Mock()Also applies to: 999-1004
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
openwisp_controller/config/tasks.pyopenwisp_controller/config/tasks_zerotier.pyopenwisp_controller/config/tests/test_notifications.pyopenwisp_controller/config/tests/test_vpn.pyopenwisp_controller/config/utils.py
🧰 Additional context used
🧬 Code graph analysis (4)
openwisp_controller/config/tasks_zerotier.py (1)
openwisp_controller/config/utils.py (2)
handle_error_notification(265-269)handle_recovery_notification(258-262)
openwisp_controller/config/utils.py (1)
openwisp_controller/config/controller/views.py (4)
get(147-161)get(199-207)get(524-530)get(538-546)
openwisp_controller/config/tests/test_vpn.py (2)
openwisp_controller/config/base/vpn.py (1)
post_save(905-916)openwisp_controller/subnet_division/base/models.py (1)
post_save(226-236)
openwisp_controller/config/tasks.py (1)
openwisp_controller/config/utils.py (2)
handle_error_notification(265-269)handle_recovery_notification(258-262)
🪛 Ruff (0.14.10)
openwisp_controller/config/tasks.py
137-137: Probable use of requests call without timeout
(S113)
140-140: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
146-146: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (10)
openwisp_controller/config/tasks.py (2)
12-13: LGTM!The new imports for notification utilities are correctly added and align with the centralized notification pattern.
155-161: LGTM!The success path correctly logs the action and triggers recovery notification with the appropriate task key and parameters.
openwisp_controller/config/tasks_zerotier.py (2)
13-13: LGTM!Import correctly added for the new notification utilities.
62-77: LGTM!The notification handling is correctly refactored:
handle_recovery_notificationis called on success with appropriate kwargslogger.warningcorrectly replaces the deprecatedlogger.warnhandle_error_notificationis called for unrecoverable errors with the exception includedopenwisp_controller/config/utils.py (2)
3-11: LGTM!New imports correctly added for cache, translation, and notification functionality.
258-269: LGTM with a consideration for cache cleanup.The cache-based state tracking pattern is well-designed:
handle_recovery_notificationonly sends success if previous state was "error"handle_error_notificationonly sends error if not already in "error" stateThis prevents notification flooding effectively. Note that using
timeout=Nonemeans cache entries persist indefinitely. If VPN servers are deleted, orphaned cache keys will remain. This is low-impact but worth documenting or addressing in a future cleanup task.openwisp_controller/config/tests/test_notifications.py (1)
34-36: LGTM!The logger path correctly updated from
logger.warntologger.warningto match the implementation change intasks_zerotier.py.openwisp_controller/config/tests/test_vpn.py (3)
6-6: LGTM!Import added to support the new mock response pattern.
830-866: LGTM!The test mocks are correctly updated to:
- Use
mock.Mock(spec=requests.Response)for proper response simulation- Configure
raise_for_statusto raiseHTTPErrorfor failure cases- Update assertions to expect
logger.warninginstead oflogger.error- Match the new error message format ("Error: Not Found")
1477-1479: LGTM!Logger path correctly updated to match the implementation change.
Added timeout parameter to request.post with verify variable simplification. Guard against missing action parameter to prevent AttributeError. Fixes #1049
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
openwisp_controller/config/tasks.py (1)
135-162: Solid error handling and notification flow!The implementation correctly wraps the HTTP request in try/except, adds timeout protection, and emits notifications for both error and recovery scenarios. The cache-backed state tracking via
task_keyensures users are notified appropriately.One minor optimization: Line 147 uses
str(e)in the f-string, which can be simplified using the explicit conversion flag.🔎 Minor f-string optimization
Based on static analysis hints, use explicit conversion flag:
logger.warning( f"Failed to update VPN Server configuration. " - f"Error: {str(e)}, " + f"Error: {e!s}, " f"VPN Server UUID: {vpn_id}" )openwisp_controller/config/utils.py (3)
258-262: Consider adding documentation.The function correctly implements state-based recovery notifications using cache. Adding a docstring would help maintainers understand the cache-based state tracking pattern.
🔎 Suggested docstring
def handle_recovery_notification(task_key, **kwargs): + """ + Handle recovery notification for background tasks. + + Sends a success notification only if the task previously failed, + then updates the cached state to "success". + + Args: + task_key (str): Cache key for tracking task state + **kwargs: Parameters passed to send_api_task_notification + """ task_result = cache.get(task_key)
265-269: Consider adding documentation.The function correctly implements state-based error notifications with deduplication. Adding a docstring would help maintainers understand the cache-based pattern and prevent notification spam.
🔎 Suggested docstring
def handle_error_notification(task_key, **kwargs): + """ + Handle error notification for background tasks. + + Sends an error notification only on the first occurrence, + preventing notification spam for repeated failures. + + Args: + task_key (str): Cache key for tracking task state + **kwargs: Parameters passed to send_api_task_notification + """ cached_value = cache.get(task_key)
214-255: Add docstring and input validation; remove obsolete comment about issue #264.The function lacks documentation and doesn't validate the
typeparameter. Additionally, the commented-outsleep(2)and associated comment can be removed—issue #264 in openwisp-notifications has been fixed upstream in the 25.10 release, making this workaround unnecessary.🔎 Suggested improvements
- Add a docstring explaining parameters and behavior:
def send_api_task_notification(type, **kwargs): """ Send API task notification via openwisp-notifications. Args: type (str): Notification type, either "error" or "success" **kwargs: Additional parameters including: - instance: The VPN instance - action: The action being performed (e.g., "update") - exception: The exception object if type is "error" """
- Add type parameter validation and remove obsolete comment:
-def send_api_task_notification(type, **kwargs): +def send_api_task_notification(type, **kwargs): + if type not in ("error", "success"): + raise ValueError(f"Invalid notification type: {type}. Must be 'error' or 'success'.") vpn = kwargs.get("instance") action = kwargs.get("action", "").replace("_", " ") exception = kwargs.get("exception") - # Adding some delay here to prevent overlapping - # of the django success message container - # with the ow-notification container - # https://github.com/openwisp/openwisp-notifications/issues/264 - # sleep(2) message_map = {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
openwisp_controller/config/tasks.pyopenwisp_controller/config/utils.py
🧰 Additional context used
🧬 Code graph analysis (1)
openwisp_controller/config/tasks.py (1)
openwisp_controller/config/utils.py (2)
handle_error_notification(265-269)handle_recovery_notification(258-262)
🪛 Ruff (0.14.10)
openwisp_controller/config/tasks.py
147-147: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (2)
openwisp_controller/config/tasks.py (1)
12-13: LGTM!The imports are correctly added to support the new centralized notification utilities.
openwisp_controller/config/utils.py (1)
3-3: LGTM!The imports are correctly added to support the new notification and caching functionality.
Also applies to: 9-10
openwisp_controller/config/utils.py
Outdated
| # of the django success message container | ||
| # with the ow-notification container | ||
| # https://github.com/openwisp/openwisp-notifications/issues/264 | ||
| # sleep(2) |
There was a problem hiding this comment.
@stktyagi are you confident of this? Why leave the comment commented out here?
There was a problem hiding this comment.
I asked about uncommenting it from @coderabbitai before testing in #1147 (comment) and when i actually tested after uncommenting it, it seemed to block my celery worker due to which I saw no notification. I left it there so I can get a second review from you before removing it completely but I feel it should be removed.
There was a problem hiding this comment.
@stktyagi, you're absolutely correct—the commented code and the entire comment block should be removed. Here's why:
The sleep doesn't make sense in this context:
- This function is called from background Celery tasks (like
trigger_vpn_server_endpoint) - Notifications sent from background tasks are displayed whenever the user next interacts with the UI, not immediately
- Django success messages appear during the synchronous request/response cycle
- A
sleep()in a background task cannot prevent overlap with UI messages that were already shown during the original request
The practical problem you observed:
sleep()blocks the Celery worker thread, preventing it from processing other tasks- This is why you saw no notifications—the worker was stuck sleeping
Recommendation:
Remove lines 218-221 entirely (the comment block and the commented sleep(2)). The referenced issue #264 is about frontend timing, which should be handled in the UI layer if needed, not in backend notification dispatch.
✏️ Learnings added
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1147
File: openwisp_controller/config/utils.py:222-222
Timestamp: 2026-02-05T20:05:51.131Z
Learning: In `openwisp_controller/config/utils.py`, the `send_api_task_notification` function is called from Celery background tasks. Do not use sleep() or blocking operations in this function as it blocks the Celery worker and prevents notifications from being sent.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@stktyagi did you do some manual testing to check the difference between with and without it? If not, please do. Is there any difference? If so, can you please post 1 recording for each case?
There was a problem hiding this comment.
I did do some manual testing last time but it was only for 2 versions. One where I simply uncommented sleep(2) and one where I kept it commented out. This time I also tested a version where I imported time and changed it to time.sleep(2) and that was the only version where I was able to see a proper error pop-up and non-blocking.
Here are the three versions:-
1.)import time
sleep(2)
sleep2.mp4
2.) import time
time.sleep(2)
timesleep.mp4
3.) # sleep(2)
sleepcomm.mp4
Very basic but unnoticed.
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Thank you very much @stktyagi for the manual testing and the recordings, that saves a lot of time, let's do more of it!
See my comments below, which should help resolve the empasses and merge this.
openwisp_controller/config/utils.py
Outdated
| ) | ||
|
|
||
|
|
||
| def handle_recovery_notification(task_key, **kwargs): |
There was a problem hiding this comment.
add sleep_time=False here to and just pass it to send_api_task_notification
openwisp_controller/config/utils.py
Outdated
| cache.set(task_key, "success", timeout=None) | ||
|
|
||
|
|
||
| def handle_error_notification(task_key, **kwargs): |
There was a problem hiding this comment.
add sleep_time=False here to and just pass it to send_api_task_notification
openwisp_controller/config/utils.py
Outdated
| # of the django success message container | ||
| # with the ow-notification container | ||
| # https://github.com/openwisp/openwisp-notifications/issues/264 | ||
| # sleep(2) |
There was a problem hiding this comment.
- add a
sleep_timekeyword argument of the function, let's explain in the docstring that it's needed to avoid triggering the toast notification in the admin while the page is reloading which is bad UX - if not false, trigger the sleep
| if task_result == "error": | ||
| self._send_api_task_notification("recovery", **kwargs) | ||
| cache.set(task_key, "success", None) | ||
| handle_recovery_notification(task_key, **kwargs) |
There was a problem hiding this comment.
| handle_recovery_notification(task_key, **kwargs) | |
| # the delay avoids showing notifications during page reload | |
| handle_recovery_notification(task_key, sleep_time=5, **kwargs) |
There was a problem hiding this comment.
It seems the sleep is missing from the zerotier tasks
| self._send_api_task_notification( | ||
| "error", status_code=response.status_code, **kwargs | ||
| ) | ||
| handle_error_notification(task_key, exception=e, **kwargs) |
There was a problem hiding this comment.
| handle_error_notification(task_key, exception=e, **kwargs) | |
| # the delay avoids showing notifications during page reload | |
| handle_error_notification(task_key, sleep_time=5, exception=e, **kwargs) |
nemesifier
changed the title
nemesifier
changed the title
Implemented sleep_time arguement passing for zerotier. Fixes #1049
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openwisp_controller/config/tasks_zerotier.py`:
- Around line 76-82: The code currently re-raises RequestException (raise e) for
non-recoverable HTTP status codes (those not in _RECOVERABLE_API_CODES), which
triggers Celery autoretry_for=(RequestException,) with API_TASK_RETRY_OPTIONS;
change this so non-recoverable errors do not cause retries: either remove the
raise so the task returns after logger.error and
handle_error_notification(task_key, ...), or raise a different exception type
that is not included in autoretry_for (e.g., a custom NonRetryableError) so
Celery marks the task failed immediately; also replace all "raise e" occurrences
with bare "raise" to preserve original tracebacks per Ruff TRY201.
🧹 Nitpick comments (1)
openwisp_controller/config/tasks_zerotier.py (1)
49-49: Nit:Falseas default for a numeric parameter.
sleep_timeis an integer (5) when specified but defaults toFalse(a boolean). UsingNoneas the sentinel would be more idiomatic and clearer about intent, though this matches the pattern already established inutils.py.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
openwisp_controller/config/tasks_zerotier.py
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1147
File: openwisp_controller/config/utils.py:222-222
Timestamp: 2026-02-05T20:06:01.589Z
Learning: In `openwisp_controller/config/utils.py`, the `send_api_task_notification` function is called from Celery background tasks. Do not use sleep() or blocking operations in this function as it blocks the Celery worker and prevents notifications from being sent.
📚 Learning: 2026-02-05T20:06:01.589Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1147
File: openwisp_controller/config/utils.py:222-222
Timestamp: 2026-02-05T20:06:01.589Z
Learning: In `openwisp_controller/config/utils.py`, the `send_api_task_notification` function is called from Celery background tasks. Do not use sleep() or blocking operations in this function as it blocks the Celery worker and prevents notifications from being sent.
Applied to files:
openwisp_controller/config/tasks_zerotier.py
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/config/tasks_zerotier.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/tasks_zerotier.py
🧬 Code graph analysis (1)
openwisp_controller/config/tasks_zerotier.py (1)
openwisp_controller/config/utils.py (2)
handle_error_notification(267-271)handle_recovery_notification(260-264)
🪛 Ruff (0.14.14)
openwisp_controller/config/tasks_zerotier.py
[warning] 82-82: Use raise without specifying exception name
Remove exception name
(TRY201)
⏰ 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.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- 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.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
🔇 Additional comments (4)
openwisp_controller/config/tasks_zerotier.py (4)
13-13: LGTM on the import change.Clean replacement of the removed private helper with the centralized
handle_error_notificationandhandle_recovery_notificationutilities.
57-59: LGTM on the response unpacking logic.The
isinstance(response, tuple)guard cleanly handles both return types from service methods.
99-114: Consistentsleep_time=5propagation across all notification-enabled tasks.The four tasks that send notifications (
trigger_zerotier_server_update,trigger_zerotier_server_update_member,trigger_zerotier_server_remove_member,trigger_zerotier_server_join) all consistently passsleep_time=5. Tasks withsend_notification=False(delete, leave) correctly omit it. Line 173 neatly embeds it in thenotification_kwargsdict.Also applies to: 138-156, 165-199, 208-233
63-68: Good improvements: centralized recovery path andlogger.warning.Using
logger.warningover the deprecatedlogger.warnis correct. Thegetattrpattern on line 65 safely handles exceptions without a.responseattribute.The kwargs at this point (after
sleep_timeis popped on line 49) containerr,info,instance, andaction. These are safely passed throughhandle_recovery_notificationtosend_api_task_notification, which accepts**kwargsand extracts only what it needs via.get()calls—extra parameters are harmlessly ignored.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Raised a different exception type excluded from autoretry_for, so Celery marks the task as failed without retrying. Fixes #1049
There was a problem hiding this comment.
Actionable comments posted: 0
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/tasks_zerotier.py (1)
64-82:⚠️ Potential issue | 🔴 CriticalConnection-level errors (no HTTP response) are silently treated as unrecoverable and won't be retried.
When a
RequestExceptionlacks an associated HTTP response (e.g.,ConnectionError,Timeout, DNS failure),response_objisNone, sostatus_codebecomesNone. Since_RECOVERABLE_API_CODEScontains only HTTP status codes (429, 500, 502, 503, 504) andNoneis not among them, the condition on line 68 fails. The code then falls through to line 77–82 and raises a genericException— which doesn't matchautoretry_for=(RequestException,). These connection-level errors are typically transient network issues and should be the highest priority for retry.Fix: treat missing response as recoverable
response_obj = getattr(e, "response", None) status_code = response_obj.status_code if response_obj is not None else None - if status_code in self._RECOVERABLE_API_CODES: + if status_code is None or status_code in self._RECOVERABLE_API_CODES: retry_logger = logger.warning # When retry limit is reached, use error logging if self.request.retries == self.max_retries:
🧹 Nitpick comments (2)
openwisp_controller/config/tasks_zerotier.py (2)
82-82: Missing exception chaining and use of bareException.Ruff B904:
raise Exception(...) from eshould be used to preserve the causal chain. Additionally, a custom exception class (e.g.,NonRetryableError) would be cleaner than bareException(Ruff TRY002) and would make the intent explicit — avoiding accidental catches elsewhere.♻️ Proposed fix
Define a custom exception (e.g., in
utils.pyor at module level):class NonRetryableError(Exception): """Raised for API errors that should not trigger Celery auto-retry."""Then use it:
- raise Exception(f"Task failed due to unrecoverable error: {e}") + raise NonRetryableError( + f"Task failed due to unrecoverable error: {e}" + ) from e
49-49:sleep_timedefault ofFalseis semantically odd for a time parameter.
sleep_timedefaults toFalse(a boolean) but is used as a numeric duration. Consider usingNoneor0as the default to better convey intent — this applies here and in the utility functions inutils.py.Also applies to: 63-63, 79-81
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
openwisp_controller/config/tasks_zerotier.py
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1147
File: openwisp_controller/config/utils.py:222-222
Timestamp: 2026-02-05T20:06:01.589Z
Learning: In `openwisp_controller/config/utils.py`, the `send_api_task_notification` function is called from Celery background tasks. Do not use sleep() or blocking operations in this function as it blocks the Celery worker and prevents notifications from being sent.
📚 Learning: 2026-02-05T20:06:01.589Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1147
File: openwisp_controller/config/utils.py:222-222
Timestamp: 2026-02-05T20:06:01.589Z
Learning: In `openwisp_controller/config/utils.py`, the `send_api_task_notification` function is called from Celery background tasks. Do not use sleep() or blocking operations in this function as it blocks the Celery worker and prevents notifications from being sent.
Applied to files:
openwisp_controller/config/tasks_zerotier.py
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/config/tasks_zerotier.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/tasks_zerotier.py
🧬 Code graph analysis (1)
openwisp_controller/config/tasks_zerotier.py (1)
openwisp_controller/config/utils.py (2)
handle_error_notification(267-271)handle_recovery_notification(260-264)
🪛 Ruff (0.14.14)
openwisp_controller/config/tasks_zerotier.py
[warning] 82-82: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
[warning] 82-82: Create your own exception
(TRY002)
[warning] 82-82: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (2)
openwisp_controller/config/tasks_zerotier.py (2)
13-13: LGTM on the import refactoring.Clean centralization of notification logic into shared utilities.
86-114: Consistentsleep_time=5additions across all notification-enabled tasks look good.The four task functions that send notifications all consistently pass
sleep_time=5, and the non-notification tasks (trigger_zerotier_server_delete,trigger_zerotier_server_leave) correctly omit it.Also applies to: 122-156, 159-199, 202-233
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Raised a different exception type excluded from autoretry_for, so Celery marks the task as failed without retrying.
Raised a different exception type excluded from autoretry_for, so Celery marks the task as failed without retrying.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_controller/config/tasks_zerotier.py (1)
64-76:⚠️ Potential issue | 🟠 MajorConnection errors (no HTTP response) are silently treated as unrecoverable.
When
RequestExceptionis raised without an attachedresponse(e.g.,ConnectionError,Timeout, DNS failures),status_codeevaluates toNoneon line 66 and falls through to the unrecoverable branch (lines 77–82). These are typically transient failures that should be retried.Consider adding
status_code is Noneto the recoverable check so network-level errors still trigger Celery retries:Proposed fix
- if status_code in self._RECOVERABLE_API_CODES: + if status_code is None or status_code in self._RECOVERABLE_API_CODES:
🤖 Fix all issues with AI agents
In `@openwisp_controller/config/tasks_zerotier.py`:
- Line 82: The current code uses an invalid "raise None" which causes a
TypeError; define a small non-retryable exception class (e.g., class
NonRetryableError(Exception):) near the top of tasks_zerotier.py and replace the
"raise None" in the failing API path with "raise NonRetryableError(...)"
(include a short message with the API status or response) so the task fails
without triggering Celery's autoretry_for=(RequestException,).
🧹 Nitpick comments (1)
openwisp_controller/config/tasks_zerotier.py (1)
76-76: Prefer bareraiseto preserve the original traceback.
raise eresets the traceback to this line. A bareraisepreserves the full original stack trace, which is more useful for debugging.Proposed fix
- raise e + raise
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
openwisp_controller/config/tasks_zerotier.py
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1147
File: openwisp_controller/config/utils.py:222-222
Timestamp: 2026-02-05T20:06:01.589Z
Learning: In `openwisp_controller/config/utils.py`, the `send_api_task_notification` function is called from Celery background tasks. Do not use sleep() or blocking operations in this function as it blocks the Celery worker and prevents notifications from being sent.
📚 Learning: 2026-02-05T20:06:01.589Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1147
File: openwisp_controller/config/utils.py:222-222
Timestamp: 2026-02-05T20:06:01.589Z
Learning: In `openwisp_controller/config/utils.py`, the `send_api_task_notification` function is called from Celery background tasks. Do not use sleep() or blocking operations in this function as it blocks the Celery worker and prevents notifications from being sent.
Applied to files:
openwisp_controller/config/tasks_zerotier.py
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/config/tasks_zerotier.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/tasks_zerotier.py
🧬 Code graph analysis (1)
openwisp_controller/config/tasks_zerotier.py (1)
openwisp_controller/config/utils.py (2)
handle_error_notification(267-271)handle_recovery_notification(260-264)
🪛 Ruff (0.14.14)
openwisp_controller/config/tasks_zerotier.py
[warning] 82-82: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
[warning] 82-82: Cannot raise a literal. Did you intend to return it or raise an Exception?
(B016)
⏰ 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). (9)
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
🔇 Additional comments (2)
openwisp_controller/config/tasks_zerotier.py (2)
86-120: LGTM — consistentsleep_time=5across all notification-enabled tasks.The
sleep_timeparameter is uniformly threaded through to the notification utilities for all four tasks that send notifications. Tasks that explicitly disable notifications (trigger_zerotier_server_delete,trigger_zerotier_server_leave) correctly omit it.Also applies to: 122-156, 159-199, 202-233
63-63: No action required. The parameter naming is consistent across bothhandle_recovery_notificationandhandle_error_notification—both correctly passsleep_time=sleep_timetosend_api_task_notification. The function signature expectssleep_time(nottime_sleep), and both callers use the correct parameter name.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openwisp_controller/config/tasks_zerotier.py`:
- Around line 76-82: handle_api_call currently returns None on unrecoverable
errors which breaks callers that unpack or access response; instead define a
NonRetryableApiError exception class and raise it from handle_api_call for
unrecoverable API results so Celery will mark the task failed (not retried) and
callers (trigger_zerotier_server_update, trigger_zerotier_server_join,
trigger_zerotier_server_delete) never receive None; alternatively, if you prefer
guards, add explicit None checks at those call sites before unpacking or
accessing response.ok / response.status_code, but prefer the
NonRetryableApiError approach and ensure the new exception is not listed in
autoretry_for.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
openwisp_controller/config/tasks_zerotier.py
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1147
File: openwisp_controller/config/utils.py:222-222
Timestamp: 2026-02-05T20:06:01.589Z
Learning: In `openwisp_controller/config/utils.py`, the `send_api_task_notification` function is called from Celery background tasks. Do not use sleep() or blocking operations in this function as it blocks the Celery worker and prevents notifications from being sent.
📚 Learning: 2026-02-05T20:06:01.589Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1147
File: openwisp_controller/config/utils.py:222-222
Timestamp: 2026-02-05T20:06:01.589Z
Learning: In `openwisp_controller/config/utils.py`, the `send_api_task_notification` function is called from Celery background tasks. Do not use sleep() or blocking operations in this function as it blocks the Celery worker and prevents notifications from being sent.
Applied to files:
openwisp_controller/config/tasks_zerotier.py
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/config/tasks_zerotier.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/tasks_zerotier.py
🧬 Code graph analysis (1)
openwisp_controller/config/tasks_zerotier.py (1)
openwisp_controller/config/utils.py (2)
handle_error_notification(267-271)handle_recovery_notification(260-264)
🔇 Additional comments (2)
openwisp_controller/config/tasks_zerotier.py (2)
27-83: Overallhandle_api_callrefactoring is well-structured.The centralization of notification logic through
handle_recovery_notificationandhandle_error_notificationis a clean improvement. The separation of recoverable vs. unrecoverable error paths and the use oflogger.warningfor recoverable errors (escalating tologger.errorwhen retries are exhausted) is thoughtful. Thesleep_timepropagation is consistent across all task call sites.
165-199: Good defensive handling of theObjectDoesNotExistcase.Setting
send_notification=Falsewhen the VPN server has been deleted avoids sending misleading notifications and handles the deletion race condition cleanly.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Raised a custom exception type excluded from autoretry_for, so Celery marks the task as failed without retrying. Fixes #1049
There was a problem hiding this comment.
Actionable comments posted: 0
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/tasks_zerotier.py (1)
69-86:⚠️ Potential issue | 🔴 CriticalNetwork-level exceptions (ConnectionError, Timeout, etc.) are silently treated as unrecoverable — retries are broken.
When a
RequestExceptionhas no HTTP response (e.g.,ConnectionError,Timeout, DNS failure),response_objisNoneandstatus_codebecomesNone. SinceNoneis not in_RECOVERABLE_API_CODES(which only contains HTTP codes 429, 500, 502, 503, 504), execution falls through to line 81–86, raisingNonRetryableApiError. BecauseNonRetryableApiErroris not inautoretry_for=(RequestException,), Celery will not retry these transient network failures.This is a regression: before this change, any
RequestExceptionwould propagate naturally and trigger autoretry. Now only HTTP responses with specific status codes are retried.🐛 Proposed fix — treat missing-response exceptions as recoverable
except RequestException as e: response_obj = getattr(e, "response", None) status_code = response_obj.status_code if response_obj is not None else None - if status_code in self._RECOVERABLE_API_CODES: + if status_code is None or status_code in self._RECOVERABLE_API_CODES: retry_logger = logger.warning # When retry limit is reached, use error logging if self.request.retries == self.max_retries: retry_logger = logger.error retry_logger( f"Try [{self.request.retries}/{self.max_retries}] " f"{err_msg}, Error: {e}" ) - raise e + raise logger.error(f"{err_msg}, Error: {e}")This ensures network-level errors without an HTTP response are still retried by Celery. Also, the bare
raiseon line 80 preserves the original traceback per Ruff TRY201.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
openwisp_controller/config/tasks_zerotier.py
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1147
File: openwisp_controller/config/utils.py:222-222
Timestamp: 2026-02-05T20:06:01.589Z
Learning: In `openwisp_controller/config/utils.py`, the `send_api_task_notification` function is called from Celery background tasks. Do not use sleep() or blocking operations in this function as it blocks the Celery worker and prevents notifications from being sent.
📚 Learning: 2026-02-05T20:06:01.589Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1147
File: openwisp_controller/config/utils.py:222-222
Timestamp: 2026-02-05T20:06:01.589Z
Learning: In `openwisp_controller/config/utils.py`, the `send_api_task_notification` function is called from Celery background tasks. Do not use sleep() or blocking operations in this function as it blocks the Celery worker and prevents notifications from being sent.
Applied to files:
openwisp_controller/config/tasks_zerotier.py
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/config/tasks_zerotier.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/tasks_zerotier.py
🧬 Code graph analysis (1)
openwisp_controller/config/tasks_zerotier.py (1)
openwisp_controller/config/utils.py (2)
handle_error_notification(267-271)handle_recovery_notification(260-264)
🪛 Ruff (0.14.14)
openwisp_controller/config/tasks_zerotier.py
[warning] 86-86: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (5)
openwisp_controller/config/tasks_zerotier.py (5)
13-19: LGTM on the new imports andNonRetryableApiErrorexception.
NonRetryableApiErrorinherits fromException(notRequestException), so it's correctly excluded fromautoretry_for=(RequestException,). This ensures Celery marks the task as failed immediately for unrecoverable errors without retrying.
96-124: LGTM on thesleep_time=5addition and task structure.The
sleep_timeis correctly passed via kwargs and popped byhandle_api_call. The return value handling and status code check are sound — execution only reaches line 119 on a successful (non-exception) response.
132-161: LGTM.Consistent
sleep_time=5addition. Return value is correctly discarded since no post-call response processing is needed.
169-203: LGTM.The
sleep_time=5is correctly included innotification_kwargsonly when the VPN instance exists and notifications are enabled. When the VPN is deleted,send_notification=Falseis passed andsleep_timedefaults toFalse— both paths are consistent.
206-282: LGTM on the remaining task functions.
sleep_time=5is added only where notifications are enabled (trigger_zerotier_server_join). The two tasks withsend_notification=False(trigger_zerotier_server_delete,trigger_zerotier_server_leave) correctly omitsleep_time. Post-call response checks are only reached on the success path.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Now only HTTP responses with specific status codes are retried. Fixes #1049
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openwisp_controller/config/tasks_zerotier.py`:
- Around line 60-87: When a recoverable RequestException exhausts retries the
code currently re-raises without notifying; modify the except RequestException
block so that when self.request.retries == self.max_retries you call
handle_error_notification(task_key, sleep_time=sleep_time, exception=e,
**kwargs) (and log an error) immediately before raising, instead of only using
logger.error/raise; keep the existing NonRetryableApiError branch unchanged.
Ensure you reference the same symbols (self.request.retries, self.max_retries,
handle_error_notification, task_key, sleep_time, exception e) and avoid
double-notifying for the non-retryable path.
🧹 Nitpick comments (2)
openwisp_controller/config/tasks_zerotier.py (2)
81-81: Considerlogger.exceptionfor the unrecoverable error path.Within the
exceptblock,logger.exceptionwould automatically include the traceback, providing better diagnostics in application logs for non-recoverable failures. TheNonRetryableApiErrorraised after will also be logged by Celery, but having the original traceback at the point of detection is useful.- logger.error(f"{err_msg}, Error: {e}") + logger.exception(f"{err_msg}, Error: {e}")
96-123:sleep_time=5is hardcoded across all task call sites.All four notification-enabled tasks pass
sleep_time=5. If this value ever needs tuning, it must be updated in multiple places. Consider extracting it as a module-level constant or deriving it fromAPI_TASK_RETRY_OPTIONS.Also applies to: 126-160, 163-203, 206-237
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
openwisp_controller/config/tasks_zerotier.py
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1147
File: openwisp_controller/config/utils.py:222-222
Timestamp: 2026-02-05T20:06:01.589Z
Learning: In `openwisp_controller/config/utils.py`, the `send_api_task_notification` function is called from Celery background tasks. Do not use sleep() or blocking operations in this function as it blocks the Celery worker and prevents notifications from being sent.
📚 Learning: 2026-02-05T20:06:01.589Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1147
File: openwisp_controller/config/utils.py:222-222
Timestamp: 2026-02-05T20:06:01.589Z
Learning: In `openwisp_controller/config/utils.py`, the `send_api_task_notification` function is called from Celery background tasks. Do not use sleep() or blocking operations in this function as it blocks the Celery worker and prevents notifications from being sent.
Applied to files:
openwisp_controller/config/tasks_zerotier.py
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/config/tasks_zerotier.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/tasks_zerotier.py
🧬 Code graph analysis (1)
openwisp_controller/config/tasks_zerotier.py (1)
openwisp_controller/config/utils.py (2)
handle_error_notification(267-271)handle_recovery_notification(260-264)
🪛 Ruff (0.14.14)
openwisp_controller/config/tasks_zerotier.py
[warning] 81-81: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
[warning] 86-86: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (2)
openwisp_controller/config/tasks_zerotier.py (2)
18-19: LGTM — clean solution for stopping retries on unrecoverable errors.
NonRetryableApiErrorsits outside theautoretry_for=(RequestException,)hierarchy, so Celery correctly marks the task as failed without retrying. This addresses the issues flagged in earlier reviews.
69-71: Good defensive handling of missing response objects.Using
getattr(e, "response", None)and the conditionalstatus_codeextraction correctly handles exceptions likeConnectionErrorthat lack a.responseattribute. Treating these as recoverable (retry-worthy) aligns with the commit message intent.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
@nemesifier, For testing purposes I registered a valid zerotier vpn server and as it would raise ValidationError and won't let me save to test I temporarily removed the validation on token and tested the toast notification. Is there a better way to test this according to you? As this proves it would work for other errors but I had to see for after removing validation. 
output.mp4 |
Removed redundant dict of keyword arguments to maintain minimal change. Fixes #1049
stktyagi
force-pushed
the
issues/1049-send-generic-message
branch
from
5423028 to
990e113
Compare
nemesifier
changed the title
Check reason for qa failure. Fixes #1049
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_controller/config/tasks_zerotier.py (1)
65-79:⚠️ Potential issue | 🟠 MajorNo user notification when recoverable errors exhaust all retries.
When a recoverable error (5xx, 429) hits the max retry count (line 69), the task logs an error and re-raises (line 75), but
handle_error_notificationis never called. The user won't be informed that the operation ultimately failed. The unrecoverable (4xx) path correctly notifies at line 78, but this gap means transient failures that never recover are silently lost from the user's perspective.Proposed fix — notify before the final re-raise
if self.request.retries == self.max_retries: retry_logger = logger.error + if send_notification: + handle_error_notification( + task_key, sleep_time=kwargs.get('sleep_time', False), exception=e, **kwargs + ) retry_logger(Note: you may need to avoid passing
sleep_timetwice (once explicitly, once via**kwargs). Extract it from kwargs or pass kwargs selectively.
🤖 Fix all issues with AI agents
In `@openwisp_controller/config/tasks_zerotier.py`:
- Line 49: Remove the leftover debug comment "# checking commit qa failure" from
tasks_zerotier.py; locate the stray comment in the module (near the top-level or
inside the relevant task function/class such as any Zerotier-related task or
function) and delete that line, then run linters/formatters to ensure no
trailing whitespace remains and commit the change.
🧹 Nitpick comments (1)
openwisp_controller/config/tasks_zerotier.py (1)
75-75: Use bareraiseto preserve the original traceback.
raise ere-raises the exception but resets the traceback to this line. A bareraisepreserves the full original traceback, which is better for debugging. This also satisfies Ruff TRY201.Proposed fix
- raise e + raise
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
openwisp_controller/config/tasks_zerotier.py
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1147
File: openwisp_controller/config/utils.py:222-222
Timestamp: 2026-02-05T20:06:01.589Z
Learning: In `openwisp_controller/config/utils.py`, the `send_api_task_notification` function is called from Celery background tasks. Do not use sleep() or blocking operations in this function as it blocks the Celery worker and prevents notifications from being sent.
📚 Learning: 2026-02-05T20:06:01.589Z
Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1147
File: openwisp_controller/config/utils.py:222-222
Timestamp: 2026-02-05T20:06:01.589Z
Learning: In `openwisp_controller/config/utils.py`, the `send_api_task_notification` function is called from Celery background tasks. Do not use sleep() or blocking operations in this function as it blocks the Celery worker and prevents notifications from being sent.
Applied to files:
openwisp_controller/config/tasks_zerotier.py
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/config/tasks_zerotier.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/tasks_zerotier.py
🧬 Code graph analysis (1)
openwisp_controller/config/tasks_zerotier.py (1)
openwisp_controller/config/utils.py (2)
handle_error_notification(267-271)handle_recovery_notification(260-264)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.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.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
🔇 Additional comments (2)
openwisp_controller/config/tasks_zerotier.py (2)
13-13: Good refactor: centralised notification helpers replace inline cache logic.Moving to
handle_error_notification/handle_recovery_notificationfromutils.pyis a clean separation of concerns and keeps this task file focused on API orchestration.Also applies to: 64-64, 78-78
95-110:sleep_time=5propagation looks consistent across all notifiable tasks.All task functions that enable notifications now pass
sleep_time=5, and the two tasks that disable notifications (trigger_zerotier_server_delete,trigger_zerotier_server_leave) correctly omit it. Thetrigger_zerotier_server_remove_membergracefully falls back tosend_notification=Falsewhen the VPN instance is missing.Also applies to: 134-152, 161-195, 212-226
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
github-project-automation
bot
moved this from In progress
to Done
in OpenWISP Contributor's Board
3 participants
Checklist
Reference to Existing Issue
Closes #1049
Description of Changes
Extended
config/utils.pyfor sending api tasks' notification with fixingtrigger_vpn_server_endpointto send generic message notification for error and recovery.Modified tasks and tasks zerotier consistently with utils. Tested via manual testing.