Skip to content

fix: close stranded MCP OAuth auth flows - #76526

Open
armsteadj1 wants to merge 2 commits into
NousResearch:mainfrom
armsteadj1:fix/mcp-oauth-login-wedge
Open

fix: close stranded MCP OAuth auth flows#76526
armsteadj1 wants to merge 2 commits into
NousResearch:mainfrom
armsteadj1:fix/mcp-oauth-login-wedge

Conversation

@armsteadj1

@armsteadj1 armsteadj1 commented Aug 2, 2026

Copy link
Copy Markdown

Summary

Close the inner MCP SDK OAuth auth-flow generator when Hermes' wrapper is abandoned, cancelled, or timed out by HTTPX — and reclaim a stranded anyio.Lock when the wrapper itself is finalized from a different task.

This is the comprehensive fix for #38193 and the same class of failure reported in #49543 and #31987. It incorporates the strongest parts of both prior PRs (#38198 by @igorhvr, #63495 by @NaMinhyeok) while adding cross-task finalizer safety that neither covers.

Root cause

HermesMCPOAuthProvider.async_auth_flow bridges httpx's bidirectional auth-flow protocol onto the SDK's generator. The SDK acquires an anyio.Lock (self.context.lock) at the top of its flow and holds it across every yield. When httpx closes our wrapper on transport failure, cancellation, or timeout, the inner SDK generator was simply dropped — still suspended inside async with self.context.lock.

Two consequences, both confirmed independently by multiple reporters (Granola, Databricks, Swiggy Instamart, Honeycomb, Reportify, gbrain):

  1. Python's async-generator GC finalizer runs aclose() on the orphaned inner generator from a different task. anyio's task-bound Lock.release() raises RuntimeError("The current task is not holding this lock"), and _owner_task is never cleared.
  2. context.lock stays permanently held. Every subsequent auth flow for that server blocks forever — a successful OAuth callback ("Authorization Successful") never produces tokens, hermes mcp login wedges, and hermes mcp test reports no cached tokens.

What this PR does

Production fix (tools/mcp_oauth_manager.py):

  • finally: await inner.aclose() — closes the inner SDK generator from the owning task, so the lock's __aexit__ runs in the correct task and the release succeeds. (Same core mechanism as fix: Close inner SDK auth generator to prevent OAuth reconnect deadlock #38198/fix(mcp-oauth): close inner SDK auth generator on teardown (salvage #38198) #63495.)
  • _close_inner_auth_flow — wraps the above with exception handling for the cross-task case: when our own wrapper is GC-finalized off-task, the inner.aclose() itself hits the task-ownership RuntimeError. We swallow it (preventing the unretrieved-task-exception console noise) and fall through to the reclaim.
  • _reclaim_stranded_auth_lock — belt-and-braces: if inner.aclose() couldn't release the lock (cross-task finalization), reclaim it by identity-checking _owner_task against the recorded prior owner. Only steals the lock when it's owned by the exact dead flow task; a lock held by a genuinely concurrent flow is never touched. Hands the lock to any queued waiter via the normal release() path.

Regression tests — organized so reviewers immediately see each scenario:

