Skip to content

Backport release/v6.7: fix FlatKV cache memory leak - #4095

Open
seidroid[bot] wants to merge 3 commits into
release/v6.7from
backport-4084-to-release/v6.7
Open

Backport release/v6.7: fix FlatKV cache memory leak#4095
seidroid[bot] wants to merge 3 commits into
release/v6.7from
backport-4084-to-release/v6.7

Conversation

@seidroid

@seidroid seidroid Bot commented Sep 3, 2026

Copy link
Copy Markdown

Backport of #4084 to release/v6.7.

@seidroid

seidroid Bot commented Sep 3, 2026

Copy link
Copy Markdown
Author

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

@seidroid seidroid Bot mentioned this pull request Sep 3, 2026
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown

The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedSep 3, 2026, 8:27 PM

@codecov

codecov Bot commented Sep 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.30%. Comparing base (2805764) to head (1e67a51).

Additional details and impacted files

Impacted file tree graph

@@               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     
Flag Coverage Δ
sei-chain-pr 86.03% <100.00%> (?)
sei-db 69.80% <ø> (ø)
sei-db-state-db ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
sei-db/db_engine/dbcache/shard.go 91.38% <100.00%> (+0.47%) ⬆️

... and 100 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@cody-littley cody-littley left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does not fix the right file

Cody Littley added 2 commits September 3, 2026 15:22
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.
@cody-littley
cody-littley marked this pull request as ready for review September 3, 2026 20:24
@cursor

cursor Bot commented Sep 3, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Touches concurrent cache coalescing and LRU accounting in a hot storage path; behavior is well-covered by new race tests but incorrect channel handling could deadlock or mis-account memory.

Overview
Backport of the FlatKV dbcache shard fix that stopped cache entries from retaining per-read valueChan handles (and buffered read results) after keys left the in-flight state.

setTerminalStateUnlocked now centralizes moving an entry to a terminal status, enrolling it in the LRU, and clearing valueChan. Set, delete, injectValue, and bulk inject all use this path so a write or delete during an async read no longer leaves the channel pinned on the entry.

Async completion always signals on the channel captured when the read was scheduled (injectValue / batch pendingRead.valueChan), so waiters are not stranded after the entry detaches its field. Batch scheduling was updated to publish on that bound channel instead of entry.valueChan.

New tests assert the invariant (valueChan non-nil iff statusScheduled) across read completion, Set/Delete during in-flight reads, and batch get/set races.

Reviewed by Cursor Bugbot for commit 1e67a51. Bugbot is set up for automated code reviews on this repo. Configure here.

@cody-littley

Copy link
Copy Markdown
Contributor

@seidroid review

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) and injectValue (shard.go:213) both do delete(s.data, key) while leaving the failed entry at statusScheduled. 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 to gcQueue and the delete does not pop it, the queue keeps a phantom entry whose size counts against maxSize until the key next reaches a terminal state. Guarding with if s.data[key] == entry (or clearing the entry's scheduled status once handled) would close both.

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) and bulkInjectValues (shard.go:365) both do delete(s.data, key) while leaving the failed entry at statusScheduled. 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 to gcQueue and the delete does not pop it, the queue keeps a phantom entry whose size counts against maxSize until the key next reaches a terminal state. Guarding with if s.data[key] == entry (or clearing the entry's scheduled status once handled) would close both.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants