fix: session_id collisions, concurrent handoff corruption, async retry hook crash - #3398
Conversation
|
@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 Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR isolates handoff chains per async context, adds random suffixes to generated memory session IDs, and introduces an asynchronous ChangesAgent execution updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Greptile SummaryThis PR applies three focused reliability fixes in the core SDK.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains, and the previously reported inherited handoff-chain sharing issue is addressed by rebinding a copied chain within each task scheduled by
|
| Filename | Overview |
|---|---|
| src/praisonai-agents/praisonaiagents/agent/handoff.py | Copies the inherited handoff chain inside each coroutine scheduled by asyncio.gather, resolving the previously reported mutable-context sharing issue. |
| src/praisonai-agents/praisonaiagents/agent/memory_mixin.py | Adds a random per-instance suffix to automatically generated session identifiers. |
| src/praisonai-agents/praisonaiagents/hooks/runner.py | Adds an asynchronous alias that delegates to the existing hook execution implementation. |
Reviews (3): Last reviewed commit: "fix(handoff): isolate handoff chain per ..." | Re-trigger Greptile
![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 Final Architecture Review — PR #3398 ✅Verdict: APPROVE (fixed the one valid blocking issue; pushed to this branch)
Phase 1 — Review of the three fixesGap 1 — Gap 2 — concurrent handoff corruption ( Gap 3 — async retry hook crash ( Phase 2 — Fix implemented (pushed:
|
![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
🤖 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/handoff.py`:
- Around line 167-181: Update _handoff_chain_var and _get_handoff_chain to store
immutable tuple values, using None or an empty tuple for cleared state. Replace
in-place push, pop, and clear mutations throughout the handoff flow with
assignments of newly built tuples, and return a fresh list when constructing
HandoffInputData so tasks cannot share mutable chain state.
In `@src/praisonai-agents/praisonaiagents/agent/memory_mixin.py`:
- Around line 243-261: Update session ID generation in both lock branches to use
the full UUID suffix instead of truncating uuid.uuid4().hex to eight characters.
Preserve the existing hour and agent_hash components, and verify storage or
routing consumers accept the resulting longer session IDs without shortening
them.
🪄 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 Plus
Run ID: 84b0c8ee-8489-4450-8c0d-943f606f7989
📒 Files selected for processing (3)
src/praisonai-agents/praisonaiagents/agent/handoff.pysrc/praisonai-agents/praisonaiagents/agent/memory_mixin.pysrc/praisonai-agents/praisonaiagents/hooks/runner.py
| # Context-local storage for tracking handoff chains. | ||
| # Uses contextvars (not threading.local) so state is correctly isolated per | ||
| # asyncio.Task across await points — required for parallel_handoffs, which runs | ||
| # multiple handoffs concurrently on the same event-loop thread via asyncio.gather. | ||
| _handoff_chain_var: contextvars.ContextVar[Optional[List[str]]] = contextvars.ContextVar( | ||
| 'handoff_chain', default=None | ||
| ) | ||
|
|
||
| def _get_handoff_chain() -> List[str]: | ||
| """Get current handoff chain from thread-local storage.""" | ||
| if not hasattr(_handoff_context, 'chain'): | ||
| _handoff_context.chain = [] | ||
| return _handoff_context.chain | ||
| """Get current handoff chain from context-local storage.""" | ||
| chain = _handoff_chain_var.get() | ||
| if chain is None: | ||
| chain = [] | ||
| _handoff_chain_var.set(chain) | ||
| return chain |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
python - <<'PY'
import asyncio
import contextvars
chain = contextvars.ContextVar("chain", default=None)
def push(value):
current = chain.get()
if current is None:
current = []
chain.set(current)
current.append(value)
async def child(value):
push(value)
await asyncio.sleep(0)
return list(chain.get())
async def main():
chain.set(["parent"])
first, second = await asyncio.gather(child("first"), child("second"))
assert first == ["parent", "first"], first
assert second == ["parent", "second"], second
asyncio.run(main())
PY
# The current mutable-list pattern should fail these assertions.Repository: MervinPraison/PraisonAI
Length of output: 737
🏁 Script executed:
#!/bin/bash
set -eu
# Locate and inspect the relevant handoff implementation without executing repository code.
if [ -f src/praisonai-agents/praisonaiagents/agent/handoff.py ]; then
wc -l src/praisonai-agents/praisonaiagents/agent/handoff.py
sed -n '1,280p' src/praisonai-agents/praisonaiagents/agent/handoff.py | cat -n
fiRepository: MervinPraison/PraisonAI
Length of output: 12506
🏁 Script executed:
#!/bin/bash
set -eu
# Find all call sites of the handoff chain helpers and identify concurrent/parallel paths.
rg -n "_get_handoff_chain|_push_handoff|_pop_handoff|_clear_handoff_chain|parallel_handoffs|asyncio\.gather|allow_parallel" src/praisonai-agents/praisonaiagents/agent/handoff.py src/praisonai-agents/praisonaiagents/agent -g '*.py'Repository: MervinPraison/PraisonAI
Length of output: 6091
🏁 Script executed:
#!/bin/bash
set -eu
# Inspect the parallel execution path and any clear/pop behavior around those calls.
sed -n '1100,1230p' src/praisonai-agents/praisonaiagents/agent/handoff.py | cat -n
sed -n '1320,1590p' src/praisonai-agents/praisonaiagents/agent/handoff.py | cat -nRepository: MervinPraison/PraisonAI
Length of output: 19693
Store an immutable handoff chain in the ContextVar.
ContextVar copies the mapping into each asyncio.Task, but mutable values are shared by identity. If one handoff task mutates the shared list, sibling tasks can see corrupted depth/cycle data or crash when popping. Keep the stored value immutable (None/empty tuple) and build a new tuple for every push/pop/clear; return a fresh list when the chain is passed into HandoffInputData.
🤖 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/handoff.py` around lines 167 -
181, Update _handoff_chain_var and _get_handoff_chain to store immutable tuple
values, using None or an empty tuple for cleared state. Replace in-place push,
pop, and clear mutations throughout the handoff flow with assignments of newly
built tuples, and return a fresh list when constructing HandoffInputData so
tasks cannot share mutable chain state.
Source: Coding guidelines
| import uuid | ||
| from datetime import datetime, timezone | ||
| with self._history_lock._lock._thread_lock: | ||
| if self._session_id is None: # Double-check after acquiring lock | ||
| hour_str = datetime.now(timezone.utc).strftime("%Y%m%d%H") | ||
| agent_hash = hashlib.sha256((self.name or "agent").encode()).hexdigest()[:6] | ||
| self._session_id = f"{hour_str}-{agent_hash}" | ||
| # per-instance suffix so same-named agents can never collide | ||
| self._session_id = f"{hour_str}-{agent_hash}-{uuid.uuid4().hex[:8]}" | ||
| else: | ||
| # Use sync lock directly | ||
| import hashlib | ||
| import uuid | ||
| from datetime import datetime, timezone | ||
| with self._history_lock.lock(): | ||
| if self._session_id is None: # Double-check after acquiring lock | ||
| hour_str = datetime.now(timezone.utc).strftime("%Y%m%d%H") | ||
| agent_hash = hashlib.sha256((self.name or "agent").encode()).hexdigest()[:6] | ||
| self._session_id = f"{hour_str}-{agent_hash}" | ||
| # per-instance suffix so same-named agents can never collide | ||
| self._session_id = f"{hour_str}-{agent_hash}-{uuid.uuid4().hex[:8]}" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
rg -n --glob '*.py' \
'session_id|uuid\.uuid4|hour_str|agent_hash|YYYYMMDDHH|\[-8:|split\(["'\'']-["'\'']\)' \
.Repository: MervinPraison/PraisonAI
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -eu
echo "== relevant files =="
git ls-files | rg '(^|/)memory_mixin\.py$|(^|/)factories\.py$|(^|/)store\.py$' | sed 's#^\./##'
echo
echo "== memory_mixin context =="
sed -n '220,275p' src/praisonai-agents/praisonaiagents/agent/memory_mixin.py
echo
echo "== factories context =="
sed -n '600,670p' src/praisonaiagRepository: MervinPraison/PraisonAI
Length of output: 3443
🏁 Script executed:
#!/bin/bash
set -eu
echo "== relevant files =="
git ls-files | rg '(^|/)memory_mixin\.py$|(^|/)factories\.py$|(^|/)store\.py$' | sed 's#^\./##'
echo
echo "== memory_mixin context =="
sed -n '220,275p' src/praisonai-agents/praisonaiagents/agent/memory_mixin.py
echo
echo "== factories context =="
sed -n '600,670p' src/praisonai-agents/praisonaiagents/memory/adapters/factories.py
echo
echo "== session store path handling =="
sed -n '610,635p' src/praisonai-agents/praisonaiagents/session/store.py
echo
echo "== narrow session_id generator occurrences =="
rg -n --glob '*.py' '(_session_id|history_session_id|MemoryConfig|session_id=) = .*uuid\.uuid4\(\)\.hex\[:8\]|uuid\.uuid4\(\)\.hex\[:8\].*session_id|session_id.*uuid\.uuid4\(\)\.hex\[:8\]' src/praisonai-agentsRepository: MervinPraison/PraisonAI
Length of output: 8467
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from math import log2
slots = 16**8
avg_after = (slots // 2) * (1 - ((1 - 1/slots) ** slots))
birthday_50 = int((2 * slots * (2**0.5 * log2(2)) )**0.5) # birthday approx for 50%
print("slots", slots)
print("avg occupied after one million ids", avg_after)
print("half-birthday threshold", birthday_50)
PY
echo
echo "== memory adapter routing/session_id forwarding =="
rg -n --glob '*.py' 'on_agent_start|session_id|session-id|routing|route' src/praisonai-agents/praisonaiagents/memory src/praisonaiagents/praisonaiagents/memory 2>/dev/null || true
echo
echo "== targeted session_id pass-throughs =="
rg -n --glob '*.py' 'session_id=self\._session_id|kwargs\.get\("session_id"|session-id=|route.*session|session.*route' src/praisonai-agents/praisonaiagents src/praisonai 2>/dev/null || trueRepository: MervinPraison/PraisonAI
Length of output: 7832
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from math import log2
slots = 16**8
avg_after = slots * (1 - (1 - 1/slots) ** 1_000_000)
birthday_50 = int((2 * slots * (2**0.5 * log2(2)))**0.5)
print("slots", slots)
print("avg occupied after one million ids", avg_after)
print("half-birthday threshold", birthday_50)
PY
echo
echo "== targeted session_id pass-throughs =="
rg -n --glob '*.py' 'self\._db\.on_agent_start|on_agent_start|session_id=self\._session_id|kwargs\.get\("session_id"|session-id=|route.*session|session.*route' src/praisonai-agents/src/praisonai-agents/praisonaiagents/praisonai 2>/dev/null || trueRepository: MervinPraison/PraisonAI
Length of output: 302
🏁 Script executed:
#!/bin/bash
set -eu
echo "== session store routing methods =="
sed -n '180,215p' src/praisonai-agents/praisonaiagents/session/sqlite_store.py
sed -n '390,422p' src/praisonai-agents/praisonaiagents/session/sqlite_store.py
sed -n '425,443p' src/praisonai-agents/praisonaiagents/session/sqlite_store.py
echo
echo "== memory mixin calls =="
sed -n '330,375p' src/praisonai-agents/praisonaiagents/agent/memory_mixin.py
echo
echo "== focused session routing/session_id usage =="
rg -n --glob '*.py' 'session_id=|gateway_session_id|get_by_gateway_session|route.*session|save_session|_save_session|save_session' \
src/praisonai-agents/praisonaiagents/agent src/praisonai-agents/praisonaiagents/session src/praisonai-agents/praisonaiagents/memory src/praisonai/praisonai/persistence/hooks/agent_hooks.py \
| sed -n '1,220p'Repository: MervinPraison/PraisonAI
Length of output: 20768
Use the full UUID suffix for session IDs.
uuid.uuid4().hex[:8] only provides 32 bits of entropy; with millions of agents or runs sharing the same yearmonthhouroffilename prefix, collisions become likely in bounded-session scenarios. These IDs are used for persistence/storage routing, so a collision can merge or expose another agent’s history. Use the full UUID in both generation paths and verify no storage/routing consumer requires shortening.
[low_effort_and_medium_reward]
🤖 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/memory_mixin.py` around lines 243
- 261, Update session ID generation in both lock branches to use the full UUID
suffix instead of truncating uuid.uuid4().hex to eight characters. Preserve the
existing hour and agent_hash components, and verify storage or routing consumers
accept the resulting longer session IDs without shortening them.
praisonai-triage-agent
Bot
removed
pipeline/blocked:ci
|
@claude this PR has merge conflicts with |
|
Claude finished @MervinPraison's task —— View job Rebase & Conflict Resolution — PR #3398 ✅
Files resolved1 conflicted file:
No conflicts in the other two PR files — both intents preserved after rebase:
No Agent params added; resolution stayed minimal (no scope creep). Testing
Skipped
Branch force-pushed: --- • View job • |
…y hook crash (fixes #3397) - memory_mixin: append per-instance uuid suffix to default session_id so same-named agents can no longer collide and bleed chat history - handoff: replace threading.local() with contextvars.ContextVar so handoff chain/depth is isolated per asyncio.Task under parallel_handoffs - hooks/runner: add HookRunner.execute_async alias so async retry-with-backoff no longer crashes with AttributeError Co-authored-by: MervinPraison <MervinPraison@users.noreply.github.com>
parallel_handoffs runs children via asyncio.gather, which copies the context per task. A copied ContextVar binding still references the same parent list object, so when parallel_handoffs runs inside a non-empty parent handoff chain, sibling tasks mutated a shared list and corrupted cycle/depth tracking. Rebind each task's chain to a fresh per-task copy at _run_one entry so siblings are fully isolated. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
08b794b to
b3349e1
Compare
MervinPraison
added
pipeline/blocked:ci
praisonai-triage-agent
Bot
removed
pipeline/blocked:ci
1 participant
Fixes #3397
Summary
Three minimal, focused fixes in the
praisonaiagentscore SDK, each independently verified against the current source:Gap 1 —
session_idcollision (agent/memory_mixin.py)Default
session_idwassha256(agent.name)[:6]+ UTC hour, so two same-named agents (e.g. two unnamedAgent()s → name"Agent") in the same hour computed an identical id and bled chat history viaon_agent_start. Appended a per-instanceuuid4().hex[:8]suffix so collisions are impossible.Gap 2 — concurrent handoff corruption (
agent/handoff.py)threading.local()isolates per OS thread, not perasyncio.Task, soparallel_handoffs(which usesasyncio.gatheron one event-loop thread) shared a single handoff chain — corrupting cycle/depth detection. Replaced withcontextvars.ContextVar, which isolates per Task across await points (mirrors the existing pattern intools/injected.py).Gap 3 — async retry hook crash (
hooks/runner.py)chat_mixin.py's async retry path callsself._hook_runner.execute_async(...), butHookRunnerhad no such method →AttributeErroron every retryable async error, breaking retry/backoff. Addedexecute_asyncas an async alias ofexecute().Testing
asyncio.Taskisolation of the handoff chain (depths all1undergather; would be1..5with the old thread-local).HookRunner.execute_asyncexists and is a coroutine.test_handoff_tool_policy.pyfailures are unrelated (reproduce on the base commit before these changes).Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes