fix(#4060): add bounded jittered backoff to StartableToolSet retry path by aheritier · Pull Request #4062 · docker/docker-agent

aheritier · 2026-08-27T09:21:17Z

🤖 Automated implementer agent — this comment was posted by the implementer bot from Docker Agentic Platform, not by a human developer

Refs #4060 (partial — MCP/LSP startup errors and RAG 5xx/408 pacing deferred to follow-up)

What

Adds bounded, cancellable, jittered exponential backoff to StartableToolSet's non-blocking start path. A rate-limited embedding-provider 429 that previously triggered a full concurrent re-index on every agent turn is now paced to at most one retry per 15 s – 5 min window.

Design

Gate location — tryStartLocked (TryStart/TryStartWithTimeout only). Blocking Start() bypasses the gate so mcpcatalog enable and skill sub-session startup always get a fresh attempt.

Classifier — startBackoffRetryable. Requires a *modelerrors.StatusError in the error chain via errors.As. Port numbers and chunk-progress counters in plain error strings cannot arm the gate.

Bounds — base 15 s, cap 5 min, additive jitter [d, 1.2d]. The additive floor guarantees the full nominal wait; jitter de-synchronises concurrent toolset sources.

Retry-After. When the 429 response carries a Retry-After header, that hint overrides the computed delay (capped at 5 min, with the same additive jitter applied to avoid re-synchronisation).

Embedding-provider gap fixed. openai/client.go and dmr/embed.go now call oaistream.WrapOpenAIError on their embedding errors. WrapOpenAIError wraps *openaisdk.Error in *modelerrors.StatusError carrying the HTTP status code, so a 429 from the embedding provider reaches the gate as *StatusError and arms it.

Known limitation. Code-mode composites (codemode.Wrap) return PartialStartError on a partial failure; the partial-start branch resets the gate and leaves the failed-subset retry unpaced. Tracked in #4067.

Changed files

File	What changed
`pkg/modelerrors/modelerrors.go`	New exported `RetryableHTTPStatus(err)` classifier
`pkg/modelerrors/modelerrors_test.go`	Tests for the classifier (incl. regex false-positive cases)
`pkg/tools/startable_backoff.go`	New: constants, `computeStartBackoff`, `startBackoffRetryable`, `additiveJitter`, `retryAfterHint`
`pkg/tools/startable_backoff_test.go`	Unit tests: gate fires on 429/408/5xx, not on plain text; Retry-After honoured/ignored/capped; jitter spread
`pkg/tools/startable_backoff_regression_test.go`	Consumer-shaped regression suite (RAG/MCP/LSP shapes, no-leak, latch, blocking Start ungated)
`pkg/tools/startable.go`	`tryStartLocked`, `StartableOption`, `WithStartRetryJitter`, `WithStartRetryClock`, variadic `NewStartable`
`pkg/tools/export_test.go`	Exported helpers for deterministic test control
`pkg/tools/builtin/rag/rag_backoff_test.go`	Real-toolset integration test via `rag.New` + fake clock
`pkg/model/provider/openai/client.go`	Wrap batch-embedding error in `oaistream.WrapOpenAIError`
`pkg/model/provider/openai/embed_test.go`	httptest 429 → proves `*StatusError` chain; 5xx equivalent
`pkg/model/provider/dmr/embed.go`	Same — wrap DMR embedding error in `oaistream.WrapOpenAIError`
`pkg/model/provider/dmr/embed_test.go`	httptest 429 → proves `*StatusError` chain
`pkg/agent/agent.go`	Update stale log messages
`pkg/tools/mcp/mcp.go`	Update stale log message
`docs/tools/rag/index.md`	New section: trigger table (429-only), retry schedule, Retry-After note, troubleshooting
`docs/tools/mcp/index.md`	Lifecycle note: local startup failures are not paced by the gate
`docs/tools/lsp/index.md`	Same note for LSP
`docs/community/troubleshooting/index.md`	Cross-reference to RAG backoff section

…rt, align bounds Addresses all blocking and should-fix findings from the aheritier review on PR #4062: [blocking #1 + #2] Generic classifier with HTTP-status precedence: - Add modelerrors.RetryableHTTPStatus(err) — catches any error carrying a retryable HTTP status (429/408/5xx) via *StatusError or message regex, without string-pattern heuristics ('connection refused' stays non-retryable). - startBackoffRetryable becomes: return err != nil && RetryableHTTPStatus(err). A StatusError{429} coexisting with context.DeadlineExceeded now arms the gate (HTTP wins), fixing the deadline-masks-rate-limit race. [blocking #3] Bounds aligned with remediation plan: - base = 15s, cap = 5min (was 1s/30s). - Additive jitter [d, 1.2d] (was equal jitter [d/2, d]), guaranteeing the full nominal wait is always respected. [blocking #4] Gate enforced only in the TryStart path: - Move gate check from startLocked into new tryStartLocked (called by TryStart/TryStartWithTimeout only). - Start() calls startLocked directly — mcpcatalog enable and skill sub-session startup are never delayed. [should-fix #5] External recovery via StartReporter: - tryStartLocked checks reporter.IsStarted() when started==false; a live reporter (e.g. after /toolset-restart) clears the gate and latches the wrapper without calling the underlying Start. - New test: TestStartableToolSet_ExternalRecoveryClearsBackoffGate. [should-fix #6] Exported constructor options for cross-package tests: - NewStartable(ts, opts...) with StartableOption, WithStartRetryJitter, WithStartRetryClock. - nowFn() clock seam; zero-value StartableToolSet still usable. [should-fix #7] Concurrent and at-boundary tests: - TestStartableToolSet_BackoffNoDoubleStartWithinWindow: 20 goroutines calling TryStart, assert underlying Start invoked exactly once. - TestStartableToolSet_BackoffAtBoundary: fake clock, gate open at expiry. [optional] Stale comment name in BackoffDormantForPlainErrors fixed. Also: - RetryableHTTPStatus test cases include plain-text regex fallback. - ExportedSetClock removed (unused; WithStartRetryClock preferred). - All gating tests converted from s.Start() to s.TryStart(). - Jitter-bounds assertions updated to [nominal, 1.2×nominal]. PR2 (#4065) will need rebasing and test updates after this lands.

[blocking] partial-start comment corrected; known limitation for code-mode composites documented with reference to follow-up issue #4067 [SF1] startBackoffRetryable requires *StatusError — regex fallback excluded to prevent false positives on port numbers and chunk counters; RetryableHTTPStatus doc corrected to describe the actual regex-fallback behaviour [SF2] TestStartableToolSet_BlockingStartSkipsGate: arm gate, assert blocking Start() invokes underlying (not gated), assert TryStart() is gated [SF3] tryStartLocked: reporter-adoption guarded by !startBackoffUntil.IsZero() — fix is now scoped to gated state; non-gated TryStart semantics unchanged [SF4] stale 'next turn' log messages updated in agent.go and mcp.go [optional] ExportedSetJitter removed (dead code; WithStartRetryJitter preferred)

…rt, align bounds Addresses all blocking and should-fix findings from the aheritier review on PR #4062: [blocking #1 + #2] Generic classifier with HTTP-status precedence: - Add modelerrors.RetryableHTTPStatus(err) — catches any error carrying a retryable HTTP status (429/408/5xx) via *StatusError or message regex, without string-pattern heuristics ('connection refused' stays non-retryable). - startBackoffRetryable becomes: return err != nil && RetryableHTTPStatus(err). A StatusError{429} coexisting with context.DeadlineExceeded now arms the gate (HTTP wins), fixing the deadline-masks-rate-limit race. [blocking #3] Bounds aligned with remediation plan: - base = 15s, cap = 5min (was 1s/30s). - Additive jitter [d, 1.2d] (was equal jitter [d/2, d]), guaranteeing the full nominal wait is always respected. [blocking #4] Gate enforced only in the TryStart path: - Move gate check from startLocked into new tryStartLocked (called by TryStart/TryStartWithTimeout only). - Start() calls startLocked directly — mcpcatalog enable and skill sub-session startup are never delayed. [should-fix #5] External recovery via StartReporter: - tryStartLocked checks reporter.IsStarted() when started==false; a live reporter (e.g. after /toolset-restart) clears the gate and latches the wrapper without calling the underlying Start. - New test: TestStartableToolSet_ExternalRecoveryClearsBackoffGate. [should-fix #6] Exported constructor options for cross-package tests: - NewStartable(ts, opts...) with StartableOption, WithStartRetryJitter, WithStartRetryClock. - nowFn() clock seam; zero-value StartableToolSet still usable. [should-fix #7] Concurrent and at-boundary tests: - TestStartableToolSet_BackoffNoDoubleStartWithinWindow: 20 goroutines calling TryStart, assert underlying Start invoked exactly once. - TestStartableToolSet_BackoffAtBoundary: fake clock, gate open at expiry. [optional] Stale comment name in BackoffDormantForPlainErrors fixed. Also: - RetryableHTTPStatus test cases include plain-text regex fallback. - ExportedSetClock removed (unused; WithStartRetryClock preferred). - All gating tests converted from s.Start() to s.TryStart(). - Jitter-bounds assertions updated to [nominal, 1.2×nominal]. PR2 (#4065) will need rebasing and test updates after this lands.

Regression suite for the backoff gate introduced in PR1 (#4062). Test files: - pkg/tools/startable_backoff_regression_test.go: 9 consumer-shaped regression tests using fakes modelled on real RAG/MCP/LSP error shapes — RAG-shaped failure+recovery, MCP/LSP compatibility (fail-fast, no backoff), HTTP 408 gate, concurrent starts no-multiplication, no timer/goroutine leak, cancellation no-window, jitter de-synchronization, already-started no-restart. - pkg/tools/builtin/rag/rag_backoff_test.go: 2 real-toolset RAG tests using rag.New + countingStatusErrStrategy; proves the real toolset's StatusError wrapping chain is traversable by errors.As and that plain errors fail fast. Docs: - docs/tools/rag/index.md: authoritative 'Indexing failures, retries and backoff' section — retry policy (1s base, 30s cap, equal jitter), what triggers backoff (429, 408, 5xx) vs fail-fast (other 4xx, cancellation), operational impact and troubleshooting guidance. - docs/tools/mcp/index.md: short note under Lifecycle clarifying MCP local startup failures fail fast; links to RAG page for full policy. - docs/tools/lsp/index.md: matching note under Auto-Restart and Lifecycle. No production code changes.

[blocking] partial-start comment corrected; known limitation for code-mode composites documented with reference to follow-up issue #4067 [SF1] startBackoffRetryable requires *StatusError — regex fallback excluded to prevent false positives on port numbers and chunk counters; RetryableHTTPStatus doc corrected to describe the actual regex-fallback behaviour [SF2] TestStartableToolSet_BlockingStartSkipsGate: arm gate, assert blocking Start() invokes underlying (not gated), assert TryStart() is gated [SF3] tryStartLocked: reporter-adoption guarded by !startBackoffUntil.IsZero() — fix is now scoped to gated state; non-gated TryStart semantics unchanged [SF4] stale 'next turn' log messages updated in agent.go and mcp.go [optional] ExportedSetJitter removed (dead code; WithStartRetryJitter preferred)

Refs #4060 (partial): RAG semantic-embeddings indexing triggered a rate-limit retry storm — repeated toolset-start attempts had no pacing after a 429 from the embedding provider (the 15s/5min gate is the partial fix; DefaultStartTimeout and chunk-level checkpointing are deferred). Implementation: - modelerrors.RetryableHTTPStatus(err): HTTP-status classifier that recognises 429, 408, and 5xx via *StatusError first, then falls back to statusCodeRegex. The toolset gate pre-filters to *StatusError via errors.As before calling it, so port numbers and chunk counts in plain error strings cannot arm the gate. - pkg/tools/startable_backoff.go: bounded exponential backoff with additive 0-20% jitter (base=15s, cap=5min, delay∈[d,1.2d]). - Gate in tryStartLocked (TryStart/TryStartWithTimeout only): blocking Start() bypasses it so mcpcatalog enable and skill startup are immediate. - Gate adopts a live StartReporter after /toolset-restart without waiting for the window to expire. - Wrap embedding errors via oaistream.WrapOpenAIError at openai/client.go and dmr/embed.go so a 429 from the embedding provider surfaces as *StatusError and correctly arms the gate. - WithStartRetryJitter / WithStartRetryClock options via variadic NewStartable for deterministic test control. - Stale 'retry on next turn' log messages updated in agent.go/mcp.go. - Partial-start exemption documented (code-mode composites remain unpaced; follow-up at issue #4067). Tests (same commit, covering the above): - startable_backoff_test.go: unit tests for the gate (gate fires on 429/408/5xx StatusError, not on plain text / context errors, blocking Start() ungated, concurrency, jitter bounds) - startable_backoff_regression_test.go: consumer-shaped regression suite (RAG/MCP/LSP error shapes, no-goroutine/timer leak, latch) - rag_backoff_test.go: real-toolset integration test via rag.New + fake clock Docs: - docs/tools/rag/index.md: 'Indexing failures, retries and backoff' section with trigger table, parameters, and troubleshooting. - docs/tools/mcp/index.md, docs/tools/lsp/index.md: lifecycle notes confirming local startup failures fail fast. Scope: DefaultStartTimeout (30s) unchanged — deferred.

A2A toolset startup fetches the remote agent's card via agentcard.Resolver, which returns *agentcard.ErrStatusNotOK{StatusCode, Status} on any non-200 response. #4074's deferral rationale ("the agent-card resolver does not expose HTTP status cleanly") was wrong on this point — the resolver already exposes the status; enrichCardError (pkg/tools/a2a/carderror.go) only needs to translate it into *modelerrors.StatusError via modelerrors.WrapHTTPError, mirroring enrichConnectError's handling of remote MCP HTTP errors (pkg/tools/mcp/remote.go). Toolset.Start now returns enrichCardError's result instead of a bare fmt.Errorf, so retryable responses arm the StartableToolSet backoff gate exactly as remote MCP and RAG embedding 429s do. The one thing the resolver genuinely doesn't expose is the Retry-After header: Resolver.Resolve discards the *http.Response after producing ErrStatusNotOK. retryAfterRecorder is a minimal http.RoundTripper installed only in front of the card-resolution GET (never the JSON-RPC transport chain built afterwards) that records the status and Retry-After of the most recent >=400 response; unlike oauthTransport's lastServerErrorSnapshot it carries no mutex, since Start issues exactly one synchronous card GET per attempt. Its header is only forwarded when its recorded status matches ErrStatusNotOK's own StatusCode, so a header from an unrelated response can never be paired with the wrong status. Classifier policy is unchanged from #4062/#4074: the gate arms only on a fixed enumeration (429, 408, 500, 502, 503, 504, 529), not a full 5xx range — 501, 505, and the Cloudflare 520-527 family do not arm it. Deliberately excluded from arming: DNS failures, connection refused, SSRF-blocked private-IP targets, malformed/unparsable agent cards, and other 4xx statuses (bad auth, bad config) — all fail promptly with no pacing. Only the agent-card fetch during startup is paced; per-call SendStreamingMessage failures on an already-started toolset are not. Adds carderror_test.go (retryable/non-retryable status tables incl. 501 to prove the enumeration isn't a full 5xx range, no-status cases for connection-refused/SSRF-block/malformed-card, and Retry-After present/absent) plus backoff_test.go end-to-end tests that drive a real *Toolset through tools.StartableToolSet.TryStart against a mock 503/403 agent-card server, and a recovery test that flips the mock from 503 to a real, working agent card + JSON-RPC handshake after the backoff window elapses. docs/tools/a2a/index.md documents the new "Startup failure behaviour" section with the same precision as MCP's; docs/tools/rag/index.md's cross-reference note now names A2A alongside remote MCP. Refs #4060, #4074, #4098

aheritier marked this pull request as ready for review August 27, 2026 09:30

aheritier requested a review from a team as a code owner August 27, 2026 09:30

aheritier added area/tools For features/issues/fixes related to the usage of built-in and MCP tools kind/fix PR fixes a bug (maps to fix:). Use on PRs only. labels Aug 27, 2026

aheritier mentioned this pull request Aug 27, 2026

test: add StartableToolSet backoff regression tests and docs #4065

Closed