Fix: avoid uncessary retries in OAuth authenticated requests - #1206
Conversation
left a comment
There was a problem hiding this comment.
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)
commented
Aug 10, 2025
|
Sure, will do this week if I find some time |
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.
| await auth_flow.asend(final_response) | ||
| except StopAsyncIteration: | ||
| pass # Expected - generator should complete | ||
|
|
There was a problem hiding this comment.
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.
commented
Aug 21, 2025
left a comment
There was a problem hiding this comment.
Thanks @felixweinberger for the test updates!
commented
Aug 24, 2025
|
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). |
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:
Now
list_tools(),list_resources()andcall_tool()run instantly.Breaking Changes
No
Types of changes
Checklist
Additional context