-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
fix: cancel/time-bound in-flight gateway agent turns #3472
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -848,6 +848,11 @@ def __init__( | |
| self._client_conns: Dict[str, _ClientConn] = {} # client_id -> bounded outbound conn | ||
| self._client_sessions: Dict[str, str] = {} # client_id -> session_id | ||
| self._client_scopes: Dict[str, List[str]] = {} # client_id -> operator scopes | ||
| # Issue #3467: in-flight turn registry so a running turn can be aborted | ||
| # (by a WS ``abort`` frame or a portable ``/stop`` chat command) and so | ||
| # a per-turn timeout can cancel a runaway turn. Maps session_id -> | ||
| # (driving asyncio.Task, InterruptController). | ||
| self._active_turns: Dict[str, Tuple[Any, Any]] = {} | ||
| # Issue #2661: fingerprint of the shared secret each authenticated | ||
| # client connected under, so rotating ``auth_token`` can force-close | ||
| # every session stamped with a stale secret (instant credential | ||
|
|
@@ -2738,10 +2743,13 @@ async def _handle_client_message(self, client_id: str, data: Dict[str, Any]) -> | |
|
|
||
| # Build features list - only advertise implemented features | ||
| features = { | ||
| "methods": ["message", "leave"], # abort not implemented | ||
| # Issue #3467: an in-flight turn can now be aborted via the | ||
| # ``abort`` method (or the ``message_abort`` event alias). | ||
| "methods": ["message", "leave", "abort"], | ||
| "events": [ | ||
| EventType.MESSAGE.value, | ||
| EventType.ERROR.value, | ||
| EventType.MESSAGE_ABORT.value, | ||
| ], | ||
| } | ||
|
|
||
|
|
@@ -2961,6 +2969,16 @@ async def _handle_client_message(self, client_id: str, data: Dict[str, Any]) -> | |
| session = self._sessions.get(session_id) | ||
| if session: | ||
| content = data.get("content", "") | ||
| # Issue #3467: portable stop command. A chat/operator client | ||
| # can abort the in-flight turn by sending "/stop" (or "stop") | ||
| # instead of a dedicated abort frame. | ||
| if isinstance(content, str) and content.strip().lower() in ("/stop", "stop"): | ||
| aborted = self._abort_active_turn(session_id, reason="user") | ||
| await self._send_to_client(client_id, { | ||
| "type": "aborted" if aborted else "no_active_turn", | ||
| "session_id": session_id, | ||
| }) | ||
| return True | ||
| message = GatewayMessage( | ||
| content=content, | ||
| sender_id=client_id, | ||
|
|
@@ -2981,6 +2999,31 @@ async def _handle_client_message(self, client_id: str, data: Dict[str, Any]) -> | |
| "message": "Not joined to any session", | ||
| }) | ||
|
|
||
| elif msg_type in ("abort", EventType.MESSAGE_ABORT.value): | ||
| # Issue #3467: cancel the in-flight turn for this client's session. | ||
| # Requires the WRITE scope (same as sending a message as the agent). | ||
| if not self._client_has_scope(client_id, OperatorScope.WRITE): | ||
| await self._send_to_client(client_id, { | ||
| "type": "error", | ||
| "code": "insufficient_scope", | ||
| "message": "insufficient scope", | ||
| "required_scope": OperatorScope.WRITE.value, | ||
| }) | ||
| return True | ||
| session_id = self._client_sessions.get(client_id) | ||
| if not session_id: | ||
| await self._send_to_client(client_id, { | ||
| "type": "error", | ||
| "message": "Not joined to any session", | ||
| }) | ||
| return True | ||
| reason = data.get("reason") or "user" | ||
| aborted = self._abort_active_turn(session_id, reason=str(reason)) | ||
| await self._send_to_client(client_id, { | ||
| "type": "aborted" if aborted else "no_active_turn", | ||
| "session_id": session_id, | ||
| }) | ||
|
|
||
| elif msg_type == "leave": | ||
| session_id = self._client_sessions.pop(client_id, None) | ||
| if session_id: | ||
|
|
@@ -3031,22 +3074,103 @@ async def _process_agent_message( | |
| return "Started processing." | ||
|
|
||
| @staticmethod | ||
| async def _dispatch_agent_turn(agent: Any, content: str) -> Any: | ||
| async def _dispatch_agent_turn( | ||
| agent: Any, content: str, interrupt: Any = None | ||
| ) -> Any: | ||
| """Execute a single agent turn. | ||
|
|
||
| Prefers the agent's native async entry point (``arun``/``achat``) so | ||
| turns run directly on the event loop, giving native asyncio concurrency, | ||
| cleaner cancellation/timeout and true async streaming. Falls back to | ||
| offloading the synchronous ``chat`` onto the default thread pool only | ||
| when no async entry point is available (sync-only agents). | ||
|
|
||
| Issue #3467: when ``interrupt`` is supplied it is stamped onto the agent | ||
| (``agent.interrupt_controller``) so the agent's run loop cooperatively | ||
| stops on the next checkpoint; the caller also cancels the driving task | ||
| for a hard stop when the agent does not yield promptly. | ||
|
greptile-apps[bot] marked this conversation as resolved.
Outdated
|
||
| """ | ||
| if interrupt is not None and hasattr(agent, "interrupt_controller"): | ||
| agent.interrupt_controller = interrupt | ||
| for _name in ("arun", "achat"): | ||
| _fn = getattr(agent, _name, None) | ||
| if _fn is not None and asyncio.iscoroutinefunction(_fn): | ||
| return await _fn(content) | ||
| loop = asyncio.get_running_loop() | ||
| return await loop.run_in_executor(None, agent.chat, content) | ||
|
greptile-apps[bot] marked this conversation as resolved.
Outdated
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift 🧩 Analysis chain🌐 Web query:
💡 Result: No, the Citations:
🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== locate relevant files =="
git ls-files | rg '(^src/praisonai-agents/praisonaiagents/agent/.*\.py$|^src/praisonai-agents/.*/.*\.py$|^src/praisonai-bot/praisonai_bot/gateway/server.py$|^src/praisonai-agents/praisonaiagents/agent/chat_mixin\.py$)' || true
echo
echo "== server.py relevant sections =="
wc -l src/praisonai-bot/praisonai_bot/gateway/server.py
sed -n '3050,3125p' src/praisonai-bot/praisonai_bot/gateway/server.py
echo
sed -n '7485,7580p' src/praisonai-bot/praisonai_bot/gateway/server.py
echo
echo "== agent-related signatures/usages =="
rg -n "class Agent|def arun|def achat|def chat|interrupt_controller|cancel_token|clone_for_channel" src/praisonai-agents src/praisonai-bot/praisonai_bot/gateway/server.py | head -n 200
echo
echo "== relevant agent.py sections =="
# locate agent.py without assuming path
for f in $(git ls-files | rg '/praisonaiagents/agent/agent\.py$'); do
echo "--- $f"
wc -l "$f"
sed -n '1,220p' "$f"
sed -n '350,600p' "$f"
done
echo
echo "== chat_mixin.py relevant sections =="
f=$(git ls-files | rg '/chat_mixin\.py$' | head -n1 || true)
if [ -n "$f" ]; then
echo "--- $f"
wc -l "$f"
sed -n '1,260p' "$f"
fiRepository: MervinPraison/PraisonAI Length of output: 50380 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== server dispatch/agent resolution =="
wc -l src/praisonai-bot/praisonai_bot/gateway/server.py
sed -n '3000,3105p' src/praisonai-bot/praisonai_bot/gateway/server.py
rg -n "_process_agent_message|_dispatch_agent_turn|self\\._agents\\.get|agent\\.interrupt_controller|agent\\.clone_for_channel" src/praisonai-bot/praisonai_bot/gateway/server.py | head -n 80
echo
echo "== agent.py class/init/run/chat/signatures =="
wc -l src/praisonai-agents/praisonaiagents/agent/agent.py
rg -n "class Agent|def run\\(|def run_async\\(|def chat\\(|def arun\\(|def achat\\(|def clone\\(|def clone_for_channel\\(|interrupt_controller|cancel_token" src/praisonai-agents/praisonaiagents/agent/agent.py src/praisonai-agents/praisonaiagents/agent/interrupt.py src/praisonai-agents/praisonaiagents/agent/chat_mixin.py | head -n 160
echo
echo "== focused agent.py sections =="
sed -n '1,360p' src/praisonai-agents/praisonaiagents/agent/agent.py
sed -n '360,740p' src/src/praisonai-agents/praisonaiagents/agent/agent.py 2>/dev/null || sed -n '360,740p' src/praisonai-agents/praisonaiagents/agent/agent.py
sed -n '740,1140p' src/praisonai-agents/praisonaiagents/agent/agent.py
echo
echo "== focused mixin/interrupt sections =="
sed -n '1,320p' src/praisonai-agents/praisonaiagents/agent/chat_mixin.py
sed -n '1,220p' src/praisonai-agents/praisonaiagents/agent/interrupt.pyRepository: MervinPraison/PraisonAI Length of output: 50381 Avoid mutating the shared agent’s
🤖 Prompt for AI AgentsSource: Coding guidelines |
||
|
|
||
| def _abort_active_turn(self, session_id: str, reason: str = "user") -> bool: | ||
| """Signal and cancel the in-flight turn for ``session_id``, if any. | ||
|
|
||
| Cooperatively requests interruption via the turn's ``InterruptController`` | ||
| (so the agent stops at its next checkpoint and preserves partial output) | ||
| and cancels the driving ``asyncio.Task`` so a stuck turn is torn down. | ||
| Returns ``True`` when a turn was active and an abort was signalled. | ||
| """ | ||
| entry = self._active_turns.get(session_id) | ||
| if entry is None: | ||
| return False | ||
| task, controller = entry | ||
| try: | ||
| if controller is not None: | ||
| controller.request(reason) | ||
| except Exception: | ||
| pass | ||
| try: | ||
| if task is not None and not task.done(): | ||
| task.cancel() | ||
| except Exception: | ||
| pass | ||
| return True | ||
|
|
||
| async def _drive_turn( | ||
| self, | ||
| session: GatewaySession, | ||
| agent: Any, | ||
| content: str, | ||
| controller: Any, | ||
| timeout: float, | ||
| ) -> Any: | ||
| """Run one agent turn cancellably and under an optional per-turn timeout. | ||
|
|
||
| Registers the driving task in ``_active_turns`` so ``_abort_active_turn`` | ||
| can interrupt it, then awaits it with ``asyncio.wait_for`` when a | ||
| positive ``timeout`` is configured. A cancelled or timed-out turn is | ||
| normalised to a terminal string rather than left hanging or surfaced as | ||
| a raw traceback. | ||
| """ | ||
| sid = session.session_id | ||
| task = asyncio.ensure_future( | ||
| self._dispatch_agent_turn(agent, content, interrupt=controller) | ||
| ) | ||
| self._active_turns[sid] = (task, controller) | ||
| try: | ||
| if timeout and timeout > 0: | ||
| try: | ||
| return await asyncio.wait_for(task, timeout=timeout) | ||
| except asyncio.TimeoutError: | ||
| controller.request("timeout") | ||
| task.cancel() | ||
| try: | ||
| await task | ||
| except (asyncio.CancelledError, Exception): | ||
| pass | ||
| return self._finalise_aborted_turn(controller, "timeout") | ||
| return await task | ||
| except asyncio.CancelledError: | ||
| reason = controller.reason or "user" | ||
| return self._finalise_aborted_turn(controller, reason) | ||
| finally: | ||
| existing = self._active_turns.get(sid) | ||
| if existing is not None and existing[0] is task: | ||
| self._active_turns.pop(sid, None) | ||
|
|
||
|
Comment on lines
+3176
to
+3222
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🌐 Web query:
💡 Result: Yes, the InterruptController in praisonaiagents.agent.interrupt utilizes an is_set method, typically accessed via its internal cancellation attributes [1]. According to official PraisonAI documentation, the InterruptController is used for cooperative cancellation [1]. When passed as a cancel_token to agent methods like chat, the agent checks for an interrupt signal at various checkpoints [1]. Documentation explicitly notes that when this token's is_set returns true, an InterruptedError is raised [1]. While the exact implementation details can vary across versions, common patterns in this framework involve the InterruptController managing an internal state (often utilizing threading.Event or similar mechanisms) where is_set is used to verify if an interruption has been requested [1][2][3]. Citations:
🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== locate files =="
git ls-files | rg 'src/praisonai-bot/praisonai_bot/gateway/server.py|InterruptController|interrupt' | head -100
echo
echo "== server excerpt =="
sed -n '2980,3190p' src/praisonai-bot/praisonai_bot/gateway/server.py
echo
echo "== interrupt-related usages in server =="
rg -n "_drive_turn|_finalise_aborted_turn|_active_turns|_abort_active_turn|InterruptController|controller\.is_set|controller\.request|reason or \"user\"" src/praisonai-bot/praisonai_bot/gateway/server.py
echo
echo "== InterruptController definitions/usages in repo and installed package if available =="
rg -n "class InterruptController|def is_set|is_set\\(" . -g '*.py' | head -200
python3 - <<'PY'
try:
import praisonaiagents.agent.interrupt as mod
print("package imported ok")
for name in dir(mod):
if "Interrupt" in name:
cls = getattr(mod, name)
print(name, type(cls), hasattr(cls, "is_set"))
except Exception as e:
print("import failed:", type(e).__name__, e)
PYRepository: MervinPraison/PraisonAI Length of output: 15971 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== server queue excerpt =="
sed -n '3188,3230p' src/praisonai-bot/praisonai_bot/gateway/server.py
echo
echo "== InterruptController implementation =="
sed -n '1,115p' src/praisonai-agents/praisonaiagents/agent/interrupt.py
echo
echo "== InterruptController tests =="
sed -n '1,135p' src/praisonai-agents/tests/unit/agent/test_interrupt.py
echo
echo "== asyncio cancellation behavior probe for await CancelledError catch =="
python3 - <<'PY'
import asyncio
async def cancels_await():
for i in range(3):
await a_task()
print("loop", i)
async def a_task():
raise asyncio.CancelledError("example external cancellation")
try:
asyncio.get_event_loop().run_until_complete(cancels_await())
except asyncio.CancelledError as e:
print("external cancellation propagated:", e.args)
else:
print("external cancellation swallowed")
PYRepository: MervinPraison/PraisonAI Length of output: 10013 Propagate external cancellations in At lines 3161-3162, the current 🧰 Tools🪛 Ruff (0.15.21)[error] 3156-3157: (S110) [warning] 3156-3156: Do not catch blind exception: (BLE001) 🤖 Prompt for AI Agents |
||
| def _finalise_aborted_turn(self, controller: Any, reason: str) -> str: | ||
| """Return a typed terminal message for an interrupted/timed-out turn.""" | ||
| if reason == "timeout": | ||
| return "Turn cancelled: exceeded per-turn timeout." | ||
| return f"Turn cancelled: {reason}." | ||
|
|
||
| async def _run_session_queue(self, session: GatewaySession, agent: Any, client_id: str) -> None: | ||
| """Background task loop that constantly pulls from `_inbox` and executes the agent task.""" | ||
| try: | ||
|
|
@@ -3062,6 +3186,13 @@ async def _run_session_queue(self, session: GatewaySession, agent: Any, client_i | |
| relay_callback = self._make_stream_relay(client_id, session) | ||
| emitter.add_callback(relay_callback) | ||
|
|
||
| # Issue #3467: run the turn under a cancel scope so it can be | ||
| # aborted (WS ``abort`` / ``/stop``) and time-bounded. The | ||
| # controller is checked by the agent's run loop; cancelling the | ||
| # task tears down a turn that does not yield promptly. | ||
| from praisonaiagents.agent.interrupt import InterruptController | ||
| controller = InterruptController() | ||
| timeout = getattr(self.config, "per_turn_timeout", 0.0) or 0.0 | ||
| try: | ||
| gate = getattr(self, "_admission_gate", None) | ||
| if gate is not None and getattr(gate, "enabled", False): | ||
|
|
@@ -3071,13 +3202,15 @@ async def _run_session_queue(self, session: GatewaySession, agent: Any, client_i | |
| from ..bots._admission import AdmissionRejected | ||
| try: | ||
| async with gate.admit(session_id=session.session_id): | ||
| response = await self._dispatch_agent_turn( | ||
| agent, content | ||
| response = await self._drive_turn( | ||
| session, agent, content, controller, timeout | ||
| ) | ||
| except AdmissionRejected as rej: | ||
| response = rej.message | ||
| else: | ||
| response = await self._dispatch_agent_turn(agent, content) | ||
| response = await self._drive_turn( | ||
| session, agent, content, controller, timeout | ||
| ) | ||
| except Exception as e: | ||
| logger.error(f"Agent error in queue processor: {e}") | ||
| response = f"Error: {str(e)}" | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.