fix: MCP per-call timeout, wire workflow context into prompt, fix parallel loop idx - #3458
Conversation
…allel loop idx (fixes #3457) - mcp.py: wrap session.call_tool/read_resource/get_prompt in asyncio.wait_for so a hung server fails one request without wedging the runner loop forever - agents.py: fold task._execution_context (inter-task context + validation feedback built by Process._build_task_context) into the prompt builder, and clear it after consumption so it doesn't leak into later runs - workflows.py: track future->idx map in parallel loop so error attribution survives exceptions instead of using an unbound/stale idx Co-authored-by: MervinPraison <MervinPraison@users.noreply.github.com>
|
@coderabbitai review |
|
/review |
✅ Action performedReview finished.
|
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
MervinPraison
added
pipeline/blocked:ci
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe changes preserve consumed workflow execution context, enforce timeouts for MCP requests, and retain item indexes when collecting parallel workflow results and errors. ChangesExecution reliability
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile SummaryThis PR fixes three execution-path defects:
Confidence Score: 3/5The PR is not yet safe to merge because workflow context can leak into a later independent execution of the same Task. Retaining Files Needing Attention: src/praisonai-agents/praisonaiagents/agents/agents.py; src/praisonai-agents/praisonaiagents/process/process.py
|
| Filename | Overview |
|---|---|
| src/praisonai-agents/praisonaiagents/agents/agents.py | Preserves workflow context across retries, but retaining it on the Task allows stale context to enter later direct or sequential executions. |
| src/praisonai-agents/praisonaiagents/mcp/mcp.py | Adds server-side coroutine timeouts and reports timeout failures through the existing response queue. |
| src/praisonai-agents/praisonaiagents/workflows/workflows.py | Maps each parallel future to its source index so exceptions are attributed and ordered correctly. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
W[Workflow selects task] --> S[Store _execution_context on Task]
S --> B[Build prompt]
B --> G{Guardrail or completion retry?}
G -->|Yes| B
G -->|No| N{Next execution path}
N -->|Workflow selector fallback| C[Clear task contexts]
N -->|Direct or sequential reuse| L[Retained context enters later prompt]
Reviews (2): Last reviewed commit: "fix: preserve workflow _execution_contex..." | Re-trigger Greptile
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
MervinPraison
added
pipeline/awaiting-merge-gate
|
Claude finished @MervinPraison's task —— View job Claude Architecture Review — PR #3458 ✅Verdict: APPROVE (after fixing the one valid blocking issue). Changes are lightweight, protocol-driven, backward-compatible, and add genuine SDK robustness. One fix pushed to this branch. Phase 1 — Architecture review (per AGENTS.md)
Phase 2 — Issues found & fixed🔴 VALID (Greptile P1) — retry loses workflow context — FIXED Fix: removed the clearing. The ✅ MCP per-call timeout ( ✅ Parallel loop Phase 3 — Verification
Files modified this review
Skipped: nothing outstanding. MCP and workflows changes were already correct. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/praisonai-agents/praisonaiagents/mcp/mcp.py`:
- Around line 87-88: Update the asyncio.TimeoutError handling in the MCP call
flow to remove the “server side” wording, since the timeout is enforced
client-side. Use neutral wording indicating that the MCP {kind} call exceeded
the configured timeout while preserving the existing response_queue.put failure
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5b96b330-4133-4779-b417-c5312403c38a
📒 Files selected for processing (3)
src/praisonai-agents/praisonaiagents/agents/agents.pysrc/praisonai-agents/praisonaiagents/mcp/mcp.pysrc/praisonai-agents/praisonaiagents/workflows/workflows.py
| except asyncio.TimeoutError: | ||
| response_queue.put((False, f"MCP {kind} call timed out after {self.timeout} seconds (server side)")) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file excerpt =="
sed -n '1,130p' src/praisonai-agents/praisonaiagents/mcp/mcp.py
echo
echo "== timeout-related occurrences =="
rg -n "TimeoutError|wait_for|timeout|Response|response_queue|server side|configured timeout" src/praisonai-agents/praisonaiagents/mcp/mcp.py src/praisonai-agents -g '*.py' || trueRepository: MervinPraison/PraisonAI
Length of output: 50380
Use neutral timeout wording.
This timeout is enforced client-side by asyncio.wait_for; labeling it “server side” can misdirect troubleshooting. Use wording such as f"MCP {kind} call exceeded the configured timeout".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/praisonai-agents/praisonaiagents/mcp/mcp.py` around lines 87 - 88, Update
the asyncio.TimeoutError handling in the MCP call flow to remove the “server
side” wording, since the timeout is enforced client-side. Use neutral wording
indicating that the MCP {kind} call exceeded the configured timeout while
preserving the existing response_queue.put failure behavior.
Do not clear task._execution_context inside _build_execution_context. The run_task/arun_task retry loops re-enter this helper on guardrail / completion failures; clearing stripped the upstream output and validation feedback the retry needs (Greptile P1). The Process engine already owns this field's lifecycle (re-set before each yield, reset to None before selecting the next task), so no cross-task leak. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
MervinPraison
added
pipeline/blocked:stale-final
| extra_context = getattr(task, '_execution_context', None) | ||
| if extra_context: | ||
| context_text = extra_context | ||
| # NOTE: We intentionally do NOT clear _execution_context here. Task | ||
| # retries (guardrail/completion failures) re-enter this helper via the | ||
| # run_task/arun_task retry loops, and clearing would strip the upstream | ||
| # output + validation feedback the retry needs. The Process engine owns | ||
| # this field's lifecycle: it re-sets it before each task yield and resets | ||
| # every task's _execution_context to None before selecting the next task. |
There was a problem hiding this comment.
When an Agents/Task instance is reused through direct run_task/arun_task execution or switched from workflow to sequential execution, _build_execution_context consumes the previous workflow's retained _execution_context because those paths do not pass through the only cleanup site. The later prompt therefore includes stale upstream output or validation feedback and can produce an answer for the wrong execution context.
Knowledge Base Used: praisonai-agents Core Library
praisonai-triage-agent
Bot
removed
pipeline/blocked:ci
1 participant
Fixes #3457
Summary
Addresses three architectural/functional gaps in
src/praisonai-agents/praisonaiagents:MCP
call_tool/read_resource/get_promptper-call timeout (mcp/mcp.py) — wrapped the in-flightsession.call_tool/read_resource/get_promptawaits withasyncio.wait_for(..., timeout=self.timeout). A hung MCP server now fails that single request (... timed out ... (server side)) instead of permanently wedging the single-threaded runner loop, so subsequent calls and the shutdown sentinel keep working.Workflow inter-task context / validation feedback now reaches the prompt (
agents/agents.py) —Process._build_task_context()stores its computed context ontask._execution_context, but the prompt builder only readtask.context. Now_build_execution_contextfoldstask._execution_contextintocontext_text(prepended before explicit context-task results) and clears it after consumption so it doesn't leak into a later run. Downstream/retriedworkflowtasks now see upstream output and rejection reasons.Parallel
Loop._execute_loopunbound/staleidx(workflows/workflows.py) — replaced the plainfutureslist with afuture -> idxmap and readidxfrom that map in theas_completedloop. Error attribution now survives an exception raised beforefuture.result()returns its index (previouslyNameErroror misattributed/duplicate indices).All changes are minimal and backward-compatible; no new params, modules, or exports.
Test plan
python -m py_compileon all three modified files: OKmcp/litellm/typerpackages and unrelated collection import errors), not related to these changes.Generated with Claude Code
Summary by CodeRabbit