Add timeout and retry logic to Ollama requests

[ssevera1]

What

Add exponential backoff retry logic (up to 3 attempts) and request timeout handling to the _request method to gracefully handle network failures and unresponsive Ollama endpoints.

Why

Prevents hanging or crashing when the Ollama service is slow or temporarily unreachable, improving reliability and user experience.

[claude]

Thanks for tackling this — the connection-refused retry is genuinely useful. But as written the retry loop makes the "Ollama is slow" case notably worse, which is the case the PR description says it fixes. Details below.

1. Blocking: retrying on socket.timeout triples the hang instead of preventing it (agent/core/llm.py:96-105)

urllib.request.urlopen(timeout=...) applies to the socket for both connect and read, so a socket.timeout on /api/chat or /api/generate overwhelmingly means "the model is still generating," not "the network blipped." Two consequences:

• Wall-clock regression. LLMConfig.timeout defaults to 120 (agent/core/config.py:24, config.yaml:13). A slow generation used to fail after 120s; it now blocks for 120 + 1 + 120 + 2 + 120 = 363s before surfacing the same error. The PR body says this "prevents hanging when the Ollama service is slow" — it does the opposite.
• It amplifies the overload. These are non-idempotent POSTs. The server is still working on attempt 1's generation when attempt 2 arrives, so a struggling Ollama instance gets three concurrent full generations queued against it. /api/pull (llm.py:294) is worse: a multi-GB pull that exceeds the timeout now restarts from scratch three times.

Suggested fix: only retry errors that mean the connection was never established — e.g. URLError whose reason is ConnectionRefusedError or socket.gaierror — and let socket.timeout fail fast on the first attempt as before. If you do want timeout retries, they should at minimum be opt-in and off by default for the generation endpoints.

Relatedly, agent/core/llm.py:107's isinstance guard includes bare OSError, which is a superclass of socket.timeout, socket.error and ConnectionRefusedError — so the tuple is effectively just OSError and the retry path catches far more than connection failures.

2. The retry doesn't cover the primary code path (agent/core/llm.py:93-94)

For stream=True, urlopen returns as soon as response headers arrive, so the retry loop only guards header receipt. Timeouts during body iteration are raised inside _stream_chat and caught at agent/core/llm.py:243, entirely outside the loop. Since streaming chat is the main path, the retry adds little there — worth calling out so the change isn't assumed to cover more than it does.

3. last_error is dead (agent/core/llm.py:83, :97, :108)

It's assigned on every failure and never read. Either drop it, or use it to chain context via raise OllamaError(...) from last_error — the current raise statements lose the original exception's __cause__.

4. No test coverage

tests/test_llm.py only patches _request wholesale, so nothing exercises the new loop. Please add a test asserting attempt counts for a retryable error vs. a non-retryable one, with time.sleep patched so the suite stays fast. This is also the natural place to pin down the intended timeout behavior from point 1.

5. Pre-existing, but this diff rewrote the block (agent/core/llm.py:121-123)

urllib.error.HTTPError subclasses URLError, so the URLError handler at :106 always wins and the HTTPError handler is unreachable. HTTPError.reason returns self.msg (a str), so the guard at :107 is False and every HTTP 4xx/5xx from Ollama is reported as "Cannot connect to Ollama... Make sure Ollama is running" rather than the actual API error body. This is on main already and not a regression from this PR, so it's your call whether to fix here — but the fix is just moving the HTTPError clause above the URLError clause, and this diff is already touching these exact lines.

Points 1 and 3 are what I'd want addressed before merge; 2, 4 and 5 are worth a look while you're in here.

[claude]

Verified each point against the code before making changes.

Fixed:

1. Retrying socket.timeout made the slow-Ollama case worse (point 1). Confirmed: urlopen(timeout=...) covers both connect and read, so a timeout on /api/chat usually means the model is still generating, not a network blip. With max_retries=3 and the default 120s timeout, that turned a 120s failure into a 363s one, and queued duplicate concurrent generations against an already-struggling server. Narrowed the retry to URLError whose reason is ConnectionRefusedError or socket.gaierror (connection never established); socket.timeout and other URLErrors now fail fast on the first attempt, same as before this feature was added.

2. last_error was dead and exception context was lost (point 3). Removed it — the original exception (e) is already in scope where it's needed, and every raise now uses from e to preserve __cause__.

3. HTTPError handler was unreachable (point 5). Confirmed pre-existing on main (not introduced by this PR), but since the diff already rewrites this exact block, fixed it here: moved the HTTPError except clause above URLError (it's a subclass, so it was always being swallowed by the URLError branch), so Ollama API error bodies (e.g. HTTP 400/500) now actually reach the caller instead of being reported as "Cannot connect to Ollama."

4. No test coverage (point 4). Added TestRequestRetry in tests/test_llm.py: attempt-count assertions for connection-refused and DNS-failure (retried max_retries times) vs. timeout and other URLErrors (fail on first attempt, no sleep call), plus a regression test for the HTTPError body-surfacing fix. time.sleep is patched so the suite stays fast.

Not changed:

• Point 2 (streaming doesn't benefit from the retry loop) — confirmed correct as an observation (urlopen returns once headers arrive for stream=True, so timeouts during body iteration are caught separately in _stream_chat at the old line 243, outside the loop) but it's not something to "fix" here — retrying a partially-consumed stream isn't safe to do transparently, so I left it as documented behavior rather than expanding scope.

Bonus: rewriting the retry loop also introduced a mypy failure not mentioned in the review (Missing return statement — the for loop had no statically-provable exit on all paths) plus a pre-existing type error on the now-removed last_error assignment. Added an unreachable-but-type-satisfying fallback raise after the loop.

Checks: pytest -q → 78 passed. mypy --ignore-missing-imports agent → Success, no issues.

[claude]

Reviewed the diff in agent/core/llm.py and tests/test_llm.py.

What I checked:

• Exception ordering (llm.py:95-107): socket.timeout is TimeoutError (not a URLError subclass) so it doesn't shadow the later clauses, and HTTPError is now caught before URLError. That's a real fix — on main the HTTPError clause was dead code, so API errors were reported as "Cannot connect to Ollama" and the response body never reached the caller.
• Retry safety (llm.py:108): retries are gated to ConnectionRefusedError/socket.gaierror only — cases where the request never reached the server — so re-POSTing to /api/chat, /api/generate and /api/pull can't duplicate server-side work. Timeouts deliberately fail fast, which is correct for a model that may still be generating.
• Loop bounds: attempt < max_retries - 1 gives 3 attempts and 2 backoff sleeps (1s, 2s); the trailing raise at llm.py:123 is unreachable-by-construction and only satisfies the return type. Request is rebuilt per attempt rather than reused.
• No collateral: time was already imported on main, and no caller of _request (llm.py:206/216/272/294) or any other module depends on the changed message strings. The timeout message now uses effective_timeout, so per-call overrides are reported accurately.
• Tests cover both retried paths, both fail-fast paths, and that the HTTP error body surfaces.

Scope is limited to the _request method and its tests, matching the PR description. Approving; merging on green CI.