fix: return I/O errors from restore and prune instead of panicking by kmatasfp · Pull Request #1 · golemcloud/rustic_core

kmatasfp · 2026-09-15T19:45:07Z

resolves GOL-599
fix: return the first error of the restore content phase: restore_contents keeps the first error of its pack reads, decrypts, read_at, set_length and write_at calls instead of a panic. Tasks that start after the error do no I/O, and the restore returns the error. A restore without faults does the same work as before.
fix: stop tree workers after receiver closes (#546): cherry-pick of upstream 9d9e478317, with the fork author. The patch id equals upstream, so a rebase onto upstream drops it.
test: commits add rustic_testing::backend::fault_injection_backend, and the unpublished crate rustic_fault_tests. That crate has the fault scenarios, a runner that runs each scenario in its own process built with the panic-abort profile and checks the exit status, and in-process tests of the same scenarios. The full-volume scenario mounts a small tmpfs in a user namespace and runs only in the runner.
test: check a sparse restore of a file that ends in zeros kills the mutants that survived at the unchanged line if filesize > 0 in restore_contents. SparseRestore is not exported, so the scenario sets the option through serde_json.
Three code reviews checked the diff against the standards and against GOL-599, at 5623d8e, at e6ee7e5 and at 24317dc. The docs: and test: commits between f2c4763 and 1a0525e fix the accepted findings, and change only doc comments and test code.

Panic hardening

The fork runs in a long-lived host process built with panic = "abort", where one panic on one thread stops the process and every unrelated workload in it. After the two fixes above, an audit of the repository-open, restore and prune paths found more abort paths, all of them upstream 0.13.0 code that those fixes never touched. These commits close the ones that panic in a release build. None of them changes behaviour for a backend that behaves.

fix: return an error when a backend read is short: nothing checked that a partial read gives the length that it asks for. DecryptBackend::read_partial now returns an error when it gets fewer bytes, CachedBackend::read_partial checks the range before it slices the file that read_full gave, and a read of a cache file compares the range against the size of the file before it allocates. LocalBackend::read_partial does the same. A short file no longer goes into the cache.
fix: check the blob range before slicing pack data: restore_contents and the repacker sliced the pack data with &read_data[start..end], which panics when the data is shorter, and computed the bounds with two expect calls. Both now call BlobLocation::part_of, which checks the range once and returns an error. BlobLocation::data_length uses saturating_sub.
fix: return an error for a directory node without a subtree: subtree is #[serde(default)], so a directory node without that key made NodeStreamer and find_used_blobs panic on unwrap.
fix: guard the unused limit against a full percentage: max_unused of 100 percent divided by zero, and a larger value went negative. 100 percent and above now means any amount of unused data.
fix: return an error when the index is still in use: GlobalIndex::into_index slept 100 ms and then panicked with expect("index still in use"). It now tries a fixed number of times and returns an error. This panic was seen firing in a test run of this repository.
fix: validate the key lengths before building a key: Key::from_keys copied three slices into fixed ranges, so a key file with wrong field lengths panicked when the repository opened.
fix: do not panic in the rclone drop and reader thread: a panic in a drop is an abort, and no thread joins the reader while it reads.
test: add a truncating fault and a cache-enabled fixture: Fault::Truncate makes a read give one byte less. The fixtures opened with no_cache(true), so no test could reach the cache code at all.
test: check the new error paths and test: pin the cache range check and the repack decision: scenarios and unit tests for each fix, and tests that pin the range check and the repack decision, which the mutation gate showed were unpinned.

These signatures now return a RusticResult: GlobalIndex::into_index, IndexedIds::into_indexed_tree, Repository::drop_data_from_index and Key::from_keys. Fault gained a variant.

Known limits, not changed here:
- No test checks that write tasks stop after the first error. With 20 fixed reader threads, the order of the writes changes from run to run. The reader thread option of GOL-601 makes a test with one thread possible.
- restore-stops-after-first-error copies MAX_READER_THREADS_NUM (20) as READER_THREADS, because the constant is pub(crate).
- restore_contents returns the first error before it finishes the progress bar. 0.13.0 also leaves the bar unfinished when the thread pool cannot be built.
- The full-volume scenario needs unprivileged user namespaces.
- A panic in a task of the global rayon pool aborts the process whatever the panic setting, because rayon aborts when no panic handler is set. stream_list uses that pool, and its task calls the progress bar of the caller. Index loading and snapshot listing reach it.
- Left for a follow-up issue: a lint gate for this class of bug; the packer and actor threads that only finalize reaps, and its untimed recv; the warm-up error path that leaves a thread and a child process; the cache temp name without a unique suffix; the tokio runtime of the opendal backend, which is built with unwrap; arithmetic that panics only where overflow checks are on, which a release build wraps and the new checks then turn into an error.
Known upstream flake, not changed here: integration::check::test_check fails 9 of 40 runs on unmodified 1030be4.

Verification

All commands ran locally at c2a1ec0 unless a line says otherwise, with CARGO_BUILD_JOBS=12 and TMPDIR in a scratch directory. GitHub Actions workflows are not active on this fork.

Baseline on 1030be4, cargo test --all-targets --all-features --workspace --examples: 299 passed, 0 failed
git diff --check 1030be4..HEAD: clean
cargo fmt --all -- --check: pass
cargo clippy --all-targets --all-features -- -D warnings: pass
cargo clippy --no-default-features -- -D warnings: pass
cargo doc --no-deps --all-features --workspace --examples: pass
typos and dprint check: pass
cargo test --all-targets --all-features --workspace --examples: pass. rustic_core lib 185 (168 at the base), integration 57 (57 at the base, so the added checks do not change the behaviour of the suite), warm_up 28, command_input 6, errors 4, keys 4, rustic_backend 33. New: rustic_testing 9, rustic_fault_tests lib 3, in_process 11.
cargo run -p rustic_fault_tests --profile panic-abort: 12 scenarios, 0 failed
Red proof, restore.rs from 1030be4: 5 in-process restore tests fail on panics, and 6 runner scenarios end with SIGABRT at restore.rs 620, 625, 643, 658 and 663
Red proof, stop checks removed: restore_stops_after_first_error fails with 40 reads of 40 packs. The bound is 20.
Red proof, tree.rs from 1030be4: the in-process prune_tree_read test counts 3 panics at tree.rs:676, and the runner scenario prune-tree-read ends with SIGABRT
Red proof for each hardening fix, with the panic that it removes: a short pack read panics at the slice of restore.rs; a short cached pack panics in Bytes; a directory node without a subtree panics on unwrap; an unused limit of 100 percent divides by zero; a shared index panics with "index still in use"; a key part of the wrong length panics in copy_from_slice.
Mutation gate, through safe-mutants with --jobs 1, against the diff of the hardening commits, over blob.rs, backend/decrypt.rs, backend/cache.rs, commands/restore.rs, blob/packer.rs, index.rs, commands/prune.rs, blob/tree.rs and crypto/aespoly1305.rs:
- 81 mutants, of which 63 do not build. Two oracles ran over the same set, one with the unit tests of rustic_core and one with the in-process fault tests, because they kill different mutants.
- The first run left 11 mutants alive under both oracles, in the range check of the cache and in decide_repack. test: pin the cache range check and the repack decision kills all 11, and that commit changes no production code: every hunk is inside a #[cfg(test)] mod tests.
- Earlier gate of the restore fix, --in-diff against 1030be4, --test-package rustic_fault_tests: 17 mutants, 8 caught, 0 missed, 9 unviable. The first run missed 3 mutants at if filesize > 0, and the sparse restore scenario kills all 3 in a rerun.
- cargo-mutants 27.1.0 runs its baseline in the mutated package, which has no in_process target, so the runs use --baseline skip --timeout 300. An unmutated run of the same filter passed first.
- tree.rs: the mutation tool lists no mutant in TreeStreamerOnce, where the fix: stop tree workers after receiver closes rustic-rs/rustic_core#546 fix is, so 17 hand-written mutants checked that work instead. The pre-fix: stop tree workers after receiver closes rustic-rs/rustic_core#546 unwrap ends prune-tree-read with SIGABRT, and a loader that ignores a failed send is caught by the count of tree reads.
- No mutation annotation, configuration or dependency is left in the tree.

Add `FaultInjectionBackend` to `rustic_testing`. A rule decides for each listing, read, write or removal whether the call fails, or whether a read returns corrupt data. Add the unpublished crate `rustic_fault_tests`. It has fixtures, checks for panics in threads that an operation does not join, a small tmpfs in a user and mount namespace, and a runner binary. The runner starts each scenario in a child process that is built with the new `panic-abort` profile, and checks the exit status. A panic thus fails the scenario.

`restore_contents` called `unwrap()` on the results of pack reads, decryption, reads of existing files, `set_length` and `write_at` in its rayon scope. An I/O error thus caused a panic, which stops a process that is built with `panic = "abort"`. Each task now stores its error in a shared `FirstError`. A task that starts after a stored error stops before it does work. After the scope, `restore_contents` returns the first stored error.

Add scenarios for a restore without faults, failed pack reads, corrupt pack data, a failed read of an existing file, a failed `set_length`, a full volume, and a check that each reader thread reads at most one pack after the first error. `tests/in_process.rs` runs them in the test harness, and the runner runs them with `panic = "abort"`.

## Summary - stop tree-loading workers when the consumer has already exited - handle a closed output channel inline without panicking Related: rustic-rs/rustic#1877, rustic-rs/rustic#1790, rustic-rs/rustic#1437, rustic-rs/rustic#1400 ## Validation - cargo test --locked -p rustic_core --lib: 168 passed - cargo clippy -p rustic_core --lib --tests --locked -- -D warnings - cargo fmt --all -- --check - git diff --check --------- Co-authored-by: Magrathean UK <magrathean-uk@users.noreply.github.com> (cherry picked from commit 9d9e478)

Add a scenario in which each tree read of `prune_plan` fails. Eight snapshots with different root trees make sure that tree loader threads still send trees after the prune stops. The scenario waits until the loader threads release the backend, and then checks the panic count, so a panic in a loader thread fails the scenario in the test harness too.

Add a scenario that saves 1 MiB of pseudo-random data followed by 9 MiB of zeros, restores it with sparse `ByContent`, and compares the length and the content. The default maximum chunk size is 8 MiB, so the last chunks hold only zeros and the sparse restore does not write them. The file then has its full length only if the restore sets the length one time, before the writes. This kills the mutants of `if filesize > 0` in `restore_contents` that the mutation gate found. `SparseRestore` is not public, so the scenario sets the option through serde, and `rustic_fault_tests` gets `serde_json` as a dependency.

Say what `FirstError` does for the whole life of the value, remove the word "subsequent", and use the active voice for the error bullets and the stop note of `restore_contents`. Only doc comments change.

Use `Box<str>`, `Box<Path>` and `Box<[u8]>` for strings, paths and byte buffers that the fault tests build once and do not change. Replace "subsequent" and passive sentences in the doc comments of the fault injection backend and of `rustic_fault_tests`.

Say that each task stops at its next check of `FirstError`, and that a read, decryption or write that has started does not stop. Describe the methods of `FirstError` without the tasks that use it. Only doc comments change.

Say what a scenario expects without faults and with faults, remove a creation fact from the `Tmpfs` doc, make the fault injection backend doc clear, and pass the expected path of `restore_set_length` as a temporary.

The `# Errors` list lost its catch-all bullet, so the error of the thread pool build was in no bullet. Name it, in the order in which the errors happen. Only doc comments change.

In this module a call is a call to a backend. The docs of `inject` and `clear` used the word for a backend call and for their own call in one sentence. Say backend calls. Only doc comments change.

The fault injection backend gets a third fault, `Truncate`. A read with this fault gets one byte less than it asks for, so a scenario can make a backend break the contract of a read without an error. The fault tests get a fixture that opens a saved repository again, with a cache. The cache holds no pack file, so each pack read reaches the backend and the cache code runs.

A backend can return `Ok` with fewer bytes than the read asks for. The callers use the data at the full length, so they panic on a short read. The decrypt backend now checks the length of each partial read and returns an error, which covers every caller in the core. The cache backend checks the range against the file before it slices the file, and it does not cache a file that cannot satisfy the read. The local backend and the cache read the size of the file before they allocate the buffer, so a range past the end of the file is an error and not an allocation from a length that the repository gives.

The restore and the repack of a prune sliced the data of a pack read with the offsets that the index gives, and the slice panics when the data is shorter than the index says. Both now ask the blob location for its part of the data, which gives an error instead. The data length of a blob without a compressed length no longer subtracts 32 bytes below zero. The value goes into the read and the slice, which give an error for a wrong value.

The subtree of a node is optional in the tree format, so a directory node without a subtree key panicked the node streamer of a restore and the blob search of a prune. Both now return an error that names the node.

A maximum of unused data of 100% divided by zero, and a higher percentage subtracted below zero. Both now mean that the prune accepts any amount of unused data, and the multiplication saturates.

The conversion of a global index into an index waited 100 ms one time and then panicked if another thread still held the index. It now tries ten times and returns an error after the last try. The callers give the error to their own caller.

A key file that another program wrote can hold parts whose lengths do not fit the key. The copy into the key panicked on such a file, while a repository was opening. The build of a key now checks the length of each part and returns an error.

A panic in a drop stops the process, so the drop of the rclone backend now warns about a failed kill instead. No thread joins the reader of the rclone output while it reads, so a panic there also stops the process, and the loop now ends on an error. The opendal backend builds a process-wide tokio runtime with a panic. The panic needs a process that is already out of threads or file descriptors, and a fix needs a change of the callers, so a doc comment names the risk.

Two fault scenarios cover the short read: a restore whose pack reads give one byte too few, and a tree read of a repository with a cache whose pack files are shorter than the index says. Both run in the test harness and in the runner that stops on a panic. A prune scenario counts the tree reads after the first error, which pins the stop of the tree loaders, and the sparse restore now checks the blocks of the restored file. Unit tests cover the part of a blob in the data of a read, the data length of a short blob, a directory node without a subtree in the node streamer and in the blob search of a prune, a full percentage of unused data, an index that another user holds, the lengths of the parts of a key, and a read after the end of a local file.

The range check of a partial cache read now has unit tests for a part inside the file, a part that ends at the end of the file, and a part that ends after the file. The last one checks the message of the range check, so a read that only finds the end of the file later does not pass it. The tests of the repack decision now use a plan that has a candidate to repack, so a decision is visible. Two more tests give a percentage that allows the unused data, and one below it, and check that the plan keeps the pack in the first case and repacks it in the second.

kmatasfp and others added 8 commits September 15, 2026 12:39

docs: simplify the FirstError and restore_contents doc comments

f2c4763

Say what `FirstError` does for the whole life of the value, remove the word "subsequent", and use the active voice for the error bullets and the stop note of `restore_contents`. Only doc comments change.

kmatasfp marked this pull request as ready for review September 15, 2026 20:50

kmatasfp added 14 commits September 15, 2026 14:07

docs: describe how restore_contents stops after the first error

d667a29

Say that each task stops at its next check of `FirstError`, and that a read, decryption or write that has started does not stop. Describe the methods of `FirstError` without the tasks that use it. Only doc comments change.

test: clarify the fault test docs and the set_length check

24317dc

Say what a scenario expects without faults and with faults, remove a creation fact from the `Tmpfs` doc, make the fault injection backend doc clear, and pass the expected path of `restore_set_length` as a temporary.

docs: name the thread pool error in the restore_contents docs

20c0ad5

The `# Errors` list lost its catch-all bullet, so the error of the thread pool build was in no bullet. Name it, in the order in which the errors happen. Only doc comments change.

test: say backend calls in the fault injection docs

1a0525e

In this module a call is a call to a backend. The docs of `inject` and `clear` used the word for a backend call and for their own call in one sentence. Say backend calls. Only doc comments change.

fix: return an error for a directory node without a subtree

29dd047

The subtree of a node is optional in the tree format, so a directory node without a subtree key panicked the node streamer of a restore and the blob search of a prune. Both now return an error that names the node.

fix: guard the unused limit against a full percentage

eb2d73f

A maximum of unused data of 100% divided by zero, and a higher percentage subtracted below zero. Both now mean that the prune accepts any amount of unused data, and the multiplication saturates.

kmatasfp merged commit c2a1ec0 into golem-rustic_core-v0.13.0 Sep 21, 2026
1 check passed

Navigation Menu

fix: return I/O errors from restore and prune instead of panicking - #1

fix: return I/O errors from restore and prune instead of panicking#1
kmatasfp merged 22 commits into
golem-rustic_core-v0.13.0from
gol-599-restore-prune-errors

kmatasfp commented Sep 15, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

kmatasfp commented Sep 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Panic hardening

Verification

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

kmatasfp commented Sep 15, 2026 •

edited

Loading