ENG-2264 Add in-memory oauth client caching

[erosselli]

Ticket ENG-2264

Description Of Changes

This PR adds an in-memory cache of oauth client details, to avoid an extra query to the DB on every request. The cache TTL can be configured via the security setting oauth_client_cache_ttl_seconds (and disabled setting it to 0). Because we default it to 5min , we're ok with having stale info for up to that period of time. We will handle cache invalidations in a follow up PR.

Code Changes

Steps to Confirm

Pre-Merge Checklist

• Issue requirements met
• All CI pipelines succeeded
• CHANGELOG.md updated
  • Add a db-migration This indicates that a change includes a database migration label to the entry if your change includes a DB migration
  • Add a high-risk This issue suggests changes that have a high-probability of breaking existing code label to the entry if your change includes a high-risk change (i.e. potential for performance impact or unexpected regression) that should be flagged
  • Updates unreleased work already in Changelog, no new entry necessary
• UX feedback:
  • All UX related changes have been reviewed by a designer
  • No UX review needed
• Followup issues:
  • Followup issues created
  • No followup issues
• Database migrations:
  • Ensure that your downrev is up to date with the latest revision on main
  • Ensure that your downgrade() migration is correct and works
    • If a downgrade migration is not possible for this change, please call this out in the PR description!
  • No migrations
• Documentation:
  • Documentation complete, PR opened in fidesdocs
  • Documentation issue created in fidesdocs
  • If there are any new client scopes created as part of the pull request, remember to update public-facing documentation that references our scope registry
  • No documentation updates required

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments

Project	Deployment	Review	Updated (UTC)
fides-plus-nightly	Ignored	Preview	Jan 9, 2026 5:33pm
fides-privacy-center	Ignored		Jan 9, 2026 5:33pm

[greptile-apps]

Greptile Overview

Greptile Summary

This PR implements in-memory caching for OAuth client details to reduce database queries during authentication. While the intent is good, the implementation has several critical issues that need to be addressed before merging.

Major Issues Found

• Critical: SQLAlchemy Detached Instance Error - Cached ClientDetail objects are mutable ORM instances that will become detached from their database sessions. When is_token_invalidated() tries to access the lazy-loaded client.user.password_reset_at relationship, it will fail with a DetachedInstanceError or return stale data
• Thread Safety - The global cache is not thread-safe. Concurrent reads/writes from multiple threads can cause race conditions
• Timezone Issues - Using naive datetime.now() instead of timezone-aware datetime.now(timezone.utc) can cause incorrect expiration calculations
• Multi-Worker Cache Inconsistency - Each worker process has its own in-memory cache, so invalidations won't propagate and different workers may serve stale data

Minor Issues

• Arbitrary eviction strategy instead of LRU
• No negative result caching (repeated lookups of non-existent clients hit DB)
• Missing test coverage for thread safety, detached instances, and timezone handling
• Tests lack autouse fixture for cache cleanup between test runs

Recommendations

1. Eagerly load the user relationship before caching to prevent detached instance errors
2. Add threading.Lock() for thread-safe cache operations
3. Use timezone-aware datetimes throughout
4. Document multi-worker limitations or consider Redis-based distributed cache
5. Add comprehensive test coverage for edge cases

Confidence Score: 1/5

