RavenDB-22849 - Move local status deletion to UpdatePeriodicBackupSta…

…tusCommand, address PR comments

src/Raven.Server/Documents/PeriodicBackup/BackupStatusStorage.cs

@@ -51,7 +51,7 @@ public void Initialize(StorageEnvironment environment, TransactionContextPool co
             }
         }
 
-        public static PeriodicBackupStatus GetBackupStatus(string databaseName, string dbId, long taskId, TransactionOperationContext context)
+        public static PeriodicBackupStatus GetBackupStatus<T>(string databaseName, string dbId, long taskId, TransactionOperationContext<T> context) where T : RavenTransaction
         {
             PeriodicBackupStatus periodicBackup = null;
             using (var backupStatusBlittable = GetBackupStatusBlittable(context, databaseName, dbId, taskId))
@@ -119,27 +119,6 @@ public static unsafe void InsertBackupStatusBlittable<T>(TransactionOperationCon
             }
         }
 
-        public bool DeleteBackupStatusesByTaskIds(string databaseName, string dbId, HashSet<long> taskIds)
-        {
-            using (_contextPool.AllocateOperationContext(out TransactionOperationContext ctx))
-            using (var tx = ctx.OpenWriteTransaction(TimeSpan.FromMinutes(1)))
-            {
-                var table = ctx.Transaction.InnerTransaction.OpenTable(BackupStatusTableSchema, BackupStatusSchema.TableName);
-                foreach (var taskId in taskIds)
-                {
-                    var backupKey = PeriodicBackupStatus.GenerateItemName(databaseName, dbId, taskId);
-                    using (Slice.From(ctx.Allocator, backupKey.ToLowerInvariant(), out Slice key))
-                    {
-                        table.DeleteByKey(key);
-                    }
-                }
-
-                tx.Commit();
-            }
-
-            return true;
-        }
-
         public static void DeleteBackupStatus(ClusterOperationContext context, string databaseName, string dbId, long taskId)
         {
             // this is called from csm, so commiting will be done outside

src/Raven.Server/Documents/PeriodicBackup/PeriodicBackupRunner.cs

@@ -631,10 +631,10 @@ private PeriodicBackupStatus GetMostUpdatedLocalBackupStatus(long taskId, Period
             return BackupUtils.ComparePeriodicBackupStatus(taskId, backupStatus, inMemoryBackupStatus);
         }
 
-        private long GetMinLastEtag(out HashSet<long> taskIdsStatusesToDelete)
+        private long GetMinimalEtagForTombstoneCleanupForBackup()
         {
             var min = long.MaxValue;
-            taskIdsStatusesToDelete = null;
+
             using (_serverStore.ContextPool.AllocateOperationContext(out TransactionOperationContext context))
             using (context.OpenReadTransaction())
             {
@@ -649,8 +649,6 @@ private long GetMinLastEtag(out HashSet<long> taskIdsStatusesToDelete)
                     }
 
                     var localStatus = BackupUtils.GetLocalBackupStatus(_serverStore, context, _database.Name, taskId);
-
-                    // if we are not the responsible node, we want to avoid gathering tombstones indefinitely since our local status will not be updated
                     var responsibleNode = BackupUtils.GetResponsibleNodeTag(_serverStore, _database.Name, taskId);
 
                     if (localStatus == null)
@@ -668,34 +666,6 @@ private long GetMinLastEtag(out HashSet<long> taskIdsStatusesToDelete)
                         }
                     }
 
-                    if (responsibleNode != null && responsibleNode != _serverStore.NodeTag && config.FullBackupFrequency != null)
-                    {
-                        var nextFullBackup = BackupUtils.GetNextBackupOccurrence(new BackupUtils.NextBackupOccurrenceParameters()
-                        {
-                            BackupFrequency = config.FullBackupFrequency,
-                            Configuration = config,
-                            LastBackupUtc = localStatus.LastFullBackupInternal ?? DateTime.MinValue
-                        });
-                        if (nextFullBackup == null)
-                        {
-                            // this is supposed to only happen if full frequency is null - shouldn't happen
-                            // let's not delete any tombstones
-                            return 0;
-                        }
-
-                        var now = DateTime.UtcNow;
-                        if (nextFullBackup.Value.ToUniversalTime() < now)
-                        {
-                            // we are overdue for a full backup, we can delete the local status to ensure the next backup will be full
-                            // this is in order to free the tombstone cleaners (for both local and compare exchange tombstones) to delete freely for this node
-
-                            taskIdsStatusesToDelete ??= new();
-                            taskIdsStatusesToDelete.Add(taskId);
-
-                            continue;
-                        }
-                    }
-
                     var etag = ChangeVectorUtils.GetEtagById(localStatus.LastDatabaseChangeVector, _database.DbBase64Id);
                     min = Math.Min(etag, min);
                 }
