public inbox for pbs-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [PATCH proxmox-backup] fix #7305: client: restore: filter out last snapshot if not finished
@ 2026-02-13 10:43 Christian Ebner
  2026-02-13 10:45 ` Christian Ebner
  2026-02-13 10:51 ` Fabian Grünbichler
  0 siblings, 2 replies; 6+ messages in thread
From: Christian Ebner @ 2026-02-13 10:43 UTC (permalink / raw)
  To: pbs-devel

When invoking the client CLI for restore via:
```
proxmox-backup-client restore <snapshot> <archive> <target>
```
but <archive> providing a backup group only, the client falls back to
restore the last snapshot of that group for convenience.

The snapshot listing used to identify the last snapshot returns
however all snapshots, including unfinished ones, a restore on an
unfinished one will therefore fail.

Fix this by filtering the snapshot to be restored by checking the
presence of the manifest file and are therefore considered finished.
If the file is not present, skip the snapshot and try for the next
one, if any. Also adapt the helper function name to reflect this
change.

Ideally this would happen on the server side, guarded by some flag,
but that will not work with older server versions.

Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=7305
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
changes since version 1:
- Skip any considered snapshot without manifest
- Refactor based on Fabian's suggestions

 proxmox-backup-client/src/main.rs | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs
index 999e50205..10a864a0f 100644
--- a/proxmox-backup-client/src/main.rs
+++ b/proxmox-backup-client/src/main.rs
@@ -160,7 +160,7 @@ async fn api_datastore_list_snapshots(
     Ok(result["data"].take())
 }
 
-pub async fn api_datastore_latest_snapshot(
+pub async fn api_datastore_latest_finished_snapshot(
     client: &HttpClient,
     store: &str,
     ns: &BackupNamespace,
@@ -175,7 +175,30 @@ pub async fn api_datastore_latest_snapshot(
 
     list.sort_unstable_by(|a, b| b.backup.time.cmp(&a.backup.time));
 
-    Ok((group, list[0].backup.time).into())
+    let last_finished_time = list
+        .iter()
+        .find_map(|snapshot_item| {
+            // only once a snapshots contains the manifest it is considered finished
+            snapshot_item
+                .files
+                .iter()
+                .find_map(|content| {
+                    if content.filename == MANIFEST_BLOB_NAME.as_ref() {
+                        Some(snapshot_item.backup.time)
+                    } else {
+                        None
+                    }
+                })
+                .or_else(|| {
+                    log::info!("skipped not finished snapshot {}", snapshot_item.backup);
+                    None
+                })
+        })
+        .ok_or_else(|| {
+            format_err!("backup group {group} does not contain any finished snapshots.")
+        })?;
+
+    Ok((group, last_finished_time).into())
 }
 
 pub async fn dir_or_last_from_group(
@@ -187,7 +210,7 @@ pub async fn dir_or_last_from_group(
     match path.parse::<BackupPart>()? {
         BackupPart::Dir(dir) => Ok(dir),
         BackupPart::Group(group) => {
-            api_datastore_latest_snapshot(client, repo.store(), ns, group).await
+            api_datastore_latest_finished_snapshot(client, repo.store(), ns, group).await
         }
     }
 }
-- 
2.47.3





^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH proxmox-backup] fix #7305: client: restore: filter out last snapshot if not finished
  2026-02-13 10:43 [PATCH proxmox-backup] fix #7305: client: restore: filter out last snapshot if not finished Christian Ebner
@ 2026-02-13 10:45 ` Christian Ebner
  2026-02-13 10:51 ` Fabian Grünbichler
  1 sibling, 0 replies; 6+ messages in thread
From: Christian Ebner @ 2026-02-13 10:45 UTC (permalink / raw)
  To: pbs-devel

missing v2 subject prefix, sorry for that




^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH proxmox-backup] fix #7305: client: restore: filter out last snapshot if not finished
  2026-02-13 10:43 [PATCH proxmox-backup] fix #7305: client: restore: filter out last snapshot if not finished Christian Ebner
  2026-02-13 10:45 ` Christian Ebner
@ 2026-02-13 10:51 ` Fabian Grünbichler
  1 sibling, 0 replies; 6+ messages in thread
From: Fabian Grünbichler @ 2026-02-13 10:51 UTC (permalink / raw)
  To: pbs-devel, Christian Ebner


On Fri, 13 Feb 2026 11:43:04 +0100, Christian Ebner wrote:
> When invoking the client CLI for restore via:
> ```
> proxmox-backup-client restore <snapshot> <archive> <target>
> ```
> but <archive> providing a backup group only, the client falls back to
> restore the last snapshot of that group for convenience.
> 
> [...]

Applied v2, thanks!

[1/1] fix #7305: client: restore: filter out last snapshot if not finished
      commit: 803d3c478671a092fb2ad836a35469a0e9192790

Best regards,
-- 
Fabian Grünbichler <f.gruenbichler@proxmox.com>




^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH proxmox-backup] fix #7305: client: restore: filter out last snapshot if not finished
  2026-02-12 14:54 ` Fabian Grünbichler
@ 2026-02-13  8:54   ` Christian Ebner
  0 siblings, 0 replies; 6+ messages in thread
From: Christian Ebner @ 2026-02-13  8:54 UTC (permalink / raw)
  To: Fabian Grünbichler, pbs-devel

On 2/12/26 3:54 PM, Fabian Grünbichler wrote:
> On February 11, 2026 4:24 pm, Christian Ebner wrote:
>> +    // only once a snapshots contains the manifest it is considered finished
>> +    let last_is_finished = list[0]
>> +        .files
>> +        .iter()
>> +        .any(|content| content.filename == MANIFEST_BLOB_NAME.as_ref());
>> +
>> +    if !last_is_finished {
>> +        if list.len() > 1 {
>> +            log::info!("last snapshot of group {group} not finished, use previous instead");
>> +            return Ok((group, list[1].backup.time).into());
>> +        } else {
>> +            bail!("backup group {group} does not contain finished snapshots.");
>> +        }
>> +    }
> 
> couldn't this be a combinator instead?
> 
> i.e., something like
> 
>      let last_finished_time = list
>          .iter()
>          .find_map(|snap| {
>              snap.files.iter().find_map(|content| {
>                  if content.filename == MANIFEST_BLOB_NAME.as_ref() {
>                      Some(snap.backup.time)
>                  } else {
>                      None
>                  }
>              })
>          })
>          .ok_or_else(|| {
>              format_err!("backup group {group} does not contain any finished snapshots.")
>          })?;
> 
> to account for multiple snapshots with missing manifests? those should
> not occur "naturally", but might exist cause of accidents/..

Yes, good point! Would however opt for logging each skipped snapshot 
instead of silently progressing to the next one. This might help debug 
in cases where such issues come up.

Will send version 2 shortly, thanks!




^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH proxmox-backup] fix #7305: client: restore: filter out last snapshot if not finished
  2026-02-11 15:24 Christian Ebner
@ 2026-02-12 14:54 ` Fabian Grünbichler
  2026-02-13  8:54   ` Christian Ebner
  0 siblings, 1 reply; 6+ messages in thread
From: Fabian Grünbichler @ 2026-02-12 14:54 UTC (permalink / raw)
  To: Christian Ebner, pbs-devel

On February 11, 2026 4:24 pm, Christian Ebner wrote:
> When invoking the client CLI for restore via:
> ```
> proxmox-backup-client restore <snapshot> <archive> <target>
> ```
> the <archive> provide a backup group only, in which case the client
> falls back to restore the last snapshot of that group as convenience.
> 
> The snapshot listing used to identify the last snapshot returns
> however all snapshots, including unfinished ones, a restore on an
> unfinished one will therefore fail.
> 
> Fix this by filtering out snapshots which do not contain a manifest
> file yet, and therefore are not considered as finished. Also adapt
> the helper function name to reflect this change. Ideally this would
> happen on the server side, guarded by some flag, but that will not
> work with older server versions.
> 
> Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=7305
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
> Note:
> The behaviour can be easily tested by simply temporarily renaming the
> index.json.blob in the last snapshot of a backup group to simulate an
> ongoing backup for that snapshot.
> 
>  proxmox-backup-client/src/main.rs | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs
> index 999e50205..7e20b9cc8 100644
> --- a/proxmox-backup-client/src/main.rs
> +++ b/proxmox-backup-client/src/main.rs
> @@ -160,7 +160,7 @@ async fn api_datastore_list_snapshots(
>      Ok(result["data"].take())
>  }
>  
> -pub async fn api_datastore_latest_snapshot(
> +pub async fn api_datastore_latest_finished_snapshot(
>      client: &HttpClient,
>      store: &str,
>      ns: &BackupNamespace,
> @@ -175,6 +175,21 @@ pub async fn api_datastore_latest_snapshot(
>  
>      list.sort_unstable_by(|a, b| b.backup.time.cmp(&a.backup.time));
>  
> +    // only once a snapshots contains the manifest it is considered finished
> +    let last_is_finished = list[0]
> +        .files
> +        .iter()
> +        .any(|content| content.filename == MANIFEST_BLOB_NAME.as_ref());
> +
> +    if !last_is_finished {
> +        if list.len() > 1 {
> +            log::info!("last snapshot of group {group} not finished, use previous instead");
> +            return Ok((group, list[1].backup.time).into());
> +        } else {
> +            bail!("backup group {group} does not contain finished snapshots.");
> +        }
> +    }

couldn't this be a combinator instead?

i.e., something like

    let last_finished_time = list
        .iter()
        .find_map(|snap| {
            snap.files.iter().find_map(|content| {
                if content.filename == MANIFEST_BLOB_NAME.as_ref() {
                    Some(snap.backup.time)
                } else {
                    None
                }
            })
        })
        .ok_or_else(|| {
            format_err!("backup group {group} does not contain any finished snapshots.")
        })?;

to account for multiple snapshots with missing manifests? those should
not occur "naturally", but might exist cause of accidents/..

> +
>      Ok((group, list[0].backup.time).into())
>  }
>  
> @@ -187,7 +202,7 @@ pub async fn dir_or_last_from_group(
>      match path.parse::<BackupPart>()? {
>          BackupPart::Dir(dir) => Ok(dir),
>          BackupPart::Group(group) => {
> -            api_datastore_latest_snapshot(client, repo.store(), ns, group).await
> +            api_datastore_latest_finished_snapshot(client, repo.store(), ns, group).await
>          }
>      }
>  }
> -- 
> 2.47.3
> 
> 
> 
> 
> 
> 




^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH proxmox-backup] fix #7305: client: restore: filter out last snapshot if not finished
@ 2026-02-11 15:24 Christian Ebner
  2026-02-12 14:54 ` Fabian Grünbichler
  0 siblings, 1 reply; 6+ messages in thread
From: Christian Ebner @ 2026-02-11 15:24 UTC (permalink / raw)
  To: pbs-devel

When invoking the client CLI for restore via:
```
proxmox-backup-client restore <snapshot> <archive> <target>
```
the <archive> provide a backup group only, in which case the client
falls back to restore the last snapshot of that group as convenience.

The snapshot listing used to identify the last snapshot returns
however all snapshots, including unfinished ones, a restore on an
unfinished one will therefore fail.

Fix this by filtering out snapshots which do not contain a manifest
file yet, and therefore are not considered as finished. Also adapt
the helper function name to reflect this change. Ideally this would
happen on the server side, guarded by some flag, but that will not
work with older server versions.

Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=7305
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
Note:
The behaviour can be easily tested by simply temporarily renaming the
index.json.blob in the last snapshot of a backup group to simulate an
ongoing backup for that snapshot.

 proxmox-backup-client/src/main.rs | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs
index 999e50205..7e20b9cc8 100644
--- a/proxmox-backup-client/src/main.rs
+++ b/proxmox-backup-client/src/main.rs
@@ -160,7 +160,7 @@ async fn api_datastore_list_snapshots(
     Ok(result["data"].take())
 }
 
-pub async fn api_datastore_latest_snapshot(
+pub async fn api_datastore_latest_finished_snapshot(
     client: &HttpClient,
     store: &str,
     ns: &BackupNamespace,
@@ -175,6 +175,21 @@ pub async fn api_datastore_latest_snapshot(
 
     list.sort_unstable_by(|a, b| b.backup.time.cmp(&a.backup.time));
 
+    // only once a snapshots contains the manifest it is considered finished
+    let last_is_finished = list[0]
+        .files
+        .iter()
+        .any(|content| content.filename == MANIFEST_BLOB_NAME.as_ref());
+
+    if !last_is_finished {
+        if list.len() > 1 {
+            log::info!("last snapshot of group {group} not finished, use previous instead");
+            return Ok((group, list[1].backup.time).into());
+        } else {
+            bail!("backup group {group} does not contain finished snapshots.");
+        }
+    }
+
     Ok((group, list[0].backup.time).into())
 }
 
@@ -187,7 +202,7 @@ pub async fn dir_or_last_from_group(
     match path.parse::<BackupPart>()? {
         BackupPart::Dir(dir) => Ok(dir),
         BackupPart::Group(group) => {
-            api_datastore_latest_snapshot(client, repo.store(), ns, group).await
+            api_datastore_latest_finished_snapshot(client, repo.store(), ns, group).await
         }
     }
 }
-- 
2.47.3





^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2026-02-13 10:51 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-02-13 10:43 [PATCH proxmox-backup] fix #7305: client: restore: filter out last snapshot if not finished Christian Ebner
2026-02-13 10:45 ` Christian Ebner
2026-02-13 10:51 ` Fabian Grünbichler
  -- strict thread matches above, loose matches on Subject: below --
2026-02-11 15:24 Christian Ebner
2026-02-12 14:54 ` Fabian Grünbichler
2026-02-13  8:54   ` Christian Ebner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal