[storage/archive/immutable] Fix Unclean (Initial) Restart #1336
Conversation
| Some(freezer) => *freezer.freezer(), | ||
| None => { | ||
| metadata.put(freezer_key.clone(), Record::Freezer(Checkpoint::default())); | ||
| *metadata.get(&freezer_key).unwrap().freezer() |
There was a problem hiding this comment.
When we load an empty table, we shouldn't initialize the checkpoint to be the default. That makes no sense.
| /// | ||
| /// This can be used to restore the [Freezer] to a consistent | ||
| /// state after shutdown. | ||
| #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Copy, Default)] |
There was a problem hiding this comment.
Remove (unsafe) defualt impl here (only use checkpoints we get)
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue with unclean shutdown and restart behavior in the storage archive system by removing the Default derive from the Checkpoint struct and updating initialization logic to handle cases where no previous checkpoint exists.
- Removes
Defaulttrait fromCheckpointto prevent automatic initialization with default values - Updates checkpoint initialization logic to explicitly handle the absence of stored checkpoints
- Adds test coverage for unclean shutdown scenarios
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| storage/src/freezer/storage.rs | Removes Default derive from Checkpoint and replaces Default::default() with explicit field initialization |
| storage/src/archive/immutable/storage.rs | Simplifies checkpoint retrieval logic to return Option<Checkpoint> instead of always returning a value |
| storage/src/archive/immutable/mod.rs | Adds comprehensive test for unclean shutdown and restart scenarios |
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #1336 +/- ##
==========================================
+ Coverage 91.37% 91.38% +0.01%
==========================================
Files 251 252 +1
Lines 63583 63617 +34
==========================================
+ Hits 58097 58139 +42
+ Misses 5486 5478 -8
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
If we restarted
archive::immutableafter unclean shutdown (and before we wrote anything), it could panic because it defaulted to an empty checkpoint if the freezer file existed (rather than just aNonecheckpoint).This PR introduces a regression test and adds a fix.