Fix Rocksdb MVCC read timestamp lifetime for iterators#2971
Merged
Conversation
RocksDB ReadOptions timestamp handling in MVCC reads had unsafe lifetime semantics: * newTSReadOptions used a local [8]byte timestamp and passed it to SetTimestamp. * Under concurrent iterator creation, the timestamp backing memory could be reused, causing versioned reads to drift to the wrong version. * Iterator read options were not owned/cleaned up with iterator lifetime, making behavior fragile and cleanup inconsistent. The changes here use heap-backed timestamp slices (`make([]byte, TimestampSize)`) when calling `SetTimestamp` / `SetIterStartTimestamp`, and create `ReadOptions` once per iterator in `Iterator` and `ReverseIterator`, passing them into the iterator wrapper. The `mvcc.iterator` is extended to own `*grocksdb.ReadOptions` and destroy them in `Close()`, while `getSlice` is updated to destroy per-read `ReadOptions` after `GetCF`. `RawIterate` is also updated to pass owned read options into the iterator lifecycle. As a minor cleanup, `Next()` is made a pointer receiver for consistency.
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2971 +/- ##
===========================================
+ Coverage 58.14% 73.68% +15.54%
===========================================
Files 2107 13 -2094
Lines 173322 2440 -170882
===========================================
- Hits 100780 1798 -98982
+ Misses 63602 522 -63080
+ Partials 8940 120 -8820
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
bdchatham
approved these changes
Feb 24, 2026
Kbhat1
approved these changes
Feb 24, 2026
jewei1997
approved these changes
Feb 24, 2026
bdchatham
approved these changes
Feb 24, 2026
yzang2019
approved these changes
Feb 24, 2026
Kbhat1
added a commit
that referenced
this pull request
Feb 24, 2026
Apply the same fix from #2971 to the EVM RocksDB backend: assign ReadOptions to a local, defer Destroy(), and use heap-allocated timestamp slices so the backing array outlives the stack frame. Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
RocksDB ReadOptions timestamp handling in MVCC reads had unsafe lifetime semantics:
The changes here use heap-backed timestamp slices
(
make([]byte, TimestampSize)) when callingSetTimestamp/SetIterStartTimestamp, and createReadOptionsonce per iterator inIteratorandReverseIterator, passing them into the iterator wrapper. Themvcc.iteratoris extended to own*grocksdb.ReadOptionsand destroy them inClose(), whilegetSliceis updated to destroy per-readReadOptionsafterGetCF.RawIterateis also updated to pass owned read options into the iterator lifecycle. As a minor cleanup,Next()is made a pointer receiver for consistency.This should also fix the flaky rocksdb tests, flaked most recently on main among other PRs with unrelated changes.