Repository navigation
Backport release/v6.7: Flush MemIAVL changelog before exiting on an upgrade panic - #4292
Conversation
|
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-4287-to-release/v6.7
git worktree add --checkout .worktree/backport-4287-to-release/v6.7 backport-4287-to-release/v6.7
cd .worktree/backport-4287-to-release/v6.7
git reset --hard HEAD^
git cherry-pick -x 53a681b964a4b2a22f202040dc72929281c8c1be
git push --force-with-lease |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
`TestUpgradeMajor` fails when the last validator to upgrade comes back with `RUNNING_UPGRADED_NODE_3=FAIL`. MemIAVL writes its changelog WAL asynchronously (`sc-async-commit-buffer` defaults to 100), so `Commit` returns before the entry for that block is on disk. A node that is catching up executes the block before the upgrade height and the upgrade height back to back, and the `UPGRADE "v2.0.0" NEEDED` panic kills the process before the writer goroutine has flushed the previous block. FlatKV, whose block WAL is flushed synchronously in `Commit`, comes back at height 149 while MemIAVL comes back at 148; `CompositeCommitStore.reconcileVersions` resolves the disagreement by rolling FlatKV back to 148. The upgraded binary then re-executes block 149, one below the plan height, and `x/upgrade` correctly panics with `BINARY UPDATED BEFORE TRIGGER`, so every restart attempt in `seid_upgrade.sh` dies within its one-second grace period and `verify_running.sh` never sees a live process. In the failed run node 3's log shows exactly this: MemIAVL and FlatKV both at 148 with the early-upgrade panic on every attempt, while nodes 1 and 2 restarted at 149. The fix makes the intentional upgrade exit durable. `sctypes.Committer` gains `Flush()`, which blocks until every committed version is persisted; `memiavl.DB.Flush` reuses the wait that `checkBackgroundSnapshotRewrite` already performed inline (now `waitForPendingWALWrites`), `CompositeCommitStore.Flush` delegates to MemIAVL since FlatKV already persists synchronously, and `rootmulti.Store.Flush` exposes it to the app. `App.ProcessBlock` calls `flushCommittedStateForUpgradeExit` right before re-panicking on an upgrade panic, so the process only dies once the last committed block is in every backend's WAL. Closing the multistore instead was rejected because the in-process `upgrade_v67` tests recover the panic and keep reading state from the same app. Flaked in: https://github.com/sei-protocol/sei-chain/actions/runs/35609798417/job/106367298402 (cherry picked from commit 53a681b)
ead5552 to
d69d1d0
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release/v6.7 #4292 +/- ##
================================================
- Coverage 61.37% 60.90% -0.48%
================================================
Files 2163 2105 -58
Lines 189024 183786 -5238
================================================
- Hits 116017 111933 -4084
+ Misses 62280 61523 -757
+ Partials 10727 10330 -397
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
PR SummaryMedium Risk Overview Adds a On upgrade-related panics in Reviewed by Cursor Bugbot for commit d69d1d0. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
Adds a Flush() path through the SC commit-store stack and invokes it from ProcessBlock's upgrade-panic re-panic so the async memiavl changelog WAL catches up before the process exits. The mechanism is sound (no lock inversion with the WAL writer goroutine, bounded by a 10s deadline, flatkv's no-op Flush is justified by its synchronous commit-time WAL flush); the notes below are non-blocking.
Findings: 0 blocking | 3 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] Neither new test is likely to fail without the production change. In
app/upgrade_exit_flush_test.go, the whole ofFinalizeBlock(3)(BeginBlock + upgrade panic) runs between the lastCommit()and theGetLatestVersionassertion, so the async WAL writer has almost certainly drained regardless of the flush.TestFlushWaitsForAsyncWALWriteshas the same shape — it asserts a post-condition the writer reaches on its own. Both are useful smoke tests, but they don't guard the regression; making the WAL writer demonstrably lag (or asserting the flush was reached through a seam) would. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
checkBackgroundSnapshotRewrite's wait for the WAL to catch up (now extracted aswaitForPendingWALWritesinsei-db/state_db/sc/memiavl/db.go) is unbounded and busy-spins with a 1ns sleep, all whileCommitholdsdb.mtx. A wedged or errored-then-stalled WAL writer hangs the consensus goroutine indefinitely. This PR gives the newFlushaflushTimeout; the older path has no equivalent.
Inline comments (could not post inline; listed here)
sei-db/state_db/sc/composite/store.go:1409(RIGHT) -- [suggestion]Flushreturns on the first backend error, so a memIAVL failure (e.g. the newflushTimeoutfiring) skips the flatKV flush entirely.Closejust below deliberately joins errors from both backends instead. Today flatKV'sFlushis a no-op so there's no behavioural impact, but the interface contract allows a real implementation, and this is an exit path where best-effort on both backends is what you want. Consider collecting intoerrsand joining, matchingClose.
Adds the `release/v6.7` entries merged since the rc1 changelog (#4110), in prep to cut **v6.7.0-rc2**: - [#4292](#4292) — Flush MemIAVL changelog before exiting on an upgrade panic - [#4285](#4285) — fix(seidb): refuse a corrupted changelog in digest replay instead of repairing it - [#4255](#4255) — feat(seidb): Add JSON output to evm-logical-digest and inspect a FlatKV migration in flight - [#4116](#4116) — rc1 version bump - [#4113](#4113) — rc1 changelog backport 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 (verified with `git apply --check` against `origin/release/v6.7`). Docs-only; no code change. Made with [Cursor](https://cursor.com) Co-authored-by: Cursor <cursoragent@cursor.com>
## Summary - Bump `version.json` from `v6.7.0-rc1` to `v6.7.0-rc2` to cut the second `v6.7` release candidate. Contents since rc1: #4285, #4255, #4292 (all `sei-db` fixes/tooling), plus the rc2 changelog update (#4294). Merge after #4294 so the tag cut by `uci-release-publish` on this `version.json` change includes the updated changelog. ## Test plan - [x] `git diff --check` Made with [Cursor](https://cursor.com) Co-authored-by: Cursor <cursoragent@cursor.com>
Backport of #4287 to
release/v6.7.