fix: resolve P1 issues from wrapper layer - thread safety and lazy loading (#1561)

* fix: resolve P1 issues from wrapper layer - thread safety and lazy loading

- Gap 1: Fix thread-unsafe lazy singleton in auto._get_openai_client()
  * Add double-check lock pattern with threading.Lock
  * Make client key-aware to prevent silent wrong-client bug
  * Follow existing pattern from tool_resolver.py:58-74

- Gap 2: Remove module-level logging.basicConfig() in auto.py
  * Replace with namespaced logger from _logging.py
  * Eliminates library anti-pattern of mutating root logger
  * Follows documented principle from _logging.py:1-29

- Gap 3: Add synchronization to __main__._get_typer_commands()
  * Add double-check lock to prevent race conditions
  * Prevent cache poisoning on exceptions (return empty set without caching)
  * Follow same thread-safe pattern as tool_resolver.py

Fixes #1560

Co-authored-by: MervinPraison <[email protected]>

* review: remove dead import logging, clean comment, add thread-safety tests

- Remove dead `import logging` from auto.py (unused after basicConfig removal)
- Simplify stale line-number comment to a single clear sentence
- Add TestThreadSafety class with 5 unit tests:
  * test_get_openai_client_returns_same_instance_for_same_key
  * test_get_openai_client_rebuilds_on_key_change
  * test_get_openai_client_no_root_logger_mutation
  * test_typer_commands_concurrent_calls_all_return_set
  * test_typer_commands_failure_does_not_poison_cache
All 13 tests pass.

Agent-Logs-Url: https://github.com/MervinPraison/PraisonAI/sessions/4edb8738-06a5-4cc0-a48b-f7f2cf5e4b70

Co-authored-by: MervinPraison <[email protected]>

* fix: address P1 race condition and resource leak in wrapper layer

- Fix OpenAI client double-check lock assignment ordering (Greptile P1)
- Add old client cleanup to prevent httpx connection leaks (CodeRabbit)
- Add exception logging to command discovery failures (CodeRabbit)
- Organize imports: move get_logger to top-level imports (CodeRabbit)

Co-authored-by: Mervin Praison <[email protected]>

---------

Co-authored-by: praisonai-triage-agent[bot] <272766704+praisonai-triage-agent[bot]@users.noreply.github.com>
Co-authored-by: MervinPraison <[email protected]>
Co-authored-by: copilot-swe-agent[bot] <[email protected]>
Co-authored-by: MervinPraison <[email protected]>

src/praisonai/praisonai/__main__.py

@@ -13,6 +13,7 @@ Design:
 """
 
 import sys
+import threading
 
 
 # ---------------------------------------------------------------------------
@@ -20,32 +21,41 @@ import sys
 # ---------------------------------------------------------------------------
 
 _typer_commands_cache = None
+_typer_commands_lock = threading.Lock()
 
 
 def _get_typer_commands():
-    """Auto-discover registered Typer commands via Click introspection.
-
-    Returns a set of command names that the Typer app knows about.
-    This is populated from app.py's register_commands() — no manual
-    lists to maintain.
-    """
+    """Auto-discover registered Typer commands via Click introspection."""
     global _typer_commands_cache
+
+    # Fast path
     if _typer_commands_cache is not None:
         return _typer_commands_cache
 
-    try:
-        from praisonai.cli.app import app, register_commands
-        register_commands()
+    with _typer_commands_lock:
+        if _typer_commands_cache is not None:  # Double-check
+            return _typer_commands_cache
 
-        import typer.main
-        import click
-        click_app = typer.main.get_command(app)
-        ctx = click.Context(click_app, info_name="praisonai")
-        _typer_commands_cache = set(click_app.list_commands(ctx))
-    except Exception:
-        _typer_commands_cache = set()
+        try:
+            from praisonai.cli.app import app, register_commands
+            register_commands()
 
-    return _typer_commands_cache
+            import typer.main
+            import click
+            click_app = typer.main.get_command(app)
+            ctx = click.Context(click_app, info_name="praisonai")
+            commands = set(click_app.list_commands(ctx))
+        except Exception:
+            # Do NOT poison the cache on failure — let the next caller retry.
+            import logging
+            logging.getLogger("praisonai.__main__").warning(
+                "Typer command discovery failed; falling back to legacy dispatch.",
+                exc_info=True,
+            )
+            return set()
+
+        _typer_commands_cache = commands
+        return _typer_commands_cache
 
 
 def _find_first_command(argv):

src/praisonai/praisonai/auto.py

@@ -16,7 +16,8 @@ import os
 import json
 import yaml
 from rich import print
-import logging
+import threading
+from praisonai._logging import get_logger
 
 # Type variable for Pydantic models
 T = TypeVar('T', bound=BaseModel)
@@ -42,6 +43,8 @@ _praisonai_classes = None  # (PraisonAgent, PraisonTask, Agents)
 _praisonai_tools = None  # dict of tool classes
 _litellm = None
 _openai_client = None
+_openai_client_lock = threading.Lock()
+_openai_client_key = None  # (api_key, base_url) the cached client was built with
 
 
 # --- CrewAI lazy loading ---
@@ -231,19 +234,32 @@ def _check_openai_available() -> bool:
 
 
 def _get_openai_client(api_key: str = None, base_url: str = None):
-    """Lazy load OpenAI client."""
-    global _openai_client
-    if _openai_client is None:
-        from openai import OpenAI
-        _openai_client = OpenAI(
-            api_key=api_key or os.environ.get("OPENAI_API_KEY"),
-            base_url=base_url
-        )
-    return _openai_client
+    """Lazy load OpenAI client (thread-safe, key-aware)."""
+    global _openai_client, _openai_client_key
+    key = (api_key or os.environ.get("OPENAI_API_KEY"), base_url)
+
+    # Fast path: already initialized with the same key
+    if _openai_client is not None and _openai_client_key == key:
+        return _openai_client
+
+    with _openai_client_lock:
+        if _openai_client is None or _openai_client_key != key:
+            from openai import OpenAI
+            old_client = _openai_client
+            # Set key first to maintain fast-path invariant
+            _openai_client_key = key
+            _openai_client = OpenAI(api_key=key[0], base_url=key[1])
+            # Close old client to prevent httpx connection leaks
+            if old_client is not None:
+                try:
+                    old_client.close()
+                except Exception:
+                    pass  # Best-effort cleanup
+        return _openai_client
 
 
-_loglevel = os.environ.get('LOGLEVEL', 'INFO').strip().upper() or 'INFO'
-logging.basicConfig(level=_loglevel, format='%(asctime)s - %(levelname)s - %(message)s')
+# Use namespaced logger; root logger is configured only by the CLI
+logger = get_logger("auto")
 
 # =============================================================================
 # Available Tools List (shared between generators) - Legacy for praisonai_tools
@@ -684,13 +700,13 @@ AG2 is not installed. Please install with:
         # Only show tools message if using a framework and tools are needed
         if (framework in ["crewai", "autogen"]) and not _check_praisonai_tools_available():
             if framework == "autogen":
-                logging.warning("""
+                logger.warning("""
 Tools are not available for autogen. To use tools, install:
     pip install "praisonai[autogen]" for v0.2
     pip install "praisonai[autogen-v4]" for v0.4
 """)
             else:
-                logging.warning(f"""
+                logger.warning(f"""
 Tools are not available for {framework}. To use tools, install:
     pip install "praisonai[{framework}]"
 """)
@@ -820,8 +836,8 @@ Tools are not available for {framework}. To use tools, install:
                 # If existing file is empty, treat as new file
                 existing_data = {"roles": {}, "dependencies": []}
         except (yaml.YAMLError, FileNotFoundError) as e:
-            logging.warning(f"Could not load existing agents file {self.agent_file}: {e}")
-            logging.warning("Creating new file instead of merging")
+            logger.warning(f"Could not load existing agents file {self.agent_file}: {e}")
+            logger.warning("Creating new file instead of merging")
             existing_data = {"roles": {}, "dependencies": []}
         
         # Start with existing data structure
@@ -1191,7 +1207,7 @@ Respond with:
             if not existing_data:
                 return new_data
         except (yaml.YAMLError, FileNotFoundError) as e:
-            logging.warning(f"Could not load existing workflow file {self.workflow_file}: {e}")
+            logger.warning(f"Could not load existing workflow file {self.workflow_file}: {e}")
             return new_data
         
         # Merge agents (avoid duplicates)

src/praisonai/tests/unit/test_auto_lazy_loading.py

@@ -109,3 +109,115 @@ class TestSafeStringSubstitution:
         
         assert 'AI' in result
         assert 'coding' in result
+
+
+class TestThreadSafety:
+    """Test thread-safe patterns added in the wrapper layer."""
+
+    def test_get_openai_client_returns_same_instance_for_same_key(self):
+        """Multiple threads requesting the same key must get the same client object."""
+        import threading
+        import praisonai.auto as auto
+
+        auto._openai_client = None
+        auto._openai_client_key = None
+
+        results = []
+        errors = []
+
+        def call_client():
+            try:
+                client = auto._get_openai_client(api_key="test-thread-key")
+                results.append(id(client))
+            except Exception as exc:
+                errors.append(exc)
+
+        threads = [threading.Thread(target=call_client) for _ in range(8)]
+        for t in threads:
+            t.start()
+        for t in threads:
+            t.join()
+
+        assert not errors, f"Unexpected errors: {errors}"
+        assert len(results) == 8
+        # All threads must receive the exact same cached instance
+        assert len(set(results)) == 1, "Got different client instances across threads"
+
+    def test_get_openai_client_rebuilds_on_key_change(self):
+        """Changing the (api_key, base_url) pair must produce a new client."""
+        import praisonai.auto as auto
+
+        auto._openai_client = None
+        auto._openai_client_key = None
+
+        client_a = auto._get_openai_client(api_key="key-a")
+        client_b = auto._get_openai_client(api_key="key-b")
+
+        assert id(client_a) != id(client_b), "Different keys must yield different clients"
+
+    def test_get_openai_client_no_root_logger_mutation(self):
+        """Importing auto must not add any new handlers to the root logger."""
+        import logging
+        import sys
+
+        # Record handlers before re-importing auto
+        root_before = set(id(h) for h in logging.getLogger().handlers)
+
+        # Force a fresh import of auto to trigger any module-level side effects
+        for key in list(sys.modules.keys()):
+            if key in ("praisonai.auto", "praisonai.praisonai.auto"):
+                del sys.modules[key]
+
+        import praisonai.auto  # noqa: F401
+
+        root_after = set(id(h) for h in logging.getLogger().handlers)
+        new_handlers = root_after - root_before
+        assert not new_handlers, (
+            "auto.py must not add handlers to the root logger on import; "
+            f"found {len(new_handlers)} new handler(s)"
+        )
+
+    def test_typer_commands_concurrent_calls_all_return_set(self):
+        """Concurrent callers of _get_typer_commands must all get a set back."""
+        import threading
+        import praisonai.__main__ as main_mod
+
+        # Reset cache so we exercise the lock path
+        main_mod._typer_commands_cache = None
+
+        results = []
+        errors = []
+
+        def call_commands():
+            try:
+                result = main_mod._get_typer_commands()
+                results.append(result)
+            except Exception as exc:
+                errors.append(exc)
+
+        threads = [threading.Thread(target=call_commands) for _ in range(10)]
+        for t in threads:
+            t.start()
+        for t in threads:
+            t.join()
+
+        assert not errors, f"Unexpected errors: {errors}"
+        assert len(results) == 10
+        assert all(isinstance(r, set) for r in results), "All results must be sets"
+
+    def test_typer_commands_failure_does_not_poison_cache(self):
+        """A failed discovery must not cache the empty set (allows retries)."""
+        import praisonai.__main__ as main_mod
+
+        main_mod._typer_commands_cache = None
+
+        # Patch register_commands to raise
+        import unittest.mock as mock
+        with mock.patch("praisonai.cli.app.register_commands", side_effect=RuntimeError("boom")):
+            result = main_mod._get_typer_commands()
+
+        assert result == set(), "Should return empty set on failure"
+        assert main_mod._typer_commands_cache is None, (
+            "Cache must remain None after failure to allow retries"
+        )
+