Skip to content

Commit 6e8385b

Browse files
committed
Fix(mcp): drain pending command futures on actor failure
- In `McpSessionActor._run_actor`, when an outer exception occurs (e.g., session startup failure), drain `_command_queue` and set exceptions on any pending command futures. This prevents callers from hanging indefinitely when the actor fails before processing queued commands. Draining is best‑effort and guarded. This hardens failure handling and makes client behavior predictable under connection errors.
1 parent f76f92d commit 6e8385b

2 files changed

Lines changed: 98 additions & 1 deletion

File tree

python/packages/autogen-ext/src/autogen_ext/tools/mcp/_actor.py

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -262,6 +262,19 @@ async def _run_actor(self) -> None:
262262
except Exception as e:
263263
cmd["future"].set_exception(e)
264264
except Exception as e:
265+
try:
266+
while True:
267+
try:
268+
pending_cmd = self._command_queue.get_nowait()
269+
except asyncio.QueueEmpty:
270+
break
271+
fut = pending_cmd.get("future")
272+
if fut is not None and not fut.done():
273+
fut.set_exception(e)
274+
except Exception:
275+
# Best-effort draining only
276+
pass
277+
265278
if self._shutdown_future and not self._shutdown_future.done():
266279
self._shutdown_future.set_exception(e)
267280
else:

python/packages/autogen-ext/tests/tools/test_mcp_actor.py

Lines changed: 85 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@
1616
RequestUsage,
1717
UserMessage,
1818
)
19-
from autogen_ext.tools.mcp import StdioServerParams
19+
from autogen_ext.tools.mcp import StdioServerParams, StreamableHttpServerParams
2020
from autogen_ext.tools.mcp._actor import (
2121
McpSessionActor,
2222
_parse_sampling_content, # pyright: ignore[reportPrivateUsage]
@@ -553,6 +553,90 @@ async def test_run_actor_session_exception() -> None:
553553
assert actor._actor_task is None # type: ignore[reportPrivateUsage]
554554

555555

556+
@pytest.mark.asyncio
557+
async def test_run_actor_drains_queue_on_session_exception() -> None:
558+
"""Ensure pending command futures are failed when session creation raises.
559+
560+
Uses StreamableHttpServerParams with an invalid URL to trigger failure,
561+
covering the queue-draining logic added in the referenced commit.
562+
"""
563+
# Use an invalid local URL/port to force immediate connection failure
564+
server_params = StreamableHttpServerParams(
565+
url="http://127.0.0.1:1/invalid", # very likely closed port
566+
timeout=0.1,
567+
sse_read_timeout=0.1,
568+
)
569+
actor = McpSessionActor(server_params)
570+
571+
# Prepare pending commands before the actor starts, so the outer except drains them
572+
fut1: asyncio.Future[Any] = asyncio.Future()
573+
fut2: asyncio.Future[Any] = asyncio.Future()
574+
await actor._command_queue.put({"type": "list_tools", "future": fut1}) # type: ignore[reportPrivateUsage]
575+
await actor._command_queue.put({"type": "call_tool", "name": "t", "args": {}, "future": fut2}) # type: ignore[reportPrivateUsage]
576+
577+
actor._active = True # type: ignore[reportPrivateUsage]
578+
task = asyncio.create_task(actor._run_actor()) # type: ignore[reportPrivateUsage]
579+
580+
# Wait for task to complete; it should handle the exception and drain the queue
581+
try:
582+
await asyncio.wait_for(task, timeout=2.0)
583+
except asyncio.TimeoutError:
584+
# If something goes wrong, ensure task cleanup for test stability
585+
task.cancel()
586+
with pytest.raises(asyncio.CancelledError):
587+
await task
588+
589+
# Verify futures were failed by the draining logic
590+
assert fut1.done()
591+
assert fut1.exception() is not None
592+
593+
assert fut2.done()
594+
assert fut2.exception() is not None
595+
596+
597+
@pytest.mark.asyncio
598+
async def test_run_actor_draining_swallows_internal_errors() -> None:
599+
"""draining errors during exception handling are swallowed.
600+
601+
We force `create_mcp_server_session` to raise so `_run_actor` enters the outer
602+
exception handler, then make `get_nowait()` itself raise a non-QueueEmpty
603+
exception. The inner `except Exception: pass` (best-effort draining) should
604+
swallow it and continue to set the shutdown future exception instead of
605+
crashing the task.
606+
"""
607+
actor = McpSessionActor(StdioServerParams(command="echo", args=["test"]))
608+
609+
# Replace the command queue with a mock that raises from get_nowait()
610+
mock_q = MagicMock()
611+
mock_q.get_nowait.side_effect = RuntimeError("drain failure")
612+
actor._command_queue = mock_q # type: ignore[reportPrivateUsage]
613+
614+
# Prepare a shutdown future to observe behavior after draining attempt
615+
actor._shutdown_future = asyncio.Future() # type: ignore[reportPrivateUsage]
616+
617+
with patch(
618+
"autogen_ext.tools.mcp._actor.create_mcp_server_session",
619+
side_effect=Exception("Session error"),
620+
):
621+
actor._active = True # type: ignore[reportPrivateUsage]
622+
task = asyncio.create_task(actor._run_actor()) # type: ignore[reportPrivateUsage]
623+
624+
# The task should finish and set the shutdown future with the session error
625+
try:
626+
await asyncio.wait_for(task, timeout=1.0)
627+
except asyncio.TimeoutError:
628+
task.cancel()
629+
with pytest.raises(asyncio.CancelledError):
630+
await task
631+
632+
# Draining raised internally, but should have been swallowed (lines 274-276)
633+
mock_q.get_nowait.assert_called() # type: ignore[reportPrivateUsage]
634+
assert actor._shutdown_future.done() # type: ignore[reportPrivateUsage]
635+
exc = actor._shutdown_future.exception() # type: ignore[reportPrivateUsage]
636+
assert isinstance(exc, Exception)
637+
assert "Session error" in str(exc)
638+
639+
556640
@pytest.mark.asyncio
557641
async def test_run_actor_shutdown_future_exception() -> None:
558642
"""Test _run_actor sets exception on shutdown future when session fails."""

0 commit comments

Comments
 (0)