Refactor MemoryCache to use ExpiresAt (less goroutines)#10
Conversation
WalkthroughMemoryCache now stores values as cacheItem with expiration timestamps, skips caching when TTL is zero, evicts expired entries on access, and runs a background cleanup goroutine (signaled via a stop channel) that periodically removes expired items. Changes
Sequence DiagramsequenceDiagram
participant Client
participant MemoryCache
participant Clock as time.Now()
participant Cleanup as CleanupGoroutine
Client->>MemoryCache: Set(key, value, ttl)
MemoryCache->>Clock: now
MemoryCache->>MemoryCache: store key -> cacheItem{value, expiresAt}
Client->>MemoryCache: Get(key)
MemoryCache->>Clock: now
alt not expired
MemoryCache-->>Client: return value
else expired
MemoryCache->>MemoryCache: delete key
MemoryCache-->>Client: return ErrCacheMiss
end
loop every 1 minute
Cleanup->>Clock: now
Cleanup->>MemoryCache: cleanup(now)
MemoryCache->>MemoryCache: remove expired items
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @internal/cache/memory.go:
- Around line 89-99: The cleanup method on MemoryCache currently iterates over
c.cache without holding c.mu, causing a data race with concurrent Set calls;
modify cleanup to acquire the write lock (c.mu.Lock()) before iterating over
c.cache and keep it held for the duration of the loop, performing deletes while
locked, then release the lock (c.mu.Unlock()) after finishing to ensure safe
concurrent access.
🧹 Nitpick comments (2)
internal/cache/memory.go (2)
29-38: Consider usingtime.NewTickerinstead oftime.Afterin the loop.Using
time.Afterin a loop creates a new timer on each iteration, which can lead to minor memory overhead. Atime.Tickeris more idiomatic for periodic operations.♻️ Suggested refactor
func NewMemoryClient() *MemoryCache { mc := &MemoryCache{ cache: make(map[string]cacheItem, 100), stop: make(chan struct{}), } go func() { + ticker := time.NewTicker(1 * time.Minute) + defer ticker.Stop() + for { select { case <-mc.stop: return - case <-time.After(1 * time.Minute): + case <-ticker.C: mc.cleanup(time.Now()) } } }() return mc }
78-87: Potential panic ifClose()is called multiple times.Calling
close()on an already-closed channel causes a panic. Consider guarding against double-close if the API permits multiple Close calls.♻️ Suggested fix using sync.Once
Update the struct to include a
sync.Once:type MemoryCache struct { cache map[string]cacheItem mu sync.RWMutex stop chan struct{} + once sync.Once }Then guard the close:
func (c *MemoryCache) Close() error { - close(c.stop) + c.once.Do(func() { + close(c.stop) + }) c.mu.Lock() clear(c.cache) c.mu.Unlock() return nil }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/cache/memory.gointernal/cache/memory_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
internal/cache/memory_test.go (2)
internal/cache/memory.go (2)
NewMemoryClient(23-41)MemoryCache(16-20)internal/cache/cache.go (1)
ErrCacheMiss(10-10)
internal/cache/memory.go (1)
internal/cache/cache.go (1)
ErrCacheMiss(10-10)
🪛 GitHub Actions: Go Build and Publish
internal/cache/memory_test.go
[error] 164-164: TestMemoryCacheTTL: should_remove_a_cache_entry_after_TTL_via_cleanup failed due to race condition.
[error] 177-177: TestMemoryCacheTTL: concurrent operations caused test failure.
internal/cache/memory.go
[warning] 90-90: DATA RACE detected in MemoryCache.cleanup vs Set during TTL caching test.
[warning] 72-72: DATA RACE detected: MemoryCache.Set() concurrently with cleanup.
🔇 Additional comments (9)
internal/cache/memory.go (4)
10-13: LGTM!The
cacheItemstruct cleanly encapsulates the cached value with its expiration timestamp.
16-20: LGTM!The updated
MemoryCachestruct properly tracks the stop channel for graceful shutdown of the cleanup goroutine.
44-62: Potential TOCTOU race in Get between RLock check and Lock deletion.Between releasing the read lock (line 47) and acquiring the write lock (line 54), another goroutine could have already deleted the expired item via cleanup or another Get call. The subsequent
deleteis safe (deleting non-existent key is a no-op), but worth noting for awareness.The implementation is functionally correct for the use case.
102-111: LGTM!The
snapshot()function properly copies the cache under read lock for safe testing.internal/cache/memory_test.go (5)
13-52: LGTM!Well-structured table-driven tests covering both normal caching and TTL=0 skip behavior. The use of
synctest.Testand proper cleanup withdefer c.Close()is good practice.
54-111: LGTM!Good coverage for cache read scenarios including cache hit and miss cases. The
defer c.Close()ensures proper cleanup of the background goroutine.
113-149: LGTM!The close test properly verifies that the cache is cleared after calling
Close().
151-217: Test failures are caused by the data race incleanup(), not test logic.The pipeline failures at lines 164 and 177 are symptoms of the data race in
memory.go'scleanup()function. Once that fix is applied (holding the lock during iteration), these tests should pass.The test logic itself is correct—it properly uses
synctestto control time advancement and validates both cleanup-based expiration and on-Get eviction.
6-6:testing/synctestis an experimental Go 1.24 feature requiringGOEXPERIMENT=synctest.The
testing/synctestpackage requires Go 1.24+ and must be enabled via theGOEXPERIMENT=synctestenvironment variable. Ensure your build and test environment supports this configuration.
nDmitry
force-pushed
the
mem-cache-expires-at
branch
from
5f42f08 to
f45a9a1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @internal/cache/memory_test.go:
- Line 6: Wrap the TestMemoryCacheRead test in synctest.Test the same way as the
other tests: add/ensure the "testing/synctest" import is present and change the
body of TestMemoryCacheRead to call synctest.Test(t, func(t *testing.T) {
...existing test body... }), keeping the test name and signature unchanged and
preserving the existing assertions and setup inside the wrapped function.
In @internal/cache/memory.go:
- Around line 78-87: The Close method on MemoryCache panics if called twice
because it unconditionally closes the stop channel; add a closeOnce sync.Once
field to the MemoryCache struct and use closeOnce.Do(func(){ close(c.stop) })
inside MemoryCache.Close (leave the rest of cleanup logic intact) so the stop
channel is only closed once and subsequent Close calls are no-ops.
🧹 Nitpick comments (4)
internal/cache/memory.go (3)
29-38: Consider usingtime.NewTickerinstead oftime.Afterfor repeated intervals.
time.Afterallocates a new timer on every iteration, which won't be garbage collected until it fires. Atime.Tickeris more efficient for periodic operations.♻️ Proposed fix
func NewMemoryClient() *MemoryCache { mc := &MemoryCache{ cache: make(map[string]cacheItem, 100), stop: make(chan struct{}), } go func() { + ticker := time.NewTicker(1 * time.Minute) + defer ticker.Stop() for { select { case <-mc.stop: return - case <-time.After(1 * time.Minute): + case <-ticker.C: mc.cleanup(time.Now()) } } }() return mc }
44-61: Potential TOCTOU race in expiration check.The item is read under
RLock, then the expiration check and deletion happen after releasing the lock. BetweenRUnlockand acquiringLockfor deletion, another goroutine could have already deleted or updated the key. While the delete is idempotent and won't cause crashes, you may want to re-check existence before deleting for correctness.♻️ Safer pattern with re-check
if item.expiresAt.Before(time.Now()) { c.mu.Lock() - delete(c.cache, key) + // Re-check in case another goroutine already handled it + if current, exists := c.cache[key]; exists && current.expiresAt.Before(time.Now()) { + delete(c.cache, key) + } c.mu.Unlock() return nil, ErrCacheMiss }
89-109: Same TOCTOU pattern as inGet.Keys collected under
RLockcould have been modified by the time you acquireLockfor deletion. This is benign (delete is idempotent), but if a key was re-set with a new expiration between the two locks, you'd incorrectly delete a valid entry.♻️ Safer cleanup with re-check
if len(expiredKeys) > 0 { c.mu.Lock() for _, key := range expiredKeys { - delete(c.cache, key) + // Re-check expiration in case key was updated + if item, exists := c.cache[key]; exists && (item.expiresAt.Before(now) || item.expiresAt.Equal(now)) { + delete(c.cache, key) + } } c.mu.Unlock() }internal/cache/memory_test.go (1)
100-110: Inconsistent use ofsynctest.Testwrapper.
TestMemoryCacheReaddoesn't usesynctest.Testwhile other tests do. For consistency and to ensure the background goroutine behaves deterministically, consider wrapping this test as well.♻️ Suggested change
for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - c := tt.prepare() - defer c.Close() - - v, err := c.Get(ctx, tt.key) - - assert.Equal(t, tt.want, v) - assert.Equal(t, tt.err, err) + synctest.Test(t, func(t *testing.T) { + c := tt.prepare() + defer c.Close() + + v, err := c.Get(ctx, tt.key) + + assert.Equal(t, tt.want, v) + assert.Equal(t, tt.err, err) + }) }) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/cache/memory.gointernal/cache/memory_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
internal/cache/memory.go (1)
internal/cache/cache.go (1)
ErrCacheMiss(10-10)
internal/cache/memory_test.go (2)
internal/cache/memory.go (2)
NewMemoryClient(23-41)MemoryCache(16-20)internal/cache/cache.go (1)
ErrCacheMiss(10-10)
🔇 Additional comments (8)
internal/cache/memory.go (4)
10-13: LGTM!Clean struct design with expiration timestamp for passive expiry checking.
15-20: LGTM!The addition of the
stopchannel provides a clean mechanism to terminate the background cleanup goroutine.
64-76: LGTM!Clean implementation with early return for TTL=0 case. The comment clearly documents the skip behavior.
111-121: LGTM!The snapshot function correctly copies the cache under a read lock for testing purposes.
internal/cache/memory_test.go (4)
36-51: LGTM!Clean table-driven test structure with proper resource cleanup via
defer c.Close().
138-146: LGTM!Good use of
synctest.Testwrapper withrequire.NoErrorfor the close operation and proper assertions on snapshot state.
176-186: LGTM!Proper use of
synctest.Testwithtime.Sleepandsynctest.Wait()to test cleanup behavior deterministically.
190-217: LGTM!Excellent test coverage for the in-Get expiration eviction path. This verifies that expired entries are removed on access before the background cleanup runs.
nDmitry
force-pushed
the
mem-cache-expires-at
branch
from
f45a9a1 to
0be466a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @internal/cache/memory.go:
- Around line 53-59: Get currently checks expiration with
item.expiresAt.Before(time.Now()) while cleanup uses Before || Equal, causing
items expiring exactly at now to be treated differently; update the expiration
check in Get (and any other checks around lines 89-98) to use the same logic as
cleanup — e.g. treat an item as expired when !item.expiresAt.After(now) (or
Before || Equal) — and use a single now := time.Now() within the function to
compare against item.expiresAt, then delete from c.cache and return ErrCacheMiss
as before.
- Around line 78-87: The Close method on MemoryCache currently closes the c.stop
channel directly which will panic if Close is called more than once; modify
MemoryCache to guard the channel close (e.g., add a sync.Once field like
closeOnce or guard with a boolean protected by mu) and invoke that once when
closing c.stop inside MemoryCache.Close, leaving the rest of the cleanup
(locking, clearing c.cache) as-is so repeated Close calls become no-ops rather
than causing a panic.
🧹 Nitpick comments (2)
internal/cache/memory.go (1)
29-38: Consider usingtime.Tickerinstead oftime.Afterin a loop.Using
time.Afterinside a loop creates a new timer channel on each iteration.time.Tickeris more efficient for recurring operations as it reuses the same underlying timer.♻️ Suggested improvement
func NewMemoryClient() *MemoryCache { mc := &MemoryCache{ cache: make(map[string]cacheItem, 100), stop: make(chan struct{}), } go func() { + ticker := time.NewTicker(1 * time.Minute) + defer ticker.Stop() + for { select { case <-mc.stop: return - case <-time.After(1 * time.Minute): + case <-ticker.C: mc.cleanup(time.Now()) } } }() return mc }internal/cache/memory_test.go (1)
144-147: Redundant length assertion.Line 146 checks
len(tt.want) == len(snapshot)and line 147 checks full equality. The full equality check on line 147 already validates length, making line 146 redundant.♻️ Suggested simplification
- assert.Equal(t, len(tt.want), len(snapshot)) assert.Equal(t, tt.want, snapshot)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/cache/memory.gointernal/cache/memory_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
internal/cache/memory.go (1)
internal/cache/cache.go (1)
ErrCacheMiss(10-10)
internal/cache/memory_test.go (2)
internal/cache/memory.go (2)
NewMemoryClient(23-41)MemoryCache(16-20)internal/cache/cache.go (1)
ErrCacheMiss(10-10)
🔇 Additional comments (6)
internal/cache/memory.go (3)
10-13: LGTM!Clean struct definition for cache items with value and expiration timestamp.
64-76: LGTM!Clear TTL=0 handling with appropriate comment. The implementation correctly stores the expiration timestamp.
111-121: LGTM!Proper snapshot implementation with read lock and map copy for safe testing.
internal/cache/memory_test.go (3)
36-51: LGTM!Clean table-driven test structure with proper setup/teardown using
defer c.Close(). The synctest wrapper correctly handles time simulation.
192-219: LGTM!Excellent test coverage for the Get-based expiration eviction path. This verifies that expired entries are removed on access even before the background cleanup runs.
6-6: No action required—thetesting/synctestimport is compatible.The project uses Go 1.25, which is well above Go 1.24 where
testing/synctestwas introduced. The import is valid and standard; noGOEXPERIMENT=synctestflag is needed.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.