praisonaiagents core: MCP server-registry never shrinks (stale capability gating), async goal loop blocks the event loop on every judge call, and FileLock's stale-lock recovery busts a live lock

Scope: src/praisonai-agents/praisonaiagents only (core SDK). Three gaps below, each confirmed by reading the actual implementation in the current main branch (not inferred from comments), with exact file:line locations, a concrete failure scenario, and a suggested fix. Not documentation, tests, coverage, or file-size issues — all three are functional correctness/concurrency bugs that violate the package's own stated MUSTs ("multi-agent + async safe by default", "production-ready... safe by default").

1. MCP._active_server_names only ever grows, so a skill's MCP-server requirement can pass validation against a server that is no longer connected — or even running in this process at all

Files: mcp/mcp.py:319-343 (registry definition + list_active_server_names), mcp/mcp.py:995-1002 (with_tool_prefix, the only place names are added), mcp/mcp.py:1243-1298 (shutdown() — no removal anywhere), skills/capability_validator.py:205-223 (_get_available_servers).

This registry was added specifically to fix issue #3307 ("MCP skill-gate always fails closed" — _get_available_servers() was a permanent stub returning set()). The fix works, but only in one direction:

# mcp/mcp.py:319-324
# Process-level registry of sanitized MCP server names that have been
# namespaced via with_tool_prefix(), mirroring how tools/registry.py tracks
# tool names. Lets skills' CapabilityValidator discover connected servers
# instead of always failing closed (issue #3307).
_active_server_names: set = set()
_active_server_names_lock = threading.Lock()

# mcp/mcp.py:995-1002 (with_tool_prefix) — the ONLY write path
with type(self)._active_server_names_lock:
    if prefix:
        type(self)._active_server_names.add(prefix)
    type(self)._active_server_names.add(sanitized)

shutdown() (mcp/mcp.py:1243-1298) tears down the stdio runner / SSE / HTTP-stream / WebSocket clients, but never touches _active_server_names. __del__ (mcp/mcp.py:1300+) just calls shutdown(). A repo-wide grep confirms there is no discard/remove/pop anywhere against this set:

$ grep -rn "_active_server_names" mcp/ skills/
mcp/mcp.py:323:    _active_server_names: set = set()
mcp/mcp.py:1001:                type(self)._active_server_names.add(prefix)
mcp/mcp.py:1002:            type(self)._active_server_names.add(sanitized)
mcp/mcp.py:340:    def list_active_server_names(cls) -> set:
mcp/mcp.py:342:            return set(cls._active_server_names)
skills/capability_validator.py:220:            return set(MCP.list_active_server_names())

CapabilityValidator._get_available_servers() reads this set live (deliberately uncached, per its own docstring, "to avoid a stale snapshot") to decide whether a skill's declared requirements: {servers: [...]} is satisfied under EnforcementLevel.STRICT:

# skills/capability_validator.py:205-223
def _get_available_servers(self) -> Set[str]:
    try:
        from ..mcp.mcp import MCP
        return set(MCP.list_active_server_names())
    except ImportError:
        logger.debug("MCP not available")
        return set()

Concrete failure scenario: a long-running process (a bot gateway, a task worker, a test suite) creates and disposes many short-lived MCP("filesystem-server-cmd") instances over its lifetime — e.g. one per user session, or one per test case. The very first time any server named filesystem connects, filesystem is added to the process-global set forever. Every subsequent skill anywhere in the same process that declares requirements: {servers: [filesystem]} under STRICT enforcement will now validate successfully and be offered to the model — even in a session that never configured a filesystem MCP server, and even after every MCP instance that ever used that name has been shutdown(). This is a live capability-gating correctness bug, not just an unbounded-memory leak: STRICT enforcement exists specifically to stop a skill from being offered when its dependency isn't actually present, and this registry defeats that guarantee after the first successful connection of any given server name.

Suggested fix: track a live refcount per name instead of a one-way set, and decrement it from shutdown() so the count — and therefore membership — reflects instances that are still actually alive:

# mcp/mcp.py
_active_server_names: Dict[str, int] = {}   # name -> live-instance refcount
_active_server_names_lock = threading.Lock()

@classmethod
def list_active_server_names(cls) -> set:
    with cls._active_server_names_lock:
        return {name for name, count in cls._active_server_names.items() if count > 0}

# with_tool_prefix(): register + remember what this instance added
with type(self)._active_server_names_lock:
    for name in filter(None, (prefix, sanitized)):
        type(self)._active_server_names[name] = type(self)._active_server_names.get(name, 0) + 1
        self._registered_server_names.add(name)   # per-instance, init'd in __init__

