Merge pull request #1335 from MervinPraison/claude/issue-1330-20260409-0928

fix: resolve three critical architectural gaps in error handling and resource management

Author: MervinPraison

.github/workflows/auto-pr-comment.yml

@@ -409,7 +409,7 @@ jobs:
     permissions:
       pull-requests: write
     steps:
-      - name: Trigger CodeRabbit, Qodo, and Gemini reviews
+      - name: Trigger CodeRabbit and Qodo reviews
         uses: actions/github-script@v7
         with:
           github-token: ${{ secrets.GH_TOKEN }}
@@ -432,12 +432,12 @@ jobs:
             });
             console.log(`Triggered Qodo for bot PR #${pr}`);
 
-            // 3. Trigger Gemini review
-            await github.rest.issues.createComment({
-              issue_number: pr, owner, repo,
-              body: '@gemini review this PR'
-            });
-            console.log(`Triggered Gemini for bot PR #${pr}`);
+            // 3. Trigger Gemini review (commented out)
+            // await github.rest.issues.createComment({
+            //   issue_number: pr, owner, repo,
+            //   body: '@gemini review this PR'
+            // });
+            // console.log(`Triggered Gemini for bot PR #${pr}`);
 
             // NOTE: @copilot is NOT triggered here.
             // It will be triggered by copilot-after-coderabbit or copilot-after-qodo

src/praisonai-agents/.agent/workflows/review-chain.md

@@ -11,8 +11,8 @@ PR opened
   ↓
 CodeRabbit (@coderabbitai) ─── auto for human PRs, triggered via comment for bot PRs
 Qodo (/review)             ─── auto for human PRs, triggered via comment for bot PRs
-Gemini (@gemini)           ─── triggered via comment for bot PRs/Issues
-  ↓ (~3-5 min)                  ↓ (auto triggers Claude on completion)
+<!-- Gemini (@gemini)           ─── triggered via comment for bot PRs/Issues
+  ↓ (~3-5 min)                  ↓ (auto triggers Claude on completion) -->
 Copilot (@copilot)         ─── triggered ONLY after CodeRabbit or Qodo post their review
   ↓
 Claude (@claude)           ─── triggered ONLY after Copilot OR Gemini finishes (final reviewer)

src/praisonai-agents/praisonaiagents/agent/agent.py

@@ -4464,6 +4464,146 @@ def _get_tools_cache_key(self, tools):
     @contextlib.contextmanager
 
 
+    # -------------------------------------------------------------------------
+    #                       Resource Lifecycle Management
+    # -------------------------------------------------------------------------
+    
+    def close(self) -> None:
+        """Synchronously close the agent and clean up resources."""
+        if getattr(self, '_closed', False):
+            return
+
+        # Memory cleanup
+        try:
+            memory = getattr(self, "_memory_instance", None)
+            if memory and hasattr(memory, 'close_connections'):
+                memory.close_connections()
+        except Exception as e:
+            logger.warning(f"Memory cleanup failed: {e}")
+
+        # MCP cleanup  
+        try:
+            if hasattr(self, '_mcp_clients') and self._mcp_clients:
+                for client_name, client in self._mcp_clients.items():
+                    if hasattr(client, 'close'):
+                        client.close()
+                self._mcp_clients.clear()
+        except Exception as e:
+            logger.warning(f"MCP cleanup failed: {e}")
+
+        # Server registry cleanup
+        try:
+            self._cleanup_server_registrations()
+        except Exception as e:
+            logger.warning(f"Server cleanup failed: {e}")
+
+        # Background tasks cleanup
+        try:
+            for task in getattr(self, '_background_tasks', []):
+                if hasattr(task, 'cancel'):
+                    task.cancel()
+        except Exception as e:
+            logger.warning(f"Task cleanup failed: {e}")
+
+        # Always set closed flag
+        self._closed = True
+    
+    async def aclose(self) -> None:
+        """Async version of close() for async context managers."""
+        try:
+            # Close memory connections asynchronously if supported
+            if hasattr(self, 'memory') and self.memory:
+                if hasattr(self.memory, 'aclose'):
+                    await self.memory.aclose()
+                elif hasattr(self.memory, 'close_connections'):
+                    self.memory.close_connections()
+            
+            # Close MCP sessions asynchronously if supported
+            if hasattr(self, '_mcp_clients') and self._mcp_clients:
+                for client in self._mcp_clients.values():
+                    if hasattr(client, 'aclose'):
+                        await client.aclose()
+                    elif hasattr(client, 'close'):
+                        client.close()
+                self._mcp_clients.clear()
+            
+            # Clean up server registrations and tasks
+            self._cleanup_server_registrations()
+            
+            if hasattr(self, '_background_tasks'):
+                for task in self._background_tasks:
+                    if hasattr(task, 'cancel'):
+                        task.cancel()
+                    # Wait for cancellation to complete
+                    try:
+                        await task
+                    except asyncio.CancelledError:
+                        pass
+            
+            self._closed = True
+            
+        except Exception as e:
+            logger.warning(f"Error during async agent cleanup: {e}")
+    
+    def _cleanup_server_registrations(self) -> None:
+        """Clean up global server registry entries for this agent."""
+        try:
+            agent_id = self.agent_id
+            with _server_lock:
+                # Remove from _registered_agents
+                ports_to_clean = []
+                for port, path_dict in _registered_agents.items():
+                    paths_to_remove = []
+                    for path, registered_id in path_dict.items():
+                        if registered_id == agent_id:
+                            paths_to_remove.append(path)
+                    
+                    for path in paths_to_remove:
+                        del path_dict[path]
+                    
+                    # If no paths left for this port, mark port for cleanup
+                    if not path_dict:
+                        ports_to_clean.append(port)
+                
+                # Clean up empty port entries
+                for port in ports_to_clean:
+                    _registered_agents.pop(port, None)
+                    _server_started.pop(port, None)
+                    # Note: We don't clean up _shared_apps here as other agents might be using them
+                    
+        except Exception as e:
+            logger.warning(f"Error cleaning up server registrations: {e}")
+    
+    def __enter__(self):
+        """Context manager entry."""
+        return self
+    
+    def __exit__(self, exc_type, exc_val, exc_tb):
+        """Context manager exit - clean up resources."""
+        self.close()
+    
+    async def __aenter__(self):
+        """Async context manager entry.""" 
+        return self
+    
+    async def __aexit__(self, exc_type, exc_val, exc_tb):
+        """Async context manager exit - clean up resources."""
+        await self.aclose()
+    
+    def __del__(self):
+        """Destructor - ensure resources are cleaned up."""
+        try:
+            if not getattr(self, '_closed', False):
+                self.close()
+        except Exception:
+            # Ignore errors in destructor to avoid issues during shutdown
+            pass
+    
+    @property
+    def is_closed(self) -> bool:
+        """Returns True if the agent has been closed."""
+        return getattr(self, '_closed', False)
+
     def __str__(self):
         return f"Agent(name='{self.name}', role='{self.role}', goal='{self.goal}')"
 

