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

["Jakob Klocker" <j.klocker@proxmox.com>]

Thanks for the feedback!

On Thu Jul 23, 2026 at 1:08 PM CEST, Christian Ebner wrote:
> Thanks for the patch, mostly looks good to me.
>
> Just one comment and nit inline.
>
> On 7/9/26 2:22 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
>>   
>>[SNIP]
>> +async fn write_manifest_blob(
>> +    manifest: &BackupManifest,
>> +    path: &std::path::Path,
>> +) -> Result<Vec<u8>, Error> {
>> +    let manifest_blob = DataBlob::encode(manifest.to_string(None)?.as_bytes(), None, true)?;
>> +    let manifest_data = manifest_blob.raw_data().to_vec();
>> +    let mut manifest_file = OpenOptions::new()
>> +        .write(true)
>> +        .truncate(true)
>> +        .open(path)
>> +        .await?;
>> +    manifest_file.write_all(&manifest_data).await?;
>> +    manifest_file.flush().await?;
>> +    nix::unistd::fsync(manifest_file.as_raw_fd())?;
>
> nit: pre-existing but this is a blocking call and we are in an async 
> context, so we should spawn a blocking tokio task for this I guess. Same 
> is true for the std::fs::rename which should be replaced by it's 
> tokio::fs::rename counterpart and the call to 
> BackupDir::cleanup_unreferenced_files().
>
> Could you also include additional patches to fix these since touching this?
>

I'll make sure to fix these in a v2.

>> +    Ok(manifest_data)
>> +}
>
> comment: This should rather live as method impl on BackupManifest itself 
> IMO. It could further be refactored to reduce common code with 
> pbs-datastores BackupDir::update_manifest().
>

Makes sense moving this to BackupManifest. Thanks for pointing me to
`update_manifest`, I'll further refactor this.

>>   /// Check if the decryption key should be used to decrypt the snapshot during
>>   /// pull based on given pull parameter, source and optionally already present
>>   /// target manifest.