Skip to content

fix: avoid OAuth auth lock on streamable MCP connections - #51149

Closed
THArrowofApollo wants to merge 1 commit into
NousResearch:mainfrom
THArrowofApollo:fix/mcp-oauth-streamable-http-bearer-data-plane
Closed

THArrowofApollo wants to merge 1 commit into
NousResearch:mainfrom
THArrowofApollo:fix/mcp-oauth-streamable-http-bearer-data-plane

Conversation

@THArrowofApollo

Copy link
Copy Markdown

Summary

  • Materialize OAuth access tokens into bearer headers before opening Streamable HTTP MCP data-plane clients
  • Avoid passing the SDK OAuth auth provider into long-lived SSE/Streamable HTTP connections where its auth-flow lock can be held by the GET stream
  • Add regression tests for bearer-header materialization, refresh handling, and Streamable HTTP client wiring

Why

Eden's Streamable HTTP MCP transport opens a long-lived GET/SSE stream after initialize. The Python MCP SDK OAuth provider can hold its auth-flow lock while that response remains open, which blocks subsequent POST requests such as tools/list. Hermes then hangs during tool discovery despite having valid OAuth credentials.

Test Plan

  • uv run --extra dev python -m pytest tests/tools/test_mcp_oauth_manager.py tests/tools/test_mcp_tool.py -q -o 'addopts='
  • uv run --extra dev ruff check tools/mcp_oauth_manager.py tools/mcp_tool.py tests/tools/test_mcp_oauth_manager.py tests/tools/test_mcp_tool.py
  • HERMES_HOME=/Users/haren/.hermes/profiles/content uv run --extra dev hermes mcp test eden

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

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Allows OAuth Streamable-HTTP data-plane to use a plain bearer header with cached tokens, avoiding the OAuthClientProvider auth lock that was blocking subsequent tools/list POST calls. Adds two regression tests for bearer header materialization and token refresh. Good fix with test coverage.


Reviewed by Hermes Agent

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the focused OAuth transport fix. The lock premise remains present on current main: tools/mcp_tool.py:2361-2367 obtains the SDK provider and tools/mcp_tool.py:2490-2501 passes it to the shared Streamable HTTP client. In MCP 1.26.0, mcp/client/auth/oauth2.py:484-503 holds the OAuth context lock while yielding the request, and mcp/client/streamable_http.py:266-275 can keep the GET/SSE request open.

Problems

  • The new regression test covers only _MCP_NEW_HTTP=True. The changed _oauth_auth = None behavior also reaches the SSE path (tools/mcp_tool.py:2383-2409) and legacy Streamable HTTP path (tools/mcp_tool.py:2523-2532), without assertions for their header/auth wiring.

Suggested changes

  • During salvage onto current main, cover the legacy Streamable HTTP branch and assert it receives the bearer header without the SDK auth provider; cover SSE too if that change is intentional.

This is an automated hermes-sweeper review.

Comment thread tools/mcp_tool.py
_bearer_header = await _oauth_manager.get_bearer_authorization_header(
self.name, url, config.get("oauth"),
)
if _bearer_header:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This clears _oauth_auth for the later SSE and legacy Streamable HTTP branches as well as the new client path. Please add coverage for at least the legacy branch to verify it receives the bearer header and no SDK auth provider; add an SSE assertion too if changing its auth behavior is intentional.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data 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 area/streaming Streaming responses: gateway delivery, provider wire labels Jul 15, 2026
@andrexibiza

Copy link
Copy Markdown

Interlock binding — this PR addresses the mcp_tool.py extraction window of EPIC #78647 (shard #78642). Rebase-on-behalf delivery: andrexibiza:rebase-on-behalf/51149.

@andrexibiza

Copy link
Copy Markdown

Rebase-on-behalf delivery — this PR's branch was rebased onto current main (9d6c5a920c7).

Rebased branch: andrexibiza:rebase-on-behalf/51149 (commit c967660b22f, original authorship preserved)

Conflict resolution (2 files):

  • tools/mcp_oauth_manager.py (2 regions) — merged this PR's get_bearer_authorization_header (avoids the OAuth auth-lock on streamable HTTP GET/SSE) into main's evolved manager: imports unioned (Awaitable, Callable, cast added), main's _key static method kept, the new async method appended.
  • tests/tools/test_mcp_oauth_manager.py — this PR's 8 new tests added (bearer-header materialization/refresh, manager caching/singleton/eviction). Verified: 0 main tests lost. One extraction artifact fixed during merge (async async double-prefix on the refresh test).

To adopt (author):

git fetch https://github.com/andrexibiza/hermes-agent.git rebase-on-behalf/51149
git checkout <your-branch-name>
git reset --hard FETCH_HEAD
git push --force-with-lease origin <your-branch-name>

py_compile verified; test-coverage superset verified. This unblocks the mcp_tool extraction window (hunk gate).

@teknium1

Copy link
Copy Markdown
Collaborator

Closing with credit — you diagnosed this lock-held-across-the-streamable-GET class first (June 23). The fix that landed (PR #97458 by @helix4u, merge 9a1eef7) takes a different mechanism: it keeps the OAuth provider on the data plane but converts the SDK's task-owned lock to a binary semaphore released around resource I/O, so refresh still works mid-session. Your bearer-materialization approach would have bypassed refresh on the data plane. Thanks for the early, correct diagnosis.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/streaming Streaming responses: gateway delivery, provider wire 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-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data 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.

5 participants