fix: Close inner SDK auth generator to prevent OAuth reconnect deadlock

[igorhvr]

This fixes #38193

Add a finally: await inner.aclose() block in
HermesMCPOAuthProvider.async_auth_flow to deterministically close the inner MCP SDK auth-flow generator when the Hermes wrapper is closed.

Without this, keepalive transport failures on OAuth-backed MCP servers (e.g. Databricks) orphan the inner SDK generator suspended inside async with self.context.lock. When Python's async-generator finalizer later runs from a different task, anyio's task-ownership check on lock release raises RuntimeError("The current task is not holding this lock") and leaves the lock permanently held. Cached provider reuse then makes reconnect hang until the gateway process is restarted.

• Add regression test proving the lock stays held after aclose() without the fix
• Add finally block to close the inner SDK generator and release the anyio.Lock
• Keep existing bidirectional .asend() forwarding logic unchanged
• All 73 existing MCP OAuth tests continue to pass

What does this PR do?

Related Issue

Fixes #

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

How to Test

Checklist

Code

• I've read the Contributing Guide
• My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
• I searched for existing PRs to make sure this isn't a duplicate
• My PR contains only changes related to this fix/feature (no unrelated commits)
• I've run pytest tests/ -q and all tests pass
• I've added tests for my changes (required for bug fixes, strongly encouraged for features)
• I've tested on my platform:

Documentation & Housekeeping

• I've updated relevant documentation (README, docs/, docstrings) — or N/A
• I've updated cli-config.yaml.example if I added/changed config keys — or N/A
• I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
• I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
• I've updated tool descriptions/schemas if I changed tool behavior — or N/A

For New Skills

• This skill is broadly useful to most users (if bundled) — see Contributing Guide
• SKILL.md follows the standard format (frontmatter, trigger conditions, steps, pitfalls)
• No external dependencies that aren't already available (prefer stdlib, curl, existing Hermes tools)
• I've tested the skill end-to-end: hermes --toolsets skills -q "Use the X skill to do Y"

Screenshots / Logs

[liuhao1024]

Found one minor issue worth fixing.

tests/tools/test_mcp_oauth_generator_cleanup.py:166 — the exception class name is misspelled as AssertionError (missing the second s). If this callback path is ever exercised by a future test variant, it would raise NameError: name 'AssertionError' is not defined instead of the intended AssertionError.

# Current (line 166):
raise AssertionError(

# Should be:
raise AssertionError(

The core fix in tools/mcp_oauth_manager.py and the lock-release regression test both look correct — closing the inner SDK auth-flow generator in finally is the right approach to prevent the anyio.Lock deadlock on reconnect.

[igorhvr]

Found one minor issue worth fixing.

tests/tools/test_mcp_oauth_generator_cleanup.py:166 — the exception class name is misspelled as AssertionError (missing the second s). If this callback path is ever exercised by a future test variant, it would raise NameError: name 'AssertionError' is not defined instead of the intended AssertionError.

# Current (line 166):
raise AssertionError(

# Should be:
raise AssertionError(

The core fix in tools/mcp_oauth_manager.py and the lock-release regression test both look correct — closing the inner SDK auth-flow generator in finally is the right approach to prevent the anyio.Lock deadlock on reconnect.

Hi @liuhao1024 ! Thanks for reviewing. However, both lines look the same to me here... I also checked and could not find the version with the missing s in the patch (see below).

Can you please help me see what am I missing?

igorhvr:hermes-agent/ $ grep AsertionError /tmp/oauth-mcp-server-deadlock-after-keepalive-reconnect.patch [16:31:04]
igorhvr:hermes-agent/ $ grep AssertionError /tmp/oauth-mcp-server-deadlock-after-keepalive-reconnect.patch [16:31:05]

• raise AssertionError(
  igorhvr:hermes-agent/ $ [16:31:11]

[tonydwb]

Code Review Summary

Verdict: Approved

Changes

Closes an OAuth reconnect deadlock in HermesMCPOAuthProvider.async_auth_flow by adding finally: await inner.aclose() to the function body so the inner SDK auth generator is deterministically closed from the owning asyncio task — releasing the anyio.Lock before the wrapper returns.

Background

Without this fix:

1. HTTPX closes the Hermes wrapper generator when the auth flow ends
2. The inner SDK generator remains suspended inside async with self.context.lock
3. Python's async generator finalizer tries to release the lock from a different task
4. anyio raises RuntimeError("The current task is not holding this lock")
5. Lock is permanently held → all subsequent reconnects deadlock until process restart

Testing

Regression test test_mcp_oauth_generator_cleanup.py directly validates the deadlock scenario — it creates a real provider, drives the inner generator to its yield point inside the lock, closes the wrapper, and asserts the lock is released without cross-task errors.

Code Quality

• The fix is minimal and surgical (4 lines).
• The detailed inline comment documenting the deadlock mechanism is excellent for future maintainers.

---

Reviewed by Hermes Agent

[askjerry96]

I hit what looks like the same failure path today with the official Swiggy Instamart remote OAuth MCP server (https://mcp.swiggy.com/im) from a Hermes gateway session.

Observed before this fix in the gateway logs:

RuntimeError: The current task is not holding this lock
WARNING tools.mcp_tool: MCP server 'swiggy-instamart' initial connection failed ... unhandled errors in a TaskGroup
MCP server 'swiggy-instamart' is not connected

I tested this PR locally in an isolated worktree against the same configured OAuth MCP server and cached credentials. It fixes the Swiggy case without any server-specific workaround.

Validation run from this PR branch:

$ python -m pytest tests/tools/test_mcp_oauth_generator_cleanup.py -q -o 'addopts='
1 passed in 0.42s

$ python -m hermes_cli.main mcp test swiggy-instamart
✓ Connected (1317ms)
✓ Tools discovered: 10

I also ran a read-only native tool call via _connect_server(...).session.call_tool('get_cart'); it reached the server and did not return the prior not connected error:

NATIVE_TOOL_CALL_REACHED_SERVER
tool=get_cart
has_not_connected_error=False
elapsed_ms=1629

So this appears to cover a real OAuth remote-MCP gateway failure beyond the original Databricks report.

[NaMinhyeok]

Cross-link: #63495 is a transparent current-main salvage of this PR, not an independent reimplementation. It cherry-picks @igorhvr's fix commit with authorship and patch preserved, then adds one follow-up commit that replaces the GC/sleep-based test with a deterministic real-HTTPX timeout and provider-reuse regression in the existing OAuth bridge suite. The salvage PR body documents the relationship and validation in full; maintainers can choose the preferred integration path.

[alt-glitch]

This was generated by AI during triage.

Related: #63495 is a transparent current-main salvage of this PR (cherry-picks the fix commit with authorship preserved, swaps the GC/sleep test for a deterministic HTTPX-timeout regression) — not an independent reimplementation, so it's linked as related, not a duplicate. #38193 is the underlying issue; #49543 is the same cross-task-lock reconnect-wedge class. Confirmed independently by @askjerry96 against the Swiggy remote OAuth MCP server.

[teknium1]

Thanks for isolating the OAuth bridge teardown failure. The production premise still holds on current main: tools/mcp_oauth_manager.py:416-424 creates and drives the SDK generator, while the method exits through the normal StopAsyncIteration branch at lines 425-429 with no finally to close that inner generator. The proposed finally: await inner.aclose() therefore binds the inner flow to the outer HTTPX-owned flow without changing the bidirectional forwarding contract.

Problems

• tests/tools/test_mcp_oauth_generator_cleanup.py:121-125 depends on gc.collect() and event-loop timing rather than HTTPX's actual timeout/teardown path; it also does not prove provider reuse after teardown.
• The temporary exception handler is restored to None rather than the prior handler (tests/tools/test_mcp_oauth_generator_cleanup.py:111-147), and its assertion only reads context["message"], not context["exception"] (:133-139).

Suggested changes

• Preserve the fix commit, but use the deterministic HTTPX timeout plus second-request regression in related current-main salvage PR #63495 (tests/tools/test_mcp_oauth_bidirectional.py). It asserts both lock release and successful reuse.

This is an automated hermes-sweeper review.

[teknium1]

This relies on GC/finalizer scheduling rather than the HTTPX teardown that triggers the reported production path. Please replace it with a MockTransport timeout through httpx.AsyncClient, then assert the lock is released and a second request using the same provider succeeds; related #63495 has that deterministic shape.

[teknium1]

Save loop.get_exception_handler() before replacing it and restore that saved handler in finally; restoring None can discard a handler installed by the test runner or enclosing test infrastructure.

[teknium1]

Also inspect exc_info["exception"]: asyncio normally puts the actual RuntimeError there while message is often only Task exception was never retrieved, so the current check can miss the error it intends to detect.