fix: offload long-running MCP tools to a thread (#46, #136) (#231)

tirth8205 · claude · web-flow · commit 7093e56202f0 · 2026-04-11T22:04:09.000+01:00
Limucc's v2.2.4 test on Windows 11 / Python 3.14 confirmed that the WindowsSelectorEventLoopPolicy fix from v2.2.4 was *necessary but not sufficient* — read-only tools work, but long-running tools still hang indefinitely over stdio MCP: build_or_update_graph_tool(full_rebuild=True) → hangs embed_graph_tool (sentence-transformers) → hangs Root cause: FastMCP 2.x dispatches sync handlers inline on the only event-loop thread. When a handler runs for more than a few seconds — especially one that spawns child processes (full_build uses ProcessPoolExecutor) or does CPU-bound inference (sentence-transformers) — the loop stops pumping stdin/stdout, Claude Code's request never gets a response, and the MCP client shows "Synthesizing…" forever. Fix: make the five heavy tool handlers ``async def`` and offload the blocking work with ``asyncio.to_thread``. The event loop stays responsive and the stdio transport keeps pumping. This is a no-op for short tools, zero-config, and works on every platform. Tools converted to async + asyncio.to_thread: - build_or_update_graph_tool (full_build / incremental_update) - run_postprocess_tool (community detection can be slow) - embed_graph_tool (sentence-transformers inference) - detect_changes_tool (git diff + BFS traversal) - generate_wiki_tool (many SQLite reads + file writes) The other 19 tools are fast SQLite-read paths and stay sync. Tests: tests/test_main.py::TestLongRunningToolsAreAsync - Asserts all 5 tools are registered as coroutines via FastMCP's get_tools() introspection - Defense-in-depth: asserts each tool's source literally contains "asyncio.to_thread" so we don't accidentally make a tool async without offloading the blocking work Verified locally on Python 3.11 / macOS: - ruff clean, mypy clean, bandit clean - 737 tests pass (+2 new async lock-in tests), coverage 74.63% - mcp.get_tools() returns 24 tools with the 5 above as coroutines Windows verification requested from @dev-limucc on #136 once v2.3.1 ships. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
diff --git a/code_review_graph/main.py b/code_review_graph/main.py
@@ -6,6 +6,7 @@
 
 from __future__ import annotations
 
+import asyncio
 import sys
 from typing import Optional
 
@@ -77,7 +78,7 @@ def _resolve_repo_root(repo_root: Optional[str]) -> Optional[str]:
 
 
 @mcp.tool()
-def build_or_update_graph_tool(
+async def build_or_update_graph_tool(
     full_rebuild: bool = False,
     repo_root: Optional[str] = None,
     base: str = "HEAD~1",
@@ -90,6 +91,13 @@ def build_or_update_graph_tool(
     By default performs an incremental update (only changed files).
     Set full_rebuild=True to re-parse every file.
 
+    Runs the blocking full_build / incremental_update work in a thread
+    via ``asyncio.to_thread`` so the stdio event loop stays responsive.
+    Without this wrapper, long builds deadlocked on Windows because
+    ``ProcessPoolExecutor`` (used by parallel parsing) interacted badly
+    with the sync handler blocking the only event-loop thread. See:
+    #46, #136.
+
     Args:
         full_rebuild: If True, re-parse all files. Default: False (incremental).
         repo_root: Repository root path. Auto-detected from current directory if omitted.
@@ -99,14 +107,18 @@ def build_or_update_graph_tool(
         recurse_submodules: If True, include files from git submodules.
             When None (default), falls back to CRG_RECURSE_SUBMODULES env var.
     """
-    return build_or_update_graph(
-        full_rebuild=full_rebuild, repo_root=_resolve_repo_root(repo_root), base=base,
-        postprocess=postprocess, recurse_submodules=recurse_submodules,
+    return await asyncio.to_thread(
+        build_or_update_graph,
+        full_rebuild=full_rebuild,
+        repo_root=_resolve_repo_root(repo_root),
+        base=base,
+        postprocess=postprocess,
+        recurse_submodules=recurse_submodules,
     )
 
 
 @mcp.tool()
-def run_postprocess_tool(
+async def run_postprocess_tool(
     flows: bool = True,
     communities: bool = True,
     fts: bool = True,
@@ -117,14 +129,20 @@ def run_postprocess_tool(
     Use after building with postprocess="none" or "minimal", or to re-run
     expensive steps independently. Signatures are always computed.
 
+    Offloaded to a thread via ``asyncio.to_thread`` so community
+    detection on large graphs doesn't block the MCP event loop. See:
+    #46, #136.
+
     Args:
         flows: Run flow detection. Default: True.
         communities: Run community detection. Default: True.
         fts: Rebuild FTS index. Default: True.
         repo_root: Repository root path. Auto-detected if omitted.
     """
-    return run_postprocess(
-        flows=flows, communities=communities, fts=fts, repo_root=_resolve_repo_root(repo_root),
+    return await asyncio.to_thread(
+        run_postprocess,
+        flows=flows, communities=communities, fts=fts,
+        repo_root=_resolve_repo_root(repo_root),
     )
 
 
@@ -273,7 +291,7 @@ def semantic_search_nodes_tool(
 
 
 @mcp.tool()
-def embed_graph_tool(
+async def embed_graph_tool(
     repo_root: Optional[str] = None,
     model: Optional[str] = None,
 ) -> dict:
@@ -287,12 +305,21 @@ def embed_graph_tool(
     After running this, semantic_search_nodes_tool will use vector similarity
     instead of keyword matching for much better results.
 
+    Runs the blocking sentence-transformers / Gemini inference in a
+    thread via ``asyncio.to_thread`` so the stdio event loop stays
+    responsive — without this wrapper, embedding a large graph would
+    silently hang the MCP server on Windows. See: #46, #136.
+
     Args:
         repo_root: Repository root path. Auto-detected if omitted.
         model: Embedding model name (HuggingFace ID or local path).
                Falls back to CRG_EMBEDDING_MODEL env var, then all-MiniLM-L6-v2.
     """
-    return embed_graph(repo_root=_resolve_repo_root(repo_root), model=model)
+    return await asyncio.to_thread(
+        embed_graph,
+        repo_root=_resolve_repo_root(repo_root),
+        model=model,
+    )
 
 
 @mcp.tool()
@@ -501,7 +528,7 @@ def get_architecture_overview_tool(
 
 
 @mcp.tool()
-def detect_changes_tool(
+async def detect_changes_tool(
     base: str = "HEAD~1",
     changed_files: Optional[list[str]] = None,
     include_source: bool = False,
@@ -515,6 +542,10 @@ def detect_changes_tool(
     flows, communities, and test coverage gaps. Returns risk scores and
     prioritized review items. Replaces get_review_context for change-aware reviews.
 
+    Offloaded to a thread via ``asyncio.to_thread`` — runs `git diff`
+    subprocesses and BFS traversals that can take several seconds on
+    large repos. See: #46, #136.
+
     Args:
         base: Git ref to diff against. Default: HEAD~1.
         changed_files: List of changed file paths (relative to repo root). Auto-detected if omitted.
@@ -524,7 +555,8 @@ def detect_changes_tool(
         detail_level: "standard" for full output, "minimal" for
             token-efficient summary. Default: standard.
     """
-    return detect_changes_func(
+    return await asyncio.to_thread(
+        detect_changes_func,
         base=base, changed_files=changed_files,
         include_source=include_source, max_depth=max_depth,
         repo_root=_resolve_repo_root(repo_root), detail_level=detail_level,
@@ -598,7 +630,7 @@ def apply_refactor_tool(
 
 
 @mcp.tool()
-def generate_wiki_tool(
+async def generate_wiki_tool(
     repo_root: Optional[str] = None,
     force: bool = False,
 ) -> dict:
@@ -608,11 +640,19 @@ def generate_wiki_tool(
     Pages are written to .code-review-graph/wiki/ inside the repository.
     Only regenerates pages whose content has changed unless force=True.
 
+    Offloaded to a thread via ``asyncio.to_thread`` — on large graphs
+    the page-generation loop touches every community and issues many
+    SQLite reads, which would block the MCP event loop. See: #46, #136.
+
     Args:
         repo_root: Repository root path. Auto-detected if omitted.
         force: If True, regenerate all pages even if content unchanged. Default: False.
     """
-    return generate_wiki_func(repo_root=_resolve_repo_root(repo_root), force=force)
+    return await asyncio.to_thread(
+        generate_wiki_func,
+        repo_root=_resolve_repo_root(repo_root),
+        force=force,
+    )
 
 
 @mcp.tool()
diff --git a/tests/test_main.py b/tests/test_main.py
@@ -1,13 +1,16 @@
 """Tests for the MCP server entry point.
 
 Focused on the ``_resolve_repo_root`` helper that threads the
-``serve --repo <X>`` CLI flag into every tool wrapper. Previously only
-``get_docs_section_tool`` respected the flag and the other 21 tools
-silently fell back to cwd.
+``serve --repo <X>`` CLI flag into every tool wrapper, and on the
+set of tools that must be registered as async coroutines so the MCP
+stdio event loop stays responsive during long-running operations.
 """
 
 from __future__ import annotations
 
+import asyncio
+import inspect
+
 import pytest
 
 from code_review_graph import main as crg_main
@@ -43,3 +46,65 @@ def test_client_arg_wins_over_flag(self):
     def test_client_arg_used_when_no_flag(self):
         crg_main._default_repo_root = None
         assert crg_main._resolve_repo_root("/explicit") == "/explicit"
+
+
+class TestLongRunningToolsAreAsync:
+    """Long-running MCP tools must be registered as coroutines so the
+    asyncio event loop stays responsive while the work runs in a
+    background thread via ``asyncio.to_thread``. Without this, Windows
+    MCP clients hang on ``build_or_update_graph_tool`` and
+    ``embed_graph_tool`` — see #46, #136.
+    """
+
+    HEAVY_TOOLS = {
+        "build_or_update_graph_tool",
+        "run_postprocess_tool",
+        "embed_graph_tool",
+        "detect_changes_tool",
+        "generate_wiki_tool",
+    }
+
+    @pytest.mark.asyncio
+    async def test_heavy_tools_are_coroutines(self):
+        tools = await crg_main.mcp.get_tools()
+        registered: dict[str, bool] = {}
+        for name, tool in tools.items():
+            if name not in self.HEAVY_TOOLS:
+                continue
+            # FastMCP 2.x stores the underlying Python function on the
+            # tool wrapper; attribute name has varied but is typically
+            # ``fn`` on FunctionTool. Fall back to a few candidates.
+            fn = (
+                getattr(tool, "fn", None)
+                or getattr(tool, "_func", None)
+                or getattr(tool, "func", None)
+                or tool
+            )
+            registered[name] = asyncio.iscoroutinefunction(fn)
+
+        missing = self.HEAVY_TOOLS - registered.keys()
+        assert not missing, f"heavy tool(s) not registered at all: {missing}"
+
+        not_async = [name for name, is_async in registered.items() if not is_async]
+        assert not not_async, (
+            f"these tools must be async but were registered as sync, "
+            f"which will hang the stdio event loop on Windows: {not_async}"
+        )
+
+    @pytest.mark.asyncio
+    async def test_heavy_tool_source_uses_to_thread(self):
+        """Defense in depth: the source of every heavy tool wrapper must
+        literally call asyncio.to_thread so we don't accidentally turn
+        a tool async without offloading the blocking work."""
+        for tool_name in self.HEAVY_TOOLS:
+            fn = getattr(crg_main, tool_name, None)
+            assert fn is not None, f"{tool_name} not found on module"
+            # The @mcp.tool() decorator wraps the original function; walk
+            # through the wrapper to find the underlying source.
+            underlying = getattr(fn, "fn", None) or fn
+            source = inspect.getsource(underlying)
+            assert "asyncio.to_thread" in source, (
+                f"{tool_name} must call asyncio.to_thread to offload its "
+                f"blocking work; otherwise Windows MCP clients will hang. "
+                f"See #46, #136."
+            )
