ollama-proxy: make response-header timeout configurable - #52
Open
pettersandvand wants to merge 5 commits into
Open
pettersandvand wants to merge 5 commits into
pettersandvand wants to merge 5 commits into
Conversation
proxyResponseTimeout was a hardcoded 120s constant governing how long the proxy waits for a backend engine (Ollama/LM Studio) to start sending a response for a real forwarded request (e.g. /v1/chat/completions). There was no flag or env var to change it, so any backend response that takes longer than 120s to start (a cold model load, a long prefill, or a tool-calling generation that an engine only flushes once complete) fails with "net/http: timeout awaiting response headers", even though the backend is healthy and would have answered. Add a --response-timeout duration flag (default 120s, matching prior behavior exactly) and thread it through both the plain local transport and the per-peer cluster-mTLS transport, since both were governed by the same constant. Existing default behavior is unchanged unless the flag is set. Adds tests covering the default, a custom override applied to the transport, and rejection of non-positive values. Updates the README flags table. Signed-off-by: crantz <petter.johan@live.no>
Author
|
Related: #25 — same underlying pattern (a hardcoded response-header timeout with no config surface), but a different service/code path. #25 is about |
The previous commit added ollama-proxy's --response-timeout flag, but nothing passed it: the broker's spawnProxy() built ollama-proxy's args itself and never included it, so a packaged install (broker spawned by the desktop app, which has no knowledge of this flag either) saw no behavior change at all. Add --proxy-response-timeout to nvpair-ui-broker (default 5m, up from ollama-proxy's own 120s default) and have spawnProxy() pass it through as ollama-proxy's --response-timeout. This means the effective default for anyone running through the broker (packaged app included, since the desktop app doesn't need to change anything to benefit) becomes 5m instead of 120s, while still being overridable via the new broker flag for anyone who wants a different value. Signed-off-by: crantz <petter.johan@live.no>
--response-timeout 0 now explicitly means "no timeout", matching http.Transport's own ResponseHeaderTimeout semantics, rather than being silently ignored. Some backends (a cold load on a very large model, a long prefill on a large context, or an engine that only flushes once a tool call finishes composing) can legitimately exceed any fixed bound; this makes "wait indefinitely" an explicit, opt-in choice rather than something inferred from an arbitrarily large duration. Negative values remain invalid and are still ignored. --proxy-response-timeout 0 on the broker threads the same choice through to the packaged app. This is a real tradeoff, not a free upgrade: with no timeout, a backend that hangs outright (rather than eventually erroring) will hang the forwarded request indefinitely too, with no automatic failover to another candidate. Documented in both flags' help text. To make that tradeoff visible either way, wrap each candidate's transport in a loggingRoundTripper that logs "still awaiting response headers" every 30s while a request is in flight. This fires whether or not a timeout is configured, so a long wait shows up as periodic progress in the logs instead of silence that looks identical to a hang — most useful exactly when the timeout is raised or disabled, since nothing else reports liveness until the request finally succeeds or fails. Signed-off-by: crantz <petter.johan@live.no>
Matches idleClientWriteTimeout's existing rationale in this file: a var instead of a const only so a test can shorten it and observe a log line without waiting 30 real seconds. Production never reassigns it. Verified end-to-end against a genuinely slow/silent fake backend (httptest server that writes zero bytes for N seconds before answering, mirroring what Ollama actually does during tool-call composition): a short --response-timeout still fails fast as before, and --response-timeout 0 waits out the silence, delivers the real response, and logs "still awaiting response headers" progress while waiting. Signed-off-by: crantz <petter.johan@live.no>
Adds a "Proxy response timeout" control to Service Settings, mirroring the existing Log level setting's persistence pattern: a ui-config.json field (getProxyResponseTimeoutMinutes/setProxyResponseTimeoutMinutes), a service:get/set-proxy-response-timeout IPC pair, and a supervisor field seeded by the connector before spawn. Unlike log level this has no live JSON-RPC fan-out, so it applies on the next service restart, matching the apply-by-restart pattern already used for engine port changes. Stored as minutes (default 5, matching the broker's own hardcoded default) and rendered to the broker's --proxy-response-timeout flag as a Go duration string (e.g. "5m"). The UI explains that 0 means waiting indefinitely with no automatic failover. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Collaborator
|
Thanks - timeout units in minutes as a granularity seem too coarse, can we make them seconds? Moving MR against development branch. |
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
2 participants
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.
Summary
proxyResponseTimeout(services/ollama-proxy/proxy.go) is a hardcoded 120s constant used as theResponseHeaderTimeouton the transport that forwards real client requests (e.g./v1/chat/completions) to a chosen backend node. There's no flag or env var to change it.In practice this means any backend response that legitimately takes longer than 120s to start — a cold model load on a large local model, a long prefill, or an engine (e.g. Ollama) that only flushes output once a tool-call has fully composed — fails with:
even though the backend is healthy and would have answered given more time. This reproduces reliably with larger local models (e.g. 27B+) doing tool-calling generations.
Change
--response-timeoutduration flag toservices/ollama-proxy/main.go(default120s, matching current behavior exactly — no change unless explicitly set).Proxyvia a newSetResponseTimeoutmethod, applied to both the plain local forwarding transport and the per-peer cluster-mTLS transport (newProxyTransport), since both were previously governed by the same hardcoded constant.modelListClient, fixed atproxyDialTimeout= 10s) untouched — it's intentionally a quick health/listing probe, not the request-forwarding path.Testing
go build ./...— cleango vet ./...— cleango test ./...— all tests pass except one pre-existing, unrelated failure (a loopback-alias test requiring a127.0.0.2bind, which fails identically on unmodifiedmainin this sandbox — confirmed viagit stash).Notes
Happy to adjust the flag name/shape (e.g. accept it in ms instead of a Go duration string, or expose it per-engine rather than globally) if maintainers have a preferred convention.