Fix SSH connection leak when update_config raises exception#1310
Fix SSH connection leak when update_config raises exception#1310AYUSH676-JARVIS wants to merge 1 commit intoopenwisp:masterfrom
Conversation
📝 WalkthroughWalkthrough
Sequence Diagram(s)Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
CI Failures: Flake8, Black, Commit MessageHello @AYUSH676-JARVIS,
For example: |
![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
🤖 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/connection/tasks.py`:
- Around line 63-79: There is a syntax/indentation error in the cleanup block
and the code calls a non-existent close method; fix the finally block so it is
properly indented under the try/except and only attempts to disconnect once by
using getattr(device_conn, "disconnect", None) (fall back to "close" only if
necessary), check callable(close_method) before calling it, wrap the single call
in a try/except that logs via logger.warning on failure, and remove the
redundant second close/disconnect attempts; refer to device_conn.update_config,
close_method, device_conn.disconnect and logger to locate the code to change.
🪄 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: 5816a327-5d85-445a-8d1a-ee8802b86bf8
📒 Files selected for processing (1)
openwisp_controller/connection/tasks.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- 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.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | 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/connection/tasks.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/connection/tasks.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/connection/tasks.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/connection/tasks.py
🧠 Learnings (4)
📚 Learning: 2026-03-17T09:20:10.456Z
Learnt from: pandafy
Repo: openwisp/openwisp-controller PR: 1292
File: openwisp_controller/connection/tasks.py:27-31
Timestamp: 2026-03-17T09:20:10.456Z
Learning: In `openwisp_controller/connection/tasks.py`, the `update_config` Celery task only accepts one argument `device_id`, which is always passed as a string (via `str(device.pk)`) from the call site in `openwisp_controller/connection/apps.py`. Do not flag `str(device_id) in task["args"]` as unreliable due to typed args — the args are always strings.
Applied to files:
openwisp_controller/connection/tasks.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/connection/tasks.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/connection/tasks.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/connection/tasks.py
| logger.info(f"Updating {device} (pk: {device_id})") | ||
|
|
||
| try: | ||
| device_conn.update_config() | ||
| except Exception as e: | ||
| logger.error(f"update_config failed for device {device_id}: {e}") | ||
| raise | ||
| finally: | ||
| # ensure connection is closed | ||
| close_method = getattr(device_conn, "close", None) | ||
| if callable(close_method): | ||
| try: | ||
| close_method() | ||
| try: | ||
| device_conn.close() | ||
| except Exception as close_err: | ||
| logger.warning(f"Error closing connection: {close_err}") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
from pathlib import Path
path = Path("openwisp_controller/connection/tasks.py")
source = path.read_text()
try:
compile(source, str(path), "exec")
print("syntax: OK")
except SyntaxError as exc:
print(f"syntax: FAIL -> {exc.filename}:{exc.lineno}:{exc.offset}: {exc.msg}")
PY
echo
rg -nP -C2 '\bdef (disconnect|close)\s*\(|getattr\(\s*device_conn,\s*"close"|device_conn\.(disconnect|close)\s*\(' \
openwisp_controller/connection/base/models.py \
openwisp_controller/connection/connectors/ssh.py \
openwisp_controller/connection/tasks.pyRepository: openwisp/openwisp-controller
Length of output: 1666
Fix syntax error and use disconnect() in a properly structured finally block.
The code has a syntax error: Line 63 is not indented under the else: statement on line 62, and line 73 incorrectly dedents out of the finally block, preventing the module from importing. Beyond the syntax issue, the code calls device_conn.close() which does not exist in the connection layer—it should call device_conn.disconnect() instead. Additionally, lines 75 and 77 redundantly attempt to close the connection twice via different paths.
🔧 Proposed fix for this block
else:
- logger.info(f"Updating {device} (pk: {device_id})")
-
- try:
- device_conn.update_config()
- except Exception as e:
- logger.error(f"update_config failed for device {device_id}: {e}")
- raise
- finally:
- # ensure connection is closed
- close_method = getattr(device_conn, "close", None)
-if callable(close_method):
- try:
- close_method()
- try:
- device_conn.close()
- except Exception as close_err:
- logger.warning(f"Error closing connection: {close_err}")
+ logger.info(f"Updating {device} (pk: {device_id})")
+ try:
+ device_conn.update_config()
+ except Exception:
+ logger.exception(f"update_config failed for device {device_id}")
+ raise
+ finally:
+ try:
+ device_conn.disconnect()
+ except Exception as disconnect_err:
+ logger.warning(
+ f"Error disconnecting device {device_id}: {disconnect_err}"
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_controller/connection/tasks.py` around lines 63 - 79, There is a
syntax/indentation error in the cleanup block and the code calls a non-existent
close method; fix the finally block so it is properly indented under the
try/except and only attempts to disconnect once by using getattr(device_conn,
"disconnect", None) (fall back to "close" only if necessary), check
callable(close_method) before calling it, wrap the single call in a try/except
that logs via logger.warning on failure, and remove the redundant second
close/disconnect attempts; refer to device_conn.update_config, close_method,
device_conn.disconnect and logger to locate the code to change.
AYUSH676-JARVIS
force-pushed
the
fix-ssh-leak
branch
from
762ae3c to
77dcdad
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
openwisp_controller/connection/tasks.py (1)
71-75:⚠️ Potential issue | 🔴 CriticalUse
disconnect()(with a safe fallback), notclose(), infinally.
DeviceConnectionexposesdisconnect(), notclose(). As written, this path hitsAttributeError, logs a warning, and can leave the SSH session open—the leak remains unresolved.🔧 Proposed fix
finally: if device_conn: - try: - device_conn.close() - except Exception as close_err: - logger.warning(f"Error closing connection: {close_err}") + close_method = getattr(device_conn, "disconnect", None) + if not callable(close_method): + close_method = getattr(device_conn, "close", None) + if callable(close_method): + try: + close_method() + except Exception as close_err: + logger.warning( + f"Error closing connection for device {device_id}: {close_err}" + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/connection/tasks.py` around lines 71 - 75, In the finally block, replace the incorrect device_conn.close() call with device_conn.disconnect() on the DeviceConnection instance; wrap the call in try/except so that if disconnect() is not available you fall back to calling close() if present (or log a clear error), e.g. call device_conn.disconnect() inside the try, in except AttributeError check for a callable device_conn.close and invoke it, otherwise log the failure via logger.warning — update the code around the device_conn.close() usage to use disconnect() with the safe fallback.
🤖 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/connection/tasks.py`:
- Around line 71-75: In the finally block, replace the incorrect
device_conn.close() call with device_conn.disconnect() on the DeviceConnection
instance; wrap the call in try/except so that if disconnect() is not available
you fall back to calling close() if present (or log a clear error), e.g. call
device_conn.disconnect() inside the try, in except AttributeError check for a
callable device_conn.close and invoke it, otherwise log the failure via
logger.warning — update the code around the device_conn.close() usage to use
disconnect() with the safe fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 729ac935-3dfb-48d5-bdb3-34e31e1e014c
📒 Files selected for processing (1)
openwisp_controller/connection/tasks.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | 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~=4.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/connection/tasks.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/connection/tasks.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/connection/tasks.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/connection/tasks.py
🧠 Learnings (4)
📚 Learning: 2026-03-17T09:20:10.456Z
Learnt from: pandafy
Repo: openwisp/openwisp-controller PR: 1292
File: openwisp_controller/connection/tasks.py:27-31
Timestamp: 2026-03-17T09:20:10.456Z
Learning: In `openwisp_controller/connection/tasks.py`, the `update_config` Celery task only accepts one argument `device_id`, which is always passed as a string (via `str(device.pk)`) from the call site in `openwisp_controller/connection/apps.py`. Do not flag `str(device_id) in task["args"]` as unreliable due to typed args — the args are always strings.
Applied to files:
openwisp_controller/connection/tasks.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/connection/tasks.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/connection/tasks.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/connection/tasks.py
…nwisp#1306 Ensure connection is properly closed using disconnect or close. Fixes openwisp#1306
AYUSH676-JARVIS
force-pushed
the
fix-ssh-leak
branch
from
77dcdad to
5755330
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
openwisp_controller/connection/tasks.py (1)
70-75:⚠️ Potential issue | 🔴 CriticalUse
disconnect()fallback infinallyto guarantee teardown.Calling only
device_conn.close()can fail on connectors that exposedisconnect()as the cleanup API, leaving sessions open after update failures (the leak this PR is fixing).🔧 Proposed fix
finally: if device_conn: + close_method = getattr(device_conn, "disconnect", None) or getattr( + device_conn, "close", None + ) + if not callable(close_method): + logger.warning( + f"No disconnect/close method available for device {device_id}" + ) + return try: - device_conn.close() + close_method() except Exception as close_err: - logger.warning(f"Error closing connection: {close_err}") + logger.warning( + f"Error closing connection for device {device_id}: {close_err}" + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/connection/tasks.py` around lines 70 - 75, The finally block currently calls device_conn.close() which may raise or be unavailable on connectors that implement disconnect() for teardown; update the cleanup to try device_conn.close() first and if that fails (AttributeError or Exception) or is not present, call device_conn.disconnect() as a fallback, and ensure both calls are wrapped in try/except so any errors are logged via logger.warning without suppressing the original exception flow; target the finally block that references device_conn and the methods device_conn.close and device_conn.disconnect.
🤖 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/connection/tasks.py`:
- Around line 67-69: In the except block that catches exceptions for
update_config (the block using "except Exception as e" in
openwisp_controller/connection/tasks.py), replace the logger.error call with
logger.exception so the error is logged at error level with the full traceback
before re-raising; keep the same message text (e.g., "update_config failed for
device {device_id}: {e}") and retain the existing "raise" to re-raise the
exception.
---
Duplicate comments:
In `@openwisp_controller/connection/tasks.py`:
- Around line 70-75: The finally block currently calls device_conn.close() which
may raise or be unavailable on connectors that implement disconnect() for
teardown; update the cleanup to try device_conn.close() first and if that fails
(AttributeError or Exception) or is not present, call device_conn.disconnect()
as a fallback, and ensure both calls are wrapped in try/except so any errors are
logged via logger.warning without suppressing the original exception flow;
target the finally block that references device_conn and the methods
device_conn.close and device_conn.disconnect.
🪄 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: 44490398-deb8-4d8b-b382-1f7edbb7006f
📒 Files selected for processing (1)
openwisp_controller/connection/tasks.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | 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/connection/tasks.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/connection/tasks.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/connection/tasks.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/connection/tasks.py
🧠 Learnings (4)
📚 Learning: 2026-03-17T09:20:10.456Z
Learnt from: pandafy
Repo: openwisp/openwisp-controller PR: 1292
File: openwisp_controller/connection/tasks.py:27-31
Timestamp: 2026-03-17T09:20:10.456Z
Learning: In `openwisp_controller/connection/tasks.py`, the `update_config` Celery task only accepts one argument `device_id`, which is always passed as a string (via `str(device.pk)`) from the call site in `openwisp_controller/connection/apps.py`. Do not flag `str(device_id) in task["args"]` as unreliable due to typed args — the args are always strings.
Applied to files:
openwisp_controller/connection/tasks.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/connection/tasks.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/connection/tasks.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/connection/tasks.py
🔇 Additional comments (1)
openwisp_controller/connection/tasks.py (1)
58-58: Defensive initialization looks correct.Setting
device_conn = Nonebefore acquisition makes the cleanup path safe when connection setup fails early.
| except Exception as e: | ||
| logger.error(f"update_config failed for device {device_id}: {e}") | ||
| raise |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Preserve traceback when logging update failures.
Use logger.exception(...) instead of logger.error(...) here so failure diagnostics include stack context before re-raising.
As per coding guidelines, unresolved errors should be logged at error level; logger.exception keeps that level and adds traceback context.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_controller/connection/tasks.py` around lines 67 - 69, In the except
block that catches exceptions for update_config (the block using "except
Exception as e" in openwisp_controller/connection/tasks.py), replace the
logger.error call with logger.exception so the error is logged at error level
with the full traceback before re-raising; keep the same message text (e.g.,
"update_config failed for device {device_id}: {e}") and retain the existing
"raise" to re-raise the exception.
Commit Message Format FailureHello @AYUSH676-JARVIS, The CI failed because the commit message does not follow the required format. Fix: Example: |
1 participant
Fixes #1306
Bug
SSH connection was not properly closed when update_config() raised an exception, leading to connection leaks.
Fix
Testing