From b262497083ccef7d72b9c838d37bf13706ee4faf Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Sun, 28 Apr 2024 19:07:39 +0800 Subject: [PATCH 1/7] Check realm file can be written to before attempting further initialisation Rather than creating a "corrupt" realm file in such cases, the game will now refuse to start. This behaviour is usually what we want. In most cases a second click on the game will start it successfully (the previous instance's file handles are still doing stuff, or windows defender is being silly). Closes https://github.com/ppy/osu/issues/28018. --- osu.Game/Database/RealmAccess.cs | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/osu.Game/Database/RealmAccess.cs b/osu.Game/Database/RealmAccess.cs index 167d170c81..4bc7ec4979 100644 --- a/osu.Game/Database/RealmAccess.cs +++ b/osu.Game/Database/RealmAccess.cs @@ -300,6 +300,21 @@ namespace osu.Game.Database private Realm prepareFirstRealmAccess() { + // Before attempting to initialise realm, make sure the realm file isn't locked and has correct permissions. + // + // This is to avoid failures like: + // Realms.Exceptions.RealmException: SetEndOfFile() failed: unknown error (1224) + // + // which can occur due to file handles still being open by a previous instance. + if (storage.Exists(Filename)) + { + // If this fails we allow it to block game startup. + // It's better than any alternative we can offer. + using (var _ = storage.GetStream(Filename, FileAccess.ReadWrite)) + { + } + } + string newerVersionFilename = $"{Filename.Replace(realm_extension, string.Empty)}_newer_version{realm_extension}"; // Attempt to recover a newer database version if available. @@ -321,7 +336,7 @@ namespace osu.Game.Database { 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 a newer version database already exists, don't create another backup. We can presume that the first backup is the one we care about. if (!storage.Exists(newerVersionFilename)) createBackup(newerVersionFilename); } From a4bc5a8fc9059e2f13d736965e8cc52eff95f7ea Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 29 Apr 2024 10:35:37 +0800 Subject: [PATCH 2/7] Use helper method for backup retry attempts --- osu.Game/Database/RealmAccess.cs | 32 +++++++++----------------------- 1 file changed, 9 insertions(+), 23 deletions(-) diff --git a/osu.Game/Database/RealmAccess.cs b/osu.Game/Database/RealmAccess.cs index 4bc7ec4979..b5faa898e7 100644 --- a/osu.Game/Database/RealmAccess.cs +++ b/osu.Game/Database/RealmAccess.cs @@ -35,6 +35,7 @@ using osu.Game.Rulesets.Mods; using osu.Game.Scoring; using osu.Game.Scoring.Legacy; using osu.Game.Skinning; +using osu.Game.Utils; using osuTK.Input; using Realms; using Realms.Exceptions; @@ -1157,33 +1158,18 @@ namespace osu.Game.Database { Logger.Log($"Creating full realm database backup at {backupFilename}", LoggingTarget.Database); - int attempts = 10; - - while (true) + FileUtils.AttemptOperation(() => { - try + using (var source = storage.GetStream(Filename, mode: FileMode.Open)) { - using (var source = storage.GetStream(Filename, mode: FileMode.Open)) - { - // source may not exist. - if (source == null) - return; + // source may not exist. + if (source == null) + return; - using (var destination = storage.GetStream(backupFilename, FileAccess.Write, FileMode.CreateNew)) - source.CopyTo(destination); - } - - return; + using (var destination = storage.GetStream(backupFilename, FileAccess.Write, FileMode.CreateNew)) + source.CopyTo(destination); } - catch (IOException) - { - if (attempts-- <= 0) - throw; - - // file may be locked during use. - Thread.Sleep(500); - } - } + }, 20); } /// From 1c1ee22aa70cfa3e92786fba7a208f2af08a2e69 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 29 Apr 2024 10:36:49 +0800 Subject: [PATCH 3/7] Add retry attempts to hopefully fix windows tests runs --- osu.Game/Database/RealmAccess.cs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/osu.Game/Database/RealmAccess.cs b/osu.Game/Database/RealmAccess.cs index b5faa898e7..31ae22178f 100644 --- a/osu.Game/Database/RealmAccess.cs +++ b/osu.Game/Database/RealmAccess.cs @@ -311,9 +311,12 @@ namespace osu.Game.Database { // If this fails we allow it to block game startup. // It's better than any alternative we can offer. - using (var _ = storage.GetStream(Filename, FileAccess.ReadWrite)) + FileUtils.AttemptOperation(() => { - } + using (var _ = storage.GetStream(Filename, FileAccess.ReadWrite)) + { + } + }); } string newerVersionFilename = $"{Filename.Replace(realm_extension, string.Empty)}_newer_version{realm_extension}"; From fa3aeca09d8656ec76ae9e0401047b4f5f15646a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Mon, 29 Apr 2024 14:06:02 +0200 Subject: [PATCH 4/7] Add failing test for skins not saving on change --- .../TestSceneSkinEditorNavigation.cs | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/osu.Game.Tests/Visual/Navigation/TestSceneSkinEditorNavigation.cs b/osu.Game.Tests/Visual/Navigation/TestSceneSkinEditorNavigation.cs index 9c180d43da..38fb2846aa 100644 --- a/osu.Game.Tests/Visual/Navigation/TestSceneSkinEditorNavigation.cs +++ b/osu.Game.Tests/Visual/Navigation/TestSceneSkinEditorNavigation.cs @@ -3,6 +3,7 @@ #nullable disable +using System; using System.Linq; using NUnit.Framework; using osu.Framework.Allocation; @@ -321,6 +322,30 @@ namespace osu.Game.Tests.Visual.Navigation AddUntilStep("nested input disabled", () => ((Player)Game.ScreenStack.CurrentScreen).ChildrenOfType().All(manager => !manager.UseParentInput)); } + [Test] + public void TestSkinSavesOnChange() + { + advanceToSongSelect(); + openSkinEditor(); + + Guid editedSkinId = Guid.Empty; + AddStep("save skin id", () => editedSkinId = Game.Dependencies.Get().CurrentSkinInfo.Value.ID); + AddStep("add skinnable component", () => + { + skinEditor.ChildrenOfType().First().TriggerClick(); + }); + + AddStep("change to triangles skin", () => Game.Dependencies.Get().SetSkinFromConfiguration(SkinInfo.TRIANGLES_SKIN.ToString())); + AddUntilStep("components loaded", () => Game.ChildrenOfType().All(c => c.ComponentsLoaded)); + // sort of implicitly relies on song select not being skinnable. + // TODO: revisit if the above ever changes + AddUntilStep("skin changed", () => !skinEditor.ChildrenOfType().Any()); + + AddStep("change back to modified skin", () => Game.Dependencies.Get().SetSkinFromConfiguration(editedSkinId.ToString())); + AddUntilStep("components loaded", () => Game.ChildrenOfType().All(c => c.ComponentsLoaded)); + AddUntilStep("changes saved", () => skinEditor.ChildrenOfType().Any()); + } + private void advanceToSongSelect() { PushAndConfirm(() => songSelect = new TestPlaySongSelect()); From f78abf801c2f2d9af66bc6baa71c0a0c6ec52406 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Mon, 29 Apr 2024 14:06:23 +0200 Subject: [PATCH 5/7] Autosave edited skin on change --- osu.Game/Overlays/SkinEditor/SkinEditor.cs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/osu.Game/Overlays/SkinEditor/SkinEditor.cs b/osu.Game/Overlays/SkinEditor/SkinEditor.cs index bc929177d1..690c6b35e3 100644 --- a/osu.Game/Overlays/SkinEditor/SkinEditor.cs +++ b/osu.Game/Overlays/SkinEditor/SkinEditor.cs @@ -255,8 +255,11 @@ namespace osu.Game.Overlays.SkinEditor // schedule ensures this only happens when the skin editor is visible. // also avoid some weird endless recursion / bindable feedback loop (something to do with tracking skins across three different bindable types). // probably something which will be factored out in a future database refactor so not too concerning for now. - currentSkin.BindValueChanged(_ => + currentSkin.BindValueChanged(val => { + if (val.OldValue != null && hasBegunMutating) + save(val.OldValue); + hasBegunMutating = false; Scheduler.AddOnce(skinChanged); }, true); @@ -537,7 +540,9 @@ namespace osu.Game.Overlays.SkinEditor protected void Redo() => changeHandler?.RestoreState(1); - public void Save(bool userTriggered = true) + public void Save(bool userTriggered = true) => save(currentSkin.Value); + + private void save(Skin skin, bool userTriggered = true) { if (!hasBegunMutating) return; @@ -551,11 +556,11 @@ namespace osu.Game.Overlays.SkinEditor return; foreach (var t in targetContainers) - currentSkin.Value.UpdateDrawableTarget(t); + skin.UpdateDrawableTarget(t); // In the case the save was user triggered, always show the save message to make them feel confident. - if (skins.Save(skins.CurrentSkin.Value) || userTriggered) - onScreenDisplay?.Display(new SkinEditorToast(ToastStrings.SkinSaved, currentSkin.Value.SkinInfo.ToString() ?? "Unknown")); + if (skins.Save(skin) || userTriggered) + onScreenDisplay?.Display(new SkinEditorToast(ToastStrings.SkinSaved, skin.SkinInfo.ToString() ?? "Unknown")); } protected override bool OnHover(HoverEvent e) => true; From a8416f3572bfa143c6019322365211fb0df1837b Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 30 Apr 2024 12:39:18 +0800 Subject: [PATCH 6/7] Move `exists` check inside retry operation Might help? --- osu.Game/Database/RealmAccess.cs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/osu.Game/Database/RealmAccess.cs b/osu.Game/Database/RealmAccess.cs index 31ae22178f..465d7b15a7 100644 --- a/osu.Game/Database/RealmAccess.cs +++ b/osu.Game/Database/RealmAccess.cs @@ -307,17 +307,17 @@ namespace osu.Game.Database // Realms.Exceptions.RealmException: SetEndOfFile() failed: unknown error (1224) // // which can occur due to file handles still being open by a previous instance. - if (storage.Exists(Filename)) + // + // If this fails we allow it to block game startup. It's better than any alternative we can offer. + FileUtils.AttemptOperation(() => { - // If this fails we allow it to block game startup. - // It's better than any alternative we can offer. - FileUtils.AttemptOperation(() => + if (storage.Exists(Filename)) { using (var _ = storage.GetStream(Filename, FileAccess.ReadWrite)) { } - }); - } + } + }); string newerVersionFilename = $"{Filename.Replace(realm_extension, string.Empty)}_newer_version{realm_extension}"; From 0bfad74907be51a9755e43cad6f44db372b57f21 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 30 Apr 2024 14:09:29 +0800 Subject: [PATCH 7/7] Move realm error handling to avoid triggering in test scenarios --- osu.Game/Database/RealmAccess.cs | 38 +++++++++++++++++--------------- 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/osu.Game/Database/RealmAccess.cs b/osu.Game/Database/RealmAccess.cs index 465d7b15a7..057bbe02e6 100644 --- a/osu.Game/Database/RealmAccess.cs +++ b/osu.Game/Database/RealmAccess.cs @@ -301,24 +301,6 @@ namespace osu.Game.Database private Realm prepareFirstRealmAccess() { - // Before attempting to initialise realm, make sure the realm file isn't locked and has correct permissions. - // - // This is to avoid failures like: - // Realms.Exceptions.RealmException: SetEndOfFile() failed: unknown error (1224) - // - // which can occur due to file handles still being open by a previous instance. - // - // If this fails we allow it to block game startup. It's better than any alternative we can offer. - FileUtils.AttemptOperation(() => - { - if (storage.Exists(Filename)) - { - using (var _ = storage.GetStream(Filename, FileAccess.ReadWrite)) - { - } - } - }); - string newerVersionFilename = $"{Filename.Replace(realm_extension, string.Empty)}_newer_version{realm_extension}"; // Attempt to recover a newer database version if available. @@ -346,6 +328,26 @@ namespace osu.Game.Database } else { + // This error can occur due to file handles still being open by a previous instance. + // If this is the case, rather than assuming the realm file is corrupt, block game startup. + if (e.Message.StartsWith("SetEndOfFile() failed", StringComparison.Ordinal)) + { + // This will throw if the realm file is not available for write access after 5 seconds. + FileUtils.AttemptOperation(() => + { + if (storage.Exists(Filename)) + { + using (var _ = storage.GetStream(Filename, FileAccess.ReadWrite)) + { + } + } + }, 20); + + // If the above eventually succeeds, try and continue startup as per normal. + // This may throw again but let's allow it to, and block startup. + return getRealmInstance(); + } + 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}"); }