Test File What it proves
test_httpx_timeout_closes_inner_flow_and_releases_lock test_mcp_oauth_bidirectional.py (a) Real HTTPX teardown path: httpx.AsyncClient(auth=provider) with MockTransport, forced ReadTimeout, lock released, provider reused with 200
test_aclose_mid_flow_releases_the_sdk_auth_lock test_mcp_oauth_generator_cleanup.py (b) Wrapper aclose() releases lock
test_flow_after_abandoned_login_does_not_wedge test_mcp_oauth_generator_cleanup.py (c) Provider reuse after abandoned login — the user-visible wedge
test_cross_task_cleanup_is_silent_and_still_frees_the_lock test_mcp_oauth_generator_cleanup.py (d) Cross-task finalizer is silent and lock is reclaimed
test_garbage_collected_flow_emits_no_unhandled_task_exception test_mcp_oauth_generator_cleanup.py (d') GC-finalized flow produces no event-loop exception noise
test_normal_completion_still_ends_with_stop_async_iteration test_mcp_oauth_generator_cleanup.py (e) Normal bidirectional contract unchanged
test_repeated_flows_reuse_the_lock test_mcp_oauth_generator_cleanup.py (e') Back-to-back flows work
test_reclaim_ignores_a_lock_held_by_a_different_flow test_mcp_oauth_generator_cleanup.py Reclaim safety: concurrent flow's lock is never stolen
test_reclaim_is_a_noop_without_a_recorded_owner test_mcp_oauth_generator_cleanup.py Reclaim safety: no-op on missing owner
test_reclaim_hands_the_lock_to_a_waiting_flow test_mcp_oauth_generator_cleanup.py Reclaim correctness: queued waiter is handed the lock

Related issues and PRs

How this compares to the other PRs

Capability #38198 #63495 This PR
finally: await inner.aclose()
Cross-task RuntimeError swallowed
_reclaim_stranded_auth_lock
Deterministic HTTPX teardown test
Provider-reuse-after-abandon test ✅ (implicit) ✅ (explicit)
Cross-task finalizer test
GC-finalized noise suppression test
Reclaim safety tests
Normal-contract preservation tests

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✅ Tests (adding or improving test coverage)

How to Test

  1. Run the focused OAuth suite:

    scripts/run_tests.sh \
      tests/tools/test_mcp_oauth_generator_cleanup.py \
      tests/tools/test_mcp_oauth_bidirectional.py \
      tests/tools/test_mcp_oauth_metadata.py -q
  2. Run the full MCP OAuth test surface:

    scripts/run_tests.sh tests/tools/test_mcp_oauth*.py tests/tools/test_mcp_tool_401_handling.py tests/tools/test_mcp_reconnect_signal.py -q
  3. Static checks:

    python3 -m py_compile tools/mcp_oauth_manager.py tests/tools/test_mcp_oauth_generator_cleanup.py tests/tools/test_mcp_oauth_bidirectional.py
    uv run ruff check tools/mcp_oauth_manager.py tests/tools/test_mcp_oauth_generator_cleanup.py tests/tools/test_mcp_oauth_bidirectional.py
    git diff --check

Checklist

Code

Documentation & Housekeeping

  • Documentation update — N/A
  • cli-config.yaml.example update — N/A
  • Cross-platform impact considered; no platform-specific code
  • Tool descriptions/schemas update — N/A

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/tools Tool registry, model_tools, toolsets tool/mcp MCP client and OAuth area/auth Authentication, OAuth, credential pools sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Aug 2, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: #63495/#38198 close the inner OAuth generator in its owning task. This patch additionally handles the cross-task finalizer case that can leave the SDK lock stranded.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing a real MCP OAuth teardown failure. Current main still constructs the delegated SDK generator at tools/mcp_oauth_manager.py:419 without closing it when the wrapper is abandoned; the added finally at PR tools/mcp_oauth_manager.py:492-495 directly covers that gap.

Problems

  • tests/tools/test_mcp_oauth_generator_cleanup.py:271-276 relies on GC and fixed sleeps to observe finalization. That is timing-dependent rather than a deterministic assertion of the HTTPX teardown path.

Suggested changes

  • Replace the GC/sleep case with an HTTPX forced-timeout plus provider-reuse regression. Related PR #63495 contains that deterministic shape while preserving the same core inner.aclose() fix.
  • Keep the direct AnyIO private-owner recovery at tools/mcp_oauth_manager.py:100-104 tightly scoped and behavior-tested because it relies on _owner_task.

Automated hermes-sweeper review.

previous_handler = loop.get_exception_handler()
loop.set_exception_handler(lambda _loop, context: handled.append(context))
try:
del flow

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This GC/fixed-sleep finalizer check is timing-dependent. Please replace it with a deterministic HTTPX timeout teardown and provider-reuse test, as in related PR #63495, so the regression does not depend on scheduler timing.

@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 Aug 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/auth Authentication, OAuth, credential pools 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 sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state 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.

[Bug]: OAuth-backed MCP server permanently deadlocks after keepalive reconnect: auth-flow generator's lock is released cross-task.

3 participants