fix mcp never failing till timeout bug#661
Conversation
Signed-off-by: David Hadas <david.hadas@gmail.com>
pdettori
left a comment
pdettori
left a comment
There was a problem hiding this comment.
Solid, well-documented fix for #660. Correctly diagnoses that mcpadapt swallows the connect exception in its daemon thread (so ready never sets → full MCP_TIMEOUT hang that also freezes the event loop), and the poll-the-thread fast-fail is the right seam. The output-bounding (tool_limits) and cooperative cancellation via a per-task threading.Event + CrewAI step_callback are thoughtful, and the guarded fallback to blocking behavior when library internals shift is a good defensive posture. CI is fully green.
All findings below are non-blocking (suggestions/nits) — approving.
Assisted-By: Claude Code
|
|
||
| if task_id: | ||
| task_updater = TaskUpdater(event_queue, task_id, task.context_id) | ||
| await task_updater.cancel() |
There was a problem hiding this comment.
suggestion: await task_updater.cancel() here is unguarded, unlike the equivalent call in execute()'s cancellation handler (~line 230) which is wrapped best-effort in try/except.
On cancel, task_updater.cancel() can be invoked twice — once here, then again in execute()'s except (asyncio.CancelledError, TaskCancelled) path once the crew observes the flag and unwinds — and/or against an already torn-down event queue. The execute() path defends against that; cancel() does not, so an exception here would propagate back to the A2A framework. Consider wrapping this best-effort for symmetry.
| return None | ||
|
|
||
|
|
||
| def _cap_result( |
There was a problem hiding this comment.
suggestion: No unit tests accompany the two new pure-logic modules — _cap_result/wrap_tool_output here and _connect_with_fast_fail in mcp_connect.py. Both have clean, deterministic contracts that are cheap to pin down: for _cap_result, the dict-with-items slice, the bare-list slice, the non-JSON passthrough, and the char-cap marker; for the connect helper, the three ready/dead/timeout states. test_startup.exp only exercises process startup, so these branches are currently uncovered. A small pytest over _cap_result would be high-value / low-cost.
| if list_key is not None: | ||
| items = parsed[list_key] | ||
| total = _read_total(parsed, default=len(items)) | ||
| if len(items) > max_items: |
There was a problem hiding this comment.
nit: The programmatic truncation note only fires when the server returns more than max_items. When the injected per_page=max_items successfully caps the fetch at exactly max_items while the true total_count is larger, len(items) > max_items is False, so no note is recorded — and "results were capped" awareness then rests entirely on the prompt guidance + the model honoring total_count. This looks intended (the prompt is the primary channel), just flagging to confirm the design intent.
| Version note | ||
| ------------ | ||
| The fast-fail path pokes at ``mcpadapt`` internals (``MCPAdapt.ready`` / | ||
| ``MCPAdapt.thread`` / ``MCPAdapt.tools()``), verified against ``crewai-tools`` |
There was a problem hiding this comment.
nit: Good call documenting the reliance on private mcpadapt internals and guarding it with getattr + a blocking fallback. One heads-up: pyproject.toml pins crewai-tools[mcp]>=1.6.1 (unbounded upper; only uv.lock fixes the deployed version), and the fallback path silently reverts to the exact slow-timeout behavior this PR fixes — logging only a warning. So a future crewai-tools bump landing via a lock refresh could quietly disable the fast-fail. Might be worth a bounded upper pin or a startup canary/log that surfaces when the fast-fail path is inactive.
3 participants
Force errors to propagate and do not wait for timeout to continue processing the task.
Fixes #660