Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce chance of destructive save failure #779

Open
rdebath opened this issue Nov 18, 2023 · 0 comments
Open

Reduce chance of destructive save failure #779

rdebath opened this issue Nov 18, 2023 · 0 comments

Comments

@rdebath
Copy link
Contributor

rdebath commented Nov 18, 2023

Forgot to post this...

If the system runs out of disk space or it crashes during a save the saved level will currently be damaged or even lost.

This change uses Move or Replace to ensure that the file is completely written to the directory before it is positioned as the current savefile. If it runs out of space it should error when writing a backup map file so the primary map is still unchanged. The secondary files will get written when there is (comparatively) lots of free space and so have very little chance of failing. With the last write being a fast copy from the primary save file to the .backup file which won't cause serious damage if it fails.

commit c1144bbe05fbaeb6107fa021a6a35cb92f047bff
Author: Robert de Bath <[email protected]>
Date:   Thu Mar 30 18:22:33 2023 +0100

    Reduce chance of destructive save failure

diff --git a/MCGalaxy/Levels/Level.cs b/MCGalaxy/Levels/Level.cs
index f811c9ef2..01f97f137 100644
--- a/MCGalaxy/Levels/Level.cs
+++ b/MCGalaxy/Levels/Level.cs
@@ -236,16 +236,57 @@ namespace MCGalaxy
         
         void SaveCore(string path) {
             if (blocks == null) return;
-            if (File.Exists(path)) {
-                string prevPath = Paths.PrevMapFile(name);
-                if (File.Exists(prevPath)) File.Delete(prevPath);
-                File.Copy(path, prevPath, true);
-                File.Delete(path);
-            }
-            
+
+            // The aim here is that no issue upto and including a system crash
+            // will leave this in an inconsistent state. Because of the layout
+            // using multiple files this is not completely possible, however,
+            // common issues such as out of space and a killed process can be
+            // mostly accomodated.
+
+            // (1)
+            // Start long job to overwrite the backup file (out of space should happen here)
+            // On out of space or crash the primary level files will be untouched.
+
+            // (2)
+            // All three files usable and primary files are untouched so it's safe to remove old prev.
+
+            // (3)
+            // File.Replace move the backup to the primary and creates prev.
+            // This should not need any free space, but the previous delete freed up sufficient anyway.
+            // This operation is the first to touch the primary level file.
+            // EXCEPT, if the primary file doesn't exist it chokes! So use
+            // File.Move in that case (This would be a race).
+
+            // Beware: on mono this is two atomic renames. (or link and rename ??)
+
+            // Windows is supposed to do the double rename as an atomic op.
+            // However, primary documentation is incomplete and while extra commentary
+            // does confirm that the operation should be atomic this only applies
+            // for local NTFS filesystems with all files on the same filesystem.
+
+            // Neverthless, this will be a fast operation; so there is that.
+
+            // (4)
+            // Level Properties etc.
+            // After the above rename the properties are inconsistent with the
+            // level file. We should write them out as soon as possible.
+            // This is before creation of the new .backup file so there should be more
+            // than enough free space.
+
+            // (5)
+            // Create a backup file, out of space here is mostly ok, but should error anyway.
+            // On error the next call of this function will likely fail too as the space occupied
+            // by this file is what's used to initially write out a new save.
+
             IMapExporter.Formats[0].Write(path + ".backup", this);
-            File.Copy(path + ".backup", path);
+            string prevPath = Paths.PrevMapFile(name);
+            if (File.Exists(prevPath)) File.Delete(prevPath);
+            if (!File.Exists(path))
+                File.Move(path + ".backup", path);
+            else
+                File.Replace(path + ".backup", path, prevPath);
             SaveSettings();
+            File.Copy(path, path + ".backup");
 
             Logger.Log(LogType.SystemActivity, "SAVED: Level \"{0}\". ({1}/{2}/{3})",
                        name, players.Count, PlayerInfo.Online.Count, Server.Config.MaxPlayers);
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant