Skip to content

test: add StartableToolSet backoff regression tests and docs - #4065

Open
aheritier wants to merge 2 commits into
fix/startable-toolset-backofffrom
fix/startable-toolset-backoff-tests
Open

test: add StartableToolSet backoff regression tests and docs#4065
aheritier wants to merge 2 commits into
fix/startable-toolset-backofffrom
fix/startable-toolset-backoff-tests

Conversation

@aheritier

Copy link
Copy Markdown
Collaborator

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

Depends on PR #4062 (stacked). Base retargets to main once PR1 merges.

Regression suite and documentation for the backoff gate added in PR1.

Tests (no production code changes):

  • pkg/tools/startable_backoff_regression_test.go — 9 consumer-shaped scenarios: RAG-shaped failure→backoff→recovery (with realistic two-level wrapping), HTTP 408 gate, MCP/LSP compatibility (fail-fast, unchanged), concurrent starts don't multiply retries, no timer/goroutine leak (synctest settling), cancellation no-window, jitter de-sync, already-started not restarted
  • pkg/tools/builtin/rag/rag_backoff_test.go — 2 real-toolset tests using rag.New + a counting strategy: StatusError(429) → gate fires, plain error → fail-fast

Docs:

  • docs/tools/rag/index.md — new ## Indexing failures, retries and backoff section: trigger table (429, 408, 5xx backoff; other 4xx/cancellation fail-fast), parameters (1s/30s/equal-jitter), before/after impact, troubleshooting
  • docs/tools/mcp/index.md + docs/tools/lsp/index.md — short lifecycle notes clarifying local failures fail fast, cross-linking to RAG section

Key architectural note: the conservative classifier (PR1) means only RAG embedding calls surface structured StatusErrors in practice. MCP and LSP local failures fail fast — their behavior is unchanged, which this PR's compatibility tests lock in.

@aheritier
aheritier marked this pull request as ready for review August 27, 2026 12:17
@aheritier
aheritier requested a review from a team as a code owner August 27, 2026 12:17
@aheritier aheritier added area/docs Documentation changes area/rag For work/issues that have to do with the RAG features area/testing Test infrastructure, CI/CD, test runners, evaluation area/tools For features/issues/fixes related to the usage of built-in and MCP tools kind/test Test-only changes labels Aug 27, 2026
Sayt-0
Sayt-0 previously approved these changes Aug 27, 2026
aheritier added a commit that referenced this pull request Aug 27, 2026
…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.
@aheritier
aheritier marked this pull request as draft August 27, 2026 14:39
@aheritier

Copy link
Copy Markdown
Collaborator Author

Review: needs work before this can land

COMMENT only — not approvable yet. This PR is still a draft, Go CI has not run for this stacked branch, and the test/docs changes are out of sync with the current #4062 implementation.

The test approach has genuine value—especially the consumer-shaped fakes, synctest coverage, and real rag.New integration coverage—but it needs to be rebased and retargeted before it can validate #4060’s production path.

CI status: Go CI did not run

Only documentation workflows ran and passed. This PR targets fix/startable-toolset-backoff, while the Go CI and CodeQL workflows trigger only for pull requests targeting main.

Therefore, build-and-test, lint, Windows tests, and CodeQL did not execute for these new Go tests. The PR should be rebased/retargeted as appropriate and receive full Go CI before approval.


[blocking] The stack is stale; five tests fail against the current #4062 head

This PR is based on the first #4062 commit (5974d12), while #4062 has since advanced through 072cc4a and e74816b.

Those changes altered the exact contracts encoded here:

Contract This PR assumes Current #4062 behavior
Retry gate startLocked; blocking Start() is gated tryStartLocked; only non-blocking TryStart* is gated
Base / maximum delay 1s / 30s 15s / 5min
Jitter equal [d/2, d] additive [d, 1.2d]
Classifier model-status-specific modelerrors.RetryableHTTPStatus

Rebasing onto the current #4062 implementation produces failures in:

  • TestBackoffRegression_RAGShapedFailureAndRecovery
  • TestBackoffRegression_HTTP408EngagesGate
  • TestBackoffRegression_ConcurrentStartsNoMultiplication
  • TestBackoffRegression_JitterDeSynchronizesRetries
  • TestRAGStartableBackoff_StatusErrorEngagesGate

Please rebase this PR before refining the test expectations. The failures are behavioral contract drift, not build or lint failures.


[blocking] The tests exercise Start(), not the production retry path

The retry storm in #4060 occurs through the non-blocking per-turn startup path:

  • pkg/agent/agent.goTryStartWithTimeout
  • pkg/runtime/runtime.goTryStartWithTimeout

Current #4062 deliberately keeps blocking Start() immediate; the retry gate applies through TryStart / TryStartWithTimeout.

However, the gating scenarios in this PR call Start() directly, for example in:

  • pkg/tools/startable_backoff_regression_test.go
  • pkg/tools/builtin/rag/rag_backoff_test.go

After rebasing, those assertions either fail outright or pass without testing the intended gate. Please:

  1. Move positive gate assertions to TryStart / TryStartWithTimeout.
  2. Add an explicit inverse test proving that blocking Start() remains immediate and bypasses automatic backoff.
  3. Keep the concurrency test, but drive concurrent TryStart calls at the retry boundary so it verifies the real single-flight behavior.

