Merge remote-tracking branch 'origin/main' into tests/prune-low-value

# Conflicts:
#	tests/agent/test_context_compressor.py
#	tests/gateway/test_startup_restart_race.py
#	tests/hermes_cli/test_voice_wrapper.py

tests/tools/test_mcp_initial_connect_shutdown.py

@@ -0,0 +1,273 @@
+"""Regression tests for initial MCP failure ownership and teardown."""
+
+import asyncio
+import json
+import threading
+from types import SimpleNamespace
+
+import pytest
+
+
+def _reset_mcp_state(mcp_tool) -> None:
+    mcp_tool.shutdown_mcp_servers()
+    with mcp_tool._lock:
+        mcp_tool._servers.clear()
+        mcp_tool._server_connecting.clear()
+        mcp_tool._server_connect_errors.clear()
+
+
+def _cleanup_mcp_state(mcp_tool, extra_servers=()) -> None:
+    with mcp_tool._lock:
+        loop = mcp_tool._mcp_loop
+    if loop is not None and loop.is_running():
+        for server in extra_servers:
+            task = getattr(server, "_task", None)
+            if task is not None and not task.done():
+                mcp_tool._run_on_mcp_loop(server.shutdown, timeout=5)
+    mcp_tool.shutdown_mcp_servers()
+    with mcp_tool._lock:
+        mcp_tool._servers.clear()
+        mcp_tool._server_connecting.clear()
+        mcp_tool._server_connect_errors.clear()
+
+
+def test_initial_connect_failure_is_registry_owned_and_reaped(monkeypatch, tmp_path):
+    """Normal discovery must retain the parked task for clean shutdown."""
+    monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+
+    from tools import mcp_tool
+
+    _reset_mcp_state(mcp_tool)
+    created = []
+
+    class _FailingServerTask(mcp_tool.MCPServerTask):
+        def __init__(self, name):
+            super().__init__(name)
+            created.append(self)
+
+        async def _run_stdio(self, config):
+            raise ConnectionError("deterministic initial failure")
+
+    monkeypatch.setattr(mcp_tool, "MCPServerTask", _FailingServerTask)
+    monkeypatch.setattr(mcp_tool, "_MCP_AVAILABLE", True)
+    monkeypatch.setattr(mcp_tool, "_MAX_INITIAL_CONNECT_RETRIES", 0)
+    monkeypatch.setattr(mcp_tool, "_PARKED_RETRY_INTERVAL", 3600)
+
+    real_stop = mcp_tool._stop_mcp_loop
+    pending_at_stop = []
+
+    async def _pending_tasks():
+        current = asyncio.current_task()
+        return sorted(
+            task.get_coro().__qualname__
+            for task in asyncio.all_tasks()
+            if task is not current and not task.done()
+        )
+
+    def _observed_stop(*, only_if_idle=False):
+        pending_at_stop.extend(
+            mcp_tool._run_on_mcp_loop(_pending_tasks, timeout=5)
+        )
+        return real_stop(only_if_idle=only_if_idle)
+
+    monkeypatch.setattr(mcp_tool, "_stop_mcp_loop", _observed_stop)
+
+    try:
+        assert mcp_tool.register_mcp_servers({
+            "initial-failure": {"command": "unused", "connect_timeout": 5}
+        }) == []
+
+        assert len(created) == 1
+        server = created[0]
+        with mcp_tool._lock:
+            assert mcp_tool._servers["initial-failure"] is server
+            assert "deterministic initial failure" in (
+                mcp_tool._server_connect_errors["initial-failure"]
+            )
+        assert server._task is not None
+        assert not server._task.done(), "recoverable initial failure was not parked"
+
+        mcp_tool.shutdown_mcp_servers()
+
+        assert pending_at_stop == [], (
+            "shutdown left MCP tasks pending at loop stop: "
+            f"{pending_at_stop!r}"
+        )
+        assert server._task.done()
+        with mcp_tool._lock:
+            assert mcp_tool._mcp_loop is None
+            assert mcp_tool._mcp_thread is None
+    finally:
+        monkeypatch.setattr(mcp_tool, "_stop_mcp_loop", real_stop)
+        _cleanup_mcp_state(mcp_tool, created)
+
+
+def test_initial_connect_failure_revives_same_registered_server(monkeypatch, tmp_path):
+    """A cached parked failure must revive through register_mcp_servers()."""
+    monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+
+    from tools import mcp_tool
+    from tools.registry import ToolRegistry
+    import tools.registry as registry_module
+
+    _reset_mcp_state(mcp_tool)
+    created = []
+    backend_up = threading.Event()
+    revived = threading.Event()
+    state = {"transport_calls": 0, "tool_calls": 0}
+    mock_registry = ToolRegistry()
+
+    class _Session:
+        async def call_tool(self, name, arguments):
+            state["tool_calls"] += 1
+            return SimpleNamespace(
+                isError=False,
+                content=[SimpleNamespace(text=f"revived:{arguments['value']}")],
+                structuredContent=None,
+            )
+
+    class _RecoveringServerTask(mcp_tool.MCPServerTask):
+        def __init__(self, name):
+            super().__init__(name)
+            created.append(self)
+
+        async def _run_stdio(self, config):
+            assert mcp_tool._connect_server_claim.get() is None
+            state["transport_calls"] += 1
+            if not backend_up.is_set():
+                raise ConnectionError("backend still booting")
+
+            self.session = _Session()
+            self._tools = [SimpleNamespace(
+                name="ping",
+                description="Return a deterministic revival result",
+                inputSchema={
+                    "type": "object",
+                    "properties": {"value": {"type": "string"}},
+                    "required": ["value"],
+                },
+            )]
+            # Match the real transports: discovery runs before _ready is set.
+            self._register_discovered_tools_if_needed()
+            self._ready.set()
+            revived.set()
+            return await self._wait_for_lifecycle_event()
+
+    monkeypatch.setattr(mcp_tool, "MCPServerTask", _RecoveringServerTask)
+    monkeypatch.setattr(mcp_tool, "_MCP_AVAILABLE", True)
+    monkeypatch.setattr(mcp_tool, "_MAX_INITIAL_CONNECT_RETRIES", 0)
+    monkeypatch.setattr(mcp_tool, "_PARKED_RETRY_INTERVAL", 3600)
+    monkeypatch.setattr(registry_module, "registry", mock_registry)
+
+    config = {
+        "recovering": {"command": "unused", "connect_timeout": 5}
+    }
+
+    try:
+        assert mcp_tool.register_mcp_servers(config) == []
+        assert len(created) == 1
+        server = created[0]
+        with mcp_tool._lock:
+            assert mcp_tool._servers["recovering"] is server
+            assert "backend still booting" in (
+                mcp_tool._server_connect_errors["recovering"]
+            )
+        assert not server._task.done()
+
+        backend_up.set()
+        mcp_tool.register_mcp_servers(config)
+
+        assert revived.wait(timeout=5), "cached parked server did not revive"
+        assert len(created) == 1, "revival created a duplicate server task"
+        with mcp_tool._lock:
+            assert mcp_tool._servers["recovering"] is server
+            assert "recovering" not in mcp_tool._server_connect_errors
+        assert state["transport_calls"] == 2
+        assert server.session is not None
+        assert server._error is None
+
+        entry = mock_registry.get_entry("mcp__recovering__ping")
+        assert entry is not None
+        assert entry.check_fn() is True
+        assert json.loads(entry.handler({"value": "ok"})) == {
+            "result": "revived:ok"
+        }
+        assert state["tool_calls"] == 1
+    finally:
+        _cleanup_mcp_state(mcp_tool, created)
+
+
+def test_terminal_initial_failure_is_not_retained(monkeypatch, tmp_path):
+    """A non-recoverable startup error must not leave a dead cache entry."""
+    monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+
+    from tools import mcp_tool
+
+    _reset_mcp_state(mcp_tool)
+    created = []
+
+    class _AuthFailingServerTask(mcp_tool.MCPServerTask):
+        def __init__(self, name):
+            super().__init__(name)
+            created.append(self)
+
+        async def _run_stdio(self, config):
+            raise PermissionError("terminal authentication failure")
+
+    monkeypatch.setattr(mcp_tool, "MCPServerTask", _AuthFailingServerTask)
+    monkeypatch.setattr(mcp_tool, "_MCP_AVAILABLE", True)
+    monkeypatch.setattr(mcp_tool, "_is_auth_error", lambda exc: True)
+
+    try:
+        assert mcp_tool.register_mcp_servers({
+            "auth-failure": {"command": "unused", "connect_timeout": 5}
+        }) == []
+        assert len(created) == 1
+        assert created[0]._task.done()
+        with mcp_tool._lock:
+            assert "auth-failure" not in mcp_tool._servers
+            assert "terminal authentication failure" in (
+                mcp_tool._server_connect_errors["auth-failure"]
+            )
+    finally:
+        _cleanup_mcp_state(mcp_tool, created)
+
+
+def test_standalone_failed_connect_is_reaped_without_global_owner(monkeypatch, tmp_path):
+    """Probe-only _connect_server failures must not publish parked servers."""
+    monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+
+    from tools import mcp_tool
+
+    _reset_mcp_state(mcp_tool)
+    created = []
+
+    class _ProbeServerTask(mcp_tool.MCPServerTask):
+        def __init__(self, name):
+            super().__init__(name)
+            created.append(self)
+
+        async def _run_stdio(self, config):
+            raise ConnectionError("probe target unavailable")
+
+    monkeypatch.setattr(mcp_tool, "MCPServerTask", _ProbeServerTask)
+    monkeypatch.setattr(mcp_tool, "_MAX_INITIAL_CONNECT_RETRIES", 0)
+    monkeypatch.setattr(mcp_tool, "_PARKED_RETRY_INTERVAL", 3600)
+    mcp_tool._ensure_mcp_loop()
+
+    try:
+        with pytest.raises(ConnectionError, match="probe target unavailable"):
+            mcp_tool._run_on_mcp_loop(
+                lambda: mcp_tool._connect_server(
+                    "probe-only", {"command": "unused"}
+                ),
+                timeout=5,
+            )
+
+        assert len(created) == 1
+        assert created[0]._task.done()
+        with mcp_tool._lock:
+            assert "probe-only" not in mcp_tool._servers
+            assert "probe-only" not in mcp_tool._server_connect_errors
+    finally:
+        _cleanup_mcp_state(mcp_tool, created)

