Skip to content

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

Open
guptaishaan wants to merge 2 commits into
modelcontextprotocol:mainfrom
guptaishaan:fix-3209
Open

Release context.lock before the protected request is sent#3252
guptaishaan wants to merge 2 commits 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.

All reported issues were addressed across 2 files

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

Re-trigger cubic

Comment thread src/mcp/client/auth/oauth2.py
Comment thread tests/client/test_auth.py Outdated
Releasing context.lock before the protected request is yielded let a second
request restamp the shared context.protocol_version while the first was in
flight. The first request's 401 or 403 re-authorization then ran on the other
request's MCP-Protocol-Version, which decides whether the resource parameter is
sent. Holding the lock across the yield used to hide this.

Read the header into a local once, and restamp context.protocol_version from it
inside the 401 and 403 blocks after the lock is re-acquired. Every read of
context.protocol_version happens under the lock in one of those regions, so this
needs no new parameter on the token-request helpers.

Also wrap the concurrency test's call_done.wait() in anyio.fail_after(5) so a
failure in the sibling task fails fast instead of hanging.
@guptaishaan

Copy link
Copy Markdown
Author

Both valid, fixed in the follow-up commit.

The P2 is a regression this PR introduced. Before it, the lock was held across the
yield, so nothing could restamp context.protocol_version while a request was in
flight. Now a second request can, and the first one's 401/403 re-authorization would
use the wrong version and flip the resource parameter.

async_auth_flow now reads the header into a local once, and restamps
self.context.protocol_version from that local inside the 401 and 403 blocks after
the lock is re-acquired. Every read of context.protocol_version
(_perform_authorization_code_grant, _exchange_token_authorization_code,
_refresh_token) happens under the lock in one of those three regions, so this
closes the window without threading a new parameter through those methods.

New test test_step_up_uses_the_protocol_version_of_its_own_request holds a
2025-06-18 request at its yield, runs a 2025-03-26 request through its initial
block, then 403s the first one and asserts the token exchange still carries
resource=. It fails on the previous commit and passes now.

P3: applied the anyio.fail_after(5) suggestion.

./scripts/test is clean at 100% branch coverage, and ruff and pyright pass.

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