fix: check agent server readiness timeout return value (fixes #1631) (#1655)

* fix: check agent server readiness timeout return value (fixes #1631)

- Add return value checking for ready_event.wait() to detect timeouts
- Log warning when server doesn't become ready within timeout period
- Make timeout configurable via PRAISONAI_SERVER_READY_TIMEOUT env var
- Add comprehensive regression tests for timeout behavior

Co-authored-by: praisonai-triage-agent[bot] <praisonai-triage-agent[bot]@users.noreply.github.com>

* fix: address reviewer feedback - add error handling for invalid timeout env var

- Add try/catch for float() conversion to prevent ValueError crashes
- Prevent real server spawning in unit tests via Thread.start mocking
- Add test for invalid env var fallback behavior
- Preserve other env vars during testing (avoid clear=True)

Addresses feedback from Gemini, CodeRabbit, Qodo, and Copilot reviewers.

Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>

* fix: use monkeypatch.delenv and move import time to module level

- Replace patch.dict(os.environ, {"": ""}) in test_default_timeout_value
  with monkeypatch.delenv so the test exercises the true "env var unset"
  default path rather than the ValueError fallback path
- Move `import time` from inside test function body to module-level imports

Agent-Logs-Url: https://github.com/MervinPraison/PraisonAI/sessions/5c472f85-484f-4a03-b47a-9307639ecd0a

Co-authored-by: MervinPraison <454862+MervinPraison@users.noreply.github.com>

---------

Co-authored-by: praisonai-triage-agent[bot] <272766704+praisonai-triage-agent[bot]@users.noreply.github.com>
Co-authored-by: praisonai-triage-agent[bot] <praisonai-triage-agent[bot]@users.noreply.github.com>
Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: MervinPraison <454862+MervinPraison@users.noreply.github.com>

Author: 3 people

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

@@ -126,7 +126,24 @@ def run_server():
 
         thread = threading.Thread(target=run_server, daemon=True)
         thread.start()
-        ready_event.wait(timeout=5.0)  # Deterministic wait instead of sleep(0.5)
+        
+        # Check for configurable timeout via environment variable
+        try:
+            timeout = float(os.environ.get("PRAISONAI_SERVER_READY_TIMEOUT", "5.0"))
+        except ValueError:
+            logger.warning("Invalid PRAISONAI_SERVER_READY_TIMEOUT value. Using default 5.0s.")
+            timeout = 5.0
+        became_ready = ready_event.wait(timeout=timeout)
+        
+        if not became_ready:
+            logger.warning(
+                "Agent server on port %s did not become ready within %.1fs. "
+                "Proceeding, but some features may not work correctly. "
+                "Check server logs for startup errors.",
+                port,
+                timeout,
+            )
+        
         return True
 
 

src/praisonai-agents/tests/unit/agents/test_server_registry_timeout.py

@@ -0,0 +1,128 @@
+"""Test for agent server readiness timeout warning functionality."""
+
+import time
+import threading
+import logging
+import pytest
+from unittest.mock import patch, MagicMock
+from praisonaiagents.agents.agents import _AgentServerRegistry
+
+
+def test_server_readiness_timeout_emits_warning(caplog):
+    """A server that does not start in time must produce a WARNING log entry."""
+    registry = _AgentServerRegistry()
+    
+    # Create a mock FastAPI app that won't signal readiness
+    mock_app = MagicMock()
+    
+    # Register the app but don't set the ready event
+    with patch.dict(registry._apps, {8000: mock_app}):
+        with patch.dict(registry._ready_events, {8000: threading.Event()}):
+            with patch.dict(registry._started, {8000: False}):
+                with patch("threading.Thread.start"):  # Prevent real server spawning
+                    with caplog.at_level(logging.WARNING):
+                        # Use a very short timeout to ensure the test doesn't hang
+                        with patch.dict("os.environ", {"PRAISONAI_SERVER_READY_TIMEOUT": "0.1"}):
+                            result = registry.start_server_if_needed(8000, host="127.0.0.1")
+                    
+                    # The method should still return True but log a warning
+                    assert result is True
+                    assert "8000" in caplog.text
+                    assert "did not become ready" in caplog.text.lower()
+
+
+def test_server_readiness_timeout_configurable_via_env():
+    """The timeout should be configurable via PRAISONAI_SERVER_READY_TIMEOUT."""
+    registry = _AgentServerRegistry()
+    
+    # Create a mock FastAPI app
+    mock_app = MagicMock()
+    
+    # Register the app but don't set the ready event
+    with patch.dict(registry._apps, {8001: mock_app}):
+        with patch.dict(registry._ready_events, {8001: threading.Event()}):
+            with patch.dict(registry._started, {8001: False}):
+                with patch("threading.Thread.start"):  # Prevent real server spawning
+                    # Test with custom timeout
+                    with patch.dict("os.environ", {"PRAISONAI_SERVER_READY_TIMEOUT": "0.05"}):
+                        start_time = time.time()
+                        result = registry.start_server_if_needed(8001, host="127.0.0.1")
+                        duration = time.time() - start_time
+                    
+                    # Should respect the custom timeout and complete quickly
+                    assert result is True
+                    assert duration < 1.0  # Should be much less than default 5s
+
+
+def test_server_readiness_success_no_warning(caplog):
+    """A server that starts in time should not produce any warning."""
+    registry = _AgentServerRegistry()
+    
+    # Create a mock FastAPI app
+    mock_app = MagicMock()
+    ready_event = threading.Event()
+    
+    # Register the app and signal readiness immediately
+    with patch.dict(registry._apps, {8002: mock_app}):
+        with patch.dict(registry._ready_events, {8002: ready_event}):
+            with patch.dict(registry._started, {8002: False}):
+                with patch("threading.Thread.start"):  # Prevent real server spawning
+                    # Set the event immediately to simulate quick startup
+                    ready_event.set()
+                    
+                    with caplog.at_level(logging.WARNING):
+                        result = registry.start_server_if_needed(8002, host="127.0.0.1")
+                    
+                    # Should succeed without warnings
+                    assert result is True
+                    assert "did not become ready" not in caplog.text.lower()
+
+
+def test_default_timeout_value(monkeypatch):
+    """Test that the default timeout is 5.0 seconds when env var is not set."""
+    registry = _AgentServerRegistry()
+    
+    # Create a mock FastAPI app
+    mock_app = MagicMock()
+    ready_event = threading.Event()
+    
+    # Register the app but don't set the ready event
+    with patch.dict(registry._apps, {8003: mock_app}):
+        with patch.dict(registry._ready_events, {8003: ready_event}):
+            with patch.dict(registry._started, {8003: False}):
+                with patch("threading.Thread.start"):  # Prevent real server spawning
+                    # Remove only the specific env var so other env vars (PATH etc.) remain intact
+                    monkeypatch.delenv("PRAISONAI_SERVER_READY_TIMEOUT", raising=False)
+                    with patch("threading.Event.wait") as mock_wait:
+                        mock_wait.return_value = False
+                        result = registry.start_server_if_needed(8003, host="127.0.0.1")
+                    
+                    # Should have called wait with default timeout of 5.0
+                    mock_wait.assert_called_once_with(timeout=5.0)
+                    assert result is True
+
+
+def test_invalid_timeout_env_var_fallback(caplog):
+    """Test that invalid PRAISONAI_SERVER_READY_TIMEOUT falls back to default with warning."""
+    registry = _AgentServerRegistry()
+    
+    # Create a mock FastAPI app
+    mock_app = MagicMock()
+    ready_event = threading.Event()
+    
+    # Register the app but don't set the ready event
+    with patch.dict(registry._apps, {8004: mock_app}):
+        with patch.dict(registry._ready_events, {8004: ready_event}):
+            with patch.dict(registry._started, {8004: False}):
+                with patch("threading.Thread.start"):  # Prevent real server spawning
+                    # Test with invalid timeout value
+                    with patch.dict("os.environ", {"PRAISONAI_SERVER_READY_TIMEOUT": "invalid"}):
+                        with caplog.at_level(logging.WARNING):
+                            with patch("threading.Event.wait") as mock_wait:
+                                mock_wait.return_value = False
+                                result = registry.start_server_if_needed(8004, host="127.0.0.1")
+                    
+                    # Should have fallen back to 5.0 and logged warning
+                    mock_wait.assert_called_once_with(timeout=5.0)
+                    assert result is True
+                    assert "Invalid PRAISONAI_SERVER_READY_TIMEOUT" in caplog.text