refactor: use new cache store in services#912
Merged
Conversation
dosubot
Bot
added
the
size:L
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a generic CacheStore (TTL, bounded eviction, WithLock) with unit tests; refactors AuthService to use cache stores for login attempts, OAuth pending sessions, and LDAP group caching; updates lockdown handling and test setup; Makefile gains vet and race test targets. ChangesAuth Service Cache-Based State Management
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/service/auth_service.go (2)
256-297:⚠️ Potential issue | 🟠 Major | ⚡ Quick winData race:
lockdown.activeread without lock at line 262.Same issue as
IsAccountLocked— this read should be protected byauth.lockdown.mu.RLock().Proposed fix
if auth.caches.login.Size() >= MaxLoginAttemptRecords { + auth.lockdown.mu.RLock() + active := auth.lockdown.active + auth.lockdown.mu.RUnlock() - if auth.lockdown.active { + if active { return } go auth.lockdownMode() return }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/service/auth_service.go` around lines 256 - 297, In RecordLoginAttempt you read lockdown.active without synchronization causing a data race; wrap the check of auth.lockdown.active with a read lock (use auth.lockdown.mu.RLock() before the check and defer auth.lockdown.mu.RUnlock() immediately after) so the read is protected, leaving the subsequent goroutine spawn of auth.lockdownMode() unchanged; ensure you reference the lockdown field on the AuthService (auth.lockdown), the RecordLoginAttempt method, and use the existing mutex names (mu, RLock/RUnlock) for consistency.
233-254:⚠️ Potential issue | 🟠 Major | ⚡ Quick winData race:
lockdown.activeandlockdown.untilread without holding the mutex.
lockdownModewrites these fields underauth.lockdown.mu.Lock(), butIsAccountLockedreads them without any lock. This is a data race that can cause torn reads or stale values under concurrent access.Proposed fix
func (auth *AuthService) IsAccountLocked(identifier string) (bool, int) { + auth.lockdown.mu.RLock() + active := auth.lockdown.active + until := auth.lockdown.until + auth.lockdown.mu.RUnlock() + - if auth.lockdown.active { - remaining := int(time.Until(auth.lockdown.until).Seconds()) + if active { + remaining := int(time.Until(until).Seconds()) return true, remaining }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/service/auth_service.go` around lines 233 - 254, IsAccountLocked currently reads auth.lockdown.active and auth.lockdown.until without synchronizing with the mutex used by lockdownMode; acquire the same mutex around those reads to avoid a data race (use auth.lockdown.mu.RLock()/RUnlock() if it's an RWMutex or Lock()/Unlock() if it's a plain Mutex), copy the needed values (active and until) into local variables while holding the lock, release the lock and then compute remaining seconds and return as before; update the code paths in IsAccountLocked that reference auth.lockdown.active and auth.lockdown.until to use the locally copied values.
🧹 Nitpick comments (1)
internal/service/cache_store.go (1)
104-134: 💤 Low value
MutateWithTTLdoesn't handlettl=0consistently withSet.In
Set,ttl > 0is checked before settingexpiresAt. Here,ttl=0causesexpiresAt = time.Now(), making the entry immediately expired. If a mutator returnsttl=0intending "no expiration," this will break.Proposed fix
newValue, ttl, shouldKeep := mutator(entry.value) if !shouldKeep { delete(cs.cache, key) return true } - expiresAt := time.Now().Add(ttl) + var expiresAt *time.Time + if ttl > 0 { + exp := time.Now().Add(ttl) + expiresAt = &exp + } cs.cache[key] = cacheEntry[T]{ value: newValue, - expiresAt: &expiresAt, + expiresAt: expiresAt, }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/service/cache_store.go` around lines 104 - 134, MutateWithTTL currently sets expiresAt unconditionally which makes ttl==0 produce an immediately expired entry; update MutateWithTTL (in type CacheStore[T]) to mirror Set's behavior: only compute and assign expiresAt when ttl > 0 (use time.Now().Add(ttl)); otherwise set expiresAt to nil to represent no expiration, and keep the rest of the logic (deleting on !shouldKeep and locking via cs.mu) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/service/cache_store.go`:
- Around line 146-167: evictOne currently returns without evicting when every
entry has expiresAt == nil (oldestKey stays empty), allowing the cache to exceed
maxSize; update CacheStore.evictOne to fall back to arbitrary eviction: if no
expiring entry was found but cs.cache is non-empty, pick and delete any one key
(e.g., the first key encountered via range) and return true; keep existing
behavior for deleting expired entries and the existing oldest-expiry logic so
evictOne always removes an entry when cache is non-empty.
---
Outside diff comments:
In `@internal/service/auth_service.go`:
- Around line 256-297: In RecordLoginAttempt you read lockdown.active without
synchronization causing a data race; wrap the check of auth.lockdown.active with
a read lock (use auth.lockdown.mu.RLock() before the check and defer
auth.lockdown.mu.RUnlock() immediately after) so the read is protected, leaving
the subsequent goroutine spawn of auth.lockdownMode() unchanged; ensure you
reference the lockdown field on the AuthService (auth.lockdown), the
RecordLoginAttempt method, and use the existing mutex names (mu, RLock/RUnlock)
for consistency.
- Around line 233-254: IsAccountLocked currently reads auth.lockdown.active and
auth.lockdown.until without synchronizing with the mutex used by lockdownMode;
acquire the same mutex around those reads to avoid a data race (use
auth.lockdown.mu.RLock()/RUnlock() if it's an RWMutex or Lock()/Unlock() if it's
a plain Mutex), copy the needed values (active and until) into local variables
while holding the lock, release the lock and then compute remaining seconds and
return as before; update the code paths in IsAccountLocked that reference
auth.lockdown.active and auth.lockdown.until to use the locally copied values.
---
Nitpick comments:
In `@internal/service/cache_store.go`:
- Around line 104-134: MutateWithTTL currently sets expiresAt unconditionally
which makes ttl==0 produce an immediately expired entry; update MutateWithTTL
(in type CacheStore[T]) to mirror Set's behavior: only compute and assign
expiresAt when ttl > 0 (use time.Now().Add(ttl)); otherwise set expiresAt to nil
to represent no expiration, and keep the rest of the logic (deleting on
!shouldKeep and locking via cs.mu) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 084b4f04-f090-443c-8c85-2bcd31dad0df
📒 Files selected for processing (4)
internal/controller/user_controller_test.gointernal/middleware/context_middleware_test.gointernal/service/auth_service.gointernal/service/cache_store.go
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
dosubot
Bot
added
size:XL
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/service/cache_store.go (2)
155-180:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
orderin sync in the TTL-based eviction paths.The expired-entry and
oldestKeybranches delete fromcs.cachedirectly but never remove the evicted key fromcs.order. Once that stale key reaches the front oforder, the fallback branch returnstruewithout freeing a real slot, so bounded stores likeinternal/service/auth_service.go:113can grow past their configured cap.Proposed fix
for k, e := range cs.cache { if e.expiresAt != nil && now.After(*e.expiresAt) { - delete(cs.cache, k) + cs.deleteCallback(k) return true } if e.expiresAt != nil && (oldestExp == nil || e.expiresAt.Before(*oldestExp)) { oldestKey, oldestExp = k, e.expiresAt } } @@ if oldestKey != "" { - delete(cs.cache, oldestKey) + cs.deleteCallback(oldestKey) return true - } else { - if len(cs.order) > 0 { - firstKey := cs.order[0] - cs.order = cs.order[1:] - delete(cs.cache, firstKey) - return true - } } + + for len(cs.order) > 0 { + firstKey := cs.order[0] + cs.order = cs.order[1:] + if _, exists := cs.cache[firstKey]; exists { + delete(cs.cache, firstKey) + return true + } + } return false }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/service/cache_store.go` around lines 155 - 180, In evictOne, the TTL-based branches remove keys from cs.cache but do not remove the same keys from cs.order, causing capacity accounting to break; update the expired-entry branch and the oldestKey branch in the evictOne method to also remove the evicted key from cs.order (e.g., locate the key in cs.order and splice it out) so the in-memory order slice and map remain in sync; reference the evictOne method, the cs.cache map and the cs.order slice when making the change.
81-100:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid appending duplicate keys to
orderon overwrite.
Setalways appendskeyat Line 100, even when the key already exists. That leaves stale duplicates incs.order; after a delete or later fallback eviction,evictOnecan pop a key that's no longer incs.cacheand the next insert pushes the store pastmaxSize.Proposed fix
func (cs *CacheStore[T]) setCallback(key string, value T, ttl time.Duration) { + _, exists := cs.cache[key] if cs.maxSize > 0 { - if _, exists := cs.cache[key]; !exists && len(cs.cache) >= cs.maxSize { + if !exists && len(cs.cache) >= cs.maxSize { cs.evictOne() } } @@ cs.cache[key] = cacheEntry[T]{ value: value, expiresAt: expiresAt, } - cs.order = append(cs.order, key) + if !exists { + cs.order = append(cs.order, key) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/service/cache_store.go` around lines 81 - 100, The setCallback method is appending keys to cs.order unconditionally which allows duplicate stale entries; update CacheStore.setCallback to check if key already exists in cs.cache (or track presence via cs.lookup) and only append to cs.order when inserting a new key (or remove the old position before re-appending), so that cs.order contains each active key once and evictOne will not encounter stale duplicates; reference CacheStore.setCallback, cs.cache, cs.order and evictOne when making the change.
🧹 Nitpick comments (1)
internal/service/cache_store_test.go (1)
230-301: ⚡ Quick winAdd a regression case for stale
orderentries.The current eviction tests only cover the happy-path FIFO/TTL behavior. They don't exercise overwrite/delete or TTL-driven eviction followed by another fallback eviction, which is the path where
orderdesynchronization letsSize()exceedmaxSize.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/service/cache_store_test.go` around lines 230 - 301, Add a regression test to exercise the stale "order" desynchronization path so Size() cannot exceed maxSize: create a new test case in TestCacheStoreEviction (use NewCacheStore, Set, Get, Size) that inserts entries to fill the cache, then causes one entry to expire (short TTL + sleep) and then performs another Set that would trigger fallback eviction; assert the expired key is absent, the cache only contains maxSize keys, and Size() equals the configured max (e.g., 2). Ensure the sequence overwrites/deletes or relies on TTL-driven eviction followed by a normal eviction so any stale ordering state is exercised and cleaned up.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Makefile`:
- Around line 70-72: The Makefile's .PHONY list is missing the target
"test-race", so if a file or directory named test-race appears the recipe won't
run; update the .PHONY declaration to include test-race alongside vet (i.e., add
"test-race" to the .PHONY list) so the test-race target is always treated as a
phony Makefile target and its recipe (go test -race ./...) always executes.
---
Outside diff comments:
In `@internal/service/cache_store.go`:
- Around line 155-180: In evictOne, the TTL-based branches remove keys from
cs.cache but do not remove the same keys from cs.order, causing capacity
accounting to break; update the expired-entry branch and the oldestKey branch in
the evictOne method to also remove the evicted key from cs.order (e.g., locate
the key in cs.order and splice it out) so the in-memory order slice and map
remain in sync; reference the evictOne method, the cs.cache map and the cs.order
slice when making the change.
- Around line 81-100: The setCallback method is appending keys to cs.order
unconditionally which allows duplicate stale entries; update
CacheStore.setCallback to check if key already exists in cs.cache (or track
presence via cs.lookup) and only append to cs.order when inserting a new key (or
remove the old position before re-appending), so that cs.order contains each
active key once and evictOne will not encounter stale duplicates; reference
CacheStore.setCallback, cs.cache, cs.order and evictOne when making the change.
---
Nitpick comments:
In `@internal/service/cache_store_test.go`:
- Around line 230-301: Add a regression test to exercise the stale "order"
desynchronization path so Size() cannot exceed maxSize: create a new test case
in TestCacheStoreEviction (use NewCacheStore, Set, Get, Size) that inserts
entries to fill the cache, then causes one entry to expire (short TTL + sleep)
and then performs another Set that would trigger fallback eviction; assert the
expired key is absent, the cache only contains maxSize keys, and Size() equals
the configured max (e.g., 2). Ensure the sequence overwrites/deletes or relies
on TTL-driven eviction followed by a normal eviction so any stale ordering state
is exercised and cleaned up.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9b621d43-b807-43be-9e17-55720f49ef1d
📒 Files selected for processing (4)
Makefileinternal/service/auth_service.gointernal/service/cache_store.gointernal/service/cache_store_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/service/auth_service.go
Rycochet
approved these changes
May 31, 2026
Member
Author
|
lgtm |
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
2 participants
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 by CodeRabbit