KEMBAR78
Fix(mcp): drain pending command futures on McpSessionActor failure by withsmilo · Pull Request #7045 · microsoft/autogen · GitHub
Skip to content

Conversation

@withsmilo
Copy link
Contributor

@withsmilo withsmilo commented Sep 22, 2025

Why are these changes needed?

  • 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.

Related issue number

None

Checks

@codecov
Copy link

codecov bot commented Sep 22, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.05%. Comparing base (f76f92d) to head (6e8385b).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7045      +/-   ##
==========================================
+ Coverage   80.99%   81.05%   +0.05%     
==========================================
  Files         238      238              
  Lines       18291    18302      +11     
==========================================
+ Hits        14815    14834      +19     
+ Misses       3476     3468       -8     
Flag Coverage Δ
unittests 81.05% <100.00%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

- 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.
@withsmilo
Copy link
Contributor Author

Hi @ekzhu . Could you please review this PR when you have time?

@ekzhu ekzhu merged commit 29931b3 into microsoft:main Sep 30, 2025
74 checks passed
@ekzhu ekzhu requested a review from Copilot September 30, 2025 04:31
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR improves error handling in the MCP (Model Context Protocol) session actor by ensuring pending command futures are properly failed when the actor crashes during startup. The change prevents clients from hanging indefinitely when connection failures occur.

Key changes:

  • Added queue draining logic to fail pending command futures when McpSessionActor._run_actor encounters exceptions
  • Implemented best-effort error handling that swallows internal draining errors to maintain robustness
  • Added comprehensive test coverage for the new failure scenarios

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
python/packages/autogen-ext/src/autogen_ext/tools/mcp/_actor.py Added queue draining logic in the outer exception handler to fail pending command futures
python/packages/autogen-ext/tests/tools/test_mcp_actor.py Added two test cases covering queue draining behavior and error swallowing during failure scenarios


@pytest.mark.asyncio
async def test_run_actor_draining_swallows_internal_errors() -> None:
"""draining errors during exception handling are swallowed.
Copy link

Copilot AI Sep 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docstring should start with a capital letter: 'Draining errors during exception handling are swallowed.'

Suggested change
"""draining errors during exception handling are swallowed.
"""Draining errors during exception handling are swallowed.

Copilot uses AI. Check for mistakes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants