fix(l1): treat datadir with non-DB files as fresh on first init#6563
fix(l1): treat datadir with non-DB files as fresh on first init#6563sueun-dev wants to merge 1 commit intolambdaclass:mainfrom
Conversation
the datadir non-empty check for pre-metadata DBs was triggering for any non-DB content (logs/, signer keys, node config), which made `--log.dir` pointed inside `--datadir` fail with NotFoundDBVersion on a fresh node. switched to looking for a RocksDB CURRENT marker instead, which is the actual signal of a pre-metadata DB. Fixes lambdaclass#6513
Greptile SummaryFixes a fresh-node startup failure (#6513) where The fix replaces the "dir is non-empty" heuristic with a check for RocksDB's Confidence Score: 5/5Safe to merge — targeted, well-reasoned fix with no behaviour change for nodes already on v1+ metadata. The change is minimal and correct: No files require special attention.
|
| Filename | Overview |
|---|---|
| crates/storage/store.rs | Replaces dir_is_empty with has_pre_metadata_db_marker (checks for RocksDB's CURRENT file) to avoid false-positive "pre-metadata DB" errors when non-DB files exist in the datadir; three unit tests added. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Store::new(path, engine_type)"] --> B{engine_type == InMemory?}
B -- Yes --> Z[Skip version check]
B -- No --> C["read_store_schema_version(db_path)"]
C --> D{version?}
D -- "None" --> E{"has_pre_metadata_db_marker(db_path)\n= db_path/CURRENT is_file()"}
E -- "true\n(CURRENT exists, no metadata.json)" --> F["Err: NotFoundDBVersion\n(pre-metadata RocksDB DB)"]
E -- "false\n(empty dir, logs/, keys, etc.)" --> G["init_metadata_file()\nFresh node start ✓"]
D -- "Some(v) where v < 1" --> H["Err: MigrationFailed"]
D -- "Some(v) where v > STORE_SCHEMA_VERSION" --> I["Err: MigrationFailed (too new)"]
D -- "Some(v) where v < STORE_SCHEMA_VERSION" --> J["run_pending_migrations()"]
D -- "Some(v) == STORE_SCHEMA_VERSION" --> K["Proceed normally"]
Reviews (1): Last reviewed commit: "fix(l1): treat datadir with non-DB files..." | Re-trigger Greptile
There was a problem hiding this comment.
Pull request overview
Adjusts storage initialization to avoid rejecting a fresh --datadir when it contains non-DB files (e.g., a logs/ directory created by --log.dir), while still detecting truly pre-metadata RocksDB databases that can’t be safely migrated.
Changes:
- Replace the “non-empty directory” heuristic with a RocksDB
CURRENTmarker check to identify pre-metadata DBs. - Add unit tests covering empty datadir, datadir with unrelated subdir/files, and datadir containing a
CURRENTfile.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn pre_metadata_db_marker_empty_dir() { | ||
| let dir = TempDir::new().unwrap(); | ||
| assert!(!has_pre_metadata_db_marker(dir.path())); | ||
| } | ||
|
|
||
| #[test] | ||
| fn pre_metadata_db_marker_unrelated_subdir() { | ||
| // Reproduces #6513: `--log.dir` pointed inside `--datadir` creates a | ||
| // logs/ subdirectory before init_store runs. The datadir is non-empty | ||
| // but contains no DB files, so we should still treat it as fresh. | ||
| let dir = TempDir::new().unwrap(); | ||
| std::fs::create_dir_all(dir.path().join("logs")).unwrap(); | ||
| std::fs::write(dir.path().join("logs").join("ethrex.log"), b"").unwrap(); | ||
| assert!(!has_pre_metadata_db_marker(dir.path())); |
| std::fs::write(dir.path().join("CURRENT"), b"MANIFEST-000001 | ||
| ").unwrap(); |
Motivation
init_storerejects a fresh datadir if the directory is non-empty. but if--log.dirpoints inside--datadir, the logger creates alogs/subdirectory before init_store runs, so the dir is technically non-empty. fresh node fails to start with:reported in #6513.
Description
the dir-non-empty check was meant to catch pre-metadata RocksDB databases (the ones without a
metadata.json). but the datadir legitimately holds non-DB stuff already: signer keys, node config, and now potentially a user-configured--log.dir. so "non-empty" is the wrong signal.switched to looking for RocksDB's
CURRENTfile instead. RocksDB always writes that on first open, so its presence (withoutmetadata.json) is the actual marker of a pre-metadata DB.added 3 unit tests covering:
logs/=> not flagged (the Cannot start fresh ethrex if log.dir is set to datadir #6513 repro)CURRENTfile => flagged (real pre-metadata DB)afaik no behavior change for users who were already on v1+ metadata, since those go through the
Some(v)arms.Checklist
STORE_SCHEMA_VERSION(crates/storage/lib.rs) if the PR includes breaking changes to theStorerequiring a re-sync. (n/a, no schema change)Closes #6513