perf: cache API key validation#139
Conversation
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an in-memory validation cache for API keys in the APIKeyStore class to optimize performance, along with a new test suite. The implementation includes TTL-based expiration and size-limited eviction. Feedback points out critical issues: the use of instance-level caching leads to inconsistency across multiple service instances, the eviction logic lacks thread-safety which could cause runtime errors, and clearing the entire cache during key creation is an unnecessary performance penalty.
| self.api_keys = None | ||
| self._connected = False | ||
| self._in_memory = False | ||
| self._validation_cache: Dict[str, tuple[float, Dict[str, Any]]] = {} |
There was a problem hiding this comment.
The _validation_cache is defined as an instance variable, but APIKeyStore is instantiated multiple times in the application (e.g., in src/api/routes/api_keys.py and src/api/dependencies.py). This means each instance maintains its own independent cache.
When a key is revoked or updated in one instance, the cache in the other instance (used by the authentication middleware) will not be cleared, allowing revoked keys to remain valid for the duration of the TTL. Consider making the cache a module-level variable (similar to _in_memory_api_keys) or ensuring a singleton instance is shared across the application.
| if len(self._validation_cache) >= VALIDATION_CACHE_MAX_SIZE: | ||
| oldest_key = next(iter(self._validation_cache)) | ||
| self._validation_cache.pop(oldest_key, None) |
There was a problem hiding this comment.
The cache eviction logic is not thread-safe. In a multi-threaded environment (like FastAPI with multiple worker threads), next(iter(self._validation_cache)) can raise a StopIteration exception if the dictionary is cleared by another thread between the length check and the next() call. It can also raise a RuntimeError if the dictionary size changes during iteration.
Consider using a threading.Lock to synchronize access to the cache or using a more robust eviction pattern that handles these concurrency edge cases.
| "is_active": True, | ||
| } | ||
| result = self.api_keys.insert_one(key_doc) | ||
| self._clear_validation_cache() |
There was a problem hiding this comment.
Clearing the entire validation cache when a new API key is created is unnecessary. A newly generated key cannot have a stale entry in the cache. Removing this call will prevent unnecessary cache misses for other active users during key creation.
|
@Genmin Please have a look at the suggestions |
3 participants
Summary
Why
Authenticated endpoints call validate_api_key on every request. Before this change, each request paid for both a MongoDB lookup and a last_used write. The cache keeps successful validations hot for a short bounded window while preserving existing behavior for invalid keys and in-memory fallback mode.
Fixes #135.
Validation
Note: the repo's default pytest addopts require pytest-cov in the environment; the targeted test was run with addopts disabled because pytest-cov is not installed in this checkout.