mirror of
https://github.com/coder/coder.git
synced 2026-11-02 13:33:30 +08:00
Michael Suchacz
cc958b73be
## Problem
During the Aug 19 dev.coder.com incident, chats stalled for up to ~15
minutes per turn. chatd reconnects to every configured MCP server on
every generation step, and one configured server
(`registry.coder.com/mcp`) was black-holing requests from the
deployment's egress IPs: TCP/requests were silently dropped, and each
connect attempt hung until the kernel gave up (~125s), far past the
nominal 10s connect budget.
The budget does not hold because go-sdk v1.7.0 detaches the context
inside `StreamableClientTransport.Connect`, and its error paths block on
HTTP work bound to that detached context. Reproduced in a test: a bare
`Connect` with a 2s deadline against a black-holed server returns after
**12s**. Step-end session cleanup (`session.Close()`) runs synchronously
in the generation loop and blocks on the same detached requests, so a
server that wedges mid-turn stalls every step boundary too.
## Changes
- **Enforce the connect budget externally** (`connectOne`): run
`Connect` + `ListTools` in a goroutine and select on the budget context.
On timeout, abandon the goroutine and leave a reaper that drains its
late result and closes any session that still materialized, so nothing
leaks and the caller returns within the budget.
- **Stop using bare `http.DefaultTransport`**: MCP traffic now rides a
cloned transport with a 5s dial timeout (converts SYN black-holes into
fast errors) and a 60s `ResponseHeaderTimeout`. `http.Client.Timeout`
stays unset so long-lived SSE streams are unaffected.
- **Close sessions in detached goroutines during cleanup**
(`ConnectAll`'s cleanup func): the sessions are discarded regardless,
and a wedged `Close` (DELETE bound to the SDK's detached context) must
not stall the generation loop at step boundaries.
One deliberate deviation from the incident plan: `ResponseHeaderTimeout`
is 60s (matching `toolCallTimeout`) instead of ~10s. The same HTTP
client serves tool-call POSTs, and a JSON-response MCP server sends no
headers until the tool finishes, so a 10s bound would falsely kill
legitimate 10-60s tools that fit today's tool-call budget. The real 10s
connect budget is enforced by the external select, not the transport.
## Tests
- Acceptance: with a black-holed server plus a healthy one configured,
`ConnectAll` returns within the budget and the healthy server's tools
are present; closing the black-holed connections makes the reaper exit
(red without the fix: 1s budget took 11s).
- A slow-but-alive server (300ms/request) still connects.
- A server that only responds after the budget: `ConnectAll` returns
promptly, the late result is reaped.
- Cleanup returns promptly while the session-teardown DELETE is wedged
server-side, and the teardown still happens in the background (red
without the fix: cleanup blocked 60s).
- Transport guard test pinning the dial/response-header bounds.
`go test ./coderd/x/chatd/...` passes (including goleak in `chatd`).
Part of the MCP connect-stall incident follow-up; observability (connect
durations in logs/debug runs) comes in a stacked follow-up PR.
> 🤖 Mux authored this PR on Mike's behalf.
<!-- mux-attribution: model=anthropic:claude-opus-4-6 thinking=high -->
90 lines
2.9 KiB
Go
90 lines
2.9 KiB
Go
package mcpclient
|
|
|
|
import (
|
|
"flag"
|
|
"net"
|
|
"net/http"
|
|
"time"
|
|
)
|
|
|
|
// dialTimeout bounds TCP connection establishment to an MCP
|
|
// server. Without it, a server that silently drops SYNs (for
|
|
// example an edge mitigation black-holing this deployment's
|
|
// egress IPs) blocks the dial until the kernel gives up
|
|
// retransmitting, roughly two minutes on Linux.
|
|
const dialTimeout = 5 * time.Second
|
|
|
|
// responseHeaderTimeout bounds how long a server may take to send
|
|
// response headers once a request is written. It matches
|
|
// toolCallTimeout rather than connectTimeout because the same
|
|
// client serves tool-call POSTs, and a JSON-response MCP server
|
|
// sends no headers until the tool finishes, so a lower value would
|
|
// kill legitimate slow tools that fit the tool-call budget.
|
|
// Long-lived SSE streams are unaffected; only their headers must
|
|
// arrive within this window. http.Client.Timeout is deliberately
|
|
// unset because it would cap the stream body too.
|
|
const responseHeaderTimeout = toolCallTimeout
|
|
|
|
// mcpSharedTransport is the transport for all production MCP
|
|
// connections. MCP traffic must not use http.DefaultTransport
|
|
// directly: the default has no dial or response-header bounds, so
|
|
// a black-holed server would hold connections for minutes.
|
|
var mcpSharedTransport = newMCPTransport()
|
|
|
|
// newMCPTransport clones http.DefaultTransport when possible,
|
|
// preserving proxy and connection-pool settings, and tightens its
|
|
// failure timeouts so an unresponsive MCP server fails in seconds.
|
|
func newMCPTransport() *http.Transport {
|
|
tr, ok := http.DefaultTransport.(*http.Transport)
|
|
if ok {
|
|
tr = tr.Clone()
|
|
} else {
|
|
tr = &http.Transport{Proxy: http.ProxyFromEnvironment}
|
|
}
|
|
tr.DialContext = (&net.Dialer{
|
|
Timeout: dialTimeout,
|
|
KeepAlive: 30 * time.Second,
|
|
}).DialContext
|
|
tr.ResponseHeaderTimeout = responseHeaderTimeout
|
|
return tr
|
|
}
|
|
|
|
func httpClientWithHeaders(headers map[string]string) *http.Client {
|
|
var base http.RoundTripper = mcpSharedTransport
|
|
if isolated := mcpHTTPClient(); isolated != nil {
|
|
base = isolated.Transport
|
|
}
|
|
if len(headers) == 0 {
|
|
return &http.Client{Transport: base}
|
|
}
|
|
return &http.Client{Transport: &headerRoundTripper{
|
|
base: base,
|
|
headers: headers,
|
|
}}
|
|
}
|
|
|
|
type headerRoundTripper struct {
|
|
base http.RoundTripper
|
|
headers map[string]string
|
|
}
|
|
|
|
func (h *headerRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
|
|
clone := req.Clone(req.Context())
|
|
for k, v := range h.headers {
|
|
clone.Header.Set(k, v)
|
|
}
|
|
return h.base.RoundTrip(clone)
|
|
}
|
|
|
|
// mcpHTTPClient returns an isolated *http.Client when running
|
|
// inside tests, or nil for production. During tests each client
|
|
// gets a fresh transport so closed httptest servers cannot leave
|
|
// stale pooled connections behind for later tests that reuse the
|
|
// same address.
|
|
func mcpHTTPClient() *http.Client {
|
|
if flag.Lookup("test.v") == nil {
|
|
return nil
|
|
}
|
|
return &http.Client{Transport: newMCPTransport()}
|
|
}
|