Stop counting an interactive OAuth login against request timeouts - #3635
Conversation
A 401 makes OAuthClientProvider await the developer's redirect_handler and callback_handler inside the challenged HTTP request, so a request's timeout also counted the person in the browser. For the connect-time server/discover probe that meant a login longer than its 10 seconds read as a legacy server, and Client(mode="auto") settled on 2025-11-25 against a server that speaks 2026-07-28. Any request with a read_timeout_seconds shorter than the login failed outright. send_raw_request now runs its timeout on a per-request clock held in a ContextVar, and the provider stops that clock while it awaits the two handlers. When they return, the timeout resumes with the budget that was left. The provider's own network calls stay on the clock, the probe keeps its 10 seconds, and a server that never answers is still given up on as before. A request timeout no longer ends a login nobody finishes: that takes a limit inside callback_handler or cancelling the caller. Fixes #3601
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/mcp/shared/_request_clock.py">
<violation number="1" location="src/mcp/shared/_request_clock.py:34">
P2: `pause()` can run after the deadline has elapsed but before AnyIO delivers its scheduled cancellation. It then moves the deadline to infinity, so an unfinished login can suppress the request timeout; cancel the scope when no budget remains.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| def pause(self) -> None: | ||
| if not self._pauses: | ||
| self._budget = self._scope.deadline - anyio.current_time() |
There was a problem hiding this comment.
P2: pause() can run after the deadline has elapsed but before AnyIO delivers its scheduled cancellation. It then moves the deadline to infinity, so an unfinished login can suppress the request timeout; cancel the scope when no budget remains.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/mcp/shared/_request_clock.py, line 34:
<comment>`pause()` can run after the deadline has elapsed but before AnyIO delivers its scheduled cancellation. It then moves the deadline to infinity, so an unfinished login can suppress the request timeout; cancel the scope when no budget remains.</comment>
<file context>
@@ -0,0 +1,81 @@
+
+ def pause(self) -> None:
+ if not self._pauses:
+ self._budget = self._scope.deadline - anyio.current_time()
+ self._scope.deadline = math.inf
+ self._pauses += 1
</file context>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/mcp/shared/jsonrpc_dispatcher.py— Legacy-mode (2025-11-25) OAuth clients whose token endpoint hangs after a login get a client where every later request blocks forever with no timeout, not just the one that timed out. When the challenged request's budget runs out during the token exchange, jsonrpc_dispatcher.py:431 writesnotifications/cancelledas a POST; post_writer runs that POST inline (streamable_http.py:682) and its auth flow waits oncontext.lock(oauth2.py:606) held by the hung exchange. The next request's_writeat :410 then blocks inside the paused clock, so its timeout never starts. Fix: bound the courtesy-cancel POST so a wedged auth flow cannot stall post_writer, or make_pauses-paused writes still honouropts["timeout"]as a total.Why this was flagged
Trigger:
Client(mode="legacy")or a 2025-11-25 server,OAuthClientProvider, read_timeout_seconds set, and/tokenhanging (test_login_time.py:252 pins exactly this hang but only with mode=LATEST_MODERN_VERSION at :269). With the diff the challenged request no longer times out during the login, so the first timeout now lands in the token exchange, where the provider lock (oauth2.py:606) is held with no bound on the httpx call. The timeout path at jsonrpc_dispatcher.py:430-441 sends a cancel notification; in legacy mode_consume_modern_cancellation(streamable_http.py:339) returns False and post_writer awaitshandle_request_async()inline at :682, which enters_auth_flowand blocks on the lock. Every subsequentsend_raw_requestnow blocks in_writeat :410, which is insiderequest_clockwith_pauses == 1, soclock.start()at :414 is never reached and no TimeoutError can ever fire.Verification:
_consume_modern_cancellation(streamable_http.py:338-340) returns False for a non-modern in-flight POST, so the notification is POSTed inline:await handle_request_async()at streamable_http.py:682 runs insideasync with tg_local(687-688). On the base,_writewas outsidefail_after(unbounded, as now) and the same cancel POST was sent on timeout.
| if not self._pauses: | ||
| self._budget = self._scope.deadline - anyio.current_time() | ||
| self._scope.deadline = math.inf |
There was a problem hiding this comment.
🟡 (optional) Users whose request budget ran out an instant before the 401 arrives now sit through the whole browser login and then still get Request ... timed out, instead of the prompt timeout the base fail_after gave. src/mcp/shared/_request_clock.py:34 reads a deadline that is already in the past when the runner has not yet delivered the expiry (trio expires deadlines only at the top of a scheduler pass; asyncio's timer handle is cancelled by the setter at :35 if it is still queued). The budget goes negative and :35 moves the deadline to inf for the entire login. Fix: in pause(), if deadline <= current_time() call self._scope.cancel() (or leave the deadline alone) instead of parking it at inf, so an exhausted budget fails the request before the person is sent to the browser.
Why this was flagged
Trigger: a request's budget expires in the same scheduler batch in which the challenged POST task receives the 401 and enters waiting_on_a_person() (oauth2.py:429). On trio the scope's cancel_called is only set when the run loop processes the deadline heap at the start of the next pass, so at _request_clock.py:34 self._scope.deadline is finite and already past; _budget becomes negative and :35 sets deadline = inf, removing the scope from the heap. On asyncio the same window exists when the timer handle is in the ready queue behind the POST task's step: the setter cancels the handle before it runs. Result: the person is sent to the browser, spends minutes logging in, and at resume() (:41) the negative budget fires the cancel so the caller gets REQUEST_TIMEOUT anyway. Base branch: fail_after fires at the deadline and the login never starts. Remedy: have pause() detect an already-exhausted budget and cancel the scope immediately rather than parking the deadline at inf.
Verification: pause() does self._budget = self._scope.deadline - anyio.current_time() (src/mcp/shared/_request_clock.py:34) with no clamp. If the deadline is already in the past but the backend has not yet delivered the expiry, _budget goes negative. So the caller waits through the entire browser login and then still receives the timeout, whereas the base fail_after would have delivered it at the next pass.
| with waiting_on_a_person(): | ||
| await self.context.redirect_handler(authorization_url) | ||
| result = await self.context.callback_handler() |
There was a problem hiding this comment.
🟡 nit (optional): Users who set read_timeout_seconds to bound an OAuth login now get a call that never returns, and no doc page says so. src/mcp/client/auth/oauth2.py:429 pauses the request clock for the whole redirect_handler/callback_handler wait, so a callback that never completes is no longer ended by the request timeout. Fix: update the OAuth client page (docs/client/oauth-clients.md, the callback_handler paragraph) in this PR to state that request timeouts exclude the login and that any bound on the person belongs inside callback_handler, as AGENTS.md asks for user-visible behaviour changes. [also at: src/mcp/client/auth/oauth2.py:429 - nit: AGENTS.md asks that user-visible behaviour changes update the relevant docs/ page in the same PR. Wrapping redirect_handler/callback_handler in waiting_on_a_person() changes what read_timeout_seconds (and per-call timeout) measures: a request's timeout no longer fires while a person is in the browser, and an unfinished login is no longer ended by it.]
Why this was flagged
A client built with Client(..., read_timeout_seconds=30) and an OAuthClientProvider whose callback_handler waits on a local redirect listener is challenged with a 401 and the person closes the browser without finishing. On the base branch anyio.fail_after(opts.get("timeout")) in send_raw_request fires after 30 s and the caller gets MCPError(REQUEST_TIMEOUT). After this change waiting_on_a_person() at src/mcp/client/auth/oauth2.py:429 has moved the scope deadline to math.inf (src/mcp/shared/_request_clock.py:34), so the call blocks until the caller is cancelled. The diff touches no file under docs/; docs/client/oauth-clients.md:60 describes callback_handler without saying the request timeout no longer covers it, and only docs/migration.md:2487 hints at bounding inside the handler. AGENTS.md requires the relevant docs page to be updated in the same PR when user-visible behaviour changes.
Verification: nit — triggered when a caller relies on read_timeout_seconds (or a per-call timeout) to bound an interactive login whose callback_handler never returns; the diff changes that user-visible behaviour and updates no page under docs/, which AGENTS.md:144-146 requires in the same PR. git diff c15566c HEAD -- docs/ is empty.
| # Transport tore down before run() noticed EOF; surface the documented contract. | ||
| raise MCPError(code=CONNECTION_CLOSED, message="Connection closed") from None | ||
| with anyio.fail_after(opts.get("timeout")): | ||
| with request_clock(opts.get("timeout")) as clock: |
There was a problem hiding this comment.
🟡 nit (optional): AGENTS.md says any change to an existing API's observable behaviour is an explicit maintainer design decision and should generally be avoided. Replacing anyio.fail_after(opts.get("timeout")) with request_clock(...) changes the observable semantics of Client.read_timeout_seconds / call timeout on 2.x: the budget now pauses for the whole of an OAuth login, so a request that previously raised REQUEST_TIMEOUT after N seconds can now outlive N indefinitely. Fix: have a maintainer explicitly sign off on the new timeout semantics (the PR links issue #3601 and states the trade-offs), and record the decision in the PR/docs rather than leaving it implicit in a private module.
Why this was flagged
Nothing fails at runtime for the happy path. The instruction guards the 2.x compatibility contract: existing callers who relied on read_timeout_seconds to bound an entire call_tool/initialize (including a stalled login) now see that bound suspended while redirect_handler/callback_handler run, so a login nobody completes hangs the call until the caller cancels. The PR author presents this as intended and scoped (private module, OAuthClientProvider only, revisited with #3517), which is the information the maintainer needs to make the call the instruction asks for.
Verification: AGENTS.md (base commit, "Branching Model") says: "v2 is released; its public API is a compatibility contract for the 2.x line. Removals, renames, or any change to an existing API's signature or observable behaviour ... is a design decision a maintainer makes explicitly, and should generally be avoided."
| """ | ||
| timeline, on_request = record_timeline() | ||
|
|
||
| with anyio.fail_after(LOGIN_SECONDS * 2): |
There was a problem hiding this comment.
🟡 nit (optional): AGENTS.md asks that indefinite waits be wrapped in anyio.fail_after(5). The new tests wrap their waits in guards of LOGIN_SECONDS * 2 (7200 s), DISCOVER_TIMEOUT_SECONDS * 2 and 1000 instead of 5. Fix: keep the widened guards where the virtual clock genuinely requires them (as the module docstring and the inline comment in test_request_clock.py:29 already explain), and add the same one-line reason at the sites that lack one, or derive a single named constant such as GUARD = LOGIN_SECONDS * 2 with the justification attached, which covers the 7 sites listed. Same instruction at 7 sites (tests/interaction/auth/test_login_time.py:151, :173, :207, :234, :263, :316, tests/shared/test_request_clock.py:29).
Why this was flagged
Nothing fails at runtime. The instruction guards against a hung test holding up CI; these tests run on trio's autojumping MockClock, so a 7200-virtual-second guard still fires in roughly zero wall-clock time whenever every task is blocked — a literal fail_after(5) would instead fire the moment the hour-long virtual login is reached. The departure is therefore well-founded, but it is a departure from the written value and only the module docstring and one inline comment articulate why.
Verification: AGENTS.md (base c15566c) Testing section says verbatim "Wrap indefinite waits (event.wait(), stream.receive()) in anyio.fail_after(5) to prevent hangs". The new tests/interaction/auth/test_login_time.py wraps its waits in with anyio.fail_after(LOGIN_SECONDS * 2): (lines 151, 206, 231, 258, 308; LOGIN_SECONDS = DISCOVER_TIMEOUT_SECONDS * 360 = 3600, so 7200 s) and anyio.fail_after(DISCOVER_TIMEOUT_SECONDS * 2) (line 178), and tests/shared/test_request_clock.py line 29 uses anyio.fail_after(1000), instead of the written value of 5; only test_cancelling_the_caller_mid_login_abandons_the_login (line 283) and the two asyncio unit tests use fail_after(5).
|
|
||
| # Wait for callback | ||
| result = await self.context.callback_handler() | ||
| with waiting_on_a_person(): |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: pre-existing: Users who open a subscription or hit a tool re-list while a login is pending still lose the call to their read timeout after merging. waiting_on_a_person() at src/mcp/client/auth/oauth2.py:429 only pauses the clock send_raw_request installs; two read_timeout_seconds bounds sit outside it as plain anyio.fail_after. Fix: every read-timeout bound around a request that can be challenged must be a pausable clock, e.g. pass the timeout into the request's opts["timeout"] and drop the outer fail_after, which covers the 2 sites listed. Same pattern at 2 sites (src/mcp/client/subscriptions.py:271, src/mcp/client/client.py:833). [also at: src/mcp/client/auth/oauth2.py:431 - pre-existing: A user whose first request after connecting is listen() still gets a TimeoutError when the login outlasts read_timeout_seconds.]
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
A client built with Client(transport, mode="2026-07-28", read_timeout_seconds=30) and an OAuthClientProvider whose token is missing or expired opens a subscription as its first call. drive() in src/mcp/client/subscriptions.py:250 sends the listen request with no opts["timeout"], so the clock from src/mcp/shared/jsonrpc_dispatcher.py:408 is infinite and the pause at src/mcp/client/auth/oauth2.py:429 changes nothing. The subscriber task meanwhile waits in anyio.fail_after(session._session_read_timeout_seconds) at src/mcp/client/subscriptions.py:271 for the ack; that scope is a plain deadline the clock cannot move, so a login longer than 30 s raises TimeoutError out of the subscribe context manager. The same holds at src/mcp/client/client.py:833, where anyio.fail_after(timeout) wraps _relist_tool and a 401 on the re-list ends in raise mismatch from relist_error. The base branch fails both the same way; the PR's description says login time is no longer counted, which holds only for the clock inside send_raw_request.
Verification: The pause at src/mcp/client/auth/oauth2.py:429 only touches the RequestClock installed at src/mcp/shared/jsonrpc_dispatcher.py:408. src/mcp/client/subscriptions.py:271-272 waits in a plain anyio.fail_after that keeps counting while the login blocks the POST. src/mcp/client/client.py:833 wraps _relist_tool in anyio.fail_after(timeout). Both files are untouched by the PR, so the base fails identically.
| @contextmanager | ||
| def waiting_on_a_person() -> Iterator[None]: | ||
| """Stop the clock of the request this code is serving while the block is open; a no-op if there is none. | ||
There was a problem hiding this comment.
🟣 pre-existing, not blocking: pre-existing: Callers with several requests in flight when a token expires still see every request but one fail with REQUEST_TIMEOUT during the login. waiting_on_a_person() at src/mcp/shared/_request_clock.py:70 finds only the challenged request's clock; the others wait for self.context.lock at src/mcp/client/auth/oauth2.py:606 on their own clocks. Fix: a request queued on the provider lock while a login is in progress must have its clock paused as well, e.g. record a login-in-progress event in the context and wrap the lock wait in waiting_on_a_person() while it is set. The requirement client-auth:login-time:other-requests-keep-counting pins this as a known limit; the failure is still a timeout nobody can avoid. [also at: src/mcp/client/auth/oauth2.py:429 - pre-existing: Callers with other requests in flight when an interactive login starts still get Request ... timed out for those requests.]
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
An application with Client(..., read_timeout_seconds=30) fans out list_tools, list_prompts and list_resources concurrently after its access token expired and the authorization server issued no refresh token. All three POSTs hit _auth_flow in src/mcp/client/auth/oauth2.py:604; the first takes self.context.lock at src/mcp/client/auth/oauth2.py:606, gets the 401 and enters waiting_on_a_person() at src/mcp/client/auth/oauth2.py:429, which pauses that one request's clock via src/mcp/shared/_request_clock.py:70. The other two block on the lock with their clocks running, so after 30 s send_raw_request raises MCPError(code=REQUEST_TIMEOUT) for each, as tests/interaction/auth/test_login_time.py:341 pins at 5.0 s. The base branch times out all requests, including the challenged one; the change fixes only the one holding the lock.
Verification: pre-existing; acknowledged in diff: tests/interaction/auth/test_login_time.py:309-311 pins this outcome. waiting_on_a_person() (src/mcp/shared/_request_clock.py:70-81) pauses only the challenged task's clock; other requests block at src/mcp/client/auth/oauth2.py:606 with clocks running, so each fails with REQUEST_TIMEOUT. The base commit timed out every request, so merging makes nothing worse.
1 participant
Fixes #3601
A request's timeout was also counting the time a person spends in the browser during an interactive OAuth login. For the
server/discoverprobe thatClient(mode="auto")sends on connect, a login longer than its 10 seconds looked like a legacy server, so the client settled on2025-11-25against a server that speaks2026-07-28.The timeout now stops counting while
OAuthClientProviderwaits onredirect_handlerandcallback_handler, and picks up again with whatever was left. The probe keeps its 10 seconds, and a server that never answers is handled as before.This is kept deliberately small. The clock lives in a private module and isn't public API. Where a request's timeout lives is likely to move as the client transport and dispatcher work (#3517) takes shape, and this will be looked at again alongside it.
Worth knowing:
callback_handler, or cancelling the caller.OAuthClientProvideris covered. A customhttpx2.Auth, like the one in the issue's example script, is still counted.The tests run the real provider on a virtual clock: a login that outlasts the 10 seconds negotiates
2026-07-28, and a silent server still falls back at 10 seconds.AI Disclaimer