Re: [PATCH proxmox-backup] api: backup: cleanup backup group created by benchmark

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On April 30, 2026 3:59 pm, Christian Ebner wrote:
> The benchmark creates it's own backup group host/benchmark, failed
> however to auto-cleanup the group after itself, because since commit
> 23be00a42 ("fix #3336: datastore: remove group if the last snapshot
> is removed"), cleanup requires an exclusive lock on the backup group
> for destroying it. The backup environment however already holds the
> exclusive lock to disallow concurrent backups to the same group.
> 
> To fix this, drop the locks held in the backup environment by
> dropping the environment itself and rely on the cleanup to reacquire
> them again.
> 
> Fixes: https://forum.proxmox.com/threads/183138/
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
>  src/api2/backup/mod.rs | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs
> index 86ec49487..8848ca99c 100644
> --- a/src/api2/backup/mod.rs
> +++ b/src/api2/backup/mod.rs
> @@ -288,9 +288,14 @@ fn upgrade_to_backup_protocol(
>                  if benchmark {
>                      env.log("benchmark finished successfully");
>                      proxmox_async::runtime::block_in_place(|| {
> -                        env.datastore.remove_backup_dir(
> -                            env.backup_dir.backup_ns(),
> -                            env.backup_dir.as_ref(),
> +                        let datastore = env.datastore.clone();
> +                        let namespace = env.backup_dir.backup_ns().clone();
> +                        let snapshot = env.backup_dir.dir().clone();
> +                        // draps all locks

nit: `draps` ;)

> +                        drop(env);
> +                        datastore.remove_backup_dir(
> +                            &namespace,
> +                            &snapshot,
>                              true,
>                          )

doesn't this also affect the "cleanup-on-error" paths a few lines below
this?

dropping the full env is also a bit problematic because it opens up a
race condition if there are back-to-back benchmarks (or backups):
- benchmark starts
- benchmark is finished, drops env
- next benchmark starts and locks group and previous "snapshot"
- cleanup fails to obtain lock(s) and doesn't run

for benchmarks that is not so bad, but for backups it would leave
half-written backup snapshots around (until they are cleaned up by other
means?).

ideally, we would not drop the locks here but just run the cleanup using
the locks we already have, which is what "force" is doing.

we currently only set force
- for the three calls here in the backup env
- when cleaning up a newly created snapshot as part of pull error
  handling

in all those case we are holding an exclusive lock on the group and on
the snapshot already. so we could just skip the group locking as well
when force is set? (ideally we'd find a way to actually encode this in
the signature, e.g. by replacing `force` with references to the lock
guards?)