[pbs-devel] [PATCH proxmox-backup] fix #3336: datastore: remove group if the last snapshot is removed

[Shannon Sterz]

empty groups are not visible in the gui. this led to a confusing issue
where users were unable to create a group because it already existed
and was still owned by another user. resolve this issue by removing
the group if its last snapshot is removed.

also fixes an issue where removing a group used the non-atomic
`remove_dir_all()` function when destroying a group unconditionally.
this could lead to two different threads suddenly holding a lock to
the same group. make sure that the new locking mechanism is used,
which prevents that, before removing the group. this is also a bit
more conservative now, as it specifically removes the owner file and
group directory separately to avoid accidentaly removing snapshots in
case we made an oversight.

Signed-off-by: Shannon Sterz <s.sterz at proxmox.com>
---
 pbs-datastore/src/backup_info.rs | 36 +++++++++++++++++++++++++++-----
 pbs-datastore/src/datastore.rs   |  6 +++++-
 2 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_info.rs
index 396b6bde..557bb196 100644
--- a/pbs-datastore/src/backup_info.rs
+++ b/pbs-datastore/src/backup_info.rs
@@ -232,17 +232,34 @@ impl BackupGroup {
             delete_stats.increment_removed_snapshots();
         }
 
-        if delete_stats.all_removed() {
-            std::fs::remove_dir_all(&path).map_err(|err| {
-                format_err!("removing group directory {:?} failed - {}", path, err)
-            })?;
+        // Note: make sure the old locking mechanism isn't used as `remove_dir_all` is not safe in
+        // that case
+        if delete_stats.all_removed() && !*OLD_LOCKING {
+            self.remove_group_dir()?;
             delete_stats.increment_removed_groups();
         }
 
-        let _ = std::fs::remove_file(self.lock_path());
         Ok(delete_stats)
     }
 
+    /// Helper function, assumes that no more snapshots are present in the group.
+    fn remove_group_dir(&self) -> Result<(), Error> {
+        let owner_path = self.store.owner_path(&self.ns, &self.group);
+
+        std::fs::remove_file(&owner_path).map_err(|err| {
+            format_err!("removing the owner file '{owner_path:?}' failed - {err}")
+        })?;
+
+        let path = self.full_group_path();
+
+        std::fs::remove_dir(&path)
+            .map_err(|err| format_err!("removing group directory {path:?} failed - {err}"))?;
+
+        let _ = std::fs::remove_file(self.lock_path());
+
+        Ok(())
+    }
+
     /// Returns the backup owner.
     ///
     /// The backup owner is the entity who first created the backup group.
@@ -581,6 +598,15 @@ impl BackupDir {
         let _ = std::fs::remove_file(self.manifest_lock_path()); // ignore errors
         let _ = std::fs::remove_file(self.lock_path()); // ignore errors
 
+        let group = BackupGroup::from(self);
+        let _guard = group.lock().with_context(|| {
+            format!("while checking if group '{group:?}' is empty during snapshot destruction")
+        })?;
+
+        if group.list_backups()?.is_empty() && !*OLD_LOCKING {
+            group.remove_group_dir()?;
+        }
+
         Ok(())
     }
 
diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 1e6157c0..ae4fb7f8 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -706,7 +706,11 @@ impl DataStore {
     }
 
     /// Return the path of the 'owner' file.
-    fn owner_path(&self, ns: &BackupNamespace, group: &pbs_api_types::BackupGroup) -> PathBuf {
+    pub(super) fn owner_path(
+        &self,
+        ns: &BackupNamespace,
+        group: &pbs_api_types::BackupGroup,
+    ) -> PathBuf {
         self.group_path(ns, group).join("owner")
     }
 
-- 
2.39.5