fix(l1): don't treat unrelated datadir files as an existing DB

[pjdurden]

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

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR improves datadir detection so the store won’t misclassify a non-empty directory containing unrelated files (e.g., JWT secret) as an existing pre-metadata DB, preventing unnecessary startup failures (regression noted in issue #5680).

Changes:

• Replace “directory is non-empty” detection with RocksDB marker-file detection.
• Add targeted unit tests covering empty, unrelated-file-only, and RocksDB-marker directories.
• Expand comments to document the behavior and rationale.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[greptile-apps]

Greptile Summary

This PR fixes a startup regression where Store::new would return NotFoundDBVersion whenever the datadir was non-empty, even if it contained no actual database — breaking setups like EthDocker that place a JWT secret alongside the DB files.

• Replaces dir_is_empty with dir_contains_existing_db, which looks for RocksDB-specific marker files (CURRENT, IDENTITY, MANIFEST-*) instead of treating any non-empty directory as a pre-metadata database.
• Adds three focused unit tests: empty dir, dir with only unrelated files (the regression case), and dir containing the RocksDB markers.

Confidence Score: 4/5

The change is narrowly scoped to the datadir-detection heuristic and is well-tested; the core initialization path is unaffected for users with a properly formed RocksDB database.

The new marker-file heuristic does not check whether matched entries are files or directories, so a subdirectory accidentally named CURRENT or MANIFEST-* in the datadir would trigger a false positive and block startup with a confusing error. This is an unlikely but real edge case that a file-type guard would close.

crates/storage/store.rs — specifically the dir_contains_existing_db function and whether a file_type guard should be added to the directory-entry loop.

Important Files Changed

Filename	Overview
crates/storage/store.rs	Replaces dir_is_empty with dir_contains_existing_db that checks for RocksDB marker files (CURRENT, IDENTITY, MANIFEST-*); fixes false-positive NotFoundDBVersion when unrelated files share the datadir. Adds three unit tests covering the regression. Minor: does not filter entries by file type, so a directory named CURRENT would be a false positive.

Prompt To Fix All With AI

Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
crates/storage/store.rs:3497-3504
Guard on `file_type` to avoid treating a directory named `CURRENT` or `MANIFEST-*` as a DB marker. While highly unlikely in practice, RocksDB only ever creates plain files with those names, so filtering to `is_file()` makes the intent explicit and eliminates the false-positive path entirely.

```suggestion
    for entry in std::fs::read_dir(path)? {
        let entry = entry?;
        // Only consider plain files; a directory named CURRENT or MANIFEST-* should
        // not be treated as a DB marker.
        let Ok(file_type) = entry.file_type() else {
            continue;
        };
        if !file_type.is_file() {
            continue;
        }
        let file_name = entry.file_name();
        let name = file_name.to_string_lossy();
        // Marker files present in every initialized RocksDB database directory.
        if name == "CURRENT" || name == "IDENTITY" || name.starts_with("MANIFEST-") {
            return Ok(true);
        }
    }
```

_{Reviews (1): Last reviewed commit: "fix(storage): don't treat unrelated data..." | Re-trigger Greptile}

[ElFantasma]

LGTM, thanks for your contribution!

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

[pjdurden]

Thanks @ElFantasma . One help i need, it says i need 3 reviews (i've got yours, so 2 more), and the
required CI workflows are still "awaiting approval" — they need a maintainer to release
them before Integration Tests can run. could you help kick off the workflows
and loop in the other reviewers, or point me to who should take a look?

[iovoid]

It might be simpler to look for STORE_METADATA_FILENAME, like has_valid_db does.

[pjdurden]

good point, but it is actually the inverse here. by the time this runs read_store_schema_version has already returned None, and it only does that when the metadata file is absent, so keying off STORE_METADATA_FILENAME in this arm would always be false. dir_contains_existing_db is doing the opposite of has_valid_db: catching a real db that has no metadata yet (the legacy pre metadata ones), which only the rocksdb CURRENT/MANIFEST markers reveal. if i looked for the metadata file instead, every legacy db would read as fresh and init_metadata_file would stamp straight over it, which is the exact unsafe migration this guard is here to stop. happy to rename the fn though if the name is the confusing part.

[iovoid]

Makes sense. I would mention in the name/comments that it looks for legacy DBs (that do not have a metadata file).

[pjdurden]

ok, renamed it to dir_contains_legacy_db and reworded the doc comment that it's a legacy DB with no metadata file (and noted has_valid_db as the metadata-keyed counterpart).
Thanks @iovoid
Let me know if anything else is needed.