diff --git a/docs/tools/lsp/index.md b/docs/tools/lsp/index.md index b2d97b7227..916d8efb38 100644 --- a/docs/tools/lsp/index.md +++ b/docs/tools/lsp/index.md @@ -197,6 +197,8 @@ 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 process failures such as a missing binary or server-unavailable error fail fast — each turn retries immediately with no artificial delay. The backoff described in [Indexing failures, retries and backoff](../rag/index.md#indexing-failures-retries-and-backoff) applies only to structured HTTP rate-limit and server-error responses from model providers, which local LSP process errors are not. + ```yaml toolsets: - type: lsp diff --git a/docs/tools/mcp/index.md b/docs/tools/mcp/index.md index e8984c6f18..ed29f8425f 100644 --- a/docs/tools/mcp/index.md +++ b/docs/tools/mcp/index.md @@ -243,6 +243,8 @@ See [Per-Toolset Model Routing](../../configuration/tools/index.md#per-toolset-m Local stdio and remote MCP servers are supervised: crashed servers reconnect automatically with exponential backoff. **Remote** MCP servers (Streamable HTTP / SSE) also reconnect after idle/clean connection closes — services like Notion and Linear periodically close idle connections, and Docker Agent reconnects transparently. Tune the policy with the `lifecycle` block: +**Startup failure behaviour:** local process failures (missing binary, connection refused, authentication error) fail fast — each turn retries immediately with no artificial delay added. Only structured HTTP rate-limit (HTTP 429) and server error (HTTP 5xx) responses from a backing model provider trigger a backoff window. See [Indexing failures, retries and backoff](../rag/index.md#indexing-failures-retries-and-backoff) for the full policy. + ```yaml toolsets: - type: mcp diff --git a/docs/tools/rag/index.md b/docs/tools/rag/index.md index 0aa77a9935..7ee5cf5415 100644 --- a/docs/tools/rag/index.md +++ b/docs/tools/rag/index.md @@ -174,6 +174,86 @@ chunking: > > Currently supports Go (`.go`) files. More languages will be added. Falls back to plain text chunking for unsupported file types. +## Indexing failures, retries and backoff + +When a knowledge-base fails to start — because the embedding provider is rate-limiting +your requests or returning a transient server error — Docker Agent spaces out retry +attempts with bounded exponential backoff instead of hammering the provider on +every agent turn. + +### What triggers backoff + +Backoff applies only to **structured HTTP rate-limit (429), request timeout (408), and server error (5xx)** +responses from the embedding or model provider. These are the signals that mean +"slow down", and spacing them out gives the provider room to recover: + +| Failure kind | Behaviour | +|---|---| +| HTTP 429 (rate limit) | Backoff: next attempt delayed | +| HTTP 5xx / overload (500, 502, 503, 504, 529) | Backoff: next attempt delayed | +| HTTP 408 (request timeout) | Backoff: next attempt delayed | +| Bad config, invalid model, wrong API key (other 4xx errors) | Fail fast: retried every turn with no added delay | +| Context cancellation or agent shutdown | Immediate: no delay | + +### Retry policy and parameters + +The backoff is **bounded exponential with additive jitter**: + +- **Base delay**: 15 seconds +- **Maximum delay**: 5 minutes +- **Growth**: doubles after each consecutive retryable failure (15s → 30s → 1m → 2m → 4m → 5m) +- **Jitter**: each wait is a random value in `[nominal, 1.2×nominal]` (additive 0–20%) + so concurrent knowledge-base sources spread their retries and avoid + hammering the provider together + +The gate is a lightweight wall-clock check — it creates no background threads or +timers. A Stop command or agent shutdown takes effect immediately regardless of +how much of the backoff window remains. + +### Operational impact + +**Before**: a rate-limited knowledge base was re-indexed on every agent turn — +`max_indexing_concurrency × max_embedding_concurrency` concurrent provider calls +could relaunch within milliseconds, easily tripping rate limits for both the +knowledge base and the agent's own model calls. + +**After**: retries are spaced out and jittered so the provider has room to recover +before the next attempt. The agent continues working with any other toolsets that +are not affected. + +### What you will see + +- Docker Agent logs a single warning when a knowledge base first fails to start. + Repeated failures in between are logged at debug level only, so you are not + flooded with alerts on every turn. Recovery is intentionally silent — the + tool appearing in the agent's tool list is the signal that indexing succeeded. +- The knowledge-base tool does not appear in the agent's tool list until indexing + succeeds. A successful start is silent — the tool is listed and the agent uses it. + +### Troubleshooting repeated 429 errors + +If you see persistent `429` errors in the logs: + +1. **Check provider rate limits.** Your embedding API key may have a low requests-per-minute + quota. Upgrading the plan or using a different API key can help. +2. **Reduce concurrency.** The chunked-embeddings and semantic-embeddings strategies + accept `max_indexing_concurrency` (default `3`) and `max_embedding_concurrency` + (default `3`) parameters. Lowering these reduces simultaneous requests: + + ```yaml + rag: + name: docs + path: ./knowledge-base + strategy: + type: chunked-embeddings + params: + max_indexing_concurrency: 1 + max_embedding_concurrency: 1 + ``` + +3. **Use a model with a higher quota.** Some providers offer higher rate limits on + specific embedding model tiers. + ## Debugging RAG Enable debug logging to see retrieval details: diff --git a/pkg/tools/builtin/rag/rag_backoff_test.go b/pkg/tools/builtin/rag/rag_backoff_test.go new file mode 100644 index 0000000000..4010e1eef9 --- /dev/null +++ b/pkg/tools/builtin/rag/rag_backoff_test.go @@ -0,0 +1,129 @@ +package rag + +// Regression tests proving that the real RAG ToolSet, when wrapped in +// tools.NewStartable, correctly engages the backoff gate for structured +// HTTP-status failures (e.g. a 429 rate-limit from the embedding provider) +// and does NOT throttle plain (non-StatusError) failures. +// +// Gate enforcement is in tryStartLocked (TryStart paths only); all gate +// assertions here use TryStart, not Start. A frozen fake clock supplied via +// WithStartRetryClock gives deterministic windows without needing synctest. + +import ( + "context" + "fmt" + "sync/atomic" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/docker/docker-agent/pkg/modelerrors" + "github.com/docker/docker-agent/pkg/rag" + "github.com/docker/docker-agent/pkg/rag/strategy" + "github.com/docker/docker-agent/pkg/tools" +) + +// countingStatusErrStrategy is a mockStrategy variant whose Initialize +// returns a *modelerrors.StatusError and counts every invocation. +type countingStatusErrStrategy struct { + mockStrategy + + statusCode int + calls atomic.Int32 +} + +func (s *countingStatusErrStrategy) Initialize(_ context.Context, _ []string, _ strategy.ChunkingConfig) error { + s.calls.Add(1) + return &modelerrors.StatusError{ + StatusCode: s.statusCode, + Err: fmt.Errorf("HTTP %d error from provider", s.statusCode), + } +} + +// countingPlainErrStrategy always returns a plain (non-StatusError) error +// so the backoff gate must never fire. +type countingPlainErrStrategy struct { + mockStrategy + + calls atomic.Int32 +} + +func (s *countingPlainErrStrategy) Initialize(_ context.Context, _ []string, _ strategy.ChunkingConfig) error { + s.calls.Add(1) + return assert.AnError +} + +func buildRAGToolSet(t *testing.T, impl strategy.Strategy) *ToolSet { + t.Helper() + cfg := rag.Config{ + StrategyConfigs: []strategy.Config{ + {Name: "test-strategy", Strategy: impl}, + }, + } + mgr, err := rag.New(t.Context(), "test-rag", cfg, nil) + require.NoError(t, err) + return &ToolSet{manager: mgr, toolName: "test-rag"} +} + +// TestRAGStartableBackoff_StatusErrorEngagesGate proves that the real RAG +// ToolSet, when it fails with a *modelerrors.StatusError, arms the backoff +// gate in TryStart: the immediate retry is gated and the underlying is not +// called again until the window expires. +// +// A frozen fake clock (WithStartRetryClock) and identity jitter give exact, +// deterministic windows without synctest complications from the RAG manager's +// internal goroutines. +func TestRAGStartableBackoff_StatusErrorEngagesGate(t *testing.T) { + t.Parallel() + + counting := &countingStatusErrStrategy{statusCode: 429} + toolset := buildRAGToolSet(t, counting) + + // Frozen clock: window = base + 0% jitter = exactly base. + // We advance the clock manually to control expiry. + now := time.Unix(1_000_000, 0) + s := tools.NewStartable(toolset, + tools.WithStartRetryJitter(func(d time.Duration) time.Duration { return d }), // identity + tools.WithStartRetryClock(func() time.Time { return now }), + ) + + // Attempt 1: TryStart invokes Initialize and arms the gate. + _, err := s.TryStart(t.Context()) + require.Error(t, err, "expected failure on attempt 1") + assert.Equal(t, int32(1), counting.calls.Load(), "Initialize must be called on attempt 1") + + // Immediate TryStart: gate must suppress it (clock not advanced). + _, err = s.TryStart(t.Context()) + require.Error(t, err) + assert.Equal(t, int32(1), counting.calls.Load(), + "gate must suppress TryStart within the window") + + // Advance clock past base window (base × 1.0 with identity jitter). + // Use 6 minutes to exceed any possible jittered window for any attempt. + now = now.Add(6 * time.Minute) + + // Gate expired: TryStart must invoke Initialize again. + _, err = s.TryStart(t.Context()) + require.Error(t, err, "still failing — expected error") + assert.Equal(t, int32(2), counting.calls.Load(), + "Initialize must be called again once the backoff window expires") +} + +// TestRAGStartableBackoff_PlainErrorNoGate proves that a plain (non-StatusError) +// failure does NOT arm the gate: every TryStart reaches Initialize. +func TestRAGStartableBackoff_PlainErrorNoGate(t *testing.T) { + t.Parallel() + + counting := &countingPlainErrStrategy{} + toolset := buildRAGToolSet(t, counting) + s := tools.NewStartable(toolset) + + for i := int32(1); i <= 3; i++ { + _, err := s.TryStart(t.Context()) + require.Error(t, err) + assert.Equal(t, i, counting.calls.Load(), + "plain error must not gate subsequent TryStart calls (attempt %d)", i) + } +} diff --git a/pkg/tools/startable_backoff_regression_test.go b/pkg/tools/startable_backoff_regression_test.go new file mode 100644 index 0000000000..af5690e5e7 --- /dev/null +++ b/pkg/tools/startable_backoff_regression_test.go @@ -0,0 +1,301 @@ +package tools_test + +// Regression suite for the StartableToolSet backoff gate (issue #4060). +// +// Gate enforcement is in tryStartLocked, called only from TryStart/TryStartWithTimeout. +// Blocking Start() bypasses the gate and always reaches the underlying toolset — +// that is intentional (mcpcatalog enable, skill sub-session startup must be immediate). +// Gate-assertion tests therefore drive TryStart; compatibility tests use Start or +// TryStart interchangeably (the gate never fires for non-StatusError errors). +// +// The helpers startErrToolSet / retryableErr / rateLimitErr / nonRetryableErr / +// identityJitter / newThrottledStartable are defined in startable_backoff_test.go +// (same package) and are reused directly here. + +import ( + "context" + "errors" + "fmt" + "sync" + "testing" + "testing/synctest" + "time" + + "gotest.tools/v3/assert" + is "gotest.tools/v3/assert/cmp" + + "github.com/docker/docker-agent/pkg/modelerrors" + "github.com/docker/docker-agent/pkg/tools" + "github.com/docker/docker-agent/pkg/tools/lifecycle" +) + +// ragWrappedStatusErr returns a StatusError wrapped through two levels of +// fmt.Errorf, approximating the real RAG toolset error chain +// (Initialize → Manager → ToolSet.Start). +func ragWrappedStatusErr(code int) error { + base := &modelerrors.StatusError{ + StatusCode: code, + Err: fmt.Errorf("HTTP %d from provider", code), + } + aborted := fmt.Errorf("indexing aborted due to non-retryable model error: %w", base) + return fmt.Errorf("failed to initialize RAG manager %q: %w", "knowledge-base", aborted) +} + +// TestBackoffRegression_RAGShapedFailureAndRecovery verifies the full +// backoff-then-recovery cycle for an error shaped like a real RAG toolset +// hitting a rate-limit (429) during indexing. +func TestBackoffRegression_RAGShapedFailureAndRecovery(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + inner := &startErrToolSet{} + inner.setErr(ragWrappedStatusErr(429)) + s := newThrottledStartable(inner) + + // Attempt 1: underlying Start runs via TryStart and fails. + _, err := s.TryStart(t.Context()) + assert.Check(t, err != nil, "expected failure on attempt 1") + assert.Check(t, is.Equal(inner.starts.Load(), int32(1)), "attempt 1 must invoke the underlying") + + // Immediate TryStart retry: gate must suppress it (within the base window). + _, err = s.TryStart(t.Context()) + assert.Check(t, err != nil) + assert.Check(t, is.Equal(inner.starts.Load(), int32(1)), "immediate TryStart must be gated within the window") + + // Advance fake clock past the base window (identity jitter ⇒ window == base exactly). + time.Sleep(tools.ExportedStartBackoffBase + time.Millisecond) //nolint:forbidigo // synctest bubble + + // Gate expired: TryStart must invoke the underlying again. + _, err = s.TryStart(t.Context()) + assert.Check(t, err != nil, "still failing — expected error") + assert.Check(t, is.Equal(inner.starts.Load(), int32(2)), "TryStart must re-run after the window expires") + + // Recovery: advance past the second window (base×2 with identity jitter) + // then clear the error so the next TryStart succeeds. + time.Sleep(2*tools.ExportedStartBackoffBase + time.Millisecond) //nolint:forbidigo // synctest bubble + inner.clearErr() + started, err := s.TryStart(t.Context()) + assert.NilError(t, err, "recovery TryStart must succeed") + assert.Check(t, started) + assert.Check(t, is.Equal(s.IsStarted(), true)) + + // After recovery, stop and verify the next failure starts a fresh base window. + assert.NilError(t, s.Stop(t.Context())) + inner.setErr(ragWrappedStatusErr(503)) + callsBefore := inner.starts.Load() + _, _ = s.TryStart(t.Context()) // arms fresh gate at base + // Immediate TryStart must be gated (fresh base window after reset). + _, _ = s.TryStart(t.Context()) + assert.Check(t, is.Equal(inner.starts.Load(), callsBefore+1), + "after recovery, the next retryable failure must start a fresh base-delay window") + }) +} + +// TestBackoffRegression_MCPShapedCompatibility verifies that Start failures +// shaped like a real MCP toolset (plain connection error, no StatusError) +// do NOT engage the backoff gate. Every call reaches the underlying. +func TestBackoffRegression_MCPShapedCompatibility(t *testing.T) { + t.Parallel() + + inner := &startErrToolSet{} + inner.setErr(errors.New("connection refused: dial tcp 127.0.0.1:9999")) + s := newThrottledStartable(inner) + + // Three consecutive TryStart calls: each must invoke the underlying (no gate). + for i := int32(1); i <= 3; i++ { + _, err := s.TryStart(t.Context()) + assert.Check(t, err != nil) + assert.Check(t, is.Equal(inner.starts.Load(), i), + "MCP connection-error failure must not engage backoff (attempt %d)", i) + } + + inner.clearErr() + assert.NilError(t, s.Start(t.Context()), "recovery must work without a backoff window") + assert.Check(t, is.Equal(s.IsStarted(), true)) +} + +// TestBackoffRegression_LSPShapedCompatibility mirrors the MCP test for the +// LSP toolset error shape (lifecycle.ErrServerUnavailable, not a StatusError). +func TestBackoffRegression_LSPShapedCompatibility(t *testing.T) { + t.Parallel() + + inner := &startErrToolSet{} + inner.setErr(fmt.Errorf("binary not found: %w", lifecycle.ErrServerUnavailable)) + s := newThrottledStartable(inner) + + for i := int32(1); i <= 3; i++ { + _, err := s.TryStart(t.Context()) + assert.Check(t, err != nil) + assert.Check(t, is.Equal(inner.starts.Load(), i), + "LSP ErrServerUnavailable must not engage backoff (attempt %d)", i) + } + + inner.clearErr() + assert.NilError(t, s.Start(t.Context())) + assert.Check(t, is.Equal(s.IsStarted(), true)) +} + +// TestBackoffRegression_BlockingStartIsNeverGated verifies that blocking +// Start() always reaches the underlying toolset even while a TryStart backoff +// window is active. This is intentional: mcpcatalog enable and skill +// sub-session startup use Start() and must be immediate. +func TestBackoffRegression_BlockingStartIsNeverGated(t *testing.T) { + t.Parallel() + + inner := &startErrToolSet{} + inner.setErr(retryableErr()) + + // Freeze the clock so the window stays active for the test duration. + now := time.Unix(1_000_000, 0) + s := tools.NewStartable(inner, + tools.WithStartRetryJitter(identityJitter), + tools.WithStartRetryClock(func() time.Time { return now }), + ) + + // Arm the gate via TryStart. + _, err := s.TryStart(t.Context()) + assert.Check(t, err != nil) + assert.Check(t, is.Equal(inner.starts.Load(), int32(1))) + + // Blocking Start() must bypass the gate and invoke the underlying. + err = s.Start(t.Context()) + assert.Check(t, err != nil, "blocking Start must fail (retryableErr still set)") + assert.Check(t, is.Equal(inner.starts.Load(), int32(2)), + "blocking Start must bypass the gate and reach the underlying") + + // TryStart within the (re-armed) window must still be gated. + _, err = s.TryStart(t.Context()) + assert.Check(t, err != nil) + assert.Check(t, is.Equal(inner.starts.Load(), int32(2)), + "TryStart within window must be gated even after a blocking Start") +} + +// TestBackoffRegression_ConcurrentStartsNoMultiplication verifies that many +// concurrent TryStart calls against a failing toolset do not multiply the +// underlying retry activity. The single-flight TryLock serialises them: the +// first goroutine runs and arms the gate; the rest either skip the lock or +// hit the gate — in both cases without invoking the underlying. +func TestBackoffRegression_ConcurrentStartsNoMultiplication(t *testing.T) { + t.Parallel() + + inner := &startErrToolSet{} + inner.setErr(retryableErr()) + s := newThrottledStartable(inner) // identity jitter → base window stays active + + const goroutines = 20 + var wg sync.WaitGroup + wg.Add(goroutines) + for range goroutines { + go func() { + defer wg.Done() + _, _ = s.TryStart(t.Context()) + }() + } + wg.Wait() + + assert.Check(t, is.Equal(inner.starts.Load(), int32(1)), + "concurrent TryStart calls must invoke the underlying exactly once (single-flight + gate)") +} + +// TestBackoffRegression_GateSpawnsNoTimersOrGoroutines verifies that the +// backoff gate is purely a wall-clock check with no background resources. +func TestBackoffRegression_GateSpawnsNoTimersOrGoroutines(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + inner := &startErrToolSet{} + inner.setErr(retryableErr()) + s := newThrottledStartable(inner) + + // Arm the gate via TryStart. + _, _ = s.TryStart(t.Context()) + + // Several gated TryStart retries — if any created a timer the bubble stalls. + for range 5 { + _, _ = s.TryStart(t.Context()) + } + // Bubble settling proves no leaked goroutines or timers. + }) +} + +// TestBackoffRegression_CancellationNoWindowSet verifies that context +// cancellation does not arm the backoff gate. +func TestBackoffRegression_CancellationNoWindowSet(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + inner := &startErrToolSet{} + inner.setErr(context.Canceled) + s := newThrottledStartable(inner) + + _, _ = s.TryStart(t.Context()) + assert.Check(t, is.Equal(inner.starts.Load(), int32(1))) + + // No window set: next TryStart must reach the underlying immediately. + _, _ = s.TryStart(t.Context()) + assert.Check(t, is.Equal(inner.starts.Load(), int32(2)), + "context cancellation must not arm the backoff gate") + }) +} + +// TestBackoffRegression_JitterDeSynchronizesRetries verifies that additive +// jitter ([nominal, 1.2×nominal]) produces a spread of distinct durations, +// preventing concurrent toolsets from synchronising retries into a burst. +func TestBackoffRegression_JitterDeSynchronizesRetries(t *testing.T) { + t.Parallel() + + const samples = 100 + attempt := 1 + nominal := tools.ExportedStartBackoffBase // 15s + + seen := make(map[time.Duration]bool, samples) + for range samples { + d := tools.ExportedComputeStartBackoff(attempt, nil) // real (random) additive jitter + assert.Check(t, d >= nominal, + "additive jitter floor must be the nominal itself, got %s < %s", d, nominal) + assert.Check(t, d <= nominal+nominal/5, + "additive jitter ceiling must be 1.2×nominal, got %s > %s", d, nominal+nominal/5) + seen[d] = true + } + // With 100 samples over a 3s range in 1ns increments, identical values + // would be astronomically improbable — assert meaningful spread. + assert.Check(t, len(seen) > 5, + "additive jitter must produce meaningful spread; got only %d distinct values", len(seen)) +} + +// TestBackoffRegression_AlreadyStartedNotRestarted pins that a latched +// toolset is not restarted by subsequent Start or TryStart calls. +func TestBackoffRegression_AlreadyStartedNotRestarted(t *testing.T) { + t.Parallel() + + inner := &startErrToolSet{} + s := newThrottledStartable(inner) + + assert.NilError(t, s.Start(t.Context())) + assert.Check(t, is.Equal(inner.starts.Load(), int32(1))) + assert.Check(t, is.Equal(s.IsStarted(), true)) + + for range 3 { + assert.NilError(t, s.Start(t.Context())) + } + assert.Check(t, is.Equal(inner.starts.Load(), int32(1)), + "a latched toolset must not be restarted by subsequent Start calls") +} + +func http408Err() error { + return &modelerrors.StatusError{StatusCode: 408, Err: errors.New("request timeout")} +} + +// TestBackoffRegression_HTTP408EngagesGate verifies that HTTP 408 (Request +// Timeout) arms the backoff gate via TryStart. +func TestBackoffRegression_HTTP408EngagesGate(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + inner := &startErrToolSet{} + inner.setErr(http408Err()) + s := newThrottledStartable(inner) + + _, err := s.TryStart(t.Context()) + assert.Check(t, err != nil, "expected failure on attempt 1") + assert.Check(t, is.Equal(inner.starts.Load(), int32(1))) + + // Immediate TryStart retry: gate must suppress it. + _, err = s.TryStart(t.Context()) + assert.Check(t, err != nil) + assert.Check(t, is.Equal(inner.starts.Load(), int32(1)), + "HTTP 408 must arm the backoff gate: TryStart must not reach the underlying") + }) +}