Backport release/v6.7: fix(seidb): fix stale FlatKV migration gauges on snapshotting nodes - #4442
Conversation
…4436) On atlantic-2, archive-0-0-0, snapshotter-0 and state-sync-node-0 finished the EVM migration (all 123,876,555 keys moved, `seidb_migration_version` went to 1), but the FlatKV migration dashboard still shows them as migrating, at 99.3% with ~875k EVM keys left in memIAVL. The migration is complete; two gauges report stale values, and only on nodes that export state-sync snapshots. A snapshot export opens a read-only composite store at the snapshot height, and when that height is before completion, the `MigrationManager` built for that handle records version 0 on the process-wide `seidb_migration_version` gauge. Nothing records 1 again until restart. Separately, `rootmulti.Store.Snapshot` records `iavl_total_num_keys` only for stores that exported at least one node, so once the memIAVL `evm` tree is empty its last pre-migration value is exported forever. This is the same retention problem #4327 fixed for `seidb_migration_boundary_snapshot`. `migration.BuildRouter` now takes `RouterOption`s, and `WithoutTelemetry()` gives the router's `MigrationManager` `newLocalMigrationMetrics()` instead of the OTel-backed instance. `CompositeCommitStore.buildRouter` passes it for derived stores (the `LoadVersionReadOnly` view and `Copy`), so only the live store publishes migration metrics. `Snapshot` sets the per-store totals to zero on each store header, so a store with no nodes records 0. Converting the version gauge to an observable gauge would also work, but it leaves read-only handles publishing the other migration counters, so they are cut off at the router instead. No consensus, state, or wire-format impact: only metric emission changes, and the option is variadic, so existing `BuildRouter` callers are unchanged. A store absent from an export entirely (rather than exported empty) still keeps its last value; that does not occur for `evm` after the migration. Affected nodes show correct values after deploy, once they restart and export their next snapshot. `TestLoadVersionReadOnlyDoesNotReportMigrationVersion` and `TestSnapshotReportsZeroKeysForEmptyStore` each fail without their half of the fix; the migration, composite and rootmulti suites pass. --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: Cursor <cursoragent@cursor.com> (cherry picked from commit 82c0cda)
PR SummaryMedium Risk Overview During snapshot export, each store header now resets per-store key/size counters to zero so stores with no exported nodes still emit For derived composite stores (including read-only views opened at historical heights for snapshotting), Adds regression tests for zero-key reporting on empty post-migration Reviewed by Cursor Bugbot for commit 9c943fe. 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.
This is a clean backport of #4436 to release/v6.7. A store exported empty in Snapshot is now reported as zero keys instead of keeping its stale gauge, and derived (read-only) composite stores no longer publish migration gauges, so they can't overwrite the values the live store reports. Everything the change relies on (newLocalMigrationMetrics, cs.derived, and the rootmulti test helpers) exists on the release branch, and both fixes have regression tests.
Findings: 0 blocking | 0 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- None at the file/PR level.
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release/v6.7 #4442 +/- ##
================================================
- Coverage 61.70% 60.88% -0.82%
================================================
Files 2171 2088 -83
Lines 189908 179957 -9951
================================================
- Hits 117187 109572 -7615
+ Misses 61838 60311 -1527
+ Partials 10883 10074 -809
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Co-authored-by: Cursor <cursoragent@cursor.com>
|
@seidroid review |
There was a problem hiding this comment.
This backport stops derived composite stores (read-only handles and copies) from writing to the process-wide migration gauges, and makes snapshot export report zero for stores that export no keys, so neither gauge keeps a stale value. The fix is right: derived is set before every buildRouter call on read-only handles and copies, and newLocalMigrationMetrics is nil-safe on every recording path. Nothing blocks; the one scout reading (codex, which found nothing) matches this, and the new tests were not run because the sandbox has no Go toolchain.
seidroid review · decision approve · session 8ffdf524d0e04c5abccf9bd37e54d9d1 · turn resp_claude_03e791c6446521b228be8ec73700cebd · item 6f3c3b9f5f5a5ef299f1f8e9863d4b4e
Findings: 0 blocking | 0 non-blocking | 0 posted inline
## Summary - Bump `version.json` from `v6.7.0-rc4` to `v6.7.0` to cut the final `v6.7` release for mainnet. There is no rc5: this goes straight from rc4 to the release. (v6.6.0 was tagged on the rc5 bump, #3783, with `version.json` still reading `v6.6.0-rc5`; this keeps `version.json` in step with the tag.) Contents since rc4: #4442 and #4439, plus the changelog update (#4443). Merge this after #4443's backport has landed on `release/v6.7`, so the `v6.7.0` tag includes the updated changelog. - Both are labeled `non-app-hash-breaking` (a FlatKV migration metrics fix and `seidb` digest tooling), so moving from rc4 to `v6.7.0` should not need a coordinated validator switch. - Push `v6.7.0` by hand on this PR's merge commit once it has merged; the tagging ruleset stops `uci-release-publish` from creating it. ## Test plan - [x] `git diff --check` Made with [Cursor](https://cursor.com) Co-authored-by: Cursor <cursoragent@cursor.com>
Adds the `release/v6.7` entries merged since the rc4 changelog (sei-protocol#4406), in prep to cut **v6.7.0**, the mainnet release (there is no rc5; the version bump is sei-protocol#4444): - [sei-protocol#4442](sei-protocol#4442) — fix(seidb): fix stale FlatKV migration gauges on snapshotting nodes - [sei-protocol#4439](sei-protocol#4439) — seidb: add changelog mode and --inspect-plan to speed up EVM digest - [sei-protocol#4416](sei-protocol#4416) — removal of the conflict markers the rc4 changelog backport left on `release/v6.7` - [sei-protocol#4415](sei-protocol#4415) — rc4 changelog backport - [sei-protocol#4407](sei-protocol#4407) — rc4 version bump Regenerated with `./scripts/generate-changelog.sh release/v6.6 release/v6.7`; only the `## v6.7` PR list changes, so the `backport release/v6.7` cherry-pick applies cleanly (simulated with `git merge-tree` against `origin/release/v6.7`). Docs-only; no code change. Made with [Cursor](https://cursor.com) Co-authored-by: Cursor <cursoragent@cursor.com>
2 participants
Backport of #4436 to
release/v6.7.