Skip to content

praisonaiagents core: session_id collisions, corrupted concurrent handoffs, and a broken async retry hook #3397

Description

@MervinPraison

Summary

In-depth pass over src/praisonai-agents/praisonaiagents (core SDK only, praisonaiagents package). Scope was restricted to real functional/architectural gaps — not docs, tests, coverage, file size/line count, or generic performance tuning. Four parallel deep-dives covered core orchestration (agent/, task/, process/), the LLM/tools/memory/session layer, package-wide concurrency/async-safety, and the runtime/hooks/plugin layer. Every finding below was independently re-verified by directly reading the current source (file + line numbers quoted below) — none are inferred.

Duplicate check performed first: searched existing issues for session_id, handoff/threading.local, and HookRunner/execute_async. No open or closed issue covers any of the three bugs below (the closest related issues — e.g. the litellm.callbacks global-overwrite bug and general unprotected-global-state audits — are different root causes in different code paths and are intentionally not repeated here).

These three all violate the SDK's own stated hard requirements ("multi-agent safe", "async-safe by default", reliable retry-with-backoff), are each reachable from a documented, supported usage pattern (default agent naming + db-backed memory; parallel_handoffs/concurrent handoff_to_async; retry-configured async chat), and each causes either silent data corruption/leakage or a hard crash — not degraded performance.


Gap 1 — Default session_id generation collides across different Agent instances, bleeding one agent's conversation history into another's

File: praisonaiagents/agent/memory_mixin.py, _init_db_session(), lines 232–280

# memory_mixin.py:237-257
# Generate session_id if not provided: default to per-hour ID (YYYYMMDDHH-agentname)
# Protected by history lock to prevent race condition between concurrent chat() calls
if self._session_id is None:
    ...
    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}"

# memory_mixin.py:260-275
history = self._db.on_agent_start(
    agent_name=self.name,
    session_id=self._session_id,
    user_id=self.user_id,
    metadata={"role": self.role, "goal": self.goal}
)
if history:
    for msg in history:
        self.chat_history.append({"role": msg.role, "content": msg.content})

self._session_id is derived only from sha256(agent.name)[:6] + the current UTC hour — no run id, no process id, no randomness. The default agent name is the literal string "Agent" (agent/agent.py:1570: self.name = name or "Agent").

Failure scenario: any two Agent() instances with the same name (trivially true for any two unnamed agents, or any worker pool that names agents by role, e.g. Agent(name="Researcher", memory=db(...))) created within the same UTC hour compute the identical session_id. When the second agent's _init_db_session() runs, on_agent_start(agent_name=self.name, session_id=self._session_id, ...) returns the first agent's prior messages, and they're appended straight into the second agent's chat_history — silent cross-agent context bleed, and both agents continue writing turns into the same DB session going forward. This is a correctness/data-leakage bug, not an edge case: it's the default behavior whenever a db-backed memory adapter is used without an explicit session_id.

Fix:

import uuid
if self._session_id is None:
    with self._history_lock.lock():
        if self._session_id is None:
            hour_str = datetime.now(timezone.utc).strftime("%Y%m%d%H")
            agent_hash = hashlib.sha256((self.name or "agent").encode()).hexdigest()[:6]
            # per-instance component so same-named agents can never collide
            self._session_id = f"{hour_str}-{agent_hash}-{uuid.uuid4().hex[:8]}"

Longer-term, consider requiring callers to pass an explicit session_id when a db adapter is configured (raise/warn instead of silently manufacturing a collidable default), since a human-readable-but-collidable ID is inherently unsafe for this purpose.

Confidence: CONFIRMED — traced agent.name default → hash/hour derivation → on_agent_start call → unconditional chat_history.append merge.


Gap 2 — threading.local() used for handoff cycle/depth tracking is corrupted by parallel_handoffs' own asyncio.gather

File: praisonaiagents/agent/handoff.py, lines 167–192 (state), 710–763 (usage in Handoff.execute_async), 1125–1198 (parallel_handoffs)

# handoff.py:167-187
_handoff_context = threading.local()

def _get_handoff_chain() -> List[str]:
    if not hasattr(_handoff_context, 'chain'):
        _handoff_context.chain = []
    return _handoff_context.chain

def _push_handoff(agent_name: str) -> None:
    chain = _get_handoff_chain()
    chain.append(agent_name)

def _pop_handoff() -> Optional[str]:
    chain = _get_handoff_chain()
    return chain.pop() if chain else None
# handoff.py:710-763, inside Handoff.execute_async
async def _execute():
    try:
        self._check_safety(source_agent)
        _push_handoff(source_agent.name)
        ...
        response = await self._execute_with_runtime_resolution_async(   # <-- await point
            source_agent, full_prompt, effective_tools, kwargs
        )
        result = HandoffResult(..., handoff_depth=_get_handoff_depth())
        ...
        return result
    finally:
        _pop_handoff()
# handoff.py:1173-1198, parallel_handoffs — runs multiple _execute()s concurrently
async def _run_one(agent, prompt):
    async def _do_handoff():
        return await source.handoff_to_async(agent, prompt, config=config)
    ...
tasks = [_run_one(agent, prompt) for agent, prompt in targets]
results = await asyncio.gather(*tasks, return_exceptions=True)

threading.local() isolates state per OS thread, not per asyncio.Task. parallel_handoffs is the SDK's own documented API for running several handoffs concurrently, and it does so via asyncio.gather — all tasks run interleaved on the same event-loop thread, so they all share the exact same _handoff_context.chain list.

