Skip to content

Commit d8cb143

Browse files
authored
fix(server): configurable HTTP timeouts + don't evict upstream on call-scoped cancellation (#968)
* fix(server): configurable HTTP timeouts + don't evict upstream on call-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. * fix(server): review round 2 — per-route deadline exemption, probe hardening (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. * fix(server): review round 3 — close epoch TOCTOU, document idle-timeout 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. * fix(server): review round 4 — TransitionTo out of epochMu, idle-fallback 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. * fix(server): review round 5 — Disconnect invalidates the probe epoch (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. * test(upstream): drive the real Disconnect in the stale-probe regression 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.
1 parent 6411a5f commit d8cb143

15 files changed

Lines changed: 1479 additions & 29 deletions

docs/configuration.md

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,36 @@ A running MCPProxy core watches `mcp_config.json` and hot-reloads external edits
108108
| `call_tool_timeout` | string | `"2m"` | Timeout for tool calls (e.g., `"30s"`, `"2m"`, `"5m"`). **Note**: When using agents like Codex or Claude as MCP servers, you may need to increase this timeout significantly, even up to 10 minutes (`"10m"`), as these agents may require longer processing times for complex operations |
109109
| `init_timeout` | duration | `"30s"` | Deadline for an upstream's MCP `initialize` handshake (e.g. `"30s"`, `"120s"`, `"3m"`). Raise this for servers that do legitimate first-run warmup — building a cache/index or prefetching — before they answer `initialize`, so they are not killed mid-startup. Global default; can be overridden per server (see [Server Fields](#server-fields)). Range: `1s``30m`; `"0s"`/unset uses the 30s default. |
110110

