feat(vmcp): add auth-retry with circuit breaker to BackendClient#4169
feat(vmcp): add auth-retry with circuit breaker to BackendClient#4169
Conversation
yrobla
requested review from
ChrisJBurns,
JAORMX,
amirejaz,
jerm-dro and
jhrozek
as code owners
yrobla
requested review from
Copilot
and removed request for
ChrisJBurns,
JAORMX,
amirejaz,
jerm-dro and
jhrozek
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Large PR Detected
This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.
How to unblock this PR:
Add a section to your PR description with the following format:
## Large PR Justification
[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformationAlternative:
Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.
See our Contributing Guidelines for more details.
This review will be automatically dismissed once you add the justification section.
There was a problem hiding this comment.
Pull request overview
Adds an auth-failure retry decorator (with exponential backoff + per-backend circuit breaker + tracing) around the vMCP BackendClient, expands auth error string detection, and introduces new unit/integration/E2E test coverage plus test utilities to simulate transient HTTP failures.
Changes:
- Wrap
NewHTTPBackendClientwithretryingBackendClientimplementing auth retry/backoff + circuit breaker + OTel spans. - Refactor
IsAuthenticationErrorinto a sharedauthErrorPatternslist (including mcp-go"unauthorized (401)"). - Add test helpers and new unit/integration/E2E tests for auth-retry and circuit breaker behavior.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/vmcp/client/auth_retry.go | Implements auth-retry decorator w/ backoff, circuit breaker, singleflight, and OTel spans |
| pkg/vmcp/client/client.go | Wraps the existing HTTP backend client with the retry decorator |
| pkg/vmcp/errors.go | Refactors authentication error detection into pattern list and expands coverage |
| pkg/vmcp/client/auth_retry_test.go | Unit tests for retry/circuit breaker behavior |
| pkg/vmcp/client/auth_retry_integration_test.go | Integration tests using a real mcp-go HTTP server with transient 401 responses |
| pkg/vmcp/client/client_test.go | Updates tests to account for the new retry decorator return type |
| test/integration/vmcp/helpers/backend.go | Adds WithHTTPMiddleware to inject transient HTTP failures in integration tests |
| test/e2e/thv-operator/virtualmcp/helpers.go | Extends E2E backend deployment helper (Args + PodTemplateSpec) and adds health status const |
| test/e2e/thv-operator/virtualmcp/virtualmcp_auth_retry_test.go | New E2E test validating retry exhaustion behavior against a persistent-401 backend |
| test/e2e/thv-operator/virtualmcp/virtualmcp_circuit_breaker_test.go | Minor refactor to use shared health-status constant |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ab7f12704
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
github-actions
bot
added
size/XL
github-actions
bot
dismissed
their stale review
Large PR justification has been provided. Thank you!
|
✅ Large PR justification has been provided. The size review has been dismissed and this PR can now proceed with normal review. |
There was a problem hiding this comment.
Pull request overview
Adds authentication-failure retry behavior (with backoff + circuit breaker + tracing) to the vMCP BackendClient, expands auth-error detection patterns, and introduces new test hooks/coverage to validate the behavior end-to-end.
Changes:
- Wrap
NewHTTPBackendClientwith aretryingBackendClientdecorator that retriesErrAuthenticationFailedwith exponential backoff and a per-backend circuit breaker; instruments retries with OTel spans. - Refactor
IsAuthenticationErrorto use a pattern list, including the mcp-go"unauthorized (401)"error format. - Extend integration/E2E test helpers and add new unit/integration/E2E tests for auth-retry and circuit-breaker behavior.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
pkg/vmcp/client/auth_retry.go |
Implements auth retry loop with backoff, circuit breaker, singleflight coalescing, and OTel spans. |
pkg/vmcp/client/client.go |
Wraps the HTTP backend client with the retry decorator; routes errors through wrapBackendError. |
pkg/vmcp/errors.go |
Refactors auth error string matching into authErrorPatterns and adds additional patterns. |
pkg/vmcp/client/auth_retry_test.go |
Unit tests for retry behavior, breaker open/reset, concurrency coalescing, and cancellation. |
pkg/vmcp/client/auth_retry_integration_test.go |
Integration test validating transient-401 retry against a real mcp-go streamable HTTP server. |
pkg/vmcp/client/client_test.go |
Updates tests to account for the new retry-wrapper returned by NewHTTPBackendClient. |
test/integration/vmcp/helpers/backend.go |
Adds WithHTTPMiddleware for injecting transient HTTP behavior in integration tests. |
test/e2e/thv-operator/virtualmcp/helpers.go |
Extends backend config to support args/pod template patches and skipping readiness wait; adds a shared health-status constant. |
test/e2e/thv-operator/virtualmcp/virtualmcp_auth_retry_test.go |
New E2E coverage validating persistent 401 leads to “unavailable” backend status. |
test/e2e/thv-operator/virtualmcp/virtualmcp_circuit_breaker_test.go |
Replaces hard-coded "healthy" with shared constant. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
github-actions
bot
added
size/XL
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4169 +/- ##
==========================================
- Coverage 69.30% 69.20% -0.10%
==========================================
Files 466 467 +1
Lines 46774 46920 +146
==========================================
+ Hits 32416 32471 +55
+ Misses 11871 11861 -10
- Partials 2487 2588 +101 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
github-actions
bot
added
size/XL
There was a problem hiding this comment.
Pull request overview
This PR adds an authentication-failure retry decorator (with exponential backoff, per-backend circuit breaker, singleflight coalescing, and OTel spans) around the vMCP BackendClient, and refactors error classification / test helpers to support the new behavior.
Changes:
- Wrap
NewHTTPBackendClientwith aretryingBackendClientand introducehttpStatusRoundTripperto map HTTP 401/403 and 5xx into vmcp sentinel errors forerrors.Ischecks. - Refactor
IsAuthenticationError/IsConnectionErrorto use package-level pattern slices and expand auth matching. - Add/adjust integration + E2E test helpers/tests to inject transient HTTP failures and validate auth-retry / circuit-breaker behavior.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
pkg/vmcp/client/client.go |
Wraps the backend client with retry logic; adds HTTP status → sentinel error mapping; routes more call errors through wrapBackendError. |
pkg/vmcp/client/auth_retry.go |
Implements auth-retry with exponential backoff, per-backend circuit breaker, singleflight coalescing, and OTel span instrumentation. |
pkg/vmcp/errors.go |
Refactors string-based auth/connection error detection to pattern slices and expands auth patterns. |
pkg/vmcp/client/client_test.go |
Updates tests to account for NewHTTPBackendClient now returning a retrying decorator. |
pkg/vmcp/client/auth_retry_test.go |
Adds unit tests covering retry success/exhaustion, breaker open/reset, concurrency coalescing, and context cancellation. |
pkg/vmcp/client/auth_retry_integration_test.go |
Adds integration test exercising retry behavior against an httptest-backed MCP server returning a transient 401. |
test/integration/vmcp/helpers/backend.go |
Adds WithHTTPMiddleware to wrap backend HTTP handlers for injecting transient HTTP failures in integration tests. |
test/e2e/thv-operator/virtualmcp/helpers.go |
Adds helpers/fields to support new E2E scenarios (pod-only readiness wait, backend args/pod template patching, skip readiness gate, health status constant). |
test/e2e/thv-operator/virtualmcp/virtualmcp_circuit_breaker_test.go |
Replaces literal "healthy" comparisons with shared constant. |
test/e2e/thv-operator/virtualmcp/virtualmcp_auth_retry_test.go |
Adds E2E test validating persistent-401 backend transitions to unavailable/degraded while stable backend remains healthy. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
`NewHTTPBackendClient` now wraps the raw HTTP client in a `retryingBackendClient` decorator that: - Intercepts `ErrAuthenticationFailed` (401/403) returned by any `BackendClient` method and retries up to `maxAuthRetries` (3) times with exponential backoff (100 ms base, doubling each attempt). - Maintains a per-backend `authCircuitBreaker` that opens after `authCircuitBreakerThreshold` (5) consecutive fully-exhausted retry sequences, preventing runaway latency from permanently broken credentials. - Uses `singleflight` to coalesce concurrent backoff waits for the same backend, so N goroutines racing on a 401 sleep only once per attempt. - Wraps each retry sequence in an OpenTelemetry `auth.retry` span so the overhead is visible in distributed traces. - Never logs raw credentials. `IsAuthenticationError` is refactored from chained `if` blocks into a package-level `authErrorPatterns` slice (fixes gocyclo limit and adds the mcp-go `"unauthorized (401)"` format that was previously missed). The integration test helper gains `WithHTTPMiddleware` so tests can inject transient HTTP errors without modifying the MCP server logic. New tests: - 9 unit tests (`auth_retry_test.go`) covering success, single-retry, max-retries, non-auth passthrough, circuit breaker open/reset, and context cancellation. - 1 integration test (`auth_retry_integration_test.go`) against a real mcp-go streamable-HTTP server. - 1 E2E Ginkgo suite (`virtualmcp_auth_retry_test.go`) deploying a Python 401 backend alongside a healthy yardstick backend and asserting that the failing backend is marked `BackendStatusUnavailable` while the stable backend remains ready. Closes: #3869
github-actions
bot
added
size/XL
There was a problem hiding this comment.
Pull request overview
Adds an auth-failure retry decorator around the vMCP BackendClient, converting HTTP status codes into sentinel errors to enable errors.Is()-based detection and supporting end-to-end verification of auth-retry exhaustion behavior.
Changes:
- Wrap
httpBackendClientwith a newretryingBackendClientthat retriesErrAuthenticationFailedwith exponential backoff and a per-backend circuit breaker; coalesces concurrent backoff sleeps viasingleflightand instruments retries with anauth.retryspan. - Introduce an HTTP
RoundTripperthat turns 401/403 and selected 5xx responses into structured sentinel errors and updates backend error wrapping to prefer these sentinels. - Expand integration/E2E test helpers and add unit + integration + E2E tests to exercise transient/persistent 401 behavior and health reporting.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
pkg/vmcp/client/client.go |
Wraps the HTTP backend client with retrying decorator; adds httpStatusRoundTripper; updates error wrapping call sites to use shared wrapBackendError. |
pkg/vmcp/client/auth_retry.go |
New retry decorator implementing auth retry/backoff, circuit breaker, singleflight coalescing, and OTel span instrumentation. |
pkg/vmcp/errors.go |
Refactors auth/connection string matchers into pattern slices and expands auth patterns (incl. unauthorized (401)). |
pkg/vmcp/client/client_test.go |
Updates tests to account for NewHTTPBackendClient now returning the retrying decorator. |
pkg/vmcp/client/auth_retry_test.go |
New unit tests for retry success, max-retry exhaustion, circuit breaker behavior, and concurrency coalescing. |
pkg/vmcp/client/auth_retry_integration_test.go |
New integration test validating transient 401 retry succeeds end-to-end with a real mcp-go server. |
test/integration/vmcp/helpers/backend.go |
Adds WithHTTPMiddleware to allow injecting transient HTTP failures in integration tests. |
test/e2e/thv-operator/virtualmcp/helpers.go |
Adds pod-only readiness wait helper; extends backend config to support args, pod template patching, and skipping readiness waits; adds backendHealthStatusHealthy constant. |
test/e2e/thv-operator/virtualmcp/virtualmcp_circuit_breaker_test.go |
Uses the new backendHealthStatusHealthy constant instead of hardcoded "healthy". |
test/e2e/thv-operator/virtualmcp/virtualmcp_auth_retry_test.go |
New E2E test deploying a persistent-401 backend and asserting it becomes unavailable while a stable backend remains healthy. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func NewHTTPBackendClient(registry vmcpauth.OutgoingAuthRegistry) (vmcp.BackendClient, error) { | ||
| if registry == nil { | ||
| return nil, fmt.Errorf("registry cannot be nil; use UnauthenticatedStrategy for no authentication") | ||
| } | ||
|
|
||
| c := &httpBackendClient{ | ||
| registry: registry, | ||
| } | ||
| c.clientFactory = c.defaultClientFactory | ||
| return c, nil | ||
| return newRetryingBackendClient(c, registry), nil | ||
| } |
| type retryingBackendClient struct { | ||
| inner vmcp.BackendClient | ||
| registry vmcpauth.OutgoingAuthRegistry | ||
|
|
jerm-dro
left a comment
jerm-dro
left a comment
There was a problem hiding this comment.
FYI: I think this potentially overlaps with the embedded auth in vMCP work: https://github.com/stacklok/stacklok-epics/issues/251
As part of that work, we want to solve the token refresh problem. It may make sense for those changes to land first.
| // breakers maps backendID -> *authCircuitBreaker. LoadOrStore is used for concurrent safety. | ||
| breakers sync.Map |
There was a problem hiding this comment.
blocker: can we implement this without a sync.Map?
The multi-session already has the notion of separate clients per backend. We don't actually have to share state, right? If so, then we should have one retrier per backend.
Summary
NewHTTPBackendClientnow wraps the raw HTTP client in aretryingBackendClientdecorator that:ErrAuthenticationFailed(401/403) returned by anyBackendClientmethod and retries up tomaxAuthRetries(3) times with exponential backoff (100 ms base, doubling each attempt).authCircuitBreakerthat opens afterauthCircuitBreakerThreshold(5) consecutive fully-exhausted retry sequences, preventing runaway latency from permanently broken credentials.singleflightto coalesce concurrent backoff waits for the same backend, so N goroutines racing on a 401 sleep only once per attempt.auth.retryspan so the overhead is visible in distributed traces.IsAuthenticationErroris refactored from chainedifblocks into a package-levelauthErrorPatternsslice (fixes gocyclo limit and adds the mcp-go"unauthorized (401)"format that was previously missed).The integration test helper gains
WithHTTPMiddlewareso tests can inject transient HTTP errors without modifying the MCP server logic.Fixes #3869
Type of change
Test plan
task test)task test-e2e)task lint-fix)Changes
Does this introduce a user-facing change?
Special notes for reviewers
Large PR Justification
This is a complete and isolated functionality, with comprehensive tests. Cannot be split.