This is important: otherwise the suite does not protect the code path that produced the incident.


[blocking] Delay and jitter documentation is materially incorrect

docs/tools/rag/index.md currently describes:

  • base delay: 1 second
  • maximum delay: 30 seconds
  • equal jitter: [nominal/2, nominal]

Current #4062 implements:

  • base delay: 15 seconds
  • maximum delay: 5 minutes
  • additive jitter: [nominal, 1.2 × nominal]

The existing docs understate the possible delay by an order of magnitude and describe the wrong jitter model. Please update the RAG documentation and test arithmetic together, preferably using exported constants in tests rather than duplicating literal bounds.

The current 31s sleep-based assertion in the RAG test must also be replaced with the injected retry clock; it is no longer beyond the maximum retry window.


[blocking] MCP/LSP documentation promises fail-fast behavior the current classifier does not guarantee

The RAG/MCP/LSP docs state that only structured HTTP errors from backing model providers trigger startup pacing and that local MCP/LSP failures fail fast.

Current #4062 delegates to modelerrors.RetryableHTTPStatus, which can extract 4xx/5xx-looking status values from error text through a regex fallback. As a result, errors such as these can enter the backoff path regardless of origin:

  • remote MCP handshake failure containing 503
  • local endpoint/process error containing a status-like 503
  • LSP/process error containing an incidental 5xx-like number

Either:

  1. narrow the classifier in fix: add bounded jittered backoff to StartableToolSet retry path #4062 to match the documented guarantee; or
  2. revise the RAG, MCP, and LSP docs—and this PR’s compatibility tests—to accurately describe the broader behavior.

Please add a test for a non-model/local error string containing a status-shaped value. That would mechanically protect the intended contract whichever direction we choose.


[should-fix] Cover TryStartWithTimeout timeout behavior

Since this is intended as the regression suite for #4060, add focused coverage for the timeout path:

  • an abandoned in-flight start must not schedule a second overlapping attempt;
  • the retry window is armed only when the underlying attempt actually settles with a qualifying failure;
  • cancellation/deadline errors do not themselves create a backoff window.

This is the most relevant lifecycle edge case for expensive RAG initialization.

[should-fix] Correct the recovery-warning claim

The RAG docs currently say a warning is logged again on recovery. The current success path intentionally suppresses recovery logging. Please either remove that statement or add the actual recovery signal in the implementation—preferably the former for this PR, to preserve scope.

[should-fix] Reduce duplicated coverage

Several scenarios overlap the base #4062 test suite: basic 429 gating, cancellation, retry boundary behavior, and jitter. The highest-value additions here are:

  • RAG integration through the real error-wrapping chain;
  • MCP/LSP compatibility cases;
  • concurrent non-blocking attempts;
  • no-timer/no-goroutine-leak behavior.

Consider consolidating duplicated generic cases into the #4062 suite so policy changes have one authoritative set of timing expectations.

[optional] Strengthen the jitter assertion

After updating to additive jitter, assert the full expected range for every sample ([d, 1.2d]) as well as meaningful variation. len(seen) > 1 alone is too weak to catch a number of broken jitter implementations.


Suggested path forward

  1. Rebase onto the latest fix: add bounded jittered backoff to StartableToolSet retry path #4062 head, or retarget to main after fix: add bounded jittered backoff to StartableToolSet retry path #4062 merges.
  2. Replace blocking Start() gate assertions with TryStart / TryStartWithTimeout.
  3. Add an explicit test that blocking Start() remains ungated.
  4. Derive test windows from the current exported base/max constants and injected clock.
  5. Update docs to the actual 15s → 30s → 1m → 2m → 4m → 5m progression and additive jitter.
  6. Reconcile MCP/LSP/RAG wording with the current status-extraction classifier.
  7. Ensure full Go CI runs on the rebased/retargeted PR.

The structure is promising. Once it follows the current #4062 contract, this should provide valuable cross-layer regression coverage for #4060.

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.
aheritier added a commit that referenced this pull request Aug 27, 2026
…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.
@aheritier
aheritier force-pushed the fix/startable-toolset-backoff-tests branch from 2ef2ff5 to 0ab3683 Compare August 27, 2026 15:48
@aheritier
aheritier requested a review from docker-agent August 27, 2026 15:53
Gate is now in TryStart only (blocking Start() is ungated); bounds changed
to 15s base / 5min cap with additive jitter [d, 1.2d].

- Switch all gate assertions from s.Start() to s.TryStart()
- Add TestBackoffRegression_BlockingStartIsNeverGated (inverse gate test)
- ConcurrentStartsNoMultiplication: goroutines use TryStart (tests real path)
- JitterDeSynchronizesRetries: fix bounds to [nominal, 1.2*nominal], strengthen assertion
- RAG backoff test: use TryStart + WithStartRetryClock fake clock (no synctest)
- Docs: update delay parameters (15s/5min/additive jitter), fix stale
  recovery-warning claim (success path is silent)
@aheritier
aheritier marked this pull request as ready for review August 27, 2026 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/docs Documentation changes area/rag For work/issues that have to do with the RAG features area/testing Test infrastructure, CI/CD, test runners, evaluation area/tools For features/issues/fixes related to the usage of built-in and MCP tools kind/test Test-only changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants