fix(server): configurable HTTP timeouts + don't evict upstream on call-scoped cancellation - #968
Merged
Merged
Conversation
…l-scoped cancellation (GH #965) Part 1: http_read_timeout / http_write_timeout / http_idle_timeout config keys (tri-state *Duration; 0 = no deadline; restart-required; env overridable). The write-timeout default changes from a hardcoded 120s to 0: a write deadline is a wall-clock cap on the whole response, so it truncated any tool call slower than 2 minutes and silently killed SSE /events streams. Slowloris protection stays via the 60s ReadHeaderTimeout. Part 2: managed.Client.CallTool no longer treats call-scoped context cancellation as proof of a dead server. Caller-ctx cancellation never evicts; ambiguous wrapped cancellations trigger one gated async liveness probe (probe failure = the hard evidence that does evict); genuine transport errors (refused/reset/broken pipe) evict immediately as before.
Deploying mcpproxy-docs with
|
| Latest commit: |
b540523
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d58c31c6.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://fix-965-http-timeouts-no-evi.mcpproxy-docs.pages.dev |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 31298900430 --repo smart-mcp-proxy/mcpproxy-go
|
…dening (GH #965) - Restore the 120s write-timeout default; instead of disabling it globally, the streaming routes (/mcp*, /v1/tool_code*, /events) clear their per-request write deadline (and read deadline for body-less GET/HEAD) via http.ResponseController. Long tool calls and SSE work out of the box while REST/UI/health keep slow-reader protection. - Ambiguous-cancellation probe now follows the health loop's policy: only hard transport evidence evicts; transient/non-connection ping failures defer to the background health loop's 3-strike threshold. - connectionEpoch guards the detached probe against SetError-ing a session that reconnected while the probe was in flight. - ReloadConfiguration warns when a file edit touches restart-required fields (pre-existing silent gap, now visible). - Per-field swagger doc comments so all three http_*_timeout keys are documented in the OAS spec.
…ut fallback (GH #965) - Connect now bumps connectionEpoch BEFORE exposing Ready, and both run under epochMu, paired with the probe's final check-and-SetError — a reconnect can no longer complete between the probe's staleness check and its verdict, and a new session is never visible with the old epoch. - http_idle_timeout "0s" documented accurately: net/http falls back to ReadTimeout when IdleTimeout is 0, so idle is unbounded only when http_read_timeout is also 0 (resolver, struct comment, all three docs, OAS regenerated); stale hot-reload comment fixed; resolver-contract test case added for idle-zero-alone.
…ack wording everywhere (GH #965) - Connect no longer holds epochMu across TransitionTo(Ready): the transition invokes its state-change callback synchronously, so the mutex now guards only the epoch bump (still ordered before Ready; correctness analysis in the comment). Lock invariant documented truthfully: SetError's callback is async, TransitionTo's is not. - Every remaining blanket '0s = no timeout' statement (docs x3, config const + struct comments, env WARN message) now carries the IdleTimeout→ReadTimeout fallback caveat; OAS regenerated.
…(GH #965) - Disconnect now bumps connectionEpoch under epochMu before resetting the state machine, closing the last stale-probe window: an in-flight ambiguous-call probe can no longer flip a freshly Disconnected client back to Error (which also burned a retry and emitted a bogus notification). Regression test simulates the teardown ordering. - Last two blanket '0s disables the deadline' comments (loader env sink, config test header) now carry the IdleTimeout→ReadTimeout caveat.
…on test (GH #965) The disconnect-race test now tears down through Client.Disconnect (nil core-client guard added, same fallback contract as toolInvoker/ healthProbe) instead of simulating the ordering. Honest coverage note: the sub-microsecond Reset-vs-verdict interleaving is unobservable from outside precisely because epochMu serializes it; the epoch-guard mutation is killed by the reconnect-path test, where the epoch is the only discriminator.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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.
Fixes #965.
Problem
Two independent failure modes, one visible outcome: a single slow tool call destroys its own response and takes the upstream offline for every client.
WriteTimeouton the proxy's own HTTP server (internal/server/server.go). A write deadline is a wall-clock cap on the entire response counted from header read, so any tool call slower than ~2 minutes had its response destroyed in transit — the upstream work completed, but the caller couldn't know. The same deadline silently killed long-lived SSE/eventsstreams and streamable-HTTP GET streams. No config override existed.context canceledmatchedisConnectionError's substrings, somanaged.Client.CallToolcalledStateManager.SetError— flipping the whole upstream toError, failing fast for every client, and burning reconnect budget. (fix(upstream): require N consecutive health-check timeouts before marking Error #470's consecutive-failure threshold only covers the health-check path.)Fix
Configurable HTTP timeouts + per-route deadline exemption for streaming
Three new global keys, tri-state
*Duration(unset = built-in default,"0s"= no deadline, positive = that deadline; validated{0} ∪ [1s, 24h]; restart-required; env overridesMCPPROXY_HTTP_READ/WRITE/IDLE_TIMEOUT):http_read_timeout120shttp.Server.ReadTimeouthttp_write_timeout120shttp.Server.WriteTimeouthttp_idle_timeout180shttp.Server.IdleTimeoutDefaults are unchanged — instead of relaxing them globally, the streaming routes opt out per-request:
/mcp*,/v1/tool_code|tool-code, and/eventsare wrapped in astreamingNoDeadlinemiddleware that clears the write deadline (and, for body-less GET/HEAD streams, the read deadline) viahttp.ResponseController. So:ReadHeaderTimeout: 60sstays hardcoded as the slowloris guard, POST bodies keep their read deadline).DetectConfigChangescompares the resolved values and reports these fields as restart-required.ReloadConfiguration(file-watcher path) now logs a warning when an external edit touches any restart-required field — previously silent (pre-existing gap forlisten/TLS too).A canceled call is not a dead server
managed.Client.CallToolnow classifies errors most-specific-first:ctx.Err() != nil) → call-scoped by definition; log, never touch server state.context.Canceled/DeadlineExceededor cancellation text, while the caller ctx is live) → don't evict on a guess: fire one gated async liveness probe (existinglivenessProberseam, 5s budget). The probe follows the health loop's own policy: probe healthy → server untouched; probe fails transiently (timeout/overload/non-connection error) → left to the background health loop's 3-strike threshold; probe fails with hard transport evidence → markError. AconnectionEpochcounter guards the detached probe against acting on a session that reconnected while it was in flight.SetError.isConnectionErroritself is untouched — ListTools, the health loop, and reconnect still rely on its matching.Behavior notes: on an ambiguous cancellation against a genuinely dead server,
Errorarrives after the probe (≤5s) or, if the ping only ever times out, from the health loop (~90s) — the deliberate trade for not evicting busy-but-alive servers. The probe error becomesLastErrorrather than the call error.Not in scope
Per-upstream write timeout (requested as "ideal" in the issue): an inbound listener deadline is not an upstream property — one
/mcpsession dispatches to many upstreams. A per-upstream execution deadline would belong besidecall_tool_timeout; with the streaming routes exempt from the write deadline the need mostly disappears.Tests
internal/config/http_timeouts_test.go— resolvers (incl. explicit-0-stays-disabled), validation bounds, JSON round-trip, env overrides incl. malformed values.internal/server/http_timeouts_test.go— resolution helper + real-net/httpbehavior test: a 300ms write deadline truncates a 600ms response on an unwrapped route; thestreamingNoDeadlinewrapper rescues the same response; a GET stream survives active read+write deadlines.internal/upstream/managed/calltool_cancel_test.go— production-shaped error strings: caller-cancel keepsReady; ambiguous cancel probes (healthy → staysReady, dead → becomesError); hard errors still evict immediately; probe is gated to one in flight; classifier table incl. British "cancelled" and real-worldPost "…": context deadline exceededshapes.go buildboth editions,go vet, golangci-lint v2 (.github/.golangci.yml) 0 issues,-racesuites for config/upstream/runtime/server,./scripts/test-api-e2e.sh65/65,make swaggerartifacts committed.