analysis: root cause analysis (af-872bf775)#5765
Closed
stempeck wants to merge 3 commits into
Closed
Conversation
added 2 commits
May 12, 2026 03:44
moonbox3
added
the
documentation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a root cause analysis write-up (and supporting concern investigations) for issue af-872bf775 / #5741, focusing on how synchronous FunctionTool execution can block the asyncio event loop and freeze Responses API polling.
Changes:
- Adds a synthesized RCA document describing the primary root cause, contributing factors, and a proposed fix + verification steps.
- Adds 8 supporting “concern investigation” documents capturing evidence and reasoning across tool invocation, MCP, hosting, and tests.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| .agentfactory/agents/rootcause-all/todos/rootcause_analysis.md | Synthesized RCA with proposed fix and verification steps. |
| .agentfactory/agents/rootcause-all/todos/rootcause_concern_1.md | Investigation of FunctionTool.__call__ executing sync functions inline. |
| .agentfactory/agents/rootcause-all/todos/rootcause_concern_2.md | Investigation of non-observability FunctionTool.invoke() blocking behavior. |
| .agentfactory/agents/rootcause-all/todos/rootcause_concern_3.md | Investigation of observability FunctionTool.invoke() blocking behavior. |
| .agentfactory/agents/rootcause-all/todos/rootcause_concern_4.md | Investigation of missing sync-to-async offloading mechanism in FunctionTool. |
| .agentfactory/agents/rootcause-all/todos/rootcause_concern_5.md | Investigation of Responses hosting loop sharing the same event loop as agent execution. |
| .agentfactory/agents/rootcause-all/todos/rootcause_concern_6.md | Investigation of MCP path interaction with tool invocation and event-loop blocking. |
| .agentfactory/agents/rootcause-all/todos/rootcause_concern_7.md | Investigation of missing test coverage for sync-tool blocking scenario. |
| .agentfactory/agents/rootcause-all/todos/rootcause_concern_8.md | Investigation comparing FunctionExecutor’s asyncio.to_thread pattern vs FunctionTool. |
Comment on lines
+16
to
+23
| | 1 | FunctionTool.__call__ executes sync functions directly on event loop thread | **VALIDATED** | [todos/rootcause_concern_1.md] | | ||
| | 2 | FunctionTool.invoke() non-observability code path blocks at line 682 | **VALIDATED** | [todos/rootcause_concern_2.md] | | ||
| | 3 | FunctionTool.invoke() observability code path blocks at line 733 | **VALIDATED** | [todos/rootcause_concern_3.md] | | ||
| | 4 | No sync-to-async offloading mechanism exists in FunctionTool (unlike FunctionExecutor) | **VALIDATED** | [todos/rootcause_concern_4.md] | | ||
| | 5 | ResponsesHostServer._handle_inner_agent runs agent.run() on same event loop as polling | **VALIDATED** | [todos/rootcause_concern_5.md] | | ||
| | 6 | MCP tool invocation path (_agents.py:1543) also calls invoke() in same async context | **VALIDATED** | [todos/rootcause_concern_6.md] | | ||
| | 7 | No test coverage for sync-blocking-event-loop scenario in test_tools.py | **VALIDATED** | [todos/rootcause_concern_7.md] | | ||
| | 8 | Existing asyncio.to_thread() pattern in _function_executor.py not applied to FunctionTool | **VALIDATED** | [todos/rootcause_concern_8.md] | |
Comment on lines
+35
to
+43
| **Evidence**: `_tools.py:511-535` (`__call__` method): | ||
| ```python | ||
| def __call__(self, *args: **kwargs): | ||
| ... | ||
| func = self.func | ||
| if self._instance is not None: | ||
| return func(self._instance, *args, **kwargs) | ||
| return func(*args, **kwargs) | ||
| ``` |
Comment on lines
+1
to
+11
| # Concern #6 Investigation: MCP tool invocation path blocks | ||
|
|
||
| **Investigated by**: Sub-agent | ||
| **Date**: 2026-05-12 | ||
|
|
||
| ## Verdict: VALIDATED | ||
|
|
||
| ## Summary | ||
|
|
||
| The MCP tool invocation path at `_agents.py:1543` calls `agent_tool.invoke()` in the same async context with no thread offloading, sharing the exact same blocking-vulnerable code path as the normal (non-MCP) tool invocation. When a synchronous tool function is invoked via the MCP server, the event loop is blocked during execution because `FunctionTool.invoke()` calls `self.__call__()` directly and only checks `inspect.isawaitable()` after the call returns -- meaning synchronous functions execute inline on the event loop thread. | ||
|
|
Comment on lines
+168
to
+190
| In `test_tools.py`, add a test that: | ||
| 1. Creates a sync `@tool` that sleeps briefly (e.g., 0.1s using `time.sleep`) | ||
| 2. Runs the tool via `invoke()` concurrently with an async flag-setting task using `asyncio.gather` | ||
| 3. Verifies the async task completed (was not blocked) — i.e., the flag was set before or during the sync sleep, proving the event loop was not blocked | ||
|
|
||
| ```python | ||
| async def test_sync_tool_does_not_block_event_loop(): | ||
| flag = asyncio.Event() | ||
|
|
||
| @tool | ||
| def blocking_tool() -> str: | ||
| """A tool that blocks.""" | ||
| time.sleep(0.1) | ||
| return "done" | ||
|
|
||
| async def set_flag(): | ||
| flag.set() | ||
|
|
||
| result_task = asyncio.create_task(blocking_tool.invoke()) | ||
| flag_task = asyncio.create_task(set_flag()) | ||
| await asyncio.gather(result_task, flag_task) | ||
| assert flag.is_set() | ||
| ``` |
| | 5 | Other modules use `asyncio.to_thread` for I/O | `_sessions.py:1008,1036`, `_skills.py:248,252`, `_harness/_todo.py:339,369` | Multiple modules in the same codebase correctly use `asyncio.to_thread()` for blocking operations | | ||
| | 6 | Auto-invocation uses `asyncio.gather` | `_tools.py:1769-1771` | All tool calls in a single iteration are gathered concurrently -- a single blocking sync tool stalls them all | | ||
| | 7 | `FunctionTool` constructor stores `func` with no wrapping | `_tools.py:297-339` | No `iscoroutinefunction` check, no thread wrapping at construction time | | ||
| | 8 | `FunctionExecutor` test validates thread execution | `tests/workflow/test_function_executor.py:480-504` | Tests confirm sync functions run in separate thread via `asyncio.to_thread` in FunctionExecutor | |
Complete design analysis of the source generator implementation for replacing ReflectingExecutor<T> with [MessageHandler] attributes. Key finding: the implementation already exists and is functionally complete with deliberate deviations from the original plan (YieldsOutput naming, ConfigureProtocol API, MAFGENWF diagnostic IDs). Identifies 13 quality gaps with implementation plan. Artifacts: design-doc.md, source.md, codebase-snapshot.md, verification.md, synthesis-checklist.md, 6 dimension analyses, audit.md, conflicts.md, dependencies.md, elevation_assessment.md, six_sigma_gaps.md, verification-report.md. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
Please keep this on-track for specific agent-framework fixes or features, @stempeck. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root cause analysis for af-872bf775.
See todos/rootcause_analysis.md for synthesized findings and solution.