Skip to content

Commit fe986e7

Browse files
committed
fix: add bounded jittered backoff to StartableToolSet retry path
Fixes issue #4060: RAG semantic-embeddings indexing triggered a rate-limit retry storm — repeated toolset-start attempts had no pacing after a 429 from the embedding provider. 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 in WrapHTTPError 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.
1 parent b407a13 commit fe986e7

17 files changed

Lines changed: 1431 additions & 13 deletions

File tree

docs/community/troubleshooting/index.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -199,6 +199,8 @@ MCP tools using stdio transport must complete the initialization handshake befor
199199
200200
If a toolset keeps crashing in a tight loop, tune the [`lifecycle`](../../configuration/tools/index.md#toolset-lifecycle) block on the toolset (e.g. raise `backoff.initial`, lower `max_restarts`, or switch to the `best-effort` profile) so a flaky dependency does not amplify into a restart storm.
201201

202+
If a **RAG knowledge base** is failing to index because the embedding provider is rate-limiting requests (HTTP 429), Docker Agent automatically backs off and retries — see [Indexing failures, retries and backoff](../../tools/rag/index.md#indexing-failures-retries-and-backoff) for the retry schedule and the `max_indexing_concurrency` / `max_embedding_concurrency` knobs that control how much concurrent load is generated.
203+
202204
## Configuration Errors
203205

204206
### YAML syntax issues

docs/tools/lsp/index.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -197,6 +197,8 @@ Available Capabilities:
197197

198198
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.
199199

200+
**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.
201+
200202
```yaml
201203
toolsets:
202204
- type: lsp

docs/tools/mcp/index.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -258,6 +258,8 @@ toolsets:
258258

259259
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.
260260

261+
**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.
262+
261263
## Combined Example
262264

263265
```yaml

docs/tools/rag/index.md

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -174,6 +174,89 @@ chunking:
174174
>
175175
> Currently supports Go (`.go`) files. More languages will be added. Falls back to plain text chunking for unsupported file types.
176176

177+
## Indexing failures, retries and backoff
178+
179+
When a knowledge-base fails to start — because the embedding provider is rate-limiting
180+
your requests or returning a transient server error — Docker Agent spaces out retry
181+
attempts with bounded exponential backoff instead of hammering the provider on
182+
every agent turn.
183+
184+
### What triggers backoff
185+
186+
Backoff applies only to **HTTP 429 rate-limit** responses from the embedding or
187+
model provider — the one signal that reliably reaches the toolset gate. Other
188+
errors (5xx, 408) are handled per-file within the indexing run and do not arm
189+
the gate. These are the current gate triggers:
190+
191+
| Failure kind | Behaviour |
192+
|---|---|
193+
| HTTP 429 (rate limit) | Backoff: next attempt delayed |
194+
| Other failures (5xx, 408, config errors, auth) | Fail fast: retried every turn with no added delay |
195+
| Context cancellation or agent shutdown | Immediate: no delay |
196+
197+
> [!NOTE]
198+
> 5xx and 408 errors from the embedding provider are retried per-file and do not
199+
> propagate to the toolset gate. Only 429 (rate-limit) terminates the indexing run
200+
> early and surfaces the gate so Docker Agent can pace the next attempt.
201+
202+
### Retry policy and parameters
203+
204+
The backoff is **bounded exponential with additive jitter**:
205+
206+
- **Base delay**: 15 seconds
207+
- **Maximum delay**: 5 minutes
208+
- **Growth**: doubles after each consecutive retryable failure (15s → 30s → 1m → 2m → 4m → 5m)
209+
- **Jitter**: each wait is a random value in `[nominal, 1.2×nominal]` (additive 0–20%)
210+
so concurrent knowledge-base sources spread their retries and avoid
211+
hammering the provider together
212+
213+
The gate is a lightweight wall-clock check — it creates no background threads or
214+
timers. A Stop command or agent shutdown takes effect immediately regardless of
215+
how much of the backoff window remains.
216+
217+
### Operational impact
218+
219+
**Before**: a rate-limited knowledge base was re-indexed on every agent turn —
220+
`max_indexing_concurrency × max_embedding_concurrency` concurrent provider calls
221+
could relaunch within milliseconds, easily tripping rate limits for both the
222+
knowledge base and the agent's own model calls.
223+
224+
**After**: retries are spaced out and jittered so the provider has room to recover
225+
before the next attempt. The agent continues working with any other toolsets that
226+
are not affected.
227+
228+
### What you will see
229+
230+
- Docker Agent logs a single warning when a knowledge base first fails to start.
231+
Repeated failures in between are logged at debug level only, so you are not
232+
flooded with alerts on every turn. Recovery is intentionally silent — the
233+
tool appearing in the agent's tool list is the signal that indexing succeeded.
234+
- The knowledge-base tool does not appear in the agent's tool list until indexing
235+
succeeds. A successful start is silent — the tool is listed and the agent uses it.
236+
237+
### Troubleshooting repeated 429 errors
238+
239+
If you see persistent `429` errors in the logs:
240+
241+
1. **Check provider rate limits.** Your embedding API key may have a low requests-per-minute
242+
quota. Upgrading the plan or using a different API key can help.
243+
2. **Reduce concurrency.** The chunked-embeddings and semantic-embeddings strategies
244+
accept `max_indexing_concurrency` (default `3`) and `max_embedding_concurrency`
245+
(default `3`) parameters. Lowering these reduces simultaneous requests:
246+
247+
```yaml
248+
rag:
249+
docs:
250+
docs: [./knowledge-base]
251+
strategies:
252+
- type: chunked-embeddings
253+
max_indexing_concurrency: 1
254+
max_embedding_concurrency: 1
255+
```
256+
257+
3. **Use a model with a higher quota.** Some providers offer higher rate limits on
258+
specific embedding model tiers.
259+
177260
## Debugging RAG
178261

179262
Enable debug logging to see retrieval details:
@@ -218,6 +301,7 @@ Look for log tags: `[RAG Manager]`, `[Chunked-Embeddings Strategy]`, `[BM25 Stra
218301
| `limit` | int | `5` | Max results from this strategy |
219302
| `embedding_batch_size` | int | `50` | Chunks per embedding request |
220303
| `max_embedding_concurrency` | int | `3` | Max concurrent embedding requests |
304+
| `max_indexing_concurrency` | int | `3` | Max concurrent file-indexing tasks |
221305
| `chunking.size` | int | `1500` | Chunk size in characters (`4000` when `code_aware` is set) |
222306
| `chunking.overlap` | int | `75` | Overlap between chunks in characters |
223307
| `chunking.code_aware` | bool | `false` | AST-based chunking (Go files only) |

pkg/agent/agent.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -645,10 +645,10 @@ func (a *Agent) ensureToolSetsAreStarted(ctx context.Context) {
645645
continue
646646
}
647647
if toolSet.ShouldReportFailure() {
648-
slog.WarnContext(ctx, "Toolset start failed; will retry on next turn", "agent", a.Name(), "toolset", desc, "error", err)
648+
slog.WarnContext(ctx, "Toolset start failed; will retry (backoff may apply)", "agent", a.Name(), "toolset", desc, "error", err)
649649
a.AddToolWarning(fmt.Sprintf("%s start failed: %v", desc, err))
650650
} else {
651-
slog.DebugContext(ctx, "Toolset still unavailable; retrying next turn", "agent", a.Name(), "toolset", desc, "error", err)
651+
slog.DebugContext(ctx, "Toolset still unavailable; will retry (backoff may apply)", "agent", a.Name(), "toolset", desc, "error", err)
652652
}
653653
}
654654
}

pkg/model/provider/dmr/embed.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import (
1717
"github.com/openai/openai-go/v3"
1818

1919
"github.com/docker/docker-agent/pkg/model/provider/base"
20+
"github.com/docker/docker-agent/pkg/model/provider/oaistream"
2021
"github.com/docker/docker-agent/pkg/rag/types"
2122
)
2223

@@ -50,7 +51,7 @@ func (c *Client) CreateBatchEmbedding(ctx context.Context, texts []string) (*bas
5051
Model: c.ModelConfig.Model,
5152
})
5253
if err != nil {
53-
return nil, fmt.Errorf("failed to create embeddings: %w", err)
54+
return nil, fmt.Errorf("failed to create embeddings: %w", oaistream.WrapOpenAIError(err))
5455
}
5556

5657
if len(response.Data) != len(texts) {
Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
package dmr
2+
3+
import (
4+
"net/http"
5+
"net/http/httptest"
6+
"testing"
7+
8+
openaisdk "github.com/openai/openai-go/v3"
9+
"github.com/stretchr/testify/assert"
10+
"github.com/stretchr/testify/require"
11+
12+
"github.com/docker/docker-agent/pkg/config/latest"
13+
"github.com/docker/docker-agent/pkg/modelerrors"
14+
)
15+
16+
// TestCreateBatchEmbedding_429SurfacesAsStatusError proves that when the
17+
// embedding endpoint returns HTTP 429 the error propagates as
18+
// *modelerrors.StatusError so the StartableToolSet backoff gate can arm.
19+
func TestCreateBatchEmbedding_429SurfacesAsStatusError(t *testing.T) {
20+
t.Parallel()
21+
22+
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
23+
w.Header().Set("Content-Type", "application/json")
24+
w.WriteHeader(http.StatusTooManyRequests)
25+
_, _ = w.Write([]byte(`{"error":{"message":"rate limit exceeded","type":"requests","code":"rate_limit_exceeded"}}`))
26+
}))
27+
t.Cleanup(srv.Close)
28+
29+
cfg := &latest.ModelConfig{
30+
Provider: "dmr",
31+
Model: "ai/nomic-embed-text",
32+
BaseURL: srv.URL + "/v1",
33+
}
34+
client, err := NewClient(t.Context(), cfg, nil)
35+
require.NoError(t, err)
36+
37+
_, err = client.CreateBatchEmbedding(t.Context(), []string{"hello"})
38+
require.Error(t, err, "expected error from 429 response")
39+
40+
// The critical assertion: the OpenAI SDK error must be wrapped in
41+
// *modelerrors.StatusError by WrapOpenAIError so the backoff gate can arm.
42+
var se *modelerrors.StatusError
43+
require.ErrorAs(t, err, &se,
44+
"error must contain *modelerrors.StatusError so the backoff gate can arm")
45+
assert.Equal(t, http.StatusTooManyRequests, se.StatusCode)
46+
47+
// Cross-check: the underlying SDK error is also reachable.
48+
var apiErr *openaisdk.Error
49+
assert.ErrorAs(t, err, &apiErr,
50+
"underlying *openaisdk.Error must be reachable through the chain")
51+
}

pkg/model/provider/openai/client.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1165,7 +1165,7 @@ func (c *Client) CreateBatchEmbedding(ctx context.Context, texts []string) (*bas
11651165
response, err := client.Embeddings.New(ctx, params)
11661166
if err != nil {
11671167
slog.ErrorContext(ctx, "OpenAI batch embedding request failed", "error", err)
1168-
return nil, fmt.Errorf("failed to create batch embeddings: %w", err)
1168+
return nil, fmt.Errorf("failed to create batch embeddings: %w", oaistream.WrapOpenAIError(err))
11691169
}
11701170

11711171
if len(response.Data) != len(texts) {

pkg/modelerrors/modelerrors.go

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -781,3 +781,18 @@ func scalarString(v any) string {
781781
return fmt.Sprint(v)
782782
}
783783
}
784+
785+
// RetryableHTTPStatus reports whether err contains an HTTP status that warrants
786+
// backoff (429 rate-limit, 408 request-timeout, or a 5xx server error). It
787+
// checks for a *StatusError in the chain first; if none is found it falls back
788+
// to matching \b([45]\d{2})\b in the error message, which can produce false
789+
// positives for port numbers or similar numeric patterns. Callers that need
790+
// strict StatusError-only classification should pre-filter with errors.As.
791+
// No context-error handling is performed.
792+
func RetryableHTTPStatus(err error) bool {
793+
code := extractHTTPStatusCode(err)
794+
if code == 0 {
795+
return false
796+
}
797+
return code == http.StatusTooManyRequests || isRetryableStatusCode(code)
798+
}

pkg/modelerrors/modelerrors_test.go

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -944,3 +944,57 @@ func TestScalarStringEdgeCases(t *testing.T) {
944944
})
945945
}
946946
}
947+
948+
func TestRetryableHTTPStatus(t *testing.T) {
949+
t.Parallel()
950+
951+
tests := []struct {
952+
name string
953+
err error
954+
expected bool
955+
}{
956+
// Retryable HTTP codes via *StatusError.
957+
{name: "429 rate-limit StatusError", err: &StatusError{StatusCode: 429, Err: errors.New("rate limited")}, expected: true},
958+
{name: "408 request timeout StatusError", err: &StatusError{StatusCode: 408, Err: errors.New("timeout")}, expected: true},
959+
{name: "500 server error StatusError", err: &StatusError{StatusCode: 500, Err: errors.New("internal server error")}, expected: true},
960+
{name: "503 unavailable StatusError", err: &StatusError{StatusCode: 503, Err: errors.New("service unavailable")}, expected: true},
961+
{name: "529 overloaded StatusError", err: &StatusError{StatusCode: 529, Err: errors.New("overloaded")}, expected: true},
962+
// Non-retryable HTTP codes via *StatusError.
963+
{name: "400 bad request StatusError", err: &StatusError{StatusCode: 400, Err: errors.New("bad request")}, expected: false},
964+
{name: "401 unauthorized StatusError", err: &StatusError{StatusCode: 401, Err: errors.New("unauthorized")}, expected: false},
965+
{name: "403 forbidden StatusError", err: &StatusError{StatusCode: 403, Err: errors.New("forbidden")}, expected: false},
966+
{name: "404 not found StatusError", err: &StatusError{StatusCode: 404, Err: errors.New("not found")}, expected: false},
967+
// Plain string errors without HTTP codes: must stay non-retryable so
968+
// MCP/LSP "connection refused" errors are not paced.
969+
{name: "connection refused", err: errors.New("connection refused: dial tcp 127.0.0.1:9999"), expected: false},
970+
{name: "no such host", err: errors.New("no such host: example.invalid"), expected: false},
971+
// Plain text that DOES contain a retryable HTTP code: the regex fallback
972+
// in extractHTTPStatusCode finds it. NOTE: RetryableHTTPStatus returns
973+
// true here, but startBackoffRetryable (the toolset gate classifier)
974+
// requires a *StatusError and returns false for the same input — the
975+
// narrowing is deliberate to prevent port-number/chunk-count false positives.
976+
{name: "503 in plain text", err: errors.New("upstream: 503 Service Unavailable"), expected: true},
977+
{name: "429 in plain text", err: errors.New("provider said: 429 Too Many Requests"), expected: true},
978+
// HTTP-status precedence: a StatusError{429} wrapped alongside
979+
// context.DeadlineExceeded must return true — the HTTP signal wins.
980+
{
981+
name: "429 StatusError + DeadlineExceeded in chain",
982+
err: fmt.Errorf("start budget exceeded: %w, provider said: %w",
983+
context.DeadlineExceeded,
984+
&StatusError{StatusCode: 429, Err: errors.New("rate limited")}),
985+
expected: true,
986+
},
987+
// Bare context errors: no HTTP code, must return false.
988+
{name: "bare DeadlineExceeded", err: context.DeadlineExceeded, expected: false},
989+
{name: "bare Canceled", err: context.Canceled, expected: false},
990+
{name: "nil error", err: nil, expected: false},
991+
}
992+
993+
for _, tc := range tests {
994+
t.Run(tc.name, func(t *testing.T) {
995+
t.Parallel()
996+
got := RetryableHTTPStatus(tc.err)
997+
assert.Equal(t, tc.expected, got)
998+
})
999+
}
1000+
}

0 commit comments

Comments
 (0)