Repository navigation
fix(tiering): cache negative policy lookups - #498
Conversation
PolicyStore.Get claimed in a comment to cache nil results but only cached found policies, so every lookup for a database with no custom policy (the common case) hit SQLite — twice per file per migration cycle. Not-found results are now cached as nil in the existing map; key-presence is the sentinel and the (nil, nil) caller contract is unchanged. The cache write is double-checked under the write lock: getFromDB runs outside the lock, so a concurrent Set could otherwise be shadowed by a stale negative that never self-heals. List skips cached negatives — it dereferences every cache value and would panic on the first nil (caught by internal review, with regression test). Set still overwrites the entry and Delete still removes it, so invalidation is unchanged. Tiering is enterprise-gated; OSS unaffected.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request optimizes the tiering policy store by caching negative (not-found) lookups, preventing redundant SQLite queries for databases using global defaults. It also updates the List method to skip these cached negatives and adds comprehensive unit tests to verify the behavior. The reviewer suggested using defer s.mu.Unlock() to handle mutex unlocking more idiomatically and safely.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Behaviorally identical — the function returns immediately after the locked section — but defer survives future return paths.
|
@gemini-code-assist Round-1 finding addressed: the cache-write block in |
There was a problem hiding this comment.
Code Review
This pull request optimizes tiering policy lookups by caching 'not-found' results, preventing redundant SQLite queries for databases without custom policies. It includes a fix for a potential race condition during cache updates and adds robust tests to verify the new caching logic. The reviewer highlighted a potential issue regarding shallow copying of the DatabasePolicy struct, noting that the HotMaxAgeDays pointer field should be deep-copied to ensure cache immutability and prevent data races.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Code Review
This pull request introduces caching for negative (not-found) tiering policy lookups to avoid redundant SQLite queries on every migration cycle. It updates the Get method to cache nil results while handling concurrent race conditions, and modifies List to skip these cached negatives. A review comment suggests optimizing memory allocation in List by counting non-nil policies before pre-allocating the slice, preventing over-allocation when many negative lookups are cached.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…nd 3) len(s.cache) now includes nil not-found entries, often the majority — count non-nil policies first so the capacity hint is exact. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@gemini-code-assist Round-3 finding addressed: |
There was a problem hiding this comment.
Code Review
This pull request implements caching of negative (not-found) tiering policy lookups to avoid redundant SQLite queries. It updates the Get method to cache nil results with safeguards against concurrent write races, adjusts the List method to skip cached negatives, and adds comprehensive unit tests. The reviewer suggested optimizing the List method by performing a single map iteration and pre-allocating the slice capacity to len(s.cache) to avoid the overhead of a double-pass iteration.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // Size the slice from the non-nil count: the cache also holds nil | ||
| // not-found entries (#345), often the majority, and len(s.cache) would | ||
| // over-allocate. Both passes run under the same RLock. | ||
| var count int | ||
| for _, policy := range s.cache { | ||
| if policy != nil { | ||
| count++ | ||
| } | ||
| } | ||
|
|
||
| policies := make([]DatabasePolicy, 0, count) | ||
| for _, policy := range s.cache { | ||
| // Skip cached negatives: nil marks "no custom policy" (#345) and | ||
| // must not be dereferenced or listed. | ||
| if policy == nil { | ||
| continue | ||
| } | ||
| policies = append(policies, *policy) | ||
| } |
There was a problem hiding this comment.
The current implementation of List performs two passes over the s.cache map: first to count the non-nil entries, and second to populate the slice. Map iteration in Go has non-trivial overhead due to bucket traversal and runtime iterator setup.
Since DatabasePolicy is a relatively small struct (~88 bytes), pre-allocating the slice with len(s.cache) as the capacity is highly efficient. It avoids the second map iteration entirely and guarantees that the slice backing array is allocated exactly once without any dynamic resizing overhead.
policies := make([]DatabasePolicy, 0, len(s.cache))
for _, policy := range s.cache {
if policy == nil {
continue
}
policies = append(policies, *policy)
}There was a problem hiding this comment.
Declining — this suggestion reverts exactly what the round-3 review on this PR requested (count first for an exact capacity vs. single pass with len(s.cache)). Both variants are fine: List backs an admin-only endpoint (GET /api/v1/tiering/policies), called rarely, where neither the second map iteration nor the over-allocation is measurable. Oscillating between two unmeasured micro-optimizations is churn; keeping the round-3 version as committed. Happy to revisit with a benchmark if this ever shows up in a profile.
1 participant
Closes #345.
Summary
PolicyStore.Gethad a comment claiming "cache even if nil" but only cached found policies — every lookup for a database with no custom tiering policy (the common case) queried SQLite, twice per file per migration cycle (IsHotOnly+GetEffectivePolicyinMigrator.FindCandidates). Not-found results are now cached asnilin the existing map: key-presence is the sentinel, and the(nil, nil)caller contract is unchanged.getFromDBruns outside the mutex, so the cache write is double-checked under the write lock — a concurrentSetcan no longer be shadowed by a stale negative entry (which, unlike today's uncached negatives, would never self-heal). The racingGetreturns the fresher cached entry.Listskips cached negatives — it dereferences every cache value and would panic on the first nil (internal review blocker, proven with a test before flagging; regression test included).Setoverwrites the entry,Deleteremoves it. Transient DB errors are never cached (Getbails before the cache write on any non-ErrNoRowserror).Delete/Getstale-positive race is out of scope — follow-up issue to be filed.Test plan
go build ./cmd/... ./internal/...,go vet,gofmt -lcleango test -race ./internal/tiering/ -count=1— all PolicyStore tests passTestPolicyStore_NegativeLookupCachedverified to FAIL pre-fix (direct-SQL insert invisible to cached lookup proves SQLite is no longer queried)TestPolicyStore_SetOverridesCachedNegativeguards invalidationTestPolicyStore_ListSkipsCachedNegativesregression test for the List panic +IsHotOnly/GetEffectivenegative-path coverage🤖 Generated with Claude Code