Fix MQTT discovery failing after bad config update (#86935)

* Fix MQTT discovery failing after bad config update

* Update last discovery payload after update success

* Improve test, correct update assignment

* send_discovery_done to finally-catch vol.Error

* Just use try..finally

* Remove extra line

* use elif to avoid log confusion

homeassistant/components/mqtt/mixins.py

@@ -744,8 +744,12 @@ class MqttDiscoveryDeviceUpdate(ABC):
                 self.log_name,
                 discovery_hash,
             )
-            await self.async_update(discovery_payload)
+            try:
-        if not discovery_payload:
+                await self.async_update(discovery_payload)
+            finally:
+                send_discovery_done(self.hass, self._discovery_data)
+            self._discovery_data[ATTR_DISCOVERY_PAYLOAD] = discovery_payload
+        elif not discovery_payload:
             # Unregister and clean up the current discovery instance
             stop_discovery_updates(
                 self.hass, self._discovery_data, self._remove_discovery_updated
@@ -869,15 +873,19 @@ class MqttDiscoveryUpdate(Entity):
                 _LOGGER.info("Removing component: %s", self.entity_id)
                 self._cleanup_discovery_on_remove()
                 await _async_remove_state_and_registry_entry(self)
+                send_discovery_done(self.hass, self._discovery_data)
             elif self._discovery_update:
                 if old_payload != self._discovery_data[ATTR_DISCOVERY_PAYLOAD]:
                     # Non-empty, changed payload: Notify component
                     _LOGGER.info("Updating component: %s", self.entity_id)
-                    await self._discovery_update(payload)
+                    try:
+                        await self._discovery_update(payload)
+                    finally:
+                        send_discovery_done(self.hass, self._discovery_data)
                 else:
                     # Non-empty, unchanged payload: Ignore to avoid changing states
                     _LOGGER.debug("Ignoring unchanged update for: %s", self.entity_id)
-            send_discovery_done(self.hass, self._discovery_data)
+                    send_discovery_done(self.hass, self._discovery_data)
 
         if discovery_hash:
             assert self._discovery_data is not None

tests/components/mqtt/test_discovery.py

@@ -6,6 +6,7 @@ import re
 from unittest.mock import AsyncMock, call, patch
 
 import pytest
+from voluptuous import MultipleInvalid
 
 from homeassistant import config_entries
 from homeassistant.components import mqtt
@@ -1494,7 +1495,7 @@ async def test_clean_up_registry_monitoring(
 async def test_unique_id_collission_has_priority(
     hass, mqtt_mock_entry_no_yaml_config, entity_reg
 ):
-    """Test tehe unique_id collision detection has priority over registry disabled items."""
+    """Test the unique_id collision detection has priority over registry disabled items."""
     await mqtt_mock_entry_no_yaml_config()
     config = {
         "name": "sbfspot_12345",
@@ -1534,3 +1535,57 @@ async def test_unique_id_collission_has_priority(
     assert entity_reg.async_get("sensor.sbfspot_12345_1") is not None
     # Verify the second entity is not created because it is not unique
     assert entity_reg.async_get("sensor.sbfspot_12345_2") is None
+
+
+@pytest.mark.xfail(raises=MultipleInvalid)
+@patch("homeassistant.components.mqtt.PLATFORMS", [Platform.SENSOR])
+async def test_update_with_bad_config_not_breaks_discovery(
+    hass: ha.HomeAssistant, mqtt_mock_entry_no_yaml_config, entity_reg
+) -> None:
+    """Test a bad update does not break discovery."""
+    await mqtt_mock_entry_no_yaml_config()
+    # discover a sensor
+    config1 = {
+        "name": "sbfspot_12345",
+        "state_topic": "homeassistant_test/sensor/sbfspot_0/state",
+    }
+    async_fire_mqtt_message(
+        hass,
+        "homeassistant/sensor/sbfspot_0/config",
+        json.dumps(config1),
+    )
+    await hass.async_block_till_done()
+    assert hass.states.get("sensor.sbfspot_12345") is not None
+    # update with a breaking config
+    config2 = {
+        "name": "sbfspot_12345",
+        "availability": 1,
+        "state_topic": "homeassistant_test/sensor/sbfspot_0/state",
+    }
+    async_fire_mqtt_message(
+        hass,
+        "homeassistant/sensor/sbfspot_0/config",
+        json.dumps(config2),
+    )
+    await hass.async_block_till_done()
+    # update the state topic
+    config3 = {
+        "name": "sbfspot_12345",
+        "state_topic": "homeassistant_test/sensor/sbfspot_0/new_state_topic",
+    }
+    async_fire_mqtt_message(
+        hass,
+        "homeassistant/sensor/sbfspot_0/config",
+        json.dumps(config3),
+    )
+    await hass.async_block_till_done()
+
+    # Send an update for the state
+    async_fire_mqtt_message(
+        hass,
+        "homeassistant_test/sensor/sbfspot_0/new_state_topic",
+        "new_value",
+    )
+    await hass.async_block_till_done()
+
+    assert hass.states.get("sensor.sbfspot_12345").state == "new_value"

tests/components/mqtt/test_tag.py

@@ -4,10 +4,12 @@ import json
 from unittest.mock import ANY, patch
 
 import pytest
+from voluptuous import MultipleInvalid
 
 from homeassistant.components.device_automation import DeviceAutomationType
 from homeassistant.components.mqtt.const import DOMAIN as MQTT_DOMAIN
 from homeassistant.const import Platform
+from homeassistant.core import HomeAssistant
 from homeassistant.helpers import device_registry as dr
 from homeassistant.setup import async_setup_component
 
@@ -805,6 +807,50 @@ async def test_cleanup_device_with_entity2(
     assert device_entry is None
 
 
+@pytest.mark.xfail(raises=MultipleInvalid)
+async def test_update_with_bad_config_not_breaks_discovery(
+    hass: HomeAssistant,
+    mqtt_mock_entry_no_yaml_config,
+    tag_mock,
+) -> None:
+    """Test a bad update does not break discovery."""
+    await mqtt_mock_entry_no_yaml_config()
+    config1 = {
+        "topic": "test-topic",
+        "device": {"identifiers": ["helloworld"]},
+    }
+    config2 = {
+        "topic": "test-topic",
+        "device": {"bad_key": "some bad value"},
+    }
+
+    config3 = {
+        "topic": "test-topic-update",
+        "device": {"identifiers": ["helloworld"]},
+    }
+
+    data1 = json.dumps(config1)
+    data2 = json.dumps(config2)
+    data3 = json.dumps(config3)
+
+    async_fire_mqtt_message(hass, "homeassistant/tag/bla1/config", data1)
+    await hass.async_block_till_done()
+
+    # Update with bad identifier
+    async_fire_mqtt_message(hass, "homeassistant/tag/bla1/config", data2)
+    await hass.async_block_till_done()
+
+    # Topic update
+    async_fire_mqtt_message(hass, "homeassistant/tag/bla1/config", data3)
+    await hass.async_block_till_done()
+
+    # Fake tag scan.
+    async_fire_mqtt_message(hass, "test-topic-update", "12345")
+
+    await hass.async_block_till_done()
+    tag_mock.assert_called_once_with(ANY, "12345", ANY)
+
+
 async def test_unload_entry(hass, device_reg, mqtt_mock, tag_mock, tmp_path) -> None:
     """Test unloading the MQTT entry."""