Skip to content

Guard OAuth callback result from post-completion overwrites#3417

Merged
jlowin merged 3 commits intomainfrom
codex/fix-oauth-callback-result-overwrite-issue
Mar 7, 2026
Merged

Guard OAuth callback result from post-completion overwrites#3417
jlowin merged 3 commits intomainfrom
codex/fix-oauth-callback-result-overwrite-issue

Conversation

@jlowin
Copy link
Member

@jlowin jlowin commented Mar 6, 2026

Motivation

  • Prevent a race where the anyio.Event-based OAuth callback flow could be overwritten by subsequent requests after the first completion, restoring first-writer semantics lost during the Future→Event refactor.

Description

  • Add a store_result_once helper in src/fastmcp/client/oauth_callback.py that writes to the shared OAuthCallbackResult and calls result_ready.set() only if the event has not already been set, and route all callback success/error paths through it.
  • Add a regression test tests/client/test_oauth_callback_race.py that starts the callback server, sends a legitimate callback, then sends a second callback and asserts the original result is retained.

Testing

  • Ran uv sync successfully and executed the targeted test with uv run pytest tests/client/test_oauth_callback_race.py -q, which passed.
  • Ran the full test suite with uv run pytest -n auto in this environment but observed unrelated failures/timeouts (14 failed, 4675 passed) that pre-existed or are environment-dependent and are not caused by this change.
  • uv run prek run --all-files failed to initialize hooks due to network/proxy restrictions when fetching pre-commit hooks (hook init error).

Codex Task

@marvin-context-protocol marvin-context-protocol bot added bug Something isn't working. Reports of errors, unexpected behavior, or broken functionality. auth Related to authentication (Bearer, JWT, OAuth, WorkOS) for client or server. client Related to the FastMCP client SDK or client-side functionality. labels Mar 6, 2026
@marvin-context-protocol
Copy link
Contributor

marvin-context-protocol bot commented Mar 6, 2026

Test Failure Analysis

(Updated: previous timeouts in test_oauth_proxy_storage.py are no longer the issue; this reflects the latest run 22802247877)

Summary: test_metadata_advertises_cimd_support in tests\server\auth\oauth_proxy\test_oauth_proxy.py timed out on Windows Python 3.10 only. All other platforms passed.

Root Cause: This failure is unrelated to this PR. The timeout occurs inside proxy.get_routes()logger.debug()rich.logging.RichHandler.emit()rich.console.is_terminalisatty() on a temp file wrapper on Windows. This is a known Windows-specific issue where rich's terminal detection can hang when pytest captures output via a temp file. The PR only modifies src/fastmcp/client/oauth_callback.py and adds tests/client/test_oauth_callback_race.py — neither touches proxy.py or test_oauth_proxy.py.

Suggested Solution: This is a pre-existing flaky test on Windows CI, not caused by this PR. Re-running the workflow should confirm if it is transient. If it recurs consistently, the affected test could be given a longer timeout or marked to skip on Windows — but that is a separate issue from this PR.

Detailed Analysis

Failing test: tests\server\auth\oauth_proxy\test_oauth_proxy.py::test_metadata_advertises_cimd_support
Platform: Windows Python 3.10 only
Failure type: pytest-timeout 5-second timeout

Relevant stack trace:

File "tests\server\auth\oauth_proxy\test_oauth_proxy.py", line 92, in test_metadata_advertises_cimd_support
    app = Starlette(routes=proxy.get_routes())
  File "src\fastmcp\server\auth\oauth_proxy\proxy.py", line 1517, in get_routes
    logger.debug(...)
  File "rich\logging.py", line 178, in emit
    self.console.print(log_renderable)
  File "rich\console.py", line 977, in is_terminal
    return False if isatty is None else isatty()
  File "C:\...\tempfile.py", line 483, in func_wrapper
    return func(*args, **kwargs)
++++++++++++++++++ Timeout ++++++++++++++++++

The hang occurs in rich's is_terminal property, which calls isatty() on a tempfile wrapper that pytest uses to capture stderr. On Windows, this call can block or be extremely slow. This is a Windows-specific interaction between rich, pytest's output capture, and the Windows filesystem.

PR changes do NOT touch:

  • tests/server/auth/oauth_proxy/test_oauth_proxy.py
  • src/fastmcp/server/auth/oauth_proxy/proxy.py

PR changes only touch:

  • src/fastmcp/client/oauth_callback.py — adds store_result_once() guard
  • tests/client/test_oauth_callback_race.py — new regression test (passed ✓ on all platforms)
Related Files
  • tests/server/auth/oauth_proxy/test_oauth_proxy.py — failing test (unrelated to PR; pre-existing Windows flakiness)
  • src/fastmcp/server/auth/oauth_proxy/proxy.py:1517 — debug log call that triggers the rich console hang
  • src/fastmcp/client/oauth_callback.py — PR's primary change (not involved in failure)
  • tests/client/test_oauth_callback_race.py — PR's new test (passed on all platforms)

@jlowin
Copy link
Member Author

jlowin commented Mar 6, 2026

/marvin fix static analysis

@marvin-context-protocol
Copy link
Contributor

marvin-context-protocol bot commented Mar 6, 2026

Claude finished @jlowin's task in 46s —— View job


Fixed static analysis

  • Identify the issue — long line in tests/client/test_oauth_callback_race.py (line 28) failing ruff format
  • Apply the fix — split client.get() call across multiple lines
  • Verify with ruff — passes cleanly
  • Commit and push — bdc7f4ca

Marvin Context Protocol | Type /marvin to interact further

Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.

Marvin Context Protocol and others added 2 commits March 6, 2026 22:33
Co-authored-by: Jeremiah Lowin <jlowin@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@jlowin
Copy link
Member Author

jlowin commented Mar 7, 2026

Auto-reviewed and merging on behalf of @jlowin — CI is green (Windows OAuth proxy timeouts are pre-existing flaky tests).

@jlowin jlowin merged commit 544f072 into main Mar 7, 2026
10 of 12 checks passed
@jlowin jlowin deleted the codex/fix-oauth-callback-result-overwrite-issue branch March 7, 2026 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

aardvark auth Related to authentication (Bearer, JWT, OAuth, WorkOS) for client or server. bug Something isn't working. Reports of errors, unexpected behavior, or broken functionality. client Related to the FastMCP client SDK or client-side functionality. codex

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant