fix(rootmulti): return errors instead of panics for malformed snapshot streams - #4368
blindchaser wants to merge 7 commits into
Conversation
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 #4368 +/- ##
==========================================
- Coverage 67.72% 66.95% -0.77%
==========================================
Files 2171 2071 -100
Lines 168560 159677 -8883
==========================================
- Hits 114149 106919 -7230
+ Misses 54402 52749 -1653
Partials 9 9
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…state-store import error Co-authored-by: Cursor <cursoragent@cursor.com>
blindchaser
changed the title
blindchaser
changed the title
…item-before-store
…item-before-store
blindchaser
changed the title
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
PR SummaryMedium Risk Overview rootmulti validates IAVL nodes are inside a named store section, routes state-store import through memIAVL rejects branch nodes without two pending children, makes New tests cover malformed streams, partial failure rollback, and composite close/abort behavior. Reviewed by Cursor Bugbot for commit 3d1f0f1. Bugbot is set up for automated code reviews on this repo. Configure here. |
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR turns malformed-snapshot and state-store-import failures during restore into errors instead of panics, and the tests cover it well. The new stateStoreImport and TreeImporter shutdown code handles its channels safely: send/Add can't block once the import goroutine has exited, and finish/Close wait for that goroutine before reading its error. I found no issues introduced by this PR.
Findings: 0 blocking | 2 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- None at the file/PR level.
- 2 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
sei-db/db_engine/pebbledb/mvcc/db.go(Import worker, ~L1300-1330) andsei-db/db_engine/rocksdb/mvcc/db.go(~L382) still panic inside worker goroutines on batch create/set/write failures. Those panics skip the newstateStoreImporterror path, so a real storage failure during restore can still stop the node (also raised by Codex). - [suggestion]
sei-cosmos/storev2/rootmulti/store.gorestorealways callsscImporter.Close(), and memIAVL'sMultiTreeImporter.Closethen finalizessnapshot-<h>-tmpintosnapshot-<h>and repointscurrent, even whenrestoreErris already set. Example: a stream that stops early at a store boundary, or an SS send failure. The later SSSetEarliestVersion/SetLatestVersioncalls also run whether or not the restore failed. A failed restore can therefore leave a partial SC snapshot that a later re-offer at the same height adopts instead of rebuilding. More error paths now reach this code, soCloseshould skip finalizing when the restore has failed.
![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: e284b51bc5
ℹ️ 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".
| if err = ssImport.finish(); err != nil && restoreErr == nil { | ||
| restoreErr = err |
There was a problem hiding this comment.
When a malformed stream sets restoreErr, or ssImport.finish() returns an error here, execution still reaches the unconditional SetEarliestVersion and SetLatestVersion calls below. Those calls persist the failed snapshot height even though Restore returns an error, and the state-store import may have committed only a prefix of the snapshot, so a restart can treat incomplete SS data as restored. Gate version initialization on restoreErr == nil at this common completion point.
AGENTS.md reference: AGENTS.md:L115-L119
Useful? React with 👍 / 👎.
…tore Co-authored-by: Cursor <cursoragent@cursor.com>
…item-before-store
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 2875447. Configure here.
Co-authored-by: Cursor <cursoragent@cursor.com>
This branch has not been deployed
1 participant
Summary
Snapshot restore now returns an error, not a panic, for three kinds of malformed stream, and for a failed state-store import.
Restoreruns on the snapshot manager's goroutine, so a panic there stopped the node. With an error, the restore fails and state sync stops cleanly with the cause in the log. A failed restore also publishes nothing: no memIAVL snapshot, no flatkv import, and no state-store versions. Retrying with another snapshot needs a separate change.sei-cosmos/storev2/rootmulti/store.go:restorerejects a node that is not inside a named store section. Before, such a node reached memIAVL'sMultiTreeImporterwith no tree importer open. The state-store import now runs throughstateStoreImport. Its error fails the restore, not the process, andrestorewaits for the import to return before it returns.finishSCImportpublishes the SC import only when the restore succeeded, and aborts it otherwise. The state store's earliest and latest versions are set only on success.sei-db/state_db/sc/types/types.go:ImportergainsAbort, which discards an import without publishing it.sei-db/state_db/sc/memiavl/import.go: the tree importer rejects a branch node that does not have two pending children.TreeImporter.Addstops queueing nodes after the import has stopped, andClosereturns the import's error.MultiTreeImporter.Abortremoves the temp directory without publishing a snapshot or movingcurrent, and a failedCloseremoves it too.sei-db/state_db/sc/composite/importer.go: when the cosmos import fails,Closeaborts the flatkv import rather than publishing it.Abortaborts both.Honest snapshots do not change, so there is no consensus or state impact. Checks on which stores a snapshot contains are not in scope.
Test plan
sei-cosmos/storev2/rootmulti/restore_test.go: undermemiavl_onlyandtest_dual_write, these streams fail the restore with an error and do not panic: a node before any store item, a node after an unnamed store item, and a branch node before its leaves. A state-store import that fails early, or after it reads the full stream, also fails the restore. A failed restore leaves memIAVL's snapshots andcurrentas they were, and leaves the state-store versions unset. A successful restore publishes the snapshot and sets the versions.sei-db/state_db/sc/composite/importer_test.go:Closepublishes flatkv only after the cosmos import succeeds, andAbortaborts both.sei-db/state_db/sc/memiavl/import_test.go: a branch node before its leaves fails the import, andAddreturns after the import has stopped, also when the node channel is full.