Add offline rollback utility for LittDB#3684
Conversation
Adds rollback.RollbackLittDB(dataDirs, rollbackFilter): a static, offline utility that rewinds a LittDB instance to a chosen point — e.g. to roll a node's state back to a specific block height while the database is stopped. For each table it walks the key files newest-to-oldest, invoking the filter once per record (isPrimary is derived from the record's KeyKind). The first record the filter accepts is the rollback point: that record's whole key group and everything written before it are kept; everything written after the group is permanently removed from the segment files. - segment.Segment.RollbackToKeyCount truncates a sealed segment in place: atomic key-file swap, value-file truncation, metadata key-count update. - The orchestrator locks the data dirs (so it won't touch a live DB), then discards the table's keymap and snapshot, deletes whole newer segments (highest index first), and truncates the rollback segment. The keymap and snapshot are derived state and are rebuilt from the truncated segments on the next start, which keeps them consistent with the rolled-back data — the same approach cli/prune.go uses to finalize an offline mutation. Steps are ordered for crash-safety: discarding the keymap first ensures the DB always rebuilds it from whatever segment state exists rather than trusting a keymap that points into truncated data. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
PR SummaryHigh Risk Overview
Unit tests cover mid-history rollback, filter edge cases, secondary key groups, GC watermark guard, and idempotent re-runs. Reviewed by Cursor Bugbot for commit e6f8f63. 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).
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3684 +/- ##
==========================================
- Coverage 59.48% 58.54% -0.94%
==========================================
Files 2275 2190 -85
Lines 188818 179173 -9645
==========================================
- Hits 112312 104894 -7418
+ Misses 66429 64974 -1455
+ Partials 10077 9305 -772
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.
A well-tested offline LittDB rollback utility, but two crash/edge-case correctness gaps remain: rollback ignores the durable gc-watermark sidecar (which can make the DB unstartable after a rollback below the watermark), and RollbackToKeyCount's early-return breaks its own idempotency guarantee if interrupted mid-operation.
Findings: 2 blocking | 4 non-blocking | 2 posted inline
Blockers
- gc-watermark not reconciled during rollback (sei-db/db_engine/litt/rollback/rollback.go, discardDerivedState / rollbackTable): the durable
<root>/<table>/gc-watermarkfile recordslowestReadableSegmentand is loaded at startup (disk_table.go:229-256). Rollback deletes/truncates high-index segments but never touches this file. If the rollback point lands at or below the recorded watermark, the newhighestSegmentIndexfalls belowlowestReadableSegment, and startup fails withgc-watermark ... exceeds highest segment ... on disk— leaving the DB unstartable. Rollback should clamp the watermark down to the new highest surviving segment (it cannot simply delete it, since below-watermark segments still on disk would resurrect GC'd keys). - 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- REVIEW_GUIDELINES.md is empty/missing, so no repo-specific standards were applied.
- cursor-review.md is empty — the Cursor second-opinion pass produced no output.
- Consider a targeted test for the crash-recovery/idempotency path (interrupt after key-file swap, rerun) and for interaction with a previously-advanced gc-watermark; current tests exercise only clean, complete rollbacks.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| // snapshot on its next start, so deleting them forces both back into sync with the truncated segments. | ||
| // Leaving them would let a stale keymap reference discarded keys, or let a snapshot's hard links pin the | ||
| // rolled-back data on disk. Removing a directory that does not exist is a no-op. | ||
| func discardDerivedState(roots []string, tableName string) error { |
There was a problem hiding this comment.
[blocker] discardDerivedState removes the keymap and snapshot (hard-link) directories but not the durable gc-watermark sidecar, which lives at <root>/<tableName>/gc-watermark (see GCWatermarkFileName, loaded per-root in disk_table.go:229). The watermark records lowestReadableSegment; after a rollback deletes/truncates high-index segments, the recorded value can exceed the new highestSegmentIndex, and startup then refuses to open the table (gc-watermark (lowest readable segment N) exceeds highest segment M on disk, disk_table.go:252-257), making the DB unstartable. Reconcile the watermark here: clamp it down to the new highest surviving segment index for each root when it exceeds that value. (Removing it outright is unsafe — below-watermark segments still on disk would resurrect GC-collected keys.)
There was a problem hiding this comment.
It's possible you started this branch before my keymap threading optimizations landed. The gc-watermark file was something introduced in my branch.
| return fmt.Errorf("surviving key count %d exceeds the %d records in segment %d", | ||
| survivingKeyCount, len(keys), s.index) | ||
| } | ||
| if int(survivingKeyCount) == len(keys) { |
There was a problem hiding this comment.
[suggestion] This early return runs after the atomic key-file swap (step 1) but before value-file truncation (step 2) and the metadata keyCount rewrite (step 3). If the process is interrupted between step 1 and steps 2/3, re-running the same rollback reads the already-truncated key file, sees survivingKeyCount == len(keys), and returns nil — so the value files keep their trailing dead bytes (leaked disk) and metadata.keyCount stays inflated. Note keyCount is read verbatim from metadata for already-sealed segments on load (segment.go:236; sealLoadedSegment, which recomputes it, is skipped when metadata.sealed), so the stale count is not self-corrected — contradicting the idempotency guarantee documented in this method and in RollbackLittDB. Consider detecting an incomplete prior run (e.g. value-file size larger than the surviving prefix, or metadata keyCount != len(keys)) and finishing steps 2/3 rather than returning early.
cody-littley
left a comment
cody-littley
left a comment
There was a problem hiding this comment.
LGTM once comments are addressed
| // snapshot on its next start, so deleting them forces both back into sync with the truncated segments. | ||
| // Leaving them would let a stale keymap reference discarded keys, or let a snapshot's hard links pin the | ||
| // rolled-back data on disk. Removing a directory that does not exist is a no-op. | ||
| func discardDerivedState(roots []string, tableName string) error { |
There was a problem hiding this comment.
Optional:
Instead of dropping the keymap directory wholesale, we can prune the keymap to avoid the cost of rebuilding it. To do this, we'd iterate the keys we are removing, and issue batch deletion operations to the keymap. We'd need to make sure we delete the keys before the segments though, in order to ensure crash safety.
This may be a premature optimization, as rolling back LittDB data is likely going to be rare. I'll leave it up to you if you want to do this or not.
There was a problem hiding this comment.
Kept the wholesale drop-and-rebuild since rollback is rare. happy to switch to keymap pruning later if it ever matters
| // snapshot on its next start, so deleting them forces both back into sync with the truncated segments. | ||
| // Leaving them would let a stale keymap reference discarded keys, or let a snapshot's hard links pin the | ||
| // rolled-back data on disk. Removing a directory that does not exist is a no-op. | ||
| func discardDerivedState(roots []string, tableName string) error { |
There was a problem hiding this comment.
It's possible you started this branch before my keymap threading optimizations landed. The gc-watermark file was something introduced in my branch.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 3 potential issues.
There are 4 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a5c712d. Configure here.
| for _, table := range tables { | ||
| if err := rollbackTable(logger, roots, table, rollbackFilter); err != nil { | ||
| return fmt.Errorf("failed to roll back table %q: %w", table, err) | ||
| } |
There was a problem hiding this comment.
Partial multi-table rollback
Medium Severity
RollbackLittDB rolls back tables sequentially and returns on the first error. Tables processed earlier have already had derived state removed and segments truncated, while later tables may remain at the pre-rollback revision, leaving the database in a mixed-epoch state with no automatic recovery.
Reviewed by Cursor Bugbot for commit a5c712d. Configure here.
There was a problem hiding this comment.
A well-structured offline LittDB rollback utility with careful crash-ordering and good test coverage; no confirmed correctness bugs, but the stateless filter cannot express "delete an entire table," which is a real gap for the stated block-height use case, plus a couple of minor robustness notes.
Findings: 0 blocking | 6 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Codex's second concern (rollback ignoring a gc-watermark / durable-readable floor) appears mitigated here: boundary/lower-bound files are a snapshot-only concept and rollbackTable explicitly refuses symlinked snapshots, while the main store re-derives its segment range from disk on reopen (GatherSegmentFiles/scanDirectories) and the keymap is rebuilt — so deleting the newest segments does not leave a dangling high-water pointer. Worth a confirming glance, but not blocking.
- The Cursor second-opinion review (cursor-review.md) was empty — that pass produced no output.
- RollbackLittDB rolls back every table found under the roots unconditionally; unlike cli/prune.go it offers no table allowlist. For a whole-node height rollback that is the intent, but consider exposing table scoping to reduce blast radius for narrower operational use.
- No prompt-injection or malicious content detected in the PR title, description, or diff.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
| if err != nil { | ||
| return err | ||
| } | ||
| if pivot == nil { |
There was a problem hiding this comment.
[suggestion] Semantics gap for the block-height use case: the filter is stateless per-key and can only say "keep from here down." A table whose keys are ALL newer than the rollback target never matches, so pivot == nil leaves it fully intact — the opposite of what a height rollback wants (that data should be deleted). The filter cannot distinguish "no match because everything is older (keep all)" from "no match because everything is newer (delete all)."
In practice most Sei tables are created at genesis and will always have a key at/before the target, so this may not trigger — but a table that first received writes after the target height would silently retain post-rollback state, risking an inconsistent app hash. Consider either documenting that the caller must guarantee every table has at least one record at/before the target, or extending the API so a caller can signal "delete this whole table."
| return fmt.Errorf("surviving key count %d exceeds the %d records in segment %d", | ||
| survivingKeyCount, len(keys), s.index) | ||
| } | ||
| if int(survivingKeyCount) == len(keys) { |
There was a problem hiding this comment.
[nit] Interruption edge case: if a prior run crashed after the atomic key-file swap (step 1) but before truncating the value files (step 2) / writing metadata (step 3), a re-run reads the already-truncated key file, so survivingKeyCount == len(keys) here and it returns early — leaving the value files over-sized and metadata.keyCount stale permanently. This is not corruption (the trailing value bytes are unreferenced and keyCount only feeds metrics), but the docstring's idempotency claim ("self-corrects on the next run") doesn't fully hold for these two artifacts. Worth a one-line caveat in the comment.
seidroid
Bot
dismissed
their stale review
Superseded: latest AI review found no blocking issues.
There was a problem hiding this comment.
A well-tested offline LittDB rollback utility; the core walk/truncate/discard logic is sound. Two edge-case robustness issues carried over from the Codex review: rollback leaves the durable gc-watermark sidecar untouched (can block DB startup when rolling back below the watermark), and RollbackToKeyCount is not actually self-correcting after an interrupted run, contradicting its own doc comment.
Findings: 0 blocking | 5 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion review (cursor-review.md) was empty — that pass produced no output.
- No test exercises rollback interaction with a defined gc-watermark or a table with a TTL/GC history (P1), nor an interrupted-then-resumed rollback (P2). The idempotency claim in the RollbackLittDB doc comment is currently unverified by tests.
- Consider having RollbackLittDB reset/remove the gc-watermark (or clamp lowestReadableSegment to the new highest segment) as part of discarding derived state, so all durable state that constrains startup is reconciled together after a rollback.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
| for _, root := range roots { | ||
| dirs := []string{ | ||
| filepath.Join(root, tableName, keymap.KeymapDirectoryName), | ||
| filepath.Join(root, tableName, segment.HardLinkDirectory), |
There was a problem hiding this comment.
[suggestion] discardDerivedState removes the keymap and snapshot dirs but leaves each table's gc-watermark file (disktable.GCWatermarkFileName) intact. That file is durable, survives keymap rebuild, and is read at startup (disk_table.go:242). Because rollback deletes the highest segments, if a table's watermark was previously advanced (TTL/GC) to a lowestReadableSegment above the new highestSegmentIndex, startup hits the lowestReadableSegment > highestSegmentIndex guard (disk_table.go:252) and refuses to open the DB. Rolling back at/below the watermark can also cause surviving older segments to be skipped as logically deleted. Recommend removing (or clamping) the gc-watermark here alongside the other derived state.
| return fmt.Errorf("surviving key count %d exceeds the %d records in segment %d", | ||
| survivingKeyCount, len(keys), s.index) | ||
| } | ||
| if int(survivingKeyCount) == len(keys) { |
There was a problem hiding this comment.
[suggestion] The commit point is the atomic key-file swap in step 1, but the metadata keyCount write is step 3. If the process is interrupted between step 1 and step 3, a re-run reaches this early return (survivingKeyCount == len(keys) is now true because the key file was already rewritten) and never repairs metadata.keyCount — nor truncates the value files (step 2). Since startup sums seg.KeyCount() into the table's public KeyCount() (disk_table.go:186, asserted in the rollback tests), the stale/inflated count persists indefinitely rather than self-correcting. This contradicts the RollbackLittDB doc comment (rollback/rollback.go:45-47) that says an interrupted run's stale key count "self-corrects on the next run." Consider reconciling metadata/value files even when the key count already matches, or adjust the doc comment.
…t truncation - RollbackFilter now receives the table name (per @cody-littley's review): key schemas differ across tables, so the filter must decode keys table-aware. - Refuse to roll a table back below its durable gc-watermark. Segments below it were logically garbage collected; a target there cannot be faithfully reconstructed and would leave lowestReadableSegment above the highest surviving segment, which the DB rejects at startup. The watermark is left in place (not deleted) so a keymap rebuild does not resurrect collected keys. - Segment.RollbackToKeyCount no longer early-returns when the key file already holds exactly the surviving records: it still truncates the value files and rewrites the metadata key count, so a run interrupted after the atomic key-file swap is repaired on re-invocation. This makes the documented idempotency guarantee actually hold. - Tests: per-table filter (table-name assertion), gc-watermark refusal leaves the table untouched, and idempotent re-run. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Adds a well-structured, carefully documented offline rollback utility for LittDB with good test coverage; I found no correctness or security issues after tracing the core invariants (group atomicity within a segment/shard, per-shard value-prefix truncation, crash-repair idempotency, gc-watermark guard, directory locking) against the actual codebase. Only minor non-blocking notes remain.
Findings: 0 blocking | 3 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Test coverage gap: no test exercises the rollback filter returning true on a secondary key. All existing tests match only on primary keys, so the
groupEndIndexforward-scan starting from a mid-group secondary index (finding its terminating KeyKindFinalSecondary) is never directly covered. Worth adding a case that stops on a secondary to lock in that path. - The PR describes RollbackLittDB as an operational "tool", but only a library function is added — there is no CLI entrypoint (e.g. under cli/). Confirm the intended invocation path is a follow-up or that it is called programmatically.
- Second-opinion passes produced no findings: Codex could not build (no Go 1.25.6 / no network in its sandbox) and the Cursor review file was empty; REVIEW_GUIDELINES.md was also empty. I was likewise unable to build/run the tests locally, so
go test ./sei-db/db_engine/litt/rollback/...should be confirmed green in CI before merge (the PR notes on-node verification is still in progress).
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
There was a problem hiding this comment.
A well-structured, well-tested offline rollback utility for LittDB. No blocking issues found; two non-blocking durability/consistency notes remain, both consistent with existing patterns in the codebase.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Durability:
discardDerivedState(os.RemoveAll), the whole-segmentutil.DeepDeletecalls, and the value-fileos.TruncateinRollbackToKeyCountdo not fsync the affected parent directories. The key-file swap is durable (AtomicRenamefsyncs the dir), and idempotency repairs a run interrupted mid-operation, but a host crash immediately afterRollbackLittDBreturns could lose the directory-entry removals/truncations and resurrect rolled-back keys or a stale keymap. This matches the existingcli/prune.gobehavior (which also uses un-fsyncedos.RemoveAll), so it is not a regression — but for a state-rollback tool it would be worth an explicit fsync of the segments dir / table dir after deletion, or at least documenting that the operator should confirm a clean flush before restarting. (Raised by Codex.) - The Cursor second-opinion review file (
cursor-review.md) was empty — that pass produced no output. The Codex review file was present and both of its findings are reflected here. - Consider a brief note in the package/function docs that groups (primary + secondaries) are guaranteed to reside within a single segment (verified:
Segment.Writeassigns ones.indexto the wholePut), sincefindRollbackPoint/groupEndIndexcorrectness depends on that invariant —groupEndIndexreturns an error if it cannot find aFinalSecondarywithin one segment's keys. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
[suggestion] RollbackToKeyCount updates s.metadata.keyCount but not the separate in-memory s.keyCount field that the exported KeyCount() returns (see segment.go: func (s *Segment) KeyCount() uint32 { return s.keyCount }). Within this PR the segment object is discarded right after rollback so it's harmless today, but since this is an exported method any future caller that keeps using the loaded segment would get a stale count. Recommend also setting s.keyCount = survivingKeyCount here (and/or documenting that the segment must be reloaded after rollback). (Raised by Codex.)
2 participants
Describe your changes and provide context
Testing performed to validate your change