Skip to content

Commit 7807924

Browse files
pjdurdenilitteri
andauthored
fix(l1): don't treat unrelated datadir files as an existing DB (#6786)
## Motivation Closes #5680. `Store::new` returns `NotFoundDBVersion` whenever the datadir is merely **non-empty**, even when there is no actual database in it. This breaks setups where unrelated files share the datadir — e.g. EthDocker places the JWT secret in the same directory, so startup fails on a fresh node. ## Description The intent of that branch is "a pre-metadata DB exists and we can't migrate it safely." It used *directory non-empty* as a proxy for *a database exists*, which is too broad. This PR replaces `dir_is_empty()` with `dir_contains_existing_db()`, which looks for RocksDB's marker files (`CURRENT`, `IDENTITY`, `MANIFEST-*`) instead. A datadir that only contains unrelated files (a JWT, etc.) is now correctly treated as fresh and initialized, while a real pre-metadata DB still returns `NotFoundDBVersion`. RocksDB is the only on-disk backend (`crates/storage/backend/` = `rocksdb` + `in_memory`), so its markers fully cover the persistent path. Happy to extend the marker set if other backends land. ## Tests Added unit tests in `datadir_tests`: - empty dir → not an existing DB - dir with only unrelated files (JWT, `LOG`) → not an existing DB (the regression) - dir with `CURRENT` / `MANIFEST-*` → existing DB ``` cargo test -p ethrex-storage --lib datadir_tests cargo clippy -p ethrex-storage --lib ``` --------- Co-authored-by: Ivan Litteri <67517699+ilitteri@users.noreply.github.com>
1 parent aa5bd30 commit 7807924

1 file changed

Lines changed: 81 additions & 5 deletions

File tree

crates/storage/store.rs

Lines changed: 81 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1649,12 +1649,16 @@ impl Store {
16491649
let version = read_store_schema_version(&db_path)?;
16501650

16511651
match version {
1652-
None if db_path.exists() && !dir_is_empty(&db_path)? => {
1652+
None if db_path.exists() && dir_contains_legacy_db(&db_path)? => {
16531653
// Pre-metadata DB — cannot migrate safely
16541654
return Err(StoreError::NotFoundDBVersion);
16551655
}
16561656
None => {
1657-
// Fresh / empty directory — write initial metadata
1657+
// No metadata and no recognizable database files. The directory
1658+
// may still hold unrelated files (e.g. a JWT secret placed in the
1659+
// datadir by tooling such as EthDocker, see issue #5680), so treat
1660+
// this as a fresh datadir and write the initial metadata instead
1661+
// of erroring out.
16581662
init_metadata_file(&db_path)?;
16591663
}
16601664
Some(v) if v < 1 => {
@@ -3594,9 +3598,37 @@ fn init_metadata_file(parent_path: &Path) -> Result<(), StoreError> {
35943598
Ok(())
35953599
}
35963600

3597-
fn dir_is_empty(path: &Path) -> Result<bool, StoreError> {
3598-
let is_empty = std::fs::read_dir(path)?.next().is_none();
3599-
Ok(is_empty)
3601+
/// Returns `true` if `path` contains a *legacy* database — one written before
3602+
/// the metadata file existed, so it has no `metadata.json` to identify it.
3603+
/// Detected by RocksDB's own marker files, as opposed to unrelated files that
3604+
/// merely share the datadir. Only meaningful once metadata has been confirmed
3605+
/// absent; otherwise prefer `has_valid_db`, which keys off the metadata file.
3606+
///
3607+
/// Previously the caller treated *any* non-empty directory as such a legacy
3608+
/// database, which made startup fail when unrelated files lived alongside the DB
3609+
/// — e.g. EthDocker writes the JWT secret into the datadir (issue #5680). We
3610+
/// instead look for RocksDB's marker files, so a datadir that only contains such
3611+
/// unrelated files is correctly treated as fresh.
3612+
fn dir_contains_legacy_db(path: &Path) -> Result<bool, StoreError> {
3613+
// `CURRENT` has a fixed name and is written by every RocksDB instance, so
3614+
// check for it directly instead of scanning a datadir that may hold many
3615+
// unrelated files.
3616+
if path.join("CURRENT").is_file() {
3617+
return Ok(true);
3618+
}
3619+
// The manifest has a numeric suffix (`MANIFEST-<n>`), so it can only be
3620+
// found by scanning. Restrict to plain files: a directory that happens to
3621+
// share the name is not a database marker.
3622+
for entry in std::fs::read_dir(path)? {
3623+
let entry = entry?;
3624+
if !entry.file_type()?.is_file() {
3625+
continue;
3626+
}
3627+
if entry.file_name().to_string_lossy().starts_with("MANIFEST-") {
3628+
return Ok(true);
3629+
}
3630+
}
3631+
Ok(false)
36003632
}
36013633

36023634
/// Checks whether a valid (or migratable) database exists at the given path
@@ -3802,3 +3834,47 @@ mod merge_tests {
38023834
assert_eq!(decode(&out).len(), 3);
38033835
}
38043836
}
3837+
3838+
#[cfg(test)]
3839+
mod datadir_tests {
3840+
use super::*;
3841+
use std::fs;
3842+
3843+
#[test]
3844+
fn empty_dir_has_no_existing_db() {
3845+
let dir = tempfile::tempdir().unwrap();
3846+
assert!(!dir_contains_legacy_db(dir.path()).unwrap());
3847+
}
3848+
3849+
#[test]
3850+
fn dir_with_only_unrelated_files_has_no_existing_db() {
3851+
// Regression for #5680: a JWT secret (or any unrelated file) in the
3852+
// datadir must not be mistaken for an existing database.
3853+
let dir = tempfile::tempdir().unwrap();
3854+
fs::write(dir.path().join("jwt.hex"), "0xdeadbeef").unwrap();
3855+
fs::write(dir.path().join("LOG"), "noise").unwrap();
3856+
assert!(!dir_contains_legacy_db(dir.path()).unwrap());
3857+
}
3858+
3859+
#[test]
3860+
fn dir_with_rocksdb_markers_has_existing_db() {
3861+
// A `CURRENT` file (and, separately, a `MANIFEST-*` file) marks a real DB.
3862+
let dir = tempfile::tempdir().unwrap();
3863+
fs::write(dir.path().join("CURRENT"), "MANIFEST-000001\n").unwrap();
3864+
assert!(dir_contains_legacy_db(dir.path()).unwrap());
3865+
3866+
let dir2 = tempfile::tempdir().unwrap();
3867+
fs::write(dir2.path().join("MANIFEST-000007"), "x").unwrap();
3868+
assert!(dir_contains_legacy_db(dir2.path()).unwrap());
3869+
}
3870+
3871+
#[test]
3872+
fn dir_with_marker_named_subdirectories_has_no_existing_db() {
3873+
// A *directory* named like a marker file must not be mistaken for a DB;
3874+
// RocksDB only ever writes these as plain files.
3875+
let dir = tempfile::tempdir().unwrap();
3876+
fs::create_dir(dir.path().join("CURRENT")).unwrap();
3877+
fs::create_dir(dir.path().join("MANIFEST-000001")).unwrap();
3878+
assert!(!dir_contains_legacy_db(dir.path()).unwrap());
3879+
}
3880+
}

0 commit comments

Comments
 (0)