fix(seidb): stop evm-logical-digest replay mode from mutating a live memiavl - #3840
fix(seidb): stop evm-logical-digest replay mode from mutating a live memiavl#3840blindchaser wants to merge 4 commits into
Conversation
…memiavl --memiavl-open-mode=replay relied on memiavl's ReadOnly option, which only skips the LOCK file. The changelog is still opened read-write, and sei-db's wal.open() answers wal.ErrCorrupt with os.Truncate on the node's segment. On a live node a torn tail is the writer mid-append, not corruption. Truncating it drops committed versions, and since the writer's fd keeps its old offset, its next append leaves a zero-filled hole that binary.Uvarint reads as valid empty records rather than an error. The node keeps producing blocks and finds out at its next restart. Clone instead, as openFlatKVReadOnly already does for the other backend: hardlink the snapshot, byte-copy the changelog, verify it still covers snapshotVersion+1, retry if the writer prunes mid-clone, then open the clone normally. Every source access is now a read. Also reject a clone that falls short of --height, since a tail repair costs the trailing version and a silently early digest looks like real divergence; and export memiavl's SeekSnapshotDir rather than reimplementing its snapshot layout in the tool. Replay mode now needs a writable dbDir and room for the changelog copy, as the FlatKV path already did. memiavl.OpenDB(ReadOnly) is still reachable from wasm snapshot export and storev2/rootmulti proof queries; fixing it at the WAL layer is tracked separately. Co-authored-by: Cursor <cursoragent@cursor.com>
66d9e50 to
c6308d5
Compare
PR SummaryMedium Risk Overview Shared tooling clone plumbing ( memiavl exports for tools: Reviewed by Cursor Bugbot for commit 69281d7. 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 #3840 +/- ##
==========================================
- Coverage 61.73% 60.65% -1.09%
==========================================
Files 2381 2280 -101
Lines 201682 190050 -11632
==========================================
- Hits 124518 115266 -9252
+ Misses 66077 64519 -1558
+ Partials 11087 10265 -822
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: 66d9e50150
ℹ️ 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".
| // our changelog copy, then prune the changelog up to that newer | ||
| // version — leaving a copy that no longer covers snapshotVersion+1 and | ||
| // a catchup that would silently skip versions. Retryable. | ||
| if err := verifyClonedWALCovers(dstChangelogDir, snapshotVersion); err != nil { |
There was a problem hiding this comment.
When memiavl has been initialized with a height greater than 1, its initial snapshot is still snapshot-0, while the first changelog entry uses the configured initial height (for example, SetInitialVersion(100) produces WAL version 100). Passing that clone to verifyClonedWALCovers treats the valid first entry as a gap because it requires firstVer <= snapshotVersion+1, so replay mode retries three times and fails until a later snapshot is written. Derive the expected successor from the snapshot's stored initial-version metadata rather than assuming version 1 follows snapshot 0.
Useful? React with 👍 / 👎.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any additional bugs, but this PR reworks memiavl's tooling replay path with several subtle concurrency/correctness invariants (hardlink-vs-copy cloning, WAL-coverage validation, retry-on-churn, shortfall rejection), so a human pass is worthwhile before merging.
What was reviewed:
- The new clone-and-replay path (
openMemiAVLReplay/tryPrepareMemIAVLToolingClone) and its reuse of the FlatKV tooling'sretryToolingClone/verifyClonedWALCovershelpers. - Confirmed the
--height 0case intentionally skips the shortfall check (no fixed target height to fall short of) — this matches a candidate issue the bug-hunting system already ruled out. - Traced clone cleanup (temp dir + lock release) on both the success and version-mismatch error paths in
openedMemIAVL.Close. - Checked that
SeekSnapshotDir's snapshot selection agrees withOpenDB's own internalseekSnapshotwhen it re-opens the clone, so the two don't disagree about which snapshot backs the replay.
Extended reasoning...
Overview
This PR reworks how the seidb tooling's evm-logical-digest --memiavl-open-mode replay" opens memiavl: instead of opening the live source directory with memiavl.Options{ReadOnly:true}(which still opens the changelog read-write and can truncate a torn tail out from under a live writer), it now clones the snapshot (hardlink) and changelog (byte copy) into a private temp directory underdbDirand replays there. It also exportsmemiavl.SeekSnapshotDir so the tool no longer reimplements memiavl's snapshot-selection logic, and factors the retry-on-churn logic (prepareFlatKVToolingCloneWith->retryToolingClone) so it's shared between the FlatKV and memiavl tooling clones. Two new files (memiavl_open.go/memiavl_open_test.go) and one exported function in sei-db/state_db/sc/memiavl/db.goare added;evm_logical_digest.gois updated to use the new*openedMemIAVL` wrapper.
Security risks
None of significance — this is offline/operator diagnostic tooling (seidb CLI), not a code path exercised during consensus, block execution, or RPC serving. The main risk class is data-safety (accidentally corrupting a live node's changelog while running a read-only diagnostic), which is precisely the bug this PR fixes, and it is covered by a dedicated regression test (TestOpenMemiAVLReplayLeavesSourceUntouched) that snapshots the source directory's file contents before/after and asserts byte-for-byte equality, plus a torn tail is deliberately left in place.
Level of scrutiny
Medium. The blast radius is limited to a tool operators run out-of-band against node data directories (not consensus-critical or app-hash-affecting, per the PR's own label), but the change involves nontrivial reasoning about concurrent writer races (hardlink vs. copy, WAL truncation mid-clone, retry semantics, snapshot-vs-changelog consistency) where a subtle mistake could reintroduce the exact class of bug being fixed, or introduce a new one only visible under a live-writer race. That combination — tool-scoped blast radius but concurrency-sensitive logic — puts this above "approve outright" but the existing tests target the specific hazard well.
Other factors
The PR body itself is unusually thorough (problem/fix/limits/tests sections) and explicitly scopes the fix to tooling only, noting the same ReadOnly weakness still exists in memiavl.OpenDB callers elsewhere (wasm snapshot export, storev2/rootmulti) and is tracked separately — that scoping honesty is a good sign. Three new tests exercise the specific hazard (source left untouched over a torn tail, opening while a live writer holds the LOCK, rejecting a clone short of the requested height and not leaving it behind). No CODEOWNERS or lint/build issues were apparent from the diff. The bug-hunting system found zero issues and one candidate (shortfall check skipped for --height 0) was independently confirmed to be correct-by-design rather than a bug.
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid, well-motivated fix: the replay path now clones the snapshot (hardlink) + changelog (byte-copy) instead of handing a live node's changelog to memiavl's WAL open, which truncates a torn tail. I verified the invariants it relies on (read-only snapshot mmap, rename-published snapshots, Version-bearing changelog entries, and Catchup silently falling short) all hold, so no blocking issues — only non-blocking notes around the unguarded --height 0 default, stale clone cleanup, and test coverage.
Findings: 0 blocking | 8 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion pass produced no output (
cursor-review.mdis empty), so it contributed nothing to this review. Codex explicitly reported no material issues. - Test coverage gap: both success-path tests use
height == 0, and the onlyheight > 0test asserts the rejection. There is no positive test that a non-zero--heightresolves an older snapshot viaSeekSnapshotDir'sseekSnapshotbranch and replays forward to exactly that height — i.e. the new strict equality check's success path and the non-currentsnapshot-selection branch are both untested. - No free-space pre-check before byte-copying the changelog. Because the clone must live inside
dbDirfor hardlinks to work, running the tool against a live node can fill that node's own data volume and halt it. The PR body notes callers "need room for the changelog copy," but nothing enforces or warns at runtime (same gap as the pre-existing FlatKV clone). - Behavior change worth surfacing to operators:
--memiavl-open-mode replaynow requires a writable--memiavl-dir(it creates a temp dir inside it). The flag help was updated to mention a "temp dir" but not that it is allocated inside the source directory, so a read-only-mounted data dir that used to work will now fail. Consider stating this in the flag help / package doc comment, not just the PR description. - 4 suggestion(s)/nit(s) flagged inline on specific lines.
| // when that falls short of the requested height. Every tail repair inside | ||
| // the clone costs the trailing version, and a digest computed one version | ||
| // early is indistinguishable from real divergence when comparing nodes. | ||
| if reached := db.Version(); height > 0 && reached != height { |
There was a problem hiding this comment.
[suggestion] The height > 0 guard leaves the flag's default (--height 0) with no coverage guarantee, even though the failure mode is identical. With --height 0, verifyClonedWALCovers opens the cloned WAL, tidwall repairs the torn tail by truncating it, and Catchup then replays to tip-1 and reports success — producing exactly the "digest one version early, indistinguishable from real divergence" outcome this check exists to prevent, on the most common invocation.
I acknowledge there's no cheap exact fix: learning the source's intended tip would mean opening the source WAL, which is the mutation this PR removes. But something is better than nothing — e.g. detect that the clone's WAL was repaired (compare the cloned changelog's byte size / last offset before and after verifyClonedWALCovers opens it) and either warn loudly or refuse for height == 0. At minimum, document in the doc comment above that height == 0 is best-effort so a reader doesn't infer the guarantee extends to it.
| } | ||
| // Not a "-tmp" suffix: memiavl's removeTmpDirs deletes every "*-tmp" | ||
| // directory under its root when a node opens the DB read-write. | ||
| tempDir, err := os.MkdirTemp(dbDir, ".seidb-memiavl-tool-*") |
There was a problem hiding this comment.
[suggestion] Nothing ever sweeps abandoned clones. If the tool is SIGKILLed (or the host reboots) between here and Close(), the .seidb-memiavl-tool-* directory survives inside the node's data dir holding hardlinks to the snapshot plus a full byte-copy of the changelog. The hardlinks mean the live node's snapshot pruning frees no space at all, and by design the name avoids the *-tmp suffix so memiavl's removeTmpDirs won't reclaim it on the next node restart either.
This is the same hazard the FlatKV clone already has, so it's not a regression — but this PR doubles the surface. Worth either a seidb subcommand / startup sweep for stale .seidb-*-tool-* dirs, or an explicit note in the flag help that operators must remove them manually after a crash.
| // The clone must sit inside dbDir to share a filesystem with the source | ||
| // snapshot: dbDir is often its own mount point, so a sibling directory is | ||
| // not enough and hardlinks would fail across the boundary. | ||
| if err := os.MkdirAll(dbDir, 0o750); err != nil { |
There was a problem hiding this comment.
[nit] This MkdirAll can never do useful work: memiavl.SeekSnapshotDir on line 85 already reads current / lists snapshots under dbDir, so it fails first if dbDir is missing. Carried over from tryPrepareFlatKVToolingClone, where it's equally moot. Dropping it (or moving it above the seek, if the intent was to create the dir) would remove a misleading suggestion that this path tolerates an absent source.
| // | ||
| // Exported for readers that need to resolve a snapshot without opening the DB, | ||
| // so they inherit this package's layout rules instead of restating them. | ||
| func SeekSnapshotDir(root string, targetVersion int64) (string, int64, error) { |
There was a problem hiding this comment.
[nit] Two small things on this newly exported API:
- The name says
Dirbut the first return value is a directory name relative toroot(callers mustfilepath.Join(root, name), astryPrepareMemIAVLToolingClonedoes). The doc comment says "directory name" correctly, butSeekSnapshotNamewould make the contract obvious at the call site. - No unit test accompanies the export — worth pinning at least the two branches (
targetVersion == 0resolving throughcurrent, andtargetVersionbelow the earliest snapshot returning thetarget version is prunederror), since this is now part of the package's public surface rather than an internal helper.
The clone-based replay open regressed nodes bootstrapped with a non-default initial height: memiavl keeps its bootstrap snapshot named snapshot-0 even after SetInitialVersion(N), while the first changelog entry is version N, so verifyClonedWALCovers treated the valid successor as a gap and replay mode failed deterministically with "source kept churning" until the first snapshot rewrite. A freshly recovered chain (genesis initial_height > 1) is exactly that shape. Derive the snapshot's successor from the cloned snapshot's metadata via utils.NextVersion instead of assuming snapshotVersion+1; FlatKV keeps the +1 rule because SetInitialVersion(N) seeds snapshot-<N-1> there. Also from review: - Reap abandoned clones. A SIGKILLed tool leaves its clone inside the data dir, where hardlinks pin snapshot inodes so pruning frees no disk and the deliberate non-"-tmp" name escapes memiavl's cleanup. Every clone now carries a flock-held owner marker (OFD/flock locks die with the process); clone creation sweeps siblings whose lock is acquirable, and marker-less directories once they are older than the mkdir-to-lock window can explain. - Document that --height 0 against a live node is best-effort: the clone is a consistent committed prefix as of the copy instant, and a detected torn-tail repair now prints a warning telling the operator to trust the report's version line. Cross-node comparisons should use an explicit common height. - Drop the MkdirAll that could never do useful work (snapshot selection already reads dbDir and fails first when it is missing). - Rename SeekSnapshotDir to SeekSnapshotName — it returns a name relative to root, not a path — and pin the exported contract with a unit test, along with ReadMetadata which the successor derivation needs. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Solid, well-motivated fix: replay mode now operates on a hardlinked/byte-copied private clone instead of handing a live changelog to memiavl's read-write WAL open, and the shared toolClone + flock ownership marker is a clean generalization of the existing FlatKV path. I verified the key assumptions hold (memiavl publishes snapshots via -tmp+rename and drops them via atomicRemoveDir, traverseSnapshots/removeTmpDirs ignore the .seidb-*-tool-* prefix, tryTruncateWAL truncates to the earliest snapshot so the coverage check really is a mid-clone-race detector, NextVersion + initEmptyDB make the SetInitialVersion case real, and FlatKV's LoadVersion does reject a version shortfall). No blockers; only polish and test-coverage notes.
Findings: 0 blocking | 5 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Test coverage gap on the memiavl side: the FlatKV suite pins the two properties that make the clone affordable (
TestPrepareFlatKVToolingCloneHardlinksSnapshotAndCopiesChangelog,TestPrepareFlatKVToolingClonePlacesTempDirInsideDBDir), butmemiavl_open_test.gohas no equivalent. Nothing currently fails iftryPrepareMemIAVLToolingCloneregressed to byte-copying a multi-GB snapshot or to creating the clone outsidedbDir— both would still pass every new test. A cheap assertion onos.SameFile(src, dst)for one snapshot file plus afilepath.Rel(dbDir, clone.dir)check would cover it. - Leaked-clone recovery is only triggered by a later invocation of the same mode on the same dir (
newToolClonesweeps just its own prefix, whichTestSweepStaleToolClonesdeliberately pins). Since a leaked clone's hardlinks pin snapshot inodes and silently stop the live node's pruning from reclaiming disk, consider printing the clone path to stderr at startup so an operator who SIGKILLs the tool can find and remove it without waiting for the next run. - Cursor's second-opinion pass produced no output (
cursor-review.mdis empty), so that perspective is missing from this synthesis. Codex reported no material issues, which matches my own read. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
| } | ||
| firstNeeded := utils.NextVersion(snapshotVersion, uint32(metadata.InitialVersion)) | ||
|
|
||
| srcChangelogDir := filepath.Join(dbDir, "changelog") |
There was a problem hiding this comment.
[suggestion] Nit: this hardcodes the memiavl changelog layout, while memiavl itself resolves it through utils.GetChangelogPath(opts.Dir) (sei-db/state_db/sc/memiavl/db.go:206). The file already imports sei-db/common/utils for NextVersion, so utils.GetChangelogPath(dbDir) here (and for dstChangelogDir below) keeps the tool inheriting the layout rather than restating it — the same rationale the PR gives for exporting SeekSnapshotName instead of reimplementing the snapshot layout.
| // publish snapshots by rename and drop them wholesale, so the same | ||
| // hardlink-the-snapshot / byte-copy-the-changelog split applies. | ||
| func tryPrepareMemIAVLToolingClone(dbDir string, height int64) (*toolClone, error) { | ||
| snapshotName, snapshotVersion, err := memiavl.SeekSnapshotName(dbDir, height) |
There was a problem hiding this comment.
[nit] For height == 0, SeekSnapshotName bottoms out in os.Readlink(current), so a missing or uninitialized dbDir returns an os.ErrNotExist-wrapped error. isCloneRetryableError treats that as retryable, so the operator gets three attempts and then clone aborted after 3 retries, source kept churning instead of the actual "no such file or directory". Same shape as the pre-existing FlatKV path, so not new, but it's a misleading diagnosis for the most common operator typo. Worth stat-ing dbDir/current once up front and failing fast.
There was a problem hiding this comment.
No new issues found in this pass. I revisited my earlier inline comment flagging that the WAL-coverage check might mis-handle a memiavl DB bootstrapped via SetInitialVersion(N) — that concern is already resolved in the current code: tryPrepareMemIAVLToolingClone derives the WAL successor from the cloned snapshot's InitialVersion metadata (via the newly exported memiavl.ReadMetadata) instead of assuming snapshotVersion+1, and TestOpenMemiAVLReplayAfterSetInitialVersion exercises and passes that exact scenario. Given this still touches memiavl's core snapshot/WAL read path and introduces new clone-lifecycle and locking logic, I think it is worth a human pass before merging.
Extended reasoning...
Overview
This PR replaces the tooling's use of memiavl.Options{ReadOnly: true} (which still opens the changelog WAL read-write and can truncate a live node's torn tail, destroying committed versions) with a clone-based approach mirroring the existing FlatKV tooling pattern: hardlink the snapshot, byte-copy the changelog, verify WAL coverage, then operate on the private clone. It adds a shared toolClone abstraction (owner flock + stale-clone sweep) used by both the FlatKV and new memiavl clone paths, and exports memiavl.SeekSnapshotName/memiavl.ReadMetadata so the tool inherits the package's snapshot-layout rules instead of re-implementing them. Production code paths (GetLatestVersion, LoadMultiTree) are only touched by the readMetadata->`ReadMetadata" export/rename, not behaviorally changed.
Security risks
None of consequence. This is operator-invoked CLI tooling (seidb evm-logical-digest), not exposed to untrusted input. The main risk class is data-integrity (a tool corrupting a live node's changelog), which is exactly what this PR fixes; it does not introduce a new one that I could find. The new clone directories inherit the source directory's permissions and live alongside the node's own data.
Level of scrutiny
Medium-high. While scoped to tooling, the change touches memiavl's snapshot/WAL layout assumptions directly (exports two previously-private helpers) and introduces new concurrency-sensitive logic (owner-lock based stale-clone sweeping, retry-on-churn cloning) that a live node's writer can race with. The regression tests are strong — they reproduce the exact torn-tail-truncation bug this PR fixes and assert the source directory is left untouched — and I ran the full operations + memiavl test suites locally; both new memiavl_open_test.go and tool_clone_test.go pass, along with the SetInitialVersion regression test.
Other factors
Two automated reviewers (chatgpt-codex-connector and seidroid) raised concerns in earlier PR comments — non-default initial-height WAL coverage, abandoned-clone cleanup, and a moot MkdirAll — that map exactly to the ReadMetadata-derived firstNeeded, sweepStaleToolClones, and warnIfCloneRepaired mechanisms already present in the current diff, so those appear to have been addressed already (possibly in the "address review findings" commit that was part of the PR from the start). My own prior inline comment repeated the same now-resolved concern; I want to flag here that it no longer applies against the current code so it doesn't mislead a future reviewer. Two local test failures (TestWriterLoopErrors, TestSnapshotWriterErrorHandling) are pre-existing and unrelated — they fail because the sandbox runs as root, which lets writes to '/invalid/path' succeed where the test expects a permission error.
…ay-clone Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # sei-db/tools/cmd/seidb/operations/flatkv_open.go
The merge resolution had folded main's verifyClonedWALCovers and the branch's memiavl equivalent into a shared helper. Main is the shared truth for the FlatKV path, so restore its function verbatim and let the memiavl verifier stand on its own: the two backends read different WAL formats and cannot share an implementation anyway. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
The core fix is correct and well-targeted: memiavl.OpenDB(ReadOnly:true) really does open the changelog read-write and truncate a torn tail (sei-db/wal/wal.go:546-577), and cloning the snapshot + changelog before opening removes that hazard while mirroring the existing FlatKV pattern. No blockers found; remaining notes are about an operational disk-space hazard, a heuristic warning path, and a few test/description gaps.
Findings: 0 blocking | 11 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Disk-space hazard on a live node: replay mode now byte-copies the entire memiavl changelog into
dbDiritself (the node's data volume). memiavl changelogs can be many GB depending on keep-recent/prune settings, and there is no pre-flight free-space check. A tool run against a nearly-full data volume can now fill it and stall the writer — the same class of harm this PR is fixing, arrived at from the other side. Consider astatfscheck against the source changelog size beforecopyDirRecursive, or at minimum printing the estimated copy size up front. - Test gap:
verifyClonedMemIAVLWALCovershas no test for its failure branch (errSourceChurningwhen the source WAL is pruned past the snapshot mid-clone) — the memiavl counterpart to the existingTestPrepareFlatKVToolingCloneDetectsWALTruncationRace. Given thatfirstNeededderivation from snapshot metadata is the subtlest new logic here, only its happy paths are currently exercised (TestOpenMemiAVLReplayAfterSetInitialVersion). - Test gap: nothing covers
walRepaired/warnIfCloneRepaired. Both the detection (byte-size delta) and the warning emission are untested for either backend, so a regression that silently stops warning would not be caught. - PR description overstates one cleanup: it says the change exports memiavl's
SeekSnapshotDir, but the exported symbol isSeekSnapshotName, andresolveMemIAVLEvmSnapshotDir(sei-db/tools/cmd/seidb/operations/evm_logical_digest.go:1500-1508) still reimplements thesnapshot-%020dlayout for the snapshot-mode path. Only the new replay path inherits the package's layout rules. Either route snapshot mode through the exported helper too, or reword the description. - Behaviour change worth calling out explicitly in release notes:
--memiavl-open-mode replaywith an explicit--heightnow hard-fails when the changelog cannot reach that height, where previously it returned a digest at whatever version it reached. This is the right call, but it will turn some previously-"working" runbook invocations into errors. - Minor design nit:
openedMemIAVLembeds*memiavl.DB, and the clone is opened read-write (noReadOnlyoption), so consumers handeddb.DBhave a fully writable memiavl handle. Nothing in this PR misuses it, but for a tool whose entire premise is "every source access is a read", narrowing what is handed toscanMemiavlReplayEVMLeaves/digestMemIAVLReplay*to a read-only interface would make that invariant structural rather than conventional. - Second-opinion passes: Codex reported no material issues (and noted it could not run tests — read-only Go module cache). The Cursor review file
cursor-review.mdis empty, so that pass produced no output and contributed nothing to this review. - I was unable to run
go build,go test, orgofmt/goimportsin this environment (the commands were blocked), so all verification above is static reading of the diff against the surrounding packages. CI results should be treated as the authority on compilation and formatting. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
| } | ||
| firstNeeded := utils.NextVersion(snapshotVersion, uint32(metadata.InitialVersion)) | ||
|
|
||
| srcChangelogDir := filepath.Join(dbDir, "changelog") |
There was a problem hiding this comment.
[nit] "changelog" is hardcoded here and on line 148, but this file already imports sei-db/common/utils, which owns the layout rule: utils.GetChangelogPath(dbDir) / utils.GetChangelogPath(clone.dir). That is memiavl.OpenDB's own way of naming the directory (sei-db/state_db/sc/memiavl/db.go:206), so if the layout ever moves, this clone silently copies the wrong thing.
This is the same principle the PR invokes to justify exporting SeekSnapshotName ("so they inherit this package's layout rules instead of restating them") — worth applying it consistently. The pre-existing copies in flatkv_open.go and replay_changelog.go could follow in the same pass or be left alone.
| // can perform), which callers surface as a warning for latest-height reads. | ||
| func changelogByteSize(dir string) int64 { | ||
| var total int64 | ||
| _ = filepath.Walk(dir, func(_ string, info os.FileInfo, err error) error { |
There was a problem hiding this comment.
[nit] filepath.Walk's error is discarded, so a failed walk returns 0 rather than being distinguishable from an empty directory. That makes the after < before comparison in the callers directionally unsafe in both directions:
- walk fails on the first call →
sizeBefore == 0→after < 0is false → a real torn-tail repair is never warned about; - walk fails on the second call →
0 < sizeBeforeis true → a spurious "torn tail was repaired" warning on a perfectly clean clone.
Only a stderr warning rides on this, so it is not blocking, but returning (int64, error) and treating a probe failure as "unknown" (neither repaired nor clean) would make the signal honest. The doc comment's claim that the size delta detects "the only mutation that open can perform" is true of the WAL open, but not of the probe itself.
| if err != nil { | ||
| return nil, fmt.Errorf("create temp dir under %s: %w", dbDir, err) | ||
| } | ||
| ownerLock, err := memiavl.LockFile(filepath.Join(dir, toolCloneOwnerLockName)) |
There was a problem hiding this comment.
[nit] The doc comment says the clone is marked owned via flock "before any expensive cloning starts", which is true — but there is a narrower window it doesn't cover, worth confirming against go-filelock's semantics: memiavl.LockFile calls filelock.New(path) (which creates the file) and only then TryLock(). Between os.MkdirTemp on line 57 and the lock actually being held on line 61, a concurrent sweepStaleToolClones in another process can os.Stat the freshly-created marker, find it, acquire it (nobody holds it yet), and os.RemoveAll a clone that is about to become live.
The practical blast radius is small — the owner's subsequent os.MkdirAll in cloneDirRecursive re-creates the directory, but with its marker now unlinked, so a later sweep classifies it as marker-less and reaps it after staleUnmarkedCloneAge. Two concurrent digests against the same node is a plausible operator action, so either close the window (create+lock the marker before it is discoverable, e.g. lock a name the sweep only recognises once fully established) or note the residual race in the comment so the next reader doesn't over-trust it.
1 participant
Problem
--memiavl-open-mode replayrelied onmemiavl.Options{ReadOnly: true},which only skips the
LOCKfile. The changelog is still opened read-write —db.gosays so outright ("Even in read-only mode we may need WAL replay") —and sei-db's
wal.open()answerswal.ErrCorruptwithos.Truncateon thenode's segment. Nothing on that path consults
ReadOnly.On a live node a torn tail is the writer mid-append, not corruption.
Truncating it drops committed versions, and since the writer's fd keeps its
old offset, its next append leaves a zero-filled hole — which
binary.Uvarintreads as valid empty records rather than an error. Nothing fails at the time;
the node finds out at its next restart.
Fix
Clone, as
openFlatKVReadOnlyalready does for the other backend: hardlinkthe snapshot, byte-copy the changelog, verify it still covers
snapshotVersion+1, retry if the writer prunes mid-clone, then open the clonenormally, lock and all. Every source access is now a read.
Two changes fall out of it:
--height(a tail repair costs thetrailing version, and a silently early digest looks like real divergence)
SeekSnapshotDirinstead of reimplementing its snapshotlayout in the tool
Limits
Tool-scoped only.
memiavl.OpenDB(ReadOnly)is still reachable from wasmsnapshot export and
storev2/rootmultiproof queries; fixing it at the WALlayer is tracked separately. The source
LOCKis still not taken — a racingwriter costs us a retry now instead of costing the node its changelog.
--memiavl-open-mode snapshot(the default) was never affected.Callers need a writable
dbDirand room for the changelog copy, as theFlatKV path already did.
Tests
the torn bytes left intact for the live writer
LOCK