use the SuperVersion seqno for get_highest_persisted_seqno if the tree is totally empty#293
Open
svix-jbrown wants to merge 4 commits into
Open
use the SuperVersion seqno for get_highest_persisted_seqno if the tree is totally empty#293svix-jbrown wants to merge 4 commits into
svix-jbrown wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughAdds a Version.created_seqno propagated from recovery/creation and preserved across version transforms; introduces SuperVersion::is_empty and updates persisted-seqno logic and Tree.clear to use created_seqno when appropriate; tests assert persisted-seqno transitions and persistence across reopen. ChangesIdle keyspace persisted seqno fix
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/tree/mod.rs`:
- Around line 632-643: get_highest_persisted_seqno currently returns
super_version.seqno for empty super versions, but that seqno is only in-memory
and not persisted so recovery resets it to 0; modify the persistence path so the
empty-tree seqno is stored in the manifest (or other durable metadata) when
upgrading versions (update upgrade_version_with_seqno to write the seqno
alongside next_version.version) and restore it during recovery (ensure
SuperVersions::new/recover reads the persisted seqno and sets the SuperVersion
field accordingly) or alternatively derive the durable seqno from existing
on-disk artifacts (e.g., sealed journals) before returning in
get_highest_persisted_seqno.
In `@tests/tree_clear.rs`:
- Around line 27-30: After calling tree.clear(), add a reopen/recovery step to
exercise the post-recovery bug: close or drop the current tree, reopen it (so
recover() runs), then re-check tree.get_highest_persisted_seqno() and assert it
still returns Some(1); this ensures the test exercises the recovery path and
will catch loss of seqno state after clear()/recover().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: db28bf42-9030-45b7-87db-72a3901071ee
📒 Files selected for processing (3)
src/tree/mod.rssrc/version/super_version.rstests/tree_clear.rs
svix-jbrown
force-pushed
the
jbrown/always-store-seqno
branch
from
3a43369 to
f0d2a93
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/tree/mod.rs`:
- Around line 636-647: The method get_highest_persisted_seqno uses
version_history.read().expect("lock is poisoned") but is missing the
#[expect(clippy::expect_used)] attribute that other similar methods (e.g.,
get_highest_memtable_seqno) include; add the #[expect(clippy::expect_used)]
attribute immediately above the fn get_highest_persisted_seqno declaration so
the use of expect for the lock is consistent with the rest of the file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a639750c-a4ca-44f6-9bee-d55c1b5304ae
📒 Files selected for processing (6)
src/tree/inner.rssrc/tree/mod.rssrc/version/mod.rssrc/version/recovery.rssrc/version/super_version.rstests/tree_clear.rs
marvin-j97
reviewed
Jun 2, 2026
marvin-j97
reviewed
Jun 2, 2026
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/version/mod.rs (1)
402-410:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRecompute
created_seqnowhen a derived version becomes empty.These helpers always carry forward the old
self.created_seqno. That breaks the new empty-tree contract once a later transform removes the last persisted table:with_dropped()can drop the final table, andwith_merge()can legally producenew_tables = [], leaving an empty version that still reports the seqno from the last create/clear instead of the seqno that was just persisted. SinceTree::get_highest_persisted_seqno()now returnsversion.created_seqnofor emptySuperVersions, persisted seqno can move backwards after compaction/drop-to-empty. Please thread the new persisted seqno into the transforms that can yield an empty version instead of always cloning the old field.Also applies to: 487-495, 569-577, 618-626
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/version/mod.rs` around lines 402 - 410, The new Version construction is incorrectly copying created_seqno from self (created_seqno: self.created_seqno) even when transforms produce an empty version; update the constructors that build VersionInner (the blocks setting created_seqno) to compute and thread the correct persisted seqno when new_tables/new_levels are empty by calling/getting the current highest persisted seqno (e.g. via Tree::get_highest_persisted_seqno() or a persisted_seqno argument) instead of blindly cloning self.created_seqno; apply the same change in the other similar VersionInner construction sites referenced (the blocks around the ranges 487-495, 569-577, 618-626) and ensure with_dropped() and with_merge() pass the recomputed persisted_seqno into the VersionInner created_seqno field.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/version/mod.rs`:
- Around line 402-410: The new Version construction is incorrectly copying
created_seqno from self (created_seqno: self.created_seqno) even when transforms
produce an empty version; update the constructors that build VersionInner (the
blocks setting created_seqno) to compute and thread the correct persisted seqno
when new_tables/new_levels are empty by calling/getting the current highest
persisted seqno (e.g. via Tree::get_highest_persisted_seqno() or a
persisted_seqno argument) instead of blindly cloning self.created_seqno; apply
the same change in the other similar VersionInner construction sites referenced
(the blocks around the ranges 487-495, 569-577, 618-626) and ensure
with_dropped() and with_merge() pass the recomputed persisted_seqno into the
VersionInner created_seqno field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 21924293-7cd7-450b-bd81-53f764225db7
📒 Files selected for processing (3)
src/tree/mod.rssrc/version/mod.rstests/tree_seqno.rs
svix-jbrown
force-pushed
the
jbrown/always-store-seqno
branch
from
4de9b2d to
c967fda
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/version/mod.rs`:
- Around line 499-505: The created_seqno calculation uses Option::max which
wrongly preserves an older created_seqno when the tree becomes empty; in the
block computing created_seqno (variables: table_count, levels,
self.created_seqno, seqno) replace the use of self.created_seqno.max(seqno) with
logic that prefers the provided drop seqno when present and falls back to the
existing created_seqno (i.e., use seqno.or(self.created_seqno)); this ensures
when table_count == 0 we record the most recent emptying seqno and otherwise
keep self.created_seqno unchanged.
- Around line 610-616: The reset logic for created_seqno incorrectly uses
self.created_seqno.max(seqno) which can keep a stale higher value when the tree
becomes empty; change the assignment to prefer the new seqno when present by
using seqno.or(self.created_seqno) instead (i.e., set created_seqno =
seqno.or(self.created_seqno)) so the new seqno is used if Some, otherwise fall
back to the existing self.created_seqno; update the expression around
table_count, created_seqno and seqno accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8119489b-7dfb-410d-aa2c-3a8859d8bfec
📒 Files selected for processing (8)
src/compaction/worker.rssrc/tree/inner.rssrc/tree/mod.rssrc/version/mod.rssrc/version/recovery.rssrc/version/super_version.rstests/tree_clear.rstests/tree_seqno.rs
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
marvin-j97
reviewed
Jun 11, 2026
Comment on lines
216
to
+219
| /// Creates a new empty version. | ||
| pub fn new(id: VersionId, tree_type: TreeType) -> Self { | ||
| Self::new_at_seqno(id, tree_type, None) | ||
| } |
Contributor
There was a problem hiding this comment.
This may not be needed anymore - we always build Versions with seqno now anyway?
| } | ||
|
|
||
| /// Creates a new empty version at the given sequence number | ||
| pub fn new_at_seqno(id: VersionId, tree_type: TreeType, created_seqno: Option<SeqNo>) -> Self { |
Contributor
There was a problem hiding this comment.
created_seqno: Option<SeqNo> -> created_seqno: SeqNo
| copy.version = copy.version.with_dropped_at( | ||
| ids_to_drop, | ||
| &mut dropped_blob_files, | ||
| Some(opts.global_seqno.get()), |
Contributor
There was a problem hiding this comment.
I think this should be opts.global_seqno.next(). Otherwise two compactions being installed in quick succession could, technically, have the same seqno.
| } else { | ||
| crate::TreeType::Standard | ||
| }, | ||
| Some(config.seqno.get()), |
| copy.version = Version::new_at_seqno( | ||
| v.version.id() + 1, | ||
| self.tree_type(), | ||
| Some(config.seqno.get()), |
| // reset the created_seqno if the tree has newly become empty | ||
| let table_count: usize = levels.iter().map(|x| x.table_count()).sum(); | ||
| let created_seqno = if table_count == 0 { | ||
| self.created_seqno.max(seqno) |
Contributor
There was a problem hiding this comment.
Is this even possible? Why should the persisted seqno be possibly higher than the next, newer version?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I talked to @marvin-j97 in discord and they suggest that this would be a simpler/better alternative to #291.
If a tree is totally empty (no tables on disk, nothing in the memtables; e.g., the state right after a clear), then we use the seqno of the SuperVersion as the highest persisted seqno.
I believe that this will fix fjall-rs/fjall#288
Summary by CodeRabbit
Bug Fixes
New Features
Tests