fix: resolve thread/async safety issues in multi-agent deployments#1182
fix: resolve thread/async safety issues in multi-agent deployments#1182github-actions[bot] wants to merge 2 commits intomainfrom
Conversation
- Add thread-safe locking for global dicts in agents.py using _agents_lock - Implement double-checked locking pattern for ToolRegistry singleton - Fix asyncio.run() usage in async contexts with event loop detection - Protect _pending_approvals dict with async locks and prevent iteration races Fixes #1167 - ensures "multi-agent + async safe by default" principle Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
|
@coderabbitai review |
|
/review |
✅ Actions performedReview triggered.
|
Code Review by Qodo
1. _check_tool_approval_sync still uses asyncio.run
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds thread-safety protections to three critical modules: the Agent class now guards approval tracking with an async lock and detects running event loops before calling Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
Claude finished @MervinPraison's task —— View job Thread/Async Safety Analysis Complete ✅I've analyzed the PR implementation against issue #1167 and confirmed all critical fixes are properly implemented. Issues Fixed✅ Issue 1: Unprotected global dicts in
|
|
Claude finished @MervinPraison's task —— View job Code Review CompletedI've thoroughly reviewed PR #1182 which addresses critical thread safety and asyncio issues. Here's my comprehensive analysis: ✅ Strengths1. Proper Thread Safety Implementation
2. Asyncio Event Loop Handling
3. Async Lock Protection
🔍 Technical Analysisagent.py Changes:
agents.py Changes:
registry.py Changes:
✅ Adherence to AGENTS.md Guidelines
🧪 Testing Verification
Minor Suggestions
🎯 Overall AssessmentThis is a high-quality fix that addresses all stated issues without introducing new problems. The implementation is:
Recommendation: Approve and merge. This PR successfully resolves critical concurrency issues while maintaining code quality and following established patterns. |
|
@claude You are the FINAL architecture reviewer. Read ALL comments above from Qodo, Coderabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: Fix valid issues from other reviewers Phase 3: Final verdict |
|
Claude finished @MervinPraison's task —— View job Final Architecture Review ✅Completed comprehensive review of PR #1182 per AGENTS.md guidelines and analyzed feedback from other reviewers. Todo List
Phase 1: Architecture Review per AGENTS.md ✅1. Protocol-driven: ✅ COMPLIANT
2. Backward compatible: ✅ COMPLIANT
3. Performance: ✅ COMPLIANT
4. DRY: ✅ COMPLIANT
5. Async-safe and multi-agent safe: ✅ COMPLIANT
Phase 2: Other Reviewer Analysis ✅Qodo Review: Still in progress ("Looking for bugs?") Phase 3: Technical Validation ✅✅ Thread Safety Patterns Validated
✅ Syntax Validation Passed
Final Verdict: ✅ APPROVEDThis PR successfully addresses critical thread safety and async concurrency issues in multi-agent deployments while maintaining full compliance with AGENTS.md architectural principles. The implementation:
Recommendation: ✅ MERGE - This PR resolves critical concurrency issues and strengthens the framework's production readiness. |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| # Handle async context properly - don't use asyncio.run() in running loop | ||
| try: | ||
| # Check if we're already in an event loop | ||
| loop = asyncio.get_running_loop() | ||
| # If we're here, we're in an async context - can't use asyncio.run() | ||
| # Create a task and run it synchronously (this is a sync method) | ||
| import concurrent.futures | ||
| with concurrent.futures.ThreadPoolExecutor() as executor: | ||
| future = executor.submit(asyncio.run, backend.request_approval(request)) | ||
| decision = future.result() | ||
| except RuntimeError: | ||
| # No running loop, safe to use asyncio.run() | ||
| decision = asyncio.run(backend.request_approval(request)) |
There was a problem hiding this comment.
1. _check_tool_approval_sync still uses asyncio.run 📎 Requirement gap ⛯ Reliability
When an event loop is already running, _check_tool_approval_sync() still invokes asyncio.run() (via a ThreadPoolExecutor), which violates the requirement to avoid asyncio.run() in that approval flow. This can also block the event loop thread and cause responsiveness issues during async execution paths.
Agent Prompt
## Issue description
`_check_tool_approval_sync()` still calls `asyncio.run()` even when `asyncio.get_running_loop()` succeeds (it just moves the call into a `ThreadPoolExecutor`). The compliance requirement is to avoid `asyncio.run()` in this approval flow when an event loop is already running.
## Issue Context
This method is a sync boundary in the tool approval flow, and can be reached during async execution paths. The current approach still relies on `asyncio.run()` and can also block the event-loop thread while waiting for `future.result()`.
## Fix Focus Areas
- src/praisonai-agents/praisonaiagents/agent/agent.py[5068-5080]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| import concurrent.futures | ||
| with concurrent.futures.ThreadPoolExecutor() as executor: | ||
| future = executor.submit(asyncio.run, backend.request_approval(request)) | ||
| decision = future.result() |
There was a problem hiding this comment.
2. Approval wait lacks timeout 🐞 Bug ⛯ Reliability
In Agent._check_tool_approval_sync(), the running-event-loop fallback executes backend.request_approval() in a new ThreadPoolExecutor but calls future.result() with no timeout, which can block indefinitely and stall the calling thread/event loop. This is inconsistent with existing approval codepaths that apply explicit timeouts for the same nested-loop scenario.
Agent Prompt
## Issue description
`Agent._check_tool_approval_sync()` runs async approval in a background thread when an event loop is already running, but waits using `future.result()` without any timeout. If the backend hangs (network, webhook, user prompt, etc.), this blocks indefinitely.
## Issue Context
The codebase already uses bounded waits for the same nested-event-loop pattern in `ApprovalRegistry.approve_sync()` and `AgentApproval.request_approval_sync()`.
## Fix Focus Areas
- src/praisonai-agents/praisonaiagents/agent/agent.py[5068-5080]
## Suggested change
- Use `ThreadPoolExecutor(max_workers=1)` and call `future.result(timeout=...)`.
- Derive the timeout from existing config:
- If `cfg_timeout is None`: preserve “indefinite” semantics intentionally (or cap to a very large value), but make it explicit.
- If `cfg_timeout > 0`: enforce it by passing it to `future.result(timeout=cfg_timeout)` and/or wrapping the coroutine with `asyncio.wait_for` inside the `asyncio.run` call (e.g., `asyncio.run(asyncio.wait_for(..., timeout=cfg_timeout))`).
- Consider reusing the existing approval registry implementation to avoid divergent timeout behavior.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
@copilot Do a thorough review of this PR. Read ALL existing reviewer comments above first. Review areas:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/praisonai-agents/praisonaiagents/tools/registry.py (2)
308-324:⚠️ Potential issue | 🟠 Major
remove_tool()bypasses thread-safe registry methods.This function directly accesses
registry._toolsandregistry._functionswithout using the lock, which can cause race conditions in multi-agent scenarios. Use the existing thread-saferegistry.unregister()method instead.🔒 Proposed fix to use the thread-safe method
def remove_tool(name: str) -> bool: """Remove a tool from the global registry. Args: name: Name of the tool to remove Returns: True if tool was found and removed, False otherwise """ - registry = get_registry() - if name in registry._tools: - del registry._tools[name] - return True - if name in registry._functions: - del registry._functions[name] - return True - return False + return get_registry().unregister(name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/praisonai-agents/praisonaiagents/tools/registry.py` around lines 308 - 324, The remove_tool function currently mutates registry._tools and registry._functions directly (via get_registry() and del registry._tools[name] / del registry._functions[name]) which is not thread-safe; replace that direct access by calling the registry's thread-safe unregister method (registry.unregister(name)) and return its boolean result instead, removing any direct references to registry._tools or registry._functions so all removals go through the registry.unregister API.
327-334:⚠️ Potential issue | 🟡 Minor
list_tools()module function bypasses thread-safe registry method.This function directly accesses internal dicts without locking. Use the existing thread-safe
registry.list_tools()method instead.🔒 Proposed fix to use the thread-safe method
def list_tools() -> List[str]: """List all registered tool names. Returns: List of tool names """ - registry = get_registry() - return list(registry._tools.keys()) + list(registry._functions.keys()) + return get_registry().list_tools()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/praisonai-agents/praisonaiagents/tools/registry.py` around lines 327 - 334, The function list_tools() currently bypasses the registry's thread-safe API by directly reading registry._tools and registry._functions; change it to call the registry's thread-safe method instead (use get_registry() to retrieve the registry and return registry.list_tools()), preserving the return type List[str] and removing direct access to internal dicts to ensure proper locking.
🧹 Nitpick comments (2)
src/praisonai-agents/praisonaiagents/tools/registry.py (1)
252-254: Remove duplicatethreadingimport.
threadingis already imported at line 18. This duplicate import is unnecessary.# Global registry instance (protected by _registry_lock for thread safety) -import threading _registry_lock = threading.Lock()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/praisonai-agents/praisonaiagents/tools/registry.py` around lines 252 - 254, Remove the duplicate import of the threading module and keep only the existing import at the top of the file; delete the redundant "import threading" near the definition of _registry_lock and leave the _registry_lock = threading.Lock() line intact so the global registry lock remains defined and uses the already-imported threading symbol.src/praisonai-agents/praisonaiagents/agents/agents.py (1)
1619-1619: Remove_agents_lockfrom global statement.The
globalkeyword is only needed for variables that are reassigned._agents_lockis never reassigned (only used via.acquire()/context manager), so it doesn't need to be in the global statement.- global _agents_server_started, _agents_registered_endpoints, _agents_shared_apps, _agents_lock + global _agents_server_started, _agents_registered_endpoints, _agents_shared_apps🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/praisonai-agents/praisonaiagents/agents/agents.py` at line 1619, The global statement declaring `_agents_server_started, _agents_registered_endpoints, _agents_shared_apps, _agents_lock` should be reduced to only the names that are actually reassigned; remove `_agents_lock` from the global list. Edit the global declaration in the function/method that currently contains that statement so it becomes `global _agents_server_started, _agents_registered_endpoints, _agents_shared_apps`, leaving `_agents_lock` referenced normally (since it is only used via methods like `.acquire()` and not reassigned).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/praisonai-agents/praisonaiagents/agent/agent.py`:
- Around line 8405-8415: The loop over approved_for_execution is calling
execute_tool_async(...) which immediately re-enters _check_tool_approval_async
and can re-request approval, dropping decision.modified_args; refactor so
approved executions bypass approval by factoring the tool-dispatch logic out of
execute_tool_async into a shared helper (e.g., _dispatch_tool or
execute_tool_no_approval) that accepts the final argument set (use
decision.modified_args when present) and is invoked directly from the
approved_for_execution path, while keeping execute_tool_async to perform the
approval path that calls the same dispatch helper; ensure unique identifiers
referenced: approved_for_execution loop, execute_tool_async,
_check_tool_approval_async, decision.modified_args, and the new dispatch helper
name you choose.
- Around line 5068-5080: The code currently wraps both
asyncio.get_running_loop() and future.result() in one try/except, so a
RuntimeError raised inside the worker thread (future.result()) is misinterpreted
as "no running loop"; to fix, call asyncio.get_running_loop() inside a small try
that only detects RuntimeError for loop presence, and if a loop exists create a
ThreadPoolExecutor and executor.submit(asyncio.run,
backend.request_approval(request)) then call future.result() in a separate
try/except that captures exceptions from the task (and re-raises or logs them)
without treating them as loop-detection errors; ensure
asyncio.run(backend.request_approval(request)) is only invoked in the branch
where get_running_loop() raised RuntimeError. Use the same symbols
(asyncio.get_running_loop, concurrent.futures.ThreadPoolExecutor,
executor.submit, future.result, asyncio.run, backend.request_approval) to locate
and change the code.
- Around line 1629-1631: Update the comment and related docstring to state that
asyncio.Lock (_pending_approvals_lock) provides coroutine/event-loop safety only
and does not provide cross-thread synchronization; replace any mention of
"thread safety" with "event-loop / coroutine safety" and add a note that
accessing _pending_approvals/_pending_approvals_lock from a different thread
will raise RuntimeError and requires a thread-safe primitive or loop-safe
scheduling (e.g., run_coroutine_threadsafe) instead. Ensure references are
changed in the inline comment next to _pending_approvals_lock and the docstring
that mentions it (the docstring around the previous line ~8435) so both places
accurately describe the lock's guarantees.
In `@src/praisonai-agents/praisonaiagents/agents/agents.py`:
- Around line 1664-1679: The async handlers root() and healthcheck()
synchronously acquire _agents_lock which can block the event loop; remove the
blocking lock from these handlers by returning a precomputed snapshot of
endpoints or accessing the shared structures in a thread via asyncio.to_thread.
Specifically, update the code paths that modify _agents_registered_endpoints to
maintain a snapshot like _agents_registered_endpoints_snapshot[port] (or refresh
a cached list) when endpoints are registered/unregistered, and change root() and
healthcheck() to read and return that snapshot (or call
asyncio.to_thread(lambda: list(_agents_registered_endpoints[port].keys()))),
avoiding direct with _agents_lock usage inside root() and healthcheck().
---
Outside diff comments:
In `@src/praisonai-agents/praisonaiagents/tools/registry.py`:
- Around line 308-324: The remove_tool function currently mutates
registry._tools and registry._functions directly (via get_registry() and del
registry._tools[name] / del registry._functions[name]) which is not thread-safe;
replace that direct access by calling the registry's thread-safe unregister
method (registry.unregister(name)) and return its boolean result instead,
removing any direct references to registry._tools or registry._functions so all
removals go through the registry.unregister API.
- Around line 327-334: The function list_tools() currently bypasses the
registry's thread-safe API by directly reading registry._tools and
registry._functions; change it to call the registry's thread-safe method instead
(use get_registry() to retrieve the registry and return registry.list_tools()),
preserving the return type List[str] and removing direct access to internal
dicts to ensure proper locking.
---
Nitpick comments:
In `@src/praisonai-agents/praisonaiagents/agents/agents.py`:
- Line 1619: The global statement declaring `_agents_server_started,
_agents_registered_endpoints, _agents_shared_apps, _agents_lock` should be
reduced to only the names that are actually reassigned; remove `_agents_lock`
from the global list. Edit the global declaration in the function/method that
currently contains that statement so it becomes `global _agents_server_started,
_agents_registered_endpoints, _agents_shared_apps`, leaving `_agents_lock`
referenced normally (since it is only used via methods like `.acquire()` and not
reassigned).
In `@src/praisonai-agents/praisonaiagents/tools/registry.py`:
- Around line 252-254: Remove the duplicate import of the threading module and
keep only the existing import at the top of the file; delete the redundant
"import threading" near the definition of _registry_lock and leave the
_registry_lock = threading.Lock() line intact so the global registry lock
remains defined and uses the already-imported threading symbol.
🪄 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
Run ID: 0a8cc75b-1576-4670-b29f-8846ac4503b9
📒 Files selected for processing (3)
src/praisonai-agents/praisonaiagents/agent/agent.pysrc/praisonai-agents/praisonaiagents/agents/agents.pysrc/praisonai-agents/praisonaiagents/tools/registry.py
| # Pending approvals for async (non-blocking) mode (protected by lock for thread safety) | ||
| self._pending_approvals = {} | ||
| self._pending_approvals_lock = asyncio.Lock() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, examine the exact lines in question (1629-1631)
sed -n '1620,1640p' src/praisonai-agents/praisonaiagents/agent/agent.pyRepository: MervinPraison/PraisonAI
Length of output: 1193
🏁 Script executed:
# Check the lines 8428-8437 referenced in "Also applies to"
sed -n '8420,8445p' src/praisonai-agents/praisonaiagents/agent/agent.pyRepository: MervinPraison/PraisonAI
Length of output: 958
🏁 Script executed:
# Search for _pending_approvals usage throughout the file
rg '_pending_approvals' src/praisonai-agents/praisonaiagents/agent/agent.py -A 2 -B 2Repository: MervinPraison/PraisonAI
Length of output: 2153
🏁 Script executed:
# Check if threading is used anywhere in this file
rg 'threading|Thread|Lock\(\)|RLock' src/praisonai-agents/praisonaiagents/agent/agent.py | head -20Repository: MervinPraison/PraisonAI
Length of output: 1102
🏁 Script executed:
# Check the docstrings and comments around line 8430
sed -n '8420,8440p' src/praisonai-agents/praisonaiagents/agent/agent.pyRepository: MervinPraison/PraisonAI
Length of output: 803
🏁 Script executed:
# Check if _pending_approvals is used across event loops anywhere
rg 'asyncio\.run|create_task|create_task_group|run_in_executor' src/praisonai-agents/praisonaiagents/agent/agent.py | grep -A 3 -B 3 pending || echo "No cross-loop patterns found"Repository: MervinPraison/PraisonAI
Length of output: 95
🏁 Script executed:
# Examine the broader context of how _pending_approvals_lock is initialized and used
sed -n '8060,8120p' src/praisonai-agents/praisonaiagents/agent/agent.py | grep -A 5 -B 5 'pending_approvals\|asyncio.Lock'Repository: MervinPraison/PraisonAI
Length of output: 49
🏁 Script executed:
# Check if Agent is documented as single-threaded or multi-threaded
head -100 src/praisonai-agents/praisonaiagents/agent/agent.py | grep -i 'thread\|async\|concurrent'Repository: MervinPraison/PraisonAI
Length of output: 440
🏁 Script executed:
# Verify the asyncio.Lock semantics - check if there are any guard clauses that prevent multi-thread access
rg 'get_event_loop|get_running_loop' src/praisonai-agents/praisonaiagents/agent/agent.py | head -5Repository: MervinPraison/PraisonAI
Length of output: 352
🏁 Script executed:
# Check the exact docstring and comment at lines 8428-8437 in context
sed -n '8425,8440p' src/praisonai-agents/praisonaiagents/agent/agent.pyRepository: MervinPraison/PraisonAI
Length of output: 682
Clarify that _pending_approvals_lock provides event-loop safety, not thread-safety.
Line 1629's comment and line 8435's docstring misrepresent asyncio.Lock as providing thread-safety. asyncio.Lock is bound to a single event loop and will raise RuntimeError if accessed from a different thread. Correct the documentation to reflect that this provides coroutine/event-loop safety only, not cross-thread synchronization.
📝 Suggested fix
- # Pending approvals for async (non-blocking) mode (protected by lock for thread safety)
+ # Pending approvals for async (non-blocking) mode (protected by asyncio.Lock for event-loop safety)
self._pending_approvals = {}
self._pending_approvals_lock = asyncio.Lock()- async def pending_approval_count_async(self) -> int:
- """Thread-safe version of pending_approval_count."""
+ async def pending_approval_count_async(self) -> int:
+ """Event-loop-safe version of pending_approval_count. Only safe when called from the same event loop."""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai-agents/praisonaiagents/agent/agent.py` around lines 1629 -
1631, Update the comment and related docstring to state that asyncio.Lock
(_pending_approvals_lock) provides coroutine/event-loop safety only and does not
provide cross-thread synchronization; replace any mention of "thread safety"
with "event-loop / coroutine safety" and add a note that accessing
_pending_approvals/_pending_approvals_lock from a different thread will raise
RuntimeError and requires a thread-safe primitive or loop-safe scheduling (e.g.,
run_coroutine_threadsafe) instead. Ensure references are changed in the inline
comment next to _pending_approvals_lock and the docstring that mentions it (the
docstring around the previous line ~8435) so both places accurately describe the
lock's guarantees.
| # Handle async context properly - don't use asyncio.run() in running loop | ||
| try: | ||
| # Check if we're already in an event loop | ||
| loop = asyncio.get_running_loop() | ||
| # If we're here, we're in an async context - can't use asyncio.run() | ||
| # Create a task and run it synchronously (this is a sync method) | ||
| import concurrent.futures | ||
| with concurrent.futures.ThreadPoolExecutor() as executor: | ||
| future = executor.submit(asyncio.run, backend.request_approval(request)) | ||
| decision = future.result() | ||
| except RuntimeError: | ||
| # No running loop, safe to use asyncio.run() | ||
| decision = asyncio.run(backend.request_approval(request)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
head -5080 src/praisonai-agents/praisonaiagents/agent/agent.py | tail -50Repository: MervinPraison/PraisonAI
Length of output: 3143
Fix exception scope—future.result() RuntimeError is not "no running loop".
The inner try/except RuntimeError (lines 5069–5080) catches exceptions from both get_running_loop() and future.result(). Any RuntimeError raised by the worker thread is misidentified as "no running loop" and falls through to asyncio.run() on the active loop, creating the exact nested-loop error this branch tries to avoid.
🛠️ Proposed fix
- try:
- # Check if we're already in an event loop
- loop = asyncio.get_running_loop()
- # If we're here, we're in an async context - can't use asyncio.run()
- # Create a task and run it synchronously (this is a sync method)
- import concurrent.futures
- with concurrent.futures.ThreadPoolExecutor() as executor:
- future = executor.submit(asyncio.run, backend.request_approval(request))
- decision = future.result()
- except RuntimeError:
- # No running loop, safe to use asyncio.run()
- decision = asyncio.run(backend.request_approval(request))
+ try:
+ asyncio.get_running_loop()
+ except RuntimeError:
+ # No running loop, safe to use asyncio.run()
+ decision = asyncio.run(backend.request_approval(request))
+ else:
+ import concurrent.futures
+ with concurrent.futures.ThreadPoolExecutor(max_workers=1) as executor:
+ future = executor.submit(
+ lambda: asyncio.run(backend.request_approval(request))
+ )
+ decision = future.result()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai-agents/praisonaiagents/agent/agent.py` around lines 5068 -
5080, The code currently wraps both asyncio.get_running_loop() and
future.result() in one try/except, so a RuntimeError raised inside the worker
thread (future.result()) is misinterpreted as "no running loop"; to fix, call
asyncio.get_running_loop() inside a small try that only detects RuntimeError for
loop presence, and if a loop exists create a ThreadPoolExecutor and
executor.submit(asyncio.run, backend.request_approval(request)) then call
future.result() in a separate try/except that captures exceptions from the task
(and re-raises or logs them) without treating them as loop-detection errors;
ensure asyncio.run(backend.request_approval(request)) is only invoked in the
branch where get_running_loop() raised RuntimeError. Use the same symbols
(asyncio.get_running_loop, concurrent.futures.ThreadPoolExecutor,
executor.submit, future.result, asyncio.run, backend.request_approval) to locate
and change the code.
| for tid, info, decision in approved_for_execution: | ||
| try: | ||
| tool_result = await self.execute_tool_async( | ||
| info["function_name"], info["arguments"], | ||
| ) | ||
| results[tid] = { | ||
| "status": "approved_and_executed", | ||
| "tool_name": info["function_name"], | ||
| "decision": decision, | ||
| "result": tool_result, | ||
| } |
There was a problem hiding this comment.
Don't send approved work back through approval again.
Line 8407 calls execute_tool_async(), which immediately re-enters _check_tool_approval_async() at Line 8469. That means a completed approval can prompt twice, and decision.modified_args from the first approval are dropped. Execute the tool through a no-approval helper/flag using the approved argument set instead.
🔁 Suggested direction
- tool_result = await self.execute_tool_async(
- info["function_name"], info["arguments"],
- )
+ approved_args = decision.modified_args or info["arguments"]
+ tool_result = await self._execute_tool_async_impl(
+ info["function_name"],
+ approved_args,
+ skip_approval=True,
+ )Factor the dispatch body out of execute_tool_async() so both paths share the same execution logic without re-requesting approval.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai-agents/praisonaiagents/agent/agent.py` around lines 8405 -
8415, The loop over approved_for_execution is calling execute_tool_async(...)
which immediately re-enters _check_tool_approval_async and can re-request
approval, dropping decision.modified_args; refactor so approved executions
bypass approval by factoring the tool-dispatch logic out of execute_tool_async
into a shared helper (e.g., _dispatch_tool or execute_tool_no_approval) that
accepts the final argument set (use decision.modified_args when present) and is
invoked directly from the approved_for_execution path, while keeping
execute_tool_async to perform the approval path that calls the same dispatch
helper; ensure unique identifiers referenced: approved_for_execution loop,
execute_tool_async, _check_tool_approval_async, decision.modified_args, and the
new dispatch helper name you choose.
| @_agents_shared_apps[port].get("/") | ||
| async def root(): | ||
| with _agents_lock: | ||
| return { | ||
| "message": f"Welcome to PraisonAI Agents API on port {port}. See /docs for usage.", | ||
| "endpoints": list(_agents_registered_endpoints[port].keys()) | ||
| } | ||
|
|
||
| # Add healthcheck endpoint | ||
| @_agents_shared_apps[port].get("/health") | ||
| async def healthcheck(): | ||
| with _agents_lock: | ||
| return { | ||
| "status": "ok", | ||
| "endpoints": list(_agents_registered_endpoints[port].keys()) | ||
| } |
There was a problem hiding this comment.
Lock acquisition inside async handlers may block the event loop.
The root() and healthcheck() async handlers acquire _agents_lock synchronously. If another thread holds this lock for an extended period (e.g., during endpoint registration), these handlers will block the asyncio event loop.
Consider using a snapshot approach: read the endpoints list once during registration and store it, or use asyncio.to_thread() to avoid blocking. However, given that lock hold times are brief and this is for low-frequency diagnostic endpoints, this is a minor concern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai-agents/praisonaiagents/agents/agents.py` around lines 1664 -
1679, The async handlers root() and healthcheck() synchronously acquire
_agents_lock which can block the event loop; remove the blocking lock from these
handlers by returning a precomputed snapshot of endpoints or accessing the
shared structures in a thread via asyncio.to_thread. Specifically, update the
code paths that modify _agents_registered_endpoints to maintain a snapshot like
_agents_registered_endpoints_snapshot[port] (or refresh a cached list) when
endpoints are registered/unregistered, and change root() and healthcheck() to
read and return that snapshot (or call asyncio.to_thread(lambda:
list(_agents_registered_endpoints[port].keys()))), avoiding direct with
_agents_lock usage inside root() and healthcheck().
|
@copilot Do a thorough review of this PR. Read ALL existing reviewer comments above first. Review areas:
|
|
Claude finished @MervinPraison's task —— View job Final Architecture Review & Issue Resolution ✅I've completed a comprehensive review of PR #1182 and addressed all critical issues identified by reviewers. Todo List
Phase 1: Architecture Review per AGENTS.md ✅1. Protocol-driven: ✅ COMPLIANT - Changes are in implementation files, not creating new protocols Phase 2: Comprehensive Code Review✅ 1. Bloat Check - MINIMAL AND FOCUSED
✅ 2. Security - NO SECURITY CONCERNS
✅ 3. Performance - NO REGRESSIONS
✅ 4. Tests - SYNTAX VALIDATED
✅ 5. Backward Compatibility - MAINTAINED
✅ 6. Code Quality - IMPROVED
Phase 3: Issue Resolution Summary🔧 All Critical Issues Fixed:Qodo Issues:
CodeRabbit Issues:
📝 Key Improvements Made:
🧪 Validation:
Final Verdict: ✅ APPROVE - READY FOR MERGEThis PR successfully addresses all 4 critical thread safety issues from #1167 AND resolves all additional issues identified by code reviewers. The implementation:
Recommendation: This PR is now production-ready and should be merged to resolve critical concurrency issues in multi-agent deployments. |
- Fix asyncio.run() nested loop handling in _check_tool_approval_sync with proper exception scoping - Add timeout support to future.result() calls to prevent indefinite blocking - Prevent re-approval in approved_for_execution loop by adding _execute_tool_async_impl bypass - Fix documentation: clarify asyncio.Lock provides event-loop safety, not thread-safety - Remove unnecessary global declarations and duplicate threading import - Fix remove_tool() and list_tools() to use thread-safe registry methods - Optimize async handlers to avoid blocking event loop with executor pattern All issues identified by Qodo and CodeRabbit reviewers have been addressed. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
|
Claude finished @MervinPraison's task —— View job Analysis of PR Closure and Remaining Issues ✅Todo List
Analysis SummaryUnderstanding the Context: PR #1175 (Agent thread safety) - MERGED ✅
PR #1176 (Async correctness) - MERGED ✅
🔍 Critical Issues Still Remained - NOW FIXEDI identified 2 specific thread safety issues that were NOT fixed by the merged PRs and created a new PR to address them: Issue 1: Thread-unsafe tool registry access 🚨 → FIXEDProblem: Module-level functions Fix:
Issue 2: Missing thread safety in registry singleton 🚨 → FIXEDProblem: Fix: Added double-checked locking pattern with ✅ Solution ImplementedNew PR Created: #1184 - fix: improve thread safety in tool registry This PR addresses the remaining thread safety issues from the original PR #1182 that were identified by code reviewers but not covered by the merged PRs #1175 and #1176. Testing Results:
Final RecommendationThe closure of PR #1182 was appropriate due to merge conflicts, and the critical fixes have now been properly addressed:
All thread/async safety concerns from issue #1167 have now been comprehensively resolved across these PRs. |
- Use thread-safe registry.unregister() in remove_tool() - Use thread-safe registry.list_tools() in list_tools() - Add double-checked locking to get_registry() singleton - Prevents race conditions in multi-agent deployments Related to #1167 and closed PR #1182 Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
PR Review Summary✅ Bloat CheckChanges are focused and minimal — 3 files modified, all targeting specific concurrency bugs. No unnecessary scope creep. ✅ SecurityNo hardcoded secrets, unsafe
|
asyncio.get_running_loop()+run_in_executorpattern (AGENTS.md compliant)server_should_start = Falsebeforewith _agents_lock:blockimport threadingin registry.py (was already imported at top of file)import concurrent.futuresfrom inside function bodies to module-level imports in agent.pyimport concurrent.futuresinside asyncroot()andhealthcheck()handlers in agents.pyasyncio.get_event_loop()→asyncio.get_running_loop()in async handlers