refactor: separate backend operations #409

aawsome · 2025-05-31T15:10:42Z

This PR refactors indexer and packer into a version which is completely independent from I/O and concurrency but simply provides the functionality.
This can then be separated into real "core" functionality.

As a side effect, this PR solves the following topics:

Ensure that really no duplicate blobs are saved (closes The copy command may copy duplicate data #146)
Solves potential race conditions (which in theory could lead to data loss) between finalizing packer/indexed and file_saver by using self in the finalize methods.

codecov · 2025-06-04T07:12:22Z

Codecov Report

❌ Patch coverage is 61.81102% with 97 lines in your changes missing coverage. Please review.
✅ Project coverage is 44.0%. Comparing base (6d3577d) to head (9555d5b).

Files with missing lines	Patch %	Lines
crates/core/src/blob/repopacker.rs	60.1%	53 Missing ⚠️
crates/core/src/commands/prune.rs	56.6%	13 Missing ⚠️
crates/core/src/blob/pack_sizer.rs	65.0%	7 Missing ⚠️
crates/core/src/blob/packer.rs	73.9%	6 Missing ⚠️
crates/core/src/commands/repair/index.rs	0.0%	6 Missing ⚠️
crates/core/src/index/indexer.rs	89.2%	3 Missing ⚠️
crates/core/src/backend/decrypt.rs	33.3%	2 Missing ⚠️
crates/core/src/commands/copy.rs	0.0%	2 Missing ⚠️
crates/core/src/commands/merge.rs	0.0%	2 Missing ⚠️
crates/core/src/commands/repair/snapshots.rs	0.0%	2 Missing ⚠️
... and 1 more

Additional details and impacted files

Files with missing lines	Coverage Δ
crates/core/src/archiver/file_archiver.rs	`63.1% <100.0%> (ø)`
crates/core/src/archiver/tree_archiver.rs	`69.7% <100.0%> (ø)`
crates/core/src/blob.rs	`72.7% <ø> (-7.3%)`	⬇️
crates/core/src/chunker.rs	`50.5% <ø> (-1.2%)`	⬇️
crates/core/src/archiver.rs	`58.9% <66.6%> (-0.8%)`	⬇️
crates/core/src/backend/decrypt.rs	`48.1% <33.3%> (+0.2%)`	⬆️
crates/core/src/commands/copy.rs	`0.0% <0.0%> (ø)`
crates/core/src/commands/merge.rs	`0.0% <0.0%> (ø)`
crates/core/src/commands/repair/snapshots.rs	`0.0% <0.0%> (ø)`
crates/core/src/index/indexer.rs	`91.6% <89.2%> (+45.5%)`	⬆️
... and 5 more

... and 19 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

simonsan

Love the direction, feel like there is a bit more error handling needed at certain places. Need to take another, deeper look at RepoPacker, few TODOs and docs still open.

crates/core/src/blob/packer.rs

simonsan · 2025-06-07T20:50:59Z

crates/core/src/index/indexer.rs

+    pub fn needs_save(&self) -> bool {
+        // check if IndexFile needs to be saved
+        let elapsed = self.created.elapsed().unwrap_or_else(|err| {
+            warn!("couldn't get elapsed time from system time: {err:?}");


This should be part of the error handling here and not a warning in the logs.

This is the error handling. When we can't get an elapsed time when we check if an index file should be saved, we have two options:

return an error which would mean we don't finish the backup

omit the check on the elapsed time, i.e. only write when the index file is "full" and warn the user. This will successfully finish the backup.

I strongly prefer option 2) which is the current error handling.

This wasn't what I meant, what I meant was if we have a warning in the log, user of rustic_core are not able to handle this, instead of a log warning this should return a rustic_error with the message so a user can handle this if we get to it from somewhere.

We could discuss is we don't log the warning but just silently handle this case.
Anyway, this discussion doesn't belong to this PR, IMO, as the behavior is not changed w.r.t. main, see https://github.com/rustic-rs/rustic_core/blob/main/crates/core/src/blob/packer.rs#L610.

crates/core/src/blob/repopacker.rs

simonsan · 2025-06-07T20:52:54Z

crates/core/src/blob/repopacker.rs

+            finish: finish_rx,
+        };
+
+        let _join_handle = std::thread::spawn(move || {


Don't we want to use the join handles somewhere and join the threads, e.g. in case we get Ctrl + C or something?

This is a good question. Actually the _join_handle_raw is joined by waiting for the status which is sent via a channel. And the join_handle is joined by the fact that it sends to the second thread, i.e. the _join_handle_raw will not finish unless _join_handle finishes. I'm not aware if there is a problem about something like Ctrl+C - can you depict what would be happening in this case?

I believe the threads can terminate quite "normally" even in case of an interruption in the main thread because they "simply" iterate on the rx:

main threads closes

tx is closed/dropped

_join_handle thread will then terminate

tx_raw is closed/dropped

_join_handle_raw thread will then terminate

crates/core/src/blob/repopacker.rs

nardoor

I think these are nice API changes, it's good to see the BE backend generic type be less propagated if that makes sense.

nardoor · 2025-07-06T12:58:11Z

crates/core/src/index/indexer.rs

-    pub fn save(&self) -> RusticResult<()> {
-        if (self.file.packs.len() + self.file.packs_to_delete.len()) > 0 {
-            _ = self.be.save_file(&self.file)?;
+    pub fn save(&mut self) -> Option<IndexFile> {


This method has totally changed, some other methods totally changed as well, would you mind quickly updating their documentations pls :)?

Also, do you think it deserves a rename? (same questions for other methods that changed signature)

nardoor · 2025-07-06T13:00:05Z

crates/core/src/index/indexer.rs

    /// * `id` - The id to check.
-    pub fn has(&self, id: &BlobId) -> bool {
+    pub fn has(&mut self, id: &BlobId) -> bool {
        self.indexed
-            .as_ref()
+            .as_mut()
            .is_some_and(|indexed| indexed.contains(id))
    }


I am not sure we need mut there, is there any reason you left it as such?

nardoor · 2025-07-06T13:19:40Z

crates/core/src/blob/repopacker.rs

+            finish: finish_rx,
+        };
+
+        let _join_handle = std::thread::spawn(move || {


I believe the threads can terminate quite "normally" even in case of an interruption in the main thread because they "simply" iterate on the rx:

main threads closes

tx is closed/dropped

_join_handle thread will then terminate

tx_raw is closed/dropped

_join_handle_raw thread will then terminate

aawsome added 7 commits May 31, 2025 17:10

refactor: separate backend operations

2e76e89

fix deadlock & duplicate pack saving

9461d15

fix clippy

085e6bb

remove repoindexer

2935fde

fix: wait for filewriter to finish

4be2bf7

fix: prune: set correct delete flag

e16be9d

remove unneccessary checks for duplicates

9c4a13e

aawsome mentioned this pull request Jun 4, 2025

The copy command may copy duplicate data #146

Open

aawsome added 4 commits June 4, 2025 11:48

clarify indexer methods

ab2214a

refactor pack sizer

080b4c7

simplify SharedIndexer

d449b87

clean up Packer errors

063017d

aawsome force-pushed the separate-be branch from 7866e28 to 063017d Compare June 4, 2025 13:09

aawsome added 4 commits June 4, 2025 15:59

simplify RepositoryPacker

228defd

use self in finalize

c38d762

corrections

8dd914d

adjust docu

0b11bde

aawsome requested a review from simonsan June 5, 2025 10:39

aawsome added the S-waiting-for-review Status: PRs waiting for review label Jun 5, 2025

simonsan reviewed Jun 7, 2025

View reviewed changes

aawsome and others added 4 commits June 14, 2025 21:47

import from root

3abadd4

Add more docu

618a1c8

Merge branch 'main' into separate-be

91e39b9

Merge branch 'main' into separate-be

8adaea2

aawsome mentioned this pull request Jun 28, 2025

Add padding blob to data packs to mitigate chunking attacks #413

Open

Merge branch 'main' into separate-be

c1ebbd5

nardoor reviewed Jul 6, 2025

View reviewed changes

Merge branch 'main' into separate-be

9555d5b

refactor: separate backend operations #409

Are you sure you want to change the base?

refactor: separate backend operations #409

Uh oh!

Conversation

aawsome commented May 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

simonsan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

simonsan Jun 7, 2025

Choose a reason for hiding this comment

Uh oh!

aawsome Jun 14, 2025

Choose a reason for hiding this comment

Uh oh!

simonsan Jun 28, 2025

Choose a reason for hiding this comment

Uh oh!

aawsome Jun 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

simonsan Jun 7, 2025

Choose a reason for hiding this comment

Uh oh!

aawsome Jun 14, 2025

Choose a reason for hiding this comment

Uh oh!

nardoor Jul 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nardoor left a comment

Choose a reason for hiding this comment

Uh oh!

nardoor Jul 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nardoor Jul 6, 2025

Choose a reason for hiding this comment

Uh oh!

nardoor Jul 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aawsome commented May 31, 2025 •

edited

Loading

codecov bot commented Jun 4, 2025 •

edited

Loading

nardoor Jul 6, 2025 •

edited

Loading

nardoor Jul 6, 2025 •

edited

Loading

nardoor Jul 6, 2025 •

edited

Loading