Skip to content

MB-72716: Fix miscalculation of rollback points#2370

Merged
CascadingRadium merged 28 commits into
masterfrom
rollback
Jul 6, 2026
Merged

MB-72716: Fix miscalculation of rollback points#2370
CascadingRadium merged 28 commits into
masterfrom
rollback

Conversation

@CascadingRadium

@CascadingRadium CascadingRadium commented Jul 3, 2026

Copy link
Copy Markdown
Member
  • We currently mark any bolt snapshot that is beyond the expiry window, but within the retention window as live and pass it on to the time series mechanism that will protect/retain the number of snapshots configured in scorch - NumSnapshotsToKeep
  • However when computing the extended boundary, we skip over the first snapshot that was after the expiry window. We basically ignore it and continue to track more snapshots up to the cutoff time determined by the first expired snapshot's boundary checkpoint.Fixed this issue and added a unit test.
B                      L E                     T
+----------------------+-+---------------------+
|    Retention Window  + |    Expiry Window    |
+----------------------+-+---------------------+ ---> time


- N  = Number Of Snapshots to keep
- D  = Rollback Sampling Interval 
- R  = Rollback Retention Factor
- T  = Current timestamp
- C  = Current set of checkpoints
- E  = Expiry timestamp    = T - [(N - 1) * D]
- B  = Boundary Checkpoint = f(C, R, Lt)
- L  = First expired snapshot
- Lt = L's timestamp
- Nr = Number of snapshots in the retention window
- Ne = Number of snapshots in the expiry window

If (Nr >= N && Ne == 0 && Lt < E):
Then 
    persisted snapshots = [{Nr} - L}]
    protected snapshots ∩ L = ∅
  • rollbackRetentionFactor config validation was broken and errored on valid input instead of invalid. Corrected the condition and added a 0 ≤ r ≤ 1 bounds check.
  • Retention settings weren't applied when loading existing snapshots on open, because we opened boltDB before rollback settings were parsed from config. This resulted in the first set of checkpoints to have a timestamp of 0, due to which the rollback points used before the restart were lost. Fixed this issue and added a unit test as well.
  • Removed a redundant closeCh select after handing a persist off to the introducer, the introducer always completes and closes applies the persist unconditionally, so that branch was dead code.
  • Ensured that the persister/merge planner options were parsed exactly once, instead of being scattered across multiple places.
  • Shifted rollback config to object construction time instead of setting it when opening the index to ensure that restart scenarios do not use null/zero values for scorch options when loading back the index.
  • Fixes an edge case where we could protect lesser than configured number of snapshots, resulting in deficient rollback checkpoints being used for the next iteration. Added a unit test for this.
  • Rollback tests were timing-dependent and flaky, driving real indexing with time.Sleep() to simulate scenarios. Fixed them with deterministic tests that capture the same semantic meaning without using sleeps.

@CascadingRadium CascadingRadium changed the title Rollback MB-70770: Fix accidental deletion of latest snapshot from boltDB Jul 3, 2026
@coveralls

coveralls commented Jul 3, 2026

Copy link
Copy Markdown

Coverage Status

coverage: 52.205% (+0.07%) from 52.138% — rollback into master

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses accidental removal of the latest persisted snapshot from the Scorch root boltDB by refactoring snapshot “liveness”/protection selection and aligning purge behavior with those rules, plus updating tests to validate the retention guarantees.

Changes:

  • Reorders Scorch initialization so rollback/snapshot retention options are parsed before opening/loading bolt state.
  • Refactors snapshot retention to distinguish “live snapshots” from “protected snapshots”, ensuring the latest snapshot is always retained in bolt.
  • Updates/rewrites rollback-related tests to seed bolt snapshots directly and validate purge/retention invariants.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File Description
index/scorch/scorch.go Parse scorch rollback/snapshot options before opening/loading bolt to avoid retention logic using zero/default values.
index/scorch/persister.go Refactors snapshot retention (live/protected), purge behavior, and related persistence flow.
index/scorch/rollback_test.go Reworks tests to seed bolt snapshots and assert latest-snapshot retention during purge scenarios.
index/scorch/merge.go Ensures merged segment filenames are tracked before work proceeds (supports correct cleanup/unmarking on error paths).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread index/scorch/persister.go
Comment thread index/scorch/persister.go Outdated
Comment thread index/scorch/persister.go
Comment thread index/scorch/rollback_test.go

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Comment thread index/scorch/persister.go Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Comment thread index/scorch/persister.go Outdated
@CascadingRadium CascadingRadium changed the title MB-70770: Fix accidental deletion of latest snapshot from boltDB MB-72716: Fix miscalculation of rollback points Jul 3, 2026
@CascadingRadium
CascadingRadium merged commit cb80aa2 into master Jul 6, 2026
10 checks passed
@CascadingRadium
CascadingRadium deleted the rollback branch July 6, 2026 10:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants