fix(state): make read_only_open_with_ephemeral_config_returns_error more robust by conradoplg · Pull Request #11146 · ZcashFoundation/zebra

conradoplg · 2026-07-29T22:33:34Z

Motivation

Solution

The issue was actually in the code, we improve error checking which makes the test reliable and not dependent on a specific folder existing or not during testing

Tests

Specifications & References

Follow-up Work

AI Disclosure

No AI tools were used in this PR
AI tools were used: Claude, comments were cleaned up manually

PR Checklist

The PR title follows conventional commits format: type(scope): description
The PR follows the contribution guidelines.
This change was discussed in an issue or with the team beforehand.
The solution is tested.
The documentation and changelogs are up to date.

natalieesk · 2026-07-30T14:19:32Z

Closing as bug no longer exists

natalieesk · 2026-07-30T17:22:41Z

Accidentally closed the wrong PR. Reopening.

v12-auditor · 2026-07-30T17:22:46Z

Note

Complete: Audit complete. V12 found four issues worth reviewing.

Open the full results here.

Finding	Severity	Details
F-107518	🟡 `Medium`	Read-only state open panics on unreadable or malformed version file The read-only branch of `ZebraDb::new()` calls `DiskDb::check_cache_dir_readable(&config.cache_dir)` and then applies `.expect("unable to read database format version file")` to the result of `database_format_version_on_disk()`. The doc comment immediately above at lines 104-108 claims the readability check exists so that a missing or unreadable directory "returns a typed `ReadOnlyCacheDirUnreadable` error here instead of panicking on the version-file read", but the guard only performs `fs::read_dir(cache_dir)` on the top-level directory. The version file actually lives several levels deeper at `cache_dir/<db_kind>/v<major>/<network>/version` per `Config::version_file_path` and `Config::db_path`. In `database_format_version_at_path`, only `ErrorKind::NotFound` is swallowed into `None`; every other I/O error is propagated via `Err(e)?`, and a version file whose contents fail both the direct semver parse and the legacy `"{major}.{content}"` fallback returns `Err(String)`. Any of these recoverable local conditions reaches the `.expect` and panics.
F-107520	🟡 `Medium`	Read-only open skips all database format validation `ZebraDb::new()` forces `debug_skip_format_upgrades = read_only \|\| (cfg!(test) && debug_skip_format_upgrades)`, and `ZebraDb::spawn_format_change()` returns immediately when that flag is set. A single boolean therefore gates two distinct concerns: applying format upgrades, which writes and must be skipped on a read-only secondary, and running format validity checks, which are write-free and safe on a secondary. The skipped work includes `format_validity_checks_detailed()`, which the read-write path treats as fatal — it panics with "unexpected invalid database format: delete and re-sync the database". Its constituent checks only read (tip comparison, subtree pre-checks, genesis-root quick check, tree-key-type iteration, and per-upgrade `validate` implementations), so they could run safely read-only. Separately, the read-only path's only version-derived guard is `format_change.is_newly_created()`; `DbFormatChange::open_database()` can also return `Upgrade` when the disk version is older and `Downgrade` when it is newer, and neither is rejected or even warned about. An `Upgrade` state is reachable at a same-major path because `try_reusing_previous_db_after_major_upgrade` renames the old major directory to the new one and then deliberately writes the OLD major version into the NEW path's version file; if that upgrade is interrupted, the file stays stale indefinitely.
F-107521	🟡 `Medium`	Indexer-enabled read-only open serves empty spend indexes Whether the on-disk database contains the spending-transaction indexes is recorded solely in the semver build metadata of the version file: `state_database_format_version_in_code()` sets `build` to `"indexer"` when the `indexer` feature is enabled and empty otherwise. Because semver `cmp_precedence` ignores build metadata, an on-disk `28.0.0` written by a non-indexer node compares Equal to an in-code `28.0.0+indexer`, so `open_database` returns `CheckOpenCurrent`. On the read-write path that variant is exactly what triggers the backfill: the code writes the `+indexer` build metadata and runs `track_tx_locs_by_spends::run(...)` over the chain. On the read-only path this entire block is skipped because `debug_skip_format_upgrades` is forced true and `spawn_format_change` returns early, so the build metadata is never read, never compared, and the index is never built. The `tx_loc_by_spent_out_loc` column family is registered unconditionally, so the open succeeds with an empty CF and no open-time failure alerts the operator.
F-107522	🟡 `Medium`	Empty database directory causes misleading panic instead of typed error The read-only guard in `ZebraDb::new()` rejects only the case where `DbFormatChange::open_database` reports `NewlyCreated`. Per `database_format_version_at_path`, `NewlyCreated` requires BOTH that the version file read returns `NotFound` AND that `fs::metadata(db_path)` returns `NotFound`. If the versioned database directory `cache_dir/<db_kind>/v<major>/<net>/` exists but holds no RocksDB files — a partially-copied cache, an interrupted delete, or a primary that created the directory but has not yet initialized RocksDB — then `fs::metadata` succeeds and the function returns `Some(Version::new(major, 0, 0))`. `is_newly_created()` is then false, the guard is bypassed, and execution reaches `DiskDb::new`, where `DB::open_cf_descriptors_as_secondary` cannot create a database and fails. The failure is handled by a `panic!` whose message reads "Database likely already open ... Hint: Check if another zebrad process is running", which misdiagnoses the actual condition.

And four more auto-invalidated findings.

Analyzed one file, diff 21b652b...2c187ad.

alchemydc

I went through the four v12 findings and verified each against the code. All four are mechanically real, but all four are pre-existing conditions of the read-only open path on main — none are introduced by this PR — so I don't think any of them should block it. Summary:

F-107518 (panic on unreadable/malformed version file): real. The .expect at zebra_db.rs:113 panics when the version file exists but is permission-denied or unparseable; check_cache_dir_readable only guards the top-level cache dir, so the doc comment above it overclaims. Note the read-write path has the identical .expect, so read-only is no worse — but a typed error would be better for a secondary reading a primary's cache.
F-107520 (read-only skips all format validation): real behaviorally, partly by design. Skipping upgrades read-only is required; silently accepting Upgrade/Downgrade disk versions is a genuine gap (the stale-version-file scenario via an interrupted try_reusing_previous_db_after_major_upgrade is real — it deliberately writes the old major into the new path's version file). Caveat on the suggested remedy: running the fatal validity checks on a secondary that lags a mid-upgrade primary could fail spuriously and panic, so "the checks are safe read-only" isn't clear-cut. A warn/error on Upgrade/Downgrade in read-only mode seems like the right-sized fix.
F-107521 (indexer read-only serves empty spend indexes): real, and the most consequential — it produces silently wrong results (None) rather than a crash, because the +indexer marker lives only in semver build metadata, which cmp_precedence ignores, and the backfill only runs from the format-change task that read-only mode skips. Filed as #11155.
F-107522 (empty versioned dir → misleading "Database likely already open" panic): real. An empty cache_dir/state/vN/<net>/ yields Some(N.0.0), bypasses the newly-created guard, and the secondary open failure panics with a misleading hint. Error-message quality issue; deserves a typed error.

This PR's change itself is correct: it fixes the real root cause of the test flake (error-precedence depended on whether the default cache dir existed on the machine) and matches the authoritative check in DiskDb::new(). It also avoids the temp-dir leak from calling database_format_version_on_disk() with ephemeral = true. LGTM; the findings above are follow-up material for the read-only open feature, not this fix.

mergify · 2026-07-30T23:40:09Z

mergify · 2026-07-31T14:42:45Z

mergify · 2026-07-31T14:42:53Z

queue

☑️ This pull request is already queued

mergify · 2026-07-31T18:12:59Z

…rror checking more robust

mergify · 2026-07-31T22:21:18Z

Link the read-only ephemeral configuration fix to PR #11146 so crate consumers can find its implementation and tests.

conradoplg added the C-exclude-from-changelog Category: The PR should be excluded from the changelog and release notes label Jul 29, 2026

conradoplg temporarily deployed to dev July 29, 2026 22:33 — with GitHub Actions Inactive

conradoplg changed the title ~~fix(state): make read_only_open_with_ephemeral_config_returns_error error checking more robust~~ fix(state): make read_only_open_with_ephemeral_config_returns_error more robust Jul 29, 2026

conradoplg temporarily deployed to dev July 29, 2026 22:36 — with GitHub Actions Inactive

natalieesk closed this Jul 30, 2026

natalieesk reopened this Jul 30, 2026

natalieesk temporarily deployed to dev July 30, 2026 17:22 — with GitHub Actions Inactive

natalieesk temporarily deployed to dev July 30, 2026 17:25 — with GitHub Actions Inactive

alchemydc self-assigned this Jul 30, 2026

alchemydc self-requested a review July 30, 2026 23:32

alchemydc mentioned this pull request Jul 30, 2026

bug: read-only state open with the indexer feature silently serves empty spending-transaction indexes #11155

Open

alchemydc approved these changes Jul 30, 2026

View reviewed changes

mergify Bot added the queued label Jul 30, 2026

mergify Bot mentioned this pull request Jul 30, 2026

merge queue: checking main (1f55db2), #10980 and #11146 together #11156

Closed

39 tasks

mergify Bot added dequeued and removed queued labels Jul 31, 2026

alchemydc mentioned this pull request Jul 31, 2026

SPIKE: Analyse the status of the test failures after test refactor #10737

Open

mergify Bot added the queued label Jul 31, 2026

mergify Bot mentioned this pull request Jul 31, 2026

merge queue: checking main (25d0c1b) and #11146 together #11160

Closed

39 tasks

mergify Bot added dequeued and removed dequeued queued labels Jul 31, 2026

mergify Bot added queued and removed dequeued labels Jul 31, 2026

mergify Bot mentioned this pull request Jul 31, 2026

merge queue: checking main (fa10763) and #11146 together #11163

Closed

39 tasks

mergify Bot added dequeued and removed queued labels Jul 31, 2026

alchemydc mentioned this pull request Jul 31, 2026

fix(ci): stop Mergify label events from cancelling required PR Gate runs #11164

Merged

6 tasks

fix(state): make read_only_open_with_ephemeral_config_returns_error e…

2bbefc2

…rror checking more robust

conradoplg force-pushed the improve-read-only-ephemeral branch from 2c187ad to 2bbefc2 Compare July 31, 2026 21:14

conradoplg temporarily deployed to dev July 31, 2026 21:14 — with GitHub Actions Inactive

mergify Bot removed the dequeued label Jul 31, 2026

conradoplg temporarily deployed to dev July 31, 2026 21:17 — with GitHub Actions Inactive

mergify Bot added the queued label Aug 1, 2026

mergify Bot merged commit 67eac5c into main Aug 1, 2026
195 of 197 checks passed

mergify Bot deleted the improve-read-only-ephemeral branch August 1, 2026 00:09

mergify Bot removed the queued label Aug 1, 2026

jvff added a commit that referenced this pull request Aug 6, 2026

Link state fix changelog entry

b72d74d

Link the read-only ephemeral configuration fix to PR #11146 so crate consumers can find its implementation and tests.

jvff added a commit that referenced this pull request Aug 10, 2026

Link state fix changelog entry

7cdc2df

Link the read-only ephemeral configuration fix to PR #11146 so crate consumers can find its implementation and tests.

jvff added a commit that referenced this pull request Aug 10, 2026

Link state fix changelog entry

411e985

Link the read-only ephemeral configuration fix to PR #11146 so crate consumers can find its implementation and tests.

Navigation Menu

Uh oh!

fix(state): make read_only_open_with_ephemeral_config_returns_error more robust - #11146

fix(state): make read_only_open_with_ephemeral_config_returns_error more robust#11146
mergify[bot] merged 1 commit into
mainfrom
improve-read-only-ephemeral

conradoplg commented Jul 29, 2026 •

edited

Loading

Uh oh!

natalieesk commented Jul 30, 2026

Uh oh!

natalieesk commented Jul 30, 2026

Uh oh!

v12-auditor Bot commented Jul 30, 2026 •

edited

Loading

Uh oh!

alchemydc left a comment

Uh oh!

mergify Bot commented Jul 30, 2026 •

edited

Loading

Uh oh!

mergify Bot commented Jul 31, 2026 •

edited

Loading

Uh oh!

mergify Bot commented Jul 31, 2026

Uh oh!

mergify Bot commented Jul 31, 2026 •

edited

Loading

Uh oh!

mergify Bot commented Jul 31, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

conradoplg commented Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Solution

Tests

Specifications & References

Follow-up Work

AI Disclosure

PR Checklist

Uh oh!

natalieesk commented Jul 30, 2026

Uh oh!

natalieesk commented Jul 30, 2026

Uh oh!

v12-auditor Bot commented Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alchemydc left a comment

Choose a reason for hiding this comment

Uh oh!

mergify Bot commented Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge Queue Status

Reason

Hint

Uh oh!

mergify Bot commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge Queue Status

Reason

Hint

Uh oh!

mergify Bot commented Jul 31, 2026

☑️ This pull request is already queued

Uh oh!

mergify Bot commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge Queue Status

Reason

Hint

Uh oh!

mergify Bot commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge Queue Status

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

conradoplg commented Jul 29, 2026 •

edited

Loading

v12-auditor Bot commented Jul 30, 2026 •

edited

Loading

mergify Bot commented Jul 30, 2026 •

edited

Loading

mergify Bot commented Jul 31, 2026 •

edited

Loading

mergify Bot commented Jul 31, 2026 •

edited

Loading

mergify Bot commented Jul 31, 2026 •

edited

Loading