fix(ollama): accept Ollama's own OLLAMA_HOST format, and keep the timeout armed through the body read - #88
Merged
Conversation
…eout armed through the body read
Two defects on the offline on-ramp, both on the path a signed-out user hits first.
1. Ollama's convention for OLLAMA_HOST is a scheme-less host:port — that is what
`ollama serve` prints and binds. The client concatenated the value raw, so
OLLAMA_HOST=127.0.0.1:11434 produced fetch("127.0.0.1:11434/v1/chat/completions")
-> "Failed to parse URL", which the unreachable handler then reported as
"Cannot reach Ollama ... or set OLLAMA_HOST" — the exact thing the user had
just done. Every local turn failed, and `smoke` plus the handoff demo were
equally unnormalized, so they echoed the same broken value instead of
diagnosing it.
Adds one exported pure function, normalizeOllamaHost(): trims, adds http://
when there is no scheme, maps the 0.0.0.0 bind address to a connectable
127.0.0.1, strips trailing slashes, and rejects anything unusable with a
message naming the bad value. All three call sites route through it, and
smokeMain now reports a bad host as its own FAIL rather than as "Ollama is
down". Accepted forms are documented in COMMANDS.md.
2. The AbortController timer was cleared in the finally attached to the fetch,
i.e. as soon as response headers arrived. Because stream:false puts the whole
completion in the body, both body reads then ran with no timer and no signal:
a server that answered 200 and stalled mid-body — a loaded local GPU, a
dropped SSH tunnel — hung the turn forever instead of failing at the timeout.
Request and parse now share one try/finally, and the AbortError branch is
reachable from the body-read path, so a stalled body surfaces as the same
"timed out after Ns" message the connect path already produced.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
The defect
Two problems on the offline Ollama path — the on-ramp the README advertises for people who have not signed in ("…or skip it and run on your own Ollama"), so it is the first thing a new user without an account hits.
1.
OLLAMA_HOSTin Ollama's own format broke every local turnOllama's convention for
OLLAMA_HOSTis a scheme-lesshost:port. That is whatollama serveprints, and what it binds:127.0.0.1:11434,0.0.0.0:11434. The client took the variable and concatenated/v1/chat/completionsonto it raw, so setting the variable the way Ollama documents it produced:which fell into the "cannot reach Ollama" handler, which told the user to set
OLLAMA_HOST— the exact thing they had just done. Reproduced verbatim before the fix:All three call sites were unnormalized —
src/core/ollama.ts,src/core/smoke.ts,scripts/handoff-demo.ts— sonpm run smokeand the handoff demo echoed the same broken value back instead of diagnosing it. Impact: for a user who followed Ollama's own documentation, the entire local backend was unusable and the error message actively pointed away from the cause.2. The request timeout was disarmed before the body was read
ollamaChatarmed anAbortControllertimer, then cleared it in thefinallyattached to the fetch try/catch — i.e. as soon as response headers arrived. Both body reads,await res.text()on the error path andawait res.json()on the success path, then ran with no timer and no signal.Because the client sends
stream: false, the entire completion is in that body. So a server that returned 200 headers and then stalled mid-body — a real failure mode for a loaded local GPU or a dropped SSH tunnel in front of a remote Ollama — hung the turn forever instead of failing atDEFAULT_TIMEOUT_MS. The user saw a frozen agent indistinguishable from slow inference. Demonstrated against a stub that writes 200 headers and then never ends the body, using the pre-fix control-flow shape with a 400 ms timeout configured:What changed
src/core/ollama.ts— new exported pure functionnormalizeOllamaHost(raw):http://localhost:11434127.0.0.1:11434http://127.0.0.1:11434localhost:11434http://localhost:114340.0.0.0:11434http://127.0.0.1:114340.0.0.0is a bind address, not a connect addresshttp://localhost:11434///http://localhost:11434https://ollama.example.com/proxy/https://ollama.example.com/proxyftp://localhost:11434,:::Also in
ollama.ts: request and parse now share onetry/finally, soclearTimeouthappens after the body read rather than after the headers, and theAbortErrorbranch is reachable from both body-read paths. A stalled body now surfaces as the sameOllama request timed out after Nsmessage the connect path already produced.src/core/smoke.ts—ollamaUp()normalizes before building/api/tags, andsmokeMain()normalizes once up front. If the value is unusable, theollamaandlocal turnchecks now report aFAILcarrying the host error instead of a misleadingSKIP: not reachable. That is the behavior change that makes the doctor diagnose the problem rather than restate it.scripts/handoff-demo.ts— normalizes the host it hands to the child CLI via env.COMMANDS.md— theOLLAMA_HOSTrow now says the scheme-less form is accepted, and a new "OLLAMA_HOSTaccepted forms" table under Environment variables documents every form above.test/ollama.test.ts— 8 new tests: thenormalizeOllamaHosttable (scheme-less,0.0.0.0, trailing slashes, empty/undefined, rejection messages), an end-to-endollamaChatturn against a genuinely scheme-lesshost:port(the regression test for defect 1), a bad-host rejection assertion, and a stalled-body test against a stub that writes 200 headers and never ends — it now fails at the configured timeout in ~420 ms instead of hanging.What I deliberately did not touch
src/core/tool_executor.ts,src/core/git_commit_guard.ts,src/core/brain_cloud.tsandsrc/commands/**belong to a sibling lane.smoke.tsfeeds onemodelvalue to bothcheckLocalTurnandcheckCloudTurn, which looks like a bug (a cloud model name would be probed against Ollama). On readingcheckCloudTurnit is inert: the function doesvoid modelbecause the cloud model is server-selected today, and the parameter exists only for signature parity. There is no behavior to fix, so I left it rather than churn the signature. Worth revisiting if the cloud model ever becomes client-selected.Test evidence
The full suite was run with
TEMP/TMP/TMPDIRpointed at a directory outside any git repository, because the worktree tests otherwise interact badly with a temp directory that sits inside a checkout on this machine. That is an environment workaround, not a change to the tests.Blast radius
Contained to the local/Ollama path.
normalizeOllamaHostis a new export with no existing caller, and for every input the old code handled correctly — a fullhttp://…URL, with or without trailing slashes — it returns the same string the old trailing-slash strip did, so no working configuration changes behavior. The timeout restructure changes control flow insideollamaChatonly: the success path returns the sameChatReply, the 404 and non-2xx errors carry the same text, and the only new outcome is that a previously-infinite hang now becomes the existing timeout error. Cloud/hosted turns, transport, auth and the tool loop are untouched.