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

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

[Christian Ebner]

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
+                        drop(env);
+                        datastore.remove_backup_dir(
+                            &namespace,
+                            &snapshot,
                             true,
                         )
                     })?;
-- 
2.47.3

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

[Fabian Grünbichler]

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?)

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

[Thomas Lamprecht]

On Thu, 30 Apr 2026 15:59:31 +0200, 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.
> 
> [...]

Applied, thanks!

[1/1] api: backup: cleanup backup group created by benchmark
      commit: c14464141f54ff6312272e1843abca9783af79c2

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

[Thomas Lamprecht]

Am 04.05.26 um 10:22 schrieb Fabian Grünbichler:
>> 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` ;)

That I fixed.

> 
>> +                        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?)
> 

Argh, missed your reply before pushing this out, sorry. Will address in
a follow-up with your proposed approach - extend `force` to also skip the
group lock and revert the env-drop on top, so all four force=true sites
use the same approach.

The signature refactor (lock-guard references instead of the bool) makes
sense too, but skipping that for now to get the more minimal fix to the
repo.