MQTT-FIX-T01: make main MQTT client lifecycle idempotent

tests/test_mqtt_client.py

@@ -4,6 +4,7 @@ All paho interaction is mocked — no real broker is required.
 """
 from __future__ import annotations
 
+import threading
 from unittest.mock import MagicMock, patch
 
 import pytest
@@ -103,7 +104,7 @@ def test_connect_creates_paho_client_with_version2() -> None:
     settings = _make_settings()
 
     mock_client = MagicMock()
-    mock_client.connect.return_value = None
+    mock_client.connect_async.return_value = None
 
     with patch("app.integrations.mqtt.mqtt.Client", return_value=mock_client) as mock_cls:
         with patch("app.integrations.mqtt.mqtt.CallbackAPIVersion"):
@@ -117,7 +118,7 @@ def test_connect_creates_paho_client_with_version2() -> None:
     # loop_start must have been called
     mock_client.loop_start.assert_called_once()
     # connect must have been called with the right host/port
-    mock_client.connect.assert_called_once_with(
+    mock_client.connect_async.assert_called_once_with(
         host="broker.test", port=1883, keepalive=60
     )
 
@@ -133,6 +134,220 @@ def test_connect_uses_configured_client_id() -> None:
     assert mock_cls.call_args.kwargs["client_id"] == "home-automation-dev"
 
 
+def test_connect_same_settings_is_idempotent_even_when_disconnected() -> None:
+    manager = MqttManager()
+    settings = _make_settings()
+    client = MagicMock()
+    with patch("app.integrations.mqtt.mqtt.Client", return_value=client) as cls:
+        manager.connect(settings)
+        manager.connect(settings)
+    cls.assert_called_once()
+    client.loop_start.assert_called_once()
+    client.connect_async.assert_called_once()
+
+
+def test_connect_replaces_client_when_paho_loop_thread_has_exited() -> None:
+    manager = MqttManager()
+    settings = _make_settings()
+    old_client, recovered_client = MagicMock(), MagicMock()
+    with patch("app.integrations.mqtt.mqtt.Client", side_effect=[old_client, recovered_client]):
+        manager.connect(settings)
+        old_client._thread = None
+        manager._client_loop_started = True
+        manager.connect(settings)
+
+    old_client.disconnect.assert_called_once()
+    old_client.loop_stop.assert_called_once()
+    recovered_client.loop_start.assert_called_once()
+
+
+def test_loop_start_failure_cleans_installed_client_for_retry() -> None:
+    manager = MqttManager()
+    failed_client = MagicMock()
+    failed_client.loop_start.return_value = -1
+    recovered_client = MagicMock()
+    with patch("app.integrations.mqtt.mqtt.Client", side_effect=[failed_client, recovered_client]):
+        manager.connect(_make_settings())
+        assert manager._client is None
+        manager.connect(_make_settings())
+
+    failed_client.disconnect.assert_called_once()
+    failed_client.loop_stop.assert_called_once()
+    assert manager._client is recovered_client
+
+
+def test_concurrent_same_config_connect_creates_one_client() -> None:
+    manager = MqttManager()
+    settings = _make_settings()
+    client = MagicMock()
+    barrier = threading.Barrier(3)
+
+    def connect() -> None:
+        barrier.wait(timeout=1)
+        manager.connect(settings)
+
+    with patch("app.integrations.mqtt.mqtt.Client", return_value=client) as cls:
+        workers = [threading.Thread(target=connect) for _ in range(2)]
+        for worker in workers:
+            worker.start()
+        barrier.wait(timeout=1)
+        for worker in workers:
+            worker.join(timeout=2)
+
+    assert all(not worker.is_alive() for worker in workers)
+    cls.assert_called_once()
+    client.loop_start.assert_called_once()
+
+
+@pytest.mark.parametrize("operation", ["disconnect", "reconnect"])
+def test_teardown_can_join_loop_while_disconnect_callback_runs(operation: str) -> None:
+    """A real callback thread must finish while teardown joins the fake loop."""
+    manager = MqttManager()
+    callback_go = threading.Event()
+    callback_entered = threading.Event()
+    callback_done = threading.Event()
+    clients: list[object] = []
+
+    class LoopClient:
+        def __init__(self) -> None:
+            self._thread: threading.Thread | None = None
+            self.on_disconnect = None
+            self.loop_joined = threading.Event()
+
+        def connect_async(self, **_kwargs: object) -> None:
+            pass
+
+        def loop_start(self) -> int:
+            def network_callback() -> None:
+                callback_go.wait(timeout=2)
+                callback_entered.set()
+                try:
+                    self.on_disconnect(self, None, None, MagicMock(is_failure=True), None)
+                finally:
+                    callback_done.set()
+
+            self._thread = threading.Thread(target=network_callback, daemon=True)
+            self._thread.start()
+            return 0
+
+        def disconnect(self) -> None:
+            callback_go.set()
+
+        def loop_stop(self) -> None:
+            assert callback_entered.wait(timeout=1), "callback thread did not start"
+            assert self._thread is not None
+            self._thread.join(timeout=1)
+            assert not self._thread.is_alive(), "callback thread did not exit before loop_stop returned"
+            self.loop_joined.set()
+
+        def cleanup(self) -> None:
+            callback_go.set()
+            if self._thread is not None:
+                self._thread.join(timeout=1)
+
+    def make_client(**_kwargs: object) -> LoopClient:
+        client = LoopClient()
+        clients.append(client)
+        return client
+
+    try:
+        with patch("app.integrations.mqtt.mqtt.Client", side_effect=make_client):
+            manager.connect(_make_settings())
+            old_client = clients[0]
+            if operation == "disconnect":
+                manager.disconnect()
+                assert manager._client is None
+            else:
+                manager.reconnect(_make_settings(mqtt_client_id="replacement"))
+                assert manager._client is clients[1]
+            assert callback_done.is_set()
+            assert old_client.loop_joined.is_set()
+            assert old_client._thread is not None and not old_client._thread.is_alive()
+    finally:
+        for client in clients:
+            client.cleanup()
+
+
+@pytest.mark.parametrize("failure", ["constructor", "tls", "credentials", "connect_async", "loop_start"])
+def test_initialization_failure_is_cleaned_and_next_connect_recovers(failure: str) -> None:
+    manager = MqttManager()
+    broken, recovered = MagicMock(), MagicMock()
+    broken.connect_async.return_value = None
+    if failure == "tls":
+        broken.tls_set.side_effect = OSError("synthetic TLS failure")
+    elif failure == "credentials":
+        broken.username_pw_set.side_effect = OSError("synthetic credential failure")
+    elif failure == "connect_async":
+        broken.connect_async.side_effect = OSError("synthetic connect setup failure")
+    elif failure == "loop_start":
+        broken.loop_start.side_effect = OSError("synthetic loop failure")
+
+    settings = _make_settings(
+        mqtt_tls_enabled=failure == "tls",
+        mqtt_username="user" if failure == "credentials" else "",
+        mqtt_password="synthetic",
+    )
+    first = (
+        [RuntimeError("synthetic constructor failure"), recovered]
+        if failure == "constructor"
+        else [broken, recovered]
+    )
+    with patch("app.integrations.mqtt.mqtt.Client", side_effect=first):
+        manager.connect(settings)
+        assert manager._client is None
+        manager.connect(settings)
+
+    assert manager._client is recovered
+    recovered.loop_start.assert_called_once()
+    recovered.connect_async.assert_called_once()
+    if failure in {"connect_async", "loop_start"}:
+        broken.disconnect.assert_called_once()
+        broken.loop_stop.assert_called_once()
+    elif failure in {"tls", "credentials"}:
+        broken.loop_start.assert_not_called()
+
+
+@pytest.mark.parametrize("old_is_failure", [False, True])
+def test_old_connect_success_and_failure_callbacks_cannot_change_replacement(
+    old_is_failure: bool,
+) -> None:
+    manager = MqttManager()
+    old_client, new_client = MagicMock(), MagicMock()
+    with patch("app.integrations.mqtt.mqtt.Client", side_effect=[old_client, new_client]):
+        manager.connect(_make_settings())
+        manager.reconnect(_make_settings(mqtt_client_id="replacement"))
+
+    manager._connected = True
+    old_client.on_connect(
+        old_client, None, None, MagicMock(is_failure=old_is_failure), None
+    )
+    assert manager._client is new_client
+    assert manager.is_connected
+    new_client.subscribe.assert_not_called()
+
+
+def test_old_client_callbacks_cannot_change_replacement_state_or_dispatch() -> None:
+    manager = MqttManager()
+    settings = _make_settings()
+    old_client, new_client = MagicMock(), MagicMock()
+    received: list[bytes] = []
+    manager.subscribe("sensor/state", received.append)
+    with patch("app.integrations.mqtt.mqtt.Client", side_effect=[old_client, new_client]):
+        manager.connect(settings)
+        manager.reconnect(_make_settings(mqtt_client_id="replacement"))
+
+    manager._connected = True
+    failed = MagicMock()
+    failed.is_failure = True
+    old_client.on_disconnect(old_client, None, None, failed, None)
+    old_client.on_message(old_client, None, MagicMock(topic="sensor/state", payload=b"stale"))
+    assert manager.is_connected
+    assert received == []
+
+    new_client.on_message(new_client, None, MagicMock(topic="sensor/state", payload=b"fresh"))
+    assert received == [b"fresh"]
+
+
 def test_deployment_client_id_variants_are_distinct() -> None:
     production_ids = {
         "home-automation",
@@ -155,7 +370,7 @@ def test_connect_sets_credentials_when_username_provided() -> None:
     settings = _make_settings(mqtt_username="user", mqtt_password="s3cr3t")
 
     mock_client = MagicMock()
-    mock_client.connect.return_value = None
+    mock_client.connect_async.return_value = None
 
     with patch("app.integrations.mqtt.mqtt.Client", return_value=mock_client):
         manager.connect(settings)
@@ -170,7 +385,7 @@ def test_connect_skips_credentials_when_username_empty() -> None:
     settings = _make_settings(mqtt_username="", mqtt_password="")
 
     mock_client = MagicMock()
-    mock_client.connect.return_value = None
+    mock_client.connect_async.return_value = None
 
     with patch("app.integrations.mqtt.mqtt.Client", return_value=mock_client):
         manager.connect(settings)
@@ -183,7 +398,7 @@ def test_connect_calls_tls_set_when_tls_enabled() -> None:
     settings = _make_settings(mqtt_tls_enabled=True)
 
     mock_client = MagicMock()
-    mock_client.connect.return_value = None
+    mock_client.connect_async.return_value = None
 
     with patch("app.integrations.mqtt.mqtt.Client", return_value=mock_client):
         manager.connect(settings)
@@ -200,7 +415,7 @@ def test_connect_does_not_raise_on_connection_failure() -> None:
     settings = _make_settings()
 
     mock_client = MagicMock()
-    mock_client.connect.side_effect = OSError("Connection refused")
+    mock_client.connect_async.side_effect = OSError("Connection refused")
 
     with patch("app.integrations.mqtt.mqtt.Client", return_value=mock_client):
         # Must not raise — failures are caught and logged
@@ -221,7 +436,7 @@ def test_disconnect_stops_loop_and_clears_client() -> None:
     settings = _make_settings()
 
     mock_client = MagicMock()
-    mock_client.connect.return_value = None
+    mock_client.connect_async.return_value = None
 
     with patch("app.integrations.mqtt.mqtt.Client", return_value=mock_client):
         manager.connect(settings)
@@ -249,7 +464,7 @@ def test_publish_passes_topic_payload_retain_to_paho() -> None:
     settings = _make_settings()
 
     mock_client = MagicMock()
-    mock_client.connect.return_value = None
+    mock_client.connect_async.return_value = None
 
     with patch("app.integrations.mqtt.mqtt.Client", return_value=mock_client):
         manager.connect(settings)
@@ -306,7 +521,7 @@ def test_reconnect_stops_old_client_and_starts_new_one() -> None:
     manager._connected = True
 
     new_client = MagicMock()
-    new_client.connect.return_value = None
+    new_client.connect_async.return_value = None
 
     with patch("app.integrations.mqtt.mqtt.Client", return_value=new_client):
         manager.reconnect(settings)
@@ -317,11 +532,60 @@ def test_reconnect_stops_old_client_and_starts_new_one() -> None:
 
     # New client must have been connected
     new_client.loop_start.assert_called_once()
-    new_client.connect.assert_called_once_with(
+    new_client.connect_async.assert_called_once_with(
         host="broker.test", port=1883, keepalive=60
     )
 
 
+@pytest.mark.parametrize(
+    "settings",
+    [
+        _make_settings(mqtt_enabled=False),
+        _make_settings(mqtt_broker_host=""),
+    ],
+    ids=["disabled", "empty-host"],
+)
+def test_unconfigured_reconnect_stops_main_client_but_keeps_dsmr_source(settings) -> None:
+    """Changing main MQTT config must not interrupt independent DSMR clients."""
+    manager = MqttManager()
+    main_client = MagicMock()
+    source_client = MagicMock()
+    received: list[bytes] = []
+
+    with patch(
+        "app.integrations.mqtt.mqtt.Client",
+        side_effect=[main_client, source_client],
+    ):
+        manager.connect(_make_settings())
+        assert manager.replace_source(
+            17,
+            host="source.test",
+            port=1883,
+            username="",
+            password="",
+            tls_enabled=False,
+            subscriptions={"meter/17": received.append},
+        )
+
+    manager.reconnect(settings)
+
+    assert manager._client is None
+    main_client.disconnect.assert_called_once()
+    main_client.loop_stop.assert_called_once()
+    assert manager._source_clients[17] is source_client
+    assert manager.source_is_active(17)
+    source_client.disconnect.assert_not_called()
+    source_client.loop_stop.assert_not_called()
+
+    message = MagicMock()
+    message.topic = "meter/17"
+    message.payload = b"reading"
+    source_client.on_message(source_client, None, message)
+    assert received == [b"reading"]
+
+    manager.disconnect()
+
+
 # ---------------------------------------------------------------------------
 # POST /api/config/mqtt/test — session + CSRF guards
 # ---------------------------------------------------------------------------

tests/test_mqtt_scheduler.py

@@ -0,0 +1,68 @@
+"""Offline regression coverage for scheduler-driven MQTT recovery."""
+from __future__ import annotations
+
+from unittest.mock import MagicMock, patch
+
+from app.integrations.mqtt import MqttManager
+from app.main import _run_scheduled_ha_state_publish
+
+
+def test_scheduler_reuses_manager_across_disconnects_then_subscribes_and_publishes() -> None:
+    session = MagicMock()
+    session_local = MagicMock(return_value=session)
+    settings = MagicMock(
+        mqtt_enabled=True,
+        mqtt_broker_host="synthetic-broker",
+        mqtt_broker_port=1883,
+        mqtt_username="",
+        mqtt_password="",
+        mqtt_tls_enabled=False,
+        mqtt_client_id="offline-test",
+    )
+    manager = MqttManager()
+    clients: list[MagicMock] = []
+
+    def make_client(*_args: object, **_kwargs: object) -> MagicMock:
+        client = MagicMock()
+        clients.append(client)
+        return client
+
+    manager.subscribe("homeassistant/status", lambda _payload: None)
+
+    def publish_states(_session: object) -> None:
+        manager.publish("homeassistant/state", b"state", retain=True)
+
+    with (
+        patch("app.main.get_session_local", return_value=session_local),
+        patch("app.main.build_runtime_settings", return_value=settings),
+        patch("app.main.get_settings", return_value=MagicMock()),
+        patch("app.main.mqtt_manager", manager),
+        patch("app.main.publish_discovery") as publish_discovery,
+        patch("app.main.publish_states", side_effect=publish_states),
+        patch("app.integrations.mqtt.mqtt.Client", side_effect=make_client),
+    ):
+        for _ in range(3):
+            _run_scheduled_ha_state_publish()
+            assert len(clients) == 1
+            assert clients[0].loop_start.call_count == 1
+            clients[0].on_disconnect(
+                clients[0], None, None, MagicMock(is_failure=True), None
+            )
+            assert not manager.is_connected
+
+        # A subsequent scheduler turn sees Paho's existing loop retrying; it
+        # must not replace that client. Simulate the successful CONNACK.
+        _run_scheduled_ha_state_publish()
+        assert len(clients) == 1
+        clients[0].on_connect(clients[0], None, None, MagicMock(is_failure=False), None)
+
+        assert manager.is_connected
+        clients[0].subscribe.assert_called_with("homeassistant/status")
+        _run_scheduled_ha_state_publish()
+        assert len(clients) == 1
+        clients[0].publish.assert_called_once_with(
+            "homeassistant/state", payload=b"state", qos=0, retain=True
+        )
+        assert publish_discovery.call_count == 5
+        assert session.close.call_count == 5
+        manager.disconnect()

tests/test_mqtt_subscribe.py

@@ -143,7 +143,7 @@ def test_unsubscribe_unknown_topic_is_noop() -> None:
 # ---------------------------------------------------------------------------
 
 
-def _extract_on_message(manager: MqttManager, settings) -> object:
+def _extract_on_message(manager: MqttManager, settings) -> tuple[object, MagicMock]:
     """Connect with a mocked paho client and return the on_message callback that
     was registered on the client mock."""
     mock_client = MagicMock()
@@ -153,7 +153,7 @@ def _extract_on_message(manager: MqttManager, settings) -> object:
         manager.connect(settings)
 
     # The callback was assigned as: client.on_message = _on_message
-    return mock_client.on_message
+    return mock_client.on_message, mock_client
 
 
 def test_on_message_dispatches_to_registered_handler() -> None:
@@ -168,10 +168,10 @@ def test_on_message_dispatches_to_registered_handler() -> None:
 
     manager.subscribe("dsmr/json", handler)
 
-    on_message = _extract_on_message(manager, settings)
+    on_message, client = _extract_on_message(manager, settings)
 
     msg = _make_mqtt_message("dsmr/json", b'{"id": 1}')
-    on_message(MagicMock(), None, msg)  # simulate paho calling the callback
+    on_message(client, None, msg)  # simulate paho calling the callback
 
     assert received_payloads == [b'{"id": 1}']
 
@@ -183,11 +183,11 @@ def test_on_message_ignores_unknown_topic() -> None:
     handler = MagicMock()
     manager.subscribe("other/topic", handler)
 
-    on_message = _extract_on_message(manager, settings)
+    on_message, client = _extract_on_message(manager, settings)
 
     msg = _make_mqtt_message("unknown/topic", b"data")
     # Must not raise
-    on_message(MagicMock(), None, msg)
+    on_message(client, None, msg)
 
     handler.assert_not_called()
 
@@ -201,11 +201,11 @@ def test_on_message_swallows_handler_exception() -> None:
         raise RuntimeError("handler exploded")
 
     manager.subscribe("dsmr/json", bad_handler)
-    on_message = _extract_on_message(manager, settings)
+    on_message, client = _extract_on_message(manager, settings)
 
     msg = _make_mqtt_message("dsmr/json", b"{}")
     # This must not raise
-    on_message(MagicMock(), None, msg)
+    on_message(client, None, msg)
 
 
 def test_on_message_does_not_crash_on_handler_exception_multiple_calls() -> None:
@@ -221,13 +221,13 @@ def test_on_message_does_not_crash_on_handler_exception_multiple_calls() -> None
             raise ValueError("first call fails")
 
     manager.subscribe("dsmr/json", flaky_handler)
-    on_message = _extract_on_message(manager, settings)
+    on_message, client = _extract_on_message(manager, settings)
 
     msg1 = _make_mqtt_message("dsmr/json", b"first")
     msg2 = _make_mqtt_message("dsmr/json", b"second")
 
-    on_message(MagicMock(), None, msg1)  # should not raise despite exception in handler
-    on_message(MagicMock(), None, msg2)  # handler called again
+    on_message(client, None, msg1)  # should not raise despite exception in handler
+    on_message(client, None, msg2)  # handler called again
 
     assert call_count[0] == 2