@@ -1057,31 +1027,7 @@ public RunningBackup OnGoingBackup(long taskId)
 
         public Dictionary<string, long> GetLastProcessedTombstonesPerCollection(ITombstoneAware.TombstoneType tombstoneType)
         {
-            var minLastEtag = GetMinLastEtag(out var taskIdsStatusesToDelete);
-
-            if (taskIdsStatusesToDelete != null && taskIdsStatusesToDelete.Count > 0)
-            {
-                try
-                {
-                    if (_serverStore.DatabaseInfoCache.BackupStatusStorage.DeleteBackupStatusesByTaskIds(_database.Name, _serverStore._env.Base64Id, taskIdsStatusesToDelete) == false)
-                        minLastEtag = 0; // deleting the local status did not succeed. we can't remove any tombstones because it is not guaranteed next backup will be full.
-                    else
-                    {
-                        if (_logger.IsInfoEnabled)
-                            _logger.Info(
-                                $"{_database.Name}: Deleted local backup statuses for the following ids [{string.Join(",", taskIdsStatusesToDelete)}], because node with db id {_serverStore._env.Base64Id} is not responsible anymore and is overdue for a full backup.");
-                    }
-                }
-                catch (Exception ex)
-                {
-                    minLastEtag = 0;
-
-                    if (_logger.IsInfoEnabled)
-                        _logger.Info(
-                        $"{_database.Name}: Could not delete the local backup statuses for the following ids [{string.Join(",", taskIdsStatusesToDelete)}]. We will not remove any tombstones.",
-                        ex);
-                }
-            }
+            var minLastEtag = GetMinimalEtagForTombstoneCleanupForBackup();
 
             if (minLastEtag == long.MaxValue)
                 return null;

src/Raven.Server/ServerWide/Commands/PeriodicBackup/UpdatePeriodicBackupStatusCommand.cs

@@ -1,6 +1,8 @@
-using Raven.Client.Documents.Operations.Backups;
+using System;
+using Raven.Client.Documents.Operations.Backups;
 using Raven.Server.Documents.PeriodicBackup;
 using Raven.Server.ServerWide.Context;
+using Raven.Server.Utils;
 using Sparrow.Json;
 using Sparrow.Json.Parsing;
 
@@ -32,11 +34,39 @@ protected override UpdatedValue GetUpdatedValue(long index, RawDatabaseRecord re
 
         public override void AfterExecute(long index, RawDatabaseRecord record, ClusterOperationContext context, ServerStore serverStore)
         {
+            // Update the local status in the relevant node
             if (PeriodicBackupStatus.NodeTag == serverStore.NodeTag)
             {
                 var status = GetUpdatedValue(index, record, context, null);
                 BackupStatusStorage.InsertBackupStatusBlittable(context, status.Value, DatabaseName, serverStore._env.Base64Id, PeriodicBackupStatus.TaskId);
             }
+
+            // Delete the local status if we are a non responsible node and we are overdue on a full backup
+            var localStatus = BackupUtils.GetLocalBackupStatus(serverStore, context, DatabaseName, PeriodicBackupStatus.TaskId);
+
+            if (localStatus == null)
+                return;
+
+            var responsibleNode = BackupUtils.GetResponsibleNodeTag(serverStore, DatabaseName, PeriodicBackupStatus.TaskId);
+            var config = record.GetPeriodicBackupConfiguration(PeriodicBackupStatus.TaskId);
+            if (responsibleNode != null && responsibleNode != serverStore.NodeTag && config.FullBackupFrequency != null)
+            {
+                var nextFullBackup = BackupUtils.GetNextBackupOccurrence(new BackupUtils.NextBackupOccurrenceParameters()
+                {
+                    BackupFrequency = config.FullBackupFrequency,
+                    Configuration = config,
+                    LastBackupUtc = localStatus.LastFullBackupInternal ?? DateTime.MinValue
+                });
+
+                var now = DateTime.UtcNow;
+                if (nextFullBackup != null && nextFullBackup.Value.ToUniversalTime() < now)
+                {
+                    // we are overdue for a full backup, we can delete the local status to ensure the next backup will be full
+                    // this is in order to free the tombstone cleaners (for both local and compare exchange tombstones) to delete freely for this node
+
+                    BackupStatusStorage.DeleteBackupStatus(context, DatabaseName, serverStore._env.Base64Id, PeriodicBackupStatus.TaskId);
+                }
+            }
         }
 
         public override void FillJson(DynamicJsonValue json)

src/Raven.Server/Utils/BackupUtils.cs

@@ -139,7 +139,7 @@ internal static BackupInfo GetBackupInfo(BackupInfoParameters parameters)
         };
     }
 
