Repository navigation
Backport release/v6.7: fix(memiavl): hold a snapshot reference for an iterator's lifetime - #4313
Conversation
…4291) ## Summary `Tree.Iterator` takes the read lock, builds the iterator, and releases the lock on return. The caller keeps `PersistedNode`s whose keys and values are slices into the snapshot's mmap, and nothing holds that mapping. A snapshot rewrite's `ReplaceWith` closes the old snapshot inline, so an iterator still scanning reads freed pages. It surfaces as `fatal error: fault` in `runtime.memmove` under `utils.Clone` under `memiavl.(*Iterator).Key`, which no `recover` can catch. The fix reuses the refcount that already exists for `Tree.Copy`. The iterator acquires the snapshot on creation and returns it on `Close`. Acquiring under the read lock is what makes it safe, because `ReplaceWith` and `Close` swap and close `t.snapshot` under the write lock. `Close` also drops the cached key and value, which under `zeroCopy` point into the mapping the release may unmap. ## When it fires An iteration has to still be running when a rotation lands, so exposure rises with iteration length and query load. Seen on the `flatkv-gates-v67e-3a022d9` cohort at as the chain crossed height 50000, with a rotation every 2000 versions. Two of three active validators faulted in the same second at the same program counter. Seen again on 2026-09-15. Every node crosses the same boundary on the same interval, so the failure is correlated across the validator set instead of independent. It is a liveness defect. AppHash agreed across all nodes afterwards and no state was lost. The 5.2 s block gap measures reopen on an 8 MiB fixture. A mainnet reopen is 4 to 5 minutes healthy, and was measured at 37 minutes against a large changelog, so the same crash on the same fraction of validators is a multi-minute halt. ``` snapshots.Manager.createSnapshot -> WasmSnapshotter.Snapshot -> wasm.Keeper.IterateCodeInfos ``` The dump ends at `prefix.(*prefixIterator).Next` and does not. Its `gaskv` frame points at block execution over an RPC query, which is an inference from one frame. The defect is not specific to the EVM store and is not introduced by the FlatKV migration. Any caller iterating a memIAVL tree across a rotation can hit it. ## Test plan `sei-db/state_db/sc/memiavl/snapshot_refcount_test.go` - `TestTreeIteratorOutlivesSnapshotRewrite` rewrites and reloads while an iterator is open, then keeps reading through it. - `TestTreeIteratorReleasesSnapshotOnClose` asserts the reference is returned, so an open iterator does not leak a snapshot. Verified on this branch: - `scripts/ramtest.sh ./sei-db/state_db/sc/memiavl/...` passes, 13.4 s. - `gofmt -s -l` reports nothing on the three changed files. - `go build ./sei-db/... ./sei-cosmos/storev2/...` succeeds. (cherry picked from commit 275e547)
PR SummaryMedium Risk Overview Iterator lifecycle: Tests: Adds regression coverage that iteration survives rewrite/reload (non-zero-copy clone path) and that snapshot refcount increments/decrements correctly on iterator open/close (including idempotent double-close). Reviewed by Cursor Bugbot for commit 86067da. 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release/v6.7 #4313 +/- ##
================================================
- Coverage 61.36% 60.62% -0.74%
================================================
Files 2163 2083 -80
Lines 189065 180455 -8610
================================================
- Hits 116014 109397 -6617
+ Misses 62320 61028 -1292
+ Partials 10731 10030 -701
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Clean backport of #4291: Tree.Iterator now takes a refcount on the backing *Snapshot under the tree's read lock and Iterator.Close returns it, closing the window where a concurrent snapshot rewrite/reload could unmap pages an open iterator was still reading. I verified the diff is byte-identical to the merged upstream PR, that the single NewIterator caller was updated, and that the acquire/release pairing is correct and idempotent; no PR-introduced issues found.
Findings: 0 blocking | 1 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- None at the file/PR level.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
Tree.Iterator(sei-db/state_db/sc/memiavl/tree.go:346) capturest.rootunder the read lock but the returned iterator walks it lock-free after the lock is released, and — unlikeTree.Copy— it does not hash the reachable nodes or raisecowVersion. A concurrentSet/Removetherefore mutates in place any reachableMemNodeabovecowVersion(viaMemNode.Mutate), so iterating a live tree while it is written races on the unpersisted half of the tree. This PR closes the mmap half of the same hazard; the heap half remains. Note also thatCompositeCommitStore.iterate's comment (sei-db/state_db/sc/composite/store.go:1499) claims "memiavl captures a COW root under the tree lock," which is not whatTree.Iteratorcurrently does. Pre-existing on the base branch and unchanged by this diff.
|
@seidroid review |
There was a problem hiding this comment.
Clean backport of #4291 to release/v6.7: the PR diff is byte-identical to the merged upstream change, the single NewIterator caller is updated, and the snapshot acquire/release pairing is correct and idempotent. No PR-introduced issues found.
Findings: 0 blocking | 1 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- None at the file/PR level.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
Tree.Iterator(sei-db/state_db/sc/memiavl/tree.go:346) capturest.rootunder the read lock but the returned iterator walks it lock-free once the lock is released, and — unlikeTree.Copy— it neither hashes the reachable nodes nor raisescowVersion. A concurrentSet/Removecan therefore mutate in place any reachableMemNodeabovecowVersion(viaMemNode.Mutate), so iterating a live tree while it is written still races on the unpersisted half of the tree. This PR closes the mmap half of the hazard; the heap half remains. Relatedly,CompositeCommitStore.iterate's comment (sei-db/state_db/sc/composite/store.go:1500) claims "memiavl captures a COW root under the tree lock," which is not whatTree.Iteratordoes. Pre-existing on the base branch and unchanged by this diff.
## Summary - Bump `version.json` from `v6.7.0-rc2` to `v6.7.0-rc3` to cut the third `v6.7` release candidate. Contents since rc2: #4334, #4315, #4313, plus the rc3 changelog update (#4335, via its `release/v6.7` backport). Merge after that backport so the `v6.7.0-rc3` tag includes the updated changelog. - #4334 (backport of #4318) is `app-hash-breaking` with no height gate, so a network that already ran the v6.7 upgrade on rc1 or rc2 has to move all of its validators to rc3 together. - As on rc1 and rc2, the tagging ruleset stops `uci-release-publish` from creating the tag on merge, so push `v6.7.0-rc3` on the merge commit by hand; that push publishes the release and runs GoReleaser. - #4315 fixes the arm64 static build that failed GoReleaser on the rc1 and rc2 tag pushes, so rc3 should be the first `v6.7` release candidate with binaries attached. ## Test plan - [x] `git diff --check` Made with [Cursor](https://cursor.com) Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Masih H. Derkani <m@derkani.org>
Adds the `release/v6.7` entries merged since the rc2 changelog (#4293), in prep to cut **v6.7.0-rc4**. The rc3 update (#4335) was closed without merging, so its entries are included here: - [#4411](#4411) — Log a pinned node's skipped migration kick-off once per batch size - [#4410](#4410) — Apply compiled KV repair files at a fixed height and generate them from digest inspect lists - [#4409](#4409) — feat: add migration pause handler - [#4397](#4397) — fix(evmrpc): release eth_getLogs DB-read slots when a block read panics - [#4378](#4378) — Raise goreleaser timeout to 2h - [#4377](#4377) — Fix FlatKV state sync bad-hash scenario - [#4371](#4371) — fix(seidb): keep writes in the old DB until the migration boundary first moves - [#4348](#4348) — fix(seidb): report only the current migration boundary on the snapshot gauge - [#4347](#4347) — fix(flatkv): keep 10 old checkpoints instead of mirroring memIAVL's count - [#4339](#4339) — rc3 version bump - [#4334](#4334) — Fail dynamic-gas precompile out-of-gas as an EVM out-of-gas call - [#4315](#4315) — Pin the Go builder image per architecture in build-static.sh - [#4313](#4313) — fix(memiavl): hold a snapshot reference for an iterator's lifetime - [#4295](#4295) — rc2 version bump - [#4294](#4294) — rc2 changelog backport Also returns `## v6.7` to the format used through v6.6: the version heading, `sei-chain`, and the generated PR list. The hand-written `### Improvements` and `### Upgrade guide` sections are removed; every PR they described is already a line in the generated list. Regenerated with `./scripts/generate-changelog.sh release/v6.6 release/v6.7`; only the `## v6.7` section changes. Docs-only; no code change. **Backport note:** the #4347 backport added its own line to the top of the `## v6.7` list on `release/v6.7`, and `main` doesn't have it, so the `backport release/v6.7` cherry-pick of this PR conflicts at that one spot (simulated with `git merge-tree`). Resolve it by taking this PR's side: its fifteen lines already include #4347. --------- Co-authored-by: Cursor <cursoragent@cursor.com>
Backport of #4291 to
release/v6.7.