fix(providers): native ollama path honours the configured LLM timeout - #3797
Open
MasterST1337 wants to merge 2 commits into
Open
fix(providers): native ollama path honours the configured LLM timeout#3797MasterST1337 wants to merge 2 commits into
MasterST1337 wants to merge 2 commits into
Conversation
…ardcoded 300s The OpenAI-compatible path builds its client with self.timeout (from HINDSIGHT_API_LLM_TIMEOUT), but the Ollama *native* path hardcodes httpx.AsyncClient(timeout=300.0). Any deployment whose LLM needs longer than 300s per call therefore cannot be configured to work: the request is aborted mid-prompt and surfaces as 'Ollama connection error', with the configured timeout silently ignored. Observed on a CPU-only ollama host: prompt processing at ~16 tok/s meant an ~8k-token retain document needed ~460s to ingest before generating a token. Every retain failed, no fact was ever extracted, and raising HINDSIGHT_API_LLM_TIMEOUT had no effect because the stage was llm.ollama_native.*.
…00 s fallback Review follow-ups on the previous commit, per the repo's code-review skill (§6 "bug fixes SHOULD have a regression test", §4 dead code): - `self.timeout or 300.0` -> `self.timeout`. __init__ always assigns `timeout or float(getenv(ENV_LLM_TIMEOUT, DEFAULT_LLM_TIMEOUT))`, so the `or` branch was unreachable and the literal duplicated a named constant. - Spell out the direction this change cuts that the first commit did not: DEFAULT_LLM_TIMEOUT is 120 s, LOWER than the 300 s literal being removed, so a deployment that never set ENV_LLM_TIMEOUT gets a SHORTER native timeout after this change and must raise the env var. That makes this a behaviour change, not a pure bug fix, and it belongs in the comment. - Add tests/test_ollama_native_timeout.py asserting both directions: a configured 900 s survives (the bug), and an unset timeout resolves to DEFAULT_LLM_TIMEOUT rather than 300.0 (the regression guard).
Strix Security ReviewNo security issues found. Updated for Reviewed by Strix |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
_call_ollama_nativebuilt its ownhttpx.AsyncClient(timeout=300.0). It is the one request path that ignoredHINDSIGHT_API_LLM_TIMEOUT— every other path threadsself.timeout.On a CPU ollama host a single fact-extraction prompt can need longer than 300 s just to be ingested. The call is aborted mid-prompt and surfaces as a bare
Ollama connection errorthat raising the configured timeout cannot fix, because the literal is what is actually in force.Please read this bit before merging — it cuts both ways
DEFAULT_LLM_TIMEOUTis 120 s, which is lower than the 300 s literal being removed. So for a deployment that never setHINDSIGHT_API_LLM_TIMEOUT, this change shortens the native-ollama timeout from 300 s to 120 s.That is the intent — one knob, honoured everywhere — but it makes this a behaviour change, not a pure bug fix, and such a deployment would need to raise the env var. If you would rather not regress that case, the alternative is
max(self.timeout, 300.0), which keeps the old floor at the cost of the native path still not fully honouring config. I went with the strict reading; say the word and I will switch it. Both the code comment and the commit message spell this out so it is not a silent change.Commits
self.timeout or 300.0→self.timeout(__init__always assignstimeout or float(getenv(ENV_LLM_TIMEOUT, DEFAULT_LLM_TIMEOUT)), so theorbranch was unreachable and the literal duplicated a named constant), and addedtests/test_ollama_native_timeout.py.The test asserts both directions: a configured 900 s survives (the bug), and an unset timeout resolves to
DEFAULT_LLM_TIMEOUTrather than 300.0 (the regression guard, which would catch the literal being reintroduced).scripts/hooks/lint.shclean.