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

[Fabian Grünbichler]

On November 27, 2024 10:05 am, Dominik Csapak wrote:
> 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)

there is no lock at that point, that would also make this much more
expensive (we need to do this for every existing snapshot after all).

in general, sync should handle snapshots disappearing gracefully, and
the race window here is tiny.

handling ENOENT might be nice as additional safeguard, not sure if we
properly bubble that up atm though..

> 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));
>> -                        }
>>                       }
>>                   }
>>               }
> 
> 
>