Merge pull request #16844 from peppy/migration-delete-fail-gracefully

Allow game folder migration to fail gracefully when cleanup cannot completely succeed

osu.Game.Tests/Visual/Settings/TestSceneMigrationScreens.cs

@@ -3,32 +3,69 @@
 
 using System.IO;
 using System.Threading;
+using NUnit.Framework;
+using osu.Framework.Allocation;
+using osu.Framework.Graphics;
 using osu.Framework.Screens;
+using osu.Game.Overlays;
 using osu.Game.Overlays.Settings.Sections.Maintenance;
 
 namespace osu.Game.Tests.Visual.Settings
 {
     public class TestSceneMigrationScreens : ScreenTestScene
     {
+        [Cached]
+        private readonly NotificationOverlay notifications;
+
         public TestSceneMigrationScreens()
         {
-            AddStep("Push screen", () => Stack.Push(new TestMigrationSelectScreen()));
+            Children = new Drawable[]
+            {
+                notifications = new NotificationOverlay
+                {
+                    Anchor = Anchor.TopRight,
+                    Origin = Anchor.TopRight,
+                }
+            };
+        }
+
+        [Test]
+        public void TestDeleteSuccess()
+        {
+            AddStep("Push screen", () => Stack.Push(new TestMigrationSelectScreen(true)));
+        }
+
+        [Test]
+        public void TestDeleteFails()
+        {
+            AddStep("Push screen", () => Stack.Push(new TestMigrationSelectScreen(false)));
         }
 
         private class TestMigrationSelectScreen : MigrationSelectScreen
         {
-            protected override void BeginMigration(DirectoryInfo target) => this.Push(new TestMigrationRunScreen());
+            private readonly bool deleteSuccess;
+
+            public TestMigrationSelectScreen(bool deleteSuccess)
+            {
+                this.deleteSuccess = deleteSuccess;
+            }
+
+            protected override void BeginMigration(DirectoryInfo target) => this.Push(new TestMigrationRunScreen(deleteSuccess));
 
             private class TestMigrationRunScreen : MigrationRunScreen
             {
-                protected override void PerformMigration()
+                private readonly bool success;
-                {
-                    Thread.Sleep(3000);
-                }
 
-                public TestMigrationRunScreen()
+                public TestMigrationRunScreen(bool success)
                     : base(null)
                 {
+                    this.success = success;
+                }
+
+                protected override bool PerformMigration()
+                {
+                    Thread.Sleep(3000);
+                    return success;
                 }
             }
         }

osu.Game.Tournament/IO/TournamentStorage.cs

@@ -70,7 +70,7 @@ namespace osu.Game.Tournament.IO
 
         public IEnumerable<string> ListTournaments() => AllTournaments.GetDirectories(string.Empty);
 
-        public override void Migrate(Storage newStorage)
+        public override bool Migrate(Storage newStorage)
         {
             // this migration only happens once on moving to the per-tournament storage system.
             // listed files are those known at that point in time.
@@ -94,6 +94,8 @@ namespace osu.Game.Tournament.IO
             ChangeTargetStorage(newStorage);
             storageConfig.SetValue(StorageConfig.CurrentTournament, default_tournament);
             storageConfig.Save();
+
+            return true;
         }
 
         private void moveFileIfExists(string file, DirectoryInfo destination)

osu.Game/IO/MigratableStorage.cs

@@ -33,7 +33,8 @@ namespace osu.Game.IO
         /// A general purpose migration method to move the storage to a different location.
         /// <param name="newStorage">The target storage of the migration.</param>
         /// </summary>
-        public virtual void Migrate(Storage newStorage)
+        /// <returns>Whether cleanup could complete.</returns>
+        public virtual bool Migrate(Storage newStorage)
         {
             var source = new DirectoryInfo(GetFullPath("."));
             var destination = new DirectoryInfo(newStorage.GetFullPath("."));
@@ -57,17 +58,20 @@ namespace osu.Game.IO
 
             CopyRecursive(source, destination);
             ChangeTargetStorage(newStorage);
-            DeleteRecursive(source);
+
+            return DeleteRecursive(source);
         }
 
-        protected void DeleteRecursive(DirectoryInfo target, bool topLevelExcludes = true)
+        protected bool DeleteRecursive(DirectoryInfo target, bool topLevelExcludes = true)
         {
+            bool allFilesDeleted = true;
+
             foreach (System.IO.FileInfo fi in target.GetFiles())
             {
                 if (topLevelExcludes && IgnoreFiles.Contains(fi.Name))
                     continue;
 
-                AttemptOperation(() => fi.Delete());
+                allFilesDeleted &= AttemptOperation(() => fi.Delete(), throwOnFailure: false);
             }
 
             foreach (DirectoryInfo dir in target.GetDirectories())
@@ -75,11 +79,13 @@ namespace osu.Game.IO
                 if (topLevelExcludes && IgnoreDirectories.Contains(dir.Name))
                     continue;
 
-                AttemptOperation(() => dir.Delete(true));
+                allFilesDeleted &= AttemptOperation(() => dir.Delete(true), throwOnFailure: false);
             }
 
             if (target.GetFiles().Length == 0 && target.GetDirectories().Length == 0)
-                AttemptOperation(target.Delete);
+                allFilesDeleted &= AttemptOperation(target.Delete, throwOnFailure: false);
+
+            return allFilesDeleted;
         }
 
         protected void CopyRecursive(DirectoryInfo source, DirectoryInfo destination, bool topLevelExcludes = true)
@@ -110,19 +116,25 @@ namespace osu.Game.IO
         /// </summary>
         /// <param name="action">The action to perform.</param>
         /// <param name="attempts">The number of attempts (250ms wait between each).</param>
-        protected static void AttemptOperation(Action action, int attempts = 10)
+        /// <param name="throwOnFailure">Whether to throw an exception on failure. If <c>false</c>, will silently fail.</param>
+        protected static bool AttemptOperation(Action action, int attempts = 10, bool throwOnFailure = true)
         {
             while (true)
             {
                 try
                 {
                     action();
-                    return;
+                    return true;
                 }
                 catch (Exception)
                 {
                     if (attempts-- == 0)
-                        throw;
+                    {
+                        if (throwOnFailure)
+                            throw;
+
+                        return false;
+                    }
                 }
 
                 Thread.Sleep(250);

osu.Game/IO/OsuStorage.cs

@@ -113,11 +113,14 @@ namespace osu.Game.IO
             }
         }
 
-        public override void Migrate(Storage newStorage)
+        public override bool Migrate(Storage newStorage)
         {
-            base.Migrate(newStorage);
+            bool cleanupSucceeded = base.Migrate(newStorage);
+
             storageConfig.SetValue(StorageConfig.FullPath, newStorage.GetFullPath("."));
             storageConfig.Save();
+
+            return cleanupSucceeded;
         }
     }
 

