fix: async guardrail offload + async tool circuit breaker - #4469
Conversation
Two async-safety parity gaps where an async code path silently failed to match its sync sibling's behaviour: 1. Async guardrail validation ran a blocking, synchronous LLM call from inside async methods, stalling the whole event loop (and every task under asyncio.gather). _aapply_guardrail_with_retry and arun_task now offload the sync guardrail chain via loop.run_in_executor, mirroring async_memory_mixin's _run_memory_in_thread pattern. 2. The async tool-execution path had no circuit breaker (only loop-guard parity was added by #4005), so a repeatedly-failing tool could be hammered every turn via achat()/async workflows and the dead circuit_open retry-skip never fired. _execute_tool_async_impl now routes the invocation through the same per-agent/per-tool CircuitBreaker.acall used by the sync path, counting error-dict results as failures and surfacing circuit_open=True when the breaker opens. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
Greptile SummaryThe PR brings async execution closer to synchronous-path behavior while preserving event-loop responsiveness.
Confidence Score: 5/5The PR appears safe to merge. The previously reported context-loss defect is fixed at both guardrail offload sites, and no blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/praisonai-agents/praisonaiagents/agent/agent.py | Offloads synchronous guardrail validation and correctly propagates the caller’s ContextVars into the executor. |
| src/praisonai-agents/praisonaiagents/agent/execution_mixin.py | Adds asynchronous circuit-breaker handling for raised exceptions and error-dict tool failures, including terminal open-circuit results. |
| src/praisonai-agents/praisonaiagents/agents/agents.py | Adds a context-preserving executor wrapper for team-level guardrail validation and awaits it from the async task path. |
Sequence Diagram
sequenceDiagram
participant Caller
participant AsyncPath as Async agent/team path
participant Context as Captured context
participant Worker as Executor worker
participant Guardrail
Caller->>AsyncPath: Await guardrail validation
AsyncPath->>Context: copy_context_to_callable
AsyncPath->>Worker: run_in_executor(wrapped callback)
Worker->>Context: Restore captured ContextVars
Worker->>Guardrail: Run synchronous validation
Guardrail-->>Worker: Validation result
Worker-->>AsyncPath: Result without blocking event loop
AsyncPath-->>Caller: Continue or retry
Reviews (2): Last reviewed commit: "fix: preserve contextvars in async guard..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| loop = asyncio.get_event_loop() | ||
| success, result, error = await loop.run_in_executor( | ||
| None, self._validate_with_guardrail, current_response | ||
| ) |
There was a problem hiding this comment.
When a custom guardrail reads caller-set context variables, these new executor calls invoke validation without copy_context_to_callable, so the worker sees default contextual state and can return a different validation result than the synchronous path; contextual tracing and telemetry can also be lost. The same issue occurs in the team-level offload at agents/agents.py:1350-1353.
Knowledge Base Used: Agent execution and workflows
|
@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: Review changes in this PR. Python SDK: praisonaiagents, praisonai. TypeScript SDK: src/praisonai-ts/. Do NOT modify src/praisonai-rust. 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 |
|
Claude finished @MervinPraison's task —— View job Final Architecture Review — PR #4469 ✅ Approved (1 fix pushed)
Phase 1 — Review per AGENTS.md
Phase 2 — Valid issue found & fixedGreptile P1 (VALID): guardrail context lost in executor offload. The two new Fixed in 2 files:
Verified: Circuit-breaker async path ( Skipped
Phase 3 — Verdict: APPROVEAll valid reviewer feedback resolved. Changes are minimal, backward-compatible, and strengthen async parity/robustness. (Note: I can't submit a formal GitHub approval for security reasons — this is my architectural sign-off.) Tests: Pushed to |
Wrap the two new guardrail run_in_executor offloads with copy_context_to_callable so a custom guardrail sees the same contextual state (trace emission, session context) as the sync path, matching every other run_in_executor call site in the SDK. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
MervinPraison
added
pipeline/blocked:ci
|
@coderabbitai review |
|
/review |
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? |
|
|
Warning Review limit reachedNext included review available in 27 minutes. View limit detailsLimit details: You’ve used all 8 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
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 |
MervinPraison
added
pipeline/awaiting-merge-gate
1 participant
Fixes #4281
Summary
Closes two of the three async-safety parity gaps reported in #4281. Both are cases where an async code path silently failed to match its sync sibling's behaviour. All changes are confined to
src/praisonai-agents/praisonaiagentsand are backward-compatible.1. Async guardrail validation no longer blocks the event loop
A string/
LLMGuardrailguardrail fires a blocking, synchronous LLM call. The async retry helper (_aapply_guardrail_with_retry) and the multi-agent async path (arun_task) called that sync chain unawaited, stalling the whole event loop — every task underasyncio.gathermade zero progress during a guardrail round trip.agent/agent.py:_aapply_guardrail_with_retrynow offloads_validate_with_guardrailvialoop.run_in_executor.agents/agents.py: added_aapply_task_guardrail(executor-offload wrapper) and call it fromarun_task.This mirrors the existing
async_memory_mixin._run_memory_in_threadoffload pattern. The sync path is untouched.2. Async tool execution now has circuit-breaker protection
The sync tool path routes every call through a per-agent/per-tool
CircuitBreaker; the async path (_execute_tool_async_impl) had none — only loop-guard parity was added by #4005. A repeatedly-failing tool could be hammered every turn viaachat()/async workflows, and thecircuit_openretry-skip atexecution_mixin.py:1566was dead code.agent/execution_mixin.py: the async invocation now runs throughCircuitBreaker.acallusing the sametool_{id(self)}_{function_name}naming/finalizer scheme as the sync path. Error-dict results are counted as failures (mirroring the sync_ToolFailurewrapper, excluding approval/permission/policy/guardrail denials), and aCircuitBreakerExceptionsurfacescircuit_open=True— activating the previously-dead retry-skip. Falls back to direct invocation if the circuit_breaker module is unavailable.Out of scope (finding 3)
Finding 3 (
memory/hooks.pyHooksManagernever invoked) is intentionally not included. Wiring.execute()into every file/command/prompt call site is a large cross-cutting change with no live consumer, and removing a documented public export is itself breaking. Per the lightweight/no-scope-creep mandate this is left for a separate, scoped decision.Test plan
test_circuit_breaker.py,test_guardrail_object_dispatch.py,test_llm_guardrail_fail_closed.py,test_clone_keeps_guardrail.py,test_async_tool_retry_parity.py— all pass.🤖 Generated with Claude Code