Merge pull request #18997 from peppy/fix-realm-backup-ctor-fail

Fix realm backup creation failing when run from `RealmAccess` constructor

osu.Game.Tests/Collections/IO/ImportCollectionsTest.cs

@@ -138,7 +138,7 @@ namespace osu.Game.Tests.Collections.IO
         {
             string firstRunName;
 
-            using (var host = new CleanRunHeadlessGameHost(bypassCleanup: true))
+            using (var host = new CleanRunHeadlessGameHost(bypassCleanupOnDispose: true))
             {
                 firstRunName = host.Name;
 

osu.Game.Tests/NonVisual/CustomDataDirectoryTest.cs

@@ -315,6 +315,26 @@ namespace osu.Game.Tests.NonVisual
             }
         }
 
+        [Test]
+        public void TestBackupCreatedOnCorruptRealm()
+        {
+            using (var host = new CustomTestHeadlessGameHost())
+            {
+                try
+                {
+                    File.WriteAllText(host.InitialStorage.GetFullPath(OsuGameBase.CLIENT_DATABASE_FILENAME, true), "i am definitely not a realm file");
+
+                    LoadOsuIntoHost(host);
+
+                    Assert.That(host.InitialStorage.GetFiles(string.Empty, "*_corrupt.realm"), Has.One.Items);
+                }
+                finally
+                {
+                    host.Exit();
+                }
+            }
+        }
+
         private static string getDefaultLocationFor(CustomTestHeadlessGameHost host)
         {
             string path = Path.Combine(TestRunHeadlessGameHost.TemporaryTestDirectory, host.Name);
@@ -347,7 +367,7 @@ namespace osu.Game.Tests.NonVisual
             public Storage InitialStorage { get; }
 
             public CustomTestHeadlessGameHost([CallerMemberName] string callingMethodName = @"")
-                : base(callingMethodName: callingMethodName)
+                : base(callingMethodName: callingMethodName, bypassCleanupOnSetup: true)
             {
                 string defaultStorageLocation = getDefaultLocationFor(this);
 

osu.Game/Database/EFToRealmMigrator.cs

@@ -132,11 +132,12 @@ namespace osu.Game.Database
             {
                 try
                 {
-                    realm.CreateBackup(Path.Combine(backup_folder, $"client.{backupSuffix}.realm"), realmBlockOperations);
+                    realm.CreateBackup(Path.Combine(backup_folder, $"client.{backupSuffix}.realm"));
                 }
                 finally
                 {
-                    // Above call will dispose of the blocking token when done.
+                    // Once the backup is created, we need to stop blocking operations so the migration can complete.
+                    realmBlockOperations.Dispose();
                     // Clean up here so we don't accidentally dispose twice.
                     realmBlockOperations = null;
                 }

osu.Game/Database/RealmAccess.cs

@@ -184,14 +184,14 @@ namespace osu.Game.Database
 
                     // If a newer version database already exists, don't backup again. We can presume that the first backup is the one we care about.
                     if (!storage.Exists(newerVersionFilename))
-                        CreateBackup(newerVersionFilename);
+                        createBackup(newerVersionFilename);
 
                     storage.Delete(Filename);
                 }
                 else
                 {
                     Logger.Error(e, "Realm startup failed with unrecoverable error; starting with a fresh database. A backup of your database has been made.");
-                    CreateBackup($"{Filename.Replace(realm_extension, string.Empty)}_{DateTimeOffset.UtcNow.ToUnixTimeSeconds()}_corrupt{realm_extension}");
+                    createBackup($"{Filename.Replace(realm_extension, string.Empty)}_{DateTimeOffset.UtcNow.ToUnixTimeSeconds()}_corrupt{realm_extension}");
                     storage.Delete(Filename);
                 }
 
@@ -236,7 +236,7 @@ namespace osu.Game.Database
             }
 
             // For extra safety, also store the temporarily-used database which we are about to replace.
-            CreateBackup($"{Filename.Replace(realm_extension, string.Empty)}_{DateTimeOffset.UtcNow.ToUnixTimeSeconds()}_newer_version_before_recovery{realm_extension}");
+            createBackup($"{Filename.Replace(realm_extension, string.Empty)}_{DateTimeOffset.UtcNow.ToUnixTimeSeconds()}_newer_version_before_recovery{realm_extension}");
 
             storage.Delete(Filename);
 
@@ -778,28 +778,37 @@ namespace osu.Game.Database
         private string? getRulesetShortNameFromLegacyID(long rulesetId) =>
             efContextFactory?.Get().RulesetInfo.FirstOrDefault(r => r.ID == rulesetId)?.ShortName;
 
-        public void CreateBackup(string backupFilename, IDisposable? blockAllOperations = null)
+        /// <summary>
+        /// Create a full realm backup.
+        /// </summary>
+        /// <param name="backupFilename">The filename for the backup.</param>
+        public void CreateBackup(string backupFilename)
         {
-            using (blockAllOperations ?? BlockAllOperations("creating backup"))
+            if (realmRetrievalLock.CurrentCount != 0)
+                throw new InvalidOperationException($"Call {nameof(BlockAllOperations)} before creating a backup.");
+
+            createBackup(backupFilename);
+        }
+
+        private void createBackup(string backupFilename)
+        {
+            Logger.Log($"Creating full realm database backup at {backupFilename}", LoggingTarget.Database);
+
+            int attempts = 10;
+
+            while (attempts-- > 0)
             {
-                Logger.Log($"Creating full realm database backup at {backupFilename}", LoggingTarget.Database);
-
-                int attempts = 10;
-
-                while (attempts-- > 0)
+                try
                 {
-                    try
-                    {
-                        using (var source = storage.GetStream(Filename, mode: FileMode.Open))
-                        using (var destination = storage.GetStream(backupFilename, FileAccess.Write, FileMode.CreateNew))
-                            source.CopyTo(destination);
-                        return;
-                    }
-                    catch (IOException)
-                    {
-                        // file may be locked during use.
-                        Thread.Sleep(500);
-                    }
+                    using (var source = storage.GetStream(Filename, mode: FileMode.Open))
+                    using (var destination = storage.GetStream(backupFilename, FileAccess.Write, FileMode.CreateNew))
+                        source.CopyTo(destination);
+                    return;
+                }
+                catch (IOException)
+                {
+                    // file may be locked during use.
+                    Thread.Sleep(500);
                 }
             }
         }

osu.Game/Tests/CleanRunHeadlessGameHost.cs

@@ -15,30 +15,38 @@ namespace osu.Game.Tests
     /// </summary>
     public class CleanRunHeadlessGameHost : TestRunHeadlessGameHost
     {
+        private readonly bool bypassCleanupOnSetup;
+
         /// <summary>
         /// Create a new instance.
         /// </summary>
         /// <param name="bindIPC">Whether to bind IPC channels.</param>
         /// <param name="realtime">Whether the host should be forced to run in realtime, rather than accelerated test time.</param>
-        /// <param name="bypassCleanup">Whether to bypass directory cleanup on host disposal. Should be used only if a subsequent test relies on the files still existing.</param>
+        /// <param name="bypassCleanupOnSetup">Whether to bypass directory cleanup on <see cref="SetupForRun"/>.</param>
+        /// <param name="bypassCleanupOnDispose">Whether to bypass directory cleanup on host disposal. Should be used only if a subsequent test relies on the files still existing.</param>
         /// <param name="callingMethodName">The name of the calling method, used for test file isolation and clean-up.</param>
-        public CleanRunHeadlessGameHost(bool bindIPC = false, bool realtime = true, bool bypassCleanup = false, [CallerMemberName] string callingMethodName = @"")
+        public CleanRunHeadlessGameHost(bool bindIPC = false, bool realtime = true, bool bypassCleanupOnSetup = false, bool bypassCleanupOnDispose = false,
+                                        [CallerMemberName] string callingMethodName = @"")
             : base($"{callingMethodName}-{Guid.NewGuid()}", new HostOptions
             {
                 BindIPC = bindIPC,
-            }, bypassCleanup: bypassCleanup, realtime: realtime)
+            }, bypassCleanup: bypassCleanupOnDispose, realtime: realtime)
         {
+            this.bypassCleanupOnSetup = bypassCleanupOnSetup;
         }
 
         protected override void SetupForRun()
         {
-            try
+            if (!bypassCleanupOnSetup)
             {
-                Storage.DeleteDirectory(string.Empty);
-            }
-            catch
-            {
-                // May fail if a logging target has already been set via OsuStorage.ChangeTargetStorage.
+                try
+                {
+                    Storage.DeleteDirectory(string.Empty);
+                }
+                catch
+                {
+                    // May fail if a logging target has already been set via OsuStorage.ChangeTargetStorage.
+                }
             }
 
             // base call needs to be run *after* storage is emptied, as it updates the (static) logger's storage and may start writing