OAuthProxy: _refresh_locks dict grows unboundedly with token churn

[strawgate]

Description

OAuthProxy._refresh_locks (line 559 of proxy.py) creates an anyio.Lock per upstream_token_id to prevent concurrent refresh races. Entries are never removed, so the dict grows monotonically with every distinct token ID.

Impact

Growth rate is one entry per unique user whose token has been refreshed. Each entry is ~200 bytes (lock object + string key). In a typical deployment this is bounded by the user base, so it's unlikely to cause issues unless the proxy runs for a very long time with high user churn.

Reproduction

# After N distinct users refresh tokens:
len(proxy._refresh_locks)  # N entries, never decreasing

Possible fixes

• Cap with an OrderedDict + LRU eviction (simplest, minor race window on evicted entries)
• Periodic sweep of unheld locks (anyio.Lock doesn't expose held state)
• Accept the current behavior and document the growth bound

Found during an audit of unaddressed review feedback on merged PRs (originally flagged by Codex bot on PR #3713).

[marvin-context-protocol]

Claude finished @strawgate's task in 2m 12s —— View job

---

tl;dr

Confirmed: _refresh_locks grows monotonically at proxy.py:1617-1619. Bounded by distinct upstream token IDs (one per user session), so in practice the dict size tracks unique-user-count — not a memory emergency, but still a real (if slow) leak. Recommendation: cap with an OrderedDict + LRU eviction (option 1 from the issue). The "minor race window on evicted entries" is genuinely minor — see reasoning below.

Note: the issue says this was "originally flagged by Codex bot on PR #3713", but PR #3713 is the docs publish PR. The actual PR that introduced _refresh_locks is #3584 (merged 2026-03-22). The codex bot on that PR flagged 4 other issues but did not flag this one — so this is a fresh finding, not unaddressed review feedback.

Recommendation

Adopt LRU eviction via OrderedDict, with a cap around 10,000 entries. This is the simplest, most defensible fix. The race window the issue mentions is benign because:

1. The lock is already advisory within a single process — cross-process races are explicitly out of scope (see the class docstring comment at proxy.py:554-558: "Does not protect against cross-process races in distributed deployments — those are handled by re-reading from storage after refresh failure.")
2. Eviction only touches least-recently-used entries. For an entry to be evicted and be the subject of an in-flight refresh, the token would need to have been untouched long enough to churn out of the LRU while simultaneously being actively refreshed — contradictory conditions.
3. Even in the worst case (evicted lock gets recreated while another task holds the old one), the outcome is the same as the pre-Transparently refresh upstream token in OAuthProxy.load_access_token() #3584 behavior — two parallel refreshes for one token, one wins, one gets a None/401. The reporter's _try_transparent_refresh error path at proxy.py:1655-1673 already handles this by reloading from storage.

The other two options are worse:

• Periodic sweep of unheld locks — anyio.Lock doesn't expose locked state without subclassing; you'd end up racing on lock state anyway.
• Accept and document — punts the problem; trivially preventable with ~5 lines.

A weakref.WeakValueDictionary was tempting but won't work: async with lock: only holds a local strong reference for the duration of the critical section — outside that window, the entry would be collectible even while the refresh's I/O is in flight.

Findings

The offending code:

# src/fastmcp/server/auth/oauth_proxy/proxy.py:1615-1621
if token_id not in self._refresh_locks:
    self._refresh_locks[token_id] = anyio.Lock()
lock = self._refresh_locks[token_id]

async with lock:
    ...

Keys are upstream_token_id (stable per-user per-session identifier from the upstream IdP). In deployments with transient/ephemeral users (B2C auth flows, short-lived service accounts), this grows unbounded over the process lifetime. Low absolute rate (~200 bytes/entry), but no ceiling.

No code path ever removes entries.

Detailed Action Plan

1. Edit src/fastmcp/server/auth/oauth_proxy/proxy.py:

   • Change the import: from collections import OrderedDict.
   • At proxy.py:559, replace self._refresh_locks: dict[str, anyio.Lock] = {} with self._refresh_locks: OrderedDict[str, anyio.Lock] = OrderedDict().
   • Add a module-level constant near the top of the file: _REFRESH_LOCK_CACHE_SIZE = 10_000 (a pragmatic cap — 2 MB worst case; raise if anyone hits it).
   • Extract the lock-getter into a small helper to keep the call site readable:

   def _get_refresh_lock(self, token_id: str) -> anyio.Lock:
       """Get-or-create a per-token refresh lock with LRU eviction.
   
       Locks are cached to serialize concurrent refreshes of the same
       upstream token within a process. Entries are evicted LRU-style
       to bound memory; evicted entries are least-recently-used by
       definition, so eviction during an active refresh is implausible.
       """
       lock = self._refresh_locks.get(token_id)
       if lock is None:
           lock = anyio.Lock()
           self._refresh_locks[token_id] = lock
           if len(self._refresh_locks) > _REFRESH_LOCK_CACHE_SIZE:
               self._refresh_locks.popitem(last=False)
       else:
           self._refresh_locks.move_to_end(token_id)
       return lock

   • Replace proxy.py:1617-1619 with lock = self._get_refresh_lock(token_id).

2. Add a test in tests/server/auth/oauth_proxy/test_tokens.py (next to the existing test_transparent_refresh_* tests around line 714):

   • Test that _refresh_locks is capped (spin through _REFRESH_LOCK_CACHE_SIZE + 100 distinct IDs by calling _get_refresh_lock directly, assert len() == _REFRESH_LOCK_CACHE_SIZE).
   • Test LRU semantics (touch an old entry, add new entries, assert the touched one survives).
   • Monkey-patch the constant down to ~3 to keep the test fast.

3. Run the workflow from CLAUDE.md:

   uv sync
   uv run pytest tests/server/auth/oauth_proxy/test_tokens.py -n auto
   uv run prek run --all-files

4. No docs change needed — this is an internal implementation detail with no user-facing API.

Related Items

Repository	Issue or PR	Relevance
PrefectHQ/fastmcp	#3584 Transparently refresh upstream token in OAuthProxy.load_access_token()	Introduced _refresh_locks to fix a per-token refresh race. This issue is a follow-up memory-bound concern on the same code.
PrefectHQ/fastmcp	#3581 Session death on upstream token expiry	Root cause that #3584 fixed; context for why the lock exists.

Related Files

File	Relevance	Lines
src/fastmcp/server/auth/oauth_proxy/proxy.py	Declaration of _refresh_locks and the get-or-create site that needs changing.	559, 1617-1619
tests/server/auth/oauth_proxy/test_tokens.py	Existing refresh-related tests; add new LRU-cap tests alongside these.	714-800

Related Webpages

Name	URL	Relevance
collections.OrderedDict — Python docs	https://docs.python.org/3/library/collections.html#collections.OrderedDict	move_to_end / popitem(last=False) primitives used in the proposed LRU pattern.
anyio.Lock — AnyIO docs	https://anyio.readthedocs.io/en/stable/api.html#anyio.Lock	Confirms no public API to inspect held state — rules out option 2 (periodic sweep) without monkeypatching.

---

Marvin Context Protocol | Type /marvin to interact further

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

[CrepuscularIRIS]

Happy to take this. Plan: switch _refresh_locks to OrderedDict, move_to_end on hit, popitem(last=False) when over a cap. Matches the existing _obo_credentials pattern in AzureProvider. Will open a PR with a test.