feat(seidb): add exact-version state store snapshots - #3919
Conversation
Add opt-in Pebble checkpoints for State Store with ordered queue barriers, crash-safe publication, retention, metrics, and SC-aligned cadence. Preserve honest per-database version bounds across snapshot and prune races, including split EVM stores. Co-authored-by: Cursor <cursoragent@cursor.com>
|
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 #3919 +/- ##
==========================================
- Coverage 59.48% 58.60% -0.88%
==========================================
Files 2325 2241 -84
Lines 198647 190011 -8636
==========================================
- Hits 118161 111363 -6798
+ Misses 69257 68067 -1190
+ Partials 11229 10581 -648
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a2ca0794e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // validateEVMSSPostRecovery reports mismatched earliest versions between SS DBs. | ||
| // Divergence is safe because GetEarliestVersion reports the highest member | ||
| // floor, which is the first version every routed store can serve. | ||
| func (s *CompositeStateStore) validateEVMSSPostRecovery() { |
There was a problem hiding this comment.
When recovery finds different member floors—for example, Cosmos at 2 and EVM at 5—this now permits startup and reports 5, but rootmulti.CacheMultiStoreWithVersion(3) does not validate that floor and CompositeStateStore.Get/Has/iterators route directly to the selected backend. A height-3 query can therefore return valid Cosmos data alongside missing EVM data instead of rejecting the unavailable height. Enforce the maximum member floor at the composite read choke points (or retain the startup rejection) so every query below the advertised floor fails consistently.
AGENTS.md reference: AGENTS.md:L92-L96
Useful? React with 👍 / 👎.
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
A large but carefully built, default-off feature: ordered in-queue checkpoint barriers give exact snapshot labels, publication is staged/renamed/symlinked with startup recovery, and the config plumbing follows the repo's characterization conventions. No blocking correctness or security problems found; the notes below are documentation, test-quality, and edge-case robustness items.
Findings: 0 blocking | 9 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
Non-blocking
CosmosStateStore.WaitForPendingWritesandEVMStateStore.WaitForPendingWritesare new exported methods with no production caller — they exist solely forsnapshot_test.go'ssettlehelper. Anexport_test.goshim or an unexported hook would keep the public store surface free of test-only plumbing.sei-cosmos/storev2/rootmultinow importssei-db/state_db/ss/compositeonly to holdvar _ stateStoreSnapshotScheduler = (*sscomposite.CompositeStateStore)(nil), coupling the Cosmos store layer to a concrete SS implementation. The same build-time protection could live in packagess(or as a method onseidbtypes.StateStore), keeping the dependency one-directional.- The trigger sits in
flush(), which is reached first fromGetWorkingHash()— i.e. beforeCommit. A boundary snapshot can therefore be published for a height the node has not yet committed (SS is already ahead in that window, so the image is self-consistent). Worth confirming this is acceptable for the rollback restore model, since exact labelling is the point of the barrier. - 5 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
sei-cosmos/storev2/rootmulti/store.goempty-block path (~line 276) callsssStore.SetLatestVersion(currentVersion)synchronously while older changesets may still sit in the async apply queue.ApplyChangesetSyncpersists the latest-version marker inside its own batch (NewBatch(..., version, ...)), so a later-applied older batch can overwrite the on-disk marker and regress the SS watermark across a crash/restart.
|
|
||
| earliestVersion := version + 1 // we increment by 1 to include the provided version | ||
| skipBelow := db.GetEarliestVersion() | ||
| if err := db.advanceEarliestVersion(earliestVersion); err != nil { |
There was a problem hiding this comment.
[suggestion] advanceEarliestVersion runs unconditionally at the top of the pass, so a pass that fails still leaves the marker at version+1. That value feeds state.Store.VersionExists and WatermarkManager.stateEarliest, so a prune that keeps failing (bad key, I/O error, compaction error from compactPrunedRange) narrows the served history window once per prune interval while nothing is deleted and disk keeps growing — and pruneIncomplete is process-local, so a restart also loses the rescan hint. The ordering is deliberate and correct for checkpoint honesty; the gap is observability. Consider a counter or escalating log for consecutive prune failures so this is visible before the window collapses (the NOTE above documents only the on-disk leak, not the shrinking readable range).
| // link semantics should change with that work too: this implementation points to | ||
| // the newest published snapshot, while FlatKV's current link points to the | ||
| // active snapshot that open/rollback clones and replays from. | ||
| const ( |
There was a problem hiding this comment.
[suggestion] This ~80-line design narrative is directly adjacent to const (, so godoc attaches all of it to the const block and hence to the exported SnapshotsDirName. It also records design history and roadmap ("the planned per-SS restructure", "The agreed direction is…", "SS rollback is not implemented in this feature", "The current link semantics should change with that work"), which AGENTS.md's godoc rules explicitly exclude. Suggest moving the overview to a package comment (e.g. a doc.go) or at minimum separating it from const ( with a blank line, and reducing the roadmap paragraphs to inline comments at the lines they actually constrain.
|
|
||
| // Snapshots can finish out of order, so only move the link forward. | ||
| if version > m.lastPublished { | ||
| if err := m.updateCurrentLink(SnapshotDirName(version)); err != nil { |
There was a problem hiding this comment.
[suggestion] If updateCurrentLink fails here, publish returns false but the deferred m.prune() still runs, and with a small keepRecent it can delete the snapshot current is still pointing at. The result is a dangling symlink, not merely the stale one the comment describes — a consumer following current gets ENOENT on a removed directory. Same shape at startup, where prune() runs before the updateCurrentLink(lastPublished) restore. Consider removing current (or retrying the swap) when the update fails, so followers see "no current snapshot" instead of a broken path.
| return fmt.Errorf("create snapshot root %q: %w", root, err) | ||
| } | ||
| for _, sourceDir := range sourceDirs { | ||
| probe, err := os.CreateTemp(sourceDir, ".ss-snapshot-link-probe-*") |
There was a problem hiding this comment.
[nit] The probe is created inside the live Pebble data directory and hardlinked into the snapshot root. If the process dies between os.Link and the two os.Remove calls, .ss-snapshot-link-probe-* files are left behind in both places, and removeStaleTmpDirs only sweeps tmp- directories and current-tmp. A fixed probe name, or a sweep for the probe prefix in removeStaleTmpDirs, would make the preflight self-healing across a crash.
| err := cs.validateEVMSSPostRecovery() | ||
| require.Error(t, err) | ||
| require.Contains(t, err.Error(), "earliest version") | ||
| cs.validateEVMSSPostRecovery() |
There was a problem hiding this comment.
[suggestion] With the error return removed, this test now makes three bare calls with no assertions, so it can only fail by panicking, while its name still promises it verifies mismatch handling. Either drop it in favour of TestCompositeGetEarliestVersionReportsHighestMemberFloor, or make it assert the new contract — that the mismatch is tolerated and cs.GetEarliestVersion() reports 75.
1 participant
Summary
Add opt-in, exact-version Pebble checkpoints for State Store. This PR contains snapshot creation and retention only; SS rollback remains separate and will use these checkpoints as state-WAL replay inputs.
currentsymlink, with startup recovery and retention.Snapshots require Pebble and hardlinks. All enabled Cosmos and EVM SS databases must share a filesystem with the snapshot root. SS snapshots are internal rollback restore points, not state-sync or archive inputs.
Test plan
go test -race -count=1 ./sei-db/db_engine/pebbledb/... ./sei-db/config/... ./sei-db/state_db/ss/... ./sei-cosmos/storev2/rootmulti/... ./app/... ./sei-cosmos/server/config/...golangci-lint run ./sei-db/... ./app/... ./sei-cosmos/storev2/... ./sei-cosmos/server/config/...Made with Cursor