feat(cache): add OAuth client cache with redis-aside support#155
feat(cache): add OAuth client cache with redis-aside support#155
Conversation
Add a new Cache[OAuthApplication] instance that caches client lookups by client_id using the cache-aside pattern. store.GetClient() is called 20+ times across all OAuth flows (device code, authorization code, token exchange, client credentials) — this was the hottest uncached DB query path. Key design decisions: - GetClient() returns cached copy with ClientSecret stripped (defense-in-depth) - GetClientWithSecret() bypasses cache for secret-verification flows - Explicit invalidation on all mutations (create, update, delete, approve, reject, secret regeneration) - Inject ClientService into DeviceService, TokenService, and AuthorizationService to replace direct store.GetClient() calls Configuration: CLIENT_CACHE_TYPE, CLIENT_CACHE_TTL (5m default), CLIENT_CACHE_CLIENT_TTL (30s), CLIENT_CACHE_SIZE_PER_CONN (32MB) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR introduces a dedicated OAuth client lookup cache (cache-aside, with optional Redis / Redis-aside backends) and wires a ClientService through OAuth flow services to eliminate repeated hot-path store.GetClient() DB queries while keeping client secret material out of the cache.
Changes:
- Add
ClientServiceclient caching (Cache[models.OAuthApplication]) with TTL configuration and explicit invalidation on client mutations. - Inject
ClientServiceintoDeviceService,TokenService, andAuthorizationServiceand switch client lookups to cached/uncached methods depending on whether the secret is needed. - Add bootstrap + config + env wiring for the new client cache and update tests/benchmarks to pass the new service dependencies.
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/services/token.go | Adds clientService dependency to TokenService construction. |
| internal/services/token_test.go | Updates test helpers to construct/pass ClientService into services. |
| internal/services/token_introspect_test.go | Updates introspection service construction with ClientService. |
| internal/services/token_exchange.go | Uses cached clientService.GetClient for device-code exchange client validation. |
| internal/services/token_client_credentials.go | Uses GetClientWithSecret for confidential client authentication flows. |
| internal/services/token_client_credentials_test.go | Passes ClientService into token service test setup. |
| internal/services/token_cache_test.go | Updates token cache tests to include ClientService and updated constructors. |
| internal/services/token_cache_bench_test.go | Updates benchmark environment to include ClientService. |
| internal/services/device.go | Adds clientService dependency and routes client validation through it. |
| internal/services/device_test.go | Updates device service tests to construct/pass ClientService. |
| internal/services/device_security_test.go | Updates security tests to construct/pass ClientService. |
| internal/services/client.go | Adds client cache fields, cached GetClient, uncached GetClientWithSecret, and invalidation hooks. |
| internal/services/client_user.go | Invalidates client cache on user-initiated update/delete mutations. |
| internal/services/client_user_test.go | Updates tests for new NewClientService signature (client cache params). |
| internal/services/client_test.go | Updates tests for new NewClientService signature. |
| internal/services/authorization.go | Adds clientService dependency and uses cached/uncached client lookups appropriately. |
| internal/services/authorization_test.go | Updates auth service tests to pass ClientService. |
| internal/handlers/token_introspect_test.go | Updates handler test wiring to include ClientService and new service constructors. |
| internal/handlers/token_client_credentials_test.go | Updates handler test wiring to include ClientService and new service constructors. |
| internal/handlers/token_cache_integration_test.go | Updates integration test wiring for new ClientService dependency. |
| internal/handlers/session_test.go | Updates session test wiring for new ClientService dependency. |
| internal/handlers/registration_test.go | Updates registration test setup for new NewClientService signature. |
| internal/handlers/device_test.go | Updates device handler test wiring for new ClientService dependency. |
| internal/config/config.go | Adds client cache configuration options + validation. |
| internal/config/config_test.go | Extends base valid config to include new client cache fields. |
| internal/bootstrap/services.go | Initializes ClientService with provided client cache and injects it into other services. |
| internal/bootstrap/server.go | Adds shutdown cleanup job helper for the client cache. |
| internal/bootstrap/cache.go | Adds cache initialization for Cache[OAuthApplication] keyed by client_id. |
| internal/bootstrap/bootstrap.go | Stores client cache + closer on the application and hooks shutdown cleanup. |
| .env.example | Documents new CLIENT_CACHE_* env vars and recommended deployment modes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…e errors - Use clientID/hash closure variables instead of key param in GetWithFetch fetchFuncs to avoid using redis-aside prefixed keys for DB lookups - Add cache-error fallback in GetClient to distinguish infrastructure failures from genuine not-found, mirroring getAccessTokenByHash pattern - Apply same prefixed-key fix to getAccessTokenByHash in TokenService Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
fetchThrough already calls the fetch function on any cache Get error, so the explicit fallback path could never execute for cache backend failures. When the DB itself fails, calling it twice is wasteful. Remove the dead fallback and drop the now-unused gorm import. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Restore gorm.ErrRecordNotFound check and DB fallback in GetClient - RueidisAsideCache.GetWithFetch can return an error without calling fetchFunc when Redis/RESP3 is unavailable, so the fallback is needed to avoid treating infrastructure failures as "client not found" - Add tests: secret stripping, cache hit (fetchFunc called once), cache invalidation on UpdateClient and RegenerateSecret Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Break GetWithFetch method signature across multiple lines to satisfy golines formatter
…GetClient - Wrap fetchFunc store errors with clientFetchErr sentinel to prevent redundant DB retry - Return the original store error instead of masking it as ErrClientNotFound - Fix DB fallback to propagate non-ErrRecordNotFound store errors correctly
…Client - fetchFunc always wraps store errors in clientFetchErr, so a raw gorm.ErrRecordNotFound can never reach this branch
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…lient - On cache.ErrInvalidValue (unmarshal failure), delete the bad key before falling back to DB so subsequent requests re-populate the cache correctly instead of hot-looping through the DB fallback on every call
…token cache - Log cache Delete errors on ErrInvalidValue eviction in GetClient (was silently discarded) - Apply same ErrInvalidValue + eviction pattern to TokenService.getAccessTokenByHash to prevent corrupted token cache entries from hot-looping through the DB fallback
…okenCache pattern
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| log.Printf( | ||
| "[ClientCache] Failed to evict corrupted entry for client=%s: %v", | ||
| clientID, | ||
| delErr, | ||
| ) |
There was a problem hiding this comment.
The cache-backend failure path logs on every lookup failure ("[ClientCache] cache lookup failed...") in what is likely a very hot code path. If Redis becomes unavailable, this can flood logs and increase latency. Consider rate-limiting/sampling this log, downgrading it, or recording a metric and logging only periodically.
…ecret - Propagate caller context through cache I/O and DB fallback so that request timeouts/cancellation are respected and tracing can propagate - Handlers pass c.Request.Context(); service callers pass their ctx; methods without a context use context.Background() as a fallback
…teAuthorizationRequest, AuthenticateClient - All three methods called from HTTP handlers but lacked ctx parameter; context.Background() replaced with the actual request context so cancellation/timeout from handlers flows through to cache and DB
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 35 out of 35 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
internal/services/token_client_credentials.go:142
- AuthenticateClient now uses context.Background() for the client lookup. Since this is called from the token endpoint handler, it would be better to accept a context.Context parameter (or otherwise use the request context) so DB/Redis operations can be canceled/timed out with the request.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Adopt core.AuditLogger interface and NewNoopAuditService() from main - Retain ctx propagation and clientCache additions from feat branch - Update test constructors to use NewNoopAuditService() instead of nil Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Preserve non-404 store errors instead of masking them as ErrClientNotFound - Remove unnecessary cache invalidation from CreateClient (new clients are never cached) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 35 out of 35 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Use tokenFetchErr sentinel (parallel to clientFetchErr) so transient DB errors inside GetWithFetch fetchFunc are distinguished from cache-backend failures and short-circuited instead of triggering a redundant DB fallback. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…d fetchErr Both types were identical wrappers used to distinguish store errors from cache-backend errors inside GetWithFetch callbacks. Extract once into errors.go and remove the per-file duplicates. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 36 out of 36 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Cover all validation branches: invalid type, redis/redis-aside without REDIS_ADDR, zero CLIENT_CACHE_TTL, and redis-aside with zero CLIENT_CACHE_CLIENT_TTL. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 36 out of 36 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…tion Prevent callers from accidentally corrupting cached backing arrays via in-place slice mutations. The cached entry now has its own independent StringArray so modifications to the returned value cannot affect the cache. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 36 out of 36 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add ## Client Cache section covering backends, configuration vars, TTL trade-offs, and multi-pod recommendations for CLIENT_CACHE_* settings - Add ## Token Cache section covering the opt-in token verification cache with TOKEN_CACHE_* settings, revocation invalidation, and RESP3 notes - Add both sections to the table of contents - Mention CLIENT_CACHE_TYPE and TOKEN_CACHE_TYPE in README Scalability section
Summary
Cache[OAuthApplication]that caches client lookups byclient_idusing the cache-aside pattern —store.GetClient()was called 20+ times across all OAuth flows (device code, authorization code, token exchange, client credentials), making it the hottest uncached DB query pathGetClient()returns cached copy withClientSecretstripped (defense-in-depth);GetClientWithSecret()bypasses cache for secret-verification flowsClientServiceintoDeviceService,TokenService, andAuthorizationServiceto replace directstore.GetClient()callsCLIENT_CACHE_TYPE(memory/redis/redis-aside),CLIENT_CACHE_TTL(5m default),CLIENT_CACHE_CLIENT_TTL(30s),CLIENT_CACHE_SIZE_PER_CONN(32MB)Test plan
make test— all tests pass (30 files changed, services + handlers + config)make lint— 0 issuesmake fmt— no formatting changesCLIENT_CACHE_TYPE=redis-aside, verify keys created/invalidated in Redis🤖 Generated with Claude Code