[PATCH proxmox-backup v2 0/3] fix #6990: server: drop verify state on push & pull job

[PATCH proxmox-backup v2 0/3] fix #6990: server: drop verify state on push & pull job

[Jakob Klocker]

When syncing a snapshot to another datastore, the source's verify_state
is currently carried over to the target. This reports the target
snapshot as verified even though its stored copy was never checked
there. Since verify jobs skip snapshots that already carry a
verify_state, the target's copy can't be verified again.

Chunks are checksummed in memory while being transferred, but a verify
serves a different purpose: confirming the write to the (external)
target actually succeeded.

This series drops verify_state on sync so the target is verified
independently. It also moves the
blocking calls in the touched code paths off the runtime's worker
threads onto the blocking thread pool.

Tested (verify_state correctly stripped on target):
 * Pull, verified source, new content, clean sync
 * Pull, verified source, existing content, clean sync
 * Pull, corrupted target, resync-corrupt
 * Push, verified source (has no corrupt path)
 * Pull, independently-verified target, clean re-sync - state preserved

changes from v1 to v2 (thanks @Christian):
 * move the manifest write helper onto `BackupManifest`
 * offload fsync, rename and cleanup to `spawn_blocking()`

Link: https://bugzilla.proxmox.com/show_bug.cgi?id=6990


proxmox-backup:

Jakob Klocker (3):
  server: pull: run blocking file operations on the blocking pool
  fix #6990: server: drop verify state on push job
  fix #6990: server: drop verify state on non-decrypt pull job

 pbs-datastore/src/manifest.rs | 25 +++++++++++++++-
 src/server/pull.rs            | 56 ++++++++++++++++++++++-------------
 src/server/push.rs            |  7 +++++
 3 files changed, 67 insertions(+), 21 deletions(-)


Summary over all repositories:
  3 files changed, 67 insertions(+), 21 deletions(-)

-- 
Generated by murpp 0.12.0

[PATCH proxmox-backup v2 1/3] server: pull: run blocking file operations on the blocking pool

[Jakob Klocker]

The atomic renames and the unreferenced-file cleanup are blocking
operations that ran directly on the runtime's worker threads, stalling
other tasks scheduled there for their duration.

Use the tokio counterpart for the renames and move the cleanup onto the
blocking thread pool.

Signed-off-by: Jakob Klocker <j.klocker@proxmox.com>
---
 src/server/pull.rs | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/src/server/pull.rs b/src/server/pull.rs
index 4eb5bcf11..fd401ac85 100644
--- a/src/server/pull.rs
+++ b/src/server/pull.rs
@@ -600,7 +600,7 @@ async fn pull_single_archive<'a>(
     } else {
         tmp_path
     };
-    if let Err(err) = std::fs::rename(&source_path, &path) {
+    if let Err(err) = tokio::fs::rename(&source_path, &path).await {
         bail!("{archive_prefix}: Atomic rename file {path:?} failed - {err}");
     }
 
@@ -890,7 +890,7 @@ async fn pull_snapshot<'a>(
         nix::unistd::fsync(tmp_manifest_file.as_raw_fd())?;
     }
 
-    if let Err(err) = std::fs::rename(&tmp_manifest_name, &manifest_name) {
+    if let Err(err) = tokio::fs::rename(&tmp_manifest_name, &manifest_name).await {
         bail!("{prefix}: Atomic rename file {manifest_name:?} failed - {err}");
     }
     if let DatastoreBackend::S3(s3_client) = backend {
@@ -910,8 +910,9 @@ async fn pull_snapshot<'a>(
 
     fetch_log(crypt_config).await?;
 
-    snapshot
-        .cleanup_unreferenced_files(&manifest)
+    let snapshot = snapshot.clone();
+    tokio::task::spawn_blocking(move || snapshot.cleanup_unreferenced_files(&manifest))
+        .await?
         .map_err(|err| format_err!("{prefix}: failed to cleanup unreferenced files - {err}"))?;
 
     Ok(Some(sync_stats))
-- 
2.47.3

[PATCH proxmox-backup v2 2/3] fix #6990: server: drop verify state on push job

[Jakob Klocker]

When pushing a snapshot the target manifest is recreated from the source
manifest, copying its verify_state. As with pull, this makes the pushed
snapshot appear verified on the remote without its stored copy having
been checked there.
Strip verify_state after copying the unprotected section so the pushed
snapshot is verified independently on the target.

Link: https://bugzilla.proxmox.com/show_bug.cgi?id=6990
Signed-off-by: Jakob Klocker <j.klocker@proxmox.com>
---
 src/server/push.rs | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/src/server/push.rs b/src/server/push.rs
index 9a69f5ce8..456b025fd 100644
--- a/src/server/push.rs
+++ b/src/server/push.rs
@@ -1416,6 +1416,13 @@ pub(crate) async fn push_snapshot(
     } else {
         target_manifest.signature = source_manifest.signature.clone();
     };
+
+    if let Some(unprotected) = target_manifest.unprotected.as_object_mut() {
+        unprotected.remove("verify_state");
+    } else {
+        bail!("Encountered unexpected manifest without 'unprotected' section.");
+    }
+
     // FIXME: replace me with to_data_blob once there is an upload_blob
     let manifest_string =
         target_manifest.to_string(encrypt_using_key.map(|(_, config)| config).as_deref())?;
-- 
2.47.3

[PATCH proxmox-backup v2 3/3] fix #6990: server: drop verify state on non-decrypt pull job

[Jakob Klocker]

On a non-decrypt pull the source manifest is written to the target
as-is, so the target inherits the source's verify_state flag instead of
being verified independently on its own storage. Because a snapshot
carrying a verify_state is skipped by verify jobs, the target's copy can
never be checked.

The decrypt path already drops verify_state; do the same on the
non-decrypt path when the snapshot is newly pulled or re-synced due to
corruption. Also factor the manifest blob encoding and writing into a
helper, shared by both paths.

Link: https://bugzilla.proxmox.com/show_bug.cgi?id=6990
Signed-off-by: Jakob Klocker <j.klocker@proxmox.com>
---
 pbs-datastore/src/manifest.rs | 25 ++++++++++++++++++-
 src/server/pull.rs            | 47 +++++++++++++++++++++++------------
 2 files changed, 55 insertions(+), 17 deletions(-)

diff --git a/pbs-datastore/src/manifest.rs b/pbs-datastore/src/manifest.rs
index 11de1085b..c87797474 100644
--- a/pbs-datastore/src/manifest.rs
+++ b/pbs-datastore/src/manifest.rs
@@ -1,5 +1,8 @@
-use anyhow::{Error, bail, format_err};
+use std::io::Write;
+use std::os::fd::AsRawFd;
+use std::path::Path;
 
+use anyhow::{Context, Error, bail, format_err};
 use serde::{Deserialize, Serialize};
 use serde_json::{Value, json};
 
@@ -278,6 +281,26 @@ impl BackupManifest {
 
         Ok(Some(Deserialize::deserialize(value)?))
     }
+
+    /// Encode the manifest and write the raw blob data to `path`, fsync'ing it.
+    ///
+    /// Returns the raw blob data.
+    pub fn write_to_path(&self, path: &Path) -> Result<Vec<u8>, Error> {
+        let raw_data = self.to_data_blob(None)?.raw_data().to_vec();
+
+        let mut file = std::fs::OpenOptions::new()
+            .write(true)
+            .create(true)
+            .truncate(true)
+            .open(path)
+            .with_context(|| format!("failed to open manifest {path:?}"))?;
+
+        file.write_all(&raw_data)?;
+        file.flush()?;
+        nix::unistd::fsync(file.as_raw_fd())?;
+
+        Ok(raw_data)
+    }
 }
 
 impl TryFrom<DataBlob> for BackupManifest {
diff --git a/src/server/pull.rs b/src/server/pull.rs
index fd401ac85..acb7a98dc 100644
--- a/src/server/pull.rs
+++ b/src/server/pull.rs
@@ -3,7 +3,6 @@
 use std::collections::hash_map::Entry;
 use std::collections::{HashMap, HashSet};
 use std::io::Seek;
-use std::os::fd::AsRawFd;
 use std::sync::atomic::{AtomicU64, AtomicUsize, Ordering};
 use std::sync::{Arc, Mutex};
 use std::time::{Duration, SystemTime};
@@ -16,8 +15,6 @@ use super::sync::{
 use crate::backup::{check_ns_modification_privs, check_ns_privs};
 use crate::server::sync::SharedGroupProgress;
 use anyhow::{Context, Error, bail, format_err};
-use tokio::fs::OpenOptions;
-use tokio::io::AsyncWriteExt;
 
 use pbs_api_types::{
     ArchiveType, Authid, BackupDir, BackupGroup, BackupNamespace, CLIENT_LOG_BLOB_NAME, CryptMode,
@@ -741,7 +738,8 @@ async fn pull_snapshot<'a>(
     };
 
     let mut manifest_data = tmp_manifest_blob.raw_data().to_vec();
-    let manifest = BackupManifest::try_from(tmp_manifest_blob).with_context(|| prefix.clone())?;
+    let mut manifest =
+        Arc::new(BackupManifest::try_from(tmp_manifest_blob).with_context(|| prefix.clone())?);
 
     if ignore_not_verified_or_encrypted(
         &manifest,
@@ -849,6 +847,10 @@ async fn pull_snapshot<'a>(
         sync_stats.add(stats);
     }
 
+    let target_verify_state = existing_target_manifest
+        .as_ref()
+        .and_then(|m| m.unprotected.get("verify_state").cloned());
+
     if let Some(new_manifest) = new_manifest {
         let mut new_manifest = Arc::try_unwrap(new_manifest)
             .map_err(|_arc| {
@@ -875,19 +877,32 @@ async fn pull_snapshot<'a>(
             new_manifest.set_sync_source_signature(expected.bytes())?;
         }
 
-        // keep signature
-        let manifest_blob = new_manifest.to_data_blob(None)?;
-        // update contents to be uploaded to backend
-        manifest_data = manifest_blob.raw_data().to_vec();
+        let tmp_manifest_name = tmp_manifest_name.clone();
+        manifest_data =
+            tokio::task::spawn_blocking(move || new_manifest.write_to_path(&tmp_manifest_name))
+                .await??;
+    } else if manifest.unprotected.get("verify_state") != target_verify_state.as_ref() {
+        // the manifest is copied as-is on the non-decrypted path: never inherit the
+        // source's verify state, but keep one the target obtained on its own
+        let manifest_mut = Arc::get_mut(&mut manifest)
+            .ok_or_else(|| format_err!("{prefix}: manifest unexpectedly shared"))?;
+        let Some(unprotected) = manifest_mut.unprotected.as_object_mut() else {
+            bail!("{prefix}: unexpected manifest without 'unprotected' section");
+        };
+        match &target_verify_state {
+            Some(state) => {
+                unprotected.insert("verify_state".to_string(), state.clone());
+            }
+            None => {
+                unprotected.remove("verify_state");
+            }
+        }
 
-        let mut tmp_manifest_file = OpenOptions::new()
-            .write(true)
-            .truncate(true) // clear pre-existing manifest content
-            .open(&tmp_manifest_name)
-            .await?;
-        tmp_manifest_file.write_all(&manifest_data).await?;
-        tmp_manifest_file.flush().await?;
-        nix::unistd::fsync(tmp_manifest_file.as_raw_fd())?;
+        let manifest = Arc::clone(&manifest);
+        let tmp_manifest_name = tmp_manifest_name.clone();
+        manifest_data =
+            tokio::task::spawn_blocking(move || manifest.write_to_path(&tmp_manifest_name))
+                .await??;
     }
 
     if let Err(err) = tokio::fs::rename(&tmp_manifest_name, &manifest_name).await {
-- 
2.47.3

Re: [PATCH proxmox-backup v2 1/3] server: pull: run blocking file operations on the blocking pool

[Christian Ebner]

One comment inline, with that addressed consider:

Reviewed-by: Christian Ebner <c.ebner@proxmox.com>

On 8/21/26 1:18 PM, Jakob Klocker wrote:
> The atomic renames and the unreferenced-file cleanup are blocking
> operations that ran directly on the runtime's worker threads, stalling
> other tasks scheduled there for their duration.
> 
> Use the tokio counterpart for the renames and move the cleanup onto the
> blocking thread pool.
> 
> Signed-off-by: Jakob Klocker <j.klocker@proxmox.com>
> ---
>   src/server/pull.rs | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/src/server/pull.rs b/src/server/pull.rs
> index 4eb5bcf11..fd401ac85 100644
> --- a/src/server/pull.rs
> +++ b/src/server/pull.rs
> @@ -600,7 +600,7 @@ async fn pull_single_archive<'a>(
>       } else {
>           tmp_path
>       };
> -    if let Err(err) = std::fs::rename(&source_path, &path) {
> +    if let Err(err) = tokio::fs::rename(&source_path, &path).await {

comment: there is an additional std::fs::remove_file() in the pull code, 
e.g. removing the temp file just above this (not seen in diff). All of 
these should be moved to tokio::fs::remove_file().

We need to be careful with performance when using tokio::fs over std::fs 
as stated in [0], but these should be fine to call on the blocking task 
pool. That this might have performance implications might be also noted 
in the commit message.

[0] https://docs.rs/tokio/latest/tokio/fs/index.html

>           bail!("{archive_prefix}: Atomic rename file {path:?} failed - {err}");
>       }
>   
> @@ -890,7 +890,7 @@ async fn pull_snapshot<'a>(
>           nix::unistd::fsync(tmp_manifest_file.as_raw_fd())?;
>       }
>   
> -    if let Err(err) = std::fs::rename(&tmp_manifest_name, &manifest_name) {
> +    if let Err(err) = tokio::fs::rename(&tmp_manifest_name, &manifest_name).await {
>           bail!("{prefix}: Atomic rename file {manifest_name:?} failed - {err}");
>       }
>       if let DatastoreBackend::S3(s3_client) = backend {
> @@ -910,8 +910,9 @@ async fn pull_snapshot<'a>(
>   
>       fetch_log(crypt_config).await?;
>   
> -    snapshot
> -        .cleanup_unreferenced_files(&manifest)
> +    let snapshot = snapshot.clone();
> +    tokio::task::spawn_blocking(move || snapshot.cleanup_unreferenced_files(&manifest))
> +        .await?
>           .map_err(|err| format_err!("{prefix}: failed to cleanup unreferenced files - {err}"))?;
>   
>       Ok(Some(sync_stats))

Re: [PATCH proxmox-backup v2 2/3] fix #6990: server: drop verify state on push job

[Christian Ebner]

On 8/21/26 1:18 PM, Jakob Klocker wrote:
> When pushing a snapshot the target manifest is recreated from the source
> manifest, copying its verify_state. As with pull, this makes the pushed
> snapshot appear verified on the remote without its stored copy having
> been checked there.
> Strip verify_state after copying the unprotected section so the pushed
> snapshot is verified independently on the target.
> 
> Link: https://bugzilla.proxmox.com/show_bug.cgi?id=6990
> Signed-off-by: Jakob Klocker <j.klocker@proxmox.com>
> ---

Reviewed-by: Christian Ebner <c.ebner@proxmox.com>

Re: [PATCH proxmox-backup v2 3/3] fix #6990: server: drop verify state on non-decrypt pull job

[Christian Ebner]

Two smaller comments and considerations inline, rest looks good to me!

On 8/21/26 1:18 PM, Jakob Klocker wrote:
> On a non-decrypt pull the source manifest is written to the target
> as-is, so the target inherits the source's verify_state flag instead of
> being verified independently on its own storage. Because a snapshot
> carrying a verify_state is skipped by verify jobs, the target's copy can
> never be checked.
> 
> The decrypt path already drops verify_state; do the same on the
> non-decrypt path when the snapshot is newly pulled or re-synced due to
> corruption. Also factor the manifest blob encoding and writing into a
> helper, shared by both paths.
> 
> Link: https://bugzilla.proxmox.com/show_bug.cgi?id=6990
> Signed-off-by: Jakob Klocker <j.klocker@proxmox.com>
> ---
>   pbs-datastore/src/manifest.rs | 25 ++++++++++++++++++-
>   src/server/pull.rs            | 47 +++++++++++++++++++++++------------
>   2 files changed, 55 insertions(+), 17 deletions(-)
> 
> diff --git a/pbs-datastore/src/manifest.rs b/pbs-datastore/src/manifest.rs
> index 11de1085b..c87797474 100644
> --- a/pbs-datastore/src/manifest.rs
> +++ b/pbs-datastore/src/manifest.rs
> @@ -1,5 +1,8 @@
> -use anyhow::{Error, bail, format_err};
> +use std::io::Write;
> +use std::os::fd::AsRawFd;
> +use std::path::Path;
>   
> +use anyhow::{Context, Error, bail, format_err};
>   use serde::{Deserialize, Serialize};
>   use serde_json::{Value, json};
>   
> @@ -278,6 +281,26 @@ impl BackupManifest {
>   
>           Ok(Some(Deserialize::deserialize(value)?))
>       }
> +
> +    /// Encode the manifest and write the raw blob data to `path`, fsync'ing it.

comment: I would like for this comment to also include a note/warning 
that this should only be used to write manifest files to temp file path 
(or maybe we even want to check/encode that)? If this will be used to 
write the mainfest directly, it would lead to concurrent readers to 
potentially read incomplete and therefore corrupt manifests.

Another option and probably even preferable would be for the rename to 
be included in this helper as well and maybe pass both, tmp and target 
path. From the diff below that should be doable and give us guarantees 
that the manifest is persisted atomically.


Also this whole content writing could be performed as async via 
tokio::fs::File, but taking the performance considerations into account 
[0] it probably is best kept sync for now. Major win would be that this 
could then use `io-uring` for some operations if enabled in tokio in the 
future without code changes, e.g. the OpenOptions used by File have 
config and feature flags for this [1].

[0] https://docs.rs/tokio/latest/tokio/fs/index.html
[1] https://docs.rs/tokio/latest/src/tokio/fs/open_options.rs.html#122

> +    ///
> +    /// Returns the raw blob data.
> +    pub fn write_to_path(&self, path: &Path) -> Result<Vec<u8>, Error> {

This could take ownership of the manifest instead, so no further 
modifications are to be made afterwards and we can get rid of the Arc 
below, since it can be moved without issues into the spawn_blocking call

> +        let raw_data = self.to_data_blob(None)?.raw_data().to_vec();
> +
> +        let mut file = std::fs::OpenOptions::new()
> +            .write(true)
> +            .create(true)
> +            .truncate(true)
> +            .open(path)
> +            .with_context(|| format!("failed to open manifest {path:?}"))?;
> +
> +        file.write_all(&raw_data)?;
> +        file.flush()?;
> +        nix::unistd::fsync(file.as_raw_fd())?;
> +
> +        Ok(raw_data)
> +    }
>   }
>   
>   impl TryFrom<DataBlob> for BackupManifest {
> diff --git a/src/server/pull.rs b/src/server/pull.rs
> index fd401ac85..acb7a98dc 100644
> --- a/src/server/pull.rs
> +++ b/src/server/pull.rs
> @@ -3,7 +3,6 @@
>   use std::collections::hash_map::Entry;
>   use std::collections::{HashMap, HashSet};
>   use std::io::Seek;
> -use std::os::fd::AsRawFd;
>   use std::sync::atomic::{AtomicU64, AtomicUsize, Ordering};
>   use std::sync::{Arc, Mutex};
>   use std::time::{Duration, SystemTime};
> @@ -16,8 +15,6 @@ use super::sync::{
>   use crate::backup::{check_ns_modification_privs, check_ns_privs};
>   use crate::server::sync::SharedGroupProgress;
>   use anyhow::{Context, Error, bail, format_err};
> -use tokio::fs::OpenOptions;
> -use tokio::io::AsyncWriteExt;
>   
>   use pbs_api_types::{
>       ArchiveType, Authid, BackupDir, BackupGroup, BackupNamespace, CLIENT_LOG_BLOB_NAME, CryptMode,
> @@ -741,7 +738,8 @@ async fn pull_snapshot<'a>(
>       };
>   
>       let mut manifest_data = tmp_manifest_blob.raw_data().to_vec();
> -    let manifest = BackupManifest::try_from(tmp_manifest_blob).with_context(|| prefix.clone())?;
> +    let mut manifest =
> +        Arc::new(BackupManifest::try_from(tmp_manifest_blob).with_context(|| prefix.clone())?);
>   
>       if ignore_not_verified_or_encrypted(
>           &manifest,
> @@ -849,6 +847,10 @@ async fn pull_snapshot<'a>(
>           sync_stats.add(stats);
>       }
>   
> +    let target_verify_state = existing_target_manifest
> +        .as_ref()
> +        .and_then(|m| m.unprotected.get("verify_state").cloned());
> +
>       if let Some(new_manifest) = new_manifest {
>           let mut new_manifest = Arc::try_unwrap(new_manifest)
>               .map_err(|_arc| {
> @@ -875,19 +877,32 @@ async fn pull_snapshot<'a>(
>               new_manifest.set_sync_source_signature(expected.bytes())?;
>           }
>   
> -        // keep signature
> -        let manifest_blob = new_manifest.to_data_blob(None)?;
> -        // update contents to be uploaded to backend
> -        manifest_data = manifest_blob.raw_data().to_vec();
> +        let tmp_manifest_name = tmp_manifest_name.clone();
> +        manifest_data =
> +            tokio::task::spawn_blocking(move || new_manifest.write_to_path(&tmp_manifest_name))
> +                .await??;
> +    } else if manifest.unprotected.get("verify_state") != target_verify_state.as_ref() {
> +        // the manifest is copied as-is on the non-decrypted path: never inherit the
> +        // source's verify state, but keep one the target obtained on its own
> +        let manifest_mut = Arc::get_mut(&mut manifest)
> +            .ok_or_else(|| format_err!("{prefix}: manifest unexpectedly shared"))?;

... as commented above, by taking ownership in 
BackupManifest::write_to_path() there is no need for the Arc anymore, 
and this can be simplified.

> +        let Some(unprotected) = manifest_mut.unprotected.as_object_mut() else {
> +            bail!("{prefix}: unexpected manifest without 'unprotected' section");
> +        };
> +        match &target_verify_state {
> +            Some(state) => {
> +                unprotected.insert("verify_state".to_string(), state.clone());
> +            }
> +            None => {
> +                unprotected.remove("verify_state");
> +            }
> +        }
>   
> -        let mut tmp_manifest_file = OpenOptions::new()
> -            .write(true)
> -            .truncate(true) // clear pre-existing manifest content
> -            .open(&tmp_manifest_name)
> -            .await?;
> -        tmp_manifest_file.write_all(&manifest_data).await?;
> -        tmp_manifest_file.flush().await?;
> -        nix::unistd::fsync(tmp_manifest_file.as_raw_fd())?;
> +        let manifest = Arc::clone(&manifest);
> +        let tmp_manifest_name = tmp_manifest_name.clone();
> +        manifest_data =
> +            tokio::task::spawn_blocking(move || manifest.write_to_path(&tmp_manifest_name))
> +                .await??;
>       }
>   
>       if let Err(err) = tokio::fs::rename(&tmp_manifest_name, &manifest_name).await {

... same as mentioned above the rename could be part of write_to_path 
and only the remaining else branch handled via the rename. Would help to 
protect against misusing write_to_path().