Repository navigation
Backport release/v6.7: fix(rootmulti): return errors instead of panics for malformed snapshot streams - #4435
blindchaser wants to merge 1 commit into
Conversation
…t streams (#4368) ## Summary Snapshot restore now returns an error, not a panic, for three kinds of malformed stream, and for a failed state-store import. `Restore` runs 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`: `restore` rejects a node that is not inside a named store section. Before, such a node reached memIAVL's `MultiTreeImporter` with no tree importer open. The state-store import now runs through `stateStoreImport`. Its error fails the restore, not the process, and `restore` waits for the import to return before it returns. `finishSCImport` publishes 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`: `Importer` gains `Abort`, 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.Add` stops queueing nodes after the import has stopped, and `Close` returns the import's error. `MultiTreeImporter.Abort` removes the temp directory without publishing a snapshot or moving `current`, and a failed `Close` removes it too. - `sei-db/state_db/sc/composite/importer.go`: when the cosmos import fails, `Close` aborts the flatkv import rather than publishing it. `Abort` aborts 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`: under `memiavl_only` and `test_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 and `current` as 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`: `Close` publishes flatkv only after the cosmos import succeeds, and `Abort` aborts both. - `sei-db/state_db/sc/memiavl/import_test.go`: a branch node before its leaves fails the import, and `Add` returns after the import has stopped, also when the node channel is full. --------- Co-authored-by: Cursor <cursoragent@cursor.com> (cherry picked from commit 7b415b7)
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. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
I found nothing that needs changing in this backport of #4368. Malformed snapshot streams now fail restore with an error instead of a panic. A failed restore no longer publishes a memIAVL or flatkv snapshot and no longer records the state store's version height. Tests cover each of these cases. Every Importer implementation in the tree gets the new Abort method.
Findings: 0 blocking | 0 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- None at the file/PR level.
![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: 0a582ac787
ℹ️ 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 si.flatkvImporter != nil { | ||
| return si.flatkvImporter.Close() |
There was a problem hiding this comment.
With the state store disabled and a dual-backend snapshot, a malformed flatKV node (for example, an empty physical value) is recorded as an error by KVImporter.AddNode but is only reported by Close. This ordering publishes the memIAVL snapshot first and then returns the flatKV error, so Restore fails after advancing memIAVL's current; worse, a valid retry at the same height will adopt that existing partial snapshot and discard its newly imported memIAVL data. The flatKV failure must be observed before publishing memIAVL, or both backends must remain staged until both are known to be valid.
Useful? React with 👍 / 👎.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release/v6.7 #4435 +/- ##
================================================
- Coverage 61.49% 60.81% -0.69%
================================================
Files 2168 2093 -75
Lines 189616 181806 -7810
================================================
- Hits 116607 110566 -6041
+ Misses 62232 61106 -1126
+ Partials 10777 10134 -643
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
1 participant
Backport of #4368 to
release/v6.7.Made with Cursor