From 64edc6888d84fc5a6b7e179a8a968c685ef5b19e Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 4 Jul 2022 16:45:21 +0900 Subject: [PATCH 1/2] Add test coverage of corrupt realm creating backup --- .../Collections/IO/ImportCollectionsTest.cs | 2 +- .../NonVisual/CustomDataDirectoryTest.cs | 22 +++++++++++++++- osu.Game/Tests/CleanRunHeadlessGameHost.cs | 26 ++++++++++++------- 3 files changed, 39 insertions(+), 11 deletions(-) diff --git a/osu.Game.Tests/Collections/IO/ImportCollectionsTest.cs b/osu.Game.Tests/Collections/IO/ImportCollectionsTest.cs index c31aafa67f..9a8f29647d 100644 --- a/osu.Game.Tests/Collections/IO/ImportCollectionsTest.cs +++ b/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; diff --git a/osu.Game.Tests/NonVisual/CustomDataDirectoryTest.cs b/osu.Game.Tests/NonVisual/CustomDataDirectoryTest.cs index 216bd0fd3c..216db2121c 100644 --- a/osu.Game.Tests/NonVisual/CustomDataDirectoryTest.cs +++ b/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); diff --git a/osu.Game/Tests/CleanRunHeadlessGameHost.cs b/osu.Game/Tests/CleanRunHeadlessGameHost.cs index d36168d3dd..02d67de5a5 100644 --- a/osu.Game/Tests/CleanRunHeadlessGameHost.cs +++ b/osu.Game/Tests/CleanRunHeadlessGameHost.cs @@ -15,30 +15,38 @@ namespace osu.Game.Tests /// public class CleanRunHeadlessGameHost : TestRunHeadlessGameHost { + private readonly bool bypassCleanupOnSetup; + /// /// Create a new instance. /// /// Whether to bind IPC channels. /// Whether the host should be forced to run in realtime, rather than accelerated test time. - /// Whether to bypass directory cleanup on host disposal. Should be used only if a subsequent test relies on the files still existing. + /// Whether to bypass directory cleanup on . + /// Whether to bypass directory cleanup on host disposal. Should be used only if a subsequent test relies on the files still existing. /// The name of the calling method, used for test file isolation and clean-up. - 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 From 506409a9c471246dc37fee8ad52fd2634c1d6d19 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 4 Jul 2022 16:27:49 +0900 Subject: [PATCH 2/2] Fix realm backup creation failing when run from `RealmAccess` constructor At the point of construction, we are not on the update thread, but it doesn't really matter at this point because there's no other usages. --- osu.Game/Database/EFToRealmMigrator.cs | 5 ++- osu.Game/Database/RealmAccess.cs | 53 +++++++++++++++----------- 2 files changed, 34 insertions(+), 24 deletions(-) diff --git a/osu.Game/Database/EFToRealmMigrator.cs b/osu.Game/Database/EFToRealmMigrator.cs index 8f2ff600d8..3b5424b3fb 100644 --- a/osu.Game/Database/EFToRealmMigrator.cs +++ b/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; } diff --git a/osu.Game/Database/RealmAccess.cs b/osu.Game/Database/RealmAccess.cs index ed56049064..00218e4fe3 100644 --- a/osu.Game/Database/RealmAccess.cs +++ b/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) + /// + /// Create a full realm backup. + /// + /// The filename for the backup. + 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); } } }