Revert broken async sleep helpers in execution_mixin.py#1279
Revert broken async sleep helpers in execution_mixin.py#1279praisonai-triage-agent[bot] wants to merge 3 commits intomainfrom
Conversation
Replace 6 blocking time.sleep() calls with async-safe alternatives to prevent event loop blocking in multi-agent workflows. - Added _smart_sleep() utility that detects async context - Added _async_safe_sleep() utility for async-safe sleeping - Replaced blocking calls at lines 956, 959, 988, 997, 1056, 1061 - Maintains backward compatibility for sync contexts - No new dependencies added - Follows AGENTS.md principles: async-safe by default and performance-first Fixes #1261 Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
|
@coderabbitai review |
|
/review |
|
@gemini review this PR |
✅ Actions performedReview triggered.
|
Code Review by Qodo
1. _smart_sleep() not awaited
|
|
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:
📝 WalkthroughWalkthroughAdded Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces _async_safe_sleep and _smart_sleep utility functions intended to handle sleeping across synchronous and asynchronous contexts. However, the implementation is fundamentally flawed for the current synchronous call sites. Because these functions return asyncio.Task or coroutine objects without being awaited, the execution flow is never actually paused. This leads to critical issues, including high-CPU busy loops in keep-alive sections and the failure to wait for server registration. It is recommended to revert to time.sleep() for these synchronous blocks or properly refactor the calling methods to be asynchronous.
| def _async_safe_sleep(seconds: float): | ||
| """Async-safe sleep that uses asyncio.sleep when in an event loop.""" | ||
| try: | ||
| # Check if there's a running event loop | ||
| loop = asyncio.get_running_loop() | ||
| if loop.is_running(): | ||
| # We're in an async context, use asyncio.sleep | ||
| return asyncio.create_task(asyncio.sleep(seconds)) | ||
| except RuntimeError: | ||
| # No running event loop, we're in sync context | ||
| pass | ||
|
|
||
| # Fallback to regular time.sleep for sync contexts | ||
| time.sleep(seconds) | ||
| return None |
There was a problem hiding this comment.
The implementation of _async_safe_sleep is problematic for synchronous callers. When an event loop is running, it returns an asyncio.Task and continues execution immediately. Since the calling code in this file is synchronous and does not (and cannot) await this task, the execution flow is not actually paused. This is not a valid replacement for time.sleep() if the intention is to wait before proceeding to the next line of code.
| def _smart_sleep(seconds: float): | ||
| """Smart sleep that detects async context and uses appropriate sleep method. | ||
|
|
||
| Returns a coroutine if in async context, otherwise sleeps synchronously. | ||
| """ | ||
| try: | ||
| # Check if there's a running event loop | ||
| loop = asyncio.get_running_loop() | ||
| if loop.is_running(): | ||
| # We're in an async context, return a coroutine | ||
| async def async_sleep(): | ||
| await asyncio.sleep(seconds) | ||
| return async_sleep() | ||
| except RuntimeError: | ||
| # No running event loop, we're in sync context | ||
| pass | ||
|
|
||
| # Sync context - use regular time.sleep | ||
| time.sleep(seconds) | ||
| return None |
There was a problem hiding this comment.
The _smart_sleep function returns a coroutine object when an event loop is detected. However, all the call sites in this PR are synchronous and do not await the result. Consequently, in any environment where an event loop is running (e.g., an async application using this library), the sleep will be skipped entirely. This breaks the logic of waiting for server registration and, most critically, leads to high-CPU busy loops in the keep-alive sections of the code.
| while True: | ||
| time.sleep(1) | ||
| _smart_sleep(1) |
There was a problem hiding this comment.
This while True loop will become a high-CPU busy loop in an asynchronous environment. Because _smart_sleep(1) returns a coroutine object immediately without awaiting it, the loop will iterate as fast as possible without any delay. Synchronous code that needs to pause execution must use a blocking sleep like time.sleep(). If the goal is to avoid blocking the event loop, the launch methods should be refactored to be async so they can properly await asyncio.sleep().
| while True: | |
| time.sleep(1) | |
| _smart_sleep(1) | |
| while True: | |
| time.sleep(1) |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| try: | ||
| print("\nAll agents registered for HTTP mode. Press Ctrl+C to stop the servers.") | ||
| while True: | ||
| time.sleep(1) | ||
| _smart_sleep(1) |
There was a problem hiding this comment.
1. _smart_sleep() not awaited 📎 Requirement gap ☼ Reliability
_smart_sleep() returns a coroutine when an event loop is running, but the updated call sites invoke it without await. In async/event-loop execution this results in no delay and can degenerate into a tight loop that blocks the event loop and harms concurrency/performance.
Agent Prompt
## Issue description
`_smart_sleep()` returns a coroutine in async contexts, but callers do not `await` it. This means the intended delay does not happen under asyncio, and keep-alive loops can become busy loops that starve the event loop.
## Issue Context
The PR compliance requirements expect dual sync/async wait behavior to be safely dispatched based on whether an event loop is running.
## Fix Focus Areas
- src/praisonai-agents/praisonaiagents/agent/execution_mixin.py[35-54]
- src/praisonai-agents/praisonaiagents/agent/execution_mixin.py[995-999]
- src/praisonai-agents/praisonaiagents/agent/execution_mixin.py[1024-1037]
- src/praisonai-agents/praisonaiagents/agent/execution_mixin.py[1095-1101]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| def _async_safe_sleep(seconds: float): | ||
| """Async-safe sleep that uses asyncio.sleep when in an event loop.""" | ||
| try: | ||
| # Check if there's a running event loop | ||
| loop = asyncio.get_running_loop() | ||
| if loop.is_running(): | ||
| # We're in an async context, use asyncio.sleep | ||
| return asyncio.create_task(asyncio.sleep(seconds)) | ||
| except RuntimeError: | ||
| # No running event loop, we're in sync context | ||
| pass | ||
|
|
||
| # Fallback to regular time.sleep for sync contexts | ||
| time.sleep(seconds) | ||
| return None |
There was a problem hiding this comment.
2. _async_safe_sleep() uses create_task 📎 Requirement gap ☼ Reliability
_async_safe_sleep() schedules asyncio.sleep() via asyncio.create_task(...) and immediately returns, so it does not actually wait/sleep for the caller. If used in polling/keep-alive code paths, this can create unbounded tasks and fails to provide a safe sync/async sleep dispatch semantics.
Agent Prompt
## Issue description
`_async_safe_sleep()` uses `asyncio.create_task(asyncio.sleep(...))`, which schedules a sleep but does not wait for it, changing semantics and potentially leaking tasks in loops.
## Issue Context
To be async-safe, code running on the event loop should use `await asyncio.sleep(...)` (or equivalent) so the caller actually yields control for the intended duration.
## Fix Focus Areas
- src/praisonai-agents/praisonaiagents/agent/execution_mixin.py[18-32]
ⓘ 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/praisonai-agents/praisonaiagents/agent/execution_mixin.py (1)
1082-1085:⚠️ Potential issue | 🔴 CriticalMissing import:
Mountis not imported from starlette.
Mountis used on line 1084 but is not imported. This will raise aNameErrorat runtime when launching in MCP mode.🐛 Proposed fix to add missing import
from starlette.applications import Starlette + from starlette.routing import Mount import threading🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/praisonai-agents/praisonaiagents/agent/execution_mixin.py` around lines 1082 - 1085, The code references Mount when constructing starlette_app inside the execution mixin (transport = SseServerTransport(f"{path}/sse"); starlette_app = Starlette(routes=[Mount(f"{path}", mcp.create_app())])) but Mount is not imported, causing a NameError at runtime; fix by adding the missing import for Mount (import Mount from starlette.routing) alongside the existing Starlette import so the Mount symbol is available when building starlette_app that wraps mcp.create_app().
🧹 Nitpick comments (1)
src/praisonai-agents/praisonaiagents/agent/execution_mixin.py (1)
18-32: Dead code:_async_safe_sleepis defined but never used.This function is never called anywhere in the diff or referenced by
_smart_sleep. Additionally, it has the same issue as_smart_sleep—it returns a task viaasyncio.create_task()but then falls through totime.sleep()anyway when in sync context, and the returned task is not awaited by callers.♻️ Remove unused function
-def _async_safe_sleep(seconds: float): - """Async-safe sleep that uses asyncio.sleep when in an event loop.""" - try: - # Check if there's a running event loop - loop = asyncio.get_running_loop() - if loop.is_running(): - # We're in an async context, use asyncio.sleep - return asyncio.create_task(asyncio.sleep(seconds)) - except RuntimeError: - # No running event loop, we're in sync context - pass - - # Fallback to regular time.sleep for sync contexts - time.sleep(seconds) - return None - -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/praisonai-agents/praisonaiagents/agent/execution_mixin.py` around lines 18 - 32, Remove the dead helper _async_safe_sleep and update usages to rely on the existing _smart_sleep; specifically delete the _async_safe_sleep function definition and ensure _smart_sleep properly handles both sync and async contexts (create and await an asyncio.sleep task when an event loop is running, and use time.sleep in sync contexts) so callers don't get an unawaited Task or fall through to time.sleep unexpectedly; reference the symbols _async_safe_sleep and _smart_sleep when making the changes.
🤖 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/execution_mixin.py`:
- Around line 35-54: _smart_sleep currently returns an un-awaited coroutine when
an event loop is running, causing the sleep to be skipped; change its
async-branch to schedule the sleep on the running loop using
asyncio.run_coroutine_threadsafe and block until completion (e.g., call
asyncio.run_coroutine_threadsafe(asyncio.sleep(seconds), loop).result()),
catching and falling back to time.sleep on errors; reference the _smart_sleep
function, asyncio.get_running_loop(), asyncio.sleep and
asyncio.run_coroutine_threadsafe so you can locate the change, and ensure
exceptions from .result() are handled (and a synchronous time.sleep used as
fallback).
---
Outside diff comments:
In `@src/praisonai-agents/praisonaiagents/agent/execution_mixin.py`:
- Around line 1082-1085: The code references Mount when constructing
starlette_app inside the execution mixin (transport =
SseServerTransport(f"{path}/sse"); starlette_app =
Starlette(routes=[Mount(f"{path}", mcp.create_app())])) but Mount is not
imported, causing a NameError at runtime; fix by adding the missing import for
Mount (import Mount from starlette.routing) alongside the existing Starlette
import so the Mount symbol is available when building starlette_app that wraps
mcp.create_app().
---
Nitpick comments:
In `@src/praisonai-agents/praisonaiagents/agent/execution_mixin.py`:
- Around line 18-32: Remove the dead helper _async_safe_sleep and update usages
to rely on the existing _smart_sleep; specifically delete the _async_safe_sleep
function definition and ensure _smart_sleep properly handles both sync and async
contexts (create and await an asyncio.sleep task when an event loop is running,
and use time.sleep in sync contexts) so callers don't get an unawaited Task or
fall through to time.sleep unexpectedly; reference the symbols _async_safe_sleep
and _smart_sleep when making the changes.
🪄 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: 094f2715-62d1-4c65-bcd3-cce050fb5945
📒 Files selected for processing (1)
src/praisonai-agents/praisonaiagents/agent/execution_mixin.py
| def _smart_sleep(seconds: float): | ||
| """Smart sleep that detects async context and uses appropriate sleep method. | ||
|
|
||
| Returns a coroutine if in async context, otherwise sleeps synchronously. | ||
| """ | ||
| try: | ||
| # Check if there's a running event loop | ||
| loop = asyncio.get_running_loop() | ||
| if loop.is_running(): | ||
| # We're in an async context, return a coroutine | ||
| async def async_sleep(): | ||
| await asyncio.sleep(seconds) | ||
| return async_sleep() | ||
| except RuntimeError: | ||
| # No running event loop, we're in sync context | ||
| pass | ||
|
|
||
| # Sync context - use regular time.sleep | ||
| time.sleep(seconds) | ||
| return None |
There was a problem hiding this comment.
Critical: Returned coroutine is never awaited, sleep won't occur in async contexts.
When a running event loop is detected, this function returns a coroutine object but the caller never awaits it. Since all call sites (lines 995, 998, 1027, 1036, 1095, 1100) are in sync methods that simply call _smart_sleep(...) without await, the sleep is silently skipped and Python emits a RuntimeWarning: coroutine 'async_sleep' was never awaited.
To properly block synchronously while not blocking the event loop, use run_coroutine_threadsafe:
🐛 Proposed fix using run_coroutine_threadsafe
def _smart_sleep(seconds: float):
"""Smart sleep that detects async context and uses appropriate sleep method.
- Returns a coroutine if in async context, otherwise sleeps synchronously.
+ Sleeps without blocking the event loop when called from a sync context
+ while an async loop is running in another thread.
"""
try:
# Check if there's a running event loop
loop = asyncio.get_running_loop()
if loop.is_running():
- # We're in an async context, return a coroutine
- async def async_sleep():
- await asyncio.sleep(seconds)
- return async_sleep()
+ # Schedule async sleep on the running loop and wait for completion
+ future = asyncio.run_coroutine_threadsafe(asyncio.sleep(seconds), loop)
+ future.result() # Block until sleep completes
+ return
except RuntimeError:
# No running event loop, we're in sync context
pass
# Sync context - use regular time.sleep
time.sleep(seconds)
- return NoneBased on learnings: "All I/O operations must have both sync and async variants; never block the event loop with sync I/O in async context."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai-agents/praisonaiagents/agent/execution_mixin.py` around lines
35 - 54, _smart_sleep currently returns an un-awaited coroutine when an event
loop is running, causing the sleep to be skipped; change its async-branch to
schedule the sleep on the running loop using asyncio.run_coroutine_threadsafe
and block until completion (e.g., call
asyncio.run_coroutine_threadsafe(asyncio.sleep(seconds), loop).result()),
catching and falling back to time.sleep on errors; reference the _smart_sleep
function, asyncio.get_running_loop(), asyncio.sleep and
asyncio.run_coroutine_threadsafe so you can locate the change, and ensure
exceptions from .result() are handled (and a synchronous time.sleep used as
fallback).
…eep() in sync methods Agent-Logs-Url: https://github.com/MervinPraison/PraisonAI/sessions/4e986fa2-76e0-466f-a027-82bf3291d31a Co-authored-by: MervinPraison <454862+MervinPraison@users.noreply.github.com>
Here is the thorough review, with issues already fixed in commit 1. Bloat check ❌The two helper functions add 39 lines but provide no benefit (see #3 below). The change could be expressed as a zero-line diff. 2. Security ✅No hardcoded secrets, eval/exec, or missing input validation. 3. Performance – Critical bug ❌
_smart_sleep(0.5) # no awaitWhen a running event loop is detected,
This is strictly worse than the original 4. Tests ❌No unit tests included. The core behavior (async vs sync context) is not tested. 5. Backward compat
|
|
@claude You are the FINAL architecture reviewer. 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 ✅Todo List
Phase 1: Architecture Review per AGENTS.md ✅1. Protocol-driven Core ✅PASS: No heavy implementations added to core SDK. The changes are in the existing execution_mixin.py which already contains server launch methods. 2. Backward Compatible ✅PASS: All changes are internal bug fixes. No public API changes or feature regressions. The 3. Performance ✅PASS: No hot-path regressions. The fix actually improves performance by removing broken async helpers that could cause 100% CPU busy loops. Phase 2: Bug Fixes Applied ✅Issue AnalysisAll reviewers correctly identified critical bugs in commit
Files Modified
What I Skipped
Phase 3: Final Verdict ✅APPROVE THE PRAll critical issues have been resolved:
Technical Summary: Status: ✅ READY TO MERGE |
- Add missing 'from starlette.routing import Mount' import - Mount was used on line 1046 but not imported - Fixes NameError when launching agents in MCP mode 🤖 Generated with Claude Code Co-authored-by: MervinPraison <454862+MervinPraison@users.noreply.github.com>
This PR addresses review feedback on issue #1261 by removing broken helper functions and restoring correct
time.sleep()calls in synchronous server-launcher methods.Problem
The previously introduced
_smart_sleep()and_async_safe_sleep()helper functions did not correctly solve the async blocking problem:_smart_sleep()returns an unawaited coroutine when called from async context (since all call sites use it withoutawait), resulting in a no-op and aRuntimeWarning: coroutine was never awaitedwhile True: _smart_sleep(1)keep-alive loops would spin at 100% CPU in async context, since the coroutine is never awaited and no actual sleep occurs_async_safe_sleep()was dead code — defined but never called anywhereSolution
_smart_sleep()and_async_safe_sleep()helper functions (39 lines of broken/dead code)time.sleep()at all 6 call sites in_launch_http_server()and_launch_mcp_server()time.sleep()is the correct primitive for blocking the main thread during server startup and keep-alive loopslaunch()would require exposing anasync def launch()variant usingawait asyncio.sleep()— that is a separate, larger changeTesting