tests/tools/test_mcp_stability.py

@@ -514,3 +514,197 @@ class TestMCPInitialConnectionRetry:
                 await task
 
         asyncio.get_event_loop().run_until_complete(_run())
+
+
+# ---------------------------------------------------------------------------
+# Fix: drain pending tasks before closing the MCP loop
+# ---------------------------------------------------------------------------
+
+class TestMCPLoopDrainOnStop:
+    """_stop_mcp_loop reaps pending tasks while the loop is still open."""
+
+    def test_pending_task_cleanup_runs_before_close(self):
+        """A task left on the loop must finish its cleanup before close.
+
+        Regression for #60197: a task still suspended when the loop closes is
+        resumed later by the GC, and its ``finally`` then drives ``cancel()``
+        -> ``call_soon()`` against the closed loop, surfacing as an ignored
+        ``RuntimeError: Event loop is closed``. Servers absent from
+        ``_servers`` (e.g. parked after an initial-connect failure) never get
+        ``shutdown()``, so this drain is their only reaper.
+        """
+        import time
+        import tools.mcp_tool as mcp_mod
+
+        state = {
+            "started": False,
+            "cleanup_ran": False,
+            "cleanup_error": None,
+            "task": None,
+        }
+
+        async def _parked():
+            state["task"] = asyncio.current_task()
+            state["started"] = True
+            try:
+                await asyncio.sleep(3600)
+            finally:
+                # Mirrors _wait_for_reconnect_or_shutdown's finally: cancelling
+                # a helper task needs call_soon(), which a closed loop rejects.
+                try:
+                    helper = asyncio.ensure_future(asyncio.sleep(0))
+                    helper.cancel()
+                    state["cleanup_ran"] = True
+                except BaseException as exc:
+                    state["cleanup_error"] = exc
+
+        with mcp_mod._lock:
+            mcp_mod._servers.clear()
+            mcp_mod._server_connecting.clear()
+        try:
+            mcp_mod._ensure_mcp_loop()
+            with mcp_mod._lock:
+                loop = mcp_mod._mcp_loop
+            assert loop is not None
+            asyncio.run_coroutine_threadsafe(_parked(), loop)
+
+            deadline = time.monotonic() + 5
+            while not state["started"] and time.monotonic() < deadline:
+                time.sleep(0.01)
+            assert state["started"], "task never started on the MCP loop"
+
+            stop_saw_cleanup = []
+            call_soon = loop.call_soon
+            call_soon_threadsafe = loop.call_soon_threadsafe
+
+            def record_stop_order(schedule, callback, *args, **kwargs):
+                if (
+                    getattr(callback, "__self__", None) is loop
+                    and getattr(callback, "__name__", None) == "stop"
+                ):
+                    stop_saw_cleanup.append(state["cleanup_ran"])
+                return schedule(callback, *args, **kwargs)
+
+            with (
+                patch.object(
+                    loop,
+                    "call_soon",
+                    side_effect=lambda callback, *args, **kwargs: record_stop_order(
+                        call_soon, callback, *args, **kwargs
+                    ),
+                ),
+                patch.object(
+                    loop,
+                    "call_soon_threadsafe",
+                    side_effect=lambda callback, *args, **kwargs: record_stop_order(
+                        call_soon_threadsafe, callback, *args, **kwargs
+                    ),
+                ),
+            ):
+                mcp_mod._stop_mcp_loop()
+
+            assert state["task"] is not None
+            assert state["task"].done(), "task left pending when the loop closed"
+            assert state["cleanup_error"] is None, (
+                f"cleanup ran against a closed loop: {state['cleanup_error']!r}"
+            )
+            assert state["cleanup_ran"], "task cleanup never ran"
+            assert stop_saw_cleanup == [True], "loop.stop ran before task cleanup"
+        finally:
+            with mcp_mod._lock:
+                mcp_mod._servers.clear()
+                mcp_mod._server_connecting.clear()
+            mcp_mod._stop_mcp_loop()
+
+    def test_drain_is_bounded_when_task_ignores_cancellation(self, caplog):
+        """A cancellation-resistant task must not hang final MCP shutdown."""
+        import tools.mcp_tool as mcp_mod
+
+        async def _run():
+            release = asyncio.Event()
+
+            async def cancellation_resistant():
+                try:
+                    await asyncio.Event().wait()
+                except asyncio.CancelledError:
+                    await release.wait()
+
+            task = asyncio.create_task(cancellation_resistant())
+            await asyncio.sleep(0)
+            try:
+                with caplog.at_level("WARNING", logger=mcp_mod.logger.name):
+                    async with asyncio.timeout(0.5):
+                        await mcp_mod._drain_mcp_loop_tasks(timeout=0.01)
+                assert not task.done(), "drain waited indefinitely for resistant task"
+            finally:
+                release.set()
+                if not task.done() and task.cancelling() == 0:
+                    task.cancel()
+                await task
+
+        asyncio.run(_run())
+
+        assert any(
+            "still pending after" in record.getMessage()
+            for record in caplog.records
+        )
+
+    def test_outer_timeout_still_allows_loop_owned_drain_before_stop(
+        self, caplog, monkeypatch
+    ):
+        """A blocked loop must drain after it resumes, not stop ahead of the drain."""
+        import threading
+        import tools.mcp_tool as mcp_mod
+
+        parked_started = threading.Event()
+        cleanup_ran = threading.Event()
+        blocker_started = threading.Event()
+        release_blocker = threading.Event()
+
+        async def parked_task():
+            parked_started.set()
+            try:
+                await asyncio.Event().wait()
+            finally:
+                cleanup_ran.set()
+
+        def block_loop():
+            blocker_started.set()
+            release_blocker.wait(timeout=5)
+
+        with mcp_mod._lock:
+            mcp_mod._servers.clear()
+            mcp_mod._server_connecting.clear()
+        mcp_mod._ensure_mcp_loop()
+        with mcp_mod._lock:
+            loop = mcp_mod._mcp_loop
+        assert loop is not None
+
+        future = asyncio.run_coroutine_threadsafe(parked_task(), loop)
+        assert parked_started.wait(timeout=2)
+        loop.call_soon_threadsafe(block_loop)
+        assert blocker_started.wait(timeout=2)
+
+        monkeypatch.setattr(mcp_mod, "_MCP_LOOP_DRAIN_TIMEOUT", 0.01)
+        release_timer = threading.Timer(1.2, release_blocker.set)
+        release_timer.start()
+        try:
+            with caplog.at_level("WARNING", logger=mcp_mod.logger.name):
+                mcp_mod._stop_mcp_loop()
+
+            assert cleanup_ran.is_set(), "drain was overtaken by loop.stop"
+            assert future.done(), "parked task remained pending after loop resumed"
+            assert loop.is_closed()
+        finally:
+            release_timer.cancel()
+            release_blocker.set()
+            future.cancel()
+            with mcp_mod._lock:
+                mcp_mod._servers.clear()
+                mcp_mod._server_connecting.clear()
+            mcp_mod._stop_mcp_loop()
+
+        assert any(
+            "Timed out waiting for MCP loop drain" in record.getMessage()
+            for record in caplog.records
+        )

