fix(seidb): refuse a digest replay that would repair the live changelog - #3983
fix(seidb): refuse a digest replay that would repair the live changelog#3983blindchaser wants to merge 19 commits into
Conversation
Use an immutable WAL view for read-only replay so digest tooling never repairs or copies the live changelog. Return actionable retry errors when a point-in-time view cannot safely reach the requested version. Co-authored-by: Cursor <cursoragent@cursor.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3983 +/- ##
==========================================
- Coverage 61.30% 60.56% -0.74%
==========================================
Files 2155 2074 -81
Lines 188507 179795 -8712
==========================================
- Hits 115556 108891 -6665
+ Misses 62205 60895 -1310
+ Partials 10746 10009 -737
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
![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: 33c6cde16d
ℹ️ 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".
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Treat a segment removed between directory listing and open as WAL churn so read-only callers return the actionable retry path. Co-authored-by: Cursor <cursoragent@cursor.com>
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid change: the immutable read-only changelog reader correctly avoids tidwall's writable open (no file creation, no tail truncation, no .START/.END recovery completion), retains fds for a stable point-in-time view, and the new OpenDB cleanup defer fixes real mtree/WAL/file-lock leaks on failure. No blockers, but the fail-closed policy now also applies to production read-only callers (CommitStore.Exporter, LoadVersion(v, true)) that have no retry, and a few guards/classifications could be tightened.
Findings: 0 blocking | 6 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion]
openReadOnlyWALeagerlyio.ReadAlls every segment in the changelog directory and keeps one fd open per segment for the lifetime of the view. With the default 20 MB tidwall segment size and a multi-GB changelog that is hundreds of fds plus a full sequential read at open, even thoughMultiTree.Catchuptypically only replays a suffix starting at the selected snapshot version, and each entry is thenpreadagain onReadAt(so the bytes are read twice). Consider indexing segments lazily, or at least skipping segments entirely below the first needed offset. - [suggestion]
Options.ReadOnlyis not only the seidb replay tool:memiavl.CommitStore.Exporter(state-sync snapshot export, whenOnlyAllowExportOnSnapshotVersionis false) andCommitStore.LoadVersion(v, true)/CompositeCommitStore.LoadVersionReadOnlyalso flow through it, and they now inherit both the fail-closed WAL open and the new strictmtree.Version() != targetVersioncheck. The test plan covers only the WAL package,db_test.go, and the CLI. A memiavl-level test that opens read-only while a writer is committing in the same process (the analogue ofTestOpenReadOnlyChangelogWALConcurrentWriter) would pin the behavior those callers actually see. - 4 suggestion(s)/nit(s) flagged inline on specific lines.
Use the complete tail prefix during concurrent writes, treat a missing changelog as empty, and classify deferred read failures through the retryable WAL error path. Co-authored-by: Cursor <cursoragent@cursor.com>
PR SummaryMedium Risk Overview Adds Replay mode sets Reviewed by Cursor Bugbot for commit 7e5af21. Bugbot is set up for automated code reviews on this repo. Configure here. |
Keep existing memiavl read-only callers on their prior WAL path and require digest replay to opt into immutable, fail-loud access. Co-authored-by: Cursor <cursoragent@cursor.com>
The digest replay mode refused a torn changelog through a purpose-built read-only WAL reader and an opt-in memIAVL mode. A check before the open reaches the same outcome without either: the opener repairs only the tail segment, so reading that one segment answers whether the open would repair anything. This leaves memIAVL and the changelog opener untouched. Co-authored-by: Cursor <cursoragent@cursor.com>
blindchaser
changed the title
| _ = db.Close() | ||
| return nil, fmt.Errorf("memiavl replay reached version %d, not the requested height %d; "+ | ||
| "the changelog does not cover that height", reached, height) | ||
| } |
There was a problem hiding this comment.
Pruned WAL gap check removed
High Severity
Removing FailOnWALRepair also dropped the immutable-view check that the changelog still covers every version after the selected snapshot. Replay now only compares the final Version() to --height, so a pruned gap can replay a contiguous suffix, reach the requested height, and return a digest that silently omitted intermediate versions.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit dcdfa41. Configure here.
VerifyIntact sat in its own file, away from truncateCorruptedTail and loadNextBinaryEntry, which are the repair it guards against and the framing it reuses. Co-authored-by: Cursor <cursoragent@cursor.com>
Catchup starts at the changelog's first offset whenever the snapshot ends before it, so a changelog pruned past the snapshot replays a contiguous suffix, reaches the requested height, and omits the versions in between. Comparing the final version to --height does not catch that, and a digest missing intermediate versions reads as a state mismatch between nodes. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9075ecb. Configure here.
main gave CommitStore.Commit a version argument. The merge was textually clean because it touched no file this branch changed, so the break only showed up at compile time. Co-authored-by: Cursor <cursoragent@cursor.com>
The check flagged any distance between the snapshot and the changelog's first entry. A chain seeded above height 1 has that distance legitimately: its changelog starts at the initial version while initEmptyDB leaves the snapshot at version 0, and the versions in between never existed. Ask instead whether the offset the replay needs existed and was pruned, which is the branch of Catchup's clamp that loses data. Co-authored-by: Cursor <cursoragent@cursor.com>
tryTruncateWAL anchors its changelog cut at the earliest retained snapshot and seekSnapshot refuses a height below every snapshot, so the pruned-gap the guard reported cannot arise unless the data directory was assembled or pruned by hand. It also had to special-case a chain seeded above height 1, where the distance it measured is legitimate. Keep the pre-flight check the change exists for, and keep one positive control for it. Guarding a silent memIAVL replay gap belongs in memIAVL rather than in one caller. Co-authored-by: Cursor <cursoragent@cursor.com>
Checking the changelog and then opening it anyway left the repair reachable. The gap between the two spans LoadMultiTree, which is orders of magnitude longer than a torn tail survives, so a tail torn after the check was truncated by the open exactly as before. The check only ever covered the conditions that persist. Refuse inside the open instead. Config.NoRepairOnOpen makes open return ErrCorrupt for a torn tail rather than truncating it, and refuse a directory holding a .START/.END marker before wal.Open completes that truncation. memiavl passes it through as Options.NoChangelogRepair, and the digest sets it for replay mode. Default is off, so every existing caller keeps the repair. Co-authored-by: Cursor <cursoragent@cursor.com>
The marker check could not hold. tidwall writes a .START segment partway through every successful TruncateFront, not only an interrupted one, so it appears after the check exactly as a torn tail does. Completing that truncation underneath the writer makes its own remove fail, and tidwall sets l.corrupt on any error past that point, so the node's appends fail until it restarts. No pre-open check fixes this, so exclude the writer instead. Options.RequireExclusive takes the LOCK under ReadOnly too, and the digest refuses a directory seid has open, offering snapshot mode. Under that exclusion a torn tail is damage rather than an append in flight, so the message asks for an offline repair rather than a rerun, and reading the directory before the open is sound because nothing can change it. Co-authored-by: Cursor <cursoragent@cursor.com>
RequireExclusive made it redundant. With no writer on the directory, a torn tail or a leftover truncation marker is damage rather than a race, and the repair for it is the one seid performs at its next start, so refusing it here changes nothing about what the node replays. It only withheld a diagnostic. Restores sei-db/wal to its state on main, so the change no longer touches that package: the refusal is the lock, and the lock alone. Co-authored-by: Cursor <cursoragent@cursor.com>
1 participant
Summary
seidb evm-logical-digest --memiavl-open-mode replayopens the changelog of a live node through a writer's API, and that API repairs what it finds. It truncates a tail ending mid-record, and it completes an interrupted truncation by removing and renaming segments without reporting anything.Options.ReadOnlyprevents neither, because it gates the DB API rather than the changelog open.Both repairs damage a running node, because each condition also occurs transiently in normal operation, so the repair fires on a healthy log:
writeis not atomic against a concurrent reader, so a reader sees a page-aligned prefix of a multi-page changeset. Truncating it discards a recordseidhas committed, and the writer's descriptor keeps its old offset, leaving a zero-filled hole the binary decoder reads as valid zero-length records. Nothing fails at the time; it surfaces when the node next reopens and replays..STARTsegment is present partway through every successfulTruncateFront, between the write of that segment and its rename. Completing it underneath the writer makes the writer's own remove fail, and tidwall setsl.corrupton any error past that point, so every later append returnsErrCorruptuntil the process restarts.No inspection of the directory can separate either case from real damage, and no check before the open can prevent it: both conditions appear and vanish faster than the gap between a check and the open, which spans
LoadMultiTree. Refuse to replay a directory a writer holds.sei-db/state_db/sc/memiavl/filelock.go,opts.go,db.go: addOptions.RequireExclusive, which takes the directory'sLOCKunderReadOnlytoo, and exportErrLockedso a caller can name the outcome.ReadOnlykeeps skipping the lock, since a reader that cannot rerun is better served by proceeding.removeTmpDirsstays behind!ReadOnly, so this open still writes nothing itself. A failed open now releases the lock, which nothing else can do once no DB is returned.sei-db/tools/cmd/seidb/operations/evm_logical_digest.go: set it for replay mode, and report a held lock as the node being up, with the two ways forward.Holding the lock for the run also stops
seidfrom starting underneath a digest already in progress.The changelog opener is unchanged, so a directory no writer holds replays exactly as before, including the repair of damage a crash left behind. That repair is what
seiditself performs at its next start, so performing it here does not change what the node would have replayed.The cost is that replay mode now requires the node stopped. It is the slow fallback for a height with no snapshot, so the common path is unaffected, and
--memiavl-open-mode snapshotdoes not read the changelog at all.Test plan
sei-db/tools/cmd/seidb/operations/memiavl_open_test.go: refuse a directory whose writer is still open, namingErrLockedand both remedies; then open the same directory once that writer releases it, so the refusal tracks the writer rather than something the first attempt left behind. Replay a stopped node to the requested height as the positive control.go test -race ./sei-db/wal ./sei-db/state_db/sc/memiavl ./sei-db/tools/cmd/seidb/operationsmake dblint