Skip to content

fix(mcp): stop retrying wrapped OAuth failures - #62025

Draft
luckymikey999-coder wants to merge 1 commit into
NousResearch:mainfrom
luckymikey999-coder:stark/t_8f457ba9
Draft

fix(mcp): stop retrying wrapped OAuth failures#62025
luckymikey999-coder wants to merge 1 commit into
NousResearch:mainfrom
luckymikey999-coder:stark/t_8f457ba9

Conversation

@luckymikey999-coder

Copy link
Copy Markdown

Summary

  • treat auth failures wrapped in nested BaseExceptionGroup / ExceptionGroup instances as terminal auth failures
  • stop the initial MCP connection retry loop from relaunching an interactive OAuth flow after the user skips it
  • add regression coverage for nested/mixed groups, HTTP 401 vs 500, one transport attempt, zero backoff sleeps, and no detached tasks

Root cause

The MCP SDK's async transport can wrap OAuthNonInteractiveError("user_skipped") in an ExceptionGroup. _is_auth_error() only inspected the top-level exception, so MCPServerTask.run() misclassified the wrapped auth abort as a transient connection failure.

That reran the whole transport four times (initial attempt plus three configured retries), producing repeated authorization URLs/browser prompts. After the retry budget was exhausted, the task parked for reconnect; cancellation during that parked wait reproduced the reported RuntimeError: Event loop is closed cleanup traceback.

This change recursively classifies a group as auth-related when any nested leaf is already auth-related. The original group is preserved in server._error; the change only prevents automatic restart of an interactive auth flow. Direct HTTP policy remains unchanged: 401 is auth-related, 500 is not.

Reference behavior

OpenCode represents authorization-required as an explicit terminal MCP state rather than a generic transport retry. Qwen Code similarly excludes auth failures from generic transient retry policy and owns a single callback-server lifecycle.

Tests

Relevant suites after rebasing onto current origin/main:

  • 10 passed, 3 deselected — focused wrapped-auth/classifier matrix with PytestUnraisableExceptionWarning promoted to error
  • 373 passed — MCP tool, OAuth, OAuth manager, and CLI MCP config modules
  • ruff check — passed for all touched files
  • git diff --check — passed

A full repository xdist run completed with 39656 passed, 226 skipped, 536 failed, 18 errors in 12m51s. The failures/errors are broad pre-existing environment/global-state failures (including web-provider, Hindsight, and website-policy suites); all touched and directly relevant MCP/OAuth/CLI modules are green.

No live OAuth provider, browser callback, credentials, config, gateway, or runtime rollout was used or changed.

@alt-glitch alt-glitch added type/bug Something isn't working comp/tools Tool registry, model_tools, toolsets tool/mcp MCP client and OAuth P2 Medium — degraded but workaround exists labels Jul 10, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: extends the concept of the closed #25624 (stop retrying initial MCP auth failures — top-level only) to the wrapped ExceptionGroup case the top-level check missed. Sibling of #62026 (startup-task retention) in the same file, different mechanism.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused regression fix. Current main still has the reported classifier gap: tools/mcp_tool.py:3183-3185 rejects an ExceptionGroup before inspecting its leaves, and MCPServerTask.run() therefore proceeds through the initial retry/backoff path at tools/mcp_tool.py:2761-2808. The recursive classifier change addresses that path while retaining the existing direct HTTP 401-only policy.

The tests cover nested and mixed groups, preserve the HTTP 401/500 distinction, and update the existing startup-retry integration test without mocking the classifier. This appropriately extends the top-level auth short-circuit introduced by 1247ff2d.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/tools Tool registry, model_tools, toolsets P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades tool/mcp MCP client and OAuth type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants