[fix] SSH session leak and silent failure on config push error

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):

openwisp_controller/connection/tasks.py

@@ -61,7 +61,14 @@ def update_config(self, device_id):
         return
     else:
         logger.info(f"Updating {device} (pk: {device_id})")
-        device_conn.update_config()
+        try:
+            device_conn.update_config()
+        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(reason=str(e))
 
 
 # task timeout is SSH_COMMAND_TIMEOUT plus a 20% margin

openwisp_controller/connection/tests/test_models.py

@@ -900,6 +900,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):
@@ -969,7 +984,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": []}'
@@ -1020,8 +1035,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")
@@ -1107,6 +1122,27 @@ 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")
+        self.assertIn("SSH channel closed", conf.error_reason)
+
     @mock.patch(_connect_path)
     def test_schedule_command_called(self, connect_mocked):
         dc = self._create_device_connection()