StateWAL + FlatKV - #3835
Conversation
PR SummaryHigh Risk Overview Commit semantics are narrowed to one block per
Reviewed by Cursor Bugbot for commit 8374d79. Bugbot is set up for automated code reviews on this repo. Configure here. |
![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 ad367c4. Configure here.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3835 +/- ##
==========================================
- Coverage 60.18% 59.65% -0.53%
==========================================
Files 2327 2271 -56
Lines 194602 188696 -5906
==========================================
- Hits 117112 112558 -4554
+ Misses 66918 65993 -925
+ Partials 10572 10145 -427
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
This comment was marked as low quality.
This comment was marked as low quality.
Sorry, something went wrong.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // range; GetRange reads it offline without a live WAL instance. | ||
| func verifyClonedWALCovers(dstChangelogDir string, snapshotVersion int64) error { | ||
| walLog, err := wal.NewChangelogWAL(dstChangelogDir, wal.Config{}) | ||
| if err != nil { | ||
| return fmt.Errorf("open cloned changelog for validation: %w", err) | ||
| } | ||
| defer func() { _ = walLog.Close() }() | ||
|
|
||
| firstOff, err := walLog.FirstOffset() | ||
| if err != nil { | ||
| return fmt.Errorf("cloned changelog first offset: %w", err) | ||
| } | ||
| lastOff, err := walLog.LastOffset() | ||
| ok, firstVer, lastVer, err := statewal.GetRange(statewal.DefaultConfig(dstChangelogDir, flatkvStateWALName)) | ||
| if err != nil { | ||
| return fmt.Errorf("cloned changelog last offset: %w", err) | ||
| return fmt.Errorf("read cloned changelog range: %w", err) | ||
| } | ||
| if firstOff == 0 || lastOff == 0 || firstOff > lastOff { | ||
| if !ok { | ||
| return nil | ||
| } | ||
|
|
||
| firstVer, err := readWALEntryVersion(walLog, firstOff) | ||
| if err != nil { | ||
| return fmt.Errorf("read first cloned changelog entry: %w", err) | ||
| } | ||
| lastVer, err := readWALEntryVersion(walLog, lastOff) | ||
| if err != nil { | ||
| return fmt.Errorf("read last cloned changelog entry: %w", err) | ||
| } | ||
|
|
||
| if lastVer <= snapshotVersion { | ||
| if int64(lastVer) <= snapshotVersion { //nolint:gosec // version fits int64 | ||
| return nil | ||
| } | ||
| if firstVer <= snapshotVersion+1 { | ||
| if int64(firstVer) <= snapshotVersion+1 { //nolint:gosec // version fits int64 | ||
| return nil | ||
| } | ||
| return fmt.Errorf("%w: cloned WAL starts at version %d but snapshot is %d (truncated past snapshot mid-clone)", | ||
| errSourceChurning, firstVer, snapshotVersion) | ||
| } |
There was a problem hiding this comment.
🟡 verifyClonedWALCovers in flatkv_open.go rejects a cloned WAL as "churning" whenever firstVer > snapshotVersion+1, but it's missing the committedVersion==0 clamp that catchup/replayInto/rollbackBaseVersion all apply elsewhere in this PR. When the only snapshot is snapshot-0 and the WAL legitimately begins above block 1 (e.g. chain initial_height > 1), this check deterministically fails every retry and the tooling permanently aborts with 'source kept churning', even though the production LoadVersion path opens and replays such a store fine. Fix: when snapshotVersion == 0, accept any firstVer.
Extended reasoning...
verifyClonedWALCovers (sei-db/tools/cmd/seidb/operations/flatkv_open.go:214-231) is a pre-flight check in the flatkv tooling-clone path. It reads the cloned state WAL's stored block range and rejects the clone as errSourceChurning whenever firstVer > snapshotVersion+1, on the theory that a live writer truncated the WAL past the snapshot mid-clone (a genuine race worth retrying).
However, this check does not account for the case where snapshotVersion == 0 — i.e. the only snapshot present is snapshot-0, and the WAL's own history legitimately begins above block 1. This is a real, PR-supported topology: a chain with initial_height > 1, or a FlatKV store materialized mid-chain via CommitBlock, has no snapshot other than the initial one but a WAL that starts at, say, block 10. This exact shape is pinned by this PR's own tests — TestReadOnlyAcceptsMidChainWALStart and TestRollbackAcceptsMidChainWALStart / rollbackFixtureMidChainWALStart — and is handled correctly by the production replay paths: catchup, replayInto, and rollbackBaseVersion all explicitly clamp their expected starting block to the WAL's first stored block instead of committedVersion+1 when committedVersion == 0.
verifyClonedWALCovers omits this clamp. Since selectFlatKVSnapshot will pick snapshot-0 for any requested height on such a store, snapshotVersion is always 0, and the check becomes firstVer(10) <= snapshotVersion+1(1) — false — so it unconditionally returns errSourceChurning.
The step-by-step proof, using exactly the topology TestReadOnlyAcceptsMidChainWALStart constructs: a store whose only snapshot is snapshot-0, with the WAL holding blocks 10-12.
- Tooling calls
tryPrepareFlatKVToolingClone(dbDir, height)for some height >= 10. selectFlatKVSnapshotresolves to snapshot-0 (the only snapshot at or below the target), sosnapshotVersion = 0.- The WAL is cloned;
verifyClonedWALCovers(dstChangelogDir, 0)reads the cloned WAL's range:firstVer=10, lastVer=12. lastVer(12) <= snapshotVersion(0)? No.firstVer(10) <= snapshotVersion+1(1)? No.- Returns
errSourceChurning.
isCloneRetryableError treats errSourceChurning as retryable, so prepareFlatKVToolingCloneWith retries up to maxCloneRetries (3) times. Because the on-disk state driving this decision hasn't actually changed between attempts — there's no live writer truncating anything — every retry evaluates the exact same deterministic condition and fails identically. The function then returns a permanent clone aborted after 3 retries, source kept churning error, and any seidb flatkv-open / state-size-analysis invocation against such a store is permanently unusable, even though the store itself opens and replays perfectly fine through the normal LoadVersion / catchup path this checker is supposed to validate against.
The fix mirrors what the PR already does at every other replay-adjacent call site: when snapshotVersion == 0, there is no snapshot-relative gap to check for, since catchup will treat the WAL's own first block as the start of history rather than expecting committedVersion+1. So verifyClonedWALCovers should accept any firstVer when snapshotVersion == 0 (matching the committedVersion == 0 clamp in catchup, replayInto, and the newly-added rollbackBaseVersion).
This is scoped entirely to offline diagnostic tooling (flatkv_open, state-size analysis) — it does not affect node startup, consensus, or the live commit/replay path, since verifyClonedWALCovers is only invoked from the tooling clone preparation flow. No data is at risk; the failure mode is a permanent, misleading tooling error on an otherwise-healthy store.
There was a problem hiding this comment.
Clean, well-documented replacement of FlatKV's tidwall changelog with the new StateWAL — the Rollback reachability pre-check, the gap detection in catchup/replayInto, and the new contiguity tests are real improvements. The blocking gap is that the new WAL reuses the existing changelog/ directory with no migration or detection of the legacy files, so pre-upgrade WAL history is silently unreachable (breaking rollback/reconcile/historical-export in the window after upgrade) and the old segments leak on disk forever.
Findings: 1 blocking | 9 non-blocking | 6 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
cursor-review.mdis empty — the Cursor pass produced no output, so this review is the synthesis of Claude + Codex only.- Test-coverage regression: deleting
wal_torn_write_test.goremoves the only FlatKV-level test for the crash window the WAL exists to cover — "WAL record durable, per-DB batch not committed" — and for a torn WAL tail.TestCatchupRecoversGappedCommitBlockAfterMetadataLagis not a substitute (its DB batch did commit; only the global watermark lagged). seiwal/statewal cover torn tails at their own layer, but the integration guarantee is now untested. It's cheap to re-add:s.wal.Write/SignalEndOfBlock/Flushat v+1,clearPendingWrites,Close, reopen, assert the version advanced andverifyLtHashConsistencyholds. - Contract change worth confirming: a
Committhat fails after the WAL write (e.g.commitBatchesreturns an error) can no longer be retried at the same height —enforceWriteOrderingrejects a secondWriteto an already-ended block with "block number N has already ended". The old changelog tolerated a duplicate append at the same version (catchup skipped it viaentry.Version <= committedVersion). This is fine if every commit failure is fatal/panics, but the store is now permanently wedged for that height if anything upstream retries. - Positive note, no action needed: moving the reachability check (
rollbackBaseVersion) ahead ofcloseDBsOnlyso an impossible target is refused before anything irreversible happens is a genuine correctness improvement over the old "truncate, then discover we can't reach the target" flow, as is thefirst > startBlockhard failure incatchupinstead of silently replaying over a hole. - 5 suggestion(s)/nit(s) flagged inline on specific lines.
| // subdirectory of the data dir, with a fixed instance name used only to label metrics. It is the single | ||
| // definition of that layout convention. | ||
| func stateWALConfig(cfg *config.Config) *statewal.Config { | ||
| return statewal.DefaultConfig(filepath.Join(cfg.DataDir, changelogDir), "flatkv") |
There was a problem hiding this comment.
[blocker] Blocking: the new WAL adopts the legacy changelog/ directory but neither reads, migrates, nor removes what is already there. (Codex flagged this too; I agree and can add the mechanism.)
seiwal only recognizes files matching ^(\d+)-(\d+)-(\d+)\.wal$ / ^(\d+)\.wal\.u$ (seiwal_file.go:46-47), and recoverOrphans/scanSealedFiles skip anything else. tidwall's 20-digit extension-less segments therefore match nothing, so on the first start after upgrade:
- The WAL reads as empty. Forward operation is fine (PebbleDB already holds the state, and an empty WAL accepts any first block), but every operation that needs history between the last snapshot
Sand the upgrade heightNnow fails. Concretely, a crash in the window between the twoCommitcalls inCompositeCommitStore.Commitleaves flatkv atN+1and cosmos atN;reconcileVersions→Rollback(N)→rollbackBaseVersionfindsbaseVersion = S != Nand the WAL holding only[N+1, N+1], so it returns "cannot roll back to version N: nearest snapshot is S, so blocks S+1-N are needed, but the WAL only holds N+1-N+1" and the node will not start. Same forseid rollbackand historical export to any height in(S, N]. - The old segments leak permanently.
Prune/PruneAfter/Deleteall operate throughparseFileName, so the pre-upgrade segments are never pruned and never deleted — dead bytes on the data volume for the life of the node.
Pick one and make it explicit rather than silent:
- write the state WAL to a new subdirectory (e.g.
statewal/) and remove the legacychangelog/on first open, or - keep the path but have
OpenStateWALscan for unrecognized entries and either delete them or fail loudly with a migration message, or - if the reduced history boundary is an accepted cost, reset it deliberately (log it, and document that a rollback across the upgrade height is unsupported) so it doesn't surface as a startup failure at 3am.
This is unrelated to §1/§2 of REVIEW_GUIDELINES.md — it's an on-disk format change, not a version gate.
| // iteration then proceeds lock-free, because a seiwal iterator reads a consistent point-in-time (hard-link) | ||
| // snapshot that concurrent appends/prunes cannot disturb. A concurrent Commit is the only overlap this | ||
| // tolerates: closing the primary while an export is in flight is not permitted (see Close). | ||
| func (s *CommitStore) replayInto(clone *CommitStore, targetVersion int64) (retErr error) { |
There was a problem hiding this comment.
[suggestion] The read-only clone now depends on the primary's WAL for the whole of LoadVersion(v, true), where previously openReadOnly opened its own changelog handle on the shared directory and was fully independent of the primary's lifetime.
The doc here and on Close says closing the primary concurrently "is not permitted", but nothing enforces it: state-sync snapshot export calls rootmulti → LoadVersion(version, true) from a background goroutine, so a shutdown that races it lands inside replayInto and gets "readonly: WAL range: state WAL is closed" (or a mid-iteration failure). Not a crash, but a confusing export failure that the old code structurally couldn't have.
Since replayInto already takes s.mu for range+iterator construction, consider either holding it for the whole replay, or having Close set a closing flag under s.mu that replayInto checks and reports as a clear "store is shutting down" error — cheaper than relying on callers to quiesce work they don't drive.
| if !s.manageWAL { | ||
| return nil | ||
| } | ||
| cfg := stateWALConfig(&s.config) |
There was a problem hiding this comment.
[suggestion] resetWAL (and the identical block in Rollback, snapshot.go:677) reconstructs the WAL location from stateWALConfig(&s.config) rather than from the instance that was injected. That makes the dependency injection only half-real: the close→delete/prune→reopen cycle silently rebinds the store to s.config.DataDir/changelog regardless of where the injected WAL actually lives.
Every current caller passes OpenStateWAL(cfg) with the same cfg, so it's correct today — but the stated direction is "managing the WAL entirely outside FlatKV", and the first caller that injects a WAL from a different path will get its data silently replaced by an unrelated directory. Worth closing now while the seam is being defined: inject a *statewal.Config (or a factory func() (statewal.StateWAL, error)) alongside the instance, or have StateWAL expose its own path.
| } | ||
| }() | ||
|
|
||
| // TODO: this should also truncate the WAL to targetVersion, the way Rollback |
There was a problem hiding this comment.
[suggestion] Good that this is written down rather than left implicit. One request: the API gives the caller no signal. LoadVersion(N, false) returns a Store that looks writable and then fails at the next Commit with a WAL contiguity error that reads like corruption.
Today the only non-zero writable caller is the export path (cmd/seid/cmd/root.go:358 → app.LoadHeight → rootmulti.LoadVersionAndUpgrade), which never commits, so nothing is broken. Until the TODO lands, consider either marking the returned store readOnly for targetVersion > 0, or documenting the failure mode in the returned error path — otherwise the first future caller that tries to resume committing from a historical height debugs a WAL error instead of an API contract.
| // record durable. An empty block (no ApplyChangeSets) writes an empty but contiguous record. Skipped | ||
| // entirely when the WAL is nil — the outer context then owns the WAL pipeline. | ||
| if s.wal != nil { | ||
| s.phaseTimer.SetPhase("commit_write_wal") |
There was a problem hiding this comment.
[nit] Phase label renamed commit_write_changelog → commit_write_wal. If any dashboard, alert, or profiling query keys on the old phase name it breaks silently on deploy. Worth a line in the PR description / release notes.
| // decoupled from the parent's. | ||
| ro, err := NewCommitStore(context.Background(), &s.config) | ||
|
|
||
| ro, err := NewCommitStore(context.Background(), &s.config, nil) |
There was a problem hiding this comment.
[nit] The behavior is unchanged (context.Background()), but the comment explaining why the clone must not derive from s.ctx — that rooting it at the parent aborts in-flight reads with "context canceled" the moment the parent closes, while the caller may still hold the returned view — was deleted along with the changelog-related lines. That rationale isn't recoverable from the code, and it's exactly the kind of thing someone "tidies up" into s.ctx later. Suggest keeping it.
1 participant
Describe your changes and provide context
Replace FlatKV WAL with new StateWAL impl