Repository navigation
Guard FlatKV rollback floor - #4496
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
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 FlatKV centralizes that logic in Composite blocks rollback (and enriches version-reconcile rollback errors) when FlatKV cannot reach the target, with guidance to use state sync; on Auto New Reviewed by Cursor Bugbot for commit f5c8937. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
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 #4496 +/- ##
=======================================
Coverage 57.10% 57.10%
=======================================
Files 2127 2127
Lines 166451 166488 +37
=======================================
+ Hits 95054 95076 +22
- Misses 71392 71407 +15
Partials 5 5
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: 94102864e1
ℹ️ 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".
| // CheckVersionReachable verifies that targetVersion can be reconstructed from this store's retained | ||
| // snapshots and WAL. It does not modify snapshot, symlink, database, or WAL state. | ||
| func (s *CommitStore) CheckVersionReachable(targetVersion int64) error { | ||
| _, err := s.reachableBaseVersion(s.flatkvDir(), targetVersion) |
There was a problem hiding this comment.
CheckVersionReachable calls seekSnapshot directly, outside the SnapshotWriter goroutine, while live stores may publish and prune snapshots concurrently. As documented by SnapshotWriter.CloneSnapshot in snapshot_writer.go, resolving a snapshot on the caller thread allows pruning to delete it between resolution and copying; the new preflight can therefore approve a snapshot that disappears before LoadVersionReadOnly clones it, causing historical reads and exports of an otherwise reachable version to fail intermittently. Route the online reachability check through the writer-serialized snapshot path rather than inspecting the snapshot directory independently.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is probably worth doing.
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Adds a read-only FlatKV reachability preflight (ErrVersionUnreachable, CheckVersionReachable) so composite rollback and historical read-only loads are refused before anything is mutated, and deletes stale idle Auto-mode FlatKV directories on load. It is blocked because the new discard path also catches a crash between the memIAVL and FlatKV commits of the kickoff block, which the old reconcile handled correctly; the only other reading (codex) found nothing, so it added no findings and this blocker contradicts it.
Blocking
1 finding on the changed lines, as inline comments.
- No test covers a torn commit on the kickoff block in Auto mode: memIAVL committed at K, FlatKV left at its seed K-1. That is the crash-recovery case the new discardStaleIdleFlatKV path changes. The new crash_in_kickoff_block and stale-idle tests only close the store before either backend commits, so they never reach the case that regresses.
Non-blocking
1 finding on the changed lines, as inline comments.
seidroid review · decision request_changes · session 2fbdda8e80564efeab67728e5074dd55 · turn resp_claude_9e39aab20adaf8349e2956497b81eed2 · item 9625cf3d8a2158b793096826a9187b0f
Findings: 2 blocking | 1 non-blocking | 2 posted inline
…unreachable Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Since my last review, the discard of an idle Auto-mode FlatKV is limited by isStaleSeed to seeds above memIAVL or more than one block behind it, so a torn kickoff commit now goes to reconcileVersions, and a new test replays block K to the canonical AppHash; seekSnapshot now tags only a missing snapshot as ErrVersionUnreachable, which fixes both of my earlier findings. Nothing blocks; codex again found nothing and contributed no findings.
Non-blocking
- The stale-seed cleanup runs only in
LoadLatest.materializeFlatKVstill adopts an existing non-zero idle seed without checking it against memIAVL, so a kickoff that fires in-process after the node has committed past an idle seed would still hit the commit version mismatch. The kickoff trigger is deterministic chain state, so this needs an unusual sequence, but a matching guard there would close the remaining path.
seidroid review · decision approve · session 2fbdda8e80564efeab67728e5074dd55 · turn resp_claude_ee6e2929a6b2cefc751bd2f4538c2a69 · item 5437a4fa02c25eb59b8913d6a1a4102d
Findings: 0 blocking | 1 non-blocking | 0 posted inline
Superseded: the latest review found nothing blocking in this change.
The LiveStateStore interface now owns the CheckVersionReachable error contract. Shared reachability messages no longer mention rollback, and the tests use require.ErrorIs, a named subtest table and a noKickoff constant. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-4496-to-release/v6.7
git worktree add --checkout .worktree/backport-4496-to-release/v6.7 backport-4496-to-release/v6.7
cd .worktree/backport-4496-to-release/v6.7
git reset --hard HEAD^
git cherry-pick -x 8b5e550e9c5de26a0e0907f9bcc422e446cdc3a3
git push --force-with-lease |
Guard FlatKV's retained-history floor before composite rollback mutates any backend, and treat stale idle FlatKV directories as disposable Auto-mode seeds instead of reconcilable state. - `sei-db/state_db/sc/flatkv/snapshot.go`: adds a single read-only reachability check for snapshots plus WAL. Rollback still uses the same check before any mutation. Only "no snapshot at or below the target" is tagged `ErrVersionUnreachable`; filesystem errors from the snapshot scan pass through untagged. - `sei-db/state_db/sc/flatkv/store.go`: rejects unreachable historical read-only loads before creating a `readonly-*` working directory. - `sei-db/state_db/giga/types/live_state_store.go`: exposes `CheckVersionReachable` so composite rollback can preflight the FlatKV backend, and owns its error contract, `ErrVersionUnreachable` (the only non-fatal error of the interface). FlatKV keeps `flatkv.ErrVersionUnreachable` as an alias. - `sei-db/state_db/sc/composite/store.go`: refuses rollback targets FlatKV cannot reach before memIAVL moves, and points operators to state sync. Before seeding or reconciliation, it removes an idle Auto-mode FlatKV dir whose seed is above memIAVL or more than one block below it. An idle seed exactly one block behind is kept for `reconcileVersions`, because that is what a crash between the memIAVL and FlatKV commits of the kickoff block leaves. - `sei-db/state_db/sc/composite/store_test.go`: updates the test fake for the new interface. - `sei-db/state_db/sc/flatkv/store_test.go`: updates the beyond-WAL read-only test for the new early reachability failure. - `sei-db/state_db/sc/composite/rollback_floor_test.go`: covers rollback to K-2 refusal without mutation, historical read/export below the floor returning errors without panic, rollback to K-1 followed by re-kickoff, delayed kickoff after a stale idle FlatKV dir, a torn kickoff commit (memIAVL at K, FlatKV seed at K-1) that must roll memIAVL back and replay K to the canonical AppHash, and state sync from a pre-kickoff snapshot with replay through migration. - `sei-db/state_db/sc/flatkv/snapshot_test.go`: an empty snapshot dir is `ErrVersionUnreachable`; a snapshot-scan read failure is not. - `sei-db/state_db/sc/flatkv/store_replay_test.go`: covers snapshot-only reachability, snapshot-plus-WAL reachability, below-history errors, and failing read-only loads before creating a read-only work dir. - `go test ./sei-db/state_db/sc/flatkv ./sei-db/state_db/sc/composite` - `make fmtcheck` - `make dblint` - `go test -race ./sei-db/state_db/sc/composite/... ./sei-db/state_db/sc/flatkv/... ./sei-db/state_db/giga/... ./sei-db/bootstrap/...` - `scripts/ramtest.sh ./sei-db/...` failed once in unrelated `TestRapidFlushes` with "Expected at most 25 flushes, got 32"; rerunning `scripts/ramtest.sh ./sei-db/db_engine/litt/disktable` passed. --------- Co-authored-by: Cursor <cursoragent@cursor.com> (cherry picked from commit 8b5e550)
Backport of #4496 to `release/v6.7`. Co-authored-by: yirenz <blindchaser@users.noreply.github.com> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: alexander-sei <alexanderh@seinetwork.io>
3 participants
Summary
Guard FlatKV's retained-history floor before composite rollback mutates any backend, and treat stale idle FlatKV directories as disposable Auto-mode seeds instead of reconcilable state.
sei-db/state_db/sc/flatkv/snapshot.go: adds a single read-only reachability check for snapshots plus WAL. Rollback still uses the same check before any mutation. Only "no snapshot at or below the target" is taggedErrVersionUnreachable; filesystem errors from the snapshot scan pass through untagged.sei-db/state_db/sc/flatkv/store.go: rejects unreachable historical read-only loads before creating areadonly-*working directory.sei-db/state_db/giga/types/live_state_store.go: exposesCheckVersionReachableso composite rollback can preflight the FlatKV backend, and owns its error contract,ErrVersionUnreachable(the only non-fatal error of the interface). FlatKV keepsflatkv.ErrVersionUnreachableas an alias.sei-db/state_db/sc/composite/store.go: refuses rollback targets FlatKV cannot reach before memIAVL moves, and points operators to state sync. Before seeding or reconciliation, it removes an idle Auto-mode FlatKV dir whose seed is above memIAVL or more than one block below it. An idle seed exactly one block behind is kept forreconcileVersions, because that is what a crash between the memIAVL and FlatKV commits of the kickoff block leaves.sei-db/state_db/sc/composite/store_test.go: updates the test fake for the new interface.sei-db/state_db/sc/flatkv/store_test.go: updates the beyond-WAL read-only test for the new early reachability failure.Test plan
sei-db/state_db/sc/composite/rollback_floor_test.go: covers rollback to K-2 refusal without mutation, historical read/export below the floor returning errors without panic, rollback to K-1 followed by re-kickoff, delayed kickoff after a stale idle FlatKV dir, a torn kickoff commit (memIAVL at K, FlatKV seed at K-1) that must roll memIAVL back and replay K to the canonical AppHash, and state sync from a pre-kickoff snapshot with replay through migration.sei-db/state_db/sc/flatkv/snapshot_test.go: an empty snapshot dir isErrVersionUnreachable; a snapshot-scan read failure is not.sei-db/state_db/sc/flatkv/store_replay_test.go: covers snapshot-only reachability, snapshot-plus-WAL reachability, below-history errors, and failing read-only loads before creating a read-only work dir.go test ./sei-db/state_db/sc/flatkv ./sei-db/state_db/sc/compositemake fmtcheckmake dblintgo test -race ./sei-db/state_db/sc/composite/... ./sei-db/state_db/sc/flatkv/... ./sei-db/state_db/giga/... ./sei-db/bootstrap/...scripts/ramtest.sh ./sei-db/...failed once in unrelatedTestRapidFlusheswith "Expected at most 25 flushes, got 32"; rerunningscripts/ramtest.sh ./sei-db/db_engine/litt/disktablepassed.