src/praisonai-agents/praisonaiagents/agent/execution_mixin.py

@@ -54,6 +54,14 @@ def _get_display_functions():
 class ExecutionMixin:
     """Mixin providing execution methods for the Agent class."""
 
+    def _safe_sleep(self, duration: float) -> None:
+        """Synchronous sleep - safe for sync contexts only."""
+        time.sleep(duration)
+
+    async def _safe_sleep_async(self, duration: float) -> None:
+        """Async sleep - use this in async contexts."""
+        await asyncio.sleep(duration)
+
     def generate_task(self) -> 'Task':
         """Generate a Task object from the agent's instructions"""
         from ..task.task import Task
@@ -954,10 +962,10 @@ def run_server():
                 server_thread.start()
 
                 # Wait for a moment to allow the server to start and register endpoints
-                time.sleep(0.5)
+                self._safe_sleep(0.5)
             else:
                 # If server is already running, wait a moment to make sure the endpoint is registered
-                time.sleep(0.1)
+                self._safe_sleep(0.1)
                 print(f"🔌 Available endpoints on port {port}: {', '.join(list(_registered_agents[port].keys()))}")
 
             # Get the stack frame to check if this is the last launch() call in the script
@@ -986,7 +994,7 @@ def run_server():
                         try:
                             print("\nAll agents registered for HTTP mode. Press Ctrl+C to stop the servers.")
                             while True:
-                                time.sleep(1)
+                                self._safe_sleep(1)
                         except KeyboardInterrupt:
                             print("\nServers stopped")
                 except Exception as e:
@@ -995,7 +1003,7 @@ def run_server():
                     try:
                         print("\nKeeping HTTP servers alive. Press Ctrl+C to stop.")
                         while True:
-                            time.sleep(1)
+                            self._safe_sleep(1)
                     except KeyboardInterrupt:
                         print("\nServers stopped")
             return None
@@ -1055,12 +1063,12 @@ def run_mcp_server():
 
             server_thread = threading.Thread(target=run_mcp_server, daemon=True)
             server_thread.start()
-            time.sleep(0.5)
+            self._safe_sleep(0.5)
 
             try:
                 print("\nKeeping MCP server alive. Press Ctrl+C to stop.")
                 while True:
-                    time.sleep(1)
+                    self._safe_sleep(1)
             except KeyboardInterrupt:
                 print("\nMCP Server stopped")
             return None

src/praisonai-agents/praisonaiagents/main.py

@@ -24,8 +24,10 @@ def _rich():
 
 # Logging is already configured in _logging.py via __init__.py
 
-# Global list to store error logs
-error_logs = []
+# Global list to store error logs - with bounded size to prevent memory leaks
+from collections import deque
+MAX_ERROR_LOGS = 1000  # Maximum number of error logs to keep in memory
+error_logs = deque(maxlen=MAX_ERROR_LOGS)  # Ring buffer that auto-evicts old errors
 _error_logs_lock = threading.Lock()
 
 # Separate registries for sync and async callbacks

src/praisonai-agents/praisonaiagents/memory/__init__.py

@@ -106,6 +106,16 @@ def __getattr__(name):
     if name == "repeat":
         from ..workflows import repeat
         return repeat
+    # Structured result types for better error handling
+    if name == "MemoryResult":
+        from .results import MemoryResult
+        return MemoryResult
+    if name == "MemoryResultStatus":
+        from .results import MemoryResultStatus
+        return MemoryResultStatus
+    if name == "SearchResult":
+        from .results import SearchResult
+        return SearchResult
     # Backward compatibility aliases
     if name == "StepInput":
         from ..workflows import WorkflowContext