Skip to content

Fix Rocksdb MVCC read timestamp lifetime for iterators#2971

Merged
masih merged 6 commits intomainfrom
masih/rocksdb-test-flake-less
Feb 24, 2026
Merged

Fix Rocksdb MVCC read timestamp lifetime for iterators#2971
masih merged 6 commits intomainfrom
masih/rocksdb-test-flake-less

Conversation

@masih
Copy link
Collaborator

@masih masih commented Feb 24, 2026

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.

This should also fix the flaky rocksdb tests, flaked most recently on main among other PRs with unrelated changes.

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.
@github-actions
Copy link

github-actions bot commented Feb 24, 2026

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

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedFeb 24, 2026, 7:22 PM

@codecov
Copy link

codecov bot commented Feb 24, 2026

Codecov Report

❌ Patch coverage is 88.52459% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.68%. Comparing base (1b1e9a2) to head (1b881dd).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
sei-db/db_engine/rocksdb/mvcc/db.go 63.15% 7 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             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     
Flag Coverage Δ
sei-chain 74.68% <100.00%> (+16.56%) ⬆️
sei-db 69.50% <85.10%> (+1.01%) ⬆️

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

Files with missing lines Coverage Δ
sei-db/db_engine/pebbledb/mvcc/iterator.go 56.01% <100.00%> (+0.41%) ⬆️
sei-db/db_engine/rocksdb/mvcc/iterator.go 86.86% <100.00%> (+1.31%) ⬆️
sei-db/state_db/ss/test/storage_test_suite.go 99.29% <100.00%> (ø)
sei-db/db_engine/rocksdb/mvcc/db.go 58.40% <63.15%> (+0.91%) ⬆️

... and 2095 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.

@masih masih enabled auto-merge (squash) February 24, 2026 19:33
@masih masih merged commit aa8885f into main Feb 24, 2026
35 checks passed
@masih masih deleted the masih/rocksdb-test-flake-less branch February 24, 2026 19:35
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>
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.

5 participants