diff --git a/docs/tools/lsp/index.md b/docs/tools/lsp/index.md index f84e054d3..58133cba7 100644 --- a/docs/tools/lsp/index.md +++ b/docs/tools/lsp/index.md @@ -197,7 +197,7 @@ Available Capabilities: LSP toolsets are managed by the same supervisor as MCP toolsets, so a crashed `gopls` (or any other language server) is reconnected automatically with exponential backoff. Use the [`lifecycle`](../../configuration/tools/index.md#toolset-lifecycle) block to tune the policy per toolset — for example, mark `gopls` as `strict` if your CI flow requires it to be available, or use `/toolset-restart gopls` from the TUI to force a reconnect when the server gets stuck. -**Startup failure behaviour:** local LSP server startup failures (missing binary, server-unavailable) fail fast — each turn retries immediately with no artificial delay. The rate-limit backoff gate applies only to model-provider embedding calls (see [Indexing failures, retries and backoff](../rag/index.md#indexing-failures-retries-and-backoff)); it does not apply to LSP server startup. +**Startup failure behaviour:** missing-binary and bad-config failures fail fast — each turn retries immediately with no artificial delay. A language server that crash-loops is not currently paced by the backoff gate; the supervisor's own reconnect policy (controlled by the `lifecycle` block) is the primary throttle for crash recovery. ```yaml toolsets: diff --git a/docs/tools/mcp/index.md b/docs/tools/mcp/index.md index ad4bb9c1d..286d1c267 100644 --- a/docs/tools/mcp/index.md +++ b/docs/tools/mcp/index.md @@ -258,7 +258,7 @@ toolsets: See [Toolset Lifecycle](../../configuration/tools/index.md#toolset-lifecycle) for all profiles and tuning knobs, and [`/toolset-restart`](../../features/tui/index.md) to force a reconnect from the TUI. -**Startup failure behaviour:** local MCP startup failures (missing binary, connection refused, authentication error) fail fast — each turn retries immediately with no artificial delay. The rate-limit backoff gate applies only to model-provider embedding calls (see [Indexing failures, retries and backoff](../rag/index.md#indexing-failures-retries-and-backoff)); it does not apply to MCP server startup. +**Startup failure behaviour:** local MCP failures (missing binary, connection refused, bad auth) fail fast — each turn retries immediately with no artificial delay. Remote MCP servers (Streamable HTTP / SSE) that respond with a retryable HTTP status (429 Too Many Requests, 503 Service Unavailable, other 5xx) are paced by the same [bounded exponential backoff gate](../rag/index.md#indexing-failures-retries-and-backoff) as RAG embedding calls, so a temporarily-overloaded remote MCP server does not trigger a new connect attempt on every agent turn. ## Combined Example diff --git a/pkg/tools/mcp/oauth.go b/pkg/tools/mcp/oauth.go index 0337d4fae..e44926530 100644 --- a/pkg/tools/mcp/oauth.go +++ b/pkg/tools/mcp/oauth.go @@ -500,6 +500,12 @@ type oauthTransport struct { // swallows in favor of a bare http.StatusText. lastErrStatus int lastErrBody []byte + // lastErrRetryAfter captures the raw Retry-After header value (if any) of + // the most recent non-2xx response, so enrichConnectError can forward it + // to modelerrors.WrapHTTPError and have the StartableToolSet backoff gate + // honor a server-supplied retry hint instead of falling back to the + // generic computed delay. + lastErrRetryAfter string // lastAuthRequired records when the transport short-circuited an // interactive OAuth flow because the request context disallowed // prompts (see WithoutInteractivePrompts). The MCP SDK wraps transport @@ -874,6 +880,7 @@ func (t *oauthTransport) logErrorResponse(req *http.Request, resp *http.Response t.mu.Lock() t.lastErrStatus = resp.StatusCode t.lastErrBody = body + t.lastErrRetryAfter = resp.Header.Get("Retry-After") t.mu.Unlock() slog.Warn("Authenticated MCP request was rejected by the server", @@ -885,23 +892,33 @@ func (t *oauthTransport) logErrorResponse(req *http.Request, resp *http.Response ) } -// lastServerError returns the status code and a short, human-readable -// explanation drawn from the most recent non-2xx response seen by this -// transport. The string is empty when no such response has been captured -// or when the body yielded no useful text. +// lastServerErrorSnapshot returns the status code, a short human-readable +// explanation, and the raw Retry-After header value, all captured together +// under a single lock from the most recent non-2xx response seen by this +// transport. status is 0 when no such response has been captured; msg and +// retryAfter are "" when the body yielded no useful text / no header was +// present, respectively. +// +// The three fields are read under one lock (rather than via separate +// accessors) so a caller building a combined error never pairs a status +// captured from one response with a Retry-After header captured from a +// different, concurrent one: this transport's RoundTrip can be invoked +// concurrently for a single logical connect attempt (e.g. a standalone SSE +// probe alongside the initialize call). // // This is how the transport surfaces provider-specific errors (e.g. Slack's // "App is not enabled for Slack MCP server access") that would otherwise // be hidden behind the MCP SDK's generic http.StatusText-derived messages. -func (t *oauthTransport) lastServerError() (int, string) { +func (t *oauthTransport) lastServerErrorSnapshot() (status int, msg, retryAfter string) { t.mu.Lock() - status := t.lastErrStatus + status = t.lastErrStatus body := t.lastErrBody + retryAfter = t.lastErrRetryAfter t.mu.Unlock() if status == 0 { - return 0, "" + return 0, "", "" } - return status, extractServerMessage(body) + return status, extractServerMessage(body), retryAfter } // authorizationRequired reports whether the transport short-circuited an diff --git a/pkg/tools/mcp/remote.go b/pkg/tools/mcp/remote.go index 9d735835e..3c1ff8fd3 100644 --- a/pkg/tools/mcp/remote.go +++ b/pkg/tools/mcp/remote.go @@ -16,6 +16,7 @@ import ( "github.com/docker/docker-agent/pkg/environment" "github.com/docker/docker-agent/pkg/httpclient" "github.com/docker/docker-agent/pkg/js" + "github.com/docker/docker-agent/pkg/modelerrors" "github.com/docker/docker-agent/pkg/upstream" ) @@ -211,8 +212,29 @@ func enrichConnectError(err error, t *oauthTransport) error { if t.authorizationRequired() { return &AuthorizationRequiredError{URL: t.baseURL} } - if status, msg := t.lastServerError(); status != 0 && msg != "" { - return fmt.Errorf("failed to connect to MCP server: %w (server responded %d: %s)", err, status, msg) + // Wrap on status alone: many rate-limit / load-balancer 429s and 503s + // carry an empty body, so gating on msg != "" (as an earlier version of + // this code did) silently dropped the *modelerrors.StatusError wrap — + // and with it, the StartableToolSet backoff gate never armed. + // + // status, msg and retryAfter are read together as a single snapshot + // (rather than via two separately-locked accessor calls) so they can + // never be pieced together from two different concurrent responses on + // this transport (e.g. a standalone SSE probe racing the initialize call). + if status, msg, retryAfter := t.lastServerErrorSnapshot(); status != 0 { + var enriched error + if msg != "" { + enriched = fmt.Errorf("failed to connect to MCP server: %w (server responded %d: %s)", err, status, msg) + } else { + enriched = fmt.Errorf("failed to connect to MCP server: %w (server responded %d)", err, status) + } + // Forward the server's Retry-After hint (if any) so the backoff gate + // honors it instead of falling back to the generic computed delay. + resp := &http.Response{Header: http.Header{}} + if retryAfter != "" { + resp.Header.Set("Retry-After", retryAfter) + } + return modelerrors.WrapHTTPError(status, resp, enriched) } return fmt.Errorf("failed to connect to MCP server: %w", err) } @@ -249,8 +271,9 @@ func (c *remoteMCPClient) SetUnmanagedOAuthRedirectURI(uri string) { // values never go stale on a long-lived connection. // // The oauthTransport is returned alongside the client so callers can inspect -// the most recent server-side failure (via lastServerError) when Connect() -// returns a bare HTTP-status error and we need to surface the actual cause. +// the most recent server-side failure (via lastServerErrorSnapshot) when +// Connect() returns a bare HTTP-status error and we need to surface the +// actual cause. // // The transport chain wraps `httpclient.WrapWithOTel` outermost so every // outbound MCP request injects W3C `traceparent` (and creates an HTTP diff --git a/pkg/tools/mcp/remote_test.go b/pkg/tools/mcp/remote_test.go index 1a452ad37..244ef0e86 100644 --- a/pkg/tools/mcp/remote_test.go +++ b/pkg/tools/mcp/remote_test.go @@ -19,6 +19,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/docker/docker-agent/pkg/modelerrors" + "github.com/docker/docker-agent/pkg/tools" "github.com/docker/docker-agent/pkg/upstream" ) @@ -875,3 +877,281 @@ func TestOAuthHTTPClientWithHeaders_ResolverKeepsEnvHeadersFreshAndHostScoped(t assert.Empty(t, v, "requests to a third-party host must NOT carry the configured header even with a resolver (credential-leak guard)") } + +// TestEnrichConnectError_RetryableStatusSurfacesAsStatusError verifies that +// enrichConnectError wraps retryable HTTP errors (5xx/429) in a *StatusError +// so the StartableToolSet backoff gate can arm on them. +func TestEnrichConnectError_RetryableStatusSurfacesAsStatusError(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + status int + wantRetryable bool + }{ + {"503 service unavailable", http.StatusServiceUnavailable, true}, + {"429 too many requests", http.StatusTooManyRequests, true}, + {"500 internal server error", http.StatusInternalServerError, true}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(tc.status) + _, _ = fmt.Fprintf(w, `{"error":{"message":"server error %d"}}`, tc.status) + })) + defer srv.Close() + + store := NewInMemoryTokenStore() + require.NoError(t, store.StoreToken(srv.URL, &OAuthToken{AccessToken: "tok", TokenType: "Bearer"})) + + client := newRemoteClient(srv.URL, "streamable", nil, store, nil, false, nil) + + _, err := client.Initialize(t.Context(), nil) + require.Error(t, err) + + var se *modelerrors.StatusError + require.ErrorAs(t, err, &se, "a %d response must surface as *StatusError", tc.status) + assert.Equal(t, tc.status, se.StatusCode) + assert.True(t, modelerrors.RetryableHTTPStatus(se), + "status %d must be classified retryable by the backoff gate", tc.status) + }) + } +} + +// TestEnrichConnectError_NonRetryableStatusDoesNotArm verifies that a 4xx +// client-error response wraps in *StatusError but is NOT classified retryable, +// so bad-config / auth failures fail promptly without triggering pacing. +func TestEnrichConnectError_NonRetryableStatusDoesNotArm(t *testing.T) { + t.Parallel() + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusForbidden) // 403 + _, _ = fmt.Fprint(w, `{"error":{"message":"access denied"}}`) + })) + defer srv.Close() + + store := NewInMemoryTokenStore() + require.NoError(t, store.StoreToken(srv.URL, &OAuthToken{AccessToken: "tok", TokenType: "Bearer"})) + + client := newRemoteClient(srv.URL, "streamable", nil, store, nil, false, nil) + + _, err := client.Initialize(t.Context(), nil) + require.Error(t, err) + + var se *modelerrors.StatusError + require.ErrorAs(t, err, &se, "403 must still surface as *StatusError (wrapped for structured access)") + assert.Equal(t, http.StatusForbidden, se.StatusCode) + assert.False(t, modelerrors.RetryableHTTPStatus(se), + "403 must NOT be classified retryable — client errors fail promptly") +} + +// TestEnrichConnectError_NoStatusNoStatusError verifies that when there is no +// recorded HTTP server error (e.g. a network-level failure before any HTTP +// response), the error does not carry a *StatusError so the gate does not arm. +func TestEnrichConnectError_NoStatusNoStatusError(t *testing.T) { + t.Parallel() + + // Point at a closed port so there is never an HTTP response. + ln, err := (&net.ListenConfig{}).Listen(t.Context(), "tcp", "127.0.0.1:0") + require.NoError(t, err) + addr := ln.Addr().String() + _ = ln.Close() + + client := newRemoteClient("http://"+addr, "streamable", nil, NewInMemoryTokenStore(), nil, false, nil) + + _, err = client.Initialize(t.Context(), nil) + require.Error(t, err) + + var se *modelerrors.StatusError + assert.NotErrorAs(t, err, &se, + "a plain network failure must not carry a *StatusError (would arm the gate spuriously)") +} + +// TestEnrichConnectError_EmptyBodyStatusStillArms verifies that a retryable +// HTTP status with an EMPTY response body (common for load-balancer or +// rate-limit responses that carry no JSON payload) still surfaces as a +// *modelerrors.StatusError. An earlier version of enrichConnectError gated +// the wrap on the extracted message being non-empty, which silently dropped +// the StatusError wrap — and with it, all backoff pacing — whenever the +// server didn't bother sending a body alongside the status code. +func TestEnrichConnectError_EmptyBodyStatusStillArms(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + status int + }{ + {"503 empty body", http.StatusServiceUnavailable}, + {"429 empty body", http.StatusTooManyRequests}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + // No Content-Type, no body — exactly what many load balancers + // and rate limiters send. + w.WriteHeader(tc.status) + })) + defer srv.Close() + + store := NewInMemoryTokenStore() + require.NoError(t, store.StoreToken(srv.URL, &OAuthToken{AccessToken: "tok", TokenType: "Bearer"})) + + client := newRemoteClient(srv.URL, "streamable", nil, store, nil, false, nil) + + _, err := client.Initialize(t.Context(), nil) + require.Error(t, err) + + var se *modelerrors.StatusError + require.ErrorAs(t, err, &se, + "an empty-body %d response must still surface as *StatusError", tc.status) + assert.Equal(t, tc.status, se.StatusCode) + assert.True(t, modelerrors.RetryableHTTPStatus(se), + "empty-body status %d must still be classified retryable", tc.status) + }) + } +} + +// TestEnrichConnectError_RetryAfterHonoured verifies that a server-supplied +// Retry-After header on a 429 response is parsed through to the resulting +// *modelerrors.StatusError, matching the handling in the sibling model-adapter +// paths (see modelerrors.WrapHTTPError). +func TestEnrichConnectError_RetryAfterHonoured(t *testing.T) { + t.Parallel() + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Retry-After", "120") + w.WriteHeader(http.StatusTooManyRequests) + })) + defer srv.Close() + + store := NewInMemoryTokenStore() + require.NoError(t, store.StoreToken(srv.URL, &OAuthToken{AccessToken: "tok", TokenType: "Bearer"})) + + client := newRemoteClient(srv.URL, "streamable", nil, store, nil, false, nil) + + _, err := client.Initialize(t.Context(), nil) + require.Error(t, err) + + var se *modelerrors.StatusError + require.ErrorAs(t, err, &se) + assert.Equal(t, http.StatusTooManyRequests, se.StatusCode) + assert.Equal(t, 120*time.Second, se.RetryAfter, + "the server's Retry-After header must be parsed onto the StatusError") +} + +// TestEnrichConnectError_NoRetryAfterHeaderLeavesZero verifies that when the +// server does not send a Retry-After header, RetryAfter stays zero (the +// gate then falls back to its own computed delay). +func TestEnrichConnectError_NoRetryAfterHeaderLeavesZero(t *testing.T) { + t.Parallel() + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusTooManyRequests) + })) + defer srv.Close() + + store := NewInMemoryTokenStore() + require.NoError(t, store.StoreToken(srv.URL, &OAuthToken{AccessToken: "tok", TokenType: "Bearer"})) + + client := newRemoteClient(srv.URL, "streamable", nil, store, nil, false, nil) + + _, err := client.Initialize(t.Context(), nil) + require.Error(t, err) + + var se *modelerrors.StatusError + require.ErrorAs(t, err, &se) + assert.Zero(t, se.RetryAfter, "no Retry-After header means the gate computes its own delay") +} + +// TestBackoffGate_RemoteMCPRetryableStatusPacesReconnect is an end-to-end +// regression test: it drives a REAL *Toolset (built via NewRemoteToolset, +// exactly as production wiring does) through tools.StartableToolSet.TryStart +// against a mock server that always answers 503. It proves the whole chain — +// enrichConnectError -> Toolset.Start -> supervisor.Start -> the backoff +// gate in tryStartLocked — stays intact end to end, rather than relying on +// enrichConnectError-only unit tests that would miss error-chain loss +// introduced anywhere between remote.go and mcp.go's Initialize wrapping. +func TestBackoffGate_RemoteMCPRetryableStatusPacesReconnect(t *testing.T) { + t.Parallel() + + var attempts atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + attempts.Add(1) + w.WriteHeader(http.StatusServiceUnavailable) + })) + defer srv.Close() + + toolset := NewRemoteToolset("test", srv.URL, "streamable", nil, nil) + + now := time.Now() + clock := func() time.Time { return now } + identityJitter := func(d time.Duration) time.Duration { return d } + + s := tools.NewStartable(toolset, tools.WithStartRetryClock(clock), tools.WithStartRetryJitter(identityJitter)) + + // Attempt 1: gate is idle, the real connect attempt runs and fails, + // arming the gate. The transport may issue more than one HTTP request + // per logical connect attempt (e.g. a standalone SSE probe), so assert + // relative growth rather than an exact per-call count. + started, err := s.TryStart(t.Context()) + assert.False(t, started) + require.Error(t, err) + afterFirst := attempts.Load() + assert.Positive(t, afterFirst, "first TryStart must hit the server") + + var se *modelerrors.StatusError + require.ErrorAs(t, err, &se, "the gate-arming error must carry the StatusError") + assert.Equal(t, http.StatusServiceUnavailable, se.StatusCode) + + // Immediately after: gate armed, TryStart returns without a new + // connect attempt reaching the server. + started, err = s.TryStart(t.Context()) + assert.False(t, started) + require.Error(t, err) + assert.Equal(t, afterFirst, attempts.Load(), "gate must block the retry from reaching the server") + + // Advance the fake clock past the backoff window (comfortably beyond + // the documented 5-minute cap, without depending on the unexported + // base/max constants which aren't visible across package test + // boundaries): gate opens, a new connect attempt reaches the server. + now = now.Add(6 * time.Minute) + started, err = s.TryStart(t.Context()) + assert.False(t, started) + require.Error(t, err) + assert.Greater(t, attempts.Load(), afterFirst, "gate must open and retry once the window elapses") +} + +// TestBackoffGate_RemoteMCPNonRetryableStatusFailsPromptly is the negative +// counterpart: a 403 (bad config / auth) must fail every turn without any +// pacing, through the same real TryStart path. +func TestBackoffGate_RemoteMCPNonRetryableStatusFailsPromptly(t *testing.T) { + t.Parallel() + + var attempts atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + attempts.Add(1) + w.WriteHeader(http.StatusForbidden) + })) + defer srv.Close() + + toolset := NewRemoteToolset("test", srv.URL, "streamable", nil, nil) + s := tools.NewStartable(toolset) + + var prev int32 + for range 3 { + started, err := s.TryStart(t.Context()) + assert.False(t, started) + require.Error(t, err) + cur := attempts.Load() + assert.Greater(t, cur, prev, "403 must reach the server every turn, no pacing") + prev = cur + } +} diff --git a/pkg/tools/startable_backoff.go b/pkg/tools/startable_backoff.go index c0ab9657b..38ad3b2fc 100644 --- a/pkg/tools/startable_backoff.go +++ b/pkg/tools/startable_backoff.go @@ -14,11 +14,27 @@ const ( ) // startBackoffRetryable reports whether err carries a retryable HTTP status -// (429, 408, or 5xx). Requires a *modelerrors.StatusError anywhere in the -// chain; plain network errors and the regex fallback in RetryableHTTPStatus -// are intentionally excluded to avoid arming the gate on port numbers or -// chunk counters that match the \b[45]\d{2}\b pattern. +// (429/408/5xx) that warrants pacing the next start attempt. Only a +// *modelerrors.StatusError in the error chain arms the gate; plain network +// errors and the regex fallback in RetryableHTTPStatus are intentionally +// excluded so port numbers, PIDs, and chunk counters in plain error text +// cannot arm the gate. +// // A StatusError wins even when context.DeadlineExceeded is also in the chain. +// +// Deliberately excluded from arming (these must never pace): +// - lifecycle.ErrServerUnavailable: missing binary / process-not-found — fast-retry. +// - lifecycle.ErrTransport: connection refused / no such host — fast-retry. +// - lifecycle.ErrAuthRequired / ErrCapabilityMissing: permanent — fail promptly. +// - lifecycle.ErrInitTimeout, ErrSessionMissing: transient, handled by the +// supervisor's own reconnect policy without per-turn pacing. +// - Plain error strings: excluded to avoid false positives on numeric +// patterns in port numbers or counters. +// +// Note: lifecycle.ErrServerCrashed (a server that started then crashed) is +// NOT currently surfaced by supervisor.Start(); it flows only through the +// supervisor's internal watcher goroutine. LSP crash-loop pacing is therefore +// deferred until that sentinel is propagated through the start path. func startBackoffRetryable(err error) bool { var se *modelerrors.StatusError if !errors.As(err, &se) { diff --git a/pkg/tools/startable_backoff_test.go b/pkg/tools/startable_backoff_test.go index 04357680e..530c48ac6 100644 --- a/pkg/tools/startable_backoff_test.go +++ b/pkg/tools/startable_backoff_test.go @@ -3,6 +3,7 @@ package tools_test import ( "context" "errors" + "fmt" "sync" "sync/atomic" "testing" @@ -14,6 +15,7 @@ import ( "github.com/docker/docker-agent/pkg/modelerrors" "github.com/docker/docker-agent/pkg/tools" + "github.com/docker/docker-agent/pkg/tools/lifecycle" ) // startErrToolSet is a minimal Startable whose error field controls the @@ -755,3 +757,61 @@ func TestStartableToolSet_RetryAfterCapAtMax(t *testing.T) { assert.Check(t, is.Equal(inner.starts.Load(), int32(2)), "gate must open at startBackoffMax, not at the uncapped hint") } + +// TestStartBackoffRetryable_ErrServerUnavailable verifies that a missing binary +// (ErrServerUnavailable) does NOT arm the gate — it fails promptly every turn. +func TestStartBackoffRetryable_ErrServerUnavailable(t *testing.T) { + inner := &startErrToolSet{} + inner.setErr(fmt.Errorf("%w: no such file or directory", lifecycle.ErrServerUnavailable)) + s := newThrottledStartable(inner) + + for range 3 { + _, err := s.TryStart(t.Context()) + assert.Check(t, err != nil) + } + assert.Check(t, is.Equal(inner.starts.Load(), int32(3)), + "missing-binary errors must not pace retries") +} + +// TestStartBackoffRetryable_ErrTransport verifies that a network-level failure +// (connection refused, no such host) does NOT arm the gate. +func TestStartBackoffRetryable_ErrTransport(t *testing.T) { + inner := &startErrToolSet{} + inner.setErr(fmt.Errorf("%w: connection refused", lifecycle.ErrTransport)) + s := newThrottledStartable(inner) + + for range 3 { + _, _ = s.TryStart(t.Context()) + } + assert.Check(t, is.Equal(inner.starts.Load(), int32(3)), + "transport errors must not pace retries") +} + +// TestStartBackoffRetryable_ErrAuthRequired verifies that a permanent auth +// failure (OAuth required / invalid token) does NOT arm the gate. +func TestStartBackoffRetryable_ErrAuthRequired(t *testing.T) { + inner := &startErrToolSet{} + inner.setErr(fmt.Errorf("%w: token expired", lifecycle.ErrAuthRequired)) + s := newThrottledStartable(inner) + + for range 3 { + _, _ = s.TryStart(t.Context()) + } + assert.Check(t, is.Equal(inner.starts.Load(), int32(3)), + "auth-required errors must not pace retries") +} + +// TestStartBackoffRetryable_4xxStatusDoesNotArm verifies that a structured +// 4xx HTTP error (client error, not a rate-limit) wraps as *StatusError but +// does not arm the gate. +func TestStartBackoffRetryable_4xxStatusDoesNotArm(t *testing.T) { + inner := &startErrToolSet{} + inner.setErr(&modelerrors.StatusError{StatusCode: 400, Err: errors.New("bad request")}) + s := newThrottledStartable(inner) + + for range 3 { + _, _ = s.TryStart(t.Context()) + } + assert.Check(t, is.Equal(inner.starts.Load(), int32(3)), + "400 client errors must not arm the backoff gate") +}