feat: add auth-failure retry with forced credential refresh for MCP tool calls - #5713
Conversation
oauth_configs to new mcp_oauth_flows table
#5709
|
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughMCP tool execution now detects authentication failures, force-refreshes credentials, retries safe per-call connections, and asynchronously reconnects shared connections. OAuth providers refresh shared or per-user tokens through new credential and client-manager contracts. ChangesMCP authentication recovery
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant MCPToolManager
participant MCPTransport
participant CredStore
participant ClientManager
MCPToolManager->>MCPTransport: Execute tools/call
MCPTransport-->>MCPToolManager: Authentication failure
MCPToolManager->>CredStore: ForceRefresh credentials
CredStore-->>MCPToolManager: Refresh result
MCPToolManager->>ClientManager: Acquire refreshed connection
ClientManager-->>MCPToolManager: New connection
MCPToolManager->>MCPTransport: Retry tools/call once
MCPTransport-->>MCPToolManager: Recovered tool result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
ce4a9b1 to
fe261ef
Compare
50d7649 to
6277f49
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/mcp/toolmanager.go`:
- Around line 844-866: Create the recovery timeout context before the
ForceRefresh call, using toolExecutionTimeout, and pass it to ForceRefresh,
AcquireClientConn, and the retry conn.CallTool invocation. Remove the later
retryCtx creation so all recovery operations share the same deadline, while
preserving cancellation and existing error handling.
- Around line 745-747: Update the retry response path in
attemptAuthFailureRecovery to pass retryResponse.IsError as the required third
argument to createToolResponseMessage, preserving the MCP tool-error state while
leaving the existing response text and return values unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3c447f93-2f1e-4c56-ab50-a550488d85a2
📒 Files selected for processing (20)
core/mcp/agent_test.gocore/mcp/auth_retry_test.gocore/mcp/codemode/starlark/starlark_test.gocore/mcp/credstore/credstore.gocore/mcp/credstore/none.gocore/mcp/credstore/per_user_headers.gocore/mcp/credstore/per_user_oauth.gocore/mcp/credstore/per_user_oauth_test.gocore/mcp/credstore/shared_headers.gocore/mcp/credstore/shared_oauth.gocore/mcp/credstore/utils.gocore/mcp/reauth_state_test.gocore/mcp/toolmanager.gocore/mcp/toolmanager_test.gocore/mcp/utils.gocore/schemas/context.gocore/schemas/mcp.gocore/schemas/oauth.goframework/oauth2/force_refresh_test.goframework/oauth2/main.go
💤 Files with no reviewable changes (1)
- core/mcp/credstore/utils.go
🚧 Files skipped from review as they are similar to previous changes (17)
- core/mcp/credstore/none.go
- core/mcp/utils.go
- core/mcp/credstore/shared_oauth.go
- core/mcp/credstore/per_user_oauth.go
- core/mcp/toolmanager_test.go
- core/mcp/credstore/per_user_oauth_test.go
- core/mcp/reauth_state_test.go
- framework/oauth2/main.go
- core/schemas/oauth.go
- core/mcp/credstore/shared_headers.go
- core/schemas/mcp.go
- core/mcp/credstore/per_user_headers.go
- core/mcp/codemode/starlark/starlark_test.go
- core/mcp/agent_test.go
- core/mcp/credstore/credstore.go
- core/schemas/context.go
- core/mcp/auth_retry_test.go
6277f49 to
e491115
Compare
fe261ef to
bd1ded9
Compare
Merge activity
|
The base branch was changed.
…losed on missing tool annotations, dedupe background reconnect
e491115 to
91ea6f6
Compare

Summary
When a live MCP tool call returns a 401/403 or an "unauthorized"/"forbidden" error despite Bifrost's own credential bookkeeping considering the credential still valid, the upstream server and Bifrost's local state have diverged. Previously this surfaced as an opaque tool call failure with no recovery attempt. This PR adds automatic auth-failure recovery: for per-user connections, it forces a credential refresh, re-acquires a fresh connection, and retries the call once synchronously; for shared persistent connections (where a synchronous reconnect could take minutes), it fails the current call immediately and triggers a background reconnect so the next call succeeds.
Changes
isAuthFailureErrorTexttocore/mcp/utils.goto detect raw upstream 401/403/unauthorized/forbidden error text frommcp-go's flattened error strings. Deliberately separate fromisTransientError(opposite polarity — same text class is a permanent signal there, a positive retry trigger here) and fromisOAuth2TokenExpiredErrorText(which matches Bifrost's own internal sentinel, not raw upstream rejections).attemptAuthFailureRecoveryandtriggerBackgroundReconnecttoToolsManagerintoolmanager.go. Per-user path: force refresh → re-acquire connection → retry once synchronously. Shared path: fail fast, fire background goroutine that force-refreshes then callsReconnectClient. Destructive, non-idempotent tools are opted out of auto-retry to avoid double side effects.ReconnectClient(id string) errorto theClientManagerinterface soToolsManagercan trigger a reconnect without a direct dependency onMCPManager.ForceRefresh(ctx, config)to theMCPCredentialStoreinterface and implemented it across all resolvers incore/mcp/credstore: no-op fornone, static headers, and per-user headers; delegates toOAuth2Provider.ForceRefreshAccessTokenfor shared and per-user OAuth.ForceRefreshAccessToken(ctx, config)to theOAuth2Providerinterface and implemented it inframework/oauth2/main.go. Branches onAuthType: shared OAuth resolves the token viaOauthConfigIDand callsRefreshAccessTokendirectly (bypassing theExpiresAtgate thatGetAccessTokenapplies); per-user OAuth derives(mode, identity)from context via the newctx.MCPIdentity(mode)helper and does the same. Inactive (needs_reauth) shared tokens short-circuit withErrOAuth2TokenExpiredrather than attempting a doomed live refresh.identityForMCPAuthModefromcore/mcp/credstore/utils.gointoBifrostContextas theMCPIdentity(mode)method, making it available to both the credstore resolvers and the OAuth2 provider without duplication. Deletedcredstore/utils.go.auth_retry_test.gowith end-to-end coverage ofExecuteToolagainst a real*client.Clientwired to a faketransport.Interface: per-user success on second attempt, per-user retry-also-fails, shared fail-fast with background goroutine verification, destructive-non-idempotent opt-out, destructive-but-idempotent still retries, and non-auth failure never triggers recovery.framework/oauth2/force_refresh_test.gocoveringForceRefreshAccessTokenfor shared OAuth (resolves and refreshes), missingOauthConfigID, inactive token short-circuit, per-user OAuth (resolves and refreshes), no identity in context, and unsupported auth type.MockClientManager,MockAutoClientManager,mockToolClientManager,testClientManager,expiredOAuthCredStore,genericFailureCredStore,fakeOAuth2Provider) to satisfy the expanded interfaces.Type of change
Affected areas
How to test
go test ./core/mcp/... ./core/schemas/... ./core/mcp/credstore/... ./framework/oauth2/...The new
auth_retry_test.gotests use a faketransport.Interfacewired to a real*client.Client(viaclient.WithSession()to skip the MCP initialize handshake), so they exercise the actualCallToolerror text path thatisAuthFailureErrorTextevaluates — not a mocked shortcut. The background-goroutine test (TestExecuteTool_AuthFailureRetry_Shared_FailsFastAndTriggersBackgroundReconnect) uses signal channels with a 2-second timeout to assert the goroutine actually runs without sleeping.Breaking changes
ClientManagergainsReconnectClient(id string) errorandMCPCredentialStoregainsForceRefresh(ctx *BifrostContext, config *MCPClientConfig) error. Any external implementations of either interface must add these methods.OAuth2ProvidergainsForceRefreshAccessToken(ctx *BifrostContext, config *MCPClientConfig) errorwith the same requirement.Security considerations
The auto-retry is gated behind
isAuthFailureErrorText, which only matches explicit HTTP 401/403 status codes or the literal strings "unauthorized"/"forbidden" in the error text. Destructive, non-idempotent tools (as declared viaMCPToolAnnotations) are explicitly excluded from auto-retry to prevent unintended double execution of side-effecting operations. The shared-connection path never retries synchronously — it only triggers a background reconnect — so there is no risk of amplifying requests against a rate-limited or actively-rejecting upstream.Checklist
docs/contributing/README.mdand followed the guidelines