RavenDB-22849 - address PR comments and avoid race in compare exchang…

…e tombstone cleaning

src/Raven.Client/Documents/Operations/Backups/PeriodicBackupStatus.cs

@@ -107,14 +107,6 @@ internal static string GenerateItemName(string databaseName, string base64DbId,
         {
             return $"{GenerateItemName(databaseName, taskId)}/{base64DbId}";
         }
-
-        public override string ToString()
-        {
-            using (var ctx = JsonOperationContext.ShortTermSingleUse())
-            {
-                return ctx.ReadObject(ToJson(), "backup-status").ToString();
-            }
-        }
     }
 
     public class Error

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

@@ -27,8 +27,6 @@ public class BackupStatusStorage
 
         private static readonly Slice BackupStatusSlice;
 
-        public SystemTime Time = new SystemTime();
-
         static BackupStatusStorage()
         {
             using (StorageEnvironment.GetStaticContext(out var ctx))
@@ -123,36 +121,20 @@ public static unsafe void InsertBackupStatusBlittable<T>(TransactionOperationCon
 
         public bool DeleteBackupStatusesByTaskIds(string databaseName, string dbId, HashSet<long> taskIds)
         {
-            try
+            using (_contextPool.AllocateOperationContext(out TransactionOperationContext ctx))
+            using (var tx = ctx.OpenWriteTransaction(TimeSpan.FromMinutes(1)))
             {
-                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)
                 {
-                    foreach (var taskId in taskIds)
+                    var backupKey = PeriodicBackupStatus.GenerateItemName(databaseName, dbId, taskId);
+                    using (Slice.From(ctx.Allocator, backupKey.ToLowerInvariant(), out Slice key))
                     {
-                        var backupKey = PeriodicBackupStatus.GenerateItemName(databaseName, dbId, taskId);
-                        using (Slice.From(ctx.Allocator, backupKey.ToLowerInvariant(), out Slice key))
-                        {
-                            var table = ctx.Transaction.InnerTransaction.OpenTable(BackupStatusTableSchema, BackupStatusSchema.TableName);
-                            table.DeleteByKey(key);
-                        }
+                        table.DeleteByKey(key);
                     }
-
-                    tx.Commit();
                 }
 
-                if (_logger.IsInfoEnabled)
-                    _logger.Info(
-                        $"{databaseName}: Deleted local backup statuses for the following ids [{string.Join(",", taskIds)}], because node with db id {dbId} is not responsible anymore and is overdue for a full backup.");
-            }
-            catch (Exception ex)
-            {
-                if (_logger.IsInfoEnabled)
-                    _logger.Info(
-                        $"{databaseName}: Could not delete the local backup statuses for the following ids [{string.Join(",", taskIds)}]. We will not remove any tombstones.",
-                        ex);
-
-                return false;
+                tx.Commit();
             }
 
             return true;

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

@@ -1061,8 +1061,26 @@ public Dictionary<string, long> GetLastProcessedTombstonesPerCollection(ITombsto
 
             if (taskIdsStatusesToDelete != null && taskIdsStatusesToDelete.Count > 0)
             {
-                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.
+                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);
+                }
             }
 
             if (minLastEtag == long.MaxValue)

src/Raven.Server/ServerWide/Maintenance/ClusterObserver.cs

@@ -98,7 +98,7 @@ public ClusterObserver(
 
         public bool Suspended = false; // don't really care about concurrency here
         private readonly BlockingCollection<ClusterObserverLogEntry> _decisionsLog = new BlockingCollection<ClusterObserverLogEntry>();
-        internal long _iteration;
+        private long _iteration;
         private readonly long _term;
         private readonly long _moveToRehabTimeMs;
         private readonly long _maxChangeVectorDistance;
@@ -556,7 +556,7 @@ internal CleanCompareExchangeTombstonesCommand GetCompareExchangeTombstonesToCle
                 return null;
             }
 
-            cleanupState = GetMaxCompareExchangeTombstonesEtagToDelete(state, out long maxEtag);
+            cleanupState = GetMaxCompareExchangeTombstonesEtagToDelete(context, databaseName, state, out long maxEtag);
 
             if(_server.ForTestingPurposes?.AfterCompareExchangeTombstonesResult != null)
                 _server.ForTestingPurposes?.AfterCompareExchangeTombstonesResult.Invoke(cleanupState);
@@ -574,7 +574,7 @@ public enum CompareExchangeTombstonesCleanupState
             NoMoreTombstones
         }
 
-        private CompareExchangeTombstonesCleanupState GetMaxCompareExchangeTombstonesEtagToDelete(DatabaseObservationState state, out long maxEtag)
+        private CompareExchangeTombstonesCleanupState GetMaxCompareExchangeTombstonesEtagToDelete(TransactionOperationContext context, string databaseName, DatabaseObservationState state, out long maxEtag)
         {
             maxEtag = -1;
             long minClusterWideTransactionIndex = -1;
@@ -611,7 +611,16 @@ private CompareExchangeTombstonesCleanupState GetMaxCompareExchangeTombstonesEta
                 foreach (var (taskId, status) in report.BackupStatuses)
                 {
                     if (status == null)
+                    {
+                        var clusterStatus = BackupUtils.GetBackupStatusFromClusterBlittable(_server, context, databaseName, taskId);
+                        if (clusterStatus == null)
+                        {
+                            // existing backup hasn't run yet for the first time, we don't want to delete anything until we have a first status
+                            return 0;
+                        }
+
                         continue;
+                    }
 
                     var lastFullBackupInternal = status.LastFullBackupInternal;
                     if (lastFullBackupInternal == null)