# shutdown(): release exactly what this instance registered, once
if not getattr(self, '_server_names_released', False):
    with type(self)._active_server_names_lock:
        for name in getattr(self, '_registered_server_names', ()):
            count = type(self)._active_server_names.get(name, 0)
            if count <= 1:
                type(self)._active_server_names.pop(name, None)
            else:
                type(self)._active_server_names[name] = count - 1
    self._server_names_released = True

2. run_autonomous_async's goal-completion gate is fully synchronous and makes a blocking LLM network call on every iteration — the exact hazard its own sibling method was written to prevent

Files: goal/loop.py:116-194 (_goal_gate), goal/judge.py:174-204 (judge_goal), agent/agent.py:5095 (async def run_autonomous_async), agent/agent.py:5348-5350 (call site), agent/agent.py:5805-5819 (_verification_gate_async, the already-fixed sibling).

_goal_gate first runs any configured verification hooks synchronously, then — regardless of whether hooks are configured — calls judge_goal(...) synchronously:

# goal/loop.py:136-140
if getattr(self, "_verification_hooks", None):
    results = self._run_verification_hooks()
    ...

# goal/loop.py:163-169
state.turns_used += 1
verdict, reason = judge_goal(
    state,
    _tail(response),
    judge_model=getattr(self, "_goal_judge_model", None),
    verification_block=verification_block,
)

judge_goal performs a real, synchronous HTTP call to the LLM provider via litellm.completion(...) — not a hook, not optional, this runs on every iteration any time a goal loop is active:

# goal/judge.py:196-204
try:
    judge = Judge(model=judge_model or _default_judge_model(), temperature=0.0)
    litellm = judge._get_litellm()
    response = litellm.completion(
        model=judge.model,
        messages=[{"role": "user", "content": prompt}],
        temperature=0.0,
        max_tokens=300,
    )

This whole chain is invoked directly — not awaited, not offloaded — from inside async def run_autonomous_async (defined at agent/agent.py:5095):

# agent/agent.py:5348-5350
_goal_gate = self._goal_gate(response_str)
if _goal_gate is not None:
    _outcome, _reason = _goal_gate

Compare this to the method that already exists in the very same class for the analogous verification-hook gate, whose docstring states exactly why this matters:

# agent/agent.py:5805-5819
async def _verification_gate_async(
    self, response_str: str, iterations: int
) -> Optional[str]:
    """Async counterpart of :meth:`_verification_gate`.

    Hook execution (which may shell out via :class:`CommandVerificationHook`)
    is offloaded to a worker thread so a slow or blocking check never stalls
    the event loop shared by other concurrent agents.
    """
    if not getattr(self, "_verification_hooks", None):
        return None
    import asyncio as _asyncio
    results = await _asyncio.to_thread(self._run_verification_hooks)
    return self._verification_gate_feedback(results)

run_autonomous_async calls this async-safe sibling correctly two lines later at agent/agent.py:5376 (_gate_feedback = await self._verification_gate_async(...)) — but the goal gate right above it at line 5348 never got the same treatment.

Concrete failure scenario: any Agent(goal=..., goal_criteria=...) running run_autonomous_async inside a server or gateway that shares one event loop across multiple concurrent agents/conversations (the normal deployment shape for an async chatbot/task backend). On every single autonomous iteration, _goal_gate blocks that shared event loop for the full duration of a real LLM API round trip (typically hundreds of ms to several seconds, more under provider rate-limiting or retries) — stalling every other concurrent agent's await points for that entire window. This is not a rare edge case gated behind an opt-in hook; it fires on every iteration of every goal-configured autonomous run, which is a mainline, documented feature of this SDK.

Suggested fix: add an async counterpart mirroring _verification_gate_async's existing pattern, and call it from the async loop instead of the sync method:

# goal/loop.py
async def _goal_gate_async(self, response: str) -> Optional[Tuple[str, str]]:
    """Async counterpart of :meth:`_goal_gate`.

    Hook execution and the judge's LLM call are both potentially blocking
    (shell-out / network I/O), so they're offloaded to a worker thread —
    mirrors ``_verification_gate_async``'s existing pattern for the same
    reason: a slow judge/hook must never stall the event loop shared by
    other concurrent agents.
    """
    state = getattr(self, "_goal_state", None)
    if state is None or state.status != "active":
        return None
    import asyncio
    return await asyncio.to_thread(self._goal_gate, response)

# agent/agent.py:5348 — change the call site
_goal_gate = await self._goal_gate_async(response_str)

(The sync call site at agent/agent.py:4884, inside the non-async run_autonomous, is correct as-is and needs no change.)

3. storage.base.FileLock's "stale lock" recovery is based on the waiter's own wait time, not the lock's age — it force-removes a lock that is still legitimately held

