Backport release/v6.7: fix FlatKV cache memory leak - #4095
Conversation
|
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 |
|
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 @@
## release/v6.7 #4095 +/- ##
================================================
- Coverage 61.34% 60.30% -1.04%
================================================
Files 2163 2064 -99
Lines 188795 177071 -11724
================================================
- Hits 115812 106787 -9025
+ Misses 62264 60508 -1756
+ Partials 10719 9776 -943
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
cody-littley
left a comment
There was a problem hiding this comment.
does not fix the right file
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.
PR SummaryMedium Risk Overview
Async completion always signals on the channel captured when the read was scheduled ( New tests assert the invariant ( Reviewed by Cursor Bugbot for commit 1e67a51. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
@seidroid review |
There was a problem hiding this comment.
Clean backport of the FlatKV cache valueChan retention fix: terminal transitions are funneled through setTerminalStateUnlocked, which detaches the channel, and both off-lock publishers now use the channel reference bound at scheduling time, so no waiter can be stranded. No PR-introduced defects; one pre-existing size-accounting/identity issue on the error path was noticed while tracing the changed code.
Findings: 0 blocking | 1 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- None at the file/PR level.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion] The read-error cleanup paths delete a cache key without checking entry identity:
bulkInjectValues(sei-db/db_engine/dbcache/shard.go:362) andinjectValue(shard.go:213) both dodelete(s.data, key)while leaving the failed entry atstatusScheduled. Two BatchGets sharing one failed read therefore each run cleanup for the same entry; if a retry populates a replacement entry in between, the second cleanup deletes that valid replacement. Because the replacement was already pushed togcQueueand the delete does not pop it, the queue keeps a phantom entry whose size counts againstmaxSizeuntil the key next reaches a terminal state. Guarding withif s.data[key] == entry(or clearing the entry's scheduled status once handled) would close both.
There was a problem hiding this comment.
Faithful adaptation of the #4084 valueChan retention fix to the older release/v6.7 shard code: terminal transitions are funneled through setTerminalStateUnlocked, which detaches the channel, and both off-lock publishers now use the channel reference bound at scheduling time, so no waiter can be stranded. No PR-introduced defects; one pre-existing identity issue on the read-error cleanup path was noticed while tracing the changed code.
Findings: 0 blocking | 1 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- None at the file/PR level.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion] The read-error cleanup paths delete a cache key without checking entry identity:
injectValue(sei-db/db_engine/dbcache/shard.go:216) andbulkInjectValues(shard.go:365) both dodelete(s.data, key)while leaving the failed entry atstatusScheduled. Two BatchGets sharing one failed read therefore each run cleanup for the same entry; if a retry populates a replacement entry in between, the second cleanup deletes that valid replacement. Because the replacement was already pushed togcQueueand the delete does not pop it, the queue keeps a phantom entry whose size counts againstmaxSizeuntil the key next reaches a terminal state. Guarding withif s.data[key] == entry(or clearing the entry's scheduled status once handled) would close both.
Backport of #4084 to
release/v6.7.