Repository navigation
Fix local_cache::free_space skipping the next entry to evict - #7824
Conversation
When the entry at the top of the heap was evicted, free_space() advanced its iterator before std::pop_heap and pop_back. The entry that pop_heap moved to the front was therefore skipped, and the following entries were evicted in heap array order instead of policy order. If the top entry was the only one left, the advanced iterator became invalid and was dereferenced while more space was still needed (undefined behavior, for example when inserting an entry larger than the whole cache). Point the iterator to the new front of the heap after the pop instead. Add regression tests. Fixes TheHPXProject#7823 Signed-off-by: Vaatsalya Srivastava <[email protected]>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Summary
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly and concisely describes the main bug fix in local_cache::free_space. |
| Description check | |
The description explains the iterator bug, the eviction behavior, the regression tests, and the issue scope. It is directly related to the changeset. |
| Linked Issues check | |
Issue #7823 requires local_cache::free_space() to evict entries in update-policy order and avoid invalid iterators. The supplied PR evidence states that free_space calls std::pop_heap and `pop_b… |
| Out of Scope Changes check | |
The reported changes are limited to the local_cache::free_space iterator fix and regression tests in local_lru_cache.cpp. These changes directly support issue #7823. No unrelated production behavi… |
| Docstring Coverage | |
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files. |
Full details: Linked Issues check
-
Explanation
Issue
#7823requireslocal_cache::free_space()to evict entries in update-policy order and avoid invalid iterators. The supplied PR evidence states thatfree_spacecallsstd::pop_heapandpop_back, then resets the iterator to the new heap front. The added tests verify LRU eviction order, including after a touch, and oversized insertion failure without a crash. The PR also preserves the issue's out-of-scope behavior for oversized entries.
Full details: Out of Scope Changes check
-
Explanation
The reported changes are limited to the
local_cache::free_spaceiterator fix and regression tests inlocal_lru_cache.cpp. These changes directly support issue#7823. No unrelated production behavior or unrelated tests are identified.
✨ Finishing Touches: 💡 1
-
🧪 Generate unit tests (beta)
-
- Create a new PR
🛠️ Fix failing CI checks: 💡
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
Up to standards ✅🟢 Issues
|
|
Can one of the admins verify this patch? |
hkaiser
left a comment
hkaiser
left a comment
There was a problem hiding this comment.
Nice catch! LGTM, thanks!
Restore the release notes update that was lost when release-2.0.X was rebuilt on master eaff5b9, and describe the PRs merged since the previous rebuild (#6514, #7767, #7816, #7817, #7819, #7821, #7824, #7831). Regenerate the closed issue and pull request lists for release-2.0.X at 4cb902e against the v1.11.0 tag (775 PRs, 167 issues). Signed-off-by: Hartmut Kaiser <[email protected]>
Fixes #7823
Proposed Changes
local_cache::free_space, point the iterator to the new front of theheap after
std::pop_heap/pop_backinstead of advancing it before thepop. Previously the entry that
pop_heapmoved to the front was skipped,the following evictions followed heap array order instead of policy order,
and an iterator past the end of an emptied heap could be dereferenced.
local_lru_cache.cpp: after shrinking a full LRUcache the least recently used entries are the ones evicted (also after
touching an entry), and inserting an entry larger than the whole cache
fails without crashing.
Any background context you want to provide?
Without this change, shrinking an LRU cache holding keys 0 to 4 (0 oldest) to
two entries keeps keys 1 and 4 instead of 3 and 4, and inserting an entry of
size 5 into a cache of capacity 3 holding one entry crashes (invalid iterator).
Inserting an entry that is larger than the whole cache still evicts all
existing entries before it fails. It might be preferable to reject such an
entry up front; I left that out of this change.
Checklist
Not all points below apply to all pull requests.