feat(seidb): add state store snapshots - #3889
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
PR SummaryHigh Risk Overview
Reviewed by Cursor Bugbot for commit 5d1e675. 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).
|
![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: b4e531462e
ℹ️ 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".
| for _, target := range targets { | ||
| target.store.ScheduleCheckpoint(target.dest, m.isRunning, func(err error) { |
There was a problem hiding this comment.
When the periodic pruning manager overlaps a snapshot, these barriers order only async block writes and do not exclude CompositeStateStore.Prune. In split mode, Prune finishes all EVM databases before pruning Cosmos (store.go:558-564), so an EVM checkpoint can capture the new earliest version while the Cosmos checkpoint captures the old one; individual checkpoints can also land between Pebble's prune batches and its final earliest-version update. The published snapshot can therefore fail the existing earliest-version consistency check when reopened or advertise historical versions whose records were only partially retained. Coordinate pruning and checkpoint creation at the backend ordering choke point before publishing the snapshot.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit bf557f4. Configure here.
| } | ||
| logger.Info("pruned state store snapshot", "dir", dir) | ||
| } | ||
| } |
There was a problem hiding this comment.
Prune can dangle current symlink
Medium Severity
After rename succeeds, a later failure in syncDir or updateCurrentLink returns failure without advancing lastPublished, while the new snapshot directory remains on disk. Deferred prune still keeps the newest on-disk versions and can delete the older snapshot that current still points at, leaving a dangling current symlink until restart restores it.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit bf557f4. Configure here.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3889 +/- ##
==========================================
- Coverage 59.48% 58.58% -0.91%
==========================================
Files 2323 2236 -87
Lines 198554 189785 -8769
==========================================
- Hits 118106 111179 -6927
+ Misses 69240 68052 -1188
+ Partials 11208 10554 -654
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Well-structured, well-tested addition of opt-in exact-version Pebble checkpoints for the State Store; the ordered drain-barrier design, fail-closed startup checks, stop/publish synchronization, and retention/publication atomicity all hold up under review. No blockers found — the notes below are a silent type-assertion wiring risk, a config-switch/choke-point mismatch, and a few nits.
Findings: 0 blocking | 10 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Second-opinion passes: Codex reported "No material issues found" (and noted it could not run tests because the sandbox cannot download Go 1.25.6).
cursor-review.mdis empty — the Cursor pass produced no output, so it contributed nothing to this review. - The snapshot trigger is split across two call sites —
composite.ApplyChangesetAsync(non-empty blocks) and the empty-block branch inrootmulti.flush— with the "only block-commit may trigger" rule enforced by convention in doc comments rather than at a choke point. That is the patternAGENTS.md("Guard at the choke point, never at each caller") asks reviewers to push back on. CallingScheduleSnapshot(currentVersion)once influshafter the if/else (and dropping it fromApplyChangesetAsync) would make the invariant structural: todaysei-db/state_db/bench/wrappers/combined_wrapper.goalready callsApplyChangesetAsyncoutside the commit path. - Fail-closed startup is a good default, but the operator-facing consequence is under-documented:
rootmulti.NewStorepanics onss.NewStateStoreerror, so flippingss-snapshot-enable = trueon a backend without checkpoint support (rocksdb) or on a layout where the SS DBs and snapshot root span filesystems prevents the node from starting. The app.toml comment covers the hardlink case but not the unsupported-backend case — worth adding one line. snapshot_metrics.go'smust()panics inside a package-levelvarinitializer. The neighbouring precedent (sei-db/common/metrics/system_metrics.go:55) ignores instrument-construction errors instead. A panic there aborts the binary before logging is configured; consider matching the existing pattern or documenting why a hard failure is preferred here.- Cadence note: SS mirrors memIAVL's defaults (interval 10000 blocks, min-time 1h). At sub-400ms block times an interval boundary can land just under the 1h gate, so roughly every other boundary is skipped and the effective SS snapshot period doubles. This matches SC exactly and is presumably intended, but it interacts with the PR's "snapshots land on interval boundaries" framing — worth confirming it's the desired operational behavior for SS.
- 5 suggestion(s)/nit(s) flagged inline on specific lines.
| if err := rs.ssStore.SetLatestVersion(currentVersion); err != nil { | ||
| panic(err) | ||
| } | ||
| if scheduler, ok := rs.ssStore.(stateStoreSnapshotScheduler); ok { |
There was a problem hiding this comment.
[suggestion] This assertion fails silently. stateStoreSnapshotScheduler is a locally-declared interface matched structurally against types.StateStore; if CompositeStateStore.ScheduleSnapshot is ever renamed, moved, or has its signature changed, ok becomes false, this branch is skipped, and empty-block snapshots stop happening forever — with no compile error and no test failure (the existing coverage calls store.ScheduleSnapshot(5) directly in snapshot_test.go rather than going through flush).
Suggest a compile-time guard, e.g. var _ stateStoreSnapshotScheduler = (*composite.CompositeStateStore)(nil) at package scope (rootmulti already depends on sei-db/state_db/ss), or a flush-level test that commits an empty boundary block through the rootmulti store and asserts a snapshot appears.
| return nil, err | ||
| } | ||
|
|
||
| if ssConfig.SnapshotInterval > 0 { |
There was a problem hiding this comment.
[suggestion] The operator-visible switch is SnapshotEnable, but the store never reads it — both this gate and startSnapshotManager (snapshot.go:156) key off SnapshotInterval > 0, which is only populated by config.AlignSSSnapshotWithSC, called from exactly one place (rootmulti.NewStore:143).
Two silent failure modes follow: (a) any other construction path — sei-cosmos/server/config.GetConfig populates SnapshotEnable but leaves the cadence zero, and the seidb tools call ss.NewStateStore directly — honors ss-snapshot-enable = true by doing nothing; (b) a caller that sets SnapshotInterval without SnapshotEnable snapshots anyway with the switch off.
Per AGENTS.md ("Guard at the choke point, never at each caller"), the invariant belongs here: gate on ssConfig.SnapshotEnable, and treat SnapshotEnable && SnapshotInterval <= 0 as a startup error rather than a quiet no-op. The existing startSnapshotManager is already the fail-closed choke point for every other precondition.
| return fmt.Errorf("version must be non-negative") | ||
| } | ||
|
|
||
| opts := newPebbleOptions(db.config, nil) |
There was a problem hiding this comment.
[nit] Passing nil for the cache deviates from the convention two functions below (OpenDB at line 168 allocates a 32 MB cache and defer cache.Unref()s it), and it's the only production pebble.Open in the repo that relies on pebble's implicit cache allocation. That runs once per backend per snapshot (and once per EVM sub-DB when SeparateEVMSubDBs is on), each time allocating pebble's default-size block cache for a single Set + Close.
Suggest a small explicit cache with defer cache.Unref() to match OpenDB, which also removes any dependence on pebble's caller-vs-callee cache-ownership semantics.
| return nil | ||
| } | ||
|
|
||
| func verifySnapshotHardlinks(root string, sourceDirs []string) error { |
There was a problem hiding this comment.
[nit] The probe creates a temp file inside the live Pebble data directory. Pebble tolerates unrecognized filenames, so this is safe in the normal case, but a crash (or a hard kill) between os.CreateTemp and the os.Remove on line 231 leaves a .ss-snapshot-link-probe-* file behind in the SS DB dir — and os.CreateTemp gives it a random suffix, so nothing ever cleans it up and each restart adds another.
Consider a deterministic probe name and an unconditional os.Remove of it before creating, so restarts are self-healing.
| # coordination that prevents pruning, before consuming a snapshot. Snapshot | ||
| # attempts, skips, outcomes, duration, in-flight state, height, count, and | ||
| # apparent bytes are exported through ss_snapshot_* metrics. Default: false. | ||
| ss-snapshot-enable = {{ .StateStore.SnapshotEnable }} |
There was a problem hiding this comment.
[nit] sei-db/config/toml_test.go:TestStateStoreConfigTemplate asserts that every other [state-store] key renders (ss-enable, ss-backend, ss-keep-recent, ss-enable-read-write-metrics, evm-ss-*, …). Adding require.Contains(t, output, "ss-snapshot-enable = false") keeps the new key consistent with that convention and pins the default-off rollout at the template layer too.
Co-authored-by: Cursor <cursoragent@cursor.com>
blindchaser
changed the title
There was a problem hiding this comment.
Well-structured and unusually well-documented/tested feature, but there is one confirmed correctness blocker: SS pruning writes the s/_earliest marker outside the checkpoint barrier, so a snapshot taken during a prune pass captures divergent earliest-version markers across the Cosmos and EVM databases and validateEVMSSPostRecovery will refuse to open the restored snapshot.
Findings: 2 blocking | 7 non-blocking | 5 posted inline
Blockers
- Snapshot layout has no manifest, so a published snapshot is not self-describing: reopening one requires the operator to already know
EVMSplit,SeparateEVMSubDBs, the backend name, and (for EVM sub-DBs)UseDefaultComparer. Every test reconstructs that config by hand. Since the whole point of the artifact is to be consumed later by something that is not this process, consider writing a small config manifest next to.apparent-sizein the same publish step — otherwise the first restore path built on top of this will hardcode assumptions that silently rot when the EVM layout changes. - 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- The Cursor second-opinion pass produced no output (
cursor-review.mdis empty), so this review reflects only the Claude and Codex passes. - I was unable to run
go buildor the test suite in this environment (sandbox denied the commands), so correctness here is from reading the code rather than from a green run. The PR description claims targeted package tests, config characterization tests, andgo test -race ./sei-db/state_db/ss/compositeall pass; worth confirming CI agrees, particularlyapp/config_fuzz_test.go(the seed indices shifted) and the two config goldens. - Opening a published snapshot mutates it:
NewCompositeStateStoreon a snapshot directory creates a freshchangelog/and Pebble WAL inside it, after.apparent-sizehas already been recorded and after the header comment insnapshot.gocalls the result "immutable". Not harmful today, but the docs should tell consumers to copy the tree before opening it, or the recorded size and the "immutable" claim both quietly stop being true on first use. - 4 suggestion(s)/nit(s) flagged inline on specific lines.
| // queue — rather than sampling what the backends had applied — makes that label | ||
| // exact without the request having to wait. See requestSnapshot. | ||
| // | ||
| // The barrier orders only the async block-commit queues. Import, recovery, |
There was a problem hiding this comment.
[blocker] This comment names pruning as a path that bypasses the barrier and concludes it "must not call ScheduleSnapshot" — but the problem is the reverse direction: pruning does not need to call anything to corrupt a snapshot, it just needs to be running while one is taken.
pruneDescending commits delete batches incrementally and then writes the earliest-version marker at the very end (db.SetEarliestVersion(earliestVersion, false), sei-db/db_engine/pebbledb/mvcc/db.go:816), all directly against Pebble rather than through pendingChanges. CompositeStateStore.Prune (store.go:558) prunes the EVM store first and Cosmos second, so between the two SetEarliestVersion calls the two databases disagree on s/_earliest for the full duration of the Cosmos full-DB scan — minutes on a multi-TB store, repeated every PruneIntervalSeconds (default 600s).
A checkpoint whose barriers land in that window produces a snapshot with mismatched earliest markers. SetCheckpointVersion normalizes latestVersion across every target, but nothing normalizes earliestVersion, so the mismatch is published. Reopening that snapshot then fails at validateEVMSSPostRecovery (store.go:181-193):
EVM SS earliest version %d does not match Cosmos SS earliest version %d: state sync the EVM SS DB, or set evm-ss-split=false
So the snapshot is unusable, and the failure surfaces only at restore time — exactly when it is least recoverable. The same divergence applies across EVM sub-DBs when SeparateEVMSubDBs is on, where nothing validates it at all and reads just serve inconsistent history per key family.
The fix that fits the design already here: extend the publish step to stamp a single agreed earliest version into every checkpoint, the way SetCheckpointVersion already stamps the latest — i.e. a SetCheckpointEarliestVersion applied to all targets from startPublish, using max over the observed markers. Serializing prune against snapshot generation, or routing prune's marker write through ScheduleAtDrain, would also close it, but both put filesystem-scale work back on the choke point this PR was careful to keep clear.
Whichever way it goes, a test that publishes a snapshot with deliberately skewed earliest markers and then reopens it as a composite store would pin the behaviour; today no test in snapshot_test.go exercises snapshot-vs-prune overlap at all.
| panic(err) | ||
| } | ||
| if scheduler, ok := rs.ssStore.(stateStoreSnapshotScheduler); ok { | ||
| scheduler.ScheduleSnapshot(currentVersion) |
There was a problem hiding this comment.
[suggestion] The invariant "every committed version gets exactly one snapshot request" is now split across two layers: non-empty blocks are requested implicitly from inside composite.ApplyChangesetAsync (store.go:325, store.go:340), and empty blocks explicitly from here via a type assertion. flush() is the one function every commit passes through, so it is the choke point — per AGENTS.md ("Guard at the choke point, never at each caller"), a single ScheduleSnapshot(currentVersion) after the if/else would make the rule an invariant a reader can see in one place, and would leave ApplyChangesetAsync a pure write API.
As written the split also has a visible edge: for a non-empty block, flush() runs twice with the same currentVersion, so the trigger fires once from each layer per block. It is correct today only because maybeSnapshot dedups on version <= lastRequested, which is a fact about the manager rather than about this call site.
The asymmetry also makes ScheduleSnapshot's own doc comment ("Callers must not use this hook for direct writes such as import, recovery, or prune") only half-enforceable: the exported hook can be guarded by convention, but ApplyChangesetAsync fires unconditionally, so any future async-write caller inherits snapshotting silently.
| switch { | ||
| case m.stopped || version <= m.lastRequested: | ||
| // A repeated commit-path call is expected and is not a skipped attempt. | ||
| case m.inFlight: |
There was a problem hiding this comment.
[nit] Because rootmulti.flush() runs twice per non-empty block with the same currentVersion, a boundary rejected by either gate records two ss_snapshot_skipped increments for one boundary, while a boundary that is accepted records one attempt (the second call short-circuits on version <= m.lastRequested). That makes skipped and attempts count different units, so a dashboard ratio of skips-to-attempts reads roughly 2x high. Recording the skip reason only when version > m.lastRequested has not already been seen for this version — or moving the trigger to a single call site as suggested on rootmulti/store.go — would make the two counters comparable.
| return fmt.Errorf("create snapshot root %q: %w", root, err) | ||
| } | ||
| for _, sourceDir := range sourceDirs { | ||
| probe, err := os.CreateTemp(sourceDir, ".ss-snapshot-link-probe-*") |
There was a problem hiding this comment.
[nit] If the process dies between CreateTemp and the two os.Remove calls, a .ss-snapshot-link-probe-* file is left behind inside a live Pebble data directory (and possibly a hardlink in the snapshot root), and nothing ever cleans it up — removeStaleTmpDirs only looks for tmp- directories under the snapshot root. Pebble ignores unrecognized filenames so this is cosmetic rather than dangerous, but a node crash-looping at startup accumulates one probe pair per source directory per attempt. Sweeping leftover .ss-snapshot-link-probe-* entries alongside the existing stale-staging cleanup would close it.
| // comparer selection), which is worth stating rather than omitting — a field a config | ||
| // struct carries that configuration cannot address is exactly what a replacement | ||
| // manager would otherwise try to map a key onto. | ||
| // These fields have no independent [state-store] key. The first two |
There was a problem hiding this comment.
[nit] The replacement comment drops the load-bearing half of what was here: that pebbledb consumes KeepLastVersion in MVCC pruning and UseDefaultComparer in comparer selection, and therefore that these are fields a config struct carries which configuration cannot address — stated explicitly as the thing a replacement config manager would otherwise try to map a key onto. AGENTS.md is direct about this case: "Relocating a load-bearing invariant is the move, never deleting one to tidy up."
The new text also flattens two different reasons into one list. KeepLastVersion/UseDefaultComparer are unreachable-by-design; SnapshotInterval/SnapshotKeepRecent/SnapshotMinTimeInterval are mapstructure:"-" and derived at runtime by AlignSSSnapshotWithSC. Keeping the original prose for the first pair and adding a separate sentence for the new three would preserve the record instead of widening the exclusion list.
The snapshot rows widened the exemption list and dropped the record of why KeepLastVersion and UseDefaultComparer are unreachable. Relocate that prose and state the derived cadence fields as a separate, differently-caused case. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Opt-in SS Pebble checkpoints land with a well-reasoned drain-barrier design, fail-closed enablement, and unusually thorough tests; I found no correctness blockers in the concurrency, shutdown, or publish/retention paths. The notes below are structural/robustness suggestions, chiefly that the snapshot trigger is split across two layers rather than owned by a single choke point.
Findings: 0 blocking | 12 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Second-opinion passes: Codex reported "No material issues found";
cursor-review.mdis empty, so the Cursor pass produced no output. This review is therefore effectively unsupported by either external tool. - Startup is fail-closed by panic:
startSnapshotManagerfailure makesNewCompositeStateStoreerror, whichrootmulti.NewStoreturns into apanic. A transient condition in the hardlink preflight (ENOSPC, read-only mount, anos.CreateTempfailure inside the live DB dir) therefore becomes a boot crash-loop rather than a legible operator error. Fail-closed is the right call for a snapshot that could otherwise silently copy TBs of SSTs; consider making the message reachable without a stack trace. - Enabling
ss-snapshot-enableon arocksdbbackend aborts boot with "cosmos backend "rocksdb" does not support checkpoints". That is defensible, but the app.toml comment block never says so — it only says snapshots are PebbleDB checkpoints. One sentence there would save an operator a crash-loop. sei-db/config/toml_test.goasserts every other[state-store]key renders (ss-enable,ss-backend,ss-keep-recent,ss-enable-read-write-metrics, …) but was not extended withss-snapshot-enable. Adding the row keeps the template test's completeness property intact.- SS rollback is explicitly out of scope, but the interaction deserves a line in the
snapshot.goheader: after a rollback,lastRequestedretains the pre-rollback high-water mark, so re-executed interval boundaries are silently skipped, and already-publishedsnapshot-NNNNNdirectories keep labels belonging to a superseded chain. A consumer picking upcurrentafter a rollback has no way to detect that from the layout. sei-cosmos/server/config/config.go:ssSnapshotEnableis computed at ~line 513 but consumed at ~line 646. It matches the surroundingv.IsSetidiom, but moving it next to theStateStore{...}literal would keep the read and its use readable together.- Test coverage note:
TestSnapshotTakenAtExactIntervalBoundariescallssettleat each boundary, so the headline property the barrier buys — an exact label while writes keep flowing past it — is never exercised with concurrent traffic. A case that writes 1..12 with no intermediate settle and then asserts snapshot 10 excludes 11/12 would pin the actual claim. - No test covers the
rootmultiwiring (thestateStoreSnapshotSchedulerassertion and the empty-block trigger); the empty-block path is only tested by callingstore.ScheduleSnapshot(5)directly in the composite package, which cannot catch a regression inflush. - 4 suggestion(s)/nit(s) flagged inline on specific lines.
| return fmt.Errorf("evm store async enqueue failed: %w", err) | ||
| } | ||
| } | ||
| s.ScheduleSnapshot(version) |
There was a problem hiding this comment.
[suggestion] The snapshot trigger is split across two layers, which weakens the invariant the design depends on. snapshot.go states "The rootmulti commit path owns the trigger, including the explicit trigger for an empty block", but in practice only the empty-block trigger lives in rootmulti.flush; non-empty blocks trigger from inside ApplyChangesetAsync (here and at line 325).
ApplyChangesetAsync is a general types.StateStore method, not a commit-path-only entry point — sei-db/state_db/bench/wrappers/{state_store,combined}_wrapper.go both call it, and any future caller inherits the trigger silently. That is exactly the "guard repeated at call sites / convention the next caller can forget" shape AGENTS.md asks reviewers to push back on.
Since flush already has both branches in front of it, the choke point is there: drop these two calls and have flush call ScheduleSnapshot(currentVersion) once after the if len(changeSets) > 0 { … } else { … } block. The store method then documents a contract it actually enforces, and "the commit path owns the trigger" becomes true by construction rather than by comment.
| if err := rs.ssStore.SetLatestVersion(currentVersion); err != nil { | ||
| panic(err) | ||
| } | ||
| if scheduler, ok := rs.ssStore.(stateStoreSnapshotScheduler); ok { |
There was a problem hiding this comment.
[suggestion] A failed type assertion here is a silent no-op: if ss.NewStateStore ever returns something other than *composite.CompositeStateStore (a metrics or tracing wrapper, say), empty-block snapshots stop being scheduled with no log line, no metric, and no test failure — the boundary is simply skipped forever on quiet chains.
ss.NewStateStore returns types.StateStore today and composite.CompositeStateStore is its only implementation, so a compile-time assertion is available: var _ stateStoreSnapshotScheduler = (*composite.CompositeStateStore)(nil). Failing that, log once at NewStore when the assertion does not hold, so the capability loss is visible rather than inferred from missing snapshots.
| if !s.separateDBs { | ||
| db := s.primaryDB() | ||
| if db == nil { | ||
| done(nil) |
There was a problem hiding this comment.
[suggestion] done(nil) reports a successful checkpoint when there is no primary DB, which is the one fail-open branch in an otherwise fail-closed feature. The caller in requestSnapshot treats that as the EVM target completing cleanly, so publish renames and links a snapshot whose evm/<backend>/ tree does not exist — an incomplete snapshot published as good, discoverable only when someone tries to restore from it.
The branch is currently unreachable (NewEVMStateStore always appends at least one managed DB or returns an error), which is the argument for making it an error rather than a success: done(fmt.Errorf("EVM state store has no managed DB to checkpoint")). Same for the mirrored return nil in SetCheckpointVersion at line 435.
| defer m.publishMu.Unlock() | ||
| defer m.prune() | ||
|
|
||
| if err := os.Rename(tmpDir, finalDir); err != nil { |
There was a problem hiding this comment.
[nit] A boundary is lost permanently when publication fails, and the rollback behaviour is asymmetric with the request path. maybeSnapshot restores previous/previousRequestAt when requestSnapshot returns an error, but once the barriers are enqueued, lastRequested stays at version no matter how the attempt ends — a rename failure here, a SetCheckpointVersion failure in startPublish, or an updateCurrentLink failure below. The next eligible boundary is version + interval, i.e. 10k blocks away at the default cadence, and the only signal is the error log plus the outcome="failure" counter.
That may well be the intended trade-off (retrying at version would need the barrier re-enqueued, and the data has moved on). If so, say so in the publish doc comment; if not, resetting lastRequested to previous on a failed publish would let the following boundary retry.
Superseded: latest AI review found no blocking issues.
ApplyChangesetAsync is a general StateStore method with callers outside the commit path, so triggering from there handed a snapshot trigger to anyone who wrote a changeset. rootmulti.flush already sees both the populated and the empty block, so it takes the trigger for both and the store method documents a contract it enforces. The capability is now resolved once at startup, pinned by a compile-time assertion and logged if a future wrapper loses the method, rather than re-asserted per block where a failure silently skipped the boundary. Also make the EVM no-managed-DB branches errors instead of reporting a checkpoint that did not happen, and record three contracts that were only discoverable by reading the code: a boundary lost past the barrier is not retried, snapshots do not survive an SS rollback, and enabling them on a non-pebbledb backend fails startup. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Opt-in SS Pebble checkpoints are well-structured: the in-queue drain barrier makes the snapshot label exact without stalling the commit path, enablement is fail-closed (backend capability + hardlink preflight), lifecycle/locking around stop/publish looks sound, and test coverage is unusually thorough. No blockers found; the notes below are durability/observability edges and one cross-DB consistency risk that will matter once restore lands.
Findings: 0 blocking | 10 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Second-opinion passes: Codex reported "No material issues found in the PR diff";
cursor-review.mdis empty, so that pass produced no output and contributed nothing to this synthesis. - Operational risk worth calling out in release notes (already documented in
toml.go, just easy to miss): withss-snapshot-enable = true, a node whose EVM SS lives on a different filesystem than<home>/data/state_storefails the hardlink preflight, androotmulti.NewStoreturns that into a startuppanic. Multi-mount SS layouts go from "works" to "won't boot" on a config flip. - The checkpoint occupies each backend's apply goroutine for the whole operation; once the 100-entry async buffer fills, block commit blocks. The existing
pendingChangesQueueDepthmetric is the only signal for that stall — a dedicated "commit blocked on snapshot barrier" measurement would make the tradeoff visible in production rather than inferred. - Lock/lifecycle review:
scheduling.Addhappens under the same mutexstop()uses before waiting, andpublishing.Addlikewise, so there is noScheduleAtDrain-after-close(pendingChanges)panic window and no lock-order inversion betweenmuandpublishMu. Retentionkeep := 1 + keepRecentcorrectly mirrors memIAVL's "besides the latest" semantics, andEffectiveMemIAVLSnapshotMinTimeIntervalmatchesOptions.FillDefaults(0 → 1h) exactly. - I reviewed this statically:
go build/go testwere blocked by the sandbox in this environment, so I did not execute the new tests. CI is the check for that. - 5 suggestion(s)/nit(s) flagged inline on specific lines.
| // exact without the request having to wait. See requestSnapshot. | ||
| // | ||
| // The barrier orders only the async block-commit queues. Import, recovery, | ||
| // pruning, and direct version-marker writes bypass those queues and must not |
There was a problem hiding this comment.
[suggestion] This correctly notes that pruning bypasses the barrier, but there is a consequence worth capturing: CompositeStateStore.Prune prunes the EVM store and then the Cosmos store (store.go:559-566), and each Prune ends in SetEarliestVersion — a direct write outside the apply queue. The Cosmos and EVM barriers execute independently, so a prune landing between them yields a snapshot whose Cosmos and EVM checkpoints carry different earliestVersion markers.
That combination is exactly what validateEVMSSPostRecovery rejects ("EVM SS earliest version %d does not match Cosmos SS earliest version %d"), so such a snapshot is unrestorable — and nothing in the layout or the metrics flags it. The window is small (barrier skew vs. the 600s prune interval), but on a mature node with ss-keep-recent reached, prunes are continuous, and with keepRecent=1 a bad snapshot can be the one current points at.
Restore is explicitly out of scope for this PR, so this need not block. But it is cheap to detect at publish time: compare the two checkpoints' earliest markers in startPublish before renaming, and drop the attempt with outcome="failure" if they disagree, rather than publishing an artifact that only fails when someone tries to use it.
| return nil, err | ||
| } | ||
|
|
||
| if ssConfig.SnapshotInterval > 0 { |
There was a problem hiding this comment.
[suggestion] The manager keys off SnapshotInterval > 0, but the operator-facing knob is SnapshotEnable; the two are only connected by a call to AlignSSSnapshotWithSC that lives in rootmulti.NewStore. Any construction path that sets SnapshotEnable = true and forgets the alignment call gets zero snapshots and zero log lines.
That is the same failure mode the type assertion in rootmulti.NewStore deliberately logs about ("visible as a boot line instead of as snapshots that never appear"). Applying the same treatment here would make the invariant hold at the choke point every path goes through:
if ssConfig.SnapshotEnable && ssConfig.SnapshotInterval <= 0 {
return nil, fmt.Errorf("ss-snapshot-enable is set but the snapshot cadence was never mirrored from state-commit")
}Erroring is defensible given the rest of the feature is fail-closed; a warning log would also do.
| m.stopped = true | ||
| m.mu.Unlock() | ||
| m.scheduling.Wait() | ||
| m.publishing.Wait() |
There was a problem hiding this comment.
[nit] stop() blocks node shutdown on the whole publish tail with no bound: SetCheckpointVersion opens and closes a Pebble DB per backend (4 more in separate-sub-DB mode), snapshotDirApparentBytes walks every file in the checkpoint, and prune() can RemoveAll an entire hardlink tree. On a multi-TB store that is plausibly tens of seconds, which is long enough for a supervisor's shutdown timeout to escalate to SIGKILL — leaving behind exactly the tmp- staging directory removeStaleTmpDirs then has to clean up.
Since a mid-publish snapshot is discardable by design, consider having stop() wait with a deadline and abandon the staging directory on expiry (startup already handles the leftover), instead of waiting unconditionally.
| defer m.publishMu.Unlock() | ||
| defer m.prune() | ||
|
|
||
| if err := os.Rename(tmpDir, finalDir); err != nil { |
There was a problem hiding this comment.
[nit] Publication fsyncs m.root after the rename, and writeSnapshotSize fsyncs tmpDir, but the intermediate tmpDir/cosmos and tmpDir/evm directories created by os.MkdirAll in requestSnapshot are never synced. After an unclean shutdown, snapshot-N/ and the current symlink can be durable while snapshot-N/cosmos/pebbledb is missing its directory entry — and startup would still adopt N as lastPublished and point current at it.
Note also that when snapshotDirApparentBytes fails, writeSnapshotSize is skipped entirely, so tmpDir itself never gets synced on that path either. Syncing each level of the staging tree before the rename (or syncing tmpDir unconditionally, independent of the size bookkeeping) would close both.
| return fmt.Errorf("create snapshot root %q: %w", root, err) | ||
| } | ||
| for _, sourceDir := range sourceDirs { | ||
| probe, err := os.CreateTemp(sourceDir, ".ss-snapshot-link-probe-*") |
There was a problem hiding this comment.
[nit] The probe file is created inside the live Pebble data directory. Pebble ignores filenames it does not recognize, so this is safe in operation, but a crash between CreateTemp and the os.Remove on line 236 orphans a .ss-snapshot-link-probe-* file in the database directory permanently, with nothing to clean it up. Creating the probe in the snapshot root and linking into the source directory tests the same filesystem property while confining the debris to a directory that already gets swept at startup by removeStaleTmpDirs.
…apshots Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Well-structured, thoroughly documented opt-in SS snapshot feature with strong test coverage; I found no correctness or security blockers. Three non-blocking suggestions, all about making silent degradation observable, plus a note that the Cursor second-opinion pass produced no output.
Findings: 0 blocking | 7 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Second-opinion passes:
cursor-review.mdis empty, so the Cursor review produced no output for this PR. Codex reported "no material issues found" but noted it could not run tests (read-only Go module cache). - Verification caveat:
go build,go vet,go test, andgofmtwere all denied in my review sandbox, so this is a static read only. The PR description reportsgo test -racegreen onsei-db/state_db/ss/composite,sei-db/config,sei-db/db_engine/pebbledb/mvcc, andsei-cosmos/storev2/rootmulti, plus cleangofmt -s/goimports— worth confirming CI agrees before merge. - Observability of the write stall:
ss_snapshot_durationmeasures request → publish, which spans the async publish goroutine (dir walk, rename, fsync, prune). The quantity that actually causes commit-path backpressure is narrower — the interval the backend's apply goroutine spends insidepebble.Checkpoint(WAL flush + fsync + hardlinking every SST). On a multi-TB store that is the number an operator needs to correlate with a block-time spike, and no metric currently isolates it. A histogram wrapped directly aroundcp.Checkpointintypes.ScheduleCheckpointwould close the gap. - The "managed snapshot directories have no lease" contract is enforced only by documentation (header comment in
snapshot.go,ss_config.go, and the app.toml template).prune()willos.RemoveAlla directory a consumer is actively reading. That is a reasonable scope call for a default-off first release and it is documented in three places, but it is the sharpest operational edge here — the follow-up that adds a lease API is worth tracking explicitly rather than leaving to the comments. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
| return nil, err | ||
| } | ||
|
|
||
| if ssConfig.SnapshotInterval > 0 { |
There was a problem hiding this comment.
[suggestion] Two things worth tightening here.
First, this gate on SnapshotInterval > 0 is duplicated by the identical gate at the top of startSnapshotManager (snapshot.go:169). Per the repo guideline on guarding at the choke point rather than at each caller, startSnapshotManager is the choke point every enablement path passes through — the caller-side copy can go.
Second, and more useful: SnapshotEnable does not itself gate anything at the store layer. Only the derived SnapshotInterval does, and that is populated exclusively by AlignSSSnapshotWithSC, which only rootmulti.NewStore calls. So a construction path that sets SnapshotEnable = true but never mirrors the SC cadence — the seidb tool commands and bench/wrappers all build StateStoreConfig directly — silently runs with snapshots off. That is the intended behaviour for those callers, but the combination SnapshotEnable == true && SnapshotInterval == 0 is indistinguishable from "disabled" in the logs, which sits awkwardly next to the fail-closed enablement the rest of this file implements. A logger.Warn on exactly that combination inside startSnapshotManager would turn a silent no-op into one boot line.
| evmScheduler: evmScheduler, | ||
| snapshotSizes: map[int64]int64{}, | ||
| } | ||
| m.lastRequested = m.newestSnapshotVersion() |
There was a problem hiding this comment.
[suggestion] This seeds lastRequested from the newest label on disk, which is the right high-water mark for the normal restart case. The header comment (lines 56-63) correctly identifies the two cases where it goes wrong — SS rollback, and state-syncing to a height below existing snapshots in a reused home — and states the snapshot root must be cleared by hand.
The gap is that neither case produces any signal. Every subsequent boundary falls into the version <= m.lastRequested arm of maybeSnapshot (line 284), which deliberately records no skip metric because a repeated commit-path call is the expected case there. So the failure mode is: snapshots stop forever, ss_snapshot_attempts stops incrementing, ss_snapshot_skipped stays flat, and nothing is logged. An operator has only the absence of new snapshot-NNNNN directories to go on.
This is cheap to detect at exactly this point: the composite store's latest version is available, so a logger.Warn when m.lastRequested > s.cosmosStore.GetLatestVersion() would name the condition and point at the manual fix the comment already prescribes.
| // contract, so record this attempt as a failure. | ||
| logger.Error("failed to update state store snapshot current link", | ||
| "version", version, "error", err) | ||
| return false |
There was a problem hiding this comment.
[nit] Returning false here is defensible — the comment above explains the link is part of the publication contract. Two small consequences of the early return, though:
snapshotMetrics.CurrentHeight.Record(...)at line 503 is skipped, andm.lastPublishedis not advanced, so the published-height gauge stays pinned at the previous snapshot even thoughsnapshot-NNNNNis on disk, fsynced, and discoverable by name. The gauge then under-reports until some later boundary succeeds.- The caller records
outcome="failure", the same label a checkpoint error or a failedos.Renamegets. A symlink-swap failure leaves a fully usable snapshot; a checkpoint failure leaves nothing. Collapsing them makes the counter harder to act on.
A distinct outcome label (say link_failed) plus recording the height would keep both signals honest without changing the control flow.
There was a problem hiding this comment.
A well-structured, thoroughly tested feature: the in-queue barrier gives an exact snapshot label without stalling the commit path, enablement is fail-closed (capability + hardlink preflight), and the config characterization suite is properly extended for the new key and the three derived fields. I found no correctness blockers; the notes below are operational-hardening and repo-style points, plus the fact that the Cursor pass produced no output.
Findings: 0 blocking | 9 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
Non-blocking
cursor-review.mdis empty — the Cursor pass produced no output for this PR, so only the Codex pass (which reported "no material issues") and this review actually covered the diff.- Style (AGENTS.md "Structural corrections"):
rootmulti/store.go:284-288is a five-line rationale comment sitting inline inflush. The guide's rule is that a long inline comment means the step was never named — extractingscheduleStateStoreSnapshot(currentVersion)(or similar) and moving the rationale onto its doc comment would match. Same shape applies to the ~15-line "why not ApplyChangesetAsync" godoc onCompositeStateStore.ScheduleSnapshotand the rationale paragraph insnapshot.go:449-458onpublish. - The trigger for SS snapshots and the trigger for SC snapshots now share one cadence but are enforced in two different places (
AlignSSSnapshotWithSCatrootmulti.NewStore,startSnapshotManagerin the composite store). Worth a follow-up note that changing memIAVL's snapshot cadence now silently changes SS disk retention too — a 10000-block interval withkeepRecent=1pins two full SST sets against compaction, and operators have no independent SS-side dial. Thetoml.gocomment covers this; the constraint just isn't discoverable from the SC side. Database.Checkpointdereferencesdb.storageunguarded, unlikePebbleMetricswhich nil-checks it for the closed-DB case. It is currently safe (the barrier only runs on the apply goroutine, whichClosedrains beforestorage.Close()), but the asymmetry with the neighbouring method invites a future caller to reach it off that path.- 5 suggestion(s)/nit(s) flagged inline on specific lines.
| evmScheduler: evmScheduler, | ||
| snapshotSizes: map[int64]int64{}, | ||
| } | ||
| m.lastRequested = m.newestSnapshotVersion() |
There was a problem hiding this comment.
[suggestion] Startup seeds lastRequested/lastPublished from the newest directory on disk without ever comparing it to the live store's version. The package comment (lines 56-63) documents the consequence — after an SS rollback or a state-sync to a lower height in a reused home dir, current keeps pointing at an abandoned-chain image and every boundary at or below the stale high-water mark is read as a repeat and skipped — but leaves detection to the operator.
The rest of this feature is fail-closed (capability assertion, hardlink preflight, ScheduleCheckpoint rejecting a backend rather than degrading), and the same posture is cheap here: startSnapshotManager already holds s.cosmosStore, so a snapshot labelled above s.cosmosStore.GetLatestVersion() is unambiguously stale and could refuse boot (or clear the root) instead of silently serving it. The current behaviour is a snapshot root that looks healthy, a current link that resolves, and no new snapshots appearing — none of which is visible in the ss_snapshot_* metrics beyond a flat current_height.
| return fmt.Errorf("create snapshot root %q: %w", root, err) | ||
| } | ||
| for _, sourceDir := range sourceDirs { | ||
| probe, err := os.CreateTemp(sourceDir, ".ss-snapshot-link-probe-*") |
There was a problem hiding this comment.
[nit] The probe file is created inside a live Pebble data directory. Pebble ignores filenames it can't parse, so this is safe while running — but if the process dies between CreateTemp and the os.Remove on line 242, a .ss-snapshot-link-probe-* file is left in the DB directory permanently, since Pebble's obsolete-file scan won't reclaim it either. Consider sweeping .ss-snapshot-link-probe-* from each sourceDir before probing, so a crashed boot doesn't accumulate debris across restarts.
| return nil, err | ||
| } | ||
|
|
||
| if ssConfig.SnapshotInterval > 0 { |
There was a problem hiding this comment.
[suggestion] This gates on SnapshotInterval > 0 and never reads SnapshotEnable, so the operator's intent is encoded in one field and enforced through another. Every production path currently runs AlignSSSnapshotWithSC first, which keeps the two consistent — but any construction path that misses it (the seidb tools and benchmark wrappers already build their own StateStoreConfig) turns ss-snapshot-enable = true into a silent no-op with no log line. Given the fail-closed posture elsewhere in this PR, if ssConfig.SnapshotEnable && ssConfig.SnapshotInterval <= 0 returning an error (or at minimum logging) would make that combination impossible to hold accidentally.
| return types.SetCheckpointVersion(s.db, destDir, version) | ||
| } | ||
|
|
||
| func (s *CosmosStateStore) WaitForPendingWrites() { |
There was a problem hiding this comment.
[nit] WaitForPendingWrites here and its counterpart in evm/store.go have no production caller — the only consumer is settle in snapshot_test.go. Since that helper already type-asserts on interface{ WaitForPendingWrites() }, the methods are effectively test-only API on production types. Either note that in a doc comment or move the helper into the test (e.g. reaching the managed DBs through an exported test hook), so the next reader doesn't take these for part of the store contract.
| "github.com/sei-protocol/sei-chain/sei-db/db_engine/types" | ||
| ) | ||
|
|
||
| // Online state-store snapshots. Every SnapshotInterval blocks the store takes a |
There was a problem hiding this comment.
[nit] This ~50-line block is attached to the const declaration below, so it becomes the godoc for SnapshotsDirName, snapshotPrefix, and friends. AGENTS.md is explicit that a godoc documents its subject rather than the surrounding architecture, and that multi-paragraph godocs are rare — the content (on-disk layout, barrier semantics, rollback interaction, lease caveat) is genuinely valuable but belongs in a doc.go package comment, where it also becomes discoverable as package documentation.
There was a problem hiding this comment.
Well-structured, thoroughly tested, and unusually well-documented addition of online SS snapshots; the barrier design genuinely delivers the "exact label" property it claims. One blocking issue: because SS pruning writes the earliest marker outside the barrier and prunes EVM before Cosmos, snapshots taken during the (minutes-long) gap bake in mismatched earliest versions and are rejected by validateEVMSSPostRecovery on reopen.
Findings: 1 blocking | 11 non-blocking | 7 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Cursor's second-opinion pass (
cursor-review.md) is empty — no findings were contributed from that tool. Codex contributed one finding (the retention-vs-currentinteraction), addressed inline. - The PR description's claim that "the commit path never waits for a checkpoint" is true only for the barrier enqueue itself.
maybeSnapshot→requestSnapshotdoesos.Stat,os.RemoveAllandos.MkdirAllsynchronously on the consensus commit path, andScheduleAtDrainis a blocking channel send. Withss-async-write-buffer = 0— which per theconfig_fuzz_test.gomanifest is exactly what anapp.tomlpredating that key resolves to — the queue is unbuffered, so the block after a boundary blocks for the full checkpoint.toml.godocuments the backpressure, but the summary reads stronger than the code. rootmultinow importssei-db/state_db/ss/compositepurely for thevar _ stateStoreSnapshotScheduler = (*sscomposite.CompositeStateStore)(nil)assertion. It's a reasonable way to pin the capability (andsc/compositeis already imported), but it does add a concrete dependency edge from the store layer onto an SS implementation package for a compile-time check only — worth a second opinion from a maintainer on layering.- Enabling
ss-snapshot-enableis fail-closed at boot: a node whose EVM SS directory is on a different filesystem than<home>/data/state_storewill refuse to start rather than start without snapshots. That's the right default, but it means the flag can turn a working node into a non-booting one. Consider calling this out in the release notes, not only in theapp.tomlcomment. controlledSnapshotScheduler.ScheduleCheckpointdoesclose(s.entered)unconditionally, so it panics if any future test drives that scheduler through two checkpoints. Async.Onceor a nil-out after close would make the helper reusable.- 6 suggestion(s)/nit(s) flagged inline on specific lines.
| // pruning, and direct version-marker writes bypass those queues and must not | ||
| // call ScheduleSnapshot. The rootmulti commit path owns the trigger for every | ||
| // block, populated or empty, and is the only caller of ScheduleSnapshot. | ||
| // Because pruning bypasses the barrier, a checkpoint can capture a partially |
There was a problem hiding this comment.
[blocker] The consequence of pruning bypassing the barrier is understated here, and it can make a published snapshot permanently unopenable.
CompositeStateStore.Prune prunes the EVM store first and Cosmos second (store.go:559), and each Database.Prune writes its own s/_earliest marker at the end of its scan (pebbledb/mvcc/db.go:816). So for the whole duration of the Cosmos prune — minutes on a large SS, repeating every ss-prune-interval (default 600s, keep-recent default 100000, so pruning is on by default) — the EVM DB is at E_new while Cosmos is still at E_old.
A snapshot whose two checkpoints land anywhere in that window captures the disagreement. Reopening it with evm-ss-split=true then fails validateEVMSSPostRecovery (store.go:187) with "EVM SS earliest version %d does not match Cosmos SS earliest version %d", because the check is strict equality and both are > 0. Nothing at generation time flags it; the snapshot publishes, counts as outcome="success", and only fails when someone tries to restore from it.
The existing tests don't cover this because no prune runs in them — TestSnapshotReopensWithEveryVersionBelowLabel reopens with both earliest markers at 0.
This hazard exists today on a crash-restart mid-prune, so the PR amplifies rather than creates it — but it turns a crash-only edge case into a routine outcome for a feature whose product is the restorable image. Options: stamp a consistent earliest into both checkpoints alongside SetCheckpointVersion, or route the earliest write through the same barrier, or (weakest) detect the mismatch at publish time and abandon that boundary rather than publishing a broken image.
|
|
||
| m.publishMu.Lock() | ||
| defer m.publishMu.Unlock() | ||
| defer m.prune() |
There was a problem hiding this comment.
[suggestion] Codex flagged this: defer m.prune() runs even when publication fails after the rename. With keepRecent == 0, keep is 1, so a publish that renames snapshot-20 into place and then fails in syncDir/updateCurrentLink prunes snapshot-10 while current still points at it, leaving a dangling symlink.
I agree the ordering is wrong in principle, but it is not reachable from the config path: AlignSSSnapshotWithSC heals a zero through EffectiveMemIAVLSnapshotCadence, and memiavl.DefaultSnapshotKeepRecent is 1, so production always has keep >= 2 and the pruned entry is never the current target. The manager is constructible with keepRecent: 0 in-package (several tests do), so the invariant is worth making explicit rather than relying on the config healing: skip any version equal to m.lastPublished in prune, so retention can never delete the directory current advertises.
| defer m.publishMu.Unlock() | ||
| defer m.prune() | ||
|
|
||
| if err := os.Rename(tmpDir, finalDir); err != nil { |
There was a problem hiding this comment.
[suggestion] The publication is careful to fsync m.root after the rename, but tmpDir and the intermediate tmpDir/cosmos and tmpDir/evm directories created by os.MkdirAll in requestSnapshot are never synced. Pebble's Checkpoint syncs the checkpoint directory itself, not its parents. A crash right after publication can therefore leave a durable snapshot-NNNNN entry whose cosmos/evm child dirents were not persisted — an empty or partial snapshot that ListSnapshotVersions will happily report and current may point at. Syncing tmpDir (and each intermediate level) before the rename closes the gap and matches the care already taken on the root.
| var skipReason string | ||
| accepted := false | ||
| switch { | ||
| case m.stopped || version <= m.lastRequested: |
There was a problem hiding this comment.
[suggestion] version <= m.lastRequested is folded in with m.stopped and records no ss_snapshot_skipped sample. For the intended case (flush runs twice per block, second pass repeats the version) that's correct. But it's also the state described in the file header for a rollback or a state-sync to a height below existing snapshots: lastRequested stays at the pre-rollback high-water mark and snapshotting stops permanently, silently. The only telemetry is ss_snapshot_current_height going stale, which is indistinguishable from a long quiet period on a chain with an hour-long minTime. A recordSnapshotSkipped("below_high_water_mark") (or a one-shot warn log) when version < m.lastRequested — as opposed to ==, which is the benign repeat — would make the documented "clear the snapshot root by hand" requirement discoverable from a dashboard.
| if s.evmStore != nil { | ||
| evmScheduler, ok = s.evmStore.(types.CheckpointScheduler) | ||
| if !ok || !evmScheduler.SupportsCheckpoint() { | ||
| return fmt.Errorf("EVM backend %q does not support checkpoints", s.config.Backend) |
There was a problem hiding this comment.
[nit] A pebbledb backend that implements Checkpointable but not DrainBarrier (or CheckpointVersionSetter) also reports "does not support checkpoints", since SupportsCheckpoint() collapses all three capabilities into one bool. TestSnapshotManagerRejectsBackendWithoutBarrier pins that misleading string. For an operator staring at a boot failure, naming the missing capability is the difference between "my backend is wrong" and "this is a code bug" — consider returning the specific missing capability from the store side.
| return fmt.Errorf("create snapshot root %q: %w", root, err) | ||
| } | ||
| for _, sourceDir := range sourceDirs { | ||
| probe, err := os.CreateTemp(sourceDir, ".ss-snapshot-link-probe-*") |
There was a problem hiding this comment.
[nit] The probe file is created inside the live Pebble data directory. Pebble ignores filenames it can't parse, so this is safe at runtime, but a crash (or a kill) between CreateTemp and the two os.Remove calls leaves a .ss-snapshot-link-probe-* file in the DB directory forever, with nothing that ever cleans it up — unlike the tmp- staging dirs, which removeStaleTmpDirs handles. Either sweep leftover probes at startup alongside the staging dirs, or probe from a dedicated subdirectory the DB doesn't own.
| // a path remaining present across a retention pass. Until a lease API exists, | ||
| // consumers must stop the node or use external coordination that prevents | ||
| // pruning before they open or copy a snapshot. | ||
| const ( |
There was a problem hiding this comment.
[nit] This 49-line narrative (layout, label semantics, rollback interaction, lease policy) is directly adjacent to const (, so godoc renders the whole essay as the documentation for SnapshotsDirName's const block. AGENTS.md is explicit that godocs describe what a thing is, that multi-paragraph godocs are rare, and that they should document the subject rather than the surrounding system.
The content itself is genuinely valuable and shouldn't be lost — a blank line between the comment and const ( demotes it to a file-level comment, or it could move to a package doc comment. SnapshotsDirName then gets the one-line godoc it actually needs.
A snapshot taken while pruning ran was scrap. Pruning bypasses the apply queues, so no barrier orders a snapshot against it, and it advances the databases one at a time: composite.Prune finishes EVM, compaction included, before it starts the Cosmos scan, which is minutes apart on a large store. A snapshot inside that gap captured a pruned EVM tree beside an unpruned Cosmos one, and reopening it failed the earliest-version agreement check in NewCompositeStateStore. Publication now writes both markers into every checkpoint rather than letting each tree keep whatever it captured. The stamped floor is the highest earliest version any tree has reached, including across the EVM sub-DBs, which prune in parallel and finish at different times, so a snapshot never promises a version one of its trees has already dropped. SetCheckpointVersion becomes SetCheckpointMarkers and writes both markers under one open, so a checkpoint is never left describing half a range. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
A large, carefully-structured feature (opt-in online SS Pebble-checkpoint snapshots) with strong test coverage, fail-closed enablement, and unusually good documentation of its own limits. I found no blocking correctness bugs; the notes below are hardening and observability suggestions, including a downgraded version of Codex's stampedEarliest finding.
Findings: 0 blocking | 12 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion pass (
cursor-review.md) is empty — that review produced no output, so this synthesis merges only Codex's single finding with my own. ScheduleAtDrain's godoc says it "does not block the caller", but it sends on the boundedpendingChangeschannel, so it blocks exactly like a changeset enqueue once the buffer is full. The surrounding docs lean hard on "the commit path never waits"; a clause noting the enqueue itself can block would keep that accurate. (sei-db/db_engine/pebbledb/mvcc/db.go,ScheduleAtDrain)- A checkpoint occupies the backend's apply goroutine for the full WAL flush + fsync + checkpoint.
toml.godocuments this, but there is no signal for it: consider a metric for time the apply queue was blocked by a barrier, sincess-async-write-bufferis one of the keysparseSSConfigsclobbers to 0 on an olderapp.toml(documented inapp/config_fuzz_test.go), which turns that stall into direct commit-path backpressure. verifySnapshotHardlinkscreates a probe file inside each live Pebble data directory. A crash betweenos.Linkand the twoos.Removecalls leaves.ss-snapshot-link-probe-*files behind in the source DB dir and/or the snapshot root, and nothing cleans them up —removeStaleTmpDirsonly handlestmp-prefixed directories. Cheap to sweep them in the same startup pass.- The composite's changelog lives at
<cosmosDB>/changelog, which a Pebble checkpoint does not copy. So opening a published snapshot withOpenDB(as the tests do) creates a fresh changelog inside the "immutable" snapshot directory. Harmless today, but worth a line in the snapshot.go header for consumers, since the layout diagram implies the directory is read-only. AlignSSSnapshotWithSCis invoked at exactly one caller (rootmulti.NewStore) rather than at a choke point every SS-store construction passes through. That is a deliberate trade (the alignment needsscConfig, which the SS constructor does not see), and it is what the inline suggestion oncomposite/store.go:119is meant to backstop.- Positive note: the test suite is thorough for a feature this size — label exactness under concurrent writes, idle EVM sub-DBs, mid-prune marker stamping, cancellation at the barrier, restart resumption, stale staging cleanup, out-of-order publication, retention after a failed publish, and cross-filesystem rejection are all pinned, plus the config characterization rows.
- 5 suggestion(s)/nit(s) flagged inline on specific lines.
| earliest = evmEarliest | ||
| } | ||
| } | ||
| if earliest > label { |
There was a problem hiding this comment.
[suggestion] Clamping earliest down to label publishes a snapshot that advertises a range it may not be able to serve. If pruning overtook the boundary before the queued checkpoint ran, the checkpoint can already be missing data at label, yet the stamped pair [label, label] says otherwise — and the reopen-time agreement check in NewCompositeStateStore will pass, so it is only discovered by whoever reads from the snapshot.
The comment above already contemplates this case ("when retention is short enough for pruning to overtake the boundary"), so rejecting is the more honest handling: abandon the boundary the same way a marker-setting failure does — log, outcome="failure", RemoveAll(tmpDir) — rather than stamping an unsupported range.
Codex rated this High. I'd put it lower: pruning targets latest - keepRecent, so earliest > label needs ss-keep-recent at or near 0 plus a publish delayed past the boundary, which the 100000 default rules out. The fix is the same either way, and it removes the only path where the label stops meaning what the rest of this file says it means.
| return nil, err | ||
| } | ||
|
|
||
| if ssConfig.SnapshotInterval > 0 { |
There was a problem hiding this comment.
[suggestion] Enablement gates on SnapshotInterval > 0, so SnapshotEnable is never read here — the store cannot distinguish "operator left snapshots off" from "operator turned snapshots on but nobody called AlignSSSnapshotWithSC". Since rootmulti.NewStore is the only caller that aligns, any other construction path (seidb tooling, benchmarks, a future caller) that honors ss-snapshot-enable gets silence instead of snapshots.
That is the opposite posture from the rest of this PR, which is deliberately fail-closed everywhere else: the compile-time scheduler assertion, the hardlink preflight, and the unsupported-backend rejection all refuse to boot rather than degrade. An error when ssConfig.SnapshotEnable && ssConfig.SnapshotInterval <= 0 would close the one remaining silent-degradation gap and cost a single condition.
| evmScheduler: evmScheduler, | ||
| snapshotSizes: map[int64]int64{}, | ||
| } | ||
| m.lastRequested = m.newestSnapshotVersion() |
There was a problem hiding this comment.
[suggestion] Startup adopts the newest label on disk as lastRequested without ever comparing it to the live store's latest version. The header comment identifies the two ways that goes wrong — SS rollback, and state-syncing below existing snapshots in a reused home — and correctly says the root must be cleared by hand, but nothing here detects either one: current keeps pointing at a snapshot from the abandoned chain, and no new snapshot is taken until the chain climbs back past the stale label.
s.cosmosStore.GetLatestVersion() is available at this point, so newest > latest is a one-line check. Even just logging an error (rather than refusing boot) would turn a silent multi-interval gap plus a misleading current symlink into something an operator can see.
| var skipReason string | ||
| accepted := false | ||
| switch { | ||
| case m.stopped || version <= m.lastRequested: |
There was a problem hiding this comment.
[nit] m.stopped shares a case with version <= m.lastRequested, and the comment only explains the latter. The two are different events: a repeat is expected and correctly unmetered, while a boundary dropped because the manager is shutting down is a real gap in coverage that produces no skip counter and no log line. A separate case with skipReason = "stopped" would make shutdown-time gaps visible in ss_snapshot_skipped alongside in_flight and minimum_time_interval.
| logger.Error("state store does not schedule snapshots; SS snapshots are disabled", | ||
| "type", fmt.Sprintf("%T", ssStore)) | ||
| } | ||
| store.ssSnapshots = scheduler |
There was a problem hiding this comment.
[nit] This is correct but leans on a subtlety worth making explicit: on a failed type assertion scheduler is the nil interface, so the assignment is what leaves ssSnapshots nil and flush's guard working. Assigning inside an else (or if !ok { logger.Error(...) } else { store.ssSnapshots = scheduler }) says that on purpose instead of relying on the reader knowing the zero value.
Superseded: latest AI review found no blocking issues.
Extracting the marker write moved the int64 to uint64 conversion away from the non-negative check that makes it sound, which gosec reported as G115. The helper now takes the encoded value, so the conversion sits in the function that rejects a negative version. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Well-structured, thoroughly tested, default-off feature: the in-queue drain barrier gives an exact snapshot label without stalling the commit path, the trigger has a single owner in rootmulti.flush, enablement is fail-closed, and shutdown/cancel ordering (scheduling/publishing WaitGroups vs. stopped under one mutex) is correct with no deadlock on Close. No blocking issues found; the notes below are durability/observability polish and one operator-facing documentation gap.
Findings: 0 blocking | 8 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The Cursor second-opinion file (
cursor-review.md) is empty — that pass produced no output. Codex (codex-review.md) reported no material issues, which matches my own read. WaitForPendingWritesis newly exported onCosmosStateStoreandEVMStateStorebut has no non-test caller (onlysettleinsnapshot_test.go). It's a defensible cross-package test seam, but worth noting it widens the production API for test convenience.- Repeated
updateCurrentLinkfailures can leavecurrentdangling:publishstill renames the snapshot into place and leaveslastPublishedbehind, so after enough consecutive link failuresprune(which selects by version, not by whatcurrentpoints at) can remove the target. Self-heals on the next successful publish; low probability, mentioned only for completeness. - Enabling
ss-snapshot-enableon a node whose EVM SS database sits on a different mount turns a previously-bootable node into a startup panic (the hardlink preflight error propagates throughss.NewStateStore, whichrootmulti.NewStorepanics on). This is the intended fail-closed behavior and is documented in the app.toml text; just confirming it's a deliberate boot-blocking change for opted-in operators. - 4 suggestion(s)/nit(s) flagged inline on specific lines.
| var skipReason string | ||
| accepted := false | ||
| switch { | ||
| case m.stopped || version <= m.lastRequested: |
There was a problem hiding this comment.
[suggestion] This case folds two very different situations together. version == m.lastRequested is the expected double-flush per block and rightly records nothing. But version < m.lastRequested can only happen after an SS rollback or a state-sync to a lower height in a reused home directory — and in that case every boundary is silently ignored until the chain climbs back past the old high-water mark, while current keeps pointing at a snapshot from the abandoned chain.
The file-level comment documents this, but there is no runtime signal: ss_snapshot_skipped deliberately doesn't move, and ss_snapshot_current_height just goes stale (which is indistinguishable from a healthy quiet period at the metric). Suggest splitting the strictly-less case out with its own skip reason (e.g. behind_high_water_mark) and a warn log naming the manual remediation, so the "clear the snapshot root by hand" instruction reaches the operator at the moment it applies.
Relatedly, the ss-snapshot-enable help text in sei-db/config/toml.go covers the no-lease and disk-overhead caveats but not this one — worth adding there too, since it's the only place an operator will look.
| defer m.publishMu.Unlock() | ||
| defer m.prune() | ||
|
|
||
| if err := os.Rename(tmpDir, finalDir); err != nil { |
There was a problem hiding this comment.
[suggestion] The fsync chain has a gap around the intermediate directories. Pebble fsyncs each checkpoint dir, writeSnapshotSize fsyncs tmpDir, and syncDir(m.root) fsyncs after the rename — but tmpDir/cosmos and tmpDir/evm (whose entries are the checkpoint directories themselves) are never fsynced. A crash immediately after this rename can therefore leave a fully-named snapshot-N whose subtree entries aren't durable.
Nothing downstream would catch that: newestSnapshotVersion trusts the directory name, startSnapshotManager points current at it and sets lastRequested, so that boundary is never retaken and the damage is only discovered by whoever tries to restore. The rename-into-place design is otherwise exactly right — it just needs either an fsync of the intermediate dirs before the rename, or a completion marker written last and validated at startup, to make the name actually mean "complete".
| } | ||
| cosmosScheduler, ok := s.cosmosStore.(types.CheckpointScheduler) | ||
| if !ok || !cosmosScheduler.SupportsCheckpoint() { | ||
| return fmt.Errorf("cosmos backend %q does not support checkpoints", s.config.Backend) |
There was a problem hiding this comment.
[nit] SupportsCheckpoint() conflates three distinct capabilities (Checkpointable, DrainBarrier, CheckpointMarkerSetter), so this message can name the wrong one. TestSnapshotManagerRejectsBackendWithoutBarrier makes that concrete: the fixture does implement Checkpoint, yet the assertion is on "does not support checkpoints". An operator debugging this would go looking at the wrong capability. Returning which one is missing (or having the store report it) would make the boot rejection self-explanatory.
| return fmt.Errorf("create snapshot root %q: %w", root, err) | ||
| } | ||
| for _, sourceDir := range sourceDirs { | ||
| probe, err := os.CreateTemp(sourceDir, ".ss-snapshot-link-probe-*") |
There was a problem hiding this comment.
[nit] The probe file is created inside a live Pebble data directory on every boot. Pebble ignores unrecognized filenames, so this is safe in steady state, but a crash between CreateTemp and the os.Remove below leaves a .ss-snapshot-link-probe-* file in the database directory indefinitely — visible to operators and to backup tooling that copies the DB dir wholesale. A cleanup sweep for that glob alongside removeStaleTmpDirs would close it. (The probe itself is the right check — comparing st_dev would be cheaper but wrong across bind mounts.)
The earlier note recorded only the data-level effect of an unordered prune and called it harmless, which is true of the data and not of the version markers. Both now sit in one paragraph, so the reason the markers are stamped is next to the reason the data does not need to be. Co-authored-by: Cursor <cursoragent@cursor.com>
1 participant
feat(seidb): add exact-version online state store snapshots
Summary
Add opt-in, online State Store (SS) snapshots. Every snapshot is a set of Pebble checkpoints (hardlink trees) whose label is exact: the image contains every version up to the label and nothing above it. The label is pinned by a barrier placed in each backend's apply queue at enqueue time, so the commit path never waits for a checkpoint. This PR covers generation and retention only; SS rollback is out of scope and does not compose with snapshots yet.
sei-db/db_engine/types/types.go: define the capability contract —Checkpointable,DrainBarrier,CheckpointVersionSetter, and theCheckpointSchedulercomposition.ScheduleCheckpointrejects any backend that lacks a capability instead of degrading.sei-db/db_engine/pebbledb/mvcc/db.go: addScheduleAtDrain, an ordered barrier on the single-writer FIFO. A barrier for version H runs after H is applied and before H+1, without blocking the caller.Checkpointuses a flushed-WAL Pebble checkpoint;SetCheckpointVersionstamps the version marker inside the checkpoint copy only and never touches the live marker.sei-db/state_db/ss/cosmos/store.go,sei-db/state_db/ss/evm/store.go: implement checkpoint scheduling for the Cosmos store, the unified EVM store, and separate EVM sub-databases. An idle sub-database is stamped at the label, which is its correct state for that block. Publication waits for every required checkpoint; the first error wins.sei-db/state_db/ss/composite/snapshot.go: thesnapshotManagerenforces one snapshot in flight, the interval boundary, and the minimum time gate. Publication stages into atmp-directory, renames atomically, fsyncs the parent directory, and moves thecurrentsymlink forward only. A failure past the barrier abandons that boundary rather than relabeling later state; recovery is the next boundary. Startup removes stale staging directories, resumes from the newest snapshot on disk, restorescurrent, and applies retention (1 + keepRecent). Enablement is fail-closed: a backend without checkpoint support, or a live database that cannot hardlink into the snapshot root, rejects boot.sei-db/state_db/ss/composite/store.go,sei-cosmos/storev2/rootmulti/store.go: give the trigger one owner.rootmulti.flushcallsScheduleSnapshotfor every block, populated or empty;ApplyChangesetAsyncdeliberately schedules nothing, so import, recovery, pruning, and benchmark callers cannot publish a partial snapshot. A compile-time assertion pins the capability onCompositeStateStore, so a wrapper that drops the method fails the build.sei-db/config/ss_config.go,sei-db/config/sc_config.go,sei-db/config/toml.go,app/seidb.go,sei-cosmos/server/config/config.go: one operator knob,ss-snapshot-enable, defaultfalse. Both read sites guard the key, so anapp.tomlrendered before this key existed keeps the default. When enabled, SS mirrors state-commit's effective snapshot interval, minimum time interval, and retention throughAlignSSSnapshotWithSC; the shared cadence helper heals unset SC values to the memIAVL defaults, so a raw zero can never silently disable SS snapshots. The three derived fields aremapstructure:"-"and cannot be set by any key.sei-db/state_db/ss/composite/snapshot_metrics.go: exportss_snapshot_*metrics — attempts, skips by reason, completions by outcome, duration, in-flight, published height, retained count, and apparent bytes (persisted per snapshot in an.apparent-sizefile).sei-db/common/utils/path.go: default snapshot root is<home>/data/state_store/snapshots; a custom SS directory<db>moves it to the sibling<db>-snapshotsso hardlinks stay on one filesystem. Managed snapshot directories have no lease; consumers must coordinate with generation and pruning.Test plan
sei-db/state_db/ss/composite/snapshot_test.go: label exactness (every version below the label present, versions written after the boundary excluded while writes continue), empty boundary blocks, idle EVM sub-databases, Cosmos-only and EVM-split layouts, one-in-flight and minimum-time gates, cancellation at the barrier during close, restart resumption from the newest snapshot, stale staging cleanup, out-of-order publication, retention after a failed publish, hardlink preflight rejection across filesystems, and rejection of backends without checkpoint or barrier support.sei-cosmos/storev2/rootmulti/store_test.go:flushschedules a snapshot at a boundary for both a populated and an empty block, pinning the trigger at its single owner.sei-db/db_engine/pebbledb/mvcc/db_test.go: a checkpoint stamp preserves the live version marker, and a queued checkpoint cancels before work starts.sei-db/config/ss_config_test.go,sei-db/config/toml_test.go,app/config_fuzz_test.go,sei-cosmos/server/configgoldens: default-off rollout, guarded reads, cadence mirroring with zero-healing, and the derived fields pinned as unreachable-by-key.go test -raceonsei-db/state_db/ss/composite,sei-db/config,sei-db/db_engine/pebbledb/mvcc, andsei-cosmos/storev2/rootmulti, plus the configuration characterization suites;gofmt -sandgoimportsare clean on all changed files.