fix #3336: datastore: remove group if the last snapshot is removed

Empty backup groups are not visible in the API or 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 accidentally removing snapshots in
case we made an oversight.

Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>

pbs-datastore/src/backup_info.rs

@@ -232,17 +232,34 @@ impl BackupGroup {
             delete_stats.increment_removed_snapshots();
         }
 
-        if delete_stats.all_removed() {
+        // Note: make sure the old locking mechanism isn't used as `remove_dir_all` is not safe in
-            std::fs::remove_dir_all(&path).map_err(|err| {
+        // that case
-                format_err!("removing group directory {:?} failed - {}", path, err)
+        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(())
     }
 

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