From 7de6b043ea07d7090e04ac190cb67a7da2c1da28 Mon Sep 17 00:00:00 2001 From: prakash-kalwaniya Date: Tue, 17 Mar 2026 12:04:55 +0530 Subject: [PATCH 1/3] [fix] SSH session leak and silent failure on config push error When connector_instance.update_config() raised an exception (e.g. SSH channel closed mid-transfer, device reboot, network partition), the old code swallowed it silently and skipped disconnect(), leaving the SSH session open and the config status permanently stuck in 'modified' with no visible error to operators. Fix: use try/finally in AbstractDeviceConnection.update_config() so disconnect() is always called, and catch the propagated exception in the update_config Celery task to call set_status_error() on the device config, making failures visible in the admin dashboard. --- openwisp_controller/connection/base/models.py | 7 +--- openwisp_controller/connection/tasks.py | 9 +++- .../connection/tests/test_models.py | 41 +++++++++++++++++-- 3 files changed, 47 insertions(+), 10 deletions(-) diff --git a/openwisp_controller/connection/base/models.py b/openwisp_controller/connection/base/models.py index 15f4eff63..8dcf0306e 100644 --- a/openwisp_controller/connection/base/models.py +++ b/openwisp_controller/connection/base/models.py @@ -1,5 +1,4 @@ import collections -import logging import django import jsonschema @@ -32,8 +31,6 @@ from ..signals import is_working_changed from ..tasks import auto_add_credentials_to_devices, launch_command -logger = logging.getLogger(__name__) - class ConnectorMixin(object): _connector_field = "connector" @@ -367,9 +364,7 @@ def update_config(self): if self.is_working: try: self.connector_instance.update_config() - except Exception as e: - logger.exception(e) - else: + finally: self.disconnect() def save(self, *args, **kwargs): diff --git a/openwisp_controller/connection/tasks.py b/openwisp_controller/connection/tasks.py index d75bbde20..62a553543 100644 --- a/openwisp_controller/connection/tasks.py +++ b/openwisp_controller/connection/tasks.py @@ -56,7 +56,14 @@ def update_config(device_id): return else: logger.info(f"Updating {device} (pk: {device_id})") - device_conn.update_config() + try: + device_conn.update_config() + except Exception: + logger.exception( + f"Failed to push configuration to {device} (pk: {device_id})" + ) + if hasattr(device, "config"): + device.config.set_status_error() # task timeout is SSH_COMMAND_TIMEOUT plus a 20% margin diff --git a/openwisp_controller/connection/tests/test_models.py b/openwisp_controller/connection/tests/test_models.py index 14693dfbd..b39ec2420 100644 --- a/openwisp_controller/connection/tests/test_models.py +++ b/openwisp_controller/connection/tests/test_models.py @@ -899,6 +899,21 @@ def test_command_multiple_connections(self, connect_mocked): command.refresh_from_db() self.assertIn(command.connection, [dc1, dc2]) + def test_update_config_disconnect_always_called(self): + """ + disconnect() must be called even when connector_instance.update_config() + raises, so that the SSH session is never leaked. + """ + dc = self._create_device_connection() + connector_mock = mock.Mock() + connector_mock.update_config.side_effect = Exception("SSH channel closed") + dc.connector_instance = connector_mock + + with self.assertRaises(Exception): + dc.update_config() + + connector_mock.disconnect.assert_called_once() + class TestModelsTransaction(BaseTestModels, TransactionTestCase): def _prepare_conf_object(self, organization=None): @@ -968,7 +983,7 @@ def _assert_applying_conf_test_command(mocked_exec): self.assertEqual(mocked_exec_command.call_count, 1) _assert_version_check_command(mocked_exec_command) conf.refresh_from_db() - self.assertEqual(conf.status, "modified") + self.assertEqual(conf.status, "error") with self.subTest("openwisp_config >= 0.6.0a"): conf.config = '{"dns_servers": []}' @@ -1019,8 +1034,8 @@ def _assert_applying_conf_test_command(mocked_exec): args, _ = mocked_exec_command.call_args_list[2] self.assertEqual(args[0], "/etc/init.d/openwisp_config restart") conf.refresh_from_db() - # exit code 1 considers the update not successful - self.assertEqual(conf.status, "modified") + # failed connector command sets config status to error + self.assertEqual(conf.status, "error") @mock.patch("time.sleep") @mock.patch.object(DeviceConnection, "update_config") @@ -1063,6 +1078,26 @@ def test_device_update_config_not_in_progress( mocked_get_working_connection.assert_called_once() mocked_update_config.assert_called_once() + @capture_any_output() + @mock.patch(_connect_path) + @mock.patch("time.sleep") + def test_update_config_task_sets_status_error_on_failure( + self, mocked_sleep, mocked_connect + ): + """ + When the SSH connector raises during a config push, the update_config + task must set the config status to 'error' so the failure is visible + in the admin dashboard rather than silently staying 'modified'. + """ + conf = self._prepare_conf_object() + conf.config = {"general": {"timezone": "UTC"}} + conf.full_clean() + with mock.patch(_exec_command_path) as mocked_exec_command: + mocked_exec_command.side_effect = Exception("SSH channel closed") + conf.save() + conf.refresh_from_db() + self.assertEqual(conf.status, "error") + @mock.patch(_connect_path) def test_schedule_command_called(self, connect_mocked): dc = self._create_device_connection() From 40f17ccaf8190eac77733d6c6d622dbd4e39d197 Mon Sep 17 00:00:00 2001 From: mn-ram <152869502+mn-ram@users.noreply.github.com> Date: Sat, 21 Mar 2026 10:57:47 +0530 Subject: [PATCH 2/3] [fix] Pass error reason to set_status_error on config push failure When update_config() raises, the exception message is now stored as error_reason on the Config object so the failure cause is visible in the admin dashboard. --- openwisp_controller/connection/tasks.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/openwisp_controller/connection/tasks.py b/openwisp_controller/connection/tasks.py index 62a553543..0bb2933f2 100644 --- a/openwisp_controller/connection/tasks.py +++ b/openwisp_controller/connection/tasks.py @@ -58,12 +58,12 @@ def update_config(device_id): logger.info(f"Updating {device} (pk: {device_id})") try: device_conn.update_config() - except Exception: + except Exception as e: logger.exception( f"Failed to push configuration to {device} (pk: {device_id})" ) if hasattr(device, "config"): - device.config.set_status_error() + device.config.set_status_error(reason=str(e)) # task timeout is SSH_COMMAND_TIMEOUT plus a 20% margin From 9532859ae6dc7e0df837dbc0a3e07e5b90af41b7 Mon Sep 17 00:00:00 2001 From: mn-ram <152869502+mn-ram@users.noreply.github.com> Date: Sat, 21 Mar 2026 11:06:44 +0530 Subject: [PATCH 3/3] [tests] Verify error_reason is persisted on config push failure --- openwisp_controller/connection/tests/test_models.py | 1 + 1 file changed, 1 insertion(+) diff --git a/openwisp_controller/connection/tests/test_models.py b/openwisp_controller/connection/tests/test_models.py index 4e96c9ca3..d3d483b80 100644 --- a/openwisp_controller/connection/tests/test_models.py +++ b/openwisp_controller/connection/tests/test_models.py @@ -1141,6 +1141,7 @@ def test_update_config_task_sets_status_error_on_failure( conf.save() conf.refresh_from_db() self.assertEqual(conf.status, "error") + self.assertIn("SSH channel closed", conf.error_reason) @mock.patch(_connect_path) def test_schedule_command_called(self, connect_mocked):