• This PR has critical issues that will cause runtime errors in production
• Score of 1 reflects multiple critical bugs: SQLAlchemy detached instance errors that will break password reset invalidation checks, thread safety issues that can cause race conditions, and timezone-naive datetime usage that can cause incorrect cache expiration. These issues will manifest as runtime errors and security vulnerabilities in production
• Pay close attention to src/fides/api/oauth/client_cache.py (new caching implementation with thread safety and SQLAlchemy session issues) and src/fides/api/oauth/utils.py (will fail when accessing cached client's lazy-loaded user relationship)

Important Files Changed

File Analysis

Filename	Score	Overview
src/fides/api/oauth/client_cache.py	1/5	New in-memory OAuth client cache with critical issues: missing timezone awareness in datetime operations, no thread safety for concurrent access, caching of mutable SQLAlchemy ORM objects that can become detached, arbitrary eviction strategy, and multi-worker cache inconsistency concerns
src/fides/api/oauth/utils.py	1/5	Updated to use cached client lookup. Critical issue: accessing lazy-loaded client.user.password_reset_at relationship on cached clients will fail with DetachedInstanceError or return stale data
src/fides/config/security_settings.py	5/5	Added oauth_client_cache_ttl_seconds configuration field with 300 second (5 minute) default. Implementation looks correct
tests/lib/test_client_cache.py	2/5	New test file with basic cache functionality tests. Missing critical test coverage: thread safety, SQLAlchemy detached instances, multi-worker scenarios, and missing autouse fixture for cache cleanup between tests

[greptile-apps]

Use datetime.now(timezone.utc) for timezone-aware datetime. Using naive datetime.now() without timezone can cause issues in distributed systems or when comparing with timezone-aware datetimes from the database.

Suggested change

	now = datetime.now()
	now = datetime.now(timezone.utc)

[greptile-apps]

Use datetime.now(timezone.utc) for timezone-aware datetime. Consistency with timezone-aware datetimes is important for accurate expiration calculations.

Suggested change

	expires_at = datetime.now() + timedelta(seconds=ttl_seconds)
	expires_at = datetime.now(timezone.utc) + timedelta(seconds=ttl_seconds)

[greptile-apps]

Thread safety concern: The global _client_cache instance is not thread-safe. Dictionary operations in Python are not atomic, and concurrent reads/writes from multiple threads (common in FastAPI with uvicorn workers) can cause race conditions. Consider using threading.Lock() for critical sections or using a thread-safe data structure.

Example fix:

import threading

class ClientCache:
    def __init__(self, max_size: int = MAX_CACHE_SIZE):
        self._cache: Dict[str, CacheEntry] = {}
        self._max_size = max_size
        self._lock = threading.Lock()
    
    def get(self, client_id: str) -> Optional[ClientDetail]:
        with self._lock:
            # existing logic

[greptile-apps]

Cached ClientDetail objects are mutable SQLAlchemy ORM instances with lazy-loaded relationships. If the cached object's client.user.password_reset_at is accessed after being cached, it could trigger a lazy-load database query using a stale or closed database session, causing errors. The cached object may also become detached from its session.

Consider either:

1. Eagerly loading required relationships before caching
2. Serializing/deserializing the client data instead of caching ORM objects
3. Documenting that cached objects should not access relationships

[greptile-apps]

Eviction strategy is arbitrary and unpredictable. Using next(iter(self._cache)) doesn't guarantee removal of the oldest or least-recently-used entry. In Python 3.7+, dicts maintain insertion order, but this doesn't account for access patterns. Consider implementing a proper LRU (Least Recently Used) eviction policy for better cache performance.

_{Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!}

[greptile-apps]

Missing timezone import from datetime module. The suggestion to use datetime.now(timezone.utc) requires this import.

Suggested change

	from dataclasses import dataclass
	from datetime import datetime, timedelta, timezone

[greptile-apps]

Multi-worker concern: In production deployments with multiple uvicorn workers or multiple server instances, each process will have its own separate in-memory cache. This means:

1. Cache invalidation won't propagate across workers
2. Different workers may serve requests with different cached data
3. When client permissions/scopes change, some requests may use stale data for up to 5 minutes depending on which worker handles them

The PR description mentions "we will handle cache invalidations in a follow up PR", but this architectural limitation should be documented or addressed with a distributed cache (Redis) instead.

[greptile-apps]

Only caches found clients. If a client lookup returns None (client doesn't exist), the cache doesn't remember this. Every request for a non-existent client will hit the database. Consider caching negative results (with a shorter TTL) to prevent potential DoS attacks where an attacker repeatedly requests invalid client IDs.

[greptile-apps]

Missing test coverage for critical scenarios:

1. Thread safety: No tests for concurrent access to the cache from multiple threads
2. SQLAlchemy detached instances: No tests verifying that cached clients with user relationships can be accessed after caching (will fail when is_token_invalidated tries to access client.user.password_reset_at)
3. Multi-worker scenarios: No tests for cache invalidation across multiple processes
4. Negative caching: No tests for repeated lookups of non-existent clients
5. Timezone handling: No tests verifying timezone-aware datetime behavior

[greptile-apps]

Missing global cache cleanup fixture. The global _client_cache instance is shared across all tests. Without an autouse fixture that clears the cache before/after each test, tests may interfere with each other, leading to flaky test results. Add:

@pytest.fixture(scope="function", autouse=True)
def clear_cache():
    """Clear the global cache before each test."""
    clear_client_cache()
    yield
    clear_client_cache()

[greptile-apps]

Additional Comments (1)

src/fides/api/oauth/utils.py
CRITICAL: Cached ClientDetail objects may have stale or detached SQLAlchemy sessions. When is_token_invalidated() (line 425) tries to access client.user.password_reset_at, it will attempt a lazy-load query on the client.user relationship. If the cached client is detached from its session or the session is closed, this will raise DetachedInstanceError or return stale data.

This needs to be addressed by either:

1. Eagerly loading the user relationship before caching: db.query(ClientDetail).options(joinedload(ClientDetail.user)).filter(...)
2. Not caching clients with associated users
3. Handling the detached instance error gracefully in is_token_invalidated()