tests/tools/test_mcp_tool.py

@@ -888,6 +888,86 @@ class TestGracefulFallback:
 
 class TestShutdown:
 
+    def test_shutdown_drains_parked_server_after_bounded_wait_expires(self):
+        """The public shutdown path drains a parked server if graceful shutdown stalls.
+
+        This exercises the production ownership path: a real ``MCPServerTask``
+        is registered, its parked waiter owns child tasks on the shared loop,
+        and the bounded wait for the scheduled shutdown expires. The loop owner
+        must still cancel and drain that waiter before closing the loop.
+        """
+        import tools.mcp_tool as mcp_mod
+        from tools.mcp_tool import MCPServerTask, shutdown_mcp_servers
+
+        shutdown_started = threading.Event()
+        parked_task_done = threading.Event()
+        scheduled_shutdown = {}
+        schedule_count = 0
+
+        class StalledShutdownServer(MCPServerTask):
+            async def shutdown(self):
+                shutdown_started.set()
+                await asyncio.Event().wait()
+
+        server = StalledShutdownServer("parked")
+        with mcp_mod._lock:
+            mcp_mod._servers.clear()
+            mcp_mod._server_connecting.clear()
+        mcp_mod._ensure_mcp_loop()
+        with mcp_mod._lock:
+            loop = mcp_mod._mcp_loop
+        assert loop is not None
+
+        async def install_parked_waiter():
+            task = asyncio.create_task(server._wait_for_reconnect_or_shutdown())
+            server._task = task
+            task.add_done_callback(lambda _task: parked_task_done.set())
+            await asyncio.sleep(0)
+            return task
+
+        parked_task = asyncio.run_coroutine_threadsafe(
+            install_parked_waiter(), loop
+        ).result(timeout=2)
+        with mcp_mod._lock:
+            mcp_mod._servers[server.name] = server
+
+        def schedule_then_report_timeout(coro, target_loop, **_kwargs):
+            nonlocal schedule_count
+            schedule_count += 1
+            future = asyncio.run_coroutine_threadsafe(coro, target_loop)
+            if schedule_count > 1:
+                return future
+
+            scheduled_shutdown["future"] = future
+
+            class TimedOutFuture:
+                def result(self, timeout):
+                    assert timeout > 0
+                    assert shutdown_started.wait(timeout=2)
+                    raise TimeoutError("simulated bounded MCP shutdown timeout")
+
+            return TimedOutFuture()
+
+        try:
+            with patch(
+                "agent.async_utils.safe_schedule_threadsafe",
+                side_effect=schedule_then_report_timeout,
+            ):
+                shutdown_mcp_servers()
+
+            assert loop.is_closed()
+            assert parked_task_done.is_set(), (
+                "parked MCPServerTask was not drained before its loop closed"
+            )
+            assert parked_task.done()
+            assert scheduled_shutdown["future"].done()
+            assert schedule_count == 2
+        finally:
+            with mcp_mod._lock:
+                mcp_mod._servers.clear()
+                mcp_mod._server_connecting.clear()
+            mcp_mod._stop_mcp_loop()
+
     def test_shutdown_deregisters_registered_tools(self):
         """shutdown_mcp_servers removes MCP tools and their raw alias."""
         import tools.mcp_tool as mcp_mod