osu.Game/OsuGameBase.cs

@@ -413,7 +413,7 @@ namespace osu.Game
                 Scheduler.AddDelayed(GracefullyExit, 2000);
         }
 
-        public void Migrate(string path)
+        public bool Migrate(string path)
         {
             Logger.Log($@"Migrating osu! data from ""{Storage.GetFullPath(string.Empty)}"" to ""{path}""...");
 
@@ -432,14 +432,15 @@ namespace osu.Game
 
                 readyToRun.Wait();
 
-                (Storage as OsuStorage)?.Migrate(Host.GetStorage(path));
+                bool? cleanupSucceded = (Storage as OsuStorage)?.Migrate(Host.GetStorage(path));
+
+                Logger.Log(@"Migration complete!");
+                return cleanupSucceded != false;
             }
             finally
             {
                 realmBlocker?.Dispose();
             }
-
-            Logger.Log(@"Migration complete!");
         }
 
         protected override UserInputManager CreateUserInputManager() => new OsuUserInputManager();

osu.Game/Overlays/Settings/Sections/Maintenance/MigrationRunScreen.cs

@@ -4,13 +4,16 @@
 using System.IO;
 using System.Threading.Tasks;
 using osu.Framework.Allocation;
+using osu.Framework.Extensions;
 using osu.Framework.Graphics;
 using osu.Framework.Graphics.Containers;
 using osu.Framework.Logging;
+using osu.Framework.Platform;
 using osu.Framework.Screens;
 using osu.Game.Graphics;
 using osu.Game.Graphics.Sprites;
 using osu.Game.Graphics.UserInterface;
+using osu.Game.Overlays.Notifications;
 using osu.Game.Screens;
 using osuTK;
 
@@ -23,6 +26,15 @@ namespace osu.Game.Overlays.Settings.Sections.Maintenance
         [Resolved(canBeNull: true)]
         private OsuGame game { get; set; }
 
+        [Resolved]
+        private NotificationOverlay notifications { get; set; }
+
+        [Resolved]
+        private Storage storage { get; set; }
+
+        [Resolved]
+        private GameHost host { get; set; }
+
         public override bool AllowBackButton => false;
 
         public override bool AllowExternalScreenChange => false;
@@ -84,17 +96,33 @@ namespace osu.Game.Overlays.Settings.Sections.Maintenance
 
             Beatmap.Value = Beatmap.Default;
 
+            var originalStorage = new NativeStorage(storage.GetFullPath(string.Empty), host);
+
             migrationTask = Task.Run(PerformMigration)
-                                .ContinueWith(t =>
+                                .ContinueWith(task =>
                                 {
-                                    if (t.IsFaulted)
+                                    if (task.IsFaulted)
-                                        Logger.Error(t.Exception, $"Error during migration: {t.Exception?.Message}");
+                                    {
+                                        Logger.Error(task.Exception, $"Error during migration: {task.Exception?.Message}");
+                                    }
+                                    else if (!task.GetResultSafely())
+                                    {
+                                        notifications.Post(new SimpleNotification
+                                        {
+                                            Text = "Some files couldn't be cleaned up during migration. Clicking this notification will open the folder so you can manually clean things up.",
+                                            Activated = () =>
+                                            {
+                                                originalStorage.PresentExternally();
+                                                return true;
+                                            }
+                                        });
+                                    }
 
                                     Schedule(this.Exit);
                                 });
         }
 
-        protected virtual void PerformMigration() => game?.Migrate(destination.FullName);
+        protected virtual bool PerformMigration() => game?.Migrate(destination.FullName) != false;
 
         public override void OnEntering(IScreen last)
         {