Skip to content

Fix: avoid uncessary retries in OAuth authenticated requests - #1206

Merged
ochafik merged 6 commits into
modelcontextprotocol:mainfrom
keurcien:main
Aug 22, 2025
Merged

Fix: avoid uncessary retries in OAuth authenticated requests#1206
ochafik merged 6 commits into
modelcontextprotocol:mainfrom
keurcien:main

Conversation

@keurcien

Copy link
Copy Markdown
Contributor

Motivation and Context

Tried to connect an MCP client to Notion's MCP client (more info here: #1204)

How Has This Been Tested?

Using a local script wrapping this code snippet:

async with streamablehttp_client("https://mcp.notion.com/mcp", auth=oauth_auth) as (read, write, _):

    async with ClientSession(read, write) as session:
        await session.initialize()
        tools = await session.list_tools()
        print(f"Available tools: {[tool.name for tool in tools.tools]}")
        resources = await session.list_resources()
        print(f"Available resources: {[r.uri for r in resources.resources]}")

        tool_result = await session.call_tool(name="search", arguments={"query": "John Doe"})

Now list_tools(), list_resources() and call_tool() run instantly.

Breaking Changes

No

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

@keurcien
keurcien requested review from a team and ochafik July 27, 2025 15:32
ochafik
ochafik previously requested changes Aug 5, 2025

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

Hi @keurcien, thanks for this PR!

I wonder if you could add a test case, maybe in tests/client/test_auth.py, based on your snippet? (if it's not too involved)

@keurcien

ghost commented Aug 10, 2025

Copy link
Copy Markdown
Contributor Author

Sure, will do this week if I find some time

@felixweinberger
felixweinberger dismissed ochafik’s stale review August 21, 2025 15:30

Added regression test

@felixweinberger
felixweinberger requested review from ochafik and removed request for ochafik August 21, 2025 15:30
keurcien and others added 5 commits August 21, 2025 16:41
This test validates the fix for PR #1206 which resolved a critical
performance issue where every request was being sent twice due to
incorrect indentation of retry logic.

The test:
- Simulates a successful authenticated request (200 response)
- Counts how many times the request is yielded
- FAILS with buggy version: 2 yields (double sending)
- PASSES with fix: 1 yield (correct behavior)

This directly tests the root cause that made OAuth requests 2x slower
and ensures this regression cannot occur again.
Add missing type hints to test_auth_flow_no_unnecessary_retry_after_oauth
to satisfy pyright type checking requirements.
The retry logic fix in PR-1206 moved the final request yield inside the OAuth
flow block, which now correctly retries requests after OAuth completion.
However, the existing tests didn't expect this final yield and would exit
the generator early, causing a GeneratorExit exception while the async lock
was still held. This resulted in RuntimeError: The current task is not
holding this lock.

Fix by properly closing the generators in both failing tests:
- test_oauth_discovery_fallback_conditions
- test_auth_flow_with_no_tokens

Both tests now send a final success response to properly complete the
auth flow generator and prevent the lock release error.
Comment thread tests/client/test_auth.py
await auth_flow.asend(final_response)
except StopAsyncIteration:
pass # Expected - generator should complete

ghost Aug 21, 2025

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.

These tests were inadvertently tuned to the buggy behavior where requests were being unconditionally retried after every response. The indentation fix ensures retries only happen after OAuth completion (401 responses), not after successful responses. This exposed that the tests weren't properly closing the auth flow generator after the legitimate OAuth retry, causing lock release errors.

@felixweinberger

ghost commented Aug 21, 2025

Copy link
Copy Markdown
Contributor

@keurcien @ochafik went ahead and added some test coverage here + fixed failing CI which was tuned to the original behavior.

ghost 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 @felixweinberger for the test updates!

@ochafik
ochafik merged commit e750a06 into modelcontextprotocol:main Aug 22, 2025
@eyalzh

ghost commented Aug 24, 2025

Copy link
Copy Markdown

This fix also resolves a timeout issue when using stored access tokens in SSE auth flow (happened as soon as I replaced InMemoryTokenStorage with a keychain-based token storage).

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants