fix(mcp): add timeout to background thread join in stop() to prevent hangs

src/strands/tools/mcp/mcp_client.py

@@ -353,9 +353,17 @@ async def _set_close_event() -> None:
                 asyncio.run_coroutine_threadsafe(coro=_set_close_event(), loop=self._background_thread_event_loop)
 
             self._log_debug_with_thread("waiting for background thread to join")
-            self._background_thread.join()
-
-        if self._background_thread_event_loop is not None:
+            self._background_thread.join(timeout=self._startup_timeout)
+            if self._background_thread.is_alive():
+                logger.warning(
+                    "background thread did not exit within %d seconds; "
+                    "skipping event loop close and state reset to avoid interfering with running thread",
+                    self._startup_timeout,
+                )
+                return
+            elif self._background_thread_event_loop is not None:
+                self._background_thread_event_loop.close()
+        elif self._background_thread_event_loop is not None:
             self._background_thread_event_loop.close()
 
         self._log_debug_with_thread("background thread is closed, MCPClient context exited")

tests/strands/tools/mcp/test_mcp_client.py

@@ -488,21 +488,41 @@ def test_stop_with_no_background_thread():
     assert client._background_thread is None
 
 
+def test_stop_closes_event_loop_when_no_background_thread():
+    """Test that stop() closes the event loop even when background thread is None.
+
+    This covers the edge case where the event loop was created but the
+    background thread was already cleaned up or never assigned.
+    """
+    client = MCPClient(MagicMock())
+
+    mock_event_loop = MagicMock()
+    client._background_thread = None
+    client._background_thread_event_loop = mock_event_loop
+
+    client.stop(None, None, None)
+
+    # Event loop should still be closed
+    mock_event_loop.close.assert_called_once()
+    assert client._background_thread_event_loop is None
+
+
 def test_stop_with_background_thread_but_no_event_loop():
     """Test that stop() handles the case when background thread exists but event loop is None."""
     client = MCPClient(MagicMock())
 
     # Mock a background thread without event loop
     mock_thread = MagicMock()
     mock_thread.join = MagicMock()
+    mock_thread.is_alive = MagicMock(return_value=False)
     client._background_thread = mock_thread
     client._background_thread_event_loop = None
 
     # Should not raise any exceptions and should join the thread
     client.stop(None, None, None)
 
-    # Verify thread was joined
-    mock_thread.join.assert_called_once()
+    # Verify thread was joined with timeout
+    mock_thread.join.assert_called_once_with(timeout=client._startup_timeout)
 
     # Verify cleanup occurred
     assert client._background_thread is None
@@ -515,6 +535,7 @@ def test_stop_closes_event_loop():
     # Mock a background thread with event loop
     mock_thread = MagicMock()
     mock_thread.join = MagicMock()
+    mock_thread.is_alive = MagicMock(return_value=False)
     mock_event_loop = MagicMock()
     mock_event_loop.close = MagicMock()
 
@@ -524,8 +545,8 @@ def test_stop_closes_event_loop():
     # Should close the event loop and join the thread
     client.stop(None, None, None)
 
-    # Verify thread was joined
-    mock_thread.join.assert_called_once()
+    # Verify thread was joined with timeout
+    mock_thread.join.assert_called_once_with(timeout=client._startup_timeout)
 
     # Verify event loop was closed
     mock_event_loop.close.assert_called_once()
@@ -535,6 +556,35 @@ def test_stop_closes_event_loop():
     assert client._background_thread_event_loop is None
 
 
+def test_stop_does_not_hang_when_join_times_out():
+    """Test that stop() returns early when the background thread doesn't exit within timeout.
+
+    When join() times out (is_alive() returns True), stop() must skip event loop
+    close AND state reset to avoid interfering with the still-running thread and
+    prevent a second background thread from being spawned via start().
+    """
+    client = MCPClient(MagicMock())
+
+    mock_thread = MagicMock()
+    # Simulate thread still alive after join (timed out)
+    mock_thread.is_alive = MagicMock(return_value=True)
+    mock_event_loop = MagicMock()
+    mock_event_loop.close = MagicMock()
+
+    client._background_thread = mock_thread
+    client._background_thread_event_loop = mock_event_loop
+
+    client.stop(None, None, None)
+
+    # join should have been called with timeout, and stop() should return promptly
+    mock_thread.join.assert_called_once_with(timeout=client._startup_timeout)
+    # When thread is still alive, stop() skips event loop close AND state reset
+    mock_event_loop.close.assert_not_called()
+    # State is NOT reset — references kept to prevent reuse while thread is still alive
+    assert client._background_thread is mock_thread
+    assert client._background_thread_event_loop is mock_event_loop
+
+
 def test_mcp_client_state_reset_after_timeout():
     """Test that all client state is properly reset after timeout."""