From 816b91bdd536a2e8e1af549f05ff62ffec7acbbb Mon Sep 17 00:00:00 2001 From: Scott Severance Date: Tue, 8 Sep 2026 14:01:17 +0000 Subject: [PATCH 1/2] feat: add request timeout and retry logic to _request method --- agent/core/llm.py | 65 +++++++++++++++++++++++++++++------------------ 1 file changed, 40 insertions(+), 25 deletions(-) diff --git a/agent/core/llm.py b/agent/core/llm.py index 4168ec1..02f0ad2 100644 --- a/agent/core/llm.py +++ b/agent/core/llm.py @@ -21,6 +21,8 @@ class LLMClient: def __init__(self, config: LLMConfig): self.config = config self.base_url = config.base_url.rstrip("/") + self.max_retries = 3 + self.retry_delay = 1.0 def _validate_response(self, response: dict, required_fields: list[str]) -> None: """Validate that response contains expected fields. @@ -76,36 +78,49 @@ def _request( ) -> dict: url = f"{self.base_url}{endpoint}" payload = json.dumps(data).encode("utf-8") - req = urllib.request.Request( - url, - data=payload, - headers={"Content-Type": "application/json"}, - method="POST", - ) effective_timeout = timeout if timeout is not None else self.config.timeout - try: - resp = urllib.request.urlopen(req, timeout=effective_timeout) - if stream: - return resp # Return response object for streaming - return json.loads(resp.read().decode("utf-8")) - except socket.timeout: - raise OllamaError( - f"Timeout connecting to Ollama at {self.base_url} " - f"(waited {self.config.timeout}s). Make sure Ollama is running." - ) - except urllib.error.URLError as e: - if isinstance(e.reason, (socket.timeout, socket.error, ConnectionRefusedError, OSError)): + + last_error = None + for attempt in range(self.max_retries): + try: + req = urllib.request.Request( + url, + data=payload, + headers={"Content-Type": "application/json"}, + method="POST", + ) + resp = urllib.request.urlopen(req, timeout=effective_timeout) + if stream: + return resp + return json.loads(resp.read().decode("utf-8")) + except socket.timeout as e: + last_error = e + if attempt < self.max_retries - 1: + time.sleep(self.retry_delay * (2 ** attempt)) + continue + raise OllamaError( + f"Timeout connecting to Ollama at {self.base_url} " + f"(waited {effective_timeout}s after {self.max_retries} attempts). " + f"Make sure Ollama is running." + ) + except urllib.error.URLError as e: + if isinstance(e.reason, (socket.timeout, socket.error, ConnectionRefusedError, OSError)): + last_error = e + if attempt < self.max_retries - 1: + time.sleep(self.retry_delay * (2 ** attempt)) + continue + raise OllamaError( + f"Cannot connect to Ollama at {self.base_url} " + f"after {self.max_retries} attempts. " + f"Make sure Ollama is running: {e}" + ) raise OllamaError( f"Cannot connect to Ollama at {self.base_url}. " f"Make sure Ollama is running: {e}" ) - raise OllamaError( - f"Cannot connect to Ollama at {self.base_url}. " - f"Make sure Ollama is running: {e}" - ) - except urllib.error.HTTPError as e: - body = e.read().decode("utf-8", errors="replace") - raise OllamaError(f"Ollama API error ({e.code}): {body}") + except urllib.error.HTTPError as e: + body = e.read().decode("utf-8", errors="replace") + raise OllamaError(f"Ollama API error ({e.code}): {body}") def check_health(self) -> bool: """Check if Ollama is running and the model is available.""" From 28d39bc767c023ed6432defc3781e34d46858cec Mon Sep 17 00:00:00 2001 From: "claude[bot]" <41898282+claude[bot]@users.noreply.github.com> Date: Tue, 8 Sep 2026 14:06:10 +0000 Subject: [PATCH 2/2] fix(review): only retry connection-refused/DNS errors, fail fast on timeout Retrying socket.timeout in _request was tripling the hang time for a slow Ollama generation (120s -> 363s) and could queue duplicate concurrent generations against a struggling server. Narrow the retry to errors that mean the connection never got established (ConnectionRefusedError, socket.gaierror); timeouts and other URLErrors fail fast as before. Also drop the dead last_error variable in favor of `raise ... from e` to preserve exception context, and reorder the HTTPError/URLError except clauses so HTTPError (a URLError subclass) is actually reachable and its response body reaches the caller instead of being reported as a generic connection failure. --- agent/core/llm.py | 32 ++++++++++++------------- tests/test_llm.py | 60 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 16 deletions(-) diff --git a/agent/core/llm.py b/agent/core/llm.py index 02f0ad2..2a744db 100644 --- a/agent/core/llm.py +++ b/agent/core/llm.py @@ -79,8 +79,7 @@ def _request( url = f"{self.base_url}{endpoint}" payload = json.dumps(data).encode("utf-8") effective_timeout = timeout if timeout is not None else self.config.timeout - - last_error = None + for attempt in range(self.max_retries): try: req = urllib.request.Request( @@ -94,18 +93,19 @@ def _request( return resp return json.loads(resp.read().decode("utf-8")) except socket.timeout as e: - last_error = e - if attempt < self.max_retries - 1: - time.sleep(self.retry_delay * (2 ** attempt)) - continue + # A timeout means Ollama is still working, not that the + # connection failed - retrying would just queue up more + # concurrent work against an already-struggling server. raise OllamaError( f"Timeout connecting to Ollama at {self.base_url} " - f"(waited {effective_timeout}s after {self.max_retries} attempts). " - f"Make sure Ollama is running." - ) + f"(waited {effective_timeout}s). Make sure Ollama is running." + ) from e + except urllib.error.HTTPError as e: + # Must be checked before URLError: HTTPError subclasses it. + body = e.read().decode("utf-8", errors="replace") + raise OllamaError(f"Ollama API error ({e.code}): {body}") from e except urllib.error.URLError as e: - if isinstance(e.reason, (socket.timeout, socket.error, ConnectionRefusedError, OSError)): - last_error = e + if isinstance(e.reason, (ConnectionRefusedError, socket.gaierror)): if attempt < self.max_retries - 1: time.sleep(self.retry_delay * (2 ** attempt)) continue @@ -113,14 +113,14 @@ def _request( f"Cannot connect to Ollama at {self.base_url} " f"after {self.max_retries} attempts. " f"Make sure Ollama is running: {e}" - ) + ) from e raise OllamaError( f"Cannot connect to Ollama at {self.base_url}. " f"Make sure Ollama is running: {e}" - ) - except urllib.error.HTTPError as e: - body = e.read().decode("utf-8", errors="replace") - raise OllamaError(f"Ollama API error ({e.code}): {body}") + ) from e + + # Unreachable: every loop iteration either returns or raises above. + raise OllamaError(f"Failed to reach Ollama at {self.base_url}.") def check_health(self) -> bool: """Check if Ollama is running and the model is available.""" diff --git a/tests/test_llm.py b/tests/test_llm.py index e9955ef..85ebd38 100644 --- a/tests/test_llm.py +++ b/tests/test_llm.py @@ -1,8 +1,11 @@ """Tests for the Ollama LLM client response validation.""" +import io import json +import socket import sys import unittest +import urllib.error from pathlib import Path from unittest.mock import patch @@ -166,5 +169,62 @@ def test_non_dict_message_is_skipped(self): self.assertEqual(result["message"]["content"], "ok") +class TestRequestRetry(unittest.TestCase): + """Test the retry behavior in LLMClient._request.""" + + def test_connection_refused_is_retried_and_eventually_raises(self): + client = make_client() + error = urllib.error.URLError(ConnectionRefusedError()) + with patch("urllib.request.urlopen", side_effect=error) as mock_urlopen, \ + patch("time.sleep") as mock_sleep: + with self.assertRaises(OllamaError): + client._request("/api/chat", {}) + self.assertEqual(mock_urlopen.call_count, client.max_retries) + self.assertEqual(mock_sleep.call_count, client.max_retries - 1) + + def test_dns_failure_is_retried_and_eventually_raises(self): + client = make_client() + error = urllib.error.URLError(socket.gaierror()) + with patch("urllib.request.urlopen", side_effect=error) as mock_urlopen, \ + patch("time.sleep"): + with self.assertRaises(OllamaError): + client._request("/api/chat", {}) + self.assertEqual(mock_urlopen.call_count, client.max_retries) + + def test_socket_timeout_fails_fast_without_retry(self): + """A timeout means Ollama is still generating - retrying would only + make a slow response slower, so it must not be retried.""" + client = make_client() + with patch("urllib.request.urlopen", side_effect=socket.timeout()) as mock_urlopen, \ + patch("time.sleep") as mock_sleep: + with self.assertRaises(OllamaError): + client._request("/api/chat", {}) + self.assertEqual(mock_urlopen.call_count, 1) + mock_sleep.assert_not_called() + + def test_non_connection_url_error_fails_fast_without_retry(self): + client = make_client() + error = urllib.error.URLError("ssl handshake failed") + with patch("urllib.request.urlopen", side_effect=error) as mock_urlopen, \ + patch("time.sleep") as mock_sleep: + with self.assertRaises(OllamaError): + client._request("/api/chat", {}) + self.assertEqual(mock_urlopen.call_count, 1) + mock_sleep.assert_not_called() + + def test_http_error_surfaces_response_body(self): + """HTTPError subclasses URLError, so its handler must be checked + first or the actual API error body never reaches the caller.""" + client = make_client() + http_error = urllib.error.HTTPError( + "http://x", 400, "Bad Request", {}, io.BytesIO(b'{"error": "bad model"}') + ) + with patch("urllib.request.urlopen", side_effect=http_error): + with self.assertRaises(OllamaError) as ctx: + client._request("/api/chat", {}) + self.assertIn("bad model", str(ctx.exception)) + self.assertIn("400", str(ctx.exception)) + + if __name__ == "__main__": unittest.main()