fix FlatKV cache memory leak - #4084
Conversation
PR SummaryMedium Risk Overview
New tests cover an end-to-end Reviewed by Cursor Bugbot for commit 3dad3b0. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4084 +/- ##
==========================================
- Coverage 61.23% 60.16% -1.07%
==========================================
Files 2177 2069 -108
Lines 190632 178126 -12506
==========================================
- Hits 116729 107170 -9559
+ Misses 62892 60987 -1905
+ Partials 11011 9969 -1042
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
The channel-binding refactor is sound: entry.valueChan is only read in lookupLocked's statusScheduled branch, and both retire paths move the entry to a terminal status under the same lock, so nil'ing the field cannot strand a waiter or hand a reader a nil channel — while it does release the stale buffered value that caused the leak. One non-blocking note on the new fuzz test's effective coverage.
Findings: 0 blocking | 1 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- None at the file/PR level.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
|
it looks the detach logic is something each caller (multiple callers there not limited to ones we addressed) has to remember rather than an invariant. could we fold all fields into one function under cacheEntry struct ? something like: |
|
Good suggestion, created method (called it |
|
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-4084-to-release/v6.7
git worktree add --checkout .worktree/backport-4084-to-release/v6.7 backport-4084-to-release/v6.7
cd .worktree/backport-4084-to-release/v6.7
git reset --hard HEAD^
git cherry-pick -x f6ef2c3e0066c8996c922f50f50f68082541b298
git push --force-with-lease |
This reverts commit 1258739. The automated backport applied #4084 to sei-db/db_engine/snapshot/read_cache.go. That is not the cache wired into FlatKV on this branch — dbcache is — so the leak went unfixed, and nothing outside the snapshot package references it at all. It also does not build: main's read_cache_test.go was dropped in verbatim, leaving a file declaring `package view` inside the snapshot package, calling a test helper (newTestManagerWithDB) that does not exist here.
Describe your changes and provide context
fix a (slow) memory leak in the FlatKV cache layer