Failure scenario: parallel_handoffs(source, [(agentA, promptA), (agentB, promptB)]) starts two _execute() coroutines on the same thread. Task A pushes "source" (chain=["source"]), then hits the await self._execute_with_runtime_resolution_async(...) point and yields control. Task B now runs, also pushes "source" (chain=["source", "source"]), and may finish first — its finally: _pop_handoff() does chain.pop(), which is LIFO and removes the last element, which could be either task's push depending on interleaving. This produces: false-positive cycle/depth errors for unrelated concurrent handoff chains, incorrect handoff_depth reported in HandoffResult, and — if a pop ever runs when the chain is already shorter than expected — permanently stranded entries that make _get_handoff_depth() grow across the life of the process, eventually tripping HandoffDepthError for legitimate, non-deep handoffs.

Fix: replace the module-level threading.local() with a contextvars.ContextVar, which correctly isolates state per asyncio.Task (each task gets its own copy across await points) while still working for plain synchronous/threaded use:

import contextvars
_handoff_chain_var: contextvars.ContextVar[List[str]] = contextvars.ContextVar('handoff_chain', default=None)

def _get_handoff_chain() -> List[str]:
    chain = _handoff_chain_var.get()
    if chain is None:
        chain = []
        _handoff_chain_var.set(chain)
    return chain

The rest of _push_handoff/_pop_handoff/_get_handoff_depth/_clear_handoff_chain need no changes since they just call _get_handoff_chain(). This mirrors the pattern the codebase already uses correctly elsewhere for per-task isolation (e.g. tools/injected.py's ContextVar-based AgentState).

Confidence: CONFIRMED — traced parallel_handoffsasyncio.gatherHandoff.execute_async/_execute → push/await/pop against the single shared thread-local list.


Gap 3 — HookRunner has no execute_async method, so async retry-with-backoff crashes on every retryable error

File: praisonaiagents/hooks/runner.py (class HookRunner, lines 30–~180) and praisonaiagents/agent/chat_mixin.py, line 4646

HookRunner only defines:

# hooks/runner.py:82  and  :128
async def execute(self, event, input_data, target=None, _hooks=None) -> List[HookExecutionResult]: ...
def execute_sync(self, event, input_data, target=None) -> List[HookExecutionResult]: ...

There is no execute_async anywhere in the file (confirmed by a package-wide grep — the only other execute_async methods in the SDK belong to unrelated Handoff/HandoffFilter classes, not HookRunner).

self._hook_runner (agent/agent.py:238-252) is always constructed as an instance of exactly this HookRunner class.

Yet chat_mixin.py's async retry handler calls it unconditionally, with no guard and no try/except:

# chat_mixin.py:4604-4646, inside _achat_completion_with_retry
for attempt in range(max_attempts):
    try:
        return await self._execute_unified_achat_completion(...)
    except Exception as e:
        from ..errors import LLMError
        if not isinstance(e, LLMError) or not e.is_retryable:
            raise
        if attempt >= max_attempts - 1:
            raise
        delay = jittered_backoff(attempt, ...)
        retry_input = OnRetryInput(...)
        await self._hook_runner.execute_async(HookEvent.ON_RETRY, retry_input)   # <-- AttributeError, always
        await self._aemit_retry_stream_event(...)
        ...
        if not await interruptible_sleep(delay, interrupt_fn=interrupt_fn):
            raise RuntimeError("Agent interrupted during retry backoff")

Contrast with the sync-tool retry path in execution_mixin.py, which correctly guards the same kind of call:

# execution_mixin.py:1734-1743
if hasattr(hook_runner, 'execute_async'):
    await hook_runner.execute_async(HookEvent.ON_RETRY, retry_input, target=tool_name)
else:
    loop = asyncio.get_running_loop()
    await loop.run_in_executor(None, lambda: hook_runner.execute_sync(HookEvent.ON_RETRY, retry_input, target=tool_name))

This proves the chat_mixin.py:4646 call site is a straightforward missing-guard bug, not an intentionally different API.

Failure scenario: any agent doing an async chat call (achat/achat_completion) with a _retry_config set hits AttributeError: 'HookRunner' object has no attribute 'execute_async' the moment a retryable LLMError occurs (rate limit, timeout, transient 5xx) — every single time, 100% reproducible. The exception propagates out of the retry handler (exception-chained onto the original error), so the retry/backoff logic never actually executes: a transient error that should have been recovered from instead becomes a hard failure. This breaks a core resilience feature entirely for the async code path while leaving the sync path unaffected, which will look like intermittent "async agents are less reliable than sync agents" reports to users.

Fix (pick one):

  1. Add execute_async as an alias on HookRunner:
# hooks/runner.py, inside class HookRunner
async def execute_async(self, event, input_data, target=None, _hooks=None):
    """Alias of execute() for call sites expecting an explicit async entry point."""
    return await self.execute(event, input_data, target=target, _hooks=_hooks)
  1. Or fix the call site to match the already-correct pattern in execution_mixin.py:1734-1743 (guard with hasattr, fall back to execute_sync via executor), and wrap it in try/except so a hook-dispatch failure can never abort the retry loop regardless of which HookRunner-like object is passed in.

Option 1 is simpler and also fixes any other future call site that assumes this method exists (the API surface implied by the guarded call site in execution_mixin.py suggests it was expected to exist).

Confidence: CONFIRMED — grepped the whole package for execute_async, confirmed HookRunner defines no such method, confirmed self._hook_runner is always a HookRunner instance, and confirmed the call at chat_mixin.py:4646 is unconditional with no guard.


Suggested priority

Gap 1 and Gap 2 are silent data-correctness bugs in multi-agent scenarios (cross-agent context leakage, corrupted handoff-depth/cycle detection) and should be treated as the higher-severity pair. Gap 3 is a 100%-reproducible crash but is narrowly scoped to one call site and is the simplest one-line fix — good first PR.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingclaudeAuto-trigger Claude analysisdocumentationImprovements or additions to documentationperformance

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions