fix(zebra-state): start without backup instead of aborting on unusable backup dir - #11234
fix(zebra-state): start without backup instead of aborting on unusable backup dir#11234natalieesk wants to merge 2 commits into
Conversation
…e backup dir `NonFinalizedState::with_backup` created the non-finalized state backup directory with `create_dir_all(...).expect(...)`. When the path already exists as a regular file (or is otherwise uncreatable), `create_dir_all` returns an error and the `expect` panics — which, under the workspace's `panic = "abort"`, aborts the whole process on every startup, leaving the node in a crash loop until the file is removed by hand. Move the directory setup out of the blocking task and handle its error: probe the path with `create_dir_all` then `read_dir` (a directory that already exists but is unreadable passes `create_dir_all` but would abort later in `restore_backup`), and on failure log a clear, actionable message and start without the non-finalized backup, routing to the existing no-backup path. The backup is a resilience optimisation, not required for correct operation, so degrading gracefully is preferable to a crash loop. A graceful hard-fail would instead require threading `Result` through the public `zebra_state::init` API and all its callers. Adds a regression test that points the backup path at a regular file and asserts `with_backup` returns instead of aborting.
And six more auto-invalidated findings. Analyzed one file, diff |
conradoplg
left a comment
There was a problem hiding this comment.
I'd prefer for Zebra to shutdown in that case rather than keep running, which could lead to a misconfiguration being unnoticed. I don't think panicking in that case is a huge deal but if you can find a non-very-intrusive way to gracefully log the error and shutdown zebra, I'd prefer that, otherwise we can just close the PR.
|
@claude update this PR so that if we fail to create a non-finalized state backup directory at the:
|
Motivation
Closes #10544.
NonFinalizedState::with_backup(zebra-state/src/service/non_finalized_state.rs) created the backup directory withcreate_dir_all(...).expect(...)inside aspawn_blocking. When the path already exists as a regular file,create_dir_allerrors and theexpectpanics; under the workspace'spanic = "abort"that aborts the process on every startup — a crash loop until the file is removed by hand.Solution
Probe the path (
create_dir_allthenread_dir) out of the blocking task and, on error, log a clear actionable message and start without the non-finalized backup, routing to the existing no-backup path. The backup is a re-downloadable resilience cache, not consensus data, so degrading is preferable to a crash loop.read_diris also probed becausecreate_dir_allsucceeds for a directory that already exists but is unreadable, which would otherwise abort deeper inrestore_backup.A graceful hard-fail instead would have to thread
Resultthrough the publiczebra_state::initAPI and all its callers — a much larger change, so this degrades in place. No consensus, RPC, config, or DB-format change.Tests
with_backup_starts_without_backup_when_path_is_a_file(non_finalized_state/tests/vectors.rs) opens an ephemeral finalized DB, writes a regular file at the backup path, callswith_backup, and asserts it returns (rather than aborting underpanic = "abort") and leaves the file untouched.cargo fmt/clippyclean. (An unreadable-directory test isn't included — creating one portably in CI, and its no-op as root, is unreliable; that branch is covered by the sameread_dirprobe.)Specifications & References
None.
Follow-up Work
run_backup_taskandrestore_backupstill callread_dir(...).expect(...)inbackup.rs, so a backup directory that becomes unreadable while zebrad runs can still abort. Pre-existing and out of scope here; worth a follow-up to convert those to warn-and-skip.AI Disclosure
PR Checklist
type(scope): description