Skip to content

Release context.lock before the protected request is sent - #3252

Open
guptaishaan wants to merge 1 commit into
modelcontextprotocol:mainfrom
guptaishaan:fix-3209
Open

Release context.lock before the protected request is sent#3252
guptaishaan wants to merge 1 commit into
modelcontextprotocol:mainfrom
guptaishaan:fix-3209

Conversation

@guptaishaan

Copy link
Copy Markdown

Fixes #3209

async_auth_flow held context.lock across response = yield request, so the lock covered the
whole round trip of the protected request rather than just token acquisition. The standalone GET
SSE stream runs through the same provider, so it pinned the lock for the lifetime of the stream
and the next request, typically the first tools/call, blocked in lock.acquire() until the
stream ended.

The lock now closes before the request is yielded and re-opens around the 401 and 403
re-authorization blocks. Token acquisition and refresh are still serialized; no protected request
is sent while the lock is held.

New test: tests/client/test_auth.py::test_in_flight_request_does_not_block_a_concurrent_request.
It drives two auth flows from two anyio tasks, holds the GET flow at its yield, and requires the
POST flow to reach its own yield inside anyio.fail_after(5). Without the patch it fails with
TimeoutError parked on async with self.context.lock; with it, it passes in 0.26s.

Verified on Linux, CPU only, Python 3.13.14:

  • pytest tests/client/test_auth.py tests/client/auth tests/client/test_streamable_http.py -> 197 passed, 1 xfailed
  • ./scripts/test -> 100.00% branch coverage, strict-no-cover clean
  • ruff format --check ., ruff check ., pyright all clean

Not verified: the reporter's ~15s figure against a live server. I reproduced the mechanism, not
the deployment. The multi-second stall needs a server or proxy that does not flush the GET SSE
response headers promptly, which is the Cloud Run behaviour in the issue; against a transport that
returns headers immediately, httpx2 resumes the generator right away and the stall does not show
up. Only Python 3.13 on Linux, asyncio backend, was exercised locally.

One knowing behaviour change: two requests in flight at once can now both get a 401 and both run a
full authorization. Before, the second could not be sent until the first finished, which hid this.
#2858 handles that with a "did the token change while I was in flight?" check. I left it out to
keep this change to the reported bug, so it is worth a follow-up.

Thanks to @stayclosetothequestion for the report, the measurements, and for tracing it to the
exact line.

async_auth_flow held context.lock across `response = yield request`, so the
lock covered the whole round trip of the protected request instead of just
token acquisition. The standalone GET SSE stream goes through the same
provider, so it pinned the lock for the lifetime of the stream and the next
request, usually the first tools/call, blocked in lock.acquire() until that
stream ended.

Close the lock before the request is yielded and re-open it around the 401
and 403 re-authorization blocks. Refresh and re-authorization stay
serialized; no protected request is sent under the lock.

The new test drives two auth flows from two anyio tasks, holds the GET flow
at its yield, and requires the POST flow to reach its own yield inside
anyio.fail_after(5).

@cubic-dev-ai cubic-dev-ai Bot 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.

2 issues found across 2 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="src/mcp/client/auth/oauth2.py">

<violation number="1" location="src/mcp/client/auth/oauth2.py:604">
P2: Concurrent requests can make a 401/403 re-authorization use another request’s `MCP-Protocol-Version`, which can flip `resource` parameter inclusion and build incorrect OAuth requests. This comes from dropping the lock before `yield request` while keeping `protocol_version` as shared mutable context; a per-request version should be carried through re-auth.</violation>
</file>

<file name="tests/client/test_auth.py">

<violation number="1" location="tests/client/test_auth.py:3281">
P3: This new concurrency test can hang indefinitely because it waits on `call_done` without a timeout. If the sibling task fails before setting the event, this task stays blocked and can make failures much harder to diagnose. Wrapping the wait in `anyio.fail_after(5)` keeps the test deterministic and fail-fast.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

# Released before the request goes out: the lock serialises token acquisition, and
# holding it for the lifetime of the response would stall every other request on
# this provider until the response ends - unbounded for the standalone GET SSE stream.
response = yield request

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2: Concurrent requests can make a 401/403 re-authorization use another request’s MCP-Protocol-Version, which can flip resource parameter inclusion and build incorrect OAuth requests. This comes from dropping the lock before yield request while keeping protocol_version as shared mutable context; a per-request version should be carried through re-auth.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/mcp/client/auth/oauth2.py, line 604:

<comment>Concurrent requests can make a 401/403 re-authorization use another request’s `MCP-Protocol-Version`, which can flip `resource` parameter inclusion and build incorrect OAuth requests. This comes from dropping the lock before `yield request` while keeping `protocol_version` as shared mutable context; a per-request version should be carried through re-auth.</comment>

<file context>
@@ -598,9 +598,13 @@ async def async_auth_flow(self, request: httpx2.Request) -> AsyncGenerator[httpx
+        # Released before the request goes out: the lock serialises token acquisition, and
+        # holding it for the lifetime of the response would stall every other request on
+        # this provider until the response ends - unbounded for the standalone GET SSE stream.
+        response = yield request
 
-            if response.status_code == 401:
</file context>

Comment thread tests/client/test_auth.py
request = await flow.__anext__()
sse_sent.set()
# The server holds the stream open, so the response lands after the call is answered.
await call_done.wait()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P3: This new concurrency test can hang indefinitely because it waits on call_done without a timeout. If the sibling task fails before setting the event, this task stays blocked and can make failures much harder to diagnose. Wrapping the wait in anyio.fail_after(5) keeps the test deterministic and fail-fast.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/client/test_auth.py, line 3281:

<comment>This new concurrency test can hang indefinitely because it waits on `call_done` without a timeout. If the sibling task fails before setting the event, this task stays blocked and can make failures much harder to diagnose. Wrapping the wait in `anyio.fail_after(5)` keeps the test deterministic and fail-fast.</comment>

<file context>
@@ -3253,3 +3254,44 @@ async def echo_callback() -> AuthorizationCodeResult:
+        request = await flow.__anext__()
+        sse_sent.set()
+        # The server holds the stream open, so the response lands after the call is answered.
+        await call_done.wait()
+        with pytest.raises(StopAsyncIteration):
+            await flow.asend(httpx2.Response(200, request=request))
</file context>
Suggested change
await call_done.wait()
with anyio.fail_after(5):
await call_done.wait()

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.

Bug: OAuth provider holds context.lock for the whole request, so the standalone GET SSE stream stalls the first tools/call by ~15s

1 participant