Fix deadlock in ReloadServiceHelper (#162775)

homeassistant/helpers/service.py

@@ -1091,11 +1091,13 @@ class ReloadServiceHelper[_T]:
 
         if do_reload:
             # Reload, then notify other tasks
-            await self._service_func(service_call)
-            async with self._service_condition:
-                self._service_running = False
-                self._pending_reload_targets -= reload_targets
-                self._service_condition.notify_all()
+            try:
+                await self._service_func(service_call)
+            finally:
+                async with self._service_condition:
+                    self._service_running = False
+                    self._pending_reload_targets -= reload_targets
+                    self._service_condition.notify_all()
 
 
 def _validate_entity_service_schema(

tests/helpers/test_service.py

@@ -2466,11 +2466,14 @@ async def test_reload_service_helper(hass: HomeAssistant) -> None:
     """Test the reload service helper."""
 
     active_reload_calls = 0
+    service_error: type[Exception] | None = None
     reloaded = []
 
     async def reload_service_handler(service_call: ServiceCall) -> None:
         """Remove all automations and load new ones from config."""
         nonlocal active_reload_calls
+        if service_error:
+            raise service_error
         # Assert the reload helper prevents parallel reloads
         assert not active_reload_calls
         active_reload_calls += 1
@@ -2677,6 +2680,41 @@ async def test_reload_service_helper(hass: HomeAssistant) -> None:
     await asyncio.gather(*tasks)
     assert reloaded == unordered(["all", "target1", "target2", "target3", "target4"])
 
+    # Test error handling when reload fails, and that we can recover from it
+    reloaded.clear()
+    service_error = Exception("Test error")
+    tasks = [
+        # This reload task will start executing first, (all)
+        reloader.execute_service(ServiceCall(hass, "test", "test")),
+        # These reload tasks will be deduplicated to (target1, target2, target3, target4)
+        # while the first task is reloaded.
+        reloader.execute_service(
+            ServiceCall(hass, "test", "test", {"target": "target1"})
+        ),
+        reloader.execute_service(
+            ServiceCall(hass, "test", "test", {"target": "target2"})
+        ),
+        reloader.execute_service(
+            ServiceCall(hass, "test", "test", {"target": "target3"})
+        ),
+        reloader.execute_service(
+            ServiceCall(hass, "test", "test", {"target": "target4"})
+        ),
+    ]
+    with pytest.raises(Exception, match="Test error"):
+        await asyncio.gather(*tasks)
+    assert reloaded == unordered([])
+
+    service_error = None
+    tasks2 = [
+        reloader.execute_service(
+            ServiceCall(hass, "test", "test", {"target": "target1"})
+        ),
+    ]
+    await asyncio.gather(*tasks2)
+    # We don't try to reload the failed targets again, so only the new reload is executed
+    assert reloaded == unordered(["target1"])
+
 
 async def test_deprecated_service_target_selector_class(hass: HomeAssistant) -> None:
     """Test that the deprecated ServiceTargetSelector class forwards correctly."""