Skip to content

fix(mcp-oauth): close inner SDK auth generator on teardown (salvage #38198) - #63495

Open
NaMinhyeok wants to merge 2 commits into
NousResearch:mainfrom
NaMinhyeok:codex/fix-mcp-oauth-generator-close
Open

fix(mcp-oauth): close inner SDK auth generator on teardown (salvage #38198)#63495
NaMinhyeok wants to merge 2 commits into
NousResearch:mainfrom
NaMinhyeok:codex/fix-mcp-oauth-generator-close

Conversation

@NaMinhyeok

Copy link
Copy Markdown

What does this PR do?

This is a transparent current-main salvage of #38198 by @igorhvr. Igor's original fix commit was cherry-picked with authorship preserved, then rebased onto current main; the follow-up commit replaces the GC/sleep-based regression test with a deterministic HTTPX timeout test in the existing bidirectional-flow suite.

Hermes manually bridges the MCP SDK's bidirectional OAuth async generator. It forwards .asend(response), but previously did not close the delegated generator when HTTPX tore down the outer auth flow after a timeout or cancellation. The SDK generator could remain suspended while holding its AnyIO lock, then be finalized from another task and raise:

RuntimeError: The current task is not holding this lock

Closing the inner generator in finally releases the lock from the owning task and leaves the cached provider reusable.

Related Issue

Fixes #38193

Related: #31987, #49543
Salvages and supersedes the stale current-main integration of #38198.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • tools/mcp_oauth_manager.py: always close the delegated SDK auth-flow generator after normal completion, timeout, cancellation, or outer-generator closure.
  • tests/tools/test_mcp_oauth_bidirectional.py: exercise the real httpx.AsyncClient(auth=provider) teardown path with a forced ReadTimeout, assert immediate lock release, then reuse the same provider successfully.

How to Test

  1. Run the focused OAuth suite:

    scripts/run_tests.sh \
      tests/tools/test_mcp_oauth.py \
      tests/tools/test_mcp_oauth_bidirectional.py \
      tests/tools/test_mcp_oauth_cold_load_expiry.py \
      tests/tools/test_mcp_oauth_integration.py \
      tests/tools/test_mcp_oauth_manager.py \
      tests/tools/test_mcp_oauth_metadata.py -q

    Result: 119 passed, 0 failed.

  2. Run static/cross-platform checks:

    .venv/bin/ruff check tools/mcp_oauth_manager.py tests/tools/test_mcp_oauth_bidirectional.py
    .venv/bin/python scripts/check-windows-footguns.py --diff origin/main

    Result: both pass.

  3. Regression proof: with the finally: await inner.aclose() change temporarily removed, the new timeout test fails synchronously because the SDK lock remains held and teardown emits the reported AnyIO ownership RuntimeError. With the fix restored, it passes and the second request through the same provider returns 200.

  4. Full-suite run after installing .[all,dev]:

    scripts/run_tests.sh

    Result: 40,419 passed, 38 failed. The 38 local-environment/platform failures are in 15 unrelated files outside this two-file diff (host Claude OAuth credentials, macOS /tmp/private/tmp resolution, Linux/systemd assumptions, platform SDK version drift, and timing-sensitive process tests). All MCP OAuth tests passed in the full run.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits
  • I searched existing PRs; this PR explicitly salvages fix: Close inner SDK auth generator to prevent OAuth reconnect deadlock #38198 and preserves @igorhvr's authorship
  • My PR contains only changes related to this fix
  • I've run pytest tests/ -q and all tests pass — full-suite local-environment failures are documented above
  • I've added tests for the bug
  • I've tested on macOS 26.5.1 arm64 with Python 3.12.13

Documentation & Housekeeping

  • Documentation update — N/A
  • cli-config.yaml.example update — N/A
  • CONTRIBUTING.md / AGENTS.md update — N/A
  • Cross-platform impact considered; Windows footgun scan passes
  • Tool descriptions/schemas update — N/A

@NaMinhyeok
NaMinhyeok marked this pull request as ready for review July 13, 2026 01:33
@alt-glitch alt-glitch added type/bug Something isn't working tool/mcp MCP client and OAuth comp/tools Tool registry, model_tools, toolsets P2 Medium — degraded but workaround exists labels Jul 13, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for preserving the original fix and replacing the timing-dependent regression with an HTTPX teardown test. Current main still creates the delegated SDK flow at tools/mcp_oauth_manager.py:416 and only handles normal StopAsyncIteration at :425-429; it has no cleanup path when HTTPX closes the outer generator. The added finally: await inner.aclose() directly binds the inner SDK flow to that outer lifecycle.

The new regression drives httpx.AsyncClient(auth=provider) through a forced timeout, verifies provider.context.lock is released, then verifies reuse of the same provider with a successful second request. This is stronger than the prior GC/timing-based coverage and matches the reported failure mode.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 16, 2026
@ctaylor86

Copy link
Copy Markdown
Contributor

Downstream validation result: this cleanup is useful, but it is not sufficient by itself for the live timeout reported in the related issues.

I applied this PR's deterministic inner.aclose() change and its regression test passed, but the live OAuth Streamable HTTP connection still timed out because the SDK's OAuthContext.lock remained held by the session-long GET while tools/list waited behind it.

With both this cleanup and the SDK lock-scope fix from modelcontextprotocol/python-sdk#3243 applied, the same connection completed in under a second and listed 171 tools. Two existing downstream MCP servers (24 and 7 tools) also remained healthy, and Hermes' 423 MCP tests passed.

So I believe this PR should still land for deterministic delegated-generator cleanup, while the functional deadlock itself also needs the SDK fix.

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-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.

5 participants