From 79694897bef29810846bc156455880b19658723f Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 30 May 2023 12:58:22 +0900 Subject: [PATCH 1/4] Ensure a potential exception from `cleanupPendingDeletions` doesn't mark realm corrupt The whole restructure here is to move the nested call out of the `try-catch`. I noticed this while looking at a corrupt database issue a user reported (https://github.com/ppy/osu/discussions/23694). It's not the first time we've seen a corrupt database error where the "corrupt" version works just fine on a second attempt. Maybe this isn't the issue and it's just a transitive file access violation but it definitely feels like this should be fixed regardless. --- osu.Game/Database/RealmAccess.cs | 79 +++++++++++++++++--------------- 1 file changed, 41 insertions(+), 38 deletions(-) diff --git a/osu.Game/Database/RealmAccess.cs b/osu.Game/Database/RealmAccess.cs index 831e328439..55b5e0114d 100644 --- a/osu.Game/Database/RealmAccess.cs +++ b/osu.Game/Database/RealmAccess.cs @@ -179,43 +179,9 @@ namespace osu.Game.Database applyFilenameSchemaSuffix(ref Filename); #endif - string newerVersionFilename = $"{Filename.Replace(realm_extension, string.Empty)}_newer_version{realm_extension}"; - - // Attempt to recover a newer database version if available. - if (storage.Exists(newerVersionFilename)) - { - Logger.Log(@"A newer realm database has been found, attempting recovery...", LoggingTarget.Database); - attemptRecoverFromFile(newerVersionFilename); - } - - try - { + using (var realm = prepareFirstRealmAccess()) // This method triggers the first `getRealmInstance` call, which will implicitly run realm migrations and bring the schema up-to-date. - cleanupPendingDeletions(); - } - catch (Exception e) - { - // See https://github.com/realm/realm-core/blob/master/src%2Frealm%2Fobject-store%2Fobject_store.cpp#L1016-L1022 - // This is the best way we can detect a schema version downgrade. - if (e.Message.StartsWith(@"Provided schema version", StringComparison.Ordinal)) - { - Logger.Error(e, "Your local database is too new to work with this version of osu!. Please close osu! and install the latest release to recover your data."); - - // 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); - - 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}"); - storage.Delete(Filename); - } - - cleanupPendingDeletions(); - } + cleanupPendingDeletions(realm); } /// @@ -312,9 +278,46 @@ namespace osu.Game.Database Logger.Log(@"Recovery complete!", LoggingTarget.Database); } - private void cleanupPendingDeletions() + private Realm prepareFirstRealmAccess() + { + string newerVersionFilename = $"{Filename.Replace(realm_extension, string.Empty)}_newer_version{realm_extension}"; + + // Attempt to recover a newer database version if available. + if (storage.Exists(newerVersionFilename)) + { + Logger.Log(@"A newer realm database has been found, attempting recovery...", LoggingTarget.Database); + attemptRecoverFromFile(newerVersionFilename); + } + + try + { + return getRealmInstance(); + } + catch (Exception e) + { + // See https://github.com/realm/realm-core/blob/master/src%2Frealm%2Fobject-store%2Fobject_store.cpp#L1016-L1022 + // This is the best way we can detect a schema version downgrade. + if (e.Message.StartsWith(@"Provided schema version", StringComparison.Ordinal)) + { + Logger.Error(e, "Your local database is too new to work with this version of osu!. Please close osu! and install the latest release to recover your data."); + + // 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); + } + 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}"); + } + + storage.Delete(Filename); + return getRealmInstance(); + } + } + + private void cleanupPendingDeletions(Realm realm) { - using (var realm = getRealmInstance()) using (var transaction = realm.BeginWrite()) { var pendingDeleteScores = realm.All().Where(s => s.DeletePending); From a0be52626639b8cbe2e6db7280fc3c106bdb9cd6 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 30 May 2023 13:04:32 +0900 Subject: [PATCH 2/4] Adjust realm backup procedure to hard fail if running out of attempts Previously, if the backup procedure failed, startup would continue and the user's realm database may be deleted. I think in such a fail case I'd rather the game didn't startup so the user gets in touch (or reads the log files themselves) rather than potentially losing data. --- osu.Game/Database/RealmAccess.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/osu.Game/Database/RealmAccess.cs b/osu.Game/Database/RealmAccess.cs index 55b5e0114d..2e66ad8b02 100644 --- a/osu.Game/Database/RealmAccess.cs +++ b/osu.Game/Database/RealmAccess.cs @@ -912,7 +912,7 @@ namespace osu.Game.Database int attempts = 10; - while (attempts-- > 0) + while (true) { try { @@ -930,6 +930,9 @@ namespace osu.Game.Database } catch (IOException) { + if (attempts-- <= 0) + throw; + // file may be locked during use. Thread.Sleep(500); } From 2e81cae201e77229445b97923aaa62be739ac47a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Tue, 30 May 2023 23:17:07 +0200 Subject: [PATCH 3/4] Move comment to more correct place --- osu.Game/Database/RealmAccess.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Database/RealmAccess.cs b/osu.Game/Database/RealmAccess.cs index 2e66ad8b02..1aef8f1c67 100644 --- a/osu.Game/Database/RealmAccess.cs +++ b/osu.Game/Database/RealmAccess.cs @@ -179,8 +179,8 @@ namespace osu.Game.Database applyFilenameSchemaSuffix(ref Filename); #endif + // `prepareFirstRealmAccess()` triggers the first `getRealmInstance` call, which will implicitly run realm migrations and bring the schema up-to-date. using (var realm = prepareFirstRealmAccess()) - // This method triggers the first `getRealmInstance` call, which will implicitly run realm migrations and bring the schema up-to-date. cleanupPendingDeletions(realm); } From 18eb15bfa5af004fc2e35f2ec6c017b0cf4a44f4 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 31 May 2023 19:39:43 +0900 Subject: [PATCH 4/4] Gracefully handle failures in cleaning up pending file deletions --- osu.Game/Database/RealmAccess.cs | 71 ++++++++++++++++++-------------- 1 file changed, 39 insertions(+), 32 deletions(-) diff --git a/osu.Game/Database/RealmAccess.cs b/osu.Game/Database/RealmAccess.cs index 1aef8f1c67..94108531e8 100644 --- a/osu.Game/Database/RealmAccess.cs +++ b/osu.Game/Database/RealmAccess.cs @@ -318,46 +318,53 @@ namespace osu.Game.Database private void cleanupPendingDeletions(Realm realm) { - using (var transaction = realm.BeginWrite()) + try { - var pendingDeleteScores = realm.All().Where(s => s.DeletePending); - - foreach (var score in pendingDeleteScores) - realm.Remove(score); - - var pendingDeleteSets = realm.All().Where(s => s.DeletePending); - - foreach (var beatmapSet in pendingDeleteSets) + using (var transaction = realm.BeginWrite()) { - foreach (var beatmap in beatmapSet.Beatmaps) - { - // Cascade delete related scores, else they will have a null beatmap against the model's spec. - foreach (var score in beatmap.Scores) - realm.Remove(score); + var pendingDeleteScores = realm.All().Where(s => s.DeletePending); - realm.Remove(beatmap.Metadata); - realm.Remove(beatmap); + foreach (var score in pendingDeleteScores) + realm.Remove(score); + + var pendingDeleteSets = realm.All().Where(s => s.DeletePending); + + foreach (var beatmapSet in pendingDeleteSets) + { + foreach (var beatmap in beatmapSet.Beatmaps) + { + // Cascade delete related scores, else they will have a null beatmap against the model's spec. + foreach (var score in beatmap.Scores) + realm.Remove(score); + + realm.Remove(beatmap.Metadata); + realm.Remove(beatmap); + } + + realm.Remove(beatmapSet); } - realm.Remove(beatmapSet); + var pendingDeleteSkins = realm.All().Where(s => s.DeletePending); + + foreach (var s in pendingDeleteSkins) + realm.Remove(s); + + var pendingDeletePresets = realm.All().Where(s => s.DeletePending); + + foreach (var s in pendingDeletePresets) + realm.Remove(s); + + transaction.Commit(); } - var pendingDeleteSkins = realm.All().Where(s => s.DeletePending); - - foreach (var s in pendingDeleteSkins) - realm.Remove(s); - - var pendingDeletePresets = realm.All().Where(s => s.DeletePending); - - foreach (var s in pendingDeletePresets) - realm.Remove(s); - - transaction.Commit(); + // clean up files after dropping any pending deletions. + // in the future we may want to only do this when the game is idle, rather than on every startup. + new RealmFileStore(this, storage).Cleanup(); + } + catch (Exception e) + { + Logger.Error(e, "Failed to clean up unused files. This is not critical but please report if it happens regularly."); } - - // clean up files after dropping any pending deletions. - // in the future we may want to only do this when the game is idle, rather than on every startup. - new RealmFileStore(this, storage).Cleanup(); } ///