feat: add credential cache TTL with active eviction#2
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an opt-in in-memory TTL cache for credential fetch results (limited to 1Password and Bitwarden), wires it into daemon config load/reload, and ensures admin credential checks always fetch live.
Changes:
- Add
proxy.credential_cache_ttlparsing and daemon wiring to configure credential caching. - Implement credential result cache with TTL + active eviction sweeper; add
WithBypassCache()to force live fetches. - Add tests for cache behavior (TTL, bypass, sweeper) and for admin
checkbypassing the cache; update docs to describe the new setting.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/daemon/daemon.go | Wires config-driven cache TTL into daemon startup/reload; admin check uses cache bypass. |
| internal/daemon/daemon_test.go | Extends config reload/run tests to assert cache TTL setter is called. |
| internal/daemon/admin_check_cache_test.go | New test ensuring admin check calls Fetch with WithBypassCache(). |
| internal/credentials/credentials.go | Adds cache lookup/store around Fetch and introduces BypassCache option. |
| internal/credentials/credentials_test.go | Adds unit test for WithBypassCache(). |
| internal/credentials/cache.go | Implements TTL cache, sweep interval selection, and active eviction goroutine. |
| internal/credentials/cache_test.go | New test suite covering caching semantics, TTL expiry, bypass, and sweeper behavior. |
| internal/config/config.go | Adds credential_cache_ttl field + GetCredentialCacheTTL() accessor. |
| internal/config/config_test.go | Tests default + parsing behavior of GetCredentialCacheTTL(). |
| docs/INSTALL.md | Documents new config setting and that claw-wrap check is always live. |
| docs/CONFIG.md | Adds detailed docs for credential_cache_ttl semantics/scope. |
| README.md | Updates examples to clarify check bypasses caching. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/credentials/credentials.go
Outdated
| } | ||
|
|
||
| if cacheEligible && result != "" { | ||
| credentialResultCache.Set(cacheKey, result, now) |
There was a problem hiding this comment.
Credential cache expiry is anchored to now captured before the upstream fetch starts. If the upstream call is slow relative to the TTL, the value can be inserted with an expiry that’s already near/at expiration, causing immediate churn. Consider capturing time separately for the write path (e.g., call credentialCacheNow() again right before Set) so TTL is measured from successful fetch completion, while still using the earlier now for the initial cache lookup if desired.
| credentialResultCache.Set(cacheKey, result, now) | |
| insertNow := credentialCacheNow() | |
| credentialResultCache.Set(cacheKey, result, insertNow) |
internal/credentials/cache.go
Outdated
| stopCh := make(chan struct{}) | ||
| doneCh := make(chan struct{}) | ||
|
|
||
| c.mu.Lock() | ||
| if c.ttl <= 0 || c.sweeperStop != nil { | ||
| c.mu.Unlock() | ||
| return | ||
| } |
There was a problem hiding this comment.
startSweeper allocates stopCh/doneCh before checking whether a sweeper is already running (c.sweeperStop != nil) or TTL is disabled. In races where another goroutine starts the sweeper first, this path returns early and drops those channels, creating avoidable allocations. Consider moving channel creation until after the locked precondition checks pass.
| stopCh := make(chan struct{}) | |
| doneCh := make(chan struct{}) | |
| c.mu.Lock() | |
| if c.ttl <= 0 || c.sweeperStop != nil { | |
| c.mu.Unlock() | |
| return | |
| } | |
| var ( | |
| stopCh chan struct{} | |
| doneCh chan struct{} | |
| ) | |
| c.mu.Lock() | |
| if c.ttl <= 0 || c.sweeperStop != nil { | |
| c.mu.Unlock() | |
| return | |
| } | |
| stopCh = make(chan struct{}) | |
| doneCh = make(chan struct{}) |
- Capture timestamp after fetch completion so TTL reflects actual cache duration - Move channel allocation after lock check to avoid wasted allocs in race
Summary
Testing