(do not merge) fix: implement proper async signal handling for graceful server shutdown #73
Conversation
- Add asyncio signal handlers for SIGTERM and SIGINT - Use anyio TaskGroup for proper cancellation handling - Fix 'cancel scope in different task' error during shutdown - Maintain compatibility with Windows platform Fixes async/threading failures when MCP server is interrupted. Co-Authored-By: AJ Steers <[email protected]>
Original prompt from AJ Steers |
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. Testing This Branch via MCPTo test the changes in this specific branch with an MCP client like Claude Desktop, use the following configuration: {
"mcpServers": {
"connector-builder-mcp-dev": {
"command": "uvx",
"args": ["--from", "git+https://github.com/airbytehq/connector-builder-mcp.git@devin/1757567587-fix-async-cleanup-issues", "connector-builder-mcp"]
}
}
}Testing This Branch via CLIYou can test this version of the MCP Server using the following CLI snippet: # Run the CLI from this branch:
uvx 'git+https://github.com/airbytehq/connector-builder-mcp.git@devin/1757567587-fix-async-cleanup-issues#egg=airbyte-connector-builder-mcp' --helpPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
- Fix whitespace on blank lines (ruff lint) - Add anyio as direct dependency (deptry requirement) - Update uv.lock with new dependency Co-Authored-By: AJ Steers <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR fixes async signal handling issues during server shutdown that caused "RuntimeError: Attempted to exit cancel scope in a different task than it was entered in" errors. The fix replaces simple exception handling with proper async signal management using task groups and coordinated shutdown events.
- Implements proper async signal handling with
asyncio.Eventfor shutdown coordination - Uses
anyio.create_task_group()for proper cancellation propagation and cleanup - Adds platform-specific signal handling (Unix signals vs Windows KeyboardInterrupt)
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pyproject.toml | Adds anyio dependency for proper async task group management |
| connector_builder_mcp/server.py | Replaces simple asyncio.run() with comprehensive async signal handling and graceful shutdown |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| try: | ||
| async with anyio.create_task_group() as tg: | ||
| tg.start_soon(app.run_stdio_async) |
There was a problem hiding this comment.
There's a potential race condition where shutdown_event.wait() could complete before app.run_stdio_async() is fully started by the task group. Consider using tg.start_task() instead of tg.start_soon() to ensure the task is properly started before waiting for shutdown.
| tg.start_soon(app.run_stdio_async) | |
| tg.start_task(app.run_stdio_async) |
Copilot uses AI. Check for mistakes.
| if sys.platform != "win32": | ||
| loop = asyncio.get_running_loop() | ||
| for sig in (signal.SIGTERM, signal.SIGINT): | ||
| loop.add_signal_handler(sig, signal_handler) |
There was a problem hiding this comment.
Signal handlers are registered but never cleaned up. If the function is called multiple times or the event loop is reused, this could lead to duplicate signal handlers. Consider adding signal handler cleanup in a finally block or using a context manager approach.
Copilot uses AI. Check for mistakes.
Technical Analysis: Rejecting Copilot Suggestion #1Suggestion: Replace Decision: REJECT ❌ Technical Reasoning:
The existing code uses the correct anyio API and should remain unchanged. |
Technical Analysis: Rejecting Copilot Suggestion #2Suggestion: Add signal handler cleanup to prevent duplicate handlers Decision: REJECT ❌ Technical Reasoning:
The existing signal handling code is correct and should remain unchanged. Summary: Both copilot suggestions are based on incorrect assumptions. The current implementation:
|
aaronsteers
changed the title
1 participant
fix: implement proper async signal handling for graceful server shutdown
Summary
Fixes async/threading failures that occurred during MCP server shutdown, specifically the "RuntimeError: Attempted to exit cancel scope in a different task than it was entered in" error when the server is interrupted.
The original implementation used a simple
asyncio.run(app.run_stdio_async())call wrapped in basic exception handling. When interrupted (e.g., Ctrl+C), the FastMCP stdio client's async context managers weren't cleaned up properly, causing cancel scope task mismatches.The fix implements proper async signal handling using:
asyncio.Eventfor coordinating shutdown signalsanyio.create_task_group()for proper cancellation propagationReview & Testing Checklist for Human
Test Plan
timeout 10s uv run python bin/test_mcp_tool.py validate_manifest '{"manifest": "@simple_api_manifest"}' & sleep 2 && kill -INT $!- should shutdown cleanly without RuntimeErroruv run python bin/test_mcp_tool.py validate_manifest '{"manifest": "@simple_api_manifest"}'uv run pytest tests/ -v- all tests should continue passingNotes
This change involves complex async patterns and signal handling, which are inherently error-prone areas. While local testing shows the fix works and doesn't break existing functionality, the async nature makes comprehensive testing challenging. Pay special attention to shutdown behavior and any platform-specific differences.
Link to Devin run: https://app.devin.ai/sessions/176dd0ed33e54610805c2dbaec715642
Requested by: @aaronsteers