Files: storage/base.py:29-84 (FileLock), memory/learn/stores.py:21,89-100 (consumer — the learned-memory store).

# storage/base.py:48-71
def __enter__(self):
    """Acquire the lock."""
    import time
    start = time.time()

    while True:
        try:
            # Try to create lock file exclusively
            self._fd = os.open(
                str(self.lock_path),
                os.O_CREAT | os.O_EXCL | os.O_WRONLY
            )
            break
        except FileExistsError:
            if time.time() - start > self.timeout:
                # Force remove stale lock
                try:
                    self.lock_path.unlink()
                except Exception:
                    pass
                continue
            time.sleep(0.01)

    return self

def __exit__(self, exc_type, exc_val, exc_tb):
    """Release the lock."""
    if self._fd is not None:
        try:
            os.close(self._fd)
        except Exception:
            pass
    try:
        self.lock_path.unlink()
    except Exception:
        pass
    return False

self.timeout (default 10.0) is measured against start, the moment this particular waiter began waiting — it has nothing to do with how long the lock file has actually existed. If Writer A legitimately holds the lock for 11 seconds (a large learned-memory file, a slow disk, a busy process), Writer B — who started waiting at second 1 — hits its own 10-second timeout at second 11 and force-unlink()s the lock file A is still using, then immediately re-os.open(O_EXCL)s and proceeds as if it now holds the lock. A is still inside its critical section with its own self._fd open and has no idea the file was removed out from under it. Both A and B now believe they hold the lock and can run their read-modify-write JSON save concurrently, and whichever __exit__ runs last silently wins — this is precisely the corruption a lock exists to prevent, and it's unconditional: __exit__ never checks whether the lock file it's unlinking is still the one this instance created before removing it.

This is used by the learned-memory store, which is explicitly documented as multi-agent-safe shared state:

# memory/learn/stores.py:89-100
# DRY: Use BaseJSONStore for thread-safe storage
self._store = BaseJSONStore(...)

Contrast with the sibling lock already in this codebase for the same purpose, session/store.py:372-411, which uses OS-level fcntl.flock/msvcrt.locking — locks that are automatically released by the OS if the holding process dies, so no "waiter timeout busts the file" hack is needed at all:

# session/store.py:394-410 (correct pattern, no stale-busting logic)
if _HAS_FCNTL:
    fcntl.flock(self._lock_file.fileno(), fcntl.LOCK_EX | fcntl.LOCK_NB)

Concrete failure scenario: two agents (or two processes) both call agent.remember(...)/the learned-memory auto-save path around the same time; one write takes longer than 10s (plausible on a loaded disk or a large learned-facts file — not a contrived edge case for a store meant to grow over an agent's lifetime). The second writer force-removes the first's lock mid-write and both writers save concurrently; one write silently clobbers the other's learned facts with no error raised to either caller.

Suggested fix: base staleness on the lock file's age (its mtime), not on how long this particular waiter has been queued — a lock actively refreshed by a live holder is never busted regardless of queue time, while one truly abandoned (holder crashed without running __exit__) is still reclaimed once it exceeds timeout:

def __enter__(self):
    import time
    while True:
        try:
            self._fd = os.open(
                str(self.lock_path),
                os.O_CREAT | os.O_EXCL | os.O_WRONLY
            )
            break
        except FileExistsError:
            try:
                age = time.time() - self.lock_path.stat().st_mtime
            except OSError:
                age = 0.0  # lock file vanished between the failed open and stat(); just retry
            if age > self.timeout:
                # Genuinely stale: the file itself has outlived the timeout,
                # not merely this waiter's patience.
                try:
                    self.lock_path.unlink()
                except Exception:
                    pass
                continue
            time.sleep(0.01)
    return self

Validation performed

Each finding was confirmed by directly reading the referenced files/lines on the current main branch:

• Finding 1: mcp/mcp.py's registry definition, its single write site in with_tool_prefix, shutdown()/__del__ (confirmed no removal path via full read + repo-wide grep for _active_server_names), and skills/capability_validator.py's consumer — cross-referenced against the original issue (#3307) this registry was built to fix.
• Finding 2: goal/loop.py's _goal_gate full body, goal/judge.py's judge_goal (confirmed the synchronous litellm.completion(...) call), both call sites in agent/agent.py (sync run_autonomous at line 4884 and async run_autonomous_async at line 5348), and the sibling _verification_gate_async at line 5805 whose own docstring states the exact hazard being avoided there but not in _goal_gate.
• Finding 3: storage/base.py's full FileLock class, its consumer in memory/learn/stores.py, and the correctly-implemented sibling FileLock in session/store.py used as a working counter-example.

Generated with Claude Code

https://claude.ai/code/session_01MVEmHcz8WRU9grFWuyW3hi