perf(cache): add singleflight, memory reaper, and context propagation#156
perf(cache): add singleflight, memory reaper, and context propagation#156
Conversation
Three cache infrastructure improvements: 1. Stampede protection (singleflight): MemoryCache and RueidisCache now use golang.org/x/sync/singleflight to deduplicate concurrent fetches for the same key. Prevents N parallel DB queries on cache miss under high concurrency. RueidisAsideCache already has built-in protection. 2. MemoryCache periodic cleanup: A background reaper goroutine evicts expired entries every 5 minutes (configurable). Previously, items that were Set but never Get again would accumulate indefinitely. The reaper is stopped on Close(). 3. UserService.GetUserByID context propagation: Changed signature from GetUserByID(id) to GetUserByID(ctx, id) so request cancellation propagates to cache operations. All callers updated to pass c.Request.Context(). 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 improves cache performance and reliability by adding stampede protection to cache-aside operations, introducing periodic cleanup for in-memory cache entries, and propagating request contexts into user cache lookups so cancellations/timeouts can affect cache I/O.
Changes:
- Add
singleflight-based deduplication inMemoryCacheandRueidisCacheGetWithFetchto prevent thundering herds on misses. - Add a background “reaper” goroutine to
MemoryCacheto periodically evict expired entries. - Update
UserService.GetUserByIDto acceptcontext.Contextand pass request context from handlers/middleware/tests.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/services/user.go | Changes GetUserByID to accept ctx and passes it into cache operations. |
| internal/services/user_test.go | Updates tests to call GetUserByID with a context. |
| internal/middleware/auth.go | Propagates request context into GetUserByID calls. |
| internal/handlers/oidc.go | Propagates request context into GetUserByID calls. |
| internal/handlers/device.go | Propagates request context into GetUserByID calls. |
| internal/handlers/authorization.go | Propagates request context into GetUserByID calls. |
| internal/cache/util.go | Removes the shared fetchThrough helper (logic inlined into cache implementations). |
| internal/cache/rueidis.go | Adds singleflight to dedupe concurrent fetches per key. |
| internal/cache/memory.go | Adds singleflight, a background reaper, and an optional cleanup interval parameter. |
| go.mod | Adds golang.org/x/sync as a direct dependency for singleflight. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…light fetches - Switch singleflight.Do to DoChan in MemoryCache and RueidisCache - Use context.WithoutCancel for shared fetch so one caller cancellation does not fail all concurrent waiters for the same key - Add per-caller select on ctx.Done to allow early return without affecting the shared fetch goroutine - Guard MemoryCache.Close with sync.Once to prevent panic on concurrent calls
- Move sharedCtx creation inside DoChan closure in rueidis.go to match memory.go - Unify singleflight comment wording across MemoryCache and RueidisCache - Remove self-evident inline comments from GetWithFetch fast path - Fix Len() doc comment to remove "(for testing)" annotation Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Allow passing a non-positive cleanup interval to NewMemoryCache to disable the background reaper and rely solely on lazy expiration - Add TestMemoryCache_Reaper to verify the reaper evicts expired entries - Add TestMemoryCache_NoReaper to verify lazy expiration still works when the reaper is disabled Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Re-apply the caller deadline to the detached singleflight context so the shared fetch cannot run unbounded when callers have timeouts - Replace fixed time.Sleep in TestMemoryCache_Reaper with a polling loop bounded by a 200ms timeout to avoid flakiness on slow CI runners Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Replace time.After+time.Sleep with time.NewTimer/time.NewTicker (defer Stop) in TestMemoryCache_Reaper to prevent goroutine/timer leaks - Add TestMemoryCache_GetWithFetch_Singleflight asserting fetchFunc is called exactly once under concurrent load
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
internal/services/user.go:295
- fetchFn’s parameters (ctx, key) are unused; consider naming them '_' to make it explicit they’re intentionally ignored, or use ctx if/when the store layer supports context-aware queries. This helps avoid confusion about whether cancellation/deadlines are expected to affect the DB lookup.
cacheKey := "user:" + id
fetchFn := func(ctx context.Context, key string) (models.User, error) {
u, err := s.store.GetUserByID(id)
if err != nil {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Extract duplicated singleflight+context logic into doWithSingleflight in util.go - Drop deadline propagation so no caller timeout governs the shared fetch - Add defer c.Close() to three GetWithFetch tests to stop reaper goroutines - Fix misleading test comment: only one goroutine enters fetchFunc with singleflight
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 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 checked type assertion in doWithSingleflight to avoid panic on unexpected type - Clarify comment: context.WithoutCancel strips both cancel and deadline intentionally so no single caller's timeout can abort the shared fetch for other waiters
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…uirement - Add early ctx.Done() check in doWithSingleflight to skip work for already-cancelled callers before enqueueing into singleflight - Clarify NewMemoryCache doc: callers must call Close when reaper is enabled
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…, services, handlers tests
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 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.
Summary
MemoryCacheandRueidisCachenow usegolang.org/x/sync/singleflightto deduplicate concurrent fetches for the same key, preventing N parallel DB queries on cache miss under high concurrency.RueidisAsideCachealready has built-in protection viarueidisaside.Get().Setbut neverGetagain would accumulate indefinitely, causing a slow memory leak in long-running servers.UserService.GetUserByID(id)changed toGetUserByID(ctx, id)so request cancellation propagates to cache operations. All callers updated to passc.Request.Context().Test plan
make test— all tests passmake lint— 0 issuesmake fmt— no formatting changes🤖 Generated with Claude Code