Expand singleflight boundary in ValidatingCache.Get to cover full hit+check and miss+load path#4798
Expand singleflight boundary in ValidatingCache.Get to cover full hit+check and miss+load path#4798
Conversation
There was a problem hiding this comment.
Pull request overview
This PR expands the singleflight boundary in ValidatingCache.Get so that the entire hit+check and miss+load paths are coalesced per key, aiming to reduce duplicate liveness checks / storage round-trips under concurrency.
Changes:
- Move
singleflight.Doto wrap the fullGetoperation (hit validation + miss load). - Simplify the miss-store path by replacing
ContainsOrAdd+ race-handling with a plainAdd, and remove the struct-levelonEvictfield used for discarded-load cleanup. - Update/replace unit tests to reflect new “expired hit falls through to load” behavior and to add a concurrency dedup test.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/cache/validating_cache.go | Wrap full Get in singleflight and simplify the miss store path. |
| pkg/cache/validating_cache_test.go | Update expired-hit expectations and replace a prior race test with a concurrent liveness-check dedup test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4798 +/- ##
==========================================
- Coverage 69.09% 69.06% -0.03%
==========================================
Files 530 530
Lines 55191 55202 +11
==========================================
- Hits 38136 38128 -8
- Misses 14134 14156 +22
+ Partials 2921 2918 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 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 2 out of 2 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 2 out of 2 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 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…+check and miss+load path Move the singleflight.Do boundary to wrap the entire Get operation rather than only the miss+load path. This coalesces concurrent liveness checks for the same key into a single storage.Load round-trip, reducing storage pressure under concurrent access. With only one goroutine running per key at a time: - ContainsOrAdd is replaced with plain Add — the concurrent-writer race between multiple Get goroutines no longer exists - The ContainsOrAdd + Get race-handling block is removed entirely - The onEvict field on the struct is removed (it was only needed to manually call the callback in the discarded-load path) As a side effect of the new design, an expired cache hit now evicts the entry and falls through to load within the same singleflight operation, returning the fresh value instead of (zero, false). Closes #4729
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
Move the singleflight.Do boundary to wrap the entire Get operation rather than only the miss+load path. This coalesces concurrent liveness checks for the same key into a single storage.Load round-trip, reducing storage pressure under concurrent access.
With only one goroutine running per key at a time, concurrent Gets no longer race with each other. However, concurrent
Setcalls still happen outside the singleflight boundary, so the miss path retainsContainsOrAdd(not plainAdd) to preserve writer-wins semantics: if aSetstores a fresher value whileload()is in-flight, that value wins and is returned instead of the loaded one.Behavior change: an expired cache hit now evicts the entry and falls through to
loadwithin the same singleflight operation, returning the fresh value (ok=true) instead of(zero, false). This is a fix: the previous behavior silently dropped a loadable value and forced callers to retry blind.Fixes #4729
Type of change
Test plan
task test)task test-e2e)task lint-fix)Changes
Does this introduce a user-facing change?
Yes —
Geton an expired entry now returns the freshly loaded value (ok=true) instead of(zero, false). Callers that were checkingok=falseto detect expiry and re-callingGetwill now receive the value on the first call.