-    internal static PeriodicBackupStatus GetBackupStatusFromCluster(ServerStore serverStore, TransactionOperationContext context, string databaseName, long taskId)
+    internal static PeriodicBackupStatus GetBackupStatusFromCluster<T>(ServerStore serverStore, TransactionOperationContext<T> context, string databaseName, long taskId) where T : RavenTransaction
     {
         using var statusBlittable = GetBackupStatusFromClusterBlittable(serverStore, context, databaseName, taskId);
 
@@ -551,7 +551,7 @@ public static PeriodicBackupStatus GetLocalBackupStatus(ServerStore serverStore,
         }
     }
 
-    public static PeriodicBackupStatus GetLocalBackupStatus(ServerStore serverStore, TransactionOperationContext context, string databaseName, long taskId)
+    public static PeriodicBackupStatus GetLocalBackupStatus<T>(ServerStore serverStore, TransactionOperationContext<T> context, string databaseName, long taskId) where T : RavenTransaction
     {
         var localStatus = BackupStatusStorage.GetBackupStatus(databaseName, serverStore._env.Base64Id, taskId, context);
         if (localStatus != null)

src/Raven.Server/Web/System/BackupDatabaseHandler.cs

@@ -41,16 +41,19 @@ public async Task GetPeriodicBackup()
         public async Task GetPeriodicBackupStatus()
         {
             var name = GetQueryStringValueAndAssertIfSingleAndNotEmpty("name");
-            var fetchLocalStatus = GetBoolValueQueryString("fetchLocalStatus", required: false) ?? false;
+            var type = GetStringQueryString("type", required: false) ?? StatusType.Cluster.ToString();
 
             if (await CanAccessDatabaseAsync(name, requireAdmin: false, requireWrite: false) == false)
                 return;
 
             var taskId = GetLongQueryString("taskId", required: true);
 
+            if (StatusType.TryParse(type, ignoreCase: true, out StatusType statusType) == false)
+                throw new ArgumentException($"provided '{type}' has to be `{StatusType.Cluster.ToString()}` or '{StatusType.Local.ToString()}'");
+
             using (ServerStore.ContextPool.AllocateOperationContext(out TransactionOperationContext context))
             using (context.OpenReadTransaction())
-            using (var statusBlittable = fetchLocalStatus ? 
+            using (var statusBlittable = statusType == StatusType.Local ? 
                        BackupUtils.GetLocalBackupStatusBlittable(ServerStore, context, name, taskId.Value) :
                        BackupUtils.GetBackupStatusFromClusterBlittable(ServerStore, context, name, taskId.Value))
             await using (var writer = new AsyncBlittableJsonTextWriter(context, ResponseBodyStream()))
@@ -62,6 +65,12 @@ public async Task GetPeriodicBackupStatus()
             }
         }
 
+        enum StatusType
+        {
+            Local,
+            Cluster
+        }
+
         [RavenAction("/admin/debug/periodic-backup/timers", "GET", AuthorizationStatus.Operator)]
         public async Task GetAllPeriodicBackupsTimers()
         {