Handle exceptions while writing a movie file, and use a temporary file until writing has completed. Addresses #4258 and prevents loss of data in the event of an exception mid-write.

Author: SuuperW

.global.editorconfig.ini

@@ -120,6 +120,15 @@ dotnet_diagnostic.CA2229.severity = silent
 # Opt in to preview features before using them
 dotnet_diagnostic.CA2252.severity = silent # CSharpDetectPreviewFeatureAnalyzer very slow
 
+## Nullable rules; generics are a bit wonky and I have no idea why we're allowed to configure these to be not errors or why they aren't errors by default.
+
+# Nullability of reference types in value doesn't match target type.
+dotnet_diagnostic.CS8619.severity = error
+# Make Foo<string?> an error for class Foo<T> where T : class. Use `where T : class?` if Foo<string?> should be allowed.
+dotnet_diagnostic.CS8634.severity = error
+# Nullability of type argument doesn't match 'notnull' constraint.
+dotnet_diagnostic.CS8714.severity = error
+
 ## .NET DocumentationAnalyzers style rules
 
 # Place text in paragraphs

src/BizHawk.Client.Common/Api/Classes/MovieApi.cs

@@ -53,6 +53,7 @@ public string GetInputAsMnemonic(int frame)
 			return Bk2LogEntryGenerator.GenerateLogEntry(_movieSession.Movie.GetInputState(frame));
 		}
 
+		// TODO: Change return type to indicate success/failure? This method is part of BizHawk's API for external tools/Lua.
 		public void Save(string filename)
 		{
 			if (_movieSession.Movie.NotActive())
@@ -70,7 +71,9 @@ public void Save(string filename)
 				}
 				_movieSession.Movie.Filename = filename;
 			}
-			_movieSession.Movie.Save();
+			FileWriteResult result = _movieSession.Movie.Save();
+			// unitl API is decided
+			if (result.Exception != null) throw result.Exception;
 		}
 
 		public IReadOnlyDictionary<string, string> GetHeader()

src/BizHawk.Client.Common/FileStreamWithTemp.cs

@@ -0,0 +1,130 @@
+#nullable enable
+
+using System.Diagnostics;
+using System.IO;
+
+using BizHawk.Common.StringExtensions;
+
+namespace BizHawk.Client.Common
+{
+	internal class FileStreamWithTemp : IDisposable
+	{
+		private FileStream? _stream; // is never null until this.Dispose()
+		public FileStream Stream
+		{
+			get => _stream ?? throw new ObjectDisposedException("Cannot access a disposed FileStream.");
+		}
+		public string FinalPath;
+		public string TempPath;
+
+		public bool UsingTempFile => TempPath != FinalPath;
+
+		private bool _finished = false;
+
+		private FileStreamWithTemp(string final, string temp)
+		{
+			FinalPath = final;
+			TempPath = temp;
+			_stream = new(temp, FileMode.Create, FileAccess.Write);
+		}
+
+		// There is no public constructor. This is the only way to create an instance.
+		public static FileWriteResult<FileStreamWithTemp> Create(string path)
+		{
+			string writePath = path;
+			try
+			{
+				// If the file already exists, we will write to a temporary location first and preserve the old one until we're done.
+				if (File.Exists(path))
+				{
+					writePath = path.InsertBeforeLast('.', ".saving", out bool inserted);
+					if (!inserted) writePath = $"{path}.saving";
+
+					if (File.Exists(writePath))
+					{
+						// The user should probably have dealt with this on the previously failed save.
+						// But maybe we should support plain old "try again", so let's delete it.
+						File.Delete(writePath);
+					}
+				}
+				return new(new FileStreamWithTemp(path, writePath), writePath);
+			}
+			catch (IOException ex)
+			{
+				return new(FileWriteEnum.FailedToOpen, writePath, ex);
+			}
+		}
+
+		/// <summary>
+		/// This method must be called after writing has finished and must not be called twice.
+		/// Dispose will be called regardless of the result.
+		/// </summary>
+		/// <exception cref="InvalidOperationException">If called twice.</exception>
+		public FileWriteResult CloseAndDispose()
+		{
+			// In theory it might make sense to allow the user to try again if we fail inside this method.
+			// If we implement that, it is probably best to make a static method that takes a FileWriteResult.
+			// So even then, this method should not ever be called twice.
+			if (_finished) throw new InvalidOperationException("Cannot close twice.");
+
+			_finished = true;
+			Dispose();
+
+			if (!UsingTempFile) return new(FileWriteEnum.Success, FinalPath, null);
+
+			if (File.Exists(FinalPath))
+			{
+				try
+				{
+					File.Delete(FinalPath);
+				}
+				catch (IOException ex)
+				{
+					return new(FileWriteEnum.FailedToDeleteOldFile, TempPath, ex);
+				}
+			}
+			try
+			{
+				File.Move(TempPath, FinalPath);
+			}
+			catch (IOException ex)
+			{
+				return new(FileWriteEnum.FailedToRename, TempPath, ex);
+			}
+
+			return new(FileWriteEnum.Success, FinalPath, null);
+		}
+
+		/// <summary>
+		/// Closes and deletes the file. Use if there was an error while writing.
+		/// Do not call <see cref="CloseAndDispose"/> after this.
+		/// </summary>
+		public void Abort()
+		{
+			if (_dispoed) throw new ObjectDisposedException("Cannot use a disposed file stream.");
+			_finished = true;
+			Dispose();
+
+			try
+			{
+				// Delete because the file is almost certainly useless and just clutter.
+				File.Delete(TempPath);
+			}
+			catch { /* eat? this is probably not very important */ }
+		}
+
+		private bool _dispoed;
+		public void Dispose()
+		{
+			if (_dispoed) return;
+			_dispoed = true;
+
+			_stream!.Flush(flushToDisk: true);
+			_stream.Dispose();
+			_stream = null;
+
+			// The caller should call CloseAndDispose and handle potential failure.
+			Debug.Assert(_finished, $"{nameof(FileWriteResult)} should not be disposed before calling {nameof(CloseAndDispose)}");
+		}
+	}
+}

