fix: async lifecycle hooks + locked display-callback swap#3174
fix: async lifecycle hooks + locked display-callback swap#3174praisonai-triage-agent[bot] wants to merge 2 commits into
Conversation
Gap 1: arun_task() now fires on_task_start/on_task_complete and propagates self.variables, matching run_task(). Extracted _run_task_start_hook and _run_task_complete_hook shared helpers so both sync and async paths stay in sync (mirrors the _apply_task_guardrail helper pattern). Gap 3: verbose+TTY status animation now swaps and restores the shared sync_display_callbacks['tool_call'] entry under main.py's existing _callbacks_lock, with an identity check on restore so concurrent verbose agents don't clobber each other's entry. Gap 2 (Memory adapter-protocol bypass) intentionally deferred: routing 34 vendor-specific call sites through the protocol is a large architectural refactor with high regression risk, out of scope for a minimal fix. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
|
@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? |
MervinPraison
added
pipeline/blocked:ci
✅ Action performedReview finished.
|
|
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:
📝 WalkthroughWalkthroughChangesTask lifecycle handling
Display callback synchronization
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/praisonai-agents/praisonaiagents/agents/agents.py (1)
1130-1132: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPrevent shared mutable state across tasks.
Assigning
self.variablesdirectly totask.variablescreates a shared dictionary reference. If a task mutates its variables during execution, it will unintentionally mutate the globalAgentTeam.variablesand affect all other tasks in the workflow.Consider using a shallow copy to isolate task variables.
As per coding guidelines, "Avoid shared mutable global state between agents; isolate agent context, memory, sessions, and resources".
♻️ Proposed fix
- if self.variables and not getattr(task, 'variables', None): - task.variables = self.variables + if self.variables and not getattr(task, 'variables', None): + task.variables = self.variables.copy()🤖 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/agents/agents.py` around lines 1130 - 1132, Update the global-variable assignment in the task setup flow to give each task an independent shallow copy of self.variables rather than the shared dictionary reference. Preserve the existing condition that only initializes task.variables when it is unset, and leave explicitly configured task variables unchanged.Source: Coding guidelines
🤖 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/agent/execution_mixin.py`:
- Around line 808-815: Update the callback restoration block under the lock so
the entire restoration logic is guarded by the identity check against
status_tool_callback. Restore original_tool_callback only when the current
callback is still status_tool_callback; otherwise leave the concurrently
installed callback unchanged, while preserving deletion when no original
callback exists.
In `@src/praisonai-agents/praisonaiagents/agents/agents.py`:
- Around line 1118-1141: Add async lifecycle-hook helpers alongside
_run_task_start_hook and _run_task_complete_hook, and update arun_task to use
them. Detect coroutine callbacks with asyncio.iscoroutinefunction() and await
them; run synchronous callbacks through the event loop’s executor so blocking
I/O does not block arun_task. Preserve the existing exception logging and
variable propagation behavior.
---
Nitpick comments:
In `@src/praisonai-agents/praisonaiagents/agents/agents.py`:
- Around line 1130-1132: Update the global-variable assignment in the task setup
flow to give each task an independent shallow copy of self.variables rather than
the shared dictionary reference. Preserve the existing condition that only
initializes task.variables when it is unset, and leave explicitly configured
task variables unchanged.
🪄 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: 45abbc4f-6996-4cfd-9300-ec4812ac9a82
📒 Files selected for processing (2)
src/praisonai-agents/praisonaiagents/agent/execution_mixin.pysrc/praisonai-agents/praisonaiagents/agents/agents.py
| # Restore original callback under the lock. The identity | ||
| # check ensures we only touch the entry if it's still ours, | ||
| # so a concurrent verbose agent isn't clobbered. | ||
| with _callbacks_lock: | ||
| if original_tool_callback: | ||
| sync_display_callbacks['tool_call'] = original_tool_callback | ||
| elif sync_display_callbacks.get('tool_call') is status_tool_callback: | ||
| del sync_display_callbacks['tool_call'] |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Identity check bypassed during callback restoration.
If original_tool_callback is truthy, it is restored unconditionally, ignoring the identity check. This will clobber the active callback if a concurrent thread has overwritten sync_display_callbacks['tool_call'] in the meantime.
The identity check must guard the entire restoration logic, not just the deletion step.
🔒️ Proposed fix to nest the restoration logic under the identity check
- # Restore original callback under the lock. The identity
- # check ensures we only touch the entry if it's still ours,
- # so a concurrent verbose agent isn't clobbered.
with _callbacks_lock:
- if original_tool_callback:
- sync_display_callbacks['tool_call'] = original_tool_callback
- elif sync_display_callbacks.get('tool_call') is status_tool_callback:
- del sync_display_callbacks['tool_call']
+ if sync_display_callbacks.get('tool_call') is status_tool_callback:
+ if original_tool_callback:
+ sync_display_callbacks['tool_call'] = original_tool_callback
+ else:
+ del sync_display_callbacks['tool_call']📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Restore original callback under the lock. The identity | |
| # check ensures we only touch the entry if it's still ours, | |
| # so a concurrent verbose agent isn't clobbered. | |
| with _callbacks_lock: | |
| if original_tool_callback: | |
| sync_display_callbacks['tool_call'] = original_tool_callback | |
| elif sync_display_callbacks.get('tool_call') is status_tool_callback: | |
| del sync_display_callbacks['tool_call'] | |
| with _callbacks_lock: | |
| if sync_display_callbacks.get('tool_call') is status_tool_callback: | |
| if original_tool_callback: | |
| sync_display_callbacks['tool_call'] = original_tool_callback | |
| else: | |
| del sync_display_callbacks['tool_call'] |
🤖 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/agent/execution_mixin.py` around lines
808 - 815, Update the callback restoration block under the lock so the entire
restoration logic is guarded by the identity check against status_tool_callback.
Restore original_tool_callback only when the current callback is still
status_tool_callback; otherwise leave the concurrently installed callback
unchanged, while preserving deletion when no original callback exists.
| def _run_task_start_hook(self, task, task_id): | ||
| """Run the on_task_start hook and propagate global variables to the task. | ||
|
|
||
| Shared by run_task (sync) and arun_task (async) so the lifecycle hooks | ||
| and variable propagation stay consistent across both paths. | ||
| """ | ||
| if self.on_task_start: | ||
| try: | ||
| self.on_task_start(task, task_id) | ||
| except Exception as e: | ||
| logger.error(f"Error in on_task_start callback: {e}") | ||
|
|
||
| # Apply global variables to task if not already set | ||
| if self.variables and not getattr(task, 'variables', None): | ||
| task.variables = self.variables | ||
|
|
||
| def _run_task_complete_hook(self, task, task_output): | ||
| """Run the on_task_complete hook. Shared by run_task and arun_task.""" | ||
| if self.on_task_complete: | ||
| try: | ||
| self.on_task_complete(task, task_output) | ||
| except Exception as e: | ||
| logger.error(f"Error in on_task_complete callback: {e}") | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Support asynchronous lifecycle callbacks and prevent event loop blocking.
By sharing synchronous helpers between run_task and arun_task, two issues arise for async execution:
- If a user provides an asynchronous callback (
async def), it will return a coroutine that is never awaited, causing the hook to silently fail and emit aRuntimeWarning. - If a user provides a synchronous callback that performs I/O, executing it directly inside
arun_taskwill block the event loop.
Unlike Task.execute_callback() which is properly awaited natively, these hooks lack async-aware execution. Consider adding async variants of these helpers for arun_task that use asyncio.iscoroutinefunction() to await async hooks, and use loop.run_in_executor() to safely execute synchronous blocking hooks.
As per coding guidelines, "All I/O operations must provide async variants; never block the event loop with synchronous I/O".
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 1127-1127: Do not catch blind exception: Exception
(BLE001)
[warning] 1139-1139: Do not catch blind exception: Exception
(BLE001)
🤖 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/agents/agents.py` around lines 1118 -
1141, Add async lifecycle-hook helpers alongside _run_task_start_hook and
_run_task_complete_hook, and update arun_task to use them. Detect coroutine
callbacks with asyncio.iscoroutinefunction() and await them; run synchronous
callbacks through the event loop’s executor so blocking I/O does not block
arun_task. Preserve the existing exception logging and variable propagation
behavior.
Source: Coding guidelines
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@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 — Final Architecture Review (PR #3174)
Verdict: ✅ Approve after fixes (pushed to this branch)Reviewed against AGENTS.md. The PR is a genuine, lightweight correctness fix — no new Agent params, no new exports, no scope creep. I validated all reviewer findings and fixed the three valid ones. Files modified (this commit)
Skipped (with rationale)
Testing
Lightweight, backward-compatible, no hot-path regression. Good to merge. Job run · Branch |
…ooks - execution_mixin: nest sync_display_callbacks restore fully under the status_tool_callback identity check so a finishing agent can't clobber a concurrent agent's live callback (3+ verbose-TTY agents race). - agents: shallow-copy self.variables into task.variables so tasks can't mutate shared AgentTeam state. - agents: add async-aware _arun_task_start_hook/_arun_task_complete_hook that await coroutine lifecycle callbacks and offload sync ones to the executor, so arun_task never blocks the loop or silently drops an async hook. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
MervinPraison
added
pipeline/blocked:stale-final
praisonai-triage-agent
Bot
removed
the
pipeline/blocked:cooldown
1 participant
Fixes #3173
Summary
Addresses two of the three validated gaps from #3173 with minimal, focused fixes. Gap 2 (Memory adapter-protocol bypass) was intentionally deferred — see rationale below.
Gap 1 —
arun_task()dropped lifecycle hooks & variablesrun_task()(sync) firedon_task_start/on_task_completeand propagatedself.variables;arun_task()(async) did not, so hooks silently never fired forasync_execution=Truetasks in a mixed run._run_task_start_hook()and_run_task_complete_hook()shared helpers (mirroring the existing_apply_task_guardrailpattern from Core SDK: identical guardrail-validation and task-callback blocks duplicated between sync run_task() and async arun_task() #1961).run_task()andarun_task()now call them, so there is a single source of truth — no third hand-synced copy.Gap 3 — unlocked
sync_display_callbacksraceThe verbose+TTY status animation swapped/restored the shared
sync_display_callbacks['tool_call']dict from a background thread without the module's_callbacks_lock, racing concurrent verbose agents._callbacks_lockfrommain.py.is status_tool_callbackidentity check so this thread'sfinallycan't clobber/delete a concurrent agent's entry.Gap 2 — deferred (critical review)
Routing
Memory's 34 vendor-specific call sites (ChromaDB query shapes, mem0 response formats, MongoDB collections) throughMemoryProtocolis a large architectural refactor with high regression risk, well beyond a minimal/lightweight fix. Per AGENTS.md it should be handled as a dedicated, separately-reviewed change rather than bundled here.Test plan
fastapi/litellm, unrelated features), not caused by these changesGenerated with Claude Code
Summary by CodeRabbit