From ae53b9fc9f70a85de155497fa7d6ad8657397eae Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=E9=82=B9=E6=B0=B8=E8=B5=AB?= <1259085392@qq.com>
Date: Mon, 9 Mar 2026 19:36:55 +0900
Subject: [PATCH] fix: harden runtime cleanup review fixes

Continue terminating remaining providers and disable MCP servers even if one provider terminate hook fails.

Also add InitialLoader failure-path coverage and extract guarded plugin routes into a shared constant for easier review and maintenance.
---
 astrbot/dashboard/routes/plugin.py | 34 ++++++++++++++++--------------
 tests/unit/test_core_lifecycle.py  | 28 ++++++++++++++++++++++++
 tests/unit/test_initial_loader.py  | 29 +++++++++++++++++++++++++
 3 files changed, 75 insertions(+), 16 deletions(-)

diff --git a/astrbot/dashboard/routes/plugin.py b/astrbot/dashboard/routes/plugin.py
index e59da5577..d4bf81ffb 100644
--- a/astrbot/dashboard/routes/plugin.py
+++ b/astrbot/dashboard/routes/plugin.py
@@ -36,6 +36,23 @@ PLUGIN_UPDATE_CONCURRENCY = (
     3  # limit concurrent updates to avoid overwhelming plugin sources
 )
 
+RUNTIME_GUARDED_PLUGIN_PATHS = {
+    "/plugin/get",
+    "/plugin/install",
+    "/plugin/install-upload",
+    "/plugin/update",
+    "/plugin/update-all",
+    "/plugin/uninstall",
+    "/plugin/uninstall-failed",
+    "/plugin/off",
+    "/plugin/on",
+    "/plugin/reload-failed",
+    "/plugin/reload",
+    "/plugin/readme",
+    "/plugin/changelog",
+    "/plugin/source/get-failed-plugins",
+}
+
 
 @dataclass
 class RegistrySource:
@@ -80,22 +97,7 @@ class PluginRoute(Route):
         )
         for path, definition in list(self.routes.items()):
             method, handler = definition
-            if path in {
-                "/plugin/get",
-                "/plugin/install",
-                "/plugin/install-upload",
-                "/plugin/update",
-                "/plugin/update-all",
-                "/plugin/uninstall",
-                "/plugin/uninstall-failed",
-                "/plugin/off",
-                "/plugin/on",
-                "/plugin/reload-failed",
-                "/plugin/reload",
-                "/plugin/readme",
-                "/plugin/changelog",
-                "/plugin/source/get-failed-plugins",
-            }:
+            if path in RUNTIME_GUARDED_PLUGIN_PATHS:
                 self.routes[path] = (method, self._guard_runtime_ready(handler))
         self.register_routes()
 
diff --git a/tests/unit/test_core_lifecycle.py b/tests/unit/test_core_lifecycle.py
index babfa5f30..1ac540ff5 100644
--- a/tests/unit/test_core_lifecycle.py
+++ b/tests/unit/test_core_lifecycle.py
@@ -419,6 +419,34 @@ class TestProviderManagerCleanup:
         assert provider_manager.curr_stt_provider_inst is None
         assert provider_manager.curr_tts_provider_inst is None
 
+    @pytest.mark.asyncio
+    async def test_terminate_continues_when_individual_provider_terminate_fails(self):
+        """Test terminate keeps cleaning remaining providers and MCP servers after one provider fails."""
+        provider_manager = build_provider_manager_for_tests()
+        provider_manager.llm_tools.disable_mcp_server = AsyncMock()
+
+        failing_provider = MagicMock()
+        failing_provider.meta.return_value.id = "failing"
+        failing_provider.terminate = AsyncMock(side_effect=RuntimeError("terminate failed"))
+
+        healthy_provider = MagicMock()
+        healthy_provider.meta.return_value.id = "healthy"
+        healthy_provider.terminate = AsyncMock()
+
+        provider_manager.provider_insts = [failing_provider, healthy_provider]
+        provider_manager.inst_map = {
+            "failing": failing_provider,
+            "healthy": healthy_provider,
+        }
+
+        with patch("astrbot.core.provider.manager.logger") as mock_logger:
+            await provider_manager.terminate()
+
+        failing_provider.terminate.assert_awaited_once()
+        healthy_provider.terminate.assert_awaited_once()
+        provider_manager.llm_tools.disable_mcp_server.assert_awaited_once()
+        mock_logger.error.assert_called()
+
 
 class TestContextRuntimeRegistrations:
     """Tests for runtime registration containers on Context."""
diff --git a/tests/unit/test_initial_loader.py b/tests/unit/test_initial_loader.py
index c820c2837..62252ad82 100644
--- a/tests/unit/test_initial_loader.py
+++ b/tests/unit/test_initial_loader.py
@@ -75,3 +75,32 @@ async def test_initial_loader_start_awaits_initialize_core_and_schedules_runtime
     assert call_order[:3] == ["initialize_core", "create_task", "dashboard_init"]
     assert len(created_tasks) == 1
     assert lifecycle.runtime_bootstrap_task is created_tasks[0]
+
+
+@pytest.mark.asyncio
+async def test_initial_loader_start_returns_without_partial_start_when_initialize_core_fails():
+    """Test InitialLoader.start aborts cleanly if initialize_core fails."""
+    loader = InitialLoader(MagicMock(), MagicMock())
+
+    lifecycle = MagicMock()
+    lifecycle.runtime_bootstrap_task = None
+    expected_error = RuntimeError("core init failed")
+    lifecycle.initialize_core = AsyncMock(side_effect=expected_error)
+    lifecycle.bootstrap_runtime = AsyncMock()
+    lifecycle.start = AsyncMock()
+
+    with (
+        patch(
+            "astrbot.core.initial_loader.AstrBotCoreLifecycle", return_value=lifecycle
+        ),
+        patch("astrbot.core.initial_loader.AstrBotDashboard") as dashboard_cls,
+        patch("astrbot.core.initial_loader.asyncio.create_task") as create_task,
+    ):
+        await loader.start()
+
+    lifecycle.initialize_core.assert_awaited_once()
+    dashboard_cls.assert_not_called()
+    create_task.assert_not_called()
+    lifecycle.bootstrap_runtime.assert_not_called()
+    lifecycle.start.assert_not_called()
+    assert lifecycle.runtime_bootstrap_task is None