fix(#4060): extend backoff gate to remote MCP HTTP errors - #4074
fix(#4060): extend backoff gate to remote MCP HTTP errors#4074aheritier wants to merge 1 commit into
Conversation
aheritier
changed the title
aheritier
added
area/docs
Three review findings on PR #4074, all addressed in this commit: 1. (must-fix) enrichConnectError previously gated the *modelerrors.StatusError wrap on the extracted server message being non-empty. Many load-balancer and rate-limit responses carry an empty body, so a bare 429/503 with no payload silently skipped the wrap and the backoff gate never armed — defeating the whole point of this PR for exactly the responses it exists to pace. Now wraps on status code alone; the enrichment text degrades gracefully to '(server responded %d)' when no message is available. 2. (should-fix) Retry-After was discarded: WrapHTTPError was always called with resp=nil. oauthTransport now also captures the raw Retry-After header value alongside the status/body it already tracks, and enrichConnectError builds a minimal *http.Response carrying that header so WrapHTTPError parses it onto the StatusError — matching the handling already in place for model-provider adapters. Status, message and Retry-After are read together as a single lastServerErrorSnapshot() under one lock (not three separately-locked accessors), so a caller can never pair a status from one response with a Retry-After header captured from a different concurrent response on the same transport (this transport's RoundTrip can run concurrently for a single logical connect attempt, e.g. a standalone SSE probe alongside the initialize call). 3. (should-fix) Added an end-to-end regression test that drives a real *mcp.Toolset (built via NewRemoteToolset, exactly as production wiring does) through tools.StartableToolSet.TryStart against a mock 503/403 server, proving the whole chain (enrichConnectError -> Toolset.Start -> supervisor.Start -> the backoff gate) stays intact end to end, not just the enrichConnectError unit boundary.
6267a9e to
d99ac16
Compare
d99ac16 to
608a099
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
608a099 to
21920e7
Compare
Remote MCP servers returning 503/429/5xx during the initialize handshake previously triggered a new connect attempt on every agent turn. enrichConnectError now wraps the HTTP status captured by the oauthTransport in modelerrors.WrapHTTPError, so retryable responses surface as *StatusError and arm the StartableToolSet backoff gate exactly as RAG embedding 429s do. The wrap happens on status code alone (not conditional on a non-empty response body), since many load-balancer and rate-limit responses carry an empty body and would otherwise silently skip the gate for exactly the responses it exists to pace. 4xx client-error responses (400/401/403) are also wrapped in *StatusError for structured access but are classified non-retryable, so bad-config and auth failures still fail promptly without pacing. A server-supplied Retry-After header, when present, is threaded through to the backoff gate: oauthTransport captures the raw header value alongside the status/body it already tracks, and enrichConnectError builds a minimal *http.Response carrying it so WrapHTTPError parses it onto the StatusError, matching the handling already in place for model-provider adapters. Status, message and Retry-After are read together as a single lastServerErrorSnapshot() under one lock (not separate accessors), so a caller can never pair a status from one response with a Retry-After header captured from a different concurrent response on the same transport (this transport's RoundTrip can run concurrently for a single logical connect attempt, e.g. a standalone SSE probe alongside the initialize call). The empty-body error message no longer repeats the status code redundantly alongside StatusError's own "HTTP %d:" prefix. Local stdio MCP failures (missing binary, connection refused) never reach enrichConnectError and are unaffected by this change. Classifier policy: startBackoffRetryable arms only on *StatusError with a retryable HTTP status — a fixed enumeration (429, 408, 500, 502, 503, 504, 529), not a full 5xx range: codes such as 501, 505, or the Cloudflare 520-527 family do not arm the gate. Doc comments and docs/tools/mcp/index.md now name this enumeration precisely instead of saying "5xx" generically. docs/tools/rag/index.md's "What triggers backoff" section is scoped explicitly to the RAG/embedding path (its 429-only trigger set does not generalize to other toolset types) with a cross-reference to MCP's broader trigger set, resolving the prior contradiction between the two pages. Deliberately excluded from arming: lifecycle.ErrServerUnavailable (missing binary), lifecycle.ErrTransport (connection refused / no such host), lifecycle.ErrAuthRequired / ErrCapabilityMissing, lifecycle.ErrInitTimeout, lifecycle.ErrSessionMissing. Note: ErrServerCrashed is NOT currently surfaced by supervisor.Start(); LSP crash-loop pacing is deferred until that propagation path is wired. Adds unit coverage for enrichConnectError (status-only gating, Retry-After present/absent) plus end-to-end tests that drive a real *mcp.Toolset (via NewRemoteToolset, matching production wiring) through tools.StartableToolSet.TryStart against a mock 503/403 server, proving the whole chain stays intact end to end. A further end-to-end test flips the mock server from 503 to a real, working MCP handshake after the backoff window elapses, proving the toolset actually recovers and starts rather than merely ceasing to error. Refs #4060 (partial — A2A pacing deferred: agent-card resolver does not expose HTTP status cleanly; LSP crash-loop pacing also deferred)
21920e7 to
0341872
Compare
aheritier
left a comment
aheritier
left a comment
There was a problem hiding this comment.
🤖 Automated implementer agent — this comment was posted by the implementer bot from Docker Agentic Platform, not by a human developer
Re-reviewed at head 0341872e (amended into the single commit per the "don't diary review rounds" rule for this arc — content changed, no new commit). This addresses a fresh review pass with 2 should-fix + 2 optional findings. Full findings and resolution below, for the record.
CI for this stacked-branch PR was still finishing several longer-running jobs (build-and-test, lint, windows-tests, build-image) at the time of this review; the quick jobs (canonical-check, link-check, llms-txt-check, markdownlint, pa11y, validate-upstream, CodeQL/Analyze) are all green. I substituted local validation for the rest: go build ./pkg/tools/... ./pkg/tools/mcp/... ./pkg/modelerrors/... clean, go test ./pkg/tools/... ./pkg/tools/mcp/... ./pkg/modelerrors/... -count=1 all green, golangci-lint run ./pkg/tools/ ./pkg/tools/mcp/ 0 issues, task lint (golangci-lint + custom lint cops + go mod tidy --diff) all clean.
Findings and resolution
[should-fix] Doc contradiction between MCP and RAG docs — fixed
docs/tools/rag/index.md's "What triggers backoff" section previously read as an absolute, toolset-wide statement ("Backoff applies only to HTTP 429..."), which became false for MCP once this PR shipped. It's now scoped explicitly: "For RAG indexing, backoff applies only to...", with an added note under the existing [!NOTE] block: "This 429-only trigger set is specific to the RAG/embedding path... for example, remote MCP toolsets also pace on 408 and a fixed set of 5xx-family statuses (see MCP startup failure behaviour)." The two pages now cross-reference rather than contradict.
[should-fix] "other 5xx" overstates what actually arms the gate — fixed (doc precision only, per your instruction not to broaden isRetryableStatusCode)
docs/tools/mcp/index.md:261 now names the actual enumerated set: "429 Too Many Requests, 408 Request Timeout, 500/502/503/504, or 529 (Anthropic-style "overloaded")", with an explicit callout: "This is a fixed enumeration, not a full 5xx range: less-common codes such as 501, 505, or the Cloudflare 520–527 family do not arm the gate." The startBackoffRetryable doc comment (pkg/tools/startable_backoff.go:16-21) was reworded the same way. isRetryableStatusCode (pkg/modelerrors/modelerrors.go:332-346) is untouched — still 500/502/503/504/529/408 only, confirmed by git diff showing no changes to that file in this round.
[optional] Redundant status in the user-facing error message — fixed
enrichConnectError's empty-msg branch (pkg/tools/mcp/remote.go:224-227) no longer appends (server responded %d), since modelerrors.StatusError.Error() already prefixes "HTTP %d: " — the duplication (HTTP 503: ... (server responded 503)) is gone; the branch now reads fmt.Errorf("failed to connect to MCP server: %w", err). No test asserted on the old exact text (confirmed via grep -n "server responded" pkg/tools/mcp/*_test.go before the change), so nothing needed updating there.
[optional] No MCP-level recovery (429/503→200) e2e test — fixed
Added TestBackoffGate_RemoteMCPRecoversAfterBackoffWindow (pkg/tools/mcp/remote_test.go:1401): a mock server starts failing with 503, then flips to a real, working gomcp.NewStreamableHTTPHandler-backed MCP server. The test drives a real *mcp.Toolset (via NewRemoteToolset) through tools.StartableToolSet.TryStart and asserts (a) the gate still blocks immediately after the server recovers but before the window elapses, and (b) once the window elapses TryStart returns started=true — the toolset actually completes a real MCP handshake and starts, not merely stops erroring. Test passes.
Re-validation
go build ./pkg/tools/... ./pkg/tools/mcp/... ./pkg/modelerrors/... # clean
go test ./pkg/tools/... ./pkg/tools/mcp/... ./pkg/modelerrors/... -count=1 # all green
golangci-lint run ./pkg/tools/ ./pkg/tools/mcp/ # 0 issues
task lint # clean (golangci-lint + custom cops + go mod tidy)
All 4 findings resolved. Filing as a comment rather than a formal approval only because I authored the PR and GitHub blocks self-approval, matching every prior round on this branch and on #4062.
1 participant
Stacked on PR #4062. Base retargets to
mainonce that merges.Refs #4060 (partial — A2A pacing and LSP crash-loop pacing deferred; see below)
What
Remote MCP servers responding with 503/429/5xx during the initialize handshake previously triggered a fresh connect attempt on every agent turn (the #4060 burst pattern). This PR fixes that by wrapping the HTTP status from the remote server in a
*modelerrors.StatusErrorso theStartableToolSetbackoff gate arming logic can pace retries — including a server-suppliedRetry-Afterhint, and correctly for responses with no body.Design
Wrap point —
enrichConnectErrorinpkg/tools/mcp/remote.go. TheoauthTransportalready records the last HTTP error status vialogErrorResponse(for any>= 400response).enrichConnectErrorwraps that status (regardless of whether the response carried a body — many rate-limit/load-balancer responses don't) viamodelerrors.WrapHTTPError, surfacing it as a*StatusErrorin the chain.Retry-After is honored.
pkg/tools/mcp/oauth.go'soauthTransportnow also captures the rawRetry-Afterheader value alongside the status/body it already tracked.lastServerErrorSnapshot()reads status, message, and Retry-After together under a single lock (not three separate accessor calls) so a caller can never pair a status from one response with a Retry-After header captured from a different concurrent response on the same transport — this transport'sRoundTripcan run concurrently for one logical connect attempt (e.g. a standalone SSE probe racing the initialize call).enrichConnectErrorbuilds a minimal*http.Responsecarrying that header and passes it toWrapHTTPError, matching the handling already in place for model-provider adapters (PR #4062).What arms the gate.
startBackoffRetryablechecks for a*modelerrors.StatusErrorwith a retryable HTTP status (429/408/5xx) viaerrors.As— exactly as it already does for RAG embedding failures. No regex heuristics; no new classification logic in the gate itself.What does NOT arm (unchanged policy):
enrichConnectError.*StatusErrorfor structured access butRetryableHTTPStatusreturns false → fail promptly.oauthDeclined,authorizationRequired) — handled by their own early-return paths before the status branch; unaffected.lifecycle.ErrServerUnavailable,ErrTransport,ErrAuthRequired,ErrInitTimeout,ErrSessionMissing— the gate classifier explicitly excludes all of these.Deferred:
lifecycle.ErrServerCrashedis produced only insidelspSession.Wait()which flows to the supervisor's internal watcher, not tosupervisor.Start(). The gate never sees it via the current error propagation path; deferred.Changed files
pkg/tools/mcp/remote.goenrichConnectError: wrap on status alone (not gated on a non-empty body); forwardsRetry-Aftervia a minimal synthetic*http.Responsepkg/tools/mcp/oauth.gooauthTransportcaptures the rawRetry-Afterheader; newlastServerErrorSnapshot()reads status/message/Retry-After together under one lockpkg/tools/startable_backoff.goErrInitTimeout,ErrSessionMissing), notes the deferred LSP crash-loop pathpkg/tools/mcp/remote_test.go*StatusError; 403 → non-retryable; empty-body 503/429 still arm;Retry-Afterpresent/absent; end-to-endTestBackoffGate_*driving a realNewRemoteToolsetthroughtools.StartableToolSet.TryStartpkg/tools/startable_backoff_test.goErrServerUnavailable,ErrTransport,ErrAuthRequired) do NOT arm the gate; 4xxStatusErrordoes NOT arm the gatedocs/tools/mcp/index.mddocs/tools/lsp/index.md