111+
### HTTP Server Timeouts
112+
113+
Deadlines applied to mcpproxy's own HTTP listener (REST API, `/mcp`, `/events`).
114+
These are separate from `call_tool_timeout`, which caps how long an *upstream
115+
tool* may run.
116+
117+
```json
118+
{
119+
"http_read_timeout": "120s",
120+
"http_write_timeout": "120s",
121+
"http_idle_timeout": "180s"
122+
}
123+
```
124+
125+
| Field | Type | Default | Description |
126+
|-------|------|---------|-------------|
127+
| `http_read_timeout` | duration | `"120s"` | Deadline for reading the entire request (headers + body). `"0s"` = no timeout. Range: `1s``24h`. |
128+
| `http_write_timeout` | duration | `"120s"` | Wall-clock cap on writing the entire response, counted from when the request headers were read. **Governs non-streaming endpoints only** (REST API, Web UI, health) — the MCP endpoints and the SSE `/events` stream are exempt by design (see below, [#965](https://github.com/smart-mcp-proxy/mcpproxy-go/issues/965)). `"0s"` disables it globally. Range: `1s``24h`. |
129+
| `http_idle_timeout` | duration | `"180s"` | Keep-alive timeout for idle persistent connections. `"0s"` removes the dedicated idle deadline, but Go's `net/http` then falls back to the read timeout — idle is fully unbounded only when `http_read_timeout` is also `"0s"`. Range: `1s``24h`. |
130+
131+
Notes:
132+
133+
- **Streaming routes are exempt from `http_write_timeout`.** A write deadline caps the whole response, so it would truncate any tool call slower than it and silently kill long-lived SSE streams. The MCP endpoints (`/mcp`, `/mcp/all`, `/mcp/code`, `/mcp/call`, `/mcp/p/<slug>`, plus the legacy `/v1/tool_code` and `/v1/tool-code` aliases) and `/events` therefore clear their own per-request write deadline (and, being body-less GETs, their read deadline). Everything else keeps the configured deadline, which is what protects a non-loopback deployment from slow readers. You do **not** need to disable `http_write_timeout` to run long tool calls ([#965](https://github.com/smart-mcp-proxy/mcpproxy-go/issues/965)).
134+
- **`"0s"` means "no timeout"**, not "use the default" — unlike `init_timeout`. Omit the key entirely to get the built-in default. Setting `http_write_timeout` to `"0s"` removes the deadline from *every* endpoint, including REST/UI/health. Exception: `http_idle_timeout: "0s"` alone does not unbound idle connections — Go's `net/http` falls back to the read timeout (see the field row above).
135+
- **A restart is required.** These values are baked into the HTTP server when it binds, so a config edit is reported as restart-required rather than hot-reloaded.
136+
- **Slowloris protection is unaffected**: the 60s request-header read deadline is hardcoded and not configurable.
137+
- **Long tool calls need `call_tool_timeout`.** It (default `"2m"`) separately caps tool execution. To allow tool calls longer than two minutes, raise `call_tool_timeout` — the MCP routes' write-deadline exemption alone is not enough.
138+
139+
Environment overrides: `MCPPROXY_HTTP_READ_TIMEOUT`, `MCPPROXY_HTTP_WRITE_TIMEOUT`, `MCPPROXY_HTTP_IDLE_TIMEOUT`.
140+
111141
### TOON Output (Adaptive Result Encoding)
112142

113143
```json

docs/configuration/config-file.md

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,9 @@ MCPProxy uses a JSON configuration file located at `~/.mcpproxy/mcp_config.json`
2929
"enable_socket": true,
3030
"health_check_interval": "30s",
3131
"tool_discovery_interval": "5m",
32+
"http_read_timeout": "120s",
33+
"http_write_timeout": "120s",
34+
"http_idle_timeout": "180s",
3235
"tools_limit": 15,
3336
"tool_response_limit": 20000,
3437
"enable_code_execution": false,
@@ -59,6 +62,25 @@ MCPProxy uses a JSON configuration file located at `~/.mcpproxy/mcp_config.json`
5962
| `require_mcp_auth` | boolean | `false` | Require an API key on the `/mcp` endpoint (off by default for client compatibility). Enable when exposing MCPProxy beyond localhost |
6063
| `enable_socket` | boolean | `true` | Enable Unix socket/named pipe for local communication |
6164

65+
### HTTP Server Timeouts
66+
67+
Deadlines applied to MCPProxy's own HTTP listener (REST API, `/mcp`, `/events`).
68+
Each accepts a duration string; **`"0s"` means "no timeout"** (not "use the
69+
default" — omit the key for that; for `http_idle_timeout`, `"0s"` falls back to
70+
the read timeout — see its row). Valid range: `1s``24h`, or `0s`.
71+
72+
| Option | Type | Default | Description |
73+
|--------|------|---------|-------------|
74+
| `http_read_timeout` | duration | `"120s"` | Deadline for reading the whole request (headers + body) |
75+
| `http_write_timeout` | duration | `"120s"` | Wall-clock cap on writing the whole response, counted from when the request headers were read. Governs **non-streaming endpoints only** (REST API, Web UI, health); MCP endpoints and SSE `/events` are exempt by design. `"0s"` disables it globally ([#965](https://github.com/smart-mcp-proxy/mcpproxy-go/issues/965)) |
76+
| `http_idle_timeout` | duration | `"180s"` | Keep-alive timeout for idle persistent connections (`"0s"` falls back to the read timeout; unbounded only if that is also `"0s"`) |
77+
78+
- **Streaming routes are exempt from `http_write_timeout`.** The MCP endpoints (`/mcp*`, plus the legacy `/v1/tool_code` and `/v1/tool-code` aliases) and `/events` clear their own per-request write deadline (and, being body-less GETs, their read deadline), so a slow tool call or a long-lived SSE stream is never truncated. You do not need to disable the deadline to run long tool calls.
79+
- **Restart required.** These are baked into the HTTP server when it binds, so a change is reported as restart-required, not hot-reloaded.
80+
- **Slowloris protection is unaffected** — the 60s request-header read deadline is hardcoded and not configurable.
81+
- **Long tool calls need `call_tool_timeout`.** It (default `2m`) separately caps tool execution; raise it when you expect tool calls longer than two minutes.
82+
- Environment overrides: `MCPPROXY_HTTP_READ_TIMEOUT`, `MCPPROXY_HTTP_WRITE_TIMEOUT`, `MCPPROXY_HTTP_IDLE_TIMEOUT`.
83+
6284
### Feature Flags
6385

6486
| Option | Type | Default | Description |
@@ -204,6 +226,8 @@ See [Upstream Servers](/configuration/upstream-servers) for detailed server conf
204226

205227
MCPProxy watches the configuration file for changes and automatically reloads when modifications are detected. No restart is required for most configuration changes.
206228

229+
Exceptions that require a restart include `listen`, `data_dir`, `api_key`, the TLS block, and the three `http_*_timeout` options.
230+
207231
## Environment Variable Overrides
208232

209233
Configuration options can be overridden using environment variables. See [Environment Variables](/configuration/environment-variables) for details.

docs/configuration/environment-variables.md

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,25 @@ set and per-server overrides are file/API-configured. See
7575
| `MCPPROXY_QUEUE_SIZE` | How many calls may wait for a global slot. `0` = shed immediately at the cap | `0` |
7676
| `MCPPROXY_QUEUE_TIMEOUT` | How long a call may wait before being shed, e.g. `30s` | `30s` when the limiter is active |
7777

78+
### HTTP Server Timeouts
79+
80+
Deadlines on MCPProxy's own HTTP listener (REST API, `/mcp`, `/events`). Each
81+
takes a duration string; **`0s` means "no timeout"** (unset means "use the
82+
default"; for the idle timeout, `0s` falls back to the read timeout — see its
83+
row). Valid range: `1s``24h`, or `0s`. Malformed values are ignored with a
84+
warning on stderr. Changing any of these requires a restart. See
85+
[HTTP Server Timeouts](./config-file.md#http-server-timeouts).
86+
87+
| Variable | Description | Default |
88+
|----------|-------------|---------|
89+
| `MCPPROXY_HTTP_READ_TIMEOUT` | Deadline for reading the whole request (headers + body) | `120s` |
90+
| `MCPPROXY_HTTP_WRITE_TIMEOUT` | Wall-clock cap on writing the whole response for non-streaming endpoints (REST, Web UI, health). MCP endpoints and SSE `/events` are exempt by design, so slow tool calls and event streams are never truncated; `0s` disables it globally ([#965](https://github.com/smart-mcp-proxy/mcpproxy-go/issues/965)) | `120s` |
91+
| `MCPPROXY_HTTP_IDLE_TIMEOUT` | Keep-alive timeout for idle persistent connections (`0s` falls back to the read timeout; unbounded only if that is also `0s`) | `180s` |
92+
93+
The 60s request-header read deadline (slowloris protection) is hardcoded and not
94+
configurable. `call_tool_timeout` (default `2m`) separately caps tool execution —
95+
raise it too when you expect tool calls longer than two minutes.
96+
7897
### Core Server Examples
7998

8099
```bash

internal/config/config.go

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,8 +67,72 @@ const (
6767
// `initialize` handshake when no per-server or global override is set
6868
// (MCP-3322 / GH #760). It preserves the historical ~30s behaviour.
6969
defaultInitTimeout = 30 * time.Second
70+
71+
// Built-in defaults for the HTTP server's request deadlines (GH #965).
72+
// Like the intervals above they live here (not in DefaultConfig) so an
73+
// unset key resolves to the built-in behaviour and existing configs are
74+
// unchanged.
75+
defaultHTTPReadTimeout = 120 * time.Second
76+
// defaultHTTPWriteTimeout stays at 120s (GH #965). A write deadline is a
77+
// wall-clock cap on the ENTIRE response counted from the moment the request
78+
// headers were read, so it must NOT apply to long tool calls or event
79+
// streams — but it is real slow-reader protection for everything else, and
80+
// dropping it globally would strip that protection from REST, Web UI and
81+
// health endpoints on non-loopback deployments.
82+
//
83+
// The streaming routes are exempted per-request instead: the MCP endpoints
84+
// (/mcp*, plus the legacy /v1/tool_code and /v1/tool-code aliases) and the
85+
// SSE /events stream clear their own write deadline via
86+
// http.ResponseController, so this default never truncates them. Setting the
87+
// key to "0s" still disables the deadline globally.
88+
defaultHTTPWriteTimeout = 120 * time.Second
89+
defaultHTTPIdleTimeout = 180 * time.Second
7090
)
7191

92+
// resolveHTTPTimeout applies the tri-state contract shared by the three HTTP
93+
// server deadlines (GH #965): nil = the built-in default, a pointer to 0 =
94+
// DISABLED (net/http's zero value means "no deadline" — except IdleTimeout,
95+
// where net/http falls back to ReadTimeout; see ResolveHTTPIdleTimeout), a
96+
// positive value = that
97+
// value. A negative value falls back to the default — validation rejects those
98+
// anyway, this only keeps a hand-edited file from producing a nonsense deadline.
99+
//
100+
// Note the deliberate asymmetry with ResolveInitTimeout, where 0 maps back to
101+
// the default: a connect handshake must always have a ceiling, whereas "no
102+
// response deadline at all" is a legitimate HTTP setting an operator may want.
103+
func resolveHTTPTimeout(v *Duration, def time.Duration) time.Duration {
104+
if v == nil {
105+
return def
106+
}
107+
if d := v.Duration(); d >= 0 {
108+
return d
109+
}
110+
return def
111+
}
112+
113+
// ResolveHTTPReadTimeout resolves http.Server.ReadTimeout: unset → 120s,
114+
// 0 → disabled, positive → that value (GH #965).
115+
func (c *Config) ResolveHTTPReadTimeout() time.Duration {
116+
return resolveHTTPTimeout(c.HTTPReadTimeout, defaultHTTPReadTimeout)
117+
}
118+
119+
// ResolveHTTPWriteTimeout resolves http.Server.WriteTimeout: unset → 120s,
120+
// 0 → disabled, positive → that value (GH #965). Streaming routes (MCP + SSE
121+
// /events) clear the resulting deadline per-request, so this value only
122+
// governs non-streaming endpoints — see defaultHTTPWriteTimeout.
123+
func (c *Config) ResolveHTTPWriteTimeout() time.Duration {
124+
return resolveHTTPTimeout(c.HTTPWriteTimeout, defaultHTTPWriteTimeout)
125+
}
126+
127+
// ResolveHTTPIdleTimeout resolves http.Server.IdleTimeout: unset → 180s,
128+
// 0 → no idle deadline of its own, positive → that value (GH #965). NOTE:
129+
// net/http falls back to ReadTimeout when IdleTimeout is zero, so an explicit
130+
// "0s" here fully disables the idle deadline only when http_read_timeout is
131+
// also 0 — otherwise idle connections are reaped after the read timeout.
132+
func (c *Config) ResolveHTTPIdleTimeout() time.Duration {
133+
return resolveHTTPTimeout(c.HTTPIdleTimeout, defaultHTTPIdleTimeout)
134+
}
135+
72136
// resolveInterval applies the per-server → global → default precedence for an
73137
// optional *Duration. A non-nil pointer wins at each level, including a pointer
74138
// to 0 ("disabled"). Returns the resolved duration; a value <= 0 means the
@@ -227,6 +291,38 @@ type Config struct {
227291
// raise this so they are not killed mid-startup.
228292
InitTimeout *Duration `json:"init_timeout,omitempty" mapstructure:"init-timeout" swaggertype:"string"`
229293

294+
// HTTP server request deadlines (GH #965). *Duration tri-state: nil =
295+
// inherit the built-in default; a pointer to 0s = DISABLED (no deadline —
296+
// with the idle-timeout caveat noted on HTTPIdleTimeout); a positive value
297+
// = that deadline. Validated to {0} ∪ [1s, 24h].
298+
//
299+
// Unlike init_timeout, an explicit 0 here is a SUPPORTED value, not a
300+
// synonym for the default: net/http treats a zero deadline as "no timeout".
301+
// Long-running tool calls and the SSE /events stream do not need that
302+
// escape hatch, though — the MCP endpoints (/mcp*, plus the legacy
303+
// /v1/tool_code and /v1/tool-code aliases) and /events clear their own
304+
// per-request write deadline via http.ResponseController, so the write
305+
// default only governs non-streaming endpoints (REST, Web UI, health).
306+
//
307+
// These are baked into http.Server at bind time, so changing any of them
308+
// REQUIRES A RESTART (DetectConfigChanges reports it as such). Resolved by
309+
// ResolveHTTPReadTimeout / ResolveHTTPWriteTimeout / ResolveHTTPIdleTimeout.
310+
// Note that call_tool_timeout separately caps tool execution (default 2m):
311+
// raise it too when allowing tool calls longer than two minutes.
312+
313+
// HTTPReadTimeout caps how long reading a whole request (headers + body)
314+
// may take. Unset = 120s; "0s" disables it. Requires a restart.
315+
HTTPReadTimeout *Duration `json:"http_read_timeout,omitempty" mapstructure:"http-read-timeout" swaggertype:"string"`
316+
// HTTPWriteTimeout caps how long producing a whole response may take on
317+
// non-streaming endpoints (REST, Web UI, health). Unset = 120s; "0s"
318+
// disables it globally. MCP and SSE /events routes are exempt by design.
319+
HTTPWriteTimeout *Duration `json:"http_write_timeout,omitempty" mapstructure:"http-write-timeout" swaggertype:"string"`
320+
// HTTPIdleTimeout caps how long an idle keep-alive connection is kept open.
321+
// Unset = 180s. "0s" removes the dedicated idle deadline, but net/http then
322+
// falls back to ReadTimeout — idle is fully unbounded only when
323+
// http_read_timeout is also "0s". Requires a restart.
324+
HTTPIdleTimeout *Duration `json:"http_idle_timeout,omitempty" mapstructure:"http-idle-timeout" swaggertype:"string"`
325+
230326
// Environment configuration for secure variable filtering
231327
Environment *secureenv.EnvConfig `json:"environment,omitempty" mapstructure:"environment"`
232328

@@ -2021,6 +2117,18 @@ func (c *Config) ValidateDetailed() []ValidationError {
20212117
errors = append(errors, *e)
20222118
}
20232119

2120+
// HTTP server request deadlines (GH #965). {0} ∪ [1s, 24h]; 0 means
2121+
// "no deadline", which validateIntervalBound already accepts.
2122+
if e := validateIntervalBound("http_read_timeout", c.HTTPReadTimeout, time.Second, 24*time.Hour); e != nil {
2123+
errors = append(errors, *e)
2124+
}
2125+
if e := validateIntervalBound("http_write_timeout", c.HTTPWriteTimeout, time.Second, 24*time.Hour); e != nil {
2126+
errors = append(errors, *e)
2127+
}
2128+
if e := validateIntervalBound("http_idle_timeout", c.HTTPIdleTimeout, time.Second, 24*time.Hour); e != nil {
2129+
errors = append(errors, *e)
2130+
}
2131+
20242132
// Concurrency limits, all three scopes after resolution (spec 093, FR-023).
20252133
errors = append(errors, c.validateConcurrency()...)
20262134

0 commit comments

Comments
 (0)