[pbs-devel] [PATCH proxmox-backup] pull: properly skip missing snapshots

[Dominik Csapak]

Code wise it looks good to me, and tested fine.
I am not sure though if there isn't a slight TOCTOU issue?
if the snapshot is deleted between the exist and verify_state check?
(not sure if there's a lock anyway here, couldn't tell from the surrounding code)

in that case we could maybe check err for ENOENT (if that's returned?) or returning
a custom Error type that includes that information

aside from that, consider this

Reviewed-by: Dominik Csapak <d.csapak at proxmox.com>
Tested-by: Dominik Csapak <d.csapak at proxmox.com>


On 11/27/24 09:26, Fabian Grünbichler wrote:
> when loading the verification state for a local snapshot, it must first be
> ensured that it actually exists, else the lack of manifest will be interpreted
> as corrupt snapshot triggering a "resync" that is actually a sync of all
> missing snapshots.
> 
> Fixes: 0974ddfa17be018f777d6ece90a71bfa8fc130d8 "fix #3786: api: add resync-corrupt option to sync jobs"
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler at proxmox.com>
> ---
>   src/server/pull.rs | 26 ++++++++++++++------------
>   1 file changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/src/server/pull.rs b/src/server/pull.rs
> index 9abb673ae..361ed0687 100644
> --- a/src/server/pull.rs
> +++ b/src/server/pull.rs
> @@ -551,21 +551,23 @@ async fn pull_group(
>                       .store
>                       .backup_dir(target_ns.clone(), dir.clone());
>                   if let Ok(local_dir) = local_dir {
> -                    match local_dir.verify_state() {
> -                        Ok(Some(state)) => {
> -                            if state == VerifyState::Failed {
> +                    if local_dir.full_path().exists() {
> +                        match local_dir.verify_state() {
> +                            Ok(Some(state)) => {
> +                                if state == VerifyState::Failed {
> +                                    return Some((dir, true));
> +                                }
> +                            }
> +                            Ok(None) => {
> +                                // The verify_state item was not found in the manifest, this means the
> +                                // snapshot is new.
> +                            }
> +                            Err(_) => {
> +                                // There was an error loading the manifest, probably better if we
> +                                // resync.
>                                   return Some((dir, true));
>                               }
>                           }
> -                        Ok(None) => {
> -                            // The verify_state item was not found in the manifest, this means the
> -                            // snapshot is new.
> -                        }
> -                        Err(_) => {
> -                            // There was an error loading the manifest, probably better if we
> -                            // resync.
> -                            return Some((dir, true));
> -                        }
>                       }
>                   }
>               }