perf: keep hot-path DynamoDB calls off the event loop#108
Open
warren830 wants to merge 2 commits into
Open
Conversation
- Cache API key validation in-process (TTL 60s, negative 5s) and run DynamoDB misses via asyncio.to_thread so auth never blocks the loop - Record usage on a single background writer thread (record_usage_nowait) so streaming responses no longer stall on put_item - Cache model mapping lookups (TTL 300s) including pass-through misses - Add shared TTLCache utility (bounded, thread-safe); new config flags API_KEY_CACHE_TTL_SECONDS / MODEL_MAPPING_CACHE_TTL_SECONDS
- validate_api_key re-raises ClientError so DynamoDB throttling is a lookup failure (401, uncached), never a negative-cached 'invalid key' that locks valid keys out in rolling 5s windows - TTLCache overflow now drops soonest-expiring entries instead of clearing everything: negative-cache spam evicts itself, not hot keys - Auth cache keyed by SHA-256 digest (no plaintext keys as dict keys, bounded per-entry size), values deep-copied both ways, lookups run on a dedicated 4-thread pool with single-flight coalescing per key - Usage writer: bounded backlog (drop + warn + prometheus counter), submit-after-shutdown guard, kwargs validated at the call site, timestamps stamped at submit with ms precision, drain hook wired into app lifespan shutdown - Shared tests/conftest.py clears the model-mapping cache between tests (class-level cache made existing tests order-dependent)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Every request currently makes 3+ synchronous boto3 DynamoDB calls directly on the asyncio event loop: API key validation (auth middleware), model mapping lookup, and usage recording (including inside streaming paths). Each call freezes the event loop for the full round trip (2–10ms), which:
1s / total_blocking_timeregardless of Bedrock latency or connection countPerformance
API_KEY_CACHE_TTL_SECONDS, default 60s; invalid keys only 5s so brute-force spam can't pin entries and new keys work almost immediately). Cache misses run on a dedicated 4-thread pool — not the default executor, which this codebase already shares with multi-second Tavily/web-fetch/docker work — with single-flight coalescing so a hot key's TTL expiry triggers exactly one DynamoDB read.UsageTracker.record_usage_nowait()(all 13 call sites). Streaming responses no longer stall onput_item.MODEL_MAPPING_CACHE_TTL_SECONDS, default 300s), including negative results — clients passing Bedrock ARNs directly (pass-through) previously paid a wasted DynamoDB read per request.Correctness / hardening (from a 5-perspective adversarial review of the first commit; see below)
APIKeyManager.validate_api_keynow re-raisesClientErrorinstead of returningNone: previously DynamoDB throttling was indistinguishable from "invalid key", which (with negative caching) would have locked valid keys out in rolling 5s windows during a throttling event. Lookup failures now return 401 without caching.TTLCacheoverflow drops the soonest-expiring entries instead of clearing everything, so attacker-inserted short-TTL negative entries evict themselves rather than hot positive entries.api_key_infofields can't poison the cache.usage_writes_dropped_totalcounter instead of unbounded memory growth if DynamoDB hangs), submit-after-shutdown guard (a usage write insideexcept/finallycan never mask the original exception), kwargs validated at the call site (a typo raisesTypeErrorinstead of becoming silently dropped billing data), timestamps stamped at submit time with millisecond precision (same-second rows for one key previously overwrote each other viaput_item; a draining backlog made that worse), and adrain_usage_writes()flush hook wired into app lifespan shutdown.Review
The first commit was reviewed by five independent passes (security, performance, testing, maintainability, adversarial). All CRITICAL and high-confidence findings are fixed in the second commit. Deferred items, documented deliberately:
get_mappingcache misses still run synchronously on the loop in the converter path (anthropic_to_bedrock._convert_model_id). The cache makes misses rare for legitimate traffic; a full fix requires async-ifying the converter chain and is left as a follow-up.config.pyandenv.example). There is no cross-process invalidation channel; operators needing faster revocation can lower or disable the TTL.app/main.pycallssys.exit(0)directly, bypassing uvicorn's graceful shutdown — pre-existing behavior, out of scope here, but it limits how much the new shutdown drain hook can help on deploys.Tableresources are shared across threads. Table operations delegate to the underlying thread-safe client and writes are serialized on one thread; noted because boto3 documents resources as not thread-safe.Test coverage
tests/unit/test_ttl_cache.py,test_auth_cache.py,test_usage_nowait.py,test_model_mapping_cache.py, covering TTL boundaries (exactly-at-expiry), negative caching, eviction under pressure, single-flight coalescing, dedicated-executor placement, off-loop execution, error paths (ClientError, shutdown executor, full backlog), and cache invalidation on set/delete.tests/conftest.pyclears the class-level model-mapping cache between tests (the review caught that the shared cache made existing moto-based tests order-dependent).tests/unit/test_cdk_deploy_env_validation.py::test_deploy_requires_bedrock_api_key_when_openai_compat_enabledfails identically on a cleanmaincheckout (environment-dependent, unrelated to this change).Config
Two new settings (documented in
env.example):Both default to safe values; setting them to 0 restores the previous per-request read behavior exactly.
🤖 Generated with Claude Code