src/BizHawk.Client.Common/FileWriteResult.cs

@@ -0,0 +1,96 @@
+#nullable enable
+
+using System.Diagnostics;
+using System.IO;
+
+namespace BizHawk.Client.Common
+{
+	public enum FileWriteEnum
+	{
+		Success,
+		FailedToOpen,
+		FailedDuringWrite,
+		FailedToDeleteOldFile,
+		FailedToRename,
+	}
+
+	/// <summary>
+	/// Provides information about the success or failure of an attempt to write to a file.
+	/// </summary>
+	public class FileWriteResult
+	{
+		public readonly FileWriteEnum Error = FileWriteEnum.Success;
+		public readonly Exception? Exception;
+		public readonly string WritePath;
+
+		public bool IsError => Error != FileWriteEnum.Success;
+
+		public FileWriteResult(FileWriteEnum error, string path, Exception? exception)
+		{
+			Error = error;
+			Exception = exception;
+			WritePath = path;
+		}
+
+		/// <summary>
+		/// Converts this instance to a different generic type.
+		/// The new instance will take the value given only if this instance has no error.
+		/// </summary>
+		/// <param name="value">The value of the new instance. Ignored if this instance has an error.</param>
+		public FileWriteResult<T> Convert<T>(T value) where T : class
+		{
+			if (Error == FileWriteEnum.Success) return new(value, WritePath);
+			else return new(this);
+		}
+
+		public FileWriteResult(FileWriteResult other) : this(other.Error, other.WritePath, other.Exception) { }
+
+		public string UserFriendlyErrorMessage()
+		{
+			switch (Error)
+			{
+				case FileWriteEnum.Success:
+					return $"The file {WritePath} was written successfully.";
+				case FileWriteEnum.FailedToOpen:
+					if (WritePath.Contains(".saving"))
+					{
+						return $"The temporary file {WritePath} already exists and could not be deleted.";
+					}
+					return $"The file {WritePath} could not be created.";
+				case FileWriteEnum.FailedDuringWrite:
+					return $"An error occurred while writing the file."; // No file name here; it should be deleted.
+				case FileWriteEnum.FailedToDeleteOldFile:
+					string fileWithoutPath = Path.GetFileName(WritePath);
+					return $"The file {WritePath} was created successfully, but the old file could not be deleted. You may manually rename the temporary file {fileWithoutPath}.";
+				case FileWriteEnum.FailedToRename:
+					fileWithoutPath = Path.GetFileName(WritePath);
+					return $"The file {WritePath} was created successfully, but could not be renamed. You may manually rename the temporary file {fileWithoutPath}.";
+				default:
+					return "unreachable";
+			}
+		}
+	}
+
+	/// <summary>
+	/// Provides information about the success or failure of an attempt to write to a file.
+	/// If successful, also provides a related object instance.
+	/// </summary>
+	public class FileWriteResult<T> : FileWriteResult where T : class // Note: "class" also means "notnull".
+	{
+		/// <summary>
+		/// Value will be null if <see cref="FileWriteResult.IsError"/> is true.
+		/// Otherwise, Value will not be null.
+		/// </summary>
+		public readonly T? Value = default;
+
+		public FileWriteResult(FileWriteEnum error, string path, Exception? exception) : base(error, path, exception) { }
+
+		public FileWriteResult(T value, string path) : base(FileWriteEnum.Success, path, null)
+		{
+			Debug.Assert(value != null, "Should not give a null value on success. Use the non-generic type if there is no value.");
+			Value = value;
+		}
+
+		public FileWriteResult(FileWriteResult other) : base(other.Error, other.WritePath, other.Exception) { }
+	}
+}