[storage] fix rewind bugs #1521
Conversation
dc164e0 to
90a4bbd
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
This PR fixes critical bugs related to journal rewind operations not being persistent until synced to disk. The changes ensure that rewind operations are properly persisted by calling sync() after rewinding, and removes a premature early return in the MMR sync method that prevented syncing when size is 0.
- Makes journal rewind operations persistent by calling sync() after rewind operations across the codebase
- Removes early return in MMR sync() when size == 0 to allow proper syncing of rewinds to offset 0
- Adds comprehensive documentation warnings about rewind operations not being guaranteed to survive restarts until sync is called
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| storage/src/store/mod.rs | Adds sync() call after locations journal rewind |
| storage/src/mmr/journaled.rs | Removes early return in sync() method and adds sync() calls after rewind operations |
| storage/src/journal/variable.rs | Adds documentation warnings about rewind persistence requirements |
| storage/src/journal/fixed.rs | Updates documentation to clarify rewind persistence behavior |
| storage/src/freezer/storage.rs | Adds sync() call after journal rewind operation |
| storage/src/adb/keyless.rs | Adds multiple sync() calls after rewind operations |
| storage/src/adb/immutable/mod.rs | Adds sync() calls after locations rewind operations |
| storage/src/adb/any/variable/mod.rs | Adds sync() calls after locations rewind operations |
| storage/src/adb/any/fixed/sync.rs | Adds sync() call after journal rewind operation |
| storage/src/adb/any/fixed/mod.rs | Adds sync() call after log rewind operation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| } | ||
|
|
||
| self.journal.rewind(new_size).await?; | ||
| self.journal.sync().await?; |
There was a problem hiding this comment.
I was going to comment that we may not want to call sync "per pop" but it looks like it already takes a number to walk back, so looks good 👍
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #1521 +/- ##
=======================================
Coverage 92.06% 92.06%
=======================================
Files 283 283
Lines 73159 73182 +23
=======================================
+ Hits 67355 67378 +23
Misses 5804 5804
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
(1) We were incorrectly assuming journal rewind() and mmr.pop() were "sticky" operations, but in fact they aren't sticky until sync is called. This PR makes pop() sticky, and calls sync() where appropriate after rewinding.
(2) The journaled MMR's sync() operation was returning immediately when size == 0, which was preventing the syncing of a rewind to offset 0. Removed the size == 0 check, and added a regression test for this scenario.
Rewind documentation is updated to clearly state its properties.