refactor(fs): migrate Tree::open recovery path to Fs trait#212
Merged
polaz merged 16 commits intoApr 5, 2026
Conversation
- Replace all `try_exists()` / `std::fs` calls in recovery path with pluggable `Fs` trait methods - Add `open_section_reader()` helper to replace `sfa::TocEntry::buf_reader()` which bypassed the Fs trait - Thread `&dyn Fs` through `recover()`, `get_current_version()`, `cleanup_orphaned_version()`, `SuperVersions::maintenance()`, and `Manifest::decode_from()` - Use `sfa::Reader::from_reader()` instead of `sfa::Reader::new()` to read version files through the Fs trait Closes #209
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThreads the pluggable Fs abstraction through manifest decoding, version recovery, version-maintenance (GC), Tree open/cleanup, and related call sites; replaces direct std::fs I/O with Fs operations, adds Changes
Sequence Diagram(s)sequenceDiagram
participant Client as "Tree / Compaction / Ingest"
participant Fs as "Fs (pluggable) rgba(100,150,240,0.5)"
participant Sfa as "sfa::Reader rgba(120,200,100,0.5)"
participant Super as "SuperVersions / version_lock rgba(240,180,80,0.5)"
Client->>Fs: open(manifest_path, FsOpenOptions::read())
Fs-->>Client: FsFile (seekable handle)
Client->>Sfa: sfa::Reader::from_reader(FsFile)
Client->>Sfa: Manifest::decode_from(path, reader, fs)
Sfa->>Fs: open_section_reader(path, section)
Fs-->>Sfa: BufReader(Take capped to section.len())
Client->>Super: maintenance(folder, gc_watermark, fs)
Super->>Fs: exists / read_dir / remove_file
Fs-->>Super: operation results (including NotFound)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
Refactors the Tree::open recovery/GC path to consistently use the pluggable Fs trait (instead of std::fs / Path::try_exists), enabling non-StdFs backends like MemFs to reopen trees and participate in recovery.
Changes:
- Thread
&dyn Fsthrough recovery/maintenance entrypoints (version GC, manifest decoding, recovery). - Introduce
fs::open_section_reader()to readsfasections viaFs(replacingsfa::TocEntry::buf_reader()which usesstd::fs). - Update call sites in tree open/recovery and compaction/ingest maintenance to pass the filesystem backend.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/fs/mod.rs | Adds open_section_reader() helper for sfa section reading via Fs. |
| src/manifest.rs | Decodes manifest sections using open_section_reader() + threads &dyn Fs. |
| src/version/recovery.rs | Migrates recovery reads (CURRENT, version file sections) from std::fs to Fs. |
| src/version/super_version.rs | Threads &dyn Fs into version GC and updates tests to pass StdFs. |
| src/tree/mod.rs | Switches try_exists/std::fs recovery logic to Fs (exists, read_dir, remove_file) and threads &dyn Fs through helper calls. |
| src/tree/ingest.rs | Passes &dyn Fs into version history maintenance after ingestion. |
| src/blob_tree/ingest.rs | Passes &dyn Fs into version history maintenance after blob ingestion. |
| src/compaction/worker.rs | Passes &dyn Fs into version history maintenance during compaction finalize paths. |
Comments suppressed due to low confidence (1)
src/tree/mod.rs:1217
- The primary goal here is enabling non-StdFs backends (MemFs) to reopen via the recovery path, but the PR still lists “Verify MemFs round-trip” as unchecked. Without an automated test that creates a tree on MemFs, drops it, and reopens it (exercising CURRENT/manifest recovery), this change is easy to regress later. Please add a dedicated regression test for MemFs reopen/recovery.
pub(crate) fn open(config: Config) -> crate::Result<Self> {
log::debug!("Opening LSM-tree at {}", config.path.display());
// Check for old version
if config.fs.exists(&config.path.join("version"))? {
log::error!(
"It looks like you are trying to open a V1 database - the database needs a manual migration, however a migration tool is not provided, as V1 is extremely outdated."
);
return Err(crate::Error::InvalidVersion(FormatVersion::V1.into()));
}
let tree = if config.fs.exists(&config.path.join(CURRENT_VERSION_FILE))? {
Self::recover(config)
} else {
Self::create_new(config)
}?;
…ned_version
Add doc comment explaining that Fs::read_dir fails fast on non-UTF-8
filenames instead of silently skipping them (as the old to_string_lossy
path did). This is correct: version files are always v{u64}, so
non-UTF-8 entries indicate filesystem corruption.
polaz
commented
Apr 5, 2026
Member
Author
polaz
left a comment
polaz
left a comment
There was a problem hiding this comment.
Acknowledged — see commit 534e633. The behavior change is intentional: version files are always v{u64}, so non-UTF-8 entries in the data directory indicate filesystem corruption. Fail-fast (via Fs::read_dir returning InvalidData) is the correct behavior here rather than silently skipping corrupted entries. Added a doc comment explaining this.
Verify that a MemFs-backed tree can be reopened after drop and recover flushed data — the core scenario unlocked by migrating Tree::open recovery path to the Fs trait. Also remove stale "known limitations" doc comments for tree-reopen and version-GC that are now resolved.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/tree/mod.rs (2)
1213-1217:⚠️ Potential issue | 🔴 CriticalDon’t branch on
CURRENTviaexists().Line 1213 takes a snapshot of
CURRENTand then decides betweenrecover()andcreate_new(). If another actor creates or removesCURRENTin between, this can take the wrong branch and initialize over an existing tree. Make the decision from a single open/read attempt, or letrecover()returnNotFound, instead of probing withexists()first.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tree/mod.rs` around lines 1213 - 1217, The code currently probes CURRENT_VERSION_FILE with config.fs.exists(...) then calls Self::recover or Self::create_new, which is racy; instead remove the exists() probe and choose the branch from a single read/open attempt—call Self::recover(config) first and if it returns a NotFound (or equivalent "missing" error) then call Self::create_new(config), or modify recover() to return a clear NotFound variant and handle that here; reference CURRENT_VERSION_FILE, exists(), Self::recover, and Self::create_new when making the change so the decision is based on one atomic read attempt rather than a separate exists() check.
1351-1393: 🛠️ Refactor suggestion | 🟠 MajorPlease add the MemFs reopen regression for this path before merge.
This is the end-to-end recovery path the PR is trying to unblock, but the summary still marks MemFs round-trip verification as pending. Without an integration test that creates a tree on
MemFs, closes it, and reopens it, the newFsplumbing throughget_current_version,Manifest::decode_from,recover(), and orphan cleanup is still unproven.I can help sketch the round-trip test if you want to add it in this PR.
Also applies to: 1500-1697
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tree/mod.rs` around lines 1351 - 1393, Add a MemFs round-trip integration test that exercises the full recover path: create an LSM tree using an in-memory Fs (MemFs) with a specific Config/comparator, write enough data to persist a manifest/version, close the tree, then reopen it via Tree::recover (which will call crate::version::recovery::get_current_version and Manifest::decode_from) and assert the reopen succeeds, the comparator name, format version and level_count are preserved, and there are no orphaned tables after recovery (verifying orphan cleanup ran). Locate the test near other integration tests for Tree recovery and reference Tree::recover, get_current_version, Manifest::decode_from, and Config to ensure the same code path is used for open/close round-trip on MemFs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/version/super_version.rs`:
- Line 263: Change tests that call history.maintenance(Path::new("."), 0,
&StdFs) to perform work inside a real temporary directory: create a
tempfile::tempdir(), write the version/manifest files into tempdir.path(), and
call history.maintenance(tempdir.path(), 0, &StdFs) (or equivalent helper) so GC
runs against an isolated on-disk directory; update all occurrences referenced
(the calls to history.maintenance and any setup creating "./v0") at the spots
corresponding to the symbols history.maintenance and StdFs (lines cited: 263,
293, 329, 353, 377) to use tempfile::tempdir() and create files inside that
tempdir instead of Path::new(".").
- Around line 109-110: The current TOCTOU uses fs.exists(&path)? followed by
fs.remove_file(&path)?, which can fail spuriously; remove the exists() probe and
call fs.remove_file(&path) directly inside the same context (in super_version.rs
where the GC/cleanup code runs), and treat a NotFound error as success (ignore
errors with std::io::ErrorKind::NotFound) while propagating other errors.
Replace the two-step check with a single remove_file call and handle the
NotFound case (e.g., match on the error or use map_err/map_or_else) so deletion
is idempotent and race-free.
---
Outside diff comments:
In `@src/tree/mod.rs`:
- Around line 1213-1217: The code currently probes CURRENT_VERSION_FILE with
config.fs.exists(...) then calls Self::recover or Self::create_new, which is
racy; instead remove the exists() probe and choose the branch from a single
read/open attempt—call Self::recover(config) first and if it returns a NotFound
(or equivalent "missing" error) then call Self::create_new(config), or modify
recover() to return a clear NotFound variant and handle that here; reference
CURRENT_VERSION_FILE, exists(), Self::recover, and Self::create_new when making
the change so the decision is based on one atomic read attempt rather than a
separate exists() check.
- Around line 1351-1393: Add a MemFs round-trip integration test that exercises
the full recover path: create an LSM tree using an in-memory Fs (MemFs) with a
specific Config/comparator, write enough data to persist a manifest/version,
close the tree, then reopen it via Tree::recover (which will call
crate::version::recovery::get_current_version and Manifest::decode_from) and
assert the reopen succeeds, the comparator name, format version and level_count
are preserved, and there are no orphaned tables after recovery (verifying orphan
cleanup ran). Locate the test near other integration tests for Tree recovery and
reference Tree::recover, get_current_version, Manifest::decode_from, and Config
to ensure the same code path is used for open/close round-trip on MemFs.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2f24af7c-0735-429e-9ef9-c88b12b5e017
📒 Files selected for processing (8)
src/blob_tree/ingest.rssrc/compaction/worker.rssrc/fs/mod.rssrc/manifest.rssrc/tree/ingest.rssrc/tree/mod.rssrc/version/recovery.rssrc/version/super_version.rs
Exercise Manifest::decode_from with MemFs backend to verify seek/take/read semantics work correctly with non-StdFs backends. Also refactor test helpers to accept &dyn Fs and extract decode_manifest() to reduce boilerplate.
- Replace exists() + remove_file() with direct remove_file() that
treats NotFound as success — eliminates TOCTOU race in version GC
- Switch maintenance tests from Path::new(".") with StdFs to MemFs,
preventing accidental deletion of files in the working directory
Member
Author
|
Thread #3 (super_version.rs:110 — TOCTOU): Fixed in ecfa47c — replaced Thread #4 (super_version.rs:263 — test isolation): Fixed in ecfa47c — switched all 5 maintenance test calls from Outside-diff #1 (tree/mod.rs:1213 — Tree::open TOCTOU): Valid but out of scope for this PR — tracked in #213. Outside-diff #2 (tree/mod.rs:1351 — MemFs reopen test): Already added in commit c057154 ( |
|
Tip For best results, initiate chat on the files or code changes.
(ᓀ ᵕ ᓂ) — solid set of fixes, lgtm. ✏️ Learnings added
🧠 Learnings used |
Member
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Member
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tree/mod.rs`:
- Around line 1206-1215: The public docs still claim reopening is unsupported
for non-StdFs backends even though Tree::open now probes using config.fs and the
new reopen test demonstrates MemFs reopen works; update the doc comment on the
public configuration type (the doc block in config::Config / the comments around
the reopen note) to remove or revise the statement that non-StdFs backends
cannot be reopened and instead document that reopen is supported when the
backend implements the required fs semantics (config.fs probe) and any caveats
(atomic read vs exists probe). Locate the comment near the reopen note in the
config module and change the text to match the new behavior (mention Tree::open
uses config.fs for recovery and that MemFs is supported).
In `@src/version/super_version.rs`:
- Around line 109-112: The code assumes missing-paths are reported as
std::io::ErrorKind::NotFound from Fs::remove_file; update the Fs trait to make
this contract explicit or add an idempotent delete helper and use it instead:
either (A) add documentation on the Fs trait's remove_file method stating
implementations must return Err(io::ErrorKind::NotFound) when the target is
absent (and update StdFs and MemFs docs/implementations to match), or (B) add a
new Fs method like remove_file_if_exists(&self, path: &Path) -> Result<(),
Error> (implemented by StdFs and MemFs to swallow NotFound) and replace the
match in super_version.rs with a call to remove_file_if_exists to guarantee
idempotent delete semantics for manifest GC; reference Fs::remove_file, StdFs,
MemFs, and the manifest GC delete site in super_version.rs when making the
change.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a18af3af-b04b-4417-bbc8-ef4845fb8f7b
📒 Files selected for processing (10)
src/blob_tree/ingest.rssrc/compaction/worker.rssrc/fs/mem_fs.rssrc/fs/mod.rssrc/manifest.rssrc/tree/ingest.rssrc/tree/mod.rssrc/version/recovery.rssrc/version/super_version.rstests/mem_fs_tree.rs
💤 Files with no reviewable changes (1)
- src/fs/mem_fs.rs
- Remove stale note in Config::fs claiming reopen is unsupported for non-StdFs backends — it now works via the Fs trait - Document NotFound contract on Fs::remove_file so implementors know version GC relies on this for idempotent deletes
Member
Author
|
Thread #5 (config docs stale): Fixed in 099f812 — removed the "reopen unsupported" note from Thread #6 (remove_file NotFound contract): Fixed in 099f812 — added explicit doc on |
|
Tip For best results, initiate chat on the files or code changes.
Also removing the now-stale learning that said MemFs-backed (ᓀ ᵕ ᓂ) — all threads closed. ✏️ Learnings added
⛔ Learnings removed🧠 Learnings used |
- Treat NotFound as success in cleanup_orphaned_version, mirroring SuperVersions::maintenance — prevents spurious recovery failure when a file is deleted between read_dir and remove_file - Use section.len() for recovery count bounds instead of Take::limit() which is unreliable through BufReader buffering
Member
Author
|
Duplicate (tree_non_utf8.rs:39-41 "arbitrary write failures"): This was fixed in commit 6bf0fd0 (3 pushes ago). The current code at those lines is Thread #17 (cleanup_orphaned_version NotFound): Fixed in 6043501 — Thread #18 (table_count bounds per-run): Kept |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/version/recovery.rs`:
- Around line 269-270: The test currently only checks result.is_err() after
calling recover(folder, &fs); tighten it to assert the specific
corruption/validation error variant instead of any error: replace the generic
assert!(result.is_err(), ...) with an assertion that matches the exact
corruption error returned by recover (e.g. using matches!(result,
Err(<expected_corruption_variant>)) or pattern-matching the Err to compare to
the corruption enum/variant). Do the same fix for the other occurrence around
lines 284-288 so both regression tests verify the expected corrupt table_count
error from 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cf31d57a-7a5d-4aff-9586-0711668276d8
📒 Files selected for processing (2)
src/tree/mod.rssrc/version/recovery.rs
…rsion_id - Read and validate checksum_type tag (must be 0 = xxh3) in get_current_version() — detects torn/corrupt CURRENT files before attempting recovery with a potentially wrong version ID - Remove stale TODO about checksum validation - Tighten corruption test assertions to match Unrecoverable specifically
Member
Author
|
Thread #19 (validate CURRENT checksum): Fixed in 49a2306 — Thread #20 (tighten corruption test assertions): Fixed in 49a2306 — both tests now assert |
|
Tip For best results, initiate chat on the files or code changes.
(ᓀ ᵕ ᓂ) — all threads closed on this one. ✏️ Learnings added
🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/version/recovery.rs`:
- Around line 219-247: In write_corrupt_table_count add a short inline comment
before the writes for "tree_type" and "blob_gc_stats" explaining these sections
are written only to satisfy recover()'s expectation that all sections exist
(even though the test targets "tables"), e.g., note that recover() requires
tree_type, tables, blob_files and blob_gc_stats to be present so we construct
minimal valid sections for the others; reference the function name
write_corrupt_table_count and the section names "tree_type" and "blob_gc_stats"
in the comment.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c3796ed9-43a5-4cf3-94b4-00bb53e37821
📒 Files selected for processing (1)
src/version/recovery.rs
Member
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
Member
Author
|
@coderabbitai Thread #21: Fixed in 6101615 — added doc comments to both test helpers explaining why all four sfa sections are written. |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
🧠 Learnings used |
Member
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
polaz
deleted the
feat/#209-refactorfs-migrate-treeopen-recovery-path-to-fs-tr
branch
Merged
polaz
pushed a commit
that referenced
this pull request
Apr 5, 2026
## 🤖 New release * `coordinode-lsm-tree`: 4.2.0 -> 4.3.0 <details><summary><i><b>Changelog</b></i></summary><p> <blockquote> ## [4.3.0](v4.2.0...v4.3.0) - 2026-04-05 ### Added - *(fs)* MemFs — in-memory Fs implementation for testing and in-memory trees ([#211](#211)) ### Fixed - *(table)* validate block type on cache-hit path ([#203](#203)) - *(table)* two-level index scan stops prematurely on empty child partitions ([#202](#202)) ### Performance - *(table)* add infallible OwnedIndexBlockIter constructor for pre-validated blocks ([#206](#206)) ### Refactored - *(fs)* migrate Tree::open recovery path to Fs trait ([#212](#212)) - *(table)* make index block bound-cursor helpers fallible ([#205](#205)) - *(table)* make all meta/trailer reads fallible for truncated blocks ([#204](#204)) - *(table)* make block decoder trailer validation fallible ([#199](#199)) </blockquote> </p></details> --- This PR was generated with [release-plz](https://github.com/release-plz/release-plz/). Co-authored-by: sw-release-bot[bot] <255865126+sw-release-bot[bot]@users.noreply.github.com>
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
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.
Summary
try_exists()/std::fscalls in the recovery path with pluggableFstrait methods, enabling MemFs and future non-StdFs backends to reopen treesopen_section_reader()helper to replacesfa::TocEntry::buf_reader()which usedstd::fsdirectly&dyn Fsthroughrecover(),get_current_version(),cleanup_orphaned_version(),SuperVersions::maintenance(), andManifest::decode_from()Tree::open— replaceexists()probe with atomic read attempt viaget_current_version()exists()+remove_file()with idempotentremove_file()that treatsNotFoundas success (bothSuperVersions::maintenanceandcleanup_orphaned_version)table_countandblob_file_countagainst section length in recovery to prevent allocation-based DoS from corrupt version filesNotFoundcontract onFs::remove_fileBehavior change
cleanup_orphaned_versionnow fails fast on non-UTF-8 filenames (viaFs::read_dirreturningInvalidData) instead of silently skipping them withto_string_lossy(). This is intentional — version files are alwaysv{u64}, so non-UTF-8 entries indicate filesystem corruption and should surface as an error.Test plan
table_countandblob_file_count→UnrecoverableCloses #209
Closes #213
Summary by CodeRabbit
Bug Fixes
Refactor
Tests
Documentation