fix(async): close unscheduled coroutines in threadsafe bridges#2657
fix(async): close unscheduled coroutines in threadsafe bridges#2657JithendraNara wants to merge 3 commits intoNousResearch:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent leaked/unawaited MCP coroutines when the MCP loop scheduler fails, by deferring coroutine creation until the scheduler is known to be usable and adding regression tests for the failure paths.
Changes:
- Updated
_run_on_mcp_loopto accept a coroutine or a zero-arg coroutine factory, and to cancel scheduled futures on timeout. - Updated MCP tool/probe call sites to pass coroutine factories instead of already-created coroutine objects.
- Added regression tests asserting scheduler failure paths do not emit “coroutine was never awaited”
RuntimeWarnings.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
tools/mcp_tool.py |
Adds factory support + timeout cancellation to _run_on_mcp_loop; updates call sites to avoid creating coroutines before scheduling. |
tests/tools/test_mcp_tool.py |
Adds a regression test ensuring tool handler failure doesn’t leak unawaited coroutine warnings; updates test loop patch helper for new signature. |
tests/tools/test_mcp_probe.py |
Refactors probe test coroutine runner to match new signature; adds regression test for probe failure without unawaited coroutine warnings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| coro = coro_or_factory() if callable(coro_or_factory) else coro_or_factory | ||
| future = asyncio.run_coroutine_threadsafe(coro, loop) | ||
| return future.result(timeout=timeout) | ||
| try: | ||
| return future.result(timeout=timeout) |
There was a problem hiding this comment.
If asyncio.run_coroutine_threadsafe(coro, loop) raises (e.g., loop is closed/stopping between the is_running() check and scheduling, or a test patches the scheduler to raise), the freshly created coroutine from the factory will never be awaited/closed and can still trigger "coroutine was never awaited" warnings. Consider wrapping the scheduling call in try/except and calling coro.close() on failure (and optionally validating that the factory returned a coroutine) before re-raising.
There was a problem hiding this comment.
Addressed in 9cc3faa. _run_on_mcp_loop now wraps asyncio.run_coroutine_threadsafe(...) in try/except, closes the created coroutine if scheduling fails, and re-raises. I also added a direct regression test in tests/tools/test_mcp_tool.py that patches the scheduler to raise and asserts we do not leak an unawaited coroutine warning.
|
Expanded this beyond the original MCP-only fix after sweeping the same pattern elsewhere in the repo. The PR now also hardens ACP permission/update bridges, DingTalk incoming dispatch, gateway sync-to-async callbacks, the async worker helper, and MCP shutdown against unscheduled coroutine leaks when threadsafe scheduling fails. Added focused regressions for each area and reran the ACP + DingTalk + tools slices listed in the PR body. |
Summary
This fixes the same coroutine-leak pattern anywhere the repo bridges sync code into an async loop with asyncio.run_coroutine_threadsafe(...).
Changes
Test plan